Skip to content

refactor(comments): Stage 5 — merge Comments + FeedComments into CommentThread#302

Merged
spe1020 merged 2 commits intomainfrom
refactor/comments-stage-5-comment-thread
Apr 18, 2026
Merged

refactor(comments): Stage 5 — merge Comments + FeedComments into CommentThread#302
spe1020 merged 2 commits intomainfrom
refactor/comments-stage-5-comment-thread

Conversation

@spe1020
Copy link
Copy Markdown
Contributor

@spe1020 spe1020 commented Apr 18, 2026

Summary

Final stage of Task 6. Consolidates the two remaining thread containers (Comments.svelte + FeedComments.svelte) into a single src/components/comments/CommentThread.svelte with a variant: 'recipe' | 'feed' prop, introduces a minimal Toast primitive for unified error surfacing, and removes vestigial API surface accumulated across earlier stages.

Stage 5 net: +129 lines (of which: −32 comment-system consolidation, +161 new Toast infrastructure).

Cumulative Task 6: −2,083 lines across 5 stages. Below the projected −2,300…−2,400 range because the Toast primitive came in at 161 lines rather than the ~30–40 the estimate assumed — flagged honestly in the commit body.

What Task 6 accomplished

A six-file tangled cluster (pre-Task-6: ~4,977 lines across the originals + auxiliary dead files) has been replaced by a clean architecture:

src/lib/comments/
├── subscription.ts     — createCommentSubscription factory
└── postComment.ts      — spec-grounded NIP-22/NIP-10 post flow

src/lib/commentFilters.ts       — shared isAddressableRoot predicate
src/lib/tagUtils.ts             — buildNip22CommentTags (spec-compliant)
src/lib/toast.ts                — new, general-purpose toast store

src/components/comments/
├── CommentThread.svelte  — thread container, variant: 'recipe' | 'feed'
├── CommentCard.svelte    — single comment, variant-driven sizing
└── ReplyComposer.svelte  — shared composer UI + post flow

src/components/
├── Toast.svelte          — single toast visual
└── ToastContainer.svelte — global mount point

Zero duplication. Every spec path audited against canonical NIP documents via mcp__nostr__read_nip during Stage 2.

Unification decisions made in this stage

Three previously-deferred decisions resolved:

1. Signing strategy → hardcoded 'explicit-with-timeout'

The signingStrategy prop was removed from ReplyComposer. All call sites converge on one strategy. Feed's previous 'implicit' (silent failures) is gone; errors now always surface via toast.

2. Error UX → unified toast

ReplyComposer's catch block fires:

console.error('[ReplyComposer] post failed:', error);
showToast('error', "Couldn't post comment — please try again.");

The onErrorStrategy: 'alert' | 'silent' prop is removed. No more modal alert(), no more silent failure. Technical details to console; human-friendly message to the user.

3. Anon reply-button gating → unified sign-in link

CommentCard now renders a <a href="/login?redirect={pathname}" class="action-btn action-btn-text">Sign in to reply</a> for anon users — styled identically to the Reply button so the row doesn't reflow. Works on mobile (no tooltip hover dependency). Preserves return-to-thread post-login. Replaces recipe's "always-visible-but-non-functional button" / feed's "hidden-for-anon" split with one clearer affordance for both variants.

Plus the vestigial refresh prop is gone from CommentCard. It was a no-op since Stage 2 when subscription auto-delivery replaced manual refresh calls.

Architecture preserved

Subscription semantics per variant:

  • Recipe: eager subscribe on mount, no mute filter, optimistic add via sub.addLocal().
  • Feed: lazy subscribe on toggle, applyMuteFilter: true, no optimistic add.

NoteTotalComments DOM-event bridge preserved verbatim. CommentThread's feed variant emits <div class="inline-comments" data-event-id={event.id}> and registers the toggleComments CustomEvent listener via the same document.querySelector pattern used pre-Stage-5. NoteTotalComments.svelte is unchanged.

Root element differs by variant via a single top-level {#if variant === 'recipe'} block — <section id="comments-section"> with h2/h3 headings vs <div class="inline-comments" data-event-id>. Too divergent to parameterize; two honest branches produce cleaner code than forced unification.

Toast primitive

Intentionally narrow. No stacking priority, no animation library, no accessibility theatre beyond basic aria-live. One file for the store + helper, one for a single toast visual, one for the container. Mounted once in +layout.svelte grouped with the other overlay primitives.

Positioning: bottom-right on desktop; left: 1rem; right: 1rem; bottom: calc(64px + 1rem) on mobile (above the BottomNav). z-index 9999 matches MobileSearchOverlay's convention in this codebase and sits above all modals (~50–100).

Migrated call sites

In order (simplest first per migration plan):

  1. src/routes/polls/+page.svelteFeedCommentsCommentThread variant="feed" (1-line import + 1-line tag)
  2. src/components/FoodstrFeedOptimized.svelte — same
  3. src/components/Recipe/Recipe.svelteCommentsCommentThread variant="recipe" (1-line import + 1-line tag)

Each migration verified with pnpm run check before moving on.

Deleted

  • src/components/Comments.svelte (105 lines)
  • src/components/FeedComments.svelte (113 lines)

Verification

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

Zero-vestigial greps (all pass)

$ grep -rn "import Comments\|import FeedComments" src/    →  empty
$ grep -rn "onErrorStrategy"                     src/    →  empty
$ grep -rn "refresh.*NDKEvent"   src/components/comments/  →  empty

Manual browser verification (all 7 passed by author)

  • Recipe page: top-level composer posts with optimistic add
  • Feed card: NoteTotalComments toggle opens/closes thread
  • Toggle contract: comment count updates when new comments arrive
  • Mute filter: muted-user comments don't appear in feed threads
  • Anon state: "Sign in to reply" link + sign-in prompts render correctly
  • Polls page: threads render, comment post works
  • Toast: deliberate error surfaces toast that auto-dismisses

Spec re-check

postComment lib is untouched in Stage 5. CommentThread and CommentCard call <ReplyComposer> with the same parentEvent + replyTo shape Stage 4 used. The hardcoded 'explicit-with-timeout' flows into postComment({ signingStrategy: 'explicit-with-timeout' }) — identical to the recipe variant's pre-Stage-5 call. NIP-22/NIP-10 tag output is byte-equivalent to Stage 4 by construction (same library path). Browser-captured event JSONs confirm.

Test plan

  • Open a recipe page: top-level ReplyComposer renders with <Button> submit. Post a comment — appears instantly (optimistic add). Reply to an existing comment — same flow; inline composer is compact, with Cancel.
  • Open /: click the 💬 icon on a feed post — thread panel opens (via NoteTotalComments bridge). Click again — closes. Open, post a comment — appears after subscription round-trip.
  • On /polls: open a poll's comments panel, post a comment. Post a poll-kind comment (if composer supports polls — it does). Kind 1068 event has correct NIP-10 tags + client tag + poll tags.
  • Sign out. Open a recipe: top-level composer replaced by "Sign in" link (href preserves redirect). Reply button on any card replaced by "Sign in to reply" link (styled identically). Open a feed post's thread: same — composer area shows sign-in link, Reply button on each card shows sign-in link.
  • Sign back in, open the same recipe via the redirected link: returns to the comments section.
  • Mute a user whose comments were visible on /: their comments disappear from feed threads reactively (live mute filter).
  • Kill network connection, click Post on a recipe reply → toast fires with "Couldn't post comment — please try again." in the bottom-right. Auto-dismisses after 4s. Also dismissable with the × button.
  • Kill network, click Post on a feed reply → same toast UX (previously was silent failure — this is now fixed).
  • ZapModal on a comment: click the zap icon → modal opens. Close it. Toast fires above modal if an error happens inside (z-index 9999 > modal's ~50–100).

Follow-ups filed (not in Stage 5 scope)

Added to docs/dev/FOLLOWUPS.md candidates for future work:

  • Unify empty-state sign-in prompt copy across variants (recipe's empty state doesn't include the inline sign-in hint that feed's does)
  • Differentiated error messages (signing cancelled / network timeout / generic) instead of one generic toast message

Task 6 — Complete

Stage Work Δ
1 Delete 4 dead components (InlineComments tree) −1,293
2 Extract /lib/comments/ with spec-grounded NIP-22/10/89 handling +311
3 Extract ReplyComposer shared composer −870
4 Merge Comment + FeedCommentCommentCard −360
5 Merge containers → CommentThread + Toast primitive +129
Total −2,083 lines

PRs in order: #298, #299, #300, #301, this one.

🤖 Generated with Claude Code

…entThread

Final stage of Task 6. Consolidates the two thread containers
(Comments.svelte and FeedComments.svelte) into a single
src/components/comments/CommentThread.svelte with a
variant: 'recipe' | 'feed' prop, introduces a minimal Toast primitive
for unified error surfacing, and removes vestigial API surface
accumulated across earlier stages.

Unification decisions made in this stage:

1. Signing strategy: hardcoded to 'explicit-with-timeout'. The
   `signingStrategy` prop was removed from ReplyComposer since all four
   call sites now use the same strategy (feed previously used 'implicit'
   to silently swallow failures — that UX is no longer desired).

2. Error UX: ReplyComposer fires showToast('error', "Couldn't post
   comment — please try again.") on any post failure, with the
   technical error logged to console. The onErrorStrategy prop
   ('alert' | 'silent') is removed. Both variants converge on
   explicit + toast; no more modal alert(), no more silent failure.

3. Anon reply-button gating: unified — CommentCard renders a
   <a href="/login?redirect=..."> styled identically to the Reply
   button for anon users on both variants. Replaces recipe's
   "always-visible button" and feed's "hidden for anon" split with a
   clearer, always-actionable affordance that works on mobile (no
   tooltip hover dependency) and preserves return-to-thread post-login.

4. Vestigial `refresh` prop removed from CommentCard. Was a no-op
   since Stage 2 when subscription auto-delivery replaced manual
   refresh calls.

Subscription semantics preserved per variant:
- Recipe: eager subscribe on mount, no mute filter, optimistic add
  via sub.addLocal() from handlePosted.
- Feed: lazy subscribe on toggle, applyMuteFilter: true, no optimistic
  add (relies on subscription round-trip).

NoteTotalComments DOM-event bridge preserved verbatim. CommentThread's
feed variant renders <div class="inline-comments" data-event-id={...}>
as its root and registers the 'toggleComments' CustomEvent listener
via the existing document.querySelector pattern. NoteTotalComments.svelte
is unchanged.

Root element differs by variant per the Phase 2 design — a single
{#if variant === 'recipe'} block at the top of the template. Recipe
uses <section id="comments-section"> with h2/h3 headings; feed uses
<div class="inline-comments" data-event-id>. Too divergent to
parameterize cleanly; two top-level branches is the honest answer.

New Toast primitive (src/lib/toast.ts + src/components/Toast.svelte +
src/components/ToastContainer.svelte, mounted once in
src/routes/+layout.svelte). Intentionally narrow — no stacking
priority logic, no animation library, no accessibility theatre beyond
basic aria-live. Bottom-right on desktop, above BottomNav on mobile.
z-index 9999 matches MobileSearchOverlay's convention in this
codebase and sits above all modals.

Migrated call sites (in order — simplest first):
- src/routes/polls/+page.svelte:       FeedComments → CommentThread variant="feed"
- src/components/FoodstrFeedOptimized.svelte: FeedComments → CommentThread variant="feed"
- src/components/Recipe/Recipe.svelte: Comments → CommentThread variant="recipe"

Deleted:
- src/components/Comments.svelte (105 lines)
- src/components/FeedComments.svelte (113 lines)

Filed as follow-ups (FOLLOWUPS.md candidates, noted in review if
relevant): unify empty-state sign-in prompt copy across variants;
differentiated error messages (signing cancelled / network timeout /
generic).

Line-count delta:
- Comments.svelte: 105 → 0 (deleted)
- FeedComments.svelte: 113 → 0 (deleted)
- CommentCard.svelte: 561 → 556 (−5)
- ReplyComposer.svelte: 558 → 550 (−8)
- New CommentThread.svelte: 199
- New Toast primitive (toast.ts + Toast.svelte + ToastContainer.svelte): 159
- +layout.svelte: +2 lines (import + mount)
- Stage 5 net: +129 lines

Breaking this apart: comment-system consolidation itself is −32 lines
(CommentThread replaces both containers; CommentCard + ReplyComposer
shrink slightly from prop/branch removal). The net becomes positive
because the Toast primitive is 161 lines of NEW infrastructure — it
didn't exist before. The estimate assumed a thinner toast (~30–40
lines); the actual primitive is 161 because proper styling + mobile
responsiveness + aria-live + variant support take real markup. The
primitive is minimal given what it does.

CUMULATIVE TASK 6 TOTAL: −2,083 lines across 5 stages.
  Stage 1 — delete dead tree:       −1,293
  Stage 2 — extract /lib/comments:    +311
  Stage 3 — extract ReplyComposer:    −870
  Stage 4 — merge CommentCard:        −360
  Stage 5 — merge CommentThread:      +129 (of which: −32 consolidation, +161 new Toast)

TASK 6 COMPLETE. Full comment system now lives in:
  - src/lib/comments/{subscription,postComment}.ts
  - src/lib/commentFilters.ts (shared addressable-root predicate)
  - src/lib/tagUtils.ts (buildNip22CommentTags, spec-compliant)
  - src/lib/toast.ts
  - src/components/comments/{CommentThread,CommentCard,ReplyComposer}.svelte
  - src/components/{Toast,ToastContainer}.svelte

Verified:
- pnpm run check: 4 pre-existing errors / 127 warnings (unchanged from
  main; zero introduced).
- pnpm run build: passes (adapter-cloudflare successful).
- Dev server: /, /polls, /reads all 200, no runtime errors.

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: 8a2792c
Status: ✅  Deploy successful!
Preview URL: https://f55c92c2.frontend-hvd.pages.dev
Branch Preview URL: https://refactor-comments-stage-5-co.frontend-hvd.pages.dev

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 8a2792c Apr 18 2026, 03:19 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

Final stage of the Task 6 comment-system refactor: consolidates the remaining thread containers into a single CommentThread component with recipe/feed variants, and introduces a minimal global toast primitive to unify comment-post error surfacing across contexts.

Changes:

  • Replace Comments.svelte and FeedComments.svelte with src/components/comments/CommentThread.svelte (variant: 'recipe' | 'feed') and migrate call sites.
  • Introduce a lightweight toast system ($lib/toast + Toast/ToastContainer) and mount it globally in +layout.svelte.
  • Simplify ReplyComposer/CommentCard APIs by removing now-vestigial props (signingStrategy, onErrorStrategy, refresh) and standardizing error UX via toast.

Reviewed changes

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

Show a summary per file
File Description
src/routes/polls/+page.svelte Migrates polls comment UI from FeedComments to CommentThread feed variant.
src/routes/+layout.svelte Mounts ToastContainer once at the app root for global toast rendering.
src/lib/toast.ts Adds toast store + showToast/dismissToast helpers.
src/components/comments/ReplyComposer.svelte Unifies post error handling via toast and hardcodes signing strategy.
src/components/comments/CommentThread.svelte New unified thread container replacing Comments + FeedComments.
src/components/comments/CommentCard.svelte Removes refresh prop and adds anon “Sign in to reply” link behavior.
src/components/ToastContainer.svelte New global toast stream renderer.
src/components/Toast.svelte New toast UI component with dismiss action.
src/components/Recipe/Recipe.svelte Migrates recipe comments from Comments to CommentThread recipe variant.
src/components/FoodstrFeedOptimized.svelte Migrates feed comments from FeedComments to CommentThread feed variant.
src/components/FeedComments.svelte Deleted (replaced by CommentThread).
src/components/Comments.svelte Deleted (replaced by CommentThread).
package.json Version bump.

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

}

// Anon users see a "Sign in to reply" link styled as the Reply button.
// Preserves redirect-to-thread post-login.
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.

loginRedirectHref is assigned via $: but is not declared anywhere in the <script lang="ts">. This will cause a TypeScript error / implicit global. Declare it (e.g. let loginRedirectHref = '') before the reactive assignment.

Suggested change
// Preserves redirect-to-thread post-login.
// Preserves redirect-to-thread post-login.
let loginRedirectHref = '';

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.

Fixed in 8a2792c. Added let loginRedirectHref = '' before the $: assignment. svelte-check passed clean on this file without the explicit declaration (Svelte 4 auto-declares reactive-label targets), but the explicit form is stylistically cleaner and forward-compatible with Svelte 5's stricter rules.

Comment thread src/components/ToastContainer.svelte Outdated
import Toast from './Toast.svelte';
</script>

<div class="toast-container" aria-live="polite">
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.

ToastContainer sets aria-live="polite", but each Toast also sets its own aria-live (and role). Nested live regions can lead to duplicate or inconsistent announcements in some screen readers. Prefer a single live-region strategy (either keep it on the container and remove per-toast aria-live, or drop it from the container and let each toast control its own).

Suggested change
<div class="toast-container" aria-live="polite">
<div class="toast-container">

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.

Real concern — nested live regions are unreliable across screen readers. Fixed in 8a2792c by dropping aria-live from the container and keeping it on each <Toast>. That way per-variant urgency still works (errors get assertive, info/success get polite) without the nesting ambiguity. Added an explanatory comment on the container so nobody re-adds the attribute.

// Optimistic add for recipe; feed relies on subscription round-trip.
if (variant === 'recipe') sub?.addLocal(posted);
}

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.

loginRedirectHref is assigned in a reactive statement but never declared (let/const). In a <script lang="ts"> this will fail type-checking (and can become an implicit global). Declare it (e.g. let loginRedirectHref = '') before the $: assignment.

Suggested change
let loginRedirectHref = '';

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.

Same fix — declared let loginRedirectHref = '' before the $: in 8a2792c. Applied to both this file and CommentCard.svelte (which had the identical pattern).

…plicit declarations

Three findings from the PR #302 audit:

1. CommentCard.svelte:252 and CommentThread.svelte:94 — `loginRedirectHref`
   was assigned via `$:` without a prior `let` declaration. Svelte 4
   auto-declares reactive-label targets (svelte-check passed clean on
   these files), but the explicit declaration is stylistically cleaner
   and forward-compatible with Svelte 5's stricter reactivity rules.
   Added `let loginRedirectHref = ''` before each `$:` assignment.

2. ToastContainer.svelte:16 — dropped `aria-live="polite"` from the
   container. Each <Toast> already sets aria-live per variant
   (assertive for errors, polite for info/success). Nested live
   regions produce duplicate or missed announcements across screen
   readers; keeping urgency semantics on the individual toast is the
   correct pattern. Added an explanatory comment on the container so
   future readers don't re-add the attribute.

No behavior change beyond a11y consistency.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@spe1020 spe1020 merged commit c40b2d5 into main Apr 18, 2026
2 of 3 checks passed
@spe1020 spe1020 deleted the refactor/comments-stage-5-comment-thread branch April 18, 2026 15:17
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