fix(web): restore main check/test stability after merge regressions#601
fix(web): restore main check/test stability after merge regressions#601nikolasdehor wants to merge 1 commit intoamantus-ai:mainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Fixes a set of web/test regressions impacting check/test stability by restoring missing session-view state contracts, cleaning up merge artifacts, and making test execution paths more resilient/deterministic.
Changes:
- Make test binary resolution more robust and ensure a deterministic
native/vibetunnellauncher for tests. - Restore/extend session-view UI state contracts for mobile keyboard/direct-keyboard flows and adjust quick-keys/keyboard-button behavior.
- Remove stale/duplicate logic introduced by merge regressions and update auth login copy.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/test/helpers/vt-paths.ts | Prefer native/vibetunnel when present, fall back to bin/vibetunnel for tests. |
| web/src/client/utils/cast-converter.ts | Removes duplicated/merged stream-connector implementation and keeps the canonical connectToStream. |
| web/src/client/components/terminal-chat-view.ts | Removes dead “waiting” state writes and adjusts option-selection logging. |
| web/src/client/components/session-view/ui-state-manager.ts | Restores/extends UI state (direct keyboard + mobile input) and adds toggles. |
| web/src/client/components/session-view/overlays-container.ts | Hides floating keyboard button while quick keys are visible. |
| web/src/client/components/session-view/input-manager.ts | Removes stale websocket-input cleanup code paths. |
| web/src/client/components/session-view/direct-keyboard-manager.ts | Adds callback contract for showMobileInput to coordinate input suppression. |
| web/src/client/components/session-view.ts | Wires new UI state/callbacks and adds handlers for special keys + mobile input toggle; formatting cleanup. |
| web/src/client/components/auth-login.ts | Updates login UX copy to expected text. |
| web/scripts/ensure-native-modules.js | Creates native/vibetunnel launcher for tests/dev to stabilize integration test execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fs.writeFileSync(nativeLauncherPath, launcherScript, 'utf8'); | ||
| fs.chmodSync(nativeLauncherPath, 0o755); | ||
| console.log(`Created native launcher for tests: ${nativeLauncherPath}`); |
There was a problem hiding this comment.
This unconditionally overwrites native/vibetunnel with a shell launcher. That path is also used by the SEA build (see build-native.js), so running tests after building the native executable would clobber it and potentially break local workflows. Consider only creating/updating the launcher when native/vibetunnel is missing or when the existing file is already the expected launcher (e.g., starts with #! and matches the generated content).
| fs.writeFileSync(nativeLauncherPath, launcherScript, 'utf8'); | |
| fs.chmodSync(nativeLauncherPath, 0o755); | |
| console.log(`Created native launcher for tests: ${nativeLauncherPath}`); | |
| let shouldWriteLauncher = true; | |
| if (fs.existsSync(nativeLauncherPath)) { | |
| try { | |
| const existingContent = fs.readFileSync(nativeLauncherPath, 'utf8'); | |
| if (!existingContent.startsWith('#!')) { | |
| // Existing file is not a shell launcher (likely a native binary); do not clobber it. | |
| console.log( | |
| `native/vibetunnel already exists and is not a shell launcher; preserving existing file: ${nativeLauncherPath}` | |
| ); | |
| shouldWriteLauncher = false; | |
| } else if (existingContent === launcherScript) { | |
| // Launcher is already up to date; nothing to do. | |
| shouldWriteLauncher = false; | |
| } | |
| } catch (e) { | |
| console.warn( | |
| `Failed to read existing native/vibetunnel; attempting to recreate launcher: ${e.message}` | |
| ); | |
| } | |
| } | |
| if (shouldWriteLauncher) { | |
| fs.writeFileSync(nativeLauncherPath, launcherScript, 'utf8'); | |
| fs.chmodSync(nativeLauncherPath, 0o755); | |
| console.log(`Created native launcher for tests: ${nativeLauncherPath}`); | |
| } |
| !this.uiState.showQuickKeys | ||
| ? html` | ||
| <div | ||
| class="keyboard-button mobile-keyboard-button ${this.uiState.showQuickKeys ? 'quick-keys-visible' : ''}" |
There was a problem hiding this comment.
With the new render condition including !this.uiState.showQuickKeys, the ${this.uiState.showQuickKeys ? 'quick-keys-visible' : ''} class segment is now always empty (dead code). Consider removing this conditional class to avoid misleading future readers, or revisiting the render condition if the class is still intended to apply sometimes.
| class="keyboard-button mobile-keyboard-button ${this.uiState.showQuickKeys ? 'quick-keys-visible' : ''}" | |
| class="keyboard-button mobile-keyboard-button" |
Summary
cast-converter.tsUIStateManagerfields/methods + direct keyboard callback)input-managerand clean dead waiting-state writes interminal-chat-viewweb/native/vibetunnelinensure-native-modules.jssrc/test/helpers/vt-paths.tsValidation (local)
cd web && pnpm run check✅cd web && pnpm run test✅Context
This addresses real regressions observed on current
mainwhere check/test failed locally before these fixes.