-
-
Notifications
You must be signed in to change notification settings - Fork 114
feat: add time filter for deployment and application logs #914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces time-range filtering for deployment logs by adding a date-time picker UI with hour, minute, and AM/PM selectors, RFC3339 timestamp conversion logic in the hook layer, and extended API parameters (start_time, end_time) to support time-bounded log retrieval. Changes
Sequence DiagramsequenceDiagram
actor User
participant DatePicker as DateFilter Component
participant Hook as use_deployment_logs_viewer
participant API as deployApi
participant Backend as Log Service
User->>DatePicker: Select date range & time
DatePicker->>DatePicker: formatToDateTimeLocal()
DatePicker->>Hook: onChange(startDate, endDate)
Hook->>Hook: convertToRFC3339(startDate, endDate)
Hook->>API: getDeploymentLogs({start_time, end_time, ...})
API->>Backend: GET /logs?start_time=RFC3339&end_time=RFC3339
Backend-->>API: Filtered logs (within time range)
API-->>Hook: ApplicationLogsResponse
Hook->>Hook: isWithinDateRange() validation
Hook->>Hook: formatTimestamp() (12-hour display)
Hook-->>DatePicker: Formatted logs
DatePicker-->>User: Display filtered logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
view/app/self-host/hooks/use_deployment_logs_viewer.ts (2)
90-98: Redundant condition check.The condition
!timePart.includes('.')is checked both in the outerif(line 90) and again inside the block (line 93). The inner check is always true when this branch executes.Suggested fix
} else if (colonCount === 2 && !timePart.includes('.')) { // Format: HH:mm:ss, already has seconds // For end dates, add milliseconds if not present - if (isEndDate && !timePart.includes('.')) { + if (isEndDate) { timeWithSeconds = `${timePart}.999`; } else { timeWithSeconds = timePart; } }
311-373: Consider consolidating date parsing logic.The date format parsing in
isWithinDateRangeduplicates much of the logic inconvertToRFC3339. While client-side filtering is useful for WebSocket messages, the duplicated parsing could diverge over time.Consider extracting a shared helper for parsing datetime-local strings, or reusing
convertToRFC3339here by comparing ISO strings directly.view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsx (3)
293-337: Move helper functions outside the component.
parseDateTimeandformatToDateTimeLocalare pure functions that don't depend on component state or props. Defining them inside the component causes them to be recreated on every render.Move functions outside the component
+// Move these outside the component, before the DateFilter function +function parseDateTime(dateTimeStr: string) { + if (!dateTimeStr) { + return { date: '', hour: null, minute: null, ampm: null }; + } + + if (!dateTimeStr.includes('T')) { + return { date: dateTimeStr, hour: null, minute: null, ampm: null }; + } + + const [datePart, timePart] = dateTimeStr.split('T'); + const [hour24, minute] = timePart.split(':').map(Number); + + const hour12 = hour24 === 0 ? 12 : hour24 > 12 ? hour24 - 12 : hour24; + const ampm = hour24 >= 12 ? 'PM' : 'AM'; + + return { date: datePart, hour: hour12, minute: minute || 0, ampm }; +} + +function formatToDateTimeLocal( + date: string, + hour12: number | null, + minute: number | null, + ampm: string | null +): string { + if (!date) return ''; + if (hour12 === null || minute === null || ampm === null) { + return date; + } + + let hour24 = hour12; + if (ampm === 'PM' && hour12 !== 12) { + hour24 = hour12 + 12; + } else if (ampm === 'AM' && hour12 === 12) { + hour24 = 0; + } + + const hour24Str = hour24.toString().padStart(2, '0'); + const minuteStr = minute.toString().padStart(2, '0'); + + return `${date}T${hour24Str}:${minuteStr}`; +} function DateFilter({ label, value, onChange }: { label: string; value: string; onChange: (v: string) => void; }) { - // Parse datetime-local format... - const parseDateTime = (dateTimeStr: string) => { - ... - }; - - // Convert 12-hour format... - const formatToDateTimeLocal = (...) => { - ... - };
350-424: Consider wrapping handlers inuseCallback.Per coding guidelines, use
useCallbackto prevent unnecessary re-renders. These handlers are passed to child components and are recreated on each render.Example for handleDateChange
- const handleDateChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const handleDateChange = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { const newDate = e.target.value; if (newDate) { onChange(formatToDateTimeLocal(newDate, hour, minute, ampm)); } else { onChange(''); } - }; + }, [hour, minute, ampm, onChange]);Apply similar pattern to
handleHourChange,handleMinuteChange,handleMinuteBlur, andhandleAmPmChange.
268-280: Add i18n for user-facing strings.Per coding guidelines, user-facing strings should use the
useTranslationhook. The following hardcoded strings in the changed code need i18n:
- Line 269:
"From"- Line 274:
"To"- Line 438:
"Hour"(placeholder)- Line 462:
"AM/PM"(placeholder)Also applies to: 427-468
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsxview/app/self-host/hooks/use_deployment_logs_viewer.tsview/redux/services/deploy/applicationsApi.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{ts,tsx}: Before writing any new logic, search the codebase for existing implementations inview/hooks/,view/lib/utils.ts,view/components/ui/, andview/redux/services/before creating custom elements
Extract repeated patterns into custom hooks or shared components
Use early returns and flat structure instead of nested conditions for improved code readability
Always use typed hooksuseAppDispatchanduseAppSelectorfrom@/redux/hooksinstead of untypeduseDispatchanduseSelector
Never useanytype; always use explicit types for function parameters and return values
Remove unused imports immediately and delete commented-out code
Keep files focused and minimal by removing unused variables and functions
In comments, explain WHY code works a certain way, not WHAT it does
Never disable lint rules without strong justification
Files:
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsxview/app/self-host/hooks/use_deployment_logs_viewer.tsview/redux/services/deploy/applicationsApi.ts
view/app/**/components/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Keep domain-related components in
app/[domain]/components/and shared components incomponents/[feature]/
Files:
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsx
view/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
view/**/*.{tsx,ts}: Each component file should export one main component
Never write plain HTML for interactive elements; always use shadcn components like Button, Card, Badge, Skeleton
Use thecn()utility function for conditional Tailwind classes
No hardcoded user-facing strings; always use i18n with theuseTranslationhook
Handle loading states by checkingisLoadingfrom RTK Query and returning skeleton loaders or loading components
Handle error states by checking error values from RTK Query and displaying error messages using i18n
Show empty state UI when data is empty using i18n messages
Always provide skeleton loaders for async content instead of blank/white space
Use semantic HTML elements for accessibility
Provide proper ARIA labels where needed for accessibility
Memoize expensive computations withuseMemo
Prevent unnecessary re-renders withuseCallback
Use React Query's caching effectively to avoid redundant API calls
Lazy load heavy components when appropriate
Ensure responsive design works on all screen sizes
Files:
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsxview/app/self-host/hooks/use_deployment_logs_viewer.tsview/redux/services/deploy/applicationsApi.ts
view/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
view/**/*.{ts,tsx}: Respect ESLint rules configured inview/eslint.config.mjs
Use Prettier for consistent formatting and fix all lint errors before committing
Files:
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsxview/app/self-host/hooks/use_deployment_logs_viewer.tsview/redux/services/deploy/applicationsApi.ts
view/redux/services/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
view/redux/services/**/*.ts: Services (RTK Query) should handle API communication only
Use RTK Query for state management withcreateApi,baseQueryWithReauth, and propertagTypesandinvalidatesTagsfor cache management
Files:
view/redux/services/deploy/applicationsApi.ts
🧠 Learnings (2)
📚 Learning: 2025-12-27T03:02:43.092Z
Learnt from: CR
Repo: raghavyuva/nixopus PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2025-12-27T03:02:43.092Z
Learning: Applies to view/**/*.{tsx,ts} : Prevent unnecessary re-renders with `useCallback`
Applied to files:
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsx
📚 Learning: 2025-12-27T03:02:43.092Z
Learnt from: CR
Repo: raghavyuva/nixopus PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2025-12-27T03:02:43.092Z
Learning: Applies to view/**/*.{tsx,ts} : Never write plain HTML for interactive elements; always use shadcn components like Button, Card, Badge, Skeleton
Applied to files:
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsx
🔇 Additional comments (4)
view/app/self-host/hooks/use_deployment_logs_viewer.ts (2)
393-411: LGTM with minor optimization opportunity.The 12-hour format logic is correct. Multiple
toLocaleStringcalls could be consolidated into a single call with combined options for slight efficiency gains, but the current approach is clear and maintainable.
169-173: LGTM!Correctly resets pagination and accumulated logs when filter values change, ensuring stale data doesn't persist when users modify the time range.
view/app/self-host/components/deployment-logs/DeploymentLogsTable.tsx (1)
659-671: LGTM!The simplified overflow wrapper maintains the scrolling behavior appropriately.
view/redux/services/deploy/applicationsApi.ts (1)
206-232: LGTM!The endpoint extension correctly adds optional
level,start_time, andend_timeparameters, consistent with thegetApplicationLogsendpoint pattern. The RTK Query implementation follows best practices with proper typing and cache tagging.
be7d566 to
8de6b2f
Compare
8de6b2f to
a6a93c6
Compare
Issue
feat: add time filter for deployment and application logs #906
Description
added date + time filter for deployment and application logs
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.