Add search, sort, filter, and windowing to the analysis catalog - #253
Add search, sort, filter, and windowing to the analysis catalog#253alex-rawlings-yyc wants to merge 14 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📝 WalkthroughWalkthroughAdds a localized Analysis Catalog panel with search, sorting, facet filters, tri-state morpheme filtering, zero-usage filtering, virtualized rows, and expanded test coverage. Jest mocks now support the required platform controls and icons. ChangesAnalysis Catalog
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The catalog enhancements are otherwise mergeable, but the resize mock can accumulate keydown listeners when its target ref changes, leading to repeated event handling in affected tests or integrations. This is a bounded, localized follow-up item. Sequence Diagram(s)sequenceDiagram
participant CatalogUser
participant AnalysisCatalogPanel
participant CatalogQueryControls
participant CatalogFilterPopover
participant useRowWindow
CatalogUser->>CatalogQueryControls: enter search or choose sort
CatalogUser->>CatalogFilterPopover: choose filters
CatalogQueryControls->>AnalysisCatalogPanel: update query state
CatalogFilterPopover->>AnalysisCatalogPanel: update filter state
AnalysisCatalogPanel->>useRowWindow: pass queried rows
useRowWindow-->>AnalysisCatalogPanel: return visible row window
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes implement the objectives in issue Full details: Docstring CoverageExplanation Docstring coverage is 97.92% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 48 functions across 21 files. (1 skipped: 1 unsupported.) ✨ 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 |
324afc3 to
cd98a29
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.
378003a to
f4b84ce
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
3b2cdb2 to
440cfc9
Compare
d85e9e3 to
46e7b1d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 13 comments.
Reviewable status: 2 of 12 files reviewed, 13 unresolved discussions (waiting on imnasnainaec).
src/__tests__/utils/language-tags.test.ts line 34 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Could define a
constforen_USin thisdescribeblock.
Left as-is. en_US appears in both describe blocks standing for two different things — a tag the collator falls back from, and an interface locale Intl rejects mid-list — so a shared const would have to sit above both and name neither case well. The literal being an obviously hand-typed tag is also the point being made, which a const would hide behind a name.
src/components/AnalysisCatalogPanel.tsx line 134 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Drafted by Claude:
This rationale is
deriveFacets's own, and it is stated there already:
@param rowsEvery row {@link buildCatalogRows} built, never what {@link applyCatalogQuery}
narrowed them to: a facet judged against the rows its own selection kept collapses as soon as
that selection is made, leaving nothing to widen it back by.Hover over the call shows that, so the copy here is a second home for a rule that can only ever have one. It is also in two more places downstream —
CatalogFilterPopoverProps.facetsandCatalogQueryControlsProps.facets— neither of which derives anything; they receive whatever they are handed.Suggest keeping the rule where the derivation happens and letting the three consumers say what the value is:
const facets = useMemo(() => deriveFacets(catalogRows), [catalogRows]);(the call is self-evident, so no doc at all reads fine here), and in both props types:
/** The choices worth offering as filters. */ facets: CatalogFacets;
CatalogFacetscarries the rest for anyone who follows the type.
Done — dropped the doc here and reduced both props types to /** The choices worth offering as filters. */.
src/components/CatalogFilterPopover.tsx line 151 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Drafted by Claude:
Three doc comments in this file explain that a field can stop offering choices while a reader is filtered to one, and that a selection with no control left is unclearable:
isFilterable, this one, andfeatureNames. One statement of it is enough, andisFilterableis where it belongs — the gate exists for exactly that case, and both of the others are visibly implementing the same rule against different shapes.Suggest leaving
isFilterable's doc as it stands and reducing these two to what they are:/** The choices to offer: the field's own, plus any still selected that it no longer lists. *//** * The feature names to raise a control for: those the rows offer choices for, and any a selection * still narrows by. */
Done, both reduced to your suggested wording. isFilterable keeps the rule.
src/components/CatalogFilterPopover.tsx line 179 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ This second paragraph is redundant with in-line comments. One or the other should probably be removed/reduced.
Dropped the paragraph. The inline comments sit where the marking actually happens and carry the loop bound, so they were the half worth keeping.
src/components/CatalogFilterPopover.tsx line 413 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Consider only showing the morphemes Select when setting "Show morphology" is on.
Done — the breakdown filter is now offered only where interlinearizer.showMorphology is on. You're right that a project not doing morphological analysis has no use for the question, and since the setting is project-scoped and durable rather than a view toggle, it's a fair read of whether this project works in morphemes at all.
One addition: the control also stays while a breakdown filter is already set, so turning the setting off behind an active has/lacks doesn't leave the list narrowed with nothing on screen to clear it by — the same case isFilterable covers for the facet filters. Both sides are tested.
The setting reaches the panel as a prop from the loader, which already resolves it for the view, rather than being read a second time in the panel.
src/components/CatalogFilterPopover.tsx line 451 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ The wording of this comment is reviewer defense. It can be dramatically cut down.
Cut to one line: Matches nothing until a write path can leave an analysis unused, which none does yet.
src/components/CatalogQueryControls.tsx line 16 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ The second sentence (Covers...) is unnecessary. That's what
satisfies Record<...means.
Dropped. CONFIDENCE_LABEL_KEYS in CatalogFilterPopover carried the same sentence about Confidence, so that one went too.
src/components/CatalogQueryControls.tsx line 93 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
This sort Select should probably have some sort of sort icon (e.g., up/down arrows) beside or at one end.
Added — ArrowUpDown inside the trigger, ahead of the value, with a stub added to the lucide-react mock.
src/hooks/usePanelResizeKeys.ts line 73 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Since
widenTravelis now only used once, it would be a touch less obfuscating to delete it and use its content directly here.
Done. Inlining collapsed it further than substitution would suggest: (readDirection() === 'rtl' ? 1 : -1) !== 1 is just readDirection() !== 'rtl', so the gate now reads as "leave it to the handle unless RTL". The one fact in widenTravel's doc the code doesn't show — that direction is settled per press, so a live language change is honored — moved to the hook's doc.
src/hooks/useRowWindow.ts line 44 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ The last (a gloss...) clause of this comment is justifying a commit but is somewhat out-of-context here. May not have enduring value.
Dropped the clause; the sentence now ends at "any edit to the underlying analysis as well."
src/hooks/useRowWindow.ts line 66 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ This comment has too much about a rejected alternative.
Dropped the rejected alternative, keeping only that the elements are held in state so the observer effect re-runs as each attaches.
src/hooks/useRowWindow.ts line 84 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Much of this comment is stating mechanism that's obvious from the code.
Trimmed to the part the code can't show — that IntersectionObserver reports transitions only, which is why count is a dependency. The re-delivery walkthrough and the line about a full window standing down both went.
src/utils/language-tags.ts line 42 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ The use of try/catch here is pretty self-explanatory, so the "Weighed one at a time" comment feels unnecessary.
Dropped the comment.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 12 files and all commit messages, and resolved 13 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).
The catalog panel listed every analysis in one fixed order with no way to narrow it, so a draft of any size was only navigable by scrolling. The query core already supported all of this; only the UI that varies it was missing. Search, sort, and filter state is ephemeral useState inside the panel. The panel is mounted only while open, so closing it clears the query — a filter that survived a reload would leave rows missing with nothing on screen saying why. Filters sit behind one control that reports how many are active, so a panel narrow enough to need filtering is not itself filled with them. All four groups ship: the facet-derived ones (books, part of speech, confidence, and each named feature), missing gloss, breakdown, and unused-only. Against today's data only books raises a control, since no write path records the others yet — the facets are rightly absent rather than offering a lone choice. Facets are derived from every row rather than from the rows a filter left standing, so a selection cannot collapse the facet that would widen it back. The new useRowWindow mounts a growing leading slice of the listing, extending as the end comes into reach and starting over when the query changes. It is deliberately not useSegmentWindow: a row list has no counterpart to the scripture reference that hook holds still, so it needs none of that geometry bookkeeping. A listing narrowed to nothing now says so, rather than reusing "No analyses recorded yet" and telling readers their draft is empty when they have merely mistyped. That message and the panel's original one both go through the platform EmptyState. Stubs the platform SearchBar, Select, MultiSelectComboBox, and EmptyState, each documenting where it diverges from the component it stands in for.
The sort option substituted the raw book code into "Most used in
{book}",
so the dropdown read "Most used in GEN" while the row column beside it
resolved the same book through Canon.bookIdToEnglishName and read "Uses
in
Genesis" — one book named two ways in one open panel.
Resolve the name once in the panel and pass it to both views, so the two
labels cannot disagree. CatalogQueryControls takes the resolved name
rather
than the code, which keeps it presentational and leaves book-name
resolution
in the panel.
The test rerendered through a bare InterlinearNavProvider rather than the PanelProviders root it mounted with. React saw a different element type at that position and remounted the provider, reinitializing the ref that holds the pending request — so the closing assertion found no request because none had survived the remount, not because navigating past EXO had abandoned one. Deleting the abandonment effect entirely left the test green. Rerender through PanelProviders instead, keeping the provider that owns the request mounted across both navigation steps. The test now fails with "EXO 3:14:8" when the abandonment effect is removed. Also lift the two collators out of the query memo. They were rebuilt on every keystroke in the search box, which changes the query but neither language tag.
Two identical declarations shadowed each other, and neither tsc nor ESLint covers __mocks__, so nothing flagged it.
The stub group seeded its layout from `defaultLayout` unconditionally, where the real group takes that prop only when it names exactly the panels mounted and discards it otherwise. A layout naming a closed panel therefore came back by itself here on the next mount, which upstream would have thrown away. That divergence hid the loader's restoring effect: the width it exists to reapply was already in the group's state before it ran, so the effect could be made a no-op with every catalog test still passing. Seed after the panels have registered instead, matching on the count as the real group does. Cover the effect with a test that mounts the group closed, leaving it knowing only of the view so the stored layout reaches it only by being applied as the catalog's panel joins. Disabling the effect now fails five tests.
The window reset exists for a changed query: a reader who narrows a listing is looking at a new list, not further down the old one. It was keyed on the rows array's identity instead, which is only a proxy for that — and a proxy that also turns over on any edit to the underlying analysis. A gloss approved in the view beside an open catalog therefore collapsed a deeply scrolled list back to its first chunk, throwing the reader to the end of forty rows while the sentinel re-extended beneath them. useRowWindow now takes the query itself and compares that by reference, so the hook does what its own doc comment already claimed. Also withhold the query controls from a draft that has recorded nothing, where a search box, sort and filter popover narrow an empty listing and the popover in particular is an invitation to a dead end. The gate reads the draft rather than the queried rows, so a query that matched nothing keeps the controls that are the only way to widen it back.
A feature value is free text, so it may be the empty string. The filter offered such a choice under its own spelling, which the platform control can neither carry as a value nor show as a label: the choice appeared as a blank, unclickable row. Give it a sentinel of its own, alongside the one the absent choice already uses, and a localized label to be read under. Route the three places that spelled a choice for the control through one function, so they cannot disagree about the two choices that need spelling. Latent for now, no write path recording feature values yet.
A choice can stop being offered while a reader is filtered to it, which left the control unmounted and the narrowed list with no way back. Also tell a value spelled like the untagged or empty label apart from it, two choices sharing a label leaving one unselectable.
The platform combo box resolves a picked option to the first entry whose label matches, so two choices sharing a label leave the later unselectable. Marking a value that read as a placeholder label was a single pass, so it collided one level up: a field holding both "(none)" and "(none) (recorded value)" gave both the same label, and the recorded value could not be filtered by. Build the labels in one pass instead, claiming each as it is taken. The absent-value choices claim the placeholder labels first, so ordering cannot let a value take one from under them, and a value is marked until its label is unclaimed rather than once.
* Offer every filter choice under the name the control reports back
The platform combo box resolves a picked option by matching the label the
command list reports back, and that label is trimmed. A choice offered
under a name carrying surrounding whitespace therefore matches no entry,
and the click is dropped with nothing on screen saying why. A part of
speech and a feature value are both free text arriving from whatever
system recorded them, so a padded one is not hypothetical.
Offer each choice under the trimmed spelling instead, which is also what
the collision marking has to compare, or two values differing only in
their padding would take the same name unmarked. A value that is nothing
but whitespace has no name left once trimmed and so borrows the empty
value's, the marking telling those two apart.
The stub trimmed nothing, which is why a padded value looked selectable
under it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Stop the collision marking spinning on a marking that moves nothing
Repeating the marking clears a chain of collisions only while each round
lengthens the name. The marking is a localized template, and one that
drops `{value}` spells back whatever name it was handed, so the loop had
no way out: two colliding choices under such a translation hung the
render, and with it the whole WebView.
Bound the rounds by the number of names already claimed, that many having
produced more distinct spellings than there are names to collide with. A
marking that does move the name still clears every collision within the
bound, so nothing changes for a translation that carries `{value}`; one
that does not leaves two choices sharing a name, and only the later of
them unselectable.
The stub keys its options by value rather than by label, so a caller
offering two choices under one name is left to the assertions rather than
buried under React's complaint about the duplicate key.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Name the language the missing-gloss filter asks about
The filter read "Missing gloss in en", the analysis language reaching its
label as the BCP 47 tag the project records it under. A reader who never
chose that tag themselves has no reason to recognize it, and the question
the filter asks is about a language rather than about a code.
Resolve the tag to the language's name in the interface's own language,
keeping the tag for one no host has a name for and for one that cannot be
parsed at all — tags reach the panel as free text, and naming an
unparsable one throws.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Return the catalog list to its top when its query changes
The window already started over on a new query, but the scroll container
is the same element throughout and kept the offset it was left at, merely
clamped to the shorter content. A reader who narrowed a deeply scrolled
list therefore landed part way down a listing they had not seen the start
of, the sentinel then extending the window from under them.
Put the scroll back with the count, before paint so the list is never
shown at the old offset first. Keyed on the query the count was reached
against rather than on the rows, so an edit to the analysis beside an open
catalog still leaves a scrolled list where it is.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Stand the row window's observer down once it covers every row
The observer re-subscribes on every count change, so that a sentinel left
inside the arming margin after an extend is reported again rather than
waited on. Once the window holds every row there is nothing left to
extend by, and each further delivery could only put the count back
through unchanged — leaving termination resting on React bailing out on
that, which an edit letting the count grow past the row count would
quietly turn into a loop.
Skip subscribing at all in that state.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Drop the resize factor the direction gate already fixes at one
The gate returns for every key outside a right-to-left interface, so past
it the handle always widens toward the screen's right and the direction
factor is always one. Multiplying by it said nothing the gate had not
already settled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Name the analysis language in the interface's own language
The name was resolved against the host's locale, which the platform's
interface language does not follow: nothing aligns the runtime locale to
the `platform.interfaceLanguage` setting. A reader on an English host with
the interface set to Spanish read "Falta glosa en French" — the template
in one language and the name it carries in another.
Resolve it against the interface languages the panel's own localized
strings were resolved for. An interface locale that cannot be parsed
falls back to the host's, which costs the name its language rather than
costing it the name; that stays separate from an unparsable analysis tag,
which still reads as itself.
The popover takes the name rather than the tag now, so the one place that
reads the setting is the one that already talks to the platform.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Drop interface locales Intl rejects one at a time
`Intl` rejects a whole locale list for any one entry it cannot parse, so
an unusable locale ahead of a usable one cost the naming both and sent it
to the host's locale. The platform resolves a localized string the other
way, walking past the locales it has nothing for, so the two disagreed: a
label resolved in the reader's second interface language beside a name
read in the host's.
Filter the list per entry instead, falling back to the host's locale only
where nothing usable is left.
Reachable rather than hypothetical: interface locales are named by the
localization files carrying them, which nothing holds to BCP 47
structure, and the settings service validates a written value but not a
read one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Trim the marking that tells one filter choice from another
A marking is free to pad what it wraps, and the control resolves a
choice
by the label it reports back trimmed — so a padded name left a marked
choice unselectable, the same way an untrimmed recorded value did.
* Keep the query controls' comments to purpose over mechanism
The scroll reset, collision bound, and locale filter each explained how
they work where the rules ask what they are for, and repeated across a
doc comment and the code below it.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Alex Rawlings <alex.rawlings@wycliffe.ca>
The per-book usage count each row is ranked and labeled by is taken against the book on screen, so moving to another book is a new listing rather than more of the old one.
afbd52b to
49c497b
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 4 files and all commit messages.
Reviewable status:
2 participants
Closes #231.
The catalog listed every analysis in one fixed order with no way to narrow it, so a draft of any size was navigable only by scrolling. The query core (#192, PR #211) already backed all of this —
applyCatalogQuery,deriveFacets,CatalogSort, andCatalogFilterswere built and tested. This is the UI that varies them: the panel's fixed query literal becomes state the controls drive, and nothing in the query core changed.Search, sort, and filter state is ephemeral
useStateinside the panel. The panel is mounted only while it is open, so closing it clears the query by construction rather than by a reset effect — a filter that survived a reload would leave rows missing with nothing on screen saying why. Open/closed and width stay tab-scoped inuseWebViewState.Filters sit behind one control
A panel narrow enough to need filtering should not itself be filled with filter controls, so all of them live in a popover whose trigger reports how many are active.
All four groups ship. Against today's data only books ever raises a control: nothing writes part of speech, confidence, or features yet, so
deriveFacetscorrectly yields nothing for them and no control appears. Used nowhere ships too, and matches nothing until PT9 import (#150) or the catalog's own delete/merge paths land —detachTokenAnalysisLinkdrops a payload with its last link, so no current write path can produce a zero-usage row. Both are the outcome #231 decided on and both are tested for, not gaps.Facets are derived from every row the draft holds rather than from the rows a filter left standing. A facet judged against its own selection's survivors would collapse to that selection, leaving nothing on screen to widen it back by.
Carrying no value is a choice of its own
CatalogFacetslists the absent value asundefined, which is what lets a reader ask which analyses are still missing a field as readily as which carry a given value. The platformMultiSelectComboBoxspeaks strings alone, so that choice needs a spelling:\u0000untagged, a leading NUL being one no part of speech, confidence level, or feature value can collide with. Values are read back through a map rather than compared against the sentinel, soundefinedis recovered as the choice it is. Worth a look — it is the one place the control's vocabulary and the filter's diverge.Books is the exception: a usage names a book by construction, so that facet never offers an untagged choice, and the selection is filtered before it reaches
CatalogFilters.Windowing
useRowWindowmounts a growing leading slice of the listing and extends by a chunk each time an end-of-list sentinel comes within reach. Grow-only and anchored to nothing — it never culls from the top and never adjusts the scroll position. Deliberately notuseSegmentWindow: that hook is anchored around a scripture reference, and a row list has no counterpart to hold still, so it needs none of that geometry bookkeeping.Two details worth the reviewer's attention:
rowsis a different array, adjusted during the render that first sees the new listing rather than in an effect afterwards — an effect would let one frame paint the new rows at the old, grown count before shrinking back. Keyed on array identity rather than length, because a query can narrow a listing to a different set of rows of the same size.IntersectionObserverreports only intersection transitions: after an extend the sentinel node is unchanged and may still sit inside the arming margin, where a stale observer would stay silent however far the reader scrolls. A fresh observer re-delivers the current state, extending once per delivery until the sentinel is pushed clear.Not in the issue: a listing narrowed to nothing says so
Reusing "No analyses recorded yet" for a query that matched nothing would tell readers their draft is empty when they have merely mistyped, and send them looking for lost work. There is now a second message for that case, and both go through the platform
EmptyStaterather than a hand-rolled paragraph.Search semantics are unchanged and accepted as-is
applyCatalogQuerymatches the whole trimmed, folded query as a single substring against a per-row blob. So a multi-word query never matches across fields, there is no match weighting, and ordering is the chosen sort key alone. The placeholder promises "Search forms and glosses" and nothing more. Multi-term search, relevance ranking, and highlighting are query-core work, not UI work, and separate issues if wanted.Mocks
Stubs the platform
SearchBar,Select,MultiSelectComboBox, andEmptyState, each documenting where it diverges from the component it stands in for — notably thatMultiSelectComboBoxresolves an entry by label as the real component's own select handler does, so a stub test cannot pass on a collision the real component would drop.Testing
Covers all eight behaviors #231 lists. Full suite passing, 100% coverage, lint clean.
This change is
Summary by CodeRabbit
New Features
Tests