fixed version to merge - #1806
Open
ikondrat wants to merge 1 commit into
Open
Conversation
- Add useCategoricalMetadataDistribution to the $lib/hooks mock in layout.workspace.test.ts (missing after the merge brought in its usage in +layout.svelte) - Add itemNoun prop to DistributionConfigDialog and pass it through to ClassSetConfigDialog so the manual-selector empty state reads "No value found." for categorical distributions - Pass itemNoun to DistributionConfigDialog from DatasetDistributionPanel - Update DistributionConfigDialog test to supply itemNoun: 'value' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughChangesThe PR adds categorical metadata distributions, filtering, selection chips, configuration dialogs, retry/loading states, and categorical filter propagation across image, frame, video, and infinite-image queries. Chart spacing and selectable-bar handling are also updated. Categorical metadata distribution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DatasetLayout
participant DistributionHooks
participant DatasetDistributionPanel
participant MetadataCategoricalFilter
participant FilterBuilder
participant MediaQueries
DatasetLayout->>DistributionHooks: request categorical buckets
DistributionHooks-->>DatasetLayout: return buckets and filtered counts
DatasetLayout->>DatasetDistributionPanel: provide distribution groups
DatasetDistributionPanel->>MetadataCategoricalFilter: render value selections
MetadataCategoricalFilter->>DatasetLayout: toggle categorical value
DatasetLayout->>FilterBuilder: build metadata predicates
FilterBuilder-->>MediaQueries: return categorical metadata filters
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
❌ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lightly_studio_view/src/lib/components/MetadataFilterChips/useMetadataFilterChips.svelte.ts (1)
115-140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winP2 — Track categorical filter changes.
Both categorical early-return branches bypass
trackFilterChanged, so enabling, disabling, or clearing categorical filters is absent frommetadata_filter_changedanalytics.Proposed fix
updateCategoricalMetadataValues({ ...categoricalStore.current, [key]: checked ? lastCategoricalValues[key] : [] }); + trackFilterChanged(key, checked ? 'enabled' : 'disabled'); return; @@ lastCategoricalValues = Object.fromEntries( Object.entries(lastCategoricalValues).filter(([valueKey]) => valueKey !== key) ); + trackFilterChanged(key, '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 115 - 140, Update the categorical branches in the filter toggle and clear handlers to call trackFilterChanged with the affected key and enabled/disabled state before returning. Ensure categorical enable, disable, and clear actions emit metadata_filter_changed analytics consistently with range filters.lightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/layout.workspace.test.ts (1)
111-118: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the categorical metadata members to the
useMetadataFiltersmock.
+layout.sveltenow derivesmetadataInfo,categoricalMetadataValues, andupdateCategoricalMetadataValues; themetadataFiltersderivation subscribes to the new categorical store, so the current mock makes these tests fail with an undefined subscription error.🛠️ Proposed mock update
vi.mock('$lib/hooks/useMetadataFilters/useMetadataFilters.js', () => ({ useMetadataFilters: vi.fn(() => ({ metadataValues: writable({}), metadataBounds: writable({}), - updateMetadataValues: vi.fn() + metadataInfo: writable([]), + categoricalMetadataValues: writable({}), + updateMetadataValues: vi.fn(), + updateCategoricalMetadataValues: vi.fn() })), createMetadataFilters: vi.fn(() => undefined) }));🤖 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/routes/datasets/`[dataset_id]/[collection_type]/[collection_id]/layout.workspace.test.ts around lines 111 - 118, Update the useMetadataFilters mock to provide the categorical metadata stores and updater expected by +layout.svelte: add writable-backed categoricalMetadataValues and a vi.fn() updateCategoricalMetadataValues alongside the existing metadata members, so metadataFilters can subscribe without undefined values.
🤖 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/ExpandDialog/ExpandDialog.svelte`:
- Around line 100-104: The ExpandDialog invocation of DistributionConfigDialog
passes the plural noun but not the singular noun, causing categorical
manual-selector text to default to “class.” Forward the corresponding singular
category noun alongside itemNounPlural, using the existing category noun symbol
from the surrounding configuration.
In
`@lightly_studio_view/src/routes/datasets/`[dataset_id]/[collection_type]/[collection_id]/+layout.svelte:
- Around line 665-675: Update the categorical distribution mapping and the
associated retry handler near onCategoricalRetry to surface failures from
categoricalMetadataFilteredQuery: expose its error when the filtered query fails
and allow retrying that query alongside the unfiltered query. Keep the existing
loading behavior unless the intended UX explicitly requires showing
filtered-query fetching as loading.
---
Outside diff comments:
In
`@lightly_studio_view/src/lib/components/MetadataFilterChips/useMetadataFilterChips.svelte.ts`:
- Around line 115-140: Update the categorical branches in the filter toggle and
clear handlers to call trackFilterChanged with the affected key and
enabled/disabled state before returning. Ensure categorical enable, disable, and
clear actions emit metadata_filter_changed analytics consistently with range
filters.
In
`@lightly_studio_view/src/routes/datasets/`[dataset_id]/[collection_type]/[collection_id]/layout.workspace.test.ts:
- Around line 111-118: Update the useMetadataFilters mock to provide the
categorical metadata stores and updater expected by +layout.svelte: add
writable-backed categoricalMetadataValues and a vi.fn()
updateCategoricalMetadataValues alongside the existing metadata members, so
metadataFilters can subscribe without undefined values.
🪄 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: 16b411dd-e515-4374-a795-c36b0d1aed0b
📒 Files selected for processing (34)
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/Images/Images.sveltelightly_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/useMetadataFilters/useMetadataFilters.tslightly_studio_view/src/lib/hooks/useVideoFilters/useVideoFilters.test.tslightly_studio_view/src/lib/hooks/useVideoFilters/useVideoFilters.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]/frames/+page.sveltelightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/layout.workspace.test.tslightly_studio_view/src/routes/datasets/[dataset_id]/[collection_type]/[collection_id]/videos/+page.svelte
💤 Files with no reviewable changes (1)
- lightly_studio_view/src/lib/hooks/useMetadataFilters/useMetadataFilters.ts
Comment on lines
+100
to
+104
| items={configurationItems} | ||
| {config} | ||
| {showCountMode} | ||
| itemNounPlural={categoryNounPlural} | ||
| {sortLabels} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward the singular category noun too.
DistributionConfigDialog defaults itemNoun to "class", so the expanded categorical manual selector will show singular class wording (for example, “No class found.”) despite receiving itemNounPlural="values".
Proposed fix
items={configurationItems}
{config}
{showCountMode}
+ itemNoun={categoryNoun}
itemNounPlural={categoryNounPlural}
{sortLabels}📝 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
| items={configurationItems} | |
| {config} | |
| {showCountMode} | |
| itemNounPlural={categoryNounPlural} | |
| {sortLabels} | |
| items={configurationItems} | |
| {config} | |
| {showCountMode} | |
| itemNoun={categoryNoun} | |
| itemNounPlural={categoryNounPlural} | |
| {sortLabels} |
🤖 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, The ExpandDialog invocation of DistributionConfigDialog
passes the plural noun but not the singular noun, causing categorical
manual-selector text to default to “class.” Forward the corresponding singular
category noun alongside itemNounPlural, using the existing category noun symbol
from the surrounding configuration.
Comment on lines
+665
to
+675
| categorical: { | ||
| buckets: categoricalMetadataDistributions[key] ?? [], | ||
| // undefined until the filtered query has returned so the | ||
| // distribution panel waits before showing background bars. | ||
| filteredBuckets: categoricalMetadataFilteredDistributions?.[key], | ||
| selectedValues: $categoricalMetadataValues[key] ?? [], | ||
| loading: categoricalMetadataQuery.isFetching, | ||
| error: categoricalMetadataQuery.error?.message | ||
| } | ||
| })) | ||
| ] |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Retry and error surfacing only cover the unfiltered categorical query.
error/loading come solely from categoricalMetadataQuery, and onCategoricalRetry refetches only that query. If categoricalMetadataFilteredQuery fails, filteredBuckets stays undefined forever with no error shown and no way to retry.
♻️ Suggested wiring
- loading: categoricalMetadataQuery.isFetching,
- error: categoricalMetadataQuery.error?.message
+ loading:
+ categoricalMetadataQuery.isFetching ||
+ categoricalMetadataFilteredQuery.isFetching,
+ error:
+ categoricalMetadataQuery.error?.message ??
+ categoricalMetadataFilteredQuery.error?.message- onCategoricalRetry={() =>
- categoricalMetadataQuery.refetch()}
+ onCategoricalRetry={() => {
+ categoricalMetadataQuery.refetch();
+ categoricalMetadataFilteredQuery.refetch();
+ }}Note that folding the filtered query's isFetching into loading will also make the "Updating values…" hint appear on filter changes — verify that is the desired UX before adopting the loading part.
Also applies to: 929-930
🤖 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/routes/datasets/`[dataset_id]/[collection_type]/[collection_id]/+layout.svelte
around lines 665 - 675, Update the categorical distribution mapping and the
associated retry handler near onCategoricalRetry to surface failures from
categoricalMetadataFilteredQuery: expose its error when the filtered query fails
and allow retrying that query alongside the unfiltered query. Keep the existing
loading behavior unless the intended UX explicitly requires showing
filtered-query fetching as loading.
2 tasks
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?
(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