Skip to content

Link tokens#91

Draft
alex-rawlings-yyc wants to merge 13 commits into
mainfrom
link-tokens
Draft

Link tokens#91
alex-rawlings-yyc wants to merge 13 commits into
mainfrom
link-tokens

Conversation

@alex-rawlings-yyc
Copy link
Copy Markdown
Contributor

@alex-rawlings-yyc alex-rawlings-yyc commented May 29, 2026

This change is Reviewable

Summary by CodeRabbit

  • New Features

    • Added phrase editing mode with create, update, and delete functionality
    • Added phrase gloss (translation) input field with blur-to-save behavior
    • Added visual arc overlays to display phrase relationships and nesting
    • Added link/unlink controls between adjacent token groups
    • Added split phrase functionality with visual preview of freed tokens
    • Improved token focus navigation with windowed rendering and fade transitions
    • Added confirm-unlink dialog for phrase deletion
  • Tests

    • Comprehensive test coverage for phrase hooks, components, and utilities

@alex-rawlings-yyc alex-rawlings-yyc self-assigned this May 29, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ba0aad8f-4bb2-4217-8403-61cd65d79ee5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch link-tokens

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@alex-rawlings-yyc alex-rawlings-yyc linked an issue May 29, 2026 that may be closed by this pull request
…om lint git workflow (it was causing a failure)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/__tests__/components/Interlinearizer.test.tsx (1)

360-393: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use a multi-token target verse here so the focus-transfer assertion is discriminating.

Both tests target GEN 1:2, but this fixture only has one word token (GEN 1:2:0). That means they still pass if Interlinearizer drops the prior focus and simply falls back to the active verse’s first token during the mode switch. Using a verse with at least two word tokens and selecting a non-first token would make these assertions actually prove focus preservation.

Also applies to: 438-470

🤖 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/Interlinearizer.test.tsx` around lines 360 - 393,
The test currently selects token 'GEN 1:2:0' from a verse that only has one
token so the focus-transfer assertion isn’t discriminating; update the test(s)
in Interlinearizer.test.tsx (the case asserting focusedTokenRef after switching
to continuousScroll, and the similar case later) to use a verse from the
GEN_1_MULTI_BOOK fixture that contains multiple word tokens and select a
non-first token (e.g., choose a token id like 'GEN 1:3:1' or another multi-token
verse and pick index 1) when calling onSelect from capturedSegmentViewPropsList,
so that after rerender and inspecting
capturedContinuousViewProps.focusedTokenRef the assertion proves true focus
preservation rather than defaulting to the first token.
🧹 Nitpick comments (4)
src/components/PhraseBox.tsx (1)

195-195: 💤 Low value

Consider extracting default empty Map to a module-level constant.

Creating new Map() inline as a default value produces a new reference on every render, causing handleEditAdd (line 301) to be recreated unnecessarily when this prop is not provided.

♻️ Proposed refactor
+/** Stable empty map used as default for tokenDocOrder prop. */
+const EMPTY_TOKEN_DOC_ORDER: ReadonlyMap<string, number> = new Map();
+
 export function PhraseBox({
   // ... other props
-  tokenDocOrder = new Map(),
+  tokenDocOrder = EMPTY_TOKEN_DOC_ORDER,
 }: PhraseBoxProps) {
🤖 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/components/PhraseBox.tsx` at line 195, The default prop value
tokenDocOrder = new Map() creates a fresh Map each render causing stable
callbacks like handleEditAdd to be recreated; extract a module-level constant
(e.g., DEFAULT_TOKEN_DOC_ORDER) initialized once to new Map() and use that
constant as the default value where tokenDocOrder is defined in PhraseBox (so
references to tokenDocOrder and the handleEditAdd callback remain stable across
renders).
src/components/ArcOverlay.tsx (1)

7-8: ⚡ Quick win

Document the ArcSplitTarget fields.

splitAfterTokenRef is not self-evident from the field name/type alone, so this exported type needs member-level JSDoc to satisfy the repo rule.

📚 Suggested doc update
 /** Identifies one specific arc boundary by phrase id and the token immediately before the split. */
-export type ArcSplitTarget = { phraseId: string; splitAfterTokenRef: string };
+export type ArcSplitTarget = {
+  /** Phrase containing the arc boundary. */
+  phraseId: string;
+  /** Token ref immediately before the split boundary. */
+  splitAfterTokenRef: string;
+};

