feat(ui): rebuild inline comments as a workbench island (0375) - #596
Merged
Conversation
added 10 commits
July 19, 2026 13:04
Audits all four inline comment surfaces (page, canvas, database, electron) and finds the reported jank has two distinct causes: a full-screen scrim modal doing an island's job in PageView, and an unconditional reply composer eating ~140px of a 384px popover. Also documents drifted island tokens, hand-rolled positioning with no reposition loop, and the fact that Coachmark already solves this correctly while crediting CommentPopover. Recommends a shared CommentIsland primitive with a three-region layout and Coachmark-style anchored positioning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
A deeper audit of the three inline surfaces overturns the first draft's lead finding. The full-screen NewCommentInput modal is unreachable dead code — handleCreateComment is returned by usePageComments and destructured by no consumer. More importantly, the page view's inline layer is barely wired at all: nothing listens for clicks on .bn-thread-mark, and no FloatingComposerController is mounted, so the only working path is sidebar -> popover. That popover's cramped layout is what users actually see. Also records: useCommentPopover has zero consumers (state machine reimplemented 3x with divergent timing), canvas and database render raw DIDs, a multi-thread cell silently shows only threads[0] while the badge counts all, CommentIndicator is dead code with a hardcoded hex, and a grep binary-heuristic gotcha on XNetEditor.tsx requiring grep -a. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
0373 was claimed ~3 minutes earlier by the Yjs document-history exploration (ff94f9214, 13:01:40) in the xnet-overhead-analysis worktree; this one committed at 13:04:51. Earliest commit wins per the repo's collision rule. 0374 is likewise taken by the xNet Index executable-plan exploration, so this moves to 0375. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
Adds a shared useAnchoredPosition hook (extracted from the coachmark pattern: measure, clamp, mirror, re-measure on scroll/resize) and builds CommentIsland on top of it. Three regions with exactly one scroll container, so a long thread no longer pushes Resolve out of reach, and the composer stays collapsed behind a Reply affordance until invited — recovering the vertical budget that an always-mounted textarea was consuming. Chrome matches the island recipe (rounded-2xl, hairline, island fill, shadow-pop); enter/exit come from the shared Presence vocabulary so reduced motion is honoured. Portals to body, so transformed ancestors (canvas, virtualised grids) cannot clip or mis-anchor it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
…ator (0375) Inline comment marks and the grid cell badge hardcoded amber, so they could not theme with the rest of the editor. Adds a --comment/-strong semantic pair (a commented passage is annotated, not alarming, so it is its own role rather than a reuse of --warning) and drives both surfaces from it. The dark ramp now comes from the token, retiring the hand-written .dark override on .bn-thread-mark. CommentIndicator was dead — referenced only by its own definition and two barrel re-exports — and held the worst offender, a hardcoded #f59e0b. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
…0375) Migrates CommentOverlay to CommentIsland with a virtual anchor derived from the live ResolvedPin rather than viewport coordinates captured at click time, so the island follows its pin instead of stranding where it opened. Also plumbs a people list through Canvas from the host, which fixes two regressions in one: canvas comment authors rendered as raw DIDs (the page resolved them to profile names), and the reply composer had no @mention typeahead because the prop was never passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
…mmentIsland (0375) Wires the page view's inline layer, which was the real defect behind the report: nothing listened to .bn-thread-mark, so clicking a commented passage did nothing and the thread popover could only be opened from the sidebar. A delegated document listener now opens a thread on click and peeks it on hover — delegated because ProseMirror destroys and recreates mark spans on every doc update. Deletes NewCommentInput, the centred scrim modal that dimmed the whole workbench to show one text field. It was also unreachable: nothing destructured handleCreateComment. Creation now anchors to the selection it annotates and echoes the quoted text. DatabaseView (web + electron) drops its bespoke composer — a fourth hand-rolled positioning implementation — and gains a thread switcher, so a cell carrying several threads is fully reachable instead of silently showing threads[0] while the badge counted them all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
…ent API (0375) The island look was a class string copy-pasted across FloatingFrame, SidebarIslands and MobileShell (plus inline elsewhere), which is how CommentPopover drifted off it unnoticed. Extracts ISLAND_SURFACE / ISLAND_CHROME / ISLAND_OVERLAY and points every consumer at them. CommentPopover, useCommentPopover and ThreadPicker now have zero consumers. Marked @deprecated with pointers rather than deleted, so the change stays additive; ThreadPicker's job is now done by the island header's thread switcher. Adds per-state stories covering long threads, resolved, preview, composing, overlap and viewport-edge placement. No changeset: every touched package is private and changeset-ignored (assert-coverage exits 0) — the exploration's major-bump concern was based on a wrong premise, corrected in the doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
Records what was verified live versus by construction, and two findings the audit did not anticipate: no changeset is required (every touched package is private and changeset-ignored), and BlockNote ships its own comment composer that BlockNoteView mounts by default — so page-view creation is anchored and scrim-free but still stock Mantine chrome, not the island. Logged as a follow-up rather than silently claimed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
Signed-off-by: xNet Test <test@xnet.dev>
Contributor
|
Preview removed for PR #596. |
Contributor
🖼️ UI changes in this PRComponents🆕 UI/Comments/Catalog — Island / composing (new comment)🆕 UI/Comments/Catalog — Island / long thread (20 replies)🆕 UI/Comments/Catalog — Island / overlapping threads🆕 UI/Comments/Catalog — Island / hover preview🆕 UI/Comments/Catalog — Island / resolved🆕 UI/Comments/Catalog — Island / single comment🆕 UI/Comments/Catalog — Island / near the viewport edgeScreens✏️ Home _(SSIM 0.982)_
InteractionsAuto-captured by CI · run. Informational — not a blocking check. |
The lint job runs `prettier --check` in addition to eslint; `pnpm lint` alone does not, so the local gate passed while CI failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: xNet Test <test@xnet.dev>
crs48
added a commit
that referenced
this pull request
Jul 19, 2026
…n (0376, 0377) (#597) Two linked explorations on document history and attribution. Docs only — no code, no changeset needed. ## 0376 — Two Histories, One Timeline The History tab (`time-machine`, wildcard context tool) scrubs the **node change log** only. On a page it replays the title, tags and icon, and says nothing about the prose. The plumbing is ~80% built and 0% surfaced: - `DocumentHistoryEngine` captures Yjs snapshots on every debounced persist, persisted to a real `yjs_snapshots` table. - `mergeTimelines()` already computes the unified timeline — **no caller outside its own test**. - `useTimeMachine` fetches the snapshots **only to `.length` them**; `TimeMachinePanel` prints that count as a caption. Findings: -⚠️ **Restore silently half-restores.** `restoreToFrontier` documents that document content is not restored, and `tm.restore()` inherits it — the confirm dialog promises a full restore. The frontier already carries `yjsSnapshotRef`, so the fix is small. - **No Yjs history is dropped** — the gap is the *time index*, not the data. `gc: false` everywhere, `PruningEngine` never touches Yjs, no compaction exists. But CRDT items are keyed `(client, clock)` with no wall time, so a complete item log still can't answer "what did this say Tuesday at 3pm". - Every snapshot stores a **full document copy** on top of that retained blob; eviction destroys *index entries*, not content. -⚠️ Do **not** "fix" the 5 MiB large-blob warning by enabling GC — retention is what makes scrubbing possible. Recommends one timeline, two lanes, coarse by default — and rejects a naive merge, because it would mix signed and unsigned entries. ## 0377 — Evidence-Grade Attribution The follow-on: how to get detailed attribution *in the CRDT* using the existing data model. **Almost none of it needs building.** `packages/sync/src/yjs-change.ts` fully implements Yjs updates as entries in the same signed per-node hash chain — `YJS_CHANGE_TYPE = 'yjs-update'`, `createYjsChange`, type guards, a ~350-line test suite, barrel exports. **Zero production callers.** Same for `ClientIdAttestation`, `validateClientIdOwnership`, `createPersistedDocState`, `YjsBatcher`. `Change<T>`'s own header names Yjs as a first-class payload type. Meanwhile every Yjs update is already signed in transit with author + timestamp + clientID, and all of it is discarded at `setDocumentContent`. Findings: -⚠️ **clientID is spoofable** and plan step 07's attestation is unwired — so any attribution UI shipped first would render a forgeable claim as fact. Step 07 is P0. -⚠️ **`plan03_4_1YjsSecurity/README.md` has five `[x]` boxes that track module existence, not wiring.** Worth correcting regardless of whether this work proceeds. - **`yjs_state` must stay authoritative.** 0254 states the local log is a non-authoritative cache, and its compaction ships and runs every boot — inverting authority would let compaction delete document content. The correct shape is the parallel that already exists. - **Cost is rows and bytes, not CPU** (38 µs native sign). `YjsBatcher`'s 2 s default gives ~7-15:1 envelope amplification and ~1,800 rows per writing hour against a whole-user budget of 5,000 changes/day; ~10 s with paragraph flush lands ~2-3:1. - `BatchCommit` is **forbidden** on the interactive lane (0357) — amortise by batching updates into fewer changes, not signatures. - 0330's D3 gate is half-open: compaction shipped, the growth model never happened. ## Notes - Numbered 0376/0377 — 0373/0374/0375 were all taken (0373 was renumbered to 0375 and merged as #596). - 0376 also flags that 0330's plan to wait for Yjs 14 `AttributionManager` is blocked on a beta frozen at `14.0.0-16` since 2025-12-07, while `latest` is 13.6.31. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
crs48
added a commit
that referenced
this pull request
Jul 20, 2026
… UI (0375) (#601) Follow-up to #596. Closes the gap that PR logged in its Risks section. ## The bug `BlockNoteView` renders BlockNote's **own** floating comment surfaces whenever `CommentsExtension` is registered — `FloatingComposer` (staging a new thread) and `FloatingThread` (caret inside an existing mark). So putting the cursor inside a commented passage popped up a stock Mantine card alongside the island: two comment UIs, one of them off-system. This is why the earlier audit missed it — nothing mounts those controllers *explicitly*; `BlockNoteView` does it by default. ## The fix Both controllers are gated by a single prop, so `comments={false}` turns them off: ```tsx {editor.getExtension(CommentsExtension) && props.comments !== false && ( <Suspense> <FloatingComposerController … /> <FloatingThreadController … /> </Suspense> )} ``` That also disables the composer, which was handling creation — so `CommentStateBridge` re-publishes the two signals those controllers consumed (`selectedThreadId`, `pendingComment`) to the host, and `PageView` opens `CommentIsland` from them instead. Same triggers, one UI. **The subtle part:** creation routes through the extension's `createThread`, *not* the thread store. `createThread` is what also applies the in-document `.bn-thread-mark`; persisting through the store alone would leave a comment with nothing highlighting it. `inline-thread-actions.ts` wraps this so the app never imports `@blocknote/core/comments` directly. ## Verification Driven in the app on a real page with a real thread: | | before | after | |---|---|---| | `.bn-thread` / `.mantine-Card-root` with caret in mark | 1 | **0** | | `[role="dialog"]` islands | 1 | **1** (`"Comment thread"`) | Also confirmed the toolbar's **Add comment** now opens the island's `composing` mode (`"New comment"`, zero BlockNote cards) and still produces a `.bn-thread-mark` — so the mark path survives the rerouting. Console clean. 10,823 tests pass (5 new for the action wrappers, including the no-op-when-extension-absent path); typecheck, lint, prettier and `assert-coverage` all green. ## Note on the caret-only path `selectedThreadId` fires on real editor interaction. A synthetic `selectionchange` doesn't move ProseMirror's state, so my scripted keyboard probe couldn't exercise it — but every real click into a mark opened the island, and in the synthetic case nothing rendered at all. Either way the invariant holds: the old UI never appears. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
Implements exploration 0375.
What was actually wrong
The report was "the whole inline comment is taken up by a text field and you don't see the other comments." Auditing all four surfaces turned up three separate causes, and the biggest one was not styling:
.bn-thread-markdespitecursor: pointer— the only reference todata-bn-thread-idin the repo was ascrollIntoViewtarget reached from the sidebar. Clicking a commented passage did nothing.max-h-96box, with a single scroll container for thread + composer + actions, so a long thread pushed Resolve out of reach. The sidebar had solved this months earlier (isReplying); the popover never inherited it.NewCommentInput— a centred, scrim-backed modal — encoded the wrong pattern and was unreachable:handleCreateCommentwas returned byusePageCommentsand destructured by no consumer.What changed
CommentIslandreplacesCommentPopoveracross every surface: three regions with exactly one scroll container, a composer collapsed behind a "Reply…" affordance, and chrome from the shared island recipe. Positioning comes from a newuseAnchoredPositionhook extracted fromCoachmark— the one place in the app already doing this correctly (measure, clamp, mirror, re-measure on scroll/resize), and whose docstring had been creditingCommentPopoverfor behaviour it never had.Canvas gets a virtual anchor derived from the live
ResolvedPin, so the island tracks its pin under pan and zoom instead of stranding at click-time coordinates.Correctness bugs fixed in passing
authorDisplayNamewas dropped on both).threads[0]while the badge counted them all.CommentIndicator.tsxwas dead code holding a hardcoded#f59e0b.--commenttoken pair.Cleanup
ISLAND_SURFACE/ISLAND_CHROME/ISLAND_OVERLAY; the recipe had been copy-pasted across four files, which is how the popover drifted off it unnoticed.CommentPopover,useCommentPopoverandThreadPickernow have zero consumers — deprecated with pointers rather than deleted, keeping this additive.Verification
Driven in the real app: creating a thread, clicking the mark to open the island (previously a no-op), replying, scroll-tracking (island top tracked the anchor exactly), Escape, dark mode, and the database cell flow. 10,813 tests pass; typecheck clean; 0 lint errors;
check:motion-vocabpasses.Two things I could not verify live, annotated in the doc rather than claimed: canvas pin behaviour (the seeded workspace has no pins) and multi-thread cells (the grid can't create a second thread on one cell locally) — both covered by unit tests.
One finding the audit missed:
BlockNoteViewmounts BlockNote's own comment composer by default, so page-view creation is anchored and scrim-free but still stock Mantine chrome, not the island. Logged as a follow-up in Risks — it's editor-extension work beyond this restyle.No changeset: every touched package is
private: trueand changeset-ignored (assert-coverageexits 0). The doc's "major bump" concern was based on a wrong premise, corrected in place.🤖 Generated with Claude Code