Open
Conversation
- Purpose: hide the server name from the onboarding flow without changing the underlying state or apply behavior. - Before: onboarding showed the server name in core settings, next steps, and the summary card. - Problem: the flow exposed server identity details that should no longer be visible to users during onboarding. - Now: server-name UI nodes remain mounted for state continuity but are visually hidden and removed from accessibility exposure. - How: adds Tailwind hidden styling, aria-hidden markers, and focused tests that assert the server-name rows stay hidden.
Contributor
WalkthroughServer-name UI elements are hidden and removed from keyboard focus across onboarding steps; server-name validation no longer blocks submission and hidden/empty server names are omitted or defaulted when persisting core settings. Tests updated to assert hidden state and revised submission behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Onboarding UI
participant Store as DraftStore
participant Mut as Mutations
participant API as Backend
User->>UI: Fill fields (serverName hidden/empty)
UI->>Store: updateDraft(settings without serverName)
User->>UI: Click Next / Submit
UI->>Store: read draftStore
alt draftStore.serverName present
UI->>Mut: call setCoreSettings(serverName: draftServerName)
else
UI->>Mut: call setCoreSettings(serverName: baselineOrDefault)
end
Mut->>API: persist core settings
API-->>Mut: ack
Mut-->>UI: success
UI-->>User: onComplete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue (1)
459-473:⚠️ Potential issue | 🟠 MajorHidden server-name control can create an unrecoverable submit block.
Because
Line 459hides the entire server-name control, but submit still fails whenserverNameValidationis non-null (Line 359,Line 624), users with an invalid preloaded server name cannot proceed or fix it.Proposed fix (gate server-name validation only when the control is visible)
+const isServerNameUiHidden = true; + const handleSubmit = async () => { - if (serverNameValidation.value || serverDescriptionValidation.value) { + if ((!isServerNameUiHidden && serverNameValidation.value) || serverDescriptionValidation.value) { error.value = t('common.error'); return; }<BrandButton :text="t('onboarding.coreSettings.next')" class="!bg-primary hover:!bg-primary/90 w-full min-w-[160px] !text-white shadow-md transition-all hover:shadow-lg sm:w-auto" - :disabled="isBusy || !!serverNameValidation || !!serverDescriptionValidation" + :disabled=" + isBusy || (!isServerNameUiHidden && !!serverNameValidation) || !!serverDescriptionValidation + " :loading="isBusy" `@click`="handleSubmit" :icon-right="ChevronRightIcon" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue` around lines 459 - 473, The server-name input (UInput bound to serverName) is rendered hidden (class "hidden", aria-hidden="true") but serverNameValidation is still blocking submission; update the validation gating so serverNameValidation only affects form submission and input styling when the control is visible. Add a visibility check (e.g., isServerNameVisible or derive from the same condition that adds "hidden"/aria-hidden) and use it wherever serverNameValidation is consulted (submit handler and any computed/disabled logic) so invalid preloaded server names do not block progress when the field is hidden; keep the existing model (serverName, serverNameValidation, isBusy) and only apply the red-border class and validation failure when the visibility flag is true.
🧹 Nitpick comments (2)
web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts (1)
482-487: Make the hidden-state assertion less brittle and async-safe.Await pending renders, assert the element exists first, then verify both
hiddenandaria-hidden.Suggested test update
- it('marks the server name hidden in the summary card', () => { + it('marks the server name hidden in the summary card', async () => { const { wrapper } = mountComponent(); + await flushPromises(); const serverNameLabel = wrapper.findAll('span').find((span) => span.text() === 'Server Name'); - expect(serverNameLabel?.element.parentElement?.classList.contains('hidden')).toBe(true); + expect(serverNameLabel).toBeDefined(); + const row = serverNameLabel!.element.parentElement as HTMLElement; + expect(row.classList.contains('hidden')).toBe(true); + expect(row.getAttribute('aria-hidden')).toBe('true'); });Based on learnings: Applies to web/**/*.test.{ts,tsx} : Vue component testing: Always await async operations before making assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts` around lines 482 - 487, The test 'marks the server name hidden in the summary card' is brittle and not awaiting Vue's async updates; make the test async, await pending renders (e.g., await wrapper.vm.$nextTick() or await flushPromises()) after calling mountComponent(), locate the Server Name element via wrapper.find / wrapper.findAll and assert it exists first, then verify its parent element has the 'hidden' class and that the element (or its parent) has aria-hidden="true"; use the existing mountComponent, wrapper, and serverNameLabel identifiers to find and assert on the DOM nodes.web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts (1)
202-208: Strengthen this test to validate behavior/accessibility, not only a CSS utility class.Assert that the row exists and carries
aria-hidden="true"in addition to thehiddenclass.Suggested test update
it('marks server name controls hidden', async () => { const { wrapper } = mountComponent(); await flushPromises(); const serverNameLabel = wrapper.findAll('label').find((label) => label.text() === 'Server Name'); - expect(serverNameLabel?.element.parentElement?.classList.contains('hidden')).toBe(true); + expect(serverNameLabel).toBeDefined(); + const row = serverNameLabel!.element.parentElement as HTMLElement; + expect(row.classList.contains('hidden')).toBe(true); + expect(row.getAttribute('aria-hidden')).toBe('true'); });Based on learnings: Applies to /test/components//*.ts : Test component behavior and output, not implementation details.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts` around lines 202 - 208, The test currently only checks for the presence of the 'hidden' CSS class on the Server Name label (serverNameLabel variable); update it to assert the row exists and that the DOM indicates it is hidden for accessibility by also checking the container/row element has aria-hidden="true" (in addition to the hidden class). Locate where serverNameLabel is found via wrapper.findAll('label') and then inspect its parent/container element (parentElement or closest row) to assert it is present and that parentElement.getAttribute('aria-hidden') === 'true' while still confirming the 'hidden' class.
🤖 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/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue`:
- Around line 459-473: The server-name input (UInput bound to serverName) is
rendered hidden (class "hidden", aria-hidden="true") but serverNameValidation is
still blocking submission; update the validation gating so serverNameValidation
only affects form submission and input styling when the control is visible. Add
a visibility check (e.g., isServerNameVisible or derive from the same condition
that adds "hidden"/aria-hidden) and use it wherever serverNameValidation is
consulted (submit handler and any computed/disabled logic) so invalid preloaded
server names do not block progress when the field is hidden; keep the existing
model (serverName, serverNameValidation, isBusy) and only apply the red-border
class and validation failure when the visibility flag is true.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts`:
- Around line 202-208: The test currently only checks for the presence of the
'hidden' CSS class on the Server Name label (serverNameLabel variable); update
it to assert the row exists and that the DOM indicates it is hidden for
accessibility by also checking the container/row element has aria-hidden="true"
(in addition to the hidden class). Locate where serverNameLabel is found via
wrapper.findAll('label') and then inspect its parent/container element
(parentElement or closest row) to assert it is present and that
parentElement.getAttribute('aria-hidden') === 'true' while still confirming the
'hidden' class.
In `@web/__test__/components/Onboarding/OnboardingSummaryStep.test.ts`:
- Around line 482-487: The test 'marks the server name hidden in the summary
card' is brittle and not awaiting Vue's async updates; make the test async,
await pending renders (e.g., await wrapper.vm.$nextTick() or await
flushPromises()) after calling mountComponent(), locate the Server Name element
via wrapper.find / wrapper.findAll and assert it exists first, then verify its
parent element has the 'hidden' class and that the element (or its parent) has
aria-hidden="true"; use the existing mountComponent, wrapper, and
serverNameLabel identifiers to find and assert on the DOM nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a6f2d1ec-6550-4254-b738-aa4765d4f0eb
📒 Files selected for processing (5)
web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vueweb/src/components/Onboarding/steps/OnboardingNextStepsStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1993 +/- ##
=======================================
Coverage 52.55% 52.56%
=======================================
Files 1033 1033
Lines 71649 71659 +10
Branches 8172 8176 +4
=======================================
+ Hits 37655 37667 +12
+ Misses 33869 33866 -3
- Partials 125 126 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
elibosley
changed the title
![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: 162b73dd4f
ℹ️ 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".
| <div class="mb-8 grid grid-cols-1 gap-x-6 gap-y-8 md:grid-cols-2"> | ||
| <!-- Server Name --> | ||
| <div class="flex flex-col gap-2"> | ||
| <div class="hidden flex-col gap-2" aria-hidden="true"> |
There was a problem hiding this comment.
Hiding this entire server-name control makes onboarding impossible to complete whenever serverName is empty or invalid (for example, a bad value from draft/API/activation metadata), because submit is still blocked by serverNameValidation in both handleSubmit and the Next button disable condition. Before this change users could correct the value; after this change they can get stuck on this step with no visible way to fix it.
Useful? React with 👍 / 👎.
- Purpose: keep onboarding progression unblocked when hidden server-name data is empty or invalid. - Before: the hidden server-name field still participated in submit validation and summary apply fallback logic. - Problem: users could be blocked by a hidden value, or an empty hidden draft could drive an unintended fallback server-name update. - Change: core settings now ignores server-name validation and stores no hidden name override. - How it works: summary apply preserves the loaded server name when the draft name is empty, and skips server identity updates when baseline data is unavailable without an explicit draft name. - Tests: update onboarding core and summary specs for hidden server-name behavior and no-reset apply paths.
- Purpose: reduce the hidden server-name fix to the core onboarding step. - Before: the previous patch added summary apply-path rules to compensate for saving an empty hidden server name. - Problem: that pushed business logic into the summary step for a field that should simply remain hidden and non-blocking. - Change: remove the summary-step special cases and keep the resolved server name in the draft. - How it works: the core step ignores hidden server-name validation, saves the current resolved server name, and falls back to baseline/default only when the local value is empty. - Tests: update focused onboarding core/summary specs to cover hidden invalid server names and empty initialized drafts.
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts (1)
217-226: Use container-scoped input lookup instead of placeholder text matching.Line 225 couples the test to localized placeholder copy (
"Tower"). Prefer querying the input withinserverNameControlto keep this behavioral.🔧 Suggested test hardening
- expect(wrapper.find('input[placeholder="Tower"]').attributes('tabindex')).toBe('-1'); + const serverNameInput = serverNameControl?.querySelector('input'); + expect(serverNameInput?.getAttribute('tabindex')).toBe('-1');As per coding guidelines: Test component behavior and output, not implementation details like exact error message wording - avoid writing tests that break when minor changes are made to error messages, log formats, or other non-essential details.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts` around lines 217 - 226, The test 'keeps valid server name controls hidden' should stop relying on the localized placeholder "Tower" and instead query the input inside the found serverNameControl; update the assertion that currently uses wrapper.find('input[placeholder="Tower"]') to locate the input by selecting the input element within serverNameControl (the variable set from serverNameLabel?.element.parentElement) and assert its tabindex is '-1' so the test is resilient to placeholder/localization changes.
🤖 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/OnboardingCoreSettingsStep.vue`:
- Around line 358-362: resolveSubmittedServerName currently treats
whitespace-only serverName.value as valid; update the resolver to trim and treat
empty/whitespace-only values as falsy before falling back. Specifically,
normalize serverName.value with .trim() and check that the trimmed result is
non-empty, then fall back to coreSettingsResult.value?.server?.name?.trim(),
coreSettingsResult.value?.vars?.name?.trim(), and finally
TRUSTED_DEFAULT_PROFILE.serverName; ensure you reference the
resolveSubmittedServerName function and the serverName and coreSettingsResult
symbols when making the change.
---
Nitpick comments:
In `@web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts`:
- Around line 217-226: The test 'keeps valid server name controls hidden' should
stop relying on the localized placeholder "Tower" and instead query the input
inside the found serverNameControl; update the assertion that currently uses
wrapper.find('input[placeholder="Tower"]') to locate the input by selecting the
input element within serverNameControl (the variable set from
serverNameLabel?.element.parentElement) and assert its tabindex is '-1' so the
test is resilient to placeholder/localization changes.
🪄 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
Run ID: 10f69d25-771c-4348-b46f-b46574555ec8
📒 Files selected for processing (4)
web/__test__/components/Onboarding/OnboardingCoreSettingsStep.test.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vueweb/src/components/Onboarding/steps/OnboardingSummaryStep.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/Onboarding/steps/OnboardingSummaryStep.vue
Comment on lines
+358
to
+362
| const resolveSubmittedServerName = () => | ||
| serverName.value || | ||
| coreSettingsResult.value?.server?.name?.trim() || | ||
| coreSettingsResult.value?.vars?.name?.trim() || | ||
| TRUSTED_DEFAULT_PROFILE.serverName; |
Contributor
There was a problem hiding this comment.
Handle whitespace-only server names as empty before fallback resolution.
Line 359 currently treats ' ' as a valid override because it is truthy, so fallback to baseline/default is skipped.
💡 Suggested fix
-const resolveSubmittedServerName = () =>
- serverName.value ||
+const resolveSubmittedServerName = () =>
+ serverName.value.trim() ||
coreSettingsResult.value?.server?.name?.trim() ||
coreSettingsResult.value?.vars?.name?.trim() ||
TRUSTED_DEFAULT_PROFILE.serverName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue` around
lines 358 - 362, resolveSubmittedServerName currently treats whitespace-only
serverName.value as valid; update the resolver to trim and treat
empty/whitespace-only values as falsy before falling back. Specifically,
normalize serverName.value with .trim() and check that the trimmed result is
non-empty, then fall back to coreSettingsResult.value?.server?.name?.trim(),
coreSettingsResult.value?.vars?.name?.trim(), and finally
TRUSTED_DEFAULT_PROFILE.serverName; ensure you reference the
resolveSubmittedServerName function and the serverName and coreSettingsResult
symbols when making the change.
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
Tests
pnpm exec eslint src/components/Onboarding/steps/OnboardingCoreSettingsStep.vue src/components/Onboarding/steps/OnboardingSummaryStep.vue __test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts __test__/components/Onboarding/OnboardingSummaryStep.test.tspnpm type-checkNODE_OPTIONS=--no-experimental-webstorage pnpm exec vitest run __test__/components/Onboarding/OnboardingCoreSettingsStep.test.ts __test__/components/Onboarding/OnboardingSummaryStep.test.tsSummary by CodeRabbit
Tests
Changes