Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@ogzhanolguncu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change introduces a centralized query time management system across several dashboard components by implementing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LogsRefresh
participant QueryTimeProvider
participant TRPCQuery
User->>LogsRefresh: Click Refresh
LogsRefresh->>QueryTimeProvider: refreshQueryTime()
QueryTimeProvider-->>LogsRefresh: Updated timestamp
LogsRefresh->>TRPCQuery: Invalidate queries (using new timestamp)
sequenceDiagram
participant Component
participant QueryTimeProvider
Component->>QueryTimeProvider: useQueryTime()
QueryTimeProvider-->>Component: Provide { queryTime, refreshQueryTime }
Component->>QueryTimeProvider: (on refresh) refreshQueryTime()
QueryTimeProvider-->>Component: Notify with updated queryTime
Suggested labels
Suggested reviewers
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. 🪧 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! 🙏 |
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
I like this
couple of questions:
- should this be a provider instead? maybe that's more ergonomic? idk
- can we have an api like this? Is this cleaner? idk
const { queryTime, refreshQueryTime } = useQueryTime()
// alternatively if we don't tend to destructure it
const {now, refresh} = useQueryTime()
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/utils.ts
Outdated
Show resolved
Hide resolved
|
Okay. Yeah If we are on the same page Provider would be better coz every other RQ will implement this. I'll do it like you proposed. I like it :chef_kissing_fingers_intensifies: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts (1)
28-29: Consider updating or removing this comment.This comment doesn't provide useful information anymore, as the primary purpose of the refactoring is to prevent double TRPC calls by using a consistent timestamp. Consider updating it to explain the relationship with the shared timestamp or removing it if no longer relevant.
- //Required for preventing double trpc call during initial render +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/dashboard/app/(app)/layout.tsx(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/hooks/use-fetch-timeseries.ts(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts(3 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/hooks/use-fetch-timeseries.ts(2 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-refresh.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts(4 hunks)apps/dashboard/providers/query-time-provider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-refresh.tsx
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/hooks/use-logs-query.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/dashboard/app/(app)/layout.tsx (1)
apps/dashboard/providers/query-time-provider.tsx (1)
QueryTimeProvider(21-38)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts (4)
apps/dashboard/providers/query-time-provider.tsx (1)
useQueryTime(40-46)apps/dashboard/components/logs/hooks/use-sort.tsx (1)
useSort(16-60)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/query-logs.schema.ts (2)
SortFields(5-5)RatelimitQueryOverviewLogsPayload(45-45)apps/dashboard/components/logs/constants.ts (1)
HISTORICAL_DATA_WINDOW(2-2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/hooks/use-fetch-timeseries.ts (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/hooks/use-filters.ts (1)
useFilters(26-149)apps/dashboard/providers/query-time-provider.tsx (1)
useQueryTime(40-46)apps/dashboard/components/logs/constants.ts (1)
TIMESERIES_DATA_WINDOW(3-3)
apps/dashboard/providers/query-time-provider.tsx (2)
apps/dashboard/lib/trpc/context.ts (1)
createContext(7-31)go/pkg/system_errors/errors.go (1)
Error(36-40)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
apps/dashboard/providers/query-time-provider.tsx (4)
5-6: Good implementation of a global query timestamp stateThe approach of using module-level variables for the timestamp and subscribers list is appropriate for maintaining a single source of truth across the entire application.
8-12: Clean implementation of the refresh mechanismThis function elegantly handles both updating the timestamp and notifying all subscribers in a predictable manner.
21-38: Well-structured context provider componentThe QueryTimeProvider correctly subscribes to timestamp updates and handles cleanup to prevent memory leaks. The subscription mechanism ensures that all instances of the provider stay in sync with the global timestamp.
40-46: Good error handling in the hookThe hook appropriately throws an error if used outside of a provider, which will help catch misuse during development.
apps/dashboard/app/(app)/layout.tsx (2)
9-9: Correctly imported the QueryTimeProviderThe import path is accurate and follows the project's import conventions.
53-53: Well-placed provider in the component hierarchyThe QueryTimeProvider is appropriately wrapped around children only when the workspace is enabled, ensuring the context is available to all components that need it.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/bar-chart/hooks/use-fetch-timeseries.ts (4)
4-4: Correctly imported the useQueryTime hookThe import path is accurate and follows the project's import conventions.
11-11: Good use of destructuring with renamingDestructuring with renaming (queryTime: timestamp) improves code readability by making it clear what this value represents in the context of this hook.
16-17: Successfully replaced local timestamp with shared timestampThe query parameters now use the shared timestamp, which will ensure consistency across different components and prevent unnecessary refetches during navigation.
56-56: Correctly updated useMemo dependenciesThe dependency array now includes 'timestamp' instead of a local timestamp variable, ensuring that the query params are recalculated when the shared timestamp changes.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/hooks/use-fetch-timeseries.ts (4)
4-4: Correctly imported the useQueryTime hookThe import path is accurate and follows the project's import conventions.
11-11: Good use of destructuring with renamingDestructuring with renaming (queryTime: timestamp) improves code readability by making it clear what this value represents in the context of this hook.
16-17: Successfully replaced local timestamp with shared timestampThe query parameters now use the shared timestamp, which will ensure consistency across different components and prevent unnecessary refetches during navigation.
56-56: Correctly updated useMemo dependenciesThe dependency array now includes 'timestamp' instead of a local timestamp variable, ensuring that the query params are recalculated when the shared timestamp changes.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/hooks/use-logs-query.ts (4)
3-3: Imported shared timestamp provider to enable consistent timestamps.This import enables the hook to use the global timestamp from the QueryTimeProvider, supporting the goal of maintaining a consistent timestamp during page navigation.
22-22: Good usage of the shared timestamp mechanism.Replacing the local timestamp with the shared
queryTimefrom the provider ensures that all components use the same timestamp reference point, preventing unnecessary refetches during page navigation.
33-34: Correctly updated time window calculation to use shared timestamp.Using the shared timestamp for calculating the query time window (
startTimeandendTime) ensures consistency across components and during refreshes.
89-89: Properly updated dependency array to include the new timestamp.Including
timestampin the dependency array ensures that the query parameters are properly recalculated when the timestamp is refreshed, supporting the manual refresh functionality.
|
Now we can navigate back and forth between log pages (basically any page that contains RQ with |
|
@chronark you gonna love this. I think this should be our first step before all those RQ optimizations. It's minimal effort with lots of gains |
|
clean :chef_kissing_fingers_intensifies: I’d love some explanations as inline docs in the provider file, on why we need this, cause right now if you have no context ,it loosk like we’re too stupid to use Date.now() and overarchitected it |
|
yeaaahh. hahahhhah 😄 i'll add it in 5 min |
2 participants
What does this PR do?
Before: Every page navigation created a new timestamp, which triggered unnecessary refetches
Before: Manual refreshes weren't working because we kept using the same old timestamp
Our solution:
Created a shared timestamp that doesn't change on navigation
Added a way to explicitly update this timestamp when refreshing
Made components react to timestamp changes automatically
Now page navigation doesn't trigger refetches.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor