Claude finished @steipete's task —— View job

📋 Summary

This PR addresses the critical mobile terminal resize loop issue by removing the complex ResizeCoordinator class and implementing a simpler, more direct approach. Mobile devices now get exactly one resize 200ms after initialization (to set width), then only height changes are allowed (for keyboard events). This directly solves the resize loop problem identified in Mario's debugging session that caused exponential data growth from re-rendering the entire scroll buffer.

✅ Positive Aspects

• Pragmatic solution: The "resize once on mobile" approach is simple and effective for solving the resize loop
• Significant simplification: Removing ResizeCoordinator eliminates ~119 lines of coordination complexity
• Clear mobile/desktop separation: Different resize strategies for different platforms are well-defined
• Improved debugging: Added extensive debug logging throughout resize operations
• Comprehensive documentation: Included Mario's detailed debugging notes explaining the root cause
• Test coverage maintained: Tests were properly updated to reflect the new behavior
• Mobile detection unified: Now uses consistent detection logic between index.html and terminal.ts

🔍 Areas for Improvement

Code Quality

1. Magic numbers without constants:

   • 200ms delay for mobile resize (terminal.ts:624)
   • 768px mobile breakpoint (mentioned in old code, not used in new detection)
   • 250ms resize debounce in terminal-lifecycle-manager

2. Potential memory leak: The setTimeout in setupResize() (line 624) is not stored or canceled in cleanup, which could cause:

   • Memory leaks if component is unmounted before timeout fires
   • Errors if fitTerminal is called on a disposed component

3. Height-only change detection: The logic for isHeightOnlyChange in terminal.ts (line 570) depends on the order of operations and could be fragile

Potential Issues

1. Race condition: The 200ms timeout might fire before the terminal is fully initialized

2. Orientation changes: Mobile users cannot adjust layout when rotating their device, which may leave them with suboptimal terminal dimensions

3. Tablet edge cases: The new mobile detection based on user agent and maxTouchPoints might have different behavior than the old width-based detection

4. Height resizing on mobile: While the PR claims to block all resizes on mobile after initial, the code actually allows height changes for keyboard events, which could still trigger some resize operations

Performance

• Multiple calls to detectMobile() could be optimized by caching the result
• The detection logic runs regex test on every call

Security

No security issues identified.

Testing

1. Missing mobile-specific tests: No explicit tests for the 200ms delay mobile resize behavior
2. Timing-dependent tests: The 200ms delay makes testing more fragile
3. No tests for orientation changes: Missing coverage for rotation scenarios on mobile

💡 Suggestions

1. Fix Memory Leak

// In terminal.ts, add class property:
private mobileResizeTimeout: NodeJS.Timeout | null = null;

// In setupResize():
if (this.isMobile) {
  logger.debug('[Terminal] Mobile detected - scheduling initial resize in 200ms');
  this.mobileResizeTimeout = setTimeout(() => {
    if (!this.terminal || !this.container) {
      logger.warn('[Terminal] Mobile resize aborted - terminal not ready');
      return;
    }
    logger.debug('[Terminal] Mobile: Executing initial resize');
    this.fitTerminal('initial-mobile-only');
    logger.debug('[Terminal] Mobile: Initial width set, future WIDTH resizes blocked');
  }, 200);
}

// In disconnectedCallback():
if (this.mobileResizeTimeout) {
  clearTimeout(this.mobileResizeTimeout);
  this.mobileResizeTimeout = null;
}

2. Extract Constants

// At top of terminal.ts
private static readonly MOBILE_RESIZE_DELAY_MS = 200;
private static readonly RESIZE_DEBOUNCE_MS = 250; // for terminal-lifecycle-manager

3. Cache Mobile Detection

private cachedIsMobile: boolean | null = null;

private detectMobile(): boolean {
  if (this.cachedIsMobile !== null) {
    return this.cachedIsMobile;
  }
  
  this.cachedIsMobile = (
    /iPhone|iPad|iPod|Android/i.test(navigator.userAgent) ||
    (navigator.maxTouchPoints !== undefined && navigator.maxTouchPoints > 1)
  );
  
  return this.cachedIsMobile;
}

4. Consider Manual Resize Option

For mobile users who need to adjust after orientation changes, consider adding a button or gesture to manually trigger a resize.

🎯 Priority Items

1. Fix memory leak: Store and cleanup the mobile resize timeout to prevent errors on component disposal
2. Define constants: Replace magic numbers (200ms, 250ms) with named constants
3. Add error handling: Ensure terminal is ready before the mobile resize executes
4. Add mobile-specific tests: Test the 200ms delay behavior and width-blocking logic
5. Document height resize behavior: The code allows height changes on mobile for keyboard events, but this isn't clearly documented in the PR description

Overall, this is a solid pragmatic fix that should significantly improve mobile stability by eliminating the resize loop issue. The simplification by removing ResizeCoordinator is a good architectural decision. The main concerns are around the memory leak and missing test coverage for the mobile-specific behavior.

Reviewed commit: c815168
Files changed: 8

Claude encountered an error —— View job

Claude finished @steipete's task —— View job

Todo List:

