Enhance error handling and logging in Emailable response validation#1292
Open
mantrakp04 wants to merge 6 commits intodevfrom
Open
Enhance error handling and logging in Emailable response validation#1292mantrakp04 wants to merge 6 commits intodevfrom
mantrakp04 wants to merge 6 commits intodevfrom
Conversation
- Updated `validateVerifyResponse` to capture errors for invalid or unexpected responses. - Improved handling of malformed responses in `checkEmailWithEmailable`, ensuring a consistent return structure. - Refactored `getDerivedSignUpCountryCode` to log errors for non-ISO country codes. - Simplified country code determination logic in `createOrUpgradeAnonymousUserWithRules`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughRefactored country code validation from set-based lookup to regex-based validation, replacing a hardcoded ISO country code list with dynamic validation. Updated UI components from dropdown selection to direct text input. Removed public exports of ISO country code constants and updated validation messaging across backend and frontend. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/backend/src/lib/emailable.tsx (1)
47-47: Add a comment explaining theanytype usage.As per coding guidelines, the
anytype should include a comment explaining why it's needed and how errors would still be flagged.📝 Suggested comment
- const res: any = await verifyFn(); + // any: verifyFn returns unknown external API response; validated downstream via validateVerifyResponse + const res: any = await verifyFn();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/emailable.tsx` at line 47, Add an inline comment explaining why the local variable "res" is typed as "any" when assigned from "await verifyFn()", clarifying that verifyFn() returns a dynamic/third-party shape that cannot be strongly typed here, how runtime/validation still enforces expected fields (e.g., via explicit runtime checks or schema validation), and noting how TypeScript errors will still be caught elsewhere; place this comment directly above the "const res: any = await verifyFn();" declaration and reference "verifyFn" and any subsequent runtime checks used to validate "res".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/lib/emailable.tsx`:
- Around line 51-54: The current retry check in emailable.tsx uses "message" in
res and then calls res.message.includes(...), which can throw if res.message is
not a string; update the condition around the res check (the block referencing
res, wait, delayBaseMs and loop index i) to ensure typeof res.message ===
"string" (or otherwise coerce to a string) before calling .includes, so the
includes call is only executed when res.message is a string and avoids a
TypeError.
---
Nitpick comments:
In `@apps/backend/src/lib/emailable.tsx`:
- Line 47: Add an inline comment explaining why the local variable "res" is
typed as "any" when assigned from "await verifyFn()", clarifying that verifyFn()
returns a dynamic/third-party shape that cannot be strongly typed here, how
runtime/validation still enforces expected fields (e.g., via explicit runtime
checks or schema validation), and noting how TypeScript errors will still be
caught elsewhere; place this comment directly above the "const res: any = await
verifyFn();" declaration and reference "verifyFn" and any subsequent runtime
checks used to validate "res".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: adbc554e-d641-4747-9b72-2e612961fbb6
📒 Files selected for processing (2)
apps/backend/src/lib/emailable.tsxapps/backend/src/lib/users.tsx
Contributor
Greptile SummaryThis PR broadens country code validation and replaces the searchable dropdown UI with a plain text input, with the primary goal of supporting non-ISO two-letter codes (such as Key changes:
Notable semantic change: Any currently-deployed sign-up rules that relied on unrecognised geo codes (e.g. Cloudflare Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/UX observations with no blocking correctness issues. The logic simplification in createOrUpgradeAnonymousUserWithRules is correct and an improvement (undefined-safety). The geo-header normalisation in end-users.tsx is a straightforward improvement. The isValidCountryCode relaxation is intentional and well-tested. The two P2 findings (potential "ZZ"/"XX" in rules, empty list entry UX) are informational, not blocking. packages/stack-shared/src/utils/country-codes.ts — the isValidCountryCode change is the semantic core of this PR; worth a quick audit of any live sign-up rules before deploying. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Sign-up Request] --> B{signUpRuleOptions.countryCode?}
B -- set --> C[requestCountryCode = signUpRuleOptions.countryCode]
B -- not set --> D{endUserRequestContext geo?}
D -- present --> E[getBrowserEndUserInfo\nnormalizeCountryCode on header]
D -- absent --> F[requestCountryCode = null]
E --> G[requestCountryCode = normalized geo code]
C --> H[getDerivedSignUpCountryCode]
F --> H
G --> H
H --> I{dev/test + email tag?}
I -- yes --> J[Extract tag from +XX@example.com]
J --> K{isValidCountryCode tag?}
K -- yes --> L[countryCode = tag]
K -- no --> M[fall through]
I -- no/prod --> M
M --> N{isValidCountryCode requestCountryCode?}
N -- yes --> O[countryCode = normalizedCode]
N -- no --> P[countryCode = null]
L --> Q[evaluateSignUpRules countryCode]
O --> Q
P --> Q
Q --> R{shouldAllow?}
R -- yes --> S[createOrUpgradeAnonymousUser]
R -- no --> T[throw SignUpRejected]
Reviews (3): Last reviewed commit: "Enhance country code validation in visua..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…esponses - Updated `validateVerifyResponse` to throw errors for non-object responses and invalid states. - Simplified response handling in `checkEmailWithEmailable` to ensure consistent error management. - Adjusted tests to reflect changes in error handling for malformed Emailable responses.
- Replaced `CountryCodeSelect` with `CountryCodeInput` for a more streamlined input experience. - Updated country code validation logic to ensure only 2-letter codes are accepted. - Enhanced error messages for invalid country codes to improve clarity. - Removed unused constants and simplified the country code validation functions. - Adjusted tests to reflect changes in country code handling and validation.
Collaborator
Author
|
@greptile-ai review |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/src/lib/cel-visual-parser.ts (1)
79-85:⚠️ Potential issue | 🟠 MajorReinstate fail-fast for numeric
countryCodevaluesAt Line 81, returning a number silently accepts an invalid state. This can emit semantically invalid rules (e.g.,
countryCode == "42") instead of surfacing the upstream type issue immediately.Proposed fix
function normalizeConditionValue(condition: ConditionNode): ConditionNode['value'] { if (condition.field !== 'countryCode') return condition.value; - if (typeof condition.value === 'number') return condition.value; + if (typeof condition.value === 'number') { + throw new StackAssertionError( + `countryCode must be a string or string[], received number: ${condition.value}`, + ); + } return Array.isArray(condition.value) ? condition.value.map(normalizeCountryCode) : normalizeCountryCode(condition.value); }As per coding guidelines, "NEVER silently use fallback values when type errors occur" and "Fail early, fail loud. Fail fast with an error instead of silently continuing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/cel-visual-parser.ts` around lines 79 - 85, In normalizeConditionValue (for ConditionNode handling), don't silently accept numeric values for field === 'countryCode'; instead detect typeof condition.value === 'number' and throw a clear TypeError (including the field name and the offending value) so callers fail fast; keep the existing Array.isArray handling and normalizeCountryCode path for strings/arrays, but replace the current numeric-return branch with an explicit thrown error to surface upstream type issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/dashboard/src/lib/cel-visual-parser.ts`:
- Around line 79-85: In normalizeConditionValue (for ConditionNode handling),
don't silently accept numeric values for field === 'countryCode'; instead detect
typeof condition.value === 'number' and throw a clear TypeError (including the
field name and the offending value) so callers fail fast; keep the existing
Array.isArray handling and normalizeCountryCode path for strings/arrays, but
replace the current numeric-return branch with an explicit thrown error to
surface upstream type issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c34add96-f699-46f9-81f9-0aaede409eef
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/backend/src/lib/end-users.tsxapps/backend/src/lib/users.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/sign-up-rules/page-client.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/users/[userId]/page-client.tsxapps/dashboard/src/components/country-code-select.tsxapps/dashboard/src/components/rule-builder/condition-builder.tsxapps/dashboard/src/lib/cel-visual-parser.tspackages/stack-shared/src/schema-fields.tspackages/stack-shared/src/utils/country-codes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/src/lib/users.tsx
- Updated `normalizeConditionValue` to throw an error for numeric values assigned to `countryCode`, ensuring that only string values are accepted. - Improved error messaging for invalid country code inputs to enhance debugging and user feedback.
Collaborator
Author
|
@greptile-ai review |
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
1 participant
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.
validateVerifyResponseto capture errors for invalid or unexpected responses.checkEmailWithEmailable, ensuring a consistent return structure.getDerivedSignUpCountryCodeto log errors for non-ISO country codes.createOrUpgradeAnonymousUserWithRules.Summary by CodeRabbit
New Features
Bug Fixes
Documentation