PT-3958: Configure simple 3-column layout with dispatched editor routing

[jolierabideau]

Code Review Summary

Branch: pt-3958-simple-layout-and-dispatch

Base: origin/main

Date: 2026-05-14

Review model: Claude Opus 4.7

Files changed: 11

Overview

This PR delivers the 10Simple workspace foundation called out in T1 of the saroj-studies-implementation-spec: a fixed 3-column layout ([Model Text | Scripture Editor | Resources & Tools]) loaded when platform.interfaceMode === 'simple'. It also adds the editor-dispatch logic that forces a single Scripture Editor tab in the designated editor column at a time. Downstream work (T3+: addedByRole/addedByUser metadata, Bible Texts tab, Commentaries tab, platform-get-resources changes) is intentionally out of scope.

Two related but at-first-glance unrelated changes are bundled in:

• firstSelectionAsync is now lazy-constructed. Before this PR the editor was rarely opened without a projectId; in simple mode the layout opens the editor empty by default, which previously triggered the eager 10s AsyncVariable timer with no listener and produced an unhandled rejection in startup logs. Making firstSelectionAsync lazy fixes that bug. The getSelection() JSDoc was updated to document the timeout/rejection contract (it now rejects after 10s if no first selection arrives, rather than resolving undefined from an already-timed-out variable).
• Empty-editor state ("No project selected"). A new localized empty-state message replaces the indefinite spinner that previously appeared when the editor was opened without a projectId. A follow-up PR will auto-open the user's last sent/received project, so this state should become rare in practice.

API Changes

• shared/models/docking-framework.model (internal, not re-exported via @papi/): added simpleLayout: LayoutInfo field to PapiDockLayout type.
• lib/papi-dts/papi.d.ts: auto-regenerated to mirror the above.
• extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts:
  • JSDoc on 'platformScriptureEditor.openScriptureEditor' and 'platformScriptureEditor.openResourceViewer''s existingTabIdToReplace parameter now notes that the parameter is honored in power mode but ignored in simple mode.
  • JSDoc on getSelection() now documents the 10s wait and rejection-on-timeout contract.
• No changes to @papi/ module exports, lib/platform-bible-react/, lib/platform-bible-utils/, or other extension .d.ts declarations.

Findings

Critical — Must address before merge

None.

Important — Should address before merge

