fix(jsonforms-renderers): show required asterisk for conditionally required fields

[ginaxu1]

Summary

@jsonforms/core's isRequired() only reads the parent's plain required array, so fields that become mandatory under allOf / if / then never get the red *. This PR derives the asterisk from the form schema via AJV.

Type of change

• Bug fix

Changes made

• Shared useIsRequired hook: ORs JSONForms' static required prop with an AJV check (clone root data, unset the control path, look for a matching required / missingProperty error).
• Wired into Text, Number, Radio, Select, Boolean, Date, SearchSelect, File, Xml, and Spreadsheet controls.
• Skips the AJV path when the control is hidden, already statically required, or the root schema has no conditional-required keywords (nested / $ref follow-up: fix(jsonforms-renderers): scan nested and $ref schemas for conditional required #117).
• Playground fixture required-conditional for reviewing the asterisk under if/then.

NumberControl min/step/key blocking and constraint error wording live in #119.

Testing

• pnpm test (focused NumberControl + required utils)
• Try the required-conditional fixture in the package playground

Release notes

Fields that become required through allOf/if/then (including in nested objects and local $refs) now show the red * on text, number, date, boolean, select, radio, search-select, file, XML and spreadsheet controls, only while the condition holds.

Related issues

Refs #115 (fixed by #117)

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5577f9c6-a364-4df2-8bfe-67eb6b4bfce2

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mushrafmim]

Thanks for picking this up! I'd like to suggest a different approach before we merge.

Scenario

In the SLTB Blend Sheet Review form, the officer first picks a Review Outcome. Only when it's "Approve blend sheet & Assess Levy" are SLTB Blendsheet Reference Number and Assessed SLTB Levy (LKR) shown, and only then are they mandatory. The bug report asks for three things:

1. Show the red * on these fields when they're required.
2. Reject negative values and characters like + / - in the levy field.
3. Accept only positive amounts (e.g. 1000, 1000.50).

Concerns with options.required

• The rule ends up in two places. Whether a field is required is already defined in the JSON Schema (allOf / if / then). Adding options.required: true in the uiSchema repeats it just for the label. The two can drift: a field could show * without actually being required, or the other way round.
• Items 2 and 3 aren't addressed. NumberControl still renders <input type="number"> with no min, so -, + and e are still accepted.

Suggested approach

The JSON Schema already holds everything we need, so the renderer should read it rather than take a separate flag:

"assessedLevy": { "type": "number", "exclusiveMinimum": 0, "multipleOf": 0.01 },
"allOf": [{
  "if":   { "properties": { "reviewOutcome": { "const": "<approve-value>" } }, "required": ["reviewOutcome"] },
  "then": { "required": ["sltbReference", "assessedLevy"] }
}]

The only gap is in @jsonforms/core: its isRequired() reads just the parent's plain required array and ignores if/then (checked in 3.8.0), so ControlProps.required comes through as false. AJV's errors can't drive the * either, because the required error goes away as soon as the field is filled. Instead of a uiSchema flag:

1. Add a small shared useIsRequired hook that asks AJV. Validate a copy of the current data with this field removed, using the form's own AJV (useJsonForms().core.ajv). If AJV reports a required error whose missingProperty is this field (under the matching parent path), the field is required right now:

   const withoutField = unset(cloneDeep(rootData), path)
   validate(withoutField)
   const isRequired = validate.errors?.some(
     e => e.keyword === 'required' && e.params.missingProperty === fieldName /* + matching parent path */
   )

   This avoids parsing allOf/if/then by hand. It works for any construct (if/then/else, oneOf, dependentRequired, nesting) and agrees with validation by definition: the * shows exactly when submitting the field empty would fail. Memoize it on the root data. Text, Number, Radio and Select controls would all use it, OR-ed with the static required prop.

2. Tighten NumberControl. Pass min from schema.minimum / exclusiveMinimum. When the minimum is ≥ 0, block the -, +, e, E keys and reject pasted values containing them. Surface the exclusiveMinimum / multipleOf errors through getErrorMessage.

3. Update the form definition (outside this repo): add exclusiveMinimum: 0 (and multipleOf: 0.01 if we want at most 2 decimals) to the levy field, and make sure both fields are in then.required.

This keeps the schema as the single source of truth for validation and for the required marker.

[ginaxu1]

@mushrafmim Thanks, switched away from options.required to the approach you outlined

[mushrafmim]

Using AJV to check whether removing the field produces a required error is the right approach: the schema stays the only source of truth, and conditionals don't need to be parsed by hand. Two things before merging:

Some controls still use the old check. Boolean, Date, SearchSelect, File, Xml and Spreadsheet controls still render required && <Text color="red">*</Text>. As a result, a field that becomes required through if/then gets a * when it's a text, number, radio or select input, but not when it's a date or SearchSelect. Could we use useIsRequired in those controls too, so every control shows the asterisk the same way? (See also the performance note inline.)

[mushrafmim]

LGTM!

[mushrafmim]

LGTM!

[mushrafmim]

Thanks, both earlier comments are addressed: every control now uses useIsRequired, and the AJV check is skipped for hidden fields, statically required fields and schemas with no conditional keywords. One small non-blocking note inline that's fine to handle in a follow-up.

[sthanikan2000]

Requesting changes for the split and the description fixes below. The inline comments are marked where they are non-blocking.

@ginaxu1 thanks for the quick iterations. Before this merges, could we split it into two PRs?

The code is fine to review together (about 175 non-test lines). The problem is release notes: each package's notes list one PR as one entry under one label, and this PR carries two changes a consumer would notice:

1. Conditional-required asterisk: the useIsRequired hook, required.ts, and the wiring in all 10 controls. This is a bug fix.
2. NumberControl constraints: min/step from the schema, blocking -/+/e/E on keydown and paste, and new wording for the exclusiveMinimum/minimum/multipleOf errors.

Proposal

• #112 stays as (1). Remove from this branch the min/step/onKeyDown/onPaste hunks in NumberControl.tsx (keep the useIsRequired lines), error.ts and error.test.ts, and the second test in NumberControl.test.tsx. Retitle to fix(jsonforms-renderers): show required asterisk for conditionally required fields.
• (2) becomes a new PR from main: fix(jsonforms-renderers): NumberControl honours minimum, exclusiveMinimum and multipleOf. Whichever merges second needs a small rebase, since both touch the NumberControl.tsx imports.

Description fixes for #112

• It still says the hook is wired into "Text, Number, Radio, and Select". It's all ten controls now.
• Please drop the options.required history; it has no place in a squash commit.
• Please fill in the template's Release notes section, which the release skill reads. Suggested wording: Fields that become required through if/then, oneOf or dependentRequired now show the red * in every control, only while the condition holds.

For the second PR's release note: Number fields with a minimum/exclusiveMinimum >= 0 no longer accept -, +, e or E, and use min/step from the schema. Error messages now read "X must be greater than 0" instead of "must be > 0". The wording change is visible to anyone matching on the old strings.

Playground fixtures (as in #95 and #101): no fixture currently uses allOf/if/then. Add a required-conditional fixture to dev/fixtures.ts in #112: a Review Outcome radio at the root, with then.required covering text, number, select, date and boolean fields. In the new PR, extend the existing number fixture with a levy field (exclusiveMinimum: 0, multipleOf: 0.01).

[sthanikan2000]

@ginaxu1 thanks. I checked b1eb25b: the hardened test now fails without the handlers, the doc line is in, and #115/#116 cover the follow-ups.

What's still open from my review is the split. #112 still carries the NumberControl changes alongside the asterisk fix. #117 doesn't touch NumberControl, so it's unaffected.

Could you remove those parts here and open them as a PR from main? Then retitle #112 to fix(jsonforms-renderers): show required asterisk for conditionally required fields, drop the NumberControl bullet and the options.required history from the description, and add the Release notes section from the template.

I'll re-review right after.

[ginaxu1]

@sthanikan2000 Split done:

• fix(jsonforms-renderers): show required asterisk for conditionally required fields #112 is asterisk-only now (075b98e): NumberControl min/step/key/paste, error.ts wording, and the constraint test are removed. Added the required-conditional playground fixture. Retitled and description updated with Release notes (no options.required history; NumberControl bullet pointed at fix(jsonforms-renderers): NumberControl honours minimum, exclusiveMinimum and multipleOf #119).
• NumberControl constraints are in fix(jsonforms-renderers): NumberControl honours minimum, exclusiveMinimum and multipleOf #119 from main, including the number fixture assessedAmount field

[sthanikan2000]

Thanks @ginaxu1, the split looks right: #112 is asterisk-only, NumberControl is back to the three useIsRequired lines, and the fixture works. I rendered it: all five controls show * when approved (empty or filled) and none on reject.

I edited the description, so please don't overwrite it:

• Release notes. They said fields become required through if/then, oneOf or dependentRequired. I checked each against JSONForms' createAjv: if/then works; dependentRequired reports nothing, so it isn't enforced and no * shows; dependencies is enforced but reported under its own keyword, so useIsRequired ignores it; oneOf alternatives ([{required:['a']},{required:['b']}]) put a * on both fields. The note now names only allOf/if/then and the ten controls that are wired.
• Closes #115 (via #117) is now Refs #115 (fixed by #117), so #115 doesn't close before the nested fix lands.

Please merge #117 before the version bump: the nested/$ref part of the Release notes is only true once it's in. Suggested order: #112, #117, #119 (the last two will each need a rebase onto main).

Non-blocking follow-up: dependencies is enforced by AJV but ignored by the hook, because AJV reports it under the dependencies keyword instead of required.