fix: Stopped calling usage pulse in air-gapped instance #38749
Conversation
WalkthroughThe pull request introduces a modification to the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/usagePulse/index.ts (1)
147-152: Consider adding logging for skipped pulses.The airgapped check implementation is correct, but consider these improvements:
- Rename
_isAirgappedtoisAirgappedInstancefor clarity- Add debug logging when skipping pulse in airgapped mode
- const _isAirgapped = isAirgapped(); + const isAirgappedInstance = isAirgapped(); + + if (isAirgappedInstance) { + log.debug("Skipping usage pulse for air-gapped instance"); + return; + } - if (_isAirgapped) { - return; - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/usagePulse/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/src/usagePulse/index.ts (1)
21-21: LGTM! Clean import addition.
app/client/src/usagePulse/index.ts
Outdated
| const _isAirgapped = isAirgapped(); | ||
|
|
||
| if (_isAirgapped) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for air-gapped usage pulse prevention
The air-gapped check implementation is correct, but test coverage is missing for this scenario. Add test cases to verify usage pulse behavior in air-gapped mode.
- Add test in
app/client/src/usagePulse/usagePulse.test.tsto verify that pulse is not sent when system is air-gapped
🔗 Analysis chain
Verify complete coverage of usage pulse prevention.
Let's verify there aren't other places where usage pulse might be triggered in air-gapped instances.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential usage pulse triggers
rg -l "sendPulse|UsagePulse" --type ts
# Search for test coverage of air-gapped scenarios
rg -l "isAirgapped.*test" --type ts
Length of output: 445
Script:
#!/bin/bash
# Check saga implementations
rg "sendPulse|UsagePulse" app/client/src/ce/sagas/userSagas.tsx app/client/src/ce/sagas/NavigationSagas.ts -C 2
# Review test coverage
cat app/client/src/usagePulse/usagePulse.test.ts
# Check air-gapped implementation
cat app/client/src/api/interceptors/request/blockAirgappedRoutes.ts
Length of output: 4876
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/usagePulse/index.ts (1)
21-21: Consider dynamic air-gapped state checkThe static initialization of
isAirgappedwon't reflect runtime changes to the air-gapped state.import { isAirgapped } from "ee/utils/airgapHelpers"; class UsagePulse { - static isAirgapped = isAirgapped(); + static get isAirgapped() { + return isAirgapped(); + }Also applies to: 30-30
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/usagePulse/index.ts(3 hunks)app/client/src/usagePulse/usagePulse.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
app/client/src/usagePulse/index.ts (1)
148-151: LGTM! Clean implementation of air-gapped checkThe early return when air-gapped effectively prevents both pulse sending and listener scheduling.
| describe("sendPulseAndScheduleNext", () => { | ||
| it("should not send pulse when airgapped", () => { | ||
| //set isAirgapped to true | ||
| UsagePulse.isAirgapped = true; | ||
| UsagePulse.sendPulseAndScheduleNext(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for air-gapped scenario
The test case needs to verify that the pulse is not sent and clean up the test state.
describe("sendPulseAndScheduleNext", () => {
it("should not send pulse when airgapped", () => {
- //set isAirgapped to true
+ const sendPulseSpy = jest.spyOn(UsagePulse, 'sendPulse');
UsagePulse.isAirgapped = true;
+
UsagePulse.sendPulseAndScheduleNext();
+
+ expect(sendPulseSpy).not.toHaveBeenCalled();
+ sendPulseSpy.mockRestore();
+ });
+
+ afterEach(() => {
+ UsagePulse.isAirgapped = false;
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("sendPulseAndScheduleNext", () => { | |
| it("should not send pulse when airgapped", () => { | |
| //set isAirgapped to true | |
| UsagePulse.isAirgapped = true; | |
| UsagePulse.sendPulseAndScheduleNext(); | |
| }); | |
| }); | |
| describe("sendPulseAndScheduleNext", () => { | |
| it("should not send pulse when airgapped", () => { | |
| const sendPulseSpy = jest.spyOn(UsagePulse, 'sendPulse'); | |
| UsagePulse.isAirgapped = true; | |
| UsagePulse.sendPulseAndScheduleNext(); | |
| expect(sendPulseSpy).not.toHaveBeenCalled(); | |
| sendPulseSpy.mockRestore(); | |
| }); | |
| afterEach(() => { | |
| UsagePulse.isAirgapped = false; | |
| }); | |
| }); |
Description
This PR added fix for not triggering usage pulse for air gapped instances
Fixes https://github.com/appsmithorg/cloud-services/issues/1883
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Important
🟣 🟣 🟣 Your tests are running.
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12861462235
Commit: 1f76f11
Workflow:
PR Automation test suiteTags:
@tag.SanitySpec: ``
Mon, 20 Jan 2025 05:15:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit