fix(Web): separate-batchers #1867
Conversation
WalkthroughThe pull request modifies the GraphQL batching logic in the Changes
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 (
|
✅ 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-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🔭 Outside diff range comments (1)
web/src/context/GraphqlBatcher.tsx (1)
Line range hint
25-39: Improve error handling and type safety in coreExecutor.The current implementation has several areas for improvement:
- The
@ts-ignorecomment suggests underlying type issues that should be addressed properly.- Returning an empty object on error could silently mask issues.
- The error message lacks specific details about what went wrong.
Consider this improved implementation:
const coreExecutor: AsyncExecutor = async ({ document, variables }) => { try { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore const result = request(getGraphqlUrl(false), document, variables).then((res) => ({ data: res, })) as Promise<ExecutionResult>; return result; } catch (error) { - console.error("Graph error: ", { error }); - debounceErrorToast("Graph query error: failed to fetch data."); - return { data: {} }; + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + console.error("Core GraphQL error:", { error, document, variables }); + debounceErrorToast(`Core GraphQL query failed: ${errorMessage}`); + throw error; // Re-throw to allow proper error handling upstream } };
🧹 Nitpick comments (1)
web/src/context/GraphqlBatcher.tsx (1)
57-71: Consider adding error boundaries and improving type safety.The batch execution setup looks good, but could benefit from additional error handling and type safety improvements.
Consider these enhancements:
const coreBatchExec = createBatchingExecutor(coreExecutor); const dtrBatchExec = createBatchingExecutor(dtrExecutor); -const fetcher = async (queries: IQuery[]) => { +const fetcher = async (queries: readonly IQuery[]): Promise<Array<{ id: string; result: unknown }>> => { const batchdata = await Promise.all( queries.map(({ document, variables, isDisputeTemplate }) => - isDisputeTemplate - ? dtrBatchExec({ - document, - variables, - }) - : coreBatchExec({ - document, - variables, - }) + (isDisputeTemplate ? dtrBatchExec : coreBatchExec)({ + document, + variables, + }).catch(error => ({ + data: null, + errors: [{ message: error.message }] + })) ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/context/GraphqlBatcher.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (1)
web/src/context/GraphqlBatcher.tsx (1)
29-29: Verify URL security and CORS configuration.The separation of URLs for core and DTR operations requires careful security consideration.
Please verify:
- The URL configurations in both environments
- CORS settings for both endpoints
- Any authentication differences between endpoints
Also applies to: 45-45
✅ 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: 1
🔭 Outside diff range comments (1)
web/src/context/GraphqlBatcher.tsx (1)
Line range hint
25-39: Improve error handling and type safety in the fetch function.The current implementation has several issues:
- Silently returning empty data on error can mask problems and make debugging difficult
- Missing type annotations for parameters
- Using @ts-ignore is not recommended
Consider this improved implementation:
-const fetch = async (url, document, variables) => { +const fetch = async ( + url: string, + document: TypedDocumentNode<any, any>, + variables: Record<string, any> +): Promise<ExecutionResult> => { try { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - //@ts-ignore const result = request(url, document, variables).then((res) => ({ data: res, })) as Promise<ExecutionResult>; return result; } catch (error) { console.error("Graph error: ", { error }); debounceErrorToast("Graph query error: failed to fetch data."); - return { data: {} }; + throw error; // Propagate the error instead of silently returning empty data } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/context/GraphqlBatcher.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
web/src/context/GraphqlBatcher.tsx (2)
41-47: Reduce code duplication between executors.The executors are nearly identical, differing only in the URL parameter. This violates the DRY principle.
49-50: LGTM! Clean separation of batch executors.The separation of core and DTR batch executors is well-implemented and provides a clear distinction between different types of queries.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
|
Code Climate has analyzed commit 07137fd and detected 60 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
PR-Codex overview
This PR refactors the
executorfunction inGraphqlBatcher.tsxto introduce separate batching executors for core and dispute template GraphQL requests, enhancing code clarity and modularity.Detailed summary
executorfunction with afetchfunction that acceptsurl,document, andvariables.coreExecutoranddtrExecutorfor handling core and dispute template requests, respectively.coreBatchExecanddtrBatchExecusing the new executors.fetcherfunction to use the new batching executors based on theisDisputeTemplateflag.Summary by CodeRabbit