feat(deployment): add Settings and Update tabs to the redesigned detail page - #3610
Conversation
…il page Split the redesigned deployment detail tabs so the SDL editor lives on a new Update tab and a consolidated Settings tab holds billing, notifications, and a danger zone. The separate Billing & Notifications tab is removed. - Settings tab: escrow balance with Add funds, per-deployment Auto Top-Up toggle, relocated notification alerts, and a Close deployment danger zone - Extract the deposit-required Auto Top-Up flow into a useAutoTopUp hook (redesign only; legacy top bar unchanged) - Header: drop the picture placeholder, add an edit-name button, show status as a pulsating badge
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approving — the flagged bug (AlertsListView linking to tab=SETTINGS) affects all non-redesign users on /alerts, which is broad production traffic, so it needs a human look before merge.
Extended reasoning...
Overview
This PR reorganizes the redesigned (flag-gated) deployment detail page: it splits the old combined Settings tab into separate Update (SDL editor) and Settings (billing, notifications, danger zone) tabs, extracts a useAutoTopUp hook, and reworks the header (status badge, inline rename). Most of the change is additive and scoped behind the deployment_detail_redesign flag, with reasonable unit test coverage for the new sections.
Security risks
No new security-sensitive surface: no auth/crypto changes. The danger zone close-deployment flow reuses existing signAndBroadcastTx/confirm patterns. No injection or data-exposure concerns identified.
Level of scrutiny
This warrants more than a rubber-stamp pass despite being flag-gated, because one change — the AlertsListView link target — sits outside the flag gate and is reachable by every signed-in user visiting /alerts, not just redesign cohort users. I verified this against DeploymentDetailLegacy.tsx: its Tab type has no SETTINGS value, and its tab-query effect calls router.replace(UrlService.deploymentDetails(dseq)) (stripping the tab param) when the query doesn't match a known tab, so legacy users end up on Leases instead of Alerts — a regression from the prior "ALERTS" link target that worked for both cohorts.
Other factors
The rest of the diff (new Settings sub-components, status badge, useAutoTopUp) is well-tested and low risk, but the alerts-link regression affects real user navigation today, so this should not be shadow-approved.
… cohort The deployment-name link in the alerts list pointed at tab=SETTINGS, which only exists on the redesigned detail page. Legacy users (redesign flag off) hit the unmatched-tab redirect in DeploymentDetailLegacy and landed on Leases instead of Alerts. Pick SETTINGS or ALERTS based on the deployment_detail_redesign flag so each cohort reaches the tab that holds the deployment's alert settings.
There was a problem hiding this comment.
Beyond the inline nits, I also checked useAutoTopUp's next-check anchor calculation (useAutoTopUp.ts) for skipping a checkpoint on odd hours — it rounds up to the following even-hour anchor rather than under-shooting, so it can only ask for a larger deposit than strictly necessary, not miss a checkpoint. Not a real bug.
Extended reasoning...
Verified the useAutoTopUp checkpoint-anchor candidate flagged by a finder agent: secondsUntilNextTopUp is computed as startOfHour(now) + 2h - now. For an odd current hour (e.g. 3:xx), this yields 5:00 rather than the schedule's actual next run at 4:00, but that only makes the estimate larger (more conservative), which can only increase the requested pre-emptive deposit, never leave the deployment under-funded before the real next check. Confirmed as not a real bug, consistent with the verifier's conclusion.
…denom Address review feedback on the redesigned deployment detail page: - Settings tab "Current balance" now shows the real-time draining escrow (realTimeLeft.escrow) for active deployments with live leases, matching DeploymentSubHeader and DeploymentListRow, instead of the settlement-lagging deployment.escrowBalance. useAutoTopUp now exposes realTimeLeft to callers. - Extract getEscrowDenom(deployment) and reuse it across the deployment views in place of the duplicated escrowAccount.state.funds[0]?.denom fallback. - Drop the unreachable "reclaiming" branch from DeploymentStatusBadge, since reclaiming is a lease state and deployment.state is only active or closed.
6505314
into
feat/deployment-details-placements-services
1 participant
Why
On the redesigned deployment detail page, the SDL editor lived under a "Settings" tab and billing sat in a combined "Billing & Notifications" tab, which isn't where users look for either one. This reorganizes the redesign tabs so editing the SDL and managing a deployment are separate, discoverable places.
Stacked on #3586. Part of CON-821.
What
All behind the
deployment_detail_redesignflag, so the legacy page is untouched.ManifestUpdate) moves here, off the old Settings tab.useAutoTopUphook: extracts the deposit-required-before-enable flow the redesign billing section needs. The legacy top bar keeps its own inline copy on purpose since it's slated for removal, so that duplication is intentional."Current balance" on the billing section is the deployment escrow balance, not the wallet balance. The billing section is kept as a self-contained component so the planned escrow-abstraction swap (hide the toggle, redirect to the billing page) is a contained follow-up.
Testing: unit specs for the new Settings sections, the status badge, the header changes, and the
useAutoTopUphook.