fix(ui): fix toolbar layout on file load #31
Conversation
Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
andyjakubowski
requested review from
Artmann,
jamesbhobbs and
saltenasl
as code owners
GRN-4946 Fix toolbar layout on file load
Ensures overflowing toolbar items get packed into an overflow popup toolbar. |
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User / App
participant NP as NotebookPicker (Widget)
participant P as Parent Widget
participant ML as MessageLoop
U->>NP: attach to DOM
activate NP
NP->>NP: onAfterAttach()
NP->>ML: postMessage(P, ResizeMessage.UnknownSize)
deactivate NP
activate ML
ML->>P: deliver ResizeMessage.UnknownSize
deactivate ML
activate P
P->>P: perform resize/layout pass
deactivate P
note right of NP #DDEBF7: Select uses width: 120px
sequenceDiagram
autonumber
actor T as Test (NotebookPicker.spec.ts)
participant DOM as DOM
participant NP as NotebookPicker
participant Sim as simulate-event
T->>NP: instantiate
T->>DOM: attach NP
T->>NP: wait framePromise / rendering
activate NP
NP->>DOM: render select/options
deactivate NP
T->>Sim: dispatch change event
Sim->>NP: user selection
NP->>NP: call fromJSON (if selection valid)
T->>T: assert DOM & fromJSON calls
Possibly related PRs
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 (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-09-03T12:54:35.368ZApplied to files:
📚 Learning: 2025-10-09T11:21:57.494ZApplied to files:
🧬 Code graph analysis (1)src/__tests__/NotebookPicker.spec.ts (1)
🪛 ast-grep (0.39.6)src/__tests__/NotebookPicker.spec.ts[warning] 49-49: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) [warning] 99-99: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first. (dom-content-modification) ⏰ 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)
🔇 Additional comments (5)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
===========================================
+ Coverage 37.29% 50.20% +12.90%
===========================================
Files 13 13
Lines 244 249 +5
Branches 26 26
===========================================
+ Hits 91 125 +34
+ Misses 153 119 -34
- Partials 0 5 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/NotebookPicker.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookSelector.ts : deepnoteNotebookSelector.ts implements UI selection logic for Deepnote notebooks
Applied to files:
src/components/NotebookPicker.tsx
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/notebookIPyWidgetCoordinator.ts : NotebookIPyWidgetCoordinator should manage per-notebook lifecycle, controller selection, webview attachment, and creation of CommonMessageCoordinator instances
Applied to files:
src/components/NotebookPicker.tsx
⏰ 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: Spell Check
- GitHub Check: check_release
- GitHub Check: build
🔇 Additional comments (2)
src/components/NotebookPicker.tsx (2)
6-7: LGTM!Imports support the new lifecycle hook.
93-93: LGTM!Fixed width stabilizes layout. Ellipsis handles overflow for long names.
| protected onAfterAttach(msg: Message): void { | ||
| super.onAfterAttach(msg); | ||
| requestAnimationFrame(() => { | ||
| MessageLoop.sendMessage(this.parent!, Widget.ResizeMessage.UnknownSize); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard against null parent.
this.parent! will throw if no parent exists at attach time. Add a null check.
protected onAfterAttach(msg: Message): void {
super.onAfterAttach(msg);
requestAnimationFrame(() => {
- MessageLoop.sendMessage(this.parent!, Widget.ResizeMessage.UnknownSize);
+ if (this.parent) {
+ MessageLoop.sendMessage(this.parent, Widget.ResizeMessage.UnknownSize);
+ }
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected onAfterAttach(msg: Message): void { | |
| super.onAfterAttach(msg); | |
| requestAnimationFrame(() => { | |
| MessageLoop.sendMessage(this.parent!, Widget.ResizeMessage.UnknownSize); | |
| }); | |
| } | |
| protected onAfterAttach(msg: Message): void { | |
| super.onAfterAttach(msg); | |
| requestAnimationFrame(() => { | |
| if (this.parent) { | |
| MessageLoop.sendMessage(this.parent, Widget.ResizeMessage.UnknownSize); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/components/NotebookPicker.tsx around lines 68 to 73, the code uses a
non-null assertion this.parent! when sending a resize message which will throw
if parent is null; update the method to check that this.parent is non-null
before calling MessageLoop.sendMessage (e.g., if (this.parent) {
MessageLoop.sendMessage(this.parent, Widget.ResizeMessage.UnknownSize); }) and
remove the non-null assertion so the resize is only sent when a parent exists.
Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
jest.config.js(1 hunks)src/__tests__/NotebookPicker.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage
Applied to files:
src/__tests__/NotebookPicker.spec.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookSelector.ts : deepnoteNotebookSelector.ts implements UI selection logic for Deepnote notebooks
Applied to files:
src/__tests__/NotebookPicker.spec.ts
🪛 ast-grep (0.39.6)
src/__tests__/NotebookPicker.spec.ts
[warning] 49-49: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 99-99: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 GitHub Actions: Build
src/__tests__/NotebookPicker.spec.ts
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
🪛 GitHub Actions: CI
src/__tests__/NotebookPicker.spec.ts
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
⏰ 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: Spell Check
- GitHub Check: check_release
🔇 Additional comments (1)
src/__tests__/NotebookPicker.spec.ts (1)
42-46: Override of onAfterAttach in tests is appropriate: in production NotebookPicker is added to JupyterLab’s shell via app.shell.add, so this.parent is never null.
Signed-off-by: Andy Jakubowski <hello@andyjakubowski.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/__tests__/NotebookPicker.spec.ts (2)
1-1: Run Prettier to fix formatting.Pipeline detected code style issues. Run the formatter to ensure consistency.
#!/bin/bash npx prettier --write src/__tests__/NotebookPicker.spec.ts
88-94: Use consistent event simulation pattern.Direct property assignment plus
simulatewithout event details differs from other tests. Use the same pattern for clarity.Apply this diff:
it('should update UI after selection', async () => { const select = document.querySelector('select') as HTMLSelectElement; - select.value = 'nb2'; - simulate(select, 'change'); + simulate(select, 'change', { target: { value: 'nb2' } }); await framePromise(); expect(select.value).toBe('nb2'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
cspell.json(1 hunks)src/__tests__/NotebookPicker.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage
Applied to files:
src/__tests__/NotebookPicker.spec.ts
🧬 Code graph analysis (1)
src/__tests__/NotebookPicker.spec.ts (1)
src/components/NotebookPicker.tsx (1)
NotebookPicker(9-111)
🪛 ast-grep (0.39.6)
src/__tests__/NotebookPicker.spec.ts
[warning] 49-49: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 99-99: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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: Spell Check
- GitHub Check: build
- GitHub Check: check_release
🔇 Additional comments (4)
cspell.json (1)
42-42: LGTM!Configuration change aligns with
exenv-es6package handling in jest config.src/__tests__/NotebookPicker.spec.ts (3)
43-44: Good test pattern.Overriding
onAfterAttachavoids null parent errors in test environment while still validating component behavior.
54-60: LGTM!Test correctly validates select element rendering and option count.
96-110: LGTM!Test correctly handles empty metadata edge case by validating fallback placeholder option.
Ensures overflowing toolbar items get packed into an overflow popup toolbar.
Signed-off-by: Andy Jakubowski hello@andyjakubowski.com
Summary by CodeRabbit
Bug Fixes
Style
Tests
Chores