SLI-2850 Decouple editor highlighting from the Current File tool window - #1876
SLI-2850 Decouple editor highlighting from the Current File tool window#1876nquinquenel wants to merge 3 commits into
Conversation
Refresh editor squiggles from on-the-fly findings even when the Current File tab was never created, and clear markup for files that became clean.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
929f5c2 to
22687e0
Compare
22687e0 to
ccf4422
Compare
This comment has been minimized.
This comment has been minimized.
| fun refreshDisplayedFindings(file: VirtualFile?) { | ||
| val criteria = getService(project, SonarLintToolWindow::class.java).getCurrentFileFilterCriteria() | ||
| ?: FilterCriteria() | ||
| val filteredFindings = FindingsFilter(project).filterAllFindings(file, criteria) | ||
| getService(project, CurrentFileDisplayedFindingsStore::class.java).setSnapshot(filteredFindings) | ||
| } |
There was a problem hiding this comment.
🚨 Bug: Single-file filtered snapshot wipes squiggles in background open files
CurrentFileDisplayedFindingsStore holds one global FilteredFindings snapshot, and both writers populate it for a single file only: the refresher calls filterAllFindings(getSelectedFile(project), criteria) and the panel calls it for currentFile, with the default FindingsScope.CURRENT_FILE loading findings for that one file. DirectHighlighter.collectHighlightPlans now reads getFindingsForFile(file) from that snapshot for every file the coordinator refreshes. Trigger: files A (selected) and B (background) are open, an analysis raises issues for B → holder publishes enabled(changedFiles={B}) → the refresher rebuilds the snapshot for A → refreshFiles([B]) → getFindingsForFile(B) returns empty → all SonarQube markup on B is erased, and selecting B afterwards goes through panel.update(B, EditorHighlightRefresh.NONE) which sets the snapshot but never re-renders markup. Key the store per file (write a per-file map, or have the highlighter compute findings for the file it is rendering) instead of a single selected-file snapshot.
Fix 1: Store one snapshot per file so refreshing markup for a non-selected file does not read an unrelated file's snapshot
// CurrentFileDisplayedFindingsStore.kt
private val snapshotsPerFile = ConcurrentHashMap<VirtualFile, FilteredFindings>()
fun setSnapshot(file: VirtualFile?, findings: FilteredFindings) {
// index by the file each finding belongs to, so background files keep their own snapshot
findings.issues.map { it.file() }.plus(findings.hotspots.map { it.file() })
.plus(findings.taints.mapNotNull { it.file() }).plus(listOfNotNull(file)).distinct()
.forEach { f -> snapshotsPerFile[f] = findings.getFindingsForFile(f) }
}
fun getFindingsForFile(file: VirtualFile): FilteredFindings = snapshotsPerFile[file] ?: EMPTY
- Apply fix
Fix 2: Drop the shared snapshot and filter per rendered file at highlight time
// DirectHighlighter.collectHighlightPlans - compute for the file being rendered
val criteria = getService(project, SonarLintToolWindow::class.java).getCurrentFileFilterCriteria()
?: FilterCriteria()
val findings = FindingsFilter(project).filterAllFindings(file, criteria).getFindingsForFile(file)
- Apply fix
Check a box to apply a fix or reply for a change | Was this helpful? React with 👍 / 👎
| @Test | ||
| fun should_highlight_taints_from_the_taint_cache_without_a_findings_store() { | ||
| val content = "class Foo {}" | ||
| val file = createAndOpenTestPsiFile("Foo.java", content).virtualFile | ||
| val issueMessage = "SQL injection" | ||
| val expectedRange = textRangeOf(content, "Foo") | ||
| seedTaint(file, content, issueMessage) | ||
|
|
||
| withOpenEditor(file) { | ||
| val highlighter = getService(project, DirectHighlighter::class.java) | ||
| highlighter.applyHighlightsForTest(file) | ||
|
|
||
| val highlights = sonarLintHighlights(file, issueMessage) | ||
| assertThat(highlights).hasSize(1) | ||
| assertThat(highlights.single().startOffset).isEqualTo(expectedRange.first) |
There was a problem hiding this comment.
⚠️ Bug: Taint highlight test now fails: taints only come from the snapshot
should_highlight_taints_from_the_taint_cache_without_a_findings_store seeds a taint only into TaintVulnerabilitiesCache (seedTaint) and asserts one highlight, but this commit removed the TaintVulnerabilitiesCache read from collectHighlightPlans — taints now come exclusively from CurrentFileDisplayedFindingsStore, whose snapshot is never set in that test, so findings.taints is empty and the hasSize(1) assertion fails. Either seed the store in seedTaint (and rename the test, since its premise "without a findings store" no longer holds) or keep the taint cache as a highlight source.
Seed the displayed-findings store alongside the taint cache and rename the test to drop the "without a findings store" claim:
private fun seedTaint(file: VirtualFile, content: String, message: String) {
// ... existing taint construction ...
getService(project, TaintVulnerabilitiesCache::class.java).taintVulnerabilities = listOf(taint)
getService(project, CurrentFileDisplayedFindingsStore::class.java).setSnapshot(
FilteredFindings(emptyList(), emptyList(), listOf(taint), emptyList()),
)
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| @@ -467,7 +461,7 @@ class CurrentFilePanel(project: Project) : CurrentFileFindingsPanel(project) { | |||
| val files = when { | |||
| highlightRefresh.allOpenFiles -> FileEditorManager.getInstance(project).openFiles.toList() | |||
There was a problem hiding this comment.
⚠️ Bug: Panel re-gates highlight files by FindingsScope, contradicting the contract
resolveEditorHighlightFiles checks filterCriteria.findingsScope == FindingsScope.CURRENT_FILE before highlightRefresh.changedFiles, so with the default CURRENT_FILE scope a refreshView(EditorHighlightRefresh.enabled(changedFiles)) collapses the file set to listOfNotNull(file) and every other open file keeps stale markup. This directly contradicts EditorHighlightRefresh's own KDoc ("[changedFiles] ... honored whenever present", "the concrete set of files is resolved by OnTheFlyFindingsCoordinator, not the Current File panel") and the PR's stated contract of no FindingsScope on the highlight file set. Let the coordinator resolve the files (applyHighlightRefresh(highlightRefresh)) and drop the scope branch.
Delegate file resolution to the coordinator instead of re-gating it by the panel's findings scope:
private fun refreshEditorHighlights(highlightRefresh: EditorHighlightRefresh) {
getService(project, OnTheFlyFindingsCoordinator::class.java).applyHighlightRefresh(highlightRefresh)
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| fun refreshDisplayedFindings(file: VirtualFile?) { | ||
| val criteria = getService(project, SonarLintToolWindow::class.java).getCurrentFileFilterCriteria() | ||
| ?: FilterCriteria() | ||
| val filteredFindings = FindingsFilter(project).filterAllFindings(file, criteria) | ||
| getService(project, CurrentFileDisplayedFindingsStore::class.java).setSnapshot(filteredFindings) | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: Filter criteria read off the EDT from the analysis publish thread
OnTheFlyFindingsHolder.updateViewsWithNewIssues / updateOnAnalysisResult run on the backend RPC/analysis thread and now reach coordinator.applyHighlightRefresh → refreshDisplayedFindings → SonarLintToolWindow.getCurrentFileFilterCriteria, which touches ToolWindowManager/ContentManager.findContent and then reads live Swing state (filtersPanel.filterText, quickFixCheckBox.isSelected, findingsScope) from that background thread — a platform threading violation and a race with concurrent EDT filter edits. Read the criteria on the EDT (or cache the last criteria in a thread-safe holder the panel updates on the EDT) before computing the snapshot.
Fetch the panel's filter criteria on the EDT before filtering off-thread:
fun refreshDisplayedFindings(file: VirtualFile?) {
val criteria = computeOnEdt(project) {
getService(project, SonarLintToolWindow::class.java).getCurrentFileFilterCriteria() ?: FilterCriteria()
}
val filteredFindings = FindingsFilter(project).filterAllFindings(file, criteria)
getService(project, CurrentFileDisplayedFindingsStore::class.java).setSnapshot(filteredFindings)
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| /** | ||
| * Builds the list of highlights to render for [file] from the findings currently displayed in the tool window. | ||
| * Reads the shared snapshot rather than the raw analysis so that editor highlights always match what the user | ||
| * sees in the Current File tab (same filtering, same resolved/new-code handling). | ||
| * Builds the list of highlights to render for [file] from the filtered findings snapshot shown in the Current File | ||
| * tab. Resolved findings are still skipped, and CAYC styling is still applied. | ||
| */ | ||
| private fun collectHighlightPlans(file: VirtualFile): List<HighlightPlan> { | ||
| val findings = getService(project, CurrentFileDisplayedFindingsStore::class.java).getFindingsForFile(file) |
There was a problem hiding this comment.
⚠️ Quality: Diff contradicts the PR's stated contract on filter decoupling
The PR description's implementation contract states the highlighter reads unfiltered coordinator issues/hotspots plus TaintVulnerabilitiesCache, that CurrentFileDisplayedFindingsStore is deleted, and that "squiggles do not follow search/severity/scope"; this commit reinstates the store as the highlighter's only source and renames the guarding test to should_hide_editor_squiggle_when_a_list_filter_hides_the_issue, so a text filter in the Findings list once again hides editor squiggles. Either update the description/ticket scope to record this reversal as intentional, or restore the unfiltered read path so the decoupling the PR advertises actually holds.
Align the stated contract with the code (or revert the code to the stated contract):
// Update the PR description's "Implementation contract" to state:
// - Highlighter reads the filtered snapshot published by CurrentFileDisplayedFindingsRefresher
// - Editor squiggles DO follow Current File tab filters (search/severity/scope)
// ... or revert DirectHighlighter.collectHighlightPlans to the holder + TaintVulnerabilitiesCache read.
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| fun applyHighlightRefreshAndRefreshPanels(highlightRefresh: EditorHighlightRefresh) { | ||
| applyHighlightRefresh(highlightRefresh) | ||
| if (!project.isDisposed) { | ||
| getService(project, SonarLintToolWindow::class.java).refreshViews() | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Performance: Highlight refresh and filtering run twice per resolve/CAYC/binding event
applyHighlightRefreshAndRefreshPanels calls applyHighlightRefresh (snapshot rebuild + refreshFiles) and then refreshViews(), which now calls panel.refreshView(EditorHighlightRefresh.enabled()) whose finally block calls applyHighlightRefresh again — so every mark-as-resolved, reopen, CAYC toggle and binding change re-filters all findings and re-queues markup twice, the second time on the EDT. Pass EditorHighlightRefresh.NONE from refreshViews() (the coordinator already handled markup) and keep the panel's highlight trigger for callers that only rebuild the panel.
Let the coordinator own the markup pass so panel rebuilds do not duplicate it:
public void refreshViews() {
this.<CurrentFilePanel>updateCurrentFileTab(panel -> panel.refreshView(EditorHighlightRefresh.NONE));
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Comment gitar unblock to override this block and allow merging.
Configure merge blocking · Maintainers can dismiss this review.
CI failed: UI integration test failures across IDE templates due to timeouts waiting for expected issue count text caused by editor highlighting and tool window decoupling changes.OverviewTwo UI integration test jobs failed across different IDE environments (IdeaUltimate2024 and PyCharmProfessional2024) due to remote-robot timeout exceptions waiting for the text 'Found 1 issue' in the UI fixture, directly relating to the PR's changes in decoupling editor highlighting from the Current File tool window. FailuresIdeaUltimateTests UI Timeout (confidence: high)
PyCharmProfessionalTests UI Timeout (confidence: high)
Summary
Code Review 🚫 Blocked 7 resolved / 13 findingsDecouples editor highlighting from the Current File tool window, but several critical issues block merge: a single-file filtered snapshot wipes squiggles in background open files (reuse the snapshot per-file instead); the panel re-gates highlight files by FindingsScope, contradicting the stated contract (drop the scope branch); filter criteria are read off the EDT from the analysis thread, a platform threading violation (read on EDT or cache in a thread-safe holder); the diff reinstates the filtered store as the highlighter's only source, contradicting the PR description's stated decoupling contract (clarify intent or restore unfiltered reads); a taint highlight test now fails because taints only come from the snapshot (seed the store or keep the taint cache as a source); and highlight refresh and filtering run twice per event (pass EditorHighlightRefresh.NONE from refreshViews() to avoid duplicate work). 🚨 Bug: Single-file filtered snapshot wipes squiggles in background open files📄 src/main/java/org/sonarlint/intellij/ui/currentfile/CurrentFileDisplayedFindingsRefresher.kt:37-42 📄 src/main/java/org/sonarlint/intellij/analysis/OnTheFlyFindingsCoordinator.kt:49-56 📄 src/main/java/org/sonarlint/intellij/editor/DirectHighlighter.kt:244-258 📄 src/main/java/org/sonarlint/intellij/ui/currentfile/CurrentFilePanel.kt:382
Store one snapshot per file so refreshing markup for a non-selected file does not read an unrelated file's snapshotDrop the shared snapshot and filter per rendered file at highlight time
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
1 participant
Summary
OnTheFlyFindingsCoordinatorrefreshesDirectHighlighterfrom unfiltered on-the-fly findings (plus taints). Files that just became clean are cleared without close/reopen.CurrentFilePanelis UI-only (list, filters, gutter).CurrentFileDisplayedFindingsStoreis deleted.SonarLintToolWindow.refreshViewshighlights via the coordinator first, then updates the tab if it is present.Implementation contract
OnTheFlyFindingsHolderkeeps the per-open-file maps.TaintVulnerabilitiesCache, not a filtered UI snapshot. NoFindingsScopeon the highlight file set.EditorHighlightRefresh.NONEif the panel exists.raiseIssues/ analysis-result replace per file including empties;analyzedFiles∩ open files with no findings are stored empty.Review
openFilesfallback is unchanged; one test does not actually apply a panel filter.Test plan
./gradlew :test --tests org.sonarlint.intellij.analysis.OnTheFlyFindingsCoordinatorTests --tests org.sonarlint.intellij.editor.DirectHighlighterTests --tests org.sonarlint.intellij.actions.ClearCurrentFileIssuesActionTests --tests org.sonarlint.intellij.editor.CodeAnalyzerRestarterTests