Skip to content

fix: resolved comment checkmark toggles the card again (fixes #268)#269

Merged
jedrazb merged 6 commits intomainfrom
fix/resolved-comment-toggle-268
Apr 16, 2026
Merged

fix: resolved comment checkmark toggles the card again (fixes #268)#269
jedrazb merged 6 commits intomainfrom
fix/resolved-comment-toggle-268

Conversation

@jedrazb
Copy link
Copy Markdown
Contributor

@jedrazb jedrazb commented Apr 16, 2026

Summary

Regression in 0.0.34: clicking the checkmark of a resolved comment in the sidebar did not re-open the comment card. Reported in #268.

Root cause

The cursor-based sidebar expansion refactor (#244) moved expansion state to the parent (DocxEditor), driven by PagedEditor.onSelectionChange. But PagedEditor.updateSelectionOverlay() fires that callback from six call sites — transactions, view-ready, font-load, a ResizeObserver on the container, a [layout] effect, and HiddenPM's selection-change path. Four of those fire even when PM state hasn't changed.

Click flow for the resolved checkmark:

  1. Click the marker — onMouseDown stops propagation, so the PM cursor does not move.
  2. onClick → toggleExpandsetExpandedSidebarItem('comment-X').
  3. Sidebar re-renders with the expanded CommentCard → container size changes.
  4. ResizeObserver fires → updateSelectionOverlayonSelectionChange(from, to) with the unchanged cursor.
  5. The handler re-runs cursor-mark detection. Cursor is outside the comment range, so cursorSidebarItem = nullsetExpandedSidebarItem(null).
  6. Card collapses back to the checkmark within a frame — looks like the click did nothing.

Fix

Dedup at the source by PM state identity in updateSelectionOverlay:

if (lastNotifiedStateRef.current !== state) {
  lastNotifiedStateRef.current = state;
  onSelectionChangeRef.current?.(from, to);
}

PM states are immutable, so reference equality is the canonical "nothing changed" signal — covers selection, doc, and stored-marks changes in one check. Overlay geometry redraw still runs; only the public callback is guarded.

Why this over a consumer-side guard in DocxEditor:

  • Every consumer of onSelectionChange benefits (toolbar sync, cursor-mark detection, user onSelectionChange prop), not just the sidebar branch.
  • Reference equality catches stored-marks-only changes (Cmd+B on empty selection) that a (from, to) compare would wrongly skip.
  • The public callback finally matches its name — it only fires on real selection / doc / marks changes.

Tests

New e2e/tests/comments-sidebar.spec.ts — 11 scenarios across three groups:

  • cursor-based expansion (3): entering/leaving a comment range, multi-comment, keyboard navigation
  • resolve / reopen regression Resolved comments are not visible on editor after resolving #268 (5): clicking the checkmark, expansion surviving multiple frames, toggle-back-to-checkmark, reopen flow, multi-resolved rendering
  • feedback-loop stability (3): no self-collapse, resize-during-expansion (the smoking gun — fails without the dedup, verified by stashing), card click doesn't move the PM cursor

Also adds data-comment-id / data-comment-resolved attributes to ResolvedCommentMarker for reliable test targeting, and registers the new suite in CLAUDE.md so future PRs touching UnifiedSidebar / sidebar/** / updateSelectionOverlay run it.

Test plan

  • bun run typecheck clean
  • All 11 comments-sidebar.spec.ts tests pass
  • resize while a resolved card is expanded does not collapse it fails on main without the dedup (verified by stashing PagedEditor.tsx and re-running)
  • CI green
  • Manual repro from Resolved comments are not visible on editor after resolving #268: add comment → resolve → click elsewhere → click checkmark → card re-expands with Reopen button visible

🤖 Generated with Claude Code

PagedEditor.updateSelectionOverlay fires from six paths — transactions,
view-ready, font-load, ResizeObserver, layout effect, HiddenPM selection
change — but the onSelectionChange callback fired unconditionally on
every one of them. Any overlay redraw re-invoked consumers with the
unchanged cursor; in the sidebar-expansion branch added by #244 that
meant the ResizeObserver fired right after the click that set
expandedSidebarItem, re-ran the cursor-mark detector against the
(unchanged) cursor at the document end, and cleared the expansion
within a frame — so the checkmark click looked like it did nothing.

Fix at the source: dedup by PM state identity. States are immutable, so
reference equality is the canonical "nothing changed" signal and covers
selection / doc / stored-marks changes in one check. Overlay geometry
redraw still runs — only the public callback is guarded.

Also adds a comprehensive e2e suite (comments-sidebar.spec.ts, 11 tests)
covering cursor-based expansion, resolve/reopen, and feedback-loop
stability. The resize-specific test fails without the dedup (verified by
stashing the fix) and passes with it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docx-editor Ready Ready Preview, Comment Apr 16, 2026 10:08am

Request Review

@jedrazb jedrazb linked an issue Apr 16, 2026 that may be closed by this pull request
Resolved comments in the sidebar should stay collapsed to the checkmark
marker unless the user clicks it. Previously, the cursor-mark detector
treated resolved and active comments the same — any time the cursor
passed through previously-commented text (editing near it, arrow keys,
text selection), the old thread re-opened in the sidebar.

Skip comments with done=true in the cursor→sidebar sync. Marker click
is now the only way to re-open a resolved thread.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lved

Clicking Resolve now immediately collapses the card to its checkmark
marker. The resolve handler updates React state only (no PM transaction),
so the cursor-based collapse path never fired — the card hung around
expanded until the user clicked somewhere in the doc. Explicitly clear
expandedSidebarItem in the handler.

This also cascades into the doc-body highlight: resolvedIdsForRender was
already excluding the currently-expanded resolved comment so its range
gets highlighted, then including all others so their yellow is hidden.
With the card now collapsing on resolve, the highlight correctly
disappears at the same instant. Clicking the checkmark re-expands the
card AND re-shows the highlight on that comment's range only.

3 new e2e tests:
- clicking Resolve immediately collapses the card
- resolved comment highlight is hidden in the doc body by default
- expanding a resolved comment re-shows the highlight on its range only

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Was doing comments.find() per mark per selection change; swap to the
already-memoized Set lookup. O(n)→O(1), and the intent reads better.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two interaction gaps:

1. Expanded cards only collapsed when the user clicked inside the PM doc
   body, because that's what moved the cursor and triggered the cursor→
   sidebar sync. Clicks in the grey area around the page didn't touch PM
   state, so the card hung around. Add a mousedown handler on the scroll
   container: if the target is not inside the pages, sidebar, or margin
   markers, clear expandedSidebarItem.

2. Toggling the sidebar off and back on preserved expandedSidebarItem
   across re-render, so a previously-expanded resolved comment came back
   as a full card instead of the checkmark default. Clear expansion on
   user-initiated toggle.

2 new e2e tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fallout from the state-identity dedup: updateSelectionOverlay no longer
re-fires onSelectionChange for overlay-only redraws, so the button's
geometry callers stopped getting called on window resize / sidebar
toggle / loading→ready. The button's `left` is computed from the page's
right edge, and the page re-centers on resize, so the button would
strand at the stale coordinate while the page moved underneath it.

Extract the position logic into a stable callback and drive it from its
own ResizeObserver + window-resize listener + zoom effect. Re-run on
state.isLoading changes so the observer gets attached after the loading
subtree swaps in (the scroll container doesn't exist on first mount).

2 new e2e tests (regression-verified by stashing the fix):
- floating button re-anchors to page right edge after window resize
- floating button tracks page right edge across multiple resize steps

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jedrazb jedrazb merged commit bcc9c6d into main Apr 16, 2026
4 checks passed
@jedrazb jedrazb deleted the fix/resolved-comment-toggle-268 branch April 16, 2026 10:19
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.

Resolved comments are not visible on editor after resolving

1 participant