Hardenings for #91#93
Merged
Merged
Conversation
Replace the heuristic with an explicit lastDisplayUpdateWasInternalRef flag set at decision time in the focusedTokenRef effect. The scroll effect now reads only refs, removing the stale closure reads and the eslint-disable comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Both are referentially stable (memoized dispatch and React state setter), so adding them costs nothing at runtime but removes the implicit reliance on that stability guarantee. Update the eslint-disable comment to explain why phraseMode itself remains intentionally omitted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
alex-rawlings-yyc
approved these changes
Jun 4, 2026
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
Contributor
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).
This was referenced Jun 4, 2026
Merged
alex-rawlings-yyc
added a commit
that referenced
this pull request
Jun 11, 2026
* Add UI for linking phrases together * Adjustments to line styles * Add link buttons and improve styling (TESTS NOT UPDATED YET) * Rework focusRef, update tests * Symmetrize token ref focus * Hoist out phrase strip parts, improve split button handling * Add useArcPaths hook * Improve split border colour clarity * Add useArcSplitHandler hook * Update/add tests * Removed `--omit=optional` from Install extension dependencies step from lint git workflow (it was causing a failure) * Refactor phrase lookup to use id-keyed map for O(1) access Replace the token-ref-keyed linear search (`[...map.values()].find(l => l.analysisId === id)`) with a new `selectPhraseLinkByAnalysisId` selector and `usePhraseLinkByIdMap` hook, giving O(1) phrase lookup by id in `ArcOverlay`, `useArcSplitHandler`, and `SegmentView`. Also: - Move the phrase-revert effect from `PhraseBox` up to `InterlinearizerInner` so it fires even when all tokens are removed from the phrase - Fix `TokenChip` state sync to use `useEffect` instead of inline mutation during render - Fix `useArcPaths` dep array to use a version counter instead of a spread to satisfy the rules of hooks - Reset `phraseMode` on project switch in `InterlinearizerLoader` - Use a stable empty map constant in `PhraseBox` to avoid breaking memo - Improve arc key to include `phraseId` and `splitAfterTokenRef` * Add clarifying comment about `--omit-optional` omission * Minor adjustments, test coverage improvements * Minor improvements * Improve handling of inter-row arcs, other arc-related bugs and nits * Consolidate duplicated code * Add usePhraseHoverState test suite * Add PhraseStripContext * Add PhraseStrip component and bundle props * Set static control pill location relative to phrase box, clean up comments, consolidate tw classes * Add regions * Add tokenDocOrder prop, fix Set mutation, correct test assertions, extend makePhraseLink fixture, add JSDoc and region markers - Lift tokenDocOrder computation out of SegmentView into callers; add prop to SegmentView and SegmentView tests - Clone incoming Set in handleSplitHoverChange (usePhraseHoverState) so React sees a new reference on every update - Fix two ArcOverlay test assertions that used invalid two-argument not.toHaveBeenCalledWith on a single-argument handler; replace with not.toHaveBeenCalled() - Extend makePhraseLink with an optional surfaceTexts parameter so tests can supply TokenSnapshot.surfaceText values independently of tokenRef - Add JSDoc blocks for handleSplitHoverEnter and handleSplitHoverLeave in ArcOverlay - Update useArcPaths @returns JSDoc to describe ArcPathsResult including requiredRowGapPx - Correct PhraseBox JSDoc: one token remaining (not two) triggers phrase deletion; add region markers to ArcOverlay, PhraseBox, usePhraseHoverState, useArcPaths * Delete empty PhraseAnalysis automatically, tokenDocOrder only considers words, memoize PhraseGroup * Eliminate unnecessary abstraction, simplify computeAllArcPaths * Move edit/unlink controls to new confirm bar, fix edit mode bug, re-derive phrase surface text from new tokens * Fix SegmentView split button not working, improve gap calculation, clean up comments * Prevent 1-token phrases being created from edit mode * Minor adjustments * Rework arc drawing and related things * Cleanup * Minor adjustments * Minor adjustments * Hardenings for #91 (#93) * Harden ContinuousView scroll effect isInternal detection Replace the heuristic with an explicit lastDisplayUpdateWasInternalRef flag set at decision time in the focusedTokenRef effect. The scroll effect now reads only refs, removing the stale closure reads and the eslint-disable comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add updatePhrase and setPhraseMode to revert effect deps Both are referentially stable (memoized dispatch and React state setter), so adding them costs nothing at runtime but removes the implicit reliance on that stability guarantee. Update the eslint-disable comment to explain why phraseMode itself remains intentionally omitted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Document groupRef closure churn and its safe ref-cycling behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Do minor phrase-arc cleanup (#95) * Do minor phrase-arc cleanup * Complete JSDocs * Refactor describeBoxPair into two locally specific function (#96) * Refactor describeBoxPair into two locally specific function * Fix mis-match * Fix onFocusPhrase breaking PhraseBox memoization * Improve visibility in light mode * Add toggles for phrase controls and options dropdown, subdivide components folder, add tooltip for disabled links * Fix punctuation rendering issues * Make SegmentView background clickable, shore up test coverage * Reposition options dropdown on resize * Minor adjustments * Minor adjustments * Improve AGENTS.md * Rename window* in ContinuousView to renderWindow*, improve render window in ContinuousView, improve handling of settings in InterlinearizerLoader, add dedicated mergePhrases reducer to analysisSlice, emit data-last-token-read on all render paths * Standardize spelling * Add REVIEW.md * Suggested cleanup on #91 (#99) * Merge mostly redundant tests * Update settings for VSCode to match our stylelint use * Add JSDocs * Fix pump() overwriting observerCallback via stub constructor new ResizeObserver(() => {}) inside pump() was invoking the stub constructor, replacing the stored observerCallback with a no-op on every call after the first. Replace with a pre-declared stubObserver plain object so pump() never touches the constructor again. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Sort cspell words * Fix brit spelling now covered in AGENTS.md --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix arc misalignment issue when out-of-segment links hidden * Add animation to make things smoother when out-of-segment links are hidden * Fix issue with inputs not focusing their token, add animation to SegmentView * Minor adjustment * Minor adjustments * Fix arc alignment, punctuation routing, and link-button polish; extract test helpers and reorganize types (#100) * Reserve link-slot space when hidden to prevent arc re-alignment Inactive link slots now use visibility:hidden instead of max-width collapse, so toggling hideInactiveLinkButtons no longer shifts phrase box positions. Removes the slotAnimationTick rAF loop and the hideInactiveLinkButtons re-center effect that compensated for the old layout shift. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix punctuation jumping when link icon is hidden link-slot was flex-col, stacking the icon wrapper above the punctuation. Changing to flex-row keeps them side-by-side so punctuation position no longer depends on the icon wrapper's height. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix in-phrase punctuation duplicating on every focus change buildRenderUnits was pushing into the shared TokenGroup.punctuationBetween arrays, which are owned by memoized objects that outlive a single render. Replace push() with assignment so each call resets the gap array instead of accumulating. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix crash on punctuation after the last token of a phrase group After the gapIndex fix, pendingIntraGroup pointed to a non-existent gap when the current token was the last in its group, causing a push() onto undefined when post-phrase punctuation arrived. Guard the assignment so the last token clears pendingIntraGroup instead; punctuation then falls into the inter-group LinkSlot as intended. Adds a regression test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Cut 'yet' * Tune link button and arc contrast Link buttons: use foreground/60 (active) and foreground/20 (inactive) instead of muted-foreground at 100%/50%, widening the visual gap between actionable and suppressed slots. Arcs: reduce focused-phrase stroke opacity from 1.0 to 0.75 so the current phrase's arcs are less dominant relative to other phrases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix punctuation layout and reduce arc contrast gap Punctuation layout: revert link-slot to flex-col so punctuation sits below the icon. Remove visibility:hidden from the icon wrapper and use opacity:0 instead; add display:inline-flex + min-height to ensure the wrapper always reserves the same vertical space even when TokenLinkIcon returns null. Arc contrast: reduce focused-phrase stroke opacity 0.75→0.60 and raise dimmed-phrase stroke opacity 0.8→1.0 (border is already low-contrast), narrowing the visual gap between the two states. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Guard mergePhrases reducer against targetPhraseId === absorbedPhraseId Without the guard the update succeeds but the subsequent delete removes the phrase that was just updated, leaving orphaned token references. Invariant was previously enforced only by callers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Move controls/modals tests into matching subdirectories Aligns __tests__/components/controls/ and __tests__/components/modals/ with src/components/controls/ and src/components/modals/. Updates relative import paths accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Disable pointer events and hide from a11y tree when link icon is suppressed opacity:0 alone left the invisible TokenLinkIcon hittable and focusable. Add pointerEvents:'none' and aria-hidden when suppressLinkIcon is true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Import defaultScrRef from test-helpers instead of redefining it Two test files redeclared the same constant that test-helpers.ts already exports. Remove the duplicates and import the canonical export. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Extract makeWordToken fixture factory into test-helpers The same minimal word-token shape was defined six times across three test files under three different local names (mkWord, mkToken, mk). A single exported factory with a stable API replaces all six. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Extract emptyAnalysis factory into src/types/emptyFactories.ts Consolidates six independently constructed all-empty TextAnalysis objects across two source files and four test files into a single shared factory. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Reorganize types: rename to kebab-case, add @file headers, move token-layout types - emptyFactories.ts → empty-factories.ts, typeGuards.ts → type-guards.ts - Extract FocusContext/SlotFocusInfo/TokenGroup/LinkSlot/RenderUnit from utils/token-layout.ts into types/token-layout.ts - Add @file JSDoc to all three type files - Update all import sites (20 files) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Unexport ArcStrokeProps — no consumers outside phrase-arc.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: extract makeStubProject and reuse project fixtures * Fix idempotent punctuation routing in buildRenderUnits * Update comments * Make common withAnalysisStore test-helper * Extract in-file project setting helpers --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Minor adjustments * bugfix and cleanup: opacity hide, doc cleanup, helper functions (#101) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix test failure --------- Co-authored-by: D. Ror. <imnasnainaec@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Merged
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
2 participants
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.
This change is
Summary by CodeRabbit