Open the help in the down area (pager) #7713
Conversation
|
|
|
So, I think this PR should be ready for a first round of reviews. As a quick summary, it brings back the behavior of the pager just like the classic notebook. This has been missing since the early Jupyter Notebook 7 releases (released a bit more than two years ago), and there's been some feedback that this was still missing. I've also added some UI tests to cover some cases, and also added a new page in the docs so that folks can opt out of this new behavior and return to the JupyterLab behavior if they want to. If folks would like to try it, that would be great, let me know what you think. I'm also happy to iterate on a different approach if you think the current one could be improved. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a pager widget that displays help documentation in the down area (inspector panel) similar to classic notebook behavior, providing an alternative to JupyterLab's inline output approach. Users can toggle this behavior through settings to maintain compatibility with either classic notebook or JupyterLab workflows.
- Implements a new pager plugin that intercepts kernel messages with pager payloads and displays them in the inspector panel
- Adds configuration option to switch between down-area display and inline cell output behavior
- Updates shell layout to support inspector placement in the down area
Reviewed Changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/notebook-extension/src/index.ts | Core pager plugin implementation with kernel message handling and inspector integration |
| packages/notebook-extension/schema/pager.json | Settings schema for configuring pager behavior |
| packages/notebook-extension/package.json | Added inspector dependency |
| packages/application/src/shell.ts | Enhanced down panel activation to support tab switching |
| packages/application-extension/schema/shell.json | Updated shell configuration to place Inspector in down area by default |
| ui-tests/test/notebook.spec.ts | Added test to verify pager opens in down area with question mark syntax |
| docs/source/user-documentation.md | Added pager documentation reference |
| docs/source/pager.md | Complete user documentation for pager functionality |
| app/package.json | Added inspector extension dependencies and configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
|
||
| // Listen for parent disposal. | ||
| panel.disposed.connect(() => { | ||
| inspectionHandler.dispose(); |
There was a problem hiding this comment.
All inspection handlers would not be properly disposed of because inspectionHandler is a global variable (L901) and each panel.disposed.connect() callback (L967-969) references this global variable, not the specific handler instance created for that panel, resulting in a memory leak.
Test by opening multiple notebooks then closing any other than last created notebook, note that only last handler gets disposed of.
| app.shell.currentChanged?.connect((_, args) => setSource(args.newValue)); | ||
| void app.restored.then(() => setSource(app.shell.currentWidget)); |
There was a problem hiding this comment.
setSource() can be called before inspectionHandler is initialized (L932), for example during app startup, causing undefined access when app.shell events fire before any notebook is opened.
To test, add logging inspectionHandler at L975, launch notebook server without opening any notebooks, refresh the browser page while monitoring the logs, see inspectionHandler is undefined.
While this doesn't break functionality (opening a notebook later initializes the handler), it puts the inspector in an invalid state during startup and could cause issues if other components try to use it during that window.
| (item) => item.source !== 'page' | ||
| ); | ||
| if (content.payload.length === 0) { | ||
| // If no other payloads remain, delete the payload array from the content. | ||
| delete content.payload; | ||
| } |
There was a problem hiding this comment.
This mutates kernel messages, other extensions / components would receive the modified version missing pager payloads. Based on my research JupyterLab's codebase as of now does not mutate kernel messages in other places. For example, @jupyterlab/outputarea copies the message.
Would it be possible to accomplish the same without mutating the kernel messages? For example watch cell output model changes to detect pager payloads during cell execution and route them to the down area before normal output rendering?
Fixes #6692
Fixes #1135
To offer a behavior similar to the classic notebook:
notebook-down-pager.mp4
The idea would be to have this behavior by default in Notebook 7.5, with a setting to disable it and have the pager data be displayed in the cell output like in JupyterLab:
notebook-pager-disabled.mp4
Example with
xeus-cppshowing atext/htmlbundle:notebook-xeus-cpp-pager.mp4