-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refactor dataconnect to use the common appUtils models #9286
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
Conversation
// Detect what platform based on current user | ||
let targetPlatform = await getPlatformFromFolder(appDir); | ||
if (targetPlatform === Platform.NONE) { | ||
let targetPlatforms = await getPlatformsFromFolder(appDir); |
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.
@maneesht is the right person to take a look at management/apps.ts
~
I took a glance. Don't see any issues.
b6bdceb
to
79b8215
Compare
79b8215
to
0eca209
Compare
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.
/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 does a great job of refactoring the app detection logic into a common appUtils
module, which improves code organization and reusability. The increased test coverage is also a welcome addition. I've found a critical issue with a new deduplication function and a couple of other points that I've detailed in the specific comments. Once these are addressed, this will be a solid improvement.
Description
Scenarios Tested
Sample Commands