Show stale free translations in place (#374) - #378
alex-rawlings-yyc wants to merge 20 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (18)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds stale free-translation selection, placement, and review controls. Users can keep, edit, or discard stale translations. Height prediction accounts for review rows, and heading translations can follow uniquely matching re-keyed headings. ChangesStale Free Translation Review
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant SegmentListView
participant SegmentFreeTranslationInput
participant AnalysisStore
participant analysisSlice
SegmentListView->>SegmentFreeTranslationInput: Pass stale translations for a segment
SegmentFreeTranslationInput->>AnalysisStore: Request keep or discard
AnalysisStore->>analysisSlice: Dispatch stale translation action
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change shows stale free translations in place with keep and discard controls. No merge-blocking risk was identified in the supplied evidence. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
6b67cdf to
1d520b4
Compare
There was a problem hiding this comment.
LGTC 😉
Heavily relied on AI for this review 🤖
⛏️ Is any of this worth an entry in user-questions.md?
Screenshots would definitely be nice for this sort of UI change 🙂. A few small things inline and one question, none blocking.
@myieye reviewed 18 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on alex-rawlings-yyc and jasonleenaylor).
src/components/AnalysisStore.tsx line 1059 at r1 (raw file):
return useSelector((state: AnalysisRootState) => selectSegmentHasApprovedTranslation(state.analysis, segmentId),
Suggestion: this runs an unmemoized .some() over every link, once per mounted input, on every store change. useSegmentsWithApprovedTranslation below already reads a memoized set; this could just read .has(segmentId) off that instead.
src/utils/stale-free-translations.ts line 104 at r1 (raw file):
const { verse, offset } = positionOf(translation.segmentId); const places = placesByVerse.get(verse) ?? vanishedHeadingPlaces(verse, placesByVerse); if (!places) return [];
A translation whose verse the book no longer holds is kept in storage but can't be seen or discarded anywhere. Is that something we are at all worried about?
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
No user-questions.md entry: there's no one outside the team to put these to yet. Screenshots are below.
@alex-rawlings-yyc made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on jasonleenaylor and myieye).
src/components/AnalysisStore.tsx line 1059 at r1 (raw file):
Previously, myieye (Tim Haasdyk) wrote…
Suggestion: this runs an unmemoized
.some()over every link, once per mounted input, on every store change.useSegmentsWithApprovedTranslationbelow already reads a memoized set; this could just read.has(segmentId)off that instead.
Done: the hook reads .has(segmentId) off the memoized set, and selectSegmentHasApprovedTranslation is gone.
src/utils/stale-free-translations.ts line 104 at r1 (raw file):
Previously, myieye (Tim Haasdyk) wrote…
A translation whose verse the book no longer holds is kept in storage but can't be seen or discarded anywhere. Is that something we are at all worried about?
Yes, but later: #349 left a wholly deleted verse's translation unreachable on purpose, for #143 (orphaned analyses) to pick up.
|
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Thanks for the screenshot 🤓
@myieye made 2 comments and resolved 2 discussions.
Reviewable status: 13 of 18 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc and jasonleenaylor).
src/components/SegmentFreeTranslationInput.tsx line 178 at r3 (raw file):
<Button data-testid="stale-free-translation-keep" onClick={() => {
With r3 a blank box keeps the row's own sentence, which is the right call. The non-blank case still goes the other way: with several stale translations listed and something typed in the box, Keep beside one of them approves that record but overwrites its text with what I typed. So the sentence next to "Keep" is the one thing that goes away. I see the intent (reuse the record so its other languages carry), but the label promises the opposite.
Suggestion: take the "commit the box" branch only when a stale translation fills the box (adopted). For listed rows, Keep always approves its own sentence, as the blank case now does, and is disabled while the draft differs from the box's starting text, with a line under the caption like "Save or clear your translation to keep one of these instead." Typing then saves as a new record on blur as today, and "other languages carry over" still works via Keep first, then edit. That also collapses the three-way branch here to one check on adopted. The test "keeps the listed stale translation chosen with a translation typed beside it" would flip to asserting Keep is disabled.
Not blocking, since it takes a fairly specific sequence to hit.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 1 comment.
Reviewable status: 13 of 18 files reviewed, 1 unresolved discussion (waiting on jasonleenaylor and myieye).
src/components/SegmentFreeTranslationInput.tsx line 178 at r3 (raw file):
Previously, myieye (Tim Haasdyk) wrote…
With r3 a blank box keeps the row's own sentence, which is the right call. The non-blank case still goes the other way: with several stale translations listed and something typed in the box, Keep beside one of them approves that record but overwrites its text with what I typed. So the sentence next to "Keep" is the one thing that goes away. I see the intent (reuse the record so its other languages carry), but the label promises the opposite.
Suggestion: take the "commit the box" branch only when a stale translation fills the box (
adopted). For listed rows, Keep always approves its own sentence, as the blank case now does, and is disabled while the draft differs from the box's starting text, with a line under the caption like "Save or clear your translation to keep one of these instead." Typing then saves as a new record on blur as today, and "other languages carry over" still works via Keep first, then edit. That also collapses the three-way branch here to one check onadopted. The test "keeps the listed stale translation chosen with a translation typed beside it" would flip to asserting Keep is disabled.Not blocking, since it takes a fairly specific sequence to hit.
Done. Keep beside a listed row now always keeps that row's own sentence, and is disabled while anything non-blank is typed. One change to the hint: it reads "Clear your translation to keep one of these instead", since saving approves the typed translation, which hides Keep altogether.
da99e6f to
ff54b38
Compare
2 participants
Closes #374. Part of #349.
A segment shows its stale free translations for review in its own box. A lone stale translation with text in the active language, standing in for an approved one, fills the input in stale styling, with Keep (re-approve it for the text as it now reads) and Discard; editing it and committing approves the result, its other languages included, while clearing it drops only that language's text and leaves the translation stale. Any other stale translations — several, one beside an approved translation, or one with no text in the active language — are listed under the input instead, with Keep offered only while the segment has no approval.
A translation whose segment vanished, such as a merged-away verse or a removed split, shows in the segment now covering where it began. This includes the reader's own boundary edits, which re-anchoring stales. A vanished heading's translation shows on its verse's heading of the same marker, else at the start of its verse. A translation of a verse the book no longer holds shows nowhere.
Re-anchoring follows a heading's translation to the heading of the same marker in its verse that reads exactly its old text, when adding or removing an earlier heading has shifted its id; where none or several do, it stays and goes stale.
The segment height estimate charges stale review rows, including the lines a long stale translation wraps onto. Checked in the running app.
This change is
Summary by CodeRabbit