Skip to content

fix: reset summary panel on stop-scan [IDE-1035]#1324

Open
acke wants to merge 14 commits into
mainfrom
fix/IDE-1035_reset-summary-on-stop-scan
Open

fix: reset summary panel on stop-scan [IDE-1035]#1324
acke wants to merge 14 commits into
mainfrom
fix/IDE-1035_reset-summary-on-stop-scan

Conversation

@acke

@acke acke commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

User description

Summary

  • New resetSummaryPanelOnStopScan helper reuses the existing resetSummaryPanelForOrgChange reset path.
  • windowWorkDoneProgressCancelHandler invokes it after progress.Cancel, so pressing the stop-scan button now clears the summary back to its initial state instead of leaving stale counts on screen.
  • Three unit tests: happy path, nil aggregator, no folders.

Why

When the user pressed stop-scan, the summary panel kept showing the previous scan's counts even though the scan was cancelled. The org-change flow already had a helper to reset the panel to initial state — this PR wires the same helper into the stop-scan LSP handler (window/workDoneProgress/cancel).

Test plan

  • TestResetSummaryPanelOnStopScan_CallsInit
  • TestResetSummaryPanelOnStopScan_NilAgg_DoesNotPanic
  • TestResetSummaryPanelOnStopScan_NoFolders_DoesNotCallInit
  • Manual: start a scan, press stop, observe the summary returns to initial / empty.

Jira: IDE-1035
Triage: Buglist


PR Type

Bug fix, Tests


Description

  • Fixes stale summary panel counts after scan stop.

  • Introduces helper to reset summary panel state.

  • Integrates reset logic into scan cancellation handler.

  • Adds unit tests for reset functionality verification.


Diagram Walkthrough

flowchart LR
  LSPClient -- "window/workDoneProgress/cancel" --> Server;
  Server -- "windowWorkDoneProgressCancelHandler" --> Server;
  Server -- "resetSummaryPanelOnStopScan" --> Server;
  Server -- "resetSummaryPanelForOrgChange" --> ScanAggregator;
  ScanAggregator -- "Init" --> SummaryPanel;
Loading

File Walkthrough

Relevant files
Enhancement
configuration.go
Add helper to reset summary panel                                               

application/server/configuration.go

  • Added resetSummaryPanelOnStopScan function.
  • This new helper retrieves current workspace folder paths and delegates
    to resetSummaryPanelForOrgChange.
  • Encapsulates the logic for resetting the summary panel to its initial
    state.
+9/-0     
Tests
configuration_test.go
Add tests for summary panel reset                                               

application/server/configuration_test.go

  • Introduced three new unit tests for resetSummaryPanelOnStopScan.
  • Tests cover: happy path calling Init, handling a nil aggregator
    without panicking, and not calling Init when no folders are present.
+39/-0   
Bug fix
server.go
Integrate summary panel reset into cancel scan handler     

application/server/server.go

  • Modified windowWorkDoneProgressCancelHandler to accept
    configuration.Configuration.
  • The handler now calls resetSummaryPanelOnStopScan after cancelling
    progress, ensuring the summary panel is cleared.
+4/-2     

When the user pressed the stop-scan button (LSP
window/workDoneProgress/cancel) the summary panel kept showing the
last scan's counts. The org-change path already had a helper that
reinitialises the panel to its initial state; this commit wires the
same reset into the stop-scan handler.

- New resetSummaryPanelOnStopScan helper delegates to the existing
  resetSummaryPanelForOrgChange with the current workspace folders.
- windowWorkDoneProgressCancelHandler now invokes it after
  progress.Cancel.
- Unit tests cover the happy path, nil-aggregator safety, and the
  no-folders case.
@acke

acke commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/describe

@snyk-io

snyk-io Bot commented Jun 5, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Description updated to latest commit (65ff59e)

Lint catch: golangci-lint misspell wants the American spelling.
@acke

acke commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/describe

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Description updated to latest commit (89251fb)

@acke acke marked this pull request as ready for review June 5, 2026 16:31
@acke acke requested a review from a team as a code owner June 5, 2026 16:31
@snyk-pr-review-bot

This comment has been minimized.

Comment thread application/server/server.go Outdated
Comment thread application/server/server.go Outdated
Comment thread application/server/server.go Outdated
Comment thread application/server/configuration.go Outdated
…utines exit [IDE-1035]

Address PR #1324 review (D, E, F, G):

D) The window/workDoneProgress/cancel handler reset the summary panel for
   every cancel, including CLI install/download trackers, wiping valid
   scan results. Introduce progress.NewScanTracker / IsScanToken so the
   handler can gate the reset on the token being a scan token. Update
   Code, IaC, OSS, and the code-client tracker factory to use the new
   constructor.

E) The reset raced in-flight scan goroutines: progress.Cancel only signals
   over a channel, and a late SetScanDone could land after the synchronous
   Init reset, re-populating the panel. Move the reset into the scan
   lifecycle: DelegatingConcurrentScanner.RegisterCancelCallback stores a
   per-folder reset fn that Scan() consumes (LoadAndDelete) after both
   wait groups return — guaranteeing SetScanDone has already happened.
   New outcome-level test TestScan_CancelCallback_CalledAfterGoroutinesFinish
   asserts SetScanDone appears before the reset (Init) in the call sequence.

F) The cancel handler discarded the `ok` from scanStateAggregatorFromContext,
   silently skipping the reset if the aggregator was missing. Log a Warn
   so wiring failures are observable (panic would be too aggressive for an
   LSP notification handler).

G) resetSummaryPanelForOrgChange is now shared by org-change and stop-scan.
   Rename to resetSummaryPanel (and update callers/comments) so the name
   matches the responsibility.

Tests:
- TestIsScanToken_{ScanTracker,PlainTracker,Unknown,AfterCancel}_*
- TestCancelProgress_NonScanToken_DoesNotResetAggregator
- TestScan_CancelCallback_CalledAfterGoroutinesFinish
- Renamed TestResetSummaryPanel_* (G)
@snyk-pr-review-bot

This comment has been minimized.

- server.go: drop pre-Go-1.22 `fp := fp` (copyloopvar)
- scanner.go: extract consumeCancelCallback so Scan's cyclomatic complexity
  stays under 15 (gocyclo); checked type assertion instead of `fn.(func())()`
  (forcetypeassert)
- scanner.go: drop var-decl-with-type (`var a, b int = -1, -1` → `a, b := -1, -1`)
  in the new outcome test (staticcheck QF1011)
- notification_test.go, scanner_test.go, progress_test.go: cancelled→canceled,
  recognised→recognized (misspell, repo prefers en_US)
acke and others added 5 commits June 10, 2026 15:51
…s first [IDE-1035]

Defer the cancel call in windowWorkDoneProgressCancelHandler so the
RegisterCancelCallback loop runs before scan goroutines observe the
cancel and drain through consumeCancelCallback. Otherwise the reset
can be silently dropped and a stale callback fires on the folder's
next scan.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pper [IDE-1035]

The wrapper had no production callers — only three TestResetSummaryPanel
OnStopScan_* tests exercised it. The real stop-scan handler in
windowWorkDoneProgressCancelHandler calls resetSummaryPanel directly,
so the wrapper plus its dedicated tests gave false coverage of a path
that never ran in production.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ype the registry, cover handler [IDE-1035]

Address the application/server ↔ domain/snyk/scanner layering and
type-safety feedback from PR review:

- Lift RegisterCancelCallback(folderPath, fn) onto scanner.Scanner so
  the cancel handler calls it through the interface — drops the
  *DelegatingConcurrentScanner downcast and the racy synchronous
  summary-panel reset that used to fire when the type assertion failed.
  TestScanner gets a no-op implementation since it doesn't run the
  cancel-drain cycle.

- Replace the cancelCallbackMap = sync.Map alias with a plain
  map[types.FilePath]func() guarded by a sync.Mutex. Drops the
  defensive v.(func()) assertion in consumeCancelCallback and keeps
  values typed; the access pattern (one register + one consume per
  folder) doesn't need sync.Map's machinery.

- Extract the handler body into handleWindowWorkDoneProgressCancel
  so the new cancel_handler_test.go can drive it without going
  through jrpc2 indirection. Tests cover scan-token register-then-
  cancel ordering, non-scan-token cancel-without-register, and
  the no-scanner skip path (proving the racy sync fallback is gone).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rWithResolverAndAgg [IDE-1035]

setupScannerWithResolver and setupScannerWithResolverAndAgg duplicated
the full NewDelegatingScanner construction; the only difference was an
explicit aggregator parameter. Have the former delegate to the latter
with NewNoopStateAggregator as the default.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…field [IDE-1035]

The parallel scanTokens map (and its mutex) had to be kept in sync with
the trackers map in three places — deleteTracker, Cancel, and
CleanupChannels — so any future tracker-removal path that forgot the
second mutation would leak or stale the scan-token state.

Model "is scan" as an isScan bool field on Tracker. IsScanToken becomes
a single trackers lookup under trackersMutex.RLock and returns false
once the tracker entry is gone, so removal logic only has to touch
trackers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

Comment thread application/server/cancel_handler_test.go Outdated
Comment thread internal/progress/progress.go
@acke acke requested a review from bastiandoetsch June 10, 2026 14:23
- gofmt -w internal/progress/progress.go: the new isScan field's
  preceding comment split the Tracker struct's alignment group; reflow
  the pre-comment fields to the gofmt-canonical narrow alignment.
- recognised → recognized in cancel_handler_test.go to satisfy
  golangci-lint's misspell checker.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Stale Callback Bug 🟠 [major]

The handler registers a reset callback for every folder in the workspace whenever any scan is cancelled. For folders not part of the current scan, these callbacks persist in the DelegatingConcurrentScanner and will be executed at the end of the next scan for those folders. This results in successful scan results being immediately wiped by the stale reset callback. The Tracker should ideally store its associated folder path so the handler can scope the registration to the affected folder only.

for _, fp := range workspaceFolderPaths(conf) {
	sc.RegisterCancelCallback(fp, func() {
		resetSummaryPanel(scanAgg, []types.FilePath{fp})
	})
}
Broad Reset Blast Radius 🟡 [minor]

In a multi-folder workspace, cancelling a scan for Folder A triggers a summary panel reset for all folders via the registered callbacks. This is overly aggressive as it clears results for other folders that may have finished successfully or are still being scanned independently. Restricting the reset to the folder(s) associated with the cancelled token would improve the user experience.

for _, fp := range workspaceFolderPaths(conf) {
	sc.RegisterCancelCallback(fp, func() {
		resetSummaryPanel(scanAgg, []types.FilePath{fp})
	})
}
📚 Repository Context Analyzed

This review considered 40 relevant code sections from 12 files (average relevance: 0.95)

Msg("scanner not in context; summary panel will not be reset")
return nil, nil
}
for _, fp := range workspaceFolderPaths(conf) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should fix — cancel resets every folder, not the cancelled one. This loop registers a reset callback for every workspace folder, but params.Token identifies a single per-product tracker belonging to one folder. In a multi-folder workspace, cancelling folder A's scan also registers reset callbacks on folders B, C… Those callbacks are not consumed now (no in-flight scan there) and instead fire on each of those folders' next Scan() completion (scanner.go consumeCancelCallback), wiping valid results the user never asked to clear. Consider mapping the cancelled token back to its owning folder and registering the reset only for that folder.

(Three of the four reviewers independently flagged this as the root cause of a cluster of data-loss bugs.)

— AI review

// for this folder (IDE-1035). Consuming the callback here — after the
// WaitGroups — guarantees the aggregator reset cannot race with in-flight
// SetScanDone calls.
sc.consumeCancelCallback(folderPath)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should fix — consume is unconditional and unreachable on early returns. Two related issues:

  1. consumeCancelCallback(folderPath) fires at the end of every Scan() with no check that this scan was cancelled. A callback registered during a cancel therefore also resets the panel after the folder's next successful scan — turning "reset on cancel" into "reset on next scan completion", wiping valid results.

  2. This call sits after the WaitGroups, past several early returns in Scan() (missing FolderConfig, offline, not authenticated, already-cancelled ctx). If the next thing that happens to the folder hits one of those guards, the registered callback is neither fired nor cleared — it lingers and fires against a still-later scan.

Consider draining/clearing the folder's callback in a defer at the top of Scan() (after folderPath is known) so every exit path consumes it exactly once, and gate the reset on whether the scan was actually cancelled.

— AI review

@basti-snyk

Copy link
Copy Markdown
Contributor

ℹ️ Automated AI review summary — non-blocking; the actionable items are posted as inline comments.

Reviewed origin/main...HEAD with four independent reviewers (semantic analysis, adversarial, security scan, code-review). Strong convergence.

What's solid: The core IDE-1035 race fix is correct and well-tested. Scan-token scoping (IsScanToken) keeps generic CLI download/install cancels from wiping results; the register-before-defer Cancel ordering plus consuming after both WaitGroups guarantees the reset can't race in-flight SetScanDone writes (covered by TestScan_CancelCallback_CalledAfterGoroutinesFinish). The removal of the synchronous racy fallback when the scanner is missing is the right call.

Should fix (data-loss cluster, one root cause): The cancel token carries no folder identity, and the reset is keyed to a future Scan() of each folder. This produces three related defects, all regressions vs. the old handler which never touched the panel — see the two inline comments. Plus a feature gap:

  • No in-flight scan at cancel time: if the user stops when the scan has already completed (or no scan is running for the folder), the callback is registered but never consumed, so the panel is never reset — the user-visible intent of stop-scan silently no-ops, and the orphaned callback later corrupts the next scan.

Open question for the author: does the IDE client send one window/workDoneProgress/cancel per scan, or one per product token? progress.Cancel(params.Token) stops only the single product tracker, while the reset waits for all products to finish — so non-cancelled products run to completion and then the panel is wiped. The blast-radius and severity of the above depend on this. Worth confirming against the ticket ACs whether "reset all folders on any stop-scan" is intended or a defect.

Suggestions (non-blocking):

  • NewScanTracker (internal/progress/progress.go) inserts the tracker via NewTracker (isScan=false), then re-locks trackersMutex to set isScan=true. Narrow TOCTOU window + a wasted lock. Set the flag in a single locked insertion via a shared private constructor taking the flag.
  • The isScan bool on the generic Tracker is a domain (scan-vs-download) classifier bolted onto generic progress plumbing; consider whether the scan/generic distinction belongs where the cancel handler already knows the scanner/aggregator.

Coverage gap: no test exercises the stale/orphaned callback paths — e.g. register a callback for a not-scanning folder, run a fresh successful scan, and assert the panel is not wiped; and drive Scan() through an early-return guard and assert the callback is drained.

Security: no findings introduced. CI is currently red — please confirm failures are unrelated before merge.

— AI review

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants