UN-2966 [FEAT] Auto-refresh and controls for execution logs #1687
Conversation
- Update DetailedLogs and ExecutionLogs components to properly handle polling state - Update index.js dependencies - Update workers dependencies in pyproject.toml and uv.lock 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Reverted files that were not part of the intended fix: - frontend/src/index.js - workers/pyproject.toml - workers/uv.lock 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Fixed date calculation bug: use raw ISO timestamps instead of formatted display strings - Added createdAtRaw and modified_atRaw fields to preserve parseable date values - Implement stale interval re-check: stop polling when execution is >1 hour old via executionDetailsRef - Replace forEach loops with for...of for improved performance (4 instances) Fixes CodeRabbit comments about infinite polling and ensures logging components correctly handle timestamp calculations and respect staleness thresholds. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Chandrasekharan M <[email protected]>
…mponents Replace isNaN() with isFinite() for date validation in ExecutionLogs and DetailedLogs to prevent infinite polling when invalid timestamps occur. - DetailedLogs.jsx: Changed isNaN() to isFinite() check - ExecutionLogs.jsx: Added missing isFinite() date validation 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Use ref instead of state for pollingIds to prevent stale closure issues in polling logic: - Added pollingIdsRef to track actively polling execution IDs - Updated all polling operations to directly mutate ref (no re-renders needed) - Removed redundant pollingIds state to eliminate unnecessary re-renders - Prevents duplicate polling loops and potential memory leaks 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Add reusable LogsRefreshControls component (toggle + refresh button) - Move date picker from nav bar to content area in ExecutionLogs - Update DetailedLogs to use shared LogsRefreshControls component - Fix table height to fill available space using flexbox layout 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds auto-refresh controls (30s), manual refresh, Execution ID search and copy, column-visibility controls, sortable execution time and expanded pagination, responsive logging CSS, backend UUID filter and file_size ordering, plus minor logging and redaction tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Controls as LogsRefreshControls
participant UI as Logs UI (Execution / Detailed)
participant API as Backend API
Note over User,Controls: User toggles auto-refresh, clicks Refresh, searches, or copies IDs
User->>Controls: toggle auto-refresh / click Refresh
Controls->>UI: setAutoRefresh(true/false) / onRefresh()
UI->>UI: start/stop 30s interval or perform immediate fetch
loop every 30s when enabled
UI->>API: GET /logs (filters, executionId, sort, pagination)
API-->>UI: 200 OK (logs payload)
UI->>UI: update table state (LogsTable / DetailedLogs)
end
User->>UI: enter Execution ID search
UI->>API: GET /logs?execution_id=...
API-->>UI: 200 OK (filtered logs)
User->>UI: click copy Execution ID
UI-->>User: write to clipboard + show success alert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.jsx (1)
6-22: Consider making the refresh interval configurable.The label displays "Auto-refresh (30s)" but the 30-second interval is hardcoded in the parent components (DetailedLogs.jsx line 312 and ExecutionLogs.jsx line 179). If the interval changes, the label would become incorrect.
Consider one of these options:
- Pass the interval duration as a prop to this component
- Define the interval as a shared constant that both the component and parents import
However, given the simplicity of the feature, the current implementation is acceptable if the interval is unlikely to change.
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
126-126: Verify if createdAtRaw field is needed.Similar to DetailedLogs.jsx line 80, the
createdAtRawfield is added but doesn't appear to be used within this file. The verification script provided in the DetailedLogs.jsx review will check usage across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
frontend/src/components/logging/detailed-logs/DetailedLogs.css(1 hunks)frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(7 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.css(3 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(6 hunks)frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.css(1 hunks)frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.jsx(1 hunks)frontend/src/components/logging/logs-table/LogsTable.css(1 hunks)frontend/src/components/logging/logs-table/LogsTable.jsx(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/logging/logs-table/LogsTable.jsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.jsx (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
autoRefresh(65-65)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
autoRefresh(47-47)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
autoRefresh(65-65)autoRefreshIntervalRef(66-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
frontend/src/components/logging/logs-table/LogsTable.css (1)
5-7: LGTM!The
height: 100%rule correctly supports the new flex-based layout introduced in ExecutionLogs.css, allowing the table to fill the available vertical space.frontend/src/components/logging/execution-logs/ExecutionLogs.css (1)
6-7: LGTM!The flex layout structure is well-designed. The parent container uses
flex-direction: column, the header is sized asflex: 0 0 auto(fixed), and the table container usesflex: 1withmin-height: 0to properly fill available space and enable scrolling.Also applies to: 52-71
frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.css (1)
1-14: LGTM!Clean and straightforward CSS for the new LogsRefreshControls component. The flex layout and styling rules are appropriate.
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (3)
1-1: LGTM!The imports and state additions are correct. Using a ref to manage the interval ID is the appropriate pattern to avoid stale closures and ensure proper cleanup.
Also applies to: 38-38, 65-66
135-146: LGTM!The refactored loops correctly skip null/undefined values and properly handle the status filter array. The logic is equivalent to the previous implementation.
438-451: LGTM!The LogsRefreshControls integration is correct. The component receives the appropriate props and is properly positioned in the header layout alongside the "View Logs" button.
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (4)
3-3: LGTM!The imports and state additions follow the same correct pattern as DetailedLogs.jsx, using a ref to manage the interval ID.
Also applies to: 7-7, 47-48
153-159: LGTM!The Tabs component is now rendered via customButtons, which aligns with the header reorganization. The component receives all necessary props correctly.
191-193: LGTM!The effect correctly resets auto-refresh when switching tabs, as specified in the PR objectives. This prevents confusion when viewing different execution types.
227-231: LGTM!The LogsRefreshControls integration is correct. The component receives appropriate props and the onRefresh handler correctly triggers a fetch with the current pagination.
…ntrols - Added sticky table header for better scrolling experience - Improved pagination positioning and styling with reduced padding - Enhanced table layout with scrollbar isolated to table body - Fixed layout spacing in DetailedLogs header with refresh controls - Updated CSS for better visual consistency across logs components 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Moved "View Logs" button from header to cards row for better layout - Kept LogsRefreshControls in header top-right - Changed "View Logs" from link to button with FileTextOutlined icon - Removed unnecessary .detailed-logs-header-controls wrapper div - Updated header padding and button margins for consistency - Fixed pagination label in LogsTable (items → executions) 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
…n fixes for execution/detailed logs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (2)
219-226: Missing null guard will cause runtime error on partial date selection.This issue was flagged in a previous review. The
onChangehandler calls.toISOString()onvalue[0]andvalue[1]without null checks. Ant Design RangePicker passes[Moment|null, Moment|null]during partial selections, which will throw "Cannot read property 'toISOString' of null".Apply the same guard used in
onOk(lines 93-98):onChange={(value) => { setDatePickerValue(value); setSelectedDateRange( - value + value && value[0] && value[1] ? [value[0].toISOString(), value[1].toISOString()] : [] ); }}
176-195: Stale closure:fetchLogscaptures outdatedselectedDateRangeandordering.The interval callback captures
fetchLogswhich closes overselectedDateRangeandorderingvalues at effect creation time. When users change the date range or sort order, the interval continues fetching with stale values.Add these dependencies to recreate the interval when filters change:
- }, [autoRefresh, pagination.current]); + }, [autoRefresh, pagination.current, selectedDateRange, ordering]);Note: If this was addressed in a subsequent commit not reflected here, please disregard.
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
117-117: Remove the unusedcreatedAtRawfield.This field is assigned but never referenced in the codebase.
Apply this diff:
const formattedData = { - createdAtRaw: item?.created_at, executedAt: formattedDateTime(item?.created_at),
350-369: Critical: Auto-refresh still uses stale ordering and statusFilter values.The auto-refresh effect captures
orderingandstatusFilterin its closure but doesn't include them in the dependency array (line 369). When these values change, the interval continues fetching with stale values, ignoring the current filter and sort settings.Note: A previous review flagged this and marked it as addressed, but the fix appears to be missing from the current code.
Apply this diff:
- }, [autoRefresh, id, pagination.current]); + }, [autoRefresh, id, pagination.current, ordering, statusFilter]);
🧹 Nitpick comments (4)
frontend/src/components/logging/detailed-logs/DetailedLogs.css (1)
115-127: Consider: Scrollbar styling is WebKit-only.The
::-webkit-scrollbarproperties only affect Chromium-based browsers and Safari. Firefox users will see the default scrollbar. If cross-browser consistency is desired, consider addingscrollbar-width: thin;for Firefox support.+.detailed-logs-table-container .ant-table-body { + scrollbar-width: thin; + scrollbar-color: rgba(0, 0, 0, 0.2) transparent; +} + .detailed-logs-table-container .ant-table-body::-webkit-scrollbar { width: 6px; }frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (3)
42-67: Consider adding props validation for StatusMessageCell.The component works correctly, but adding PropTypes or TypeScript would improve maintainability.
Apply this diff to add props validation:
+import PropTypes from 'prop-types'; + // Component for status message with conditional tooltip (only when truncated) const StatusMessageCell = ({ text }) => { const textRef = useRef(null); // ... rest of component }; + +StatusMessageCell.propTypes = { + text: PropTypes.string +};
98-105: Consider adding error handling for clipboard operations.The clipboard copy logic is clean, but
navigator.clipboard.writeTextcan reject if permissions are denied or in some browser contexts.Apply this diff to add error handling:
const handleCopyToClipboard = (text, label = "Text") => { - navigator.clipboard.writeText(text).then(() => { - setAlertDetails({ - type: "success", - content: `${label} copied to clipboard`, + navigator.clipboard.writeText(text) + .then(() => { + setAlertDetails({ + type: "success", + content: `${label} copied to clipboard`, + }); + }) + .catch(() => { + setAlertDetails({ + type: "error", + content: `Failed to copy ${label}`, + }); }); - }); };
399-420: Consider extracting the inline title function to reduce re-renders.The inline arrow function at line 407 creates a new component on every render, which can cause unnecessary re-renders of the table header.
Apply this diff to extract the component:
+const ActionColumnHeader = ({ menu }) => ( + <div className="action-column-header"> + <span>Action</span> + <Dropdown menu={menu} trigger={["click"]} placement="bottomRight"> + <span className="column-settings-trigger"> + <MoreOutlined className="column-settings-icon" /> + </span> + </Dropdown> + </div> +); + const columnsToShow = columnsDetailedTable .filter((col) => columnsVisibility[col.key]) .map((col) => { if (col.key === "action") { return { ...col, width: 80, align: "center", - title: () => ( - <div className="action-column-header"> - <span>Action</span> - <Dropdown menu={menu} trigger={["click"]} placement="bottomRight"> - <span className="column-settings-trigger"> - <MoreOutlined className="column-settings-icon" /> - </span> - </Dropdown> - </div> - ), + title: () => <ActionColumnHeader menu={menu} />, }; } return col; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
frontend/src/components/logging/detailed-logs/DetailedLogs.css(3 hunks)frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(14 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.css(3 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(9 hunks)frontend/src/components/logging/logs-table/LogsTable.css(1 hunks)frontend/src/components/logging/logs-table/LogsTable.jsx(1 hunks)frontend/src/store/alert-store.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/store/alert-store.js (1)
frontend/src/helpers/GetStaticData.js (1)
isNonNegativeNumber(455-457)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (4)
autoRefresh(47-47)autoRefreshIntervalRef(48-48)pagination(39-43)ordering(46-46)frontend/src/components/logging/logs-table/LogsTable.jsx (1)
handleTableChange(114-129)
frontend/src/components/logging/logs-table/LogsTable.jsx (3)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
pagination(81-85)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
pagination(39-43)frontend/src/components/logging/log-modal/LogModal.jsx (1)
pagination(32-36)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (3)
autoRefresh(93-93)pagination(81-85)ordering(88-88)frontend/src/components/logging/log-modal/LogModal.jsx (2)
pagination(32-36)ordering(31-31)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 43-43: 'text' is missing in props validation
[warning] 407-407: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
frontend/src/store/alert-store.js (1)
7-7: LGTM! Success alerts now use a shorter duration.The implementation correctly differentiates success-type alerts (2s) from other types (6s), which improves UX—especially with auto-refresh generating frequent notifications. The logic follows the existing pattern for
isErrorTypeand preserves the ability to override duration via thedetails.durationparameter.Also applies to: 25-26, 32-32
frontend/src/components/logging/logs-table/LogsTable.jsx (1)
135-141: LGTM! Enhanced pagination configuration.The pagination improvements with
showSizeChanger,pageSizeOptions, andshowTotalprovide better UX for navigating large log datasets. The implementation correctly spreads the existing pagination state while adding the new options.frontend/src/components/logging/execution-logs/ExecutionLogs.css (2)
1-8: LGTM! Flexbox layout for responsive table.The flex column layout with
overflow: hiddenproperly contains the table content and prevents double scrollbars while allowing the table container to fill available space.
52-72: LGTM! Well-structured flex container hierarchy.The
min-height: 0on.logs-table-containeris essential for allowing the flex child to shrink below its content size, enabling proper scrolling behavior within the table.frontend/src/components/logging/logs-table/LogsTable.css (2)
5-50: LGTM! Comprehensive flex layout for scrollable table.The flex layout chain correctly propagates through Ant Design's nested table structure with appropriate
min-height: 0declarations. The sticky header implementation with proper z-index and background ensures headers remain visible during scroll.
52-58: Minor: Color code normalization.Converting hex colors to lowercase is a good consistency improvement.
frontend/src/components/logging/detailed-logs/DetailedLogs.css (2)
1-64: LGTM! Consistent flex layout structure.The layout follows the same pattern as LogsTable.css, ensuring consistent behavior across log views.
150-156: LGTM! Proper text truncation.The ellipsis styling with
overflow: hidden,text-overflow: ellipsis, andwhite-space: nowrapis the correct approach for truncating status messages.frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (3)
47-48: LGTM! Auto-refresh state and ref setup.Using
useReffor the interval ID is the correct pattern to avoid stale closure issues withsetIntervalin React.
197-200: LGTM! Auto-refresh reset on tab change.Correctly resets auto-refresh to OFF when switching tabs, as specified in the PR requirements.
231-235: LGTM! LogsRefreshControls integration.The component is correctly wired with auto-refresh state and manual refresh callback.
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
423-555: LGTM! Well-structured layout improvements.The header restructuring with copy button and refresh controls, along with the improved table container and pagination configuration, significantly enhance the UX while maintaining clean code organization.
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx
Outdated
Show resolved
Hide resolved
…entage-based widths - Implement responsive column widths using fixed px for predictable content - Status Message column gets 30% (most important variable content) - File Name gets 12%, File Path gets 20% for variable content - Reduced fixed columns: Executed At (140px), Status (110px), File Size (70px) - Execution Time (90px), Action (60px) for better space distribution - Added explanatory comment for width allocation strategy
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
117-117: Remove unused field (duplicate).The
createdAtRawfield is assigned but never used. This was flagged in a previous review and should be removed.Apply this diff:
const formattedData = { - createdAtRaw: item?.created_at, executedAt: formattedDateTime(item?.created_at),
357-376: Critical: Auto-refresh still uses stale ordering and statusFilter (not fixed).Despite a previous review comment being marked as addressed, the auto-refresh effect is still missing
orderingandstatusFilterfrom its dependency array (line 376). When users change these values, the interval continues fetching with stale values captured in the closure, causing auto-refresh to ignore the current filter and sort settings.Apply this diff to fix:
- }, [autoRefresh, id, pagination.current]); + }, [autoRefresh, id, pagination.current, ordering, statusFilter]);This ensures the interval is recreated whenever ordering or statusFilter changes.
🧹 Nitpick comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
42-67: Consider adding PropTypes validation (optional).Static analysis flagged missing props validation for the
textprop. While this is functional, adding PropTypes can improve development experience.Apply this diff if you want to add PropTypes:
+import PropTypes from 'prop-types'; + // Component for status message with conditional tooltip (only when truncated) const StatusMessageCell = ({ text }) => { const textRef = useRef(null); const [isOverflowing, setIsOverflowing] = useState(false); // ... rest of component }; + +StatusMessageCell.propTypes = { + text: PropTypes.string.isRequired, +};
406-427: Optimize the action column title to avoid recreating it on every render.The inline title function is recreated on every render, which may trigger unnecessary re-renders of the table header and impacts performance.
Consider extracting it to a separate component or moving it outside the render path:
// Outside DetailedLogs component const ActionColumnHeader = ({ menu }) => ( <div className="action-column-header"> <span>Action</span> <Dropdown menu={menu} trigger={["click"]} placement="bottomRight"> <span className="column-settings-trigger"> <MoreOutlined className="column-settings-icon" /> </span> </Dropdown> </div> ); // Then in columnsToShow: const columnsToShow = columnsDetailedTable .filter((col) => columnsVisibility[col.key]) .map((col) => { if (col.key === "action") { return { ...col, width: 80, align: "center", title: () => <ActionColumnHeader menu={menu} />, }; } return col; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(14 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 43-43: 'text' is missing in props validation
[warning] 414-414: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (4)
98-105: LGTM!The clipboard utility is well-implemented with clear feedback to the user.
172-182: LGTM!The for-of loops properly handle query parameter construction and skip null/undefined values.
226-319: LGTM!The column width strategy is well-documented and the use of ellipsis with tooltips provides a good user experience. The addition of the Execution Time column enhances the logs view.
429-562: LGTM!The new layout structure with the header, refresh controls, and table container is well-organized and aligns with the PR objectives. The integration of LogsRefreshControls and the copy functionality enhances the user experience.
- Add file_size and execution_time sorting to DetailedLogs and LogsTable - Add execution ID filter search to ExecutionLogs page - Implement field mapping for proper backend sort ordering - Update API filter backend with id field for search capability 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
226-307: Hiding the Action column removes access to the column-visibility menu.
columnsVisibilityallows toggling any column, including"action", andcolumnsToShowonly wraps the"action"column to inject the header dropdown that controls column visibility. If a user unchecks “Action” in the menu, the Action column disappears along with the dropdown, leaving no way to bring the column (or the menu) back without reloading the page.You probably want either:
- to always keep
"action"visible (remove it from the toggle menu), or- move the column-visibility dropdown into a different, non-hideable area (e.g., a button in the header bar) so it can’t be toggled away.
One possible minimal fix is to exclude
"action"from the toggle menu and fromcolumnsVisibility:- const menu = { - items: columnsDetailedTable.map((col) => ({ + const menu = { + items: columnsDetailedTable + .filter((col) => col.key !== "action") + .map((col) => ({ key: col.key, label: ( <Checkbox checked={columnsVisibility[col.key]} onChange={() => handleColumnToggle(col.key)} > {col.title} </Checkbox> ), - })), + })), };Also applies to: 395-413, 415-436
♻️ Duplicate comments (5)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (3)
121-143:createdAtRawappears unused in the list data model.The
formattedDataobject addscreatedAtRaw: item?.created_at, but this field isn’t referenced elsewhere in this component or passed through to consumers (the table usesexecutedAt/executedAtWithSeconds). Unless you have an imminent use planned, consider dropping it to keep the data shape minimal:- return { - key: item?.id, - createdAtRaw: item?.created_at, + return { + key: item?.id, executedAt: formattedDateTime(item?.created_at),If you intend to use
createdAtRawsoon (e.g., for client-side re-sorting), a brief comment near its definition would help future readers understand why it’s kept.
47-50: Auto-refresh interval still uses stale filter/sort values and keeps polling when viewing details.The auto-refresh effect recreates the interval only when
autoRefreshorpagination.currentchange. The interval callback closes over the values ofselectedDateRange,ordering,executionIdSearch,activeTab, etc. from the render when the interval was created. If the user changes filters/sort/tab while auto-refresh is ON, they get one immediate fetch from the other effect, but subsequent interval ticks keep using the old values.Additionally, auto-refresh is only reset on tab change, not when drilling into a specific execution (
idpresent). If a user turns auto-refresh on in the list view and then opens detailed logs, the list view continues polling/execution/in the background even though it’s no longer visible.Consider updating the auto-refresh effect to depend on all inputs that affect
fetchLogsand to disable interval polling when you’re not on the list view (e.g. whenidis present orcurrentPathis true):- useEffect(() => { - if (autoRefreshIntervalRef.current) { - clearInterval(autoRefreshIntervalRef.current); - autoRefreshIntervalRef.current = null; - } - - if (autoRefresh) { - autoRefreshIntervalRef.current = setInterval(() => { - fetchLogs(pagination.current); - }, 30000); - } - - return () => { - if (autoRefreshIntervalRef.current) { - clearInterval(autoRefreshIntervalRef.current); - autoRefreshIntervalRef.current = null; - } - }; - }, [autoRefresh, pagination.current]); + useEffect(() => { + if (autoRefreshIntervalRef.current) { + clearInterval(autoRefreshIntervalRef.current); + autoRefreshIntervalRef.current = null; + } + + // Only auto-refresh on the main list view + if (autoRefresh && !currentPath) { + autoRefreshIntervalRef.current = setInterval(() => { + fetchLogs(pagination.current); + }, 30000); + } + + return () => { + if (autoRefreshIntervalRef.current) { + clearInterval(autoRefreshIntervalRef.current); + autoRefreshIntervalRef.current = null; + } + }; + }, [ + autoRefresh, + pagination.current, + selectedDateRange, + ordering, + executionIdSearch, + activeTab, + currentPath, + ]);Also applies to: 101-121, 133-177, 179-203
219-233: RangePickeronChangehandler still lacks null checks for partial selections.
onChangecallsvalue[0].toISOString()/value[1].toISOString()whenevervalueis truthy. Ant Design’sRangePickercan pass[Moment|null, Moment|null](or evennull) during partial selections/clears, so this will throw when either entry isnull/undefined.You already have safe guards in
onOk; mirror that here:onChange={(value) => { setDatePickerValue(value); - setSelectedDateRange( - value - ? [value[0].toISOString(), value[1].toISOString()] - : [] - ); + const hasFullRange = + value && value.length === 2 && value[0] && value[1]; + setSelectedDateRange( + hasFullRange + ? [value[0].toISOString(), value[1].toISOString()] + : [] + ); }}Ant Design RangePicker onChange null partial selection behaviorfrontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
107-131:createdAtRawon execution details looks unused.In
fetchExecutionDetails,formattedDataincludescreatedAtRaw: item?.created_at, but the rest of the component reads onlyexecutedAt/executedAtWithSeconds(and other derived fields). Unless there is a hidden consumer, this appears to be dead data and can be removed for clarity:- const formattedData = { - createdAtRaw: item?.created_at, + const formattedData = { executedAt: formattedDateTime(item?.created_at),
140-183: Auto-refresh interval still closes over stale ordering/status filter values.The auto-refresh effect depends on
[autoRefresh, id, pagination.current], butfetchExecutionFiles/fetchExecutionDetailsalso depend onordering,statusFilter, andsearchTextvia closure. If a user changes sort order or status filter while auto-refresh is enabled, the interval continues to fetch with the old values.Consider including the relevant dependencies so the interval always reflects current filters/sort:
- useEffect(() => { + useEffect(() => { if (autoRefreshIntervalRef.current) { clearInterval(autoRefreshIntervalRef.current); autoRefreshIntervalRef.current = null; } if (autoRefresh) { autoRefreshIntervalRef.current = setInterval(() => { fetchExecutionDetails(id); fetchExecutionFiles(id, pagination.current); }, 30000); } return () => { if (autoRefreshIntervalRef.current) { clearInterval(autoRefreshIntervalRef.current); autoRefreshIntervalRef.current = null; } }; - }, [autoRefresh, id, pagination.current]); + }, [autoRefresh, id, pagination.current, ordering, statusFilter, searchText]);Also applies to: 360-385
🧹 Nitpick comments (6)
workers/shared/workflow/execution/context.py (1)
166-176: Redactingcustom_datavalues in logs looks correct; consider centralizing redaction keysExtending the redaction rule to cover keys containing
"custom_data"is a sensible hardening step and aligns with the existing pattern forpassword/secret/token. This will reduce chances of leaking arbitrary user-provided payloads into logs.As a small maintainability improvement, you might consider centralizing the redaction substrings into a shared iterable or constant instead of chaining
orconditions inline, e.g.:SENSITIVE_PARAM_SUBSTRINGS = ("password", "secret", "token", "custom_data") if any(s in key.lower() for s in SENSITIVE_PARAM_SUBSTRINGS): value = "***REDACTED***"This would make future additions (e.g.,
api_key,credential) less error-prone and easier to reuse across modules.unstract/workflow-execution/src/unstract/workflow_execution/execution_file_handler.py (1)
176-181: Tighten metadata creation log message for clarity and structureMinor nit: the message is slightly awkward (
"added in to execution directory") and doesn’t include thefile_execution_idormetadata_path, which can be useful when correlating logs.Consider something like this for clearer, structured logging:
- logger.info(f"metadata for {input_file_path} is added in to execution directory") + logger.info( + "Created metadata for file_execution_id=%s at %s (source=%s)", + file_execution_id, + metadata_path, + input_file_path, + )This keeps the signal but makes troubleshooting easier when many executions are happening concurrently.
backend/workflow_manager/file_execution/views.py (1)
19-19: Consider adding a database index tofile_sizefor sort performance.The
file_sizefield exists on theFileExecutionmodel and is correctly defined as aBigIntegerField. However, adding it toordering_fieldswithout a database index may cause performance degradation when sorting large result sets. Consider addingdb_index=Trueto the field definition:file_size = models.BigIntegerField(null=True, db_index=True, db_comment="Size of the file in bytes")Optional: Add
ClassVartype annotation for the class attribute.For improved type safety, annotate the mutable class attribute with
typing.ClassVar:from typing import ClassVar class FileCentricExecutionViewSet(viewsets.ReadOnlyModelViewSet): # ... ordering_fields: ClassVar[list[str]] = ["created_at", "execution_time", "file_size"]frontend/src/components/logging/logs-table/LogsTable.jsx (1)
119-130: Sorting → backend field mapping and pagination config look good (optional small tidy-up).The
fieldMapmapping (executedAt→created_at,executionTime→execution_time) and theorderingconstruction match the columndataIndexvalues and should interop correctly with the backend API. Pagination enhancements (showSizeChanger, page size options,showTotal) are also fine.If you want to simplify later, you could collapse the two
setPaginationcalls inhandleTableChangeinto a single update that also setscurrent: 1when a sorter is applied, but this is non-blocking.Also applies to: 133-153, 159-165
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
42-67: StatusMessageCell logic is fine; PropTypes/typing would help static analysis (optional).The overflow-detection + tooltip behavior is reasonable and the resize listener is correctly cleaned up. To satisfy static analysis and future readers, you might consider adding PropTypes or explicit typing for the
textprop (and possibly guarding againstnull/undefined), but this is non-blocking.
350-358: Detailed logs refresh & table layout changes look good overall (minor nits only).
handleRefreshcorrectly reuses the existing fetch helpers; the newLogsRefreshControlshook-up and the percent-based column widths / ellipsis on key columns make the table more readable. Pagination configuration mirrors the list view and should behave as expected.Optional nits for later:
- Consider setting
loadingtotrueat the start offetchExecutionDetails/fetchExecutionFilesfor a clearer spinner behavior.navigator.clipboard.writeTextinhandleCopyToClipboardcould be wrapped in a try/catch or feature-check for older/unsupported environments.Also applies to: 438-572
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
backend/workflow_manager/execution/filter.py(1 hunks)backend/workflow_manager/file_execution/views.py(1 hunks)frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(14 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(10 hunks)frontend/src/components/logging/logs-table/LogsTable.css(2 hunks)frontend/src/components/logging/logs-table/LogsTable.jsx(5 hunks)unstract/workflow-execution/src/unstract/workflow_execution/execution_file_handler.py(1 hunks)workers/shared/workflow/execution/context.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/logging/logs-table/LogsTable.css
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (5)
autoRefresh(93-93)autoRefreshIntervalRef(94-94)pagination(81-85)ordering(88-88)loading(80-80)frontend/src/components/logging/logs-table/LogsTable.jsx (1)
LogsTable(16-186)
frontend/src/components/logging/logs-table/LogsTable.jsx (3)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (2)
executionIdSearch(47-47)pagination(39-43)frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
handleTableChange(323-348)pagination(81-85)frontend/src/components/logging/log-modal/LogModal.jsx (2)
handleTableChange(127-144)pagination(32-36)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (4)
autoRefresh(48-48)autoRefreshIntervalRef(49-49)pagination(39-43)ordering(46-46)frontend/src/components/logging/logs-table/LogsTable.jsx (1)
handleTableChange(133-153)
unstract/workflow-execution/src/unstract/workflow_execution/execution_file_handler.py (3)
unstract/workflow-execution/src/unstract/workflow_execution/constants.py (1)
MetaDataKey(39-52)unstract/filesystem/src/unstract/filesystem/filesystem.py (2)
FileSystem(14-26)get_file_storage(18-26)unstract/filesystem/src/unstract/filesystem/file_storage_types.py (1)
FileStorageType(4-6)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/logs-table/LogsTable.jsx
[warning] 55-55: Move this component definition out of the parent component and pass data as props.
[warning] 45-45: Move this component definition out of the parent component and pass data as props.
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 43-43: 'text' is missing in props validation
[warning] 423-423: Move this component definition out of the parent component and pass data as props.
🪛 Ruff (0.14.7)
backend/workflow_manager/file_execution/views.py
19-19: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
unstract/workflow-execution/src/unstract/workflow_execution/execution_file_handler.py (1)
169-175: Good balance between observability and privacy forcustom_dataloggingLogging only the presence of
custom_data(without dumping the payload) is a solid choice here: it improves debuggability while avoiding accidental PII leakage into logs. No changes needed.frontend/src/components/logging/logs-table/LogsTable.jsx (1)
16-25: Execution ID search wiring looks consistent and correctly lifted to parent state.The
executionIdSearchstate is cleanly lifted intoExecutionLogsand fed into the Execution ID column’sfilterDropdown/filterIcon, with the backend param wired viaid: executionIdSearch || null. This keeps filtering logic centralized while still giving users a familiar per-column search UI. No issues from my side here.Also applies to: 42-59, 188-197
Fixed RangePicker null guard to prevent runtime errors on partial date selection. Removed unused createdAtRaw field from ExecutionLogs and DetailedLogs components. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
480-487: Fix potential crash fromexecutionDetails?.status.toLowerCase().
executionDetailsis initially undefined, so expressions like:executionDetails?.status.toLowerCase()will evaluate
executionDetails?.statustoundefinedon the first render and then call.toLowerCase()onundefined, throwing a runtime error.Use optional chaining on
statusas well:- {executionDetails?.status.toLowerCase() === "executing" + {executionDetails?.status?.toLowerCase() === "executing"Apply this in both places where the status is checked in the cards (the “Running for / Ran for” label and the “Processing files / Processed files” label.
- <Typography className="logging-card-title"> - {executionDetails?.status.toLowerCase() === "executing" - ? "Running for" - : "Ran for"} - </Typography> + <Typography className="logging-card-title"> + {executionDetails?.status?.toLowerCase() === "executing" + ? "Running for" + : "Ran for"} + </Typography> @@ - <Typography className="logging-card-title"> - {executionDetails?.status.toLowerCase() === "executing" - ? "Processing files" - : "Processed files"}{" "} + <Typography className="logging-card-title"> + {executionDetails?.status?.toLowerCase() === "executing" + ? "Processing files" + : "Processed files"}{" "}Also applies to: 494-497
400-435: Prevent hiding the Action column (otherwise column settings become unreachable).The column visibility menu includes the
actioncolumn andcolumnsToShowfilters out any column whose visibility is false. If a user unchecks “Action”, the Action column (and its header) disappears, which also removes the column-settings dropdown trigger. After that, there’s no way to re-open the column menu without reloading the page.To keep the settings always accessible, exclude the Action column from the toggle menu so it’s always visible:
- const menu = { - items: columnsDetailedTable.map((col) => ({ + const menu = { + items: columnsDetailedTable + .filter((col) => col.key !== "action") + .map((col) => ({ key: col.key, label: ( <Checkbox checked={columnsVisibility[col.key]} onChange={() => handleColumnToggle(col.key)} > {col.title} </Checkbox> ), - })), + })), };Optionally, if you’d like to address Sonar’s “move this component out of the parent” suggestion, you could also extract the
Actionheader JSX (thetitle: () => (...)part) into a smallActionHeadercomponent defined outsideDetailedLogsand use it here, but that’s more of a minor perf/clarity tweak.
♻️ Duplicate comments (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
178-197: Critical: Auto-refresh interval captures stale filter values.The interval callback captures
fetchLogsat effect creation time. WhenselectedDateRange,ordering,executionIdSearch, orpagination.pageSizechange, the interval continues fetching with the old values because those aren't in the dependency array.This appears to be a regression from the previously addressed issue.
Apply this diff to ensure the interval uses current filter values:
return () => { if (autoRefreshIntervalRef.current) { clearInterval(autoRefreshIntervalRef.current); autoRefreshIntervalRef.current = null; } }; -}, [autoRefresh, pagination.current]); +}, [autoRefresh, pagination.current, pagination.pageSize, selectedDateRange, ordering, executionIdSearch]);Alternatively, consider using a
useCallbackforfetchLogsto make the dependencies explicit and avoid recreating the interval unnecessarily.frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
359-385: Auto-refresh interval still uses stale sort/filter/search/page-size state.The auto-refresh effect only depends on
autoRefresh,id, andpagination.current, butfetchExecutionFilesalso usespagination.pageSize,ordering,statusFilter, andsearchTextfrom its closure. If any of those change while auto-refresh is ON, the interval keeps polling with the old values.This recreates the earlier issue where auto-refresh ignores updated sort/filter settings (and now also search and page size).
You can fix this by including these in the effect dependencies so the interval is recreated with the latest state:
- }, [autoRefresh, id, pagination.current]); + }, [ + autoRefresh, + id, + pagination.current, + pagination.pageSize, + ordering, + statusFilter, + searchText, + ]);
🧹 Nitpick comments (5)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (2)
163-176: Consider debouncingexecutionIdSearchto reduce API calls.The effect fires on every
executionIdSearchchange, which triggers an API call on each keystroke. This could overwhelm the backend with requests as users type.Consider adding a debounced version of the search value:
// Add at the top with other state const [debouncedExecutionIdSearch, setDebouncedExecutionIdSearch] = useState(""); // Add debounce effect useEffect(() => { const timer = setTimeout(() => { setDebouncedExecutionIdSearch(executionIdSearch); }, 300); return () => clearTimeout(timer); }, [executionIdSearch]);Then use
debouncedExecutionIdSearchin the dependency array and infetchLogsparams.
221-228: Use optional chaining for cleaner null checks.Per static analysis, the null guard can be simplified using optional chaining for improved readability.
onChange={(value) => { setDatePickerValue(value); setSelectedDateRange( - value && value[0] && value[1] + value?.[0] && value?.[1] ? [value[0].toISOString(), value[1].toISOString()] : [] ); }}frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (3)
42-67: HardenStatusMessageCellprops and consider lint alignment.
StatusMessageCellassumestextis always a string and non-null. If it ever receivesundefined/null, you’ll get “undefined” text in the UI and the tooltip. Also, Sonar is flagging missing props validation fortext.If the project uses PropTypes or another validation mechanism, consider declaring the expected type (string) and defaulting to an empty string. Otherwise, you may want to tune the Sonar rule for small local components.
98-105: Add error handling for clipboard failures.
navigator.clipboard.writeTextcan fail (permissions, HTTP origin, older browsers). Currently failures are silent, so the user gets no feedback.Consider adding a
.catchbranch to surface an error alert or at least log the failure, e.g.:- navigator.clipboard.writeText(text).then(() => { - setAlertDetails({ - type: "success", - content: `${label} copied to clipboard`, - }); - }); + navigator.clipboard.writeText(text) + .then(() => { + setAlertDetails({ + type: "success", + content: `${label} copied to clipboard`, + }); + }) + .catch(() => { + setAlertDetails({ + type: "error", + content: `Failed to copy ${label} to clipboard`, + }); + });
78-81: Clarify or fix the sharedloadingstate usage.Both
fetchExecutionDetailsandfetchExecutionFilescallsetLoading(false)infinally, butloadingis never set totrue. That means the Table’sloading={loading}will always be falsy and spinners won’t show.Either:
- Remove
loadingentirely if you don’t intend to show a loading state, or- Set
loadingtotruebefore starting each fetch and consider splitting into two flags (summary vs files) to avoid them stepping on each other.Also applies to: 107-198
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(13 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
frontend/src/components/logging/logs-table/LogsTable.jsx (1)
LogsTable(16-186)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx
[warning] 224-224: Prefer using an optional chain expression instead, as it's more concise and easier to read.
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 43-43: 'text' is missing in props validation
[warning] 422-422: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (4)
47-49: State setup for auto-refresh and search filter looks good.The
useReffor interval management is the correct pattern for cleanup, and separating theautoRefreshboolean state from the interval reference allows proper control flow.
101-143: LGTM - fetchLogs correctly includes the execution ID filter.The function properly passes
executionIdSearchto the API and handles the falsy case with|| null.
199-202: LGTM - Auto-refresh correctly resets on tab change.This ensures users don't inadvertently continue auto-refreshing stale data when switching between log types.
233-237: LogsRefreshControls integration looks correct.The component is properly wired with the auto-refresh state and manual refresh callback. Manual refresh will correctly use current filter values since it calls
fetchLogssynchronously.
Changed `id` filter from `CharFilter(lookup_expr="icontains")` to `UUIDFilter()` in ExecutionFilter. UUIDs don't support icontains lookup, so this fixes a potential FieldError. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…l state - Added terminal state detection for COMPLETED/ERROR/STOPPED statuses - Auto-refresh automatically disables when execution completes - Refresh button and toggle are disabled with visual feedback - Tooltip shows execution completion message when disabled 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
377-397: Critical: Auto-refresh still uses stale ordering and statusFilter values.This issue was flagged in a previous review but appears unresolved. The auto-refresh effect captures
orderingandstatusFilterfrom the closure but doesn't include them in the dependency array. When users change these values, the interval continues fetching with stale values, causing auto-refresh to ignore current filter and sort settings.Apply this diff to fix:
- }, [autoRefresh, id, pagination.current]); + }, [autoRefresh, id, pagination.current, ordering, statusFilter]);This ensures the interval is recreated whenever ordering or statusFilter changes, so it always uses current values.
🧹 Nitpick comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
104-111: Consider adding error handling for clipboard operations.The clipboard API can fail in certain contexts (permissions denied, insecure origins, browser restrictions). Adding a try-catch would improve user experience.
- const handleCopyToClipboard = (text, label = "Text") => { - navigator.clipboard.writeText(text).then(() => { - setAlertDetails({ - type: "success", - content: `${label} copied to clipboard`, - }); - }); - }; + const handleCopyToClipboard = (text, label = "Text") => { + navigator.clipboard + .writeText(text) + .then(() => { + setAlertDetails({ + type: "success", + content: `${label} copied to clipboard`, + }); + }) + .catch(() => { + setAlertDetails({ + type: "error", + content: `Failed to copy ${label}`, + }); + }); + };
427-448: Consider extracting the Action column title render function.The inline function component at line 435 is recreated on every render. While the impact is minimal since it's just a table header, extracting it would follow React best practices.
+const ActionColumnHeader = ({ menu }) => ( + <div className="action-column-header"> + <span>Action</span> + <Dropdown menu={menu} trigger={["click"]} placement="bottomRight"> + <span className="column-settings-trigger"> + <MoreOutlined className="column-settings-icon" /> + </span> + </Dropdown> + </div> +); + +ActionColumnHeader.propTypes = { + menu: PropTypes.object.isRequired, +}; + const columnsToShow = columnsDetailedTable .filter((col) => columnsVisibility[col.key]) .map((col) => { if (col.key === "action") { return { ...col, width: 80, align: "center", - title: () => ( - <div className="action-column-header"> - <span>Action</span> - <Dropdown menu={menu} trigger={["click"]} placement="bottomRight"> - <span className="column-settings-trigger"> - <MoreOutlined className="column-settings-icon" /> - </span> - </Dropdown> - </div> - ), + title: () => <ActionColumnHeader menu={menu} />, }; } return col; });Based on static analysis hints from SonarCloud.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(13 hunks)frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.css(1 hunks)frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.jsx (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
autoRefresh(93-93)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
autoRefresh(48-48)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 43-43: 'text' is missing in props validation
[warning] 435-435: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.css (1)
1-18: LGTM!The CSS is clean, well-scoped, and appropriately structured for the refresh controls component. The flexbox layout, disabled state styling, and fixed button height are all reasonable choices.
frontend/src/components/logging/logs-refresh-controls/LogsRefreshControls.jsx (2)
6-35: LGTM!The component is well-structured, reusable, and properly handles the disabled state. The conditional tooltip provides clear feedback to users when controls are disabled.
37-42: LGTM!PropTypes validation is complete and correctly reflects the component's required and optional props.
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (4)
97-102: LGTM!The terminal state detection logic is clean and correctly identifies when execution has completed, enabling proper auto-disable of the auto-refresh feature.
231-311: LGTM!The column configuration improvements are well-thought-out. The documented width strategy balances fixed and flexible columns appropriately, and the new Execution Time column and File Size sorting enhance usability.
328-353: LGTM!The table change handler correctly maps frontend column keys to backend field names and properly constructs ordering parameters. The functional update for pagination state is a good practice.
450-584: LGTM!The restructured layout with the dedicated container, enhanced header with copy functionality, integrated LogsRefreshControls, and improved pagination configuration align well with the PR objectives. The removal of the explicit scroll prop enables better flexbox-based space utilization.
Added PropTypes import and validation for StatusMessageCell component's 'text' prop to resolve ESLint error and prevent potential runtime issues. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- ExecutionLogs.jsx: Use optional chaining (value?.[0]) for cleaner null checks - LogsTable.jsx: Extract inline components (SearchFilterDropdown, SearchFilterIcon) with proper PropTypes for reusability and maintainability 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
365-402: Critical: auto‑refresh still uses stale sort/filter/search/pageSize valuesThe auto‑refresh interval effect only depends on
autoRefresh,id, andpagination.current, so when users changeordering,statusFilter,pagination.pageSize, or the search text, the existing interval keeps calling the oldfetchExecutionFilesclosure. This is exactly the stale‑ordering issue flagged in the earlier review, and it now also applies to filters, page size, and search text.Result: the table is briefly updated with the new sort/filter/search, but subsequent auto‑refresh ticks revert to the old query parameters.
To fix, recreate the interval whenever any query‑shaping state changes:
- useEffect(() => { + useEffect(() => { if (autoRefreshIntervalRef.current) { clearInterval(autoRefreshIntervalRef.current); autoRefreshIntervalRef.current = null; } if (autoRefresh) { autoRefreshIntervalRef.current = setInterval(() => { fetchExecutionDetails(id); fetchExecutionFiles(id, pagination.current); }, 30000); } return () => { if (autoRefreshIntervalRef.current) { clearInterval(autoRefreshIntervalRef.current); autoRefreshIntervalRef.current = null; } }; - }, [autoRefresh, id, pagination.current]); + }, [ + autoRefresh, + id, + pagination.current, + pagination.pageSize, + ordering, + statusFilter, + searchText, + ]);You may also want to include
autoRefreshin the dependencies of the terminal‑state effect to satisfy hook‑deps linters, though behavior is currently correct whenisTerminalStatetransitions from false → true.
🧹 Nitpick comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
98-107: Align TERMINAL_STATES with filter options or centralize status constants
TERMINAL_STATESincludes"STOPPED"whilefilterOptionsomits it, so a STOPPED execution would be treated as terminal (disabling auto‑refresh) but not be directly filterable in the table. Consider either adding"STOPPED"tofilterOptionsor centralizing status definitions so UI filters and terminal‑state logic cannot drift apart.Also applies to: 472-477
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (4)
autoRefresh(48-48)autoRefreshIntervalRef(49-49)pagination(39-43)ordering(46-46)frontend/src/components/logging/logs-table/LogsTable.jsx (1)
handleTableChange(133-153)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 440-440: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (5)
1-2: StatusMessageCell extraction and PropTypes look goodMoving
StatusMessageCellto top level and adding PropTypes addresses the earlier Sonar warning and avoids recreating the component on every render. The overflow/tooltip logic is self‑contained and appropriate for the status message column.Also applies to: 40-40, 43-72
182-193: Query parameter construction is robustUsing
Object.entriesand explicit null/undefined checks, plus a separate loop forstatusFilter, keeps the URLSearchParams clean and avoids sending empty values. This is a nice improvement over ad‑hoc query string concatenation.
236-321: Column width and sorting configuration is consistentThe fixed/percentage widths, added
ellipsis, andExecution Timecolumn withsorter: trueand field mapping (executionTime → execution_time) are all consistent with the rest of the table logic and backend expectations. This should significantly improve readability without breaking sorting.
333-358: Table change handler correctly wires sorting and status filters
handleTableChangenow cleanly updates pagination, maps sortable fields to backend names (includingfileSizeandexecutionTime), and stores the status filter array for use when building the request. This matches the pattern inLogsTable.jsxand should behave as users expect.
455-479: Header, controls, and table pagination integration look solidThe new header layout (back button, Execution ID with copy, and
LogsRefreshControls) plus the table’s enhanced pagination (showSizeChanger,pageSizeOptions,showTotal) are wired correctly to existing handlers (navigate,handleCopyToClipboard,handleRefresh,handleTableChange) and should provide a smoother UX.Also applies to: 565-577
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
178-203: Auto‑refresh interval still uses stale filters/sort and keeps polling off the main listThe auto‑refresh effect only depends on
autoRefreshandpagination.current, so the interval callback closes over whateverselectedDateRange,ordering,executionIdSearch,activeTab, andpagination.pageSizewere when the interval was created. Subsequent changes to filters, sort, or page size won’t be reflected in auto‑refresh, and the interval also continues to run even when you’re no longer on the root logs list (e.g., detailed view), causing unnecessary API calls and confusing “jump back” behavior when results suddenly reflect old criteria.You can fix this by:
- Including all fetch‑relevant state (and view context) in the effect’s dependency array.
- Only starting the interval when you’re on the main list view (no
id,!currentPath).Example patch:
- // Auto-refresh interval management - useEffect(() => { - if (autoRefreshIntervalRef.current) { - clearInterval(autoRefreshIntervalRef.current); - autoRefreshIntervalRef.current = null; - } - - if (autoRefresh) { - autoRefreshIntervalRef.current = setInterval(() => { - fetchLogs(pagination.current); - }, 30000); - } - - return () => { - if (autoRefreshIntervalRef.current) { - clearInterval(autoRefreshIntervalRef.current); - autoRefreshIntervalRef.current = null; - } - }; - }, [autoRefresh, pagination.current]); + // Auto-refresh interval management + useEffect(() => { + if (autoRefreshIntervalRef.current) { + clearInterval(autoRefreshIntervalRef.current); + autoRefreshIntervalRef.current = null; + } + + // Only auto-refresh on the main logs list with up-to-date filters/sort. + const shouldAutoRefresh = autoRefresh && !currentPath && !id; + + if (shouldAutoRefresh) { + autoRefreshIntervalRef.current = setInterval(() => { + fetchLogs(pagination.current); + }, 30000); + } + + return () => { + if (autoRefreshIntervalRef.current) { + clearInterval(autoRefreshIntervalRef.current); + autoRefreshIntervalRef.current = null; + } + }; + }, [ + autoRefresh, + pagination.current, + pagination.pageSize, + selectedDateRange, + ordering, + executionIdSearch, + activeTab, + currentPath, + id, + ]);This ensures auto‑refresh always uses the latest filters/sort and stops polling once you leave the main execution‑logs list.
🧹 Nitpick comments (2)
frontend/src/components/logging/logs-table/LogsTable.jsx (1)
71-78: Inline filterDropdown/filterIcon functions are fine; Sonar warning can be treated as optionalSonar’s warning about “component definition inside parent” here is effectively about these inline arrow functions returning JSX. Given their trivial size and that they just wire global state (
executionIdSearch) into the column, the additional allocations per render are negligible. If you want to appease Sonar or marginally reduce allocations, you could hoist these to named callbacks (e.g., withuseCallback) or wrap the Execution ID column definition in a small memoized component, but it’s not required.frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
163-176: Server‑side fetch wiring is correct; consider page reset on filter/search changesThe main data‑fetch effect correctly ties
fetchLogstoactiveTab, pagination, date range, ordering,executionIdSearch, andcurrentPath, and the params passed intoaxiosPrivate.getalign with those dependencies. One UX quirk to be aware of: whenselectedDateRangeorexecutionIdSearchchange while you are on a later page,pagination.currentis preserved, so the API may legitimately return no rows even if matches exist on page 1. If that’s undesirable, you could reset to the first page whenever filters/search change (e.g., by updatingpagination.currentto 1 in the handlers that update those filters).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(9 hunks)frontend/src/components/logging/logs-table/LogsTable.jsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (5)
autoRefresh(98-98)autoRefreshIntervalRef(99-99)pagination(86-90)ordering(93-93)loading(85-85)frontend/src/components/logging/logs-table/LogsTable.jsx (1)
LogsTable(42-204)
frontend/src/components/logging/logs-table/LogsTable.jsx (3)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (3)
onChange(90-93)executionIdSearch(47-47)pagination(39-43)frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
handleTableChange(333-358)pagination(86-90)frontend/src/components/logging/log-modal/LogModal.jsx (2)
handleTableChange(127-144)pagination(32-36)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/logs-table/LogsTable.jsx
[warning] 71-71: Move this component definition out of the parent component and pass data as props.
[warning] 77-77: Move this component definition out of the parent component and pass data as props.
🔇 Additional comments (5)
frontend/src/components/logging/logs-table/LogsTable.jsx (3)
16-41: SearchFilterDropdown/SearchFilterIcon look correct and self‑containedThe small, stateless components cleanly encapsulate the search input and active‑state icon and are correctly typed with PropTypes; no issues from a correctness or readability standpoint.
151-171: Backend sort mapping in handleTableChange is consistent and safeThe
fieldMapcorrectly aligns frontend fields (executedAt,executionTime) to backend fields and produces the expectedorderingstrings (created_atvs-created_at, etc.). Resettingorderingtonullwhensorter.orderis falsy is also appropriate for clearing sort on the API side.
173-183: Pagination config is compatible with remote data and caller statePassing the full
paginationobject through, enablingshowSizeChanger, and using stringpageSizeOptionswith a customshowTotalformatter all match antd’s expectations and ExecutionLogs’ pagination shape, so server‑side pagination should behave as intended.frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (2)
216-237: RangePicker and refresh controls are wired safelyThe RangePicker
onChangenow guards against partial/cleared selections before callingtoISOString, which avoids the null‑access issue from earlier iterations, and it keepsdatePickerValueandselectedDateRangein sync.LogsRefreshControlsis integrated cleanly, with manual refresh correctly invokingfetchLogs(pagination.current)andautoRefreshstate managed locally.
241-249: Execution ID search plumbing between ExecutionLogs and LogsTable is soundPassing
executionIdSearchandsetExecutionIdSearchdown toLogsTablelets the Execution ID column’s filter input directly drive the backendidparam via the main effect. From a data‑flow standpoint, this is consistent and keeps the source of truth for filters in the parent.
chandrasekharan-zipstack
requested review from
athul-rs,
jagadeeswaran-zipstack,
jaseemjaskp and
vishnuszipstack
Excluded "action" column from the visibility menu to prevent users from losing access to the column visibility dropdown. This ensures the Action column remains always visible, maintaining user ability to interact with the table controls. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
502-509: Potential null reference onexecutionDetails.status.The optional chaining stops at
executionDetails?but.status.toLowerCase()will throw ifstatusisundefinedornull. This can cause a runtime crash if the API returns execution details without a status field.- <Typography className="logging-card-title"> - {executionDetails?.status.toLowerCase() === "executing" - ? "Running for" - : "Ran for"} - </Typography> + <Typography className="logging-card-title"> + {executionDetails?.status?.toLowerCase() === "executing" + ? "Running for" + : "Ran for"} + </Typography>The same fix should be applied at line 516:
- {executionDetails?.status.toLowerCase() === "executing" + {executionDetails?.status?.toLowerCase() === "executing"
♻️ Duplicate comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
109-116: Handle clipboard failures and missingnavigator.clipboard.This was flagged in a previous review. The function should check for
navigator.clipboardavailability and handle promise rejection to provide user feedback when copy fails.
382-402: Auto-refresh uses staleorderingandstatusFiltervalues.The auto-refresh interval captures
fetchExecutionFilesandfetchExecutionDetailsin its closure. WhenorderingorstatusFilterchange, the interval continues using the stale function references, causing refreshes to ignore current filter and sort settings.Add
orderingandstatusFilterto the dependency array:- }, [autoRefresh, id, pagination.current]); + }, [autoRefresh, id, pagination.current, ordering, statusFilter]);
🧹 Nitpick comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
376-380: MissingautoRefreshin dependency array.The effect checks
autoRefreshbut doesn't list it as a dependency, which can lead to stale behavior if the dependency linter isn't enforced.useEffect(() => { if (isTerminalState && autoRefresh) { setAutoRefresh(false); } - }, [isTerminalState]); + }, [isTerminalState, autoRefresh]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(13 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 442-442: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (9)
43-72: Good refactor: StatusMessageCell properly extracted.The component is now defined outside the parent, preventing unnecessary re-creation on each render. PropTypes validation is also in place.
118-148: LGTM!The
createdAtRawunused field has been removed as flagged in the previous review, and error handling is properly implemented.
150-209: LGTM!Clean implementation with proper parameter handling and error management.
236-331: Well-structured column configuration.The documented column width strategy (fixed px for predictable content, percentages for variable content) provides a good balance of readability and responsiveness. The addition of sorting on File Size and Execution Time enhances usability.
333-358: LGTM!The field mapping for backend API compatibility is well-implemented, and the handler correctly manages pagination, sorting, and filtering state.
418-431: Good fix: Action column excluded from visibility menu.Excluding the Action column from the toggle menu prevents users from hiding the column visibility controls, addressing the accessibility concern from the previous review.
437-455: Static analysis note: inline render function is acceptable here.SonarCloud flags the inline function at line 442 as a component definition. However, this is a render function pattern (common in Ant Design table
titleprops), not a stateful component. Since it doesn't maintain internal state or use hooks, the re-creation on each render is acceptable and won't cause issues.
474-479: LGTM!The
LogsRefreshControlsintegration properly passes thedisabledprop based on terminal state, ensuring auto-refresh is disabled when execution completes.
566-582: Good enhancement to table pagination.Adding
showSizeChanger,pageSizeOptions, andshowTotalimproves the user experience by providing more control over pagination and visibility into the total record count.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
397-417: Critical: Auto-refresh still uses stale ordering and statusFilter values.The auto-refresh interval effect captures
orderingandstatusFilterin its closure but doesn't depend on them. When users change these values, the 30-second interval continues fetching with stale values, overwriting the user's intended sort and filter settings.Apply this diff to fix the stale closure:
- }, [autoRefresh, id, pagination.current]); + }, [autoRefresh, id, pagination.current, ordering, statusFilter]);This ensures the interval is recreated with fresh closures whenever sorting or filtering changes.
🧹 Nitpick comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
390-395: Consider addingautoRefreshto dependency array for completeness.While the effect works correctly (controls are disabled when
isTerminalStateis true), React's exhaustive-deps rule would flagautoRefreshas a missing dependency. Adding it improves code correctness without changing behavior.useEffect(() => { if (isTerminalState && autoRefresh) { setAutoRefresh(false); } - }, [isTerminalState]); + }, [isTerminalState, autoRefresh]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (4)
autoRefresh(48-48)pagination(39-43)ordering(46-46)navigate(34-34)frontend/src/components/logging/logs-table/LogsTable.jsx (2)
handleTableChange(151-171)navigate(52-52)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 457-457: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (4)
43-72: Excellent refactor: StatusMessageCell moved outside parent component.This change addresses the previous review feedback by moving
StatusMessageCellto module scope and adding PropTypes validation. This prevents unnecessary re-creation on every render and improves performance.
109-131: Well-implemented error handling for clipboard operations.The clipboard handler now properly checks for
navigator.clipboardavailability and handles both success and failure cases with appropriate user feedback. This addresses the previous review concerns.
385-388: Good fix: Effect now includesidin dependency array.This correctly addresses the previous review feedback by ensuring the effect re-runs when the route parameter changes.
433-447: Good fix: Action column excluded from visibility toggle.By filtering out the Action column from the visibility menu, you've ensured the column-visibility dropdown remains accessible at all times. This addresses the previous accessibility concern.
athul-rs
left a comment
athul-rs
left a comment
There was a problem hiding this comment.
LGTM overall, left a NIT comment @chandrasekharan-zipstack
Please look into the sonar comment as well.
muhammad-ali-e
left a comment
muhammad-ali-e
left a comment
There was a problem hiding this comment.
plase resolve sonar cloud issues if valid
|
@chandrasekharan-zipstack Would be better if we add serach box to search workflow_execution_id and workflow_file_execution id in logs |
|
@muhammad-ali-e not the intention of this PR. Please create a jira ticket for the same. This was done to rectify a ux issue one of our customer was facing
|
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/components/logging/logs-table/LogsTable.css (1)
69-81: Search filter styles are consistent; consider reuse if other tables add searchThe
.search-container,.search-input, and.search-filter-icon-activestyles cleanly match the new Execution ID filter UI. If you end up adding similar column search to other tables, consider centralizing these classes (or using a shared form/search CSS module) to avoid duplicating styling later.frontend/src/components/logging/logs-table/LogsTable.jsx (3)
1-1: Execution ID search UI is clean; verify wiring and PR scopeNice extraction of
SearchFilterDropdown/SearchFilterIconinto small reusable components with PropTypes. A couple of things to double‑check:
- The
Execution IDcolumn’sfilterDropdown/filterIcononly driveexecutionIdSearch; Ant Design’s built‑in filter plumbing (filteredValue,onFilter,confirm, etc.) isn’t used. That’s fine if the parent (e.g.ExecutionLogs.jsx) actually filters onexecutionIdSearchat the API/query layer, but worth confirming so the search box isn’t purely cosmetic.- Because you control the “active” state via
!!executionIdSearch, the icon will show as active whenever there’s a non‑empty string, regardless of any server‑side errors or mismatch. That’s probably OK, just be aware it’s decoupled from Table’s internal filter state.- From the PR discussion, there was some hesitation about adding search in this PR; make sure this Execution ID search behaviour is aligned with the agreed scope before merge.
If this is meant to be a general pattern, you might later want to pass the full
filterDropdownprops intoSearchFilterDropdownso it can triggerconfirm/clearFiltersfor more canonical Ant Design UX.Also applies to: 16-40, 68-78
137-148: Tighten column renderers for edge values (0, undefined)Two minor robustness tweaks:
Execution Timecurrently usesrecord?.execution_time || "-". Ifexecution_timecan ever be0, it will render as"-". Using nullish coalescing keeps0visible:- render: (_, record) => record?.execution_time || "-", + render: (_, record) => record?.execution_time ?? "-",
- For
Files Processed, if eitherprocessedortotalFilesis missing you’ll get"undefined/undefined". If that’s a realistic state, consider a safe fallback (e.g."-"or"0/0") instead of leakingundefinedinto the UI.
151-171: SimplifyhandleTableChangepagination updates and confirm sorter mappingThe backend field mapping for sorting (
executedAt → created_at,executionTime → execution_time) looks correct and matches the patterns used in other logging components.In the
sorter.orderbranch you callsetPaginationtwice: once to mergepagination, then again to also forcecurrent: 1. The first call is effectively redundant and just adds an extra state update:- setPagination((prev) => { - return { ...prev, ...pagination }; - }); … if (sorter.order) { … setOrdering(order); setPagination((prev) => { return { ...prev, ...pagination, current: 1 }; }); } else { setOrdering(null); }Collapsing to a single
setPaginationin this branch will avoid an unnecessary rerender. Also verify that resetting to page 1 on sort change is the desired UX here (it’s consistent with many tables, but worth confirming with PM/UX).Also applies to: 173-183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/logging/logs-table/LogsTable.css(2 hunks)frontend/src/components/logging/logs-table/LogsTable.jsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/logging/logs-table/LogsTable.jsx (3)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (3)
onChange(90-93)executionIdSearch(47-47)pagination(39-43)frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
handleTableChange(348-373)pagination(86-90)frontend/src/components/logging/log-modal/LogModal.jsx (2)
handleTableChange(127-144)pagination(32-36)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/logs-table/LogsTable.jsx
[warning] 77-77: Move this component definition out of the parent component and pass data as props.
[warning] 71-71: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
frontend/src/components/logging/logs-table/LogsTable.css (1)
1-50: Flex layout and sticky header look good; watch for nested scrollbarsThe flex +
min-height: 0adjustments on the Ant Table wrappers and containers should solve the “table not filling vertical space” issue and make the header stick nicely. With both.ant-tableand.ant-table-bodyhavingoverflowset, just sanity‑check in the app that you don’t get double scrollbars in certain parent layouts (small viewports, nested modals, etc.).Also applies to: 52-58
frontend/src/components/logging/logs-table/LogsTable.jsx (1)
173-183: Pagination and new props are wired correctlyThe enhanced pagination config (
showSizeChanger,pageSizeOptions,showTotal) is consistent with antd’s API, and reusing the caller’spaginationobject keeps state centralized. The addedexecutionIdSearch/setExecutionIdSearchPropTypes correctly document the new public API surface for this table.Also applies to: 206-215
- DetailedLogs.jsx: Extracted ActionColumnHeader component to module level - LogsTable.jsx: Removed unnecessary arrow function wrappers for filterDropdown and filterIcon - Resolves SonarQube code smell for inline component definitions 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
465-477: Consider memoizingcolumnsToShowfor performance.The filtered and mapped column array is recalculated on every render. For better performance, especially with frequent re-renders during auto-refresh, consider wrapping this logic in
useMemowith dependencies oncolumnsVisibilityandcolumnsDetailedTable.Example:
const columnsToShow = useMemo( () => columnsDetailedTable .filter((col) => columnsVisibility[col.key]) .map((col) => { if (col.key === "action") { return { ...col, width: 80, align: "center", title: () => <ActionColumnHeader menu={menu} />, }; } return col; }), [columnsVisibility, columnsDetailedTable, menu] );Note: The static analysis warning at line 473 about "component definition" is a false positive—the arrow function in the
titleproperty is standard Ant Design Table API usage, not a problematic component definition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/src/components/logging/detailed-logs/DetailedLogs.css(3 hunks)frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(13 hunks)frontend/src/components/logging/logs-table/LogsTable.jsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (3)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (3)
autoRefresh(48-48)pagination(39-43)ordering(46-46)frontend/src/components/logging/log-modal/LogModal.jsx (3)
handleTableChange(127-144)pagination(32-36)ordering(31-31)frontend/src/components/logging/logs-table/LogsTable.jsx (1)
handleTableChange(151-171)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx
[warning] 473-473: Move this component definition out of the parent component and pass data as props.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
frontend/src/components/logging/detailed-logs/DetailedLogs.css (1)
1-171: LGTM! Clean CSS supporting the new responsive layout.The flexbox layout strategy properly fills available vertical space, custom scrollbar styling improves UX, and the action column header with visibility controls is well-implemented. The percentage-based widths for variable content (Status Message 30%, File Name 12%, File Path 20%) align with the PR objectives.
frontend/src/components/logging/logs-table/LogsTable.jsx (3)
16-40: Well-structured search filter components.The
SearchFilterDropdownandSearchFilterIconcomponents are cleanly implemented with proper PropTypes validation. The active state styling provides good visual feedback.
151-171: Good backend field mapping for sorting.The
fieldMapcorrectly translates frontend column keys (executedAt,executionTime) to their backend equivalents (created_at,execution_time), and properly handles ascending/descending order with the-prefix. Resetting tonullwhen sorting is cleared is the right approach.
177-183: Pagination enhancements improve usability.Enabling
showSizeChangerwith multiple page size options (10, 20, 50, 100) and addingshowTotaldisplay gives users better control and visibility over the data set.frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (6)
43-72: Excellent refactor: StatusMessageCell properly extracted.Moving this component outside the parent prevents unnecessary re-creation on every render and includes proper PropTypes validation. The overflow detection with conditional tooltip is a nice UX touch.
74-88: ActionColumnHeader properly extracted with clean API.This component is correctly defined outside the parent and receives the menu via props, avoiding re-creation issues flagged in previous reviews.
125-147: Robust clipboard handling with proper error cases.The error handling now covers missing
navigator.clipboard, permission denials, and write failures with appropriate user feedback. This addresses the previous review concern.
364-389: Backend field mapping correctly implemented.The
fieldMapproperly translatesexecutedAt → created_at,fileSize → file_size, andexecutionTime → execution_time, matching the pattern established in LogsTable.jsx. The sorting prefix and reset logic are correct.
414-433: Auto-refresh interval management looks correct.The interval properly cleans up on unmount or when auto-refresh is disabled. Since
fetchExecutionFilesreadsordering,statusFilter, andsearchTextfrom current state (not closures), the interval will use up-to-date values even though those aren't in the dependency array. Includingpagination.currentensures the interval is recreated when the page changes.
449-463: Good fix: Action column excluded from visibility toggle.Filtering out
col.key !== "action"ensures users can't hide the column that contains the visibility dropdown itself, preventing the inaccessibility issue flagged in previous reviews.
…le prop Simplified the title prop by removing redundant arrow function wrapper. The title prop accepts JSX elements directly, so the wrapper was unnecessary. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
414-433: Critical: Auto-refresh still uses staleorderingandstatusFiltervalues.Despite being marked as addressed in previous reviews, the auto-refresh effect is still missing
orderingandstatusFilterin the dependency array. The interval callback captures these values from the closure at the time the effect runs. When users change sort order or status filter, the interval continues fetching with stale values, causing auto-refresh to ignore current filter and sort settings.Apply this diff to fix the stale closure issue:
return () => { if (autoRefreshIntervalRef.current) { clearInterval(autoRefreshIntervalRef.current); autoRefreshIntervalRef.current = null; } }; - }, [autoRefresh, id, pagination.current]); + }, [autoRefresh, id, pagination.current, ordering, statusFilter]);This ensures the interval is recreated whenever ordering or statusFilter change, so it always uses current values when calling
fetchExecutionFiles.Note: You may also want to include
searchTextin the dependency array for the same reason, though search changes are less likely during auto-refresh scenarios.
🧹 Nitpick comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
86-87: Use more specific PropTypes shape for themenuprop.The
menuprop is validated asPropTypes.object.isRequired, which is too generic. Consider using a more specific shape that matches Ant Design's Dropdown menu structure.Apply this diff to improve type validation:
ActionColumnHeader.propTypes = { - menu: PropTypes.object.isRequired, + menu: PropTypes.shape({ + items: PropTypes.arrayOf( + PropTypes.shape({ + key: PropTypes.string, + label: PropTypes.node, + }) + ), + }).isRequired, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (3)
autoRefresh(48-48)autoRefreshIntervalRef(49-49)pagination(39-43)frontend/src/components/logging/logs-table/LogsTable.jsx (1)
handleTableChange(151-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
chandrasekharan-zipstack
deleted the
UN-2966-auto-refresh-execution-logs
branch
What
LogsRefreshControlscomponent for both list and detail viewsWhy
How
logs-refresh-controls/LogsRefreshControls.jsxwith accompanying CSSExecutionLogs.jsx/css- integrated refresh controls, updated layout, removed fixed scroll height constraintsDetailedLogs.jsx/css- uses shared LogsRefreshControls component with percentage-based column widthsLogsTable.jsx/css- adjusted for dynamic height management with flexbox layoutCan this PR break any existing features? If yes, please list possible items. If no, please explain why.
No. Changes are purely additive with default OFF state for new features. Layout changes use CSS-based flexbox without altering data flow or business logic. Existing logging functionality and API calls remain unchanged. Column width changes are visual-only improvements.
Relevant Docs
Related Issues or PRs
Dependencies Versions / Env Variables
Notes on Testing
Screenshots
Checklist