PT-4534: Regroup Simple's Project menu - #2847
Conversation
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 24 comments.
Reviewable status: 0 of 43 files reviewed, 23 unresolved discussions (waiting on katherinejensen00).
a discussion (no related file):
A review of the Simple Project-menu regroup at f2ff018, covering the 31 changed source files. The regenerated lib/platform-bible-react/dist/ bundle was skipped as build output.
27 findings follow — one high, five medium, the rest low. Each carries the severity it settled at and a short status: checked and confirmed means the finding was verified against the code, and needs a human call means it could not be settled either way and the judgement is yours.
23 of them are inline comments on the files below. The remaining four are collected here, because they sit in files this pull request does not change and so have no line in this review to attach to.
The one high finding (inline, on menus.json) is a product question rather than a code one, so it is worth reading first.
Findings in files this pull request does not change
#15 — low · checked and confirmed
lib/platform-bible-react/src/components/advanced/menus/platform-menubar.component.tsx:89
A submenu item in the application menubar that declares a tooltip shows no tooltip on hover and announces no description to a screen reader.
What happens: getMenubarContent wraps every item in one TooltipTrigger asChild at platform-menubar.component.tsx:69-70, and for a submenu the child is MenubarSub (:90), which menubar.tsx:283 renders as MenubarPrimitive.Sub — a context provider with no DOM node — so the cloned props never reach MenubarSubTrigger.
Why it matters: this is the same defect fixed in tab-dropdown-menu.component.tsx, applied at only one of the two menu renderers, so the application menubar keeps silently dropping submenu descriptions. It affects no shipped UI today: this file is byte-identical at the merge-base, so it is pre-existing rather than introduced, and no menus.json or menu.data.json in this repo or the three sibling extension repos uses tooltip on any item.
Fix: restructure getMenubarContent the same way as tab-dropdown-menu.component.tsx:62-104 — early-return the 'command' in item branch with its own Tooltip, and in the submenu branch put <Tooltip><TooltipTrigger asChild><MenubarSubTrigger>… inside MenubarSub; add a case to platform-menubar.component.test.tsx asserting the tooltip text appears for a submenu item, mirroring shows a submenu item's tooltip on hover at tab-dropdown-menu.component.test.tsx:272.
#22 — low · checked and confirmed
lib/platform-bible-react/src/components/shadcn-ui/menubar.tsx:45
In an RTL interface the application menubar's submenu opens to the right of its parent and ArrowLeft does not open it, while the tab menu's flyout correctly opens to the left.
What happens: DropdownMenu reads readDirection() and passes dir to the Radix root (dropdown-menu.tsx:76-92), which is what makes the new chevron logic and the RTL key mapping agree. MenubarPrimitive.Root (menubar.tsx:45-52) receives no dir and menubar.tsx never imports readDirection, so Radix falls back to ltr: the flyout opens right, ArrowRight opens it, and the hardcoded IconChevronRight at menubar.tsx:310 is consistent with that wrong behaviour. context-menu.tsx has the same shape (hardcoded chevron at :131, no dir), and ContextMenuSub does ship in production — src/renderer/components/docking/platform-tab-title.component.tsx:174 uses it for the tab right-click "Move to window" submenu, as do two components under extensions/src/platform-enhanced-resources/src/components/dictionary-tab/ — so the same defect reaches users there too.
Why it matters: only reachable where layout direction is set to RTL, which nothing in the shipped app currently does, so this is a correctness gap rather than a shipping regression — but changing only menubar.tsx:310 to follow readDirection() would make the arrow point away from where the flyout actually opens.
Fix: in menubar.tsx, add const dir: Direction = readDirection(); to Menubar and pass dir={dir} to MenubarPrimitive.Root before making MenubarSubTrigger pick its chevron the way DropdownMenuSubTrigger does; annotate both edits with // CUSTOM: comments per the shadcn convention, and add LTR/RTL chevron cases to platform-menubar.component.test.tsx mirroring tab-dropdown-menu.component.test.tsx:313-337.
How this was checked: Side-by-side confirms the asymmetry: DropdownMenu in dropdown-menu.tsx:76-92 computes dir: Direction = readDirection() and passes dir={dir} to DropdownMenuPrimitive.Root, while Menubar in menubar.tsx:26-55 — unchanged by this change, with zero diff between merge-base and head — passes no dir and the file never imports readDirection. MenubarSubTrigger (menubar.tsx:288-313) hardcodes <IconChevronRight> with no RTL branch. In Radix, an unset dir on MenubarPrimitive.Root defaults to ltr, so a submenu opens rightward and the wrong arrow key drives it. ContextMenuSub shares the same shape and does ship in production — src/renderer/components/docking/platform-tab-title.component.tsx:174 (the tab right-click "Move to window" submenu) and two components under extensions/src/platform-enhanced-resources/src/components/dictionary-tab/ — so the footprint reaches users there as well. Nothing in the shipped app can currently set layoutDirection to 'rtl', so this asymmetry has no observable effect on a real user today.
#23 — low · checked and confirmed
src/extension-host/data/menu.data.json:127
Simple's Project section reads "Open Project Settings…" where the design says "Project settings".
What happens: the item comes from defaultWebViewTopMenu (menu.data.json:127-133), which every web view's top menu inherits through includeDefaults: true (menus.json:9); hiddenInterfaceModes lives on items only, and the combiner merges arrays by plain concat with no id- or command-based override (document-combiner.ts:359-366), so there is no way to give Simple a different label for that one item without forking the shared defaults or shipping a visible duplicate.
Why it matters: modestly. menu.data.json is byte-identical at the merge-base, so this change inherits the mismatch rather than creating it, and the trade-off is already stated in the ticket — it is simply not transcribed into any repo artifact, so a later reader comparing the shipped menu with the design re-derives it from scratch. The served-menu test pins the command id, not the label.
Fix: add a one-line note to the platform.openSettings item's localizeNotes in menu.data.json recording that the Simple design calls this "Project settings" but the item is served from the shared defaultWebViewTopMenu used by every mode and every web view, so it cannot be relabelled for Simple alone without either a schema change to gate labels per mode or duplicating the item. Do not rename the shared localization string: %webView_openProjectSettings% backs every web view's menu, Power's label must not change per adr-menu-per-mode-layout-via-mode-gated-columns, and the existing key is immutable under Localization-Guide.md's "Existing Strings Are Immutable" rule — a genuine reword would need a new key with a metadata.json fallbackKey, which still would not make the label mode-specific.
#27 — low · checked and confirmed
src/shared/data/keyboard-shortcuts.data.ts:471
Simple's Project menu shows a shortcut hint on only two items — Find and Insert comment — while Switch Scripture view, Show footnotes, Insert footnote, Insert cross-reference and Project settings show none.
What happens: menu-data.service-host.ts:45-56 attaches a hint only when getShortcutHintForCommand finds a catalog entry with a matching command; just platformScripture.openFind (:452) and platformScriptureEditor.insertCommentAtSelection (:468) have one, and a test in keyboard-shortcuts.data.test.ts asserts that list is exhaustive. ⌃T/⌃⇧T are catalogued but deliberately command-less (:477, :490); ⌃J, ⌃E and an editor F7 are not catalogued at all.
Why it matters: every Simple user sees a menu that teaches two shortcuts where the design teaches five. Worse for ⌃J, ⌃E and F7: no handler for them exists in the editor at all — the only F7 handler in the tree belongs to a different extension — so those three chords do not work, rather than merely lacking a hint. Nothing in the repo points at the follow-up: a search for PT-4735 returns zero matches anywhere in the tracked source. The anchor line predates this change (the hint mechanism came with PT-4532); what this change does is surface these items together in one Simple menu alongside two that do show hints, making the inconsistency visible for the first time.
Fix: add // TODO(PT-4735): ... beside the scripture-insert-footnote and scripture-insert-cross-reference entries in src/shared/data/keyboard-shortcuts.data.ts, naming the chords still to be catalogued, so the deferral is searchable from the code. Adding any command later also needs its row in EXPECTED_MENU_HINTS in keyboard-shortcuts.data.test.ts.
How this was checked: keyboard-shortcuts.data.test.ts's own EXPECTED_MENU_HINTS map (:20-32) lists exactly two commands with a catalog command — platformScripture.openFind and platformScriptureEditor.insertCommentAtSelection — and a test there asserts every catalog entry with a command matches that list, so no other item in any menu anywhere in the bundled app can carry a hint; the file passes 6/6 today. The scripture-insert-footnote/scripture-insert-cross-reference entries (keyboard-shortcuts.data.ts:471-495) carry ⌃T/⌃⇧T but are deliberately command-less, each with an inline comment saying so. No catalog entry exists at all for ⌃J or the editor's ⌃E/F7; a repo-wide search for a keydown handler on j/e found none, and the only F7 handler in the tree lives in a different extension (extensions/src/platform-enhanced-resources/src/web-views/enhanced-resource.web-view.tsx:2913), not the scripture editor — so those three chords have no working handler in the editor at all, not merely no hint. A repo-wide search for PT-4735 returned zero matches in the tracked source tree. The anchor line itself is unchanged by this change; the gap predates it and comes from the earlier hint mechanism, but this change is what surfaces the affected items together in one visible Simple menu alongside two that do show hints.
(AI-assisted, with my guidance)
.context/standards/Entry-Point-Guide.md line 131 at r1 (raw file):
rationale and history. When a mode needs a different *section* structure, not just fewer items, give that mode its own
#2 - medium · checked and confirmed
A developer following Entry-Point-Guide.md to add a menu item is told "Commands must match exactly what's registered in main.ts", while five shipped menu items name commands that main.ts never registers.
What happens: the Edit flyout's items declare platformScriptureEditor.undo/.redo/.cutSelection/.copySelection/.pasteAtSelection (menus.json:218-253). None is passed to papi.commands.registerCommand in extensions/src/platform-scripture-editor/src/main.ts and none is declared in types/platform-scripture-editor.d.ts; they are intercepted by isEditMenuCommand inside menuCommandHandler (platform-scripture-editor.web-view.tsx:3555-3559). The ADR records this as a deliberate second route, but the promotion into Entry-Point-Guide.md covered only the per-mode-columns half (:131-132), leaving the contradicting bullet at :108 untouched.
Why it matters: the standards are what the next developer and the next agent read; the decisions log keeps the why. Anyone adding an editor menu item now gets contradictory guidance from one page, and no other standard documents the second route.
Fix: amend the ### Menu Item Structure note at Entry-Point-Guide.md:108 to state the second route — a menu command id handled by the web view's SelectMenuItemHandler rather than registered in main.ts — and cross-reference adr-menu-per-mode-layout-via-mode-gated-columns beside it, naming EDIT_MENU_COMMANDS in edit-menu-actions.util.ts as the list that keeps the menu and the interception in step.
How this was checked: Entry-Point-Guide.md:108 (unchanged by this change, under "### Menu Item Structure") states flatly, as a Notes bullet with no hedging or scoping, "Commands must match exactly what's registered in main.ts." The Edit flyout's five commands (menus.json:218-253) are absent from every papi.commands.registerCommand call in extensions/src/platform-scripture-editor/src/main.ts (16 registrations, none of these five) and absent from types/platform-scripture-editor.d.ts; they are read via isEditMenuCommand/EDIT_MENU_COMMANDS inside menuCommandHandler (platform-scripture-editor.web-view.tsx:3555-3559), a SelectMenuItemHandler. The ADR's Decision paragraph explicitly records this as deliberate: "The Edit flyout's Undo/Redo/Cut/Copy/Paste are menu command ids handled inside the editor web view (menuCommandHandler) through EditorRef, not PAPI commands." A repo-wide search of .context/standards/ and .claude/rules/ for SelectMenuItemHandler, menuCommandHandler, EDIT_MENU_COMMANDS and isEditMenuCommand finds no other standard documenting this route, so the contradiction is not resolved elsewhere.
extensions/src/platform-scripture-editor/src/localized-strings.test.ts line 312 at r1 (raw file):
* tsconfig with no `@node/*` path alias), matched on the manifest `name` field. */ const DEV_ONLY_EXTENSION_NAMES: readonly string[] = [
#10 - low · checked and confirmed
Adding a dev-only sample extension to the canonical list leaves this copy stale, and the menu-label test then counts that extension's strings as shipped.
What happens: this change exports the canonical list at locale-assets.test-helper.ts:29 and the new harness imports it at menu-data.service-host.test-utils.ts:6, but this file re-declares it by hand at :312. The comment at :308-310 cites the missing @node/* alias under the extensions/ tsconfig as the reason, yet a relative import bypasses that alias entirely, exactly as menu-data.service-host.scripture-editor-menu.test.ts:8 already does crossing the same boundary the other way. Nothing compares the two lists, so they drift silently and allKeys() at :330 stops excluding the new dev-only extension.
Why it matters: the exclusion exists so a key reachable only in noisy dev mode cannot vouch for a production string; a stale copy re-opens exactly that false negative.
Fix: import DEV_ONLY_EXTENSION_NAMES from ../../../../src/node/utils/locale-assets.test-helper with an explicit relative path — the technique menu-data.service-host.scripture-editor-menu.test.ts:8 already relies on — and delete the local copy.
How this was checked: The DEV_ONLY_EXTENSION_NAMES list at extensions/src/platform-scripture-editor/src/localized-strings.test.ts:312 is hand-copied from the canonical list exported at src/node/utils/locale-assets.test-helper.ts:29, and nothing keeps the two in sync — a name added to only one means allKeys() at :330 starts miscounting that extension's strings. The comment justifying the copy cites the missing @node/* path alias under the extensions/ tsconfig, but a relative import bypasses that alias entirely, exactly as src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts:8 already does crossing the same boundary the other way. Reproducing the real extensions/tsconfig.json and tsconfig.lint.json in an isolated copy and running TypeScript's own module resolver and a full program build against the mirrored layout, the relative import resolved cleanly under both configs with no import-related diagnostics. The no-restricted-imports rule at extensions/.eslintrc.cjs:91-99 only matches bare specifiers like node/*, not relative paths, so it does not block this either.
Similar fix as #26.
Other findings in this file: #11
extensions/src/platform-scripture-editor/src/localized-strings.test.ts line 349 at r1 (raw file):
} function menuLabelKeys(): string[] {
#11 - low · checked and confirmed
A tooltip added to a Project menu item shows up as a raw %…% key in the UI with no test failing, despite the comment promising new menu strings are covered automatically.
What happens: menuLabelKeys collects only columns[].label and items[].label. tooltip?: LocalizeKey is declared on MenuItemBase (menus.model.ts:68), shared by both item kinds, and Localized<T> (menus.model.ts:241) resolves it exactly as it resolves label; tab-dropdown-menu.component.tsx:84 and :95 render it as TooltipContent from that resolved value. The editor's menus.json has zero tooltips today, so the gap is invisible; the moment one is added with an undefined key, the comment at :358-362 — "a label added to the menu is covered here without anyone remembering to update this file" — is false for it.
Why it matters: the whole value of a document-driven test is that it needs no maintenance; a reader who adds a tooltip will trust it and ship an untranslated string. No other test in the repo walks a menu document collecting tooltip keys.
Fix: widen menuLabelKeys to also collect item.tooltip and rename it to menuLocalizeKeys, filtering to values matching /^%.*%$/ so undefined entries are dropped.
How this was checked: menuLabelKeys() (localized-strings.test.ts:349-356) collects only column.label and item.label from the raw ../contributions/menus.json import; it never reads item.tooltip. tooltip?: LocalizeKey is declared on MenuItemBase (lib/platform-bible-utils/src/extension-contributions/menus.model.ts:68), shared by both MenuItemContainingCommand and MenuItemContainingSubmenu, and Localized<T> (menus.model.ts:241) replaces every LocalizeKey-typed field — including tooltip — with a resolved string, exactly like label. tab-dropdown-menu.component.tsx:84 and :95 render {item.tooltip && <TooltipContent>{item.tooltip}</TooltipContent>} from that same localized value, so a tooltip goes through the identical %key% mechanism as a label. No other test in the repo walks a menu document collecting tooltip keys; src/shared/utils/menu-document-combiner.localization.test.ts only asserts the resolution pipeline runs. Every menus.json repo-wide contains zero "tooltip" occurrences, and the suite passes 289/289 — nothing here would fail if a tooltip key were added undefined.
Other findings in this file: #10
src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts line 52 at r1 (raw file):
} function describeItem(menu: Menu, item: Item): string {
#24 - low · checked and confirmed
Changing a menu item's label in Simple's copy and not Power's leaves the two modes showing different wording for the same command with every test still green.
What happens: four commands now exist as two items each — for example platformScriptureEditor.changeView at menus.json:109 (hidden in Simple) and :253 (hidden in Power). describeItem (:53) reduces every served item to its command id, so both mode snapshots compare command ids and order only; a search for label across this test file returns zero hits, so no assertion anywhere reads one.
Why it matters: the duplication is the price of this design, so label drift between modes is the failure it invites, and it is invisible to review once the diff is large. The ADR names the hole itself in its Consequences: "it pins commands, not labels, so a label edited in one copy and not the other passes silently."
Fix: add a test to menu-data.service-host.scripture-editor-menu.test.ts that walks both modes' served items, builds command→label maps, and asserts any command present in both is served with the same label — it needs no new fixture, both menus are already built there.
How this was checked: describeItem at menu-data.service-host.scripture-editor-menu.test.ts:52-60 returns only item.command (or an id ▸ children string built from other describeItem calls) and never touches item.label; a search for label inside this test file returns zero hits, so no assertion anywhere in it reads a label. Exactly four commands are duplicated between the Simple and Power copies in extensions/src/platform-scripture-editor/contributions/menus.json — platformScriptureEditor.changeView (109/253), .toggleFootnotes (116/261), .changeFootnotesPaneLocation (123/269) and platformScripture.openFind — and today all four pairs share the identical literal label key, so nothing is drifted yet, but the test's toEqual arrays are built entirely from command ids and would not change if one copy's label key were edited. The file passes 5/5 today. The ADR documents this same hole in its Consequences section: "it pins commands, not labels, so a label edited in one copy and not the other passes silently."
Similar fix as #25.
Other findings in this file: #25
src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts line 67 at r1 (raw file):
* rule as `getMenuSectionsWithItems` in platform-bible-react. */ function describeSections(menu: Menu): [string, string[]][] {
#25 - low · checked and confirmed
The pinned "Power serves its established layout" snapshot omits any menu group that the real dropdown does render, whenever that group is keyed the same as its column.
What happens: the docstring at :63 says this mirrors getMenuSectionsWithItems, but the filter at :74 only accepts group.column === columnKey. Production's isGroupUnderColumnOrSubMenu (menu.util.ts:76-84) accepts a second case — groupKey === columnOrSubMenuKey — which TabDropdownMenu uses at tab-dropdown-menu.component.tsx:48-49 to pick a column's groups. A group keyed identically to its column therefore renders in the app and is invisible to both snapshots.
Why it matters: the file's stated job is to pin the served menu exactly, and a re-implementation that has drifted from the renderer cannot do that. No shipped menu relies on the second branch today — across all 10 shipped menus.json plus menu.data.json, 14 columns and 27 groups, no group's own key collides with any column key — so this is a latent divergence rather than an active blind spot, but nothing prevents the next contributed menu from tripping it.
Fix: in describeSections and itemsInGroup, replace the inline group.column === columnKey filter with a local helper restating both branches of isGroupUnderColumnOrSubMenu — ('column' in group && group.column === key) || groupKey === key — with a comment citing lib/platform-bible-react/src/components/advanced/menus/menu.util.ts and its isGroupUnderColumnOrSubMenu as the rule it must be kept in sync with. That symbol is not exported from platform-bible-react's public entry point or its exports map, so it cannot be imported here.
Similar fix as #24.
Other findings in this file: #24
extensions/src/platform-scripture-editor/src/show-panel.util.ts line 62 at r1 (raw file):
*/ export async function showTextCollectionTab(): Promise<string | undefined> { let shownId: string | undefined;
#14 - low · checked and confirmed
When the Text collection tab fails to open for a passing reason, the user is told the feature isn't available at all, and the real error is only visible at debug log level.
What happens: the try at :63 spans both the raise and the create inside showOrCreateTab, so it catches every rejection openWebView can produce — not only the missing-provider case it names. throwIfWindowIsClosing (web-view.service-shard.ts:3339), the router's window-routing errors (web-view.service-router.ts:690,726) and a LogError from a lost reuse race all land here, fall through :71-82, and raise "Text collection isn't available, so there's no tab to show." while the actual cause is written at logger.debug (:69), which a default log level does not record.
Why it matters: enableScriptureTextGrid defaults to true (settings.json:6), so the expected reason for this catch is the uncommon one; every other reason arrives mislabelled as a permanently unavailable feature and leaves no trace in the log.
Fix: narrow the catch with a substring test, not equality — getErrorMessage(e).includes('getWebView: Cannot find Web View Provider') — because the message that reaches this catch is wrapped as JSON-RPC Request error (<code>): getWebView: Cannot find Web View Provider for webview type <type>, not the bare string. Keep logger.debug plus the "isn't available" notification only when that substring is present; otherwise logger.warn plus a new generic "couldn't open the Text collection tab" notification, whose key must be added to both en and es in localizedStrings.json. Add a case in show-panel.util.test.ts that rejects with a non-provider error and asserts the generic-warning path, and update the existing provider-missing test at :79-89 to reject with the fully wrapped message so it exercises the substring match realistically rather than an exact one that cannot occur in production.
How this was checked: The try at show-panel.util.ts:63 wraps showOrCreateTab, which itself makes two papi.webViews.openWebView calls (the raise at :22-27, the create at :47) — a rejection from either lands in the same catch at :65, not only the provider-missing case its comment names. The provider-missing throw (src/renderer/services/web-view.service-shard.ts:2769), its window-closing guard (:3339), and the router's window-routing throws (src/main/services/web-view.service-router.ts:690,726) are all real, distinct, reachable throw sites, and every one would be swallowed into the same "Text collection isn't available" message while the real cause is written only at logger.debug, which a default log level drops. enableScriptureTextGrid defaults to true (extensions/src/platform-scripture-editor/contributions/settings.json:6), so the provider-missing case this catch is written for is the uncommon one in practice.
Similar fix as #12.
extensions/src/platform-scripture-editor/src/main.ts line 1145 at r1 (raw file):
* view, or `getOpenWebViewDefinition` could not be answered */ async function getProjectIdOfWebView(webViewId: string | undefined): Promise<string | undefined> {
#4 - medium · checked and confirmed
A newly opened Bible texts or Commentaries tab silently loses its project label with no test failing.
What happens: getProjectIdOfWebView (main.ts:1145) is the only thing that gives a newly created Bible texts / Commentaries tab its projectId, via the callbacks at main.ts:1446 and :1470. No test imports this main.ts, so mutating line 1146 to return undefined; unconditionally leaves the whole suite green. The identical helper in legacy-comment-manager/src/main.ts:189-202 got three tests (main.test.ts:127, :141, :155) covering resolve, throw-and-degrade and the warn; the editor's copy got none.
Why it matters: a tab opened for the wrong project, or none, is what the user sees the first time they use two of the five Tools items.
Fix: add an activate()-driven test beside show-panel.util.test.ts with a mocked @papi/backend covering getProjectIdOfWebView's three paths — webViewId absent, getOpenWebViewDefinition resolving a projectId, and it rejecting (asserting logger.warn and a { projectId: undefined } create) — mirroring legacy-comment-manager/src/main.test.ts:141-170. Budget for a larger mock surface than that reference: activate() here needs commands, webViewProviders, webViews, network, dataProviders, projectDataProviders, settings, dialogs, localization and projectLookup stubbed, plus the extension's eight ?inline asset imports and a no-op WebViewFactory base class.
How this was checked: getProjectIdOfWebView (extensions/src/platform-scripture-editor/src/main.ts:1145-1156, new in this change) is reachable only through the two panel commands registered at main.ts:1446 and :1470. A repo-wide search for any import of this main.ts found only extension-host-import-boundary.test.ts, which reads the file's text with readFileSync and regex-parses its import statements — it never imports or executes the module. show-panel.util.test.ts calls showOrCreateTab directly with literal web-view-type strings (:32, :46), so it never touches the panel commands or this helper. Confirmed directly by copying main.ts and its local dependency graph into an isolated scratch directory, building an activate()-driven harness with a mocked @papi/backend, and mutating getProjectIdOfWebView to return undefined; unconditionally: show-panel.util.test.ts's 6 tests stayed fully green under that mutation, while a new test exercising the three code paths went red.
Similar fix as #6.
Other findings in this file: #12
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.test.tsx line 219 at r1 (raw file):
}); it('opens a flyout and reaches its items by keyboard', async () => {
#16 - low · checked and confirmed
The same submenu fixture is written out three times in one test file — once as the named SUBMENU_MENU constant and twice more inline — so adding a field the submenu path needs means editing three literals.
What happens: SUBMENU_MENU is defined at :40-62 with the test.editSubmenu / test.editActions shape. The keyboard test at :224-253 and the tooltip-hover test at :276-299 each re-inline that same columns/groups/items structure, differing only by an extra Redo item and a tooltip field respectively. The two chevron tests at :315 and :328 reference SUBMENU_MENU by variable, so they are reuse rather than copies.
Why it matters: confined to one test file and the tests pass today, so the cost is maintenance rather than correctness — but the file already demonstrates the fix by extracting SUBMENU_MENU for the chevron tests.
Fix: replace the inline literals at :225 and :277 with spreads over SUBMENU_MENU: for the keyboard test, { ...SUBMENU_MENU, items: [...SUBMENU_MENU.items, redoItem] }; for the tooltip test, rebuild the items array with the first item overridden — { ...SUBMENU_MENU, items: [{ ...SUBMENU_MENU.items[0], tooltip: 'Edit actions' }, SUBMENU_MENU.items[1]] } — since a shallow spread over SUBMENU_MENU cannot add a field onto one of its nested items entries. Keep SUBMENU_MENU as the one place the submenu group wiring is spelled out.
Similar fix as #17.
Other findings in this file: #17
extensions/src/platform-scripture-editor/src/main.ts line 1443 at r1 (raw file):
); const showBibleTextsPanelPromise = papi.commands.registerCommand(
#12 - low · checked and confirmed
If opening the Bible texts or Commentaries tab fails, the menu item does nothing at all and the user is given no indication anything went wrong.
What happens: showOrCreateTab (show-panel.util.ts:39-48) has no error handling of its own, so a rejection from either openWebView call propagates out of the command. The only handler is the blanket .catch at the call site (platform-scripture-editor.web-view.tsx:3606-3614), which writes a logger.warn and returns. The sibling Text collection command deliberately does the opposite and notifies the user (show-panel.util.ts:71-82), so the three Tools items behave inconsistently on failure.
Why it matters: rare in practice, because in Simple mode both panels are pinned non-closable tabs that raiseExistingTab will always find; it bites on a closing-window race or after a layout restore that lost the tab. The cost is a menu item that silently does nothing, which reads as a broken build.
Fix: give showOrCreateTab's two main.ts callers the same treatment as showTextCollectionTab — wrap the create and send a generic "couldn't open the tab" warning notification on failure. That needs a new localized key in both en and es in extensions/src/platform-scripture-editor/contributions/localizedStrings.json, plus a case in show-panel.util.test.ts.
How this was checked: The two new commands (showBibleTextsPanel, showCommentariesPanel, main.ts:1443-1471) hand their promise straight to showOrCreateTab (show-panel.util.ts:39-48), which has no error handling of its own. A rejection from either papi.webViews.openWebView call inside it propagates to the only catch in the chain, the generic .catch at platform-scripture-editor.web-view.tsx:3606-3614, which does logger.warn and nothing else. Nothing else in the stack surfaces this to the user: there is no global command-error toast, and because the rejection is already caught here it never reaches the renderer's unhandledrejection listener (src/renderer/index.tsx:67) either. The sibling showTextCollectionTab in the same file (show-panel.util.ts:61-83) already warns the user on failure, so this is a checked inconsistency rather than a universal silent-failure pattern for the module.
Similar fix as #14.
Other findings in this file: #4
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.test.tsx line 325 at r1 (raw file):
}); it('points the submenu trigger chevron left in RTL, toward the flyout it opens', async () => {
#17 - low · checked and confirmed
Nothing fails if the RTL flyout stops opening on ArrowLeft, because the only RTL test checks which chevron icon is rendered.
What happens: the keyboard test at :219-245 genuinely exercises the keyboard path — focus the trigger, {ArrowRight}, a toHaveFocus assertion on the first flyout item, then {ArrowDown}{Enter} — but runs under the beforeEach LTR default (:36). The two RTL-aware tests at :312 and :325 assert only querySelector('.tabler-icon-chevron-left').
Why it matters: the chevron direction and the key that opens the flyout are two independent consequences of dir; the icon assertion passes even if the dir prop stops reaching DropdownMenuPrimitive.Root (dropdown-menu.tsx:80), which is what actually maps the arrow keys. Nothing in the shipped app currently sets the layoutDirection key that readDirection() reads, so no user session runs in RTL today and this gap cannot yet cause a defect anyone hits — it leaves the shared menu's RTL keyboard contract unverified for whenever direction plumbing reaches production.
Fix: add a case to tab-dropdown-menu.component.test.tsx that calls persistDirection('rtl'), focuses the sub-trigger, presses {ArrowLeft} and asserts the first flyout item has focus — the RTL twin of the test at :219.
Similar fix as #16.
Other findings in this file: #16
src/node/utils/locale-assets.test-helper.ts line 29 at r1 (raw file):
* that exist only in one of these must NOT count as shipped. */ export const DEV_ONLY_EXTENSION_NAMES: readonly string[] = [
#26 - low · checked and confirmed
A test-support file that the third-party-notices guard does not recognise as test-only is now an exported dependency of the file that was just renamed precisely to be recognised.
What happens: shipping-set.ts drops test support by two patterns — TEST_SUPPORT_FILE = /\.(?:test|spec|stories|test-harness)\./ at :1532 and !/\.test-(?:utils?|harness)\./ at :453. Neither matches .test-helper.: the first needs a literal .test., which the -helper suffix breaks, and the second recognises only -utils/-util/-harness. So locale-assets.test-helper.ts is scanned as shipping source. Today it imports only node:fs, node:path and a type-only platform-bible-utils, so nothing is reported. The sibling menu-data.service-host.test-helper.ts was renamed to .test-utils.ts for exactly this reason, and that renamed file now imports DEV_ONLY_EXTENSION_NAMES from this one.
Why it matters: no CI failure today — the trap fires when someone adds vitest or another devDependency import to this file, at which point the notices guard reports a shipping row that is not real.
Fix: rename src/node/utils/locale-assets.test-helper.ts to locale-assets.test-utils.ts and update all three importers — src/node/data/shipped-locale-assets.test.ts, src/extension-host/services/menu-data.service-host.test-utils.ts, and src/extension-host/data/language-details.data.test.ts — plus the file's own header comment and the mirroring note in extensions/src/platform-scripture-editor/src/localized-strings.test.ts. Widening the guard's regex to \.test-(?:utils?|harness|helper)\. is the smaller diff but leaves two spellings live.
How this was checked: .erb/scripts/third-party-notices/shipping-set.ts filters test-support files with two patterns: TEST_SUPPORT_FILE = /\.(?:test|spec|stories|test-harness)\./ at line 1532, and a second filter at line 453 (!/\.test-(?:utils?|harness)\./) inside importedPackages. Neither matches .test-helper. — the first needs a literal .test., which the -helper suffix breaks; the second recognises only -utils/-util/-harness. So src/node/utils/locale-assets.test-helper.ts is scanned as shipping source today, the same gap this change's own second commit fixed by renaming the sibling menu-data.service-host.test-helper.ts to .test-utils.ts. The file currently imports only node:fs, node:path and a type-only platform-bible-utils, so nothing trips the guard yet — a convention gap rather than a live CI failure.
Similar fix as #10.
extensions/src/platform-scripture-editor/contributions/menus.json line 131 at r1 (raw file):
{ "label": "%webView_platformScriptureEditor_toggleFootnotesAutoShow%", "hiddenInterfaceModes": ["simple"],
#3 - medium · checked and confirmed
In Simple mode the footnotes pane never opens by itself when a chapter has notes, and there is no longer any way to turn that behaviour on.
What happens: the auto-show toggle is hidden in Simple (menus.json:131) and the Simple View section (menus.json:253-275) carries only changeView, toggleFootnotes and changeFootnotesPaneLocation, so the command has no Simple entry point. Its state is per-web-view (platform-scripture-editor.web-view.tsx:918) and the effective value is footnotesAutoShowChoice ?? isPowerMode (:931) — off in Simple. Nothing else in the repo sends platformScriptureEditor.toggleFootnotesAutoShow.
Why it matters: unlike the Power-only tools, this is a stateful preference whose Simple default is exactly the disabled state, so the gate does not just hide a rarely-used tool — it forecloses the only path to the enabled state. The ticket does not mention this toggle anywhere, so 1.6e's Quality-checks-only authorisation does not cover it.
Fix: add a Simple copy of this item to the platformScriptureEditor.simpleViewLayout group with hiddenInterfaceModes: ["power"] and order: 4, mirroring the three existing View duplicates (menus.json:252-275); update the expected Simple layout in menu-data.service-host.scripture-editor-menu.test.ts:163-170 and drop the command from POWER_ONLY_COMMANDS (:236). Also update the Consequences prose in adr-menu-per-mode-layout-via-mode-gated-columns (Architecture-Decisions.md:2916-2917) to remove "Auto-show footnote pane" from the list of items with no Simple route, since it would no longer apply. If it is meant to stay Power-only, keep the gate and replace the POWER_ONLY_COMMANDS comment at :236 with the actual reason Simple should not have this toggle, rather than restating that it currently has no place for it.
How this was checked: At the merge-base, toggleFootnotesAutoShow had no hiddenInterfaceModes and was visible in both modes; this change adds "hiddenInterfaceModes": ["simple"] at menus.json:131 and gives it no counterpart in Simple's simpleViewLayout group, which holds only changeView/toggleFootnotes/changeFootnotesPaneLocation (menus.json:253-275). Reading platform-scripture-editor.web-view.tsx:918-937: the per-web-view state is footnotesAutoShowChoice (persisted via useWebViewState, default undefined), and the effective value is footnotesAutoShow = footnotesAutoShowChoice ?? isPowerMode — so in Simple it defaults to disabled, and with no menu route left to invoke the command a Simple user can never flip it to true. This differs from the Power-only tools, which are unreachable but otherwise inert: here the gate forecloses the only path to the enabled state. Nothing in the ticket's decisions, target structure or known limitations mentions this toggle.
Similar fix as #1.
Other findings in this file: #1, #8
extensions/src/platform-scripture-editor/contributions/menus.json line 138 at r1 (raw file):
{ "label": "%webView_platformScriptureEditor_charactersInventory%", "hiddenInterfaceModes": ["simple"],
#1 - high · checked and confirmed
In Simple mode a user can no longer open the Characters, Repeated Words, Markers or Punctuation inventories, the Markers Checklist, or the Checks side panel from anywhere in the app.
What happens: six items in the editor's Tools column gain hiddenInterfaceModes: ["simple"] (menus.json:136-178), so filterItemsForInterfaceMode (src/extension-host/services/menu-data.service-host.ts:35-40) drops them from the served Simple menu and getMenuSectionsWithItems then suppresses the whole platformScriptureEditor.tools column. At the merge-base none of the six was mode-gated. Nothing re-homes them into a Simple column, and no other menu contributes these commands.
Why it matters: this is a capability loss for every Simple user, not a relocation — changeView, toggleFootnotes and openFind were re-homed into simpleView/simpleTools, these were not. The ticket's one hiding authorisation (1.6e, Quality checks) covers exactly one of the six, platformScripture.openChecksSidePanel; the four inventories and Markers Checklist appear nowhere in the ticket, its target structure, its known limitations or its Definition of Done.
Fix: either (a) give each of the five unauthorised commands a Simple entry point — one item per command with hiddenInterfaceModes: ["power"] in a Simple group (e.g. a platformScriptureEditor.simpleInventory group under the simpleTools column, following the shape of the three simpleViewLayout duplicates already in the file) — then update the "Simple serves its shipped layout" expectation and remove those five from POWER_ONLY_COMMANDS (menu-data.service-host.scripture-editor-menu.test.ts:225-243), leaving openChecksSidePanel there since 1.6e authorises it; or (b) record an explicit product decision naming who approved dropping Simple's only route to the four inventories and Markers Checklist, replace those five POWER_ONLY_COMMANDS comments with that decision and owner (matching the substantive reasons already given for openManageBooks and legacyCommentManager.openCommentList in the same list), and update the Consequences prose in adr-menu-per-mode-layout-via-mode-gated-columns so it records a decision rather than an unexplained side effect. A TAB_FOR_COMMAND entry does not apply either way — these commands open floating or docked views, not a third-column tab.
How this was checked: At the merge-base copy of extensions/src/platform-scripture-editor/contributions/menus.json, none of the six items (charactersInventory, repeatedWordsInventory, markersInventory, punctuationInventory, markersChecklist, openChecksSidePanel) carried hiddenInterfaceModes, so Simple could reach them. This change adds "hiddenInterfaceModes": ["simple"] to all six at menus.json:137-178, and no Simple column or group re-homes any of them — Simple's simpleTools/simpleView columns hold only changeView/toggleFootnotes/changeFootnotesPaneLocation and the panel-raising items. filterItemsForInterfaceMode (menu-data.service-host.ts:35-40) drops any item whose hiddenInterfaceModes includes the current mode. A repo-wide search for all six command ids, plus the sibling paratext-bible-internal-extensions, paratext-bible-extensions and paratext-10-studio checkouts, finds no other menu, contribution or code path that reaches them — platform-scripture/src/main.ts only registers the commands. The ADR states the same outcome as a Consequence (Architecture-Decisions.md:2916-2917): "Simple now has no menu route at all to the four Inventories, Markers Checklist, Open Checks, or Auto-show footnote pane."
Similar fix as #3.
Other findings in this file: #3, #8
extensions/src/platform-scripture-editor/contributions/menus.json line 212 at r1 (raw file):
"order": 3.5 }, {
#8 - low · checked and confirmed
In Simple mode the Project menu shows "Ctrl+F" beside Tools ▸ Find but nothing beside Edit ▸ Undo or Redo, even though Ctrl+Z and Ctrl+Y work in that editor and the catalog documents both.
What happens: addShortcutHints (menu-data.service-host.ts:48-56) attaches a hint only when a menu item's command matches a catalog entry's command. The editor-undo and editor-redo entries (keyboard-shortcuts.data.ts:657-676) have none, so the new Undo/Redo flyout items join to nothing while platformScripture.openFind two sections below joins to ⌃F/Ctrl+F.
Why it matters: every Simple user opening the new Edit flyout sees this, and the inconsistency sits inside one menu. It is a structural limit rather than an oversight: KeyboardShortcutEntry.command is typed CommandNames (keyboard-shortcuts.model.ts:41), and these five ids are deliberately never registered as PAPI commands, so no command can be set on those entries at all. The chords themselves still work — only the hint is absent.
Fix: record the limit rather than declaring commands PAPI never registers, which would make the .d.ts lie: add to the Consequences section of adr-menu-per-mode-layout-via-mode-gated-columns in .context/standards/Architecture-Decisions.md that web-view-handled menu ids cannot carry catalog hints, because KeyboardShortcutEntry.command is typed to registered commands. No keyboard-shortcuts.data.test.ts change is owed, since no command is being added.
Other findings in this file: #1, #3
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 89 at r1 (raw file):
} return (
#18 - low · checked and confirmed
The tooltip wiring for submenu triggers is rewritten for every consumer of TabDropdownMenu, although no menu item this change ships carries a tooltip.
What happens: the command branch and the submenu branch of getGroupContent are split (tab-dropdown-menu.component.tsx:56-113) so the tooltip attaches to DropdownMenuSubTrigger rather than to DropdownMenuSub. The Edit flyout item (menus.json:203-209) declares no tooltip, so the shipped menu renders identically either way; the behaviour is observable only through the new fixture-built test. The sibling RTL-chevron change in dropdown-menu.tsx:353-378 is not in this class — the flyout opens leftward in RTL, so the old hardcoded right chevron pointed away from it.
Why it matters: it widens the blast radius of a menu-content change into a shared component used by every tab menu, and a reviewer of the Simple-menu change has no way to tell the component change is not load-bearing for it.
Fix: keep the fix, and call it out in the PR description as a separate fix to the shared menu component with its own test, distinct from the RTL chevron change which this feature does depend on.
How this was checked: The Edit-flyout submenu item at extensions/src/platform-scripture-editor/contributions/menus.json:203-209 (platformScriptureEditor.editSubmenu) declares no tooltip field. A repo-wide sweep of every menus.json and src/extension-host/data/menu.data.json for a "tooltip" entry on any item returns zero hits anywhere in the codebase, so no menu item shipped by this change or any other extension currently uses the field. The two other real consumers of TabDropdownMenu (tab-toolbar.component.tsx, tab-floating-menu.component.tsx) do not reference tooltip either, so the change is not load-bearing for them today. The diff at tab-dropdown-menu.component.tsx:56-113 confirms the rewrite touches both the command branch and the submenu branch of a shared component used by every tab menu in the app, for a behaviour no shipped item currently exercises.
Similar fix as #19.
Other findings in this file: #19, #20
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 91 at r1 (raw file):
return ( <DropdownMenuSub key={`dropdown-menu-sub-${item.label}-${item.id}`}> <Tooltip>
#19 - low · checked and confirmed
With the Edit flyout open, its trigger row reports data-state="closed" and data-slot="tooltip-trigger" instead of the submenu's own open state and slot.
What happens: TooltipTrigger asChild (tooltip.tsx:47-58) clones data-slot="tooltip-trigger" and its own data-state onto DropdownMenuSubTrigger, which writes data-slot/data-inset before {...props} (dropdown-menu.tsx:360-378), so the clone wins. Opening the flyout with ArrowRight gives data-state="closed", data-slot="tooltip-trigger", aria-expanded="true". The wrapper is applied unconditionally, so an item with no tooltip is affected too.
Why it matters: DropdownMenuSubTrigger's own tw:data-open:bg-accent (dropdown-menu.tsx:371) resolves to [data-state="open"], which can never match while data-state is pinned to "closed", so those classes are dead and any styling or test written against [data-state=open] or [data-slot=dropdown-menu-sub-trigger] silently misses. The visible open-highlight still works, because DropdownMenuContent's selector keys on [data-slot$="-trigger"] and aria-expanded, and "tooltip-trigger" also ends in -trigger.
Fix: in tab-dropdown-menu.component.tsx, render the Tooltip/TooltipTrigger wrapper around DropdownMenuSubTrigger only when item.tooltip is set, mirroring the {item.tooltip && <TooltipContent>…} guard already used for the content — this alone fixes every case that exists in the repo today. If a future submenu item is expected to carry both a submenu and a tooltip, additionally harden DropdownMenuSubTrigger in dropdown-menu.tsx with a // CUSTOM: comment explaining that an asChild TooltipTrigger clones its own data-slot/data-state onto this element, so (a) data-slot="dropdown-menu-sub-trigger" must be re-asserted after {...props}, and (b) any inbound data-state must be explicitly deleted from props before spreading — not merely reordered, since this component never writes an explicit data-state of its own — so Radix's real open/closed value survives; add a test rendering a submenu item with tooltip set, opening its flyout, and asserting data-state="open" and data-slot="dropdown-menu-sub-trigger" on the trigger.
How this was checked: Verified by rendering. A probe using this change's own fixture shape (a submenu item with no tooltip, matching the shipped Edit item) through TabDropdownMenu, opened with ArrowRight, read the live sub-trigger element: data-state="closed", data-slot="tooltip-trigger", aria-expanded="true". The mechanics: TooltipTrigger asChild (tooltip.tsx:47-58) clones its own data-slot/data-state via Radix's Slot merge, and DropdownMenuSubTrigger (dropdown-menu.tsx:360-378) writes data-slot/data-inset before {...props}, so the clone wins; the wrapper (tab-dropdown-menu.component.tsx:91-96) is applied regardless of item.tooltip. tw:data-open:bg-accent (dropdown-menu.tsx:371) traces to shadcn's @custom-variant data-open, which matches [data-state="open"] — with data-state pinned to "closed" that class genuinely can never apply. The consequence is narrower than a first read suggests: DropdownMenuContent's highlight selector (dropdown-menu.tsx:137) still matches because "tooltip-trigger" also ends in -trigger and aria-expanded was not clobbered, so the visible open highlight still renders; only the sub-trigger's own data-open:* classes are dead.
Similar fix as #18.
Other findings in this file: #18, #20
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 98 at r1 (raw file):
</Tooltip> <DropdownMenuPortal>
#20 - low · checked and confirmed
A flyout row whose every child is hidden in the current interface mode still opens, showing an empty box the user can arrow into and get nothing from.
What happens: getMenuSectionsWithItems drops a column with no items (menu.util.ts:101-113) but counts a submenu parent regardless of its contents (menu.util.ts:96), so getGroupContent renders the DropdownMenuSub at tab-dropdown-menu.component.tsx:89 and the recursive call at line 100 returns an empty array into DropdownMenuSubContent. A probe with a menu holding only a submenu parent and no items in its group opened the trigger with ArrowRight and found two role="menu" elements — the second an empty data-slot="dropdown-menu-sub-content" panel.
Why it matters: the suppression rule that keeps a column from heading nothing does not cover flyouts. It cannot occur today: menu.util.ts is byte-identical at the merge-base, and this change's own submenu is symmetric — editSubmenu and all five Edit children carry the same hiddenInterfaceModes: ["power"], so they show and hide together. It becomes reachable the moment any future hiddenInterfaceModes or permission gating is applied to submenu children independently of their parent.
Fix: in getGroupContent (tab-dropdown-menu.component.tsx:89) compute the submenu's children before rendering and return null for that map entry when the list is empty — the recursive call already returns [] rather than undefined, and null entries render as nothing through the surrounding flatMap without needing a key. Add the emptiness check to menu.util.ts as a helper the way getMenuSectionsWithItems covers columns, and wire it into platform-menubar.component.tsx:90-100 as well — that renderer has the identical submenu branch, so adding the helper alone does not make "both renderers share one rule" true. Cover it with a test asserting no menuitem named for the parent when its group has no items.
Other findings in this file: #18, #19
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 102 at r1 (raw file):
* TOOLS item and an entry here. */ const TAB_FOR_COMMAND: Record<string, string> = {
#6 - medium · checked and confirmed
Every test stays green when the Simple Tools menu's "Bible texts" item is wired to raise the Commentaries tab instead.
What happens: main.ts:1446 pairs platformScriptureEditor.showBibleTextsPanel with BIBLE_TEXTS_PANEL_WEBVIEW_TYPE, and :1470 pairs showCommentariesPanel with COMMENTARIES_PANEL_WEBVIEW_TYPE. No test imports that main.ts — show-panel.util.test.ts:32 and :48 pass the web view type in themselves as an argument, so they pin nothing about the wiring. TAB_FOR_COMMAND restates the pairing by hand and is compared only against menus.json and the static layout data, never against what the registered handlers actually open, so swapping the two constants leaves the order test green.
Why it matters: the two Bible-resource panels are adjacent Tools items; raising the wrong one is the failure a user hits on every click.
Fix: add an activate()-driven test in extensions/src/platform-scripture-editor/src (beside show-panel.util.test.ts) with a mocked @papi/backend — the shape legacy-comment-manager/src/main.test.ts:127-135 uses — that invokes the registered showBibleTextsPanel and showCommentariesPanel handlers and asserts each one's openWebView call names its own web view type (platformScriptureEditor.bibleTexts / platformScriptureEditor.commentaries), so swapping the two constants at main.ts:1446/:1470 fails that test directly. Leave TAB_FOR_COMMAND as the hand-maintained mirror it already is: core cannot import extension source, as that file notes at :208, so its accuracy stays a by-eye review concern rather than something derivable from or assertable against the registrations.
How this was checked: The showBibleTextsPanel/showCommentariesPanel registrations (main.ts:1443-1472, new in this change) pair each command with its web-view-type constant only inside main.ts, which no test imports. TAB_FOR_COMMAND (shipped-simple-layout-order.test.ts:102-108, also new) is a hand-typed dictionary compared only against the menu document and the static layout data, never against what the registered handlers actually open; that same file notes at :208 that "core cannot import extension source", which is why the gap exists structurally rather than by omission. In an isolated copy, swapping BIBLE_TEXTS_PANEL_WEBVIEW_TYPE and COMMENTARIES_PANEL_WEBVIEW_TYPE at main.ts:1446/:1470 left show-panel.util.test.ts fully green and left TAB_FOR_COMMAND's comparison unaffected by construction, while a new test asserting each handler's actual openWebView call caught the swap.
Similar fix as #4.
extensions/src/legacy-comment-manager/src/main.ts line 180 at r1 (raw file):
* @returns The Comments web view's ID, or `undefined` if it couldn't be shown */ async function showCommentListPanel(
#7 - low · checked and confirmed
The raise-an-open-tab-else-create-one behaviour behind the four new Simple Tools items exists as three independent copies, so a change to how a panel is raised has to be made in three files or the Comments item silently behaves differently from its neighbours.
What happens: show-panel.util.ts:21-48 extracts the shape as raiseExistingTab + showOrCreateTab, and platform-scripture-editor/src/main.ts:1145-1156 extracts the project lookup as getProjectIdOfWebView. legacy-comment-manager/src/main.ts:180-204 then re-implements both inline — the same { existingId: '?', createNewIfNotFound: false, bringToFront: true } bag, the same if (existingId) return existingId short-circuit, and a near-verbatim copy of the degrade-to-no-project-id comment. The four Tools items sit in one menu section and are meant to behave identically.
Why it matters: all three functions are new in this change — none exists at the merge-base — so this is duplication introduced across three sites at once, not an inherited pattern. Nothing enforces the agreement: shipped-simple-layout-order.test.ts pins the order of the Tools section, not the behaviour behind each item, so a future change to the raise policy in showOrCreateTab would leave Comments out of step with no test failing.
Fix: hoist the options bag in legacy-comment-manager/src/main.ts into a named module constant the way extensions/src/platform-scripture/src/main.ts:390 does with REUSE_EXISTING_FIND_ONLY, and point its TSDoc at showOrCreateTab as the shape it mirrors, so the duplication is declared rather than accidental. A shared helper is the better end state but must live somewhere both extensions can import.
How this was checked: legacy-comment-manager/src/main.ts:180-204 (showCommentListPanel) reimplements the exact raise-or-create shape that show-panel.util.ts:21-48 and platform-scripture-editor/src/main.ts:1145-1156 already factor out: the identical { existingId: '?', createNewIfNotFound: false, bringToFront: true } options bag, the identical if (existingId) return existingId short-circuit, and a near-verbatim copy of the try/catch comment — compare main.ts:191-193 ("degrade to 'no project id' so a new Comments tab still opens, unlabeled, rather than rejecting the whole show request") with platform-scripture-editor/src/main.ts:1147-1149. None of the three functions exists at the merge-base — show-panel.util.ts is not there at all — so this is duplication this change introduces across three sites. shipped-simple-layout-order.test.ts only pins the Tools section's item order, not the raise-vs-reload behaviour behind each item, so nothing would fail if showOrCreateTab's policy changed without a matching edit to the Comments copy.
extensions/src/platform-scripture-editor/src/editor-side-effects.utils.test.ts line 61 at r1 (raw file):
}); it('uses a message key distinct from the sync-blocked notice', () => {
#9 - low · checked and confirmed
This test can only fail if someone literally sets two adjacent string constants to the same value, so it tells a future reader nothing about what the Edit-flyout notice does.
What happens: EDIT_ACTION_BLOCKED_KEY (editor-side-effects.utils.ts:68-69) and SYNC_EDIT_BLOCKED_KEY (:26-27) are two literal LocalizeKey constants declared about 40 lines apart. The assertion at :62 compares them and nothing else — no function is called, no behaviour exercised. The behaviour that matters, which of the two messages a blocked action shows, lives in menuCommandHandler's notifyBlocked branch at platform-scripture-editor.web-view.tsx:3559-3560, which this file does not cover.
Why it matters: modestly. EDIT_ACTION_BLOCKED_KEY was added in this change by copy-pasting the existing SYNC_EDIT_BLOCKED_KEY block, and forgetting to change the copied suffix is exactly what this assertion catches — the file's other three tests reference the constants by name, so they would not. It is a cheap copy-paste tripwire sitting alongside three tests that do drive real behaviour.
Fix: delete the test at :61-63. notifyBlocked is a closure inside menuCommandHandler in platform-scripture-editor.web-view.tsx (:3556-3560) that no test imports or can reach, so asserting which message it sends is not possible from this file without first extracting that branch into a testable function, which is out of scope here. If that extraction happens, add the message-selection test at the new site; until then keep only the three tests that already exercise notifyEditMenuActionBlocked and notifySyncEditBlocked directly (:32-59, :69-80).
lib/platform-bible-react/src/components/shadcn-ui/dropdown-menu.tsx line 356 at r1 (raw file):
// CUSTOM: Use menu context to apply variant-driven styles const context = useMenuContext(); // CUSTOM: In RTL, Radix opens the submenu to the left and maps ArrowRight to close, so the
#21 - low · checked and confirmed
The new left-pointing submenu chevron never appears in the running application — only in Storybook and tests.
What happens: readDirection() returns 'ltr' unless localStorage['layoutDirection'] holds 'rtl' (dir-helper.util.ts:7-13). A sweep across this repo and six sibling repos for persistDirection and the literal layoutDirection finds writers only in .storybook/preview.ts:77, two test files and two Storybook stories — no shipped renderer, extension or service ever writes the key. The read is also non-reactive: a plain call in the render body, the same shape every other consumer uses, so even if something wrote it the chevron would not flip until the trigger re-renders.
Why it matters: the RTL half of this change is verified only by the Storybook and test path; it does not change what an RTL user sees today, which is worth stating so it is not read as shipping RTL support.
Fix: no change required at this site. Any future fix must make readDirection()'s value flow from whatever sets the interface language and re-render its consumers — a shared hook or context consumed by dropdown-menu.tsx, menubar.tsx and select.tsx alike — rather than each component calling readDirection() at render.
How this was checked: readDirection() (lib/platform-bible-react/src/utils/dir-helper.util.ts:7-13) returns 'rtl' only when localStorage.getItem('layoutDirection') === 'rtl', otherwise 'ltr' — a plain synchronous read in the render body, not a subscription. A repo-wide search for the bare identifier persistDirection and the literal layoutDirection across paranext-core plus the sibling paratext-10-studio, paratext-bible-extensions, paratext-bible-internal-extensions, scripture-editors, platform-bible-sample-extensions, paranext-extension-template and paranext-multi-extension-template repos turns up writers only in .storybook/preview.ts:77, tab-dropdown-menu.component.test.tsx, navigation-history-buttons.component.test.tsx, and two Storybook stories — every other hit is a mirrored copy of the same repo file, not an independent writer. There is no settings UI, command or other code path that writes this key. .context/standards/Localization-Guide.md:363-365 independently documents readDirection() as reading "the user's global UI direction preference", separate from per-project content direction, confirming this is a real named concept with no way to set it today.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 3560 at r1 (raw file):
// so it runs here rather than as a PAPI command if (isEditMenuCommand(projectMenuCommand.command)) { // Sync blocks editing for a named, transient reason; every other block is durable and
#13 - low · checked and confirmed
A user with no Scripture-edit permission who picks Edit ▸ Paste while a scheduled Send/Receive is running is told editing is paused for the Send/Receive, which implies it will work once the sync finishes — it will not.
What happens: isReadOnlyEffective (:1040-1058) ORs the durable reasons — no permission, project not editable, markers view — with the transient isSyncBlocked, so runEditMenuAction returns false for either. notifyBlocked (:3560-3563) then branches on isSyncBlocked alone, so whenever a sync is in flight the sync message wins over a durable block. isSyncBlocked is driven by auto-sync-edit-block-driver.ts, whose isWebViewBlocked keys only on projectId (:76-87), independent of the viewing user's own permissions.
Why it matters: narrow — it needs a permission-less or non-editable project and an automatic Send/Receive in flight — but auto-sync runs on a schedule, so a reviewer-role user hits it by chance and is told to wait for something that will not help.
Fix: in menuCommandHandler, make notifyBlocked prefer the durable message — isDurablyReadOnly ? notifyEditMenuActionBlocked(localizedStrings) : isSyncBlocked ? notifySyncEditBlocked() : notifyEditMenuActionBlocked(localizedStrings) — using the existing isDurablyReadOnly at :1162, add it to the useCallback dep array at :3616-3623, and update the comment at :3560-3561 to state the durable-over-transient priority. Cover the durable-block-during-a-sync case with a test.
How this was checked: isReadOnlyEffective (platform-scripture-editor.web-view.tsx:1040-1058) ORs the durable reasons (no permission, project not editable, markers view) together with the transient isSyncBlocked, but notifyBlocked (:3560-3563) branches on isSyncBlocked alone. isSyncBlocked is driven by src/renderer/services/auto-sync-edit-block-driver.ts, which flags every open edit-blockable web view of a project under an automatic Send/Receive independent of the viewing user's own permissions (isWebViewBlocked keys only on projectId, :76-87). So a permission-less or non-editable-project viewer who tries an Edit action while that project's automatic sync happens to be running is told editing is paused for the sync — which will not help once the sync finishes, since the real block is durable. isDurablyReadOnly (:1162-1169) is exactly the non-transient set the branch needs and is already in scope but unused there. The adjacent comment at :3560-3561 explains why sync alone gets a named message but says nothing about which message should win when a durable reason and a sync both hold.
Similar fix as #5.
Other findings in this file: #5
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 3564 at r1 (raw file):
const notifyBlocked = () => isSyncBlocked ? notifySyncEditBlocked() : notifyEditMenuActionBlocked(localizedStrings); const editor = editorRef.current;
#5 - medium · checked and confirmed
Choosing Edit ▸ Paste while the project is read-only shows no notification at all, and no test fails.
What happens: runEditMenuAction and notifyEditMenuActionBlocked each have their own unit test, but nothing tests the code that joins them, and no test imports platform-scripture-editor.web-view.tsx. Deleting if (!ran) notifyBlocked(); at :3580, inverting the isSyncBlocked ternary at :3562-3563, or dropping requestAnimationFrame(() => editorRef.current?.focus()) at :3585 all leave the suite green. The editorRef-absent branch at :3564-3571 and the restoreSelectionIfLost call at :3572 are likewise unexercised.
Why it matters: the silent no-op is the whole point of the read-only gate — runEditMenuAction returning false is invisible to the user unless this branch fires.
Fix: extract the branch into edit-menu-actions.util.ts as handleEditMenuCommand(command, editor: EditMenuTarget, state: { isReadOnlyEffective, isDurablyReadOnly, isSyncBlocked }, effects: { notifyEditMenuActionBlocked, notifySyncEditBlocked, restoreSelectionIfLost, focusEditor }), passing restoreSelectionIfLost and focusEditor as callbacks — matching the pattern already used at :2015-2017 — rather than widening editor's type, since EditMenuTarget is Pick<EditorRef, 'undo'|'redo'|'cut'|'copy'|'paste'> and has neither .focus() nor getSelection/setSelection. Inside the helper notifyBlocked must prefer isDurablyReadOnly over isSyncBlocked, not proxy today's check. Keep runEditMenuAction and the requestAnimationFrame refocus synchronous and un-awaited exactly as today so the clipboard call stays inside the click's user-activation window. Test all four outcomes — ran, blocked-by-sync, blocked-generic, no-editor — in edit-menu-actions.util.test.ts, and keep web-view.tsx calling the helper so the extraction is what ships.
How this was checked: A search for the bare basename platform-scripture-editor.web-view across every test file in the extension found zero test files importing it; running edit-menu-actions.util.test.ts and editor-side-effects.utils.test.ts (16 tests, all pass) confirms those two files exercise runEditMenuAction and the notify helpers in isolation, never the branch in menuCommandHandler that joins them (platform-scripture-editor.web-view.tsx:3556-3587). Because nothing imports this file, any single-line change inside the branch — deleting if (!ran) notifyBlocked(); at :3580, inverting the ternary at :3562-3563, or dropping requestAnimationFrame(() => editorRef.current?.focus()); at :3585 — leaves the full suite green. The editorRef-absent branch (:3564-3571) and the restoreSelectionIfLost call (:3572) are likewise unexercised.
Similar fix as #13.
Other findings in this file: #13
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 made 24 comments and resolved 23 discussions.
Reviewable status: 0 of 48 files reviewed, all discussions resolved.
a discussion (no related file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
A review of the Simple Project-menu regroup at
f2ff018, covering the 31 changed source files. The regeneratedlib/platform-bible-react/dist/bundle was skipped as build output.27 findings follow — one high, five medium, the rest low. Each carries the severity it settled at and a short status: checked and confirmed means the finding was verified against the code, and needs a human call means it could not be settled either way and the judgement is yours.
23 of them are inline comments on the files below. The remaining four are collected here, because they sit in files this pull request does not change and so have no line in this review to attach to.
The one high finding (inline, on
menus.json) is a product question rather than a code one, so it is worth reading first.
Findings in files this pull request does not change
#15 — low · checked and confirmed
lib/platform-bible-react/src/components/advanced/menus/platform-menubar.component.tsx:89A submenu item in the application menubar that declares a
tooltipshows no tooltip on hover and announces no description to a screen reader.What happens:
getMenubarContentwraps every item in oneTooltipTrigger asChildatplatform-menubar.component.tsx:69-70, and for a submenu the child isMenubarSub(:90), whichmenubar.tsx:283renders asMenubarPrimitive.Sub— a context provider with no DOM node — so the cloned props never reachMenubarSubTrigger.Why it matters: this is the same defect fixed in
tab-dropdown-menu.component.tsx, applied at only one of the two menu renderers, so the application menubar keeps silently dropping submenu descriptions. It affects no shipped UI today: this file is byte-identical at the merge-base, so it is pre-existing rather than introduced, and nomenus.jsonormenu.data.jsonin this repo or the three sibling extension repos usestooltipon any item.Fix: restructure
getMenubarContentthe same way astab-dropdown-menu.component.tsx:62-104— early-return the'command' in itembranch with its ownTooltip, and in the submenu branch put<Tooltip><TooltipTrigger asChild><MenubarSubTrigger>…insideMenubarSub; add a case toplatform-menubar.component.test.tsxasserting the tooltip text appears for a submenu item, mirroringshows a submenu item's tooltip on hoverattab-dropdown-menu.component.test.tsx:272.#22 — low · checked and confirmed
lib/platform-bible-react/src/components/shadcn-ui/menubar.tsx:45In an RTL interface the application menubar's submenu opens to the right of its parent and ArrowLeft does not open it, while the tab menu's flyout correctly opens to the left.
What happens:
DropdownMenureadsreadDirection()and passesdirto the Radix root (dropdown-menu.tsx:76-92), which is what makes the new chevron logic and the RTL key mapping agree.MenubarPrimitive.Root(menubar.tsx:45-52) receives nodirandmenubar.tsxnever importsreadDirection, so Radix falls back toltr: the flyout opens right, ArrowRight opens it, and the hardcodedIconChevronRightatmenubar.tsx:310is consistent with that wrong behaviour.context-menu.tsxhas the same shape (hardcoded chevron at:131, nodir), andContextMenuSubdoes ship in production —src/renderer/components/docking/platform-tab-title.component.tsx:174uses it for the tab right-click "Move to window" submenu, as do two components underextensions/src/platform-enhanced-resources/src/components/dictionary-tab/— so the same defect reaches users there too.Why it matters: only reachable where layout direction is set to RTL, which nothing in the shipped app currently does, so this is a correctness gap rather than a shipping regression — but changing only
menubar.tsx:310to followreadDirection()would make the arrow point away from where the flyout actually opens.Fix: in
menubar.tsx, addconst dir: Direction = readDirection();toMenubarand passdir={dir}toMenubarPrimitive.Rootbefore makingMenubarSubTriggerpick its chevron the wayDropdownMenuSubTriggerdoes; annotate both edits with// CUSTOM:comments per the shadcn convention, and add LTR/RTL chevron cases toplatform-menubar.component.test.tsxmirroringtab-dropdown-menu.component.test.tsx:313-337.How this was checked: Side-by-side confirms the asymmetry:
DropdownMenuindropdown-menu.tsx:76-92computesdir: Direction = readDirection()and passesdir={dir}toDropdownMenuPrimitive.Root, whileMenubarinmenubar.tsx:26-55— unchanged by this change, with zero diff between merge-base and head — passes nodirand the file never importsreadDirection.MenubarSubTrigger(menubar.tsx:288-313) hardcodes<IconChevronRight>with no RTL branch. In Radix, an unsetdironMenubarPrimitive.Rootdefaults toltr, so a submenu opens rightward and the wrong arrow key drives it.ContextMenuSubshares the same shape and does ship in production —src/renderer/components/docking/platform-tab-title.component.tsx:174(the tab right-click "Move to window" submenu) and two components underextensions/src/platform-enhanced-resources/src/components/dictionary-tab/— so the footprint reaches users there as well. Nothing in the shipped app can currently setlayoutDirectionto'rtl', so this asymmetry has no observable effect on a real user today.#23 — low · checked and confirmed
src/extension-host/data/menu.data.json:127Simple's Project section reads "Open Project Settings…" where the design says "Project settings".
What happens: the item comes from
defaultWebViewTopMenu(menu.data.json:127-133), which every web view's top menu inherits throughincludeDefaults: true(menus.json:9);hiddenInterfaceModeslives on items only, and the combiner merges arrays by plain concat with no id- or command-based override (document-combiner.ts:359-366), so there is no way to give Simple a different label for that one item without forking the shared defaults or shipping a visible duplicate.Why it matters: modestly.
menu.data.jsonis byte-identical at the merge-base, so this change inherits the mismatch rather than creating it, and the trade-off is already stated in the ticket — it is simply not transcribed into any repo artifact, so a later reader comparing the shipped menu with the design re-derives it from scratch. The served-menu test pins the command id, not the label.Fix: add a one-line note to the
platform.openSettingsitem'slocalizeNotesinmenu.data.jsonrecording that the Simple design calls this "Project settings" but the item is served from the shareddefaultWebViewTopMenuused by every mode and every web view, so it cannot be relabelled for Simple alone without either a schema change to gate labels per mode or duplicating the item. Do not rename the shared localization string:%webView_openProjectSettings%backs every web view's menu, Power's label must not change peradr-menu-per-mode-layout-via-mode-gated-columns, and the existing key is immutable underLocalization-Guide.md's "Existing Strings Are Immutable" rule — a genuine reword would need a new key with ametadata.jsonfallbackKey, which still would not make the label mode-specific.#27 — low · checked and confirmed
src/shared/data/keyboard-shortcuts.data.ts:471Simple's Project menu shows a shortcut hint on only two items — Find and Insert comment — while Switch Scripture view, Show footnotes, Insert footnote, Insert cross-reference and Project settings show none.
What happens:
menu-data.service-host.ts:45-56attaches a hint only whengetShortcutHintForCommandfinds a catalog entry with a matchingcommand; justplatformScripture.openFind(:452) andplatformScriptureEditor.insertCommentAtSelection(:468) have one, and a test inkeyboard-shortcuts.data.test.tsasserts that list is exhaustive. ⌃T/⌃⇧T are catalogued but deliberatelycommand-less (:477,:490); ⌃J, ⌃E and an editor F7 are not catalogued at all.Why it matters: every Simple user sees a menu that teaches two shortcuts where the design teaches five. Worse for ⌃J, ⌃E and F7: no handler for them exists in the editor at all — the only F7 handler in the tree belongs to a different extension — so those three chords do not work, rather than merely lacking a hint. Nothing in the repo points at the follow-up: a search for
PT-4735returns zero matches anywhere in the tracked source. The anchor line predates this change (the hint mechanism came with PT-4532); what this change does is surface these items together in one Simple menu alongside two that do show hints, making the inconsistency visible for the first time.Fix: add
// TODO(PT-4735): ...beside thescripture-insert-footnoteandscripture-insert-cross-referenceentries insrc/shared/data/keyboard-shortcuts.data.ts, naming the chords still to be catalogued, so the deferral is searchable from the code. Adding anycommandlater also needs its row inEXPECTED_MENU_HINTSinkeyboard-shortcuts.data.test.ts.How this was checked:
keyboard-shortcuts.data.test.ts's ownEXPECTED_MENU_HINTSmap (:20-32) lists exactly two commands with a catalogcommand—platformScripture.openFindandplatformScriptureEditor.insertCommentAtSelection— and a test there asserts every catalog entry with acommandmatches that list, so no other item in any menu anywhere in the bundled app can carry a hint; the file passes 6/6 today. Thescripture-insert-footnote/scripture-insert-cross-referenceentries (keyboard-shortcuts.data.ts:471-495) carry ⌃T/⌃⇧T but are deliberatelycommand-less, each with an inline comment saying so. No catalog entry exists at all for ⌃J or the editor's ⌃E/F7; a repo-wide search for a keydown handler onj/efound none, and the onlyF7handler in the tree lives in a different extension (extensions/src/platform-enhanced-resources/src/web-views/enhanced-resource.web-view.tsx:2913), not the scripture editor — so those three chords have no working handler in the editor at all, not merely no hint. A repo-wide search forPT-4735returned zero matches in the tracked source tree. The anchor line itself is unchanged by this change; the gap predates it and comes from the earlier hint mechanism, but this change is what surfaces the affected items together in one visible Simple menu alongside two that do show hints.(AI-assisted, with my guidance)
#15 — Pre-existing and not reachable today (no menu uses tooltip), so I'd rather not widen this PR into the menubar. Happy to file a follow-up ticket.
#22 — Pre-existing and unreachable until something sets layoutDirection; same follow-up as #15.
#23 — Done. The openSettings localizeNotes now records why Simple can't relabel it.
#27 — Done. Added TODO(PT-4735) above the insert-footnote entry, naming the chords still missing.
.context/standards/Entry-Point-Guide.md line 131 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#2 - medium · checked and confirmed
A developer following
Entry-Point-Guide.mdto add a menu item is told "Commands must match exactly what's registered in main.ts", while five shipped menu items name commands thatmain.tsnever registers.What happens: the Edit flyout's items declare
platformScriptureEditor.undo/.redo/.cutSelection/.copySelection/.pasteAtSelection(menus.json:218-253). None is passed topapi.commands.registerCommandinextensions/src/platform-scripture-editor/src/main.tsand none is declared intypes/platform-scripture-editor.d.ts; they are intercepted byisEditMenuCommandinsidemenuCommandHandler(platform-scripture-editor.web-view.tsx:3555-3559). The ADR records this as a deliberate second route, but the promotion intoEntry-Point-Guide.mdcovered only the per-mode-columns half (:131-132), leaving the contradicting bullet at:108untouched.Why it matters: the standards are what the next developer and the next agent read; the decisions log keeps the why. Anyone adding an editor menu item now gets contradictory guidance from one page, and no other standard documents the second route.
Fix: amend the
### Menu Item Structurenote atEntry-Point-Guide.md:108to state the second route — a menu command id handled by the web view'sSelectMenuItemHandlerrather than registered inmain.ts— and cross-referenceadr-menu-per-mode-layout-via-mode-gated-columnsbeside it, namingEDIT_MENU_COMMANDSinedit-menu-actions.util.tsas the list that keeps the menu and the interception in step.How this was checked:
Entry-Point-Guide.md:108(unchanged by this change, under "### Menu Item Structure") states flatly, as a Notes bullet with no hedging or scoping, "Commands must match exactly what's registered in main.ts." The Edit flyout's five commands (menus.json:218-253) are absent from everypapi.commands.registerCommandcall inextensions/src/platform-scripture-editor/src/main.ts(16 registrations, none of these five) and absent fromtypes/platform-scripture-editor.d.ts; they are read viaisEditMenuCommand/EDIT_MENU_COMMANDSinsidemenuCommandHandler(platform-scripture-editor.web-view.tsx:3555-3559), aSelectMenuItemHandler. The ADR's Decision paragraph explicitly records this as deliberate: "The Edit flyout's Undo/Redo/Cut/Copy/Paste are menu command ids handled inside the editor web view (menuCommandHandler) throughEditorRef, not PAPI commands." A repo-wide search of.context/standards/and.claude/rules/forSelectMenuItemHandler,menuCommandHandler,EDIT_MENU_COMMANDSandisEditMenuCommandfinds no other standard documenting this route, so the contradiction is not resolved elsewhere.
#2 — Done. The Menu Item Structure note in Entry-Point-Guide.md now describes the second route (a command id the web view's SelectMenuItemHandler handles itself), names EDIT_MENU_COMMANDS, and points at the ADR.
extensions/src/legacy-comment-manager/src/main.ts line 180 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#7 - low · checked and confirmed
The raise-an-open-tab-else-create-one behaviour behind the four new Simple Tools items exists as three independent copies, so a change to how a panel is raised has to be made in three files or the Comments item silently behaves differently from its neighbours.
What happens:
show-panel.util.ts:21-48extracts the shape asraiseExistingTab+showOrCreateTab, andplatform-scripture-editor/src/main.ts:1145-1156extracts the project lookup asgetProjectIdOfWebView.legacy-comment-manager/src/main.ts:180-204then re-implements both inline — the same{ existingId: '?', createNewIfNotFound: false, bringToFront: true }bag, the sameif (existingId) return existingIdshort-circuit, and a near-verbatim copy of the degrade-to-no-project-id comment. The four Tools items sit in one menu section and are meant to behave identically.Why it matters: all three functions are new in this change — none exists at the merge-base — so this is duplication introduced across three sites at once, not an inherited pattern. Nothing enforces the agreement:
shipped-simple-layout-order.test.tspins the order of the Tools section, not the behaviour behind each item, so a future change to the raise policy inshowOrCreateTabwould leave Comments out of step with no test failing.Fix: hoist the options bag in
legacy-comment-manager/src/main.tsinto a named module constant the wayextensions/src/platform-scripture/src/main.ts:390does withREUSE_EXISTING_FIND_ONLY, and point its TSDoc atshowOrCreateTabas the shape it mirrors, so the duplication is declared rather than accidental. A shared helper is the better end state but must live somewhere both extensions can import.How this was checked:
legacy-comment-manager/src/main.ts:180-204(showCommentListPanel) reimplements the exact raise-or-create shape thatshow-panel.util.ts:21-48andplatform-scripture-editor/src/main.ts:1145-1156already factor out: the identical{ existingId: '?', createNewIfNotFound: false, bringToFront: true }options bag, the identicalif (existingId) return existingIdshort-circuit, and a near-verbatim copy of the try/catch comment — comparemain.ts:191-193("degrade to 'no project id' so a new Comments tab still opens, unlabeled, rather than rejecting the whole show request") withplatform-scripture-editor/src/main.ts:1147-1149. None of the three functions exists at the merge-base —show-panel.util.tsis not there at all — so this is duplication this change introduces across three sites.shipped-simple-layout-order.test.tsonly pins the Tools section's item order, not the raise-vs-reload behaviour behind each item, so nothing would fail ifshowOrCreateTab's policy changed without a matching edit to the Comments copy.
#7 — Done. The options bag in legacy-comment-manager is now a named RAISE_EXISTING_TAB_ONLY constant whose TSDoc points at showOrCreateTab.
extensions/src/platform-scripture-editor/contributions/menus.json line 131 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#3 - medium · checked and confirmed
In Simple mode the footnotes pane never opens by itself when a chapter has notes, and there is no longer any way to turn that behaviour on.
What happens: the auto-show toggle is hidden in Simple (
menus.json:131) and the Simple View section (menus.json:253-275) carries onlychangeView,toggleFootnotesandchangeFootnotesPaneLocation, so the command has no Simple entry point. Its state is per-web-view (platform-scripture-editor.web-view.tsx:918) and the effective value isfootnotesAutoShowChoice ?? isPowerMode(:931) — off in Simple. Nothing else in the repo sendsplatformScriptureEditor.toggleFootnotesAutoShow.Why it matters: unlike the Power-only tools, this is a stateful preference whose Simple default is exactly the disabled state, so the gate does not just hide a rarely-used tool — it forecloses the only path to the enabled state. The ticket does not mention this toggle anywhere, so 1.6e's Quality-checks-only authorisation does not cover it.
Fix: add a Simple copy of this item to the
platformScriptureEditor.simpleViewLayoutgroup withhiddenInterfaceModes: ["power"]andorder: 4, mirroring the three existing View duplicates (menus.json:252-275); update the expected Simple layout inmenu-data.service-host.scripture-editor-menu.test.ts:163-170and drop the command fromPOWER_ONLY_COMMANDS(:236). Also update the Consequences prose inadr-menu-per-mode-layout-via-mode-gated-columns(Architecture-Decisions.md:2916-2917) to remove "Auto-show footnote pane" from the list of items with no Simple route, since it would no longer apply. If it is meant to stay Power-only, keep the gate and replace thePOWER_ONLY_COMMANDScomment at:236with the actual reason Simple should not have this toggle, rather than restating that it currently has no place for it.How this was checked: At the merge-base,
toggleFootnotesAutoShowhad nohiddenInterfaceModesand was visible in both modes; this change adds"hiddenInterfaceModes": ["simple"]atmenus.json:131and gives it no counterpart in Simple'ssimpleViewLayoutgroup, which holds onlychangeView/toggleFootnotes/changeFootnotesPaneLocation(menus.json:253-275). Readingplatform-scripture-editor.web-view.tsx:918-937: the per-web-view state isfootnotesAutoShowChoice(persisted viauseWebViewState, defaultundefined), and the effective value isfootnotesAutoShow = footnotesAutoShowChoice ?? isPowerMode— so in Simple it defaults to disabled, and with no menu route left to invoke the command a Simple user can never flip it totrue. This differs from the Power-only tools, which are unreachable but otherwise inert: here the gate forecloses the only path to the enabled state. Nothing in the ticket's decisions, target structure or known limitations mentions this toggle.Similar fix as #1.
Other findings in this file: #1, #8
#3 — Keeping it Power-only on purpose. Simple keeps PT9's manual footnotes pane: Show footnotes opens it and it stays open, so Simple has no automatic behavior to turn on. The POWER_ONLY_COMMANDS comment and the ADR Consequences now say that instead of "has no place for it".
extensions/src/platform-scripture-editor/contributions/menus.json line 138 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#1 - high · checked and confirmed
In Simple mode a user can no longer open the Characters, Repeated Words, Markers or Punctuation inventories, the Markers Checklist, or the Checks side panel from anywhere in the app.
What happens: six items in the editor's Tools column gain
hiddenInterfaceModes: ["simple"](menus.json:136-178), sofilterItemsForInterfaceMode(src/extension-host/services/menu-data.service-host.ts:35-40) drops them from the served Simple menu andgetMenuSectionsWithItemsthen suppresses the wholeplatformScriptureEditor.toolscolumn. At the merge-base none of the six was mode-gated. Nothing re-homes them into a Simple column, and no other menu contributes these commands.Why it matters: this is a capability loss for every Simple user, not a relocation —
changeView,toggleFootnotesandopenFindwere re-homed intosimpleView/simpleTools, these were not. The ticket's one hiding authorisation (1.6e, Quality checks) covers exactly one of the six,platformScripture.openChecksSidePanel; the four inventories and Markers Checklist appear nowhere in the ticket, its target structure, its known limitations or its Definition of Done.Fix: either (a) give each of the five unauthorised commands a Simple entry point — one item per command with
hiddenInterfaceModes: ["power"]in a Simple group (e.g. aplatformScriptureEditor.simpleInventorygroup under thesimpleToolscolumn, following the shape of the threesimpleViewLayoutduplicates already in the file) — then update the "Simple serves its shipped layout" expectation and remove those five fromPOWER_ONLY_COMMANDS(menu-data.service-host.scripture-editor-menu.test.ts:225-243), leavingopenChecksSidePanelthere since 1.6e authorises it; or (b) record an explicit product decision naming who approved dropping Simple's only route to the four inventories and Markers Checklist, replace those fivePOWER_ONLY_COMMANDScomments with that decision and owner (matching the substantive reasons already given foropenManageBooksandlegacyCommentManager.openCommentListin the same list), and update the Consequences prose inadr-menu-per-mode-layout-via-mode-gated-columnsso it records a decision rather than an unexplained side effect. ATAB_FOR_COMMANDentry does not apply either way — these commands open floating or docked views, not a third-column tab.How this was checked: At the merge-base copy of
extensions/src/platform-scripture-editor/contributions/menus.json, none of the six items (charactersInventory, repeatedWordsInventory, markersInventory, punctuationInventory, markersChecklist, openChecksSidePanel) carriedhiddenInterfaceModes, so Simple could reach them. This change adds"hiddenInterfaceModes": ["simple"]to all six atmenus.json:137-178, and no Simple column or group re-homes any of them — Simple'ssimpleTools/simpleViewcolumns hold only changeView/toggleFootnotes/changeFootnotesPaneLocation and the panel-raising items.filterItemsForInterfaceMode(menu-data.service-host.ts:35-40) drops any item whosehiddenInterfaceModesincludes the current mode. A repo-wide search for all six command ids, plus the siblingparatext-bible-internal-extensions,paratext-bible-extensionsandparatext-10-studiocheckouts, finds no other menu, contribution or code path that reaches them —platform-scripture/src/main.tsonly registers the commands. The ADR states the same outcome as a Consequence (Architecture-Decisions.md:2916-2917): "Simple now has no menu route at all to the four Inventories, Markers Checklist, Open Checks, or Auto-show footnote pane."Similar fix as #3.
Other findings in this file: #3, #8
#1 — Keeping these hidden in Simple to follow the v0 Simple design, which has no quality tools in the Project menu. I have asked UX to confirm and will revisit if they want them back. The POWER_ONLY_COMMANDS comment and the ADR Consequences now give that reason (and note UX has not confirmed yet) instead of "has no entry point".
extensions/src/platform-scripture-editor/contributions/menus.json line 212 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#8 - low · checked and confirmed
In Simple mode the Project menu shows "Ctrl+F" beside Tools ▸ Find but nothing beside Edit ▸ Undo or Redo, even though Ctrl+Z and Ctrl+Y work in that editor and the catalog documents both.
What happens:
addShortcutHints(menu-data.service-host.ts:48-56) attaches a hint only when a menu item'scommandmatches a catalog entry'scommand. Theeditor-undoandeditor-redoentries (keyboard-shortcuts.data.ts:657-676) have none, so the new Undo/Redo flyout items join to nothing whileplatformScripture.openFindtwo sections below joins to⌃F/Ctrl+F.Why it matters: every Simple user opening the new Edit flyout sees this, and the inconsistency sits inside one menu. It is a structural limit rather than an oversight:
KeyboardShortcutEntry.commandis typedCommandNames(keyboard-shortcuts.model.ts:41), and these five ids are deliberately never registered as PAPI commands, so nocommandcan be set on those entries at all. The chords themselves still work — only the hint is absent.Fix: record the limit rather than declaring commands PAPI never registers, which would make the
.d.tslie: add to the Consequences section ofadr-menu-per-mode-layout-via-mode-gated-columnsin.context/standards/Architecture-Decisions.mdthat web-view-handled menu ids cannot carry catalog hints, becauseKeyboardShortcutEntry.commandis typed to registered commands. Nokeyboard-shortcuts.data.test.tschange is owed, since nocommandis being added.Other findings in this file: #1, #3
#8 — Done. Recorded in the ADR Consequences.
extensions/src/platform-scripture-editor/src/editor-side-effects.utils.test.ts line 61 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#9 - low · checked and confirmed
This test can only fail if someone literally sets two adjacent string constants to the same value, so it tells a future reader nothing about what the Edit-flyout notice does.
What happens:
EDIT_ACTION_BLOCKED_KEY(editor-side-effects.utils.ts:68-69) andSYNC_EDIT_BLOCKED_KEY(:26-27) are two literalLocalizeKeyconstants declared about 40 lines apart. The assertion at:62compares them and nothing else — no function is called, no behaviour exercised. The behaviour that matters, which of the two messages a blocked action shows, lives inmenuCommandHandler'snotifyBlockedbranch atplatform-scripture-editor.web-view.tsx:3559-3560, which this file does not cover.Why it matters: modestly.
EDIT_ACTION_BLOCKED_KEYwas added in this change by copy-pasting the existingSYNC_EDIT_BLOCKED_KEYblock, and forgetting to change the copied suffix is exactly what this assertion catches — the file's other three tests reference the constants by name, so they would not. It is a cheap copy-paste tripwire sitting alongside three tests that do drive real behaviour.Fix: delete the test at
:61-63.notifyBlockedis a closure insidemenuCommandHandlerinplatform-scripture-editor.web-view.tsx(:3556-3560) that no test imports or can reach, so asserting which message it sends is not possible from this file without first extracting that branch into a testable function, which is out of scope here. If that extraction happens, add the message-selection test at the new site; until then keep only the three tests that already exercisenotifyEditMenuActionBlockedandnotifySyncEditBlockeddirectly (:32-59,:69-80).
#9 — Removed.
extensions/src/platform-scripture-editor/src/localized-strings.test.ts line 312 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#10 - low · checked and confirmed
Adding a dev-only sample extension to the canonical list leaves this copy stale, and the menu-label test then counts that extension's strings as shipped.
What happens: this change exports the canonical list at
locale-assets.test-helper.ts:29and the new harness imports it atmenu-data.service-host.test-utils.ts:6, but this file re-declares it by hand at:312. The comment at:308-310cites the missing@node/*alias under theextensions/tsconfig as the reason, yet a relative import bypasses that alias entirely, exactly asmenu-data.service-host.scripture-editor-menu.test.ts:8already does crossing the same boundary the other way. Nothing compares the two lists, so they drift silently andallKeys()at:330stops excluding the new dev-only extension.Why it matters: the exclusion exists so a key reachable only in noisy dev mode cannot vouch for a production string; a stale copy re-opens exactly that false negative.
Fix: import
DEV_ONLY_EXTENSION_NAMESfrom../../../../src/node/utils/locale-assets.test-helperwith an explicit relative path — the techniquemenu-data.service-host.scripture-editor-menu.test.ts:8already relies on — and delete the local copy.How this was checked: The
DEV_ONLY_EXTENSION_NAMESlist atextensions/src/platform-scripture-editor/src/localized-strings.test.ts:312is hand-copied from the canonical list exported atsrc/node/utils/locale-assets.test-helper.ts:29, and nothing keeps the two in sync — a name added to only one meansallKeys()at:330starts miscounting that extension's strings. The comment justifying the copy cites the missing@node/*path alias under theextensions/tsconfig, but a relative import bypasses that alias entirely, exactly assrc/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts:8already does crossing the same boundary the other way. Reproducing the realextensions/tsconfig.jsonandtsconfig.lint.jsonin an isolated copy and running TypeScript's own module resolver and a full program build against the mirrored layout, the relative import resolved cleanly under both configs with no import-related diagnostics. Theno-restricted-importsrule atextensions/.eslintrc.cjs:91-99only matches bare specifiers likenode/*, not relative paths, so it does not block this either.Similar fix as #26.
Other findings in this file: #11
#10 / #26 — Done. Renamed to locale-assets.test-utils.ts (all importers updated), and localized-strings.test.ts imports DEV_ONLY_EXTENSION_NAMES by relative path instead of keeping a copy.
extensions/src/platform-scripture-editor/src/localized-strings.test.ts line 349 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#11 - low · checked and confirmed
A tooltip added to a Project menu item shows up as a raw
%…%key in the UI with no test failing, despite the comment promising new menu strings are covered automatically.What happens:
menuLabelKeyscollects onlycolumns[].labelanditems[].label.tooltip?: LocalizeKeyis declared onMenuItemBase(menus.model.ts:68), shared by both item kinds, andLocalized<T>(menus.model.ts:241) resolves it exactly as it resolveslabel;tab-dropdown-menu.component.tsx:84and:95render it asTooltipContentfrom that resolved value. The editor'smenus.jsonhas zero tooltips today, so the gap is invisible; the moment one is added with an undefined key, the comment at:358-362— "a label added to the menu is covered here without anyone remembering to update this file" — is false for it.Why it matters: the whole value of a document-driven test is that it needs no maintenance; a reader who adds a tooltip will trust it and ship an untranslated string. No other test in the repo walks a menu document collecting tooltip keys.
Fix: widen
menuLabelKeysto also collectitem.tooltipand rename it tomenuLocalizeKeys, filtering to values matching/^%.*%$/soundefinedentries are dropped.How this was checked:
menuLabelKeys()(localized-strings.test.ts:349-356) collects onlycolumn.labelanditem.labelfrom the raw../contributions/menus.jsonimport; it never readsitem.tooltip.tooltip?: LocalizeKeyis declared onMenuItemBase(lib/platform-bible-utils/src/extension-contributions/menus.model.ts:68), shared by bothMenuItemContainingCommandandMenuItemContainingSubmenu, andLocalized<T>(menus.model.ts:241) replaces everyLocalizeKey-typed field — includingtooltip— with a resolved string, exactly likelabel.tab-dropdown-menu.component.tsx:84and:95render{item.tooltip && <TooltipContent>{item.tooltip}</TooltipContent>}from that same localized value, so a tooltip goes through the identical%key%mechanism as a label. No other test in the repo walks a menu document collecting tooltip keys;src/shared/utils/menu-document-combiner.localization.test.tsonly asserts the resolution pipeline runs. Everymenus.jsonrepo-wide contains zero"tooltip"occurrences, and the suite passes 289/289 — nothing here would fail if a tooltip key were added undefined.Other findings in this file: #10
#11 — Done. menuLocalizeKeys also collects item tooltips.
extensions/src/platform-scripture-editor/src/main.ts line 1145 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#4 - medium · checked and confirmed
A newly opened Bible texts or Commentaries tab silently loses its project label with no test failing.
What happens:
getProjectIdOfWebView(main.ts:1145) is the only thing that gives a newly created Bible texts / Commentaries tab itsprojectId, via the callbacks atmain.ts:1446and:1470. No test imports thismain.ts, so mutating line 1146 toreturn undefined;unconditionally leaves the whole suite green. The identical helper inlegacy-comment-manager/src/main.ts:189-202got three tests (main.test.ts:127,:141,:155) covering resolve, throw-and-degrade and the warn; the editor's copy got none.Why it matters: a tab opened for the wrong project, or none, is what the user sees the first time they use two of the five Tools items.
Fix: add an
activate()-driven test besideshow-panel.util.test.tswith a mocked@papi/backendcoveringgetProjectIdOfWebView's three paths —webViewIdabsent,getOpenWebViewDefinitionresolving aprojectId, and it rejecting (assertinglogger.warnand a{ projectId: undefined }create) — mirroringlegacy-comment-manager/src/main.test.ts:141-170. Budget for a larger mock surface than that reference:activate()here needscommands,webViewProviders,webViews,network,dataProviders,projectDataProviders,settings,dialogs,localizationandprojectLookupstubbed, plus the extension's eight?inlineasset imports and a no-opWebViewFactorybase class.How this was checked:
getProjectIdOfWebView(extensions/src/platform-scripture-editor/src/main.ts:1145-1156, new in this change) is reachable only through the two panel commands registered atmain.ts:1446and:1470. A repo-wide search for any import of thismain.tsfound onlyextension-host-import-boundary.test.ts, which reads the file's text withreadFileSyncand regex-parses its import statements — it never imports or executes the module.show-panel.util.test.tscallsshowOrCreateTabdirectly with literal web-view-type strings (:32,:46), so it never touches the panel commands or this helper. Confirmed directly by copyingmain.tsand its local dependency graph into an isolated scratch directory, building anactivate()-driven harness with a mocked@papi/backend, and mutatinggetProjectIdOfWebViewtoreturn undefined;unconditionally:show-panel.util.test.ts's 6 tests stayed fully green under that mutation, while a new test exercising the three code paths went red.Similar fix as #6.
Other findings in this file: #12
#4 / #6 — Done, without an activate() harness. The Bible texts/Commentaries handlers and getProjectIdOfWebView moved into show-panel.util.ts (showBibleTextsTab / showCommentariesTab bind their own web view type), and main.ts registers those functions directly. show-panel.util.test.ts now covers each function raising and creating its own panel type (swapping the constants fails), plus getProjectIdOfWebView's three paths.
extensions/src/platform-scripture-editor/src/main.ts line 1443 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#12 - low · checked and confirmed
If opening the Bible texts or Commentaries tab fails, the menu item does nothing at all and the user is given no indication anything went wrong.
What happens:
showOrCreateTab(show-panel.util.ts:39-48) has no error handling of its own, so a rejection from eitheropenWebViewcall propagates out of the command. The only handler is the blanket.catchat the call site (platform-scripture-editor.web-view.tsx:3606-3614), which writes alogger.warnand returns. The sibling Text collection command deliberately does the opposite and notifies the user (show-panel.util.ts:71-82), so the three Tools items behave inconsistently on failure.Why it matters: rare in practice, because in Simple mode both panels are pinned non-closable tabs that
raiseExistingTabwill always find; it bites on a closing-window race or after a layout restore that lost the tab. The cost is a menu item that silently does nothing, which reads as a broken build.Fix: give
showOrCreateTab's twomain.tscallers the same treatment asshowTextCollectionTab— wrap the create and send a generic "couldn't open the tab" warning notification on failure. That needs a new localized key in bothenandesinextensions/src/platform-scripture-editor/contributions/localizedStrings.json, plus a case inshow-panel.util.test.ts.How this was checked: The two new commands (
showBibleTextsPanel,showCommentariesPanel,main.ts:1443-1471) hand their promise straight toshowOrCreateTab(show-panel.util.ts:39-48), which has no error handling of its own. A rejection from eitherpapi.webViews.openWebViewcall inside it propagates to the only catch in the chain, the generic.catchatplatform-scripture-editor.web-view.tsx:3606-3614, which doeslogger.warnand nothing else. Nothing else in the stack surfaces this to the user: there is no global command-error toast, and because the rejection is already caught here it never reaches the renderer'sunhandledrejectionlistener (src/renderer/index.tsx:67) either. The siblingshowTextCollectionTabin the same file (show-panel.util.ts:61-83) already warns the user on failure, so this is a checked inconsistency rather than a universal silent-failure pattern for the module.Similar fix as #14.
Other findings in this file: #4
#12 / #14 — Done. Bible texts and Commentaries warn with a new generic "That tab couldn't be opened" notice (en + es) instead of failing silently. Text collection now shows "isn't available" only when the error contains "Cannot find Web View Provider", and otherwise warns in the log and shows the generic notice. Tests cover both paths, using the wrapped JSON-RPC message.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 3560 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#13 - low · checked and confirmed
A user with no Scripture-edit permission who picks Edit ▸ Paste while a scheduled Send/Receive is running is told editing is paused for the Send/Receive, which implies it will work once the sync finishes — it will not.
What happens:
isReadOnlyEffective(:1040-1058) ORs the durable reasons — no permission, project not editable, markers view — with the transientisSyncBlocked, sorunEditMenuActionreturnsfalsefor either.notifyBlocked(:3560-3563) then branches onisSyncBlockedalone, so whenever a sync is in flight the sync message wins over a durable block.isSyncBlockedis driven byauto-sync-edit-block-driver.ts, whoseisWebViewBlockedkeys only onprojectId(:76-87), independent of the viewing user's own permissions.Why it matters: narrow — it needs a permission-less or non-editable project and an automatic Send/Receive in flight — but auto-sync runs on a schedule, so a reviewer-role user hits it by chance and is told to wait for something that will not help.
Fix: in
menuCommandHandler, makenotifyBlockedprefer the durable message —isDurablyReadOnly ? notifyEditMenuActionBlocked(localizedStrings) : isSyncBlocked ? notifySyncEditBlocked() : notifyEditMenuActionBlocked(localizedStrings)— using the existingisDurablyReadOnlyat:1162, add it to theuseCallbackdep array at:3616-3623, and update the comment at:3560-3561to state the durable-over-transient priority. Cover the durable-block-during-a-sync case with a test.How this was checked:
isReadOnlyEffective(platform-scripture-editor.web-view.tsx:1040-1058) ORs the durable reasons (no permission, project not editable, markers view) together with the transientisSyncBlocked, butnotifyBlocked(:3560-3563) branches onisSyncBlockedalone.isSyncBlockedis driven bysrc/renderer/services/auto-sync-edit-block-driver.ts, which flags every open edit-blockable web view of a project under an automatic Send/Receive independent of the viewing user's own permissions (isWebViewBlockedkeys only onprojectId,:76-87). So a permission-less or non-editable-project viewer who tries an Edit action while that project's automatic sync happens to be running is told editing is paused for the sync — which will not help once the sync finishes, since the real block is durable.isDurablyReadOnly(:1162-1169) is exactly the non-transient set the branch needs and is already in scope but unused there. The adjacent comment at:3560-3561explains why sync alone gets a named message but says nothing about which message should win when a durable reason and a sync both hold.Similar fix as #5.
Other findings in this file: #5
#5 / #13 — Done. The branch moved to handleEditMenuCommand in edit-menu-actions.util.ts, with notices, selection restore, error reporting and refocus passed in as callbacks; the web view still calls it synchronously. A durable block now takes priority over the sync message. Tests cover ran, sync-only block, durable block during a sync, no editor, and a throwing action.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 3564 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#5 - medium · checked and confirmed
Choosing Edit ▸ Paste while the project is read-only shows no notification at all, and no test fails.
What happens:
runEditMenuActionandnotifyEditMenuActionBlockedeach have their own unit test, but nothing tests the code that joins them, and no test importsplatform-scripture-editor.web-view.tsx. Deletingif (!ran) notifyBlocked();at:3580, inverting theisSyncBlockedternary at:3562-3563, or droppingrequestAnimationFrame(() => editorRef.current?.focus())at:3585all leave the suite green. TheeditorRef-absent branch at:3564-3571and therestoreSelectionIfLostcall at:3572are likewise unexercised.Why it matters: the silent no-op is the whole point of the read-only gate —
runEditMenuActionreturningfalseis invisible to the user unless this branch fires.Fix: extract the branch into
edit-menu-actions.util.tsashandleEditMenuCommand(command, editor: EditMenuTarget, state: { isReadOnlyEffective, isDurablyReadOnly, isSyncBlocked }, effects: { notifyEditMenuActionBlocked, notifySyncEditBlocked, restoreSelectionIfLost, focusEditor }), passingrestoreSelectionIfLostandfocusEditoras callbacks — matching the pattern already used at:2015-2017— rather than wideningeditor's type, sinceEditMenuTargetisPick<EditorRef, 'undo'|'redo'|'cut'|'copy'|'paste'>and has neither.focus()norgetSelection/setSelection. Inside the helpernotifyBlockedmust preferisDurablyReadOnlyoverisSyncBlocked, not proxy today's check. KeeprunEditMenuActionand therequestAnimationFramerefocus synchronous and un-awaited exactly as today so the clipboard call stays inside the click's user-activation window. Test all four outcomes — ran, blocked-by-sync, blocked-generic, no-editor — inedit-menu-actions.util.test.ts, and keepweb-view.tsxcalling the helper so the extraction is what ships.How this was checked: A search for the bare basename
platform-scripture-editor.web-viewacross every test file in the extension found zero test files importing it; runningedit-menu-actions.util.test.tsandeditor-side-effects.utils.test.ts(16 tests, all pass) confirms those two files exerciserunEditMenuActionand the notify helpers in isolation, never the branch inmenuCommandHandlerthat joins them (platform-scripture-editor.web-view.tsx:3556-3587). Because nothing imports this file, any single-line change inside the branch — deletingif (!ran) notifyBlocked();at:3580, inverting the ternary at:3562-3563, or droppingrequestAnimationFrame(() => editorRef.current?.focus());at:3585— leaves the full suite green. TheeditorRef-absent branch (:3564-3571) and therestoreSelectionIfLostcall (:3572) are likewise unexercised.Similar fix as #13.
Other findings in this file: #13
#5 / #13 — Done. The branch moved to handleEditMenuCommand in edit-menu-actions.util.ts, with notices, selection restore, error reporting and refocus passed in as callbacks; the web view still calls it synchronously. A durable block now takes priority over the sync message. Tests cover ran, sync-only block, durable block during a sync, no editor, and a throwing action.
extensions/src/platform-scripture-editor/src/show-panel.util.ts line 62 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#14 - low · checked and confirmed
When the Text collection tab fails to open for a passing reason, the user is told the feature isn't available at all, and the real error is only visible at debug log level.
What happens: the
tryat:63spans both the raise and the create insideshowOrCreateTab, so it catches every rejectionopenWebViewcan produce — not only the missing-provider case it names.throwIfWindowIsClosing(web-view.service-shard.ts:3339), the router's window-routing errors (web-view.service-router.ts:690,726) and aLogErrorfrom a lost reuse race all land here, fall through:71-82, and raise "Text collection isn't available, so there's no tab to show." while the actual cause is written atlogger.debug(:69), which a default log level does not record.Why it matters:
enableScriptureTextGriddefaults totrue(settings.json:6), so the expected reason for this catch is the uncommon one; every other reason arrives mislabelled as a permanently unavailable feature and leaves no trace in the log.Fix: narrow the catch with a substring test, not equality —
getErrorMessage(e).includes('getWebView: Cannot find Web View Provider')— because the message that reaches this catch is wrapped asJSON-RPC Request error (<code>): getWebView: Cannot find Web View Provider for webview type <type>, not the bare string. Keeplogger.debugplus the "isn't available" notification only when that substring is present; otherwiselogger.warnplus a new generic "couldn't open the Text collection tab" notification, whose key must be added to bothenandesinlocalizedStrings.json. Add a case inshow-panel.util.test.tsthat rejects with a non-provider error and asserts the generic-warning path, and update the existing provider-missing test at:79-89to reject with the fully wrapped message so it exercises the substring match realistically rather than an exact one that cannot occur in production.How this was checked: The
tryatshow-panel.util.ts:63wrapsshowOrCreateTab, which itself makes twopapi.webViews.openWebViewcalls (the raise at:22-27, the create at:47) — a rejection from either lands in the samecatchat:65, not only the provider-missing case its comment names. The provider-missing throw (src/renderer/services/web-view.service-shard.ts:2769), its window-closing guard (:3339), and the router's window-routing throws (src/main/services/web-view.service-router.ts:690,726) are all real, distinct, reachable throw sites, and every one would be swallowed into the same "Text collection isn't available" message while the real cause is written only atlogger.debug, which a default log level drops.enableScriptureTextGriddefaults totrue(extensions/src/platform-scripture-editor/contributions/settings.json:6), so the provider-missing case this catch is written for is the uncommon one in practice.Similar fix as #12.
#12 / #14 — Done. Bible texts and Commentaries warn with a new generic "That tab couldn't be opened" notice (en + es) instead of failing silently. Text collection now shows "isn't available" only when the error contains "Cannot find Web View Provider", and otherwise warns in the log and shows the generic notice. Tests cover both paths, using the wrapped JSON-RPC message.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 89 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#18 - low · checked and confirmed
The tooltip wiring for submenu triggers is rewritten for every consumer of
TabDropdownMenu, although no menu item this change ships carries a tooltip.What happens: the command branch and the submenu branch of
getGroupContentare split (tab-dropdown-menu.component.tsx:56-113) so the tooltip attaches toDropdownMenuSubTriggerrather than toDropdownMenuSub. The Edit flyout item (menus.json:203-209) declares notooltip, so the shipped menu renders identically either way; the behaviour is observable only through the new fixture-built test. The sibling RTL-chevron change indropdown-menu.tsx:353-378is not in this class — the flyout opens leftward in RTL, so the old hardcoded right chevron pointed away from it.Why it matters: it widens the blast radius of a menu-content change into a shared component used by every tab menu, and a reviewer of the Simple-menu change has no way to tell the component change is not load-bearing for it.
Fix: keep the fix, and call it out in the PR description as a separate fix to the shared menu component with its own test, distinct from the RTL chevron change which this feature does depend on.
How this was checked: The Edit-flyout submenu item at
extensions/src/platform-scripture-editor/contributions/menus.json:203-209(platformScriptureEditor.editSubmenu) declares notooltipfield. A repo-wide sweep of everymenus.jsonandsrc/extension-host/data/menu.data.jsonfor a"tooltip"entry on any item returns zero hits anywhere in the codebase, so no menu item shipped by this change or any other extension currently uses the field. The two other real consumers ofTabDropdownMenu(tab-toolbar.component.tsx,tab-floating-menu.component.tsx) do not referencetooltipeither, so the change is not load-bearing for them today. The diff attab-dropdown-menu.component.tsx:56-113confirms the rewrite touches both the command branch and the submenu branch of a shared component used by every tab menu in the app, for a behaviour no shipped item currently exercises.Similar fix as #19.
#18 — Called out in the PR description.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 91 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#19 - low · checked and confirmed
With the Edit flyout open, its trigger row reports
data-state="closed"anddata-slot="tooltip-trigger"instead of the submenu's own open state and slot.What happens:
TooltipTrigger asChild(tooltip.tsx:47-58) clonesdata-slot="tooltip-trigger"and its owndata-stateontoDropdownMenuSubTrigger, which writesdata-slot/data-insetbefore{...props}(dropdown-menu.tsx:360-378), so the clone wins. Opening the flyout with ArrowRight givesdata-state="closed",data-slot="tooltip-trigger",aria-expanded="true". The wrapper is applied unconditionally, so an item with notooltipis affected too.Why it matters:
DropdownMenuSubTrigger's owntw:data-open:bg-accent(dropdown-menu.tsx:371) resolves to[data-state="open"], which can never match whiledata-stateis pinned to"closed", so those classes are dead and any styling or test written against[data-state=open]or[data-slot=dropdown-menu-sub-trigger]silently misses. The visible open-highlight still works, becauseDropdownMenuContent's selector keys on[data-slot$="-trigger"]andaria-expanded, and"tooltip-trigger"also ends in-trigger.Fix: in
tab-dropdown-menu.component.tsx, render theTooltip/TooltipTriggerwrapper aroundDropdownMenuSubTriggeronly whenitem.tooltipis set, mirroring the{item.tooltip && <TooltipContent>…}guard already used for the content — this alone fixes every case that exists in the repo today. If a future submenu item is expected to carry both a submenu and a tooltip, additionally hardenDropdownMenuSubTriggerindropdown-menu.tsxwith a// CUSTOM:comment explaining that anasChildTooltipTriggerclones its owndata-slot/data-stateonto this element, so (a)data-slot="dropdown-menu-sub-trigger"must be re-asserted after{...props}, and (b) any inbounddata-statemust be explicitly deleted frompropsbefore spreading — not merely reordered, since this component never writes an explicitdata-stateof its own — so Radix's real open/closed value survives; add a test rendering a submenu item withtooltipset, opening its flyout, and assertingdata-state="open"anddata-slot="dropdown-menu-sub-trigger"on the trigger.How this was checked: Verified by rendering. A probe using this change's own fixture shape (a submenu item with no
tooltip, matching the shipped Edit item) throughTabDropdownMenu, opened with ArrowRight, read the live sub-trigger element:data-state="closed",data-slot="tooltip-trigger",aria-expanded="true". The mechanics:TooltipTrigger asChild(tooltip.tsx:47-58) clones its owndata-slot/data-statevia Radix's Slot merge, andDropdownMenuSubTrigger(dropdown-menu.tsx:360-378) writesdata-slot/data-insetbefore{...props}, so the clone wins; the wrapper (tab-dropdown-menu.component.tsx:91-96) is applied regardless ofitem.tooltip.tw:data-open:bg-accent(dropdown-menu.tsx:371) traces to shadcn's@custom-variant data-open, which matches[data-state="open"]— withdata-statepinned to"closed"that class genuinely can never apply. The consequence is narrower than a first read suggests:DropdownMenuContent's highlight selector (dropdown-menu.tsx:137) still matches because"tooltip-trigger"also ends in-triggerandaria-expandedwas not clobbered, so the visible open highlight still renders; only the sub-trigger's owndata-open:*classes are dead.Similar fix as #18.
#19 — Done. The Tooltip wrapper is only rendered when the item has a tooltip. Added a test asserting data-state="open" and data-slot="dropdown-menu-sub-trigger" on an open flyout's trigger. I didn't harden DropdownMenuSubTrigger for the tooltip-plus-submenu case, since nothing uses it yet.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 98 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#20 - low · checked and confirmed
A flyout row whose every child is hidden in the current interface mode still opens, showing an empty box the user can arrow into and get nothing from.
What happens:
getMenuSectionsWithItemsdrops a column with no items (menu.util.ts:101-113) but counts a submenu parent regardless of its contents (menu.util.ts:96), sogetGroupContentrenders theDropdownMenuSubattab-dropdown-menu.component.tsx:89and the recursive call at line 100 returns an empty array intoDropdownMenuSubContent. A probe with a menu holding only a submenu parent and no items in its group opened the trigger with ArrowRight and found tworole="menu"elements — the second an emptydata-slot="dropdown-menu-sub-content"panel.Why it matters: the suppression rule that keeps a column from heading nothing does not cover flyouts. It cannot occur today:
menu.util.tsis byte-identical at the merge-base, and this change's own submenu is symmetric —editSubmenuand all five Edit children carry the samehiddenInterfaceModes: ["power"], so they show and hide together. It becomes reachable the moment any futurehiddenInterfaceModesor permission gating is applied to submenu children independently of their parent.Fix: in
getGroupContent(tab-dropdown-menu.component.tsx:89) compute the submenu's children before rendering and returnnullfor that map entry when the list is empty — the recursive call already returns[]rather thanundefined, andnullentries render as nothing through the surroundingflatMapwithout needing a key. Add the emptiness check tomenu.util.tsas a helper the waygetMenuSectionsWithItemscovers columns, and wire it intoplatform-menubar.component.tsx:90-100as well — that renderer has the identical submenu branch, so adding the helper alone does not make "both renderers share one rule" true. Cover it with a test asserting nomenuitemnamed for the parent when its group has no items.
#20 — Latent (menu.util.ts is unchanged and every shipped flyout's children share their parent's gating). I'd rather handle it with both renderers in a follow-up than half-fix it here.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.test.tsx line 219 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#16 - low · checked and confirmed
The same submenu fixture is written out three times in one test file — once as the named
SUBMENU_MENUconstant and twice more inline — so adding a field the submenu path needs means editing three literals.What happens:
SUBMENU_MENUis defined at:40-62with thetest.editSubmenu/test.editActionsshape. The keyboard test at:224-253and the tooltip-hover test at:276-299each re-inline that same columns/groups/items structure, differing only by an extraRedoitem and atooltipfield respectively. The two chevron tests at:315and:328referenceSUBMENU_MENUby variable, so they are reuse rather than copies.Why it matters: confined to one test file and the tests pass today, so the cost is maintenance rather than correctness — but the file already demonstrates the fix by extracting
SUBMENU_MENUfor the chevron tests.Fix: replace the inline literals at
:225and:277with spreads overSUBMENU_MENU: for the keyboard test,{ ...SUBMENU_MENU, items: [...SUBMENU_MENU.items, redoItem] }; for the tooltip test, rebuild the items array with the first item overridden —{ ...SUBMENU_MENU, items: [{ ...SUBMENU_MENU.items[0], tooltip: 'Edit actions' }, SUBMENU_MENU.items[1]] }— since a shallow spread overSUBMENU_MENUcannot add a field onto one of its nesteditemsentries. KeepSUBMENU_MENUas the one place the submenu group wiring is spelled out.Similar fix as #17.
Other findings in this file: #17
#16 — Done. Both tests build on SUBMENU_MENU.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.test.tsx line 325 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#17 - low · checked and confirmed
Nothing fails if the RTL flyout stops opening on ArrowLeft, because the only RTL test checks which chevron icon is rendered.
What happens: the keyboard test at
:219-245genuinely exercises the keyboard path — focus the trigger,{ArrowRight}, atoHaveFocusassertion on the first flyout item, then{ArrowDown}{Enter}— but runs under thebeforeEachLTR default (:36). The two RTL-aware tests at:312and:325assert onlyquerySelector('.tabler-icon-chevron-left').Why it matters: the chevron direction and the key that opens the flyout are two independent consequences of
dir; the icon assertion passes even if thedirprop stops reachingDropdownMenuPrimitive.Root(dropdown-menu.tsx:80), which is what actually maps the arrow keys. Nothing in the shipped app currently sets thelayoutDirectionkey thatreadDirection()reads, so no user session runs in RTL today and this gap cannot yet cause a defect anyone hits — it leaves the shared menu's RTL keyboard contract unverified for whenever direction plumbing reaches production.Fix: add a case to
tab-dropdown-menu.component.test.tsxthat callspersistDirection('rtl'), focuses the sub-trigger, presses{ArrowLeft}and asserts the first flyout item has focus — the RTL twin of the test at:219.Similar fix as #16.
Other findings in this file: #16
#17 — Done. Added the RTL ArrowLeft twin.
lib/platform-bible-react/src/components/shadcn-ui/dropdown-menu.tsx line 356 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#21 - low · checked and confirmed
The new left-pointing submenu chevron never appears in the running application — only in Storybook and tests.
What happens:
readDirection()returns'ltr'unlesslocalStorage['layoutDirection']holds'rtl'(dir-helper.util.ts:7-13). A sweep across this repo and six sibling repos forpersistDirectionand the literallayoutDirectionfinds writers only in.storybook/preview.ts:77, two test files and two Storybook stories — no shipped renderer, extension or service ever writes the key. The read is also non-reactive: a plain call in the render body, the same shape every other consumer uses, so even if something wrote it the chevron would not flip until the trigger re-renders.Why it matters: the RTL half of this change is verified only by the Storybook and test path; it does not change what an RTL user sees today, which is worth stating so it is not read as shipping RTL support.
Fix: no change required at this site. Any future fix must make
readDirection()'s value flow from whatever sets the interface language and re-render its consumers — a shared hook or context consumed bydropdown-menu.tsx,menubar.tsxandselect.tsxalike — rather than each component callingreadDirection()at render.How this was checked:
readDirection()(lib/platform-bible-react/src/utils/dir-helper.util.ts:7-13) returns'rtl'only whenlocalStorage.getItem('layoutDirection') === 'rtl', otherwise'ltr'— a plain synchronous read in the render body, not a subscription. A repo-wide search for the bare identifierpersistDirectionand the literallayoutDirectionacross paranext-core plus the sibling paratext-10-studio, paratext-bible-extensions, paratext-bible-internal-extensions, scripture-editors, platform-bible-sample-extensions, paranext-extension-template and paranext-multi-extension-template repos turns up writers only in.storybook/preview.ts:77,tab-dropdown-menu.component.test.tsx,navigation-history-buttons.component.test.tsx, and two Storybook stories — every other hit is a mirrored copy of the same repo file, not an independent writer. There is no settings UI, command or other code path that writes this key..context/standards/Localization-Guide.md:363-365independently documentsreadDirection()as reading "the user's global UI direction preference", separate from per-project content direction, confirming this is a real named concept with no way to set it today.
#21 — Agreed, no change. RTL isn't reachable in the shipped app yet.
src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts line 52 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#24 - low · checked and confirmed
Changing a menu item's label in Simple's copy and not Power's leaves the two modes showing different wording for the same command with every test still green.
What happens: four commands now exist as two items each — for example
platformScriptureEditor.changeViewatmenus.json:109(hidden in Simple) and:253(hidden in Power).describeItem(:53) reduces every served item to its command id, so both mode snapshots compare command ids and order only; a search forlabelacross this test file returns zero hits, so no assertion anywhere reads one.Why it matters: the duplication is the price of this design, so label drift between modes is the failure it invites, and it is invisible to review once the diff is large. The ADR names the hole itself in its Consequences: "it pins commands, not labels, so a label edited in one copy and not the other passes silently."
Fix: add a test to
menu-data.service-host.scripture-editor-menu.test.tsthat walks both modes' served items, builds command→label maps, and asserts any command present in both is served with the same label — it needs no new fixture, both menus are already built there.How this was checked:
describeItematmenu-data.service-host.scripture-editor-menu.test.ts:52-60returns onlyitem.command(or anid ▸ childrenstring built from otherdescribeItemcalls) and never touchesitem.label; a search forlabelinside this test file returns zero hits, so no assertion anywhere in it reads a label. Exactly four commands are duplicated between the Simple and Power copies inextensions/src/platform-scripture-editor/contributions/menus.json—platformScriptureEditor.changeView(109/253),.toggleFootnotes(116/261),.changeFootnotesPaneLocation(123/269) andplatformScripture.openFind— and today all four pairs share the identical literal label key, so nothing is drifted yet, but the test'stoEqualarrays are built entirely from command ids and would not change if one copy's label key were edited. The file passes 5/5 today. The ADR documents this same hole in its Consequences section: "it pins commands, not labels, so a label edited in one copy and not the other passes silently."Similar fix as #25.
Other findings in this file: #25
#24 — Done. Added a test that every command served in both modes has the same label in each. The ADR Consequences are updated to match.
src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts line 67 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#25 - low · checked and confirmed
The pinned "Power serves its established layout" snapshot omits any menu group that the real dropdown does render, whenever that group is keyed the same as its column.
What happens: the docstring at
:63says this mirrorsgetMenuSectionsWithItems, but the filter at:74only acceptsgroup.column === columnKey. Production'sisGroupUnderColumnOrSubMenu(menu.util.ts:76-84) accepts a second case —groupKey === columnOrSubMenuKey— whichTabDropdownMenuuses attab-dropdown-menu.component.tsx:48-49to pick a column's groups. A group keyed identically to its column therefore renders in the app and is invisible to both snapshots.Why it matters: the file's stated job is to pin the served menu exactly, and a re-implementation that has drifted from the renderer cannot do that. No shipped menu relies on the second branch today — across all 10 shipped
menus.jsonplusmenu.data.json, 14 columns and 27 groups, no group's own key collides with any column key — so this is a latent divergence rather than an active blind spot, but nothing prevents the next contributed menu from tripping it.Fix: in
describeSectionsanditemsInGroup, replace the inlinegroup.column === columnKeyfilter with a local helper restating both branches ofisGroupUnderColumnOrSubMenu—('column' in group && group.column === key) || groupKey === key— with a comment citinglib/platform-bible-react/src/components/advanced/menus/menu.util.tsand itsisGroupUnderColumnOrSubMenuas the rule it must be kept in sync with. That symbol is not exported fromplatform-bible-react's public entry point or itsexportsmap, so it cannot be imported here.Similar fix as #24.
Other findings in this file: #24
#25 — Done. describeSections uses a local isGroupUnderColumn that restates both branches, with a keep-in-sync note.
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 102 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#6 - medium · checked and confirmed
Every test stays green when the Simple Tools menu's "Bible texts" item is wired to raise the Commentaries tab instead.
What happens:
main.ts:1446pairsplatformScriptureEditor.showBibleTextsPanelwithBIBLE_TEXTS_PANEL_WEBVIEW_TYPE, and:1470pairsshowCommentariesPanelwithCOMMENTARIES_PANEL_WEBVIEW_TYPE. No test imports thatmain.ts—show-panel.util.test.ts:32and:48pass the web view type in themselves as an argument, so they pin nothing about the wiring.TAB_FOR_COMMANDrestates the pairing by hand and is compared only againstmenus.jsonand the static layout data, never against what the registered handlers actually open, so swapping the two constants leaves the order test green.Why it matters: the two Bible-resource panels are adjacent Tools items; raising the wrong one is the failure a user hits on every click.
Fix: add an
activate()-driven test inextensions/src/platform-scripture-editor/src(besideshow-panel.util.test.ts) with a mocked@papi/backend— the shapelegacy-comment-manager/src/main.test.ts:127-135uses — that invokes the registeredshowBibleTextsPanelandshowCommentariesPanelhandlers and asserts each one'sopenWebViewcall names its own web view type (platformScriptureEditor.bibleTexts/platformScriptureEditor.commentaries), so swapping the two constants atmain.ts:1446/:1470fails that test directly. LeaveTAB_FOR_COMMANDas the hand-maintained mirror it already is: core cannot import extension source, as that file notes at:208, so its accuracy stays a by-eye review concern rather than something derivable from or assertable against the registrations.How this was checked: The
showBibleTextsPanel/showCommentariesPanelregistrations (main.ts:1443-1472, new in this change) pair each command with its web-view-type constant only insidemain.ts, which no test imports.TAB_FOR_COMMAND(shipped-simple-layout-order.test.ts:102-108, also new) is a hand-typed dictionary compared only against the menu document and the static layout data, never against what the registered handlers actually open; that same file notes at:208that "core cannot import extension source", which is why the gap exists structurally rather than by omission. In an isolated copy, swappingBIBLE_TEXTS_PANEL_WEBVIEW_TYPEandCOMMENTARIES_PANEL_WEBVIEW_TYPEatmain.ts:1446/:1470leftshow-panel.util.test.tsfully green and leftTAB_FOR_COMMAND's comparison unaffected by construction, while a new test asserting each handler's actualopenWebViewcall caught the swap.Similar fix as #4.
#4 / #6 — Done, without an activate() harness. The Bible texts/Commentaries handlers and getProjectIdOfWebView moved into show-panel.util.ts (showBibleTextsTab / showCommentariesTab bind their own web view type), and main.ts registers those functions directly. show-panel.util.test.ts now covers each function raising and creating its own panel type (swapping the constants fails), plus getProjectIdOfWebView's three paths.
src/node/utils/locale-assets.test-helper.ts line 29 at r1 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#26 - low · checked and confirmed
A test-support file that the third-party-notices guard does not recognise as test-only is now an exported dependency of the file that was just renamed precisely to be recognised.
What happens:
shipping-set.tsdrops test support by two patterns —TEST_SUPPORT_FILE = /\.(?:test|spec|stories|test-harness)\./at:1532and!/\.test-(?:utils?|harness)\./at:453. Neither matches.test-helper.: the first needs a literal.test., which the-helpersuffix breaks, and the second recognises only-utils/-util/-harness. Solocale-assets.test-helper.tsis scanned as shipping source. Today it imports onlynode:fs,node:pathand a type-onlyplatform-bible-utils, so nothing is reported. The siblingmenu-data.service-host.test-helper.tswas renamed to.test-utils.tsfor exactly this reason, and that renamed file now importsDEV_ONLY_EXTENSION_NAMESfrom this one.Why it matters: no CI failure today — the trap fires when someone adds
vitestor another devDependency import to this file, at which point the notices guard reports a shipping row that is not real.Fix: rename
src/node/utils/locale-assets.test-helper.tstolocale-assets.test-utils.tsand update all three importers —src/node/data/shipped-locale-assets.test.ts,src/extension-host/services/menu-data.service-host.test-utils.ts, andsrc/extension-host/data/language-details.data.test.ts— plus the file's own header comment and the mirroring note inextensions/src/platform-scripture-editor/src/localized-strings.test.ts. Widening the guard's regex to\.test-(?:utils?|harness|helper)\.is the smaller diff but leaves two spellings live.How this was checked:
.erb/scripts/third-party-notices/shipping-set.tsfilters test-support files with two patterns:TEST_SUPPORT_FILE = /\.(?:test|spec|stories|test-harness)\./at line 1532, and a second filter at line 453 (!/\.test-(?:utils?|harness)\./) insideimportedPackages. Neither matches.test-helper.— the first needs a literal.test., which the-helpersuffix breaks; the second recognises only-utils/-util/-harness. Sosrc/node/utils/locale-assets.test-helper.tsis scanned as shipping source today, the same gap this change's own second commit fixed by renaming the siblingmenu-data.service-host.test-helper.tsto.test-utils.ts. The file currently imports onlynode:fs,node:pathand a type-onlyplatform-bible-utils, so nothing trips the guard yet — a convention gap rather than a live CI failure.Similar fix as #10.
#10 / #26 — Done. Renamed to locale-assets.test-utils.ts (all importers updated), and localized-strings.test.ts imports DEV_ONLY_EXTENSION_NAMES by relative path instead of keeping a copy.
81b36d2 to
74d74b6
Compare
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 12 comments.
Reviewable status: 0 of 54 files reviewed, all discussions resolved.
a discussion (no related file):
Previously, katherinejensen00 wrote…
#15 — Pre-existing and not reachable today (no menu uses tooltip), so I'd rather not widen this PR into the menubar. Happy to file a follow-up ticket.
#22 — Pre-existing and unreachable until something sets layoutDirection; same follow-up as #15.
#23 — Done. The openSettings localizeNotes now records why Simple can't relabel it.
#27 — Done. Added TODO(PT-4735) above the insert-footnote entry, naming the chords still missing.
Checked all four against 74d74b6.
#23 — the localizeNotes note is there and reads correctly.
#27 — TODO(PT-4735) is in place above the insert-footnote entry, naming the chords.
#15 / #22 — agreed, and I don't think either belongs in this PR. Two notes for whenever the follow-up gets written: they're one fix shape (attach the tooltip to MenubarSubTrigger rather than the non-rendering MenubarSub, and pass dir to the Radix root so the chevron and the arrow-key mapping agree), and context-menu.tsx has the same shape and does ship — ContextMenuSub backs the tab right-click "Move to window" submenu (platform-tab-title.component.tsx:174) and two platform-enhanced-resources dictionary-tab components. So the follow-up is three files, not two.
Branch state — the PR now conflicts with main. You rebased onto 6071f98, which is #2837's merge commit, so that part is fine; but main has moved seven commits since and GitHub reports the branch as CONFLICTING.
Eight conflicting paths against main at 00bf170, and they split cleanly:
- Six are the committed
platform-bible-reactbundle —dist/experimental.cjs,dist/experimental.js,dist/index.cjs,dist/index.cjs.map,dist/index.js, plus a rename/rename on the hash-named chunk (yourresizable-CKuMD-fd.jsagainst main'sresizable-39QAMVZg.js). Mechanical — take main's side and rebuild. - Two are real source conflicts, both from PT-4557 (#2835), which also edited
extensions/src/platform-scripture-editor/contributions/localizedStrings.jsonandsrc/localized-strings.test.ts. Those are the same two files this PR touches for the new menu labels and the tooltip-key widening inmenuLocalizeKeys, so they want reading rather than taking either side wholesale.
Nothing needed from me on this — just flagging it so the rebase isn't a surprise.
extensions/src/platform-scripture-editor/contributions/menus.json line 131 at r1 (raw file):
Previously, katherinejensen00 wrote…
#3 — Keeping it Power-only on purpose. Simple keeps PT9's manual footnotes pane: Show footnotes opens it and it stays open, so Simple has no automatic behavior to turn on. The POWER_ONLY_COMMANDS comment and the ADR Consequences now say that instead of "has no place for it".
That's a better reason than I had, and it corrects my premise rather than just outweighing it. I read the Simple default being false as evidence the feature was stranded; I didn't know Simple keeps PT9's manual pane, which means there's no automatic behaviour to switch on in the first place. Recorded reason reads correctly — no change wanted.
extensions/src/platform-scripture-editor/contributions/menus.json line 138 at r1 (raw file):
Previously, katherinejensen00 wrote…
#1 — Keeping these hidden in Simple to follow the v0 Simple design, which has no quality tools in the Project menu. I have asked UX to confirm and will revisit if they want them back. The POWER_ONLY_COMMANDS comment and the ADR Consequences now give that reason (and note UX has not confirmed yet) instead of "has no entry point".
Agreed as a decision — the recorded reason is what this was really asking for. POWER_ONLY_COMMANDS and the ADR Consequences now both say it follows the v0 Simple design and that UX has not yet confirmed, which is the honest state and no longer circular.
That leaves UX's answer as the only thing still open here. If they want the inventories or the Markers Checklist back in Simple it's a menus.json change plus dropping those ids from the allow-list — the test will tell you exactly which.
extensions/src/platform-scripture-editor/src/main.ts line 1145 at r1 (raw file):
Previously, katherinejensen00 wrote…
#4 / #6 — Done, without an activate() harness. The Bible texts/Commentaries handlers and getProjectIdOfWebView moved into show-panel.util.ts (showBibleTextsTab / showCommentariesTab bind their own web view type), and main.ts registers those functions directly. show-panel.util.test.ts now covers each function raising and creating its own panel type (swapping the constants fails), plus getProjectIdOfWebView's three paths.
Verified at 74d74b6 — and this is a better answer than the one I suggested. Moving the handlers into show-panel.util.ts as showBibleTextsTab/showCommentariesTab, each binding its own web view type, puts the wiring under test without an activate() harness and the dozen-odd @papi/backend mocks it would have needed. getProjectIdOfWebView moving across with them means it's exported and covered rather than only reachable through a command.
extensions/src/platform-scripture-editor/src/main.ts line 1443 at r1 (raw file):
Previously, katherinejensen00 wrote…
#12 / #14 — Done. Bible texts and Commentaries warn with a new generic "That tab couldn't be opened" notice (en + es) instead of failing silently. Text collection now shows "isn't available" only when the error contains "Cannot find Web View Provider", and otherwise warns in the log and shows the generic notice. Tests cover both paths, using the wrapped JSON-RPC message.
Verified at 74d74b6 — both panels now route through showProjectPanelTab, which warns with the new tabCouldNotOpen key rather than failing silently, and the key is in both en and es. The three Tools items behave consistently on failure now.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 3560 at r1 (raw file):
Previously, katherinejensen00 wrote…
#5 / #13 — Done. The branch moved to handleEditMenuCommand in edit-menu-actions.util.ts, with notices, selection restore, error reporting and refocus passed in as callbacks; the web view still calls it synchronously. A durable block now takes priority over the sync message. Tests cover ran, sync-only block, durable block during a sync, no editor, and a throwing action.
Verified at 74d74b6 — if (isSyncBlocked && !isDurablyReadOnly) gives the durable reason priority, and the comment beside it now states why rather than leaving the overlap unexplained. A reviewer-role user during an automatic sync gets the right message.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 3564 at r1 (raw file):
Previously, katherinejensen00 wrote…
#5 / #13 — Done. The branch moved to handleEditMenuCommand in edit-menu-actions.util.ts, with notices, selection restore, error reporting and refocus passed in as callbacks; the web view still calls it synchronously. A durable block now takes priority over the sync message. Tests cover ran, sync-only block, durable block during a sync, no editor, and a throwing action.
Verified at 74d74b6, and the property I most wanted kept is intact: handleEditMenuCommand is synchronous and returns void, and the web view still calls it inline, so the click's user activation survives all the way to the clipboard call.
Passing restoreSelectionIfLost and focusEditor in as callbacks, with a generic <TEditor extends EditMenuTarget>, also sidesteps the type problem neatly — EditMenuTarget has neither .focus() nor the selection pair, so widening it would have been the awkward way to do this.
extensions/src/platform-scripture-editor/src/show-panel.util.ts line 62 at r1 (raw file):
Previously, katherinejensen00 wrote…
#12 / #14 — Done. Bible texts and Commentaries warn with a new generic "That tab couldn't be opened" notice (en + es) instead of failing silently. Text collection now shows "isn't available" only when the error contains "Cannot find Web View Provider", and otherwise warns in the log and shows the generic notice. Tests cover both paths, using the wrapped JSON-RPC message.
Verified at 74d74b6. The substring match is the right shape, and the test rejecting with the fully wrapped JSON-RPC Request error (-32603): getWebView: Cannot find Web View Provider for webview type … string is the part that actually matters — an equality match on the bare text would have passed CI and then failed for every real user, because the message crosses a process boundary before it reaches this catch. The constant's doc comment saying so is a good touch.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 91 at r1 (raw file):
Previously, katherinejensen00 wrote…
#19 — Done. The Tooltip wrapper is only rendered when the item has a tooltip. Added a test asserting data-state="open" and data-slot="dropdown-menu-sub-trigger" on an open flyout's trigger. I didn't harden DropdownMenuSubTrigger for the tooltip-plus-submenu case, since nothing uses it yet.
Verified at 74d74b6. Agreed on not hardening DropdownMenuSubTrigger while nothing pairs a submenu with a tooltip — the conditional wrapper fixes every case that exists, and the data-state="open" / data-slot="dropdown-menu-sub-trigger" test pins it.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 98 at r1 (raw file):
Previously, katherinejensen00 wrote…
#20 — Latent (menu.util.ts is unchanged and every shipped flyout's children share their parent's gating). I'd rather handle it with both renderers in a follow-up than half-fix it here.
Agreed — latent today, and doing it across both renderers in one go is the right shape rather than half of it here. Worth folding into the same follow-up as the menubar tooltip and dir items, since the emptiness check lands in platform-menubar.component.tsx too and those are all the same file set.
lib/platform-bible-react/src/components/shadcn-ui/dropdown-menu.tsx line 356 at r1 (raw file):
Previously, katherinejensen00 wrote…
#21 — Agreed, no change. RTL isn't reachable in the shipped app yet.
Agreed, no change — this one was only ever worth stating so the RTL work isn't read as shipped.
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 102 at r1 (raw file):
Previously, katherinejensen00 wrote…
#4 / #6 — Done, without an activate() harness. The Bible texts/Commentaries handlers and getProjectIdOfWebView moved into show-panel.util.ts (showBibleTextsTab / showCommentariesTab bind their own web view type), and main.ts registers those functions directly. show-panel.util.test.ts now covers each function raising and creating its own panel type (swapping the constants fails), plus getProjectIdOfWebView's three paths.
Verified at 74d74b6. The show-panel.util.ts restructure covers this — each function binds its own web view type, so swapping the two constants fails a test directly. Leaving TAB_FOR_COMMAND as the hand-maintained mirror is the right call given core can't import extension source.
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 1 comment.
Reviewable status: 0 of 54 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 129 at r2 (raw file):
} = topMenu; return Object.entries(groups) .filter(([, group]) => group.column === 'platformScriptureEditor.simpleTools')
#28 - low · checked and confirmed
A TOOLS group keyed the same as its column drops out of the Tools-order assertion silently, so the check the Definition of Done leans on would go quiet rather than fail.
What happens: simpleToolsCommands() selects the TOOLS groups with group.column === 'platformScriptureEditor.simpleTools' alone (shipped-simple-layout-order.test.ts:128-136). A group keyed the same as its column also renders under it — the second branch of isGroupUnderColumnOrSubMenu in lib/platform-bible-react/src/components/advanced/menus/menu.util.ts. That is the same rule you restated as isGroupUnderColumn in menu-data.service-host.scripture-editor-menu.test.ts:68-70, so the two-branch rule landed in one of the two tests that re-implement it and not the other.
Why it matters: latent rather than live. No shipped menu keys a group the same as its column today, so the assertion is correct for the menu as it stands. It bites when a future TOOLS group is written that way: those commands drop out of the derived list, both sides still match, and the test stays green. That assertion is the one the ticket's Definition of Done rests on — "Tools' order is derived from, and asserted against, the column-3 tab order" — and losing it quietly is worse than losing it loudly.
Fix: apply the two-branch rule at :129 as well — either lift isGroupUnderColumn somewhere both tests can import it, or restate it locally with the same keep-in-sync comment pointing at menu.util.ts.
4880415 to
7b5089f
Compare
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 made 1 comment.
Reviewable status: 0 of 60 files reviewed, 1 unresolved discussion (waiting on irahopkinson+AI).
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 129 at r2 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
#28 - low · checked and confirmed
A TOOLS group keyed the same as its column drops out of the Tools-order assertion silently, so the check the Definition of Done leans on would go quiet rather than fail.
What happens:
simpleToolsCommands()selects the TOOLS groups withgroup.column === 'platformScriptureEditor.simpleTools'alone (shipped-simple-layout-order.test.ts:128-136). A group keyed the same as its column also renders under it — the second branch ofisGroupUnderColumnOrSubMenuinlib/platform-bible-react/src/components/advanced/menus/menu.util.ts. That is the same rule you restated asisGroupUnderColumninmenu-data.service-host.scripture-editor-menu.test.ts:68-70, so the two-branch rule landed in one of the two tests that re-implement it and not the other.Why it matters: latent rather than live. No shipped menu keys a group the same as its column today, so the assertion is correct for the menu as it stands. It bites when a future TOOLS group is written that way: those commands drop out of the derived list, both sides still match, and the test stays green. That assertion is the one the ticket's Definition of Done rests on — "Tools' order is derived from, and asserted against, the column-3 tab order" — and losing it quietly is worse than losing it loudly.
Fix: apply the two-branch rule at
:129as well — either liftisGroupUnderColumnsomewhere both tests can import it, or restate it locally with the same keep-in-sync comment pointing atmenu.util.ts.
#28 — Done. simpleToolsCommands() now applies the two-branch rule. I restated it locally as isGroupUnderColumn with the keep-in-sync comment pointing at isGroupUnderColumnOrSubMenu in menu.util.ts, matching the sibling restatement in menu-data.service-host.scripture-editor-menu.test.ts rather than lifting a shared helper — the package doesn't re-export the real one from index.ts, and the two restatements now read identically so they drift together or not at all. A TOOLS group keyed the same as its column is picked up by the derived list again, soils loudly instead ofboth sides shrinking. 5a034fc.
950a411 to
63af688
Compare
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 16 comments.
Reviewable status: 0 of 70 files reviewed, 17 unresolved discussions (waiting on katherinejensen00).
a discussion (no related file):
A third review pass, scoped to the five commits added since the last round — c4769a6..63af688, 12 source files and 195 insertions. The earlier commits were deliberately not re-reviewed; both dist/ bundles were skipped and their sources read instead.
15 inline comments follow — two high, six medium, the rest low. Each carries its severity and a short status: checked and confirmed means it was verified against the code, needs a human call means it could not be settled either way and the judgement is yours.
Several of these exist because you did what the last round asked, and they read better with that context:
- The Quality checks section is here because of round 1's finding about Simple losing every quality tool. The direction is right; the notes on it are about the record and the label.
- The Tools-order comment follows from the mapping fix — adding
TAB_FOR_COMMANDwas right, and it is the set comparison that came with it that dropped the order assertion. - The "rule is now in three places" note is the direct sum of two earlier findings, each of which asked for a local copy. I have said so in that comment rather than presenting it as a new defect.
Two things to read first. The Tools-order comment on shipped-simple-layout-order.test.ts:293 is the only one marked blocking, and it also carries a single ticket-record ask covering both of this PR's 2026-09-23 reversals, so the Quality-checks comment points at it rather than repeating it. The Open Checks comment on menus.json:368 is the one I would most like a second opinion on: the reasoning is a static trace through rc-dock, and seeing the split in a running app would settle it in a minute.
A correction to something an earlier round of mine got wrong, since it is load-bearing for the zoom comments: I initially read the gating of the zoom items as leaving Simple with no zoom at all. That is not right. Ctrl+wheel and Ctrl+=/−/0 act on the editor in every mode through a path this PR does not touch, and the keyboard route into the tab menu appears intact too. What the gating removes is one mouse route, and the recorded justification names a substitute that does not work by mouse for the editor's column. That comment is low, not high, and it is about the wording rather than the behaviour.
One finding in a file this pull request does not change
low · checked and confirmed
lib/platform-bible-react/src/stories/advanced/tab-navigation/tab-dropdown-menu.stories.tsx:18
Storybook still tells a component consumer that every section is labeled when there are two or more, and offers no page where the new headingless section can be seen.
What happens: the component-level description states the heading rule without the new exception, which the component's own TSDoc now carries (tab-dropdown-menu.component.tsx:168-169). The sibling rendering variation has a story — SingleSection at :272-297 documents "no heading and no divider" — but isHeaderHidden has none.
Why it matters: extension authors read this page to decide how to shape their columns, and the one rendering mode whose accessibility is in question has no rendered example. Worth being precise about the stakes: the repo's Storybook a11y checks run as a11y: { test: 'todo' } (.storybook/preview.ts:38-41), so they surface violations rather than failing CI. Adding a story would make this mode visible to those checks, not enforced by them — right now neither happens.
Fix: amend the description to name the isHeaderHidden exception, and add a story beside SingleSection rendering a column with the flag set, tagged ['test'] like its neighbours, whose description states that the section keeps its divider and its accessible name.
The pull request description is describing the previous state
The review-paratext summary between the <!-- review-paratext:summary --> markers still describes the branch as it stood before these five commits. Six points are now wrong at 63af688:
- The sections row reads "Project · View · Insert · Tools" — it omits the new unheaded Edit section and Quality checks.
- "Project | Open Project Settings… · Edit ▸" — Edit ▸ moved out of Project into its own column (
menus.json:12-16,:251-257). - The Tools row lists Comments third; the served order now has it last.
- "Tools now mirrors the third column's tabs, in the order they appear there" — no longer true, and the subject of the blocking comment.
- "The inventories, Markers Checklist and Check Results are hidden in Simple" — Open Checks is now shown in Simple.
- "Diff size: 31 source files. The other 13 are the regenerated
platform-bible-react/distbundle" — the pull request is now 58 files: 39 source, plus 14 inplatform-bible-react/distand 5 inplatform-bible-utils/dist.
isHeaderHidden is not mentioned at all, and it is the most consequential thing in this delta — a new field on the public contribution schema. The "Why Simple gets its own columns" panel also still says that teaching the shared menu model per-mode columns "is a platform change for one consumer, so it was rejected for now", which now sits beside an accepted platform change to that same shared model for that same consumer. The inline note on Architecture-Decisions.md:3122 covers why those two are in fact different; the description would read better if it drew the same distinction.
The description is yours to regenerate — flagging it because it is the first thing a reviewer reads.
extensions/src/platform-scripture-editor/contributions/menus.json line 13 at r3 (raw file):
"columns": { "platformScriptureEditor.simpleEdit": { "label": "%webView_platformScriptureEditor_edit%",
low · needs a human call
In the one place isHeaderHidden ships, the section and its only item have the same name.
What happens: this column's label is %webView_platformScriptureEditor_edit%, which TabDropdownMenu turns into the group's aria-label. The column holds exactly one group (platformScriptureEditor.simpleEditMenu, :93-95) with exactly one item, the submenu trigger platformScriptureEditor.editSubmenu (:250-256), whose label is the same key. Both resolve to "Edit" in English and "Editar" in Spanish (localizedStrings.json:91, :309). A screen-reader user entering the section hears "Edit", then "Edit".
Why it matters: less as a harm than as a signal. This is the only production use of isHeaderHidden in the tree, so the accessible name that justifies the whole aria-label branch — and the untested guard flagged on tab-dropdown-menu.component.tsx:234 — buys nothing in the one place it is exercised. A sighted user just sees the flyout row. That is worth a moment's thought about whether the flag is the right tool here, rather than only a wording fix.
Fix: this is a content decision rather than a mechanical one, so it needs your judgement or UX's. Either give the column its own label describing the section rather than repeating its item — a new key needs en and es entries in alphabetical position — or keep the duplication deliberately and note it in the ADR entry, so the next reader does not mistake the group name for information.
extensions/src/platform-scripture-editor/contributions/menus.json line 363 at r3 (raw file):
}, { "label": "%webView_platformScriptureEditor_openChecks%",
medium · checked and confirmed
Two things about this new section: it is neither outcome the Definition of Done allows, and its label is not the one the ticket names.
Quality checks is shown in Simple. The new simpleQualityChecks column (menus.json:46-49) and this Simple-only item put a visible section into Simple, pinned at menu-data.service-host.scripture-editor-menu.test.ts:202. The DoD says "Quality checks is hidden, or deliberately omitted with the reason recorded", and the ticket's Testing Idea says "Assert the Quality-checks section is absent in Simple". Neither holds now. The reversal is recorded with a date in the ADR — "as product asked on 2026-09-23" (Architecture-Decisions.md:3139) — which is the right instinct. What is missing is the ticket-level record: PT-4534 has its own numbered "Decisions (2026-09-18)" section, and this second round did not extend it, so anyone verifying the branch against the ticket reads this as a defect. The ask covering this and the Tools-order reversal is on shipped-simple-layout-order.test.ts:293, so I am not repeating it here.
Worth saying plainly: this section exists because of round 1's finding about Simple losing every quality tool. The direction is right — it is the paperwork and the label that need to catch up.
The label is not the one the ticket specifies. This item reuses the Power key %webView_platformScriptureEditor_openChecks% → "Open Checks..." / "Abrir verificaciones...". Every sibling in Simple's menu is a sentence-case noun with no verb and no ellipsis — "Bible texts", "Commentaries", "Text collection", "Find", "Comments" — and under a "Quality checks" heading the verb and object also restate the heading just above them. PT-4534's target structure names this row "Checking assistant", so the answer is already on the ticket: it needs neither "Open Checks…" nor an invented short form. Since the DoD requires Simple to match v0's "sections, order and labels", this is a DoD item rather than a style preference.
Fix: mint a new key rather than editing the existing one — it is immutable under Localization-Guide.md:400-402 and is shared with the Power item at menus.json:219. Add en and es values in alphabetical position in contributions/localizedStrings.json and point this item's label at it. menuLocalizeKeys in localized-strings.test.ts reads labels straight off menus.json, so it picks the new key up with no test change. The Spanish needs a real translation rather than a transliteration.
extensions/src/platform-scripture-editor/contributions/menus.json line 368 at r3 (raw file):
"group": "platformScriptureEditor.simpleChecks", "order": 1, "command": "platformScripture.openChecksSidePanel"
high · checked and confirmed
In Simple mode, choosing Quality checks ▸ Open Checks… splits the editor's column into two panes, so the workspace stops matching Simple's fixed three-column layout — and it does this again on every click.
What happens: this item runs platformScripture.openChecksSidePanel, whose handler (extensions/src/platform-scripture/src/main.ts:158-190) calls openWebView(checksSidePanelWebViewType, { type: 'panel', direction: 'right', targetTabId }, options) with no interface-mode branch. The renderer's panel branch resolves the editor tab's parent and calls dockLayout.dockMove(tab, targetTab.parent, 'right') (platform-dock-layout-storage.util.ts:1113-1125). rc-dock's dockPanelToPanel takes Column 2's own wrapper box, and because the requested split is horizontal where the wrapper is vertical, builds a new horizontal child box and substitutes it inside — so dockbox.children.length stays 3, but the user sees four resizable panes. Nothing reverts it mid-session: saveLayout no-ops in Simple and loadLayout re-applies the static layout only on startup or a mode switch.
It compounds. options never sets existingId, and openWebView guards its whole reuse branch on if (optionsDefaulted.existingId) (src/renderer/services/web-view.service-shard.ts:3381), with defaults injected only when one is already present (:2511). So no reuse lookup happens and every invocation creates another web view and another split — a fifth pane, a sixth, and so on. The reuse idiom is well established in this very file: REUSE_EXISTING_FIND_ONLY at main.ts:390 (used at :417, :497, :543) and Manage Books' existingId: '?' at :344. openFind even carries the comment "Ignored in Simple mode, where the fixed layout already holds a Find tab for the probe below to find". Open Checks is the one panel-opener without a probe.
Why it matters: it is reachable by an ordinary menu click with no unusual setup, and Simple's fixed, non-restructurable layout is the point of the mode. checksSidePanel is absent from FIXED_LAYOUT_WEBVIEW_GROUPS (platform-dock-layout-positioning.util.ts:91-99), so nothing pins it into Column 3, and because its group hangs off simpleQualityChecks rather than simpleTools, TAB_FOR_COMMAND in shipped-simple-layout-order.test.ts:136 does not cover it — that suite passes 11/11 while exercising none of this.
Fix: four parts, and the first is the one that matters most.
- Give
openChecksSidePanela reuse-first probe mirroringREUSE_EXISTING_FIND_ONLY. This alone stops the stacking. - Give Checks a static presence in Column 3 for Simple — either in
simple-layout.data.tsor viadefault-layout-supplement.jsonbehind a flag, following thescriptureTextGridprecedent. Without this the probe finds nothing on first use and still falls through to a placement decision. - Add
'platformScripture.checksSidePanel'toFIXED_LAYOUT_WEBVIEW_GROUPSwithTAB_GROUP_RESOURCES. - Extend
TAB_FOR_COMMANDso the layout suite covers the Quality checks group — which only works once (2) exists.
Swapping the layout literal alone would not match the established pattern and would leave the compounding in place even if the first click landed correctly. Worth one run in the app to see the split, since the exact rendering was reasoned from the rc-dock algorithm rather than observed.
src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts line 206 at r3 (raw file):
}); test("Simple's Edit section is shown without a heading, and every other section has one", async () => {
low · checked and confirmed
The name promises rendering coverage this layer cannot provide, and the second half is never asserted.
What happens: the body reads simpleMenu.columns and asserts which columns carry a truthy isHeaderHidden. Nothing renders, so "is shown without a heading" is not checked here — that behaviour is pinned separately at tab-dropdown-menu.component.test.tsx:178-192. And the filter runs over every declared column in the served document, including ones with no items in Simple (platformScriptureEditor.options, .edit, .tools, .info), which are not sections at all — so "every other section has one" is not checked either, in either direction.
Why it matters: a reader takes the heading behaviour as pinned at the served-menu level when it is not. Someone deleting or rewriting the component test later would believe this still covers it.
Fix: rename it to what it pins — something like 'the served Simple menu marks only the Edit column isHeaderHidden' — and leave the rendering claim to the component test. If you would rather the name stay as it is, the alternative is to narrow headerHiddenColumns to the columns that actually have items in Simple and assert the complement explicitly.
.context/standards/Entry-Point-Guide.md line 149 at r3 (raw file):
section titles, and hide a section by hiding its items. See `adr-menu-section-headings-from-column-labels`. - **A section with no heading** sets `"isHeaderHidden": true` on its column. It is still divided from its neighbors, and its `label` still names it for screen readers, so give it a real label.
low · checked and confirmed
This states the accessible name unconditionally, and there is a live case where it does not hold.
What happens: tab-dropdown-menu.component.tsx:186 computes showHeadings = showSectionHeadings && sections.length > 1, and :234 sets aria-label={showHeadings && isHeaderHidden ? label : undefined}. So the label names the section only while two or more sections have items.
The showSectionHeadings half is largely theoretical — both bundled consumers hard-code it true (tab-toolbar.component.tsx:78, :116; tab-floating-menu.component.tsx:26). The sections.length > 1 half is not: a column with isHeaderHidden: true left as the only populated section gets no heading and no accessible name at all, and interface-mode filtering emptying its siblings is exactly the mechanism this guide documents two bullets up. The component's own props doc already carries the caveat — "Only takes effect when two or more sections have items" (tab-dropdown-menu.component.tsx:146-147) — and this bullet drops it when restating the feature. It is untested too: tab-dropdown-menu.component.test.tsx:196-205 covers the single-section case but never pairs it with isHeaderHidden.
Why it matters: .claude/rules/docs-durability.md asks that a behaviour stated in a living doc be checked against the live repo. An author reading this sets the flag expecting a name and, in the one configuration where the section stands alone, gets neither heading nor name.
Fix: qualify the sentence — the label becomes the group's accessible name in a menu that heads its sections, and only while two or more sections are shown; a lone remaining section, including one left alone by interface-mode filtering, gets no heading and no name. Give it a real label regardless. The same qualification belongs on the TSDoc at menus.model.ts:60-61, which makes the promise in the same unconditional form.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 234 at r3 (raw file):
aria-labelledby={showHeading ? headingId : undefined} // A section shown without its heading is still named for assistive technology aria-label={showHeadings && isHeaderHidden ? label : undefined}
medium · needs a human call
A section now gets its accessible name from one of two different mechanisms depending on a flag, and the unheaded branch is both untested and unprecedented in this repo. I think one mechanism would be better than two, but the deciding question needs a screen reader, so this is your call rather than mine.
What happens: a headed section is named by aria-labelledby pointing at a real DropdownMenuLabel child. The unheaded one instead sets aria-label on the bare <div role="group"> that DropdownMenuGroup renders (shadcn-ui/dropdown-menu.tsx:172-174 → Radix MenuGroup, which emits role="group" literally). The attribute does land — that part is verified.
The guard is untested, and two obvious rewrites survive the suite. Running the full 25-test suite against a mutated copy: replacing the aria-label expression with aria-label={label} passes 25/25, and with aria-label={isHeaderHidden ? label : undefined} also passes 25/25. The new test at tab-dropdown-menu.component.test.tsx:178-194 only ever opens with showSectionHeadings=true, and the no-headings test at :207 uses a fixture with no isHeaderHidden column, so the combination the guard exists for is never rendered. aria-label={label} is precisely the "simplification" a later reader makes; it would name every group in the deliberately-unnamed mode and stack a redundant name under aria-labelledby on the headed ones.
The open question. Whether a name on role="group" nested in role="menu" is actually announced varies by screen reader. There is no convention here to lean on: this is the only aria-label on a DropdownMenuGroup anywhere in the repo, and there is no accessibility page under stories/guidelines/. If it is not conveyed, the flag's accessibility half is decoration and the user who most needs the cue gets an anonymous run of items between two named ones.
Suggested direction — keep one mechanism. Always render the DropdownMenuLabel id={headingId} and always set aria-labelledby, adding className="tw:sr-only" when isHeaderHidden. That moves naming onto a mechanism this codebase already uses for exactly this purpose (conflict-note-card.component.tsx:210-213, with a comment documenting the visible-versus-announced split; also data-table-pagination.component.tsx:56), and it deletes the untested branch above rather than asking you to write a test for it.
I checked the obvious objection — that an always-present label node would disturb the menu — and it does not hold: only MenuItem is registered in Radix's Collection.ItemSlot that backs roving focus and typeahead (@radix-ui/react-menu MenuItem, wrapping MenuItemImpl); MenuLabel is a bare Primitive.div that was never part of that machinery, visible or not.
Fix: either adopt the single-mechanism change above and add one test asserting the hidden-header section's accessible name comes through in both showSectionHeadings states — or, if you prefer to keep aria-label, record a check against at least one screen reader (NVDA or VoiceOver) confirming the group name is spoken on entry, and add a case pairing isHeaderHidden: true with showSectionHeadings=false asserting no group is named. That single case kills both mutants above; I verified it does.
lib/platform-bible-react/src/components/advanced/menus/menu.util.ts line 92 at r3 (raw file):
/** The column's localized label */ label: string; /** Whether the section is shown without its label as a heading; see `isHeaderHidden` */
low · checked and confirmed
"see isHeaderHidden" points at the field it is documenting, so the pointer resolves to itself.
What happens: the referent you mean is MenuColumnWithHeader.isHeaderHidden in platform-bible-utils (menus.model.ts:58-64), which is where the semantics, the menubar carve-out and the accessibility promise are written down. Unqualified, the name resolves to the member it annotates — this one, set from the column's flag at menu.util.ts:113.
Why it matters: MenuSection is the shape every future renderer of menu sections destructures, and this sentence is the only documentation the field gets on this side. It is internal — neither MenuSection nor getMenuSectionsWithItems is re-exported from lib/platform-bible-react/src/index.ts — so no extension author hits it, which keeps this minor.
Fix: name the source of truth. MenuColumnWithHeader is already imported at menu.util.ts:5, so a TSDoc link resolves:
/** Whether the section is shown without its label as a heading; see {at-link MenuColumnWithHeader.isHeaderHidden} */
I have written that as {at-link} on purpose — the real @ spelling inside a review comment is parsed as a mention here, which creates a phantom participant and blocks the review from publishing. Use the proper @ form in the source file. If you would rather avoid the link entirely, plain prose does the job too: "mirrors MenuColumnWithHeader's isHeaderHidden field in platform-bible-utils".
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 123 at r3 (raw file):
* it is keyed the same as the column. */ function isGroupUnderColumn(groupKey: string, group: { column?: string }, columnKey: string) {
medium · checked and confirmed
The group-under-column rule now exists in three places, and this copy is the third.
What happens: menu-data.service-host.scripture-editor-menu.test.ts:62-69 already held this function and docblock — it was there at the review base — and this commit adds a byte-identical copy here. Both restate isGroupUnderColumnOrSubMenu (lib/platform-bible-react/src/components/advanced/menus/menu.util.ts:76-84), and both carry a comment instructing the reader to keep them in sync by hand.
I should own where this came from. The two copies exist because two earlier findings in this review asked for them — one on the served-menu snapshot and one on this file — each proposing a local restatement, and each noting the symbol is not reachable from platform-bible-react's public entry point. That was accurate as far as it went: isGroupUnderColumnOrSubMenu is exported from its own module, just not re-exported from lib/platform-bible-react/src/index.ts. What neither finding anticipated is that satisfying both would put the same nine lines in two test files plus production. The consolidation those fixes deferred is now worth doing.
Why it matters: the rule has to change in three files at once, and the copies' own comments admit it. That is the sync burden made explicit rather than removed.
Fix: export isGroupUnderColumnOrSubMenu from lib/platform-bible-react/src/index.ts and import it in both tests, which removes the restatement entirely and makes drift impossible. If widening the public surface is unwelcome for a test-only need, the alternative is one shared helper module both tests import, with the docblock in that one place.
On the second branch specifically: no group in either shipped menu document is keyed the same as a column, so groupKey === columnKey never fires when the helper is called with a column key — I re-checked that this round. That is not a defect to fix. The earlier finding said the same thing when it asked for the branch ("a latent divergence rather than an active blind spot"), and the branch is live in production via tab-dropdown-menu.component.tsx:56, where the key can be a submenu key. Keeping it is right; it is only the duplication that is worth removing.
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 293 at r3 (raw file):
mappedTabs.forEach((tab) => expect(tab).toBeDefined()); expect(mappedTabs).toHaveLength(new Set(mappedTabs).size); expect(new Set(mappedTabs)).toEqual(new Set(columnWebViewTypes(merged, 2)));
high · checked and confirmed
Nothing in the repo now compares Simple's TOOLS order to the third-column tab order. The two survive only as independent hand-written literals that are deliberately different from each other.
What happens: this line changed from expect(mappedTabs).toEqual(columnWebViewTypes(merged, 2)) to a new Set(...) comparison, so it checks membership only. The served TOOLS order is now a hard-coded array at menu-data.service-host.scripture-editor-menu.test.ts:192-201 — Bible texts · Commentaries · Text collection · Find · Comments — and the tab order a separate hard-coded array at shipped-simple-layout-order.test.ts:269-277 — Bible texts · Commentaries · Comments · Text collection · Find. They differ only in where Comments sits. A reorder of either still fails its own literal, so nothing is silent; what is gone is the derivation.
Why it matters: this is not a stale Definition-of-Done line that the code has outgrown. PT-4534's "As shipped (2026-09-21)" section records that Tools' order was pinned to the tab order, explicitly "because 1.6f says the menu mirrors the UI" — and that this was itself a deliberate deviation from the v0 screenshot, made so that DoD line could be met. The ADR sentence this PR replaces said Simple's Tools order "is pinned to the third-column tab order"; the new text says it "follows the design … not the third-column tab order". So a previously satisfied requirement is being reversed, not clarified. The WI-9 Dictionary case the Testing Idea names is still caught (set equality fails when a tab has no item), but a tab and an item both added in mismatched relative order now passes.
One root cause, noted once here. This PR encodes two reversals of previously settled PT-4534 decisions, both re-decided around 2026-09-23: Tools' order (this one) and Quality checks (omitted in Simple, now shown — see the note on menus.json:363). Only the Quality-checks reversal carries a citation anywhere in the diff — "as product asked on 2026-09-23", Architecture-Decisions.md:3139. This one carries none: not in the ADR, not in the commit message, not on the ticket. PT-4534 already has a numbered "Decisions (2026-09-18)" section; adding one "Decisions (2026-09-23)" section covering both reversals, and amending the DoD and Testing-Idea bullets to match, closes both gaps at once.
Fix: settle which reading of 1.6f governs and record it. If 1.6f still holds, restore the order-preserving assertion and put Comments back where the tab order has it. If product genuinely prefers the v0 static order, that is a fine answer — but then the DoD line and the Testing Idea need amending on the ticket, because as written no test can satisfy them. Either way, if the set comparison stays, rename the test away from "order" language and say in its doc comment that the menu order is pinned elsewhere by literal rather than derived.
extensions/src/platform-scripture-editor/src/contributions-zoom-menu.test.ts line 56 at r3 (raw file):
* `lib/platform-bible-react/src/components/advanced/menus/menu.util.ts`) — so a single ungated * zoom item here would put the whole Options column, heading included, into Simple's Project * menu, which the Simple design has no Options column in. Simple still reaches zoom from the tab
low · checked and confirmed
The closing sentence here is imprecise about the editor specifically, and the same paragraph exists in three places — so the imprecision had to be written three times.
On the sentence. "Simple still reaches zoom from the tab menu, which core's defaultWebViewTabMenu serves in every mode" is true for Column 3's tabs and misleading for the editor. The tab menu's only trigger is a context menu on the tab title (platform-tab-title.component.tsx:1086-1095), and in Simple the editor sits in HEADLESS_GROUP (platform-dock-layout-positioning.util.ts:100-102) whose dock bar is pointer-events: none, with opacity: 0 held even on :focus-within (dock-layout-wrapper.simple-mode.scss:56-65). So for this column there is no tab title to right-click.
To be clear about what is and is not lost, because the first version of this note overstated it: no zoom capability goes away. Ctrl+wheel and Ctrl+=/−/0 act on the editor in every mode via web-view-content-zoom.chrome-keys.ts:84-114, registered unconditionally at renderer/index.tsx:158 and gated only on input-blocking, never on interface mode — neither file is touched by this PR. The keyboard route into the tab menu itself also appears intact: .dock-tab-btn carries tabIndex: 0 from rc-tabs, simpleConfig sets only tabLocked, nothing sets inert or aria-hidden, and platform-tab-title.component.tsx:1023-1053 forwards contextmenu into the trigger — covered by platform-tab-title.zoom-menu.test.tsx:434-471 ("keyboard access in Simple mode"). What commit 947a194 did remove is a working mouse route: before it, the three zoom items were ungated and appeared in Simple's Project menu. Gating them is the right call; the recorded reason just names a substitute that does not work by mouse for this column.
On the triplication. The same chain — every Options item is Power-only → a column ships if any item survives → one ungated item resurrects the column → Simple gets zoom from the tab menu — is written out here, at menu-data.service-host.scripture-editor-menu.test.ts:262-266, and at Architecture-Decisions.md:3141-3147, in near-identical prose. That one wrong sentence appearing in all three is the argument for not keeping three copies: when the Options column's composition changes, whichever copies are not edited become confident, wrong guidance.
Fix: correct the closing sentence in all three places to name the routes that actually work in Simple — the Ctrl+wheel and Ctrl+=/−/0 chords, with the tab menu noted as keyboard-only for this column. Then thin the duplication: keep the full reasoning here, where it sits next to the assertion it explains, and shorten the menu-data.service-host copy to a one-line pointer at this test. I would not point either test at the ADR — sending a reader of a failing test into a 7500-line standards file is a worse trade than the duplication, and the sibling comments in that same array are all self-contained.
Please don't add a Simple-owned zoom column to solve this. It would duplicate a capability that already works, and this delta is already adding four columns.
extensions/src/platform-scripture-editor/src/contributions-zoom-menu.test.ts line 59 at r3 (raw file):
* menu, which core's `defaultWebViewTabMenu` serves in every mode. */ it('hides every zoom item in Simple, and marks experimental on the group rather than the items', () => {
low · checked and confirmed
The name credits an assertion that lives in a different test.
What happens: the body reads items and asserts, for each zoom item, its hiddenInterfaceModes and the absence of isExperimental. So "hides every zoom item in Simple" is covered, and so is the "rather than the items" half — expect('isExperimental' in item).toBe(false) is exactly that. What is not here is the "marks experimental on the group" half: expect(zoomGroup.isExperimental).toBe(true) lives in the separate test at :29-37.
Why it matters: a green run reads as evidence for a group-level claim this case does not check, and a failure report names an assertion that is not in the body. Minor, but the earlier name had the same shape, so the rename carried it forward rather than fixing it.
Fix: name only what it asserts — e.g. 'hides every zoom item in Simple and leaves isExperimental off the items' — and let the test at :29 keep the group claim.
e2e-tests/tests/markers-checklist/wiring-theme-5.spec.ts line 41 at r3 (raw file):
import { waitForAppReady } from '../../fixtures/helpers'; // Markers Checklist and Open Checks are opened from the scripture editor's hamburger menu, which
medium · checked and confirmed
This gate's stated reason is no longer true, and it was this PR that made it untrue.
What happens: the comment says Simple "hides both items". Commit 63af688 serves platformScripture.openChecksSidePanel in Simple (menus.json:362-369, pinned at menu-data.service-host.scripture-editor-menu.test.ts:202), and openChecksSidePanel is absent from POWER_ONLY_COMMANDS. Test 8 at :577-596 clicks the Project button and picks /Open Checks/i — a flow that now works in Simple too. The Markers Checklist half is still correct: platformScripture.openMarkersChecklist remains in POWER_ONLY_COMMANDS at :254, so the requiredInterfaceMode: 'power' line below should stay.
Timeline, for fairness: the comment came in at 7fe924bdfc2 ("Regroup Simple's Project menu to the v0 structure", 2026-09-23 16:05), and 63af688 invalidated it at 16:09 — four minutes later, in the same PR. So this is not a stale assumption inherited from elsewhere; it is one commit in this PR overtaking another. Nothing wrong with that happening, it just needs the comment brought along.
Why it matters: this comment is the only thing explaining why the file is Power-gated. A future reader either believes Open Checks is Power-only, or discovers it is not and deletes the gate — at which point the Markers Checklist tests fail in Simple.
Fix: reword the comment to attribute the gate to Markers Checklist alone, and note that Open Checks is reachable in both modes. The test.use line itself does not need to change.
lib/platform-bible-utils/src/extension-contributions/menus.model.ts line 60 at r3 (raw file):
/** * Set to `true` to show this column's items without its header text in a menu that heads each * section with its column's header, such as a web view's tab menu. The label still names the
medium · checked and confirmed
The one example this description gives is the one menu where the flag cannot be set.
What happens: the text — in both this TSDoc and the JSON-schema description at :333-337, which must stay aligned — says the flag applies "in a menu that heads each section with its column's header, such as a web view's tab menu". But in this same model a tab menu is tabMenu / defaultWebViewTabMenu, both $ref: '#/$defs/singleColumnMenu' (:556-559, :270), and SingleColumnMenu is { groups, items } (:155-160) — no columns field at all. Since isHeaderHidden lives on MenuColumnWithHeader and is only reachable through columns, it cannot be expressed on a tab menu; the schema would reject the key outright. The menu that actually heads its sections with column labels is topMenu (:548-551), which is what TabDropdownMenu renders — it takes Localized<MultiColumnMenu> at tab-dropdown-menu.component.tsx:134.
Why it matters: this is the published contribution surface, and this description is what an extension author sees in editor IntelliSense while writing menus.json. Following the example literally leaves them nowhere to put the flag. The confusion is sharpened by this file's own vocabulary: "tab menu" is used at :178, :194 and :232 to mean the single-column tabMenu field specifically, in explicit contrast to topMenu. The feature's own origin entry in Architecture-Decisions.md never calls it "the tab menu" either — it names the component and the Project menu.
I can see how the wording happened: the component is called TabDropdownMenu and the menu does open from a tab, so "tab menu" is natural shorthand. It just collides with a field of that exact name 140 lines below.
Fix: in both places, use the phrase this file already uses for topMenu at :183 — "the menu that opens when you click on the top left corner of a tab (topMenu)" — rather than introducing a second sense of "tab menu". Then rebuild lib/platform-bible-utils/dist so the shipped index.d.ts carries the corrected text.
Unrelated but worth recording since it is easy to wonder about: lib/papi-dts/papi.d.ts correctly needs no regeneration here. It reaches this model only by importing from platform-bible-utils and never inlines it — no MenuColumnWithHeader, no ambient re-declaration — and platform-bible-utils/dist/index.d.ts:1705 already carries the new field from this PR.
.context/standards/Architecture-Decisions.md line 3115 at r3 (raw file):
Simple's Comments entry is a new item that fronts the third-column Comments tab; the Power item (`legacyCommentManager.openCommentList`, which opens a separate Comment List web view) stays hidden in Simple. The design puts Edit ▸ in a section of its own with no heading, so columns take
medium · checked and confirmed
This entry was amended in place on 2026-09-23 without the log's dating and sourcing discipline. Three things, best fixed in one pass since they touch the same fifty lines.
1. A recorded consequence was reversed with no amendment note. The base entry read "Simple now has no menu route at all to the four Inventories, Markers Checklist, or Open Checks. That follows the v0 Simple design, which has no quality tools in the Project menu; UX has not yet confirmed it." That sentence is deleted and replaced at :3137-3139 with the opposite outcome for Open Checks. Meanwhile **Date:** 2026-09-18 (:3100) and **Source:** … decisions recorded on the ticket 2026-09-18 (:3151) are untouched, so the entry now carries content postdating its own stated date by five days. The log's contract (:7-45) and the root CLAUDE.md both say a consequence that no longer holds gets an **Amended YYYY-MM-DD:** note rather than being rewritten away, and the file already applies that form 15 times (:229, :243, :2089). **Status:** Accepted is still correct — updating a consequence does not change the entry's status.
2. This line asserts a design the ticket contradicts, and cites nothing. "The design puts Edit ▸ in a section of its own with no heading" carries no source or date, while the sibling Quality-checks sentence two paragraphs down cites "as product asked on 2026-09-23" (:3139) — and the same commit made both changes. PT-4534's own Target structure transcribes "PROJECT — Send/Receive this project · Project settings ⌃J · Edit ▸", i.e. Edit inside Project. So this does not merely lack a source; it contradicts a recorded one with nothing explaining the divergence. .claude/rules/docs-durability.md asks for exactly this — "date every status claim", "verify claims before landing them".
3. Six lines lost their indentation. :3142-3147 sit at column 0 inside the - **Consequences:** bullet, while :3131-3141 and :3148-3150 carry the two-space continuation indent. A sweep for a non-blank column-0 line following an indented line, excluding list, heading, quote, table, code-fence and digit starts, returns exactly one hit across the file's 7521 lines — this one. (It would miss a break on the very first continuation line after a - **Label:** start, or one beginning with a digit.) .context is in .prettierignore, so no formatter will ever normalise it, and the union-by-slug merge rule in CLAUDE.md depends on entries lining up by eye.
Fix: restore the original Consequences sentence and append an **Amended 2026-09-23:** note carrying the Quality-checks reversal, the isHeaderHidden column flag with its rejected alternatives, and the zoom-gating consequence; give the Edit ▸ relocation a source and date the way the Quality-checks sentence has one; extend the **Source:** line with the later date; and re-indent :3142-3147 by two spaces while you are in the file.
.context/standards/Architecture-Decisions.md line 3122 at r3 (raw file):
web view's own editor, and the clipboard needs the click's user activation, which a PAPI round trip loses. - **Alternatives:** `hiddenInterfaceModes` on columns/groups — rejected for now: a schema and filter
low · checked and confirmed
This rejection invites a comparison with the flag the same entry accepts, and does not answer it.
What happens: the alternative is rejected as "a schema and filter change to a shared model for one consumer" — while this PR adds isHeaderHidden to MenuColumnWithHeader and to menuDocumentSchema.$defs.columnsWithHeaders (menus.model.ts:58-64, :333-337): a column-level field in that same shared model, driven by the scripture editor alone.
Having checked, the two are genuinely different and the entry is not self-contradictory: the rejected option needed a change to the shared mode-filtering pipeline that every menu consumer runs, whereas isHeaderHidden is additive and render-only, read solely by TabDropdownMenu and explicitly ignored by the menubar. So the distinction is real. The problem is only that the sentence compresses "schema and filter" into a phrase whose first half now describes the accepted change too, leaving the next reader to work out the difference — or to conclude the log contradicts itself, which is where I started.
Why it matters: this entry is what someone reads before re-litigating a per-mode menu question, and the Alternatives section is the part they read to find out what was already ruled out.
Fix: name the filter half explicitly, so the distinction carries — something like "a schema change plus a change to the shared mode-filtering pipeline every menu consumer runs, for one consumer's need — unlike isHeaderHidden below, which is render-only and which the menubar ignores."
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 70 files reviewed, 16 unresolved discussions (waiting on katherinejensen00).
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 129 at r2 (raw file):
Previously, katherinejensen00 wrote…
#28 — Done. simpleToolsCommands() now applies the two-branch rule. I restated it locally as isGroupUnderColumn with the keep-in-sync comment pointing at isGroupUnderColumnOrSubMenu in menu.util.ts, matching the sibling restatement in menu-data.service-host.scripture-editor-menu.test.ts rather than lifting a shared helper — the package doesn't re-export the real one from index.ts, and the two restatements now read identically so they drift together or not at all. A TOOLS group keyed the same as its column is picked up by the derived list again, soils loudly instead ofboth sides shrinking. 5a034fc.
Verified — simpleToolsCommands() now filters through isGroupUnderColumn, and both branches of the rule are applied. The commit landed as 261f5c1 rather than 5a034fc after the rebase; I diffed the two and the change is byte-identical, so nothing was lost in the move.
Your reasoning for restating locally rather than lifting a shared helper is right on the facts as I gave them: the symbol is exported from its own module but not re-exported from lib/platform-bible-react/src/index.ts, so it genuinely is not importable here today, and matching the sibling restatement word for word is the best version of that choice.
One thing to flag so it does not arrive as a surprise: I have opened a note on shipped-simple-layout-order.test.ts:123 in this round about the rule now existing in three places. That is not me rejecting this fix. It is the consequence of two of my earlier findings each asking for a local copy without either one anticipating the other — the consolidation those fixes deferred. Exporting the real symbol and importing it in both tests would remove the restatement entirely; keeping the two copies in step, as you have done, is a defensible answer too. Your call, and I have said as much there.
I also re-checked the second branch this round: no group in either shipped menu document is keyed the same as a column, so groupKey === columnKey does not fire against today's data. That is what the original note said when it asked for the branch, and it remains the right thing to carry — the branch is live in production through tab-dropdown-menu.component.tsx:56, where the key can be a submenu key.
71cce5d to
d6cc687
Compare
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 2 comments.
Reviewable status: 0 of 82 files reviewed, 16 unresolved discussions (waiting on katherinejensen00).
.context/standards/Architecture-Decisions.md line 3115 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
medium · checked and confirmed
This entry was amended in place on 2026-09-23 without the log's dating and sourcing discipline. Three things, best fixed in one pass since they touch the same fifty lines.
1. A recorded consequence was reversed with no amendment note. The base entry read "Simple now has no menu route at all to the four Inventories, Markers Checklist, or Open Checks. That follows the v0 Simple design, which has no quality tools in the Project menu; UX has not yet confirmed it." That sentence is deleted and replaced at
:3137-3139with the opposite outcome for Open Checks. Meanwhile**Date:** 2026-09-18(:3100) and**Source:** … decisions recorded on the ticket 2026-09-18(:3151) are untouched, so the entry now carries content postdating its own stated date by five days. The log's contract (:7-45) and the rootCLAUDE.mdboth say a consequence that no longer holds gets an**Amended YYYY-MM-DD:**note rather than being rewritten away, and the file already applies that form 15 times (:229,:243,:2089).**Status:** Acceptedis still correct — updating a consequence does not change the entry's status.2. This line asserts a design the ticket contradicts, and cites nothing. "The design puts Edit ▸ in a section of its own with no heading" carries no source or date, while the sibling Quality-checks sentence two paragraphs down cites "as product asked on 2026-09-23" (
:3139) — and the same commit made both changes. PT-4534's own Target structure transcribes "PROJECT — Send/Receive this project · Project settings ⌃J · Edit ▸", i.e. Edit inside Project. So this does not merely lack a source; it contradicts a recorded one with nothing explaining the divergence..claude/rules/docs-durability.mdasks for exactly this — "date every status claim", "verify claims before landing them".3. Six lines lost their indentation.
:3142-3147sit at column 0 inside the- **Consequences:**bullet, while:3131-3141and:3148-3150carry the two-space continuation indent. A sweep for a non-blank column-0 line following an indented line, excluding list, heading, quote, table, code-fence and digit starts, returns exactly one hit across the file's 7521 lines — this one. (It would miss a break on the very first continuation line after a- **Label:**start, or one beginning with a digit.).contextis in.prettierignore, so no formatter will ever normalise it, and the union-by-slug merge rule inCLAUDE.mddepends on entries lining up by eye.Fix: restore the original Consequences sentence and append an
**Amended 2026-09-23:**note carrying the Quality-checks reversal, theisHeaderHiddencolumn flag with its rejected alternatives, and the zoom-gating consequence; give the Edit ▸ relocation a source and date the way the Quality-checks sentence has one; extend the**Source:**line with the later date; and re-indent:3142-3147by two spaces while you are in the file.
Line-number correction, since the rebase moved everything this comment points at.
The numbers above are from 63af688. The branch has since been rebased onto a newer main, which inserted upstream entries ahead of this one: the entry moved from line 3098 to 3180, and the file grew from 7521 to 8020 lines. The entry itself did not change — I diffed it against the old head and it is byte-identical — so all three asks stand exactly as written.
If you are reading against the current revision, anchor on the text rather than the numbers. Inside adr-menu-per-mode-layout-via-mode-gated-columns:
| what | was | now |
|---|---|---|
- **Date:** 2026-09-18 |
:3100 |
:3182 |
| "The design puts Edit ▸ in a section of its own with no heading" | :3115 |
:3197 |
| "Its only quality tool is Open Checks, under a Quality checks section, as product asked on 2026-09-23" | :3137-3139 |
:3220-3221 |
the unindented block — the Per-pane zoom (...) is Power-only for a structural reason paragraph |
:3142-3147 |
:3224-3229 |
- **Source:** PT-4534 (parent PT-4530) |
:3151 |
:3233 |
The indentation point is still live at the new numbers: :3224 and :3229 both start at column 0 while the lines around them carry the two-space continuation indent.
My fault for citing line numbers in a file that is append-only and gets rebased often — the slug plus the sentence would have survived this. Sorry for the chase.
.context/standards/Architecture-Decisions.md line 3122 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
low · checked and confirmed
This rejection invites a comparison with the flag the same entry accepts, and does not answer it.
What happens: the alternative is rejected as "a schema and filter change to a shared model for one consumer" — while this PR adds
isHeaderHiddentoMenuColumnWithHeaderand tomenuDocumentSchema.$defs.columnsWithHeaders(menus.model.ts:58-64,:333-337): a column-level field in that same shared model, driven by the scripture editor alone.Having checked, the two are genuinely different and the entry is not self-contradictory: the rejected option needed a change to the shared mode-filtering pipeline that every menu consumer runs, whereas
isHeaderHiddenis additive and render-only, read solely byTabDropdownMenuand explicitly ignored by the menubar. So the distinction is real. The problem is only that the sentence compresses "schema and filter" into a phrase whose first half now describes the accepted change too, leaving the next reader to work out the difference — or to conclude the log contradicts itself, which is where I started.Why it matters: this entry is what someone reads before re-litigating a per-mode menu question, and the Alternatives section is the part they read to find out what was already ruled out.
Fix: name the filter half explicitly, so the distinction carries — something like "a schema change plus a change to the shared mode-filtering pipeline every menu consumer runs, for one consumer's need — unlike
isHeaderHiddenbelow, which is render-only and which the menubar ignores."
Line-number correction, same cause as on the sibling thread.
The :3121-3123 above is from 63af688. The rebase onto a newer main shifted this entry down by 82 lines without changing a byte of it. The sentence this is about — the hiddenInterfaceModes on columns/groups rejection, "a schema and filter change to a shared model for one consumer" — now begins at :3204, on the - **Alternatives:** line inside adr-menu-per-mode-layout-via-mode-gated-columns.
The ask is unchanged: name the filter half explicitly so the distinction from isHeaderHidden carries.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 made 16 comments.
Reviewable status: 0 of 96 files reviewed, 16 unresolved discussions (waiting on irahopkinson+AI).
a discussion (no related file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
A third review pass, scoped to the five commits added since the last round —
c4769a6..63af688, 12 source files and 195 insertions. The earlier commits were deliberately not re-reviewed; bothdist/bundles were skipped and their sources read instead.15 inline comments follow — two high, six medium, the rest low. Each carries its severity and a short status: checked and confirmed means it was verified against the code, needs a human call means it could not be settled either way and the judgement is yours.
Several of these exist because you did what the last round asked, and they read better with that context:
- The Quality checks section is here because of round 1's finding about Simple losing every quality tool. The direction is right; the notes on it are about the record and the label.
- The Tools-order comment follows from the mapping fix — adding
TAB_FOR_COMMANDwas right, and it is the set comparison that came with it that dropped the order assertion.- The "rule is now in three places" note is the direct sum of two earlier findings, each of which asked for a local copy. I have said so in that comment rather than presenting it as a new defect.
Two things to read first. The Tools-order comment on
shipped-simple-layout-order.test.ts:293is the only one marked blocking, and it also carries a single ticket-record ask covering both of this PR's 2026-09-23 reversals, so the Quality-checks comment points at it rather than repeating it. The Open Checks comment onmenus.json:368is the one I would most like a second opinion on: the reasoning is a static trace through rc-dock, and seeing the split in a running app would settle it in a minute.A correction to something an earlier round of mine got wrong, since it is load-bearing for the zoom comments: I initially read the gating of the zoom items as leaving Simple with no zoom at all. That is not right. Ctrl+wheel and Ctrl+=/−/0 act on the editor in every mode through a path this PR does not touch, and the keyboard route into the tab menu appears intact too. What the gating removes is one mouse route, and the recorded justification names a substitute that does not work by mouse for the editor's column. That comment is low, not high, and it is about the wording rather than the behaviour.
One finding in a file this pull request does not change
low · checked and confirmed
lib/platform-bible-react/src/stories/advanced/tab-navigation/tab-dropdown-menu.stories.tsx:18Storybook still tells a component consumer that every section is labeled when there are two or more, and offers no page where the new headingless section can be seen.
What happens: the component-level description states the heading rule without the new exception, which the component's own TSDoc now carries (
tab-dropdown-menu.component.tsx:168-169). The sibling rendering variation has a story —SingleSectionat:272-297documents "no heading and no divider" — butisHeaderHiddenhas none.Why it matters: extension authors read this page to decide how to shape their columns, and the one rendering mode whose accessibility is in question has no rendered example. Worth being precise about the stakes: the repo's Storybook a11y checks run as
a11y: { test: 'todo' }(.storybook/preview.ts:38-41), so they surface violations rather than failing CI. Adding a story would make this mode visible to those checks, not enforced by them — right now neither happens.Fix: amend the description to name the
isHeaderHiddenexception, and add a story besideSingleSectionrendering a column with the flag set, tagged['test']like its neighbours, whose description states that the section keeps its divider and its accessible name.
The pull request description is describing the previous state
The
review-paratextsummary between the<!-- review-paratext:summary -->markers still describes the branch as it stood before these five commits. Six points are now wrong at63af688:
- The sections row reads "Project · View · Insert · Tools" — it omits the new unheaded Edit section and Quality checks.
- "Project | Open Project Settings… · Edit ▸" — Edit ▸ moved out of Project into its own column (
menus.json:12-16,:251-257).- The Tools row lists Comments third; the served order now has it last.
- "Tools now mirrors the third column's tabs, in the order they appear there" — no longer true, and the subject of the blocking comment.
- "The inventories, Markers Checklist and Check Results are hidden in Simple" — Open Checks is now shown in Simple.
- "Diff size: 31 source files. The other 13 are the regenerated
platform-bible-react/distbundle" — the pull request is now 58 files: 39 source, plus 14 inplatform-bible-react/distand 5 inplatform-bible-utils/dist.
isHeaderHiddenis not mentioned at all, and it is the most consequential thing in this delta — a new field on the public contribution schema. The "Why Simple gets its own columns" panel also still says that teaching the shared menu model per-mode columns "is a platform change for one consumer, so it was rejected for now", which now sits beside an accepted platform change to that same shared model for that same consumer. The inline note onArchitecture-Decisions.md:3122covers why those two are in fact different; the description would read better if it drew the same distinction.The description is yours to regenerate — flagging it because it is the first thing a reviewer reads.
Thanks for your help with these reviews, Ira! Both of the reversals are now settled and on the ticket. PT-4534 has a new "Decisions (2026-09-23)" section, and its Definition of Done and Testing Idea bullets are amended to match. Tools goes back to the third-column tab order (1.6f). The Quality checks change stands, with Open Checks in Simple, because product asked for it.
The Storybook finding (outside the diff): done. The component description now names the `isHeaderHidden` exception. There is also a new `HiddenSectionHeading` story after `SingleSection`, tagged `['test']`. Its description says the section keeps its divider, and keeps its screen-reader name while two or more sections are shown.
The PR description: rewritten for the current branch. It now covers the unheaded Edit section, Quality checks, `isHeaderHidden`, and the current file counts. The "Why Simple gets its own columns" panel now explains why `isHeaderHidden` is different from the per-mode columns change we rejected.
.context/standards/Architecture-Decisions.md line 3115 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
Line-number correction, since the rebase moved everything this comment points at.
The numbers above are from
63af688. The branch has since been rebased onto a newer main, which inserted upstream entries ahead of this one: the entry moved from line 3098 to 3180, and the file grew from 7521 to 8020 lines. The entry itself did not change — I diffed it against the old head and it is byte-identical — so all three asks stand exactly as written.If you are reading against the current revision, anchor on the text rather than the numbers. Inside
adr-menu-per-mode-layout-via-mode-gated-columns:
what was now - **Date:** 2026-09-18:3100:3182"The design puts Edit ▸ in a section of its own with no heading" :3115:3197"Its only quality tool is Open Checks, under a Quality checks section, as product asked on 2026-09-23" :3137-3139:3220-3221the unindented block — the Per-pane zoom (...) is Power-only for a structural reasonparagraph:3142-3147:3224-3229- **Source:** PT-4534 (parent PT-4530):3151:3233The indentation point is still live at the new numbers:
:3224and:3229both start at column 0 while the lines around them carry the two-space continuation indent.My fault for citing line numbers in a file that is append-only and gets rebased often — the slug plus the sentence would have survived this. Sorry for the chase.
## Architecture-Decisions.md line 3115: amended in place
I restored the 2026-09-18 Consequences, and added an "Amended 2026-09-23 (PT-4534, review of #2847)" note covering three things:
- **The Edit ▸ section, with its source.** In the v0 demo's code, the Edit submenu sits outside the Project group, between two dividers and with no heading. The ticket's 2026-09-18 transcription had listed it inside Project.
- **The Quality checks reversal**, citing PT-4734 for the Checking assistant.
- **The zoom gating.**
The Source line now carries both dates. The six mis-indented lines were in the zoom paragraph, which has moved into the amendment with normal indentation. With Tools back on the tab order, the restored sentence about it is true again.
.context/standards/Architecture-Decisions.md line 3122 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
Line-number correction, same cause as on the sibling thread.
The
:3121-3123above is from63af688. The rebase onto a newer main shifted this entry down by 82 lines without changing a byte of it. The sentence this is about — thehiddenInterfaceModeson columns/groups rejection, "a schema and filter change to a shared model for one consumer" — now begins at:3204, on the- **Alternatives:**line insideadr-menu-per-mode-layout-via-mode-gated-columns.The ask is unchanged: name the filter half explicitly so the distinction from
isHeaderHiddencarries.
Reworded as you suggested. It now reads "a schema change plus a change to the shared mode-filtering pipeline every menu consumer runs, for one consumer's need", and points at `isHeaderHidden` in the amendment as render-only and ignored by the menubar.
.context/standards/Entry-Point-Guide.md line 149 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
low · checked and confirmed
This states the accessible name unconditionally, and there is a live case where it does not hold.
What happens:
tab-dropdown-menu.component.tsx:186computesshowHeadings = showSectionHeadings && sections.length > 1, and:234setsaria-label={showHeadings && isHeaderHidden ? label : undefined}. So the label names the section only while two or more sections have items.The
showSectionHeadingshalf is largely theoretical — both bundled consumers hard-code it true (tab-toolbar.component.tsx:78,:116;tab-floating-menu.component.tsx:26). Thesections.length > 1half is not: a column withisHeaderHidden: trueleft as the only populated section gets no heading and no accessible name at all, and interface-mode filtering emptying its siblings is exactly the mechanism this guide documents two bullets up. The component's own props doc already carries the caveat — "Only takes effect when two or more sections have items" (tab-dropdown-menu.component.tsx:146-147) — and this bullet drops it when restating the feature. It is untested too:tab-dropdown-menu.component.test.tsx:196-205covers the single-section case but never pairs it withisHeaderHidden.Why it matters:
.claude/rules/docs-durability.mdasks that a behaviour stated in a living doc be checked against the live repo. An author reading this sets the flag expecting a name and, in the one configuration where the section stands alone, gets neither heading nor name.Fix: qualify the sentence — the label becomes the group's accessible name in a menu that heads its sections, and only while two or more sections are shown; a lone remaining section, including one left alone by interface-mode filtering, gets no heading and no name. Give it a real label regardless. The same qualification belongs on the TSDoc at
menus.model.ts:60-61, which makes the promise in the same unconditional form.
## Entry-Point-Guide.md line 149: the accessible name is conditional
Qualified as you suggested. The bullet now says the label names the section only while two or more sections are shown, and a section left on its own (including one left alone by interface-mode filtering) gets neither a heading nor a name. The same qualification is in the `menus.model.ts` TSDoc and in the JSON-schema description.
e2e-tests/tests/markers-checklist/wiring-theme-5.spec.ts line 41 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
medium · checked and confirmed
This gate's stated reason is no longer true, and it was this PR that made it untrue.
What happens: the comment says Simple "hides both items". Commit
63af688servesplatformScripture.openChecksSidePanelin Simple (menus.json:362-369, pinned atmenu-data.service-host.scripture-editor-menu.test.ts:202), andopenChecksSidePanelis absent fromPOWER_ONLY_COMMANDS. Test 8 at:577-596clicks the Project button and picks/Open Checks/i— a flow that now works in Simple too. The Markers Checklist half is still correct:platformScripture.openMarkersChecklistremains inPOWER_ONLY_COMMANDSat:254, so therequiredInterfaceMode: 'power'line below should stay.Timeline, for fairness: the comment came in at
7fe924bdfc2("Regroup Simple's Project menu to the v0 structure", 2026-09-23 16:05), and63af688invalidated it at 16:09 — four minutes later, in the same PR. So this is not a stale assumption inherited from elsewhere; it is one commit in this PR overtaking another. Nothing wrong with that happening, it just needs the comment brought along.Why it matters: this comment is the only thing explaining why the file is Power-gated. A future reader either believes Open Checks is Power-only, or discovers it is not and deletes the gate — at which point the Markers Checklist tests fail in Simple.
Fix: reword the comment to attribute the gate to Markers Checklist alone, and note that Open Checks is reachable in both modes. The
test.useline itself does not need to change.
Reworded. The comment now puts the Power gate on Markers Checklist alone and says Open Checks is reachable in both modes. The `test.use` line is unchanged.
extensions/src/platform-scripture-editor/contributions/menus.json line 13 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
low · needs a human call
In the one place
isHeaderHiddenships, the section and its only item have the same name.What happens: this column's label is
%webView_platformScriptureEditor_edit%, whichTabDropdownMenuturns into the group'saria-label. The column holds exactly one group (platformScriptureEditor.simpleEditMenu,:93-95) with exactly one item, the submenu triggerplatformScriptureEditor.editSubmenu(:250-256), whose label is the same key. Both resolve to "Edit" in English and "Editar" in Spanish (localizedStrings.json:91,:309). A screen-reader user entering the section hears "Edit", then "Edit".Why it matters: less as a harm than as a signal. This is the only production use of
isHeaderHiddenin the tree, so the accessible name that justifies the wholearia-labelbranch — and the untested guard flagged ontab-dropdown-menu.component.tsx:234— buys nothing in the one place it is exercised. A sighted user just sees the flyout row. That is worth a moment's thought about whether the flag is the right tool here, rather than only a wording fix.Fix: this is a content decision rather than a mechanical one, so it needs your judgement or UX's. Either give the column its own label describing the section rather than repeating its item — a new key needs
enandesentries in alphabetical position — or keep the duplication deliberately and note it in the ADR entry, so the next reader does not mistake the group name for information.
## menus.json line 13: the Edit section and its only item share a name
I kept the duplication on purpose, and the ADR amendment records it. A column has to have a real label, and this section holds nothing but the Edit flyout, so any other label would just say "Edit" in different words. v0 gives it no name at all, because its Edit submenu sits outside every group. Since the `tab-dropdown-menu.component.tsx:234` change, the name also comes through `aria-labelledby`, the same way every headed section is named.
extensions/src/platform-scripture-editor/contributions/menus.json line 363 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
medium · checked and confirmed
Two things about this new section: it is neither outcome the Definition of Done allows, and its label is not the one the ticket names.
Quality checks is shown in Simple. The new
simpleQualityCheckscolumn (menus.json:46-49) and this Simple-only item put a visible section into Simple, pinned atmenu-data.service-host.scripture-editor-menu.test.ts:202. The DoD says "Quality checks is hidden, or deliberately omitted with the reason recorded", and the ticket's Testing Idea says "Assert the Quality-checks section is absent in Simple". Neither holds now. The reversal is recorded with a date in the ADR — "as product asked on 2026-09-23" (Architecture-Decisions.md:3139) — which is the right instinct. What is missing is the ticket-level record: PT-4534 has its own numbered "Decisions (2026-09-18)" section, and this second round did not extend it, so anyone verifying the branch against the ticket reads this as a defect. The ask covering this and the Tools-order reversal is onshipped-simple-layout-order.test.ts:293, so I am not repeating it here.Worth saying plainly: this section exists because of round 1's finding about Simple losing every quality tool. The direction is right — it is the paperwork and the label that need to catch up.
The label is not the one the ticket specifies. This item reuses the Power key
%webView_platformScriptureEditor_openChecks%→ "Open Checks..." / "Abrir verificaciones...". Every sibling in Simple's menu is a sentence-case noun with no verb and no ellipsis — "Bible texts", "Commentaries", "Text collection", "Find", "Comments" — and under a "Quality checks" heading the verb and object also restate the heading just above them. PT-4534's target structure names this row "Checking assistant", so the answer is already on the ticket: it needs neither "Open Checks…" nor an invented short form. Since the DoD requires Simple to match v0's "sections, order and labels", this is a DoD item rather than a style preference.Fix: mint a new key rather than editing the existing one — it is immutable under
Localization-Guide.md:400-402and is shared with the Power item atmenus.json:219. Addenandesvalues in alphabetical position incontributions/localizedStrings.jsonand point this item'slabelat it.menuLocalizeKeysinlocalized-strings.test.tsreads labels straight offmenus.json, so it picks the new key up with no test change. The Spanish needs a real translation rather than a transliteration.
## menus.json line 363: Quality checks vs the Definition of Done, and the label
The ticket now records it. "Decisions (2026-09-23)" says product asked for Open Checks in Simple. The "Quality checks is omitted" deviation is marked superseded, and the DoD and Testing Idea bullets are amended.
On the label, we're deliberately not using "Checking assistant" yet. That row names the assistant, and bringing the assistant into Simple is PT-4734 ((Simple) Checking Assistant full integration). Until then the item opens the regular Checks panel, so it keeps the Power label instead of naming a tool it doesn't open. The ADR amendment and the ticket both say this.
extensions/src/platform-scripture-editor/contributions/menus.json line 368 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
high · checked and confirmed
In Simple mode, choosing Quality checks ▸ Open Checks… splits the editor's column into two panes, so the workspace stops matching Simple's fixed three-column layout — and it does this again on every click.
What happens: this item runs
platformScripture.openChecksSidePanel, whose handler (extensions/src/platform-scripture/src/main.ts:158-190) callsopenWebView(checksSidePanelWebViewType, { type: 'panel', direction: 'right', targetTabId }, options)with no interface-mode branch. The renderer'spanelbranch resolves the editor tab's parent and callsdockLayout.dockMove(tab, targetTab.parent, 'right')(platform-dock-layout-storage.util.ts:1113-1125). rc-dock'sdockPanelToPaneltakes Column 2's own wrapper box, and because the requested split is horizontal where the wrapper is vertical, builds a new horizontal child box and substitutes it inside — sodockbox.children.lengthstays 3, but the user sees four resizable panes. Nothing reverts it mid-session:saveLayoutno-ops in Simple andloadLayoutre-applies the static layout only on startup or a mode switch.It compounds.
optionsnever setsexistingId, andopenWebViewguards its whole reuse branch onif (optionsDefaulted.existingId)(src/renderer/services/web-view.service-shard.ts:3381), with defaults injected only when one is already present (:2511). So no reuse lookup happens and every invocation creates another web view and another split — a fifth pane, a sixth, and so on. The reuse idiom is well established in this very file:REUSE_EXISTING_FIND_ONLYatmain.ts:390(used at:417,:497,:543) and Manage Books'existingId: '?'at:344.openFindeven carries the comment "Ignored in Simple mode, where the fixed layout already holds a Find tab for the probe below to find". Open Checks is the one panel-opener without a probe.Why it matters: it is reachable by an ordinary menu click with no unusual setup, and Simple's fixed, non-restructurable layout is the point of the mode.
checksSidePanelis absent fromFIXED_LAYOUT_WEBVIEW_GROUPS(platform-dock-layout-positioning.util.ts:91-99), so nothing pins it into Column 3, and because its group hangs offsimpleQualityChecksrather thansimpleTools,TAB_FOR_COMMANDinshipped-simple-layout-order.test.ts:136does not cover it — that suite passes 11/11 while exercising none of this.Fix: four parts, and the first is the one that matters most.
- Give
openChecksSidePanela reuse-first probe mirroringREUSE_EXISTING_FIND_ONLY. This alone stops the stacking.- Give Checks a static presence in Column 3 for Simple — either in
simple-layout.data.tsor viadefault-layout-supplement.jsonbehind a flag, following thescriptureTextGridprecedent. Without this the probe finds nothing on first use and still falls through to a placement decision.- Add
'platformScripture.checksSidePanel'toFIXED_LAYOUT_WEBVIEW_GROUPSwithTAB_GROUP_RESOURCES.- Extend
TAB_FOR_COMMANDso the layout suite covers the Quality checks group — which only works once (2) exists.Swapping the layout literal alone would not match the established pattern and would leave the compounding in place even if the first click landed correctly. Worth one run in the app to see the split, since the exact rendering was reasoned from the rc-dock algorithm rather than observed.
## menus.json line 368: Open Checks splits the editor's column
Agreed, fixed. Your first and third parts are in, and the second became an on-demand tab instead of a static one, so Simple's starting layout doesn't change:
- In Simple, `openChecksSidePanel` first checks for an existing Checks tab (`existingId: '?'`, `createNewIfNotFound: false`) and brings it to the front. If there isn't one, it opens the panel as a tab in Column 3 with `{ type: 'tab', parentTabGroupId: 'simple-panel-resources' }`. The extension can't import renderer source, so it keeps its own copy of that panel id, and a check in `simple-layout.data.test.ts` fails if the copy drifts. Power still opens the panel to the right of the editor.
- A reused tab showing a different project or editor gets reloaded with the current options.
- The tab is pinned like the other Column 3 tabs (`isClosable` is false in Simple), and `platformScripture.checksSidePanel` is in `FIXED_LAYOUT_WEBVIEW_GROUPS` under `TAB_GROUP_RESOURCES`.
- An open Checks tab now follows a project switch, the way Find does. The editor calls a new `platformScripture.updateChecksSidePanelProject` right after re-pointing Find, and only in Simple for an editable project. That call creates nothing if no Checks tab is open, and it reloads with `bringToFront: false` so a switch doesn't pull Column 3 away from the user's tab.
- Part 4 doesn't apply now. `TAB_FOR_COMMAND` covers the tabs in the static layout plus the supplement, and Checks is in neither.
Recorded as `adr-simple-column-3-tools-open-on-demand`, since it's the first time an extension adds a tab to Column 3 while the app runs.
Tests: `open-checks-side-panel.utils.test.ts` pins Simple checking for an existing tab first, never using `type: 'panel'`, reusing the tab, and reloading on a project or editor mismatch. It also pins the project-switch update and Power's right-of-editor panel. `checks-side-panel.web-view-provider.test.ts` pins whether the tab can be closed in each mode.
I haven't seen it in the running app yet. Thanks for flagging that the split was reasoned rather than observed; I'll check the first click, a later click and a project switch before this merges.
extensions/src/platform-scripture-editor/src/contributions-zoom-menu.test.ts line 56 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
low · checked and confirmed
The closing sentence here is imprecise about the editor specifically, and the same paragraph exists in three places — so the imprecision had to be written three times.
On the sentence. "Simple still reaches zoom from the tab menu, which core's
defaultWebViewTabMenuserves in every mode" is true for Column 3's tabs and misleading for the editor. The tab menu's only trigger is a context menu on the tab title (platform-tab-title.component.tsx:1086-1095), and in Simple the editor sits inHEADLESS_GROUP(platform-dock-layout-positioning.util.ts:100-102) whose dock bar ispointer-events: none, withopacity: 0held even on:focus-within(dock-layout-wrapper.simple-mode.scss:56-65). So for this column there is no tab title to right-click.To be clear about what is and is not lost, because the first version of this note overstated it: no zoom capability goes away. Ctrl+wheel and Ctrl+=/−/0 act on the editor in every mode via
web-view-content-zoom.chrome-keys.ts:84-114, registered unconditionally atrenderer/index.tsx:158and gated only on input-blocking, never on interface mode — neither file is touched by this PR. The keyboard route into the tab menu itself also appears intact:.dock-tab-btncarriestabIndex: 0from rc-tabs,simpleConfigsets onlytabLocked, nothing setsinertoraria-hidden, andplatform-tab-title.component.tsx:1023-1053forwardscontextmenuinto the trigger — covered byplatform-tab-title.zoom-menu.test.tsx:434-471("keyboard access in Simple mode"). What commit947a194did remove is a working mouse route: before it, the three zoom items were ungated and appeared in Simple's Project menu. Gating them is the right call; the recorded reason just names a substitute that does not work by mouse for this column.On the triplication. The same chain — every Options item is Power-only → a column ships if any item survives → one ungated item resurrects the column → Simple gets zoom from the tab menu — is written out here, at
menu-data.service-host.scripture-editor-menu.test.ts:262-266, and atArchitecture-Decisions.md:3141-3147, in near-identical prose. That one wrong sentence appearing in all three is the argument for not keeping three copies: when the Options column's composition changes, whichever copies are not edited become confident, wrong guidance.Fix: correct the closing sentence in all three places to name the routes that actually work in Simple — the Ctrl+wheel and Ctrl+=/−/0 chords, with the tab menu noted as keyboard-only for this column. Then thin the duplication: keep the full reasoning here, where it sits next to the assertion it explains, and shorten the
menu-data.service-hostcopy to a one-line pointer at this test. I would not point either test at the ADR — sending a reader of a failing test into a 7500-line standards file is a worse trade than the duplication, and the sibling comments in that same array are all self-contained.Please don't add a Simple-owned zoom column to solve this. It would duplicate a capability that already works, and this delta is already adding four columns.
## contributions-zoom-menu.test.ts line 56: the closing sentence and the three copies
Corrected and thinned out. The full reasoning stays in this test, and now names the routes that work in Simple: Ctrl/⌘+wheel and Ctrl/⌘+`+`/`-`/`0`. It also notes that the tab menu reaches the editor only from the keyboard, because the editor's tab title is hidden. The `menu-data.service-host` copy is now a one-line pointer to this test. The ADR amendment has the corrected sentence. No Simple zoom column.
extensions/src/platform-scripture-editor/src/contributions-zoom-menu.test.ts line 59 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
low · checked and confirmed
The name credits an assertion that lives in a different test.
What happens: the body reads
itemsand asserts, for each zoom item, itshiddenInterfaceModesand the absence ofisExperimental. So "hides every zoom item in Simple" is covered, and so is the "rather than the items" half —expect('isExperimental' in item).toBe(false)is exactly that. What is not here is the "marks experimental on the group" half:expect(zoomGroup.isExperimental).toBe(true)lives in the separate test at:29-37.Why it matters: a green run reads as evidence for a group-level claim this case does not check, and a failure report names an assertion that is not in the body. Minor, but the earlier name had the same shape, so the rename carried it forward rather than fixing it.
Fix: name only what it asserts — e.g.
'hides every zoom item in Simple and leaves isExperimental off the items'— and let the test at:29keep the group claim.
Renamed it to 'hides every zoom item in Simple and leaves isExperimental off the items'.
lib/platform-bible-react/src/components/advanced/menus/menu.util.ts line 92 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
low · checked and confirmed
"see
isHeaderHidden" points at the field it is documenting, so the pointer resolves to itself.What happens: the referent you mean is
MenuColumnWithHeader.isHeaderHiddeninplatform-bible-utils(menus.model.ts:58-64), which is where the semantics, the menubar carve-out and the accessibility promise are written down. Unqualified, the name resolves to the member it annotates — this one, set from the column's flag atmenu.util.ts:113.Why it matters:
MenuSectionis the shape every future renderer of menu sections destructures, and this sentence is the only documentation the field gets on this side. It is internal — neitherMenuSectionnorgetMenuSectionsWithItemsis re-exported fromlib/platform-bible-react/src/index.ts— so no extension author hits it, which keeps this minor.Fix: name the source of truth.
MenuColumnWithHeaderis already imported atmenu.util.ts:5, so a TSDoc link resolves:
/** Whether the section is shown without its label as a heading; see {at-link MenuColumnWithHeader.isHeaderHidden} */I have written that as
{at-link}on purpose — the real@spelling inside a review comment is parsed as a mention here, which creates a phantom participant and blocks the review from publishing. Use the proper@form in the source file. If you would rather avoid the link entirely, plain prose does the job too: "mirrorsMenuColumnWithHeader'sisHeaderHiddenfield inplatform-bible-utils".
Now `{@link MenuColumnWithHeader.isHeaderHidden}`.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 234 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
medium · needs a human call
A section now gets its accessible name from one of two different mechanisms depending on a flag, and the unheaded branch is both untested and unprecedented in this repo. I think one mechanism would be better than two, but the deciding question needs a screen reader, so this is your call rather than mine.
What happens: a headed section is named by
aria-labelledbypointing at a realDropdownMenuLabelchild. The unheaded one instead setsaria-labelon the bare<div role="group">thatDropdownMenuGrouprenders (shadcn-ui/dropdown-menu.tsx:172-174→ RadixMenuGroup, which emitsrole="group"literally). The attribute does land — that part is verified.The guard is untested, and two obvious rewrites survive the suite. Running the full 25-test suite against a mutated copy: replacing the
aria-labelexpression witharia-label={label}passes 25/25, and witharia-label={isHeaderHidden ? label : undefined}also passes 25/25. The new test attab-dropdown-menu.component.test.tsx:178-194only ever opens withshowSectionHeadings=true, and the no-headings test at:207uses a fixture with noisHeaderHiddencolumn, so the combination the guard exists for is never rendered.aria-label={label}is precisely the "simplification" a later reader makes; it would name every group in the deliberately-unnamed mode and stack a redundant name underaria-labelledbyon the headed ones.The open question. Whether a name on
role="group"nested inrole="menu"is actually announced varies by screen reader. There is no convention here to lean on: this is the onlyaria-labelon aDropdownMenuGroupanywhere in the repo, and there is no accessibility page understories/guidelines/. If it is not conveyed, the flag's accessibility half is decoration and the user who most needs the cue gets an anonymous run of items between two named ones.Suggested direction — keep one mechanism. Always render the
DropdownMenuLabel id={headingId}and always setaria-labelledby, addingclassName="tw:sr-only"whenisHeaderHidden. That moves naming onto a mechanism this codebase already uses for exactly this purpose (conflict-note-card.component.tsx:210-213, with a comment documenting the visible-versus-announced split; alsodata-table-pagination.component.tsx:56), and it deletes the untested branch above rather than asking you to write a test for it.I checked the obvious objection — that an always-present label node would disturb the menu — and it does not hold: only
MenuItemis registered in Radix'sCollection.ItemSlotthat backs roving focus and typeahead (@radix-ui/react-menuMenuItem, wrappingMenuItemImpl);MenuLabelis a barePrimitive.divthat was never part of that machinery, visible or not.Fix: either adopt the single-mechanism change above and add one test asserting the hidden-header section's accessible name comes through in both
showSectionHeadingsstates — or, if you prefer to keeparia-label, record a check against at least one screen reader (NVDA or VoiceOver) confirming the group name is spoken on entry, and add a case pairingisHeaderHidden: truewithshowSectionHeadings=falseasserting no group is named. That single case kills both mutants above; I verified it does.
## tab-dropdown-menu.component.tsx line 234: one naming mechanism
I went with the single mechanism. In a menu that shows headings, every section now renders its `DropdownMenuLabel` and is named by `aria-labelledby`. An `isHeaderHidden` column's label just gets `tw:sr-only`, and the `aria-label` branch is gone.
Tests: the existing `isHeaderHidden` case asserts the group is still named "Edit" and its label carries `tw:sr-only`, while the neighbouring headings don't. Two new cases pair `isHeaderHidden` with `showSectionHeadings=false`, and with the column left as the only section; both assert that no group is named. Making the label render unconditionally turns both new cases red.
lib/platform-bible-utils/src/extension-contributions/menus.model.ts line 60 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
medium · checked and confirmed
The one example this description gives is the one menu where the flag cannot be set.
What happens: the text — in both this TSDoc and the JSON-schema
descriptionat:333-337, which must stay aligned — says the flag applies "in a menu that heads each section with its column's header, such as a web view's tab menu". But in this same model a tab menu istabMenu/defaultWebViewTabMenu, both$ref: '#/$defs/singleColumnMenu'(:556-559,:270), andSingleColumnMenuis{ groups, items }(:155-160) — nocolumnsfield at all. SinceisHeaderHiddenlives onMenuColumnWithHeaderand is only reachable throughcolumns, it cannot be expressed on a tab menu; the schema would reject the key outright. The menu that actually heads its sections with column labels istopMenu(:548-551), which is whatTabDropdownMenurenders — it takesLocalized<MultiColumnMenu>attab-dropdown-menu.component.tsx:134.Why it matters: this is the published contribution surface, and this description is what an extension author sees in editor IntelliSense while writing
menus.json. Following the example literally leaves them nowhere to put the flag. The confusion is sharpened by this file's own vocabulary: "tab menu" is used at:178,:194and:232to mean the single-columntabMenufield specifically, in explicit contrast totopMenu. The feature's own origin entry inArchitecture-Decisions.mdnever calls it "the tab menu" either — it names the component and the Project menu.I can see how the wording happened: the component is called
TabDropdownMenuand the menu does open from a tab, so "tab menu" is natural shorthand. It just collides with a field of that exact name 140 lines below.Fix: in both places, use the phrase this file already uses for
topMenuat:183— "the menu that opens when you click on the top left corner of a tab (topMenu)" — rather than introducing a second sense of "tab menu". Then rebuildlib/platform-bible-utils/distso the shippedindex.d.tscarries the corrected text.Unrelated but worth recording since it is easy to wonder about:
lib/papi-dts/papi.d.tscorrectly needs no regeneration here. It reaches this model only by importing fromplatform-bible-utilsand never inlines it — noMenuColumnWithHeader, no ambient re-declaration — andplatform-bible-utils/dist/index.d.ts:1705already carries the new field from this PR.
## menus.model.ts line 60: "such as a web view's tab menu"
Both the TSDoc and the schema description now use the file's own wording for `topMenu` ("the menu that opens when you click on the top left corner of a tab"), plus the two-or-more-sections qualification. The platform-bible-utils dist is rebuilt.
src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts line 206 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
low · checked and confirmed
The name promises rendering coverage this layer cannot provide, and the second half is never asserted.
What happens: the body reads
simpleMenu.columnsand asserts which columns carry a truthyisHeaderHidden. Nothing renders, so "is shown without a heading" is not checked here — that behaviour is pinned separately attab-dropdown-menu.component.test.tsx:178-192. And the filter runs over every declared column in the served document, including ones with no items in Simple (platformScriptureEditor.options,.edit,.tools,.info), which are not sections at all — so "every other section has one" is not checked either, in either direction.Why it matters: a reader takes the heading behaviour as pinned at the served-menu level when it is not. Someone deleting or rewriting the component test later would believe this still covers it.
Fix: rename it to what it pins — something like
'the served Simple menu marks only the Edit column isHeaderHidden'— and leave the rendering claim to the component test. If you would rather the name stay as it is, the alternative is to narrowheaderHiddenColumnsto the columns that actually have items in Simple and assert the complement explicitly.
## menu-data.service-host.scripture-editor-menu.test.ts line 206: the test name
Renamed it to 'the served Simple menu marks only the Edit column isHeaderHidden'. The component test keeps the rendering claim.
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 123 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
medium · checked and confirmed
The group-under-column rule now exists in three places, and this copy is the third.
What happens:
menu-data.service-host.scripture-editor-menu.test.ts:62-69already held this function and docblock — it was there at the review base — and this commit adds a byte-identical copy here. Both restateisGroupUnderColumnOrSubMenu(lib/platform-bible-react/src/components/advanced/menus/menu.util.ts:76-84), and both carry a comment instructing the reader to keep them in sync by hand.I should own where this came from. The two copies exist because two earlier findings in this review asked for them — one on the served-menu snapshot and one on this file — each proposing a local restatement, and each noting the symbol is not reachable from
platform-bible-react's public entry point. That was accurate as far as it went:isGroupUnderColumnOrSubMenuis exported from its own module, just not re-exported fromlib/platform-bible-react/src/index.ts. What neither finding anticipated is that satisfying both would put the same nine lines in two test files plus production. The consolidation those fixes deferred is now worth doing.Why it matters: the rule has to change in three files at once, and the copies' own comments admit it. That is the sync burden made explicit rather than removed.
Fix: export
isGroupUnderColumnOrSubMenufromlib/platform-bible-react/src/index.tsand import it in both tests, which removes the restatement entirely and makes drift impossible. If widening the public surface is unwelcome for a test-only need, the alternative is one shared helper module both tests import, with the docblock in that one place.On the second branch specifically: no group in either shipped menu document is keyed the same as a column, so
groupKey === columnKeynever fires when the helper is called with a column key — I re-checked that this round. That is not a defect to fix. The earlier finding said the same thing when it asked for the branch ("a latent divergence rather than an active blind spot"), and the branch is live in production viatab-dropdown-menu.component.tsx:56, where the key can be a submenu key. Keeping it is right; it is only the duplication that is worth removing.
## shipped-simple-layout-order.test.ts line 123: the rule in three places
Took your first option. `isGroupUnderColumnOrSubMenu` is now exported from platform-bible-react's `index.ts`, and both tests import it, so both copies of the rule are gone. One side effect: its TSDoc linked to `getMenuSectionsWithItems`, which isn't exported, and typedoc fails the build on that, so the link is now plain code text.
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 293 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
high · checked and confirmed
Nothing in the repo now compares Simple's TOOLS order to the third-column tab order. The two survive only as independent hand-written literals that are deliberately different from each other.
What happens: this line changed from
expect(mappedTabs).toEqual(columnWebViewTypes(merged, 2))to anew Set(...)comparison, so it checks membership only. The served TOOLS order is now a hard-coded array atmenu-data.service-host.scripture-editor-menu.test.ts:192-201— Bible texts · Commentaries · Text collection · Find · Comments — and the tab order a separate hard-coded array atshipped-simple-layout-order.test.ts:269-277— Bible texts · Commentaries · Comments · Text collection · Find. They differ only in where Comments sits. A reorder of either still fails its own literal, so nothing is silent; what is gone is the derivation.Why it matters: this is not a stale Definition-of-Done line that the code has outgrown. PT-4534's "As shipped (2026-09-21)" section records that Tools' order was pinned to the tab order, explicitly "because 1.6f says the menu mirrors the UI" — and that this was itself a deliberate deviation from the v0 screenshot, made so that DoD line could be met. The ADR sentence this PR replaces said Simple's Tools order "is pinned to the third-column tab order"; the new text says it "follows the design … not the third-column tab order". So a previously satisfied requirement is being reversed, not clarified. The WI-9 Dictionary case the Testing Idea names is still caught (set equality fails when a tab has no item), but a tab and an item both added in mismatched relative order now passes.
One root cause, noted once here. This PR encodes two reversals of previously settled PT-4534 decisions, both re-decided around 2026-09-23: Tools' order (this one) and Quality checks (omitted in Simple, now shown — see the note on
menus.json:363). Only the Quality-checks reversal carries a citation anywhere in the diff — "as product asked on 2026-09-23",Architecture-Decisions.md:3139. This one carries none: not in the ADR, not in the commit message, not on the ticket. PT-4534 already has a numbered "Decisions (2026-09-18)" section; adding one "Decisions (2026-09-23)" section covering both reversals, and amending the DoD and Testing-Idea bullets to match, closes both gaps at once.Fix: settle which reading of 1.6f governs and record it. If 1.6f still holds, restore the order-preserving assertion and put Comments back where the tab order has it. If product genuinely prefers the v0 static order, that is a fine answer — but then the DoD line and the Testing Idea need amending on the ticket, because as written no test can satisfy them. Either way, if the set comparison stays, rename the test away from "order" language and say in its doc comment that the menu order is pinned elsewhere by literal rather than derived.
## shipped-simple-layout-order.test.ts line 293: the Tools order (blocking)
Settled on 1.6f. Comments is back to third in Simple's Tools, matching the tab order, and the order-preserving `toEqual` assertion is restored in place of the set comparison. The ADR's Consequences say again that Tools is pinned to the third-column tab order. The ticket's "Decisions (2026-09-23)" section records that it stays that way, alongside the Quality checks reversal.
617c0b6 to
be80b73
Compare
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 16 comments and resolved 15 discussions.
Reviewable status: 0 of 96 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).
a discussion (no related file):
Previously, katherinejensen00 wrote…
Thanks for your help with these reviews, Ira! Both of the reversals are now settled and on the ticket. PT-4534 has a new "Decisions (2026-09-23)" section, and its Definition of Done and Testing Idea bullets are amended to match. Tools goes back to the third-column tab order (1.6f). The Quality checks change stands, with Open Checks in Simple, because product asked for it.
The Storybook finding (outside the diff): done. The component description now names the `isHeaderHidden` exception. There is also a new `HiddenSectionHeading` story after `SingleSection`, tagged `['test']`. Its description says the section keeps its divider, and keeps its screen-reader name while two or more sections are shown.
The PR description: rewritten for the current branch. It now covers the unheaded Edit section, Quality checks, `isHeaderHidden`, and the current file counts. The "Why Simple gets its own columns" panel now explains why `isHeaderHidden` is different from the per-mode columns change we rejected.
All sixteen checked against the code rather than the replies. Fifteen fixed, one declined with a better argument than mine (the Checking assistant label — I had reasoned from the ticket's wording without asking what the item actually opens).
Verified: Tools back on the tab order with toEqual restored; the ADR amendment with both dates and the Edit ▸ source; the single naming mechanism with positive controls on both new cases; the helper exported and both restatements gone; the schema description using this file's own phrase for topMenu; the Alternatives wording naming the filter half; the zoom sentence corrected and down to one copy; both test names; the doc-comment link; the guide qualification; the E2E comment; the HiddenSectionHeading story; and the description rewritten — the sections table, the Tools order, Open Checks as a Column 3 tab, and isHeaderHidden called out as a new public schema field.
I also went over the Open Checks rework in full and left the detail on that thread. Two things to carry forward from it: the Power path still opens a new panel per click, which is pre-existing and fine but unstated in the ADR; and the app check is the one part static reading cannot settle, so that thread stays open until you have tried it.
The new ADR is in correct byte-order slug position — I checked all 144 entries sort under LC_ALL=C — and its cited symbols resolve. Worth saying given I spent a comment last round on this file's dating and sourcing discipline: this entry meets the bar it sets.
One process note, since it cost you a round of chasing: I cited line numbers into Architecture-Decisions.md, and the rebase moved every one of them. The slug plus the quoted sentence is the durable anchor in an append-only file that sorts by slug. My mistake, and I will anchor that way from the start next time.
Thank you for the thoroughness on this round — particularly for settling 1.6f rather than amending the requirement to fit the code, and for pushing back where you were right.
.context/standards/Architecture-Decisions.md line 3115 at r3 (raw file):
Previously, katherinejensen00 wrote…
## Architecture-Decisions.md line 3115: amended in place
I restored the 2026-09-18 Consequences, and added an "Amended 2026-09-23 (PT-4534, review of #2847)" note covering three things:
- **The Edit ▸ section, with its source.** In the v0 demo's code, the Edit submenu sits outside the Project group, between two dividers and with no heading. The ticket's 2026-09-18 transcription had listed it inside Project.
- **The Quality checks reversal**, citing PT-4734 for the Checking assistant.
- **The zoom gating.**
The Source line now carries both dates. The six mis-indented lines were in the zoom paragraph, which has moved into the amendment with normal indentation. With Tools back on the tab order, the restored sentence about it is true again.
Verified, all three.
The **Amended 2026-09-23 (PT-4534, review of #2847)** note is there, the 2026-09-18 Consequences are restored above it, and the Source line carries both dates. The six mis-indented lines are gone — moving that paragraph into the amendment at normal indentation is a cleaner fix than re-indenting in place, since the text belonged in the amendment anyway.
The Edit ▸ source is the part I care most about, and citing the v0 demo's own code — with the note that the 2026-09-18 transcription had it inside Project — is better than what I asked for. I asked for a source; you gave the reason the two records disagree, which is what a reader hitting this next actually needs.
And with Tools back on the tab order, the restored sentence about it is true again rather than restored-but-stale.
.context/standards/Architecture-Decisions.md line 3122 at r3 (raw file):
Previously, katherinejensen00 wrote…
Reworded as you suggested. It now reads "a schema change plus a change to the shared mode-filtering pipeline every menu consumer runs, for one consumer's need", and points at `isHeaderHidden` in the amendment as render-only and ignored by the menubar.
Verified. It now reads "a schema change plus a change to the shared mode-filtering pipeline every menu consumer runs, for one consumer's need (unlike isHeaderHidden in the 2026-09-23 amendment below, which is render-only and which the menubar ignores)".
That is exactly the distinction that was missing — and pointing forward to the amendment means a reader who starts at the Alternatives section lands on the accepted change rather than having to notice the tension and resolve it themselves.
.context/standards/Entry-Point-Guide.md line 149 at r3 (raw file):
Previously, katherinejensen00 wrote…
## Entry-Point-Guide.md line 149: the accessible name is conditional
Qualified as you suggested. The bullet now says the label names the section only while two or more sections are shown, and a section left on its own (including one left alone by interface-mode filtering) gets neither a heading nor a name. The same qualification is in the `menus.model.ts` TSDoc and in the JSON-schema description.
Verified. The bullet now says the label names the section "while two or more sections are shown", and spells out that a section left on its own — including one left alone by interface-mode filtering — gets neither a heading nor a name, with "Give it a real label regardless" kept.
Naming interface-mode filtering explicitly is the part that earns its place: that is the mechanism most likely to produce the lone-section case in practice, and it is not obvious from the component's own caveat.
e2e-tests/tests/markers-checklist/wiring-theme-5.spec.ts line 41 at r3 (raw file):
Previously, katherinejensen00 wrote…
Reworded. The comment now puts the Power gate on Markers Checklist alone and says Open Checks is reachable in both modes. The `test.use` line is unchanged.
Verified. The comment now reads "Markers Checklist is opened from the scripture editor's hamburger menu, which hides it in Simple mode. Open Checks is reachable from that menu in both modes." — the gate is attributed to the half that still justifies it, and the other half is stated rather than left for the next reader to discover and act on.
test.use({ requiredInterfaceMode: 'power' }) correctly stays, since platformScripture.openMarkersChecklist is still in POWER_ONLY_COMMANDS.
extensions/src/platform-scripture-editor/contributions/menus.json line 13 at r3 (raw file):
Previously, katherinejensen00 wrote…
## menus.json line 13: the Edit section and its only item share a name
I kept the duplication on purpose, and the ADR amendment records it. A column has to have a real label, and this section holds nothing but the Edit flyout, so any other label would just say "Edit" in different words. v0 gives it no name at all, because its Edit submenu sits outside every group. Since the `tab-dropdown-menu.component.tsx:234` change, the name also comes through `aria-labelledby`, the same way every headed section is named.
That is a good call and the reasoning settles it. A column needs a real label, this section holds nothing but the Edit flyout, and any alternative would be "Edit" in other words — so the duplication is the honest option rather than a gap. Recording it in the ADR was one of the two branches I offered, and it is the right one here.
The point also softened on its own: since the single-mechanism change, the name comes through aria-labelledby and a real label node like every other section, so the duplication no longer carries the weight of justifying a bespoke aria-label branch. That was most of why I raised it.
extensions/src/platform-scripture-editor/contributions/menus.json line 363 at r3 (raw file):
Previously, katherinejensen00 wrote…
## menus.json line 363: Quality checks vs the Definition of Done, and the label
The ticket now records it. "Decisions (2026-09-23)" says product asked for Open Checks in Simple. The "Quality checks is omitted" deviation is marked superseded, and the DoD and Testing Idea bullets are amended.
On the label, we're deliberately not using "Checking assistant" yet. That row names the assistant, and bringing the assistant into Simple is PT-4734 ((Simple) Checking Assistant full integration). Until then the item opens the regular Checks panel, so it keeps the Power label instead of naming a tool it doesn't open. The ADR amendment and the ticket both say this.
You are right about the label and I was wrong — worth saying plainly rather than just accepting it.
I reasoned from "the ticket names this row, so the ticket's label is the answer", and skipped the question of what the row actually opens. "Checking assistant" names the assistant, whose Simple integration is PT-4734; this item opens the ordinary Checks panel. Shipping that label now would name a tool the item does not open, which is a worse defect than the register mismatch I was complaining about. Keeping the Power label until PT-4734 lands is the better call, and the ADR and ticket both carrying the reason is exactly the right amount of paperwork for it.
The Definition-of-Done half is verified: the ticket has the "Decisions (2026-09-23)" section, the omission deviation is marked superseded, and the DoD and Testing Idea bullets are amended, so the ticket and the branch now agree.
extensions/src/platform-scripture-editor/contributions/menus.json line 368 at r3 (raw file):
Previously, katherinejensen00 wrote…
## menus.json line 368: Open Checks splits the editor's column
Agreed, fixed. Your first and third parts are in, and the second became an on-demand tab instead of a static one, so Simple's starting layout doesn't change:
- In Simple, `openChecksSidePanel` first checks for an existing Checks tab (`existingId: '?'`, `createNewIfNotFound: false`) and brings it to the front. If there isn't one, it opens the panel as a tab in Column 3 with `{ type: 'tab', parentTabGroupId: 'simple-panel-resources' }`. The extension can't import renderer source, so it keeps its own copy of that panel id, and a check in `simple-layout.data.test.ts` fails if the copy drifts. Power still opens the panel to the right of the editor.
- A reused tab showing a different project or editor gets reloaded with the current options.
- The tab is pinned like the other Column 3 tabs (`isClosable` is false in Simple), and `platformScripture.checksSidePanel` is in `FIXED_LAYOUT_WEBVIEW_GROUPS` under `TAB_GROUP_RESOURCES`.
- An open Checks tab now follows a project switch, the way Find does. The editor calls a new `platformScripture.updateChecksSidePanelProject` right after re-pointing Find, and only in Simple for an editable project. That call creates nothing if no Checks tab is open, and it reloads with `bringToFront: false` so a switch doesn't pull Column 3 away from the user's tab.
- Part 4 doesn't apply now. `TAB_FOR_COMMAND` covers the tabs in the static layout plus the supplement, and Checks is in neither.
Recorded as `adr-simple-column-3-tools-open-on-demand`, since it's the first time an extension adds a tab to Column 3 while the app runs.
Tests: `open-checks-side-panel.utils.test.ts` pins Simple checking for an existing tab first, never using `type: 'panel'`, reusing the tab, and reloading on a project or editor mismatch. It also pins the project-switch update and Power's right-of-editor panel. `checks-side-panel.web-view-provider.test.ts` pins whether the tab can be closed in each mode.
I haven't seen it in the running app yet. Thanks for flagging that the split was reasoned rather than observed; I'll check the first click, a later click and a project switch before this merges.
I read the whole rework rather than just the summary, and it holds up. Recording what I checked, because this is a fair amount of new code that no one else has been over.
Verified:
- The Simple path probes with
createNewIfNotFound: falseand opens{ type: 'tab', parentTabGroupId: SIMPLE_RESOURCES_PANEL_ID }— notype: 'panel'anywhere in that branch, which your "never docks the panel beside the editor" case pins directly. 'platformScripture.checksSidePanel'is inFIXED_LAYOUT_WEBVIEW_GROUPSunderTAB_GROUP_RESOURCES, and the docblock above it now explains the three sources rather than two.- The probe/reload race is handled in both functions —
reloadWebViewresolvingundefinedfalls through to a fresh tab in the opener and to a logged no-op in the updater, and both paths have a test. bringToFront: falseon both calls inupdateChecksSidePanelProject, with the comment noting the default istrue. That is the detail most likely to be dropped in a later edit, so I am glad it is written down.editorScrollGroupIdis preserved from the existing definition rather than passed asundefined, with the reason inline.updateRelatedChecksSidePanelis Simple-only and skips published resources, andisProjectPublishedfails open to "treat it as a translation project", which is the safe direction.- The mirrored panel id: the drift test treats a null regex match as drift, so renaming or moving the constant fails too, not just changing its value. That is stronger than the equality check I would have asked for.
- The new ADR is in correct byte-order slug position (I checked all 144 entries sort under
LC_ALL=C), and its cited symbols resolve.
One thing worth recording, not a defect: the probe is Simple-only, so in Power every click still opens another panel beside the editor — your Power case pins exactly that with getOpenWebViewDefinition never called. That is pre-existing behaviour and this PR does not make it worse, and unbounded panels matter far less in a free-form mode. But the compounding was the heart of the original finding, and the ADR's "Power keeps the panel beside the editor" does not say that it also keeps opening a new one per click. A clause there would tell the next reader it was a decision rather than an oversight. Fixing Power too would be a fine answer as well, but I would not hold this PR for it.
Still open: the app check. Everything above is static reading, and the original claim about the split was reasoned from the rc-dock algorithm rather than seen — so your plan to try the first click, a later click and a project switch before merge is the part that actually settles it. I will leave this thread open until you have done that rather than mark it satisfied on the code alone.
extensions/src/platform-scripture-editor/src/contributions-zoom-menu.test.ts line 56 at r3 (raw file):
Previously, katherinejensen00 wrote…
## contributions-zoom-menu.test.ts line 56: the closing sentence and the three copies
Corrected and thinned out. The full reasoning stays in this test, and now names the routes that work in Simple: Ctrl/⌘+wheel and Ctrl/⌘+`+`/`-`/`0`. It also notes that the tab menu reaches the editor only from the keyboard, because the editor's tab title is hidden. The `menu-data.service-host` copy is now a one-line pointer to this test. The ADR amendment has the corrected sentence. No Simple zoom column.
Verified. The closing sentence now names the routes that actually work in Simple — the wheel and chord shortcuts — and says the tab menu reaches the editor by keyboard only because its tab title is hidden. The menu-data.service-host copy is a one-line pointer here, and the ADR amendment carries the corrected wording, so the one explanation lives in one place with two references to it rather than three copies drifting apart.
Keeping the full reasoning next to the assertion it explains, rather than sending a reader of a failing test into the standards file, is the right way round.
extensions/src/platform-scripture-editor/src/contributions-zoom-menu.test.ts line 59 at r3 (raw file):
Previously, katherinejensen00 wrote…
Renamed it to 'hides every zoom item in Simple and leaves isExperimental off the items'.
Verified — now 'hides every zoom item in Simple and leaves isExperimental off the items', which is exactly what the body checks. The group-level claim stays attributed to the test at :29, where the assertion actually is.
lib/platform-bible-react/src/components/advanced/menus/menu.util.ts line 92 at r3 (raw file):
Previously, katherinejensen00 wrote…
Now `{@link MenuColumnWithHeader.isHeaderHidden}`.
Verified — the comment now links MenuColumnWithHeader.isHeaderHidden, so the pointer resolves to where the meaning and the menubar carve-out actually live. Using the real @ form in the source file is correct; the placeholder I used was only to keep the review comment publishable.
lib/platform-bible-react/src/components/advanced/menus/tab-dropdown-menu.component.tsx line 234 at r3 (raw file):
Previously, katherinejensen00 wrote…
## tab-dropdown-menu.component.tsx line 234: one naming mechanism
I went with the single mechanism. In a menu that shows headings, every section now renders its `DropdownMenuLabel` and is named by `aria-labelledby`. An `isHeaderHidden` column's label just gets `tw:sr-only`, and the `aria-label` branch is gone.
Tests: the existing `isHeaderHidden` case asserts the group is still named "Edit" and its label carries `tw:sr-only`, while the neighbouring headings don't. Two new cases pair `isHeaderHidden` with `showSectionHeadings=false`, and with the column left as the only section; both assert that no group is named. Making the label render unconditionally turns both new cases red.
Verified, and the tests are better than what I asked for.
The aria-label branch is gone; aria-labelledby is now set whenever showHeadings, and the DropdownMenuLabel always renders in that case with tw:sr-only when the column hides its header. I checked that the class actually lands rather than being dropped — DropdownMenuLabel merges className through cn() — because a swallowed class here would render the "hidden" heading visibly and no test that asserts on the class would notice.
Your cases cover the two shapes I was worried about, and both carry positive controls: the neighbouring sections asserted not to have tw:sr-only, and the no-headings case asserting all three groups render before asserting none is named. That second one matters — a bare "nothing is named" passes just as well against a menu that rendered nothing at all, and you gave it something to be a negative of.
The note that jsdom applies no styles, so the class is the observable part of "visually hidden", is the right caveat to leave in place. It tells the next reader what this test does not prove.
That also disposes of the screen-reader question I could not settle: naming now runs through aria-labelledby and a real label node, the same mechanism every headed section already used, so there is no longer an unverified mechanism to check.
lib/platform-bible-utils/src/extension-contributions/menus.model.ts line 60 at r3 (raw file):
Previously, katherinejensen00 wrote…
## menus.model.ts line 60: "such as a web view's tab menu"
Both the TSDoc and the schema description now use the file's own wording for `topMenu` ("the menu that opens when you click on the top left corner of a tab"), plus the two-or-more-sections qualification. The platform-bible-utils dist is rebuilt.
Verified. It now reads "the menu that opens when you click on the top left corner of a tab (topMenu)", which is this file's own phrase for that menu, so the description no longer collides with the tabMenu field below it.
You also folded in the qualification I raised separately on the guide — that the label names the section only while two or more are shown, and a section left on its own gets neither a heading nor a name. Putting that in the schema description as well as the guide is the right call: the schema text is what an author sees in IntelliSense at the moment they set the flag, which is earlier than they would reach the guide.
src/extension-host/services/menu-data.service-host.scripture-editor-menu.test.ts line 206 at r3 (raw file):
Previously, katherinejensen00 wrote…
## menu-data.service-host.scripture-editor-menu.test.ts line 206: the test name
Renamed it to 'the served Simple menu marks only the Edit column isHeaderHidden'. The component test keeps the rendering claim.
Verified — now 'the served Simple menu marks only the Edit column isHeaderHidden', which is what the body actually asserts. The rendering claim stays with the component test, where it can be checked.
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 123 at r3 (raw file):
Previously, katherinejensen00 wrote…
## shipped-simple-layout-order.test.ts line 123: the rule in three places
Took your first option. `isGroupUnderColumnOrSubMenu` is now exported from platform-bible-react's `index.ts`, and both tests import it, so both copies of the rule are gone. One side effect: its TSDoc linked to `getMenuSectionsWithItems`, which isn't exported, and typedoc fails the build on that, so the link is now plain code text.
Verified — exported at index.ts:169, both tests import it from platform-bible-react, and both local restatements are gone. The rule now lives in one place.
The typedoc casualty is on me. I offered that option without checking what exporting the symbol would drag in, and the TSDoc link to getMenuSectionsWithItems having to become plain code text is a real if small cost of taking it. Worth noting in case it bites again: exporting one symbol from that entry point pulls its whole doc comment into the typedoc build, so any link it makes to an unexported neighbour fails the build.
I checked the wider worry too, since widening a published surface for a test-only need is the kind of thing that ages badly: isGroupUnderColumnOrSubMenu does not appear in papi.d.ts, so this did not reach the PAPI surface — and it is a small, documented helper that production already depends on, not a test hook. I am comfortable with where it landed.
src/renderer/components/docking/shipped-simple-layout-order.test.ts line 293 at r3 (raw file):
Previously, katherinejensen00 wrote…
## shipped-simple-layout-order.test.ts line 293: the Tools order (blocking)
Settled on 1.6f. Comments is back to third in Simple's Tools, matching the tab order, and the order-preserving `toEqual` assertion is restored in place of the set comparison. The ADR's Consequences say again that Tools is pinned to the third-column tab order. The ticket's "Decisions (2026-09-23)" section records that it stays that way, alongside the Quality checks reversal.
Verified, and unblocking.
expect(mappedTabs).toEqual(columnWebViewTypes(merged, 2)) is back, and the Simple Tools order in menus.json now reads Bible texts · Commentaries · Comments · Text collection · Find — Comments at third, matching the tab order. So the derivation the Definition of Done names is real again rather than two literals that happen to agree, and the WI-9 case is caught by position, not just membership.
The ticket now carries the "Decisions (2026-09-23)" section covering both reversals, which was the one ask that spanned this thread and the Quality-checks one. Thanks for settling 1.6f rather than amending the requirement to match the code — that was the harder of the two directions.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 made 2 comments.
Reviewable status: 0 of 99 files reviewed, 1 unresolved discussion (waiting on irahopkinson+AI).
a discussion (no related file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
All sixteen checked against the code rather than the replies. Fifteen fixed, one declined with a better argument than mine (the Checking assistant label — I had reasoned from the ticket's wording without asking what the item actually opens).
Verified: Tools back on the tab order with
toEqualrestored; the ADR amendment with both dates and the Edit ▸ source; the single naming mechanism with positive controls on both new cases; the helper exported and both restatements gone; the schema description using this file's own phrase fortopMenu; the Alternatives wording naming the filter half; the zoom sentence corrected and down to one copy; both test names; the doc-comment link; the guide qualification; the E2E comment; theHiddenSectionHeadingstory; and the description rewritten — the sections table, the Tools order, Open Checks as a Column 3 tab, andisHeaderHiddencalled out as a new public schema field.I also went over the Open Checks rework in full and left the detail on that thread. Two things to carry forward from it: the Power path still opens a new panel per click, which is pre-existing and fine but unstated in the ADR; and the app check is the one part static reading cannot settle, so that thread stays open until you have tried it.
The new ADR is in correct byte-order slug position — I checked all 144 entries sort under
LC_ALL=C— and its cited symbols resolve. Worth saying given I spent a comment last round on this file's dating and sourcing discipline: this entry meets the bar it sets.One process note, since it cost you a round of chasing: I cited line numbers into
Architecture-Decisions.md, and the rebase moved every one of them. The slug plus the quoted sentence is the durable anchor in an append-only file that sorts by slug. My mistake, and I will anchor that way from the start next time.Thank you for the thoroughness on this round — particularly for settling 1.6f rather than amending the requirement to fit the code, and for pushing back where you were right.
Thanks, Ira. The app check found one more bug, which is now fixed: a background reload was pulling its tab to the front once it loaded. It was a platform issue that Find had too. Details on the Open Checks thread. The fix changes web-view.component.tsx, so it's worth a look: a web view loading in a hidden tab no longer focuses it.
extensions/src/platform-scripture-editor/contributions/menus.json line 368 at r3 (raw file):
Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…
I read the whole rework rather than just the summary, and it holds up. Recording what I checked, because this is a fair amount of new code that no one else has been over.
Verified:
- The Simple path probes with
createNewIfNotFound: falseand opens{ type: 'tab', parentTabGroupId: SIMPLE_RESOURCES_PANEL_ID }— notype: 'panel'anywhere in that branch, which your "never docks the panel beside the editor" case pins directly.'platformScripture.checksSidePanel'is inFIXED_LAYOUT_WEBVIEW_GROUPSunderTAB_GROUP_RESOURCES, and the docblock above it now explains the three sources rather than two.- The probe/reload race is handled in both functions —
reloadWebViewresolvingundefinedfalls through to a fresh tab in the opener and to a logged no-op in the updater, and both paths have a test.bringToFront: falseon both calls inupdateChecksSidePanelProject, with the comment noting the default istrue. That is the detail most likely to be dropped in a later edit, so I am glad it is written down.editorScrollGroupIdis preserved from the existing definition rather than passed asundefined, with the reason inline.updateRelatedChecksSidePanelis Simple-only and skips published resources, andisProjectPublishedfails open to "treat it as a translation project", which is the safe direction.- The mirrored panel id: the drift test treats a null regex match as drift, so renaming or moving the constant fails too, not just changing its value. That is stronger than the equality check I would have asked for.
- The new ADR is in correct byte-order slug position (I checked all 144 entries sort under
LC_ALL=C), and its cited symbols resolve.One thing worth recording, not a defect: the probe is Simple-only, so in Power every click still opens another panel beside the editor — your Power case pins exactly that with
getOpenWebViewDefinitionnever called. That is pre-existing behaviour and this PR does not make it worse, and unbounded panels matter far less in a free-form mode. But the compounding was the heart of the original finding, and the ADR's "Power keeps the panel beside the editor" does not say that it also keeps opening a new one per click. A clause there would tell the next reader it was a decision rather than an oversight. Fixing Power too would be a fine answer as well, but I would not hold this PR for it.Still open: the app check. Everything above is static reading, and the original claim about the split was reasoned from the rc-dock algorithm rather than seen — so your plan to try the first click, a later click and a project switch before merge is the part that actually settles it. I will leave this thread open until you have done that rather than mark it satisfied on the code alone.
Added the clause. The ADR now says Power deliberately skips the existing-tab check, so each click still opens another panel beside the editor. Layout is free-form in Power and extra panels are the user's to close. main does the same.
Checked in the app, in Simple: the first click adds one Checks tab to Column 3 without splitting the editor's column, and a later click brings the same tab back to the front.
The project switch found a real bug. Checks re-pointed to the new project correctly, but it jumped to the front of Column 3 even though both calls pass bringToFront: false. The cause is platform-wide: every web view focuses its tab when its iframe loads, and that focus puts the tab in front, so a background reload came forward as soon as its content arrived. Find's re-point did the same. web-view.component.tsx now skips that focus while the tab is hidden (no client rects, since rc-dock hides inactive panes with display: none). A tab asked to come forward is already in front when it loads. Recorded as adr-web-view-load-focus-skips-hidden-tabs, with tests for both cases. Re-checked in the app: Column 3 now stays on the user's tab.
The Checks tab also showed the generic logo in Simple, so it now uses Lucide clipboard-check there, the way Find uses search.
05f5581 to
b734bc6
Compare
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 105 files reviewed, all discussions resolved.
a discussion (no related file):
Previously, katherinejensen00 wrote…
Thanks, Ira. The app check found one more bug, which is now fixed: a background reload was pulling its tab to the front once it loaded. It was a platform issue that Find had too. Details on the Open Checks thread. The fix changes web-view.component.tsx, so it's worth a look: a web view loading in a hidden tab no longer focuses it.
Reviewed web-view.component.tsx — detail on the Open Checks thread, and it holds up. Both threads are satisfied from my side now.
I also looked at the third commit, since neither reply covered it: aligning with main's Simple-only Zoom section. I checked main directly rather than inferring — platformScriptureEditor.zoomSection, the hiddenInterfaceModes: ['power'] zoom items and adr-simple-mode-tab-menu-offers-zoom-only are all there — so dropping the two Power-gating commits and rewriting the ADR paragraph is the right response to the collision rather than a change of mind. The Options column is still fully gated, so the leak that gating originally prevented is still prevented, and the served Simple order now pins Zoom between Edit and View.
There is a pleasing symmetry in how that landed: the zoom paragraph we went back and forth over was ultimately about the editor's tab title being unreachable by mouse in Simple, and main has resolved it by giving the menu items to Simple — the mode that cannot reach the tab menu — and leaving Power to the tab menu. The conclusion inverted, but for exactly the reason that thread was about.
One thing to note, low and not blocking, and not your change. Simple's menu now has a Zoom section that the v0 design does not, so the Definition of Done line "matches the v0 demo's sections, order and labels" is overtaken again — the third time, and this time by main rather than by anything in this PR. The ADR amendment records the Zoom section but does not say it diverges from v0. Given the ticket already has a "Decisions (2026-09-23)" section, a line there noting that main's Zoom section supersedes the v0 section list would stop the next person reading the shipped menu against the DoD and filing it as a defect. Your call whether that is worth doing here or belongs with whichever ticket introduced the section.
Heads-up on mergeability: GitHub now reports the PR as conflicting. I checked what actually collides, and it is only lib/platform-bible-react/dist/** — Architecture-Decisions.md, localizedStrings.json and localized-strings.test.ts all auto-merge cleanly. So it is a rebase-and-rebuild rather than anything to untangle, and notably the ADR is clean, so the union-by-slug resolution the repo warns about is not needed this time.
extensions/src/platform-scripture-editor/contributions/menus.json line 368 at r3 (raw file):
Previously, katherinejensen00 wrote…
Added the clause. The ADR now says Power deliberately skips the existing-tab check, so each click still opens another panel beside the editor. Layout is free-form in Power and extra panels are the user's to close. main does the same.
Checked in the app, in Simple: the first click adds one Checks tab to Column 3 without splitting the editor's column, and a later click brings the same tab back to the front.
The project switch found a real bug. Checks re-pointed to the new project correctly, but it jumped to the front of Column 3 even though both calls pass bringToFront: false. The cause is platform-wide: every web view focuses its tab when its iframe loads, and that focus puts the tab in front, so a background reload came forward as soon as its content arrived. Find's re-point did the same. web-view.component.tsx now skips that focus while the tab is hidden (no client rects, since rc-dock hides inactive panes with display: none). A tab asked to come forward is already in front when it loads. Recorded as adr-web-view-load-focus-skips-hidden-tabs, with tests for both cases. Re-checked in the app: Column 3 now stays on the user's tab.
The Checks tab also showed the generic logo in Simple, so it now uses Lucide clipboard-check there, the way Find uses search.
That is the best possible outcome for holding this thread open, and it is worth naming what happened: the app check found a platform bug that neither the static trace nor the tests would have surfaced, and it was not even the bug we were looking for.
The in-app results settle the original finding — one tab in Column 3, no split, and a later click fronting the same tab is exactly the behaviour the rework was aiming at.
I reviewed the core fix, since it changes every web view. It holds up:
iframe.getClientRects().length > 0is the right test. rc-dock keeps an inactive pane mounted underdisplay: none, where geometry reads return nothing — this repo's own hidden-view guidance names that as the detection method, and the// Hidden case:label is the form it asks for. A visible-but-zero-sized iframe still returns one rect, so it still focuses.- The reasoning that a tab asked to come forward is already active by load time checks out:
bringToFrontactivates the tab before the iframe's load event fires, so the visible path is unchanged. - Both tests are there, and the hidden one has a positive control —
applyContentZoomForWebViewasserted called beforesetFocusis asserted not called. Without that, "no focus" would pass just as well against a handler that never ran at all. adr-web-view-load-focus-skips-hidden-tabsis in correct byte-order slug position; I re-checked all 150 entries sort underLC_ALL=C.
The Power clause is in the ADR too, which closes the one thing I left on this thread.
One residual to keep an eye on, not a defect and not blocking. The fix now makes focus conditional on layout having happened. If an iframe in a legitimately-fronted tab ever fires load before its rects exist, that tab silently loses this second focus attempt. The fallback is the mount-time focus in platform-panel.component.tsx, which is the primary mechanism and which this code exists to backstop — so the worst case is the imperfect behaviour that comment already describes, not a new failure. An app check would not reliably surface it either, since it depends on timing. Worth remembering if focus-on-open ever gets reported as flaky.
Marking this satisfied. Thank you for going and looking — the finding as I wrote it was about a layout split, and what it actually shook out was a focus bug affecting Find as well.
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson partially reviewed 27 files and made 1 comment.
Reviewable status: 12 of 105 files reviewed, all discussions resolved.
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson partially reviewed 93 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
In Simple interface mode, the scripture editor's Project menu now reads Project / View / Insert / Tools, with an Edit flyout under Project. Power mode's menu renders exactly as before. Simple and Power share one menu document and only items can be hidden per mode, so Simple gets its own columns (simpleView, simpleTools) holding items hidden in Power, while Power-only items are hidden in Simple. The Insert column is shared. Empty columns are suppressed, so each mode sees only its own sections. Tools lists the third column's tabs in the order they appear there — Bible texts, Commentaries, Comments, Text collection, Find — per the PRD's rule that the menu mirrors the UI. Each item raises the existing tab rather than reloading it, so a panel keeps its scroll position and any in-progress edit, and opens the tab if it is not there. Simple's Comments entry is a new command that fronts that tab; the Power item opens a separate Comment List web view and stays Power-only. The Edit flyout's Undo, Redo, Cut, Copy and Paste run through the editor's own API inside the web view rather than as PAPI commands, because the clipboard needs the click's user activation. Everything but Copy is blocked while editing is unavailable, and a blocked choice says so — naming a Send/Receive as the cause when that is what is blocking. Simple loses its only entry point to several Power tools: the four inventories, Markers Checklist, Check Results, and the auto-show footnote pane toggle. The v0 design has no place for them in Simple. A test now pins the union of both modes against the commands the menu declares, so an item leaving Simple is a decision someone signs for rather than a snapshot edit. Both modes' menus are pinned by a test that builds the real combined menu document, so an accidental change to Power fails. Tools' order is pinned to the shipped third-column layout, and the Edit flyout's command list is pinned to the ids the menu serves. This is the first submenu in a contributed menu, which surfaced two defects in the shared menu components: a submenu item's tooltip attached to a Radix context provider that renders no element, so it silently did nothing; and the submenu trigger's chevron pointed right regardless of direction, while right-to-left layouts open the flyout to the left and map ArrowRight to close. Both are fixed, so the bundle is rebuilt. Deliberate omissions: the Quality checks section (it would hold only a hidden item in Simple), shortcut hints inside the Edit flyout (its chords are handled by the editor, not a catalogued command — PT-4735 covers the missing chords), and item icons. A paste that fails is still silent, which is upstream in the editor's clipboard handling — PT-4741. Also gives the Markers Checklist menu label a Spanish value, so every label in this menu now has one, and four E2E specs that open now-Power-only items declare they require Power mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The third-party-notices omission guard skips test-support files named *.test-utils.ts / *.test-harness.tsx. The `.test-helper.ts` name fell outside that convention, so its `vitest` import was read as a shipping dependency and `verify:third-party-notices` failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Tools panels: move the Bible texts/Commentaries handlers and getProjectIdOfWebView into show-panel.util so each is tested against its own web view type; warn the user when a panel tab can't be opened, and only report Text collection as unavailable when its provider is missing - Edit flyout: extract handleEditMenuCommand so the branch is tested; a durable read-only reason now wins over the Send/Receive notice - Tab menu: wrap a submenu trigger in a tooltip only when it has one, so the trigger keeps its own data-state/data-slot; add RTL ArrowLeft and open-state tests; reuse the submenu fixture - Menu tests: check a command served in both modes carries the same label, and match groups keyed like their column as the renderer does - Rename locale-assets.test-helper to .test-utils and import its dev-only list instead of copying it; cover menu tooltips in the localization test; drop a test that only compared two constants - Docs: document web-view-handled menu command ids in the Entry-Point guide; record why auto-show footnotes stays Power-only and why the Edit flyout can't show shortcut hints; note the Simple settings label and the PT-4735 shortcut follow-up Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
simpleToolsCommands() matched TOOLS groups on `group.column` alone, so a group keyed the same as its column — which also renders under it, per isGroupUnderColumnOrSubMenu in menu.util.ts — would drop out of the derived list. Both sides of the assertion would then shrink together and the check stay green, losing the Definition of Done's Tools-order guarantee silently rather than loudly. Restate the two-branch rule locally, matching the sibling restatement in menu-data.service-host.scripture-editor-menu.test.ts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PT-4575 added platform.webViewContentZoomIn/Out/Reset to the scripture editor's Project menu: appended to Power's options column, and served in Simple under a platformScriptureEditor.options column of its own. Both layout assertions pin their mode's menu in full, so both now include them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Brings Simple's Project menu in line with the v0 demo: - Edit ▸ moves out of the Project section into a section of its own with no heading, between Project and View. - Tools follows the v0 order: Bible texts, Commentaries, Text collection, Find, Comments. The column-3 tabs keep their order, so the layout test now requires one Tools item per tab (as sets) instead of matching the tab order. - A Quality checks section shows Open Checks (the Checks side panel) in Simple. Inventories and Markers Checklist stay Power-only. An unheaded section needs a new optional `isHeaderHidden` flag on menu columns (type and JSON schema in platform-bible-utils). TabDropdownMenu draws no heading for such a column but still divides it and names the group with its label. Rebuilt the platform-bible-utils and platform-bible-react dist bundles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tools order goes back to the third-column tab order (1.6f): Comments is third again, and the layout test compares the served order to the tabs with toEqual rather than as sets. Open Checks no longer splits the editor's column in Simple. It reuses one Checks tab in the third column, opened on first use with parentTabGroupId 'simple-panel-resources' and pinned like its neighbors, and a project switch re-points an open tab the way Find's does (platformScripture.updateChecksSidePanelProject). Power keeps the panel beside the editor. Recorded as adr-simple-column-3-tools-open-on-demand. An isHeaderHidden section is now named the same way as a headed one: its DropdownMenuLabel always renders and is visually hidden, replacing the aria-label branch. New tests pin that no group is named without headings or when the section stands alone, and a Storybook story shows the mode. isGroupUnderColumnOrSubMenu is exported from platform-bible-react so both menu tests import it instead of restating it. Docs: isHeaderHidden's TSDoc, schema description and Entry-Point-Guide bullet name topMenu and say the name holds only while two or more sections show; the per-mode menu ADR restores its 2026-09-18 consequences and adds a dated amendment (Edit's own section, sourced to the v0 demo; Quality checks; zoom gating); the zoom reasoning names the chords that work in Simple and lives in one test; the Markers Checklist E2E gate comment no longer claims Open Checks is Power-only. Two test names now describe only what they assert. The Checks re-point follows the project-kind rule PT-4716 set for the Text Collection: it skips a published resource, reads platform.isPublished rather than isEditable (so an Editable=F translation project is followed), and, like updateRelatedFindPanel, reads the Simple-mode check itself. Rebuilt the platform-bible-utils and platform-bible-react dist bundles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The on-demand third-column ADR said Power keeps the panel beside the editor but not that it skips the existing-tab probe, so every click opens another panel. State that this is deliberate: Power's layout is free-form, and the behavior predates PT-4534. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Checking Open Checks in the running app showed the Checks tab jumping to the front of Column 3 on a project switch, although both of its re-point calls pass bringToFront: false. A web view focuses its tab when its iframe loads, and that focus activates the tab, so a background reload came to the front as soon as its content arrived. Find's re-point had the same problem. A web view now skips that focus while its tab is hidden (no client rects, since rc-dock hides inactive panes with display: none); a tab asked to come to the front is already active when its content loads. Recorded as adr-web-view-load-focus-skips-hidden-tabs. The Checks tab also showed the generic app logo in Simple, where Column 3 collapses tabs to icons. It now uses Lucide's clipboard-check there, the way Find uses search; Power keeps its saved icon. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main now serves the editor's per-pane zoom items in a Zoom section of their own, only in Simple, because the hamburger is the Simple editor's only mouse route to zoom; Power reaches zoom from the tab menu. That replaces this branch's Power-only zoom gate, so: - Pin both modes' layouts to it: Power's Options column no longer lists zoom, and Simple serves it under platformScriptureEditor.zoomSection, between Edit and View. - Rewrite the ADR bullet to record the Simple-only Zoom section, keeping the general rule that an ungated item pulls a Power-only column into Simple. - Build main's hamburger-zoom test menu with the shared test utilities, the same way the editor-menu layout test does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b734bc6 to
b56df81
Compare
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson partially reviewed 25 files and all commit messages.
Reviewable status:
…eout The project-name adoption sweep read ~2,000 source files one at a time. On a cold file cache on Windows, where each first open is also scanned by the antivirus, that took 30-40 s and failed a full `npm test` run at the 15 s test timeout, while passing in about 1 s warm. Measured on idle worktrees, concurrent reads cost 2-4 ms a file cold against about 16 ms sequentially, so the sweep now reads the files with `fs/promises` in parallel. The two tests also share one sweep instead of reading the tree twice, and the non-blocking reads let vitest's timeout fire rather than be overshot by a synchronous scan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 participants
PT-4534: Regroup Simple's Project menu
Summary
In Simple mode, the scripture editor's Project menu (the hamburger in the editor's tab bar) is
regrouped into the sections the design calls for, with an Edit ▸ flyout. Power mode's menu is
unchanged, and a test proves it.
Tools mirrors the third column's tabs, in the order they appear there, and each item brings that tab
to the front. Open Checks opens the Checks panel as a tab in the third column in Simple (reused on
later clicks, and moved to the new project on a project switch) instead of splitting the editor's
column. The inventories and Markers Checklist are hidden in Simple — see
What Simple loses.
The unheaded Edit section needs a new optional
isHeaderHiddenflag on menu columns — a newfield on the public contribution schema (
platform-bible-utils).TabDropdownMenukeeps such acolumn's label as a visually hidden heading, so the section is still divided and still named for
screen readers.
Diff size: 56 source files. The other 19 are the regenerated
platform-bible-react/dist(14) andplatform-bible-utils/dist(5) bundles — skip those.Where to look
Read these four in order and you have the whole change:
platform-scripture-editor/contributions/menus.jsonsimpleView/simpleToolscolumns, theeditSubmenuflyout, andhiddenInterfaceModeson the items each mode shouldn't see.extension-host/services/menu-data.service-host.scripture-editor-menu.test.tsPOWER_ONLY_COMMANDSblock — it's the guard that stops an item silently leaving Simple.platform-scripture-editor/src/show-panel.util.tsplatform-scripture-editor/src/edit-menu-actions.util.tsplatform-scripture/src/open-checks-side-panel.utils.tsrenderer/components/web-view.component.tsxbringToFront: false) came to the front once its content arrived, as the Checks and Find tabs did on a project switch.Everything else is a consequence: the
main.tsfiles register the new commands, the.d.tsfilesdeclare them,
localizedStrings.json× 2 holds the new labels,menus.model.tsandtab-dropdown-menu.component.tsxcarryisHeaderHidden, and the rest are tests and docs.Why Simple gets its own columns (the one design decision worth understanding)
Simple and Power are served from one menu document, and
hiddenInterfaceModesexists on itemsonly — not on columns or groups. So there is no way to say "this section is Simple's".
The approach: give Simple its own columns holding items hidden in Power, hide the Power-only items in
Simple, and let PT-4532's empty-column suppression drop whichever columns end up empty in each mode.
The Insert column is shared by both since it's identical.
The cost is that four items are declared twice, once per mode, and the two modes' column orders are
coupled through the decimal interleave (
simpleView4.5,simpleTools5.5, between Power's 4 and 5).The alternative — teaching the shared menu model per-mode columns — would change the mode-filtering
pipeline every menu consumer runs, for one consumer, so it was rejected for now.
isHeaderHiddenis adifferent kind of change: it is additive and render-only, read solely by
TabDropdownMenu, and theapplication menubar ignores it.
Recorded as
adr-menu-per-mode-layout-via-mode-gated-columnsinArchitecture-Decisions.md, with therule promoted into
Entry-Point-Guide.md.What Simple loses — please sanity-check this with product
For most items hidden in Simple, the editor's Project menu is their only entry point anywhere in
the app, so Simple loses them entirely:
Open Checks stays reachable in Simple, under Quality checks, as product asked on 2026-09-23. The
design's "Checking assistant" row waits for PT-4734. The per-pane zoom items are hidden from Simple's
Project menu too, but only so the Power-only Options column stays out of it; Ctrl/⌘+wheel and
Ctrl/⌘+
+/-/0zoom the editor in every mode.Four markers-checklist E2E specs declare
requiredInterfaceMode: 'power'because of MarkersChecklist.
Going forward, the
POWER_ONLY_COMMANDSallow-list in the menu test means the next item to leaveSimple fails a test with a stated reason, rather than passing as a snapshot edit.
The Edit flyout runs outside PAPI (the design's one oddity)
The flyout's five ids —
platformScriptureEditor.undo,.redo,.cutSelection,.copySelection,.pasteAtSelection— are not registered PAPI commands. They're intercepted in the web view'smenuCommandHandlerand run through the editor's ownEditorRef.Why: the clipboard only works while the click still counts as user activation, and a round trip
through PAPI loses it. Verified in the running app — Cut/Copy/Paste from the menu do reach the system
clipboard.
Two consequences:
EDIT_MENU_COMMANDSagainst the ids the menu actually serves. Without it, adivergence would make the interception stop matching and clicks would fall through to a command PAPI
never registered.
commandfield is typed to registeredcommands only. That's PT-4735, which records the fix (make Undo/Redo real commands — they don't
need activation) and why the cheaper routes are wrong.
Blocked actions notify rather than doing nothing, and name a Send/Receive as the cause when that's
what's blocking.
Two shared-component fixes ride along (why
distis in the diff)This PR ships the first submenu in a contributed menu, which is why it's the first thing to hit two
latent bugs in
platform-bible-react:this feature: no shipped menu item has a tooltip, so the Edit flyout renders the same either way.
The tooltip trigger cloned its props onto
DropdownMenuSub, a Radix context provider thatrenders no DOM node, so everything — including
aria-describedby— was dropped silently. Nowattached to
DropdownMenuSubTrigger, and only wrapped when the item has a tooltip, so thetrigger keeps its own
data-state. It has its own tests intab-dropdown-menu.component.test.tsx.ArrowRight to close, so the arrow pointed at the wrong edge and argued for the dismissing key. Now
follows
readDirection(), with tests for both directions.Both are annotated with
// CUSTOM:markers per the shadcn rule. Becausedistis committed, thisbranch is in the serialized bundle-rebuild queue — coordinate with anyone else holding a pbr
branch.
Tests and verification
New coverage, and what each guard is for:
menu-data.service-host.scripture-editor-menu.test.tsshipped-simple-layout-order.test.ts(extended)show-panel.util.test.tsedit-menu-actions.util.test.tstab-dropdown-menu.component.test.tsxisHeaderHiddensection named only while headings showopen-checks-side-panel.utils.test.tsweb-view.component.test.tsx(extended)checks-side-panel.web-view-provider.test.tslocalized-strings.test.ts(extended)npm test: core 6182 passed / 16 skipped, and every workspace suite green. One footnote-editortiming test flaked under load and passes alone, and three Storybook files failed on a stale deps cache
and pass once it is cleared.
npm run typecheckclean,papi.d.tsunchanged,format:checkcleanfor tracked files. Lint runs scoped to the changed files (repo-wide lint takes 40+ minutes here).
Verified by hand in the running app (Simple and Power): section order, Edit ▸ by pointer and
keyboard, Cut/Copy/Paste reaching the system clipboard, Tools items raising tabs without losing scroll
position, the blocked-action warning in markers view, Power's menu unchanged, and Open Checks as a third-column tab in Simple: the first click adds one tab
without splitting the editor's column, a later click brings back the same tab, and a project switch
re-points it without pulling it to the front. Not yet checked by hand: the unheaded Edit section
with a screen reader.
Known limitations and follow-ups
the Edit flyout's hints and the typing constraint behind them.
disabled state today; a TODO at the registrations points here.
observe it (
EditorRef.paste()returnsvoid; the rejection is uncaught and the renderer's handlerdoesn't reach inside the web view iframe). Upstream in
scripture-editors.can't exist. Menu items can't be gated on a setting — same missing capability as PT-3666.
here, not by a translator. A native-speaker glance would help; a separate PR is expected to refine it.
icons aren't implemented.
regular Checks panel under the Power label "Open Checks…".
the layout is next rebuilt.
Review findings already addressed (from four analysis passes + an adversarial audit)
No Critical findings. Fixed during review:
// CUSTOM:marker...._2plus afallbackKeyfor a v1 string that never existed — theimmutable-strings rule only protects strings translators have seen).
CUSTOM:comment that narrated old behavior instead of stating the constraint.Dismissed with reasons: the silent catch on a throwing Edit action (unreachable — those throws only
happen when read-only, which is gated, or in a layout this editor never sets); cross-extension
duplication of the raise-a-tab helper (no shared home exists —
platform-bible-utilsis a dependencyof PAPI,
platform-bible-reactis frontend, and extensions can't import core); and the English"Comments" vs Spanish "Comentarios de proyecto" difference (the Spanish disambiguates a pair English
leaves ambiguous, and renaming the English would break the match with the tab it raises).
Suggested focus for the review meeting
web-view.component.tsx, decided deliberately for the hidden case (adr-web-view-load-focus-skips-hidden-tabs).It also affects tabs restored into inactive positions at startup.
isHeaderHidden— a new public schema field; the section keeps a screen-reader-only heading.distqueue — sequence with other pbr branches.🤖 Generated with Claude Code
This change is