Refactor/graphql batching and optimisations #1850
Conversation
WalkthroughThis pull request introduces two new GraphQL-related dependencies to the project's Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphqlBatcher
participant BatchExecutor
participant GraphQLServer
Client->>GraphqlBatcher: Initiate Multiple Queries
GraphqlBatcher->>BatchExecutor: Batch Queries
BatchExecutor->>GraphQLServer: Send Batched Request
GraphQLServer-->>BatchExecutor: Return Batched Results
BatchExecutor-->>GraphqlBatcher: Processed Results
GraphqlBatcher-->>Client: Deliver Query Results
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
web/src/hooks/queries/useAllCasesQuery.ts (1)
5-5: Consider enhancing the query configuration.While adding
staleTimeis good for optimization, consider these improvements:
- Make the
queryKeymore specific by including version/type information- Add error boundaries to handle failed queries gracefully
return useQuery({ - queryKey: [`allCasesQuery`], + queryKey: ['cases', 'all', 'v1'], staleTime: STALE_TIME, queryFn: async () => await graphqlBatcher.fetch({ id: crypto.randomUUID(), document: allCasesQuery, variables: {} }), + retry: 2, + onError: (error) => { + console.error('Failed to fetch cases:', error); + }, });Also applies to: 24-24
web/src/hooks/queries/useCourtDetails.ts (1)
3-3: Consider implementing field-level caching for court details.While adding
staleTimehelps with query-level caching, the court details query fetches multiple fields that might have different freshness requirements. Consider:
- Implementing field-level caching for slowly-changing data like
policy,minStake,alpha- Using shorter stale times only for frequently-changing fields like
numberDisputes,numberVotesThis can be achieved using type policies in Apollo Client's cache. Would you like me to provide an example implementation?
Also applies to: 39-39
web/src/hooks/queries/useDisputeMaintenanceQuery.ts (1)
5-5: Enhance type safety and caching strategy for dispute maintenance.The current implementation has room for improvement:
- The ID conversion to string could be more type-safe
- The complex nested data structure might benefit from a more granular caching strategy
return useQuery<DisputeMaintenanceQuery>({ - queryKey: [`disputeMaintenanceQuery-${id}`], + queryKey: ['dispute', 'maintenance', id], enabled: isEnabled, staleTime: STALE_TIME, queryFn: async () => { + const disputeId = id?.toString() ?? ''; + if (!disputeId) throw new Error('Invalid dispute ID'); await graphqlBatcher.fetch({ id: crypto.randomUUID(), document: disputeMaintenance, - variables: { disputeId: id?.toString(), disputeIdAsString: id?.toString() }, + variables: { disputeId, disputeIdAsString: disputeId }, }), } });Also applies to: 44-44
web/src/hooks/queries/useCourtTree.ts (2)
5-5: Fix inconsistent import path.The import path
src/constsdiffers from other files that useconsts/index. Maintain consistency across the codebase.-import { STALE_TIME } from "src/consts"; +import { STALE_TIME } from "consts/index";
43-43: Consider longer stale time for court tree data.Since court hierarchy rarely changes, consider using a longer
staleTimefor this query to reduce unnecessary refetches.web/src/hooks/queries/useVotingHistory.ts (1)
62-62: Consider dynamic stale time based on voting period.For voting history, a static
staleTimemight not be optimal. Consider adjusting the stale time dynamically based on:
- Active voting period (shorter stale time)
- Completed disputes (longer stale time)
Example approach:
const getStaleTime = (dispute?: VotingHistoryQuery['dispute']) => { if (!dispute) return STALE_TIME; return dispute.ruled ? STALE_TIME * 10 : STALE_TIME; };web/src/hooks/queries/useClassicAppealQuery.ts (1)
3-3: Consider a more flexible stale time configuration.The current implementation uses a single
STALE_TIMEconstant across different types of queries. Consider:
- Creating different stale time constants for different data types (e.g.,
COURT_STALE_TIME,VOTING_STALE_TIME)- Implementing a configuration system that allows fine-tuning stale times based on data type and context
Example approach:
// src/consts/staleTime.ts export const STALE_TIMES = { COURT_TREE: 1000 * 60 * 60, // 1 hour VOTING_HISTORY: 1000 * 30, // 30 seconds DISPUTE_DETAILS: 1000 * 60, // 1 minute APPEAL: 1000 * 15, // 15 seconds } as const;web/src/consts/index.ts (1)
10-10: Consider increasing STALE_TIME and adding documentation.A stale time of 1 second might be too aggressive and could lead to unnecessary refetches. Consider:
- Increasing the value based on your data freshness requirements
- Adding documentation explaining the chosen value
+// Duration (in ms) for which the query data is considered fresh. +// Set to 1 second to balance data freshness with performance. export const STALE_TIME = 1000;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
web/package.json(1 hunks)web/src/consts/index.ts(2 hunks)web/src/context/GraphqlBatcher.tsx(2 hunks)web/src/hooks/queries/useAllCasesQuery.ts(2 hunks)web/src/hooks/queries/useClassicAppealQuery.ts(2 hunks)web/src/hooks/queries/useCourtDetails.ts(2 hunks)web/src/hooks/queries/useCourtTree.ts(2 hunks)web/src/hooks/queries/useDisputeDetailsQuery.ts(2 hunks)web/src/hooks/queries/useDisputeMaintenanceQuery.ts(2 hunks)web/src/hooks/queries/useJurorStakeDetailsQuery.ts(2 hunks)web/src/hooks/queries/useUser.ts(2 hunks)web/src/hooks/queries/useVotingHistory.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: dependency-review
🔇 Additional comments (6)
web/src/hooks/queries/useJurorStakeDetailsQuery.ts (1)
3-3: Verify the relationship betweenstaleTimeandrefetchInterval.The query uses both
staleTimeandrefetchInterval. This combination might lead to unnecessary refetches ifrefetchIntervalis shorter thanstaleTime. Consider:
- Ensuring
REFETCH_INTERVAL > STALE_TIMEto prevent wasted requests- Documenting why both intervals are needed
Also applies to: 32-32
✅ Verification successful
The timing configuration is correct and well-designed
The relationship between the constants is properly configured:
STALE_TIME(1s) <REFETCH_INTERVAL(5s)- This allows for efficient caching while maintaining data freshness:
- Data is considered stale after 1 second, enabling manual refreshes when needed
- Automatic background refetches occur every 5 seconds to minimize unnecessary requests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the values of both constants echo "Checking timing constants..." rg "export const (REFETCH_INTERVAL|STALE_TIME)" "src/consts"Length of output: 178
Script:
#!/bin/bash # Check the values of both constants and their usage echo "Checking timing constants in web/src..." rg "export const (REFETCH_INTERVAL|STALE_TIME)" "web/src/consts" echo -e "\nChecking usage patterns..." rg "(REFETCH_INTERVAL|STALE_TIME)" "web/src" -A 2 -B 2Length of output: 17753
web/src/hooks/queries/useDisputeDetailsQuery.ts (1)
3-3: Consider the relationship between STALE_TIME and REFETCH_INTERVAL.The combination of
staleTimeandrefetchIntervalneeds careful consideration for dispute details:
- If
STALE_TIMEis greater thanREFETCH_INTERVAL, the refetch might not trigger as expected- For dispute details that require fresh data (e.g., ruling status), consider if caching for
STALE_TIMEis appropriateLet's verify the constant values and their relationship:
Also applies to: 51-51
✅ Verification successful
The STALE_TIME and REFETCH_INTERVAL configuration is appropriate ✅
The current setup with
STALE_TIME(1s) being less thanREFETCH_INTERVAL(5s) is correct. This configuration ensures that:
- Data is considered stale quickly (after 1s)
- Regular refetching occurs every 5s
- React Query's background refetching works as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the values of STALE_TIME and REFETCH_INTERVAL echo "Checking constant definitions..." rg "export const (STALE_TIME|REFETCH_INTERVAL)" src/consts/Length of output: 188
Script:
#!/bin/bash # First, find potential const files echo "Finding potential const files..." fd index.ts -t f web/src/consts # Then search for the constants with more flexible pattern echo -e "\nSearching for constant definitions..." rg "export const (STALE_TIME|REFETCH_INTERVAL)" web/srcLength of output: 472
web/src/hooks/queries/useClassicAppealQuery.ts (1)
47-47: Review caching strategy for time-sensitive appeal data.Appeal-related data (especially fee information) is time-sensitive. The current implementation might cache this data for too long during critical periods.
Consider:
- Does the appeal period status affect how long we should cache?
- Are there any race conditions with fee updates?
Let's verify the appeal period handling:
web/src/context/GraphqlBatcher.tsx (1)
3-4: LGTM! New dependencies for improved GraphQL batching.The addition of
@graphql-toolspackages enables better batching capabilities.web/src/hooks/queries/useUser.ts (1)
62-62: LGTM! Consistent stale time implementation.The addition of STALE_TIME to the useQuery options aligns with the PR's optimization goals.
web/package.json (1)
81-82: LGTM! Dependencies align with batching implementation.The added GraphQL tools dependencies support the new batching functionality.
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
web/src/context/GraphqlBatcher.tsx (3)
27-31: Improve type safety by removing @ts-ignore.The @ts-ignore comment suggests potential type mismatches. Consider adding proper type definitions for the
requestfunction's return type.- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore - const result = request(extensions.url, document, variables).then((res) => ({ - data: res, - })) as Promise<ExecutionResult>; + const result = request<ExecutionResult['data']>( + extensions.url, + document, + variables + ).then((res) => ({ + data: res, + }));
35-35: Replace console.error with proper logging.Consider using a proper logging service for better error tracking and monitoring in production.
- console.error("Graph error: ", { error }); + // TODO: Replace with proper logging service + logger.error("GraphQL request failed", { + error, + document, + variables + });
44-53: Improve type safety in data processing.The @ts-ignore comment suggests potential type mismatches in the data processing logic.
const batchdata = await Promise.all( queries.map(({ document, variables, isDisputeTemplate, chainId }) => myBatchExec({ document, variables, extensions: { url: getGraphqlUrl(isDisputeTemplate ?? false, chainId) } }) ) ); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore - const processedData = batchdata.map((data, index) => ({ id: queries[index].id, result: data.data })); + const processedData = batchdata.map((data, index) => ({ + id: queries[index].id, + result: (data as ExecutionResult).data + })); return processedData;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/context/GraphqlBatcher.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/src/context/GraphqlBatcher.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1850
File: web/src/context/GraphqlBatcher.tsx:25-41
Timestamp: 2025-01-27T11:25:08.246Z
Learning: In GraphQL batch operations using Promise.all, individual request errors should be caught and handled (e.g., returning {data: {}}) rather than thrown, to prevent a single failure from causing the entire batch to fail.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: dependency-review
🔇 Additional comments (2)
web/src/context/GraphqlBatcher.tsx (2)
3-4: LGTM! Imports align with batching objectives.The new imports from @graphql-tools provide the necessary functionality for implementing GraphQL request batching.
41-41: LGTM! Batch executor creation is correct.The batching executor is properly created using the executor function.
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/src/context/GraphqlBatcher.tsx (2)
25-39: Improve type safety and logging in the executor.While the error handling approach is correct for batch operations, there are a few improvements to consider:
- The @ts-ignore comment should be addressed
- Console.error should use structured logging
Consider this implementation:
const executor: AsyncExecutor = async ({ document, variables, extensions }) => { try { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore + const { url } = extensions as { url: string }; const result = request(extensions.url, document, variables).then((res) => ({ data: res, })) as Promise<ExecutionResult>; return result; } catch (error) { - console.error("Graph error: ", { error }); + // TODO: Replace with structured logging + const errorDetails = error instanceof Error ? { message: error.message, stack: error.stack } : { error }; + console.error("Graph error: ", errorDetails); debounceErrorToast("Graph query error: failed to fetch data."); return { data: {} }; } };
44-53: Improve type safety in the fetcher implementation.The implementation correctly uses Promise.all for parallel execution, but the type safety can be improved.
Consider this implementation:
const batchdata = await Promise.all( queries.map(({ document, variables, isDisputeTemplate, chainId }) => batchExec({ document, variables, extensions: { url: getGraphqlUrl(isDisputeTemplate ?? false, chainId) } }) ) ); - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore - const processedData = batchdata.map((data, index) => ({ id: queries[index].id, result: data.data })); + const processedData = batchdata.map((data, index) => ({ + id: queries[index].id, + result: (data as ExecutionResult).data + })); return processedData;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/context/GraphqlBatcher.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/src/context/GraphqlBatcher.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1850
File: web/src/context/GraphqlBatcher.tsx:25-41
Timestamp: 2025-01-27T11:25:08.246Z
Learning: In GraphQL batch operations using Promise.all, individual request errors should be caught and handled (e.g., returning {data: {}}) rather than thrown, to prevent a single failure from causing the entire batch to fail.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: contracts-testing
- GitHub Check: dependency-review
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
web/src/context/GraphqlBatcher.tsx (2)
3-4: LGTM! Necessary imports added for GraphQL batching.The new imports from @graphql-tools provide the required functionality for batch execution and type definitions.
41-41: LGTM! Batch executor creation is correct.The batching executor is properly created using the custom executor implementation.
|
Code Climate has analyzed commit 11c8057 and detected 5 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
PR-Codex overview
This PR introduces the
@graphql-tools/batch-executeand@graphql-tools/utilsdependencies and enhances several query hooks by addingstaleTimefor cache management. It also updates theGraphqlBatcherto use a batching executor for improved performance.Detailed summary
@graphql-tools/batch-executeand@graphql-tools/utilstopackage.json.STALE_TIMEconstant inweb/src/consts/index.ts.useClassicAppealQuery,useAllCasesQuery, etc.) to includestaleTime.GraphqlBatcherwith a new executor for batch processing of GraphQL requests.Summary by CodeRabbit
Dependencies
@graphql-tools/batch-executeand@graphql-tools/utilsto project dependenciesPerformance Improvements
STALE_TIMEconstant to optimize query caching across multiple hooksQuery Management