fix(onboarding): persist installed plugins in summary#1915
Open
Ajit-Mehrotra wants to merge 6 commits intomainfrom
Open
fix(onboarding): persist installed plugins in summary#1915Ajit-Mehrotra wants to merge 6 commits intomainfrom
Ajit-Mehrotra wants to merge 6 commits intomainfrom
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds handling for installed-plugins loading state, disables the primary action while plugin data is pending, and persists the union of manually selected plugins and pre-installed plugins when advancing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1915 +/- ##
=======================================
Coverage 50.98% 50.98%
=======================================
Files 1024 1024
Lines 70558 70569 +11
Branches 7714 7718 +4
=======================================
+ Hits 35974 35982 +8
- Misses 34458 34461 +3
Partials 126 126 ☔ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts (1)
138-146: Prefer semantic selectors overdata-testidin this component test.Use semantic queries (
findAll('input[type="checkbox"]'),findAll('button')) to keep tests closer to user-observable behavior.♻️ Suggested refactor
- const switches = wrapper.findAll('[data-testid="plugin-switch"]'); + const switches = wrapper.findAll('input[type="checkbox"]'); @@ - const nextButton = wrapper - .findAll('[data-testid="brand-button"]') + const nextButton = wrapper + .findAll('button') .find((button) => button.text().toLowerCase().includes('next'));As per coding guidelines "Use semantic queries like
find('button')orfind('[data-test=\"id\"]')but prefer not to use data test ID's when finding elements".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts` around lines 138 - 146, Replace the test's non-semantic data-testid queries with semantic DOM queries: use wrapper.findAll('input[type="checkbox"]') instead of wrapper.findAll('[data-testid="plugin-switch"]') to populate the switches array and assert their checked state, and use wrapper.findAll('button') (then filter by text) instead of wrapper.findAll('[data-testid="brand-button"]') to locate nextButton; update references to the switches variable and nextButton accordingly in the assertions so the test queries reflect user-observable elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts`:
- Around line 129-160: The test block in OnboardingPluginsStep.test is failing
Prettier; reformat the affected test to match project styling: run the code
formatter or adjust spacing/indentation and line breaks within the "persists
already installed plugins alongside manual selections" test so constructs like
installedPluginsResult.value assignment, the mountComponent() call, await
flushPromises(), the switches assertions, nextButton lookup, and the
draftStore.setPlugins/mock call assertions are all properly wrapped and
line-broken per Prettier rules; ensure no trailing commas or inconsistent
quote/paren placement remain so the file passes CI formatting checks.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts`:
- Around line 138-146: Replace the test's non-semantic data-testid queries with
semantic DOM queries: use wrapper.findAll('input[type="checkbox"]') instead of
wrapper.findAll('[data-testid="plugin-switch"]') to populate the switches array
and assert their checked state, and use wrapper.findAll('button') (then filter
by text) instead of wrapper.findAll('[data-testid="brand-button"]') to locate
nextButton; update references to the switches variable and nextButton
accordingly in the assertions so the test queries reflect user-observable
elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 27d4bbad-d163-4926-8338-e0f2778444bf
📒 Files selected for processing (2)
web/__test__/components/Onboarding/OnboardingPluginsStep.test.tsweb/src/components/Onboarding/steps/OnboardingPluginsStep.vue
- Purpose: keep the onboarding summary aligned with the plugin step when recommended plugins are already installed. - Before: the plugin step rendered preinstalled plugins as enabled, but only manually selected plugin IDs were saved into the onboarding draft. - Problem: after the default selection was narrowed to Community Apps, already installed Fix Common Problems and Tailscale dropped out of the summary unless they were pending install. - Change: persist the union of selected plugins and detected installed plugins when advancing from the plugins step. - How: add a computed persisted selection set in the plugins step and cover the installed-plugin path with a regression test.
Ajit-Mehrotra
force-pushed
the
codex/fix-onboarding-installed-plugin-summary
branch
from
cf55955 to
98dd70d
Compare
- Purpose: checkpoint the current local repository changes while excluding the nested webgui repo and local .codex files. - Before: these edits existed only in the working tree and were not pushed anywhere. - Problem: the workspace had a mix of generated updates, onboarding context files, and test formatting changes that could be lost or stay hard to track. - Change: commit the staged API generated updates, onboarding context query/store files, and related test-file edits on the current branch. - How: stage only repository files under api/ and web/, leave .codex and webgui untracked, then record the snapshot as a conventional commit.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/Onboarding/steps/OnboardingPluginsStep.vue`:
- Line 155: The persistence call
draftStore.setPlugins(persistedSelectedPlugins.value) can run before the async
installedPluginIds are populated, omitting preinstalled plugins; change the flow
in the OnboardingPluginsStep component to wait for or merge installedPluginIds
into the persisted list before persisting: either await the query that fills
installedPluginIds (or check a loading flag) and then call
draftStore.setPlugins(union(persistedSelectedPlugins.value,
installedPluginIds)), or compute a merged array that includes any IDs from
installedPluginIds when setting draftStore.setPlugins; ensure you reference the
existing symbols persistedSelectedPlugins, installedPluginIds, and
draftStore.setPlugins when implementing the fix.
In `@web/src/components/Onboarding/store/onboardingContextData.ts`:
- Line 7: The file onboardingContextData.ts defines the same Pinia store ID used
by useActivationCodeDataStore ('activationCodeData'), causing registration
conflicts; either remove onboardingContextData.ts if it's a stray duplicate, or
change its store definition to a uniquely named store (rename the exported
function from useActivationCodeDataStore to something like
useOnboardingContextStore and change the defineStore ID away from
'activationCodeData'), or merge its contents (e.g., the onboardingCompleted
computed property) into the existing activationCodeData.ts store so only one
defineStore('activationCodeData', ...) remains.
- Line 5: Update the local import to include the .js extension to satisfy ESM
rules: in onboardingContextData.ts replace the import of ACTIVATION_CODE_QUERY
from '~/components/Onboarding/graphql/onboardingContext.query' with the same
path including '.js' (i.e., onboardingContext.query.js) so the import of
ACTIVATION_CODE_QUERY resolves under the repo's ESM-compatible convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b608467f-fb37-447b-a1cf-5e54545e039b
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
web/__test__/components/Onboarding/OnboardingPluginsStep.test.tsweb/src/components/Onboarding/graphql/onboardingContext.query.tsweb/src/components/Onboarding/steps/OnboardingPluginsStep.vueweb/src/components/Onboarding/store/onboardingContextData.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/test/components/Onboarding/OnboardingPluginsStep.test.ts
| import { defineStore } from 'pinia'; | ||
| import { useQuery } from '@vue/apollo-composable'; | ||
|
|
||
| import { ACTIVATION_CODE_QUERY } from '~/components/Onboarding/graphql/onboardingContext.query'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the imported file exists
find web/src/components/Onboarding -name "onboardingContext.query.ts" 2>/dev/nullRepository: unraid/api
Length of output: 118
🏁 Script executed:
# Examine the store file to see other imports and confirm pattern
cat -n web/src/components/Onboarding/store/onboardingContextData.ts 2>/dev/null | head -20Repository: unraid/api
Length of output: 901
🏁 Script executed:
# Check other imports in similar store files to verify .js extension pattern
fd "store.*\.ts$" web/src/components --type f -x head -10 {} + 2>/dev/null | head -50Repository: unraid/api
Length of output: 36
🏁 Script executed:
# Search for other imports from .ts files in store files to check pattern consistency
rg "from\s+['\"]~.*\.(ts|tsx)['\"]" web/src --type ts | head -20Repository: unraid/api
Length of output: 36
🏁 Script executed:
# Check imports that already use .js extension to confirm the pattern
rg "from\s+['\"]~.*\.js['\"]" web/src --type ts | head -20Repository: unraid/api
Length of output: 959
🏁 Script executed:
# Check if onboardingContext.query is imported anywhere else and how
rg "onboardingContext\.query" web/src --type tsRepository: unraid/api
Length of output: 211
Use .js extension for this local TypeScript import.
This import must include the .js extension to comply with the repository's ESM compatibility requirements. The codebase consistently uses .js extensions for all local TypeScript file imports.
Proposed fix
-import { ACTIVATION_CODE_QUERY } from '~/components/Onboarding/graphql/onboardingContext.query';
+import { ACTIVATION_CODE_QUERY } from '~/components/Onboarding/graphql/onboardingContext.query.js';📝 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
| import { ACTIVATION_CODE_QUERY } from '~/components/Onboarding/graphql/onboardingContext.query'; | |
| import { ACTIVATION_CODE_QUERY } from '~/components/Onboarding/graphql/onboardingContext.query.js'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/Onboarding/store/onboardingContextData.ts` at line 5,
Update the local import to include the .js extension to satisfy ESM rules: in
onboardingContextData.ts replace the import of ACTIVATION_CODE_QUERY from
'~/components/Onboarding/graphql/onboardingContext.query' with the same path
including '.js' (i.e., onboardingContext.query.js) so the import of
ACTIVATION_CODE_QUERY resolves under the repo's ESM-compatible convention.
- Purpose: resolve the outstanding PR review comments and tighten the onboarding plugin step behavior. - Before: the branch included duplicate onboarding context files, a stray generated GraphQL diff, and the plugin step could still persist before installed-plugin detection finished loading. - Problem: those extra files widened the PR, triggered duplicate-store and ESM review feedback, hurt patch coverage, and left a race where preinstalled plugins could still be omitted if the user advanced too quickly. - Change: remove the unrelated files from the PR, gate the primary action on the initial installed-plugins query, and update the component test to use semantic selectors with cleaner formatting. - How: revert the accidental generated change, delete the duplicate onboarding context files, wire the Apollo loading ref into the plugin step busy state, and add a targeted loading-state regression test.
- Purpose: fix the failing PR checks caused by the new onboarding plugin-step test setup. - Before: the test referenced installedPluginsLoading without destructuring it from the hoisted mock, which caused ReferenceError failures in CI. - Problem: the broken test took down the Web test shard and kept the web lint/build job red until formatting was normalized. - Change: destructure the loading mock, keep the loading-gate coverage, and accept the repo formatter output for the updated component and test. - How: add installedPluginsLoading to the hoisted mock binding, rerun the targeted onboarding test, and run pnpm lint:fix plus pnpm type-check across the workspace.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts (1)
9-24:⚠️ Potential issue | 🔴 CriticalCritical:
installedPluginsLoadingis not destructured, causing all tests to fail.The variable is defined inside the
vi.hoisted()return object but is missing from the destructuring assignment on line 9. This causesReferenceError: installedPluginsLoading is not definedat lines 69 and 78, breaking all test cases.🐛 Proposed fix
-const { draftStore, installedPluginsResult, useQueryMock } = vi.hoisted(() => ({ +const { draftStore, installedPluginsResult, installedPluginsLoading, useQueryMock } = vi.hoisted(() => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts` around lines 9 - 24, The test setup destructuring omits installedPluginsLoading which leads to ReferenceError; update the hoisted destructuring on the vi.hoisted() return to include installedPluginsLoading alongside draftStore, installedPluginsResult, and useQueryMock so the test uses the provided installedPluginsLoading object (with value: false) when referenced by the tests; ensure the identifier names match (installedPluginsLoading) and keep the same shape as defined in the hoisted return so existing references to installedPluginsLoading.value work correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web/__test__/components/Onboarding/OnboardingPluginsStep.test.ts`:
- Around line 9-24: The test setup destructuring omits installedPluginsLoading
which leads to ReferenceError; update the hoisted destructuring on the
vi.hoisted() return to include installedPluginsLoading alongside draftStore,
installedPluginsResult, and useQueryMock so the test uses the provided
installedPluginsLoading object (with value: false) when referenced by the tests;
ensure the identifier names match (installedPluginsLoading) and keep the same
shape as defined in the hoisted return so existing references to
installedPluginsLoading.value work correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ae71117-1a2c-41f9-aa2e-54d669e30c76
📒 Files selected for processing (2)
web/__test__/components/Onboarding/OnboardingPluginsStep.test.tsweb/src/components/Onboarding/steps/OnboardingPluginsStep.vue
pujitm
approved these changes
Mar 16, 2026
| () => | ||
| installedPluginsLoading.value && !Array.isArray(installedPluginsResult.value?.installedUnraidPlugins) | ||
| ); | ||
| const isBusy = computed(() => (props.isSavingStep ?? false) || isInstalledPluginsPending.value); |
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
3 participants
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
Problem
When recommended plugins were already installed, the plugins step showed them as enabled via installed state, but only manually selected plugin IDs were saved into the draft. After origin/main narrowed the default first-visit selection to Community Apps, the summary could end up showing only Community Apps even when Fix Common Problems and Tailscale were already installed.
Testing
Summary by CodeRabbit
Bug Fixes
Tests