Add interlinearizer project storage and createProject command#33
Open
alex-rawlings-yyc wants to merge 33 commits into
Open
Add interlinearizer project storage and createProject command#33alex-rawlings-yyc wants to merge 33 commits into
alex-rawlings-yyc wants to merge 33 commits into
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 InterlinearProject types, a projectStorage service with CRUD, backend JSON-RPC handlers, WebView state and modal UI (create/select/metadata), extended Jest mocks/config, parser/JSDoc/localization updates, and tests covering storage, handlers, and UI flows. ChangesInterlinearizer project persistence and WebView UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
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
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:
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
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.
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
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
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).
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/components/ProjectMetadataModal.tsx`:
- Around line 138-142: The modal rendered by ProjectMetadataModal is missing
dialog semantics; update the outer modal container (the top-level div wrapping
the panel) to include role="dialog" and aria-modal="true", give the heading
element inside ProjectMetadataModal (the h2 showing
localizedStrings['%interlinearizer_modal_metadata_title%']) a stable id (e.g.,
modal-title) and add aria-labelledby="that-id" to the dialog container (or
provide an aria-label if you prefer a label string). Ensure the referenced id is
unique within the component so screen readers can announce the dialog title.
- Around line 95-107: The call to
papi.commands.sendCommand('interlinearizer.updateProjectMetadata', ...) can
resolve to undefined but the code always invokes onProjectSaved and onClose;
update the implementation that calls papi.commands.sendCommand (using
identifiers interlinearProjectId, newName, newDescription, newLanguage) to
capture the response value and only invoke onProjectSaved (with {name: newName,
description: newDescription, analysisWritingSystem: newLanguage}) and onClose
when the response indicates success (i.e., is not undefined/falsey); otherwise,
handle the failure path (e.g., show an error or return early) so the UI does not
show a false-success state.
In `@src/types/interlinearizer.d.ts`:
- Around line 728-731: Replace the invalid Flow-style union syntax "('| ... )"
with a standard TypeScript union in each of the three offending type
definitions: remove the leading "(|" and trailing ")" so the union reads "{
gloss: MultiString; glossSenseRef?: never } | { glossSenseRef: SenseRef; gloss?:
never } | { gloss?: never; glossSenseRef?: never }" (apply the same change to
the other two identical union expressions reported at lines 875 and 966). Ensure
you only remove the Flow-specific "(| ... )" wrapper and keep the member object
types, their property names (e.g., gloss, glossSenseRef) and referenced types
(MultiString, SenseRef) unchanged.
🪄 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: 09cd8788-83c0-4e81-8d41-000c1439bc00
📒 Files selected for processing (9)
src/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/interlinearizer.web-view.test.tsxsrc/__tests__/main.test.tssrc/components/CreateProjectModal.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/types/interlinearizer.d.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/CreateProjectModal.tsx
- src/components/SelectInterlinearProjectModal.tsx
- src/tests/interlinearizer.web-view.test.tsx
- src/interlinearizer.web-view.tsx
- src/tests/main.test.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 6 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
…t was still created", update docs
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
alex-rawlings-yyc
force-pushed
the
create-interlinearizer-project
branch
from
253c32e to
f5083ae
Compare
Comment on lines
+49
to
+65
| function deleteDirectory(dirPath) { | ||
| if (!dirPath) { | ||
| console.log('⊘ Skipping empty path'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| if (fs.existsSync(dirPath)) { | ||
| fs.rmSync(dirPath, { recursive: true, force: true }); | ||
| console.log(`✓ Deleted ${dirPath}`); | ||
| } else { | ||
| console.log(`⊘ ${dirPath} does not exist`); | ||
| } | ||
| } catch (error) { | ||
| console.error(`✗ Failed to delete ${dirPath}:`, error.message); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
@imnasnainaec Is this a change you want to make in main quickly or shall we address it here?
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 8 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
…ations on error, disable metadata modal buttons when submitting
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc partially reviewed 7 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/CreateProjectModal.tsx (1)
59-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an in-handler re-entrancy guard for create submission.
interlinearizer.createProjectis non-idempotent, and button disabling alone can still allow rapid re-entry before state commits. GuardhandleSubmititself to prevent duplicate creates.Suggested patch
const handleSubmit = useCallback(async () => { + if (isSubmitting) return; setIsSubmitting(true); const normalizedAnalysisLanguage = analysisLanguage.trim() || 'und'; try { const projectJson = await papi.commands.sendCommand( 'interlinearizer.createProject', @@ } finally { setIsSubmitting(false); } - }, [projectId, analysisLanguage, name, description, onClose, onProjectCreated]); + }, [projectId, analysisLanguage, name, description, onClose, onProjectCreated, isSubmitting]);🤖 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/CreateProjectModal.tsx` around lines 59 - 81, The handleSubmit function can be re-entered before React state disables the button; add an in-handler reentrancy guard by introducing a stable flag (e.g., isSubmittingRef via useRef) that is checked at the top of handleSubmit and set to true immediately when starting, then cleared in the finally block alongside setIsSubmitting(false); update handleSubmit to return early if the ref is already true, leaving the existing try/catch/finally intact and keeping setIsSubmitting for UI state, and reference the new isSubmittingRef in the dependency-free stable guard so handleSubmit’s dependency array remains correct.
🤖 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__/services/projectStorage.test.ts`:
- Around line 450-455: The test's mockImplementation for __mockWriteUserData
declares an unused third parameter (_v) which triggers
`@typescript-eslint/no-unused-vars`; fix it by either (A) matching the original
signature but marking the parameter as intentionally unused via rest syntax
(e.g., replace the third param with a rest param so only key is named) in the
anonymous function passed to __mockWriteUserData.mockImplementation, or (B) if
the exact signature must remain, add a one-line ESLint disable comment for
`@typescript-eslint/no-unused-vars` immediately above the mockImplementation
callback to silence the rule for this line; update the mockImplementation call
site accordingly.
In `@src/components/ProjectMetadataModal.tsx`:
- Around line 126-132: handleDelete calls onProjectDeleted and onClose
regardless of the resolved value from
papi.commands.sendCommand('interlinearizer.deleteProject',
interlinearProjectId); change it to gate the success path on an explicit truthy
success result (like the save-path guard): await the command, check the returned
value (e.g., const result = await papi.commands.sendCommand(...)); only call
onProjectDeleted?.(interlinearProjectId) and onClose() if result is truthy;
otherwise handle the failure path (leave modal open, setIsSubmitting(false), and
surface an error) so falsy backend resolutions don't report a successful delete.
---
Duplicate comments:
In `@src/components/CreateProjectModal.tsx`:
- Around line 59-81: The handleSubmit function can be re-entered before React
state disables the button; add an in-handler reentrancy guard by introducing a
stable flag (e.g., isSubmittingRef via useRef) that is checked at the top of
handleSubmit and set to true immediately when starting, then cleared in the
finally block alongside setIsSubmitting(false); update handleSubmit to return
early if the ref is already true, leaving the existing try/catch/finally intact
and keeping setIsSubmitting for UI state, and reference the new isSubmittingRef
in the dependency-free stable guard so handleSubmit’s dependency array remains
correct.
🪄 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: 64913279-3b87-4bbd-95c7-6c42e0fb6452
📒 Files selected for processing (30)
__mocks__/lucide-react.tsx__mocks__/papi-backend.ts__mocks__/papi-frontend-react.ts__mocks__/papi-frontend.ts__mocks__/platform-bible-react.tsx__mocks__/platform-bible-utils.tscontributions/localizedStrings.jsoncontributions/menus.jsoncspell.jsonjest.config.tssrc/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/components/InterlinearizerLoader.test.tsxsrc/__tests__/components/ProjectMetadataModal.test.tsxsrc/__tests__/components/SelectInterlinearProjectModal.test.tsxsrc/__tests__/main.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/__tests__/services/projectStorage.test.tssrc/components/ContinuousView.tsxsrc/components/CreateProjectModal.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/ProjectModals.tsxsrc/components/ScriptureNavControls.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/parsers/pt9/interlinearXmlParser.tssrc/services/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (4)
- cspell.json
- src/components/ContinuousView.tsx
- src/components/ScriptureNavControls.tsx
- contributions/localizedStrings.json
🚧 Files skipped from review as they are similar to previous changes (18)
- src/interlinearizer.web-view.tsx
- src/components/Interlinearizer.tsx
- src/tests/components/SelectInterlinearProjectModal.test.tsx
- mocks/lucide-react.tsx
- contributions/menus.json
- mocks/papi-backend.ts
- src/components/InterlinearizerLoader.tsx
- mocks/papi-frontend.ts
- mocks/platform-bible-utils.ts
- src/components/ProjectModals.tsx
- src/tests/parsers/papi/usjBookExtractor.test.ts
- src/main.ts
- mocks/papi-frontend-react.ts
- src/services/projectStorage.ts
- src/tests/components/InterlinearizerLoader.test.tsx
- src/parsers/pt9/interlinearXmlParser.ts
- src/components/SelectInterlinearProjectModal.tsx
- mocks/platform-bible-react.tsx
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc partially reviewed 6 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/SelectInterlinearProjectModal.tsx (1)
37-50: 💤 Low valueConsider validating optional
nameanddescriptionfields in the type guard.The type guard checks required fields but skips the optional
nameanddescriptionproperties. If the backend or a future refactor returns those fields with incorrect types, the guard will pass and rendering (line 152) will defensively fall back to "unnamed", but TypeScript's narrowing will be incorrect.♻️ Proposed enhancement
export function isInterlinearProjectSummary(p: unknown): p is InterlinearProjectSummary { return ( !!p && typeof p === 'object' && 'id' in p && typeof p.id === 'string' && 'createdAt' in p && typeof p.createdAt === 'string' && 'sourceProjectId' in p && typeof p.sourceProjectId === 'string' && 'analysisWritingSystem' in p && - typeof p.analysisWritingSystem === 'string' + typeof p.analysisWritingSystem === 'string' && + (!('name' in p) || p.name === undefined || typeof p.name === 'string') && + (!('description' in p) || p.description === undefined || typeof p.description === '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 37 - 50, Update the type guard isInterlinearProjectSummary to also validate the optional properties name and description: check if 'name' exists then typeof p.name === 'string' (or allow undefined) and similarly for 'description' to ensure they are either strings or absent; locate the function isInterlinearProjectSummary and add these conditional checks so TypeScript narrowing matches the runtime rendering logic (which currently falls back to "unnamed" when name is missing).
🤖 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/components/SelectInterlinearProjectModal.tsx`:
- Around line 37-50: Update the type guard isInterlinearProjectSummary to also
validate the optional properties name and description: check if 'name' exists
then typeof p.name === 'string' (or allow undefined) and similarly for
'description' to ensure they are either strings or absent; locate the function
isInterlinearProjectSummary and add these conditional checks so TypeScript
narrowing matches the runtime rendering logic (which currently falls back to
"unnamed" when name is missing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cb3b62c-433b-4824-a1ce-0abd3d67b9a3
📒 Files selected for processing (30)
__mocks__/lucide-react.tsx__mocks__/papi-backend.ts__mocks__/papi-frontend-react.ts__mocks__/papi-frontend.ts__mocks__/platform-bible-react.tsx__mocks__/platform-bible-utils.tscontributions/localizedStrings.jsoncontributions/menus.jsoncspell.jsonjest.config.tssrc/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/components/InterlinearizerLoader.test.tsxsrc/__tests__/components/ProjectMetadataModal.test.tsxsrc/__tests__/components/SelectInterlinearProjectModal.test.tsxsrc/__tests__/main.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/__tests__/services/projectStorage.test.tssrc/components/ContinuousView.tsxsrc/components/CreateProjectModal.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/ProjectModals.tsxsrc/components/ScriptureNavControls.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/parsers/pt9/interlinearXmlParser.tssrc/services/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (5)
- src/components/Interlinearizer.tsx
- cspell.json
- contributions/localizedStrings.json
- src/components/ContinuousView.tsx
- src/components/ScriptureNavControls.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- contributions/menus.json
- mocks/lucide-react.tsx
- src/tests/components/SelectInterlinearProjectModal.test.tsx
- jest.config.ts
- src/interlinearizer.web-view.tsx
- mocks/papi-frontend-react.ts
- mocks/papi-backend.ts
- src/main.ts
- src/types/interlinearizer.d.ts
- src/components/CreateProjectModal.tsx
- mocks/platform-bible-utils.ts
- src/tests/services/projectStorage.test.ts
- src/components/ProjectModals.tsx
- src/parsers/pt9/interlinearXmlParser.ts
- mocks/papi-frontend.ts
- src/tests/parsers/papi/usjBookExtractor.test.ts
- src/tests/components/InterlinearizerLoader.test.tsx
- src/services/projectStorage.ts
- src/components/InterlinearizerLoader.tsx
- mocks/platform-bible-react.tsx
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: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/ProjectMetadataModal.tsx (1)
130-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate delete success callbacks on confirmed command success.
handleDeletecallsonProjectDeletedandonCloseunconditionally after thesendCommandpromise resolves. If the backend command resolves with a falsy value to signal a handled failure (as the save path anticipates), the modal will close and report a successful delete incorrectly.🔒 Proposed fix
const handleDelete = useCallback(async () => { if (isSubmittingRef.current) return; isSubmittingRef.current = true; setIsSubmitting(true); try { - await papi.commands.sendCommand('interlinearizer.deleteProject', interlinearProjectId); + const deletedProjectJson = await papi.commands.sendCommand( + 'interlinearizer.deleteProject', + interlinearProjectId, + ); + if (!deletedProjectJson) return; onProjectDeleted?.(interlinearProjectId); onClose(); } catch (e) {🤖 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/ProjectMetadataModal.tsx` around lines 130 - 144, handleDelete currently invokes onProjectDeleted and onClose immediately after papi.commands.sendCommand resolves, which can incorrectly treat a falsy/handled-failure response as success; change handleDelete to capture the response from papi.commands.sendCommand('interlinearizer.deleteProject', interlinearProjectId), check that the response is truthy/successful before calling onProjectDeleted(interlinearProjectId) and onClose(), and only clear isSubmitting flags in finally; keep existing error logging (logger.error) on catch so failures don't close the modal or trigger success callbacks.
🤖 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/ProjectMetadataModal.test.tsx`:
- Around line 228-243: Add a new unit test for ProjectMetadataModal that mirrors
the existing save-path falsy-resolution test: create a test named like "does not
call onProjectDeleted, onClose, or send a notification when delete sendCommand
resolves with a falsy value", set mockSendCommand.mockResolvedValue(undefined)
(or another falsy value), render <ProjectMetadataModal ...
onProjectDeleted={onProjectDeleted} onClose={onClose} />, trigger the delete
confirmation clicks (button with name /^delete$/i) and then waitFor
mockSendCommand to be called; assert that onProjectDeleted, onClose, and
papi.notifications.send were not called. Ensure you reference mockSendCommand,
ProjectMetadataModal, onProjectDeleted, onClose, and papi.notifications.send so
the test matches the rejection-case test structure.
In `@src/components/ProjectMetadataModal.tsx`:
- Around line 150-154: The dialog in ProjectMetadataModal should explicitly
signal modal behavior to assistive tech; update the <dialog> element in the
ProjectMetadataModal component to include aria-modal="true" (alongside the
existing open attribute and backdrop overlay) so screen readers understand it
blocks interaction with the rest of the page.
In `@src/services/projectStorage.ts`:
- Around line 197-220: The updateProjectMetadata function performs an unguarded
read-modify-write on projectKey(id) (getProject -> papi.storage.writeUserData)
which can race with deleteProject (and the projectIds index) and create orphaned
records; change updateProjectMetadata to use the same serialization mechanism
you use for the shared index (or introduce a per-project queue/lock or CAS) so
all mutations for a given project id are serialized (prevent interleaving
between getProject, deleteProject and writeUserData). Apply the same protection
to the other project-level mutator block referenced (the similar code around the
other update/delete paths) so all per-project read-modify-write operations use
the per-project queue/lock/CAS.
---
Duplicate comments:
In `@src/components/ProjectMetadataModal.tsx`:
- Around line 130-144: handleDelete currently invokes onProjectDeleted and
onClose immediately after papi.commands.sendCommand resolves, which can
incorrectly treat a falsy/handled-failure response as success; change
handleDelete to capture the response from
papi.commands.sendCommand('interlinearizer.deleteProject',
interlinearProjectId), check that the response is truthy/successful before
calling onProjectDeleted(interlinearProjectId) and onClose(), and only clear
isSubmitting flags in finally; keep existing error logging (logger.error) on
catch so failures don't close the modal or trigger success callbacks.
🪄 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: 5f42f4b6-544b-400d-9101-501b660e245d
📒 Files selected for processing (30)
__mocks__/lucide-react.tsx__mocks__/papi-backend.ts__mocks__/papi-frontend-react.ts__mocks__/papi-frontend.ts__mocks__/platform-bible-react.tsx__mocks__/platform-bible-utils.tscontributions/localizedStrings.jsoncontributions/menus.jsoncspell.jsonjest.config.tssrc/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/components/InterlinearizerLoader.test.tsxsrc/__tests__/components/ProjectMetadataModal.test.tsxsrc/__tests__/components/SelectInterlinearProjectModal.test.tsxsrc/__tests__/main.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/__tests__/services/projectStorage.test.tssrc/components/ContinuousView.tsxsrc/components/CreateProjectModal.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/ProjectModals.tsxsrc/components/ScriptureNavControls.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/parsers/pt9/interlinearXmlParser.tssrc/services/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (3)
- src/components/ContinuousView.tsx
- src/components/ScriptureNavControls.tsx
- src/components/Interlinearizer.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- contributions/menus.json
- cspell.json
- src/parsers/pt9/interlinearXmlParser.ts
- mocks/papi-frontend-react.ts
- mocks/papi-backend.ts
- mocks/papi-frontend.ts
- jest.config.ts
- mocks/lucide-react.tsx
- src/components/CreateProjectModal.tsx
- src/tests/components/SelectInterlinearProjectModal.test.tsx
- src/tests/parsers/papi/usjBookExtractor.test.ts
- src/main.ts
- src/components/SelectInterlinearProjectModal.tsx
- src/components/ProjectModals.tsx
- mocks/platform-bible-utils.ts
- src/tests/main.test.ts
- src/components/InterlinearizerLoader.tsx
- contributions/localizedStrings.json
- src/types/interlinearizer.d.ts
- mocks/platform-bible-react.tsx
Comment on lines
+228
to
+243
| it('does not call onProjectDeleted, onClose, or send a notification when delete sendCommand rejects', async () => { | ||
| mockSendCommand.mockRejectedValue(new Error('delete failed')); | ||
| const onProjectDeleted = jest.fn(); | ||
| const onClose = jest.fn(); | ||
| render( | ||
| <ProjectMetadataModal {...testProps} onProjectDeleted={onProjectDeleted} onClose={onClose} />, | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByRole('button', { name: /^delete$/i })); | ||
| await userEvent.click(screen.getByRole('button', { name: /^delete$/i })); | ||
|
|
||
| await waitFor(() => expect(mockSendCommand).toHaveBeenCalled()); | ||
| expect(onProjectDeleted).not.toHaveBeenCalled(); | ||
| expect(onClose).not.toHaveBeenCalled(); | ||
| expect(papi.notifications.send).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for delete command resolving falsy.
The save path has coverage for both rejection (lines 212–226) and falsy resolution (lines 196–210), but the delete path only covers rejection. Add a test verifying that when sendCommand('interlinearizer.deleteProject', ...) resolves with undefined or another falsy value, onProjectDeleted, onClose, and papi.notifications.send are not called—mirroring the save test at lines 196–210.
🧪 Proposed test
it('does not call onProjectDeleted, onClose, or send a notification when delete sendCommand resolves with a falsy value', async () => {
mockSendCommand.mockResolvedValue(undefined);
const onProjectDeleted = jest.fn();
const onClose = jest.fn();
render(
<ProjectMetadataModal {...testProps} onProjectDeleted={onProjectDeleted} onClose={onClose} />,
);
await userEvent.click(screen.getByRole('button', { name: /^delete$/i }));
await userEvent.click(screen.getByRole('button', { name: /^delete$/i }));
await waitFor(() => expect(mockSendCommand).toHaveBeenCalled());
expect(onProjectDeleted).not.toHaveBeenCalled();
expect(onClose).not.toHaveBeenCalled();
expect(papi.notifications.send).not.toHaveBeenCalled();
});🤖 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__/components/ProjectMetadataModal.test.tsx` around lines 228 -
243, Add a new unit test for ProjectMetadataModal that mirrors the existing
save-path falsy-resolution test: create a test named like "does not call
onProjectDeleted, onClose, or send a notification when delete sendCommand
resolves with a falsy value", set mockSendCommand.mockResolvedValue(undefined)
(or another falsy value), render <ProjectMetadataModal ...
onProjectDeleted={onProjectDeleted} onClose={onClose} />, trigger the delete
confirmation clicks (button with name /^delete$/i) and then waitFor
mockSendCommand to be called; assert that onProjectDeleted, onClose, and
papi.notifications.send were not called. Ensure you reference mockSendCommand,
ProjectMetadataModal, onProjectDeleted, onClose, and papi.notifications.send so
the test matches the rejection-case test structure.
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc partially reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/projectStorage.ts (1)
259-273: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winComplete
deleteProject’s@throwscontract to match actual propagated failures.The current doc omits non-
ENOENTdelete/read failures that can still escape this function, so callers don’t get an accurate error contract.📝 Suggested JSDoc adjustment
/** * Deletes the project with the given ID from storage and removes it from the index. No-ops silently * if the project does not exist. * * `@param` token - The execution token for storage access. * `@param` id - The project UUID to delete. * `@throws` {SyntaxError} If the `projectIds` storage value contains invalid JSON. - * `@throws` If `papi.storage.writeUserData` rejects when updating the index. + * `@throws` If `papi.storage.deleteUserData` rejects for any non-ENOENT reason. + * `@throws` If `papi.storage.readUserData` rejects for any non-ENOENT reason while reading `projectIds`. + * `@throws` If `papi.storage.writeUserData` rejects when updating the index. */As per coding guidelines:
Every function and method—exported or internal—must have ...@throwsfor every error condition the caller must handle.🤖 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/services/projectStorage.ts` around lines 259 - 273, The JSDoc for deleteProject is incomplete: it currently only documents SyntaxError and the writeUserData rejection but omits other propagated failures; update the `@throws` block for deleteProject to list that papi.storage.deleteUserData can throw (non-ENOENT errors are rethrown), papi.storage.writeUserData can reject when updating PROJECT_IDS_KEY, and any errors from readIds (and the enqueueProjectOp/enqueueIndexOp wrappers) may propagate; reference the deleteProject function, the call sites papi.storage.deleteUserData(token, projectKey(id)), readIds(token), and papi.storage.writeUserData(token, PROJECT_IDS_KEY, ...) so callers know to handle these error conditions.
🤖 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/ProjectMetadataModal.tsx`:
- Around line 247-249: The Cancel button in the delete-confirm modal must be
disabled while the delete request is in-flight; add/consume a boolean
"isDeleting" (or reuse existing deletion state) and set the Cancel Button's
disabled prop to that state (Button ... disabled={isDeleting}) and ensure any
onClick handler (setConfirmingDelete(false)) is a no-op while isDeleting is true
so the UI cannot cancel the modal while the delete call (e.g., in
deleteProject/handleDeleteProject) is pending.
---
Duplicate comments:
In `@src/services/projectStorage.ts`:
- Around line 259-273: The JSDoc for deleteProject is incomplete: it currently
only documents SyntaxError and the writeUserData rejection but omits other
propagated failures; update the `@throws` block for deleteProject to list that
papi.storage.deleteUserData can throw (non-ENOENT errors are rethrown),
papi.storage.writeUserData can reject when updating PROJECT_IDS_KEY, and any
errors from readIds (and the enqueueProjectOp/enqueueIndexOp wrappers) may
propagate; reference the deleteProject function, the call sites
papi.storage.deleteUserData(token, projectKey(id)), readIds(token), and
papi.storage.writeUserData(token, PROJECT_IDS_KEY, ...) so callers know to
handle these error conditions.
🪄 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: 93fc3194-314f-4717-9f1c-3c55bef69dcf
📒 Files selected for processing (30)
__mocks__/lucide-react.tsx__mocks__/papi-backend.ts__mocks__/papi-frontend-react.ts__mocks__/papi-frontend.ts__mocks__/platform-bible-react.tsx__mocks__/platform-bible-utils.tscontributions/localizedStrings.jsoncontributions/menus.jsoncspell.jsonjest.config.tssrc/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/components/InterlinearizerLoader.test.tsxsrc/__tests__/components/ProjectMetadataModal.test.tsxsrc/__tests__/components/SelectInterlinearProjectModal.test.tsxsrc/__tests__/main.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/__tests__/services/projectStorage.test.tssrc/components/ContinuousView.tsxsrc/components/CreateProjectModal.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/ProjectModals.tsxsrc/components/ScriptureNavControls.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/parsers/pt9/interlinearXmlParser.tssrc/services/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (4)
- cspell.json
- mocks/lucide-react.tsx
- src/components/ContinuousView.tsx
- contributions/localizedStrings.json
🚧 Files skipped from review as they are similar to previous changes (20)
- src/parsers/pt9/interlinearXmlParser.ts
- mocks/papi-frontend-react.ts
- src/components/ScriptureNavControls.tsx
- jest.config.ts
- src/interlinearizer.web-view.tsx
- src/components/Interlinearizer.tsx
- contributions/menus.json
- src/tests/components/InterlinearizerLoader.test.tsx
- src/main.ts
- src/components/ProjectModals.tsx
- src/components/SelectInterlinearProjectModal.tsx
- mocks/papi-frontend.ts
- src/components/InterlinearizerLoader.tsx
- src/tests/services/projectStorage.test.ts
- src/tests/components/SelectInterlinearProjectModal.test.tsx
- src/tests/parsers/papi/usjBookExtractor.test.ts
- src/types/interlinearizer.d.ts
- mocks/platform-bible-utils.ts
- src/tests/main.test.ts
- mocks/platform-bible-react.tsx
…zero-check in ContinuousView explicit
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc partially reviewed 6 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/projectStorage.ts (1)
185-189: ⚡ Quick winConsider per-project error tolerance so one bad record doesn't hide the whole list.
getProjectalready returnsundefinedfor ENOENT and those are filtered out at Line 188, but any other failure (corrupted JSON in a single project file, transient I/O error on one read) rejectsPromise.alland propagates out oflistProjects/getProjectsForSource. The effect is that a user with N projects becomes unable to list any of them if just one record is unreadable. Given the function is already designed to tolerate missing entries, extending the same tolerance to read/parse failures (log the offender and skip it) keeps the rest of the UI usable.♻️ Sketch of a more resilient read
export async function listProjects(token: ExecutionToken): Promise<InterlinearProject[]> { const ids = await readIds(token); - const projects = await Promise.all(ids.map((id) => getProject(token, id))); - return projects.filter((p): p is InterlinearProject => p !== undefined); + const projects = await Promise.all( + ids.map(async (id) => { + try { + return await getProject(token, id); + } catch (e) { + logger.warn(`Skipping project ${id} due to read/parse failure:`, e); + return undefined; + } + }), + ); + return projects.filter((p): p is InterlinearProject => p !== undefined); }If you take this path, update the
@throwsblock to reflect that per-project read failures are logged and skipped rather than propagated.🤖 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/services/projectStorage.ts` around lines 185 - 189, listProjects currently uses Promise.all(ids.map(getProject)) which fails entirely if any single getProject throws (e.g., corrupted JSON); change the map to call an async wrapper per id that awaits getProject(token, id) inside try/catch, logs the id and error on any non-ENOENT failure, and returns undefined so the final filter((p): p is InterlinearProject => p !== undefined) still works; update the `@throws` doc to note that per-project read/parse errors are logged and skipped.
🤖 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 84-86: In the CreateProjectModal.tsx catch block where you
currently call logger.error('Interlinearizer: failed to create project', e),
also surface a localized user-facing error notification so the modal doesn't
silently fail; call the same notification helper used elsewhere in this
component (e.g., the component's error state setter or the shared
toast/notify/enqueueSnackbar helper) and pass a localized message plus error
details (use your i18n key or t('...') if available) before the finally block so
users see the failure in the UI as well as in logs.
---
Nitpick comments:
In `@src/services/projectStorage.ts`:
- Around line 185-189: listProjects currently uses
Promise.all(ids.map(getProject)) which fails entirely if any single getProject
throws (e.g., corrupted JSON); change the map to call an async wrapper per id
that awaits getProject(token, id) inside try/catch, logs the id and error on any
non-ENOENT failure, and returns undefined so the final filter((p): p is
InterlinearProject => p !== undefined) still works; update the `@throws` doc to
note that per-project read/parse errors are logged and skipped.
🪄 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: ac10a4a1-b888-4e0a-b6e2-d619d94ee77e
📒 Files selected for processing (30)
__mocks__/lucide-react.tsx__mocks__/papi-backend.ts__mocks__/papi-frontend-react.ts__mocks__/papi-frontend.ts__mocks__/platform-bible-react.tsx__mocks__/platform-bible-utils.tscontributions/localizedStrings.jsoncontributions/menus.jsoncspell.jsonjest.config.tssrc/__tests__/components/CreateProjectModal.test.tsxsrc/__tests__/components/InterlinearizerLoader.test.tsxsrc/__tests__/components/ProjectMetadataModal.test.tsxsrc/__tests__/components/SelectInterlinearProjectModal.test.tsxsrc/__tests__/main.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/__tests__/services/projectStorage.test.tssrc/components/ContinuousView.tsxsrc/components/CreateProjectModal.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/ProjectMetadataModal.tsxsrc/components/ProjectModals.tsxsrc/components/ScriptureNavControls.tsxsrc/components/SelectInterlinearProjectModal.tsxsrc/interlinearizer.web-view.tsxsrc/main.tssrc/parsers/pt9/interlinearXmlParser.tssrc/services/projectStorage.tssrc/types/interlinearizer.d.ts
✅ Files skipped from review due to trivial changes (4)
- src/components/ContinuousView.tsx
- cspell.json
- src/components/ScriptureNavControls.tsx
- contributions/localizedStrings.json
🚧 Files skipped from review as they are similar to previous changes (20)
- src/interlinearizer.web-view.tsx
- contributions/menus.json
- jest.config.ts
- src/parsers/pt9/interlinearXmlParser.ts
- src/components/ProjectModals.tsx
- src/components/Interlinearizer.tsx
- src/main.ts
- mocks/papi-frontend-react.ts
- src/tests/components/InterlinearizerLoader.test.tsx
- src/types/interlinearizer.d.ts
- src/tests/components/CreateProjectModal.test.tsx
- src/tests/services/projectStorage.test.ts
- src/tests/components/SelectInterlinearProjectModal.test.tsx
- src/tests/parsers/papi/usjBookExtractor.test.ts
- mocks/platform-bible-utils.ts
- src/components/InterlinearizerLoader.tsx
- src/components/ProjectMetadataModal.tsx
- src/components/SelectInterlinearProjectModal.tsx
- src/tests/main.test.ts
- mocks/platform-bible-react.tsx
Comment on lines
+84
to
+86
| } catch (e) { | ||
| logger.error('Interlinearizer: failed to create project', e); | ||
| } finally { |
There was a problem hiding this comment.
Surface user-facing failure feedback in catch.
On Line 84, parse/transport failures fall into catch and only log, so the modal stays open with no visible error. Add the same localized error notification here so non-backend failures aren’t silent.
💡 Minimal fix
} catch (e) {
logger.error('Interlinearizer: failed to create project', e);
+ await papi.notifications
+ .send({
+ message: '%interlinearizer_error_create_project_failed%',
+ severity: 'error',
+ })
+ .catch(() => {});
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (e) { | |
| logger.error('Interlinearizer: failed to create project', e); | |
| } finally { | |
| } catch (e) { | |
| logger.error('Interlinearizer: failed to create project', e); | |
| await papi.notifications | |
| .send({ | |
| message: '%interlinearizer_error_create_project_failed%', | |
| severity: 'error', | |
| }) | |
| .catch(() => {}); | |
| } finally { |
🤖 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/CreateProjectModal.tsx` around lines 84 - 86, In the
CreateProjectModal.tsx catch block where you currently call
logger.error('Interlinearizer: failed to create project', e), also surface a
localized user-facing error notification so the modal doesn't silently fail;
call the same notification helper used elsewhere in this component (e.g., the
component's error state setter or the shared toast/notify/enqueueSnackbar
helper) and pass a localized message plus error details (use your i18n key or
t('...') if available) before the finally block so users see the failure in the
UI as well as in logs.
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc partially reviewed 2 files and all commit messages, and resolved 1 discussion.
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
Tests