Skip to content

feat(frontend): pool switcher and pool-scoped titles for logs and compute - #5499

Merged
abcxff merged 1 commit into
mainfrom
stack/feat-frontend-pool-switcher-and-pool-scoped-titles-for-logs-and-compute-xwmxvxvp
Jul 31, 2026
Merged

feat(frontend): pool switcher and pool-scoped titles for logs and compute#5499
abcxff merged 1 commit into
mainfrom
stack/feat-frontend-pool-switcher-and-pool-scoped-titles-for-logs-and-compute-xwmxvxvp

Conversation

@abcxff

@abcxff abcxff commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@abcxff

abcxff commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5499 July 29, 2026 15:55 Destroyed
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review: pool switcher and pool-scoped titles for logs and compute

Nice feature, and the Ladle story choices (integration unit with the title, realistic fixtures like the no-config legacy pool and the long-name truncation case) follow the frontend conventions well.

Two issues from the previous pass on this PR are now fixed:

  • ComputeForm now takes key={selectedPool} (frontend/src/app/settings-pages/namespace-compute.tsx:256-263), so switching back to an already-cached pool remounts the form instead of keeping stale RHF values.
  • The Logs route loader now resolves ?pool= before prefetching (ensureQueryData on the pools list, then resolvePoolName(pools, deps.pool)), so a deep link to a non-default pool prefetches the right pool.

Open finding

The conditional pool deletion in _context.tsx likely breaks the pool switcher on the Logs pagefrontend/src/routes/_context.tsx:46-56

validateSearch: (search) => {
  const validated = searchSchema.parse(search);
  if (validated.settings !== "compute") {
    delete (validated as { pool?: string }).pool;
  }
  return validated;
},

The comment says this scopes pool to "the pages that actually use it," relying on the Logs route re-declaring pool in its own validateSearch. But the condition only checks settings === "compute" (the settings-drawer tab), it says nothing about being on the Logs route. _context is an ancestor of the Logs route, and TanStack Router chains validateSearch down the matched route tree (each route's validator receives the accumulated output of its ancestors, not the raw URL search independently) — which is presumably why this schema is written as .and(z.record(z.string(), z.any())) in the first place, to avoid stripping search keys owned by child routes. On the Logs page search.settings is never "compute", so this branch deletes pool before the Logs route's own validateSearch ever sees it, and before loaderDeps/Route.useSearch() can read it back. That would mean:

  • A deep link like /logs?pool=backend loses the pool param at the parent before the child's resolvePoolName(pools, deps.pool) runs.
  • Clicking the new PoolSwitcher on the Logs page calls navigate({ to: ".", search: (old) => ({ ...old, pool: name }) }), which re-triggers the full validateSearch chain including _context's — so the just-set pool would be stripped again on the very next resolve, and the switcher would appear to do nothing (or immediately snap back).

I can't spin up the dev server in this sandbox to confirm the exact chaining behavior empirically, but the logic as written doesn't match the stated intent regardless: it should preserve pool while on the Logs route too, not only while settings === "compute". Worth a quick manual check per the repo's frontend-testing convention: open a namespace with 2+ managed pools, go to Logs, use the switcher, and confirm the URL/log stream actually update and stick.

Minor

  • No unit tests for the new pure helpers in frontend/src/app/pool-switcher.tsx (resolvePoolName, poolHeaderText). Both have non-obvious fallback precedence (requested pool, then a pool literally named "default", then the first pool, then the "default" string) that would be easy to regress silently. The repo already has precedent for testing this kind of pure logic (e.g. frontend/src/lib/format-value.test.ts).
  • frontend/src/app/settings-drawer.tsx:463poolHeaderText(pools, "Config", fallback) is only ever called after the pools.length <= 1 early return, so its generic-title branch is unreachable there. Not wrong, just slightly redundant.

No security or backend concerns. This PR is frontend-only (routing/state/UI), and features.compute already requires platform, so OSS is unaffected.

@railway-app

railway-app Bot commented Jul 29, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5499 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-inspector 😴 Sleeping (View Logs) Web Jul 29, 2026 at 4:10 pm
kitchen-sink 😴 Sleeping (View Logs) Web Jul 29, 2026 at 4:06 pm
website 😴 Sleeping (View Logs) Web Jul 29, 2026 at 4:05 pm
frontend-cloud 😴 Sleeping (View Logs) Web Jul 29, 2026 at 4:04 pm
ladle ✅ Success (View Logs) Web Jul 29, 2026 at 3:58 pm
mcp-hub ✅ Success (View Logs) Web Jul 29, 2026 at 3:56 pm

@abcxff
abcxff force-pushed the stack/feat-frontend-pool-switcher-and-pool-scoped-titles-for-logs-and-compute-xwmxvxvp branch from 3220ea4 to 289ca6e Compare July 30, 2026 19:59
@abcxff
abcxff force-pushed the stack/feat-frontend-pool-switcher-and-pool-scoped-titles-for-logs-and-compute-xwmxvxvp branch 2 times, most recently from 919f407 to d81fef1 Compare July 31, 2026 16:29
@abcxff
abcxff force-pushed the stack/feat-frontend-pool-switcher-and-pool-scoped-titles-for-logs-and-compute-xwmxvxvp branch from d81fef1 to 02e7183 Compare July 31, 2026 16:56
@abcxff
abcxff merged commit 02e7183 into main Jul 31, 2026
15 of 17 checks passed
@abcxff
abcxff deleted the stack/feat-frontend-pool-switcher-and-pool-scoped-titles-for-logs-and-compute-xwmxvxvp branch July 31, 2026 17:31
@railway-app
railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5499 July 31, 2026 17:31 Destroyed
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