Fix/explorer sort dropdown auto close - #4481
Merged
Merged
Conversation
Migrate the daisyUI sort dropdown from pure-CSS focus-within to a controlled signal pattern (isOpen + document:click HostListener), matching MapSelector / DeltaSelector. The menu now closes after the user picks a sort key, toggles the order, or clicks outside. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the top ribbonBar, metricChooser, scenariosPanel and slider stack with a new DaisyUI-based floating metrics bar (features/metricsBar/): per-axis distribution histograms, color/edge segments, and native-popover search/settings. Removes the @angular/material dependency from these surfaces and deletes the superseded components. Includes fixes from a multi-agent review of the change: - visibleNodeMetricValues selector uses boundary-safe path matching so sibling folders sharing a path prefix are no longer pulled into the histogram - nodeSelection.findTopLevelNode guards the empty-visible-nodes case - metricColorRangeDiagram percentile calcs always return a finite value (no NaN) - screenshot capture again subtracts the bottom bar height - color range slider thumbs get slider ARIA roles + keyboard control; link Color/Height button gets an accessible label - colorSettingsPopover cancels its pending debounce on destroy - axisColorRamp normalises inverted color ranges - shared settingsInput util (debounce constant + parse/validate helper) Backfills unit specs across the new metrics bar components and the visibleNodeMetricValues selector. Full suite: 363 suites / 2094 tests green.
Top metric labels were selected by rendered building height, but getHeightValue inverts the rendered height when the height metric has inverted attribute direction or 'invert height' is enabled (maxHeight - value). As a result the lowest-value buildings were labeled instead of the highest (e.g. a file with complexity 4 labeled while one with 223 was not). Rank the height-mode label candidates by the height-metric value itself, so the highest-value buildings are always labeled regardless of how height is rendered. Behavior is unchanged for non-inverted metrics (rendered height already tracks the value). Adds a regression test.
The active/hovered metric option in the select dropdown showed a primary-colored ring and no fill, because the static bg-transparent class tied with bg-base-200 at equal specificity and won. Drop the ring and move bg-transparent into the inactive branch only, so the hovered/active option paints a solid grey (bg-base-200).
…vers - Extract MetricSegmentBase shared by the area/height segments (#44). - Extract a reusable cc-settings-popover-shell container used by the area, height, color and edge settings popovers (#47); all popover ids, anchors, testids and width behavior preserved. - parseNumberInput now uses parseFloat so numeric settings inputs accept decimals. - Fix the Color 'Reset thresholds' button resetting delta map colors as a side effect; it now resets only dynamicSettings.colorRange.
…elect e2e Strengthen dispatch and popover-wiring coverage across the metric segments, toggles and settings popovers; add the settings-popover-shell spec, parseFloat decimal cases, visibleNodeMetricValues selector edge cases, and a Playwright e2e for the metric-select flow (unverified locally — needs a build + Chrome).
setLabels ranked the top-N labels by sorting every leaf building on each render (O(n log n)). Replace the full sort with a partial top-N selection (selectTopNByValue) that keeps an n-sized window in one pass — identical result and ordering, but avoids sorting thousands of buildings to pick a handful of labels. Behaviour (rank by height-metric value) is unchanged.
…another selectBuilding overwrote the selected reference and colored the new building but never reset the previously selected one, so its highlight lingered. Two parts: - Reset the previously selected building's color when switching selection. - Invalidate the mesh's incremental highlight cache (_prevHighlightedIds) on any selection change, so the next highlight pass does a full recompute. Without this, the just-deselected building kept its bright default color against the dimmed focus state (the selected building is always skipped by the incremental diff), which is why it only cleared after repeatedly hovering elsewhere.
…architecture The metrics bar lived under features/ but accessed the ngrx Store directly from components and imported the scenarios feature internals, failing the dependency-cruiser architecture lint (21 errors) that CI gates on. Mirror the labelSettings architecture: add per-slice stores/ (the only layer that imports @ngrx/store) and delegating services/, move feature-local selectors into selectors/, and route every component through the services instead of injecting Store. Access the scenarios feature via its facade. Behavior is unchanged. Architecture lint: 0 errors (was 21). Full suite: 365 suites / 2128 tests green.
…nd top-label perf
|
Important Review skippedToo many files! This PR contains 300 files, which is 150 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (300)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…and improve metrics-bar a11y Reuse returnIgnore in createBlacklistMatcher and the explorer rule counts so flatten/exclude matching mirrors NodeDecorator, including negated !-rules. Only line-through flattened files (not folders) in the explorer tree. Replace the color range slider's role=slider divs with native range inputs. Extract helpers in histogramBins and visibleNodeMetricValues selectors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…and mid-typing values - parseNumberInput no longer overwrites the input element while typing, so intermediate out-of-range values (the "4" while typing "45") survive; adds an optional round flag for integer-only settings - extract shared cc-slider-number-input control used by the area, height and edge settings popovers: debounced commits, step rounding, display normalization on change, and no stale intermediate commits when retyping the current value - selectTopNByValue floors fractional n and ranks NaN below real values instead of crashing or blocking the top-N window - edgePreviewNodes selector floors fractional amountOfEdgePreviews - debounce util gains flush() and cancel() Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tate and ramp colors - measure the color-range slider track width instead of assuming a fixed 395px, so thumbs stay on the track on narrow viewports - abort thumb drags when the popover is light-dismissed mid-drag and clean up document listeners on destroy instead of dispatching a garbage range - step arrow-key and typed threshold changes from the pending value so repeated presses within the debounce window accumulate - track color inversion with an explicit flag in mapColors state so the Invert Colors checkbox stays meaningful after colors are customized - flush the pending color range on popover destroy instead of discarding it, and use the shared debounce util instead of a hand-rolled timer - extract getColorByMetricValue as the single classification used by the 3D map and the axis color ramp: bins at the thresholds now match their buildings, and the ramp respects the selected color mode Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
createBlacklistMatcher is now the single engine for blacklist rules, used by NodeDecorator, both layouts, the metric calculators, the file explorer counts and selection cleanup, so they can never disagree on what a rule affects: - isExcludedLeaf applies NodeDecorator's leaf-only exclude semantics, including negated !-rules - isExcludedSubtree is the only folder-level check; it ignores negated rules, so the street layout no longer prunes folder subtrees that still contain non-excluded leaves - isPathBlacklisted/isPathHiddenOrExcluded use the shared matcher instead of raw gitignore semantics, so selection cleanup agrees with what NodeDecorator removes from the map - NodeDecorator's private engine copy is deleted; the explorer's per-rule counts get back the combined-engine prefilter for positive rules - delete unused isNodeExcludedOrFlattened Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…bar migration - keep the edge metric chooser usable while edges are hidden (dim instead of disable) and restore the sum/median aggregation indicator next to hovered edge values - initialize keyboard navigation in the metric select popover on the currently selected metric so Enter right after opening is a no-op - guard METRIC_ALIASES lookups with Object.hasOwn so metrics named like Object prototype members cannot break the metric list - position settings popovers via JS next to their anchor in browsers without CSS Anchor Positioning (Firefox) instead of viewport-centered - wrap metrics-bar segments in real boxes so divide-x separators paint (segment hosts are display:contents) - render one full histogram bin for metrics where min equals max so a populated metric is distinguishable from no data - metricChooserType uses the tested createAttributeTypeSelector instead of re-implementing the aggregation logic inline - metricsBar spec: histogram test now feeds visibleNodeMetricValuesSelector (what the component actually reads) and asserts rendered bars Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ing state - clearSelection nulls the selection before the highlight repaint so the just-deselected building cannot keep its stale selection color - remap the selected building onto the new mesh on mesh swap (or drop it when the building is gone) instead of keeping a stale old-mesh reference - top labels rank by rendered building height again, and the color-label branch reuses selectTopNByValue instead of sorting all matching leaves - hovering a folder outside the focused subtree no longer rebuilds the metrics-bar histograms from buildings that are not rendered - screenshots measure the bottom bar via its inner footer (the host box is always zero-height), so the capture actually crops the bottom bar Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- visibleNodeMetricValues keys on the derived hovered-folder path, so leaf hovers (10-30/s while sweeping) no longer re-walk the whole map; it also collects only the displayed area/height/color metrics and drops the unused sum - topLevelNode depends on area/height metric instead of the whole dynamicSettings slice, so search keystrokes and margin drags no longer re-run the full layout for a fallback display value - color range diagram computes percentiles with one sort instead of an O(unique x N) scan, redraws only the colored areas for thumb-only changes, and defers all work while its popover is closed - metric select popovers render their option list lazily on open instead of keeping ~300 buttons per popover in the DOM - selection clicks update the highlight diff incrementally (repaint the formerly selected building) instead of invalidating the cache and re-uploading the full color buffer Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er shells - delete the unused AccumulatedData/DynamicSettings/HoveredNode/SelectedNode store+service mirror pairs (zero consumers) - rename the duplicate root singletons: sidebarExplorer's AreaMetricStore to ExplorerAreaMetricStore and 3dPrint's ColorModeStore (which forces ColorMode.absolute) to Export3DColorModeStore, so auto-imports cannot mix them up with the metricsBar stores - remove never-called setters setIsEdgeMetricVisible, setIsHeightAndColorMetricLinked and setMapColors from store and service - metric select popover and the labels/scenarios segment use SettingsPopoverShellComponent instead of hand-rolled popover markup (which had already drifted), gaining the Firefox positioning fallback - visibleNodeMetricValues uses the shared isLeaf helper - fix the two remaining spec type errors so tsc --noEmit is clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A failing assertion in the settings popover specs could previously skip the inline jest.useRealTimers() call and leak fake timers into the rest of the test file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Also check in the max-effort review plan and findings plan; all findings are fixed except F48 (store/service layer collapse), which is deferred as a team decision. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… type The new boolean inversion flags on MapColors widened the template-indexed access in cc-color-picker-for-map-color to string | boolean, which only the AOT template check catches. Restrict mapColorFor to color-valued keys. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… top-N selection Benchmarked against the pre-refactor implementations with 200k nodes: - BlacklistMatcher gains classify(path, isLeaf) that transforms the path once and uses plain loops instead of Array.some closures; NodeDecorator and the explorer counts use it, removing the ~10% decoration-pass regression the shared-matcher refactor had introduced (now at parity, ~500ms for 200k nodes x 6 rules, with folders still skipping negated exclude engines) - selectTopNByValue caches the window's boundary value so the common case (item not in the top n) costs one comparison instead of re-reading the boundary item (2.2ms -> 1.4ms for 200k items) Verified unchanged results and confirmed the earlier wins: percentile ranks 2928ms -> 14ms (100k values), per-rule explorer counts 985ms -> 466ms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Retyping the value that was already pending skipped re-arming the timer, so the commit could fire mid-typing. The number inputs now push the debounce window out on each keystroke; the label settings panel also drops a pending intermediate value when typing returns to the committed one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r segments Design decision after seeing them render; also reverts the wrapper divs that only existed to give divide-x real boxes to paint between. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The displayed metric values follow hovered ?? selected ?? top level, but the histograms only knew hover and focus: after selecting a folder floor the value stayed pinned while the histograms fell back to the whole map. The histogram prefix now mirrors the same precedence (selected leaves are ignored, and a selection outside the focused subtree cannot widen the view). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…trap tasks Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-axis histograms and the color ramp did not add enough value to justify their surface: delete the axisDistribution and axisColorRamp components and the histogramBins util with their specs. The segments keep the hovered/selected metric value and the min/max range of the rendered buildings (still narrowing to the focused, hovered or selected subtree). - rename DistributionSegmentComponent to MetricSegmentComponent, since the distribution was the only thing the name referred to - visibleNodeMetricValues collects only min/max per metric, no longer the full value arrays the histograms binned - the color segment derives its min/max labels from the global color metric data alone and drops the ramp's colorMode/mapColors/colorRange wiring Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…layer metricChooserType injected the ngrx Store directly, which violates the feature-only-stores-can-import-ngrx-store dependency-cruiser rule that failed CI. The createAttributeTypeSelector is now exposed via AttributeTypesStore/-Service like every other selector in the feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… segments The row was the histograms' axis labeling and is not needed without them. With it gone the whole visibleNodeMetricValues selector/store/service chain and MetricSegmentBase have no consumers left and are deleted; the segments show only the hovered/selected metric value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bumps transitive dev dependencies (fast-xml-parser family, postcss, nanoid, qs, ws, xmldom and others) to patched versions. The three newly added transitive packages (@nodable/entities, xml-naming, anynum) are part of the fast-xml-parser author's package family and were verified against the registry and their tarball contents (no install scripts, no suspicious code). The remaining high advisory is electron itself, which only an across-major bump (audit fix --force) would address. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dependency lifecycle scripts (preinstall/install/postinstall) are the main npm supply-chain attack vector; with ignore-scripts=true in .npmrc they can no longer execute code during npm install. Audit of both lockfiles: no root dependency has an install script, and all native visualization dependencies (esbuild, lmdb, @parcel/watcher, msgpackr-extract, unrs-resolver, fsevents) ship prebuilt platform packages whose install scripts are only fallbacks. The two real exceptions are handled explicitly: - electron: script/ensureElectron.js downloads the binary on demand before npm run start / npm run package (this also repairs installs where node_modules/electron/path.txt is missing) - husky: git hooks are set up via npm run prepare, wired into the mise install task Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- extract the per-rule counting loops out of buildRulesWithCount to get its
cognitive complexity under the threshold (S3776)
- make the NaN guards on floored limits explicit (Number.isNaN || <= 0)
instead of the negated comparison Sonar flags (S1940) — plain <= 0 alone
would let NaN through
- use entries.at(-1) for the last resize observer entry (S7755)
- drop a type assertion that querySelector("footer") already implies (S4325)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… code - selectTopNByValue/parseNumberInput: named helpers (rankNaNBelowEveryValue, collectTopWindow, clamp) instead of comment blocks - codeMapMesh: extract invalidateDiffCacheWhenPresentationModeChanged and repaintFormerlySelectedWhenSelectionChanged - metric alias lookup uses a Map, removing the hasOwn guard and its comment - metricMetaValue builds one display view model from small pure functions; the template shrinks to a single state switch - explorer counts read the decorated isFlattened/isExcluded flags directly (they are re-decorated on every blacklist change, so the matcher round trip was redundant); the rule-count prefilter comment became buildRuleEnginesMatchingNodeDecorator Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- extract cc-axis-card-header (label + settings cog) and share the card content between the clickable and static variants via one template instead of duplicating it per branch - extract cc-metric-select-option so the popover template is the search field plus a list, with the option row markup in its own component Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
codeMapHelper had grown into a grab-bag. The gitignore-based blacklist machinery (transformPath, returnIgnore, addRulePatternsToEngine, the BlacklistMatcher and the isPathBlacklisted/isPathHiddenOrExcluded helpers) now lives in util/blacklist/blacklistMatcher.ts next to the other blacklist utilities; codeMapHelper keeps only the generic code-map node helpers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The li now lives in the parent list, the option component renders only the button — fixes the Sonar item-tag warning and the invalid ul nesting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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.
{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