Hosting signup: customize step before payment - #1438
Open
feruzm wants to merge 1 commit into
Open
Conversation
Signup asked only title and description; theme was hardcoded and every new instance started visually identical. The configure step becomes a customize step: template cards rendered from a new GET /v1/templates catalog (roster + display metadata, so the signup can never carry its own template list), accent quick picks plus a validated free hex field that blocks Continue while invalid, a font pairing select, and identity prefilled from the account profile into empty fields only, taken back out again if the name changes before payment. Choices persist per name in localStorage so an abandoned tab resumes, and are cleared on success. Skipping every choice produces exactly the previous default payload. Server side: createTenantSchema and the flat PATCH vocabulary accept accent (#rgb/#rrggbb) and fontPreset (closed key set shared with the SPA via appearance.ts, with a lockstep test against FONT_PRESETS); normalizeFlatOverrides maps them under general.styles without planting an empty styles object. The reservation lifecycle now honors the step's promise that the look on screen is the look that activates: a same-owner unpaid reservation is refreshed by re-creation (route lets it through after ownership validation; the upsert takes the new config), and the client re-sends creation whenever the name or the composed config changed instead of guarding on the name alone. Live tenants and other owners' reservations still 409. Closes #1414
Contributor
|
Warning Review limit reached
Next review available in: 27 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
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 |
PR Summary by QodoCustomize hosted sites before payment
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
Code Review by Qodo
1. Stale appearance state leaks across username change
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+58
to
+59
| onChange={(e: any) => onInput(e.target.value)} | ||
| placeholder="#0066cc" |
There was a problem hiding this comment.
1. any types in signup 📘 Rule violation ⚙ Maintainability
New TypeScript code introduces any (event handler, profile casting, and test mocks), weakening type safety and making refactors riskier. This violates the rule disallowing any in new/modified TS code.
Agent Prompt
## Issue description
New/modified TypeScript introduces `any` (including event handlers and data casting), reducing type safety.
## Issue Context
The compliance checklist requires no new implicit/explicit `any` in changed TS/TSX.
## Fix Focus Areas
- apps/web/src/features/hosting-signup/accent-picker.tsx[55-62]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[257-264]
- apps/web/src/specs/features/hosting-signup/hosting-signup.spec.tsx[10-16]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+233
to
+241
| useEffect(() => { | ||
| if (step !== "customize" || templates || templatesFailed) return; | ||
| Promise.resolve() | ||
| .then(() => hostingApi.templates?.()) | ||
| .then((r) => { | ||
| if (r && Array.isArray(r.templates)) setTemplates(r.templates); | ||
| else setTemplatesFailed(true); | ||
| }) | ||
| .catch(() => setTemplatesFailed(true)); |
There was a problem hiding this comment.
2. Template fetch lacks cancellation 📜 Skill insight ☼ Reliability
The new template-catalog useEffect performs an async request and calls setTemplates/setTemplatesFailed without a cancellation guard. This can cause setState-on-unmounted issues during navigation or fast step changes.
Agent Prompt
## Issue description
An async `useEffect` fetch updates React state without a cancellation/cleanup guard, risking state updates after unmount.
## Issue Context
The compliance checklist requires guarding against setState on unmounted components for async callbacks.
## Fix Focus Areas
- apps/web/src/features/hosting-signup/hosting-signup.tsx[233-242]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| THEN EXCLUDED.owner ELSE tenants.owner END, | ||
| config = CASE WHEN tenants.subscription_status = 'abandoned' | ||
| THEN EXCLUDED.config ELSE tenants.config END, | ||
| config = EXCLUDED.config, |
There was a problem hiding this comment.
3. Unauthenticated reservation overwrite 🐞 Bug ⛨ Security
POST /v1/tenants can replace an inactive reservation’s saved config when a caller-supplied owner matches the stored owner, but for personal blogs that owner is deterministically derived from an unauthenticated username, letting any caller overwrite another user’s unpaid title/description/appearance before activation. Separately, the ?resume flow triggers payment with empty customization state so the new upsert replaces the inactive row with defaults, causing legitimate owners to lose their previously reserved look and identity fields.
Agent Prompt
## Issue description
Fix the inactive-reservation update semantics so that (1) refreshing/mutating an unpaid reservation cannot be authorized using a forgeable owner derived from an unauthenticated username, and (2) the resume-to-payment flow does not submit empty/default client state that overwrites an existing reservation’s saved customization.
## Issue Context
Anonymous initial creation may remain public, but any subsequent mutation of an existing inactive reservation must require authenticated proof of ownership or a server-issued, unguessable reservation capability/token bound to the reservation row. Additionally, a resume should either preserve/touch the existing config server-side, fetch and restore the saved draft before submitting, or route users through customization before using any replacement/upsert semantics; add tests/coverage showing an unauthenticated caller cannot refresh an existing inactive reservation without proof, and that resuming a reservation with non-default appearance/identity fields preserves those fields after the resume/payment step.
## Fix Focus Areas
- apps/self-hosted/hosting/api/src/routes/tenants.ts[218-258]
- apps/self-hosted/hosting/api/src/services/tenant-service.ts[233-249]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[371-415]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[300-312]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+212
to
+226
| // An abandoned tab resumes its customization for the same name. | ||
| const draft = readCustomizeDraft(tenantUsername); | ||
| if (draft) { | ||
| if (draft.styleTemplate !== undefined) setStyleTemplate(draft.styleTemplate); | ||
| if (draft.accent !== undefined) { | ||
| setAccent(draft.accent); | ||
| setAccentInput(draft.accent ?? ""); | ||
| } | ||
| if (draft.fontPreset !== undefined) setFontPreset(draft.fontPreset); | ||
| if (draft.title && !nextTitle) nextTitle = draft.title; | ||
| if (draft.description && !nextDescription) nextDescription = draft.description; | ||
| } | ||
| setTitle(nextTitle); | ||
| setDescription(nextDescription); | ||
| setStep("customize"); |
There was a problem hiding this comment.
4. Stale appearance state leaks across username change 🐞 Bug ≡ Correctness
goCustomize() clears the prefilled title/description when the tenant name changes, but it does not reset styleTemplate, accent/accentInput, or fontPreset, so appearance choices from a previous name can linger in component state. As a result, switching from one tenant name to another with no (or partial) saved draft can silently submit the prior name’s appearance settings in the new name’s createTenant payload and persist them under the new draft key.
Agent Prompt
## Issue description
When the user changes the tenant username after selecting appearance options (style template, accent/accentInput, font preset) for a previous name, those appearance values are not cleared. If the newly entered name has no saved customize draft (or only a partial one), stale appearance state from the previous name persists and is silently included in the createTenant payload and can be saved under the new name’s draft key.
## Issue Context
`goCustomize()` already tracks name changes via `plantedRef` and resets only the prefilled title/description when the tenant name changes, but it does not reset `styleTemplate`, `accent`, `accentInput`, or `fontPreset`. Appearance state lives at the component level, and the draft restoration logic only overwrites fields when `readCustomizeDraft(tenantUsername)` returns a draft and/or when those keys are present; if the draft is missing or incomplete for the new name, old values remain and later get included in the config/payload built during the payment/creation step.
Implement a more robust name-scoped initialization: on tenant name change, initialize all appearance fields to defaults for the destination name, then apply that name’s validated draft as a complete overlay. Also add a test covering “customize Alice → switch to Bob without a draft → verify Bob sends no appearance overrides.”
## Fix Focus Areas
- apps/web/src/features/hosting-signup/hosting-signup.tsx[200-226]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[276-285]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[300-312]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+237
to
+239
| .then((r) => { | ||
| if (r && Array.isArray(r.templates)) setTemplates(r.templates); | ||
| else setTemplatesFailed(true); |
There was a problem hiding this comment.
5. Empty template catalog renders no picker or error 🐞 Bug ☼ Reliability
The customize-step effect treats any array response (including an empty one) from `GET /v1/templates as success, setting templates to []`; TemplatePicker then renders an empty grid with no cards and no failure message, leaving the user unable to pick a template with no explanation. This degrades silently instead of falling back to the plain-form failure path the PR describes for a failed catalog load.
Agent Prompt
## Issue description
A `GET /v1/templates` response with an empty `templates` array is currently treated as a successful catalog load, leaving the signup's customize step with an empty grid and no picker UI and no explanation to the user.
## Issue Context
The fetch effect in HostingSignup only checks `Array.isArray(r.templates)`, not its length, before calling `setTemplates(r.templates)`. TemplatePicker has no dedicated empty-state message; it only shows the load-failed text when `failed` is explicitly true.
## Fix Focus Areas
- apps/web/src/features/hosting-signup/hosting-signup.tsx[233-242]
- apps/web/src/features/hosting-signup/template-picker.tsx[26-38]
Treat a zero-length `templates` array the same as a failed load (call `setTemplatesFailed(true)` instead of `setTemplates([])`), or add an explicit empty-state message in TemplatePicker.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Code Review by Qodo
1. Stale appearance state leaks across username change
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+280
to
+283
| localStorage.setItem( | ||
| customizeDraftKey(tenantUsername), | ||
| JSON.stringify({ styleTemplate, accent, fontPreset, title, description }) | ||
| ); |
There was a problem hiding this comment.
1. Draft lacks grace-window expiry 📎 Requirement gap ⛨ Security
The new customize draft is persisted in localStorage per name but has no timestamp/TTL and is only cleared on success, so draft/customization data can persist indefinitely past the unpaid-reservation grace window. This violates the requirement to discard draft/customization configuration after the grace period.
Agent Prompt
## Issue description
`localStorage` customize drafts are saved without any expiry metadata and therefore can persist indefinitely, even after an unpaid reservation should have expired.
## Issue Context
PR adds per-name draft persistence via `customizeDraftKey()` + `localStorage.setItem(...)` and only removes it on `step === "success"`. Compliance requires draft/customization configuration to be discarded after the unpaid-reservation grace window (proposed 7 days).
## Fix Focus Areas
- apps/web/src/features/hosting-signup/hosting-signup.tsx[65-75]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[276-285]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[363-369]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+213
to
+215
| const draft = readCustomizeDraft(tenantUsername); | ||
| if (draft) { | ||
| if (draft.styleTemplate !== undefined) setStyleTemplate(draft.styleTemplate); |
There was a problem hiding this comment.
2. Appearance leaks across names 🐞 Bug ≡ Correctness
When the user goes Back and changes the tenant name, goCustomize only updates appearance state if the new name has a draft; otherwise the previous name's template, accent, and font remain selected. Those retained values are then persisted and submitted for the new tenant, violating the per-name draft boundary.
Agent Prompt
## Issue description
Appearance state from one tenant name survives when navigating back and selecting another name with no saved draft.
## Issue Context
Initialize every per-name customization field to its default before overlaying the selected name's draft. Preserve explicit `null` values and also reset `accentInput` consistently.
## Fix Focus Areas
- apps/web/src/features/hosting-signup/hosting-signup.tsx[181-226]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[276-285]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[300-307]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+365
to
+367
| if (step !== "success" || !tenantUsername) return; | ||
| try { | ||
| localStorage.removeItem(customizeDraftKey(tenantUsername)); |
There was a problem hiding this comment.
3. Reload clears wrong draft 🐞 Bug ≡ Correctness
Pending-payment recovery polls the tenant stored in session storage but advances to success without synchronizing tenantUsername; the new cleanup deletes the draft keyed by that unchanged state. If a logged-in account paid for a different personal blog, recovery retains the paid blog's stale draft and may delete the payer's unrelated draft.
Agent Prompt
## Issue description
Success cleanup uses the form's current tenant name rather than the tenant recovered from the pending-payment marker.
## Issue Context
Keep the successfully activated tenant as the cleanup source of truth, including the reload recovery path. Add coverage where the active account and pending tenant differ and both have local drafts.
## Fix Focus Areas
- apps/web/src/features/hosting-signup/hosting-signup.tsx[363-369]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[556-608]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
236
to
+239
| ON CONFLICT (username) DO UPDATE | ||
| SET owner = CASE WHEN tenants.subscription_status = 'abandoned' | ||
| THEN EXCLUDED.owner ELSE tenants.owner END, | ||
| config = CASE WHEN tenants.subscription_status = 'abandoned' | ||
| THEN EXCLUDED.config ELSE tenants.config END, | ||
| config = EXCLUDED.config, |
There was a problem hiding this comment.
4. Config overwrite discards concurrent edits on refresh 🐞 Bug ≡ Correctness
The upsert for same-owner inactive reservations now unconditionally sets `tenants.config = EXCLUDED.config, so any newer configuration changes saved to the tenant (e.g., via PATCH /v1/tenants/:username` / Configuration Editor or from another tab/device) can be silently overwritten by an older or default client payload. This is especially risky in the ?resume= flow, which calls goPayment from fresh/empty appearance state without restoring the saved draft, causing an activated site to differ from what the user previously customized.
Agent Prompt
## Issue description
The `POST /v1/tenants` create upsert for same-owner `inactive` reservations now always replaces the stored `tenants.config` with `EXCLUDED.config`, which can discard newer configuration changes (e.g., made via `PATCH /v1/tenants/:username` / Configuration Editor or another tab/device) and can also overwrite saved customization during `?resume=` because the resume path submits from fresh/empty appearance state.
## Issue Context
The customize step wants the latest signup submission’s look to win, but `buildConfig` builds from defaults plus the client’s flat overrides rather than from the currently stored tenant config. Separately, the `?resume=` flow bypasses the customize step and its local draft restoration by calling `goPayment` directly, so resuming before payment can submit default/empty appearance values and—combined with the unconditional upsert—silently reset the stored customization, resulting in the activated site not matching what the user saved.
## Fix Focus Areas
- apps/self-hosted/hosting/api/src/services/tenant-service.ts[209-258]
- apps/self-hosted/hosting/api/src/services/tenant-service.ts[1178-1200]
- apps/self-hosted/hosting/api/src/services/tenant-service.ts[223-249]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[300-315]
- apps/web/src/features/hosting-signup/hosting-signup.tsx[371-415]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+300
to
+312
| const config = { | ||
| theme: "system" as const, | ||
| title: title.trim() || undefined, | ||
| description: description.trim() || undefined, | ||
| styleTemplate: styleTemplate ?? undefined, | ||
| accent: accent ?? undefined, | ||
| fontPreset: fontPreset ?? undefined, | ||
| ...(isCommunity ? { type: "community" as const, communityId: uname } : {}) | ||
| }; | ||
| const payload = JSON.stringify(config); | ||
| if (createdForRef.current?.name !== uname || createdForRef.current?.payload !== payload) { | ||
| const res = await hostingApi.createTenant(uname, owner, config); | ||
| createdForRef.current = { name: uname, payload }; |
There was a problem hiding this comment.
5. Createdforref dedup key omits owner 🐞 Bug ≡ Correctness
In goPayment, the guard that decides whether to re-send createTenant compares only `{name,
payload}, but payload is derived from a config object that never includes owner`; for a
community signup owner is computed separately from activeUser?.username and passed directly to
createTenant. If the active user changes (e.g., logout/login as a different admin) while the
community id and the rest of the config stay the same, the stale createdForRef entry prevents a
required re-creation with the new owner, so the reservation is not re-associated and the server may
reject the next mutating request as belonging to a different owner.
Agent Prompt
## Issue description
`createdForRef` is used to decide whether `createTenant` needs to be re-sent before moving to the payment step. It compares only the tenant name and a JSON-serialized config that does not include `owner`, even though `owner` is passed to `createTenant` and can change independently of the config for a community signup.
## Issue Context
For a community instance, `owner` is derived from `activeUser?.username`, separate from the `config` object. If the active user changes between two calls to `goPayment` while the community id and rest of config are unchanged, the guard incorrectly treats the reservation as already up to date and skips re-creating it with the new owner.
## Fix Focus Areas
- apps/web/src/features/hosting-signup/hosting-signup.tsx[292-361]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Closes #1414.
Signup asked only title and description with the theme hardcoded, so design choices were only reachable after payment and every new instance started identical. The configure step becomes a customize step, ahead of payment.
Client (apps/web):
GET /v1/templatescatalog served by the hosting API (roster ids + name, tagline, palette swatches, heading style), so the signup can never carry its own copy of the template list. A failed catalog load degrades to a plain form and never blocks signup.Server (hosting API):
appearance.tsaddsFONT_PRESET_KEYSandACCENT_HEX_PATTERNnext to the template roster, under the same rules (dependency-free, SPA imports them; a lockstep test pins the SPA'sFONT_PRESETSto the key set).accentandfontPreset;normalizeFlatOverridesmaps them undergeneral.styleswithout planting an empty styles object.An adversarial review pass drove most of the hardening here: the silent-discard of re-customization (client guard, upsert keeping old config and the 409 path together), the cross-name prefill leak and four surviving test mutants (fontPreset payload, server-side validation, draft restore/clear, prefill) are all fixed with tests pinning them. Totals: hosting API 415 tests, apps/web 2620, self-hosted SPA 880, all typechecks and production builds green.
Known follow-up: the payment page could show the composed look beside the price; that lands with the draft-expiry work (#1415) where the pay-to-keep messaging belongs.