feat(hostd): replace per-price pin toggles with a single switch - #1539
ChrisSchinnerl merged 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 6b47dc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
🟡 Changes recommended
Resolve mixed-state save behavior, pinning-disabled UI handling, effective-state validation, and Save availability for explicit unpinning.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR replaces hostd’s per-price pin toggles with a single all-or-nothing switch while preserving legacy mixed configurations.
Changes:
- Updates pinning transforms, UI, validation, and save-state handling.
- Adds mixed-pinning notices and interaction tracking.
- Adds unit/E2E coverage, fixtures, and a changeset.
File summaries
| File | Summary |
|---|---|
libs/e2e/src/fixtures/textInput.ts |
Adds visibility assertions. |
apps/hostd/contexts/config/usePinningTouched.tsx |
Tracks explicit pinning changes. |
apps/hostd/contexts/config/useOnValid.tsx |
Handles pinning save behavior. |
apps/hostd/contexts/config/types.ts |
Defines pinning state types. |
apps/hostd/contexts/config/transform.ts |
Transforms aggregate and legacy pinning states. |
apps/hostd/contexts/config/transform.spec.ts |
Tests transform behavior. |
apps/hostd/contexts/config/index.tsx |
Exposes legacy pinning state. |
apps/hostd/contexts/config/fields.tsx |
Updates pricing fields and validation. |
apps/hostd/components/Config/PricingSetting.tsx |
Adds shared pricing controls and notices. |
apps/hostd/components/Config/index.tsx |
Renders unified pricing controls. |
apps/hostd-e2e/src/specs/configLegacyPinning.spec.ts |
Tests legacy mixed configurations. |
apps/hostd-e2e/src/specs/config.spec.ts |
Updates pricing configuration tests. |
apps/hostd-e2e/src/fixtures/configSetMixedPinning.ts |
Provides mixed-pinning setup. |
apps/hostd-e2e/src/fixtures/configResetAllSettings.ts |
Updates configuration reset behavior. |
.changeset/pin-all-prices-or-none.md |
Documents the user-facing change. |
Review details
Suppressed comments (6)
apps/hostd/contexts/config/fields.tsx:264
- In the mixed legacy state, this fiat input is rendered for a pinned storage price while
values.shouldPinPricesis false. The short-circuit therefore accepts a negative pinned value andtransformUpSettingsPinnedcan send it in the preserved mixed payload; validate against the effective per-price pin state instead (and apply the same correction to the other pinned-price validators).
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:313
- In the mixed legacy state, this fiat input is rendered for a pinned egress price while
values.shouldPinPricesis false. The short-circuit therefore accepts a negative pinned value andtransformUpSettingsPinnedcan send it in the preserved mixed payload; validate against the effective per-price pin state instead.
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:361
- In the mixed legacy state, this fiat input is rendered for a pinned ingress price while
values.shouldPinPricesis false. The short-circuit therefore accepts a negative pinned value andtransformUpSettingsPinnedcan send it in the preserved mixed payload; validate against the effective per-price pin state instead.
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:426
- In the mixed legacy state, this fiat input is rendered for a pinned max-collateral value while
values.shouldPinPricesis false. The short-circuit therefore accepts a negative pinned value andtransformUpSettingsPinnedcan send it in the preserved mixed payload; validate against the effective per-price pin state instead.
!values.shouldPinPrices ||
apps/hostd/contexts/config/index.tsx:78
- This notice is derived from the pinned response without checking
pinningEnabled. Since the pinned resource is requested even when the explorer feature is disabled, a disabled host with a legacy mixed response can see an alert telling it to change pinning whileshouldPinPricesitself is hidden. IncludepinningEnabledin this condition so the notice is only shown when the host can act on it.
const showMixedPinningNotice = hasMixedPinning && !pinningTouched
apps/hostd/contexts/config/usePinningTouched.tsx:24
- Tracking touched state here is not enough to make the all-unpinned resolution savable.
ConfigActionsstill disables Save whenform.formState.isDirtyis false; toggling a legacy mixed switch false → true → false marks this field touched but restores its default value, soisDirtybecomes false. With no other edit, the host cannot submit the deliberate choice and the mixed configuration remains. ExposepinningTouchedthrough the config context and include it in the save-enabled condition (or otherwise mark this action dirty).
pinningTouched: !!touchedFields.shouldPinPrices,
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2a8f7b5 to
b1baabe
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Mixed legacy pin validation and pinning-change tracking need correction before approval.
Review details
Suppressed comments (6)
apps/hostd/contexts/config/fields.tsx:264
- For a mixed legacy host,
transformDownsetsshouldPinPricesto false even when this storage fiat input is still shown as pinned. This condition therefore short-circuits the non-negative check, allowing a negative value to be submitted for an active pinned price. Validate against the effective per-price pin state, including the legacy state, rather than only the aggregate switch.
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:313
- For a mixed legacy host,
transformDownsetsshouldPinPricesto false even when this egress fiat input is still shown as pinned. This condition therefore short-circuits the non-negative check, allowing a negative value to be submitted for an active pinned price. Validate against the effective per-price pin state, including the legacy state, rather than only the aggregate switch.
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:361
- For a mixed legacy host,
transformDownsetsshouldPinPricesto false even when this ingress fiat input is still shown as pinned. This condition therefore short-circuits the non-negative check, allowing a negative value to be submitted for an active pinned price. Validate against the effective per-price pin state, including the legacy state, rather than only the aggregate switch.
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:426
- For a mixed legacy host,
transformDownsetsshouldPinPricesto false even when this max-collateral fiat input is still shown as pinned. This condition therefore short-circuits the non-negative check, allowing a negative value to be submitted for an active pinned price. Validate against the effective per-price pin state, including the legacy state, rather than only the aggregate switch.
!values.shouldPinPrices ||
apps/hostd/contexts/config/transform.ts:313
- For a mixed legacy host,
transformDowndeliberately setsshouldPinPricestofalse, but the pinned fiat inputs for the individually pinned prices are still displayed. The range validators infields.tsxnow key off this aggregate flag, so a negative value entered into a displayed legacy-pinned field passes validation and is sent back withpinned: true; validate against the effective per-price pin state instead of the aggregate switch.
shouldPinPrices:
settingsPinned.storage.pinned &&
settingsPinned.egress.pinned &&
settingsPinned.ingress.pinned &&
settingsPinned.maxCollateral.pinned,
apps/hostd/contexts/config/usePinningTouched.tsx:24
- This treats a field blur as an explicit pinning change.
FieldSwitchpasses itsonBlurinto React Hook Form, so a user who merely tabs through the switch setstouchedFields.shouldPinPricesand the next unrelated save resolves a mixed configuration even though the switch value never changed. Track an actual checked-value change separately while retaining the on/off/on case.
pinningTouched: !!touchedFields.shouldPinPrices,
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
ChrisSchinnerl
left a comment
ChrisSchinnerl
left a comment
There was a problem hiding this comment.
Look at the deployment with Zeus as the backend and it looks great. I'll leave it up to Alex to judge code quality.
alexfreska
left a comment
alexfreska
left a comment
There was a problem hiding this comment.
Hey, this looks great. Keeping the mixed config instead of silently resolving it is the right call, and the notice makes it clear what's going on.
I think it could be simplified a fair bit by loading a mixed config as shouldPinPrices: null instead of false. The switch still shows off, but the form itself then knows the host hasn't picked yet, so most of the legacy tracking can go.
It would also close one gap. Right now a host with a mixed config can't save to all unpinned from the switch alone. Flipping it on and back off leaves 0 changes and Save disabled, so they have to edit some unrelated field before they can save. With null as the starting value, switching to off counts as a change on its own.
Left some inline notes on where, plus a couple of ideas for making the notice push hosts to resolve it.
b1baabe to
35fa2ea
Compare
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved pinning data-loading/save behavior and validation issues must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
apps/hostd/contexts/config/fields.tsx:313
- The same
!values.shouldPinPricesguard skips range validation for the pinned egress input whenever the legacy mixed state sets the global value tonull; a negative fiat value can therefore be submitted. Treat onlyfalseas the disabled case.
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:361
- The same
!values.shouldPinPricesguard skips range validation for the pinned ingress input whenever the legacy mixed state sets the global value tonull; a negative fiat value can therefore be submitted. Treat onlyfalseas the disabled case.
!values.shouldPinPrices ||
apps/hostd/contexts/config/fields.tsx:426
- The same
!values.shouldPinPricesguard skips range validation for the pinned max-collateral input whenever the legacy mixed state sets the global value tonull; a negative fiat value can therefore be submitted. Treat onlyfalseas the disabled case.
!values.shouldPinPrices ||
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
35fa2ea to
00018f1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect pinning saves and legacy mixed-state validation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
apps/hostd/contexts/config/fields.tsx:266
- When
shouldPinPricesisnullfor a legacy mixed configuration, this predicate validates the pinned value even for prices whose daemon-sidepinnedflag is false. The old per-price predicates skipped this check for those fields, so a mixed host can have a hidden, previously saved negative value that now blocks an unrelated save even thoughtransformUpSettingsPinnedintentionally preserves that configuration. Please make the range validation aware of the legacy per-price flags (or otherwise skip validation for unpinned legacy values) while still validating the price the user is actually choosing to pin.
values.shouldPinPrices === false ||
value?.gte(0) ||
'storage price must not be negative',
apps/hostd/contexts/config/fields.tsx:315
- In a legacy mixed configuration (
shouldPinPrices === null), this validates the hidden egress pinned value even when the daemon reports egress as unpinned. The previous per-price condition skipped validation in that state, so an old negative value can block unrelated saves. Please use the daemon's per-price flag for this validator, or otherwise exempt unpinned legacy values while validating values the user chooses to pin.
values.shouldPinPrices === false ||
value?.gte(0) ||
'egress price must not be negative',
apps/hostd/contexts/config/fields.tsx:363
- In a legacy mixed configuration (
shouldPinPrices === null), this validates the hidden ingress pinned value even when the daemon reports ingress as unpinned. The previous per-price condition skipped validation in that state, so an old negative value can block unrelated saves. Please use the daemon's per-price flag for this validator, or otherwise exempt unpinned legacy values while validating values the user chooses to pin.
values.shouldPinPrices === false ||
value?.gte(0) ||
'ingress price must not be negative',
apps/hostd/contexts/config/fields.tsx:428
- In a legacy mixed configuration (
shouldPinPrices === null), this validates the hidden max-collateral pinned value even when the daemon reports max collateral as unpinned. The previous per-price condition skipped validation in that state, so an old negative value can block unrelated saves. Please use the daemon's per-price flag for this validator, or otherwise exempt unpinned legacy values while validating values the user chooses to pin.
values.shouldPinPrices === false ||
value?.gte(0) ||
'max collateral must not be negative',
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
00018f1 to
5611d8c
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
A failed pinned-settings request can allow persistence while reporting a save error and leaving the form dirty.
Review details
Suppressed comments (1)
apps/hostd/contexts/config/useOnValid.tsx:50
- When the pinned-settings GET has failed, this guard skips the pinned PUT, but
onValidstill callsrevalidateAndResetFormafter the settings PATCH. With explorer enabled, that function treats the missing_settingsPinnedresponse as an error, so the settings change is already persisted while the user sees an error and the form remains dirty. Either block the save until pinned settings are available or make the revalidation path handle this intentional no-data case without reporting a partial-save failure.
if (state.data?.explorer.enabled && resources.settingsPinned.data) {
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Went ahead and implemented the |
alexfreska
left a comment
alexfreska
left a comment
There was a problem hiding this comment.
Changes look great. One small thing: a few files aren't Prettier formatted (components/Config/index.tsx, contexts/config/transform.ts, contexts/config/transform.spec.ts), so maybe run npx prettier --write on those before merging. Otherwise good to go.
5611d8c to
6b47dc5
Compare
This PR changes hostd's configuration page to support either pinning all pinnable fields or none of them, while still respecting the legacy mixed configuration. After this PR, it will not be possible through the UI to set a new mixed configuration. I think it's still possible to set a mixed config through the API, though?
Single field pinning toggles are gone. If you're in that legacy state, the overall pin toggle is set to the off position. I wrestled with that and can see arguments for having it in the on position. I'm open to whatever we think is best.
I tested this on my own host without saving anything and used clusterd beyond that, so more manual testing on a real hostd daemon would be useful before we release this into the wild.