Skip to content

Fix task description sync issue with smart merge strategy #625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
leex279 wants to merge 2 commits into from
Closed

Fix task description sync issue with smart merge strategy #625

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ed0508
Fix task description/priority sync issue with smart merge strategy
leex279 Sep 9, 2025
6cce7c5
Update optimistic timestamp to enable smart merge strategy
leex279 Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 40 additions & 6 deletions archon-ui-main/src/features/projects/tasks/hooks/useTaskQueries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,10 @@ export function useUpdateTask(projectId: string) {
// Optimistically update
queryClient.setQueryData<Task[]>(taskKeys.all(projectId), (old) => {
if (!old) return old;
return old.map((task) => (task.id === taskId ? { ...task, ...updates } : task));
const nowIso = new Date().toISOString();
return old.map((task) =>
task.id === taskId ? { ...task, ...updates, updated_at: nowIso } : task,
);
});

return { previousTasks };
Expand All @@ -129,11 +132,42 @@ export function useUpdateTask(projectId: string) {
queryClient.invalidateQueries({ queryKey: taskKeys.all(projectId) });
queryClient.invalidateQueries({ queryKey: projectKeys.taskCounts() });
},
onSuccess: (data, { updates }) => {
// Merge server response to keep timestamps and computed fields in sync
queryClient.setQueryData<Task[]>(taskKeys.all(projectId), (old) =>
old ? old.map((t) => (t.id === data.id ? data : t)) : old,
);
onSuccess: (data, { updates, taskId }) => {
// Smart merge: preserve optimistic updates if server data appears stale
queryClient.setQueryData<Task[]>(taskKeys.all(projectId), (old) => {
if (!old) return old;

return old.map((task) => {
if (task.id !== taskId) return task;

try {
// Compare timestamps to detect stale server responses
const serverUpdatedAt = new Date(data.updated_at);
const optimisticUpdatedAt = new Date(task.updated_at);

// If server data is newer or equal, use it completely
if (serverUpdatedAt >= optimisticUpdatedAt) {
return data;
}

// Server data appears stale - merge carefully
// Keep server data for computed fields but preserve optimistic updates
const mergedTask = {
...data, // Start with server data for computed fields
...updates, // Apply the original updates that were made
updated_at: task.updated_at, // Keep the more recent optimistic timestamp
};

return mergedTask;
} catch (timestampError) {
// If timestamp parsing fails, fall back to server data
// This maintains existing behavior for edge cases
console.warn('Failed to parse timestamps for smart merge, using server data:', timestampError);
return data;
}
});
});

Comment on lines +135 to +170
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Add tests for stale/newer/invalid timestamp paths and priority preservation.

Cover: server newer, server stale, invalid timestamp (NaN), updates without priority while cached task has priority.

Run to locate Task/priority definitions and other updated_at usages:

🏁 Script executed:

#!/bin/bash
# Inspect Task type and presence of `priority`
fd -t f -a src | rg -nP '(^|\b)(interface|type)\s+Task\b' -n -C3
rg -n --type=ts '\bpriority\b' -C2
# Check other optimistic updates bumping updated_at
rg -n --type=ts 'onMutate:.*update' -C3
rg -n --type=ts '\bupdated_at\b' -C2

Length of output: 43210

Add tests for timestamp-based merge behaviors and priority retention

Cover these scenarios in useTaskQueries.test.ts:

  • server response newer than optimistic update → replace task with server data
  • server response older than optimistic update → smart-merge and preserve updates
  • invalid updated_at (NaN) → fallback to server data
  • cached task has a priority while updates (and server data) lack it → ensure priority survives the merge
🤖 Prompt for AI Agents 
In archon-ui-main/src/features/projects/tasks/hooks/useTaskQueries.ts around
lines 132 to 167, add unit tests in useTaskQueries.test.ts that cover
timestamp-based merge behavior: 1) simulate an optimistic update then a server
response with updated_at newer than the optimistic task and assert the cached
task is replaced by server data; 2) simulate a server response older than the
optimistic task and assert the cache performs the smart-merge (server computed
fields from response but preserves optimistic updates and keeps the optimistic
updated_at); 3) simulate a server response with invalid updated_at (NaN or
unparsable) and assert the fallback returns server data; 4) create a cached task
that has a priority field while both updates and server response lack priority
and assert the merged task retains the cached priority; set up the queryClient
initial cache, call the onSuccess handler with appropriate data/updates/taskId
for each scenario, and assert the final queryClient.getQueryData for
taskKeys.all(projectId) matches expected merged/replaced result.

Comment on lines +143 to +170
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Fix timestamp handling: Invalid Date doesn’t throw; also retain client-only fields (e.g., priority) regardless of freshness.

new Date(...) never throws; invalid inputs yield “Invalid Date” (NaN time), so the try/catch won’t run and the comparison will mis-route to the stale-merge path. Additionally, when the server is fresher, client-only fields like priority are still dropped. Replace the block to (1) compare via Date.parse with explicit validity checks and (2) always preserve client-only fields when present.

Apply:

-          try {
-            // Compare timestamps to detect stale server responses
-            const serverUpdatedAt = new Date(data.updated_at);
-            const optimisticUpdatedAt = new Date(task.updated_at);
-            
-            // If server data is newer or equal, use it completely
-            if (serverUpdatedAt >= optimisticUpdatedAt) {
-              return data;
-            }
-            
-            // Server data appears stale - merge carefully
-            // Keep server data for computed fields but preserve optimistic updates
-            const mergedTask = {
-              ...data, // Start with server data for computed fields
-              ...updates, // Apply the original updates that were made
-              updated_at: task.updated_at, // Keep the more recent optimistic timestamp
-            };
-            
-            return mergedTask;
-          } catch (timestampError) {
-            // If timestamp parsing fails, fall back to server data
-            // This maintains existing behavior for edge cases
-            console.warn('Failed to parse timestamps for smart merge, using server data:', timestampError);
-            return data;
-          }
+          // Compare timestamps robustly; Date.parse returns NaN for invalid values (no exception)
+          const serverMs = Date.parse(data.updated_at);
+          const optimisticMs = Date.parse(task.updated_at);
+
+          // Carry over client-only fields (e.g., priority) when not part of this update
+          const carryClientOnly = (t: Task) =>
+            (t as any).priority !== undefined && (updates as any).priority === undefined
+              ? { priority: (t as any).priority }
+              : {};
+
+          // Fallback: if either timestamp is invalid, prefer server while keeping client-only fields
+          if (!Number.isFinite(serverMs) || !Number.isFinite(optimisticMs)) {
+            console.warn("Invalid updated_at; preferring server while keeping client-only fields");
+            return { ...data, ...carryClientOnly(task) } as Task;
+          }
+
+          if (serverMs >= optimisticMs) {
+            // Server is fresher or equal: prefer server but keep client-only fields
+            return { ...data, ...carryClientOnly(task) } as Task;
+          }
+
+          // Server appears stale: start from server, reapply updates, keep optimistic timestamp, and client-only fields
+          return {
+            ...data,
+            ...carryClientOnly(task),
+            ...updates,
+            updated_at: task.updated_at,
+          } as Task;
📝 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
try {
// Compare timestamps to detect stale server responses
const serverUpdatedAt = new Date(data.updated_at);
const optimisticUpdatedAt = new Date(task.updated_at);
// If server data is newer or equal, use it completely
if (serverUpdatedAt >= optimisticUpdatedAt) {
return data;
}
// Server data appears stale - merge carefully
// Keep server data for computed fields but preserve optimistic updates
const mergedTask = {
...data, // Start with server data for computed fields
...updates, // Apply the original updates that were made
updated_at: task.updated_at, // Keep the more recent optimistic timestamp
};
return mergedTask;
} catch (timestampError) {
// If timestamp parsing fails, fall back to server data
// This maintains existing behavior for edge cases
console.warn('Failed to parse timestamps for smart merge, using server data:', timestampError);
return data;
}
});
});
// Compare timestamps robustly; Date.parse returns NaN for invalid values (no exception)
const serverMs = Date.parse(data.updated_at);
const optimisticMs = Date.parse(task.updated_at);
// Carry over client-only fields (e.g., priority) when not part of this update
const carryClientOnly = (t: Task) =>
(t as any).priority !== undefined && (updates as any).priority === undefined
? { priority: (t as any).priority }
: {};
// Fallback: if either timestamp is invalid, prefer server while keeping client-only fields
if (!Number.isFinite(serverMs) || !Number.isFinite(optimisticMs)) {
console.warn("Invalid updated_at; preferring server while keeping client-only fields");
return { ...data, ...carryClientOnly(task) } as Task;
}
if (serverMs >= optimisticMs) {
// Server is fresher or equal: prefer server but keep client-only fields
return { ...data, ...carryClientOnly(task) } as Task;
}
// Server appears stale: start from server, reapply updates, keep optimistic timestamp, and client-only fields
return {
...data,
...carryClientOnly(task),
...updates,
updated_at: task.updated_at,
} as Task;
});
});
🤖 Prompt for AI Agents 
In archon-ui-main/src/features/projects/tasks/hooks/useTaskQueries.ts around
lines 140 to 167, replace the try/catch/new Date logic because new Date(...)
never throws and invalid dates produce NaN; instead call Date.parse(...) (or
getTime()) for both server.updated_at and task.updated_at, explicitly check
isNaN for each parsed value and branch accordingly: if both are valid compare
numerically and if server is newer return server data merged with any
client-only fields (e.g., priority) from task or updates so client-only fields
are never dropped; if client is newer return a merged object that preserves
optimistic updated_at and computed server fields; if either timestamp is invalid
fall back to server data but still copy client-only fields from task/updates
into the returned object.

// Only invalidate counts if status changed (which affects counts)
if (updates.status) {
queryClient.invalidateQueries({ queryKey: projectKeys.taskCounts() });
Expand Down