As per coding guidelines, src/**/*.{ts,tsx,d.ts}: "Type declarations (interfaces, type aliases, enums) must have a JSDoc summary on the type itself and on each field or member whose purpose is not self-evident from its name and type."

🤖 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/components/ArcOverlay.tsx` around lines 7 - 8, Add JSDoc for the exported
type ArcSplitTarget and its members: provide a one-line summary for
ArcSplitTarget and member-level JSDoc for phraseId (describe that it is the
unique identifier of the phrase/arc) and for splitAfterTokenRef (explain it is
the token reference immediately before the split—i.e., the token after which the
arc boundary occurs, and what form the token ref takes). Ensure the comments
appear immediately above the type and each field so they satisfy the repo rule
for type/member documentation.
src/__tests__/components/ArcOverlay.test.tsx (1)

304-330: ⚡ Quick win

Assert the candidate styling branch directly.

This test never inspects the arc stroke, so it would still pass if candidatePhraseIds stopped affecting styling entirely. Please assert the rendered path style/class for the candidate case instead of only checking that a path exists.

💡 Tighten the assertion
   // candidatePhraseIds affects arc stroke colour only; the split button requires hovered/focused.
   expect(screen.queryByTestId('split-arc-btn')).not.toBeInTheDocument();
-  // The arc path is still rendered.
-  expect(document.querySelector('path')).toBeInTheDocument();
+  const pathEl = document.querySelector('path');
+  expect(pathEl).toBeInTheDocument();
+  expect(pathEl?.getAttribute('style')).toContain('white');
🤖 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/ArcOverlay.test.tsx` around lines 304 - 330, The
test for ArcOverlay currently only checks that a path exists and the split
button is not rendered, so it would pass even if candidate styling stopped
working; update the assertion to verify that the rendered arc path reflects
candidatePhraseIds styling by selecting the path rendered by ArcOverlay (the one
created by makeArcPath / rendered when candidatePhraseIds includes 'p1') and
asserting its stroke attribute or CSS class matches the candidate style used by
ArcOverlay (e.g., the candidate arc class or expected stroke color string); keep
the existing checks for split-arc-btn and document path presence, but replace or
add an assertion that document.querySelector('path') (or the path element found
from the render) has the correct className or getAttribute('stroke') value so
the test fails if candidatePhraseIds no longer affects styling.
src/utils/token-layout.ts (1)

173-179: ⚡ Quick win

Add JSDoc for emitSlot.

This named helper is missing the required summary and @param docs, unlike the rest of the module.

As per coding guidelines, "Every exported or internal function and method must have a JSDoc block with: (1) a summary sentence describing what the function does and why it exists; (2) @param for every parameter; (3) @returns describing the return value (omit only for void/Promise); (4) @throws for every error condition the caller must handle."

🤖 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/token-layout.ts` around lines 173 - 179, The helper emitSlot is
missing a JSDoc block; add a JSDoc immediately above the emitSlot declaration
that (1) gives a one-sentence summary describing that emitSlot creates and
appends a 'slot' TokenUnit to units for the boundary between groups, (2)
documents the `@param` nextGroup {TokenGroup|undefined} describing its role as the
following group in the slot, (3) documents that the function returns void (no
return value) and instead mutates the outer scope by pushing into units and
clearing pendingPunctuation, and (4) states there are no thrown errors (or lists
any if applicable). Include references to the mutated symbols (units,
pendingPunctuation, lastGroup) in the description so readers understand side
effects.
🤖 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 `@jest.config.ts`:
- Line 41: The coverage exclude path is wrong: replace the pattern
'!src/components/phrase-mode.ts' in jest.config.ts with the correct path for the
introduced file (e.g., '!src/types/phrase-mode.ts') so the intended file is
actually excluded from coverage; locate the string literal
'!src/components/phrase-mode.ts' and update it to the correct module path used
in the codebase (or remove the entry if exclusion is no longer needed).

In `@src/__tests__/components/EditPhraseControls.test.tsx`:
- Around line 7-14: The test fixture uses a type assertion "as const" which
violates the no-type-assertion rule; update the PHRASE_MODE declaration in the
EditPhraseControls test to use a proper type instead (e.g., declare PHRASE_MODE
with an explicit PhraseMode type annotation or use the TypeScript "satisfies
PhraseMode" clause) so the shape remains the same but without using "as const";
you should find the PHRASE_MODE variable in the EditPhraseControls test file and
replace the assertion accordingly.

In `@src/__tests__/components/PhraseBox.test.tsx`:
- Around line 345-359: The test's second assertion expects the wrong dispatched
value because typing via userEvent.type sends incremental accumulated values;
update the expectation in the PhraseBox test so mockUseGlossDispatch's spy is
asserted with the accumulated value "hi" on the second keystroke (i.e., change
the second expect in the test that references mockUseGlossDispatch/spy to expect
'hi' rather than 'i'), keeping the other assertions (first call 'h', token id
'token-1', and source 'Hello') intact.
- Around line 172-181: The beforeEach needs to re-seed the phrase-gloss hooks
because resetMocks: true clears module-level mockReturnValue setups; update the
beforeEach to call mockUseGloss.mockReturnValue('') and
mockUseGlossDispatch.mockReturnValue(jest.fn()) (or appropriate defaults) so
usePhraseGloss() and usePhraseGlossDispatch() return deterministic values for
each test; ensure the names referenced are mockUseGloss and mockUseGlossDispatch
and keep other mockReturnValue calls for mockUsePhraseLinkForToken and
mockUsePhraseDispatch as already present.

In `@src/__tests__/components/PhraseStripParts.test.tsx`:
- Around line 319-326: Update the test in PhraseStripParts.test.tsx to actually
assert hover handlers are not attached: render PhraseGroup with
allowHover={false} and mocked onHoverEnter/onHoverLeave, select the wrapper span
(e.g., document.querySelector('span')), dispatch mouseEnter and mouseLeave
events (or use fireEvent.mouseEnter/fireEvent.mouseLeave) against that element,
and assert that onHoverEnter and onHoverLeave were NOT called; keep the existing
existence assertion but add the event dispatches and
expect(onHoverEnter).not.toHaveBeenCalled() and
expect(onHoverLeave).not.toHaveBeenCalled() to verify suppression of hover when
allowHover is false.

In `@src/__tests__/components/SegmentView.test.tsx`:
- Around line 286-310: The test currently only checks that the child token
buttons render, which doesn't verify grouping; update the test in
SegmentView.test.tsx (the case exercising SegmentView with mockUsePhraseLinkMap
and sharedLink) to assert the number of rendered PhraseBox wrappers instead of
just the buttons — e.g., query for the mocked PhraseBox wrapper elements (or
ensure both token buttons share the same wrapper) and expect exactly one wrapper
for the sharedLink; keep using the same mockUsePhraseLinkMap/phraseLinkMap setup
and replace the two getByRole button assertions with an assertion that counts
PhraseBox wrapper instances (or verifies a single wrapper contains both token
buttons).

In `@src/__tests__/hooks/useArcPaths.test.ts`:
- Around line 8-10: The test resets only computeAllArcPaths in beforeEach
causing computeStripTopPadding (mocked in jest.mock) to be cleared by
resetMocks; update the beforeEach in src/__tests__/hooks/useArcPaths.test.ts to
import computeStripTopPadding via jest.requireMock('../../utils/phrase-arc')
alongside computeAllArcPaths and call computeStripTopPadding.mockReturnValue(8)
(in addition to computeAllArcPaths.mockReturnValue({ paths: [], levelByPhraseId:
new Map(), maxLevel: 0 })) so useArcPaths's dependency on
stripTopPadding/useLayoutEffect sees the expected value.

In `@src/__tests__/utils/phrase-arc.test.ts`:
- Around line 412-417: In the test helper function makePhraseLink remove the
TypeScript const assertion from the status property: replace the property
assignment status: 'approved' as const with a plain string value status:
'approved' so the helper no longer uses a type assertion and complies with the
no-type-assertion lint rule; update the object returned by makePhraseLink
accordingly (function name: makePhraseLink, property: status).

In `@src/components/TokenChip.tsx`:
- Around line 42-46: The render is mutating state because TokenChip updates
prevOnRemoveRef and calls setIsRemoveHovered(false) during render; move that
logic into a useEffect that watches onRemove: inside the effect compare
prevOnRemoveRef.current with onRemove, if different update
prevOnRemoveRef.current = onRemove, and if onRemove is falsy and isRemoveHovered
is true call setIsRemoveHovered(false); remove the render-time
setIsRemoveHovered call and only keep the ref initialization useRef(onRemove) in
the component body.

In `@src/hooks/useArcPaths.ts`:
- Around line 65-69: The state update in setArcPaths currently compares only the
geometry string (`d`) to decide whether to reuse prev, which can keep old
ArcPath objects when phrase-id or split-boundary changed; update the equality
check inside setArcPaths to compare the full ArcPath identity (e.g., include
properties like phraseId and splitBoundary along with d) by mapping prev and
next to a composite key (or deep-equal the objects) and return prev only when
all identity fields match so hover styling and split actions point to the
correct phrase/boundary.

In `@src/store/analysisSlice.ts`:
- Around line 161-168: When adding or updating a PhraseAnalysisLink with status
'approved' (in the createPhrase and updatePhrase reducers), enforce the
invariant by scanning existing state.analysis.phraseAnalysisLinks for any link
that shares any tokenRef with the new/updated link and changing those
conflicting links' status so they are no longer 'approved' (e.g., set to
'unapproved' or the appropriate non-approved state) before pushing/replacing the
newLink; also ensure the same logic is applied wherever phraseAnalysisLinks are
mutated so selectPhraseLinkByTokenRef no longer needs to resolve multiple
approved matches. Use the reducer blocks that construct newLink (the code shown
in the reducer at the top of the diff) and the updatePhrase reducer to locate
and fix these mutations.
- Around line 177-181: The updatePhrase reducer updates the link.tokens but
doesn't update the corresponding PhraseAnalysis.surfaceText, causing metadata
drift; modify updatePhrase (in analysisSlice) so after finding the link (from
phraseAnalysisLinks) you recompute and set link.analysis.surfaceText (or
link.surfaceText depending on shape) from the new tokens — e.g., derive the
surface text by mapping tokens to their text fields and joining appropriately or
call the existing tokens->text helper if one exists — ensuring
PhraseAnalysis.surfaceText stays in sync whenever tokens change.

In `@src/tailwind.css`:
- Around line 54-64: Stylelint is rejecting the custom Tailwind `@utility` blocks
(link-slot, token-row, arc-container) due to scss/at-rule-no-unknown; either
whitelist the `@utility` at-rule in the Stylelint configuration (preferred) or
locally suppress the rule around these blocks; specifically update the stylelint
config to allow at-rule "utility" or add the appropriate scss/at-rule-no-unknown
disable/enable comments surrounding the `@utility` blocks for link-slot,
token-row, and arc-container so lint passes.

---

Outside diff comments:
In `@src/__tests__/components/Interlinearizer.test.tsx`:
- Around line 360-393: The test currently selects token 'GEN 1:2:0' from a verse
that only has one token so the focus-transfer assertion isn’t discriminating;
update the test(s) in Interlinearizer.test.tsx (the case asserting
focusedTokenRef after switching to continuousScroll, and the similar case later)
to use a verse from the GEN_1_MULTI_BOOK fixture that contains multiple word
tokens and select a non-first token (e.g., choose a token id like 'GEN 1:3:1' or
another multi-token verse and pick index 1) when calling onSelect from
capturedSegmentViewPropsList, so that after rerender and inspecting
capturedContinuousViewProps.focusedTokenRef the assertion proves true focus
preservation rather than defaulting to the first token.

---

Nitpick comments:
In `@src/__tests__/components/ArcOverlay.test.tsx`:
- Around line 304-330: The test for ArcOverlay currently only checks that a path
exists and the split button is not rendered, so it would pass even if candidate
styling stopped working; update the assertion to verify that the rendered arc
path reflects candidatePhraseIds styling by selecting the path rendered by
ArcOverlay (the one created by makeArcPath / rendered when candidatePhraseIds
includes 'p1') and asserting its stroke attribute or CSS class matches the
candidate style used by ArcOverlay (e.g., the candidate arc class or expected
stroke color string); keep the existing checks for split-arc-btn and document
path presence, but replace or add an assertion that
document.querySelector('path') (or the path element found from the render) has
the correct className or getAttribute('stroke') value so the test fails if
candidatePhraseIds no longer affects styling.

In `@src/components/ArcOverlay.tsx`:
- Around line 7-8: Add JSDoc for the exported type ArcSplitTarget and its
members: provide a one-line summary for ArcSplitTarget and member-level JSDoc
for phraseId (describe that it is the unique identifier of the phrase/arc) and
for splitAfterTokenRef (explain it is the token reference immediately before the
split—i.e., the token after which the arc boundary occurs, and what form the
token ref takes). Ensure the comments appear immediately above the type and each
field so they satisfy the repo rule for type/member documentation.

In `@src/components/PhraseBox.tsx`:
- Line 195: The default prop value tokenDocOrder = new Map() creates a fresh Map
each render causing stable callbacks like handleEditAdd to be recreated; extract
a module-level constant (e.g., DEFAULT_TOKEN_DOC_ORDER) initialized once to new
Map() and use that constant as the default value where tokenDocOrder is defined
in PhraseBox (so references to tokenDocOrder and the handleEditAdd callback
remain stable across renders).

In `@src/utils/token-layout.ts`:
- Around line 173-179: The helper emitSlot is missing a JSDoc block; add a JSDoc
immediately above the emitSlot declaration that (1) gives a one-sentence summary
describing that emitSlot creates and appends a 'slot' TokenUnit to units for the
boundary between groups, (2) documents the `@param` nextGroup
{TokenGroup|undefined} describing its role as the following group in the slot,
(3) documents that the function returns void (no return value) and instead
mutates the outer scope by pushing into units and clearing pendingPunctuation,
and (4) states there are no thrown errors (or lists any if applicable). Include
references to the mutated symbols (units, pendingPunctuation, lastGroup) in the
description so readers understand side effects.
🪄 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: b6202b55-2436-4b06-96c8-31170dca9a25

📥 Commits

Reviewing files that changed from the base of the PR and between 713e095 and 338010d.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (42)
  • __mocks__/lucide-react.tsx
  • jest.config.ts
  • src/__tests__/components/AnalysisStore.test.tsx
  • src/__tests__/components/ArcOverlay.test.tsx
  • src/__tests__/components/ContinuousView.test.tsx
  • src/__tests__/components/EditPhraseControls.test.tsx
  • src/__tests__/components/Interlinearizer.test.tsx
  • src/__tests__/components/InterlinearizerLoader.test.tsx
  • src/__tests__/components/PhraseBox.test.tsx
  • src/__tests__/components/PhraseStripParts.test.tsx
  • src/__tests__/components/SegmentView.test.tsx
  • src/__tests__/components/TokenChip.test.tsx
  • src/__tests__/components/TokenLinkIcon.test.tsx
  • src/__tests__/components/UnlinkPhraseConfirm.test.tsx
  • src/__tests__/hooks/useArcPaths.test.ts
  • src/__tests__/hooks/useArcSplitHandler.test.ts
  • src/__tests__/hooks/useCandidatePhraseIds.test.ts
  • src/__tests__/store/analysisSlice.test.ts
  • src/__tests__/utils/phrase-arc.test.ts
  • src/__tests__/utils/token-layout.test.ts
  • src/components/AnalysisStore.tsx
  • src/components/ArcOverlay.tsx
  • src/components/ContinuousView.tsx
  • src/components/EditPhraseControls.tsx
  • src/components/Interlinearizer.tsx
  • src/components/InterlinearizerLoader.tsx
  • src/components/PhraseBox.tsx
  • src/components/PhraseStripParts.tsx
  • src/components/SegmentView.tsx
  • src/components/TokenChip.tsx
  • src/components/TokenLinkIcon.tsx
  • src/components/UnlinkPhraseConfirm.tsx
  • src/components/component-types.ts
  • src/hooks/useArcPaths.ts
  • src/hooks/useArcSplitHandler.ts
  • src/hooks/useCandidatePhraseIds.ts
  • src/store/analysisSlice.ts
  • src/tailwind.css
  • src/types/phrase-mode.ts
  • src/types/typeGuards.ts
  • src/utils/phrase-arc.ts
  • src/utils/token-layout.ts
💤 Files with no reviewable changes (1)
  • src/components/component-types.ts

Comment thread jest.config.ts Outdated
Comment thread src/__tests__/components/EditPhraseControls.test.tsx
Comment thread src/__tests__/components/PhraseBox.test.tsx
Comment thread src/__tests__/components/PhraseBox.test.tsx
Comment thread src/__tests__/components/PhraseStripParts.test.tsx
Comment thread src/components/TokenChip.tsx Outdated
Comment thread src/hooks/useArcPaths.ts
Comment thread src/store/analysisSlice.ts
Comment thread src/store/analysisSlice.ts
Comment thread src/tailwind.css
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`
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.github/workflows/lint.yml (1)

