Update/readme.md and .gitignore #105
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe update standardizes environment-variable validation across multiple Appwrite functions, enhances send-otp with cryptographic OTP generation and persistence, switches email credentials to environment variables in appwrite.json, refines README environment-setup instructions, and adjusts .gitignore patterns to root-scoped entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant FE as Frontend
participant F as send-otp Function
participant AP as Appwrite (DB)
participant EM as Email Service
rect rgb(240,248,255)
note over F: Validate envs (APPWRITE_API_KEY, APPWRITE_FUNCTION_PROJECT_ID, SENDER_MAIL, SENDER_PASSWORD, ...)
User->>FE: Request OTP
FE->>F: POST /send-otp (email)
F->>F: Generate OTP (crypto.randomInt)
F->>AP: createOtpDocument(otpID, otp, timestamp)
AP-->>F: OK
F->>EM: Send email with OTP
EM-->>F: Accepted
F-->>FE: 200 OK (status)
end
note over FE,User: User proceeds to verify OTP via separate function
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
functions/join-room/src/main.js (1)
5-10: Remove unused APPWRITE_FUNCTION_PROJECT_ID env var requirement
APPWRITE_FUNCTION_PROJECT_IDis listed as required but isn’t referenced in this function or in livekit.js. Remove it from thethrowIfMissingcall.appwrite.json (1)
85-101: Remove$prefix from environment variable values in appwrite.json.The
$prefix isn’t supported in Appwrite’s function config—varsvalues must be plain strings. For example:- "value": "$SENDER_MAIL" + "value": "SENDER_MAIL"Do the same for
SENDER_PASSWORD, then configure the real secrets via the Appwrite console or CLI. Rotate the previously exposed password immediately.
🧹 Nitpick comments (1)
.gitignore (1)
9-10: Consider removing redundant root-level patterns.The root-level patterns
/.envand/node_modules/are already covered by the recursive patterns**/.envand**/node_modules/on lines 5-6. While not harmful, this redundancy could be simplified for better maintainability.Apply this diff to remove the redundant patterns:
**/resonate-service-account.json meili_data/ -/.env -/node_modules/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)README.md(1 hunks)appwrite.json(1 hunks)functions/create-room/src/main.js(1 hunks)functions/database-cleaner/src/main.js(1 hunks)functions/delete-room/src/main.js(1 hunks)functions/join-room/src/main.js(1 hunks)functions/livekit-webhook/src/main.js(1 hunks)functions/match-maker/src/main.js(1 hunks)functions/send-otp/src/main.js(3 hunks)functions/verify-email/src/main.js(1 hunks)functions/verify-otp/src/main.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
functions/send-otp/src/main.js (1)
functions/send-otp/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
functions/verify-email/src/main.js (1)
functions/verify-email/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)
🪛 markdownlint-cli2 (0.18.1)
README.md
25-25: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
🔇 Additional comments (12)
functions/database-cleaner/src/main.js (1)
5-14: LGTM! Environment validation standardization.Adding
APPWRITE_FUNCTION_PROJECT_IDto required environment variables aligns with the broader standardization effort across all functions and ensures the AppwriteService initialization has the necessary project context.functions/livekit-webhook/src/main.js (1)
8-16: LGTM! Consistent environment validation.The addition of
APPWRITE_FUNCTION_PROJECT_IDto required environment variables is consistent with the standardization pattern and ensures proper Appwrite client initialization.functions/delete-room/src/main.js (1)
6-15: LGTM! Proper environment validation.The addition of
APPWRITE_FUNCTION_PROJECT_IDis correct. The environment variable is validated upfront and properly used at line 22 when configuring the Appwrite client.functions/verify-otp/src/main.js (1)
5-11: LGTM! Correct environment validation.The
APPWRITE_FUNCTION_PROJECT_IDis appropriately added to required environment variables and correctly used at line 17 for client configuration.functions/create-room/src/main.js (1)
6-15: LGTM! Environment validation standardization.Adding
APPWRITE_FUNCTION_PROJECT_IDaligns with the standardization effort and ensures the AppwriteService has proper project context for initialization.functions/verify-email/src/main.js (1)
5-12: LGTM! Proper environment validation.The addition of
APPWRITE_FUNCTION_PROJECT_IDis correct. The variable is validated at line 5 and properly used at line 11 for Appwrite client configuration.functions/match-maker/src/main.js (1)
7-7: LGTM! Consistent environment validation.Adding
APPWRITE_FUNCTION_PROJECT_IDto the validation list ensures the function fails fast if the required environment variable is missing, aligning with the PR-wide standardization across all functions.functions/send-otp/src/main.js (4)
4-4: Excellent security improvement!Importing
randomIntfromnode:cryptofor OTP generation is a significant security enhancement overMath.random().
9-9: LGTM! Consistent environment validation.Adding
APPWRITE_FUNCTION_PROJECT_IDvalidation aligns with the PR-wide standardization across all Appwrite functions.
13-13: Good catch on missing validation.Adding
SENDER_PASSWORDto the required environment variables ensures proper validation of credentials that are already being used by the mail service.
23-24: Cryptographically secure OTP generation.Replacing
Math.random()withcrypto.randomInt()provides cryptographically secure random number generation, which is essential for OTP security. The range (100000, 1000000) correctly generates 6-digit OTPs (100000-999999).README.md (1)
10-48: Excellent documentation improvements!The restructured environment setup section provides clear, step-by-step guidance with code blocks and explicit instructions. The security note about not hardcoding secrets is particularly valuable and aligns with the environment variable standardization across the codebase.
README.md
Outdated
| - Leave the database/collection IDs as they are (they match the project structure) | ||
| - Add your Livekit credentials (see step 3) | ||
| - Add your email credentials (see step 5) |
There was a problem hiding this comment.
Fix list indentation for consistency.
The sub-items under step 2 have inconsistent indentation (3 spaces instead of the expected 0 or standard 2/4 spaces), which may affect markdown rendering.
As per static analysis hints.
Apply this diff to fix the indentation:
Then fill in the values in `.env`:
- - Leave the database/collection IDs as they are (they match the project structure)
- - Add your Livekit credentials (see step 3)
- - Add your email credentials (see step 5)
+- Leave the database/collection IDs as they are (they match the project structure)
+- Add your Livekit credentials (see step 3)
+- Add your email credentials (see step 5)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Leave the database/collection IDs as they are (they match the project structure) | |
| - Add your Livekit credentials (see step 3) | |
| - Add your email credentials (see step 5) | |
| Then fill in the values in `.env`: | |
| - Leave the database/collection IDs as they are (they match the project structure) | |
| - Add your Livekit credentials (see step 3) | |
| - Add your email credentials (see step 5) |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
25-25: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
🤖 Prompt for AI Agents
In README.md around lines 25 to 27, the three sub-items under step 2 use
inconsistent indentation (3 spaces) which can affect Markdown rendering; update
those lines to use consistent indentation (prefer 0 or standard 2/4 spaces) so
they align with the surrounding list format, ensuring each bullet uses the same
leading spaces and dash spacing for consistent rendering across Markdown
parsers.
There was a problem hiding this comment.
Unnecessary and breaking change
There was a problem hiding this comment.
Unnecessary and breaking change
There was a problem hiding this comment.
Unnecessary and breaking change
There was a problem hiding this comment.
Unnecessary and breaking change
|
Thank you for the thorough review and detailed feedback. I acknowledge the issues raised regarding documentation inconsistency, missing files and guides, unnecessary changes in multiple files, and incomplete instructional content. I am preparing a follow-up PR to address these systematically I appreciate the patience and guidance; please keep the feedback coming to ensure the setup and code quality meet team standards. |
|
Please try and make the changes in this PR instead of opening a follow up PR. Also, open your PRs to the dev branch instead of main |
|
sure |
There was a problem hiding this comment.
This is not the preferred approach, as user will have to manually run these commands outside the setup script, preferred approach would be to have this integrated in the automatic setup as mentioned in #101 (comment)
|
Thanks for clarifying. Two ways we could handle automated setup: Option 1: Template approach Option 2: Interactive prompts I'd suggest Option 1 since it's easier to verify what you entered and follows what most projects do. |
|
Hey @VasuS609 , out of these approaches, I would suggest 2, also because Resonate already uses it in other places. Additionally could you look at a way to generate the keys through the script as well? |
|
Hey @M4dhav For generating keys through the script - which keys are you referring to? The email credentials (SENDER_MAIL/SENDER_PASSWORD) need to come from the user's actual Gmail account, and the database/collection IDs are already set in the project. Or did you mean something like API keys for Appwrite/Livekit? Let me know what needs to be auto-generated and I'll figure out how to integrate that into the setup flow. |
|
I mean the Email Creds. Try and see if there's a way to generate them at runtime in the script, where a user can login to his gmail account and the keys are generated for him |
|
I see what you mean, but that's tricky for a few reasons Technical challenges: |
|
Okay, if this is the only possible approach then let's go ahead with this, but an
So let's have it like this, but automate the fetching and uploading of these variables in the setup script, and remove unrelated changes from the PR |
|
sure |
2 participants
Summary:
This PR updates the backend documentation and configuration files to clarify setup and enhance security. - Fixes #101
Changes Made:
README.md
.gitignore
appwrite.json
Synced configurations with updated function structure.
Why this change:
Testing:
Verified environment setup locally.
Confirmed all function directories exist and correspond correctly.