fix(web): include connect metadata in callbacks#1957
Conversation
- Purpose: send Connect capability data in account callback payloads so downstream apps can preserve sign-in/sign-out actions during callback resolution. - Before: server callback payloads omitted connectPluginVersion, so sign-in/sign-out callbacks could arrive without any Connect version field and be re-resolved as generic license flows. - Problem: the account app could not reliably keep Connect actions visible after redirect, especially when license mismatch logic also applied. - Now: callback payloads include connectPluginVersion and store coverage asserts the field is preserved in both purchase and account callback payload builders. - How: add connectPluginVersion to buildServerCallbackPayload() and extend the web store tests to cover the callback contract.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Disabled knowledge base sources:
WalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 0154ab79dc
ℹ️ 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".
web/src/store/server.ts
Outdated
|
|
||
| const buildServerCallbackPayload = (overrides: Partial<ServerData> = {}): ServerData => { | ||
| const payload: ServerData = { | ||
| connectPluginVersion: connectPluginVersion.value, |
There was a problem hiding this comment.
buildServerCallbackPayload is explicitly typed as ServerData from @unraid/shared-callbacks, but that contract (current dependency version) does not include connectPluginVersion; adding it here introduces TS2353 and also breaks type checking where this payload is consumed in tests. This means the new callback field is not actually represented in the shared typed interface and will fail any strict type-check workflow until the shared callbacks package/schema is updated (or an explicit extended type is used).
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1957 +/- ##
==========================================
+ Coverage 51.74% 51.95% +0.20%
==========================================
Files 1028 1030 +2
Lines 70792 71137 +345
Branches 7881 7937 +56
==========================================
+ Hits 36630 36956 +326
- Misses 34039 34058 +19
Partials 123 123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/__test__/store/server.test.ts (1)
183-186: Align mocked payload behavior with the real store contract.At Line 185 and Line 214,
store.connectPluginVersion || undefinedcan hide empty-string behavior that production currently emits. Consider mirroring store behavior exactly to avoid test drift.Proposed diff
- connectPluginVersion: store.connectPluginVersion || undefined, + connectPluginVersion: store.connectPluginVersion, ... - connectPluginVersion: store.connectPluginVersion || undefined, + connectPluginVersion: store.connectPluginVersion,Also applies to: 213-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/store/server.test.ts` around lines 183 - 186, The mocked get() builds a payload that currently uses "connectPluginVersion: store.connectPluginVersion || undefined", which masks empty-string values unlike the real store; update the mock in the get() implementation to assign connectPluginVersion directly from store (connectPluginVersion: store.connectPluginVersion) so empty strings are preserved and the test mirrors the real store contract—apply the same change where this pattern appears around the other mock (lines referenced near connectPluginVersion and the payload fields such as description).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/__test__/store/server.test.ts`:
- Around line 183-186: The mocked get() builds a payload that currently uses
"connectPluginVersion: store.connectPluginVersion || undefined", which masks
empty-string values unlike the real store; update the mock in the get()
implementation to assign connectPluginVersion directly from store
(connectPluginVersion: store.connectPluginVersion) so empty strings are
preserved and the test mirrors the real store contract—apply the same change
where this pattern appears around the other mock (lines referenced near
connectPluginVersion and the payload fields such as description).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 680f2db0-8e4d-4031-980a-8881d2c75e15
📒 Files selected for processing (2)
web/__test__/store/server.test.tsweb/src/store/server.ts
- Purpose: align the callback payload branch with the latest local shared-callbacks ServerData shape for Connect-aware flows. - Before: the PR only threaded connectPluginVersion through callback payloads, so newer shared-callbacks consumers would still miss connectState context. - Problem: Connect callbacks could not carry the full Connect runtime metadata expected by the updated local library, especially on replace/account/purchase flows. - Now: the server store includes both connectPluginVersion and connectState in callback payloads, and keeps the extra typing local so the branch still type-checks before the package release lands. - How: add a local callback payload type alias in the server store, source connectState from the cloud minigraph status, and extend the server-store tests to cover purchase, account, and replace payloads.
elibosley
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/__test__/store/server.test.ts (1)
647-650: Optional: add one fallback-path test for missing Connect metadata.Consider adding a case where
cloudorconnectPluginVersionis absent to assert the payload behavior when values are unavailable (e.g.,undefined), so regressions in the fallback path are also covered.Also applies to: 723-726, 777-780
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/store/server.test.ts` around lines 647 - 650, Add a test that exercises the fallback path when Connect metadata is missing: create a case where the test harness uses createTestData(...) without providing cloud.minigraphql (or with cloud set to undefined) and/or omits connectPluginVersion (undefined) and assert the produced payload/behavior matches the expected fallback (e.g., fields absent or defaulted). Locate the existing tests that use createTestData and connectPluginVersion (the blocks around cloud: createTestData(...) as PartialCloudFragment and connectPluginVersion: '2024.05.06.1049') and add one additional spec that passes undefined for cloud or connectPluginVersion and verifies the handler/output returns the correct fallback values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/__test__/store/server.test.ts`:
- Around line 647-650: Add a test that exercises the fallback path when Connect
metadata is missing: create a case where the test harness uses
createTestData(...) without providing cloud.minigraphql (or with cloud set to
undefined) and/or omits connectPluginVersion (undefined) and assert the produced
payload/behavior matches the expected fallback (e.g., fields absent or
defaulted). Locate the existing tests that use createTestData and
connectPluginVersion (the blocks around cloud: createTestData(...) as
PartialCloudFragment and connectPluginVersion: '2024.05.06.1049') and add one
additional spec that passes undefined for cloud or connectPluginVersion and
verifies the handler/output returns the correct fallback values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 306b7d44-918c-4d97-8a3c-c4d8b150d931
📒 Files selected for processing (2)
web/__test__/store/server.test.tsweb/src/store/server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/store/server.ts
- Purpose: install the published release on the PR branch and stop carrying a local callback payload typing shim. - Before: the branch depended on , so the web store had to define local callback types to model the new Connect metadata while waiting for the package release. - Problem: that kept the branch temporarily divergent from the real package API and added local maintenance overhead in the callback payload builder. - Now: the web workspace depends on , the lockfile points at the published package, and the server store uses the shared package types directly. - How: bump the dependency and lockfile, import from , remove the local callback type aliases, and normalize the GraphQL Connect status through a typed helper before putting it into .
- Purpose: verify the callback payload fallback path when Connect metadata is absent. - Before: the server store tests covered only the positive case where connectPluginVersion and cloud minigraph status were both present. - Problem: that left the current branch without an assertion for the expected fallback behavior when those fields are missing. - Now: the test suite includes a focused spec that omits both cloud metadata and connectPluginVersion and confirms the payload leaves those fields undefined while preserving the rest of the callback data. - How: add a single serverPurchasePayload test in server.test.ts and re-run the focused Vitest file.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
1 participant
Summary
@unraid/shared-callbacks@3.1.0in the web workspaceconnectPluginVersionandconnectStatethrough the published shared package contractProblem
The earlier branch work had to model the new Connect callback fields locally because the web app was still pinned to
@unraid/shared-callbacks@3.0.0. Now that3.1.0is available, the branch should depend on the published package directly so the callback payload contract lives in one place and the web store stops carrying stopgap local types.Testing
pnpm test __test__/store/server.test.tspnpm type-checkstill fails on pre-existing issues insrc/components/ApiKey/ApiKeyCreate.vue,src/components/Common/ResizableSlideover.vue,src/components/Onboarding/OnboardingModal.vue, andsrc/components/UpdateOs/CheckUpdateResponseModal.vueSummary by CodeRabbit
New Features
Tests