feat: Phase 6–8 dead code, security account, sync performance - #11
Merged
Conversation
- Delete downloadAudio, deleteCloudAudio, deleteMultipleCloudAudios, getCloudAudioUrl, checkCloudAudioExists (zero in-repo callers) - Keep uploadAudio / uploadAudioWithRetry / uploadPendingAudios Co-authored-by: Cursor <cursoragent@cursor.com>
- WeatherState is now score + condition only - Remove description writers from weather slice Co-authored-by: Cursor <cursoragent@cursor.com>
- Avoid SyncStatus name collision with entry/audio sync status - Collapse ProfileSettingsSection inline chrome union to shared type Co-authored-by: Cursor <cursoragent@cursor.com>
- Route user/entries/storage/audioSync/supabase errors through utils/logger - Keep operational context only; leave __DEV__ console.log unchanged Co-authored-by: Cursor <cursoragent@cursor.com>
- Point 安全 section at CONCERNS Accepted Product Risks (DOC-02) - CONCERNS Phase 6 debt cleared locally under .planning (gitignored) Co-authored-by: Cursor <cursoragent@cursor.com>
- Segment-equality path matching contract (not substring) - Guest strip preserves text and clears audios/remoteUrl Co-authored-by: Cursor <cursoragent@cursor.com>
- pathSegmentIncludesUserId + filterWipeCandidatePaths (segment equality) - stripAudiosFromEntries omits audios without mutating input Co-authored-by: Cursor <cursoragent@cursor.com>
- Fail-closed list/remove with path-segment userId match - Prefer userId/ walk then bucket drift filter; batch remove ≤1000 Co-authored-by: Cursor <cursoragent@cursor.com>
- Rewrite guest with stripAudiosFromEntries only after delete-account OK - Best-effort deleteAsync for snapshot localUris; FS failures do not undo Edge Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover bare path, public URL, query strip, sign marker, null cases Co-authored-by: Cursor <cursoragent@cursor.com>
- Pure path parse for bare paths and legacy public/sign audios URLs Co-authored-by: Cursor <cursoragent@cursor.com>
- Assert uploadAudio returns object path remoteUrl - Assert resolvePlayableRemoteUrl uses createSignedUrl TTL 86400 Co-authored-by: Cursor <cursoragent@cursor.com>
- Store object path after upload; drop getPublicUrl minting - Export resolvePlayableRemoteUrl with TTL 86400 (D-06..D-08, D-10) Co-authored-by: Cursor <cursoragent@cursor.com>
- Inject resolvePlayableRemoteUrl via setAudioRemoteResolver at store init - Local file hits skip signing; remote path/URL signed at play time Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover first-try success, retry success, and final failure → handler once Co-authored-by: Cursor <cursoragent@cursor.com>
- Retry setItem up to 2 times then invoke persist-failure handler once - Register Alert + signOut + clear user from store init (no lib→store) - Add zh-Hans/en-US sessionPersistFailed auth copy Co-authored-by: Cursor <cursoragent@cursor.com>
- Encode D-01 bump, D-03 backfill, D-07 skip, D-09 advance as RED targets - Import shared/sync/revisionMeta helpers Plan 08-02/08-03 will export Co-authored-by: Cursor <cursoragent@cursor.com>
- Encode D-10 cap-3 saturation and D-11 fail-continue as RED targets - Mock supabase upload + sleepMs; no new package deps Co-authored-by: Cursor <cursoragent@cursor.com>
- MoodEntry.updatedAt revision field (maps to DB updatedat) - SQL migration ADD COLUMN updatedat bigint + NULL backfill - revisionMeta backfillUpdatedAt / withBumpedUpdatedAt; skip/advance stubs for 08-03 Co-authored-by: Cursor <cursoragent@cursor.com>
- D-01/D-02 writers set or bump updatedAt; audio retry does not - D-03 _loadEntries backfills via backfillUpdatedAt - syncToCloud/syncFromCloud map updatedat bigint revision Co-authored-by: Cursor <cursoragent@cursor.com>
- Replace serial for-await with hand-rolled worker pool (UPLOAD_CONCURRENCY=3) - Keep uploadAudioWithRetry per item; fail-continue collects failedAudioIds - No applyAudioUploadResults import (D-12); no new deps Co-authored-by: Cursor <cursoragent@cursor.com>
- Drop applyAudioUploadResults mention from audioSync comments so rg gate passes - Regression: audioSync + concurrency unit suites green Co-authored-by: Cursor <cursoragent@cursor.com>
- shouldUpsertEntry / advanceLastSyncedAfterSuccess (D-07/D-09)
- load/save SyncRevisionMeta via sync_revision_meta_${userId} (D-08)
- pullCursorUpdatedAt skeleton only (D-06)
Co-authored-by: Cursor <cursoragent@cursor.com>
- Filter upsert set with shouldUpsertEntry after tombstone filter (D-07) - Advance/save revision meta only on successful upsert paths (D-09) - Keep audio pending collection on full entries; pull unfiltered (D-06) Co-authored-by: Cursor <cursoragent@cursor.com>
- Add syncProgress and lastSyncTime to SyncModule / initial store state - Hydrate last_sync_time AsyncStorage into store.lastSyncTime (D-16) Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove ProfileSyncChrome / local syncProgress / lastSyncTime state - Handlers write store syncStatus, syncProgress, lastSyncTime (D-14..D-16) - statusRow icons follow UI-SPEC (spinner / X / CheckCircle) Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Support local Maestro 016 against private audios (env example + login subflow); add UI testIDs for play/login/sync without committing credentials. Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds revision-aware cloud sync, signed audio playback, SecureStore recovery, account audio cleanup, centralized profile sync state, weather-state simplification, and a Maestro acceptance flow with supporting tests and UI identifiers. ChangesRevision-aware sync and audio playback
Persistence failure and account cleanup
Profile sync UI and Maestro flow
Weather state simplification
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Maestro
participant ProfileScreen
participant AppStore
participant AudioCoordinator
participant SupabaseStorage
Maestro->>ProfileScreen: log in and trigger profile sync
ProfileScreen->>AppStore: start sync action
AppStore-->>ProfileScreen: update sync progress and status
Maestro->>AudioCoordinator: play synced audio entry
AudioCoordinator->>SupabaseStorage: create signed playback URL
SupabaseStorage-->>AudioCoordinator: return signed URL
AudioCoordinator-->>Maestro: display audio-playing state
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
store/modules/weather.ts (1)
40-65: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd tests for the changed weather contract.
Please cover threshold boundaries, inactive/soft-deleted entries, and the removal of
descriptionfrom the persisted state. As per coding guidelines, “行为变更应补充或更新测试”.🤖 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 `@store/modules/weather.ts` around lines 40 - 65, Update the tests for _calculateWeather to cover exact and adjacent WEATHER_THRESHOLDS boundaries, excluding inactive and soft-deleted entries from the score, and verifying persisted weather state no longer includes description. Reuse the existing weather store test setup and assert both score and condition for each relevant case.Source: Coding guidelines
🧹 Nitpick comments (4)
features/profile/hooks/useProfileSyncHandlers.ts (2)
114-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the caught sync error before discarding it.
The original
erroris only used to derive a transient UI string (cleared after 3s) and is never logged. Per coding guidelines, new code should route errors throughutils/loggerrather than dropping them, especially on this hot user-triggered path — without it, production sync failures leave no trace oncesyncProgressclears.As per coding guidelines: "新代码使用
utils/logger，生产环境避免刷信息级日志。"♻️ Proposed fix
} catch (error: unknown) { const err = error as { message?: string }; const errorMessage = err?.message || i18n.t("sync.operationFailed", { ns: "system" }); + logger.error("profileSync", "sync action failed", error); useAppStore.setState({ syncStatus: "error", syncProgress: errorMessage, });#!/bin/bash # Confirm whether `logger` is already imported in this file fd 'useProfileSyncHandlers.ts' -x sed -n '1,13p' {}🤖 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 `@features/profile/hooks/useProfileSyncHandlers.ts` around lines 114 - 126, In the catch block of useProfileSyncHandlers, log the original caught sync error through the existing utils/logger mechanism before converting it into the transient errorMessage; use an error-level log with relevant sync context and avoid adding informational-level logging.Source: Coding guidelines
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
RefObjectinstead ofMutableRefObjectfor the profile sync handler ref.This project is on React 19.1 /
@types/react19.1.0, whereMutableRefObjectis deprecated. Replace the import andisSyncingRef: MutableRefObject<boolean>withRefObject<boolean>for consistency.🤖 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 `@features/profile/hooks/useProfileSyncHandlers.ts` around lines 14 - 19, Replace the deprecated MutableRefObject import and usage in the StateRef type with RefObject from React, updating isSyncingRef to use RefObject<boolean> while leaving the remaining state callbacks unchanged.supabase/functions/delete-account/index.ts (1)
28-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce cognitive complexity of
listObjectPathsRecursive.Static analysis flags this function at complexity 24 vs. the 15 allowed, driven by the nested
while/infinitefor/for...ofwith multiple early-return branches. Extracting the inner pagination loop (Lines 39-60) into a small helper (e.g.,listOnePage(current, offset)) would flatten the nesting and satisfy the linter without changing behavior.🤖 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/functions/delete-account/index.ts` around lines 28 - 64, Reduce the cognitive complexity of listObjectPathsRecursive by extracting the inner paginated storage-listing logic into a helper such as listOnePage(current, offset). Keep listObjectPathsRecursive responsible for queue traversal and path classification, while preserving the existing pagination, error propagation, and path results.Source: Linters/SAST tools
store/modules/user.ts (1)
710-733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass
{ idempotent: true }todeleteAsyncto avoid false "failures".
deleteAsync(uri)defaults toidempotent: false, so it throws whenever the local file no longer exists (already cleaned up, cloud-only entry, restored device, etc.) — a routine, non-error case here. As written, those routine misses get folded intounlinkFailedand surface in the Line 729 warn log as if something actually went wrong, muddying the signal for real failures.🔧 Suggested fix
try { - await deleteAsync(uri); + await deleteAsync(uri, { idempotent: true }); } catch { unlinkFailed += 1; }Separately (forward-looking only, not a current defect given the declared
expo-file-system@18.1.6): starting with Expo SDK 54 /expo-file-system@19+, the top-level legacy functional API (includingdeleteAsync) throws at runtime and must be imported fromexpo-file-system/legacyinstead. Worth keeping in mind for a future SDK upgrade.🤖 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 `@store/modules/user.ts` around lines 710 - 733, Update the deleteAsync call in the localUri cleanup loop to pass the idempotent option, so missing files are treated as successful cleanup rather than counted in unlinkFailed. Leave the existing warning behavior for genuine deletion errors unchanged.
🤖 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 `@lib/supabase.ts`:
- Around line 45-63: Update the retry loop around write() to bind each caught
error and preserve the last failure reason across attempts. Pass that captured
error to onSecureStorePersistFailure and ensure the final logger.warn at the
call site receives the same diagnostic detail, while keeping successful writes
and retry timing unchanged.
In `@services/audioSync.ts`:
- Around line 72-99: Add a Storage RLS policy for the audios bucket covering
signed URL generation and other relevant object operations, permitting access
only when the object path begins with auth.uid(). Update the storage
policy/migration configuration rather than resolvePlayableRemoteUrl, and add
coverage verifying owned paths are accessible while foreign audio paths are
rejected.
In `@shared/audio/coordinator.ts`:
- Around line 96-108: The remote resolution path in resolvePlayableUri must
preserve the stored source when resolveRemote cannot produce a signed URL.
Update the final resolveRemote(source) handling to return its resolved URL when
available and fall back to source when it resolves to null, while keeping local
playback and the !resolveRemote branches unchanged.
In `@shared/audio/storagePath.ts`:
- Around line 13-31: Update extractAudiosObjectPath so malformed
percent-encoding in the matched path returns null instead of allowing
decodeURIComponent to throw. Guard the decodeURIComponent(rest) call within the
marker-handling branch, while preserving successful decoding and existing null
behavior for other unparseable inputs.
In `@store/useAppStore.ts`:
- Around line 469-473: Replace the newly added console.warn and console.error
calls in the update-error blocks around the entry upsert and related flows with
the app’s existing logger, matching the structured logging usage in
store/modules/entries.ts. Preserve each message and error context while updating
all three affected blocks, including the symbols surrounding successfulUpsertIds
and entry.id.
In `@supabase/functions/delete-account/index.ts`:
- Around line 66-90: The collectUserAudioPaths function performs an unbounded
full-bucket scan during every deletion; remove the
listObjectPathsRecursive(supabaseAdmin, "") drift-scan and
pathSegmentIncludesUserId filtering, relying on the userId-prefixed recursive
listing as the authoritative source while preserving its existing error handling
and result aggregation.
---
Outside diff comments:
In `@store/modules/weather.ts`:
- Around line 40-65: Update the tests for _calculateWeather to cover exact and
adjacent WEATHER_THRESHOLDS boundaries, excluding inactive and soft-deleted
entries from the score, and verifying persisted weather state no longer includes
description. Reuse the existing weather store test setup and assert both score
and condition for each relevant case.
---
Nitpick comments:
In `@features/profile/hooks/useProfileSyncHandlers.ts`:
- Around line 114-126: In the catch block of useProfileSyncHandlers, log the
original caught sync error through the existing utils/logger mechanism before
converting it into the transient errorMessage; use an error-level log with
relevant sync context and avoid adding informational-level logging.
- Around line 14-19: Replace the deprecated MutableRefObject import and usage in
the StateRef type with RefObject from React, updating isSyncingRef to use
RefObject<boolean> while leaving the remaining state callbacks unchanged.
In `@store/modules/user.ts`:
- Around line 710-733: Update the deleteAsync call in the localUri cleanup loop
to pass the idempotent option, so missing files are treated as successful
cleanup rather than counted in unlinkFailed. Leave the existing warning behavior
for genuine deletion errors unchanged.
In `@supabase/functions/delete-account/index.ts`:
- Around line 28-64: Reduce the cognitive complexity of listObjectPathsRecursive
by extracting the inner paginated storage-listing logic into a helper such as
listOnePage(current, offset). Keep listObjectPathsRecursive responsible for
queue traversal and path classification, while preserving the existing
pagination, error propagation, and path results.
🪄 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 Plus
Run ID: 91a392e6-f853-431b-bccd-c542d9848c27
📒 Files selected for processing (39)
.gitignore.maestro.env.example.maestro/flows/016-signed-audio-playback.yaml.maestro/subflows/login.yamlAGENTS.md__tests__/unit/lib/secureStoreAdapter.test.ts__tests__/unit/services/audioSync.concurrency.test.ts__tests__/unit/services/audioSync.test.ts__tests__/unit/shared/audio/guestAudioStrip.test.ts__tests__/unit/shared/audio/playback.test.ts__tests__/unit/shared/audio/storagePath.test.ts__tests__/unit/shared/audio/storageWipe.test.ts__tests__/unit/shared/sync/revisionMeta.test.tscomponents/EntryCard.tsxcomponents/Profile/ProfileUserCard.tsxfeatures/profile/ProfileScreen.tsxfeatures/profile/components/ProfileSettingsSection.tsxfeatures/profile/hooks/useProfileScreenState.tsfeatures/profile/hooks/useProfileSyncHandlers.tslib/supabase.tslocales/en-US/auth.jsonlocales/zh-Hans/auth.jsonpackage.jsonservices/audioSync.tsshared/audio/coordinator.tsshared/audio/guestAudioStrip.tsshared/audio/playback.tsshared/audio/storagePath.tsshared/audio/storageWipe.tsshared/sync/revisionMeta.tsstore/modules/entries.tsstore/modules/storage.tsstore/modules/types.tsstore/modules/user.tsstore/modules/weather.tsstore/useAppStore.tssupabase/functions/delete-account/index.tssupabase/migrations/20260730120000_entries_updatedat.sqltypes.ts
💤 Files with no reviewable changes (1)
- features/profile/hooks/useProfileScreenState.ts
Comment on lines
+72
to
+99
| export async function resolvePlayableRemoteUrl( | ||
| stored: string, | ||
| ): Promise<string | null> { | ||
| if (!isSupabaseConfigured()) { | ||
| return null; | ||
| } | ||
|
|
||
| const objectPath = extractAudiosObjectPath(stored); | ||
| if (!objectPath) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const { data, error } = await supabase.storage | ||
| .from(AUDIO_BUCKET) | ||
| .createSignedUrl(objectPath, SIGNED_URL_TTL_SEC); | ||
|
|
||
| if (error || !data?.signedUrl) { | ||
| logger.warn("audioSync", "createSignedUrl 失败", error ?? undefined); | ||
| return null; | ||
| } | ||
|
|
||
| return data.signedUrl; | ||
| } catch (error) { | ||
| logger.warn("audioSync", "createSignedUrl 异常", error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for Storage RLS policies on the `audios` bucket.
rg -n -C5 "audios" supabase/migrations --iglob '*.sql'
rg -n -C5 "storage.objects" supabase/migrations --iglob '*.sql'Repository: moyunzero/Emotion-Diary
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files matching audio/bucket/supabase =="
git ls-files | rg -n 'audio|Audio|storage|supabase|bucket|Policy|Policies|auth\.uid|signedUrl|createSignedUrl|removeOwnership|ownership' || true
echo
echo "== supabase/migrations =="
ls -la supabase 2>/dev/null || true
find supabase -maxdepth 4 -type f -name '*.sql' 2>/dev/null | sort | sed -n '1,120p' || true
echo
echo "== relevant usages =="
rg -n -C4 "resolvePlayableRemoteUrl|extractAudiosObjectPath|createSignedUrl|AUDIO_BUCKET|storage\.from|audios|signedUrl" .Repository: moyunzero/Emotion-Diary
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -u
echo "== supabase/migrations files =="
find supabase/migrations -type f \( -name '*.sql' -o -o \) -print | sort | sed -n '1,200p'
echo
echo "== policy-related SQL in repo =="
rg -n -C4 "CREATE POLICY|ALTER POLICY|ENABLE ROW LEVEL SECURITY|row level security|storage\.objects|audios|owner|auth\.uid|current_user|jwt\(\)|public|private" supabase utils shared store features --iglob '*.sql' --iglob '*.ts' --iglob '*.tsx' || true
echo
echo "== audio storage path implementation =="
cat -n shared/audio/storagePath.ts | sed -n '1,220p'
echo
echo "== audio storage implementation =="
cat -n utils/audioStorage.ts | sed -n '1,260p'
echo
echo "== supabase delete account =="
cat -n supabase/functions/delete-account/index.ts | sed -n '1,180p'
echo
echo "== local supabase config and seed files =="
find supabase -maxdepth 3 -type f \( -name '*.sql' -o -name '*.json' -o -name '*.yaml' -o -name '*.yml' -o -name '*.toml' \) -print | sort | sed -n '1,200p'Repository: moyunzero/Emotion-Diary
Length of output: 39959
🏁 Script executed:
#!/bin/bash
set -u
echo "== all Supabase-related files =="
git ls-files supabase | sort | sed -n '1,240p'
echo
echo "== Storage SQL anywhere in tracked repo =="
git ls-files | xargs rg -n -C3 "supabase.*storage|storage\.objects|storage\.buckets|CREATE.*AUDIOS|CREATE.*audios|CREATE.*audios|audios.*CREATE|CREATE.*POLICY.*audios|CREATE POLICY\"audios|CREATE POLICY.*audios|insert_own|select_own|remove_own|delete_own|createSignedUrl|RLS|row level security|auth\.uid|owner" || true
echo
echo "== exact file sizes for supabase =="
find supabase -maxdepth 4 -type f -print | sort | xargs -r du -h | sed -n '1,120p'Repository: moyunzero/Emotion-Diary
Length of output: 14905
🏁 Script executed:
#!/bin/bash
set -u
echo "== resolvePlayableRemoteUrl usages =="
rg -n -C4 "resolvePlayableRemoteUrl\\(|setAudioRemoteResolver|AudioRemoteResolver" services store shared components __tests__ --iglob '*.ts' --iglob '*.tsx' || true
echo
echo "== sync path ownership implementation =="
cat -n services/audioSync.ts | sed -n '1,190p'
echo
echo "== all read-only object path/signed url tests =="
rg -n -C3 "owner|auth_uid|first segment|createSignedUrl|private|audios|signed" __tests__ services shared --iglob '*.ts' --iglob '*.tsx' || trueRepository: moyunzero/Emotion-Diary
Length of output: 42105
Ensure the audios bucket restricts signed URL generation to owned paths.
resolvePlayableRemoteUrl mints a signed URL directly from user-supplied/legacy remoteUrl without an explicit auth.uid() prefix check in the client. Add a Storage RLS policy covering Storage operations on audios so only paths starting with the caller’s auth.uid() are accessible, and add coverage for foreign audio paths.
🤖 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 `@services/audioSync.ts` around lines 72 - 99, Add a Storage RLS policy for the
audios bucket covering signed URL generation and other relevant object
operations, permitting access only when the object path begins with auth.uid().
Update the storage policy/migration configuration rather than
resolvePlayableRemoteUrl, and add coverage verifying owned paths are accessible
while foreign audio paths are rejected.
Comment on lines
+96
to
+108
| const source = await resolveAudioSource(audio, async (u) => { | ||
| const info = await getInfoAsync(u); | ||
| return info.exists; | ||
| }); | ||
| if (!source) return null; | ||
| // Local file hit — skip network sign (D-06 play-time only for remote/path) | ||
| if (isLocalPlaybackSource(source, audio)) { | ||
| return source; | ||
| } | ||
| if (!resolveRemote) { | ||
| return source; | ||
| } | ||
| return resolveRemote(source); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect extractAudiosObjectPath and confirm it handles all previously-stored remoteUrl shapes.
fd -e ts . services shared --exec grep -l "extractAudiosObjectPath" {} \;
rg -n -A 15 "function extractAudiosObjectPath" services shared
rg -n "remoteUrl" __tests__/unit/services/audioSync.test.ts __tests__/unit/shared/audio -A2 -B2Repository: moyunzero/Emotion-Diary
Length of output: 12525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -e ts '^coordinator|storagePath|playback|sync' shared services | sed -n '1,120p'
echo
echo "== shared/audio/storagePath.ts =="
cat -n shared/audio/storagePath.ts
echo
echo "== shared/audio/coordinator.ts relevant sections =="
wc -l shared/audio/coordinator.ts
sed -n '1,180p' shared/audio/coordinator.ts | cat -n
echo
echo "== services/audioSync.ts relevant remoteUrl paths =="
rg -n "remoteUrl|resolvePlayableRemoteUrl|extractAudiosObjectPath|playableUrl|sign" services/audioSync.ts shared --include='*.ts'
fd -e ts '^audioSync$' services | xargs -r -n1 sh -c 'echo "--- $0"; cat -n "$0" | sed -n "1,220p"'
echo
echo "== playback tests around resolvePlayableUri/remoteUrl =="
fd -e ts '^playback\.test$' shared | xargs -r -n1 sh -c 'echo "--- $0"; cat -n "$0"'Repository: moyunzero/Emotion-Diary
Length of output: 7908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -e ts '^coordinator|storagePath|playback|sync' shared services | sed -n '1,120p'
echo
echo "== shared/audio/storagePath.ts =="
cat -n shared/audio/storagePath.ts
echo
echo "== shared/audio/coordinator.ts relevant sections =="
wc -l shared/audio/coordinator.ts
sed -n '1,180p' shared/audio/coordinator.ts | cat -n
echo
echo "== services/audioSync.ts relevant remoteUrl paths =="
rg -n "remoteUrl|resolvePlayableRemoteUrl|extractAudiosObjectPath|playableUrl|sign" services/audioSync.ts shared --include='*.ts'
fd -e ts '^audioSync$' services | xargs -r -n1 sh -c 'echo "--- $0"; cat -n "$0" | sed -n "1,220p"'
echo
echo "== playback tests around resolvePlayableUri/remoteUrl =="
fd -e.ts '^playback\.test$' shared | xargs -r -n1 sh -c 'echo "--- $0"; cat -n "$0"'Repository: moyunzero/Emotion-Diary
Length of output: 7908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared/audio/playback.ts =="
cat -n shared/audio/playback.ts
echo
echo "== services/audioSync.ts =="
cat -n services/audioSync.ts
echo
echo "== shared/audio/sync.ts =="
cat -n shared/audio/sync.ts
echo
echo "== test files =="
python3 - <<'PY'
import pathlib
for p in [pathlib.Path("shared/audio/playback.test.ts"), pathlib.Path("shared/audio/sync.test.ts"), pathlib.Path("services/audioSync.test.ts")]:
print(f"\n--- {p}")
print(f"exists={p.exists()}")
if p.exists():
print(p.read_text())
PY
echo
echo "== read-only parser simulation of extractAudiosObjectPath and coordinator fallback == "
python3 - <<'PY'
def extract(stored: str) -> str | None:
trimmed = stored.strip()
if not trimmed:
return None
if not trimmed.lower().startswith(("http://", "https://")):
return trimmed.removeprefix("/")
for marker in ("/storage/v1/object/public/audios/", "/storage/v1/object/sign/audios/"):
i = trimmed.find(marker)
if i >= 0:
rest = trimmed[i + len(marker):].split("?", 1)[0]
return rest
return None
def fallback(source: str, resolveRemote: str | None, remote_url="https://legacy.cdn/x.m4a") -> str | None:
# mirrored shared/audio/coordinator.ts: local file -> local; no resolver -> source; resolver -> resolver(source)
if source and not source.lower().startswith("file"):
if not resolveRemote:
return source
return resolveRemote
return None
samples = [
"user-1/foo.m4a",
"/user-1/foo.m4a",
"https://project.supabase.co/storage/v1/object/public/audios/user-1/foo%20bar.m4a?token=x",
"https://project.supabase.co/storage/v1/object/sign/audios/user-1/a.m4a?policy=x",
"https://legacy.cdn/x.m4a",
"/storage/v1/object/public/foo.m4a",
"/storage/v1/object/public/audios/foo.m4a",
]
for sample in samples:
print({"source": sample, "extract_path": extract(sample)})
PYRepository: moyunzero/Emotion-Diary
Length of output: 10724
Fallback when the play-time resolver can’t sign the remote.remoteUrl.
uploadAudio now writes object paths, but persisted entries may still hold legacy CDN/sign URLs. With resolveRemote injected, resolvePlayableUri currently returns null when resolvePlayableRemoteUrl does not produce a signed URL, causing playback to fail. Fall back to the stored source URL when resolveRemote resolves to null, or otherwise keep the legacy playable URL usable.
🤖 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 `@shared/audio/coordinator.ts` around lines 96 - 108, The remote resolution
path in resolvePlayableUri must preserve the stored source when resolveRemote
cannot produce a signed URL. Update the final resolveRemote(source) handling to
return its resolved URL when available and fall back to source when it resolves
to null, while keeping local playback and the !resolveRemote branches unchanged.
Comment on lines
+66
to
+90
| /** | ||
| * Collect wipe candidates: prefer `${userId}/` walk, then full-bucket drift filter. | ||
| */ | ||
| async function collectUserAudioPaths( | ||
| supabaseAdmin: AdminClient, | ||
| userId: string, | ||
| ): Promise<{ paths: string[]; error: { message: string; name?: string } | null }> { | ||
| const collected = new Set<string>(); | ||
|
|
||
| const primary = await listObjectPathsRecursive(supabaseAdmin, userId); | ||
| if (primary.error) return { paths: [], error: primary.error }; | ||
| for (const p of primary.paths) { | ||
| collected.add(p); | ||
| } | ||
|
|
||
| const all = await listObjectPathsRecursive(supabaseAdmin, ""); | ||
| if (all.error) return { paths: [], error: all.error }; | ||
| for (const p of all.paths) { | ||
| if (pathSegmentIncludesUserId(p, userId)) { | ||
| collected.add(p); | ||
| } | ||
| } | ||
|
|
||
| return { paths: Array.from(collected), error: null }; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Full-bucket scan on every account deletion doesn't scale.
collectUserAudioPaths always performs a second recursive listing of the entire bucket root (listObjectPathsRecursive(supabaseAdmin, "")) in addition to the ${userId}/ walk, to catch "drift" paths where userId appears as a segment outside the expected prefix. As the total number of audio objects across all users grows, every single account deletion becomes O(total bucket objects) instead of O(that user's objects) — this can make deletion slow or hit Edge Function execution limits as the platform scales, and since the flow is fail-closed on list errors (Lines 100-111, 174-185), a timeout here would block a user's ability to delete their own data/account.
Consider dropping the full-bucket drift scan (relying on the ${userId}/ prefix walk, which should be authoritative if uploads always use that convention), or moving drift detection to an offline/async job rather than the synchronous account-deletion path.
🤖 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/functions/delete-account/index.ts` around lines 66 - 90, The
collectUserAudioPaths function performs an unbounded full-bucket scan during
every deletion; remove the listObjectPathsRecursive(supabaseAdmin, "")
drift-scan and pathSegmentIncludesUserId filtering, relying on the
userId-prefixed recursive listing as the authoritative source while preserving
its existing error handling and result aggregation.
Preserve SecureStore retry lastError; harden extractAudiosObjectPath decode; route sync/profile errors through logger; add weather unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
CodeRabbit follow-upPushed fixes for the reasonable quick wins:
Deferred / declined (with rationale):
|
|
7 tasks
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
1 participant
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.
Summary
MoodEntry.updatedAt+ DBupdatedat, push-side skip via revision meta, audio upload concurrency cap 3, Profile sync chrome single-sourced from store; code-review fixes (CR-01/02, WR-01..03).maestro.env)Test plan
yarn typecheck && yarn lint && yarn testentries.updatedat(bigint) and keepsupdated_at(timestamptz)yarn test:maestro:016with local.maestro.envMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests