feat: notebook support keep scroll position when tab switched #4670
Conversation
|
/next |
HellowVirgil
force-pushed
the
feat/keep-notebook-switch-position
branch
from
cf5911c to
bdfd315
Compare
|
/next |
Walkthrough新增 LibroStateManager 服务及其 DI 提供器，用于按 URI 持久化与恢复笔记本视图状态（滚动位置、最后活跃时间）；在贡献点注册提供者并在 libro 视图中绑定保存/恢复逻辑与防抖滚动监听器（500ms）。 Changes
Sequence Diagram(s)sequenceDiagram
participant View as LibroView
participant Manager as LibroStateManager
participant Storage as StorageProvider
participant Cache as StateCache
Note over View,Manager: 初始化/加载完成时尝试恢复状态
View->>Manager: restoreState(uri, libroView)
Manager->>Cache: 查找 uri 对应状态
alt 有缓存状态
Manager->>View: 将 scrollTop 应用到 DOM
Manager-->>View: 返回 true
else 无缓存
Manager-->>View: 返回 false
end
Note over View,Manager: 滚动事件（防抖 500ms）触发保存
View->>View: 监听 scroll（500ms 防抖）
View->>Manager: saveState(uri, libroView)
Manager->>View: 从 LIBRO_SCROLLER_SELECTOR 读取 scrollTop
Manager->>Cache: 更新内存缓存
Manager->>Storage: saveToStorage() 持久化整个缓存
Storage->>Storage: 写入持久化存储（namespace: libro-notebook-storage）
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 分钟 需要额外关注：
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/notebook/src/browser/libro.view.tsx (1)
48-98: 在 effect 中正确清理 debounce 防抖的滚动监听，避免事件泄漏和定时器触发原始审查评论正确识别了问题。验证确认：
DOM 引用不匹配：effect 依赖数组中缺少
libroView，导致清理函数中使用的libroView?.container?.current总是初始值（通常为undefined），而绑定时使用的是.then()回调中的libro对象防抖回调未取消：推荐做法是在 cleanup 中既调用
removeEventListener(同一引用)，也调用debounced.cancel()以取消任何待执行的延迟调用，防止卸载后触发副作用，但当前代码中没有调用handleScroll.cancel()建议修复方案（保持原评论的 diff）完全符合最佳实践。需实施该修改以确保：
- 绑定和解绑使用同一 DOM 元素引用
- 组件卸载时取消待执行的防抖回调
🧹 Nitpick comments (1)
packages/notebook/src/browser/libro-state-manager.ts (1)
6-17: 考虑优化 StorageProvider 实例复用模式以提升效率当前实现在
saveToStorage()（第 48-59 行）中每次调用都会通过this.storageProvider(new URI('libro-notebook-storage'))创建新的存储实例。由于saveToStorage()被saveState()（第 82 行）和clearState()（第 107 行）多次调用，这会导致频繁的实例创建开销。根据 OpenSumi 存储最佳实践，不建议为每次读/写都新建同一命名空间的存储实例，推荐在模块/服务生命周期内创建一次（或按必需的更大粒度）并复用该实例，以减少创建/初始化开销并保持一致性。
建议方案：在
LibroStateManager中添加一个storage成员变量，在构造函数或loadFromStorage()初始化时创建一次，后续在saveToStorage()和restoreState()等方法中复用该实例，避免重复创建。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/notebook/src/browser/libro-state-manager.ts(1 hunks)packages/notebook/src/browser/libro-state.ts(1 hunks)packages/notebook/src/browser/libro.contribution.tsx(2 hunks)packages/notebook/src/browser/libro.view.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-12T08:27:06.038Z
Learnt from: ensorrow
Repo: opensumi/core PR: 3945
File: packages/notebook/src/browser/libro.contribution.tsx:170-174
Timestamp: 2024-11-12T08:27:06.038Z
Learning: 在文件 'packages/notebook/src/browser/libro.contribution.tsx' 中，notebook 图标已适配暗色和亮色主题，无需修改。
Applied to files:
packages/notebook/src/browser/libro.view.tsxpackages/notebook/src/browser/libro.contribution.tsx
📚 Learning: 2024-10-12T07:43:08.790Z
Learnt from: bytemain
Repo: opensumi/core PR: 4088
File: packages/ai-native/src/browser/widget/inline-stream-diff/live-preview.decoration.tsx:73-74
Timestamp: 2024-10-12T07:43:08.790Z
Learning: 在 `LivePreviewDiffDecorationModel` 类（位于 `packages/ai-native/src/browser/widget/inline-stream-diff/live-preview.decoration.tsx`）中，通过依赖注入（`Autowired`）从 module 的 `providers` 声明中获取的依赖（如 `_onPartialEditEvent`），其销毁逻辑由 `ClientApp` 的生命周期管理，不需要手动调用 `dispose()` 进行销毁。
Applied to files:
packages/notebook/src/browser/libro.view.tsxpackages/notebook/src/browser/libro.contribution.tsx
🧬 Code graph analysis (2)
packages/notebook/src/browser/libro.view.tsx (1)
packages/core-browser/src/react-hooks/injectable-hooks.tsx (1)
useInjectable(16-39)
packages/notebook/src/browser/libro.contribution.tsx (1)
packages/notebook/src/browser/libro-state.ts (1)
LibroStateModule(5-10)
⏰ 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). (8)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build-windows
🔇 Additional comments (3)
packages/notebook/src/browser/libro.contribution.tsx (1)
47-48: Libro 状态模块注入方式合理在
initialize中通过app.injector.addProviders(...LibroStateModule)注册LibroStateManager，配合useInjectable(LibroStateManager)的使用方式是符合 OpenSumi DI 模式的，不会影响现有逻辑，只是补充了状态管理能力，看起来没有问题。Also applies to: 136-138
packages/notebook/src/browser/libro-state.ts (1)
1-10: DI 模块定义简洁有效
LibroStateModule采用{ token: LibroStateManager, useClass: LibroStateManager }的 ClassProvider 方式注册，能满足通过useInjectable(LibroStateManager)获取单例状态管理器的需求，实现上很干净，没有明显问题。packages/notebook/src/browser/libro.view.tsx (1)
21-42: 状态保存 / 恢复整体思路清晰通过
useInjectable(LibroStateManager)注入服务，再用saveNotebookState与restoreNotebookState以 URI 为 key 读写视图状态，同时用isStateRestored作为防重入标志，并在首个 effect 和 “延迟 100ms” 的第二个 effect 中各尝试一次恢复，基本可以覆盖视图创建早期 DOM 未完全 ready 的情况，逻辑上是合理的。Also applies to: 100-108
|
/next |
1 similar comment
|
/next |
|
🎉 PR Next publish successful! 3.9.1-next-1764049322.0 |
|
/next |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/notebook/src/browser/libro-state-manager.ts (2)
22-23: DOM 查询与scrollTop访问建议做更严格的类型约束当前通过：
const libroViewContent = libroView.container.current?.querySelector(LibroStateManager.LIBRO_SCROLLER_SELECTOR); const scrollTop = libroViewContent?.scrollTop || 0;在 TS DOM 类型里，
querySelector返回Element | null，Element默认没有scrollTop属性，容易在严格类型设置下报错，也不利于后续重构。建议：
- 使用泛型或类型断言把结果限定为
HTMLElement：const libroViewContent = libroView.container.current?.querySelector<HTMLElement>( LibroStateManager.LIBRO_SCROLLER_SELECTOR, ); const state: INotebookViewState = { uri: uri.toString(), scrollTop: libroViewContent ? libroViewContent.scrollTop : 0, lastActiveTime: Date.now(), };
restoreState中同理，对libroViewContent做HTMLElement断言后再设置scrollTop。这样既避免类型警告，也更明确表达「这里一定是可滚动容器」。
Also applies to: 68-79, 95-100
64-66:isValidState校验字段与未来扩展的一致性建议目前
isValidState只校验uri和scrollTop类型，lastActiveTime未做校验；而INotebookViewState要求三个字段都存在。短期内不影响滚动恢复（只依赖
scrollTop），但如果后续会按lastActiveTime做淘汰或排序，建议提前把校验补上：private isValidState(state: any): state is INotebookViewState { return ( state && typeof state === 'object' && typeof state.uri === 'string' && typeof state.scrollTop === 'number' && typeof state.lastActiveTime === 'number' ); }同时
getAllStates直接返回缓存快照是 OK 的，如果后续要防御性编程，也可以按需浅拷贝状态对象。Also applies to: 75-80, 115-121
packages/notebook/src/browser/libro.view.tsx (1)
24-26:isStateRestored与 URI 关系的可预期性（可选改进）当前
isStateRestored只在成功恢复状态时置为true，但在以下情况下不会自动重置：
- 若未来有场景在同一
OpensumiLibroView实例上切换params[0].resource.uri（即uri变化），旧的isStateRestored状态会被复用，新 notebook 将不会再尝试恢复状态。如果组件在实际使用中每个 URI 都对应新的 React 实例，这个问题不会出现；但为了防御未来重用场景，可以考虑：
- 在
uri变化时重置isStateRestored与libroView，例如增加一个 effect：React.useEffect(() => { setIsStateRestored(false); }, [uri]);仅作为前瞻性防御，可按实际使用场景决定是否需要。
Also applies to: 35-42, 100-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/notebook/src/browser/libro-state-manager.ts(1 hunks)packages/notebook/src/browser/libro.view.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-12T08:27:06.038Z
Learnt from: ensorrow
Repo: opensumi/core PR: 3945
File: packages/notebook/src/browser/libro.contribution.tsx:170-174
Timestamp: 2024-11-12T08:27:06.038Z
Learning: 在文件 'packages/notebook/src/browser/libro.contribution.tsx' 中，notebook 图标已适配暗色和亮色主题，无需修改。
Applied to files:
packages/notebook/src/browser/libro.view.tsx
📚 Learning: 2024-10-12T07:43:08.790Z
Learnt from: bytemain
Repo: opensumi/core PR: 4088
File: packages/ai-native/src/browser/widget/inline-stream-diff/live-preview.decoration.tsx:73-74
Timestamp: 2024-10-12T07:43:08.790Z
Learning: 在 `LivePreviewDiffDecorationModel` 类（位于 `packages/ai-native/src/browser/widget/inline-stream-diff/live-preview.decoration.tsx`）中，通过依赖注入（`Autowired`）从 module 的 `providers` 声明中获取的依赖（如 `_onPartialEditEvent`），其销毁逻辑由 `ClientApp` 的生命周期管理，不需要手动调用 `dispose()` 进行销毁。
Applied to files:
packages/notebook/src/browser/libro.view.tsx
🧬 Code graph analysis (1)
packages/notebook/src/browser/libro.view.tsx (1)
packages/core-browser/src/react-hooks/injectable-hooks.tsx (1)
useInjectable(16-39)
⏰ 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). (7)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build (macos-latest, 20.x)
🔇 Additional comments (2)
packages/notebook/src/browser/libro-state-manager.ts (1)
29-48: I'm encountering repository access issues. Let me analyze the provided code snippet and the race condition concern based on the code context you've shared.Based on the code snippet in the review comment, I can see:
- Constructor calls
loadFromStorage()without awaiting - this is confirmed in the coderestoreStateonly reads fromstateCache- the snippet doesn't showrestoreState, but this is a logical concern- The race condition claim - if
restoreStateis called beforeloadFromStoragecompletes, it would indeed return false/not restore stateHowever, to fully verify this concern, I need to:
- See the actual
restoreStateimplementation (lines 89-108)- Check where/when
restoreStateis called in the application lifecycle- Understand if there's any synchronization mechanism already in place
Since the repository is not accessible, I cannot provide definitive verification. However, based on async/await best practices:
The concern about the async race condition is valid in principle - firing an async operation in a constructor without awaiting it and then immediately trying to use its results is a known anti-pattern that can cause race conditions.
Let me provide the rewritten review comment based on standard async initialization patterns:
Constructor initiates async storage load without coordination, risking first
restoreStatecall during loading
loadFromStorage()is called in the constructor but not awaited. IfrestoreState()is invoked before the storage load completes, it reads from an empty or incompletestateCache, causing the first state restoration to fail.Recommended fixes:
- Add a
loadingPromiseproperty to track initialization state. InrestoreState(), if loading is pending, await it before checking the cache.- Or: Expose an explicit
async init(): Promise<void>method and ensure callers await it before callingrestoreState().This prevents intermittent scroll position loss when switching back to tabs during navigation.
packages/notebook/src/browser/libro.view.tsx (1)
21-25: I need access to the actual code to verify the claims in the review comment. Since the repository clone failed, could you please provide:
- The full content of
packages/notebook/src/browser/libro.view.tsx(or at least lines 1-120 to see the complete context of theuseEffecthooks and their dependencies)Specifically, I need to verify:
- Whether
libroViewfrom state is used in the cleanup function (which would be the bug, since it starts asundefined)- Whether
handleScroll.cancel()is called in the cleanup- The exact dependency array of the
useEffecthooks- Whether
isStateRestoredis reset whenurichangesOnce you provide the code, I can verify if the review comment's claims are accurate and produce a properly rewritten review.
|
🎉 PR Next publish successful! 3.9.1-next-1764145332.0 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4670 +/- ##
=======================================
Coverage 52.62% 52.63%
=======================================
Files 1686 1686
Lines 104568 104568
Branches 22724 22733 +9
=======================================
+ Hits 55034 55040 +6
+ Misses 41153 41149 -4
+ Partials 8381 8379 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Types
Background or solution
当前 .ipynb 文件（Notebook 文件）使用 LibroOpensumiView 组件，这是一个基于 Libro 框架的自定义视图
Notebook 的状态管理在 Libro 框架内部，而不是 OpenSumi 的标准编辑器状态系统
当前Notebook 编辑器没有实现类似的状态保存/恢复机制，Libro 框架的视图状态没有与 OpenSumi 的编辑器状态系统集成，所以当切换 .ipynb 文件时，整个 LibroView 会被重新创建，而不是恢复之前的状态。
Changelog
新增
libro-state-manager来对 libro 视图进行状态管理，实现多个ipynb文件之间切换能够保留位置。2025-11-18.19.44.16.mov
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.