NoMongo:Fixing the 404 error even after having a running talawa-api #3266
Conversation
WalkthroughThe pull request modifies the Changes
Suggested Reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/setup/askForTalawaApiUrl/askForTalawaApiUrl.ts (1)
8-9: LGTM! Good UX improvement to prevent configuration errors.The changes effectively address the root cause of the 404 issue by:
- Explicitly warning users about trailing slashes in the prompt
- Setting a correct default value without a trailing slash
This is a simple but effective solution that will help prevent configuration errors.
However, we could further improve the user experience with input validation.
Consider adding validation to automatically trim any trailing slashes from the user input:
export async function askForTalawaApiUrl(): Promise<string> { const { endpoint } = await inquirer.prompt<{ endpoint: string }>([ { type: 'input', name: 'endpoint', message: 'Enter your talawa-api endpoint:(omit the trailing "/")', default: 'http://localhost:4000/graphql', + validate: (input: string) => { + return true; + }, + filter: (input: string) => input.replace(/\/+$/, ''), }, ]); return endpoint; }This would ensure that even if users accidentally include trailing slashes, they will be automatically removed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/setup/askForTalawaApiUrl/askForTalawaApiUrl.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
palisadoes
changed the title
|
Make the code trim the trailing slash instead. The user experience must be improved, not just the messaging |
|
|
I think there wont be any issue with graphql after this . Will have to check db operations and authorization part if both are fine then it should work |
experience
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3266 +/- ##
===================================================
- Coverage 8.49% 8.48% -0.01%
===================================================
Files 309 309
Lines 7832 7834 +2
Branches 1730 1731 +1
===================================================
Hits 665 665
- Misses 7093 7095 +2
Partials 74 74
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/setup/askForDocker/askForDocker.ts (1)
50-52: LGTM! Consider enhancing the trailing slash handling.The implementation successfully addresses the 404 error issue. However, consider making it more robust:
- if (endpoint.endsWith('/')) { - endpoint = endpoint.slice(0, -1); - } + // Normalize the endpoint URL by removing trailing slashes to prevent 404 errors + endpoint = endpoint.replace(/\/+$/, '');This enhancement:
- Handles multiple consecutive trailing slashes
- Uses a more idiomatic approach with regex
- Adds a descriptive comment explaining the purpose
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/setup/askForDocker/askForDocker.ts(1 hunks)src/setup/askForTalawaApiUrl/askForTalawaApiUrl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/setup/askForTalawaApiUrl/askForTalawaApiUrl.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (1)
src/setup/askForDocker/askForDocker.ts (1)
50-52: Verify URL transformations after trailing slash removal.Please ensure that WebSocket and Docker URL transformations work correctly with the modified endpoint. Test the following scenarios:
Test cases to verify:
- HTTP → WS transformation:
http://localhost:4000/graphql→ws://localhost:4000/graphql- HTTPS → WSS transformation:
https://localhost:4000/graphql→wss://localhost:4000/graphql- Docker URL transformation:
http://localhost:4000/graphql→http://host.docker.internal:4000/graphql✅ Verification successful
URL transformations are correctly ordered and safe ✅
The trailing slash removal is performed before URL validation and transformations, ensuring consistent behavior across all URL operations:
- URL validation occurs immediately after slash removal
- WebSocket protocol transformation only affects the protocol part
- Docker URL transformation only replaces the hostname
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify URL handling patterns in the codebase # Check for other URL transformations that might be affected echo "Checking for URL transformation patterns..." rg -n "\.replace\(.*http.*\)" --type ts # Check for other endpoint URL handling echo "Checking for endpoint URL handling..." rg -n "endpoint.*endsWith|endpoint.*slice|endpoint.*replace" --type ts # Check for URL validation patterns echo "Checking for URL validation..." rg -n "new URL\(" --type tsLength of output: 2002
|
@Nikhilh26 the title of the pr should reflect the original issue to avoid confusion. Does this solve the issue with login? |
|
The issue with login is not only in admin but in API as well . What this PR resolves is the thing that even after having a running |
|
all this information can be inferred from the environment variables defined in |
|
What changes are therefore required in this PR? Would Admin would still need to trim the / ? |
Probably nothing to do with the leading slash Edit: related to this probably: https://fastify.dev/docs/latest/Reference/Server/#ignoretrailingslash |
|
@xoldd What is the URL for GraphQL vs iGraphQL in the postgres branch? |
|
|
|
Closing. You are not assigned this issue |
What kind of change does this PR introduce?
Bug fix
Issue Number: #3236
Fixes # #3236
Snapshots/Videos:
Earlier:
After making changes there were no erros
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit