Code Review Bench PR #25463 - Moved to kebab-case formatting signup-form

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#5

Refactors signup-form components to use kebab-case naming convention.

[gitar-bot-staging]

⚠️ Bug: Removed setLoading(false) causes stuck loading spinner

The removal of setLoading(false) on the minimal success path is a behavioral change that introduces a UI bug. When a user successfully submits the form in minimal mode:

1. setLoading(true) is called at line 21
2. After success, setSuccess(true) is called at line 29, but loading remains true

In form-view.tsx, the "Email sent" text visibility depends on loading || !success (line 80) — when loading is still true, this condition is truthy, so the "Email sent" confirmation text stays invisible. Meanwhile, the loading spinner's visibility depends on !loading (line 81) — since loading is still true, the spinner remains visible.

Result: After successful submission in minimal mode, the user sees an infinite loading spinner instead of the "Email sent" confirmation message. The form appears to hang even though the submission succeeded.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	setSuccess(true);
	// Don't go to the success page, but show the success state in the form
	setSuccess(true);
	setLoading(false);

• Apply suggested fix

[gitar-bot-staging]

⚠️ Bug: getUrlHistory can now return undefined for same-site embeds

The previous code had a guard:

if (history) {
    return history;
}

This ensured that when getDefaultUrlHistory() returned undefined (no sessionStorage data, non-array data, or error), execution would fall through to construct a default URLHistory array with the current page info.

The new code unconditionally returns the result:

return history;

getDefaultUrlHistory() returns undefined in three cases: (1) no ghost-history key in sessionStorage, (2) parsed value is not an array, (3) sessionStorage access error. In all these cases, getUrlHistory now returns undefined instead of the fallback history entry.

Since the return type is URLHistory (a typed array), this is a type mismatch. The value is passed to JSON.stringify in api.tsx line 38 as urlHistory, which would serialize to undefined (omitted from JSON). This means the API request loses attribution data for same-site embeds when sessionStorage has no history, instead of providing the fallback with the current page as referrer.

_{Was this helpful? React with 👍 / 👎}

Suggested change

	const history = getDefaultUrlHistory();
	if (history) {
	return history;
	}
	return history;
	const history = getDefaultUrlHistory();
	if (history) {
	return history;
	}

• Apply suggested fix

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}

[gitar-bot-staging]

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Kebab-case rename looks correct, but this PR includes two unrelated behavioral changes that introduce bugs: (1) removed setLoading(false) causes a stuck spinner in minimal mode, and (2) removed null guard in getUrlHistory can return undefined and lose attribution data for same-site embeds.

⚠️ Bug: Removed setLoading(false) causes stuck loading spinner

📄 apps/signup-form/src/components/pages/form-page.tsx:29

The removal of setLoading(false) on the minimal success path is a behavioral change that introduces a UI bug. When a user successfully submits the form in minimal mode:

1. setLoading(true) is called at line 21
2. After success, setSuccess(true) is called at line 29, but loading remains true

In form-view.tsx, the "Email sent" text visibility depends on loading || !success (line 80) — when loading is still true, this condition is truthy, so the "Email sent" confirmation text stays invisible. Meanwhile, the loading spinner's visibility depends on !loading (line 81) — since loading is still true, the spinner remains visible.

Result: After successful submission in minimal mode, the user sees an infinite loading spinner instead of the "Email sent" confirmation message. The form appears to hang even though the submission succeeded.

Suggested fix

                // Don't go to the success page, but show the success state in the form
                setSuccess(true);
                setLoading(false);

⚠️ Bug: getUrlHistory can now return undefined for same-site embeds

📄 apps/signup-form/src/utils/helpers.tsx:44-45

The previous code had a guard:

if (history) {
    return history;
}

This ensured that when getDefaultUrlHistory() returned undefined (no sessionStorage data, non-array data, or error), execution would fall through to construct a default URLHistory array with the current page info.

The new code unconditionally returns the result:

return history;

getDefaultUrlHistory() returns undefined in three cases: (1) no ghost-history key in sessionStorage, (2) parsed value is not an array, (3) sessionStorage access error. In all these cases, getUrlHistory now returns undefined instead of the fallback history entry.

Since the return type is URLHistory (a typed array), this is a type mismatch. The value is passed to JSON.stringify in api.tsx line 38 as urlHistory, which would serialize to undefined (omitted from JSON). This means the API request loses attribution data for same-site embeds when sessionStorage has no history, instead of providing the fallback with the current page as referrer.

Suggested fix

            const history = getDefaultUrlHistory();
            if (history) {
                return history;
            }

💡 Bug: README references wrong config filename default.json

📄 apps/signup-form/README.md:62

The README instructs developers to update the version in core/shared/config/default.json, but the actual file is core/shared/config/defaults.json (with an 's'). This is visible in the diff itself — the changed file is ghost/core/core/shared/config/defaults.json. A developer following these instructions would look for the wrong file.

Suggested fix

To use the new version of signup form in Ghost, update the version in Ghost core's default configuration (currently at `core/shared/config/defaults.json`)

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}