On app selecetion keep retrying when failing but avoid capturing unrelated errors#6904
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report
Test suite run success3774 tests passing in 1448 suites. Report generated by 🧪jest coverage report action from 47e6658 |
alfonso-noriega
force-pushed
the
02-27-on_app_selecetion_keep_retrying_when_failing_but_avoid_capturing_unrelated_errors
branch
from
f35bc45 to
47e6658
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
| ) | ||
| } | ||
| return appChoice | ||
| return currentAppChoices.find((app) => app.apiKey === apiKey)! |
There was a problem hiding this comment.
Non-null assertion can crash with unhelpful error (diagnostics regression)
selectAppPrompt now returns currentAppChoices.find((app) => app.apiKey === apiKey)!. If renderAutocompletePrompt returns a value not present in currentAppChoices (the incident described in the PR links), find(...) returns undefined and ! can cause a runtime failure with a generic error instead of a targeted, actionable message.
Impact:
- User:
shopify app devcan crash during selection with a confusing error, blocking development. - Support/diagnostics: reduced ability to diagnose prompt/list mismatch issues.
- Scale: affects any environment where prompt selection can desync from the in-memory list.
- Consequences: hard crash + loss of diagnostic context.
| apps, | ||
| hasMorePages, | ||
| {directory: options.directory}, | ||
| ) |
There was a problem hiding this comment.
Retry behavior removed for prompt failures; loop no longer matches intent
The PR removes the try/catch around await selectAppPrompt(...) and around appFromIdentifiers. If selectAppPrompt throws (prompt interrupted/UI failure/etc.), the exception exits selectOrCreateApp immediately and no retry occurs, even though the loop is still present. This contradicts the PR intent (“keep retrying…”) and the previous behavior (retrying non-BugError failures). The old code already avoided retrying BugError; removing the catch stops retries on prompt errors entirely.
Impact:
- User: transient prompt interruptions won’t be retried; immediate failure.
- Infra/support: more repeated command invocations/tickets; less observability.
- Scale: any user with flaky terminal/container/WSL interruptions.
- Consequences: higher failure rate and worse UX.
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - 2 findings 📋 History✅ 2 findings |
1 participant
WHY are these changes introduced?
Vault Error
GH Issue #15725
Observe link
The app selection logic contained redundant error handling and retry mechanisms that were making the code unnecessarily complex and potentially masking actual issues during the selection process.
WHAT is this pull request doing?
selectAppPromptfunction that was checking for null after using the non-null assertion operatorselectOrCreateAppby removing try-catch blocks and letting natural control flow handle retriesHow to test your changes?
shopify app devin a project directoryMeasuring impact
How do we know this change was effective? Please choose one:
Checklist