feature: task-dnd-ux (1/3) - #1122
Conversation
|
Warning Review limit reached
Next review available in: 17 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 selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds shared task-organization schemas and extension-host contracts. It adds atomic JSON updates and a persistent store for folders, pins, history reconciliation, revision control, corruption recovery, and filesystem synchronization. Comprehensive tests cover persistence, mutations, concurrency, and watcher behavior. ChangesTask organization
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@packages/types/src/task-organization.ts`:
- Around line 68-76: The TaskOrganizationStore.load flow must inspect and
validate only schemaVersion before applying taskOrganizationStateSchema, so
structurally incompatible documents with versions greater than 1 are not
quarantined or replaced. Update the load and mutation-protection logic to
preserve future-version source files and reject mutations, while retaining V1
validation for version 1 documents; add coverage for a structurally incompatible
future document.
In `@src/core/task-persistence/TaskOrganizationStore.ts`:
- Around line 204-205: Update the post-save notification in
src/core/task-persistence/TaskOrganizationStore.ts:204-205 to clone the
committed state under writeLock, then queue onChange after the persistence
transaction completes without awaiting it or propagating observer errors; ensure
the callback receives the snapshot rather than mutable this.state. Apply the
same post-commit notification path to reconciliation at
src/core/task-persistence/TaskOrganizationStore.ts:237-238, preserving mutation
success even when onChange rejects.
- Around line 345-351: Make corrupt-file recovery writable by updating
TaskOrganizationStore.quarantine to remove or move the malformed active file
only after successful archival, and ensure the load flow at
src/core/task-persistence/TaskOrganizationStore.ts:281-292 initializes empty
state only once that file is no longer active. In
src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts:24-34, make
the read mock ignore only ENOENT and rethrow parse or other read errors; at
:114-126, mutate after recovery and assert the active file contains valid JSON.
- Around line 458-477: Update the validation in the mutation method around
resolveUnit and orderedIds so it counts distinct canonical units that contribute
new members, rather than total member IDs. Track whether each resolved unit adds
any previously unseen IDs, increment the unit count once per contributing
target, and reject when fewer than two canonical units contribute while
preserving source-order de-duplication.
- Around line 621-660: Update the root traversal around parentMap and visibleIds
so rootId only follows parentTaskId values present in visibleIds; stop when the
parent is missing rather than promoting that absent ID to rootId. Keep
descendant collection unchanged so the returned closure contains only visible
tasks and preserves reconciliation of auto-group pins.
- Around line 362-365: Update setPinned and the corresponding pin-update path
around lines 563-577 to detect when the requested pins already match the current
state before cloning or modifying revision and updatedAt. For unchanged pins,
return without persistence or observer notification; only increment revision,
update timestamps, save, and notify when pins actually change.
- Around line 298-301: Update the future-schema branch in TaskOrganizationStore
to avoid the unexplained double assertion when assigning data to this.state:
model the future-version state explicitly and use that type, or add an adjacent
comment documenting why casting through unknown is required. Preserve the
existing warning and early return behavior.
- Around line 842-875: Update the watcher setup in the load flow around
getTasksDir and fsSync.watch so the tasks directory is created or ensured
accessible before registering the watcher, including when _taskOrganization.json
is initially absent. Preserve the existing disposed checks and watcher behavior,
and add a test covering watcher initialization with a missing tasks directory.
In `@src/eslint-suppressions.json`:
- Around line 187-190: Remove the increased no-explicit-any suppression
baselines for api/providers/__tests__/mimo.spec.ts and the other affected
entries, restoring their previous counts instead of raising them. Update the
affected test doubles in the corresponding Mimo provider tests to use explicit
types so the existing suppression counts remain unchanged.
In `@src/utils/safeWriteJson.ts`:
- Around line 354-394: Update the rollback handling in the safe-write catch path
to ensure a failed fs.rename in the backup restoration block does not allow the
backup to be deleted by the later cleanup block; preserve
actualTempBackupFilePath when rollback fails and only clear it after successful
restoration. Add a failure-injection test covering rollback failure and
verifying the backup remains available.
- Around line 327-340: Update the commit flow in safeUpdateJson around the
existing backup and rename operations so absoluteFilePath is never absent
between removing the old content and installing the new content. Replace the
two-step rename sequence with a single atomic replacement into the target
pathname, or otherwise retain the target file until replacement succeeds, while
preserving backup behavior and error handling.
🪄 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: b92ee421-2e3c-4f12-b79e-36560699e804
📒 Files selected for processing (9)
packages/types/src/index.tspackages/types/src/task-organization.tspackages/types/src/vscode-extension-host.tssrc/core/task-persistence/TaskOrganizationStore.tssrc/core/task-persistence/__tests__/TaskOrganizationStore.spec.tssrc/core/task-persistence/index.tssrc/eslint-suppressions.jsonsrc/shared/globalFileNames.tssrc/utils/safeWriteJson.ts
| export const taskOrganizationStateSchema = z.object({ | ||
| // Accept any positive integer so that future schema versions can be | ||
| // detected and handled gracefully by the store instead of failing | ||
| // Zod validation and being quarantined as corrupt data. | ||
| schemaVersion: z.number().int().min(1), | ||
| revision: z.number().int().min(0), | ||
| folders: z.array(manualTaskFolderSchema), | ||
| pins: z.array(pinnedItemSchema).max(MAX_PINNED_TARGETS), | ||
| updatedAt: z.number(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Check the schema version before V1 schema validation.
TaskOrganizationStore.load() calls taskOrganizationStateSchema.safeParse(parsed) before it checks schemaVersion. A valid future document that removes or changes a V1-required field such as folders fails validation, is quarantined, and can later be replaced by empty state. This defeats future-schema protection.
Parse and validate only schemaVersion first. If it is greater than 1, preserve the source file and reject mutations without applying the V1 schema. Add a test with a structurally incompatible future document.
🤖 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 `@packages/types/src/task-organization.ts` around lines 68 - 76, The
TaskOrganizationStore.load flow must inspect and validate only schemaVersion
before applying taskOrganizationStateSchema, so structurally incompatible
documents with versions greater than 1 are not quarantined or replaced. Update
the load and mutation-protection logic to preserve future-version source files
and reject mutations, while retaining V1 validation for version 1 documents; add
coverage for a structurally incompatible future document.
| if (this.onChange) { | ||
| await this.onChange(committed) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Isolate onChange from the persistence transaction.
The callback is awaited while writeLock is held and receives the mutable this.state reference. A callback that awaits mutate() or reconcile() deadlocks. A rejected callback also reports failure after save() has committed. Capture a cloned snapshot under the lock, then dispatch notifications through an error-isolated queue after commit.
src/core/task-persistence/TaskOrganizationStore.ts#L204-L205: queue a cloned post-commit snapshot without converting observer failure into a mutation failure.src/core/task-persistence/TaskOrganizationStore.ts#L237-L238: use the same post-commit notification path for reconciliation.
📍 Affects 1 file
src/core/task-persistence/TaskOrganizationStore.ts#L204-L205(this comment)src/core/task-persistence/TaskOrganizationStore.ts#L237-L238
🤖 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 `@src/core/task-persistence/TaskOrganizationStore.ts` around lines 204 - 205,
Update the post-save notification in
src/core/task-persistence/TaskOrganizationStore.ts:204-205 to clone the
committed state under writeLock, then queue onChange after the persistence
transaction completes without awaiting it or propagating observer errors; ensure
the callback receives the snapshot rather than mutable this.state. Apply the
same post-commit notification path to reconciliation at
src/core/task-persistence/TaskOrganizationStore.ts:237-238, preserving mutation
success even when onChange rejects.
| if (data.schemaVersion > 1) { | ||
| console.warn("[TaskOrganizationStore] Organization file has a future schema version.") | ||
| this.state = data as unknown as TaskOrganizationStateV1 | ||
| return |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'TaskOrganizationStore\.ts$' . || true
file="$(fd 'TaskOrganizationStore\.ts$' . | head -n1)"
if [ -n "$file" ]; then
echo
echo "Line count:"
wc -l "$file"
echo
echo "Relevant lines 260-315:"
sed -n '260,315p' "$file" | cat -n -v
echo
echo "Double assertions in file:"
rg -n "\b(as unknown as)\b|as unknown as|as any" "$file" || true
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2744
Explain or remove the double assertion.
data as unknown as TaskOrganizationStateV1 bypasses the schema type and is the only double assertion in this file. Model the future-version state explicitly, or add an adjacent comment explaining why this cast is the required option.
🤖 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 `@src/core/task-persistence/TaskOrganizationStore.ts` around lines 298 - 301,
Update the future-schema branch in TaskOrganizationStore to avoid the
unexplained double assertion when assigning data to this.state: model the
future-version state explicitly and use that type, or add an adjacent comment
documenting why casting through unknown is required. Preserve the existing
warning and early return behavior.
Source: Coding guidelines
| private async quarantine(filePath: string, raw: string): Promise<void> { | ||
| const quarantinePath = `${filePath}.corrupt_${this.now()}.json` | ||
| try { | ||
| await fs.writeFile(quarantinePath, raw, "utf8") | ||
| } catch (err) { | ||
| console.error("[TaskOrganizationStore] Failed to quarantine corrupted organization file:", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make corrupt-file recovery writable.
quarantine() copies corrupt content but leaves the malformed target in place. The next production safeUpdateJson parses that same target and rejects before the updater runs. The test mock converts that parse failure into an absent file, so it cannot detect the failure.
src/core/task-persistence/TaskOrganizationStore.ts#L345-L351: move or remove the malformed target after successful archival.src/core/task-persistence/TaskOrganizationStore.ts#L281-L292: load empty state only after the malformed target is no longer the active file.src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts#L24-L34: ignore onlyENOENT; rethrow JSON parse and other read failures.src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts#L114-L126: mutate after recovery and assert that the active file is valid JSON.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 347-347: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(quarantinePath, raw, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
📍 Affects 2 files
src/core/task-persistence/TaskOrganizationStore.ts#L345-L351(this comment)src/core/task-persistence/TaskOrganizationStore.ts#L281-L292src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts#L24-L34src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts#L114-L126
🤖 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 `@src/core/task-persistence/TaskOrganizationStore.ts` around lines 345 - 351,
Make corrupt-file recovery writable by updating TaskOrganizationStore.quarantine
to remove or move the malformed active file only after successful archival, and
ensure the load flow at
src/core/task-persistence/TaskOrganizationStore.ts:281-292 initializes empty
state only once that file is no longer active. In
src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts:24-34, make
the read mock ignore only ENOENT and rethrow parse or other read errors; at
:114-126, mutate after recovery and assert the active file contains valid JSON.
| const now = this.now() | ||
| const next = structuredClone(this.state) | ||
| next.revision += 1 | ||
| next.updatedAt = now |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep pin no-ops out of the revision protocol.
Lines 364-365 modify next before setPinned detects either no-op. Both no-op branches then save a higher revision and notify observers. A duplicate pin request can make other clients stale without changing pins. Detect unchanged pins before updating revision and updatedAt, then skip persistence and notification.
Also applies to: 563-577
🤖 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 `@src/core/task-persistence/TaskOrganizationStore.ts` around lines 362 - 365,
Update setPinned and the corresponding pin-update path around lines 563-577 to
detect when the requested pins already match the current state before cloning or
modifying revision and updatedAt. For unchanged pins, return without persistence
or observer notification; only increment revision, update timestamps, save, and
notify when pins actually change.
| if (history && "getAll" in history && typeof history.getAll === "function") { | ||
| for (const item of history.getAll()) { | ||
| visibleIds.add(item.id) | ||
| if (item.parentTaskId) { | ||
| parentMap.set(item.id, item.parentTaskId) | ||
| const siblings = childMap.get(item.parentTaskId) ?? [] | ||
| siblings.push(item.id) | ||
| childMap.set(item.parentTaskId, siblings) | ||
| } | ||
| } | ||
| } else { | ||
| visibleIds.add(startTaskId) | ||
| } | ||
|
|
||
| // Walk to the highest known root. | ||
| let rootId = startTaskId | ||
| while (true) { | ||
| const parent = parentMap.get(rootId) | ||
| if (!parent) break | ||
| rootId = parent | ||
| } | ||
|
|
||
| // Collect all descendants. | ||
| const ids: string[] = [] | ||
| const visited = new Set<string>() | ||
| const stack = [rootId] | ||
| while (stack.length > 0) { | ||
| const id = stack.pop()! | ||
| if (visited.has(id)) continue | ||
| visited.add(id) | ||
| ids.push(id) | ||
| const children = childMap.get(id) ?? [] | ||
| for (const child of children) { | ||
| if (!visited.has(child)) { | ||
| stack.push(child) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return { rootId, ids } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not promote a missing parent to the canonical root.
A visible child can retain a parentTaskId after its parent is deleted. Lines 637-641 still select that absent ID as rootId, and Lines 646-660 persist it in the closure. This writes phantom membership and makes auto-group pins disappear during reconciliation. Stop traversal when the parent is absent from visibleIds.
🤖 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 `@src/core/task-persistence/TaskOrganizationStore.ts` around lines 621 - 660,
Update the root traversal around parentMap and visibleIds so rootId only follows
parentTaskId values present in visibleIds; stop when the parent is missing
rather than promoting that absent ID to rootId. Keep descendant collection
unchanged so the returned closure contains only visible tasks and preserves
reconciliation of auto-group pins.
| this.getTasksDir() | ||
| .then((tasksDir) => { | ||
| if (this.disposed) { | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| this.fsWatcher = fsSync.watch(tasksDir, { recursive: false }, (_eventType, filename) => { | ||
| if (this.disposed) { | ||
| return | ||
| } | ||
| if (filename !== GlobalFileNames.taskOrganization) { | ||
| return | ||
| } | ||
| if (this.watcherDebounce) { | ||
| clearTimeout(this.watcherDebounce) | ||
| } | ||
| this.watcherDebounce = setTimeout(() => { | ||
| this.reloadFromWatcher().catch((err) => { | ||
| console.error("[TaskOrganizationStore] Watcher reload failed:", err) | ||
| }) | ||
| }, 500) | ||
| }) | ||
|
|
||
| this.fsWatcher.on("error", (err) => { | ||
| console.error("[TaskOrganizationStore] fs.watch error:", err) | ||
| }) | ||
| } catch (err) { | ||
| console.error("[TaskOrganizationStore] Failed to start fs.watch:", err) | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| console.error("[TaskOrganizationStore] Failed to get tasks dir for watcher:", err) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'TaskOrganizationStore\.ts$' . || true
echo "== file stats =="
wc -l src/core/task-persistence/TaskOrganizationStore.ts
echo "== outline relevant symbols =="
ast-grep outline src/core/task-persistence/TaskOrganizationStore.ts --view expanded | sed -n '1,220p' || true
echo "== relevant lines 780-900 =="
sed -n '780,900p' src/core/task-persistence/TaskOrganizationStore.ts | nl -ba -v780
echo "== search load/getTasksDir/watch/tests references =="
rg -n "load\(|getTasksDir|fs\.watch|tasksDir|taskOrganization|Watcher|watch" src/core/task-persistence/TaskOrganizationStore.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 3892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant lines 230-340 =="
sed -n '230,340p' src/core/task-persistence/TaskOrganizationStore.ts
echo "== relevant lines 830-900 =="
sed -n '830,900p' src/core/task-persistence/TaskOrganizationStore.ts
echo "== tests for TaskOrganizationStore =="
fd -a 'TaskOrganizationStore.*Spec.*|TaskOrganizationStore.*test.*|task-organization.*spec.*|task-organization.*test.*' . || true
echo "== search tests for watcher/load/taskOrganization =="
rg -n "TaskOrganizationStore|taskOrganization|startWatcher|fs\.watch|watcherDebounce|tasksDir|getTasksDir|load\(" src tests .github --glob '!node_modules' --glob '!dist' 2>/dev/null || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 21843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== safeUpdateJson definitions/usages =="
rg -n "function safeUpdateJson|const safeUpdateJson|export .*safeUpdateJson|safeUpdateJson" src --glob '*.ts' | head -80
echo "== safeUpdateJson occurrences =="
for f in $(rg -l "safeUpdateJson" src --glob '*.ts'); do
echo "--- $f"
sed -n '/safeUpdateJson/,+90p' "$f" | head -120
done
echo "== initialize/taskOrganization specs relevant sections =="
sed -n '72,145p' src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts
sed -n '700,860p' src/core/task-persistence/__tests__/TaskOrganizationStore.spec.ts
echo "== Node fs.watch behavior probe for absent directory =="
node - <<'JS'
const fs = require("fs");
const path = require("path");
const { tmpdir } = require("os");
const base = path.join(tmpdir, "zoo-absent-dir-watch-probe-" + process.pid.toString().split("").join(""))
const tasksDir = path.join(base, "tasks");
fs.mkdir(base, { recursive: true });
const watcher = fs.watch(tasksDir, { recursive: false }, () => {});
watcher.once("error", (err) => {
console.log(JSON.stringify({
name: err.name,
code: err.code,
messageContainsEnoent: String(err.message).includes("ENOENT"),
watcherDidEmitError: true,
}));
watcher.close();
});
setTimeout(() => {
console.log("watcherDidEmitError:", false);
watcher.close();
}, 250);
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 19638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== node fs.watch behavior probe for absent directory =="
node - <<'JS'
const fs = require("fs");
const path = require("path");
const { tmpdir } = require("os");
const base = path.join(tmpdir(), "zoo-absent-dir-watch-probe-" + process.pid);
const tasksDir = path.join(base, "tasks");
fs.mkdirSync(base, { recursive: true });
const watcher = fs.watch(tasksDir, { recursive: false }, () => {});
watcher.once("error", (err) => {
console.log(JSON.stringify({
watcherDidEmitErrorBeforeCreate: true,
watcherClosedDueToError: !watcher.isRunning(),
name: err.name,
code: err.code,
messageContainsEnoent: String(err.message).includes("ENOENT"),
}));
watcher.close();
});
setTimeout(() => {
if (watcher.isRunning()) {
console.log(JSON.stringify({
watcherDidNotEmitErrorBeforeCreate: true,
}));
watcher.close();
}
}, 250);
setTimeout(() => {
fs.mkdirSync(tasksDir, { recursive: true });
console.log(JSON.stringify({ directoryCreatedAfterWatcherStarted: true }));
const watcher2 = fs.watch(tasksDir, { recursive: false }, () => {});
let errorSeenForAfterCreated = false;
watcher2.once("error", (err) => {
errorSeenForAfterCreated = true;
console.log(JSON.stringify({
watcher2DidEmitErrorAfterDirectoryCreation: true,
name2: err.name,
code2: err.code,
}));
watcher2.close();
});
setTimeout(() => {
if (!errorSeenForAfterCreated && watcher2.isRunning()) {
console.log(JSON.stringify({
watcher2DidNotEmitErrorAfterDirectoryCreation: true,
}));
watcher2.close();
}
}, 200);
}, 500);
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 1020
Create tasksDir before fs.watch(tasksDir).
load() accepts a missing _taskOrganization.json, but the watcher tries to watch tasks/ before the first write. fs.watch() rejects absent directories, so cross-instance changes will not be reloaded. Create and access the directory before registering the watcher, then add an absent-directory test.
🤖 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 `@src/core/task-persistence/TaskOrganizationStore.ts` around lines 842 - 875,
Update the watcher setup in the load flow around getTasksDir and fsSync.watch so
the tasks directory is created or ensured accessible before registering the
watcher, including when _taskOrganization.json is initially absent. Preserve the
existing disposed checks and watcher behavior, and add a test covering watcher
initialization with a missing tasks directory.
| try { | ||
| await fs.access(absoluteFilePath) | ||
| actualTempBackupFilePath = path.join( | ||
| path.dirname(absoluteFilePath), | ||
| `.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, | ||
| ) | ||
| await fs.rename(absoluteFilePath, actualTempBackupFilePath) | ||
| } catch (accessError: unknown) { | ||
| if (accessError instanceof Error && (accessError as NodeJS.ErrnoException).code !== "ENOENT") { | ||
| throw accessError | ||
| } | ||
| } | ||
|
|
||
| await fs.rename(actualTempNewFilePath, absoluteFilePath) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)safeWriteJson\.ts$|TaskOrganizationStore\.ts$' || true
echo
echo "safeWriteJson outline:"
ast-grep outline src/utils/safeWriteJson.ts --view expanded || true
echo
echo "Relevant safeWriteJson lines:"
sed -n '260,360p' src/utils/safeWriteJson.ts 2>/dev/null || sed -n '260,360p' src/utils/safeWriteJson.ts
echo
echo "Search safeUpdateJson load usages:"
rg -n "safeUpdateJson|safeWriteJson|load\\(" src/core src || true
echo
echo "TaskOrganizationStore relevant lines if exists:"
if [ -f src/core/task-persistence/TaskOrganizationStore.ts ]; then
sed -n '1,260p' src/core/task-persistence/TaskOrganizationStore.ts
fi
echo
echo "Check for atomic write helpers in repository:"
rg -n "rename\\(|fs\\.(rename|mkdir|createWriteStream|access)|atomic|temp|tmp|safeWrite|safeUpdate" src || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate exact files:"
fd -a 'safeWriteJson\.ts|TaskOrganizationStore\.ts' . || true
echo
echo "safeWriteJson target lines:"
FILE=$(fd 'safeWriteJson\.ts' . | head -n1)
if [ -n "${FILE:-}" ]; then
sed -n '260,360p' "$FILE"
fi
echo
echo "Locate TaskOrganizationStore:"
TFILE=$(fd 'TaskOrganizationStore\.ts' . | head -n1)
if [ -n "${TFILE:-}" ]; then
echo "file=$TFILE"
sed -n '1,240p' "$TFILE"
fi
echo
echo "Focused safeUpdateJson references:"
rg -n "safeUpdateJson|safeWriteJson|load\\(" "$FILE" "$TFILE" src/tasks src/core 2>/dev/null | head -n 200
echo
echo "Find rename target in diff/stat if available:"
git diff --stat || true
git diff -- src/utils/safeWriteJson.ts src/core/task-persistence/TaskOrganizationStore.ts 2>/dev/null | sed -n '1,220p' || true
echo
echo "Behavior probe for Node fs.rename rename semantics and ENOENT visibility (temporary scratch files):"
node - <<'JS'
const fs = require('fs');
const path = require('path');
const os = require('os');
const { spawnSync } = require('child_process');
async function probe(label, writeFileSyncFn) {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'safe-write-probe-'));
try {
const target = path.join(dir, 'target.json');
const tmp = path.join(dir, 'tmp.json');
const start = Date.now();
const enoents = [];
const checker = setInterval(() => {
try {
fs.accessSync(target);
} catch (err) {
if (err.code === 'ENOENT') enoents.push(Date.now() - start);
}
}, 0);
writeFileSyncFn(target, tmp);
checker.next && checker.next.return?.();
checker.stop && checker.stop();
console.log(JSON.stringify({ label, enoentReads: enoents.length, times: enoents }));
} finally {
try { fs.rmSync(dir, { recursive: true, force: true }); } catch {}
}
}
(async () => {
const target = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'rename-atomic-check-')), 'target.json');
const tmp = path.join(path.dirname(target), 'tmp.json');
fs.writeFileSync(target, '{}\n');
fs.writeFileSync(tmp, '{}\n');
fs.renameSync(target, tmp + '.back');
const result = fs.statSync(target);
fs.writeFileSync(target, '{}\n');
fs.renameSync(tmp + '.back', tmp);
fs.writeFileSync(tmp, '{}\n');
console.log(JSON.stringify({ originalAtomicityCheck: 'target stat after rename-to-tmp = ' + (result ? 'false' : 'true'), renameMovesFileInSameDir: true }));
})();
JSRepository: Zoo-Code-Org/Zoo-Code
Length of output: 23444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=$(fd 'safeWriteJson\.ts' . | head -n1)
TFILE=$(fd 'TaskOrganizationStore\.ts' . | head -n1)
echo "safeWriteJson later cleanup:"
sed -n '340,410p' "$FILE"
echo
echo "TaskOrganizationStore load/save/watcher:"
sed -n '240,340p' "$TFILE"
sed -n '840,900p' "$TFILE"
sed -n '760,830p' "$TFILE"
echo
echo "Any safeWriteJson/lockfile target removal tests:"
rg -n "safeWriteJson|safeUpdateJson|lockfile|rename\\(|ENOENT|TaskOrganizationStore" src/core/task-persistence __tests__ src/utils || true
echo
echo "Lockfile semantics documentation/source references if installed:"
if [ -d node_modules/lockfile ]; then
rg -n "lock|lockDir|lockfile|stale|unlock|chmod|file" node_modules/lockfile 2>/dev/null | head -n 120 || true
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 23738
🌐 Web query:
proper-lockfile lockfile lock file lock directory exists documentation
💡 Result:
The proper-lockfile library is an inter-process and inter-machine utility designed to manage lock files on local or network file systems [1][2]. Regarding the use of lock files for directories, you can lock a directory by providing the directory path as the file argument and using the lockfilePath option to specify the location of the lock file [1][3]. For example, if you want to lock a directory and create the lock file inside it, you should pass the directory path as the file and specify the full path to the lock file (e.g., /path/to/dir/dir.lock) in the options.lockfilePath property [1][2][4]. Key technical considerations include: 1. Realpath Behavior: By default, proper-lockfile resolves symlinks using realpath (options.realpath defaults to true) [1][5]. When this is enabled, the target file or directory must already exist, or the library will throw an error [1][6]. If you are working with paths that may not yet exist, you can set realpath to false to avoid this behavior [6][7]. 2. Lock Existence: The library manages the creation and cleanup of these files automatically [8]. When checking for a lock, the library verifies if the lock file exists and checks whether it is stale [2][3]. 3. Custom Lock Paths: The lockfilePath option provides flexibility to decouple the lock file location from the resource being locked, which is particularly useful for locking directories or managing custom locking strategies [1][4].
Citations:
- 1: https://www.npmjs.com/package/proper-lockfile
- 2: https://github.com/moxystudio/node-proper-lockfile
- 3: https://github.com/moxystudio/node-proper-lockfile/blob/master/README.md
- 4: moxystudio/node-proper-lockfile@32f1b8d
- 5: https://npmx.dev/package/proper-lockfile
- 6: Throws error if path leading to a file doesn't exist yet. moxystudio/node-proper-lockfile#99
- 7: [BUG] ENOENT from proper-lockfile realpath() after proactive stale lock cleanup CortexReach/memory-lancedb-pro#670
- 8: https://app.unpkg.com/proper-lockfile@4.1.1/files/README.md
Keep the target file present during the commit.
safeUpdateJson() acquires lockfile.lock(absoluteFilePath, { realpath: false }), and that lock is released after the write path, including after fs.rename(absoluteFilePath, actualTempBackupFilePath). Between that rename and fs.rename(actualTempNewFilePath, absoluteFilePath), an unlocked TaskOrganizationStore.load() / watcher reload can see ENOENT and reset the aggregate to empty. Use a single atomic rename into the target pathname, or keep the real file in place until replacement succeeds.
🤖 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 `@src/utils/safeWriteJson.ts` around lines 327 - 340, Update the commit flow in
safeUpdateJson around the existing backup and rename operations so
absoluteFilePath is never absent between removing the old content and installing
the new content. Replace the two-step rename sequence with a single atomic
replacement into the target pathname, or otherwise retain the target file until
replacement succeeds, while preserving backup behavior and error handling.
| } catch (writeError) { | ||
| console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, writeError) | ||
|
|
||
| const newFileToCleanupWithinCatch = actualTempNewFilePath | ||
| const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath | ||
|
|
||
| if (backupFileToRollbackOrCleanupWithinCatch) { | ||
| try { | ||
| await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) | ||
| actualTempBackupFilePath = null | ||
| } catch (rollbackError) { | ||
| console.error( | ||
| `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`, | ||
| rollbackError, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| if (newFileToCleanupWithinCatch) { | ||
| try { | ||
| await fs.unlink(newFileToCleanupWithinCatch) | ||
| } catch (cleanupError) { | ||
| console.error( | ||
| `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, | ||
| cleanupError, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| if (actualTempBackupFilePath) { | ||
| try { | ||
| await fs.unlink(actualTempBackupFilePath) | ||
| } catch (cleanupError) { | ||
| console.error( | ||
| `[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`, | ||
| cleanupError, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| throw writeError |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate safeWriteJson =="
fd -a 'safeWriteJson\.ts$' . || true
file="$(fd 'safeWriteJson\.ts$' . | head -n1 || true)"
if [ -n "$file" ]; then
echo "== outline $file =="
ast-grep outline "$file" --view compact || true
echo "== relevant lines 280-410 =="
cat -n "$file" | sed -n '280,410p'
fi
echo "== search safeWriteJson function names =="
rg -n "safeWriteJson|actualTempBackupFilePath|backupFile|rollback|unlink\\(" . -g 'safeWriteJson.ts' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 9270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'safeWriteJson\.ts$' . | head -n1 || true)"
if [ -z "$file" ]; then
echo "safeWriteJson.ts not found"
exit 0
fi
# Read-only parser-style verifier: extract the state machine around actualTempBackupFilePath
# and report whether any rollback-error catch assigns null/undefined before later unlink of that variable.
python3 - <<'PY'
from pathlib import Path
p = Path(list(Path('.').rglob('safeWriteJson.ts'))[0])
text = p.read_text()
lines = text.splitlines()
for i, line in enumerate(lines, start=1):
if 330 <= i <= 394:
print(f"{i}: {line}")
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2500
Retain the backup when rollback fails.
When fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) fails, actualTempBackupFilePath is not set to null, so the later cleanup block can delete that backup even though the commit did not complete. Keep the backup after rollback failure and cover this path with a failure-injection test.
🤖 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 `@src/utils/safeWriteJson.ts` around lines 354 - 394, Update the rollback
handling in the safe-write catch path to ensure a failed fs.rename in the backup
restoration block does not allow the backup to be deleted by the later cleanup
block; preserve actualTempBackupFilePath when rollback fails and only clear it
after successful restoration. Add a failure-injection test covering rollback
failure and verifying the backup remains available.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… resolution The squash merge used --theirs for eslint-suppressions.json, which kept stale suppression entries that no longer match any code. ESLint's --prune-suppressions removed 13 dead entries, resolving the CI lint failure.
github-actions
Bot
added
the
awaiting-review
Patch coverage checks were blocking 10+ PRs with 80%/70% thresholds. Changed to informational: true so patch coverage is reported but not a required status check.
1 participant
Stack Position
feature/task-dnd-uxDescription
https://youtube.com/shorts/6kx-bNScYew?feature=share
Full Feature Description
feature/task-dnd-uxtask-organization.ts,TaskOrganizationStore.ts,safeWriteJson.ts,taskOrganizationMessageHandler.ts,ClineProvider.ts,HistoryView.tsx,ExtensionStateContext.tsx.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Adds folder/pin/membership/order schema, workspace-scoped aggregate, atomic write, lock/revision conflict, future-schema protection, and corrupt-file recovery. Does not include IPC or UI.
Included Files
packages/types/src/task-organization.tssrc/core/task-persistence/TaskOrganizationStore.tssrc/utils/safeWriteJson.tspackages/types/src/__tests__/task-organization.spec.tssrc/core/task-persistence/__tests__/TaskOrganizationStore.spec.tsExclusion Scope
Summary by CodeRabbit
New Features
Bug Fixes