Conversation
Co-authored-by: Fortune Oluwasemilore Alebiosu <fortunealebiosu710@gmail.com>
Co-authored-by: Fortune Oluwasemilore Alebiosu <fortunealebiosu710@gmail.com>
Co-authored-by: Fortune Oluwasemilore Alebiosu <fortunealebiosu710@gmail.com>
Co-authored-by: Fortune Oluwasemilore Alebiosu <fortunealebiosu710@gmail.com>
Co-authored-by: Fortune Oluwasemilore Alebiosu <fortunealebiosu710@gmail.com>
Co-authored-by: Fortune Oluwasemilore Alebiosu <fortunealebiosu710@gmail.com>
|
Cursor Agent can help with this pull request. Just |
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughA comprehensive phone number verification system with OTP (one-time password) via Twilio SMS is introduced across backend and frontend. The backend adds configuration, two OTP endpoints (start and resend), and database schema. The frontend contributes reusable input components, a two-step verification bottom sheet, OTP hashing, and device-level prompt state management integrated into the capture screen. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend<br/>(React Native)
participant Backend as Backend<br/>(FastAPI)
participant Supabase as Supabase<br/>(Database)
participant Twilio as Twilio<br/>(SMS API)
User->>Frontend: Opens capture screen
Frontend->>Frontend: Check phone_number_updates for pending OTP
Frontend->>Frontend: Evaluate prompt state (skip/schedule)
alt Show phone prompt
Frontend->>User: Display PhoneNumberBottomSheet
User->>Frontend: Enter phone number
Frontend->>Backend: POST /otp/start (phone_number)
end
Backend->>Backend: Generate 6-digit OTP
Backend->>Backend: Hash OTP with SHA-256
Backend->>Supabase: Upsert phone_number_updates record
Supabase-->>Backend: Record stored
Backend->>Twilio: Send SMS with OTP
Twilio-->>Backend: SMS sent
Backend-->>Frontend: Success response
Frontend->>User: Prompt for OTP input
User->>Frontend: Enter OTP digits
Frontend->>Frontend: Hash OTP client-side
Frontend->>Frontend: Compare hash with stored hash
alt Hash matches
Frontend->>Supabase: Update phone_number in profile
Frontend->>Supabase: Delete phone_number_updates record
Frontend->>Frontend: Clear prompt state
Frontend->>User: Verification complete
else Hash mismatch
Frontend->>User: Show error, allow resend
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@backend/routers/phone_number.py`:
- Around line 103-117: The PR stores an otp_hash but lacks an OTP verification
endpoint; add a new route (e.g., verify_phone_otp) in
backend/routers/phone_number.py that: 1) accepts user_id/phone_number and the
plaintext otp, 2) looks up the corresponding row in the phone_number_updates
table, 3) checks created_at against now (use an expiration window like
EXPIRATION_MINUTES = 10) and returns 400 if expired, 4) verifies the plaintext
otp against the stored otp_hash using the same hashing/verify routine used when
generating otp_hash, 5) on success mark the verification complete (delete or
flag the row) and update the user’s phone verification status, and 6) return
appropriate HTTPExceptions for not found, invalid, or expired OTPs; reference
the existing upsert logic and the phone_number_updates table and reuse the same
hashing/verification helper so comparisons are consistent.
In `@backend/services/ingestion_service.py`:
- Around line 24-33: The synchronous _get_index() causes blocking and race
conditions; make it async (async def _get_index(self)) and protect
initialization with an instance-level asyncio.Lock (e.g., self._index_lock) so
concurrent coroutines serialize; inside the locked section, call
get_pinecone_index() via asyncio.to_thread(...) to offload blocking network I/O;
assign the result to self._pinecone_index only once and return it; finally,
update all call sites (ingest_entry, delete_entry, etc.) to await
self._get_index() instead of calling it synchronously.
In `@frontend/app/capture/index.tsx`:
- Around line 140-160: The Supabase query for pending OTPs in
checkShouldShowPhonePrompt uses maybeSingle() but ignores the returned error,
and the final catch silently swallows failures; update the
supabase.from('phone_number_updates').select(...).maybeSingle() call to
destructure both { data: pendingRecord, error } and handle/log the error (e.g.,
processLogger / console.error) so RLS/network issues are observable, then only
proceed to check pendingRecord?.id and call setShowPhoneSheet(…) when not
cancelled; also avoid swallowing errors in the catch by logging or rethrowing to
aid debugging.
In `@frontend/components/phone-number-bottom-sheet.tsx`:
- Around line 202-244: The client currently verifies OTP in verifyOtp using
matchesHash and then calls updateProfile, which allows bypass; instead implement
a server-side RPC/endpoint (e.g., rpc_verify_and_update_phone) that accepts user
id, phone_number and the raw OTP, looks up phone_number_updates, validates the
OTP hash server-side, performs the profile update and deletes the
phone_number_updates record inside a single transaction, and enforces
RLS/permission checks; update verifyOtp to call this RPC via supabase.rpc
(remove client-side matchesHash logic and avoid exposing otp_hash to the client)
and handle RPC errors/toasts accordingly, and keep clearPhonePromptState/onClose
usage after successful RPC.
In `@frontend/components/profile/phone-number-input.tsx`:
- Around line 45-62: The effect watching initialValue currently returns early
when initialValue is falsy, leaving previous input state intact; update the
useEffect so that when initialValue is empty/undefined it clears the component
state (call setCountryCode(''), setCountryIso(''), and setValue('')) before
returning, otherwise preserve the existing matching-country logic that uses
countries, formatPhoneNumber, extractPhoneNumber to set the derived values.
- Around line 45-61: The seeding logic in the useEffect can truncate non‑US
numbers because formatPhoneNumber/extractPhoneNumber assume 10‑digit US
formatting; update the fallback so that when no matching country is found you
preserve all digits except for US (+1) numbers: extract all digits from
initialValue (but only trim the leading +1 if you detect a +1 country) and call
setValue with the untruncated digits for non‑US numbers; keep the existing
behavior for detected countries by using longestMatch to
setCountryCode/setCountryIso and formatting only the national part, and ensure
you reference the same functions/variables (useEffect, countries, initialValue,
longestMatch, setCountryCode, setCountryIso, setValue, formatPhoneNumber,
extractPhoneNumber) when implementing the conditional.
🧹 Nitpick comments (6)
frontend/hooks/use-otp-hash.ts (1)
16-16: Consider replacing deprecatedunescapewithTextEncoder.The
unescape(encodeURIComponent(message))pattern works but relies on the deprecatedunescapefunction. IfTextEncoderis available in your target React Native/Expo environment, it would be a more modern approach.♻️ Alternative using TextEncoder (if available)
- const utf8 = unescape(encodeURIComponent(message)); + const encoder = new TextEncoder(); + const utf8Bytes = encoder.encode(message); + const utf8 = String.fromCharCode(...utf8Bytes);Note: Verify
TextEncoderavailability in your Expo/React Native environment first.frontend/components/profile/phone-update-form.tsx (1)
1-1: Remove unuseduseEffectimport.The
useEffectimport is no longer used after the refactor toPhoneNumberInput.🧹 Proposed fix
-import React, { useState, useEffect } from 'react'; +import React, { useState } from 'react';frontend/components/ui/otp-input.tsx (1)
24-28: Potential re-render loop ifonChangeis not memoized by the parent.The
useEffectdepends ononChangeand calls it whencleaned !== value. If the parent component doesn't wraponChangeinuseCallback, each render creates a new function reference, potentially causing unnecessary effect runs. Thecleaned !== valueguard should prevent infinite loops, but consider documenting that consumers should memoizeonChange, or exclude it from the dependency array with an eslint disable comment if intentional.💡 Option: Store onChange in a ref to avoid dependency
+import React, { useEffect, useMemo, useRef, useCallback } from 'react'; ... export function OtpInput({ value, onChange, length = 6 }: OtpInputProps) { const inputs = useRef<Array<TextInput | null>>([]); + const onChangeRef = useRef(onChange); + onChangeRef.current = onChange; ... useEffect(() => { // Keep the underlying value normalized (digits only). const cleaned = value.replace(/\D/g, '').slice(0, length); - if (cleaned !== value) onChange(cleaned); + if (cleaned !== value) onChangeRef.current(cleaned); - }, [length, onChange, value]); + }, [length, value]);backend/routers/phone_number.py (2)
19-22: Consider validating E.164 phone number format.The docstring mentions E.164 format, but there's no validation. Invalid phone numbers will result in SMS delivery failures at Twilio's end, wasting API calls and returning unclear errors to users.
🛡️ Add phone number validation
+import re + +E164_PATTERN = re.compile(r"^\+[1-9]\d{6,14}$") + class StartPhoneOtpRequest(BaseModel): """Request payload for starting a phone-number OTP verification flow.""" - phone_number: str = Field(..., description="E.164 phone number (e.g. +15551234567).") + phone_number: str = Field( + ..., + description="E.164 phone number (e.g. +15551234567).", + pattern=r"^\+[1-9]\d{6,14}$", + )Alternatively, use Pydantic's
field_validatorfor more complex validation or clearer error messages.
88-121: Implement rate limiting on OTP endpoints to prevent abuse.The
/otp/startand/otp/resendendpoints lack any rate limiting protection. An authenticated user could repeatedly generate OTPs and trigger SMS sends, causing excessive Twilio costs or user lockout scenarios.Recommended protections:
- Per-user rate limit (e.g., max 3–5 OTP requests per 10 minutes)
- Cooldown period between requests (e.g., minimum 30–60 seconds)
Consider using
slowapior a custom Redis-based rate limiter integrated with FastAPI.frontend/hooks/use-phone-number-update-record.ts (1)
38-41: Prefer TABLES constant to avoid string drift.Using the shared constant prevents typos and keeps table names consistent across the codebase.
♻️ Suggested tweak
-import { supabase } from '@/lib/supabase'; +import { supabase } from '@/lib/supabase'; +import { TABLES } from '@/constants/supabase'; @@ - const { data, error } = await supabase - .from('phone_number_updates') + const { data, error } = await supabase + .from(TABLES.PHONE_NUMBER_UPDATES)
| // If the user already has a pending OTP record, always show the sheet. | ||
| const { data: pendingRecord } = await supabase | ||
| .from('phone_number_updates') | ||
| .select('id') | ||
| .eq('user_id', user.id) | ||
| .maybeSingle(); | ||
|
|
||
| if (pendingRecord?.id) { | ||
| if (!cancelled) setShowPhoneSheet(true); | ||
| return; | ||
| } | ||
|
|
||
| const state = await getPhonePromptState(user.id); | ||
| const now = Date.now(); | ||
| const shouldShow = | ||
| !state.dontAskAgain && (!state.nextPromptAtMs || now >= state.nextPromptAtMs); | ||
|
|
||
| if (!cancelled) setShowPhoneSheet(shouldShow); | ||
| }; | ||
|
|
||
| checkShouldShowPhonePrompt().catch(() => {}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find schema/migration files and phone_number_updates references
rg -l "phone_number_updates" --type ts --type tsx --type sql --type js --type jsxRepository: fortune710/keepsafe
Length of output: 90
🏁 Script executed:
# Search for schema or migrations directory
fd -t d "schema|migration|supabase" | head -20Repository: fortune710/keepsafe
Length of output: 121
🏁 Script executed:
# Look for phone_number_updates table definition
rg "phone_number_updates" -A 5 -B 2Repository: fortune710/keepsafe
Length of output: 11654
Improve error handling for pending OTP record query.
The Supabase query uses maybeSingle() but doesn't destructure the error, and the catch block silently swallows errors. While a unique constraint on user_id guarantees at most one row per user, unhandled errors (e.g., RLS or network issues) can silently prevent the phone prompt from appearing. The hook usePhoneNumberUpdateRecord handles this correctly by checking the error.
Consider destructuring and logging errors for observability:
Suggested error handling
- const { data: pendingRecord } = await supabase
+ const { data: pendingRecord, error: pendingError } = await supabase
.from('phone_number_updates')
.select('id')
.eq('user_id', user.id)
.maybeSingle();
+ if (pendingError) {
+ logger.warn('Failed to check pending phone updates', pendingError);
+ }- checkShouldShowPhonePrompt().catch(() => {});
+ checkShouldShowPhonePrompt().catch((error) => {
+ logger.warn('Failed to determine phone prompt visibility', error);
+ });🤖 Prompt for AI Agents
In `@frontend/app/capture/index.tsx` around lines 140 - 160, The Supabase query
for pending OTPs in checkShouldShowPhonePrompt uses maybeSingle() but ignores
the returned error, and the final catch silently swallows failures; update the
supabase.from('phone_number_updates').select(...).maybeSingle() call to
destructure both { data: pendingRecord, error } and handle/log the error (e.g.,
processLogger / console.error) so RLS/network issues are observable, then only
proceed to check pendingRecord?.id and call setShowPhoneSheet(…) when not
cancelled; also avoid swallowing errors in the catch by logging or rethrowing to
aid debugging.
2 participants
Implement a phone number verification bottom sheet on the
/capturepage to prompt users for their phone number, including OTP verification and skip tracking.Linear Issue: DEF-98
Summary by CodeRabbit
Release Notes