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 persisted Table/Graph view toggle and Reaviz-based scatterplot for config changes, artifact types/services/UI/pages, a Basic auth flow with login/context/session checker, middleware.basic, API proxy simplifications, and broad error/toast handling updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant Toggle as ConfigChangesViewToggle
participant Page as ConfigDetailsChangesPage
participant Graph as ConfigChangesGraph
participant Query as useGetConfigChangesById
participant Modal as ConfigDetailChangeModal
User->>Toggle: Select "Graph"
Toggle->>Page: Persist view state ("Graph")
Page->>Graph: Render scatterplot (changes)
User->>Graph: Click point
Graph->>Page: onItemClicked(change)
Page->>Page: set selectedChange
Page->>Query: Fetch change details (enabled when Graph && selected)
Query-->>Page: Return changeDetails / loading
Page->>Modal: Render modal with changeDetails + loading
User->>Modal: Close
Page->>Page: clear selectedChange
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 docstrings
🧪 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: 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
moshloop
force-pushed
the
feat/config-changes-timeline-view
branch
from
6e5c7ea to
9d19422
Compare
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.
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.
.env
Outdated
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.
e2e/playwright.config.ts
Outdated
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
+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.
src/ui/Icons/Icon.tsx
Outdated
| 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.
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
b0a7611 to
ab4d394
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/pages/config/details/ConfigDetailsChangesPage.tsx (1)
42-46:⚠️ Potential issue | 🟡 MinorQuery runs unnecessarily when not in Graph view.
The
useGetConfigChangesByIdquery is enabled wheneverselectedChangeexists, regardless of the current view. If a user opens a modal in Graph view then switches to Table view, the query continues fetching for a hidden modal, and the stale selection reappears when switching back to Graph.🛠️ Proposed fix: gate query by view and clear selection on view change
-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]);🤖 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 is enabled whenever selectedChange exists, causing unnecessary fetches when the UI is not in Graph view; update the useGetConfigChangesById call to include the current view in its enabled condition (e.g., enabled: !!selectedChange && isGraphView) and add an effect to clear selection when switching away from Graph (call setSelectedChange(undefined) in a useEffect that runs on view changes) so the modal/query won't persist when not in Graph view.src/components/Configs/Changes/ConfigChangesGraph.tsx (1)
33-39:⚠️ Potential issue | 🔴 CriticalUnsafe handling of optional fields persists.
This issue was flagged in a previous review and remains unresolved:
- Line 35:
dayjs(undefined).toDate()returns the current date whenfirst_observedis missing, causing incorrect chart positioning.- Line 36: Non-null assertion after optional chaining (
change.config?.name!) is invalid—ifconfigisundefined, the result is stillundefined, not a string.🐛 Proposed fix: filter out invalid entries
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]);🤖 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 current useMemo mapping for the data array creates entries with invalid dates and uses a non-null assertion on change.config?.name; instead, update the useMemo in ConfigChangesGraph to filter out any change where change.first_observed is falsy or not a valid date and where change.config?.name is missing/empty, then map only the validated changes to ChartShallowDataShape and assign key using dayjs(change.first_observed).toDate() (without calling dayjs on undefined) and data using the safe string change.config.name (no non-null assertion). Locate the useMemo that defines `data: ChartShallowDataShape[]` and modify the mapping to validate `change.first_observed` and `change.config?.name` before producing items.
🧹 Nitpick comments (1)
src/components/Configs/Changes/ConfigChangesGraph.tsx (1)
86-86: Tooltip content usesanytype for data parameter.The
contentcallback receivesdata: any, losing type safety. Consider typing it properly or extractingmetadatawith a type guard.♻️ Suggested typing improvement
- content={(data: any) => { - const change = data.metadata as ConfigChange; + content={(data: { metadata?: ConfigChange }) => { + const change = data.metadata; + if (!change) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Configs/Changes/ConfigChangesGraph.tsx` at line 86, The tooltip callback currently types its parameter as any (content={(data: any) => ...}), which loses type safety; change the parameter to a concrete type (e.g., the graph/tooltip data type used in this component such as TooltipData or ChangeNode) or to unknown and then narrow it with a type guard that checks for data.metadata and the expected fields before accessing them; update the function signature in ConfigChangesGraph's content prop and replace direct accesses with safely-guarded extracts of metadata so TypeScript can enforce correct shapes (refer to the content callback and any code accessing data.metadata inside it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/Configs/Changes/ConfigChangesGraph.tsx`:
- Around line 33-39: The current useMemo mapping for the data array creates
entries with invalid dates and uses a non-null assertion on change.config?.name;
instead, update the useMemo in ConfigChangesGraph to filter out any change where
change.first_observed is falsy or not a valid date and where change.config?.name
is missing/empty, then map only the validated changes to ChartShallowDataShape
and assign key using dayjs(change.first_observed).toDate() (without calling
dayjs on undefined) and data using the safe string change.config.name (no
non-null assertion). Locate the useMemo that defines `data:
ChartShallowDataShape[]` and modify the mapping to validate
`change.first_observed` and `change.config?.name` before producing items.
In `@src/pages/config/details/ConfigDetailsChangesPage.tsx`:
- Around line 42-46: The query is enabled whenever selectedChange exists,
causing unnecessary fetches when the UI is not in Graph view; update the
useGetConfigChangesById call to include the current view in its enabled
condition (e.g., enabled: !!selectedChange && isGraphView) and add an effect to
clear selection when switching away from Graph (call
setSelectedChange(undefined) in a useEffect that runs on view changes) so the
modal/query won't persist when not in Graph view.
---
Nitpick comments:
In `@src/components/Configs/Changes/ConfigChangesGraph.tsx`:
- Line 86: The tooltip callback currently types its parameter as any
(content={(data: any) => ...}), which loses type safety; change the parameter to
a concrete type (e.g., the graph/tooltip data type used in this component such
as TooltipData or ChangeNode) or to unknown and then narrow it with a type guard
that checks for data.metadata and the expected fields before accessing them;
update the function signature in ConfigChangesGraph's content prop and replace
direct accesses with safely-guarded extracts of metadata so TypeScript can
enforce correct shapes (refer to the content callback and any code accessing
data.metadata inside it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d8f9ed7-381f-4407-b226-472e1a2ff558
⛔ 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
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 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
…ndling Introduces basic username/password auth as an alternative to Clerk and Kratos. Adds BasicLogin component, auth context provider, session checker, and API endpoint for login. Includes new middleware for basic auth flow and updates auth system detection to support three authentication backends.
- Update toastError to accept unknown and delegate to getErrorMessage - Replace all (ex as Error).message casts with toastError(ex) - Dynamically size toast width based on message length
- Fix created_by sending full user object instead of UUID string - Show save errors inline in the form dialog with truncation - Type createScrapePlugin to accept string for created_by
- Render image content types with img tag instead of raw text - Use isFetching instead of isLoading to fix spinner for images - Add artifactDownloadURL helper with filename query param - Update all download URL call sites to include filename
moshloop
force-pushed
the
feat/config-changes-timeline-view
branch
from
1c66bb1 to
32164d7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/config/settings/ConfigPluginsPage.tsx (1)
218-223:⚠️ Potential issue | 🟡 MinorClear
formErrorwhen the modal closes to avoid stale errors.Line 222 resets
editedRowbut leavesformErrorintact, so reopening the modal can show an old error banner.Suggested fix
useEffect(() => { if (isOpen) { return; } setEditedRow(undefined); + setFormError(undefined); }, [isOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/config/settings/ConfigPluginsPage.tsx` around lines 218 - 223, When the modal closes the useEffect currently clears editedRow but leaves stale formError; update that effect (the useEffect watching isOpen) to also clear the form error by calling the state updater (e.g., setFormError(undefined) or equivalent) alongside setEditedRow so any error banner is reset when isOpen becomes false.
♻️ Duplicate comments (1)
src/pages/config/details/ConfigDetailsChangesPage.tsx (1)
42-46:⚠️ Potential issue | 🟡 MinorClear graph selection when view changes away from Graph.
Line 42 keeps
selectedChangealive across view switches, so detail fetching can continue while hidden and stale selection reappears later.💡 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 selectedChange state (selectedChange / setSelectedChange) is kept when the UI switches away from the Graph view, causing hidden detail fetches via useGetConfigChangesById and stale selections later; add an effect that watches the current view variable (the component's view/activeView state) and calls setSelectedChange(undefined) whenever the view is not the Graph view so selection is cleared and the query remains disabled (useGetConfigChangesById's enabled prop stays consistent with !!selectedChange). Ensure this cleanup also runs when the component unmounts or when a different tab/view is selected.
🧹 Nitpick comments (3)
src/ui/ToasterWithCloseButton.tsx (2)
39-42: RedundantresolveValuecall.The
messageprovided byToastBar's render prop is already resolved. CallingresolveValue(message, t)again is redundant sincemessageis no longer aRenderablethat needs resolution.♻️ Suggested fix
<div className={`ml-1 flex-1 ${widthClass}`}> <p className="break-words text-sm font-medium text-gray-900"> - {resolveValue(message, t)} + {message} </p> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/ToasterWithCloseButton.tsx` around lines 39 - 42, In ToasterWithCloseButton, remove the redundant resolveValue call around the ToastBar render prop's message: the render prop already passes a resolved string/ReactNode as message, so replace resolveValue(message, t) with message directly (locate the JSX inside the ToastBar render function where resolveValue(message, t) is used), ensuring any i18n t usage is preserved elsewhere if needed.
17-18: Edge case: JSX message elements get arbitrary width.When
resolvedis not a string (e.g., a JSX element fromtoastSuccess),textLendefaults to 40, which may not reflect the actual rendered width. This is acceptable as a fallback, but consider whether JSX messages might warrant a different default width.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/ToasterWithCloseButton.tsx` around lines 17 - 18, The code uses a fixed fallback of 40 for textLen when resolved (from resolveValue) is not a string, which can mis-size JSX messages; update the logic in ToasterWithCloseButton (where resolved and textLen are computed) to: detect React elements via React.isValidElement(resolved) and try to extract text by using ReactDOMServer.renderToStaticMarkup(resolved) and measuring its length, falling back to a smaller configurable DEFAULT_TEXT_LEN constant if renderToStaticMarkup is unavailable or returns empty; keep the existing branch for string cases unchanged and expose DEFAULT_TEXT_LEN as a local constant so callers (e.g., toastSuccess) can tune it if needed.src/components/Plugins/PluginsForm.tsx (1)
34-35: Reset error expansion state when the error message changes.If a previous long error was expanded, the next error inherits that state unexpectedly. Consider resetting
isErrorExpandedonerrorMessageupdates.Suggested fix
-import { useState } from "react"; +import { useEffect, useState } from "react"; @@ const [isConfirmDialogOpen, setIsConfirmDialogOpen] = useState(false); const [isErrorExpanded, setIsErrorExpanded] = useState(false); + useEffect(() => { + setIsErrorExpanded(false); + }, [errorMessage]);Also applies to: 83-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Plugins/PluginsForm.tsx` around lines 34 - 35, The isErrorExpanded state can persist across different error messages; add a useEffect inside the PluginsForm component that watches errorMessage and calls setIsErrorExpanded(false) whenever errorMessage changes (including when it becomes undefined) so each new error starts collapsed; reference the isErrorExpanded and setIsErrorExpanded state hooks and the errorMessage variable to locate where to add this effect (also ensure the same fix is applied for the related error display logic around the other error block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware.basic.ts`:
- Around line 6-7: The public-path matcher in isPublicPath is too permissive
(e.g., startsWith("/login") matches "/login-anything"); update isPublicPath to
only allow exact matches or proper path segment prefixes: for each entry in
publicPaths (used in isPublicPath) return true if pathname === p or
pathname.startsWith(p + "/") (and handle root "/" appropriately), ensuring
entries like "/login" only match "/login" or "/login/..." but not
"/login-anything".
- Around line 17-20: The middleware currently treats any non-empty query param
returned by request.nextUrl.searchParams.get("token") as authenticated and
immediately returns NextResponse.next(), which allows auth bypass; instead, in
the middleware function validate the token value server-side (e.g., verify JWT
signature/expiry or compare against a secure server-side secret) before allowing
access, only call NextResponse.next() when verification succeeds (otherwise
return a redirect/NextResponse.rewrite or a 401/403 Response), and update logic
around the token variable and NextResponse.next() so an empty or invalid token
does not grant access.
In `@pages/api/`[...paths].ts:
- Around line 11-19: The current code puts Clerk on the critical path and
assumes org_id and publicMetadata.backend_url exist; wrap the Clerk lookup in a
try/catch and validate values so failures fall back to process.env.BACKEND_URL:
check process.env.NEXT_PUBLIC_AUTH_IS_CLERK === "true", then try to import
clerkClient/getAuth, call getAuth(req) and only proceed to
clerkClient.organizations.getOrganization(...) if user.sessionClaims?.org_id is
present, verify org?.publicMetadata?.backend_url exists and is non-empty, and if
any step throws or returns missing data return process.env.BACKEND_URL;
optionally enforce a short timeout around the Clerk call to avoid slow responses
blocking requests.
In `@pages/api/auth/login.ts`:
- Around line 16-23: The external POST to `${BACKEND_URL}/auth/login` is
unprotected from hangs and thrown fetch errors; wrap the call in an
AbortController with a short timeout (e.g., 5–10s), pass controller.signal into
fetch, and clear the timeout on success, and surround the await fetch(...) with
try/catch to handle both AbortError (return a 504 with a clear message) and
other network errors (return a 502), while preserving the existing
headers/Authorization (basicAuth) and JSON body handling so callers get a
controlled response instead of an unhandled exception.
In `@src/api/axios.ts`:
- Around line 247-249: When redirecting for basic auth in the isBasicAuthSystem
branch, the return_to parameter is built from window.location.pathname +
window.location.search without encoding which can break if the current URL
contains query separators; update the assignment that sets window.location.href
to URL-encode the combined return path (e.g., use encodeURIComponent on
window.location.pathname + window.location.search) so the login URL is built
safely and post-login navigation works reliably.
In `@src/components/Artifacts/ArtifactPreviewModal.tsx`:
- Around line 41-47: The useQuery call in ArtifactPreviewModal is not capturing
error state, so download failures are silent; update the hook to destructure
error and isError (e.g., const { data: content, isFetching: isLoading, error,
isError } = useQuery(...)) and ensure the queryFn still calls
downloadArtifact(artifact!.id). Then add explicit error rendering in the modal
preview area (inside ArtifactPreviewModal render path where content/isLoading
are checked): when isError render a user-facing error message (optionally show
error.message) and avoid showing the empty preview area; keep existing loading
and success branches intact so failures are visible to users.
- Around line 45-46: ArtifactPreviewModal is using React Query options with
staleTime: Infinity and cacheTime: 24h which causes large preview blobs to
persist; update the query/options in ArtifactPreviewModal to use
transient-friendly values (e.g., staleTime: 0 or a few minutes and cacheTime: a
short interval like 5 minutes) so preview data is garbage-collectable soon after
the modal closes, and ensure you update the staleTime and cacheTime symbols in
the query call that loads artifact preview data.
In `@src/components/Artifacts/ArtifactsSummaryCards.tsx`:
- Around line 35-40: The card list uses the display label (s.label) as React
keys which can duplicate; instead use the unique storageKey derived when
aggregating entries (see storageMap and storageKey in ArtifactsSummaryCards) as
the key for the rendered cards. Update the component render so each card uses
the unique storageKey (or attach that storageKey to the aggregated object and
use it) rather than s.label to avoid duplicate keys and broken reconciliation.
In `@src/components/Authentication/Basic/BasicAuthContextProvider.tsx`:
- Around line 37-46: The current render logic in BasicAuthContextProvider hides
non-401 whoami() failures behind FullPageSkeletonLoader and performs a redirect
during render; change this by moving the 401 redirect into a useEffect that
watches error and payload, use encodeURIComponent(window.location.pathname +
window.location.search) for the return_to query value, and for non-401 errors
(when error && !payload and error.response?.status !== 401) render a visible
error/retry state instead of FullPageSkeletonLoader; reference the existing
error and payload variables, FullPageSkeletonLoader component, and the redirect
assignment to window.location.href when implementing these changes.
In `@src/components/Authentication/Basic/BasicLogin.tsx`:
- Around line 15-16: The returnTo value obtained via
searchParams.get("return_to") in BasicLogin (the returnTo variable) is used for
post-login redirects and must be validated to prevent open-redirects; update the
code where returnTo is set and used (searchParams.get("return_to") and the
redirect call around line ~40) to allow only relative paths or same-origin URLs:
parse the value, reject full absolute URLs with different origins, ensure it
begins with "/" and does not contain protocol/host or dangerous schemes, and
fall back to "/" when invalid or missing. Ensure the validation logic is
centralized (e.g., a small helper used by BasicLogin) so the redirect always
uses the sanitized returnTo.
- Around line 3-4: In BasicLogin.tsx the call to useSearchParams() can return
null during static prerendering causing searchParams.get(...) to throw; update
the returnTo resolution to use the Pages Router query (router.query.return_to)
instead of searchParams.get, e.g., derive const returnTo from router.query
(falling back to "/") or alternatively guard searchParams with a null-check
before calling .get; locate the useSearchParams import and the returnTo variable
(searchParams.get("return_to")) and replace with a safe router.query-based or
null-checked implementation.
In `@src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsx`:
- Line 47: The window.open call in PlaybookResultsDropdownButton uses "_blank"
without safety flags which allows reverse-tabnabbing; change the call that opens
artifactDownloadURL(artifactID, fileName) to include the noopener,noreferrer
feature string (e.g. pass "noopener,noreferrer" as the third argument) and also
capture the returned window object and set newWindow.opener = null when non-null
to ensure the opened page cannot access window.opener; update the call site
where artifactDownloadURL, artifactID, and fileName are used.
In `@src/pages/Settings/ArtifactsPage.tsx`:
- Around line 119-127: When updating filters in the onChange handlers that build
nextParams (the search input handler using nextParams/setSearchParams and the
content type dropdown handler that sets "content_type"), ensure you also remove
the "pageIndex" query parameter so pagination resets to the first page; i.e.,
call nextParams.delete("pageIndex") before calling setSearchParams(nextParams)
in both handlers.
---
Outside diff comments:
In `@src/pages/config/settings/ConfigPluginsPage.tsx`:
- Around line 218-223: When the modal closes the useEffect currently clears
editedRow but leaves stale formError; update that effect (the useEffect watching
isOpen) to also clear the form error by calling the state updater (e.g.,
setFormError(undefined) or equivalent) alongside setEditedRow so any error
banner is reset when isOpen becomes false.
---
Duplicate comments:
In `@src/pages/config/details/ConfigDetailsChangesPage.tsx`:
- Around line 42-46: The selectedChange state (selectedChange /
setSelectedChange) is kept when the UI switches away from the Graph view,
causing hidden detail fetches via useGetConfigChangesById and stale selections
later; add an effect that watches the current view variable (the component's
view/activeView state) and calls setSelectedChange(undefined) whenever the view
is not the Graph view so selection is cleared and the query remains disabled
(useGetConfigChangesById's enabled prop stays consistent with !!selectedChange).
Ensure this cleanup also runs when the component unmounts or when a different
tab/view is selected.
---
Nitpick comments:
In `@src/components/Plugins/PluginsForm.tsx`:
- Around line 34-35: The isErrorExpanded state can persist across different
error messages; add a useEffect inside the PluginsForm component that watches
errorMessage and calls setIsErrorExpanded(false) whenever errorMessage changes
(including when it becomes undefined) so each new error starts collapsed;
reference the isErrorExpanded and setIsErrorExpanded state hooks and the
errorMessage variable to locate where to add this effect (also ensure the same
fix is applied for the related error display logic around the other error
block).
In `@src/ui/ToasterWithCloseButton.tsx`:
- Around line 39-42: In ToasterWithCloseButton, remove the redundant
resolveValue call around the ToastBar render prop's message: the render prop
already passes a resolved string/ReactNode as message, so replace
resolveValue(message, t) with message directly (locate the JSX inside the
ToastBar render function where resolveValue(message, t) is used), ensuring any
i18n t usage is preserved elsewhere if needed.
- Around line 17-18: The code uses a fixed fallback of 40 for textLen when
resolved (from resolveValue) is not a string, which can mis-size JSX messages;
update the logic in ToasterWithCloseButton (where resolved and textLen are
computed) to: detect React elements via React.isValidElement(resolved) and try
to extract text by using ReactDOMServer.renderToStaticMarkup(resolved) and
measuring its length, falling back to a smaller configurable DEFAULT_TEXT_LEN
constant if renderToStaticMarkup is unavailable or returns empty; keep the
existing branch for string cases unchanged and expose DEFAULT_TEXT_LEN as a
local constant so callers (e.g., toastSuccess) can tune it if needed.
🪄 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: 3f22e783-50d0-49be-a882-1f6c3c94e124
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (47)
.gitignoremiddleware.basic.tsmiddleware.clerk.tsmiddleware.kratos.tspackage.jsonpages/api/[...paths].tspages/api/auth/login.tspages/auth-state-checker.tsxpages/login/[[...index]].tsxsrc/App.tsxsrc/api/axios.tssrc/api/query-hooks/useFeatureFlags.tsxsrc/api/services/artifacts.tssrc/api/services/configs.tssrc/api/services/scrapePlugins.tssrc/api/types/artifacts.tssrc/api/types/configs.tssrc/components/Artifacts/ArtifactPreviewModal.tsxsrc/components/Artifacts/ArtifactsSummaryCards.tsxsrc/components/Artifacts/ArtifactsTable.tsxsrc/components/Authentication/AuthProviderWrapper.tsxsrc/components/Authentication/AuthSessionChecker.tsxsrc/components/Authentication/Basic/BasicAuthContextProvider.tsxsrc/components/Authentication/Basic/BasicAuthSessionChecker.tsxsrc/components/Authentication/Basic/BasicLogin.tsxsrc/components/Authentication/useDetermineAuthSystem.tsxsrc/components/Configs/Changes/ConfigChangesGraph.tsxsrc/components/Configs/Changes/ConfigChangesViewToggle.tsxsrc/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsxsrc/components/Configs/Changes/ConfigsRelatedChanges/FilterBar/ConfigRelatedChangesFilters.tsxsrc/components/Configs/Sidebar/ConfigsPanel.tsxsrc/components/Forms/Formik/FormikConnectionField.tsxsrc/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsxsrc/components/Playbooks/Runs/Actions/PlaybooksActionsResults.tsxsrc/components/Plugins/PluginsForm.tsxsrc/components/Plugins/PluginsFormModal.tsxsrc/components/Toast/toast.tssrc/context/UserAccessContext/permissions.tssrc/pages/Settings/ArtifactsPage.tsxsrc/pages/Settings/ConnectionsPage.tsxsrc/pages/applications/ApplicationsPage.tsxsrc/pages/config/details/ConfigDetailsChangesPage.tsxsrc/pages/config/settings/ConfigPluginsPage.tsxsrc/pages/views/ViewsPage.tsxsrc/services/permissions/features.tssrc/ui/SkeletonLoader/FullPageSkeletonLoader.tsxsrc/ui/ToasterWithCloseButton.tsx
✅ Files skipped from review due to trivial changes (5)
- src/services/permissions/features.ts
- .gitignore
- src/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsx
- src/api/types/artifacts.ts
- src/components/Configs/Changes/ConfigChangesViewToggle.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Configs/Changes/ConfigsRelatedChanges/FilterBar/ConfigRelatedChangesFilters.tsx
- src/components/Configs/Changes/ConfigChangesGraph.tsx
- package.json
Comment on lines
+6
to
+7
| function isPublicPath(pathname: string): boolean { | ||
| return publicPaths.some((p) => pathname.startsWith(p)); |
Contributor
There was a problem hiding this comment.
Public-path matching is overly broad.
On Line 7, startsWith("/login") and startsWith("/registration") also match paths like /login-anything, which can unintentionally expand unauthenticated surface area.
✅ Safer matcher
function isPublicPath(pathname: string): boolean {
- return publicPaths.some((p) => pathname.startsWith(p));
+ return (
+ pathname === "/login" ||
+ pathname === "/registration" ||
+ pathname.startsWith("/api/")
+ );
}📝 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 isPublicPath(pathname: string): boolean { | |
| return publicPaths.some((p) => pathname.startsWith(p)); | |
| function isPublicPath(pathname: string): boolean { | |
| return ( | |
| pathname === "/login" || | |
| pathname === "/registration" || | |
| pathname.startsWith("/api/") | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware.basic.ts` around lines 6 - 7, The public-path matcher in
isPublicPath is too permissive (e.g., startsWith("/login") matches
"/login-anything"); update isPublicPath to only allow exact matches or proper
path segment prefixes: for each entry in publicPaths (used in isPublicPath)
return true if pathname === p or pathname.startsWith(p + "/") (and handle root
"/" appropriately), ensuring entries like "/login" only match "/login" or
"/login/..." but not "/login-anything".
Comment on lines
+17
to
+20
| const token = request.nextUrl.searchParams.get("token"); | ||
| if (token) { | ||
| return NextResponse.next(); | ||
| } |
Contributor
There was a problem hiding this comment.
Unvalidated token query param creates an auth bypass.
On Line 17–20, any non-empty ?token= skips authentication for protected pages. This is effectively unauthenticated access unless the token is validated server-side in middleware.
🔐 Suggested hardening
- const token = request.nextUrl.searchParams.get("token");
- if (token) {
- return NextResponse.next();
- }
+ // Do not bypass auth solely on token presence.
+ // If token-based login is required, validate token cryptographically here
+ // and only for explicit callback routes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware.basic.ts` around lines 17 - 20, The middleware currently treats
any non-empty query param returned by request.nextUrl.searchParams.get("token")
as authenticated and immediately returns NextResponse.next(), which allows auth
bypass; instead, in the middleware function validate the token value server-side
(e.g., verify JWT signature/expiry or compare against a secure server-side
secret) before allowing access, only call NextResponse.next() when verification
succeeds (otherwise return a redirect/NextResponse.rewrite or a 401/403
Response), and update logic around the token variable and NextResponse.next() so
an empty or invalid token does not grant access.
Comment on lines
+11
to
+19
| if (process.env.NEXT_PUBLIC_AUTH_IS_CLERK === "true") { | ||
| const { clerkClient, getAuth } = await import("@clerk/nextjs/server"); | ||
| const user = getAuth(req); | ||
| const org = await clerkClient.organizations.getOrganization({ | ||
| organizationId: user.sessionClaims?.org_id! | ||
| }); | ||
| const backendURL = org?.publicMetadata?.backend_url; | ||
| // for now, lets fallback to the old way of doing things, if the backend_url | ||
| // is not set in the org metadata | ||
| const target = backendURL; | ||
| return target; | ||
| return org?.publicMetadata?.backend_url; | ||
| } | ||
| return API_URL; | ||
| return process.env.BACKEND_URL; |
Contributor
There was a problem hiding this comment.
Keep the proxy off Clerk's critical path.
Line 14 now adds a live Clerk round-trip to every /api/* request, and Lines 15-17 still assume both org_id and publicMetadata.backend_url are present. A slow Clerk response, missing org claim, or unprovisioned org will now turn normal backend traffic into 500s instead of falling back to BACKEND_URL.
🔧 Minimal fallback fix
async function getTargetURL(req: NextApiRequest) {
if (process.env.NEXT_PUBLIC_AUTH_IS_CLERK === "true") {
const { clerkClient, getAuth } = await import("@clerk/nextjs/server");
const user = getAuth(req);
- const org = await clerkClient.organizations.getOrganization({
- organizationId: user.sessionClaims?.org_id!
- });
- return org?.publicMetadata?.backend_url;
+ const orgId = user.sessionClaims?.org_id;
+ if (!orgId) {
+ return process.env.BACKEND_URL;
+ }
+ const org = await clerkClient.organizations.getOrganization({
+ organizationId: orgId
+ });
+ return typeof org?.publicMetadata?.backend_url === "string"
+ ? org.publicMetadata.backend_url
+ : process.env.BACKEND_URL;
}
return process.env.BACKEND_URL;
}📝 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 (process.env.NEXT_PUBLIC_AUTH_IS_CLERK === "true") { | |
| const { clerkClient, getAuth } = await import("@clerk/nextjs/server"); | |
| const user = getAuth(req); | |
| const org = await clerkClient.organizations.getOrganization({ | |
| organizationId: user.sessionClaims?.org_id! | |
| }); | |
| const backendURL = org?.publicMetadata?.backend_url; | |
| // for now, lets fallback to the old way of doing things, if the backend_url | |
| // is not set in the org metadata | |
| const target = backendURL; | |
| return target; | |
| return org?.publicMetadata?.backend_url; | |
| } | |
| return API_URL; | |
| return process.env.BACKEND_URL; | |
| if (process.env.NEXT_PUBLIC_AUTH_IS_CLERK === "true") { | |
| const { clerkClient, getAuth } = await import("@clerk/nextjs/server"); | |
| const user = getAuth(req); | |
| const orgId = user.sessionClaims?.org_id; | |
| if (!orgId) { | |
| return process.env.BACKEND_URL; | |
| } | |
| const org = await clerkClient.organizations.getOrganization({ | |
| organizationId: orgId | |
| }); | |
| return typeof org?.publicMetadata?.backend_url === "string" | |
| ? org.publicMetadata.backend_url | |
| : process.env.BACKEND_URL; | |
| } | |
| return process.env.BACKEND_URL; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/api/`[...paths].ts around lines 11 - 19, The current code puts Clerk on
the critical path and assumes org_id and publicMetadata.backend_url exist; wrap
the Clerk lookup in a try/catch and validate values so failures fall back to
process.env.BACKEND_URL: check process.env.NEXT_PUBLIC_AUTH_IS_CLERK === "true",
then try to import clerkClient/getAuth, call getAuth(req) and only proceed to
clerkClient.organizations.getOrganization(...) if user.sessionClaims?.org_id is
present, verify org?.publicMetadata?.backend_url exists and is non-empty, and if
any step throws or returns missing data return process.env.BACKEND_URL;
optionally enforce a short timeout around the Clerk call to avoid slow responses
blocking requests.
Comment on lines
+37
to
+46
| if (error && !payload) { | ||
| if (error.response?.status === 401) { | ||
| window.location.href = `/login?return_to=${window.location.pathname}${window.location.search}`; | ||
| return <FullPageSkeletonLoader />; | ||
| } | ||
| } | ||
|
|
||
| if (!payload) { | ||
| return <FullPageSkeletonLoader />; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't hide non-401 whoami() failures behind the loading skeleton.
Line 39 performs a redirect during render, and any other failure then falls through to Lines 44-45, which leaves the app on FullPageSkeletonLoader forever. Please move the 401 redirect into an effect, encode the return_to value, and surface a real error/retry state for non-401 failures instead of reusing the loading UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Authentication/Basic/BasicAuthContextProvider.tsx` around
lines 37 - 46, The current render logic in BasicAuthContextProvider hides
non-401 whoami() failures behind FullPageSkeletonLoader and performs a redirect
during render; change this by moving the 401 redirect into a useEffect that
watches error and payload, use encodeURIComponent(window.location.pathname +
window.location.search) for the return_to query value, and for non-401 errors
(when error && !payload and error.response?.status !== 401) render a visible
error/retry state instead of FullPageSkeletonLoader; reference the existing
error and payload variables, FullPageSkeletonLoader component, and the redirect
assignment to window.location.href when implementing these changes.
Comment on lines
+3
to
+4
| import { useSearchParams } from "next/navigation"; | ||
| import { useRouter } from "next/router"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Next.js 14.2.2 pages router (pages/**), is useSearchParamsfromnext/navigationofficially supported, or should query params be read fromnext/router (router.query)?
💡 Result:
useSearchParams from next/navigation is officially supported in the Pages Router (i.e., pages/**)—Next.js docs explicitly state it “works in both the Pages Router and App Router.” [1]
That said, in pages/** you can still read query params via next/router (router.query), and it’s often the simplest choice—especially because useSearchParams() can be null during prerendering for statically optimized pages and only becomes available after hydration (unless you use getServerSideProps, where it’s available immediately). [1] The Pages Router docs also show using next/router alongside useSearchParams (and next/compat/router for shared components). [2]
Practical guidance (Pages Router):
- If you’re purely in
pages/**:router.queryis fine (and commonly used). - If you want a hook that also works in
app/**/ shared components:useSearchParamsis supported—just handle the prerenderingnullcase. [1]
Sources
[1] Next.js Docs — Pages Router API: useSearchParams https://nextjs.org/docs/pages/api-reference/functions/use-search-params
[2] Next.js Docs — Pages Router API: useRouter / next/compat/router https://nextjs.org/docs/pages/api-reference/functions/use-router
🏁 Script executed:
# Check if the file exists and read it
cat -n src/components/Authentication/Basic/BasicLogin.tsxRepository: flanksource/flanksource-ui
Length of output: 4844
🏁 Script executed:
# Check the directory structure to understand if this is a pages or app router context
find . -type d -name "components" | head -20
find . -type d -name "pages" | head -20
find . -type d -name "app" | head -20Repository: flanksource/flanksource-ui
Length of output: 231
🏁 Script executed:
rg "BasicLogin" --type ts --type tsx -A 2 -B 2Repository: flanksource/flanksource-ui
Length of output: 97
🏁 Script executed:
rg "BasicLogin" -A 2 -B 2Repository: flanksource/flanksource-ui
Length of output: 1278
🏁 Script executed:
cat -n pages/login/[[...index]].tsxRepository: flanksource/flanksource-ui
Length of output: 1074
🏁 Script executed:
# Check if the login page has getServerSideProps or any static generation config
rg "getServerSideProps|getStaticProps|revalidate" pages/login/[[...index]].tsxRepository: flanksource/flanksource-ui
Length of output: 52
Handle null useSearchParams() during static prerendering or use router.query instead.
The login page (pages/login/[[...index]].tsx) is statically optimized (no getServerSideProps), which causes useSearchParams() to return null during prerendering. Line 15 calls .get() without null-checking, causing a runtime error:
const returnTo = searchParams.get("return_to") || "/"; // TypeError if searchParams is nullIn Pages Router, prefer router.query (line 13 already has useRouter()):
const returnTo = router.query.return_to || "/";Alternatively, add getServerSideProps to prevent static prerendering, or null-check searchParams before calling .get().
Also applies to: 13-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Authentication/Basic/BasicLogin.tsx` around lines 3 - 4, In
BasicLogin.tsx the call to useSearchParams() can return null during static
prerendering causing searchParams.get(...) to throw; update the returnTo
resolution to use the Pages Router query (router.query.return_to) instead of
searchParams.get, e.g., derive const returnTo from router.query (falling back to
"/") or alternatively guard searchParams with a null-check before calling .get;
locate the useSearchParams import and the returnTo variable
(searchParams.get("return_to")) and replace with a safe router.query-based or
null-checked implementation.
Comment on lines
+15
to
+16
| const returnTo = searchParams.get("return_to") || "/"; | ||
|
|
Contributor
There was a problem hiding this comment.
return_to is not validated before redirect (open redirect risk).
On Line 15 and Line 40, a crafted /login?return_to=https://attacker.tld can redirect users off-site after login.
🔒 Suggested safe redirect guard
- const returnTo = searchParams.get("return_to") || "/";
+ const rawReturnTo = searchParams.get("return_to") || "/";
+ const returnTo =
+ rawReturnTo.startsWith("/") && !rawReturnTo.startsWith("//")
+ ? rawReturnTo
+ : "/";Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Authentication/Basic/BasicLogin.tsx` around lines 15 - 16, The
returnTo value obtained via searchParams.get("return_to") in BasicLogin (the
returnTo variable) is used for post-login redirects and must be validated to
prevent open-redirects; update the code where returnTo is set and used
(searchParams.get("return_to") and the redirect call around line ~40) to allow
only relative paths or same-origin URLs: parse the value, reject full absolute
URLs with different origins, ensure it begins with "/" and does not contain
protocol/host or dangerous schemes, and fall back to "/" when invalid or
missing. Ensure the validation logic is centralized (e.g., a small helper used
by BasicLogin) so the redirect always uses the sanitized returnTo.
| const { artifactDownloadURL } = await import( | ||
| "@flanksource-ui/api/services/artifacts" | ||
| ); | ||
| window.open(artifactDownloadURL(artifactID, fileName), "_blank"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect _blank window.open usage in this file.
rg -n 'window\.open\(' src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsxRepository: flanksource/flanksource-ui
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Get context around line 47 to understand the function/flow
sed -n '40,55p' src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsxRepository: flanksource/flanksource-ui
Length of output: 558
🏁 Script executed:
#!/bin/bash
# Check for all window.open calls in the file (not just ones with "blank")
rg -n 'window\.open' src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsx -A 2 -B 2Repository: flanksource/flanksource-ui
Length of output: 234
Add security flags to window.open() call to prevent reverse-tabnabbing.
Line 47 opens a new tab with "_blank" without noopener,noreferrer parameters, leaving the opened page able to access window.opener. Add these flags to prevent opener attacks.
Suggested fix
- window.open(artifactDownloadURL(artifactID, fileName), "_blank");
+ window.open(
+ artifactDownloadURL(artifactID, fileName),
+ "_blank",
+ "noopener,noreferrer"
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsx` at
line 47, The window.open call in PlaybookResultsDropdownButton uses "_blank"
without safety flags which allows reverse-tabnabbing; change the call that opens
artifactDownloadURL(artifactID, fileName) to include the noopener,noreferrer
feature string (e.g. pass "noopener,noreferrer" as the third argument) and also
capture the returned window object and set newWindow.opener = null when non-null
to ensure the opened page cannot access window.opener; update the call site
where artifactDownloadURL, artifactID, and fileName are used.
Comment on lines
+119
to
+127
| onChange={(e) => { | ||
| const nextParams = new URLSearchParams(searchParams); | ||
| if (e.target.value) { | ||
| nextParams.set("search", e.target.value); | ||
| } else { | ||
| nextParams.delete("search"); | ||
| } | ||
| setSearchParams(nextParams); | ||
| }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate pagination hook implementation:"
fd -i 'useReactTablePaginationState.ts' .
fd -i 'useReactTablePaginationState.tsx' .
echo
echo "Inspect pagination query-param keys and behavior:"
rg -n -C3 'page|searchParams|useSearchParams|setSearchParams|pageIndex|pageSize' $(fd -i 'useReactTablePaginationState.ts' .; fd -i 'useReactTablePaginationState.tsx' .)
echo
echo "Inspect current filter handlers in ArtifactsPage:"
rg -n -C3 'new URLSearchParams|setSearchParams|content_type|search' src/pages/Settings/ArtifactsPage.tsxRepository: flanksource/flanksource-ui
Length of output: 14188
🏁 Script executed:
rg -n 'useReactTablePaginationState|useReactTableSortState|paginationState|pagination' src/pages/Settings/ArtifactsPage.tsx -A 3 -B 1Repository: flanksource/flanksource-ui
Length of output: 811
Reset pagination when filters change.
When search or content_type filters are updated, the code preserves all existing query parameters, including pageIndex. If a user is viewing page 2+ and narrows the results such that fewer pages exist, the page index may point beyond the available pages, resulting in an empty result set despite matches existing.
Clear the pageIndex query parameter when filters change to reset pagination to the first page.
This applies to both the search input handler (lines 119-127) and the content type dropdown handler (lines 138-146).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Settings/ArtifactsPage.tsx` around lines 119 - 127, When updating
filters in the onChange handlers that build nextParams (the search input handler
using nextParams/setSearchParams and the content type dropdown handler that sets
"content_type"), ensure you also remove the "pageIndex" query parameter so
pagination resets to the first page; i.e., call nextParams.delete("pageIndex")
before calling setSearchParams(nextParams) in both handlers.
The reaviz library imports @upsetjs/venn.js which Jest cannot resolve, causing 8 test suites to fail. Lazy-importing the graph component prevents reaviz from being eagerly loaded during test module resolution.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pages/config/settings/ConfigPluginsPage.tsx (1)
218-223:⚠️ Potential issue | 🟡 MinorConsider clearing
formErrorwhen the modal closes.The
useEffectresetseditedRowwhen the modal closes, butformErrorpersists. If a user triggers an error, closes the modal, and reopens it (especially for a new plugin), the stale error message will still be visible.Proposed fix
useEffect(() => { if (isOpen) { return; } setEditedRow(undefined); + setFormError(undefined); }, [isOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/config/settings/ConfigPluginsPage.tsx` around lines 218 - 223, The effect that runs when the modal open state changes currently clears editedRow but not the formError, causing stale errors to persist; update the same useEffect (the one using isOpen and setEditedRow) to also clear the form error state (e.g., call setFormError(undefined) or setFormError('')) when isOpen is false so that closing the modal resets both editedRow and formError before the modal is reopened.pages/auth-state-checker.tsx (1)
14-20:⚠️ Potential issue | 🔴 CriticalLogic error causes signed-in users to be redirected to error page.
The
elsebranch executes whenisSignedInistrue(or whenisLoadedisfalse), which incorrectly redirects authenticated users to the error page. The condition should handle the loading state and only redirect to error when loaded but in an unexpected state.🐛 Proposed fix
React.useEffect(() => { - if (isLoaded && !isSignedIn) { - push(`/login?return_to=${returnURL}`); - } else { - push("/error?error_code=BAD_SESSION"); + if (!isLoaded) { + return; } + if (!isSignedIn) { + push(`/login?return_to=${encodeURIComponent(returnURL ?? "/")}`); + } else { + // User is signed in but landed here — unexpected state + push("/"); + } }, [isLoaded, isSignedIn, push, returnURL]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pages/auth-state-checker.tsx` around lines 14 - 20, The current useEffect redirects authenticated users to the error page because the else branch runs when isSignedIn is true or when isLoaded is false; change the logic to check isLoaded first, then only redirect: inside React.useEffect, if (isLoaded) { if (!isSignedIn) push(`/login?return_to=${returnURL}`); else return; } and remove the unconditional else that pushes "/error"; if you need an error path, make it an explicit condition like (isLoaded && isSignedIn == null) to push the error. Ensure you update the useEffect dependencies (isLoaded, isSignedIn, push, returnURL) accordingly and reference the existing useEffect, isLoaded, isSignedIn, push, and returnURL symbols.src/components/Playbooks/Runs/Actions/PlaybooksActionsResults.tsx (1)
503-506:⚠️ Potential issue | 🟠 MajorAdd
noopener,noreferrertowindow.open()for the download button.Similar to the other file, this
window.open()call lacks security flags.🔒 Proposed fix
<Button onClick={() => { - window.open(downloadURL, "_blank"); + window.open(downloadURL, "_blank", "noopener,noreferrer"); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Playbooks/Runs/Actions/PlaybooksActionsResults.tsx` around lines 503 - 506, The onClick handler for the Button currently calls window.open(downloadURL, "_blank") without security flags; update the handler in PlaybooksActionsResults (where Button triggers window.open) to include noopener and noreferrer by calling window.open(downloadURL, "_blank", "noopener,noreferrer") (or alternatively open the URL via an anchor with rel="noopener noreferrer" and target="_blank") so the new tab cannot access the opener.
♻️ Duplicate comments (6)
src/components/Artifacts/ArtifactsSummaryCards.tsx (1)
7-11:⚠️ Potential issue | 🟡 MinorUse a unique key for storage cards instead of the display label.
The
storageKey(line 31-34) is unique, but only the displaylabelis stored inStorageBreakdownand used as the React key (line 91). When multiple external storage entries lack aconnection_name, they all get label"External", causing duplicate keys and broken React reconciliation.🔧 Proposed fix
type StorageBreakdown = { + key: string; label: string; count: number; size: number; };const existing = storageMap.get(storageKey) ?? { + key: storageKey, label: row.storage === "inline" ? "Inline (Database)" : (row.connection_name ?? "External"), count: 0, size: 0 };{summary.storage.map((s) => ( <SummaryCard - key={s.label} + key={s.key} title={s.label} value={`${s.count}`} subtitle={formatBytes(s.size)} /> ))}Also applies to: 31-45, 89-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Artifacts/ArtifactsSummaryCards.tsx` around lines 7 - 11, The StorageBreakdown type only contains display label, causing duplicate React keys when multiple external storages share the same label; add a unique identifier field (e.g., storageKey: string) to the StorageBreakdown type and populate it where you build the storage aggregates (the logic that currently sets storageKey, label, count, size in this file), then switch the React key usage from label to storageKey in the JSX (where the map renders the storage cards). Ensure any other places that construct or consume StorageBreakdown (references in this file) are updated to set and use storageKey so keys are unique even when labels are identical.src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsx (1)
44-48:⚠️ Potential issue | 🟠 MajorAdd
noopener,noreferrertowindow.open()to prevent reverse-tabnabbing.The
window.open()call on line 47 opens a new tab without thenoopener,noreferrerflags, allowing the opened page to accesswindow.opener. This is a security concern.🔒 Proposed fix
const { artifactDownloadURL } = await import( "@flanksource-ui/api/services/artifacts" ); - window.open(artifactDownloadURL(artifactID, fileName), "_blank"); + window.open( + artifactDownloadURL(artifactID, fileName), + "_blank", + "noopener,noreferrer" + ); return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsx` around lines 44 - 48, The window.open call in PlaybookResultsDropdownButton (where artifactDownloadURL(artifactID, fileName) is opened) lacks noopener/noreferrer which allows reverse-tabnabbing; update the open to include these flags (e.g., window.open(artifactDownloadURL(artifactID, fileName), "_blank", "noopener,noreferrer")) or, if you prefer broader compatibility, capture the returned window and set newWin.opener = null after opening; ensure the change is made where artifactDownloadURL is imported and invoked so the new tab cannot access window.opener.src/pages/Settings/ArtifactsPage.tsx (1)
119-146:⚠️ Potential issue | 🟡 MinorReset pagination when filters change.
These handlers preserve the current
pageIndex. Narrowing the result set from page 2+ can leave the table on an out-of-range page and show no rows even though matching artifacts exist.💡 Suggested fix
onChange={(e) => { const nextParams = new URLSearchParams(searchParams); + nextParams.delete("pageIndex"); if (e.target.value) { nextParams.set("search", e.target.value); } else { @@ onClear={() => { const nextParams = new URLSearchParams(searchParams); + nextParams.delete("pageIndex"); nextParams.delete("search"); setSearchParams(nextParams); }} @@ onChange={(value) => { const nextParams = new URLSearchParams(searchParams); + nextParams.delete("pageIndex"); if (!value || value === "all") { nextParams.delete("content_type"); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Settings/ArtifactsPage.tsx` around lines 119 - 146, The search and filter handlers (the inline onChange for the search input, onClear, and the ReactSelectDropdown onChange) currently preserve pagination; update each to also reset pagination by removing any existing page-related params before calling setSearchParams — e.g., delete "page" and "pageIndex" (or set to the first page) from the URLSearchParams so changing filters always returns to page 1; ensure you modify the same handlers that create nextParams and call setSearchParams.src/components/Artifacts/ArtifactPreviewModal.tsx (2)
41-46:⚠️ Potential issue | 🟠 MajorShorten the cache lifetime for preview content.
This modal can cache up to 50MB per artifact.
staleTime: Infinityplus a 24h inactive cache keeps transient preview blobs around far longer than needed and can bloat memory after a few opens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Artifacts/ArtifactPreviewModal.tsx` around lines 41 - 46, The query for preview content (useQuery with queryKey ["artifact", artifact?.id] and queryFn downloadArtifact(artifact!.id)) currently uses staleTime: Infinity and cacheTime of 24h which retains large transient blobs too long; change staleTime to a short value (e.g., 0 or a few seconds) and reduce cacheTime from 1000 * 60 * 60 * 24 to a much shorter period (e.g., 5–15 minutes) so preview blobs are released sooner while keeping the enabled condition (!!artifact && isSmallFile && !isImage) intact.
41-47:⚠️ Potential issue | 🟡 MinorDon't let preview failures and empty files render as blank space.
downloadArtifactreturns a string, so""is a valid payload. Right now empty artifacts fail the truthy check, and rejected queries also fall through to an empty body becauseisError/erroris never rendered.💡 Suggested fix
- const { data: content, isFetching: isLoading } = useQuery({ + const { + data: content, + isFetching: isLoading, + isError, + error + } = useQuery({ @@ <div className="max-h-[60vh] overflow-auto"> @@ {isLoading && ( <div className="flex items-center justify-center py-8"> <div className="h-6 w-6 animate-spin rounded-full border-b-2 border-blue-500" /> <span className="ml-2">Loading...</span> </div> )} + + {isError && ( + <div className="py-8 text-center text-red-600"> + Failed to load preview:{" "} + {error instanceof Error ? error.message : "Unknown error"} + </div> + )} @@ - {content && + {content !== undefined && artifact && !isImage && renderContent(artifact.content_type, content)}Also applies to: 67-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Artifacts/ArtifactPreviewModal.tsx` around lines 41 - 47, The query and render logic treat an empty string from downloadArtifact as falsy and never show error states; change the useQuery enabled check to explicitly test artifact != null (not artifact truthiness) and keep isSmallFile && !isImage, and in the render use content !== undefined (or content !== null) instead of a truthy check so empty strings render a clear "empty file" placeholder; also handle and render isError and error from useQuery (show the error message/UI) alongside existing isLoading handling so failed downloads don't render blank space.src/pages/config/details/ConfigDetailsChangesPage.tsx (1)
45-49:⚠️ Potential issue | 🟡 MinorClear
selectedChangewhen Graph view unmounts.The detail query stays enabled as long as
selectedChangeis set. If the user switches back to Table with a point selected, the hidden modal keeps fetching, and the stale selection comes back when Graph is reopened.💡 Suggested fix
-import { Suspense, lazy, useState } from "react"; +import { Suspense, lazy, 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: 85-94
🤖 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 45 - 49, SelectedChange remains set when the Graph view unmounts so the detail query (useGetConfigChangesById) keeps running and the stale selection resurfaces; fix by clearing selectedChange (call setSelectedChange(undefined)) when the Graph view or its modal unmounts or when switching back to Table. Add a useEffect in ConfigDetailsChangesPage that watches the Graph view visibility (or the component that mounts/unmounts the graph) and in its cleanup calls setSelectedChange(undefined), and also ensure any modal close handlers clear selectedChange; this will disable useGetConfigChangesById (enabled: !!selectedChange) and stop the stale fetches (apply same change to the other instance referenced around lines 85-94).
🧹 Nitpick comments (3)
src/components/Plugins/PluginsForm.tsx (1)
83-98: LGTM — error banner provides clear in-context feedback.The implementation with truncation and expand/collapse is user-friendly. The placement between form fields and action buttons is appropriate.
Optional accessibility improvement: Consider adding
role="alert"to announce errors to screen readers.- <div className="mx-4 rounded-md border border-red-300 bg-red-50 p-3 text-sm text-red-800"> + <div role="alert" className="mx-4 rounded-md border border-red-300 bg-red-50 p-3 text-sm text-red-800">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Plugins/PluginsForm.tsx` around lines 83 - 98, Add an accessibility role to the error banner so screen readers announce it: update the container that renders when errorMessage is truthy (the div that uses isErrorExpanded and displays {errorMessage}) to include role="alert"; keep the existing truncation/expand logic and the toggle button (which uses isErrorExpanded and setIsErrorExpanded) unchanged so behavior and state handling remain the same.src/ui/ToasterWithCloseButton.tsx (1)
25-26: Remove redundant wrapper<div>for cleaner markup.You can inline the
flexcontainer and drop one unnecessary DOM node.♻️ Suggested simplification
- <div> - <div className="flex items-start"> + <div className="flex items-start"> <div className="flex-shrink-0 pt-0.5"> {t.type === "success" && ( <FaCheckCircle className="h-5 w-5 text-green-400" /> @@ {t.type !== "loading" && ( <div className="ml-4 flex flex-shrink-0"> @@ )} - </div> </div>Also applies to: 58-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/ToasterWithCloseButton.tsx` around lines 25 - 26, In the ToasterWithCloseButton component remove the redundant wrapper <div> that simply contains the flex container: inline the className="flex items-start" (and any other similar wrapper around lines referenced) directly on the parent element so you drop the extra DOM node; update the JSX where the outer empty wrapper wraps the .flex container (also at the second occurrence around lines 58-60) to use the flex container as the immediate element instead of nesting it inside an extra div.src/api/axios.ts (1)
7-7: Consider importingisBasicAuthSystemfromuseDetermineAuthSystemto avoid duplication.The same constant is defined in both files. Importing from the single source would ensure consistency.
♻️ Suggested refactor
+import { isBasicAuthSystem } from "@flanksource-ui/components/Authentication/useDetermineAuthSystem"; + const isClerkAuthSystem = !!process.env.NEXT_PUBLIC_AUTH_IS_CLERK === true; -const isBasicAuthSystem = process.env.NEXT_PUBLIC_AUTH_IS_BASIC === "true";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/axios.ts` at line 7, Replace the duplicated local constant isBasicAuthSystem in this module with an import from the single source of truth (the exported value from useDetermineAuthSystem); remove the local declaration (const isBasicAuthSystem = ...) and import the exported symbol (named or default as declared in useDetermineAuthSystem), then update any usages in this file to use the imported isBasicAuthSystem so both modules reference the same value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pages/api/auth/login.ts`:
- Around line 13-14: The handler builds Basic auth from req.body without
validating inputs, so ensure username and password are present and non-empty
strings before creating basicAuth: check the destructured variables (username,
password) from req.body, validate (e.g., typeof === "string" and .trim() !==
""), and if invalid return an HTTP 400 response (with a clear error message)
instead of proceeding to construct the Buffer and forwarding; only build const
basicAuth = Buffer.from(`${username}:${password}`).toString("base64") after
validation succeeds.
In `@src/api/services/artifacts.ts`:
- Around line 49-51: The code interpolates filenameSearch directly into the
PostgREST ilike pattern (query.set("filename", `ilike.*${filenameSearch}*`)),
which can let special characters like *, %, . change the query; sanitize/escape
filenameSearch before building the pattern so PostgREST treats it as a literal
string (escape %, _, *, . and other meta chars used by ilike patterns and/or
URL-encode the value) and then call query.set("filename",
`ilike.*${escapedFilenameSearch}*`) instead of using the raw filenameSearch;
ensure the escaping utility is used wherever filenameSearch is accepted.
In `@src/components/Artifacts/ArtifactPreviewModal.tsx`:
- Around line 37-45: The preview currently enables downloadArtifact for any
small non-image file which will decode binaries as text; restrict previews to
text-like MIME types by adding a content-type check (e.g.,
content_type.startsWith("text/") or allow-list structured types like
application/json, application/xml, etc.) and use that together with isSmallFile
and !isImage as the enabled condition in the useQuery (refer to isSmallFile,
isImage, isImageContentType, downloadArtifact, MAX_PREVIEW_SIZE and the useQuery
block). For all other non-text small artifacts (including PDFs and other binary
application/* types) disable the preview query and render a download-only
fallback; apply this same change to the second preview block referenced around
lines 117-160.
- Around line 73-75: The download button is wrapped in an anchor which nests
interactive elements causing invalid HTML; in ArtifactPreviewModal update the
usage around downloadURL so the anchor is not wrapping a native <button>: either
render the Button as an anchor using its asChild/link API (or pass href/target
props if Button supports them) or replace the anchor with a styled anchor
element and move the icon/label into it, preserving target="_blank" and
rel="noreferrer"; apply the same change to the other occurrence that currently
wraps Button (the block referencing downloadURL and IoMdDownload).
---
Outside diff comments:
In `@pages/auth-state-checker.tsx`:
- Around line 14-20: The current useEffect redirects authenticated users to the
error page because the else branch runs when isSignedIn is true or when isLoaded
is false; change the logic to check isLoaded first, then only redirect: inside
React.useEffect, if (isLoaded) { if (!isSignedIn)
push(`/login?return_to=${returnURL}`); else return; } and remove the
unconditional else that pushes "/error"; if you need an error path, make it an
explicit condition like (isLoaded && isSignedIn == null) to push the error.
Ensure you update the useEffect dependencies (isLoaded, isSignedIn, push,
returnURL) accordingly and reference the existing useEffect, isLoaded,
isSignedIn, push, and returnURL symbols.
In `@src/components/Playbooks/Runs/Actions/PlaybooksActionsResults.tsx`:
- Around line 503-506: The onClick handler for the Button currently calls
window.open(downloadURL, "_blank") without security flags; update the handler in
PlaybooksActionsResults (where Button triggers window.open) to include noopener
and noreferrer by calling window.open(downloadURL, "_blank",
"noopener,noreferrer") (or alternatively open the URL via an anchor with
rel="noopener noreferrer" and target="_blank") so the new tab cannot access the
opener.
In `@src/pages/config/settings/ConfigPluginsPage.tsx`:
- Around line 218-223: The effect that runs when the modal open state changes
currently clears editedRow but not the formError, causing stale errors to
persist; update the same useEffect (the one using isOpen and setEditedRow) to
also clear the form error state (e.g., call setFormError(undefined) or
setFormError('')) when isOpen is false so that closing the modal resets both
editedRow and formError before the modal is reopened.
---
Duplicate comments:
In `@src/components/Artifacts/ArtifactPreviewModal.tsx`:
- Around line 41-46: The query for preview content (useQuery with queryKey
["artifact", artifact?.id] and queryFn downloadArtifact(artifact!.id)) currently
uses staleTime: Infinity and cacheTime of 24h which retains large transient
blobs too long; change staleTime to a short value (e.g., 0 or a few seconds) and
reduce cacheTime from 1000 * 60 * 60 * 24 to a much shorter period (e.g., 5–15
minutes) so preview blobs are released sooner while keeping the enabled
condition (!!artifact && isSmallFile && !isImage) intact.
- Around line 41-47: The query and render logic treat an empty string from
downloadArtifact as falsy and never show error states; change the useQuery
enabled check to explicitly test artifact != null (not artifact truthiness) and
keep isSmallFile && !isImage, and in the render use content !== undefined (or
content !== null) instead of a truthy check so empty strings render a clear
"empty file" placeholder; also handle and render isError and error from useQuery
(show the error message/UI) alongside existing isLoading handling so failed
downloads don't render blank space.
In `@src/components/Artifacts/ArtifactsSummaryCards.tsx`:
- Around line 7-11: The StorageBreakdown type only contains display label,
causing duplicate React keys when multiple external storages share the same
label; add a unique identifier field (e.g., storageKey: string) to the
StorageBreakdown type and populate it where you build the storage aggregates
(the logic that currently sets storageKey, label, count, size in this file),
then switch the React key usage from label to storageKey in the JSX (where the
map renders the storage cards). Ensure any other places that construct or
consume StorageBreakdown (references in this file) are updated to set and use
storageKey so keys are unique even when labels are identical.
In `@src/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsx`:
- Around line 44-48: The window.open call in PlaybookResultsDropdownButton
(where artifactDownloadURL(artifactID, fileName) is opened) lacks
noopener/noreferrer which allows reverse-tabnabbing; update the open to include
these flags (e.g., window.open(artifactDownloadURL(artifactID, fileName),
"_blank", "noopener,noreferrer")) or, if you prefer broader compatibility,
capture the returned window and set newWin.opener = null after opening; ensure
the change is made where artifactDownloadURL is imported and invoked so the new
tab cannot access window.opener.
In `@src/pages/config/details/ConfigDetailsChangesPage.tsx`:
- Around line 45-49: SelectedChange remains set when the Graph view unmounts so
the detail query (useGetConfigChangesById) keeps running and the stale selection
resurfaces; fix by clearing selectedChange (call setSelectedChange(undefined))
when the Graph view or its modal unmounts or when switching back to Table. Add a
useEffect in ConfigDetailsChangesPage that watches the Graph view visibility (or
the component that mounts/unmounts the graph) and in its cleanup calls
setSelectedChange(undefined), and also ensure any modal close handlers clear
selectedChange; this will disable useGetConfigChangesById (enabled:
!!selectedChange) and stop the stale fetches (apply same change to the other
instance referenced around lines 85-94).
In `@src/pages/Settings/ArtifactsPage.tsx`:
- Around line 119-146: The search and filter handlers (the inline onChange for
the search input, onClear, and the ReactSelectDropdown onChange) currently
preserve pagination; update each to also reset pagination by removing any
existing page-related params before calling setSearchParams — e.g., delete
"page" and "pageIndex" (or set to the first page) from the URLSearchParams so
changing filters always returns to page 1; ensure you modify the same handlers
that create nextParams and call setSearchParams.
---
Nitpick comments:
In `@src/api/axios.ts`:
- Line 7: Replace the duplicated local constant isBasicAuthSystem in this module
with an import from the single source of truth (the exported value from
useDetermineAuthSystem); remove the local declaration (const isBasicAuthSystem =
...) and import the exported symbol (named or default as declared in
useDetermineAuthSystem), then update any usages in this file to use the imported
isBasicAuthSystem so both modules reference the same value.
In `@src/components/Plugins/PluginsForm.tsx`:
- Around line 83-98: Add an accessibility role to the error banner so screen
readers announce it: update the container that renders when errorMessage is
truthy (the div that uses isErrorExpanded and displays {errorMessage}) to
include role="alert"; keep the existing truncation/expand logic and the toggle
button (which uses isErrorExpanded and setIsErrorExpanded) unchanged so behavior
and state handling remain the same.
In `@src/ui/ToasterWithCloseButton.tsx`:
- Around line 25-26: In the ToasterWithCloseButton component remove the
redundant wrapper <div> that simply contains the flex container: inline the
className="flex items-start" (and any other similar wrapper around lines
referenced) directly on the parent element so you drop the extra DOM node;
update the JSX where the outer empty wrapper wraps the .flex container (also at
the second occurrence around lines 58-60) to use the flex container as the
immediate element instead of nesting it inside an extra div.
🪄 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: a583ad43-ca15-4a18-8897-ba165dbea043
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (47)
.gitignoremiddleware.basic.tsmiddleware.clerk.tsmiddleware.kratos.tspackage.jsonpages/api/[...paths].tspages/api/auth/login.tspages/auth-state-checker.tsxpages/login/[[...index]].tsxsrc/App.tsxsrc/api/axios.tssrc/api/query-hooks/useFeatureFlags.tsxsrc/api/services/artifacts.tssrc/api/services/configs.tssrc/api/services/scrapePlugins.tssrc/api/types/artifacts.tssrc/api/types/configs.tssrc/components/Artifacts/ArtifactPreviewModal.tsxsrc/components/Artifacts/ArtifactsSummaryCards.tsxsrc/components/Artifacts/ArtifactsTable.tsxsrc/components/Authentication/AuthProviderWrapper.tsxsrc/components/Authentication/AuthSessionChecker.tsxsrc/components/Authentication/Basic/BasicAuthContextProvider.tsxsrc/components/Authentication/Basic/BasicAuthSessionChecker.tsxsrc/components/Authentication/Basic/BasicLogin.tsxsrc/components/Authentication/useDetermineAuthSystem.tsxsrc/components/Configs/Changes/ConfigChangesGraph.tsxsrc/components/Configs/Changes/ConfigChangesViewToggle.tsxsrc/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsxsrc/components/Configs/Changes/ConfigsRelatedChanges/FilterBar/ConfigRelatedChangesFilters.tsxsrc/components/Configs/Sidebar/ConfigsPanel.tsxsrc/components/Forms/Formik/FormikConnectionField.tsxsrc/components/Playbooks/Runs/Actions/PlaybookResultsDropdownButton.tsxsrc/components/Playbooks/Runs/Actions/PlaybooksActionsResults.tsxsrc/components/Plugins/PluginsForm.tsxsrc/components/Plugins/PluginsFormModal.tsxsrc/components/Toast/toast.tssrc/context/UserAccessContext/permissions.tssrc/pages/Settings/ArtifactsPage.tsxsrc/pages/Settings/ConnectionsPage.tsxsrc/pages/applications/ApplicationsPage.tsxsrc/pages/config/details/ConfigDetailsChangesPage.tsxsrc/pages/config/settings/ConfigPluginsPage.tsxsrc/pages/views/ViewsPage.tsxsrc/services/permissions/features.tssrc/ui/SkeletonLoader/FullPageSkeletonLoader.tsxsrc/ui/ToasterWithCloseButton.tsx
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- src/api/services/configs.ts
- src/api/types/artifacts.ts
- src/components/Configs/Changes/ConfigChangesViewToggle.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Configs/Changes/ConfigsRelatedChanges/FilterBar/ConfigRelatedChangesFilters.tsx
- package.json
- src/components/Configs/Changes/ConfigChangesGraph.tsx
Comment on lines
+13
to
+14
| const { username, password } = req.body; | ||
| const basicAuth = Buffer.from(`${username}:${password}`).toString("base64"); |
Contributor
There was a problem hiding this comment.
Missing input validation for credentials.
If username or password is undefined or missing from the request body, the code proceeds to build invalid Basic auth credentials (undefined:undefined) and forwards them to the backend. Validate inputs before processing.
🛡️ Proposed validation
const { username, password } = req.body;
+
+ if (typeof username !== "string" || typeof password !== "string") {
+ return res.status(400).json({ error: "Username and password are required" });
+ }
+
const basicAuth = Buffer.from(`${username}:${password}`).toString("base64");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/api/auth/login.ts` around lines 13 - 14, The handler builds Basic auth
from req.body without validating inputs, so ensure username and password are
present and non-empty strings before creating basicAuth: check the destructured
variables (username, password) from req.body, validate (e.g., typeof ===
"string" and .trim() !== ""), and if invalid return an HTTP 400 response (with a
clear error message) instead of proceeding to construct the Buffer and
forwarding; only build const basicAuth =
Buffer.from(`${username}:${password}`).toString("base64") after validation
succeeds.
Comment on lines
+49
to
+51
| if (filenameSearch) { | ||
| query.set("filename", `ilike.*${filenameSearch}*`); | ||
| } |
Contributor
There was a problem hiding this comment.
Sanitize filenameSearch to escape PostgREST special characters.
The filenameSearch value is interpolated directly into the PostgREST ilike pattern. If user input contains characters like *, ., or %, this could cause unexpected query behavior or errors.
🛡️ Proposed fix
if (filenameSearch) {
- query.set("filename", `ilike.*${filenameSearch}*`);
+ // Escape PostgREST pattern special characters
+ const escaped = filenameSearch.replace(/[%_*]/g, "\\$&");
+ query.set("filename", `ilike.*${escaped}*`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/services/artifacts.ts` around lines 49 - 51, The code interpolates
filenameSearch directly into the PostgREST ilike pattern (query.set("filename",
`ilike.*${filenameSearch}*`)), which can let special characters like *, %, .
change the query; sanitize/escape filenameSearch before building the pattern so
PostgREST treats it as a literal string (escape %, _, *, . and other meta chars
used by ilike patterns and/or URL-encode the value) and then call
query.set("filename", `ilike.*${escapedFilenameSearch}*`) instead of using the
raw filenameSearch; ensure the escaping utility is used wherever filenameSearch
is accepted.
Comment on lines
+37
to
+45
| const isSmallFile = artifact ? artifact.size < MAX_PREVIEW_SIZE : false; | ||
|
|
||
| const isImage = artifact ? isImageContentType(artifact.content_type) : false; | ||
|
|
||
| const { data: content, isFetching: isLoading } = useQuery({ | ||
| queryKey: ["artifact", artifact?.id], | ||
| queryFn: () => downloadArtifact(artifact!.id), | ||
| enabled: !!artifact && isSmallFile && !isImage, | ||
| staleTime: Infinity, |
Contributor
There was a problem hiding this comment.
Only inline-preview text-like MIME types.
The query is enabled for every small non-image artifact, but downloadArtifact always requests text. PDFs and other binary formats will be decoded as text and rendered as garbage here; gate the preview path to text/structured-text types and fall back to download-only for the rest.
Also applies to: 117-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Artifacts/ArtifactPreviewModal.tsx` around lines 37 - 45, The
preview currently enables downloadArtifact for any small non-image file which
will decode binaries as text; restrict previews to text-like MIME types by
adding a content-type check (e.g., content_type.startsWith("text/") or
allow-list structured types like application/json, application/xml, etc.) and
use that together with isSmallFile and !isImage as the enabled condition in the
useQuery (refer to isSmallFile, isImage, isImageContentType, downloadArtifact,
MAX_PREVIEW_SIZE and the useQuery block). For all other non-text small artifacts
(including PDFs and other binary application/* types) disable the preview query
and render a download-only fallback; apply this same change to the second
preview block referenced around lines 117-160.
Comment on lines
+73
to
+75
| <a href={downloadURL} target="_blank" rel="noreferrer"> | ||
| <Button text="Download" icon={<IoMdDownload />} /> | ||
| </a> |
Contributor
There was a problem hiding this comment.
Avoid wrapping the download button inside a link.
If Button renders a native <button>, both download actions become nested interactive elements. That's invalid HTML and can cause keyboard/focus issues. Prefer a styled anchor or the Button component's link/asChild API instead.
Also applies to: 101-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Artifacts/ArtifactPreviewModal.tsx` around lines 73 - 75, The
download button is wrapped in an anchor which nests interactive elements causing
invalid HTML; in ArtifactPreviewModal update the usage around downloadURL so the
anchor is not wrapping a native <button>: either render the Button as an anchor
using its asChild/link API (or pass href/target props if Button supports them)
or replace the anchor with a styled anchor element and move the icon/label into
it, preserving target="_blank" and rel="noreferrer"; apply the same change to
the other occurrence that currently wraps Button (the block referencing
downloadURL and IoMdDownload).
Implement 10-second timeout on auth service requests and proper error responses for timeouts and connection failures. Also refactors return_to parameter handling to reduce code duplication and normalize severity values in change icons to handle case-insensitive comparisons.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/Configs/Changes/ConfigChangesGraph.tsx (1)
34-39:⚠️ Potential issue | 🟡 MinorDon't silently omit rows that lack
first_observed.Line 35 filters these records out entirely. Since
first_observedis optional, Graph can disagree with Table and hide valid changes instead of plotting them at a fallback timestamp. Preferchange.first_observed ?? change.created_atand a label fallback like"Unknown config"so the dataset stays complete.🤖 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 34 - 39, In the mapping that builds the graph dataset in ConfigChangesGraph (the .filter(...).map(...) block), stop dropping rows missing first_observed and instead use a fallback timestamp and label: replace the strict filter with one that only ensures a config exists (or remove the filter) and set key to change.first_observed ?? change.created_at and data to change.config?.name ?? "Unknown config", keeping metadata as change so the graph and table remain consistent.src/pages/config/details/ConfigDetailsChangesPage.tsx (1)
76-108:⚠️ Potential issue | 🟠 MajorGraph view still hides changes beyond the current page.
This branch renders the paginated
changesslice, but it drops the pagination UI/props that the table gets. Once a config has more than one page of history, page 2+ never appears in Graph view. Either fetch the full result set forview === "Graph"or add equivalent pagination controls here.🤖 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 76 - 108, The graph view currently receives only the paginated changes slice (changes) and thus hides pages beyond the current page; update the logic so that when view === "Graph" you either request the full changes result set (e.g., modify the data-fetching hook/useEffect to fetch all pages when rendering ConfigChangesGraph) or add equivalent pagination controls and props to the graph area (reuse totalChangesPages, totalChanges, isLoading and a page setter) so ConfigChangesGraph can display/navigate all pages; make changes around the ConfigChangesGraph usage and the data-fetch hook to ensure selectedChange, changeLoading and changeDetails behavior remains unchanged.
🧹 Nitpick comments (2)
src/components/Authentication/Kratos/KratosLogin.tsx (1)
9-9: Remove empty import statement.This empty import from
@tanstack/react-queryserves no purpose and appears to be a leftover from refactoring.🧹 Suggested fix
-import {} from "@tanstack/react-query";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Authentication/Kratos/KratosLogin.tsx` at line 9, Remove the unnecessary empty import statement "import {} from '@tanstack/react-query';" in KratosLogin.tsx; delete that line and, if any react-query hooks (e.g., useQuery, useMutation) are actually used in the KratosLogin component, replace the empty import with a proper named import (for example import { useMutation } from '@tanstack/react-query') to only import what’s needed.src/pages/config/ConfigChangesPage.tsx (1)
39-47: Extract this config-change normalizer.This shaping logic now exists here and in
src/pages/config/details/ConfigDetailsChangesPage.tsx, and it has already started to drift (deleted_atis only kept in one copy). A shared helper would keep the table/graph inputs consistent and make future fixes land in one place.🤖 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 39 - 47, Extract the mapping logic that builds the normalized config object into a shared helper (e.g., export function normalizeConfigChange(change): NormalizedChange) and replace the inline mapping in ConfigChangesPage (variable changes) and in ConfigDetailsChangesPage to call normalizeConfigChange(c); ensure the helper returns { ...change, config: { id: change.config_id ?? "", type: change.type ?? change.config?.type ?? "", name: change.name ?? change.config?.name ?? "", deleted_at: change.deleted_at } } so deleted_at is preserved consistently across consumers and update imports accordingly.
🤖 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/ConfigChangesFilters/ConfigChangeSeverity.tsx`:
- Line 60: ConfigChangeSeverity now uses the canonical value "info", but inbound
URLs may still carry the legacy "Info"; update the query-param reading logic
inside the ConfigChangeSeverity component to normalize incoming severity strings
(e.g., map "Info" -> "info", case-insensitively) before using them to set the
filter state or to build API calls so existing deep links keep working. Locate
where the component reads/parses severity from the URL or initial props and add
a small normalization step that lowercases or explicitly maps legacy values to
the new canonical values before applying them.
---
Duplicate comments:
In `@src/components/Configs/Changes/ConfigChangesGraph.tsx`:
- Around line 34-39: In the mapping that builds the graph dataset in
ConfigChangesGraph (the .filter(...).map(...) block), stop dropping rows missing
first_observed and instead use a fallback timestamp and label: replace the
strict filter with one that only ensures a config exists (or remove the filter)
and set key to change.first_observed ?? change.created_at and data to
change.config?.name ?? "Unknown config", keeping metadata as change so the graph
and table remain consistent.
In `@src/pages/config/details/ConfigDetailsChangesPage.tsx`:
- Around line 76-108: The graph view currently receives only the paginated
changes slice (changes) and thus hides pages beyond the current page; update the
logic so that when view === "Graph" you either request the full changes result
set (e.g., modify the data-fetching hook/useEffect to fetch all pages when
rendering ConfigChangesGraph) or add equivalent pagination controls and props to
the graph area (reuse totalChangesPages, totalChanges, isLoading and a page
setter) so ConfigChangesGraph can display/navigate all pages; make changes
around the ConfigChangesGraph usage and the data-fetch hook to ensure
selectedChange, changeLoading and changeDetails behavior remains unchanged.
---
Nitpick comments:
In `@src/components/Authentication/Kratos/KratosLogin.tsx`:
- Line 9: Remove the unnecessary empty import statement "import {} from
'@tanstack/react-query';" in KratosLogin.tsx; delete that line and, if any
react-query hooks (e.g., useQuery, useMutation) are actually used in the
KratosLogin component, replace the empty import with a proper named import (for
example import { useMutation } from '@tanstack/react-query') to only import
what’s needed.
In `@src/pages/config/ConfigChangesPage.tsx`:
- Around line 39-47: Extract the mapping logic that builds the normalized config
object into a shared helper (e.g., export function
normalizeConfigChange(change): NormalizedChange) and replace the inline mapping
in ConfigChangesPage (variable changes) and in ConfigDetailsChangesPage to call
normalizeConfigChange(c); ensure the helper returns { ...change, config: { id:
change.config_id ?? "", type: change.type ?? change.config?.type ?? "", name:
change.name ?? change.config?.name ?? "", deleted_at: change.deleted_at } } so
deleted_at is preserved consistently across consumers and update imports
accordingly.
🪄 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: c44d36b6-f0ea-49ec-815c-5170bbeb5244
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonpages/api/auth/login.tssrc/api/axios.tssrc/components/Authentication/Kratos/KratosLogin.tsxsrc/components/Configs/Changes/ConfigChangesFilters/ConfigChangeSeverity.tsxsrc/components/Configs/Changes/ConfigChangesGraph.tsxsrc/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsxsrc/pages/config/ConfigChangesPage.tsxsrc/pages/config/details/ConfigDetailsChangesPage.tsxsrc/ui/Icons/ChangeIcon.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/Configs/Changes/ConfigDetailsChanges/ConfigDetailsChanges.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- pages/api/auth/login.ts
| name: "Info", | ||
| description: "Info", | ||
| value: "Info", | ||
| value: "info", |
Contributor
There was a problem hiding this comment.
Normalize legacy "Info" values on read.
Line 60 changes the canonical filter value, but older URLs/bookmarks that still carry severity=Info will now flow through unchanged and stop matching the API/filter logic. Please normalize inbound severity params as well so existing deep links keep working.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Changes/ConfigChangesFilters/ConfigChangeSeverity.tsx`
at line 60, ConfigChangeSeverity now uses the canonical value "info", but
inbound URLs may still carry the legacy "Info"; update the query-param reading
logic inside the ConfigChangeSeverity component to normalize incoming severity
strings (e.g., map "Info" -> "info", case-insensitively) before using them to
set the filter state or to build API calls so existing deep links keep working.
Locate where the component reads/parses severity from the URL or initial props
and add a small normalization step that lowercases or explicitly maps legacy
values to the new canonical values before applying them.
Adds a view toggle to switch between table and graph representations of config changes. Graph view uses lazy loading and displays change details in a modal when an item is selected.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/config/ConfigChangesPage.tsx (1)
124-156:⚠️ Potential issue | 🟠 MajorGraph view still drops results beyond the current page.
changesis still coming from the same paginated query as the table, but only the table branch gets navigation vianumberOfPages/totalRecords. In Graph mode, page 2+ becomes unreachable, so the scatter plot can silently omit part of the filtered result set. Please either fetch the full filtered dataset whenview === "Graph"or add graph-side pagination controls.🤖 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 124 - 156, The Graph branch is using the same paginated `changes` as the table so results beyond page 1 are dropped; update the component so when `view === "Graph"` you either fetch the full filtered dataset (e.g., call a new helper like `fetchAllChanges` or request with a large `pageSize` instead of the paginated hook) and pass that complete array into `ConfigChangesGraph`, or add pagination controls to `ConfigChangesGraph` and wire them to `totalChanges`/`totalChangesPages`; locate the view switch around `view`, the `changes` variable, `ConfigChangesGraph`, and `ConfigChangeTable` and implement one of these two fixes so the graph shows the entire filtered result set (or supports paging) rather than only the current page.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pages/config/ConfigChangesPage.tsx`:
- Around line 124-156: The Graph branch is using the same paginated `changes` as
the table so results beyond page 1 are dropped; update the component so when
`view === "Graph"` you either fetch the full filtered dataset (e.g., call a new
helper like `fetchAllChanges` or request with a large `pageSize` instead of the
paginated hook) and pass that complete array into `ConfigChangesGraph`, or add
pagination controls to `ConfigChangesGraph` and wire them to
`totalChanges`/`totalChangesPages`; locate the view switch around `view`, the
`changes` variable, `ConfigChangesGraph`, and `ConfigChangeTable` and implement
one of these two fixes so the graph shows the entire filtered result set (or
supports paging) rather than only the current page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d76c4bd9-51a9-4dd8-8aef-1342e1975df9
📒 Files selected for processing (2)
src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesFilters.tsxsrc/pages/config/ConfigChangesPage.tsx
✅ Files skipped from review due to trivial changes (1)
- src/components/Configs/Changes/ConfigChangesFilters/ConfigChangesFilters.tsx
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
New Features
Enhancements