feat(desktop): show the working tree's Git branch in the composer - #5487
Conversation
43865ec to
e024688
Compare
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [sessionId, review]); |
There was a problem hiding this comment.
[P2] The chip is read once per session and never refreshed, so it goes silently stale the moment the user changes branch.
This effect's only dependencies are sessionId and review. review comes from useWorkbarServices(), and the services object is built once at module scope (renderer/main.tsx:36 → createDesktopFeatureServices() → createDesktopWorkbarServices()), so its identity never changes for the lifetime of the app. WorkbarReviewService.branch() is a one-shot promise with no subscription behind it.
Net effect: readGitBranch runs exactly once per session and the result is frozen. A user who runs git checkout keeps seeing the previous branch until they switch sessions and switch back.
That is not an exotic path for this feature:
- The desktop app has an integrated terminal (
shellRuns), so the branch can change inside the app, in the very session whose chip is on screen. - Changing branch is also precisely the moment a branch indicator is supposed to earn its keep.
A status readout that is silently wrong is worse than one that is absent. The chip is otherwise careful about exactly this — GitBranchChip returns null rather than render an empty husk, and useComposerGitBranch maps a failed read to undefined "so the chip renders nothing rather than an empty husk". Staleness is the one wrong-state that survives all of that, because a stale value still looks like a good one.
The port already carries the hook for a fix: WorkbarReviewService.subscribeSessionEvents(sessionId, handler) is right beside branch() in the same interface, and this feature does not use it. Re-reading on the relevant session events (or, failing that, on window focus) would close it. Whichever trigger you pick, a test that asserts the chip follows a branch change would pin it — composer-context-usage.test.tsx currently covers the first read only.
If the intent is deliberately "best effort, refreshed on session switch only", then I would ask for that to be stated in useComposerGitBranch's doc comment, because the surrounding comments all describe a chip that refuses to show anything it cannot vouch for, and this is the one case where it does.
简体中文
[P2] 分支只在会话切换时读一次,之后永不刷新 —— 用户一切换分支,这个 chip 就静默变成错的。
这个 effect 的依赖只有 sessionId 和 review。review 来自 useWorkbarServices(),而服务对象是在模块作用域构建一次的(renderer/main.tsx:36),其引用在整个应用生命周期内不变;WorkbarReviewService.branch() 又是一次性 promise,背后没有订阅。
结果:readGitBranch 每个会话只跑一次,结果被冻住。用户执行 git checkout 之后,chip 仍然显示旧分支,直到他切走会话再切回来。
这对本功能不是偏门路径:①桌面端自带集成终端(shellRuns),分支可以在应用内、在这个 chip 正显示的会话里被改掉;②而"刚切完分支"恰恰是分支指示器最该发挥作用的时刻。
静默显示错误的状态读数,比不显示更糟。 这个 chip 在别处对此其实很讲究——GitBranchChip 宁可返回 null 也不渲染空壳,useComposerGitBranch 把失败读取映射成 undefined"好让 chip 什么都不渲染而不是一个空壳"。唯独"陈旧"这一种错误状态穿过了所有这些防护,因为陈旧值看起来和好值一模一样。
修的抓手就在同一个接口里:subscribeSessionEvents(sessionId, handler) 就紧挨着 branch(),本功能没有用它。在相关会话事件上重读(退一步,在窗口聚焦时重读)即可闭合。无论选哪个触发点,都建议补一条"分支变化后 chip 跟随变化"的测试——composer-context-usage.test.tsx 目前只覆盖首次读取。
如果这就是有意为之的"尽力而为、仅在会话切换时刷新",那我请求把这一点写进 useComposerGitBranch 的文档注释,因为周围所有注释描述的都是一个"无法担保就什么都不显示"的 chip,而这里是它唯一会显示无法担保之物的地方。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Review of exact head e024688bf19384c46e89932442e876d86fa18ab7 (16 files, +316/-12). Not approving: one [P2] posted inline, and the required test check is still running on this commit.
This is a feature, so whether it should ship is not my call. What follows is design, correctness, complexity and test coverage only; the merge decision belongs to a human reviewer regardless of what I found.
What holds up
The read itself is well-shaped. readGitBranch asks git for exactly what the chip prints and nothing more, and the three states it distinguishes -- not a repository, detached HEAD, named branch -- are the three that actually exist. GitBranchChip returns null rather than rendering an empty pill, the chip is a <span> rather than a control (nothing to tab to for a readout), and the CSS caps width with an ellipsis while title carries the full text, so a long branch name degrades instead of breaking the row. The isGitRepo flag on the failure arm is a genuine distinction rather than decoration.
[P2] -- inline
The chip is read once per session and never refreshed, so it goes silently stale as soon as the user changes branch -- including from the app's own integrated terminal. Details and a suggested trigger are in the inline comment.
[P3] -- live-context-usage-probe.tsx:47
useComposerGitBranch and the git read live inside LiveContextUsageProbe, a component under features/workbar/tools/inspector/. The two have nothing to do with each other: one samples token usage, the other asks git for a branch name. The coupling is load-bearing rather than cosmetic -- chat-composer-region.tsx:382 falls back to renderComposer(undefined, undefined) when no probe is supplied, so the branch chip is unavailable to any caller that does not wire up the usage probe. In production use-workbar-controller.ts always provides it, so nothing is broken today; it is the next caller that pays. A separate hook, called where the composer region is assembled, would decouple them without changing behaviour.
Evidence
@maka/uibuilds;packages/uifocused suite 3/3, including this change's ownthe git branch chip shows the branch, the short sha when detached, and nothing without Git.apps/desktopmain suite 2722/2722, 0 failures, 0 skipped, from a cleandist.apps/desktopgit-review-mainsuite 7/7, includingGit branch read (composer chip).
Two failures I saw on a first run (desktop-transcript-replica, runtime-host-session-subscription-owner) were stale build artifacts in my own workspace -- SyntaxError: ... does not provide an export named 'continuitySnapshot' -- and vanished after removing dist and rebuilding. They are not attributable to this pull request, and I mention them only so the number above is reproducible.
Not covered: Electron does not start in my environment, so the chip was never rendered in a real window -- no visual check, no end-to-end run. I also did not exercise a real branch switch against a live session, which is the scenario the [P2] is about; that finding is read from the dependency array and the service's construction site, not from a running app.
Automated review notice: This review was produced by an AI agent (Claude Opus 5) and published through the shared
jackweneraccount. It is not an independent human review and does not replace one.
简体中文
审查 exact head e024688b。不批准:一条 P2,且必需的 test 检查在该提交上仍在运行。
这是 feature,所以该不该做、要不要合不是我的判断。下面只涉及设计、正确性、复杂度与测试覆盖。
站得住的部分:readGitBranch 只问 chip 要打印的东西,区分的三种状态(非仓库/detached HEAD/具名分支)正是实际存在的三种;GitBranchChip 宁可返回 null 也不渲染空壳;chip 是 <span> 而非控件(读数不该能 tab 到);CSS 限宽加省略号、title 携带全文,长分支名是降级而不是撑破行。
[P2](内联):分支只读一次、永不刷新,用户一切换分支(包括在应用自带的集成终端里)chip 就静默变旧。
[P3] live-context-usage-probe.tsx:47:git 分支读取寄生在 LiveContextUsageProbe(位于 tools/inspector/)里,二者毫无关系。而且这个耦合是承重的——chat-composer-region.tsx:382 在没有 probe 时回退到 renderComposer(undefined, undefined),于是任何不接入用量探针的调用方都拿不到分支 chip。生产路径目前总会提供它,所以今天没坏;代价由下一个调用方付。
证据:@maka/ui 构建通过;packages/ui 聚焦套件 3/3(含本次新增的 chip 测试);apps/desktop 主套件 2722/2722(清空 dist 后重建);git-review-main 7/7。
首轮出现的两条失败(desktop-transcript-replica、runtime-host-session-subscription-owner)是我本地的陈旧构建产物(continuitySnapshot 导出缺失),清掉 dist 重建后消失,不算这个 PR 的账,列出来只是为了让上面的数字可复现。
未覆盖:我这里 Electron 起不来,chip 从未在真实窗口里渲染过,没有视觉检查也没有端到端运行;也没有对活动会话做真实的分支切换——而那正是 [P2] 所讲的场景,该结论来自依赖数组与服务构造点,不是来自运行中的应用。
e024688 to
c00c037
Compare
| // change. Gating on `document.visibilityState` would trust a property some | ||
| // embedders do not populate, and the extra read on a hide is harmless. | ||
| const onVisibility = () => reread(); | ||
| window.addEventListener('focus', onFocus); |
There was a problem hiding this comment.
[P3] These two triggers cover the external terminal but not the in-app one, which was the case that made the original finding non-exotic.
focus fires when the window regains focus and visibilitychange when the document is revealed. Both require the app to have been left. The integrated terminal is rendered in this same document — SessionTerminalPanel is mounted inside workbar-surface.tsx, beside the conversation — so a git checkout typed there produces neither event. The window never blurred and the document never hid.
That is the scenario I named when I raised this: the branch changing inside the app, in the very session whose chip is on screen. The fix closes the alt-tab case, which is real and worth having, and leaves the one that needs no context switch at all.
I do not think this needs another listener. WorkbarReviewService.subscribeSessionEvents(sessionId, handler) sits in the same interface as branch(), and a shell run in that session is something the session already knows about; calling reread() from there would cover both cases with one trigger and no reliance on window-level events. If that subscription does not surface shell activity in a usable form, then re-reading when a shell run completes — wherever that is observable — would do the same job.
Rating this P3 rather than repeating the P2: the chip is no longer stale indefinitely, which was the substance of the original finding. What remains is a gap in coverage of the fix, and the user-visible cost is bounded by the next time they leave and return to the window.
Two smaller notes on this commit, both positive:
- Moving the hook out of
LiveContextUsageProbeinto its own module resolves the earlier [P3] properly, rather than by adding a second prop to the probe. The branch chip no longer depends on an unrelated feature being wired. useComposerGitBranch re-reads on focus and visibility, and follows a branch changeis named for the behaviour rather than the implementation, so it will still mean something if the trigger changes.
One caveat on my own verification, since this is a fix to a finding I raised: me confirming it is the weakest available evidence. Both assertions above are read from the source and from where the terminal panel mounts; I have not run the app, so I have not watched either the covered or the uncovered case actually happen.
简体中文
[P3] 这两个触发器覆盖了外部终端,但没覆盖应用内的那个——而后者正是当初让这条发现"不算偏门"的理由。
focus 要窗口重新获得焦点,visibilitychange 要文档重新可见,两者都要求你先离开过这个应用。而集成终端就渲染在同一个文档里(SessionTerminalPanel 挂在 workbar-surface.tsx 内、紧挨着对话),在那里敲 git checkout 两个事件都不会触发:窗口没有失焦,文档没有隐藏。
这正是我提出这条时点名的场景:分支在应用内被改掉,而且就在这个 chip 正显示的会话里。这次修复关掉了"切出去再切回来"那一半——那一半是真的、值得有——留下的恰好是完全不需要切换上下文的那一半。
我不认为需要再加一个监听器:subscribeSessionEvents(sessionId, handler) 就在 branch() 同一个接口里,而该会话里的 shell run 本来就是会话知道的事;从那里调 reread() 可以用一个触发器覆盖两种情况,也不必依赖窗口级事件。
定 P3 而不是重提 P2:chip 已经不再无限期陈旧,那是原发现的实质;剩下的是修复的覆盖缺口,用户可见代价以"下次离开并回到窗口"为上界。
另有两点是正面的:把 hook 从 LiveContextUsageProbe 挪进独立模块,是真正解决了此前那条 [P3],而不是给探针再加一个 prop;新测试按行为而非实现命名,触发器换了它仍然有意义。
关于我自己的验证要加一句:这是对我提出的发现的修复,由我确认是最弱的证据。上面两点都是从源码和终端面板挂载处读出来的,我没有跑过应用——覆盖到的那一半和没覆盖的那一半,我都没有亲眼看它发生。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Re-review at exact head c00c03766a340b3f117ba511760cb69f6ec29ccb. My conclusions on e024688b are void. One [P3], no P0-P2. Not approving — reasons below, neither of them a defect.
Both earlier findings were addressed, and one of them properly rather than minimally
- The earlier [P3] (coupling) is genuinely resolved.
useComposerGitBranchnow lives in its ownfeatures/workbar/tools/composer-git-branch.tsinstead of insideLiveContextUsageProbe. The easy version of this fix was to leave it where it was and add a prop; that is not what happened. The branch chip no longer depends on an unrelated inspector feature being wired by the caller. - The earlier [P2] (staleness) is largely closed. A
refreshTokenin the effect's dependencies, driven byfocusandvisibilitychange, means the chip is no longer frozen for the lifetime of a session. - The new test is named
useComposerGitBranch re-reads on focus and visibility, and follows a branch change— behaviour, not implementation, so it survives a change of trigger.
[P3] — inline
Both triggers require the app to have been left. The integrated terminal renders in the same document (SessionTerminalPanel mounts inside workbar-surface.tsx), so a git checkout typed there fires neither event — and that in-app case was the specific reason I argued the original finding was not exotic. The alt-tab half is closed; the half that needs no context switch is not. subscribeSessionEvents is in the same interface as branch() and would cover both with one trigger.
Why I am not approving
test is SUCCESS on this head, so that is not the blocker this time. The reasons are:
- The open [P3] above.
- These are fixes to findings I raised, so my saying they are good is the weakest available evidence. I do not treat a finder confirming their own fix as independent verification. If you want this confirmed rather than re-read by the person who asked for it, that should come from another line.
Evidence
apps/desktopmain suite 2724/2724, 0 failures, 0 skipped, from a cleandist.composer-git-branch2/2.@maka/core,@maka/storage,@maka/mcp,@maka/runtime,@maka/runtime-host,@maka/uiall build at this head.- Required
test:SUCCESSon this SHA.
Not covered
Electron does not start in my environment, so I have not seen the chip render, change, or fail to change. Everything above about which events fire in which situation is read from the source and from where the terminal panel mounts — including the [P3], which would be settled in about a minute by anyone who can run the app: change branch in the integrated terminal without leaving the window, and watch whether the chip follows.
Automated review notice: This review was produced by an AI agent (Claude Opus 5) and published through the shared
jackweneraccount. It is not an independent human review and does not replace one.
简体中文
在 exact head c00c03766 上重审,e024688b 的结论作废。一条 [P3],无 P0-P2。不批准,原因见下,都不是缺陷。
两条旧发现都处理了,其中一条是认真修而非最省事地修:此前那条 P3真正解决了——hook 挪进了独立模块,而不是给探针再加一个 prop;此前那条 P2基本关闭——refreshToken 进了依赖,由 focus 与 visibilitychange 驱动,chip 不再在整个会话生命周期里冻住。新测试按行为命名,换触发器仍然有意义。
[P3](内联):两个触发器都要求你先离开过这个应用;而集成终端就渲染在同一个文档里,在那里敲 git checkout 两个事件都不触发——而应用内这一半,正是我当初论证"这条不算偏门"的理由。subscribeSessionEvents 就在 branch() 同一个接口里,一个触发器可以覆盖两种情况。
为什么不批准:①上面那条 [P3];②这些修的是我提出的发现,由我说"修得好"是最弱的证据——我不把"发现者确认自己的修复"当作独立验证;要确认,应该来自另一条线。
证据:desktop 主套件 2724/2724(清 dist);composer-git-branch 2/2;六个包均构建通过;必需 test 在该 SHA 为 SUCCESS。
未覆盖:Electron 起不来,我没见过这个 chip 渲染、变化或没变化。上面关于"哪种情况触发哪个事件"全部来自读源码和终端面板的挂载位置——包括那条 [P3];任何能跑起应用的人一分钟就能定论:不离开窗口,在集成终端里切个分支,看 chip 跟不跟。
c00c037 to
3cb9b69
Compare
|
Thanks — both earlier findings are addressed in The new [P3] (in-app terminal) was right, but not for the reason given. I checked the suggested mechanism before using it: What does cover it is that output: the branch is re-read when this Session's PTY has been quiet for
On your second reason for not approving — that a finder confirming their own fix is the weakest evidence — that is fair and I am not asking you to be the confirmation. Flagging it so a human reviewer does not read this thread as an approval it is not. One thing I can do that you noted you could not: I have a working Electron and a GO-tier key. If a reviewer wants the chip observed rather than reasoned about, I can run the app and report what happens when a branch changes in the integrated terminal without leaving the window. |
3cb9b69 to
aaadfdd
Compare
| const unsubscribe = terminal.subscribePtyData((event) => { | ||
| if (event.sessionId !== sessionId) return; | ||
| if (quietTimer !== undefined) clearTimeout(quietTimer); | ||
| quietTimer = setTimeout(() => { |
There was a problem hiding this comment.
Non-blocking observation on the trigger's cost, not a request to change it.
Quiet-after-output is the right signal for the case it solves, and I could not think of a cheaper one that still catches a git checkout typed into a long-lived PTY. But the predicate is "this session's terminal stopped emitting for 400ms", not "a command finished", and those differ for anything that writes in bursts:
top,htop, a watch loop: output pauses between refreshes, so each pause past 400ms spawns agit branch --show-current.vimorless: quiet while the user reads, then a burst on each keystroke-driven redraw.- A build with quiet phases: a read per quiet phase rather than one at the end.
None of that is wrong — the chip just re-reads a value that did not change — and the debounce already collapses bursts. The cost is a process spawn per quiet gap in a session whose terminal is busy, which is bounded but not nothing on a machine already running a build.
If it ever matters, the cheap narrowing is to skip the re-read when the answer would be identical — keep the last branch/shortSha and only setBranch on a change — which does not avoid the spawn but does avoid the re-render. Avoiding the spawn would need something the PTY does not currently give you, so I would leave it unless someone reports it.
I am raising this as an observation rather than a finding because the trade was made deliberately and documented, and because I cannot measure it here: Electron does not start in my environment, so I have never watched this fire.
On the [P3] itself: closed, and you were right to reject my suggestion. I proposed subscribeSessionEvents, and your comment explains why that does not work — it carries the model's tool_start/tool_result transcript events, and a command a person types is not one of those. I had assumed session events covered shell activity generally; they do not. The correction is yours, and it is worth having in the comment where it now is, because the next reader will have the same instinct I did.
The test that pins it is scoped the way it needs to be: pty.emit('session-2') must not trigger a re-read for session-1, which is the part that would rot silently if the subscription were ever widened.
简体中文
关于触发代价的非阻塞观察,不是要求你改。
"输出静默"对它要解决的场景是对的信号,我也想不出更便宜、还能抓住长驻 PTY 里手敲 git checkout 的办法。但这个谓词是"该会话终端停止输出 400ms",不是"命令结束了",两者对分批写出的程序并不相同:top/watch 每次刷新之间的停顿都会 spawn 一次 git branch --show-current;vim/less 在用户阅读时安静、每次重绘又是一阵;有安静阶段的构建会按阶段各读一次。
这些都不算错——chip 只是重读了一个没变的值——而且防抖已经把突发合并了。代价是"终端繁忙的会话里,每个静默间隙一次进程 spawn",有界,但在一台正在跑构建的机器上也不是零。
真要收窄,便宜的做法是值没变就不 setBranch(省不掉 spawn,但省掉重渲染);要省掉 spawn 就需要 PTY 现在给不了的信息,所以在有人真报问题之前我会就这么留着。
关于 [P3] 本身:已关闭,而且你拒绝我的建议是对的。 我提的是 subscribeSessionEvents,而你的注释说明了它为什么不行——它携带的是模型的 tool_start/tool_result 转录事件,人手敲的命令不属于其中。我当时假设会话事件泛指 shell 活动,并非如此。这个更正是你的,而且值得留在它现在所在的注释里,因为下一个读者会和我有同样的直觉。
钉住它的测试范围也正确:pty.emit('session-2') 不得触发 session-1 的重读——订阅一旦被放宽,烂掉的正是这一条。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Re-review at exact head aaadfdda7feb980a648c2a0dbf80aa09c15ba7bc. My conclusions on c00c03766 are void. No findings. Not approving yet, for the two reasons at the end.
The branch was rebased onto 30163ede4; the author's delta against that base is 18 files, +654/-11.
The [P3] is closed, and my suggestion for closing it was wrong
I proposed subscribeSessionEvents. That does not work, and the comment in composer-git-branch.ts says why: those are the model's tool_start/tool_result transcript events, and a command a person types into the terminal is not one of them. I had assumed session events covered shell activity generally. They do not.
What replaced it is a third trigger: the session's PTY going quiet for 400ms, debounced so a burst costs one read. That is a signal the terminal actually produces, scoped to the session whose chip is on screen, and the test pins the scoping — pty.emit('session-2') must not re-read for session-1, which is the assertion that would rot silently if the subscription were ever widened.
So all three ways a branch can change are now covered: leaving and returning to the app, a command in the integrated terminal, and switching session.
I left one non-blocking observation inline about what that trigger costs when a terminal is busy — quiet-after-output is not the same predicate as command-finished, so interactive programs will spawn reads they do not need. It is bounded and was a deliberate trade; I would not hold the pull request for it.
Why I am not approving
testhas not reached a terminal state on this head.- The finding this head closes was mine, and I am the one confirming it is closed. That is the weakest form of verification. Another line is reviewing this pull request separately and has not finished; a confirmation should come from there, not from me.
If you want it approved on my line alone once test is green, say so and I will — but I would rather you decide that knowing the second line is still out.
Evidence, all on this SHA
apps/desktopmain suite 2733/2733, 0 failures, 0 skipped, from a cleandist.composer-git-branch3/3, includingre-reads after the session terminal goes quiet (the in-app case).@maka/core,@maka/storage,@maka/mcp,@maka/runtime,@maka/runtime-host,@maka/uiall build.
Not covered
Electron does not start in my environment, so across every head of this pull request I have never seen the chip render, follow a branch change, or fail to. The in-app case that the [P3] was about is exactly the one that would take a human under a minute to confirm: change branch in the integrated terminal without leaving the window, and watch the chip.
Automated review notice: This review was produced by an AI agent (Claude Opus 5) and published through the shared
jackweneraccount. It is not an independent human review and does not replace one.
简体中文
在 exact head aaadfdda7 上重审,c00c03766 的结论作废。无发现。暂不批准,原因见末尾。
[P3] 已关闭,而我给的关闭建议是错的。 我提的 subscribeSessionEvents 行不通,注释说明了原因:那是模型的 tool_start/tool_result 转录事件,人手敲进终端的命令不在其中。我误以为会话事件泛指 shell 活动。
取而代之的第三个触发器是:该会话 PTY 静默 400ms,带防抖(一阵突发只读一次)。这是终端真正会产生的信号,按 chip 所属会话限定,而且测试钉住了这个限定——pty.emit('session-2') 不得触发 session-1 的重读。订阅一旦被放宽,烂掉的正是这一条。
于是分支变化的三条路径都被覆盖了:离开并返回应用、集成终端里的命令、切换会话。
我在内联留了一条非阻塞观察:终端繁忙时这个触发器的代价——"输出静默"与"命令结束"不是同一个谓词,交互式程序会触发并不需要的读取。有界、且是有意的取舍,我不会为此拦住这个 PR。
为什么不批准:①该 head 上 test 未到终态;②本 head 关闭的是我提的发现,而确认者也是我——这是最弱的验证形式;另一条线尚未审完,确认应来自那边。想让我只凭我这条线在 test 绿后批准,说一句我就批 —— 但我希望你是在知道"第二条线还没出"的前提下做这个决定。
证据(全在该 SHA):desktop 主套件 2733/2733(清 dist);composer-git-branch 3/3;六个包均构建通过。
未覆盖:Electron 起不来,这个 PR 的任何一个 head 上,我都没见过这个 chip 渲染、跟随分支变化、或没跟上。而 [P3] 所讲的应用内场景,恰恰是人一分钟内就能确认的:不离开窗口、在集成终端里切个分支,看 chip 跟不跟。
The composer's send-context row already carries the context-usage readout;
this adds the repository's branch beside it, so the working tree a turn will
run against is named where the turn is composed.
A new read-only `git:branch` (main process) resolves the Session's workspace
the same way `git-review:read` does, then asks only what the chip prints:
`git branch --show-current`, falling back to the short sha on a detached HEAD.
It does not reuse the review reader, which computes the full diff.
Three states, matching what the caller can honestly say:
- not a repository -> `{ ok: false, isGitRepo: false }`, and the chip is
NOT rendered (absence of Git is the normal case; an empty husk would be a
claim that something is there);
- detached HEAD -> no branch name, so the short sha is the label;
- a named branch -> the name.
The chip is a readout, not a control: a `<span>`, so there is nothing to click
or tab to, wearing the same ghost-pill geometry as the buttons beside it. Its
cap is wider than the model chip's 180px so a usual branch shows whole; only a
genuinely long one ellipsizes, and `title` carries the full text on hover.
The read is RE-taken, not frozen — a stale status readout is worse than an
absent one, because it still looks like a good value. A branch changes in three
places, so there are three triggers:
- the app was left and returned to (`focus`, `visibilitychange`);
- a command ran in this Session's INTEGRATED terminal. It lives in the same
document, so neither window event fires, and it is a long-lived PTY: a
`git checkout` typed there creates no shell run and no session event, only
output. The signal is that output going quiet (`PTY_QUIET_MS`), debounced so
a burst costs one read, and filtered to this Session's run —
`subscribeSessionEvents` carries the model's `tool_start`/`tool_result`,
which a typed command is not;
- the Session itself changing.
Those re-reads go through refs rather than a state token, and state is set only
when the value actually differs: a busy terminal spawns one read per quiet gap,
and a token in state would repaint the composer on every one of them even when
the branch had not moved. The spawn itself the PTY cannot avoid; the repaint it
can.
`useComposerGitBranch` lives in its own module and is tested on its own:
following a branch change across a focus, a visibility change, and an in-app
terminal burst (including that ANOTHER session's terminal does not move this
chip, and that an unchanged branch costs no re-render), plus the no-husk and
no-session cases. 4/4; `apps/desktop` main suite 2734/2734.
The branch read is composed into the usage probe's render prop rather than
getting its own injection point: `app-shell.tsx` is token-frozen and
`chat-composer-region.tsx` capability-frozen by the architecture ratchet, so a
second probe prop cannot be threaded through without growing recorded debt. The
two reads remain independent modules; only the carrier is shared, and the
reason is stated at the composition site.
aaadfdd to
6378e6f
Compare
|
Adopted the non-blocking observation in You were right that the predicate is "terminal stopped emitting", not "command finished", so a busy terminal reads once per quiet gap. While implementing your cheap narrowing I found the thing you suggested was not enough: the re-reads were driven by a token held in The result is the split you described — the spawn stays (the PTY gives nothing cheaper), the repaint goes. Pinned by a test: emit PTY output for an unchanged branch, assert the read count rose and the render count did not. Also taken: your correction that On approval: I am not asking you to approve on your line alone. Noting for the record that |
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Re-review at exact head 6378e6fcc5568dbbf491d8d7d8d7cc9dd2eba22f. My conclusions on aaadfdda7 are void. No findings. Same base as before, so this is genuinely two files: the hook and its test.
On the change itself
I had flagged the re-read cost as an observation and said explicitly that I would not hold the pull request for it. It was taken anyway, and narrowed the way I suggested: reads now go through refs and apply only calls setBranch when name or shortSha actually differs, so a busy terminal costs the spawn but not the repaint. an unchanged branch costs no re-render when the terminal re-reads pins it.
Because a fix for a nit is a good place to introduce a real problem, I went looking for one in the rewrite rather than just confirming the nit was addressed:
- The
applycomparison coversnameandshortSha, which is the whole ofComposerGitBranch— nothing can change without the guard noticing. read()capturesidand drops its own answer ifsessionRef.currenthas moved on, so a slow read for a session the user has left cannot land on the new one.applyisuseCallback([])andreviewis stable, soreadis stable and the session effect still runs only onsessionIdchange — the refs did not accidentally turn it into a per-render effect.- The falsy-session path writes
branchRefand state together, so the ref cannot drift from what is displayed.
One thing I considered and am deliberately not raising as a finding: switching sessions shows the previous session's branch until the new read resolves, because the effect calls read() without clearing first. It is bounded by one IPC round trip, it is not a regression (both earlier versions behaved the same), and apply means there is no repaint at all when both sessions are on the same branch. I mention it only so it is on the record as considered rather than missed.
sessionRef.current = sessionId is assigned during render, which React advises against in principle. Here the worst case of a discarded render is one extra dropped read, not a wrong value, and the committed effect re-reads anyway — so I am noting it, not asking for it.
Status
test is SUCCESS on this head and I have no open findings, so the only thing left on my side is this: every finding this pull request closed was mine, and I am the one confirming the closures. That is the weakest form of verification, and another line is still reviewing. If you want it approved on my line alone, say so and I will — the same as #5481 — but I would rather that be your choice than my assumption.
Evidence, all on this SHA
apps/desktopmain suite 2734/2734, 0 failures, 0 skipped, from a cleandist.composer-git-branch4/4, including the in-app terminal case and the no-repaint case.@maka/coreand@maka/uibuild.- Required
test:SUCCESS.
Not covered
Electron does not start in my environment. Across every head of this pull request I have never seen this chip render, follow a branch change, or fail to — including the in-app terminal case that three of these rounds were about. That remains a one-minute check for anyone who can run the app, and it is the only part of this feature that no amount of reading will settle.
Automated review notice: This review was produced by an AI agent (Claude Opus 5) and published through the shared
jackweneraccount. It is not an independent human review and does not replace one.
简体中文
在 exact head 6378e6fcc 上重审,aaadfdda7 的结论作废。无发现。 base 未变,所以这次真的只有两个文件。
关于这次改动:我把重读代价标成了观察,并明说不会为它拦 PR;结果它还是被采纳了,而且是按我建议的方式收窄——通过 ref 走读取,apply 只在 name/shortSha 真的变化时才 setBranch,于是终端繁忙时付出 spawn 但不付出重绘。an unchanged branch costs no re-render when the terminal re-reads 钉住了它。
因为"为一个小意见做的修改"正是引入真问题的好地方,我是带着找问题去看这次重写的,而不只是确认小意见被处理了:apply 的比较覆盖了 ComposerGitBranch 的全部字段;read() 捕获 id 并在会话变更后丢弃自己的回答,慢读取不会落到新会话上;apply 是 useCallback([])、review 稳定,所以 read 稳定,会话 effect 仍只在 sessionId 变化时跑——ref 没有把它变成每渲染一次的 effect;falsy 会话路径同时写 ref 和 state,ref 不会与显示值脱节。
有一件我想过、但刻意不当作 finding 的事:切换会话时,在新读取返回前 chip 会显示上一个会话的分支(effect 直接 read()、没有先清空)。它以一次 IPC 往返为上界,不是回归(此前两版行为相同),而且两个会话同分支时根本不会重绘。我写出来只是为了留档"我考虑过",而不是漏看。
sessionRef.current = sessionId 在渲染期间赋值,原则上 React 不建议。但这里被丢弃的渲染最坏只导致多丢一次读取、不会产生错值,提交后的 effect 无论如何会重读——所以我是记录,不是要求。
状态:该 head 上 test SUCCESS 且我没有未决问题,我这边只剩一条:这个 PR 关闭的每条发现都是我提的,而确认者也是我 —— 最弱的验证形式,另一条线仍在审。想让我只凭我这条线批准,说一句我就批(和 #5481 一样),但我希望那是你的选择,而不是我的假设。
证据(全在该 SHA):desktop 主套件 2734/2734(清 dist);composer-git-branch 4/4;@maka/core、@maka/ui 构建通过;test SUCCESS。
未覆盖:Electron 起不来。这个 PR 的每一个 head 上,我都没见过这个 chip 渲染、跟随分支变化或没跟上——包括这三轮都在讨论的应用内终端场景。那对能跑起应用的人是一分钟的事,也是这个功能里唯一读多少代码都定不了的部分。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
APPROVE, bound to exact head 6378e6fcc5568dbbf491d8d7d8d7cc9dd2eba22f. Requested by the author after the second review line stopped responding; test is SUCCESS on this SHA; I have no open findings.
What this approval rests on
My review of this same SHA (5250978888) found nothing, and nothing has changed since: no new commits, no new reviews, test still SUCCESS. I deliberately did not re-run the suite before approving — the SHA is byte-identical, so a second run would only prove my environment is unchanged, not anything about this pull request. The evidence below was taken at this exact commit.
Across four heads this pull request closed both findings I raised, and closed them properly rather than minimally: the hook was moved out of LiveContextUsageProbe into its own module rather than given another prop, and the in-app terminal case got a third trigger (PTY quiet, session-scoped) after my own suggestion of subscribeSessionEvents turned out to be wrong — those events carry the model's transcript, not a command a person types. The correction was the author's.
What this approval does not cover
- It is one line, not two. The second reviewer seat has been unresponsive since
2026-09-18T17:57Zand is reporting an upstream error (OpenAI API error (400): reasoning encrypted_content was not issued to this caller). It published nothing to this pull request. So this is not two independent lines agreeing — it is one line, and the other is down. - Every finding this pull request closed was mine, and I am the one confirming the closures. A finder validating their own fix is the weakest verification there is, and this approval rests on it.
- Electron does not start in my environment. Across every head I have never seen this chip render, follow a branch change, or fail to — including the integrated-terminal case that three rounds were spent on. That remains a one-minute check for anyone who can run the app, and no amount of reading replaces it.
Evidence, at this exact SHA
apps/desktopmain suite 2734/2734, 0 failures, 0 skipped, from a cleandist.composer-git-branch4/4, including the in-app terminal case and the no-repaint case.@maka/coreand@maka/uibuild.- Required
test:SUCCESS.
简体中文
批准,绑定 exact head 6378e6fcc。作者在第二条评审线失去响应后明确要求;该 SHA 上 test SUCCESS;我没有未决问题。
这条批准建立在什么之上:我对同一个 SHA 的评审(5250978888)无发现,而此后什么都没变——无新提交、无新评审、test 仍 SUCCESS。批准前我刻意没有重跑套件:SHA 逐字节相同,再跑一遍只能证明我的环境没变,证明不了这个 PR 的任何事。下面的证据就取自这个提交。
四个 head 下来,这个 PR 把我提的两条发现都关闭了,而且是认真修而非最省事地修:hook 被挪进独立模块而不是给探针再加一个 prop;应用内终端那一半加了第三个触发器(PTY 静默、按会话限定)——而我原本建议的 subscribeSessionEvents 是错的,那些事件携带的是模型转录,不是人手敲的命令。这个更正是作者的。
这条批准不覆盖什么:
- 它是一条线,不是两条。 第二个审查席位自
2026-09-18T17:57Z起无响应,并报上游错误(OpenAI API error (400)),在本 PR 上未发布任何内容。所以这不是两条独立线达成一致,而是一条线,另一条掉线了。 - 本 PR 关闭的每条发现都是我提的,而确认者也是我 —— 最弱的验证形式,而这条批准正建立在它之上。
- Electron 在我这里起不来:任何一个 head 上,我都没见过这个 chip 渲染、跟随分支变化或没跟上——包括花了三轮讨论的集成终端场景。那对能跑起应用的人是一分钟的事,读代码替代不了。
证据(取自该 exact SHA):desktop 主套件 2734/2734(清 dist);composer-git-branch 4/4;@maka/core、@maka/ui 构建通过;test SUCCESS。
Automated review notice: This review was produced by an AI agent (Claude Opus 5) and published through the shared
jackweneraccount, which is used by more than one automated seat. It is not an independent human review and does not replace one.
Remove the composer footer's Git-branch chip and the read pipeline that fed it (#5487): the chip was that pipeline's only consumer, so nothing is left stranded. The branch is ambient session state the agent owns, not a parameter of the send, so the composer's control row is the wrong home for a read-only value there — and it was the one item on that row drawn as a hand-rolled span instead of an Astryx primitive. The workbar's Review face names the current branch once #5120 lands; this lands first so the two never show the same fact twice. Migration: none. Between this merge and #5120's there is no surface naming the branch — a deliberate gap over duplicating the readout. #5120 needs a trivial rebase on git-review-main.ts and packages/core/src/git-review.ts to drop the readGitBranch / GitBranchReadResult it inherited from #5487. Refs #2171, #5487 Generated-by: Maka
3 participants
What
The composer's send-context row carries the context-usage readout; this adds the repository's Git branch beside it, so the working tree a turn will run against is named where the turn is composed.
How
A new read-only
git:branchIPC in the main process resolves the Session's workspace the same waygit-review:readdoes, then asks only what the chip prints:git branch --show-current, falling back to the short sha on a detached HEAD. It deliberately does not reusereadGitReview, which computes the full diff.Three states, matching what the caller can honestly say:
{ ok: false, isGitRepo: false }branch: null,shortShasetbranchsetAbsence of Git is the normal case, so the chip simply does not exist there — an empty husk would be a claim that something is present. The chip takes the branch from the Session's own workspace, so it follows a Session switch.
Files
packages/core/src/git-review.ts—GitBranchSnapshot/GitBranchReadResult(shared by main + preload).apps/desktop/src/main/git-review-main.ts—readGitBranch.apps/desktop/src/main/runtime-host-workspace-ipc-main.ts—git:branchhandler.apps/desktop/src/preload/{preload.ts,bridge-contract.d.ts}—gitReview.branch().apps/desktop/src/renderer/chat-composer-region.tsx— the hook, inlined (a new renderer file would widen the legacy closure the architecture ratchet tracks).packages/ui/src/composer.tsx—gitBranchprop +GitBranchChip.apps/desktop/renderer-architecture.json— the ratchet records the one new bridge access + two hooks forchat-composer-region.tsx.Tests
apps/desktop/src/main/__tests__/git-review-main.test.ts— named branch, detached HEAD (short sha), non-repository.packages/ui/src/__tests__/composer-context-usage.test.tsx— chip renders the name, renders the short sha when detached, and is absent (not empty) without Git.Not in scope
Other callers do not yet read the branch (a rebase/merge indicator, filling the branch into the provider request headers, etc.).