wave8: status/error UX (#94, #100, #46) - #123
Merged
Merged
Conversation
The tsbuildinfo incremental-build cache was tracked and went dirty after every webview build. Untrack it and add `*.tsbuildinfo` to .gitignore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Bug B) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…trigger (#46 Bugs C+D)
📝 WalkthroughWalkthroughThis PR implements Wave 8 — Status / Error UX, a coordinated fix addressing three functional areas: removing the 404-as-valid behavior from API key validation, handling HTTP 429 rate limits with ChangesWave 8 — Status / Error UX
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
192-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
npm run builddoes not include dashboard build.The current
buildscript skipsbuild:dashboard, which violates the required build contract and can leavedashboard-dist/missing.Suggested fix
- "build": "npm run build:webview && npm run build:ext", + "build": "npm run build:dashboard && npm run build:webview && npm run build:ext",As per coding guidelines: "npm run build must build all three (dashboard + webview + extension)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 192, The top-level npm "build" script currently runs only "build:webview" and "build:ext"; update the "build" npm script to include "build:dashboard" (e.g., run "build:dashboard" alongside "build:webview" and "build:ext") so that the dashboard is built and dashboard-dist/ is produced; modify the "build" script entry that references "build:webview" and "build:ext" to also invoke "build:dashboard" in the sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-05-22-wave8-status-error-ux-design.md`:
- Line 244: Update the contradictory "Files touched" conclusion in
docs/superpowers/specs/2026-05-22-wave8-status-error-ux-design.md (the line that
currently reads "No new files") so it matches the table above; change that
sentence to acknowledge the new documentation and test files included in this
wave (e.g., "Includes new docs and a new test file as listed above") and ensure
the phrase "Files touched" or the concluding sentence references the same items
shown in the table.
In `@src/webview/scan-publishing-handler.ts`:
- Around line 793-794: The 429 error path is calling
publishLocalOnlyResults(...) again, producing duplicate emissions because scan
results were already published before remote submission; modify the handler to
avoid re-publishing by tracking whether local publication has already occurred
(e.g., a localPublished boolean scoped to the publish flow or a flag on
this.ctx) and only call publishLocalOnlyResults(manualProjectId ??
this.ctx.getProjectId() ?? "local", `local-${Date.now()}`) when that flag is
false, or remove the redundant call in the 429 branch so publishLocalOnlyResults
is invoked exactly once per scan; update the publish flow around
publishLocalOnlyResults and the 429 handling to set the flag when first
published.
---
Outside diff comments:
In `@package.json`:
- Line 192: The top-level npm "build" script currently runs only "build:webview"
and "build:ext"; update the "build" npm script to include "build:dashboard"
(e.g., run "build:dashboard" alongside "build:webview" and "build:ext") so that
the dashboard is built and dashboard-dist/ is produced; modify the "build"
script entry that references "build:webview" and "build:ext" to also invoke
"build:dashboard" in the sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9526149-3cb4-4b6e-87ae-25901fbd37e0
📒 Files selected for processing (12)
.gitignoreCLAUDE.mddocs/superpowers/plans/2026-05-23-wave8-status-error-ux.mddocs/superpowers/specs/2026-05-22-wave8-status-error-ux-design.mdpackage.jsonsrc/api-client.tssrc/extension.tssrc/test/api-client.test.tssrc/test/scan-publishing-handler.test.tssrc/webview-provider.tssrc/webview/scan-publishing-handler.tswebview/tsconfig.tsbuildinfo
💤 Files with no reviewable changes (1)
- webview/tsconfig.tsbuildinfo
| | `src/test/scan-publishing-handler.test.ts` (new) | 429 scanNotification tests | | ||
| | `CLAUDE.md` | Rename `validateRcApiKey` → `validateApiKey` in the auth section | | ||
|
|
||
| No new files. No removed files. |
There was a problem hiding this comment.
Fix internal contradiction in “Files touched” conclusion.
Line 244 says “No new files,” but this wave explicitly includes new docs (and a new test file in the implementation plan). Please reword to avoid mismatch with the table above.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-05-22-wave8-status-error-ux-design.md` at line
244, Update the contradictory "Files touched" conclusion in
docs/superpowers/specs/2026-05-22-wave8-status-error-ux-design.md (the line that
currently reads "No new files") so it matches the table above; change that
sentence to acknowledge the new documentation and test files included in this
wave (e.g., "Includes new docs and a new test file as listed above") and ensure
the phrase "Files touched" or the concluding sentence references the same items
shown in the table.
Comment on lines
+793
to
+794
| publishLocalOnlyResults(manualProjectId ?? this.ctx.getProjectId() ?? "local", `local-${Date.now()}`); | ||
| return; |
There was a problem hiding this comment.
Avoid re-publishing identical local results in the 429 path.
Line 703 already publishes local-only results before remote submission. Calling publishLocalOnlyResults(...) again here duplicates scanResults emissions and debug-export writes for the same scan.
Suggested fix
if (status === 429) {
const retryAfter = apiErr.retryAfterSeconds;
const waitText = retryAfter !== undefined
? `Try again in ${retryAfter} second${retryAfter === 1 ? "" : "s"}.`
: "Try again in a moment.";
this.ctx.postMessage({
type: "scanNotification",
message: `ReCost scan rate limit reached. ${waitText} Showing local results.`,
});
- publishLocalOnlyResults(manualProjectId ?? this.ctx.getProjectId() ?? "local", `local-${Date.now()}`);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| publishLocalOnlyResults(manualProjectId ?? this.ctx.getProjectId() ?? "local", `local-${Date.now()}`); | |
| return; | |
| if (status === 429) { | |
| const retryAfter = apiErr.retryAfterSeconds; | |
| const waitText = retryAfter !== undefined | |
| ? `Try again in ${retryAfter} second${retryAfter === 1 ? "" : "s"}.` | |
| : "Try again in a moment."; | |
| this.ctx.postMessage({ | |
| type: "scanNotification", | |
| message: `ReCost scan rate limit reached. ${waitText} Showing local results.`, | |
| }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/webview/scan-publishing-handler.ts` around lines 793 - 794, The 429 error
path is calling publishLocalOnlyResults(...) again, producing duplicate
emissions because scan results were already published before remote submission;
modify the handler to avoid re-publishing by tracking whether local publication
has already occurred (e.g., a localPublished boolean scoped to the publish flow
or a flag on this.ctx) and only call publishLocalOnlyResults(manualProjectId ??
this.ctx.getProjectId() ?? "local", `local-${Date.now()}`) when that flag is
false, or remove the redundant call in the 429 branch so publishLocalOnlyResults
is invoked exactly once per scan; update the publish flow around
publishLocalOnlyResults and the 429 handling to set the flag when first
published.
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
1 participant
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.
Closes #94
Closes #100
Closes #46
Summary
validateRcApiKey()treats 404 as "valid in dev mode" #94 —validateApiKey()no longer treats a 404 from/auth/meas "valid in dev mode" (was a fail-open at the auth boundary). Return type narrowed fromAuthMeUser | nulltoAuthMeUser.scanNotificationwithRetry-Afterwait text and still publishes local results.apiFetchparses the header intoerr.retryAfterSeconds(silently drops HTTP-date form per RFC 7231).refreshStatusBarcallback wired throughReCostSidebarProviderinstead of waiting for the next window focus.updateStatusBardrops stale concurrent writes after everyawaitboundary;lastValidationAtonly updates on non-stale completions.windowFocusedrefresh is debounced to 60 seconds; all other trigger reasons bypass the debounce.onDidChangeWorkspaceFolderstrigger (workspace folder identity is unrelated to auth state).New exported type:
ApiClientErrorinsrc/api-client.tscarriesstatus+ optionalretryAfterSeconds.Test plan
npm run test:scanner— existing suite + newscan-publishing-handlertest (4 cases) + updatedapi-clienttests (10 cases total) all passnpm run build— dashboard + webview + extension build clean/auth/mereturns 404 → status bar shows "Invalid Key", not "Connected". (ExtensionvalidateRcApiKey()treats 404 as "valid in dev mode" #94)scanNotificationwith a wait time. (Rate-limit 429 from scan submission is not surfaced in the extension UI #100)Plan:
docs/superpowers/plans/2026-05-23-wave8-status-error-ux.md. Design spec:docs/superpowers/specs/2026-05-22-wave8-status-error-ux-design.md.🤖 Generated with Claude Code