-
Notifications
You must be signed in to change notification settings - Fork 26
Added: Add App Bridge Login Flow #430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Added: Add App Bridge Login Flow #430
Conversation
|
@gemini-code-assist please review this PR. |
|
Hello! You've tagged me. How can I assist you with this pull request? If you'd like a code review, please use |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a login flow for embedded Shopify apps using App Bridge. The changes are mostly in DxpLogin.vue and new utility functions in src/utils/index.ts.
My review has identified a few critical and high-severity issues:
- A critical logic bug in the new
openPosScannerutility function will cause it to not work as expected. - The project now depends on
@shopify/app-bridge-utils, which is a deprecated package and should be replaced. - There's a potential for an unhandled exception in
DxpLogin.vuedue to unsafe parsing of an environment variable.
Additionally, I've pointed out several medium-severity issues, including leftover console.log statements, throwing strings instead of Error objects, and a confusing TODO comment. Please address these points to improve the code's robustness and maintainability.
Related PRsFulfillment - hotwax/fulfillment#1352 |
|
Shopify claims to use Socket mobile Scanners in Shopify POS and connect with them seemlessly and manage them in settings, it has been observed that if we connect a scanner with Shopify POS then that scanner only fills out the Shopify POS inputs and not the input fields in Shopify POS embedded apps. |
Related Issue: #429
Added App Bridge Login Flow for Shopify Embedded App User Login. And removed usage of getAppLoginUrl from DxpLogin since it's not needed on DxpLogin now.