feat(deployment): add flag-gated redesigned deployment detail page - #3580
Conversation
f4e0beb to
20b1c79
Compare
📝 WalkthroughWalkthroughChangesThe deployment detail redesign adds feature-flagged routing, a data-driven detail page with URL-synchronized tabs, a metrics header, a gated preview route, shared GPU lease detection, and unit and Playwright coverage. Deployment detail redesign
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.spec.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. apps/deploy-web/src/components/deployments/DeploymentSubHeader.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3580 +/- ##
==========================================
- Coverage 76.46% 75.90% -0.57%
==========================================
Files 1137 1052 -85
Lines 29679 27461 -2218
Branches 7372 6969 -403
==========================================
- Hits 22695 20843 -1852
+ Misses 6151 5817 -334
+ Partials 833 801 -32
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetail.tsx`:
- Line 8: Update DeploymentDetail’s router usage to import useRouter from
next/router instead of next/navigation. Read router.query.tab only after
router.isReady, preserving the existing tab behavior once the Pages Router is
initialized.
In
`@apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.spec.tsx`:
- Around line 46-50: Update the leaseStatus construction in the hasLeaseStatus
branch to create mock<LeaseStatusDto>() without nested forwarded_ports or
services overrides, then assign forwarded_ports and services afterward as plain
objects using the existing values. Preserve the current service URI mapping and
return behavior.
In
`@apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailRouter.spec.tsx`:
- Around line 30-31: Update the setup function’s useFlag mock to use the typed
mock helper with typeof DEPENDENCIES.useFlag, then configure its return value
from input.isRedesignEnabled; remove the unsafe as unknown as cast.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2de96f09-2d3a-41d9-8752-81fa01b4785f
📒 Files selected for processing (11)
apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetail.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailPreview.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailRouter.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailRouter.tsxapps/deploy-web/src/components/deployments/DeploymentDetailLegacy.tsxapps/deploy-web/src/pages/deployments/[dseq]/index.tsxapps/deploy-web/src/pages/deployments/[dseq]/preview.tsxapps/deploy-web/src/types/feature-flags.tsapps/deploy-web/tests/ui/deployment-detail-preview.spec.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether a freshly-loaded closed deployment would show a blank body on the redesigned page — isLeasesLoaded never resolves for non-active deployments since the lease query is gated with enabled: deployment?.state === "active", but that same gating pattern exists in the legacy page today, so it's not a regression introduced by this PR.
Extended reasoning...
Checked this specifically because the redesigned page's body render condition (deployment && isLeasesLoaded) looked suspicious for closed deployments, where the lease list query is disabled and isSuccess never flips true. Confirmed the legacy DeploymentDetailLegacy.tsx has the identical enabled: deployment?.state === "active" gating and the identical deployment && isLeasesLoaded render guard, so this behavior (if it is even a real issue) predates this PR and isn't introduced by the redesign.
20b1c79 to
c50ee4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetail.spec.tsx`:
- Around line 21-28: Update the “tracks a navigate_tab analytics event when
switching tabs” test to also assert that window.location.search equals
"?tab=LOGS" after clicking the Logs tab, covering the tab query-parameter update
via history replacement while preserving the existing analytics and
rendered-content assertions.
In
`@apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailPreview.spec.tsx`:
- Around line 24-29: Update setup’s useFlag in DeploymentDetailPreview tests to
be a spy that returns input.isPreviewEnabled, expose that spy from setup, and
assert it is called with "deployment_detail_preview" in the relevant test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7adb87c5-06e0-4902-b742-32789c01aceb
📒 Files selected for processing (8)
apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetail.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetail.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailPreview.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailPreview.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailRouter.spec.tsxapps/deploy-web/src/types/feature-flags.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailRouter.spec.tsx
- apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.spec.tsx
- apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.tsx
- apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailPreview.tsx
- apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetail.tsx
- apps/deploy-web/src/types/feature-flags.ts
547009f to
a8b768c
Compare
Introduce a redesigned deployment detail page behind two Unleash flags, alongside the current one. - `deployment_detail_redesign` gates the canonical `/deployments/[dseq]` route (new vs legacy page). - `deployment_detail_preview` gates a temporary parallel `/deployments/[dseq]/preview` route that renders the same page for e2e and demos while the canonical route keeps serving the legacy page. The new page leads with a status-first header (thumbnail, running state, live URL, and a summary card: services, cost, balance, auto top-up, aggregate GPU/vCPU/memory/storage) and a six-tab bar (Details, Logs, Events, Shell, Settings, Billing & Notifications). Tab bodies reuse the existing components so nothing regresses, and tabs update the `?tab=` param without navigating. Rename the old page to `DeploymentDetailLegacy` so the redesign owns the `DeploymentDetail` name, keeping rollout teardown deletion-only.
a8b768c to
37a3df3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/deploy-web/src/utils/reclamationUtils.spec.ts (1)
149-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
reclaiminglive-lease branch.
hasLiveGpuLeasetreats bothactiveandreclaimingleases as live. The suite only exercisesactiveandclosed. Add a positivereclaimingcase so a regression in the second live state fails the test.Suggested test
it("is true when a live lease has a GPU", () => { expect(hasLiveGpuLease([createLease({ state: "active", gpuAmount: 1 })])).toBe(true); }); + + it("is true when a reclaiming lease has a GPU", () => { + expect(hasLiveGpuLease([createLease({ state: "reclaiming", gpuAmount: 1 })])).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/deploy-web/src/utils/reclamationUtils.spec.ts` around lines 149 - 160, Add a positive test in the hasLiveGpuLease suite using createLease with state "reclaiming" and a positive gpuAmount, asserting the result is true. Keep the existing active, closed, and zero-GPU coverage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/deploy-web/src/utils/reclamationUtils.spec.ts`:
- Around line 149-160: Add a positive test in the hasLiveGpuLease suite using
createLease with state "reclaiming" and a positive gpuAmount, asserting the
result is true. Keep the existing active, closed, and zero-GPU coverage
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0f4a0c7-3031-4f3b-b6eb-56697db601ba
📒 Files selected for processing (5)
apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.tsxapps/deploy-web/src/components/deployments/DeploymentSubHeader.tsxapps/deploy-web/src/utils/reclamationUtils.spec.tsapps/deploy-web/src/utils/reclamationUtils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.tsx
- apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentDetailHeader.spec.tsx
There was a problem hiding this comment.
Beyond the inline nits found this run, I also checked two other candidates and ruled them out: the COST tile summing lease prices without checking denom mirrors the same pre-existing pattern already used by DeploymentSubHeader/useDeploymentMetrics, not a new risk introduced here; and the header's lease status still refreshes on window refocus/remount even without the Details tab mounted, so it does not go permanently stale the way the earlier manifest-resend refresh issue does.
Extended reasoning...
This is a follow-up note on top of the four nit-level findings already posted inline this run (deploymentCost/hasGpu duplication between DeploymentDetailHeader and DeploymentSubHeader/useDeploymentMetrics, the TOTAL SERVICES fallback counting historical leases, and the closeManifestEditor tab/URL desync). Two additional candidates surfaced by finder agents were investigated and refuted: (1) the COST tile summing lease.price.amount across leases without checking denom — this is not new behavior, it is the same computation useDeploymentMetrics already performs for the legacy DeploymentSubHeader, so it carries no additional risk from this PR; (2) whether the header's own useLeaseStatus call goes stale when the Details tab (and its polling LeaseRow) is not mounted — it still refetches on window focus and component remount by default TanStack Query behavior, so it degrades gracefully rather than freezing indefinitely. Recording this so a later pass on this PR does not need to re-derive the same analysis. This is informational only and does not change the fact that a human should still review the substantive findings already posted inline.
| export const DeploymentDetailHeader: FC<DeploymentDetailHeaderProps> = ({ deployment, leases, providers, dependencies: d = DEPENDENCIES }) => { | ||
| const { deploymentLocalStorage } = d.useServices(); | ||
| const { address } = d.useWallet(); | ||
| const deploymentCost = leases?.reduce((sum, lease) => sum + parseFloat(lease.price.amount), 0) ?? 0; |
There was a problem hiding this comment.
🟡 DeploymentDetailHeader.tsx:47 reimplements the same lease-price reduce already defined in useDeploymentMetrics.tsx:18 (leases.reduce((p,c) => p + parseFloat(c.price.amount), 0)) instead of extracting a shared getDeploymentCost(leases) helper. Both DeploymentSubHeader (via useDeploymentMetrics) and DeploymentDetailHeader now carry the identical formula independently for the duration of the flag-gated rollout — the same duplication pattern this PR already fixed for the GPU predicate via hasLiveGpuLease in reclamationUtils.ts.
Extended reasoning...
What the duplication is: useDeploymentMetrics.tsx:18 computes deploymentCost as leases.reduce((prev, current) => prev + parseFloat(current.price.amount), 0). The new DeploymentDetailHeader.tsx:47 (added by this PR) reimplements the identical formula inline: leases?.reduce((sum, lease) => sum + parseFloat(lease.price.amount), 0) ?? 0. Both compute the exact same domain value — total lease cost as a sum of lease prices — differing only in null-guard style (?? 0 vs. relying on the hook's non-null leases param) and variable names.
Why the header avoided the hook in the first place: this PR's own review thread already flagged that pulling in useDeploymentMetrics for this single field is wasteful, because that hook also unconditionally calls useRealTimeLeft → useBlock(\"latest\", { refetchInterval: 30000 }), opening a standing 30s chain-API poll whose result (realTimeLeft) DeploymentDetailHeader never uses. The fix applied was exactly what's flagged here: compute deploymentCost inline instead of calling the hook. That's the right call for avoiding the poll, but it reintroduces a second, independent copy of the cost formula that DeploymentSubHeader (via useDeploymentMetrics) also carries.
Why this matters during the rollout: per the PR description, DeploymentSubHeader/DeploymentDetailLegacy and DeploymentDetailHeader/DeploymentDetail coexist in production behind the deployment_detail_redesign flag until the legacy page is deleted. Any future change to the cost formula (e.g. handling a different price denom, adding a discount, rounding rule) has no single point of update — a maintainer editing one file gets no compiler or test signal that the sibling copy also needs the same edit.
Precedent already set in this very PR: the analogous GPU-lease predicate (leases?.some(l => isLeaseLive(l) && l.gpuAmount && l.gpuAmount > 0)) was duplicated in exactly the same way between DeploymentSubHeader and DeploymentDetailHeader, and this PR extracted it into a shared hasLiveGpuLease(leases) helper in reclamationUtils.ts, with both headers now calling it (see DeploymentSubHeader.tsx diff and DeploymentDetailHeader.tsx:60). The cost formula is the same shape of problem and would benefit from the same treatment: a small pure getDeploymentCost(leases) helper (e.g. colocated with useDeploymentMetrics or alongside hasLiveGpuLease) that both useDeploymentMetrics and DeploymentDetailHeader call.
Step-by-step proof of the duplication:
useDeploymentMetrics.tsx:18:leases.reduce((prev, current) => prev + parseFloat(current.price.amount), 0).DeploymentDetailHeader.tsx:47:leases?.reduce((sum, lease) => sum + parseFloat(lease.price.amount), 0) ?? 0.- Both expressions sum the same field (
price.amount, parsed as a float) across the same collection (leases) for the same purpose (total lease cost feedingPricePerTimeUnit/the COST tile). - The two host components are mutually exclusive at render time (flag-gated) but coexist in the codebase for the duration of the rollout, so a fix or behavior change to one copy silently fails to propagate to the other, with nothing catching the drift.
Impact: this is a pure reuse/code-quality issue — no incorrect behavior, crash, or data loss results from the current duplication, so it does not block merging this PR. It's raised because the fix is small, well-precedented within this same diff, and worth doing before the duplication has two independent lifetimes to track during the rollout.
2 participants
Why
The deployment detail page opens on a dense grid of text (balance, cost, spent, status, time-left, dseq). The redesign leads with what a user cares about at a glance — is it running, where can I reach it, what does it cost, how much balance is left — and reorganizes the tabs. This PR lays the foundational shell every subsequent tab slice builds on, shipped behind feature flags in parallel with the current page so nothing changes for users until we deliberately roll it over.
Closes CON-821
What
A redesigned deployment detail page behind two Unleash flags:
deployment_detail_redesign— gates the canonical/deployments/[dseq]route (redesigned vs legacy page). Off → the current page is byte-for-byte unchanged.deployment_detail_preview— gates a temporary, development-only/deployments/[dseq]/previewroute that renders the same page for e2e and demos, while the canonical route keeps serving the legacy page. Both routes mount the identicalDeploymentDetailcomponent; removing the preview later is pure file deletion.The page:
?tab=query param viahistory.replaceState— no navigation.#f5f5f5content band and underline tabs./previewroute: deploy a container, then assert the redesigned header + full tab bar render and tabs update?tab=without navigating.The old page is renamed to
DeploymentDetailLegacyso the redesign permanently owns theDeploymentDetailname and rollout teardown stays deletion-only (delete the legacy page, the preview route, and the flags).Provisional, refined in later slices: header balance uses the account total; services count and live URL derive from the primary live lease; the thumbnail is a placeholder pending a product decision.
Summary by CodeRabbit
New Features
Tests