fix(card): don't render "#null" for a waitlist entry without a position#2437
fix(card): don't render "#null" for a waitlist entry without a position#2437innolope-dev wants to merge 3 commits into
Conversation
getPhysicalWaitlist types position as nullable and the joined branch is gated on joinedAt, so a user on the list with no queue position was told 'You are #null on the list.' Drop the number for that case and format real positions for readability.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWaitlist copy handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
Code-analysis diffPainscore total: 6207.05 → 6207.36 (+0.31) 🆕 New findings (2)
✅ Resolved (2)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/Card/__tests__/PhysicalCardScreen.test.tsx`:
- Around line 55-59: Update the large-position test in PhysicalCardScreen to
derive the expected position text from the numeric value using toLocaleString(),
rather than hardcoding the comma-formatted “1,234” string. Keep the assertion’s
surrounding wording and existing mock behavior unchanged.
In `@src/components/Card/PhysicalCardScreen.tsx`:
- Around line 79-81: Update the position check in the PhysicalCardScreen display
expression to treat both null and undefined as the “on the list” state by using
a loose null comparison. Keep the toLocaleString formatting path for defined
position values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb6811f8-a9a3-4375-b4c8-24fd024222aa
📒 Files selected for processing (2)
src/components/Card/PhysicalCardScreen.tsxsrc/components/Card/__tests__/PhysicalCardScreen.test.tsx
| {data.position === null | ||
| ? `You are on the list. We'll let you know when cards are ready to be shipped.` | ||
| : `You are #${data.position.toLocaleString()} on the list. We'll let you know when cards are ready to be shipped.`} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a loose equality check to prevent runtime crashes if position is undefined.
The strict equality check (=== null) will evaluate to false if the API omits the position field, making data.position undefined. This would cause the app to crash with a TypeError: Cannot read properties of undefined (reading 'toLocaleString').
Using a loose equality check (== null) handles both null and undefined safely.
🛡️ Proposed fix
- {data.position === null
+ {data.position == null
? `You are on the list. We'll let you know when cards are ready to be shipped.`
: `You are #${data.position.toLocaleString()} on the list. We'll let you know when cards are ready to be shipped.`}📝 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.
| {data.position === null | |
| ? `You are on the list. We'll let you know when cards are ready to be shipped.` | |
| : `You are #${data.position.toLocaleString()} on the list. We'll let you know when cards are ready to be shipped.`} | |
| {data.position == null | |
| ? `You are on the list. We'll let you know when cards are ready to be shipped.` | |
| : `You are #${data.position.toLocaleString()} on the list. We'll let you know when cards are ready to be shipped.`} |
🤖 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/components/Card/PhysicalCardScreen.tsx` around lines 79 - 81, Update the
position check in the PhysicalCardScreen display expression to treat both null
and undefined as the “on the list” state by using a loose null comparison. Keep
the toLocaleString formatting path for defined position values.
The hardcoded "1,234" assumed an en-US default locale, so the test failed under any locale with a different thousands separator (de_DE renders 1.234). Assert against toLocaleString output instead — that is the actual contract.
kushagrasarathe
left a comment
kushagrasarathe
left a comment
There was a problem hiding this comment.
Approved. Live bug — nullable position interpolated to "#null". === null branch is correct (keeps #0 valid, unlike a falsy check). toLocaleString() is a justified in-line extra and is tested. Regression test verified to fail pre-fix.
2 participants
The bug
The physical-card waitlist screen can tell a user they are "You are #null on the list."
getPhysicalWaitlisttypes the position as nullable (src/services/rain.ts):…but
PhysicalCardScreenrenders the joined branch gated onjoinedAtonly, interpolatingpositionregardless:A JS template literal stringifies
nullto"null", so a user who is on the list with no assigned queue position gets#null. The same component already treats the field as nullable a few lines up —position: data?.position ?? nullfor its PostHog event — which confirms null is a real, expected state rather than a type-only edge case.This is not related to the earlier
#16on→#16 onspacing fix (#2311); that one repaired the spacing on this same line and left the null case untouched. The bug is live onmaintoday.The fix
Branch on the null case with its own sentence instead of interpolating a number that isn't there. Since the interpolation was being touched anyway, real positions now go through
toLocaleString(), so a queue position of 1234 reads#1,234rather than#1234.Tests
New
PhysicalCardScreen.test.tsx(the component had none) covering three cases: a normal position, the null case asserting no#reaches the body, and thousands-separator formatting.The test was verified to actually catch the bug — against the original code the null and formatting cases fail (with exactly the reported
#null) while the normal-position case still passes. A regression test that passes either way is worthless, so this was checked rather than assumed.Verification
pnpm typecheck✓ ·jest PhysicalCardScreen3/3 ✓ ·eslintno new errors ·prettierclean.Summary by CodeRabbit
Bug Fixes
Tests