feat(apps): stop persisting intermediate status transitions to database #37167
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 4c145ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughRefactors AppManager internals to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppManager
participant ProxiedApp
participant DB
rect rgba(220,180,200,0.12)
Note right of AppManager: Old flow — per-call runtime status writes
Client->>AppManager: initializeApp(storageItem, app, saveToDb?)
AppManager->>ProxiedApp: initialize()
AppManager->>DB: save(runtime status)
end
rect rgba(180,220,200,0.12)
Note right of AppManager: New flow — ProxiedApp-centric, persist desired status only
Client->>AppManager: initializeApp(ProxiedApp, silenceStatus?)
AppManager->>ProxiedApp: initialize()
AppManager->>DB: save(desired status)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37167 +/- ##
===========================================
- Coverage 68.13% 68.11% -0.02%
===========================================
Files 3364 3364
Lines 115753 115753
Branches 20914 20909 -5
===========================================
- Hits 78866 78846 -20
- Misses 34192 34213 +21
+ Partials 2695 2694 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
force-pushed
the
feat/apps-target-status
branch
3 times, most recently
from
47356e7 to
aa6bc50
Compare
d-gubert
force-pushed
the
feat/apps-target-status
branch
from
6e4372d to
bd0f5a9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
856-877: Remove redundant in-memory status updates inchangeStatus()method.The code updates
storageItem.statusat lines 866 and 875, but these are unnecessary sinceupdateStatus()directly persists to the database without reading from the localstorageItemobject. After these lines execute, the method returnsrlrather thanstorageItem, so the in-memory updates have no functional purpose.Suggested fix: Remove lines 866 and 875 to simplify the code:
// Enable branch (lines 858-867) if (AppStatusUtils.isEnabled(status)) { if (AppStatusUtils.isEnabled(await rl.getStatus())) { throw new Error('Can not enable an App which is already enabled.'); } await this.enable(rl.getID()); await this.appMetadataStorage.updateStatus(storageItem._id, AppStatus.MANUALLY_ENABLED); } else { // Disable branch (lines 868-876) if (!AppStatusUtils.isEnabled(await rl.getStatus())) { throw new Error('Can not disable an App which is not enabled.'); } await this.disable(rl.getID(), AppStatus.MANUALLY_DISABLED); await this.appMetadataStorage.updateStatus(storageItem._id, AppStatus.MANUALLY_DISABLED); }
📜 Review details
Configuration used: CodeRabbit 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(12 hunks)
⏰ 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: 🚢 Build Docker Images for Testing (alpine)
🔇 Additional comments (7)
packages/apps-engine/src/server/AppManager.ts (7)
313-313: LGTM: Simplified initializeApp signature.The refactored call correctly passes the app instance and silences status updates during bulk initialization.
332-332: LGTM: Refactored enableApp call.The
isManualflag correctly reflects whether the app was previously manually enabled, aligning with the objective to track desired status.
636-644: LGTM: Simplified method calls.The refactored
installAppandinitializeAppcalls correctly use the new signatures, removing the now-unnecessarystorageItemparameters.
824-824: LGTM: Updated to new signature.The
initializeAppcall correctly uses the refactored signature.
967-974: LGTM: Refactored method calls.Both
initializeAppandenableAppcalls correctly use the new signatures and properly determine theisManualflag from the previous status.
987-998: LGTM: Startup process refactored correctly.The
runStartUpProcessmethod correctly uses the new method signatures forinitializeAppandenableApp, passing the appropriate parameters including theisManualflag.
1001-1116: LGTM: Refactored signatures align with PR objectives.The simplified method signatures effectively support the separation of target status from runtime status:
installApp(line 1001): RemovedstorageItemparameter, now takes onlyappanduser.initializeApp(line 1038): Streamlined to(app, silenceStatus), removingstorageItemandsaveToDbparameters.enableApp(line 1116): Refactored to(app, isManual, silenceStatus), removingstorageItemandsaveToDb. TheisManualflag explicitly tracks whether the enable action represents user intent (desired status) versus automatic state, which directly supports the PR's goal of storing only desired status in the database.These changes improve clarity and ensure that manual vs. automatic status distinctions are explicit throughout the app lifecycle.
d-gubert
force-pushed
the
feat/apps-target-status
branch
from
2e90cbf to
bcffc9c
Compare
d-gubert
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
856-856: Remove unnecessaryawaitkeyword.The
getStorageItem()method returnsIAppStorageItemsynchronously (not a Promise), so theawaitkeyword here is unnecessary and misleading.Apply this diff:
- const storageItem = await rl.getStorageItem(); + const storageItem = rl.getStorageItem();
📜 Review details
Configuration used: CodeRabbit 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 (5)
.changeset/real-pans-confess.md(1 hunks)apps/meteor/ee/server/apps/communication/rest.ts(1 hunks)apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts(1 hunks)apps/meteor/tests/end-to-end/apps/installation.ts(1 hunks)packages/apps-engine/src/server/AppManager.ts(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/real-pans-confess.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/apps-engine/src/server/AppManager.ts (1)
packages/apps-engine/src/server/ProxiedApp.ts (1)
ProxiedApp(16-160)
🔇 Additional comments (10)
packages/apps-engine/src/server/AppManager.ts (7)
313-313: LGTM!The
initializeAppcall correctly uses the new signature accepting theProxiedAppdirectly withsilenceStatus=true.
332-332: LGTM!The
enableAppcall correctly determinesisManualfrom the previous status, preserving the manual/auto distinction across server restarts.
561-561: LGTM! Past review concern addressed.The descriptor status now correctly reflects the
enableparameter, settingMANUALLY_ENABLEDwhen enabled andMANUALLY_DISABLEDwhen disabled. This resolves the status inconsistency flagged in the previous review.
636-636: LGTM!The
installAppcall uses the new simplified signature acceptingProxiedAppdirectly.
866-867: LGTM!The status persistence pattern correctly updates both the in-memory
storageItemand the database, ensuring consistency. This aligns with the PR's objective to persist only the desired status.Also applies to: 875-876
967-967: LGTM!All method calls use the new signatures consistently, passing
ProxiedAppdirectly and propagatingsilenceStatusandisManualparameters appropriately.Also applies to: 974-974, 987-987, 998-998
1001-1001: LGTM!The private method signatures have been successfully refactored to accept
ProxiedAppdirectly, simplifying the API and improving consistency. All call sites have been updated to match these new signatures.Also applies to: 1038-1038, 1116-1116
apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts (1)
104-104: LGTM!The test expectation correctly updated to reflect the new behavior where installed apps have a status of
manually_enabledinstead ofauto_enabled.apps/meteor/tests/end-to-end/apps/installation.ts (1)
55-55: LGTM!The test expectation correctly updated to
manually_enabled, consistent with the new app status management approach.apps/meteor/ee/server/apps/communication/rest.ts (1)
477-478: LGTM!The REST API correctly updated to use the new
changeStatusmethod and retrieves the status from the returnedProxiedAppobject, aligning with the refactored status management flow.
d-gubert
force-pushed
the
feat/apps-target-status
branch
from
6d44bc7 to
9d5a6af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
1001-1018: Well-executed refactoring of private helper methods.The method signatures have been simplified to accept
ProxiedAppdirectly:
installApp(app: ProxiedApp, user: IUser)(line 1001)initializeApp(app: ProxiedApp, silenceStatus = false)(line 1038)enableApp(app: ProxiedApp, isManual: boolean, silenceStatus = false)(line 1116)These methods correctly manage runtime status transitions without persisting them to the database, using
app.setStatus()for runtime state changes. All call sites have been updated consistently throughout the file.Also applies to: 1038-1069, 1116-1164
📜 Review details
Configuration used: CodeRabbit 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 (5)
.changeset/real-pans-confess.md(1 hunks)apps/meteor/ee/server/apps/communication/rest.ts(1 hunks)apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts(1 hunks)apps/meteor/tests/end-to-end/apps/installation.ts(1 hunks)packages/apps-engine/src/server/AppManager.ts(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
- apps/meteor/ee/server/apps/communication/rest.ts
- .changeset/real-pans-confess.md
- apps/meteor/tests/end-to-end/apps/installation.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/apps-engine/src/server/AppManager.ts (1)
packages/apps-engine/src/server/ProxiedApp.ts (1)
ProxiedApp(16-160)
⏰ 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). (3)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/apps-engine/src/server/AppManager.ts (2)
561-561: LGTM! Past review concern addressed.The status is now correctly set based on the
enableparameter. The database will storeMANUALLY_ENABLEDwhenenable=trueandMANUALLY_DISABLEDwhenenable=false, properly reflecting the desired status as per the PR objectives.
856-880: Correct implementation of desired status persistence.The
changeStatusmethod appropriately persists only the desired status (MANUALLY_ENABLEDorMANUALLY_DISABLED) to the database at lines 867 and 876. This aligns with the PR objectives:
- Desired status changes are explicitly persisted to the database
- Runtime status changes in
initializeAppandenableAppare NOT persisted- This prevents transient runtime states from affecting other instances
d-gubert
force-pushed
the
feat/apps-target-status
branch
from
9d5a6af to
25ba513
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
332-332: Consider passingsilenceStatusconsistently.At line 313,
initializeAppis called withsilenceStatus=true, but line 332 callsenableAppwithout passing thesilenceStatusparameter (defaulting tofalse). If the intent is to silence status notifications during the entireenableAllstartup process, consider passingtrueexplicitly here as well for consistency.- await this.enableApp(app).catch(console.error); + await this.enableApp(app, true).catch(console.error);
📜 Review details
Configuration used: CodeRabbit 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 (5)
.changeset/real-pans-confess.md(1 hunks)apps/meteor/ee/server/apps/communication/rest.ts(1 hunks)apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts(1 hunks)apps/meteor/tests/end-to-end/apps/installation.ts(1 hunks)packages/apps-engine/src/server/AppManager.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
- .changeset/real-pans-confess.md
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
PR: RocketChat/Rocket.Chat#37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/apps-engine/src/server/AppManager.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
PR: RocketChat/Rocket.Chat#36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
packages/apps-engine/src/server/AppManager.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure a clean state for each test execution
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/tests/end-to-end/apps/installation.ts
🧬 Code graph analysis (1)
packages/apps-engine/src/server/AppManager.ts (1)
packages/apps-engine/src/server/ProxiedApp.ts (1)
ProxiedApp(16-160)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/ee/server/apps/communication/rest.ts (1)
477-478: LGTM! Status handling is now explicit and clear.The change from
enable()tochangeStatus(info.id, AppStatus.MANUALLY_ENABLED)aligns well with the PR's objective to persist only the desired status. The explicit status parameter makes the intent clear, and retrieving the status viagetStatus()ensures consistency with the app's actual state.packages/apps-engine/src/server/AppManager.ts (4)
561-561: Good fix! Status now correctly reflects the enable parameter.The conditional status assignment addresses the previous review concern. The database now correctly stores
MANUALLY_DISABLEDwhenenable=false, ensuring the persisted state matches the user's intent.
1111-1159: LGTM! Runtime status handling without DB persistence.The
enableAppmethod correctly sets the runtime status toMANUALLY_ENABLED(line 1122) without directly persisting to the database. This aligns with the PR's objective: transient runtime status changes are not persisted, and only explicit user actions viachangeStatus()write to the DB.
642-644: Add integration tests covering app installation status flow with server restart.The code alignment at lines 561, 642, and 1122 is correct. However, verification found no integration tests covering the installation flow when
enable=true, specifically:
- Status persistence from initial
MANUALLY_ENABLED(line 561) through app startup (line 642)- Runtime status set to
MANUALLY_ENABLED(line 1122) viaenableApp- Status retention after server restart
Before merging, ensure integration tests are added to verify this complete flow, as current test coverage for this scenario is absent.
856-877: The in-memory update serves cache consistency but consider order of operations.The
storageItem.statusupdate (lines 866, 875) maintains consistency with the cached storage object returned bygetStorageItem(). However, sinceupdateStatus()is called with explicit status values rather thanstorageItem.status, and the in-memory update precedes the awaited database write, consider whether updating the cache after the database write succeeds would be safer—preventing stale cache ifupdateStatus()fails.apps/meteor/tests/end-to-end/apps/installation.ts (1)
55-55: LGTM! Test assertion updated to match new status handling.The expected status change from
'auto_enabled'to'manually_enabled'correctly reflects the implementation changes in AppManager where successful installations now explicitly setAppStatus.MANUALLY_ENABLED. This ensures test coverage aligns with the PR's objective to persist only the desired (manually set) status.
d-gubert
changed the title
d-gubert
force-pushed
the
feat/apps-target-status
branch
from
25ba513 to
220b797
Compare
d-gubert
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
d-gubert
force-pushed
the
feat/apps-target-status
branch
from
220b797 to
4c145ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
437-463: Consider makingenable()private.The
enable()method doesn't persist status changes to the database—onlychangeStatus()does (lines 866-867). Since the REST API and other external callers have been updated to usechangeStatus()instead,enable()now serves as an internal helper. Making it private would clarify its role and prevent external callers from bypassing persistence.Apply this diff to make the method private:
- public async enable(id: string): Promise<boolean> { + private async enable(id: string): Promise<boolean> {
📜 Review details
Configuration used: CodeRabbit 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 (5)
.changeset/real-pans-confess.md(1 hunks)apps/meteor/ee/server/apps/communication/rest.ts(1 hunks)apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts(1 hunks)apps/meteor/tests/end-to-end/apps/installation.ts(1 hunks)packages/apps-engine/src/server/AppManager.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/real-pans-confess.md
- apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts
- apps/meteor/ee/server/apps/communication/rest.ts
- apps/meteor/tests/end-to-end/apps/installation.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
packages/apps-engine/src/server/AppManager.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
packages/apps-engine/src/server/AppManager.ts
🧬 Code graph analysis (1)
packages/apps-engine/src/server/AppManager.ts (2)
packages/apps-engine/src/server/ProxiedApp.ts (1)
ProxiedApp(16-160)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
⏰ 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: CodeQL-Build
🔇 Additional comments (6)
packages/apps-engine/src/server/AppManager.ts (6)
313-313: LGTM! Initialization silenced appropriately during startup.During
enableAll, initialization notifications are silenced (line 313), while enabling notifications remain active (line 332). This ensures the UI is notified when apps reach their final enabled state without intermediate initialization noise.
558-568: ✅ Past issue resolved: Descriptor status now correctly reflects enable parameter.The descriptor status is now appropriately set to
MANUALLY_ENABLEDwhenenable=trueandMANUALLY_DISABLEDwhenenable=false(line 561). This ensures the database stores the correct desired status from the start.
640-645: ✅ Past issue resolved: Runtime status now matches database status.The previous inconsistency where
runStartUpProcesswas called withisManual=false(causingAUTO_ENABLEDruntime status while DB hadMANUALLY_ENABLED) has been resolved. The parameter was renamed tosilenceStatus(which controls notifications, not status value), andenableAppnow always setsMANUALLY_ENABLEDon success (line 1122). This ensures runtime and database status remain consistent from installation onward.
856-877: LGTM! Desired status persistence correctly centralized.The
changeStatusmethod properly centralizes desired status persistence per the PR objectives. After callingenable()ordisable()to set runtime status, it explicitly updates the in-memorystorageItem.statusand persists to the database (lines 866-867, 875-876). This ensures:
- Runtime status changes don't auto-persist (avoiding intermediate transitions in DB)
- Desired status is explicitly persisted only when changed by user intent
- In-memory storage item stays consistent with persisted state
980-994: LGTM! Clean refactoring to app-centric signatures.The
runStartUpProcessmethod has been refactored to delegate initialization and enabling to dedicated methods (initializeAppandenableApp) while maintaining thestorageItemparameter only for required settings validation. ThesilenceStatusparameter is properly threaded through to control notification behavior. This aligns well with the PR's goal of centralizing status management.
1111-1159: LGTM! Always using MANUALLY_ENABLED resolves previous inconsistency.The
enableAppmethod now consistently setsstatus = AppStatus.MANUALLY_ENABLEDon success (line 1122), removing the previous distinction between manual and automatic enabling that caused runtime/DB mismatches. The status is set viaapp.setStatus()(line 1156), which updates runtime status without persisting to the database—persistence is handled separately bychangeStatus(). This correctly implements the PR objective of avoiding intermediate status persistence.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
ARCH-1792
Summary by CodeRabbit
Refactor
Behavior
Tests
Chores