Conversation
- Updated form_field.json and form_field.py to include 'Table' in the list of selectable field types for enhanced functionality. - Adjusted the modified timestamp in form_field.json to reflect recent changes.
- Added a new Table component for rendering tabular data input. - Updated RenderField and FieldRenderer components to support dynamic options for Select and Link fields. - Introduced useFieldOptions composable for improved option management. - Enhanced form_fields utility to include Table as a selectable field type. - Updated auto-imports and TypeScript definitions to accommodate new functionality.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new "Table" form field: backend type declarations updated, frontend type and registry extended, a Table field component implemented, and a composable for loading select/link options integrated into field rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableComponent as Table.vue
participant FieldRenderer
participant OptionsComposable as useFieldOptions()
participant BackendAPI as /api/v2/method/...
User->>TableComponent: click "Add Row" / edit cell
TableComponent->>FieldRenderer: render per-column FieldRenderer with model-value
FieldRenderer->>OptionsComposable: request options for field (fieldtype, options)
OptionsComposable->>BackendAPI: fetch link/select options (if Link) or return parsed options
BackendAPI-->>OptionsComposable: options data
OptionsComposable-->>FieldRenderer: resolved options
FieldRenderer-->>TableComponent: emit updated cell value
TableComponent-->>User: updated rows displayed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 7
🧹 Nitpick comments (3)
frontend/src/components/fields/Table.vue (1)
63-82: Index-based:keycauses incorrect diffing when rows are removed.Using
:key="index"means Vue reuses DOM nodes based on position after a removal — e.g., removing row 0 visually shifts every remaining row's content while keeping the component instance in place. A stable key prevents this:♻️ Proposed fix
-const rows = defineModel<Row[]>({ default: [] }); +type Row = { _id?: string; [key: string]: any }; + +function addRow() { + ... + const newRow = { _id: crypto.randomUUID(), ...columns... }; + ... +}Then in template:
- v-for="(_, index) in rows" - :key="index" + v-for="(row, index) in rows" + :key="row._id ?? index"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/fields/Table.vue` around lines 63 - 82, The template uses :key="index" in the v-for over rows which causes Vue to reuse DOM nodes incorrectly when a row is removed; change the key to a stable identifier (e.g., :key="row.id" or :key="rows[index].id") on the outer div and each FieldRenderer iteration, and ensure every row object is assigned a unique, persistent id when created (update the row-creation logic such as addRow/initialization to generate an id); keep removeRow and updateCell behavior unchanged but rely on the stable id for correct diffing.frontend/src/components/RenderField.vue (1)
22-24:useFieldOptionsruns for every field regardless of type, causing unnecessary API calls.For non-Select/non-Link fields,
getFieldOptionsstill resolves (returning""or the rawoptionsstring), triggering a reactive cycle on everyfieldtype/optionschange. Consider gating the load inside the composable — or here before instantiating it — so Link fields are the only ones that make network requests.-const fieldRef = computed(() => props.field); -const { options: loadedOptions } = useFieldOptions(fieldRef); +const fieldRef = computed(() => props.field); +const fieldtype = computed(() => props.field?.fieldtype); +const { options: loadedOptions } = useFieldOptions(fieldRef); +// NOTE: useFieldOptions should short-circuit for non-Select/Link types(The real fix belongs in
useFieldOptions— see theselectOptions.tscomment.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/RenderField.vue` around lines 22 - 24, The current code always calls useFieldOptions(fieldRef), causing unnecessary getFieldOptions runs for non-Link/Select fields; change it to only instantiate/use useFieldOptions when the field type requires remote loading (e.g., when fieldRef.value.fieldtype === 'Link' or 'Select'). Keep fieldRef as-is, ensure loadedOptions defaults to an empty/ref value when the composable is not used, and update resolvedOptions (the computed using props.options ?? loadedOptions.value) to work with that default so non-Link/Select fields avoid network requests.frontend/src/utils/selectOptions.ts (1)
31-44: No error handling onresource.fetch()and a newcreateResourceis allocated on every call.If the API call fails (network error, permission denied), the rejection propagates uncaught through
load()into thewatchcallback, producing an unhandled promise rejection. Additionally, a freshcreateResourceinstance is created on everyload()invocation with no cleanup.♻️ Proposed fix
if (field.fieldtype === "Link") { const resource = createResource({ url: "forms_pro.api.form.get_link_field_options", makeParams: () => ({ doctype: field.options, filters: {}, page_length: 999, }), }); - await resource.fetch(); - return resource.data as string[] | SelectOption[]; + try { + await resource.fetch(); + return resource.data as string[] | SelectOption[]; + } catch { + return ""; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/selectOptions.ts` around lines 31 - 44, Wrap the call to resource.fetch() in a try/catch inside the code path that handles field.fieldtype === "Link" so any network/permission error is caught and you return a safe fallback (e.g., field.options or an empty array) instead of letting the rejection bubble into load()/watch(); also stop allocating a new createResource() on every invocation by caching the resource (e.g., a Map keyed by field.options) or by hoisting the createResource call out of the load/watch so you reuse the same resource instance for the same link field; if the resource API exposes a dispose/abort/unsubscribe method, call it when replacing/clearing the cache to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/builder/FieldRenderer.vue`:
- Around line 164-171: The <small> for fieldData.description should be
conditional like the Checkbox branch and the Table prop :doctype must never be
undefined; update FieldRenderer.vue to render the description only when
fieldData.description is truthy (use the same v-if guard as the Checkbox branch)
and pass a safe doctype to the Table component (e.g., compute a fallback or pass
null/empty string when fieldData.options is falsy) so Table does not receive
doctype=undefined; ensure references are to fieldData.description and the Table
component props (v-model, :in-edit-mode, :doctype) so you modify the correct
elements.
- Line 168: FieldRenderer.vue is silencing a type mismatch by using
v-model="modelValue as undefined"; instead give the component a proper typed
model value: update the defineModel usage in FieldRenderer (replace the untyped
defineModel/unknown modelValue) to the correct generic type (e.g.,
defineModel<Row[]>() or declare a properly typed prop/computed wrapper) so
v-model binds a typed modelValue rather than casting to undefined; ensure the
symbol names involved are FieldRenderer.vue, defineModel, modelValue and that
Table.vue's defineModel<Row[]>() type is respected by this component (or
introduce a typed intermediate like modelValueTyped: Ref<Row[]> and use
v-model="modelValueTyped").
In `@frontend/src/components/fields/Table.vue`:
- Around line 39-45: addRow currently builds a newRow from columns.value which
can be empty if columnResource hasn't loaded, producing keyless rows; update
addRow to early-return (or disable action) when columns.value is empty (e.g., if
(!columns.value || columns.value.length === 0) return) so it never appends an
empty object to rows.value, ensuring newRow creation uses actual column keys;
reference the addRow function, columns.value, rows.value and the Row type (and
FieldRenderer rendering) when applying the guard.
- Around line 27-37: The computed columns builder spreads raw doctype column
data into the column objects, which leaves Frappe-native fieldtypes (e.g.,
"Int", "Datetime") in the props and causes FieldRenderer to fail because
formFields expects forms_pro types; update the columns mapping in the columns
computed to translate each column.fieldtype using the existing
mapDoctypeFieldForForm function (from form_fields.ts) before returning the
column object so FieldRenderer receives a mapped fieldtype that matches
formFields (ensure you import mapDoctypeFieldForForm and apply it to
column.fieldtype when constructing the mapped column).
In `@frontend/src/utils/form_fields.ts`:
- Around line 148-158: TableField currently maps to the read-only ListView with
only an emptyState, which breaks form-fill rendering paths; update the
TableField definition so it either points to the interactive table component
(Table.vue) instead of ListView or provides a self-contained editable table stub
that supplies required props (columns, rows, rowKey) and data-binding handlers
used by RenderField/FieldRenderer.vue; remove placeholder emptyState text and
ensure the TableField exposes the same API as Table.vue (props/events) so direct
RenderField usage renders the editable table rather than a perpetual empty
state.
In `@frontend/src/utils/selectOptions.ts`:
- Around line 56-62: In load() inside selectOptions.ts, avoid casting non-array
results from getFieldOptions(field.value) into string[] | SelectOption[]; change
the assignment to only set options.value when Array.isArray(result) (and not
empty string/undefined), otherwise set options.value to null—i.e. use
Array.isArray(result) to guard the assignment in the load function so downstream
code that iterates options.value always receives an actual array.
- Around line 64-72: The watch call already invokes load() immediately ({
immediate: true }), so remove the redundant onMounted(() => load()) call and
drop onMounted from the imports to avoid duplicate API requests; update the code
around watch(...) and the import list (referencing watch, load, onMounted, and
field in selectOptions.ts) so only the watch with immediate remains.
---
Nitpick comments:
In `@frontend/src/components/fields/Table.vue`:
- Around line 63-82: The template uses :key="index" in the v-for over rows which
causes Vue to reuse DOM nodes incorrectly when a row is removed; change the key
to a stable identifier (e.g., :key="row.id" or :key="rows[index].id") on the
outer div and each FieldRenderer iteration, and ensure every row object is
assigned a unique, persistent id when created (update the row-creation logic
such as addRow/initialization to generate an id); keep removeRow and updateCell
behavior unchanged but rely on the stable id for correct diffing.
In `@frontend/src/components/RenderField.vue`:
- Around line 22-24: The current code always calls useFieldOptions(fieldRef),
causing unnecessary getFieldOptions runs for non-Link/Select fields; change it
to only instantiate/use useFieldOptions when the field type requires remote
loading (e.g., when fieldRef.value.fieldtype === 'Link' or 'Select'). Keep
fieldRef as-is, ensure loadedOptions defaults to an empty/ref value when the
composable is not used, and update resolvedOptions (the computed using
props.options ?? loadedOptions.value) to work with that default so
non-Link/Select fields avoid network requests.
In `@frontend/src/utils/selectOptions.ts`:
- Around line 31-44: Wrap the call to resource.fetch() in a try/catch inside the
code path that handles field.fieldtype === "Link" so any network/permission
error is caught and you return a safe fallback (e.g., field.options or an empty
array) instead of letting the rejection bubble into load()/watch(); also stop
allocating a new createResource() on every invocation by caching the resource
(e.g., a Map keyed by field.options) or by hoisting the createResource call out
of the load/watch so you reuse the same resource instance for the same link
field; if the resource API exposes a dispose/abort/unsubscribe method, call it
when replacing/clearing the cache to avoid leaks.
| <small class="text-gray-500"> | ||
| {{ fieldData.description }} | ||
| </small> | ||
| <Table | ||
| v-model="modelValue as undefined" | ||
| :in-edit-mode="inEditMode" | ||
| :doctype="fieldData.options" | ||
| /> |
There was a problem hiding this comment.
Two small issues in the Table branch.
-
Missing description guard (line 164): The
<small>forfieldData.descriptionrenders an empty element when description is absent, unlike the Checkbox branch (line 99) which hasv-if="fieldData.description". -
Undefined doctype (line 170): If the form field was saved without setting
options(the child doctype),fieldData.optionsisundefined, andTable.vuewill fire an API call fordoctype=undefined.
♻️ Proposed fix
- <small class="text-gray-500">
- {{ fieldData.description }}
- </small>
+ <small v-if="fieldData.description" class="text-gray-500">
+ {{ fieldData.description }}
+ </small>
<Table
v-model="modelValue as undefined"
:in-edit-mode="inEditMode"
- :doctype="fieldData.options"
+ :doctype="fieldData.options || ''"
/>📝 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.
| <small class="text-gray-500"> | |
| {{ fieldData.description }} | |
| </small> | |
| <Table | |
| v-model="modelValue as undefined" | |
| :in-edit-mode="inEditMode" | |
| :doctype="fieldData.options" | |
| /> | |
| <small v-if="fieldData.description" class="text-gray-500"> | |
| {{ fieldData.description }} | |
| </small> | |
| <Table | |
| v-model="modelValue as undefined" | |
| :in-edit-mode="inEditMode" | |
| :doctype="fieldData.options || ''" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/builder/FieldRenderer.vue` around lines 164 - 171,
The <small> for fieldData.description should be conditional like the Checkbox
branch and the Table prop :doctype must never be undefined; update
FieldRenderer.vue to render the description only when fieldData.description is
truthy (use the same v-if guard as the Checkbox branch) and pass a safe doctype
to the Table component (e.g., compute a fallback or pass null/empty string when
fieldData.options is falsy) so Table does not receive doctype=undefined; ensure
references are to fieldData.description and the Table component props (v-model,
:in-edit-mode, :doctype) so you modify the correct elements.
| {{ fieldData.description }} | ||
| </small> | ||
| <Table | ||
| v-model="modelValue as undefined" |
There was a problem hiding this comment.
v-model="modelValue as undefined" silences a legitimate type mismatch.
defineModel() in FieldRenderer is untyped (unknown), while Table.vue declares defineModel<Row[]>(). Cast-to-undefined is a no-op at runtime but hides the real fix: either type modelValue as Row[] in this component or use a properly typed intermediate.
♻️ Proposed fix
-const modelValue = defineModel();
+const modelValue = defineModel<Row[]>();Then in the template:
-<Table v-model="modelValue as undefined" .../>
+<Table v-model="modelValue" .../>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/builder/FieldRenderer.vue` at line 168,
FieldRenderer.vue is silencing a type mismatch by using v-model="modelValue as
undefined"; instead give the component a proper typed model value: update the
defineModel usage in FieldRenderer (replace the untyped defineModel/unknown
modelValue) to the correct generic type (e.g., defineModel<Row[]>() or declare a
properly typed prop/computed wrapper) so v-model binds a typed modelValue rather
than casting to undefined; ensure the symbol names involved are
FieldRenderer.vue, defineModel, modelValue and that Table.vue's
defineModel<Row[]>() type is respected by this component (or introduce a typed
intermediate like modelValueTyped: Ref<Row[]> and use
v-model="modelValueTyped").
| function addRow() { | ||
| const newRow = columns.value.reduce((acc, column) => { | ||
| acc[column.key] = null; | ||
| return acc; | ||
| }, {} as Row); | ||
| rows.value = [...(rows.value ?? []), newRow]; | ||
| } |
There was a problem hiding this comment.
addRow when columns haven't loaded yet produces empty, keyless rows.
If a user clicks "Add Row" before the columnResource finishes, columns.value is [], so newRow is {} — a row with no keys. Those rows then render zero FieldRenderer instances but still appear in the list. Guard the action:
♻️ Proposed fix
function addRow() {
+ if (!columns.value.length) return;
const newRow = columns.value.reduce((acc, column) => {
acc[column.key] = null;
return acc;
}, {} as Row);
rows.value = [...(rows.value ?? []), newRow];
}📝 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.
| function addRow() { | |
| const newRow = columns.value.reduce((acc, column) => { | |
| acc[column.key] = null; | |
| return acc; | |
| }, {} as Row); | |
| rows.value = [...(rows.value ?? []), newRow]; | |
| } | |
| function addRow() { | |
| if (!columns.value.length) return; | |
| const newRow = columns.value.reduce((acc, column) => { | |
| acc[column.key] = null; | |
| return acc; | |
| }, {} as Row); | |
| rows.value = [...(rows.value ?? []), newRow]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/fields/Table.vue` around lines 39 - 45, addRow
currently builds a newRow from columns.value which can be empty if
columnResource hasn't loaded, producing keyless rows; update addRow to
early-return (or disable action) when columns.value is empty (e.g., if
(!columns.value || columns.value.length === 0) return) so it never appends an
empty object to rows.value, ensuring newRow creation uses actual column keys;
reference the addRow function, columns.value, rows.value and the Row type (and
FieldRenderer rendering) when applying the guard.
| export const TableField: FormFieldType = { | ||
| component: ListView, | ||
| props: { | ||
| options: { | ||
| emptyState: { | ||
| title: "This is a table field", | ||
| description: "Use this field to input a list of items.", | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
TableField mapping to ListView is a non-functional placeholder that breaks form-fill rendering paths.
ListView is a read-only list display component — it requires at minimum columns, rows, and rowKey to render anything useful. With only an emptyState object supplied, it will perpetually render the empty state with no data binding.
FieldRenderer.vue avoids this by short-circuiting to Table.vue in its dedicated v-else-if branch, but any call path that uses RenderField directly (e.g. standalone form-fill / submission views) with a Table-typed field will hit this mapping and produce a broken UI. The emptyState strings also look like placeholder text that shouldn't reach production.
Replace this with a proper self-contained table-input stub or point it at Table.vue to keep both rendering paths consistent:
♻️ Proposed fix
+import TableComponent from "@/components/fields/Table.vue";
-export const TableField: FormFieldType = {
- component: ListView,
- props: {
- options: {
- emptyState: {
- title: "This is a table field",
- description: "Use this field to input a list of items.",
- },
- },
- },
-};
+export const TableField: FormFieldType = {
+ component: TableComponent,
+ props: {},
+};📝 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.
| export const TableField: FormFieldType = { | |
| component: ListView, | |
| props: { | |
| options: { | |
| emptyState: { | |
| title: "This is a table field", | |
| description: "Use this field to input a list of items.", | |
| }, | |
| }, | |
| }, | |
| }; | |
| import TableComponent from "@/components/fields/Table.vue"; | |
| export const TableField: FormFieldType = { | |
| component: TableComponent, | |
| props: {}, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/form_fields.ts` around lines 148 - 158, TableField
currently maps to the read-only ListView with only an emptyState, which breaks
form-fill rendering paths; update the TableField definition so it either points
to the interactive table component (Table.vue) instead of ListView or provides a
self-contained editable table stub that supplies required props (columns, rows,
rowKey) and data-binding handlers used by RenderField/FieldRenderer.vue; remove
placeholder emptyState text and ensure the TableField exposes the same API as
Table.vue (props/events) so direct RenderField usage renders the editable table
rather than a perpetual empty state.
| const load = async () => { | ||
| const result = await getFieldOptions(field.value); | ||
| options.value = | ||
| result === "" || result === undefined | ||
| ? null | ||
| : (result as string[] | SelectOption[]); | ||
| }; |
There was a problem hiding this comment.
Non-array result (raw string) is incorrectly cast to string[] | SelectOption[].
When fieldtype is neither "Select" nor "Link" but options is non-empty (e.g. a "Data" field with options: "Email"), getFieldOptions returns the raw string "Email". The guard result === "" is false, so useFieldOptions assigns "Email" as string[] | SelectOption[]. Any downstream component that iterates options.value would receive individual characters.
Restrict the assignment to actual array results:
♻️ Proposed fix
const load = async () => {
const result = await getFieldOptions(field.value);
options.value =
- result === "" || result === undefined
+ !result || !Array.isArray(result)
? null
: (result as string[] | SelectOption[]);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/selectOptions.ts` around lines 56 - 62, In load() inside
selectOptions.ts, avoid casting non-array results from
getFieldOptions(field.value) into string[] | SelectOption[]; change the
assignment to only set options.value when Array.isArray(result) (and not empty
string/undefined), otherwise set options.value to null—i.e. use
Array.isArray(result) to guard the assignment in the load function so downstream
code that iterates options.value always receives an actual array.
- Integrated utility function to map fieldtype for Table component, ensuring proper handling of field types. - Defaulted unmapped fieldtypes to "Data" for improved consistency in table rendering.
- Replaced ListView with Table in the TableField definition to ensure proper rendering of tabular data. - Adjusted imports in form_fields utility to reflect the change in component usage.
- Eliminated the onMounted lifecycle hook from the useFieldOptions composable to streamline the loading process of options. - Adjusted imports to reflect the removal of the unused onMounted function.
1 participant
Still need to use doctypes created via desk.
Child table creation on the fly should come in next update of this
Summary by CodeRabbit