Skip to content

fix: show hover in notebook when kernal switches#3633

Open
NarxPal wants to merge 3 commits into
facebook:mainfrom
NarxPal:fix-issue-3598
Open

fix: show hover in notebook when kernal switches#3633
NarxPal wants to merge 3 commits into
facebook:mainfrom
NarxPal:fix-issue-3598

Conversation

@NarxPal
Copy link
Copy Markdown
Contributor

@NarxPal NarxPal commented Jun 1, 2026

Summary

Cell content metdata was being updated when there are changes made to cells inside the notebook. LspEvent's didChange event runs the notebook_document_did_change fn which earlier updated the cell metdata of notebook only when there are changes made to the cell and since kernel switching doesn't cause any cell changes, the existing cells didn't get inserted into notebookDocument struct's cell field. this caused hover to stop working on the existing cells after kernel switching.

Fixes #3598

Test Plan

  • Manually verified in VS Code using a local Pyrefly build from this branch.
  • created a test.ipynb file, created new cell in it.
  • wrote x=5 , and switched kernel and hovered over x

@github-actions

This comment has been minimized.

@kinto0 kinto0 self-assigned this Jun 1, 2026
Copy link
Copy Markdown
Contributor

@kinto0 kinto0 left a comment

Choose a reason for hiding this comment

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

thanks!

is there any way to test this behavior with a lsp_interaction test that failed before your changes?

Comment thread pyrefly/lib/lsp/non_wasm/server.rs Outdated
@github-actions github-actions Bot added size/s and removed size/xs labels Jun 2, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@NarxPal
Copy link
Copy Markdown
Contributor Author

NarxPal commented Jun 2, 2026

previously test_notebook_hover_basic fn was not handling the hover over cell_content after kernel change, so i added that step in another fn called test_notebook_hover_after_kernel_change and since kernel change causes an lsp didChange event we use the change_notebook fn to stimulate the behaviour of kernel change.

@meta-codesync
Copy link
Copy Markdown
Contributor

meta-codesync Bot commented Jun 4, 2026

@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D107533803.

Copy link
Copy Markdown
Contributor

@kinto0 kinto0 left a comment

Choose a reason for hiding this comment

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

thank you! clean fix with great test coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[VSCode][ipynb] Changing kernels causes hover to stop working

2 participants