Add the read-only analysis catalog panel - #234
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Interlinearizer adds a localized, resizable Analysis Catalog with usage navigation, persisted panel state, command wiring, writing-system support, and expanded test coverage. ChangesAnalysis Catalog feature
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to The panel behavior is localized, but resizing retains bounded RTL-direction and duplicate-write risks that warrant explicit owner awareness or follow-up before merge; no high-impact correctness or availability risk is indicated. Sequence Diagram(s)sequenceDiagram
participant Menu
participant InterlinearizerLoader
participant AnalysisStore
participant AnalysisCatalogPanel
participant CatalogRowView
Menu->>InterlinearizerLoader: openAnalysisCatalog
InterlinearizerLoader->>AnalysisStore: provide book catalog state
AnalysisCatalogPanel->>AnalysisStore: select catalog rows
AnalysisStore-->>AnalysisCatalogPanel: return catalog rows
AnalysisCatalogPanel->>CatalogRowView: render rows
CatalogRowView->>InterlinearizerLoader: navigate to selected usage
🚥 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 |
41c174d to
89835a1
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Hoists the analysis store above the cross-book fade curtain so a jump to a usage in another book cannot dim the panel. Search, sort, filter, and row windowing are deferred to #231.
Also commits a released drag width from a ref rather than from inside the setDragWidth updater, which React may run more than once.
An unparseable analysis-language tag threw out of Intl.Collator and blanked the whole view; the resize handle inverted in right-to-left interfaces; and the row toggle's aria-label suppressed the analysis it named. Rows now share one localization subscription instead of one apiece.
A pointer released where the window cannot see it — over a native menu, which takes the pointer with it — left the drag running, so the panel went on resizing under a button-less pointer and committed that width at the next click anywhere. A move reporting no button held now ends the drag at the width it had reached, since that move is the only word the window gets of such a release. Arrow keys stand aside while a drag is in flight. They stepped off the width the drag began at, reporting a width the panel was not showing only for the release to overwrite it; the pointer owns the width while it is held. The drag test helper now dispatches its moves with a held button, which a real one carries and jsdom does not.
The held-focus-request test left the reference on GEN while EXO's view mounted, a state the host never produces; move it to EXO with the jump.
The simulated drag omitted `buttons`, so the move arrived reporting none — which the resize hook reads as a release it never saw, ending the drag before it recorded a width. The panel fell back to its default, and the remount assertion compared that default against itself. Dropping width persistence outright left the test green. Carry `buttons` on the move so the drag resizes, and name the expected width at both ends rather than checking the two renders agree, since the default is what a dead drag and a dropped write alike leave behind.
Any button began a drag: the move handler asks only whether some button is held, so a middle-button press followed the pointer to its release and persisted the width it reached. A right-button press does the same wherever the context menu opens on release rather than on press. Also correct the loader's width-restore comment, which described a move reporting no buttons held while the move beneath it carries one.
456d1bb to
ede5153
Compare
| * Each row owns its own layout so that its detail can be nested inside it. One element per analysis | ||
| * is what lets the list window and be walked by keyboard a row at a time. | ||
| */ | ||
| export default function CatalogRowView({ |
There was a problem hiding this comment.
⛏️ Consider wrapping this in React.memo. applyCatalogQuery filters and sorts without cloning, so rows keep their identity and the remaining props are stable; without memo, every resize-drag frame (setDragWidth fires per mousemove) and every gloss commit reconciles every row's subtree in an unwindowed list. Worth confirming #231 covers memoizing the row alongside windowing.
There was a problem hiding this comment.
Done — MemoizedCatalogRowView, matching the TokenChip / SegmentView pattern. Your premise checks out: applyCatalogQuery is filter + toSorted, so rows keep identity, and the rest of the props are stable (onUsageSelect is a useCallback, localizedStrings comes off the hoisted key array). Filed the windowing half against #231 rather than duplicating it here.
| const hiddenUsageCount = row.usages.length - visibleUsages.length; | ||
|
|
||
| const usageCountLabel = localizedStrings['%interlinearizer_analysisCatalog_usageCount%']; | ||
| const usageCountInBookLabel = formatReplacementString( |
There was a problem hiding this comment.
❓ usageCountInBookLabel substitutes currentBook verbatim, so the tooltip and screen-reader text read "Uses in GEN". The other place this extension fills a {book} placeholder in user-facing prose resolves it to a name first (Canon.bookIdToEnglishName, src/components/SegmentListView.tsx:220). Raw codes are right for the usage links themselves — those are written as references (GEN 1:1) — but is the code deliberate for the count label?
There was a problem hiding this comment.
Not deliberate — good catch. Switched to Canon.bookIdToEnglishName, so it reads "Uses in Genesis" while the usage links stay references. Same treatment and same caveat as SegmentListView.tsx:220 (a platform-localized name needs PAPI wiring this view doesn't have). Covered by a test that layers a resolved value over the key-as-value mock.
| {hiddenUsageCount > 0 && ( | ||
| <Button | ||
| data-testid="catalog-usages-show-all" | ||
| onClick={() => setShowsAllUsages(true)} |
There was a problem hiding this comment.
⛏️ showsAllUsages is only ever set to true, and it survives collapsing and re-expanding the row (both flags are row-local state that resets only on unmount), so a row expanded to hundreds of usages cannot be returned to the inline cap. Consider resetting it when the row collapses, or offering a "show fewer" affordance.
There was a problem hiding this comment.
Fixed. A handleToggle clears showsAllUsages alongside the expand flag, so collapsing returns the row to the inline cap. Went with the reset rather than a "show fewer" affordance — the expander is already gone by then, so there's nothing to pair it with.
|
|
||
| window.addEventListener('mousemove', handleMouseMove); | ||
| window.addEventListener('mouseup', handleMouseUp); | ||
| return () => { |
There was a problem hiding this comment.
⛏️ Commits happen only in endDrag, so if the panel unmounts mid-drag this cleanup drops the listeners and the width reached is silently discarded — the panel comes back at its previous width. The close control is not reachable mid-drag, but a draftVersion bump or isDraftLoading flipping true does unmount the panel (src/components/InterlinearizerLoader.tsx:639). A commit in the cleanup would cover it.
There was a problem hiding this comment.
Fixed, though not in that cleanup: it also runs when the drag ends normally and when the caller's callback changes identity, so committing there would fire part-way through a gesture. Added a separate mount-lifetime effect that commits dragWidthRef.current on unmount, reading the callback off a ref. Tests cover both unmounting with a drag in flight and without.
| const onKeyDown = useCallback( | ||
| (event: ReactKeyboardEvent) => { | ||
| // eslint-disable-next-line no-nested-ternary -- a two-key lookup reads worse as a map | ||
| const travel = event.key === 'ArrowLeft' ? -1 : event.key === 'ArrowRight' ? 1 : 0; |
There was a problem hiding this comment.
⛏️ The splitter handles Left/Right only. Home/End (jump to min/max) is part of the ARIA window-splitter pattern and is a couple of lines in this same handler.
There was a problem hiding this comment.
Added. Both jump to the bounds, so they land on the aria-valuemin / aria-valuemax the splitter already announces — which also means they need no RTL mirroring, unlike the arrows. Pulled the key→intent mapping into two small helpers, which retired the no-nested-ternary disable.
| // The arrow that moves the handle the way a drag would widen the panel widens it too, | ||
| // whichever side of the container the interface language anchors it to. | ||
| const step = travel === widenTravel() ? 1 : -1; | ||
| onWidthChange(clampWidth(width + step * KEYBOARD_RESIZE_STEP_PX)); |
There was a problem hiding this comment.
❓ Each arrow press writes straight through to useWebViewState (src/components/InterlinearizerLoader.tsx:394, :687), so holding the key with OS key repeat produces one host write per repeat — the same per-frame write the drag path defers to release. The hook's own doc acknowledges the difference, so this reads as intentional: is the host write cheap enough that the keyboard path does not want the same deferral (commit on keyup, or debounce)?
There was a problem hiding this comment.
Intentional, and not only on cost grounds. displayWidth falls back to width, which comes from useWebViewState — and that hook updates its local state only when onDidUpdateWebView comes back. So on the keyboard path the write is the redraw: commit-on-keyup would leave the handle frozen for the whole hold unless the keyboard path also grew its own in-flight width and a blur commit to go with it. The drag path has no such coupling — it already draws from dragWidth — which is why deferral is free there and isn't here. Left as is; the hook doc now also states the unmount commit.
| [width, onWidthChange, clampWidth], | ||
| ); | ||
|
|
||
| return { displayWidth: dragWidth ?? width, onMouseDown, onKeyDown }; |
There was a problem hiding this comment.
⛏️ displayWidth falls back to the caller's committed width unchanged, and that width is read straight out of WebView state — only the drag and arrow paths clamp. Every reachable value is inside WIDTH_BOUNDS today, but dragWidth ?? clampWidth(width) would make the bounds authoritative and keep aria-valuenow within aria-valuemin/aria-valuemax if the bounds are ever narrowed (or a persisted value ever arrives non-numeric).
There was a problem hiding this comment.
Done — dragWidth ?? clampWidth(width). Test renders at 5000 and asserts both the drawn width and aria-valuenow come back at the maximum.
| selectAnalysis, | ||
| selectAnalysisLanguage, | ||
| selectApprovedGloss, | ||
| selectCatalogRows, |
There was a problem hiding this comment.
⛏️ This import sits between selectApprovedGloss and selectApprovedMorphemes, splitting the two Approved entries.
There was a problem hiding this comment.
Moved after selectApprovedMorphemes.
| /** | ||
| * Returns one row per distinct token analysis in the draft, each carrying the usage data the | ||
| * analysis catalog lists it by, in the analysis's own order. Narrowing and ordering are the | ||
| * caller's ({@link applyCatalogQuery}), so a keystroke re-runs only that pass. |
There was a problem hiding this comment.
⛏️ {@link applyCatalogQuery} has no resolvable target in this file — only the CatalogRow type is imported from ../utils/analysis-query (line 37), while every other {@link} in this module names an imported symbol. Either import the symbol or reword to state the guarantee ("narrowing and ordering are the caller's") without naming the collaborator.
There was a problem hiding this comment.
Reworded rather than imported. comment-rules has a "link the contract, not the collaborator" rule that makes naming the callee wrong even where it would resolve, so the sentence now just states the guarantee: "Narrowing and ordering are the caller's, so a keystroke re-runs only that pass."
| // The store below waits for the draft: it seeds on mount alone, and the draft version that | ||
| // remounts it does not bump when the load completes. Nothing is lost by waiting — while the | ||
| // draft loads there is only ever a placeholder or an error panel to show. | ||
| <div className="tw:flex tw:flex-col tw:flex-1 tw:min-h-0">{loadingOrErrorPanel}</div> |
There was a problem hiding this comment.
⛏️ This branch lost the data-testid="book-fade-wrapper" element and its opacity transition. Harmless today — draft loading only happens at mount, when fadePhase is idle — but any future path that leaves isDraftLoading true during a cross-book fade would render the placeholder outside the curtain, and a test reaching for book-fade-wrapper while the draft loads would throw rather than fail informatively.
There was a problem hiding this comment.
Fixed by extracting a BookFadeWrapper component both branches render, so the draft-loading branch gets the test id and the fade back without a second copy of the zero-duration logic. The store still sits above the curtain, so the catalog stays undimmed. Copying the JSX instead would have left the fadePhase === 'out' arm unreachable on that branch and broken the coverage gate, which is what pushed it to a shared component.
The splitter gains Home/End and commits a drag the panel is unmounted holding; rows memoize and return to the inline usage cap when collapsed.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 10 comments.
Reviewable status: 0 of 20 files reviewed, 10 unresolved discussions (waiting on imnasnainaec).
| selectAnalysis, | ||
| selectAnalysisLanguage, | ||
| selectApprovedGloss, | ||
| selectCatalogRows, |
There was a problem hiding this comment.
Moved after selectApprovedMorphemes.
| /** | ||
| * Returns one row per distinct token analysis in the draft, each carrying the usage data the | ||
| * analysis catalog lists it by, in the analysis's own order. Narrowing and ordering are the | ||
| * caller's ({@link applyCatalogQuery}), so a keystroke re-runs only that pass. |
There was a problem hiding this comment.
Reworded rather than imported. comment-rules has a "link the contract, not the collaborator" rule that makes naming the callee wrong even where it would resolve, so the sentence now just states the guarantee: "Narrowing and ordering are the caller's, so a keystroke re-runs only that pass."
| * Each row owns its own layout so that its detail can be nested inside it. One element per analysis | ||
| * is what lets the list window and be walked by keyboard a row at a time. | ||
| */ | ||
| export default function CatalogRowView({ |
There was a problem hiding this comment.
Done — MemoizedCatalogRowView, matching the TokenChip / SegmentView pattern. Your premise checks out: applyCatalogQuery is filter + toSorted, so rows keep identity, and the rest of the props are stable (onUsageSelect is a useCallback, localizedStrings comes off the hoisted key array). Filed the windowing half against #231 rather than duplicating it here.
| const hiddenUsageCount = row.usages.length - visibleUsages.length; | ||
|
|
||
| const usageCountLabel = localizedStrings['%interlinearizer_analysisCatalog_usageCount%']; | ||
| const usageCountInBookLabel = formatReplacementString( |
There was a problem hiding this comment.
Not deliberate — good catch. Switched to Canon.bookIdToEnglishName, so it reads "Uses in Genesis" while the usage links stay references. Same treatment and same caveat as SegmentListView.tsx:220 (a platform-localized name needs PAPI wiring this view doesn't have). Covered by a test that layers a resolved value over the key-as-value mock.
| {hiddenUsageCount > 0 && ( | ||
| <Button | ||
| data-testid="catalog-usages-show-all" | ||
| onClick={() => setShowsAllUsages(true)} |
There was a problem hiding this comment.
Fixed. A handleToggle clears showsAllUsages alongside the expand flag, so collapsing returns the row to the inline cap. Went with the reset rather than a "show fewer" affordance — the expander is already gone by then, so there's nothing to pair it with.
| // The store below waits for the draft: it seeds on mount alone, and the draft version that | ||
| // remounts it does not bump when the load completes. Nothing is lost by waiting — while the | ||
| // draft loads there is only ever a placeholder or an error panel to show. | ||
| <div className="tw:flex tw:flex-col tw:flex-1 tw:min-h-0">{loadingOrErrorPanel}</div> |
There was a problem hiding this comment.
Fixed by extracting a BookFadeWrapper component both branches render, so the draft-loading branch gets the test id and the fade back without a second copy of the zero-duration logic. The store still sits above the curtain, so the catalog stays undimmed. Copying the JSX instead would have left the fadePhase === 'out' arm unreachable on that branch and broken the coverage gate, which is what pushed it to a shared component.
|
|
||
| window.addEventListener('mousemove', handleMouseMove); | ||
| window.addEventListener('mouseup', handleMouseUp); | ||
| return () => { |
There was a problem hiding this comment.
Fixed, though not in that cleanup: it also runs when the drag ends normally and when the caller's callback changes identity, so committing there would fire part-way through a gesture. Added a separate mount-lifetime effect that commits dragWidthRef.current on unmount, reading the callback off a ref. Tests cover both unmounting with a drag in flight and without.
| const onKeyDown = useCallback( | ||
| (event: ReactKeyboardEvent) => { | ||
| // eslint-disable-next-line no-nested-ternary -- a two-key lookup reads worse as a map | ||
| const travel = event.key === 'ArrowLeft' ? -1 : event.key === 'ArrowRight' ? 1 : 0; |
There was a problem hiding this comment.
Added. Both jump to the bounds, so they land on the aria-valuemin / aria-valuemax the splitter already announces — which also means they need no RTL mirroring, unlike the arrows. Pulled the key→intent mapping into two small helpers, which retired the no-nested-ternary disable.
| // The arrow that moves the handle the way a drag would widen the panel widens it too, | ||
| // whichever side of the container the interface language anchors it to. | ||
| const step = travel === widenTravel() ? 1 : -1; | ||
| onWidthChange(clampWidth(width + step * KEYBOARD_RESIZE_STEP_PX)); |
There was a problem hiding this comment.
Intentional, and not only on cost grounds. displayWidth falls back to width, which comes from useWebViewState — and that hook updates its local state only when onDidUpdateWebView comes back. So on the keyboard path the write is the redraw: commit-on-keyup would leave the handle frozen for the whole hold unless the keyboard path also grew its own in-flight width and a blur commit to go with it. The drag path has no such coupling — it already draws from dragWidth — which is why deferral is free there and isn't here. Left as is; the hook doc now also states the unmount commit.
| [width, onWidthChange, clampWidth], | ||
| ); | ||
|
|
||
| return { displayWidth: dragWidth ?? width, onMouseDown, onKeyDown }; |
There was a problem hiding this comment.
Done — dragWidth ?? clampWidth(width). Test renders at 5000 and asserts both the drawn width and aria-valuenow come back at the maximum.
A drag seeded from a committed width outside the bounds drew the panel past its announced maximum on the press alone. The jump and arrow keys now skip writing a width the panel already holds.
Only writes notified subscribers, so a component reading a key another component reset never re-rendered — a future test would have failed for a reason the production code has nothing to do with. A reset also now lands on the resetting caller's default rather than restoring the seed, matching what the real hook leaves behind.
The widest width gives way to a container too narrow to hold the panel and the text both, an arrow key steps from the width the panel is drawn at rather than the committed one behind it, and a drag that ends where it began commits nothing.
2 participants
Hoists the analysis store above the cross-book fade curtain so a jump to a
usage in another book cannot dim the panel. Search, sort, filter, and row
windowing are deferred to #231.
This change is
Summary by CodeRabbit
New Features
Bug Fixes