• Dispatch in simple mode collides editable editors and read-only resource viewers — resolveOpenEditorDispatch filtered only by webViewType === SCRIPTURE_EDITOR_WEBVIEW_TYPE, so opening a Resource Viewer for project X while an editable Scripture Editor for project X was open returned focus-existing on the wrong view. (fixed during review: added isReadOnly to dispatch input, derived from state?.isReadOnly per editor def; focus-existing now requires both projectId AND isReadOnly to match. Added 4 new test cases covering the editable/readonly distinction in simple mode and a confirmation that power mode ignores isReadOnly.)
• JSDoc on existingTabIdToReplace didn't note simple-mode behavior — Callers (e.g., platform-get-resources passing its NewTab id) couldn't reliably reason about the parameter in simple mode. (fixed during review: both openScriptureEditor and openResourceViewer JSDocs now state that the parameter is honored in power mode but ignored in simple mode.)
• Localized strings out of alphabetical order — New emptyState_noProject key was inserted between error_* keys (emptyState sorts before error), violating the alphabetization rule in the style guide. (fixed during review: moved the new key to the correct alphabetical position in both en and es blocks.)
• Empty state is a dead-end for the user — "No project selected" message offers no path forward (no link, button, or hint). For Saroj in simple mode this is likely confusing. (A follow-up PR will auto-open the user's last sent/received project on startup, so this empty state should become rare. Author chose to address there rather than wire a guided action into this PR.)
• Spanish translation source — Was the new Spanish string "Ningún proyecto seleccionado" translated with the localization team? (Author confirmed the translation was machine-translated. Flag for the localization team to review when they next pass over this file.)

Minor — Consider

• getSelection() JSDoc didn't note the new timeout/rejection contract (fixed during review: expanded JSDoc to describe lazy first-selection wait, 10s timeout, and explicit rejection behavior so callers can prepare for both undefined and rejected-promise outcomes.)
• interfaceMode parameter typed 'simple' | 'power' | undefined — Setting type is 'simple' | 'power' with default 'simple'; the undefined branch is unreachable. (fixed during review: tightened parameter type to 'simple' | 'power' and updated the JSDoc accordingly.)
• TODO in web-view.service-host.ts left as dead-end documentation (fixed during review: removed the TODO comment; kept the if (layout) branch since it remains semantically valid for an explicit override even if no callers use it today.)
• loadLayout reads platform.interfaceMode only at startup — Toggling at runtime won't switch layouts. (fixed during review: added a code comment explaining this is intentional since no runtime mode switcher exists yet.)
• Placeholder webViewType strings could be extracted to named constants in simple-layout.data.ts (Author chose to defer; placeholder constants will be touched when the follow-up tickets PT-3961/3962/3963 land.)
• firstSelectionAsync refactor appears unrelated to PR scope (Author explained: the eager 10s timer fired with no caller listening because no getSelection() caller exists during the pre-project window, producing unhandled rejections in startup logs. The bug is directly caused by this PR's new empty-editor case in simple mode, so it's on-topic. See the Overview for the explicit linkage included in this PR description.)
• Test hardcodes TAB_TYPE_WEBVIEW: 'webView' mock value (On inspection, the mock exists only to satisfy module-load when simple-layout.data.ts imports from web-view.component.tsx. No assertion in the test depends on the mocked value, so staleness risk is zero. Would only be worth addressing if the constant were extracted to a tiny shared module to drop the mock entirely.)
• loadLayout interface-mode branching has no tests (Author will address in a follow-up issue; matches the file's current pattern of no existing tests.)
• simple-layout.data.ts placement diverges from test-layout.data.ts (Author explained: simple-layout.data.ts is production code used when interfaceMode === 'simple', not test code, so it correctly lives next to platform-dock-layout.component.tsx rather than under src/renderer/testing/.)
• Empty state UI is a bare string with no semantic wrapper or visual treatment (Author chose to keep consistent with the existing bookNotFound* branches in the same render path. Combined with the auto-open follow-up making the empty state rare, the styling is left as is for this PR.)
• "No project selected." had a trailing period inconsistent with short-label pattern (fixed during review: removed the trailing period in both en and es so the string reads as a status label rather than a sentence.)
• Column sizes hardcoded 1:2:1 may approach the 300px-per-tab responsive floor on smaller windows (Author will discuss column sizing and resize behavior with UX and address in a follow-up issue. See Suggested Review Focus for the open UX question.)

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

• resolveOpenEditorDispatch is a pure function with thorough TSDoc and (post-review-fix) 14 unit tests covering each branch in both simple and power modes, including the new (projectId, isReadOnly)-keyed focus-existing logic.
• The OpenEditorDispatch tagged-union type follows the style guide's preference for string unions over interdependent booleans.
• simpleLayout uses kebab-case filename, follows the existing testLayout pattern, and ships with a colocated .test.ts that asserts the structural invariants (3 columns, vertical boxes, unique tab ids, expected webViewType placeholders).
• papi.d.ts was regenerated correctly — diff mirrors the source change and is not hand-edited.
• ESLint-disable comments throughout the new code each have an accompanying comment explaining why, matching the style guide's "Explain rule exceptions" rule.
• New localized string key %webView_platformScriptureEditor_emptyState_noProject% was correctly added to both en and es locales rather than hardcoded, and is registered in EDITOR_LOCALIZED_STRINGS so it actually loads at runtime.
• Pre-existing convertScriptureRangeToEditorRange utility had no tests before this PR; the new test file adds a comprehensive suite covering all four ScriptureRange location input shapes, mixed types, error paths, and data-provider interactions.
• Empty-state UI reuses the same Tailwind class structure (tw:flex tw:items-center tw:justify-center tw:h-full tw:px-4) as the existing bookNotFound* branches — consistent treatment, RTL-safe.
• The lazy firstSelectionAsync fix resolves a real bug (unhandled rejection in startup logs) that was triggered by this PR's empty-editor-in-simple-mode case.
• No deletions of existing exports; no parameter removals or reorderings; no type narrowings on extension-facing API surfaces.

Interview Notes

Stated purpose: Create the 3-column layout for interfaceMode === 'simple' only and force a single editor tab in the designated editor column. Follows the T1 ticket in the saroj-studies-implementation-spec.

Key design explanations (verified in interview):

• simple-layout.data.ts placement: deliberately colocated with platform-dock-layout.component.tsx rather than under src/renderer/testing/ because it is production code used when interfaceMode === 'simple', not test fixture data. The placement is correct; the divergence from test-layout.data.ts's location is intentional.
• firstSelectionAsync lazy construction: directly motivated by this PR's new empty-editor case. The eager 10s timer fired with no caller listening, producing unhandled rejection in startup logs. The author confirmed they explicitly considered whether to split this into a separate PR and chose to keep it bundled so that the bug and its triggering case ship together — the PR description (this summary's Overview) calls out the linkage explicitly.

Decisions made during the interview:

• Editable vs read-only dispatch: author confirmed the collision was not intentional. We implemented Option A (add isReadOnly to the dispatch helper input so focus-existing requires both projectId AND isReadOnly to match).
• Empty state action affordance: deferred to a follow-up PR that will auto-open the last sent/received project.
• Spanish translation: author confirmed machine-translated; flagging for localization team review.
• loadLayout TODO: removed the comment (kept the branch as a valid override path).
• Period on empty-state string: removed for consistency with short-label patterns.
• Column sizes / responsive behavior: deferred to a follow-up after UX consultation.

No unresolved understanding gaps. The author explained both non-obvious design decisions (simple-layout placement and firstSelectionAsync linkage) clearly and without deferring to AI tooling.

In-Review Quality Check

• npm run typecheck — Reported TS2688 errors for hello-world, platform-home, and resource-viewer type definitions. Verified pre-existing on the branch (reproduces with in-review changes stashed). Not caused by this review's changes — these three extensions lack a top-level package.json with a types field, unlike platform-scripture-editor. Author should be aware these exist but they are out of scope for this PR.
• npm run lint — Passed (warnings only). 8 prettier/JSDoc reformatting warnings in the JSDoc strings I added/edited were auto-fixed via npm run lint-fix in the platform-scripture-editor extension. Re-ran lint: 0 warnings.
• npm run format — Clean after the lint-fix pass.
• npm test — All 1,845 tests across 8 suites passed, 1 skipped, 0 failures. The 33 tests in platform-scripture-editor.utils.test.ts include the 4 newly-added editable/readonly distinction cases.

Suggested Review Focus

Prioritized areas for the author-reviewer meeting:

• Editor dispatch keying on (projectId, isReadOnly): walk through the simple-mode focus-existing vs replace-tab decision tree. Is there a scenario the reviewer wants covered that the new 4 tests don't (e.g., multiple existing editors of different modes)? Verify the state?.isReadOnly derivation in main.ts handles the case where state is undefined for newly-opened editors.
• firstSelectionAsync lazy construction: confirm with the reviewer that bundling this fix into this PR (rather than splitting) is acceptable. The Overview section above describes the linkage; the getSelection() JSDoc documents the new timeout/rejection contract.
• loadLayout is one-shot on startup: there is no runtime mode switcher, so changing platform.interfaceMode at runtime does not switch layouts. Confirm with the reviewer this is the right product behavior for the demo deadline.
• Open UX question — column sizing/resize behavior in simple mode: the layout hardcodes 1:2:1 column proportions. The 10Simple PRD does not specify whether columns can be dragged/resized; the implementation spec lists "prevent columns from being moved or closed" as a nice-to-have. At smaller widths, the side columns can approach the 300px-per-tab responsive floor. Question for UX: should simple-mode columns be resizable? Are the 1:2:1 proportions correct, and should they adapt at narrower widths (e.g., reflow, collapse, or hide a column)?
• Spanish translation: the new "Ningún proyecto seleccionado" string was machine-translated. Localization team should pass over it.
• Pre-existing typecheck issue: TS2688 errors for hello-world, platform-home, resource-viewer are unrelated to this PR but visible in npm run typecheck output. Flag for separate cleanup.

---

---

---

This change is 

[katherinejensen00]

These changes to support a three column layout look really helpful, Jolie! Thank you for making them! Just a few things (mostly from Claude) to look at.

@katherinejensen00 reviewed 12 files and all commit messages, and made 10 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on jolierabideau).

