Conversation
Reviewer's GuideAdds defensive hacks around the HikVision JSVideoPlugin to avoid runtime exceptions during resize and destroy operations, and ensures the plugin is cleanly stopped before destruction. Sequence diagram for hacked JS_DestroyPlugin shutdown flowsequenceDiagram
participant Caller
participant JSVideoPlugin
participant oPlugin as Plugin
participant oRequest as Request
participant oWebSocket as WebSocket
Caller->>JSVideoPlugin: JS_DestroyPlugin(n)
activate JSVideoPlugin
JSVideoPlugin->>oPlugin: access oRequest
oPlugin->>oRequest: override sendRequest(r)
activate oRequest
Note over oRequest,oWebSocket: New sendRequest checks WebSocket.readyState
oRequest->>oWebSocket: check readyState
alt WebSocket is open
oWebSocket-->>oRequest: OPEN
oRequest->>oRequest: origianlSendRequestProxy(r)
else WebSocket not open
oWebSocket-->>oRequest: not OPEN
oRequest-->>oRequest: do not send request
end
deactivate oRequest
JSVideoPlugin->>oPlugin: JS_DestroyPlugin(true)
deactivate JSVideoPlugin
Sequence diagram for initWindow initialization and plugin hackssequenceDiagram
participant UI as CallerInit
participant Module as hikvisionModule
participant WebVideoCtrl
UI->>Module: init(id)
activate Module
Module->>Module: initWindow(id)
activate WebVideoCtrl
WebVideoCtrl->>WebVideoCtrl: I_InitPlugin(options)
WebVideoCtrl-->>Module: callback(result.inited, result.iWndIndex)
deactivate WebVideoCtrl
Module->>Module: poll with setInterval
alt inited false or inited and iWndIndex != -1
Module->>Module: clearInterval(handler)
Module->>Module: hackJSResize()
Module->>Module: hackJSDestroyPlugin()
Module-->>UI: resolve(result)
end
deactivate Module
Sequence diagram for dispose with explicit stop before destroysequenceDiagram
participant UI as CallerDispose
participant Module as hikvisionModule
participant WebVideoCtrl
UI->>Module: dispose(id)
activate Module
alt logined is true
Module->>Module: logout(id)
end
Module->>WebVideoCtrl: I_Stop()
Module->>WebVideoCtrl: I_DestroyPlugin()
deactivate Module
Class diagram for JSVideoPlugin hacks and related objectsclassDiagram
class JSVideoPlugin {
oOptions
oPlugin
JS_Resize(e, t)
JS_DestroyPlugin(n)
}
class Plugin {
oRequest
JS_DestroyPlugin(force)
}
class RequestRoot {
oRequest
}
class RequestInner {
oWebSocket
sendRequest(r)
}
class WebSocket {
readyState
}
JSVideoPlugin *-- Plugin : oPlugin
Plugin *-- RequestRoot : oRequest
RequestRoot *-- RequestInner : oRequest
RequestInner *-- WebSocket : oWebSocket
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The monkey-patch helpers
hackJSResizeandhackJSDestroyPluginassumeJSVideoPluginis defined; consider guarding them (e.g.,if (!window.JSVideoPlugin) return;) to avoid new runtime errors when the plugin script fails to load or changes its global name. - In
hackJSDestroyPlugin, the override ignores the originalnargument and callsthis.oPlugin.JS_DestroyPlugin(true)directly; if the original method relied on the argument, this may change behavior—consider delegating tooriginalDestroy.call(this, n)after wrappingsendRequestinstead of hard-codingtrue. - The new override of
sendRequestinsidehackJSDestroyPluginis never restored and also contains a typo inorigianlSendRequestProxy; it would be safer to cache and restore the original function (and fix the spelling) to avoid permanently altering the request behavior beyond the destroy lifecycle.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The monkey-patch helpers `hackJSResize` and `hackJSDestroyPlugin` assume `JSVideoPlugin` is defined; consider guarding them (e.g., `if (!window.JSVideoPlugin) return;`) to avoid new runtime errors when the plugin script fails to load or changes its global name.
- In `hackJSDestroyPlugin`, the override ignores the original `n` argument and calls `this.oPlugin.JS_DestroyPlugin(true)` directly; if the original method relied on the argument, this may change behavior—consider delegating to `originalDestroy.call(this, n)` after wrapping `sendRequest` instead of hard-coding `true`.
- The new override of `sendRequest` inside `hackJSDestroyPlugin` is never restored and also contains a typo in `origianlSendRequestProxy`; it would be safer to cache and restore the original function (and fix the spelling) to avoid permanently altering the request behavior beyond the destroy lifecycle.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:38-47` </location>
<code_context>
+ }
+}
+
+const hackJSDestroyPlugin = function () {
+ const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin;
+ JSVideoPlugin.prototype.JS_DestroyPlugin = function (n) {
+ const origianlSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest;
+ this.oPlugin.oRequest.oRequest.sendRequest = function (r) {
+ if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) {
+ return origianlSendRequestProxy.call(this, r);
+ }
+ }
+ return this.oPlugin.JS_DestroyPlugin(true);
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Overriding `JS_DestroyPlugin` bypasses the original implementation and ignores its arguments.
This override replaces `JSVideoPlugin.prototype.JS_DestroyPlugin` without ever calling `originalDestroy`, and it always invokes `this.oPlugin.JS_DestroyPlugin(true)` regardless of `n`. This changes the method’s contract and may break callers or future upstream changes that depend on the argument. Please preserve the original behavior by delegating to `originalDestroy` with the original arguments (potentially after wrapping `sendRequest`).
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:79-80` </location>
<code_context>
if (result.inited === false || (result.inited && result.iWndIndex !== -1)) {
- clearInterval(handler)
+ clearInterval(handler);
+ hackJSResize();
+ hackJSDestroyPlugin();
resolve(result);
}
</code_context>
<issue_to_address>
**suggestion:** Prototype hacks are applied every time `initWindow` resolves, which can repeatedly reassign the same methods.
`hackJSResize` and `hackJSDestroyPlugin` patch `JSVideoPlugin.prototype`, and `initWindow` can run multiple times, causing the same prototype methods to be reassigned on each successful init. This adds unnecessary repetition and makes it harder to see when the patch is applied. Consider a module-level guard (e.g. `let jsPluginPatched = false`) so the prototype is patched only once.
Suggested implementation:
```javascript
}
let jsPluginPatched = false;
const initWindow = id => {
```
```javascript
if (result.inited === false || (result.inited && result.iWndIndex !== -1)) {
clearInterval(handler);
if (!jsPluginPatched) {
hackJSResize();
hackJSDestroyPlugin();
jsPluginPatched = true;
}
resolve(result);
}
```
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:28` </location>
<code_context>
return true;
}
+const hackJSResize = function () {
+ const originalResize = JSVideoPlugin.prototype.JS_Resize;
+ JSVideoPlugin.prototype.JS_Resize = function (e, t) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating the plugin monkey-patching into a single idempotent function, while cleaning up naming and unused imports to keep the code simpler and clearer.
You can reduce the added complexity by centralizing and making the plugin patching idempotent, and by simplifying the destroy override.
### 1. Consolidate monkey‑patching into a single, idempotent function
Instead of `hackJSResize` and `hackJSDestroyPlugin` being called on every init loop resolution, create a single `patchJSVideoPluginOnce` that:
- Patches both `JS_Resize` and `JS_DestroyPlugin`.
- Guards against multiple executions.
- Does the `sendRequest` guarding at patch time instead of per‑destroy call.
```js
let jsVideoPluginPatched = false;
function patchJSVideoPluginOnce() {
if (jsVideoPluginPatched || typeof JSVideoPlugin === 'undefined') return;
jsVideoPluginPatched = true;
const originalResize = JSVideoPlugin.prototype.JS_Resize;
JSVideoPlugin.prototype.JS_Resize = function (e, t) {
const { szId } = this.oOptions;
if (document.getElementById(szId)) {
return originalResize.call(this, e, t);
}
// silently ignore if element no longer exists (current behavior)
};
const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin;
JSVideoPlugin.prototype.JS_DestroyPlugin = function (n) {
const request = this.oPlugin?.oRequest?.oRequest;
if (request && !request._sendRequestPatched) {
const originalSendRequest = request.sendRequest;
request._sendRequestPatched = true;
request.sendRequest = function (r) {
if (this.oWebSocket && this.oWebSocket.readyState === WebSocket.OPEN) {
return originalSendRequest.call(this, r);
}
// ignore when WebSocket is not open (current behavior)
};
}
// preserve the new behavior of forcing `true`
return originalDestroy.call(this, true);
};
}
```
Then call it once when the plugin is ready (keeping the same lifecycle point), but without re‑patching on every init:
```js
return new Promise((resolve, reject) => {
const handler = setInterval(() => {
if (result.inited === false || (result.inited && result.iWndIndex !== -1)) {
clearInterval(handler);
patchJSVideoPluginOnce();
resolve(result);
}
}, 16);
});
```
This keeps all current behavior (resize guarded by DOM presence, destroy guarded by WebSocket state, `true` passed to destroy) but:
- Removes multiple patch functions and call sites.
- Ensures prototype mutations only happen once.
- Avoids reassigning `sendRequest` on every destroy.
### 2. Fix naming/typo to reduce cognitive load
While you’re touching this code, correct the variable typo when you move it:
```js
const originalSendRequest = request.sendRequest;
```
### 3. Remove unused import
If `registerBootstrapBlazorModule` is not used, drop it to avoid unnecessary noise:
```js
- import { addScript, registerBootstrapBlazorModule } from '../BootstrapBlazor/modules/utility.js';
+ import { addScript } from '../BootstrapBlazor/modules/utility.js';
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+38
to
+47
| const hackJSDestroyPlugin = function () { | ||
| const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin; | ||
| JSVideoPlugin.prototype.JS_DestroyPlugin = function (n) { | ||
| const origianlSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | ||
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | ||
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | ||
| return origianlSendRequestProxy.call(this, r); | ||
| } | ||
| } | ||
| return this.oPlugin.JS_DestroyPlugin(true); |
There was a problem hiding this comment.
issue (bug_risk): Overriding JS_DestroyPlugin bypasses the original implementation and ignores its arguments.
This override replaces JSVideoPlugin.prototype.JS_DestroyPlugin without ever calling originalDestroy, and it always invokes this.oPlugin.JS_DestroyPlugin(true) regardless of n. This changes the method’s contract and may break callers or future upstream changes that depend on the argument. Please preserve the original behavior by delegating to originalDestroy with the original arguments (potentially after wrapping sendRequest).
Comment on lines
+79
to
+80
| hackJSResize(); | ||
| hackJSDestroyPlugin(); |
There was a problem hiding this comment.
suggestion: Prototype hacks are applied every time initWindow resolves, which can repeatedly reassign the same methods.
hackJSResize and hackJSDestroyPlugin patch JSVideoPlugin.prototype, and initWindow can run multiple times, causing the same prototype methods to be reassigned on each successful init. This adds unnecessary repetition and makes it harder to see when the patch is applied. Consider a module-level guard (e.g. let jsPluginPatched = false) so the prototype is patched only once.
Suggested implementation:
}
let jsPluginPatched = false;
const initWindow = id => { if (result.inited === false || (result.inited && result.iWndIndex !== -1)) {
clearInterval(handler);
if (!jsPluginPatched) {
hackJSResize();
hackJSDestroyPlugin();
jsPluginPatched = true;
}
resolve(result);
}
There was a problem hiding this comment.
Pull request overview
This PR fixes exception handling issues in the HikVision video plugin integration by adding protective wrapper functions ("hacks") around the JSVideoPlugin methods. The changes prevent errors when the plugin attempts to resize or destroy elements that no longer exist in the DOM.
- Removes redundant manual loading of jsVideoPlugin-1.0.0.min.js (already loaded by webVideoCtrl.js)
- Adds defensive wrappers for
JS_ResizeandJS_DestroyPluginmethods to prevent exceptions - Adds
WebVideoCtrl.I_Stop()call in dispose function to ensure proper cleanup - Bumps version from 10.0.0-beta05 to 10.0.0
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js | Adds protective wrapper functions to prevent exceptions during resize and destroy operations, removes redundant script loading, and adds explicit stop call in dispose |
| src/components/BootstrapBlazor.HikVision/BootstrapBlazor.HikVision.csproj | Updates version from beta05 to stable 10.0.0 release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+79
to
+80
| hackJSResize(); | ||
| hackJSDestroyPlugin(); |
There was a problem hiding this comment.
The hack functions are called unconditionally, even when result.inited === false. Consider only applying these hacks when initialization succeeds to avoid potential errors when the plugin is not properly initialized.
Suggested change
| hackJSResize(); | |
| hackJSDestroyPlugin(); | |
| if (result.inited === true) { | |
| hackJSResize(); | |
| hackJSDestroyPlugin(); | |
| } |
Comment on lines
+28
to
+36
| const hackJSResize = function () { | ||
| const originalResize = JSVideoPlugin.prototype.JS_Resize; | ||
| JSVideoPlugin.prototype.JS_Resize = function (e, t) { | ||
| const { szId } = this.oOptions; | ||
| if (document.getElementById(szId)) { | ||
| return originalResize.call(this, e, t); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider checking if JSVideoPlugin exists before attempting to hack its prototype. If the script is not loaded, this could throw a ReferenceError.
Comment on lines
+38
to
+49
| const hackJSDestroyPlugin = function () { | ||
| const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin; | ||
| JSVideoPlugin.prototype.JS_DestroyPlugin = function (n) { | ||
| const origianlSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | ||
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | ||
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | ||
| return origianlSendRequestProxy.call(this, r); | ||
| } | ||
| } | ||
| return this.oPlugin.JS_DestroyPlugin(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider checking if JSVideoPlugin exists before attempting to hack its prototype. If the script is not loaded, this could throw a ReferenceError.
Comment on lines
+41
to
+44
| const origianlSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | ||
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | ||
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | ||
| return origianlSendRequestProxy.call(this, r); |
There was a problem hiding this comment.
Typo in variable name: origianlSendRequestProxy should be originalSendRequestProxy. The letter 'a' and 'l' are swapped.
Suggested change
| const origianlSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | |
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | |
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | |
| return origianlSendRequestProxy.call(this, r); | |
| const originalSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | |
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | |
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | |
| return originalSendRequestProxy.call(this, r); |
Comment on lines
+41
to
+44
| const origianlSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | ||
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | ||
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | ||
| return origianlSendRequestProxy.call(this, r); |
There was a problem hiding this comment.
Typo in variable name: origianlSendRequestProxy should be originalSendRequestProxy. The letter 'a' and 'l' are swapped (same typo as line 41).
Suggested change
| const origianlSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | |
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | |
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | |
| return origianlSendRequestProxy.call(this, r); | |
| const originalSendRequestProxy = this.oPlugin.oRequest.oRequest.sendRequest; | |
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | |
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | |
| return originalSendRequestProxy.call(this, r); |
| const { szId } = this.oOptions; | ||
| if (document.getElementById(szId)) { | ||
| return originalResize.call(this, e, t); | ||
| } |
There was a problem hiding this comment.
The hacked JS_Resize function doesn't return anything when the element is not found. This could lead to unexpected behavior for callers expecting a return value. Consider returning a default value or explicitly returning undefined.
Suggested change
| } | |
| } | |
| return undefined; |
| this.oPlugin.oRequest.oRequest.sendRequest = function (r) { | ||
| if (this.oWebSocket && WebSocket.OPEN === this.oWebSocket.readyState) { | ||
| return origianlSendRequestProxy.call(this, r); | ||
| } |
There was a problem hiding this comment.
The hacked sendRequest function doesn't return anything when the WebSocket is not open. This could lead to unexpected behavior for callers expecting a return value. Consider returning a default value or explicitly handling this case.
Suggested change
| } | |
| } | |
| // Explicitly return a default value when WebSocket is not open | |
| return null; |
| @@ -1,8 +1,7 @@ | |||
| import { addScript } from '../BootstrapBlazor/modules/utility.js'; | |||
| import { addScript, registerBootstrapBlazorModule } from '../BootstrapBlazor/modules/utility.js'; | |||
There was a problem hiding this comment.
Unused import registerBootstrapBlazorModule.
Suggested change
| import { addScript, registerBootstrapBlazorModule } from '../BootstrapBlazor/modules/utility.js'; | |
| import { addScript } from '../BootstrapBlazor/modules/utility.js'; |
| } | ||
|
|
||
| const hackJSDestroyPlugin = function () { | ||
| const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin; |
There was a problem hiding this comment.
Unused variable originalDestroy.
Suggested change
| const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin; |
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
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.
Link issues
fixes #794
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve HikVision video plugin lifecycle handling to avoid runtime exceptions during resize and destruction and ensure clean shutdown when disposing the control.
Bug Fixes:
Enhancements: