Skip to content

refactor(comments): Stage 2 — extract /lib/comments/ foundation#299

Merged
spe1020 merged 2 commits intomainfrom
refactor/comments-stage-2-extract-lib
Apr 18, 2026
Merged

refactor(comments): Stage 2 — extract /lib/comments/ foundation#299
spe1020 merged 2 commits intomainfrom
refactor/comments-stage-2-extract-lib

Conversation

@spe1020
Copy link
Copy Markdown
Contributor

@spe1020 spe1020 commented Apr 18, 2026

Summary

Stage 2 of the comment-system refactor. Moves the shared subscription and post-publish machinery out of the two live thread containers into plain TypeScript modules in src/lib/comments/, keeping each container's visible behaviour (optimistic-add, mute-filter, signing strategy, alert vs silent-catch) unchanged. Stage 5 will unify the behavioural divergences once the shared foundation is in place.

Behaviour is intentionally preserved per-caller in this stage — this commit is pure extraction + one design-level bug fix. No visible UX changes.

New modules

src/lib/comments/subscription.ts (140 lines) — createCommentSubscription factory. Owns the subscription lifecycle, event dedup, chronological sort, and optional live-mute filtering. Caller holds the returned reference and checks !sub before creating a new one; nulling the reference in onDestroy is what drives re-subscription on reconnect. Replaces the pre-existing subscribed-flag-never-reset bug in both Comments.svelte and FeedComments.svelte by design — the flag no longer exists.

src/lib/comments/postComment.ts (232 lines) — postComment() + PostCommentError. Delegates tag-building to the existing spec-compliant buildNip22CommentTags utility. Adds:

  • Kind detection (addressable parent → kind 1111 NIP-22; regular → kind 1 NIP-10; replies to kind-1111 comments stay kind 1111)
  • Extra-tag merging (dedupes @-mention p-tags)
  • NIP-89 client-tag append
  • Signing-strategy routing ('explicit-with-timeout' for Comments.svelte, 'implicit' for FeedComments.svelte)
  • Typed errors via PostCommentError with code: 'sign-failed' | 'publish-timeout' | 'publish-failed' | 'invalid-parent'. Optional signedEvent field carries the signed-but-unpublished event on publish-timeout / publish-failed so future callers can implement partial-success recovery; Stage 2 callers ignore it.

Consumer migrations (script-only; no visible behavior change)

src/components/Comments.svelte (recipe page): uses createCommentSubscription({ closeOnEose: false, applyMuteFilter: false }) and postCommentLib({ signingStrategy: 'explicit-with-timeout' }). Optimistic add preserved via sub.addLocal(posted). alert() error UI preserved (Stage 5 replaces with inline toast).

src/components/FeedComments.svelte (feed + polls): uses createCommentSubscription({ closeOnEose: false, applyMuteFilter: true }) with lazy gating ($ndk && !sub && showComments) preserved. postCommentLib({ signingStrategy: 'implicit' }). Silent catch preserved. Does not call addLocal() — feed context relies on subscription round-trip. The .inline-comments class + data-event-id DOM contract is unchanged so NoteTotalComments' toggle bridge keeps working.

Spec-grounded fixes to src/lib/tagUtils.ts

  • Widened addressable-event check from kind !== 30023 to the canonical NIP-01 range kind >= 30000 && kind < 40000. Zero visible effect today (30023 is the only addressable root this app publishes for), prevents regressions when future addressable kinds (e.g., 30018 marketplace listings) get comment UI.
  • Added a TODO at the NIP-10 reply p-tag block documenting that spec says "should contain all of E's p tags as well as the pubkey"; current behaviour only forwards root + parent pubkeys. Not fixing in Stage 2 — widening notifications is a visible behaviour change that belongs in its own commit with a changelog note. Captured in docs/dev/FOLLOWUPS.md.

MCP-grounded design

Every NIP-related tag-structure decision in this stage cites the canonical spec. mcp__nostr__read_nip was consulted for:

  • NIP-22 (kind 1111 comments on addressable events) — uppercase/lowercase tag semantics, required tags, reply-to-comment structure.
  • NIP-10 (kind 1 threaded replies) — e-tag markers, p-tag propagation rule.
  • NIP-89 (client tag) — format + placement + privacy consent guidance.

