fix(apps-engine): stop initialization when installApp fails #38070
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
WalkthroughAppManager.add now captures the boolean result of installApp(app, user); on false it writes a storage error ("App installation failed") and returns early. On true it proceeds with the existing behavior: runStartUpProcess when enabled or initializeApp when disabled. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant AppManager
participant Installer as installApp()
participant Storage
participant Runtime as runStartUpProcess/initializeApp
Note over AppManager,Installer: Add app flow
Client->>AppManager: add(app, user, enable)
AppManager->>Installer: installApp(app, user)
alt install succeeded (true)
Installer-->>AppManager: true
AppManager->>Runtime: runStartUpProcess(app) / initializeApp(app)
Runtime-->>AppManager: success
AppManager-->>Client: return (success)
else install failed (false)
Installer-->>AppManager: false
AppManager->>Storage: storeError("App installation failed")
AppManager-->>Client: return (early)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @packages/apps-engine/src/server/AppManager.ts:
- Around line 645-649: The conditional block using enable with calls to
runStartUpProcess(created, app, false) and initializeApp(app) mixes tabs and
spaces; normalize the indentation to use consistent spaces (or the repo's
configured indent style) for the entire block and ensure the statements remain
exactly as runStartUpProcess(created, app, false) in the true branch and await
this.initializeApp(app) in the else branch so behavior is unchanged.
- Around line 638-641: The if-block in AppManager.ts has mixed tabs and spaces
around the installation failure handling (the condition checking the local
variable `installed`, the call to `aff.setStorageError('App installation
failed')`, and `return aff`), causing inconsistent indentation; fix it by
normalizing the indentation to match the project's style (convert the tabbed
lines to spaces or vice versa to match surrounding lines) for the block
containing `if (!installed) {`, `aff.setStorageError(...)`, and `return aff` so
the three lines use the same indentation characters and width.
🧹 Nitpick comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
643-644: Remove implementation comments.As per coding guidelines, avoid code comments in the implementation.
🔎 Proposed fix
- // Should enable === true, then we go through the entire start up process - // Otherwise, we only initialize it. if (enable) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/apps-engine/src/server/AppManager.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps-engine/src/server/AppManager.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
636-637: LGTM - Installation result properly captured.The boolean result from
installAppis correctly captured for validation before proceeding with startup/initialization.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/apps-engine/src/server/AppManager.ts">
<violation number="1" location="packages/apps-engine/src/server/AppManager.ts:638">
P1: Missing cleanup when installation fails. Other error cases in this method call `undoSteps` to clean up resources (app package, runtime, app user). Returning without cleanup may leave the system in an inconsistent state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38070 +/- ##
===========================================
- Coverage 70.71% 70.68% -0.03%
===========================================
Files 3142 3142
Lines 108819 108820 +1
Branches 19609 19578 -31
===========================================
- Hits 76949 76919 -30
- Misses 29869 29897 +28
- Partials 2001 2004 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@vednarvekar did you manage to cause the |
2 participants
Proposed changes
This PR fixes a control-flow issue in the Apps Engine where app initialization could continue even when
installAppfails.Previously, the return value of
installAppwas ignored, allowing the startup or initialization process to run despite a failed installation. This could leave the app in an inconsistent or partially-initialized state.Issue
#37472
What’s changed
installAppWhy this matters
No behavior changes occur for successful installations, and no additional side effects are introduced.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.