fix: Production=false not being respected by apn configuration#38852
Merged
Conversation
Contributor
|
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: dc99d15 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
Contributor
WalkthroughAdds a changeset documenting a patch release that fixes Push Notifications initialization to respect the Production flag setting. Includes test configuration updates, new unit tests for APN provider initialization, and a code fix ensuring the production flag is explicitly passed to the APN provider constructor. Changes
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 #38852 +/- ##
===========================================
+ Coverage 70.55% 70.59% +0.04%
===========================================
Files 3188 3189 +1
Lines 112666 112703 +37
Branches 20395 20413 +18
===========================================
+ Hits 79494 79567 +73
+ Misses 31113 31078 -35
+ Partials 2059 2058 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
approved these changes
Feb 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/meteor/app/push/server/apn.spec.ts (3)
55-130: Missing test for the core bug scenario:options.productionoverridingoptions.apn.productionThe root cause in CORE-1771 is that
production: truecould end up insideoptions.apn, whileoptions.productionisfalse. The fix relies on the trailingproduction: options.productionin the spread to win. No test currently validates this override. Consider adding:it('should override production in apn options with top-level options.production', () => { initAPN({ options: buildOptions({ production: false }, { production: true } as Record<string, unknown>), absoluteUrl: 'https://example.com', }); expect(mocks.ApnProvider.firstCall.args[0]).to.have.property('production', false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/push/server/apn.spec.ts` around lines 55 - 130, Add a unit test verifying that a top-level options.production value overrides any production flag inside options.apn: call initAPN with options from buildOptions({ production: false }, { production: true }) and assert that mocks.ApnProvider.firstCall.args[0] has property 'production' equal to false; place the test alongside the other 'APN provider initialization' specs to ensure the spread logic in initAPN uses the trailing production: options.production override.
50-53:beforeEachreset can be simplified tosandbox.reset()
sandbox.resetHistory()already resetsmocks.ApnProvider's call history as part of the sandbox. The subsequentmocks.ApnProvider.reset()re-resets its history (redundant) and additionally clears thethrowsbehavior from the last test. Usingsandbox.reset()alone covers both history and behavior for every stub in the sandbox in one call.♻️ Proposed simplification
beforeEach(() => { - sandbox.resetHistory(); - mocks.ApnProvider.reset(); + sandbox.reset(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/push/server/apn.spec.ts` around lines 50 - 53, Replace the two-line test setup in the beforeEach block that calls sandbox.resetHistory() followed by mocks.ApnProvider.reset() with a single sandbox.reset() call: update the beforeEach to call sandbox.reset() (which resets histories and stub behaviors) and remove the direct mocks.ApnProvider.reset() invocation to avoid redundant resets and accidental clearing of throws behavior.
17-28: Redundant@noCallThruwithin the mock object
proxyquire.noCallThru()is already applied globally at the load call (line 23), so'@noCallThru': trueinsideapnMock(line 20) and its spread into the@parse/node-apnmock entry has no additional effect.🧹 Proposed cleanup
const apnMock = { 'Provider': mocks.ApnProvider, 'Notification': sandbox.stub(), - '@noCallThru': true, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/push/server/apn.spec.ts` around lines 17 - 28, Remove the redundant '@noCallThru' property from the apnMock object and stop spreading apnMock into the '@parse/node-apn' mock entry; proxyquire.noCallThru().load is already applied when loading './apn', so delete the '@noCallThru': true key from apnMock and replace the '@parse/node-apn' mock with an explicit mapping that only provides default: apnMock (or otherwise only the intended keys like 'Provider' and 'Notification') to avoid duplicating the noCallThru flag while keeping apnMock and the initAPN test setup intact.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/tame-humans-greet.mdapps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.tsapps/meteor/app/push/server/apn.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
apps/meteor/app/push/server/apn.spec.tsapps/meteor/app/push/server/apn.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/app/push/server/apn.spec.ts
🧠 Learnings (16)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use `.spec.ts` extension for test files (e.g., `login.spec.ts`)
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/app/push/server/apn.spec.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
.changeset/tame-humans-greet.md
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
.changeset/tame-humans-greet.md
🧬 Code graph analysis (1)
apps/meteor/app/push/server/apn.spec.ts (1)
apps/meteor/app/push/server/apn.ts (1)
initAPN(92-147)
🔇 Additional comments (3)
apps/meteor/.mocharc.js (1)
33-33: LGTM!Correct glob pattern, consistent with adjacent entries.
.changeset/tame-humans-greet.md (1)
1-5: LGTM!Changeset is accurate and correctly scoped as a
patch.apps/meteor/app/push/server/apn.ts (1)
139-146: Fix is correct —productionoption is well-supported and properly overrides any value inoptions.apnThe
@parse/node-apnProviderconstructor acceptsproduction: Booleanto specify which environment to connect to, defaulting toNODE_ENV === "production"if not provided. Placingproduction: options.productionafter...options.apnin the spread ensures it always wins regardless of whatoptions.apncontains — which is exactly the scenario described in CORE-1771.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/push/server/apn.spec.ts`:
- Around line 55-130: Add a unit test verifying that a top-level
options.production value overrides any production flag inside options.apn: call
initAPN with options from buildOptions({ production: false }, { production: true
}) and assert that mocks.ApnProvider.firstCall.args[0] has property 'production'
equal to false; place the test alongside the other 'APN provider initialization'
specs to ensure the spread logic in initAPN uses the trailing production:
options.production override.
- Around line 50-53: Replace the two-line test setup in the beforeEach block
that calls sandbox.resetHistory() followed by mocks.ApnProvider.reset() with a
single sandbox.reset() call: update the beforeEach to call sandbox.reset()
(which resets histories and stub behaviors) and remove the direct
mocks.ApnProvider.reset() invocation to avoid redundant resets and accidental
clearing of throws behavior.
- Around line 17-28: Remove the redundant '@noCallThru' property from the
apnMock object and stop spreading apnMock into the '@parse/node-apn' mock entry;
proxyquire.noCallThru().load is already applied when loading './apn', so delete
the '@noCallThru': true key from apnMock and replace the '@parse/node-apn' mock
with an explicit mapping that only provides default: apnMock (or otherwise only
the intended keys like 'Provider' and 'Notification') to avoid duplicating the
noCallThru flag while keeping apnMock and the initAPN test setup intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1771
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests