-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add cmd+enter command for running focused block #102
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
📝 WalkthroughWalkthroughAdds a new notebook command and keybinding to run the focused cell. Exports command constant Sequence DiagramsequenceDiagram
actor User
participant Keybinding as package.json Keybinding
participant Registry as Command Registry
participant Listener as NotebookCommandListener
participant Editor as Active Notebook Editor
participant API as Notebook API
Note over Keybinding,Registry #f0f8ff: New keybinding -> jupyter.notebookeditor.runfocusedcell
User->>Keybinding: Press cmd/ctrl+Enter
Keybinding->>Registry: Trigger jupyter.notebookeditor.runfocusedcell
Registry->>Listener: Invoke handler
Listener->>Editor: Request active editor & selection
Editor-->>Listener: Return editor + selection range
alt valid selection
Listener->>API: Execute cell at selection start (start..start+1)
API-->>Listener: Execution result (ignored)
else missing editor or invalid range
Listener-->>Registry: No-op / ignore
end
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (3)**/!(*.node|*.web).ts📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/platform/**/*.ts📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (5)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
=====================================
Coverage 72% 72%
=====================================
Files 539 539
Lines 41267 41267
Branches 5012 5012
=====================================
Hits 30001 30001
Misses 9592 9592
Partials 1674 1674 🚀 New features to boost your workflow:
|
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
package.json(2 hunks)src/commands.ts(1 hunks)src/notebooks/notebookCommandListener.ts(2 hunks)src/platform/common/constants.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/platform/common/constants.tssrc/notebooks/notebookCommandListener.tssrc/commands.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/platform/common/constants.tssrc/notebooks/notebookCommandListener.tssrc/commands.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/common/constants.ts
🧬 Code graph analysis (1)
src/notebooks/notebookCommandListener.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (2)
range(602-604)range(606-611)src/test/datascience/mockDocument.ts (1)
range(40-42)
🔇 Additional comments (4)
src/commands.ts (1)
61-61: LGTM!Command mapping correctly specifies empty argument list for the new command.
src/notebooks/notebookCommandListener.ts (2)
71-73: LGTM!Command registration follows the established pattern in this file.
121-140: LGTM!Implementation correctly executes the cell at the start of the first selection. Early returns handle missing editor/range safely. Using
range.starttorange.start + 1executes exactly one cell, which aligns with "run focused cell" behavior.src/platform/common/constants.ts (1)
192-192: LGTM!Constant follows naming conventions and is correctly placed among related notebook editor commands.
This reverts commit 4e5470b.
|
@saltenasl You were right about the shortcut not working. Coderabbit suggested a change that broke it. Fixed now. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
package.json(2 hunks)src/platform/common/constants.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/platform/common/constants.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/platform/common/constants.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/common/constants.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build & Package Extension
- GitHub Check: Lint & Format
- GitHub Check: Build & Test
- GitHub Check: copilot-setup-steps
🔇 Additional comments (2)
src/platform/common/constants.ts (1)
192-192: LGTM!Command constant follows existing naming conventions and matches the command ID declared in package.json.
package.json (1)
489-494: LGTM!Command declaration properly uses localization and the enablement clause is consistent with the keybinding's
whencondition.
saltenasl
left a comment
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.
Screen.Recording.2025-10-28.at.21.35.36.mov
its still producing new lines, but it may be related to my shortcuts as well, can you get someone else to review and check if its working for them instead?
|
@Artmann says it works fine for him! |
Asked Chris instead. It probably is related to your custom mapping:(
Summary by CodeRabbit