Show stale free translations in place (#374) - #378
alex-rawlings-yyc wants to merge 15 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 |
1b6635b to
b8ad4a2
Compare
… charge the no-text placeholder in stale review heights (#374)
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.
|
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