Feat/offline first powersync - #8
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 ignored due to path filters (1)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe change introduces PowerSync-backed offline storage for ideas, cached offline identity resolution, authenticated token and upload routes, local database mutations, provider integration, and updated tests and documentation. ChangesPowerSync synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant IdeaHook
participant PowerSyncDatabase
participant BackendConnector
participant TokenRoute
participant UploadRoute
participant IdeasDatabase
User->>IdeaHook: create or update idea
IdeaHook->>PowerSyncDatabase: write local row
PowerSyncDatabase-->>IdeaHook: updated query result
BackendConnector->>TokenRoute: request credentials
TokenRoute-->>BackendConnector: PowerSync JWT
BackendConnector->>UploadRoute: upload queued CRUD operations
UploadRoute->>IdeasDatabase: apply database changes
IdeasDatabase-->>UploadRoute: operation result
UploadRoute-->>BackendConnector: upload response
BackendConnector-->>PowerSyncDatabase: complete transaction
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
React Doctor found 7 issues in 5 files · 7 warnings · score 81 / 100 (Needs work) · vs 7 warnings
|
There was a problem hiding this comment.
Pull request overview
This PR converts Trojes’ idea workflows to an offline-first model using PowerSync (local SQLite mirror + bidirectional sync), replacing the prior SWR + REST-driven idea list/mutations and adding server endpoints to mint PowerSync credentials and accept upload batches.
Changes:
- Introduces PowerSync client schema/db/connector + provider wiring, and moves ideas/pins hooks to PowerSync
useQuery. - Adds offline identity caching + local-first idea insert/update/delete paths.
- Adds PowerSync server routes (
/api/powersync/token,/api/powersync/upload) and updates docs/config/test scaffolding accordingly.
Reviewed changes
Copilot reviewed 33 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/lib/offline-identity.test.ts | Adds unit coverage for cached offline user id resolution paths. |
| tests/lib/create-idea.test.ts | Updates tests to validate local DB insert behavior instead of MSW API posting. |
| tests/integration/use-ideas.test.tsx | Reworks integration tests to use a PowerSync fake DB + mocked useQuery. |
| tests/integration/synchronization.test.tsx | Updates cross-hook synchronization tests for local DB-backed reads/writes. |
| tests/integration/auth-callbacks.test.ts | Adjusts callback test typing and callback extraction. |
| tests/helpers/powersync-fake.ts | Adds an in-memory SQL-surface fake DB for PowerSync-backed hooks/tests. |
| README.md | Documents PowerSync offline sync behavior and required env vars. |
| public/sw.js | Bumps service worker cache version. |
| package.json | Adds PowerSync dependencies and a postinstall to copy worker assets. |
| notes/references.md | Records the PowerSync SDK versions and testing approach reference notes. |
| next.config.mjs | Enables turbopack config object. |
| MISTAKES.md | Captures offline identity fallback and DB id-type drift learnings. |
| lib/swr-helpers.ts | Removes SWR cache helper utilities now replaced by local DB queries. |
| lib/powersync/schema.ts | Defines the PowerSync schema for the ideas table. |
| lib/powersync/mappers.ts | Adds row→domain mapping (including tag parsing). |
| lib/powersync/db.ts | Creates the PowerSync database instance and exports a backend connector. |
| lib/powersync/connector.ts | Implements credential fetch + upload of queued CRUD ops to the server. |
| lib/offline-identity.ts | Implements cached user id storage + offline-aware user id resolution. |
| lib/create-idea.ts | Switches create/insert to local SQLite writes via PowerSync. |
| lib/api-client.ts | Removes ideasApi REST helpers (ideas now use PowerSync). |
| LEARNINGS.md | Documents NextAuth offline session fetch behavior/logging constraints. |
| hooks/use-pinned-ideas.ts | Replaces SWR pinned ideas fetch with a PowerSync query. |
| hooks/use-ideas.ts | Replaces SWR infinite list with PowerSync query + local mutation methods. |
| hooks/use-hydrated.ts | Adds hydration guard helper to suppress SSR/hydration loading mismatch. |
| db/users.ts | Tightens return typing for findUserIdByEmail. |
| db/ideas.ts | Adds upsert types and a new upsertIdea used by PowerSync upload. |
| components/providers/session-provider.tsx | Sets refetchWhenOffline={false} on NextAuth SessionProvider. |
| components/providers/powersync-provider.tsx | Wires PowerSync connect/clear behavior to auth status + offline identity. |
| components/editor/plugins/emoji-picker-plugin.tsx | Makes emoji list dynamic import resilient when offline. |
| app/layout.tsx | Wraps the app in the new PowerSync provider. |
| app/api/powersync/upload/route.ts | Adds server endpoint to apply client-uploaded CRUD operations. |
| app/api/powersync/token/route.ts | Adds server endpoint to mint PowerSync JWT credentials. |
| .gitignore | Ignores generated PowerSync worker assets and local agent folders. |
| .env.example | Documents required PowerSync env vars. |
| bun.lock | Locks newly added PowerSync + wa-sqlite + jose dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export async function upsertIdea({ | ||
| id, | ||
| userId, | ||
| values, | ||
| }: { | ||
| id: string | ||
| userId: string | ||
| values: IdeaUpsert | ||
| }) { | ||
| const db = getDb() | ||
| const [idea] = await db | ||
| .insert(ideas) | ||
| .values({ id, user_id: userId, ...values }) | ||
| .onConflictDoUpdate({ | ||
| target: ideas.id, | ||
| set: values, | ||
| }) | ||
| .returning() | ||
|
|
||
| return idea ?? null | ||
| } |
| const result = (await res.json()) as { success: boolean; errors?: string[] } | ||
|
|
||
| if (!result.success) { | ||
| console.warn("Upload had errors:", result.errors) | ||
| } | ||
|
|
||
| await transaction.complete() | ||
| } catch (ex) { |
| @@ -0,0 +1,60 @@ | |||
| import { UpdateType } from "@powersync/web" | |||
| const hydrated = useHydrated() | ||
| const { data: session } = useSession() | ||
| const userId = session?.user?.id ?? getCachedUserId() | ||
| const [size, setSize] = useState(1) | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
lib/powersync/db.ts (1)
1-3: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider marking this module client-only.
The database is constructed at import time. Add
"use client"or wrap the construction in a lazy getter. This prevents an accidental server import from instantiating a browser-only SQLite database during SSR or prerender.🤖 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 `@lib/powersync/db.ts` around lines 1 - 3, Mark the module containing the import-time PowerSyncDatabase construction as client-only by adding the appropriate "use client" directive at the top of the module. Keep the existing PowerSyncDatabase initialization and exports unchanged.lib/powersync/connector.ts (2)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
try/catch.The
catchblock only rethrows. Delete both thetryand thecatch.♻️ Proposed change
- } catch (ex) { - throw ex - } }🤖 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 `@lib/powersync/connector.ts` around lines 56 - 58, Remove the no-op try/catch surrounding the affected logic in the connector flow, including the catch block that only rethrows ex, and leave the enclosed statements executing directly with their existing behavior.
11-27: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a timeout and validate the token response body.
The
fetchhas noAbortSignal. A hung token request stalls the credential refresh. The response body is also cast without a check. Ifendpointortokenis missing, PowerSync receivesundefinedcredentials and fails with an opaque error.♻️ Proposed change
- const res = await fetch("/api/powersync/token", { cache: "no-store" }) + const res = await fetch("/api/powersync/token", { + cache: "no-store", + signal: AbortSignal.timeout(10_000), + }) if (!res.ok) { throw new Error(`Failed to get PowerSync credentials: ${res.status}`) } const body = (await res.json()) as { endpoint: string token: string } + if (!body?.endpoint || !body?.token) { + throw new Error("PowerSync token response is missing endpoint or token") + } + return { endpoint: body.endpoint, token: body.token, }🤖 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 `@lib/powersync/connector.ts` around lines 11 - 27, Update fetchCredentials to use an AbortSignal timeout for the token fetch, ensuring hung requests terminate. Validate the parsed response body before returning credentials, requiring endpoint and token to be present and valid; reject malformed responses with a clear error instead of returning undefined values.lib/powersync/mappers.ts (1)
19-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
parseTagsis duplicated in the upload route.
app/api/powersync/upload/route.tslines 18-34 implements the same JSON tag parsing. Extract one shared helper. See the consolidated comment for the full set of sites.🤖 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 `@lib/powersync/mappers.ts` around lines 19 - 27, Extract the JSON tag parsing logic from parseTags in mappers.ts into a shared helper, then update both parseTags and the upload route’s duplicate implementation to reuse it. Preserve the existing null handling, JSON parsing behavior, and string-only filtering.app/api/powersync/upload/route.ts (1)
156-174: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winApply the batch in one transaction and bound its size.
The loop applies each operation with a separate database round trip and no enclosing transaction. A failure part way through leaves the batch half applied.
operationsis also unbounded, so a single request can hold a serial chain of writes open for an arbitrary time.Wrap the loop in a database transaction and reject batches above a maximum operation count.
🤖 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 `@app/api/powersync/upload/route.ts` around lines 156 - 174, Update the upload handling around operations and applyOp to reject batches exceeding the established maximum operation count, then execute the entire batch within one database transaction so all operations commit or roll back together. Preserve the existing per-operation error collection and response behavior while ensuring transaction failures do not leave partial writes.package.json (1)
14-14: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the build against skipped install scripts.
public/@powersync/is git-ignored and produced only bypostinstall. If a deployment or CI job installs with--ignore-scripts,/@powersync/worker.jsis missing andlib/powersync/db.tsfails at runtime. Add the samecopy-assetscall to thebuildscript, or verify that every install path runs lifecycle scripts.trustedDependenciesonly covers Bun, so npm/pnpm installs need separate confirmation.♻️ Proposed change
- "postinstall": "powersync-web copy-assets -o public", + "postinstall": "powersync-web copy-assets -o public", + "prebuild": "powersync-web copy-assets -o public",Also applies to: 89-92
🤖 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 `@package.json` at line 14, Update the package.json build workflow to invoke powersync-web copy-assets -o public in addition to the postinstall hook, ensuring public/@powersync/worker.js exists when dependencies were installed with skipped lifecycle scripts. Preserve the existing postinstall behavior and integrate the asset-copy command into the build script.tests/helpers/powersync-fake.ts (2)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
setErrordoes not affectexecute.
selectreturns[]whilecurrentErroris set, butexecuteignores it. Tests that need a failing write must replaceexecuteby hand, astests/integration/use-ideas.test.tsxdoes at lines 143-146. Rejecting fromexecutewhile an error is set would make the fake consistent and simplify those tests.♻️ Proposed change
async function execute(sql: string, params: unknown[]): Promise<{ rowsAffected: number }> { + if (currentError) throw currentError const trimmed = sql.trim()🤖 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 `@tests/helpers/powersync-fake.ts` around lines 108 - 110, Update the fake’s execute function to reject with currentError when setError has configured an error, before processing the SQL; otherwise preserve the existing execution behavior and return value.
100-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the single-row detection in
getOptional.
sql.includes("id = ?")also matchesuser_id = ?. Any list query that filters byuser_idtakes the single-row branch and matchesparams[0]againstr.id. No current caller hits this, but the helper is shared and the failure would be silent.Match the full clause instead.
♻️ Proposed change
- if (sql.includes("id = ?")) { + if (/\bWHERE\s+id\s*=\s*\?/i.test(sql)) {🤖 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 `@tests/helpers/powersync-fake.ts` around lines 100 - 106, Update getOptional to detect the exact id = ? clause rather than using a substring match that also accepts user_id = ?. Preserve the existing direct row lookup for genuine id filters and route other queries, including user_id filters, through select.tests/integration/synchronization.test.tsx (2)
34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isLoadingis alwaysfalsein this mock.
holder.dbis assigned inbeforeEach, so!dbnever evaluates totrue. EverywaitFor(() => expect(result.current.isLoading).toBe(false))in the suites passes on the first tick and asserts nothing. Either driveisLoadingfrom an explicit flag on the fake, or drop those assertions and wait on the data instead.🤖 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 `@tests/integration/synchronization.test.tsx` around lines 34 - 40, Update the mock query state returned by the fake around holder.db so isLoading reflects an explicit loading flag rather than the always-initialized db value, or remove the ineffective isLoading assertions and wait for the expected data instead. Ensure synchronization tests still verify completion meaningfully.
6-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared PowerSync test harness.
Lines 6-74 duplicate
tests/integration/use-ideas.test.tsxlines 6-73 almost exactly: theholderobject, the threevi.mockcalls, therowfactory, andseed. Move them intotests/helpers/, next topowersync-fake.ts, and import them in both suites. The mock factories must stay hoisted, so export a setup function that the suites call.🤖 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 `@tests/integration/synchronization.test.tsx` around lines 6 - 74, Extract the duplicated holder, PowerSync/NextAuth mock setup, row factory, and seed helpers into a shared module under tests/helpers next to powersync-fake.ts. Export a hoisted-safe setup function for the vi.mock factories, then invoke it from both synchronization and use-ideas suites while importing the shared row and seed helpers; preserve their existing behavior and types.tests/lib/offline-identity.test.ts (1)
10-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe backing
storeis never read.
localStorageMock.getItemis replaced inbeforeEachwith an implementation that builds its ownMap. Thestorecreated at line 11 therefore only receivessetItemandremoveItemwrites and never serves a read. The rebuiltMapis also allocated on everygetItemcall.Reset one shared
MapinbeforeEachand keep the originalgetItemimplementation. That also lets you assert the round trip afterresolveUserIdrefreshes the cache.♻️ Proposed change
const localStorageMock = vi.hoisted(() => { const store = new Map<string, string>() return { + store, getItem: vi.fn((key: string) => store.get(key) ?? null), setItem: vi.fn((key: string, value: string) => store.set(key, value)), removeItem: vi.fn((key: string) => store.delete(key)), } }) beforeEach(() => { vi.clearAllMocks() - localStorageMock.getItem.mockImplementation((key: string) => { - const store = new Map<string, string>([ - ["trojes:offline-user-id", "cached-user"], - ]) - return store.get(key) ?? null - }) + localStorageMock.store.clear() + localStorageMock.store.set("trojes:offline-user-id", "cached-user")The last test then clears the store instead of overriding
getItem:- localStorageMock.getItem.mockImplementation(() => null) + localStorageMock.store.clear()🤖 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 `@tests/lib/offline-identity.test.ts` around lines 10 - 26, Update the shared store setup in the localStorageMock fixture and beforeEach: clear and seed the existing Map before each test, while preserving the original getItem implementation instead of replacing it with a per-call Map. Adjust the final test to clear the shared store when simulating an empty cache, so setItem/getItem round-trip behavior remains testable.hooks/use-ideas.ts (1)
89-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated mutation wrapper.
updatePin,updateColor,updateContent, andpermanentDeleteshare the same shape: build a timestamp, run one statement, log on failure, and return{ ok }. A small helper removes the duplication and keeps the error handling consistent.♻️ Example helper
+ const run = useCallback( + async (label: string, sql: string, params: unknown[]): Promise<{ ok: boolean }> => { + try { + await db.execute(sql, params) + return { ok: true } + } catch (error) { + console.error(`Failed to ${label}:`, error) + return { ok: false } + } + }, + [], + ) + + const updatePin = useCallback( + (id: string, pinned: boolean) => + run("update pin", "UPDATE ideas SET pinned = ?, updated_at = ? WHERE id = ?", [ + pinned ? 1 : 0, + new Date().toISOString(), + id, + ]), + [run], + )🤖 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 `@hooks/use-ideas.ts` around lines 89 - 151, Extract the shared database mutation and error-handling flow from updatePin, updateColor, updateContent, and permanentDelete into a small helper, parameterized by the SQL statement, values, and operation label. Refactor each callback to use the helper while preserving timestamp updates, existing SQL parameters, error logging context, and the { ok: boolean } result contract.
🤖 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 `@app/api/powersync/upload/route.ts`:
- Around line 44-53: Update the normalization logic for opData.status and
opData.source to reject values not present in VALID_STATUSES or VALID_SOURCES
instead of defaulting to "inbox" or "web". Return a per-operation error
containing the invalid value, while preserving accepted values and the existing
handling for omitted fields.
- Around line 126-154: Update app/api/powersync/upload/route.ts in POST (lines
126-154) to return retryable non-2xx responses for authentication and
infrastructure failures, while retaining 2xx responses only for permanently
invalid operations. Update lib/powersync/connector.ts in the upload transaction
handling (lines 51-55) to throw on retryable failure responses instead of
calling transaction.complete(), allowing PowerSync to retry the queued
transaction.
In `@components/providers/powersync-provider.tsx`:
- Around line 28-42: Remove the unauthenticated-state database clearing from the
status handling around isOnline(), including clearCachedUserId() and
db.disconnectAndClear(). Preserve the local PowerSync mirror and pending writes
for all session-fetch failures; move cleanup to the existing explicit confirmed
logout or user-switch flow, if available.
In `@db/ideas.ts`:
- Around line 106-126: Update upsertIdea’s onConflictDoUpdate configuration to
restrict conflict updates to rows whose ideas.user_id matches the supplied
userId, while preserving the existing insert and values update behavior. Reuse
the ownership-filtering pattern already used by PATCH, UPDATE, and DELETE so
client-supplied IDs cannot modify another user’s idea.
In `@hooks/use-ideas.ts`:
- Around line 23-28: Guard cached identity resolution with hydrated state in
hooks/use-ideas.ts lines 23-28 and hooks/use-pinned-ideas.ts lines 10-12: update
each userId expression so getCachedUserId() is only evaluated when hydrated is
true, while preserving the session user ID fallback and null otherwise.
In `@lib/create-idea.ts`:
- Around line 13-16: Fix the parameter binding in createIdea by ensuring the
INSERT statement’s six placeholders align with all six values, with deleted_at
explicitly bound as NULL rather than now. Update tests around insertIdea to
assert the complete parameter list and verify the inserted record has a null
deleted_at value.
In `@tests/integration/use-ideas.test.tsx`:
- Around line 102-114: The create() integration test should assert the persisted
created idea’s deleted_at, created_at, and updated_at values, not just its
content and collection count. Extend the assertions in the “create() inserts
locally and returns ok” test to locate “New test idea” and verify deleted_at is
null and both timestamp fields are populated, catching any parameter-binding
shift.
In `@tests/lib/create-idea.test.ts`:
- Around line 33-36: Update the assertions for the executeMock call in the
create-idea test to verify the complete parameter array: assert its length
equals the number of SQL ? placeholders and retain value checks for the expected
parameters. Use the captured sql and params symbols without changing the
production create-idea implementation.
---
Nitpick comments:
In `@app/api/powersync/upload/route.ts`:
- Around line 156-174: Update the upload handling around operations and applyOp
to reject batches exceeding the established maximum operation count, then
execute the entire batch within one database transaction so all operations
commit or roll back together. Preserve the existing per-operation error
collection and response behavior while ensuring transaction failures do not
leave partial writes.
In `@hooks/use-ideas.ts`:
- Around line 89-151: Extract the shared database mutation and error-handling
flow from updatePin, updateColor, updateContent, and permanentDelete into a
small helper, parameterized by the SQL statement, values, and operation label.
Refactor each callback to use the helper while preserving timestamp updates,
existing SQL parameters, error logging context, and the { ok: boolean } result
contract.
In `@lib/powersync/connector.ts`:
- Around line 56-58: Remove the no-op try/catch surrounding the affected logic
in the connector flow, including the catch block that only rethrows ex, and
leave the enclosed statements executing directly with their existing behavior.
- Around line 11-27: Update fetchCredentials to use an AbortSignal timeout for
the token fetch, ensuring hung requests terminate. Validate the parsed response
body before returning credentials, requiring endpoint and token to be present
and valid; reject malformed responses with a clear error instead of returning
undefined values.
In `@lib/powersync/db.ts`:
- Around line 1-3: Mark the module containing the import-time PowerSyncDatabase
construction as client-only by adding the appropriate "use client" directive at
the top of the module. Keep the existing PowerSyncDatabase initialization and
exports unchanged.
In `@lib/powersync/mappers.ts`:
- Around line 19-27: Extract the JSON tag parsing logic from parseTags in
mappers.ts into a shared helper, then update both parseTags and the upload
route’s duplicate implementation to reuse it. Preserve the existing null
handling, JSON parsing behavior, and string-only filtering.
In `@package.json`:
- Line 14: Update the package.json build workflow to invoke powersync-web
copy-assets -o public in addition to the postinstall hook, ensuring
public/@powersync/worker.js exists when dependencies were installed with skipped
lifecycle scripts. Preserve the existing postinstall behavior and integrate the
asset-copy command into the build script.
In `@tests/helpers/powersync-fake.ts`:
- Around line 108-110: Update the fake’s execute function to reject with
currentError when setError has configured an error, before processing the SQL;
otherwise preserve the existing execution behavior and return value.
- Around line 100-106: Update getOptional to detect the exact id = ? clause
rather than using a substring match that also accepts user_id = ?. Preserve the
existing direct row lookup for genuine id filters and route other queries,
including user_id filters, through select.
In `@tests/integration/synchronization.test.tsx`:
- Around line 34-40: Update the mock query state returned by the fake around
holder.db so isLoading reflects an explicit loading flag rather than the
always-initialized db value, or remove the ineffective isLoading assertions and
wait for the expected data instead. Ensure synchronization tests still verify
completion meaningfully.
- Around line 6-74: Extract the duplicated holder, PowerSync/NextAuth mock
setup, row factory, and seed helpers into a shared module under tests/helpers
next to powersync-fake.ts. Export a hoisted-safe setup function for the vi.mock
factories, then invoke it from both synchronization and use-ideas suites while
importing the shared row and seed helpers; preserve their existing behavior and
types.
In `@tests/lib/offline-identity.test.ts`:
- Around line 10-26: Update the shared store setup in the localStorageMock
fixture and beforeEach: clear and seed the existing Map before each test, while
preserving the original getItem implementation instead of replacing it with a
per-call Map. Adjust the final test to clear the shared store when simulating an
empty cache, so setItem/getItem round-trip behavior remains testable.
🪄 Autofix
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: d360d87b-58e0-41fa-99d4-2baadfdf558f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.env.example.gitignoreLEARNINGS.mdMISTAKES.mdREADME.mdapp/api/powersync/token/route.tsapp/api/powersync/upload/route.tsapp/layout.tsxcomponents/editor/plugins/emoji-picker-plugin.tsxcomponents/providers/powersync-provider.tsxcomponents/providers/session-provider.tsxdb/ideas.tsdb/users.tshooks/use-hydrated.tshooks/use-ideas.tshooks/use-pinned-ideas.tslib/api-client.tslib/create-idea.tslib/offline-identity.tslib/powersync/connector.tslib/powersync/db.tslib/powersync/mappers.tslib/powersync/schema.tslib/swr-helpers.tsnext.config.mjsnotes/references.mdpackage.jsonpublic/sw.jstests/helpers/powersync-fake.tstests/integration/auth-callbacks.test.tstests/integration/synchronization.test.tsxtests/integration/use-ideas.test.tsxtests/lib/create-idea.test.tstests/lib/offline-identity.test.tstsconfig.tsbuildinfo
💤 Files with no reviewable changes (2)
- lib/swr-helpers.ts
- lib/api-client.ts
| if (opData.source !== undefined) { | ||
| normalized.source = VALID_SOURCES.includes(opData.source as IdeaSource) | ||
| ? (opData.source as IdeaSource) | ||
| : "web" | ||
| } | ||
| if (opData.status !== undefined) { | ||
| normalized.status = VALID_STATUSES.includes(opData.status as IdeaStatus) | ||
| ? (opData.status as IdeaStatus) | ||
| : "inbox" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject invalid status and source instead of coercing them.
An unrecognized status becomes "inbox". That silently moves an archived or deleted idea back to the inbox. An unrecognized source becomes "web". Return a per-operation error so the caller sees the bad value.
🤖 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 `@app/api/powersync/upload/route.ts` around lines 44 - 53, Update the
normalization logic for opData.status and opData.source to reject values not
present in VALID_STATUSES or VALID_SOURCES instead of defaulting to "inbox" or
"web". Return a per-operation error containing the invalid value, while
preserving accepted values and the existing handling for omitted fields.
| export async function POST(request: NextRequest) { | ||
| let userId: string | null | ||
| try { | ||
| userId = await getAuthenticatedUserId(request) | ||
| } catch (error) { | ||
| console.error("Failed to authenticate upload:", error) | ||
| return NextResponse.json( | ||
| { success: false, error: "Authentication failed" }, | ||
| { status: 200 }, | ||
| ) | ||
| } | ||
|
|
||
| if (!userId) { | ||
| return NextResponse.json( | ||
| { success: false, error: "Unauthorized" }, | ||
| { status: 200 }, | ||
| ) | ||
| } | ||
|
|
||
| let body: { operations?: UploadOp[] } | ||
| try { | ||
| body = await request.json() | ||
| } catch (error) { | ||
| console.error("Invalid upload payload:", error) | ||
| return NextResponse.json( | ||
| { success: false, error: "Invalid JSON payload" }, | ||
| { status: 200 }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Queued offline writes are discarded when an upload fails. The upload route signals every failure with HTTP 200 and a success: false body, and the connector treats any 200 as delivered and completes the CRUD transaction. Together these two halves erase the local queue for failures that should be retried, including an expired session after a period offline.
app/api/powersync/upload/route.ts#L126-L154: return a retryable non-2xx status for authentication failures and infrastructure errors; keep 2xx only for operations that are permanently invalid.lib/powersync/connector.ts#L51-L55: throw instead of callingtransaction.complete()when the response reports a retryable failure, so PowerSync retries the same transaction.
📍 Affects 2 files
app/api/powersync/upload/route.ts#L126-L154(this comment)lib/powersync/connector.ts#L51-L55
🤖 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 `@app/api/powersync/upload/route.ts` around lines 126 - 154, Update
app/api/powersync/upload/route.ts in POST (lines 126-154) to return retryable
non-2xx responses for authentication and infrastructure failures, while
retaining 2xx responses only for permanently invalid operations. Update
lib/powersync/connector.ts in the upload transaction handling (lines 51-55) to
throw on retryable failure responses instead of calling transaction.complete(),
allowing PowerSync to retry the queued transaction.
| it("create() inserts locally and returns ok", async () => { | ||
| const { result } = renderHook(() => useIdeas({ status: "inbox" })) | ||
| await waitFor(() => expect(result.current.isLoading).toBe(false)) | ||
|
|
||
| await expect( | ||
| act(async () => result.current.create("New test idea")), | ||
| ).resolves.toEqual({ ok: true }) | ||
|
|
||
| await waitFor(() => expect(result.current.ideas).toHaveLength(3)) | ||
| expect( | ||
| result.current.ideas.some((i) => i.content === "New test idea"), | ||
| ).toBe(true) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the persisted field values, not only the count.
The test checks the row count and the content. It does not check deleted_at, created_at, or updated_at. That gap hides the placeholder/parameter mismatch in lib/create-idea.ts lines 13-16, where deleted_at is bound to a timestamp and updated_at is left unbound. Add assertions on the created idea so a binding shift fails the suite.
💚 Suggested assertions
await waitFor(() => expect(result.current.ideas).toHaveLength(3))
- expect(
- result.current.ideas.some((i) => i.content === "New test idea"),
- ).toBe(true)
+ const created = result.current.ideas.find((i) => i.content === "New test idea")
+ expect(created).toBeDefined()
+ expect(created?.deleted_at).toBeNull()
+ expect(created?.updated_at).toBeTruthy()🤖 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 `@tests/integration/use-ideas.test.tsx` around lines 102 - 114, The create()
integration test should assert the persisted created idea’s deleted_at,
created_at, and updated_at values, not just its content and collection count.
Extend the assertions in the “create() inserts locally and returns ok” test to
locate “New test idea” and verify deleted_at is null and both timestamp fields
are populated, catching any parameter-binding shift.
- Bind deleted_at as literal NULL so the ideas INSERT no longer has a placeholder/param mismatch that left updated_at unbound. - Guard upsertIdea's conflict-update with a user_id ownership condition so one account cannot overwrite another user's idea. - Return non-2xx from the upload route for auth/infrastructure failures so the client keeps its local queue and retries instead of completing and erasing offline writes; keep 2xx only for permanently invalid operations. - Stop clearing the local mirror on session 'unauthenticated' (which a transient fetch failure also produces); wipe only on explicit sign-out and on a server-confirmed switch to a different user. - Gate getCachedUserId() behind useHydrated in use-ideas and use-pinned-ideas to avoid a hydration mismatch. - Add tests covering placeholder/param alignment, persisted field values, and provider user-switch clearing.
Fixes two security advisories affecting <=4.24.14: an OAuth state cookie provider binding issue (GHSA-x445-f3h2-j279) and a malformed-Bearer getToken DoS.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| for (const op of operations) { | ||
| try { | ||
| const result = await applyOp(op, userId) |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This makes the for…of loop slow because each await runs one after another, so collect the independent calls & run them together with await Promise.all(items.map(...))
Fix → Collect the items, then use await Promise.all(items.map(...)) so independent work runs at the same time
| .toUpperCase() | ||
| .slice(0, 2) || "U"; | ||
|
|
||
| const handleSignOut = async () => { |
There was a problem hiding this comment.
React Doctor · react-doctor/prefer-module-scope-pure-function (warning)
handleSignOut inside SettingsAccount uses no local state but is rebuilt on every render, so it wastes work & breaks memoized children. Move it to the top of the file, outside the component.
Fix → Move the function above the component, at the top of the file. It doesn't use local state, so rebuilding it each update is wasted work.
| export function useHydrated(): boolean { | ||
| const [hydrated, setHydrated] = useState(false) | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
React Doctor · react-doctor/rendering-hydration-no-flicker (warning)
useEffect(setState, []) runs after the first paint, so users can see the initial state flash. Initialize from a render-safe value or use useSyncExternalStore for external values.
Fix → Initialize state from a render-safe value before the first paint, or read external mutable values with useSyncExternalStore.
| const [hydrated, setHydrated] = useState(false) | ||
|
|
||
| useEffect(() => { | ||
| setHydrated(true) |
There was a problem hiding this comment.
React Doctor · react-doctor/no-initialize-state (warning)
Your users see an extra render with empty "hydrated" because a useEffect sets its starting value.
Fix → Pass the initial value directly to useState() instead of setting it from a mount-only useEffect. For SSR hydration, prefer useSyncExternalStore().
Summary by CodeRabbit