Split modals into separate component#62
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR refactors project modal state management by extracting modal UI orchestration from ChangesModal State Extraction and Delegation
Sequence Diagram(s)sequenceDiagram
participant SelectModal as SelectInterlinearProjectModal
participant ProjectModals
participant MetadataModal as ProjectMetadataModal
participant WebViewState as WebView State
SelectModal->>ProjectModals: User opens metadata for project
ProjectModals->>ProjectModals: Set metadataProject from selection
ProjectModals->>MetadataModal: Render with metadataProject
MetadataModal->>ProjectModals: Save metadata changes
ProjectModals->>ProjectModals: Merge updates into activeProject
ProjectModals->>WebViewState: Persist updated activeProject
ProjectModals->>SelectModal: Return to originating modal
SelectModal->>ProjectModals: User selects project
ProjectModals->>WebViewState: Persist as activeProject
ProjectModals->>ProjectModals: Set modal to none
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/ProjectModals.tsx (1)
14-24: ⚡ Quick winAdd
@returnsto the JSDoc block for consistency.The coding guidelines require
@returnsfor all functions except those returningvoidorPromise<void>. React components returnJSX.Element, so should include this tag. The similar componentSelectInterlinearProjectModalinSelectInterlinearProjectModal.tsx(line 59) includes@returns, so adding it here maintains consistency across the codebase.📝 Suggested JSDoc addition
* `@param` props.useWebViewState - Hook for reading and writing values persisted in the WebView's * saved state (survives tab restores) + * `@returns` A container that conditionally renders one of the project modals (select, create, or + * metadata) based on the current modal state, or an empty div when no modal is open. */ export default function ProjectModals({As per coding guidelines: "Every function and method—exported or internal—must have a JSDoc block with a summary sentence describing what the function does and why it exists,
@paramfor every parameter,@returnsdescribing the return value (omit only forvoid/Promise<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 `@src/components/ProjectModals.tsx` around lines 14 - 24, The JSDoc for the ProjectModals React component is missing an `@returns` tag; update the comment block above the ProjectModals component to include "@returns {JSX.Element} The rendered project modals" (or similar) so it matches the project's guideline and the other component examples like SelectInterlinearProjectModal; ensure the tag is added to the existing JSDoc that documents props.modal, props.projectId, props.setModal, and props.useWebViewState for the ProjectModals component.
🤖 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/ProjectModals.tsx`:
- Around line 14-24: The JSDoc for the ProjectModals React component is missing
an `@returns` tag; update the comment block above the ProjectModals component to
include "@returns {JSX.Element} The rendered project modals" (or similar) so it
matches the project's guideline and the other component examples like
SelectInterlinearProjectModal; ensure the tag is added to the existing JSDoc
that documents props.modal, props.projectId, props.setModal, and
props.useWebViewState for the ProjectModals component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3737e7d-8a13-4842-bc80-913b3703225f
📒 Files selected for processing (3)
src/components/InterlinearizerLoader.tsxsrc/components/ProjectModals.tsxsrc/components/SelectInterlinearProjectModal.tsx
|
Somehow I didn't quite catch what you meant when you made the earlier comment. This looks great; thanks for doing it. I will make some adjustments and get it tested properly. |
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 4 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca>
* Add interlinearizer project storage and createProject command - 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 * Documentation improvement * Documentation improvements, improve error handling when deleting project and add associated test, fix schema inconsistency * Set `restoreMocks` to `true` in jest config * Make sure notification failure doesn't cause unnecessary throw, reorder index/data write to `papi` storage to ensure data doesn't become inaccessible * Prevent same-project selection when creating an interlinear project, 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. * Add missing `localizedStrings`, add clarifying comment, improve test coverage * Improve docstring coverage * Add project creation, metadata editing, and project selection UI 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. * Fix modal close/callback ordering and trim whitespace from language input - `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 * Return full project JSON from createProject and add error handling for 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 * Use `<dialog>` for modals, guard on falsy update return, and relax gloss/senseRef constraint - Replace `<div>` containers with `<dialog open aria-labelledby="…">` in `CreateProjectModal`, `ProjectMetadataModal`, and `SelectInterlinearProjectModal` for proper accessibility semantics. - In `ProjectMetadataModal.handleSave`, return early when `updateProjectMetadata` resolves with a falsy value (mirrors the existing guard in `CreateProjectModal.handleCreate`). - In `interlinearizer.d.ts`, replace the discriminated union on `Token` and `Phrase` that forbade setting both `gloss`/`glossSenseRef` (or `gloss`/`senseRef`) with optional fields, reflecting the updated rule that `gloss` serves as a local override when both are present. - Add a test for the falsy-return early-exit path in `ProjectMetadataModal`, and add the missing `waitFor` before negative assertions in the `CreateProjectModal` falsy-return test. * Fix analysis language default, button state, and storage error propagation - Default analysis language to "und" (undetermined) instead of "en" - Normalize whitespace-only language input to "und" on submit - Disable the create button while submission is in progress - Rethrow storage errors in getProjectsForSource so callers can distinguish an outage from an empty list * Throw on malformed createProject response instead of silently skipping onProjectCreated Previously, if the backend returned a JSON object that failed isInterlinearProjectSummary, the modal would call onClose() without invoking onProjectCreated, silently dropping the contract. Now it throws, routing through the existing error handler. Also fixes an incomplete fixture in the success test that was missing required summary fields. * Cleanup after incredibly messy rebase * More post-rebase cleanup * Improve command registration usage and sanity * Disable cancel during submission to avoid "I cancelled but the project was still created", update docs * Move `projectStorage` to `services` directory * Improve project creation/update error handling, improve project load validation * Improve `projectStorage` error-handling and documentation * Add missing `\@throws` docstring * Rename modal control commands for clarity, fix handlers in InterlinearizerLoader * Improve docs/schema description * Split modals into separate component (#62) Co-authored-by: alex-rawlings-yyc <alex.rawlings@wycliffe.ca> * Remove TOCTOU race from PAPI storage writes, remove duplicate notifications on error, disable metadata modal buttons when submitting * Add eslint ignore, add submitting ref * Add missing docs * Send notification when created project has unexpected shape, add submitting ref * Validate name and description in type guard * Prevent project write race, add aria prop * Disable buttons while submitting/loading, update docs, make implicit zero-check in ContinuousView explicit * Skip and log singleton corrupted project records * Update remaining `tw-` tags * Extract modal logic into ProjectModals, tighten type guards and coverage - Add ProjectModals component to own modal state (select/create/metadata) and their transitions, removing this responsibility from InterlinearizerLoader - Add ProjectModals.test.tsx with full coverage of modal visibility and transition flows - Strengthen isPlatformError mock to require platformErrorVersion to be a valid number, not just present - Handle SyntaxError in CreateProjectModal.handleSubmit with a user-visible notification instead of silently failing - Add test for invalid JSON response in CreateProjectModal - Add test for non-string description field in SelectInterlinearProjectModal - Mark isSubmittingRef guard branches with v8 ignore in modals * Update docs, fix misleading test * Align `atStart` and `atEnd` length checks, get rid of `createSourceIsSelect`, add function to reset storage queues for testing * Fix and consolidate modal styles (#74) * Simplify interlinear model (#63) * Simplify interlinear model: remove InterlinearAlignment/InterlinearText, add ActiveProject * Fix model gaps for lossless LCM / PT9 / BT Extension import * Make `analysisLanguages` required * Add comments about mapping of BT Extension's `sideNum` * Update docs/schema * Further refinement; please see updated description * Suggested model tweak * Model idea: Split linking out from analyses --------- Co-authored-by: D. Ror. <imnasnainaec@gmail.com> * Align code with simplified interlinear model Update components, storage, main, and types to match the model introduced in #63: rename commands, fix return types, update JSDoc, and adjust tests throughout. * Memoize Modal handler callbacks, add `targetProjectId` to modals where relevant to prevent silent deletion, docstring audit * Add missing JSDocs * Align stub with actual implementation, send notification when save/submission fails, docs adjustments * Trimmed down `AGENTS.md` to where it was before post-model-change update * Prevent stale data race condition, update old TW classes, TW class consolidation * Prevent double notifications * Improve docs, minor fixes * Add aria modal tag where missing * Post-rebase cleanup * Make `webViewNonce` optional, revert to earlier TW classes, introduce missing TW classes * Fix lint error * Fix Tailwind classes, clean up test files, ignore branches that don't need testing, reintroduce createSourceIsSelect to reopen SelectProjectModal if user cancels project creation * Fix lint issue * Fix test failure --------- Co-authored-by: Danny Rorabaugh <imnasnainaec@gmail.com>
Suggested refactor for #33
This is what it might look like to have the models handled in their own component. The only change in behavior is that after a project is created via the select-project modal, you don't return to that modal, which I think would be simpler for users anyway.
Note: the tests haven't been updated to match this change.
Also: some of the cross-component types should perhaps be extracted to a common types file.
Devin has a few useful flags as well if this goes through: https://app.devin.ai/review/sillsdev/interlinearizer-extension/pull/62
This change is
Summary by CodeRabbit