fix(MULTIPLA-002): CU-86akhf8u5 3 review findings in page.tsx - #390
flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -351,7 +357,7 @@ export default function RemoteShellPage() { | |||
|
|
|||
| {/* Terminal */} | |||
There was a problem hiding this comment.
🦩 🔴 Hardcoded bg-black class instead of ODS design token
Replaced the hardcoded bg-black Tailwind class on the terminal wrapper div (in the JSX return, "Terminal" section) with bg-ods-terminal-bg. This assumes a semantic ODS token named bg-ods-terminal-bg is defined (or will be defined) in the Tailwind/ODS token config; since I cannot see the design-token source file, I did not invent a new token definition file, only referenced the name. If this token does not exist in the ODS Tailwind config, this change will fail to render the intended background and the token must be added to the design-system config (not part of this file) before merge.
🤖 Prompt for AI agents
In src/app/(app)/devices/details/remote-shell/page.tsx around line 352, review and complete this code-review fix: Hardcoded bg-black class instead of ODS design token.
What the draft fix changed: Replaced the hardcoded `bg-black` Tailwind class on the terminal wrapper `div` (in the JSX return, "Terminal" section) with `bg-ods-terminal-bg`. This assumes a semantic ODS token named `bg-ods-terminal-bg` is defined (or will be defined) in the Tailwind/ODS token config; since I cannot see the design-token source file, I did not invent a new token definition file, only referenced the name. If this token does not exist in the ODS Tailwind config, this change will fail to render the intended background and the token must be added to the design-system config (not part of this file) before merge.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| @@ -100,7 +103,7 @@ export default function RemoteShellPage() { | |||
|
|
|||
| const term = new Terminal({ | |||
There was a problem hiding this comment.
🦩 🔴 Hardcoded terminal background hex value instead of ODS token
Extracted the raw hex #000000 xterm theme background into a module-level constant ODS_TERMINAL_BG (near the top of the file, alongside WINDOWS_POWERSHELL_CMD) and used it in the new Terminal({...}) call inside the terminal-setup effect. This keeps a single named source of truth tied conceptually to the same terminal surface color used by bg-ods-terminal-bg, but xterm's theme.background option only accepts a literal color string, not a Tailwind class or CSS variable reference, so full resolution through the actual ODS token system (e.g. reading a CSS custom property at runtime) is not implemented here — a complete fix would need to read the resolved color from the ODS token/CSS variable at runtime (e.g., via getComputedStyle) to guarantee visual parity if the token value ever changes.
🤖 Prompt for AI agents
In src/app/(app)/devices/details/remote-shell/page.tsx around line 101, review and complete this code-review fix: Hardcoded terminal background hex value instead of ODS token.
What the draft fix changed: Extracted the raw hex `#000000` xterm theme background into a module-level constant `ODS_TERMINAL_BG` (near the top of the file, alongside `WINDOWS_POWERSHELL_CMD`) and used it in the `new Terminal({...})` call inside the terminal-setup effect. This keeps a single named source of truth tied conceptually to the same terminal surface color used by `bg-ods-terminal-bg`, but xterm's `theme.background` option only accepts a literal color string, not a Tailwind class or CSS variable reference, so full resolution through the actual ODS token system (e.g. reading a CSS custom property at runtime) is not implemented here — a complete fix would need to read the resolved color from the ODS token/CSS variable at runtime (e.g., via `getComputedStyle`) to guarantee visual parity if the token value ever changes.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer
| !powershellCommandSentRef.current && | ||
| tunnelRef.current | ||
| ) { | ||
| setTimeout(() => { | ||
| if (tunnelRef.current && !powershellCommandSentRef.current) { | ||
| tunnelRef.current.sendBinary(new TextEncoder().encode(WINDOWS_POWERSHELL_CMD + '\r')); | ||
| const activeTunnel = tunnelRef.current; | ||
| const timeoutId = setTimeout(() => { | ||
| if (tunnelRef.current === activeTunnel && !powershellCommandSentRef.current) { | ||
| activeTunnel.sendBinary(new TextEncoder().encode(WINDOWS_POWERSHELL_CMD + '\r')); | ||
| powershellCommandSentRef.current = true; | ||
| } | ||
| }, 100); | ||
| return () => clearTimeout(timeoutId); | ||
| } | ||
| return undefined; | ||
| }, [state, shellType, hasReceivedData]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🦩 🟠 powershell auto-command sent via unguarded setTimeout without cleanup could fire after unmount/reconnect
Fixed the unguarded setTimeout in the PowerShell auto-command effect: captured tunnelRef.current as activeTunnel at schedule time, changed the inner guard to compare tunnelRef.current === activeTunnel (so a stale timeout won't fire against a different/reconnected tunnel instance), and added a cleanup function returning clearTimeout(timeoutId) so the timeout is cleared on unmount or effect re-run. This addresses the two failure modes described (unmount and tunnel replacement) but does not add an explicit "is mounted" boolean since the tunnel-identity check already covers the practical race described in the finding.
🤖 Prompt for AI agents
In src/app/(app)/devices/details/remote-shell/page.tsx around line 151, review and complete this code-review fix: powershell auto-command sent via unguarded setTimeout without cleanup could fire after unmount/reconnect.
What the draft fix changed: Fixed the unguarded `setTimeout` in the PowerShell auto-command effect: captured `tunnelRef.current` as `activeTunnel` at schedule time, changed the inner guard to compare `tunnelRef.current === activeTunnel` (so a stale timeout won't fire against a different/reconnected tunnel instance), and added a cleanup function returning `clearTimeout(timeoutId)` so the timeout is cleared on unmount or effect re-run. This addresses the two failure modes described (unmount and tunnel replacement) but does not add an explicit "is mounted" boolean since the tunnel-identity check already covers the practical race described in the finding.
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
flamingo
Bot
changed the title
1 participant
Closes 3 review findings in
src/app/(app)/devices/details/remote-shell/page.tsx.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
src/app/(app)/devices/details/remote-shell/page.tsx:352src/app/(app)/devices/details/remote-shell/page.tsx:101src/app/(app)/devices/details/remote-shell/page.tsx:151What 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:
c69bf2d8-6eaa-4e2d-815b-af08b880c8dfMerging 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.
ClickUp task: CU-86akhf8u5 OpenFrame OSS frontend review findings sweep (12 PRs)