PT-4068/PT-4069: Add Comments tab to Column 3 in Simple mode#2500
Conversation
tombogle
requested review from
irahopkinson,
lyonsil and
tjcouch-sil
as code owners
|
Devin review found a potential bug. Here is the prompt it suggests feeding into Claude: I'm not actually sure if this is likely to be a real problem since we will seldom (never???) want to clear out the active project after once successfully setting one. (The only time that could maybe happen, I think, would be if we deleted the only project, but since deleting a project can't be done in Simple mode, that's probably not a real scenario.) Claude's recommendation: record Devin's finding as a known limitation in the summary (alongside the race condition), but don't fix it unless you want to. The fix is straightforward — add a boolean flag: let pendingCommentListPanelProjectId: string | undefined; |
|
Devin also detected that |
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Great work, Tom! The feature is sound and follows the existing Column-3 panel patterns faithfully. Only one thing I'd fix before merge (a misleading code comment that Claude found); the rest are optional cleanups and a suggestion to split out the unrelated housekeeping. Even the comment is minor so I went ahead and approved the changes.
@katherinejensen00 reviewed 18 files and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on tombogle).
e2e-tests/tests/isolated/comments-tab.spec.ts line 201 at r1 (raw file):
// Switch to Project B — triggers openOrUpdateRelatedPanels(projectB.projectId) await sendPapiRequestOnce( 'command:platformScriptureEditor.openResourceViewer',
NIT Claude Finding
Summary: The PT-4069 project-switch test dispatches a PAPI command rather than driving the switch through the UI, so it verifies the wiring more than the user journey.
Per the Testing-Guide UI-Only Interaction Rule (Testing-Guide.md:1054-1062), per-feature tests should switch projects through visible controls. This test switches via command:platformScriptureEditor.openResourceViewer (:186, :201). I'll note the mitigating context: comment.fixture is a pre-existing, already-sanctioned fixture (used by the comment-assignment tests), and the locale/mode setup genuinely has no UI path — so this is not a blocker. But the project switch does have a UI path; if it's practical, driving it through the UI (or a one-line comment justifying the command-based approach) would make the test prove the real flow.
e2e-tests/tests/manage-books/README.md line 3 at r1 (raw file):
# manage-books E2E Tests > **Note:** These tests are experimental and are **not wired into any standard test run**
Will they ever not be experimental?
e2e-tests/tests/markers-checklist/README.md line 3 at r1 (raw file):
# markers-checklist E2E Tests > **Note:** These tests are experimental and are **not wired into any standard test run**
Will they ever not be experimental?
e2e-tests/tests/smoke/README.md line 14 at r1 (raw file):
Feature-specific or state-mutating tests belong in `isolated/` instead.
This is nice. I like the option to run it separately since e2e tests tend to be a bit flaky so they aren't great for CLI, but are helpful when developing in a similar area.
extensions/src/legacy-comment-manager/src/comment-list-panel.utils.ts line 4 at r1 (raw file):
* Resolves the projectId to display in the Comment List Panel. * * Priority: pending sentinel (set by openCommentListPanel before reloadWebView) >
NIT Claude Finding (comment clarification)
Summary: The priority-chain JSDoc is garbled by a stray > that splits the sentence into a spurious blockquote.
Lines 4-7 render as "Priority: pending sentinel (…) > openWebViewOptions" then, after a blank comment line, "> SavedWebView. The sentinel exists because…" — the > separating openWebViewOptions and SavedWebView got orphaned onto its own line and reads as a Markdown blockquote. Reflow to one line: Priority: pending sentinel > openWebViewOptions > SavedWebView. (Compare the clear inline version at platform-scripture-editor/src/main.ts:847.)
extensions/src/legacy-comment-manager/src/main.ts line 167 at r1 (raw file):
*/ async function openCommentListPanel(projectId: string | undefined): Promise<string | undefined> { // Use existingId: '?' to detect-and-reuse the fixed Column 3 tab without calling
NIT Claude Finding (Comment clarification)
Summary: The comment justifying existingId: '?' states something that isn't true — getAllOpenWebViewDefinitions is available from extension-host context, so the rationale is misleading even though the code works.
The comment says we use existingId: '?' "without calling getAllOpenWebViewDefinitions (which is not reliably available from extension context)." But the two sibling Column-3 panels this feature mirrors call it from extension-host context in the very same openOrUpdateRelatedPanels flow — openModelText (platform-scripture-editor/src/main.ts:1207) and openResourceText (:942) — and it's also used at main.ts:246 and platform-scripture/src/hooks/use-open-project-tabs.ts:111. The existingId: '?' approach itself is fine and documented, so nothing needs to be re-architected — but please just delete or correct the parenthetical, since it will mislead the next maintainer. (Note: don't "fix" it by switching to the sibling mechanism — this panel deliberately reloads with bringToFront: false whereas the siblings use true, so they aren't drop-in equivalent.)
extensions/src/legacy-comment-manager/src/types/legacy-comment-manager.d.ts line 439 at r1 (raw file):
* * @param projectId The project whose comments to display * @returns The webView ID of the panel
NIT Claude Finding (pretty nitpicky but could be helpful for intellisense)
Summary: The public command's @returns doesn't mention it can resolve to undefined.
The declared type is Promise<string | undefined> and the implementation's own JSDoc documents the undefined-on-failure case, but this papi-shared-types CommandHandlers entry — the surface other extensions see in IntelliSense — omits it. The sibling openCommentList entry (line ~425) documents its undefined case. Suggest matching that.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts line 912 at r1 (raw file):
/** * Opens or updates the model text, commentary, and scripture resource panels for a project.
NIT Claude Finding (Minor clarification)
Summary: After the rename, the JSDoc, @param, and region label still describe the old "text connections" concept and don't mention the comment-list panel this PR adds.
The function is now openOrUpdateRelatedPanels and opens a fourth panel (the comment list, line 945), but: the summary (:912) lists only "model text, commentary, and scripture resource panels"; the @param (:915) still says "open the text connections for"; and the region markers (:909 / :951) still read Text Connection Panels. Suggest refreshing all three to the "related panels" framing.
tombogle
left a comment
tombogle
left a comment
There was a problem hiding this comment.
@tombogle made 3 comments and resolved 6 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on katherinejensen00 and lyonsil).
e2e-tests/tests/manage-books/README.md line 3 at r1 (raw file):
Previously, katherinejensen00 wrote…
Will they ever not be experimental?
I don't know. This is just what @lyonsil told me. Presumably if they things they test get fully implemented, they could be reworked to test them and then they would get moved and wired into a standard test run, at which point this comment and the readmes would be changed or go away.
e2e-tests/tests/markers-checklist/README.md line 3 at r1 (raw file):
Previously, katherinejensen00 wrote…
Will they ever not be experimental?
see above
extensions/src/legacy-comment-manager/src/main.ts line 167 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT Claude Finding (Comment clarification)
Summary: The comment justifying
existingId: '?'states something that isn't true —getAllOpenWebViewDefinitionsis available from extension-host context, so the rationale is misleading even though the code works.The comment says we use
existingId: '?'"without callinggetAllOpenWebViewDefinitions(which is not reliably available from extension context)." But the two sibling Column-3 panels this feature mirrors call it from extension-host context in the very sameopenOrUpdateRelatedPanelsflow —openModelText(platform-scripture-editor/src/main.ts:1207) andopenResourceText(:942) — and it's also used atmain.ts:246andplatform-scripture/src/hooks/use-open-project-tabs.ts:111. TheexistingId: '?'approach itself is fine and documented, so nothing needs to be re-architected — but please just delete or correct the parenthetical, since it will mislead the next maintainer. (Note: don't "fix" it by switching to the sibling mechanism — this panel deliberately reloads withbringToFront: falsewhereas the siblings usetrue, so they aren't drop-in equivalent.)
Fixed the comment. FWIW, in the app I just tried changing the active project a couple times, and regardless of which tab or pane had focus, the focus is always set to the main editor pane, and the first tab "Bible Texts" is always the one shown in column 3. So whatever bringToFront:true may be doing or claiming to do, it does not seem to have the final say.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Thanks for making those changes. Looks great!
@katherinejensen00 reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on lyonsil).
|
@katherinejensen00 Note that the production-code changes in |
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Good to know. Thanks!
@katherinejensen00 reviewed 4 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on lyonsil).
e2e-tests/tests/manage-books/README.md line 3 at r1 (raw file):
Previously, tombogle (Tom Bogle) wrote…
I don't know. This is just what @lyonsil told me. Presumably if they things they test get fully implemented, they could be reworked to test them and then they would get moved and wired into a standard test run, at which point this comment and the readmes would be changed or go away.
Fair enough. I trust Matt L, let's go with what he said.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
2 super minor optional improvements that you could make in another PR or not at all.
@katherinejensen00 reviewed 14 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on lyonsil and tombogle).
e2e-tests/tests/isolated/comments-tab.spec.ts line 218 at r4 (raw file):
const commentsFrame = commentsFrameLocator(mainPage); await commentsFrame.locator('body').waitFor({ state: 'attached', timeout: 15_000 });
NIT Claude Finding
This test only waits for the iframe body to reach attached, which is true as soon as the frame element exists — so it doesn't actually verify the panel displayed anything, despite the test name. It would pass on a panel that mounts empty, errors, or shows the wrong project. Consider asserting on a stable rendered element inside the panel (a filter control, the empty-state text, or a data-testid) so the assertion matches the test's intent.
extensions/src/platform-scripture-editor/src/main.ts line 255 at r4 (raw file):
} catch (e) { logger.warn( `openResourceViewer: getAllOpenWebViewDefinitions timed out or failed (${getErrorMessage(e)}); opening as new tab`,
NIT Claude Finding
Two new warning logs are prefixed openResourceViewer:, but they live inside open() — which serves both the editable Scripture Editor and the read-only Resource Viewer paths — so the prefix is misleading on the editable path. Consider open: (or branch the prefix on isReadOnly) so log triage points at the right entry point. Same fix applies to the re-check log at line 352.
…r tab distinctiveness
…ject-switch (PT-4069)
…ents tab tests Change PAPI_REQUEST_TIMEOUT_MS (30s) to SETTINGS_TIMEOUT_MS (60s) in Tests 3 and 5 to account for slower CI environments during extension registration. Co-Authored-By: Claude Code <noreply@anthropic.com>
…comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ventually to become a tooltip) text to diverge from the floating pane title
…egistration
- Add dedicated localization key %webView_legacyCommentManager_commentListPanel_title%
(Spanish: "Comentarios de proyecto") so panel tab and floating list can diverge
- Revert %webView_legacyCommentManager_commentList_title% Spanish to "Comentarios"
- Rename openTextConnectionPanels → openOrUpdateRelatedPanels to reflect expanded scope
- Fix JSDoc on openCommentListPanel to describe behavior, not callers
- Fix error message casing ("WebView" → "web view") for consistency
- Move E2E spec from simple-mode-comments/ into isolated/ (already-registered project)
- Add READMEs to smoke/, isolated/, enhanced-resources/, manage-books/, markers-checklist/
- Add e2e-tests/CLAUDE.md documenting where new E2E tests belong
- Gitignore Claude Code local settings files (.claude/*.lock, settings.local.json)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Session-URL: <session URL>
…nWebViewDefinitions getAllOpenWebViewDefinitions is not reliably available from extension context (JSON-RPC -32601 at runtime). The existingId:'?' + createNewIfNotFound:false two-call pattern is the documented codebase standard for single-instance webview reuse (see openManageBooks/openFind in platform-scripture). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- openCommentListPanel's probe openWebView call was missing bringToFront:false, causing the Comments panel to briefly steal focus on every project switch. - Extract resolveCommentListPanelProjectId into comment-list-panel.utils.ts so the sentinel priority logic (pending > options > saved) can be unit-tested without the ?inline webpack imports that Vitest can't process. - Add 4 unit tests covering the priority chain and the undefined-all case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Session-URL: https://claude.ai/code/28cdd4e1-f95f-409e-9721-f18c433482a3
- Fix non-falsifiable Commentaries-title localization test (read the key from platform-scripture-editor's strings file; guard with toBeDefined) - Fix lint errors: disable-comment justifications, no-continue retry loop, prettier formatting - Document accepted sentinel limitation in openCommentListPanel JSDoc - Gate replace-tab re-check to simple mode (re-resolve could not help in power mode) - Gate openOrUpdateRelatedPanels on project editability so opening a read-only resource no longer re-points related panels (incl. Comments) - Switch comment E2E tests from openResourceViewer to openScriptureEditor to match the real user flow for editable fixture projects - Add fallbackKey for the comment list panel title localization key - Emit data-web-view-id only for WebView tabs (new webViewId prop) - Restore developer's dev-appdata settings in E2E worker teardown - Extract toScriptureEditorInfos into utils with unit tests (dedupe) - Extract waitForOverlayGone E2E helper; simplify sentinel resolution to a ?? chain; rename webview-type constant to camelCase - Remove .claude .gitignore entries (moving to separate housekeeping PR) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Session-URL: <session URL>
… import TAB_TYPE_WEBVIEW from new location: docking-framework.model.ts PT-4069: Addressed confusing comments and warning message cause by previous change
tombogle
force-pushed
the
pt-4068-add-comments-tab-to-column-3
branch
from
c5c1422 to
1ee08f0
Compare
tombogle
left a comment
tombogle
left a comment
There was a problem hiding this comment.
@tombogle resolved 3 discussions.
Reviewable status: 10 of 25 files reviewed, all discussions resolved (waiting on katherinejensen00).
2 participants
Code Review Summary
Branch: pt-4068-add-comments-tab-to-column-3
Base: origin/main
Date: 2026-07-08
Review model: Claude Fable 5
Files changed: 24 (23 at review start;
.gitignorereverted to base and 2 renderer/test files touched by review fixes)Overview
This review pass covers the latest commits and staged changes since the last review pass on the PT-4068 PR (Comments tab in Column 3 of Simple mode). The changes are mostly (a) improved/added comments and documentation, and (b) reliability work for the new E2E tests: pre-configuring locale/interface-mode before app launch, UUID-based locale-independent selectors (
data-web-view-id), retry/re-check logic for the "Replacing tab failed" dock race, overlay-clear waits, and E2E test-organization docs.During the interview, a pre-existing behavior gap was found and fixed:
openOrUpdateRelatedPanels(formerlyopenTextConnectionPanels) ran for any project opened in simple mode, including read-only published resources — which would have re-pointed the model text, Bible texts, Commentaries, and (new in this PR) Comments panels at the resource. The call is now gated on the project being editable, and the E2E tests were aligned to callopenScriptureEditor(semantically correct for the editable fixture projects) instead ofopenResourceViewer.API Changes
extensions/src/legacy-comment-manager/src/types/legacy-comment-manager.d.ts(papi-shared-types/CommandHandlers): Added'legacyCommentManager.openCommentListPanel': (projectId?: string | undefined) => Promise<string | undefined>— new command, additive only.lib/platform-bible-react/: no changes.lib/platform-bible-utils/: no changes.lib/papi-dts/papi.d.ts(@papi/modules): no changes.openTextConnectionPanelswas renamed toopenOrUpdateRelatedPanelsinextensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts. This is an extension-internal module, not a public API surface; zero references to the old name remain.Findings
Critical — Must address before merge
None.
Important — Should address before merge
localized-strings.test.ts: the "Spanish label differs from the Spanish Commentaries tab title" test read%webView_resourcePanel_commentaries_title%from legacy-comment-manager's strings file, but the key lives in platform-scripture-editor's file, so the comparison was always againstundefinedand could never fail. (fixed during review: test now loads platform-scripture-editor'slocalizedStrings.jsonvia a sharedreadLocalizedStringshelper and asserts the Commentaries keytoBeDefined()so a future rename fails loudly)localized-strings.test.ts:eslint-disablewithout required justification comment + prettier warnings. (fixed during review: justification added as part of the test rewrite; file lints clean)e2e-tests/fixtures/helpers.ts:eslint-disable no-type-assertioninpreConfigureSettingswithout justification + prettier warning. (fixed during review: justification comment added; prettier applied)comments-tab.spec.ts:continuein the retry loop flagged byno-continue. (fixed during review: condition inverted to throw in the non-retry case and fall through otherwise; behavior unchanged)platform-tab-title.component.tsx:data-web-view-idpushed the<div>past print width. (fixed during review: prettier applied — one attribute per line)openCommentListPanel: claimedprojectId: undefinedshows an empty panel, but an already-open panel keeps its current project becauseundefineddoubles as the sentinel's "no pending value". This is the same issue Devin review flagged; the team decision (recorded on the PR) was to accept it as a known limitation since clearing an active project is never needed in Simple mode. (fixed during review: JSDoc and sentinel comment now document the actual behavior and the accepted limitation)platform-scripture-editor/src/main.ts): re-resolving with the caller-suppliedexistingTabIdToReplacereturns the same gone tab id in power mode, soopenWebViewwould still throw. (fixed during review: re-check gated tointerfaceMode === 'simple'— the race source,openOrUpdateRelatedPanels, only runs there; also avoids an extragetAllOpenWebViewDefinitionsround trip in power mode)openOrUpdateRelatedPanelscall gated onprojectForWebView.isEditable; E2E tests aligned to callopenScriptureEditorinstead ofopenResourceViewer— see Interview Notes)Mismatched meanings across locales for(deliberate: in the Spanish Simple UI the Comments and Commentaries ("Comentarios bíblicos") tabs sit side-by-side and must be clearly distinct — both cannot say "Comentarios"; in English the comments/commentaries distinction is judged self-explanatory. Final wording awaits UX research covering both upgrading P9 users and new P10 users. The floating comment list (power) and the fixed tab (simple) will soon never be visible simultaneously, so the same-locale duplicate-title concern does not apply.)%webView_legacyCommentManager_commentListPanel_title%(en "Comments" vs es "Comentarios de proyecto")fallbackKeyfor the new title key: users in languages other than en/es would see English "Comments" on the Column 3 panel even though the equivalent tab title is localized via Paratext fallback. (fixed during review: samefallbackKeyas%webView_legacyCommentManager_commentList_title%added. Author notes it is possible-but-unlikely this yields the same word for both tabs in some language — which underscores the need to eventually ensure good P10 localization.)Minor — Consider
focus-existingre-resolve edge case (platform-scripture-editor/src/main.ts): if the simple-mode re-resolve returnsfocus-existing(an editor for the same(projectId, isReadOnly)appeared concurrently — plausibly via the default project picker reacting to web-view-open events during dock rebuilds), the finalopenWebViewtreats it like open-new and creates a duplicate editordata-web-view-idemitted on non-WebView tabs (platform-tab-title.component.tsx): the attribute used the dock tab id for every tab, misleading for non-WebView tabs. (fixed during review: new optionalwebViewIdprop;platform-dock-tab.component.tsxpasses it only whentabInfo.tabType === TAB_TYPE_WEBVIEW, so the attribute is omitted elsewhere. E2E selectors unaffected — their targets are WebView tabs.)preConfigureSettingspermanently switched the developer's dev profile to English/simple mode while its JSDoc claimed state preservation. (fixed during review: it now snapshots the original file contents and returns a restore function;comment.fixture.tscalls it in worker teardown after the app closes. If the file did not exist, restore deletes it.)Duplicated defs→editors mapping in
platform-scripture-editor/src/main.ts(same comment +eslint-disabletwice) and the new resilience logic was not unit-testable. (fixed during review: extractedtoScriptureEditorInfos(+ScriptureEditorInfotype) intoplatform-scripture-editor.utils.ts; both call sites use it; 4 new unit tests cover filtering, mapping,isReadOnlydefaulting, and empty input — 89/89 tests in the file pass)Ternary in
comment-list-panel.utils.tsequivalent to a??chain. (fixed during review:pending ?? fromOptions ?? fromSaved; the 4 priority unit tests still pass)Naming inconsistency
COMMENT_LIST_PANEL_WEBVIEW_TYPEvs the file's existing camelCasecommentListWebViewType. (fixed during review: renamed tocommentListPanelWebViewType; neither the local Code-Style-Guide nor the paranext wiki style guide prescribes constant casing, so the file's pre-existing convention wins)Panel provider is an object literal with a module-level mutable sentinel while its in-file sibling uses the class-based(dismissed: the provider deliberately mirrors its functional siblings —WebViewFactorypattern ("state tracking needed → use class")modelTextPanelWebViewProviderand the resource text panel providers in platform-scripture-editor use exactly this object-literal + reload-sentinel shape. Flagged for review: which pattern should the reload-sentinel panels converge on?)Overlay-gone wait duplicated 4× across
helpers.tsandcomments-tab.spec.ts. (fixed during review: extractedwaitForOverlayGone(page, timeout)intohelpers.ts; the.pr-twp [role="status"]selector now has one home)Inline
import('@playwright/test').Pagerepeated 3× in the spec. (fixed during review: single top-levelimport type { Page })Sentence-case test asserts on translation content ((dismissed as-is; flagged for review: the suite's main purpose is ensuring uniqueness vs. the Commentaries tab, so this particular test is probably not really necessary — tweak or scrap it if a legitimate proper-noun localization ever arrives)localized-strings.test.ts): would fail on a legitimate future translation containing a proper noun/acronym.gitignorescope creep:/.claude/*.lockand/.claude/settings.local.jsonwere unrelated to PT-4068. (fixed during review: removed from this branch —.gitignorenow matches the merge base. Plan: include them in a separate housekeeping branch/PR together with the stashedstop-processes.mjschange, stash commitf5e77a08. Until then.claude/scheduled_tasks.lockshows as untracked locally and must not be committed.)waitForAppReadydefault timeout raised 60s→90s + new overlay wait — silently extends worst-case runtime for all E2E suites using it"WebView" → "web view" in error messages diverges from the design-guide terminology page(dismissed: deliberate — matches the prevailing "web view" wording used in the dozens of similar provider-mismatch messages across the codebase; changing all the others to "WebView" was judged not worth it)Template Propagation
Shared Regions Modified
None. (No changed file contains a
#region shared withmarker.)Extension Config Changes
extensions/src/legacy-comment-manager/contributions/localizedStrings.json— extension-specific content, no propagation neededlegacy-comment-manager/andplatform-scripture-editor/— no propagation needede2e-tests/playwright.config.ts,.gitignore— repo-root files, not extension template filesPositive Observations
.d.tsdeclaration, command registration metadata, and handler signature are all consistent, and both new registrations are added tocontext.registrationsfor cleanup.resolveCommentListPanelProjectIdextracted as a pure, well-documented utility with unit tests covering all four priority combinations; theexistingId: '?'/createNewIfNotFound: false/bringToFront: falseprobe matches the documentedOpenWebViewOptionscontract exactly.openCommentListPanelpassesbringToFront: falseon project-switch reloads so focus is not stolen from the Scripture editor; the panel reuses the existing comment list webview content and theming, keeping the Column 3 panel and standalone Comments tab consistent.papi.localization.getLocalizedString) — no hardcoded UI strings anywhere in the new code.data-web-view-idattribute instead of localized tab text — locale-independent, with the rc-tabs overflow-clipping behavior thoughtfully handled and documented.getAllOpenWebViewDefinitionstimeout fallbacks degrade gracefully with clear warnings rather than throwing, and each fallback's reasoning is documented inline.openOrUpdateRelatedPanelswraps the comments-panel command in try/catch matching the sibling panels, so a comments-panel failure cannot break editor opening.simple-layout.data.test.tswas updated in lockstep with the layout change, keeping the layout invariants enforced.e2e-tests/CLAUDE.md, per-directory READMEs,playwright.config.tscomment) codify conventions clearly, including warning away from the legacy directories.Interview Notes
fallbackKeywas accepted knowing the small cross-language collision risk.hasPendingflag; the fix was documentation, not behavior.openOrUpdateRelatedPanelsgating: the author's instinct that only the editable path should trigger related panels was verified as correct against the code; the gap pre-existed the branch (asopenTextConnectionPanels) but became user-visible with the Comments panel. Fixed with a one-line editability gate.openResourceViewerretry logic in the E2E spec was still needed, and suspected the comment tests shouldn't callopenResourceViewerdirectly. Analysis confirmed: the retry remains justified as defense-in-depth (the app-side re-check's timeout fallback can still throw), the PT-4069 test must go through the editor-open command (notopenCommentListPanel) because the integration under test is "active project change → Comments tab follows", and the tests were switched to the semantically correctopenScriptureEditor.waitForAppReady90s default) or measurably better reliability..claude.gitignoreentries removed from this branch should ship in a housekeeping branch/PR together with the stashedstop-processes.mjschange (stash commitf5e77a08).In-Review Quality Check
npm run typecheck— passed (only the known pre-existing, unrelatedPARAGRAPH_STRUCTURE_VIEW_MODEerror inplatform-scripture-editor.web-view.tsxfrom@eten-tech-foundation/platform-editor).npm run lint— passed, 0 errors (5 pre-existingno-consolewarnings in files untouched by this review).npm test— all review-related workspaces pass, including the newlocalized-strings.test.ts,comment-list-panel.utils.test.ts(4 tests), andplatform-scripture-editor.utils.test.ts(89 tests incl. 4 newtoScriptureEditorInfostests). The only failure was environmental:lib/platform-bible-react(untouched) could not launch its vitest browser mode because the local Playwright chromium-headless-shell binary was missing; it was installed machine-level for future runs.Suggested Review Focus
Prioritized areas for the author-reviewer meeting:
openOrUpdateRelatedPanels(platform-scripture-editor/src/main.tsopen()): simple-mode opens of read-only published resources no longer update the related panels. This is a behavior change slightly beyond the PR's comments scope (it also affects model text / Bible texts / Commentaries panels) — confirm it matches the intended simple-mode UX for opening resources.WebViewFactoryper the extension-patterns guidance — pick a direction for future panels.focus-existingre-resolve edge case: deferred with a ready-to-apply snippet (above) — decide whether to ticket it.Summary
legacyCommentManager.commentListPanelWebView provider andlegacyCommentManager.openCommentListPanelPAPI command inlegacy-comment-manageropenCommentListPanelfromopenOrUpdateRelatedPanels(renamed fromopenTextConnectionPanels) inplatform-scripture-editor%webView_legacyCommentManager_commentListPanel_title%(en: "Comments", es: "Comentarios de proyecto") so the panel tab and floating list can diverge independentlytests/isolated/for tab visibility, panel rendering, comments content, Spanish label, and project-tracking (PT-4069)Changes made during review (both sessions)
bringToFront: falseadded to theexistingId: '?'probe call inopenCommentListPanel— the omission caused the Comments panel to briefly steal focus on every project switchresolveCommentListPanelProjectIdintocomment-list-panel.utils.tswith 4 unit tests covering the sentinel priority chainopenTextConnectionPanelsrenamed toopenOrUpdateRelatedPanels"WebView"→"web view")tests/isolated/; READMEs added to all standard E2E directories;e2e-tests/CLAUDE.mdadded documenting test placement conventionsKnown limitations / reviewer notes
currentCommentListPanelProjectIdusesundefinedas both sentinel and valid value;openCommentListPanel(undefined)when a panel is open would silently keep the old project. Accepted: this path is never taken in current UX (Simple mode doesn't allow clearing a project). Mirrors the same pattern ascurrentModelTextProjectIdinplatform-scripture-editor.Test plan
comment-list-panel.utils.test.ts(4 tests, all pass)simple-layout.data.test.tsupdated (tab count 2→3)platform-scripture-editor.web-view.tsx)comment-assignmentisolated tests — not introduced by this PR)AI-assisted — session 1, session 2, session 3
This change is