feat(HikVision): add remove plugin function #797
Merged
Conversation
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds cleanup logic for the HikVision JS video plugin by explicitly destroying the plugin instance, clearing the global reference, and removing the plugin script tag from the document head when the plugin is destroyed. Sequence diagram for destroying and removing the HikVision JSVideoPluginsequenceDiagram
participant Caller
participant hackJSDestroyPlugin
participant JSVideoPluginInstance as oPlugin
participant Window
participant DocumentHead as document_head
Caller->>hackJSDestroyPlugin: invoke()
hackJSDestroyPlugin->>JSVideoPluginInstance: JS_DestroyPlugin(true)
JSVideoPluginInstance-->>hackJSDestroyPlugin: plugin_destroyed
hackJSDestroyPlugin->>Window: set JSVideoPlugin = null
hackJSDestroyPlugin->>Window: delete JSVideoPlugin
hackJSDestroyPlugin->>DocumentHead: removePlugin()
DocumentHead->>DocumentHead: find script jsVideoPlugin_1_0_0
DocumentHead->>DocumentHead: remove script node(s)
DocumentHead-->>hackJSDestroyPlugin: scripts_removed
hackJSDestroyPlugin-->>Caller: destroy_completed
Flow diagram for removePlugin cleanup logicflowchart TD
A["Call removePlugin"] --> B["Query all script elements in document.head"]
B --> C["Filter scripts by src containing /jsVideoPlugin-1.0.0.min.js"]
C --> D{"Any matching script nodes?"}
D -->|Yes| E["Iterate over matching nodes by index"]
E --> F["Remove current script node from document.head"]
F --> G{"More nodes to remove?"}
G -->|Yes| E
G -->|No| H["Cleanup finished"]
D -->|No| H
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 hard-coded
'/jsVideoPlugin-1.0.0.min.js'substring inremovePluginis brittle; consider matching against a configurable constant or a more flexible pattern (e.g.,includes('jsVideoPlugin')) so future version changes don’t silently break the cleanup. - In
removePlugin, you can simplify and slightly optimize by using a single loop overdocument.head.querySelectorAll('script[src]')and removing matching nodes in place instead of first creating an intermediate array and filtering.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded `'/jsVideoPlugin-1.0.0.min.js'` substring in `removePlugin` is brittle; consider matching against a configurable constant or a more flexible pattern (e.g., `includes('jsVideoPlugin')`) so future version changes don’t silently break the cleanup.
- In `removePlugin`, you can simplify and slightly optimize by using a single loop over `document.head.querySelectorAll('script[src]')` and removing matching nodes in place instead of first creating an intermediate array and filtering.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds a plugin cleanup function to the HikVision component to properly remove plugin resources when destroying the video player instance. The changes address issue #796 by implementing proper cleanup of the JSVideoPlugin global object and removing the dynamically loaded script from the DOM. The version is also updated from beta to stable release (10.0.0).
Key Changes
- Added
removePlugin()function to remove thejsVideoPlugin-1.0.0.min.jsscript from the DOM - Enhanced
hackJSDestroyPlugin()to nullify the globalJSVideoPluginobject and call the new cleanup function - Updated package version from
10.0.0-beta06to10.0.0(stable release)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js |
Implements plugin cleanup logic to remove script tags and nullify global objects when destroying the video plugin |
src/components/BootstrapBlazor.HikVision/BootstrapBlazor.HikVision.csproj |
Updates package version to stable release 10.0.0 |
Comments suppressed due to low confidence (3)
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:1
- Unused import registerBootstrapBlazorModule.
import { addScript, registerBootstrapBlazorModule } from '../BootstrapBlazor/modules/utility.js';
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:39
- Unused variable originalDestroy.
const originalDestroy = JSVideoPlugin.prototype.JS_DestroyPlugin;
src/components/BootstrapBlazor.HikVision/wwwroot/hikvision.js:148
- Unused variable logined.
const { szDeviceIdentify, logined } = vision;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return this.oPlugin.JS_DestroyPlugin(true); | ||
| this.oPlugin.JS_DestroyPlugin(true); | ||
|
|
||
| JSVideoPlugin = null; |
There was a problem hiding this comment.
Attempting to set JSVideoPlugin = null in local scope has no effect on the global JSVideoPlugin. Since JSVideoPlugin is a global constructor, you should use window.JSVideoPlugin = null instead for proper cleanup.
Suggested change
| JSVideoPlugin = null; | |
| window.JSVideoPlugin = null; |
| const removePlugin = () => { | ||
| const scripts = [...document.head.querySelectorAll('script')] | ||
| const nodes = scripts.filter(function (link) { | ||
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 |
There was a problem hiding this comment.
The hardcoded filename '/jsVideoPlugin-1.0.0.min.js' makes this function fragile. If the plugin version changes, this cleanup won't work. Consider extracting this as a constant or making it more flexible (e.g., using a pattern like /jsVideoPlugin.*\.js).
Suggested change
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 | |
| return /\/jsVideoPlugin.*\.js$/.test(link.src); |
Comment on lines
+57
to
+58
| const nodes = scripts.filter(function (link) { | ||
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 |
There was a problem hiding this comment.
The variable name link in the filter callback is misleading since the elements being filtered are <script> elements, not <link> elements. Consider renaming to script for clarity.
Suggested change
| const nodes = scripts.filter(function (link) { | |
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 | |
| const nodes = scripts.filter(function (script) { | |
| return script.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 |
Comment on lines
+56
to
+59
| const scripts = [...document.head.querySelectorAll('script')] | ||
| const nodes = scripts.filter(function (link) { | ||
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 | ||
| }) |
There was a problem hiding this comment.
Missing semicolons after statements on lines 56 and 57. Consider adding semicolons for consistency with the rest of the codebase (lines 47, 49, 50, 51, etc.).
Suggested change
| const scripts = [...document.head.querySelectorAll('script')] | |
| const nodes = scripts.filter(function (link) { | |
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 | |
| }) | |
| const scripts = [...document.head.querySelectorAll('script')]; | |
| const nodes = scripts.filter(function (link) { | |
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 | |
| }); |
| const scripts = [...document.head.querySelectorAll('script')] | ||
| const nodes = scripts.filter(function (link) { | ||
| return link.src.indexOf('/jsVideoPlugin-1.0.0.min.js') > -1 | ||
| }) |
There was a problem hiding this comment.
Missing semicolon after the closing brace. Consider adding a semicolon for consistency with the rest of the codebase.
Suggested change
| }) | |
| }); |
Comment on lines
+60
to
+61
| for (let index = 0; index < nodes.length; index++) { | ||
| document.head.removeChild(nodes[index]) |
There was a problem hiding this comment.
A standard for-of loop would be more idiomatic and readable here: for (const node of nodes) { document.head.removeChild(node) }.
Suggested change
| for (let index = 0; index < nodes.length; index++) { | |
| document.head.removeChild(nodes[index]) | |
| for (const node of nodes) { | |
| document.head.removeChild(node); |
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 #796
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enhancements: