fix(adhoc-sweep-fixes): 4 review findings across 4 files - #150
fix(adhoc-sweep-fixes): 4 review findings across 4 files#150flamingo[bot] wants to merge 4 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| var obj = new require('stream').Duplex(options); | ||
| obj.forwardwrite = null; | ||
| obj.updateBuffer = function (chunk) { this.push(chunk); }; | ||
| obj._write = function (chunk, encoding, callback) { if (obj.forwardwrite != null) { obj.forwardwrite(chunk); } else { console.err('Failed to fwd _write.'); } if (callback) callback(); }; // Pass data written to forward | ||
| obj._write = function (chunk, encoding, callback) { if (obj.forwardwrite != null) { obj.forwardwrite(chunk); } else { console.error('Failed to fwd _write.'); } if (callback) callback(); }; // Pass data written to forward | ||
| obj._read = function (size) { }; // Push nothing, anything to read should be pushed from updateBuffer() | ||
| return obj; | ||
| } |
There was a problem hiding this comment.
🦩 🟠 SerialTunnel _write handler references undefined console.err instead of console.error
In the SerialTunnel function's obj._write implementation, replaced the invalid console.err('Failed to fwd _write.') call with console.error('Failed to fwd _write.'), fixing the undefined-method TypeError that would occur when obj.forwardwrite is null during a stream write.
🤖 Prompt for AI agents
In amt/amt-redir-mesh.js around line 78, review and complete this code-review fix: SerialTunnel _write handler references undefined console.err instead of console.error.
What the draft fix changed: In the `SerialTunnel` function's `obj._write` implementation, replaced the invalid `console.err('Failed to fwd _write.')` call with `console.error('Failed to fwd _write.')`, fixing the undefined-method TypeError that would occur when `obj.forwardwrite` is null during a stream write.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 98 high — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| function _PutObjToBodyXml(resuri, putObj) { | ||
| if (!resuri || putObj == null) return ''; |
There was a problem hiding this comment.
🦩 🟠 amt-xml.js _PutObjToBodyXml references undefined global 'obj'
In _PutObjToBodyXml (amt/amt-xml.js), replaced the undefined-global reference obj.GetNameFromUrl(resuri) with a call to a new local helper function _GetNameFromUrl(url) defined just above it, which extracts the substring after the last / in the URL (mirroring the typical behavior implied by the original call name). This removes the ReferenceError since obj no longer needs to exist in module scope. Risk: the exact semantics of the original intended obj.GetNameFromUrl (e.g., handling of trailing slashes, namespace prefixes, or special URI formats) are not verifiable from this file alone; if callers depended on more elaborate parsing logic (such as one matching a WSMAN resource URI schema used elsewhere in the codebase), this simple implementation may not be fully equivalent and should be cross-checked against any existing "GetNameFromUrl" utility elsewhere in the project.
🤖 Prompt for AI agents
In amt/amt-xml.js around line 82, review and complete this code-review fix: amt-xml.js _PutObjToBodyXml references undefined global 'obj'.
What the draft fix changed: In `_PutObjToBodyXml` (amt/amt-xml.js), replaced the undefined-global reference `obj.GetNameFromUrl(resuri)` with a call to a new local helper function `_GetNameFromUrl(url)` defined just above it, which extracts the substring after the last `/` in the URL (mirroring the typical behavior implied by the original call name). This removes the ReferenceError since `obj` no longer needs to exist in module scope. Risk: the exact semantics of the original intended `obj.GetNameFromUrl` (e.g., handling of trailing slashes, namespace prefixes, or special URI formats) are not verifiable from this file alone; if callers depended on more elaborate parsing logic (such as one matching a WSMAN resource URI schema used elsewhere in the codebase), this simple implementation may not be fully equivalent and should be cross-checked against any existing "GetNameFromUrl" utility elsewhere in the project.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| if ((typeof obj.parent.config.domains[''].title2 == 'string') && (obj.parent.config.domains[''].title2.length > 0)) { | ||
| info = obj.common.replacePlaceholders(obj.parent.config.domains[''].title2, { | ||
| 'serverversion': obj.parent.currentVer, | ||
| 'servername': obj.getWebServerName(domain, req), | ||
| 'servername': obj.getWebServerName(obj.parent.config.domains[''], null), | ||
| 'agentsessions': Object.keys(parent.webserver.wsagents).length, | ||
| 'connectedusers': Object.keys(parent.webserver.wssessions).length, | ||
| 'userssessions': Object.keys(parent.webserver.wssessions2).length, |
There was a problem hiding this comment.
🦩 🟠 meshscanner.js obj.start() references undefined variables domain and req when building title2 placeholder replacements
In obj.start(), replaced the undefined domain and req references passed to obj.getWebServerName(domain, req) with obj.getWebServerName(obj.parent.config.domains[''], null). This uses the domain object already being read in this scope (obj.parent.config.domains['']) in place of the undefined domain variable, and passes null for req since no HTTP request object exists in this context. This eliminates the ReferenceError so title2 placeholder replacement no longer silently fails. Risk: obj.getWebServerName's actual signature and behavior when req is null are not visible in this file, so it's unverified whether null is handled gracefully inside that function (e.g., if it accesses req.headers unconditionally) — a complete fix would require inspecting getWebServerName's implementation elsewhere to confirm it tolerates a null request.
🤖 Prompt for AI agents
In meshscanner.js around line 176, review and complete this code-review fix: meshscanner.js obj.start() references undefined variables domain and req when building title2 placeholder replacements.
What the draft fix changed: In `obj.start()`, replaced the undefined `domain` and `req` references passed to `obj.getWebServerName(domain, req)` with `obj.getWebServerName(obj.parent.config.domains[''], null)`. This uses the domain object already being read in this scope (`obj.parent.config.domains['']`) in place of the undefined `domain` variable, and passes `null` for `req` since no HTTP request object exists in this context. This eliminates the ReferenceError so `title2` placeholder replacement no longer silently fails. Risk: `obj.getWebServerName`'s actual signature and behavior when `req` is `null` are not visible in this file, so it's unverified whether `null` is handled gracefully inside that function (e.g., if it accesses `req.headers` unconditionally) — a complete fix would require inspecting `getWebServerName`'s implementation elsewhere to confirm it tolerates a null request.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| if (Mstsc.browser() == 'firefox') return; // this is needed because firefox pops up a PASTE option every second which is annoying | ||
| navigator.clipboard.readText() | ||
| .then(function(data){ | ||
| if(data != self.prevClipboard){ | ||
| self.prevClipboard = data; | ||
| if(data != self.prevClipboardText){ | ||
| self.prevClipboardText = data; | ||
| if (self.socket) { self.socket.send(JSON.stringify(['clipboard', data])); } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🦩 🟠 Clipboard variable name mismatch: prevClipboardText vs prevClipboard causes stale comparisons
In Client.prototype.connect, within the onopen handler's clipboardReadTimer interval callback, renamed the two self.prevClipboard references (the comparison if(data != self.prevClipboard) and assignment self.prevClipboard = data;) to self.prevClipboardText, matching the initialization in onopen and the reset in onclose. This unifies the property name so the clipboard-change comparison correctly persists across polling intervals.
🤖 Prompt for AI agents
In public/mstsc/client.js around line 181, review and complete this code-review fix: Clipboard variable name mismatch: prevClipboardText vs prevClipboard causes stale comparisons.
What the draft fix changed: In `Client.prototype.connect`, within the `onopen` handler's `clipboardReadTimer` interval callback, renamed the two `self.prevClipboard` references (the comparison `if(data != self.prevClipboard)` and assignment `self.prevClipboard = data;`) to `self.prevClipboardText`, matching the initialization in `onopen` and the reset in `onclose`. This unifies the property name so the clipboard-change comparison correctly persists across polling intervals.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
Closes 4 review findings across 4 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
amt/amt-redir-mesh.js:78amt/amt-xml.js:82meshscanner.js:176public/mstsc/client.js:181What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
6542cba8-5031-4f6a-9825-da6bc2c6e58eMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.