User-defined segment boundaries#126
Conversation
📝 WalkthroughWalkthroughThis PR adds user-defined segment boundaries stored as deltas from default verse segmentation. It introduces boundary controls, resegmentation, phrase-aware cross-segment linking, verse labels, draft autosave, project persistence, and updated view settings. ChangesUser-defined segment boundary editing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
alex-rawlings-yyc
force-pushed
the
segment-boundaries
branch
from
cfa0a17 to
69f8147
Compare
alex-rawlings-yyc
force-pushed
the
segment-boundaries
branch
from
f43875e to
c75337e
Compare
alex-rawlings-yyc
force-pushed
the
segment-boundaries
branch
5 times, most recently
from
11eb61a to
1c939f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 0 of 50 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
alex-rawlings-yyc
force-pushed
the
segment-boundaries
branch
from
5e4e6f9 to
a05ca88
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 2 discussions.
Reviewable status: 4 of 66 files reviewed, 1 unresolved discussion.
|
The segment join-split interaction when holding ALT is really slick!! Simple hover-text on the split and join buttons would help clarify what's about to happen when they're clicked. A bigger concern is discoverability. We may want the segment join buttons always visible, with hover text that also mentions the ALT action for splitting segments. |
|
⛏️ I think that (at least in continuous view, but perhaps also in the full-detail segment view) the verse superscript should only show where the verse actual starts, not at mid-verse segment starts (like the second 1 below). 
|
|
We had also discussed an alternate (from the verse superscripts) for segment view, in which the verse(-range) shows up in an initial gutter/column instead. |
|
Claude Sonnet analysis of two Devin flags: 1.
|
|
@imnasnainaec Clarify for me: You said:
Do we want join buttons always visible, with splitting buttons only visible with ALT? If that's the case, would the split tooltip be on the join buttons? And should the join buttons be enabled even when ALT isn't held? |
Join button always enabled; splitting only visible with ALT. Join and split buttons both with very simple hover text when ALT is held. Otherwise, join button with additional hover text about holding ALT to enable splitting segments. |
alex-rawlings-yyc
force-pushed
the
segment-boundaries
branch
from
c86049d to
7196254
Compare
autosaveSegmentation now clears the persisted segmentation field when the delta has no removed or added starts, not just when it is undefined — an empty delta represents the default segmentation and shouldn't persist a redundant custom object. Extract getSegmentationDispatch helper in the loader test and tighten the move-boundary assertion to check the exact normalized delta.
- TokenLinkIcon: guard the focus-is-next boundary pull against stranding a word-less segment when only punctuation precedes the pulled word (mirror the focus-is-prev branch); add coverage - ContinuousView: defer the segmentation-reconcile commit while an internal-nav smooth scroll is in flight so a mid-glide boundary edit no longer snaps it - segmentation: cache per-book lookups (WeakMap) so an edit walks the book once - useDraftProject: reuse isDefaultSegmentation instead of an inline empty check - SegmentationDelta/formerBoundaries: correct the anchor-convention docs Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
associated with superverse
header band Source verse numbers verbatim from the USJ verse marker through the parser pipeline; render them as inline superscripts (chapter-qualified at chapter transitions) in both display modes. Drop the top-left segment labels, retire the chapterLabelInVerse setting, and delete segment-labels. Show the current chapter as a scroll-tracked "Book N" band above the list so it survives segment culling.
Hold Alt to reveal UnfoldHorizontal split markers on eligible word-word gaps; punctuation travel (resolveSplitAnchor) picks the anchor side. Add useAltHeld/AltHeldContext, rework baseline-text mode into a byte-exact token walk, and drop the merge/split hover previews for a mergeHint tooltip.
Verse labels now render in a fixed three-row phrase slot (shared by both strips, so ContinuousView gains inline verse numbers), replaced by the Alt-gated merge/split button while Alt is held. Swap Fold*/Unfold* icons for the mirrored Merge/Split pair, drop the former-boundary tick, and add a dashed rail in the segment-list gap and a caret in the baseline text.
Make the row-gap merge rail inert until Alt is held, give inactive segment cards a faint border, and restructure the phrase-slot column so verse numbers always show (peeking above), punctuation aligns with surface text, and the Alt-gated merge/split button overlays the punctuation without shifting the strip.
labels Preserve empty verses through custom segmentation; carry chapter on VerseStart so chapter qualifiers stay correct for empty/whitespace-led verses; resolve verse containment against covered verses (no cross-chapter phantom over-claim); render the top-of-window merge control; align the baseline split caret with the dispatched anchor; share the verse-label helpers between both strips.
paranext-core added a trailing sourceProjectId element to UseWebViewScrollGroupScrRefHook; alias ScrollGroupTuple from the hook's return type and pad the stubs so tests type-check against it.
Join (merge) controls now render always-visible and always-enabled in both strips with a dedicated slot row below the link icon; split stays Alt-gated, and merge tooltips carry a Mac-inclusive Alt-split hint. Suppress the duplicate verse superscript at a mid-verse split's continuation piece via a new VerseStart.isContinuation flag. Also coalesce the segment-list scroll handler to one read per frame, and fix the active-verse fallback to resolve an over-shoot in a chapter opened by a cross-chapter segment.
Show each segment's verse/range label (5, 5a, 5b–7, 29–2:1) in a fixed-width left gutter in both display modes, alongside the inline verse superscripts. The label revives buildSegmentLabels, now derived from the verseStarts covered-verse set rather than the startRef/endRef interval so cross-chapter merges don't over-claim.
alex-rawlings-yyc
force-pushed
the
segment-boundaries
branch
from
7196254 to
45faf36
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
labels Suppress the no-op merge above an empty verse and drop a wiped book's boundaries; render buried verse superscripts in group-based strips; accept Unicode-dash ranges; confine Alt re-renders to split-gap leaves.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 23 files and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: 25 of 75 files reviewed, 2 unresolved discussions (waiting on alex-rawlings-yyc).
a discussion (no related file):
The gutter verses should be a view-toggle alternative to the in-line superscripts, not in addition to them. And we don't want to reintroduce the letters. The gutter can just give the verse-range that the segments overlaps with: e.g.,
1
1
2-3
4
4-6
7
8
a discussion (no related file):
🐛 (may or may not be a regression)
If there is a discontinuous phrase arc on the in-focus phrase. When continuous view is turned on, the arc renders in the correct place, but then doesn't jump when the tokens it touches jump.
In this screenshot, the arc is supposed to span from "and" to "on the".
src/__tests__/main.test.ts line 987 at r9 (raw file):
await handler('proj-id', JSON.stringify(stubAnalysis), 'null'); // eslint-disable-next-line no-null/no-null -- asserting the clear-boundaries sentinel is forwarded
⛏️ This eslint-disable... looks stale, from when the args below were all on 1 line.
Add a `showVerseGutter` view option (persisted project setting, default off) that swaps the segment-view verse display between two mutually-exclusive styles: the left verse-range gutter column or the inline verse superscripts, never both. Previously the gutter always rendered alongside the superscripts. Rewrite `buildSegmentLabels` to drop the split-verse portion lettering: the gutter now shows the plain verse range a segment overlaps (`2–3`, `29–2:1`, or a bare number), so two adjacent portions of one verse both read that verse's bare number rather than `1a`/`1b`. Also drop a stale `no-null/no-null` eslint-disable in main.test.ts left over from when the asserted args were on one line; the sentinel `null` a few lines down keeps its own disable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-centering after an instant jump in continuous view ran for a fixed LINK_SLOT_TRANSITION_MS (200ms) hold. But the render window mounts dozens of phrase groups whose glosses, morpheme rows, and arcs finish laying out asynchronously over many frames; content that widens to the left of the focus keeps shifting the focused box sideways well past 200ms. The hold expired before that late reflow landed, stranding the focus (and its arcs) off-center — the box drifted ~940px right after the hold stopped, with the scroll offset unchanged, and clicking a visible token then triggered the browser's native scroll-into-view "jump". Make holdCentered settle-aware: observe the strip content row and treat each resize as a fresh LINK_SLOT_TRANSITION_MS quiet-period extension, re-centering on every reflow and only stopping once the content goes quiet. A HOLD_CENTERED_MAX_MS cap bounds the total hold so a strip that never stabilizes can't spin the rAF loop forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every TokenChip fetched the gloss-input placeholder via its own async useLocalizedStrings; with field-sizing: content each chip mounted ~14px too narrow and widened when the string resolved, shifting the strip under already-measured arcs (arc drawn off-screen after load, right endpoint inward, per-nav jitter). The arc hook's observer watches the fixed-width fade wrapper, which never resizes for these shifts, so no corrective measure ran — and the 2-deep echo history could swallow a genuine correction as a phantom oscillation. Fetch the placeholder once per strip and deliver it through PhraseStripContext (as a prop into the memoized TokenChip), so chips render at final width on their first frame. Make useArcPaths settle-aware as a backstop: re-verify at 200/400/800ms after the last measurement until geometry reaches a fixed point, and allow up to two consecutive flip-backs to a recent signature before freezing — genuine collapse-then-recover corrections now apply while the cross-row gutter period-2 oscillation still terminates.
Match the intra-phrase gap column's punctuation/icon stacking to the PhraseSlot's, swap Link2Off for the height-matched Unlink2 glyph, and make the icon wrapper inline-flex so its line-box leading no longer pushes the unlink button below its inter-phrase counterpart.
The re-center hold disconnected its ResizeObserver when the tick loop went quiet, so the ~300-500ms gloss/arc reflow stranded the focus off-center on first load. Decouple observer lifetime from the loop: stay connected until HOLD_CENTERED_MAX_MS and re-arm on late reflows.
Remove bug-fix backstory and comments that restate the code or a test assertion; keep tightened invariants. Comments only — no behavior change.
Previously, imnasnainaec (D. Ror.) wrote…
Think I fixed this |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useSegmentWindow.ts (1)
563-569: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdjust
rangeduring boundary edits to prevent segments from vanishing.Because a boundary edit alters the
segmentsarray length, an unadjustedrangeslice will point to shifted elements. If a user merges the top-most visible segment with its predecessor, the newly merged segment shifts to an index beforerange.startand falls out of thewindowSegmentsarray entirely, causing it to vanish from the screen.To fix this, track the ID of the window's starting segment and shift the range delta accordingly when an edit occurs.
🐛 Proposed fix to shift the range synchronously
const isBoundaryEdit = segments !== prev.segments && segmentationVersion !== prev.segmentationVersion; if (isBoundaryEdit || consumeInternalNavRef.current(currentScrRef)) { + if (isBoundaryEdit) { + const r = rangeRef.current; + const oldStartSeg = prev.segments[r.start]; + const oldPrevSeg = r.start > 0 ? prev.segments[r.start - 1] : undefined; + + let newStartIndex = segments.findIndex((s) => s.id === oldStartSeg?.id); + if (newStartIndex === -1 && oldPrevSeg) { + // If the old start segment was merged into its predecessor, the predecessor's ID survived. + newStartIndex = segments.findIndex((s) => s.id === oldPrevSeg.id); + } + + if (newStartIndex !== -1 && newStartIndex !== r.start) { + const delta = newStartIndex - r.start; + setRange({ start: Math.max(0, r.start + delta), end: r.end + delta }); + } + } setDisplayScrRef(currentScrRef); setDisplayFocusedTokenRef(focusedTokenRefRef.current); return;🤖 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/hooks/useSegmentWindow.ts` around lines 563 - 569, Update the boundary-edit handling in the segment-window logic around isBoundaryEdit to track the starting segment ID of the current window and synchronously adjust range by the resulting index delta when segments change. Ensure a top-visible segment merged with its predecessor remains included in windowSegments instead of being dropped due to the shifted range.
🤖 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.
Inline comments:
In `@src/__tests__/components/TokenChip.suggestions.test.tsx`:
- Around line 264-265: Update the comment near the approved-token
alternative-row behavior to use the American English spelling “gray” instead of
“grey,” without changing its meaning or code.
In `@src/hooks/useArcPaths.ts`:
- Around line 179-216: Update the signature generated by signatureOf to include
split-button geometry fields midX, midY, runLeft, and runRight, so changes to
these values are treated as new measurements and reach setArcPaths instead of
matching history[0]. Add a regression test covering midX-only drift and
verifying the split-button coordinates update.
- Around line 224-225: Add JSDoc blocks to the named helpers `key` in
src/hooks/useArcPaths.ts (lines 224-225), `pump` in
src/__tests__/hooks/useArcPaths.test.ts (lines 82-90), and `apply` in
src/components/InterlinearizerLoader.tsx (lines 302-304); document `key` with
`@param p` and `@returns`, `pump` with `@returns`, and `apply` with `@param
next`.
In `@src/utils/segment-labels.ts`:
- Around line 43-46: Update labelForSegment to handle an empty verseStarts array
before accessing first.number. Return the appropriate existing fallback label
for empty segments, while preserving the current first/last comparison and
numbering behavior for non-empty arrays.
---
Outside diff comments:
In `@src/hooks/useSegmentWindow.ts`:
- Around line 563-569: Update the boundary-edit handling in the segment-window
logic around isBoundaryEdit to track the starting segment ID of the current
window and synchronously adjust range by the resulting index delta when segments
change. Ensure a top-visible segment merged with its predecessor remains
included in windowSegments instead of being dropped due to the shifted range.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10d6e85a-0c19-4bad-85b2-9ffb5a8ca4b7
📒 Files selected for processing (61)
__mocks__/lucide-react.tsxcontributions/localizedStrings.jsoncontributions/projectSettings.jsonsrc/__tests__/components/AnalysisStore.test.tsxsrc/__tests__/components/ContinuousView.test.tsxsrc/__tests__/components/Interlinearizer.test.tsxsrc/__tests__/components/InterlinearizerLoader.test.tsxsrc/__tests__/components/PhraseBox.test.tsxsrc/__tests__/components/PhraseStripParts.test.tsxsrc/__tests__/components/SegmentView.test.tsxsrc/__tests__/components/TokenChip.suggestions.test.tsxsrc/__tests__/components/TokenLinkIcon.test.tsxsrc/__tests__/components/controls/ViewOptionsDropdown.test.tsxsrc/__tests__/components/modals/ProjectModals.test.tsxsrc/__tests__/components/test-helpers.tsxsrc/__tests__/hooks/useArcPaths.test.tssrc/__tests__/hooks/useDraftProject.test.tssrc/__tests__/hooks/useInterlinearizerBookData.test.tssrc/__tests__/hooks/useSegmentWindow.test.tssrc/__tests__/main.test.tssrc/__tests__/parsers/papi/bookTokenizer.test.tssrc/__tests__/parsers/papi/usjBookExtractor.test.tssrc/__tests__/services/projectStorage.test.tssrc/__tests__/test-helpers.tssrc/__tests__/utils/segment-labels.test.tssrc/__tests__/utils/segmentation.test.tssrc/__tests__/utils/token-layout.test.tssrc/__tests__/utils/verse-ref.test.tssrc/__tests__/utils/verse-superscripts.test.tssrc/components/AltHeldContext.tsxsrc/components/AnalysisStore.tsxsrc/components/ContinuousView.tsxsrc/components/Interlinearizer.tsxsrc/components/InterlinearizerLoader.tsxsrc/components/PhraseBox.tsxsrc/components/PhraseStripContext.tsxsrc/components/PhraseStripParts.tsxsrc/components/SegmentListView.tsxsrc/components/SegmentView.tsxsrc/components/SegmentationStore.tsxsrc/components/TokenChip.tsxsrc/components/TokenLinkIcon.tsxsrc/components/controls/ViewOptionsDropdown.tsxsrc/components/modals/ProjectModals.tsxsrc/hooks/useAltHeld.tssrc/hooks/useArcPaths.tssrc/hooks/useDraftProject.tssrc/hooks/usePhraseStripSetup.tssrc/hooks/useSegmentWindow.tssrc/main.tssrc/parsers/papi/resegmentBook.tssrc/parsers/papi/usjBookExtractor.tssrc/types/interlinearizer.d.tssrc/types/token-layout.tssrc/types/view-options.tssrc/utils/phrase-arc.tssrc/utils/segment-labels.tssrc/utils/segmentation.tssrc/utils/split-anchor.tssrc/utils/verse-ref.tssrc/utils/verse-superscripts.ts
💤 Files with no reviewable changes (2)
- src/tests/services/projectStorage.test.ts
- src/tests/parsers/papi/usjBookExtractor.test.ts
🚧 Files skipped from review as they are similar to previous changes (43)
- src/hooks/useAltHeld.ts
- src/tests/test-helpers.ts
- src/components/AltHeldContext.tsx
- src/tests/utils/verse-superscripts.test.ts
- src/types/token-layout.ts
- src/components/PhraseStripContext.tsx
- src/tests/main.test.ts
- src/tests/utils/verse-ref.test.ts
- src/utils/split-anchor.ts
- src/tests/hooks/useInterlinearizerBookData.test.ts
- src/tests/utils/segmentation.test.ts
- src/parsers/papi/resegmentBook.ts
- contributions/localizedStrings.json
- src/components/SegmentationStore.tsx
- src/parsers/papi/usjBookExtractor.ts
- src/utils/verse-ref.ts
- src/utils/verse-superscripts.ts
- src/tests/components/test-helpers.tsx
- src/tests/parsers/papi/bookTokenizer.test.ts
- src/tests/utils/token-layout.test.ts
- src/tests/components/AnalysisStore.test.tsx
- src/components/AnalysisStore.tsx
- src/components/TokenLinkIcon.tsx
- src/main.ts
- src/utils/segmentation.ts
- src/utils/phrase-arc.ts
- src/tests/components/TokenLinkIcon.test.tsx
- src/tests/hooks/useDraftProject.test.ts
- src/components/modals/ProjectModals.tsx
- src/tests/components/PhraseBox.test.tsx
- src/components/ContinuousView.tsx
- src/types/interlinearizer.d.ts
- src/tests/components/modals/ProjectModals.test.tsx
- src/hooks/useDraftProject.ts
- src/tests/components/SegmentView.test.tsx
- src/components/SegmentView.tsx
- src/tests/components/PhraseStripParts.test.tsx
- src/components/PhraseStripParts.tsx
- src/tests/hooks/useSegmentWindow.test.ts
- src/tests/components/ContinuousView.test.tsx
- src/components/Interlinearizer.tsx
- src/tests/components/Interlinearizer.test.tsx
- src/tests/components/InterlinearizerLoader.test.tsx
| // On an already-approved token every alternative is a grey "promote" row, not a blue "accept" | ||
| // row: there is nothing to accept, only candidates to promote to. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use American English: replace “grey” with “gray.”
As per coding guidelines, “Use American English throughout code, comments, JSDoc, and documentation.”
🤖 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/__tests__/components/TokenChip.suggestions.test.tsx` around lines 264 -
265, Update the comment near the approved-token alternative-row behavior to use
the American English spelling “gray” instead of “grey,” without changing its
meaning or code.
Source: Coding guidelines
| const signature = signatureOf(paths, maxLevel, leftPadding, rightPadding); | ||
| const history = recentArcSignaturesRef.current; | ||
|
|
||
| // Decide whether to apply this measurement. `history[0]` is always the currently-applied | ||
| // signature, so matching it is a harmless fixed point. Matching a deeper entry is either a | ||
| // period-2 oscillation echo or a genuine recovery from a transient mis-measure; allow a | ||
| // bounded number of consecutive flip-backs (the recovery case), then freeze (the loop case). | ||
| let apply: boolean; | ||
| if (force) { | ||
| flipCountRef.current = 0; | ||
| apply = true; | ||
| } else if (signature === history[0]) { | ||
| flipCountRef.current = 0; | ||
| apply = false; | ||
| } else if (history.includes(signature)) { | ||
| apply = flipCountRef.current < MAX_CONSECUTIVE_SIGNATURE_FLIPS; | ||
| if (apply) flipCountRef.current += 1; | ||
| } else { | ||
| flipCountRef.current = 0; | ||
| apply = true; | ||
| } | ||
|
|
||
| // Settle verification: geometry can drift with no resize event on the observed container, so | ||
| // every pass arms a delayed re-measure. Ordinary passes restart the schedule; verification | ||
| // passes that found nothing new advance to the next (longer) delay so the chain stops after | ||
| // SETTLE_VERIFY_DELAYS_MS runs out — including in the frozen-oscillation state, where each | ||
| // verification skips and the chain dies instead of re-fueling the loop. | ||
| if (!isVerify || apply) settleVerifyRoundRef.current = 0; | ||
| else settleVerifyRoundRef.current += 1; | ||
| clearTimeout(settleVerifyTimerRef.current); | ||
| const round = settleVerifyRoundRef.current; | ||
| if (round < SETTLE_VERIFY_DELAYS_MS.length) { | ||
| settleVerifyTimerRef.current = setTimeout(() => { | ||
| measurePass(container, false, true); | ||
| }, SETTLE_VERIFY_DELAYS_MS[round]); | ||
| } | ||
|
|
||
| if (!apply) return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include split-button geometry in the measurement signature.
A pass where only midX, midY, runLeft, or runRight changes matches history[0] and returns at Line 216. The richer setArcPaths key never runs, leaving the split button at stale coordinates. Include these fields in signatureOf and add a midX-only drift regression test.
Proposed fix
- .map((p) => `${p.phraseId}:${p.splitAfterTokenRef}:${p.d}`)
+ .map(
+ (p) =>
+ `${p.phraseId}:${p.splitAfterTokenRef}:${p.d}:${p.midX}:${p.midY}:${p.runLeft}:${p.runRight}`,
+ )Also applies to: 220-228
🤖 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/hooks/useArcPaths.ts` around lines 179 - 216, Update the signature
generated by signatureOf to include split-button geometry fields midX, midY,
runLeft, and runRight, so changes to these values are treated as new
measurements and reach setArcPaths instead of matching history[0]. Add a
regression test covering midX-only drift and verifying the split-button
coordinates update.
| const key = (p: ArcPath) => | ||
| `${p.phraseId}:${p.splitAfterTokenRef}:${p.d}:${p.midX}:${p.midY}:${p.runLeft}:${p.runRight}`; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required JSDoc to newly introduced named helpers.
src/hooks/useArcPaths.ts#L224-L225: documentkey, including@param pand@returns.src/__tests__/hooks/useArcPaths.test.ts#L82-L90: documentpump, including@returns.src/components/InterlinearizerLoader.tsx#L302-L304: documentapply, including@param next.
As per coding guidelines, “Every named function and method — exported or internal — must have a JSDoc block.”
📍 Affects 3 files
src/hooks/useArcPaths.ts#L224-L225(this comment)src/__tests__/hooks/useArcPaths.test.ts#L82-L90src/components/InterlinearizerLoader.tsx#L302-L304
🤖 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/hooks/useArcPaths.ts` around lines 224 - 225, Add JSDoc blocks to the
named helpers `key` in src/hooks/useArcPaths.ts (lines 224-225), `pump` in
src/__tests__/hooks/useArcPaths.test.ts (lines 82-90), and `apply` in
src/components/InterlinearizerLoader.tsx (lines 302-304); document `key` with
`@param p` and `@returns`, `pump` with `@returns`, and `apply` with `@param
next`.
Source: Coding guidelines
| function labelForSegment(verseStarts: readonly VerseStart[]): SegmentLabel { | ||
| const first = verseStarts[0]; | ||
| const last = verseStarts[verseStarts.length - 1]; | ||
| if (first === last) return first.number; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against empty verseStarts to prevent runtime crashes.
If verseStarts is ever empty (e.g., an empty segment or a non-verse boundary), first evaluates to undefined, and accessing first.number will throw a TypeError and crash the view.
🛡️ Proposed fix
function labelForSegment(verseStarts: readonly VerseStart[]): SegmentLabel {
+ if (verseStarts.length === 0) return '';
const first = verseStarts[0];
const last = verseStarts[verseStarts.length - 1];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function labelForSegment(verseStarts: readonly VerseStart[]): SegmentLabel { | |
| const first = verseStarts[0]; | |
| const last = verseStarts[verseStarts.length - 1]; | |
| if (first === last) return first.number; | |
| function labelForSegment(verseStarts: readonly VerseStart[]): SegmentLabel { | |
| if (verseStarts.length === 0) return ''; | |
| const first = verseStarts[0]; | |
| const last = verseStarts[verseStarts.length - 1]; | |
| if (first === last) return first.number; |
🤖 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/utils/segment-labels.ts` around lines 43 - 46, Update labelForSegment to
handle an empty verseStarts array before accessing first.number. Return the
appropriate existing fallback label for empty segments, while preserving the
current first/last comparison and numbering behavior for non-empty arrays.
2 participants
This change is
Summary by CodeRabbit
saveAnalysis(including clearing with"null").