[Backport of #25403] fix(data-consumption): prevent hook usage counter from going negative - #25503
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe plugin engine now centralizes hook usage count updates. It logs a warning and clamps the result to zero when a delta would produce a negative count. Both argument-aware and general hook utilization paths use this helper. The HTML5 plugin SDK version is updated to Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change prevents an invalid negative usage count and logs a warning when an invalid update is attempted; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.ts (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the shared underflow contract.
Cover both update paths with
count = 0anddelta = -1; assert the count remains0and the warning includes the hook name, current count, and delta. Also cover normal positive and negative updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.ts` around lines 13 - 25, Add focused tests for applyHookUsageCountDelta covering underflow from count 0 with delta -1, asserting a returned count of 0 and a warning containing hookName, currentCount, and delta. Also test normal positive and negative deltas, preserving the expected arithmetic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bigbluebutton-html5/package.json`:
- Line 82: Replace the mutable bigbluebutton-html-plugin-sdk PR URL in
package.json with an immutable committed SHA or published release version, then
regenerate the lockfile so it records the pinned dependency and resolved
tarball.
---
Nitpick comments:
In
`@bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.ts`:
- Around line 13-25: Add focused tests for applyHookUsageCountDelta covering
underflow from count 0 with delta -1, asserting a returned count of 0 and a
warning containing hookName, currentCount, and delta. Also test normal positive
and negative deltas, preserving the expected arithmetic behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c6bee2f-ac4e-488a-a6e0-64652a1affb5
⛔ Files ignored due to path filters (1)
bigbluebutton-html5/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.tsbigbluebutton-html5/package.json
|
This pull request has conflicts ☹ |
18253f3 to
35de132
Compare
The usage counter that tracks how many plugins are listening to a given subscription can reach negative values, which is invalid and likely indicates a bug in the unsubscribe logic. Extract the hook usage update logic into a dedicated function that guards against negative values and logs a warning when such an update is attempted.
35de132 to
e3d0f1e
Compare
✅ Automated tests have passed! |
What does this PR do?
The usage counter that tracks how many plugins are listening to a given subscription can reach negative values, which is invalid and likely indicates a bug in the unsubscribe logic.
Extract the hook usage update logic into a dedicated function that guards against negative values and logs a warning when such an update is attempted.
Closes Issue(s)
Closes N/A
More
Closely related to:
useCustomSubscriptionstuck inloading: truestate bigbluebutton-html-plugin-sdk#275