Introduce devtools panel for debugging and testing purposes#6241
Introduce devtools panel for debugging and testing purposes#6241
Conversation
… not confused when the name matches
When no products are available in the channel or channel is inactive.
- Create new OrderCaptureDialog component using macaw-ui-next - Supports three capture options: order total, authorized amount, custom - Shows contextual warnings when authorized differs from total - Integrate with both Legacy Payments API and Transactions API flows - Include development overrides for testing (to be removed)
🦋 Changeset detectedLatest commit: 9f2d8e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR introduces a developer tools panel for debugging and testing, along with significant refactoring of the notification system and order-related components. The key changes include:
- DevTools Panel: A new keyboard-activated (Cmd+Shift+D) debug panel system with pluggable tools
- Notification System Migration: Replaces custom message system with Sonner toast library
- Order Capture Dialog: Unified, comprehensive payment capture dialog supporting both legacy and transaction APIs
- Order Summary Improvements: Enhanced order summary with editable shipping/discount fields for draft/unconfirmed orders
Reviewed changes
Copilot reviewed 78 out of 79 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/DevTools/* | New devtools panel infrastructure with registration system |
| src/components/notifications/* | New Sonner-based notification system with Toast component |
| src/components/messages/* | Deleted old message system files |
| src/hooks/useNotifier/* | Refactored to use new notification system |
| src/orders/components/OrderCaptureDialog/* | New unified capture dialog with comprehensive validation |
| src/orders/components/OrderSummary/* | Enhanced with editable shipping/discount functionality |
| src/orders/components/OrderDraftDetails/* | Refactored to use new OrderSummary component |
| src/orders/views/OrderDetails/* | Updated to use OrderCaptureDialog instead of OrderPaymentDialog |
| src/index.tsx | Integrated DevTools and new notification providers |
| package.json | Added Sonner dependency (^2.0.7) |
| .github/actions/prepare-api-variables/action.yml | Improved branch detection logic |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Differences FoundExpandLicense Package MIT sonner SummaryExpand
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 57 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [panel.id]); |
There was a problem hiding this comment.
The useEffect dependency array includes only panel.id, but the entire panel object (including label and component) could change between renders. If panel.label or panel.component changes while panel.id stays the same, the registered panel won't update. Either include the full panel object in dependencies (and ensure proper memoization in calling components), or use panel.id, panel.label, and panel.component separately in the dependency array.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [panel.id]); | |
| }, [register, panel.id, panel.label, panel.component]); |
| const [selectedOption, setSelectedOption] = useState<CaptureAmountOption>(getDefaultOption); | ||
| const [customAmount, setCustomAmount] = useState<string>(getDefaultCustomAmount); |
There was a problem hiding this comment.
The useState initializer is calling functions that depend on props, but these functions are defined inside the component body. This means the initial state will only be computed once on mount with the initial prop values. If the dialog is reused with different props (without unmounting), the initial state won't reflect the new props. While there's a useEffect that resets the state when 'open' changes, it would be cleaner and more reliable to use a callback function in useState that captures the current prop values at initialization time, or to use the functions directly without calling them.
| useEffect(() => { | ||
| if (open) { | ||
| setSelectedOption(getDefaultOption()); | ||
| setCustomAmount(getDefaultCustomAmount()); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [open]); |
There was a problem hiding this comment.
The useEffect has disabled the exhaustive-deps rule, but the functions getDefaultOption and getDefaultCustomAmount depend on several props and state values (authStatus, availableToCapture, remainingToPay). If any of these values change while the dialog is open, the state won't update accordingly. Consider either including all dependencies properly, or restructuring the code to avoid the need for disabling the rule. Alternatively, if the reset should truly only happen when 'open' changes, the functions should be memoized with their dependencies.
|
|
||
| const context = useMemo<INotificationContext>( | ||
| () => ({ | ||
| show: () => {}, |
There was a problem hiding this comment.
The 'show' method in the context is implemented as an empty function that does nothing. This means the NotificationContext.show() will not work as expected - it should actually trigger a notification. Based on the codebase, it appears notifications are now handled directly through useNotifier hook which uses toast.custom from Sonner, so this context method is likely vestigial. Consider either implementing it properly or documenting that notifications should be triggered via useNotifier hook instead of the context directly.
| show: () => {}, | |
| show: () => { | |
| // Deprecated: NotificationContext.show is vestigial. | |
| // Use the `useNotifier` hook to trigger notifications instead. | |
| console.warn( | |
| "NotificationContext.show is deprecated and does not trigger notifications. " + | |
| "Please use the `useNotifier` hook instead.", | |
| ); | |
| }, |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6241 +/- ##
==========================================
+ Coverage 40.96% 41.18% +0.21%
==========================================
Files 2432 2434 +2
Lines 40957 41246 +289
Branches 9414 9540 +126
==========================================
+ Hits 16779 16986 +207
- Misses 22939 23015 +76
- Partials 1239 1245 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
witoszekdev
left a comment
witoszekdev
left a comment
There was a problem hiding this comment.
it looks to me like there are 3 PRs in one here 😅
we have:
- redesign of notifications
- new order capture dialog
- devtools
To keep this simple let's focus on notifications and capture dialog and if we device to keep devtools instead of using Storybook we can create separate PR for that.
There are issues with the code that will create bugs + some of the logic for currency input is duplicated to what we already have in Dashboard
| "saleor-dashboard": patch | ||
| --- | ||
|
|
||
| Introduce new, unified order capture dialog |
There was a problem hiding this comment.
suggestion: let's add more description here what's new about this capture dialog
| Introduce new, unified order capture dialog | |
| Introduced new, unified order capture dialog |
| id: "UUVUyy", | ||
| }, | ||
| }); | ||
| // Re-export common messages for backward compatibility |
There was a problem hiding this comment.
nitpick: let's add a todo to remove this altogether
| // Re-export common messages for backward compatibility | |
| // Re-export common messages for backward compatibility | |
| // TODO: Remove and use `commonMessages` from @dashboard/intl |
| data: any; | ||
| intl: IntlShape; | ||
| notify: (message: IMessage) => void; | ||
| notify: (notification: INotification) => void; |
There was a problem hiding this comment.
question: Shouldn't we use INotificationCallback here? isn't it the same thing?
| notify: (notification: INotification) => void; | |
| notify: INotificationCallback; |
There was a problem hiding this comment.
question: Do we really need this? I think we can test notifications inside Storybook instead 😄
This will be shipped in our production code and it feels like adding a Conami code, rather than proper devtools 😅
| type Status = "success" | "error" | "info" | "warning"; | ||
|
|
||
| export interface INotification { | ||
| actionBtn?: { |
There was a problem hiding this comment.
nitpick: imho it's better to use verbose names whenever possible
| actionBtn?: { | |
| actionButton?: { |
| // Determine authorization status | ||
| const getAuthorizationStatus = (): AuthorizationStatus => { | ||
| // Check if fully charged first (nothing left to pay) | ||
| if (remainingToPay <= 0) { | ||
| return "charged"; | ||
| } | ||
|
|
||
| if (availableToCapture <= 0) { | ||
| return "none"; | ||
| } | ||
|
|
||
| if (availableToCapture >= remainingToPay) { | ||
| return "full"; | ||
| } | ||
|
|
||
| return "partial"; | ||
| }; |
There was a problem hiding this comment.
suggestion: instead of creating a function which gets it's own closure from our state anyway we should re-write this to a hook
| setSelectedOption(getDefaultOption()); | ||
| setCustomAmount(getDefaultCustomAmount()); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
issue: this warning is important here, these functions have stale closure when we call them in useEffect which means they will reset state to invalid values, instead of using useEffect we can rely on React to do cleanup for us, if we render this component conditionally wherever is used:
{params.action === "transaction-charge-action" && (
<OrderCaptureDialog ... />
)}and we can remove open as prop.
| } | ||
| orderBalance={order?.totalBalance ?? { amount: 0, currency: "USD" }} | ||
| isTransaction | ||
| open={true} |
There was a problem hiding this comment.
we should remove open since we're already conditionally rendering this component
| errors={orderPaymentCapture.opts.data?.orderCapture?.errors ?? []} | ||
| orderTotal={order?.total.gross ?? { amount: 0, currency: "USD" }} | ||
| authorizedAmount={order?.totalAuthorized ?? { amount: 0, currency: "USD" }} | ||
| open={params.action === "capture"} |
There was a problem hiding this comment.
issue: we should replace this with conditionally rendering component, and remove open parameter
|
ok, I see that these were indeed 3 separate PRs 🙈
@mirekm please make sure you look at the commits, if they were created from the same branch we will have all unmerged changes in the diff, and later we'll have conflicts. This can be fixed by: where previous-branch is the name of the branch from those 2 other PRs 😄 |
4 participants
A developer tools panel that allows registering debug tools from anywhere in the app.
For now it includes a notifications debug panel for testing toast messages (trigger different notification types, test auto-dismiss behavior, simulate long descriptions, etc.). Useful for development and QA.