Skip to content

fix: address review comments on offline license no-egress - #41419

Merged
KevLehman merged 1 commit into
feat/offline-license-no-egressfrom
feat/offline-license-no-egress-review
Jul 16, 2026
Merged

fix: address review comments on offline license no-egress#41419
KevLehman merged 1 commit into
feat/offline-license-no-egressfrom
feat/offline-license-no-egress-review

Conversation

@KevLehman

@KevLehman KevLehman commented Jul 16, 2026

Copy link
Copy Markdown
Member

Proposed changes (including videos or screenshots)

Addresses review comments on #41148:

  • r3588734534: drop the hasOfflineLicense() wrapper — callers now use License.hasOfflineLicense() directly. assertNotOfflineLicense() stays since it carries real behavior (throws for interactive cloud flows). Unit test mocks updated to stub @rocket.chat/license instead of the local module.
  • r3588777995: removed the comment block in server/main.ts per suggestion.
  • r3588657914: simplified the offline-license startup log — License.onInstall fires once per license application (and immediately when a license is already applied), so the once-per-activation flag closure and the extra validate/invalidate/remove registrations are gone.

Issue(s)

Review follow-up for #41148

Steps to test or reproduce

Apply an offline license and confirm the startup log line appears once; cloud/push egress suppression is unchanged. userLogout and registerPreIntentWorkspaceWizard unit tests pass.

Further comments

No behavior change except the startup log now triggers on license install instead of every validation cycle (less noise, same signal).

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved offline-license handling across cloud synchronization, workspace registration, token access, logout, and user avatar operations.
    • Offline deployments now consistently prevent outbound cloud requests while preserving local functionality.
    • Improved offline-license startup detection and logging.
  • Tests

    • Updated automated coverage to reflect the centralized offline-license behavior.

@dionisio-bot

dionisio-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot

changeset-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 6a2ebe9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dd47fb22-9283-4b88-b49d-ac629b8cb839

📥 Commits

Reviewing files that changed from the base of the PR and between b87c238 and 6a2ebe9.

📒 Files selected for processing (12)
  • apps/meteor/ee/server/lib/license/startup.ts
  • apps/meteor/server/lib/cloud/getWorkspaceAccessToken.ts
  • apps/meteor/server/lib/cloud/getWorkspaceAccessTokenWithScope.ts
  • apps/meteor/server/lib/cloud/offlineLicense.ts
  • apps/meteor/server/lib/cloud/registerPreIntentWorkspaceWizard.ts
  • apps/meteor/server/lib/cloud/syncWorkspace/index.ts
  • apps/meteor/server/lib/cloud/userLogout.ts
  • apps/meteor/server/lib/users/getAvatarSuggestionForUser.ts
  • apps/meteor/server/lib/users/saveUser/saveNewUser.ts
  • apps/meteor/server/main.ts
  • apps/meteor/tests/unit/server/lib/cloud/registerPreIntentWorkspaceWizard.spec.ts
  • apps/meteor/tests/unit/server/lib/cloud/userLogout.spec.ts

Walkthrough

Offline-license detection is centralized on License.hasOfflineLicense(). Startup logging moves to the license installation hook, Cloud and avatar flows update their guards, the local helper is removed, and related test stubs are adjusted.

Changes

Offline license consolidation

Layer / File(s) Summary
License lifecycle and guard source
apps/meteor/ee/server/lib/license/startup.ts, apps/meteor/server/lib/cloud/offlineLicense.ts, apps/meteor/server/main.ts
Offline-license startup logging is registered through License.onInstall, while the local helper usage and obsolete boot-order comment are removed.
Cloud egress guard migration
apps/meteor/server/lib/cloud/*, apps/meteor/tests/unit/server/lib/cloud/*
Cloud token, registration, synchronization, and logout paths now call License.hasOfflineLicense(); proxyquire stubs are updated accordingly.
Avatar egress guard migration
apps/meteor/server/lib/users/getAvatarSuggestionForUser.ts, apps/meteor/server/lib/users/saveUser/saveNewUser.ts
Avatar suggestion and default-avatar fetch suppression now use License.hasOfflineLicense().

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: type: bug

Suggested reviewers: cardoso, sampaiodiego


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.36%. Comparing base (b87c238) to head (6a2ebe9).
⚠️ Report is 1 commits behind head on feat/offline-license-no-egress.

Additional details and impacted files

Impacted file tree graph

@@                        Coverage Diff                         @@
##           feat/offline-license-no-egress   #41419      +/-   ##
==================================================================
+ Coverage                           68.35%   68.36%   +0.01%     
==================================================================
  Files                                3970     3970              
  Lines                              154925   154925              
  Branches                            27843    27832      -11     
==================================================================
+ Hits                               105904   105922      +18     
+ Misses                              44271    44253      -18     
  Partials                             4750     4750              
Flag Coverage Δ
unit 70.27% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KevLehman
KevLehman marked this pull request as ready for review July 16, 2026 14:57
@KevLehman
KevLehman requested a review from a team as a code owner July 16, 2026 14:57
@KevLehman
KevLehman merged commit 4591a02 into feat/offline-license-no-egress Jul 16, 2026
25 checks passed
@KevLehman
KevLehman deleted the feat/offline-license-no-egress-review branch July 16, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant