-
Notifications
You must be signed in to change notification settings - Fork 6
Template properties extraction #110
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
Template properties extraction #110
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #110 +/- ##
==========================================
- Coverage 91.85% 87.91% -3.95%
==========================================
Files 162 170 +8
Lines 6604 7056 +452
Branches 957 972 +15
==========================================
+ Hits 6066 6203 +137
- Misses 538 853 +315
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ): string { | ||
| const platformLower = input.platform.toLowerCase(); | ||
|
|
||
| // Build template properties flags if they exist |
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.
I think @haifeng-li-at-salesforce has a PR that puts this behind a node tool (which is great!) but we'll need to reconcile with these changes
| .string() | ||
| .optional() | ||
| .describe('Optional Salesforce login host URL (e.g., https://test.salesforce.com for sandbox)'), | ||
| templateProperties: z |
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.
in a separate PR I want to address the similarities (and differences) between templateProperties and other app properties (i.e. connectedAppCallback, loginHost, etc) - I think ALL properties should be gathered in a similar fashion. Right now the system is very biased towards this being a mobilesdk app but in reality alot of B2C mobile sdk apps won't need loginHost, connectedAppClientId, etc and it should be the template metadata which describes if these properties should be collected at all
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.
To expand on the comment, MobileNativeWorkflowState can be split into two sections: One for MSDK-essential state and another for app-specific state.
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.
yeah I think we're going to have to think holistically about state as the workflow grows - we have a lot of state which is transient and just a means to carry data from one node to the next and then we're done with it so we should have a strategy where we clear state when we are done with that part of the workflow - @khawkins we've discussed this briefly, might be worth a targeted convo as we're going to continually hit it with our various workflows as they grow
packages/mobile-native-mcp-server/src/tools/plan/sfmobile-native-template-selection/tool.ts
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/tools/plan/sfmobile-native-template-selection/tool.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/workflow/nodes/templatePropertiesExtraction.ts
Outdated
Show resolved
Hide resolved
| let templatePropertiesFlags = ''; | ||
| if (input.templateProperties && Object.keys(input.templateProperties).length > 0) { | ||
| const propertyFlags = Object.entries(input.templateProperties) | ||
| .map(([key, value]) => `--template-property-${key}="${value}"`) |
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.
Here we generate the shell command to run. Do we need to worry the command line to be valid in case value has special character for shell, like " here for the value?
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.
@haifeng-li-at-salesforce is doing work to move this work into a node I wonder if that's a more appropriate place to do deterministic validation since we'll also have access to the shell result - what do you think @haifeng-li-at-salesforce ? seems like doing that work within the tool itself is not appropriate - can you add it to your pr when resolving to do some basic validation on the input?
packages/mobile-native-mcp-server/src/tools/plan/sfmobile-native-project-generation/tool.ts
Outdated
Show resolved
Hide resolved
packages/mobile-native-mcp-server/src/workflow/nodes/templateSelection.ts
Show resolved
Hide resolved
haifeng-li-at-salesforce
left a comment
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.
Great job! There are a few comments from @ben-zhang-at-salesforce. Once those conversations are resolved, we can merge it.
ben-zhang-at-salesforce
left a comment
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.
good job!
What does this PR do?
Templates can have custom template properties (agentforce agent id, etc) in addition to the standard mobilesdk properties (loginHost, connectedAppId, etc)
Upon selecting a template, the custom template properties are extracted and the graph goes through a custom template extraction process to ensure all necessary template properties are extracted prior to using the template.
This also moves the describe of the template into the node itself and presents the available templates to the LLM (rather than instruct the LLM to execute the describe command on its own)