fix(debug): refresh variables after debug console updates#4733
Merged
coffeedeveloper merged 5 commits intomainfrom Apr 24, 2026
Merged
fix(debug): refresh variables after debug console updates#4733coffeedeveloper merged 5 commits intomainfrom
coffeedeveloper merged 5 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough添加浏览器端 DebugSession REPL 求值测试；在 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant DebugSession as DebugSession
participant VariableStore as "Debug Variables Model\n(service)"
participant Tree as "Variables Tree View"
Client->>DebugSession: evaluate(expression, context='repl')
DebugSession->>DebugSession: sendRequest(evaluate)
DebugSession-->>Client: response
DebugSession->>VariableStore: emit onVariableChange
VariableStore->>VariableStore: enqueue path (dedupe roots, sort by depth)
VariableStore->>VariableStore: flushEventQueue() (batched, pSeries)
VariableStore->>Tree: dispatch WatchEvent.Changed for paths
Tree->>Tree: refresh / restoreExpandedScopes()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts (1)
243-251:⚠️ Potential issue | 🟠 Major先恢复缓存，再执行默认展开逻辑。
新建 tree 的 scope 通常都是折叠态，所以这里会先展开所有
expensive === false的 scope；随后restoreExpandedScopes只能“展开”，不能把未缓存或原本折叠的Globals/Closure再收起，容易抵消本次保留展开状态的目标。建议调整方向
const scopes = (this._activeTreeModel?.root.children as Array<DebugVariableWithRawScope>) || []; + await this.restoreExpandedScopes(scopes); if (scopes.length > 0 && scopes.every((s: DebugScope) => !s.expanded)) { for (const s of scopes) { if ((s as DebugScope).getRawScope().expensive === false && !s.expanded) { await this.toggleDirectory(s); } } } - await this.restoreExpandedScopes(scopes);如果需要区分首次加载和已有缓存刷新，建议给
KeepExpandedScopesModel增加hasState()，只在无缓存时执行默认展开。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts` around lines 243 - 251, The current code expands all non-expensive scopes before restoring cached expanded state, which can re-open scopes that should remain collapsed; change the order and guard the default-expand logic by cache presence: first call restoreExpandedScopes(scopes), then only run the loop that awaits toggleDirectory(s) for (s.getRawScope().expensive === false) when the KeepExpandedScopesModel indicates no saved state (add a hasState() method to KeepExpandedScopesModel and use it to skip the default expansion when a cache exists).
🧹 Nitpick comments (1)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts (1)
152-160: 测试结束后恢复refreshspy，避免污染共享实例。这个 suite 复用同一个
debugVariablesModelService，mockResolvedValue()会一直替换refresh实现；建议在断言后mockRestore()，避免后续或未来测试误用 mock 后的方法。建议调整
- const refreshSpy = jest.spyOn(debugVariablesModelService, 'refresh').mockResolvedValue(); + const refreshSpy = jest.spyOn(debugVariablesModelService, 'refresh').mockResolvedValue(); - mockDebugViewModel.currentSession = mockSession; - await viewModelChangeListener?.(); - await variableChangeListener?.(); - - expect(mockSession.onVariableChange).toHaveBeenCalledTimes(1); - expect(refreshSpy).toHaveBeenCalledTimes(1); + try { + mockDebugViewModel.currentSession = mockSession; + await viewModelChangeListener?.(); + await variableChangeListener?.(); + + expect(mockSession.onVariableChange).toHaveBeenCalledTimes(1); + expect(refreshSpy).toHaveBeenCalledTimes(1); + } finally { + refreshSpy.mockRestore(); + mockDebugViewModel.currentSession = undefined; + variableChangeListener = undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts` around lines 152 - 160, The test creates a spy on debugVariablesModelService.refresh (const refreshSpy = jest.spyOn(debugVariablesModelService, 'refresh').mockResolvedValue()) but never restores it, polluting the shared debugVariablesModelService; after the assertions (or in an afterEach) restore the original implementation by calling refreshSpy.mockRestore() (or refreshSpy.mockRestore() in the test teardown) so subsequent tests see the real debugVariablesModelService.refresh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 359-369: 当前实现中在 queueChangeEvent 中若 flushEventQueueDeferred
非空（flush 正在进行）会把新 path 仅推入 _changeEventDispatchQueue 但不重建定时器，导致这些新事件在当前 flush
结束后永远不会被调度；请修改 queueChangeEvent/flush 流程：把每个 callback 也入队（而不是立即 await
callback），并在 flushEventQueue 完成或 flushEventQueueDeferred 清空时检查
_changeEventDispatchQueue
是否仍有未处理项且没有活动定时器（_eventFlushTimeout/flushEventQueueDeferred），若有则重新创建定时器或立即触发下一次
flush（使用 DEFAULT_REFRESH_DELAY 一致的调度），确保不会丢失在 flush
期间到达的事件；参考符号：queueChangeEvent, flushEventQueue, _changeEventDispatchQueue,
flushEventQueueDeferred, _eventFlushTimeout, DEFAULT_REFRESH_DELAY。
- Around line 385-388: The current de-dup logic in
debug-variables-tree.model.service.ts incorrectly treats prefix matches
(path.indexOf(root) === 0) as parent relationships, causing siblings like
".../foo" and ".../foobar" to be misclassified and skipped; update the loop that
builds roots from _changeEventDispatchQueue to use a proper path-boundary or
canonical path check (e.g., use your Path utility’s "same or parent" semantic or
implement an isSameOrParent(root, path) helper) so you only skip when root is
actually the same path or a true parent of path, referring to the variables
_changeEventDispatchQueue, roots, path and root to locate where to change the
logic.
---
Outside diff comments:
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 243-251: The current code expands all non-expensive scopes before
restoring cached expanded state, which can re-open scopes that should remain
collapsed; change the order and guard the default-expand logic by cache
presence: first call restoreExpandedScopes(scopes), then only run the loop that
awaits toggleDirectory(s) for (s.getRawScope().expensive === false) when the
KeepExpandedScopesModel indicates no saved state (add a hasState() method to
KeepExpandedScopesModel and use it to skip the default expansion when a cache
exists).
---
Nitpick comments:
In
`@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts`:
- Around line 152-160: The test creates a spy on
debugVariablesModelService.refresh (const refreshSpy =
jest.spyOn(debugVariablesModelService, 'refresh').mockResolvedValue()) but never
restores it, polluting the shared debugVariablesModelService; after the
assertions (or in an afterEach) restore the original implementation by calling
refreshSpy.mockRestore() (or refreshSpy.mockRestore() in the test teardown) so
subsequent tests see the real debugVariablesModelService.refresh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 23d4b8d4-16d1-4bcd-8daa-4906c7fa5b1c
📒 Files selected for processing (4)
packages/debug/__tests__/browser/debug-session.test.tspackages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.tspackages/debug/src/browser/debug-session.tspackages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4733 +/- ##
==========================================
+ Coverage 52.89% 52.97% +0.07%
==========================================
Files 1686 1686
Lines 104890 105031 +141
Branches 22794 22996 +202
==========================================
+ Hits 55483 55638 +155
+ Misses 41049 41039 -10
+ Partials 8358 8354 -4
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:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts (1)
244-252:⚠️ Potential issue | 🟡 Minor不要在恢复缓存状态前强制展开已折叠的 scope。
如果用户刷新前显式折叠了
Locals/Globals，Line 245-250 会因为新 tree 初始全折叠而再次展开所有非 expensive scope，覆盖缓存里的折叠状态。建议仅在没有该 scope 的缓存状态时执行默认展开。建议调整方向
const scopes = (this._activeTreeModel?.root.children as Array<DebugVariableWithRawScope>) || []; - if (scopes.length > 0 && scopes.every((s: DebugScope) => !s.expanded)) { + const hasCachedRootScopeState = scopes.some( + (s) => this.isPreservedRootScope(s) && this.keepExpandedScopesModel.hasScopeState(s), + ); + if (scopes.length > 0 && !hasCachedRootScopeState && scopes.every((s: DebugScope) => !s.expanded)) { for (const s of scopes) { if ((s as DebugScope).getRawScope().expensive === false && !s.expanded) { await this.toggleDirectory(s); } }需要在
KeepExpandedScopesModel中补一个基于getMirrorScope()的hasScopeState()方法。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts` around lines 244 - 252, The code currently force-expands non-expensive scopes before restoring cached expanded state; change the logic in debug-variables-tree.model.service (around where scopes are iterated and toggleDirectory is called) to skip expanding a scope if the KeepExpandedScopesModel already has a saved state for that scope—i.e., call a new hasScopeState(getMirrorScope(scope)) and only call toggleDirectory(s) for scopes that have no saved state and are non-expensive; add hasScopeState(mirrorScope) to KeepExpandedScopesModel (using getMirrorScope() to locate the mirror key) so restoreExpandedScopes can rely on it.
🧹 Nitpick comments (1)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts (1)
152-160: 恢复refreshspy，避免污染后续用例。这个 suite 复用同一个
debugVariablesModelService，Line 152 的 spy 会让后续新增用例继续拿到 mockedrefresh()，可能静默绕过真实刷新逻辑。建议调整
- const refreshSpy = jest.spyOn(debugVariablesModelService, 'refresh').mockResolvedValue(); - - mockDebugViewModel.currentSession = mockSession; - await viewModelChangeListener?.(); - await variableChangeListener?.(); - - expect(mockSession.onVariableChange).toHaveBeenCalledTimes(1); - expect(refreshSpy).toHaveBeenCalledTimes(1); + const refreshSpy = jest.spyOn(debugVariablesModelService, 'refresh').mockResolvedValue(); + try { + mockDebugViewModel.currentSession = mockSession; + await viewModelChangeListener?.(); + await variableChangeListener?.(); + + expect(mockSession.onVariableChange).toHaveBeenCalledTimes(1); + expect(refreshSpy).toHaveBeenCalledTimes(1); + } finally { + refreshSpy.mockRestore(); + mockDebugViewModel.currentSession = undefined; + variableChangeListener = undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts` around lines 152 - 160, The test creates a jest.spyOn for debugVariablesModelService.refresh which remains mocked and can leak into later tests; after asserting expect(refreshSpy).toHaveBeenCalledTimes(1) restore the spy (e.g., call refreshSpy.mockRestore()) or call jest.restoreAllMocks() in an afterEach to return debugVariablesModelService.refresh to its original implementation so subsequent specs exercise the real refresh implementation; locate the spy by the symbol refreshSpy created from jest.spyOn(debugVariablesModelService, 'refresh') and ensure it is restored after the test that calls viewModelChangeListener and variableChangeListener.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 342-390: The refresh() method currently only enqueues work via
queueChangeEvent() and returns immediately, so callers awaiting refresh() don't
wait for restoreExpandedScopes()/onDidRefreshedEmitter.fire() or errors; change
refresh() to await the queued flush by returning a Promise that resolves/rejects
when that queued work completes (use the existing flushEventQueueDeferred or
create/return a Deferred tied to the queue runner). Specifically, in refresh()
after calling queueChangeEvent(node.path, ...), return the Deferred promise that
queueChangeEvent sets/uses (flushEventQueueDeferred) so the caller awaits the
actual execution of the queued callback which performs restoreExpandedScopes()
and fires onDidRefreshedEmitter; ensure errors from the queued callback
propagate by rejecting the same Deferred so await refresh() sees them.
- Around line 398-428: flushEventQueue currently clears and processes
_changeEventDispatchQueue immediately and is vulnerable to reentrant calls that
can drop newly queued items; add a reentrancy guard (e.g. a private boolean
_isFlushing and/or a Promise _currentFlushPromise) at the start of public
flushEventQueue to detect in-progress execution, return the existing
_currentFlushPromise when a flush is already running (or simply no-op if you
prefer), and ensure the guard is set before mutating _changeEventDispatchQueue
and cleared in a finally block so timers and _pendingFlushCallback won't be left
pending; update references in flushEventQueue and any timer code that sets
_pendingFlushCallback to await or rely on _currentFlushPromise so no events are
skipped.
- Around line 222-226: disposeTreeListeners() currently disposes
disposableCollection but code in listenTreeViewChange() later pushes new
watchers into the same instance; change the pattern to rebuild the collection
after disposal (same safe approach used for currentSessionDisposableCollection)
so that after calling disposeTreeListeners() you assign a fresh
DisposableCollection to this.disposableCollection before registering new
listeners (ensure listenTreeViewChange() and any push calls reference the newly
created instance).
---
Outside diff comments:
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 244-252: The code currently force-expands non-expensive scopes
before restoring cached expanded state; change the logic in
debug-variables-tree.model.service (around where scopes are iterated and
toggleDirectory is called) to skip expanding a scope if the
KeepExpandedScopesModel already has a saved state for that scope—i.e., call a
new hasScopeState(getMirrorScope(scope)) and only call toggleDirectory(s) for
scopes that have no saved state and are non-expensive; add
hasScopeState(mirrorScope) to KeepExpandedScopesModel (using getMirrorScope() to
locate the mirror key) so restoreExpandedScopes can rely on it.
---
Nitpick comments:
In
`@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts`:
- Around line 152-160: The test creates a jest.spyOn for
debugVariablesModelService.refresh which remains mocked and can leak into later
tests; after asserting expect(refreshSpy).toHaveBeenCalledTimes(1) restore the
spy (e.g., call refreshSpy.mockRestore()) or call jest.restoreAllMocks() in an
afterEach to return debugVariablesModelService.refresh to its original
implementation so subsequent specs exercise the real refresh implementation;
locate the spy by the symbol refreshSpy created from
jest.spyOn(debugVariablesModelService, 'refresh') and ensure it is restored
after the test that calls viewModelChangeListener and variableChangeListener.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7cd9dbe1-1ab1-48f0-a066-509615d19fcc
📒 Files selected for processing (2)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.tspackages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts (1)
216-228: 建议在dispose()中一并清理刷新管道的残留状态。当前
dispose()只处理两个DisposableCollection，但_eventFlushTimeout、flushEventQueueDeferred、_changeEventDispatchQueue、_pendingFlushCallbacks均未被清理。若服务在queueChangeEvent已排定但尚未触发的窗口内被销毁，定时器仍会回调并对this.treeModel执行restoreExpandedScopes/ 触发onDidRefreshedEmitter，也会让仍在await flushEventQueuePromise的调用方一直等待到定时器自然完成。虽然实际影响很小（服务通常与插件生命周期同在），但作为收尾的卫生处理是值得加上的。♻️ 建议修改
dispose() { this.disposeTreeListeners(); if (!this.currentSessionDisposableCollection.disposed) { this.currentSessionDisposableCollection.dispose(); } + clearTimeout(this._eventFlushTimeout); + this._changeEventDispatchQueue = []; + this._pendingFlushCallbacks = []; + if (this.flushEventQueueDeferred) { + this.flushEventQueueDeferred.resolve(); + this.flushEventQueueDeferred = null; + } + this._isFlushingEventQueue = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts` around lines 216 - 228, Update dispose() to clean up pending flush/dispatch state: clear and null out the timeout stored in _eventFlushTimeout, resolve or reject and clear flushEventQueueDeferred (so awaiting callers don't hang), drain/clear _changeEventDispatchQueue and _pendingFlushCallbacks, and ensure any pending callbacks tied to queueChangeEvent are not invoked (avoid calling restoreExpandedScopes or onDidRefreshedEmitter after dispose). Keep disposeTreeListeners() behavior but add these flush-related cleanups in dispose() so the service cannot later use treeModel or trigger onDidRefreshedEmitter; reference the symbols dispose(), disposeTreeListeners(), _eventFlushTimeout, flushEventQueueDeferred, _changeEventDispatchQueue, _pendingFlushCallbacks, queueChangeEvent, restoreExpandedScopes, onDidRefreshedEmitter, and flushEventQueuePromise when locating where to apply the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 265-281: The variable-change handler can run for a previous tree
because listenCurrentSessionVariableChange() swaps subscriptions synchronously
while the new tree build is delayed; update refresh() to early-return if the
session that triggered the event is not the current session (compare the session
instance or an id against this.currentSession / this.viewModel.currentSession)
to ensure node.path is read only for the matching tree; additionally, make the
onVariableChange subscription installed in listenCurrentSessionVariableChange()
wrap its callback with a lightweight guard that checks the session is still
active/current before calling refresh(), and add defensive checks in evaluate()
and setVariableValue() to verify the session is still alive/attached before
firing _onVariableChange.
---
Nitpick comments:
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 216-228: Update dispose() to clean up pending flush/dispatch
state: clear and null out the timeout stored in _eventFlushTimeout, resolve or
reject and clear flushEventQueueDeferred (so awaiting callers don't hang),
drain/clear _changeEventDispatchQueue and _pendingFlushCallbacks, and ensure any
pending callbacks tied to queueChangeEvent are not invoked (avoid calling
restoreExpandedScopes or onDidRefreshedEmitter after dispose). Keep
disposeTreeListeners() behavior but add these flush-related cleanups in
dispose() so the service cannot later use treeModel or trigger
onDidRefreshedEmitter; reference the symbols dispose(), disposeTreeListeners(),
_eventFlushTimeout, flushEventQueueDeferred, _changeEventDispatchQueue,
_pendingFlushCallbacks, queueChangeEvent, restoreExpandedScopes,
onDidRefreshedEmitter, and flushEventQueuePromise when locating where to apply
the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7d9078a1-3d4b-41ff-a697-934835b65f32
📒 Files selected for processing (2)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.tspackages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/debug/tests/browser/view/variables/debug-variables-tree.model.service.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts (2)
391-444: 建议补充一例“Closure 即使被缓存为展开也不应恢复”的覆盖。当前用例只断言 Closure 不在缓存中时不会被展开，但没有覆盖
isPreservedRootScope的关键逻辑：只有Locals/Globals才应从缓存恢复scopeExpanded。若将Closure对应的旧 scope 同样通过keepExpandedScopesModel.set()缓存起来，再断言refreshedClosureScope.setExpanded仍未被调用，可以防止后续有人误扩展保留列表导致回归。🧪 建议增量断言
+ const oldClosureScope = { + expanded: true, + variablesReference: 3, + getRawScope: () => ({ name: 'Closure', expensive: false }), + }; + (debugVariablesModelService as any).keepExpandedScopesModel.set(oldClosureScope); + await (debugVariablesModelService as any).restoreExpandedScopes([ refreshedLocalsScope, refreshedGlobalsScope, refreshedClosureScope, ]); expect(refreshedLocalsScope.setExpanded).toHaveBeenCalledTimes(1); expect(refreshedGlobalsScope.setExpanded).toHaveBeenCalledTimes(1); expect(refreshedClosureScope.setExpanded).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts` around lines 391 - 444, Add a test variation that verifies a cached "Closure" scope is not restored: create an oldClosureScope object with expanded: true, variablesReference matching a closure, and getRawScope returning { name: 'Closure', expensive: false }, call (debugVariablesModelService as any).keepExpandedScopesModel.set(oldClosureScope) before invoking (debugVariablesModelService as any).restoreExpandedScopes([...]) and then assert refreshedClosureScope.setExpanded was not called; this ensures restoreExpandedScopes and isPreservedRootScope only restore Locals/Globals and do not mistakenly expand Closure even when cached (refer to keepExpandedScopesModel, restoreExpandedScopes, isPreservedRootScope, setExpanded, refreshedClosureScope).
144-240: 测试间共享可变状态，存在顺序耦合风险。用例直接通过
mockDebugViewModel.currentSession = ...以及(debugVariablesModelService as any).currentSession / _activeTreeModel = ...修改共享对象的内部状态，而没有在各自的afterEach中重置。这些残留会影响后续依赖viewModel.currentSession的用例，容易因执行顺序调整而出现隐蔽的失败。建议在每个改动这些状态的用例结尾（或统一的
afterEach）重置一下：🧪 建议补充 teardown
+ afterEach(() => { + mockDebugViewModel.currentSession = undefined; + (debugVariablesModelService as any)._activeTreeModel = undefined; + (debugVariablesModelService as any).currentSession = undefined; + (debugVariablesModelService as any)._changeEventDispatchQueue = []; + (debugVariablesModelService as any)._pendingFlushCallbacks = []; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts` around lines 144 - 240, Tests mutate shared state (mockDebugViewModel.currentSession, (debugVariablesModelService as any).currentSession, and (debugVariablesModelService as any)._activeTreeModel) without resetting it, causing order-dependent failures; add a teardown to reset these after each test (either an afterEach block or at the end of individual tests) by setting mockDebugViewModel.currentSession = undefined, (debugVariablesModelService as any).currentSession = undefined, and (debugVariablesModelService as any)._activeTreeModel = undefined, and ensure any fake timers or spies created in tests (e.g., jest.useFakeTimers / jest.useRealTimers and spies like refreshSpy) are restored/cleared so no state leaks between tests.packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts (1)
416-420:isSameOrParentPath中的等值判断是冗余的。OpenSumi 的
Path.isEqualOrParent(target)实现通过!!this.relative(path)完成。当路径相等时，relative()返回空字符串对应的 Path 对象，该对象为真值。因此isEqualOrParent已包含路径相等的判断，base.isEqual(target)这一项可以移除。♻️ 简化方案
private isSameOrParentPath(basePath: string, targetPath: string) { const base = new Path(basePath); const target = new Path(targetPath); - return base.isEqual(target) || base.isEqualOrParent(target); + return base.isEqualOrParent(target); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts` around lines 416 - 420, The isSameOrParentPath function contains a redundant equality check: remove the explicit base.isEqual(target) because Path.isEqualOrParent(target) already returns true for equal paths (it uses !!this.relative(path)), so update isSameOrParentPath (which constructs new Path(basePath) and new Path(targetPath)) to return only base.isEqualOrParent(target) and drop the ORed isEqual branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 216-221: In dispose(), in addition to disposeTreeListeners() and
currentSessionDisposableCollection.dispose(), ensure you cancel the pending
flush timer and settle the pending deferred: clear the _eventFlushTimeout (so
the scheduled callback that calls restoreExpandedScopes and
onDidRefreshedEmitter.fire cannot run after destruction) and explicitly resolve
or reject flushEventQueueDeferred (so any awaiting callers of refresh() do not
hang or receive a late resolution after teardown); locate the dispose() method
and add logic to clear _eventFlushTimeout and to settle flushEventQueueDeferred
before returning.
---
Nitpick comments:
In
`@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts`:
- Around line 391-444: Add a test variation that verifies a cached "Closure"
scope is not restored: create an oldClosureScope object with expanded: true,
variablesReference matching a closure, and getRawScope returning { name:
'Closure', expensive: false }, call (debugVariablesModelService as
any).keepExpandedScopesModel.set(oldClosureScope) before invoking
(debugVariablesModelService as any).restoreExpandedScopes([...]) and then assert
refreshedClosureScope.setExpanded was not called; this ensures
restoreExpandedScopes and isPreservedRootScope only restore Locals/Globals and
do not mistakenly expand Closure even when cached (refer to
keepExpandedScopesModel, restoreExpandedScopes, isPreservedRootScope,
setExpanded, refreshedClosureScope).
- Around line 144-240: Tests mutate shared state
(mockDebugViewModel.currentSession, (debugVariablesModelService as
any).currentSession, and (debugVariablesModelService as any)._activeTreeModel)
without resetting it, causing order-dependent failures; add a teardown to reset
these after each test (either an afterEach block or at the end of individual
tests) by setting mockDebugViewModel.currentSession = undefined,
(debugVariablesModelService as any).currentSession = undefined, and
(debugVariablesModelService as any)._activeTreeModel = undefined, and ensure any
fake timers or spies created in tests (e.g., jest.useFakeTimers /
jest.useRealTimers and spies like refreshSpy) are restored/cleared so no state
leaks between tests.
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 416-420: The isSameOrParentPath function contains a redundant
equality check: remove the explicit base.isEqual(target) because
Path.isEqualOrParent(target) already returns true for equal paths (it uses
!!this.relative(path)), so update isSameOrParentPath (which constructs new
Path(basePath) and new Path(targetPath)) to return only
base.isEqualOrParent(target) and drop the ORed isEqual branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd679cea-03b5-481b-a1f0-e41761b2cb3a
📒 Files selected for processing (2)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.tspackages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts (3)
144-161:mockDebugViewModel.currentSession未在测试结束后恢复，存在跨用例状态泄漏。此测试把
mockDebugViewModel.currentSession设为mockSession后未复位（同时variableChangeListener、debugVariablesModelService.currentSession也保持对该 mock 的引用）。后续用例（如 line 163、205）虽自行覆盖currentSession，但debugVariablesModelService.currentSession仍指向上一个会话，依赖当前DebugVariablesModelService的 mirror/lifecycle 分支才能正常工作，增加了测试脆弱性。建议在每个用例用afterEach清理mockDebugViewModel.currentSession以及相关 listener，或者改用独立 service 实例。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts` around lines 144 - 161, The test sets mockDebugViewModel.currentSession (and installs variableChangeListener) but never resets it, causing cross-test state leakage; update the test suite to clean up after each case by restoring mockDebugViewModel.currentSession to null/undefined and clearing variableChangeListener and any references on debugVariablesModelService.currentSession (or instantiate a fresh DebugVariablesModelService per test) in an afterEach block so each spec starts with a clean session and no lingering listeners.
427-480:restoreExpandedScopes对 Locals/Globals 恢复、Closure 排除行为的用例清晰。通过
keepExpandedScopesModel.set(oldLocalsScope)预置缓存、对比刷新后的refreshedLocalsScope / refreshedGlobalsScope / refreshedClosureScope的setExpanded调用次数，完整覆盖了 PR 中"仅保留 Locals/Globals 顶层展开、Closure 等非保留作用域不被恢复"的语义。不过两个 rawScope 在此测试中不含
variablesReference等易变字段，建议补充一个用例，验证当底层 DAP 在刷新后把Locals的variablesReference换成新值时getMirrorScope的匹配行为是否仍符合预期（若与同文件其他评论中的匹配策略对齐再处理）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts` around lines 427 - 480, Add a new unit test that verifies restoreExpandedScopes/getMirrorScope still matches cached Locals when the DAP changes variablesReference: seed keepExpandedScopesModel with an old Locals scope (old variablesReference), then call restoreExpandedScopes with a refreshedLocalsScope that has a different variablesReference value and a mocked setExpanded; assert setExpanded is still called once for Locals (and not called for unrelated scopes like Closure). Reference restoreExpandedScopes, getMirrorScope and keepExpandedScopesModel when locating the code to update.
295-329: 使用_disposed = false反向复位共享服务状态是测试 hack，建议改用每用例独立实例或显式reset方法。
finally中通过(debugVariablesModelService as any)._disposed = false恢复共享服务以便后续用例能继续运行，但：
currentSessionDisposableCollection已被 dispose 且未在此处重建，若后续用例触发listenCurrentSessionVariableChange路径会意外调用 dispose-on-disposed（当前实现是安全的，但行为依赖内部细节）。- 用例顺序一旦调整（如添加新的用例在其后），可能出现难以排查的测试污染。
建议改为在每个需要 dispose 的用例里通过工厂获取新的
DebugVariablesModelService实例，或为该服务提供受测友好的重置/重新初始化接口：♻️ 建议方向
- it('cancels pending queued refresh work when disposed', async () => { + it('cancels pending queued refresh work when disposed', async () => { + const scopedService = mockInjector.get(DebugVariablesModelService, [], { overrideExisting: false }) as DebugVariablesModelService; jest.useFakeTimers(); try { ... - const refreshPromise = debugVariablesModelService.refresh(root as any); + const refreshPromise = scopedService.refresh(root as any); ... - debugVariablesModelService.dispose(); + scopedService.dispose(); ... } finally { - (debugVariablesModelService as any)._disposed = false; jest.useRealTimers(); } });具体 API 以依赖注入容器支持为准。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts` around lines 295 - 329, The test resets shared service state by directly toggling (debugVariablesModelService as any)._disposed = false which hacks around disposal and risks cross-test pollution (and leaves currentSessionDisposableCollection disposed); instead instantiate a fresh DebugVariablesModelService for this test or add and call a test-friendly reset/reinitialize method on DebugVariablesModelService that rebuilds internal disposables and clears _disposed and currentSessionDisposableCollection; locate usages of debugVariablesModelService, _disposed, currentSessionDisposableCollection and listenCurrentSessionVariableChange in the test and replace the shared-service reuse with either a factory-created new instance or an explicit reset call in the finally block so no internal state is mutated directly.packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts (1)
461-466:flushEventQueue在 reentrancy 时静默返回undefined，考虑回传进行中的 flush promise。当
_isFlushingEventQueue为true时，公共flushEventQueue()直接 return，await它的外部调用方无法感知当前 flush 何时完成。虽然while循环会继续 drain 新入队项，但 API 语义上，调用方await flushEventQueue()更符合直觉的行为是等待本次/当前 flush 完成再返回。建议在 reentrancy 分支回传flushEventQueuePromise：♻️ 建议调整
public flushEventQueue = () => { - if (this._disposed || this._isFlushingEventQueue) { + if (this._disposed) { return; } + if (this._isFlushingEventQueue) { + return this.flushEventQueuePromise; + } return this.flushQueuedEventBatch(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts` around lines 461 - 466, The public flushEventQueue currently returns undefined when re-entered due to _isFlushingEventQueue being true; change the reentrancy branch so that instead of silently returning, it returns the ongoing flush promise (flushEventQueuePromise) so callers awaiting flushEventQueue() observe completion; keep the existing _disposed short-circuit behavior, but when _isFlushingEventQueue is true have flushEventQueue return this.flushEventQueuePromise (or a resolved promise if that field can be null) and ensure flushQueuedEventBatch remains the canonical runner that populates that promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.ts`:
- Around line 144-161: The test sets mockDebugViewModel.currentSession (and
installs variableChangeListener) but never resets it, causing cross-test state
leakage; update the test suite to clean up after each case by restoring
mockDebugViewModel.currentSession to null/undefined and clearing
variableChangeListener and any references on
debugVariablesModelService.currentSession (or instantiate a fresh
DebugVariablesModelService per test) in an afterEach block so each spec starts
with a clean session and no lingering listeners.
- Around line 427-480: Add a new unit test that verifies
restoreExpandedScopes/getMirrorScope still matches cached Locals when the DAP
changes variablesReference: seed keepExpandedScopesModel with an old Locals
scope (old variablesReference), then call restoreExpandedScopes with a
refreshedLocalsScope that has a different variablesReference value and a mocked
setExpanded; assert setExpanded is still called once for Locals (and not called
for unrelated scopes like Closure). Reference restoreExpandedScopes,
getMirrorScope and keepExpandedScopesModel when locating the code to update.
- Around line 295-329: The test resets shared service state by directly toggling
(debugVariablesModelService as any)._disposed = false which hacks around
disposal and risks cross-test pollution (and leaves
currentSessionDisposableCollection disposed); instead instantiate a fresh
DebugVariablesModelService for this test or add and call a test-friendly
reset/reinitialize method on DebugVariablesModelService that rebuilds internal
disposables and clears _disposed and currentSessionDisposableCollection; locate
usages of debugVariablesModelService, _disposed,
currentSessionDisposableCollection and listenCurrentSessionVariableChange in the
test and replace the shared-service reuse with either a factory-created new
instance or an explicit reset call in the finally block so no internal state is
mutated directly.
In
`@packages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts`:
- Around line 461-466: The public flushEventQueue currently returns undefined
when re-entered due to _isFlushingEventQueue being true; change the reentrancy
branch so that instead of silently returning, it returns the ongoing flush
promise (flushEventQueuePromise) so callers awaiting flushEventQueue() observe
completion; keep the existing _disposed short-circuit behavior, but when
_isFlushingEventQueue is true have flushEventQueue return
this.flushEventQueuePromise (or a resolved promise if that field can be null)
and ensure flushQueuedEventBatch remains the canonical runner that populates
that promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb393de4-ee0c-4025-abcc-794d1a3784c0
📒 Files selected for processing (2)
packages/debug/__tests__/browser/view/variables/debug-variables-tree.model.service.test.tspackages/debug/src/browser/view/variables/debug-variables-tree.model.service.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types
Background
When using the Python debugger, executing assignments in the Debug Console, such as
t = 1, updates the console output immediately but does not refresh theVARIABLESview until the next step or stop event.There is also a follow-up UX issue after refresh: if top-level
LocalsorGlobalsscopes were expanded before the refresh, they can collapse afterward, which makes the VARIABLES panel feel out of sync with the latest debug state.Before
After
Solution
This PR makes REPL-driven state changes refresh the VARIABLES panel immediately and preserves the expected expansion state during that refresh.
Changes included in this PR:
replevaluate requests.LocalsandGlobalsscopes across VARIABLES refreshes.Changelog
VARIABLESuntil the next debug step.LocalsandGlobalsscopes could collapse after VARIABLES refresh.Summary by CodeRabbit
发布说明
**Tests（测试）
**Improvements（改进）