feat: range selection via chart #2867
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes enhance the interactivity and data handling of the dashboard logs. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LC as LogsChart Component
participant F as Filter State
U->>LC: MouseDown (start selection)
LC->>LC: Set selection.start based on mouse position
U->>LC: MouseMove (drag to select)
LC->>LC: Update selection.end dynamically
U->>LC: MouseUp (complete selection)
LC->>F: Update filters with selected time range
LC->>LC: Reset selection state after processing
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/dashboard/lib/trpc/routers/logs/query-distinct-paths.ts (2)
7-8: Consider validating input range forcurrentDate.Currently, only the type of
currentDateis being validated (i.e., it is a number). If needed, consider adding additional constraints (e.g., must be positive, must not exceed current time, etc.) to guard against malformed or extreme values.
15-21: Log the original error for debugging purposes.While throwing a
TRPCErrorwith a custom message is a good practice, storing or logging the_errcan help diagnose issues when investigating support tickets or logs. Ensure that the error details are captured somewhere, possibly in a logging or monitoring system.apps/dashboard/app/(app)/logs/components/charts/index.tsx (3)
37-42: Consider using more specific types for Selection.The
Selectiontype could be more specific to improve type safety:
startandendcould be more strictly typed based on the actual values they can hold- Consider using
number | nullinstead of optional properties for timestampstype Selection = { - start: string | number; - end: string | number; - startTimestamp?: number; - endTimestamp?: number; + start: number; + end: number; + startTimestamp: number | null; + endTimestamp: number | null; };
51-51: Improve selection state management.The selection state initialization and reset could be improved:
- Consider using
nullfor initial/reset state instead of empty strings- This would better represent the absence of a selection
- const [selection, setSelection] = useState<Selection>({ start: "", end: "" }); + const [selection, setSelection] = useState<Selection | null>(null); // In reset - setSelection({ - start: "", - end: "", - startTimestamp: undefined, - endTimestamp: undefined, - }); + setSelection(null);Also applies to: 118-124
89-124: Simplify time range sorting logic.The sort operation could be simplified using destructuring:
- const [start, end] = [selection.startTimestamp, selection.endTimestamp].sort((a, b) => a - b); + const [start, end] = [selection.startTimestamp, selection.endTimestamp].sort();Also, consider extracting the filter update logic to a separate function for better readability and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/logs/components/charts/index.tsx(5 hunks)apps/dashboard/app/(app)/logs/components/charts/utils/convert-date-to-local.ts(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/paths-filter.tsx(2 hunks)apps/dashboard/lib/trpc/routers/logs/query-distinct-paths.ts(1 hunks)apps/dashboard/styles/tailwind/tailwind.css(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/dashboard/lib/trpc/routers/logs/query-distinct-paths.ts (3)
23-28: Good use ofNOT_FOUNDTRPCError.The error handling pattern for a missing workspace is clear and consistent, improving readability and debugging for consumers of this API.
30-30: Evaluate the 12-hour offset logic.Subtracting 12 hours from
currentDatemay be correct for the current requirement, but ensure it's not inadvertently omitting data or missing edge cases (e.g., different user time zones or shifts at midnight). Clarify if 12 hours is a hard-coded standard or subject to future parameterization.
47-48: Ensure empty result handling meets business logic.Returning an empty array is a safe fallback. Confirm this is aligned with consumers' expectations to gracefully handle cases where no paths are found.
apps/dashboard/app/(app)/logs/components/charts/utils/convert-date-to-local.ts (1)
3-8: Confirm local time conversion requirements.The function offsets the provided date by the current timezone offset. This may be sufficient for simple local conversions, but might not account for DST transitions in all locales. Verify this aligns with user expectations and ensure thorough testing in different time zones.
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/components/paths-filter.tsx (5)
6-6: Check necessity ofuseCallbackanduseMemo.You’ve introduced
useMemo(() => Date.now(), []); this ensuresdateNowremains constant across renders. Verify that the desired behavior is truly a once-on-mount snapshot, rather than a continuously updated timestamp.
10-12: PassingcurrentDateto the query is well-structured.Fetching paths based on a specific timestamp fosters clarity and consistency. This aligns with the server-side logic in
queryDistinctPaths.
14-22: Neat transformation ofpathsdata.Selecting and mapping the array to a standardized shape keeps the component clean and decoupled from the raw API response.
24-24: Well-defined query options block.Providing a custom
selectoption is a neat way to shape your data without extra transformations in downstream logic.
84-84: Smooth scroll behavior.The
max-h-64 overflow-autoensures the scroll is contained. Removing the old scroll event-tracking logic appears to simplify the component. Double-check if any previous "scroll to bottom" auto-load or pagination logic is still needed.apps/dashboard/styles/tailwind/tailwind.css (1)
5-5: LGTM!The new
--chart-selectionCSS variable is well-implemented:
- Appropriate colors chosen for both light and dark themes
- Follows existing naming conventions
- Maintains consistency with the existing theme system
Also applies to: 40-40
|
From Oguzhan Olguncu ‣ Give this a try folks |
|
the highlight is sooo subtle, I missed it and was about to ask why it’s not there we definitely need to increase contrast, right now it’s not really usable |
|
From Oguzhan Olguncu ‣ yeah didn't wanna spend time on it without discussing |
|
From Oguzhan Olguncu ‣ let's discuss on general |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/logs/components/charts/index.tsx (3)
37-42: Consider using more specific types for Selection.The Selection type could be improved by using more specific types for timestamps.
type Selection = { - start: string | number; - end: string | number; - startTimestamp?: number; - endTimestamp?: number; + start: string | number | null; + end: string | number | null; + startTimestamp?: number | null; + endTimestamp?: number | null; };
51-51: Use null for initial selection state.For better type safety and clarity, consider using null instead of empty strings for the initial selection state.
- const [selection, setSelection] = useState<Selection>({ start: "", end: "" }); + const [selection, setSelection] = useState<Selection>({ start: null, end: null });
70-79: Optimize handleMouseMove with debouncing.The handleMouseMove event handler could trigger frequently during mouse movement. Consider debouncing to improve performance.
+import { debounce } from "lodash"; + const handleMouseMove = (e: any) => { if (selection.start) { const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; setSelection((prev) => ({ ...prev, end: e.activeLabel, startTimestamp: timestamp, })); } }; + +const debouncedHandleMouseMove = debounce(handleMouseMove, 16); // ~60fps
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/(app)/logs/components/charts/index.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/app/(app)/logs/components/charts/index.tsx (1)
60-79:⚠️ Potential issueAdd proper TypeScript types for event handlers.
The mouse event handlers use
anytype which reduces type safety. Consider using proper types from therechartslibrary.Additionally, add error handling for missing payload in
handleMouseMove:const handleMouseMove = (e: any) => { if (selection.start) { + if (!e.activePayload?.[0]?.payload?.originalTimestamp) return; const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; setSelection((prev) => ({
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/logs/components/charts/index.tsx (1)
81-116: Extract filter field names to constants.The hardcoded filter field names could be moved to constants for better maintainability:
+const TIME_FILTER_FIELDS = ["startTime", "endTime", "since"] as const; + const handleMouseUp = () => { if (selection.start && selection.end) { const activeFilters = filters.filter( - (f) => !["startTime", "endTime", "since"].includes(f.field), + (f) => !TIME_FILTER_FIELDS.includes(f.field), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/logs/components/charts/index.tsx(6 hunks)apps/dashboard/styles/tailwind/tailwind.css(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/logs/components/charts/index.tsx (2)
37-42: LGTM! Well-structured type definition.The
Selectiontype clearly defines the structure for managing chart selection state with appropriate types for timestamps.
214-222: Increase highlight contrast for better visibility.Based on user feedback in the PR, the current highlight is too subtle to be noticed effectively.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/logs/components/charts/index.tsx(6 hunks)apps/dashboard/styles/tailwind/tailwind.css(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/app/(app)/logs/components/charts/index.tsx (4)
9-11: LGTM! Well-structured type definition and imports.The Selection type is well-defined with clear properties, and the imports are appropriate for the new feature.
Also applies to: 37-42
81-116: LGTM! Well-implemented filter update logic.The function correctly:
- Filters out existing time-related filters
- Ensures start time is less than end time
- Resets selection state after updating filters
214-222: 🛠️ Refactor suggestionEnhance selection visibility.
Based on user feedback, the current highlight is too subtle to be noticed effectively.
<ReferenceArea isAnimationActive x1={Math.min(Number(selection.start), Number(selection.end))} x2={Math.max(Number(selection.start), Number(selection.end))} - fill="hsl(var(--chart-selection))" + fill="hsl(var(--accent-5))" + fillOpacity={0.5} + stroke="hsl(var(--accent-7))" + strokeWidth={1} radius={[4, 4, 0, 0]} />Likely invalid or redundant comment.
60-79: 🛠️ Refactor suggestionAdd proper TypeScript types and error handling.
The mouse event handlers need proper typing and error handling:
- const handleMouseDown = (e: any) => { + const handleMouseDown = (e: CategoricalChartState) => { + if (!e.activePayload?.[0]?.payload?.originalTimestamp) return; const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; setSelection({ start: e.activeLabel, end: e.activeLabel, startTimestamp: timestamp, endTimestamp: timestamp, }); }; - const handleMouseMove = (e: any) => { + const handleMouseMove = (e: CategoricalChartState) => { if (selection.start) { + if (!e.activePayload?.[0]?.payload?.originalTimestamp) return; const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; setSelection((prev) => ({ ...prev, end: e.activeLabel, startTimestamp: timestamp, })); } };Likely invalid or redundant comment.
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit