[DEV-IMP] Dashboard UI Enhancements and Performance Improvements#87
Open
[DEV-IMP] Dashboard UI Enhancements and Performance Improvements#87
Conversation
…es anymore, will see if it's an issue in teh future uses)
📝 WalkthroughWalkthroughImplements list virtualization for event and table views, refactors dashboard metric UI and input control behavior, adds metric design tokens and border/z-index tweaks, and adjusts various layout, sizing, and data-shaping changes across multiple UI components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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)
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 |
moda20
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/custom/general/LogFileList.tsx (1)
28-46:⚠️ Potential issue | 🟠 MajorComplete the
useMemodependency list to avoid stale props.
props.originNameandprops.downloadLogFileare used inside the memoized callback but omitted from the dependency array. When these optional props change, the memoizedFileListwill not be recalculated, causing stale values in the rendered items.💡 Proposed fix
- }, [props.logFiles, props.readLogFile]) + }, [ + props.logFiles, + props.readLogFile, + props.downloadLogFile, + props.originName, + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/LogFileList.tsx` around lines 28 - 46, The memoized FileList created by useMemo currently lists [props.logFiles, props.readLogFile] but omits props.originName and props.downloadLogFile, causing stale values when those props change; update the useMemo dependency array for the FileList variable (the useMemo call that builds FileItemProps) to include props.originName and props.downloadLogFile along with props.logFiles and props.readLogFile so the memo recalculates when any of those inputs change.
🤖 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/custom/dashboard/statsBar.tsx`:
- Around line 148-173: The two MetricSection bindings are inconsistent: change
the Scheduled Jobs MetricSection to read the scheduled count from jobMetrics
(use jobMetrics.scheduled_jobs_count) instead of jobMetrics.running_jobs_count,
and change the Running Jobs MetricSection to use the snake_case running count
(jobMetrics.running_jobs_count) instead of jobMetrics.runningJobsCount so both
metrics match the payload's snake_case keys used elsewhere (e.g., job_count).
- Around line 47-50: The runtime-built Tailwind class `${bgColorLight}/5` in the
className passed to cn prevents Tailwind from generating the background opacity
utility; change the component API to accept a fully-qualified tint class (e.g.,
"bg-metric1-light/5") instead of bgColorLight and use that prop directly in the
className; update references to the prop name (e.g., tintClass or bgTint) in the
component where cn is used (and anywhere StatsBar is instantiated) and remove
any runtime string interpolation that appends "/5", preserving the hasError
branch that uses "bg-destructive/5".
In `@src/components/custom/DrawerJobEvents.tsx`:
- Around line 74-83: The virtualizer is using index-based identity which causes
remounts when latestEvents are prepended; update the useVirtualizer call that
creates rowVirtualizer to include a getItemKey function that returns a stable
unique identifier from each event object in allEvents (e.g., event.id or
event.eventId, falling back to a composite of timestamp+type only if no stable
id exists) so items keep stable identity when latestEvents is prepended and you
can remove the timeout remount workaround; reference the useVirtualizer call
(rowVirtualizer) and the allEvents/latestEvents arrays when implementing this
change.
In `@src/components/custom/DrawerLatestRuns.tsx`:
- Around line 141-144: The code dereferences the resolved value of
getLatestRuns() (and similarly getMoreRuns()) without guarding against its
error-path fallback (which can resolve to []). Update the promise handlers for
getLatestRuns() and getMoreRuns() so they first check the resolved value and
provide a safe default (e.g., an object with data: [] and total: 0) before
calling setLogItems and setItemsTotal; alternatively, destructure with defaults
or early-return when data is falsy so the drawer falls back to the empty state
instead of crashing. Ensure you modify the promise then() callbacks where
setLogItems and setItemsTotal are invoked and handle both success shape and the
error fallback consistently.
In `@src/components/custom/general/LogFileList.tsx`:
- Line 36: The createAt value is being set using logFile.date?.toString(), which
yields a decimal string that moment() may misparse; update the code that assigns
createAt (used by FileItem) to pass a parse-safe ISO or numeric timestamp
instead—e.g., convert the numeric Unix timestamp to a Number or an ISO string
before assigning (use logFile.date as a Number or new
Date(logFile.date).toISOString()), so FileItem/moment() receives a reliably
parseable date; locate the createAt assignment in LogFileList and replace the
toString() usage accordingly.
In `@src/components/custom/general/MultiTypedInput.tsx`:
- Line 57: The change removed the default for the MultiTypedInput "type" prop
which unintentionally exposes the mode picker (see MultiTypedInput component
around the mode-picker logic at lines ~225-235) and allows "exact"/"regex"
values for fields like averageTime; to fix, restore the previous default type
behavior in MultiTypedInput (so the picker is hidden and "between"-style remains
the only selection) or update callers that expect only "between" (notably
advancedJobFilteringDialog's averageTime usage) to explicitly pass
type="between" or acceptedInputTypes={["between"]} so the UI cannot produce
invalid payloads.
- Around line 136-149: The handleRegexChange callback currently only reports
validation errors via onError when validateRegex(newValue) returns an error, but
never clears a previous error when the pattern becomes valid; update
handleRegexChange (and use validateRegex) so that after computing error you call
onError?.({ name }, error) when error exists and otherwise call onError?.({ name
}, undefined) (or null per your onError contract) to clear the stale error, then
proceed to setRegexValue and onChange as before.
- Around line 153-160: handleBetweenChange closes over the stale betweenValue;
change it to use a functional state update and remove betweenValue from the
dependency array: inside handleBetweenChange (the function defined with
useCallback) call setBetweenValue(prev => { const updated = { ...prev, [field]:
newValue, type: "between" }; onChange?.(updated); return updated; }) and keep
only onChange and setBetweenValue in the useCallback deps so the callback never
reads a stale betweenValue.
- Around line 102-117: handleTypeChange currently resets state and emits
payloads with the wrong shape when a controlled type prop is synced, and its
dependencies are incomplete; change the sync effect that reads prop `type` to
set `inputType` directly (do not call handleTypeChange on mount/prop-sync) so it
won’t reset values or emit mismatched payloads, and update handleTypeChange to
emit type-correct payloads (include the proper `type` field for exact/regex and
`{ type: "between", value1, value2 }` for between) when invoked by user actions;
also add the missing dependencies: include `name` in handleTypeChange, include
`betweenValue` in handleBetweenChange, and add `inputType` and
`handleTypeChange` to the effect that syncs controlled `type` prop so hooks
don’t use stale closures.
In `@src/components/custom/jobsTable/advancedJobFilteringDialog.tsx`:
- Around line 218-220: The current code returns the full FlexibleInput objects
(e.g., values.name, values.cronSetting, values.consumer) which contain {value,
type} instead of scalar strings, causing advancedFilters to propagate nested
objects into JobsService (affecting filterJobs and exportJobsToJSON); change the
assignments so they return the scalar text (e.g., use values.name?.value ?
values.name.value : undefined for name, and likewise for cronSetting and
consumer) so advancedFilters sends plain strings to the service.
In `@src/components/custom/notifications/NotificationServicesPanel.tsx`:
- Line 385: The details pane wrapper in NotificationServicesPanel (the div with
class "flex flex-col gap-2 overflow-y-auto") needs a bounded height so it
becomes the scroll container; update that element to include a height constraint
(e.g., add "h-full" or "min-h-0") in its class list so it doesn't auto-size and
the overflow-y works, and if the Spinner component wraps children with its own
container apply the same "min-h-0" constraint to the Spinner wrapper so the
scroll behavior remains contained.
In `@src/components/ui/date-picker-presets.tsx`:
- Line 147: The border classes are on the wrong element; remove "border-2
border-border" from the SelectValue className and add those classes to the
SelectTrigger className so the trigger renders the control chrome while
SelectValue keeps only text/placeholder styles (e.g., keep "bg-background
text-foreground" on SelectValue and ensure SelectTrigger receives "border-2
border-border" alongside its existing classes).
In `@src/components/ui/sidebar.tsx`:
- Line 225: The desktop sidebar's root element currently uses the class string
"group peer hidden md:block text-sidebar-foreground z-50" which conflicts with
modal/sheet/dropdown z-50 layers; update the sidebar's className in
src/components/ui/sidebar.tsx (the element with that exact string) to use a
lower z-index (e.g., z-40 or remove the z-* utility) so dialogs/sheets/menus
(which use z-50) deterministically render above the Sidebar.
In `@src/features/jobsTable/interfaces.tsx`:
- Around line 113-116: The flex container currently has the truncation class
which won't ellipsize because the flex item needs a shrinkable basis; update the
JSX so the outer div keeps "flex gap-2 text-[13px] items-center font-light"
(remove "truncate") and add "min-w-0" to that outer div, then move "truncate"
(and optionally "overflow-hidden" and "whitespace-nowrap" if used elsewhere) to
the consumer text node that renders row.original.consumer (the element alongside
<FileArchive />) so long consumer names properly ellipsize.
In `@src/features/jobsTable/jobsTable.tsx`:
- Around line 71-75: The empty-state branch currently applies min-h-[800px] when
data.length === 0 causing an oversized blank panel; change the conditional class
logic inside the component that builds className (the cn call using size and
data.length) to apply a bounded, compact empty-state height (for example replace
"min-h-[800px]" with a smaller bounded class such as "min-h-[160px] h-auto
max-h-[40vh] flex items-center justify-center" or similar design-system
empty-state classes) so the table stays within the viewport-bounded area and
visually shows a no-results state; make the same adjustment to the other
identical block that handles the empty rows (the second cn/className usage
around the lines referenced).
In `@src/hooks/useJobEvents.tsx`:
- Around line 122-125: The callbacks in useJobEvents.tsx (notably readEvents
plus the callbacks at the .finally block and the other two callbacks referenced)
close over a stale changeCounter; replace setChangeCounter(changeCounter + 1)
with the functional updater setChangeCounter(c => c + 1) in each location so
updates are always based on the latest state, and ensure readEvents and the
other callbacks no longer rely on changeCounter in their dependency arrays (use
the functional updater instead of adding changeCounter to deps); look for the
symbol readEvents and the three occurrences of setChangeCounter in the file and
update them to use the functional form.
---
Outside diff comments:
In `@src/components/custom/general/LogFileList.tsx`:
- Around line 28-46: The memoized FileList created by useMemo currently lists
[props.logFiles, props.readLogFile] but omits props.originName and
props.downloadLogFile, causing stale values when those props change; update the
useMemo dependency array for the FileList variable (the useMemo call that builds
FileItemProps) to include props.originName and props.downloadLogFile along with
props.logFiles and props.readLogFile so the memo recalculates when any of those
inputs change.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a765adcb-6c53-4c4e-b723-c7a62adadd23
📒 Files selected for processing (15)
src/components/custom/DrawerJobEvents.tsxsrc/components/custom/DrawerLatestRuns.tsxsrc/components/custom/dashboard/statsBar.tsxsrc/components/custom/general/LogFileList.tsxsrc/components/custom/general/MultiTypedInput.tsxsrc/components/custom/jobsTable/advancedJobFilteringDialog.tsxsrc/components/custom/notifications/NotificationServicesPanel.tsxsrc/components/ui/date-picker-presets.tsxsrc/components/ui/sidebar.tsxsrc/features/jobsTable/interfaces.tsxsrc/features/jobsTable/jobsPage.tsxsrc/features/jobsTable/jobsTable.tsxsrc/hooks/useJobEvents.tsxsrc/styles/global.csstailwind.config.ts
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/custom/jobsTable/advancedJobFilteringDialog.tsx (1)
301-308:⚠️ Potential issue | 🟠 Major
FlexibleInputwithtype="regex"will reset pre-filled values on mount.Setting
type="regex"triggers theuseEffectinMultiTypedInput.tsx(lines 162-166) which callshandleTypeChange, resetting all internal state and emitting an empty value. If the form is opened with existing filter values, they will be silently wiped.This is a downstream effect of the issue in
MultiTypedInput.tsx. Once the root cause is fixed there (by syncinginputTypedirectly instead of callinghandleTypeChange), this usage will work correctly.Also applies to: 321-328, 341-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/jobsTable/advancedJobFilteringDialog.tsx` around lines 301 - 308, The FlexibleInput usages (e.g., the instance with type="regex" in advancedJobFilteringDialog) are getting their pre-filled values wiped because MultiTypedInput.tsx's useEffect invokes handleTypeChange (lines around 162-166), which resets internal state and emits an empty value; fix MultiTypedInput by syncing the internal inputType state directly from props (set inputType = props.type or similar) in that effect instead of calling handleTypeChange so you don't reset other internal state or call the value-emitting logic; ensure handleTypeChange remains the explicit user-triggered transition and that existing field.value is preserved and not overwritten when the component mounts.src/features/jobsTable/jobsTable.tsx (1)
78-102:⚠️ Potential issue | 🟠 MajorRemove
w-fullfrom body cells or replaceflex-growwithflex-shrink-0in both headers and body to enforce strict column sizing.Body cells (lines 185-187) have conflicting width constraints:
w-fullconflicts with the inlinestyle={{ width: cell.column.getSize() }}. Headers lackw-full, creating inconsistency. Combined with mixed column sizing (some columns use fixedsize, others use onlyminSize), theflex-growproperty allows flex-basis to override explicit widths, causing subtle misalignment.Remove
w-fullfrom body cells to match headers, or useflex-shrink-0in both to enforce strict sizing without proportional expansion.Also applies to: 181-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/jobsTable/jobsTable.tsx` around lines 78 - 102, Table cells are misaligning because body cells use the tailwind class "w-full" while headers use "flex flex-grow" and widths are set via header.column.getSize()/cell.column.getSize(), causing flex-basis to be overridden; remove "w-full" from the body cell elements (the TableCell elements that use style={{ width: cell.column.getSize() }}) so they mirror the TableHead layout, or alternatively change both header and body containers from "flex-grow" to "flex-shrink-0" (update classNames on TableRow/TableHead and the corresponding TableCell) to enforce strict sizing based on column.getSize().
♻️ Duplicate comments (7)
src/components/custom/general/MultiTypedInput.tsx (4)
112-117:⚠️ Potential issue | 🟠 MajorEmitted payloads have incorrect
typefield.When
newTypeis"regex", line 114 emitsdefaultMultiTypeNullValuewhich hardcodestype: "exact"(see line 28). The emitted object'stypeshould matchnewType.Suggested fix
// Emit initial value for new type if (newType === "exact" || newType === "regex") { - onChange?.(defaultMultiTypeNullValue) + onChange?.({ value: "", type: newType }) } else if (newType === "between") { - onChange?.({ value1: "", value2: "" }) + onChange?.({ value1: "", value2: "", type: "between" }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 112 - 117, The emitted payload uses defaultMultiTypeNullValue which hardcodes type:"exact", so when newType is "regex" (or other types) the emitted object's type is wrong; update the emission logic in the onChange block (the code referencing newType, defaultMultiTypeNullValue, and onChange) to emit an object whose type field equals newType (e.g., construct { type: newType, value: ... } or clone defaultMultiTypeNullValue but set type: newType) and for "between" continue to emit { value1: "", value2: "" } as before.
162-166:⚠️ Potential issue | 🔴 CriticalControlled
typeprop causes value reset on mount.When a parent passes
type="regex"along with a pre-filledvalue(as inadvancedJobFilteringDialog.tsxlines 301-308), thisuseEffectfires on mount and callshandleTypeChange(type). That handler resets all internal state (lines 107-109) and emits an empty value viaonChange, silently wiping the pre-filledfield.value.Synchronize
inputTypedirectly without invoking the reset logic:Suggested fix
useEffect(() => { - if (type) { - handleTypeChange(type) + if (type && type !== inputType) { + setInputType(type) } - }, [type, handleTypeChange]) + }, [type, inputType])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 162 - 166, The useEffect currently calls handleTypeChange(type) on mount which triggers the reset logic (clearing internal state and calling onChange); instead, update the internal inputType state directly without invoking reset/emit: in the component MultiTypedInput, replace calling handleTypeChange from the useEffect with a direct state sync (e.g., setInputType(type) or the existing inputType setter) so the controlled prop synchronizes the UI but does not call handleTypeChange or emit an empty value; alternatively, add an optional flag parameter to handleTypeChange (e.g., handleTypeChange(type, { preserveValue: true })) and call that from the useEffect to avoid clearing value and calling onChange.
153-160:⚠️ Potential issue | 🔴 CriticalStale closure on
betweenValuecauses lost edits.
handleBetweenChangespreadsbetweenValue(line 155) but the dependency array includes it, creating a new callback on every state change. Worse, if React batches updates, the closure can still reference stale state. Use functionalsetStateto avoid the stale-closure bug entirely.Suggested fix
const handleBetweenChange = useCallback( (field: keyof BetweenValue, newValue: string) => { - const updated = { ...betweenValue, [field]: newValue, type: "between" } - setBetweenValue(updated) - onChange?.(updated) + setBetweenValue(prev => { + const updated = { ...prev, [field]: newValue, type: "between" } + onChange?.(updated) + return updated + }) }, - [onChange, setBetweenValue, betweenValue], + [onChange], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 153 - 160, The handleBetweenChange callback closes over betweenValue causing stale updates; change it to use the functional form of setBetweenValue so you compute the new state from the previous value (e.g. setBetweenValue(prev => ({ ...prev, [field]: newValue, type: "between" }))) and call onChange with that computed updated object; then remove betweenValue from the useCallback dependency array so the callback only depends on onChange and setBetweenValue (reference: handleBetweenChange, setBetweenValue, betweenValue, onChange).
143-147:⚠️ Potential issue | 🟠 MajorStale regex error persists after pattern is corrected.
onErroris only called whenvalidateRegexreturns an error. Once the user fixes the pattern, the previous error is never cleared.Suggested fix
const error = validateRegex(newValue) - if (error) { - onError?.({ name }, error) - } + onError?.({ name }, error) // clears when error is undefined onChange?.(typedValue)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 143 - 147, validateRegex is only used to call onError when an error exists, so once the user corrects the pattern the previous error remains; after computing error = validateRegex(newValue) call onError with a cleared error when no error is present (e.g., onError?.({ name }, undefined) or null) before calling onChange so that the parent clears validation state; update the block around validateRegex/newValue/typedValue to explicitly clear the error when validateRegex returns falsy.src/components/custom/jobsTable/advancedJobFilteringDialog.tsx (1)
218-220:⚠️ Potential issue | 🟠 MajorVerify backend API contract for filter payload shape.
These lines send the full
{ value, type }objects to the backend. PerJobsService.ts:25-34,filterJobsspreadsadvancedFiltersdirectly into the POST body. If the backend expects scalar strings forname/cronSetting/consumer, this breaks filtering; if it expects the object shape to distinguish regex vs exact matching, this is correct.#!/bin/bash # Check how the backend handles these filter fields # Look for filter handling in backend or API types # Search for filterJobs endpoint handling or types rg -n -C5 'filterJobs|advancedFilters' --type ts --type js # Check if there are any API type definitions fd -t f -i 'api' -e ts -e d.ts --exec cat {}If the backend expects plain strings, apply this fix:
Suggested fix for scalar values
- name: values.name?.value ? values.name : undefined, - cronSetting: values.cronSetting?.value ? values.cronSetting : undefined, - consumer: values.consumer?.value ? values.consumer : undefined, + name: values.name?.value ? values.name.value : undefined, + cronSetting: values.cronSetting?.value ? values.cronSetting.value : undefined, + consumer: values.consumer?.value ? values.consumer.value : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/jobsTable/advancedJobFilteringDialog.tsx` around lines 218 - 220, The code currently sends full {value,type} objects for name/cronSetting/consumer from advancedJobFilteringDialog; verify the backend contract in JobsService.filterJobs (see JobsService.ts lines around 25-34) to confirm whether advancedFilters should contain scalar strings or the {value,type} shape; if the backend expects scalars, change the payload construction in advancedJobFilteringDialog so name: values.name?.value ? values.name.value : undefined (and likewise for cronSetting and consumer); if the backend expects the object shape, leave as-is but add/type the advancedFilters payload to reflect the {value,type} structure so filterJobs and any API types match.src/components/custom/notifications/NotificationServicesPanel.tsx (1)
385-385:⚠️ Potential issue | 🟠 MajorRight details pane still lacks a bounded scroll container.
Line 385 is still an unconstrained column wrapper, so in short viewports this section can grow/clamp under the parent
overflow-hiddencontainer instead of scrolling internally.Suggested fix
- <div className="flex flex-col gap-2"> + <div className="flex h-full min-h-0 flex-col gap-2 overflow-y-auto">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/notifications/NotificationServicesPanel.tsx` at line 385, The right details pane in NotificationServicesPanel uses an unconstrained <div className="flex flex-col gap-2"> which allows content to grow past the parent overflow-hidden instead of scrolling; update that element (or its immediate wrapper in NotificationServicesPanel) to be a bounded, scrollable container by adding sizing and scroll utility classes such as: include min-h-0 and either flex-1 or a max-h value plus overflow-auto (e.g. className="flex flex-col gap-2 flex-1 min-h-0 overflow-auto") so the column can shrink and scroll correctly inside the parent.src/features/jobsTable/jobsTable.tsx (1)
71-75:⚠️ Potential issue | 🟡 MinorRemove
min-h-[800px]for empty state—already handled inTableBodyVirtualized.The empty state is properly rendered in
TableBodyVirtualized(lines 141-146) with a "No results" message. Themin-h-[800px]on the wrapper creates an unnecessarily large blank panel when data is empty, conflicting with the viewport-bounded height passed viasize.🔧 Suggested fix
className={cn( "rounded-md border border-border w-full overflow-auto relative", size, - data.length > 0 ? "" : "min-h-[800px]", )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/jobsTable/jobsTable.tsx` around lines 71 - 75, The wrapper div in jobsTable.tsx currently injects "min-h-[800px]" when data is empty, causing a large blank area; remove that fallback from the className expression so the wrapper only uses cn("rounded-md border border-border w-full overflow-auto relative", size) and let TableBodyVirtualized handle the empty state rendering (refer to TableBodyVirtualized and the size and data variables used in the className logic).
🤖 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/custom/dashboard/statsBar.tsx`:
- Around line 79-86: The Tooltip instance (Tooltip, TooltipTrigger,
TooltipContent) needs to be wrapped with Radix's TooltipProvider to provide
context; import TooltipProvider and wrap the existing Tooltip block (the
component using FileWarningIcon and tooltipContent) with <TooltipProvider> so
the provider is an ancestor of Tooltip; ensure the import list for Tooltip
includes TooltipProvider and that any props required by TooltipProvider (if used
elsewhere) are passed through.
- Around line 57-63: The nested ternary mapping of the textSize prop in the
className (inside the StatsBar component in statsBar.tsx) is redundant because
textSize already contains the Tailwind class string; replace the ternary logic
in the cn(...) call to include textSize directly (e.g., pass textSize) and
provide a fallback default like "text-2xl" when textSize is falsy to preserve
current behavior; update any references to the textSize prop usage to rely on
the direct class string instead of remapping.
In `@src/components/custom/general/MultiTypedInput.tsx`:
- Line 119: The dependency arrays in MultiTypedInput's hooks include stable
React state setters (setExactValue, setRegexValue, setBetweenValue)
unnecessarily; remove these setters from the dependency arrays of the affected
callbacks/effects (the hooks referencing onChange, onError, name) so only actual
changing deps (e.g., onChange, onError, name) remain—apply the same removal to
the other two occurrences in the file that match the pattern.
In `@src/features/jobsTable/jobsPage.tsx`:
- Line 271: The sentinel div with ref (the line <div ref={ref}
className="h-[1px]"></div>) is used with useInView but useInView is defaulting
to the viewport, so when the table has a fixed internal scroll
(size="h-[calc(100vh-150px)]") the sentinel stays visible and the sticky header
never toggles; update the useInView call to supply the table scrolling container
as the root option (i.e., pass the element reference for the table
container/wrapper used for the fixed-height scroll) so the intersection observer
is scoped to that container, or alternatively remove/adjust the sentinel and
sticky styling if sticky behavior is no longer desired.
In `@src/features/jobsTable/jobsTable.tsx`:
- Line 176: The className string in the JSX element in jobsTable.tsx contains a
leading space (" flex absolute w-full border-b-border"); remove the leading
space so the attribute reads "flex absolute w-full border-b-border". Locate the
JSX element with className=" flex absolute w-full border-b-border" and update
the string to drop the initial space to match other class definitions.
- Around line 126-132: The overscan for the virtualized table is set very high
in the rowVirtualizer configuration (useVirtualizer call) which defeats
virtualization benefits; change the overscan value from 42 to a much smaller,
typical number (e.g., 5-10—try 8) in the options passed to useVirtualizer to
reduce offscreen rendering while keeping smooth scroll behavior.
- Line 49: The ref tableContainerRef is initialized with useRef(null) which
infers RefObject<null>; update its initialization to explicitly type it as
React.RefObject<HTMLDivElement> (or useRef<HTMLDivElement | null>(null)) so it
matches the TableBodyProps typing; change the declaration of tableContainerRef
to useRef<HTMLDivElement | null>(null) (or the equivalent
React.RefObject<HTMLDivElement>) to ensure type safety across the component.
---
Outside diff comments:
In `@src/components/custom/jobsTable/advancedJobFilteringDialog.tsx`:
- Around line 301-308: The FlexibleInput usages (e.g., the instance with
type="regex" in advancedJobFilteringDialog) are getting their pre-filled values
wiped because MultiTypedInput.tsx's useEffect invokes handleTypeChange (lines
around 162-166), which resets internal state and emits an empty value; fix
MultiTypedInput by syncing the internal inputType state directly from props (set
inputType = props.type or similar) in that effect instead of calling
handleTypeChange so you don't reset other internal state or call the
value-emitting logic; ensure handleTypeChange remains the explicit
user-triggered transition and that existing field.value is preserved and not
overwritten when the component mounts.
In `@src/features/jobsTable/jobsTable.tsx`:
- Around line 78-102: Table cells are misaligning because body cells use the
tailwind class "w-full" while headers use "flex flex-grow" and widths are set
via header.column.getSize()/cell.column.getSize(), causing flex-basis to be
overridden; remove "w-full" from the body cell elements (the TableCell elements
that use style={{ width: cell.column.getSize() }}) so they mirror the TableHead
layout, or alternatively change both header and body containers from "flex-grow"
to "flex-shrink-0" (update classNames on TableRow/TableHead and the
corresponding TableCell) to enforce strict sizing based on column.getSize().
---
Duplicate comments:
In `@src/components/custom/general/MultiTypedInput.tsx`:
- Around line 112-117: The emitted payload uses defaultMultiTypeNullValue which
hardcodes type:"exact", so when newType is "regex" (or other types) the emitted
object's type is wrong; update the emission logic in the onChange block (the
code referencing newType, defaultMultiTypeNullValue, and onChange) to emit an
object whose type field equals newType (e.g., construct { type: newType, value:
... } or clone defaultMultiTypeNullValue but set type: newType) and for
"between" continue to emit { value1: "", value2: "" } as before.
- Around line 162-166: The useEffect currently calls handleTypeChange(type) on
mount which triggers the reset logic (clearing internal state and calling
onChange); instead, update the internal inputType state directly without
invoking reset/emit: in the component MultiTypedInput, replace calling
handleTypeChange from the useEffect with a direct state sync (e.g.,
setInputType(type) or the existing inputType setter) so the controlled prop
synchronizes the UI but does not call handleTypeChange or emit an empty value;
alternatively, add an optional flag parameter to handleTypeChange (e.g.,
handleTypeChange(type, { preserveValue: true })) and call that from the
useEffect to avoid clearing value and calling onChange.
- Around line 153-160: The handleBetweenChange callback closes over betweenValue
causing stale updates; change it to use the functional form of setBetweenValue
so you compute the new state from the previous value (e.g. setBetweenValue(prev
=> ({ ...prev, [field]: newValue, type: "between" }))) and call onChange with
that computed updated object; then remove betweenValue from the useCallback
dependency array so the callback only depends on onChange and setBetweenValue
(reference: handleBetweenChange, setBetweenValue, betweenValue, onChange).
- Around line 143-147: validateRegex is only used to call onError when an error
exists, so once the user corrects the pattern the previous error remains; after
computing error = validateRegex(newValue) call onError with a cleared error when
no error is present (e.g., onError?.({ name }, undefined) or null) before
calling onChange so that the parent clears validation state; update the block
around validateRegex/newValue/typedValue to explicitly clear the error when
validateRegex returns falsy.
In `@src/components/custom/jobsTable/advancedJobFilteringDialog.tsx`:
- Around line 218-220: The code currently sends full {value,type} objects for
name/cronSetting/consumer from advancedJobFilteringDialog; verify the backend
contract in JobsService.filterJobs (see JobsService.ts lines around 25-34) to
confirm whether advancedFilters should contain scalar strings or the
{value,type} shape; if the backend expects scalars, change the payload
construction in advancedJobFilteringDialog so name: values.name?.value ?
values.name.value : undefined (and likewise for cronSetting and consumer); if
the backend expects the object shape, leave as-is but add/type the
advancedFilters payload to reflect the {value,type} structure so filterJobs and
any API types match.
In `@src/components/custom/notifications/NotificationServicesPanel.tsx`:
- Line 385: The right details pane in NotificationServicesPanel uses an
unconstrained <div className="flex flex-col gap-2"> which allows content to grow
past the parent overflow-hidden instead of scrolling; update that element (or
its immediate wrapper in NotificationServicesPanel) to be a bounded, scrollable
container by adding sizing and scroll utility classes such as: include min-h-0
and either flex-1 or a max-h value plus overflow-auto (e.g. className="flex
flex-col gap-2 flex-1 min-h-0 overflow-auto") so the column can shrink and
scroll correctly inside the parent.
In `@src/features/jobsTable/jobsTable.tsx`:
- Around line 71-75: The wrapper div in jobsTable.tsx currently injects
"min-h-[800px]" when data is empty, causing a large blank area; remove that
fallback from the className expression so the wrapper only uses cn("rounded-md
border border-border w-full overflow-auto relative", size) and let
TableBodyVirtualized handle the empty state rendering (refer to
TableBodyVirtualized and the size and data variables used in the className
logic).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: fbd1c300-2100-4501-bd5b-502d5c08cd11
📒 Files selected for processing (8)
src/components/custom/dashboard/statsBar.tsxsrc/components/custom/general/MultiTypedInput.tsxsrc/components/custom/jobsTable/advancedJobFilteringDialog.tsxsrc/components/custom/notifications/NotificationServicesPanel.tsxsrc/components/ui/date-picker-presets.tsxsrc/features/jobsTable/jobsPage.tsxsrc/features/jobsTable/jobsTable.tsxsrc/features/system/database.tsx
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 (3)
src/components/custom/dashboard/statsBar.tsx (1)
7-11:⚠️ Potential issue | 🔴 Critical
TooltipProvideris used but not imported — runtime error.Line 75 uses
<TooltipProvider>but it's missing from the import. This will cause aReferenceErrorwhen rendering the error state.🐛 Proposed fix
import { Tooltip, TooltipContent, TooltipTrigger, + TooltipProvider, } from "@/components/ui/tooltip"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/dashboard/statsBar.tsx` around lines 7 - 11, The component is using TooltipProvider (rendered at line ~75) but it's not imported, causing a runtime ReferenceError; fix by adding TooltipProvider to the import list alongside Tooltip, TooltipContent, TooltipTrigger from "@/components/ui/tooltip" so the symbol is available when rendering the error state (update the import statement that currently imports Tooltip, TooltipContent, TooltipTrigger to also import TooltipProvider).src/components/custom/general/LogFileList.tsx (2)
61-68:⚠️ Potential issue | 🟡 MinorMissing
keyprop on virtualized list items.Each
FileItemrendered in the.map()lacks akeyprop. This causes React reconciliation issues—especially problematic in virtualized lists where elements are reused. UsevirtualRow.indexor a unique identifier likefileId.🔧 Proposed fix
{rowVirtualizer.getVirtualItems().map((virtualRow, index) => ( <FileItem + key={virtualRow.index} className="mb-2 absolute" style={{ transform: `translateY(${virtualRow.start}px)`, }} {...FileList[virtualRow?.index ?? index]} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/LogFileList.tsx` around lines 61 - 68, The FileItem elements rendered from rowVirtualizer.getVirtualItems() are missing a React key; update the map so each FileItem receives a stable key (use virtualRow.index or a unique identifier from the data). For example, inside the mapping that spreads {...FileList[virtualRow?.index ?? index]}, add a key prop referencing virtualRow.index or FileList[virtualRow.index].fileId (fall back to virtualRow.index if no fileId) to ensure proper reconciliation for the FileItem components.
48-48:⚠️ Potential issue | 🟡 MinorMissing
props.downloadLogFilein useMemo dependency array.
props.downloadLogFileis captured on line 45 but not listed in the dependencies. If this prop changes,FileListwon't re-compute, leading to stale callback references.🔧 Proposed fix
- }, [props.logFiles, props.readLogFile]) + }, [props.logFiles, props.readLogFile, props.downloadLogFile, props.originName])Note:
props.originName(line 39) is also missing from the dependency array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/LogFileList.tsx` at line 48, The useMemo that builds FileList is missing dependencies: add props.downloadLogFile and props.originName to the dependency array alongside props.logFiles and props.readLogFile so the memo recomputes when those callbacks/props change; update the dependency list used in the useMemo call within the FileList-producing block to include downloadLogFile and originName to avoid stale references.
♻️ Duplicate comments (6)
src/components/custom/DrawerJobEvents.tsx (2)
83-83:⚠️ Potential issue | 🟡 MinorAdd fallback for potentially undefined items in
getItemKey.If
allEvents[index]is undefined during a render-update race, this will throw. The previous review suggested adding a fallback which appears to have been partially addressed but the safety guard is missing.🛡️ Suggested fix
- getItemKey: index => String(allEvents[index].id), + getItemKey: index => String(allEvents[index]?.id ?? index),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/DrawerJobEvents.tsx` at line 83, The getItemKey function can throw if allEvents[index] is undefined during render; update getItemKey (in the DrawerJobEvents component) to guard against missing items and return a stable fallback key, e.g. use the event id when present and otherwise fall back to the index or a prefixed index string (e.g. use allEvents[index]?.id ?? index), ensuring the returned value is always a string.
144-149: 🧹 Nitpick | 🔵 Trivial
key={allEvents.length}forces unnecessary remounts.Using
allEvents.lengthas the key on the container div forces React to unmount and remount the entire virtualized list whenever items are added or removed. This defeats some benefits of virtualization and may cause scroll position resets.Consider removing this key or using a more stable identifier if the goal is to trigger re-measurement. The
refreshVirtualizercallback withrowVirtualizer.measure()should handle updates.♻️ Proposed fix
<div className="relative w-full py-4" style={{ height: `${rowVirtualizer.getTotalSize()}px`, }} - key={allEvents.length} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/DrawerJobEvents.tsx` around lines 144 - 149, The container div is using key={allEvents.length}, which forces full remounts of the virtualized list; remove that unstable key (or replace it with a stable identifier if you truly need remounting) and rely on the virtualizer re-measure flow instead—call refreshVirtualizer / rowVirtualizer.measure() after updates so the list recomputes sizes rather than forcing React to unmount/remount the element; update the JSX around the div in DrawerJobEvents.tsx to drop the length-based key and ensure any existing refreshVirtualizer usage invokes rowVirtualizer.measure() when items change.src/components/custom/general/MultiTypedInput.tsx (4)
155-162: 🛠️ Refactor suggestion | 🟠 MajorCallback recreated on every value change; prefer functional setState.
Adding
betweenValueto the dependency array fixes the stale closure but defeats memoization—the callback is recreated on every keystroke. Using functional state update avoids the closure issue while keeping a stable callback identity.Proposed fix using functional setState
const handleBetweenChange = useCallback( (field: keyof BetweenValue, newValue: string) => { - const updated = { ...betweenValue, [field]: newValue, type: "between" } - setBetweenValue(updated) - onChange?.(updated) + setBetweenValue(prev => { + const updated = { ...prev, [field]: newValue, type: "between" } + onChange?.(updated) + return updated + }) }, - [onChange, setBetweenValue, betweenValue], + [onChange], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 155 - 162, The handleBetweenChange callback closes over betweenValue causing either stale state or recreation on every keystroke; change it to use the functional form of setBetweenValue so the callback can omit betweenValue from its deps and remain stable: inside handleBetweenChange call setBetweenValue(prev => { const updated = { ...prev, [field]: newValue, type: "between" }; onChange?.(updated); return updated; }); then update the dependency array to only include onChange (and any other stable refs) so handleBetweenChange remains memoized; reference functions/values: handleBetweenChange, setBetweenValue, onChange, BetweenValue.
113-114:⚠️ Potential issue | 🔴 CriticalEmitted payload has incorrect
typefield for regex mode.When
newType === "regex", the code emitsdefaultMultiTypeNullValuewhich is hardcoded to{ value: "", type: "exact" }. The emitted value should reflect the actual selected type.Proposed fix
// Emit initial value for new type if (newType === "exact" || newType === "regex") { - onChange?.(defaultMultiTypeNullValue) + onChange?.({ value: "", type: newType }) } else if (newType === "between") { - onChange?.({ value1: "", value2: "" }) + onChange?.({ value1: "", value2: "", type: "between" }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 113 - 114, The emitted payload uses defaultMultiTypeNullValue which hardcodes type:"exact", so when newType === "regex" the emitted value has the wrong type; update the logic in the MultiTypedInput component where newType is handled (the block that calls onChange?.(defaultMultiTypeNullValue)) to emit a value whose type matches newType (e.g., clone/construct the default payload but set type: newType) so onChange receives the correct type for "regex" and "exact".
164-168:⚠️ Potential issue | 🔴 CriticalEffect resets values on mount when controlled
typeis provided.Calling
handleTypeChange(type)in this effect resets all internal state and emits empty payloads. On mount, if bothtypeandvalueprops are provided, the value initialization effect (lines 86-99) runs, then this effect fires and clears everything. The effect should only synchronizeinputTypestate without triggering the reset logic.Proposed fix
useEffect(() => { - if (type) { - handleTypeChange(type) + if (type && type !== inputType) { + setInputType(type) } - }, [type, handleTypeChange]) + }, [type, inputType])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 164 - 168, The effect currently calls handleTypeChange(type) on mount which triggers full reset logic; instead only synchronize the local inputType state when a controlled type prop is provided. Replace the call to handleTypeChange(type) in the useEffect with logic that sets the internal inputType (e.g. setInputType(type) or equivalent) and avoid invoking handleTypeChange so the value initialization effect (value prop handling) isn't clobbered by the reset; ensure you still react to actual type changes by invoking handleTypeChange only when a real user-driven change is intended.
136-152:⚠️ Potential issue | 🟡 MinorMissing
namein dependency array causes stale closure.The callback references
nameat lines 145 and 147 but the dependency array at line 151 only includes[onChange, onError]. If thenameprop changes, the callback will use the stale value.Proposed fix
[onChange, onError], + [name, onChange, onError],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/custom/general/MultiTypedInput.tsx` around lines 136 - 152, The handleRegexChange callback captures the prop name but it's not declared in the dependency array, causing a stale closure; update the useCallback dependency array for handleRegexChange to include name (in addition to onChange and onError) so the closure sees updated name values, and ensure related symbols (handleRegexChange, setRegexValue, validateRegex, onError, onChange, name) are referenced correctly when updating the dependency list.
🤖 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/custom/DrawerJobEvents.tsx`:
- Around line 86-92: The workaround using setTimeout in refreshVirtualizer and
the remount via key={allEvents.length} causes unreliable virtualizer updates;
remove the key={allEvents.length} and replace the timeout-based refresh by
adding a useEffect that watches allEvents (or its length/identity) and calls
rowVirtualizer.measure() when it changes (while keeping refreshVirtualizer for
manual triggers if needed); ensure you reference rowVirtualizer.measure and the
allEvents state inside the effect and clean up any stale closures by including
rowVirtualizer in the effect dependency array.
- Line 19: Remove the unused import "debounce" from the top of the
DrawerJobEvents component file; locate the import line that reads import {
debounce } from "@/utils/generalUtils" and delete it (or remove debounce from
the named imports if combined), ensuring the DrawerJobEvents component and any
other imports remain unchanged.
- Around line 70-72: The useMemo for allEvents includes redundant dependencies
(latestEvents.length and events.length); update the dependency array to only
include latestEvents and events so it becomes useMemo(() => [...latestEvents,
...events], [latestEvents, events]) — modify the dependency list around the
allEvents declaration to remove the .length entries while keeping the useMemo
logic unchanged.
In `@src/features/jobsTable/jobsTable.tsx`:
- Around line 170-195: The TableRow inside TableBodyRow has a redundant key prop
(key={row.id}) because the parent map already sets the key; remove the inner key
attribute from the TableRow in the TableBodyRow component (leave data-index,
ref, className, style, and children unchanged) so there’s a single key provided
by the parent mapping over rows.
- Around line 137-146: The TableBody is given a fixed height from
rowVirtualizer.getTotalSize(), which is 0 when rows.length === 0 and causes the
"No results." TableRow (h-24) to be clipped; update the rendering in
jobsTable.tsx so the style for TableBody only sets height when rows exist (e.g.,
if rows.length > 0 use height: `${rowVirtualizer.getTotalSize()}px`) or
alternatively apply a minHeight (e.g., minHeight: '6rem') when rows are empty;
locate the TableBody usage that references rowVirtualizer.getTotalSize() and
adjust the conditional styling to ensure the empty-state TableRow is visible.
---
Outside diff comments:
In `@src/components/custom/dashboard/statsBar.tsx`:
- Around line 7-11: The component is using TooltipProvider (rendered at line
~75) but it's not imported, causing a runtime ReferenceError; fix by adding
TooltipProvider to the import list alongside Tooltip, TooltipContent,
TooltipTrigger from "@/components/ui/tooltip" so the symbol is available when
rendering the error state (update the import statement that currently imports
Tooltip, TooltipContent, TooltipTrigger to also import TooltipProvider).
In `@src/components/custom/general/LogFileList.tsx`:
- Around line 61-68: The FileItem elements rendered from
rowVirtualizer.getVirtualItems() are missing a React key; update the map so each
FileItem receives a stable key (use virtualRow.index or a unique identifier from
the data). For example, inside the mapping that spreads
{...FileList[virtualRow?.index ?? index]}, add a key prop referencing
virtualRow.index or FileList[virtualRow.index].fileId (fall back to
virtualRow.index if no fileId) to ensure proper reconciliation for the FileItem
components.
- Line 48: The useMemo that builds FileList is missing dependencies: add
props.downloadLogFile and props.originName to the dependency array alongside
props.logFiles and props.readLogFile so the memo recomputes when those
callbacks/props change; update the dependency list used in the useMemo call
within the FileList-producing block to include downloadLogFile and originName to
avoid stale references.
---
Duplicate comments:
In `@src/components/custom/DrawerJobEvents.tsx`:
- Line 83: The getItemKey function can throw if allEvents[index] is undefined
during render; update getItemKey (in the DrawerJobEvents component) to guard
against missing items and return a stable fallback key, e.g. use the event id
when present and otherwise fall back to the index or a prefixed index string
(e.g. use allEvents[index]?.id ?? index), ensuring the returned value is always
a string.
- Around line 144-149: The container div is using key={allEvents.length}, which
forces full remounts of the virtualized list; remove that unstable key (or
replace it with a stable identifier if you truly need remounting) and rely on
the virtualizer re-measure flow instead—call refreshVirtualizer /
rowVirtualizer.measure() after updates so the list recomputes sizes rather than
forcing React to unmount/remount the element; update the JSX around the div in
DrawerJobEvents.tsx to drop the length-based key and ensure any existing
refreshVirtualizer usage invokes rowVirtualizer.measure() when items change.
In `@src/components/custom/general/MultiTypedInput.tsx`:
- Around line 155-162: The handleBetweenChange callback closes over betweenValue
causing either stale state or recreation on every keystroke; change it to use
the functional form of setBetweenValue so the callback can omit betweenValue
from its deps and remain stable: inside handleBetweenChange call
setBetweenValue(prev => { const updated = { ...prev, [field]: newValue, type:
"between" }; onChange?.(updated); return updated; }); then update the dependency
array to only include onChange (and any other stable refs) so
handleBetweenChange remains memoized; reference functions/values:
handleBetweenChange, setBetweenValue, onChange, BetweenValue.
- Around line 113-114: The emitted payload uses defaultMultiTypeNullValue which
hardcodes type:"exact", so when newType === "regex" the emitted value has the
wrong type; update the logic in the MultiTypedInput component where newType is
handled (the block that calls onChange?.(defaultMultiTypeNullValue)) to emit a
value whose type matches newType (e.g., clone/construct the default payload but
set type: newType) so onChange receives the correct type for "regex" and
"exact".
- Around line 164-168: The effect currently calls handleTypeChange(type) on
mount which triggers full reset logic; instead only synchronize the local
inputType state when a controlled type prop is provided. Replace the call to
handleTypeChange(type) in the useEffect with logic that sets the internal
inputType (e.g. setInputType(type) or equivalent) and avoid invoking
handleTypeChange so the value initialization effect (value prop handling) isn't
clobbered by the reset; ensure you still react to actual type changes by
invoking handleTypeChange only when a real user-driven change is intended.
- Around line 136-152: The handleRegexChange callback captures the prop name but
it's not declared in the dependency array, causing a stale closure; update the
useCallback dependency array for handleRegexChange to include name (in addition
to onChange and onError) so the closure sees updated name values, and ensure
related symbols (handleRegexChange, setRegexValue, validateRegex, onError,
onChange, name) are referenced correctly when updating the dependency list.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a819b92c-6b92-44ea-bbd7-02dcef828a68
📒 Files selected for processing (6)
src/components/custom/DrawerJobEvents.tsxsrc/components/custom/DrawerLatestRuns.tsxsrc/components/custom/dashboard/statsBar.tsxsrc/components/custom/general/LogFileList.tsxsrc/components/custom/general/MultiTypedInput.tsxsrc/features/jobsTable/jobsTable.tsx
| import { ListCheck } from "lucide-react" | ||
| import { useVirtualizer } from "@tanstack/react-virtual" | ||
| import LoadingOverlay from "@/components/custom/LoadingOverlay" | ||
| import { debounce } from "@/utils/generalUtils" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import: debounce.
The debounce function is imported but never used in this file. This appears to be leftover from development.
🧹 Proposed fix
import LoadingOverlay from "@/components/custom/LoadingOverlay"
-import { debounce } from "@/utils/generalUtils"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { debounce } from "@/utils/generalUtils" | |
| import LoadingOverlay from "@/components/custom/LoadingOverlay" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/custom/DrawerJobEvents.tsx` at line 19, Remove the unused
import "debounce" from the top of the DrawerJobEvents component file; locate the
import line that reads import { debounce } from "@/utils/generalUtils" and
delete it (or remove debounce from the named imports if combined), ensuring the
DrawerJobEvents component and any other imports remain unchanged.
Comment on lines
+70
to
+72
| const allEvents = useMemo(() => { | ||
| return [...latestEvents, ...events] | ||
| }, [events, latestEvents, latestEvents.length, events.length]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant dependency array entries.
The .length properties are redundant since events and latestEvents references change when the arrays are updated.
🧹 Simplified dependencies
const allEvents = useMemo(() => {
return [...latestEvents, ...events]
- }, [events, latestEvents, latestEvents.length, events.length])
+ }, [events, latestEvents])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const allEvents = useMemo(() => { | |
| return [...latestEvents, ...events] | |
| }, [events, latestEvents, latestEvents.length, events.length]) | |
| const allEvents = useMemo(() => { | |
| return [...latestEvents, ...events] | |
| }, [events, latestEvents]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/custom/DrawerJobEvents.tsx` around lines 70 - 72, The useMemo
for allEvents includes redundant dependencies (latestEvents.length and
events.length); update the dependency array to only include latestEvents and
events so it becomes useMemo(() => [...latestEvents, ...events], [latestEvents,
events]) — modify the dependency list around the allEvents declaration to remove
the .length entries while keeping the useMemo logic unchanged.
Comment on lines
+86
to
+92
| const refreshVirtualizer = useCallback(() => { | ||
| setTimeout(() => { | ||
| // TODO find another solution for this timeout issue : | ||
| // the allEvent array although is always updated, doesn't trigger the virtualizer getVirtualItems() function to return filled item array | ||
| rowVirtualizer.measure() | ||
| }, 200) | ||
| }, [rowVirtualizer]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Acknowledged workaround with setTimeout.
The PR notes mention this timeout is needed because the virtualizer doesn't refresh correctly when the event list updates. This is likely related to the key={allEvents.length} forcing remounts and the timing of when the virtualizer measures.
Consider addressing the root cause by:
- Removing the
key={allEvents.length}from line 149 - Using
useEffectto callrowVirtualizer.measure()whenallEventschanges
This would be more React-idiomatic than the current onOpenChange + setTimeout approach.
Would you like me to help draft an alternative implementation using useEffect to handle virtualizer refresh more reliably?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/custom/DrawerJobEvents.tsx` around lines 86 - 92, The
workaround using setTimeout in refreshVirtualizer and the remount via
key={allEvents.length} causes unreliable virtualizer updates; remove the
key={allEvents.length} and replace the timeout-based refresh by adding a
useEffect that watches allEvents (or its length/identity) and calls
rowVirtualizer.measure() when it changes (while keeping refreshVirtualizer for
manual triggers if needed); ensure you reference rowVirtualizer.measure and the
allEvents state inside the effect and clean up any stale closures by including
rowVirtualizer in the effect dependency array.
Comment on lines
+137
to
+146
| style={{ | ||
| height: `${rowVirtualizer.getTotalSize()}px`, | ||
| }} | ||
| > | ||
| {rows.length === 0 ? ( | ||
| <TableRow> | ||
| <TableCell colSpan={columnsLength} className="h-24 text-center"> | ||
| No results. | ||
| </TableCell> | ||
| </TableRow> |
There was a problem hiding this comment.
Potential layout issue: empty state inside zero-height container.
When rows.length === 0, rowVirtualizer.getTotalSize() returns 0, setting TableBody height to 0px. However, the "No results." row inside has h-24 (6rem) content. The relative positioning on TableBody with explicit height: 0px may clip or hide the empty-state row.
Consider conditionally setting height only when rows exist:
<TableBody
className="grid relative"
style={{
- height: `${rowVirtualizer.getTotalSize()}px`,
+ height: rows.length > 0 ? `${rowVirtualizer.getTotalSize()}px` : undefined,
}}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/jobsTable/jobsTable.tsx` around lines 137 - 146, The TableBody
is given a fixed height from rowVirtualizer.getTotalSize(), which is 0 when
rows.length === 0 and causes the "No results." TableRow (h-24) to be clipped;
update the rendering in jobsTable.tsx so the style for TableBody only sets
height when rows exist (e.g., if rows.length > 0 use height:
`${rowVirtualizer.getTotalSize()}px`) or alternatively apply a minHeight (e.g.,
minHeight: '6rem') when rows are empty; locate the TableBody usage that
references rowVirtualizer.getTotalSize() and adjust the conditional styling to
ensure the empty-state TableRow is visible.
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.
Improvements:
Notes
setTimeoutas it doesn't refresh correctly when the full list of events is updated via the event hookMotivation and Context
How Has This Been Tested?
Screenshots (if applicable)
Add screenshots to help explain your changes.
Related Issues
Additional Context
Summary by CodeRabbit
New Features
Improvements
Bug Fixes