The existing buildNip22CommentTags was reviewed against these specs and found spec-compliant for the cases this app publishes — reused verbatim rather than reimplemented. Divergences between the pre-existing four components that looked like style choices were actually spec-compliance differences (e.g., P vs p root-author derivation); the consolidated call path now uses the spec-aligned approach uniformly.

Verification

Check Result
pnpm run check 4 pre-existing errors / 129 warnings — unchanged from main. Zero introduced.
pnpm run build passes (44.41s; adapter-cloudflare successful).
Dev server HTTP sanity — /, /polls, /reads all 200, no runtime errors.

Spec-compliance manual verification (5/5 passed in author's browser session)

  1. Recipe page — existing comments render, new comment posts via optimistic add. ✅
  2. / feedNoteTotalComments toggle opens/closes FeedComments panel; comments render and post. ✅
  3. /polls — poll comments render and post. ✅
  4. Event JSON spec check — published recipe comment's tag structure verified against the canonical NIP-22 kind-1111-on-addressable example (uppercase A / K / P root scope with a / e / k / p parent scope; client tag appended). Published feed comment verified against the NIP-10 marked-e-tag example (single ["e", id, "", "root"] plus p tag plus client tag). Event samples captured in the author's browser; available on request or will be attached as a PR comment. ✅
  5. Offline/reconnect — re-subscription works correctly because the subscribed-flag-never-reset bug is removed by design. ✅

Line-count delta

  • Comments.svelte: 429 → 387 (−42)
  • FeedComments.svelte: 418 → 390 (−28)
  • tagUtils.ts: +9 (range-widen doc + TODO note)
  • New: subscription.ts (140) + postComment.ts (232)
  • Net: +311 lines, mostly JSDoc in the new lib modules that documents the Stage 3–5 consumer contract. Pure-code delta is closer to +80 lines.

Cumulative for Task 6 across Stage 1 (−1,293) + Stage 2 (+311) = −982 lines. Stages 3–5 will pay this back and more.

Intentionally not touched (deferred to later stages)

  • Comment.svelte, FeedComment.svelte — Stage 4 (CommentCard merge)
  • Signing strategy unification + toast UI — Stage 5
  • ReplyComposer extraction — Stage 3

Follow-ups

Six items captured in docs/dev/FOLLOWUPS.md, new file in this commit:

  1. NoteTotalComments count inflation for self-posted comments
  2. NIP-10 p-tag under-propagation
  3. NIP-10 e-tag missing optional pubkey at position 5
  4. NIP-22 relay hints empty (should use seen-on relays)
  5. Toast primitive build (Stage 5 prep)
  6. NIP-89 client-tag opt-out (privacy SHOULD)

Test plan

  • Load a recipe detail page. Verify existing comments render. Post a new comment (text only), confirm it appears immediately (optimistic add). Inspect the published event's JSON via devtools Network tab — confirm NIP-22 tag structure matches the spec example: uppercase A / K / P root-scope + lowercase a / e / k / p parent-scope + client tag last.
  • Load /. Click a feed post's 💬 icon — comments panel opens (FeedComments lazy subscription starts). Post a comment. Verify the published event's JSON: single ["e", id, "", "root"], p tag for parent author, client tag last.
  • Load /polls. Open a poll's comments panel. Verify render + post.
  • Post a comment with an @mention. Confirm p-tag for the mention is present and not duplicated.
  • Post a poll comment. Confirm kind: 1068 on the outgoing event and that tag structure still follows NIP-22 / NIP-10 rules for the parent.
  • Mute filter: on /, mute an author whose comments are visible. Panel refreshes without their comments (reactive filter).
  • Recipe page with no $ndk.signer: posting shows alert() (current behaviour; Stage 5 swaps for toast).
  • Feed with a post-publish failure (simulate by blocking outbound WS): comment silently fails (current behaviour; Stage 5 swaps for toast).

Context

Stage 2 of a 5-stage Task 6 refactor. Stage 1 (#298) deleted 1,293 lines of dead code. Stages 3–5 consume these lib modules:

  • Stage 3: extract ReplyComposer.svelte shared component.
  • Stage 4: merge Comment.svelte + FeedComment.svelteCommentCard.svelte with variant: 'recipe' | 'feed' prop. Fix postingReply reset with try/finally.
  • Stage 5: merge Comments.svelte + FeedComments.svelteCommentThread.svelte. Unified error strategy: explicit sign + inline toast.

Each stage merges independently.

🤖 Generated with Claude Code

Stage 2 of the comment-system refactor. Moves the shared subscription
and post-publish machinery out of the two live thread containers into
plain TypeScript modules in src/lib/comments/, keeping each container's
visible behaviour (optimistic-add, mute-filter, signing strategy, alert
vs silent-catch) unchanged. Stage 5 will unify the behavioural divergences
once the shared foundation is in place.

New modules:

- src/lib/comments/subscription.ts — createCommentSubscription factory.
  Owns the subscription lifecycle, event dedup, chronological sort, and
  optional live-mute filtering. Caller holds the returned reference and
  checks `!sub` before creating a new one; nulling the reference in
  onDestroy is what drives re-subscription on reconnect.
  Replaces the pre-existing `subscribed`-flag-never-reset bug in both
  Comments.svelte and FeedComments.svelte by design — the flag no longer
  exists.

- src/lib/comments/postComment.ts — postComment() + PostCommentError.
  Delegates tag-building to the existing spec-compliant
  buildNip22CommentTags utility. Adds kind detection (addressable parent
  → kind 1111 NIP-22; regular → kind 1 NIP-10; replies to kind-1111
  comments stay kind 1111), extra-tag merging (dedupes @-mention p-tags),
  NIP-89 client-tag append, and signing-strategy routing
  ('explicit-with-timeout' for Comments, 'implicit' for FeedComments).
  PostCommentError carries an optional signedEvent on publish-timeout /
  publish-failed codes so callers that want partial-success recovery can
  inspect it; Stage 2 callers ignore that field.

Consumer migrations (script-only, no visible behavior change):

- Comments.svelte: uses createCommentSubscription (closeOnEose: false,
  applyMuteFilter: false) and postCommentLib with signingStrategy
  'explicit-with-timeout'. Optimistic add preserved via sub.addLocal().
  alert() error UI preserved (Stage 5 replaces with inline toast).

- FeedComments.svelte: uses createCommentSubscription (closeOnEose: false,
  applyMuteFilter: true) with lazy gating ($ndk && !sub && showComments)
  preserved. postCommentLib with signingStrategy 'implicit'. Silent catch
  preserved. addLocal() not called — feed relies on subscription round-trip.
  The .inline-comments class + data-event-id DOM contract is unchanged
  so NoteTotalComments' toggle bridge keeps working.

Spec-grounded fixes in src/lib/tagUtils.ts:

- buildNip22CommentTags: widened the addressable-event check from
  `kind !== 30023` to the canonical NIP-01 range `kind >= 30000 &&
  kind < 40000`. Zero visible effect today (30023 is the only addressable
  root this app publishes for), prevents regressions when future
  addressable kinds (e.g., 30018 marketplace listings) get comment UI.

- Added a TODO note at the NIP-10 reply p-tag block documenting that
  spec says "should contain all of E's `p` tags as well as the pubkey";
  current behaviour only forwards root + parent pubkeys. Not fixing in
  Stage 2 — widening notifications is a visible behaviour change that
  belongs in its own commit with a changelog note. See follow-ups.

MCP-grounded design: NIP-22 (kind 1111), NIP-10 (kind 1 threaded
replies), NIP-89 (client tag) consulted via mcp__nostr__read_nip for
every tag-structure decision in this stage. The existing
buildNip22CommentTags was reviewed against the spec and found
spec-compliant for the cases this app publishes; reused verbatim rather
than reimplemented.

Verified:
- pnpm run check: 4 pre-existing errors / 129 warnings (unchanged from main)
- pnpm run build: passes
- Dev server: /, /polls, /reads all return 200 with no runtime errors

Intentionally not touched (deferred to later stages):
- Comment.svelte, FeedComment.svelte — Stage 4 (CommentCard merge)
- Signing strategy unification + toast UI — Stage 5
- ReplyComposer extraction — Stage 3

Follow-up tasks (not in Stage 2 scope):
1. NIP-10 p-tag under-propagation. Spec says reply SHOULD contain all
   of parent's p-tags plus parent.pubkey; current util forwards only
   root + parent. Fixing widens notification traffic — visible behaviour
   change that warrants its own commit + changelog note.
2. NIP-89 client-tag opt-out. Spec's privacy SHOULD: clients should
   allow users to disable the client tag. addClientTagToEvent currently
   has no opt-out path.
3. Likes subscription closeOnEose tradeoff. Comment.svelte /
   FeedComment.svelte subscribe to kind 7 with default closeOnEose:true,
   so live like counts go stale post-EOSE. Changing to false means N
   extra subscriptions per page; performance tradeoff needs measurement.

Line-count delta:
- Comments.svelte: 429 → 387 (−42)
- FeedComments.svelte: 418 → 390 (−28)
- tagUtils.ts: +9 (doc/check widening)
- new: subscription.ts (140) + postComment.ts (232)
- Net: +311 lines, mostly JSDoc in the new lib modules (Stage 5 consumer
  contract). Pure-code delta is closer to +80 lines.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 18, 2026

Deploying frontend with  Cloudflare Pages  Cloudflare Pages

Latest commit: 30659cf
Status:⚡️  Build in progress...

View logs

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 18, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
❌ Deployment failed
View logs
frontend 30659cf Apr 18 2026, 12:53 PM

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Stage 2 of the comment-system refactor: extracts shared comment subscription and publishing logic into src/lib/comments/* while keeping Comments.svelte and FeedComments.svelte behavior intentionally unchanged (optimistic add vs subscription round-trip, mute filtering, alert vs silent catch). Also widens the “addressable root” detection in buildNip22CommentTags to cover the full NIP-01 parameterized-replaceable kind range.

Changes:

  • Added createCommentSubscription() factory to centralize comment subscription lifecycle, deduping, sorting, and optional reactive mute filtering.
  • Added postComment() helper with signing strategy routing, NIP-22/NIP-10 kind derivation, extra-tag merging, and typed errors.
  • Updated buildNip22CommentTags() to treat kinds 30000–39999 as addressable roots (plus TODO note about NIP-10 p propagation).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/lib/tagUtils.ts Widens addressable-root detection and documents deferred NIP-10 behavior.
src/lib/comments/subscription.ts New shared subscription factory (dedupe/sort/mute filter/cleanup).
src/lib/comments/postComment.ts New shared posting helper (kind derivation, tag merge, NIP-89 client tag, signing/publish strategies, typed errors).
src/components/FeedComments.svelte Migrates feed comments to shared subscription + post helper; preserves lazy subscribe and silent failure behavior.
src/components/Comments.svelte Migrates recipe comments to shared subscription + post helper; preserves optimistic add and alert() errors.
package.json Version bump.
docs/dev/FOLLOWUPS.md Captures deferred follow-ups discovered during the refactor.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/lib/tagUtils.ts Outdated
// NIP-22 addressable-root structure applies when the root is an
// addressable event (parameterized replaceable, kind range 30000–39999
// per NIP-01) with a `d` tag. Everything else falls through to NIP-10.
const isAddressable = event.kind >= 30000 && event.kind < 40000;
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isAddressable is currently based only on the kind range, but the comment says the NIP-22 addressable-root structure applies when the root is addressable with a d tag and that “everything else falls through to NIP-10”. As written, kind 30000–39999 events without a d tag will go down the NIP-22 branch and hit the non‑NIP‑10 fallback at lines 251–266. Consider treating “missing d tag” as non-addressable here (fall back to the NIP-10 branch), or include dTag in the addressable predicate so the code matches the stated behavior.

Suggested change
const isAddressable = event.kind >= 30000 && event.kind < 40000;
const hasDTag = event.tags.some((tag) => tag[0] === 'd');
const isAddressable = event.kind >= 30000 && event.kind < 40000 && hasDTag;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legitimate — fixed in 30659cf. isAddressable now includes hasDTag in the predicate (adopting your suggested one-line change). Addressable-range events without a d tag fall through to NIP-10 rather than hitting the non-canonical d-less fallback inside the NIP-22 branch.

Comment on lines +115 to +124
function deriveKind(parent: NDKEvent, replyTo: NDKEvent | undefined): number {
// Addressable parent → NIP-22 comment.
const parentKind = parent.kind ?? 1;
const isAddressable = parentKind >= 30000 && parentKind < 40000;
if (isAddressable) return 1111;
// Parent is itself a NIP-22 comment → reply stays kind 1111.
if (replyTo && replyTo.kind === 1111) return 1111;
if (!replyTo && parentKind === 1111) return 1111;
// Otherwise plain NIP-10 kind-1 reply.
return 1;
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deriveKind() treats any kind 30000–39999 root as addressable and forces kind 1111. However, createCommentFilter() (used by the new subscription factory) only uses the NIP-22 #A filter for kind 30023, and otherwise subscribes to kind 1 + #e. This means comments posted here for other addressable kinds (e.g. 30018) would be kind 1111 but would not be fetched by the standard comment subscription/count paths. Suggest centralizing an “isAddressableRoot” predicate (including d tag availability) and updating both kind-derivation and filters to use it.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the filter and publish paths were inconsistent, and the inconsistency would have bitten us the first time a non-30023 addressable kind (like 30018 marketplace listings) got comment UI.

Fixed in 30659cf: extracted isAddressableRoot(event) into src/lib/commentFilters.ts, now used by both createCommentFilter (filter selection) and deriveKind (published-kind selection). Predicate includes the d-tag check per your comment #1 thread. Preserves the legacy-fallback behaviour for kind 30023 without a d tag (filter widens to kinds: [1, 1111] with #e) so older/malformed longform events still get their comments.

Comment thread src/lib/comments/postComment.ts Outdated
Comment on lines +213 to +225
const publishedRelays = relaySet
? Array.from(relaySet).map((r: any) => r.url as string)
: [];
return { event: ev, publishedRelays };
}

// 'implicit' — no explicit sign, no timeout. NDK signs internally.
try {
const relaySet = await ev.publish();
const publishedRelays = relaySet
? Array.from(relaySet).map((r: any) => r.url as string)
: [];
return { event: ev, publishedRelays };
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

publishedRelays extraction assumes ev.publish() returns a Set whose elements always have a .url property and uses any to make that work. This is brittle (and will trigger @typescript-eslint/no-explicit-any warnings) if NDK changes the return type or returns a Set of strings. Prefer typing relaySet (e.g. Set<NDKRelay>) and mapping defensively (handle string vs object) so publishedRelays is reliably string[].

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair — confirmed NDK's NDKEvent.publish() returns Promise<Set<NDKRelay>> (from node_modules/@nostr-dev-kit/ndk/dist/index.d.ts:1731) and NDKRelay has readonly url: WebSocket["url"] (line 874).

Fixed in 30659cf: typed relaySet: Set<NDKRelay> | undefined, extracted the mapping into extractRelayUrls() which defends against both object-with-url and string-element shapes so publishedRelays stays string[] across future NDK return-shape changes. No more any cast, no eslint warning.

…ublish result

Three legitimate findings from the PR #299 audit, all fixed:

1. tagUtils.ts: `isAddressable` predicate now includes `d` tag presence.
   An event in kind range 30000–39999 without a `d` tag is malformed per
   NIP-01; the previous check would route it into the NIP-22 branch's
   no-d-tag fallback that emits non-canonical lowercase e-tags. Now it
   falls through to the NIP-10 path, matching the stated comment.

2. commentFilters.ts + postComment.ts: extracted `isAddressableRoot`
   predicate, used by both `createCommentFilter` (subscription side) and
   `deriveKind` (publish side). Previously the two could disagree —
   `deriveKind` returned 1111 for any kind 30000–39999, but
   `createCommentFilter` only used the NIP-22 `#A` filter for kind 30023,
   so comments on other addressable kinds (e.g., future 30018 marketplace
   listings) would have been published as kind 1111 but not fetched by
   the standard comment subscription. Centralizing the predicate prevents
   this drift.

   Preserves the existing legacy-fallback behaviour for kind 30023 without
   a `d` tag (filter widens to kinds [1, 1111] with `#e`).

3. postComment.ts: replaced the `(r: any) => r.url as string` cast in
   publishedRelays extraction with a properly-typed `Set<NDKRelay> | undefined`
   and a defensive `extractRelayUrls` helper that handles both string and
   object-with-url entries. NDK's current type is `Promise<Set<NDKRelay>>`;
   the defensive mapping stays stable across future NDK return-shape changes.

pnpm run check: 4 pre-existing errors / 129 warnings unchanged.
pnpm run build: passes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@spe1020 spe1020 merged commit ffb1a4f into main Apr 18, 2026
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants