Repository navigation
Replace Wipe with Discard Unsaved Changes that reverts to saved - #512
alex-rawlings-yyc wants to merge 3 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
|
| Layer / File(s) | Summary |
|---|---|
Command and scope dialog src/types/interlinearizer.d.ts, src/main.ts, contributions/menus.json, __mocks__/platform-bible-react.tsx, src/components/modals/DiscardChangesModal.tsx, src/components/modals/WipeModal.tsx, contributions/localizedStrings.json, src/__tests__/components/modals/*, user-questions.md |
The registered command, menu item, and dialog use discard terminology. The modal offers current-book and entire-draft scopes, with the book option disabled when no book is active. Localization and dialog tests cover the new labels, options, and actions. |
Restore analysis and draft state src/utils/analysis-book.ts, src/hooks/useDraftProject.ts, src/__tests__/utils/analysis-book.test.ts, src/__tests__/hooks/useDraftProject.test.ts, src/__tests__/services/projectStorage.test.ts, docs/data-layer-plan/plan.md, src/hooks/useLostBoundaryDismissal.ts, src/hooks/useRecordDismissal.ts, src/hooks/useStaleAnalysisDismissal.ts |
New helpers restore a book’s analysis records and segmentation anchors from optional saved content. The hook exposes discardBook and discardAll, preserves the documented dirty-state behavior, and records discard operations for undo and redo. Tests cover restoration, clearing, and undo/redo states. |
Project loading and discard integration src/utils/read-openable-project.ts, src/components/InterlinearizerLoader.tsx, src/components/modals/ProjectModals.tsx, src/__tests__/utils/read-openable-project.test.ts, src/__tests__/components/InterlinearizerLoader.test.tsx, e2e-tests/fixtures/helpers.ts, e2e-tests/tests/features/*, e2e-tests/README.md, e2e-tests/global-setup-cdp.ts |
The loader reads the active project before discarding. If the project read fails or returns no project, it reports an error and leaves the draft unchanged. Otherwise, it passes saved content to the selected discard operation. Project opening also uses the shared project reader, and E2E setup and tests use discardAllChanges. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant User
participant InterlinearizerLoader
participant DiscardChangesModal
participant readOpenableProject
participant getProject as interlinearizer.getProject
participant useDraftProject
User->>InterlinearizerLoader: Open discard-changes command
InterlinearizerLoader->>DiscardChangesModal: Render scope choices
User->>DiscardChangesModal: Confirm selected scope
DiscardChangesModal->>InterlinearizerLoader: Return scope
InterlinearizerLoader->>readOpenableProject: Read active project
readOpenableProject->>getProject: Request project data
getProject-->>readOpenableProject: Return serialized project
readOpenableProject-->>InterlinearizerLoader: Return validated project or undefined
InterlinearizerLoader->>useDraftProject: Discard selected scope using saved content
Possibly related issues
- [3i] Add catalog-wide edits, wipes and previewRemovals #448 — The issue concerns catalog-wide wipe behavior, and this PR changes the existing draft discard behavior and its book and whole-draft scopes.
Possibly related PRs
- sillsdev/interlinearizer-extension#499 — This PR imports the
makeTokenLinktest helper introduced by PR#499.
Suggested reviewers: imnasnainaec
Merge Risk: 
Low · previous review 125c6
-
An edit made while a discard is loading can unexpectedly disappear. Fix the pending-discard interaction before merging, or explicitly accept this narrow risk.
Pre-merge checks: 
5
-
Full details: Title check
-
Explanation
The title names the primary change: the Wipe command is replaced by a Discard Unsaved Changes flow that restores the active project's saved analysis. The changeset matches this. It renames the command, modal, hooks, strings, and E2E helper, and adds readOpenableProject, restoreBookInAnalysis, and restoreBookInSegmentation to restore saved content. The title is specific and single-sentence, and it contains no vague terms.
Full details: Linked Issues check
-
Explanation
[
#506] Requests rewording of the Wipe menu item and dialog, preferring "Discard unsaved changes...". The PR renames the menu item to "Discard Unsaved Changes…" (contributions/localizedStrings.json,%interlinearizer_discardChanges%). The dialog title "Discard unsaved changes", prompt "Choose how much of the unsaved analyses to discard.", and the scope options "Current book" and "Entire draft" with the requested descriptions match the issue text. The confirm action is labeled "Discard". The line "Save afterward to persist changes to the interlinear" is omitted. The PR explains this: a reverted draft has nothing to save. The omission is a documented, reasonable deviation from suggested text, not an unmet requirement. The behavior now matches the wording. Discard restores saved analysis and segment boundaries from the active project viarestoreBookInAnalysis,restoreBookInSegmentation, anddiscardBook/discardAllin src/hooks/useDraftProject.ts, and empties the scope when no project is active. Unit tests cover DiscardChangesModal (src/tests/components/modals/DiscardChangesModal.test.tsx) and the discard behavior in useDraftProject.test.ts. No coding requirements of#506remain unmet. The issue has no non-coding requirements.
Full details: Out of Scope Changes check
-
Explanation
The rename changes (command id
interlinearizer.openDiscardChangesModal, component, string keys, test IDs,WipeModalremoval,wipeDrafttodiscardAllChangese2e helper, comment updates in hooks, and mock updates) directly implement the requested rewording. The behavior change from emptying to reverting to saved state is not stated in#506. It is a connected implementation, because the issue's required wording "Discard all unsaved analyses" describes restoring saved state. Without it, the Wipe behavior would contradict the new label. The sharedreadOpenableProjectreader (src/utils/read-openable-project.ts) is used by both Open (src/components/modals/ProjectModals.tsx) and discard. This removes duplicated project parsing and supports the failure path that leaves the draft unchanged. The helpers in src/utils/analysis-book.ts support the book-scoped restore. Documentation changes in docs/data-layer-plan/plan.md and user-questions.md describe the renamed behavior and do not add unrelated scope. Tests added or updated for these changes support the implemented behavior. No demonstrated unrelated change was found. The PR's ● behavior on book discard is a stated design decision, not an out-of-scope change.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 22 files. (5 skipped: 5 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec made 1 comment.
Reviewable status: 0 of 27 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
src/hooks/useDraftProject.ts line 569 at r2 (raw file):
// Left as dirty as it was: other books may still differ from the saved project. applyReplacement( withContent(current, restored, current.dirty),
From Devin, unverified:
A book discard with no active project can leave unsaved work marked as saved
This line keeps the draft's old dirty flag in every case:
withContent(current, restored, current.dirty),When saved is undefined (no active project, e.g. after it was deleted), the other books' analysis has no saved copy anywhere. wipeBook used to set dirty: true here, so the ● came on and switching projects asked for confirmation. Now a book discard on a clean draft leaves the remaining books unsaved but still marked clean, so they could be lost without a prompt.
Suggested fix: keep the dirty flag only when restoring from a saved project. Otherwise, mark the draft unsaved, as Wipe did:
// With a saved project, other books may still differ from it, so keep the flag as it was.
// Without one, nothing the draft still holds is saved anywhere, so mark it unsaved, as Wipe did.
applyReplacement(
withContent(current, restored, saved ? current.dirty : true),
You could narrow this so that a draft the discard leaves empty stays clean, by marking it unsaved only if analysis links or restored.segmentation remain.
Please also add a test in src/__tests__/hooks/useDraftProject.test.ts under discardBook → "with no saved project": start from a clean draft with GEN and MAT analysis, call discardBook('GEN'), and expect dirty to be true both in memory and in the persisted draft.
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 1 comment.
Reviewable status: 0 of 27 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc and imnasnainaec).
src/hooks/useDraftProject.ts line 569 at r2 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
From Devin, unverified:
A book discard with no active project can leave unsaved work marked as saved
This line keeps the draft's old dirty flag in every case:
withContent(current, restored, current.dirty),When
savedisundefined(no active project, e.g. after it was deleted), the other books' analysis has no saved copy anywhere.wipeBookused to setdirty: truehere, so the ● came on and switching projects asked for confirmation. Now a book discard on a clean draft leaves the remaining books unsaved but still marked clean, so they could be lost without a prompt.Suggested fix: keep the dirty flag only when restoring from a saved project. Otherwise, mark the draft unsaved, as Wipe did:
// With a saved project, other books may still differ from it, so keep the flag as it was. // Without one, nothing the draft still holds is saved anywhere, so mark it unsaved, as Wipe did. applyReplacement( withContent(current, restored, saved ? current.dirty : true),You could narrow this so that a draft the discard leaves empty stays clean, by marking it unsaved only if analysis links or
restored.segmentationremain.Please also add a test in
src/__tests__/hooks/useDraftProject.test.tsunderdiscardBook→ "with no saved project": start from a clean draft with GEN and MAT analysis, calldiscardBook('GEN'), and expectdirtyto betrueboth in memory and in the persisted draft.
Fixed as suggested, with the test. The gap starts before the discard, though: deleting the active project already leaves a clean draft whose analysis is saved nowhere. That's now #519.
Closes #506.
Summary
Wipe… becomes Discard Unsaved Changes…, and it now does what that name says. It resets the chosen scope (the current book or the entire draft) to the active project's saved analysis and segment boundaries. With no active project, it empties the scope, as Wipe did. The dialog uses the wording proposed in the issue, minus "Save afterward to persist changes", because a reverted draft has nothing to save.
interlinearizer.openDiscardChangesModaland the component isDiscardChangesModal. The string keys, test ids, the draft hook'sdiscardBook/discardAlland the e2e reset helperdiscardAllChangesfollow suit.readOpenableProject, which validates the project and its analysis and boundaries.Screenshots
Menu:

Dialog:

After discarding the entire draft:

Decisions with a strong alternative
Evidence
tscare clean.Docs
The data-layer plan calls this action an "unsaved-change discard", because "discard" there already names a stale-handling action. Entries 3, 8 and 10 in user-questions.md describe the new behavior.
Merge Danger
Door: two-way
The command id and localized string keys are renamed. Nothing outside this extension calls them.
Blast Radius: the discard dialog and menu item, the draft's book and whole-draft discards, the Open flow's project read, and the e2e reset step
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit