Make machine settings text-first and responsive - #2050
Conversation
d3eeec2 to
8e12be8
Compare
![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
I am SlopCop. I am reviewing commit d3eeec20bc3d9e4b7d0c2fd1accf2e63df8c99be for security, quality, performance, architecture, and end-to-end behavior.
This pull request makes the machine settings pages text-first and responsive on narrow screens.
The live pull request now points to 8e12be8f65fbdcaeadbe09d264b24ba3822d00bc. This review run applies to the supplied commit.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This PR replaces many machine settings icons with clear text. It also lets rows stack on narrow screens.
I found one non-blocking performance issue. MachineSettingsView still imports 95 extended icons, although the route now uses only core icons. I left an inline comment.
The live head removed the ten unreferenced images from the supplied commit. That later change removes 583,007 bytes.
Security review found no issue. React still escapes machine data, and the server still enforces all machine management limits.
The architecture scan found two optional cleanups. MachineStatusIcon has no consumers after this change. Both machine views also duplicate the platform labels and primary removal message. A shared host display helper can own these values.
Validation passed:
- The full
@bb/appsuite passed 3,088 tests across 402 files. Three tests were skipped. - The focused machine settings tests passed all 21 tests.
- The
@bb/apptype check passed. - The lint task completed with zero errors and 144 warnings.
- Doobie verified the list and detail routes at 1280 and 320 pixels. Both routes had no horizontal overflow at 320 pixels.
This review applies to d3eeec20bc3d9e4b7d0c2fd1accf2e63df8c99be. The live pull request now points to 8e12be8f65fbdcaeadbe09d264b24ba3822d00bc.
|
Addressed the SlopCop review in d2627ec:
I kept the suggested shared platform-label/removal-message helper out of this PR because it is an optional, behavior-neutral refactor beyond the focused UI change. Verification: focused machine settings tests (21/21), |
## What was wrong runBuildProcess spawned its build process before registering SIGINT and SIGTERM handlers. A fast child could become externally observable during that window, allowing another process to send SIGTERM while Node still had its default signal behavior. The parent then exited with signal=SIGTERM instead of cleaning up the build leader and grandchild, which caused the flaky packages job on PR #2050. ## What changed Register the build signal handlers before spawning the child. Keep spawn inside the existing try/finally lifecycle so synchronous startup failures also remove both handlers. There are no wire, daemon protocol, CLI, persistence, or documentation contract changes. ## How you verified - The first CI attempt on PR #2050 failed in test/start-bb.test.mjs with code=null and signal=SIGTERM; its rerun passed, confirming the timing-dependent behavior. - Repeated the focused @bb/scripts process-group integration test 100 times with Turbo cache bypass; all 100 passed without Vitest retries. - pnpm exec turbo run test --filter=@bb/scripts --force — 18 files and 97 tests passed. - pnpm exec turbo run typecheck --filter=@bb/scripts — passed. - git diff --check — passed. Follow-up to #2050. > AGENT GENERATED: by GPT-5.6 Codex
1 participant
What was wrong
The machine list and detail redesign in #1996 compressed status, project count, permissions, and update state into a dense set of glyphs and tooltips. That made common metadata slower to scan, turned the add action into an ambiguous bare plus, and put the detail identity subtitle inside a card that implied a separate section. Several flex items were also forced to remain unshrinkable, so long metadata and detail key/value groups became cramped on phone-width screens. Tracks #2049.
What changed
Machine list
Machine detail
Narrow screens at 320px
How you verified
pnpm exec turbo run test --filter=@bb/app -- src/components/settings/MachinesSettingsSection.test.tsx src/views/MachineSettingsView.test.tsx— 21 tests passed. The new assertions fail against the previous icon-only and card-based markup.pnpm exec turbo run typecheck --filter=@bb/app— passed.pnpm exec turbo run lint --filter=@bb/app— passed.Fixes #2049