Add UI that enables user to gloss tokens#77
Open
alex-rawlings-yyc wants to merge 3 commits into
Open
Conversation
- Add gloss input to TokenChip (word tokens only): bordered chip now stacks surface text above an editable gloss field - Thread `glosses` (Record<string, string>) and `onGlossChange(tokenId, value)` props down through TokenChip → PhraseBox → SegmentView → ContinuousView - Introduce a phrase-window around the focused index in ContinuousView to cap rendered PhraseBoxes and reduce overhead on large books - Replace SegmentView's `onClick` with `onSelect(ref, tokenId?)` so the clicked token id propagates up to Interlinearizer - Add `activePhraseIndex` prop to ContinuousView for external phrase jumps when a segment-view token is clicked in continuous-scroll mode - Update tests to cover gloss propagation, `activePhraseIndex` jumps, and both phrase-window boundary branches
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR expands token-level gloss editing and external phrase focus control: TokenChip/PhraseBox/SegmentView/ContinuousView APIs and implementations now carry gloss state and callbacks, continuous view window-renders around an externally driven focus index, and Interlinearizer coordinates shared glosses, focus preservation, and scrolling; tests were updated and extended accordingly. ChangesGloss editing and phrase focus
Sequence Diagram(s)sequenceDiagram
participant User
participant SegmentView
participant ContinuousView
participant TokenChip
participant Interlinearizer
User->>SegmentView: click token
SegmentView->>Interlinearizer: onSelect(ref, tokenId)
Interlinearizer->>Interlinearizer: compute activePhraseIndex
Interlinearizer->>ContinuousView: activePhraseIndex
ContinuousView->>ContinuousView: update focusPhraseIndex
ContinuousView->>Interlinearizer: onFocusPhraseIndexChange
Interlinearizer->>SegmentView: focusedTokenId
User->>TokenChip: type gloss
TokenChip->>Interlinearizer: onGlossChange(tokenId, value)
Interlinearizer->>ContinuousView: glosses map
Interlinearizer->>SegmentView: glosses map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Open
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/PhraseBox.tsx (1)
54-63:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAvoid nesting gloss
<input>controls inside a<button>phrase wrapper.
PhraseBoxnow wrapsTokenChip(which renders<input>) inside a button. Interactive-in-interactive nesting is invalid and can break focus/click semantics for gloss editing.Suggested direction
- if (onClick) { - return ( - <button + if (onClick) { + return ( + <div className={`${baseClass} tw:cursor-pointer tw:text-left tw:hover:bg-muted/30`} data-focus-state={isFocused ? 'focused' : 'default'} data-phrase-box="true" onClick={() => onClick?.(index)} - tabIndex={-1} - type="button" + role="button" + tabIndex={0} > {innerContent} - </button> + </div> ); }(Then add keyboard handlers for Enter/Space to preserve keyboard activation behavior.)
🤖 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` around lines 54 - 63, PhraseBox currently renders a native <button> around innerContent (which can include TokenChip that renders an <input>), creating invalid interactive-in-interactive nesting; replace the <button> wrapper with a non-interactive container (e.g., a <div> or <span>) that carries the existing className/baseClass, data-* attributes and onClick handler (onClick?.(index)), and add explicit keyboard handlers on that wrapper (onKeyDown) to trigger the same activation when Enter or Space are pressed so keyboard activation is preserved; ensure tabIndex and data-focus-state are preserved and do not interfere with the TokenChip input focus, and keep TokenChip unchanged so its native input remains accessible.
🧹 Nitpick comments (1)
src/components/SegmentView.tsx (1)
54-54: ⚡ Quick winAdd JSDoc for
handleTokenClickto match repo function-doc requirements.This helper is undocumented, but repo rules require JSDoc on every function/method in
src/**/*.{ts,tsx}.As per coding guidelines: “Every function and method — exported or internal — must have a JSDoc block with summary sentence,
@paramfor every parameter,@returnsdescribing return value (omit for void/Promise), and@throwsfor every error condition”.🤖 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/SegmentView.tsx` at line 54, Add a JSDoc block above the handleTokenClick declaration describing its purpose and parameters: document the summary sentence (what handleTokenClick does), include an `@param` for tokenId (string) and an `@param` for any closed-over variables if desired (mention ref is used), omit `@returns` because it returns void, and add an `@throws` noting that it may propagate errors thrown by onSelect; reference the handleTokenClick function, the onSelect callback, the ref variable, and the tokenId parameter so the maintainer can locate and verify the documentation.
🤖 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/ContinuousView.test.tsx`:
- Around line 297-298: Remove the unnecessary type assertions "as const" on the
test object fields (writingSystem and type) in the ContinuousView tests; instead
assign the string literals directly (e.g., writingSystem: 'en', type: 'word') —
the Token/Book types already enforce correct typings, so update the test data in
the ContinuousView.test.tsx test case where those fields are set to use plain
literals and remove the "as const" casts.
In `@src/__tests__/components/Interlinearizer.test.tsx`:
- Around line 256-267: The test invokes capturedSegmentViewPropsList[1].onSelect
which triggers handleSegmentSelect and updates state via setFocusedTokenId; wrap
that invocation in React's act() to ensure the state update is flushed before
assertions. Update the test in Interlinearizer.test.tsx so the call to
capturedSegmentViewPropsList[1].onSelect?.({ book: 'GEN', chapter: 1, verse: 2
}, 'GEN 1:2:0') is executed inside act(() => { ... }); then keep the
expect(mockSetScrRef) assertion after the act block to avoid React state-update
warnings.
In `@src/__tests__/components/PhraseBox.test.tsx`:
- Around line 194-199: The test's second expectation for onGlossChange is
asserting the last typed character instead of the cumulative gloss value; update
the assertion for handleGlossChange (used in this test) so the second call
expects ('token-1', 'hi') rather than ('token-1', 'i') — locate the
userEvent.type invocation and the subsequent
expect(handleGlossChange).toHaveBeenNthCalledWith assertions and change the
second expected gloss value to the full accumulated string.
In `@src/__tests__/components/SegmentView.test.tsx`:
- Around line 221-226: The test's second gloss callback assertion is wrong: when
typing via userEvent.type the handler should receive the cumulative text, so
update the expectation for handleGlossChange to assert the second call was with
'In' (e.g., expect(handleGlossChange).toHaveBeenNthCalledWith(2, 'tok-0', 'In'))
instead of 'n'; locate the assertions referencing handleGlossChange in the
SegmentView.test.tsx block around the userEvent.type call and change the second
expected argument accordingly.
In `@src/components/ContinuousView.tsx`:
- Around line 155-156: The code stores an external index (activePhraseIndex)
into focusPhraseIndex and later uses it to compute windowStartTokenIndex from
phraseEntries[windowStart], but it never clamps or validates the external index;
update the assignment sites (where activePhraseIndex is applied to
focusPhraseIndex and any similar assignments in the regions around lines 215–251
and 319–325) to validate and clamp the incoming index to the valid range [0,
phraseEntries.length - 1] before storing, and handle empty phraseEntries by
setting a safe default (e.g., undefined or 0) to avoid dereferencing
phraseEntries[windowStart]; ensure the clamp logic references activePhraseIndex,
focusPhraseIndex, phraseEntries, and windowStart so out-of-range or stale
indices cannot cause phraseEntries[windowStart] === undefined at runtime.
In `@src/components/TokenChip.tsx`:
- Around line 33-38: In TokenChip, the input uses defaultValue which makes it
uncontrolled and prevents future gloss prop updates from showing; change it to a
controlled input by replacing defaultValue={gloss ?? ''} with value={gloss ??
''} and keep onChange={(e) => onGlossChange?.(e.target.value)} so updates flow
from the gloss prop into the textbox; if TokenChip intentionally needs local
editing state, introduce a controlled local state initialized from gloss and
sync props via useEffect, but otherwise remove any local uncontrolled logic so
gloss prop and onGlossChange stay authoritative.
---
Outside diff comments:
In `@src/components/PhraseBox.tsx`:
- Around line 54-63: PhraseBox currently renders a native <button> around
innerContent (which can include TokenChip that renders an <input>), creating
invalid interactive-in-interactive nesting; replace the <button> wrapper with a
non-interactive container (e.g., a <div> or <span>) that carries the existing
className/baseClass, data-* attributes and onClick handler (onClick?.(index)),
and add explicit keyboard handlers on that wrapper (onKeyDown) to trigger the
same activation when Enter or Space are pressed so keyboard activation is
preserved; ensure tabIndex and data-focus-state are preserved and do not
interfere with the TokenChip input focus, and keep TokenChip unchanged so its
native input remains accessible.
---
Nitpick comments:
In `@src/components/SegmentView.tsx`:
- Line 54: Add a JSDoc block above the handleTokenClick declaration describing
its purpose and parameters: document the summary sentence (what handleTokenClick
does), include an `@param` for tokenId (string) and an `@param` for any closed-over
variables if desired (mention ref is used), omit `@returns` because it returns
void, and add an `@throws` noting that it may propagate errors thrown by onSelect;
reference the handleTokenClick function, the onSelect callback, the ref
variable, and the tokenId parameter so the maintainer can locate and verify the
documentation.
🪄 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: 8fba8299-8ba6-4983-ad70-29001326b68a
📒 Files selected for processing (10)
src/__tests__/components/ContinuousView.test.tsxsrc/__tests__/components/Interlinearizer.test.tsxsrc/__tests__/components/PhraseBox.test.tsxsrc/__tests__/components/SegmentView.test.tsxsrc/__tests__/components/TokenChip.test.tsxsrc/components/ContinuousView.tsxsrc/components/Interlinearizer.tsxsrc/components/PhraseBox.tsxsrc/components/SegmentView.tsxsrc/components/TokenChip.tsx
Contributor
Author
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 5 discussions.
Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/SegmentView.tsx`:
- Around line 95-104: SegmentView is passing unstable per-render values to
MemoizedPhraseBox (tokens={[token]} and onClick={() =>
handleTokenClick(token.id)}), which defeats memoization; stabilize these by (a)
wrapping handleTokenClick in useCallback (or reusing the ContinuousView pattern)
so the onClick prop is stable, and (b) stop creating a new array per
token—either pass the single token (change Prop name from tokens to token on
MemoizedPhraseBox/PhraseBox) or precompute a stable tokens array/share a
reference—so MemoizedPhraseBox receives stable props and can properly memoize.
In `@src/components/TokenChip.tsx`:
- Around line 33-40: The input for the gloss in TokenChip is rendered as a
controlled field even when the optional onGlossChange handler is missing,
causing confusing UX and React warnings; update the input element (the <input>
used in TokenChip that binds value={gloss} and onChange={(e) =>
onGlossChange?.(e.target.value)}) to explicitly set readOnly to true when
onGlossChange is not provided (readOnly={!onGlossChange}) so the field appears
and behaves as non-editable unless an edit handler is passed.
🪄 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: 8f2c537f-015f-4f39-ad08-f0d52b44820a
📒 Files selected for processing (4)
src/__tests__/components/Interlinearizer.test.tsxsrc/__tests__/components/TokenChip.test.tsxsrc/components/SegmentView.tsxsrc/components/TokenChip.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/components/TokenChip.test.tsx
- src/tests/components/Interlinearizer.test.tsx
…nChip` type guard to ensure that `input` always has a handler; update docs
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
1 participant
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.
glosses(Record<string, string>) andonGlossChange(tokenId, value)props down through TokenChip → PhraseBox → SegmentView → ContinuousViewonClickwithonSelect(ref, tokenId?)so the clicked token id propagates up to InterlinearizeractivePhraseIndexprop to ContinuousView for external phrase jumps when a segment-view token is clicked in continuous-scroll modeactivePhraseIndexjumps, and both phrase-window boundary branchesThis change is
Summary by CodeRabbit
New Features
Improvements
Tests