Adjust DatasetDistributionPanel to be able to render categorical metadata values - #1814
Open
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ikondrat
changed the base branch from
main
to
feature/lig-9585-categorical-metadata-distribution.ExpandDialog
July 30, 2026 14:53
Contributor
📝 WalkthroughWalkthroughChangesCategorical distribution experience
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DatasetDistributionPanel
participant MetadataCategoricalFilter
participant BarChart
participant DistributionConfigDialog
User->>MetadataCategoricalFilter: select categorical value
MetadataCategoricalFilter->>DatasetDistributionPanel: toggle value
DatasetDistributionPanel->>BarChart: render filtered buckets
User->>BarChart: click bucket
BarChart->>DatasetDistributionPanel: report selectable bucket
User->>DistributionConfigDialog: configure values
DistributionConfigDialog->>DatasetDistributionPanel: apply categorical configuration
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 |
❌ Fast Track: checks did not pass
To run the guardrails locally, from Reflects |
Contributor
Author
|
/review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.ts (1)
232-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUncovered categorical paths:
onCategoricalValuesClearandfilteredBuckets.The new suite covers toggling, config, orientation, and load/error states, but not the clear-all callback (
onCategoricalValuesClear) nor the background/foreground rendering driven byfilteredBuckets→filteredCount, which is the main new visual contract inDatasetDistributionPanel.svelte(Lines 129-135). Adding two small cases here would lock that behavior in.🤖 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 232 - 282, Extend the DatasetDistributionPanel tests with two focused cases: verify the clear-all interaction invokes onCategoricalValuesClear for the relevant group, and verify filteredBuckets drives filteredCount/background-versus-foreground rendering in the chart options. Reuse the existing render and echartsMock patterns, and assert the resulting callback arguments and visual data contract.lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte (1)
176-192: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOrientation toggle freezes the categorical visible count at the current bucket count.
categoricalConfigonly derivesnfromcategoricalData.lengthwhile no config is stored for the group. As soon as anysetCategoricalConfigcall happens — includingonToggleOrientationat Line 396, which is unrelated to top-N — the group'snis persisted at the then-current bucket count. If buckets later grow (refetch, different filter), bars beyond that stalenare hidden and a "Show all" hint appears without the user having configured anything.Consider tracking orientation separately from the top-N config, or keeping
mode: 'topN'auto-sized until the user explicitly applies a config from the dialog.🤖 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 176 - 192, Update the categorical configuration flow around categoricalConfig and setCategoricalConfig so orientation-only changes from onToggleOrientation do not persist the current categoricalData.length as a fixed n. Keep mode: 'topN' automatically sized to the latest bucket count until the user explicitly applies a top-N configuration from the dialog, while preserving explicitly configured n 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.
Nitpick comments:
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.svelte`:
- Around line 176-192: Update the categorical configuration flow around
categoricalConfig and setCategoricalConfig so orientation-only changes from
onToggleOrientation do not persist the current categoricalData.length as a fixed
n. Keep mode: 'topN' automatically sized to the latest bucket count until the
user explicitly applies a top-N configuration from the dialog, while preserving
explicitly configured n values.
In
`@lightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.test.ts`:
- Around line 232-282: Extend the DatasetDistributionPanel tests with two
focused cases: verify the clear-all interaction invokes onCategoricalValuesClear
for the relevant group, and verify filteredBuckets drives
filteredCount/background-versus-foreground rendering in the chart options. Reuse
the existing render and echartsMock patterns, and assert the resulting callback
arguments and visual data contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8e47f3c-91ff-4565-a935-e2464bfb80a8
📒 Files selected for processing (16)
lightly_studio_view/src/lib/components/BarChart/BarChart.sveltelightly_studio_view/src/lib/components/DatasetDistributionPanel/DatasetDistributionPanel.stories.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/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/sourceFixtures.tslightly_studio_view/src/lib/components/DatasetDistributionPanel/types.tslightly_studio_view/src/lib/components/MetadataFilterChips/MetadataFilterChips.sveltelightly_studio_view/src/lib/components/MetadataFilterChips/MetadataFilterChips.test.ts
| @@ -12,12 +12,15 @@ | |||
| import PanelHeader from './PanelHeader/PanelHeader.svelte'; | |||
Contributor
There was a problem hiding this comment.
This component will be more than 500 lines long. Did you check whether we could factor it out?
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
2 participants
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 ability to render categorical metadata values within distribution Panel.
There is no breaking changes for existing functionality for classes and numeric metadata values.
How has it been tested?
Accompanied by test and storybook stores to show how dataset distribition panel looks with categorical metadata values.
Did you update CHANGELOG.md?
Summary by CodeRabbit
New Features
Bug Fixes