Storage resilience: fix cross-account/data-loss bugs, sync essays and templates, add realtime + backup#249
Merged
Merged
Conversation
…bugs, sync essays and templates, add realtime + backup
Reframes the storage model from "offline-first" to "Supabase-primary, offline-capable
with reduced capabilities, self-hostable with full functionality" throughout
CLAUDE.md/README, and fixes the correctness gaps that audit surfaced.
Phase 1 — correctness fixes:
- Merge essayTemplates/gradingTasks in mergeStoreData (were fetched on hydrate but
silently dropped, invisible across devices).
- Fix cross-account data bleed: a different user signing in on the same browser could
have their pending-sync queue flushed into the previous user's account. Local data is
now wiped on owner switch (storage.ts clearLocalData + StorageSync.guardOwnerSwitch).
- Surface pending-queue quota errors instead of silently dropping writes; cap the queue.
Phase 2 — bring essays/templates into the sync pattern:
- New tables (essay_batch_assignments, essay_offline_submissions) for local
batch-assignment tracking and offline-import submissions — kept separate from the
existing single-row-per-teacherKey essay_assignments table used by the share-link
mechanism, to avoid students silently overwriting each other's rows.
- userTemplates ("save as template") now syncs via Supabase instead of being
localStorage-only; Dashboard/RubricBuilder now route through AppContext instead of
calling storage.ts directly.
- Fixed a related bug: the pending-queue dedup key only read payload.id/.guid, which
would have collapsed all offline edits to a composite-key entity into one slot.
- Sign-out now wipes local data once the pending queue is confirmed empty (shared
classroom devices), otherwise warns instead of risking data loss.
Phase 3 — resilience:
- Realtime sync: StorageSync subscribes to Postgres changes on every synced table,
debounced into the existing hydrate()+mergeStoreData() pipeline rather than a new
payload-decoding path.
- Orphaned IndexedDB recording blobs are pruned after every sync (a session deleted on
another device previously left its local blob behind forever).
- nightly-backup edge function + export_owner_backup() SQL function for deployments
with no server-side pg_dump access (Supabase Cloud, or Supabase self-hosted
separately from this repo's own docker-compose.yml).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 50 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates RubricMaker to treat Supabase as the primary persistence path when configured, with offline fallback behavior, new syncable collections, realtime refresh, safer sign-out handling, and a nightly backup export flow. It also updates docs, locales, tests, and database migrations to match. ChangesStorage, sync, templates, and backups
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SupabaseAdapter
participant StorageSyncService
participant AppContext
participant Supabase
StorageSyncService->>Supabase: subscribe to postgres_changes
Supabase-->>StorageSyncService: change event
StorageSyncService->>StorageSyncService: debounce refresh
StorageSyncService->>AppContext: notifyReconnect()
AppContext->>SupabaseAdapter: hydrate and push merged store
sequenceDiagram
participant Cron
participant nightly-backup
participant Postgres
participant StorageBucket
Cron->>nightly-backup: bearer-authenticated trigger
nightly-backup->>Postgres: list admin/teacher profiles
loop each profile
nightly-backup->>Postgres: export_owner_backup(target_owner)
Postgres-->>nightly-backup: jsonb snapshot
nightly-backup->>StorageBucket: upload {userId}/{timestamp}.json
nightly-backup->>StorageBucket: pruneOldBackups(userId)
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…4ed0 # Conflicts: # src/context/AppContext.tsx # src/services/database/StorageSync.ts # src/services/database/SupabaseAdapter.ts
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
33-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTech-stack table still says "
localStorageprimary; Supabase optional sync".Line 41 wasn't updated alongside the new "Storage rule (Supabase-primary, offline-capable)" section (lines 62-70), so the doc now contradicts itself on the core architectural positioning this PR is meant to fix.
📝 Proposed fix
-| Persistence | `localStorage` primary; Supabase optional sync | +| Persistence | Supabase primary when configured; `localStorage` offline fallback |🤖 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 `@CLAUDE.md` around lines 33 - 48, Update the Tech stack table in CLAUDE.md so it matches the new storage architecture; the current “Persistence” entry still says “localStorage primary; Supabase optional sync,” which conflicts with the “Storage rule (Supabase-primary, offline-capable)” section. Change the persistence description to reflect Supabase as the primary source of truth with offline-capable localStorage support, and keep the wording consistent with the storage rule section so the document does not contradict itself.
🤖 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 `@src/context/AppContext.tsx`:
- Around line 617-627: The SAVE_USER_TEMPLATE reducer in AppContext is capping
the authoritative userTemplates state to 20 items, which makes the
diff(prev.userTemplates, state.userTemplates, 'userTemplate', ...) effect
interpret evicted entries as deletions and sync them to Supabase. Remove the
.slice(0, 20) cap from the state update in SAVE_USER_TEMPLATE so the sync
source-of-truth is preserved, and if a 20-item limit is still needed, apply it
only in presentation code or local-only storage helpers like saveUserTemplates
rather than in the reducer state used by the sync logic.
In `@src/pages/LandingPage.tsx`:
- Around line 284-288: The landing page still hardcodes user-facing English
text, and the new checklist label in LandingPage.tsx continues that pattern.
Update the checklist labels in LandingPage and the surrounding JSX to use
useTranslation instead of inline strings, so all visible copy is sourced from
localized keys rather than hardcoded English.
In `@src/services/database/StorageSync.ts`:
- Around line 256-263: `startRealtimeSync()` can return early when
`this.realtimeChannel` is already set, so an owner change through `configure()`
may leave the old subscription active. Update
`StorageSync.configure()`/`guardOwnerSwitch()` to clear or stop the existing
realtime channel before starting a new owner, or make `startRealtimeSync()`
track the current UID and replace stale channels so the new `sync:${uid}`
subscription is always created.
In `@src/store/storage.ts`:
- Around line 630-633: Remove the stray “ponytail:” fragment from the comment
near MAX_PENDING_OPS in storage.ts so the documentation reads as intentional
prose; update the nearby comment text in the storage logic around the pending
ops cap without changing the behavior of MAX_PENDING_OPS or the related
deduping/eviction explanation.
- Around line 648-654: The pending-sync cap in storage.queue handling silently
discards the oldest entry in the MAX_PENDING_OPS path without notifying the
user, unlike the quota-exceeded flow. Update the queue eviction logic in
storage.ts around the pending-sync queue branch so it invokes the same
user-facing notification path used by quotaExceededHandler when an item is
dropped, and keep the existing console.warn for diagnostics. Use the
pending-sync queue handling and quotaExceededHandler symbols to locate the
change.
In `@supabase/functions/nightly-backup/index.ts`:
- Around line 1-3: The header comment for nightly-backup is inconsistent with
the README about self-hosted scheduling. Update the top-of-file comment in the
nightly-backup Edge Function to match the deployment guidance in the README, so
it states that official self-hosted deployments can also schedule this function
instead of steering operators only to scripts/backup.sh. Keep the note aligned
with the Edge Function entry point and avoid suggesting the function is
Cloud-only.
- Around line 69-72: The prune path in pruneOldBackups currently ignores storage
list/remove failures, which can make backup retention silently fail while the
caller still reports success. Update pruneOldBackups to propagate or return a
warning when admin.storage.from('backups').list or remove fails, and make the
backup flow in index.ts use that result so the response reflects pruning errors
instead of swallowing them.
- Around line 15-24: The nightly-backup handler currently defaults
SUPABASE_SERVICE_ROLE_KEY and SUPABASE_URL to empty strings, which can let an
empty Bearer token through and then create an invalid admin client. Update the
auth/initialization flow in index.ts so the request is rejected with a server
error before calling createClient whenever either required env var is missing,
and keep the Authorization check in the same early-validation path.
- Around line 27-30: The profile lookup in nightly-backup is still doing a
single unbounded scan, so it can stop at the Supabase row limit and miss later
admin/teacher records. Update the profiles query in the backup flow to page
through results using .range() in a loop, accumulate all matching rows, and only
then continue into the export loop. Keep the change localized around the
admin.from('profiles').select('id').in(...) logic so the backup job processes
every page before exporting.
In `@supabase/migrations/045_essay_local_tracking_sync.sql`:
- Around line 20-27: Add a short safety comment near the index statements in the
migration noting that these are newly created empty tables, so the indexes are
intentionally created without CONCURRENTLY to remain compatible with
transaction-wrapped Supabase migrations. Keep the note close to the
essay_batch_assignments and essay_offline_submissions index definitions so
future readers understand why SQLFluff’s suggestion is not applied.
In `@supabase/migrations/046_user_templates_sync.sql`:
- Line 10: Document in the 046_user_templates_sync migration that the
user_templates_owner_id_idx creation is intentionally non-concurrent and must
remain inside the transaction-wrapped migration flow. Add a brief rationale near
the create index statement explaining that Supabase migrations should stay
transaction-safe, so CREATE INDEX CONCURRENTLY must not be used here. Reference
the user_templates_owner_id_idx index creation on public.user_templates so
future edits don’t “fix” it into a transaction-incompatible form.
In `@supabase/migrations/047_enable_realtime.sql`:
- Around line 7-28: The supabase_realtime publication update is not idempotent
because `ALTER PUBLICATION ... ADD TABLE` will fail if a table is already
included. Update the migration logic around the `ALTER PUBLICATION
supabase_realtime` statement to guard each table in the list with a
`pg_publication_tables` membership check before adding it, or use an equivalent
per-table conditional approach. Keep the fix localized to this publication
update so retries or manual reruns do not abort.
In `@supabase/migrations/048_nightly_backup.sql`:
- Around line 67-73: The nightly backup logic in the backup JSON builder is only
exporting `essay_assignments` and `essay_submissions` rows, but
`essay_submissions` depends on Storage object paths and will not be restorable
if bucket contents are lost. Update the backup flow in the migration’s backup
function to either include the referenced Storage objects in the artifact or
clearly mark this backup as metadata-only by adding explicit
documentation/comments and naming. Use the `essay_submissions` export block and
the surrounding backup assembly as the location to implement the chosen
approach.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 33-48: Update the Tech stack table in CLAUDE.md so it matches the
new storage architecture; the current “Persistence” entry still says
“localStorage primary; Supabase optional sync,” which conflicts with the
“Storage rule (Supabase-primary, offline-capable)” section. Change the
persistence description to reflect Supabase as the primary source of truth with
offline-capable localStorage support, and keep the wording consistent with the
storage rule section so the document does not contradict itself.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35cb9b7d-9f4d-4f3e-96db-1164da385ad9
📒 Files selected for processing (26)
CLAUDE.mdREADME.mdsrc/context/AppContext.tsxsrc/locales/de.jsonsrc/locales/en.jsonsrc/locales/es.jsonsrc/locales/fr.jsonsrc/locales/nl.jsonsrc/pages/Dashboard.tsxsrc/pages/GradeStudent.tsxsrc/pages/LandingPage.tsxsrc/pages/RubricBuilder.tsxsrc/pages/__tests__/pages.deepcoverage.test.tsxsrc/services/__tests__/mediaStore.test.tssrc/services/database/StorageSync.tssrc/services/database/SupabaseAdapter.tssrc/services/mediaStore.tssrc/store/storage.test.tssrc/store/storage.tssrc/utils/syncMerge.test.tssrc/utils/syncMerge.tssupabase/functions/nightly-backup/index.tssupabase/migrations/045_essay_local_tracking_sync.sqlsupabase/migrations/046_user_templates_sync.sqlsupabase/migrations/047_enable_realtime.sqlsupabase/migrations/048_nightly_backup.sql
- Critical: SAVE_USER_TEMPLATE's .slice(0, 20) cap evicted entries from the reducer's synced state array, which the delta-sync diff() effect reads as a delete and pushes to Supabase — saving a 21st template silently deleted the oldest one from the cloud and every other device. Cap removed. - Major: StorageSync.startRealtimeSync() no-ops when a channel already exists, so configure() called twice without an intervening disconnect()/signOut() (e.g. an owner switch) left the realtime subscription scoped to the previous user's uid. Now stops any existing channel before guardOwnerSwitch() runs. - Major: nightly-backup edge function defaulted SUPABASE_URL/SERVICE_ROLE_KEY to '', letting an empty bearer token pass the auth check if the env vars were unset. Now fails closed with a 500 before checking auth. - Major: nightly-backup's profile scan was unbounded — a school with 1000+ teacher/admin accounts would silently lose backup coverage past the API's default row cap. Now pages through with .range(). - Minor: pending-sync queue eviction at MAX_PENDING_OPS only logged a console.warn; now also fires the same quotaExceededHandler used for quota errors, since a dropped queue entry is the same kind of data loss. - Minor: nightly-backup's pruneOldBackups() swallowed list/remove errors, letting retention drift while still reporting success; now throws so the caller's existing per-profile error handling picks it up. - Minor: nightly-backup's header comment said self-hosted should use scripts/backup.sh instead, contradicting the README (which documents the official self-hosted stack using this function too, since scripts/backup.sh is hardwired to this repo's own docker-compose.yml). - Major: 047's `ALTER PUBLICATION ... ADD TABLE` has no IF NOT EXISTS form and errors (not a no-op) on a table that's already a member, so a retry would abort the migration. Wrapped in a per-table pg_publication_tables guard. - Trivial: removed a stray "ponytail:" fragment from a storage.ts comment (an AI-assistant-session marker with no meaning to other tools/readers). - Trivial: documented why 045/046's indexes intentionally skip CONCURRENTLY (Supabase migrations run in a transaction, which disallows it; both tables are newly created in the same migration, so there's no data to lock). - Documented (not fixed — flagged "heavy lift" by the reviewer, and out of scope for a lightweight per-user DB snapshot): 048's backup covers table rows only, not the files referenced by Storage-path columns (essay submissions, recordings) — noted in both the migration and the README. - Not changed: LandingPage.tsx's hardcoded "Works offline" string — the reviewer flagged this as low-value on its own, since the entire page is already unlocalized; not worth an isolated fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ion fallback E2E — Supabase Integration caught a real regression: guardOwnerSwitch() (added this PR) was wiping local data on a plain page reload, deleting data an offline grading test had just seeded before the app could flush/use it. Root cause: SupabaseAdapter.connect()'s pre-existing session-restore has a fallback — if client.auth.getSession() returns null (e.g. a transient race right after a fresh client is created on page load, before Supabase-js has finished restoring the persisted session from storage), it silently signs in anonymously instead, producing a brand new uid. Before this PR that was mostly harmless. With guardOwnerSwitch() now wiping local data whenever the uid doesn't match the last-known owner, that same fallback became destructive: a spurious anonymous fallback looks identical to a genuine account switch. Fix: track whether the current session is anonymous (SupabaseAdapter. isAnonymousSession()) and skip the owner-switch check entirely when it is — never wipe, and don't overwrite the stored owner, so a later reconnect with the real session still compares against the correct last-known owner. This doesn't reopen the original cross-account-bleed gap the guard exists for: that scenario is always a real authenticated login (OTP/magic link/password), never anonymous. Also (from remaining CI feedback): - CodeQL: nightly-backup's profile-listing failure returned the raw exception message in the HTTP response; now logs it server-side and returns a generic message instead. - CodeRabbit (stale review pass against an earlier commit): CLAUDE.md's tech stack table still said "localStorage primary; Supabase optional sync", contradicting the "Storage rule (Supabase-primary, offline-capable)" section from earlier in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@supabase/functions/nightly-backup/index.ts`:
- Around line 76-84: The profile pagination in the backup sweep is unstable
because the query in nightly-backup’s profile loop uses .range() without a
deterministic sort. Update the profiles query in the backup iteration to include
an explicit .order('id') before .range(from, from + PROFILE_PAGE_SIZE - 1), so
paging stays consistent across batches and the loop in the nightly-backup
function does not skip or duplicate profiles.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff2fdf03-fc8c-4f14-ad62-3a371cb5b7b3
📒 Files selected for processing (11)
CLAUDE.mdREADME.mdsrc/context/AppContext.tsxsrc/services/database/StorageSync.tssrc/services/database/SupabaseAdapter.tssrc/store/storage.tssupabase/functions/nightly-backup/index.tssupabase/migrations/045_essay_local_tracking_sync.sqlsupabase/migrations/046_user_templates_sync.sqlsupabase/migrations/047_enable_realtime.sqlsupabase/migrations/048_nightly_backup.sql
.range() without an explicit .order() has no guaranteed row ordering across
successive requests (PostgREST/Postgres row order is otherwise unspecified),
so paginating without one could skip or duplicate profiles across pages.
Added .order('id') before .range().
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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
Full audit-and-fix pass on the persistence/sync layer, done in three phases. Also repositions the project from "offline-first" to Supabase-primary, offline-capable (reduced capabilities) — self-hostable with full functionality throughout
CLAUDE.md/README.md, since that's the actual intended architecture.Phase 1 — correctness fixes
essayTemplates/gradingTaskswere fetched on hydrate but never merged (missing frommergeStoreData'sCOLLECTIONS) — silently dropped, invisible on other devices. Fixed + regression tests.StorageSync.guardOwnerSwitch()now wipes local data (storage.ts#clearLocalData) before flushing when a different user's uid is detected.localStoragequota were silently dropped — now surfaced via the existing quota-exceeded handler, and the queue is capped at 500 entries (drop-oldest).Phase 2 — bring essays/templates into the sync pattern
essay_batch_assignments/essay_offline_submissions(migration 045) for local class-assignment tracking and offline-code-import submissions. Deliberately not merged into the existingessay_assignmentstable, which is a different single-row-per-teacherKeymechanism backing the share-link/DB-submission flow — doing so would have let one student's row silently overwrite another's.userTemplates("save as template") now syncs via Supabase (migration 046) instead of beinglocalStorage-only;Dashboard/RubricBuilderroute throughAppContextinstead of callingstorage.tsdirectly.payload.id/.guid, which would've collapsed every offline edit to a composite-key entity (like the new essay-batch-assignment) into a single queue slot, losing all but the last one.toast.signout_pending_writeskey across all 5 locales.Phase 3 — resilience
StorageSyncsubscribes to Postgres change events on every synced table (RLS-scoped per user), debounced into the existinghydrate()+mergeStoreData()pipeline rather than a new payload-decoding path for ~20 table shapes.deleteBlob.nightly-backupedge function): for deployments with no server-sidepg_dumpaccess (Supabase Cloud, or Supabase self-hosted separately from this repo's owndocker-compose.yml, which has no functions runtime).scripts/backup.shonly works against this repo's bundled compose stack.Compatibility check with PR #248
Confirmed no real conflicts — migration numbers were bumped to avoid colliding with #248's
044_test_assignments.sql, and the touched-file overlaps (AppContext.tsx,StorageSync.ts,SupabaseAdapter.ts, locale files) are all purely additive at different insertion points.Test plan
npm run typecheck— cleannpm run lint— clean (only pre-existing warnings)npm test— 2112/2112 passlocalStoragecorrectly with no console errorssupabase db resetto verify migrations 045–048 apply cleanly — not run, no Docker daemon available in this sandbox; please verify locally before mergingnightly-backupedge function, and the cross-account wipe path are untestable without a live Supabase project — please smoke-test manually before relying on them in production🤖 Generated with Claude Code
Summary by CodeRabbit