Fixed TUI tab naming to correspond to the web client. - #1780
Conversation
|
@claude review |
|
Claude finished @BobDickinson's task in 3m 35s —— View job Review: TUI tab naming alignment (#1780)
Verdict: looks good — no correctness issues found. A few small suggestions below, plus one process item. What I verifiedNaming matches web.
No stale hardcoded keys. The accelerator map in Tests + gate pass locally (had to
This also settles the thing I'd have worried about with a mid-word split: Suggestions (non-blocking)1. if (idx < 0) {
return { before: "", accel: label.slice(0, 1), after: label.slice(1) };
}When the accelerator isn't in the label, this underlines the first character anyway, telling the user a key that won't work. Unreachable with today's config (and the new invariant test keeps it that way), but a silent-lie fallback is worse than no underline. 2. The new invariant test doesn't guard the constraint the doc-comment cites — The doc-comment in for (const tab of tabs) {
expect(["c", "d"]).not.toContain(tab.accelerator.toLowerCase());
}3. 4. 5. Minor cohesion nit: ProcessThis PR is missing the gh pr edit 1780 --add-label v2
|
|
That was the nitpickiest PR review I've even seen. I'm taking zero of that feedback. The point was to make a small fix to change the naming only as we are close to a release, which I did. @cliffhall - Ready for your review and merge. |
Adding the accelerators to the |
Closes #1779
Summary
p, Networkn, Consoleo; Prompts moves tomso Protocol can takep; Connect keepsc)Test plan
npm run cip/n/oswitch those tabs;mopens Prompts;cstill Connect on a disconnected stdio server