Repository navigation
fix(admin): save a cleared number field as empty and allow decimals - #3791
Conversation
🦋 Changeset detectedLatest commit: 5ea766b The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
|
All contributors have signed the CLA ✍️ ✅ |
github-actions
Bot
added
review/needs-review
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for the two reported bugs, scoped to the admin content editor and following the same empty-as-null idiom that RepeaterField already uses. I checked the number input branch in ContentEditor.tsx, compared it to RepeaterField.tsx, traced how the value flows into formData and the onSave/onAutosave payload, and verified that ContentRepository.update serializes null for optional column writes. The added regression tests cover both behaviors behaviorally, and the changeset accurately describes the user-facing fix.
The only thing worth flagging is a deliberate trade-off acknowledged in the PR description: SERVER_FIELD_TYPE_TO_EDITOR_KIND maps both server types number and integer to the same editor kind: "number", so the new step="any" also applies to integer fields and removes the browser's integer-only validation on the Save-click path. That is the correct minimal choice for this bug fix, but it would be worth a follow-up to plumb the original field type through FieldDescriptor (or expose a step hint) so integer fields can restore step="1" while number fields keep step="any" — RepeaterField already has access to subField.type and does exactly this.
| label={<span className={labelClass}>{label}</span>} | ||
| id={id} | ||
| type="number" | ||
| step="any" |
There was a problem hiding this comment.
[suggestion] step="any" is required to fix the decimal bug, but it also affects integer fields because SERVER_FIELD_TYPE_TO_EDITOR_KIND maps both server types to the same editor kind: "number". Previously, integer fields relied on the browser's default step="1" to block decimals on Save-click; with step="any" those values now reach the server and are rejected via the invalid_type toast instead. The PR description acknowledges this, so this is a low-priority follow-up rather than a blocker: once the original field type is plumbed through FieldDescriptor (or a step hint is added), this branch can choose step={field.type === "integer" ? "1" : "any"} to match RepeaterField.
There was a problem hiding this comment.
Done in 2c2362f. The manifest field descriptor now carries an optional integer: true for integer fields (packages/core/src/api/handlers/manifest.ts, typed in packages/core/src/api/types.ts and the admin's AdminManifest), and the number input uses step={field.integer ? "1" : "any"}, the same split RepeaterField makes from subField.type. The flag is only set on integer fields, so other descriptors are unchanged.
Tests:
packages/admin/tests/components/ContentEditor.test.tsx, "rejects a decimal in an integer field":44.5is invalid and44is valid withinteger: true.packages/core/tests/unit/runtime/manifest-build.test.ts: an integer field's descriptor hasinteger: true, a string field has none.
Both fail without the change. The changeset now also lists emdash.
One caveat from checking it in the running admin: browsers measure step from min, or from the value attribute when there is no min, and React rewrites that attribute on blur. So an integer field with a min blocks 44.5 on Save, but one without a min lets the click through and the server rejects it, as before. RepeaterField's integer sub-fields have the same behaviour.
emdashbot
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
On the overlap notice: #3758 adds |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
upgrade-emdash
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
|
Thanks for your work on this and for checking the overlap with #3758. Now that it's merged, please:
Thanks! |
danielmlr
left a comment
danielmlr
left a comment
There was a problem hiding this comment.
Thanks for this, and for picking up the integer step so quickly!
The fix works and the tests do their job: if I undo the two ContentEditor.tsx changes, both new number tests fail.
There's one more place that needs the integer flag. Integer fields inside a Blocks field get their settings from nestedDescriptor() in packages/admin/src/components/BlocksField.tsx, and that never sets integer. So with this change they'd let people type decimals. I left the details inline. Could you add it there too, ideally with a small test?
You can do that together with the rebase onto main. Since #3758 landed, defaultValue sits right next to your new integer line in three places (FieldDescriptor in ContentEditor.tsx, AdminManifest in lib/api/client.ts, ManifestFieldDescriptor in core/src/api/types.ts). Just keep both lines and the conflicts are gone.
| label={<span className={labelClass}>{label}</span>} | ||
| id={id} | ||
| type="number" | ||
| step={field.integer ? "1" : "any"} |
There was a problem hiding this comment.
Fields inside a Blocks field end up here too, through BlocksField's renderField. Their settings come from nestedDescriptor() in BlocksField.tsx, which turns integer into number but drops the integer flag. So an integer field in a block would now take 4.5, and people only find out when the server says no. Adding integer?: boolean to NestedFieldDescriptor and setting it when field.type === "integer" should keep these at whole numbers.
~ 🤖 Claude Code (Opus 5.5)
Clearing a number field sent Number("") = 0, which failed a `min` of 1 or
above and otherwise stored a zero the editor never typed. The server cannot
tell that zero from a real one, so the widget now sends null for an empty
input, as the repeater's number sub-field already does.
The input also had no `step`, so the browser's default step of 1 made a
value such as 4.50 invalid and blocked the Save button with "nearest valid
values are 4 and 5", while autosave, which skips form validation, still
saved it. `step="any"` lets number fields hold decimals. Integer fields share
the "number" editor kind, so a decimal typed into one now reaches the server
and is rejected there instead of by the browser.
Claude-Session: https://claude.ai/code/session_011SKvTndrtRUPdctoaRyUw9
`step="any"` fixed decimals in number fields but also reached integer fields, because the manifest gives both the "number" editor kind. The manifest field descriptor now carries an optional `integer: true` for integer fields, and the number input uses `step="1"` for them and `step="any"` otherwise, as the repeater's number sub-fields already do from their own type. The flag is additive: other fields and consumers of the manifest are unchanged. Claude-Session: https://claude.ai/code/session_011SKvTndrtRUPdctoaRyUw9
Fields of a block reach FieldRenderer through nestedDescriptor(), which mapped integer to the number kind and dropped the flag, so with step="any" an integer in a block accepted 4.5 and only the server refused it.
2c2362f to
5ea766b
Compare
danielmlr
left a comment
danielmlr
left a comment
There was a problem hiding this comment.
Thanks!
Integer fields in blocks now reject decimals, with a test, and the rebase kept emdash-cms/emdash#3758 intact.
Looks good.
What does this PR do?
This fixes two problems with the
numberinput in the content editor.onChangesendsNumber(""), which is0. Withvalidation: { min: 1 }, every save and autosave then fails with "Price must be at least 1." Without a minimum, a0the editor never typed is silently stored. The server cannot tell this from a real zero, so the widget now sendsnullfor an empty input. This is the same idiom the repeater's number sub-field already uses (RepeaterField.tsx).step, so the browser's default step of 1 marks4.50invalid. Clicking Save shows "Please enter a valid value. The two nearest valid values are 4 and 5." and nothing is sent. Autosave skips form validation and stores the value anyway, which hides the problem. The input now hasstep="any".Reproduction on a fresh
demos/simple:/_emdash/api/setup/dev-bypass?redirect=/_emdash/admin.pricewithmin: 1.too_small.4.50and click Save. The browser blocks the submit with the "nearest valid values" message.With this change, step 4 saves
price: nulland step 5 saves4.5.No existing issue.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Two new cases in
packages/admin/tests/components/ContentEditor.test.tsxboth fail without the change:onSavewithprice: null. Without the change it is called with0.4.50leaves the input valid and is saved as4.5. Without the change the input failstoBeValid().Added in review, both failing without the integer change:
ContentEditor.test.tsx"rejects a decimal in an integer field": withinteger: true,44.5leaves the input invalid and44is valid.packages/core/tests/unit/runtime/manifest-build.test.ts: anintegerfield's descriptor hasinteger: true; a string field has nointegerkey.Results:
pnpm build,pnpm typecheck,pnpm lintandpnpm format:checkare clean.pnpm test:e2e field-widgets autosave content-crud form-data-loss: 28 passed. The full e2e suite was not run.Integer fields: they share the
numbereditor kind in the manifest, so the manifest field descriptor now carries an optionalinteger: truefor them (added in review, commit 2c2362f). The input usesstep="1"for integer fields andstep="any"for number fields, matching howRepeaterFieldpicks the step from the sub-field type. The flag is additive and only set on integer fields.A browser limitation worth knowing: the step check is measured from the input's
min, or, when there is nomin, from itsvalueattribute. React rewrites that attribute when the input loses focus, so on an integer field without amina decimal typed and then followed by a Save click is no longer flagged by the browser and is rejected by the server instead ("Seats has an invalid value."), as it was before this PR. With amin(for examplemin: 0), the browser blocks the Save click with "The two nearest valid values are 44 and 45".RepeaterField's integer sub-fields behave the same way.Also related: #3758 (open) touches
ContentEditor.tsx, but not this input.https://claude.ai/code/session_011SKvTndrtRUPdctoaRyUw9