41-43: ⚡ Quick win

Add explanatory comment for the optional dependency requirement.

The test and publish workflows both include comments explaining why --omit=optional cannot be used (SWC and lightningcss native binaries are needed by the build toolchain). Adding the same explanatory comment here would improve maintainability and prevent someone from re-introducing --omit=optional in the future.

📝 Suggested comment to match test.yml/publish.yml pattern
       - name: Install extension dependencies
         working-directory: extension-repo
+        # Cannot --omit=optional: `@swc/core` (for Jest) and lightningcss (for Tailwind) need their
+        # optional native binaries to run linters and formatting checks.
         run: npm ci --ignore-scripts
🤖 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 @.github/workflows/lint.yml around lines 41 - 43, Add an inline explanatory
comment above the "Install extension dependencies" step (the job step with name
"Install extension dependencies", working-directory "extension-repo" and run
"npm ci --ignore-scripts") explaining why you must not use --omit=optional
(e.g., SWC and lightningcss require native optional dependencies for the build
toolchain), matching the pattern used in test.yml/publish.yml so future editors
don't reintroduce --omit=optional.
🤖 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.

Nitpick comments:
In @.github/workflows/lint.yml:
- Around line 41-43: Add an inline explanatory comment above the "Install
extension dependencies" step (the job step with name "Install extension
dependencies", working-directory "extension-repo" and run "npm ci
--ignore-scripts") explaining why you must not use --omit=optional (e.g., SWC
and lightningcss require native optional dependencies for the build toolchain),
matching the pattern used in test.yml/publish.yml so future editors don't
reintroduce --omit=optional.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 77a8b7b7-951a-4a28-b5b5-0153df04091b

