Make the morpheme-break reset explicit: rename Delete, and reset on the bare surface form - #173
Open
alex-rawlings-yyc wants to merge 3 commits into
Open
Make the morpheme-break reset explicit: rename Delete, and reset on the bare surface form#173alex-rawlings-yyc wants to merge 3 commits into
alex-rawlings-yyc wants to merge 3 commits into
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesMorpheme reset workflow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TokenChip
participant AnalysisStore
participant analysisSlice
participant MorphemeBreakdownPopover
participant Editor
TokenChip->>AnalysisStore: Read reset gloss-loss state
AnalysisStore->>analysisSlice: Select state for token
analysisSlice-->>AnalysisStore: Return confirmation flag
AnalysisStore-->>TokenChip: Provide resetLosesGlosses
TokenChip->>MorphemeBreakdownPopover: Pass onReset and needsResetConfirm
Editor->>MorphemeBreakdownPopover: Choose Reset
alt Confirmation required
MorphemeBreakdownPopover-->>Editor: Render confirmation prompt
Editor->>MorphemeBreakdownPopover: Confirm reset
end
MorphemeBreakdownPopover->>TokenChip: Invoke onReset
TokenChip->>TokenChip: Dispatch morpheme deletion
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
alex-rawlings-yyc
force-pushed
the
improve-morph-break-ui
branch
from
July 27, 2026 18:30
18cad6d to
cdef94c
Compare
alex-rawlings-yyc
force-pushed
the
improve-morph-break-ui
branch
from
July 27, 2026 20:01
cdef94c to
35d8573
Compare
handleSave returns early on an empty draft without closing, so an outside click after clearing the input left the modal popover stuck open.
PR #168 migrated this popover's buttons to platform-bible-react `Button` but left the breakdown text field and its label as raw HTML. Bring them in line with the convention the three project modals already follow, so the field picks up the host's focus ring, disabled styling, and dark-mode tokens instead of the hand-rolled approximations. Drops the classes `Input` already supplies (rounded, border, border-input, px/py, text-sm) and keeps only `w-full` and the `font-mono` that the morpheme forms want. `Label` defaults to text-sm, so the popover's smaller `text-xs` stays as an explicit override. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alex-rawlings-yyc
force-pushed
the
improve-morph-break-ui
branch
from
July 27, 2026 22:01
e8be8dd to
43c3202
Compare
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
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.
What & why
Two usability problems with the morpheme-break popover's destructive control:
unbelievableoverun- believe -able) disabledDoneand silently dismissed. But asking for "one morpheme equal to the whole word" is a request for the unsegmented state, so it should reset rather than be rejected.Changes
Renamed the button to "Reset" (value only — the
%interlinearizer_morphemeEditor_delete%key is unchanged, see Open question below). TheonDeleteprop is renamedonReset.Reworked the commit gate.
isMeaninglessis retired; committing now resolves to one of four outcomes:DoneNotable points:
Doneis no longer disabled while unchanged. The panel always opens pre-filled, so disabling it would leave a dead primary button on every open with no explanation of why.Donemeans "I'm finished here"; empty is the only true error state, and it now gets a hint explaining the expected format.running→run). Only a form equal to the surface text means "no segmentation". Verified safe:MorphemeAnalysis.formis documented as the form "as it appears in this analysis", morphemes carry no offsets, and — unlike the token-levelSegment.baselineText.slice(charStart, charEnd) === Token.surfaceText— there is no invariant requiring morphemes to reconstruct the surface.Added a confirmation before an irreversible reset. Both routes (the button and typing the bare surface form) funnel through one
requestReset(), which confirms only when the reset would destroy gloss data this token solely owns.The second half of that condition matters:
deleteMorphemesalready branches on whether the payload is shared, forking a private clone so co-linked tokens keep their morphemes. When the payload is shared, nothing is lost project-wide — the user is detaching one occurrence — so no prompt. A breakdown with no glosses is bare segmentation that's cheap to retype, so no prompt there either. The friction lands only where the loss is real, which keeps experimenting-with-splits a single click.Justified because the app has no undo, and the repo already confirms a smaller loss (
UnlinkPhraseConfirm).The confirmation replaces the popover panel's own content rather than opening a second surface. The panel is portaled to
document.body, so it floats over the token chip and can't reflow it, and nesting a modal inside this already-modal popover would stack two focus traps. An outside click while confirming dismisses without resetting — the confirmation exists precisely because the loss is irreversible, so a stray click must not answer it.Wiring. New
selectMorphemeResetLosesGlossesselector +useMorphemeResetLosesGlosseshook;TokenChipthreads the result to the popover asneedsResetConfirm. The selector reads sharing through the existing memoizedselectApprovedTokenCountByAnalysisId— the same approved-link count the reducer'sisPayloadSharedByOtherLinkstests — so the two can't disagree about what "shared" means.Strings. Three new keys in
localizedStrings.json(_emptyHint%,_confirmResetPrompt%,_confirmResetAction%), all localized.The confirm copy is deliberately count-free (
Discard this breakdown and its glosses?rather thanDiscard 2 morpheme glosses?). This codebase's localization is a flat key→string lookup with a single manual.replace()call site and no plural-shaped keys, so a counted string would mean hand-rolling the project's first pluralization scheme in English's two-form model — which misfits languages with dual forms, last-digit selection, or no count inflection at all. The user can see the morphemes in the chip behind the panel anyway.Relationship to #168
#168 migrates this file's three buttons to
platform-bible-reactButton. This PR deliberately keeps raw<button>, because writing pbr components here would conflict with #168 rather than rebase onto it. The new confirm buttons reuse the exact class strings #168 maps tovariant/size, so its migration extends to them mechanically.Post-merge follow-up: convert the new buttons alongside the rest, and adopt
PopoverDescription/PopoverTitlefor the confirm copy.Open question
"Reset" vs. "Remove breakdown" is still under review with the team. The English value changed but the
_delete%key did not, so flipping the wording is a one-line diff rather than churn across translation files andPOPOVER_STRING_KEYS._confirmResetAction%is a separate key so a translator can use a shorter verb in the confirmation, where the prompt has already established the subject.Testing
npm test— 1594 passing, 100% coverage (statements / branches / functions / lines).npm run lintclean.New coverage: the reset-on-surface-form path, the no-breakdown dismissal, single-morpheme saves, the empty hint, all six confirmation behaviors (both entry routes, accept, cancel, outside-click, and the no-confirm-needed case), the four selector cases, and prop threading through
TokenChip.Two pre-existing tests asserting that a whole-word draft is blocked were rewritten — that behavior is intentionally replaced by the reset.
`
This change is
Summary by CodeRabbit
New Features
Bug Fixes