Skip to content

fix(development): make driveless /dashboard/development a GLOBAL view - #2027

Merged
2witstudios merged 4 commits into
masterfrom
pu/development-global-view
Jul 12, 2026
Merged

fix(development): make driveless /dashboard/development a GLOBAL view#2027
2witstudios merged 4 commits into
masterfrom
pu/development-global-view

Conversation

@2witstudios

@2witstudios 2witstudios commented Jul 12, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes a scope bug in the Development surface (merged in #2015): the driveless /dashboard/development route redirected to a resolved "current/last" drive, opening a seemingly-random drive instead of showing everything. Per the "Routing correction (2026-07-12)" section of the Development surface spec, it's now a real GLOBAL command center — all machines across all drives the user can access, grouped by drive. /dashboard/{driveId}/development (per-drive) is unchanged.

  • packages/lib/services/machines/machine-list: new listMachinesAcrossDrives, grouping the existing per-drive listMachinesInDrive result by drive and dropping drives with no visible machines (avoids empty group headers). Per-drive failures are isolated (Promise.allSettled) so one bad drive can't blank the whole global view.
  • GET /api/machines: a request with no driveId now returns { drives: [{ driveId, driveName, machines }] } instead of 400 — same app-admin gate, same per-page canUserViewPage filtering as the per-drive path (a machine withheld by a page-level grant never appears in either mode).
  • machine-list-runtime: listAllMachines(), sourcing accessible drives from listAccessibleDrives — the same drive universe GET /api/drives already hands DriveSwitcher.
  • useDriveMachines: new useAllMachines(enabled) hook, same SWR/polling convention as useDriveMachines.
  • DevelopmentSidebar: branches on driveId presence — drive-scoped list unchanged, new grouped-by-drive list in global mode. MachineTreeSection routes via a centralized buildMachineHref (next to parseSelectedMachineId in development-route.ts), so global-mode machines route to /dashboard/development/{machineId} rather than crossing into a specific drive's route tree.
  • New /dashboard/development/layout.tsx + [machineId]/page.tsx: the global twin of the drive-scoped detail region, sourced from useAllMachines(). Deliberately its own route tree (not folded into the drive-scoped layout) — Next.js remounts a segment when the route tree changes, so crossing into /dashboard/{driveId}/development would tear down the global layout's keep-alive host and every terminal it's keeping warm. (Known, documented tradeoff: navigating from global into a specific drive's Development view still crosses that boundary and disconnects every warm terminal, not just the one being left — mirrors an already-accepted tradeoff elsewhere in the surface and is recoverable, not data loss.)
  • Extracted useStickyMachineIds, useDrainPendingSession, DetailState/DetailNotice, and resolveDisplayedMachine out of the drive-scoped layout into lib/development/ so both layouts share identical logic instead of duplicating it.
  • Removed the now-dead resolveActiveDriveId redirect helper (and its test).

MachineTree/MachineView/Machine-page tabs are untouched, per the sub-task's scope.

Merged origin/master to resolve a real conflict: #2017 (workspace-owns-pane-grid) changed the same [driveId]/development/layout.tsx this PR refactors (pending-session workspace lookup moved from a flat map to selectActiveWorkspace). Ported that into the shared use-drain-pending-session.ts hook.

Convergence pass (8-angle automated review + Codex): fixed the Promise.allPromise.allSettled correctness bug above, a stale doc comment in sidebar-routes.ts, deduped fetcher/globalFetcher and the sidebar's list-notice guard chains, replaced a GLOBAL_SCOPE magic string with undefined, and replaced a raw NUL byte Codex flagged in use-sticky-machine-ids.ts with a proper escape. See PR comments for full detail on what was fixed vs. deliberately left alone (and why).

Test plan

  • packages/lib/services/machines/machine-list.test.ts — grouping, per-page filtering, and per-drive failure isolation for listMachinesAcrossDrives
  • apps/web/api/machines/route.test.ts — global-mode branch: admin gate, filtering, empty list, auth propagation
  • apps/web/app/dashboard/development/__tests__/layout.test.tsx — new global layout (flattening, sticky ids, admin gate, detail states)
  • apps/web/app/dashboard/[driveId]/development/__tests__/layout.test.tsx — unchanged behavior, still green after extracting shared hooks + the master merge
  • DevelopmentSidebar.test.tsx — drive-scoped tests still green + global-mode tests (grouped rendering, routing to /dashboard/development/{machineId}, admin gate, empty/error states)
  • development-route.test.ts — driveless URL prefix + new buildMachineHref, including a parse/build round-trip test
  • displayed-machine.test.ts — new, for the extracted resolveDisplayedMachine
  • bun run typecheck (full monorepo, including next build) — green
  • bun run lint (web + lib) — green, no new warnings
  • Full apps/web vitest suite — 12,800+ passed; a handful of pre-existing failures are unrelated (DB-role provisioning tests needing bun run test's DB setup, one pre-existing timezone flake)

🤖 Generated with Claude Code

https://claude.ai/code/session_01PZNYjLCR2FxEcvxZbsfDqp

The Development surface's driveless route redirected to a resolved
"current/last" drive instead of showing everything, per the routing
correction in the Development surface spec. It's now a real global
command center: all machines across all drives the user can access,
grouped by drive.

- packages/lib/services/machines/machine-list: add
  listMachinesAcrossDrives, grouping listMachinesInDrive's per-drive
  result by drive and dropping drives with no visible machines.
- apps/web api/machines route: no-driveId request now returns
  { drives: [...] } instead of 400, under the same app-admin gate +
  per-page canUserViewPage filtering as the per-drive path.
- machine-list-runtime: listAllMachines(), sourcing accessible drives
  from listAccessibleDrives (the same drive universe GET /api/drives
  hands DriveSwitcher).
- useDriveMachines: add useAllMachines(enabled), same SWR convention.
- DevelopmentSidebar: branches on driveId presence — drive-scoped list
  unchanged, new grouped-by-drive list in global mode. MachineTreeSection
  now takes a basePath so global-mode machines route to
  /dashboard/development/{machineId} rather than crossing into a
  specific drive's route tree.
- New /dashboard/development/layout.tsx + [machineId]/page.tsx: the
  global twin of the drive-scoped detail region, sourced from
  useAllMachines(). Kept as its own route tree (not merged into the
  drive-scoped layout) so opening a machine from the global list never
  crosses into /dashboard/{driveId}/development and tears down the
  keep-alive host.
- Extracted useStickyMachineIds, useDrainPendingSession, and
  DetailState/DetailNotice out of the drive-scoped layout into
  lib/development/ so both layouts share the exact same logic instead
  of duplicating it.
- Removed the now-dead resolveActiveDriveId redirect helper.

Colocated tests cover the new grouping/filtering logic, the API route's
global branch, the global layout, and the sidebar's global mode.
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@2witstudios, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dacbfd34-5482-4fcd-a242-fafdbcf8f6aa

📥 Commits

Reviewing files that changed from the base of the PR and between b7859dc and f475d16.

📒 Files selected for processing (23)
  • apps/web/src/app/api/machines/__tests__/route.test.ts
  • apps/web/src/app/api/machines/route.ts
  • apps/web/src/app/dashboard/[driveId]/development/layout.tsx
  • apps/web/src/app/dashboard/development/[machineId]/page.tsx
  • apps/web/src/app/dashboard/development/__tests__/layout.test.tsx
  • apps/web/src/app/dashboard/development/layout.tsx
  • apps/web/src/app/dashboard/development/page.tsx
  • apps/web/src/components/layout/left-sidebar/DevelopmentSidebar.tsx
  • apps/web/src/components/layout/left-sidebar/__tests__/DevelopmentSidebar.test.tsx
  • apps/web/src/components/layout/left-sidebar/sidebar-routes.ts
  • apps/web/src/hooks/useDriveMachines.ts
  • apps/web/src/lib/development/DetailState.tsx
  • apps/web/src/lib/development/__tests__/development-route.test.ts
  • apps/web/src/lib/development/__tests__/displayed-machine.test.ts
  • apps/web/src/lib/development/__tests__/resolve-active-drive.test.ts
  • apps/web/src/lib/development/development-route.ts
  • apps/web/src/lib/development/displayed-machine.ts
  • apps/web/src/lib/development/resolve-active-drive.ts
  • apps/web/src/lib/development/use-drain-pending-session.ts
  • apps/web/src/lib/development/use-sticky-machine-ids.ts
  • apps/web/src/lib/machines/machine-list-runtime.ts
  • packages/lib/src/services/machines/__tests__/machine-list.test.ts
  • packages/lib/src/services/machines/machine-list.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pu/development-global-view

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

Copy link
Copy Markdown

💡 Codex Review


P2 Badge Escape the NUL separator in source

This template literal contains an actual NUL byte rather than the textual \u0000 escape used by the previous inline hook. The runtime string is the same, but embedding NUL makes this TypeScript file look binary to some editors/search/diff scripts and can break tooling around the repo; replace the raw byte with \u0000 so the separator stays source-safe.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

The key template literal accidentally embedded an actual NUL byte (introduced during extraction from the per-drive layout) instead of a textual escape. Same runtime string, but a raw NUL byte in a .ts file confuses editors/diff/search tooling. Flagged by Codex review on PR #2027.
@2witstudios

Copy link
Copy Markdown
Owner Author

Addressed in 210effc: replaced the raw NUL byte in use-sticky-machine-ids.ts's key template literal with a proper JS escape sequence. Same runtime string, now source-safe.

Resolves conflict in apps/web/src/app/dashboard/[driveId]/development/layout.tsx against master's #2017 (workspace-owns-pane-grid): kept this branch's extraction of useStickyMachineIds/useDrainPendingSession/DetailState into shared apps/web/src/lib/development/ files, and ported master's updated workspace lookup (selectActiveWorkspace instead of a flat workspaces map) into the shared use-drain-pending-session.ts hook so both the drive-scoped and new global layout pick it up. Also updated the new global layout test to reset the store via the new machines shape.
Fixes from an 8-angle automated code review plus a Codex review comment:

- listMachinesAcrossDrives: one drive's scan failing (or its per-page
  visibility check throwing) no longer 500s the ENTIRE global view --
  Promise.allSettled + drop the failed drive, matching this file's own
  existing "swallow and hide" philosophy for per-page permission-check
  failures. New tests cover both failure points.
- sidebar-routes.ts: fixed a stale DEVELOPMENT_PATH doc comment that still
  described the driveless route as "which redirects" with "no driveless
  twin" -- both were made false by this PR.
- Extracted the duplicated isKnownMachine/displayedMachineId derivation
  (present in both layouts, previously copy-pasted) into a shared
  resolveDisplayedMachine, completing the extraction this PR already did
  for useStickyMachineIds/useDrainPendingSession/DetailState.
- Centralized the machine detail-URL builder (buildMachineHref) alongside
  the existing parseSelectedMachineId in development-route.ts, so the
  parse and build sides of the URL shape can't drift apart. MachineTreeSection
  now takes a driveId again instead of an ad hoc basePath string.
- Deduped the near-identical guard-chain early-returns in DriveMachineList/
  GlobalMachineList into a shared resolveListNotice helper.
- Deduped the byte-for-byte identical fetcher/globalFetcher bodies in
  useDriveMachines.ts into one generic machinesFetcher<T>.
- Dropped the GLOBAL_SCOPE magic-string sentinel in favor of passing
  undefined to useStickyMachineIds, matching the hook's own "no scope"
  idiom.
- Documented (rather than attempted to fix, as out of scope for this PR)
  a known tradeoff the global layout's route-tree separation doesn't
  solve: navigating from the global Development view into a specific
  drive's Development view crosses route trees and disconnects every
  warm terminal across every drive, not just the one being left. This
  mirrors an already-accepted tradeoff elsewhere in the surface (switching
  drives already tears down the other drive's terminals) and is
  recoverable (reconnects on remount), not data loss.

Also merged origin/master (bb72a70..b7859dc) to resolve a real conflict:
master's #2017 changed [driveId]/development/layout.tsx's pending-session
workspace lookup from a flat workspaces map to selectActiveWorkspace (the
new workspace-owns-pane-grid model). Ported that into the shared
use-drain-pending-session.ts hook so both layouts pick it up.

All targeted tests green (232 tests across 16 files), full monorepo
typecheck+build green, lint green (no new warnings).
@2witstudios

Copy link
Copy Markdown
Owner Author

Follow-up convergence pass on this PR — ran an 8-angle automated review (correctness ×3, reuse, simplification, efficiency, altitude, CLAUDE.md conventions) plus resolved a real merge conflict against master (#2017 changed the same layout.tsx this PR refactors). Summary of what changed, pushed in f475d16:

Correctness fix (real bug): listMachinesAcrossDrives used Promise.all across every accessible drive — one drive's scan (or its per-page visibility check) throwing would 500 the entire global view, hiding every other drive too. Switched to Promise.allSettled + drop the failed drive, matching this file's own existing "swallow and hide" philosophy for per-page permission-check failures (which already do this at the machine level). New tests cover both failure points.

Merge conflict resolved: master's #2017 (workspace-owns-pane-grid) changed [driveId]/development/layout.tsx's pending-session workspace lookup from a flat workspaces map to selectActiveWorkspace — the same file this PR had refactored to extract shared hooks. Ported that change into the extracted use-drain-pending-session.ts so both the drive-scoped and new global layout pick it up correctly.

Cleanup (converged on by 2-3 finders independently):

  • Completed the hook extraction this PR started: isKnownMachine/displayedMachineId derivation was left duplicated across both layouts — now a shared resolveDisplayedMachine.
  • Centralized the machine detail-URL builder (buildMachineHref) next to the existing parseSelectedMachineId, so parse/build can't drift apart — MachineTreeSection goes back to a driveId prop instead of an ad hoc basePath string.
  • Deduped the near-identical guard-chain early-returns in DriveMachineList/GlobalMachineList into a shared resolveListNotice.
  • Deduped the byte-for-byte identical fetcher/globalFetcher in useDriveMachines.ts into one generic machinesFetcher<T>.
  • Dropped a GLOBAL_SCOPE magic-string sentinel in favor of undefined (the hook's own "no scope" idiom).
  • Fixed a stale doc comment in sidebar-routes.ts that still described the driveless route as "which redirects" / "no driveless twin" — both now false.

Documented, not fixed (explicitly out of scope for this PR): navigating from the global Development view into a specific drive's Development view crosses route trees and disconnects every warm terminal across every drive, not just the one being left. This mirrors an already-accepted tradeoff elsewhere in the surface (switching drives already tears down the other drive's terminals — "a PTY stream must not outlive its drive context") and is recoverable (reconnects on remount), not data loss. A real fix would mean hoisting one MachineKeepAliveHost above both route trees, which is a bigger change than this bug-fix PR warrants. Noted in the global layout's doc comment for whoever picks it up next.

Not changed (considered and deliberately left alone): the per-drive→per-machine canUserViewPage fan-out (already an accepted, documented tradeoff at the per-drive level from #2015, just widened across drives — the file's own comment already anticipates revisiting this "if a drive ever holds enough machines for it to matter"); the 30s poll interval on useAllMachines (this was explicitly directed by the sub-task spec — "same SWR/fetch convention" as useDriveMachines); isAdmin being computed independently client-side in a few places (a pre-existing, 15+-files-wide codebase convention, not something to centralize in this PR); useStickyMachineIds' in-render setState pattern (inherited verbatim from the already-shipped per-drive layout, not introduced by this PR).

All targeted tests green (232+ tests), full monorepo typecheck+build green, lint green.

@2witstudios
2witstudios merged commit ad5769e into master Jul 12, 2026
10 checks passed
@2witstudios
2witstudios deleted the pu/development-global-view branch July 12, 2026 23:15
2witstudios added a commit that referenced this pull request Jul 13, 2026
Master landed PR #2027 (fix(development): make driveless
/dashboard/development a GLOBAL view) after this branch was cut, which:
- Extracted useStickyMachineIds/useDrainPendingSession/DetailState/
  resolveDisplayedMachine out of [driveId]/development/layout.tsx into
  shared apps/web/src/lib/development/ hooks, reused by a new
  apps/web/src/app/dashboard/development/layout.tsx (the driveless
  GLOBAL twin).
- Reworked DevelopmentSidebar into two list bodies (DriveMachineList /
  GlobalMachineList) sharing one MachineTreeSection, routed via a new
  buildMachineHref helper.

Both touched the exact same seam this branch reworks (the sidebar's
click-intent flow and MachineKeepAliveHost wiring), causing real
conflicts. Resolved by:
- Porting the workspace-id rename (usePendingSessionStore/
  resolvePendingSession -> usePendingWorkspaceStore/
  resolvePendingWorkspace, openTerminal(scope) -> setActiveWorkspace)
  into the now-shared apps/web/src/lib/development/
  use-drain-pending-session.ts hook, so BOTH the drive-scoped and new
  global layout pick up the workspace-selector model, not just one.
- Keeping master's DriveMachineList/GlobalMachineList split; git's
  3-way merge already combined it correctly with WorkspaceLeaves/
  WorkspaceNodeExtras in MachineTreeSection (verified by reading the
  merged output, not just trusting a clean merge).
- Adding the `embedded` prop to the new global
  dashboard/development/layout.tsx's MachineKeepAliveHost call too
  (master's version predates that prop), so a machine opened from the
  global Development view also skips its redundant inner Terminal-tab
  tree.
- Updating dashboard/development/__tests__/layout.test.tsx (global
  layout) to the renamed store/hook and to assert embedded:true,
  mirroring the drive-scoped layout's test.

typecheck clean (apps/web + packages/lib), lint clean, 462 tests green
across 34 files in the affected area.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011sTw4qnoPwfzC2E4xnZQ78
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant