fix: restore banner dismissal fallback for cloud announcements#38282
fix: restore banner dismissal fallback for cloud announcements#38282kodiakhq[bot] merged 4 commits intodevelopfrom
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: 7540b00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 banner dismissal to accept Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as CloudAnnouncementsModule
participant Model as BannersModel
participant DB as Database
Client->>Server: viewClosed({ viewId?, id? })
Server->>Model: findByIds([viewId, id].filter(Boolean))
Model->>DB: find({_id: { $in: [...] }})
DB-->>Model: matching banner docs
Model-->>Server: banner cursor/array
Server->>Server: select announcement by viewId or id
Server->>Server: determine interaction from announcement.surface
Server->>Model: dismiss(announcement._id, userId)
Server-->>Client: { viewId: announcement._id, type: interaction }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38282 +/- ##
===========================================
+ Coverage 70.77% 70.78% +0.01%
===========================================
Files 3158 3158
Lines 109355 109359 +4
Branches 19648 19677 +29
===========================================
+ Hits 77397 77414 +17
+ Misses 29934 29914 -20
- Partials 2024 2031 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/modules/core-apps/cloudAnnouncements.module.ts`:
- Around line 65-69: The current lookup using bannerIds and Banners.findOne({
_id: { $in: bannerIds } }) is non-deterministic when both id and viewId exist;
change to an ordered lookup that prefers id then falls back to viewId: first
attempt a findOne for _id equal to id (using the same projection), and only if
that returns null/undefined attempt a second findOne for _id equal to viewId
(keeping projection). Update the code around bannerIds, Banners.findOne, id, and
viewId to implement this deterministic fallback.
🧹 Nitpick comments (1)
apps/meteor/server/modules/core-apps/cloudAnnouncements.module.ts (1)
64-64: Drop the inline comment to keep implementation comment-free.The code is self-explanatory; consider removing the comment at Line 64 to align with the no-comments guideline.
Proposed change
- // Find banner by _id matching either view.id or view.viewId for backwards compatibilityAs per coding guidelines, avoid code comments in the implementation.
Customers reported that the "Enterprise plan active" popup displays on every server restart. When inspecting the network tab, a POST request to
/api/apps/ui.interaction/cloud-announcements-corereturns error 500 with "Banner not found".Upon investigation, we found that some banners from Cloud have different values for
view.idandview.viewId. For example:view.id: "enterprise_active_main"view.viewId: "enterprise_active"_id: "enterprise_active"The change was introduced in PR #31090 (a051362) when the cloud announcements system was refactored from using
CloudAnnouncementsmodel to theBannerservice. During this refactor, the fallback logic for resolving the banner ID was removed.This mismatch is expected and was handled by the original code, but the refactored code assumed they would always match.
Proposed changes (including videos or screenshots)
Find the banner by
_idmatching eitherview.idorview.viewId, then dismiss using the actual_id.Issue(s)
Steps to test or reproduce
Manual Test Scenarios
view.idview.viewId_id_ididcorrect only_idviewIdcorrect only_idSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.