feat: add Kitty follow integration#341
Conversation
|
@codex review |
Greptile SummaryThis PR adds a Kitty terminal follow integration: a Python watcher (
Confidence Score: 3/5The core new feature works correctly for the happy path, but The src/kitty/sync.ts — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant KW as hunk-follow.py
participant KC as hunk kitty sync
participant KK as kitten @ ls
participant HD as Hunk daemon
participant HS as Hunk session
Note over KW: on_focus_change fires
KW->>KW: debounce check
KW->>KC: spawn hunk kitty sync --window-id N
KC->>KK: "spawnSync kitten @ ls"
KK-->>KC: KittyOsWindow[] JSON
KC->>KC: resolveActiveKittyPane
alt stale event
KC-->>KW: noop kitty-window-not-active
end
KC->>HD: listSessions()
alt daemon unavailable
KC-->>KW: noop no-marked-hunk-session
end
KC->>KC: isActiveHunkWindow check
KC->>KC: detectVcs(pane.cwd)
alt not a repo
KC-->>KW: noop not-a-repo
end
KC->>KC: selectKittyFollowTarget
KC->>HD: reloadSession with new sourcePath
HD->>HS: reload diff from new repo root
HD-->>KC: ReloadResult
KC-->>KW: status reloaded
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/kitty/sync.ts:233-238
`isHunkCommand` scans every argument in `cmdline`, not just the executable. This means any foreground process whose argument list contains an element with basename `hunk` — e.g., `["vim", "/projects/hunk"]` or `["cat", "/home/user/hunkdiff"]` — will be treated as an active Hunk window and suppress the sync silently. Users of this project who edit the `hunk` binary/source in a side pane will hit this constantly. The `matchesRegisteredHunkWindow` check already handles the registered case (including `bun hunk diff`), so the fallback only needs to check `cmdline[0]`.
```suggestion
function isHunkCommand(cmdline: string[]) {
if (cmdline.length === 0) return false;
const name = basename(cmdline[0]).toLowerCase();
return name === "hunk" || name === "hunkdiff" || name.startsWith("hunkdiff-");
}
```
### Issue 2 of 3
src/kitty/sync.ts:328-332
When the daemon HTTP client throws (e.g., daemon not running, connection refused), the catch block returns `"no-marked-hunk-session"` — a reason that implies the session list was fetched and came back empty. In practice this makes it impossible to distinguish "no follow sessions registered" from "can't reach the daemon at all", which complicates debugging silently failed syncs.
```suggestion
try {
sessions = await client.listSessions();
} catch {
return { status: "noop", reason: "daemon-unavailable", windowId: input.windowId };
}
```
### Issue 3 of 3
kitty/hunk-follow.py:12
**Unbounded growth of `_last_sync_at_by_window`**
`_last_sync_at_by_window` is a module-level dict that accumulates one entry per unique `window.id` but never evicts stale entries. Kitty recycles integer window IDs within a session, so the actual risk of unbounded growth is low — but there is no `on_close` hook to clean up closed windows. Over a long Kitty session with many windows, this will hold a non-trivial number of stale float entries. Registering an `on_close` callback (`def on_close(boss, window): _last_sync_at_by_window.pop(int(window.id), None)`) would keep the dict bounded.
Reviews (1): Last reviewed commit: "feat: add Kitty follow integration" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isHunkCommand(cmdline: string[]) { | ||
| return cmdline.some((part) => { | ||
| const name = basename(part).toLowerCase(); | ||
| return name === "hunk" || name === "hunkdiff" || name.startsWith("hunkdiff-"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
isHunkCommand scans every argument in cmdline, not just the executable. This means any foreground process whose argument list contains an element with basename hunk — e.g., ["vim", "/projects/hunk"] or ["cat", "/home/user/hunkdiff"] — will be treated as an active Hunk window and suppress the sync silently. Users of this project who edit the hunk binary/source in a side pane will hit this constantly. The matchesRegisteredHunkWindow check already handles the registered case (including bun hunk diff), so the fallback only needs to check cmdline[0].
| function isHunkCommand(cmdline: string[]) { | |
| return cmdline.some((part) => { | |
| const name = basename(part).toLowerCase(); | |
| return name === "hunk" || name === "hunkdiff" || name.startsWith("hunkdiff-"); | |
| }); | |
| } | |
| function isHunkCommand(cmdline: string[]) { | |
| if (cmdline.length === 0) return false; | |
| const name = basename(cmdline[0]).toLowerCase(); | |
| return name === "hunk" || name === "hunkdiff" || name.startsWith("hunkdiff-"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/kitty/sync.ts
Line: 233-238
Comment:
`isHunkCommand` scans every argument in `cmdline`, not just the executable. This means any foreground process whose argument list contains an element with basename `hunk` — e.g., `["vim", "/projects/hunk"]` or `["cat", "/home/user/hunkdiff"]` — will be treated as an active Hunk window and suppress the sync silently. Users of this project who edit the `hunk` binary/source in a side pane will hit this constantly. The `matchesRegisteredHunkWindow` check already handles the registered case (including `bun hunk diff`), so the fallback only needs to check `cmdline[0]`.
```suggestion
function isHunkCommand(cmdline: string[]) {
if (cmdline.length === 0) return false;
const name = basename(cmdline[0]).toLowerCase();
return name === "hunk" || name === "hunkdiff" || name.startsWith("hunkdiff-");
}
```
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| sessions = await client.listSessions(); | ||
| } catch { | ||
| return { status: "noop", reason: "no-marked-hunk-session", windowId: input.windowId }; | ||
| } |
There was a problem hiding this comment.
"no-marked-hunk-session" — a reason that implies the session list was fetched and came back empty. In practice this makes it impossible to distinguish "no follow sessions registered" from "can't reach the daemon at all", which complicates debugging silently failed syncs.
| try { | |
| sessions = await client.listSessions(); | |
| } catch { | |
| return { status: "noop", reason: "no-marked-hunk-session", windowId: input.windowId }; | |
| } | |
| try { | |
| sessions = await client.listSessions(); | |
| } catch { | |
| return { status: "noop", reason: "daemon-unavailable", windowId: input.windowId }; | |
| } |
Would you prefer to keep the catch reason as "no-marked-hunk-session" for simplicity, or add "daemon-unavailable" to KittySyncNoopReason so callers can distinguish the two cases?
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/kitty/sync.ts
Line: 328-332
Comment:
When the daemon HTTP client throws (e.g., daemon not running, connection refused), the catch block returns `"no-marked-hunk-session"` — a reason that implies the session list was fetched and came back empty. In practice this makes it impossible to distinguish "no follow sessions registered" from "can't reach the daemon at all", which complicates debugging silently failed syncs.
```suggestion
try {
sessions = await client.listSessions();
} catch {
return { status: "noop", reason: "daemon-unavailable", windowId: input.windowId };
}
```
Would you prefer to keep the catch reason as `"no-marked-hunk-session"` for simplicity, or add `"daemon-unavailable"` to `KittySyncNoopReason` so callers can distinguish the two cases?
How can I resolve this? If you propose a fix, please make it concise.| from typing import Any | ||
|
|
||
|
|
||
| _last_sync_at_by_window: dict[int, float] = {} |
There was a problem hiding this comment.
_last_sync_at_by_window
_last_sync_at_by_window is a module-level dict that accumulates one entry per unique window.id but never evicts stale entries. Kitty recycles integer window IDs within a session, so the actual risk of unbounded growth is low — but there is no on_close hook to clean up closed windows. Over a long Kitty session with many windows, this will hold a non-trivial number of stale float entries. Registering an on_close callback (def on_close(boss, window): _last_sync_at_by_window.pop(int(window.id), None)) would keep the dict bounded.
Prompt To Fix With AI
This is a comment left during a code review.
Path: kitty/hunk-follow.py
Line: 12
Comment:
**Unbounded growth of `_last_sync_at_by_window`**
`_last_sync_at_by_window` is a module-level dict that accumulates one entry per unique `window.id` but never evicts stale entries. Kitty recycles integer window IDs within a session, so the actual risk of unbounded growth is low — but there is no `on_close` hook to clean up closed windows. Over a long Kitty session with many windows, this will hold a non-trivial number of stale float entries. Registering an `on_close` callback (`def on_close(boss, window): _last_sync_at_by_window.pop(int(window.id), None)`) would keep the dict bounded.
How can I resolve this? If you propose a fix, please make it concise.1 participant
No description provided.