docs(plans): add benchmark-verified optimization plans from full-app analysis (checkpoint 1/2)#350
Conversation
…analysis (checkpoint 1/2) 11 self-contained implementation plans, each verified against the real code and (where measurable) backed by module-level benchmarks run in this container. plans/ is gitignored; force-added on this branch so the analysis survives the ephemeral remote environment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHPY8k8d3TS4BhGVtGMTgC
…2/2) Adds the remaining 17 implementation plans and a README index. Every performance claim was benchmarked against the real implementation in this container (A/B against a prototyped fix where possible); 5 plausible findings were rejected after measurement and are recorded in the README so they are not re-proposed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHPY8k8d3TS4BhGVtGMTgC
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHPY8k8d3TS4BhGVtGMTgC
✅ Deploy Preview for grand-selkie-225f87 canceled.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Escape literal NULs in Markdown snippets
This snippet embeds actual NUL bytes in a .md file rather than escaped text like \0 or \u0000. As a result, Git and ripgrep classify the plan as binary (git diff --numstat shows - -, and rg only reports “binary file matches”), which hides future review diffs and makes the implementation plan hard to search. The same literal-NUL pattern appears in plans/17-terminaltabs-subscribes-to-whole-agentstatuses-age.md, so these delimiters should be represented with escapes in the Markdown source.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ```rust | ||
| #[tauri::command] | ||
| #[tracing::instrument(skip_all)] | ||
| pub async fn write_to_pty( |
There was a problem hiding this comment.
Converting write_to_pty into an async command here means each terminal input event is handed to an independent spawn_blocking task. Terminal.tsx fires writeToPty from term.onData without awaiting the previous call, so paste/typing bursts can have multiple in-flight tasks, and those workers can acquire the per-session writer mutex in a different order than IPC arrival, transposing input bytes. The current sync handler serialized writes; if this is offloaded, it needs a single per-session queue/worker rather than one blocking task per command.
Useful? React with 👍 / 👎.
| .take_writer() | ||
| .map_err(|e| AppError::PtyError(e.to_string()))?; | ||
|
|
||
| let (input_tx, input_rx) = std::sync::mpsc::channel::<Vec<u8>>(); |
There was a problem hiding this comment.
This uses an unbounded std::sync::mpsc channel for PTY input. When the child stops reading, the writer thread blocks after the kernel PTY buffer fills, but the sync handler keeps accepting paste/onData chunks and tx.send(data.to_vec()) queues all remaining input in memory. That changes the freeze into unbounded RSS growth for one wedged session, despite the plan later describing the leak as bounded; use a bounded queue with an explicit full-queue policy.
Useful? React with 👍 / 👎.
| tauri::async_runtime::spawn_blocking(move || { | ||
| if let Ok(mut conn) = db.lock() { | ||
| repo::pty::update_dimensions(&mut conn, &session_id, cols, rows); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This fire-and-forget task creates one DB update per resize, while the frontend does not await resizePty. During rapid resizes, older tasks can acquire the DB mutex after newer tasks and overwrite rows/cols with stale dimensions; before this change the synchronous handler serialized those writes in IPC order. The ioctl can stay inline, but persisted dimensions need last-write-wins ordering, such as coalescing through one worker or guarding updates with a sequence.
Useful? React with 👍 / 👎.
| for path in status_z_paths(status_z) { | ||
| match std::fs::symlink_metadata(Path::new(folder).join(&path)) { | ||
| Ok(md) => { | ||
| md.len().hash(&mut hasher); | ||
| md.modified() |
There was a problem hiding this comment.
The proposed fingerprint hashes status bytes, HEAD, and worktree lstat data only. If the index changes while the worktree metadata and status code/path stay the same, such as a cached patch or plumbing update to an already-modified path, diff_snapshot would return the cached diff even though the temp index copied by compute_diff_snapshot_uncached would produce different staged changes. Include the real index mtime/hash or another staged-content signal in the cache key.
Useful? React with 👍 / 👎.
| function useGitDiff(profileId: string, isVisible = true) { | ||
| return useSuspenseQuery({ | ||
| queryKey: queryKeys.git.diff(profileId), | ||
| queryFn: () => getGitDiffSnapshot({ profileId }), | ||
| select: (snapshot) => snapshot.diff, | ||
| staleTime: GIT_DIFF_SNAPSHOT_STALE_MS, | ||
| refetchInterval: isVisible ? GIT_LIGHT_REFRESH_INTERVAL_MS : false, | ||
| }); |
There was a problem hiding this comment.
Passing isVisible only to refetchInterval stops the 10s timer, but the hidden SidebarGitPanel remains mounted with an active useSuspenseQuery under queryKeys.git.diff(profileId). The watcher still calls invalidateQueries for that key on every file event, and active queries refetch on invalidation, so a hidden profile in git mode will still run the full diff during active editing. To actually drop hidden-profile work to zero, avoid mounting the full-diff query while hidden or otherwise make the query inactive, not just interval-free.
Useful? React with 👍 / 👎.
| - `getPtySessionHistory` either disappears from / is harmlessly mistyped in `src/generated/commands.ts` — nothing imports it anymore (also fine to add both raw commands to `plugins.typegen.excludePatterns` in `tauri.conf.json` if the generator errors on `tauri::ipc::Response`). | ||
| - `RestoreResult` in `src/generated/types.ts` becomes `{ newSessionId: string; historyLen: number }`. | ||
|
|
||
| If typegen cannot run in the working environment (it may require a full app build; CI containers lack GTK), hand-edit the local **gitignored** copies of `src/generated/commands.ts` and `src/generated/types.ts` to match the shapes above so `tsc` passes, and note in the PR that dev machines must regenerate. (The "do not edit generated files" rule exists because they are regenerated — a stale local copy that breaks the type-check is worse.) |
There was a problem hiding this comment.
This fallback tells implementers to edit src/generated, but the repo instructions explicitly mark that directory as generated/DO NOT EDIT and it is gitignored, so those local edits will not be committed or evaluated and can mask the real problem until a clean checkout or a regenerated binding is used. If tauri-typegen cannot represent tauri::ipc::Response, the plan should require excluding those raw commands and using the hand-written terminal helper, or regenerating on a capable machine, rather than changing generated files by hand.
Useful? React with 👍 / 👎.
2 participants
11 self-contained implementation plans, each verified against the real
code and (where measurable) backed by module-level benchmarks run in
this container. plans/ is gitignored; force-added on this branch so the
analysis survives the ephemeral remote environment.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01DHPY8k8d3TS4BhGVtGMTgC