refactor(ui): widget menu cleanup — Export submenu + Save in modal (#912, #913) - #959
Merged
Merged
Conversation
, #913) Closes #912 and #913 together — same dropdown surface. #912: Export CSV/PNG/SVG were three separate top-level items in every chart widget's action dropdown, crowding the menu. They now collapse into a single "Export ▸" submenu (Radix DropdownMenuSub). Single-format widgets (e.g. data-only, no ECharts) keep a flat "Export CSV" entry to avoid an extra click for the common case. #913: "Save to Widget Library" moved from the per-widget dropdown into the widget editor modal footer. Lives left-aligned next to Cancel/Save as "Save as new template", visible only in edit mode. Discoverable at the moment users finish configuring, which is when they're most likely to want to reuse the result. Implementation: - component/src/components/composed/widget-card.tsx — WidgetCardAction gains an optional `children?: WidgetCardAction[]` field; render block switches to DropdownMenuSub when children are present. - app/src/components/dashboard-container.tsx — collapses the three Export pushes into one submenu (flat fallback for 1 format), drops the Save action and the onSaveAsTemplate prop. - app/src/components/widget-editor-modal.tsx + modal-footer.tsx — new onSaveAsTemplate prop wired through; footer renders "Save as new template" only when mode === "edit" and the callback is provided. - app/src/app/(dashboard)/[id]/edit/page.tsx — passes the callback to WidgetEditorModal (was already wiring SaveTemplateDialog state). - app/src/app/(dashboard)/[id]/page.tsx — view mode no longer renders SaveTemplateDialog at all (users access via Edit → modal). Tests: - widget-card vitest: new "submenu actions" describe — submenu trigger renders, child click fires correctly via keyboard nav. - dashboard-container test stub flattens submenu children so existing testid assertions ("action-csv") keep working; replaced the obsolete "Save to Widget Library in dropdown" test with a negative assertion. - chart-export.spec.ts updated to traverse the new "Export ▸" submenu. - widget-library.spec.ts: the main happy-path test now opens the editor modal and clicks "Save as new template"; the three "from view mode" tests deleted (feature no longer available from view mode by design). E2E: 17/17 passed locally (chart-export + widget-library). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds nested submenu support to WidgetCard actions, consolidates multiple export formats under an "Export ▸" submenu, moves "Save as template" into the widget editor modal footer (with callback wiring), and updates tests and dashboard pages to match. ChangesWidget Action Menu Refactor & Save-as-Template Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
app/src/components/__tests__/dashboard-container-branches.test.tsx (1)
41-47: ⚡ Quick winAssert the grouped Export action shape at least once in unit tests
Line 45 flattens submenu actions, so these tests can still pass even if
buildActionsstops emitting theExportparent withchildren. Add one assertion against capturedwidgetCardPropsto verify the grouped structure for multi-format widgets.As per coding guidelines,
**/*.ts: “Every new behavior, bug fix, and edge case gets a test.”Also applies to: 309-324
🤖 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 `@app/src/components/__tests__/dashboard-container-branches.test.tsx` around lines 41 - 47, Add a unit assertion that verifies buildActions emits a grouped "Export" parent with a non-empty children array for multi-format widgets: after computing flatActions in dashboard-container-branches.test.tsx (the test that captures widgetCardProps), assert that widgetCardProps.actions (or the captured actions passed into the WidgetCard mock) contains an entry with label "Export" whose children is an array including the expected leaf entries (e.g., "CSV", "JSON" or whatever formats your fixtures use); this ensures the grouped parent+children shape created by buildActions is tested even though flatActions flattens them for other lookups.Source: Coding guidelines
🤖 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 `@app/src/components/widget-editor-modal.tsx`:
- Around line 910-917: The save-as-template handler currently forwards the stale
prop `widget` to `onSaveAsTemplate`, so unsaved edits are lost; change the
handler to call the editor state builder (e.g., `buildWidgetForSave()`) and pass
its result to `onSaveAsTemplate` instead of `widget`, while still calling
`onOpenChange(false)` to close the modal (ensure `buildWidgetForSave()` is
invoked at the time of the click and its return value is passed to
`onSaveAsTemplate`).
In `@component/src/components/composed/__tests__/widget-card.test.tsx`:
- Around line 296-307: The test "renders a flat menu item when action has no
children (backwards compat)" is a false positive because the menu is never
opened and portal content isn’t inside container; update the test to open the
menu before asserting. Locate the menu trigger in the rendered WidgetCard (e.g.,
find the button with role="button" or aria-haspopup using
container.querySelector or screen.getByRole), simulate a click (use
userEvent.click or fireEvent.click) to open the dropdown, then query document
(not container) or use within(document.body) for '[data-state][role="menuitem"]'
and assert it is present for the flat action; reference the test name,
WidgetCard render call, the actions array, and the menu trigger/button when
making changes.
---
Nitpick comments:
In `@app/src/components/__tests__/dashboard-container-branches.test.tsx`:
- Around line 41-47: Add a unit assertion that verifies buildActions emits a
grouped "Export" parent with a non-empty children array for multi-format
widgets: after computing flatActions in dashboard-container-branches.test.tsx
(the test that captures widgetCardProps), assert that widgetCardProps.actions
(or the captured actions passed into the WidgetCard mock) contains an entry with
label "Export" whose children is an array including the expected leaf entries
(e.g., "CSV", "JSON" or whatever formats your fixtures use); this ensures the
grouped parent+children shape created by buildActions is tested even though
flatActions flattens them for other lookups.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb707f19-cc46-422a-afe0-42c0f2d59a0e
⛔ Files ignored due to path filters (1)
app/tsconfig.tsbuildinfois excluded by!app/tsconfig.tsbuildinfo
📒 Files selected for processing (10)
app/e2e/chart-export.spec.tsapp/e2e/widget-library.spec.tsapp/src/app/(dashboard)/[id]/edit/page.tsxapp/src/app/(dashboard)/[id]/page.tsxapp/src/components/__tests__/dashboard-container-branches.test.tsxapp/src/components/dashboard-container.tsxapp/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/modal-footer.tsxcomponent/src/components/composed/__tests__/widget-card.test.tsxcomponent/src/components/composed/widget-card.tsx
- widget-card.tsx: drop array indices from React keys (label-based now). - widget-editor-modal.tsx: extract the SaveAsTemplate inline conditional into a top-level handler so the component's cognitive complexity stays under Sonar's threshold of 15. - modal-footer.test.tsx: 6 new tests covering the "Save as new template" button (#913). Lifts the new-code coverage from 79.4% back above 80%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- widget-editor-modal.tsx: SaveAsTemplate now snapshots current editor state via buildWidgetForSave() instead of the (potentially stale) widget prop, so unsaved edits land in the template payload. Wrapped the handler in useCallback to also reduce the function's overall cognitive complexity per Sonar. - widget-card.test.tsx: backwards-compat submenu test was a false positive — it queried the unopened dropdown via container, not the portal. Rewrote to actually open the menu, assert aria-haspopup is absent (flat menuitem, not a sub-trigger), and confirm click fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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
2 participants
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.
Closes #912. Closes #913.
Summary
Two related widget action-menu changes from the dogfood session, shipped together as a single PR since both touch the same dropdown surface.
#912 — Export submenu
Export CSV/PNG/SVG were three flat top-level items, crowding the menu next to primary actions like Edit / Remove. They now collapse into a single `Export ▸` submenu via Radix `DropdownMenuSub`. Single-format widgets (e.g. a data-only widget with no ECharts rendering) keep a flat "Export CSV" entry to avoid an extra click for the common case.
#913 — Save as new template moves to modal
"Save to Widget Library" moved from the per-widget dropdown into the widget editor modal footer, left-aligned next to Cancel/Save:
```
[ Save as new template ] ... [ Cancel ] [ Save ]
```
Visible only when `mode === "edit"`. The discoverability point: users want to reuse a widget at the moment they've finished configuring it, not via a dropdown next to Edit/Remove.
Implementation
Trade-off explicitly accepted
View-mode users (creators viewing a dashboard they didn't author) lose the one-click "Save as template" from the menu. They have to enter Edit mode → open the widget editor → click the footer button. The issue makes this trade-off deliberately: the new placement is much more discoverable at the right moment.
Tests
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests