Skip to content

fix: tab and esc are properly reserved #7268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

jpoly1219
Copy link
Collaborator

@jpoly1219 jpoly1219 commented Aug 21, 2025

Description

Closes CON-3594.

Properly handle reservation and freeing of tab and esc. More user tests on different machines is needed once it's merged.

AI Code Review

  • Team members only: AI review runs automatically when PR is opened or marked ready for review
  • Team members can also trigger a review by commenting @continue-general-review or @continue-detailed-review

Checklist

  • I've read the contributing guide
  • The relevant docs, if any, have been updated or created
  • The relevant tests, if any, have been updated or created

Screen recording or screenshot

[ When applicable, please include a short screen recording or screenshot - this makes it much easier for us as contributors to review and understand your changes. See this PR as a good example. ]

Tests

[ What tests were added or updated to ensure the changes work as expected? ]


Summary by cubic

Ensure Tab and Esc are correctly reserved and released for the Next Edit window to prevent conflicts and stuck keys across editors. Addresses CON-3594 by serializing key reservation and adding robust error handling and cleanup.

  • Bug Fixes
    • Added internal state machine (free/reserved/transitioning) with promise gating to prevent race conditions.
    • Wrapped setContext in setKeyReservation; no-ops when already in desired state and waits for in-flight transitions.
    • On reserve failure, hides Next Edit windows; on hide/dispose, always frees keys with error logging.

@jpoly1219 jpoly1219 requested a review from a team as a code owner August 21, 2025 00:05
@jpoly1219 jpoly1219 requested review from RomneyDa and removed request for a team August 21, 2025 00:05
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Aug 21, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds state management for key reservation (Tab/Esc) in the NextEditWindowManager to prevent race conditions and ensure proper cleanup. The implementation uses a state machine approach with proper error handling and async coordination. Overall the changes are well-structured, but there's a typo in the equality check and some cleanup needed for commented code.


💡 To request a new detailed review, comment @continue-detailed-review

Copy link

Code Review Summary

✅ Strengths

  • State Management: Excellent introduction of proper state management for key reservation with states (free, reserved, transitioning) that prevent race conditions
  • Error Handling: Comprehensive error handling added throughout the async operations with proper logging
  • Async Safety: Implementation correctly handles inflight transitions and prevents concurrent operations
  • Code Structure: Clean separation of concerns with setKeyReservation and performKeyReservation methods
  • Defensive Programming: Multiple safety checks including early returns for redundant operations and proper cleanup in error cases

⚠️ Issues Found

Medium

  • Dead Code: The commented-out code in reserveTabAndEsc() and freeTabAndEsc() methods should be removed rather than left as comments. This creates confusion about which implementation is active.

  • Error Recovery: When key reservation fails in showNextEditWindow(), the method returns early but doesn't provide any user feedback. Consider adding a notification or status bar message to inform the user that the feature is temporarily unavailable.

  • Dispose Method Inconsistency: The dispose() method uses a fire-and-forget pattern for freeing keys, which could lead to silent failures. While error handling is present, the approach differs from the more robust error handling used elsewhere.

Low

  • Typo in Comparison: Line 181 uses == instead of === for the equality check. While functionally equivalent for strings, it's inconsistent with the strict equality used elsewhere in the code.

  • Console Logging: The code uses console.error for error logging. Consider using VS Code's output channel or a proper logging service for better error tracking and debugging.

💡 Suggestions

  • Testing: Add unit tests for the new state management logic, particularly for concurrent operation scenarios and error cases
  • Documentation: Add JSDoc comments for the new private methods explaining their purpose and the state machine logic
  • Timeout Configuration: Consider making the key reservation operation timeout configurable for better control in slow environments
  • Telemetry: Consider adding telemetry for key reservation failures to track how often users encounter this issue

🚀 Overall Assessment

APPROVE

This is a well-implemented solution to prevent race conditions in key reservation. The state machine approach is robust and handles edge cases properly. The code maintains backward compatibility while significantly improving reliability. The minor issues identified don't impact functionality and can be addressed in follow-up PRs.


💡 To request a new review, comment @continue-general-review

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

1 participant