fix(web): await automatic license key installation - #2055
Draft
Ajit Mehrotra (Ajit-Mehrotra) wants to merge 1 commit into
Draft
fix(web): await automatic license key installation#2055Ajit Mehrotra (Ajit-Mehrotra) wants to merge 1 commit into
Ajit Mehrotra (Ajit-Mehrotra) wants to merge 1 commit into
Conversation
Purpose: - Prevent automatic license-key installation from reporting success before WebGUI confirms the result. Previous behavior: - Awaiting Wretch .get() returned its response-chain immediately without consuming the HTTP response. Why this was a problem: - Delayed HTTP 406 and network failures bypassed the store catch path, produced background rejections, and hid manual-install recovery UI. New behavior: - Keep installation pending until a valid success response arrives, and mark HTTP, network, or malformed responses as failed. How it works: - Consume the response with .json(), validate the InstallKey.php success contract, preserve safe WebGUI errors, and add real-Wretch plus UI regression coverage. Verification: - Pass focused and full web tests, codegen, typecheck, lint, and git diff checks.
Contributor
WalkthroughThe install-key store now validates JSON response shapes, extracts errors consistently, and handles request failures. Tests cover mocked and delayed Wretch responses, invalid payloads, network errors, and manual-install recovery UI guidance. ChangesInstall key handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant useInstallKeyStore
participant Wretch
participant useErrorsStore
User->>useInstallKeyStore: start key installation
useInstallKeyStore->>Wretch: request InstallKey.php
Wretch-->>useInstallKeyStore: JSON response or error
useInstallKeyStore->>useErrorsStore: store derived error message
useInstallKeyStore-->>User: success or failed status
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2055 +/- ##
==========================================
+ Coverage 53.07% 53.10% +0.02%
==========================================
Files 1041 1041
Lines 72425 72467 +42
Branches 8354 8371 +17
==========================================
+ Hits 38440 38480 +40
- Misses 33858 33860 +2
Partials 127 127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/__test__/store/installKey.wretch.test.ts (1)
54-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear all mocks between tests.
The cleanup only clears
unhandledRejectiondirectly. Addvi.clearAllMocks()so every mock is reset between cases.Proposed fix
afterEach(() => { process.off('unhandledRejection', unhandledRejection); + vi.clearAllMocks(); vi.unstubAllGlobals(); vi.restoreAllMocks(); - unhandledRejection.mockClear(); });As per coding guidelines, reset mocks between tests with
vi.clearAllMocks().#!/bin/bash sed -n '43,62p' web/__test__/store/installKey.wretch.test.ts rg -n 'vi\.(clearAllMocks|restoreAllMocks|unstubAllGlobals)' web/__test__/store/installKey.wretch.test.ts🤖 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 `@web/__test__/store/installKey.wretch.test.ts` around lines 54 - 59, Update the afterEach cleanup hook to call vi.clearAllMocks() so all mocks are reset between tests, while preserving the existing unhandledRejection cleanup, global unstubbing, and mock restoration.web/__test__/components/CallbackFeedback.test.ts (1)
309-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuery the copy control semantically.
.brand-buttoncouples this behavior test to a styling class. Prefer the native button selector, orfindComponent(BrandButton)if it does not render a button.As per coding guidelines, use semantic queries such as
find('button')and verify element existence in Vue component tests.#!/bin/bash fd -HI -t f '^BrandButton\.vue$' web -x sh -c 'echo "--- $1"; rg -n -C2 "<button|<a|brand-button" "$1"' sh {} sed -n '297,313p' web/__test__/components/CallbackFeedback.test.ts🤖 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 `@web/__test__/components/CallbackFeedback.test.ts` at line 309, Update the test around the callback copy control to query it semantically with find('button') or findComponent(BrandButton) instead of the styling-dependent '.brand-button' selector, and assert that the queried control exists before verifying its 'Copy Key URL' text.
🤖 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 `@web/__test__/store/installKey.wretch.test.ts`:
- Around line 7-9: Update the local imports in the installKey test to use `.js`
extensions, including the imports for resolveCallbackStatus, useErrorsStore, and
useInstallKeyStore, while leaving package imports unchanged.
---
Nitpick comments:
In `@web/__test__/components/CallbackFeedback.test.ts`:
- Line 309: Update the test around the callback copy control to query it
semantically with find('button') or findComponent(BrandButton) instead of the
styling-dependent '.brand-button' selector, and assert that the queried control
exists before verifying its 'Copy Key URL' text.
In `@web/__test__/store/installKey.wretch.test.ts`:
- Around line 54-59: Update the afterEach cleanup hook to call
vi.clearAllMocks() so all mocks are reset between tests, while preserving the
existing unhandledRejection cleanup, global unstubbing, and mock restoration.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bd576f5-2dbe-472d-a79e-ce32c373a5a6
📒 Files selected for processing (4)
web/__test__/components/CallbackFeedback.test.tsweb/__test__/store/installKey.test.tsweb/__test__/store/installKey.wretch.test.tsweb/src/store/installKey.ts
Comment on lines
+7
to
+9
| import { resolveCallbackStatus } from '~/store/callbackActions.helpers'; | ||
| import { useErrorsStore } from '~/store/errors'; | ||
| import { useInstallKeyStore } from '~/store/installKey'; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use .js extensions for local ESM imports.
These local TypeScript imports violate the repository ESM import convention.
Proposed fix
-import { resolveCallbackStatus } from '~/store/callbackActions.helpers';
-import { useErrorsStore } from '~/store/errors';
-import { useInstallKeyStore } from '~/store/installKey';
+import { resolveCallbackStatus } from '~/store/callbackActions.helpers.js';
+import { useErrorsStore } from '~/store/errors.js';
+import { useInstallKeyStore } from '~/store/installKey.js';As per coding guidelines, use .js extensions in TypeScript imports for ESM compatibility.
#!/bin/bash
sed -n '1,15p' web/__test__/store/installKey.wretch.test.ts
fd -HI -t f '^(tsconfig.*\.json|package\.json)$' . -x sh -c 'echo "--- $1"; sed -n "1,180p" "$1"' sh {}🤖 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 `@web/__test__/store/installKey.wretch.test.ts` around lines 7 - 9, Update the
local imports in the installKey test to use `.js` extensions, including the
imports for resolveCallbackStatus, useErrorsStore, and useInstallKeyStore, while
leaving package imports unchanged.
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
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.
Summary
Automatic license-key installation could report success before WebGUI answered; this waits for and validates the response so HTTP and network failures reach the existing manual-install recovery flow.
Why This Exists
Wretch 2.11.0 starts the request when
.get()is called but returns a non-thenable response-chain. Awaiting that chain without consuming it completed immediately, allowing the store to setkeyInstallStatustosuccesswhile the HTTP request was still pending.A later WebGUI 406 response, including download failures, or a fetch rejection then became an unhandled background rejection. The callback stayed successful and users did not see the existing Copy Key URL and Tools > Registration recovery instructions.
Resolution
Consume the Wretch response with
.json(), locally rethrow shared request errors into the install store catch path, and accept success only when the response matches{ status: "success", message: string }.This keeps the existing trial action and key URL intact. The change does not retry or reissue a license; it makes the existing fallback reliable.
Reviewer Considerations
ErrorandTypeErrorrethrow handlers closely. The shared request resolver records those errors and otherwise returnsundefined, so this install call overrides them to preserve the original HTTP or fetch failure for its own state transition.InstallKey.php. Malformed 200 responses now fail closed instead of being treated as successful installs.WebguiNotifyhelper has the same unconsumed response-chain pattern but is intentionally outside this PR because no repository callers were found.Behavior Changes
installinguntil WebGUI responds, then becomessuccess.failed, resolve callback status toerror, and preserve a useful safe error message where available.Implementation Summary
InstallKey.phpJSON response before marking installation successful.Verification
pnpm test __test__/store/installKey.test.ts __test__/store/installKey.wretch.test.ts __test__/store/callbackActions.helpers.test.ts __test__/components/CallbackFeedback.test.ts— 4 files, 50 tests passed.pnpm test— 68 files passed, 660 tests passed, 6 skipped.pnpm codegen— passed.pnpm type-check— passed.pnpm lint— passed.git diff --check— passed.Risk
Low. The change is isolated to client-side install response handling and tests. It does not change the request URL, key URL, server endpoint, trial issuance, or installation retry behavior. Validation used the installed Wretch version with controlled HTTP responses; a live WebGUI install was not reissued.
Summary by CodeRabbit
Bug Fixes
Tests