Make environment host labels responsive - #2056
Conversation
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This PR changes environment labels so wide screens show host names and narrow screens show shorter labels.
I am the Slop Cop. I am reviewing this PR for security, code quality, performance, architecture, and end-to-end behavior.
| label="Environment" | ||
| value={visibleEnvironmentLabel} | ||
| compactValue={visibleEnvironmentLabel} | ||
| value={environmentLabel} |
There was a problem hiding this comment.
🚨 slopcop/review — The wide follow-up label truncates.
value now receives a host-prefixed label. The chip still has a 10-rem maximum width.
In the two-host browser test, the text needed 189 pixels but received 130 pixels. The compact label displayed correctly at 390 pixels.
Please allow more width or preserve the mode when the host name truncates. The new unit test checks text nodes only.
| value={visibleEnvironmentLabel} | ||
| compactValue={visibleEnvironmentLabel} | ||
| value={environmentLabel} | ||
| compactValue={environmentCompactLabel} |
There was a problem hiding this comment.
🚨 slopcop/review — The optional compact label can disappear.
environmentCompactLabel remains optional. When a caller omits it, OptionDisplay omits the compact span.
The container CSS hides the full span below 34 rem. The environment label then becomes empty.
Restore environmentCompactLabel ?? environmentLabel, or require the compact label.
|
|
||
| describe("describeEnvironmentSelection", () => { | ||
| it("omits the machine name from the mobile environment label", () => { | ||
| expect(describeEnvironmentSelection(worktreeSelection, host, []).label).toBe( |
There was a problem hiding this comment.
🚨 slopcop/review — Prettier rejects this new test file.
pnpm exec prettier --check apps/mobile/src/screens/pickers/environment-picker-model.test.ts reports a style error here.
Please format this test.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: Wide web views now show the computer name. Narrow web views and mobile show shorter environment labels.
I found one visible layout issue and two smaller issues.
- Medium: The wide follow-up label still uses a 10-rem limit. The browser clipped a 189-pixel label into a 130-pixel text area.
- Low: An omitted compact label creates an empty environment label below 34 rem. The public component prop still permits this omission.
- Low: Prettier rejects the new mobile test file.
I found no security defect. React renders host names as text, and this change gives no host data to a new user.
I found no material performance problem. The new host lookup uses a memoized, linear search and adds no network request.
I scanned the related label code. The new-thread picker and the follow-up summary use different domain inputs, so a shared formatter would not simplify them.
The mobile model split follows the existing folder pattern. The primaryHostId field remains necessary for the projectless machine picker.
All GitHub CI checks pass. The focused web tests passed 12 tests. The full mobile suite passed 826 tests, and both type checks passed.
The doobie test used two hosts. At 1280 pixels, web showed the host-prefixed labels. At 390 pixels, it showed Local and Worktree.
An extra full web run ended without a final summary during concurrent local QA. The passing CI app shards provide the complete web test result.
I posted this review as a comment only.
1 participant
What was wrong
The environment label treated the primary host as implicit even when a server had multiple hosts, so large-screen composers did not consistently identify where a thread would run. The follow-up summary also rendered its compact label at every width, which let a secondary host remain visible on narrow screens instead of switching to a host-free compact label.
What changed
HOST_DAEMON_PROTOCOL_VERSIONis unchanged.How you verified
pnpm exec turbo run test --filter=@bb/app --force -- src/components/pickers/EnvironmentPicker.test.tsx src/components/promptbox/ThreadEnvironmentSummary.test.tsx— 12 tests passed.pnpm exec turbo run test --filter=@bb/mobile --force -- src/screens/pickers/environment-picker-model.test.ts— 1 test passed.pnpm exec turbo run typecheck --filter=@bb/apppnpm exec turbo run typecheck --filter=@bb/mobilepnpm exec turbo run lint --filter=@bb/app --filter=@bb/mobile— 0 errors; 144 pre-existing app warnings.Dev-browser comparison against
origin/main, using the same two-host fixture at 1280px and 390px:Work locallyMichael-M4 · Work locallyLocalLocalLocalMichael-M4 · Working locallyMac Studio · RemoteMac Studio · Working remotelyMac Studio · RemoteRemoteFixes: N/A (no issue filed).