feat: add scatter-plot timeline view for config changes#2909
Open
feat: add scatter-plot timeline view for config changes#2909
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughAdds a persistent view toggle and a new time-based swimlane visualization for config changes, integrates the toggle into filter bars and pages, adds on-demand change-detail fetching and modal presentation, and includes related utilities, icons, tests, and e2e Playwright setup. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ViewToggle as View Toggle
participant Page as Config Page
participant Swimlane as ConfigChangesSwimlane
participant Table as ConfigChangeTable
participant API as Backend API
participant Modal as ConfigDetailChangeModal
User->>ViewToggle: Toggle view ("Graph" / "Table")
ViewToggle->>Page: Update view state (URL/search param)
alt Graph view selected
Page->>Swimlane: Render swimlane (infinite query)
User->>Swimlane: Click change marker
Swimlane->>Page: onItemClicked(change)
Page->>API: Request change details (useGetConfigChangesById, enabled)
API-->>Page: Return changeDetails
Page->>Modal: Open modal with details
User->>Modal: Close modal
Modal->>Page: Clear selection
else Table view selected
Page->>Table: Render table view (paginated query)
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🧹 Nitpick comments (1)
src/pages/config/details/ConfigDetailsChangesPage.tsx (1)
30-37: Non-null assertions on potentially missing fields.The data transformation uses non-null assertions (
!) forconfig_id,type, andname. If any change object lacks these fields, this could lead to runtime issues when the graph attempts to render.Consider adding defensive checks or filtering out incomplete records:
♻️ Suggested defensive transformation
- const changes = (data?.changes ?? []).map((changes) => ({ - ...changes, - config: { - id: changes.config_id!, - type: changes.type!, - name: changes.name! - } - })); + const changes = (data?.changes ?? []) + .filter((change) => change.config_id && change.type && change.name) + .map((change) => ({ + ...change, + config: { + id: change.config_id!, + type: change.type!, + name: change.name! + } + }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/config/details/ConfigDetailsChangesPage.tsx` around lines 30 - 37, The mapping in ConfigDetailsChangesPage.tsx currently uses non-null assertions on changes.config_id, changes.type, and changes.name inside the const changes = (data?.changes ?? []).map(...) block; replace this with a defensive transformation that filters out or skips entries missing any of those fields (or provide safe defaults) before mapping so the produced objects always have a valid config.id, config.type and config.name; specifically, update the pipeline that builds changes to first filter items where config_id, type, and name are present (or handle undefined by assigning fallback values) and then map to the desired shape to avoid runtime errors when rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 132: The package.json dependency entry for "reaviz" references a
non-existent version ("16.1.2"); update the dependency value to a released
version (change the "reaviz" entry from "^16.1.2" to "^16.1.1") so npm install
can resolve the package successfully, then run npm install to verify.
In `@src/components/Configs/Changes/ConfigChangesGraph.tsx`:
- Around line 33-39: The mapping in the useMemo for const data
(ChartShallowDataShape[]) unsafely assumes change.first_observed and
change.config.name exist; replace the non-null assertion and implicit
dayjs(undefined) with defensive handling: compute a safe key (e.g., const key =
change.first_observed ? dayjs(change.first_observed).toDate() : null or
skip/filter the change if a date is required by the chart) and compute a safe
label (e.g., const data = change.config?.name ?? 'Unknown config' or empty
string) instead of change.config?.name!; update the useMemo mapping to use these
safe values (or filter out entries missing first_observed) so
ConfigChangesGraph’s data array never relies on dayjs(undefined) or non-null
assertions.
---
Nitpick comments:
In `@src/pages/config/details/ConfigDetailsChangesPage.tsx`:
- Around line 30-37: The mapping in ConfigDetailsChangesPage.tsx currently uses
non-null assertions on changes.config_id, changes.type, and changes.name inside
the const changes = (data?.changes ?? []).map(...) block; replace this with a
defensive transformation that filters out or skips entries missing any of those
fields (or provide safe defaults) before mapping so the produced objects always
have a valid config.id, config.type and config.name; specifically, update the
pipeline that builds changes to first filter items where config_id, type, and
name are present (or handle undefined by assigning fallback values) and then map
to the desired shape to avoid runtime errors when rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75e5de0f-879a-44b6-af5b-03ea1ec07d00
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/components/Configs/Changes/ConfigChangesGraph.tsxsrc/components/Configs/Changes/ConfigChangesViewToggle.tsxsrc/components/Configs/Changes/ConfigsRelatedChanges/FilterBar/ConfigRelatedChangesFilters.tsxsrc/pages/config/details/ConfigDetailsChangesPage.tsx
package.json
Outdated
| "react-use-size": "^3.0.3", | ||
| "react-windowed-select": "^5.2.0", | ||
| "reactflow": "^11.11.3", | ||
| "reaviz": "^16.1.2", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
reaviz latest version npm 2026
💡 Result:
As of March 9, 2026, the latest reaviz version on npm is 16.1.1. [1], [2]
Install (latest):
npm i reaviz@latest
# or explicitly
npm i reaviz@16.1.1Update reaviz version to a released release.
The specified version 16.1.2 does not exist on npm. As of March 2026, the latest available version is 16.1.1. Change to "reaviz": "^16.1.1" to ensure npm install succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 132, The package.json dependency entry for "reaviz"
references a non-existent version ("16.1.2"); update the dependency value to a
released version (change the "reaviz" entry from "^16.1.2" to "^16.1.1") so npm
install can resolve the package successfully, then run npm install to verify.
Comment on lines
+33
to
+39
| const data: ChartShallowDataShape[] = useMemo(() => { | ||
| return changes.map((change) => ({ | ||
| key: dayjs(change.first_observed).toDate(), | ||
| data: change.config?.name!, | ||
| metadata: change | ||
| })); | ||
| }, [changes]); |
Contributor
There was a problem hiding this comment.
Critical: Unsafe handling of optional fields in data transformation.
The ConfigChange interface marks first_observed and config as optional fields. This code has two issues:
- Line 36: Non-null assertion after optional chaining (
change.config?.name!) is problematic—ifconfigisundefined, this evaluates toundefined, not a guaranteed string. - Line 35:
dayjs(undefined).toDate()returns current date, causing incorrect positioning for changes withoutfirst_observed.
🐛 Proposed fix with defensive handling
const data: ChartShallowDataShape[] = useMemo(() => {
- return changes.map((change) => ({
- key: dayjs(change.first_observed).toDate(),
- data: change.config?.name!,
- metadata: change
- }));
+ return changes
+ .filter((change) => change.first_observed && change.config?.name)
+ .map((change) => ({
+ key: dayjs(change.first_observed).toDate(),
+ data: change.config!.name!,
+ metadata: change
+ }));
}, [changes]);📝 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
| const data: ChartShallowDataShape[] = useMemo(() => { | |
| return changes.map((change) => ({ | |
| key: dayjs(change.first_observed).toDate(), | |
| data: change.config?.name!, | |
| metadata: change | |
| })); | |
| }, [changes]); | |
| const data: ChartShallowDataShape[] = useMemo(() => { | |
| return changes | |
| .filter((change) => change.first_observed && change.config?.name) | |
| .map((change) => ({ | |
| key: dayjs(change.first_observed).toDate(), | |
| data: change.config!.name!, | |
| metadata: change | |
| })); | |
| }, [changes]); |
🧰 Tools
🪛 Biome (2.4.4)
[error] 36-36: Forbidden non-null assertion after optional chaining.
(lint/suspicious/noNonNullAssertedOptionalChain)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Changes/ConfigChangesGraph.tsx` around lines 33 - 39,
The mapping in the useMemo for const data (ChartShallowDataShape[]) unsafely
assumes change.first_observed and change.config.name exist; replace the non-null
assertion and implicit dayjs(undefined) with defensive handling: compute a safe
key (e.g., const key = change.first_observed ?
dayjs(change.first_observed).toDate() : null or skip/filter the change if a date
is required by the chart) and compute a safe label (e.g., const data =
change.config?.name ?? 'Unknown config' or empty string) instead of
change.config?.name!; update the useMemo mapping to use these safe values (or
filter out entries missing first_observed) so ConfigChangesGraph’s data array
never relies on dayjs(undefined) or non-null assertions.
Add a Graph/Table toggle to the config changes view using reaviz ScatterPlot. Changes are plotted by time (x-axis) and config name (y-axis) with tooltips showing change details. Toggle state persists in localStorage via jotai atomWithStorage.
moshloop
force-pushed
the
feat/config-changes-timeline-view
branch
from
6e5c7ea to
9d19422
Compare
- Widen resource column to 300px with draggable resize handle (150-500px) - Sticky header row and label column for scroll stability - Use created_at for marker positioning instead of first_observed - Bucket-based anti-overlap using flex-wrap within time buckets - First-observed indicator: gray dot for in-range, ↩ badge for pre-range
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Configs/Changes/ConfigDetailsChanges/ConfigChangeDetailsSection.tsx (1)
3-10:⚠️ Potential issue | 🟡 MinorUnused
labelprop.The
labelprop is defined as required in the type (line 4) but is never destructured or used in the component. Callers (e.g.,<ConfigChangeDetailSection label="Details">) pass it, but it's silently ignored.Either remove
labelfrom the props type and update callers, or implement its usage if it should be rendered.🧹 Proposed fix (if label is not needed)
-type ConfigChangeDetailSectionProps = { - label: string; - children: React.ReactNode; -}; +type ConfigChangeDetailSectionProps = { + children: React.ReactNode; +}; export default function ConfigChangeDetailSection({ children }: ConfigChangeDetailSectionProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Configs/Changes/ConfigDetailsChanges/ConfigChangeDetailsSection.tsx` around lines 3 - 10, The prop "label" declared on ConfigChangeDetailSectionProps is unused; update the ConfigChangeDetailSection component to destructure the label (in addition to children) and render it in the component's JSX (e.g., as a heading or label element before the children) so callers' passed labels are actually displayed; keep the ConfigChangeDetailSectionProps type as-is and reference the symbol names ConfigChangeDetailSection and ConfigChangeDetailSectionProps when making the change.
🧹 Nitpick comments (1)
src/components/Configs/Changes/ConfigDetailsChanges/ConfigChangeDetailsSection.tsx (1)
14-17: Width constraint looks good; consider removing unnecessaryclsx.The
max-w-[100ch]constraint is appropriate for JSON/diff content readability.However,
clsxis designed for conditional class composition. Since this is a single static string, you can simplify by removingclsx:✨ Suggested simplification
-import clsx from "clsx"; - type ConfigChangeDetailSectionProps = { label: string; children: React.ReactNode; };<div - className={clsx( - "flex max-w-[100ch] flex-1 overflow-x-auto overflow-y-auto rounded border border-gray-200 text-sm" - )} + className="flex max-w-[100ch] flex-1 overflow-x-auto overflow-y-auto rounded border border-gray-200 text-sm" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Configs/Changes/ConfigDetailsChanges/ConfigChangeDetailsSection.tsx` around lines 14 - 17, The JSX in ConfigChangeDetailsSection.tsx uses clsx unnecessarily for a single static class string; replace className={clsx("flex max-w-[100ch] flex-1 overflow-x-auto overflow-y-auto rounded border border-gray-200 text-sm")} with a plain string className="flex max-w-[100ch] flex-1 overflow-x-auto overflow-y-auto rounded border border-gray-200 text-sm" and then remove the unused clsx import (if present) to keep imports clean.
🤖 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 157-165: The tooltip uses viewport-relative clientX/clientY when
setting state in the onMouseEnter handler (see containerRef and setTooltip
usage) but is absolutely positioned inside the scrollable swimlane, causing
offsets after scroll; fix by converting coordinates into container-local
coordinates by adding the container's current scrollLeft and scrollTop (or
compute position using e.clientX - rect.left + containerRef.current.scrollLeft
and e.clientY - rect.top + containerRef.current.scrollTop) before calling
setTooltip, or alternatively render/portal the tooltip outside the scroller so
it uses viewport coordinates consistent with clientX/clientY (update the same
onMouseEnter/onMouseMove handlers and the tooltip render logic that reads
tooltip.x/tooltip.y).
- Around line 69-95: The grouping uses config.name as the Map key which
collapses distinct configs with the same display name; change the grouping key
to the stable identifier (config_id or a composite like
`${c.config_id}:${c.config?.name}`) when building grouped in the useMemo (the
loop that creates grouped and later produces rows), keep the human-readable name
in the row object (rows.map items use name for display only), and ensure any
React key usage (e.g. key={row.name} around the swimlane items) is switched to
the stable id (e.g. key={row.config_id || row.config?.id ||
`${row.config?.id}:${row.name}`}) so keys and grouping match; leave
generateTimeTicks(min, max) and other time calculations unchanged.
In `@src/pages/config/ConfigChangesPage.tsx`:
- Around line 115-140: The Graph view in ConfigChangesPage is showing only the
current paginated page because useGetAllConfigsChangesQuery returns page-scoped
results (changes) but ConfigChangesSwimlane is not given pagination info; update
the ConfigChangesPage component to pass numberOfPages (totalChangesPages) and
totalRecords (totalChanges) into ConfigChangesSwimlane (and mirror the same fix
in ConfigDetailsChangesPage.tsx), or alternatively detect view === "Graph" and
call the query without page params (fetch all results) before rendering
ConfigChangesSwimlane so the swimlane has either full data or explicit
pagination controls to navigate pages; ensure props and handlers align with
ConfigChangesSwimlane’s interface so navigation actions update
pageIndex/pageSize via the existing URL search param logic used by
useGetAllConfigsChangesQuery.
In `@src/pages/config/details/ConfigDetailsChangesPage.tsx`:
- Around line 42-46: The query for change details is only gated by
selectedChange, so when the user leaves the Graph view the hidden modal still
triggers useGetConfigChangesById and the stale selection reappears later; fix by
either adding the view check to the query's enabled flag (e.g., enabled:
!!selectedChange && view === "Graph") or by clearing selectedChange when view
changes (call setSelectedChange(undefined) inside the view-change handler or
useEffect watching view), and apply the same change to the analogous block
around the code at lines 74-83 to ensure no hidden fetches occur when not in
Graph view.
---
Outside diff comments:
In
`@src/components/Configs/Changes/ConfigDetailsChanges/ConfigChangeDetailsSection.tsx`:
- Around line 3-10: The prop "label" declared on ConfigChangeDetailSectionProps
is unused; update the ConfigChangeDetailSection component to destructure the
label (in addition to children) and render it in the component's JSX (e.g., as a
heading or label element before the children) so callers' passed labels are
actually displayed; keep the ConfigChangeDetailSectionProps type as-is and
reference the symbol names ConfigChangeDetailSection and
ConfigChangeDetailSectionProps when making the change.
---
Nitpick comments:
In
`@src/components/Configs/Changes/ConfigDetailsChanges/ConfigChangeDetailsSection.tsx`:
- Around line 14-17: The JSX in ConfigChangeDetailsSection.tsx uses clsx
unnecessarily for a single static class string; replace className={clsx("flex
max-w-[100ch] flex-1 overflow-x-auto overflow-y-auto rounded border
border-gray-200 text-sm")} with a plain string className="flex max-w-[100ch]
flex-1 overflow-x-auto overflow-y-auto rounded border border-gray-200 text-sm"
and then remove the unused clsx import (if present) to keep imports clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdf2997f-2f5b-4371-997e-ac80744e5d86
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesFilters.tsxsrc/components/Configs/Changes/ConfigChangesSwimlane.tsxsrc/components/Configs/Changes/ConfigChangesViewToggle.tsxsrc/components/Configs/Changes/ConfigDetailsChanges/ConfigChangeDetailsSection.tsxsrc/components/Configs/Changes/ConfigsRelatedChanges/FilterBar/ConfigRelatedChangesFilters.tsxsrc/pages/config/ConfigChangesPage.tsxsrc/pages/config/details/ConfigDetailsChangesPage.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Configs/Changes/ConfigsRelatedChanges/FilterBar/ConfigRelatedChangesFilters.tsx
- src/components/Configs/Changes/ConfigChangesViewToggle.tsx
Comment on lines
+69
to
+95
| const { rows, min, max, ticks } = useMemo(() => { | ||
| const grouped = new Map<string, ConfigChange[]>(); | ||
| for (const c of changes) { | ||
| const key = c.config?.name ?? c.config_id ?? "unknown"; | ||
| if (!grouped.has(key)) grouped.set(key, []); | ||
| grouped.get(key)!.push(c); | ||
| } | ||
|
|
||
| let min = Infinity; | ||
| let max = -Infinity; | ||
| for (const c of changes) { | ||
| const t = dayjs(c.first_observed).valueOf(); | ||
| if (t < min) min = t; | ||
| if (t > max) max = t; | ||
| } | ||
|
|
||
| if (min === Infinity) { | ||
| min = max = Date.now(); | ||
| } | ||
|
|
||
| const rows = Array.from(grouped.entries()).map(([name, items]) => ({ | ||
| name, | ||
| config: items[0]!.config, | ||
| items | ||
| })); | ||
|
|
||
| return { rows, min, max, ticks: generateTimeTicks(min, max) }; |
Contributor
There was a problem hiding this comment.
Group lanes by a stable config identifier.
Line 72 uses config.name as the map key. If two distinct configs share a display name, their events collapse into one lane and key={row.name} on Line 136 stops being unique. Use config_id (or another stable composite key) for grouping/keying and keep name only for display.
💡 Suggested fix
- const grouped = new Map<string, ConfigChange[]>();
+ const grouped = new Map<
+ string,
+ { name: string; items: ConfigChange[] }
+ >();
for (const c of changes) {
- const key = c.config?.name ?? c.config_id ?? "unknown";
- if (!grouped.has(key)) grouped.set(key, []);
- grouped.get(key)!.push(c);
+ const key =
+ c.config_id ??
+ `${c.config?.type ?? "unknown"}:${c.config?.name ?? "unknown"}`;
+ if (!grouped.has(key)) {
+ grouped.set(key, {
+ name: c.config?.name ?? c.config_id ?? "unknown",
+ items: []
+ });
+ }
+ grouped.get(key)!.items.push(c);
}
@@
- const rows = Array.from(grouped.entries()).map(([name, items]) => ({
- name,
- config: items[0]!.config,
- items
+ const rows = Array.from(grouped.entries()).map(([key, row]) => ({
+ key,
+ name: row.name,
+ config: row.items[0]!.config,
+ items: row.items
}));
@@
- key={row.name}
+ key={row.key}Also applies to: 134-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 69 -
95, The grouping uses config.name as the Map key which collapses distinct
configs with the same display name; change the grouping key to the stable
identifier (config_id or a composite like `${c.config_id}:${c.config?.name}`)
when building grouped in the useMemo (the loop that creates grouped and later
produces rows), keep the human-readable name in the row object (rows.map items
use name for display only), and ensure any React key usage (e.g. key={row.name}
around the swimlane items) is switched to the stable id (e.g. key={row.config_id
|| row.config?.id || `${row.config?.id}:${row.name}`}) so keys and grouping
match; leave generateTimeTicks(min, max) and other time calculations unchanged.
Comment on lines
+152
to
+170
| <button | ||
| key={change.id} | ||
| className="absolute top-1/2 -translate-x-1/2 -translate-y-1/2 cursor-pointer p-0.5 transition-transform hover:scale-125" | ||
| style={{ left: `${pct}%` }} | ||
| onClick={() => onItemClicked(change)} | ||
| onMouseEnter={(e) => { | ||
| const rect = | ||
| containerRef.current?.getBoundingClientRect(); | ||
| if (!rect) return; | ||
| setTooltip({ | ||
| change, | ||
| x: e.clientX - rect.left, | ||
| y: e.clientY - rect.top | ||
| }); | ||
| }} | ||
| onMouseLeave={() => setTooltip(null)} | ||
| > | ||
| <ChangeIcon change={change} className="h-4 w-4" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible label to each graph marker.
These buttons only render ChangeIcon, so assistive tech currently gets a row of unlabeled controls. Add an aria-label; if you want parity with mouse users, also expose the same details on focus/blur.
💡 Suggested fix
<button
key={change.id}
+ type="button"
+ aria-label={[
+ change.config?.name ?? "Unknown config",
+ change.change_type ?? "change"
+ ].join(" • ")}
className="absolute top-1/2 -translate-x-1/2 -translate-y-1/2 cursor-pointer p-0.5 transition-transform hover:scale-125"
style={{ left: `${pct}%` }}
Comment on lines
+115
to
+140
| {view === "Graph" ? ( | ||
| <> | ||
| <ConfigChangesSwimlane | ||
| changes={changes} | ||
| onItemClicked={(change) => setSelectedChange(change)} | ||
| /> | ||
| {selectedChange && ( | ||
| <ConfigDetailChangeModal | ||
| isLoading={changeLoading} | ||
| open={!!selectedChange} | ||
| setOpen={(open) => { | ||
| if (!open) setSelectedChange(undefined); | ||
| }} | ||
| changeDetails={changeDetails ?? selectedChange} | ||
| /> | ||
| )} | ||
| </> | ||
| ) : ( | ||
| <ConfigChangeTable | ||
| data={changes} | ||
| isLoading={isLoading} | ||
| isRefetching={isRefetching} | ||
| totalRecords={totalChanges} | ||
| numberOfPages={totalChangesPages} | ||
| /> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== useConfigChangesHooks definitions =="
fd 'useConfigChangesHooks\.ts$' -t f | while read -r file; do
echo "-- $file --"
sed -n '1,260p' "$file"
done
echo
echo "== ConfigChangeTable pagination inputs =="
fd 'ConfigChangeTable\.tsx$' -t f | while read -r file; do
echo "-- $file --"
sed -n '1,260p' "$file"
done
echo
echo "== Pagination-related usage around config changes =="
rg -n -C3 'useGetAllConfigsChangesQuery|useGetConfigChangesByIDQuery|pageSize|numberOfPages|totalRecords|\bpage\b' srcRepository: flanksource/flanksource-ui
Length of output: 50383
🏁 Script executed:
fd 'ConfigChangesPage\.tsx$' -type f | head -5Repository: flanksource/flanksource-ui
Length of output: 240
🏁 Script executed:
fd 'ConfigChangesPage\.tsx$' -type fileRepository: flanksource/flanksource-ui
Length of output: 240
🏁 Script executed:
fd 'ConfigChangesPage\.tsx'Repository: flanksource/flanksource-ui
Length of output: 108
🏁 Script executed:
wc -l src/pages/config/ConfigChangesPage.tsxRepository: flanksource/flanksource-ui
Length of output: 112
🏁 Script executed:
cat -n src/pages/config/ConfigChangesPage.tsxRepository: flanksource/flanksource-ui
Length of output: 6622
🏁 Script executed:
# Check if ConfigChangesSwimlane has any pagination capability
grep -r "ConfigChangesSwimlane" src --include="*.tsx" -A 10 | head -60Repository: flanksource/flanksource-ui
Length of output: 5551
🏁 Script executed:
# Check useReactTablePaginationState to understand how pageIndex/pageSize params work
cat -n src/ui/DataTable/Hooks/useReactTablePaginationState.tsx | head -85Repository: flanksource/flanksource-ui
Length of output: 2810
🏁 Script executed:
# Verify ConfigDetailsChangesPage also has the same pattern
sed -n '1,100p' src/pages/config/details/ConfigDetailsChangesPage.tsx | grep -A 30 "view === \"Graph\""Repository: flanksource/flanksource-ui
Length of output: 1032
Graph view silently hides paginated data with no navigation UI.
The useGetAllConfigsChangesQuery hook is page-scoped—it extracts pageIndex and pageSize from URL search params and passes them to the API. This means changes contains only the current page's results, not the full filtered set. While totalChangesPages is correctly calculated (line 62), it's passed only to the Table branch (line 138). The Graph branch renders ConfigChangesSwimlane with no pagination controls, so users cannot navigate to pages 2+, effectively hiding them. The same issue exists in ConfigDetailsChangesPage.tsx.
Pass numberOfPages and totalRecords to the Graph view as well, or fetch all results without pagination for visualization views.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/config/ConfigChangesPage.tsx` around lines 115 - 140, The Graph
view in ConfigChangesPage is showing only the current paginated page because
useGetAllConfigsChangesQuery returns page-scoped results (changes) but
ConfigChangesSwimlane is not given pagination info; update the ConfigChangesPage
component to pass numberOfPages (totalChangesPages) and totalRecords
(totalChanges) into ConfigChangesSwimlane (and mirror the same fix in
ConfigDetailsChangesPage.tsx), or alternatively detect view === "Graph" and call
the query without page params (fetch all results) before rendering
ConfigChangesSwimlane so the swimlane has either full data or explicit
pagination controls to navigate pages; ensure props and handlers align with
ConfigChangesSwimlane’s interface so navigation actions update
pageIndex/pageSize via the existing URL search param logic used by
useGetAllConfigsChangesQuery.
Comment on lines
+42
to
+46
| const [selectedChange, setSelectedChange] = useState<ConfigChange>(); | ||
| const { data: changeDetails, isLoading: changeLoading } = | ||
| useGetConfigChangesById(selectedChange?.id ?? "", { | ||
| enabled: !!selectedChange | ||
| }); |
Contributor
There was a problem hiding this comment.
Clear the graph selection when leaving Graph view.
The detail query on Lines 43-46 is gated only by selectedChange. If a modal is open and the user flips back to Table, the page keeps fetching details for a hidden modal, and the stale selection reappears when Graph is reopened. Gate the query by view === "Graph" or reset selectedChange when view changes.
💡 Suggested fix
-import { useState } from "react";
+import { useEffect, useState } from "react";
@@
const [selectedChange, setSelectedChange] = useState<ConfigChange>();
const { data: changeDetails, isLoading: changeLoading } =
useGetConfigChangesById(selectedChange?.id ?? "", {
- enabled: !!selectedChange
+ enabled: view === "Graph" && !!selectedChange
});
+
+ useEffect(() => {
+ if (view !== "Graph") {
+ setSelectedChange(undefined);
+ }
+ }, [view]);Also applies to: 74-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/config/details/ConfigDetailsChangesPage.tsx` around lines 42 - 46,
The query for change details is only gated by selectedChange, so when the user
leaves the Graph view the hidden modal still triggers useGetConfigChangesById
and the stale selection reappears later; fix by either adding the view check to
the query's enabled flag (e.g., enabled: !!selectedChange && view === "Graph")
or by clearing selectedChange when view changes (call
setSelectedChange(undefined) inside the view-change handler or useEffect
watching view), and apply the same change to the analogous block around the code
at lines 74-83 to ensure no hidden fetches occur when not in Graph view.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/components/Configs/Changes/ConfigChangesSwimlane.tsx (3)
307-323:⚠️ Potential issue | 🟠 MajorAdd an accessible name to each marker button.
These controls only render
ChangeIcon, so assistive tech gets a row of unlabeled buttons. Add anaria-label;type="button"is also safer if this ever sits inside a form.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 307 - 323, The marker button rendering the ChangeIcon lacks an accessible name and an explicit button type; update the button in ConfigChangesSwimlane (the element that uses onItemClicked(change), ChangeIcon, containerRef and setTooltip) to include a descriptive aria-label (e.g., based on the change object/id or change.type) and add type="button" to prevent form submission when nested in a form; ensure the aria-label text is meaningful and unique enough to identify the specific change represented by that marker.
184-188:⚠️ Potential issue | 🟠 MajorGroup lanes and React keys by a stable config identifier.
This still uses
config.nameas the grouping key, so two distinct configs with the same display name collapse into one lane andkey={row.name}stays non-unique. Group byconfig_id(or a stable composite) and keepnamefor display only.Also applies to: 203-212, 254-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 184 - 188, The grouping and React key logic in ConfigChangesSwimlane is using config.name (via the local variable key and key={row.name}), which collapses distinct configs with the same display name; change the grouping to use the stable identifier config_id (or a composite like `${config_id}:${name}`) when creating the Map (the grouped variable and its key assignment) and update any JSX key props (e.g., key={row.name}) to use the stable id (e.g., key={row.config_id} or the composite) while still rendering name for display only; apply the same change to the other grouping/render spots in this component (the blocks corresponding to the other occurrences you noted).
310-318:⚠️ Potential issue | 🟠 MajorAccount for scroll when placing the tooltip.
The stored coordinates are viewport-relative, but the tooltip is absolutely positioned inside the scroll container. After scrolling, hover state renders offset from the marker. Add
scrollLeft/scrollTop, or portal the tooltip outside the scroller.Also applies to: 335-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 310 - 318, The tooltip coordinates are computed from viewport mouse positions but the container is scrollable, causing offsets after scrolling; in the onMouseEnter handlers that call setTooltip (referencing containerRef, setTooltip, and change) add the container's scroll offsets when calculating x and y (e.g. x = e.clientX - rect.left + containerRef.current.scrollLeft and y = e.clientY - rect.top + containerRef.current.scrollTop) so the absolute-positioned tooltip inside the scroller aligns with the marker; apply the same fix to the other occurrence (lines referencing setTooltip at 335-338) or alternatively portal the tooltip outside the scroller if preferred.
🤖 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 276-286: Rows with row.preRangeBadge are currently flex siblings
of the bucket columns (rendered alongside row.buckets with style width: `${100 /
numBuckets}%`) which shrinks the plotted bucket width; change the layout so the
badge does not consume bucket width by either (a) rendering the badge as an
absolutely positioned overlay inside the row container (keep the bucket
container full width based on numBuckets and position the badge with CSS
absolute/left offset), or (b) reserve a fixed gutter for the badge in both the
header and every row by adding a dedicated badge gutter element (same fixed
pixel width) before the bucket map so the bucket columns and header share
identical plotting width; update the JSX around row.preRangeBadge and the bucket
container where numBuckets is used (and corresponding CSS) to implement one of
these approaches.
- Around line 47-53: The tooltip's primary age uses change.first_observed but
the plotted point uses change.created_at; update the first Age component to use
change.created_at (e.g., change the Age from={change.first_observed} to
from={change.created_at}) and keep the suffix Age showing the older
first_observed (i.e., keep the nested Age from={change.first_observed});
consider falling back to first_observed if created_at is missing (Age
from={change.created_at || change.first_observed}).
---
Duplicate comments:
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx`:
- Around line 307-323: The marker button rendering the ChangeIcon lacks an
accessible name and an explicit button type; update the button in
ConfigChangesSwimlane (the element that uses onItemClicked(change), ChangeIcon,
containerRef and setTooltip) to include a descriptive aria-label (e.g., based on
the change object/id or change.type) and add type="button" to prevent form
submission when nested in a form; ensure the aria-label text is meaningful and
unique enough to identify the specific change represented by that marker.
- Around line 184-188: The grouping and React key logic in ConfigChangesSwimlane
is using config.name (via the local variable key and key={row.name}), which
collapses distinct configs with the same display name; change the grouping to
use the stable identifier config_id (or a composite like `${config_id}:${name}`)
when creating the Map (the grouped variable and its key assignment) and update
any JSX key props (e.g., key={row.name}) to use the stable id (e.g.,
key={row.config_id} or the composite) while still rendering name for display
only; apply the same change to the other grouping/render spots in this component
(the blocks corresponding to the other occurrences you noted).
- Around line 310-318: The tooltip coordinates are computed from viewport mouse
positions but the container is scrollable, causing offsets after scrolling; in
the onMouseEnter handlers that call setTooltip (referencing containerRef,
setTooltip, and change) add the container's scroll offsets when calculating x
and y (e.g. x = e.clientX - rect.left + containerRef.current.scrollLeft and y =
e.clientY - rect.top + containerRef.current.scrollTop) so the
absolute-positioned tooltip inside the scroller aligns with the marker; apply
the same fix to the other occurrence (lines referencing setTooltip at 335-338)
or alternatively portal the tooltip outside the scroller if preferred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c75449af-19b7-4447-b0bc-72c6025af1e0
📒 Files selected for processing (1)
src/components/Configs/Changes/ConfigChangesSwimlane.tsx
Comment on lines
+47
to
+53
| <span className="font-semibold"> | ||
| <Age from={change.first_observed} /> | ||
| {(change.count || 1) > 1 && ( | ||
| <span className="inline-block pl-1 text-gray-500"> | ||
| (x{change.count} over <Age from={change.first_observed} />) | ||
| </span> | ||
| )} |
Contributor
There was a problem hiding this comment.
Use created_at for the primary tooltip age.
The point is plotted from created_at, but the tooltip currently shows first_observed as the main age and then repeats that same timestamp in the xN over … suffix. That makes repeated changes read older than the point’s actual position.
Suggested fix
- <span className="font-semibold">
- <Age from={change.first_observed} />
- {(change.count || 1) > 1 && (
+ <span className="font-semibold">
+ <Age from={change.created_at} />
+ {(change.count || 1) > 1 && change.first_observed && (
<span className="inline-block pl-1 text-gray-500">
(x{change.count} over <Age from={change.first_observed} />)
</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
| <span className="font-semibold"> | |
| <Age from={change.first_observed} /> | |
| {(change.count || 1) > 1 && ( | |
| <span className="inline-block pl-1 text-gray-500"> | |
| (x{change.count} over <Age from={change.first_observed} />) | |
| </span> | |
| )} | |
| <span className="font-semibold"> | |
| <Age from={change.created_at} /> | |
| {(change.count || 1) > 1 && change.first_observed && ( | |
| <span className="inline-block pl-1 text-gray-500"> | |
| (x{change.count} over <Age from={change.first_observed} />) | |
| </span> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 47 -
53, The tooltip's primary age uses change.first_observed but the plotted point
uses change.created_at; update the first Age component to use change.created_at
(e.g., change the Age from={change.first_observed} to from={change.created_at})
and keep the suffix Age showing the older first_observed (i.e., keep the nested
Age from={change.first_observed}); consider falling back to first_observed if
created_at is missing (Age from={change.created_at || change.first_observed}).
Comment on lines
+276
to
+286
| <div className="flex flex-1 flex-row items-stretch"> | ||
| {row.preRangeBadge && ( | ||
| <span className="flex items-center px-1 text-[10px] text-gray-400"> | ||
| {row.preRangeBadge} | ||
| </span> | ||
| )} | ||
| {row.buckets.map((bucket, bIdx) => ( | ||
| <div | ||
| key={bIdx} | ||
| className="inline-flex flex-wrap items-center gap-0.5 py-0.5" | ||
| style={{ width: `${100 / numBuckets}%` }} |
Contributor
There was a problem hiding this comment.
Keep the pre-range badge out of the plotting width.
row.preRangeBadge is a flex sibling of the bucket columns, so any row that has a badge gets a narrower timeline than the header. Those rows no longer align with the shared x-axis. Render the badge as an overlay, or reserve the same gutter in both the header and every row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Changes/ConfigChangesSwimlane.tsx` around lines 276 -
286, Rows with row.preRangeBadge are currently flex siblings of the bucket
columns (rendered alongside row.buckets with style width: `${100 /
numBuckets}%`) which shrinks the plotted bucket width; change the layout so the
badge does not consume bucket width by either (a) rendering the badge as an
absolutely positioned overlay inside the row container (keep the bucket
container full width based on numBuckets and position the badge with CSS
absolute/left offset), or (b) reserve a fixed gutter for the badge in both the
header and every row by adding a dedicated badge gutter element (same fixed
pixel width) before the bucket map so the bucket columns and header share
identical plotting width; update the JSX around row.preRangeBadge and the bucket
container where numBuckets is used (and corresponding CSS) to implement one of
these approaches.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
src/components/ui/hover-card.tsx (1)
14-25: Keep the shadcn hover-card upstream and wrap it instead.This works, but it edits an upstream
src/components/ui/*primitive directly. Please move the portalized variant into a project-specific wrapper/re-export so future shadcn syncs stay clean.Based on learnings: Do not modify shadcn/ui components themselves; keep them in their original form and prefer wrappers/composition to simplify upgrades.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/hover-card.tsx` around lines 14 - 25, The current change modifies the upstream primitive by adding a portalized variant; instead create a new project-specific wrapper that composes HoverCardPrimitive.Portal and HoverCardPrimitive.Content (preserving ref forwarding, props spread, align, sideOffset and the cn className merge) instead of editing the original shadcn component; restore the upstream HoverCardPrimitive.Content to its original form, add a new component (e.g., PortalizedHoverCard or HoverCardPortal) that returns HoverCardPrimitive.Portal > HoverCardPrimitive.Content with the same className string and {...props}, export that wrapper for use across the app, and update imports to use the new wrapper where the portalized behavior is needed.src/api/query-hooks/useConfigChangesHooks.ts (1)
173-205: Extract the shared filter builder before table and graph drift apart.This block is almost a copy of
useGetAllConfigsChangesQuery. The next filter addition/fix will be easy to miss in one path, which means the table and graph can silently query different datasets.♻️ Suggested direction
+function useAllConfigChangesFilters(paramPrefix?: string) { + const showChangesFromDeletedConfigs = useShowDeletedConfigs(); + const { timeRangeValue } = useTimeRangeParams( + configChangesDefaultDateFilter, + paramPrefix + ); + const [params] = usePrefixedSearchParams(paramPrefix, false, { + sortBy: "created_at", + sortDirection: "desc" + }); + const [sortBy] = useReactTableSortState({ paramPrefix }); + const tags = useConfigChangesTagsFilter(paramPrefix); + const arbitraryFilter = useConfigChangesArbitraryFilters(paramPrefix); + + return { + include_deleted_configs: showChangesFromDeletedConfigs, + changeType: params.get("changeType") ?? undefined, + severity: params.get("severity") ?? undefined, + from: timeRangeValue?.from ?? undefined, + to: timeRangeValue?.to ?? undefined, + configTypes: params.get("configTypes") ?? "all", + configType: params.get("configType") ?? undefined, + sortBy: sortBy[0]?.id, + sortOrder: sortBy[0]?.desc ? "desc" : "asc", + tags, + arbitraryFilter + } satisfies Omit<GetConfigsRelatedChangesParams, "pageIndex" | "pageSize">; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/query-hooks/useConfigChangesHooks.ts` around lines 173 - 205, Extract the repeated filter construction into a shared helper function (e.g., buildConfigChangesFilter) and use it from both useConfigChangesHooks and useGetAllConfigsChangesQuery so the table and graph always use the same filters; move the logic that creates filterProps (all usages of showChangesFromDeletedConfigs, timeRangeValue/from/to, params.get for changeType/severity/configType/configTypes, useReactTableSortState sortBy/sortOrder, pageSize, arbitraryFilter, and tags) into that helper and have both call it to return a single canonical filter object instead of duplicating the block that currently creates filterProps.e2e/tests/config-changes-swimlane.spec.ts (2)
14-23: Redundant button click after URL navigation.
gotoGraphViewnavigates directly to?view=Graphbut then clicks the "Graph" button. The click may be intentional to ensure UI state synchronization, but if the URL param already sets the view, the click could be removed.♻️ Suggested simplification
async function gotoGraphView(page: Page) { await page.goto("/catalog/changes?view=Graph"); await expect( page.locator("li").filter({ hasText: "Catalog" }).getByRole("link") ).toBeVisible({ timeout: 30000 }); - await page.getByRole("button", { name: "Graph" }).click(); await expect(page.locator(ROW_SEL).first()).toBeVisible({ timeout: 30000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/config-changes-swimlane.spec.ts` around lines 14 - 23, The function gotoGraphView contains a redundant click of the "Graph" button after navigating to "/catalog/changes?view=Graph"; remove the line that calls page.getByRole("button", { name: "Graph" }).click() and rely on the URL param to select the view, but keep the existing waits (the Catalog link visibility check and await expect(page.locator(ROW_SEL).first()).toBeVisible({ timeout: 30000 })) to ensure the UI has settled; update gotoGraphView accordingly so it only navigates, waits for the Catalog link, and then waits for ROW_SEL to be visible.
1-5: Consider removing credential management instructions from source.The 1Password CLI instructions with vault item references could be moved to a separate
CONTRIBUTING.mdore2e/README.mdfile to keep test files focused on test logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/config-changes-swimlane.spec.ts` around lines 1 - 5, Remove the 1Password CLI credential block (the commented "With 1Password CLI: eval $(op signin) && ..." snippet) from the test file so the test (config-changes-swimlane.spec.ts) contains only test logic, and move those instructions into a documentation file such as e2e/README.md or CONTRIBUTING.md; ensure the moved instructions avoid embedding vault/item identifiers or secrets and instead give a high-level guide or link to internal secret-management docs.src/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsx (1)
54-63: Brittle assertion on exact change type count.The assertion
expect(expectedTypes.size).toBe(34)is tightly coupled to the specific HAR fixture data. If the fixture changes or is updated, this test will fail even if the legend functionality works correctly.♻️ Suggested improvement
it("legend shows all unique change types", () => { renderSwimlane({ changes: allChanges }); const expectedTypes = new Set(allChanges.map((c) => c.change_type)); - expect(expectedTypes.size).toBe(34); + // Verify we have a reasonable number of change types from the fixture + expect(expectedTypes.size).toBeGreaterThan(0); for (const type of ["diff", "Pulling", "OOMKilled", "Healthy", "Sync"]) { expect(screen.getAllByText(type).length).toBeGreaterThan(0); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsx` around lines 54 - 63, The test uses a brittle exact count assertion (expect(expectedTypes.size).toBe(34)) tied to the fixture; update the "legend shows all unique change types" test to avoid hardcoding 34: compute expectedTypes as you already do from allChanges (variable expectedTypes) and replace the toBe(34) assertion with a more resilient check such as expect(expectedTypes.size).toBeGreaterThan(0) or expect(expectedTypes.size).toBeGreaterThanOrEqual(5) (at least the number of specific types you assert later), or instead assert that every value in expectedTypes has a corresponding rendered legend entry by iterating expectedTypes and checking screen.getAllByText(type).length > 0; keep using renderSwimlane and the allChanges fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env:
- Around line 16-20: Remove the actual Clerk keys from the tracked .env by
replacing the values for NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY and CLERK_SECRET_KEY
with clear placeholder strings (e.g.,
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY=placeholder and CLERK_SECRET_KEY=placeholder),
ensure any commented-out real keys are deleted, and add a note to rotate those
credentials outside this PR; update the .env so it contains only non-sensitive
placeholders and commit the change.
- Around line 21-23: Update the sample environment variable names in the .env
file to match the e2e tests: replace USERNAME and PASSWORD with TEST_USERNAME
and TEST_PASSWORD so they align with e2e/tests/user-login-flow.spec.ts (which
expects TEST_USERNAME and TEST_PASSWORD), and ensure TEST_URL remains present as
the test host. Make sure the commented examples use the exact variable names
TEST_URL, TEST_USERNAME, and TEST_PASSWORD so copying them will authenticate the
login flow.
In `@e2e/playwright.config.ts`:
- Around line 35-48: The Playwright config currently hardcodes channel: "chrome"
in the browser use blocks (e.g., the unnamed block and the "chromium" block),
which forces a system Chrome instead of the Playwright-managed Chromium used in
CI; update those use objects to either remove the channel property entirely so
Playwright uses the installed browser, or make channel conditional on CI (e.g.,
check process.env.CI) so local runs can use "chrome" but CI uses the installed
Playwright browser; locate the channel entries inside the use: { ... } objects
for the relevant browser configs and change them accordingly.
In `@src/api/query-hooks/useConfigChangesHooks.ts`:
- Around line 211-212: The pagination stop logic in getNextPageParam wrongly
uses lastPage.changes.length which allows one extra empty request when total is
an exact multiple of pageSize; change it to use the backend-provided
lastPage.total to determine whether more pages exist (e.g., compute loaded =
allPages.length * pageSize or sum of fetched items and return undefined when
loaded >= lastPage.total, otherwise return allPages.length) so getNextPageParam
uses lastPage.total and pageSize (and/or a sum of changes lengths) instead of
only lastPage.changes?.length.
In `@src/components/Authentication/Kratos/KratosLogin.tsx`:
- Around line 71-76: The createBrowserLoginFlow call is still receiving a
returnTo value when the original query was missing (because the code normalizes
missing return_to to "/"), causing the forbidden-return_to retry path to run
unnecessarily; update the parameter spread in the async handler (the function
that calls ory.createBrowserLoginFlow) to only include returnTo when it is
defined and not the default "/" (e.g., check returnTo && returnTo !== "/" before
spreading) so the initial request omits returnTo and avoids the bad request
path.
- Around line 106-109: getFlow currently swallows errors by calling
handleError() without returning its result, so callers (like the flowId branch
that does getFlow(flowId).catch(...)) never see a rejection and createFlow is
never invoked; update getFlow (the function that calls handleError) to return
the result/throw the error from handleError so the promise rejects and lets the
caller's .catch trigger, ensuring the fallback in the flowId branch will call
createFlow(refresh, aal, returnTo || undefined).
In `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts`:
- Around line 169-201: The mousemove/mouseup listeners added inside onMouseDown
(handlers onMouseMove and onMouseUp) aren’t removed if the component unmounts
during a drag, causing a leak; update the component to clean them up on unmount
by keeping the handlers (or their references) accessible outside onMouseDown
(e.g., refs for onMouseMove/onMouseUp or refs for a boolean dragging) and add a
useEffect cleanup that removes document.removeEventListener("mousemove",
onMouseMove) and document.removeEventListener("mouseup", onMouseUp) (and clears
dragging.current) so listeners are always removed when the component unmounts or
dependencies change.
- Around line 352-373: The current loop marks every label candidate as seen (and
deletes from deferredTypes) before computing actual placements, which causes
overflowed candidates to be suppressed later; deferredTypes is never populated.
Fix by removing the early seenTypes.add(pos.type)/deferredTypes.delete(pos.type)
block and instead: compute labelCount and pattern, then in the placement loop
determine placement for each candidate (using PLACEMENT_PATTERNS and
labelCandidates), call seenTypes.add(type) only when placement is a real
left/right label, and when there are more candidates than labelCount add those
overflowed types to deferredTypes so they remain available for subsequent
buckets; update logic around candidateIdx, placement, seenTypes, and
deferredTypes accordingly (refer to labelCandidates, iconPositions,
PLACEMENT_PATTERNS, seenTypes, deferredTypes, and LabelPlacement).
- Around line 23-60: countSeverities maps unknown or missing severities to
"none" but filterBySeverity can't select those because SEVERITY_MATCHERS lacks a
"none" entry; add a "none" matcher to SEVERITY_MATCHERS (and keep the same input
normalization) that returns true when the incoming normalized severity is falsy
or not one of the known values (critical/blocker, high, medium/warning, low,
info), so filterBySeverity(changes, "none") returns changes counted as none;
reference SEVERITY_MATCHERS and filterBySeverity to implement this.
In
`@src/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsx`:
- Around line 73-75: The non-null assertion (!) should be removed from the Age
props—Age accepts an optional from, so replace changeDetails?.created_at! with
changeDetails?.created_at in both places where Age is used (the Age component
instances), removing the trailing ! to let the optional type flow correctly and
satisfy the Biome lint rule.
In `@src/pages/config/ConfigChangesPage.tsx`:
- Around line 27-37: The normalizeChange function uses non-null assertions on
optional fields (c.type!, c.name!, c.config_id!) which can hide undefined
values; update normalizeChange to avoid "!" by providing safe fallbacks or
preserving undefined: build the returned config using c.config_id ?? null (or
c.config_id as-is), c.type ?? 'unknown' (or undefined), and c.name ?? '' (or
undefined) instead of casting, e.g. reference normalizeChange and the
config/config_id/type/name/deleted_at properties and replace the non-null
assertions with explicit default values or pass-through undefined so the result
reflects actual optionality and avoids unsafe assertions.
In `@src/ui/Icons/ChangeIcon.tsx`:
- Around line 9-13: Normalize the incoming severity string in severityColorClass
before doing the lookup against configChangeSeverity: trim and toLowerCase the
input, apply a small alias map for known synonyms (e.g. map "failure" ->
"error", map any variant of "info" to the canonical "info" if your config uses a
different case/naming, and map "success" synonyms appropriately), then use that
normalized key to find the item from Object.values(configChangeSeverity) and
return item?.colorClass ?? ""; update severityColorClass to perform these
normalization/mapping steps before the existing find.
In `@src/ui/Icons/Icon.tsx`:
- Line 935: The ChangeIcon component currently ignores the provided fallback and
returns null when icon resolution fails; update the failure branches in Icon.tsx
(including the similar block around lines 997-1008) so that instead of returning
null they return the passed-in fallback element (i.e., render fallback when icon
lookup/mapping for change_type fails), ensuring the fallback prop (fallback?:
React.ReactNode) is used whenever the resolved icon is missing.
---
Nitpick comments:
In `@e2e/tests/config-changes-swimlane.spec.ts`:
- Around line 14-23: The function gotoGraphView contains a redundant click of
the "Graph" button after navigating to "/catalog/changes?view=Graph"; remove the
line that calls page.getByRole("button", { name: "Graph" }).click() and rely on
the URL param to select the view, but keep the existing waits (the Catalog link
visibility check and await expect(page.locator(ROW_SEL).first()).toBeVisible({
timeout: 30000 })) to ensure the UI has settled; update gotoGraphView
accordingly so it only navigates, waits for the Catalog link, and then waits for
ROW_SEL to be visible.
- Around line 1-5: Remove the 1Password CLI credential block (the commented
"With 1Password CLI: eval $(op signin) && ..." snippet) from the test file so
the test (config-changes-swimlane.spec.ts) contains only test logic, and move
those instructions into a documentation file such as e2e/README.md or
CONTRIBUTING.md; ensure the moved instructions avoid embedding vault/item
identifiers or secrets and instead give a high-level guide or link to internal
secret-management docs.
In `@src/api/query-hooks/useConfigChangesHooks.ts`:
- Around line 173-205: Extract the repeated filter construction into a shared
helper function (e.g., buildConfigChangesFilter) and use it from both
useConfigChangesHooks and useGetAllConfigsChangesQuery so the table and graph
always use the same filters; move the logic that creates filterProps (all usages
of showChangesFromDeletedConfigs, timeRangeValue/from/to, params.get for
changeType/severity/configType/configTypes, useReactTableSortState
sortBy/sortOrder, pageSize, arbitraryFilter, and tags) into that helper and have
both call it to return a single canonical filter object instead of duplicating
the block that currently creates filterProps.
In
`@src/components/Configs/Changes/__tests__/ConfigChangesSwimlane.integration.test.tsx`:
- Around line 54-63: The test uses a brittle exact count assertion
(expect(expectedTypes.size).toBe(34)) tied to the fixture; update the "legend
shows all unique change types" test to avoid hardcoding 34: compute
expectedTypes as you already do from allChanges (variable expectedTypes) and
replace the toBe(34) assertion with a more resilient check such as
expect(expectedTypes.size).toBeGreaterThan(0) or
expect(expectedTypes.size).toBeGreaterThanOrEqual(5) (at least the number of
specific types you assert later), or instead assert that every value in
expectedTypes has a corresponding rendered legend entry by iterating
expectedTypes and checking screen.getAllByText(type).length > 0; keep using
renderSwimlane and the allChanges fixture.
In `@src/components/ui/hover-card.tsx`:
- Around line 14-25: The current change modifies the upstream primitive by
adding a portalized variant; instead create a new project-specific wrapper that
composes HoverCardPrimitive.Portal and HoverCardPrimitive.Content (preserving
ref forwarding, props spread, align, sideOffset and the cn className merge)
instead of editing the original shadcn component; restore the upstream
HoverCardPrimitive.Content to its original form, add a new component (e.g.,
PortalizedHoverCard or HoverCardPortal) that returns HoverCardPrimitive.Portal >
HoverCardPrimitive.Content with the same className string and {...props}, export
that wrapper for use across the app, and update imports to use the new wrapper
where the portalized behavior is needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72f46650-03ce-4aec-9f45-597d50a4d5fb
⛔ Files ignored due to path filters (1)
e2e/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.env.gitignoree2e/.gitignoree2e/playwright.config.tse2e/tests/auth.setup.tse2e/tests/config-changes-swimlane.spec.tse2e/tests/user-login-flow.spec.tssrc/api/query-hooks/useConfigChangesHooks.tssrc/components/Authentication/Kratos/KratosLogin.tsxsrc/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/ConfigDetailsChanges/ConfigDetailsChanges.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
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Configs/Changes/ConfigChangesViewToggle.tsx
Comment on lines
16
to
20
| # For clerk SAAS | ||
| # # NEXT_PUBLIC_AUTH_IS_CLERK=true | ||
| # NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY=pk_test_YmlnLW1vbmdvb3NlLTkxLmNsZXJrLmFjY291bnRzLmRldiQ | ||
| # CLERK_SECRET_KEY=sk_test_EeUiC56XXbSKiMZQULyhD7FZk8RGMpcLW7pVEO7emD | ||
|
|
Contributor
There was a problem hiding this comment.
Remove the Clerk keys from the tracked .env.
Even commented credentials end up in git history and should be treated as leaked. Please replace these with obvious placeholders and rotate the current secret outside this PR.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 18-18: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 19-19: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env around lines 16 - 20, Remove the actual Clerk keys from the tracked
.env by replacing the values for NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY and
CLERK_SECRET_KEY with clear placeholder strings (e.g.,
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY=placeholder and CLERK_SECRET_KEY=placeholder),
ensure any commented-out real keys are deleted, and add a note to rotate those
credentials outside this PR; update the .env so it contains only non-sensitive
placeholders and commit the change.
Comment on lines
+21
to
+23
| #TEST_URL=https://demo.flanksource.com | ||
| #USERNAME=demo@flanksource.com | ||
| #PASSWORD=demo@flanksource.com |
Contributor
There was a problem hiding this comment.
Update the sample e2e variable names to match the tests.
e2e/tests/user-login-flow.spec.ts now reads TEST_USERNAME and TEST_PASSWORD, so copying these examples as-is will leave the login flow unauthenticated.
📝 Suggested fix
-#USERNAME=demo@flanksource.com
-#PASSWORD=demo@flanksource.com
+#TEST_USERNAME=demo@flanksource.com
+#TEST_PASSWORD=demo@flanksource.com📝 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
| #TEST_URL=https://demo.flanksource.com | |
| #USERNAME=demo@flanksource.com | |
| #PASSWORD=demo@flanksource.com | |
| `#TEST_URL`=https://demo.flanksource.com | |
| `#TEST_USERNAME`=demo@flanksource.com | |
| `#TEST_PASSWORD`=demo@flanksource.com |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env around lines 21 - 23, Update the sample environment variable names in
the .env file to match the e2e tests: replace USERNAME and PASSWORD with
TEST_USERNAME and TEST_PASSWORD so they align with
e2e/tests/user-login-flow.spec.ts (which expects TEST_USERNAME and
TEST_PASSWORD), and ensure TEST_URL remains present as the test host. Make sure
the commented examples use the exact variable names TEST_URL, TEST_USERNAME, and
TEST_PASSWORD so copying them will authenticate the login flow.
Comment on lines
+35
to
+48
| use: { | ||
| headless: true, | ||
| channel: "chrome", | ||
| viewport: { width: 1920, height: 1024 } | ||
| } | ||
| }, | ||
|
|
||
| { | ||
| name: "webkit", | ||
| use: { ...devices["Desktop Safari"] } | ||
| name: "chromium", | ||
| dependencies: ["setup"], | ||
| use: { | ||
| headless: true, | ||
| channel: "chrome", | ||
| viewport: { width: 1920, height: 1024 }, | ||
| storageState: ".auth/user.json" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Playwright channel configuration:"
rg -nC2 'channel:\s*"chrome"' e2e
echo
echo "CI/browser provisioning hints:"
fd -HI 'Dockerfile*' . .github 2>/dev/null | xargs -r rg -nC2 'google-chrome|setup-chrome|playwright install'
fd -HI '*.yml' .github 2>/dev/null | xargs -r rg -nC2 'google-chrome|setup-chrome|playwright install'
fd -HI '*.yaml' .github 2>/dev/null | xargs -r rg -nC2 'google-chrome|setup-chrome|playwright install'Repository: flanksource/flanksource-ui
Length of output: 923
🏁 Script executed:
cat -n e2e/playwright.config.tsRepository: flanksource/flanksource-ui
Length of output: 2115
Change channel: "chrome" to use Playwright-managed browser or add CI detection.
Both projects hardcode channel: "chrome", which requires system-installed Google Chrome. However, CI (see ./e2e/Dockerfile) only installs Playwright-managed Chromium via npx playwright install --with-deps chromium, so tests will fail in CI before any test runs.
Either remove the channel specification to use the installed browser, or use environment detection (e.g., process.env.CI) like the rest of the config already does for retries and workers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/playwright.config.ts` around lines 35 - 48, The Playwright config
currently hardcodes channel: "chrome" in the browser use blocks (e.g., the
unnamed block and the "chromium" block), which forces a system Chrome instead of
the Playwright-managed Chromium used in CI; update those use objects to either
remove the channel property entirely so Playwright uses the installed browser,
or make channel conditional on CI (e.g., check process.env.CI) so local runs can
use "chrome" but CI uses the installed Playwright browser; locate the channel
entries inside the use: { ... } objects for the relevant browser configs and
change them accordingly.
Comment on lines
+211
to
+212
| getNextPageParam: (lastPage, allPages) => | ||
| lastPage.changes?.length < pageSize ? undefined : allPages.length, |
Contributor
There was a problem hiding this comment.
Use total to stop pagination instead of page length.
When the backend total is an exact multiple of pageSize, this still reports another page and triggers one extra empty request. The response already carries total, so the stop condition can be exact.
💡 Proposed fix
- getNextPageParam: (lastPage, allPages) =>
- lastPage.changes?.length < pageSize ? undefined : allPages.length,
+ getNextPageParam: (lastPage, allPages) => {
+ const loaded = allPages.reduce(
+ (sum, page) => sum + page.changes.length,
+ 0
+ );
+ return loaded >= lastPage.total ? undefined : allPages.length;
+ },📝 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
| getNextPageParam: (lastPage, allPages) => | |
| lastPage.changes?.length < pageSize ? undefined : allPages.length, | |
| getNextPageParam: (lastPage, allPages) => { | |
| const loaded = allPages.reduce( | |
| (sum, page) => sum + page.changes.length, | |
| 0 | |
| ); | |
| return loaded >= lastPage.total ? undefined : allPages.length; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/query-hooks/useConfigChangesHooks.ts` around lines 211 - 212, The
pagination stop logic in getNextPageParam wrongly uses lastPage.changes.length
which allows one extra empty request when total is an exact multiple of
pageSize; change it to use the backend-provided lastPage.total to determine
whether more pages exist (e.g., compute loaded = allPages.length * pageSize or
sum of fetched items and return undefined when loaded >= lastPage.total,
otherwise return allPages.length) so getNextPageParam uses lastPage.total and
pageSize (and/or a sum of changes lengths) instead of only
lastPage.changes?.length.
Comment on lines
+71
to
+76
| async (refresh: boolean, aal: string, returnTo?: string) => { | ||
| try { | ||
| const { data } = await ory.createBrowserLoginFlow({ | ||
| refresh: refresh, | ||
| // Check for two-factor authentication | ||
| aal: aal, | ||
| returnTo: returnTo | ||
| ...(returnTo ? { returnTo } : {}) |
Contributor
There was a problem hiding this comment.
returnTo is still effectively unconditional here.
Because Line 36 normalizes a missing query param to "/", this spread still sends returnTo on the first request even when the URL had no return_to. That means the new forbidden-return_to retry path is doing the real work instead of avoiding the bad request up front.
Proposed fix
- const returnTo = searchParams.get("return_to") || "/";
+ const returnTo = searchParams.get("return_to") || undefined;
…
- push(String(returnTo || "/"));
+ push(returnTo ?? "/");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Authentication/Kratos/KratosLogin.tsx` around lines 71 - 76,
The createBrowserLoginFlow call is still receiving a returnTo value when the
original query was missing (because the code normalizes missing return_to to
"/"), causing the forbidden-return_to retry path to run unnecessarily; update
the parameter spread in the async handler (the function that calls
ory.createBrowserLoginFlow) to only include returnTo when it is defined and not
the default "/" (e.g., check returnTo && returnTo !== "/" before spreading) so
the initial request omits returnTo and avoids the bad request path.
Comment on lines
+352
to
+373
| if (!seenTypes.has(type) || deferredTypes.has(type)) { | ||
| labelCandidates.push(iconPositions.length - 1); | ||
| } | ||
| } | ||
|
|
||
| const labelCount = Math.min(labelCandidates.length, 4); | ||
| const pattern = labelCount > 0 ? PLACEMENT_PATTERNS[labelCount - 1]! : []; | ||
|
|
||
| for (let i = 0; i < labelCandidates.length; i++) { | ||
| const pos = iconPositions[labelCandidates[i]!]!; | ||
| seenTypes.add(pos.type); | ||
| deferredTypes.delete(pos.type); | ||
| } | ||
|
|
||
| for (const pos of iconPositions) { | ||
| const candidateIdx = labelCandidates.indexOf(iconPositions.indexOf(pos)); | ||
| let placement: LabelPlacement = "none"; | ||
| if (candidateIdx >= 0 && candidateIdx < pattern.length) { | ||
| placement = pattern[candidateIdx]!; | ||
| } else if (candidateIdx >= pattern.length) { | ||
| placement = "extra"; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not mark overflowed label candidates as seen yet.
When a bucket has more than four candidate types, Line 360-364 marks all of them as seen before placement is assigned. The overflowed ones then fall into extra/none at Line 371-372 and are suppressed in later buckets, so some change types never get a real left/right label. deferredTypes looks intended to cover this case, but it is never populated.
Possible fix
const labelCount = Math.min(labelCandidates.length, 4);
const pattern = labelCount > 0 ? PLACEMENT_PATTERNS[labelCount - 1]! : [];
for (let i = 0; i < labelCandidates.length; i++) {
const pos = iconPositions[labelCandidates[i]!]!;
- seenTypes.add(pos.type);
- deferredTypes.delete(pos.type);
+ if (i < pattern.length) {
+ seenTypes.add(pos.type);
+ deferredTypes.delete(pos.type);
+ } else {
+ deferredTypes.add(pos.type);
+ }
}📝 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
| if (!seenTypes.has(type) || deferredTypes.has(type)) { | |
| labelCandidates.push(iconPositions.length - 1); | |
| } | |
| } | |
| const labelCount = Math.min(labelCandidates.length, 4); | |
| const pattern = labelCount > 0 ? PLACEMENT_PATTERNS[labelCount - 1]! : []; | |
| for (let i = 0; i < labelCandidates.length; i++) { | |
| const pos = iconPositions[labelCandidates[i]!]!; | |
| seenTypes.add(pos.type); | |
| deferredTypes.delete(pos.type); | |
| } | |
| for (const pos of iconPositions) { | |
| const candidateIdx = labelCandidates.indexOf(iconPositions.indexOf(pos)); | |
| let placement: LabelPlacement = "none"; | |
| if (candidateIdx >= 0 && candidateIdx < pattern.length) { | |
| placement = pattern[candidateIdx]!; | |
| } else if (candidateIdx >= pattern.length) { | |
| placement = "extra"; | |
| } | |
| if (!seenTypes.has(type) || deferredTypes.has(type)) { | |
| labelCandidates.push(iconPositions.length - 1); | |
| } | |
| } | |
| const labelCount = Math.min(labelCandidates.length, 4); | |
| const pattern = labelCount > 0 ? PLACEMENT_PATTERNS[labelCount - 1]! : []; | |
| for (let i = 0; i < labelCandidates.length; i++) { | |
| const pos = iconPositions[labelCandidates[i]!]!; | |
| if (i < pattern.length) { | |
| seenTypes.add(pos.type); | |
| deferredTypes.delete(pos.type); | |
| } else { | |
| deferredTypes.add(pos.type); | |
| } | |
| } | |
| for (const pos of iconPositions) { | |
| const candidateIdx = labelCandidates.indexOf(iconPositions.indexOf(pos)); | |
| let placement: LabelPlacement = "none"; | |
| if (candidateIdx >= 0 && candidateIdx < pattern.length) { | |
| placement = pattern[candidateIdx]!; | |
| } else if (candidateIdx >= pattern.length) { | |
| placement = "extra"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Changes/ConfigChangesSwimlaneUtils.ts` around lines
352 - 373, The current loop marks every label candidate as seen (and deletes
from deferredTypes) before computing actual placements, which causes overflowed
candidates to be suppressed later; deferredTypes is never populated. Fix by
removing the early seenTypes.add(pos.type)/deferredTypes.delete(pos.type) block
and instead: compute labelCount and pattern, then in the placement loop
determine placement for each candidate (using PLACEMENT_PATTERNS and
labelCandidates), call seenTypes.add(type) only when placement is a real
left/right label, and when there are more candidates than labelCount add those
overflowed types to deferredTypes so they remain available for subsequent
buckets; update logic around candidateIdx, placement, seenTypes, and
deferredTypes accordingly (refer to labelCandidates, iconPositions,
PLACEMENT_PATTERNS, seenTypes, deferredTypes, and LabelPlacement).
Comment on lines
+73
to
+75
| <Age from={changeDetails?.created_at!} format="timestamp" /> | ||
| <span className="text-xs text-gray-500"> | ||
| (<Age from={changeDetails?.created_at!} suffix={true} />) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsx | sed -n '70,80p'Repository: flanksource/flanksource-ui
Length of output: 558
🏁 Script executed:
cat -n src/ui/Age/Age.tsxRepository: flanksource/flanksource-ui
Length of output: 4981
🏁 Script executed:
rg "noNonNullAssertedOptionalChain" --type typescript --type tsxRepository: flanksource/flanksource-ui
Length of output: 104
🏁 Script executed:
fd -e json -e jsonc "biome\|eslint" . | head -20Repository: flanksource/flanksource-ui
Length of output: 52
🏁 Script executed:
rg "export.*isEmpty" src/utils/dateRepository: flanksource/flanksource-ui
Length of output: 124
🏁 Script executed:
cat src/utils/date.tsRepository: flanksource/flanksource-ui
Length of output: 8199
Remove the non-null assertions.
Age already accepts an optional from, so changeDetails?.created_at! only suppresses the type check while still allowing undefined through at runtime. It also triggers the Biome lint rule on lines 73 and 75.
Suggested fix
- <Age from={changeDetails?.created_at!} format="timestamp" />
+ <Age from={changeDetails?.created_at} format="timestamp" />
<span className="text-xs text-gray-500">
- (<Age from={changeDetails?.created_at!} suffix={true} />)
+ (<Age from={changeDetails?.created_at} suffix={true} />)
</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
| <Age from={changeDetails?.created_at!} format="timestamp" /> | |
| <span className="text-xs text-gray-500"> | |
| (<Age from={changeDetails?.created_at!} suffix={true} />) | |
| <Age from={changeDetails?.created_at} format="timestamp" /> | |
| <span className="text-xs text-gray-500"> | |
| (<Age from={changeDetails?.created_at} suffix={true} />) |
🧰 Tools
🪛 Biome (2.4.6)
[error] 73-73: Forbidden non-null assertion after optional chaining.
(lint/suspicious/noNonNullAssertedOptionalChain)
[error] 75-75: Forbidden non-null assertion after optional chaining.
(lint/suspicious/noNonNullAssertedOptionalChain)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsx`
around lines 73 - 75, The non-null assertion (!) should be removed from the Age
props—Age accepts an optional from, so replace changeDetails?.created_at! with
changeDetails?.created_at in both places where Age is used (the Age component
instances), removing the trailing ! to let the optional type flow correctly and
satisfy the Biome lint rule.
Comment on lines
+27
to
+37
| 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.
Non-null assertions on potentially undefined fields.
The normalizeChange function uses non-null assertions (!) on c.type and c.name, but according to the ConfigChange type definition, these fields are optional. If a change lacks these fields, the resulting config object will have undefined values cast as non-null.
🛡️ Proposed fix with fallbacks
function normalizeChange(c: ConfigChange): ConfigChange {
return {
...c,
config: {
id: c.config_id!,
- type: c.type!,
- name: c.name!,
+ type: c.type ?? c.config?.type ?? "",
+ name: c.name ?? c.config?.name ?? "",
deleted_at: c.deleted_at
}
};
}📝 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 { | |
| return { | |
| ...c, | |
| config: { | |
| id: c.config_id!, | |
| type: c.type ?? c.config?.type ?? "", | |
| name: c.name ?? c.config?.name ?? "", | |
| deleted_at: c.deleted_at | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/config/ConfigChangesPage.tsx` around lines 27 - 37, The
normalizeChange function uses non-null assertions on optional fields (c.type!,
c.name!, c.config_id!) which can hide undefined values; update normalizeChange
to avoid "!" by providing safe fallbacks or preserving undefined: build the
returned config using c.config_id ?? null (or c.config_id as-is), c.type ??
'unknown' (or undefined), and c.name ?? '' (or undefined) instead of casting,
e.g. reference normalizeChange and the config/config_id/type/name/deleted_at
properties and replace the non-null assertions with explicit default values or
pass-through undefined so the result reflects actual optionality and avoids
unsafe assertions.
Comment on lines
+9
to
+13
| export function severityColorClass(severity?: string): string { | ||
| const item = Object.values(configChangeSeverity).find( | ||
| (i) => i.value === severity | ||
| ); | ||
| return item?.colorClass ?? ""; |
Contributor
There was a problem hiding this comment.
Normalize API severities before looking them up.
This exact match only works for a subset of values in configChangeSeverity. failure, success, and lowercase info will fall through to "", so those changes lose their color in the new graph/legend.
🎨 Suggested fix
export function severityColorClass(severity?: string): string {
+ const normalized = severity?.toLowerCase();
+ const severityAliases: Record<string, string> = {
+ failure: "critical",
+ success: "low",
+ info: "info"
+ };
const item = Object.values(configChangeSeverity).find(
- (i) => i.value === severity
+ (i) =>
+ i.value.toLowerCase() ===
+ (severityAliases[normalized ?? ""] ?? normalized)
);
return item?.colorClass ?? "";
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/Icons/ChangeIcon.tsx` around lines 9 - 13, Normalize the incoming
severity string in severityColorClass before doing the lookup against
configChangeSeverity: trim and toLowerCase the input, apply a small alias map
for known synonyms (e.g. map "failure" -> "error", map any variant of "info" to
the canonical "info" if your config uses a different case/naming, and map
"success" synonyms appropriately), then use that normalized key to find the item
from Object.values(configChangeSeverity) and return item?.colorClass ?? "";
update severityColorClass to perform these normalization/mapping steps before
the existing find.
| prefix?: any; | ||
| size?: any; | ||
| iconWithColor?: string; | ||
| fallback?: React.ReactNode; |
Contributor
There was a problem hiding this comment.
Render fallback when icon resolution fails.
ChangeIcon now passes a fallback element, but this branch still returns null. Any unmapped change_type will disappear instead of showing the dot fallback.
🛠️ Minimal fix
- if (!Icon || !Icon.SVG) {
- return null;
- }
+ if (!Icon || !Icon.SVG) {
+ return fallback ? (
+ <>
+ {prefix} {fallback}
+ </>
+ ) : null;
+ }Also applies to: 997-1008
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/Icons/Icon.tsx` at line 935, The ChangeIcon component currently
ignores the provided fallback and returns null when icon resolution fails;
update the failure branches in Icon.tsx (including the similar block around
lines 997-1008) so that instead of returning null they return the passed-in
fallback element (i.e., render fallback when icon lookup/mapping for change_type
fails), ensuring the fallback prop (fallback?: React.ReactNode) is used whenever
the resolved icon is missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reavizScatterPlotatomWithStorageRebases the work from #2355 onto current main with minimal changes (Option A: keeps
ConfigChangeTable's internal modal handling intact).Test plan
Summary by CodeRabbit