Conversation
Reason for change: debug Test Procedure: None Risks: Low Priority: P1 Signed-off-by: apatel599@cable.comcast.com
|
b'## Copyright scan failure |
|
b'## Copyright scan failure |
|
b'## Copyright scan failure |
|
b'## Copyright scan failure |
|
b'## Copyright scan failure |
|
Hi @Aniket0606 : Apologies for the delay. This awkward PR came in over a holiday weekend. BD scanning is held up because the PR is very large.
Thank you. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces a browser-based CSI/motion analysis GUI and vendors JSON tree viewing libraries, while wiring the Web GUI app into the motion/CSI pipeline and app instance registry.
Changes:
- Add new web UI (HTML/CSS) for CSI analysis + comparison views.
- Vendor JsonTree.js (TypeScript/SASS sources, build configs, docs, examples) and jsonTreeViewer assets under the web server data directory.
- Update motion-core CSI ingestion/processing code and register a new
wifi_app_inst_web_guiapp instance.
Reviewed changes
Copilot reviewed 65 out of 177 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| source/apps/web_gui/webserver/data/main.css | Adds UI styling for tabs, grids, and form controls in the CSI GUI. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/tsup.build.min.config.ts | Adds tsup config for producing minified CJS bundle. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/tsup.build.esm.config.ts | Adds tsup config for ESM build and type output. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/tsup.build.config.ts | Adds tsup config for non-minified CJS build. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/tsconfig.json | Adds TypeScript compiler configuration for JsonTree.js sources. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/jsontree.js.translations.html | Adds library example page for translations usage. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/jsontree.js.theme.html | Adds library example page for theme usage. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/jsontree.js.paging.html | Adds library example page for array paging. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/jsontree.js.no-paging.html | Adds example page for paging disabled. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/jsontree.js.no-colors.html | Adds example page for colors disabled. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/jsontree.js.bootstrap.html | Adds example page demonstrating Bootstrap integration. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/jsontree.js.basic.html | Adds basic usage example page. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/js/scripts.js | Adds example script data + bindings for the test pages. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/test/css/styles.css | Adds styling used by the JsonTree.js test pages. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/type.ts | Adds JsonTree.js public/internal type definitions. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/options/config.ts | Adds configuration option normalization/defaulting. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/options/binding.ts | Adds binding option normalization + instance initialization. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/dom/dom.ts | Adds DOM helper utilities for library rendering. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/str.ts | Adds string helpers (padding/capitalization/truncation). |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/size.ts | Adds size/length calculation helpers for displayed values. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/obj.ts | Adds object property enumeration/sorting helpers. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/is.ts | Adds type guards and string validators used across library. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/filename.ts | Adds filename/extension helpers for import logic. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/enum.ts | Adds constants/enums used across library. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/default.ts | Adds defaulting helpers + URL loading helper. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/datetime.ts | Adds date formatting helpers. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/convert.ts | Adds conversion/parsing helpers (json, csv, html, types). |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/data/arr.ts | Adds array index/padding/mutation helpers. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/constant.ts | Adds constants for DOM attribute names. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/area/trigger.ts | Adds trigger invocation helper. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/area/tooltip.ts | Adds tooltip rendering and event hookup logic. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/area/context-menu.ts | Adds context-menu rendering and event hookup logic. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/ts/api.ts | Adds TypeScript types for the public API surface. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/src/sass/_styles.scss | Adds SASS mixins and styling primitives for library CSS. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/package.json | Adds vendored library package metadata and build scripts. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/jsontree.js.nuspec | Adds NuGet packaging metadata for the vendored library. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/SERVE.sh | Adds helper script to serve the library locally for testing. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/SECURITY.md | Adds upstream library security policy document. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/README_NUGET.md | Adds upstream library README for NuGet distribution. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/README.md | Adds upstream library README. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/PUBLISH.sh | Adds helper script for publishing to npm. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/PACK.sh | Adds helper script for producing NuGet package. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/LICENSE.txt | Adds upstream library MIT license. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/CONTRIBUTING.md | Adds upstream contributing guide. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/CODE_OF_CONDUCT.md | Adds upstream code of conduct. |
| source/apps/web_gui/webserver/data/jsontree-js-4.7.1/BUILD_INSTRUCTIONS.md | Adds upstream build instructions. |
| source/apps/web_gui/webserver/data/jsonTreeViewer-master/libs/jsonTree/jsonTree.css | Adds JSONTreeViewer CSS (vendored). |
| source/apps/web_gui/webserver/data/jsonTreeViewer-master/jsonTreeViewer.js | Adds JSONTreeViewer page JS (vendored). |
| source/apps/web_gui/webserver/data/jsonTreeViewer-master/index.htm | Adds JSONTreeViewer HTML page (vendored). |
| source/apps/web_gui/webserver/data/jsonTreeViewer-master/README.md | Adds JSONTreeViewer README (vendored). |
| source/apps/web_gui/webserver/data/jsonTreeViewer-master/LICENSE | Adds JSONTreeViewer license. |
| source/apps/web_gui/webserver/data/jsonTreeViewer-master/CHANGELOG.md | Adds JSONTreeViewer changelog. |
| source/apps/web_gui/webserver/data/index.html | Adds CSI analysis/comparison web UI page and JS event wiring. |
| source/apps/web_gui/webserver/data/associated_clients.json | Adds example/seed data for associated clients list. |
| source/apps/web_gui/motion/src/sounder.cpp | Updates CSI parsing/variance logic and adds map helper entrypoints. |
| source/apps/web_gui/motion/src/motion_cpp_wrapper.c | Adds web GUI app glue, JSON persistence, and assoc/disassoc updates. |
| source/apps/web_gui/motion/src/motion_core.cpp | Adds motion-core CSI ingestion via bus + pipe loop + sounder dispatch. |
| source/apps/web_gui/motion/inc/sounder.h | Updates motion sounder API surface used by motion core. |
| source/apps/web_gui/motion/inc/csimgr.h | Adds motion-core bus/pipe state and methods to CSI manager. |
| source/apps/web_gui/common_web_gui.h | Adds shared web GUI types and C/C++ bridge declarations. |
| include/wifi_base.h | Adds wifi_app_inst_web_gui and shifts subsequent enum values. |
Files not reviewed (1)
- source/apps/web_gui/webserver/data/jsontree-js-4.7.1/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| result.object = eval( `(${objectString})` ); | ||
|
|
||
| if ( Is.definedFunction( result.object ) ) { | ||
| result.object = result.object(); | ||
| } | ||
|
|
||
| } catch ( exception2: any ) { | ||
| if ( !configuration.safeMode ) { | ||
| console.error( configuration.text!.objectErrorText!.replace( "{{error_1}}", exception1.message ).replace( "{{error_2}}", exception2.message ) ); | ||
| result.parsed = false; | ||
| } | ||
|
|
||
| result.object = null; | ||
| } |
There was a problem hiding this comment.
jsonStringToObject() uses eval() as a fallback parser. This is a code execution primitive if objectString is attacker-controlled (and it executes regardless of configuration.safeMode). Recommended: remove the eval() fallback entirely, or gate it behind an explicit unsafe option (default disabled) and document it clearly; in safe mode it should never execute untrusted code.
| try { | |
| result.object = eval( `(${objectString})` ); | |
| if ( Is.definedFunction( result.object ) ) { | |
| result.object = result.object(); | |
| } | |
| } catch ( exception2: any ) { | |
| if ( !configuration.safeMode ) { | |
| console.error( configuration.text!.objectErrorText!.replace( "{{error_1}}", exception1.message ).replace( "{{error_2}}", exception2.message ) ); | |
| result.parsed = false; | |
| } | |
| result.object = null; | |
| } | |
| if ( !configuration.safeMode ) { | |
| console.error( configuration.text!.objectErrorText!.replace( "{{error_1}}", exception1.message ).replace( "{{error_2}}", exception1.message ) ); | |
| } | |
| result.parsed = false; | |
| result.object = null; |
| } else if ( bindingOptions.parse!.stringsToDates && Is.String.date( value ) ) { | ||
| parsedValue = new Date( value ); | ||
|
|
||
| } else if ( bindingOptions.parse!.stringsToSymbols && Is.String.symbol( value ) ) { | ||
| parsedValue = Symbol( Convert.symbolToString( value ) ); | ||
| } |
There was a problem hiding this comment.
Type mismatch/logic error: Convert.symbolToString() expects a Symbol, but here value is a string. This will fail at runtime and should be replaced with string parsing (e.g., strip the Symbol( / ) wrapper from the string) or introduce a dedicated stringSymbolToString() helper and use that here.
| let valid: boolean = value.length >= 2 && value.length <= 7; | ||
|
|
||
| if ( valid && value[ 0 ] === Char.hash ) { | ||
| valid = isNaN( +value.substring( 1, value.length - 1 ) ); | ||
| } else { | ||
| valid = false; | ||
| } | ||
|
|
||
| return valid; |
There was a problem hiding this comment.
hexColor() validation is incorrect: it drops the last character (substring(1, value.length - 1)) and uses numeric coercion (+...) with isNaN which doesn't correctly validate hex strings (and currently treats many invalid values as valid). Recommended: validate via a hex regex like ^#[0-9a-fA-F]{3}([0-9a-fA-F]{3})?$ (or the supported lengths you want).
| let valid: boolean = value.length >= 2 && value.length <= 7; | |
| if ( valid && value[ 0 ] === Char.hash ) { | |
| valid = isNaN( +value.substring( 1, value.length - 1 ) ); | |
| } else { | |
| valid = false; | |
| } | |
| return valid; | |
| const regex: RegExp = /^#[0-9a-fA-F]{3}([0-9a-fA-F]{3})?$/; | |
| return regex.test( value ); |
| export function assignToEvents( bindingOptions: BindingOptions, add: boolean = true ) : void { | ||
| const addEventListener_Window: Function = add ? window.addEventListener : window.removeEventListener; | ||
| const addEventListener_Document: Function = add ? document.addEventListener : document.removeEventListener; | ||
|
|
||
| addEventListener_Window( "mousemove", () => hide( bindingOptions ) ); | ||
| addEventListener_Document( "scroll", () => hide( bindingOptions ) ); | ||
| } |
There was a problem hiding this comment.
Event listener removal will not work here: removeEventListener is passed a new anonymous function, so the original handlers are never removed. This can lead to leaking listeners across instance lifecycle. Fix by storing the exact handler function references (e.g., on bindingOptions._currentView) and using the same references for both add/remove.
| export function assignToEvents( bindingOptions: BindingOptions, add: boolean = true ) : void { | ||
| const addEventListener_Window: Function = add ? window.addEventListener : window.removeEventListener; | ||
| const addEventListener_Document: Function = add ? document.addEventListener : document.removeEventListener; | ||
|
|
||
| addEventListener_Window( "contextmenu", () => hide( bindingOptions ) ); | ||
| addEventListener_Window( "click", () => hide( bindingOptions ) ); | ||
| addEventListener_Document( "scroll", () => hide( bindingOptions ) ); | ||
| } |
There was a problem hiding this comment.
Same issue as tooltip: removeEventListener won't remove these anonymous arrow functions. Store handler references and use them for both add/remove to prevent listener leaks and unexpected cross-instance behavior.
| export function csvStringToObject( csvData: string ) : any[] { | ||
| const jsonObjects: any[] = []; | ||
|
|
||
| const csvLines: string[] = csvData.split( /\r\n|\n/ ); | ||
| const csvLinesLength: number = csvLines.length; | ||
|
|
||
| if ( csvLinesLength > 1 ) { | ||
| const csvHeaders: string[] = csvLines[ 0 ].split( Char.coma ); | ||
| const csvHeadersLength: number = csvHeaders.length; | ||
|
|
||
| if ( csvHeadersLength > 0 ) { | ||
|
|
||
|
|
||
| for ( let csvLineIndex: number = 1; csvLineIndex < csvLinesLength - 1; csvLineIndex++ ) { | ||
| const csvLine: string = csvLines[ csvLineIndex ]; | ||
| const csvLineValues: string[] = csvLine.split( Char.coma ); | ||
| const csvLineValuesLength: number = csvLineValues.length; | ||
| const jsonObject: any = {}; | ||
|
|
||
| for ( let csvLineValueIndex: number = 0; csvLineValueIndex < csvLineValuesLength - 1; csvLineValueIndex++ ) { | ||
| jsonObject[ csvHeaders[ csvLineValueIndex ] ] = csvLineValues[ csvLineValueIndex ]; | ||
| } | ||
|
|
||
| jsonObjects.push( jsonObject ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The loops intentionally drop the last line (csvLinesLength - 1) and the last column (csvLineValuesLength - 1) unconditionally. This will lose data for valid CSVs (unless you always have a trailing empty line and trailing empty column). Recommended: iterate full lengths and explicitly skip empty trailing lines, and map values up to min(headers.length, values.length).
| @@ -0,0 +1,65 @@ | |||
| { | |||
| "name": "jjsontree.js", | |||
There was a problem hiding this comment.
The package name appears to have an extra leading j (jjsontree.js). The README badges/instructions reference jjsontree.js, but if the intent is jsontree.js (or another canonical name), this will affect consumers and publishing. Recommended: confirm the intended published name and make it consistent across package.json, README install instructions, and nuspec id.
| "name": "jjsontree.js", | |
| "name": "jsontree.js", |
| <label for="OverrideVarianceThreshold">Override</label> | ||
| </div> | ||
| <div class="form-row"> | ||
| <label for="ConsecutiveSamples">Number of Consecitive Samples:</label> |
There was a problem hiding this comment.
Correct spelling in the UI label: 'Consecitive' should be 'Consecutive'.
| <label for="ConsecutiveSamples">Number of Consecitive Samples:</label> | |
| <label for="ConsecutiveSamples">Number of Consecutive Samples:</label> |
| color: initial; /* Restores the color for the button part */ | ||
| } | ||
|
|
||
| .souding-devices-select { |
There was a problem hiding this comment.
Class name likely has a typo: .souding-devices-select should be .sounding-devices-select for clarity/consistency (and update the corresponding class attribute in index.html).
| .souding-devices-select { | |
| .sounding-devices-select { |
|
|
||
| When creating a Pull Request: | ||
|
|
||
| 1. State what the intension of the Pull Request is. For example, what are you fixing? what are you adding? etc. |
There was a problem hiding this comment.
Typo in documentation: 'intension' should be 'intention'.
| 1. State what the intension of the Pull Request is. For example, what are you fixing? what are you adding? etc. | |
| 1. State what the intention of the Pull Request is. For example, what are you fixing? what are you adding? etc. |
No description provided.