fix(frontend): refactor graphql-codegen config to async function export#3283
fix(frontend): refactor graphql-codegen config to async function export#3283Shubb07 wants to merge 6 commits intoOWASP:mainfrom
Conversation
WalkthroughReplaces Node built-in imports in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/graphql-codegen.ts (1)
1-68: Use async function pattern for better compatibility with GraphQL Codegen.This PR uses top-level await in the config file, which conflicts with the established pattern in this repository. PR #2155 (August 2025) documented that GraphQL Codegen configurations should export an async function directly (
export default async function() { ... }) rather than using top-level await.While top-level await is technically viable with Node.js 24.0.0, the official GraphQL Codegen documentation recommends the async function pattern for maximum compatibility and stability. This aligns with the previous guidance from PR #2155:
export default async function(): Promise<CodegenConfig> { const response = await fetch(`${PUBLIC_API_URL}/csrf/`, { method: 'GET', }) // ... rest of fetch logic const csrfToken = (await response.json()).csrftoken return { documents: ['src/**/*.{ts,tsx}', '!src/types/__generated__/**'], // ... rest of config } }Please refactor to use the async function pattern for consistency with previous repo decisions and official tool guidance.
🤖 Fix all issues with AI agents
In @frontend/graphql-codegen.ts:
- Around line 5-15: Replace the top-level await/config export with an exported
async function: change the module to "export default async function()" and move
the CSRF fetch logic inside it (keep the existing try/catch but after the fetch
add an if (!response.ok) throw new Error(`Failed to fetch CSRF token:
${response.status} ${response.statusText}`)); then parse const csrfToken =
(await response.json()).csrftoken and return the GraphQL Codegen configuration
object using that csrfToken and the existing PUBLIC_API_URL usage so the config
is produced asynchronously for codegen.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/eslint.config.mjsfrontend/graphql-codegen.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/graphql-codegen.ts
🔇 Additional comments (2)
frontend/eslint.config.mjs (1)
1-1: LGTM! Import consolidation improves code organization.Consolidating the
pathmodule imports into a single statement is a good practice and improves readability.frontend/graphql-codegen.ts (1)
17-19: Good addition of response validation.The explicit check for
response.okwith a descriptive error message improves debugging and error visibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @frontend/graphql-codegen.ts:
- Around line 5-6: Update the PR description to state that the change exports an
async configuration function (the default exported async function) for GraphQL
Codegen instead of using an IIFE or top-level await; mention that the file now
returns a Promise<CodegenConfig> via the exported async function and that this
matches GraphQL Codegen's documented async config pattern.
🧹 Nitpick comments (1)
frontend/graphql-codegen.ts (1)
22-22: Add validation for CSRF token extraction.The code assumes the JSON response contains a
csrftokenfield without validation. If the field is missing or the response structure is unexpected, this will cause a runtime error.🛡️ Proposed fix with validation
- const csrfToken = (await response.json()).csrftoken + const data = await response.json() + const csrfToken = data?.csrftoken + + if (!csrfToken) { + throw new Error('CSRF token not found in response') + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/graphql-codegen.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1675
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:13-21
Timestamp: 2025-07-03T03:47:05.436Z
Learning: The OWASP Nest project does not use async operations anywhere in the codebase and doesn't have the required database driver for async operations. All database methods and GraphQL resolvers should remain synchronous to maintain consistency with the existing architecture.
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/graphql-codegen.ts
📚 Learning: 2025-07-03T03:47:05.436Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1675
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:13-21
Timestamp: 2025-07-03T03:47:05.436Z
Learning: The OWASP Nest project does not use async operations anywhere in the codebase and doesn't have the required database driver for async operations. All database methods and GraphQL resolvers should remain synchronous to maintain consistency with the existing architecture.
Applied to files:
frontend/graphql-codegen.ts
🔇 Additional comments (3)
frontend/graphql-codegen.ts (3)
12-16: Improved error handling.The error handling has been enhanced by throwing an error when the CSRF token fetch fails, ensuring the configuration loading fails properly rather than continuing with undefined values.
18-20: Good response validation.The HTTP response validation properly checks
response.okand provides informative error messages including status code and status text.
24-68: Configuration structure looks good.The CodegenConfig structure is properly maintained with all required fields, and the CSRF token is correctly integrated into the schema headers.
|
Thanks for the review! I’ve ensured this PR only contains the graphql-codegen refactor and updated the description to accurately reflect the async function export approach. Please let me know if any other changes are needed. |
Shubb07
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/graphql-codegen.ts (1)
5-16: Fix eslint-disable scope and preserve error context.The
/* eslint-disable no-console */at line 13 disables the rule for the rest of the file rather than just the single line, andcatch {}discards the original error without preserving context.Proposed fix
export default async function graphqlCodegenConfig(): Promise<CodegenConfig> { let response: Response try { response = await fetch(`${PUBLIC_API_URL}/csrf/`, { method: 'GET', }) - } catch { - /* eslint-disable no-console */ - console.log('Failed to fetch CSRF token: make sure the backend is running.') - throw new Error('Failed to fetch CSRF token') + } catch (err) { + // eslint-disable-next-line no-console + console.error('Failed to fetch CSRF token: make sure the backend is running.', err) + throw new Error('Failed to fetch CSRF token', { cause: err }) }
🤖 Fix all issues with AI agents
In @frontend/graphql-codegen.ts:
- Around line 18-23: The CSRF handling currently assumes (await
response.json()).csrftoken exists and may build headers with an undefined token;
update the logic around parsing the response in frontend/graphql-codegen.ts to
safely parse JSON (wrap json parsing in try/catch), validate that the parsed
object has a non-empty string property csrftoken (e.g., typeof body?.csrftoken
=== 'string' && body.csrftoken.trim() !== ''), and if validation fails throw a
clear Error including response.status/statusText and the raw body so headers are
never constructed with an undefined csrfToken; reference the existing response
and csrfToken variables when implementing this check.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/graphql-codegen.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
📚 Learning: 2025-08-30T12:52:32.214Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/graphql-codegen.ts:52-52
Timestamp: 2025-08-30T12:52:32.214Z
Learning: When using GraphQL Codegen with async configuration that needs to fetch data (like CSRF tokens), export the async function directly instead of using top-level await: `export default async function()` rather than `export default await function()`.
Applied to files:
frontend/graphql-codegen.ts
📚 Learning: 2025-07-03T03:47:05.436Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1675
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:13-21
Timestamp: 2025-07-03T03:47:05.436Z
Learning: The OWASP Nest project does not use async operations anywhere in the codebase and doesn't have the required database driver for async operations. All database methods and GraphQL resolvers should remain synchronous to maintain consistency with the existing architecture.
Applied to files:
frontend/graphql-codegen.ts
|
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
Please run make check-test and ensure all tests and checks pass.
Also, the codegen configuration is invalid and does not work.
✖ Invalid Codegen Configuration!
Please make sure that your codegen config file contains the "generates" field, with a specification for the plugins you need.
It should looks like that:
schema:
- my-schema.graphql
generates:
my-file.ts:
- plugin1
- plugin2
- plugin3
◼ Generate outputs
ELIFECYCLE Command failed with exit code 1.| preset: 'near-operation-file', | ||
| presetConfig: { | ||
| // This should be the file generated by the "typescript" plugin above, | ||
| // relative to the directory specified for this configuration |
There was a problem hiding this comment.
please do not remove these comments.
There was a problem hiding this comment.
Thanks for pointing this out.
You’re right — my current changes broke the codegen configuration and I did not run make check-test successfully before pushing.
I’ll reset this PR to only address the async IIFE refactor, restore the comments, and make sure make check-test passes before updating.
There was a problem hiding this comment.
okay, can you also restore the original pull request description format and fill in the details?
There was a problem hiding this comment.
Hey, thanks for letting me know .
I’ve restored the original OWASP PR template format and properly filled in the details, including the issue reference, change description, and checklist.
Let me know if there’s anything else you’d like me to update.
|
Hi, I’m currently blocked by serious local environment issues (Docker/WSL setup) and I’m unable to properly run and validate the required checks for this PR. Since this may take time to resolve, I don’t want to block progress here. I’m going to close this PR so the issue can be reassigned and worked on by someone else. Sorry about this, and thanks for your time. |
2 participants
##Proposed change
Resolves #3087
This PR refactors the GraphQL codegen configuration to remove the async IIFE pattern and export an async configuration function directly, aligning the implementation with the intended approach described in the issue.
It also improves error handling around CSRF token fetching by:
Preserving the original error context
Safely parsing the JSON response
Validating the presence of a non-empty csrftoken before constructing headers
This ensures the codegen configuration fails clearly and safely if the backend is unavailable or returns an unexpected response.
Checklist