---

extensions/src/platform-scripture-editor/src/main.ts line 683 at r1 (raw file):

      },
      async getSelection() {
        // If we already have a selection, return it directly.

## 1. `getSelection()` — rejection only reaches the first caller

After the 10s timeout fires and `firstSelectionAsync` rejects, `hasSettled` becomes `true`. Any subsequent call to `getSelection()` will skip the `return firstSelectionAsync.promise` line and fall through to `return currentSelection` — which resolves to `undefined`. So the rejection only surfaces to the first concurrent caller.

The JSDoc now says "rejects if none arrives in that window — callers should be prepared to handle a rejected promise as well as a resolved `undefined`." That technically covers both cases, but is this the intended contract? Callers who retry after the timeout will silently get `undefined` instead of another rejection.

---

---

extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts line 453 at r1 (raw file):

  if (interfaceMode === 'simple') {
    // Simple mode invariant: every Scripture Editor open lands in the editor column. The
    // caller-supplied `existingTabIdToReplace` is intentionally ignored — e.g., the column 3

NIT: "The column 3" sounds a tiny bit awkward. Could this be "the last column" or "the third column" or just "column 3"

---

extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.test.ts line 423 at r1 (raw file):

      'caller-supplied-tab-id',
    );
    // Simple-mode invariant: opens land in the editor column. Caller's tab is not the editor —

NIT: What does "opens land in the editor column" mean? Is it opening space or something else? It's just a test, but it is confusing.

---

lib/papi-dts/papi.d.ts line 3131 at r1 (raw file):

     * service is refactored to split the code between processes. The only reason this is passed from
     * `platform-dock-layout.component.tsx` is that we cannot import `testLayout` here since this
     * service is currently all shared code. Refactor should happen in PT-2799

Thanks for updating this work item number!

---

src/renderer/components/docking/simple-layout.data.ts line 7 at r1 (raw file):

// Using `as` here simplifies type changes.
/* eslint-disable no-type-assertion/no-type-assertion */
export const simpleLayout: LayoutBase = {

NIT ```markdown

3. Redundant export default alongside the named export

All callers (the component and the test) use the named import { simpleLayout }. The default export on line 733 goes unused and leaves a dangling API surface — suggest removing it.

---

---

src/renderer/components/docking/simple-layout.data.ts line 65 at r1 (raw file):

                tabType: TAB_TYPE_WEBVIEW,
                data: {
                  // TODO PT-3962: Replace with Bible texts tab webViewType when implemented

## 2. Column 3 has two tabs with identical `webViewType`

Both column 3 placeholder tabs render the same `'platformGetResources.newTab'` component — they're visually indistinguishable at runtime. Is it necessary for both to be in the layout now, or could the second be deferred until PT-3963 actually lands? Also minor: the test asserts `.toContain('platformGetResources.newTab')` but doesn't assert the count, so the duplication is invisible in test output.

---

---

src/renderer/components/docking/simple-layout.data.test.ts line 11 at r1 (raw file):

vi.mock('../../../shared/services/logger.service');

describe('Dock Layout Component', () => {

NIT

## 4. Test describe label is misleading

The outer `describe('Dock Layout Component', ...)` suggests this tests a React component, but `simple-layout.data.ts` is pure data. Failed test output would print `Dock Layout Component > simpleLayout > ...` which is confusing. Suggest renaming to `'simpleLayout data'` or `'simple-layout.data'`.

---

---

src/renderer/services/web-view.service-host.ts line 649 at r1 (raw file):

  }

  // Startup load — pick the layout based on interface mode. This runs once at startup; changes

Question for you. Claude pointed out that "In simple mode, saveLayout still fires whenever the layout changes (e.g., user resizes a column). That writes to DOCK_LAYOUT_KEY in localStorage. But on the next startup in simple mode, the code loads dockLayoutVar.simpleLayout (the static definition), not localStorage. This means column resizes are ephemeral in simple mode — they reset on every restart."

Is that intentional behavior? It sounds like it could be, just checking

---

src/renderer/services/web-view.service-host.ts line 653 at r1 (raw file):

  // mode switcher exists yet).
  const interfaceMode = await settingsService.get('platform.interfaceMode');
  const layoutToLoad =

## 5. Simple-mode user dock changes are silently discarded on restart

`onLayoutChange` still fires when the user drags tabs in simple mode and writes to `DOCK_LAYOUT_KEY`. But on next startup that stored value is ignored — `simpleLayout` is always loaded fresh. If the intent is to enforce a fixed layout, consider suppressing `onLayoutChange` storage writes in simple mode so stale data doesn't accumulate in storage and the intent is explicit in the code.