Bail on animations for now, clean up some other stuff#862
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe mobile app configuration is migrated from app.json to app.config.js/ts format, API base URL resolution logic is simplified to use environment variables directly, react-native-worklets babel plugin is added, environment validation schemas are streamlined, and dependencies including dotenv and expo-system-ui are introduced while enforcing consistent code formatting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@apps/mobile/app.config.js`:
- Line 49: Fix the typo in the app config's owner field by changing the value of
the owner property in the exported config object from "supserset-sh" to
"superset-sh" so EAS builds and updates target the correct project; locate the
owner key in apps/mobile/app.config.js (the owner property in the
config/exported object) and update its string value accordingly.
In `@apps/mobile/lib/auth/client.ts`:
- Around line 4-7: The baseURL passed into createAuthClient via
env.EXPO_PUBLIC_API_URL can contain a trailing slash which leads to double-slash
auth paths; fix by normalizing the env value (in env.ts) using z.url().transform
to strip a trailing slash (e.g., replace(/\/$/, '')) so authClient is always
initialized with a URL without a trailing slash; update the z.url() schema for
EXPO_PUBLIC_API_URL rather than changing auth/client.ts.
🧹 Nitpick comments (1)
apps/mobile/uniwind-types.d.ts (1)
1-10: Auto-generated file modified.This file notes it "should not be edited manually." These formatting changes may be overwritten when uniwind regenerates the file. Consider adding this file to Biome's ignore list if the formatter is causing conflicts with the generated output.
| projectId: "fa9332a8-896a-4d2a-be5b-d82469b46e5d", | ||
| }, | ||
| }, | ||
| owner: "supserset-sh", |
There was a problem hiding this comment.
Typo in owner field.
The owner is set to "supserset-sh" but based on the repository organization (superset-sh/superset), this should likely be "superset-sh". This typo would cause EAS builds and updates to fail or target the wrong project.
🐛 Proposed fix
- owner: "supserset-sh",
+ owner: "superset-sh",📝 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.
| owner: "supserset-sh", | |
| owner: "superset-sh", |
🤖 Prompt for AI Agents
In `@apps/mobile/app.config.js` at line 49, Fix the typo in the app config's owner
field by changing the value of the owner property in the exported config object
from "supserset-sh" to "superset-sh" so EAS builds and updates target the
correct project; locate the owner key in apps/mobile/app.config.js (the owner
property in the config/exported object) and update its string value accordingly.
| import { env } from "../env"; | ||
|
|
||
| export const authClient = createAuthClient({ | ||
| baseURL: BASE_URL, | ||
| baseURL: env.EXPO_PUBLIC_API_URL, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find env.ts in apps/mobile
find apps/mobile -name "env.ts" -o -name "env.js" -o -name "env.tsx"Repository: superset-sh/superset
Length of output: 86
🏁 Script executed:
# Check the current content of apps/mobile/lib/auth/client.ts
cat apps/mobile/lib/auth/client.tsRepository: superset-sh/superset
Length of output: 520
🏁 Script executed:
# Look for base-url.ts to understand the old implementation
find . -name "base-url.ts" -o -name "base-url.js"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check git history to see what was removed (focusing on base-url)
git log --all --full-history -p -- "**/base-url.ts" | head -100Repository: superset-sh/superset
Length of output: 2066
🏁 Script executed:
# View the env.ts file to see z.url() implementation
cat apps/mobile/lib/env.tsRepository: superset-sh/superset
Length of output: 791
🏁 Script executed:
# Check if base-url.ts currently exists
ls -la apps/mobile/lib/base-url.ts 2>&1Repository: superset-sh/superset
Length of output: 138
🏁 Script executed:
# More comprehensive git history for base-url.ts (show all changes)
git log --all --oneline -- "apps/mobile/lib/base-url.ts" | head -20Repository: superset-sh/superset
Length of output: 124
🏁 Script executed:
# Check git status and recent removals in this directory
git log --all --oneline -- "apps/mobile/lib/" | head -30Repository: superset-sh/superset
Length of output: 136
🏁 Script executed:
# Check the most recent state - see if base-url.ts was removed in a later commit
git log --all -p -- "apps/mobile/lib/base-url.ts" | head -200Repository: superset-sh/superset
Length of output: 2066
🏁 Script executed:
# Look for recent commits that might have removed it
git log --all --oneline -20Repository: superset-sh/superset
Length of output: 136
🏁 Script executed:
# Check what files are in apps/mobile/lib/
ls -la apps/mobile/lib/Repository: superset-sh/superset
Length of output: 401
🏁 Script executed:
# Check what commit e699cc7 did - specifically what it removed
git show e699cc7 --statRepository: superset-sh/superset
Length of output: 50376
🏁 Script executed:
# Get the full diff for e699cc7 to see if it removed base-url.ts
git show e699cc7Repository: superset-sh/superset
Length of output: 50380
🏁 Script executed:
# Search the entire codebase for trailing slash normalization patterns
rg 'replace.*/$' apps/mobile/ --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Also check for slash normalization in different ways
rg 'trailing|slash.*replace|normalize.*url' apps/mobile/ --type ts --type tsx -iRepository: superset-sh/superset
Length of output: 91
🌐 Web query:
better-auth baseURL trailing slash handling double slash
💡 Result:
Short answer — better-auth expects a normalized baseURL (no trailing path slash) and can be confused by double-slashes coming from baseURL + basePath or from un-normalized incoming requests; set BETTER_AUTH_URL / baseURL without a trailing slash, set basePath starting with a single leading slash, and ensure your proxy/host normalizes or forwards correct host/proto headers. Also upgrade to a fixed release (1.4.2+ and 1.4.5+ for later rou3 fixes) because older versions had baseURL/double-slash-related issues. [1][2][3]
Practical checklist
- Explicitly set baseURL (or BETTER_AUTH_URL) — use e.g. https://example.com (no trailing slash). [4]
- Use basePath with a single leading slash: e.g. basePath: "/api/auth". Avoid concatenating strings that produce "//".
- If you must build URLs, join paths safely (remove trailing slash from baseURL or use URL/path join helpers) to avoid double-slash.
- Ensure your proxy (NGINX/Vercel/Cloudflare, etc.) collapses multiple slashes and forwards correct X-Forwarded-Host / X-Forwarded-Proto, or configure better-auth to read a trusted baseURL so it cannot be poisoned by the first incoming request. (Older versions allowed an external request to set baseURL when it was unset.) [2][3]
- Upgrade better-auth to a version that includes fixes: vulnerability/fixes referenced in advisories (upgrade to >=1.4.2 for baseURL poisoning fix and >=1.4.5 for rou3 double-slash normalization fix). [2][5]
Sources
- better-auth docs — options/baseURL and BETTER_AUTH_URL. [4]
- Security advisories describing baseURL-poisoning and rou3 double-slash issues and fixed versions. [2][3][5]
Ensure baseURL doesn't have trailing slash to avoid better-auth issues.
The z.url() validator in env.ts does not normalize trailing slashes. If EXPO_PUBLIC_API_URL is set with a trailing slash (e.g., https://api.example.com/), it will be passed directly to createAuthClient, which expects a normalized URL without trailing slashes. This can result in double slashes in auth request paths (e.g., https://api.example.com//auth), causing request failures. better-auth's own documentation and security advisories explicitly warn against this pattern.
Either:
- Add trailing slash normalization to
env.ts(e.g.,z.url().transform(url => url.replace(/\/$/, ''))), or - Document that
EXPO_PUBLIC_API_URLmust not include a trailing slash.
🤖 Prompt for AI Agents
In `@apps/mobile/lib/auth/client.ts` around lines 4 - 7, The baseURL passed into
createAuthClient via env.EXPO_PUBLIC_API_URL can contain a trailing slash which
leads to double-slash auth paths; fix by normalizing the env value (in env.ts)
using z.url().transform to strip a trailing slash (e.g., replace(/\/$/, '')) so
authClient is always initialized with a URL without a trailing slash; update the
z.url() schema for EXPO_PUBLIC_API_URL rather than changing auth/client.ts.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
force-pushed
the
mobile-animations
branch
from
e699cc7 to
92f9615
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/mobile/apps/mobile/app.config.ts`:
- Line 53: The owner field in app.config.ts contains a typo: change the string
value in the owner property (owner: 'supserset-sh') to the correct organization
name 'superset-sh' so it matches other configs; locate the owner property in the
exported config object and update its value accordingly.
- Around line 1-54: Remove the duplicate nested TypeScript Expo config file (the
extra app.config.ts) since the existing app.config.js is used by Expo, and in
the remaining/primary config (app.config.js) fix the owner property value from
"supserset-sh" to "superset-sh" (update the owner key in the exported config
object).
♻️ Duplicate comments (1)
apps/mobile/app.config.js (1)
49-49: Typo inownerfield.The owner is set to
"supserset-sh"but should be"superset-sh"(note the extra "s"). This was already flagged in a previous review.
| import { ExpoConfig, ConfigContext } from 'expo/config'; | ||
| import { config } from 'dotenv'; | ||
| import path from 'node:path'; | ||
|
|
||
| // Load .env file | ||
| config({ | ||
| path: path.resolve(__dirname, '../../.env'), | ||
| override: true, | ||
| }); | ||
|
|
||
| export default ({ config }: ConfigContext): ExpoConfig => ({ | ||
| ...config, | ||
| name: 'Superset', | ||
| slug: 'superset', | ||
| version: '1.0.0', | ||
| orientation: 'portrait', | ||
| icon: './assets/icon.png', | ||
| userInterfaceStyle: 'light', | ||
| newArchEnabled: true, | ||
| scheme: 'superset', | ||
| splash: { | ||
| image: './assets/splash-icon.png', | ||
| resizeMode: 'contain', | ||
| backgroundColor: '#ffffff', | ||
| }, | ||
| ios: { | ||
| supportsTablet: true, | ||
| bundleIdentifier: 'sh.superset.mobile', | ||
| infoPlist: { | ||
| ITSAppUsesNonExemptEncryption: false, | ||
| }, | ||
| }, | ||
| android: { | ||
| adaptiveIcon: { | ||
| foregroundImage: './assets/adaptive-icon.png', | ||
| backgroundColor: '#ffffff', | ||
| }, | ||
| package: 'sh.superset.mobile', | ||
| edgeToEdgeEnabled: true, | ||
| predictiveBackGestureEnabled: false, | ||
| }, | ||
| web: { | ||
| favicon: './assets/favicon.png', | ||
| bundler: 'metro', | ||
| }, | ||
| plugins: ['expo-router'], | ||
| extra: { | ||
| router: {}, | ||
| eas: { | ||
| projectId: 'fa9332a8-896a-4d2a-be5b-d82469b46e5d', | ||
| }, | ||
| }, | ||
| owner: 'supserset-sh', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check actual directory structure under apps/mobile
echo "=== Checking directory structure ==="
find apps/mobile -type f -name "app.config.*" 2>/dev/null | head -20
echo -e "\n=== Full directory tree of apps/mobile (first 50 lines) ==="
find apps/mobile -type d | head -50
echo -e "\n=== Check if apps/mobile/apps/mobile exists ==="
if [ -d "apps/mobile/apps/mobile" ]; then
echo "YES - nested structure exists"
ls -la apps/mobile/apps/mobile/
else
echo "NO - nested structure does not exist"
fi
echo -e "\n=== Listing app.config files across entire repo ==="
find . -name "app.config.*" -type f 2>/dev/nullRepository: superset-sh/superset
Length of output: 904
🏁 Script executed:
#!/bin/bash
# Read the TypeScript config file
echo "=== Content of apps/mobile/apps/mobile/app.config.ts ==="
cat -n apps/mobile/apps/mobile/app.config.ts
echo -e "\n=== Content of apps/mobile/app.config.js ==="
cat -n apps/mobile/app.config.js
echo -e "\n=== Check .env location ==="
find . -maxdepth 2 -name ".env*" -type f 2>/dev/null
echo -e "\n=== Package.json scripts in apps/mobile ==="
grep -A 20 '"scripts"' apps/mobile/package.json | head -30Repository: superset-sh/superset
Length of output: 3816
🏁 Script executed:
#!/bin/bash
# Check if .env exists in monorepo root
echo "=== Checking for .env in monorepo root ==="
ls -la | grep "^-.*\.env"
echo -e "\n=== Checking for .env in apps/mobile ==="
ls -la apps/mobile/ | grep "^-.*\.env"
echo -e "\n=== Which config is actually used - check git history or build config ==="
grep -r "app.config" apps/mobile/package.json 2>/dev/null
echo -e "\n=== Check Expo configuration documentation reference ==="
grep -r "expo.*config" apps/mobile/tsconfig.json apps/mobile/app.json 2>/dev/null || echo "No app.json or tsconfig reference found"
echo -e "\n=== Check if both configs would load different .env files ==="
echo "From apps/mobile/app.config.js: ../../.env resolves to monorepo .env"
echo "From apps/mobile/apps/mobile/app.config.ts: ../../.env resolves to apps/mobile/.env"
echo ""
echo "Neither location exists currently (only .env.example at root)"Repository: superset-sh/superset
Length of output: 725
🏁 Script executed:
#!/bin/bash
# Check which config Expo might actually use
echo "=== Expo config lookup order ==="
echo "Expo looks for: app.json, app.config.js, app.config.ts, app.config.cjs (in that order)"
echo ""
echo "Files found:"
ls -la apps/mobile/app.config.* apps/mobile/app.json 2>/dev/null
echo -e "\n=== Check if there's an app.json at apps/mobile ==="
ls -la apps/mobile/app.json 2>/dev/null || echo "No app.json found"
echo -e "\n=== Check git or build references to these config files ==="
grep -r "apps/mobile/apps/mobile/app.config" . --include="*.json" --include="*.ts" --include="*.js" --include="*.yml" --include="*.yaml" 2>/dev/null | head -10
echo -e "\n=== Check which is newer ==="
stat apps/mobile/app.config.js | grep Modify
stat apps/mobile/apps/mobile/app.config.ts | grep Modify
echo -e "\n=== Typo verification: owner field should be 'superset-sh' not 'supserset-sh' ==="
grep "supserset-sh" apps/mobile/app.config.js apps/mobile/apps/mobile/app.config.tsRepository: superset-sh/superset
Length of output: 735
🏁 Script executed:
#!/bin/bash
# Final verification: confirm the .ts file is effectively dead code
echo "=== Confirming .ts file is unused ==="
echo "According to Expo's config resolution order (app.json > app.config.js > app.config.ts):"
echo "- app.config.js exists at apps/mobile/ ← THIS WILL BE USED"
echo "- app.config.ts exists at apps/mobile/apps/mobile/ ← THIS IS IGNORED/DEAD CODE"
echo ""
echo "=== Summary of issues found ==="
echo "1. Unused nested app.config.ts at apps/mobile/apps/mobile/app.config.ts"
echo "2. Duplicate configuration between .js and .ts files"
echo "3. Typo: 'supserset-sh' should be 'superset-sh' (in both files)"
echo "4. If .ts file were used, .env path ../../.env would resolve incorrectly to apps/mobile/.env"
echo ""
echo "=== Check if .env actually exists ==="
[ -f .env ] && echo ".env exists at root" || echo ".env does NOT exist at root (only .env.example)"Repository: superset-sh/superset
Length of output: 729
Delete the dead code file apps/mobile/apps/mobile/app.config.ts and fix the typo in both configs.
The nested app.config.ts is unreachable—Expo's config resolution order prioritizes app.config.js, which already exists at apps/mobile/app.config.js and will always be used. The app.config.ts at apps/mobile/apps/mobile/ should be deleted.
Additionally, both app.config.js and app.config.ts contain a typo in the owner field: "supserset-sh" should be "superset-sh". This should be corrected in app.config.js.
🤖 Prompt for AI Agents
In `@apps/mobile/apps/mobile/app.config.ts` around lines 1 - 54, Remove the
duplicate nested TypeScript Expo config file (the extra app.config.ts) since the
existing app.config.js is used by Expo, and in the remaining/primary config
(app.config.js) fix the owner property value from "supserset-sh" to
"superset-sh" (update the owner key in the exported config object).
| projectId: 'fa9332a8-896a-4d2a-be5b-d82469b46e5d', | ||
| }, | ||
| }, | ||
| owner: 'supserset-sh', |
There was a problem hiding this comment.
Same typo in owner field.
The owner field contains the same typo "supserset-sh" as in app.config.js. Should be "superset-sh".
🐛 Proposed fix
- owner: 'supserset-sh',
+ owner: 'superset-sh',📝 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.
| owner: 'supserset-sh', | |
| owner: 'superset-sh', |
🤖 Prompt for AI Agents
In `@apps/mobile/apps/mobile/app.config.ts` at line 53, The owner field in
app.config.ts contains a typo: change the string value in the owner property
(owner: 'supserset-sh') to the correct organization name 'superset-sh' so it
matches other configs; locate the owner property in the exported config object
and update its value accordingly.
saddlepaddle
force-pushed
the
mobile-animations
branch
from
92f9615 to
d1383d3
Compare
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.