Fix to signup, new inbox naming behaviour #23
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe inbox creation flow is refactored to support random email generation instead of deterministic generation. The POST /inboxes route now extracts usernames from email parameters and applies conditional logic for domain resolution and email assignment. Two utility functions are exported from InboxController to support this. A navigation link in the login page is changed from external to internal routing. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Route as POST /inboxes Route
participant Controller as InboxController
participant DB as Database
Client->>Route: POST /inboxes { email, domainId? }
Route->>Route: Extract username from email
alt domainId provided
Route->>Controller: createInbox with provided email
else domainId NOT provided
Route->>Controller: getInboxByEmail(default_email)
Controller->>DB: Query inbox by email
alt Inbox exists
Route->>Controller: getNewRandomInboxEmail(username)
Note over Controller: Generate random email
Controller->>DB: Check for conflicts
alt Conflict found
Controller->>Controller: Recurse
else No conflict
Route->>Controller: createInbox with random email
end
else Inbox does NOT exist
Route->>Controller: createInbox with default email
end
end
Controller->>DB: Insert new inbox
DB-->>Controller: Inbox created
Controller-->>Route: Return inbox
Route-->>Client: 201 Created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/routes/v1/inboxes/index.ts (1)
45-72: Fix empty local-part validation vulnerability and add.isEmail()checkThe current email validator passes invalid emails like
@domain.comthrough to inbox creation:
Critical issue: Email "@domain.com" passes
.notEmpty()validation, gets split intousername=""anddomainName="domain.com". If the domain is verified, line 64 stores the invalid email "@domain.com" in the database.Use
.isEmail(): Already available in express-validator (used in auth.ts), it would catch malformed emails earlier and align with auth endpoint validation.Normalize username consistently: Line 66 uses raw
usernamewhen building default emails, butgetNewRandomInboxEmailapplies strict normalization (lowercase +replace(/[^a-zA-Z0-9]/g, "-")). Apply the same normalization at line 66 to ensure predictable email formats.Clarify email typing: Add explicit type
let email: string | undefined;at line 62 for consistency with strict mode.Location: api/routes/v1/inboxes/index.ts, lines 34, 47–70
🧹 Nitpick comments (1)
api/controllers/InboxController.ts (1)
62-71: Lowercasing + sanitization ingetNewRandomInboxEmaillooks good; a couple of optional refinementsLowercasing
namebefore sanitizing is a nice normalization step and keeps generated local parts predictable.If you want to tighten this further (optional):
- Extract a
sanitizedNamevariable to avoid recomputing inside the template string and make the intent clearer.- Replace the recursive collision retry with a simple loop to avoid theoretical stack growth, even though collisions with a 10-char base-36 token are vanishingly rare.
- Ensure at process start that
process.env.DEFAULT_EMAIL_DOMAINis defined/non-empty (e.g., via centralized config validation), so you never accidentally end up with@undefinedlocally-part addresses.These are polish-level improvements; the current behavior is functionally fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/controllers/InboxController.ts(2 hunks)api/routes/v1/inboxes/index.ts(3 hunks)app/app/pages/login.vue(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/routes/v1/inboxes/index.ts (1)
api/controllers/InboxController.ts (3)
getInboxByEmail(58-60)getNewRandomInboxEmail(62-72)createInbox(4-24)
🔇 Additional comments (3)
app/app/pages/login.vue (1)
43-46: Signup link now correctly uses internal routingSwitching to
<NuxtLink to="/signup">is a good move for SPA navigation and avoids hard-coding the hosted domain; assuming the/signuproute exists, this looks solid.api/routes/v1/inboxes/index.ts (1)
31-79: New email/domain resolution flow in POST/inboxeslooks coherent—confirm product intentThe new flow—extract username/domain from
req.body.email, attachdomainIdonly when a verified domain exists, and otherwise fall back to a default-domain email (with randomization on conflict)—fits well with the goal of making inbox creation more forgiving and aligns with the updatedcreateInboxhelper.One notable behavioral change is that providing an email on an unverified domain now results in an inbox on the default domain (same local part) instead of a “Domain not found” 404. If that’s the intended UX for signups/onboarding, this implementation looks consistent; just worth confirming with product so callers aren’t surprised by the domain being silently swapped.
api/controllers/InboxController.ts (1)
15-23: No edge case found—all call sites correctly omit the email parameterVerification confirms that all four call sites to
createInboxomit the
Summary by CodeRabbit
Bug Fixes
Improvements