📥 Commits

Reviewing files that changed from the base of the PR and between 338010d and e5c4d87.

📒 Files selected for processing (25)
  • .github/workflows/lint.yml
  • jest.config.ts
  • src/__tests__/components/AnalysisStore.test.tsx
  • src/__tests__/components/ArcOverlay.test.tsx
  • src/__tests__/components/ContinuousView.test.tsx
  • src/__tests__/components/EditPhraseControls.test.tsx
  • src/__tests__/components/Interlinearizer.test.tsx
  • src/__tests__/components/InterlinearizerLoader.test.tsx
  • src/__tests__/components/PhraseBox.test.tsx
  • src/__tests__/components/PhraseStripParts.test.tsx
  • src/__tests__/components/SegmentView.test.tsx
  • src/__tests__/hooks/useArcPaths.test.ts
  • src/__tests__/hooks/useArcSplitHandler.test.ts
  • src/__tests__/utils/phrase-arc.test.ts
  • src/components/AnalysisStore.tsx
  • src/components/ArcOverlay.tsx
  • src/components/ContinuousView.tsx
  • src/components/Interlinearizer.tsx
  • src/components/InterlinearizerLoader.tsx
  • src/components/PhraseBox.tsx
  • src/components/SegmentView.tsx
  • src/components/TokenChip.tsx
  • src/hooks/useArcPaths.ts
  • src/hooks/useArcSplitHandler.ts
  • src/store/analysisSlice.ts
