fix: resolve CORS issues via reverse proxy configuration (Vite, Nginx, Apache) #4880
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
No significant changes currently retry |
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
Contributor
WalkthroughAdds webserver proxying for GraphQL (HTTP + WebSocket), introduces an Apache VirtualHost to serve the React app and proxy /graphql to Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Web as Webserver (Apache / Nginx)
participant API as Backend API (api:4000)
Browser->>Web: GET / (load SPA)
Web-->>Browser: 200 + index.html / static assets
Browser->>Web: POST /graphql (HTTP)
Web->>API: POST /graphql (proxy to http://api:4000/graphql)
API-->>Web: 200 JSON
Web-->>Browser: 200 JSON
Note over Browser,API: Subscriptions / real-time via WebSocket
Browser->>Web: Upgrade /graphql (ws/wss)
Web->>API: Forward upgrade to ws://api:4000 (Rewrite/Proxy)
API-->>Browser: WebSocket established
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
CONTRIBUTING.mdis excluded by!*.mddocs/docs/docs/getting-started/configuration.mdis excluded by!**/docs/docs/**
📒 Files selected for processing (4)
.env.example(2 hunks)config/docker/setup/apache.conf(1 hunks)config/docker/setup/nginx.conf(1 hunks)config/vite.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
📚 Learning: 2025-11-13T18:07:48.621Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-13T18:07:48.621Z
Learning: In talawa-admin, Apollo Client pinned to 3.11.10 (not 3.14.0) to avoid deprecation warnings during test isolation work (PR #4565 Phase 2A-2B). Apollo 3.14.0's MockedProvider internally uses deprecated options causing console noise that interferes with debugging. Upgrade to 4.x blocked by dependency conflicts with apollo-upload-client and apollo/link-error. All addTypename props removed from 115+ test files for future compatibility. Will upgrade when dependencies support Apollo 4.x.
Applied to files:
.env.example
📚 Learning: 2025-07-11T20:39:54.613Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Applied to files:
.env.example
📚 Learning: 2024-10-25T19:27:07.624Z
Learnt from: meetulr
Repo: PalisadoesFoundation/talawa-admin PR: 2355
File: src/GraphQl/Queries/userTagQueries.ts:111-115
Timestamp: 2024-10-25T19:27:07.624Z
Learning: In the Talawa Admin project, when constructing GraphQL queries (e.g., in `src/GraphQl/Queries/userTagQueries.ts`), only query the required user fields necessary for functionality to minimize data transfer and maintain efficiency.
Applied to files:
.env.example
📚 Learning: 2025-08-11T07:33:55.926Z
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 4073
File: src/GraphQl/Queries/OrganizationQueries.ts:324-343
Timestamp: 2025-08-11T07:33:55.926Z
Learning: In the Talawa Admin project, the capacity and file/imageUrl fields were temporarily removed from the VENUE_LIST query (venuesByOrganization) in src/GraphQl/Queries/OrganizationQueries.ts because the backend API doesn't currently support these fields. These fields are planned to be added back once the API is updated to support them. The UI code may still reference these fields, creating a temporary mismatch.
Applied to files:
.env.example
📚 Learning: 2025-11-24T08:30:07.940Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-24T08:30:07.940Z
Learning: In talawa-admin PR #4743, Apollo Client is pinned to 3.13.0 (stable version) in package.json resolutions and pnpm overrides. The addTypename deprecation warnings from Apollo Client will be addressed in a future upgrade to Apollo 4.x once it becomes stable and dependency conflicts with apollo-upload-client and apollo/link-error are resolved. All addTypename props have already been removed from 115+ test files in preparation for this future upgrade.
Applied to files:
.env.example
📚 Learning: 2025-02-02T10:18:37.840Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Applied to files:
.env.exampleconfig/vite.config.tsconfig/docker/setup/apache.confconfig/docker/setup/nginx.conf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Generate and Validate Documentation
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (5)
config/docker/setup/apache.conf (2)
1-3: Clarify DocumentRoot path naming convention.The DocumentRoot is set to
/usr/share/nginx/htmlin an Apache configuration. While this may be intentional (e.g., shared Docker volume), the "nginx" reference in an Apache config could cause confusion.Verify:
- Is this path shared between nginx and Apache containers?
- Consider using a more neutral path like
/usr/share/talawa/htmlor/var/www/htmlfor clarity in Apache-specific deployments.
6-7: I'll help you verify the review comment about the Apache proxy configuration. Let me start by examining the relevant files and configuration context.
<function_calls>
#!/bin/bashFirst, let's examine the apache.conf file to understand the full context
cat -n config/docker/setup/apache.conf
</function_calls>Now let me search for the nginx configuration to compare:
<function_calls>
#!/bin/bashFind nginx configuration files
find . -type f -name "*.conf" | grep -i nginx | head -20
</function_calls>Let me also check the API configuration to understand the endpoint structure:
<function_calls>
#!/bin/bashSearch for API configuration and GraphQL endpoint definitions
rg -n "4000|graphql" --type=conf -i -C 3 | head -50
</function_calls>Let me look for Docker Compose or other service configuration:
<function_calls>
#!/bin/bashLook for docker-compose or service configuration
find . -type f ( -name "docker-compose*" -o -name "docker.yml" -o -name "docker.yaml" ) | head -10
</function_calls>config/docker/setup/nginx.conf (1)
12-25: I encountered a technical issue accessing the repository directly. However, I can work with the context provided and the learnings from your previous work on this project.From the learnings retrieved, I have relevant context about the Talawa Admin project:
Key Learning (PR 3524): In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
This is directly relevant to the verification needed. Let me generate the rewritten review comment based on this context and the analysis of the nginx configuration change:
Verify Docker service name "api" exists in docker-compose configuration.
The updated
proxy_pass http://api:4000correctly preserves the/graphqlpath in proxied requests (nginx appends the request URI when proxy_pass has no path component). The WebSocket upgrade headers (lines 17-19) are properly configured.However, verify the following:
Docker service name: Ensure a Docker service named
apiis defined in your docker-compose.yml. If the service has a different name, DNS resolution will fail at runtime.CORS handling: Based on the project's established configuration patterns (as documented in nginx.prod.conf), CORS headers for the GraphQL endpoint are intentionally managed. Confirm that:
- The backend API continues to handle CORS headers directly, OR
- CORS configuration is maintained elsewhere in the nginx setup (e.g., in a separate nginx.prod.conf or a conditional block)
.env.example (2)
27-27: Based on my web search, I've found critical information about how Apollo Client handles WebSocket URLs.Key Finding: According to Apollo's GraphQLWsLink documentation and the graphql-ws library, the
createClient()function requires a full WebSocket URL with explicitws://orwss://scheme. While the browser's native WebSocket API can accept relative paths (like/graphql) and auto-detect the protocol, Apollo GraphQLWsLink does not automatically convert relative paths to full WebSocket URLs.The standard recommended pattern is:
const proto = window.location.protocol === 'https:' ? 'wss' : 'ws'; const url = `${proto}://${window.location.host}/graphql`; const wsLink = new GraphQLWsLink(createClient({ url }));This means the original review comment's concern is justified. The change from
ws://localhost:4000/graphqlto/graphqlis problematic unless:
- The codebase has wrapper code that explicitly constructs the full URL with protocol detection before passing it to Apollo's
createClient()- The app is using a different WebSocket library (not Apollo GraphQLWsLink)
- The change is intentional for a runtime-injection configuration pattern
Since I cannot access the repository to verify how
REACT_APP_BACKEND_WEBSOCKET_URLis actually used in the Apollo setup, I cannot definitively confirm whether this change will work correctly.
The review comment's concern is valid and still requires verification of the actual Apollo Client initialization code in the repository.
11-11: The environment variable change from absolute to relative URL is standard practice, not a breaking change.Based on Apollo Client and Create React App documentation, using a relative path (
/graphql) is the recommended approach for same-origin GraphQL endpoints. This typically uses CRA's built-in proxy configuration during development, which forwards/graphqlrequests to the backend (e.g.,http://localhost:4000/graphql).The original review's concern about "breaking change" overstates the issue:
- Relative URLs are fully supported by Apollo Client and are best practice
- The change assumes the application's proxy configuration already handles this routing (standard setup)
- Environment file updates are configuration changes, not code breaking changes
- Existing installations update their
.envfiles through standard onboarding/deployment processesIf concerns exist about this specific implementation, verify the actual Apollo Client initialization and proxy configuration in the codebase directly rather than assuming relative URLs will fail.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4880 +/- ##
===========================================
+ Coverage 92.77% 94.95% +2.17%
===========================================
Files 134 388 +254
Lines 3238 15056 +11818
Branches 904 4449 +3545
===========================================
+ Hits 3004 14296 +11292
- Misses 225 731 +506
- Partials 9 29 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
config/docker/setup/apache.conf(1 hunks)config/vite.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
📚 Learning: 2025-02-02T10:18:37.840Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Applied to files:
config/vite.config.tsconfig/docker/setup/apache.conf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Generate and Validate Documentation
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
config/docker/setup/apache.conf (1)
1-21: Apache VirtualHost, GraphQL proxy, and SPA routing look solid; double‑check the document root path.The VirtualHost correctly:
- Proxies
/graphqlHTTP traffic tohttp://api:4000/graphql.- Handles WebSocket upgrades for
/graphqlvia the fixed^/graphql(.*)$rule.- Serves the React SPA from
/usr/share/nginx/htmlwithFallbackResource /index.htmland tightened options (FollowSymLinks,AllowOverride None).Only ask: verify that your Docker image actually builds/mounts the frontend assets into
/usr/share/nginx/htmlfor Apache, since that path is more typical of nginx images than stock httpd. If the assets land elsewhere, alignDocumentRootand the<Directory>path accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
config/vite.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
📚 Learning: 2025-02-02T10:18:37.840Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-11-01T16:20:31.293Z
Learnt from: GlenDsza
Repo: PalisadoesFoundation/talawa-admin PR: 2397
File: src/screens/UserPortal/Volunteer/UpcomingEvents/UpcomingEvents.tsx:94-115
Timestamp: 2024-11-01T16:20:31.293Z
Learning: In the TypeScript files of this project, avoid suggesting explicit response types for `useQuery` calls unless necessary, as the team prefers to rely on TypeScript's type inference.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-11T11:51:09.236Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Applied to files:
config/vite.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Generate and Validate Documentation
- GitHub Check: Analyse Code With CodeQL (javascript)
palisadoes
requested changes
Nov 28, 2025
config/docker/setup/apache.conf
Outdated
| @@ -0,0 +1,21 @@ | |||
| <VirtualHost *:80> | |||
| ServerName localhost | |||
| DocumentRoot /usr/share/nginx/html | |||
Contributor
There was a problem hiding this comment.
- Why was this added?
- Why does it refer to nginx?
Author
There was a problem hiding this comment.
Thanks for reviewing!
-
I added this file because the comment suggestion mentioned adding an apache.conf example as a guide.
-
The Nginx path was a copy-paste mistake on my part. I have fixed it to use the standard Apache path (
/usr/local/apache2/htdocs) in the latest commit.
| @@ -0,0 +1,21 @@ | |||
| <VirtualHost *:80> | |||
| ServerName localhost | |||
Contributor
There was a problem hiding this comment.
Add comments as to when this needs to be changed
Author
There was a problem hiding this comment.
I have added a comment explaining that ServerName needs to be updated to the actual domain in production.
| # Serve React App (FIXED SECURITY) | ||
| <Directory /usr/share/nginx/html> | ||
| Options FollowSymLinks | ||
| AllowOverride None |
Author
There was a problem hiding this comment.
I included this to prevent 404 errors on page refresh. Since it's a Single Page Application, Apache needs to know to serve index.html for non-root paths.
config/vite.config.ts
Outdated
| port: PORT, | ||
| proxy: { | ||
| '/graphql': { | ||
| target: process.env.VITE_API_URL || 'http://localhost:4000', |
Contributor
There was a problem hiding this comment.
- What is
VITE_API_URL? - Where is it documented?
- Where is it added to the code base, for example in the setup script?
Author
There was a problem hiding this comment.
I added VITE_API_URL as per CodeRabbit suggestion, though I too was unsure about this. So I am reverting this change to standard http://localhost:4000.
Contributor
There was a problem hiding this comment.
- How has your update resolved the issue?
- Localhost is now hard coded in the code base. A variable is required. You added one without any follow up code. Also the VITE_API_URL name is specific to a package. Is that the only naming convention vite will accept? Can a more user friendly name be used?
Author
There was a problem hiding this comment.
Thanks for the guidance @palisadoes. You are right, hardcoding localhost is not good for flexibility.
I propose using TALAWA_API_URL as the variable name.
My Plan:
- Update
vite.config.tsto useloadEnvto read this variable. - Update
.env.exampleto document it.
| ``` | ||
| :::tip Proxy Configuration | ||
| To avoid CORS issues when hosting the Admin and API on different IP addresses, you can use a **relative path** for the backend URL. | ||
|
|
Contributor
There was a problem hiding this comment.
- The documentation must assume that the API and Admin apps are always running on separate IP addresses whether or not it's a Dev or Prod environment.
- You must assume the configuration requires IP addresses to be
ALWAYSprovided. - Your approach is limited to the software developer perspective and not the end user who will be deploying this on distributed systems.
Author
There was a problem hiding this comment.
I have updated the documentation to reflect the distributed system perspective, clarifying that the Web Server must be configured to proxy traffic to the API IP.
abhishekblue
force-pushed
the
fix/xss-cors
branch
from
10db1b5 to
9ce303c
Compare
palisadoes
requested changes
Nov 29, 2025
Contributor
palisadoes
left a comment
palisadoes
left a comment
There was a problem hiding this comment.
- Please test this on your local system.
- Make the configuration for the API be the IP address of your network interface and not localhost to test.
- There may be firewall rules to update, so test to see whether you can connect to the the network IP on the API TCP/IP port beforehand
- That's the only way you'll be able to validate your work.
Author
|
@palisadoes Here is how I tested it: Result: |
Contributor
|
@VanshikaSabharwal Your input will be useful here based on your experience with other related issues |
palisadoes
added
the
ignore-sensitive-files-pr
Contributor
|
@palisadoes it looks good to me but we have apache server running the demo site, so do we need to have 2 reverse proxies? |
Contributor
|
If we are using NGINX here, we need to remove the Apache server deployment and redeploy. DO you have a plan for it? |
abhishekblue
force-pushed
the
fix/xss-cors
branch
from
f4fbd41 to
f4f916a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.env.example(1 hunks)config/vite.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.212Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.212Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 4073
File: src/GraphQl/Mutations/VenueMutations.ts:13-31
Timestamp: 2025-08-11T07:28:44.493Z
Learning: In the Talawa Admin project, the capacity and file fields were temporarily removed from venue-related GraphQL mutations (CREATE_VENUE_MUTATION, UPDATE_VENUE_MUTATION) because the backend API doesn't currently support these fields. These fields are planned to be added back once the API is updated to support them.
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 4073
File: src/GraphQl/Queries/OrganizationQueries.ts:324-343
Timestamp: 2025-08-11T07:33:55.926Z
Learning: In the Talawa Admin project, the capacity and file/imageUrl fields were temporarily removed from the VENUE_LIST query (venuesByOrganization) in src/GraphQl/Queries/OrganizationQueries.ts because the backend API doesn't currently support these fields. These fields are planned to be added back once the API is updated to support them. The UI code may still reference these fields, creating a temporary mismatch.
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 4022
File: .github/workflows/cypress.yml:95-103
Timestamp: 2025-07-23T22:29:25.082Z
Learning: In the talawa-admin project, the npm script used to start the development server for Cypress testing is "npm run serve", not the more common "npm run dev" or "npm start".
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-13T18:07:48.621Z
Learning: In talawa-admin, Apollo Client pinned to 3.11.10 (not 3.14.0) to avoid deprecation warnings during test isolation work (PR #4565 Phase 2A-2B). Apollo 3.14.0's MockedProvider internally uses deprecated options causing console noise that interferes with debugging. Upgrade to 4.x blocked by dependency conflicts with apollo-upload-client and apollo/link-error. All addTypename props removed from 115+ test files for future compatibility. Will upgrade when dependencies support Apollo 4.x.
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
📚 Learning: 2025-07-11T20:39:54.613Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Applied to files:
config/vite.config.ts.env.example
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?error}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
config/vite.config.ts.env.example
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?Error message}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
config/vite.config.ts.env.example
📚 Learning: 2025-02-02T10:18:37.840Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Applied to files:
.env.example
🧬 Code graph analysis (1)
config/vite.config.ts (3)
scripts/run-shard.js (1)
env(29-29)scripts/start-server.js (2)
parsed(7-7)PORT(8-8)src/plugin/vite/internalFileWriterPlugin.ts (1)
createInternalFileWriterPlugin(32-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Generate and Validate Documentation
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
config/vite.config.ts (2)
6-7: Verify if new plugins are necessary for the CORS fix.The PR introduces two new plugins that don't appear directly related to the CORS resolution:
createInternalFileWriterPlugin(lines 32-36): Adds file write capabilities to the dev serveristanbul(lines 37-54): Adds code coverage instrumentationThe PR objectives focus on resolving CORS issues via reverse proxy configuration, but these plugins serve different purposes. Including unrelated changes in a bug-fix PR can complicate review and testing.
Please confirm whether these plugins are:
- Required dependencies for the CORS proxy functionality, or
- Separate enhancements that should be in a different PR
If these are unrelated to the CORS fix, consider moving them to a separate PR to maintain focused, atomic changes.
Also applies to: 32-54
56-66: Server configuration appropriately supports the CORS fix.The server configuration correctly enables remote access and flexible host handling:
host: '0.0.0.0'allows connections from any network interface (necessary for testing on different IPs)allowedHosts: trueremoves host header validation (supports the multi-domain/IP scenario described in issue #4697)port: PORTuses the environment variable for flexibilityNote: The
allowedHosts: truesetting disables host header validation, which could be a security concern. However, this is acceptable since it only affects the development server and production deployments use Nginx/Apache with proper host validation. The comment correctly explains the rationale.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.env.example(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.212Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.212Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-06T05:15:51.829Z
Learning: In talawa-admin PR #4565, test sharding implementation achieved 2x speedup (8min → 4min) with conservative settings (maxConcurrency: 1, concurrent: false, maxThreads: 2 in CI) due to test isolation issues across 269 test files. These settings prevent OOM and race conditions. Higher speedup (3-4x) is achievable but requires systematic test isolation improvements. Apollo Client addTypename={false} deprecation warnings (142 files) are suppressed with TODO for follow-up fixes.
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:9-14
Timestamp: 2025-11-29T19:45:52.397Z
Learning: In the talawa-admin project, environment variables use the TALAWA_ prefix (e.g., TALAWA_API_URL) rather than the Vite-conventional VITE_ prefix to maintain a user-friendly, non-package-specific naming scheme that is consistent with other project variables like REACT_APP_TALAWA_URL.
📚 Learning: 2025-11-29T19:45:52.397Z
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:9-14
Timestamp: 2025-11-29T19:45:52.397Z
Learning: In the talawa-admin project, environment variables use the TALAWA_ prefix (e.g., TALAWA_API_URL) rather than the Vite-conventional VITE_ prefix to maintain a user-friendly, non-package-specific naming scheme that is consistent with other project variables like REACT_APP_TALAWA_URL.
Applied to files:
.env.example
📚 Learning: 2025-07-11T20:39:54.613Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Applied to files:
.env.example
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?error}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
.env.example
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?Error message}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
.env.example
📚 Learning: 2025-02-13T20:43:05.966Z
Learnt from: hustlernik
Repo: PalisadoesFoundation/talawa-admin PR: 3595
File: src/screens/CommunityProfile/CommunityProfile.tsx:162-163
Timestamp: 2025-02-13T20:43:05.966Z
Learning: In the talawa-admin GraphQL schema, the LinkedIn URL field is defined as `linkedinURL` (lowercase 'in'), even though the state variable in the frontend uses `linkedInURL` (camelCase).
Applied to files:
.env.example
📚 Learning: 2025-02-02T10:18:37.840Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Applied to files:
.env.example
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Generate and Validate Documentation
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
palisadoes
requested changes
Dec 8, 2025
Contributor
palisadoes
left a comment
palisadoes
left a comment
There was a problem hiding this comment.
It's failing in this scenario
palisadoes
requested changes
Dec 8, 2025
Contributor
palisadoes
left a comment
palisadoes
left a comment
There was a problem hiding this comment.
It's not clear in the documentation how the configuration needs to be updated when the app runs:
- With Docker
- Without Docker
You need to add examples for all the following cases:
- Docker
- API and Admin run on local host
- API and Admin run on different systems
- Non Docker
- API and Admin run on local host
- API and Admin run on different systems
Please provide those details
| **1. React Configuration:** | ||
| ```bash | ||
| REACT_APP_TALAWA_URL=/graphql | ||
| REACT_APP_BACKEND_WEBSOCKET_URL=/graphql |
Contributor
There was a problem hiding this comment.
It's not clear in the documentation how the configuration needs to be updated when the app runs:
- With Docker
- Without Docker
You need to add examples for all the following cases:
- Docker
- API and Admin run on local host
- API and Admin run on different systems
- Non Docker
- API and Admin run on local host
- API and Admin run on different systems
Contributor
|
I was delayed in evaluating this. I was traveling and didn't have access to my systems. |
abhishekblue
force-pushed
the
fix/xss-cors
branch
from
3afbc47 to
01cf5d0
Compare
palisadoes
requested changes
Dec 8, 2025
Contributor
palisadoes
left a comment
palisadoes
left a comment
There was a problem hiding this comment.
Thee is nothing here related to this variable which uses a similar URL
REACT_APP_BACKEND_WEBSOCKET_URL
Author
No worries! I hope the trip was good. |
Contributor
|
Please fix the conflicting file |
palisadoes
requested changes
Dec 8, 2025
Contributor
palisadoes
left a comment
palisadoes
left a comment
There was a problem hiding this comment.
This reference to REACT_APP_BACKEND_WEBSOCKET_URL in config/vite.config may need to be updated.
./config/vite.config.ts:37: 'process.env.REACT_APP_BACKEND_WEBSOCKET_URL': JSON.stringify('/graphql'),
Author
This is intentional. I set this to |
Author
I think the issue is this: REACT_APP_TALAWA_URL was set to You simply set |
Contributor
|
Contributor
|
@coderabbitai full review and reevaluate all resolved items and include the following points:
|
Contributor
There was a problem hiding this comment.
Comprehensive Review - Changes Required
@abhishekblue, I've completed a thorough review of PR #4880 as requested by @palisadoes. While the reverse proxy implementation approach is sound, there are several critical issues that need to be addressed before this PR can be approved.
1. ❌ False Test Coverage Claim
Issue: The PR description states "tests were added and meet or exceed 95% coverage," but the diff shows zero test files were modified or added.
Evidence:
# Files changed in this PR:
- CONTRIBUTING.md (4+/6-) - TOC changes only
- config/docker/setup/apache.conf (26+/0-) - New file
- config/docker/setup/nginx.conf (3+/7-)
- config/vite.config.ts (82+/54-)
- docs/docs/docs/getting-started/configuration.md (11+/42-)
# Test files modified: 0Required: Either remove the false claim from the PR description or add the missing tests.
2. ❌ Missing Unit Tests for vite.config.ts Logic
Issue: The new configuration logic in config/vite.config.ts is completely untested. Configuration files are explicitly excluded from coverage in vitest.config.ts, so this code has 0% test coverage.
Untested Code Paths (Lines 9-26):
-
PORT parsing and validation (lines 11-13):
const parsed = parseInt(env.PORT || '', 10); const PORT = !isNaN(parsed) && parsed >= 1024 && parsed <= 65535 ? parsed : 4321;
-
URL parsing with try-catch (lines 18-26):
let apiTarget = 'http://localhost:4000'; try { const urlObj = new URL(fullBackendUrl); apiTarget = urlObj.origin; } catch { apiTarget = 'http://localhost:4000'; }
-
define block forcing relative paths (lines 35-37):
define: { 'process.env.REACT_APP_TALAWA_URL': JSON.stringify('/graphql'), 'process.env.REACT_APP_BACKEND_WEBSOCKET_URL': JSON.stringify('/graphql'), }
Required Tests: Create config/vite.config.spec.ts (or similar) to test:
- PORT validation accepts valid ports (1024-65535)
- PORT validation rejects invalid ports (<1024, >65535, NaN, empty)
- PORT defaults to 4321 when invalid
- URL parsing correctly extracts origin from valid URLs
- URL parsing falls back to localhost:4000 for invalid URLs
- define block correctly overrides environment variables
Pattern to follow: Similar logic exists in src/setup/askForDocker/askForDocker.ts and IS properly tested in askForDocker.spec.ts. Follow that pattern.
3. ❌ No Integration/E2E Tests for Proxy Behavior
Issue: No tests verify that the proxy configuration actually works in development or production.
Required: Add integration tests or document manual testing steps that verify:
- Development proxy forwards /graphql to the API correctly
- WebSocket connections (ws: true) work through the proxy
- CORS headers are not needed when using the proxy
- Relative path /graphql is correctly resolved
4. ⚠️ Nginx Configuration Files Need Clarification
Issue: Both config/docker/setup/nginx.conf and config/docker/setup/nginx.prod.conf exist with different configurations:
nginx.conf: Simple HTTP-only configurationnginx.prod.conf: Full production config with HTTPS, SSL, Brotli compression, security headers
Questions:
- Which file is actually used in production?
- Should
nginx.prod.confbe updated with the same proxy changes? - Should one of these files be removed to avoid confusion?
The PR description mentions: "existing potentially unused file config/docker/setup/nginx.prod.conf left unchanged" - but doesn't explain WHY or which one should be used.
Required: Clarify the purpose of each file and update both if necessary, or remove the unused one.
5. ⚠️ Missing Documentation File
Issue: The PR diff shows changes to docs/docs/docs/getting-started/configuration.md but this file couldn't be found in the repository.
Required: Verify the documentation file path and ensure it exists.
6. ✅ What Looks Good
- Vite proxy configuration approach is correct
- Apache configuration follows best practices
- Nginx configuration uses proper proxy headers
- Code comments clearly explain Docker service name vs IP usage
- URL parsing uses try-catch for safety
Summary
This PR requires the following changes before it can be approved:
- Add unit tests for the new logic in
config/vite.config.ts - Remove or substantiate the test coverage claim in the PR description
- Clarify the nginx.conf vs nginx.prod.conf situation and update both if needed
- Verify documentation file path exists
- Consider adding integration tests or document manual testing steps for proxy behavior
Once these issues are addressed, I'll be happy to re-review and approve.
Note: Configuration is critical infrastructure code. While it's often overlooked in testing, the logic you've added (PORT validation, URL parsing) contains business logic that should be tested to prevent regressions.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2884 🏁 Script executed: Length of output: 5013 🏁 Script executed: Length of output: 5867 🏁 Script executed: Length of output: 2115 🏁 Script executed: Length of output: 8447 🏁 Script executed: Length of output: 4656 🏁 Script executed: Length of output: 195 🏁 Script executed: Length of output: 3006 🏁 Script executed: Length of output: 2010 🏁 Script executed: Length of output: 3749 🏁 Script executed: Length of output: 7211 🏁 Script executed: Length of output: 6928 🏁 Script executed: Length of output: 88 🏁 Script executed: Length of output: 0 @palisadoes, I've completed the comprehensive full review and reevaluation of all items in PR #4880 as requested. Here's my analysis: Review SummaryI've posted a detailed review with "Request Changes" status via GitHub CLI, as per the learning from our previous interactions. Critical Findings1. Test Coverage Issues (Your Point #1 & #2)
Required: Create 2. Unnecessary/Unclear Files (Your Point #3)
Required: Clarify which nginx file(s) should be used and update accordingly 3. Missing Documentation
What Looks Good ✅
Review Status (Your Point #4)✅ Review status set to "Request Changes" via GitHub CLI per the retrieved learning guidelines. The PR cannot be approved until the test coverage issues are resolved and the nginx file situation is clarified. 🧠 Learnings used |
Contributor
|
How can we treat the vite config for as an exception? It's setting up important operational parameters. Is this sometime that is normally done? |
Contributor
|
Contributor
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)
vitest.config.ts (1)
73-73: Inconsistency: Config files tested but excluded from coverage.Lines 17-20 include config//*.{spec,test} files in the test suite, but line 73 excludes config/ from coverage collection. This means config tests will execute but won't contribute to coverage metrics, creating an incomplete picture of test coverage.
If config file tests are valuable enough to run (which they are, given the new vite.config.spec.ts), their coverage should be measured. Apply this diff:
- 'config/**', // Exclude configuration files + 'config/**/*.config.{js,ts}', // Exclude other config files but allow coverage for tested utilitiesAlternatively, if config files should truly be excluded from coverage, document the rationale (e.g., "Configuration files are integration-tested in CI, unit test coverage not required").
♻️ Duplicate comments (1)
config/vite.config.ts (1)
40-43: Hardcoded '/graphql' paths remove configuration flexibility.The
defineblock forces bothREACT_APP_TALAWA_URLandREACT_APP_BACKEND_WEBSOCKET_URLto '/graphql', removing the ability to configure different paths for HTTP vs WebSocket or to use custom GraphQL endpoints (e.g., '/api/graphql', '/v2/graphql').While this simplification aligns with the PR's proxy-based approach, consider:
- Document the constraint: Add a comment explaining that custom GraphQL paths require updating both vite.config.ts and reverse proxy configurations
- Validation: If custom paths are not supported, validate that user-provided environment variables use '/graphql' and warn if they differ
- Future flexibility: Consider deriving WebSocket path from the HTTP path rather than hardcoding both:
const graphqlPath = '/graphql'; // Single source of truth define: { 'process.env.REACT_APP_TALAWA_URL': JSON.stringify(graphqlPath), 'process.env.REACT_APP_BACKEND_WEBSOCKET_URL': JSON.stringify(graphqlPath), },This makes future path changes easier (one place to update) and makes the constraint explicit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/docs/docs/getting-started/configuration.mdis excluded by!**/docs/docs/**
📒 Files selected for processing (4)
config/docker/setup/nginx.prod.conf(0 hunks)config/vite.config.spec.ts(1 hunks)config/vite.config.ts(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- config/docker/setup/nginx.prod.conf
🧰 Additional context used
🧠 Learnings (29)
📓 Common learnings
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
📚 Learning: 2025-11-29T19:45:52.413Z
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:9-14
Timestamp: 2025-11-29T19:45:52.413Z
Learning: In the talawa-admin project, environment variables use the TALAWA_ prefix (e.g., TALAWA_API_URL) rather than the Vite-conventional VITE_ prefix to maintain a user-friendly, non-package-specific naming scheme that is consistent with other project variables like REACT_APP_TALAWA_URL.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-07-11T20:39:54.613Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?error}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?Error message}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-11-20T11:32:56.672Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-28T23:56:12.253Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-27T15:33:20.511Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 4866
File: .github/workflows/pull-request-target.yml:28-30
Timestamp: 2025-11-27T15:33:20.511Z
Learning: In the talawa-admin repository, the PR greeting message in .github/workflows/pull-request-target.yml is intentionally simplified to only reference the centralized Palisadoes Contributing Guidelines (https://developer.palisadoes.org/docs/contributor-guide/contributing). This simplification is part of a strategic decision to centralize PR guidance and reporting across multiple Palisadoes repositories, rather than maintaining separate detailed documentation in each repository's workflow files.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-04-13T10:05:41.802Z
Learnt from: bandhan-majumder
Repo: PalisadoesFoundation/talawa-admin PR: 3926
File: src/style/app-fixed.module.css:8456-8463
Timestamp: 2025-04-13T10:05:41.802Z
Learning: For the Talawa Admin repository, focus feedback on substantive issues rather than minor styling suggestions, especially when the code follows established patterns in the codebase.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-11T11:51:09.236Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Applied to files:
config/vite.config.tsconfig/vite.config.spec.ts
📚 Learning: 2025-11-06T05:15:51.829Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-06T05:15:51.829Z
Learning: In talawa-admin PR #4565, test sharding implementation achieved 2x speedup (8min → 4min) with conservative settings (maxConcurrency: 1, concurrent: false, maxThreads: 2 in CI) due to test isolation issues across 269 test files. These settings prevent OOM and race conditions. Higher speedup (3-4x) is achievable but requires systematic test isolation improvements. Apollo Client addTypename={false} deprecation warnings (142 files) are suppressed with TODO for follow-up fixes.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-12T00:28:53.713Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Applied to files:
config/vite.config.tsvitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2025-11-30T23:13:22.697Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Applied to files:
config/vite.config.tsconfig/vite.config.spec.ts
📚 Learning: 2025-11-28T22:51:12.245Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T22:51:12.245Z
Learning: In talawa-admin PR #4826, mock cleanup implementation uses global vi.clearAllMocks() in vitest.setup.ts afterEach hook, combined with proper vi.hoisted() usage for module-level mocks. This strategy successfully achieved test isolation across 273 test files, passing the mock cleanup checker script with exit code 0. The implementation is ready for increasing maxConcurrency from conservative settings (CI: 1, local: 2) to higher concurrency levels (recommended gradual rollout: CI 1→2→4, local 2→4→8) to achieve Phase 2A goals of 2.3-2.5x speedup. Global cleanup strategy eliminates need for explicit afterEach blocks in every individual test file.
Applied to files:
config/vite.config.tsvitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2025-10-29T04:36:50.503Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 4550
File: .github/workflows/pull-request.yml:377-382
Timestamp: 2025-10-29T04:36:50.503Z
Learning: In the talawa-admin repository's .github/workflows/pull-request.yml, the Check-AutoDocs job is intentionally added as a dependency of Test-Application to create a sequential chain (Code-Quality-Checks → Check-AutoDocs → Test-Application), ensuring documentation validation must complete before tests run. This design is approved and should not be flagged as a CI latency concern.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-13T18:07:48.621Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-13T18:07:48.621Z
Learning: In talawa-admin, Apollo Client pinned to 3.11.10 (not 3.14.0) to avoid deprecation warnings during test isolation work (PR #4565 Phase 2A-2B). Apollo 3.14.0's MockedProvider internally uses deprecated options causing console noise that interferes with debugging. Upgrade to 4.x blocked by dependency conflicts with apollo-upload-client and apollo/link-error. All addTypename props removed from 115+ test files for future compatibility. Will upgrade when dependencies support Apollo 4.x.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-12-02T04:20:11.745Z
Learnt from: bitbard3
Repo: PalisadoesFoundation/talawa-admin PR: 2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155
Timestamp: 2024-12-02T04:20:11.745Z
Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.
Applied to files:
config/vite.config.tsvitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2025-02-02T10:18:37.840Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-11-26T04:34:26.475Z
Learnt from: varshith257
Repo: PalisadoesFoundation/talawa-admin PR: 2457
File: jest.config.js:9-9
Timestamp: 2024-11-26T04:34:26.475Z
Learning: In `jest.config.js`, configure `testMatch` to include only `.test.*` files, as `.spec.*` files are used for Vitest tests.
Applied to files:
vitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2024-12-09T15:54:04.872Z
Learnt from: MayankJha014
Repo: PalisadoesFoundation/talawa-admin PR: 2619
File: vitest.config.ts:0-0
Timestamp: 2024-12-09T15:54:04.872Z
Learning: The files `scripts/custom-test-env.js`, `src/utils/i18nForTest.ts`, and `vitest.setup.ts` are not test files and should not be included in the test pattern.
Applied to files:
vitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2025-08-17T07:39:34.255Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 4077
File: package.json:189-213
Timestamp: 2025-08-17T07:39:34.255Z
Learning: The Talawa Admin codebase primarily uses .spec.tsx/.spec.ts naming convention for unit tests, with Cypress tests using .cy.ts pattern. However, there is at least one .test.tsx file in the codebase, so NYC exclude patterns should include both .spec and .test patterns.
Applied to files:
vitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2024-12-09T15:54:04.872Z
Learnt from: MayankJha014
Repo: PalisadoesFoundation/talawa-admin PR: 2619
File: vitest.config.ts:0-0
Timestamp: 2024-12-09T15:54:04.872Z
Learning: In the Talawa-Admin project, `src/setupTests.ts` is defined for Jest, and including it in Vitest causes reference errors.
Applied to files:
vitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2024-11-27T06:37:47.871Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2482
File: src/components/AddOn/support/components/Action/Action.spec.tsx:1-8
Timestamp: 2024-11-27T06:37:47.871Z
Learning: In the Talawa-Admin project, the `testing-library/jest-dom` package is imported globally in `vitest.setup.ts`, so individual test files do not need to import it separately.
Applied to files:
vitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2024-10-08T16:13:41.996Z
Learnt from: Chaitanya1672
Repo: PalisadoesFoundation/talawa-admin PR: 2049
File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138
Timestamp: 2024-10-08T16:13:41.996Z
Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.
Applied to files:
vitest.config.ts
📚 Learning: 2024-11-26T07:19:31.226Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 2457
File: .github/workflows/pull-request.yml:0-0
Timestamp: 2024-11-26T07:19:31.226Z
Learning: When merging coverage reports from Jest and Vitest, ensure both testing frameworks output coverage in the same format (e.g., 'lcov') and use a compatible tool like 'lcov-result-merger' to combine the reports, as 'nyc merge' may not handle the different coverage formats correctly.
Applied to files:
vitest.config.ts
📚 Learning: 2025-02-02T14:28:38.521Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3526
File: src/setupTests.ts:35-36
Timestamp: 2025-02-02T14:28:38.521Z
Learning: In talawa-admin project, global timer setup using `vi.useFakeTimers()` and `vi.advanceTimersByTime(18000)` in `setupTests.ts` is the accepted approach for test files, as it works well with their test suite requirements.
Applied to files:
vitest.config.ts
📚 Learning: 2025-12-06T11:50:22.732Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Each shared component must have a corresponding Component.spec.tsx test file.
Applied to files:
vitest.config.ts
📚 Learning: 2025-11-28T16:02:31.814Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 4882
File: src/screens/UserPortal/Volunteer/UpcomingEvents/RecurringEventVolunteerModal.spec.tsx:69-109
Timestamp: 2025-11-28T16:02:31.814Z
Learning: In Talawa Admin test files (e.g., src/screens/UserPortal/Volunteer/UpcomingEvents/RecurringEventVolunteerModal.spec.tsx), developers prefer keeping date/time mocks (like vi.spyOn(Date.prototype, 'toLocaleDateString')) inline within individual test cases rather than consolidating them in beforeEach blocks, to maintain clarity and keep mocks close to the tests that use them, even if it results in some duplication.
Applied to files:
vitest.config.tsconfig/vite.config.spec.ts
📚 Learning: 2025-11-17T22:18:09.680Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-17T22:18:09.680Z
Learning: Talawa Admin Phase 2A testing guideline: Allow vi.hoisted() + vi.mock() module-level mocks for shared dependencies; prefer afterEach(() => vi.clearAllMocks()) as default cleanup. Use afterEach(() => vi.restoreAllMocks()) only in suites that create vi.spyOn spies or patch real implementations. Avoid vi.resetAllMocks() globally. The generic “no module-level mocks” rule is updated to “no module-level vi.fn except vi.hoisted module mocks.”
Applied to files:
config/vite.config.spec.ts
🧬 Code graph analysis (2)
config/vite.config.ts (2)
scripts/start-server.js (2)
parsed(7-7)PORT(8-8)src/plugin/vite/internalFileWriterPlugin.ts (1)
createInternalFileWriterPlugin(32-161)
config/vite.config.spec.ts (2)
src/setup/askForCustomPort/askForCustomPort.ts (1)
validatePort(6-17)scripts/start-server.js (1)
parsed(7-7)
🔇 Additional comments (8)
vitest.config.ts (1)
17-26: LGTM! Test pattern extension aligns with new config tests.The expanded test pattern correctly includes config/**/*.{spec,test}.{js,jsx,ts,tsx} to support the new config/vite.config.spec.ts test file, and the exclude list appropriately filters out common non-test directories.
config/vite.config.spec.ts (3)
33-159: LGTM! Comprehensive test coverage for config utilities.The test suite thoroughly validates:
- Port validation boundaries (1024-65535 range, NaN handling, defaults)
- API target extraction (HTTP/HTTPS, ports, IPv6, error fallbacks)
- WebSocket path derivation (missing/empty/invalid URLs, query params, complex paths)
Test structure is clear, edge cases are well-covered, and assertions are precise.
24-31: Tested utility doesn't exist in actual implementation.The
deriveWebSocketPathfunction is tested comprehensively (lines 111-158) but doesn't appear inconfig/vite.config.ts. This indicates either:
- Missing implementation: The utility should be used for WebSocket path derivation in the config
- Over-testing: The utility is not needed and tests should be removed
Verify the intended usage:
#!/bin/bash # Search for WebSocket path derivation or REACT_APP_BACKEND_WEBSOCKET_URL processing in vite.config.ts rg -n "WEBSOCKET|websocket|deriveWebSocketPath" config/vite.config.ts -C3If the utility is needed, update
vite.config.tsto use it. If not, remove the unused test suite to avoid misleading coverage.⛔ Skipped due to learnings
Learnt from: MayankJha014 Repo: PalisadoesFoundation/talawa-admin PR: 2619 File: vitest.config.ts:0-0 Timestamp: 2024-12-09T15:54:04.872Z Learning: The files `scripts/custom-test-env.js`, `src/utils/i18nForTest.ts`, and `vitest.setup.ts` are not test files and should not be included in the test pattern.Learnt from: MayankJha014 Repo: PalisadoesFoundation/talawa-admin PR: 2619 File: vitest.config.ts:0-0 Timestamp: 2024-12-09T15:54:04.872Z Learning: In the Talawa-Admin project, `src/setupTests.ts` is defined for Jest, and including it in Vitest causes reference errors.Learnt from: adithyanotfound Repo: PalisadoesFoundation/talawa-admin PR: 2350 File: setup.ts:87-96 Timestamp: 2024-10-29T01:19:06.787Z Learning: Validation for WebSocket URL construction is not required in the setup script, as per the user's preference.
3-11: Code duplication creates maintenance and drift risk.The inline function approach (duplicating validatePort from vite.config.ts lines 13-15) creates a maintenance burden: changes to the actual implementation require manual synchronization with test utilities, risking drift and false positives.
Recommended solutions:
Extract to shared utility module (preferred):
- Create
config/utils.tswithvalidatePort,extractApiTarget,deriveWebSocketPath- Import utilities in both
vite.config.tsandvite.config.spec.ts- Eliminates duplication and esbuild timing issues
If esbuild timing is truly blocking (fallback):
- Add a validation step in CI that compares inline test utilities with actual implementation using AST analysis or regex matching
- Document the synchronization requirement prominently
The current comment "MUST match vite.config.ts exactly" is a manual process prone to human error.
⛔ Skipped due to learnings
Learnt from: bitbard3 Repo: PalisadoesFoundation/talawa-admin PR: 2588 File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155 Timestamp: 2024-12-02T04:20:11.745Z Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.Learnt from: MayankJha014 Repo: PalisadoesFoundation/talawa-admin PR: 2619 File: vitest.config.ts:0-0 Timestamp: 2024-12-09T15:54:04.872Z Learning: The files `scripts/custom-test-env.js`, `src/utils/i18nForTest.ts`, and `vitest.setup.ts` are not test files and should not be included in the test pattern.Learnt from: Ansingh0305 Repo: PalisadoesFoundation/talawa-admin PR: 0 File: :0-0 Timestamp: 2025-11-12T00:28:53.713Z Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).Learnt from: syedali237 Repo: PalisadoesFoundation/talawa-admin PR: 4077 File: package.json:189-213 Timestamp: 2025-08-17T07:39:34.255Z Learning: The Talawa Admin codebase primarily uses .spec.tsx/.spec.ts naming convention for unit tests, with Cypress tests using .cy.ts pattern. However, there is at least one .test.tsx file in the codebase, so NYC exclude patterns should include both .spec and .test patterns.Learnt from: Chaitanya1672 Repo: PalisadoesFoundation/talawa-admin PR: 2049 File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138 Timestamp: 2024-10-08T16:13:41.996Z Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.Learnt from: MayankJha014 Repo: PalisadoesFoundation/talawa-admin PR: 2619 File: vitest.config.ts:0-0 Timestamp: 2024-12-09T15:54:04.872Z Learning: In the Talawa-Admin project, `src/setupTests.ts` is defined for Jest, and including it in Vitest causes reference errors.Learnt from: krikera Repo: PalisadoesFoundation/talawa-admin PR: 4887 File: src/components/LoginPortalToggle/LoginPortalToggle.spec.tsx:57-59 Timestamp: 2025-11-28T18:25:57.426Z Learning: In talawa-admin tests, when testing CSS module class names (imported as `styles from '*.module.css'`), prefer importing the styles module and using `toHaveClass(styles.className)` over `className.toContain('className')`. Vite hashes CSS module class names (e.g., activeLink → _activeLink_d8535b), so `toHaveClass('activeLink')` with plain strings will fail. The styles import approach is semantic, type-safe, and matches patterns in SideToggle.spec.tsx. Alternatively, `className.toContain()` is acceptable for substring matching without imports.Learnt from: Ansingh0305 Repo: PalisadoesFoundation/talawa-admin PR: 0 File: :0-0 Timestamp: 2025-11-28T22:51:12.245Z Learning: In talawa-admin PR #4826, mock cleanup implementation uses global vi.clearAllMocks() in vitest.setup.ts afterEach hook, combined with proper vi.hoisted() usage for module-level mocks. This strategy successfully achieved test isolation across 273 test files, passing the mock cleanup checker script with exit code 0. The implementation is ready for increasing maxConcurrency from conservative settings (CI: 1, local: 2) to higher concurrency levels (recommended gradual rollout: CI 1→2→4, local 2→4→8) to achieve Phase 2A goals of 2.3-2.5x speedup. Global cleanup strategy eliminates need for explicit afterEach blocks in every individual test file.Learnt from: meetulr Repo: PalisadoesFoundation/talawa-admin PR: 2398 File: src/components/AddPeopleToTag/AddPeopleToTag.test.tsx:177-241 Timestamp: 2024-11-02T07:48:36.443Z Learning: In `src/components/AddPeopleToTag/AddPeopleToTag.test.tsx`, prefer keeping test cases separate and more readable, even if it involves some duplication, instead of extracting common logic into helper functions.Learnt from: rawadhossain Repo: PalisadoesFoundation/talawa-admin PR: 4882 File: src/screens/UserPortal/Volunteer/UpcomingEvents/RecurringEventVolunteerModal.spec.tsx:69-109 Timestamp: 2025-11-28T16:02:31.814Z Learning: In Talawa Admin test files (e.g., src/screens/UserPortal/Volunteer/UpcomingEvents/RecurringEventVolunteerModal.spec.tsx), developers prefer keeping date/time mocks (like vi.spyOn(Date.prototype, 'toLocaleDateString')) inline within individual test cases rather than consolidating them in beforeEach blocks, to maintain clarity and keep mocks close to the tests that use them, even if it results in some duplication.config/vite.config.ts (4)
9-11: LGTM! Factory function enables mode-aware configuration.The dynamic factory correctly uses
loadEnv(mode, process.cwd(), '')to load environment variables based on the build mode, enabling different configurations for development/production without hardcoding.
13-15: Port validation logic matches established pattern.The PORT validation (range 1024-65535, default 4321) correctly matches the existing pattern in
scripts/start-server.jslines 6-7, ensuring consistency across the codebase. The implementation is well-tested by the corresponding test suite inconfig/vite.config.spec.ts.
17-28: API target extraction is correct and well-tested.The fullBackendUrl derivation and apiTarget extraction properly:
- Defaults to
http://localhost:4000/graphqlwhenREACT_APP_TALAWA_URLis not set- Extracts the origin for proxy targeting
- Handles URL parsing errors with appropriate fallback
The logic matches the test utilities in
config/vite.config.spec.tslines 13-22.
82-98: Development server configuration is correct but has security considerations.The server configuration properly implements:
- Dynamic PORT with fallback (line 91)
- Proxy to apiTarget with WebSocket support (lines 92-97)
- Open network access via
host: '0.0.0.0'(line 89)However,
allowedHosts: true(line 83) disables host header checking, which could allow DNS rebinding attacks in development.Recommendations if security constraints are required:
- If Talawa runs on multiple specific domains in development, specify them explicitly:
allowedHosts: ['localhost', '.talawa.io', '.local'],- If only LAN IP access is needed (e.g., 192.168.x.x), use:
allowedHosts: 'auto', // Allows localhost and LAN IPsNote: Verify with the team whether
allowedHosts: trueis necessary for the project's development requirements before applying changes.
config/vite.config.ts
Outdated
Comment on lines
30
to
32
| // Override environment variables for client-side builds to use relative paths | ||
| process.env.REACT_APP_TALAWA_URL = '/graphql'; | ||
| process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql'; |
Contributor
There was a problem hiding this comment.
Process.env mutation is unconventional and affects all subsequent imports.
Mutating process.env directly (lines 30-32) to force relative paths is unusual and can cause unexpected behavior:
- Affects all code loaded after vite.config.ts
- Makes debugging difficult (environment doesn't match .env files)
- Breaks the principle of least surprise
The define block (lines 40-43) already handles build-time replacements. Consider:
- // Override environment variables for client-side builds to use relative paths
- process.env.REACT_APP_TALAWA_URL = '/graphql';
- process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql';
-
return {
// Production build configuration
build: {
outDir: 'build',
},
// Global build definitions
define: {
- // Backup build definitions (process.env overrides take precedence)
+ // Force relative paths for proxy usage in both dev and production
'process.env.REACT_APP_TALAWA_URL': JSON.stringify('/graphql'),
'process.env.REACT_APP_BACKEND_WEBSOCKET_URL': JSON.stringify('/graphql'),
},This achieves the same result (client code sees '/graphql') without mutating the global process.env. The define block performs compile-time replacement, which is the standard Vite/Rollup pattern for environment variable overrides.
🤖 Prompt for AI Agents
In config/vite.config.ts around lines 30-32, remove the direct process.env
assignments that mutate globals; instead, add compile-time replacements in the
existing define block (lines ~40-43) so the client build sees '/graphql' without
changing process.env at runtime — e.g. add entries mapping the exact keys you
use in client code (process.env.REACT_APP_TALAWA_URL and
process.env.REACT_APP_BACKEND_WEBSOCKET_URL or import.meta.env equivalents) to
the string '/graphql' via JSON.stringify('/graphql'), then delete the three
lines that set process.env directly.
Author
There was a problem hiding this comment.
I understand that mutating process.env is not ideal. These lines are added to strictly force the relative path (/graphql) for the proxy to work correctly in dev mode. If we remove this, the connection breaks.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of REACT_APP_TALAWA_URL in the source code
echo "=== Searching for REACT_APP_TALAWA_URL usage ==="
rg -n -C3 --type=ts --type=tsx --type=js --type=jsx 'REACT_APP_TALAWA_URL' src/
echo ""
echo "=== Searching for REACT_APP_BACKEND_WEBSOCKET_URL usage ==="
rg -n -C3 --type=ts --type=tsx --type=js --type=jsx 'REACT_APP_BACKEND_WEBSOCKET_URL' src/
echo ""
echo "=== Checking how EnvironmentPlugin works ==="
rg -n 'import.*EnvironmentPlugin' config/
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Search for usage of REACT_APP_TALAWA_URL in the source code
echo "=== Searching for REACT_APP_TALAWA_URL usage ==="
rg -n -C3 'REACT_APP_TALAWA_URL' src/ -g '*.{ts,tsx,js,jsx}'
echo ""
echo "=== Searching for REACT_APP_BACKEND_WEBSOCKET_URL usage ==="
rg -n -C3 'REACT_APP_BACKEND_WEBSOCKET_URL' src/ -g '*.{ts,tsx,js,jsx}'
echo ""
echo "=== Check EnvironmentPlugin behavior ==="
echo "EnvironmentPlugin('all') loads all process.env vars and makes them available"
echo "Let's check if there are any dynamic env access patterns:"
rg -n 'process\.env\[' src/ -g '*.{ts,tsx,js,jsx}' | head -20
Length of output: 9350
@abhishekblue, thank you for the clarification! After reviewing the codebase, I can see why the process.env mutation is necessary in your case.
Key finding: Your code uses EnvironmentPlugin('all') which reads from process.env and injects those values into the client bundle. I found that src/Constant/constant.ts exports:
export const BACKEND_URL = process.env.REACT_APP_TALAWA_URL;
export const REACT_APP_BACKEND_WEBSOCKET_URL: string =
process.env.REACT_APP_BACKEND_WEBSOCKET_URL || '';These are then imported in src/index.tsx to configure Apollo Client and WebSocket connections.
Why your approach works:
- The mutations (lines 30-32) occur before the config is returned
- When
EnvironmentPlugin('all')initializes, it reads the already-mutatedprocess.envvalues - The plugin then injects these mutated values into the client code
- The
defineblock alone wouldn't work becauseEnvironmentPluginis what makesprocess.envavailable at runtime
Recommendation:
Your implementation is valid. To improve code clarity, consider adding a more detailed comment explaining the interaction with EnvironmentPlugin:
// Override environment variables to force relative proxy paths.
// These mutations must occur before EnvironmentPlugin('all') processes them,
// ensuring the client code receives '/graphql' for both dev proxy and production builds.
process.env.REACT_APP_TALAWA_URL = '/graphql';
process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql';Your testing confirms this is the correct approach for your architecture. The implementation is sound. ✅
🧬 Code Graph Analysis Results
[file: config/vite.config.ts]
config/vite.config.ts, lines 34-41:
- Override environment variables for client-side builds to use relative paths
process.env.REACT_APP_TALAWA_URL = '/graphql';
process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql';
config/vite.config.ts, lines 43-50:
- Build-time definitions to reflect the overridden env vars
'process.env.REACT_APP_TALAWA_URL': JSON.stringify('/graphql'),
'process.env.REACT_APP_BACKEND_WEBSOCKET_URL': JSON.stringify('/graphql'),
config/vite.config.ts, lines 66-72:
- Development server proxy configuration for GraphQL
'/graphql': {
target: apiTarget,
changeOrigin: true,
ws: true,
},
🧠 Learnings used
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:9-14
Timestamp: 2025-11-29T19:45:52.413Z
Learning: In the talawa-admin project, environment variables use the TALAWA_ prefix (e.g., TALAWA_API_URL) rather than the Vite-conventional VITE_ prefix to maintain a user-friendly, non-package-specific naming scheme that is consistent with other project variables like REACT_APP_TALAWA_URL.
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?error}` syntax, making additional validation in the TypeScript code redundant.
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?Error message}` syntax, making additional validation in the TypeScript code redundant.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Author
|
Author
|
@coderabbitai Please check my comment and review my latest commit. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9740 🏁 Script executed: Length of output: 747 🏁 Script executed: Length of output: 11524 🌐 Web query: 💡 Result: Common causes
How to fix (quick checklist)
References If you want, paste your vite.config.js, the fetch/axios call you make, and any server middleware (express body-parser usage) and I’ll point out exactly what to change. 🏁 Script executed: Length of output: 245 🏁 Script executed: Length of output: 9981 🏁 Script executed: Length of output: 1880 🏁 Script executed: Length of output: 3580 🏁 Script executed: Length of output: 438 🏁 Script executed: Length of output: 3721 @abhishekblue, excellent work on addressing the previous review findings! Let me provide feedback on your latest changes and help debug the proxy issue. ✅ What You've Done Right
🐛 Debugging the "Data Drop" Proxy IssueBased on your description (browser sends payload, backend receives empty body), here are the most likely causes and solutions: Solution 1: Add
|
| Issue | Status |
|---|---|
| Unit tests for vite.config.ts | ✅ Fixed |
| Test coverage claim | ✅ Fixed (tests added) |
| nginx.conf vs nginx.prod.conf | ✅ Fixed (prod.conf deleted) |
| Documentation clarity | ✅ Fixed |
| vitest.config.ts patterns | ✅ Fixed |
| Proxy body forwarding | ⏳ Needs debugging |
Once you resolve the proxy issue, this PR will be ready for approval! The test implementation approach you took is solid and acceptable given the esbuild constraints.
Let me know what you find from the debugging steps, and I can help narrow down the exact cause.
🧠 Learnings used
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-06T05:15:51.829Z
Learning: In talawa-admin PR #4565, test sharding implementation achieved 2x speedup (8min → 4min) with conservative settings (maxConcurrency: 1, concurrent: false, maxThreads: 2 in CI) due to test isolation issues across 269 test files. These settings prevent OOM and race conditions. Higher speedup (3-4x) is achievable but requires systematic test isolation improvements. Apollo Client addTypename={false} deprecation warnings (142 files) are suppressed with TODO for follow-up fixes.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T22:51:12.245Z
Learning: In talawa-admin PR #4826, mock cleanup implementation uses global vi.clearAllMocks() in vitest.setup.ts afterEach hook, combined with proper vi.hoisted() usage for module-level mocks. This strategy successfully achieved test isolation across 273 test files, passing the mock cleanup checker script with exit code 0. The implementation is ready for increasing maxConcurrency from conservative settings (CI: 1, local: 2) to higher concurrency levels (recommended gradual rollout: CI 1→2→4, local 2→4→8) to achieve Phase 2A goals of 2.3-2.5x speedup. Global cleanup strategy eliminates need for explicit afterEach blocks in every individual test file.
Learnt from: Priyanshuthapliyal2005
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-01-30T10:03:22.905Z
Learning: CodeRabbit does not have permissions to add labels to PRs in the talawa-admin repository. Labels need to be added manually by users with appropriate permissions.
Learnt from: bandhan-majumder
Repo: PalisadoesFoundation/talawa-admin PR: 3926
File: src/style/app-fixed.module.css:8456-8463
Timestamp: 2025-04-13T10:05:41.802Z
Learning: For the Talawa Admin repository, focus feedback on substantive issues rather than minor styling suggestions, especially when the code follows established patterns in the codebase.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 4866
File: .github/workflows/pull-request-target.yml:28-30
Timestamp: 2025-11-27T15:33:20.511Z
Learning: In the talawa-admin repository, the PR greeting message in .github/workflows/pull-request-target.yml is intentionally simplified to only reference the centralized Palisadoes Contributing Guidelines (https://developer.palisadoes.org/docs/contributor-guide/contributing). This simplification is part of a strategic decision to centralize PR guidance and reporting across multiple Palisadoes repositories, rather than maintaining separate detailed documentation in each repository's workflow files.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Author
But in the console I am getting this error: I have also made a new commit. Please check! |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20460 🌐 Web query: 💡 Result: Short answer — two common causes and fixes:
server: { (If using http-proxy-middleware’s onProxyReq the same restream logic applies.) [3][4] Checklist to resolve GraphQL POST-body missing:
References:
Would you like me to adapt the exact configure/onProxyReq code to your Vite config and GraphQL client code? @abhishekblue, excellent debugging information! I found the root cause. This is a known Vite proxy issue where the body stream gets consumed by Vite's internal middleware before the proxy can forward it. 🔍 Root CauseVite's dev server uses ✅ Solution: Re-stream the Request BodyYou need to explicitly re-stream the body in your proxy: {
'/graphql': {
target: apiTarget,
changeOrigin: true,
secure: false,
ws: true,
rewrite: (path) => path,
configure: (proxy, options) => {
// Handle body restreaming for POST requests
proxy.on('proxyReq', (proxyReq, req, res) => {
console.log(
'[Proxy Request]',
req.method,
req.url,
'→',
options.target + req.url,
);
console.log('[Headers]', JSON.stringify(req.headers, null, 2));
// Re-stream the body if it exists
if (req.body) {
const bodyData = JSON.stringify(req.body);
// Update headers
proxyReq.setHeader('Content-Type', 'application/json');
proxyReq.setHeader('Content-Length', Buffer.byteLength(bodyData));
// Write the body to the proxy request
proxyReq.write(bodyData);
proxyReq.end();
}
});
proxy.on('proxyRes', (proxyRes, req, res) => {
console.log(
'[Proxy Response]',
req.method,
req.url,
'←',
proxyRes.statusCode,
);
});
proxy.on('error', (err, req, res) => {
console.error('[Proxy Error]', err.message);
});
},
},
}🐛 Additional Fix NeededLine 32 formatting issue - You have a comment and code on the same line without a newline: // Current (line 30-32):
// These mutations must occur before EnvironmentPlugin('all') processes them,
// ensuring the client code receives '/graphql' for both dev proxy and production builds. process.env.REACT_APP_TALAWA_URL = '/graphql';Should be: // These mutations must occur before EnvironmentPlugin('all') processes them,
// ensuring the client code receives '/graphql' for both dev proxy and production builds.
process.env.REACT_APP_TALAWA_URL = '/graphql';🔍 What to CheckAfter applying the fix:
Alternative Approach (if body restreaming doesn't work)If proxy.on('proxyReq', (proxyReq, req, res) => {
// Collect body chunks
let bodyChunks = [];
req.on('data', (chunk) => {
bodyChunks.push(chunk);
});
req.on('end', () => {
const bodyData = Buffer.concat(bodyChunks);
if (bodyData.length > 0) {
proxyReq.setHeader('Content-Length', bodyData.length);
proxyReq.write(bodyData);
}
proxyReq.end();
});
});Try the first solution first and let me know what the terminal logs show. This should resolve the data drop issue! 🚀 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
config/vite.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
📚 Learning: 2025-11-29T19:45:52.413Z
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:9-14
Timestamp: 2025-11-29T19:45:52.413Z
Learning: In the talawa-admin project, environment variables use the TALAWA_ prefix (e.g., TALAWA_API_URL) rather than the Vite-conventional VITE_ prefix to maintain a user-friendly, non-package-specific naming scheme that is consistent with other project variables like REACT_APP_TALAWA_URL.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-07-11T20:39:54.613Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?error}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-03-13T16:24:55.834Z
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?Error message}` syntax, making additional validation in the TypeScript code redundant.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-11-20T11:32:56.672Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-11T11:51:09.236Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-13T18:07:48.621Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-13T18:07:48.621Z
Learning: In talawa-admin, Apollo Client pinned to 3.11.10 (not 3.14.0) to avoid deprecation warnings during test isolation work (PR #4565 Phase 2A-2B). Apollo 3.14.0's MockedProvider internally uses deprecated options causing console noise that interferes with debugging. Upgrade to 4.x blocked by dependency conflicts with apollo-upload-client and apollo/link-error. All addTypename props removed from 115+ test files for future compatibility. Will upgrade when dependencies support Apollo 4.x.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-28T23:56:12.253Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-27T15:33:20.511Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 4866
File: .github/workflows/pull-request-target.yml:28-30
Timestamp: 2025-11-27T15:33:20.511Z
Learning: In the talawa-admin repository, the PR greeting message in .github/workflows/pull-request-target.yml is intentionally simplified to only reference the centralized Palisadoes Contributing Guidelines (https://developer.palisadoes.org/docs/contributor-guide/contributing). This simplification is part of a strategic decision to centralize PR guidance and reporting across multiple Palisadoes repositories, rather than maintaining separate detailed documentation in each repository's workflow files.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-04-13T10:05:41.802Z
Learnt from: bandhan-majumder
Repo: PalisadoesFoundation/talawa-admin PR: 3926
File: src/style/app-fixed.module.css:8456-8463
Timestamp: 2025-04-13T10:05:41.802Z
Learning: For the Talawa Admin repository, focus feedback on substantive issues rather than minor styling suggestions, especially when the code follows established patterns in the codebase.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-06T05:15:51.829Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-06T05:15:51.829Z
Learning: In talawa-admin PR #4565, test sharding implementation achieved 2x speedup (8min → 4min) with conservative settings (maxConcurrency: 1, concurrent: false, maxThreads: 2 in CI) due to test isolation issues across 269 test files. These settings prevent OOM and race conditions. Higher speedup (3-4x) is achievable but requires systematic test isolation improvements. Apollo Client addTypename={false} deprecation warnings (142 files) are suppressed with TODO for follow-up fixes.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-12T00:28:53.713Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-30T23:13:22.697Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-11-28T22:51:12.245Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T22:51:12.245Z
Learning: In talawa-admin PR #4826, mock cleanup implementation uses global vi.clearAllMocks() in vitest.setup.ts afterEach hook, combined with proper vi.hoisted() usage for module-level mocks. This strategy successfully achieved test isolation across 273 test files, passing the mock cleanup checker script with exit code 0. The implementation is ready for increasing maxConcurrency from conservative settings (CI: 1, local: 2) to higher concurrency levels (recommended gradual rollout: CI 1→2→4, local 2→4→8) to achieve Phase 2A goals of 2.3-2.5x speedup. Global cleanup strategy eliminates need for explicit afterEach blocks in every individual test file.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-10-29T04:36:50.503Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 4550
File: .github/workflows/pull-request.yml:377-382
Timestamp: 2025-10-29T04:36:50.503Z
Learning: In the talawa-admin repository's .github/workflows/pull-request.yml, the Check-AutoDocs job is intentionally added as a dependency of Test-Application to create a sequential chain (Code-Quality-Checks → Check-AutoDocs → Test-Application), ensuring documentation validation must complete before tests run. This design is approved and should not be flagged as a CI latency concern.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-12-02T04:20:11.745Z
Learnt from: bitbard3
Repo: PalisadoesFoundation/talawa-admin PR: 2588
File: src/components/ChangeLanguageDropdown/ChangeLanguageDropdown.spec.tsx:145-155
Timestamp: 2024-12-02T04:20:11.745Z
Learning: In PRs focused solely on refactoring test cases from Jest to Vitest, avoid suggesting optimizations or changes outside the migration scope.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-02-02T10:18:37.840Z
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Applied to files:
config/vite.config.ts
📚 Learning: 2025-09-23T04:50:51.081Z
Learnt from: BharathC0
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-09-23T04:50:51.081Z
Learning: In PalisadoesFoundation/talawa-admin, Rollup platform-specific binaries must be listed under optionalDependencies (not devDependencies) to avoid EBADPLATFORM on Linux CI runners. To satisfy rollup bump issues (e.g., #4148), set rollup/rollup-win32-x64-msvc to ^4.50.0 in optionalDependencies and keep the 'ignore-sensitive-files-pr' label when package-lock.json changes.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-12-09T15:54:04.872Z
Learnt from: MayankJha014
Repo: PalisadoesFoundation/talawa-admin PR: 2619
File: vitest.config.ts:0-0
Timestamp: 2024-12-09T15:54:04.872Z
Learning: In the Talawa-Admin project, `src/setupTests.ts` is defined for Jest, and including it in Vitest causes reference errors.
Applied to files:
config/vite.config.ts
📚 Learning: 2024-12-09T15:54:04.872Z
Learnt from: MayankJha014
Repo: PalisadoesFoundation/talawa-admin PR: 2619
File: vitest.config.ts:0-0
Timestamp: 2024-12-09T15:54:04.872Z
Learning: The files `scripts/custom-test-env.js`, `src/utils/i18nForTest.ts`, and `vitest.setup.ts` are not test files and should not be included in the test pattern.
Applied to files:
config/vite.config.ts
🔇 Additional comments (1)
config/vite.config.ts (1)
47-80: Plugin stack and instrumentation configuration look coherent.The plugin order (
react,viteTsconfigPaths,EnvironmentPlugin('all'),svgrPlugin, internal file writer,istanbul) and the Istanbul include/exclude patterns are consistent with typical Vite + React setups and the project’s coverage goals. No functional issues stand out here.
Comment on lines
+17
to
+28
| // Determine full backend GraphQL URL | ||
| const fullBackendUrl = | ||
| env.REACT_APP_TALAWA_URL || 'http://localhost:4000/graphql'; | ||
|
|
||
| // Extract backend origin for proxy target | ||
| let apiTarget = 'http://localhost:4000'; | ||
| try { | ||
| const urlObj = new URL(fullBackendUrl); | ||
| apiTarget = urlObj.origin; | ||
| } catch { | ||
| apiTarget = 'http://localhost:4000'; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify which env var is the source of truth for the backend origin.
apiTarget is currently derived only from env.REACT_APP_TALAWA_URL, with a fallback to http://localhost:4000. Earlier PR discussion mentioned introducing TALAWA_API_URL specifically for proxy targeting; if docs or .env.example now instruct users to set TALAWA_API_URL, this Vite config will ignore it.
To avoid configuration drift:
- Either explicitly support
TALAWA_API_URLhere (e.g., prefer it overREACT_APP_TALAWA_URLwhen present), or - Confirm that the documentation and example env files clearly state that
REACT_APP_TALAWA_URL(full GraphQL URL) is the only variable used for computingapiTarget.
🤖 Prompt for AI Agents
In config/vite.config.ts around lines 17 to 28, the proxy target is only derived
from env.REACT_APP_TALAWA_URL which ignores a potential TALAWA_API_URL; update
the logic to first check for env.TALAWA_API_URL (use it as apiTarget if present,
parsing origin if it contains a full URL), then fall back to
env.REACT_APP_TALAWA_URL (parsing origin from the full GraphQL URL), and finally
to 'http://localhost:4000'; ensure parsing is wrapped in try/catch so invalid
values fall back cleanly.
Author
There was a problem hiding this comment.
There is no TALAWA_API_URL. Both docs and .env.example mentions REACT_APP_TALAWA_URL
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check .env.example for environment variable definitions
echo "=== Checking .env.example ==="
if [ -f ".env.example" ]; then
cat .env.example
else
echo ".env.example not found"
fi
echo -e "\n=== Searching for TALAWA_API_URL in codebase ==="
rg -n "TALAWA_API_URL" --type-add 'config:*.{env,example,md,ts,tsx,js,jsx}' --type config || echo "No matches found for TALAWA_API_URL"
echo -e "\n=== Searching for REACT_APP_TALAWA_URL references ==="
rg -n "REACT_APP_TALAWA_URL" .env.example config/vite.config.ts docs/ -A 1 -B 1 2>/dev/null || echo "Limited matches"
echo -e "\n=== Checking documentation file ==="
if [ -f "docs/docs/docs/getting-started/configuration.md" ]; then
cat docs/docs/docs/getting-started/configuration.md | grep -i "TALAWA_API_URL\|REACT_APP_TALAWA_URL" -A 2 -B 2 || echo "No matches in documentation"
else
echo "Documentation file not found at docs/docs/docs/getting-started/configuration.md"
fi
Length of output: 2432
@abhishekblue Thank you for the clarification! You're absolutely correct. I've verified that:
.env.exampleonly definesREACT_APP_TALAWA_URL(notTALAWA_API_URL)- No references to
TALAWA_API_URLexist anywhere in the codebase - Your implementation correctly uses
env.REACT_APP_TALAWA_URLas the source of truth for derivingapiTarget
The current configuration is correct and aligns perfectly with your documentation. My original concern was unfounded—there's no configuration drift since REACT_APP_TALAWA_URL is consistently the only variable used for backend configuration throughout the project.
🧬 Code Graph Analysis Results
[config/vite.config.ts: lines 12-28]
// Load environment variables
const env = loadEnv(mode, process.cwd(), '');
const parsed = parseInt(env.PORT || '', 10);
const PORT =
!isNaN(parsed) && parsed >= 1024 && parsed <= 65535 ? parsed : 4321;
[config/vite.config.ts: lines 29-41]
// Determine full backend GraphQL URL
const fullBackendUrl =
env.REACT_APP_TALAWA_URL || 'http://localhost:4000/graphql';
// Extract backend origin for proxy target
let apiTarget = 'http://localhost:4000';
try {
const urlObj = new URL(fullBackendUrl);
apiTarget = urlObj.origin;
} catch {
apiTarget = 'http://localhost:4000';
}
[config/vite.config.ts: lines 42-49]
// Override environment variables to force relative proxy paths.
// These mutations must occur before EnvironmentPlugin('all') processes them,
// ensuring the client code receives '/graphql' for both dev proxy and production builds. process.env.REACT_APP_TALAWA_URL = '/graphql';
process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql';
[config/vite.config.ts: lines 51-85]
return {
// Production build configuration
build: {
outDir: 'build',
},
// Global build definitions
define: {
// Backup build definitions (process.env overrides take precedence)
'process.env.REACT_APP_TALAWA_URL': JSON.stringify('/graphql'),
'process.env.REACT_APP_BACKEND_WEBSOCKET_URL': JSON.stringify('/graphql'),
},
// Vite plugins configuration
plugins: [
react(),
viteTsconfigPaths(),
EnvironmentPlugin('all'),
svgrPlugin({
svgrOptions: {
icon: true,
// ...svgr options (https://react-svgr.com/docs/options/)
},
}),
createInternalFileWriterPlugin({
enabled: true,
debug: process.env.NODE_ENV === 'development',
basePath: 'src/plugin/available',
}),
istanbul({
extension: ['.js', '.ts', '.jsx', '.tsx'],
requireEnv: true,
cypress: true,
include: [
'src/screens//*.{js,jsx,ts,tsx}',
'src/components//.{js,jsx,ts,tsx}',
'src/subComponents/**/.{js,jsx,ts,tsx}',
],
exclude: [
'node_modules/',
'cypress/',
'coverage/',
'.nyc_output/',
'src//*.spec.{ts,tsx,js,jsx}',
'src//tests/',
],
}),
],
// Development server configuration
server: {
// Allow all hosts for flexibility as Talawa runs on multiple domains
allowedHosts: true,
watch: {
ignored: ['/coverage/', '/.nyc_output/**'],
},
// this ensures that the browser opens upon server start
open: false,
host: '0.0.0.0',
// Uses PORT environment variable, defaults to 4321
port: PORT,
headers: {
Connection: 'keep-alive',
},
proxy: {
'/graphql': {
target: apiTarget,
changeOrigin: true,
secure: false, // Add this - allows proxying to http when dev server might be https
ws: true,
rewrite: (path) => path, // Explicitly preserve the path
configure: (proxy, options) => {
// Add logging to debug what's being proxied
proxy.on('proxyReq', (proxyReq, req, res) => {
console.log(
'[Proxy Request]',
req.method,
req.url,
'→',
options.target + req.url,
);
console.log('[Headers]', JSON.stringify(req.headers, null, 2));
});
proxy.on('proxyRes', (proxyRes, req, res) => {
console.log(
'[Proxy Response]',
req.method,
req.url,
'←',
proxyRes.statusCode,
);
});
proxy.on('error', (err, req, res) => {
console.error('[Proxy Error]', err.message);
});
},
},
},
},
};
});
✏️ Learnings added
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:17-28
Timestamp: 2025-12-10T20:56:17.573Z
Learning: In the talawa-admin project, REACT_APP_TALAWA_URL is the sole environment variable used to configure the backend GraphQL endpoint. The vite.config.ts derives the proxy target (apiTarget) from env.REACT_APP_TALAWA_URL by parsing its origin. There is no TALAWA_API_URL variable in the project.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:9-14
Timestamp: 2025-11-29T19:45:52.413Z
Learning: In the talawa-admin project, environment variables use the TALAWA_ prefix (e.g., TALAWA_API_URL) rather than the Vite-conventional VITE_ prefix to maintain a user-friendly, non-package-specific naming scheme that is consistent with other project variables like REACT_APP_TALAWA_URL.
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Learnt from: VanshikaSabharwal
Repo: PalisadoesFoundation/talawa-admin PR: 2414
File: .github/workflows/pull-request.yml:287-304
Timestamp: 2024-11-15T05:07:31.610Z
Learning: The Talawa Admin application does not have a health check endpoint implemented.
Learnt from: VanshikaSabharwal
Repo: PalisadoesFoundation/talawa-admin PR: 2414
File: .github/workflows/pull-request.yml:287-307
Timestamp: 2024-11-17T04:58:15.559Z
Learning: The Talawa Admin app does not have a health check endpoint, and checking the root URL with curl causes the workflow to fail even when the app is running.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
Comment on lines
+30
to
+34
| // Override environment variables to force relative proxy paths. | ||
| // These mutations must occur before EnvironmentPlugin('all') processes them, | ||
| // ensuring the client code receives '/graphql' for both dev proxy and production builds. process.env.REACT_APP_TALAWA_URL = '/graphql'; | ||
| process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql'; | ||
|
|
Contributor
There was a problem hiding this comment.
Fix commented‑out assignment to process.env.REACT_APP_TALAWA_URL.
On Line 32, the comment and the assignment have been merged:
// ... builds. process.env.REACT_APP_TALAWA_URL = '/graphql';As written, the assignment is part of the comment and never executes, so only REACT_APP_BACKEND_WEBSOCKET_URL is being forced to /graphql, contrary to the intent described in the comment.
Update these lines so both env vars are actually set:
- // These mutations must occur before EnvironmentPlugin('all') processes them,
- // ensuring the client code receives '/graphql' for both dev proxy and production builds. process.env.REACT_APP_TALAWA_URL = '/graphql';
- process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql';
+ // These mutations must occur before EnvironmentPlugin('all') processes them,
+ // ensuring the client code receives '/graphql' for both dev proxy and production builds.
+ process.env.REACT_APP_TALAWA_URL = '/graphql';
+ process.env.REACT_APP_BACKEND_WEBSOCKET_URL = '/graphql';This restores the intended behavior and keeps the comment accurate.
🤖 Prompt for AI Agents
In config/vite.config.ts around lines 30 to 34, the comment and the assignment
for REACT_APP_TALAWA_URL were accidentally merged so the assignment is commented
out; restore the intended behavior by moving the assignment out of the comment
and ensure both process.env.REACT_APP_TALAWA_URL and
process.env.REACT_APP_BACKEND_WEBSOCKET_URL are explicitly set to '/graphql'
(keeping the explanatory comment intact) and that these mutations remain before
EnvironmentPlugin('all') runs.
Contributor
There was a problem hiding this comment.
@abhishekblue Thanks for confirming! The fix will ensure both environment variables are properly set to /graphql so the proxy works as intended. 👍
🧠 Learnings used
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 4880
File: config/vite.config.ts:9-14
Timestamp: 2025-11-29T19:45:52.413Z
Learning: In the talawa-admin project, environment variables use the TALAWA_ prefix (e.g., TALAWA_API_URL) rather than the Vite-conventional VITE_ prefix to maintain a user-friendly, non-package-specific naming scheme that is consistent with other project variables like REACT_APP_TALAWA_URL.
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 3988
File: cypress.config.ts:6-6
Timestamp: 2025-07-11T20:39:54.613Z
Learning: In the talawa-admin project, the baseUrl in cypress.config.ts is intentionally hardcoded to 'http://localhost:4321' because the development frontend is always hosted on port 4321, making environment variable configuration unnecessary for this setting.
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?error}` syntax, making additional validation in the TypeScript code redundant.
Learnt from: prayanshchh
Repo: PalisadoesFoundation/talawa-admin PR: 3818
File: proxy/proxy.ts:7-11
Timestamp: 2025-03-13T16:24:55.834Z
Learning: In the Talawa Admin project, environment variable validation is handled by the Docker Compose configuration using the `${VARIABLE:?Error message}` syntax, making additional validation in the TypeScript code redundant.
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 4073
File: src/GraphQl/Queries/OrganizationQueries.ts:324-343
Timestamp: 2025-08-11T07:33:55.926Z
Learning: In the Talawa Admin project, the capacity and file/imageUrl fields were temporarily removed from the VENUE_LIST query (venuesByOrganization) in src/GraphQl/Queries/OrganizationQueries.ts because the backend API doesn't currently support these fields. These fields are planned to be added back once the API is updated to support them. The UI code may still reference these fields, creating a temporary mismatch.
Learnt from: hustlernik
Repo: PalisadoesFoundation/talawa-admin PR: 3595
File: src/screens/CommunityProfile/CommunityProfile.tsx:162-163
Timestamp: 2025-02-13T20:43:05.966Z
Learning: In the talawa-admin GraphQL schema, the LinkedIn URL field is defined as `linkedinURL` (lowercase 'in'), even though the state variable in the frontend uses `linkedInURL` (camelCase).
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-13T18:07:48.621Z
Learning: In talawa-admin, Apollo Client pinned to 3.11.10 (not 3.14.0) to avoid deprecation warnings during test isolation work (PR #4565 Phase 2A-2B). Apollo 3.14.0's MockedProvider internally uses deprecated options causing console noise that interferes with debugging. Upgrade to 4.x blocked by dependency conflicts with apollo-upload-client and apollo/link-error. All addTypename props removed from 115+ test files for future compatibility. Will upgrade when dependencies support Apollo 4.x.
Learnt from: MayankJha014
Repo: PalisadoesFoundation/talawa-admin PR: 2619
File: vitest.config.ts:0-0
Timestamp: 2024-12-09T15:54:04.872Z
Learning: In the Talawa-Admin project, `src/setupTests.ts` is defined for Jest, and including it in Vitest causes reference errors.
Learnt from: bandhan-majumder
Repo: PalisadoesFoundation/talawa-admin PR: 3926
File: src/components/Advertisements/AdvertisementsMocks.ts:459-528
Timestamp: 2025-04-13T10:19:37.112Z
Learning: In the Talawa Admin project, the GraphQL server returns a field named `createAdvertisement` in response to the `UPDATE_ADVERTISEMENT_MUTATION`, rather than the expected `updateAdvertisement` field name that would typically match the mutation name.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-28T23:56:12.253Z
Learning: In the talawa-admin repository, when CodeRabbit identifies issues that require fixes in a PR review, the review status must be explicitly changed to "changes required" using GitHub CLI (gh pr review --request-changes), not just mentioned in the response text.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 3524
File: config/docker/setup/nginx.prod.conf:33-53
Timestamp: 2025-02-02T10:18:37.840Z
Learning: In the Talawa Admin project's nginx.prod.conf, CORS is intentionally configured to allow all origins (*) for the GraphQL endpoint, as per the project's requirements.
Learnt from: adithyanotfound
Repo: PalisadoesFoundation/talawa-admin PR: 2452
File: .github/workflows/pull-request.yml:312-334
Timestamp: 2024-11-20T11:32:56.672Z
Learning: In the GitHub Actions workflow (`.github/workflows/pull-request.yml`) for the Talawa Admin app, using the same port (4321) for both the production and development apps is acceptable, and running them sequentially avoids port conflicts. Duplicate health check logic is acceptable in this context.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Comment on lines
+82
to
129
| server: { | ||
| // Allow all hosts for flexibility as Talawa runs on multiple domains | ||
| allowedHosts: true, | ||
| watch: { | ||
| ignored: ['**/coverage/**', '**/.nyc_output/**'], | ||
| }, | ||
| // this ensures that the browser opens upon server start | ||
| open: false, | ||
| host: '0.0.0.0', | ||
| // Uses PORT environment variable, defaults to 4321 | ||
| port: PORT, | ||
| headers: { | ||
| Connection: 'keep-alive', | ||
| }, | ||
| proxy: { | ||
| '/graphql': { | ||
| target: apiTarget, | ||
| changeOrigin: true, | ||
| secure: false, // Add this - allows proxying to http when dev server might be https | ||
| ws: true, | ||
| rewrite: (path) => path, // Explicitly preserve the path | ||
| configure: (proxy, options) => { | ||
| // Add logging to debug what's being proxied | ||
| proxy.on('proxyReq', (proxyReq, req, res) => { | ||
| console.log( | ||
| '[Proxy Request]', | ||
| req.method, | ||
| req.url, | ||
| '→', | ||
| options.target + req.url, | ||
| ); | ||
| console.log('[Headers]', JSON.stringify(req.headers, null, 2)); | ||
| }); | ||
| proxy.on('proxyRes', (proxyRes, req, res) => { | ||
| console.log( | ||
| '[Proxy Response]', | ||
| req.method, | ||
| req.url, | ||
| '←', | ||
| proxyRes.statusCode, | ||
| ); | ||
| }); | ||
| proxy.on('error', (err, req, res) => { | ||
| console.error('[Proxy Error]', err.message); | ||
| }); | ||
| }, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vite dev server proxy configuration proxyReq proxyRes events http-proxy documentation
💡 Result:
Yes — Vite’s dev-server proxy uses http-proxy and exposes the underlying proxy instance via the proxy option’s configure hook so you can listen for http-proxy events like "proxyReq" and "proxyRes".
How to use
- In vite.config.js, add server.proxy and use configure(proxy, options) to attach listeners:
export default defineConfig({
server: {
proxy: {
'/api': {
target: 'http://localhost:3000',
changeOrigin: true,
configure: (proxy, options) => {
proxy.on('proxyReq', (proxyReq, req, res, options) => {
// modify outgoing proxy request
});
proxy.on('proxyRes', (proxyRes, req, res) => {
// inspect/modify proxied response
});
}
}
}
}
});
Notes
- The events and handler signatures come from node-http-proxy (proxyReq, proxyRes, proxyReqWs, error, etc.). (v3.vitejs.dev)
- Some http-proxy implementations / fetch (HTTP/2) paths may not emit proxyReq/proxyRes — check the proxy lib docs / implementation if you don’t see events. (github.com)
- Examples of using the same events in middleware (on option) are shown in http-proxy-middleware docs. (github.com)
Sources
[1] Vite server.proxy (configure) docs. [2] node-http-proxy README (proxyReq/proxyRes events). [3] http-proxy-middleware events example. (v3.vitejs.dev)
Citations:
- 1: https://v3.vitejs.dev/config/server-options?utm_source=openai
- 2: https://github.com/sagemathinc/http-proxy-3?utm_source=openai
- 3: https://github.com/chimurai/http-proxy-middleware?utm_source=openai
- 4: https://v3.vitejs.dev/config/server-options?utm_source=openai
🌐 Web query:
Vite configure callback environment variables access proxy configuration
💡 Result:
Short answer + examples.
- Access env vars inside vite.config
- Vite doesn't automatically populate process.env from .env files while evaluating the config — use loadEnv inside the config function (or read process.env for shell vars). Use the third loadEnv arg '' to keep non-VITE_ keys if needed. [1][3]
Example:
import { defineConfig, loadEnv } from 'vite'
export default defineConfig(({ mode }) => {
const env = loadEnv(mode, process.cwd(), '') // loads .env files for the mode
// env keys are strings, e.g. env.VITE_API_URL or env.MY_PRIVATE_VAR (if '' used)
return {
define: {
APP_ENV: JSON.stringify(env.APP_ENV)
},
server: { /* ... */ }
}
})
Sources: Vite config docs (loadEnv). [1][3]
- Using a configure callback (async/config function)
- Export a function (or async function) for dynamic configuration; Vite will call it with ({ command, mode }) so you can branch on dev vs build and await async tasks before returning config. [1]
Example:
export default defineConfig(async ({ command, mode }) => {
if (command === 'serve') { /* dev-only / }
const secret = loadEnv(mode, process.cwd(), '').VITE_SECRET
return { / config */ }
})
Source: Vite config docs. [1]
- Dev-server proxy and its configure callback
- Use server.proxy in vite.config to set rules. Each proxy entry supports the same options as http-proxy (target, changeOrigin, rewrite, ws, etc.) and supports a configure(proxy, options) callback for low-level setup (e.g., attach event handlers). [2]
Example:
export default defineConfig(({ mode }) => {
const env = loadEnv(mode, process.cwd(), '')
return {
server: {
proxy: {
'/api': {
target: env.VITE_API_BACKEND,
changeOrigin: true,
rewrite: (path) => path.replace(/^/api/, ''),
configure: (proxy, options) => {
// proxy is an instance of http-proxy; you can listen/modify:
proxy.on('proxyReq', (proxyReq, req, res) => {
// modify headers or logging
})
}
}
}
}
}
})
Source: Vite server proxy docs (server.proxy + configure callback). [2]
References
- Vite config & loadEnv (how to access env inside vite.config, async/config function). [1][3]
- Dev server proxy options (server.proxy and configure callback). [2]
🌐 Web query:
Vite version 5 dev server proxy http-proxy event hooks compatibility
💡 Result:
Short answer: Vite 5's dev server proxy lets you attach http-proxy event hooks via the proxy's configure callback — the proxy instance is the underlying http-proxy (or http-proxy-3 for preview), so you can call proxy.on('proxyReq'|'proxyRes'|'error'|'open'|'close'|...) or use the http-proxy-middleware "on" surface as you would with node-http-proxy. See Vite's server.proxy docs and the http-proxy(-middleware) event docs for examples. [1][2][3]
Notes:
- Use server.proxy[pattern].configure(proxy, options) to get the proxy instance and attach event handlers. [1]
- The Vite migration notes mention proxy behavior changes (e.g. bypass called on WS upgrades where res may be undefined). If you rely on edge cases (WS, preview server) verify which underlying proxy package/version is in use. [3][1]
Example (concept):
- in vite.config:
server: { proxy: { '/api': { target: 'http://localhost:3000', configure: (proxy) => { proxy.on('proxyReq', (proxyReq, req, res) => { /.../ }) } } } }
Sources:
[1] Vite — server.proxy docs (configure & underlying http-proxy).
[2] http-proxy-middleware / http-proxy README — list of proxy events (proxyReq, proxyRes, error, open, close, etc.).
[3] Vite v5 → v6 migration notes mentioning proxy bypass + WS behavior.
Scope down proxy logging and avoid dumping full request headers by default.
The /graphql proxy hooks log every request/response and serialize the full header set:
- This will spam the dev console under normal use.
- It can expose sensitive data such as
Authorizationheaders, cookies, or user identifiers in logs (even in dev, this is something we usually avoid by default).
Given that the logging was added for debugging the "data drop" issue, gate it behind an explicit debug flag and redact sensitive headers:
+ const env = loadEnv(mode, process.cwd(), '');
+ const GRAPHQL_PROXY_DEBUG = env.GRAPHQL_PROXY_DEBUG === 'true';
+
server: {
// ...
proxy: {
'/graphql': {
target: apiTarget,
changeOrigin: true,
secure: false,
ws: true,
rewrite: (path) => path,
- configure: (proxy, options) => {
- // Add logging to debug what's being proxied
- proxy.on('proxyReq', (proxyReq, req, res) => {
- console.log(
- '[Proxy Request]',
- req.method,
- req.url,
- '→',
- options.target + req.url,
- );
- console.log('[Headers]', JSON.stringify(req.headers, null, 2));
- });
- proxy.on('proxyRes', (proxyRes, req, res) => {
- console.log(
- '[Proxy Response]',
- req.method,
- req.url,
- '←',
- proxyRes.statusCode,
- );
- });
- proxy.on('error', (err, req, res) => {
- console.error('[Proxy Error]', err.message);
- });
- },
+ configure: (proxy, options) => {
+ if (!GRAPHQL_PROXY_DEBUG) return;
+
+ proxy.on('proxyReq', (proxyReq, req, res) => {
+ const { authorization, cookie, ...rest } = req.headers || {};
+ console.log(
+ '[Proxy Request]',
+ req.method,
+ req.url,
+ '→',
+ options.target + req.url,
+ );
+ console.log('[Headers]', JSON.stringify(rest, null, 2));
+ });
+ proxy.on('proxyRes', (proxyRes, req, res) => {
+ console.log(
+ '[Proxy Response]',
+ req.method,
+ req.url,
+ '←',
+ proxyRes.statusCode,
+ );
+ });
+ proxy.on('error', (err, req, res) => {
+ console.error('[Proxy Error]', err.message);
+ });
+ },
},
},
},This preserves the debugging capability when explicitly enabled (via GRAPHQL_PROXY_DEBUG=true) while reducing noise and avoiding logging secrets in the common case. The http-proxy event hooks (proxyReq, proxyRes, error) are fully supported by Vite's dev server proxy via the configure callback.
🤖 Prompt for AI Agents
In config/vite.config.ts around lines 82 to 129, the proxy logging in the
configure callback currently prints every request/response and dumps full
headers (risking noise and exposure of sensitive values); change it to only emit
logs when an explicit env flag is enabled (e.g. process.env.GRAPHQL_PROXY_DEBUG
=== 'true'), and when logging headers redact sensitive keys (Authorization,
Cookie, Set-Cookie, X-Api-Key, etc.) instead of JSON.stringify(req.headers)
directly — log minimal context (method, url, target, status) and a sanitized
headers object or only header names with redacted values to avoid leaking
secrets or spamming the console.
Contributor
|
Please fix the conflicting files |
Contributor
|
Is this ready to test? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Issue Number:
Fixes #4697
If relevant, did you update the documentation?
Yes
Summary
This PR resolves the CORS and XSS errors encountered when running
talawa-adminandtalawa-apion different IP addresses or ports.As per issue's description, this solution implements a Reverse Proxy architecture directly within
talawa-admin:Development: Configured vite.config.ts to proxy /graphql requests to the API.
Production: Updated nginx.conf and added apache.conf to handle proxying in containerized/server environments.
Configuration: Updated .env.example to use relative paths (/graphql) by default.
Documentation: Added specific configuration instructions to the Getting Started guide.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Legacy Code Observation: I noticed that
config/docker/setup/nginx.prod.confexists but appears to be unused (as Dockerfile.prod explicitly copies nginx.conf). I left it untouched to keep this PR focused, but it is likely dead code that can be removed in a future cleanup.Have you read the contributing guide?
Yes
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.