Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR updates the tooltip label formatting logic across several chart components. The old function Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as Chart Component
participant T as ChartTooltip
participant F as Formatter (createTimeIntervalFormatter)
U->>C: Hover over chart
C->>T: Trigger tooltip rendering
T->>F: Call createTimeIntervalFormatter(data)(payload)
F-->>T: Return formatted label
T-->>C: Display tooltip with updated label
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 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: 1
🧹 Nitpick comments (6)
apps/dashboard/components/logs/overview-charts/utils.tsx (6)
12-13: Consider using more specific types instead ofanyThe function parameter
tooltipPayload: any[]uses theanytype, which bypasses TypeScript's type checking. This can lead to potential runtime errors and makes the code harder to maintain.-export function createTimeIntervalFormatter(data?: TimeseriesData[]) { - return (tooltipPayload: any[]) => { +export function createTimeIntervalFormatter(data?: TimeseriesData[]) { + return (tooltipPayload: Array<{payload?: {originalTimestamp?: number; displayX?: string}}>) => {
15-17: Improve validation to handle edge casesThe validation logic only checks if the first element's payload exists. If
tooltipPayloadis empty,tooltipPayload[0]would be undefined, leading to a potential runtime error.- if (!tooltipPayload?.[0]?.payload) { + if (!tooltipPayload?.length || !tooltipPayload[0]?.payload) { return ""; }
19-21: Add type assertions for better safetyThese assignments are extracting properties from
anytype objects. Adding type assertions would make the code more robust.- const currentPayload = tooltipPayload[0].payload; - const currentTimestamp = currentPayload.originalTimestamp; - const currentDisplayX = currentPayload.displayX; + const currentPayload = tooltipPayload[0].payload; + const currentTimestamp = currentPayload.originalTimestamp as number | undefined; + const currentDisplayX = currentPayload.displayX as string | undefined;
34-35: Consider caching timestamp indexes for better performanceUsing
findIndexinside the tooltip formatter function could be inefficient, especially if this formatter is called frequently during chart interactions. Consider pre-computing a lookup map outside this function.+export function createTimeIntervalFormatter(data?: TimeseriesData[]) { + // Pre-compute a timestamp-to-index lookup map for faster access + const timestampToIndexMap = new Map(); + data?.forEach((item, index) => { + if (item?.originalTimestamp) { + timestampToIndexMap.set(item.originalTimestamp, index); + } + }); + return (tooltipPayload: any[]) => { // ... - // Find position in the data array - const currentIndex = data.findIndex((item) => item?.originalTimestamp === currentTimestamp); + // Get position from lookup map + const currentIndex = timestampToIndexMap.get(currentTimestamp) ?? -1;
38-54: Reduce code duplication in conditional renderingThe code for rendering just the current timestamp is duplicated in two different conditions. This can be refactored to eliminate redundancy.
- // If this is the last item or not found, just show current timestamp - if (currentIndex === -1 || currentIndex >= data.length - 1) { - return ( - <div> - <span className="font-mono text-accent-9 text-xs px-4">{currentDisplayX}</span> - </div> - ); - } - - // Get the next point in the sequence - const nextPoint = data[currentIndex + 1]; - if (!nextPoint) { - return ( - <div> - <span className="font-mono text-accent-9 text-xs px-4">{currentDisplayX}</span> - </div> - ); - } + // If this is the last item, not found, or next point doesn't exist + if (currentIndex === -1 || currentIndex >= data.length - 1) { + return ( + <div> + <span className="font-mono text-accent-9 text-xs px-4">{currentDisplayX}</span> + </div> + ); + } + + // Get the next point in the sequence + const nextPoint = data[currentIndex + 1]; + if (!nextPoint) { + // This condition should rarely be hit given the previous check + return ( + <div> + <span className="font-mono text-accent-9 text-xs px-4">{currentDisplayX}</span> + </div> + ); + }
56-60: Extract common rendering logic to a helper functionThe pattern of rendering a timestamp into a tooltip appears multiple times. Consider extracting this to reduce repetition.
+ // Helper to render a single timestamp tooltip + function renderSingleTimestamp(displayText: string) { + return ( + <div> + <span className="font-mono text-accent-9 text-xs px-4">{displayText}</span> + </div> + ); + } + // Format the next timestamp const nextDisplayX = nextPoint.displayX || (nextPoint.originalTimestamp ? formatTimestampTooltip(nextPoint.originalTimestamp) : "");Then use this helper function in all the places where you're rendering a single timestamp.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/components/logs/chart/index.tsx(2 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx(3 hunks)apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx(3 hunks)apps/dashboard/components/logs/overview-charts/utils.tsx(1 hunks)apps/dashboard/components/stats-card/components/chart/stats-chart.tsx(2 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
apps/dashboard/components/stats-card/components/chart/stats-chart.tsx (1)
apps/dashboard/components/logs/overview-charts/utils.tsx (1) (1)
createTimeIntervalFormatter(12:70)
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (1)
apps/dashboard/components/logs/overview-charts/utils.tsx (1) (1)
createTimeIntervalFormatter(12:70)
apps/dashboard/components/logs/chart/index.tsx (1)
apps/dashboard/components/logs/overview-charts/utils.tsx (1) (1)
createTimeIntervalFormatter(12:70)
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
apps/dashboard/components/logs/overview-charts/utils.tsx (1) (1)
createTimeIntervalFormatter(12:70)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (2)
18-18: Imported new time interval formatter utility.The
createTimeIntervalFormatterutility has been imported from the local utils module to replace the previous timestamp tooltip formatting approach.
245-247: Enhanced tooltip to show time ranges between data points.The tooltip now displays a time range interval between consecutive data points, which provides more context than the previous single-point timestamp display. This implementation correctly passes the entire data array to the formatter for interval calculation.
The new approach appears more informative as it shows users the time span that each data bar represents, rather than just a single moment in time.
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (2)
24-24: Imported new time interval formatter utility.The
createTimeIntervalFormatterutility has been imported from the local utils module to replace the previous timestamp tooltip formatting approach.
242-244: Enhanced tooltip to show time ranges between data points.The tooltip now displays a time range interval between consecutive data points, which provides more context than the previous single-point timestamp display. This implementation correctly passes the entire data array to the formatter for interval calculation.
This change maintains consistency with the same approach used in the bar chart component, ensuring a uniform user experience across different chart types.
apps/dashboard/components/stats-card/components/chart/stats-chart.tsx (2)
3-3: Imported time interval formatter from logs components.The
createTimeIntervalFormatteris now imported from a shared location in the logs component directory, replacing the previous timestamp tooltip formatter.
100-100: Enhanced tooltip to show time ranges between data points.The tooltip now displays a time range interval between consecutive data points, providing more context than the previous single-point timestamp display. This implementation correctly passes the entire data array to the formatter for interval calculation.
This change ensures consistency across all chart components in the application, delivering a unified tooltip experience for users when interacting with time-based data.
apps/dashboard/components/logs/chart/index.tsx (3)
14-14: Imported time interval formatter from overview charts utils.The
createTimeIntervalFormatterutility has been imported from the overview charts utils module to replace the previous timestamp tooltip formatting approach.
18-18: Updated import to only include formatTimestampLabel.The import statement has been updated to only include the
formatTimestampLabelfunction, removing the previously usedformatTimestampTooltipfunction.
187-187: Enhanced tooltip to show time ranges between data points.The tooltip now displays a time range interval between consecutive data points instead of a single timestamp. This aligns with the PR objective to show timerange on chart tooltips, improving the data context provided to users.
The implementation correctly passes both the required parameters to the formatter function:
- The
dataarray necessary for finding adjacent data points- The
payloadfrom the tooltip event to identify the current pointapps/dashboard/components/logs/overview-charts/utils.tsx (1)
1-70: Good implementation of the time interval tooltip formatterOverall, this utility function is well-structured with good error handling and fallbacks. The JSDoc is clear and informative, and the code generally follows a logical flow with appropriate comments explaining each section.
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 (1)
internal/clickhouse/src/ratelimits.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
|
@chronark check and lemme know |
|
ya |
2 participants
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