feat: Phase 1 Quick Wins — 7 stability & UX improvements#76
Merged
Conversation
|
PR Summary: Phase 1 Quick Wins: 7 small stability and UX improvements — safer file I/O, worktree/branch UX, log sanitization, plan-file concurrency, smart scrolling, and minor UI polish.
Behavior notes / caveats:
|
![pantoaibot[bot]](https://avatars.githubusercontent.com/in/934356?s=60&v=4){kind=small}
Comment on lines
+652
to
+663
| # Auto-push branch with tracking (-u) to simplify later push/PR operations. | ||
| # Non-fatal: if push fails (e.g., no remote, offline), just warn and continue. | ||
| if not branch_exists: | ||
| push_result = self._run_git(["push", "-u", "origin", branch_name]) | ||
| if push_result.returncode == 0: | ||
| print(f"Auto-pushed branch {branch_name} with upstream tracking") | ||
| else: | ||
| print( | ||
| f"Warning: Could not auto-push branch {branch_name}: " | ||
| f"{push_result.stderr.strip()}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
[REFACTORING] Auto-pushing a newly-created branch by default introduces network calls and side-effects (CI, offline runs, repos without 'origin', or credential prompts). Make this behaviour opt-in via a config flag (e.g. WorktreeManager.auto_push or env var) and verify a remote exists before pushing (git remote get-url origin or similar). Also avoid printing raw stderr to stdout — use debug/logging or print to stderr with clear context. Example improvements: (1) early check for remote existence, (2) guard the push with an if self.config.auto_push: and (3) log failures at debug/warning level rather than printing full stderr to normal stdout.
# Before auto-push, make it configurable and check for a remote
class WorktreeManager:
def __init__(self, repo_path: Path, base_branch: str, auto_push: bool = False):
self.repo_path = Path(repo_path)
self.base_branch = base_branch
self.auto_push = auto_push
# ...
def create_worktree(self, spec_name: str) -> WorktreeInfo:
# ... existing branch creation logic ...
print(f"Created worktree: {worktree_path.name} on branch {branch_name}")
# Auto-push branch with tracking (-u) to simplify later push/PR operations.
# Non-fatal: if push fails (e.g., no remote, offline), just warn and continue.
if not branch_exists and self.auto_push:
# Verify that origin exists before attempting to push
remote_check = self._run_git(["remote", "get-url", "origin"])
if remote_check.returncode != 0:
debug_warning(
"worktree",
"Skipping auto-push: no 'origin' remote configured",
stderr=remote_check.stderr.strip(),
)
else:
push_result = self._run_git(["push", "-u", "origin", branch_name])
if push_result.returncode == 0:
debug_success(
"worktree",
f"Auto-pushed branch {branch_name} with upstream tracking",
)
else:
debug_warning(
"worktree",
f"Could not auto-push branch {branch_name}",
stderr=push_result.stderr.strip(),
)
return WorktreeInfo(
path=worktree_path,
branch=branch_name,
spec_name=spec_name,
base_branch=self.base_branch,
is_active=True,
)
apps/backend/prompts_pkg/prompts.py
Outdated
Comment on lines
+201
to
+290
| @@ -232,6 +287,11 @@ def get_coding_prompt(spec_dir: Path) -> str: | |||
|
|
|||
| """ | |||
|
|
|||
| # Inject worktree isolation warning if applicable | |||
There was a problem hiding this comment.
[CRITICAL_BUG] The new worktree isolation warning (lines ~201-255) conflicts with the existing instruction in spec_context (lines ~276-286) which tells the agent to write code to the project root (the parent of auto-claude). When running inside a worktree the prompt will simultaneously say "write to the project root" and "do NOT cd to the parent project", creating contradictory instructions that can cause the agent to escape the worktree and write to the wrong place. Fix by making the spec_context conditional: when a worktree is detected, update the statement about 'project root' to refer to the worktree root (or explicitly instruct the agent that the worktree root is the only allowed project root). Also add smaller, targeted unit tests for Windows paths and mixed separators to ensure detection logic is robust. Finally consider sanitizing/masking absolute paths in prompts if exposing them to agents is a concern.
def _get_worktree_isolation_warning(spec_dir: Path) -> str:
"""Generate a worktree isolation warning and adjusted root instructions.
When running inside a worktree, we treat the worktree root as the
"project root" for all read/write operations to avoid escaping into
the parent mono‑repo.
"""
spec_dir_str = str(spec_dir).replace("\\", "/")
worktree_markers = [
".auto-claude/worktrees/tasks/",
".worktrees/",
]
worktree_root = None
parent_project = None
for marker in worktree_markers:
idx = spec_dir_str.find(marker)
if idx != -1:
parent_project = spec_dir_str[:idx].rstrip("/")
after_marker = spec_dir_str[idx + len(marker) :]
worktree_name = after_marker.split("/")[0] if after_marker else ""
worktree_root = spec_dir_str[: idx + len(marker)] + worktree_name
break
if not worktree_root or not parent_project:
return ""
return f"""## 🔒 WORKTREE ISOLATION — READ THIS CAREFULLY
You are working inside an **isolated git worktree**, NOT the main project.
- **Worktree root (treat this as your project root):** `{worktree_root}`
- **Forbidden parent mono‑repo root:** `{parent_project}`
🚫 **DO NOT** `cd` to `{parent_project}` or any path outside your worktree.
🚫 **DO NOT** read or write files in the parent project directory.
✅ **ALL** your work must stay within `{worktree_root}`.
When an instruction says "project root", interpret it as **`{worktree_root}`**
for this task. Never write files outside this directory.
If you see absolute paths referencing the parent project in error messages or
imports, resolve them relative to your worktree — do NOT follow them outside.
---
"""
def get_coding_prompt(spec_dir: Path) -> str:
...
worktree_warning = _get_worktree_isolation_warning(spec_dir)
if worktree_warning:
# In worktree mode, clarify what "project root" means for this spec
spec_context = f"""## SPEC LOCATION
Your spec and progress files are located at:
- Spec: `{spec_dir}/spec.md`
- Implementation plan: `{spec_dir}/implementation_plan.json`
- Progress notes: `{spec_dir}/build-progress.txt`
- Recovery context: `{spec_dir}/memory/attempt_history.json`
For this task, **treat your project root as the worktree root described below**.
Do not write code outside that worktree.
---
""" + worktree_warning
else:
spec_context = f"""## SPEC LOCATION
Your spec and progress files are located at:
- Spec: `{spec_dir}/spec.md`
- Implementation plan: `{spec_dir}/implementation_plan.json`
- Progress notes: `{spec_dir}/build-progress.txt`
- Recovery context: `{spec_dir}/memory/attempt_history.json`
The project root is the parent of auto-claude/. All code goes in the project root, not in the spec directory.
---
"""
...
apps/backend/task_logger/storage.py
Outdated
Comment on lines
+15
to
+16
| # Regex to strip ANSI escape codes (CSI sequences like colors, cursor moves) | ||
| _ANSI_ESCAPE_RE = re.compile(r"\033\[[0-9;]*[A-Za-z]") |
There was a problem hiding this comment.
[VALIDATION] The ANSI regex _ANSI_ESCAPE_RE (r"\033[[0-9;][A-Za-z]") is too narrow and will miss many valid CSI/ANSI sequences and control sequences. Replace with a more comprehensive pattern such as: r"\x1b[[0-?][ -/]*[@-~]" (or use a well-tested library) to reliably strip colour/formatting and cursor-control sequences across terminals. Also prefer \x1b for clarity instead of \033.
# Regex to strip ANSI escape codes (CSI sequences like colors, cursor moves)
_ANSI_ESCAPE_RE = re.compile(r"\x1b\[[0-?]*[ -/]*[@-~]")
Comment on lines
+150
to
+164
| export function atomicWriteFileSync(filePath: string, content: string, encoding: BufferEncoding = 'utf-8'): void { | ||
| const tmpPath = filePath + '.tmp'; | ||
| fs.writeFileSync(tmpPath, content, encoding); | ||
| fs.renameSync(tmpPath, filePath); | ||
| } | ||
|
|
||
| /** | ||
| * Write a file atomically (async version). | ||
| * Writes to a temp file first, then renames to prevent 0-byte corruption. | ||
| * | ||
| * @param filePath - The target file path | ||
| * @param content - The content to write | ||
| * @param encoding - File encoding (default: 'utf-8') | ||
| */ | ||
| export async function atomicWriteFile(filePath: string, content: string, encoding: BufferEncoding = 'utf-8'): Promise<void> { |
There was a problem hiding this comment.
[CRITICAL_BUG] atomicWriteFileSync / atomicWriteFile currently write to a fixed tmp filename (filePath + '.tmp') and then rename. This can lead to: (1) races / collisions if two writers run concurrently, (2) possible data loss if the process crashes before the OS flushes buffers because there is no fsync, and (3) orphaned .tmp files on failure. Replace the simple write+rename with a safer sequence: open a uniquely-named temp file in the same directory (e.g. include PID + random suffix or use fs.mkdtemp in the same dir), write via a file descriptor, call fs.fsyncSync (or fsPromises.fsync) after write, close the fd, then fs.rename. For async variant use fsPromises.open + filehandle.write + filehandle.sync + filehandle.close before rename. Also ensure temp filename is created in the same directory as the final file to guarantee rename is atomic on the same filesystem.
export function atomicWriteFileSync(
filePath: string,
content: string,
encoding: BufferEncoding = 'utf-8',
): void {
const dir = path.dirname(filePath);
// Ensure parent directory exists
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true });
}
const uniqueSuffix = `${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
const tmpPath = path.join(dir, `${path.basename(filePath)}.${uniqueSuffix}.tmp`);
let fd: number | null = null;
try {
fd = fs.openSync(tmpPath, 'w');
fs.writeFileSync(fd, content, { encoding });
// Best-effort fsync to reduce corruption risk on crash
try {
fs.fsyncSync(fd);
} catch {
// Ignore fsync errors on platforms/filesystems that don't support it
}
fs.closeSync(fd);
fd = null;
fs.renameSync(tmpPath, filePath);
} catch (err) {
if (fd !== null) {
try {
fs.closeSync(fd);
} catch {
// ignore
}
}
try {
if (fs.existsSync(tmpPath)) {
fs.unlinkSync(tmpPath);
}
} catch {
// ignore cleanup errors
}
throw err;
}
}
export async function atomicWriteFile(
filePath: string,
content: string,
encoding: BufferEncoding = 'utf-8',
): Promise<void> {
const dir = path.dirname(filePath);
// Ensure parent directory exists
await fsPromises.mkdir(dir, { recursive: true });
const uniqueSuffix = `${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
const tmpPath = path.join(dir, `${path.basename(filePath)}.${uniqueSuffix}.tmp`);
let handle: fsPromises.FileHandle | null = null;
try {
handle = await fsPromises.open(tmpPath, 'w');
await handle.writeFile(content, { encoding });
// Best-effort sync for durability
try {
// Node 20+: FileHandle has .sync(); fall back to fsync if not available
// @ts-expect-error Node type versions may lag runtime
if (typeof handle.sync === 'function') {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
await (handle as any).sync();
} else {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
await (fsPromises as any).fsync?.(handle.fd);
}
} catch {
// ignore fsync errors
}
await handle.close();
handle = null;
await fsPromises.rename(tmpPath, filePath);
} catch (err) {
if (handle) {
try {
await handle.close();
} catch {
// ignore
}
}
try {
await fsPromises.unlink(tmpPath);
} catch {
// ignore cleanup errors
}
throw err;
}
}
Comment on lines
+150
to
+154
| export function atomicWriteFileSync(filePath: string, content: string, encoding: BufferEncoding = 'utf-8'): void { | ||
| const tmpPath = filePath + '.tmp'; | ||
| fs.writeFileSync(tmpPath, content, encoding); | ||
| fs.renameSync(tmpPath, filePath); | ||
| } |
There was a problem hiding this comment.
[REFACTORING] Ensure parent directory exists before writing the temp file. atomicWriteFileSync currently does fs.writeFileSync(tmpPath, ...) which will throw ENOENT if parent directories are missing. Either ensureDir(path.dirname(filePath)) at start of atomicWriteFileSync or document the requirement and call ensureDir in callers. Also add try/catch to unlink(tmpPath) on failure to avoid leaving stale .tmp files.
export function atomicWriteFileSync(
filePath: string,
content: string,
encoding: BufferEncoding = 'utf-8',
): void {
const dir = path.dirname(filePath);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true });
}
const tmpPath = filePath + '.tmp';
try {
fs.writeFileSync(tmpPath, content, encoding);
fs.renameSync(tmpPath, filePath);
} catch (err) {
try {
if (fs.existsSync(tmpPath)) {
fs.unlinkSync(tmpPath);
}
} catch {
// ignore cleanup failure
}
throw err;
}
}
Comment on lines
+229
to
+246
| // Spell check context menu: show suggestions, "Add to Dictionary", and standard edit actions | ||
| mainWindow.webContents.on('context-menu', (_event, params) => { | ||
| if (!params.misspelledWord) return; | ||
| const menuItems: Electron.MenuItemConstructorOptions[] = params.dictionarySuggestions.map( | ||
| (suggestion) => ({ | ||
| label: suggestion, | ||
| click: () => mainWindow?.webContents.replaceMisspelling(suggestion), | ||
| }) | ||
| ); | ||
| if (menuItems.length > 0) { | ||
| menuItems.push({ type: 'separator' }); | ||
| } | ||
| menuItems.push({ | ||
| label: 'Add to Dictionary', | ||
| click: () => mainWindow?.webContents.session.addWordToSpellCheckerDictionary(params.misspelledWord), | ||
| }); | ||
| Menu.buildFromTemplate(menuItems).popup(); | ||
| }); |
There was a problem hiding this comment.
[CRITICAL_BUG] The context-menu handler assumes several Electron APIs always exist (replaceMisspelling, session.addWordToSpellCheckerDictionary) and calls Menu.buildFromTemplate(...).popup() without specifying the target BrowserWindow. If any of these APIs are undefined in some Electron versions or contexts the click handlers will throw. Fixes: (1) accept the event parameter (don't use _event), call event.preventDefault() to avoid default menu showing, (2) guard each API call with if (typeof ... === 'function') and wrap click callbacks in try/catch, (3) call menu.popup({ window: mainWindow }) to ensure the menu is shown in the right window, and (4) avoid replacing misspelling if the renderer doesn't support replaceMisspelling (check existence). This avoids uncaught exceptions from context-menu interactions crashing main.
// Spell check context menu: show suggestions, "Add to Dictionary", and standard edit actions
mainWindow.webContents.on('context-menu', (event, params) => {
if (!params.misspelledWord) return;
event.preventDefault();
const menuItems: Electron.MenuItemConstructorOptions[] = params.dictionarySuggestions.map(
(suggestion) => ({
label: suggestion,
click: () => {
try {
if (mainWindow && typeof mainWindow.webContents.replaceMisspelling === 'function') {
mainWindow.webContents.replaceMisspelling(suggestion);
}
} catch (err) {
console.error('[Spellcheck] Failed to replace misspelling:', err);
}
},
})
);
if (menuItems.length > 0) {
menuItems.push({ type: 'separator' });
}
menuItems.push({
label: 'Add to Dictionary',
click: () => {
try {
const wc = mainWindow?.webContents;
const ses = wc?.session as Electron.Session | undefined;
if (ses && typeof ses.addWordToSpellCheckerDictionary === 'function') {
ses.addWordToSpellCheckerDictionary(params.misspelledWord);
}
} catch (err) {
console.error('[Spellcheck] Failed to add word to dictionary:', err);
}
},
});
const menu = Menu.buildFromTemplate(menuItems);
menu.popup({
window: mainWindow ?? undefined,
});
});
Comment on lines
115
to
270
| @@ -267,7 +268,7 @@ export async function createPlanIfNotExists( | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[PERFORMANCE_OPTIMIZATION] You replaced writes with atomicWriteFileSync inside async functions (persistPlanStatus at ~115, updatePlanFile at ~211, createPlanIfNotExists at ~270). Synchronous filesystem calls block the Node event loop and can degrade responsiveness under load. Prefer non-blocking async atomic writes (fs.promises + rename/temp file) in async functions, or provide an async atomicWriteFile variant and use that here. If you must use sync calls, document and accept the blocking behaviour and ensure these operations are rare/expected.
// In apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts
// Prefer async atomic writes to avoid blocking the event loop
import { atomicWriteFile } from '../../fs-utils';
export async function persistPlanStatus(planPath: string, status: TaskStatus, projectId?: string): Promise<boolean> {
return withPlanLock(planPath, async () => {
try {
const planContent = await fsPromises.readFile(planPath, 'utf-8');
const plan = JSON.parse(planContent);
plan.status = status;
plan.planStatus = mapStatusToPlanStatus(status);
plan.updated_at = new Date().toISOString();
await atomicWriteFile(planPath, JSON.stringify(plan, null, 2));
// ...rest of function unchanged
} catch (err) {
// existing error handling
}
});
}
export async function updatePlanFile<T extends Record<string, unknown>>(
planPath: string,
updater: (plan: T) => T
): Promise<T | null> {
return withPlanLock(planPath, async () => {
try {
const planContent = await fsPromises.readFile(planPath, 'utf-8');
const plan = JSON.parse(planContent) as T;
const updatedPlan = updater(plan);
(updatedPlan as Record<string, unknown>).updated_at = new Date().toISOString();
await atomicWriteFile(planPath, JSON.stringify(updatedPlan, null, 2));
return updatedPlan;
} catch (err) {
// existing error handling
return null;
}
});
}
export async function createPlanIfNotExists(
planPath: string,
task: Task,
status: TaskStatus
): Promise<void> {
return withPlanLock(planPath, async () => {
try {
await fsPromises.readFile(planPath, 'utf-8');
return; // already exists
} catch (err) {
if (!isFileNotFoundError(err)) throw err;
}
const plan = {
feature: task.title,
description: task.description || '',
created_at: task.createdAt.toISOString(),
updated_at: new Date().toISOString(),
status,
planStatus: mapStatusToPlanStatus(status),
phases: [],
};
const planDir = path.dirname(planPath);
await fsPromises.mkdir(planDir, { recursive: true });
await atomicWriteFile(planPath, JSON.stringify(plan, null, 2));
});
}
| @@ -167,7 +168,7 @@ export function persistPlanStatusSync(planPath: string, status: TaskStatus, proj | |||
| plan.planStatus = mapStatusToPlanStatus(status); | |||
| plan.updated_at = new Date().toISOString(); | |||
|
|
|||
| writeFileSync(planPath, JSON.stringify(plan, null, 2)); | |||
| atomicWriteFileSync(planPath, JSON.stringify(plan, null, 2)); | |||
There was a problem hiding this comment.
[VALIDATION] persistPlanStatusSync uses atomicWriteFileSync (line ~171). Confirm that atomicWriteFileSync is a true synchronous API. If the implementation is asynchronous (returns Promise), using it from a sync function will be a bug (file write may not complete before function returns). Ensure the exported API names make sync/async semantics explicit (e.g., atomicWriteFile vs atomicWriteFileSync).
| @@ -1,6 +1,7 @@ | |||
| import { app } from 'electron'; | |||
| import { existsSync, Dirent, promises as fsPromises } from 'fs'; | |||
| import path from 'path'; | |||
| import { atomicWriteFile } from './fs-utils'; | |||
There was a problem hiding this comment.
[CRITICAL_BUG] You added import of atomicWriteFile (line 4). Verify the fs-utils API signature and behavior matches how it is used here (async function that fully writes and renames the temp file). Confirm it returns a Promise that rejects on failure and that callers can rely on it to have flushed data (fsync) and to atomically rename the file — otherwise you may still risk partial/zero-byte files. If fs-utils doesn't perform an fsync on the temp file and the parent directory before rename, consider updating it to do so (or document the tradeoff).
// fs-utils.ts
export async function atomicWriteFile(
filePath: string,
content: string,
encoding: BufferEncoding = 'utf-8',
): Promise<void> {
const dir = path.dirname(filePath);
await fsPromises.mkdir(dir, { recursive: true });
const tmpPath = `${filePath}.${process.pid}.${Date.now()}.tmp`;
await fsPromises.writeFile(tmpPath, content, { encoding });
// Best-effort fsync for durability
const handle = await fsPromises.open(tmpPath, 'r+');
try {
await handle.sync();
} finally {
await handle.close();
}
await fsPromises.rename(tmpPath, filePath);
}
| @@ -127,7 +128,7 @@ export class ProjectStore { | |||
| this.writeInProgress = true; | |||
| try { | |||
| const content = JSON.stringify(this.data, null, 2); | |||
| await fsPromises.writeFile(this.storePath, content); | |||
| await atomicWriteFile(this.storePath, content); | |||
There was a problem hiding this comment.
[CRITICAL_BUG] You replaced fsPromises.writeFile with atomicWriteFile(this.storePath, content). Ensure the parent directory for this.storePath exists before calling atomicWriteFile (previous callers might have relied on writeFile to fail or directory to exist). Either (a) ensure atomicWriteFile creates the directory with { recursive: true } or (b) mkdir the parent dir (await fsPromises.mkdir(path.dirname(this.storePath), { recursive: true })) before this call. Also add/verify unit/integration tests that simulate concurrent writes and disk errors to ensure writeInProgress/writePending logic combined with atomic writes does not cause lost writes. Finally, ensure the atomic implementation handles file permissions (mode) consistently so saved files don't suddenly change permission bits.
// project-store.ts (inside saveAsync)
private async saveAsync(): Promise<void> {
if (this.writeInProgress) {
this.writePending = true;
return;
}
this.writeInProgress = true;
try {
const content = JSON.stringify(this.data, null, 2);
// Ensure parent directory exists before atomic write
await fsPromises.mkdir(path.dirname(this.storePath), { recursive: true });
await atomicWriteFile(this.storePath, content);
this.consecutiveFailures = 0;
} catch (error) {
this.consecutiveFailures++;
console.error('[ProjectStore] Error saving store:', error);
if (this.consecutiveFailures >= ProjectStore.MAX_FAILURES_BEFORE_WARNING) {
console.error(
`[ProjectStore] WARNING: ${this.consecutiveFailures} consecutive save failures. ` +
'Store data may not be persisting. Check disk space and permissions.',
);
}
} finally {
this.writeInProgress = false;
if (this.writePending) {
this.writePending = false;
setImmediate(() => this.saveAsync());
}
}
}
Comment on lines
132
to
141
| useEffect(() => { | ||
| const viewport = messagesViewportRef.current; | ||
| if (!viewport) return; | ||
| const onScroll = () => { | ||
| const isNearBottom = viewport.scrollHeight - viewport.scrollTop - viewport.clientHeight < 100; | ||
| setIsUserScrolledUp(!isNearBottom); | ||
| }; | ||
| viewport.addEventListener('scroll', onScroll, { passive: true }); | ||
| return () => viewport.removeEventListener('scroll', onScroll); | ||
| }, []); |
There was a problem hiding this comment.
[CRITICAL_BUG] The effect that attaches the scroll listener runs only once (empty deps) and reads messagesViewportRef.current at mount time. If the ScrollArea sets the viewport ref after mount (typical for callback refs), the listener will never be attached. Change this to attach/detach when the ref changes — either by using a callback ref (useCallback) that installs/removes the listener when the element argument changes, or by adding a stateful ref value to the effect dependencies. Example approach: create a stable onScroll handler with useCallback, then in a callback ref (handleMessagesViewportRef = useCallback((el) => { if (prevEl) prevEl.removeEventListener(...); messagesViewportRef.current = el; if (el) el.addEventListener(...); prevEl = el; }, [...])) so the listener is always attached to the current element and cleaned up properly.
// Replace the manual ref + effect with a callback ref that always has the listener attached
const [isUserScrolledUp, setIsUserScrolledUp] = useState(false);
const messagesEndRef = useRef<HTMLDivElement>(null);
const messagesViewportRef = useRef<HTMLDivElement | null>(null);
const handleMessagesViewportRef = useCallback((el: HTMLDivElement | null) => {
// Detach listener from previous element
if (messagesViewportRef.current) {
messagesViewportRef.current.removeEventListener('scroll', onScrollRef.current!, { capture: false } as any);
}
messagesViewportRef.current = el;
// Attach listener to new element
if (el && onScrollRef.current) {
el.addEventListener('scroll', onScrollRef.current, { passive: true });
}
}, []);
const onScrollRef = useRef<(this: HTMLDivElement, ev: Event) => void>();
useEffect(() => {
const handler = () => {
const viewport = messagesViewportRef.current;
if (!viewport) return;
const isNearBottom =
viewport.scrollHeight - viewport.scrollTop - viewport.clientHeight < 100;
setIsUserScrolledUp(!isNearBottom);
};
onScrollRef.current = handler;
if (messagesViewportRef.current) {
messagesViewportRef.current.addEventListener('scroll', handler, { passive: true });
}
return () => {
if (messagesViewportRef.current && handler) {
messagesViewportRef.current.removeEventListener('scroll', handler);
}
};
}, []);
Comment on lines
+136
to
+137
| const isNearBottom = viewport.scrollHeight - viewport.scrollTop - viewport.clientHeight < 100; | ||
| setIsUserScrolledUp(!isNearBottom); |
There was a problem hiding this comment.
[PERFORMANCE_OPTIMIZATION] Your onScroll handler calls setIsUserScrolledUp on every scroll event which may be high-frequency. Debounce or throttle the handler (or only update state when the boolean actually changes) to avoid excessive re-renders during fast scrolling. For example, compute isNearBottom and only call setIsUserScrolledUp(prev => { if (prev === !isNearBottom) return prev; return !isNearBottom; }).
// Only update state when the value actually changes to avoid redundant renders
useEffect(() => {
const viewport = messagesViewportRef.current;
if (!viewport) return;
const onScroll = () => {
const isNearBottom =
viewport.scrollHeight - viewport.scrollTop - viewport.clientHeight < 100;
setIsUserScrolledUp((prev) => {
const next = !isNearBottom;
return prev === next ? prev : next;
});
};
viewport.addEventListener('scroll', onScroll, { passive: true });
return () => viewport.removeEventListener('scroll', onScroll);
}, []);
Comment on lines
+16
to
+20
| const handleScroll = useCallback((e: React.UIEvent<HTMLDivElement>) => { | ||
| const target = e.target as HTMLDivElement; | ||
| const isNearBottom = target.scrollHeight - target.scrollTop - target.clientHeight < threshold; | ||
| setIsUserScrolledUp(!isNearBottom); | ||
| }, [threshold]); |
There was a problem hiding this comment.
[CRITICAL_BUG] The handler is typed as React.UIEvent but this file does not import React (so the React type will be unresolved). Either import type React from 'react' (import type React from 'react') or change the event type to a DOM type (e.g. Event / UIEvent) or provide a generic param type. Without this the TypeScript build will fail.
import { useState, useRef, useEffect, useCallback } from 'react';
import type React from 'react';
export function useSmartScroll(deps: unknown[] = [], threshold = 100) {
const [isUserScrolledUp, setIsUserScrolledUp] = useState(false);
const endRef = useRef<HTMLDivElement>(null);
const containerRef = useRef<HTMLDivElement>(null);
const handleScroll = useCallback((e: React.UIEvent<HTMLDivElement>) => {
const target = e.currentTarget;
const isNearBottom = target.scrollHeight - target.scrollTop - target.clientHeight < threshold;
setIsUserScrolledUp(!isNearBottom);
}, [threshold]);
useEffect(() => {
if (!isUserScrolledUp && endRef.current) {
endRef.current.scrollIntoView({ behavior: 'smooth' });
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [...deps, isUserScrolledUp]);
const resetScroll = useCallback(() => {
setIsUserScrolledUp(false);
}, []);
return {
endRef,
containerRef,
isUserScrolledUp,
handleScroll,
resetScroll,
};
}
Comment on lines
+76
to
+84
| const viewport = logsViewportRef.current; | ||
| if (!viewport) return; | ||
| const onScroll = () => { | ||
| const isNearBottom = viewport.scrollHeight - viewport.scrollTop - viewport.clientHeight < 100; | ||
| setIsUserScrolledUp(!isNearBottom); | ||
| }; | ||
| viewport.addEventListener('scroll', onScroll, { passive: true }); | ||
| return () => viewport.removeEventListener('scroll', onScroll); | ||
| }, [showLogs]); |
There was a problem hiding this comment.
[REFACTORING] When you attach the scroll listener you never call the handler once to compute the initial isUserScrolledUp state. Add an initial invocation (e.g. onScroll()) after attaching the listener so the auto-scroll decision reflects the current scroll position immediately instead of assuming the user is at bottom.
useEffect(() => {
const viewport = logsViewportRef.current;
if (!viewport) return;
const onScroll = () => {
const isNearBottom =
viewport.scrollHeight - viewport.scrollTop - viewport.clientHeight < 100;
setIsUserScrolledUp(!isNearBottom);
};
viewport.addEventListener('scroll', onScroll, { passive: true });
// Initialize state based on current scroll position
onScroll();
return () => {
viewport.removeEventListener('scroll', onScroll);
};
}, [showLogs]);
Comment on lines
+66
to
+72
| // Detect if description overflows the collapsed height | ||
| useEffect(() => { | ||
| const el = descriptionRef.current; | ||
| if (el) { | ||
| setIsOverflowing(el.scrollHeight > COLLAPSED_MAX_HEIGHT); | ||
| } | ||
| }, [task.description]); |
There was a problem hiding this comment.
[REFACTORING] The overflow detection uses a one-time measurement (el.scrollHeight > COLLAPSED_MAX_HEIGHT) on mount / when task.description changes. Content can change size after images load, markdown renders, or window resizes. Consider using a ResizeObserver on descriptionRef (or re-check on window resize and image load events) to reliably detect overflow and update isOverflowing.
// Detect if description overflows the collapsed height
useEffect(() => {
const el = descriptionRef.current;
if (!el) return;
const updateOverflow = () => {
setIsOverflowing(el.scrollHeight > COLLAPSED_MAX_HEIGHT);
};
// Initial measurement
updateOverflow();
// Recalculate on resize (and when markdown/images change layout)
const resizeObserver = new ResizeObserver(updateOverflow);
resizeObserver.observe(el);
// Fallback: also listen to window resize for older environments
window.addEventListener('resize', updateOverflow);
return () => {
resizeObserver.disconnect();
window.removeEventListener('resize', updateOverflow);
};
}, [task.description]);
Comment on lines
+176
to
+180
| ref={descriptionRef} | ||
| className={cn( | ||
| "prose prose-sm dark:prose-invert max-w-none overflow-hidden prose-p:text-foreground/90 prose-p:leading-relaxed prose-headings:text-foreground prose-strong:text-foreground prose-li:text-foreground/90 prose-ul:my-2 prose-li:my-0.5 prose-a:break-all prose-pre:overflow-x-auto prose-img:max-w-full [&_img]:!max-w-full [&_img]:h-auto [&_code]:break-all [&_code]:whitespace-pre-wrap [&_*]:max-w-full", | ||
| !isExpanded && isOverflowing && "max-h-48" | ||
| )} |
There was a problem hiding this comment.
[NITPICK] You declare COLLAPSED_MAX_HEIGHT = 192 but the conditional class uses the Tailwind utility 'max-h-48' (48 * 0.25rem = 12rem = 192px). For clarity/maintainability prefer a single source of truth: either keep the numeric constant and document the class mapping or compute/derive the class from the constant (or comment the equivalence) so future edits stay consistent.
const COLLAPSED_MAX_HEIGHT = 192; // ~8 lines at 24px line height
const COLLAPSED_MAX_HEIGHT_CLASS = 'max-h-48'; // 48 * 0.25rem = 12rem = 192px
// ...
<div
ref={descriptionRef}
className={cn(
"prose prose-sm dark:prose-invert max-w-none overflow-hidden prose-p:text-foreground/90 prose-p:leading-relaxed prose-headings:text-foreground prose-strong:text-foreground prose-li:text-foreground/90 prose-ul:my-2 prose-li:my-0.5 prose-a:break-all prose-pre:overflow-x-auto prose-img:max-w-full [&_img]:!max-w-full [&_img]:h-auto [&_code]:break-all [&_code]:whitespace-pre-wrap [&_*]:max-w-full",
!isExpanded && isOverflowing && COLLAPSED_MAX_HEIGHT_CLASS,
)}
style={{ overflowWrap: 'anywhere' }}
>
{/* ... */}
</div>
Comment on lines
+76
to
+77
| "showMore": "Show more", | ||
| "showLess": "Show less", |
There was a problem hiding this comment.
[VALIDATION] You added buttons.showMore / buttons.showLess. Actionable items:
- Search the codebase for usages of these keys (search for 'showMore' and 'showLess') to ensure the UI is wired to the new keys and to avoid runtime missing-key issues.
- Avoid duplicating accessibility labels: there are existing keys expandAriaLabel / collapseAriaLabel earlier in this file — prefer reusing those ARIA keys for the expand/collapse control instead of adding separate strings unless you need different text for the visible button vs aria label.
- Add these keys to every other locale (not only FR) or ensure your i18n fallback logic will not surface raw keys in other locales. Missing translations cause UX regressions.
- Run your i18n extraction/lint step and update any translation snapshots or translation platform exports so CI/localization pipelines remain consistent.
// apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx
// ...inside TaskMetadata component render, button label usage already correct:
<Button
variant="ghost"
size="sm"
onClick={() => setIsExpanded(!isExpanded)}
className="text-xs text-muted-foreground hover:text-foreground gap-1 h-6"
>
{isExpanded ? (
<>
<ChevronUp className="h-3 w-3" />
{/* Visible label and ARIA can share the same i18n keys */}
{t('common:buttons.showLess', 'Show less')}
</>
) : (
<>
<ChevronDown className="h-3 w-3" />
{t('common:buttons.showMore', 'Show more')}
</>
)}
</Button>
// To avoid duplication and ensure ARIA consistency, you can wire existing
// accessibility keys when adding an aria-label, for example if the button
// were icon-only:
<Button
variant="ghost"
size="sm"
onClick={() => setIsExpanded(!isExpanded)}
className="text-xs text-muted-foreground hover:text-foreground gap-1 h-6"
aria-label={isExpanded ? t('common:accessibility.collapseAriaLabel') : t('common:accessibility.expandAriaLabel')}
>
{isExpanded ? <ChevronUp className="h-3 w-3" /> : <ChevronDown className="h-3 w-3" />}
</Button>
|
Reviewed up to commit:47d1e5a082e6469c3234a464b7501eb2bf56a254 Additional Suggestionapps/frontend/src/main/fs-utils.ts, line:164-167The async atomicWriteFile writes then renames without fsync; on many filesystems rename may succeed before data is durable. Use filehandle.sync()/fsPromises.fsync on the file descriptor before closing and renaming to reduce risk of corruption on crashes. This will cost a synchronous fsync but is required when durability matters for store files.export async function atomicWriteFile(
filePath: string,
content: string,
encoding: BufferEncoding = 'utf-8',
): Promise<void> {
const dir = path.dirname(filePath);
await fsPromises.mkdir(dir, { recursive: true });
const tmpPath = filePath + '.tmp';
let handle: fsPromises.FileHandle | null = null;
try {
handle = await fsPromises.open(tmpPath, 'w');
await handle.writeFile(content, { encoding });
try {
// Prefer FileHandle.sync when available
// @ts-expect-error Node types may not yet expose sync()
if (typeof handle.sync === 'function') {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
await (handle as any).sync();
} else {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
await (fsPromises as any).fsync?.(handle.fd);
}
} catch {
// Ignore fsync errors to avoid breaking on filesystems that don't support it
}
await handle.close();
handle = null;
await fsPromises.rename(tmpPath, filePath);
} catch (err) {
if (handle) {
try {
await handle.close();
} catch {
// ignore
}
}
try {
await fsPromises.unlink(tmpPath);
} catch {
// ignore cleanup failure
}
throw err;
}
}apps/frontend/src/renderer/components/Insights.tsx, line:281You pass onViewportRef={handleMessagesViewportRef} to ScrollArea (line 281). Confirm that the ScrollArea component actually exposes an onViewportRef prop and that it passes an HTMLDivElement to that callback. Some ScrollArea implementations use a different prop name (e.g., viewportRef or getViewportNode). If this doesn't match the ScrollArea API the ref will be undefined and scrolling logic will fail; update to the correct prop or adapt the ScrollArea component.// If ScrollArea expects a different prop (e.g. viewportRef), adapt accordingly:
<ScrollArea
className="flex-1 px-6 py-4"
viewportRef={handleMessagesViewportRef}
>
{/* messages */}
</ScrollArea>
// Or, if onViewportRef is correct, tighten the type so we know we get an HTMLDivElement
const handleMessagesViewportRef = useCallback((el: HTMLDivElement | null) => {
messagesViewportRef.current = el;
}, []);apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx, line:160-162Inline style uses wordBreak: 'break-word' together with overflowWrap: 'anywhere'. 'word-break: break-word' is non-standard in some environments — relying on overflowWrap: 'anywhere' is sufficient and more standard. Consider removing or documenting the duplicate behavior to avoid confusion.<div
ref={descriptionRef}
className={cn(
"prose prose-sm dark:prose-invert max-w-none overflow-hidden prose-p:text-foreground/90 prose-p:leading-relaxed prose-headings:text-foreground prose-strong:text-foreground prose-li:text-foreground/90 prose-ul:my-2 prose-li:my-0.5 prose-a:break-all prose-pre:overflow-x-auto prose-img:max-w-full [&_img]:!max-w-full [&_img]:h-auto [&_code]:break-all [&_code]:whitespace-pre-wrap [&_*]:max-w-full",
!isExpanded && isOverflowing && 'max-h-48',
)}
style={{ overflowWrap: 'anywhere' }}
>
<ReactMarkdown remarkPlugins={[remarkGfm]}>
{displayDescription}
</ReactMarkdown>
</div>Others- The repository now contains duplicated smart-scroll logic (GenerationProgressScreen implements its own scroll listener while a new useSmartScroll hook also exists). Prefer consolidating: either use the new useSmartScroll hook in GenerationProgressScreen or move the shared logic into the hook (including auto-attach and cleanup). This reduces duplication and the chance of divergent behavior.// apps/frontend/src/renderer/components/Insights.tsx
// Replace local smart-scroll implementation with hook usage
import { useSmartScroll } from '../../hooks/useSmartScroll';
export function Insights({ projectId }: InsightsProps) {
// ...existing state/selectors...
const {
endRef: messagesEndRef,
handleScroll,
resetScroll,
} = useSmartScroll([
session?.messages?.length,
streamingContent,
]);
useEffect(() => {
loadInsightsSession(projectId).then(() => {
resetScroll();
});
const cleanup = setupInsightsListeners();
return cleanup;
}, [projectId, resetScroll]);
// ...
<ScrollArea
className="flex-1 px-6 py-4"
// assuming ScrollArea forwards onScroll to the viewport
onScroll={handleScroll}
>
{/* ... */}
<div ref={messagesEndRef} />
</ScrollArea>
}
// apps/frontend/src/renderer/components/ideation/GenerationProgressScreen.tsx
import { useSmartScroll } from '../../hooks/useSmartScroll';
export function GenerationProgressScreen(/* props */) {
const logsEndRef = useRef<HTMLDivElement>(null);
const {
handleScroll: handleLogsScroll,
} = useSmartScroll([logs.length, showLogs]);
// ...
{showLogs && logs.length > 0 && (
<div className="shrink-0 border-b border-border p-4 bg-muted/20">
<ScrollArea
className="h-32 rounded-md border border-border bg-muted/30"
onScroll={handleLogsScroll}
>
<div className="p-3 space-y-1 font-mono text-xs">
{/* ...logs... */}
<div ref={logsEndRef} />
</div>
</ScrollArea>
</div>
)}
} |
…ptions, worktree improvements, spell check, ANSI stripping Seven low-effort, high-value improvements: 1. Atomic file writes: Write to temp file + rename to prevent 0-byte corruption on crash. Applied to settings, project store, plan files, session storage, and profile storage. 2. Smart auto-scroll: Only auto-scroll streaming content when user is near bottom (100px threshold). Prevents jarring scroll during reading. Applied to Insights chat and ideation generation logs. 3. Expandable task descriptions: Long descriptions (>8 lines) show a gradient overlay with "Show more"/"Show less" button in task detail view. 4. Worktree branch auto-push: After creating a new worktree branch, auto-push with -u flag for upstream tracking. Non-fatal on failure. 5. Worktree isolation warning: Inject prominent isolation warning in agent prompts when working inside a worktree, preventing agents from escaping to the parent project directory. 6. Spell check: Enable Electron's built-in spell checker with context menu suggestions and "Add to Dictionary" support. 7. ANSI stripping: Strip ANSI escape codes from task logger storage (Python) and log service (TypeScript) before persisting to prevent color noise in stored logs and UI display. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add error classification, JSON recovery, memory monitoring, circuit breaker, adaptive PR polling, merge progress streaming, structured PR review findings, auth failure badge, and rate-limit markers. Includes 52 new tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OBenner
force-pushed
the
feat/phase1-quick-wins
branch
from
e8b4b60 to
573a23b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ixes - fs-utils: unique tmp filenames, mkdir parent, cleanup on failure - index.ts: guard Electron spellcheck APIs with try/catch and type checks - Insights.tsx: callback ref for scroll listener (fixes empty deps issue) - useSmartScroll.ts: add React type import - worktree.py: check remote exists before auto-push, use logger - storage.py: broader ANSI regex for full CSI sequence stripping - plan-file-utils.ts: async atomicWriteFile in async functions - GenerationProgressScreen.tsx: initial onScroll call after attach - TaskMetadata.tsx: ResizeObserver, COLLAPSED_MAX_HEIGHT_CLASS constant - model_fallback.py: reset_circuit_breakers() to fix test isolation - test_multi_model_orchestration.py: autouse fixture to reset breakers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds resilience primitives (circuit breakers, error classification, memory pressure/session bounds), JSON-recovery parsing, atomic file writes, merge progress callbacks, UI improvements (smart scroll, spellcheck, auth-failure badge), ANSI log sanitization, and tests; duplicates detected in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AgentSession
participant MemoryMonitor as Memory (rgba(100,149,237,0.5))
participant CircuitBreaker as CB (rgba(220,20,60,0.5))
participant ErrorClassifier as EC (rgba(34,139,34,0.5))
participant ExternalAPI as SDK (rgba(255,165,0,0.5))
Client->>AgentSession: start run_agent_session(request)
AgentSession->>Memory: check_pressure()
alt CRITICAL
Memory-->>AgentSession: CRITICAL
AgentSession->>Client: abort (memory pressure)
else not CRITICAL
AgentSession->>CB: can_execute()
alt OPEN
CB-->>AgentSession: OPEN
AgentSession->>Client: abort (circuit open)
else CLOSED/HALF_OPEN
AgentSession->>SDK: call API
SDK-->>AgentSession: response / exception
alt response
AgentSession->>CB: record_success()
AgentSession->>EC: classify_response(response)
alt fatal
EC-->>AgentSession: fatal
AgentSession->>Client: abort (fatal classified)
else non-fatal
AgentSession->>Client: continue
end
else exception
AgentSession->>EC: classify_exception(exc)
AgentSession->>CB: record_failure(exc)
AgentSession->>Client: handle mapped error
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/frontend/src/main/settings-utils.ts (1)
52-62: 🧹 Nitpick | 🔵 TrivialMinor redundancy: directory creation is duplicated.
Lines 56-59 create the directory before calling
atomicWriteFileSync, butatomicWriteFileSyncalready handles directory creation internally (seeapps/frontend/src/main/fs-utils.tslines 151-154). The code works correctly, but the explicit directory creation here is redundant.🧹 Optional cleanup
export function writeSettingsFile(settings: Record<string, unknown>): void { const settingsPath = getSettingsPath(); - // Ensure the directory exists - const dir = path.dirname(settingsPath); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } - atomicWriteFileSync(settingsPath, JSON.stringify(settings, null, 2), 'utf-8'); }apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts (1)
1701-1724:⚠️ Potential issue | 🟡 MinorBuffer stdout lines across chunks before parsing merge-progress events, and use
getMainWindow()for consistent window targeting.Node.js stream
'data'events deliver arbitrary chunk boundaries that can split newlines, causing incomplete JSON lines to be dropped. The current per-chunk parsing silently loses progress updates when a JSON line spans chunks. Additionally,BrowserWindow.getAllWindows()[0]is fragile and inconsistent with the rest of the file, which usesgetMainWindow().🔧 Suggested fix
- mergeProcess.stdout.on('data', (data: Buffer) => { - const chunk = data.toString(); - stdout += chunk; - debug('STDOUT:', chunk); - - // Parse merge progress events from stdout and forward to renderer - // Progress lines look like: {"phase":"merging","current":3,"total":10,"file":"src/foo.ts"} - for (const line of chunk.split('\n')) { + let progressBuffer = ''; + mergeProcess.stdout.on('data', (data: Buffer) => { + const chunk = data.toString(); + stdout += chunk; + debug('STDOUT:', chunk); + progressBuffer += chunk; + const lines = progressBuffer.split('\n'); + progressBuffer = lines.pop() ?? ''; + + // Parse merge progress events from stdout and forward to renderer + // Progress lines look like: {"phase":"merging","current":3,"total":10,"file":"src/foo.ts"} + for (const line of lines) { const trimmed = line.trim(); if (trimmed.startsWith('{"phase":')) { try { const progress = JSON.parse(trimmed); - const mainWindow = BrowserWindow.getAllWindows()[0]; + const mainWindow = getMainWindow(); if (mainWindow) { mainWindow.webContents.send('merge-progress', { taskId: task.id, ...progress, }); } } catch { // Not valid JSON progress event, ignore } } } });apps/backend/core/model_fallback.py (1)
95-170:⚠️ Potential issue | 🟠 MajorStop retrying when the breaker opens mid-retry.
If a HALF_OPEN probe fails,
record_failure()flips the breaker to OPEN, but the retry loop continues with the same model becausecan_execute()is only checked before entering retries. This defeats the circuit breaker and can hammer a failing model. Break out once the breaker opens.🐛 Suggested fix
if is_retryable: breaker.record_failure(e) + if not breaker.can_execute(): + logger.warning( + f"[SKIP] Circuit breaker opened for model '{current_model}' — " + "aborting remaining retries for this model" + ) + break if retry < max_retries_per_model - 1: logger.warning( f"[RETRY] API error with model '{current_model}' ({error_type}: {error_msg}). " f"Retrying... (retry {retry + 1}/{max_retries_per_model}, attempt {attempt_num}/{len(models_to_try)})" ) continue # Retry with same modelAs per coding guidelines, Check for proper error handling and security considerations.
🤖 Fix all issues with AI agents
In `@apps/backend/agents/session.py`:
- Around line 962-973: The periodic GC uses a magic literal 50; extract it to a
clearly named constant (e.g., GC_MESSAGE_INTERVAL or MEMORY_GC_MESSAGE_INTERVAL)
and replace the hard-coded check in the block that calls
_memory_monitor.maybe_gc() (the line using message_count % 50 == 0) with
message_count % GC_MESSAGE_INTERVAL == 0; define the constant near the top of
the module or with other configuration values so SessionBounds.check / reason,
debug_error, and task_logger.log_error logic remains unchanged while improving
readability and configurability.
In `@apps/backend/core/json_recovery.py`:
- Around line 132-179: Add an early size check at the start of
parse_json_with_recovery using the module constant _MAX_INPUT_SIZE: if
len(raw_text) exceeds _MAX_INPUT_SIZE, immediately raise a json.JSONDecodeError
(include a short message and the raw_text slice/context similar to the existing
final raise) so tier-1 direct parse and tier-3 extraction never run on oversized
inputs; update the function body before any try/except or calls to
_repair_json_syntax and _extract_json_block and include the context string (ctx)
in the error message.
In `@apps/backend/core/memory_monitor.py`:
- Around line 55-59: The current except clause references psutil.Error which
couples this code to psutil being imported; change the exception handling in the
method that calls self._process.memory_info() (in
apps/backend/core/memory_monitor.py) to catch a general Exception (or at minimum
Exception instead of psutil.Error) so the block becomes defensive if psutil is
unavailable or the code is refactored; keep the behavior the same (return -1.0)
on any caught exception.
In `@apps/backend/core/worktree.py`:
- Around line 652-668: The auto-push block uses self._run_git without specifying
a timeout so it falls back to the default (60s) which is inconsistent with
push_branch that uses self.GIT_PUSH_TIMEOUT; update the auto-push calls to pass
the same timeout value (self.GIT_PUSH_TIMEOUT) when invoking self._run_git for
both the "remote get-url" check and the "push -u origin branch_name" call to
match push_branch behavior and avoid premature timeouts; locate the logic around
branch_exists, remote_check, push_result and branch_name in worktree.py and
apply the timeout argument to those _run_git invocations.
In `@apps/backend/merge/orchestrator.py`:
- Around line 323-345: The calls to progress_callback in merge_task (the
analyzing and per-file "merging" callbacks) and in merge_tasks are unguarded and
can abort the merge if the callback raises; wrap each progress_callback
invocation in a try/except that catches Exception and calls debug_warning(...)
with context (phase, file path, current/total) so failures are logged but the
merge continues; update the invocations inside merge_task (the initial analyzing
callback and the per-file merging callback) and the progress_callback call
inside merge_tasks to use this pattern and preserve the original arguments when
logging.
In `@apps/backend/runners/github/providers/github_provider.py`:
- Around line 160-199: The post_review method can produce a body exceeding
GitHub's 65,536-character limit when appending findings/evidence; modify
post_review to enforce a max length (65_536) before calling
_gh_client.pr_review: build the body as now using ReviewData/findings/evidence
but track cumulative length, and if adding a finding or evidence would exceed
the cap truncate the appended text (preferably truncate evidence lines first)
and append a clear truncation notice like "…(truncated X characters of
findings/evidence)"; ensure the final body is trimmed to the limit and still
passes review.event.upper() to _gh_client.pr_review to avoid the 422 error.
In `@apps/backend/task_logger/storage.py`:
- Around line 105-109: The add_entry method's docstring is inaccurate: it says
only content and detail fields are stripped of ANSI escape codes, but the
implementation also strips tool_input. Update the add_entry docstring to state
that ANSI escape codes are removed from content, detail, and tool_input (or any
relevant input fields on LogEntry) before storage, and mention that sanitization
happens within add_entry to keep docs in sync with the behavior.
In `@apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts`:
- Around line 224-250: The effect restarts every fetch because prs is in the
dependency array; change it to depend only on the open-PR state instead of the
entire prs array: compute and store a derived value (e.g., openCount or hasOpen)
outside the effect (using useMemo/useRef or a small piece of state updated when
setPrs runs), reference that derived symbol in the useEffect dependency list
instead of prs, and keep pollRef, fetchPRs, isActive, isConnected, and
hasLoadedRef in the deps; ensure the cleanup logic in the existing useEffect
(clearing pollRef) remains unchanged.
In `@apps/frontend/src/renderer/components/ideation/GenerationProgressScreen.tsx`:
- Line 178: The inline arrow passed to ScrollArea's onViewportRef recreates on
each render; extract it to a stable callback to avoid changing the prop
reference. Create a memoized setter using useCallback (e.g., setLogsViewportRef)
that accepts el and assigns logsViewportRef.current = el, and pass that function
to ScrollArea's onViewportRef instead of the inline (el) => {
logsViewportRef.current = el; } so the prop reference remains stable across
renders.
- Around line 74-85: The effect that attaches the scroll listener can miss the
first showLogs toggle because logsViewportRef.current may be null when useEffect
runs; change from relying on logsViewportRef mutable ref to a callback ref so
the ScrollArea element calls a setter when mounted (follow the callback ref
pattern used in Insights.tsx), move the addEventListener/onScroll initialization
into that callback so you attach the scroll listener immediately when the DOM
node is available, and ensure you still call onScroll() to initialize
setIsUserScrolledUp and remove the listener in the cleanup when the node changes
or unmounts; update references to logsViewportRef, onScroll, setIsUserScrolledUp
and showLogs accordingly.
In `@apps/frontend/src/renderer/components/Insights.tsx`:
- Around line 120-125: The effect in Insights.tsx that auto-scrolls (useEffect
referencing messagesEndRef.scrollIntoView) should not include isUserScrolledUp
in its dependency list because that causes a manual scroll to the bottom to
re-trigger the effect; instead either remove isUserScrolledUp from the
dependency array and rely on session?.messages?.length and streamingContent, or
read isUserScrolledUp via a stable ref (e.g., isUserScrolledUpRef) inside the
effect so the effect only re-runs for new messages/streaming events; update the
useEffect that references messagesEndRef, and ensure consistency with the
behavior used by the useSmartScroll hook.
In `@apps/frontend/src/renderer/components/Sidebar.tsx`:
- Around line 515-535: The Settings Button becomes icon-only when isCollapsed
and lacks an accessible name; update the Button (the element rendering <Button
... onClick={onSettingsClick}>) so that when isCollapsed is true it receives an
aria-label (use the translated string t('actions.settings')), and make the badge
rendered when hasPendingAuthFailure decorative by removing its aria-label and
adding aria-hidden="true" (or role="presentation") so screen readers announce
the button name instead of the badge; keep TooltipTrigger/TooltipContent
behavior unchanged.
In `@apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx`:
- Around line 62-80: The component keeps isExpanded across different tasks; add
a useEffect that listens for the task identity (e.g., task.id or
task.description if id isn't available) and calls setIsExpanded(false) to reset
collapsed state whenever the task changes; place this alongside the existing
ResizeObserver effect so isExpanded is initialized per-task while retaining the
isOverflowing logic in TaskMetadata.
In `@apps/frontend/src/renderer/hooks/useSmartScroll.ts`:
- Around line 12-43: Replace the inline smart-scroll implementation in
Insights.tsx with the shared hook useSmartScroll: import useSmartScroll, remove
the duplicate state/refs/handlers (scroll state, endRef, containerRef,
handleScroll, resetScroll, isUserScrolledUp) currently implemented in
Insights.tsx, call useSmartScroll([...deps]) in the component, attach the
returned containerRef to the scrollable container, wire onScroll to
handleScroll, place endRef at the bottom where auto-scroll should target, and
call resetScroll in the same places you currently reset scroll state; ensure you
still read isUserScrolledUp where needed for UI logic.
- Around line 23-29: The effect in useSmartScroll currently lists
isUserScrolledUp in its dependency array causing a scroll-to-bottom when the
user scrolls back down; change the effect to only run when deps change and read
the up-scroll state from a stable ref instead of state. Create (or reuse) an
isUserScrolledUpRef that mirrors isUserScrolledUp, remove isUserScrolledUp from
the dependency array in the useEffect that calls endRef.current.scrollIntoView,
and inside the effect check isUserScrolledUpRef.current before scrolling; update
the places that set isUserScrolledUp to also update isUserScrolledUpRef so
behavior remains consistent.
In `@tests/test_circuit_breaker.py`:
- Around line 37-66: Tests currently use time.sleep which causes flakiness;
instead patch the time source used by CircuitBreaker by mocking time.monotonic
(or the module's time function) so you can advance time deterministically in
tests. Update the half-open tests that instantiate CircuitBreaker and call
record_failure/record_success (symbols: CircuitBreaker, CircuitState,
record_failure, record_success, can_execute) to replace real sleeps with a
patched monotonic value that you increment by recovery_timeout + epsilon to
force the transition to HALF_OPEN, then assert states and can_execute; apply the
same approach to all three half-open tests for deterministic behavior.
In `@tests/test_memory_monitor.py`:
- Around line 19-25: The test currently patches _PSUTIL_AVAILABLE after the
class has been imported, so MemoryMonitor.__init__ already evaluated that flag
and the patch is ineffective (the test only passes because mm._process is
manually set to None); either remove the misleading `@patch` and keep the explicit
mm._process = None (simplify test_no_psutil_returns_normal), or ensure the
no-psutil scenario is exercised correctly by setting the module-level
_PSUTIL_AVAILABLE before instantiating MemoryMonitor (e.g., use importlib.reload
or a fixture/monkeypatch to set core.memory_monitor._PSUTIL_AVAILABLE then
reload the module) so that MemoryMonitor.__init__ sees the flag—then assert
MemoryMonitor.check_pressure(), get_usage_mb(), and should_gc() behave as
expected.
Comment on lines
+132
to
+179
| def parse_json_with_recovery( | ||
| raw_text: str, context: str = "" | ||
| ) -> tuple[dict[str, Any] | list[Any], str]: | ||
| """Parse JSON text with three tiers of progressive recovery. | ||
|
|
||
| Args: | ||
| raw_text: The raw text to parse as JSON. | ||
| context: Optional label for log messages (e.g. "implementation_plan"). | ||
|
|
||
| Returns: | ||
| A tuple of ``(parsed_object, tier_used)`` where *tier_used* is one of | ||
| ``"tier1_direct"``, ``"tier2_repair"``, or ``"tier3_extract"``. | ||
|
|
||
| Raises: | ||
| json.JSONDecodeError: If all recovery tiers fail. | ||
| """ | ||
| ctx = f" ({context})" if context else "" | ||
|
|
||
| # --- Tier 1: direct parse -------------------------------------------------- | ||
| try: | ||
| return json.loads(raw_text), "tier1_direct" | ||
| except json.JSONDecodeError: | ||
| logger.debug("JSON tier-1 (direct) failed%s", ctx) | ||
|
|
||
| # --- Tier 2: syntax repair ------------------------------------------------- | ||
| repaired = _repair_json_syntax(raw_text) | ||
| if repaired is not None: | ||
| try: | ||
| logger.info("JSON tier-2 (repair) succeeded%s", ctx) | ||
| return json.loads(repaired), "tier2_repair" | ||
| except json.JSONDecodeError: | ||
| pass # fall through to tier 3 | ||
|
|
||
| # --- Tier 3: extract from surrounding text / fences ------------------------ | ||
| extracted = _extract_json_block(raw_text) | ||
| if extracted is not None: | ||
| try: | ||
| logger.info("JSON tier-3 (extract) succeeded%s", ctx) | ||
| return json.loads(extracted), "tier3_extract" | ||
| except json.JSONDecodeError: | ||
| pass | ||
|
|
||
| # All tiers failed | ||
| raise json.JSONDecodeError( | ||
| f"All JSON recovery tiers failed{ctx}", | ||
| raw_text[:200], | ||
| 0, | ||
| ) |
Contributor
There was a problem hiding this comment.
Enforce _MAX_INPUT_SIZE before tier-1/3 to avoid large-input CPU/memory spikes.
Tier-1 parsing and tier-3 extraction still run on arbitrarily large inputs, even though _MAX_INPUT_SIZE exists. For untrusted or accidental large blobs, this can cause heavy CPU/memory usage. Add an early length guard in parse_json_with_recovery.
🛡️ Suggested guard
def parse_json_with_recovery(
raw_text: str, context: str = ""
) -> tuple[dict[str, Any] | list[Any], str]:
@@
ctx = f" ({context})" if context else ""
+
+ if len(raw_text) > _MAX_INPUT_SIZE:
+ raise json.JSONDecodeError(
+ f"Input exceeds {_MAX_INPUT_SIZE} bytes{ctx}",
+ raw_text[:200],
+ 0,
+ )As per coding guidelines, Check for proper error handling and security considerations.
🤖 Prompt for AI Agents
In `@apps/backend/core/json_recovery.py` around lines 132 - 179, Add an early size
check at the start of parse_json_with_recovery using the module constant
_MAX_INPUT_SIZE: if len(raw_text) exceeds _MAX_INPUT_SIZE, immediately raise a
json.JSONDecodeError (include a short message and the raw_text slice/context
similar to the existing final raise) so tier-1 direct parse and tier-3
extraction never run on oversized inputs; update the function body before any
try/except or calls to _repair_json_syntax and _extract_json_block and include
the context string (ctx) in the error message.
Comment on lines
+652
to
+668
| # Auto-push branch with tracking (-u) to simplify later push/PR operations. | ||
| # Non-fatal: if push fails (e.g., no remote, offline), just warn and continue. | ||
| if not branch_exists: | ||
| remote_check = self._run_git(["remote", "get-url", "origin"]) | ||
| if remote_check.returncode != 0: | ||
| logger.warning("Skipping auto-push: no 'origin' remote configured") | ||
| else: | ||
| push_result = self._run_git(["push", "-u", "origin", branch_name]) | ||
| if push_result.returncode == 0: | ||
| logger.info( | ||
| f"Auto-pushed branch {branch_name} with upstream tracking" | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| f"Could not auto-push branch {branch_name}: " | ||
| f"{push_result.stderr.strip()}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Use consistent timeout for push operations.
The auto-push uses _run_git with the default 60-second timeout, while push_branch (line 1016) explicitly uses self.GIT_PUSH_TIMEOUT (120 seconds) for the same operation. On slower connections or large repositories, the auto-push could timeout prematurely compared to explicit push operations.
Proposed fix to use consistent timeout
else:
- push_result = self._run_git(["push", "-u", "origin", branch_name])
+ push_result = self._run_git(
+ ["push", "-u", "origin", branch_name],
+ timeout=self.GIT_PUSH_TIMEOUT,
+ )
if push_result.returncode == 0:📝 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
| # Auto-push branch with tracking (-u) to simplify later push/PR operations. | |
| # Non-fatal: if push fails (e.g., no remote, offline), just warn and continue. | |
| if not branch_exists: | |
| remote_check = self._run_git(["remote", "get-url", "origin"]) | |
| if remote_check.returncode != 0: | |
| logger.warning("Skipping auto-push: no 'origin' remote configured") | |
| else: | |
| push_result = self._run_git(["push", "-u", "origin", branch_name]) | |
| if push_result.returncode == 0: | |
| logger.info( | |
| f"Auto-pushed branch {branch_name} with upstream tracking" | |
| ) | |
| else: | |
| logger.warning( | |
| f"Could not auto-push branch {branch_name}: " | |
| f"{push_result.stderr.strip()}" | |
| ) | |
| # Auto-push branch with tracking (-u) to simplify later push/PR operations. | |
| # Non-fatal: if push fails (e.g., no remote, offline), just warn and continue. | |
| if not branch_exists: | |
| remote_check = self._run_git(["remote", "get-url", "origin"]) | |
| if remote_check.returncode != 0: | |
| logger.warning("Skipping auto-push: no 'origin' remote configured") | |
| else: | |
| push_result = self._run_git( | |
| ["push", "-u", "origin", branch_name], | |
| timeout=self.GIT_PUSH_TIMEOUT, | |
| ) | |
| if push_result.returncode == 0: | |
| logger.info( | |
| f"Auto-pushed branch {branch_name} with upstream tracking" | |
| ) | |
| else: | |
| logger.warning( | |
| f"Could not auto-push branch {branch_name}: " | |
| f"{push_result.stderr.strip()}" | |
| ) |
🤖 Prompt for AI Agents
In `@apps/backend/core/worktree.py` around lines 652 - 668, The auto-push block
uses self._run_git without specifying a timeout so it falls back to the default
(60s) which is inconsistent with push_branch that uses self.GIT_PUSH_TIMEOUT;
update the auto-push calls to pass the same timeout value
(self.GIT_PUSH_TIMEOUT) when invoking self._run_git for both the "remote
get-url" check and the "push -u origin branch_name" call to match push_branch
behavior and avoid premature timeouts; locate the logic around branch_exists,
remote_check, push_result and branch_name in worktree.py and apply the timeout
argument to those _run_git invocations.
Comment on lines
+323
to
345
| total_files = len(modifications) | ||
| if progress_callback: | ||
| progress_callback( | ||
| { | ||
| "phase": "analyzing", | ||
| "current": 0, | ||
| "total": total_files, | ||
| "file": "", | ||
| } | ||
| ) | ||
|
|
||
| # Process each modified file | ||
| for file_path, snapshot in modifications: | ||
| for file_idx, (file_path, snapshot) in enumerate(modifications): | ||
| if progress_callback: | ||
| progress_callback( | ||
| { | ||
| "phase": "merging", | ||
| "current": file_idx + 1, | ||
| "total": total_files, | ||
| "file": file_path, | ||
| } | ||
| ) | ||
| debug_detailed( |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, get file size and read the relevant sections
wc -l apps/backend/merge/orchestrator.pyRepository: OBenner/Auto-Coding
Length of output: 101
🏁 Script executed:
# Read the code at lines 323-345
sed -n '320,350p' apps/backend/merge/orchestrator.pyRepository: OBenner/Auto-Coding
Length of output: 1190
🏁 Script executed:
# Read the code at lines 452-463
sed -n '450,470p' apps/backend/merge/orchestrator.pyRepository: OBenner/Auto-Coding
Length of output: 921
🏁 Script executed:
# Find all progress_callback calls in the file
rg -n "progress_callback" apps/backend/merge/orchestrator.py -A 2Repository: OBenner/Auto-Coding
Length of output: 975
🏁 Script executed:
# Check for debug_warning function availability
rg -n "def debug_warning|from.*debug_warning|import.*debug_warning" apps/backend/merge/orchestrator.pyRepository: OBenner/Auto-Coding
Length of output: 105
🏁 Script executed:
# Check how debug functions are used elsewhere in the file
rg -n "debug_" apps/backend/merge/orchestrator.py | head -20Repository: OBenner/Auto-Coding
Length of output: 826
🏁 Script executed:
# Get the full method signature and more context around line 324
sed -n '280,360p' apps/backend/merge/orchestrator.pyRepository: OBenner/Auto-Coding
Length of output: 3460
🏁 Script executed:
# Get more context around line 454
sed -n '410,470p' apps/backend/merge/orchestrator.pyRepository: OBenner/Auto-Coding
Length of output: 2428
🏁 Script executed:
# Do a comprehensive search for ALL progress_callback calls
rg -c "progress_callback" apps/backend/merge/orchestrator.pyRepository: OBenner/Auto-Coding
Length of output: 65
🏁 Script executed:
# Find all exact lines with progress_callback calls (not just references)
rg "progress_callback\(" apps/backend/merge/orchestrator.py -nRepository: OBenner/Auto-Coding
Length of output: 187
Wrap progress_callback invocations in exception handlers to prevent merge failures.
Three unhandled progress_callback() calls across merge_task() (lines 325, 337) and merge_tasks() (line 455) could abort the merge if the callback raises. Wrap each invocation in try/except and log failures with debug_warning() to maintain operation resilience.
🛡️ Suggested fix
total_files = len(modifications)
if progress_callback:
- progress_callback(
- {
- "phase": "analyzing",
- "current": 0,
- "total": total_files,
- "file": "",
- }
- )
+ try:
+ progress_callback(
+ {
+ "phase": "analyzing",
+ "current": 0,
+ "total": total_files,
+ "file": "",
+ }
+ )
+ except Exception as exc:
+ debug_warning(MODULE, "progress_callback failed", error=str(exc))
# Process each modified file
for file_idx, (file_path, snapshot) in enumerate(modifications):
if progress_callback:
- progress_callback(
- {
- "phase": "merging",
- "current": file_idx + 1,
- "total": total_files,
- "file": file_path,
- }
- )
+ try:
+ progress_callback(
+ {
+ "phase": "merging",
+ "current": file_idx + 1,
+ "total": total_files,
+ "file": file_path,
+ }
+ )
+ except Exception as exc:
+ debug_warning(MODULE, "progress_callback failed", error=str(exc))Apply the same pattern to the merge_tasks() method call at line 455.
🤖 Prompt for AI Agents
In `@apps/backend/merge/orchestrator.py` around lines 323 - 345, The calls to
progress_callback in merge_task (the analyzing and per-file "merging" callbacks)
and in merge_tasks are unguarded and can abort the merge if the callback raises;
wrap each progress_callback invocation in a try/except that catches Exception
and calls debug_warning(...) with context (phase, file path, current/total) so
failures are logged but the merge continues; update the invocations inside
merge_task (the initial analyzing callback and the per-file merging callback)
and the progress_callback call inside merge_tasks to use this pattern and
preserve the original arguments when logging.
Comment on lines
+62
to
+80
| const { t } = useTranslation(['tasks', 'errors', 'common']); | ||
| const [isExpanded, setIsExpanded] = useState(false); | ||
| const [isOverflowing, setIsOverflowing] = useState(false); | ||
| const descriptionRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| // Detect if description overflows the collapsed height (using ResizeObserver for accuracy) | ||
| useEffect(() => { | ||
| const el = descriptionRef.current; | ||
| if (!el) return; | ||
|
|
||
| const updateOverflow = () => { | ||
| setIsOverflowing(el.scrollHeight > COLLAPSED_MAX_HEIGHT); | ||
| }; | ||
| updateOverflow(); | ||
|
|
||
| const resizeObserver = new ResizeObserver(updateOverflow); | ||
| resizeObserver.observe(el); | ||
| return () => resizeObserver.disconnect(); | ||
| }, [task.description]); |
Contributor
There was a problem hiding this comment.
Reset expanded state when switching tasks.
If a user expands one task, the next long task will render expanded by default because isExpanded persists. Reset the state on task change to keep the default collapsed behavior per task.
🛠️ Proposed fix
const [isExpanded, setIsExpanded] = useState(false);
const [isOverflowing, setIsOverflowing] = useState(false);
const descriptionRef = useRef<HTMLDivElement>(null);
+useEffect(() => {
+ setIsExpanded(false);
+}, [task.id]);📝 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
| const { t } = useTranslation(['tasks', 'errors', 'common']); | |
| const [isExpanded, setIsExpanded] = useState(false); | |
| const [isOverflowing, setIsOverflowing] = useState(false); | |
| const descriptionRef = useRef<HTMLDivElement>(null); | |
| // Detect if description overflows the collapsed height (using ResizeObserver for accuracy) | |
| useEffect(() => { | |
| const el = descriptionRef.current; | |
| if (!el) return; | |
| const updateOverflow = () => { | |
| setIsOverflowing(el.scrollHeight > COLLAPSED_MAX_HEIGHT); | |
| }; | |
| updateOverflow(); | |
| const resizeObserver = new ResizeObserver(updateOverflow); | |
| resizeObserver.observe(el); | |
| return () => resizeObserver.disconnect(); | |
| }, [task.description]); | |
| const { t } = useTranslation(['tasks', 'errors', 'common']); | |
| const [isExpanded, setIsExpanded] = useState(false); | |
| const [isOverflowing, setIsOverflowing] = useState(false); | |
| const descriptionRef = useRef<HTMLDivElement>(null); | |
| useEffect(() => { | |
| setIsExpanded(false); | |
| }, [task.id]); | |
| // Detect if description overflows the collapsed height (using ResizeObserver for accuracy) | |
| useEffect(() => { | |
| const el = descriptionRef.current; | |
| if (!el) return; | |
| const updateOverflow = () => { | |
| setIsOverflowing(el.scrollHeight > COLLAPSED_MAX_HEIGHT); | |
| }; | |
| updateOverflow(); | |
| const resizeObserver = new ResizeObserver(updateOverflow); | |
| resizeObserver.observe(el); | |
| return () => resizeObserver.disconnect(); | |
| }, [task.description]); |
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx` around
lines 62 - 80, The component keeps isExpanded across different tasks; add a
useEffect that listens for the task identity (e.g., task.id or task.description
if id isn't available) and calls setIsExpanded(false) to reset collapsed state
whenever the task changes; place this alongside the existing ResizeObserver
effect so isExpanded is initialized per-task while retaining the isOverflowing
logic in TaskMetadata.
Comment on lines
+12
to
+43
| export function useSmartScroll(deps: unknown[] = [], threshold = 100) { | ||
| const [isUserScrolledUp, setIsUserScrolledUp] = useState(false); | ||
| const endRef = useRef<HTMLDivElement>(null); | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const handleScroll = useCallback((e: React.UIEvent<HTMLDivElement>) => { | ||
| const target = e.target as HTMLDivElement; | ||
| const isNearBottom = target.scrollHeight - target.scrollTop - target.clientHeight < threshold; | ||
| setIsUserScrolledUp(!isNearBottom); | ||
| }, [threshold]); | ||
|
|
||
| // Auto-scroll to bottom when deps change, but only if user hasn't scrolled up | ||
| useEffect(() => { | ||
| if (!isUserScrolledUp && endRef.current) { | ||
| endRef.current.scrollIntoView({ behavior: 'smooth' }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [...deps, isUserScrolledUp]); | ||
|
|
||
| // Reset scroll state (e.g., when switching tabs or starting a new session) | ||
| const resetScroll = useCallback(() => { | ||
| setIsUserScrolledUp(false); | ||
| }, []); | ||
|
|
||
| return { | ||
| endRef, | ||
| containerRef, | ||
| isUserScrolledUp, | ||
| handleScroll, | ||
| resetScroll, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using this hook in Insights.tsx for consistency.
This hook provides the same smart scroll functionality that Insights.tsx implements inline (lines 109-149). Using this hook would reduce duplication and ensure consistent behavior across components.
Note: The containerRef is exposed but unused internally—this appears intentional for consumers who need direct container access, which is fine.
🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/hooks/useSmartScroll.ts` around lines 12 - 43,
Replace the inline smart-scroll implementation in Insights.tsx with the shared
hook useSmartScroll: import useSmartScroll, remove the duplicate
state/refs/handlers (scroll state, endRef, containerRef, handleScroll,
resetScroll, isUserScrolledUp) currently implemented in Insights.tsx, call
useSmartScroll([...deps]) in the component, attach the returned containerRef to
the scrollable container, wire onScroll to handleScroll, place endRef at the
bottom where auto-scroll should target, and call resetScroll in the same places
you currently reset scroll state; ensure you still read isUserScrolledUp where
needed for UI logic.
Comment on lines
+23
to
+29
| // Auto-scroll to bottom when deps change, but only if user hasn't scrolled up | ||
| useEffect(() => { | ||
| if (!isUserScrolledUp && endRef.current) { | ||
| endRef.current.scrollIntoView({ behavior: 'smooth' }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [...deps, isUserScrolledUp]); |
Contributor
There was a problem hiding this comment.
Potential unintended scroll on isUserScrolledUp state change.
Including isUserScrolledUp in the effect's dependencies means:
- User scrolls up →
isUserScrolledUpbecomestrue - Effect re-runs, but skips scroll (condition
!isUserScrolledUpis false) ✓ - User scrolls back down →
isUserScrolledUpbecomesfalse - Effect re-runs and scrolls to bottom, even if
depshaven't changed
This may cause an unexpected scroll when the user manually scrolls back to the bottom. Consider whether isUserScrolledUp should be in the dependency array or accessed via a ref.
💡 Alternative approach using ref
+ const isUserScrolledUpRef = useRef(false);
+
+ const handleScroll = useCallback((e: React.UIEvent<HTMLDivElement>) => {
+ const target = e.target as HTMLDivElement;
+ const isNearBottom = target.scrollHeight - target.scrollTop - target.clientHeight < threshold;
+ isUserScrolledUpRef.current = !isNearBottom;
+ setIsUserScrolledUp(!isNearBottom);
+ }, [threshold]);
+
// Auto-scroll to bottom when deps change, but only if user hasn't scrolled up
useEffect(() => {
- if (!isUserScrolledUp && endRef.current) {
+ if (!isUserScrolledUpRef.current && endRef.current) {
endRef.current.scrollIntoView({ behavior: 'smooth' });
}
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [...deps, isUserScrolledUp]);
+ }, [...deps]);🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/hooks/useSmartScroll.ts` around lines 23 - 29, The
effect in useSmartScroll currently lists isUserScrolledUp in its dependency
array causing a scroll-to-bottom when the user scrolls back down; change the
effect to only run when deps change and read the up-scroll state from a stable
ref instead of state. Create (or reuse) an isUserScrolledUpRef that mirrors
isUserScrolledUp, remove isUserScrolledUp from the dependency array in the
useEffect that calls endRef.current.scrollIntoView, and inside the effect check
isUserScrolledUpRef.current before scrolling; update the places that set
isUserScrolledUp to also update isUserScrolledUpRef so behavior remains
consistent.
Comment on lines
+37
to
+66
| def test_transitions_to_half_open(self): | ||
| cb = CircuitBreaker("test", failure_threshold=2, recovery_timeout=0.1) | ||
| cb.record_failure(Exception("err")) | ||
| cb.record_failure(Exception("err")) | ||
| assert cb.state == CircuitState.OPEN | ||
|
|
||
| # Wait for recovery timeout | ||
| time.sleep(0.15) | ||
| assert cb.state == CircuitState.HALF_OPEN | ||
| assert cb.can_execute() is True | ||
|
|
||
| def test_half_open_success_closes(self): | ||
| cb = CircuitBreaker("test", failure_threshold=2, recovery_timeout=0.1) | ||
| cb.record_failure(Exception("err")) | ||
| cb.record_failure(Exception("err")) | ||
| time.sleep(0.15) | ||
| assert cb.state == CircuitState.HALF_OPEN | ||
|
|
||
| cb.record_success() | ||
| assert cb.state == CircuitState.CLOSED | ||
|
|
||
| def test_half_open_failure_reopens(self): | ||
| cb = CircuitBreaker("test", failure_threshold=2, recovery_timeout=0.1) | ||
| cb.record_failure(Exception("err")) | ||
| cb.record_failure(Exception("err")) | ||
| time.sleep(0.15) | ||
| assert cb.state == CircuitState.HALF_OPEN | ||
|
|
||
| cb.record_failure(Exception("probe failed")) | ||
| assert cb.state == CircuitState.OPEN |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid time.sleep in tests to prevent flakiness.
The half-open tests depend on real time and can be flaky on slow CI. Prefer patching time.monotonic to simulate elapsed time deterministically.
♻️ Example approach (apply similarly to all half-open tests)
def test_transitions_to_half_open(self):
cb = CircuitBreaker("test", failure_threshold=2, recovery_timeout=0.1)
cb.record_failure(Exception("err"))
cb.record_failure(Exception("err"))
assert cb.state == CircuitState.OPEN
-
- # Wait for recovery timeout
- time.sleep(0.15)
- assert cb.state == CircuitState.HALF_OPEN
+ with patch("core.circuit_breaker.time.monotonic") as mono:
+ mono.return_value = cb._last_failure_time + 0.2
+ assert cb.state == CircuitState.HALF_OPEN
assert cb.can_execute() is TrueAs per coding guidelines, Check for proper mocking and test isolation.
🤖 Prompt for AI Agents
In `@tests/test_circuit_breaker.py` around lines 37 - 66, Tests currently use
time.sleep which causes flakiness; instead patch the time source used by
CircuitBreaker by mocking time.monotonic (or the module's time function) so you
can advance time deterministically in tests. Update the half-open tests that
instantiate CircuitBreaker and call record_failure/record_success (symbols:
CircuitBreaker, CircuitState, record_failure, record_success, can_execute) to
replace real sleeps with a patched monotonic value that you increment by
recovery_timeout + epsilon to force the transition to HALF_OPEN, then assert
states and can_execute; apply the same approach to all three half-open tests for
deterministic behavior.
Comment on lines
+19
to
+25
| @patch("core.memory_monitor._PSUTIL_AVAILABLE", False) | ||
| def test_no_psutil_returns_normal(self): | ||
| mm = MemoryMonitor() | ||
| mm._process = None | ||
| assert mm.check_pressure() == MemoryPressure.NORMAL | ||
| assert mm.get_usage_mb() == -1.0 | ||
| assert not mm.should_gc() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The _PSUTIL_AVAILABLE patch is misleading and may not work as intended.
The _PSUTIL_AVAILABLE flag is evaluated at module import time, so patching it after import has no effect on MemoryMonitor.__init__(). The test works only because of the manual mm._process = None assignment on line 22.
Consider simplifying or clarifying:
♻️ Suggested simplification
- `@patch`("core.memory_monitor._PSUTIL_AVAILABLE", False)
def test_no_psutil_returns_normal(self):
+ """Test behavior when psutil process handle is unavailable."""
mm = MemoryMonitor()
mm._process = None
assert mm.check_pressure() == MemoryPressure.NORMAL
assert mm.get_usage_mb() == -1.0
assert not mm.should_gc()Alternatively, to properly test the no-psutil scenario, you'd need to patch before the class is instantiated or use importlib.reload.
🤖 Prompt for AI Agents
In `@tests/test_memory_monitor.py` around lines 19 - 25, The test currently
patches _PSUTIL_AVAILABLE after the class has been imported, so
MemoryMonitor.__init__ already evaluated that flag and the patch is ineffective
(the test only passes because mm._process is manually set to None); either
remove the misleading `@patch` and keep the explicit mm._process = None (simplify
test_no_psutil_returns_normal), or ensure the no-psutil scenario is exercised
correctly by setting the module-level _PSUTIL_AVAILABLE before instantiating
MemoryMonitor (e.g., use importlib.reload or a fixture/monkeypatch to set
core.memory_monitor._PSUTIL_AVAILABLE then reload the module) so that
MemoryMonitor.__init__ sees the flag—then assert MemoryMonitor.check_pressure(),
get_usage_mb(), and should_gc() behave as expected.
- json_recovery.py: replace \s* with [ \t]* in fence regex to prevent polynomial backtracking (S5852 security hotspot) - error_classifier.py: remove redundant "login" alternative from regex since "log\s*in" already covers it (S5855) - test_memory_monitor.py: use pytest.approx() for float comparisons (S1244) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/backend/core/error_classifier.py`:
- Around line 43-107: The _BILLING_PATTERNS list is too broad—generic patterns
like re.compile(r"billing", ...) and re.compile(r"credit", ...) cause false
positives; update the _BILLING_PATTERNS constant to remove those generic entries
and replace them with more specific expressions (e.g. match "payment required"
and numeric 402, "insufficient funds", "quota exceeded", "credit card
(declined|expired|invalid|limit)", "billing (issue|error|required)" or "card
(declined|expired)" ) so only clear billing/payment failure messages are caught;
modify the list in error_classifier.py (symbol: _BILLING_PATTERNS) and keep
existing specific patterns like r"\b402\b" and "insufficient\s+funds" unchanged.
In `@apps/backend/core/json_recovery.py`:
- Around line 109-111: The condition `not escape_next` in the string-toggle
branch is redundant because `escape_next` is always False when reached; update
the `if ch == '"' and not escape_next:` check inside the loop in
json_recovery.py to simply `if ch == '"' :` and toggle `in_string` accordingly
(preserve the existing `continue` behavior), keeping the existing escape
handling logic (the earlier `if escape_next: continue` and the backslash
handling) intact so behavior doesn't change.
- Around line 159-172: The try/except blocks around json.loads(repaired) and
json.loads(extracted) are dead because _repair_json_syntax and
_extract_json_block only return strings that already passed json validation;
remove the unnecessary try/except and directly return (json.loads(repaired),
"tier2_repair") and (json.loads(extracted), "tier3_extract") respectively, or if
you prefer defensive coding replace the except branches with a single
comment/assertion noting the upstream validation; target the json.loads calls
and the surrounding try/except in the tier-2 and tier-3 sections and adjust
accordingly.
…allback refs, accessibility - Enforce _MAX_INPUT_SIZE guard at parse_json_with_recovery() entry point - Truncate PR review body to 65536 chars (GitHub API limit) - Fix conflicting worktree isolation vs project-root instructions in prompts - Add aria-label for collapsed Settings button (accessibility) - Use GIT_PUSH_TIMEOUT for auto-push consistency - Wrap progress_callback in try/except to prevent merge abort - Convert GenerationProgressScreen scroll to callback ref pattern - Optimize useGitHubPRs polling deps with useMemo(hasOpenPRs) - Extract _GC_MESSAGE_INTERVAL constant, update docstrings - Remove unused imports in test files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/prompts_pkg/prompts.py (1)
276-312: 🧹 Nitpick | 🔵 TrivialAvoid parsing the warning string to derive
worktree_root.The regex couples logic to presentation; any copy change can break extraction and incorrectly fall back to
spec_dir. Prefer returning structured data from the helper.♻️ Suggested refactor
-def _get_worktree_isolation_warning(spec_dir: Path) -> str: +def _get_worktree_isolation_info(spec_dir: Path) -> tuple[str | None, str]: ... - if not worktree_root or not parent_project: - return "" + if not worktree_root or not parent_project: + return None, "" ... - return f"""## 🔒 WORKTREE ISOLATION — READ THIS CAREFULLY + warning = f"""## 🔒 WORKTREE ISOLATION — READ THIS CAREFULLY ... -""" +""" + return worktree_root, warning- isolation_warning = _get_worktree_isolation_warning(spec_dir) + worktree_root, isolation_warning = _get_worktree_isolation_info(spec_dir) - if isolation_warning: - import re as _re - _wt_match = _re.search(r"Worktree root:\*\* `([^`]+)`", isolation_warning) - worktree_root = _wt_match.group(1) if _wt_match else str(spec_dir) + if isolation_warning: + if not worktree_root: + worktree_root = str(spec_dir) project_root_note = ( f"The project root (your worktree) is `{worktree_root}`. " "All code goes in the worktree root, not in the spec directory." )
🤖 Fix all issues with AI agents
In `@apps/backend/agents/session.py`:
- Around line 70-76: The module-level ErrorClassifier (_error_classifier) is
shared across sessions and its internal state (e.g., _recent_responses deque and
use of reset()) causes cross-session interference; change this to a
session-scoped instance by removing/stop-using the module singleton
_error_classifier and instead instantiate ErrorClassifier inside
run_agent_session (or the session bootstrap path), assign to a local variable
(e.g., error_classifier) and replace all references to _error_classifier with
that local variable so each session has its own classifier while leaving
_api_circuit_breaker and _memory_monitor as shared singletons.
In `@apps/backend/core/memory_monitor.py`:
- Around line 79-89: The debug log in maybe_gc() currently calls get_usage_mb()
after gc.collect(), which reports post-GC usage and hides the value that
triggered the collection; change maybe_gc() to capture pre_gc =
self.get_usage_mb() before calling gc.collect(), then call gc.collect(),
optionally capture post_gc = self.get_usage_mb(), and update the logger.debug
message to include both pre_gc and self._max_memory_mb (and post_gc if desired)
so the log shows the usage that triggered GC (use function names maybe_gc,
should_gc, get_usage_mb and the attribute _max_memory_mb to locate and update
the code).
In `@apps/backend/prompts_pkg/prompts.py`:
- Around line 214-235: The logic that builds spec_dir_str and searches for
worktree_markers can miss matches when spec_dir is relative (making
parent_project empty); before creating spec_dir_str, resolve spec_dir to an
absolute path (e.g., call Path(spec_dir).resolve() or equivalent) so the marker
search sees the full path; then continue to create spec_dir_str, run the
existing marker loop (worktree_markers, worktree_root, parent_project) and
return as before.
In `@tests/test_circuit_breaker.py`:
- Around line 33-34: The test currently asserts the private attribute
cb._failure_count; change it to assert observable behavior instead by verifying
cb.state == CircuitState.CLOSED and cb.can_execute() is True after a successful
call (or after the operation that should reset failures), so replace the
private-attribute assertion with assertions on CircuitBreaker.can_execute() and
the public state property (using CircuitState) to avoid coupling to internals.
apps/backend/prompts_pkg/prompts.py
Outdated
Comment on lines
+214
to
+235
| spec_dir_str = str(spec_dir).replace("\\", "/") | ||
|
|
||
| # Detect worktree patterns | ||
| worktree_markers = [ | ||
| ".auto-claude/worktrees/tasks/", | ||
| ".worktrees/", | ||
| ] | ||
|
|
||
| worktree_root = None | ||
| parent_project = None | ||
| for marker in worktree_markers: | ||
| idx = spec_dir_str.find(marker) | ||
| if idx != -1: | ||
| parent_project = spec_dir_str[:idx].rstrip("/") | ||
| # Worktree root is the directory immediately under the marker | ||
| after_marker = spec_dir_str[idx + len(marker) :] | ||
| worktree_name = after_marker.split("/")[0] if after_marker else "" | ||
| worktree_root = spec_dir_str[: idx + len(marker)] + worktree_name | ||
| break | ||
|
|
||
| if not worktree_root or not parent_project: | ||
| return "" |
Contributor
There was a problem hiding this comment.
Resolve spec_dir before marker search to avoid missing worktree detection.
If spec_dir is relative and starts with the marker, parent_project becomes empty and the warning is skipped. Resolving to an absolute path prevents that false negative.
🔧 Proposed fix
- spec_dir_str = str(spec_dir).replace("\\", "/")
+ spec_dir_str = spec_dir.resolve(strict=False).as_posix()🤖 Prompt for AI Agents
In `@apps/backend/prompts_pkg/prompts.py` around lines 214 - 235, The logic that
builds spec_dir_str and searches for worktree_markers can miss matches when
spec_dir is relative (making parent_project empty); before creating
spec_dir_str, resolve spec_dir to an absolute path (e.g., call
Path(spec_dir).resolve() or equivalent) so the marker search sees the full path;
then continue to create spec_dir_str, run the existing marker loop
(worktree_markers, worktree_root, parent_project) and return as before.
Comment on lines
+33
to
+34
| assert cb._failure_count == 0 | ||
| assert cb.state == CircuitState.CLOSED |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid asserting on private attributes in tests.
Asserting cb._failure_count == 0 couples the test to internal implementation details. Consider testing observable behavior instead (e.g., verifying state remains CLOSED and can_execute() returns True after success).
♻️ Suggested improvement
def test_success_resets_failures(self):
cb = CircuitBreaker("test", failure_threshold=3)
cb.record_failure(Exception("err"))
cb.record_failure(Exception("err"))
cb.record_success()
- assert cb._failure_count == 0
+ # Verify observable behavior: failures cleared, state remains CLOSED
+ # Recording more failures should not immediately trip the breaker
+ cb.record_failure(Exception("err"))
+ cb.record_failure(Exception("err"))
assert cb.state == CircuitState.CLOSED
+ assert cb.can_execute() is True🤖 Prompt for AI Agents
In `@tests/test_circuit_breaker.py` around lines 33 - 34, The test currently
asserts the private attribute cb._failure_count; change it to assert observable
behavior instead by verifying cb.state == CircuitState.CLOSED and
cb.can_execute() is True after a successful call (or after the operation that
should reset failures), so replace the private-attribute assertion with
assertions on CircuitBreaker.can_execute() and the public state property (using
CircuitState) to avoid coupling to internals.
…scoped classifier, GC logging - Narrow billing patterns to avoid false positives (e.g. "billing issue" not bare "billing") - Make ErrorClassifier session-scoped instead of module singleton (prevents cross-session state leak) - Log pre-GC memory usage in maybe_gc() to show what triggered collection - Remove redundant `not escape_next` check in JSON block extraction - Remove dead try/except around tier-2/tier-3 json.loads (upstream already validates) - Return structured (worktree_root, warning) tuple from _get_worktree_isolation_info instead of regex-parsing warning string - Use ref for isUserScrolledUp in Insights auto-scroll effect to avoid re-triggering on manual scroll Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/agents/session.py (1)
1307-1378: 🧹 Nitpick | 🔵 TrivialConsider adding resilience pre-checks to
run_agent_session_isolatedfor consistency.The standard
run_agent_sessionperforms memory pressure and circuit breaker checks before starting (lines 924-942), butrun_agent_session_isolateddoes not. While the isolated session runs in a subprocess with its own resource limits, adding these pre-checks would:
- Fail fast if API is unhealthy (circuit breaker OPEN)
- Avoid spawning subprocesses under critical memory pressure
♻️ Optional: Add pre-checks for consistency
# Initialize recovery manager for automatic crash recovery recovery_manager = RecoveryManager(spec_dir=spec_dir, project_dir=project_dir) + # Check memory pressure before spawning subprocess + pressure = _memory_monitor.check_pressure() + if pressure == MemoryPressure.CRITICAL: + msg = "Cannot start isolated session: memory pressure is CRITICAL" + debug_error("session", msg, usage_mb=_memory_monitor.get_usage_mb()) + return "error", msg, None + + # Check circuit breaker + if not _api_circuit_breaker.can_execute(): + msg = ( + f"API circuit breaker is OPEN ({_api_circuit_breaker.name}). " + "Too many consecutive failures — waiting for recovery." + ) + debug_error("session", msg) + return "error", msg, None + # Initialize process isolator with resource limits isolator = AgentProcessIsolator(project_dir=project_dir, limits=limits)
🤖 Fix all issues with AI agents
In `@apps/backend/prompts_pkg/prompts.py`:
- Around line 230-236: The logic building worktree_root can produce an invalid
path when worktree_name is empty; update the code around the computation of
after_marker/worktree_name/worktree_root to guard against an empty worktree_name
(derived from spec_dir_str and marker). Specifically, after computing
after_marker and worktree_name, only construct worktree_root = spec_dir_str[:
idx + len(marker)] + worktree_name if worktree_name is non-empty; if
worktree_name is empty, skip this branch (e.g., continue the loop or return
(None, "") depending on surrounding flow) so you don't treat the container path
as a valid worktree. Ensure the check that follows uses the validated
worktree_root/worktree_name variables.
- Add memory pressure + circuit breaker pre-checks to run_agent_session_isolated for consistency with run_agent_session - Skip worktree marker match when worktree_name is empty to avoid invalid paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This was referenced Feb 15, 2026
This was referenced Feb 22, 2026
Merged
Merged
This was referenced Mar 3, 2026
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
Seven low-effort, high-value improvements from the Phase 1 Quick Wins plan:
rename()to prevent 0-byte JSON corruption on crash. Applied to settings, project store, plan files, session storage, and profile storage.-uflag for upstream tracking. Non-fatal warning on failure.cd-ing to the parent project.Test plan
tsc --noEmit)vitest run)ruff check+ruff format --check)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests