feat(renderers): add currency/file renderers, filler generators, and list sub-schema modal#6
Open
feat(renderers): add currency/file renderers, filler generators, and list sub-schema modal#6
Conversation
…list sub-schema modal
📝 WalkthroughWalkthroughThis pull request introduces three new field renderer components (CurrencyField, FileField, ListField modal editing) across four UI frameworks (React Native, React Web, SvelteKit, Vue Quasar), adds support for additional filler types in the core package, and refactors ListFieldDefinition to apply schema transformation via provide(). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ListField
participant Modal as ItemFormModal
participant Form as useDataForm
participant Renderer as FieldRenderer
participant Parent
User->>ListField: Click "Add" or "Edit"
ListField->>ListField: Determine itemSchema
ListField->>Modal: Open modal with itemSchema
Modal->>Form: Initialize form with schema
Form->>Renderer: Request field renderers
Renderer->>Modal: Return rendered fields
Modal->>User: Display form in modal
User->>Modal: Fill form and click "OK"
Modal->>Form: Validate form data
Form-->>Modal: Validation result
Modal->>ListField: Emit save with values
ListField->>Parent: Call onChange with updated items
ListField->>ListField: Reset modal state
ListField->>User: Close modal, update list display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
packages/core/src/filler.test.ts (1)
166-170: Strengthen the time filler test to validate real HH:MM bounds.Line 169’s regex accepts invalid values like
29:99, so the test can still pass with a broken generator.♻️ Proposed test tightening
it('generates time string for time', () => { const value = defaultFillers.time(makeFieldConfig({ component: 'time' })) expect(value).toBeTypeOf('string') expect(value as string).toMatch(/^\d{2}:\d{2}$/) + const [hh, mm] = (value as string).split(':').map(Number) + expect(hh).toBeGreaterThanOrEqual(0) + expect(hh).toBeLessThanOrEqual(23) + expect(mm).toBeGreaterThanOrEqual(0) + expect(mm).toBeLessThanOrEqual(59) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/filler.test.ts` around lines 166 - 170, The current test for defaultFillers.time in filler.test.ts only checks format via /^\d{2}:\d{2}$/ which allows invalid times; update the it('generates time string for time', ...) test for defaultFillers.time(makeFieldConfig({ component: 'time' })) to assert valid HH:MM bounds by either using a stricter regex that enforces 00–23 for hours and 00–59 for minutes (e.g. /^(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$/) or by parsing the produced string into hour and minute numbers and asserting 0 <= hour <= 23 and 0 <= minute <= 59 to ensure generated times are real.packages/sveltekit/src/renderers/ListField.svelte (2)
21-33: Inconsistent$derivedusage — some are values, some are getter functions.
label(Line 21) anditemSchema(Line 28) use$derived(() => ...)producing getter functions (called aslabel(),itemSchema()), whileitems(Line 26) andreorderable(Line 27) use$derived(expr)producing direct reactive values. This inconsistency is error-prone — if someone later accessesitems()orlabelwithout parentheses, they'll get silent bugs.Consider making them all consistent. In Svelte 5,
$derived(expr)is the standard form; the arrow-function variant is typically unnecessary unless you need lazy evaluation.♻️ Normalize to $derived(expr) for consistency
- let label = $derived(() => { + let label = $derived((() => { const key = `${domain}.fields.${name}` return hasTranslation(key) ? translate(key) : name - }) + })())Or more idiomatically, use an IIFE or keep a simple expression. Alternatively, switch all to the function form and call them consistently. The key is to pick one pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sveltekit/src/renderers/ListField.svelte` around lines 21 - 33, Normalize all $derived usages so they consistently return reactive values (not getter functions): replace the arrow-function form $derived(() => ...) used by label and itemSchema with the expression form $derived(expr) like items and reorderable, i.e., compute label from the expression `${domain}.fields.${name}` using hasTranslation/translate and compute itemSchema by evaluating config.attrs.itemSchema and returning the SchemaProvide-like object directly; keep items as $derived(Array.isArray(value) ? (value as Record<string, unknown>[]) : []) and reorderable as $derived(config.attrs.reorderable === true) so all four (label, items, reorderable, itemSchema) behave the same and callers can access them as plain reactive values.
157-183: Modal accessibility: the overlay traps focus visually but not programmatically.The modal uses
role="dialog"andtabindex="-1"on the overlay (good), and handles Escape viaonkeydown. However, there's no focus trap — keyboard users can Tab out of the modal into the background content. This is a known accessibility gap for custom modals.For now this is acceptable, but consider adding a focus-trap utility in a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sveltekit/src/renderers/ListField.svelte` around lines 157 - 183, The modal overlay (modal-overlay with role="dialog" controlled by modalOpen and cancelModal) visually traps users but does not programmatically trap focus, allowing Tab to move focus outside the modal; add a focus-trap when modalOpen is true by mounting a focus-trap instance (or equivalent hook) on the modal-card element returned by the {`#if` modalOpen && form} block, ensure initial focus moves into the modal (e.g., first focusable control), cycle Tab/Shift+Tab within modal-card, capture Escape via existing cancelModal, and restore focus to the element that opened the modal when the modal closes (use editIndex, saveEdit, saveNew hooks to ensure cleanup).packages/react-web/src/renderers/ListField.tsx (2)
51-83: Modal lacks keyboard dismiss (Escape key) — accessibility gap.The Svelte variant explicitly handles
Escapekey on the overlay (onkeydown), and the Vue Quasar variant relies onq-dialog's built-in keyboard handling. This React portal modal has noonKeyDownhandler, so pressing Escape does nothing. Screen-reader and keyboard-only users will have no way to dismiss the modal without clicking Cancel.♻️ Add Escape key handling to the overlay
return createPortal( - <div style={modalStyles.overlay} onClick={onCancel}> + <div style={modalStyles.overlay} onClick={onCancel} onKeyDown={(e) => e.key === 'Escape' && onCancel()} tabIndex={-1}> <div style={modalStyles.card} onClick={(e) => e.stopPropagation()}>You may also want to auto-focus the overlay on mount so it receives keyboard events without requiring a click first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/renderers/ListField.tsx` around lines 51 - 83, Add Escape-key dismissal to the modal by making the overlay focusable and handling key events: in the createPortal return, add a keydown handler on the overlay div (the one using modalStyles.overlay) that checks for Escape and calls onCancel, ensure the overlay can receive focus by adding a tabIndex (e.g., 0 or -1) or programmatically focus it on mount (use a ref and useEffect) so the keydown is captured; keep existing onClick/onCancel behavior and reuse the existing onCancel callback used by the cancel button and overlay click.
45-49:form.statepassed by reference — consider spreading for safety.Line 47 passes
form.statedirectly toonSave. Ifform.stateis a mutable object that could be modified after the modal unmounts, the consumer may see stale or corrupted data. The Vue Quasar variant uses{ ...form.state }to create a defensive copy.♻️ Proposed fix
const handleSave = () => { if (form.validate()) { - onSave(form.state); + onSave({ ...form.state }); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-web/src/renderers/ListField.tsx` around lines 45 - 49, In handleSave(), don't pass the mutable form.state object by reference to onSave; instead create and pass a defensive copy (e.g., spread into a new object) after successful validation (form.validate()) so downstream consumers can't mutate the original state; update the call in handleSave to pass a cloned object derived from form.state (referencing handleSave, onSave, form.state, and form.validate).packages/sveltekit/src/renderers/CurrencyField.svelte (1)
16-23: Same$derivedinconsistency as ListField.svelte —labelis a getter, others are direct values.
labeluses$derived(() => ...)(called aslabel()on Line 28), whileprefix,precision, andstepuse$derived(expr). See the comment onListField.svelteLines 21–33 for the same concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sveltekit/src/renderers/CurrencyField.svelte` around lines 16 - 23, label is defined as a $derived getter (used as label()), while prefix, precision and step are defined as direct values, causing inconsistency; make prefix, precision and step use the same getter form as label by wrapping their expressions in $derived(() => ... ) so they become callable stores too (update the declarations for prefix, precision, step to use $derived with arrow functions to match label).packages/react-native/src/renderers/FileField.tsx (2)
11-11:createStyles(theme)is called on every render; consider memoizing.
StyleSheet.createdoes not cache across calls — this allocates a new style object on each render. Wrapping inuseMemois the idiomatic React Native fix.♻️ Proposed refactor
- const styles = createStyles(theme); + const styles = useMemo(() => createStyles(theme), [theme]);(Also applies identically to
CurrencyField.tsxline 11.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/renderers/FileField.tsx` at line 11, The styles object is recreated on every render because createStyles(theme) is called directly; wrap the call in React.useMemo to memoize it based on the theme so StyleSheet.create isn't invoked each render. Update the FileField component (and similarly CurrencyField) to compute styles using useMemo(() => createStyles(theme), [theme]) so the styles variable is stable across renders and only recomputed when theme changes.
22-30:Pressablehas noonPresshandler — file selection is entirely non-functional.The
onChangeprop is not destructured and there is no document-picker call wired up, so tapping the button does nothing. The TODO documents this, but it is worth tracking.Would you like me to open an issue or draft the expo-document-picker / react-native-document-picker integration?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/renderers/FileField.tsx` around lines 22 - 30, The Pressable in FileField is missing an onPress handler and the component never uses the onChange prop, so tapping does nothing; update the FileField component to destructure onChange from props and add an onPress on the Pressable (or a helper like handleChooseFile) that calls a document picker (expo-document-picker or react-native-document-picker) to let the user select a file/image, then invoke onChange with the selected file data and respect proxy.disabled; wire this into the existing isImage conditional UI and ensure the handler handles cancelation/errors before calling onChange.packages/react-native/src/renderers/CurrencyField.tsx (1)
19-19: Label styling doesn't reflect error state — minor inconsistency with the React Web counterpart.
react-web/CurrencyField.tsxapplies alabelErrorstyle (red foreground) to the label whenerrors.length > 0. The RN implementation renders the label with a constant style. Consider adding the same visual cue for platform consistency.♻️ Proposed refactor
+ const hasError = errors.length > 0; ... - <Text style={styles.label}>{fieldLabel}</Text> + <Text style={[styles.label, hasError && styles.labelError]}>{fieldLabel}</Text>Add to
createStyles:+ labelError: { + color: theme.colors.destructive, + },Also applies to: 49-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native/src/renderers/CurrencyField.tsx` at line 19, The label Text currently always uses styles.label; update CurrencyField.tsx to conditionally apply styles.labelError when errors.length > 0 (same logic as react-web/CurrencyField.tsx) so the label shows the error color; to implement this, add a labelError style in createStyles and change the label render (the Text showing fieldLabel) to use [styles.label, errors.length > 0 && styles.labelError] (or equivalent conditional merge) so the label reflects the error state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-native/src/renderers/CurrencyField.tsx`:
- Around line 25-28: When the user clears the field Number("") evaluates to 0 so
onChangeText currently calls onChange(0); modify the onChangeText handler in
CurrencyField.tsx (and mirror the same change in react-web/CurrencyField.tsx) to
treat an empty string explicitly: if text.trim() === "" call onChange(null) (or
your chosen empty sentinel) else parse Number(text) and call onChange(isNaN(num)
? text : num) so an erased field is distinguishable from an explicit 0.
In `@packages/react-web/src/renderers/CurrencyField.tsx`:
- Around line 34-37: In CurrencyField.tsx the inline onChange handler coerces an
empty string to 0 via Number(e.target.value), so clearing the input writes 0
instead of empty; update the handler used in the CurrencyField component to
explicitly check e.target.value === '' (or trim() === '') and call the onChange
prop with an empty string or undefined in that case, otherwise parse to Number
and fall back to the original value when NaN — keep references to the existing
onChange prop, e.target.value and the Number(...) conversion so you only change
the branching logic.
In `@packages/react-web/src/renderers/FileField.tsx`:
- Line 8: The FileField renderer does not forward focus/blur handlers from
FieldRendererProps to the underlying input, so add forwarding of the relevant
props (e.g., onFocus, onBlur, onFocusCapture, onBlurCapture or whatever names
are defined on FieldRendererProps) to the <input type="file"> inside the
FileField function; update FileField to destructure those handler props from the
props object and pass them through to the input element (also apply the same
change to the other similar renderer block referenced around the FileField
implementation, lines 43-53).
- Line 13: The FileField component currently returns early when proxy.hidden
which skips calling useMemo and useEffect and thereby breaks React hook
ordering; update FileField so hooks (the useMemo at the former line 21 and
useEffect at line 26) are invoked unconditionally by computing a local isHidden
variable from proxy.hidden and performing the early return/render conditional
after the hooks run. Also add onBlur and onFocus to the component
props/signature and forward those handlers to the underlying input element
(match TextField’s prop names and forwarding behavior) so blur/focus events are
handled correctly.
In `@packages/sveltekit/src/renderers/FileField.svelte`:
- Around line 4-14: The FileField component declares onBlur and onFocus props
but never forwards them to the underlying file input; update the file input
element in FileField.svelte (the element handling change events in the block
that currently emits only change) to also call the onBlur and onFocus callbacks
(e.g., add on:blur={() => onBlur()} and on:focus={() => onFocus()} or
equivalent) so that focus/blur state is propagated; keep the existing onChange
handling intact and ensure the callbacks are only called if provided.
In `@packages/vue-quasar/src/renderers/CurrencyField.vue`:
- Line 14: The current model update handler directly does
props.onChange(Number($event)) which can write NaN; replace that inline
expression with a small guarded handler (e.g., implement handleModelUpdate) that
treats '' and null as undefined, attempts Number(raw) for non-empty values, and
only passes the numeric value when Number.isNaN(num) is false—otherwise pass the
original raw value (or undefined for empty) to props.onChange to avoid
persisting NaN; update the template to call this handler instead of calling
props.onChange(Number(...)) directly.
In `@packages/vue-quasar/src/renderers/FileField.vue`:
- Around line 49-53: The change handler handleChange currently reads the
selected file and calls props.onChange but doesn't clear the native input value,
so selecting the same file again won't emit a change event; after calling
props.onChange(file ?? null) reset the file input's value (e.g., fileInput.value
= '') so the browser will fire change on re-selecting the same file; locate
handleChange and the file input ref (fileInput) and add the value reset
immediately after invoking props.onChange.
- Around line 9-12: Replace the hardcoded English strings in FileField.vue with
i18n lookups: use the component's i18n function (e.g., useI18n() or $t) in the
setup and change the :label binding to call t("common.file.choose", { type:
isImage ? "image" : "file" }) instead of "Choose image…" / "Choose file…", and
replace the span content to t("common.file.none") when fileName is falsy; update
any references to fileInput, fileName, and isImage in the template accordingly
so the label and fallback text are localized.
---
Nitpick comments:
In `@packages/core/src/filler.test.ts`:
- Around line 166-170: The current test for defaultFillers.time in
filler.test.ts only checks format via /^\d{2}:\d{2}$/ which allows invalid
times; update the it('generates time string for time', ...) test for
defaultFillers.time(makeFieldConfig({ component: 'time' })) to assert valid
HH:MM bounds by either using a stricter regex that enforces 00–23 for hours and
00–59 for minutes (e.g. /^(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$/) or by parsing
the produced string into hour and minute numbers and asserting 0 <= hour <= 23
and 0 <= minute <= 59 to ensure generated times are real.
In `@packages/react-native/src/renderers/CurrencyField.tsx`:
- Line 19: The label Text currently always uses styles.label; update
CurrencyField.tsx to conditionally apply styles.labelError when errors.length >
0 (same logic as react-web/CurrencyField.tsx) so the label shows the error
color; to implement this, add a labelError style in createStyles and change the
label render (the Text showing fieldLabel) to use [styles.label, errors.length >
0 && styles.labelError] (or equivalent conditional merge) so the label reflects
the error state.
In `@packages/react-native/src/renderers/FileField.tsx`:
- Line 11: The styles object is recreated on every render because
createStyles(theme) is called directly; wrap the call in React.useMemo to
memoize it based on the theme so StyleSheet.create isn't invoked each render.
Update the FileField component (and similarly CurrencyField) to compute styles
using useMemo(() => createStyles(theme), [theme]) so the styles variable is
stable across renders and only recomputed when theme changes.
- Around line 22-30: The Pressable in FileField is missing an onPress handler
and the component never uses the onChange prop, so tapping does nothing; update
the FileField component to destructure onChange from props and add an onPress on
the Pressable (or a helper like handleChooseFile) that calls a document picker
(expo-document-picker or react-native-document-picker) to let the user select a
file/image, then invoke onChange with the selected file data and respect
proxy.disabled; wire this into the existing isImage conditional UI and ensure
the handler handles cancelation/errors before calling onChange.
In `@packages/react-web/src/renderers/ListField.tsx`:
- Around line 51-83: Add Escape-key dismissal to the modal by making the overlay
focusable and handling key events: in the createPortal return, add a keydown
handler on the overlay div (the one using modalStyles.overlay) that checks for
Escape and calls onCancel, ensure the overlay can receive focus by adding a
tabIndex (e.g., 0 or -1) or programmatically focus it on mount (use a ref and
useEffect) so the keydown is captured; keep existing onClick/onCancel behavior
and reuse the existing onCancel callback used by the cancel button and overlay
click.
- Around line 45-49: In handleSave(), don't pass the mutable form.state object
by reference to onSave; instead create and pass a defensive copy (e.g., spread
into a new object) after successful validation (form.validate()) so downstream
consumers can't mutate the original state; update the call in handleSave to pass
a cloned object derived from form.state (referencing handleSave, onSave,
form.state, and form.validate).
In `@packages/sveltekit/src/renderers/CurrencyField.svelte`:
- Around line 16-23: label is defined as a $derived getter (used as label()),
while prefix, precision and step are defined as direct values, causing
inconsistency; make prefix, precision and step use the same getter form as label
by wrapping their expressions in $derived(() => ... ) so they become callable
stores too (update the declarations for prefix, precision, step to use $derived
with arrow functions to match label).
In `@packages/sveltekit/src/renderers/ListField.svelte`:
- Around line 21-33: Normalize all $derived usages so they consistently return
reactive values (not getter functions): replace the arrow-function form
$derived(() => ...) used by label and itemSchema with the expression form
$derived(expr) like items and reorderable, i.e., compute label from the
expression `${domain}.fields.${name}` using hasTranslation/translate and compute
itemSchema by evaluating config.attrs.itemSchema and returning the
SchemaProvide-like object directly; keep items as $derived(Array.isArray(value)
? (value as Record<string, unknown>[]) : []) and reorderable as
$derived(config.attrs.reorderable === true) so all four (label, items,
reorderable, itemSchema) behave the same and callers can access them as plain
reactive values.
- Around line 157-183: The modal overlay (modal-overlay with role="dialog"
controlled by modalOpen and cancelModal) visually traps users but does not
programmatically trap focus, allowing Tab to move focus outside the modal; add a
focus-trap when modalOpen is true by mounting a focus-trap instance (or
equivalent hook) on the modal-card element returned by the {`#if` modalOpen &&
form} block, ensure initial focus moves into the modal (e.g., first focusable
control), cycle Tab/Shift+Tab within modal-card, capture Escape via existing
cancelModal, and restore focus to the element that opened the modal when the
modal closes (use editIndex, saveEdit, saveNew hooks to ensure cleanup).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/core/src/fields/list.tspackages/core/src/filler.test.tspackages/core/src/filler.tspackages/react-native/src/renderers/CurrencyField.tsxpackages/react-native/src/renderers/FileField.tsxpackages/react-native/src/renderers/ListField.tsxpackages/react-native/src/renderers/index.tspackages/react-web/src/renderers/CurrencyField.tsxpackages/react-web/src/renderers/FileField.tsxpackages/react-web/src/renderers/ListField.tsxpackages/react-web/src/renderers/index.tspackages/sveltekit/src/renderers/CurrencyField.sveltepackages/sveltekit/src/renderers/FileField.sveltepackages/sveltekit/src/renderers/ListField.sveltepackages/sveltekit/src/renderers/index.tspackages/vue-quasar/src/renderers/CurrencyField.vuepackages/vue-quasar/src/renderers/FileField.vuepackages/vue-quasar/src/renderers/ListField.vuepackages/vue-quasar/src/renderers/ListItemFormModal.vuepackages/vue-quasar/src/renderers/index.ts
Comment on lines
+25
to
+28
| onChangeText={(text) => { | ||
| const num = Number(text); | ||
| onChange(isNaN(num) ? text : num); | ||
| }} |
There was a problem hiding this comment.
Clearing the input silently produces onChange(0) instead of an empty/null value.
Number("") === 0 and isNaN(0) === false, so when the user erases all digits, onChange(0) is called. A cleared currency field and an explicitly entered 0 are indistinguishable downstream. The same behavior exists in react-web/CurrencyField.tsx, so if this is intentional parity, it should be explicitly documented; otherwise, guard for the empty string:
🐛 Proposed fix
- onChangeText={(text) => {
- const num = Number(text);
- onChange(isNaN(num) ? text : num);
- }}
+ onChangeText={(text) => {
+ if (text === "") {
+ onChange(undefined);
+ return;
+ }
+ const num = Number(text);
+ onChange(isNaN(num) ? text : num);
+ }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-native/src/renderers/CurrencyField.tsx` around lines 25 - 28,
When the user clears the field Number("") evaluates to 0 so onChangeText
currently calls onChange(0); modify the onChangeText handler in
CurrencyField.tsx (and mirror the same change in react-web/CurrencyField.tsx) to
treat an empty string explicitly: if text.trim() === "" call onChange(null) (or
your chosen empty sentinel) else parse Number(text) and call onChange(isNaN(num)
? text : num) so an erased field is distinguishable from an explicit 0.
Comment on lines
+34
to
+37
| onChange={(e) => { | ||
| const num = Number(e.target.value); | ||
| onChange(isNaN(num) ? e.target.value : num); | ||
| }} |
There was a problem hiding this comment.
Handle empty input explicitly before numeric coercion.
Line 35-37 converts '' to 0, so clearing the field writes 0 rather than an empty/undefined value.
🐛 Proposed fix
onChange={(e) => {
- const num = Number(e.target.value);
- onChange(isNaN(num) ? e.target.value : num);
+ const raw = e.target.value;
+ if (raw === "") {
+ onChange(undefined);
+ return;
+ }
+ const num = Number(raw);
+ onChange(Number.isNaN(num) ? raw : num);
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onChange={(e) => { | |
| const num = Number(e.target.value); | |
| onChange(isNaN(num) ? e.target.value : num); | |
| }} | |
| onChange={(e) => { | |
| const raw = e.target.value; | |
| if (raw === "") { | |
| onChange(undefined); | |
| return; | |
| } | |
| const num = Number(raw); | |
| onChange(Number.isNaN(num) ? raw : num); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/renderers/CurrencyField.tsx` around lines 34 - 37, In
CurrencyField.tsx the inline onChange handler coerces an empty string to 0 via
Number(e.target.value), so clearing the input writes 0 instead of empty; update
the handler used in the CurrencyField component to explicitly check
e.target.value === '' (or trim() === '') and call the onChange prop with an
empty string or undefined in that case, otherwise parse to Number and fall back
to the original value when NaN — keep references to the existing onChange prop,
e.target.value and the Number(...) conversion so you only change the branching
logic.
| import type { Theme } from "../theme/default"; | ||
| import { ds } from "../support/ds"; | ||
|
|
||
| export function FileField({ domain, name, value, config, proxy, errors, onChange }: FieldRendererProps) { |
There was a problem hiding this comment.
Forward blur/focus handlers from FieldRendererProps to the input.
This renderer only emits change events today, so focus/touched-driven form behavior can become inconsistent.
🐛 Proposed fix
-export function FileField({ domain, name, value, config, proxy, errors, onChange }: FieldRendererProps) {
+export function FileField({ domain, name, value, config, proxy, errors, onChange, onBlur, onFocus }: FieldRendererProps) {
@@
<input
ref={inputRef}
type="file"
accept={accept ?? (isImage ? "image/*" : undefined)}
style={{ display: "none" }}
onChange={(e) => {
const file = e.target.files?.[0];
onChange(file ?? null);
}}
+ onBlur={onBlur}
+ onFocus={onFocus}
disabled={proxy.disabled}
/>Also applies to: 43-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/renderers/FileField.tsx` at line 8, The FileField
renderer does not forward focus/blur handlers from FieldRendererProps to the
underlying input, so add forwarding of the relevant props (e.g., onFocus,
onBlur, onFocusCapture, onBlurCapture or whatever names are defined on
FieldRendererProps) to the <input type="file"> inside the FileField function;
update FileField to destructure those handler props from the props object and
pass them through to the input element (also apply the same change to the other
similar renderer block referenced around the FileField implementation, lines
43-53).
| const theme = useTheme(); | ||
| const styles = createStyles(theme); | ||
| const inputRef = useRef<HTMLInputElement>(null); | ||
| if (proxy.hidden) return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/react-web/src/renderers/FileField.tsx | head -70Repository: devitools/ybyra
Length of output: 3048
🏁 Script executed:
rg -n "onBlur|onFocus" packages/react-web/src/renderers/FileField.tsx
rg -n "FieldRendererProps" packages/react-web/src/renderers/FileField.tsx -A 5Repository: devitools/ybyra
Length of output: 559
🏁 Script executed:
# Find FieldRendererProps type definition
fd -e ts -e tsx | xargs rg -l "type FieldRendererProps|interface FieldRendererProps" | head -5
# Check other field renderers to see the pattern
fd "renderers.*\\.tsx$" packages/react-web/src/renderers/ | head -10Repository: devitools/ybyra
Length of output: 141
🏁 Script executed:
# Check FieldRendererProps type definition
rg -A 10 "type FieldRendererProps" packages/react/src/types.ts
# List other renderer files to compare implementations
ls -la packages/react-web/src/renderers/ | grep -E "\.tsx$"Repository: devitools/ybyra
Length of output: 847
🏁 Script executed:
# Get FieldRendererProps type definition
rg -A 10 "type FieldRendererProps" packages/react/src/types.ts
# Check another renderer to see if it has the same pattern
cat -n packages/react-web/src/renderers/TextField.tsx | head -30Repository: devitools/ybyra
Length of output: 1642
🏁 Script executed:
# Verify FieldRendererProps definition
rg -B 2 -A 10 "type FieldRendererProps" packages/react/src/types.tsRepository: devitools/ybyra
Length of output: 41
🏁 Script executed:
# Search for FieldRendererProps more broadly
rg "FieldRendererProps" packages/react/src/types.ts -B 2 -A 10
# Or search across all files
rg "type FieldRendererProps|interface FieldRendererProps" packages/ -B 2 -A 10Repository: devitools/ybyra
Length of output: 2548
🏁 Script executed:
# Check if FileField forwards onBlur/onFocus to the input
cat -n packages/react-web/src/renderers/FileField.tsx | grep -E "(onBlur|onFocus)"
# Check input element in FileField
cat -n packages/react-web/src/renderers/FileField.tsx | sed -n '43,53p'Repository: devitools/ybyra
Length of output: 469
Fix hook ordering and missing event handlers.
Hooks must be called unconditionally in consistent order. Currently, proxy.hidden returns early before useMemo (line 21) and useEffect (line 26) execute, violating React's hooks rule. Additionally, onBlur and onFocus props are missing from the function signature and are not forwarded to the input element (compare with TextField which correctly includes them).
🐛 Proposed fix
-export function FileField({ domain, name, value, config, proxy, errors, onChange }: FieldRendererProps) {
+export function FileField({ domain, name, value, config, proxy, errors, onChange, onBlur, onFocus }: FieldRendererProps) {
const { t } = useTranslation();
const theme = useTheme();
const styles = createStyles(theme);
const inputRef = useRef<HTMLInputElement>(null);
- if (proxy.hidden) return null;
const fieldLabel = t(`${domain}.fields.${name}`, { defaultValue: name });
const hasError = errors.length > 0;
const accept = (config.attrs.accept as string) ?? undefined;
const isImage = config.component === "image";
const fileName = value instanceof File ? value.name : (value ? String(value) : "");
const previewUrl = useMemo(() => {
if (value instanceof File && isImage) return URL.createObjectURL(value);
return null;
}, [value, isImage]);
useEffect(() => {
return () => { if (previewUrl) URL.revokeObjectURL(previewUrl); };
}, [previewUrl]);
+
+ if (proxy.hidden) return null;
return (
<div style={styles.container} {...ds(`FileField:${name}`)}>
<label style={{ ...styles.label, ...(hasError ? styles.labelError : {}) }}>{fieldLabel}</label>
<div style={styles.inputWrapper}>
<button
type="button"
style={{ ...styles.chooseBtn, ...(proxy.disabled ? styles.chooseBtnDisabled : {}) }}
onClick={() => inputRef.current?.click()}
disabled={proxy.disabled}
>
{t("common.file.choose", { defaultValue: isImage ? "Choose image…" : "Choose file…" })}
</button>
<span style={styles.fileName}>{fileName || t("common.file.none", { defaultValue: "No file selected" })}</span>
<input
ref={inputRef}
type="file"
accept={accept ?? (isImage ? "image/*" : undefined)}
style={{ display: "none" }}
onChange={(e) => {
const file = e.target.files?.[0];
onChange(file ?? null);
}}
+ onBlur={onBlur}
+ onFocus={onFocus}
disabled={proxy.disabled}
/>
</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (proxy.hidden) return null; | |
| export function FileField({ domain, name, value, config, proxy, errors, onChange, onBlur, onFocus }: FieldRendererProps) { | |
| const { t } = useTranslation(); | |
| const theme = useTheme(); | |
| const styles = createStyles(theme); | |
| const inputRef = useRef<HTMLInputElement>(null); | |
| const fieldLabel = t(`${domain}.fields.${name}`, { defaultValue: name }); | |
| const hasError = errors.length > 0; | |
| const accept = (config.attrs.accept as string) ?? undefined; | |
| const isImage = config.component === "image"; | |
| const fileName = value instanceof File ? value.name : (value ? String(value) : ""); | |
| const previewUrl = useMemo(() => { | |
| if (value instanceof File && isImage) return URL.createObjectURL(value); | |
| return null; | |
| }, [value, isImage]); | |
| useEffect(() => { | |
| return () => { if (previewUrl) URL.revokeObjectURL(previewUrl); }; | |
| }, [previewUrl]); | |
| if (proxy.hidden) return null; | |
| return ( | |
| <div style={styles.container} {...ds(`FileField:${name}`)}> | |
| <label style={{ ...styles.label, ...(hasError ? styles.labelError : {}) }}>{fieldLabel}</label> | |
| <div style={styles.inputWrapper}> | |
| <button | |
| type="button" | |
| style={{ ...styles.chooseBtn, ...(proxy.disabled ? styles.chooseBtnDisabled : {}) }} | |
| onClick={() => inputRef.current?.click()} | |
| disabled={proxy.disabled} | |
| > | |
| {t("common.file.choose", { defaultValue: isImage ? "Choose image…" : "Choose file…" })} | |
| </button> | |
| <span style={styles.fileName}>{fileName || t("common.file.none", { defaultValue: "No file selected" })}</span> | |
| <input | |
| ref={inputRef} | |
| type="file" | |
| accept={accept ?? (isImage ? "image/*" : undefined)} | |
| style={{ display: "none" }} | |
| onChange={(e) => { | |
| const file = e.target.files?.[0]; | |
| onChange(file ?? null); | |
| }} | |
| onBlur={onBlur} | |
| onFocus={onFocus} | |
| disabled={proxy.disabled} | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-web/src/renderers/FileField.tsx` at line 13, The FileField
component currently returns early when proxy.hidden which skips calling useMemo
and useEffect and thereby breaks React hook ordering; update FileField so hooks
(the useMemo at the former line 21 and useEffect at line 26) are invoked
unconditionally by computing a local isHidden variable from proxy.hidden and
performing the early return/render conditional after the hooks run. Also add
onBlur and onFocus to the component props/signature and forward those handlers
to the underlying input element (match TextField’s prop names and forwarding
behavior) so blur/focus events are handled correctly.
Comment on lines
+4
to
+14
| let { domain, name, value, config, proxy, errors, onChange }: { | ||
| domain: string | ||
| name: string | ||
| value: unknown | ||
| config: { component: string; attrs: Record<string, unknown> } | ||
| proxy: { hidden: boolean; disabled: boolean } | ||
| errors: string[] | ||
| onChange: (v: unknown) => void | ||
| onBlur: () => void | ||
| onFocus: () => void | ||
| } = $props() |
There was a problem hiding this comment.
Wire onBlur/onFocus through the file input.
Line 47-53 only emits change events; blur/focus callbacks are declared but never forwarded, so touched/focus state can drift.
🐛 Proposed fix
-let { domain, name, value, config, proxy, errors, onChange }: {
+let { domain, name, value, config, proxy, errors, onChange, onBlur, onFocus }: {
domain: string
name: string
value: unknown
config: { component: string; attrs: Record<string, unknown> }
proxy: { hidden: boolean; disabled: boolean }
errors: string[]
onChange: (v: unknown) => void
onBlur: () => void
onFocus: () => void
} = $props()
@@
<input
bind:this={fileInput}
type="file"
{accept}
style="display:none"
onchange={handleChange}
+ onblur={onBlur}
+ onfocus={onFocus}
disabled={proxy.disabled}
/>Also applies to: 47-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sveltekit/src/renderers/FileField.svelte` around lines 4 - 14, The
FileField component declares onBlur and onFocus props but never forwards them to
the underlying file input; update the file input element in FileField.svelte
(the element handling change events in the block that currently emits only
change) to also call the onBlur and onFocus callbacks (e.g., add on:blur={() =>
onBlur()} and on:focus={() => onFocus()} or equivalent) so that focus/blur state
is propagated; keep the existing onChange handling intact and ensure the
callbacks are only called if provided.
| type="number" | ||
| outlined | ||
| dense | ||
| @update:model-value="props.onChange($event === '' ? undefined : Number($event))" |
There was a problem hiding this comment.
Guard numeric conversion to avoid NaN state writes.
Line 14 currently sends Number($event) for any non-empty value, which can propagate NaN during intermediate input states.
🐛 Proposed fix
- `@update`:model-value="props.onChange($event === '' ? undefined : Number($event))"
+ `@update`:model-value="handleModelUpdate"const handleModelUpdate = (raw: string | number | null) => {
if (raw === '' || raw === null) {
props.onChange(undefined)
return
}
const num = Number(raw)
props.onChange(Number.isNaN(num) ? raw : num)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue-quasar/src/renderers/CurrencyField.vue` at line 14, The current
model update handler directly does props.onChange(Number($event)) which can
write NaN; replace that inline expression with a small guarded handler (e.g.,
implement handleModelUpdate) that treats '' and null as undefined, attempts
Number(raw) for non-empty values, and only passes the numeric value when
Number.isNaN(num) is false—otherwise pass the original raw value (or undefined
for empty) to props.onChange to avoid persisting NaN; update the template to
call this handler instead of calling props.onChange(Number(...)) directly.
Comment on lines
+9
to
+12
| :label="isImage ? 'Choose image…' : 'Choose file…'" | ||
| @click="fileInput?.click()" | ||
| /> | ||
| <span class="text-caption text-grey">{{ fileName || 'No file selected' }}</span> |
There was a problem hiding this comment.
Button label and fallback text bypass i18n — inconsistent with other framework implementations.
"Choose image…", "Choose file…", and "No file selected" are hardcoded English strings. The React Native FileField passes these through t("common.file.choose", ...) and t("common.file.none", ...). This component should do the same for consistency and localization support.
🌐 Proposed fix
- :label="isImage ? 'Choose image…' : 'Choose file…'"
+ :label="isImage ? t('common.file.choose', 'Choose image…') : t('common.file.choose', 'Choose file…')"- <span class="text-caption text-grey">{{ fileName || 'No file selected' }}</span>
+ <span class="text-caption text-grey">{{ fileName || t('common.file.none', 'No file selected') }}</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| :label="isImage ? 'Choose image…' : 'Choose file…'" | |
| @click="fileInput?.click()" | |
| /> | |
| <span class="text-caption text-grey">{{ fileName || 'No file selected' }}</span> | |
| :label="isImage ? t('common.file.choose', 'Choose image…') : t('common.file.choose', 'Choose file…')" | |
| `@click`="fileInput?.click()" | |
| /> | |
| <span class="text-caption text-grey">{{ fileName || t('common.file.none', 'No file selected') }}</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue-quasar/src/renderers/FileField.vue` around lines 9 - 12, Replace
the hardcoded English strings in FileField.vue with i18n lookups: use the
component's i18n function (e.g., useI18n() or $t) in the setup and change the
:label binding to call t("common.file.choose", { type: isImage ? "image" :
"file" }) instead of "Choose image…" / "Choose file…", and replace the span
content to t("common.file.none") when fileName is falsy; update any references
to fileInput, fileName, and isImage in the template accordingly so the label and
fallback text are localized.
Comment on lines
+49
to
+53
| function handleChange(e: Event) { | ||
| const input = e.target as HTMLInputElement | ||
| const file = input.files?.[0] | ||
| props.onChange(file ?? null) | ||
| } |
There was a problem hiding this comment.
Re-selecting the same file is silently ignored — reset fileInput.value after handling.
Once a file is selected, the native input's value holds the file path. A second attempt to choose the same file won't fire @change because the value hasn't changed. This blocks users from re-submitting after a validation error.
🐛 Proposed fix
function handleChange(e: Event) {
const input = e.target as HTMLInputElement
const file = input.files?.[0]
props.onChange(file ?? null)
+ input.value = ''
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function handleChange(e: Event) { | |
| const input = e.target as HTMLInputElement | |
| const file = input.files?.[0] | |
| props.onChange(file ?? null) | |
| } | |
| function handleChange(e: Event) { | |
| const input = e.target as HTMLInputElement | |
| const file = input.files?.[0] | |
| props.onChange(file ?? null) | |
| input.value = '' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vue-quasar/src/renderers/FileField.vue` around lines 49 - 53, The
change handler handleChange currently reads the selected file and calls
props.onChange but doesn't clear the native input value, so selecting the same
file again won't emit a change event; after calling props.onChange(file ?? null)
reset the file input's value (e.g., fileInput.value = '') so the browser will
fire change on re-selecting the same file; locate handleChange and the file
input ref (fileInput) and add the value reset immediately after invoking
props.onChange.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CurrencyFieldandFileFieldrenderers across all 4 frameworks (react-web, react-native, sveltekit, vue-quasar), closing the gap where core field definitions existed but had no rendererpassword,textarea,time,select, andmultiselectfield typesListFieldrenderers with modal-based nested form editing whenitemSchemais defined — usesuseDataForm+getRendererto render a full sub-form inside a modal dialogListFieldDefinition.itemSchema()to storeSchemaProvide(via.provide()) instead of a rawSchemaDefinition, so renderers can consume it directlyDetails
New renderers (9 files)
CurrencyFieldFileFieldAll registered as
currency,file, andimagein each framework'srenderers/index.ts.Filler generators (
@ybyra/core)text(password kind)faker.internet.password()textareafaker.lorem.paragraph()timeHH:MMstringselectconfig.attrs.optionsmultiselectfile/imageundefined(skipped in output)ListField sub-schema modal
When
list().itemSchema(schema)is defined, the ListField renderer now:useDataFormfor full validation, state management, and proxy resolution{}) when noitemSchemais definedBreaking change
ListFieldDefinition.itemSchema(schema)now callsschema.provide()before storing inconfig.attrs.itemSchema. This means consumers that previously read the rawSchemaDefinitionfrom attrs will now get aSchemaProvideobject instead. No existing code in the repo relied on the old behavior.Known limitations
FileFieldis a UI placeholder — needsexpo-document-pickerintegration for actual file selectionlist/treefiller generators yet (consistent withfile/image)Test plan
@ybyra/core— 83 tests pass (includes 7 new filler tests)@ybyra/react— 79 tests pass@ybyra/corebuild succeeds (tsup + DTS)Summary by CodeRabbit
Release Notes
New Features
Tests