• Read terminal.ts to understand new resize implementation
• Review deleted resize-coordinator.ts to understand what was removed
• Review test changes in terminal.test.ts
• Check other modified files for consistency
• Review new documentation files
• Provide comprehensive code review

📋 Summary

This PR addresses the critical mobile terminal resize loop issue by removing the complex ResizeCoordinator class (~119 lines) and implementing a simpler, more direct approach. Mobile devices now get exactly one resize 200ms after initialization to set width, then only height changes are allowed (for keyboard events). This directly solves the resize loop problem identified in Mario's debugging session that caused exponential data growth from re-rendering the entire scroll buffer.

✅ Positive Aspects

• Pragmatic solution: The "resize once on mobile" approach is simple and effective for solving the resize loop
• Significant simplification: Removing ResizeCoordinator eliminates ~119 lines of coordination complexity
• Clear mobile/desktop separation: Different resize strategies for different platforms are well-defined
• Improved debugging: Added extensive debug logging throughout resize operations
• Comprehensive documentation: Included Mario's detailed debugging notes explaining the root cause
• Test coverage maintained: Tests were properly updated to reflect the new behavior
• Mobile detection unified: Now uses consistent detection logic between index.html and terminal.ts

🔍 Areas for Improvement

Code Quality

1. Magic numbers without constants:

   • 200ms delay for mobile resize (terminal.ts:624)
   • 768px mobile breakpoint (mentioned in old code, not used in new detection)
   • 250ms resize debounce in terminal-lifecycle-manager

2. Potential memory leak: The setTimeout in setupResize() (line 624) is not stored or canceled in cleanup, which could cause:

   • Memory leaks if component is unmounted before timeout fires
   • Errors if fitTerminal is called on a disposed component

3. Height-only change detection: The logic for isHeightOnlyChange in terminal.ts (line 570) depends on the order of operations and could be fragile

Potential Issues

1. Race condition: The 200ms timeout might fire before the terminal is fully initialized

2. Orientation changes: Mobile users cannot adjust layout when rotating their device, which may leave them with suboptimal terminal dimensions

3. Tablet edge cases: The new mobile detection based on user agent and maxTouchPoints might have different behavior than the old width-based detection

4. Height resizing on mobile: While the PR claims to block all resizes on mobile after initial, the code actually allows height changes for keyboard events, which could still trigger some resize operations

Performance

• Multiple calls to detectMobile() could be optimized by caching the result
• The detection logic runs regex test on every call

Security

No security issues identified.

Testing

1. Missing mobile-specific tests: No explicit tests for the 200ms delay mobile resize behavior
2. Timing-dependent tests: The 200ms delay makes testing more fragile
3. No tests for orientation changes: Missing coverage for rotation scenarios on mobile

💡 Suggestions

1. Fix Memory Leak

// In terminal.ts, add class property:
private mobileResizeTimeout: NodeJS.Timeout | null = null;

// In setupResize():
if (this.isMobile) {
  logger.debug('[Terminal] Mobile detected - scheduling initial resize in 200ms');
  this.mobileResizeTimeout = setTimeout(() => {
    if (!this.terminal || !this.container) {
      logger.warn('[Terminal] Mobile resize aborted - terminal not ready');
      return;
    }
    logger.debug('[Terminal] Mobile: Executing initial resize');
    this.fitTerminal('initial-mobile-only');
    logger.debug('[Terminal] Mobile: Initial width set, future WIDTH resizes blocked');
  }, 200);
}

// In disconnectedCallback():
if (this.mobileResizeTimeout) {
  clearTimeout(this.mobileResizeTimeout);
  this.mobileResizeTimeout = null;
}

2. Extract Constants

// At top of terminal.ts
private static readonly MOBILE_RESIZE_DELAY_MS = 200;
private static readonly RESIZE_DEBOUNCE_MS = 250; // for terminal-lifecycle-manager

3. Cache Mobile Detection

private cachedIsMobile: boolean | null = null;

private detectMobile(): boolean {
  if (this.cachedIsMobile !== null) {
    return this.cachedIsMobile;
  }
  
  this.cachedIsMobile = (
    /iPhone|iPad|iPod|Android/i.test(navigator.userAgent) ||
    (navigator.maxTouchPoints !== undefined && navigator.maxTouchPoints > 1)
  );
  
  return this.cachedIsMobile;
}

4. Consider Manual Resize Option

For mobile users who need to adjust after orientation changes, consider adding a button or gesture to manually trigger a resize.

🎯 Priority Items

1. Fix memory leak: Store and cleanup the mobile resize timeout to prevent errors on component disposal
2. Define constants: Replace magic numbers (200ms, 250ms) with named constants
3. Add error handling: Ensure terminal is ready before the mobile resize executes
4. Add mobile-specific tests: Test the 200ms delay behavior and width-blocking logic
5. Document height resize behavior: The code allows height changes on mobile for keyboard events, but this isn't clearly documented in the PR description

Overall, this is a solid pragmatic fix that should significantly improve mobile stability by eliminating the resize loop issue. The simplification by removing ResizeCoordinator is a good architectural decision. The main concerns are around the memory leak and missing test coverage for the mobile-specific behavior.

Reviewed commit: 8e3d2de
Files changed: 50