Fixing password flow#190
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe ChangesPassword Set Endpoint Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/server.test.ts (1)
106-133: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding edge case tests.
The happy path is well-tested. Consider adding tests for:
setPasswordfailure → expect 400 responsegetSessionreturnsnullor{ user: {} }→ verify behavior🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.test.ts` around lines 106 - 133, Add additional test cases for the '/api/auth/password/set' endpoint to cover edge cases beyond the current happy path test. Create a test where auth.api.setPassword is mocked to reject with an error and verify the endpoint returns a 400 status response. Additionally, create tests where auth.api.getSession is mocked to resolve with null or with an object containing an empty user object (like { user: {} }), and verify the endpoint handles these scenarios appropriately by checking the response status and any error messages returned. Follow the same test structure and mock setup patterns as the existing test case, using mockResolvedValue and mockReturnValue to set up the mock behaviors and assertions to verify the expected outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domains/recordings/recordings.repository.ts`:
- Around line 31-37: The onConflictDoUpdate block in the recordings repository
is incorrectly updating the createdAt field to now() when a conflict occurs,
which overwrites the original creation timestamp and violates the semantic
meaning of createdAt. Remove the createdAt entry from the set object within
onConflictDoUpdate so that the original creation timestamp is preserved during
upserts, leaving only the metadata field to be updated on conflict.
In `@src/domains/recordings/recordings.route.ts`:
- Around line 122-138: The current handler validates the existence of
project_unit_id but does not validate bible_text_id before uploading to R2,
causing orphaned files when an invalid bible_text_id causes the subsequent
database insert to fail on the foreign key constraint. Add a similar existence
check for bible_text_id using a database query to verify it exists in the
appropriate schema table before proceeding with the R2 upload, ensuring the
validation is performed in the same try-catch block as the project_unit_id check
to maintain consistency and error handling.
In `@src/server/server.ts`:
- Around line 106-113: When the password update succeeds but the session or
session.user.email is missing, the status update to 'verified' is silently
skipped while still returning success, leaving the user in an inconsistent
state. Add a warning log inside the conditional block after the database update
completes to confirm the status was updated, or add an else clause that logs a
warning when session?.user?.email is falsy to indicate the status update was
skipped. This ensures visibility into cases where the user remains in an
'invited' status despite a successful password set operation.
---
Outside diff comments:
In `@src/server/server.test.ts`:
- Around line 106-133: Add additional test cases for the
'/api/auth/password/set' endpoint to cover edge cases beyond the current happy
path test. Create a test where auth.api.setPassword is mocked to reject with an
error and verify the endpoint returns a 400 status response. Additionally,
create tests where auth.api.getSession is mocked to resolve with null or with an
object containing an empty user object (like { user: {} }), and verify the
endpoint handles these scenarios appropriately by checking the response status
and any error messages returned. Follow the same test structure and mock setup
patterns as the existing test case, using mockResolvedValue and mockReturnValue
to set up the mock behaviors and assertions to verify the expected outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4354ee9a-5d7a-49cc-966a-4e51a4639d77
📒 Files selected for processing (15)
.env.example.env.testsrc/app.tssrc/db/migrations/0012_add_recordings.sqlsrc/db/migrations/0013_add_metadata_to_recordings.sqlsrc/db/migrations/meta/0012_snapshot.jsonsrc/db/migrations/meta/0013_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema.tssrc/domains/recordings/recordings.repository.tssrc/domains/recordings/recordings.route.tssrc/env.tssrc/lib/r2-upload.tssrc/server/server.test.tssrc/server/server.ts
Joel-Joseph-George
force-pushed
the
fix/password-flow
branch
from
June 19, 2026 05:45
6a3ff71 to
7618e58
Compare
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)
src/server/server.test.ts (1)
106-133: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding test coverage for the session-absent edge case.
The test covers the happy path but doesn't verify behavior when
getSessionreturnsnull(password set succeeds, but status update should be skipped while still returning success). Adding this test would help ensure the conditional logic at lines 106-111 ofserver.tsbehaves correctly.💡 Example test for session-absent case
it('should succeed without updating status when session is absent', async () => { (auth.api.setPassword as any).mockResolvedValue(undefined); (auth.api.getSession as any).mockResolvedValue(null); const res = await app.request('/api/auth/password/set', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ newPassword: 'hunter2' }), }); expect(res.status).toBe(200); expect(await res.json()).toEqual({ success: true }); expect(auth.api.setPassword).toHaveBeenCalled(); expect(mockDb.update).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.test.ts` around lines 106 - 133, Add a new test case in the test file to cover the edge case where getSession returns null after a successful password set operation. Create a test that mocks auth.api.setPassword to resolve successfully and auth.api.getSession to return null, then verify that the endpoint still returns a 200 success response, the setPassword function was called, but the database update (mockDb.update) was NOT called since there's no session available to extract user information for the status update. This ensures the conditional logic properly handles the session-absent scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/server.ts`:
- Around line 94-96: The JSON parsing in the POST /api/auth/password/set
endpoint handler is occurring outside the try block, causing malformed JSON
errors to escape and return a 500 status instead of a proper 400 client error.
Move the line containing const { newPassword } = await c.req.json(); from before
the try block into the beginning of the try block, so that JSON parsing errors
are properly caught and can be handled with a 400 status response.
---
Outside diff comments:
In `@src/server/server.test.ts`:
- Around line 106-133: Add a new test case in the test file to cover the edge
case where getSession returns null after a successful password set operation.
Create a test that mocks auth.api.setPassword to resolve successfully and
auth.api.getSession to return null, then verify that the endpoint still returns
a 200 success response, the setPassword function was called, but the database
update (mockDb.update) was NOT called since there's no session available to
extract user information for the status update. This ensures the conditional
logic properly handles the session-absent scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eaeeb8ad-eef6-457b-809f-d178b1dfe39f
📒 Files selected for processing (2)
src/server/server.test.tssrc/server/server.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/server.ts (1)
119-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t collapse all failures into HTTP 400.
Lines 119-124 currently classify infrastructure/runtime failures as client errors. That masks server faults and breaks API semantics.
Proposed fix (separate client-input/auth step from server-finalization step)
app.post('/api/auth/password/set', async (c) => { try { const { newPassword } = await c.req.json(); await auth.api.setPassword({ body: { newPassword }, headers: c.req.raw.headers, }); + } catch (err) { + logger.error('Failed to set password:', err); + return c.json( + { message: err instanceof Error ? err.message : 'Failed to set password' }, + 400 + ); + } + try { const session = await auth.api.getSession({ headers: c.req.raw.headers, }); if (session?.user?.email) { await db .update(schema.users) .set({ status: 'verified', updatedAt: new Date() }) .where(eq(schema.users.email, session.user.email)); logger.info(`User status updated to verified for email: ${session.user.email}`); } else { logger.warn( 'Password set succeeded, but session or user email is missing. Skipped updating user status to verified.' ); } return c.json({ success: true }); } catch (err) { - logger.error('Failed to set password:', err); - return c.json( - { message: err instanceof Error ? err.message : 'Failed to set password' }, - 400 - ); + logger.error('Failed to finalize password-set flow:', err); + return c.json({ message: 'Internal Server Error' }, 500); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/server.ts` around lines 119 - 124, The catch block in the password-setting endpoint (around lines 119-124) currently returns HTTP 400 for all errors, which incorrectly treats server/infrastructure failures as client errors. Refactor the code to separate input validation and authentication checks from the actual password-setting operation. Validation and authentication errors (client-side issues) should return HTTP 400, while unexpected runtime or infrastructure errors should return HTTP 500 to accurately reflect the nature of the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/server.test.ts`:
- Around line 136-146: The malformed-JSON test in the 'should return 400 when
JSON body is malformed' test case needs to verify that the setPassword operation
was not executed when parsing fails. After the
expect(json.message).toBeDefined() assertion, add a verification that
auth.api.setPassword was not called to confirm the request handler properly
short-circuits on JSON parse failure without proceeding to business logic
execution.
In `@src/server/server.ts`:
- Line 111: The logger.info call that logs user status updates contains a raw
email address which is personally identifiable information (PII) and should not
be exposed in logs. Modify the success log message in the logger.info call to
remove the ${session.user.email} reference entirely. Either replace it with a
generic message like "User status updated to verified" or use a non-PII
identifier such as a user ID if available from the session object, to maintain
the ability to track the action without exposing sensitive email data.
---
Outside diff comments:
In `@src/server/server.ts`:
- Around line 119-124: The catch block in the password-setting endpoint (around
lines 119-124) currently returns HTTP 400 for all errors, which incorrectly
treats server/infrastructure failures as client errors. Refactor the code to
separate input validation and authentication checks from the actual
password-setting operation. Validation and authentication errors (client-side
issues) should return HTTP 400, while unexpected runtime or infrastructure
errors should return HTTP 500 to accurately reflect the nature of the failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8632b89-d3a5-4177-8c46-a29671d537be
📒 Files selected for processing (2)
src/server/server.test.tssrc/server/server.ts
Comment on lines
+136
to
+146
| it('should return 400 when JSON body is malformed', async () => { | ||
| const res = await server.request('/api/auth/password/set', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer test-token' }, | ||
| body: 'invalid-json-body', | ||
| }); | ||
|
|
||
| expect(res.status).toBe(400); | ||
| const json = await res.json(); | ||
| expect(json.message).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Strengthen malformed-JSON test with a no-side-effect assertion.
After Line 145, assert auth.api.setPassword was not called to lock in short-circuit behavior on parse failure.
Proposed test addition
expect(res.status).toBe(400);
const json = await res.json();
expect(json.message).toBeDefined();
+ expect(auth.api.setPassword).not.toHaveBeenCalled();📝 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.
Suggested change
| it('should return 400 when JSON body is malformed', async () => { | |
| const res = await server.request('/api/auth/password/set', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer test-token' }, | |
| body: 'invalid-json-body', | |
| }); | |
| expect(res.status).toBe(400); | |
| const json = await res.json(); | |
| expect(json.message).toBeDefined(); | |
| }); | |
| it('should return 400 when JSON body is malformed', async () => { | |
| const res = await server.request('/api/auth/password/set', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json', Authorization: 'Bearer test-token' }, | |
| body: 'invalid-json-body', | |
| }); | |
| expect(res.status).toBe(400); | |
| const json = await res.json(); | |
| expect(json.message).toBeDefined(); | |
| expect(auth.api.setPassword).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/server.test.ts` around lines 136 - 146, The malformed-JSON test in
the 'should return 400 when JSON body is malformed' test case needs to verify
that the setPassword operation was not executed when parsing fails. After the
expect(json.message).toBeDefined() assertion, add a verification that
auth.api.setPassword was not called to confirm the request handler properly
short-circuits on JSON parse failure without proceeding to business logic
execution.
| .update(schema.users) | ||
| .set({ status: 'verified', updatedAt: new Date() }) | ||
| .where(eq(schema.users.email, session.user.email)); | ||
| logger.info(`User status updated to verified for email: ${session.user.email}`); |
There was a problem hiding this comment.
Remove raw email from success logs.
Line 111 logs a full email address, which introduces avoidable PII exposure in application logs.
Proposed fix
- logger.info(`User status updated to verified for email: ${session.user.email}`);
+ logger.info('User status updated to verified');📝 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.
Suggested change
| logger.info(`User status updated to verified for email: ${session.user.email}`); | |
| logger.info('User status updated to verified'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/server.ts` at line 111, The logger.info call that logs user status
updates contains a raw email address which is personally identifiable
information (PII) and should not be exposed in logs. Modify the success log
message in the logger.info call to remove the ${session.user.email} reference
entirely. Either replace it with a generic message like "User status updated to
verified" or use a non-PII identifier such as a user ID if available from the
session object, to maintain the ability to track the action without exposing
sensitive email data.
vipinpaul
approved these changes
Jun 19, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary by CodeRabbit
Release Notes
400responses with an explanatory message.