Repository navigation
fix: open the What's New page only once its feed has loaded - #2102
ralphstodomingo wants to merge 4 commits into
Conversation
An automatic open after a minor upgrade used to create the panel first and fetch the changelog feed afterwards. On a machine where altimate.ai is blocked (corporate proxy or firewall, no DNS, slow link) the page then opened straight into its error state, and the failure was logged and reported as an error event under the command name. - `checkAndShowOnActivation` now fetches the manifest before opening; when the fetch fails and nothing is cached the page stays closed, and the fetched manifest is served to the page so it does not fetch twice. - A failed fetch is reported as `whats_new_feed_unavailable` with a reason class (`http_<status>`, `timeout`, `dns`, `proxy`, `network`, `tls`, `other`), the trigger and whether the cached manifest stood in; the message itself is not sent. - The page's manifest request is answered directly, so a blocked feed is a failed response rendered as the existing offline state, not a `dbtTerminal.error`. - Debug log lines when the page opens and when its manifest is served, so a support log shows what happened. Tests: trigger suite covers the closed-when-unreachable and fetch-first paths (both fail on master); a new suite covers the reason classes, the failed response, the cache fallback and the pre-fetched manifest. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AhtJPr3nmT1Po92ZQk4VE
|
@codex review against the numbered claims (C1–C7) and the disclosed residuals (R1–R3) in this PR's description: report only a reproducible trace that violates a numbered claim, or a defect the residuals do not already disclose. Scope: the diff of this PR (whatsNewPanel.ts, events.ts, dbtPowerUserExtension.ts and the two test files). Do not re-review unrelated code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe What's New panel now checks activation asynchronously, shares pending manifest fetches with manual page requests, and reports classified feed failures through telemetry. Tests cover cached fallback, activation decisions, and page behavior during pending fetches. ChangesWhat's New Feed
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant DBTPowerUserExtension
participant WhatsNewPanel
participant ManifestFeed
participant WebviewPanel
DBTPowerUserExtension->>WhatsNewPanel: Start checkAndShowOnActivation without awaiting
DBTPowerUserExtension->>DBTPowerUserExtension: Continue dbt detection and project initialization
WhatsNewPanel->>ManifestFeed: Fetch manifest
ManifestFeed-->>WhatsNewPanel: Return manifest or fetch failure
WhatsNewPanel->>WebviewPanel: Open automatically after successful fetch when eligible
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the primary change: delaying the What's New page until its feed has loaded. |
| Description check | |
The description provides a detailed problem statement, solution, testing evidence, limitations, telemetry context, and verification results. It does not use the exact template headings and omits the c… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 5… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3da3399d93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // fetchManifest has logged the failure and reported its reason. | ||
| return; | ||
| } | ||
| this.show("auto"); |
There was a problem hiding this comment.
On a minor upgrade with a slow activation fetch, invoke Show What's New after activation returns but before that fetch settles: the manual panel's first getWhatsNewManifest request sees preloaded as undefined and starts a second fetch. When the activation fetch then resolves, this call only reveals the existing panel, so its manifest does not serve that first request and may remain stale in preloaded. This reproducibly violates C2; share the in-flight fetch with manifest requests or account for an already-open panel before storing the preload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea41bb0. The activation fetch is now kept as a pending promise that the page's request awaits, so either opener costs one network fetch. When the user opened the page while it was in flight, the automatic open is skipped (their page is already in front, and its request took the shared fetch); a failed fetch is dropped so a later manual open fetches afresh. Test: "a manual open while the activation fetch is in flight shares that fetch" (fetchManifest called once, the response carries the shared manifest, no second open).
| this.dbtProjectContainer.initializeWalkthrough(); | ||
| this.whatsNewPanel.checkAndShowOnActivation(); | ||
| // Settles once the open decision is made; it never rejects. | ||
| void this.whatsNewPanel.checkAndShowOnActivation(); |
There was a problem hiding this comment.
On a minor upgrade where the feed resolves but show("auto") throws—for example, a test can stub show to throw—showWhenFeedAvailable does not catch that exception, so checkAndShowOnActivation() rejects. This fire-and-forget call has no rejection handler, producing an unhandled rejection instead of the former synchronous exception being caught by activate's surrounding try; this directly violates C6. Await the promise or attach a catch that reports the failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea41bb0. checkAndShowOnActivation wraps its whole body (state read/write, setting, fetch, show) in try/catch and logs through dbtTerminal.error, so the fire-and-forget call at activation cannot reject; a show() failure also drops the pending fetch. Tests: "a page that fails to open is logged, and activation still settles" and "a failing state read is logged, and activation still settles".
…activation check Review follow-ups: - The fetch started for an automatic open is kept as a pending promise that the page's own request awaits, so a `Show What's New` run while that fetch is in flight shares it instead of fetching again; the automatic open is then skipped because the page the user asked for is already in front. A failed fetch is dropped so a later manual open fetches afresh. - `checkAndShowOnActivation` wraps its whole body in try/catch and logs through the terminal, so the fire-and-forget call at activation cannot reject; a failing `show` also drops the pending fetch. Tests: in-flight fetch shared by a manual open (one fetch, no second open), a throwing `show`, a throwing state read. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AhtJPr3nmT1Po92ZQk4VE
|
@codex round 2: the push since round 1 is ea41bb0 (the activation fetch is shared with a manual open as a pending promise; the activation check never rejects; three tests). Re-check C2 and C6 against that commit and report only a reproducible trace that violates a numbered claim. Do not re-review the PR as a whole. |
Bundle Size Reportdarwin-arm64: 75.7 MB
linux-x64: 77.6 MB
win32-x64: 78.0 MB
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea41bb0f5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const openedMeanwhile = this._panel !== undefined; | ||
| if (this.pending === fetch && (!fetched || openedMeanwhile)) { | ||
| this.pending = undefined; | ||
| } | ||
| if (!fetched || openedMeanwhile) { |
There was a problem hiding this comment.
On a minor upgrade with a deferred activation fetch, manually open the page, let its manifest request capture pending, then close the panel before the fetch resolves. onDidDispose resets _panel, so this check concludes that no manual open occurred and creates a new automatic panel; the old request's finally then clears pending and posts its response to the new panel under an unknown request ID, causing the new panel's first request to start a second network fetch. This close-before-resolution trace is fresh evidence against the ea41bb0 fix and still violates C2.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1423e2b. The panel now records when a page's request has taken the pending fetch (pendingTaken), and the automatic open stays away in that case even if the user closed the page before the fetch settled, so no second panel is created and no second fetch happens; the old request's response goes to the disposed webview (_panel?.webview?.postMessage, a no-op). Test: "a manual page closed before the shared fetch settles is not reopened" (open, take, close, settle → no automatic open, one fetch, nothing pending); it fails on ea41bb0.
…was in flight A page the user opened during the activation fetch takes that fetch for its own request. If they close it before the fetch settles, the panel is gone by the time the automatic open decides, and it would bring the page back. The panel now remembers that its pending fetch was taken by a page, and the automatic open stays away in that case too. Test: manual open, request takes the fetch, panel closed, fetch settles → no automatic open, one fetch, nothing pending (fails on the previous commit). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AhtJPr3nmT1Po92ZQk4VE
|
@codex round 3 (last): the push since round 2 is 1423e2b (a flag records that a page took the pending fetch; the automatic open is skipped on that flag as well as on an existing panel; one test). Re-check C2 against that commit only and report a reproducible trace that violates a numbered claim; do not re-review the PR as a whole. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1423e2b70e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const openedMeanwhile = this.pendingTaken || this._panel !== undefined; | ||
| if (this.pending === fetch && (!fetched || openedMeanwhile)) { | ||
| this.pending = undefined; |
There was a problem hiding this comment.
A reproducible C2 violation remains when the manual panel is created before the activation fetch settles, but its webview has not yet sent getWhatsNewManifest: set _panel, resolve the deferred fetch, await activation, and only then call serveManifest. This branch sees pendingTaken === false but _panel !== undefined, clears pending, and the subsequent request calls fetchManifest again, producing two network fetches. This is fresh evidence distinct from the closed-after-taking trace addressed by 1423e2b: merely having a panel does not mean its asynchronously initialized page has taken the pending promise.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f6d2614. A successful activation fetch is now kept for whichever page asks next: the automatic open stands down on a flag that any manual open sets while the fetch is in flight (userOpenedMeanwhile), and clears pending only when the fetch failed. A panel disposed before its page asked drops the pending fetch, so a later open is not served a stale one. Tests: "a manual page still loading when the fetch settles takes it afterwards" (one fetch, the page's later request gets the manifest) and "a manual page closed before it ever asked is neither reopened nor served stale"; both fail on 1423e2b. This was the third review round, which is the cap for this PR; a further round is the maintainer's call.
Review follow-up: when the user opened the page during the activation fetch but its webview had not yet asked for the manifest, the automatic open cleared the pending fetch on the strength of the panel existing, and the page then fetched again. - The automatic open now stands down on a flag set by any manual open during the fetch, and clears the pending fetch only when it failed; a successful fetch stays for whichever page asks next. - A panel disposed before its page asked drops the pending fetch, so nothing stale is served to a later open. Tests: manual page still loading when the fetch settles takes it afterwards (one fetch); manual page closed before it ever asked is neither reopened nor served stale. Both fail on the previous commit. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AhtJPr3nmT1Po92ZQk4VE
E2E results (head f6d2614 vs master 3d0930a)Real VS Code 1.141.1 on Linux. Automatic rows: plain launches of the extension development host on an empty dbt project with the stored last-seen version rewritten to 0.64.7 (a minor behind) and the cached manifest cleared; "blocked" sets
Unit: 52 tests in |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge I traced the full caller chain for the feed-fetch / auto-open / manual-open interplay across Files Reviewed (5 files)
Reviewed by deepseek-pro-latest · Input: 71.6K · Output: 24.7K · Cached: 567.2K Review guidance: REVIEW.md from base branch |
|
@codex round 4, scoped to the last push only: f6d2614 (the automatic open stands down on a flag set by any manual open during the activation fetch; a successful fetch is kept for whichever page asks next; a panel disposed before asking drops it). Re-check C2 against that commit and report only a reproducible trace that violates a numbered claim; do not re-review the PR as a whole. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
2 participants
What
After a minor-version upgrade the extension auto-opens the "What's New" page. On a machine where the changelog feed (
https://altimate.ai/changelog.rss.xml) cannot be fetched (corporate proxy or firewall, no DNS, slow link) the page opened straight into its error state, and the failure was logged and reported as an error event under the command namegetWhatsNewManifest. That was the first thing such users saw after updating to 0.65.0: on 13 and 14 October about 8% of the automatic opens failed this way (251 and 314 machines; 403 and 10 s timeouts dominate, the feed itself answers 200 from outside).This PR:
whats_new_feed_unavailablewith a reason class (http_<status>,timeout,dns,proxy,network,tls,other), the trigger and whether the cached manifest stood in (the message text is not sent);dbtTerminal.error;The manual command (
Show What's New) still opens the page the user asked for and shows the existing error state when the feed is unreachable.Claims
show("auto")is called only afterfetchManifestresolved (fresh or cached manifest); if it rejects the page is not opened and the last-seen version is still recorded.getWhatsNewManifestrequest without a second network fetch, and is then dropped.whats_new_feed_unavailableevent with{reason, trigger, fallback};reasonis one ofhttp_<3 digits>,timeout,dns,proxy,network,tls,other; the error message is not a dimension.fallback: "cache"), both for automatic and manual opens.getWhatsNewManifestrequest never goes throughhandleSyncRequestFromWebview: a failure yields{status: false, error}to the page and adbtTerminal.warn, neverdbtTerminal.error; a success yields{status: true, data}and the existingwhats_new_page_openedevent.checkAndShowOnActivationkeeps its synchronous side effect (the version is persisted before any await) and its returned promise never rejects.dbt.showChangelogOnUpdate = falsesetting do not touch the feed at all.Residuals
node-fetchthrough the extension host; proxy authentication (407) is not addressed here. The newproxyreason class makes that slice countable.Verification
Unit (jest)
whatsNewTrigger.test.ts: the harness now awaits the activation decision; new cases "keeps the page closed when the feed is unreachable and nothing is cached", "fetches before opening and keeps the manifest for the page", "does not touch the feed when there is nothing to open". The first two fail on master's panel source (showis called before any fetch).showand a throwing state read are logged while activation still settles. Each of these failed on the commit before its fix.whatsNewFeed.test.ts(new):feedFailureReasonmatrix (13 inputs), fetched manifest served + cached + page-open event, blocked feed → failed response +whats_new_feed_unavailable+ nodbtTerminal.error, cache fallback, pre-fetched manifest served without a fetch, timeout classed astimeout.E2E (real VS Code 1.141.1, Linux)
Plain (non-test-mode) launches of the extension development host on an empty dbt project, because VS Code's extension-test mode keeps global state in memory and cannot replay an upgrade across launches. Each launch: the extension's stored last-seen version rewritten to 0.64.7 (a minor behind 0.65.1) and the cached manifest cleared; "offline" sets
http.proxyto a dead port withhttp.proxySupport: "override"so the feed fetch fails the way a blocked network does; "online" removes it. Assertions read the extension's own log (--log debug).[error] getWhatsNewManifest: … Failed to establish a socket connection to proxies …[warning] whatsNew:fetchManifest: … (proxy): …, no[error]Parsed 11 dbt-power-user entries(fetched on the page's request)Parsed 11 entries→Opening the What's New page (trigger=auto)→Serving 11 entries to the page (fetched before opening), one fetch[error] getWhatsNewManifest×2[warning] … (proxy), 0[error]linesThe automatic-open rows for this PR were re-run on the final commit (f6d2614) with the same outcome. Two rig notes for anyone repeating this: VS Code's extension-test mode keeps global state in memory, so the upgrade replay needs plain launches (state in
User/globalStorage/state.vscdb,SIGTERMshuts down cleanly); and the launch must not pass--disable-gpu, or webviews never render and the page never asks for its manifest on either build.Telemetry context
Power User sink, 0.65.0, 12–14 Oct:
getWhatsNewManifesterror events 251 machines (13 Oct) and 314 (14 Oct) against 2 901 and 4 122 successful automatic opens. Failure text over 14 days:403257 machines, aborted (timeout) 201,ECONNRESET36,50325, DNS 10,4076,4014,5022, certificate 1.🤖 Generated with Claude Code
https://claude.ai/code/session_011AhtJPr3nmT1Po92ZQk4VE
Summary by CodeRabbit