fix(onboarding): handle server rename reconnect flow#1986
Draft
Ajit-Mehrotra wants to merge 7 commits intocodex/server-side-onboarding-wizardfrom
Draft
fix(onboarding): handle server rename reconnect flow#1986Ajit-Mehrotra wants to merge 7 commits intocodex/server-side-onboarding-wizardfrom
Ajit-Mehrotra wants to merge 7 commits intocodex/server-side-onboarding-wizardfrom
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds support for a Changes
Sequence DiagramsequenceDiagram
participant Client as Browser Client
participant Component as OnboardingSummaryStep
participant Mutation as updateServerIdentity Mutation
participant Resolver as Server Resolver
participant Service as ServerService
participant Avahi as AvahiService
participant Store as State Store
participant Nginx as Nginx Config
Client->>Component: Request server rename
Component->>Mutation: Call updateServerIdentity($name, $comment)
Mutation->>Resolver: Execute GraphQL resolver
Resolver->>Service: Call updateServerIdentity()
alt Name Changed
Service->>Service: Persist new identity to emcmd
Service->>Avahi: restart() to apply new mDNS name
Avahi-->>Service: Avahi restarted
Service->>Store: dispatch loadSingleStateFile(nginx)
Store->>Nginx: Reload live nginx state
Nginx-->>Store: Return updated state with defaultUrl
Service->>Service: Validate convergence (lanName matches, defaultUrl present)
Service-->>Resolver: Return updated Server response with defaultUrl
else Name Unchanged
Service-->>Resolver: Return Server response with defaultUrl
end
Resolver-->>Mutation: Return Server { id, name, comment, defaultUrl, ... }
Mutation-->>Component: Receive defaultUrl in response
alt Should Redirect (hostname is not IP)
Component->>Component: Parse hostname from defaultUrl
Component->>Client: location.replace(defaultUrl + pathname/search/hash)
else Should Reload (hostname is IP)
Component->>Component: Set transitioning state
Component->>Client: Await onComplete() callback
Component->>Client: location.reload()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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 @@
## codex/server-side-onboarding-wizard #1986 +/- ##
=======================================================================
+ Coverage 52.62% 52.75% +0.12%
=======================================================================
Files 1036 1039 +3
Lines 72535 72714 +179
Branches 8395 8447 +52
=======================================================================
+ Hits 38174 38360 +186
+ Misses 34237 34230 -7
Partials 124 124 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Ajit-Mehrotra
force-pushed
the
codex/server-side-onboarding-wizard
branch
from
712d594 to
a0bf3b1
Compare
Ajit-Mehrotra
force-pushed
the
codex/server-identity-mdns-reload
branch
from
d2f4608 to
69c728b
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/servers/server.service.spec.ts (1)
194-211: Model the nginx reload asynchronously in these rename tests.These stubs swap
getters.emhttpbeforeunwrap()resolves, so the tests still pass ifupdateServerIdentity()reads the live state too early or never awaits the reload. Because Line 398 only checks call count, dispatching the wrong reload action would also satisfy the assertion.🧪 Suggested test adjustment
- vi.mocked(store.dispatch).mockImplementation(() => { - vi.mocked(getters.emhttp).mockReturnValue( - createEmhttpState({ - name: 'NewTower', - comment: 'desc', - sysModel: '', - fsState: 'Started', - mdState: ArrayState.STOPPED, - defaultUrl: 'https://NewTower.local:4443', - lanMdns: 'NewTower.local', - lanName: 'NewTower', - }) - ); - - return { - unwrap: vi.fn().mockResolvedValue({ nginx: {} }), - } as unknown as ReturnType<typeof store.dispatch>; - }); + vi.mocked(store.dispatch).mockImplementation(() => ({ + unwrap: vi.fn().mockImplementation(async () => { + vi.mocked(getters.emhttp).mockReturnValue( + createEmhttpState({ + name: 'NewTower', + comment: 'desc', + sysModel: '', + fsState: 'Started', + mdState: ArrayState.STOPPED, + defaultUrl: 'https://NewTower.local:4443', + lanMdns: 'NewTower.local', + lanName: 'NewTower', + }) + ); + return { nginx: {} }; + }), + }) as unknown as ReturnType<typeof store.dispatch>);Apply the same pattern to the other rename-path stubs, then assert the specific nginx reload action instead of only the dispatch count.
Also applies to: 227-242, 293-308, 378-399
api/src/unraid-api/graph/resolvers/servers/build-server-response.ts (1)
32-35: Use nullish coalescing to prevent undefined values from overwriting defaults.The spread merge allows
undefinedvalues fromownerto overwrite theDEFAULT_OWNERfallback. While current callers use??to avoid this, the function should defensively handle anyPartial<ProfileModel>where fields could beundefined. Use nullish coalescing for each field to ensure the default values are used when properties are missing or undefined.Suggested fix
- owner: { - ...DEFAULT_OWNER, - ...owner, - }, + owner: { + id: owner?.id ?? DEFAULT_OWNER.id, + username: owner?.username ?? DEFAULT_OWNER.username, + url: owner?.url ?? DEFAULT_OWNER.url, + avatar: owner?.avatar ?? DEFAULT_OWNER.avatar, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/graph/resolvers/servers/build-server-response.ts` around lines 32 - 35, The owner merge is using spread which allows undefined fields in the incoming owner to overwrite DEFAULT_OWNER; update the owner construction in build-server-response.ts to set each owner property using nullish coalescing (e.g., for each field on DEFAULT_OWNER use owner?.<field> ?? DEFAULT_OWNER.<field>) instead of the current {...DEFAULT_OWNER, ...owner} pattern so missing or undefined properties do not replace defaults; reference DEFAULT_OWNER and the owner object in the build-server-response logic and apply this per-field nullish coalescing for all ProfileModel fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/unraid-api/avahi/avahi.service.ts`:
- Around line 9-11: The restart method currently calls
execa('/etc/rc.d/rc.avahidaemon', ['restart']) with no timeout; update the
AvahiService.restart method to call execa with a timeout option (matching other
services, e.g. 10_000 ms) and wrap the call in a try/catch so timeouts or
failures are logged via this.logger.error and rethrown or handled consistently,
then keep the success this.logger.log('Avahi daemon restarted') on success;
refer to the restart method, the execa invocation, and
this.logger.log/this.logger.error when making the change.
In `@api/src/unraid-api/graph/resolvers/servers/server.service.ts`:
- Around line 72-119: Replace the hard GraphQLError thrown when live identity
doesn't converge in refreshNginxStateAfterNameChange with a bounded
retry-and-fallback: after the Avahi restart and nginx state reload, poll
getters.emhttp() and compute liveIdentity via
this.getLiveIdentityState(refreshedEmhttp) in a short loop (e.g., 5 attempts
with 200–500ms delay) and break early if liveIdentity.lanName === name and
liveIdentity.defaultUrl is present; if the loop still fails to converge, do NOT
throw—log a warning including persistedIdentity and the last liveIdentity
(this.logger.warn/error), return the last refreshedEmhttp as a best-effort
result, and include similar retry+fallback behavior for the second occurrence
referenced around lines 225–228 so both paths avoid turning transient refresh
misses into hard mutation failures.
In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue`:
- Around line 658-660: The reconnect/redirect decision must be armed before any
redirect-building can fail: ensure you set shouldReloadAfterApplyResult (the
flag controlling reconnect/redirect) prior to parsing/throwing on defaultUrl and
compute serverNameChanged based on the actual result of updateServerIdentity
when baselineLoaded is false (rather than forcing false); specifically,
move/ensure evaluation of shouldReloadAfterApplyResult happens before any call
that could throw while building the redirect (e.g., defaultUrl parsing) and
update serverNameChanged using the outcome of updateServerIdentity (or the
returned new name) when baselineLoaded is false so the rename path triggers
reload instead of falling back to the old origin (affecting serverNameChanged,
updateServerIdentity, shouldReloadAfterApplyResult, defaultUrl,
useReturnedDefaultUrlAfterRename).
---
Nitpick comments:
In `@api/src/unraid-api/graph/resolvers/servers/build-server-response.ts`:
- Around line 32-35: The owner merge is using spread which allows undefined
fields in the incoming owner to overwrite DEFAULT_OWNER; update the owner
construction in build-server-response.ts to set each owner property using
nullish coalescing (e.g., for each field on DEFAULT_OWNER use owner?.<field> ??
DEFAULT_OWNER.<field>) instead of the current {...DEFAULT_OWNER, ...owner}
pattern so missing or undefined properties do not replace defaults; reference
DEFAULT_OWNER and the owner object in the build-server-response logic and apply
this per-field nullish coalescing for all ProfileModel fields.
🪄 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: 3f78d576-9698-4c83-b16f-8f97e5f390ea
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
api/generated-schema.graphqlapi/src/unraid-api/avahi/avahi.service.tsapi/src/unraid-api/graph/resolvers/resolvers.module.tsapi/src/unraid-api/graph/resolvers/servers/build-server-response.tsapi/src/unraid-api/graph/resolvers/servers/server.model.tsapi/src/unraid-api/graph/resolvers/servers/server.resolver.spec.tsapi/src/unraid-api/graph/resolvers/servers/server.resolver.tsapi/src/unraid-api/graph/resolvers/servers/server.service.spec.tsapi/src/unraid-api/graph/resolvers/servers/server.service.tsweb/__test__/components/Onboarding/OnboardingSummaryStep.test.tsweb/src/components/Onboarding/graphql/coreSettings.mutations.tsweb/src/components/Onboarding/steps/OnboardingSummaryStep.vueweb/src/composables/gql/gql.tsweb/src/composables/gql/graphql.tsweb/src/locales/en.json
Comment on lines
+9
to
+11
| async restart(): Promise<void> { | ||
| await execa('/etc/rc.d/rc.avahidaemon', ['restart']); | ||
| this.logger.log('Avahi daemon restarted'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== execa callsites in api/src/unraid-api =="
rg -nP --type=ts '\bexeca\s*\(' api/src/unraid-api
echo
echo "== execa callsites that include 'timeout' nearby (quick heuristic) =="
python - <<'PY'
from pathlib import Path
import re
root = Path("api/src/unraid-api")
for p in root.rglob("*.ts"):
text = p.read_text(encoding="utf-8")
for m in re.finditer(r'\bexeca\s*\(', text):
snippet = text[m.start():m.start()+260]
has_timeout = 'timeout' in snippet
print(f"{p}:{text.count(chr(10),0,m.start())+1}: timeout_nearby={has_timeout}")
PYRepository: unraid/api
Length of output: 6611
Add a timeout to the Avahi restart command.
Line 10 executes an external process without a timeout. If the rc script hangs, this can block the onboarding flow. Other system service calls in the codebase (e.g., ups.service.ts restarting apcupsd) use explicit timeouts; Avahi should follow the same pattern.
Suggested fix
async restart(): Promise<void> {
- await execa('/etc/rc.d/rc.avahidaemon', ['restart']);
- this.logger.log('Avahi daemon restarted');
+ await execa('/etc/rc.d/rc.avahidaemon', ['restart'], {
+ timeout: 15_000,
+ });
+ this.logger.log('Avahi daemon restarted');
}📝 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
| async restart(): Promise<void> { | |
| await execa('/etc/rc.d/rc.avahidaemon', ['restart']); | |
| this.logger.log('Avahi daemon restarted'); | |
| async restart(): Promise<void> { | |
| await execa('/etc/rc.d/rc.avahidaemon', ['restart'], { | |
| timeout: 15_000, | |
| }); | |
| this.logger.log('Avahi daemon restarted'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/unraid-api/avahi/avahi.service.ts` around lines 9 - 11, The restart
method currently calls execa('/etc/rc.d/rc.avahidaemon', ['restart']) with no
timeout; update the AvahiService.restart method to call execa with a timeout
option (matching other services, e.g. 10_000 ms) and wrap the call in a
try/catch so timeouts or failures are logged via this.logger.error and rethrown
or handled consistently, then keep the success this.logger.log('Avahi daemon
restarted') on success; refer to the restart method, the execa invocation, and
this.logger.log/this.logger.error when making the change.
Comment on lines
+72
to
+119
| private async refreshNginxStateAfterNameChange( | ||
| name: string, | ||
| persistedIdentity: Awaited<ReturnType<ServerService['readPersistedIdentity']>> | ||
| ): Promise<ReturnType<typeof getters.emhttp>> { | ||
| try { | ||
| await this.avahiService.restart(); | ||
| } catch (error) { | ||
| this.logger.error('Failed to restart Avahi after server rename', error as Error); | ||
| throw new GraphQLError('Failed to update server identity', { | ||
| extensions: { | ||
| cause: | ||
| error instanceof Error && error.message | ||
| ? error.message | ||
| : 'Avahi restart failed after ident.cfg update', | ||
| persistedIdentity, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| try { | ||
| await store.dispatch(loadSingleStateFile(StateFileKey.nginx)).unwrap(); | ||
| } catch (error) { | ||
| this.logger.error('Failed to reload nginx state after server rename', error as Error); | ||
| throw new GraphQLError('Failed to update server identity', { | ||
| extensions: { | ||
| cause: | ||
| error instanceof Error && error.message | ||
| ? error.message | ||
| : 'Failed to reload nginx.ini after Avahi restart', | ||
| persistedIdentity, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const refreshedEmhttp = getters.emhttp(); | ||
| const liveIdentity = this.getLiveIdentityState(refreshedEmhttp); | ||
|
|
||
| if (liveIdentity.lanName !== name || !liveIdentity.defaultUrl) { | ||
| throw new GraphQLError('Failed to update server identity', { | ||
| extensions: { | ||
| cause: 'Live network identity did not converge after Avahi restart', | ||
| persistedIdentity, | ||
| liveIdentity, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| return refreshedEmhttp; |
Contributor
There was a problem hiding this comment.
Don’t turn post-commit convergence checks into a hard mutation failure.
By the time this branch runs, emcmd has returned and ident.cfg already matches the requested identity. A transient Avahi/nginx refresh miss now throws a GraphQL error after the rename has committed. The frontend only enters its reconnect path on a resolved mutation result, so this can strand onboarding on the stale host even though the hostname changed. Please give live identity a short bounded retry window and, if it still doesn’t converge, return a best-effort response with diagnostics instead of rejecting after the side effect.
Also applies to: 225-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/unraid-api/graph/resolvers/servers/server.service.ts` around lines 72
- 119, Replace the hard GraphQLError thrown when live identity doesn't converge
in refreshNginxStateAfterNameChange with a bounded retry-and-fallback: after the
Avahi restart and nginx state reload, poll getters.emhttp() and compute
liveIdentity via this.getLiveIdentityState(refreshedEmhttp) in a short loop
(e.g., 5 attempts with 200–500ms delay) and break early if liveIdentity.lanName
=== name and liveIdentity.defaultUrl is present; if the loop still fails to
converge, do NOT throw—log a warning including persistedIdentity and the last
liveIdentity (this.logger.warn/error), return the last refreshedEmhttp as a
best-effort result, and include similar retry+fallback behavior for the second
occurrence referenced around lines 225–228 so both paths avoid turning transient
refresh misses into hard mutation failures.
Comment on lines
658
to
660
| const serverNameChanged = baselineLoaded ? targetCoreSettings.serverName !== currentName : false; | ||
| const useReturnedDefaultUrlAfterRename = shouldRedirectAfterRename(location.hostname); | ||
| const shouldApplyPartnerSysModel = Boolean( |
Contributor
There was a problem hiding this comment.
Arm the reconnect flow before redirect building can fail.
This logic still has two rename paths that fall back to a normal OK flow on the old origin: when baseline data is unavailable, serverNameChanged is forced to false even though updateServerIdentity() still runs; and when defaultUrl is missing/invalid, the throw happens before shouldReloadAfterApplyResult is set. A successful rename in either case can leave onboarding on a stale host.
Possible direction
- const serverNameChanged = baselineLoaded ? targetCoreSettings.serverName !== currentName : false;
+ const renameMayHaveOccurred =
+ !baselineLoaded || targetCoreSettings.serverName !== currentName;
...
- if (serverNameChanged) {
+ if (renameMayHaveOccurred) {
applyResultFollowUpMessage.value = summaryT('result.renameFollowUpMessage');
+ shouldReloadAfterApplyResult.value = true;
if (useReturnedDefaultUrlAfterRename) {
const defaultUrl = result?.data?.updateServerIdentity?.defaultUrl;
- if (!defaultUrl) {
- throw new Error('Server rename succeeded but no defaultUrl was returned');
+ if (defaultUrl) {
+ redirectUrlAfterApplyResult.value = buildRenameRedirectUrl(defaultUrl);
}
-
- redirectUrlAfterApplyResult.value = buildRenameRedirectUrl(defaultUrl);
}
-
- shouldReloadAfterApplyResult.value = true;
}Also applies to: 700-711
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/Onboarding/steps/OnboardingSummaryStep.vue` around lines
658 - 660, The reconnect/redirect decision must be armed before any
redirect-building can fail: ensure you set shouldReloadAfterApplyResult (the
flag controlling reconnect/redirect) prior to parsing/throwing on defaultUrl and
compute serverNameChanged based on the actual result of updateServerIdentity
when baselineLoaded is false (rather than forcing false); specifically,
move/ensure evaluation of shouldReloadAfterApplyResult happens before any call
that could throw while building the redirect (e.g., defaultUrl parsing) and
update serverNameChanged using the outcome of updateServerIdentity (or the
returned new name) when baselineLoaded is false so the rename path triggers
reload instead of falling back to the old origin (affecting serverNameChanged,
updateServerIdentity, shouldReloadAfterApplyResult, defaultUrl,
useReturnedDefaultUrlAfterRename).
5154311 to
72c9cf4
Compare
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Ajit-Mehrotra
force-pushed
the
codex/server-identity-mdns-reload
branch
from
4460b91 to
f5981ef
Compare
- Purpose: make server-name updates fully switch the live hostname before the mutation returns. - Before: the mutation treated ident.cfg persistence plus Avahi restart as sufficient, so stale hostnames like the old .local name could still be accepted by nginx until a manual reload happened. - Problem: onboarding could report success while the old hostname still served partial UI responses, leading to broken websockets, stale redirects, and a confusing mix of old/new host behavior. - New behavior: name changes now verify ident.cfg, restart Avahi, reload nginx, refresh live nginx state, and fail if the live hostname/defaultUrl still have not converged. - Also included: shared Server response shaping, GraphQL/defaultUrl exposure, onboarding hostname-vs-IP redirect handling, and the matching generated artifacts/tests needed for the stacked branch.
- Purpose: simplify the rename flow so the API only performs the live apply step that still appears necessary: restarting Avahi. - Before: the API retried nginx state convergence and explicitly reloaded nginx after server-name changes, on top of the reload already triggered by emhttpd /update. - Problem: box-level debugging showed /update already runs rc.nginx reload, and extra API-side reloads did not fix stale old-host acceptance; they only added more moving parts and test complexity. - New behavior: name changes now verify ident.cfg, restart Avahi, refresh nginx.ini state once, and return the live defaultUrl without invoking NginxService from the API. - How it works: removes the NginxModule dependency from the resolver stack and trims the server identity tests back to the Avahi-only convergence path.
- Purpose: make the server-rename success flow clearer while the browser reconnects on the new host. - Before: the summary dialog closed immediately into a redirect/reload with no dedicated transition state, and the rename notice used stronger visual emphasis with longer copy. - Problem: users could feel like the page jumped unexpectedly or worry that a reload/sign-in prompt meant onboarding progress was lost. - Change: add a neutral informational alert to the result dialog, show a loading state while waiting for the redirect/reload handoff, and tighten the rename messaging. - How: reuse the existing loading component during post-confirm transition, store a rename follow-up message for the success modal, and extend the summary-step tests to cover the new reconnect UX.
- Purpose: keep onboarding moving to Next Steps when a server rename forces the browser onto a new host, including SSL/self-signed flows. - Before: the summary step waited for the old-host draft save before redirecting, so SSL hostname changes could stall on GraphQL requests to the stale hostname and reopen onboarding on Summary after login. - Problem: users could get stuck on the reconnect spinner, hit certificate and host errors, and then re-run Confirm & Apply even though the rename already succeeded. - Change: redirect immediately with a one-time resume hint, reopen onboarding on Next Steps after the new host loads, and persist that resumed step from the new host. - How: append a resume step query param to rename redirects/reloads, consume and clear that param in the modal after hydrating wizard state, and extend modal/summary tests to cover the redirected resume path.
- Purpose: make server-name changes survive TLS certificate prompts and re-login without dropping users back onto pre-apply summary state. - Before: onboarding relied on a client-side resume hint after rename, which could be lost when the browser hit the new host's certificate warning and /login flow. - Problem: users could land back on SUMMARY after authenticating on the new hostname and be tempted to run Confirm & Apply twice. - Change: add an expectedServerName guard to saveOnboardingDraft, then use a single ordered GraphQL request that renames first and only persists NEXT_STEPS resume state when the live server name actually matches. - Result: the server-owned tracker becomes the durable handoff across hostname change, cert acceptance, and login, while the URL resume hint remains as a fallback.
- remove the onboardingResumeStep query-param fallback from the rename reconnect flow - before this change, the browser carried a URL hint in addition to the server-owned tracker state - that extra path was no longer needed after we added the guarded same-request resume write on the server - now the hostname change flow relies only on the durable tracker handoff, which keeps the reconnect logic simpler and makes this cleanup easy to revert if we ever need the fallback back
- add a 10 second timeout to the Avahi restart used after hostname changes - before this change, the rename flow could wait indefinitely if the rc.avahidaemon script hung - that was risky in onboarding because the user-facing reconnect path depends on this service refresh completing or failing predictably - now the restart is bounded, logs a useful error on failure, and still rethrows so the existing strict mutation failure behavior is preserved
Ajit-Mehrotra
force-pushed
the
codex/server-identity-mdns-reload
branch
from
f5981ef to
ef4556d
Compare
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
defaultUrlfrom the rename flow and use it to reconnect onboarding after a hostname change/logindo not send users back to pre-applySUMMARYProblem From The Start
Changing the server name during onboarding can change the browser entrypoint out from under the wizard.
The non-TLS flow was mostly fine because the browser could reconnect immediately. With SSL/self-signed certs enabled, the browser could be interrupted by:
That broke the original client-side resume hint. After login, onboarding would reopen from the durable tracker state, which still said
SUMMARY, so the user landed back on the pre-confirm screen and could accidentally try to apply the same settings again.Alternatives Considered
We talked through a few options before landing on the final shape:
Persist resume state from the browser before rename.
This was rejected because it is optimistic. We would be saving
NEXT_STEPSbefore knowing whether the rename actually succeeded.Teach
updateServerIdentityto also write onboarding resume state.This would work, but it leaks onboarding-specific behavior into a generic server identity mutation.
Add a brand new onboarding orchestration mutation.
This would keep ownership cleaner, but it felt like too much scope for a targeted fix.
Keep the existing mutations, but send them in one ordered GraphQL request and guard the second write.
This gives us server-side verification without creating a new mutation or teaching
updateServerIdentityabout onboarding.What We Landed On
We kept the existing mutations and moved the durable resume write into the same GraphQL request as the rename:
updateServerIdentity(...)runs firstonboarding.saveOnboardingDraft(...)runs second in the same requestsaveOnboardingDraftnow accepts an optionalexpectedServerNameThat means the wizard only persists
NEXT_STEPSafter the server has actually switched identities. If the browser then gets interrupted by TLS trust or/login, the new host can recover from server-owned state instead of relying on a query param that may be lost.The URL resume hint remains as a fallback, but it is no longer the only thing protecting the flow.
The modal also keeps its own in-memory
localCurrentStepId, separate from the durable tracker state. That means the server-ownedNEXT_STEPShandoff is intentionally picked up on the next bootstrap after cert acceptance/login, rather than trying to push a live step change into the already-mounted old-host modal.Testing
API
pnpm codegenpnpm exec vitest run src/unraid-api/graph/resolvers/servers/server.service.spec.ts src/unraid-api/graph/resolvers/servers/server.resolver.spec.tspnpm exec vitest run src/unraid-api/graph/resolvers/onboarding/onboarding.mutation.spec.ts src/unraid-api/graph/resolvers/customization/onboarding.service.spec.tspnpm type-checkpnpm lintWeb
pnpm codegenpnpm exec vitest run __test__/components/Onboarding/OnboardingSummaryStep.test.ts __test__/components/Onboarding/OnboardingModal.test.tspnpm type-checkpnpm lint