💤 Files with no reviewable changes (1)
  • jest.config.ts
🚧 Files skipped from review as they are similar to previous changes (18)
  • src/tests/components/EditPhraseControls.test.tsx
  • src/tests/hooks/useArcPaths.test.ts
  • src/tests/components/InterlinearizerLoader.test.tsx
  • src/tests/components/AnalysisStore.test.tsx
  • src/tests/components/ArcOverlay.test.tsx
  • src/hooks/useArcSplitHandler.ts
  • src/components/InterlinearizerLoader.tsx
  • src/components/AnalysisStore.tsx
  • src/components/ArcOverlay.tsx
  • src/components/Interlinearizer.tsx
  • src/components/TokenChip.tsx
  • src/tests/components/SegmentView.test.tsx
  • src/components/SegmentView.tsx
  • src/components/PhraseBox.tsx
  • src/store/analysisSlice.ts
  • src/tests/utils/phrase-arc.test.ts
  • src/tests/components/Interlinearizer.test.tsx
  • src/tests/components/PhraseBox.test.tsx

Copy link
Copy Markdown
Contributor Author

@alex-rawlings-yyc alex-rawlings-yyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-rawlings-yyc resolved 4 discussions.
Reviewable status: 0 of 44 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

@alex-rawlings-yyc
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Link tokens together into the same phrase

1 participant