feat(notifications): add hosted file-based in-IDE notifications - #566
Merged
araarush merged 10 commits intoJul 23, 2026
Conversation
Adds a client-side notifications feature to the Eclipse plugin, matching the schema and behavior of the other Amazon Q IDE plugins. On startup (after the language server is ready) a background poller fetches a hosted JSON file over HTTPS every 10 minutes and shows targeted in-IDE toasts. - Schema 2.x "combined" payload model + a polymorphic condition DSL (==, !=, >, >=, <, <=, anyOf, noneOf, and, or, not) with a single Jackson deserializer. - Rules engine gates display on compute/os/ide/extension/authx conditions (semver for ide/extension versions; SNAPSHOT builds and not-installed extensions are never shown). - ETag-cached fetcher that degrades gracefully: any failure (absent file, 403/404, empty, malformed, offline) resolves to "show nothing" and only logs. Supports a file:// endpoint for local testing. - Toasts: Info/Warning auto-dismiss; Critical persists until dismissed. Actions: ShowUrl, UpdateExtension, OpenChangelog, plus More and Dismiss. Dismissals persist for 60 days; emergencies re-show until dismissed. - User preference "Show Amazon Q notifications" (default on) as a kill switch; telemetry (toolkit_showNotification / toolkit_invokeAction) is independent of notification polling and respects the telemetry opt-in. - 42 unit tests covering parsing, the DSL, rules, fetch/degradation, dismissal, and filtering/dedup. Wired into LspStartupActivity (start) and Activator.stop (clean shutdown).
The OpenChangelog notification action opened https://github.com/aws/amazon-q-eclipse/blob/main/CHANGELOG.md, which 404s (there is no CHANGELOG.md in the repo). Point AMAZON_Q_CHANGELOG_URL at the GitHub releases page (the de-facto changelog for the repo) so 'View changelog' resolves.
Replace an internal-only reference in the CRITICAL-persistence javadoc with a provider-neutral phrasing; no behavior change.
Contributor
Author
Screen.Recording.2026-07-16.at.11.05.01.AM.movDemonstration of notification functionality |
ashishrp-aws
left a comment
ashishrp-aws
left a comment
There was a problem hiding this comment.
Multi-persona review (Principal SDE / Senior SDE / code quality / test coverage / release-risk personas) surfaced several issues. Posting critical → medium findings as inline comments. Full findings summary below.
Critical
- ETag is read/removed but never written after a successful 200 (
NotificationsFetcher) — conditional GET is dead code; every poll does a full download and the 304 branch is unreachable in production. - Semver comparison falls back to lexical string compare for real Eclipse versions (
RulesEngine.compareSemver) —CLEAN_SEMVERregex rejects qualifier suffixes like4.30.0.v20231201-0110, soide.versioncomparisons will almost always use lexical ordering, which is wrong for numeric versions (e.g."9.0" > "10.0"lexically).
High
- First poll runs synchronously in
start(), blocking a shared worker-pool thread for up to ~37s worst case (30s timeout × retries) — competes with other post-startup work. - Opt-out (default=true) recurring HTTPS polling every 10 minutes, independent of existing telemetry/data-sharing consent — worth reconsidering the default or documenting clearly.
- "Safe no-op" claim doesn't fully hold — network failures (not 404s) emit
FAILEDtelemetry every 10 minutes indefinitely on networks that block the new endpoint.
Medium
- Corrupt dismissal state reset in-memory but not persisted (
NotificationDismissalStore.loadAndClean) — repeated warn-logging until nextdismiss()call. Thread.sleepin retry backoff blocks a shared worker-pool thread for up to ~3s.- No dedicated test coverage for
NotificationPollingService— the most lifecycle-sensitive class in the PR (start/stop/reschedule races) is untested.
Inline comments below point to the specific lines.
Correctness/robustness fixes surfaced by review of the notifications feature: - ETag: persist the response ETag after a successful 200 so the conditional GET (If-None-Match) actually works; previously the ETag was never written, so every poll re-downloaded and the 304 path was unreachable. Also clean up the temp cache file if the atomic move fails. - Version targeting: sanitize the plugin Bundle-Version to major.minor.micro (drop the OSGi qualifier) before rules evaluation, so extension.version conditions compare with semver instead of lexical ordering (e.g. 2.7.4 < 2.7.10). - Dev builds: suppress polling on unreleased/dev builds (qualifier == "qualifier" or contains "snapshot") unless an explicit endpoint override is set, so dev builds don't receive production notifications while local testing still works. - STARTUP-once: consume the startup window only when a STARTUP notification actually renders, not merely when the first poll runs, so one filtered on the first poll can still show later in the session once it qualifies. - Telemetry accuracy: emit showNotification and mark the id shown only after the toast actually renders (completion callback); on skip/failure the id is un-marked so it retries on a later poll. - First poll no longer runs synchronously in start(); it is scheduled so start() does not block the shared startup worker thread on network I/O. - Batch resilience: skip null notification elements and isolate per-notification processing so one bad entry cannot abort the whole poll. - Severity parsing is case-insensitive (a mis-cased "critical" no longer downgrades to an auto-fading INFO toast). - Tighten the stop()/reschedule() race so a poll cannot be armed after shutdown. Adds tests: ETag write + If-None-Match, 304-with-cache, 500/offline degradation, clean-version extension targeting, case-insensitive severity, STARTUP filtered-then- qualifies, startup-window-after-render, render-failure retry, null batch element.
… bound fetch - Kill-switch is now reversible in-session: a preference-change listener pauses polling when disabled and resumes it when re-enabled, instead of requiring an IDE restart. NotificationPollingService is refactored into a restartable lifecycle (start / pause via onEnabledPreferenceChanged / permanent shutdown) with an injectable scheduler + suppliers. - Dismissal store: persist the reset when stored state is corrupt (so it stops re-parsing and re-warning on the bad value every poll), and make the id comparison null-safe so a stored entry with a null id cannot NPE and abort all notification processing. - Fetch: tighten the per-request timeout (30s -> 10s) and backoff base (1s -> 0.5s) so a poll cannot occupy a shared worker-pool thread for tens of seconds on a slow/blocked network. Adds NotificationPollingServiceTest (start-once, kill-switch off/on resume, dev-build gating, reschedule-at-interval, shutdown-cancels-and-prevents-rearm, start-after-shutdown) and dismissal tests for corrupt-state repair and null-id safety.
…e coverage - Cap the fetched payload at ~1MB so a mis-pointed or oversized endpoint cannot buffer an arbitrary amount into memory and attempt to parse it. - Rules engine: make equality/ordering operators null-safe on the actual system value so a null os.version/ide.version (etc.) evaluates to a non-match instead of throwing NPE mid-poll. - Document that authx.ssoScopes is intentionally a no-op until Eclipse collects SSO scopes, so payloads don't rely on it for targeting. Tests: oversized-payload ignored, null-actual-value non-match (+ null os.version ordering), null/duplicate-id and non-array-anyOf parsing behavior.
Follow-ups from an adversarial re-review of the hardening commits (no functional bugs were found; these are robustness/clarity refinements): - NotificationPollingService.start(): set running=true only after the scheduler actually accepts the first poll, so a worker-pool rejection at startup leaves the service restartable instead of latched into a started-but-never-scheduled state. Add a test for the rejection-then-retry path. - Remove a dead post-assignment shutdown recheck in reschedule() (both methods are synchronized on the same monitor, so it could never fire) and clarify the comment. - Make the dismissal corrupt-state test deterministic: seed the wrong-shape value through the same putObject/getObject byte path the store uses, so Gson reliably throws, rather than relying on Base64-decoding-of-non-Base64 behavior. - Drop a vacuous assertion in the polling test.
Regression from the earlier lifecycle refactor: the first poll is scheduled with delay 0, so on a real ScheduledExecutorService the pool thread can run pollOnce() before start() returns. pollOnce() early-returns unless running==true, and running was being set AFTER scheduling — so the very first poll could silently no-op (no fetch, no toast, no reschedule, no log). Set running=true before scheduling (the field is volatile so the poll thread observes it) and roll it back only if the scheduler rejects the task. The prior unit tests used a scheduler that deferred execution, so they never reproduced the delay-0 inline-execution race. Add InlineScheduler + firstPollExecutingInlineAtScheduleTimeStillRuns, which runs the delay-0 task synchronously at schedule time and fails against the old ordering.
…Eclipse 4.40+) The plugin bundles Apache HttpClient 4.x (via the AWS SDK apache-client), which requires org.apache.commons.logging at runtime. That was satisfied by a platform Require-Bundle on 'org.apache.commons.logging'. Newer Eclipse (4.40+) renamed that Orbit bundle to 'org.apache.commons.commons-logging', so the old Require-Bundle no longer resolves and the ENTIRE plugin fails to load on current Eclipse. Fix: bundle commons-logging.jar as a plugin lib like the other third-party deps (add commons-logging to maven-dependency-plugin includeGroupIds + Bundle-Classpath) and drop the platform Require-Bundle. The plugin now self-supplies commons-logging and no longer depends on the platform bundle's name, so it loads on 4.32 (baseline) through 4.40+ regardless of Orbit renames. Verified: built jar contains target/dependency/commons-logging.jar; installed into a bare Eclipse 4.40 dropins/ (which lacks the old bundle name) -> plugin loads cleanly, no unresolved- bundle errors, notifications render. 499 tests still green.
…es off-screen shift) A notification toast could be shifted right, off the edge of the screen. Root cause was in repositionNotifications() (run on close()): it derived a single x from the CLOSING shell's width and applied it to every surviving toast, ignoring that each survivor has its own width. Because toast widths differ (a short Info toast is ~250px; a Critical toast with an action-button row hits the 400px cap) and Info toasts auto-dismiss while Critical toasts persist, the common case was a narrow toast closing while a wide one remained -> the wide toast was placed at the narrow toast's x and its right edge hung ~150px off-screen. Unlike the open path (Mylyn's open() self-corrects via fixupDisplayBounds), reposition had no such guard. Fix: right-align and stack each surviving toast by its own getShell().getSize(), mirroring initializeBounds(). Also floor x at the client-area left edge in both initializeBounds() and repositionNotifications() as belt-and-suspenders against any residual width/DPI skew, so a toast can never be pushed off-screen to the right.
araarush
marked this pull request as ready for review
July 22, 2026 16:33
ashishrp-aws
approved these changes
Jul 22, 2026
laileni-aws
approved these changes
Jul 23, 2026
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
3 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.
Summary
Adds hosted file-based in-IDE notifications to the Amazon Q Eclipse plugin, bringing it to parity with the
other Amazon Q IDE plugins (VS Code, JetBrains, Visual Studio), which already support this capability. This lets
operators proactively notify users of known issues and available fixes directly in the IDE, without requiring
users to check GitHub or external channels.
On startup (after the language server is ready), a background service polls a hosted JSON file over HTTPS every
10 minutes and shows targeted in-IDE toast notifications based on configurable display conditions.
What it does
reusing the existing
HttpClientFactory/ manifest-fetcher patterns. The fetcher is total (never throws) anddegrades gracefully — an absent/404/empty/malformed endpoint, or a network failure, resolves to "show nothing"
and is only logged (no error dialog, no crash). Supports a
file://endpoint for local development.condition expression language (
==,!=,>,>=,<,<=,anyOf,noneOf,and,or,not).compute/os/ide/extension/authxconditions. Uses semanticversion comparison for IDE and extension versions; development (SNAPSHOT) builds and not-installed extensions
are never shown.
(open URL / update / open changelog) plus a "More" dialog and an explicit "Dismiss". Dismissals persist for
60 days; emergency notifications re-show until dismissed.
toolkit_showNotification/toolkit_invokeAction, independent of notification polling andrespecting the existing telemetry opt-in.
Scope
separately and is not part of this change; until that content exists the client is a safe no-op (verified).
notificationspackage; the only edits to existing files are minimal wiring(start polling on startup, stop cleanly on shutdown) plus two preference constants and two URL constants.
Testing
rules engine (all operators, semver, SNAPSHOT/none-installed rules, auth matching), the fetcher's full
degradation matrix (200/304/404/malformed/timeout/
file://) via an injected mockHttpClient, dismissalpersistence + cleanup, and the filtering/dedup/startup-once logic.
degradation confirmed on macOS, Windows, and Linux (all three SWT backends), across the oldest and newest
supported Eclipse releases. Both severities, all action buttons, persistence, and error-log cleanliness
verified on each.
Notes for reviewers
delivered out-of-band and the client shows nothing until it exists.