Feature/contact syncn.new#5
Conversation
This workflow installs Deno and runs linting and tests on push and pull request events to the main branch.
📝 WalkthroughWalkthroughThis PR adds contact syncing capabilities to a WhatsApp clone by introducing a Supabase profiles table with security policies, updating app configuration for contact access, adding a new contact list screen with synced contacts display, and establishing a Deno CI workflow for backend code validation. ChangesContact Syncing Feature
Deno CI Workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
supabase/migrations/001_add_phone_to_profiles.sql (1)
14-15: ⚡ Quick winDrop the redundant indexes on unique columns.
Both
phoneanduser_idalready get indexes from theirUNIQUEconstraints, so these extra indexes only add storage and write cost.♻️ Proposed cleanup
-CREATE INDEX IF NOT EXISTS idx_profiles_phone ON public.profiles(phone); -CREATE INDEX IF NOT EXISTS idx_profiles_user_id ON public.profiles(user_id);🤖 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 `@supabase/migrations/001_add_phone_to_profiles.sql` around lines 14 - 15, The migration unnecessarily creates indexes idx_profiles_phone and idx_profiles_user_id on columns that already have UNIQUE constraints; remove the redundant CREATE INDEX lines for idx_profiles_phone and idx_profiles_user_id (the UNIQUE constraints on profiles(phone) and profiles(user_id) already provide indexes) so the migration only leaves the necessary schema changes without duplicating index storage and write overhead.
🤖 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 @.github/workflows/deno.yml:
- Around line 25-26: Update the GitHub Actions checkout step so it is pinned to
a specific commit hash instead of the tag and disable credential persistence;
locate the "uses: actions/checkout@v4" line in the workflow and replace the tag
with the repo commit SHA (e.g., actions/checkout@<commit-hash>) and add the key
"persist-credentials: false" under that step to prevent GITHUB_TOKEN from being
persisted.
In `@app.json`:
- Around line 30-33: The permissions list in app.json unnecessarily includes
"android.permission.READ_PHONE_STATE"; remove this permission from the
"permissions" array and keep only "android.permission.READ_CONTACTS" (or any
other permissions your app actually uses) so the app does not request telephony
state; update any related documentation or rationale comments if present to
reflect the trimmed permission set.
In `@app/`(tabs)/new-chat.tsx:
- Around line 58-63: The placeholder avatar rendering assumes
item.device_contact_name[0] exists and crashes for empty or missing names;
update the conditional in the new-chat.tsx render block to safely derive an
initial (e.g., compute const initial = item.device_contact_name ?
item.device_contact_name.trim()[0]?.toUpperCase() : null) and render that
initial only when present, otherwise render a fallback (such as a default icon
or empty string). Locate the JSX that uses item.device_contact_name[0] (the
avatar placeholder branch) and replace the direct index access with a guarded
value using trim() and optional chaining before calling toUpperCase().
In `@package.json`:
- Around line 21-22: The package.json entry for the dependency "expo-contacts"
is incompatible with Expo SDK 50; update the "expo-contacts" version string from
"^14.5.0" to the SDK-50 compatible "12.8.2" (or run npx expo install --fix to
reconcile all Expo package versions automatically) so it matches the installed
"expo" ^50.0.2; change the "expo-contacts" value in package.json and then
reinstall (npm/yarn) to update lockfiles.
In `@supabase/migrations/001_add_phone_to_profiles.sql`:
- Around line 20-24: The current SELECT policy "Users can view profiles" on
public.profiles uses USING (auth.uid() IS NOT NULL) which allows any signed-in
user to read all profile rows; replace it with a tighter policy so SELECT only
returns the caller's own profile (e.g., restrict USING to compare the profile's
owner column to auth.uid(), such as USING (user_id = auth.uid()) on
public.profiles) and remove or narrow any global read grants; for contact-sync
functionality, implement a server-side RPC (e.g., match_contacts) that accepts
phone hashes and performs the matching on the DB side, returning only allowed
fields, instead of broadening the SELECT policy.
- Line 5: The migration currently adds phone TEXT NOT NULL UNIQUE which enforces
uniqueness on raw formatting; change it to add a separate normalized column
(e.g., phone_normalized) and enforce uniqueness on that column instead: keep the
original phone TEXT for display, add phone_normalized TEXT NOT NULL UNIQUE, and
populate/maintain it via a BEFORE INSERT OR UPDATE trigger that calls a
normalize_phone function (create normalize_phone(text) to produce canonical
E.164-style values or at least digits-only + country normalization); update the
migration to create the normalize_phone function and trigger (e.g.,
normalize_phone_trigger) so all new and updated rows store a canonical value in
phone_normalized used by the UNIQUE constraint.
---
Nitpick comments:
In `@supabase/migrations/001_add_phone_to_profiles.sql`:
- Around line 14-15: The migration unnecessarily creates indexes
idx_profiles_phone and idx_profiles_user_id on columns that already have UNIQUE
constraints; remove the redundant CREATE INDEX lines for idx_profiles_phone and
idx_profiles_user_id (the UNIQUE constraints on profiles(phone) and
profiles(user_id) already provide indexes) so the migration only leaves the
necessary schema changes without duplicating index storage and write overhead.
🪄 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: 17276209-390f-426c-9c21-420c16099cee
📒 Files selected for processing (5)
.github/workflows/deno.ymlapp.jsonapp/(tabs)/new-chat.tsxpackage.jsonsupabase/migrations/001_add_phone_to_profiles.sql
| - name: Setup repo | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Pin checkout action to hash and disable credential persistence.
For security consistency with the pinned denoland/setup-deno action, actions/checkout should also be pinned to a commit hash rather than a tag. Additionally, set persist-credentials: false to prevent the GITHUB_TOKEN from being exposed in subsequent steps where it's not needed.
🔒 Proposed security hardening
- name: Setup repo
- uses: actions/checkout@v4
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: falseAs per static analysis hints, the unpinned action reference and missing credential persistence settings were flagged by the zizmor tool.
📝 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.
| - name: Setup repo | |
| uses: actions/checkout@v4 | |
| - name: Setup repo | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 25-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 26-26: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/deno.yml around lines 25 - 26, Update the GitHub Actions
checkout step so it is pinned to a specific commit hash instead of the tag and
disable credential persistence; locate the "uses: actions/checkout@v4" line in
the workflow and replace the tag with the repo commit SHA (e.g.,
actions/checkout@<commit-hash>) and add the key "persist-credentials: false"
under that step to prevent GITHUB_TOKEN from being persisted.
| "permissions": [ | ||
| "android.permission.READ_CONTACTS", | ||
| "android.permission.READ_PHONE_STATE" | ||
| ], |
There was a problem hiding this comment.
Remove READ_PHONE_STATE unless the app really uses telephony state.
Contact syncing needs READ_CONTACTS, not phone-state access. Keeping READ_PHONE_STATE expands the privacy surface and can create avoidable consent/store-review friction.
🔒 Proposed permission trim
"permissions": [
- "android.permission.READ_CONTACTS",
- "android.permission.READ_PHONE_STATE"
+ "android.permission.READ_CONTACTS"
],📝 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.
| "permissions": [ | |
| "android.permission.READ_CONTACTS", | |
| "android.permission.READ_PHONE_STATE" | |
| ], | |
| "permissions": [ | |
| "android.permission.READ_CONTACTS" | |
| ], |
🤖 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 `@app.json` around lines 30 - 33, The permissions list in app.json
unnecessarily includes "android.permission.READ_PHONE_STATE"; remove this
permission from the "permissions" array and keep only
"android.permission.READ_CONTACTS" (or any other permissions your app actually
uses) so the app does not request telephony state; update any related
documentation or rationale comments if present to reflect the trimmed permission
set.
| {item.avatar_url ? ( | ||
| <Image source={{ uri: item.avatar_url }} style={styles.avatar} /> | ||
| ) : ( | ||
| <View style={[styles.avatar, styles.avatarPlaceholder]}> | ||
| <Text style={styles.avatarText}>{item.device_contact_name[0].toUpperCase()}</Text> | ||
| </View> |
There was a problem hiding this comment.
Guard the avatar initial for empty contact names.
This path indexes item.device_contact_name[0] unconditionally. If a synced contact has an empty or missing display name, the screen will crash while rendering the placeholder avatar.
🛡️ Proposed guard
) : (
<View style={[styles.avatar, styles.avatarPlaceholder]}>
- <Text style={styles.avatarText}>{item.device_contact_name[0].toUpperCase()}</Text>
+ <Text style={styles.avatarText}>
+ {(item.device_contact_name?.trim()?.[0] ?? '#').toUpperCase()}
+ </Text>
</View>
)}📝 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.
| {item.avatar_url ? ( | |
| <Image source={{ uri: item.avatar_url }} style={styles.avatar} /> | |
| ) : ( | |
| <View style={[styles.avatar, styles.avatarPlaceholder]}> | |
| <Text style={styles.avatarText}>{item.device_contact_name[0].toUpperCase()}</Text> | |
| </View> | |
| {item.avatar_url ? ( | |
| <Image source={{ uri: item.avatar_url }} style={styles.avatar} /> | |
| ) : ( | |
| <View style={[styles.avatar, styles.avatarPlaceholder]}> | |
| <Text style={styles.avatarText}> | |
| {(item.device_contact_name?.trim()?.[0] ?? '#').toUpperCase()} | |
| </Text> | |
| </View> | |
| )} |
🤖 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 `@app/`(tabs)/new-chat.tsx around lines 58 - 63, The placeholder avatar
rendering assumes item.device_contact_name[0] exists and crashes for empty or
missing names; update the conditional in the new-chat.tsx render block to safely
derive an initial (e.g., compute const initial = item.device_contact_name ?
item.device_contact_name.trim()[0]?.toUpperCase() : null) and render that
initial only when present, otherwise render a fallback (such as a default icon
or empty string). Locate the JSX that uses item.device_contact_name[0] (the
avatar placeholder branch) and replace the direct index access with a guarded
value using trim() and optional chaining before calling toUpperCase().
| "expo": "^50.0.2", | ||
| "expo-contacts": "^14.5.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Which expo-contactsversion is compatible with Expo SDK 50, and does Expo recommend~-pinned versions for Expo SDK packages in package.json?
💡 Result:
Expo SDK 50 compatible expo-contacts version: use expo-contacts sdk-50 tag version 12.8.2. The npm registry dist-tags for expo-contacts include “sdk-50”: “12.8.2” [1]. Does Expo recommend ~-pinned versions for Expo SDK packages in package.json? Expo’s guidance is to use expo install (and expo install --fix / --check) to realign dependency versions with the versions expected for your current Expo SDK, rather than manually guessing version ranges [2]. The dependency-reconciliation doc explains tested ranges (including that ~1.2.3 allows v1.2.3 plus a greater patch version) but it does not recommend “always use ~” specifically for Expo SDK packages; instead it emphasizes using the CLI to keep your versions aligned to the SDK expectations [2].
Citations:
- 1: https://registry.npmjs.org/expo-contacts
- 2: https://github.com/expo/fyi/blob/main/resolving-dependency-issues.md
Align expo-contacts with Expo SDK 50 (package.json, lines 21–22).
expo-contacts is set to ^14.5.0, but Expo’s SDK 50 compatible expo-contacts version is 12.8.2 (npm sdk-50 dist-tag). Update expo-contacts to the SDK 50 version (or run npx expo install --fix to reconcile automatically).
🤖 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 `@package.json` around lines 21 - 22, The package.json entry for the dependency
"expo-contacts" is incompatible with Expo SDK 50; update the "expo-contacts"
version string from "^14.5.0" to the SDK-50 compatible "12.8.2" (or run npx expo
install --fix to reconcile all Expo package versions automatically) so it
matches the installed "expo" ^50.0.2; change the "expo-contacts" value in
package.json and then reinstall (npm/yarn) to update lockfiles.
| CREATE TABLE IF NOT EXISTS public.profiles ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| user_id UUID NOT NULL UNIQUE REFERENCES auth.users(id) ON DELETE CASCADE, | ||
| phone TEXT NOT NULL UNIQUE, |
There was a problem hiding this comment.
Normalize phone numbers before enforcing uniqueness.
UNIQUE on raw TEXT will treat the same number as different values when formatting changes (+1..., spaces, dashes, local prefixes). That will make contact matching unreliable and can create duplicate logical identities. Store a canonical value such as E.164 in the unique/indexed field, or add a dedicated normalized column and query against that.
🤖 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 `@supabase/migrations/001_add_phone_to_profiles.sql` at line 5, The migration
currently adds phone TEXT NOT NULL UNIQUE which enforces uniqueness on raw
formatting; change it to add a separate normalized column (e.g.,
phone_normalized) and enforce uniqueness on that column instead: keep the
original phone TEXT for display, add phone_normalized TEXT NOT NULL UNIQUE, and
populate/maintain it via a BEFORE INSERT OR UPDATE trigger that calls a
normalize_phone function (create normalize_phone(text) to produce canonical
E.164-style values or at least digits-only + country normalization); update the
migration to create the normalize_phone function and trigger (e.g.,
normalize_phone_trigger) so all new and updated rows store a canonical value in
phone_normalized used by the UNIQUE constraint.
| -- Policy: Users can view all profiles | ||
| DROP POLICY IF EXISTS "Users can view profiles" ON public.profiles; | ||
| CREATE POLICY "Users can view profiles" | ||
| ON public.profiles FOR SELECT | ||
| USING (auth.uid() IS NOT NULL); |
There was a problem hiding this comment.
Restrict profile reads instead of exposing the whole table.
USING (auth.uid() IS NOT NULL) lets any signed-in user enumerate every profile row, including phone numbers and emails. For contact sync, reads should be limited to the caller's allowed match set via a narrower policy or an RPC that performs the phone matching server-side.
🤖 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 `@supabase/migrations/001_add_phone_to_profiles.sql` around lines 20 - 24, The
current SELECT policy "Users can view profiles" on public.profiles uses USING
(auth.uid() IS NOT NULL) which allows any signed-in user to read all profile
rows; replace it with a tighter policy so SELECT only returns the caller's own
profile (e.g., restrict USING to compare the profile's owner column to
auth.uid(), such as USING (user_id = auth.uid()) on public.profiles) and remove
or narrow any global read grants; for contact-sync functionality, implement a
server-side RPC (e.g., match_contacts) that accepts phone hashes and performs
the matching on the DB side, returning only allowed fields, instead of
broadening the SELECT policy.
1 participant
New
Summary by CodeRabbit