-
Notifications
You must be signed in to change notification settings - Fork 119
Positron nb focus on open #9805
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
Conversation
E2E Tests 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where Positron notebooks didn't automatically focus a cell when opened, requiring users to click before keyboard navigation would work. The fix replaces the NoSelection
state with a NoCells
state in the selection state machine and adds an invariant to ensure at least one cell is always selected when cells exist in the notebook.
Key changes:
- Refactored the selection state machine with comprehensive documentation and transition validation
- Added automatic cell selection when notebooks are opened
- Enhanced test coverage with new focus and selection behavior tests
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts |
Core fix: refactored state machine with NoCells state and invariant enforcement for automatic selection |
test/e2e/tests/notebook/notebook-focus-and-selection.test.ts |
Added comprehensive tests for initial focus behavior and keyboard navigation without clicks |
test/e2e/infra/test-runner/test-tags.ts |
Added POSITRON_NOTEBOOKS test tag for categorizing Positron-specific notebook tests |
Multiple test files | Updated test descriptions to include the new POSITRON_NOTEBOOKS tag for proper test organization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from test perspective! As mentioned in a comment, I'm going to do a pass through of all the positron nb e2e test code and will likely make some infra optimizations. But... can you also tag this PR for @:win
and re-run? Basically, anytime we add a new test we def want to make sure it passes there as well... because well... Windows. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM! A couple questions about the spec
src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts
Outdated
Show resolved
Hide resolved
…it state selection 'selectedCell' in favor of always using selected with type descrimination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Test failure is a data explorer test that seems flakey. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
Addresses #9707.
When opening a Positron notebook, the editor was not properly focusing a cell, requiring users to click a cell before keyboard shortcuts like
j
andk
for navigation would work. This PR fixes the issue by replacing theno selection
state with ano-cells
state in the selection state machine and adding an invariant to ensure at least one cell is always selected when the notebook contains cells.I took this opportunity to refactor the selection state machine too as it had fallen out of good state-machine form after piecemeal edits for a while. Docs have been added at the top and we now verify transition correctness and throw errors in dev mode to hopefully catch bugs (It already caught one while making this PR.)
Release Notes
New Features
Bug Fixes
QA Notes
@:positron-notebooks
The test suite includes comprehensive coverage of the fix:
To manually test: