Add show-replies support to nostr-post#72
Conversation
📝 WalkthroughWalkthroughThe PR adds a new Changes
Sequence DiagramsequenceDiagram
participant Component as NostrPost<br/>Component
participant NDK as NDK/Relays
participant Profiles as Profile<br/>Cache
participant Render as Render<br/>Pipeline
Component->>Component: show-replies attr changed
Component->>Component: loadReplies() triggered
Note over Component: Request Seq Guard<br/>repliesRequestSeq++
Component->>NDK: Fetch events by `#e`=[eventId]<br/>kind=1
NDK-->>Component: Reply events[]
Component->>Component: filterDirectReplies()<br/>by e-tag match
Component->>Component: Sort by created_at<br/>Extract authors
Component->>Profiles: Fetch distinct<br/>author profiles
Profiles-->>Component: NDKUserProfile[]
Component->>Component: buildReplyItem() x N<br/>Construct ReplyItem[]
Component->>Component: Update state<br/>replyItems, repliesLoaded=true
Component->>Render: Pass replyItems +<br/>reply state
Render->>Render: renderRepliesSection()<br/>renderReplyItem()
Render-->>Component: HTML (replies + post)
Component->>Component: DOM update & re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common/utils.ts`:
- Around line 91-100: isDirectReplyToEvent currently treats multi-e replies as
direct only when an explicit "reply" marker exists, but per NIP-10 we must treat
the last unmarked e tag as the parent in legacy chains. Update
isDirectReplyToEvent to: gather e tags (as eTags), find matchingETags with
tag[1] === parentEventId, return false if none; if any matching tag has tag[3]
=== 'reply' return true; otherwise determine the last unmarked e tag among eTags
(those with no tag[3]) and return true if that last unmarked e tag is the same
tag (by position or id) as the matching tag for parentEventId; otherwise return
false. Ensure you reference and modify the isDirectReplyToEvent function and use
tag indices tag[1] and tag[3] as in the diff.
In `@src/nostr-post/nostr-post.ts`:
- Around line 169-171: The finally blocks currently use early returns which
violate no-unsafe-finally; instead, remove the return statements and wrap the
cleanup logic in a guard that inverts the condition (e.g., replace "if (seq !==
this.statsRequestSeq || this.event?.id !== eventId) { return; }" with "if (seq
=== this.statsRequestSeq && this.event?.id === eventId) { /* cleanup here */
}"), and apply the exact same pattern inside loadReplies() so that any side
effects only run when the guard passes and no return occurs inside finally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 079ccd96-7b05-476e-88ca-9d1fb0321758
📒 Files selected for processing (12)
README.mdsrc/common/utils.tssrc/nostr-post/nostr-post.tssrc/nostr-post/render.tssrc/nostr-post/reply-utils.tssrc/nostr-post/style.tsstories/Introduction.mdxstories/nostr-post/NostrPost.stories.tsxstories/nostr-post/NostrPost.testing.dynamic.stories.tsxstories/nostr-post/NostrPost.testing.valid.stories.tsxstories/nostr-post/parameters.tsstories/nostr-post/test-cases-valid.ts
| export function isDirectReplyToEvent(reply: NDKEvent, parentEventId: string): boolean { | ||
| if (!parentEventId) return false; | ||
|
|
||
| const eTags = reply.tags.filter(tag => tag[0] === 'e'); | ||
| if (eTags.length === 0) return false; | ||
|
|
||
| const matchingETags = eTags.filter(tag => tag[1] === parentEventId); | ||
| if (matchingETags.length === 0) return false; | ||
|
|
||
| return eTags.length === 1 || matchingETags.some(tag => tag[3] === 'reply'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Nostr NIP-10, if an event has multiple etags and no explicitreply marker, how should clients determine the direct parent event?
💡 Result:
In Nostr NIP-10, if an event has multiple e tags and no explicit reply marker, clients should determine the direct parent event as the last e tag in the list. This follows the deprecated positional e tags convention, maintained for backward compatibility with older events.
Citations:
- 1: https://nips.nostr.com/10
- 2: https://nostr.co.uk/nips/nip-10/
- 3: NIP-10: Let's go back to positional e tags nostr-protocol/nips#1741
Handle legacy unmarked reply chains per NIP-10 spec.
Line 100 only treats multi-e notes as direct replies when an explicit "reply" marker is present. According to Nostr NIP-10, when multiple e tags exist without markers, the last unmarked tag designates the direct parent. The current implementation will reject valid legacy replies, undercounting them in getPostStats() and hiding first-level replies in loadReplies().
🔧 Suggested fix
export function isDirectReplyToEvent(reply: NDKEvent, parentEventId: string): boolean {
if (!parentEventId) return false;
const eTags = reply.tags.filter(tag => tag[0] === 'e');
if (eTags.length === 0) return false;
- const matchingETags = eTags.filter(tag => tag[1] === parentEventId);
- if (matchingETags.length === 0) return false;
-
- return eTags.length === 1 || matchingETags.some(tag => tag[3] === 'reply');
+ const explicitReplyTag = eTags.find(tag => tag[3] === 'reply');
+ if (explicitReplyTag) {
+ return explicitReplyTag[1] === parentEventId;
+ }
+
+ if (eTags.length === 1) {
+ return eTags[0][1] === parentEventId;
+ }
+
+ const unmarkedETags = eTags.filter(tag => !tag[3]);
+ return unmarkedETags[unmarkedETags.length - 1]?.[1] === parentEventId;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/common/utils.ts` around lines 91 - 100, isDirectReplyToEvent currently
treats multi-e replies as direct only when an explicit "reply" marker exists,
but per NIP-10 we must treat the last unmarked e tag as the parent in legacy
chains. Update isDirectReplyToEvent to: gather e tags (as eTags), find
matchingETags with tag[1] === parentEventId, return false if none; if any
matching tag has tag[3] === 'reply' return true; otherwise determine the last
unmarked e tag among eTags (those with no tag[3]) and return true if that last
unmarked e tag is the same tag (by position or id) as the matching tag for
parentEventId; otherwise return false. Ensure you reference and modify the
isDirectReplyToEvent function and use tag indices tag[1] and tag[3] as in the
diff.
| if (seq !== this.statsRequestSeq || this.event?.id !== eventId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, what happens when a finallyblock executesreturn, and why do linters flag that as unsafe?
💡 Result:
When a return statement executes in a finally block in JavaScript/TypeScript, it overrides any prior return, throw, break, or continue from the try or catch blocks. The finally block always executes after try/catch, and control flow statements in finally take precedence because the engine suspends the earlier completion until finally completes. Example: function example { try { return 'try'; // Suspended } catch (e) { return 'catch'; // Suspended if reached } finally { return 'finally'; // Overrides, function returns this } } example; // 'finally' This matches ECMAScript spec behavior via Completion Records: try/catch create a pending return, but finally's return creates a new one that wins. Linters like ESLint's no-unsafe-finally (recommended config) flag this as unsafe because it leads to unexpected overrides, making code hard to reason about. finally should only handle cleanup (e.g., closing resources), not control flow. The rule disallows return/throw/break/continue in finally. TypeScript follows JS runtime behavior (compiles to JS), with type checking on suspended returns (intentional, per MSFT issues). No TS-specific linter mentioned, but ESLint works for both. This is unchanged as of ECMAScript 2026.
Citations:
- 1: https://eslint.org/docs/latest/rules/no-unsafe-finally
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
- 3: https://eslint.org/docs/rules/no-unsafe-finally
- 4: https://devdoc.net/web/developer.mozilla.org/en-US/docs/JavaScript/Guide/Statements.html
- 5: https://stackoverflow.com/questions/3837994/why-does-a-return-in-finally-override-return-value-in-try-block
- 6: https://tc39.es/ecma262/multipage/ecmascript-language-statements-and-declarations.html
🏁 Script executed:
cat -n src/nostr-post/nostr-post.ts | sed -n '160,180p'Repository: saiy2k/nostr-components
Length of output: 767
🏁 Script executed:
cat -n src/nostr-post/nostr-post.ts | sed -n '240,255p'Repository: saiy2k/nostr-components
Length of output: 617
Replace early returns in finally blocks with condition guards.
Lines 169–171 and 246–248 contain return statements inside finally blocks, triggering ESLint's no-unsafe-finally rule. In JavaScript/TypeScript, a return in finally overrides any prior return or throw, which can mask control-flow bugs and is unsafe for cleanup code.
Instead of returning early, invert the condition and execute the cleanup side effects only when the guard passes:
Suggested fix
} finally {
- if (seq !== this.statsRequestSeq || this.event?.id !== eventId) {
- return;
- }
- this.statsLoading = false;
- this.render();
+ if (seq === this.statsRequestSeq && this.event?.id === eventId) {
+ this.statsLoading = false;
+ this.render();
+ }
}Apply the same pattern in loadReplies() at lines 246–248.
📝 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.
| if (seq !== this.statsRequestSeq || this.event?.id !== eventId) { | |
| return; | |
| } | |
| if (seq === this.statsRequestSeq && this.event?.id === eventId) { | |
| this.statsLoading = false; | |
| this.render(); | |
| } |
🧰 Tools
🪛 Biome (2.4.9)
[error] 170-170: Unsafe usage of 'return'.
(lint/correctness/noUnsafeFinally)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nostr-post/nostr-post.ts` around lines 169 - 171, The finally blocks
currently use early returns which violate no-unsafe-finally; instead, remove the
return statements and wrap the cleanup logic in a guard that inverts the
condition (e.g., replace "if (seq !== this.statsRequestSeq || this.event?.id !==
eventId) { return; }" with "if (seq === this.statsRequestSeq && this.event?.id
=== eventId) { /* cleanup here */ }"), and apply the exact same pattern inside
loadReplies() so that any side effects only run when the guard passes and no
return occurs inside finally.
|
@vagxrth Here as well, might want to resolve the issues flagged by @coderabbitai.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10295 🏁 Script executed: Length of output: 2540 🏁 Script executed: Length of output: 3703
Fix 1 — NIP-10 legacy fallback in
|
|
@vagxrth Thanks a lot for the PR. Appreciate it very much. I will review this later this week. |
3 participants
Closes #8
Summary
This PR adds
show-repliessupport tonostr-poston top of the currentNostrEventComponentarchitecture.Instead of porting the old linked PR directly, this implementation adapts the feature to the current component stack and adds:
show-repliesattribute fornostr-postWhat changed
nostr-postreply supportnostr-postshow-repliestoobservedAttributesshow-repliesattributeshow-replies="true"is set initiallyDirect reply filtering
src/common/utils.ts#ereference as a visible replygetPostStats()now uses the same direct-reply logic so the visible replies list and reply count stay alignedReply rendering
src/nostr-post/reply-utils.tsFooter UI
Show Repliesis available even whenshow-statsis not enabledDocs / Storybook
show-repliesto post Storybook parametersShow Repliesstory/test casenostr-postdocs in:README.mdstories/Introduction.mdxstories/nostr-post/*Verification
Ran:
npm run buildnpm run build-storybookNotes
build/build-storybookcomplete successfully, but still print pre-existing declaration/type diagnostics from older legacy modules such asnostr-comment,nostr-dm, andnostr-live-chatshow-repliesattribute tonostr-postScreenshots
Summary by CodeRabbit
New Features
show-repliesattributenoteid,eventId, or raw hex event IDsshow-statsattribute for optional like/reply count visibilityDocumentation