Skip to content

Conversation

@Artmann
Copy link
Contributor

@Artmann Artmann commented Dec 15, 2025

Fixes #218

Summary by CodeRabbit

  • New Features

    • Notebook cells now auto-center in the viewport after navigating dataframe pages or changing page size.
  • Tests

    • Test mock objects updated to include an optional prompt field to support prompt-related test scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

📝 Walkthrough

Walkthrough

The dataframe controller now reveals the affected notebook cell after re-executing it from goToPage and selectPageSize. After re-execution the code builds a NotebookRange for the cell and calls editor.revealRange(..., NotebookEditorRevealType.InCenterIfOutsideViewport) to center the cell if it's outside the viewport. Tests updated: MockQuickPick gained a new optional prompt: string | undefined property in two test helper files.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant DataframeController
  participant NotebookKernel as Kernel/Executor
  participant NotebookEditor as Editor

  User->>DataframeController: change page / change page size
  DataframeController->>Kernel/Executor: re-execute target cell
  Kernel/Executor-->>DataframeController: execution complete
  DataframeController->>Editor: create NotebookRange for cell
  DataframeController->>Editor: editor.revealRange(range, InCenterIfOutsideViewport)
  Editor-->>User: cell is centered/visible
Loading

Pre-merge checks

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive Test file changes adding prompt property to MockQuickPick appear unrelated to #218's scroll position objective; unclear connection to dataframe reveal functionality. Clarify why MockQuickPick changes in test files are necessary for the dataframe reveal feature, or remove if genuinely out-of-scope.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title directly addresses the main change: revealing the dataframe in view after size reduction, matching the changeset's focus on calling revealRange in goToPage and selectPageSize handlers.
Linked Issues check ✅ Passed Changes implement the core requirement from #218 by revealing notebook cells after pagination/size changes, preserving dataframe visibility when row count changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a7d830e and 84a8666.

📒 Files selected for processing (2)
  • src/test/datascience/mockQuickPick.ts (1 hunks)
  • src/test/datascience/notebook/helper.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

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

**/*.{ts,tsx}: Use l10n.t() for all user-facing strings in TypeScript files
Use typed error classes from src/platform/errors/ instead of generic errors
Use ILogger service instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationToken

Order method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order

Files:

  • src/test/datascience/mockQuickPick.ts
  • src/test/datascience/notebook/helper.ts
**/*.ts

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS run npm run format-fix before committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Use npm run format-fix to auto-fix TypeScript formatting issues before committing
Use npm run lint to check for linter issues in TypeScript files and attempt to fix before committing

Files:

  • src/test/datascience/mockQuickPick.ts
  • src/test/datascience/notebook/helper.ts
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
Use Uri.joinPath() for constructing file paths instead of string concatenation with / to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., use import { generateUuid } from '../platform/common/uuid' instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports

Files:

  • src/test/datascience/mockQuickPick.ts
  • src/test/datascience/notebook/helper.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: Check Licenses
  • GitHub Check: Build & Test
  • GitHub Check: Spell Check
🔇 Additional comments (2)
src/test/datascience/mockQuickPick.ts (1)

9-9: LGTM - Mock API extended correctly.

Property added to match VS Code's QuickPick interface.

src/test/datascience/notebook/helper.ts (1)

1424-1424: LGTM - Consistent with other mock.


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

@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0%. Comparing base (e1ec0a7) to head (84a8666).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #251   +/-   ##
===========================
===========================
🚀 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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e1ec0a7 and a7d830e.

📒 Files selected for processing (1)
  • src/webviews/extension-side/dataframe/dataframeController.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

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

**/*.{ts,tsx}: Use l10n.t() for all user-facing strings in TypeScript files
Use typed error classes from src/platform/errors/ instead of generic errors
Use ILogger service instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationToken

Order method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order

Files:

  • src/webviews/extension-side/dataframe/dataframeController.ts
**/*.ts

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS run npm run format-fix before committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Use npm run format-fix to auto-fix TypeScript formatting issues before committing
Use npm run lint to check for linter issues in TypeScript files and attempt to fix before committing

Files:

  • src/webviews/extension-side/dataframe/dataframeController.ts
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
Use Uri.joinPath() for constructing file paths instead of string concatenation with / to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., use import { generateUuid } from '../platform/common/uuid' instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports

Files:

  • src/webviews/extension-side/dataframe/dataframeController.ts
🧬 Code graph analysis (1)
src/webviews/extension-side/dataframe/dataframeController.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • NotebookRange (2437-2467)
⏰ 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). (2)
  • GitHub Check: Package Lock Drift Check
  • GitHub Check: Audit - All
🔇 Additional comments (1)
src/webviews/extension-side/dataframe/dataframeController.ts (1)

7-8: New VS Code notebook imports look appropriate

NotebookEditorRevealType and NotebookRange are used correctly below to reveal the executing cell; no issues from a typing or usage standpoint.

Please confirm your minimum VS Code engine version supports these APIs (they’re fairly recent in the notebook API surface).

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 15, 2025
@Artmann Artmann marked this pull request as ready for review December 15, 2025 15:08
@Artmann Artmann requested a review from a team as a code owner December 15, 2025 15:08
Copy link
Contributor

@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.

🚀

@Artmann Artmann merged commit 71abe5f into main Dec 18, 2025
13 checks passed
@Artmann Artmann deleted the chris/218-keep-dataframe-in-view branch December 18, 2025 12:36
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.

Changing number of rows displayed on dataframe output loses scroll position

3 participants