Conversation
Contributor
WalkthroughThis pull request introduces development-mode instrumentation and debugging enhancements across multiple modules. The main window initialization now includes a configurable timeout mechanism (DevInitTimeoutMs = 5000) with a helper method to race promises against dev timeouts and log results. The terminal wrapper gains extensive state-tracking fields (viewport scroll position, recent writes buffer, transaction flags) and a new handleViewportScroll method for tracking scroll behavior, along with CSI handler extensions to monitor repaint and sync transactions. A debug console.log statement is removed from key handling. The maximum terminal file size is increased from 256 KB to 2 MB. Minor formatting adjustments are applied to telemetry struct field alignment. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Deploying waveterm with
|
| Latest commit: |
18f48e8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://37e3ce40.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-scroll-bug.waveterm.pages.dev |
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Review NotesThis PR adds debugging and scroll behavior improvements for terminal handling: Key Changes:
All changes appear well-implemented with appropriate dev-mode guards and no runtime issues detected. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
emain/emain-window.ts (1)
419-434: Bind timeout DevTools to the timed-out tab instead ofactiveTabView.Using
this.activeTabView(Line 432) can open DevTools on a different tab if focus switched during async init. Pass the initializingtabView(or itswebContents) intoawaitWithDevTimeoutand use that directly.Proposed refactor
- await this.awaitWithDevTimeout(tabView.initPromise, "initPromise", tabView.waveTabId); + await this.awaitWithDevTimeout(tabView.initPromise, "initPromise", tabView); ... - await this.awaitWithDevTimeout(tabView.waveReadyPromise, "waveReadyPromise", tabView.waveTabId); + await this.awaitWithDevTimeout(tabView.waveReadyPromise, "waveReadyPromise", tabView); - private async awaitWithDevTimeout<T>(promise: Promise<T>, name: string, tabId: string): Promise<T> { + private async awaitWithDevTimeout<T>(promise: Promise<T>, name: string, tabView: WaveTabView): Promise<T> { ... - `[dev] ${name} timed out after ${DevInitTimeoutMs}ms for tab ${tabId}, showing window for devtools` + `[dev] ${name} timed out after ${DevInitTimeoutMs}ms for tab ${tabView.waveTabId}, showing window for devtools` ); ... - if (this.activeTabView?.webContents && !this.activeTabView.webContents.isDevToolsOpened()) { - this.activeTabView.webContents.openDevTools(); + if (tabView?.webContents && !tabView.webContents.isDevToolsOpened()) { + tabView.webContents.openDevTools(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emain/emain-window.ts` around lines 419 - 434, The timeout handler in awaitWithDevTimeout uses this.activeTabView.webContents, which can target the wrong tab if focus changes during async init; update awaitWithDevTimeout to accept the initializing TabView (or its WebContents) as an extra parameter (e.g., tabView or tabWebContents) and use that passed instance instead of this.activeTabView when checking isDevToolsOpened() and calling openDevTools(); adjust all call sites to pass the creating tab view/webContents and ensure the existing tabId/name logic and timeout cleanup remain unchanged.frontend/app/view/term/termwrap.ts (1)
472-478: Cap cached write payload length in dev mode to prevent avoidable memory spikes.Keeping only 50 entries helps, but each
dataentry can still be very large. Consider truncating stored payloads (e.g., first N KB) for safer debugging memory usage.Proposed refactor
+const MaxRecentWriteChars = 8 * 1024; if (isDev() && this.loaded) { const dataStr = data instanceof Uint8Array ? new TextDecoder().decode(data) : data; - this.recentWrites.push({ idx: this.recentWritesCounter++, ts: Date.now(), data: dataStr }); + const clipped = dataStr.length > MaxRecentWriteChars ? dataStr.slice(0, MaxRecentWriteChars) : dataStr; + this.recentWrites.push({ idx: this.recentWritesCounter++, ts: Date.now(), data: clipped }); if (this.recentWrites.length > 50) { this.recentWrites.shift(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termwrap.ts` around lines 472 - 478, The dev-mode recentWrites entries can hold very large payloads; cap stored payload size by introducing a MAX_WRITE_LOG_BYTES constant and truncate data before pushing to this.recentWrites. In the block guarded by isDev() && this.loaded, compute a truncated string (for Uint8Array decode only the first N bytes or for string slice the first N chars), append an indicator like "…[truncated]" and also store metadata such as originalLength (use this.recentWritesCounter, ts, originalLength) so debugging still knows full size; update uses of dataStr, this.recentWrites and recentWritesCounter accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 206-243: Wrap the high-frequency console.log calls in the CSI
handlers so they only run in development mode (use the existing isDev() guard
used elsewhere); specifically, gate the logs inside the handlers registered via
terminal.parser.registerCsiHandler (the handler for { final: "J" } that logs
"[termwrap] repaint transaction starting" and the delayed handler that logs
"[termwrap] repaint transaction complete, scrolling to bottom") so they only
execute when isDev() is true, leaving the logic that sets
this.lastClearScrollbackTs, this.inSyncTransaction, this.inRepaintTransaction,
this.lastMode2026SetTs, this.lastMode2026ResetTs and the call to
this.terminal.scrollToBottom() intact.
---
Nitpick comments:
In `@emain/emain-window.ts`:
- Around line 419-434: The timeout handler in awaitWithDevTimeout uses
this.activeTabView.webContents, which can target the wrong tab if focus changes
during async init; update awaitWithDevTimeout to accept the initializing TabView
(or its WebContents) as an extra parameter (e.g., tabView or tabWebContents) and
use that passed instance instead of this.activeTabView when checking
isDevToolsOpened() and calling openDevTools(); adjust all call sites to pass the
creating tab view/webContents and ensure the existing tabId/name logic and
timeout cleanup remain unchanged.
In `@frontend/app/view/term/termwrap.ts`:
- Around line 472-478: The dev-mode recentWrites entries can hold very large
payloads; cap stored payload size by introducing a MAX_WRITE_LOG_BYTES constant
and truncate data before pushing to this.recentWrites. In the block guarded by
isDev() && this.loaded, compute a truncated string (for Uint8Array decode only
the first N bytes or for string slice the first N chars), append an indicator
like "…[truncated]" and also store metadata such as originalLength (use
this.recentWritesCounter, ts, originalLength) so debugging still knows full
size; update uses of dataStr, this.recentWrites and recentWritesCounter
accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emain/emain-window.tsfrontend/app/store/keymodel.tsfrontend/app/view/term/termwrap.tspkg/blockcontroller/blockcontroller.gopkg/telemetry/telemetrydata/telemetrydata.go
💤 Files with no reviewable changes (1)
- frontend/app/store/keymodel.ts
Comment on lines
+206
to
+243
| this.toDispose.push( | ||
| this.terminal.parser.registerCsiHandler({ final: "J" }, (params) => { | ||
| if (params[0] === 3) { | ||
| this.lastClearScrollbackTs = Date.now(); | ||
| if (this.inSyncTransaction) { | ||
| console.log("[termwrap] repaint transaction starting"); | ||
| this.inRepaintTransaction = true; | ||
| } | ||
| } | ||
| return false; | ||
| }) | ||
| ); | ||
| this.toDispose.push( | ||
| this.terminal.parser.registerCsiHandler({ prefix: "?", final: "h" }, (params) => { | ||
| if (params[0] === 2026) { | ||
| this.lastMode2026SetTs = Date.now(); | ||
| this.inSyncTransaction = true; | ||
| } | ||
| return false; | ||
| }) | ||
| ); | ||
| this.toDispose.push( | ||
| this.terminal.parser.registerCsiHandler({ prefix: "?", final: "l" }, (params) => { | ||
| if (params[0] === 2026) { | ||
| this.lastMode2026ResetTs = Date.now(); | ||
| this.inSyncTransaction = false; | ||
| const wasRepaint = this.inRepaintTransaction; | ||
| this.inRepaintTransaction = false; | ||
| if (wasRepaint && Date.now() - this.lastClearScrollbackTs <= MaxRepaintTransactionMs) { | ||
| setTimeout(() => { | ||
| console.log("[termwrap] repaint transaction complete, scrolling to bottom"); | ||
| this.terminal.scrollToBottom(); | ||
| }, 20); | ||
| } | ||
| } | ||
| return false; | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
Gate instrumentation console.log calls behind isDev() to avoid production log spam.
Lines 211, 236, 584-591, and 601 add high-frequency logs on repaint/resize paths. These should be dev-gated like the rest of the debugging instrumentation.
Proposed fix
- console.log("[termwrap] repaint transaction starting");
+ if (isDev()) {
+ console.log("[termwrap] repaint transaction starting");
+ }
...
- console.log("[termwrap] repaint transaction complete, scrolling to bottom");
+ if (isDev()) {
+ console.log("[termwrap] repaint transaction complete, scrolling to bottom");
+ }
this.terminal.scrollToBottom();
...
- console.log(
- "[termwrap] resize",
- `${oldRows}x${oldCols}`,
- "->",
- `${this.terminal.rows}x${this.terminal.cols}`,
- "atBottom:",
- atBottom
- );
+ if (isDev()) {
+ console.log(
+ "[termwrap] resize",
+ `${oldRows}x${oldCols}`,
+ "->",
+ `${this.terminal.rows}x${this.terminal.cols}`,
+ "atBottom:",
+ atBottom
+ );
+ }
...
- console.log("[termwrap] resize scroll-to-bottom");
+ if (isDev()) {
+ console.log("[termwrap] resize scroll-to-bottom");
+ }Also applies to: 584-602
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/termwrap.ts` around lines 206 - 243, Wrap the
high-frequency console.log calls in the CSI handlers so they only run in
development mode (use the existing isDev() guard used elsewhere); specifically,
gate the logs inside the handlers registered via
terminal.parser.registerCsiHandler (the handler for { final: "J" } that logs
"[termwrap] repaint transaction starting" and the delayed handler that logs
"[termwrap] repaint transaction complete, scrolling to bottom") so they only
execute when isDev() is true, leaving the logic that sets
this.lastClearScrollbackTs, this.inSyncTransaction, this.inRepaintTransaction,
this.lastMode2026SetTs, this.lastMode2026ResetTs and the call to
this.terminal.scrollToBottom() intact.
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There may be more cases here that I don't know about, but this fixes a good chunk of them. This catches the CC "repaint" transaction and forces a scrollToBottom. That should handle context repaints and resize repaints.
Also adds a new (hidden) terminal escape sequence debugger, and (in dev mode) adds a last 50 writes cache that can be used to look at and debug output.