PT-4342: dock find in simple - #2680
Conversation
b0b2632 to
e1304a7
Compare
Review — PT-4342: dock find in simple@mattgetgen — posting everything in one comment so you have the whole picture at once; inline comments on the individual lines will follow. Every finding below is anchored to a line inside this PR's diff, so each one can be inlined directly. Where the fix belongs in a file this PR doesn't touch, the anchor is the in-diff line that creates the exposure and a Fix goes in pointer names the real target. Four findings are like that (#3, #4, #8, #20) — they're consequences of making Find permanent, not defects in the lines you wrote. The mechanism you chose is right, and I want to lead with that: pinning Find into the static What the review turned up is a theme rather than a list: making this tab permanent silently opts it out of four contracts the other Column 3 tabs honor — editability-following, sync edit-blocking, project hydration, and rc-dock group registration — and the comment justifying the new Every claim was traced to code; where a plausible-sounding concern turned out not to be real, I've said so at the bottom rather than leaving it in the list. Merge-blocking1.
|
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Thank you for making this fix, Matt!
Claude summary: The mechanism you chose is right, and I want to be clear about that up front: pinning Find into the static simpleLayout and reusing openFind's existing existingId: '?' probe is the least-invention path, and I verified end-to-end that the probe fronts a hidden tab without re-entering getWebView. What the deeper passes turned up is that making this tab permanent silently opts it out of four separate contracts the other Column 3 tabs honor — editability-following, sync edit-blocking, project hydration, and group registration — and that the comment justifying the new allSettled guard is factually wrong in a way that introduces a Power-mode defect. Those are the merge-blockers. Everything from #12 down is tests, docs, and polish.
@katherinejensen00 made 31 comments.
Reviewable status: 0 of 13 files reviewed, 30 unresolved discussions (waiting on mattgetgen).
extensions/src/platform-scripture/src/find.web-view-provider.ts line 72 at r1 (raw file):
...savedWebView, title, // Mirrors the title so the tab's name is discoverable on hover once simple mode shrinks it to
- Collapsed Column 3 renders Find as the generic app logo with no label · extensions/src/platform-scripture/src/find.web-view-provider.ts:72 · Medium — platform-tab-title.component.scss:49-51 hides the label in icon-only Simple mode and .tab-menu-icon:59 hard-codes url('/assets/icon.png'). Not "no icon" — the wrong one. Adding a 4th/5th tab makes icon-only trigger at wider widths, so this gets more likely. Note there's no candidate asset yet — extensions/src/platform-scripture/assets/icons/ holds only external-link.svg.
extensions/src/platform-scripture/src/find.web-view-provider.ts line 35 at r1 (raw file):
// eslint-disable-next-line @typescript-eslint/class-methods-use-this async getWebView( savedWebView: SavedWebViewDefinition,
- The comment justifying the whole allSettled guard is factually wrong — and that premise is what introduces #1 · extensions/src/platform-scripture/src/find.web-view-provider.ts:35 · High
A rejected getWebView cannot "turn the tab into an error tab": loadWebViewTab calls retrieveWebViewContent in a fire-and-forget IIFE whose catch only logs (web-view.component.tsx:622-632), and createErrorTab is reachable only from a synchronous tab-loader throw (platform-dock-layout-storage.util.ts:110-118). The real outcome of a rejection is the blank "Unknown" placeholder — which is what allSettled produces anyway (#13). All four sibling providers use a bare await.
extensions/src/platform-scripture/src/find.web-view-provider.ts line 62 at r1 (raw file):
interfaceModeResult.reason, )}`, );
- 'simple' is the unsafe fail-safe direction — it can leave an uncloseable Find in Power mode · extensions/src/platform-scripture/src/find.web-view-provider.ts:62 · High
One rejected settings.get in Power mode yields isClosable: false → getTabGroup returns TAB_GROUP_RESOURCES (platform-dock-layout-positioning.util.ts:125-131), but getGroups(true) registers only TAB_GROUP (:72-79). I confirmed rc-dock then returns defaultGroup for an unregistered name (node_modules/rc-dock/es/DockLayout.js:154). Result: a Find tab with no close button in Power mode, floatable/maximizable, no panelExtra, no moreIcon, unrecoverable in-session. The Text Collection provider documents this exact hazard as already-fixed (platform-scripture-editor/src/main.ts:1020-1030). The comment claims 'simple' is the safe direction; it's the safe direction for Simple mode only.
extensions/src/platform-scripture/src/find.web-view-provider.ts line 33 at r1 (raw file):
// getWebView doesn't use instance state but cannot be static because it implements the // IWebViewProvider interface // eslint-disable-next-line @typescript-eslint/class-methods-use-this
- Find's iframe is built twice per Simple-mode session · extensions/src/platform-scripture/src/find.web-view-provider.ts:33 · Medium — mounts with projectId: undefined, then the first Ctrl+F triggers reloadWebView, which regenerates srcNonce and re-renders (the TODO at web-view.service-host.ts:1862-1866 says so outright). Resolving the project inside the provider — as platform-scripture-editor/src/main.ts:1108-1119 does — removes the wasted build and fixes #7.
extensions/src/platform-scripture/src/find.web-view-provider.ts line 29 at r1 (raw file):
initialSearchText?: string; }
NIT 26. No savedWebView.webViewType guard · find.web-view-provider.ts:29 · Low — the one validation all four siblings perform.
extensions/src/platform-scripture/src/find.web-view-provider.ts line 50 at r1 (raw file):
if (titleResult.status === 'rejected') logger.warn(
- The title fallback is undefined for exactly the tab this PR adds · extensions/src/platform-scripture/src/find.web-view-provider.ts:50 · Medium — the static layout sets no title, so on a localization failure the tab reads "Unknown" (web-view.component.tsx:601), tooltip becomes '', and aria-label is undefined. The comment's "keeping the saved one" assumes a saved title that never exists. Passing the raw key degrades gracefully — PlatformTabTitle resolves LocalizeKey titles itself (:118-124).
extensions/src/platform-scripture/src/find.web-view-provider.ts line 66 at r1 (raw file):
// than leaving a Column 3 tab closable and draggable across columns. Mirrors the Scripture Finder // PDP's fail-safe read (platform-scripture-finder-pdpe.model.ts). const interfaceMode =
- The pre-resolved title never re-localizes on a runtime UI-language change · extensions/src/platform-scripture/src/find.web-view-provider.ts:66 · Medium — bibleTexts, commentaries and scriptureTextGrid all pass the raw localize key for precisely this reason. Self-corrected before, when the tab was transient; now it's permanent and stays in the old language until the next getWebView.
extensions/src/platform-scripture/src/find.web-view-provider.ts line 16 at r1 (raw file):
export interface FindWebViewOptions extends OpenWebViewOptions { projectId: string | undefined;
NIT 23. FindWebViewOptions declares mandatory keys the platform explicitly forbids · find.web-view-provider.ts:16 · Med-Low — web-view.model.ts:523-524, verbatim: "THIS CANNOT ADD ANY MANDATORY PROPERTIES THAT ARE NOT IN ReloadWebViewOptions…". Both paths this PR touches violate it. The sibling gets it right (ResourceViewerOptions extends OpenWebViewOptions { projectId?: string }).
extensions/src/platform-scripture/src/find.web-view-provider.ts line 70 at r1 (raw file):
return { ...savedWebView,
NIT 27. The Power-mode arm of the tooltip ternary is a no-op self-assignment · find.web-view-provider.ts:70 · Low — ...savedWebView already supplied it, and the renderer already suppresses a title-mirroring tooltip outside icon-only mode, naming this caller pattern in its comment (platform-tab-title.component.tsx:468-474).
extensions/src/platform-scripture/src/find.web-view-provider.ts line 78 at r1 (raw file):
projectId, // This is the fixed Column 3 Find tab and must always remain open in simple mode, so it's // non-closable there — closing it would leave Find with no tab to bring to the front. Power
- scrollGroupScrRef is unconditionally clobbered with undefined · extensions/src/platform-scripture/src/find.web-view-provider.ts:78 · Medium — every hydration goes through reloadWebView(type, id, {bringToFront:false}), so the spread-in saved value is destroyed. All four siblings use interfaceMode === 'simple' ? 0 : savedWebView.scrollGroupScrRef. Power mode: a user's chosen scroll group snaps back to A on every restart. Simple mode is correct only by luck (?? 0).
extensions/src/platform-scripture/src/find.web-view-provider.ts line 75 at r1 (raw file):
// icon-only — matching the Comment List convention, and the only identification this tab has // until it gets an icon. Gated on simple mode: power mode never had a tooltip here. tooltip: interfaceMode === 'simple' ? title : savedWebView.tooltip,
NIT 29. 6th copy of isClosable: interfaceMode === 'power' across 3 extensions · find.web-view-provider.ts:75 · Low — worth a ticket, not a fix here. Note the renderer already holds both halves (readCachedInterfaceMode() + FIXED_LAYOUT_WEBVIEW_GROUPS), so forcing isClosable: false centrally would delete all six and make #5 impossible.
extensions/src/platform-scripture/src/find.web-view-provider.ts line 78 at r1 (raw file):
projectId, // This is the fixed Column 3 Find tab and must always remain open in simple mode, so it's // non-closable there — closing it would leave Find with no tab to bring to the front. Power
- Hidden auto-search loop · extensions/src/platform-scripture/src/find.web-view.tsx:752 · Med-High — Find defaults to scroll group 0 and this effect's scope key includes verseRefSetting.book. Once searchTerm is non-empty, every editor book change (every chapter change under scope:'chapter') fires a full-book find job inside a display:none iframe — uninterruptible mid-scope, polled at ~10 Hz over JSON-RPC, pulling the book's USJ into an iframe that then refuses to use it (search-result.component.tsx:146 gates on isVisible). Previously the user could close the panel to stop it.
extensions/src/platform-scripture/src/main.ts line 290 at r1 (raw file):
let tabIdFromWebViewId: string | undefined; let editorScrollGroupId: FindWebViewOptions['editorScrollGroupId'];
- Find re-binds to a read-only resource that Column 3 deliberately refuses to follow · extensions/src/platform-scripture/src/main.ts:290 · High
openFind reads projectId off whatever the editor holds with no editability check, while platform-scripture-editor/src/main.ts:388-393 gates openOrUpdateRelatedPanels on isEditable and says why: "the related panels follow the active translation project, so opening a read-only published resource in the editor column must not switch them over to the resource." Find is now one of those panels and is exempt. I grepped find.web-view.tsx and all of src/find/ for readOnly|ReadOnly|isEditable — zero hits, so there's no UI-side gate eit
extensions/src/platform-scripture/src/main.ts line 302 at r1 (raw file):
if (!projectId) { logger.debug('No project! Bringing any existing Find web view to the front as-is.'); // Simple mode keeps Find as a permanent tab, so invoking Find must always land on that tab —
- The new branch can reject where the old one couldn't, and the caller floats the promise · extensions/src/platform-scripture/src/main.ts:302 · Medium — any existingId routes through findOwner, which ends throw new Error('… some windows were unreachable.') (web-view-routing.service.ts:110-113). The Ctrl+F handler is sendCommand(...) with no await and no .catch (platform-scripture-editor.web-view.tsx:1160) → unhandled rejection where the old code was a silent return undefined.
extensions/src/platform-scripture/src/main.ts line 290 at r1 (raw file):
let tabIdFromWebViewId: string | undefined; let editorScrollGroupId: FindWebViewOptions['editorScrollGroupId'];
- Replace All can write into the project the user is no longer looking at · extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts:1039 · High
openOrUpdateRelatedPanels re-points exactly four Column 3 panels; Find isn't one, and openFind is the only path that ever pushes a project in. Switch A→B in Simple mode: the other three re-point, the permanently-visible Find keeps projectId: A and A's results, and replacePdp is still bound to A. This is the deferred "Critical" — the (a) half is cosmetic, but this half is data mutation and per the PR body nobody has confirmed it was weighed or tracked.
extensions/src/platform-scripture/src/main.ts line 305 at r1 (raw file):
// doing nothing would look like a dead shortcut with the tab sitting in plain view. Bring an // existing Find web view to the front without touching its project, and don't create one if // none exists: a Find with no project has nothing to search, so there is nothing to open.
NIT 28. bringToFront: true is dead config, and the probe is now written twice · main.ts:305 · Low — already the default (web-view.service-host.ts:1597); the sibling openManageBooks call omits it. The existingId:'?' / createNewIfNotFound:false pair and the {type:'panel',…} literal each appear twice in openFind, in a file that already hoists exactly this kind of literal (const floatingLayout).
extensions/src/platform-scripture/src/find.web-view-provider.ts line 81 at r1 (raw file):
// mode allows closing/rearranging freely. isClosable: interfaceMode === 'power', content: findWebView,
- state.editorWebViewId goes stale on the permanent tab · extensions/src/platform-scripture/src/find.web-view-provider.ts:81 · Medium — main.ts:328 reloads only when projectId differs, and the new bring-to-front branch never re-runs getWebView. Replace the editor tab without changing project and Find holds a dead id, silently downgrading result clicks to scroll-group-only navigation (find.web-view.tsx:831-844) with no log.
extensions/src/platform-scripture/src/find/search-result.component.tsx line 129 at r1 (raw file):
isSelected, usjReaderWriter, cachedUsfm,
- The hidden-case dismissal covers one of three hidden-pane effects in the same file · extensions/src/platform-scripture/src/find/search-result.component.tsx:129 · Med-Low
— undocumented: the IntersectionObserver at :110 reports isIntersecting: false under display:none, so textParts bails at :146 and an expanded card shows "Loading verse text…" on activation; and the rAF at :125 never runs, so the replace countdown sits at 0% while the 1-second revert window elapses. Also the pre-existing leader line at :129 ("we should calculate the context") describes a different effect, so the durable record now points at the wrong mechanism. The dismissal itself is correct — I verified the deps and the no-op — just incomplete.
extensions/src/platform-scripture/src/main.ts line 302 at r1 (raw file):
if (!projectId) { logger.debug('No project! Bringing any existing Find web view to the front as-is.'); // Simple mode keeps Find as a permanent tab, so invoking Find must always land on that tab —
- The command's public @returns now contradicts the code · extensions/src/platform-scripture/src/types/platform-scripture.d.ts:2468 · Medium — "undefined if … the web view has no project (nothing is opened in that case)" is false. Same drift at main.ts:687 and :674. Hand-maintained and declared as the package's types (package.json:6), so it's what other extensions compile against. You updated the Storybook catalog's wording but not the actual API description.
src/renderer/components/docking/default-layout-supplement.util.ts line 117 at r1 (raw file):
const tabs = panel.tabs ?? []; const insertAt = entry.insertBeforeWebViewType ? tabs.findIndex((t) => webViewTypeOf(t) === entry.insertBeforeWebViewType)
- A typo'd insertBeforeWebViewType compiles, lints, and passes every test · src/renderer/components/docking/default-layout-supplement.util.ts:117 · Medium — the JSON arrives as a property access on an imported module (web-view.service-host.ts:830), so TypeScript does no excess-property check; "insertBeforeWebView" would silently reintroduce the exact ordering bug this PR exists to fix. '' is falsy so it also appends. The file already has the precedent for reporting anomalies (onFlagError).
src/renderer/components/docking/default-layout-supplement.util.ts line 125 at r1 (raw file):
// panel" — both mean append. panel.tabs = insertAt < 0 ? [...tabs, tab] : [...tabs.slice(0, insertAt), tab, ...tabs.slice(insertAt)];
NIT 25. insertAt === 0 would silently change which tab is active · default-layout-supplement.util.ts:125 · Med-Low — rc-dock derives activeId from tabs[0] when unset (Algorithm.js:537-539) and no simpleLayout panel sets one. Inverting insertAt < 0 to <= 0 also passes everything.
src/renderer/components/docking/default-layout-supplement.model.ts line 6 at r1 (raw file):
export interface DefaultLayoutSupplementEntry { /** * `webViewType` of an existing tab in the base layout. The supplement tab is appended to the
NIT 30. No ADR, and anchorWebViewType's TSDoc is now false · default-layout-supplement.model.ts:16 and :6 · Low — :6 still says the tab "is appended", now conditionally untrue. On the ADR: the mechanism itself predates you (#2494), so I'd only expect a short entry because the PR body explicitly asks the team to confirm this approach "versus a different way of pinning a static tab last" — that question is the ADR.
src/renderer/components/docking/simple-layout.data.test.ts line 50 at r1 (raw file):
// eslint-disable-next-line no-type-assertion/no-type-assertion const col3Panel = (columns[2] as BoxData).children[0] as PanelData; expect(col3Panel.tabs).toHaveLength(4);
- Zero test coverage of anything this PR actually does, in an area that already regressed on this branch · src/renderer/components/docking/simple-layout.data.test.ts:50 · Medium — toHaveLength(4) and toContain(...) are both order-agnostic, so reordering Column 3 so Find is second passes this file and both new supplement tests while Text Collection lands mid-panel. Nothing covers pinning, isClosable, the tooltip gate, the fail-safe, or the no-project branch. 0c1d180 shipped the tab closable and 94390f6 repaired it with no test added. Directly reusable precedent: e2e-tests/tests/enhanced-resources/scripture-text-grid.spec.ts:91-106 asserts no .dock-tab-close-btn with a positive control.
src/renderer/components/docking/platform-dock-layout-positioning.util.ts line 107 at r1 (raw file):
'legacyCommentManager.commentListPanel': TAB_GROUP_RESOURCES, 'platformScriptureEditor.scriptureTextGrid': TAB_GROUP_RESOURCES, 'platformScripture.find': TAB_GROUP_RESOURCES,
NIT 22. FIXED_LAYOUT_WEBVIEW_GROUPS' "kept in sync with two sources" is enforced by nothing · platform-dock-layout-positioning.util.ts:107 · Med-Low — a future 5th pinned tab missing from the map gets TAB_GROUP, which is registered in Simple mode, so nothing errors; the tab just silently becomes draggable across columns. simple-layout.data.test.ts already imports both modules.
src/renderer/components/docking/platform-dock-layout-positioning.util.ts line 96 at r1 (raw file):
* WebViewTypes that make up Simple mode's fixed 3-column layout, mapped to the rc-dock group each * is confined to while pinned there. Kept in sync with two sources: every webViewType hardcoded in * `simple-layout.data.ts` (all of Columns 1 and 2, and all of Column 3 except the one below), plus
- The JSDoc you rewrote broke in the same hunk · src/renderer/components/docking/platform-dock-layout-positioning.util.ts:96 · Medium — "all of Column 3 except the one below" pointed at scriptureTextGrid when it was last; you appended 'platformScripture.find' at :107, below it, and Find is in simple-layout.data.ts:101. The sentence now asserts the opposite of the truth. Name scriptureTextGrid explicitly. (Four independent passes flagged this.)
src/renderer/components/docking/simple-layout.data.ts line 97 at r1 (raw file):
}, }, {
- A startup window where the tab is closable — close it and Ctrl+F builds a 4th column · src/renderer/components/docking/simple-layout.data.ts:97 · High
The static entry carries no isClosable and createRCDockTabFromTabInfo does closable: tabInfo.isClosable ?? true (platform-dock-tab.component.tsx:55), so until the async provider round-trip lands the tab has a close button. Close it, then Ctrl+F falls through to the create branch (main.ts:333) with {type:'panel', direction:'right', targetTabId:} → a new panel beside the editor holding a now-uncloseable Find, broken for the session since saveLayout no-ops in Simple mode. This is the hole 94390f6 closed only for the post-provider state.
src/renderer/components/docking/simple-layout.data.ts line 101 at r1 (raw file):
tabType: TAB_TYPE_WEBVIEW, data: { webViewType: 'platformScripture.find',
- Find's Replace is not edit-blocked during an automatic Send/Receive · src/renderer/services/auto-sync-edit-block-driver.ts:53 · High
EDIT_BLOCKABLE_WEB_VIEW_TYPES holds only the editor and the two comment views, and grep isSyncBlocked extensions/src/platform-scripture/ returns zero hits — both verified. Making Find always-visible turns this into a one-click path: editor and comment panels go read-only, Find stays live, Replace All hits the armed SendReceiveWriteLock and fails mid-batch with (SR_EDIT_BLOCKED) and no UI explanation. Directly in tension with CLAUDE.md's Send/Receive Write Gate section.
src/renderer/components/docking/simple-layout.data.ts line 101 at r1 (raw file):
tabType: TAB_TYPE_WEBVIEW, data: { webViewType: 'platformScripture.find',
NIT 24. Bare 'platformScripture.find' in 3 core files + JSON with no drift guard · simple-layout.data.ts:101 · Med-Low — I retracted this earlier and was wrong to. SCRIPTURE_EDITOR_WEBVIEW_TYPE exists at web-view.model.ts:316 precisely as a core-side mirror because core can't import from extensions, and it's protected by a real drift-guard test (web-view.model.test.ts:5-25) whose comment spells out that rationale. So the established answer here is mirror + guard, not a bare literal. Rename findWebViewType today and Simple mode loses its Find tab with zero failing tests.
src/renderer/components/docking/simple-layout.data.ts line 101 at r1 (raw file):
tabType: TAB_TYPE_WEBVIEW, data: { webViewType: 'platformScripture.find',
- The permanent tab renders a fully interactive UI that silently does nothing · src/renderer/components/docking/simple-layout.data.ts:101 · Med-High — no projectId, so findPdp is undefined and handleStartSearch early-returns at find.web-view.tsx:461. No status, no error, no disabled state. Acknowledged and deferred; flagging that the deferral is visible to any user who clicks the tab before pressing Ctrl+F.
src/renderer/components/docking/default-layout-supplement.json line 5 at r1 (raw file):
{ "anchorWebViewType": "platformScriptureEditor.bibleTexts", "insertBeforeWebViewType": "platformScripture.find",
- The shipped Column 3 order is untestable as written · src/renderer/components/docking/default-layout-supplement.json:5 · Medium — the real JSON is vi.mock'd in the only file that imports it (web-view.service-host.test.ts:31-48), and the new tests use a synthetic find-tab. Combined with the silent append fallback, a rename moves Text Collection after Find with everything green.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 reviewed 13 files and all commit messages.
Reviewable status: all files reviewed, 30 unresolved discussions (waiting on mattgetgen).
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Thanks for making all of those changes, Matt! Just a few more things to look at and I think you'll have it.
-
isClosable: false in default-layout-supplement.json re-creates the #1 hazard in Power mode. simple-layout.data.ts is only ever loaded in Simple mode (its new TSDoc says so), but the supplement merge is mode-agnostic — loadLayout merges enabled entries into the persisted Power layout too (web-view.service-host.ts:945-960). So in Power mode the merged Text Collection tab renders non-closable, getTabGroup routes it to TAB_GROUP_RESOURCES, and getGroups(true) registers only TAB_GROUP → rc-dock's unregistered-group fallback until the provider's async response flips it back. That provider's own comment (platform-scripture-editor/src/main.ts:1020-1028) exists because this bug was already found and fixed there once, and loadLayout:945 already warns "if isClosable: false, uncloseable". Preconditions: Power mode + a persisted layout containing a platformScriptureEditor.bibleTexts panel + no grid tab with the matching id (merge dedups by exact id). Transient, but same class as the merge-blocker just fixed.
-
Same root cause: the new anomaly warning will fire spuriously in Powemode. insertBeforeWebVind' cannot resolve in aPower-mode panel, so every Power-mode load where the anchor resolves logs "not found in the panel… appending last instead" for a perfectly correct append — noise in the one channel the warning exists to keep clean. The new test validates resolution against simpleLayout only. Both this and #1 wathe same fix: make the
-
state.editorWebViewId is still stale after a Simple-mode project switthe residual half of #1o editor id, and it runs(main.ts:393) before the {type: 'replace-tab'} openWebView at main.ts:43mints the new editor welaced editor's id:handleFocusedResultChange (find.web-view.tsx:1019) skips selectRange + setAnnotation and handleOpenAtResult skips setFocus, so clicking a result moves the scroll group but no longer selects or highlights the match — until the next Ctrl+F, where the new hasEditorChanged check repairs it. Either hand the new editor id to Find after openWebView, or have Find resolve the aceditor instead of cachi
-
updateFindProject hardcodes isReadOnly: false but is public API. The precondition ("only evelation project") is anno vitest installed.
New findings this round
1. isClosable: false inon re-creates the #1hazard in Power mode. simple-layout.data.ts is only ever loaded in Simple mode (its new TSDoc says so), but the supplement merge is mode-agnostic — loadLayout merges enabled entries into the persisted Power layout too (web-view.service-host.ts:945-960). So in Power mode the merged Text Collection tab renders non-closable, getTabGroup routes it to TAB_GROUP_RESOURCES, and getGroups(true) registers only TAB_GROUP → rc-dock's unregistered-group fallback until the provider's async response flips it back. That provider's own comment (platform-scripture-editor/src/main.ts:1020-1028) exists because this bug was already found and fixed there once, and loadLayout:945 already warns "if isClosable: false, uncloseable". Preconditions: Power mode + a persisted layout containing a plats panel + no grid tabwith the matching id (merge dedups by exact id). Transient, but same class as the merge-blocker just fixed.
2. Same root cause: thee spuriously in Powermode. insertBeforeWebViewType: 'platformScripture.find' cannot resolve in a Power-mode panel, so every Power-mode load where the anchor resolves logs "not found in the panel… appending last instead" for a perfectly correct append — noise in the one channel the warning exists to keep clean. The new test validates resolution against simpleLayout only. Both this and #1 want the same fix: make the mode explicit at the merge.
-
state.editorWebViewId is still stale after a Simple-mode project switch — the residual half of #19. updateFindProject passes no editor id, and it run(main.ts:393) before thebView at main.ts:431mints the new editor web view. So Find keeps the replaced editor's id: handleFocusedResultChankips selectRange +setAnnotation and handleOpenAtResult skips setFocus, so clicking a result moves the scroll group but no longer selects or highlights the match — until the next Ctrl+F, where the new hasEditorChanged check repairs it. Either hand the new editor id to Find after openWebView, or have Find resolve the active editor instead of caching an id.
-
updateFindProject hardcodes isReadOnly: false but is public API. The precondition ("only ever called for the active translation project") is an internal comment, whileapi-shared-types for anyextension to call with published resource, which would re-enable Replace on read-only text. Document it in the .d.ts or derive editability. Related: on layout hydration projectId falls back to the saved value while isReadOnly is scrubbed to false, so a Power-mode Find restored onto a resourcs at the PDP). Not aregression, but the scrub is asymmetric — scrubbing only when the caller re-points the project would close it.
-
The PR body is stale and its "API Changes: None." section is now false. It still says "13 files" and describes none of the read-only behavior, updateFindProject, useRunWhenVisible, the icon, or the two ADRs. platformScripture.updateFindProject is a new papi-shared-types command in the package's declared types — a public API addition. This squash-merges, so the body is the commit message.
-
Still no tickets. grep of the whole new diff for PT-/TODO returns nothing: the sync-edit-block gap (a 4-step plan in a core code comment), focus-on-invoke, Escape-to-dismiss, the six-way isClosable duplication, andthe double iframe build prose. The Code StyleGuide's exception is precisely "forward-facing ticket pointers". Note the double build (#16) is now eager: updateFindProject rebuilds the Find iframe on every Simple-mode project open/switch, where before it happened once on first Ctrl+F.
Nits: getReplaceUnavailableReason(false, true, false) — three positional booleans the tests need comments to decode; an options object reads itself. useRunWhenVisible is a fully generic hook with zero Find-specific logic, yet it sits in extensions/src/platform-scripture/src/find/ while .claude/rules/cross-view-sync-hidden-views.md now advertises it repo-wide — no other extension can import it; its partner useViewVisibility lives in lib/platform-bible-react. Two comments still narrate history against the style guide ("Before this tab was permanent the user could stop that…"; "Gating it also produce. New strings areEnglish-only, which matches existing practice in that file — not a finding.
@katherinejensen00 reviewed 28 files and all commit messages, made 2 comments, and resolved 30 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on mattgetgen).
src/renderer/components/docking/simple-layout.data.ts line 1 at r1 (raw file):
import { SavedTabInfo, TAB_TYPE_WEBVIEW } from '@shared/models/docking-framework.model';
- isClosable: false in default-layout-supplement.json re-creates the #1 hazard in Power mode. simple-layout.data.ts is only ever loaded in Simple mode (its new TSDoc says so), but the supplement merge is mode-agnostic — loadLayout merges enabled entries into the persisted Power layout too (web-view.service-host.ts:945-960). So in Power mode the merged Text Collection tab renders non-closable, getTabGroup routes it to TAB_GROUP_RESOURCES, and getGroups(true) registers only TAB_GROUP → rc-dock's unregistered-group fallback until the provider's async response flips it back. That provider's own comment (platform-scripture-editor/src/main.ts:1020-1028) exists because this bug was already found and fixed there once, and loadLayout:945 already warns "if isClosable: false, uncloseable". Preconditions: Power mode + a persisted layout containing a platformScriptureEditor.bibleTexts panel + no grid tab with the matching id (merge dedups by exact id). Transient, but same class as the merge-blocker just fixed.
mattgetgen
left a comment
mattgetgen
left a comment
There was a problem hiding this comment.
What do you mean for 6? Do we have PT JIRA tickets for those already?
@mattgetgen made 1 comment.
Reviewable status: 11 of 46 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 partially reviewed 42 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved.
ebead1c to
6b34287
Compare
Find previously opened as an overlay panel beside the calling editor. Seeding it into the static simpleLayout makes it a permanent tab in the Resources & Tools column, so openFind's existing existingId: '?' probe brings that one tab to the front instead of creating a panel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The seeded Find tab was the only column 3 tab with a close button, because FindWebViewProvider left isClosable at its default of true. Closing it and pressing Ctrl+F reopened Find as an overlay panel beside the editor with its state reset -- the exact behavior this tab replaces. Set isClosable from the interface mode, matching every other fixed column 3 provider, and register the web view type in FIXED_LAYOUT_WEBVIEW_GROUPS so getTabGroup routes the pinned tab to TAB_GROUP_RESOURCES rather than the generic group. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Column 3 ordering: Find is last in the static simple layout, but Text Collection is merged in at runtime from default-layout-supplement.json and was appended after every static tab. Add an optional insertBeforeWebViewType to the supplement entry (absent or unmatched still appends) and set it to platformScripture.find, so Column 3 reads Bible texts, Commentaries, Comments, Text Collection, Find while Text Collection stays flag-gated. Review fixes: - Give the Find tab a simple-mode tooltip so it stays identifiable when Column 3 collapses to icon-only; it has no icon yet. - Bring an existing Find web view to the front even when no project resolves, so invoking Find never looks like a dead shortcut. - Read the localized title and interface mode together under allSettled, each with a fallback, so a failed read can't turn the permanently open tab into a startup error tab. - Document the hidden-tab case for the search result scrollIntoView, which no-ops in an inactive display:none dock pane. - Trim the isClosable comment to the behavioral reason and drop the Find entry's comment block in the layout data. - Correct the FIXED_LAYOUT_WEBVIEW_GROUPS doc comment, which no longer described where Column 3's entries come from. - Update the scripture-find keyboard shortcut entry: it now fronts a permanent tab rather than opening a dialog. - Drop the ellipsis from the Find menu label in English and Spanish; the action completes immediately. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Session-URL: <session URL>
Six findings from review, five fixed and one rescoped after verifying the Simple-mode replace surface. Interface-mode fail-safe (findings 1 and 6): the Find provider guessed an interface mode with Promise.allSettled when a settings read failed, justified by a comment claiming a rejection would produce an error tab. It would not — retrieveWebViewContent is awaited in a fire-and-forget IIFE whose catch only logs, and createErrorTab is reachable only from a synchronous tab-loader throw. Worse, neither guess is safe: 'simple' yields isClosable: false, routing the tab to TAB_GROUP_RESOURCES, which getGroups only registers in simple mode, so in power mode it lands in rc-dock's unregistered-name fallback with no close button; 'power' yields a closable tab in simple mode the user can strand. Both reads now reject like every sibling provider does. Pre-provider startup window (finding 5): fixed-layout tabs carried no isClosable, and createRCDockTabFromTabInfo defaults a missing value to closable, so every Column 3 tab rendered with a close button until its provider responded. Closing Find there stranded it — Simple mode never persists layout, and the next Ctrl+F built a fourth column. Declared on all six fixed tabs, which also stops them flipping rc-dock groups mid-startup. Read-only resources (finding 2): openFind took whatever project the editor held with no editability check, so Ctrl+F on a published resource re-pointed the always-visible Find tab at it. Find now deliberately follows the editor onto resources — searching a resource is legitimate — but threads the editor's already-resolved state.isReadOnly through so Replace, Replace All, and the per-result replace affordances are withheld. The per-result gate matters separately: each result's Replace button and its Enter/Space shortcut call onReplace directly and would bypass the disabled top-level buttons. Stale project (finding 3): openOrUpdateRelatedPanels re-points four Column 3 panels and Find was not one of them, so after a project switch the permanent Find tab kept the previous project's results. Adds platformScripture.updateFindProject, which re-points an open Find and creates nothing, called from that same function. Auto-sync edit block (finding 4): rescoped to a documented gap rather than a fix. The reported one-click path does not exist — Simple-mode Find hides the replace UI entirely (hideModeToggle, the coercion effect, and the activeMode === 'replace' gates on both the replace row and the per-result controls), so the always-visible tab cannot write. Power-mode Find is genuinely missing from EDIT_BLOCKABLE_WEB_VIEW_TYPES, but that predates this branch; recorded there with the four steps a fix needs. Records ADR-0013 for letting Find diverge from the Column-3-follows-the-active- translation-project contract. Co-authored-by: Claude Code <noreply@anthropic.com>
Thirteen of fifteen findings fixed; two rescoped after checking the premise. Provider correctness. The title is now the raw localize key rather than resolved text, so PlatformTabTitle re-resolves it on a UI language change instead of stranding a permanent tab in the old language, and a failed localization can no longer leave the tab reading "Unknown" (findings 13, 14). scrollGroupScrRef falls back instead of overwriting: layout hydration reloads with no options at all, so the old unconditional assignment destroyed the saved value on every restart, silently resetting a power-mode user's chosen scroll group — simple mode now forces group 0 explicitly rather than landing there via useScrollGroupScrRef's ?? 0 default (finding 15). Adds a Lucide search glyph, so Simple mode's icon-only collapse no longer falls back to the generic app logo with the label hidden (finding 11). Hidden auto-search (finding 8). Find follows the editor's scroll group, so every book change — every chapter change under chapter scope — fired a full find job into a display:none pane: uninterruptible past its scope boundary, polled at ~10 Hz, pulling a whole book's USJ into an iframe whose result cards then decline to render it. Closing the panel used to stop that; a permanent tab cannot be closed. Auto-searches now defer while hidden and collapse into one catch-up on activation, via a new useRunWhenVisible hook. openFind. The no-project branch no longer floats a rejection: any existingId routes through the main process's findOwner, which throws when a window is unreachable, and the Ctrl+F handler sends this command without awaiting it (finding 18). It also reloads when the editor web view id changes, not only the project — replacing the editor tab without changing project left Find holding a dead id, silently downgrading result clicks to scroll-group-only navigation (finding 19). The declared contract in the hand-maintained .d.ts, which other extensions compile against, now matches what the branch actually returns (finding 20). Layout robustness. A misspelled insertBeforeWebViewType used to compile, lint, and pass every test while silently appending — the JSON arrives as an untyped property access, and -1 means append for both "not requested" and "not found". Unresolvable placements are now reported (finding 17). The supplement's Text Collection tab gains the isClosable: false the static layout tabs got in the previous commit. Tests (findings 9, 10). Adds shipped-simple-layout-order.test.ts, which uses the REAL layout data and the REAL supplement JSON together — the shipped order was untestable before, since the util's own tests build synthetic entries and the only importer of the real JSON mocks it away. Order-sensitive assertions replace the order-agnostic toHaveLength/toContain, plus a recognized-keys check that catches the typo case. Verified by reproducing the typo and watching the merge test fail. The provider's computed fields move to a tested util, since the provider module imports the webpack ?inline bundle and cannot be imported by any test runner here. Documentation. Documents the two remaining hidden-pane effects in search-result.component.tsx — the IntersectionObserver (self-correcting) and the replace progress rAF (accepted) — per the cross-view-sync rule, and points that rule at the new hook (finding 21). Fixes the FIXED_LAYOUT_WEBVIEW_GROUPS JSDoc, whose positional "except the one below" stopped being true the moment Find was appended under it (finding 12). Rescoped, with reasoning: Finding 7 (permanent tab does nothing) and finding 16 (iframe built twice) both rest on Find being special, and it isn't. No Column 3 panel carries a projectId in the static layout; all four get one from openOrUpdateRelatedPanels when a project opens, and the previous commit put Find in that same call. The cited precedent for "resolve the project in the provider" is a pending > options > saved priority chain, not resolution from nothing — no provider in this repo resolves the active project itself. So the second build is inherent to static layout plus async binding and is shared by every sibling. What remained of finding 7 is the genuine no-project state, which now says so instead of accepting a search that silently returns nothing. Also: finding 21's closing point about the leader line describing the wrong effect was already resolved by rebasing onto main, which corrected that comment. Co-authored-by: Claude Code <noreply@anthropic.com>
Provider hygiene. FindWebViewOptions no longer declares mandatory properties, which OpenWebViewOptions explicitly forbids because both reloadWebView and openWebView hand their options straight to getWebView — and the reload path routinely passes none of them, which is why the provider needed its || chain in the first place (finding 23). Adds the savedWebView.webViewType guard every sibling provider performs (finding 26). The tooltip is now set unconditionally: PlatformTabTitle already drops a tooltip that merely repeats the visible title unless the tab is icon-only, and its comment names this caller pattern, so the mode gate only re-implemented that suppression one layer out — and its power arm was a self-assignment, since the provider spreads savedWebView first and is the field's only writer (finding 27). openFind. Drops bringToFront: true, which is already the default, and hoists the existingId probe and the panel layout literal, each of which had come to appear twice in a function whose sibling already hoists exactly this kind of literal (finding 28). While removing that dead config: updateFindProject was relying on the same default in the opposite direction. bringToFront defaults to TRUE on both the probe and the reload, so re-pointing Find during a project switch was pulling Column 3 to the Find tab — the code did the opposite of what its own comment claimed. Both calls now opt out explicitly. Head-insert active tab (finding 25). rc-dock derives activeId from tabs[0] when unset and no Simple-mode panel sets one, so a future supplement entry anchored before Column 3's first tab would have become the column's default view as a side effect of ordering. The incumbent first tab is now pinned as activeId before it stops being first, with tests for both the head insert and the explicit-activeId case — nothing exercised index 0 before. Drift guards. Two new tests, both verified by breaking the invariant and watching them fail. FIXED_LAYOUT_WEBVIEW_GROUPS' "kept in sync with two sources" is now enforced: a pinned webViewType missing from the map falls back to TAB_GROUP, which IS registered in Simple mode, so nothing errors and the tab silently becomes draggable across columns (finding 22). And every pinned webViewType is checked to still exist in the extension that provides it, reading extension source the way the SCRIPTURE_EDITOR_WEBVIEW_TYPE guard does (finding 24). On finding 24's suggested shape: this covers all six pinned literals rather than mirroring one constant into core. Adding a core-side constant for Find alone would leave the three pre-existing Column 3 literals unguarded while implying the convention had been applied, and the six types are declared under several differently-named, mostly non-exported constants, so matching the literal is what works uniformly. The guard skips tests and stories — the first draft passed a rename because a test fixture of mine hard-coded the same string and vouched for it. Docs. anchorWebViewType's TSDoc no longer claims the tab "is appended", which became conditional once insertBeforeWebViewType existed, and the fallback is documented as reported rather than silent (finding 30). Records ADR-0014 answering the question the PR body put to the team — anchor + insert-before on the supplement entry versus another way of pinning a static tab last — so the next Column 3 tab does not re-derive it. Finding 29 (sixth copy of isClosable: interfaceMode === 'power') is left alone deliberately: it cannot be fixed inside this PR, and per the review a comment is the wrong vehicle. It needs a ticket, which I cannot file — the renderer already holds both halves needed to centralize it. Co-authored-by: Claude Code <noreply@anthropic.com>
… editor id Make the interface mode an explicit argument at the default-layout-supplement merge. The merge runs against both modes' layouts, but an entry's ordering (`insertBeforeWebViewType`) and pinning (`isClosable: false`) only describe Simple mode's fixed columns. Carrying them into a Power-mode layout put the merged tab in a rc-dock group `getGroups` registers only in Simple mode, and logged a placement anomaly on every load of a correct layout. Power mode now appends without reporting, and an entry's Simple-mode pin is rewritten to closable. Both modes' behavior for the shipped entry is now asserted. Hand Find the editor web view id a project switch produces. The Column 3 re-point runs before the editor tab is replaced, so Find kept the replaced editor's id and silently skipped the selection, highlight, and focus that a result click depends on. `updateFindProject` takes the id, its early-return weighs it, and the Find re-point moved to `updateRelatedFindPanel`, called once the new editor exists. Document `updateFindProject`'s writable-project precondition in the declared types rather than only in a code comment, and make the read-only scrub follow the project it describes: recomputed when a caller re-points the project, carried over when none is named, matching `projectId`'s own fallback. Layout hydration no longer restores a resource-bound Find with Replace enabled. Promote `useRunWhenVisible` to `platform-bible-react` beside its partner `useViewVisibility`, so the repo-wide rule that advertises it can be followed. Give `getReplaceUnavailableReason` an options object, and rewrite two comments that narrated the branch's own history. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ters Invoking Find landed the user on a tab they could not type into. Bringing the tab to the front focuses the web view's iframe, but that lands on the iframe's `body`, so the caret sat nowhere and the next keystroke was lost — against the stated purpose that every Find entry point lands on the one Find tab. Focus is taken on explicit invoke only, in both interface modes. A plain tab click still gets the platform's restore-last-focused-element behavior, so a user who was mid-edit in the Replace field is not yanked back to Search every time they return to the tab. `openFind` has three exits and they need two different signals. The two that reuse an already-mounted panel send an addressed `platformScripture.focusFindSearch` event, which the panel is already subscribed to; the one that rebuilds the web view carries the request in `FindWebViewOptions.shouldFocusSearch`, because an event emitted around that mount would race the new mount's own subscription. The event names a web view rather than broadcasting, since Power mode can hold more than one Find panel and only the resolved one should take the caret. Both routes go through `useRunWhenVisible`. In Simple mode the Find tab is the inactive one when Ctrl+F is pressed, so its pane is `display: none` and focusing an element inside it would be a silent no-op; a request arriving before rc-dock re-renders is deferred to the visibility transition instead of dropped. The composition lives in `useFocusSearchOnInvoke` so both delivery routes and the deferral are testable without PAPI or a real iframe. The request flag is scrubbed whenever a caller supplies nothing, so it cannot fire on layout hydration or on a project switch, neither of which is an invoke. Also records the two tickets that now cover deferred work: PT-4404 for Find's replace during an automatic Send/Receive, and PT-4405 for centralizing the pinned-tab isClosable computation instead of duplicating it per provider. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Build output, not a hand-merged artifact: the rebase's dist conflicts were resolved by rebuilding the workspace rather than reconciling bundles by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6b34287 to
8ebcd69
Compare
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 partially reviewed 33 files and all commit messages.
Reviewable status:
2 participants
Code Review Summary
Branch: pt-4342-dock-find-in-simple
Base: origin/main
Date: 2026-08-14
Review model: Claude Opus 5
Files changed: 5 at review start; 13 after in-review changes (8 files newly touched during the review)
Overview
Makes Find a permanent tab in Simple mode's Column 3 instead of a panel opened beside the editor, so
every Find entry point — Ctrl+F and the editor menu's Find item — lands on that one tab and brings it
to the front. The branch adds the tab to the static
simpleLayout, registersplatformScripture.findinFIXED_LAYOUT_WEBVIEW_GROUPSso the tab is confined to Column 3'src-dock group, and sets
isClosable: interfaceMode === 'power'in the Find web view provider so thetab cannot be closed in Simple mode.
During the review the author also asked for Text Collection to sit ahead of Find in Column 3. That
required a mechanism change rather than a reorder: Find was already last in the static layout, but
Text Collection is merged in at runtime from
default-layout-supplement.jsonand was appendedafter every static tab. An optional
insertBeforeWebViewTypewas added to the supplement entry(absent or unmatched still appends), so the final Column 3 order is Bible texts, Commentaries,
Comments, Text Collection, Find — while Text Collection stays flag-gated and disappears cleanly when
its feature flag is off. Five findings were fixed during the review (tooltip, no-project bring-to-
front, hidden-view documentation, catalog accuracy, parallel/fail-safe settings read) plus three
minor cleanups; the project-hydration gap and the focus/Escape interaction work were deliberately
deferred.
API Changes
None. No public API surfaces changed. The diff touches no files under
lib/platform-bible-react/,lib/platform-bible-utils/,lib/papi-dts/papi.d.ts, or anyextensions/src/*/src/types/*.d.ts.The extension change adds an
isClosable(and nowtooltip) value to theWebViewDefinitionthatFindWebViewProvider.getWebViewreturns — both are already-existing optional properties of thepublic
WebViewDefinitionBase, so no exported type, signature, or default changed.In-review changes added one internal, non-exported-API field: the optional
insertBeforeWebViewTypeonDefaultLayoutSupplementEntry(
src/renderer/components/docking/default-layout-supplement.model.ts). This is renderer-internal andbackward compatible — entries without it behave exactly as before.
Findings
Critical — Must address before merge
The permanent Simple-mode Find tab never receives the active project except through
openFind, producing two user-reachable broken states: (1) before the first Ctrl+F, clickingthe Find tab renders a fully interactive UI that silently does nothing —
findPdpisundefinedsohandleStartSearchearly-returns atfind.web-view.tsx:460, with no status,error, or explanation; (2) after a Simple-mode project switch, the tab keeps the previous
project's term, results and scope until the next Ctrl+F, because Find was not added to
openOrUpdateRelatedPanels(platform-scripture-editor.utils.ts:1039-1070) andopenFindonly reloads when the project differs (
main.ts:316-322) — so Replace / Replace All act onthe stale
projectId, mutating a project the user is no longer looking at.(Author: considered, but judged out of scope for this PR.)
[Author response: The author confirmed this was considered and consciously deferred. When asked
specifically whether the Replace/Replace-All-on-stale-project consequence was part of what was judged
out of scope, and whether a follow-up ticket exists, the author said "I don't really know, so I can't
say for sure." This half is therefore unresolved — the scope decision is recorded, but nobody has
confirmed that the data-mutation consequence was weighed or is tracked anywhere.]
Important — Should address before merge
tooltip, so with Column 3 collapsed to icon-only it had no iconidentity, no visible title, and no hover label. (fixed during review: added
tooltip: interfaceMode === 'simple' ? title : savedWebView.tooltip, mirroring the CommentList factory. Both locales already exist under
%webView_find_title%— "Find" / "Buscar" —so no new strings were needed.)
openFindreturned early when the editor had no project, so Ctrl+F and the menu item didnothing at all while the permanent tab sat in plain view — contradicting the stated purpose.
(fixed during review: the no-project branch now brings any existing Find web view to the
front via an
existingId: '?'probe withcreateNewIfNotFound: false, so invoking Find alwayslands on the tab; it still declines to create one, since a project-less Find has nothing to
search.)
.claude/rules/cross-view-sync-hidden-views.md): making Find permanentlymounted puts
search-result.component.tsx'sscrollIntoView({ block: 'nearest' })inside adisplay: nonepane for most of the session, where it no-ops. Confirmed path: in Replace modean external scripture change re-runs the search while hidden, clearing and re-selecting
focusedResultIndex, so activating the tab shows a selected result scrolled out of view.(fixed during review: added the explicit "Hidden case: intentionally not handled because …"
dismissal comment the rule requires, documenting why the catch-up isn't worth a
useViewVisibilitymechanism. Flagged here for reviewer scrutiny per the rule.)The Find tab sets no(Author: will add once UX supplies an icon. PartiallyiconUrl, so when Column 3 collapses to icon-only it shows the genericapp logo (
.tab-menu-icondefaulturl('/assets/icon.png')), unlike all three Column 3siblings which set distinct icons.
mitigated by the tooltip added above, and an in-code comment now records that the tooltip is
the tab's only identification until it gets an icon — the design doc noting this deferral lives
under gitignored
docs/superpowers/.)Ctrl+F fronts the tab but performs no focus management — nothing in
find.web-view.tsxorfind.component.tsxfocuses the search input, so the user's typing still goes to the editor.Previously the common path opened a new panel; now every invocation is the reuse path.
(Author: follow-up, out of scope for this PR.)
No automated coverage of the behavior the PR is about. The branch's tests assert static shape
only (tab count, webViewType present, group mapping); nothing verifies that invoking Find
activates the pinned tab rather than opening a panel, or that the tab is non-closable in Simple
mode.
openFindhas no tests at any level. The sibling feature is covered end-to-end bye2e-tests/tests/isolated/comments-tab.spec.ts, whose harness is directly reusable, and thisarea already regressed once on this branch (
94390f6repairs0c1d180) with no test added.(Author: declined to add an e2e test.)
[Author response: Author fixed the tooltip, the no-project bring-to-front, and the hidden-view
documentation during the review. The icon is deferred pending a UX-supplied glyph; focus-on-invoke is
an explicit follow-up; e2e coverage was declined. Note the two unit tests added during review cover
only the new supplement ordering, not the pinned-tab behavior — so the feature itself remains
verified by manual testing only.]
Minor — Consider
FIXED_LAYOUT_WEBVIEW_GROUPS— described Column 3's members as not comingfrom the static layout, which the new Find entry made more misleading. (fixed during review:
reworded to say the map tracks every webViewType hardcoded in
simple-layout.data.tsplusscriptureTextGrid, which alone joins Column 3 at runtime from the supplement.)keyboard-shortcuts.data.tsaccuracy —scripture-find's purpose read "Open the finddialog", which is now wrong (it fronts a permanent tab) and conflicts with
Guidelines/Terminology's Tab-vs-Dialog distinction. (fixed during review: purpose now reads
"Bring Find to the front — a permanent tab in Simple mode, a panel beside the editor in Power
mode", and
extensions/src/platform-scripture/src/main.tswas added tolocations.)"Find..."/"Buscar..."ellipsis inplatform-scripture-editor/contributions/localizedStrings.json:94,257— three periods insteadof
…, and per the Ellipses guideline no ellipsis belongs there at all now, since the actioncompletes immediately. (fixed during review: both values are now "Find" / "Buscar". Edited in
place, which the Localization Guide's punctuation-correction exception explicitly permits since
the meaning is unchanged — no new key or
fallbackKeyneeded.)getTabGroup,TAB_GROUP_RESOURCES,getGroups()) from extension code, and the siblings' "Re-read every call so mode changes arepicked up" line was missing. (fixed during review: comment trimmed to the behavioral reason,
closely mirroring the comment-list factory; settings-read line added. The tooltip comment added
earlier in the review was trimmed to the same shape.)
startup error tab. (fixed during review: the localization and settings reads now run
together under
Promise.allSettled, each with its own fallback andlogger.warn— title fallsback to the saved title, interface mode fails safe to
'simple', mirroring the existingfail-safe in
platform-scripture-finder-pdpe.model.ts:749-756.)simple-layout.data.test.ts:92is column-agnostic — asserts Find exists somewhere, not thatit's in Column 3, and doesn't pin the fixed UUID.
"if Find exists at all is good enough for now." Note the finding's premise was partly wrong —
openFindmatches by web view type viaexistingId: '?'→findFirstWebViewDefinitionByType, not by that UUID, so the id is only load-bearing for layoutid-uniqueness, which an existing test already covers.)
No Escape handling in the Find web view; in Simple mode the tab is non-closable, so there is(Author: follow-up, out of scope —no way to dismiss it or hand focus back to the editor.
same call as focus-on-invoke.)
'platformScripture.find'is a bare literal in three core locations (four counting thesupplement JSON) where Column 2 uses the shared
SCRIPTURE_EDITOR_WEBVIEW_TYPEconstant.(Author: leave it consistent with the three pre-existing Column 3 literals.)
interfaceModeread +isClosable: interfaceMode === 'power'is now duplicated across sixproviders in three extensions; this change makes it six.
extension boundaries and isn't fixable within this PR.)
Column 3 density: 4 static tabs (5 with Text Collection) makes icon-only collapse trigger at(Author: accepted trade-off. Nowider column widths, hiding labels for all Column 3 tabs.
e2e breakage —
comments-tab.spec.tsalready falls back to the.dock-nav-moreoverflowdropdown, and the grid spec works inside the iframe without clicking tab titles.)
[Author response: Author fixed five minor findings and dismissed five with reasons — test tightening
(UI position is expected to change), Escape handling (follow-up), the bare literal and the
cross-extension duplication (consistency / later cleanup), and Column 3 density (accepted).]
Template Propagation
Shared Regions Modified
None. No
#region shared withmarkers exist in any changed file.Extension Config Changes
extensions/src/platform-scripture/src/find.web-view-provider.ts— extension source, not aconfig/build file; no propagation needed.
extensions/src/platform-scripture/src/main.ts,extensions/src/platform-scripture/src/find/search-result.component.tsx,extensions/src/platform-scripture-editor/contributions/localizedStrings.json— extension sourceand localized strings; no propagation needed.
Positive Observations
isClosable: interfaceMode === 'power'matches the established pinned-tab convention exactly(scripture editor, model text, Text Collection, comment list all use the same expression), rather
than inventing a parallel mechanism.
openFind's existingexistingId: '?'/createNewIfNotFound: false/bringToFront: trueprobe. That path wasverified to raise a hidden tab (
web-view.service-host.ts:2155→updateWebViewDefinitionSync(id, {}, true), andplatform-dock-layout-storage.util.ts:783proceeds on
shouldBringToFronteven when no properties changed), so repeat Ctrl+F presses doactivate the tab.
simpleLayoutrather than the supplement mechanism is the right call —the supplement is for flag-gated, build-specific tabs, and Find ships in vanilla core.
just the assertions, so counts don't silently disagree with their descriptions.
isClosablein the provider rather than the layout data is correct, sincesaveTabInfoBasedeliberately strips
isClosablefrom persisted tabs (web-view.service-host.ts:1280).scrollGroupScrRefto fall through asundefinedat startup is harmless —useScrollGroupScrRefdefaults it to group 0, the group Simple mode forces the editor onto.from
%webView_find_title%viagetLocalizedString.simple-layout.data.tsfall inside the file's existingeslint-disable no-type-assertionblock rather than adding new per-line suppressions.hardcoded GUID.
Interview Notes
Stated purpose: make Find a consistent tab in Column 3 of Simple mode, brought to the front
whenever Find is invoked, via Ctrl+F or the editor menu's Find item.
Decisions the author made during the review:
Collection. They then asked to make Text Collection a static tab instead of using the supplement
mechanism. After seeing that its provider is only registered when
platformScriptureEditor.enableScriptureTextGridis true — a user-facing setting — and that astatic tab would therefore render provider-less and uncloseable in Simple mode whenever that
setting is off, they chose to keep the supplement mechanism.
simple-layout.data.ts("It's notneeded"), leaving the entry shaped like its three siblings. The ordering rationale now lives with
the mechanism, in the
insertBeforeWebViewTypeTSDoc and the supplement JSON.Author does not understand: the pinning mechanism. Asked to walk through the
isClosable === false→getTabGroup→TAB_GROUP_RESOURCES→getGroups()chain — the exactthing commit
94390f6was fixing — the author said "I'm not sure what to say honestly." This is thecore mechanism of the change and should be walked through in the review meeting. (A written
explanation was provided during the review, but the author did not demonstrate independent
understanding of it.)
Author could not confirm whether the Replace-on-stale-project consequence of the Critical finding
was part of the out-of-scope judgement, or whether it is tracked anywhere: "I don't really know, so I
can't say for sure."
Discovered during the review — separate from this branch, worth tickets:
platformScriptureEditor.enableScriptureTextGriddoes not persist. The author toggled it inthe app and it reverted. Confirmed on disk:
~/.platform.bible/data/settings.jsoncontains onlyplatform.firstRunCompleteandplatform.interfaceMode, and its mtime was still the previousday.
getcorrectly falls back to the declared default (true) for any absent key(
settings.service-host.ts:147-148), andsetitself looks correct(
:156-170— validate, assign,writeSettingsDataToFile), with no validator registered for thekey and no code writing it back. That narrows the bug to the settings-tab control's write call.
Pre-existing and unrelated to this branch.
npm run typecheckis currently red repo-wide:platform-scripture-editor/src/character-marker-bar/use-remove-character-marker.hook.ts:91—Property 'removeCharacterMarker' does not exist on type 'EditorRef'. Proven pre-existing bystashing every change and re-running; it is the locally-linked
scripture-editorsbuild, not thisbranch. This will fail CI until the editor dependency is resolved.
"Find..."string exists atplatform-enhanced-resources/contributions/localizedStrings.json:519(
%enhancedResources_toolbar_menu_find%, English only). It belongs to the enhanced-resourcesviewer's own find (
onFindInResource), a different feature, so it was deliberately left alone.In-Review Quality Check
npm test: all pass. Every workspace green — 141, 130, 54, 29, 16, 12, 9, 8, 5, 1, 1 testfiles, no failures. Includes 2 new tests for the supplement ordering; the ordering test was
verified to actually fail when the insert logic is disabled (falsifiability check).
npm run lint: 0 errors. 4 pre-existing warnings remain, all inplatform-scripture-editorfiles untouched by this branch (duplicate imports,
consolestatements).(
default-layout-supplement.model.ts,default-layout-supplement.util.ts). Cosmetic only.npm run typecheck: 1 failure, proven pre-existing — see Interview Notes item 2. Not fixed,because it is not caused by this branch and fixing it means resolving the linked editor build.
Suggested Review Focus
Prioritized areas for the author-reviewer meeting:
isClosable === false→getTabGroup→
TAB_GROUP_RESOURCES→getGroups()registering that group only in Simple mode. The authorcould not explain this, and it is both the core of the change and what
94390f6had to fixafter
0c1d180.on-stale-project path is understood and tracked. Clicking the Find tab before the first Ctrl+F
also gives a silently dead UI. Decide whether either needs to block merge.
.claude/rules/cross-view-sync-hidden-views.md,the reviewer should scrutinize the "intentionally not handled" comment in
search-result.component.tsxrather than take it on faith.insertBeforeWebViewTypesupplement addition — a small core-mechanism change mademid-review to satisfy the Column 3 ordering request. Confirm the approach is what the team
wants, versus a different way of pinning a static tab last.
this branch. Confirm the team is comfortable shipping it verified by manual testing only.
non-closable, so today there is no keyboard way into the search box or out of the tab.
enableScriptureTextGridsetting notpersisting, and the red
npm run typecheckfrom the linkedscripture-editorsbuild.This change is