feat: Introduce component updates to support categorical metadata - #1795
feat: Introduce component updates to support categorical metadata#1795ikondrat wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe distribution panel now supports categorical metadata buckets, selection, sorting, configuration, loading/error states, expanded views, accessibility summaries, and retry handling. Shared chart, header, dialog, selection, histogram, and filter-chip components were generalized and covered with tests. ChangesCategorical distribution support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MetadataCategoricalFilter
participant DatasetDistributionPanel
participant BarChart
User->>MetadataCategoricalFilter: toggle categorical value
MetadataCategoricalFilter->>DatasetDistributionPanel: value toggle callback
DatasetDistributionPanel->>BarChart: derived bucket data and chart options
BarChart->>DatasetDistributionPanel: selectable bar click
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
❌ Fast Track: checks did not pass
To run the guardrails locally, from Reflects |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte (1)
361-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared
Buttonfor the inline retry.The full-error state at Line 448 uses the
Buttoncomponent while this refetch-error retry is a raw<button>withunderline; styling/focus treatment diverges. AButtonwithvariant="link"/size="sm"keeps both retries consistent.🤖 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 `@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte` around lines 361 - 377, Replace the raw retry button in the activeCategorical error block with the shared Button component, using variant="link" and size="sm" while preserving its type, click handler, label, and existing layout classes. Match the full-error retry implementation for consistent styling and focus treatment.lightly_studio_view/src/lib/components/DatasetDistributionPanel/MetadataCategoricalFilter.svelte (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded top-N assumptions.
Not in top 20and the> 5search threshold both bake in numbers owned elsewhere (the distribution hook's limit). If the bucket limit changes, this text silently lies. Consider deriving the wording from a shared constant or a prop.Also applies to: 139-141
🤖 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 `@lightly_studio_view/src/lib/components/DatasetDistributionPanel/MetadataCategoricalFilter.svelte` at line 78, Replace the hardcoded bucket-limit assumptions in MetadataCategoricalFilter, including the showSearch threshold and “Not in top 20” wording, with the distribution hook’s shared limit or an explicit prop. Use that single source for both the displayed top-N text and search behavior so they remain consistent when the limit changes.lightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.svelte (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the exported
DistributionSortOptionkey type.
ExpandDialogtypes the same prop asRecord<DistributionSortOption, string>; usingkeyof typeof DISTRIBUTION_SORT_LABELShere is equivalent but inconsistent across the layer.♻️ Proposed tweak
- sortLabels?: Record<keyof typeof DISTRIBUTION_SORT_LABELS, string>; + sortLabels?: Record<DistributionSortOption, string>;Update the import accordingly:
- import { DISTRIBUTION_SORT_LABELS, type DistributionConfig } from '../types'; + import { + DISTRIBUTION_SORT_LABELS, + type DistributionConfig, + type DistributionSortOption + } from '../types';🤖 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 `@lightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.svelte` at line 26, Update the sortLabels prop type in PanelHeader to use the exported DistributionSortOption key type, matching ExpandDialog. Adjust the related import and leave the existing prop behavior unchanged.lightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.test.ts (1)
141-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNegative class assertions are weak.
Asserting the absence of
mb-2/mb-1passes for any unrelated class rename, so it won't catch spacing regressions. If the intent is consistent chart spacing, asserting the actual expected wrapper classes (or the chartgrid.top, as done inExpandDialog.test.ts) is more durable.🤖 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 `@lightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.test.ts` around lines 141 - 147, Strengthen the “does not add plot-specific bottom margins” test in the PanelHeader test suite by asserting the expected spacing classes on the summary element and its parent, rather than only asserting mb-2 and mb-1 are absent. Align the assertions with the intended chart spacing contract and the durable grid.top approach used by ExpandDialog.test.lightly_studio_view/src/lib/components/DatasetDistributionPanel/types.ts (1)
42-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument mutual exclusivity like the sibling fields.
data/histogramboth state they are mutually exclusive;categoricaldoesn't, yetDatasetDistributionPanelpreferscategoricaloverdatawhen both are set. A one-line comment keeps the contract discoverable.♻️ Proposed doc tweak
- /** Controlled categorical distribution and selection state. */ + /** + * Controlled categorical distribution and selection state. Takes precedence + * over `data` and is mutually exclusive with `histogram`. + */ categorical?: {🤖 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 `@lightly_studio_view/src/lib/components/DatasetDistributionPanel/types.ts` around lines 42 - 56, Add a concise documentation comment to the categorical field in the relevant type definition stating that categorical and data/histogram distribution states are mutually exclusive, matching the sibling field contracts. Preserve the existing DatasetDistributionPanel precedence and type shape.lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.ts (1)
273-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrder assertion is really a sort-by-count assertion.
The bucket counts (4, 3, 2) already match
sortBy: 'count', so this passes even if bucket order were not preserved. Using counts that differ from bucket order would actually pin the behavior the test name claims. The hardcodedrgba(59,217,159,0.85)is also worth importing from the chart's colour constant if one is exported.🤖 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 `@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.ts` around lines 273 - 276, The test data in DatasetDistributionPanel should use bucket counts whose descending count order differs from the original bucket order, so the assertion verifies that sortBy: 'count' reorders buckets rather than merely matching the existing order. Update the expected order accordingly, and replace the hardcoded selected-item color with the chart color constant if an exported symbol is available.
🤖 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
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte`:
- Around line 489-498: Unify the manual-selector data contract between
DatasetDistributionPanel’s DistributionConfigDialog usage and ExpandDialog:
determine whether the dialog should consume stable item ids or labels, then
remove the redundant allClasses/items prop and update the consuming logic and
call sites to use the single source consistently. Preserve the displayed labels
while retaining stable ids for selection.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DistributionConfigDialog/DistributionConfigDialog.svelte`:
- Around line 22-38: Expose an optional singular itemNoun prop in
DistributionConfigDialog alongside itemNounPlural, defaulting to the existing
singular label, and forward it to the manual selector and relevant categorical
wording. Update ClassSetConfigDialog integration so its itemNoun value is passed
through while preserving existing plural-label behavior.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/ExpandDialog/ExpandDialog.svelte`:
- Around line 100-104: Align the prop contract between DistributionConfigDialog
call sites: inspect how DatasetDistributionPanel uses allClasses and items, then
either remove the redundant allClasses prop everywhere or pass it from
ExpandDialog alongside items if it is required. Update the component’s
declaration and all call sites consistently.
---
Nitpick comments:
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte`:
- Around line 361-377: Replace the raw retry button in the activeCategorical
error block with the shared Button component, using variant="link" and size="sm"
while preserving its type, click handler, label, and existing layout classes.
Match the full-error retry implementation for consistent styling and focus
treatment.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.ts`:
- Around line 273-276: The test data in DatasetDistributionPanel should use
bucket counts whose descending count order differs from the original bucket
order, so the assertion verifies that sortBy: 'count' reorders buckets rather
than merely matching the existing order. Update the expected order accordingly,
and replace the hardcoded selected-item color with the chart color constant if
an exported symbol is available.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/MetadataCategoricalFilter.svelte`:
- Line 78: Replace the hardcoded bucket-limit assumptions in
MetadataCategoricalFilter, including the showSearch threshold and “Not in top
20” wording, with the distribution hook’s shared limit or an explicit prop. Use
that single source for both the displayed top-N text and search behavior so they
remain consistent when the limit changes.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.svelte`:
- Line 26: Update the sortLabels prop type in PanelHeader to use the exported
DistributionSortOption key type, matching ExpandDialog. Adjust the related
import and leave the existing prop behavior unchanged.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.test.ts`:
- Around line 141-147: Strengthen the “does not add plot-specific bottom
margins” test in the PanelHeader test suite by asserting the expected spacing
classes on the summary element and its parent, rather than only asserting mb-2
and mb-1 are absent. Align the assertions with the intended chart spacing
contract and the durable grid.top approach used by ExpandDialog.test.
In `@lightly_studio_view/src/lib/components/DatasetDistributionPanel/types.ts`:
- Around line 42-56: Add a concise documentation comment to the categorical
field in the relevant type definition stating that categorical and
data/histogram distribution states are mutually exclusive, matching the sibling
field contracts. Preserve the existing DatasetDistributionPanel precedence and
type shape.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d91f7e1f-3c37-4091-98f0-bdf621fea6c7
📒 Files selected for processing (18)
lightly_studio_view/src/lib/components/BarChart/BarChart.sveltelightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.sveltelightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/DistributionConfigDialog/DistributionConfigDialog.sveltelightly_studio_view/src/lib/components/DatasetDistributionPanel/DistributionConfigDialog/DistributionConfigDialog.test.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/ExpandDialog/ExpandDialog.sveltelightly_studio_view/src/lib/components/DatasetDistributionPanel/ExpandDialog/ExpandDialog.test.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/MetadataCategoricalFilter.sveltelightly_studio_view/src/lib/components/DatasetDistributionPanel/MetadataCategoricalFilter.test.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.sveltelightly_studio_view/src/lib/components/DatasetDistributionPanel/PanelHeader/PanelHeader.test.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/selectVisibleCounts.test.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/selectVisibleCounts.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/types.tslightly_studio_view/src/lib/components/Histogram/buildHistogramOption.test.tslightly_studio_view/src/lib/components/Histogram/buildHistogramOption.tslightly_studio_view/src/lib/components/MetadataFilterChips/MetadataFilterChips.sveltelightly_studio_view/src/lib/components/MetadataFilterChips/MetadataFilterChips.test.ts
| <DistributionConfigDialog | ||
| bind:open={configDialogOpen} | ||
| allClasses={displayedData.map((item) => item.label)} | ||
| items={configurationItems} | ||
| config={activeViewConfig} | ||
| showCountMode={!activeCategorical} | ||
| itemNounPlural={activeCategorical ? 'values' : 'classes'} | ||
| sortLabels={activeCategorical ? CATEGORICAL_DISTRIBUTION_SORT_LABELS : undefined} | ||
| onApply={applyConfig} | ||
| /> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Both allClasses and items are passed to DistributionConfigDialog.
allClasses carries labels while items carries stable ids — two sources of truth for the manual selector, and ExpandDialog passes only items. Whichever the dialog actually consumes, one call site is wrong or one prop is dead. See the consolidated note.
🤖 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
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte`
around lines 489 - 498, Unify the manual-selector data contract between
DatasetDistributionPanel’s DistributionConfigDialog usage and ExpandDialog:
determine whether the dialog should consume stable item ids or labels, then
remove the redundant allClasses/items prop and update the consuming logic and
call sites to use the single source consistently. Preserve the displayed labels
while retaining stable ids for selection.
| /** Singular/plural labels used by categorical distributions. */ | ||
| itemNounPlural?: string; | ||
| /** Labels for the available sort modes. */ | ||
| sortLabels?: Record<DistributionSortOption, string>; | ||
| /** Invoked with the new config when the user clicks Apply. */ | ||
| onApply: (config: DistributionConfig) => void; | ||
| } | ||
|
|
||
| let { open = $bindable(), allClasses, config, showCountMode = true, onApply }: Props = $props(); | ||
| let { | ||
| open = $bindable(), | ||
| allClasses, | ||
| items, | ||
| config, | ||
| showCountMode = true, | ||
| itemNounPlural = 'classes', | ||
| sortLabels = DISTRIBUTION_SORT_LABELS, | ||
| onApply |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose and forward the singular noun too.
ClassSetConfigDialog defaults itemNoun to "class" and passes it to the manual selector. Categorical callers can currently customize only the plural label, resulting in mixed “values”/“class” wording.
Proposed fix
interface Props {
+ itemNoun?: string;
itemNounPlural?: string;
}
let {
+ itemNoun = 'class',
itemNounPlural = 'classes',
}: Props = $props();
<ClassSetConfigDialog
+ {itemNoun}
{itemNounPlural}
/>Also applies to: 64-70
🤖 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
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DistributionConfigDialog/DistributionConfigDialog.svelte`
around lines 22 - 38, Expose an optional singular itemNoun prop in
DistributionConfigDialog alongside itemNounPlural, defaulting to the existing
singular label, and forward it to the manual selector and relevant categorical
wording. Update ClassSetConfigDialog integration so its itemNoun value is passed
through while preserving existing plural-label behavior.
| items={configurationItems} | ||
| {config} | ||
| {showCountMode} | ||
| itemNounPlural={categoryNounPlural} | ||
| {sortLabels} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
items vs allClasses prop paths look inconsistent across call sites.
Here DistributionConfigDialog receives only items, while DatasetDistributionPanel passes both allClasses and items. One of the two is redundant (or allClasses is still required and missing here). See the consolidated note for the full picture.
🤖 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
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/ExpandDialog/ExpandDialog.svelte`
around lines 100 - 104, Align the prop contract between DistributionConfigDialog
call sites: inspect how DatasetDistributionPanel uses allClasses and items, then
either remove the redundant allClasses prop everywhere or pass it from
ExpandDialog alongside items if it is required. Update the component’s
declaration and all call sites consistently.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c71452558
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,163 @@ | |||
| <script lang="ts"> | |||
There was a problem hiding this comment.
The repository already contains MetadataCategoricalFilter/MetadataCategoricalFilter.svelte with colocated helpers, tests, stories, and a barrel export. Adding a second same-named component here duplicates that implementation, violates the component-per-folder convention, and allows fixes to drift between two versions; import and extend the existing module instead.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| const categoricalData = $derived<CategoryCount[]>( | ||
| (activeCategorical?.buckets ?? []).map((bucket) => { | ||
| // When filteredBuckets is defined (query has returned) look up the | ||
| // filtered count for this bucket. Absent = 0 (filter removed it entirely). | ||
| const filteredBucket = activeCategorical?.filteredBuckets?.find( |
There was a problem hiding this comment.
This mapping begins another substantial block of data transformation, configuration state, handlers, and presentation inside an already oversized panel. The frontend guidelines call for components under 100 lines and separate business logic from presentation; move the categorical view-model/configuration logic into a component-specific helper or hook and render its controls through a focused child component.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| import type { ClassSetSelection } from '$lib/components/ClassSetConfig'; | ||
| import { type AnnotationCountMode } from '$lib/api/lightly_studio_local/types.gen'; | ||
| import type { HistogramData, HistogramRange } from '$lib/components/Histogram'; | ||
| import type { CategoricalMetadataBucket } from '$lib/hooks/useCategoricalMetadataDistribution/useCategoricalMetadataDistribution'; |
There was a problem hiding this comment.
This deep import reaches into the hook implementation instead of its public module API, and the same pattern is repeated in the new filter component. Re-export CategoricalMetadataBucket from the hook barrel and import it from the module-level entry point so consumers are not coupled to the hook's implementation filename.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| const option = echartsMock.instance.setOption.mock.lastCall?.[0] as { | ||
| yAxis: { data: string[] }; | ||
| series: [{ data: { itemStyle: { color: string } }[] }]; | ||
| grid: { top: number }; | ||
| }; |
There was a problem hiding this comment.
These assertions inspect ECharts option structure and exact rendering colors, so harmless chart-implementation changes can break the component test without changing user behavior. Keep option-shape and color checks in buildEchartsOption.test.ts; here assert the accessible categorical summary, controls, and emitted toggle behavior as required by the frontend testing guidelines.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| const sources: DistributionSource[] = [ | ||
| { | ||
| id: 'metadata', | ||
| label: 'Metadata', | ||
| groups: [ |
There was a problem hiding this comment.
The new categorical tests repeatedly rebuild the same nested metadata source, group, selection, and bucket structure, making this test file lengthy and brittle. Add a shared defaultCategoricalSource or builder and override only the buckets or state relevant to each case, following the frontend guideline to use default-prop helpers.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
1 participant
What has changed and why?
(Delete this: Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.)
How has it been tested?
(Delete this: Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.)
Did you update CHANGELOG.md?
Summary by CodeRabbit
New Features
Bug Fixes