fix(apps): prevent installation invalidation on app cron updates #37152
Conversation
🦋 Changeset detectedLatest commit: 4564a22 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 |
|
Looks like this PR is ready to merge! 🎉 |
WalkthroughImplements signed marketplace info updates in AppManager using partial storage updates and preserves scheduled jobs during license purge. Updates test utilities, storage test stubs, and multiple specs to use IAppStorageItem shape and new storage APIs. Adds a changeset patching dependencies and noting a bug fix. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Scheduler as Scheduler/Update Trigger
participant AppManager
participant Storage as App Storage
participant Signer as Signature Service
Scheduler->>AppManager: updateAppsMarketplaceInfo(appsOverview)
loop For each app with subscription changes
AppManager->>Signer: sign(updatedMarketplaceItem)
Signer-->>AppManager: signature
AppManager->>Storage: updatePartialAndReturnDocument({ _id, marketplaceInfo, signature })
Storage-->>AppManager: updatedDocument
end
Note over AppManager: If license invalid -> purgeAppConfig({ keepScheduledJobs: true })
AppManager-->>Scheduler: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps-engine/src/server/AppManager.ts (1)
886-915: Empty catch silently swallows all errors from marketplace info updates.The empty catch block at line 915 suppresses all errors from the
updateAppsMarketplaceInfocall, including:
- Database update failures (line 909-913)
- Signature generation failures (line 907)
- License validation failures (lines 922-939)
This could hide critical issues and make debugging difficult. Consider at minimum logging errors or failing fast for critical failures.
Apply this diff to add error logging:
).catch(() => {}); + ).catch((error) => { + console.error('Failed to update apps marketplace info:', 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/rotten-jars-occur.md(1 hunks)packages/apps-engine/src/server/AppManager.ts(2 hunks)packages/apps-engine/tests/server/AppManager.spec.ts(3 hunks)packages/apps-engine/tests/server/managers/AppApiManager.spec.ts(2 hunks)packages/apps-engine/tests/server/managers/AppSlashCommand.spec.ts(2 hunks)packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts(2 hunks)packages/apps-engine/tests/server/managers/AppVideoConfProviderManager.spec.ts(2 hunks)packages/apps-engine/tests/test-data/storage/storage.ts(2 hunks)packages/apps-engine/tests/test-data/utilities.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/tests/test-data/storage/storage.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/AppManager.spec.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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (11)
packages/apps-engine/tests/server/managers/AppApiManager.spec.ts (1)
19-19: LGTM! Test scaffolding updated to use IAppStorageItem shape.The mock app construction now correctly uses the nested
info: { id, name }structure, aligning with the IAppStorageItem interface.Also applies to: 41-41
packages/apps-engine/tests/server/managers/AppSlashCommand.spec.ts (1)
3-3: LGTM! Consistent with IAppStorageItem adoption.Also applies to: 15-15
packages/apps-engine/tests/server/managers/AppVideoConfProviderManager.spec.ts (1)
11-11: LGTM! Test updated to use IAppStorageItem shape.Also applies to: 31-31
packages/apps-engine/tests/server/managers/AppSlashCommandManager.spec.ts (1)
20-20: LGTM! Consistent test scaffolding update.Also applies to: 42-42
packages/apps-engine/src/server/AppManager.ts (1)
936-936: Verify that preserving scheduled jobs on invalid license is intentional.The addition of
keepScheduledJobs: truechanges the cleanup behavior for apps with invalid licenses. Previously, scheduled jobs were removed when an app's license became invalid. Now they're preserved.Ensure this aligns with the intended behavior and doesn't cause issues with:
- Disabled apps continuing to execute scheduled jobs
- Scheduled jobs running without valid licensing
- Resource cleanup expectations
If this behavior is correct, consider adding a comment explaining why scheduled jobs should be preserved even when the license is invalid.
packages/apps-engine/tests/server/AppManager.spec.ts (1)
1-1: LGTM! Excellent test coverage for marketplace info updates.The four new test methods comprehensively cover:
- Skipping apps without subscription info
- Skipping apps not in manager
- Skipping apps with same license
- Updating subscription and signing app
The tests properly use spies to verify the expected behavior and follow good testing practices.
Also applies to: 17-17, 125-261
packages/apps-engine/tests/test-data/utilities.ts (4)
547-555: LGTM! getMockApp updated to support IAppStorageItem shape.The refactored signature now accepts
Partial<IAppStorageItem>and extracts id/name from the nestedinfofield with sensible defaults.
575-601: LGTM! Comprehensive marketplace info test data generator.
603-630: LGTM! Complete storage item test data generator.Provides all required fields with sensible defaults, making test setup easier.
632-640: LGTM! Helper for apps overview test data.packages/apps-engine/tests/test-data/storage/storage.ts (1)
1-4: LGTM!The type-only imports are correctly added to support the new method signatures.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37152 +/- ##
========================================
Coverage 67.66% 67.67%
========================================
Files 3342 3342
Lines 114030 114041 +11
Branches 20678 20680 +2
========================================
+ Hits 77157 77173 +16
+ Misses 34195 34188 -7
- Partials 2678 2680 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
changed the title
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
ARCH-1818
Summary by CodeRabbit
Bug Fixes
Chores