start work on settings sync #524
Conversation
Deploying edutools-testing with
|
| Latest commit: |
8a84cde
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8b6fc209.edutools-testing.pages.dev |
| Branch Preview URL: | https://510-settings-sync-with-accou.edutools-testing.pages.dev |
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces user accounts table and switches foreign keys to user IDs; adds JWT utilities and types; refactors backups to resolve user via DB; adds sync query/mutation for user settings/favourites/history; adds frontend sync save helper and state/store; updates UI components for theming, settings events, sidebar auth/link; adds account profile route. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Svelte UI (settings)
participant SyncLib as lib/sync.save()
participant Convex as Convex Client
participant API as api.sync.update (mutation)
participant Utils as utils.verifyJwt / getAndUpdateUser
participant DB as DB (users)
User->>UI: Change setting / click
UI->>SyncLib: save(jwt, client, { settings|history|favourites })
activate SyncLib
SyncLib->>SyncLib: Build mutationData from stores
SyncLib->>Convex: mutate(api.sync.update, { jwt, ...mutationData })
activate Convex
Convex->>API: invoke
activate API
API->>Utils: verifyJwtAndGetPayload(jwt)
Utils-->>API: payload (sub)
API->>Utils: getAndUpdateUser(ctx, payload)
activate Utils
Utils->>DB: find by clerkId / insert-or-patch
DB-->>Utils: user(_id)
Utils-->>API: user with _id
API->>DB: patch user fields (conditional)
DB-->>API: ok
deactivate Utils
API-->>Convex: ok
deactivate API
Convex-->>SyncLib: ok
deactivate Convex
SyncLib->>SyncLib: syncState = ''
deactivate SyncLib
UI-->>User: UI idle
note over API,DB: New flow uses users table ID<br/>instead of JWT sub for writes.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
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 |
WalkthroughIntroduces a user model and JWT utility module, refactors backups to reference users, adds a Convex sync API (get/update), wires preliminary frontend sync plumbing (state, stores, save helper), updates several UI components (providers theming, settings change handlers, sidebar tweaks), and adds an account profile page. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Frontend (Svelte)
participant Clerk as Clerk JWT
participant Convex as Convex Client
participant Sync as convex.sync
participant DB as users
rect rgba(220,235,255,0.4)
note over UI,Convex: Sync - Update flow
User->>UI: Trigger save (settings/history/favourites)
UI->>Clerk: getToken()
Clerk-->>UI: jwt
UI->>Convex: mutation api.sync.update({ jwt, ...payload })
Convex->>Sync: update(jwt, partials)
Sync->>Sync: verifyJwtAndGetPayload(jwt)
Sync->>DB: getAndUpdateUser(payload.sub)
alt user found
Sync->>DB: patch user with provided fields
DB-->>Sync: ok
Sync-->>Convex: success
Convex-->>UI: success
else error
Sync-->>Convex: throw
Convex-->>UI: error
end
end
sequenceDiagram
autonumber
actor User
participant UI as Frontend
participant Convex as Convex Client
participant Backups as convex.backups
participant Utils as utils (JWT/User)
participant DB as users/backups
rect rgba(220,255,220,0.35)
note over UI,Backups: Backups - Get/Create/Remove
User->>UI: Request backups
UI->>Convex: query backups.get(jwt)
Convex->>Backups: get(jwt)
Backups->>Utils: verifyJwtAndGetPayload
Backups->>Utils: getUser(ctx, payload)
alt no user
Backups-->>Convex: []
else user found
Backups->>DB: filter backups by user._id
Backups-->>Convex: list with id/name/data/creationTime
end
User->>UI: Create backup
UI->>Convex: mutation backups.create(jwt, data)
Backups->>Utils: getAndUpdateUser(ctx, payload)
Backups->>DB: insert { user: user._id, ... }
Backups-->>Convex: ok
User->>UI: Remove backup
UI->>Convex: mutation backups.remove(jwt, id)
Backups->>Utils: getAndUpdateUser(ctx, payload)
Backups->>DB: load backup, verify backup.user == user._id
Backups->>DB: delete id
Backups-->>Convex: ok
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 10
🧹 Nitpick comments (12)
src/lib/components/app-sidebar.svelte (3)
258-263: Badge replacement LGTM; add hover title for truncated branch.Keep your custom badge, but expose the full branch name on hover for UX/a11y.
Apply this diff:
- <div - class="bg-muted text-muted-foreground pointer-events-none inline-flex h-5 items-center gap-1 truncate rounded border px-1.5 font-mono text-[10px] font-medium opacity-100 select-none" - > + <div + class="bg-muted text-muted-foreground pointer-events-none inline-flex h-5 items-center gap-1 truncate rounded border px-1.5 font-mono text-[10px] font-medium opacity-100 select-none" + title={PUBLIC_BRANCH_NAME ?? 'main'} + aria-label={`Git branch: ${PUBLIC_BRANCH_NAME ?? 'main'}`} + >
256-256: Expose full repo name on hover for truncated label.Add a title so users can see the complete text.
- <div class="truncate">EducationalTools/src</div> + <div class="truncate" title="EducationalTools/src">EducationalTools/src</div>
261-263: Use SvelteKit $env/public for client-side env variables (recommended)vite.config.ts currently injects these via Vite define (so runtime ReferenceError is unlikely), but convert client-side components to $env/static/public for clarity and SvelteKit correctness.
- src/lib/components/app-sidebar.svelte (≈lines 260–263) — apply this diff and add the import:
- <GitBranch class="size-2" /> - {process.env.BRANCH_NAME} + <GitBranch class="size-2" /> + {PUBLIC_BRANCH_NAME ?? 'main'}Add near the top:
import { PUBLIC_BRANCH_NAME } from '$env/static/public';
src/lib/components/providers.svelte (lines ~13 and ~25) — replace process.env.PUBLIC_CONVEX_URL and process.env.PUBLIC_CLERK_PUBLISHABLE_KEY with $env/static/public imports (add
import { PUBLIC_CONVEX_URL, PUBLIC_CLERK_PUBLISHABLE_KEY } from '$env/static/public';and use those constants, e.g.PUBLIC_CONVEX_URLandPUBLIC_CLERK_PUBLISHABLE_KEY || '').Server-side usage (src/convex/utils.ts — process.env.CLERK_JWT_KEY) is fine to keep as process.env.
Ignore third‑party references under static/_app (they're external bundles).
src/convex/schema.ts (1)
15-45: Users table looks good; consider optional fields and future indexes.
usernameandverifiedas required may reject valid Clerk users lacking these. Considerv.optionalunless guaranteed.- If you’ll query by
Optional tweaks:
- username: v.string(), - verified: v.boolean(), + username: v.optional(v.string()), + verified: v.optional(v.boolean()), + }).index('by_email', ['email']).index('clerkid', ['clerkId']) - }).index('clerkid', ['clerkId'])src/convex/backups.ts (1)
58-66: Remove redundant user update and handle missing backup.
- Redundant second
getAndUpdateUsercall.- If
backupis null, throw Not Found before auth check.const backup = await ctx.db.get(args.id); const userInfo = await getAndUpdateUser(ctx, payload); - if (backup?.user !== userInfo?._id) { + if (!backup) { + throw new Error('Not found'); + } + if (backup.user !== userInfo?._id) { throw new Error('Unauthorized'); } - await getAndUpdateUser(ctx, payload); await ctx.db.delete(args.id);src/lib/components/sidebar-auth.svelte (1)
58-147: Commented-out Sync UI: gate behind a feature flag or remove until ready.Long commented blocks add noise. Consider a feature flag to toggle rendering or delete until implementation lands.
src/lib/components/providers.svelte (1)
24-27: Prefer SvelteKit public env imports in client Svelte (or keep Vite define intentionally)vite.config.ts currently injects process.env.PUBLIC_* via define (so the code works), but prefer importing public vars from '$env/static/public' (or '$env/dynamic/public' if truly runtime) for clarity and correctness in client Svelte.
- src/lib/components/providers.svelte — import { PUBLIC_CLERK_PUBLISHABLE_KEY, PUBLIC_CONVEX_URL } from '$env/static/public' and replace process.env.PUBLIC_CLERK_PUBLISHABLE_KEY and process.env.PUBLIC_CONVEX_URL.
- src/lib/components/app-sidebar.svelte — either expose the branch as a PUBLIC_ env var (e.g. PUBLIC_BRANCH_NAME) and import it from '$env/static/public', or keep the current vite.config.ts define that injects process.env.BRANCH_NAME (be explicit about the choice).
- Keep server-only secrets (src/convex/utils.ts — CLERK_JWT_KEY) as server env (process.env or $env/dynamic/private); do not expose to client.
- Optional: remove the Vite 'define' mappings for PUBLIC_* in vite.config.ts if you migrate fully to SvelteKit env imports.
src/convex/sync.ts (2)
58-73: Batch DB updates into a single patch and use explicit undefined checksFewer round‑trips and clearer intent. Also guards future cases where a falsy value (e.g., empty string) should still overwrite.
- if (args.favourites) { - await ctx.db.patch(userInfo._id, { - favourites: args.favourites - }); - } - if (args.history) { - await ctx.db.patch(userInfo._id, { - history: args.history - }); - } - if (args.settings) { - await ctx.db.patch(userInfo._id, { - settings: args.settings - }); - } + const patch: { + favourites?: string[]; + history?: string[]; + settings?: { + experimentalFeatures: boolean; + open: string; + theme: string; + panic: { + enabled: boolean; + key: string; + url: string; + disableExperimentalMode: boolean; + }; + cloak: { mode: string; name: string; icon: string }; + history: boolean; + }; + } = {}; + if (args.favourites !== undefined) patch.favourites = args.favourites; + if (args.history !== undefined) patch.history = args.history; + if (args.settings !== undefined) patch.settings = args.settings; + if (Object.keys(patch).length) { + await ctx.db.patch(userInfo._id, patch); + }
28-43: Tighten validation for enum-like fieldsConstrain values for open/theme/cloak.mode to prevent invalid state persisting server‑side.
- open: v.string(), - theme: v.string(), + open: v.union(v.literal('tab'), v.literal('window')), + theme: v.string(), // consider narrowing to your supported theme keys ... - cloak: v.object({ - mode: v.string(), + cloak: v.object({ + mode: v.union(v.literal('off'), v.literal('blur'), v.literal('on')),Also applies to: 37-41
src/lib/sync.ts (2)
16-18: Guard empty JWT and surface errors; dedupe state resetAvoid calling the mutation with an empty token and log failures.
syncState.current = 'uploading'; - let mutationData: { settings?: any; history?: any; favourites?: any } = {}; + if (!jwt) { + syncState.current = ''; + return; + } + let mutationData: { settings?: any; history?: any; favourites?: any } = {}; @@ - try { - await client.mutation(api.sync.update, { ...mutationData, jwt }); - } catch { - syncState.current = ''; - } - syncState.current = ''; + try { + await client.mutation(api.sync.update, { ...mutationData, jwt }); + } catch (e) { + console.error('[sync.save] update failed', e); + } finally { + syncState.current = ''; + }Also applies to: 42-48
17-17: Leverage generated types for mutation argsUse the Convex API type to avoid any and drift.
- let mutationData: { settings?: any; history?: any; favourites?: any } = {}; + let mutationData: Omit<Parameters<typeof api.sync.update>[0], 'jwt'> = {};src/convex/utils.ts (1)
5-19: Type the verifier return and narrow algorithmsReturn a well-typed payload; optionally restrict alg for defense‑in‑depth.
-export async function verifyJwtAndGetPayload(jwt: string) { +export async function verifyJwtAndGetPayload(jwt: string): Promise<ClerkJwtPayload> { if (!process.env.CLERK_JWT_KEY) { throw new Error('Missing CLERK_JWT_KEY environment variable'); } const publicKey = await jose.importSPKI(process.env.CLERK_JWT_KEY, 'RS256'); if (jwt.length === 0) { throw new Error('Missing JWT'); } - const { payload } = await jose.jwtVerify(jwt, publicKey, {}); + const { payload } = await jose.jwtVerify(jwt, publicKey, { algorithms: ['RS256'] }); if (!payload.sub) { throw new Error('Invalid JWT'); } - return payload; + return payload as ClerkJwtPayload; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (13)
src/convex/backups.ts(3 hunks)src/convex/schema.ts(1 hunks)src/convex/sync.ts(1 hunks)src/convex/types.ts(1 hunks)src/convex/utils.ts(1 hunks)src/lib/components/app-sidebar.svelte(1 hunks)src/lib/components/providers.svelte(2 hunks)src/lib/components/settings.svelte(4 hunks)src/lib/components/sidebar-auth.svelte(3 hunks)src/lib/state.svelte.ts(1 hunks)src/lib/stores.ts(1 hunks)src/lib/sync.ts(1 hunks)src/routes/account/[...slug]/+page.svelte(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{svelte,ts,js,jsx,tsx,css,scss,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Code must be formatted with Prettier before committing
Files:
src/lib/components/app-sidebar.sveltesrc/lib/components/providers.sveltesrc/convex/types.tssrc/lib/components/sidebar-auth.sveltesrc/lib/sync.tssrc/convex/sync.tssrc/convex/schema.tssrc/routes/account/[...slug]/+page.sveltesrc/convex/backups.tssrc/lib/components/settings.sveltesrc/lib/state.svelte.tssrc/convex/utils.tssrc/lib/stores.ts
**/*.{ts,svelte}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Resolve all TypeScript and Svelte component type errors (svelte-check/ts checks must pass)
Files:
src/lib/components/app-sidebar.sveltesrc/lib/components/providers.sveltesrc/convex/types.tssrc/lib/components/sidebar-auth.sveltesrc/lib/sync.tssrc/convex/sync.tssrc/convex/schema.tssrc/routes/account/[...slug]/+page.sveltesrc/convex/backups.tssrc/lib/components/settings.sveltesrc/lib/state.svelte.tssrc/convex/utils.tssrc/lib/stores.ts
**/*.{ts,js,svelte}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure all import paths are correct and case-sensitive
Files:
src/lib/components/app-sidebar.sveltesrc/lib/components/providers.sveltesrc/convex/types.tssrc/lib/components/sidebar-auth.sveltesrc/lib/sync.tssrc/convex/sync.tssrc/convex/schema.tssrc/routes/account/[...slug]/+page.sveltesrc/convex/backups.tssrc/lib/components/settings.sveltesrc/lib/state.svelte.tssrc/convex/utils.tssrc/lib/stores.ts
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: Do not correct spelling errors or grammar mistakes.
Files:
src/lib/components/app-sidebar.sveltesrc/lib/components/providers.sveltesrc/convex/types.tssrc/lib/components/sidebar-auth.sveltesrc/lib/sync.tssrc/convex/sync.tssrc/convex/schema.tssrc/routes/account/[...slug]/+page.sveltesrc/convex/backups.tssrc/lib/components/settings.sveltesrc/lib/state.svelte.tssrc/convex/utils.tssrc/lib/stores.ts
🧠 Learnings (2)
📚 Learning: 2025-08-25T03:31:18.401Z
Learnt from: CR
PR: EducationalTools/src#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-25T03:31:18.401Z
Learning: Applies to src/routes/tools/*/+page.svelte : Add new educational tools by creating a route at src/routes/tools/[tool-name]/ with a +page.svelte implementation
Applied to files:
src/routes/account/[...slug]/+page.svelte
📚 Learning: 2025-08-25T03:31:18.401Z
Learnt from: CR
PR: EducationalTools/src#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-25T03:31:18.401Z
Learning: Applies to src/routes/+page.svelte : Update the tools list in src/routes/+page.svelte when adding a new tool
Applied to files:
src/routes/account/[...slug]/+page.svelte
🧬 Code graph analysis (4)
src/lib/sync.ts (2)
src/lib/state.svelte.ts (1)
syncState(2-2)src/lib/stores.ts (3)
preferencesStore(3-19)historyStore(22-22)favoritesStore(21-21)
src/convex/sync.ts (1)
src/convex/utils.ts (3)
verifyJwtAndGetPayload(6-19)getUser(21-35)getAndUpdateUser(37-69)
src/convex/backups.ts (1)
src/convex/utils.ts (3)
verifyJwtAndGetPayload(6-19)getUser(21-35)getAndUpdateUser(37-69)
src/convex/utils.ts (1)
src/convex/_generated/server.d.ts (2)
QueryCtx(106-106)MutationCtx(114-114)
🔇 Additional comments (6)
src/lib/components/providers.svelte (1)
26-26: Confirm reactive theme switching works at runtime.If
mode.currentisn’t a Svelte reactive source in this context,appearancemay not update on theme change. Verify visually; if not reactive, wrap in a reactive statement or derived value.src/routes/account/[...slug]/+page.svelte (1)
21-36: LGTM: Account profile page wiring.
UserProfilewith path routing and appearance overrides looks correct and matches provider theming.src/convex/backups.ts (1)
40-46: Create: good switch to user IDs; ensure utils uses correct name fields.This relies on
getAndUpdateUser. Double-check it assignslastNamefrom payload.lastName, not payload.firstname.src/lib/components/sidebar-auth.svelte (1)
149-163: LGTM: Direct link to /account improves discoverability.The avatar + username anchor is clean, and the host-gated account UX remains intact.
src/lib/components/settings.svelte (1)
80-90: Verify custom components accept DOM-style event props or switch to Svelte eventsrg returned no matches for
export let onclick|oninputunder src/lib/components/ui and svelte-check failed (missing npm script). Confirm components used in src/lib/components/settings.svelte (lines 80–90, 102–106, 128–132, 138–139, 144–155, 159–165, 166–175, 187–205, 206–220) either exportonclick/oninput(and forward them to a native element) or have callers use Svelte events (on:click/on:input). If not, either export/forward those props (or forward$$restProps) or change callers to Svelte events; add/enable asvelte-checkscript and re-run type checks.src/convex/sync.ts (1)
15-19: Verify spelling consistency: "favourites" vs "favorites" across backend, UI and DBFile: src/convex/sync.ts (lines 15–19; also applies to 45–47).
Sandbox ripgrep returned no searchable files; please run locally and confirm with:
rg -nP --hidden --no-ignore -S '\bfavou?rites\b' .
rg -nP --hidden --no-ignore -S 'favoritesStore|favourites' .
rg -nP --hidden --no-ignore -S 'table.*users|schema.*users' .If both spellings exist, normalize names or add a backwards-compatible mapping in the DB and all callers to avoid silent data splits.
merging now to avoid conflicts
Summary by CodeRabbit
New Features
Improvements
Style