Skip to content

Conversation

@jankuca
Copy link
Member

@jankuca jankuca commented Oct 21, 2025

Summary by CodeRabbit

  • New Features
    • Added a command to execute the currently focused notebook cell.
    • Added a keyboard shortcut to run the focused cell: Cmd+Enter (Mac) / Ctrl+Enter (Windows/Linux).
    • Shortcut active only when the notebook editor is focused and disabled while find/replace inputs are focused.
    • Added a localized command title "Run Focused Cell" for UI display.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

📝 Walkthrough

Walkthrough

Adds a new notebook command and keybinding to run the focused cell. Exports command constant jupyter.notebookeditor.runfocusedcell, registers the command in package.json with enablement notebookEditorFocused, adds a localization entry, extends the command-argument mapping, and implements a handler that finds the active notebook editor, validates the selection range, and executes the cell at the selection start (range start..start+1). Keybinding: cmd+Enter (mac) / ctrl+Enter (win/linux) with when notebookEditorFocused && !findInputFocussed && !replaceInputFocussed.

Sequence Diagram

sequenceDiagram
    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
Loading

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "feat: add cmd+enter command for running focused block" directly describes the main change in this PR. The implementation adds a keyboard shortcut (cmd+enter/ctrl+enter) and associates it with a new command to execute the focused cell in a notebook, exactly as the title states. The title is concise, specific about the shortcut and action, and clear enough for someone scanning history to understand the feature being added.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cd28a93 and b993356.

📒 Files selected for processing (4)
  • package.json (2 hunks)
  • package.nls.json (1 hunks)
  • src/commands.ts (1 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 .ts files (not suffixed with .node or .web)

Files:

  • src/platform/common/constants.ts
  • src/commands.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{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/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
⏰ 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)
  • GitHub Check: Build & Package Extension
  • GitHub Check: Build & Test
  • GitHub Check: Lint & Format
🔇 Additional comments (5)
package.nls.json (1)

95-95: LGTM!

Localization entry correctly formatted and matches the command key pattern.

package.json (2)

73-81: LGTM!

Keybinding properly configured with platform-specific keys and appropriate context guards.


555-560: LGTM!

Command declaration correctly uses localization reference and appropriate enablement context.

src/platform/common/constants.ts (1)

192-192: LGTM!

Command constant properly defined and consistent with command ID in package.json.

src/commands.ts (1)

61-61: LGTM!

Command-argument mapping correctly defined with empty argument list, consistent with similar commands.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72%. Comparing base (192c29c) to head (b993356).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

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:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 614334b and 8edb01d.

📒 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 .ts files (not suffixed with .node or .web)

Files:

  • src/platform/common/constants.ts
  • src/notebooks/notebookCommandListener.ts
  • src/commands.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{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/notebooks/notebookCommandListener.ts
  • src/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.start to range.start + 1 executes 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.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 22, 2025
@jankuca jankuca marked this pull request as ready for review October 22, 2025 15:21
@jankuca jankuca enabled auto-merge (squash) October 22, 2025 15:21
@jankuca
Copy link
Member Author

jankuca commented Oct 23, 2025

@saltenasl You were right about the shortcut not working. Coderabbit suggested a change that broke it. Fixed now.

@deepnote deepnote deleted a comment from linear bot Oct 23, 2025
@jankuca
Copy link
Member Author

jankuca commented Oct 24, 2025

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 24, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9a4152b and cd28a93.

📒 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 .ts files (not suffixed with .node or .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
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{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 when condition.

Copy link
Member

@saltenasl saltenasl left a 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?

@jankuca jankuca requested a review from a team as a code owner October 29, 2025 12:22
@jankuca
Copy link
Member Author

jankuca commented Oct 29, 2025

@Artmann says it works fine for him!

@jankuca jankuca dismissed saltenasl’s stale review October 29, 2025 12:28

Asked Chris instead. It probably is related to your custom mapping:(

@jankuca jankuca merged commit c85f4fb into main Oct 29, 2025
12 of 13 checks passed
@jankuca jankuca deleted the jk/feat/cmd-enter-run-block branch October 29, 2025 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants