Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
More reviews will be available in 9 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR hardens desktop services through validation improvements and database durability. Config coercion switches to Zod schemas; SQL queries deduplicate and refactor clause assembly; global state and database writes implement atomic temp-file + rename + fsync patterns with CRR-aware migrations and WAL checkpointing. ChangesDesktop Services Enhancement
State and Database Atomicity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Owner
Author
|
@copilot review but do not make fixes |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 31, 2026 23:59
arul28
force-pushed
the
ade-71-state-sync-config-persistence-kvdb-hardening-crash-safe-writes-migration-versioning-and-god-file-dedup-decomposition
branch
from
cbd8db7 to
184c2c8
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 1, 2026 00:24
arul28
force-pushed
the
ade-71-state-sync-config-persistence-kvdb-hardening-crash-safe-writes-migration-versioning-and-god-file-dedup-decomposition
branch
from
184c2c8 to
bf1290e
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 1, 2026 00:37
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/desktop/src/main/services/config/projectConfigService.ts (1)
95-104: 💤 Low valueSilent error handling may hide config validation issues.
All schemas use the
.optional().catch(undefined)pattern, which silently returnsundefinedfor any parsing errors. While this permissive approach may be intentional for config coercion (allowing partial config loading), it means malformed config values are discarded without warning.Consider whether certain schema violations (e.g., type mismatches for critical fields) should be logged at debug level to aid troubleshooting, even if the value is ultimately coerced to
undefined.Also applies to: 126-172
🤖 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 `@apps/desktop/src/main/services/config/projectConfigService.ts` around lines 95 - 104, The current schemas (OPTIONAL_STRING_SCHEMA, OPTIONAL_NUMBER_SCHEMA, OPTIONAL_BOOL_SCHEMA, STRING_ARRAY_SCHEMA and other similar optional schemas) swallow all parse errors via .optional().catch(undefined), hiding malformed config values; update each .catch to capture the thrown error and emit a debug-level log (e.g., processLogger.debug or configured logger) with the schema name and error details before returning undefined so malformed values are visible during troubleshooting while preserving coercion behavior.
🤖 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 `@apps/desktop/src/main/services/config/projectConfigService.ts`:
- Around line 105-114: STRING_MAP_SCHEMA currently uses z.record(z.string(),
z.unknown()) with a transform and a final .catch(() => undefined), which means
arrays will be rejected by z.record but then silently turned into undefined (and
numeric array indices might be misinterpreted); either make the schema
explicitly accept arrays and convert them, or remove the silent catch so errors
surface. Fix by replacing STRING_MAP_SCHEMA with a union like
z.union([z.record(z.string(), z.unknown()), z.array(z.unknown())]) and update
the transform in STRING_MAP_SCHEMA to handle both shapes (for arrays, iterate
values and map numeric/indexed or string entries into the output
Record<string,string>), and remove or tighten the .catch(() => undefined) so
malformed configs produce validation errors instead of being swallowed.
In `@apps/desktop/src/main/services/state/globalState.test.ts`:
- Around line 69-84: The test currently doesn't verify the atomic-temp rename;
update the "writeGlobalState" test to assert the actual rename behavior by
spying/mocking fs.renameSync (or fs.promises.rename) when calling
writeGlobalState: create an initial file at filePath, call
writeGlobalState(filePath, state), and verify fs.renameSync was invoked with a
source path that endsWith(".tmp") and whose directory equals
path.dirname(filePath) and a destination equal to filePath; alternatively inject
a failure before rename and assert the original file content remains unchanged.
Reference writeGlobalState, readGlobalState and fs.renameSync (or
fs.promises.rename) when locating where to add the spy/assert.
In `@apps/desktop/src/main/services/state/kvDb.test.ts`:
- Around line 220-229: The test currently only verifies reads from the same open
connection so it doesn't prove a checkpoint occurred; update the "checkpoints
pending writes when flushed" test to assert a WAL-side observable effect after
calling db.flushNow(): e.g., call db.flushNow(), then either close the DB
(db.close()), and assert the WAL file for dbPath (dbPath + "-wal") is absent or
truncated on disk, or run a SQLite checkpoint query (PRAGMA wal_checkpoint or
PRAGMA wal_checkpoint(TRUNCATE)) via the DB handle and assert the returned
checkpoint stats indicate a completed checkpoint; use openKvDb, db.flushNow,
db.close and dbPath to locate the WAL and ensure the test fails if no checkpoint
happened.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 1158-1171: If an ALTER is targeted at a CRR-managed table
(alterTable is set and rawHasTable(db, `${alterTable}__crsql_clock`) is true)
but isCrsqliteLoaded() is false, do not fall back to raw runStatement; instead
abort with a clear error so CRR metadata won't become inconsistent. Update the
branch around isCrsqliteLoaded()/rawHasTable check in kvDb.ts to detect this
case and throw a descriptive error (or return a failure) referencing alterTable
and advising to load cr-sqlite, rather than calling runStatement; keep existing
behavior when isCrsqliteLoaded() is true (use getRow("select
crsql_begin_alter(...)"), runStatement, then getRow("select
crsql_commit_alter(...)")).
- Around line 3343-3345: flushNow currently calls getRow(db, "pragma
wal_checkpoint(TRUNCATE)") and discards the result; change it to capture the
returned row from getRow in flushNow, inspect the returned "busy" field (or
equivalent column) and if busy !== 0 treat the checkpoint as failed by logging
an error via the existing logger and/or throwing/returning an error so callers
(including shutdown paths) can observe failure; ensure the change references the
flushNow function and the getRow(db, "pragma wal_checkpoint(TRUNCATE)") call and
preserves existing error handling semantics when the query itself errors.
---
Nitpick comments:
In `@apps/desktop/src/main/services/config/projectConfigService.ts`:
- Around line 95-104: The current schemas (OPTIONAL_STRING_SCHEMA,
OPTIONAL_NUMBER_SCHEMA, OPTIONAL_BOOL_SCHEMA, STRING_ARRAY_SCHEMA and other
similar optional schemas) swallow all parse errors via
.optional().catch(undefined), hiding malformed config values; update each .catch
to capture the thrown error and emit a debug-level log (e.g.,
processLogger.debug or configured logger) with the schema name and error details
before returning undefined so malformed values are visible during
troubleshooting while preserving coercion behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12826bfb-7786-4aec-aad3-22c476508edb
📒 Files selected for processing (7)
apps/desktop/src/main/services/config/projectConfigService.tsapps/desktop/src/main/services/history/operationService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/state/globalState.test.tsapps/desktop/src/main/services/state/globalState.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.ts
Comment on lines
+105
to
+114
| const STRING_MAP_SCHEMA = z.record(z.string(), z.unknown()) | ||
| .transform((value) => { | ||
| const out: Record<string, string> = {}; | ||
| for (const [key, entry] of Object.entries(value)) { | ||
| if (typeof entry === "string") out[key] = entry; | ||
| } | ||
| return out; | ||
| }) | ||
| .optional() | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Test whether z.record accepts arrays in Zod 4
cat > /tmp/test_zod_record.mjs << 'EOF'
import { z } from 'zod';
const STRING_MAP_SCHEMA = z.record(z.string(), z.unknown())
.transform((value) => {
const out = {};
for (const [key, entry] of Object.entries(value)) {
if (typeof entry === "string") out[key] = entry;
}
return out;
})
.optional()
.catch(() => undefined);
// Test cases
const testCases = [
{ input: { a: "x", b: "y" }, label: "object with strings" },
{ input: { a: "x", b: 123 }, label: "object with mixed types" },
{ input: ["x", "y", "z"], label: "array of strings" },
{ input: [1, 2, 3], label: "array of numbers" },
{ input: "not-an-object", label: "string" },
{ input: undefined, label: "undefined" },
];
console.log("Testing STRING_MAP_SCHEMA with Zod 4:\n");
for (const { input, label } of testCases) {
try {
const result = STRING_MAP_SCHEMA.parse(input);
console.log(`✓ ${label}:`, JSON.stringify(result));
} catch (error) {
console.log(`✗ ${label}: ERROR -`, error.message);
}
}
EOF
cd /tmp && node test_zod_record.mjs
rm test_zod_record.mjsRepository: arul28/ADE
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Check if zod is available from the repo and print its resolved path/version
node -e "try{console.log('zod resolve:',require.resolve('zod'))}catch(e){console.error('require.resolve failed:',e.message);process.exit(0)}"
node -e "try{const {z}=require('zod'); console.log('zod loaded (commonjs)');}catch(e){console.error('require(zod) failed:',e.message)}"
ls -la node_modules/zod 2>/dev/null || true
# Print zod version from package.json/lock if present
node -e "const fs=require('fs'); const p=['package.json','package-lock.json','npm-shrinkwrap.json','yarn.lock','pnpm-lock.yaml']; for(const f of p){ if(fs.existsSync(f)){ console.log('found',f); break}}"
rg -n "\"zod\"|zod@" package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null || trueRepository: arul28/ADE
Length of output: 272
🌐 Web query:
Zod 4 z.record array input accepts or rejects Array.isArray behavior record schema
💡 Result:
In Zod, including in version 4, z.record() is designed specifically to validate objects (the Record<K, V> type in TypeScript) and does not inherently treat arrays as valid input [1][2]. If you pass an array to a z.record() schema, it will be rejected during validation [1]. While JavaScript arrays are technically objects, z.record() explicitly expects an object structure with keys and values that match the provided schemas [1]. If you need to validate a structure that might be an array, you should use z.array() [1][3][4]. If you need to support both types of input for the same field, you can use z.union([z.array(...), z.record(...)]) or z.discriminatedUnion(), depending on your requirements. For context, z.record() is used to validate objects where you define the schema for the keys and the schema for the values (e.g., z.record(z.string(), z.number())) [1]. It is not a generic collection validator for both arrays and objects [1][2].
Citations:
- 1: https://zod.dev/api
- 2: https://github.com/colinhacks/zod/blob/cc8ad1981ba580d1250520fde8878073d4b7d40a/README.md
- 3: https://stackoverflow.com/questions/74967542/zod-validation-for-an-array-of-objects
- 4: https://dev.to/akaranjkar08/zod-validation-type-safe-apis-forms-in-typescript-complete-guide-3gjj
Fix/clarify STRING_MAP_SCHEMA array-handling vs PR summary
z.record(z.string(), z.unknown())is designed for object inputs; passing an array should fail validation (so with.catch(() => undefined)the result should beundefined, not partially accepted numeric keys).- Either update the PR summary/tests to reflect arrays are rejected, or change the schema to explicitly accept arrays (e.g.,
z.union([z.array(...), z.record(...)])plus conversion), if that’s the intended behavior. - Reconsider
.catch(() => undefined)here if you want malformed config fields to be diagnosable rather than silently ignored.
🤖 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 `@apps/desktop/src/main/services/config/projectConfigService.ts` around lines
105 - 114, STRING_MAP_SCHEMA currently uses z.record(z.string(), z.unknown())
with a transform and a final .catch(() => undefined), which means arrays will be
rejected by z.record but then silently turned into undefined (and numeric array
indices might be misinterpreted); either make the schema explicitly accept
arrays and convert them, or remove the silent catch so errors surface. Fix by
replacing STRING_MAP_SCHEMA with a union like z.union([z.record(z.string(),
z.unknown()), z.array(z.unknown())]) and update the transform in
STRING_MAP_SCHEMA to handle both shapes (for arrays, iterate values and map
numeric/indexed or string entries into the output Record<string,string>), and
remove or tighten the .catch(() => undefined) so malformed configs produce
validation errors instead of being swallowed.
Comment on lines
+69
to
+84
| describe("writeGlobalState", () => { | ||
| it("persists state through an atomic temp-file rename", () => { | ||
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-global-state-")); | ||
| const filePath = path.join(dir, "global-state.json"); | ||
| const state: GlobalState = { | ||
| lastProjectRoot: "/repo/ade", | ||
| recentProjects: [ | ||
| { rootPath: "/repo/ade", displayName: "ADE", lastOpenedAt: "2026-05-31T00:00:00.000Z" }, | ||
| ], | ||
| }; | ||
|
|
||
| writeGlobalState(filePath, state); | ||
|
|
||
| expect(readGlobalState(filePath)).toEqual(state); | ||
| expect(fs.readdirSync(dir).filter((entry) => entry.endsWith(".tmp"))).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
This test doesn't actually prove the temp-file rename path.
A direct overwrite of filePath would still pass these assertions, so the atomic-write contract can regress unnoticed. Please assert the renameSync call from a .tmp path in the same directory, or inject a failure before rename and verify the original file stays intact.
🧪 One way to lock down the rename behavior
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
@@
it("persists state through an atomic temp-file rename", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-global-state-"));
const filePath = path.join(dir, "global-state.json");
+ const renameSpy = vi.spyOn(fs, "renameSync");
const state: GlobalState = {
@@
writeGlobalState(filePath, state);
expect(readGlobalState(filePath)).toEqual(state);
+ expect(renameSpy).toHaveBeenCalledTimes(1);
+ const [from, to] = renameSpy.mock.calls[0]!;
+ expect(path.dirname(from)).toBe(dir);
+ expect(from.endsWith(".tmp")).toBe(true);
+ expect(to).toBe(filePath);
expect(fs.readdirSync(dir).filter((entry) => entry.endsWith(".tmp"))).toEqual([]);
+ renameSpy.mockRestore();
});📝 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
| describe("writeGlobalState", () => { | |
| it("persists state through an atomic temp-file rename", () => { | |
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-global-state-")); | |
| const filePath = path.join(dir, "global-state.json"); | |
| const state: GlobalState = { | |
| lastProjectRoot: "/repo/ade", | |
| recentProjects: [ | |
| { rootPath: "/repo/ade", displayName: "ADE", lastOpenedAt: "2026-05-31T00:00:00.000Z" }, | |
| ], | |
| }; | |
| writeGlobalState(filePath, state); | |
| expect(readGlobalState(filePath)).toEqual(state); | |
| expect(fs.readdirSync(dir).filter((entry) => entry.endsWith(".tmp"))).toEqual([]); | |
| }); | |
| describe("writeGlobalState", () => { | |
| it("persists state through an atomic temp-file rename", () => { | |
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-global-state-")); | |
| const filePath = path.join(dir, "global-state.json"); | |
| const renameSpy = vi.spyOn(fs, "renameSync"); | |
| const state: GlobalState = { | |
| lastProjectRoot: "/repo/ade", | |
| recentProjects: [ | |
| { rootPath: "/repo/ade", displayName: "ADE", lastOpenedAt: "2026-05-31T00:00:00.000Z" }, | |
| ], | |
| }; | |
| writeGlobalState(filePath, state); | |
| expect(readGlobalState(filePath)).toEqual(state); | |
| expect(renameSpy).toHaveBeenCalledTimes(1); | |
| const [from, to] = renameSpy.mock.calls[0]!; | |
| expect(path.dirname(from)).toBe(dir); | |
| expect(from.endsWith(".tmp")).toBe(true); | |
| expect(to).toBe(filePath); | |
| expect(fs.readdirSync(dir).filter((entry) => entry.endsWith(".tmp"))).toEqual([]); | |
| renameSpy.mockRestore(); | |
| }); |
🤖 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 `@apps/desktop/src/main/services/state/globalState.test.ts` around lines 69 -
84, The test currently doesn't verify the atomic-temp rename; update the
"writeGlobalState" test to assert the actual rename behavior by spying/mocking
fs.renameSync (or fs.promises.rename) when calling writeGlobalState: create an
initial file at filePath, call writeGlobalState(filePath, state), and verify
fs.renameSync was invoked with a source path that endsWith(".tmp") and whose
directory equals path.dirname(filePath) and a destination equal to filePath;
alternatively inject a failure before rename and assert the original file
content remains unchanged. Reference writeGlobalState, readGlobalState and
fs.renameSync (or fs.promises.rename) when locating where to add the spy/assert.
Comment on lines
+220
to
+229
| it("checkpoints pending writes when flushed", async () => { | ||
| const projectRoot = makeProjectRoot("ade-kvdb-flush-"); | ||
| const dbPath = path.join(projectRoot, ".ade", "ade.db"); | ||
| const db = await openKvDb(dbPath, createLogger() as any); | ||
| activeDisposers.push(async () => db.close()); | ||
|
|
||
| db.setJson("flush:probe", { ok: true }); | ||
| expect(() => db.flushNow()).not.toThrow(); | ||
| expect(db.getJson<{ ok: boolean }>("flush:probe")).toEqual({ ok: true }); | ||
| }); |
There was a problem hiding this comment.
This assertion doesn't prove a checkpoint happened.
The same connection can read uncheckpointed WAL data, so this still passes if flushNow() becomes a no-op. Please assert an observable checkpoint effect instead, such as a truncated/absent -wal file after flushNow() or checkpoint stats from SQLite.
🧪 Example of checking the WAL side effect directly
db.setJson("flush:probe", { ok: true });
expect(() => db.flushNow()).not.toThrow();
expect(db.getJson<{ ok: boolean }>("flush:probe")).toEqual({ ok: true });
+ const walPath = `${dbPath}-wal`;
+ if (fs.existsSync(walPath)) {
+ expect(fs.statSync(walPath).size).toBe(0);
+ }🤖 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 `@apps/desktop/src/main/services/state/kvDb.test.ts` around lines 220 - 229,
The test currently only verifies reads from the same open connection so it
doesn't prove a checkpoint occurred; update the "checkpoints pending writes when
flushed" test to assert a WAL-side observable effect after calling
db.flushNow(): e.g., call db.flushNow(), then either close the DB (db.close()),
and assert the WAL file for dbPath (dbPath + "-wal") is absent or truncated on
disk, or run a SQLite checkpoint query (PRAGMA wal_checkpoint or PRAGMA
wal_checkpoint(TRUNCATE)) via the DB handle and assert the returned checkpoint
stats indicate a completed checkpoint; use openKvDb, db.flushNow, db.close and
dbPath to locate the WAL and ensure the test fails if no checkpoint happened.
Comment on lines
+1158
to
+1171
| if (alterTable && isCrsqliteLoaded() && rawHasTable(db, `${alterTable}__crsql_clock`)) { | ||
| getRow(db, "select crsql_begin_alter(?) as ok", [alterTable]); | ||
| try { | ||
| runStatement(db, sql, params); | ||
| } catch (error) { | ||
| // Commit the alter even on failure so the CRR state stays consistent, | ||
| // then re-throw so callers can handle duplicate-column upgrades. | ||
| getRow(db, "select crsql_commit_alter(?) as ok", [alterTable]); | ||
| throw error; | ||
| } | ||
| getRow(db, "select crsql_commit_alter(?) as ok", [alterTable]); | ||
| return; | ||
| } | ||
| runStatement(db, sql, params); |
There was a problem hiding this comment.
Prevent raw ALTER TABLE on CRR-managed tables when cr-sqlite is unavailable
kvDb.ts only uses the crsql_begin_alter/crsql_commit_alter flow when isCrsqliteLoaded() is true; otherwise it falls back to plain runStatement(db, sql, params), even if ${alterTable}__crsql_clock exists—silently risking CRR metadata inconsistency.
Suggested guard
run: (sql: string, params: SqlValue[] = []) => {
const db = getDb();
const alterTable = parseAlterTableTarget(sql);
- if (alterTable && isCrsqliteLoaded() && rawHasTable(db, `${alterTable}__crsql_clock`)) {
+ if (alterTable && rawHasTable(db, `${alterTable}__crsql_clock`)) {
+ if (!isCrsqliteLoaded()) {
+ throw new Error(`Cannot alter CRR table "${alterTable}" without cr-sqlite loaded`);
+ }
getRow(db, "select crsql_begin_alter(?) as ok", [alterTable]);
try {
runStatement(db, sql, params);
} catch (error) {
// Commit the alter even on failure so the CRR state stays consistent,
// then re-throw so callers can handle duplicate-column upgrades.
getRow(db, "select crsql_commit_alter(?) as ok", [alterTable]);
throw error;
}
getRow(db, "select crsql_commit_alter(?) as ok", [alterTable]);
return;
}
runStatement(db, sql, params);🤖 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 `@apps/desktop/src/main/services/state/kvDb.ts` around lines 1158 - 1171, If an
ALTER is targeted at a CRR-managed table (alterTable is set and rawHasTable(db,
`${alterTable}__crsql_clock`) is true) but isCrsqliteLoaded() is false, do not
fall back to raw runStatement; instead abort with a clear error so CRR metadata
won't become inconsistent. Update the branch around
isCrsqliteLoaded()/rawHasTable check in kvDb.ts to detect this case and throw a
descriptive error (or return a failure) referencing alterTable and advising to
load cr-sqlite, rather than calling runStatement; keep existing behavior when
isCrsqliteLoaded() is true (use getRow("select crsql_begin_alter(...)"),
runStatement, then getRow("select crsql_commit_alter(...)")).
Comment on lines
+3343
to
+3345
| flushNow: () => { | ||
| getRow(db, "pragma wal_checkpoint(TRUNCATE)"); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/desktop/src/main/services/state/kvDb.ts"
# Ensure file exists
test -f "$FILE"
# Show the exact section around the reported lines
python3 - <<'PY'
import itertools
path="apps/desktop/src/main/services/state/kvDb.ts"
start=3320
end=3380
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:5d}:{line.rstrip()}")
if i>end:
break
PY
# Locate definition of getRow and its typing/behavior
rg -n "function getRow|const getRow|getRow<|getRow\(" apps/desktop/src/main/services/state/kvDb.ts
# Extract the getRow implementation block (approximate)
python3 - <<'PY'
import re
path="apps/desktop/src/main/services/state/kvDb.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
def find(pattern):
for idx,l in enumerate(lines):
if re.search(pattern,l):
return idx
return None
idx=find(r"\bgetRow\b")
print("First getRow occurrence line:", (idx+1) if idx is not None else None)
PY
# Show larger context around first getRow occurrence
python3 - <<'PY'
path="apps/desktop/src/main/services/state/kvDb.ts"
target=None
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if "getRow" in line and ("function getRow" in line or "const getRow" in line):
target=i
break
print("Implementation start line:", target)
if target:
start=max(1,target-40)
end=target+120
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:5d}:{line.rstrip()}")
if i>end:
break
PY
# Find docblock describing flushNow durability/behavior
rg -n "flushNow" apps/desktop/src/main/services/state/kvDb.ts
rg -n "WAL|wal_checkpoint|checkpoint" apps/desktop/src/main/services/state/kvDb.tsRepository: arul28/ADE
Length of output: 10095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd "$(git rev-parse --show-toplevel)"
FILE="apps/desktop/src/main/services/state/kvDb.ts"
# 1) show docblock around flushNow + interface signature
python3 - <<'PY'
path="apps/desktop/src/main/services/state/kvDb.ts"
start=60
end=110
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:5d}:{line.rstrip()}")
if i>end:
break
PY
# 2) find all call sites
rg -n "flushNow\s*\(" apps/desktop/src/main/services/state apps/desktop/src/main --hidden || true
rg -n "\.flushNow\s*\(" apps/desktop/src/main || true
rg -n "flushNow" apps/desktop/src/main | head -n 50 || trueRepository: arul28/ADE
Length of output: 3547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd "$(git rev-parse --show-toplevel)"
# Show flushNow usage contexts
python3 - <<'PY'
import re
path="apps/desktop/src/main/main.ts"
targets=[4484,4629,5228]
with open(path,'r',encoding='utf-8') as f:
lines=list(f.readlines())
for t in targets:
start=max(1,t-35)
end=min(len(lines),t+25)
print("\n--- main.ts around",t,"---")
for i in range(start,end+1):
if start<=i<=end:
print(f"{i:5d}:{lines[i-1].rstrip()}")
PY
# Show kvDb.test.ts around flushNow expectation
python3 - <<'PY'
path="apps/desktop/src/main/services/state/kvDb.test.ts"
with open(path,'r',encoding='utf-8') as f:
lines=list(f.readlines())
# locate line containing flushNow
for idx,l in enumerate(lines, start=1):
if "db.flushNow" in l:
t=idx
start=max(1,t-30); end=min(len(lines),t+30)
print("\n--- kvDb.test.ts around",t,"---")
for i in range(start,end+1):
print(f"{i:5d}:{lines[i-1].rstrip()}")
break
PY
# Check sqlite PRAGMA wal_checkpoint return shape in code (if any helpers exist)
rg -n "wal_checkpoint\\(TRUNCATE\\)|wal_checkpoint\\(" apps/desktop/src/main/services/state/kvDb.tsRepository: arul28/ADE
Length of output: 8940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cd "$(git rev-parse --show-toplevel)"
FILE="apps/desktop/src/main/services/state/kvDb.ts"
# Find openKvDb and logger usage
rg -n "function openKvDb|openKvDb\s*\(" "$FILE"
rg -n "logger" "$FILE" | head -n 80
# Show the section where the db object is created (around where flushNow is defined)
python3 - <<'PY'
path="apps/desktop/src/main/services/state/kvDb.ts"
# print around the return { ... flushNow ... } block
start=3260
end=3380
with open(path,'r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if start<=i<=end:
print(f"{i:5d}:{line.rstrip()}")
PYRepository: arul28/ADE
Length of output: 6976
🌐 Web query:
SQLite PRAGMA wal_checkpoint(TRUNCATE) return columns busy log checkpointed
💡 Result:
In SQLite, PRAGMA wal_checkpoint(TRUNCATE) returns three columns in a single result row: busy, log, checkpointed. [1] 1) busy - 0 means the checkpoint proceeded; 1 means it did not complete (the checkpoint was blocked / SQLITE_BUSY). [1][2] 2) log - The number of pages still in the WAL file after the checkpoint attempt (i.e., remaining “log” pages). [2] 3) checkpointed - The number of WAL pages that were checkpointed (copied/merged into the main database) by that call. [2] Notes relevant to “busy log checkpointed” - The wal_checkpoint interface uses checkpoint locking semantics; in FULL/RESTART/TRUNCATE modes it can block (and invoke the busy-handler for reader/writer conditions), and it can return SQLITE_BUSY if it cannot obtain required locks or must stop due to contention. [1] - In TRUNCATE mode specifically, the checkpoint is performed and then the WAL file is truncated to zero length when the checkpoint completes. [3][2]
Citations:
- 1: http://sqlite.org/c3ref/wal_checkpoint_v2.html
- 2: https://oneuptime.com/blog/post/2026-03-02-how-to-set-up-sqlite-with-wal-mode-on-ubuntu/view
- 3: https://www2.sqlite.org/wal.html
Check wal_checkpoint(TRUNCATE) result and surface failure in flushNow()
PRAGMA wal_checkpoint(TRUNCATE) returns busy/log/checkpointed (where busy != 0 means the checkpoint didn’t complete). flushNow() currently ignores the returned row and just returns success (getRow(...) result is discarded), while shutdown call sites swallow any thrown errors—so a blocked/unfinished checkpoint can fail silently even on the shutdown path this method is meant to protect. Update flushNow to inspect busy and log/return failure (or otherwise propagate it) when the checkpoint doesn’t complete.
🤖 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 `@apps/desktop/src/main/services/state/kvDb.ts` around lines 3343 - 3345,
flushNow currently calls getRow(db, "pragma wal_checkpoint(TRUNCATE)") and
discards the result; change it to capture the returned row from getRow in
flushNow, inspect the returned "busy" field (or equivalent column) and if busy
!== 0 treat the checkpoint as failed by logging an error via the existing logger
and/or throwing/returning an error so callers (including shutdown paths) can
observe failure; ensure the change references the flushNow function and the
getRow(db, "pragma wal_checkpoint(TRUNCATE)") call and preserves existing error
handling semantics when the query itself errors.
bf1290e to
baa4f5b
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/arul28s-projects?upgradeToPro=build-rate-limit |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
June 1, 2026 01:02
arul28
force-pushed
the
ade-71-state-sync-config-persistence-kvdb-hardening-crash-safe-writes-migration-versioning-and-god-file-dedup-decomposition
branch
from
baa4f5b to
adb2d11
Compare
arul28
force-pushed
the
ade-71-state-sync-config-persistence-kvdb-hardening-crash-safe-writes-migration-versioning-and-god-file-dedup-decomposition
branch
from
adb2d11 to
ab42bc6
Compare
arul28
deleted the
ade-71-state-sync-config-persistence-kvdb-hardening-crash-safe-writes-migration-versioning-and-god-file-dedup-decomposition
branch
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.
Refs ADE-71
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Linked Linear issues
Summary by CodeRabbit
Bug Fixes
Improvements
Greptile Summary
This PR hardens state and database persistence across three areas:
globalState.tsgains a crash-safe atomic write (temp file → fsync → rename → dir-fsync);kvDb.tsreplaces ~40 silent baretry/catchmigration blocks with an idempotentsafeAddColumnhelper, extractsmakeCrrAwareDbto remove duplicated CRR-wrap code, upgrades two deferredBEGINtransactions toBEGIN IMMEDIATE, and implements the previously no-opflushNowviapragma wal_checkpoint(TRUNCATE).globalState.ts: atomic write with fd tracking and cleanup on error; test verifies no.tmpfiles survive a successful write.kvDb.ts:safeAddColumnguards everyALTER TABLE … ADD COLUMNmigration with a pre-check + TOCTOU catch, andthroughDbVersion/createdAtare now captured inside thediscardUnpublishedChangesForTablestransaction for consistency.sessionService.ts/operationService.ts/projectConfigService.ts: SQL-clause coupling, column-list deduplication, and Zod-backed config coercion with no behavioural change.Confidence Score: 5/5
Safe to merge — all changes are additive hardening with no altered public contracts, and the migration path is protected by the new idempotency guards.
The atomic-write change in globalState and the safeAddColumn migration refactor are both well-implemented with appropriate error handling. The BEGIN IMMEDIATE upgrades and the discardUnpublishedChangesForTables fix are correctness improvements. The sessionService SQL-clause refactoring preserves parameter ordering exactly. The only observation is a defensive gap in safeAddColumn's fallback path for unparseable SQL, which cannot be triggered by any current migration string.
apps/desktop/src/main/services/state/kvDb.ts — the safeAddColumn fallback path warrants a second look if new migrations with unusual identifier syntax are ever added.
Important Files Changed
makeCrrAwareDbfactory to remove ~30 lines of duplicated CRR-wrap logic, introducessafeAddColumnfor idempotent ADD COLUMN migrations replacing ~40 bare try/catch blocks, upgradesbegintoBEGIN IMMEDIATEin two write transactions, implementsflushNowviapragma wal_checkpoint(TRUNCATE), and movesthroughDbVersion/createdAtinside thediscardUnpublishedChangesForTablestransaction for correctness.fs.writeFileSyncwith a crash-safe temp-file-then-rename pattern (open → writeFileSync → fsync → close → rename → optional dir-fsync) with proper fd/tempPath cleanup on error.SqlClause = { sql, params }type, joining them at the call site; parameter ordering is semantically identical to the original.OPERATION_COLUMNSconstant used in three queries; no logic change.writeGlobalState→readGlobalStateand asserts no.tmpfiles are left behind after a successful write.flushNow()does not throw and that a previously written key survives the checkpoint.Sequence Diagram
sequenceDiagram participant C as Caller participant K as openKvDb participant CR as makeCrrAwareDb participant M as migrate() participant DB as SQLite C->>K: openKvDb(dbPath, logger) K->>DB: open WAL database K->>CR: "makeCrrAwareDb({ getDb, isCrsqliteLoaded })" CR-->>K: crrAwareDb (run/get/all with CRR-alter wrapping) K->>DB: load crsqlite extension K->>M: migrate(crrAwareDb) — initial pass M->>CR: safeAddColumn(db, sql) × N CR->>DB: pragma table_info / ALTER TABLE / crsql_begin_alter+commit alt CRR table retrofit needed K->>M: migrate(crrAwareDb) — retrofit pass (idempotent via safeAddColumn) end K->>DB: pragma wal_checkpoint on flushNow() K-->>C: "AdeDb { run, get, all, sync, flushNow, close }"Comments Outside Diff (1)
apps/desktop/src/main/services/state/kvDb.ts, line 464-471 (link)console.warnis used here, but the rest ofkvDb.tsuses theloggerinstance (aLoggerinjected viaopenKvDb). A migration column-add failure logged viaconsole.warnwill be invisible to Sentry and to any file/structured log sink the app attaches to theLogger, making it hard to diagnose schema upgrade problems in production builds. Consider threadingloggerintosafeAddColumnas a parameter, or moving the warn call to a site whereloggeris in scope.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "Document kvDb flush checkpoint semantics" | Re-trigger Greptile