Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR refactors status management by replacing the monolithic Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/status-manager/disconnect-test.ts (1)
19-25: RepeatedupdateStatus(true)calls after threshold is reached.Once
MAX_DISCONNECTS_COUNT(3) is reached, every subsequentreportDisconnect()call will triggerupdateStatus('too-many-disconnects', true)again sincesizewill remain>= 3. If theStatusManager.updateStatusdeduplicates these calls, this is fine. Otherwise, consider tracking whether the threshold has already been reported to avoid redundant updates.♻️ Optional: Track threshold state to avoid redundant calls
export class DisconnectTest { + private thresholdReached = false; private readonly disconnects = new TTLCache<string, number>({ ttl: DISCONNECTS_TTL, dispose: () => { if (this.disconnects.size === 0) { + this.thresholdReached = false; this.updateStatus('too-many-disconnects', false); } }, }); constructor (private readonly updateStatus: (status: 'too-many-disconnects', value: boolean) => void) {} public reportDisconnect () { this.disconnects.set(randomUUID(), Date.now()); - if (this.disconnects.size >= MAX_DISCONNECTS_COUNT) { + if (this.disconnects.size >= MAX_DISCONNECTS_COUNT && !this.thresholdReached) { + this.thresholdReached = true; this.updateStatus('too-many-disconnects', true); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/status-manager/disconnect-test.ts` around lines 19 - 25, reportDisconnect currently calls updateStatus('too-many-disconnects', true) on every call once disconnects.size >= MAX_DISCONNECTS_COUNT; to avoid redundant updates add a guard that only triggers when the threshold is first crossed (e.g. only when disconnects.size === MAX_DISCONNECTS_COUNT) or maintain a boolean like tooManyDisconnectsReported on the StatusManager class; set it true when you call updateStatus in reportDisconnect and reset it to false wherever disconnects entries are pruned (or when size falls below MAX_DISCONNECTS_COUNT) so subsequent crossings will re-report correctly; update references: reportDisconnect, disconnects, updateStatus, MAX_DISCONNECTS_COUNT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/probe.ts`:
- Around line 74-76: The startup log in connect() (logger.info referencing
VERSION, process.env['NODE_ENV'], and probeUuid) runs per throng worker; move
that logger.info call out of the connect() function so it runs once at module
startup, or add a guard inside connect() to only log when workerId is the
primary worker (e.g., when workerId === 0 or workerId is undefined) so the
message emits only once during initialization.
In `@src/status-manager/icmp-tcp-test.ts`:
- Around line 26-29: The readiness path currently blocks on the socket
'api:connect:isProxy' handler (where you set this.isProxy and call
evaluateAndUpdate()), causing evaluateAndUpdate() to never set
"icmp-tcp-test-failed" if that event is delayed; update the logic in the
socket.on('api:connect:isProxy', ...) area and the analogous handler around the
other occurrence so that isProxy has a safe default (e.g., false/unknown) and/or
add a short timeout that assigns the fallback and calls evaluateAndUpdate() if
the event hasn't arrived; ensure evaluateAndUpdate(), the isProxy property, and
any StatusManager.getStatus() callers use that fallback so readiness won't be
gated indefinitely and adjust tests that assumed the event never triggers.
- Around line 91-129: The current VPN decision (isVpnDetected/isVpn) treats IPv4
and IPv6 separately and lets a single over-100 RTT-diff block a probe; update
logic to aggregate signals across protocols and require a minimum of two
“points” before declaring VPN: combine diffs from diffsIPv4 and diffsIPv6 into
one numeric array inside isVpnDetected (instead of calling isVpn twice), include
traceroute/public-hop signals (the traceroute/public-hop result(s) you added
elsewhere as extra point(s)) when computing the score, and change isVpn so that
an over-100 diff contributes a single point but does not trigger VPN unless
total points >= 2 (and similarly require two over-60s across the combined data
or one over-60 plus a traceroute/public-hop point). Keep measureDiff as-is but
ensure its nulls are treated as non-points when aggregating.
In `@test/unit/status-manager/disconnect-test.test.ts`:
- Around line 47-50: The test's assertions for updateStatus are wrong: change
the expected call count from 4 to 2 and adjust the inspected argument index from
args[3] to args[1]; specifically update the assertions around
sandbox.clock.tickAsync(...) to expect updateStatus.callCount === 2 and
expect(updateStatus.args[1]).to.deep.equal(['too-many-disconnects', false]);
this targets the updateStatus spy used in the disconnect test so the assertion
matches the implementation where a single true and a single false call are made
after TTL expiry.
---
Nitpick comments:
In `@src/status-manager/disconnect-test.ts`:
- Around line 19-25: reportDisconnect currently calls
updateStatus('too-many-disconnects', true) on every call once disconnects.size
>= MAX_DISCONNECTS_COUNT; to avoid redundant updates add a guard that only
triggers when the threshold is first crossed (e.g. only when disconnects.size
=== MAX_DISCONNECTS_COUNT) or maintain a boolean like tooManyDisconnectsReported
on the StatusManager class; set it true when you call updateStatus in
reportDisconnect and reset it to false wherever disconnects entries are pruned
(or when size falls below MAX_DISCONNECTS_COUNT) so subsequent crossings will
re-report correctly; update references: reportDisconnect, disconnects,
updateStatus, MAX_DISCONNECTS_COUNT.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2d4b8e2-6813-4873-9e4e-f08ecc832587
📒 Files selected for processing (15)
.gitignoreconfig/default.cjssrc/helper/api-error-handler.tssrc/lib/status-manager.tssrc/probe.tssrc/status-manager/disconnect-test.tssrc/status-manager/icmp-tcp-test.tssrc/status-manager/ping-test.tssrc/status-manager/status-manager.tstest/unit/lib/status-manager.test.tstest/unit/probe.test.tstest/unit/status-manager/disconnect-test.test.tstest/unit/status-manager/icmp-tcp-test.test.tstest/unit/status-manager/ping-test.test.tstest/unit/status-manager/status-manager.test.ts
💤 Files with no reviewable changes (2)
- src/lib/status-manager.ts
- test/unit/lib/status-manager.test.ts
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
1 participant
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.
Fixes jsdelivr/globalping#766