feat(ui): add swimlane view for config changes#3013
Open
moshloop wants to merge 17 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
WalkthroughThis PR introduces a swimlane graph visualization for config changes alongside the existing table view. It adds an infinite-pagination query hook, a complete swimlane component hierarchy with hierarchical grouping and collapsible rows, view-toggle UI, Graph-specific date-range enforcement (max 7 days), live-tail polling gated to table mode, and modal detail inspection in graph mode. ChangesConfig Changes Swimlane Visualization
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![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: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx`:
- Around line 79-89: Replace the non-focusable span used as the HoverCard
trigger with a focusable, named control and ensure icon-only lane buttons get an
accessible name: in ConfigChangesSwimlane swap the <HoverCardTrigger
asChild><span ...> usage to a semantic focusable element (e.g., <button> or an
element with role="button", tabIndex={0}) and add a clear aria-label like
`aria-label={`Severity details for ${key}`}`; similarly, for the lane button
rendering when labelPlacement === "none", ensure it includes an aria-label or a
visually-hidden text (use labelPlacement and the lane identifier to construct
the label) so icon-only buttons have a reliable accessible name. Also ensure
keyboard activation (onKeyDown/Enter/Space) is supported if you keep a
non-button element.
- Around line 514-518: The grouping key currently uses the display name
(c.config?.name) which can merge distinct configs with identical names; change
all grouping and keying logic to use the stable identifier (c.config?.id ??
c.config_id ?? "unknown") when creating the Map (variable grouped) and when
generating row keys, while keeping the human-readable name (c.config?.name) only
for display; update the other occurrences noted (the block around lines 533-548
and the rendering/key usage where row.name is used) so aggregation, severity
totals, and React keys are based on the stable id not the name.
In `@src/components/Configs/Changes/ConfigChangesSwimlaneTooltip.tsx`:
- Around line 123-133: The tooltip currently renders a Link to
`/catalog/${change.config?.id}` even when change.config or change.config.id is
missing, producing `/catalog/undefined`; update the JSX in
ConfigChangesSwimlaneTooltip so the Link is only rendered when change.config &&
change.config.id (use the existing allSameConfig guard plus an explicit check
for change.config?.id), and when the nested config is absent render the same
ConfigsTypeIcon with a non-link fallback label (preserve the onClick
stopPropagation behavior where relevant and keep the <span
className="truncate">{change.config?.name ?? "Unnamed"}</span> to show a safe
label).
In `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts`:
- Around line 173-199: The drag listeners added in onMouseDown
(document.addEventListener("mousemove", onMouseMove) and
document.addEventListener("mouseup", onMouseUp)) are only removed in onMouseUp,
so if the component unmounts mid-drag they leak; modify the component to ensure
those handlers are removed on unmount by capturing the same
onMouseMove/onMouseUp references and providing a cleanup that calls
document.removeEventListener for both (e.g., create the handlers inside
onMouseDown but also store them in refs or expose them to a useEffect cleanup
that removes them when the component unmounts), ensuring you update references
to dragging/userResized/width consistently in onMouseMove and onMouseUp (refer
to onMouseDown, onMouseMove, onMouseUp, and any refs like
dragging.current/startX.current/startWidth.current).
In `@src/components/Configs/Changes/ConfigChangesViewToggle.tsx`:
- Around line 6-14: The hook and component are currently casting the raw query
param with (params.get("view") as ConfigChangesView) which accepts any string;
update useConfigChangesViewToggleState and ConfigChangesViewToggle to validate
the parsed value against the allowed ConfigChangesView options (e.g., "Table"
and "Graph") and fall back to "Table" for any unknown/malformed value; implement
a small validator helper (or inline check) that reads params.get("view"),
returns the matching ConfigChangesView if it equals "Table" or "Graph",
otherwise returns "Table", and use that validated value everywhere instead of
the direct cast.
In `@src/components/ui/hover-card.tsx`:
- Around line 14-25: The current change wraps HoverCardPrimitive.Content inside
HoverCardPrimitive.Portal in src/components/ui/hover-card.tsx which modifies the
upstream shadcn base component; revert HoverCardPrimitive.Content to its
original form and instead create a project-specific wrapper (e.g.,
PortaledHoverCardContent) that composes HoverCardPrimitive.Portal and
HoverCardPrimitive.Content and accepts the same props (ref, align, sideOffset,
className, ...props); update any local usages to import and use
PortaledHoverCardContent while leaving the original HoverCardPrimitive.Content
export intact so the shadcn component file remains unmodified for future
upstream compatibility.
In `@src/pages/config/ConfigChangesPage.tsx`:
- Around line 82-84: The hook useGetAllConfigsChangesInfiniteQuery is always
instantiated causing an unnecessary 200-row fetch even when in Table view;
update this call site to pass the query options including enabled: isGraphView
(e.g., useGetAllConfigsChangesInfiniteQuery({ pageSize: parseInt(pageSize),
enabled: isGraphView })) so the hook only fetches when isGraphView is true;
locate the call in ConfigChangesPage where infiniteQuery is defined and add the
enabled flag using the existing isGraphView variable.
- Around line 95-101: The effect that clears tail state currently runs on mount
when liveTail === false and always calls tableQuery.refetch(); update the guard
to also check that the page is not in Graph mode (e.g., change the condition to
run only when !liveTail && !isGraphView) so Graph view won't trigger the
refetch; ensure the effect references isGraphView (add it to the dependency
array instead of keeping the eslint-disable) and keep the existing calls to
setTailedChanges, setTailCursor, and tableQuery.refetch inside the guarded
block.
In `@src/ui/Icons/Icon.tsx`:
- Around line 1007-1008: The fallback is returned raw, bypassing the normal
styling and prop forwarding used for Icon.SVG; update the unresolved-icon path
in the Icon component so that when Icon is falsy but fallback exists you clone
or render the fallback element with the same props contract (apply prefix,
className, and forward the component props) instead of returning it directly.
Locate the check around Icon and Icon.SVG and replace the direct return of
fallback with logic that clones the fallback React element (or wraps it) so it
receives the computed prefix, merged className and all forwarded props (same
behavior as the normal SVG rendering path, e.g., where Icon.SVG would be used),
ensuring ChangeIcon / FaDotCircle get the intended size/severity styling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b10a78b1-c3c3-4ba9-91fa-f18e7db681b2
📒 Files selected for processing (13)
src/api/query-hooks/useConfigChangesHooks.tssrc/components/Configs/Changes/ConfigChangesSwimlane.tsxsrc/components/Configs/Changes/ConfigChangesSwimlaneLegend.tsxsrc/components/Configs/Changes/ConfigChangesSwimlaneTooltip.tsxsrc/components/Configs/Changes/ConfigChangesSwimlaneUtils.tssrc/components/Configs/Changes/ConfigChangesViewToggle.tsxsrc/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsxsrc/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.tssrc/components/Configs/Changes/__tests__/changes.harsrc/components/ui/hover-card.tsxsrc/pages/config/ConfigChangesPage.tsxsrc/ui/Icons/ChangeIcon.tsxsrc/ui/Icons/Icon.tsx
adityathebe
force-pushed
the
feat/config-changes-swimlane
branch
from
94af248 to
d909ed2
Compare
adityathebe
force-pushed
the
feat/config-changes-swimlane
branch
from
d909ed2 to
ee28dbc
Compare
adityathebe
force-pushed
the
feat/config-changes-swimlane
branch
from
b3f97a9 to
af72693
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/components/Configs/Changes/ConfigChangesViewToggle.tsx (1)
6-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
viewinstead of casting raw query-string input.
params.get("view") as ConfigChangesViewstill accepts arbitrary strings (e.g.?view=foo), which leaks an invalid value into both the hook return andSwitch.value. A small type guard that falls back to"Table"resolves this.🔧 Suggested fix
export type ConfigChangesView = "Table" | "Graph"; +function isConfigChangesView(value: string | null): value is ConfigChangesView { + return value === "Table" || value === "Graph"; +} + export function useConfigChangesViewToggleState(): ConfigChangesView { const [params] = useSearchParams(); - return (params.get("view") as ConfigChangesView) || "Table"; + const view = params.get("view"); + return isConfigChangesView(view) ? view : "Table"; } export default function ConfigChangesViewToggle() { const [params, setParams] = useSearchParams(); - const value = (params.get("view") as ConfigChangesView) || "Table"; + const view = params.get("view"); + const value = isConfigChangesView(view) ? view : "Table";🤖 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 `@src/components/Configs/Changes/ConfigChangesViewToggle.tsx` around lines 6 - 13, The code casts the raw query param into ConfigChangesView, which can admit invalid values; update both useConfigChangesViewToggleState and ConfigChangesViewToggle to validate params.get("view") against the allowed ConfigChangesView union (e.g., check membership in a set/array of allowed values like "Table" and "Timeline") and return/fall back to "Table" when the value is not one of those; replace the two occurrences of (params.get("view") as ConfigChangesView) with a small type-guard/helper that returns the validated ConfigChangesView or "Table".src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts (1)
173-202:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStill leaking
mousemove/mouseuplisteners on unmount mid-drag.Listeners are only removed inside
onMouseUp. If the swimlane unmounts while the user is dragging, the handlers remain attached todocumentand continue to callsetWidthon the unmounted hook (until the nextmouseup). Capturing the active handlers in refs and tearing them down via auseEffectcleanup will close this gap.🔧 Suggested fix
const dragging = useRef(false); const userResized = useRef(false); const startX = useRef(0); const startWidth = useRef(0); + const moveHandler = useRef<((ev: MouseEvent) => void) | null>(null); + const upHandler = useRef<(() => void) | null>(null); useEffect(() => { if (!userResized.current) setWidth(initial); }, [initial]); + useEffect(() => { + return () => { + if (moveHandler.current) { + document.removeEventListener("mousemove", moveHandler.current); + } + if (upHandler.current) { + document.removeEventListener("mouseup", upHandler.current); + } + }; + }, []); + const onMouseDown = useCallback( (e: React.MouseEvent) => { e.preventDefault(); dragging.current = true; startX.current = e.clientX; startWidth.current = width; const onMouseMove = (ev: MouseEvent) => { /* ... */ }; const onMouseUp = () => { dragging.current = false; userResized.current = true; document.removeEventListener("mousemove", onMouseMove); document.removeEventListener("mouseup", onMouseUp); + moveHandler.current = null; + upHandler.current = null; }; + moveHandler.current = onMouseMove; + upHandler.current = onMouseUp; document.addEventListener("mousemove", onMouseMove); document.addEventListener("mouseup", onMouseUp); }, [width] );🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts` around lines 173 - 202, The onMouseDown logic attaches onMouseMove/onMouseUp directly to document but only removes them inside onMouseUp, leaking listeners if the component unmounts mid-drag; fix by storing the created handler functions (onMouseMove and onMouseUp) in refs (e.g., mouseMoveRef, mouseUpRef) so the exact same function references can be removed, and add a useEffect with a cleanup that checks refs and removes any attached document listeners and clears dragging.current (and preserves userResized.current) on unmount; update onMouseDown to assign those refs before calling document.addEventListener so the cleanup can reliably remove them.src/components/Configs/Changes/ConfigChangesSwimlaneTooltip.tsx (1)
126-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBroken
/catalog/undefinedlink when nested config is missing.When
change.config(or itsid) is absent, this still emits aLinkto/catalog/undefined. Guard the rendering so a non-link fallback is used in that case.🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlaneTooltip.tsx` around lines 126 - 136, The current rendering in ConfigChangesSwimlaneTooltip creates a Link to `/catalog/${change.config?.id}` even when change.config or change.config.id is missing, producing `/catalog/undefined`; update the conditional around the Link so it only renders when change.config?.id is truthy, and render a non-link fallback (e.g., the same ConfigsTypeIcon + span/truncated name or a placeholder text) when id is absent; modify the JSX that currently shows Link and ConfigsTypeIcon (the block referencing change.config, ConfigsTypeIcon and the to={`/catalog/${change.config?.id}`} prop) to first check change.config?.id and use the fallback UI otherwise.src/components/Configs/Changes/ConfigChangesSwimlane.tsx (2)
510-515:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGroup rows still keyed by display name, not stable id.
const key = c.config?.name ?? c.config_id ?? "unknown";will merge distinct configs that happen to share a name, silently corrupting row aggregation, severity totals, and click targets. Use the stable identifier (c.config?.id ?? c.config_id) for grouping/keying and keepnameonly for the displayed label. The downstreamkey={row.name}at Line 664 inherits the same collision risk.🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 510 - 515, The grouping currently uses display names which can conflate distinct configs; change the grouping key in the Map construction (where "const key = c.config?.name ?? c.config_id ?? 'unknown'") to use the stable id (c.config?.id ?? c.config_id) and reserve c.config?.name only for UI labels; also update the downstream React key usage (currently key={row.name}) to use the stable id (e.g., key={row.id} or the same id field used for grouping) so rows, aggregations, and click targets remain stable and unique.
79-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHover/click triggers remain non-focusable / unnamed.
Line 81 still wraps the severity icon in a
<span>used asHoverCardTrigger asChild, so keyboard users can't open the popover. The lane button on Line 254 also renders icon-only whenlabelPlacement === "none"and lacks anaria-label. Replace the span with a focusable element and give the lane button a stable accessible name.Also applies to: 253-262
🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 79 - 88, The HoverCard trigger currently uses a plain <span> (HoverCardTrigger asChild) which is not keyboard-focusable; replace that wrapper with a focusable element such as a <button> (or an element with tabIndex=0 and role="button") inside the ConfigChangesSwimlane so keyboard users can open the popover via HoverCardTrigger and the severity badge remains unchanged (use the same unique key and severity[key] logic). Also update the lane button rendering (where labelPlacement === "none") to include a stable accessible name via aria-label (e.g., derive from lane.name or a fallback like `aria-label={`Open ${laneName} lane`}`) so the icon-only button has a clear accessible label; make sure both the HoverCardTrigger/button and the lane button keep any existing onClick handlers and visual styling while adding the focusability and aria-label.
🧹 Nitpick comments (4)
src/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.ts (1)
1-249: 💤 Low valueSolid baseline coverage; consider extending to severity and placement utilities.
The tests exercise the core data-shaping path well. As a follow-up,
countSeverities,filterBySeverity,groupBucketByType, andcomputeLabelPlacementsare exported but not covered here — adding focused cases would harden the swimlane's icon placement and severity badge logic, which are core to the visualization.🤖 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 `@src/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.ts` around lines 1 - 249, Add unit tests covering the missing exported utilities: write focused tests for countSeverities (verify it tallies severity counts from a BucketedRow or array of changes), filterBySeverity (ensure it filters changes by requested severities and returns empty when none match), groupBucketByType (confirm buckets are grouped by change_type like "diff" vs "create" and that empty buckets are handled), and computeLabelPlacements (use deterministic buckets and timestamps to assert label x-positions and collision handling). Reuse the existing helpers makeChange and makeRow and the same min/max tick setup from ConfigChangesSwimlaneUtils.unit.test.ts to create predictable inputs and assert expected outputs for each function.src/api/query-hooks/useConfigChangesHooks.ts (1)
175-216: ⚡ Quick winExtract shared filter-prop derivation to avoid drift.
This block duplicates almost the entire body of
useGetAllConfigsChangesQuery(timeRange, search params, sort, tags, arbitraryFilter, severity/changeType/configType handling). Future filter additions or fixes will need to be applied in two places. Consider extracting a shareduseConfigChangesFilterProps(paramPrefix)hook returning the common filter object so both hooks stay in sync.🤖 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 `@src/api/query-hooks/useConfigChangesHooks.ts` around lines 175 - 216, The block in useGetAllConfigsChangesInfiniteQuery duplicates filter derivation from useGetAllConfigsChangesQuery; extract a new hook useConfigChangesFilterProps(paramPrefix, overrides?) that composes showChangesFromDeletedConfigs (useShowDeletedConfigs), timeRangeValue (useTimeRangeParams with configChangesDefaultDateFilter), prefixed params (usePrefixedSearchParams), sort state (useReactTableSortState), tags (useConfigChangesTagsFilter) and arbitraryFilter (useConfigChangesArbitraryFilters) and returns the unified filter object (include_deleted_configs, changeType, severity, from, to, configTypes, configType, sortBy, sortOrder, arbitraryFilter, tags) so both useGetAllConfigsChangesInfiniteQuery and useGetAllConfigsChangesQuery consume that hook; preserve pageSize, paramPrefix and enabled handling and ensure sort mapping (sortBy[0]?.id and desc -> "desc"|"asc") remains identical.src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts (1)
366-383: 💤 Low valueReplace
iconPositions.indexOf(pos)with the loop index.
labelCandidates.indexOf(iconPositions.indexOf(pos))does two linear scans per iteration, making this O(n²) in the number of grouped change types per bucket. Using the loop index directly is both clearer and linear.- for (const pos of iconPositions) { - const candidateIdx = labelCandidates.indexOf(iconPositions.indexOf(pos)); + for (let posIdx = 0; posIdx < iconPositions.length; posIdx++) { + const pos = iconPositions[posIdx]!; + const candidateIdx = labelCandidates.indexOf(posIdx); let placement: LabelPlacement = "none";🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts` around lines 366 - 383, The loop over iconPositions uses labelCandidates.indexOf(iconPositions.indexOf(pos)) causing two nested scans; replace the inner iconPositions.indexOf(pos) with the current loop index (e.g., use a for loop or forEach with an index) so candidateIdx = labelCandidates.indexOf(i) where i is the iteration index; update the block that computes candidateIdx/placement (symbols: iconPositions, labelCandidates, candidateIdx, placement, pattern) and leave the result.set(...) logic (symbols: result.set, bIdx, pos.gIdx) unchanged.src/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsx (1)
51-114: ⚡ Quick winTests are tightly coupled to Tailwind class selectors and a magic count.
Multiple assertions look up rows/buttons via concrete utility classes (
.flex.h-full.w-full,.flex.flex-row.border-b,span.text-xs.text-gray-400,span.truncate.font-medium). Any styling refactor will silently break these tests without changing user-observable behavior. Likewise,expect(expectedTypes.size).toBe(34)on Line 58 is tied to the exact contents of the HAR fixture and will become a maintenance hazard if it's ever refreshed. Consider exposing semantic hooks (e.g.data-testid="swimlane",data-testid="group-row",role/aria-labelon the group button) and asserting on those instead, and assertingexpectedTypes.size > 0or against a derived expectation rather than a hard-coded number.🤖 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 `@src/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsx` around lines 51 - 114, Tests are brittle because they query DOM by Tailwind classes and assert a magic fixture size; update the component to expose semantic hooks and change assertions to be resilient: add data-testid attributes (e.g. data-testid="swimlane", data-testid="group-row", data-testid="group-button", data-testid="group-count", data-testid="group-name") or ARIA labels on the group button so tests can use screen.getByTestId / getAllByTestId or getByRole+name instead of container.querySelector(".flex...") and span.class selectors, and replace the hard-coded expectedTypes.size === 34 check in the test using allChanges with a looser assertion (e.g. expect(expectedTypes.size).toBeGreaterThan(0) or assert it matches a derived value) while updating references in this test file to use renderSwimlane, allChanges, and the new testids/roles.
🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts`:
- Around line 322-326: Remove the unused variable bucketWidthPct to satisfy
ESLint: locate where bucketWidthPct is declared alongside result, range,
bucketSpan and numBuckets (the const bucketWidthPct = 100 / numBuckets line) and
delete that declaration; leave result, range and bucketSpan as-is so behavior is
unchanged.
In `@src/components/ui/portaled-hover-card.tsx`:
- Around line 6-22: Replace the duplicated styling in PortaledHoverCardContent
by composing the existing HoverCardContent: import HoverCardContent from
./hover-card, change PortaledHoverCardContent to forwardRef using
React.ElementRef<typeof HoverCardContent> and
React.ComponentPropsWithoutRef<typeof HoverCardContent>, render
<HoverCardPrimitive.Portal><HoverCardContent ref={ref} {...props}
/></HoverCardPrimitive.Portal> instead of re-declaring className/styles, and set
PortaledHoverCardContent.displayName = "PortaledHoverCardContent".
In `@src/pages/config/ConfigChangesPage.tsx`:
- Around line 98-113: The current useEffect in ConfigChangesPage triggers
infiniteQuery.fetchNextPage repeatedly whenever hasNextPage becomes true,
causing eager bulk-loading; change the behavior so fetchNextPage is no longer
auto-triggered on state changes: remove or short-circuit this useEffect and
instead initiate pagination from an IntersectionObserver sentinel rendered
inside ConfigChangesSwimlane (observe a sentinel element and call
infiniteQuery.fetchNextPage when it enters view), or implement a bounded
safeguard in the same useEffect (e.g., track a maxPages or maxRows counter tied
to infiniteQuery.fetchNextPage calls and stop auto-fetching once the cap is
reached) so opening Graph view does not recursively pull every page.
- Around line 51-61: normalizeChange currently overwrites any existing nested
config and uses non-null assertions; change it to preserve c.config (if present)
and only fill or override missing fields from top-level properties without using
"!" assertions. Specifically, in normalizeChange merge as config: { ...(c.config
|| {}), id: c.config?.id ?? c.config_id ?? null, type: c.config?.type ?? c.type
?? null, name: c.config?.name ?? c.name ?? null, deleted_at:
c.config?.deleted_at ?? c.deleted_at ?? null } so fields like agent, tags, and
icon metadata are kept and top-level values are used as fallbacks without unsafe
non-null assertions.
---
Duplicate comments:
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx`:
- Around line 510-515: The grouping currently uses display names which can
conflate distinct configs; change the grouping key in the Map construction
(where "const key = c.config?.name ?? c.config_id ?? 'unknown'") to use the
stable id (c.config?.id ?? c.config_id) and reserve c.config?.name only for UI
labels; also update the downstream React key usage (currently key={row.name}) to
use the stable id (e.g., key={row.id} or the same id field used for grouping) so
rows, aggregations, and click targets remain stable and unique.
- Around line 79-88: The HoverCard trigger currently uses a plain <span>
(HoverCardTrigger asChild) which is not keyboard-focusable; replace that wrapper
with a focusable element such as a <button> (or an element with tabIndex=0 and
role="button") inside the ConfigChangesSwimlane so keyboard users can open the
popover via HoverCardTrigger and the severity badge remains unchanged (use the
same unique key and severity[key] logic). Also update the lane button rendering
(where labelPlacement === "none") to include a stable accessible name via
aria-label (e.g., derive from lane.name or a fallback like `aria-label={`Open
${laneName} lane`}`) so the icon-only button has a clear accessible label; make
sure both the HoverCardTrigger/button and the lane button keep any existing
onClick handlers and visual styling while adding the focusability and
aria-label.
In `@src/components/Configs/Changes/ConfigChangesSwimlaneTooltip.tsx`:
- Around line 126-136: The current rendering in ConfigChangesSwimlaneTooltip
creates a Link to `/catalog/${change.config?.id}` even when change.config or
change.config.id is missing, producing `/catalog/undefined`; update the
conditional around the Link so it only renders when change.config?.id is truthy,
and render a non-link fallback (e.g., the same ConfigsTypeIcon + span/truncated
name or a placeholder text) when id is absent; modify the JSX that currently
shows Link and ConfigsTypeIcon (the block referencing change.config,
ConfigsTypeIcon and the to={`/catalog/${change.config?.id}`} prop) to first
check change.config?.id and use the fallback UI otherwise.
In `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts`:
- Around line 173-202: The onMouseDown logic attaches onMouseMove/onMouseUp
directly to document but only removes them inside onMouseUp, leaking listeners
if the component unmounts mid-drag; fix by storing the created handler functions
(onMouseMove and onMouseUp) in refs (e.g., mouseMoveRef, mouseUpRef) so the
exact same function references can be removed, and add a useEffect with a
cleanup that checks refs and removes any attached document listeners and clears
dragging.current (and preserves userResized.current) on unmount; update
onMouseDown to assign those refs before calling document.addEventListener so the
cleanup can reliably remove them.
In `@src/components/Configs/Changes/ConfigChangesViewToggle.tsx`:
- Around line 6-13: The code casts the raw query param into ConfigChangesView,
which can admit invalid values; update both useConfigChangesViewToggleState and
ConfigChangesViewToggle to validate params.get("view") against the allowed
ConfigChangesView union (e.g., check membership in a set/array of allowed values
like "Table" and "Timeline") and return/fall back to "Table" when the value is
not one of those; replace the two occurrences of (params.get("view") as
ConfigChangesView) with a small type-guard/helper that returns the validated
ConfigChangesView or "Table".
---
Nitpick comments:
In `@src/api/query-hooks/useConfigChangesHooks.ts`:
- Around line 175-216: The block in useGetAllConfigsChangesInfiniteQuery
duplicates filter derivation from useGetAllConfigsChangesQuery; extract a new
hook useConfigChangesFilterProps(paramPrefix, overrides?) that composes
showChangesFromDeletedConfigs (useShowDeletedConfigs), timeRangeValue
(useTimeRangeParams with configChangesDefaultDateFilter), prefixed params
(usePrefixedSearchParams), sort state (useReactTableSortState), tags
(useConfigChangesTagsFilter) and arbitraryFilter
(useConfigChangesArbitraryFilters) and returns the unified filter object
(include_deleted_configs, changeType, severity, from, to, configTypes,
configType, sortBy, sortOrder, arbitraryFilter, tags) so both
useGetAllConfigsChangesInfiniteQuery and useGetAllConfigsChangesQuery consume
that hook; preserve pageSize, paramPrefix and enabled handling and ensure sort
mapping (sortBy[0]?.id and desc -> "desc"|"asc") remains identical.
In
`@src/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsx`:
- Around line 51-114: Tests are brittle because they query DOM by Tailwind
classes and assert a magic fixture size; update the component to expose semantic
hooks and change assertions to be resilient: add data-testid attributes (e.g.
data-testid="swimlane", data-testid="group-row", data-testid="group-button",
data-testid="group-count", data-testid="group-name") or ARIA labels on the group
button so tests can use screen.getByTestId / getAllByTestId or getByRole+name
instead of container.querySelector(".flex...") and span.class selectors, and
replace the hard-coded expectedTypes.size === 34 check in the test using
allChanges with a looser assertion (e.g.
expect(expectedTypes.size).toBeGreaterThan(0) or assert it matches a derived
value) while updating references in this test file to use renderSwimlane,
allChanges, and the new testids/roles.
In
`@src/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.ts`:
- Around line 1-249: Add unit tests covering the missing exported utilities:
write focused tests for countSeverities (verify it tallies severity counts from
a BucketedRow or array of changes), filterBySeverity (ensure it filters changes
by requested severities and returns empty when none match), groupBucketByType
(confirm buckets are grouped by change_type like "diff" vs "create" and that
empty buckets are handled), and computeLabelPlacements (use deterministic
buckets and timestamps to assert label x-positions and collision handling).
Reuse the existing helpers makeChange and makeRow and the same min/max tick
setup from ConfigChangesSwimlaneUtils.unit.test.ts to create predictable inputs
and assert expected outputs for each function.
In `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts`:
- Around line 366-383: The loop over iconPositions uses
labelCandidates.indexOf(iconPositions.indexOf(pos)) causing two nested scans;
replace the inner iconPositions.indexOf(pos) with the current loop index (e.g.,
use a for loop or forEach with an index) so candidateIdx =
labelCandidates.indexOf(i) where i is the iteration index; update the block that
computes candidateIdx/placement (symbols: iconPositions, labelCandidates,
candidateIdx, placement, pattern) and leave the result.set(...) logic (symbols:
result.set, bIdx, pos.gIdx) 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da95a909-b16d-429f-8ba6-4d85ae8dd40c
📒 Files selected for processing (14)
src/api/query-hooks/useConfigChangesHooks.tssrc/components/Configs/Changes/ConfigChangesSwimlane.tsxsrc/components/Configs/Changes/ConfigChangesSwimlaneLegend.tsxsrc/components/Configs/Changes/ConfigChangesSwimlaneTooltip.tsxsrc/components/Configs/Changes/ConfigChangesSwimlaneUtils.tssrc/components/Configs/Changes/ConfigChangesViewToggle.tsxsrc/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsxsrc/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.tssrc/components/Configs/Changes/__tests__/changes.harsrc/components/ui/portaled-hover-card.tsxsrc/pages/config/ConfigChangesPage.tsxsrc/ui/DataTable/FilterByCellValue.tsxsrc/ui/Icons/ChangeIcon.tsxsrc/ui/Icons/Icon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/Icons/ChangeIcon.tsx
Comment on lines
+6
to
+22
| const PortaledHoverCardContent = React.forwardRef< | ||
| React.ElementRef<typeof HoverCardPrimitive.Content>, | ||
| React.ComponentPropsWithoutRef<typeof HoverCardPrimitive.Content> | ||
| >(({ className, align = "center", sideOffset = 4, ...props }, ref) => ( | ||
| <HoverCardPrimitive.Portal> | ||
| <HoverCardPrimitive.Content | ||
| ref={ref} | ||
| align={align} | ||
| sideOffset={sideOffset} | ||
| className={cn( | ||
| "z-50 w-64 origin-[--radix-hover-card-content-transform-origin] rounded-md border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", | ||
| className | ||
| )} | ||
| {...props} | ||
| /> | ||
| </HoverCardPrimitive.Portal> | ||
| )); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate any existing shadcn hover-card to confirm whether HoverCardContent is already defined and whether it portals.
fd -t f 'hover-card' src/components/ui
rg -nP -C3 '\bHoverCardContent\b|HoverCardPrimitive\.(Content|Portal)'Repository: flanksource/flanksource-ui
Length of output: 8718
🏁 Script executed:
cat -n src/components/ui/hover-card.tsxRepository: flanksource/flanksource-ui
Length of output: 1518
🏁 Script executed:
cat -n src/components/ui/portaled-hover-card.tsxRepository: flanksource/flanksource-ui
Length of output: 1482
Compose the existing HoverCardContent instead of duplicating its styles.
The className here is identical to the one in hover-card.tsx. Rather than hand-writing shadcn's styles, wrap the existing HoverCardContent component inside HoverCardPrimitive.Portal:
Suggested approach
const PortaledHoverCardContent = React.forwardRef<
React.ElementRef<typeof HoverCardContent>,
React.ComponentPropsWithoutRef<typeof HoverCardContent>
>((props, ref) => (
<HoverCardPrimitive.Portal>
<HoverCardContent ref={ref} {...props} />
</HoverCardPrimitive.Portal>
));
PortaledHoverCardContent.displayName = "PortaledHoverCardContent";First, import HoverCardContent from ./hover-card.
This keeps styling in one place and ensures future shadcn upgrades propagate automatically. Per coding guidelines, do not hand-write shadcn components—compose existing ones instead.
🤖 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 `@src/components/ui/portaled-hover-card.tsx` around lines 6 - 22, Replace the
duplicated styling in PortaledHoverCardContent by composing the existing
HoverCardContent: import HoverCardContent from ./hover-card, change
PortaledHoverCardContent to forwardRef using React.ElementRef<typeof
HoverCardContent> and React.ComponentPropsWithoutRef<typeof HoverCardContent>,
render <HoverCardPrimitive.Portal><HoverCardContent ref={ref} {...props}
/></HoverCardPrimitive.Portal> instead of re-declaring className/styles, and set
PortaledHoverCardContent.displayName = "PortaledHoverCardContent".
Comment on lines
+51
to
+61
| function normalizeChange(c: ConfigChange): ConfigChange { | ||
| return { | ||
| ...c, | ||
| config: { | ||
| id: c.config_id!, | ||
| type: c.type!, | ||
| name: c.name!, | ||
| deleted_at: c.deleted_at | ||
| } | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
normalizeChange clobbers an existing config and uses unsafe non-null assertions.
Two issues here:
- The spread
...cfollowed byconfig: {...}unconditionally replaces any nestedconfigalready returned by the API with a four-field reconstruction, dropping fields likeagent,tags, icon metadata, etc. that downstream components (e.g.ConfigsTypeIcon) rely on. c.type!andc.name!assert non-null on optional top-level fields. If the backend ever omits them (which is the whole reason these are top-level and nested), you'll silently produce{ type: undefined, name: undefined }and surface "undefined" in the UI / break grouping.
Suggested fix
function normalizeChange(c: ConfigChange): ConfigChange {
- return {
- ...c,
- config: {
- id: c.config_id!,
- type: c.type!,
- name: c.name!,
- deleted_at: c.deleted_at
- }
- };
+ if (c.config?.id) return c;
+ return {
+ ...c,
+ config: {
+ id: c.config_id,
+ type: c.type,
+ name: c.name,
+ deleted_at: c.deleted_at,
+ ...c.config
+ }
+ };
}📝 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
| function normalizeChange(c: ConfigChange): ConfigChange { | |
| return { | |
| ...c, | |
| config: { | |
| id: c.config_id!, | |
| type: c.type!, | |
| name: c.name!, | |
| deleted_at: c.deleted_at | |
| } | |
| }; | |
| } | |
| function normalizeChange(c: ConfigChange): ConfigChange { | |
| if (c.config?.id) return c; | |
| return { | |
| ...c, | |
| config: { | |
| id: c.config_id, | |
| type: c.type, | |
| name: c.name, | |
| deleted_at: c.deleted_at, | |
| ...c.config | |
| } | |
| }; | |
| } |
🤖 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 `@src/pages/config/ConfigChangesPage.tsx` around lines 51 - 61, normalizeChange
currently overwrites any existing nested config and uses non-null assertions;
change it to preserve c.config (if present) and only fill or override missing
fields from top-level properties without using "!" assertions. Specifically, in
normalizeChange merge as config: { ...(c.config || {}), id: c.config?.id ??
c.config_id ?? null, type: c.config?.type ?? c.type ?? null, name:
c.config?.name ?? c.name ?? null, deleted_at: c.config?.deleted_at ??
c.deleted_at ?? null } so fields like agent, tags, and icon metadata are kept
and top-level values are used as fallbacks without unsafe non-null assertions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/components/Configs/Changes/Swimlane/Utils.ts (1)
179-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrag event listeners leak on unmount — previously flagged, still unresolved.
onMouseDownaddsmousemove/mouseuphandlers todocumentbut they are only removed insideonMouseUp. If the component unmounts while a drag is in progress, both handlers remain attached indefinitely and will keep callingsetWidthon an unmounted component (and hold a closure reference to the staleonMouseMove/onMouseUpfunctions).🛠️ Proposed fix — add unmount cleanup via refs
export function useResizableColumn(initial: number) { const [width, setWidth] = useState(initial); const dragging = useRef(false); const userResized = useRef(false); const startX = useRef(0); const startWidth = useRef(0); + const handlersRef = useRef<{ + onMouseMove: (ev: MouseEvent) => void; + onMouseUp: () => void; + } | null>(null); useEffect(() => { if (!userResized.current) setWidth(initial); }, [initial]); + useEffect(() => { + return () => { + if (handlersRef.current) { + document.removeEventListener("mousemove", handlersRef.current.onMouseMove); + document.removeEventListener("mouseup", handlersRef.current.onMouseUp); + handlersRef.current = null; + } + }; + }, []); const onMouseDown = useCallback( (e: React.MouseEvent) => { e.preventDefault(); dragging.current = true; startX.current = e.clientX; startWidth.current = width; const onMouseMove = (ev: MouseEvent) => { if (!dragging.current) return; const delta = ev.clientX - startX.current; setWidth( Math.min( MAX_COLUMN_WIDTH, Math.max(MIN_COLUMN_WIDTH, startWidth.current + delta) ) ); }; const onMouseUp = () => { dragging.current = false; userResized.current = true; document.removeEventListener("mousemove", onMouseMove); document.removeEventListener("mouseup", onMouseUp); + handlersRef.current = null; }; + handlersRef.current = { onMouseMove, onMouseUp }; document.addEventListener("mousemove", onMouseMove); document.addEventListener("mouseup", onMouseUp); }, [width] ); return { width, onMouseDown }; }🤖 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 `@src/components/Configs/Changes/Swimlane/Utils.ts` around lines 179 - 208, onMouseDown currently installs document "mousemove" and "mouseup" handlers but only removes them inside onMouseUp, so if the component unmounts during a drag the listeners leak. Fix by ensuring those handlers are always removed on unmount: store the created onMouseMove and onMouseUp functions in refs (or module-scope refs) so the same function references can be removed, and add a useEffect cleanup that calls document.removeEventListener("mousemove", moveRef.current) and document.removeEventListener("mouseup", upRef.current) and resets dragging.current to false; alternatively ensure the effect that declares onMouseDown returns a cleanup that removes the listeners. Update references to onMouseMove/onMouseUp (and dragging/userResized/startX/startWidth refs) so the cleanup removes the exact functions added.src/components/Configs/Changes/Swimlane/Tooltip.tsx (1)
29-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDerive tooltip config identity from the same fallback key and guard missing links.
allSameConfigonly comparesconfig_id, while the swimlane groups rows byconfig_id ?? config?.id ?? idinsrc/components/Configs/Changes/ConfigChangesSwimlane.tsxLine 55. Whenconfig_idis absent, a mixed-config bucket is treated as single-config here, and Line 42 can still generate/catalog/undefined. Use the same fallback chain for the comparison and only render aLinkwhenchange.config?.idexists.Suggested fix
export function GroupedSwimlaneTooltip({ group, onExpand }: { group: GroupedChange; onExpand?: (change: ConfigChange) => void; }) { const { count, changes } = group; - const allSameConfig = changes.every( - (c) => c.config_id === changes[0]?.config_id - ); + const getConfigKey = (change: ConfigChange) => + change.config_id ?? change.config?.id ?? change.id; + const allSameConfig = changes.every( + (c) => getConfigKey(c) === getConfigKey(changes[0]!) + ); return ( <div className="flex flex-col gap-1.5 rounded-lg bg-gray-100 p-2 text-black shadow-sm"> <div className="flex flex-col gap-2 divide-y divide-gray-300"> {changes.slice(0, 5).map((change, i) => ( <div key={change.id} className="flex flex-col gap-1 pt-1.5 first:pt-0" > - {!allSameConfig && ( - <Link - to={`/catalog/${change.config?.id}`} - className="flex items-center gap-1 text-xs hover:underline" - onClick={(e) => e.stopPropagation()} - > - <ConfigsTypeIcon config={change.config}> - <span className="truncate">{change.config?.name}</span> - </ConfigsTypeIcon> - </Link> - )} + {!allSameConfig && + (change.config?.id ? ( + <Link + to={`/catalog/${change.config.id}`} + className="flex items-center gap-1 text-xs hover:underline" + onClick={(e) => e.stopPropagation()} + > + <ConfigsTypeIcon config={change.config}> + <span className="truncate"> + {change.config?.name ?? "Unnamed"} + </span> + </ConfigsTypeIcon> + </Link> + ) : ( + <div className="flex items-center gap-1 text-xs"> + <ConfigsTypeIcon config={change.config}> + <span className="truncate"> + {change.config?.name ?? "Unnamed"} + </span> + </ConfigsTypeIcon> + </div> + ))} <ChangeRow change={change} onExpand={onExpand ? () => onExpand(change) : undefined} showType={i === 0} />🤖 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 `@src/components/Configs/Changes/Swimlane/Tooltip.tsx` around lines 29 - 49, allSameConfig currently compares only c.config_id which can mismatch the swimlane grouping that uses the fallback key config_id ?? config?.id ?? id; update the allSameConfig computation to use the same fallback chain (e.g., derive a key like const key = c.config_id ?? c.config?.id ?? c.id) so buckets are compared consistently, and guard rendering of the <Link> inside the map by only rendering it when change.config?.id is truthy to avoid generating /catalog/undefined; adjust references in this file to use the same fallback key and the conditional around the Link that contains ConfigsTypeIcon.src/components/Configs/Changes/Swimlane/GroupParentRow.tsx (1)
77-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the existing button as the hover-card trigger.
Line 77 already gives you a focusable control, but Line 84 attaches
HoverCardTriggerto a nestedspan. That leaves the group-type tooltip pointer-only; keyboard focus lands on the button, not the trigger.🤖 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 `@src/components/Configs/Changes/Swimlane/GroupParentRow.tsx` around lines 77 - 86, The HoverCardTrigger is attached to the nested span so keyboard focus lands on the outer button (in GroupParentRow) rather than the real trigger; instead wrap or replace the button element with HoverCardTrigger (use HoverCardTrigger asChild on the existing button) so the button itself becomes the trigger, keeping onClick/onToggle, Chevron, className, style and the truncated group.prefix inside the button; ensure HoverCard still uses the same openDelay/closeDelay and that the button remains accessible/focusable.
🧹 Nitpick comments (4)
src/components/Configs/Changes/Swimlane/Utils.ts (2)
257-261: 💤 Low value
nearestPresentParentPathloop bound is off-by-one for single-token paths.For a path with a single token (e.g.,
"foo"),tokens.lengthis 1, so the loop runs forendfrom0down — i.e., it never executes, which is correct (no parent). But the loop starts attokens.length - 1 = 0, and the conditionend > 0immediately fails. This is safe, but the name "nearest" implies it walks up; a brief comment noting thatend = 0is deliberately excluded (as it would produce an empty string) would help readers.🤖 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 `@src/components/Configs/Changes/Swimlane/Utils.ts` around lines 257 - 261, In nearestPresentParentPath, the for-loop using tokens, PATH_DELIMITER and presentPaths intentionally starts at tokens.length - 1 and uses the condition end > 0 to avoid producing an empty parent path for single-token inputs; add a concise comment directly above the loop explaining that end = 0 is deliberately excluded because slicing to 0 would produce an empty string and therefore no valid parent, so the loop intentionally skips that case (no functional change required).
380-403: 💤 Low value
computeLabelPlacements: loop at line 380–384 doesn't assign labels — the label pattern is applied in the wrong loop.The two loops over
labelCandidatesare logically split:
- Lines 380–384 — updates
seenTypes/deferredTypes(side-effects) but never assigns aLabelPlacementto any icon.- Lines 386–403 — iterates
iconPositionsand computescandidateIdxvia the double-indexOfchain. For icons that are not inlabelCandidates,candidateIdxwill be-1(becauselabelCandidates.indexOf(iconPositions.indexOf(pos))searches for the gIdx among the candidates), soplacementstays"none". For icons that are candidates but whose index inlabelCandidatesexceedspattern.length - 1, they get"extra". This looks structurally correct.However, the
PLACEMENT_PATTERNSarray only covers up to 4 candidates (indices 0–3). WhenlabelCandidates.length > 4,labelCountis capped at 4, butlabelCandidatesitself is not trimmed, so candidates at index ≥ 4 will hitcandidateIdx >= pattern.lengthand receive"extra". This is intentional per the design — but worth a brief comment in the code to make the intent clear.🤖 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 `@src/components/Configs/Changes/Swimlane/Utils.ts` around lines 380 - 403, computeLabelPlacements currently updates seenTypes/deferredTypes in the first loop but applies label placements in the second loop using a fragile double-indexOf lookup; trim labelCandidates to the effective labelCount (derived from PLACEMENT_PATTERNS and labelCount) before both loops so the candidate index aligns with pattern positions, then compute candidateIdx by searching iconPositions for pos.gIdx in that trimmed labelCandidates (or build a Map<gIdx, candidateIndex> once) and use that to assign placement from pattern (and "extra" when appropriate); also add a short comment next to PLACEMENT_PATTERNS/labelCount explaining that candidates beyond the pattern length intentionally become "extra".src/components/Configs/Changes/Swimlane/ChangeIconWithBadge.tsx (2)
2-7: ⚡ Quick winImport
GroupedChangedirectly instead of deriving the type viaReturnType<typeof groupBucketByType>[number].The
GroupedChangetype is already exported fromUtils.ts. UsingReturnType<typeof groupBucketByType>[number]imports a runtime function purely for type inference, coupling the prop API to an implementation detail.♻️ Proposed refactor
-import { groupBucketByType } from "./Utils"; +import { GroupedChange } from "./Utils"; export function ChangeIconWithBadge({ group }: { - group: ReturnType<typeof groupBucketByType>[number]; + group: GroupedChange; }) {🤖 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 `@src/components/Configs/Changes/Swimlane/ChangeIconWithBadge.tsx` around lines 2 - 7, The prop type for ChangeIconWithBadge currently derives from the runtime function groupBucketByType (ReturnType<typeof groupBucketByType>[number]); instead import the exported type GroupedChange from "./Utils" and use it as the prop type (i.e. group: GroupedChange) to avoid coupling the component's API to an implementation detail; update the import statement to include GroupedChange and remove the type-deriving ReturnType usage (and if groupBucketByType was only imported for this type, remove that import).
16-18: ⚡ Quick winBadge count has no display cap — large counts will overflow the 12px container.
group.countis rendered raw. When it reaches 3+ digits (e.g., 100), themin-w-3(12px) badge will overflow its container. Consider capping at"99+".🛠️ Proposed fix
- {group.count} + {group.count > 99 ? "99+" : group.count}🤖 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 `@src/components/Configs/Changes/Swimlane/ChangeIconWithBadge.tsx` around lines 16 - 18, The badge in ChangeIconWithBadge renders group.count raw and uses a fixed small width (the span with className containing min-w-3), causing overflow for 3+ digit counts; update the rendering to cap the displayed value (e.g., displayCount = group.count > 99 ? "99+" : String(group.count)) and use that displayCount in place of group.count, and adjust the badge styling on the same span (remove the strict min-w-3 and allow an auto/min-width or slightly larger padding like px-1.5) so the "99+" label fits without overflow.
🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx`:
- Around line 85-90: The current lane-builder always uses items[0] to derive
config, parentPath, path and name which breaks when the first ConfigChange is
sparse; instead locate the first populated change in items that has metadata
(e.g. config?.path, config?.name, path or name) and use that item's config to
compute parentPath and name; keep configId for the final path suffix (path =
parentPath ? `${parentPath}.${configId}` : configId) and fall back to the first
item's raw values only if no populated item is found. Ensure references: items,
config, parentPath, path, name, ConfigChange, and configId are updated
accordingly.
In `@src/components/Configs/Changes/Swimlane/IconWithLabel.tsx`:
- Around line 31-51: The button in IconWithLabel.tsx currently always calls
onItemClicked(group.representative) when labelPlacement === "extra", which
wrongly opens only the representative for multi-change groups; update the click
behavior so when group.count > 1 it either delegates to the grouped UI (e.g.,
trigger GroupedSwimlaneTooltip or call a new handler like onGroupClicked) or
disables the click to avoid silently dropping items, otherwise keep the existing
onItemClicked(group.representative) behavior; locate the button using ExtraDot,
group.representative, and onItemClicked to implement the conditional click
routing.
In `@src/components/Configs/Changes/Swimlane/SeverityBadges.tsx`:
- Around line 1-11: Import the React namespace so the React.ReactNode type
resolves: add an import for React (e.g., import React from "react") at the top
of the file where severityEntries is declared so the type annotation
React.ReactNode used in the severityEntries definition compiles correctly;
alternatively you can import the type directly (import type { ReactNode } from
"react") and update the declaration to use ReactNode.
In `@src/components/Configs/Changes/Swimlane/TimeAxisHeader.tsx`:
- Around line 1-3: The file references React.MutableRefObject in the
TimeAxisHeader props but never imports React, causing the type to be unresolved;
fix by adding an import for the React namespace (or a type-only import like
`import type React from "react"`) at the top of
src/components/Configs/Changes/Swimlane/TimeAxisHeader.tsx so the
React.MutableRefObject type used in the component prop definition (the prop line
that currently mentions React.MutableRefObject) resolves correctly.
---
Duplicate comments:
In `@src/components/Configs/Changes/Swimlane/GroupParentRow.tsx`:
- Around line 77-86: The HoverCardTrigger is attached to the nested span so
keyboard focus lands on the outer button (in GroupParentRow) rather than the
real trigger; instead wrap or replace the button element with HoverCardTrigger
(use HoverCardTrigger asChild on the existing button) so the button itself
becomes the trigger, keeping onClick/onToggle, Chevron, className, style and the
truncated group.prefix inside the button; ensure HoverCard still uses the same
openDelay/closeDelay and that the button remains accessible/focusable.
In `@src/components/Configs/Changes/Swimlane/Tooltip.tsx`:
- Around line 29-49: allSameConfig currently compares only c.config_id which can
mismatch the swimlane grouping that uses the fallback key config_id ??
config?.id ?? id; update the allSameConfig computation to use the same fallback
chain (e.g., derive a key like const key = c.config_id ?? c.config?.id ?? c.id)
so buckets are compared consistently, and guard rendering of the <Link> inside
the map by only rendering it when change.config?.id is truthy to avoid
generating /catalog/undefined; adjust references in this file to use the same
fallback key and the conditional around the Link that contains ConfigsTypeIcon.
In `@src/components/Configs/Changes/Swimlane/Utils.ts`:
- Around line 179-208: onMouseDown currently installs document "mousemove" and
"mouseup" handlers but only removes them inside onMouseUp, so if the component
unmounts during a drag the listeners leak. Fix by ensuring those handlers are
always removed on unmount: store the created onMouseMove and onMouseUp functions
in refs (or module-scope refs) so the same function references can be removed,
and add a useEffect cleanup that calls document.removeEventListener("mousemove",
moveRef.current) and document.removeEventListener("mouseup", upRef.current) and
resets dragging.current to false; alternatively ensure the effect that declares
onMouseDown returns a cleanup that removes the listeners. Update references to
onMouseMove/onMouseUp (and dragging/userResized/startX/startWidth refs) so the
cleanup removes the exact functions added.
---
Nitpick comments:
In `@src/components/Configs/Changes/Swimlane/ChangeIconWithBadge.tsx`:
- Around line 2-7: The prop type for ChangeIconWithBadge currently derives from
the runtime function groupBucketByType (ReturnType<typeof
groupBucketByType>[number]); instead import the exported type GroupedChange from
"./Utils" and use it as the prop type (i.e. group: GroupedChange) to avoid
coupling the component's API to an implementation detail; update the import
statement to include GroupedChange and remove the type-deriving ReturnType usage
(and if groupBucketByType was only imported for this type, remove that import).
- Around line 16-18: The badge in ChangeIconWithBadge renders group.count raw
and uses a fixed small width (the span with className containing min-w-3),
causing overflow for 3+ digit counts; update the rendering to cap the displayed
value (e.g., displayCount = group.count > 99 ? "99+" : String(group.count)) and
use that displayCount in place of group.count, and adjust the badge styling on
the same span (remove the strict min-w-3 and allow an auto/min-width or slightly
larger padding like px-1.5) so the "99+" label fits without overflow.
In `@src/components/Configs/Changes/Swimlane/Utils.ts`:
- Around line 257-261: In nearestPresentParentPath, the for-loop using tokens,
PATH_DELIMITER and presentPaths intentionally starts at tokens.length - 1 and
uses the condition end > 0 to avoid producing an empty parent path for
single-token inputs; add a concise comment directly above the loop explaining
that end = 0 is deliberately excluded because slicing to 0 would produce an
empty string and therefore no valid parent, so the loop intentionally skips that
case (no functional change required).
- Around line 380-403: computeLabelPlacements currently updates
seenTypes/deferredTypes in the first loop but applies label placements in the
second loop using a fragile double-indexOf lookup; trim labelCandidates to the
effective labelCount (derived from PLACEMENT_PATTERNS and labelCount) before
both loops so the candidate index aligns with pattern positions, then compute
candidateIdx by searching iconPositions for pos.gIdx in that trimmed
labelCandidates (or build a Map<gIdx, candidateIndex> once) and use that to
assign placement from pattern (and "extra" when appropriate); also add a short
comment next to PLACEMENT_PATTERNS/labelCount explaining that candidates beyond
the pattern length intentionally become "extra".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a6f9b55-4648-4e42-a03f-8bab278d97c6
📒 Files selected for processing (20)
src/api/types/configs.tssrc/components/Configs/Changes/ConfigChangesSwimlane.tsxsrc/components/Configs/Changes/Swimlane/BucketCells.tsxsrc/components/Configs/Changes/Swimlane/ChangeIconWithBadge.tsxsrc/components/Configs/Changes/Swimlane/ChangeRow.tsxsrc/components/Configs/Changes/Swimlane/EmptySwimlaneState.tsxsrc/components/Configs/Changes/Swimlane/FlexLabel.tsxsrc/components/Configs/Changes/Swimlane/GroupParentRow.tsxsrc/components/Configs/Changes/Swimlane/IconWithLabel.tsxsrc/components/Configs/Changes/Swimlane/Legend.tsxsrc/components/Configs/Changes/Swimlane/LoadingSwimlaneState.tsxsrc/components/Configs/Changes/Swimlane/ResizeHandle.tsxsrc/components/Configs/Changes/Swimlane/SeverityBadges.tsxsrc/components/Configs/Changes/Swimlane/SeverityTooltip.tsxsrc/components/Configs/Changes/Swimlane/SwimlaneRow.tsxsrc/components/Configs/Changes/Swimlane/TimeAxisHeader.tsxsrc/components/Configs/Changes/Swimlane/Tooltip.tsxsrc/components/Configs/Changes/Swimlane/Utils.tssrc/components/Configs/Changes/Swimlane/index.tssrc/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.ts
Comment on lines
+85
to
+90
| const config = items[0]!.config; | ||
| const parentPath = items[0]!.path ?? config?.path; | ||
| const path = parentPath ? `${parentPath}.${configId}` : configId; | ||
| return { | ||
| name: config?.name ?? items[0]!.name ?? configId, | ||
| path, |
Contributor
There was a problem hiding this comment.
Pick row metadata from the first populated change, not items[0].
config, path, and name are optional on ConfigChange, but this block always builds the lane from the first grouped record. If that first record is one of the sparse ones, the lane falls back to the raw id and can lose its ancestor path even when later records in the same config group already have the metadata. That makes grouping and labels order-dependent.
Suggested fix
- const config = items[0]!.config;
- const parentPath = items[0]!.path ?? config?.path;
+ const representative =
+ items.find((item) => item.config || item.path || item.name) ??
+ items[0]!;
+ const config =
+ representative.config ?? items.find((item) => item.config)?.config;
+ const parentPath = representative.path ?? config?.path;
const path = parentPath ? `${parentPath}.${configId}` : configId;
return {
- name: config?.name ?? items[0]!.name ?? configId,
+ name: config?.name ?? representative.name ?? configId,
path,
config,🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 85 -
90, The current lane-builder always uses items[0] to derive config, parentPath,
path and name which breaks when the first ConfigChange is sparse; instead locate
the first populated change in items that has metadata (e.g. config?.path,
config?.name, path or name) and use that item's config to compute parentPath and
name; keep configId for the final path suffix (path = parentPath ?
`${parentPath}.${configId}` : configId) and fall back to the first item's raw
values only if no populated item is found. Ensure references: items, config,
parentPath, path, name, ConfigChange, and configId are updated accordingly.
Comment on lines
+31
to
+51
| if (labelPlacement === "extra") { | ||
| return ( | ||
| <HoverCard openDelay={200} closeDelay={100}> | ||
| <HoverCardTrigger asChild> | ||
| <button | ||
| className="inline-flex cursor-pointer items-center" | ||
| onClick={() => onItemClicked(group.representative)} | ||
| > | ||
| <ExtraDot text={group.representative.change_type} /> | ||
| </button> | ||
| </HoverCardTrigger> | ||
| <HoverCardContent | ||
| side="top" | ||
| align="end" | ||
| collisionPadding={16} | ||
| className="w-fit min-w-56 max-w-lg p-0" | ||
| > | ||
| {tooltip} | ||
| </HoverCardContent> | ||
| </HoverCard> | ||
| ); |
Contributor
There was a problem hiding this comment.
extra placement click always opens only the representative change regardless of group count.
When group.count > 1 and labelPlacement === "extra", the button's onClick on Line 37 calls onItemClicked(group.representative), opening only the representative item. The hover tooltip via GroupedSwimlaneTooltip does show all grouped changes, but a direct click silently drops the rest. Verify this is the intended UX; if not, consider routing the click through GroupedSwimlaneTooltip or disabling the click handler for multi-change groups.
🤖 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 `@src/components/Configs/Changes/Swimlane/IconWithLabel.tsx` around lines 31 -
51, The button in IconWithLabel.tsx currently always calls
onItemClicked(group.representative) when labelPlacement === "extra", which
wrongly opens only the representative for multi-change groups; update the click
behavior so when group.count > 1 it either delegates to the grouped UI (e.g.,
trigger GroupedSwimlaneTooltip or call a new handler like onGroupClicked) or
disables the click to avoid silently dropping items, otherwise keep the existing
onItemClicked(group.representative) behavior; locate the button using ExtraDot,
group.representative, and onItemClicked to implement the conditional click
routing.
Comment on lines
+1
to
+11
| import { ConfigChange } from "@flanksource-ui/api/types/configs"; | ||
| import { | ||
| HoverCard, | ||
| HoverCardTrigger | ||
| } from "@flanksource-ui/components/ui/hover-card"; | ||
| import { PortaledHoverCardContent as HoverCardContent } from "@flanksource-ui/components/ui/portaled-hover-card"; | ||
| import { CircleAlert, Info, OctagonAlert, TriangleAlert } from "lucide-react"; | ||
| import { SeverityCounts, filterBySeverity } from "./Utils"; | ||
| import { SeverityTooltip } from "./SeverityTooltip"; | ||
|
|
||
| const severityEntries: { key: keyof SeverityCounts; icon: React.ReactNode }[] = |
Contributor
There was a problem hiding this comment.
Missing React import — React.ReactNode type reference will fail.
React.ReactNode is used in the type annotation for severityEntries (Line 11) but React is not imported.
🐛 Proposed fix
import { ConfigChange } from "@flanksource-ui/api/types/configs";
+import { type ReactNode } from "react";
import {
HoverCard,
HoverCardTrigger
} from "@flanksource-ui/components/ui/hover-card";-const severityEntries: { key: keyof SeverityCounts; icon: React.ReactNode }[] =
+const severityEntries: { key: keyof SeverityCounts; icon: ReactNode }[] =📝 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
| import { ConfigChange } from "@flanksource-ui/api/types/configs"; | |
| import { | |
| HoverCard, | |
| HoverCardTrigger | |
| } from "@flanksource-ui/components/ui/hover-card"; | |
| import { PortaledHoverCardContent as HoverCardContent } from "@flanksource-ui/components/ui/portaled-hover-card"; | |
| import { CircleAlert, Info, OctagonAlert, TriangleAlert } from "lucide-react"; | |
| import { SeverityCounts, filterBySeverity } from "./Utils"; | |
| import { SeverityTooltip } from "./SeverityTooltip"; | |
| const severityEntries: { key: keyof SeverityCounts; icon: React.ReactNode }[] = | |
| import { ConfigChange } from "@flanksource-ui/api/types/configs"; | |
| import { type ReactNode } from "react"; | |
| import { | |
| HoverCard, | |
| HoverCardTrigger | |
| } from "@flanksource-ui/components/ui/hover-card"; | |
| import { PortaledHoverCardContent as HoverCardContent } from "@flanksource-ui/components/ui/portaled-hover-card"; | |
| import { CircleAlert, Info, OctagonAlert, TriangleAlert } from "lucide-react"; | |
| import { SeverityCounts, filterBySeverity } from "./Utils"; | |
| import { SeverityTooltip } from "./SeverityTooltip"; | |
| const severityEntries: { key: keyof SeverityCounts; icon: ReactNode }[] = |
🤖 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 `@src/components/Configs/Changes/Swimlane/SeverityBadges.tsx` around lines 1 -
11, Import the React namespace so the React.ReactNode type resolves: add an
import for React (e.g., import React from "react") at the top of the file where
severityEntries is declared so the type annotation React.ReactNode used in the
severityEntries definition compiles correctly; alternatively you can import the
type directly (import type { ReactNode } from "react") and update the
declaration to use ReactNode.
Comment on lines
+1
to
+3
| import { relativeDateTime } from "@flanksource-ui/utils/date"; | ||
| import dayjs from "dayjs"; | ||
| import { calcPercent } from "./Utils"; |
Contributor
There was a problem hiding this comment.
Missing React import — React.MutableRefObject will fail to resolve.
The type annotation on Line 13 references React.MutableRefObject, but React is never imported. With the new JSX transform, React doesn't need to be imported for JSX, but the React namespace is still required for type references like this.
🐛 Proposed fix
+import React from "react";
import { relativeDateTime } from "@flanksource-ui/utils/date";
import dayjs from "dayjs";
import { calcPercent } from "./Utils";Alternatively, import just the type:
+import { type MutableRefObject } from "react";
import { relativeDateTime } from "@flanksource-ui/utils/date";
import dayjs from "dayjs";
import { calcPercent } from "./Utils";And update the prop type on Line 13:
- markersRef: React.MutableRefObject<HTMLDivElement | null>;
+ markersRef: MutableRefObject<HTMLDivElement | null>;📝 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
| import { relativeDateTime } from "@flanksource-ui/utils/date"; | |
| import dayjs from "dayjs"; | |
| import { calcPercent } from "./Utils"; | |
| import React from "react"; | |
| import { relativeDateTime } from "@flanksource-ui/utils/date"; | |
| import dayjs from "dayjs"; | |
| import { calcPercent } from "./Utils"; |
🤖 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 `@src/components/Configs/Changes/Swimlane/TimeAxisHeader.tsx` around lines 1 -
3, The file references React.MutableRefObject in the TimeAxisHeader props but
never imports React, causing the type to be unresolved; fix by adding an import
for the React namespace (or a type-only import like `import type React from
"react"`) at the top of
src/components/Configs/Changes/Swimlane/TimeAxisHeader.tsx so the
React.MutableRefObject type used in the component prop definition (the prop line
that currently mentions React.MutableRefObject) resolves correctly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/ui/Dates/TimeRangePicker/TimeRangeList.tsx (1)
24-35: ⚡ Quick winInvalid preset clicks currently fail silently.
validateRangereturns a message, but this path only treats it as a boolean and drops the reason. Any caller that supplies validation without also pruning its options will render clickable rows that do nothing. Please either disable invalid options during render or plumb the returned message back to the body so the failure is visible.🤖 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 `@src/ui/Dates/TimeRangePicker/TimeRangeList.tsx` around lines 24 - 35, The click-silent bug is caused by treating validateRange's return value as a boolean in TimeRangeList (see validateRange and setOption) and ignoring its message; fix by using validateRange(option) to get a potential error string and either (A) disable invalid options during render (compute const reason = validateRange?.(option); pass disabled={!!reason} on each row/item and show reason in a tooltip/aria-description) or (B) surface the message when a user clicks (in setOption, if const reason = validateRange?.(option) then set a local error state or call a provided onError handler with reason so the UI displays the message rather than silently returning). Ensure isChecked logic remains unchanged and update any click handlers to no-op only when disabled, not silently swallow a reason.
🤖 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
`@src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsx`:
- Around line 140-144: The effect in useEffect currently overwrites the shared
URL time-range params when toggling to Graph view by calling
setTimeRangeParams(configChangesGraphDefaultDateFilter, paramsToReset) whenever
isGraphView && isRangeOverGraphLimit(timeRangeValue); instead, avoid mutating
the shared filter: implement a graph-only fallback or separate per-view
state—e.g., introduce a graphTimeRange (or graph-specific param key) and only
apply configChangesGraphDefaultDateFilter to that graph-only state when
isGraphView is true, leaving the shared timeRangeValue and URL params untouched;
update the useEffect to conditionally set only the graph-specific state/params
(using isRangeOverGraphLimit(timeRangeValue) to detect overflow) rather than
calling setTimeRangeParams on the shared paramsToReset.
- Around line 54-61: The code currently treats labels in
mappedRangesOverGraphLimit (notably "This month so far" and "This year so far")
as always over-limit which incorrectly rejects dynamic ranges; update the logic
that rejects ranges (the check referencing mappedRangesOverGraphLimit in
ConfigChangesDateRangeFIlter.tsx) to not special-case these "so far" labels but
instead compute the actual duration from the resolved from/to bounds (the
mapped/display range → resolved from/to used elsewhere in this component) and
use that duration for the graph-limit decision; remove "This month so far" and
"This year so far" from mappedRangesOverGraphLimit and ensure the rejection uses
the computed difference between resolved from and to for all mapped ranges.
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx`:
- Around line 67-93: getIncreasedTimeRange currently returns the same preset
when the range is already at the 7-day cap, so the "More" control remains active
but is a no-op; modify getIncreasedTimeRange to detect when the computed
nextPreset matches the currentRange (e.g., when currentRange.type === "relative"
&& currentRange.range === nextPreset.range) and return a sentinel
(null/undefined) or a flag indicating the cap is reached; then update the UI
rendering logic that shows/enables the "More" affordance (the code path using
hasPreRangeChanges / the caller of getIncreasedTimeRange in
ConfigChangesSwimlane) to hide or disable the control when the function returns
that sentinel / flag.
In `@src/components/Configs/Changes/Swimlane/GroupParentRow.tsx`:
- Around line 79-83: The group toggle button in the GroupParentRow component is
missing an explicit type and will act as a submit button inside forms; update
the <button> element used for the expand/collapse control in GroupParentRow to
include type="button" (keep existing className, style using indentLevel, and the
onClick={onToggle} handler) so it does not submit surrounding forms.
In `@src/components/Configs/Changes/Swimlane/SwimlaneRow.tsx`:
- Around line 64-73: The pre-range badge (row.preRangeBadge) is currently
rendered as an inline sibling before <BucketCells> which shifts bucket columns;
change SwimlaneRow so the left gutter remains reserved by timelineOffsetWidth
but the badge is removed from normal flow: make the container that uses style={{
paddingLeft: timelineOffsetWidth }} position: relative, render row.preRangeBadge
as an absolutely positioned element anchored into that left gutter (e.g.,
position absolute with left 0 and vertically centered) so it overlays the
reserved space without affecting <BucketCells> layout, and keep <BucketCells> as
the sole flow child so columns stay aligned with the header and neighboring
lanes.
---
Nitpick comments:
In `@src/ui/Dates/TimeRangePicker/TimeRangeList.tsx`:
- Around line 24-35: The click-silent bug is caused by treating validateRange's
return value as a boolean in TimeRangeList (see validateRange and setOption) and
ignoring its message; fix by using validateRange(option) to get a potential
error string and either (A) disable invalid options during render (compute const
reason = validateRange?.(option); pass disabled={!!reason} on each row/item and
show reason in a tooltip/aria-description) or (B) surface the message when a
user clicks (in setOption, if const reason = validateRange?.(option) then set a
local error state or call a provided onError handler with reason so the UI
displays the message rather than silently returning). Ensure isChecked logic
remains unchanged and update any click handlers to no-op only when disabled, not
silently swallow a reason.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dbb11c9-b0c8-4df7-ac15-3024329071a9
📒 Files selected for processing (9)
src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsxsrc/components/Configs/Changes/ConfigChangesSwimlane.tsxsrc/components/Configs/Changes/Swimlane/GroupParentRow.tsxsrc/components/Configs/Changes/Swimlane/MoreTimeRangeHandle.tsxsrc/components/Configs/Changes/Swimlane/SwimlaneRow.tsxsrc/components/Configs/Changes/Swimlane/TimeAxisHeader.tsxsrc/ui/Dates/TimeRangePicker/TimeRangeList.tsxsrc/ui/Dates/TimeRangePicker/TimeRangePicker.tsxsrc/ui/Dates/TimeRangePicker/TimeRangePickerBody.tsx
Comment on lines
+54
to
+61
| const mappedRangesOverGraphLimit = new Set([ | ||
| "Previous month", | ||
| "Previous year", | ||
| "This month", | ||
| "This month so far", | ||
| "This year", | ||
| "This year so far" | ||
| ]); |
Contributor
There was a problem hiding this comment.
Don't hard-code This month so far / This year so far as over-limit.
These labels are not always longer than 7 days. On May 6, 2026, This month so far spans May 1, 2026 → May 6, 2026, so Line 79 rejects a valid graph range before the absolute-duration check runs. This year so far has the same problem during January 1–7. Please rely on the resolved from/to bounds for these dynamic mapped ranges instead of the display text.
💡 Minimal fix
const mappedRangesOverGraphLimit = new Set([
"Previous month",
"Previous year",
"This month",
- "This month so far",
"This year",
- "This year so far"
]);Also applies to: 77-82
🤖 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
`@src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsx`
around lines 54 - 61, The code currently treats labels in
mappedRangesOverGraphLimit (notably "This month so far" and "This year so far")
as always over-limit which incorrectly rejects dynamic ranges; update the logic
that rejects ranges (the check referencing mappedRangesOverGraphLimit in
ConfigChangesDateRangeFIlter.tsx) to not special-case these "so far" labels but
instead compute the actual duration from the resolved from/to bounds (the
mapped/display range → resolved from/to used elsewhere in this component) and
use that duration for the graph-limit decision; remove "This month so far" and
"This year so far" from mappedRangesOverGraphLimit and ensure the rejection uses
the computed difference between resolved from and to for all mapped ranges.
Comment on lines
+140
to
+144
| useEffect(() => { | ||
| if (isGraphView && isRangeOverGraphLimit(timeRangeValue)) { | ||
| setTimeRangeParams(configChangesGraphDefaultDateFilter, paramsToReset); | ||
| } | ||
| }, [isGraphView, paramsToReset, setTimeRangeParams, timeRangeValue]); |
Contributor
There was a problem hiding this comment.
Avoid overwriting the shared time-range params on a view toggle.
This effect turns a non-graph range into now-2h as soon as the user enters Graph view. Because the time range lives in the URL, switching back to Table loses the user's original selection as well. A view toggle should not destructively rewrite shared filters; this needs a graph-only fallback or separate range state per view.
🤖 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
`@src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsx`
around lines 140 - 144, The effect in useEffect currently overwrites the shared
URL time-range params when toggling to Graph view by calling
setTimeRangeParams(configChangesGraphDefaultDateFilter, paramsToReset) whenever
isGraphView && isRangeOverGraphLimit(timeRangeValue); instead, avoid mutating
the shared filter: implement a graph-only fallback or separate per-view
state—e.g., introduce a graphTimeRange (or graph-specific param key) and only
apply configChangesGraphDefaultDateFilter to that graph-only state when
isGraphView is true, leaving the shared timeRangeValue and URL params untouched;
update the useEffect to conditionally set only the graph-specific state/params
(using isRangeOverGraphLimit(timeRangeValue) to detect overflow) rather than
calling setTimeRangeParams on the shared paramsToReset.
Comment on lines
+67
to
+93
| function getIncreasedTimeRange(range?: TimeRangeOption): TimeRangeOption { | ||
| const currentRange = range ?? GRAPH_DEFAULT_RANGE; | ||
| const { from, to } = timeRangeOptionsToAbsolute(currentRange); | ||
| const fromDate = resolveRangeDate(from); | ||
| const toDate = resolveRangeDate(to, true); | ||
| const durationMs = Math.max(0, toDate.diff(fromDate)); | ||
| const nextPreset = | ||
| GRAPH_RANGE_PRESETS.find((preset) => preset.ms > durationMs + 1000) ?? | ||
| GRAPH_RANGE_PRESETS[GRAPH_RANGE_PRESETS.length - 1]!; | ||
|
|
||
| if (currentRange.type === "relative" && to === "now") { | ||
| return { | ||
| type: "relative", | ||
| display: nextPreset.display, | ||
| range: nextPreset.range | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| type: "absolute", | ||
| display: "Custom", | ||
| from: toDate | ||
| .subtract(nextPreset.ms, "millisecond") | ||
| .format(displayTimeFormat), | ||
| to: toDate.format(displayTimeFormat) | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Hide or disable the “More” affordance once the graph reaches the 7-day cap.
When the current range is already now-7d, getIncreasedTimeRange() resolves back to the same preset. Because the bar is still rendered whenever hasPreRangeChanges is true, users can click a control that does nothing.
💡 One way to gate the control
-function getIncreasedTimeRange(range?: TimeRangeOption): TimeRangeOption {
+function getIncreasedTimeRange(
+ range?: TimeRangeOption
+): TimeRangeOption | undefined {
const currentRange = range ?? GRAPH_DEFAULT_RANGE;
const { from, to } = timeRangeOptionsToAbsolute(currentRange);
const fromDate = resolveRangeDate(from);
const toDate = resolveRangeDate(to, true);
const durationMs = Math.max(0, toDate.diff(fromDate));
- const nextPreset =
- GRAPH_RANGE_PRESETS.find((preset) => preset.ms > durationMs + 1000) ??
- GRAPH_RANGE_PRESETS[GRAPH_RANGE_PRESETS.length - 1]!;
+ const nextPreset = GRAPH_RANGE_PRESETS.find(
+ (preset) => preset.ms > durationMs + 1000
+ );
+
+ if (!nextPreset) {
+ return undefined;
+ }- const increaseTimeRange = useCallback(() => {
- setTimeRangeParams(getIncreasedTimeRange(timeRangeValue));
- }, [setTimeRangeParams, timeRangeValue]);
+ const nextTimeRange = useMemo(
+ () => getIncreasedTimeRange(timeRangeValue),
+ [timeRangeValue]
+ );
+
+ const increaseTimeRange = useCallback(() => {
+ if (nextTimeRange) {
+ setTimeRangeParams(nextTimeRange);
+ }
+ }, [nextTimeRange, setTimeRangeParams]);
...
- {hasPreRangeChanges && (
+ {hasPreRangeChanges && nextTimeRange && (
<MoreTimeRangeBar left={columnWidth} onClick={increaseTimeRange} />
)}Also applies to: 266-268
🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 67 -
93, getIncreasedTimeRange currently returns the same preset when the range is
already at the 7-day cap, so the "More" control remains active but is a no-op;
modify getIncreasedTimeRange to detect when the computed nextPreset matches the
currentRange (e.g., when currentRange.type === "relative" && currentRange.range
=== nextPreset.range) and return a sentinel (null/undefined) or a flag
indicating the cap is reached; then update the UI rendering logic that
shows/enables the "More" affordance (the code path using hasPreRangeChanges /
the caller of getIncreasedTimeRange in ConfigChangesSwimlane) to hide or disable
the control when the function returns that sentinel / flag.
Comment on lines
+79
to
+83
| <button | ||
| className="mr-1 flex min-w-0 flex-1 items-center gap-1 text-gray-600 hover:text-gray-900" | ||
| style={{ paddingLeft: indentLevel * 24 }} | ||
| onClick={onToggle} | ||
| > |
Contributor
There was a problem hiding this comment.
Add type="button" to the group toggle.
This control only expands/collapses the lane. Without an explicit type, it will submit any surrounding form by default.
🔧 Minimal fix
<button
+ type="button"
className="mr-1 flex min-w-0 flex-1 items-center gap-1 text-gray-600 hover:text-gray-900"
style={{ paddingLeft: indentLevel * 24 }}
onClick={onToggle}
>📝 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
| <button | |
| className="mr-1 flex min-w-0 flex-1 items-center gap-1 text-gray-600 hover:text-gray-900" | |
| style={{ paddingLeft: indentLevel * 24 }} | |
| onClick={onToggle} | |
| > | |
| <button | |
| type="button" | |
| className="mr-1 flex min-w-0 flex-1 items-center gap-1 text-gray-600 hover:text-gray-900" | |
| style={{ paddingLeft: indentLevel * 24 }} | |
| onClick={onToggle} | |
| > |
🤖 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 `@src/components/Configs/Changes/Swimlane/GroupParentRow.tsx` around lines 79 -
83, The group toggle button in the GroupParentRow component is missing an
explicit type and will act as a submit button inside forms; update the <button>
element used for the expand/collapse control in GroupParentRow to include
type="button" (keep existing className, style using indentLevel, and the
onClick={onToggle} handler) so it does not submit surrounding forms.
Comment on lines
+64
to
+73
| <div | ||
| className="flex flex-1 flex-row items-stretch border-l border-gray-200" | ||
| style={{ paddingLeft: timelineOffsetWidth }} | ||
| > | ||
| {row.preRangeBadge && ( | ||
| <span className="flex items-center px-1 text-[10px] text-gray-400"> | ||
| {row.preRangeBadge} | ||
| </span> | ||
| )} | ||
| <BucketCells |
Contributor
There was a problem hiding this comment.
Keep the pre-range marker out of the bucket flow.
timelineOffsetWidth already reserves the shared left gutter. Rendering row.preRangeBadge as an inline sibling before BucketCells adds extra width only on rows that have the marker, so those bucket columns shift right relative to the header and neighboring lanes.
💡 One way to keep the grid aligned
<div
- className="flex flex-1 flex-row items-stretch border-l border-gray-200"
+ className="relative flex flex-1 flex-row items-stretch border-l border-gray-200"
style={{ paddingLeft: timelineOffsetWidth }}
>
{row.preRangeBadge && (
- <span className="flex items-center px-1 text-[10px] text-gray-400">
+ <span className="absolute left-0 top-0 flex h-full items-center px-1 text-[10px] text-gray-400">
{row.preRangeBadge}
</span>
)}📝 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
| <div | |
| className="flex flex-1 flex-row items-stretch border-l border-gray-200" | |
| style={{ paddingLeft: timelineOffsetWidth }} | |
| > | |
| {row.preRangeBadge && ( | |
| <span className="flex items-center px-1 text-[10px] text-gray-400"> | |
| {row.preRangeBadge} | |
| </span> | |
| )} | |
| <BucketCells | |
| <div | |
| className="relative flex flex-1 flex-row items-stretch border-l border-gray-200" | |
| style={{ paddingLeft: timelineOffsetWidth }} | |
| > | |
| {row.preRangeBadge && ( | |
| <span className="absolute left-0 top-0 flex h-full items-center px-1 text-[10px] text-gray-400"> | |
| {row.preRangeBadge} | |
| </span> | |
| )} | |
| <BucketCells |
🤖 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 `@src/components/Configs/Changes/Swimlane/SwimlaneRow.tsx` around lines 64 -
73, The pre-range badge (row.preRangeBadge) is currently rendered as an inline
sibling before <BucketCells> which shifts bucket columns; change SwimlaneRow so
the left gutter remains reserved by timelineOffsetWidth but the badge is removed
from normal flow: make the container that uses style={{ paddingLeft:
timelineOffsetWidth }} position: relative, render row.preRangeBadge as an
absolutely positioned element anchored into that left gutter (e.g., position
absolute with left 0 and vertically centered) so it overlays the reserved space
without affecting <BucketCells> layout, and keep <BucketCells> as the sole flow
child so columns stay aligned with the header and neighboring lanes.
Replace scatter-plot timeline with a hand-rolled swimlane that scales to many configs and bursty change patterns: - Hierarchical grouping by config name prefix with collapse/expand - Sticky header row and resizable resource column (150-500px) - Bucket-based anti-overlap rendering using flex-wrap - Severity badges with multi-change counts and filtered tooltips - Pre-range indicator (gray dot in-range, ↩ badge for pre-range) - Infinite scroll via useGetAllConfigsChangesInfiniteQuery (200/page) - URL-param backed Table/Graph toggle (?view=) - Live tail remains, scoped to Table view Adds ConfigChangesSwimlane plus Legend, Tooltip, and Utils components with unit and integration test coverage backed by a real HAR fixture.
Avoid modifying the base shadcn hover-card by moving portaled content into a project wrapper used by the swimlane. Gate the config changes infinite query so table view does not issue the extra swimlane request, and clone fallback icons with the same className and forwarded props as normal icons.
Switching from table to graph view starts a separate infinite query, leaving the swimlane with no changes until the first page resolves. Pass the loading state into the swimlane so it shows an initial loading indicator instead of the empty state, and cover it with an integration test.
Add an opt-in prop to FilterByCellValue so callers can keep include and exclude actions visible instead of showing them only on hover. Enable it in the config changes swimlane tooltip where hover-only actions are hard to discover inside the hover card.
The config type tooltip in the swimlane left column was clipped by row stacking and overflow rules, making it partially or completely hidden. Raise the hovered row stacking context and allow the sticky config cell to overflow while keeping the config name itself truncated.
The swimlane used vertical scrolling to trigger fetching more changes, but that axis represents the resource list rather than the time range. Fetch graph data in 1000-change batches and automatically paginate until the selected time range is fully loaded. Remove the swimlane sentinel and loading-more row.
Move bloated ConfigChangesSwimlane presentation pieces into focused components under the Swimlane directory. Keep the main swimlane component responsible for state, bucketing, grouping, and orchestration while preserving existing behavior.
Build swimlane hierarchy from config paths instead of name prefixes, including recursive nested groups for ancestor relationships. Fetch missing ancestor config metadata in one PostgREST call so group labels use names instead of UUIDs, and expose group type in a hover tooltip.
Only render hierarchy nodes for configs present in the current change result set. Missing intermediate path ancestors are skipped so leaf configs do not appear under synthetic UUID-only parent chains.
Use graph-specific time range presets with a 2 hour default and cap available graph ranges at 7 days. Add validation support to the time range picker so custom absolute from/to selections over 7 days are rejected in graph mode.
Limit graph view loading to 2,500 changes while keeping table pagination unchanged. Show progress after the first graph response reports total changes, and keep the progress indicator stable until the loaded count reaches the cap or total. Add an inline warning near the view toggle when the selected date range has more changes than graph view will render, and default graph mode to a two-hour range when switching from the table default.
Group headers and leaf rows used different row structures, which caused inconsistent layering and duplicated parent rows when rendering config hierarchies. Introduce a shared swimlane config row base for common label, severity, resize, and timeline layout. Render group headers as the parent config row and remove the synthetic duplicate child row.
This reverts commit 6da4484.
adityathebe
force-pushed
the
feat/config-changes-swimlane
branch
from
ea18c5e to
bc52623
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/components/Configs/Changes/ConfigChangesViewToggle.tsx (1)
17-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
viewbefore treating it asConfigChangesView.Line 19 and Line 24 still cast arbitrary query-string input, so
?view=foocan leak an unsupported value into the toggle and any hook consumers. Fall back to"Table"unless the param is exactly"Table"or"Graph".Suggested fix
export type ConfigChangesView = "Table" | "Graph"; + +function parseConfigChangesView(value: string | null): ConfigChangesView { + return value === "Table" || value === "Graph" ? value : "Table"; +} export function useConfigChangesViewToggleState(): ConfigChangesView { const [params] = useSearchParams(); - return (params.get("view") as ConfigChangesView) || "Table"; + return parseConfigChangesView(params.get("view")); } export default function ConfigChangesViewToggle() { const [params, setParams] = useSearchParams(); - const value = (params.get("view") as ConfigChangesView) || "Table"; + const value = parseConfigChangesView(params.get("view"));Also applies to: 23-24
🤖 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 `@src/components/Configs/Changes/ConfigChangesViewToggle.tsx` around lines 17 - 20, The hook useConfigChangesViewToggleState currently casts the raw query param to ConfigChangesView without validation; change it to read params via useSearchParams(), check that params.get("view") strictly equals "Table" or "Graph", and only then return that value (typed as ConfigChangesView); otherwise return the default "Table" to avoid leaking unsupported values to consumers of useConfigChangesViewToggleState or any code relying on ConfigChangesView.src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsx (2)
60-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't reject
"so far"ranges by label alone.On May 7, 2026,
"This month so far"only spans May 1, 2026 → May 7, 2026, so this short-circuit blocks a valid Graph range before the absolute-duration check runs."This year so far"has the same problem during January 1–7.Minimal fix
const mappedRangesOverGraphLimit = new Set([ "Previous month", "Previous year", "This month", - "This month so far", "This year", - "This year so far" ]);Also applies to: 83-88
🤖 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 `@src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsx` around lines 60 - 67, The current mappedRangesOverGraphLimit set (named mappedRangesOverGraphLimit) rejects labels like "This month so far" and "This year so far" by string match, which incorrectly blocks short "so far" ranges; change the logic so that these "so far" labels are not rejected by label alone—remove "This month so far" and "This year so far" from mappedRangesOverGraphLimit (and the duplicate set used elsewhere), and instead rely on the existing absolute-duration check that evaluates the actual date span for Graph limit decisions (i.e., only short-circuit labels that truly imply over-limit spans, keep the duration-based guard for all "so far" ranges).
142-146:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the table range when clamping Graph mode.
This effect rewrites the shared URL range to
now-2has soon as Graph view sees an over-limit selection. If a user enters Graph from a 30-day Table range, switching back to Table loses that original filter entirely.Use Graph-specific fallback state/params instead of calling
setTimeRangeParamson the shared range.🤖 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 `@src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsx` around lines 142 - 146, The current useEffect in ConfigChangesDateRangeFIlter calls setTimeRangeParams (shared URL range) when Graph detects an over-limit selection, which overwrites the Table range; instead, introduce and use a Graph-specific fallback (e.g. local state or a dedicated setter like setGraphTimeRangeParams) so only Graph view's range is clamped — keep the shared setTimeRangeParams untouched. Concretely: stop calling setTimeRangeParams inside the effect; when isGraphView && isRangeOverGraphLimit(timeRangeValue) update only graph-scoped params (create/use setGraphTimeRangeParams or a local state variable named graphTimeRangeFallback initialized from configChangesGraphDefaultDateFilter) and ensure switching back to Table reads the original shared params (paramsToReset) unchanged. Reference symbols: useEffect, isGraphView, isRangeOverGraphLimit, setTimeRangeParams, configChangesGraphDefaultDateFilter, timeRangeValue, paramsToReset.src/components/Configs/Changes/ConfigChangesSwimlane.tsx (1)
85-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRow metadata is still derived from
items[0], not the first populated change.
items[0]!.configanditems[0]!.pathmay both beundefinedif the first record in the group is sparse, even when later records in the same group carry the full metadata. This makes lane labels and grouping paths order-dependent.The fix proposed in the previous review remains valid:
💡 Proposed fix — resolve metadata from first populated item
- const config = items[0]!.config; - const parentPath = items[0]!.path ?? config?.path; + const representative = + items.find((item) => item.config || item.path || item.name) ?? + items[0]!; + const config = + representative.config ?? items.find((item) => item.config)?.config; + const parentPath = representative.path ?? config?.path; const path = parentPath ? `${parentPath}.${configId}` : configId; return { - name: config?.name ?? items[0]!.name ?? configId, + name: config?.name ?? representative.name ?? configId,🤖 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 `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 85 - 89, The row metadata incorrectly always uses items[0]; change the logic in ConfigChangesSwimlane (where config, parentPath, path, and name are derived) to locate the first populated item in items (e.g., first item where item.config or item.path or item.name is defined) and use that item instead of items[0] when computing const config, const parentPath, const path = parentPath ? `${parentPath}.${configId}` : configId, and the returned name; ensure you fall back to configId if nothing populated.src/components/Configs/Changes/Swimlane/SwimlaneRow.tsx (1)
41-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-range badge is still an inline sibling and will misalign bucket columns.
The
preRangeBadge<span>adds width to the flex row only when present, soBucketCellsstarts at a different offset than theTimeAxisHeaderand rows without the badge, breaking column alignment.The fix suggested in the previous review is still applicable: make the container
relativeand render the badge as an absolutely-positioned overlay within the reservedpaddingLeftgutter.💡 Proposed alignment fix
- <div className="flex flex-1 flex-row items-stretch border-l border-gray-200"> + <div + className="relative flex flex-1 flex-row items-stretch border-l border-gray-200" + style={{ paddingLeft: timelineOffsetWidth }} + > {row.preRangeBadge && ( - <span className="flex items-center px-1 text-[10px] text-gray-400"> + <span className="absolute left-0 top-0 flex h-full items-center px-1 text-[10px] text-gray-400"> {row.preRangeBadge} </span> )} <BucketCells buckets={row.buckets} numBuckets={numBuckets} onItemClicked={onItemClicked} min={min} max={max} /> </div>🤖 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 `@src/components/Configs/Changes/Swimlane/SwimlaneRow.tsx` around lines 41 - 54, SwimlaneRow renders the preRangeBadge as an inline sibling which shifts BucketCells and breaks alignment with TimeAxisHeader; to fix, make the row container (the div around BucketCells) position: relative and reserve a fixed left gutter (e.g., padding-left equal to the badge width) so BucketCells always starts at the same offset, then render the badge absolutely positioned inside that container (change the preRangeBadge span to position: absolute; left: 0; vertically centered) so it overlays the reserved gutter without affecting layout; update CSS/classes on the container and the badge span and ensure BucketCells and TimeAxisHeader use the same gutter width.
🤖 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
`@src/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.ts`:
- Around line 32-40: The helper function makeRow is unused and flagged by
ESLint; either remove makeRow from the test file or add a unit test that uses it
to construct a BucketedRow and exercise the bucketChanges/mergeBuckets flow (for
example call groupRowsByPath on a set of rows including makeRow output and then
assert mergeBuckets produces the expected merged buckets), referencing the
helper name makeRow and the utilities bucketChanges, mergeBuckets and
groupRowsByPath so the linter warning is resolved.
In `@src/components/Configs/Changes/Swimlane/SeverityTooltip.tsx`:
- Around line 41-50: The icon-only control in SeverityTooltip should be made
explicitly accessible and form-safe: update the button in the SeverityTooltip
component (the element that calls onExpand(change) and renders <Maximize2 />) to
include type="button" to prevent accidental form submission and add an
accessible name such as aria-label="View full details" (or aria-labelledby
pointing to a visible label) so screen readers can announce its purpose; keep
the existing e.stopPropagation() and title attribute intact.
---
Duplicate comments:
In
`@src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsx`:
- Around line 60-67: The current mappedRangesOverGraphLimit set (named
mappedRangesOverGraphLimit) rejects labels like "This month so far" and "This
year so far" by string match, which incorrectly blocks short "so far" ranges;
change the logic so that these "so far" labels are not rejected by label
alone—remove "This month so far" and "This year so far" from
mappedRangesOverGraphLimit (and the duplicate set used elsewhere), and instead
rely on the existing absolute-duration check that evaluates the actual date span
for Graph limit decisions (i.e., only short-circuit labels that truly imply
over-limit spans, keep the duration-based guard for all "so far" ranges).
- Around line 142-146: The current useEffect in ConfigChangesDateRangeFIlter
calls setTimeRangeParams (shared URL range) when Graph detects an over-limit
selection, which overwrites the Table range; instead, introduce and use a
Graph-specific fallback (e.g. local state or a dedicated setter like
setGraphTimeRangeParams) so only Graph view's range is clamped — keep the shared
setTimeRangeParams untouched. Concretely: stop calling setTimeRangeParams inside
the effect; when isGraphView && isRangeOverGraphLimit(timeRangeValue) update
only graph-scoped params (create/use setGraphTimeRangeParams or a local state
variable named graphTimeRangeFallback initialized from
configChangesGraphDefaultDateFilter) and ensure switching back to Table reads
the original shared params (paramsToReset) unchanged. Reference symbols:
useEffect, isGraphView, isRangeOverGraphLimit, setTimeRangeParams,
configChangesGraphDefaultDateFilter, timeRangeValue, paramsToReset.
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx`:
- Around line 85-89: The row metadata incorrectly always uses items[0]; change
the logic in ConfigChangesSwimlane (where config, parentPath, path, and name are
derived) to locate the first populated item in items (e.g., first item where
item.config or item.path or item.name is defined) and use that item instead of
items[0] when computing const config, const parentPath, const path = parentPath
? `${parentPath}.${configId}` : configId, and the returned name; ensure you fall
back to configId if nothing populated.
In `@src/components/Configs/Changes/ConfigChangesViewToggle.tsx`:
- Around line 17-20: The hook useConfigChangesViewToggleState currently casts
the raw query param to ConfigChangesView without validation; change it to read
params via useSearchParams(), check that params.get("view") strictly equals
"Table" or "Graph", and only then return that value (typed as
ConfigChangesView); otherwise return the default "Table" to avoid leaking
unsupported values to consumers of useConfigChangesViewToggleState or any code
relying on ConfigChangesView.
In `@src/components/Configs/Changes/Swimlane/SwimlaneRow.tsx`:
- Around line 41-54: SwimlaneRow renders the preRangeBadge as an inline sibling
which shifts BucketCells and breaks alignment with TimeAxisHeader; to fix, make
the row container (the div around BucketCells) position: relative and reserve a
fixed left gutter (e.g., padding-left equal to the badge width) so BucketCells
always starts at the same offset, then render the badge absolutely positioned
inside that container (change the preRangeBadge span to position: absolute;
left: 0; vertically centered) so it overlays the reserved gutter without
affecting layout; update CSS/classes on the container and the badge span and
ensure BucketCells and TimeAxisHeader use the same gutter width.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ae4b1c0-695d-4803-8da7-86f60cec6206
📒 Files selected for processing (34)
src/api/query-hooks/useConfigChangesHooks.tssrc/api/types/configs.tssrc/components/Configs/Changes/ConfigChangesFilters/ConfigChangesDateRangeFIlter.tsxsrc/components/Configs/Changes/ConfigChangesSwimlane.tsxsrc/components/Configs/Changes/ConfigChangesViewToggle.tsxsrc/components/Configs/Changes/Swimlane/BucketCells.tsxsrc/components/Configs/Changes/Swimlane/ChangeIconWithBadge.tsxsrc/components/Configs/Changes/Swimlane/ChangeRow.tsxsrc/components/Configs/Changes/Swimlane/EmptySwimlaneState.tsxsrc/components/Configs/Changes/Swimlane/FlexLabel.tsxsrc/components/Configs/Changes/Swimlane/GroupParentRow.tsxsrc/components/Configs/Changes/Swimlane/IconWithLabel.tsxsrc/components/Configs/Changes/Swimlane/Legend.tsxsrc/components/Configs/Changes/Swimlane/LoadingSwimlaneState.tsxsrc/components/Configs/Changes/Swimlane/ResizeHandle.tsxsrc/components/Configs/Changes/Swimlane/SeverityBadges.tsxsrc/components/Configs/Changes/Swimlane/SeverityTooltip.tsxsrc/components/Configs/Changes/Swimlane/SwimlaneConfigRowBase.tsxsrc/components/Configs/Changes/Swimlane/SwimlaneRow.tsxsrc/components/Configs/Changes/Swimlane/TimeAxisHeader.tsxsrc/components/Configs/Changes/Swimlane/Tooltip.tsxsrc/components/Configs/Changes/Swimlane/Utils.tssrc/components/Configs/Changes/Swimlane/index.tssrc/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsxsrc/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.tssrc/components/Configs/Changes/__tests__/changes.harsrc/components/ui/portaled-hover-card.tsxsrc/pages/config/ConfigChangesPage.tsxsrc/ui/DataTable/FilterByCellValue.tsxsrc/ui/Dates/TimeRangePicker/TimeRangeList.tsxsrc/ui/Dates/TimeRangePicker/TimeRangePicker.tsxsrc/ui/Dates/TimeRangePicker/TimeRangePickerBody.tsxsrc/ui/Icons/ChangeIcon.tsxsrc/ui/Icons/Icon.tsx
✅ Files skipped from review due to trivial changes (11)
- src/components/Configs/Changes/Swimlane/EmptySwimlaneState.tsx
- src/components/Configs/Changes/Swimlane/ResizeHandle.tsx
- src/components/Configs/Changes/Swimlane/FlexLabel.tsx
- src/components/Configs/Changes/Swimlane/LoadingSwimlaneState.tsx
- src/components/Configs/Changes/Swimlane/ChangeIconWithBadge.tsx
- src/components/Configs/Changes/Swimlane/index.ts
- src/components/ui/portaled-hover-card.tsx
- src/api/types/configs.ts
- src/components/Configs/Changes/Swimlane/ChangeRow.tsx
- src/components/Configs/Changes/Swimlane/GroupParentRow.tsx
- src/components/Configs/Changes/Swimlane/IconWithLabel.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- src/components/Configs/Changes/Swimlane/Legend.tsx
- src/ui/Icons/ChangeIcon.tsx
- src/ui/Dates/TimeRangePicker/TimeRangePicker.tsx
- src/ui/Icons/Icon.tsx
- src/ui/DataTable/FilterByCellValue.tsx
- src/components/Configs/Changes/Swimlane/SeverityBadges.tsx
- src/ui/Dates/TimeRangePicker/TimeRangePickerBody.tsx
- src/components/Configs/Changes/Swimlane/BucketCells.tsx
- src/components/Configs/Changes/Swimlane/TimeAxisHeader.tsx
- src/api/query-hooks/useConfigChangesHooks.ts
- src/components/Configs/Changes/tests/ConfigChangesSwimlane.integration.test.tsx
- src/pages/config/ConfigChangesPage.tsx
- src/ui/Dates/TimeRangePicker/TimeRangeList.tsx
- src/components/Configs/Changes/Swimlane/Utils.ts
Comment on lines
+32
to
+40
| function makeRow(name: string, numBuckets: number): BucketedRow { | ||
| return { | ||
| name, | ||
| config: { id: "cfg1", type: "Kubernetes::Pod", name }, | ||
| buckets: Array.from({ length: numBuckets }, () => []), | ||
| severity: ZERO_SEVERITY, | ||
| totalCount: 0 | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Remove or use the makeRow helper — it is flagged as unused by ESLint.
makeRow is defined but not referenced in any test case. Delete it, or add a test that exercises bucketChanges/mergeBuckets with a fully-formed BucketedRow produced by this helper (e.g., to cover the groupRowsByPath + mergeBuckets interaction).
🗑️ Minimal fix — remove the dead helper
-function makeRow(name: string, numBuckets: number): BucketedRow {
- return {
- name,
- config: { id: "cfg1", type: "Kubernetes::Pod", name },
- buckets: Array.from({ length: numBuckets }, () => []),
- severity: ZERO_SEVERITY,
- totalCount: 0
- };
-}📝 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
| function makeRow(name: string, numBuckets: number): BucketedRow { | |
| return { | |
| name, | |
| config: { id: "cfg1", type: "Kubernetes::Pod", name }, | |
| buckets: Array.from({ length: numBuckets }, () => []), | |
| severity: ZERO_SEVERITY, | |
| totalCount: 0 | |
| }; | |
| } |
🧰 Tools
🪛 GitHub Check: eslint
[warning] 32-32:
'makeRow' is defined but never used
🤖 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
`@src/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.ts`
around lines 32 - 40, The helper function makeRow is unused and flagged by
ESLint; either remove makeRow from the test file or add a unit test that uses it
to construct a BucketedRow and exercise the bucketChanges/mergeBuckets flow (for
example call groupRowsByPath on a set of rows including makeRow output and then
assert mergeBuckets produces the expected merged buckets), referencing the
helper name makeRow and the utilities bucketChanges, mergeBuckets and
groupRowsByPath so the linter warning is resolved.
Comment on lines
+41
to
+50
| <button | ||
| className="shrink-0 rounded p-0.5 text-gray-400 hover:bg-gray-200 hover:text-gray-600" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onExpand(change); | ||
| }} | ||
| title="View full details" | ||
| > | ||
| <Maximize2 className="h-3 w-3" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add explicit button semantics for accessibility and form-safety (Line 41).
The icon-only button should provide an explicit accessible name and type="button" to avoid accidental form submission.
Suggested fix
<button
+ type="button"
+ aria-label="View full details"
className="shrink-0 rounded p-0.5 text-gray-400 hover:bg-gray-200 hover:text-gray-600"
onClick={(e) => {
e.stopPropagation();
onExpand(change);
}}
title="View full details"
>📝 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
| <button | |
| className="shrink-0 rounded p-0.5 text-gray-400 hover:bg-gray-200 hover:text-gray-600" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onExpand(change); | |
| }} | |
| title="View full details" | |
| > | |
| <Maximize2 className="h-3 w-3" /> | |
| </button> | |
| <button | |
| type="button" | |
| aria-label="View full details" | |
| className="shrink-0 rounded p-0.5 text-gray-400 hover:bg-gray-200 hover:text-gray-600" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| onExpand(change); | |
| }} | |
| title="View full details" | |
| > | |
| <Maximize2 className="h-3 w-3" /> | |
| </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 `@src/components/Configs/Changes/Swimlane/SeverityTooltip.tsx` around lines 41
- 50, The icon-only control in SeverityTooltip should be made explicitly
accessible and form-safe: update the button in the SeverityTooltip component
(the element that calls onExpand(change) and renders <Maximize2 />) to include
type="button" to prevent accidental form submission and add an accessible name
such as aria-label="View full details" (or aria-labelledby pointing to a visible
label) so screen readers can announce its purpose; keep the existing
e.stopPropagation() and title attribute intact.
Member
|
Member
|
Carry config tags through swimlane rows and render them in the existing config type tooltip, avoiding extra inline space and duplicate tooltip instances. Update the swimlane grouping tests to use explicit path-based fixture data instead of relying on HAR records without paths.
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.
Summary
useGetAllConfigsChangesInfiniteQuery(200/page) so the Graph view no longer silently truncates long ranges, and switches the Table/Graph toggle to URL-param backed (?view=). Live-tail remains, scoped to Table view.Ported from
feat/config-changes-timeline-view(commitsc81bb876f+b0a7611d8), rebased onto currentorigin/mainwith only the changes the swimlane requires.Test plan
npx tsc --noEmitclean for the swimlane areanpx jest src/components/Configs/Changes/__tests__/ConfigChangesSwimlaneUtils.unit.test.ts— 20/20 passingnpx jest src/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsx— 7/7 passing/catalog/changes, toggle to Graph view via the switch — verify swimlane renders, hierarchical groups expand/collapse, resource column resize handle works (150–500px), legend shows all change typesConfigDetailChangeModalopensfetchNextPagefires, loading indicator visible?view=Graphsurvives reload; back/forward navigation respects toggle stateSummary by CodeRabbit
Release Notes
New Features
Improvements