chore: better ui to remove options in multiselect#87
Conversation
- Introduced `inEditMode` prop to `RenderField.vue` to control edit state. - Updated `FieldRenderer.vue` to pass `inEditMode` to `RenderField`. - Enhanced `Multiselect.vue` to utilize `inEditMode` for conditional rendering and option removal functionality.
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FieldRenderer
participant RenderField
participant Multiselect
User->>FieldRenderer: Component mounted with inEditMode=true
FieldRenderer->>RenderField: Pass inEditMode prop
RenderField->>Multiselect: Render with :in-edit-mode="true"
Multiselect->>Multiselect: Render remove button for each option
User->>Multiselect: Click remove button on option
Multiselect->>Multiselect: removeOption(option)
Note over Multiselect: Parse field.options (newline-separated)<br/>Remove matching option<br/>Update field.options and selected
Multiselect->>User: Option removed from field definition
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/components/builder/FieldLabel.vue (1)
25-25: Prefer flex sizing for the editable label width.
w-fullcan crowd the required asterisk in this flex row.flex-1 min-w-0keeps the input full-width within the remaining space and shrinks cleanly on narrow builder panels.Suggested tweak
- class="bg-transparent border-none outline-none text-base focus:ring-0 w-full px-0 py-1" + class="bg-transparent border-none outline-none text-base focus:ring-0 flex-1 min-w-0 px-0 py-1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/builder/FieldLabel.vue` at line 25, The editable label's class uses w-full which can push the required asterisk out in the flex row; update the element in FieldLabel.vue that has class="bg-transparent border-none outline-none text-base focus:ring-0 w-full px-0 py-1" to use flex sizing instead (replace w-full with "flex-1 min-w-0") so the input consumes remaining space and can shrink properly next to the asterisk in the flex container.frontend/src/components/builder/FieldRenderer.vue (1)
53-58: ForwardinEditModethrough everyRenderFieldbranch.Line 80 wires edit mode only for
description-first. SinceRenderFieldnow supports this prop generally, other layouts will silently render child field components withinEditMode = falseif they start consuming edit-mode behavior.Suggested consistency pass
<RenderField v-model="modelValue" :field="fieldData" + :in-edit-mode="inEditMode" :class="{ 'pointer-events-none mt-1': inEditMode }" :disabled="disabled" /> @@ <RenderField v-model="modelValue" :field="fieldData" + :in-edit-mode="inEditMode" :class="{ 'pointer-events-none': inEditMode }" :disabled="disabled" /> @@ <RenderField v-model="modelValue" :field="fieldData" + :in-edit-mode="inEditMode" :class="{ 'pointer-events-none': inEditMode }" :disabled="disabled" :options="selectOptions"Also applies to: 77-83, 93-98, 119-125
🤖 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 53 - 58, The RenderField usages are not consistently passing the inEditMode prop so only the description-first branch receives edit-mode state; update every RenderField instance (the ones using v-model="modelValue", :field="fieldData", :class=..., and :disabled="disabled") to also forward :inEditMode="inEditMode" so all layout branches receive the prop; ensure the same change is applied to the other RenderField occurrences noted (the blocks around the existing RenderField calls) so child field components observe edit-mode consistently.
🤖 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/fields/multiselect/Multiselect.vue`:
- Around line 32-37: The removeOption function currently mutates
props.field.options directly and can be invoked when props.field is undefined;
change it to compute the new options string and emit an update event instead of
mutating props (use the component's emit to fire update:field with a shallow
copy like { ...props.field, options: newOptions }) and update local selected
state as needed; also guard the remove button so it only renders/calls
removeOption when props.field is truthy (or return early) to avoid no-op
clicks—refer to removeOption, props.field, selected and the update:field
convention used in MultiselectBuilderExtras.vue / FieldRenderer.vue.
---
Nitpick comments:
In `@frontend/src/components/builder/FieldLabel.vue`:
- Line 25: The editable label's class uses w-full which can push the required
asterisk out in the flex row; update the element in FieldLabel.vue that has
class="bg-transparent border-none outline-none text-base focus:ring-0 w-full
px-0 py-1" to use flex sizing instead (replace w-full with "flex-1 min-w-0") so
the input consumes remaining space and can shrink properly next to the asterisk
in the flex container.
In `@frontend/src/components/builder/FieldRenderer.vue`:
- Around line 53-58: The RenderField usages are not consistently passing the
inEditMode prop so only the description-first branch receives edit-mode state;
update every RenderField instance (the ones using v-model="modelValue",
:field="fieldData", :class=..., and :disabled="disabled") to also forward
:inEditMode="inEditMode" so all layout branches receive the prop; ensure the
same change is applied to the other RenderField occurrences noted (the blocks
around the existing RenderField calls) so child field components observe
edit-mode consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58f71d47-a33f-4ae9-a31b-0b354ba1bd40
📒 Files selected for processing (4)
frontend/src/components/RenderField.vuefrontend/src/components/builder/FieldLabel.vuefrontend/src/components/builder/FieldRenderer.vuefrontend/src/components/fields/multiselect/Multiselect.vue
| function removeOption(option: string) { | ||
| if (!props.field) return; | ||
| const items = (props.field.options ?? "").split("\n").filter(Boolean); | ||
| props.field.options = items.filter((v: string) => v !== option).join("\n"); | ||
| selected.value = selected.value.filter((v) => v !== option); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect field update propagation for Multiselect option removal.
# Expected: Either Multiselect option removal is explicitly forwarded via update:field,
# or parent builder state intentionally persists nested field mutations.
rg -n -C3 \
'removeOption|props\.field\.options|@update:field|defineEmits|<RenderField' \
--iglob '*.vue'Repository: BuildWithHussain/forms_pro
Length of output: 10207
Emit field updates instead of mutating nested props.
Line 35 mutates props.field.options directly, bypassing Vue's event propagation pattern. Additionally, the remove button renders when inEditMode is true even if field is undefined, making the click handler a no-op. Either emit an update:field event up through the component hierarchy (as seen in MultiselectBuilderExtras.vue and expected by FieldRenderer.vue), or guard the button with field.
Suggested refactor
+const emit = defineEmits<{
+ "update:field": [value: { options?: string } & Record<string, unknown>];
+}>();
+
function removeOption(option: string) {
if (!props.field) return;
const items = (props.field.options ?? "").split("\n").filter(Boolean);
- props.field.options = items.filter((v: string) => v !== option).join("\n");
+ emit("update:field", {
+ ...props.field,
+ options: items.filter((v: string) => v !== option).join("\n"),
+ });
selected.value = selected.value.filter((v) => v !== option);
}
@@
<Button
- v-if="inEditMode"
+ v-if="inEditMode && field"Ensure parent components (RenderField/FieldRenderer) forward the event up the hierarchy with @update:field="...".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/fields/multiselect/Multiselect.vue` around lines 32 -
37, The removeOption function currently mutates props.field.options directly and
can be invoked when props.field is undefined; change it to compute the new
options string and emit an update event instead of mutating props (use the
component's emit to fire update:field with a shallow copy like { ...props.field,
options: newOptions }) and update local selected state as needed; also guard the
remove button so it only renders/calls removeOption when props.field is truthy
(or return early) to avoid no-op clicks—refer to removeOption, props.field,
selected and the update:field convention used in MultiselectBuilderExtras.vue /
FieldRenderer.vue.
…114) * fix(multiselect): reset option input and error message on startAddingOption (cherry picked from commit 96a0f94) * chore: better ui to remove options in multiselect (#87) * chore: make labels w-full in builder * chore(multiselect): better ui for removing options - Introduced `inEditMode` prop to `RenderField.vue` to control edit state. - Updated `FieldRenderer.vue` to pass `inEditMode` to `RenderField`. - Enhanced `Multiselect.vue` to utilize `inEditMode` for conditional rendering and option removal functionality. (cherry picked from commit 55c4fc0) * chore(deps-dev): bump postcss from 8.5.8 to 8.5.10 in /frontend (#95) Bumps [postcss](https://github.com/postcss/postcss) from 8.5.8 to 8.5.10. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.5.8...8.5.10) --- updated-dependencies: - dependency-name: postcss dependency-version: 8.5.10 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 7a3aa8c) * chore(deps): bump dayjs from 1.11.19 to 1.11.20 in /frontend (#84) Bumps [dayjs](https://github.com/iamkun/dayjs) from 1.11.19 to 1.11.20. - [Release notes](https://github.com/iamkun/dayjs/releases) - [Changelog](https://github.com/iamkun/dayjs/blob/dev/CHANGELOG.md) - [Commits](iamkun/dayjs@v1.11.19...v1.11.20) --- updated-dependencies: - dependency-name: dayjs dependency-version: 1.11.20 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 222c73d) * chore(deps): bump @lottiefiles/dotlottie-vue in /frontend (#93) Bumps [@lottiefiles/dotlottie-vue](https://github.com/LottieFiles/dotlottie-web/tree/HEAD/packages/vue) from 0.10.4 to 0.11.11. - [Release notes](https://github.com/LottieFiles/dotlottie-web/releases) - [Changelog](https://github.com/LottieFiles/dotlottie-web/blob/main/packages/vue/CHANGELOG.md) - [Commits](https://github.com/LottieFiles/dotlottie-web/commits/@lottiefiles/dotlottie-vue@0.11.11/packages/vue) --- updated-dependencies: - dependency-name: "@lottiefiles/dotlottie-vue" dependency-version: 0.11.11 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 092d2e8) * chore(deps): bump actions/setup-node from 4 to 6 (#88) Bumps [actions/setup-node](https://github.com/actions/setup-node) from 4 to 6. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@v4...v6) --- updated-dependencies: - dependency-name: actions/setup-node dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 9aa693f) * chore(deps): bump actions/checkout from 4 to 6 (#90) Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit d147e0d) * chore(deps): bump actions/setup-python from 5 to 6 (#91) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5 to 6. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v5...v6) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 00a219c) * chore(deps): bump actions/upload-artifact from 4 to 7 (#89) Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4 to 7. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v4...v7) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit e95b934) * ci: add merge_group trigger to enable GitHub merge queue (#97) (cherry picked from commit cd63fa7) * fix(e2e): auto-fill form title in FormBuilderPage.goto() to prevent MandatoryError (#96) Forms created via e2e fixtures have title "Untitled Form" which the frontend transforms to "" on load. When tests modify the form and save, the blank title causes a MandatoryError. This was causing CI failures in multiselect-field.spec.ts. Changes: - FormBuilderPage.goto() now auto-fills a unique title and saves - Added skipTitleFill option for forms that already have a title set - createPublishedForm fixture now sets title alongside is_published - Removed manual title fill from multiselect-field.spec.ts (cherry picked from commit 26e2a74) * chore: update gitignore to ignore semgrep folder (cherry picked from commit a6f6b2a) * fix: resolve semgrep findings across codebase (#98) * fix: resolve semgrep findings across codebase - Add nosemgrep comments for reviewed guest-whitelisted endpoints - Fix format string injection by converting exceptions to str() - Remove redundant db.commit() in test teardown (DDL auto-commits) - Add explanation comment for required CSRF token commit * ci: remove paths from pull_request trigger in typecheck workflow - Simplified the typecheck workflow by removing specific paths from the pull_request trigger, allowing for broader checks on all changes. * fix: use correct semgrep rule ID prefix (frappe-semgrep-rules) (cherry picked from commit d5cde23) * feat: add Heading 1/2/3 field types (#103) * feat(form-field): add Heading 1/2/3 fieldtypes to doctype and backend mapping Maps heading fieldtypes to Frappe HTML, generates h1/h2/h3 options content for the CustomField, and skips heading fields in server-side required validation. * feat(heading): wire up Heading 1/2/3 field types across the frontend Adds heading layout type, Heading component with h2/h3/h4 tag rendering, FieldRenderer branch for edit/view modes, isHeading util, and submission display handling. * test(heading): add backend and E2E tests for heading field types - Unit tests for heading fields skipped in validation - Integration tests for get_options() and to_frappe_field - E2E tests for builder, public form rendering, and submission - Fix missing Heading imports in FieldRenderer and SubmissionFieldValue * fix(form-field): escape HTML in heading labels for get_options method - Updated get_options method to use escape_html for heading labels to prevent potential HTML injection. - Adjusted return type annotation to allow for None in addition to str. * chore: minor styling (cherry picked from commit 16989b1) * refactor: redesign the form builder layout (#105) * refactor: redesign the form builder layout - Introduced a new FieldActions component to handle field removal and drag functionality. - Integrated FieldActions into FormBuilderContent for improved user interaction with form fields. - Updated styles for better visibility and interaction feedback. * feat: enhance FieldActions component for improved drag-and-drop functionality - Updated FieldActions to include drag state handling, allowing for better user feedback during field manipulation. - Integrated the new FieldActions component into FormBuilderContent, enhancing the interaction experience with form fields. - Adjusted styles for visibility based on selection and drag state. (cherry picked from commit 4e9b9d1) * enhance(FieldActions): add tooltips for field actions buttons - Added tooltip text for the remove and drag buttons in the FieldActions component to improve user experience and accessibility. - Updated button structure for better readability and maintainability. (cherry picked from commit 66e04de) * fix(Form): correct initial route generation string (#106) - Updated the initial route generation method to remove the unnecessary 's/' prefix, ensuring the route is generated correctly as 'forms_pro_' followed by a random string. (cherry picked from commit 72f63f8) * fix: prevent duplicate fieldnames on form save (#107) * feat(frontend): add global dialog component for imperative dialogs Adds dialog utility with confirm/alert/show methods callable from anywhere via TypeScript, similar to vue-sonner pattern. * feat(dialog): add html support for rich message content * fix(editForm): prevent save when duplicate fieldnames detected Shows dialog listing conflicting Label(fieldname) pairs before save. (cherry picked from commit 9e7849d) * chore(deps): bump vue from 3.5.32 to 3.5.33 in /frontend (#110) Bumps [vue](https://github.com/vuejs/core) from 3.5.32 to 3.5.33. - [Release notes](https://github.com/vuejs/core/releases) - [Changelog](https://github.com/vuejs/core/blob/main/CHANGELOG.md) - [Commits](vuejs/core@v3.5.32...v3.5.33) --- updated-dependencies: - dependency-name: vue dependency-version: 3.5.33 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 0ffdad3) * chore(deps-dev): bump @vitejs/plugin-vue in /frontend (#109) Bumps [@vitejs/plugin-vue](https://github.com/vitejs/vite-plugin-vue/tree/HEAD/packages/plugin-vue) from 6.0.5 to 6.0.6. - [Release notes](https://github.com/vitejs/vite-plugin-vue/releases) - [Changelog](https://github.com/vitejs/vite-plugin-vue/blob/main/packages/plugin-vue/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite-plugin-vue/commits/plugin-vue@6.0.6/packages/plugin-vue) --- updated-dependencies: - dependency-name: "@vitejs/plugin-vue" dependency-version: 6.0.6 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 9035fdb) * fix(backport): adapt cherry-picked code to v15 lucide + dual-enum types - FieldActions.vue: use lucide-vue-next (v15 hasn't migrated to @lucide/vue) - SubmissionFieldValue.vue: cast FormFieldTypes prop to Fieldtype where the Heading helper / component expect the doctype-generated enum (refactor #75 consolidated these enums on develop; v15 still has both) * fix(test): use v15-compatible FrappeTestCase import in test_form_field frappe.tests.IntegrationTestCase doesn't exist on Frappe v15. Match the import pattern used by the other v15 tests (test_roles, test_invitations). --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Style