fix(builder): replace live field previews in Add Fields palette#135
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughField types are extended with Lucide icon components, the Add Fields sidebar is refactored to render filterable field buttons with icons (no live input previews), and E2E tests plus a helper selector were added/updated to validate and interact with the new palette UI. ChangesAdd Fields Palette Icons and Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
frontend/e2e/specs/add-fields-palette.spec.ts (1)
28-41: ⚡ Quick winConsider testing Password button as well.
The test validates that Email and Phone buttons contain no autofillable inputs. Since Password fields also trigger browser autofill (mentioned in PR objectives), consider adding:
const passwordBtn = sidebar.getByRole("button", { name: "Password", exact: true, }); await expect(passwordBtn.locator("input")).toHaveCount(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/specs/add-fields-palette.spec.ts` around lines 28 - 41, The test currently asserts no autofillable inputs for Email and Phone buttons (using sidebar.getByRole and locators emailBtn and phoneBtn) but misses the Password case; add a similar assertion for the Password palette button by locating it via sidebar.getByRole({ name: "Password", exact: true }) (e.g., passwordBtn) and assert passwordBtn.locator("input") has count 0 so Password buttons are covered just like Email and Phone.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/specs/add-fields-palette.spec.ts`:
- Around line 60-72: Add missing assertions verifying that "Date Time" and "Date
Range" remain visible after filtering; locate the block where the test fills the
search input via sidebar.getByPlaceholder("Search Fields").fill("date") and
currently only asserts sidebar.getByRole("button", { name: "Date", exact: true
}).toBeVisible(), then add two additional expectations using
sidebar.getByRole("button", { name: "Date Time", exact: true }) and
sidebar.getByRole("button", { name: "Date Range", exact: true }) and assert they
are visible so all three date-related buttons are covered.
---
Nitpick comments:
In `@frontend/e2e/specs/add-fields-palette.spec.ts`:
- Around line 28-41: The test currently asserts no autofillable inputs for Email
and Phone buttons (using sidebar.getByRole and locators emailBtn and phoneBtn)
but misses the Password case; add a similar assertion for the Password palette
button by locating it via sidebar.getByRole({ name: "Password", exact: true })
(e.g., passwordBtn) and assert passwordBtn.locator("input") has count 0 so
Password buttons are covered just like Email and Phone.
🪄 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: 922368a4-5f43-42cd-a294-36d05265565f
📒 Files selected for processing (4)
frontend/e2e/helpers/form-builder.tsfrontend/e2e/specs/add-fields-palette.spec.tsfrontend/src/components/builder/sidebar/AddFieldsSection.vuefrontend/src/config/fieldTypes.ts
| await sidebar.getByPlaceholder("Search Fields").fill("date"); | ||
|
|
||
| // "Date", "Date Time", "Date Range" remain (case-insensitive substring) | ||
| await expect( | ||
| sidebar.getByRole("button", { name: "Date", exact: true }) | ||
| ).toBeVisible(); | ||
| // Unrelated types are filtered out | ||
| await expect( | ||
| sidebar.getByRole("button", { name: "Phone", exact: true }) | ||
| ).toHaveCount(0); | ||
| await expect( | ||
| sidebar.getByRole("button", { name: "Data", exact: true }) | ||
| ).toHaveCount(0); |
There was a problem hiding this comment.
Add assertions for Date Time and Date Range.
The comment on line 62 states that "Date", "Date Time", and "Date Range" should remain after filtering for "date", but only the "Date" button is verified. Add:
await expect(
sidebar.getByRole("button", { name: "Date Time", exact: true })
).toBeVisible();
await expect(
sidebar.getByRole("button", { name: "Date Range", exact: true })
).toBeVisible();This ensures the filter correctly matches all date-related field types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/specs/add-fields-palette.spec.ts` around lines 60 - 72, Add
missing assertions verifying that "Date Time" and "Date Range" remain visible
after filtering; locate the block where the test fills the search input via
sidebar.getByPlaceholder("Search Fields").fill("date") and currently only
asserts sidebar.getByRole("button", { name: "Date", exact: true
}).toBeVisible(), then add two additional expectations using
sidebar.getByRole("button", { name: "Date Time", exact: true }) and
sidebar.getByRole("button", { name: "Date Range", exact: true }) and assert they
are visible so all three date-related buttons are covered.
…alette buttons - transition-colors → transition-all so active:scale-[0.98] animates - remove constant-text Tooltip wrapper (icon + label already convey action) - drop redundant `as Fieldtype` cast and unused Fieldtype import - type-only import for FormFields
- tablist semantics on rail (role=tablist/tab/tabpanel, aria-selected, aria-controls)
- aria-label on icon-only tab buttons (SR-accessible)
- left accent bar on active tab for stronger emphasis (bg-surface-gray-7)
- section-fade transition (120ms ease-out enter, ease-in leave) on content panel
- respect prefers-reduced-motion
- 100vh → 100dvh on sidebar height (mobile-safe)
- swap to frappe-ui semantic tokens: bg-surface-gray-{1,2,3}, bg-surface-white, border-outline-gray-{1,2}
- default active section via id lookup, sidebarSections const (never mutated)
1 participant
Summary
RenderFieldinstances, so browser autofill chips appeared on Email/Phone/Password palette tiles (bad UX, palette is not a real form).FIELD_TYPE_DEFINITIONSnow carries a lucideicon. Whole tile is now the click target.e2e/helpers/form-builder.ts:addFieldfor the new structure (palette item is a<button>directly).Test plan
yarn typecheckcleannpx playwright test add-fields-palette form-creation form-layout— 16/16 greenSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests