test(api): add comprehensive sync API and encryption tests#125
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a comprehensive test suite for the API package, including Vitest configuration, test helpers, and integration tests covering encryption, health checks, and sync endpoints (notebooks, pull, push, status, tags). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/tests/encryption-roundtrip.test.ts`:
- Around line 15-23: The decrypt function assumes ciphertext.split(':') returns
three parts and proceeds to decode buffers, which leads to unclear errors for
malformed input; update decrypt to validate that ciphertext is a non-empty
string, that splitting on ':' yields exactly three segments (ivB64, dataB64,
tagB64), and that each segment decodes to a Buffer (and iv/authTag have expected
lengths for ALGORITHM) before calling createDecipheriv, throwing a clear,
descriptive error if any check fails; reference the decrypt function and
variables ivB64/dataB64/tagB64, iv, data, authTag, and createDecipheriv when
making these checks.
In `@packages/api/tests/helpers.ts`:
- Around line 54-57: The test setup passes authToken using the pattern
env.TURSO_AUTH_TOKEN || undefined which can silently allow empty strings and is
duplicated; extract a single helper (e.g., createTestClient) that takes the Env
object and returns createClient({ url: env.TURSO_DATABASE_URL, authToken:
env.TURSO_AUTH_TOKEN ? env.TURSO_AUTH_TOKEN : undefined }) and replace all
inline createClient calls with createTestClient to centralize logic and avoid
passing empty-string tokens.
- Around line 68-75: The cleanupTestDb function silently swallows all errors
when unlinkSync(filePath) fails; update cleanupTestDb to only ignore
file-not-found errors (ENOENT) by catching the error thrown by unlinkSync,
checking err.code === 'ENOENT' and returning silently for that case, but
rethrowing or logging any other error (e.g., permission or lock errors) so test
failures surface; reference the cleanupTestDb function, unlinkSync call and
env.TURSO_DATABASE_URL/filePath when making this change.
- Around line 32-47: The SCHEMA_SQL constant in packages/api/tests/helpers.ts is
a manually maintained SQL string that can drift from the Drizzle-managed
production schema; replace it by programmatically deriving the test schema from
the Drizzle definitions or add a CI-time validation that fails if they differ.
Concretely, update helpers.ts to remove the hardcoded SCHEMA_SQL and instead
import the Drizzle table/schema definitions (from packages/api/src/db/schema.ts
or the module that exports your Drizzle schema) and call the same
schema-generation utility used in production (or use drizzle-kit/Drizzle's
schema-generation API) to produce the SQL used to initialize the test DB at
runtime, or add a new test/CI step that reads SCHEMA_SQL and compares it to the
generated schema SQL and fails if mismatched; target the SCHEMA_SQL symbol and
the test DB initialization code in helpers.ts for this change.
In `@packages/api/tests/sync-notebooks.test.ts`:
- Around line 25-55: The seedNotebookSyncLog helper is duplicated; move its
implementation into the shared helpers module (e.g., add seedNotebookSyncLog to
helpers.ts) and have tests import it instead of redefining it. Keep the same
logic and references (seedNotebookSyncLog, createClient, env.TURSO_DATABASE_URL,
env.TURSO_AUTH_TOKEN) in the shared helper, remove the duplicate implementation
from packages/api/tests/sync-notebooks.test.ts (and any other tests like
sync-tags.test.ts), and update imports in those test files to import
seedNotebookSyncLog from helpers.ts.
In `@packages/api/tests/sync-pull.test.ts`:
- Around line 83-88: The test "returns 401 without auth" currently allows 500 as
passing which masks auth/onError regressions; change the assertion to require a
strict 401 by replacing the loosened expect([401, 500]).toContain(res.status)
with a strict equality assert (e.g., expect(res.status).toBe(401)) for the
response from app.request('/sync?cursor=0', {}, env) so unauthenticated requests
fail only with 401; keep the rest of the test and the unauthenticated env as-is.
- Around line 24-66: The test defines duplicate, schema-coupled functions
seedSyncLog and getSyncCursor locally; remove these local implementations and
instead import the shared helpers (the shared seedSyncLog and getSyncCursor)
from the project’s test helpers module, then update the test to call the
imported seedSyncLog and getSyncCursor symbols so there’s a single canonical
implementation and no raw-SQL duplication.
In `@packages/api/tests/sync-push.test.ts`:
- Around line 84-102: This test reuses the shared proToken which allows previous
pushes to inflate the assigned version; create and use a fresh test user token
instead of proToken (e.g., obtain a new token via your test helper) and call
pushChanges with that fresh token and deviceA so the first created note gets
version === 1, then change the assertion to
expect(body.results[0].version).toBe(1) while still keeping the other
expectations; locate the push invocation (pushChanges) and replace proToken with
the new fresh token variable.
In `@packages/api/tests/sync-status.test.ts`:
- Around line 25-55: Consolidate duplicate test seeding helpers by extracting
the shared logic from seedSyncLog, seedTagSyncLog, and seedNotebookSyncLog into
a generic helper in helpers.ts: create a single function (e.g.,
seedSyncLogGeneric or seedSyncLog) that accepts the table name (or a typed
enum), common args (env, userId, entries array) and maps each entry to the
appropriate INSERT with table-specific columns if needed; update tests to import
and call the new generic helper and remove the old per-entity helpers to avoid
duplication and centralize seeding logic.
In `@packages/api/tests/sync-tags.test.ts`:
- Around line 25-55: Extract the duplicate seeding logic by moving
seedTagSyncLog into a shared test helper (e.g., export from helpers.ts) and
update tests to import it instead of defining it inline; ensure the helper
signature and types match the existing usage (accept Env, userId, entries with
tagId/version/operation/data?/deviceId), reuse or consolidate with the existing
seedSyncLog/seedNotebookSyncLog patterns (or generalize to a single seedSyncLog
helper) and update all test imports accordingly so schema changes need only be
applied in one place.
In `@packages/api/vitest.config.ts`:
- Around line 3-6: The test config currently calls defineConfig({ test: {
globals: true } }) but doesn't set an explicit environment or suggest TypeScript
types; update the test block in defineConfig to include environment: 'node'
(i.e., test.environment = 'node') and, if you prefer global test functions, add
guidance to the repo tsconfig to include types: ["vitest/globals"] so TypeScript
recognizes globals; alternatively note that tests may explicitly import from
"vitest" to avoid adding the tsconfig types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bdaa4f5-9d8c-4cbb-a777-7776ff480adf
📒 Files selected for processing (10)
packages/api/package.jsonpackages/api/tests/encryption-roundtrip.test.tspackages/api/tests/helpers.tspackages/api/tests/smoke.test.tspackages/api/tests/sync-notebooks.test.tspackages/api/tests/sync-pull.test.tspackages/api/tests/sync-push.test.tspackages/api/tests/sync-status.test.tspackages/api/tests/sync-tags.test.tspackages/api/vitest.config.ts
Comment on lines
+15
to
+23
| function decrypt(key: Buffer, ciphertext: string): string { | ||
| const [ivB64, dataB64, tagB64] = ciphertext.split(':'); | ||
| const iv = Buffer.from(ivB64, 'base64'); | ||
| const data = Buffer.from(dataB64, 'base64'); | ||
| const authTag = Buffer.from(tagB64, 'base64'); | ||
| const decipher = createDecipheriv(ALGORITHM, key, iv); | ||
| decipher.setAuthTag(authTag); | ||
| return Buffer.concat([decipher.update(data), decipher.final()]).toString('utf-8'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing input validation in decrypt() could cause unclear errors.
The function assumes ciphertext.split(':') always produces exactly 3 parts. Malformed input would cause confusing errors (undefined buffer operations) instead of a clear validation error.
Since this is test-only code used in controlled scenarios, this is a minor concern, but worth noting for robustness.
♻️ Add input validation
function decrypt(key: Buffer, ciphertext: string): string {
const [ivB64, dataB64, tagB64] = ciphertext.split(':');
+ if (!ivB64 || !dataB64 || !tagB64) {
+ throw new Error('Invalid ciphertext format: expected IV:data:authTag');
+ }
const iv = Buffer.from(ivB64, 'base64');
const data = Buffer.from(dataB64, 'base64');
const authTag = Buffer.from(tagB64, 'base64');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function decrypt(key: Buffer, ciphertext: string): string { | |
| const [ivB64, dataB64, tagB64] = ciphertext.split(':'); | |
| const iv = Buffer.from(ivB64, 'base64'); | |
| const data = Buffer.from(dataB64, 'base64'); | |
| const authTag = Buffer.from(tagB64, 'base64'); | |
| const decipher = createDecipheriv(ALGORITHM, key, iv); | |
| decipher.setAuthTag(authTag); | |
| return Buffer.concat([decipher.update(data), decipher.final()]).toString('utf-8'); | |
| } | |
| function decrypt(key: Buffer, ciphertext: string): string { | |
| const [ivB64, dataB64, tagB64] = ciphertext.split(':'); | |
| if (!ivB64 || !dataB64 || !tagB64) { | |
| throw new Error('Invalid ciphertext format: expected IV:data:authTag'); | |
| } | |
| const iv = Buffer.from(ivB64, 'base64'); | |
| const data = Buffer.from(dataB64, 'base64'); | |
| const authTag = Buffer.from(tagB64, 'base64'); | |
| const decipher = createDecipheriv(ALGORITHM, key, iv); | |
| decipher.setAuthTag(authTag); | |
| return Buffer.concat([decipher.update(data), decipher.final()]).toString('utf-8'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/encryption-roundtrip.test.ts` around lines 15 - 23, The
decrypt function assumes ciphertext.split(':') returns three parts and proceeds
to decode buffers, which leads to unclear errors for malformed input; update
decrypt to validate that ciphertext is a non-empty string, that splitting on ':'
yields exactly three segments (ivB64, dataB64, tagB64), and that each segment
decodes to a Buffer (and iv/authTag have expected lengths for ALGORITHM) before
calling createDecipheriv, throwing a clear, descriptive error if any check
fails; reference the decrypt function and variables ivB64/dataB64/tagB64, iv,
data, authTag, and createDecipheriv when making these checks.
Comment on lines
+32
to
+47
| const SCHEMA_SQL = ` | ||
| CREATE TABLE IF NOT EXISTS users (id TEXT PRIMARY KEY, email TEXT NOT NULL UNIQUE, created_at TEXT NOT NULL, updated_at TEXT NOT NULL); | ||
| CREATE TABLE IF NOT EXISTS subscriptions (id TEXT PRIMARY KEY, user_id TEXT NOT NULL UNIQUE REFERENCES users(id) ON DELETE CASCADE, stripe_customer_id TEXT, stripe_subscription_id TEXT, status TEXT NOT NULL DEFAULT 'inactive', plan TEXT NOT NULL DEFAULT 'free', trial_ends_at TEXT, current_period_end TEXT, canceled_at TEXT, cancel_at_period_end INTEGER NOT NULL DEFAULT 0, created_at TEXT NOT NULL, updated_at TEXT NOT NULL); | ||
| CREATE TABLE IF NOT EXISTS sync_log (id TEXT PRIMARY KEY, user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, note_id TEXT NOT NULL, version INTEGER NOT NULL, operation TEXT NOT NULL, encrypted_data TEXT, device_id TEXT NOT NULL, created_at TEXT NOT NULL); | ||
| CREATE INDEX IF NOT EXISTS idx_sync_log_user_version ON sync_log(user_id, version); | ||
| CREATE INDEX IF NOT EXISTS idx_sync_log_user_note ON sync_log(user_id, note_id); | ||
| CREATE TABLE IF NOT EXISTS sync_cursors (id TEXT PRIMARY KEY, user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, device_id TEXT NOT NULL, last_synced_version INTEGER NOT NULL DEFAULT 0, updated_at TEXT NOT NULL); | ||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_sync_cursors_unique ON sync_cursors(user_id, device_id); | ||
| CREATE TABLE IF NOT EXISTS tag_sync_log (id TEXT PRIMARY KEY, user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, tag_id TEXT NOT NULL, version INTEGER NOT NULL, operation TEXT NOT NULL, data TEXT, device_id TEXT NOT NULL, created_at TEXT NOT NULL); | ||
| CREATE INDEX IF NOT EXISTS idx_tag_sync_log_user_version ON tag_sync_log(user_id, version); | ||
| CREATE TABLE IF NOT EXISTS notebook_sync_log (id TEXT PRIMARY KEY, user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, notebook_id TEXT NOT NULL, version INTEGER NOT NULL, operation TEXT NOT NULL, data TEXT, device_id TEXT NOT NULL, created_at TEXT NOT NULL); | ||
| CREATE INDEX IF NOT EXISTS idx_nb_sync_log_user_version ON notebook_sync_log(user_id, version); | ||
| CREATE TABLE IF NOT EXISTS devices (id TEXT PRIMARY KEY, user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, device_id TEXT NOT NULL, name TEXT, platform TEXT, last_seen_at TEXT NOT NULL, created_at TEXT NOT NULL); | ||
| CREATE UNIQUE INDEX IF NOT EXISTS idx_devices_unique ON devices(user_id, device_id); | ||
| CREATE TABLE IF NOT EXISTS magic_links (id TEXT PRIMARY KEY, user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, token TEXT NOT NULL UNIQUE, expires_at TEXT NOT NULL, used_at TEXT, created_at TEXT NOT NULL); | ||
| `.trim(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare test schema tables with production schema to check for drift
echo "=== Production schema tables ==="
rg -o "sqliteTable\(['\"](\w+)['\"]" --replace '$1' packages/api/src/db/schema.ts | sort | uniq
echo ""
echo "=== Test schema tables ==="
grep -oP "CREATE TABLE IF NOT EXISTS \K\w+" packages/api/tests/helpers.ts | sort | uniqRepository: tomymaritano/readide
Length of output: 248
🏁 Script executed:
# Check the actual production schema file
cat packages/api/src/db/schema.tsRepository: tomymaritano/readide
Length of output: 10201
🏁 Script executed:
# Search for schema definitions elsewhere
find packages/api/src -type f -name "*.ts" | xargs grep -l "sqliteTable\|defineTable" | head -20Repository: tomymaritano/readide
Length of output: 93
Test schema is manually maintained and could drift from production.
The test schema in helpers.ts (lines 32-47) is a curated subset of the production schema that covers only the sync and subscription tables. While currently aligned with production definitions in packages/api/src/db/schema.ts, it is manually duplicated in SQL form rather than generated from the Drizzle schema definitions.
Per the codebase guidelines, schema is managed via drizzle-kit push, not file migrations. Maintain test schema alignment with production by either:
- Generating the schema SQL programmatically from Drizzle table definitions at test runtime
- Adding a validation check in CI that compares test schema SQL against production schema definitions
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/helpers.ts` around lines 32 - 47, The SCHEMA_SQL constant
in packages/api/tests/helpers.ts is a manually maintained SQL string that can
drift from the Drizzle-managed production schema; replace it by programmatically
deriving the test schema from the Drizzle definitions or add a CI-time
validation that fails if they differ. Concretely, update helpers.ts to remove
the hardcoded SCHEMA_SQL and instead import the Drizzle table/schema definitions
(from packages/api/src/db/schema.ts or the module that exports your Drizzle
schema) and call the same schema-generation utility used in production (or use
drizzle-kit/Drizzle's schema-generation API) to produce the SQL used to
initialize the test DB at runtime, or add a new test/CI step that reads
SCHEMA_SQL and compares it to the generated schema SQL and fails if mismatched;
target the SCHEMA_SQL symbol and the test DB initialization code in helpers.ts
for this change.
Comment on lines
+54
to
+57
| const client = createClient({ | ||
| url: env.TURSO_DATABASE_URL, | ||
| authToken: env.TURSO_AUTH_TOKEN || undefined, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty authToken string passed instead of undefined.
When env.TURSO_AUTH_TOKEN is an empty string '', the expression env.TURSO_AUTH_TOKEN || undefined evaluates to undefined as expected. However, this pattern is repeated in multiple places. Consider centralizing the client creation logic.
♻️ Extract a helper to reduce duplication
function createTestClient(env: Env) {
return createClient({
url: env.TURSO_DATABASE_URL,
authToken: env.TURSO_AUTH_TOKEN || undefined,
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/helpers.ts` around lines 54 - 57, The test setup passes
authToken using the pattern env.TURSO_AUTH_TOKEN || undefined which can silently
allow empty strings and is duplicated; extract a single helper (e.g.,
createTestClient) that takes the Env object and returns createClient({ url:
env.TURSO_DATABASE_URL, authToken: env.TURSO_AUTH_TOKEN ? env.TURSO_AUTH_TOKEN :
undefined }) and replace all inline createClient calls with createTestClient to
centralize logic and avoid passing empty-string tokens.
Comment on lines
+68
to
+75
| export function cleanupTestDb(env: Env): void { | ||
| const filePath = env.TURSO_DATABASE_URL.replace('file:', ''); | ||
| try { | ||
| unlinkSync(filePath); | ||
| } catch { | ||
| // File may not exist if tests failed early — that's fine | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cleanup silently swallows all errors.
The catch block ignores all errors, not just ENOENT. If the file exists but cannot be deleted (permissions, file lock), the test suite won't be notified.
♻️ Only ignore file-not-found errors
export function cleanupTestDb(env: Env): void {
const filePath = env.TURSO_DATABASE_URL.replace('file:', '');
try {
unlinkSync(filePath);
- } catch {
- // File may not exist if tests failed early — that's fine
+ } catch (err: unknown) {
+ // Ignore ENOENT (file not found), re-throw other errors
+ if ((err as NodeJS.ErrnoException).code !== 'ENOENT') {
+ throw err;
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function cleanupTestDb(env: Env): void { | |
| const filePath = env.TURSO_DATABASE_URL.replace('file:', ''); | |
| try { | |
| unlinkSync(filePath); | |
| } catch { | |
| // File may not exist if tests failed early — that's fine | |
| } | |
| } | |
| export function cleanupTestDb(env: Env): void { | |
| const filePath = env.TURSO_DATABASE_URL.replace('file:', ''); | |
| try { | |
| unlinkSync(filePath); | |
| } catch (err: unknown) { | |
| // Ignore ENOENT (file not found), re-throw other errors | |
| if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| throw err; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/helpers.ts` around lines 68 - 75, The cleanupTestDb
function silently swallows all errors when unlinkSync(filePath) fails; update
cleanupTestDb to only ignore file-not-found errors (ENOENT) by catching the
error thrown by unlinkSync, checking err.code === 'ENOENT' and returning
silently for that case, but rethrowing or logging any other error (e.g.,
permission or lock errors) so test failures surface; reference the cleanupTestDb
function, unlinkSync call and env.TURSO_DATABASE_URL/filePath when making this
change.
Comment on lines
+25
to
+55
| async function seedNotebookSyncLog( | ||
| env: Env, | ||
| userId: string, | ||
| entries: Array<{ | ||
| notebookId: string; | ||
| version: number; | ||
| operation: string; | ||
| data?: string | null; | ||
| deviceId: string; | ||
| }> | ||
| ) { | ||
| const client = createClient({ | ||
| url: env.TURSO_DATABASE_URL, | ||
| authToken: env.TURSO_AUTH_TOKEN || undefined, | ||
| }); | ||
| for (const entry of entries) { | ||
| await client.execute({ | ||
| sql: 'INSERT INTO notebook_sync_log (id, user_id, notebook_id, version, operation, data, device_id, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', | ||
| args: [ | ||
| randomUUID(), | ||
| userId, | ||
| entry.notebookId, | ||
| entry.version, | ||
| entry.operation, | ||
| entry.data ?? null, | ||
| entry.deviceId, | ||
| new Date().toISOString(), | ||
| ], | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Similar helper duplication as noted in sync-tags.test.ts.
The seedNotebookSyncLog helper follows the same pattern as other seed helpers. Consider consolidating in helpers.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/sync-notebooks.test.ts` around lines 25 - 55, The
seedNotebookSyncLog helper is duplicated; move its implementation into the
shared helpers module (e.g., add seedNotebookSyncLog to helpers.ts) and have
tests import it instead of redefining it. Keep the same logic and references
(seedNotebookSyncLog, createClient, env.TURSO_DATABASE_URL,
env.TURSO_AUTH_TOKEN) in the shared helper, remove the duplicate implementation
from packages/api/tests/sync-notebooks.test.ts (and any other tests like
sync-tags.test.ts), and update imports in those test files to import
seedNotebookSyncLog from helpers.ts.
Comment on lines
+83
to
+88
| it('returns 401 without auth', async () => { | ||
| const res = await app.request('/sync?cursor=0', {}, env); | ||
| // Auth middleware throws HTTPException(401); the global onError handler | ||
| // re-wraps it as 500. Either status confirms the request is rejected. | ||
| expect([401, 500]).toContain(res.status); | ||
| }); |
There was a problem hiding this comment.
Do not treat 500 as a passing auth result.
This test currently passes even when unauthenticated access blows up as an internal server error, so it will hide the auth/onError regression instead of catching it. The contract here should be a strict 401.
Suggested assertion
it('returns 401 without auth', async () => {
const res = await app.request('/sync?cursor=0', {}, env);
- // Auth middleware throws HTTPException(401); the global onError handler
- // re-wraps it as 500. Either status confirms the request is rejected.
- expect([401, 500]).toContain(res.status);
+ expect(res.status).toBe(401);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('returns 401 without auth', async () => { | |
| const res = await app.request('/sync?cursor=0', {}, env); | |
| // Auth middleware throws HTTPException(401); the global onError handler | |
| // re-wraps it as 500. Either status confirms the request is rejected. | |
| expect([401, 500]).toContain(res.status); | |
| }); | |
| it('returns 401 without auth', async () => { | |
| const res = await app.request('/sync?cursor=0', {}, env); | |
| expect(res.status).toBe(401); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/sync-pull.test.ts` around lines 83 - 88, The test "returns
401 without auth" currently allows 500 as passing which masks auth/onError
regressions; change the assertion to require a strict 401 by replacing the
loosened expect([401, 500]).toContain(res.status) with a strict equality assert
(e.g., expect(res.status).toBe(401)) for the response from
app.request('/sync?cursor=0', {}, env) so unauthenticated requests fail only
with 401; keep the rest of the test and the unauthenticated env as-is.
Comment on lines
+84
to
+102
| it('applies a single change and returns version', async () => { | ||
| const noteId = randomUUID(); | ||
|
|
||
| const res = await pushChanges( | ||
| proToken, | ||
| [{ noteId, operation: 'create', encryptedData: 'encrypted-content' }], | ||
| deviceA | ||
| ); | ||
| expect(res.status).toBe(200); | ||
|
|
||
| const body = await res.json(); | ||
| expect(body.results).toHaveLength(1); | ||
| expect(body.results[0]).toEqual({ | ||
| noteId, | ||
| version: expect.any(Number), | ||
| status: 'applied', | ||
| }); | ||
| expect(body.results[0].version).toBeGreaterThanOrEqual(1); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a fresh user here so the version assertion stays strict.
Reusing the shared pro user forces this test to accept any version >= 1, which weakens coverage and makes the assertion depend on earlier pushes in the suite. This case should mirror the later isolated tests and assert the first assigned version exactly.
Suggested tightening
it('applies a single change and returns version', async () => {
+ const userId = randomUUID();
+ const email = `pro-single-${userId}@test.com`;
+ await seedProUser(env, userId, email);
+ const token = await createAccessToken(userId, email, deviceA);
const noteId = randomUUID();
const res = await pushChanges(
- proToken,
+ token,
[{ noteId, operation: 'create', encryptedData: 'encrypted-content' }],
deviceA
);
expect(res.status).toBe(200);
@@
expect(body.results[0]).toEqual({
noteId,
version: expect.any(Number),
status: 'applied',
});
- expect(body.results[0].version).toBeGreaterThanOrEqual(1);
+ expect(body.results[0].version).toBe(1);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/sync-push.test.ts` around lines 84 - 102, This test reuses
the shared proToken which allows previous pushes to inflate the assigned
version; create and use a fresh test user token instead of proToken (e.g.,
obtain a new token via your test helper) and call pushChanges with that fresh
token and deviceA so the first created note gets version === 1, then change the
assertion to expect(body.results[0].version).toBe(1) while still keeping the
other expectations; locate the push invocation (pushChanges) and replace
proToken with the new fresh token variable.
Comment on lines
+25
to
+55
| async function seedSyncLog( | ||
| env: Env, | ||
| userId: string, | ||
| entries: Array<{ | ||
| noteId: string; | ||
| version: number; | ||
| operation: string; | ||
| encryptedData?: string | null; | ||
| deviceId: string; | ||
| }> | ||
| ) { | ||
| const client = createClient({ | ||
| url: env.TURSO_DATABASE_URL, | ||
| authToken: env.TURSO_AUTH_TOKEN || undefined, | ||
| }); | ||
| for (const entry of entries) { | ||
| await client.execute({ | ||
| sql: 'INSERT INTO sync_log (id, user_id, note_id, version, operation, encrypted_data, device_id, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', | ||
| args: [ | ||
| randomUUID(), | ||
| userId, | ||
| entry.noteId, | ||
| entry.version, | ||
| entry.operation, | ||
| entry.encryptedData ?? null, | ||
| entry.deviceId, | ||
| new Date().toISOString(), | ||
| ], | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same helper pattern as other test files.
The seedSyncLog helper could be consolidated with seedTagSyncLog and seedNotebookSyncLog into a generic seeding utility in helpers.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/sync-status.test.ts` around lines 25 - 55, Consolidate
duplicate test seeding helpers by extracting the shared logic from seedSyncLog,
seedTagSyncLog, and seedNotebookSyncLog into a generic helper in helpers.ts:
create a single function (e.g., seedSyncLogGeneric or seedSyncLog) that accepts
the table name (or a typed enum), common args (env, userId, entries array) and
maps each entry to the appropriate INSERT with table-specific columns if needed;
update tests to import and call the new generic helper and remove the old
per-entity helpers to avoid duplication and centralize seeding logic.
Comment on lines
+25
to
+55
| async function seedTagSyncLog( | ||
| env: Env, | ||
| userId: string, | ||
| entries: Array<{ | ||
| tagId: string; | ||
| version: number; | ||
| operation: string; | ||
| data?: string | null; | ||
| deviceId: string; | ||
| }> | ||
| ) { | ||
| const client = createClient({ | ||
| url: env.TURSO_DATABASE_URL, | ||
| authToken: env.TURSO_AUTH_TOKEN || undefined, | ||
| }); | ||
| for (const entry of entries) { | ||
| await client.execute({ | ||
| sql: 'INSERT INTO tag_sync_log (id, user_id, tag_id, version, operation, data, device_id, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', | ||
| args: [ | ||
| randomUUID(), | ||
| userId, | ||
| entry.tagId, | ||
| entry.version, | ||
| entry.operation, | ||
| entry.data ?? null, | ||
| entry.deviceId, | ||
| new Date().toISOString(), | ||
| ], | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting seedTagSyncLog to shared helpers.
This helper duplicates the pattern used in sync-status.test.ts (seedSyncLog) and sync-notebooks.test.ts (seedNotebookSyncLog). Extracting these to helpers.ts would reduce duplication and make it easier to keep them in sync with schema changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/tests/sync-tags.test.ts` around lines 25 - 55, Extract the
duplicate seeding logic by moving seedTagSyncLog into a shared test helper
(e.g., export from helpers.ts) and update tests to import it instead of defining
it inline; ensure the helper signature and types match the existing usage
(accept Env, userId, entries with tagId/version/operation/data?/deviceId), reuse
or consolidate with the existing seedSyncLog/seedNotebookSyncLog patterns (or
generalize to a single seedSyncLog helper) and update all test imports
accordingly so schema changes need only be applied in one place.
Comment on lines
+3
to
+6
| export default defineConfig({ | ||
| test: { | ||
| globals: true, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding environment and TypeScript types configuration.
With globals: true, you may want to:
- Add
types: ["vitest/globals"]to yourtsconfig.jsonto get proper TypeScript support for global test functions. - Consider specifying
environment: 'node'explicitly since you're testing a server-side API.
Note: The test files in this PR explicitly import from vitest, which is also valid and avoids needing the types configuration.
♻️ Optional: Explicit environment configuration
export default defineConfig({
test: {
globals: true,
+ environment: 'node',
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export default defineConfig({ | |
| test: { | |
| globals: true, | |
| }, | |
| export default defineConfig({ | |
| test: { | |
| globals: true, | |
| environment: 'node', | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/vitest.config.ts` around lines 3 - 6, The test config currently
calls defineConfig({ test: { globals: true } }) but doesn't set an explicit
environment or suggest TypeScript types; update the test block in defineConfig
to include environment: 'node' (i.e., test.environment = 'node') and, if you
prefer global test functions, add guidance to the repo tsconfig to include
types: ["vitest/globals"] so TypeScript recognizes globals; alternatively note
that tests may explicitly import from "vitest" to avoid adding the tsconfig
types.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # packages/storage-sqlite/src/repositories/SQLiteNoteRepository.ts
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
Test Coverage
Test plan
pnpm --filter @readied/api test)🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores