WIP#4652
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR replaces the ChangesMobile Terminal Input Migration
🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit 9aa5eb5 on May 16, 2026 11:32pm UTC. |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Greptile SummaryThis PR extracts duplicate mobile-keyboard-toolbar code from
Confidence Score: 4/5Safe to merge — the refactor is a clean extraction with no functional regressions; the two open issues are non-blocking style/performance concerns. The inline Both
|
| Filename | Overview |
|---|---|
| apps/web/src/components/MobileTerminalInput/MobileTerminalInput.tsx | New shared mobile terminal input component with hidden textarea for keyboard capture, key button toolbar, and composition/paste handling — logic is sound with one dependency-stability concern |
| apps/web/src/app/(public)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx | Switches from deleted MobileToolbar to shared MobileTerminalInput; adds sendInputText wrapper and adapts onSend signature from Uint8Array to string — functionally equivalent |
| apps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/WebTerminal.tsx | Replaces inline key-button strip with MobileTerminalInput; adds terminalRef for programmatic focus; MobileTerminalInput is always rendered with toolbarVisibility="always" |
| apps/web/src/app/(public)/agents/remote-control/[sessionId]/components/RemoteTerminal/components/MobileToolbar/MobileToolbar.tsx | Deleted — superseded by the new shared MobileTerminalInput component |
| apps/web/src/components/MobileTerminalInput/index.ts | New barrel export for MobileTerminalInput |
Sequence Diagram
sequenceDiagram
participant User as User (mobile touch)
participant CT as containerRef div
participant MIT as MobileTerminalInput
participant TA as Hidden Textarea
participant OS as onSend callback
participant WS as WebSocket / tRPC
User->>CT: touchstart / pointerdown (touch)
CT->>MIT: event listener fires
MIT->>TA: "focus({ preventScroll: true })"
Note over TA: Virtual keyboard opens
User->>TA: types character (onInput)
TA->>MIT: flushTextareaValue()
MIT->>OS: onSend(string)
OS->>WS: send input message
User->>MIT: taps toolbar button (Tab / Esc / Arrow / etc.)
MIT->>TA: focus (via sendButtonSequence)
MIT->>OS: onSend(sequence)
OS->>WS: send input message
User->>CT: pointerdown (mouse)
CT->>MIT: event listener fires
MIT->>OS: onFocusTerminal()
Note over OS: xterm Terminal.focus()
Comments Outside Diff (1)
-
apps/web/src/app/(public)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx, line 376-378 (link)The
onFocusTerminalprop receives a new inline arrow function on every render ofRemoteTerminal. BecauseMobileTerminalInputlistsonFocusTerminalin itsuseEffectdependency array, this causes the effect to tear down and re-add thepointerdown/touchstartlisteners on every parent render — during active use (state changes, pings, viewer-count updates) that can be frequent. Wrapping it inuseCallbackmakes the reference stable.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/web/src/app/(public)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx Line: 376-378 Comment: The `onFocusTerminal` prop receives a new inline arrow function on every render of `RemoteTerminal`. Because `MobileTerminalInput` lists `onFocusTerminal` in its `useEffect` dependency array, this causes the effect to tear down and re-add the `pointerdown`/`touchstart` listeners on every parent render — during active use (state changes, pings, viewer-count updates) that can be frequent. Wrapping it in `useCallback` makes the reference stable. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/web/src/app/(public)/agents/remote-control/[sessionId]/components/RemoteTerminal/RemoteTerminal.tsx:376-378
The `onFocusTerminal` prop receives a new inline arrow function on every render of `RemoteTerminal`. Because `MobileTerminalInput` lists `onFocusTerminal` in its `useEffect` dependency array, this causes the effect to tear down and re-add the `pointerdown`/`touchstart` listeners on every parent render — during active use (state changes, pings, viewer-count updates) that can be frequent. Wrapping it in `useCallback` makes the reference stable.
```suggestion
const isFull = meta?.mode === "full" && state === "open";
const focusTerminal = useCallback(() => {
termRef.current?.focus();
}, []);
return (
```
### Issue 2 of 3
apps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/WebTerminal.tsx:243-248
Same pattern as `RemoteTerminal`: an inline `() => terminalRef.current?.focus()` is passed as `onFocusTerminal`, so `MobileTerminalInput`'s event-listener effect tears down and re-adds `pointerdown`/`touchstart` listeners on every render of `WebTerminal` (connection-state changes, etc.). Define a stable callback with `useCallback` — e.g. `const focusTerminal = useCallback(() => { terminalRef.current?.focus(); }, [])` — and pass that reference instead.
### Issue 3 of 3
apps/web/src/components/MobileTerminalInput/MobileTerminalInput.tsx:39-60
**Stale ref if target is null on first mount**
`focusTargetRef.current` is read once when the effect runs. If it is `null` at that point (e.g., the container mounts asynchronously after `MobileTerminalInput`), the effect exits early without registering any listeners, and because `focusTargetRef` itself is a stable object its identity never changes, so the effect never re-runs to retry. In practice the current callers always mount the container before or simultaneously with `MobileTerminalInput`, but the component would silently do nothing if consumed in a context where the ref is populated later.
Reviews (1): Last reviewed commit: "WIP" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <MobileTerminalInput | ||
| focusTargetRef={containerRef} | ||
| onFocusTerminal={() => terminalRef.current?.focus()} | ||
| onSend={sendSequence} | ||
| toolbarVisibility="always" | ||
| /> |
There was a problem hiding this comment.
RemoteTerminal: an inline () => terminalRef.current?.focus() is passed as onFocusTerminal, so MobileTerminalInput's event-listener effect tears down and re-adds pointerdown/touchstart listeners on every render of WebTerminal (connection-state changes, etc.). Define a stable callback with useCallback — e.g. const focusTerminal = useCallback(() => { terminalRef.current?.focus(); }, []) — and pass that reference instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/app/workspaces/[workspaceId]/components/WebTerminal/WebTerminal.tsx
Line: 243-248
Comment:
Same pattern as `RemoteTerminal`: an inline `() => terminalRef.current?.focus()` is passed as `onFocusTerminal`, so `MobileTerminalInput`'s event-listener effect tears down and re-adds `pointerdown`/`touchstart` listeners on every render of `WebTerminal` (connection-state changes, etc.). Define a stable callback with `useCallback` — e.g. `const focusTerminal = useCallback(() => { terminalRef.current?.focus(); }, [])` — and pass that reference instead.
How can I resolve this? If you propose a fix, please make it concise.| useEffect(() => { | ||
| const target = focusTargetRef.current; | ||
| if (!target) return; | ||
|
|
||
| const onPointerDown = (event: PointerEvent) => { | ||
| if (event.pointerType === "mouse") { | ||
| onFocusTerminal?.(); | ||
| return; | ||
| } | ||
| focusKeyboardInput(); | ||
| }; | ||
| const onTouchStart = () => { | ||
| focusKeyboardInput(); | ||
| }; | ||
|
|
||
| target.addEventListener("pointerdown", onPointerDown, { passive: true }); | ||
| target.addEventListener("touchstart", onTouchStart, { passive: true }); | ||
| return () => { | ||
| target.removeEventListener("pointerdown", onPointerDown); | ||
| target.removeEventListener("touchstart", onTouchStart); | ||
| }; | ||
| }, [focusKeyboardInput, focusTargetRef, onFocusTerminal]); |
There was a problem hiding this comment.
focusTargetRef.current is read once when the effect runs. If it is null at that point (e.g., the container mounts asynchronously after MobileTerminalInput), the effect exits early without registering any listeners, and because focusTargetRef itself is a stable object its identity never changes, so the effect never re-runs to retry. In practice the current callers always mount the container before or simultaneously with MobileTerminalInput, but the component would silently do nothing if consumed in a context where the ref is populated later.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/components/MobileTerminalInput/MobileTerminalInput.tsx
Line: 39-60
Comment:
**Stale ref if target is null on first mount**
`focusTargetRef.current` is read once when the effect runs. If it is `null` at that point (e.g., the container mounts asynchronously after `MobileTerminalInput`), the effect exits early without registering any listeners, and because `focusTargetRef` itself is a stable object its identity never changes, so the effect never re-runs to retry. In practice the current callers always mount the container before or simultaneously with `MobileTerminalInput`, but the component would silently do nothing if consumed in a context where the ref is populated later.
How can I resolve this? If you propose a fix, please make it concise.
…4652) No code change: apps/web WebTerminal and MobileTerminalInput are byte-aligned with upstream/main for the superset-sh#4652-relevant files; remote-control viewer paths from that WIP are not present because the remote-control feature remains deferred.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Introduce a shared
MobileTerminalInputto improve typing and key controls on mobile for both Remote and Web terminals. Replaces the old toolbar, adds IME-safe input, paste support, and better focus behavior.New Features
MobileTerminalInputused by RemoteTerminal and WebTerminal.Dependencies
bun.lock:@superset/desktopto 1.9.6,@superset/clito 0.2.19,@superset/host-serviceto 0.8.6.Written for commit 9aa5eb5. Summary will update on new commits. Review in cubic
Summary by CodeRabbit