feat(tree-view): show LS HTML tree view in Eclipse plugin [IDE-1837]#387
Open
bastiandoetsch wants to merge 41 commits into
Open
feat(tree-view): show LS HTML tree view in Eclipse plugin [IDE-1837]#387bastiandoetsch wants to merge 41 commits into
bastiandoetsch wants to merge 41 commits into
Conversation
…owser [IDE-1837] - Add TreeViewParams DTO mirroring SummaryPanelParams - Add SNYK_TREE_VIEW constant to LsConstants - Add @JsonNotification handler in SnykExtendedLanguageClient - Create TreeViewBrowserHandler (reuses ExecuteCommandBridge) - Add USE_HTML_TREE_VIEW preference gate (default false) - Host treeBrowser in SnykToolView.createPartControl behind preference - Implement updateTreeViewHtml delegating to treeBrowserHandler on SWT thread - Add TDD tests (T-S-001, T-I-001) verifying DTO and notification dispatch Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…[IDE-1837] - Guard snykTreeView() against null params/html at the LSP boundary - Skip dispatch when USE_HTML_TREE_VIEW preference is disabled (avoids invisible browser renders in the default-off configuration) - Add isDisposed() guard in TreeViewBrowserHandler.setBrowserText() - Remove leaking OSGi IPreferenceChangeListener (no UI toggle exists yet; preference is read once at createPartControl time) - Remove unused InstanceScope and Activator imports from SnykToolView - Remove totalIssues dead field from TreeViewParams (no caller) - Guard treeBrowserHandler null in updateTreeViewHtml - Expand tests: null params, null html, preference-disabled, and preference-enabled dispatch all verified (5 tests, 0 failures) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Add EclipseThemeCssProvider: maps SWT colors to --vscode-* CSS vars - TreeViewBrowserHandler.setBrowserText injects theme style block - Tests: EclipseThemeCssProviderTest (toHex, buildStyleBlock), TreeViewBrowserHandlerTest (theme injection, null no-op) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…-1837]
- Use pt unit for font-size (SWT FontData.getHeight() returns points, not px)
- Quote multi-word font family names to produce valid CSS font-family values
- Use rgba() for widget-shadow and list-hoverBackground to preserve
transparency intent (consistent with the LS template fallback values)
- Refactor doubled getSystemFont().getFontData()[0] calls into resolveFontData()
- Use replaceFirst("(?i)</head>") instead of replace() to avoid injecting
style block at multiple </head> occurrences
- Add toRgba, quoteFontFamily, font-size-unit, and rgba-fallback tests
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Remove toRgba regex that could corrupt alpha values >= 0.10/0.20; %.2f already produces correct output (0.12, 0.15) without post-processing - Remove Javadoc from ISnykToolView.updateTreeViewHtml (interface has no Javadoc on other abstract methods — consistent with project convention) - Add missing newline at EOF in LsConstants.java - Delete redundant TreeViewBridgeTest (tested ExecuteCommandBridge.buildClientScript which is already covered by ExecuteCommandBridgeTest; added no tree-view coverage) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/describe |
Contributor
Author
|
/review |
|
PR Description updated to latest commit (04c8394) |
This comment has been minimized.
This comment has been minimized.
…-1837] - Add TreeViewBrowserHandler.selectNode(issueId): injects window.__selectTreeNode__ into the HTML tree browser - Wire selectNode into SnykToolView: called on IssueTreeNode selection and from selectTreeNode(Issue, String) programmatic navigation - Buffer pending HTML in SnykToolView when treeBrowserHandler not yet initialized; drain buffer in createPartControl after handler init - Tests: selectNode null-safety (null browser, null/empty id)
… [IDE-1837] String.format with %.2f uses system locale by default; on non-US locales (e.g. Germany) this produces a comma as decimal separator, generating invalid CSS rgba() values. Fix by passing Locale.ROOT explicitly.
- Declare treeBrowserHandler volatile to make safe-publication explicit for background-thread reads in updateTreeViewHtml and selectTreeNode - Remove double selectNode call: selectTreeNode(Issue, String) no longer calls treeBrowserHandler.selectNode directly; the selectionChanged listener is the sole coupling point from JFace selection to HTML tree - Fix pendingHtml drain: call setBrowserText directly in createPartControl (already on UI thread) instead of via asyncExec to avoid stale-overwrite when a concurrent updateTreeViewHtml arrives before the drain executes - Extract escapeJsSingleQuotedString: extends escaping to \n, \r, U+2028, U+2029 which break single-quoted JS string literals - Remove selectTreeNode(String) from ISnykToolView interface (no external callers; removed dead implementation from SnykToolView) - Add 4 tests for escapeJsSingleQuotedString covering all escape cases Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…837] - Replace volatile String pendingHtml with AtomicReference<String> to eliminate NullAssignment PMD violation (priority 3) in createPartControl; AtomicReference.getAndSet(null) is both PMD-clean and provides stronger atomic semantics than volatile + manual null assignment - Sync settings-fallback.html from snyk-ls main (sha512 drift caused static-resource-checks CI failure; unrelated to IDE-1837 changes) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Replace catch (Exception e) in resolveFontData with direct call; the null/disposed guards above make the broad catch unnecessary. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ion [IDE-1837]
Add EclipseThemeCssProvider.buildVariableMap() returning a full map of all
--vscode-* variables to their Eclipse SWT colour values. Rewrite
TreeViewBrowserHandler.injectThemeCss() to do a single-pass regex
replacement of var(--vscode-xxx, fallback) expressions with resolved values,
and to strip the ${ideStyle}/${ideScript} placeholders instead of injecting
a :root block. buildStyleBlock() is retained for backward compatibility and
now delegates to buildVariableMap().
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…[IDE-1837] Remove the DIAGNOSTIC_SOURCE_SNYK_CODE-only guard from SnykShowFixUriDetails.isValid() so that OSS, IaC, and Secrets products are all accepted when the action is showInDetailPanel. Add a guard in SnykExtendedLanguageClient.showDocument() to return ShowDocumentResult(false) for any snyk:// URI with an unrecognised action instead of delegating to super.showDocument() which crashes with "No file system defined for scheme: snyk". Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/describe |
Contributor
Author
|
/review |
|
PR Description updated to latest commit (ec8d7f1) |
This comment has been minimized.
This comment has been minimized.
…[IDE-1837] - Increase testSnykTreeViewDispatchesHtmlToToolView timeout 5s→15s to accommodate macOS CI thread scheduling latency (test was passing at 5.155s, just over the 5000ms window) - Fix showDocument() to return CompletableFuture.completedFuture(false) instead of null when URISyntaxException occurs or issue is not in cache; lsp4j calls .thenAccept() on the returned future which NPEs on null Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
When toolView is already set, call updateTreeViewHtml synchronously instead of via CompletableFuture.runAsync — openToolView() is a no-op in that path anyway. The async path is only needed when the view must be opened first. This removes the test's dependency on thread-pool scheduling latency (was causing 15s+ delays on Ubuntu/macOS CI runners). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Normalize LS product codenames (oss/code/iac) to Eclipse cache keys (Snyk Open Source/Code/IaC) before cache lookup; the HTML tree sends short codenames via data-product attribute, but the cache is keyed on display names - Font size: use px unit (matching IntelliJ) instead of pt; SWT FontData.getHeight() returns points but CSS px is the correct unit for screen rendering at the browser's reference pixel - Font family: add system-ui, -apple-system, sans-serif fallback chain after the system font name, matching IntelliJ's ThemeBasedStylingGenerator Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…tion [IDE-1837] Replace custom --vscode-only regex with IntelliJ's ThemeBasedStylingGenerator CSS_PATTERN that handles both var(--xxx) usages and --xxx: declarations in a single pass. Declaration replacement bakes concrete values directly into the CSS property declarations, eliminating all --vscode-* variable references from the rendered HTML. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…837] - Font family/size now read from JFaceResources.DEFAULT_FONT (same theme source as BaseHtmlProvider) instead of SWT system font or hardcoded values; font changes with the Eclipse theme - Navigation: convert issue.filterableIssueType() via SCAN_PARAMS_TO_DISPLAYED before calling selectTreeNode; ContentRootNode.getProductNode() expects DISPLAYED_* format, not scan-params format, causing IllegalArgumentException Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…nteraction [IDE-1837] Replace the three-child SashForm (summaryBrowser + treeViewer + treeBrowser) with a two-child layout: summaryBrowser and a StackLayout Composite that wraps both tree controls. This eliminates any ambiguity between SashForm weight-tracking and control visibility: the SashForm always sees exactly two children and the StackLayout.topControl selects the active tree.
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (ce5ceb1) |
This comment has been minimized.
This comment has been minimized.
andrewrobinsonhodges-snyk
approved these changes
May 22, 2026
| @Override | ||
| public void updateTreeViewHtml(String html) { | ||
| if (treeBrowserHandler == null) { | ||
| pendingHtml.set(html); |
Contributor
There was a problem hiding this comment.
This func does not run on the UI thread (it's called from the SnykExtendedLanguageClient), but the pendingHTML setup runs on the UI thread (in createPartControl above).
If a notification arrives before the tree view is ready, this call will never do anything.
Contributor
There was a problem hiding this comment.
Ah i see the latest reviewbot also picked up on this
Contributor
Author
There was a problem hiding this comment.
Confirmed — this is the race where the notification arrives between the null-check and createPartControl's drain. Will fix by always posting to the UI thread: pendingHtml.set(html) then asyncExec to drain, so the null-check and drain are serialized with createPartControl on the same thread.
|
|
||
| // Create TreeViewer | ||
| treeViewer = new TreeViewer(verticalSashForm, SWT.BORDER); | ||
| treeViewer = new TreeViewer(treeStack, SWT.BORDER); |
Contributor
There was a problem hiding this comment.
We should lazy-create this instead of alway creating it, even when turned off.
| return null; | ||
| } | ||
| switch (product) { | ||
| case "oss": |
Contributor
There was a problem hiding this comment.
Should not use magic strings here.
| @BeforeEach | ||
| void setUp() { | ||
| Preferences prefs = Preferences.getTestInstance(new InMemoryPreferenceStore(), new InMemorySecurePreferenceStore()); | ||
| prefs.store(Preferences.USE_HTML_TREE_VIEW, "false"); |
Contributor
There was a problem hiding this comment.
Set to false to exercise the classic tree-viewer code path. Will add equivalent tests with USE_HTML_TREE_VIEW=true to cover the treeBrowserHandler.selectNode() branch.
Contributor
Author
There was a problem hiding this comment.
Set to false to exercise the classic tree-viewer code path. Will add equivalent tests with USE_HTML_TREE_VIEW=true to cover the treeBrowserHandler.selectNode() branch.
| return (ProductTreeNode) this.getChildren()[2]; | ||
| default: | ||
| throw new IllegalArgumentException("unknown product in tree"); | ||
| return null; |
Contributor
There was a problem hiding this comment.
Is this safe? What's wrong with the old behaviour?
Contributor
Author
There was a problem hiding this comment.
Safe — the old code threw IllegalArgumentException, but the caller uses getOrDefault with a raw fallback, so an unknown filterableIssueType can reach this switch. The call site in SnykToolView.selectTreeNode now guards if (productNode == null) return, so returning null is handled consistently without surfacing an exception.
| {{CHANNEL_STABLE_SELECTED}} - "selected" or "" | ||
| {{CHANNEL_RC_SELECTED}} - "selected" or "" | ||
| {{CHANNEL_PREVIEW_SELECTED}} - "selected" or "" | ||
| {{CHANNEL_CUSTOM_SELECTED}} - "selected" or "" |
Contributor
There was a problem hiding this comment.
This file needs updating in LS then copying to all the IDE plugins.
Contributor
Author
There was a problem hiding this comment.
Agreed — settings-fallback.html is owned by snyk-ls. Will update it there first, then copy the result to this plugin and the other IDE plugins as a follow-up.
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (ce5ceb1) |
…-1837] Merge origin/main (IDE-1652 LsSettingsRegistry + FolderConfigSettings refactor) into the IDE-1837 tree-view branch. Conflict resolutions: - Preferences.java: kept both constant sets (USE_HTML_TREE_VIEW / USE_LS_HTML_CONFIG_DIALOG from IDE-1837; RELEASE_CHANNEL_* / EXPLICIT_CHANGES_KEY from IDE-1652); dropped setDefault(RELEASE_CHANNEL) that IDE-1652 moved to LsSettingsRegistry. - SnykExtendedLanguageClient.java: merged both import sets (LsKey / LsSettingsRegistry from IDE-1652; TreeViewParams from IDE-1837). Also adds -javaagent pre-load of byte-buddy-agent via maven-dependency- plugin to allow Mockito inline mocking in containerised JVMs where ByteBuddy self-attachment is blocked by PID namespace restrictions.
…ests [IDE-1837] - Make toolView field volatile to ensure cross-thread visibility - Add null guard in showDocument() so a missing tool view returns ShowDocumentResult(false) instead of NPE - Add two regression tests that inject an issue into IssueCacheHolder and mock ResourceUtils so the supplyAsync block is actually reached, covering the null guard path and the happy path - Add updateTreeViewHtml buffering tests for pending-HTML when handler is null; drop the no-op third test that never exercised the guard branch
Contributor
Author
|
/describe |
1 similar comment
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (d83c72a) |
This comment has been minimized.
This comment has been minimized.
|
PR Description updated to latest commit (d83c72a) |
…UI-thread drain, html-view tests [IDE-1837] - Lazy-create treeBrowser/treeBrowserHandler behind useHtmlTreeView flag so no SWT Browser is allocated when the HTML tree view is disabled - Replace "oss"/"code"/"iac" literals in normalizeProductCodename with SCAN_PARAMS_OSS/CODE/IAC constants from ProductConstants - Fix updateTreeViewHtml race: always set pendingHtml first, then post a drain via asyncExec so the null-check and drain are serialized on the UI thread with createPartControl - Narrow drainPendingHtmlAsync catch from Exception|Error to UnsatisfiedLinkError|NoClassDefFoundError (SWT native absence) - Add SnykToolViewTest cases with USE_HTML_TREE_VIEW=true to cover the treeBrowserHandler.selectNode() branch
…uplicate test [IDE-1837] - Add SWTException to the catch in drainPendingHtmlAsync alongside UnsatisfiedLinkError/NoClassDefFoundError — Display.asyncExec() can raise SWTException(ERROR_DEVICE_DISPOSED) if the display is disposed in the TOCTOU window between the isDisposed() guard and the asyncExec call; without this catch it propagates to the LSP message handler - Remove updateTreeViewHtml_alwaysSetsPendingHtmlBeforeAsyncExec which was structurally identical to the existing buffer test (treeBrowserHandler is always null in headless tests so both assertions were equivalent)
Contributor
Author
|
/describe |
| } | ||
| }); | ||
| } | ||
| } catch (SWTException | UnsatisfiedLinkError | NoClassDefFoundError e) { // NOPMD — SWT unavailable in headless/test environments |
|
PR Description updated to latest commit (5843599) |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (5843599) |
This comment has been minimized.
This comment has been minimized.
… [IDE-1837] PMD UnnecessaryWarningSuppression CI failure: the // NOPMD annotation on the drainPendingHtmlAsync catch was left over from when the catch caught bare Exception|Error. After narrowing to SWTException|UnsatisfiedLinkError |NoClassDefFoundError, no PMD rule fires on those specific types, so the suppression itself became a violation. Remove the comment; the explanatory rationale is preserved in the adjacent logInfo message.
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (5e12eb3) |
This comment has been minimized.
This comment has been minimized.
PR Reviewer Guide 🔍
|
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
4 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.
User description
Description
Implements the Eclipse client side of the language server HTML tree view feature (IDE-1837). The language server already emits
$/snyk.treeViewnotifications containing a fully-rendered HTML tree; this PR wires that into an embedded SWTBrowserin the Snyk tool view.PR stack:
What this PR includes (Steps 2.1 + 2.2 + 2.3):
LSP wire-up (Step 2.1):
LsConstants.SNYK_TREE_VIEW— LSP notification method constantTreeViewParamsDTO — deserializes{ treeViewHtml }from LS@JsonNotificationhandler inSnykExtendedLanguageClient— guarded against null params, null HTML, and disabled preferenceTreeViewBrowserHandler— SWT Browser wrapper; installsExecuteCommandBridge(reused from settings page) so allsnyk.*commands from tree.js reach the LS viaworkspace/executeCommandPreferences.USE_HTML_TREE_VIEW— feature flag, default OFFTheme CSS (Step 2.2):
EclipseThemeCssProvider— maps 15 Eclipse SWT system colors to--vscode-*CSS variables; usesrgba()for transparency-sensitive properties, properly quoted font families,ptunits for font size,Locale.ROOTfor decimal formattingSelection sync + edge cases (Step 2.3):
TreeViewBrowserHandler.selectNode(issueId)— injectswindow.__selectTreeNode__into the HTML tree browser; full JS string escaping including\n,\r, U+2028, U+2029SnykToolViewwired to callselectNodewhen anIssueTreeNodeis selected in the legacy treependingHtmlbuffer (AtomicReference<String>) — stores HTML iftreeBrowserHandlernot yet initialized; drains increatePartControltreeBrowserHandlerdeclaredvolatilefor safe publication to background threadsChecklist
Screenshots / GIFs
UI is gated behind
USE_HTML_TREE_VIEW=truepreference (default OFF). Screenshots to follow when the feature is promoted to default.PR Type
enhancement
Description
Integrate HTML tree view using SWT Browser.
Map Eclipse themes to LS HTML styles.
Add preference and HTML buffering logic.
Enhance LS notification handling and JS bridging.
Diagram Walkthrough
File Walkthrough
9 files
Enhance CSS variable replacement and theming support.Add new preference for HTML tree view.Add method to update HTML tree view content.Integrate HTML tree view and manage UI stack.New handler for rendering HTML tree in SWT Browser.Add constant for HTML tree view notification.Handle `$/snyk.treeView` LSP notification.Improve URI validation logic.New message object for HTML tree view payload.1 files
Improve error handling in getProductNode.6 files
Add tests for CSS variable replacement and formatting.Add tests for HTML tree view buffering and null handlers.Add tests for JS string escaping and null handlers.Add tests for showDocument, product codename normalization.Add tests for URI validation and product codename mapping.Add tests for tree view notification handling.1 files
Document the new HTML tree view feature.1 files
Update test dependencies and agent configuration.