Render nostr: mentions and fix line breaks in longform articles#3659
Open
alltheseas wants to merge 4 commits intodamus-io:masterfrom
Open
Render nostr: mentions and fix line breaks in longform articles#3659alltheseas wants to merge 4 commits intodamus-io:masterfrom
alltheseas wants to merge 4 commits intodamus-io:masterfrom
Conversation
Pre-process NIP-23 longform markdown to convert bare nostr: references into markdown links before MarkdownUI parses them. Resolves display names for npub/nprofile mentions via profile lookup and abbreviates note/nevent/naddr identifiers. Skips code blocks and existing links. Closes damus-io#3658 Closes damus-io#3426 Changelog-Added: Added clickable @mentions in longform articles Signed-off-by: alltheseas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughPreprocessing for longform notes now sanitizes Unicode line/paragraph separators and resolves bare Nostr NIP-27 mentions into Markdown links using profile data. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
commit one before
after
commit two before
after
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
damusTests/NoteContentViewTests.swift (1)
439-499: Good test coverage for the new mention resolution logic.The tests cover key scenarios: bare mentions, existing links (skip), code blocks (skip), multiple mentions, and display text formatting.
Consider adding tests for:
nprofile1andnaddr1mentions (to verify display text handling for these types)- Inline code with single backticks (e.g.,
`nostr:npub...`) to ensure it's also skipped📝 Suggested additional test case for inline code
`@MainActor` func testResolveNostrMentions_insideInlineCode_skipped() { let npub = test_pubkey.npub let input = "Some `nostr:\(npub)` inline" let result = LongformContent.resolveNostrMentions(in: input, profiles: test_damus_state.profiles) XCTAssertEqual(result, input, "nostr reference inside inline code should not be modified") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damusTests/NoteContentViewTests.swift` around lines 439 - 499, Add tests to cover nprofile1 and naddr1 mention types and inline code with single backticks so mention resolution skips them; specifically, add new `@MainActor` test methods (e.g., testResolveNostrMentions_nprofile1DisplayText and testResolveNostrMentions_naddr1DisplayText) that call LongformContent.resolveNostrMentions(in:profiles:) with inputs containing nostr:nprofile1... and nostr:naddr1... and assert the expected display text formatting and that links are wrapped, and add testResolveNostrMentions_insideInlineCode_skipped which passes a string containing `nostr:\(npub)` (single backticks) and asserts the result equals the input; ensure these tests use the existing fixtures (test_pubkey, test_note, test_damus_state.profiles) and follow the same assertion style as the other resolveNostrMentions tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@damusTests/NoteContentViewTests.swift`:
- Around line 439-499: Add tests to cover nprofile1 and naddr1 mention types and
inline code with single backticks so mention resolution skips them;
specifically, add new `@MainActor` test methods (e.g.,
testResolveNostrMentions_nprofile1DisplayText and
testResolveNostrMentions_naddr1DisplayText) that call
LongformContent.resolveNostrMentions(in:profiles:) with inputs containing
nostr:nprofile1... and nostr:naddr1... and assert the expected display text
formatting and that links are wrapped, and add
testResolveNostrMentions_insideInlineCode_skipped which passes a string
containing `nostr:\(npub)` (single backticks) and asserts the result equals the
input; ensure these tests use the existing fixtures (test_pubkey, test_note,
test_damus_state.profiles) and follow the same assertion style as the other
resolveNostrMentions tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68f0b121-a3e0-4f3a-a0e2-4d74babceff8
📒 Files selected for processing (2)
damus/Features/Events/Models/NoteContent.swiftdamusTests/NoteContentViewTests.swift
Signed-off-by: alltheseas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Strip U+2028 (Line Separator) and U+2029 (Paragraph Separator) during longform markdown preprocessing. These characters pass through the CommonMark parser as text but SwiftUI Text renders them as visible line breaks, causing extra spacing not intended by the author. Closes damus-io#1581 Changelog-Fixed: Fixed unexpected line breaks in longform articles Signed-off-by: alltheseas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ypes Signed-off-by: alltheseas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alltheseas
force-pushed
the
longform-nostr-mentions
branch
from
dfacd12 to
2dd5050
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
damus/Features/Events/Models/NoteContent.swift (1)
388-397:⚠️ Potential issue | 🟡 MinorAdd a doc comment for the modified initializer.
This initializer now performs multiple preprocessing steps and should be documented for behavior and ordering guarantees.
As per coding guidelines "Ensure docstring coverage for any code added or modified".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damus/Features/Events/Models/NoteContent.swift` around lines 388 - 397, Add a doc comment above the init(_ markdown: String, profiles: Profiles) initializer describing its behavior and the exact preprocessing order: that it first sanitizes unicode separators via LongformContent.sanitizeUnicodeSeparators, then resolves Nostr mentions with LongformContent.resolveNostrMentions(using profiles), then ensures images are block-level via LongformContent.ensureBlockLevelImages before parsing into BlockNode and constructing MarkdownContent and computing words with count_markdown_words; include notes about why ordering matters, what inputs are expected, and any guarantees about output (e.g., images being block-level and mentions resolved).
🧹 Nitpick comments (1)
damusTests/NoteContentViewTests.swift (1)
463-469: Add regressions for link-label mentions and markdown-special display names.Please add cases for
nostr:inside markdown link labels (should be skipped) and display names containing[]()\\(should remain well-formed after wrapping). These will guard the most fragile parsing edges.As per coding guidelines "
damusTests/**/*.swift: Add or update unit tests in damusTests/ alongside feature changes, especially when touching parsing, storage, or replay logic".Also applies to: 492-508
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@damusTests/NoteContentViewTests.swift` around lines 463 - 469, Add two unit test cases in damusTests/NoteContentViewTests.swift next to testResolveNostrMentions_existingMarkdownLink_skipped: one that asserts a markdown link label containing "nostr:..." (e.g., "[nostr:npub](https://example.com)") is not altered by LongformContent.resolveNostrMentions (should be skipped), and another that passes a display name containing markdown-special characters "[]()\\" to resolveNostrMentions and asserts the output is still well-formed (wrapping/escaping preserved) to guard parsing edges; use the same test helpers (test_pubkey, test_damus_state.profiles) and mirror naming convention like testResolveNostrMentions_markdownLabelSkipped and testResolveNostrMentions_displayNameWithSpecialChars_preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@damus/Features/Events/Models/NoteContent.swift`:
- Around line 569-582: The current check only skips matches preceded by "](",
which misses matches inside markdown link labels and allows nested links; update
the loop that iterates matches (using variables regex, matches, result, range,
excludedRanges) to also detect and skip matches that are inside a markdown link
label or URL: before wrapping a match, scan left from range.lowerBound for a '['
with no intervening ']' (indicating start of a label) and scan right from
range.upperBound for a ']' followed immediately by '(' (indicating a link
target); if either condition is true (or the existing "](" check), continue
(skip) so matches inside "[...](...)" are not wrapped. Ensure checks use safe
Range/Index operations on result and account for string boundaries.
- Around line 588-600: displayText is inserted unescaped into markdown link text
which allows characters like [, ], (, ), and \ from getDisplayName(...) to break
the link; before calling result.replaceSubrange(... with:
"[\(displayText)](\(nostrURI))") escape Markdown meta-characters (at minimum [,
], (, ), and backslash) in displayText (e.g., via a helper like
escapeMarkdownText or sanitizeDisplayName) and use the escaped string when
building the link; update all branches that assign displayText (the .npub,
.nprofile, and fallback cases) to apply the escape so replaceSubrange always
receives a safe value.
---
Outside diff comments:
In `@damus/Features/Events/Models/NoteContent.swift`:
- Around line 388-397: Add a doc comment above the init(_ markdown: String,
profiles: Profiles) initializer describing its behavior and the exact
preprocessing order: that it first sanitizes unicode separators via
LongformContent.sanitizeUnicodeSeparators, then resolves Nostr mentions with
LongformContent.resolveNostrMentions(using profiles), then ensures images are
block-level via LongformContent.ensureBlockLevelImages before parsing into
BlockNode and constructing MarkdownContent and computing words with
count_markdown_words; include notes about why ordering matters, what inputs are
expected, and any guarantees about output (e.g., images being block-level and
mentions resolved).
---
Nitpick comments:
In `@damusTests/NoteContentViewTests.swift`:
- Around line 463-469: Add two unit test cases in
damusTests/NoteContentViewTests.swift next to
testResolveNostrMentions_existingMarkdownLink_skipped: one that asserts a
markdown link label containing "nostr:..." (e.g.,
"[nostr:npub](https://example.com)") is not altered by
LongformContent.resolveNostrMentions (should be skipped), and another that
passes a display name containing markdown-special characters "[]()\\" to
resolveNostrMentions and asserts the output is still well-formed
(wrapping/escaping preserved) to guard parsing edges; use the same test helpers
(test_pubkey, test_damus_state.profiles) and mirror naming convention like
testResolveNostrMentions_markdownLabelSkipped and
testResolveNostrMentions_displayNameWithSpecialChars_preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dfe5fc9-6a6c-4185-bb70-1132159afbe7
📒 Files selected for processing (2)
damus/Features/Events/Models/NoteContent.swiftdamusTests/NoteContentViewTests.swift
Comment on lines
+569
to
+582
| let excludedRanges = findExcludedRanges(in: markdown) | ||
| let matches = regex.matches(in: markdown, options: [], range: NSRange(markdown.startIndex..., in: markdown)) | ||
|
|
||
| var result = markdown | ||
| for match in matches.reversed() { | ||
| guard let range = Range(match.range, in: result) else { continue } | ||
|
|
||
| if excludedRanges.contains(where: { $0.overlaps(range) }) { continue } | ||
|
|
||
| // Skip if already inside a markdown link URL: preceded by "](" | ||
| if range.lowerBound > result.index(result.startIndex, offsetBy: 1), | ||
| result[result.index(range.lowerBound, offsetBy: -2)..<range.lowerBound] == "](" { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Existing-link detection is too narrow and can produce nested markdown links.
Current skip logic only checks whether the match is preceded by "](", so nostr: inside markdown link labels (e.g. [nostr:npub...](https://...)) can still be wrapped and break markdown rendering.
💡 Suggested fix
static func resolveNostrMentions(in markdown: String, profiles: Profiles) -> String {
@@
- let excludedRanges = findExcludedRanges(in: markdown)
+ let excludedRanges = findExcludedRanges(in: markdown)
+ let markdownLinkRanges = findMarkdownLinkRanges(in: markdown)
@@
- if excludedRanges.contains(where: { $0.overlaps(range) }) { continue }
-
- // Skip if already inside a markdown link URL: preceded by "]("
- if range.lowerBound > result.index(result.startIndex, offsetBy: 1),
- result[result.index(range.lowerBound, offsetBy: -2)..<range.lowerBound] == "](" {
+ if excludedRanges.contains(where: { $0.overlaps(range) }) ||
+ markdownLinkRanges.contains(where: { $0.overlaps(range) }) {
continue
}
@@
}
+
+private static func findMarkdownLinkRanges(in markdown: String) -> [Range<String.Index>] {
+ let pattern = #"\[[^\]]*\]\([^)]+\)"#
+ guard let regex = try? NSRegularExpression(pattern: pattern) else { return [] }
+ let nsRange = NSRange(markdown.startIndex..., in: markdown)
+ return regex.matches(in: markdown, range: nsRange)
+ .compactMap { Range($0.range, in: markdown) }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let excludedRanges = findExcludedRanges(in: markdown) | |
| let matches = regex.matches(in: markdown, options: [], range: NSRange(markdown.startIndex..., in: markdown)) | |
| var result = markdown | |
| for match in matches.reversed() { | |
| guard let range = Range(match.range, in: result) else { continue } | |
| if excludedRanges.contains(where: { $0.overlaps(range) }) { continue } | |
| // Skip if already inside a markdown link URL: preceded by "](" | |
| if range.lowerBound > result.index(result.startIndex, offsetBy: 1), | |
| result[result.index(range.lowerBound, offsetBy: -2)..<range.lowerBound] == "](" { | |
| continue | |
| } | |
| let excludedRanges = findExcludedRanges(in: markdown) | |
| let markdownLinkRanges = findMarkdownLinkRanges(in: markdown) | |
| let matches = regex.matches(in: markdown, options: [], range: NSRange(markdown.startIndex..., in: markdown)) | |
| var result = markdown | |
| for match in matches.reversed() { | |
| guard let range = Range(match.range, in: result) else { continue } | |
| if excludedRanges.contains(where: { $0.overlaps(range) }) || | |
| markdownLinkRanges.contains(where: { $0.overlaps(range) }) { | |
| continue | |
| } | |
| } | |
| } | |
| /// Finds all ranges occupied by complete markdown links in the given string. | |
| /// This prevents wrapping `nostr:` mentions that appear in markdown link labels or URLs. | |
| /// - Parameter markdown: The string to search for markdown links | |
| /// - Returns: An array of ranges, each spanning a complete markdown link `[label](url)` | |
| private static func findMarkdownLinkRanges(in markdown: String) -> [Range<String.Index>] { | |
| let pattern = #"\[[^\]]*\]\([^)]+\)"# | |
| guard let regex = try? NSRegularExpression(pattern: pattern) else { return [] } | |
| let nsRange = NSRange(markdown.startIndex..., in: markdown) | |
| return regex.matches(in: markdown, range: nsRange) | |
| .compactMap { Range($0.range, in: markdown) } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Features/Events/Models/NoteContent.swift` around lines 569 - 582, The
current check only skips matches preceded by "](", which misses matches inside
markdown link labels and allows nested links; update the loop that iterates
matches (using variables regex, matches, result, range, excludedRanges) to also
detect and skip matches that are inside a markdown link label or URL: before
wrapping a match, scan left from range.lowerBound for a '[' with no intervening
']' (indicating start of a label) and scan right from range.upperBound for a ']'
followed immediately by '(' (indicating a link target); if either condition is
true (or the existing "](" check), continue (skip) so matches inside
"[...](...)" are not wrapped. Ensure checks use safe Range/Index operations on
result and account for string boundaries.
Comment on lines
+588
to
+600
| let displayText: String | ||
| switch decoded { | ||
| case .npub(let pk): | ||
| displayText = "@\(getDisplayName(pk: pk, profiles: profiles))" | ||
| case .nprofile(let nprofile): | ||
| displayText = "@\(getDisplayName(pk: nprofile.author, profiles: profiles))" | ||
| case .note, .nevent, .naddr: | ||
| displayText = abbrev_identifier(bech32) | ||
| default: | ||
| continue | ||
| } | ||
|
|
||
| result.replaceSubrange(range, with: "[\(displayText)](\(nostrURI))") |
Contributor
There was a problem hiding this comment.
Escape mention display text before injecting it into markdown link text.
displayText is derived from profile metadata and is inserted directly into [text](url); unescaped []()\\ can break markdown or alter rendered links.
💡 Suggested fix
- result.replaceSubrange(range, with: "[\(displayText)](\(nostrURI))")
+ let safeDisplayText = escapeMarkdownLinkText(displayText)
+ result.replaceSubrange(range, with: "[\(safeDisplayText)](\(nostrURI))")
@@
return result
}
+
+private static func escapeMarkdownLinkText(_ text: String) -> String {
+ var escaped = text.replacingOccurrences(of: "\\", with: "\\\\")
+ escaped = escaped.replacingOccurrences(of: "[", with: "\\[")
+ escaped = escaped.replacingOccurrences(of: "]", with: "\\]")
+ escaped = escaped.replacingOccurrences(of: "(", with: "\\(")
+ escaped = escaped.replacingOccurrences(of: ")", with: "\\)")
+ return escaped
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@damus/Features/Events/Models/NoteContent.swift` around lines 588 - 600,
displayText is inserted unescaped into markdown link text which allows
characters like [, ], (, ), and \ from getDisplayName(...) to break the link;
before calling result.replaceSubrange(... with: "[\(displayText)](\(nostrURI))")
escape Markdown meta-characters (at minimum [, ], (, ), and backslash) in
displayText (e.g., via a helper like escapeMarkdownText or sanitizeDisplayName)
and use the escaped string when building the link; update all branches that
assign displayText (the .npub, .nprofile, and fallback cases) to apply the
escape so replaceSubrange always receives a safe value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nostr:NIP-27 references as clickable @mention links in longform (NIP-23) articles. MarkdownUI treatsnostr:npub1...as plain text; this pre-processes the markdown to wrap them as[@DisplayName](nostr:bech32)before parsing.Checklist
Standard PR Checklist
Closes:orFixes:tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patchesTest report
Device: iPhone 17 Pro (Simulator)
iOS: 26.2
Damus:
dfacd124(this branch)Setup: Loaded longform articles containing
nostr:npub,nostr:nprofile, andnostr:notereferences, including the Nostr Compass newsletter referenced in #3426.Steps:
nevent1qqst8cujky046negxgwwm5ynqwn53t8aqjr6afd8g59nfqwxpdhylpcpzamhxue69uhhyetvv9ujuetcv9khqmr99e3k7mg8arnc9)nostr:npub1...andnostr:nprofile1...render as clickable@DisplayNamelinksnostr:note1...andnostr:nevent1...render as abbreviated clickable linksnostr:text are untouchednote16vl9z038l3pgq8prya0zjx7wja3ume37ryzuagwga0ksetfchtdqj56yc6)Results:
Other notes
PostViewTests.swiftandProfilesManagerTests.swift(TagsIterator type mismatches) that prevent test execution. These are unrelated to this PR. Unit tests forresolveNostrMentionsandsanitizeUnicodeSeparatorsare included and compile correctly.npub1...references without thenostr:prefix (non-NIP-27 compliant) are intentionally not matched, as that is an author formatting error per the NIP-27 spec.Summary by CodeRabbit