♻️ refactor(expo): route all platform-sensitive deps through lib/ wrappers#2564
Open
andrew-bierman wants to merge 6 commits into
Open
♻️ refactor(expo): route all platform-sensitive deps through lib/ wrappers#2564andrew-bierman wants to merge 6 commits into
andrew-bierman wants to merge 6 commits into
Conversation
Add apps/expo/lib/ wrapper modules so platform-sensitive native/expo dependencies are imported through a single seam, with .web.ts variants where web behavior diverges: - secureStore (+ .web localStorage shim — expo-secure-store throws on web) - googleSignin (+ .web stub — native-only module) - expoSqliteKvStore (+ .web localStorage shim — native SQLite has no web build) - asyncStorage (single re-export — works on web via localStorage) Add scripts/lint/no-direct-wrapped-imports.ts with a WRAPPED map that fails when any wrapped module is imported directly outside its wrapper, and wire it into lint:custom.
Replace direct imports of expo-secure-store, expo-apple-authentication, expo-updates, @react-native-async-storage/async-storage, @react-native-google-signin/google-signin, and expo-sqlite/kv-store with their apps/expo/lib/ wrappers across 19 files (call sites, jotai storage atoms, the API/auth clients, and the existing kvStorage/persist-plugin web wrappers). Behavior-preserving on native; the .web.ts variants now back web auth and storage so no Platform branches are needed.
…rapped-imports # Conflicts: # apps/expo/app/(app)/(tabs)/profile/index.tsx # apps/expo/features/ai/atoms/chatStorageAtoms.ts # apps/expo/features/auth/hooks/useAuthActions.ts # apps/expo/features/trail-conditions/hooks/useTrailConditionReports.ts # apps/expo/features/weather/screens/LocationSearchScreen.tsx # apps/expo/lib/auth-client.ts # apps/expo/lib/secureStore.ts # apps/expo/lib/secureStore.web.ts # scripts/lint/no-direct-wrapped-imports.ts
The web shim mirrors expo-sqlite/kv-store's positional (key, value) API (setItem/setItemSync/setItemAsync), same as the secureStore.web shim.
Contributor
WalkthroughThis PR abstracts Expo app storage and authentication dependencies by creating local wrapper modules ( ChangesStorage and Authentication Wrapper Abstraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ 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 |
Contributor
Coverage Report for packages/mcp (./packages/mcp)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/analytics (./packages/analytics)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/units (./packages/units)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Contributor
Coverage Report for apps/expo (./apps/expo)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
Coverage Report for packages/api (./packages/api)
File CoverageNo changed files found. |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | c9b064b | Commit Preview URL Branch Preview URL |
Jun 16 2026, 04:07 PM |
Contributor
Deploying packrat-guides with
|
| Latest commit: |
c9b064b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b1f50038.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://refactor-lib-wrapped-imports.packrat-guides-6gq.pages.dev |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/expo/features/auth/hooks/useAuthActions.ts (1)
88-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
databefore dereferencingdata.userin sign-in.Line 88 checks
erroronly, but Line 90 assumesdataexists. A{ error: null, data: null }response would throw before your intended auth error flow.Suggested fix
- if (error) throw toAuthError({ source: error, fallback: 'Sign in failed' }); - applySession(data.user as Record<string, unknown>); // safe-cast: Better Auth user type omits additionalFields; role/preferredWeightUnit present at runtime + if (error || !data?.user) { + throw toAuthError({ source: error, fallback: 'Sign in failed' }); + } + applySession(data.user as Record<string, unknown>); // safe-cast: Better Auth user type omits additionalFields; role/preferredWeightUnit present at runtimeAs per coding guidelines, "API client responses must be checked with
if (error || !data)before using the data object."🤖 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 `@apps/expo/features/auth/hooks/useAuthActions.ts` around lines 88 - 90, Guard the sign-in response before dereferencing data.user: in the code path using authClient.signIn.email(...) check for both error and missing data (if (error || !data) ...) and throw toAuthError({ source: error, fallback: 'Sign in failed' }) when either occurs; only call applySession(data.user as Record<string, unknown>) after this guard so data is guaranteed to exist. Use the existing toAuthError and applySession symbols and keep the error message/fallback consistent with current behavior.apps/expo/features/trail-conditions/hooks/useTrailConditionReports.ts (1)
60-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCapture the original API error, not a wrapped
new Error, in Sentry.Line 62 creates a new error and Line 63 captures that wrapper, which drops original error context.
Suggested fix
if (error) { console.error('Failed to fetch trail condition reports:', error.value); - const err = new Error(`Failed to fetch trail condition reports: ${String(error.value)}`); - Sentry.captureException(err, { + Sentry.captureException(error, { tags: { feature: 'trailConditions', action: 'fetchReports' }, extra: { trailName, apiError: error.value, httpStatus: error.status }, }); - throw err; + throw new Error(`Failed to fetch trail condition reports: ${String(error.value)}`); }As per coding guidelines, "Never wrap the original error in
new Error(...)before passing to Sentry'scaptureException."🤖 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 `@apps/expo/features/trail-conditions/hooks/useTrailConditionReports.ts` around lines 60 - 67, The code currently creates a wrapper Error (err) and passes that to Sentry, losing the original API error context; change Sentry.captureException to send the original error object (error.value) instead of err, while still supplying the same tags/extra (use error.status, trailName, apiError: error.value). Keep the console.error and continue to throw the existing err if you need a normalized thrown Error, but ensure Sentry.captureException(error.value, { tags: {...}, extra: {...} }) so the original error is preserved; update the Sentry call at the Sentry.captureException invocation in useTrailConditionReports.ts accordingly.
🤖 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 `@apps/expo/lib/expoSqliteKvStore.web.ts`:
- Around line 41-43: The clear() implementation currently calls
localStorage.clear(), wiping all keys; change it to only remove keys managed by
this KV store by using a unique prefix (or a stored registry) for this Storage
instance and iterating through localStorage keys to remove only those that start
with that prefix; update clear (and any removeItem/getItem/getItemSync wrappers
like clear:(), getItemSync, setItem/removeItem) to add/remove/lookup keys with
that prefix so no unrelated localStorage entries are deleted, and ensure any
existing getItemSync calls prepend the same prefix when reading.
- Around line 13-15: The web shim setItemSync currently only accepts a string
but must mirror the kv-store contract that allows an UpdateFn; update
setItemSync (and similarly setItemAsync/setItem shims if present) to accept
value: string | UpdateFn, detect when value is a function, call getItemSync(key)
to obtain the current string (treat null/undefined as null), invoke the UpdateFn
with that current value to compute the new string, and then call
localStorage.setItem(key, finalValue); ensure the function signature/overload
reflects UpdateFn and that non-function values are stored unchanged.
In `@scripts/lint/no-direct-wrapped-imports.ts`:
- Around line 78-80: The isTargetFile function currently only treats TS
extensions as targets and only strips tests/specs via a suffix regex that
ignores JS-family files; update isTargetFile to include JS/JSX/MJS/CJS
extensions (js, jsx, mjs, cjs) in the positive extension check and in the
test/spec exclusion check so filenames like *.test.js or *.spec.mjs are
excluded; locate the isTargetFile function and broaden both regexes to cover
(ts|tsx|cts|mts|js|jsx|mjs|cjs) and ensure the test/spec pattern matches before
any of those extensions.
---
Outside diff comments:
In `@apps/expo/features/auth/hooks/useAuthActions.ts`:
- Around line 88-90: Guard the sign-in response before dereferencing data.user:
in the code path using authClient.signIn.email(...) check for both error and
missing data (if (error || !data) ...) and throw toAuthError({ source: error,
fallback: 'Sign in failed' }) when either occurs; only call
applySession(data.user as Record<string, unknown>) after this guard so data is
guaranteed to exist. Use the existing toAuthError and applySession symbols and
keep the error message/fallback consistent with current behavior.
In `@apps/expo/features/trail-conditions/hooks/useTrailConditionReports.ts`:
- Around line 60-67: The code currently creates a wrapper Error (err) and passes
that to Sentry, losing the original API error context; change
Sentry.captureException to send the original error object (error.value) instead
of err, while still supplying the same tags/extra (use error.status, trailName,
apiError: error.value). Keep the console.error and continue to throw the
existing err if you need a normalized thrown Error, but ensure
Sentry.captureException(error.value, { tags: {...}, extra: {...} }) so the
original error is preserved; update the Sentry call at the
Sentry.captureException invocation in useTrailConditionReports.ts accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 20370310-5843-404a-91b6-3a72f9f2775d
📒 Files selected for processing (23)
apps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/(app)/settings/index.tsxapps/expo/app/auth/index.tsxapps/expo/atoms/atomWithAsyncStorage.tsapps/expo/atoms/atomWithKvStorage.tsapps/expo/features/ai/atoms/chatStorageAtoms.tsapps/expo/features/auth/hooks/useAuthActions.tsapps/expo/features/auth/hooks/useAuthInit.tsapps/expo/features/trail-conditions/hooks/useTrailConditionReports.tsapps/expo/features/weather/screens/LocationSearchScreen.tsxapps/expo/features/wildlife/atoms/wildlifeAtoms.tsapps/expo/lib/asyncStorage.tsapps/expo/lib/expoSqliteKvStore.tsapps/expo/lib/expoSqliteKvStore.web.tsapps/expo/lib/googleSignin.tsapps/expo/lib/googleSignin.web.tsapps/expo/lib/kvStorage.tsapps/expo/lib/kvStorage.web.tsapps/expo/lib/persist-plugin.tsapps/expo/lib/persist-plugin.web.tsapps/expo/utils/storage.tsscripts/lint/no-direct-wrapped-imports.tsscripts/lint/no-owned-max-params.ts
Comment on lines
+13
to
+15
| const setItemSync = (key: string, value: string): void => { | ||
| localStorage.setItem(key, value); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for setItemSync calls with function arguments.
# Test: Look for setItemSync calls passing callbacks or arrow functions.
rg -nP --type=ts --type=tsx 'setItemSync\s*\(\s*[^,]+,\s*(\(|function)' apps/expo/Repository: PackRat-AI/PackRat
Length of output: 90
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the target web shim around the reported lines
echo "---- File head (apps/expo/lib/expoSqliteKvStore.web.ts) ----"
nl -ba apps/expo/lib/expoSqliteKvStore.web.ts | sed -n '1,120p'
# 2) Find the declaration(s) of setItemSync
echo
echo "---- setItemSync declarations ----"
rg -n --hidden --no-ignore -S "setItemSync\s*=" apps/expo
# 3) Search for any calls that pass a function as the second argument
echo
echo "---- setItemSync calls passing function/arrow as 2nd arg ----"
rg -n --hidden --no-ignore -S "setItemSync\s*\(\s*[^,]+,\s*(\(|function|async\s*\()" apps/expo -g'*.ts' -g'*.tsx'
# 4) Also search for setItemSync calls generally (to see how it's used)
echo
echo "---- setItemSync call sites (general) ----"
rg -n --hidden --no-ignore -S "setItemSync\s*\(" apps/expo -g'*.ts' -g'*.tsx' | head -n 200Repository: PackRat-AI/PackRat
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show file with line numbers (without nl)
echo "---- File head (apps/expo/lib/expoSqliteKvStore.web.ts) ----"
awk '{printf "%d\t%s\n", NR, $0}' apps/expo/lib/expoSqliteKvStore.web.ts | sed -n '1,160p'
echo
echo "---- setItemSync declarations ----"
rg -n --hidden --no-ignore -S "setItemSync\s*=" apps/expo -g'*.ts' -g'*.tsx'
echo
echo "---- setItemSync calls passing function/arrow as 2nd arg ----"
rg -n --hidden --no-ignore -S 'setItemSync\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' apps/expo -g'*.ts' -g'*.tsx'
echo
echo "---- setItemSync call sites (general) ----"
rg -n --hidden --no-ignore -S 'setItemSync\s*\(' apps/expo -g'*.ts' -g'*.tsx' | head -n 200Repository: PackRat-AI/PackRat
Length of output: 2279
🏁 Script executed:
#!/bin/bash
set -u
file="apps/expo/lib/expoSqliteKvStore.web.ts"
echo "---- $file (relevant snippet) ----"
awk 'NR>=1 && NR<=120 {printf "%d\t%s\n", NR, $0}' "$file"
echo
echo "---- setItemSync calls passing function/arrow as 2nd arg (should be empty) ----"
rg -n --hidden --no-ignore -S 'setItemSync\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' apps/expo -g'*.ts' -g'*.tsx' || true
echo
echo "---- setItemSync call sites (2nd arg types) ----"
rg -n --hidden --no-ignore -S 'setItemSync\s*\(' apps/expo -g'*.ts' -g'*.tsx' || true
echo
echo "---- Storage.setItemSync call sites ----"
rg -n --hidden --no-ignore -S 'Storage\.(setItemSync)\s*\(' apps/expo -g'*.ts' -g'*.tsx' || true
echo
echo "---- Any UpdateFn-like usage for setItemSync ----"
rg -n --hidden --no-ignore -S '(UpdateFn|prev\s*=>|prev\s*\))' apps/expo -g'*.ts' -g'*.tsx' || trueRepository: PackRat-AI/PackRat
Length of output: 6353
🏁 Script executed:
#!/bin/bash
set -u
echo "---- apps/expo/lib/kvStorage.ts ----"
awk 'NR>=1 && NR<=200 {printf "%d\t%s\n", NR, $0}' apps/expo/lib/kvStorage.ts
echo
echo "---- apps/expo/mocks/expo-sqlite-kv-store.ts (UpdateFn + setItemSync/setItem patterns) ----"
awk 'NR>=1 && NR<=220 {printf "%d\t%s\n", NR, $0}' apps/expo/mocks/expo-sqlite-kv-store.ts
echo
echo "---- Call sites: setItemSync with function/arrow as 2nd arg (should be empty) ----"
rg -n --hidden --no-ignore -S 'setItemSync\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' apps/expo -g'*.ts' -g'*.tsx' || true
echo
echo "---- Call sites: Storage.setItemSync with function/arrow as 2nd arg ----"
rg -n --hidden --no-ignore -S 'Storage\.setItemSync\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' apps/expo -g'*.ts' -g'*.tsx' || true
echo
echo "---- Call sites: setItem* with function/arrow as 2nd arg (any) ----"
rg -n --hidden --no-ignore -S 'setItem(?:Async|Sync)?\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' apps/expo -g'*.ts' -g'*.tsx' || true
echo
echo "---- Call sites: Storage.setItem* with function/arrow as 2nd arg ----"
rg -n --hidden --no-ignore -S 'Storage\.setItem(?:Async|Sync)?\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' apps/expo -g'*.ts' -g'*.tsx' || true
echo
echo "---- Call sites: setItem* where 2nd arg is an arrow function specifically (prev => ...) ----"
rg -n --hidden --no-ignore -S 'setItem(?:Async|Sync)?\s*\(\s*[^,]+,\s*[^,=]*=>\s*' apps/expo -g'*.ts' -g'*.tsx' || trueRepository: PackRat-AI/PackRat
Length of output: 5905
🏁 Script executed:
#!/bin/bash
set -u
echo "---- Find UpdateFn definitions/usages in apps/expo/lib ----"
rg -n --hidden --no-ignore -S "UpdateFn" apps/expo/lib
echo
echo "---- Inspect expoSqliteKvStore type surface (files named expoSqliteKvStore.*) ----"
fd -a -t f "expoSqliteKvStore" apps/expo/lib apps/expo -E node_modules
echo
echo "---- Print expoSqliteKvStore files (small regions around setItem signatures) ----"
for f in $(fd -a -t f "expoSqliteKvStore" apps/expo/lib apps/expo -E node_modules); do
echo "===== $f (setItem signatures) ====="
rg -n --hidden --no-ignore -S "setItem(Async|Sync)?\s*\(|setItem\s*:\s*|UpdateFn|prevValue" "$f" || true
echo "===== $f (around setItemSync) ====="
rg -n --hidden --no-ignore -S "setItemSync" "$f" | head -n 20 || true
doneRepository: PackRat-AI/PackRat
Length of output: 2007
🏁 Script executed:
#!/bin/bash
set -u
file="apps/expo/lib/expoSqliteKvStore.ts"
echo "---- $file (first 220 lines) ----"
awk 'NR<=220 {printf "%d\t%s\n", NR, $0}' "$file" | head -n 220
echo
echo "---- $file: lines containing setItem/setItemSync/UpdateFn ----"
rg -n --hidden --no-ignore -S "setItemSync|setItemAsync|setItem\s*\(|UpdateFn|prevValue|UpdateFn|value:\s*string" "$file"Repository: PackRat-AI/PackRat
Length of output: 626
🏁 Script executed:
#!/bin/bash
set -u
echo "---- Locate expo-sqlite/kv-store type files ----"
rg -n --hidden --no-ignore -S "expo-sqlite/kv-store" package.json apps/expo -g'package.json' -g'*.ts' -g'*.tsx' || true
echo
echo "---- Search node_modules for UpdateFn and setItem signatures ----"
rg -n --hidden --no-ignore -S "type UpdateFn|interface.*UpdateFn|UpdateFn\s*=\s*\(|UpdateFn" node_modules -g'*.ts' -g'*.d.ts' -g'*.tsx' 2>/dev/null | head -n 50 || true
echo
rg -n --hidden --no-ignore -S "setItemSync\s*\(" node_modules -g'*.d.ts' -g'*.ts' 2>/dev/null | head -n 50 || true
echo
rg -n --hidden --no-ignore -S "setItem\s*\(" node_modules -g'*.d.ts' -g'*.ts' 2>/dev/null | head -n 50 || true
echo
echo "---- If present, print the kv-store .d.ts around setItem/setItemSync ----"
kvpath=$(rg -l --hidden --no-ignore -S "setItemSync\s*\(" node_modules -g'*.d.ts' 2>/dev/null | rg -m 1 "kv-store" || true)
echo "kvpath=$kvpath"
if [ -n "${kvpath:-}" ]; then
sed -n '1,200p' "$kvpath" | cat
fiRepository: PackRat-AI/PackRat
Length of output: 581
🏁 Script executed:
#!/bin/bash
set -u
echo "---- setItemSync calls passing function/arrow as 2nd arg (repo-wide) ----"
rg -n --hidden --no-ignore -S 'setItemSync\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' . -g'*.ts' -g'*.tsx' || true
echo
echo "---- setItemAsync calls passing function/arrow as 2nd arg (repo-wide) ----"
rg -n --hidden --no-ignore -S 'setItemAsync\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' . -g'*.ts' -g'*.tsx' || true
echo
echo "---- Storage.setItemSync / Storage.setItemAsync / Storage.setItem calls passing arrow/function (repo-wide) ----"
rg -n --hidden --no-ignore -S '(Storage|AsyncStorage)\.setItem(?:Sync|Async)?\s*\(\s*[^,]+,\s*(\(|function|async\s*\()' . -g'*.ts' -g'*.tsx' || true
echo
echo "---- setItem* where 2nd arg contains an arrow '=>' (repo-wide; may include other setState calls) ----"
rg -n --hidden --no-ignore -S 'setItem(?:Sync|Async)?\s*\(\s*[^,]+,\s*\(?\s*[^)]*=>\s*' . -g'*.ts' -g'*.tsx' || trueRepository: PackRat-AI/PackRat
Length of output: 428
Align web setItemSync with UpdateFn-capable kv-store contract
No call sites pass a functional updater to setItemSync/setItemAsync/setItem in this repo, so nothing breaks today. However, apps/expo/mocks/expo-sqlite-kv-store.ts supports setItemSync(key, value: string | UpdateFn) while apps/expo/lib/expoSqliteKvStore.web.ts only accepts value: string, creating a contract mismatch—add the UpdateFn overload to the web shim and derive the final value from getItemSync(key) when value is a function.
🤖 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 `@apps/expo/lib/expoSqliteKvStore.web.ts` around lines 13 - 15, The web shim
setItemSync currently only accepts a string but must mirror the kv-store
contract that allows an UpdateFn; update setItemSync (and similarly
setItemAsync/setItem shims if present) to accept value: string | UpdateFn,
detect when value is a function, call getItemSync(key) to obtain the current
string (treat null/undefined as null), invoke the UpdateFn with that current
value to compute the new string, and then call localStorage.setItem(key,
finalValue); ensure the function signature/overload reflects UpdateFn and that
non-function values are stored unchanged.
Comment on lines
+41
to
+43
| clear: (): Promise<void> => { | ||
| localStorage.clear(); | ||
| return Promise.resolve(); |
Contributor
There was a problem hiding this comment.
Critical: clear() destroys all localStorage data, not just KV store scope.
Native clearSync() only removes keys in the isolated SQLite table. This web shim calls localStorage.clear(), which wipes everything in localStorage—breaking unrelated features that persist data there.
🛡️ Proposed fix to scope clear() to managed keys
Track a prefix or registry of managed keys. For now, iterate all keys and remove only those set via this Storage instance:
+const STORAGE_PREFIX = '__expo_kv_';
+
const setItemSync = (key: string, value: string): void => {
- localStorage.setItem(key, value);
+ localStorage.setItem(STORAGE_PREFIX + key, value);
};
const removeItemSync = (key: string): void => {
- localStorage.removeItem(key);
+ localStorage.removeItem(STORAGE_PREFIX + key);
};
const getAllKeysSync = (): string[] => {
const keys: string[] = [];
for (let i = 0; i < localStorage.length; i++) {
const key = localStorage.key(i);
- if (key !== null) keys.push(key);
+ if (key !== null && key.startsWith(STORAGE_PREFIX)) {
+ keys.push(key.slice(STORAGE_PREFIX.length));
+ }
}
return keys;
};
// ... in Storage object:
clear: (): Promise<void> => {
- localStorage.clear();
+ const keys = getAllKeysSync();
+ keys.forEach(k => removeItemSync(k));
return Promise.resolve();
},Adjust getItemSync similarly to prepend prefix.
🤖 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 `@apps/expo/lib/expoSqliteKvStore.web.ts` around lines 41 - 43, The clear()
implementation currently calls localStorage.clear(), wiping all keys; change it
to only remove keys managed by this KV store by using a unique prefix (or a
stored registry) for this Storage instance and iterating through localStorage
keys to remove only those that start with that prefix; update clear (and any
removeItem/getItem/getItemSync wrappers like clear:(), getItemSync,
setItem/removeItem) to add/remove/lookup keys with that prefix so no unrelated
localStorage entries are deleted, and ensure any existing getItemSync calls
prepend the same prefix when reading.
Comment on lines
+78
to
80
| function isTargetFile(name: string): boolean { | ||
| return /\.(ts|tsx|cts|mts)$/.test(name) && !/\.(test|spec)\.(ts|tsx|cts|mts)$/.test(name); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether JS-family files exist under apps/expo and can currently bypass this lint.
fd -t f -e js -e jsx -e mjs -e cjs . apps/expo \
| rg -n -v '(app\.config\.js|metro\.config\.js|\.test\.|\.spec\.)'
# Check for direct wrapped imports in apps/expo across JS/TS files.
rg -nP "(?:from|import)\s+['\"](expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)(?:/[^'\"]*)?['\"]" apps/expo \
-g '!**/*.test.*' -g '!**/*.spec.*'Repository: PackRat-AI/PackRat
Length of output: 577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the lint script around isTargetFile + call site to confirm what gets passed.
echo "=== scripts/lint/no-direct-wrapped-imports.ts (relevant region) ==="
sed -n '60,160p' scripts/lint/no-direct-wrapped-imports.ts
# 2) JS-only search: files with .js/.jsx/.mjs/.cjs importing the flagged packages.
echo
echo "=== JS-family files under apps/expo that import/export flagged modules ==="
rg -nP --glob '**/*.{js,jsx,mjs,cjs}' \
"(?:from|import)\s+['\"](expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)(?:/[^'\"]*)?['\"]" \
apps/expo \
--glob '!**/*.test.*' \
--glob '!**/*.spec.*'
# 3) Path-based test dir check (e.g., __tests__/...) for flagged imports in any JS/TS.
echo
echo "=== Flagged imports inside __tests__/ under apps/expo (any language) ==="
rg -nP \
"(?:from|import)\s+['\"](expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)(?:/[^'\"]*)?['\"]" \
apps/expo \
--glob '**/__tests__/**'Repository: PackRat-AI/PackRat
Length of output: 3198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Helper: don't fail on no matches
rg_no_fail() {
# usage: rg_no_fail <rg_args...>
rg "$@" || true
}
echo "=== JS-family files under apps/expo (for context) ==="
fd -t f . apps/expo -e js -e jsx -e mjs -e cjs | wc -l
echo
echo "=== Flagged-module imports in JS-family files under apps/expo (no fail on no matches) ==="
rg_no_fail -nP \
--glob '**/*.{js,jsx,mjs,cjs}' \
"(?:from|import)\s+['\"](expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)(?:/[^'\"]*)?['\"]" \
apps/expo \
--glob '!**/*.test.*' \
--glob '!**/*.spec.*'
echo
echo "=== Flagged-module imports inside __tests__/ under apps/expo (no fail on no matches) ==="
rg_no_fail -nP \
"(?:from|import)\s+['\"](expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)(?:/[^'\"]*)?['\"]" \
apps/expo \
--glob '**/__tests__/**'Repository: PackRat-AI/PackRat
Length of output: 414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
MOD_PATTERN='(expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)'
# Find all files in apps/expo that directly import the wrapped modules
echo "=== Files with direct imports of wrapped modules under apps/expo ==="
rg -lP "(?:from|import)\s+['\"]${MOD_PATTERN}(?:/[^'\"]*)?['\"]" apps/expo || true
# Evaluate whether lint would treat each matched file as a "target" based on current isTargetFile logic
# isTargetFile(name): ts/tsx/cts/mts AND NOT .(test|spec).(ts|tsx|cts|mts)
echo
echo "=== Lint inclusion check (simulating isTargetFile on basename) ==="
python3 - <<'PY'
import os, re, subprocess, shlex, sys
mod_pattern = '(expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)'
cmd = ["rg","-lP",f"(?:from|import)\\s+['\"]{mod_pattern}(?:/[^'\"]*)?['\"]","apps/expo"]
files = subprocess.check_output(cmd, text=True).splitlines()
if not files:
print("(no matches)")
sys.exit(0)
def is_target(name: str) -> bool:
return bool(re.search(r"\.(ts|tsx|cts|mts)$", name)) and not bool(re.search(r"\.(test|spec)\.(ts|tsx|cts|mts)$", name))
for p in sorted(files):
base = os.path.basename(p)
print(f"{p} -> isTargetFile(basename={base!r})={is_target(base)}")
PY
# Also specifically check for direct imports in JS-family files (to validate the bypass)
echo
echo "=== JS-family files with direct imports of wrapped modules (if any) ==="
rg -nP "(?:from|import)\s+['\"]${MOD_PATTERN}(?:/[^'\"]*)?['\"]" apps/expo \
--glob '**/*.{js,jsx,mjs,cjs}' --glob '!**/*.test.*' --glob '!**/*.spec.*' || trueRepository: PackRat-AI/PackRat
Length of output: 1078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== scripts/lint/no-direct-wrapped-imports.ts (top: EXPO_ROOT + WRAPPED) ==="
sed -n '1,120p' scripts/lint/no-direct-wrapped-imports.ts
python3 - <<'PY'
import re, subprocess, os, pathlib, json
path = 'scripts/lint/no-direct-wrapped-imports.ts'
txt = pathlib.Path(path).read_text('utf-8')
# Extract wrapperFiles string literals like 'lib/googleSignin.ts'
wrapper_files = set(re.findall(r"wrapperFiles:\s*\[\s*([^\]]+)\s*\]", txt, flags=re.S))
# The above regex grabs whole arrays; do a simpler pass for 'lib/...'
wrapper_file_literals = set(re.findall(r"'lib/[^']+'", txt))
wrapper_file_literals_web = set(re.findall(r"'lib/[^']+\.web\.ts'", txt))
wrapper_file_set = set(x.strip("'") for x in wrapper_file_literals)
# Build import regex for direct imports of wrapped modules
modules = [
'expo-secure-store',
'expo-apple-authentication',
'expo-updates',
'`@react-native-async-storage/async-storage`',
'`@react-native-google-signin/google-signin`',
'expo-sqlite/kv-store',
]
mod_pat = '(' + '|'.join(map(re.escape, modules)) + ')'
rg_cmd = ["rg","-lP",f"(?:from|import)\\s+['\"]{mod_pat}(?:/[^'\"]*)?['\"]","apps/expo"]
files = subprocess.check_output(rg_cmd, text=True).splitlines()
print("\n=== Direct-import files found (apps/expo) ===")
print("\n".join(files) if files else "(no matches)")
def is_target(basename: str) -> bool:
return bool(re.search(r"\.(ts|tsx|cts|mts)$", basename)) and not bool(re.search(r"\.(test|spec)\.(ts|tsx|cts|mts)$", basename))
def rel_entry(p: str) -> str:
# lint uses entryRel relative to EXPO_ROOT (apps/expo), which is the path without the prefix
return p.removeprefix('apps/expo/')
# Determine which direct-import files would actually be considered target and non-wrapper by the lint
violating_candidates = []
for f in files:
base = os.path.basename(f)
entry_rel = rel_entry(f)
if not is_target(base):
continue
if entry_rel in wrapper_file_set:
continue
violating_candidates.append((f, base, entry_rel))
print("\n=== Would the current lint flag any non-wrapper direct-import files? ===")
if not violating_candidates:
print("(no)")
else:
for f, base, entry_rel in violating_candidates:
print(f"- {f} (basename={base}, entryRel={entry_rel})")
# List JS-family files under apps/expo for context
fd_cmd = ["fd","-t","f",".","apps/expo","-e","js","-e","jsx","-e","mjs","-e","cjs"]
js_files = subprocess.check_output(fd_cmd, text=True).splitlines()
print("\n=== JS/JSX/MJS/CJS files under apps/expo ===")
print("\n".join(js_files) if js_files else "(none)")
PY
echo
echo "=== Direct imports restricted to JS-family files (sanity) ==="
rg -nP "(?:from|import)\s+['\"](expo-secure-store|expo-apple-authentication|expo-updates|`@react-native-async-storage/async-storage`|`@react-native-google-signin/google-signin`|expo-sqlite/kv-store)(?:/[^'\"]*)?['\"]" \
apps/expo --glob '**/*.{js,jsx,mjs,cjs}' --glob '!**/*.test.*' --glob '!**/*.spec.*' || trueRepository: PackRat-AI/PackRat
Length of output: 4812
Loosened enforcement isn’t needed right now, but hardening isTargetFile is still worthwhile.
The current rule already finds no violations in apps/expo: direct imports of the wrapped modules only occur in the wrapper files under apps/expo/lib/* (exempted) and in apps/expo/utils/__tests__/storage.test.ts (excluded by the existing .test.ts suffix). There are no direct wrapped-module imports in apps/expo JS/JSX/MJS/CJS sources (only config files in that extension set).
Still, isTargetFile currently ignores .js/.jsx/.mjs/.cjs entirely and does suffix-based test/spec filtering only on the basename, so future .js usage or test layouts not using .test/.spec suffixes could bypass the rule. Consider the broader detection below.
🔧 Proposed fix (optional hardening)
-function isTargetFile(name: string): boolean {
- return /\.(ts|tsx|cts|mts)$/.test(name) && !/\.(test|spec)\.(ts|tsx|cts|mts)$/.test(name);
+function isTargetFile(relPath: string): boolean {
+ const isSource = /\.(ts|tsx|cts|mts|js|jsx|cjs|mjs)$/.test(relPath);
+ const isTestLike =
+ /(?:^|\/)__tests__(?:\/|$)/.test(relPath) ||
+ /\.(test|spec)\.(ts|tsx|cts|mts|js|jsx|cjs|mjs)$/.test(relPath);
+ return isSource && !isTestLike;
}
@@
- if (!isTargetFile(entry)) continue;
+ if (!isTargetFile(entryRel)) continue;🤖 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 `@scripts/lint/no-direct-wrapped-imports.ts` around lines 78 - 80, The
isTargetFile function currently only treats TS extensions as targets and only
strips tests/specs via a suffix regex that ignores JS-family files; update
isTargetFile to include JS/JSX/MJS/CJS extensions (js, jsx, mjs, cjs) in the
positive extension check and in the test/spec exclusion check so filenames like
*.test.js or *.spec.mjs are excluded; locate the isTargetFile function and
broaden both regexes to cover (ts|tsx|cts|mts|js|jsx|mjs|cjs) and ensure the
test/spec pattern matches before any of those extensions.
The Web E2E job built the web app against a remote dev API
(secrets.EXPO_PUBLIC_API_URL) and seeded the real dev DB. The remote API
was unreachable from the runner, so Better Auth sign-in failed at
globalSetup.ts ("Failed to fetch", status 0) and the whole job went red.
Rework it to stand up the full stack locally, mirroring the Maestro job
and the documented local web-e2e setup:
- Start Postgres (pgvector) + the Neon HTTP proxy via
packages/api/docker-compose.test.yml. The proxy lets wrangler dev's
@neondatabase/serverless HTTP driver hit local Postgres on :4444 — the
same driver path as prod, reliable under the web flow's query load
(verified locally: repeated sign-ins 200, no DB timeouts).
- Migrations + seed connect to the raw Postgres port (:5433) so
migrate.ts uses the node-postgres driver; db.localtest.me:4444 is the
HTTP proxy, not the pg wire protocol. Both target the same database.
- Write .dev.vars with ENVIRONMENT=development so Better Auth trusts
http://localhost:* origins (web app is served cross-origin on :8081),
and NEON_DATABASE_URL pointed at the proxy.
- Build the web bundle against the local API (http://localhost:8787),
serve on :8081, run Playwright against it.
- Drop the NEON_DEV_DATABASE_URL dependency from the e2e-gate; the stack
is fully local now. Also trigger on packages/api/** changes.
Verified locally end-to-end: docker stack up, migrate + seed via raw pg,
wrangler dev healthy, and cross-origin POST /api/auth/sign-in/email
returns 200 with Access-Control-Allow-Origin + an HttpOnly
better-auth.session_token cookie.
Contributor
Deploying packrat-landing with
|
| Latest commit: |
c9b064b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://68189c6f.packrat-landing.pages.dev |
| Branch Preview URL: | https://refactor-lib-wrapped-imports.packrat-landing.pages.dev |
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 (2)
apps/expo/features/auth/hooks/useAuthActions.ts (1)
259-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard sign-out cleanup so the flow can’t dead-end.
Line 283 awaits
clearLocalData()insidefinally. If cleanup fails,signOut()rejects before the caller can present post-sign-out actions, while Line 263 has already set redirecting state totrue. This can leave logout UX/state stuck on failure paths.Proposed fix
} finally { userStore.set(null); - await clearLocalData(); + try { + await clearLocalData(); + } catch (cleanupError) { + Sentry.captureException(cleanupError, { + tags: { auth_action: 'sign_out', phase: 'cleanup' }, + }); + } setNeedsReauth(false); // isLoadingAtom intentionally left true — the caller (profile/handleSignOut)🤖 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 `@apps/expo/features/auth/hooks/useAuthActions.ts` around lines 259 - 289, The signOut function's finally block awaits clearLocalData() without error handling. If clearLocalData() rejects, the entire signOut promise will reject, preventing the caller from presenting post-sign-out actions while setIsSignOutRedirecting has already been set to true, leaving the UX stuck. Guard the clearLocalData() call in the finally block with a try-catch (or .catch()) to ensure any cleanup failures are caught silently or logged without rejecting the signOut flow, allowing the caller to proceed with post-sign-out actions regardless of cleanup success.apps/expo/features/auth/hooks/useAuthInit.ts (1)
154-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a Sentry helper call in the background refresh catch path.
The catch block at Lines 154-161 currently swallows transient failures without a breadcrumb or capture, which removes diagnostic visibility for auth-init failures.
Proposed fix
.catch((error) => { + Sentry.addBreadcrumb({ + category: 'auth', + message: 'Background session refresh failed', + level: 'warning', + }); if (isDefinitiveAuthFailure(error)) { Sentry.setUser(null); userStore.set(null); router.replace('/auth'); } // Network/transient error — keep cached user silently });As per coding guidelines, “In apps/expo, import Sentry instrumentation from
@sentry/react-nativeand call helpers before async operations and in all catch blocks.”🤖 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 `@apps/expo/features/auth/hooks/useAuthInit.ts` around lines 154 - 161, Add Sentry instrumentation to the catch block in useAuthInit.ts to capture diagnostic visibility for transient auth failures. In the catch handler around lines 154-161, after the isDefinitiveAuthFailure check, add a Sentry helper call (such as captureException or addBreadcrumb) for the transient error case before the comment about keeping the cached user silently. Ensure Sentry is imported from `@sentry/react-native` at the top of the file, and call the appropriate Sentry method to log the transient error without treating it as a definitive auth failure.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@apps/expo/features/auth/hooks/useAuthActions.ts`:
- Around line 259-289: The signOut function's finally block awaits
clearLocalData() without error handling. If clearLocalData() rejects, the entire
signOut promise will reject, preventing the caller from presenting post-sign-out
actions while setIsSignOutRedirecting has already been set to true, leaving the
UX stuck. Guard the clearLocalData() call in the finally block with a try-catch
(or .catch()) to ensure any cleanup failures are caught silently or logged
without rejecting the signOut flow, allowing the caller to proceed with
post-sign-out actions regardless of cleanup success.
In `@apps/expo/features/auth/hooks/useAuthInit.ts`:
- Around line 154-161: Add Sentry instrumentation to the catch block in
useAuthInit.ts to capture diagnostic visibility for transient auth failures. In
the catch handler around lines 154-161, after the isDefinitiveAuthFailure check,
add a Sentry helper call (such as captureException or addBreadcrumb) for the
transient error case before the comment about keeping the cached user silently.
Ensure Sentry is imported from `@sentry/react-native` at the top of the file, and
call the appropriate Sentry method to log the transient error without treating
it as a definitive auth failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d827074-9414-4f51-b966-6e90c27185c9
📒 Files selected for processing (5)
apps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/(app)/settings/index.tsxapps/expo/features/auth/hooks/useAuthActions.tsapps/expo/features/auth/hooks/useAuthInit.tsscripts/lint/no-owned-max-params.ts
💤 Files with no reviewable changes (1)
- scripts/lint/no-owned-max-params.ts
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
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.
What
Extends the
apps/expo/lib/wrapper convention to every platform-sensitive native dependency, so call sites import one cross-platform path instead of the raw module — noPlatform.OSbranches, andexpo export -p webkeeps working.Establishes the rule introduced in #2366 (which wrapped
expo-secure-store,expo-apple-authentication,expo-updates) and applies it consistently across the app.New wrappers
@react-native-async-storage/async-storagelib/asyncStorage.tsexpo-sqlite/kv-storelib/expoSqliteKvStore.ts(+.web.ts)@react-native-google-signin/google-signinlib/googleSignin.ts(+.web.ts)Enforcement
scripts/lint/no-direct-wrapped-imports.tsupgraded from a 3-entry map to a structuredWrappedEntry[]covering all 6 wrapped modules, each with its own exempt wrapper files. A direct import of any wrapped module anywhere inapps/expo(outside the wrapper + tests) now failslint:custom. This is what makes the convention self-sustaining: "if something goes intolib/, everyone useslib/."scripts/lint/no-owned-max-params.tsexempts the two web shims that must mirror their native module's positional(key, value)API.Call sites routed through wrappers (16)
atomWith{Async,Kv}Storage,kvStorage(.web),persist-plugin(.web),utils/storage,wildlifeAtoms,chatStorageAtoms,useAuthActions,useAuthInit,settings/index,auth/index,profile/index,useTrailConditionReports,LocationSearchScreen.Risk
Near-zero. On native, every wrapper re-exports the identical module, so runtime behavior is unchanged. The web shims are additive. Verified:
bun run lint:custom(all 8 checks, incl.no-direct-wrapped-imports)bunx expo export -p webbuilds cleanMinor note for reviewers
expoSqliteKvStore.web.ts'sclear()callslocalStorage.clear()(broader than native kv-store's isolated-table clear). It is currently unused — all.clear()call sites go throughAsyncStorage, whose web behavior is untouched by this PR — so it's a theoretical-only sharp edge. Flagging in case a future web caller wires it up.Context
Pairs with the planned
@packrat/apppackage extraction — a single wrapped import surface per dependency makes that extraction mechanical rather than a platform-branch untangle.🤖 Generated with Claude Code
Summary by CodeRabbit