feat(visualization): redesign color settings popover - #4486
Conversation
Single-column layout following the new design mockup: header with metric name and reset-thresholds icon, compact quantile distribution chart, segmented gradient-mode control, color bands with building counts, and a folder overrides section to pin, recolor, and unpin fixed folder colors (backed by marked packages) with a scrollable list and folder search. Colors are picked through a new inline color picker that renders inside the native popover, since the previous overlay-based picker light-dismissed the popover. Reset colors now also resets the gradient mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughRefactors the Color settings popover into modular single-column sections, adds an inline color picker and gradient-mode radios, implements folder overrides (pin/recolor/unpin) with selectors/stores/services, updates diagram sizing and reset button sizing, and adds comprehensive unit tests and plan/changelog updates. ChangesColor Settings Popover Restyle with Folder Overrides
Estimated code review effort:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.ts (2)
23-25: 💤 Low valueConsider defensive handling for selector synchronization.
The
requireSync: trueoption on Lines 23-25 will throw an error if the selectors don't emit synchronously. While this may be intentional to catch configuration errors early, it makes the component fragile during initialization or testing.♻️ Alternative: Provide default values
- readonly overrides = toSignal(this.store.select(markedPackagesWithCountsSelector), { requireSync: true }) - private readonly folderPaths = toSignal(this.store.select(markableFolderPathsSelector), { requireSync: true }) - private readonly mapColors = toSignal(this.store.select(mapColorsSelector), { requireSync: true }) + readonly overrides = toSignal(this.store.select(markedPackagesWithCountsSelector), { initialValue: [] }) + private readonly folderPaths = toSignal(this.store.select(markableFolderPathsSelector), { initialValue: [] }) + private readonly mapColors = toSignal(this.store.select(mapColorsSelector))🤖 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 `@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.ts` around lines 23 - 25, The component currently uses toSignal(...) with requireSync: true for overrides, folderPaths, and mapColors (from markedPackagesWithCountsSelector, markableFolderPathsSelector, and mapColorsSelector) which will throw if the selectors do not emit synchronously; change to a defensive pattern by removing requireSync or setting it to false and provide safe default values so the component can initialize without synchronous selector emissions (e.g., use toSignal(this.store.select(...), { requireSync: false }) or ensure the selector stream is seeded with a default via startWith) and update any code that assumes immediate non-null values to handle the default until the real selector emits.
47-50: ⚡ Quick winAdd cleanup for the
setTimeoutto prevent potential memory leaks.The
setTimeouton Line 49 schedules focus without tracking the timer ID. If the component is destroyed before the timeout fires, the callback will attempt to access the destroyed view reference, potentially causing errors or memory leaks.♻️ Proposed fix using Angular's `afterNextRender`
-import { ChangeDetectionStrategy, Component, computed, ElementRef, inject, signal, viewChild } from "`@angular/core`" +import { afterNextRender, ChangeDetectionStrategy, Component, computed, ElementRef, inject, signal, viewChild } from "`@angular/core`"+ private readonly focusInputAfterRender = afterNextRender(() => { + if (this.isPinning()) { + this.pinInput()?.nativeElement.focus() + } + }) + startPinning() { this.isPinning.set(true) - setTimeout(() => this.pinInput()?.nativeElement.focus()) }🤖 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 `@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.ts` around lines 47 - 50, The startPinning method schedules a focus with setTimeout but doesn't store the timer ID; modify startPinning (in folderOverrides.component.ts) to capture the returned timeout ID (e.g., this.focusTimeoutId = setTimeout(...)) and ensure the callback uses existing pinInput()?nativeElement.focus(), then implement ngOnDestroy in the same component to call clearTimeout(this.focusTimeoutId) to cancel any pending timer and prevent the callback from running after component teardown.visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.html (1)
21-23: ⚡ Quick winVerify accessibility: Ensure the unpin button has appropriate ARIA label.
The unpin button on Line 21 uses a
titleattribute but no explicitaria-label. Screen readers may not announce the button's purpose clearly, especially since the visible content is just an icon (fa-times).♿ Proposed fix to add ARIA label
- <button class="btn btn-ghost btn-xs btn-square" [title]="'Unpin ' + override.path" (click)="handleUnpin(override.path)"> + <button class="btn btn-ghost btn-xs btn-square" [attr.aria-label]="'Unpin ' + override.path" [title]="'Unpin ' + override.path" (click)="handleUnpin(override.path)"> <i class="fa fa-times"></i> </button>🤖 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 `@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.html` around lines 21 - 23, Add an explicit ARIA label to the unpin button so screen readers announce its purpose: update the button element that calls handleUnpin(override.path) to include an aria-label (e.g., aria-label="'Unpin ' + override.path" or use the same i18n string as the title) so the icon-only button has accessible text describing the action.
🤖 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
`@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandRow.component.spec.ts`:
- Around line 19-20: Split the combined "// Arrange & Act" comments into three
explicit comment blocks "// Arrange", "// Act", and "// Assert" in the unit
test(s) that call setup({ mapColorFor: "positive", count: 312 }) (and the
related tests at the other occurrences mentioned), so that the setup call and
any preconditions are under "// Arrange", the awaited setup invocation (or
actions that trigger behavior) are under "// Act", and the expectations
(expect(...) assertions) are under "// Assert"; update all three occurrences
(lines with setup and the two other paired lines) to follow this AAA pattern
consistently.
In
`@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.html`:
- Line 16: Remove the invalid ARIA role by deleting role="colorpicker" from the
color-chrome element in inlineColorPicker.component.html (the color-chrome
element referenced in the diff), and if necessary replace it with appropriate
accessible attributes such as aria-label or aria-describedby to convey purpose
rather than an unsupported role.
---
Nitpick comments:
In
`@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.html`:
- Around line 21-23: Add an explicit ARIA label to the unpin button so screen
readers announce its purpose: update the button element that calls
handleUnpin(override.path) to include an aria-label (e.g., aria-label="'Unpin '
+ override.path" or use the same i18n string as the title) so the icon-only
button has accessible text describing the action.
In
`@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.ts`:
- Around line 23-25: The component currently uses toSignal(...) with
requireSync: true for overrides, folderPaths, and mapColors (from
markedPackagesWithCountsSelector, markableFolderPathsSelector, and
mapColorsSelector) which will throw if the selectors do not emit synchronously;
change to a defensive pattern by removing requireSync or setting it to false and
provide safe default values so the component can initialize without synchronous
selector emissions (e.g., use toSignal(this.store.select(...), { requireSync:
false }) or ensure the selector stream is seeded with a default via startWith)
and update any code that assumes immediate non-null values to handle the default
until the real selector emits.
- Around line 47-50: The startPinning method schedules a focus with setTimeout
but doesn't store the timer ID; modify startPinning (in
folderOverrides.component.ts) to capture the returned timeout ID (e.g.,
this.focusTimeoutId = setTimeout(...)) and ensure the callback uses existing
pinInput()?nativeElement.focus(), then implement ngOnDestroy in the same
component to call clearTimeout(this.focusTimeoutId) to cancel any pending timer
and prevent the callback from running after component teardown.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e153ff0-4030-4129-8cfa-2dfb18aba56c
⛔ Files ignored due to path filters (1)
plans/color_metric_popover/restyle_color_metric.pngis excluded by!**/*.png
📒 Files selected for processing (22)
plans/2026-05-13-redesign-color-settings-popover.mdplans/color_metric_popover/2026-06-11-restyle-color-metric-popover.mdvisualization/CHANGELOG.mdvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandRow.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandRow.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandRow.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsPopover.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsPopover.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsPopover.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/metricColorRangeDiagram.component.tsvisualization/app/codeCharta/features/metricsBar/selectors/markableFolderPaths.selector.spec.tsvisualization/app/codeCharta/features/metricsBar/selectors/markableFolderPaths.selector.tsvisualization/app/codeCharta/features/metricsBar/selectors/markedPackagesWithCounts.selector.spec.tsvisualization/app/codeCharta/features/metricsBar/selectors/markedPackagesWithCounts.selector.tsvisualization/app/codeCharta/ui/resetSettingsButton/resetSettingsButton.component.htmlvisualization/app/codeCharta/ui/resetSettingsButton/resetSettingsButton.component.ts
| // Arrange & Act | ||
| await setup({ mapColorFor: "positive", count: 312 }) |
There was a problem hiding this comment.
Use explicit separate AAA comments instead of combined Arrange & Act.
Please split these into distinct // Arrange, // Act, // Assert sections to match the repository’s test pattern consistently.
As per coding guidelines, "Use Arrange-Act-Assert pattern with comments in tests".
Also applies to: 27-28, 36-37
🤖 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
`@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandRow.component.spec.ts`
around lines 19 - 20, Split the combined "// Arrange & Act" comments into three
explicit comment blocks "// Arrange", "// Act", and "// Assert" in the unit
test(s) that call setup({ mapColorFor: "positive", count: 312 }) (and the
related tests at the other occurrences mentioned), so that the setup call and
any preconditions are under "// Arrange", the awaited setup invocation (or
actions that trigger behavior) are under "// Act", and the expectations
(expect(...) assertions) are under "// Assert"; update all three occurrences
(lines with setup and the two other paired lines) to follow this AAA pattern
consistently.
Source: Coding guidelines
Addresses all findings from the code review of the popover redesign: - Inline color picker closes on pointerdown instead of click, so a drag released outside the panel no longer discards the picked color; a pending color is emitted if closing races the debounced change event - Picker panel also closes on container scroll, window resize, and popover close instead of floating at stale viewport coordinates - Escape cancels the folder search without light-dismissing the popover - Suggestion mousedown preventDefault moved off the list so the scrollbar stays draggable - Folder override copy corrected (marking colors tint folder floors) and counts got a "n files" tooltip - Excluded nodes no longer counted or suggested; count attribution is an O(leaves x depth) path walk-up instead of O(leaves x packages) - Nested pins avoid their marked parent's color, which the reducer would drop as redundant - rounded-btn (removed in DaisyUI 5) replaced with rounded-md - Gradient mode group exposes role=radiogroup with a per-popover radio name; reset tooltip no longer mentions gradient mode in delta mode - Components rewired through feature stores/services to satisfy the dependency-cruiser ngrx boundary rule - New tests covering the above plus header threshold reset and the diagram size inputs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The popover and folder overrides templates had grown too large to read. The popover is now a thin composition of focused section components: - cc-color-settings-header (metric name, gradient dot, reset thresholds) - cc-color-range-section (threshold slider + distribution diagram) - cc-gradient-mode-picker (segmented control) - cc-color-bands-section (band rows with counts) - cc-invert-reset-row (invert toggle + reset colors) - cc-folder-override-row / cc-pin-folder-search (extracted from cc-folder-overrides) Each section owns its service wiring, and the moved logic took its tests along (debounced threshold commit, threshold reset, inversion flags, reset keys, gradient mode dispatch). No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Replace the label-styled wrapper around the folder search with a div and give the input an aria-label (Web:S6853) - Drop the invalid "colorpicker" ARIA role from the inline picker (Web:S6821) - Type the inline picker's ElementRef as HTMLElement so the remaining target cast is meaningful (typescript:S4325) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
`@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.ts`:
- Around line 41-48: The code assumes markingColors has at least one entry
before returning markingColors[0]; add a guard for an empty markingColors array
in the function that uses mapColors() and findMarkedParentColor(path): if
markingColors.length === 0 return a safe fallback (e.g., undefined or a known
default color) instead of indexing markingColors[0], so no invalid color is
passed into markPackage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40e56fec-2870-4b7e-820e-440ecc820761
📒 Files selected for processing (39)
plans/color_metric_popover/2026-06-11-restyle-color-metric-popover.mdvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandRow.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandsSection.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandsSection.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandsSection.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorRangeSection.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorRangeSection.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorRangeSection.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsHeader.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsHeader.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsHeader.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsPopover.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsPopover.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorSettingsPopover.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrideRow.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrideRow.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/gradientModePicker.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/gradientModePicker.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/gradientModePicker.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/invertResetRow.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/invertResetRow.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/invertResetRow.component.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/metricColorRangeDiagram.component.spec.tsvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/pinFolderSearch.component.htmlvisualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/pinFolderSearch.component.tsvisualization/app/codeCharta/features/metricsBar/selectors/markableFolderPaths.selector.spec.tsvisualization/app/codeCharta/features/metricsBar/selectors/markableFolderPaths.selector.tsvisualization/app/codeCharta/features/metricsBar/selectors/markedPackagesWithCounts.selector.spec.tsvisualization/app/codeCharta/features/metricsBar/selectors/markedPackagesWithCounts.selector.tsvisualization/app/codeCharta/features/metricsBar/services/folderOverrides.service.tsvisualization/app/codeCharta/features/metricsBar/services/mapColors.service.tsvisualization/app/codeCharta/features/metricsBar/stores/folderOverrides.store.tsvisualization/app/codeCharta/features/metricsBar/stores/mapColors.store.ts
✅ Files skipped from review due to trivial changes (3)
- visualization/app/codeCharta/features/metricsBar/services/folderOverrides.service.ts
- visualization/app/codeCharta/features/metricsBar/stores/folderOverrides.store.ts
- plans/color_metric_popover/2026-06-11-restyle-color-metric-popover.md
🚧 Files skipped from review as they are similar to previous changes (7)
- visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.html
- visualization/app/codeCharta/features/metricsBar/selectors/markedPackagesWithCounts.selector.spec.ts
- visualization/app/codeCharta/features/metricsBar/selectors/markableFolderPaths.selector.spec.ts
- visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/colorBandRow.component.ts
- visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.spec.ts
- visualization/app/codeCharta/features/metricsBar/selectors/markedPackagesWithCounts.selector.ts
- visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/inlineColorPicker.component.ts
| const markingColors = this.mapColors().markingColors | ||
| // a pin with its marked parent's color is dropped as redundant by the reducer, | ||
| // so that color must not be handed out for a nested pin | ||
| const parentColor = this.findMarkedParentColor(path) | ||
| const candidates = markingColors.filter(color => color !== parentColor) | ||
| if (candidates.length === 0) { | ||
| return markingColors[0] | ||
| } |
There was a problem hiding this comment.
Guard empty markingColors before fallback indexing.
At Line 47, markingColors[0] is returned when candidates is empty. If markingColors is empty, this yields undefined and propagates an invalid color into markPackage.
Suggested fix
private nextMarkingColor(path: string) {
const markingColors = this.mapColors().markingColors
+ if (markingColors.length === 0) {
+ return defaultMapColors.markingColors[0]
+ }
// a pin with its marked parent's color is dropped as redundant by the reducer,
// so that color must not be handed out for a nested pin
const parentColor = this.findMarkedParentColor(path)🤖 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
`@visualization/app/codeCharta/features/metricsBar/components/colorSettingsPopover/folderOverrides.component.ts`
around lines 41 - 48, The code assumes markingColors has at least one entry
before returning markingColors[0]; add a guard for an empty markingColors array
in the function that uses mapColors() and findMarkedParentColor(path): if
markingColors.length === 0 return a safe fallback (e.g., undefined or a known
default color) instead of indexing markingColors[0], so no invalid color is
passed into markPackage.
Resolves Sonar S4325 on the color picker outside-click handler. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|



Single-column layout following the new design mockup: header with metric name and reset-thresholds icon, compact quantile distribution chart, segmented gradient-mode control, color bands with building counts, and a folder overrides section to pin, recolor, and unpin fixed folder colors (backed by marked packages) with a scrollable list and folder search.
Colors are picked through a new inline color picker that renders inside the native popover, since the previous overlay-based picker light-dismissed the popover. Reset colors now also resets the gradient mode.
{Meaningful title}
Please read the CONTRIBUTING.md before opening a PR.
Closes: #
Description
Descriptive pull request text, answering:
Definition of Done
A PR is only ready for merge once all the following acceptance criteria are fulfilled:
Screenshots or gifs
Summary by CodeRabbit
New Features
Changed
Bug Fixes