Link tokens#91
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (81)
📝 WalkthroughWalkthroughAdds phrase-mode state, store APIs, token-layout and arc utilities, phrase-strip components, view/loader rewrites, Tailwind utilities, Jest/jsdom ResizeObserver test stub, and extensive unit/integration tests for phrase interactions. ChangesPhrase interaction pipeline
Phrase interaction pipeline validation
Sequence Diagram(s)sequenceDiagram
participant Interlinearizer
participant Views as Continuous/Segment Views
participant Strip as PhraseStrip/ArcOverlay
participant Store as AnalysisStore (Redux)
Interlinearizer->>Views: pass focusedTokenRef, phraseMode, lookup maps
Views->>Strip: render stripItems, arcPaths, hover/focus inputs
Strip->>Store: create/update/delete/mergePhrase, writePhraseGloss via dispatch hooks
Views->>Store: splitPhraseAtBoundary (ArcOverlay/TokenLinkIcon)
Store-->>Views: selectors (phraseLink maps, gloss) update views
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
alex-rawlings-yyc
force-pushed
the
link-tokens
branch
from
99ae079 to
338010d
Compare
…om lint git workflow (it was causing a failure)
![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.
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`
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
Contributor
Author
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 4 discussions.
Reviewable status: 0 of 44 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
* 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>
imnasnainaec
requested changes
Jun 5, 2026
imnasnainaec
left a comment
imnasnainaec
left a comment
Contributor
There was a problem hiding this comment.
@imnasnainaec made 1 comment.
Reviewable status: 15 of 78 files reviewed, 5 unresolved discussions (waiting on alex-rawlings-yyc).
a discussion (no related file):
🐛 Linking is misaligned when out-of-segment links are hidden:
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
Contributor
Author
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 3 discussions.
Reviewable status: 15 of 78 files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
Contributor
Author
Previously, imnasnainaec (D. Ror.) wrote…
Would we rather the links become invisible and leave space or the gaps be closed? I'm leaning towards the gaps being closed, but it will be more work |
Contributor
Author
Previously, alex-rawlings-yyc (Alex Rawlings) wrote…
Never mind, this is a redraw timing issue. Fix is simple |
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.
imnasnainaec
requested changes
Jun 8, 2026
imnasnainaec
left a comment
imnasnainaec
left a comment
Contributor
There was a problem hiding this comment.
@imnasnainaec reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: 20 of 78 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
a discussion (no related file):
Previously, alex-rawlings-yyc (Alex Rawlings) wrote…
Never mind, this is a redraw timing issue. Fix is simple
The arcs still surf all over the place when "Hide out-of-segment link buttons" is toggled on and off. I think it's fine to keep the space there rather than close the gap.
imnasnainaec
reviewed
Jun 8, 2026
imnasnainaec
left a comment
imnasnainaec
left a comment
Contributor
There was a problem hiding this comment.
@imnasnainaec partially reviewed 24 files.
Reviewable status: 44 of 78 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
…ct 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>
Contributor
Author
|
@imnasnainaec GitHub's auth API is down so we may have to override the reviewable requirement, assuming that merging your sub-PR made this PR mergeable in your eyes. If it is, go ahead and merge. EDIT: GitHub auth API seems to be back up for now so Reviewable is a go |
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.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
Contributor
Author
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 3 discussions.
Reviewable status: 30 of 86 files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
imnasnainaec
approved these changes
Jun 11, 2026
imnasnainaec
left a comment
imnasnainaec
left a comment
Contributor
There was a problem hiding this comment.
@imnasnainaec partially reviewed 56 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
imnasnainaec
approved these changes
Jun 11, 2026
imnasnainaec
left a comment
imnasnainaec
left a comment
Contributor
There was a problem hiding this comment.
@imnasnainaec resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
This was referenced Jun 11, 2026
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
New Features
Improvements