fix(web): await automatic license key installation - #2055
Draft
Ajit Mehrotra (Ajit-Mehrotra) wants to merge 2 commits into
Draft
fix(web): await automatic license key installation#2055Ajit Mehrotra (Ajit-Mehrotra) wants to merge 2 commits into
Ajit Mehrotra (Ajit-Mehrotra) wants to merge 2 commits 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 useInstallKeyStore
participant Wretch
participant useErrorsStore
useInstallKeyStore->>Wretch: request InstallKey.php
Wretch-->>useInstallKeyStore: JSON response or error
useInstallKeyStore->>useErrorsStore: store derived error message
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2055 +/- ##
==========================================
+ Coverage 53.07% 53.15% +0.07%
==========================================
Files 1041 1041
Lines 72425 72468 +43
Branches 8354 8369 +15
==========================================
+ Hits 38440 38520 +80
+ Misses 33858 33821 -37
Partials 127 127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://cdn.github.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. |
- Preserve automatic key installation across supported Unraid releases. - Previously required a success status and message, rejecting valid legacy 2xx responses after the key could already be written. - Accept non-array response objects with a string status, an optional string message, and no error member. - Add historical response fixtures plus malformed and resolved-error regression coverage.
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 the response, routes failures into the existing manual recovery flow, and accepts successful response bodies from every supported Unraid release.
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 a download failure, 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.
The initial fix also validated only
{ status: "success", message: string }. That shape is produced by Unraid 7.2+, but supported older WebGUI releases return{ status: "" }, a localized instruction instatus, or{ status: "success" }without a message. Rejecting those responses could report failure after the key had already been written.Resolution
Consume the Wretch response with
.json()and locally rethrow shared request errors into the install store catch path. Treat a 2xx JSON body as successful only when it is a non-array object with a stringstatus, an optional stringmessage, and noerrormember.This preserves the historical WebGUI success contract without weakening failure handling. The existing trial action and key URL remain intact, and the change does not retry or reissue a license.
Reviewer Considerations
message, and 7.2+ returns both fields.ErrorandTypeErrorrethrow handlers. The shared request resolver otherwise consumes those failures after recording them, while this store owns the installation lifecycle state.errormember is rejected and preserves the safe error string.WebguiNotifyhelper has the same unconsumed response-chain pattern but remains outside this PR because no repository callers were found.Behavior Changes
installinguntil WebGUI responds, then becomessuccess.failed.error, retain the key URL, and show the existing Copy Key URL and Tools > Registration instructions.Implementation Summary
InstallKey.phpJSON response before marking installation successful.download error 8in the errors store.Verification
pnpm test __test__/store/installKey.test.ts— all 3 historical success fixtures failed by producingfailedinstead ofsuccess.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, 56 tests passed.pnpm test— 68 files passed, 666 tests passed, 6 skipped.pnpm codegen— passed with no retained generated changes.pnpm type-check— passed.pnpm lint— passed.git diff --check— passed.Risk
Low. The change remains isolated to client-side install response handling and tests. It does not change the request URL, server endpoint, key URL, trial issuance, retry behavior, or WebGUI installation logic. Historical behavior was verified from WebGUI release tags; no live key was issued or installed.
Summary by CodeRabbit
Bug Fixes
Tests