Add interlinearizer project storage and createProject command#33
Open
alex-rawlings-yyc wants to merge 11 commits intomainfrom
Open
Add interlinearizer project storage and createProject command#33alex-rawlings-yyc wants to merge 11 commits intomainfrom
alex-rawlings-yyc wants to merge 11 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persistent Interlinearizer project CRUD, a new interlinearizer.createProject command, WebView menu/modal orchestration and three modal components, expanded Jest mocks (backend/frontend/react), parser tweaks, localization/menu entries, and comprehensive unit tests covering storage and UI flows. ChangesProject Creation & Storage
Mocks, Test Config & Miscellaneous
Sequence DiagramsequenceDiagram
participant User
participant Handler as createInterlinearProject
participant Storage as projectStorage
participant Backend as papi.storage
participant Notify as papi.notifications
User->>Handler: invoke createProject(sourceProjectId, analysisWritingSystem, name?, description?)
Handler->>Storage: createProject(token, sourceProjectId, analysisWritingSystem, name?, description?)
Storage->>Backend: readUserData(projectIds)
Backend-->>Storage: projectIds or ENOENT
Storage->>Backend: writeUserData(project:{id}, JSON)
Storage->>Backend: writeUserData(projectIds, JSON)
alt storage succeeds
Storage-->>Handler: created project id
Handler-->>User: project id
else storage fails
Handler->>Notify: send(error notification)
Handler-->>User: undefined
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Add `src/projectStorage.ts` with `createProject`, `getProject`, `listProjects`, and `deleteProject` backed by `papi.storage` - Register `interlinearizer.createProject` command in `main.ts` that prompts for source/target projects, writes the record, and surfaces storage errors as notifications - Add `InterlinearProject` type and `interlinearizer.createProject` signature to shared type declarations - Extend PAPI backend mock with `papi.storage` and `papi.notifications` - Broaden Jest coverage to all `src/**` files - Add full test suites for the storage module and the new command
alex-rawlings-yyc
force-pushed
the
create-interlinearizer-project
branch
from
e2e2f8a to
3da254f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main.ts`:
- Around line 209-213: The result schema for the createProject command deviates
from the established WebView convention by using { type: ['string', 'null'] }
instead of the conventionally used { type: 'string' } for commands that return
undefined on cancellation; update the result schema in the createProject
command's result object to { type: 'string' } to match openForWebView and other
WebView-opening commands (or alternatively add a concise comment next to the
createProject result explaining why null is intentionally used if you truly need
null serialization), ensuring you edit the result object associated with
createProject and keep the summary text describing that undefined is returned on
cancel consistent.
In `@src/projectStorage.ts`:
- Around line 131-136: deleteProject currently calls papi.storage.deleteUserData
unconditionally and will throw if the project file is missing; wrap the
deleteUserData call in a try/catch inside deleteProject (referencing
deleteProject, papi.storage.deleteUserData, and projectKey) and swallow only
ENOENT errors (i.e., if err.code === 'ENOENT' treat as no-op) while rethrowing
other errors, then continue to readIds and write the updated PROJECT_IDS_KEY
index; also add the provided test to src/__tests__/projectStorage.test.ts to
assert index cleanup when deleteUserData rejects with ENOENT.
In `@src/types/interlinearizer.d.ts`:
- Around line 15-23: Update the JSDoc for the 'interlinearizer.createProject'
declaration to state that the Promise may resolve to undefined not only when the
user cancels either picker but also if creating the project in storage fails
(e.g., when projectStorage.createProject throws and the implementation logs an
error/sends a notification in main.ts), so callers know undefined can indicate a
storage failure as well as a user cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 602669d3-ae6c-442a-9329-6a0a9f86bf08
📒 Files selected for processing (6)
__mocks__/papi-backend.tssrc/__tests__/main.test.tssrc/__tests__/projectStorage.test.tssrc/main.tssrc/projectStorage.tssrc/types/interlinearizer.d.ts
…ect and add associated test, fix schema inconsistency
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 3 discussions.
Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 9 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/projectStorage.test.ts`:
- Around line 36-40: The test setup fails to reset __mockReadUserData and leaves
crypto.randomUUID permanently spied-on; update the tests to call
jest.resetAllMocks() at the start of beforeEach (or explicitly reset
__mockReadUserData) so any once-queues are cleared before re-establishing
default mockResolvedValue, and add an afterEach that calls
jest.restoreAllMocks() to unwrap the spy on crypto.randomUUID; locate the setup
in the test file where __mockWriteUserData, __mockDeleteUserData and
jest.spyOn(crypto, 'randomUUID') are referenced and apply these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb6b9254-c4f1-4b22-a279-06c6ef431f4f
📒 Files selected for processing (5)
src/__tests__/projectStorage.test.tssrc/main.tssrc/parsers/pt9/interlinearXmlParser.tssrc/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (1)
- src/parsers/pt9/interlinearXmlParser.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/types/interlinearizer.d.ts
- src/projectStorage.ts
- src/main.ts
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status:
…er index/data write to `papi` storage to ensure data doesn't become inaccessible
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main.ts`:
- Around line 211-215: Update the result schema summary for
createInterlinearProject to reflect both cancellation and storage failures:
change the text in the result object (the "result" entry that currently reads
"The UUID of the new project, or undefined if the user cancelled") to something
like "The UUID of the new project, or undefined if the user cancelled or if
projectStorage.createProject failed" so JSON-RPC consumers know undefined can
mean a storage failure as well as user cancellation.
- Around line 120-130: Ensure the source and target projects chosen via
papi.dialogs.selectProject are distinct: after obtaining sourceProjectId and
before/after prompting for targetProjectId, validate that the newly selected
targetProjectId !== sourceProjectId and if equal re-prompt the user (or show an
error and re-open the selectProject dialog) until a different project is chosen;
update any user-facing error text by adding a localization key (e.g., for
interlinearizer_error_same_project) and use it when rejecting identical
selections so the InterlinearProject never gets created with sourceProjectId ===
targetProjectId (this also avoids confusing entries in listProjects).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b872de8-61ff-46d2-b985-f3d814186f84
📒 Files selected for processing (3)
jest.config.tssrc/main.tssrc/projectStorage.ts
…update mocks to match real API signatures - If the user picks the same project for both source and target roles, a warning notification is shown and the target picker re-opens until distinct projects are chosen or cancelled. - Includes new tests for this flow, updated platform-bible-react/utils mocks to match current API shapes, a new localized error string, and minor test/parser cleanups.
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 9 files and all commit messages.
Reviewable status:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main.ts (1)
115-127: 💤 Low valueOptional: convert recursion to a loop.
promptForDistinctTargetProjectrecurses on every same-project mistake. JavaScript doesn't optimize tail calls, so the call stack grows with each retry. In practice this is fine, but awhileloop expresses the "re-prompt until valid" intent more directly and avoids any stack growth concern.♻️ Suggested loop form
async function promptForDistinctTargetProject( sourceProjectId: string, ): Promise<string | undefined> { - const targetProjectId = await papi.dialogs.selectProject({ - title: '%interlinearizer_dialog_create_target_title%', - prompt: '%interlinearizer_dialog_create_target_prompt%', - }); - if (!targetProjectId || targetProjectId !== sourceProjectId) return targetProjectId; - await papi.notifications - .send({ message: '%interlinearizer_error_same_project%', severity: 'warning' }) - .catch(() => {}); - return promptForDistinctTargetProject(sourceProjectId); + while (true) { + const targetProjectId = await papi.dialogs.selectProject({ + title: '%interlinearizer_dialog_create_target_title%', + prompt: '%interlinearizer_dialog_create_target_prompt%', + }); + if (!targetProjectId || targetProjectId !== sourceProjectId) return targetProjectId; + await papi.notifications + .send({ message: '%interlinearizer_error_same_project%', severity: 'warning' }) + .catch(() => {}); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.ts` around lines 115 - 127, The function promptForDistinctTargetProject currently uses recursion to re-prompt on same-project selection; replace the recursive pattern with an explicit loop: repeatedly call papi.dialogs.selectProject inside a while loop, return the selected targetProjectId immediately if it's falsy or different from sourceProjectId, otherwise await papi.notifications.send(...) and continue the loop; remove the recursive call to promptForDistinctTargetProject to avoid stack growth.__mocks__/platform-bible-react.tsx (1)
37-65: 💤 Low valueMock prop typing is inconsistent with usage.
onSelectProjectMenuItemandonSelectViewInfoMenuItemare typed as required (no?), but the JSX renders them conditionally withonSelectProjectMenuItem && (…)/onSelectViewInfoMenuItem && (…). Real consumers likesrc/interlinearizer.web-view.tsx(per the provided snippet) don't pass these callbacks at all, so the conditional rendering is the correct behavior — the type should reflect that they're optional.♻️ Make the callbacks optional
- onSelectProjectMenuItem: (selectedMenuItem: unknown) => void; - onSelectViewInfoMenuItem: (selectedMenuItem: unknown) => void; + onSelectProjectMenuItem?: (selectedMenuItem: unknown) => void; + onSelectViewInfoMenuItem?: (selectedMenuItem: unknown) => void;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__mocks__/platform-bible-react.tsx` around lines 37 - 65, The prop types for onSelectProjectMenuItem and onSelectViewInfoMenuItem are declared as required but the component conditionally renders based on their existence; update the prop definitions to make these two props optional (add ? to onSelectProjectMenuItem and onSelectViewInfoMenuItem in the component's props type) and ensure their callback signature allows being called with undefined (e.g., accept unknown | undefined) so the onClick handlers that call onSelectProjectMenuItem(undefined) / onSelectViewInfoMenuItem(undefined) remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contributions/localizedStrings.json`:
- Around line 4-9: Add the missing localization keys referenced by src/main.ts
so UI dialogs don't show raw placeholders: add entries for
"%interlinearizer_dialog_create_source_title%",
"%interlinearizer_dialog_create_source_prompt%",
"%interlinearizer_dialog_create_target_title%",
"%interlinearizer_dialog_create_target_prompt%", and
"%interlinearizer_error_create_project_failed%" to the existing "en" locale
block (use wording consistent with existing keys like
"%interlinearizer_dialog_open_title%" and
"%interlinearizer_error_same_project%"); ensure each new key has an appropriate
human-friendly string for the createProject flow and follow the same JSON
structure and comma placement as surrounding entries.
In `@src/__tests__/interlinearizer.web-view.test.tsx`:
- Around line 358-378: The two tests named "logs with 'und' writing system when
tokenization fails …" claim to verify that the fallback writing system 'und' is
used but only assert the error heading; update each test to either (A) tighten
the assertion by spying on logger.error from `@papi/frontend` and assert it was
called with a message or metadata containing the writing system 'und' when
tokenizeBook throws (locate the tests using InterlinearizerWebView and the
mocked tokenizeBook), or (B) if you prefer not to inspect logs, rename the tests
to reflect that they only assert the error UI is shown; for option A import
logger, create a jest.spyOn(logger, 'error'), trigger the failing tokenizeBook
in the existing tests, and assert the spy was called with a payload including
'und'.
---
Nitpick comments:
In `@__mocks__/platform-bible-react.tsx`:
- Around line 37-65: The prop types for onSelectProjectMenuItem and
onSelectViewInfoMenuItem are declared as required but the component
conditionally renders based on their existence; update the prop definitions to
make these two props optional (add ? to onSelectProjectMenuItem and
onSelectViewInfoMenuItem in the component's props type) and ensure their
callback signature allows being called with undefined (e.g., accept unknown |
undefined) so the onClick handlers that call onSelectProjectMenuItem(undefined)
/ onSelectViewInfoMenuItem(undefined) remain valid.
In `@src/main.ts`:
- Around line 115-127: The function promptForDistinctTargetProject currently
uses recursion to re-prompt on same-project selection; replace the recursive
pattern with an explicit loop: repeatedly call papi.dialogs.selectProject inside
a while loop, return the selected targetProjectId immediately if it's falsy or
different from sourceProjectId, otherwise await papi.notifications.send(...) and
continue the loop; remove the recursive call to promptForDistinctTargetProject
to avoid stack growth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bd5c686-a23c-48ca-b23d-88481c6ae8c8
📒 Files selected for processing (8)
__mocks__/platform-bible-react.tsx__mocks__/platform-bible-utils.tscontributions/localizedStrings.jsonsrc/__tests__/interlinearizer.web-view.test.tsxsrc/__tests__/main.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/main.tssrc/parsers/pt9/interlinearXmlParser.ts
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 3 files and all commit messages.
Reviewable status:
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 1 file and all commit messages.
Reviewable status:
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main.ts (1)
56-60: 💤 Low valueConsider a definite-assignment assertion on
executionToken.
let executionToken: ExecutionToken;types reads asExecutionToken, but TypeScript does not actually verify the variable is assigned before use. If a future refactor lets a command handler run beforeactivate()completes (or after deactivation if you decide to clear it), reads would silently seeundefined. Either narrow the type or signal definite assignment:-let executionToken: ExecutionToken; +let executionToken!: ExecutionToken;Or
let executionToken: ExecutionToken | undefined;with an explicit guard insidecreateInterlinearProjectfor stronger safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.ts` around lines 56 - 60, The module-level variable executionToken is declared without a definite-assignment guarantee, which can allow undefined reads if command handlers run before activate() or after deactivation; change the declaration to either use a definite-assignment assertion (let executionToken!: ExecutionToken) or make it nullable (let executionToken: ExecutionToken | undefined) and then add an explicit runtime guard at the call site—specifically inside createInterlinearProject and any command handlers that reference executionToken—to throw or return early when executionToken is undefined, and ensure activate() still assigns it and deactivate() clears it consistently.src/__tests__/interlinearizer.web-view.test.tsx (1)
359-391: 💤 Low valueRestore the
logger.errorspy to prevent leaking into later tests.
jest.spyOn(logger, 'error').mockImplementation(() => {})is created inside each test but never restored. The mock implementation persists for the rest of the worker, silencing any subsequent test that exerciseslogger.error(or that you add later). AddmockRestore()(or a globalafterEach(() => jest.restoreAllMocks())) so the spy is scoped to the test that creates it.♻️ Proposed scoped restore
it('logs with "und" writing system when tokenization fails and writing system is a PlatformError', () => { const loggerErrorSpy = jest.spyOn(logger, 'error').mockImplementation(() => {}); mockBookData({}); mockWritingSystem({ platformErrorVersion: 1, message: 'Setting unavailable' }); jest.mocked(tokenizeBook).mockImplementation(() => { throw new Error('parse failure'); }); render(<InterlinearizerWebView {...makeProps(testProjectId)} />); expect(screen.getByRole('heading', { name: /error processing book/i })).toBeInTheDocument(); expect(loggerErrorSpy).toHaveBeenCalledWith( expect.any(String), expect.anything(), expect.objectContaining({ writingSystem: 'und' }), ); + loggerErrorSpy.mockRestore(); });Apply the same change to the empty-string variant, or add a single
afterEach(() => jest.restoreAllMocks())to thedescribeblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/interlinearizer.web-view.test.tsx` around lines 359 - 391, The tests create a spy via jest.spyOn(logger, 'error').mockImplementation(() => {}) in the two it blocks ('logs with "und" writing system when tokenization fails...') but never restore it, leaking the mock to later tests; fix by restoring the spy after each test that creates it (call mockRestore() on the loggerErrorSpy at the end of each test) or add a scoped cleanup like afterEach(() => jest.restoreAllMocks()) in the describe containing these tests so the logger.error mock does not persist beyond the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/__tests__/interlinearizer.web-view.test.tsx`:
- Around line 359-391: The tests create a spy via jest.spyOn(logger,
'error').mockImplementation(() => {}) in the two it blocks ('logs with "und"
writing system when tokenization fails...') but never restore it, leaking the
mock to later tests; fix by restoring the spy after each test that creates it
(call mockRestore() on the loggerErrorSpy at the end of each test) or add a
scoped cleanup like afterEach(() => jest.restoreAllMocks()) in the describe
containing these tests so the logger.error mock does not persist beyond the
test.
In `@src/main.ts`:
- Around line 56-60: The module-level variable executionToken is declared
without a definite-assignment guarantee, which can allow undefined reads if
command handlers run before activate() or after deactivation; change the
declaration to either use a definite-assignment assertion (let executionToken!:
ExecutionToken) or make it nullable (let executionToken: ExecutionToken |
undefined) and then add an explicit runtime guard at the call site—specifically
inside createInterlinearProject and any command handlers that reference
executionToken—to throw or return early when executionToken is undefined, and
ensure activate() still assigns it and deactivate() clears it consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d134961c-f2b1-42cc-a7d3-086903328e5d
📒 Files selected for processing (4)
contributions/localizedStrings.jsonsrc/__tests__/interlinearizer.web-view.test.tsxsrc/main.tssrc/types/interlinearizer.d.ts
imnasnainaec
reviewed
May 6, 2026
Contributor
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec partially reviewed 7 files and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
src/main.ts line 151 at r7 (raw file):
const targetProjectId = await promptForDistinctTargetProject(sourceProjectId); if (!targetProjectId) return undefined;
This seems restrictive. Shouldn't it be optional to have a target project? We definitely want to support that, but I thought we'd want users to be able to start using the interlinearizer without having to first create an "output" project for it.
Introduces three modal components (CreateProjectModal, ProjectMetadataModal, SelectInterlinearProjectModal) with supporting menu contributions, localized strings, command registrations in main.ts, storage helpers in projectStorage.ts, and updated type definitions. Adds full test coverage for all new components and expands existing test suites to cover the new flows.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/SelectInterlinearProjectModal.tsx (1)
108-158: ⚡ Quick winAdd ARIA dialog role,
aria-modal,aria-labelledby, and an Escape-key handler.The modal overlay is an unsemantic
<div>. Screen readers won't announce it as a dialog, and the virtual cursor isn't constrained to it. The standard ARIA dialog pattern requires:
role="dialog"andaria-modal="true"on the container so AT restricts navigation.- A stable
idon the<h2>and a matchingaria-labelledbyso the dialog has an accessible name.- Closing on Escape (standard keyboard expectation for dialogs).
♿ Proposed ARIA additions
+const TITLE_ID = 'select-interlinear-project-title'; ... -<div className="tw-fixed tw-inset-0 tw-z-50 tw-flex tw-items-center tw-justify-center tw-bg-black/40"> - <div className="tw-bg-background tw-rounded-lg tw-border tw-border-border tw-p-6 tw-w-[32rem] tw-shadow-lg"> - <h2 className="tw-text-base tw-font-semibold tw-mb-4"> +<div + className="tw-fixed tw-inset-0 tw-z-50 tw-flex tw-items-center tw-justify-center tw-bg-black/40" + role="dialog" + aria-modal="true" + aria-labelledby={TITLE_ID} + onKeyDown={(e) => { if (e.key === 'Escape') onClose(); }} +> + <div className="tw-bg-background tw-rounded-lg tw-border tw-border-border tw-p-6 tw-w-[32rem] tw-shadow-lg"> + <h2 id={TITLE_ID} className="tw-text-base tw-font-semibold tw-mb-4">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/SelectInterlinearProjectModal.tsx` around lines 108 - 158, The modal in SelectInterlinearProjectModal lacks ARIA dialog attributes and Escape-key handling; add role="dialog" and aria-modal="true" to the outer container div, give the <h2> a stable id (e.g., modalTitleId) and add aria-labelledby referencing that id so the dialog has an accessible name, and implement an Escape-key handler using useEffect inside the SelectInterlinearProjectModal component that listens for keydown (Escape) and calls the existing onClose prop, with proper cleanup on unmount; keep existing handlers (handleSelect, handleViewInfo, onCreateNew) unchanged.src/interlinearizer.web-view.tsx (1)
450-456: 💤 Low value
makeHandleProjectCreated(projectId)returns a new function on every render.Calling the factory inline in JSX produces a fresh
onProjectCreatedreference each render, which invalidatesCreateProjectModal'shandleSubmituseCallbackdependency. Memoizing once for the currentprojectIdavoids that churn:♻️ Proposed fix
- {modal === 'create' && projectId && ( - <CreateProjectModal - projectId={projectId} - onClose={() => setModal('none')} - onProjectCreated={makeHandleProjectCreated(projectId)} - /> - )} + {modal === 'create' && projectId && ( + <CreateProjectModalForSource + projectId={projectId} + onClose={() => setModal('none')} + makeHandleProjectCreated={makeHandleProjectCreated} + /> + )}…or simply hoist the memoized handler:
+ const handleProjectCreated = useMemo( + () => (projectId ? makeHandleProjectCreated(projectId) : undefined), + [makeHandleProjectCreated, projectId], + ); ... - onProjectCreated={makeHandleProjectCreated(projectId)} + onProjectCreated={handleProjectCreated}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interlinearizer.web-view.tsx` around lines 450 - 456, The inline call to makeHandleProjectCreated(projectId) inside the JSX creates a new function each render, causing CreateProjectModal's onProjectCreated prop to change and invalidating its internal useCallback (e.g., handleSubmit); fix by memoizing the handler for the current projectId (e.g., useMemo or useCallback at component scope) and pass that stable reference to onProjectCreated, ensuring you compute the memoizedHandler when projectId changes and still call setModal('none') in the handler as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/CreateProjectModal.tsx`:
- Around line 53-70: The handleSubmit in CreateProjectModal calls onClose() even
when papi.commands.sendCommand('interlinearizer.createProject', ...) returns
undefined (storage errors are swallowed in main.ts), which causes the modal to
close and discard user input; change the logic in handleSubmit so that onClose()
is only invoked when a valid newId is returned (i.e., after the
onProjectCreated?.(newId, analysisLanguage) branch), and instead handle the
undefined case by keeping the modal open and surfacing an appropriate
notification or error state to the user; refer to handleSubmit,
onProjectCreated, onClose, and the interlinearizer.createProject call to locate
where to gate the dismiss.
In `@src/components/ProjectMetadataModal.tsx`:
- Around line 88-103: handleSave currently sends and propagates the raw
editLanguage (which may contain surrounding whitespace) even though the Save
button uses editLanguage.trim() to gate submission; trim editLanguage once
before persisting and passing to callbacks to avoid leaking whitespace. Update
handleSave to compute a trimmedLanguage (e.g. from editLanguage?.trim()) and use
that value when calling
papi.commands.sendCommand('interlinearizer.updateProjectMetadata', ...) and
onProjectSaved(...), ensuring you still coerce empty strings to undefined as
done for newName/newDescription.
---
Nitpick comments:
In `@src/components/SelectInterlinearProjectModal.tsx`:
- Around line 108-158: The modal in SelectInterlinearProjectModal lacks ARIA
dialog attributes and Escape-key handling; add role="dialog" and
aria-modal="true" to the outer container div, give the <h2> a stable id (e.g.,
modalTitleId) and add aria-labelledby referencing that id so the dialog has an
accessible name, and implement an Escape-key handler using useEffect inside the
SelectInterlinearProjectModal component that listens for keydown (Escape) and
calls the existing onClose prop, with proper cleanup on unmount; keep existing
handlers (handleSelect, handleViewInfo, onCreateNew) unchanged.
In `@src/interlinearizer.web-view.tsx`:
- Around line 450-456: The inline call to makeHandleProjectCreated(projectId)
inside the JSX creates a new function each render, causing CreateProjectModal's
onProjectCreated prop to change and invalidating its internal useCallback (e.g.,
handleSubmit); fix by memoizing the handler for the current projectId (e.g.,
useMemo or useCallback at component scope) and pass that stable reference to
onProjectCreated, ensuring you compute the memoizedHandler when projectId
changes and still call setModal('none') in the handler as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b260117-e9da-4fc9-8fa6-d76d6f3c6c07
📒 Files selected for processing (21)
__mocks__/lucide-react.tsx__mocks__/papi-frontend-react.ts__mocks__/papi-frontend.ts__mocks__/platform-bible-react.tsxcontributions/localizedStrings.jsoncontributions/menus.jsoncspell.jsonjest.config.tssrc/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/components/ProjectMetadataModal.test.tsxsrc/__tests__/components/SelectInterlinearProjectModal.test.tsxsrc/__tests__/interlinearizer.web-view.test.tsxsrc/__tests__/main.test.tssrc/__tests__/projectStorage.test.tssrc/components/CreateProjectModal.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (3)
- cspell.json
- contributions/localizedStrings.json
- src/tests/components/CreateProjectModal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tests/interlinearizer.web-view.test.tsx
- src/main.ts
- src/types/interlinearizer.d.ts
- mocks/platform-bible-react.tsx
…nput - `CreateProjectModal`: guard `onClose()` behind a `!newId` early-return so the modal doesn't close when project creation returns no ID - `ProjectMetadataModal`: trim whitespace from the language field before saving and passing to callbacks - Replace `makeHandleProjectCreated` factory with a plain `handleProjectCreated` callback; removes the closure-over-srcId pattern and reads `projectId` from the enclosing scope directly
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 21 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
src/main.ts line 151 at r7 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
This seems restrictive. Shouldn't it be optional to have a target project? We definitely want to support that, but I thought we'd want users to be able to start using the interlinearizer without having to first create an "output" project for it.
Correct. Addressed in latest commit
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 5 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/SelectInterlinearProjectModal.tsx (1)
64-74: ⚡ Quick winValidate required fields when filtering parsed projects.
The current type predicate accepts any non-null object. If
interlinearizer.getProjectsForSourceever returns entries lacking required fields (e.g. due to a future schema change or storage corruption), they’ll pass the filter and cause subtler failures downstream —key={project.id}withundefined,onSelect/onViewInforeceiving objects whoseidis missing, etc. Tightening the predicate fails fast and keeps malformed entries out of state.🛡️ Proposed fix
- if (Array.isArray(parsed)) - setProjects( - parsed.filter((p): p is InterlinearProjectSummary => !!p && typeof p === 'object'), - ); + if (Array.isArray(parsed)) + setProjects( + parsed.filter((p): p is InterlinearProjectSummary => { + if (!p || typeof p !== 'object') return false; + const o = p as Record<string, unknown>; + return ( + typeof o.id === 'string' && + typeof o.createdAt === 'string' && + typeof o.sourceProjectId === 'string' && + typeof o.analysisWritingSystem === 'string' + ); + }), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/SelectInterlinearProjectModal.tsx` around lines 64 - 74, The JSON parsing/filter step currently treats any non-null object as an InterlinearProjectSummary; tighten the type guard used when calling setProjects so you explicitly check for required fields (e.g. typeof p === 'object' && p !== null && typeof (p as any).id === 'string' && typeof (p as any).name === 'string' and any other required props per InterlinearProjectSummary) before narrowing; update the predicate around parsed.filter(...) (the block that sets projects after interlinearizer.getProjectsForSource) to return only objects that satisfy those checks so downstream uses like key={project.id}, onSelect and onViewInfo always receive well-formed projects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/components/CreateProjectModal.test.tsx`:
- Around line 131-134: The test asserts onProjectCreated immediately after
awaiting userEvent.click; mirror the onClose test by wrapping the
expect(onProjectCreated).toHaveBeenCalledWith('new-project-id', 'en') inside a
waitFor to ensure promise continuations from sendCommand are flushed. Locate the
assertion using the onProjectCreated mock and userEvent.click call in
CreateProjectModal.test.tsx and replace the direct expect with a waitFor-based
expect to make the test consistent and stable.
In `@src/interlinearizer.web-view.tsx`:
- Around line 311-321: handleProjectCreated currently only receives
(interlinearProjectId, analysisWritingSystem) and fabricates createdAt while
dropping name/description; fix by making createProject/its command handler
return the full persisted Project object instead of just the id, update
CreateProjectModal to pass that returned Project into onProjectCreated, and
change handleProjectCreated to accept the Project (use its id, name,
description, createdAt and analysisWritingSystem) and call setActiveProject with
the exact persisted object (no local new Date()). Update related call sites of
interlinearizer.createProject and the command handler to return the full project
shape so UI and metadata modal see the authoritative createdAt, name and
description.
In `@src/main.ts`:
- Around line 154-184: Wrap the bodies of deleteInterlinearProject and
updateProjectMetadata in try/catch blocks mirroring
createInterlinearProject/getProjectsForSource: on error catch the thrown storage
error from projectStorage.deleteProject or projectStorage.updateProjectMetadata,
log it with the same logger used elsewhere (e.g. processLogger.error) including
contextual text and the error, send a user-facing notification using the same
WebView/user-notification mechanism used by the other functions, and return a
safe fallback (swallow the error for deleteInterlinearProject so it doesn't
reject the WebView caller; for updateProjectMetadata return undefined on
failure).
- Around line 314-325: Add a no-op registration for the
interlinearizer.viewProjectInfo command analogous to the existing
interlinearizer.newProject registration: call
papi.commands.registerCommand('interlinearizer.viewProjectInfo', async () => {},
{...}) in the same initialization area (same pattern as
newProjectCommandRegistration) and provide method metadata (summary, params: [],
result schema void/null) so the platform recognizes the command referenced by
contributions/menus.json; ensure the variable name and registration are
consistent with the existing newProjectCommandRegistration usage.
---
Nitpick comments:
In `@src/components/SelectInterlinearProjectModal.tsx`:
- Around line 64-74: The JSON parsing/filter step currently treats any non-null
object as an InterlinearProjectSummary; tighten the type guard used when calling
setProjects so you explicitly check for required fields (e.g. typeof p ===
'object' && p !== null && typeof (p as any).id === 'string' && typeof (p as
any).name === 'string' and any other required props per
InterlinearProjectSummary) before narrowing; update the predicate around
parsed.filter(...) (the block that sets projects after
interlinearizer.getProjectsForSource) to return only objects that satisfy those
checks so downstream uses like key={project.id}, onSelect and onViewInfo always
receive well-formed projects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cddc4664-6910-4300-aa98-d747b1dc5377
📒 Files selected for processing (21)
__mocks__/lucide-react.tsx__mocks__/papi-frontend-react.ts__mocks__/papi-frontend.ts__mocks__/platform-bible-react.tsxcontributions/localizedStrings.jsoncontributions/menus.jsoncspell.jsonjest.config.tssrc/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/components/ProjectMetadataModal.test.tsxsrc/__tests__/components/SelectInterlinearProjectModal.test.tsxsrc/__tests__/interlinearizer.web-view.test.tsxsrc/__tests__/main.test.tssrc/__tests__/projectStorage.test.tssrc/components/CreateProjectModal.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (3)
- cspell.json
- contributions/localizedStrings.json
- src/components/CreateProjectModal.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tests/interlinearizer.web-view.test.tsx
- mocks/platform-bible-react.tsx
- src/tests/projectStorage.test.ts
…r delete/update - `interlinearizer.createProject` now returns the full persisted project as a JSON string instead of just the UUID, so the WebView can populate `activeProject` with authoritative server data rather than reconstructing it locally - `CreateProjectModal.onProjectCreated` callback now receives the parsed `InterlinearProjectSummary` object instead of `(id, writingSystem)` pair - Add `isInterlinearProjectSummary` type guard to `SelectInterlinearProjectModal` and reuse it in the project list filter and the new create flow - Wrap `deleteProject` and `updateProjectMetadata` backend handlers in try/catch with logging and error notifications (previously unhandled rejections) - Register a no-op `interlinearizer.viewProjectInfo` backend command so the platform menu system can surface it; all logic runs in the WebView - Update tests and type declarations to match
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 4 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 9 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change is
Summary by CodeRabbit
New Features
User Notifications
Localization
Tests