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)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 177 out of 178 changed files in this pull request and generated 3 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.
|
|
||
| // Assert | ||
| expect(result.current).toBeTruthy(); | ||
| expect(typeof result.current?.show).toBe("function"); |
There was a problem hiding this comment.
The NotificationProvider test expects a 'show' method on the context, but the actual NotificationContext interface only provides 'remove' and 'clearErrorNotifications' methods. The notification functionality is now accessed through the useNotifier hook directly, not through the context. This test assertion is incorrect and should be removed.
| // Also check for Apollo errors (network errors, GraphQL execution errors) | ||
| // These are stored in opts.error, not in the mutation response data | ||
| const hasApolloError = !!opts.error; | ||
|
|
||
| return getMutationState( | ||
| opts.called, | ||
| opts.loading, | ||
| errors, | ||
| hasApolloError ? [{ error: opts.error }] : [], | ||
| ); |
There was a problem hiding this comment.
The getMutationStatus function is checking for Apollo errors and adding them to an array, but the array structure ({ error: opts.error }) doesn't match the expected error type for getMutationState. This could cause type errors or unexpected behavior in error handling. Consider verifying that getMutationState can handle Apollo errors properly or restructure how these errors are passed.
| // Re-export common messages for backward compatibility | ||
| export const calloutTitleMessages = { |
There was a problem hiding this comment.
The calloutTitleMessages object is now referencing commonMessages for backward compatibility, but this creates a dependency structure where Callout messages import from intl. If commonMessages.info or commonMessages.warning are ever removed or renamed, this will break silently. Consider adding a comment or explicit type check to ensure these dependencies remain stable, or update all consumers to use commonMessages directly.
| // Re-export common messages for backward compatibility | |
| export const calloutTitleMessages = { | |
| // Re-export common messages for backward compatibility. | |
| // Typed against commonMessages to ensure these keys remain stable. | |
| export const calloutTitleMessages: Pick<typeof commonMessages, "info" | "warning"> = { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 175 out of 176 changed files in this pull request and generated 2 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.
| }} | ||
| > | ||
| <Text size={2} color="default2"> | ||
| {isExpanded ? "Show less" : "Show more"} |
There was a problem hiding this comment.
The hardcoded text "Show less" and "Show more" should be internationalized using the intl system, similar to how other user-facing text in the dashboard is handled. These strings should be defined in a messages file and formatted using intl.formatMessage().
| } | ||
|
|
||
| export interface INotificationContext { | ||
| remove: (notificationId: number) => void; |
There was a problem hiding this comment.
The remove method in INotificationContext only accepts number, but Sonner's toast.dismiss() can receive string | number IDs. The Toast component correctly accepts string | number for its id prop. To maintain type safety and consistency with Sonner's API, update the remove method signature to accept (notificationId: string | number) => void.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 174 out of 175 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3 participants
Scope
apiMessagehandling via See more (we have to improve the error massages, as logs are not helping any of the Dashboard users)