fix(apps): prevent unwanted changes to app status #36990
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 |
🦋 Changeset detectedLatest commit: 11ec639 The changes in this PR will be included in the next version bump. This PR includes changesets to release 43 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 |
WalkthroughReplaces full-document app storage updates with targeted partial-update methods across EE storage, engine abstractions, managers, migrations, and tests; removes legacy full-update signature and adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AM as AppManager
participant S as AppMetadataStorage
participant DB as DB
Note over AM,S: Persist status only (targeted)
AM->>S: updateStatus(_id, status)
S->>DB: updateOne({ _id }, { $set: { status } })
DB-->>S: { modifiedCount }
S-->>AM: boolean
Note over AM,S: Partial update and return document
AM->>S: updatePartialAndReturnDocument({ _id, signature?, migrated?, marketplaceInfo? }, { unsetPermissionsGranted? })
S->>DB: findOneAndUpdate({ _id }, { $set: fields, $unset?: { permissionsGranted: "" } }, { returnDocument: "after" })
DB-->>S: updatedDocument
S-->>AM: IAppStorageItem
sequenceDiagram
autonumber
participant ASM as AppSettingsManager
participant RL as RuntimeLayer
participant S as AppMetadataStorage
participant DB as DB
ASM->>RL: getStorageItem()
RL-->>ASM: storageItem
ASM->>S: updateSetting(storageItem._id, decoratedSetting)
S->>DB: updateOne({ _id }, { $set: { "settings.<id>": decoratedSetting } })
DB-->>S: { modifiedCount }
S-->>ASM: boolean
ASM->>RL: call(ONSETTINGUPDATED, decoratedSetting)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-16T22:08:51.490ZApplied to files:
🧬 Code graph analysis (1)apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts (1)
⏰ 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). (7)
🔇 Additional comments (8)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
d-gubert
force-pushed
the
feat/split-app-metadata-storage-update-method
branch
from
21841e3 to
1385656
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36990 +/- ##
===========================================
+ Coverage 67.37% 67.38% +0.01%
===========================================
Files 3330 3330
Lines 113482 113565 +83
Branches 20598 20599 +1
===========================================
+ Hits 76454 76531 +77
- Misses 34418 34423 +5
- Partials 2610 2611 +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/split-app-metadata-storage-update-method
branch
5 times, most recently
from
59bcc99 to
5911502
Compare
d-gubert
changed the title
d-gubert
force-pushed
the
feat/split-app-metadata-storage-update-method
branch
4 times, most recently
from
c2fc4e0 to
8c2f214
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts (4)
73-76: Consider addingupdatedAttimestamp for consistency.The method correctly updates the status field, but it doesn't set
updatedAt. For audit trail consistency and to maintain the pattern established in thecreatemethod, consider tracking modification time.Apply this diff:
public async updateStatus(_id: string, status: AppStatus): Promise<boolean> { - const result = await this.db.updateOne({ _id }, { $set: { status } }); + const result = await this.db.updateOne({ _id }, { $set: { status, updatedAt: new Date() } }); return result.modifiedCount > 0; }
78-82: Consider addingupdatedAttimestamp for consistency.The method correctly updates a specific setting using dot notation, but like
updateStatus, it doesn't setupdatedAtfor audit tracking.Apply this diff:
public async updateSetting(_id: string, setting: ISetting): Promise<boolean> { - const result = await this.db.updateOne({ _id }, { $set: { [`settings.${setting.id}`]: setting } }); + const result = await this.db.updateOne({ _id }, { $set: { [`settings.${setting.id}`]: setting, updatedAt: new Date() } }); return result.modifiedCount > 0; }
84-87: Consider addingupdatedAttimestamp for consistency.The method correctly replaces the
infoobject, but should setupdatedAtto maintain the audit trail pattern.Apply this diff:
public async updateAppInfo(_id: string, info: IAppInfo): Promise<boolean> { - const result = await this.db.updateOne({ _id }, { $set: { info } }); + const result = await this.db.updateOne({ _id }, { $set: { info, updatedAt: new Date() } }); return result.modifiedCount > 0; }
89-92: Consider addingupdatedAttimestamp for consistency.The method correctly replaces the
marketplaceInfoarray, but should setupdatedAtto maintain the audit trail pattern.Apply this diff:
public async updateMarketplaceInfo(_id: string, marketplaceInfo: IMarketplaceInfo[]): Promise<boolean> { - const result = await this.db.updateOne({ _id }, { $set: { marketplaceInfo } }); + const result = await this.db.updateOne({ _id }, { $set: { marketplaceInfo, updatedAt: new Date() } }); return result.modifiedCount > 0; }
📜 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 (8)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts(2 hunks)apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts(3 hunks)apps/meteor/server/startup/migrations/v294.ts(1 hunks)apps/meteor/server/startup/migrations/v307.ts(1 hunks)packages/apps-engine/src/server/AppManager.ts(8 hunks)packages/apps-engine/src/server/managers/AppSettingsManager.ts(2 hunks)packages/apps-engine/src/server/storage/AppMetadataStorage.ts(2 hunks)packages/apps-engine/tests/server/managers/AppSettingsManager.spec.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 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:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.tsapps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.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:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.tsapps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.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 test.beforeAll() and test.afterAll() for setup and teardown
Applied to files:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.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:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.tsapps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.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:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.tsapps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.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:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.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:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.tsapps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.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} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
packages/apps-engine/tests/server/managers/AppSettingsManager.spec.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/ee/tests/unit/server/apps/AppRealStorage.spec.ts
🧬 Code graph analysis (1)
apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts (1)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts (1)
AppRealStorage(11-93)
🔇 Additional comments (32)
packages/apps-engine/src/server/managers/AppSettingsManager.ts (2)
37-50: Partial update implementation looks correct.The change correctly uses
updateSettingfor a targeted partial update, which aligns with the PR's goal of reducing race condition risk. The local mutation ofstorageItem.settings[decoratedSetting.id]at line 48 maintains consistency between in-memory state and persisted storage.
54-54: New post-update callback added.The addition of
ONSETTINGUPDATEDcallback provides a symmetrical hook to complementON_PRE_SETTING_UPDATE, allowing apps to react after a setting has been persisted.packages/apps-engine/tests/server/managers/AppSettingsManager.spec.ts (6)
1-1: LGTM: Import addition for enhanced matching.The
Anymatcher is added to support flexible assertion matching in line 143.
4-4: LGTM: Type import for new API.The
ISettingtype import aligns with the newupdateSettingmethod signature that takes a setting object.
37-37: LGTM: Mock data enhanced with _id.Adding
_idto the mock storage item supports the new partial update API that requires document identifiers.
59-62: LGTM: Mock storage updated to new API.The mock correctly reflects the new
updateSetting(appId: string, setting: ISetting): Promise<boolean>signature, replacing the old full-documentupdatemethod.
130-130: LGTM: Spy updated for new method.The spy now targets
updateSettinginstead of the removedupdatemethod.
143-144: LGTM: Assertion validates new API contract.The test correctly verifies that
updateSettingis called with the app's_idand the setting object, ensuring the transition from full-document updates to field-specific updates.apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts (8)
3-3: LGTM: Import for test data construction.
SettingTypeis needed to construct validISettingobjects in theupdateSettingtests.
14-21: LGTM: Enhanced mock database structure.The explicit typing of mockDb stubs improves type safety and makes the test setup clearer. The
updateOnestub supports the new partial update methods.Also applies to: 30-30
136-145: LGTM: Remove method test updated.The test correctly validates that
removecallsdeleteOneand returns{ success: true }.
147-186: LGTM: Comprehensive coverage of updatePartialAndReturnDocument.The tests cover all three cases:
unsetPermissionsGranted: true- correctly validates $unset operationunsetPermissionsGranted: false- validates standard $set without $unset- No options - validates default behavior
This ensures the conditional unset logic works correctly.
188-206: LGTM: updateStatus tests validate boolean return.Tests correctly verify both success (modifiedCount > 0) and failure (modifiedCount = 0) cases, ensuring callers can detect when status updates don't take effect.
208-244: LGTM: updateSetting tests validate nested path updates.The tests correctly verify that settings are updated using the nested path
settings.${setting.id}rather than replacing the entire settings object. This is crucial for the race condition mitigation strategy.
246-296: LGTM: updateAppInfo tests cover success and failure.Both test cases validate the expected MongoDB update query structure and boolean return values based on
modifiedCount.
298-372: LGTM: updateMarketplaceInfo tests provide comprehensive coverage.The tests validate updating marketplace info with realistic data structures and verify both success and failure scenarios.
packages/apps-engine/src/server/storage/AppMetadataStorage.ts (6)
2-5: LGTM: Type imports for new method signatures.The imported types support the new granular update methods, enabling type-safe partial updates.
24-27: LGTM: Partial update with optional unset behavior.The
updatePartialAndReturnDocumentmethod signature correctly:
- Takes
Partial<IAppStorageItem>to support partial updates- Includes
unsetPermissionsGrantedoption for conditional field removal- Returns the updated document for immediate use
This addresses the race condition risk by allowing selective field updates rather than full document replacement.
29-29: LGTM: Granular status update method.Using
_idas the identifier and returning a boolean success indicator provides a clean API for status-only updates, reducing the risk of overwriting other fields during concurrent operations.
31-31: LGTM: Setting-specific update method.This method enables updating a single setting without touching other app metadata, directly addressing the race condition concern mentioned in the PR objectives.
33-33: LGTM: App info update method.Granular update for app info field reduces race condition exposure.
35-35: LGTM: Marketplace info update method.Field-specific update for marketplace information aligns with the PR's objective of reducing race conditions.
packages/apps-engine/src/server/AppManager.ts (9)
465-465: LGTM: Granular status update reduces race condition risk.Replacing the full document update with
updateStatusensures only the status field is modified, preventing concurrent operations from overwriting other fields with stale data.
498-498: LGTM: Status-only update in disable flow.Consistent use of
updateStatusfor granular updates.
517-517: LGTM: Error suppression for non-critical validation.The
.catch(() => {})is appropriate here as license validation failures don't prevent the migration flow.
522-523: LGTM: Selective field update in migrate method.Destructuring
{ marketplaceInfo, signature, migrated, _id }and passing only these fields toupdatePartialAndReturnDocumentensures the update touches only the necessary fields, significantly reducing race condition exposure compared to the previous full-document update.
751-753: LGTM: Partial update with conditional unset.Using
updatePartialAndReturnDocumentwith theunsetPermissionsGrantedoption allows the update to conditionally remove thepermissionsGrantedfield whenundefinedis passed, while only updating the fields present in the descriptor. This is a cleaner approach than full document replacement.
908-908: LGTM: Marketplace-specific update method.Using
updateMarketplaceInfofor targeted updates prevents overwriting unrelated app metadata during marketplace info synchronization.
944-944: LGTM: Status update with error handling.The
.catch(console.error)provides visibility into update failures while allowing the process to continue.
1075-1075: LGTM: Status persistence in initialization.Granular status update reduces race condition risk during app initialization.
1179-1179: LGTM: Status update in enableApp.Consistent use of
updateStatusfor status persistence.apps/meteor/ee/server/apps/storage/AppRealStorage.ts (1)
1-4: LGTM! Type imports are appropriate.The new type imports correctly support the granular update methods introduced in this refactor.
d-gubert
force-pushed
the
feat/split-app-metadata-storage-update-method
branch
from
8c2f214 to
e507dc5
Compare
d-gubert
changed the title
ggazzo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts (1)
60-74: Fix missing updatedAt and clarify permissionsGranted handling.
Major: The method does not set
updatedAt, which is inconsistent with thecreatemethod (Line 18). Partial updates should also track when the document was last modified.Minor: The
permissionsGranteddeletion logic on Line 69 mutatesitemafter it has been assigned toupdateQuery.$set(Line 65). While this works because JavaScript objects are passed by reference, it is fragile and unclear. It would be clearer to excludepermissionsGrantedfrom the destructureditemwhenunsetPermissionsGrantedis true.Apply this diff to fix the missing
updatedAtand improve clarity:public async updatePartialAndReturnDocument( { _id, ...item }: IAppStorageItem, { unsetPermissionsGranted = false } = {}, ): Promise<IAppStorageItem> { + const updateFields = { ...item }; + if (unsetPermissionsGranted) { + delete updateFields.permissionsGranted; + } + const updateQuery: UpdateFilter<IAppStorageItem> = { - $set: item, + $set: { + ...updateFields, + updatedAt: new Date(), + }, }; if (unsetPermissionsGranted) { - delete item.permissionsGranted; updateQuery.$unset = { permissionsGranted: 1 }; } return this.db.findOneAndUpdate({ _id }, updateQuery, { returnDocument: 'after' }); }
🧹 Nitpick comments (4)
packages/apps-engine/src/server/AppManager.ts (4)
465-465: Consider logging errors for observability.The empty catch silently swallows any database errors during status persistence. While this is marked as intentional (fire-and-forget), logging the error would improve operational visibility without blocking the flow.
- await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {}); + await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(console.error);
498-498: Consider logging errors for observability.Same pattern as Line 465 - the empty catch silently swallows database errors. Consider logging for operational visibility.
- await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {}); + await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(console.error);
517-517: Consider logging license validation errors.The empty catch silently swallows license validation errors. If validation fails, the app continues to be marked as migrated, which might not be the desired behavior. Consider logging the error or handling validation failure explicitly.
- await app.validateLicense().catch(() => {}); + await app.validateLicense().catch(console.error);
1179-1179: Consider logging errors for observability.Consistent with Lines 465 and 498, the empty catch silently swallows database errors. Consider logging for operational visibility.
- await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {}); + await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).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 (9)
.changeset/gorgeous-cougars-visit.md(1 hunks)apps/meteor/ee/server/apps/storage/AppRealStorage.ts(2 hunks)apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts(3 hunks)apps/meteor/server/startup/migrations/v294.ts(1 hunks)apps/meteor/server/startup/migrations/v307.ts(1 hunks)packages/apps-engine/src/server/AppManager.ts(8 hunks)packages/apps-engine/src/server/managers/AppSettingsManager.ts(2 hunks)packages/apps-engine/src/server/storage/AppMetadataStorage.ts(2 hunks)packages/apps-engine/tests/server/managers/AppSettingsManager.spec.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps-engine/tests/server/managers/AppSettingsManager.spec.ts
- apps/meteor/server/startup/migrations/v307.ts
- apps/meteor/server/startup/migrations/v294.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/ee/tests/unit/server/apps/AppRealStorage.spec.ts
📚 Learning: 2025-09-30T13:00:05.440Z
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.440Z
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:
apps/meteor/ee/server/apps/storage/AppRealStorage.ts
🧬 Code graph analysis (1)
apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts (1)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts (1)
AppRealStorage(11-96)
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (21)
.changeset/gorgeous-cougars-visit.md (1)
1-7: LGTM!The changeset correctly documents the patch-level changes to both packages and provides a clear description that aligns with the PR objectives.
packages/apps-engine/src/server/managers/AppSettingsManager.ts (3)
37-39: LGTM!Caching
storageItemlocally and readingoldSettingfrom it ensures consistent reads and aligns with the partial update strategy.
47-48: Good: updatedAt timestamp and in-memory update.Setting
updatedAton the decorated setting before persisting is correct and maintains consistency with thecreatemethod pattern. The in-memory mutation ofstorageItem.settingsis appropriate since the storage update on Line 50 will persist the change.
50-54: LGTM! Aligns with partial update strategy.The call to
updateSettingreplaces the previous full-document update pattern and correctly persists only the modified setting. The subsequentONSETTINGUPDATEDnotification ensures app lifecycle hooks are invoked after persistence.apps/meteor/ee/server/apps/storage/AppRealStorage.ts (4)
1-4: LGTM!The new type imports correctly support the granular update methods introduced in this PR.
76-79: LGTM!The
updateStatusmethod correctly performs a targeted status update and returns a boolean indicating whether the document was modified, aligning with the partial update strategy.
81-85: LGTM!The
updateSettingmethod correctly updates a single nested setting using thesettings.${setting.id}path, which is the appropriate way to perform granular updates on nested objects in MongoDB.
87-95: LGTM!Both
updateAppInfoandupdateMarketplaceInfomethods correctly perform targeted field updates and return boolean values indicating modification success, consistent with the other update methods.apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts (7)
3-3: LGTM!The test setup correctly adds the necessary imports, stubs, and type definitions to support the new storage methods.
Also applies to: 14-21, 30-33
136-145: LGTM!The test correctly validates the idempotent
removemethod, which unconditionally returns{ success: true }as per the design. Based on learnings.
147-186: Tests align with implementation but should validate updatedAt.The tests correctly validate the
updatePartialAndReturnDocumentmethod's behavior, including theunsetPermissionsGrantedoption. However, they do not assert thatupdatedAtis set, which should be added after the implementation is fixed to includeupdatedAt(as noted in the review of AppRealStorage.ts).
188-206: LGTM!The
updateStatustests correctly validate both success (modifiedCount > 0) and no-op (modifiedCount === 0) scenarios, ensuring the method returns the appropriate boolean value.
208-244: LGTM!The
updateSettingtests correctly validate the nested path update (settings.test-setting) and return value logic for both modified and unmodified scenarios.
246-296: LGTM!The
updateAppInfotests comprehensively validate the info field update and boolean return logic for both success and no-op cases.
298-372: LGTM!The
updateMarketplaceInfotests thoroughly validate the array field update and boolean return logic for both modified and unmodified scenarios.packages/apps-engine/src/server/storage/AppMetadataStorage.ts (2)
2-5: LGTM!The new type imports correctly support the granular update method signatures introduced in this file.
29-35: LGTM!The new abstract methods for targeted updates have consistent signatures, taking
_idand the respective data type, and returning a boolean to indicate modification success. This aligns well with the partial update strategy.packages/apps-engine/src/server/AppManager.ts (4)
522-523: LGTM! Excellent use of targeted partial updates.The code correctly extracts only the necessary fields (marketplaceInfo, signature, migrated, _id) for the partial update, avoiding full document replacement and reducing race condition risks. This aligns perfectly with the PR objectives.
908-908: LGTM! Proper use of targeted update method.The code correctly uses the new
updateMarketplaceInfomethod to update only the marketplace information field, avoiding full document replacement. This aligns with the PR objectives to reduce race condition risks.
944-944: LGTM! Proper error handling with logging.This updateStatus call correctly handles errors with
console.error, providing operational visibility while maintaining the fire-and-forget pattern.
751-753: Restrict fields in updatePartialAndReturnDocument
The call at Line 751 passes the fulldescriptorintoupdatePartialAndReturnDocument(…), which will persist every provided field (and unsetpermissionsGrantedwhen undefined). Unlike the targeted update on Line 523, this may overwrite unchanged properties and risk race conditions. If you truly intend to update all descriptor fields here, add a clarifying comment; otherwise, pass only the specific properties that need updating.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts (1)
60-78: Critical: Fix permissionsGranted deletion logic and add updatedAt.The current implementation has several issues:
Critical: Line 69 assigns
$set: item, capturing all fields at that moment. Line 73 then deletesitem.permissionsGranted, but this mutation doesn't affect the already-constructedupdateQuery.$set. The result is that MongoDB will process both$set: { permissionsGranted: [...] }and$unset: { permissionsGranted: 1 }, which is inefficient and confusing.Major: Missing
updatedAttimestamp. Thecreatemethod sets bothcreatedAtandupdatedAt(lines 17-18), but this update method doesn't maintainupdatedAt, breaking consistency.Apply this diff to fix both issues:
public async updatePartialAndReturnDocument( { _id, ...item }: IAppStorageItem, { unsetPermissionsGranted = false } = {}, ): Promise<IAppStorageItem> { if (!_id) { throw new Error('Property _id is required to update an app storage item'); } + if (unsetPermissionsGranted) { + delete item.permissionsGranted; + } + const updateQuery: UpdateFilter<IAppStorageItem> = { $set: { + ...item, + updatedAt: new Date(), - item, }, }; if (unsetPermissionsGranted) { - delete item.permissionsGranted; updateQuery.$unset = { permissionsGranted: 1 }; } return this.db.findOneAndUpdate({ _id }, updateQuery, { returnDocument: 'after' }); }This ensures:
permissionsGrantedis excluded from$setbefore constructing the update query whenunsetPermissionsGrantedistrueupdatedAtis always set to track modification time- The object spread creates a clean copy for
$set
📜 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 (3)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts(2 hunks)apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts(3 hunks)packages/apps-engine/src/server/AppManager.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/apps-engine/src/server/AppManager.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/ee/tests/unit/server/apps/AppRealStorage.spec.ts
📚 Learning: 2025-09-30T13:00:05.440Z
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.440Z
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:
apps/meteor/ee/server/apps/storage/AppRealStorage.ts
🧬 Code graph analysis (1)
apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts (1)
apps/meteor/ee/server/apps/storage/AppRealStorage.ts (1)
AppRealStorage(11-100)
⏰ 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 (11)
apps/meteor/ee/tests/unit/server/apps/AppRealStorage.spec.ts (5)
136-145: LGTM!The test correctly validates the idempotent behavior of
remove, expecting{ success: true }regardless of whether a document was deleted.
212-230: LGTM!The tests correctly validate
updateStatusbehavior, returningtruewhen a document is modified andfalseotherwise.
232-268: LGTM!The tests correctly validate
updateSettingbehavior, including the use of the nested path'settings.${setting.id}'for targeted updates.
270-320: LGTM!The tests correctly validate
updateAppInfobehavior for both successful and unsuccessful modifications.
322-396: LGTM!The tests correctly validate
updateMarketplaceInfobehavior for both successful and unsuccessful modifications.apps/meteor/ee/server/apps/storage/AppRealStorage.ts (6)
1-4: LGTM!The new type imports support the targeted update methods, aligning with the PR's objective to replace full-document updates with field-specific operations.
55-58: LGTM!The
removemethod correctly implements idempotent deletion behavior, returning{ success: true }to indicate the app is removed (regardless of whether this call performed the deletion). Database errors will throw exceptions.Based on learnings.
80-83: LGTM!The
updateStatusmethod correctly implements a targeted field update, reducing race condition risk by updating only thestatusfield.
85-89: LGTM!The
updateSettingmethod correctly uses MongoDB dot notation to update a specific nested setting, avoiding full-document replacement.
91-94: LGTM!The
updateAppInfomethod correctly implements a targeted field update for theinfoobject.
96-99: LGTM!The
updateMarketplaceInfomethod correctly implements a targeted field update for marketplace metadata.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Making full document updates makes the system more sensitive do race conditions, as data can be overriden by their outdated counterparts.
This could cause an invalid app signature to be stored and lead to the app being marked as invalid_installation_disabled during server startup
By only updating the fields we need to modify, we reduce the chances of being hit by side effects of race conditions
Issue(s)
Steps to test or reproduce
Further comments
ARCH-1796
Summary by CodeRabbit