PT-4584: Content zoom for the comment list and the Comments panel - #2825
Conversation
9b7e0e4 to
2a2cfd9
Compare
31bbb11 to
4e5a926
Compare
2a2cfd9 to
5ae33da
Compare
b626fd9 to
661076b
Compare
lyonsil
left a comment
There was a problem hiding this comment.
Review of the commits above pt-4575-content-zoom (#2821) at 2672ef451d6 — the PT-4584 and
PT-4634 work only, not the base branch.
27 findings. Most are attached to the line they concern; the few with no line in this diff are
collected at the end of this comment. Each carries the severity it was judged at and a short
status — checked and confirmed means the finding was verified against the code.
Several findings share one root cause and are cross-referenced to each other, so start with the
blocker and the highs; fixing the cause resolves most of the rest.
Findings that could not be attached to a line in this diff
#6 (high · checked and confirmed) src/renderer/services/web-view-content-zoom.service.ts:989 - After the platform gives up on storing a zoom level, the next Settings change silently snaps the user's zoom back.
What happens: flushMemoryWrites calls clearStoredMemoryWrites(flushing) at web-view-content-zoom.service.ts:990 after MAX_MEMORY_FLUSH_ATTEMPTS, and clearStoredMemoryWrites:930 deletes the key from pendingMemoryWrites - which is also the echo guard read at :1282. The give-up record that used to accompany it (givenUpMemoryWrites.set(key, { superseded: cachedMemory[key] })) is gone along with the map and all its uses. The pane's own definition still holds the level the user chose while the setting still holds the old one, so the next emission passes :1282, levels[areaId] !== remembered is true, and setOwnLevels plus pushContentZoom write the old value back.
Why it matters: the user's deliberate zoom is reverted with no action from them, and nothing logs it.
Fix: restore givenUpMemoryWrites and its five sites - the reset in __setContentZoomDepsForTesting, the success branch of flushMemoryWrites, this give-up branch, writeMemory, and the guard in syncSiblingsFromMemory - from git show a70a44a358e:src/renderer/services/web-view-content-zoom.service.ts. None of them conflict with the identity-stamp work. Restore the two deleted tests keeps a pane's level after the memory write was given up, when an unrelated key changes and follows another window's later level for a key it gave up on.
How this was checked: Confirmed. givenUpMemoryWrites and all its use sites are gone from HEAD (grep count 0, against 7 on origin/main), including the give-up branch of flushMemoryWrites (the base had givenUpMemoryWrites.set(key, { superseded: cachedMemory[key] }), now missing after head:990's clearStoredMemoryWrites(flushing)) and the guard pendingMemoryWrites doubles as at head:1282 (if (pendingMemoryWrites.has(key) && pendingMemoryWrites.get(key) !== remembered) return;). Since clearStoredMemoryWrites (head:930) deletes the key from pendingMemoryWrites once a flush gives up, that guard no longer protects the given-up key, so the next memory emission for it reads as real news and syncSiblingsFromMemory writes it back over the pane's un-persisted level.
Not attached to a line: no longer on a changed line; cannot be posted inline
#21 (low · checked and confirmed) extensions/src/platform-scripture-editor/src/content-zoom-markers.contract.test.ts:74 - The contract test named as the guard that both editor areas stay main no longer covers the second one.
What happens: this change replaced the footnote popover's inner <ContentZoomRoot> with <ContentZoomAreaProvider> (platform-scripture-editor.web-view.tsx:3962), but the assertion at content-zoom-markers.contract.test.ts:75 still only matches the ContentZoomRoot spelling. Mutating the source to <ContentZoomAreaProvider area="footnotes"> and running the file confirms this test still passes.
Why it matters: the mutation is caught - the sibling test at :44 goes red, because its regex requires the literal <ContentZoomAreaProvider> with nothing between the tag name and >. So this is a stale test title and a coverage gap that reads as covered, not an open regression path: :74's title describes two ContentZoomRoots from before the refactor, when there is now one of each.
Fix: add expect(source).not.toContain('<ContentZoomAreaProvider area='); beside the existing assertion and update the test's name to name both components. A probe confirms not.toContain catches both a single-line mutation and a prettier-wrapped one, because the file's own collapseWhitespace at :24-26 reduces the wrapped form to <ContentZoomAreaProvider area="footnotes" >; this matches the idiom the file already uses rather than introducing a weaker regex.
Not attached to a line: no longer on a changed line; cannot be posted inline
(AI-assisted, with my guidance)
a70a44a to
5de6a14
Compare
2672ef4 to
afa9e7e
Compare
|
🤖 Claude: Both fixed. #6 — Confirmed. #21 — Confirmed. The contract test's assertion only matched the |
d7cd8ba to
31592c6
Compare
cd7c011 to
0a5d9eb
Compare
…ed round Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TdDfiggLePYPTGCuNoYNCE
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MddYES5CZ1wg3CjnyakeXw
|
Not blocking - three claims in the PR description the branch has since outgrown. All 25 review threads are resolved: every fix verified on
(AI-assisted, with my guidance) |
cd607f7 to
ad2a5d0
Compare
lyonsil
left a comment
There was a problem hiding this comment.
A full review of this PR at 9e1e1052a2b, against the tip of pt-4575-content-zoom. It has 40 findings: 1 high, 9 medium and 30 low. Most are inline; three sit on lines this PR does not change, so they are listed in this review body.
Each comment is labeled with its severity and with checked and confirmed, which means it was verified against the code, often with a probe test or a mutation. Each one ends with a Fix: line, which names any follow-on obligations the change triggers: a // CUSTOM: note, a test, or a rebuild of dist.
Findings that could not be attached to a line in this diff
#5 (medium · checked and confirmed) extensions/src/platform-scripture-editor/src/character-marker-bar/character-marker-bar.utils.ts:82 - At 200 %, the Simple-mode character-marker bar is drawn at twice its offset from the top of the chapter, so past roughly the first screen it lands off-screen.
What happens: the bar sits inside renderEditor() within the text ContentZoomRoot (platform-scripture-editor.web-view.tsx:3909). Its position is a getBoundingClientRect() difference, in painted px (character-marker-bar.utils.ts:82-89), written back as CSS inside the zoom, which scales it again. Measured with the real overlay at zoom 2: caret line at y=270 but marker bar at y=1758.
Why it matters: this predates the PR (bb3511546ed on the base branch). It matters now because this PR zooms these pop-ups and its description reports them following the area at 200 %.
Fix: keep the bar rendered in place inside the zoomed subtree, and divide the painted-px offset computeBarTop returns by the area's live zoom factor before writing it to top in character-marker-bar-overlay.component.tsx. Read the factor from the area's --platform-content-zoom-main custom property, as ContentZoomRoot's doc comment prescribes for measurements inside an area. Add an e2e at 200 % asserting the bar sits beside the caret deep in a chapter.
How this was checked: CharacterMarkerBarOverlay wraps the tree renderEditor() returns (platform-scripture-editor.web-view.tsx:3785-3804), which renders inside <ContentZoomRoot> at 3909-3911. computeBarTop (character-marker-bar.utils.ts:76-91) takes the difference of two getBoundingClientRect() calls, already in painted px, and character-marker-bar-overlay.component.tsx:271 writes it into top (444) on a div inside the same zoomed subtree, so it is scaled a second time: at zoom 2 the offset from the chapter top is exactly doubled. None of these files is touched by this PR, and the zoom-area wrapping (bb3511546ed) is already on the base branch. No unit or e2e test exercises the bar at any zoom level.
Not attached to a line: no longer on a changed line; cannot be posted inline
#6 (medium · checked and confirmed) extensions/src/platform-scripture-editor/src/paragraph-marker-tooltip/paragraph-marker-tooltip.utils.ts:30 - At 200 %, the paragraph-marker tooltip is drawn at twice its offset from the top of the chapter, so past roughly the first screen it lands off-screen.
What happens: the tooltip sits inside renderEditor() within the text ContentZoomRoot (platform-scripture-editor.web-view.tsx:3909). Its position is a getBoundingClientRect() difference, in painted px (paragraph-marker-tooltip.utils.ts:30-38), written back as CSS inside the zoom, which scales it again.
Why it matters: this predates the PR (bb3511546ed on the base branch). It matters now because this PR zooms these pop-ups and its description reports them following the area at 200 %.
Fix: position the tooltip in unzoomed px: divide the written offsets by the area's zoom factor (read from --platform-content-zoom-<areaId>), keeping the current Tooltip-trigger structure; a virtual anchor would need the Popover + PopoverAnchor virtualRef pattern, since Radix Tooltip has no virtualRef. Add an e2e at 200 % asserting it sits beside its marker deep in a chapter.
How this was checked: ParagraphMarkerTooltipOverlay wraps editorTree (platform-scripture-editor.web-view.tsx:3802-3804) inside renderEditor(), rendered inside <ContentZoomRoot> at 3909-3911. computePosition (paragraph-marker-tooltip.utils.ts:25-39) returns a difference of two painted-px getBoundingClientRect() calls, and paragraph-marker-tooltip-overlay.component.tsx:338-339 writes it as top/left on the invisible TooltipTrigger inside the same zoomed subtree, so it is scaled again; the portaled TooltipContent then opens beside the doubled trigger position. This PR does not touch these files, and the zoom-area wrapping is on the base branch. No test exercises it at any zoom level.
Not attached to a line: no longer on a changed line; cannot be posted inline
#7 (medium · checked and confirmed) extensions/src/platform-scripture-editor/src/two-step-delete-tooltip/two-step-delete-tooltip.utils.ts:51 - At 200 %, the two-step-delete hint is drawn at twice its offset from the top of the chapter, so past roughly the first screen it lands off-screen.
What happens: the hint sits inside renderEditor() within the text ContentZoomRoot (platform-scripture-editor.web-view.tsx:3909). computeAnchorRect (51-58) returns a getBoundingClientRect() difference in painted px, written back as CSS inside the zoom, which scales it again. Measured at zoom 2: marker at y=294, hint trigger at y=1532.
Why it matters: this predates the PR (bb3511546ed on the base branch). It matters now because this PR zooms these pop-ups and its description reports them following the area at 200 %.
Fix: position the hint in unzoomed px: divide computeAnchorRect's offsets by the area's zoom factor (read from --platform-content-zoom-<areaId>), keeping the current Tooltip-trigger structure; a virtual anchor would need the Popover + PopoverAnchor virtualRef pattern, since Radix Tooltip has no virtualRef. Add an e2e at 200 % asserting it sits beside its marker deep in a chapter.
How this was checked: TwoStepDeleteTooltipOverlay wraps the editor at platform-scripture-editor.web-view.tsx:3704, inside renderEditor(), rendered inside <ContentZoomRoot> at 3909-3911. computeAnchorRect (two-step-delete-tooltip.utils.ts:51-60) diffs two painted-px getBoundingClientRect() calls with no zoom division, and two-step-delete-tooltip-overlay.component.tsx:87, 122 passes it to DestructiveKeyConfirmation, which writes it as top/left/width/height on an invisible TooltipTrigger inside the same zoomed subtree (destructive-key-confirmation.component.tsx:116-120), so it is scaled again. This PR touches none of these files; the zoom-area wrapping is on the base branch. No test exercises it at any zoom level.
Not attached to a line: no longer on a changed line; cannot be posted inline
(AI-assisted, with my guidance)
ad2a5d0 to
82f394c
Compare
9e1e105 to
489a8a4
Compare
…ed round Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TdDfiggLePYPTGCuNoYNCE
|
🤖 Claude: On the three findings in your review body (#5 character-marker bar, #6 paragraph-marker tooltip, #7 two-step-delete hint): All three confirmed — and worth saying plainly: they are on |
|
🤖 Claude: Thanks — all three are corrected in the description: "the dropdown caps height and width, the tooltip caps width only"; the hidden-tab paragraph now says a pop-up can be open while its pane is hidden and the anchor holds its last good position until the tab is shown; and the Cross-view sync bullet now describes the deferred, collapsed, instant-on-activation |
|
LGTM - Sounds like essentially all fixes from this round land in #2849, so further reviews can happen there. |
489a8a4 to
435c52f
Compare
Summary
The Notes views join the per-pane content zoom (epic PT-4575, work item PT-4584). The comment list, whether opened as its own tab or as the fixed Comments panel of Simple mode, now scales its thread cards with Ctrl/⌘ plus the mouse wheel, the chords, the tab menu's Zoom in / Zoom out / Reset zoom to default, and the zoom commands. The filter toolbar and the editing-paused notice above the list keep their size. Each project's list remembers its level separately from that project's Scripture editor, so a translator can read notes large while keeping the text at its default.
ContentZoomRootwith the same layout classes; the sticky header above it stays outside. Both web-view types (legacyCommentManager.commentListand…commentListPanel) render this component, so one edit covers the tab and the panel. Levels are remembered undernotes:<projectId>:main.scrollIntoView({ block: 'start' }), which stops with the card's top under that header — about 250 px at 150 %. The scroll now setsscroll-padding-topfrom the header's measured height on every sync, so the card lands just below it at any zoom level. The e2e's overshoot check found this once its tolerance was tightened.reloadWebViewon the same web-view id, and the view rebuilds its definition by spreading its previous saved state, zoom levels included. Before this change the new project inherited the previous project's level. The service now stamps the identity (kind:identity) its levels belong to under a core-internal state key, and on a definition update whose identity no longer matches the stamp it replaces the levels with what memory remembers for the new project, or removes them so the Settings default applies. The stamp lives exactly as long as the levels: a user-chosen level is stamped when it is committed, and a reset removes both. The same path is hardened for the edges review found: a re-point while memory has not loaded leaves the pane alone rather than dropping restored levels; a pending (debounced) level write records the identity it was chosen under, counts as the pane's own level only under that identity (so a sibling-window sync or a zoom step after a re-point never re-labels it), and is discarded, timer included, if the pane shows a different identity by the time it would commit; and a failing definition write during a re-point is logged and cannot skip the pane's CSS-variable push.Why review this
Part of the current zoom epic. It is the first view that re-points to another project without remounting, which exposed and fixes a platform gap the earlier work items only noted as a TODO; and the first view with a sticky header over a zoomed scroll area, which exposed the verse-sync overshoot.
Decisions worth a look
scroll-padding-topshare the document's unzoomed coordinate space, so onegetBoundingClientRect().heightread per sync is correct from 50 % to 200 % with no factor arithmetic. The padding is valid while the document, not the zoom root, is the scroller (the same pre-existing fact as the previous bullet). A unit test pins that the height is set and re-read per sync; the e2e checks both bounds of the card's position at 150 %.CONTENT_ZOOM_IDENTITY_STATE_KEYlives incontent-zoom.model.ts, not in the extension-facingweb-view.model.ts; nothing outside core has a reason to read it. Because the levels key is public, its TSDoc now says that a view rebuilding its definition on re-point must carry its savedstatethrough wholesale, or it loses the re-seed silently. All five in-repo views already spread the state. Publishing the stamp key, or merging stamp and levels into one value, was considered and declined: nothing outside core should read the stamp, and the documented rule covers the one way to lose it.selectThreadscroll while its tab is hidden, collapses repeat requests into the most recent one, consumes it instantly on activation, and retries once if the thread's card has not rendered yet. Hidden panes are covered by design: the view feedsuseViewVisibility()intouseBcvSyncScroll, which defers while hidden and catches up instantly on activation, so the header is never measured in adisplay: nonepane.overflow-autocontainer never becomes a scroll container (pre-existing; the web-view document scrolls instead, which is why the header is sticky and the verse sync measures it) — PT-4631, with a spike's findings. A zoomed card's portaled overlays — the "Assign user" popover, the per-comment Edit/Delete menu and the conflict-note tooltips — needed the same rule as the Scripture editor's scaled footnote popover, which turned out to be mispositioned at 200 %; that work is PT-4634 and is now part of this PR (see the section below), so it is no longer a follow-up. An intermittent, pre-existing console error inuseWebViewScrollGroupScrRef(papinot yet bound on first render, caught by the error boundary) appears in some runs and is unrelated to zoom.Pop-ups follow their pane's zoom (PT-4634)
The second work item in this PR. A pop-up opened from zoomed content used to stay at interface
scale, and the Scripture editor's footnote popover — the one view that already zoomed its pop-up —
opened about 690 px away from its caller at 200 % and was clipped at the right edge. Now every
library pop-up opened from a marked area takes that area's zoom level, stays inside the pane, and
follows its trigger while the text scrolls or reflows.
ContentZoomRootpublishes its area through React context(
ContentZoomAreaProvider/useContentZoomArea, both experimental).PopoverContent,DropdownMenuContentandTooltipContentread it and mark their Radix content element with thearea plus a pop-up flag (
data-platform-content-zoom-popup), so the zoom CSS variable reaches apop-up that Radix has portaled far outside the pane.
variable and caps its size at Radix's available space divided by that factor — necessary
because
getBoundingClientRectis zoomed under Chromium's standardizedzoomwhileoffsetWidthand computed styles are not. A capped popover scrolls its content rather thangrowing past the pane; the dropdown caps height and width, the tooltip caps width only.
when it collects areas and when it picks the corner for the zoom indicator, so a portaled pop-up
never registers as a pane of its own or moves the indicator.
editor now anchor on a re-measured virtual element (
useLivePopoverAnchor) instead of the rectthey had when they opened, so they track their caller through scrolling, reflow and a zoom change
while open. The comment anchor takes the union of the pending comment's rendered fragments and
prefers the annotation mark, which fixes a wrapped selection anchoring to its first line only.
(
.claude/rules/cross-view-sync-hidden-views.md). A pop-up can be open while its pane ishidden — the footnote popover deliberately survives Escape and an outside click — and a hidden
pane has no layout, so every measurement there reads zero. A source reports that by returning
nothing rather than a zero rect, the anchor keeps the last rect it had instead of collapsing
into the pane's corner, and the first frame after the tab is shown measures again and catches
up. Recorded as a comment at the hook and called out here for scrutiny.
Visible changes beyond the rule. The footnote editor's width lock now reads its unzoomed width
(it was measuring a zoomed rect, which fought the size cap), its minimum width yields to the cap in
a narrow pane, and its action row wraps instead of clipping. Its own internal marker menu (opened
with
\inside a note) also anchors on the caret's live rect, so at 200 % it sits beside the caretinstead of the popover's corner.
Known, unchanged, worth a reviewer's eye. The capped popover's horizontal
overflowmay clip afocus ring at the very edge of its content. The comment anchor re-measures once per positioning
frame while its popover is open.
Two facts about this machine, not the code. The extension CSS pipeline (SCSS +
@tailwindcss/postcss) silently dropped an arbitrarymin()utility class, so the footnotepopover's minimum width is an inline style; the scope of that drop is not investigated.
And
lib/platform-bible-react/distdoes not rebuild byte-for-byte here — a clean build ofunmodified source differs in four files by a handful of lines — so the committed bytes are the ones
this branch carries.
adr-pop-ups-follow-their-content-zoom-areain.context/standards/Architecture-Decisions.md.Select,ContextMenu,Menubarand dropdownsub-menus do not follow an area yet; each needs the same small change when something first opens
it from zoomed content. The author guides
(
Extension-Development-Guide.md,Component-Builder-Patterns.md) state the rule and warn that apop-up a view builds by hand gets none of the library's size caps.
Found in hands-on testing, ticketed, not fixed here
The whole stack was run on the Windows dev app on 2026-09-18 and driven by hand. Five things came
up; two are the mechanism working as designed, three are now sub-tasks of the epic. None of them is
a regression from this PR.
sends the caret back to the text) leaves Ctrl+0 zooming the text rather than the footnotes pane.
Ctrl+wheel is unaffected, since it follows the pointer.
\menu is the platform command palette, rendered outside the webview, so it stays at interface scale inside a zoomed pane. The inline marker menu this PR made
zoom-aware is the Formatted/Markers-view one. The Enter palette and the footnote editor's palette
are the same mechanism.
paranext/scripture-editors),not by this repo's
ContextMenuContent, so it does not follow the pane's zoom and cannot be fixedfrom here.
whole-frame fallback applies. Views that mark an area (the editor, the comment list) have their
level written in before first paint and do not flash.
Working as designed, recorded rather than ticketed: views that mark no zoom area ignore the chords
while still following the Settings default (adoption is per view — PT-4582, PT-4583); and the
Settings page does not scale because it is part of the application shell rather than a web view, as
the setting's own description promises.
Testing
scroll-padding-top(set from the measured height, re-read on every sync).ProjectUserAccess.xml, so no registration dependency). The content-zoom helpers (ctrlWheel, memory read, indicator, geometry) are shared ine2e-tests/fixtures/content-zoom-helpers.tswith the Scripture editor specs, and the Comments-tab helpers incomment-test-helpers.tswithcomments-tab.spec.ts:comment-list-content-zoom.spec.ts: Ctrl+wheel scales the cards (ratiotoBeCloseTo(1.1, 1), so an unscaled 1.0 fails) and not the toolbar; indicator text anddata-area;notes:<ID>:mainmemory key; the same project's editor keeps its own level and key; command reset deletes the key; BCV-sync at 150 % settles the card at the sticky header's bottom edge (within 4 px) and no further down than its own height; the closed tab is confirmed gone before reopening, and reopening restores the level; another project starts at the default.comments-panel-content-zoom.spec.ts(Simple mode): wheel scales the cards; right-click tab menu ladder Zoom in / Zoom out / Reset; memory identity; re-point to project B shows the default and writes nothing under B's key, re-point back restores A's level. That step is skipped until PT-4745 (the panel's re-point delivery is unreliable in the harness), so the platform's re-seed on re-point currently rests on the renderer service tests listed above.area" / "inside the main area" / "named area" / "a caller's own cap wins", so the unchanged output
outside an area is pinned as well as the marked-and-capped output inside one; the area context
covers nested providers and the nearest-provider-wins case; the platform/library constants are
pinned equal across the two packages; the bootstrap covers a pop-up being excluded from area
reporting and from the indicator's corner while clicks and the wheel inside it still resolve to
its area; a contract test forbids a zoom root inside the footnote popover's content, so it cannot
be zoomed twice; and the live anchor and the pending-comment geometry have their own unit tests
(including the unmeasurable-source fallback and a wrapped selection's fragment union). Each new
test was checked against a deliberately broken version of the code it covers.
content-zoom.spec.tsdrives the footnote popover (scrolled, zoomedwhile open, in a 900 px window at 200 % and 300 %, closed by Cancel), the editor's marker menu and
comment editor (including a selection that wraps onto a second line), the footnote editor's own
\marker menu beside the caret, and a toolbar pop-up at 100 % as the negative control;comment-list-content-zoom.spec.tsdrives the card's Edit/Delete menu and the assign popover at150 % and 200 %. The shared check fails on an overlap, a gap, a pop-up leaving the frame, or
content overflowing a capped box, and it measures only after the open animation has finished.
its trigger tooltip each follow the area, scale with the text and stay inside the pane. The
conflict-note tooltip has no cheap fixture, so it is covered by the shared
TooltipContentbehaviour rather than its own check.
/review-paratext(four analyzers) and an OpenCodeReview delegate pass run on the PR, every confirmed finding fixed here.🤖 Generated with Claude Code
https://claude.ai/code/session_019w3LE6ckc9mpyDmCDhKEqQ
This change is