feat: lig-9585 Introduce changes in hooks required to render categorical metadata in distribution panel - #1796
Open
ikondrat wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCategorical metadata selections are supported in filter chips, propagated through image, frame, infinite-image, and video filter construction, and scoped through derived collection IDs. Dimension store handling and response validation are also updated. ChangesMetadata filtering and collection scoping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MetadataFilterChips
participant useMetadataFilterChips
participant useMetadataFilters
participant FilterBuilder
participant SampleFilter
MetadataFilterChips->>useMetadataFilterChips: toggle categorical chip
useMetadataFilterChips->>useMetadataFilters: update categorical metadata values
useMetadataFilters->>FilterBuilder: create metadata filters
FilterBuilder->>SampleFilter: add metadata_filters
SampleFilter-->>MetadataFilterChips: updated filter state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
ikondrat
force-pushed
the
feature/lig-9585-categorical-metadata-distribution.hooks
branch
from
July 29, 2026 10:37
e3017e5 to
82dc719
Compare
![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: e3017e5b4d
ℹ️ 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".
❌ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lightly_studio_view/src/lib/components/MetadataFilterChips/useMetadataFilterChips.svelte.ts (1)
114-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCategorical toggles skip PostHog tracking and hinge on
lastCategoricalValuesmembership.Two things in the categorical branch:
trackFilterChangedis never called, sometadata_filter_changedevents are only emitted for numeric chips — the analytics for categorical filters will silently be missing.- Using
lastCategoricalValues[key]as the "is categorical" discriminator is fragile: a categorical key whose selection was never recorded (empty selection, or effect not yet flushed) falls through to the numeric path and no-ops. Checking the store (key in categoricalStore.current) or passingchip.kindfrom the caller is more direct.♻️ Sketch
- if (lastCategoricalValues[key]) { + if (key in categoricalStore.current || lastCategoricalValues[key]) { updateCategoricalMetadataValues({ ...categoricalStore.current, - [key]: checked ? lastCategoricalValues[key] : [] + [key]: checked ? (lastCategoricalValues[key] ?? []) : [] }); + trackFilterChanged(key, checked ? 'enabled' : 'disabled'); return; }🤖 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/MetadataFilterChips/useMetadataFilterChips.svelte.ts` around lines 114 - 130, Update handleToggle so categorical detection uses key membership in categoricalStore.current rather than lastCategoricalValues[key], preserving categorical handling even when no prior selection exists. Emit trackFilterChanged for categorical toggles as well, using the same enabled/disabled status, while keeping the numeric range behavior unchanged.lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte (2)
366-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline retry diverges from the full-state retry control.
The stale-data retry is a bare
<button class="underline">with nodata-testid, while the empty-state retry at Line 448 uses theButtoncomponent withdata-testid="metadata-categorical-retry". Reusing the same component/test id keeps styling consistent and makes this path assertable (the test atDatasetDistributionPanel.test.tsLines 462-496 currently only checks the alert text, never the click).🤖 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 366 - 376, The stale-data retry in DatasetDistributionPanel should use the same Button component and data-testid="metadata-categorical-retry" as the empty-state retry, replacing the bare button while preserving onCategoricalRetry and the existing Retry label. Update the related test to query this test id and verify clicking it invokes the retry handler.
184-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKey categorical configs by source + group id.
categoricalConfigsis keyed onactiveGroup.idalone. Group ids are only unique within a source, so two sources exposing the same group id (e.g.city) would share orientation/top-N state. Composing the key avoids the collision.♻️ Proposed change
- let categoricalConfigs = $state<Record<string, DistributionConfig>>({}); + let categoricalConfigs = $state<Record<string, DistributionConfig>>({}); + const categoricalConfigKey = $derived( + activeGroup ? `${activeSource.id}:${activeGroup.id}` : undefined + ); const categoricalConfig = $derived<DistributionConfig>( - activeGroup - ? (categoricalConfigs[activeGroup.id] ?? { + categoricalConfigKey + ? (categoricalConfigs[categoricalConfigKey] ?? { ...defaultCategoricalConfig, n: Math.max(categoricalData.length, 1) }) : defaultCategoricalConfig );
setCategoricalConfigneeds the same key.🤖 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 184 - 192, Update categoricalConfigs and its related accessors to key entries by both source and group identity rather than activeGroup.id alone, preventing collisions between sources with the same group id. Apply the identical composite-key construction in setCategoricalConfig so reads and writes use the same key.lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.ts (1)
418-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the
Missinglookup to the dialog.
getAllByText('Missing')searches the whole document, so it also picks up anything the categorical filter or sr-only summary renders with that exact text — thetoHaveLength(2)assertion and the[1]index then depend on unrelated markup. Query within the dialog instead.♻️ Proposed change
await fireEvent.click(screen.getByRole('tab', { name: 'Manual' })); - const missingOptions = screen.getAllByText('Missing'); + const missingOptions = within(screen.getByRole('dialog')).getAllByText('Missing'); expect(missingOptions).toHaveLength(2);Add
withinto the@testing-library/svelteimport.🤖 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 418 - 423, Scope the `Missing` text lookup in the test around the dialog opened by `dataset-distribution-configure` to avoid unrelated document content. Add the `within` testing-library helper to the existing import, obtain the dialog element, and use it for the lookup while preserving the expected count and selected option.lightly_studio_view/src/lib/components/DatasetDistributionPanel/MetadataCategoricalFilter.svelte (1)
54-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a short comment explaining the Missing/Other disambiguation rules (P2 style).
The four-branch disambiguation logic (literal
'Missing'/'Other'values vs. semantic missing/aggregate buckets) is correct but non-obvious from the code alone. A one-line comment per branch would help future readers avoid re-deriving the intent.As per coding guidelines, "focus on code style, use succinct comments, assign style comments priority P2, and ensure the pull request follows the
ai_guidelinesdocumentation."🤖 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` around lines 54 - 74, Add succinct comments to the branches in optionLabel explaining the disambiguation between literal “Missing”/“Other” values and semantic missing/aggregate buckets; preserve the existing conditions and labels without changing behavior.Source: Coding guidelines
🤖 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/MetadataCategoricalFilter.svelte`:
- Around line 109-112: Update the trigger button’s accessible name in
MetadataCategoricalFilter so it includes the dynamic loading or selection
summary instead of overriding it with the static “Select metadata values” label.
Preserve the visible span’s existing loading and summary behavior while ensuring
screen readers receive the current state.
---
Nitpick comments:
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte`:
- Around line 366-376: The stale-data retry in DatasetDistributionPanel should
use the same Button component and data-testid="metadata-categorical-retry" as
the empty-state retry, replacing the bare button while preserving
onCategoricalRetry and the existing Retry label. Update the related test to
query this test id and verify clicking it invokes the retry handler.
- Around line 184-192: Update categoricalConfigs and its related accessors to
key entries by both source and group identity rather than activeGroup.id alone,
preventing collisions between sources with the same group id. Apply the
identical composite-key construction in setCategoricalConfig so reads and writes
use the same key.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.ts`:
- Around line 418-423: Scope the `Missing` text lookup in the test around the
dialog opened by `dataset-distribution-configure` to avoid unrelated document
content. Add the `within` testing-library helper to the existing import, obtain
the dialog element, and use it for the lookup while preserving the expected
count and selected option.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/MetadataCategoricalFilter.svelte`:
- Around line 54-74: Add succinct comments to the branches in optionLabel
explaining the disambiguation between literal “Missing”/“Other” values and
semantic missing/aggregate buckets; preserve the existing conditions and labels
without changing behavior.
In
`@lightly_studio_view/src/lib/components/MetadataFilterChips/useMetadataFilterChips.svelte.ts`:
- Around line 114-130: Update handleToggle so categorical detection uses key
membership in categoricalStore.current rather than lastCategoricalValues[key],
preserving categorical handling even when no prior selection exists. Emit
trackFilterChanged for categorical toggles as well, using the same
enabled/disabled status, while keeping the numeric range behavior unchanged.
🪄 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: cc4b08d1-11cd-4397-bafe-72db88d0a23c
📒 Files selected for processing (28)
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.tslightly_studio_view/src/lib/components/MetadataFilterChips/useMetadataFilterChips.svelte.tslightly_studio_view/src/lib/components/MetadataFilterChips/useMetadataFilterChips.test.tslightly_studio_view/src/lib/hooks/useFramesFilter/frameFilter.test.tslightly_studio_view/src/lib/hooks/useFramesFilter/frameFilter.tslightly_studio_view/src/lib/hooks/useImageFilters/useImageFilters.test.tslightly_studio_view/src/lib/hooks/useImagesInfinite/buildRequestBody.test.tslightly_studio_view/src/lib/hooks/useImagesInfinite/createImagesInfiniteOptions.test.tslightly_studio_view/src/lib/hooks/useMetadataFilters/useMetadataFilters.test.tslightly_studio_view/src/lib/hooks/useVideoFilters/useVideoFilters.test.tslightly_studio_view/src/lib/hooks/useVideoFilters/useVideoFilters.ts
ikondrat
changed the title
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/useDimensions/useDimensions.ts`:
- Around line 56-72: Update the collectionId subscription in the readable
returned by the dimensions hook to handle falsy values by invoking the existing
reset helper that clears dimensionsBounds and dimensionsValues and resets
lastCollectionId. Apply the same reset handling in the no-collection-ID path
before returning source, ensuring undefined invalidates prior collection state
and prevents stale responses from repopulating it.
🪄 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: 61b25f0a-4fbf-4cc7-a1af-2c2e364ddbd2
📒 Files selected for processing (11)
lightly_studio_view/src/lib/components/MetadataFilterChips/MetadataFilterChips.sveltelightly_studio_view/src/lib/components/MetadataFilterChips/MetadataFilterChips.test.tslightly_studio_view/src/lib/hooks/useDimensions/useDimensions.test.tslightly_studio_view/src/lib/hooks/useDimensions/useDimensions.tslightly_studio_view/src/lib/services/loadDimensionBounds.test.tslightly_studio_view/src/lib/services/loadDimensionBounds.tslightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/+layout.sveltelightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/getDimensionsCollectionId.test.tslightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/getDimensionsCollectionId.tslightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/getMetadataCollectionId.test.tslightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/getMetadataCollectionId.ts
Comment on lines
+56
to
+72
| if (!isReadableStore<string | undefined>(collectionId)) { | ||
| loadInitialDimensionBounds(collectionId); | ||
| return source; | ||
| } | ||
|
|
||
| return derived([source, collectionId], ([$source, $collectionId]) => { | ||
| if ($collectionId) { | ||
| loadInitialDimensionBounds($collectionId); | ||
| } | ||
| return readable(get(source), (set) => { | ||
| const unsubscribeSource = source.subscribe(set); | ||
| const unsubscribeCollectionId = collectionId.subscribe(($collectionId) => { | ||
| if ($collectionId) { | ||
| loadInitialDimensionBounds($collectionId); | ||
| } | ||
| }); | ||
|
|
||
| return $source; | ||
| return () => { | ||
| unsubscribeSource(); | ||
| unsubscribeCollectionId(); | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear dimension state when the readable collection ID becomes undefined.
getDimensionsCollectionId intentionally emits undefined for views without an image-backed collection, but this subscriber silently ignores that transition. The module-level dimensionsBounds and dimensionsValues therefore retain the previous image collection’s range, allowing stale dimensions to affect later filter/count requests. Clear both stores and reset lastCollectionId on the falsy transition so in-flight responses cannot repopulate stale values. Based on the supplied collection-scoping contract, undefined is a valid state that must invalidate the previous collection.
Suggested reset handling
const unsubscribeCollectionId = collectionId.subscribe(($collectionId) => {
if ($collectionId) {
loadInitialDimensionBounds($collectionId);
+ } else {
+ lastCollectionId.set(null);
+ dimensionsBounds.set(null);
+ dimensionsValues.set(null);
}
});Apply the same reset helper to the no-collection-ID path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!isReadableStore<string | undefined>(collectionId)) { | |
| loadInitialDimensionBounds(collectionId); | |
| return source; | |
| } | |
| return derived([source, collectionId], ([$source, $collectionId]) => { | |
| if ($collectionId) { | |
| loadInitialDimensionBounds($collectionId); | |
| } | |
| return readable(get(source), (set) => { | |
| const unsubscribeSource = source.subscribe(set); | |
| const unsubscribeCollectionId = collectionId.subscribe(($collectionId) => { | |
| if ($collectionId) { | |
| loadInitialDimensionBounds($collectionId); | |
| } | |
| }); | |
| return $source; | |
| return () => { | |
| unsubscribeSource(); | |
| unsubscribeCollectionId(); | |
| }; | |
| if (!isReadableStore<string | undefined>(collectionId)) { | |
| loadInitialDimensionBounds(collectionId); | |
| return source; | |
| } | |
| return readable(get(source), (set) => { | |
| const unsubscribeSource = source.subscribe(set); | |
| const unsubscribeCollectionId = collectionId.subscribe(($collectionId) => { | |
| if ($collectionId) { | |
| loadInitialDimensionBounds($collectionId); | |
| } else { | |
| lastCollectionId.set(null); | |
| dimensionsBounds.set(null); | |
| dimensionsValues.set(null); | |
| } | |
| }); | |
| return () => { | |
| unsubscribeSource(); | |
| unsubscribeCollectionId(); | |
| }; |
🤖 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/hooks/useDimensions/useDimensions.ts` around
lines 56 - 72, Update the collectionId subscription in the readable returned by
the dimensions hook to handle falsy values by invoking the existing reset helper
that clears dimensionsBounds and dimensionsValues and resets lastCollectionId.
Apply the same reset handling in the no-collection-ID path before returning
source, ensuring undefined invalidates prior collection state and prevents stale
responses from repopulating it.
…s' of github.com:lightly-ai/lightly-studio into feature/lig-9585-categorical-metadata-distribution.hooks
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.
What has changed and why?
This PR introduces changes required to render dataset distribution panel with categorical metadata values
How has it been tested?
Accompanied by unit tests
Did you update CHANGELOG.md?
Summary by CodeRabbit
nullin filter predicates and chip labels.