Added scripts for REACT_APP_BACKEND_WEBSOCKET_URL #2350
Conversation
WalkthroughThe pull request introduces modifications to three files: Changes
Assessment against linked issues
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 ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2350 +/- ##
========================================
Coverage 98.28% 98.28%
========================================
Files 283 283
Lines 8224 8224
Branches 2356 2384 +28
========================================
Hits 8083 8083
Misses 132 132
Partials 9 9 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.ts (1)
3-10: LGTM with a suggestion for improvement.The
isValidWebSocketUrlfunction correctly validates WebSocket URLs by checking the protocol. However, consider adding a check for the presence of a host to ensure more complete URL validation.Consider modifying the function as follows:
function isValidWebSocketUrl(url: string): boolean { try { const parsedUrl = new URL(url); return (parsedUrl.protocol === 'ws:' || parsedUrl.protocol === 'wss:') && !!parsedUrl.host; } catch { return false; } }This change ensures that the URL has both a valid protocol and a host.
src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.test.ts (3)
60-78: Consider removing redundant test case.This test case appears to be redundant as it tests the same behavior as the second test case ("should return the default endpoint when the user does not enter anything"). Consider removing this test to avoid duplication and maintain a more concise test suite.
80-98: LGTM: Good test for unexpected input. Consider adding input validation.This test case effectively verifies that the function doesn't fail on unexpected input types, which is good for robustness. However, consider adding a validation check in the main
askForTalawaWebsocketUrlfunction to ensure the input is a valid WebSocket URL format before returning it.In the main
askForTalawaWebsocketUrlfunction, you could add a validation like this:function isValidWebSocketUrl(url: string): boolean { try { new URL(url); return url.startsWith('ws://') || url.startsWith('wss://'); } catch { return false; } } // In the main function if (!isValidWebSocketUrl(endpoint)) { throw new Error('Invalid WebSocket URL format'); }This would ensure that only valid WebSocket URLs are accepted.
1-99: Overall good test coverage. Consider these improvements:The test suite provides comprehensive coverage of the
askForTalawaWebsocketUrlfunction's behavior, including valid input, default values, error handling, and unexpected input. The tests are well-structured and consistent. To further improve the suite:
- Remove the redundant test case as mentioned earlier.
- Consider adding more edge cases, such as:
- Testing with a valid
wss://URL- Testing with an invalid URL format
- Testing with extremely long input
These additions would enhance the robustness of your test suite.
INSTALLATION.md (2)
170-176: LGTM: New section for REACT_APP_BACKEND_WEBSOCKET_URL addedThe new section provides clear instructions for setting up the
REACT_APP_BACKEND_WEBSOCKET_URLenvironment variable, including examples for both local development and remote access scenarios. The environment variables table has also been updated to include this new variable.Consider adding a brief explanation of why the WebSocket URL is necessary for the application, similar to how you've explained the purpose of other environment variables. This would help users understand its importance.
Also applies to: 204-224
69-88: Formatting improvements and minor correctionsThe formatting changes in the repository setup instructions improve readability. However, there are a few minor issues to address:
- Line 70: Consider hyphenating "Open Source Contributor" as it's a compound adjective modifying "Software Developers".
- Line 116: Remove the repeated word "Exit".
- Line 119: Change "MacOS" to "macOS" for correct capitalization.
Please apply the following changes:
- 1. An easy way to do this is to right-click and choose appropriate option based on your OS. + 1. An easy way to do this is to right-click and choose the appropriate option based on your OS. -3. **For Our Open Source Contributor Software Developers:** +3. **For Our Open-Source Contributor Software Developers:** - 6. Exit `notepad` - 7. Exit PowerShell + 6. Exit `notepad` + 7. Exit PowerShell -2. For Linux and MacOS, use the terminal window. +2. For Linux and macOS, use the terminal window.Also applies to: 70-70, 116-116, 119-119
🧰 Tools
🪛 LanguageTool
[uncategorized] ~70-~70: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e option based on your OS. 3. For Our Open Source Contributor Software Developers: ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- .env.example (1 hunks)
- INSTALLATION.md (5 hunks)
- setup.ts (2 hunks)
- src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.test.ts (1 hunks)
- src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.ts (1 hunks)
🧰 Additional context used
🪛 LanguageTool
INSTALLATION.md
[uncategorized] ~70-~70: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e option based on your OS. 3. For Our Open Source Contributor Software Developers: ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[duplication] ~116-~116: Possible typo: you repeated a word
Context: ... 5. Save the document. 6. Exitnotepad7. Exit PowerShell 8. This will ensure...(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~119-~119: The operating system from Apple is written “macOS”.
Context: ...t version ofnode.js2. For Linux and MacOS, use the terminal window. 1. install...(MAC_OS)
[grammar] ~123-~123: The word “setup” is a noun. The verb is spelled with a white space.
Context: ... the respective recommended commands to setup your node environment 3. This wil...(NOUN_VERB_CONFUSION)
🪛 Markdownlint
INSTALLATION.md
95-95: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
105-105: null
Bare URL used(MD034, no-bare-urls)
178-178: Expected: #setting-up-react_app_talawa_url-in-env-file; Actual: #setting-up-REACT_APP_TALAWA_URL-in-env-file
Link fragments should be valid(MD051, link-fragments)
178-178: Expected: #setting-up-react_app_recaptcha_site_key-in-env-file; Actual: #setting-up-REACT_APP_RECAPTCHA_SITE_KEY-in-env-file
Link fragments should be valid(MD051, link-fragments)
208-208: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
214-214: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
220-220: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
🔇 Additional comments (9)
src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.ts (2)
1-1: LGTM: Import statement is correct.The import of the
inquirerlibrary is appropriate for the functionality implemented in this file.
1-32: Overall, well-implemented functionality with minor improvements suggested.This new file successfully implements the functionality to prompt for and validate WebSocket URLs, aligning with the PR objective of configuring the
REACT_APP_BACKEND_WEBSOCKET_URL. The code is well-structured and follows TypeScript best practices. With the suggested minor improvements, this implementation will provide a robust solution for setting up the WebSocket URL..env.example (1)
27-27: Approve the removal of the default WebSocket URL value.The change to remove the default value for
REACT_APP_BACKEND_WEBSOCKET_URLis appropriate. It aligns with the following best practices:
- Security: It prevents accidental use of a default WebSocket URL in production environments.
- Flexibility: It allows users to set their own WebSocket URL based on their specific setup.
- Consistency: It matches the pattern of other variables like
REACT_APP_TALAWA_URLin this file.However, to ensure a smooth user experience:
- Verify that the
INSTALLATION.mdfile has been updated with clear instructions on how to set this variable.- Confirm that the
setup.tsscript now prompts users to input this value during the setup process.These complementary changes will help users understand how to properly configure this crucial variable for plugin functionality.
To verify the associated documentation and setup script changes, please run:
✅ Verification successful
Approval Confirmed for Removal of Default WebSocket URL Value
The removal of the default value for
REACT_APP_BACKEND_WEBSOCKET_URLaligns with best practices:
- Security: Prevents accidental use of a default WebSocket URL in production environments.
- Flexibility: Allows users to set their own WebSocket URL based on their specific setup.
- Consistency: Matches the pattern of other variables like
REACT_APP_TALAWA_URLin this file.Additionally, verification confirms that:
INSTALLATION.mdhas been updated with clear instructions on setting this variable.setup.tsscript now prompts users to input this value during the setup process.These changes ensure that users can properly configure this crucial variable for plugin functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for WebSocket URL instructions in INSTALLATION.md echo "Checking INSTALLATION.md for WebSocket URL setup instructions:" rg -i "REACT_APP_BACKEND_WEBSOCKET_URL" INSTALLATION.md # Check for WebSocket URL prompt in setup.ts echo "Checking setup.ts for WebSocket URL prompt:" rg -i "REACT_APP_BACKEND_WEBSOCKET_URL" setup.tsLength of output: 1795
src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.test.ts (4)
1-11: LGTM: Imports and test setup are well-structured.The imports, mocking of the
inquirerlibrary, and the use ofbeforeEachto clear mocks follow best practices for Jest testing.
13-31: LGTM: Well-structured test for custom endpoint input.This test case effectively verifies that the function returns the user-provided endpoint and correctly calls
inquirer.promptwith the expected parameters.
33-51: LGTM: Proper testing of default endpoint behavior.This test case effectively verifies that the function returns the default endpoint when the user provides no input, and correctly calls
inquirer.promptwith the expected parameters.
53-58: LGTM: Proper error handling test.This test case effectively verifies that the function throws an error when the prompt fails, ensuring robust error handling.
INSTALLATION.md (2)
20-20: LGTM: Table of contents updated correctlyThe new entry for setting up
REACT_APP_BACKEND_WEBSOCKET_URLhas been added to the table of contents, accurately reflecting the new section in the document.
Line range hint
1-224: Overall improvements to INSTALLATION.mdThe changes to the INSTALLATION.md file are beneficial and align well with the PR objectives. Key improvements include:
- Addition of clear instructions for setting up the
REACT_APP_BACKEND_WEBSOCKET_URLenvironment variable.- Updated table of contents to reflect the new section.
- Improved formatting and readability of the repository setup instructions.
These changes address the issue mentioned in #2183 by providing documentation for the WebSocket URL setup. The instructions are consistent with the existing style and provide examples for different scenarios.
To further enhance the document:
- Consider adding a brief explanation of why the WebSocket URL is necessary for the application.
- Address the minor formatting and spelling issues mentioned in the previous comment.
Great job on improving the installation documentation!
🧰 Tools
🪛 LanguageTool
[grammar] ~67-~67: The word “setup” is a noun. The verb is spelled with a space.
Context: ...avigate to the folder where you want to setup the repository. 2. Open acmd(Window...(NOUN_VERB_CONFUSION)
[grammar] ~68-~68: The operating system from Apple is written “macOS”.
Context: ...cmd(Windows) orterminal(Linux or MacOS) session in this folder. 1. An easy ...(MAC_OS)
[uncategorized] ~70-~70: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e option based on your OS. 3. For Our Open Source Contributor Software Developers: ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Markdownlint
95-95: null
Dollar signs used before commands without showing output(MD014, commands-show-output)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- setup.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
setup.ts
[failure] 9-9:
'"./src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl"' has no exported member named 'askForTalawaWebSocketUrl'. Did you mean 'askForTalawaWebsocketUrl'?
[failure] 114-114:
Cannot find name 'askForTalawaWebsocketUrl'. Did you mean 'askForTalawaWebSocketUrl'?
🔇 Additional comments (3)
setup.ts (3)
92-107: LGTM: WebSocket URL setup checkThe implementation for checking and prompting for the WebSocket URL setup is consistent with the existing pattern in the file. It correctly handles both cases where the URL is already set and where it needs to be configured.
109-117: LGTM: WebSocket URL setup implementationThe implementation for setting up the WebSocket URL is well-structured and consistent with the existing pattern for the API URL setup. It correctly handles connection checking and retries, ensuring a valid WebSocket endpoint is obtained.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[failure] 114-114:
Cannot find name 'askForTalawaWebsocketUrl'. Did you mean 'askForTalawaWebSocketUrl'?
92-129: LGTM: Overall WebSocket URL setup implementationThe implementation for setting up the REACT_APP_BACKEND_WEBSOCKET_URL environment variable is well-structured, consistent with the existing patterns in the file, and addresses the issue mentioned in the PR objectives. It covers all necessary steps and maintains good readability.
Great job on implementing this feature! It effectively solves the problem of configuring the WebSocket URL during setup.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[failure] 114-114:
Cannot find name 'askForTalawaWebsocketUrl'. Did you mean 'askForTalawaWebSocketUrl'?
…d/talawa-admin into feat/websocketurl
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
setup.ts (2)
92-107: LGTM: WebSocket URL setup logicThe new code block for setting up the WebSocket URL is well-structured and follows the existing pattern in the file. It correctly checks for an existing value and prompts the user when needed.
Minor suggestion: For consistency, consider changing
shouldSetTalawaWebsocketUrltoshouldSetTalawaWebSocketUrl(capital 'S' in 'Socket').
109-133: LGTM: WebSocket URL configuration logicThe implementation for configuring the WebSocket URL is well-structured and includes proper validation. The addition of error handling for reading the .env file is a good improvement.
Suggestion: Consider adding error handling for writing to the .env file as well. This would make the error handling more comprehensive.
Example:
fs.writeFile('.env', result, 'utf8', (writeErr) => { if (writeErr) { console.error('Error writing to .env file:', writeErr); process.exit(1); } console.log('WebSocket URL updated successfully.'); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- setup.ts (2 hunks)
- src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.test.ts (1 hunks)
- src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.test.ts
- src/setup/askForTalawaWebSocketUrl/askForTalawaWebSocketUrl.ts
🧰 Additional context used
🔇 Additional comments (2)
setup.ts (2)
9-9: LGTM: New import for WebSocket URL setupThe import statement for
askForTalawaWebSocketUrlis correctly added and follows the existing code style. This addition aligns with the PR objective of setting up theREACT_APP_BACKEND_WEBSOCKET_URL.
Line range hint
1-134: Overall: Successful implementation of WebSocket URL configurationThe changes in this file successfully implement the setup process for the
REACT_APP_BACKEND_WEBSOCKET_URL, addressing the main objective of the PR. The new code integrates well with the existing setup logic, maintaining consistency in structure and style.Key points:
- The WebSocket URL configuration follows the same pattern as other configurations in the file.
- Proper validation and error handling have been implemented.
- The changes align with the PR objectives and the issue description.
Minor suggestions for improvement have been noted in previous comments, but overall, this implementation effectively solves the issue of setting up the WebSocket URL during the Talawa Admin setup process.
There was a problem hiding this comment.
You have not followed the need in the issue. Please fix.
The parameter must be automatically configured based on the value of REACT_APP_TALAWA_URL parameter value
There should be no need to prompt for this parameter. Let me know if you can think of other scenarios where this may apply.
adithyanotfound
force-pushed
the
feat/websocketurl
branch
from
bdd7aa0
to
6f81300
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- setup.ts (1 hunks)
🔇 Additional comments (1)
setup.ts (1)
89-94: Implementation meets the requirementsThe code successfully:
- Automatically configures the WebSocket URL based on the Talawa API URL
- Correctly transforms HTTP(S) to WS(S) protocol
- Integrates with the existing setup flow
This implementation aligns well with the requirements from issue #2183.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
INSTALLATION.md (2)
170-176: LGTM! Consider adding default values column.The environment variables table is well-structured and includes clear descriptions. Consider adding a "Default Value" column to help users understand the expected format of each variable.
| Variable | Description | -| ------------------------------- | ------------------------------------------------- | +| ------------------------------- | ------------------------------------------------- | ------------- | | PORT | Custom port for Talawa-Admin development purposes | +| | | 4321 | | REACT_APP_TALAWA_URL | URL endpoint for talawa-api graphql service | +| | | http://localhost:4000/graphql/ | | REACT_APP_BACKEND_WEBSOCKET_URL | URL endpoint for websocket end point | +| | | ws://localhost:4000/graphql/ | | REACT_APP_USE_RECAPTCHA | Whether you want to use reCAPTCHA or not | +| | | false | | REACT_APP_RECAPTCHA_SITE_KEY | Site key for authentication using reCAPTCHA | +| | | "" |
204-224: Fix grammar and formatting issues in the WebSocket configuration section.The content is accurate and helpful, but there are a few issues to address:
- Grammar fix needed in the reference line
- Code blocks should specify the language
- Formatting could be more consistent with other sections
Apply these changes:
## Setting up REACT_APP_BACKEND_WEBSOCKET_URL in .env file The endpoint for accessing talawa-api WebSocket graphql service for handling subscriptions is automatically added to the variable named `REACT_APP_BACKEND_WEBSOCKET_URL` in the `.env` file. -``` +```env REACT_APP_BACKEND_WEBSOCKET_URL="ws://API-IP-ADRESS:4000/graphql/"If you are a software developer working on your local system, then the URL would be:
-
+env
REACT_APP_BACKEND_WEBSOCKET_URL="ws://localhost:4000/graphql/"If you are trying to access Talawa Admin from a remote host with the API URL containing "localhost", You will have to change the API URL to -``` +```env REACT_APP_BACKEND_WEBSOCKET_URL="ws://YOUR-REMOTE-ADDRESS:4000/graphql/"-For additional details, please refer the
How to Access the Talawa-API URLsection in the INSTALLATION.md file found in the Talawa-API repo.
+For additional details, please refer to theHow to Access the Talawa-API URLsection in the INSTALLATION.md file found in the Talawa-API repo.<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> [uncategorized] ~224-~224: Possible missing preposition found. Context: ...` For additional details, please refer the `How to Access the Talawa-API URL` sect... (AI_HYDRA_LEO_MISSING_TO) </blockquote></details> <details> <summary>🪛 Markdownlint</summary><blockquote> 208-208: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 214-214: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 220-220: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Files that changed from the base of the PR and between 6f81300c78a21e287d235322c4101e2d9575b309 and bc64d4d000a6edd850a745b07ae4654d9237818a. </details> <details> <summary>📒 Files selected for processing (1)</summary> * INSTALLATION.md (5 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary><blockquote> <details> <summary>INSTALLATION.md</summary><blockquote> [uncategorized] ~69-~69: Possible missing article found. Context: ...to do this is to right-click and choose appropriate option based on your OS. 3. **For Our O... (AI_HYDRA_LEO_MISSING_THE) --- [uncategorized] ~70-~70: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...e option based on your OS. 3. **For Our Open Source Contributor Software Developers:** ... (EN_COMPOUND_ADJECTIVE_INTERNAL) --- [duplication] ~116-~116: Possible typo: you repeated a word Context: ... 5. Save the document. 6. Exit `notepad` 7. Exit PowerShell 8. This will ensure... (ENGLISH_WORD_REPEAT_RULE) --- [uncategorized] ~118-~118: Possible missing comma found. Context: ...ou are always using the correct version of `node.js` 2. For Linux and MacOS, use t... (AI_HYDRA_LEO_MISSING_COMMA) --- [grammar] ~119-~119: The operating system from Apple is written “macOS”. Context: ...t version of `node.js` 2. For Linux and MacOS, use the terminal window. 1. install... (MAC_OS) --- [grammar] ~123-~123: The word “setup” is a noun. The verb is spelled with a white space. Context: ... the respective recommended commands to setup your node environment 3. This wil... (NOUN_VERB_CONFUSION) --- [uncategorized] ~224-~224: Possible missing preposition found. Context: ...` For additional details, please refer the `How to Access the Talawa-API URL` sect... (AI_HYDRA_LEO_MISSING_TO) </blockquote></details> </blockquote></details> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>INSTALLATION.md</summary><blockquote> 95-95: null Dollar signs used before commands without showing output (MD014, commands-show-output) --- 105-105: null Bare URL used (MD034, no-bare-urls) --- 178-178: Expected: #setting-up-react_app_talawa_url-in-env-file; Actual: #setting-up-REACT_APP_TALAWA_URL-in-env-file Link fragments should be valid (MD051, link-fragments) --- 178-178: Expected: #setting-up-react_app_recaptcha_site_key-in-env-file; Actual: #setting-up-REACT_APP_RECAPTCHA_SITE_KEY-in-env-file Link fragments should be valid (MD051, link-fragments) --- 208-208: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 214-214: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 220-220: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </blockquote></details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>INSTALLATION.md (2)</summary><blockquote> `20-20`: **LGTM!** The table of contents update correctly reflects the new WebSocket configuration section. --- `206-206`: **Verify automatic configuration claim.** The documentation states that the WebSocket URL is "automatically added" but we should verify this claim against the setup script implementation. <details> <summary>✅ Verification successful</summary> **Documentation accurately reflects WebSocket URL configuration** The setup script does automatically configure the WebSocket URL by: 1. Taking the HTTP/HTTPS endpoint 2. Converting it to a WebSocket URL (ws/wss) using regex replacement 3. Appending it to the .env file as REACT_APP_BACKEND_WEBSOCKET_URL </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify if setup script handles WebSocket URL configuration # Test: Search for WebSocket URL configuration in setup script rg -l "REACT_APP_BACKEND_WEBSOCKET_URL" "setup.ts"Length of output: 57
Script:
#!/bin/bash # Let's examine how setup.ts handles the WebSocket URL configuration rg -A 10 "REACT_APP_BACKEND_WEBSOCKET_URL" "setup.ts"Length of output: 310
Script:
#!/bin/bash # Let's see the broader context of how websocketUrl is determined rg -B 15 -A 2 "REACT_APP_BACKEND_WEBSOCKET_URL" "setup.ts"Length of output: 576
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- setup.ts (1 hunks)
🔇 Additional comments (1)
setup.ts (1)
77-86: LGTM! Clean implementation of API URL update.The code properly reads and updates the environment file in a synchronous manner, maintaining consistency with the rest of the codebase.
|
Where are the test updates? The setup script has a test suite that you'll need to update |
@palisadoes I have updated the tests. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/setup/askForTalawaApiUrl/setupTalawaWebSocketUrl.test.ts (1)
1-38: Consider enhancing the WebSocket URL configuration implementation.Given that this functionality is crucial for plugin operation (as mentioned in issue #2183), consider implementing and testing additional features:
- URL validation to ensure proper WebSocket endpoint format
- Configuration persistence verification
- Fallback mechanism for when the conversion fails
- Integration tests with the plugin system
This will make the feature more robust and maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/setup/askForTalawaApiUrl/setupTalawaWebSocketUrl.test.ts(1 hunks)
🔇 Additional comments (3)
src/setup/askForTalawaApiUrl/setupTalawaWebSocketUrl.test.ts (3)
10-14: LGTM! Test suite setup follows best practices.
The test suite is well-structured with proper test isolation using beforeEach to reset mocks.
1-9: LGTM! Consider verifying import usage.
The module imports and mocks are correctly set up. However, askForTalawaApiUrl is only used in the last test case.
Let's verify if this import is used elsewhere in the codebase:
✅ Verification successful
The previous search might have had incorrect output. Let's try a broader search to find all usages of this import in TypeScript files within the src directory to make a proper assessment.
Import is correctly used and necessary
The askForTalawaApiUrl import is actively used in the test file for verifying WebSocket URL setup. The import is used in a test case that validates the integration between WebSocket URL configuration and the API URL prompt functionality. This is a legitimate use case and the import should be retained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of askForTalawaApiUrl in test files
rg -t ts "askForTalawaApiUrl" --glob "**/*.test.ts"
Length of output: 415
Script:
#!/bin/bash
# Let's search for askForTalawaApiUrl usage more broadly
rg -t ts "askForTalawaApiUrl" src/
Length of output: 776
15-38:
Test cases need improvement for better coverage and clarity.
Several issues with the current test implementation:
-
The first two tests (lines 15-28) are testing string manipulation directly rather than the actual implementation that should handle the URL conversion.
-
The third test's description "should retain default WebSocket URL if no new endpoint is provided" doesn't match its implementation, which mocks an endpoint being provided.
-
Missing test cases for:
- Invalid URLs
- URLs without http/https prefix
- Error handling scenarios
Consider refactoring the tests like this:
- test('should convert http URL to ws WebSocket URL', async () => {
- const endpoint = 'http://example.com/graphql';
- const websocketUrl = endpoint.replace(/^http(s)?:\/\//, 'ws$1://');
-
- expect(websocketUrl).toBe('ws://example.com/graphql');
+ test('should convert http URL to ws WebSocket URL', async () => {
+ const mockEndpoint = 'http://example.com/graphql';
+ jest.spyOn(inquirer, 'prompt').mockResolvedValueOnce({ endpoint: mockEndpoint });
+
+ await setupTalawaWebSocketUrl();
+
+ expect(fs.writeFileSync).toHaveBeenCalledWith(
+ expect.any(String),
+ expect.stringContaining('REACT_APP_BACKEND_WEBSOCKET_URL=ws://example.com/graphql')
+ );
});
+ test('should handle invalid URL', async () => {
+ const mockEndpoint = 'invalid-url';
+ jest.spyOn(inquirer, 'prompt').mockResolvedValueOnce({ endpoint: mockEndpoint });
+
+ await expect(setupTalawaWebSocketUrl()).rejects.toThrow('Invalid URL format');
+ });Let's verify if there are any error handling tests in related files:
palisadoes
added
the
ignore-sensitive-files-pr
.env.example
Outdated
| @@ -24,7 +24,7 @@ REACT_APP_USE_RECAPTCHA= | |||
| REACT_APP_RECAPTCHA_SITE_KEY= | |||
|
|
|||
| # has to be inserted in the env file to use plugins and other websocket based features. | |||
| REACT_APP_BACKEND_WEBSOCKET_URL=ws://localhost:4000/graphql | |||
| REACT_APP_BACKEND_WEBSOCKET_URL= | |||
There was a problem hiding this comment.
Why was this changed when it's a known working default value?
There was a problem hiding this comment.
Changed it back to the default value.
* changed color scheme for the organization people screen * fix precommit * merge * Update pre-commit * fix conflicts * fix type checks * fix type checks * fix type checks * fix ts eslint errors * fix ts eslint errors * fix ts eslint errors * fix ts eslint errors * testing * testing * testing * reverted changes in yaml file * cr comments * Update pull-request.yml * cr comments * cr comments and single css file * CR comments * delete button margin from top * prettier for commit and pull request * remove hard coded colors * fix failing test cases
|
@palisadoes Please review |
palisadoes
merged commit 470be76
into
PalisadoesFoundation:develop
Issue Number:
Fixes #2183
Did you add tests for your changes?
Yes.
If relevant, did you update the documentation?
Yes. (INSTALLATION.md)
Summary
Does this PR introduce a breaking change?
No.
Have you read the contributing guide?
Yes.
Summary by CodeRabbit
New Features
REACT_APP_BACKEND_WEBSOCKET_URLenvironment variable in the installation guide.Documentation
INSTALLATION.mdwith detailed setup instructions for the WebSocket URL.Tests
Chores