This repository was archived by the owner on Apr 21, 2026. It is now read-only.
feat: lead-readiness acquisition flow and smoke gates#456
Merged
LucasSantana-Dev merged 7 commits intomainfrom Mar 11, 2026
Merged
feat: lead-readiness acquisition flow and smoke gates#456LucasSantana-Dev merged 7 commits intomainfrom
LucasSantana-Dev merged 7 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds end-to-end lead readiness tooling and tests, lead attribution capture and analytics integration, server-side template ownership filtering, gallery and plugins error handling, local Siza agent generation fallback with provider-router changes, multiple new orchestration scripts, and several Playwright/e2e helpers and tests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.3)apps/web/e2e/helpers/admin-client.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m apps/web/e2e/lead-readiness.spec.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m apps/web/src/__tests__/lib/services/provider-router.test.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m
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 |
Project Scorecard |
Member
Author
|
Code review pass completed for #456.\n\nFindings (by severity):\n- None blocking.\n\nOpen assumptions / risks:\n- SonarCloud check currently reports failure without a detailed file-level issue payload in the GitHub check summary; assuming this is external quality-gate drift unless the follow-up Analyze run surfaces actionable findings.\n- Lead scripts intentionally include shellcheck suppressions for note-level rules to match the existing Bash style while keeping CI shell lint green.\n\nSummary:\n- The slice is coherent for lead-readiness + marketplace smoke, with CI-focused shell-lint recovery and no API-breaking changes. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
apps/web/src/app/auth/auth-code-error/page.tsx (2)
16-20: Consider marking props as read-only.Per static analysis, the component props type can be marked as
Readonly<>to prevent accidental mutation and improve type safety.♻️ Proposed fix
export default async function AuthCodeErrorPage({ searchParams, }: { - searchParams: Promise<SearchParams>; + searchParams: Readonly<Promise<SearchParams>>; }) {Alternatively, extract to a named type:
type AuthCodeErrorPageProps = Readonly<{ searchParams: Promise<SearchParams>; }>; export default async function AuthCodeErrorPage({ searchParams }: AuthCodeErrorPageProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/auth/auth-code-error/page.tsx` around lines 16 - 20, The component props for AuthCodeErrorPage should be marked readonly to prevent mutation; create a named props type like AuthCodeErrorPageProps = Readonly<{ searchParams: Promise<SearchParams>; }> and update the function signature to export default async function AuthCodeErrorPage({ searchParams }: AuthCodeErrorPageProps) (or annotate the destructured parameter inline with Readonly<...>) so the searchParams prop is typed as readonly and immutable.
40-51: Duplicate sign-in links may confuse users.Both "Retry sign in" (lines 40-45) and "Go to Sign in" (lines 46-51) link to the same
/signindestination but have different visual treatments. This redundancy may confuse users about which action to take.Consider removing one of these links or differentiating their behavior (e.g., one could preserve the original OAuth provider selection or return URL context).
♻️ Suggested simplification
<div className="space-y-3"> - <Link - href="/signin" - className="block w-full rounded-lg border border-border px-4 py-2 text-center text-sm font-medium text-muted-foreground hover:bg-surface-alt" - > - Retry sign in - </Link> <Link href="/signin" className="block w-full rounded-lg bg-primary px-4 py-2 text-center text-sm font-medium text-white hover:bg-primary-hover" > - Go to Sign in + Try again </Link>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/auth/auth-code-error/page.tsx` around lines 40 - 51, The two Link elements in auth-code-error page (the "Retry sign in" and "Go to Sign in" anchors) both point to "/signin" and are redundant; remove one of them or make their behavior distinct — for example keep the primary CTA (the Link with text "Go to Sign in") and remove the "Retry sign in" Link, or alternatively change the secondary action to include context (append the original provider or return URL as query params, e.g., preserve returnTo or provider) so the two links have different destinations and purposes; update the JSX in page.tsx where these Link elements are defined to reflect the chosen approach.apps/web/src/app/api/gallery/route.ts (1)
36-59: Consider defining the payload type as an interface for cleaner typing.The inline
astype assertion works but is verbose. Extracting to an interface improves readability and reusability.♻️ Suggested refactor
+interface GalleryResponse { + generations: typeof data; + pagination: { + page: number; + limit: number; + total: number; + totalPages: number; + }; + message?: string; +} + const total = count ?? 0; - const payload = { + const payload: GalleryResponse = { generations: data ?? [], pagination: { page, limit, total, totalPages: Math.ceil(total / limit), }, - } as { - generations: typeof data; - pagination: { - page: number; - limit: number; - total: number; - totalPages: number; - }; - message?: string; }; if (total === 0) { payload.message = 'No featured generations available yet.'; } return NextResponse.json(payload);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/gallery/route.ts` around lines 36 - 59, Extract the inline assertion into a named interface (e.g., GalleryResponsePayload) and use it for the `payload` variable to improve readability: define an interface with `generations: typeof data`, `pagination` (page, limit, total, totalPages: number) and optional `message?: string`, then replace the `as { ... }` cast on `payload` with `: GalleryResponsePayload` and keep the rest of the logic (setting `payload.message` when `total === 0` and returning `NextResponse.json(payload)`) unchanged.apps/web/src/app/(marketing)/gallery/gallery-client.tsx (2)
173-197: Consider extracting nested ternary into separate components or helper functions.The nested ternary
loading ? ... : error ? ... : generations.length === 0 ? ... : ...reduces readability. Extracting render logic can improve maintainability.♻️ Suggested refactor using early returns or helper components
+function GalleryContent({ + loading, + error, + generations, + emptyMessage, + onRetry, +}: { + loading: boolean; + error: string | null; + generations: Generation[]; + emptyMessage: string; + onRetry: () => void; +}) { + if (loading) { + return ( + <div className="grid grid-cols-1 gap-4 md:grid-cols-2 lg:grid-cols-3"> + {Array.from({ length: 6 }).map((_, i) => ( + <div + key={i} + className="h-64 animate-pulse rounded-lg border border-surface-3 bg-surface-1" + /> + ))} + </div> + ); + } + + if (error) { + return ( + <div className="flex flex-col items-center justify-center py-24 text-center"> + <Search className="mb-4 h-12 w-12 text-muted-foreground/50" /> + <h2 className="mb-2 text-lg font-medium text-foreground">Gallery unavailable</h2> + <p className="mb-4 text-sm text-muted-foreground">{error}</p> + <button + onClick={onRetry} + className="rounded-md border border-surface-3 px-3 py-1.5 text-sm text-muted-foreground transition-colors hover:text-foreground" + > + Retry + </button> + </div> + ); + } + + if (generations.length === 0) { + return ( + <div className="flex flex-col items-center justify-center py-24 text-center"> + <Search className="mb-4 h-12 w-12 text-muted-foreground/50" /> + <h2 className="mb-2 text-lg font-medium text-foreground">No featured generations yet</h2> + <p className="text-sm text-muted-foreground">{emptyMessage}</p> + </div> + ); + } + + return ( + <div className="grid grid-cols-1 gap-4 md:grid-cols-2 lg:grid-cols-3"> + {generations.map((gen) => ( + <GalleryCard key={gen.id} generation={gen} /> + ))} + </div> + ); +}Then use it in the main component:
<GalleryContent loading={loading} error={error} generations={generations} emptyMessage={emptyMessage} onRetry={() => handlePageChange(1)} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(marketing)/gallery/gallery-client.tsx around lines 173 - 197, The render block in gallery-client.tsx uses a nested ternary (loading ? ... : error ? ... : generations.length === 0 ? ... : ...) which harms readability; extract this into a dedicated presentational component or helper function (e.g., create a GalleryContent component or renderGalleryContent helper) that accepts loading, error, generations, emptyMessage and onRetry (call handlePageChange(1) from the parent via onRetry), then replace the inline nested ternary with a single <GalleryContent .../> call and keep GalleryCard mapping (generation => <GalleryCard key={gen.id} generation={gen} />) inside the extracted component for clarity.
41-45: Type narrowing after.catch(() => null)could mask JSON parse errors.If
res.json()throws (malformed JSON),databecomesnulland the error is silently swallowed with a generic message. This is acceptable for UX but consider logging the parse failure for debugging.♻️ Optional: Log JSON parse errors
- const data = (await res.json().catch(() => null)) as { error?: string } | null; + const data = (await res.json().catch((e) => { + console.error('Failed to parse gallery response:', e); + return null; + })) as { error?: string } | null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(marketing)/gallery/gallery-client.tsx around lines 41 - 45, The code swallows JSON parse errors when doing (await res.json().catch(() => null)) so parsing failures become indistinguishable from empty responses; update the res.json() catch to capture and log the parse error before returning null (e.g., replace .catch(() => null) with .catch((err) => { console.error('gallery json parse error', err); return null; } ) or call the existing app logger), keeping the rest of the flow (checking res.ok and throwing with data?.error) and preserving the return type as GalleryPayload.apps/web/src/lib/analytics/lead-attribution.ts (1)
16-19: Minor simplification available.The conditional expression can be simplified per SonarCloud hint:
Simplified normalizeParam
function normalizeParam(value: string | null): string | null { const normalized = value?.trim(); - return normalized ? normalized : null; + return normalized || null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/analytics/lead-attribution.ts` around lines 16 - 19, The normalizeParam function can be simplified: replace the body that assigns normalized and uses a ternary with a single expression that returns value?.trim() || null (or return normalized || null if you prefer keeping the temporary), ensuring empty or missing strings become null; update the function normalizeParam accordingly.apps/web/src/__tests__/components/plugins/PluginsClient.test.tsx (1)
23-37: Consider expanding test coverage.The current test covers the error state well. For comprehensive coverage, consider adding tests for:
- Loading state (skeleton rendering)
- Empty state (no plugins found message)
- Success state with plugin rendering
- Install/uninstall mutation callbacks and toast notifications
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/__tests__/components/plugins/PluginsClient.test.tsx` around lines 23 - 37, Add tests to PluginsClient.test.tsx to cover loading, empty, success rendering, and mutation callbacks: 1) For loading state, set mockUseInstallPlugin.mockReturnValue and mockUseUninstallPlugin.mockReturnValue to return isPending: true and assert the skeleton/loading UI is rendered; 2) For empty state, return a plugins prop of [] (or mock data source) and assert the "no plugins found" message is shown; 3) For success state, provide a sample plugins array and assert plugin items render with expected text; 4) For install/uninstall flows, simulate user clicks on install/uninstall buttons, assert installMutate/uninstallMutate are called, and mock the mutation results to assert toast notifications are shown (spy on toast or use mock implementation). Use the existing installMutate, uninstallMutate, mockUseInstallPlugin, mockUseUninstallPlugin, and PluginsClient component names to locate and update tests.apps/web/src/app/(dashboard)/templates/templates-client.tsx (1)
130-168: Prefer React Query for this templates fetch state.The new
latestFetchIdguard solves stale responses, but the component is still manually owning loading/error/retry/race handling for server data. Moving this to a query keyed by the filter tuple would align with the repo standard and remove the bespoke fetch bookkeeping.Based on learnings: Applies to
**/*.{ts,tsx}: Use TanStack React Query for data fetching and Zustand for state management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/templates/templates-client.tsx around lines 130 - 168, Replace the manual fetchTemplates + latestFetchId guard with a TanStack React Query useQuery keyed on ['templates', ownershipFilter, selectedCategory, selectedFramework, debouncedSearch, sortBy] and move the fetch logic into the queryFn (keep URLSearchParams logic and mapDBTemplate transformation). Remove latestFetchId, setLoading, setError manual state and race-condition checks; instead derive loading/error/data from useQuery (or use onSuccess to setTemplates if component still expects local state). Ensure the queryFn throws on non-ok responses and returns the mapped templates array so the component uses query.data, query.isLoading, and query.error; keep useDebounce and dependency values only in the query key.apps/web/playwright.config.ts (1)
39-49: PreferwebServer.envalone over shelling throughenv -u.You're already stripping
NO_COLOR/FORCE_COLORfromwebServerEnvand passing that viawebServer.env. Keeping the inline shell prefix only makes the command POSIX-only, so localplaywright testruns fail on Windows shells for no extra isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/playwright.config.ts` around lines 39 - 49, Remove the POSIX-only "env -u NO_COLOR -u FORCE_COLOR " prefix from the web server "command" and rely on the env object (webServer.env / webServerEnv) to control NO_COLOR/FORCE_COLOR; specifically update the code that sets command (inside the Playwright config where process.env.CI ? ... : ...) to use `npx next start -p ${port}` (or `npm run dev -- --port ${port} --webpack` for the non-CI branch) without the env -u prefix so the configuration is cross-platform while continuing to pass sanitized variables via env/webServerEnv.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/e2e/fixtures.ts`:
- Around line 60-68: The fixture currently only logs warnings when both profile
upserts fail (using adminSupabase.from('profiles').upsert with fullProfile and
fallbackProfile) which allows tests to continue in an invalid state; update the
error handling in the profile upsert block so that if the first upsert fails and
the fallback upsert also fails you throw a descriptive Error (or reject the
fixture) instead of just console.warn, including the original error messages
from profileError and fallbackError to make the failure explicit to
authenticatedPage consumers.
In `@apps/web/e2e/helpers/admin-client.ts`:
- Around line 103-125: The function resolveAdminKey currently calls
requiredEnv('SUPABASE_SERVICE_ROLE_KEY') unconditionally which throws before
trying the local JWT path; change the flow so you only call requiredEnv when
needed: check the cache first (cachedKey/cachedUrl), then if
isLocalSupabaseUrl(supabaseUrl) try createLocalServiceRoleJwt(supabaseUrl) in a
try/catch and on error fall back to calling
requiredEnv('SUPABASE_SERVICE_ROLE_KEY'), and for non-local URLs call
requiredEnv directly; update cachedUrl/cachedKey in each branch accordingly to
preserve the existing caching behavior.
In `@apps/web/e2e/helpers/auth.ts`:
- Around line 32-57: The login success predicate is too loose: update the
navigationPromise used above so it only resolves when the URL leaves signin and
is not an auth error route (e.g., change the predicate to reject URLs where
url.pathname.includes('/signin') OR url.pathname.startsWith('/auth') so routes
like '/auth/auth-code-error' are not considered success), and adjust the retry
condition in the catch block so you retry when the page is still on the signin
page (e.g., page.url().includes('/signin')) rather than only when it contains
'/signin?'; keep using the existing symbols navigationPromise, signedIn,
signInError and the page.url() check to implement these changes.
In `@apps/web/e2e/marketplace.spec.ts`:
- Around line 16-25: The test seeds a project (const { data: project, error:
projectError } = await admin.from('projects').insert(...).select('id').single())
but later only deletes the template and not this project, and the gallery tests
insert 13 generations that are never deleted; update the suite to explicitly
delete every seeded record after each test (or in an afterEach/afterAll): remove
the inserted generations (cleanup the rows created by
admin.from('generations').insert... using the recorded generation IDs), delete
the template row(s) as already done, and finally delete the project row using
the saved project.id (or call admin.from('projects').delete().eq('id',
project.id)); ensure the cleanup runs even on failure so subsequent test runs
remain deterministic.
In `@apps/web/src/__tests__/lib/services/provider-router.test.ts`:
- Around line 24-29: The test mutates the global SIZA_AGENT_LOCAL_FALLBACK env
var unnecessarily (the suite already mocks isSizaLocalFallbackEnabled to true),
which can leak into other tests; either remove the
process.env.SIZA_AGENT_LOCAL_FALLBACK assignment lines entirely from
provider-router.test or, if you need to set it for this file, save the original
value and restore it in afterAll/afterEach (capture old =
process.env.SIZA_AGENT_LOCAL_FALLBACK and reassign it back) so global worker
state isn't mutated; ensure this change references the mocked symbols
isSizaLocalFallbackEnabled and generateWithSizaLocalAgent used in the test.
In `@apps/web/src/app/`(marketing)/gallery/gallery-client.tsx:
- Around line 185-190: The current empty-state renders a hardcoded heading "No
featured generations yet" alongside the `emptyMessage` prop which often defaults
to the same text, causing duplicate content; update the render logic in the
Gallery component (the block that checks `generations.length === 0`) to avoid
duplication by either (a) using `emptyMessage` as the heading when provided and
omitting the separate hardcoded heading, or (b) keep the fixed heading and only
render `emptyMessage` when it provides distinct supplementary info (i.e.,
conditionally render the paragraph only if `emptyMessage` is present and not
equal to the default string); adjust logic around `emptyMessage` and the
empty-state JSX to ensure a single clear message is shown.
In `@apps/web/src/app/api/templates/__tests__/route.test.ts`:
- Around line 62-72: The test currently only asserts that eq('created_by', ...)
is not applied for authenticated requests with ownership=all; add an assertion
to ensure eq('is_official', true) is also not applied so "all" doesn’t silently
behave like "official". Update the test in route.test.ts (the case using GET,
mockGetSession and mockEq) to include
expect(mockEq).not.toHaveBeenCalledWith('is_official', true) (or equivalent
negative assertion against mockEq calls) after the existing assertions.
In `@apps/web/src/app/api/templates/route.ts`:
- Around line 22-36: The ownership fallback currently forces is_official=true so
ownership='all' is never returned; update the branching around
validated.ownership in the route handler so that 'official' applies
query.eq('is_official', true) but 'all' (or the default else) leaves the query
unchanged (i.e., do not add an is_official filter) and ensure the 'mine' branch
still checks session.user.id and applies query.eq('created_by',
session.user.id); locate and modify the code that sets query (the
validated.ownership checks and the query variable in route.ts) to implement this
behavior.
In `@apps/web/src/app/layout.tsx`:
- Around line 92-97: The Suspense currently wrapping {children} with
AnalyticsProvider causes the whole app tree to suspend; move {children} out of
the Suspense so the app can prerender, and isolate AnalyticsProvider inside its
own Suspense boundary instead. Specifically, update the block containing
QueryProvider, Suspense, AnalyticsProvider, FeatureFlagProvider and children so
that FeatureFlagProvider (and the {children} tree) are rendered outside the
Suspense, and only AnalyticsProvider is wrapped by Suspense (or its own
lightweight boundary) to contain any client-only async behavior from
useSearchParams; keep the existing providers (QueryProvider,
FeatureFlagProvider) intact and ensure AnalyticsProvider still mounts to run its
useEffect side effects.
In `@apps/web/src/components/analytics/AnalyticsProvider.tsx`:
- Around line 55-61: The route-change effect in AnalyticsProvider's useEffect is
sending a relative path (using pathname and searchParamsString) to window.gtag
which fragments page_location; update the effect to construct and send an
absolute URL (e.g., build from window.location.origin + pathname + optional
?searchParamsString) when calling window.gtag('config', GA_TRACKING_ID, {
page_location: ... }), keeping the GA_TRACKING_ID and window.gtag checks intact.
In `@apps/web/src/lib/services/provider-router.ts`:
- Around line 185-187: The current fallback to Anthropic can append output after
streaming has started because only the local Siza path is guarded by hasChunk;
update the branching that checks quotaError/providerError to also prevent
falling back to Anthropic (or any alternate provider) once any chunk has been
emitted. Concretely, in the code that handles a provider failure (references:
quotaError, providerError, hasChunk and the branch that appends Anthropic
output), add a guard like "if (hasChunk) { do not perform Anthropic fallback;
surface the providerError instead }" (or equivalently only allow fallback when
hasChunk is false), and apply the same guard to the other similar blocks
handling fallback (the other occurrences around the provider failure handling)
so no Anthropic output is appended after streaming has begun.
In `@apps/web/src/lib/services/siza-local-agent.ts`:
- Around line 31-44: The function resolveLibrary currently only recognizes
'shadcn', 'mui'/'chakra' and 'tailwind' so inputs like 'radix', 'headlessui',
'primevue' or 'material' fall through to 'none'; update resolveLibrary to
explicitly return 'radix', 'headlessui', 'primevue' and 'material' when those
exact tokens are passed (and still map 'mui'/'chakra' to 'material', 'tailwind'
to 'none' and preserve 'shadcn'), and consider normalizing the input (e.g.,
lowercasing) before the comparisons so callers' selections are preserved.
---
Nitpick comments:
In `@apps/web/playwright.config.ts`:
- Around line 39-49: Remove the POSIX-only "env -u NO_COLOR -u FORCE_COLOR "
prefix from the web server "command" and rely on the env object (webServer.env /
webServerEnv) to control NO_COLOR/FORCE_COLOR; specifically update the code that
sets command (inside the Playwright config where process.env.CI ? ... : ...) to
use `npx next start -p ${port}` (or `npm run dev -- --port ${port} --webpack`
for the non-CI branch) without the env -u prefix so the configuration is
cross-platform while continuing to pass sanitized variables via
env/webServerEnv.
In `@apps/web/src/__tests__/components/plugins/PluginsClient.test.tsx`:
- Around line 23-37: Add tests to PluginsClient.test.tsx to cover loading,
empty, success rendering, and mutation callbacks: 1) For loading state, set
mockUseInstallPlugin.mockReturnValue and mockUseUninstallPlugin.mockReturnValue
to return isPending: true and assert the skeleton/loading UI is rendered; 2) For
empty state, return a plugins prop of [] (or mock data source) and assert the
"no plugins found" message is shown; 3) For success state, provide a sample
plugins array and assert plugin items render with expected text; 4) For
install/uninstall flows, simulate user clicks on install/uninstall buttons,
assert installMutate/uninstallMutate are called, and mock the mutation results
to assert toast notifications are shown (spy on toast or use mock
implementation). Use the existing installMutate, uninstallMutate,
mockUseInstallPlugin, mockUseUninstallPlugin, and PluginsClient component names
to locate and update tests.
In `@apps/web/src/app/`(dashboard)/templates/templates-client.tsx:
- Around line 130-168: Replace the manual fetchTemplates + latestFetchId guard
with a TanStack React Query useQuery keyed on ['templates', ownershipFilter,
selectedCategory, selectedFramework, debouncedSearch, sortBy] and move the fetch
logic into the queryFn (keep URLSearchParams logic and mapDBTemplate
transformation). Remove latestFetchId, setLoading, setError manual state and
race-condition checks; instead derive loading/error/data from useQuery (or use
onSuccess to setTemplates if component still expects local state). Ensure the
queryFn throws on non-ok responses and returns the mapped templates array so the
component uses query.data, query.isLoading, and query.error; keep useDebounce
and dependency values only in the query key.
In `@apps/web/src/app/`(marketing)/gallery/gallery-client.tsx:
- Around line 173-197: The render block in gallery-client.tsx uses a nested
ternary (loading ? ... : error ? ... : generations.length === 0 ? ... : ...)
which harms readability; extract this into a dedicated presentational component
or helper function (e.g., create a GalleryContent component or
renderGalleryContent helper) that accepts loading, error, generations,
emptyMessage and onRetry (call handlePageChange(1) from the parent via onRetry),
then replace the inline nested ternary with a single <GalleryContent .../> call
and keep GalleryCard mapping (generation => <GalleryCard key={gen.id}
generation={gen} />) inside the extracted component for clarity.
- Around line 41-45: The code swallows JSON parse errors when doing (await
res.json().catch(() => null)) so parsing failures become indistinguishable from
empty responses; update the res.json() catch to capture and log the parse error
before returning null (e.g., replace .catch(() => null) with .catch((err) => {
console.error('gallery json parse error', err); return null; } ) or call the
existing app logger), keeping the rest of the flow (checking res.ok and throwing
with data?.error) and preserving the return type as GalleryPayload.
In `@apps/web/src/app/api/gallery/route.ts`:
- Around line 36-59: Extract the inline assertion into a named interface (e.g.,
GalleryResponsePayload) and use it for the `payload` variable to improve
readability: define an interface with `generations: typeof data`, `pagination`
(page, limit, total, totalPages: number) and optional `message?: string`, then
replace the `as { ... }` cast on `payload` with `: GalleryResponsePayload` and
keep the rest of the logic (setting `payload.message` when `total === 0` and
returning `NextResponse.json(payload)`) unchanged.
In `@apps/web/src/app/auth/auth-code-error/page.tsx`:
- Around line 16-20: The component props for AuthCodeErrorPage should be marked
readonly to prevent mutation; create a named props type like
AuthCodeErrorPageProps = Readonly<{ searchParams: Promise<SearchParams>; }> and
update the function signature to export default async function
AuthCodeErrorPage({ searchParams }: AuthCodeErrorPageProps) (or annotate the
destructured parameter inline with Readonly<...>) so the searchParams prop is
typed as readonly and immutable.
- Around line 40-51: The two Link elements in auth-code-error page (the "Retry
sign in" and "Go to Sign in" anchors) both point to "/signin" and are redundant;
remove one of them or make their behavior distinct — for example keep the
primary CTA (the Link with text "Go to Sign in") and remove the "Retry sign in"
Link, or alternatively change the secondary action to include context (append
the original provider or return URL as query params, e.g., preserve returnTo or
provider) so the two links have different destinations and purposes; update the
JSX in page.tsx where these Link elements are defined to reflect the chosen
approach.
In `@apps/web/src/lib/analytics/lead-attribution.ts`:
- Around line 16-19: The normalizeParam function can be simplified: replace the
body that assigns normalized and uses a ternary with a single expression that
returns value?.trim() || null (or return normalized || null if you prefer
keeping the temporary), ensuring empty or missing strings become null; update
the function normalizeParam accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2effd9a4-ede7-4e7b-b6ff-20646a26a19c
⛔ Files ignored due to path filters (5)
CHANGELOG.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.mdapps/web/marketing/google-ads/siza_br_en_leadtest_v1/day1-ops.mdis excluded by!**/*.mdapps/web/marketing/google-ads/siza_br_en_leadtest_v1/keywords.csvis excluded by!**/*.csvapps/web/marketing/google-ads/siza_br_en_leadtest_v1/negative-keywords.csvis excluded by!**/*.csv
📒 Files selected for processing (35)
apps/web/.env.exampleapps/web/e2e/fixtures.tsapps/web/e2e/helpers/admin-client.tsapps/web/e2e/helpers/auth.tsapps/web/e2e/helpers/lead-readiness.tsapps/web/e2e/lead-readiness.spec.tsapps/web/e2e/marketplace.spec.tsapps/web/marketing/google-ads/siza_br_en_leadtest_v1/campaign-config.jsonapps/web/marketing/google-ads/siza_br_en_leadtest_v1/rsa.jsonapps/web/package.jsonapps/web/playwright.config.tsapps/web/scripts/google-ads-prepublish.shapps/web/scripts/lead-chromium.shapps/web/scripts/lead-preflight.shapps/web/src/__tests__/components/plugins/PluginsClient.test.tsxapps/web/src/__tests__/lib/api/validation/templates.test.tsapps/web/src/__tests__/lib/lead-attribution.test.tsapps/web/src/__tests__/lib/services/provider-router.test.tsapps/web/src/app/(auth)/signup/signup-client.tsxapps/web/src/app/(dashboard)/plugins/plugins-client.tsxapps/web/src/app/(dashboard)/templates/templates-client.tsxapps/web/src/app/(marketing)/gallery/gallery-client.tsxapps/web/src/app/api/gallery/__tests__/route.test.tsapps/web/src/app/api/gallery/route.tsapps/web/src/app/api/onboarding/complete/route.tsapps/web/src/app/api/templates/__tests__/route.test.tsapps/web/src/app/api/templates/route.tsapps/web/src/app/auth/auth-code-error/page.tsxapps/web/src/app/layout.tsxapps/web/src/components/analytics/AnalyticsProvider.tsxapps/web/src/components/tour/TourProvider.tsxapps/web/src/lib/analytics/lead-attribution.tsapps/web/src/lib/api/validation/templates.tsapps/web/src/lib/services/provider-router.tsapps/web/src/lib/services/siza-local-agent.ts
Comment on lines
+60
to
68
| const { error: profileError } = await adminSupabase.from('profiles').upsert(fullProfile); | ||
| if (profileError && /tour_completed_at/i.test(profileError.message)) { | ||
| const { error: fallbackError } = await adminSupabase.from('profiles').upsert(fallbackProfile); | ||
| if (fallbackError) { | ||
| console.warn('E2E fixture profile upsert error:', fallbackError.message); | ||
| } | ||
| const reason = verifyProfileError?.message || 'onboarding/tour flags missing after update'; | ||
| base.skip(true, `Failed to verify onboarding/tour fixture flags: ${reason}`); | ||
| return; | ||
| } else if (profileError) { | ||
| console.warn('E2E fixture profile upsert error:', profileError.message); | ||
| } |
There was a problem hiding this comment.
Fail the fixture when profile seeding can't be recovered.
authenticatedPage is supposed to hand tests a post-onboarding session. If both upserts fail, this just logs a warning and continues, which pushes the failure into unrelated page assertions or onboarding redirects later in the spec.
Suggested fix
const { error: profileError } = await adminSupabase.from('profiles').upsert(fullProfile);
if (profileError && /tour_completed_at/i.test(profileError.message)) {
const { error: fallbackError } = await adminSupabase.from('profiles').upsert(fallbackProfile);
if (fallbackError) {
- console.warn('E2E fixture profile upsert error:', fallbackError.message);
+ throw new Error(`E2E fixture profile upsert error: ${fallbackError.message}`);
}
} else if (profileError) {
- console.warn('E2E fixture profile upsert error:', profileError.message);
+ throw new Error(`E2E fixture profile upsert error: ${profileError.message}`);
}📝 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
| const { error: profileError } = await adminSupabase.from('profiles').upsert(fullProfile); | |
| if (profileError && /tour_completed_at/i.test(profileError.message)) { | |
| const { error: fallbackError } = await adminSupabase.from('profiles').upsert(fallbackProfile); | |
| if (fallbackError) { | |
| console.warn('E2E fixture profile upsert error:', fallbackError.message); | |
| } | |
| const reason = verifyProfileError?.message || 'onboarding/tour flags missing after update'; | |
| base.skip(true, `Failed to verify onboarding/tour fixture flags: ${reason}`); | |
| return; | |
| } else if (profileError) { | |
| console.warn('E2E fixture profile upsert error:', profileError.message); | |
| } | |
| const { error: profileError } = await adminSupabase.from('profiles').upsert(fullProfile); | |
| if (profileError && /tour_completed_at/i.test(profileError.message)) { | |
| const { error: fallbackError } = await adminSupabase.from('profiles').upsert(fallbackProfile); | |
| if (fallbackError) { | |
| throw new Error(`E2E fixture profile upsert error: ${fallbackError.message}`); | |
| } | |
| } else if (profileError) { | |
| throw new Error(`E2E fixture profile upsert error: ${profileError.message}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/e2e/fixtures.ts` around lines 60 - 68, The fixture currently only
logs warnings when both profile upserts fail (using
adminSupabase.from('profiles').upsert with fullProfile and fallbackProfile)
which allows tests to continue in an invalid state; update the error handling in
the profile upsert block so that if the first upsert fails and the fallback
upsert also fails you throw a descriptive Error (or reject the fixture) instead
of just console.warn, including the original error messages from profileError
and fallbackError to make the failure explicit to authenticatedPage consumers.
Comment on lines
+103
to
+125
| function resolveAdminKey(supabaseUrl: string): string { | ||
| const configuredKey = requiredEnv('SUPABASE_SERVICE_ROLE_KEY'); | ||
|
|
||
| if (cachedKey && cachedUrl === supabaseUrl) { | ||
| return cachedKey; | ||
| } | ||
|
|
||
| if (!isLocalSupabaseUrl(supabaseUrl)) { | ||
| cachedUrl = supabaseUrl; | ||
| cachedKey = configuredKey; | ||
| return configuredKey; | ||
| } | ||
|
|
||
| try { | ||
| const localKey = createLocalServiceRoleJwt(supabaseUrl); | ||
| cachedUrl = supabaseUrl; | ||
| cachedKey = localKey; | ||
| return localKey; | ||
| } catch { | ||
| cachedUrl = supabaseUrl; | ||
| cachedKey = configuredKey; | ||
| return configuredKey; | ||
| } |
There was a problem hiding this comment.
Don't require SUPABASE_SERVICE_ROLE_KEY before trying the local JWT branch.
requiredEnv('SUPABASE_SERVICE_ROLE_KEY') runs unconditionally, so a local stack that could have been served by createLocalServiceRoleJwt() still throws before the Docker-derived fallback is attempted.
Suggested fix
function resolveAdminKey(supabaseUrl: string): string {
- const configuredKey = requiredEnv('SUPABASE_SERVICE_ROLE_KEY');
+ const configuredKey = process.env.SUPABASE_SERVICE_ROLE_KEY;
if (cachedKey && cachedUrl === supabaseUrl) {
return cachedKey;
}
if (!isLocalSupabaseUrl(supabaseUrl)) {
+ if (!configuredKey) {
+ throw new Error('Missing required env: SUPABASE_SERVICE_ROLE_KEY');
+ }
cachedUrl = supabaseUrl;
cachedKey = configuredKey;
return configuredKey;
}
try {
const localKey = createLocalServiceRoleJwt(supabaseUrl);
cachedUrl = supabaseUrl;
cachedKey = localKey;
return localKey;
} catch {
+ if (!configuredKey) {
+ throw new Error('Missing required env: SUPABASE_SERVICE_ROLE_KEY');
+ }
cachedUrl = supabaseUrl;
cachedKey = configuredKey;
return configuredKey;
}
}📝 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
| function resolveAdminKey(supabaseUrl: string): string { | |
| const configuredKey = requiredEnv('SUPABASE_SERVICE_ROLE_KEY'); | |
| if (cachedKey && cachedUrl === supabaseUrl) { | |
| return cachedKey; | |
| } | |
| if (!isLocalSupabaseUrl(supabaseUrl)) { | |
| cachedUrl = supabaseUrl; | |
| cachedKey = configuredKey; | |
| return configuredKey; | |
| } | |
| try { | |
| const localKey = createLocalServiceRoleJwt(supabaseUrl); | |
| cachedUrl = supabaseUrl; | |
| cachedKey = localKey; | |
| return localKey; | |
| } catch { | |
| cachedUrl = supabaseUrl; | |
| cachedKey = configuredKey; | |
| return configuredKey; | |
| } | |
| function resolveAdminKey(supabaseUrl: string): string { | |
| const configuredKey = process.env.SUPABASE_SERVICE_ROLE_KEY; | |
| if (cachedKey && cachedUrl === supabaseUrl) { | |
| return cachedKey; | |
| } | |
| if (!isLocalSupabaseUrl(supabaseUrl)) { | |
| if (!configuredKey) { | |
| throw new Error('Missing required env: SUPABASE_SERVICE_ROLE_KEY'); | |
| } | |
| cachedUrl = supabaseUrl; | |
| cachedKey = configuredKey; | |
| return configuredKey; | |
| } | |
| try { | |
| const localKey = createLocalServiceRoleJwt(supabaseUrl); | |
| cachedUrl = supabaseUrl; | |
| cachedKey = localKey; | |
| return localKey; | |
| } catch { | |
| if (!configuredKey) { | |
| throw new Error('Missing required env: SUPABASE_SERVICE_ROLE_KEY'); | |
| } | |
| cachedUrl = supabaseUrl; | |
| cachedKey = configuredKey; | |
| return configuredKey; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/e2e/helpers/admin-client.ts` around lines 103 - 125, The function
resolveAdminKey currently calls requiredEnv('SUPABASE_SERVICE_ROLE_KEY')
unconditionally which throws before trying the local JWT path; change the flow
so you only call requiredEnv when needed: check the cache first
(cachedKey/cachedUrl), then if isLocalSupabaseUrl(supabaseUrl) try
createLocalServiceRoleJwt(supabaseUrl) in a try/catch and on error fall back to
calling requiredEnv('SUPABASE_SERVICE_ROLE_KEY'), and for non-local URLs call
requiredEnv directly; update cachedUrl/cachedKey in each branch accordingly to
preserve the existing caching behavior.
Comment on lines
+32
to
+57
| for (let attempt = 0; attempt < 2; attempt++) { | ||
| await page.goto('/signin'); | ||
| await page.waitForLoadState('networkidle'); | ||
| await page.getByLabel(/email/i).fill(testEmail); | ||
| await page.getByLabel(/password/i).fill(testPassword); | ||
|
|
||
| const navigationPromise = page.waitForURL( | ||
| (url) => !url.pathname.includes('/signin') && !url.pathname.includes('/auth/callback'), | ||
| { timeout: 20000 } | ||
| ); | ||
|
|
||
| await page.getByRole('button', { name: /sign in/i }).click(); | ||
|
|
||
| try { | ||
| await navigationPromise; | ||
| signedIn = true; | ||
| break; | ||
| } catch { | ||
| signInError = await page | ||
| .locator('.text-destructive') | ||
| .first() | ||
| .textContent() | ||
| .catch(() => null); | ||
| if (!page.url().includes('/signin?')) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Tighten the post-login success check and retry loop.
This block treats any non-/signin//auth/callback navigation as success, so /auth/auth-code-error or any other failure route will still set signedIn = true. The catch path then only retries when the URL contains /signin?, so failures that stay on plain /signin break after the first attempt instead of exercising the intended second try for freshly created users.
As per coding guidelines, "apps/web/e2e/**: Review Playwright test quality. Verify test coverage of critical flows."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/e2e/helpers/auth.ts` around lines 32 - 57, The login success
predicate is too loose: update the navigationPromise used above so it only
resolves when the URL leaves signin and is not an auth error route (e.g., change
the predicate to reject URLs where url.pathname.includes('/signin') OR
url.pathname.startsWith('/auth') so routes like '/auth/auth-code-error' are not
considered success), and adjust the retry condition in the catch block so you
retry when the page is still on the signin page (e.g.,
page.url().includes('/signin')) rather than only when it contains '/signin?';
keep using the existing symbols navigationPromise, signedIn, signInError and the
page.url() check to implement these changes.
Comment on lines
+16
to
+25
| const { data: project, error: projectError } = await admin | ||
| .from('projects') | ||
| .insert({ | ||
| user_id: testUser.id, | ||
| name: `lead-marketplace-${suffix}`, | ||
| framework: 'react', | ||
| component_library: 'none', | ||
| }) | ||
| .select('id') | ||
| .single(); |
There was a problem hiding this comment.
Explicitly clean up every seeded record in this suite.
The template case deletes its template but not the project, and the gallery case never deletes the 13 inserted generations. Because the gallery assertions check total count and pagination, reruns against any reused Supabase DB stop being deterministic as smoke data accumulates.
Also applies to: 71-75, 159-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/e2e/marketplace.spec.ts` around lines 16 - 25, The test seeds a
project (const { data: project, error: projectError } = await
admin.from('projects').insert(...).select('id').single()) but later only deletes
the template and not this project, and the gallery tests insert 13 generations
that are never deleted; update the suite to explicitly delete every seeded
record after each test (or in an afterEach/afterAll): remove the inserted
generations (cleanup the rows created by admin.from('generations').insert...
using the recorded generation IDs), delete the template row(s) as already done,
and finally delete the project row using the saved project.id (or call
admin.from('projects').delete().eq('id', project.id)); ensure the cleanup runs
even on failure so subsequent test runs remain deterministic.
Comment on lines
+24
to
+29
| jest.mock('@/lib/services/siza-local-agent', () => ({ | ||
| isSizaLocalFallbackEnabled: jest.fn(() => true), | ||
| generateWithSizaLocalAgent: jest.fn( | ||
| () => 'export default function LocalSizaAgent() { return null; }' | ||
| ), | ||
| })); |
There was a problem hiding this comment.
Drop or restore the SIZA_AGENT_LOCAL_FALLBACK env mutation.
isSizaLocalFallbackEnabled is already mocked to return true, so this assignment doesn't change this suite's behavior. It does mutate global worker state, though, and can leak local-fallback behavior into later Jest files if the worker is reused.
Also applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/__tests__/lib/services/provider-router.test.ts` around lines 24
- 29, The test mutates the global SIZA_AGENT_LOCAL_FALLBACK env var
unnecessarily (the suite already mocks isSizaLocalFallbackEnabled to true),
which can leak into other tests; either remove the
process.env.SIZA_AGENT_LOCAL_FALLBACK assignment lines entirely from
provider-router.test or, if you need to set it for this file, save the original
value and restore it in afterAll/afterEach (capture old =
process.env.SIZA_AGENT_LOCAL_FALLBACK and reassign it back) so global worker
state isn't mutated; ensure this change references the mocked symbols
isSizaLocalFallbackEnabled and generateWithSizaLocalAgent used in the test.
Comment on lines
+22
to
+36
| const session = await getSession(); | ||
|
|
||
| const supabase = await createClient(); | ||
| let query = supabase.from('templates').select('*'); | ||
|
|
||
| if (validated.ownership === 'mine') { | ||
| if (!session?.user?.id) { | ||
| throw new UnauthorizedError('Authentication required for ownership=mine'); | ||
| } | ||
| query = query.eq('created_by', session.user.id); | ||
| } else if (validated.ownership === 'official') { | ||
| query = query.eq('is_official', true); | ||
| } else { | ||
| query = query.eq('is_official', true); | ||
| } |
There was a problem hiding this comment.
ownership=all is currently treated the same as official.
Lines 32-36 still apply is_official = true in the fallback branch, so the schema default of 'all' can never return non-official templates. That makes the new filter misleading and prevents the marketplace flow from exercising the community-template path.
🛠️ Suggested fix
- const session = await getSession();
-
const supabase = await createClient();
let query = supabase.from('templates').select('*');
if (validated.ownership === 'mine') {
+ const session = await getSession();
if (!session?.user?.id) {
throw new UnauthorizedError('Authentication required for ownership=mine');
}
query = query.eq('created_by', session.user.id);
} else if (validated.ownership === 'official') {
query = query.eq('is_official', true);
- } else {
- query = query.eq('is_official', true);
}📝 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
| const session = await getSession(); | |
| const supabase = await createClient(); | |
| let query = supabase.from('templates').select('*'); | |
| if (validated.ownership === 'mine') { | |
| if (!session?.user?.id) { | |
| throw new UnauthorizedError('Authentication required for ownership=mine'); | |
| } | |
| query = query.eq('created_by', session.user.id); | |
| } else if (validated.ownership === 'official') { | |
| query = query.eq('is_official', true); | |
| } else { | |
| query = query.eq('is_official', true); | |
| } | |
| const supabase = await createClient(); | |
| let query = supabase.from('templates').select('*'); | |
| if (validated.ownership === 'mine') { | |
| const session = await getSession(); | |
| if (!session?.user?.id) { | |
| throw new UnauthorizedError('Authentication required for ownership=mine'); | |
| } | |
| query = query.eq('created_by', session.user.id); | |
| } else if (validated.ownership === 'official') { | |
| query = query.eq('is_official', true); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/templates/route.ts` around lines 22 - 36, The ownership
fallback currently forces is_official=true so ownership='all' is never returned;
update the branching around validated.ownership in the route handler so that
'official' applies query.eq('is_official', true) but 'all' (or the default else)
leaves the query unchanged (i.e., do not add an is_official filter) and ensure
the 'mine' branch still checks session.user.id and applies
query.eq('created_by', session.user.id); locate and modify the code that sets
query (the validated.ownership checks and the query variable in route.ts) to
implement this behavior.
Comment on lines
+92
to
+97
| <QueryProvider> | ||
| <Suspense fallback={null}> | ||
| <AnalyticsProvider> | ||
| <FeatureFlagProvider>{children}</FeatureFlagProvider> | ||
| </AnalyticsProvider> | ||
| </Suspense> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/app/layout.tsx | head -120Repository: Forge-Space/siza
Length of output: 3724
🏁 Script executed:
find apps -name "AnalyticsProvider*" -type fRepository: Forge-Space/siza
Length of output: 115
🏁 Script executed:
cat -n apps/web/src/components/analytics/AnalyticsProvider.tsxRepository: Forge-Space/siza
Length of output: 4426
🏁 Script executed:
find apps -name "*FeatureFlagProvider*" -o -name "*feature*provider*" | grep -i providerRepository: Forge-Space/siza
Length of output: 42
🏁 Script executed:
rg "FeatureFlagProvider" --type ts --type tsx -lRepository: Forge-Space/siza
Length of output: 87
🏁 Script executed:
rg "FeatureFlagProvider" -lRepository: Forge-Space/siza
Length of output: 232
🏁 Script executed:
cat -n apps/web/src/lib/features/provider.tsxRepository: Forge-Space/siza
Length of output: 2397
Don't suspend the entire app tree just to read analytics search params.
Lines 92-97 wrap {children} in a Suspense boundary whose async consumer is AnalyticsProvider. Since AnalyticsProvider is a Client Component that calls useSearchParams(), this forces client-side resolution of the entire app tree. With fallback={null}, statically prerendered routes render a blank page until the client hydrates, breaking prerender compatibility.
AnalyticsProvider only runs side effects (analytics tracking in useEffect); it doesn't need to wrap page content. Move {children} outside the Suspense boundary and place AnalyticsProvider in its own separate Suspense:
♻️ Suggested refactor
<QueryProvider>
- <Suspense fallback={null}>
- <AnalyticsProvider>
- <FeatureFlagProvider>{children}</FeatureFlagProvider>
- </AnalyticsProvider>
- </Suspense>
+ <FeatureFlagProvider>{children}</FeatureFlagProvider>
+ <Suspense fallback={null}>
+ <AnalyticsProvider>{null}</AnalyticsProvider>
+ </Suspense>
</QueryProvider>📝 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
| <QueryProvider> | |
| <Suspense fallback={null}> | |
| <AnalyticsProvider> | |
| <FeatureFlagProvider>{children}</FeatureFlagProvider> | |
| </AnalyticsProvider> | |
| </Suspense> | |
| <QueryProvider> | |
| <FeatureFlagProvider>{children}</FeatureFlagProvider> | |
| <Suspense fallback={null}> | |
| <AnalyticsProvider>{null}</AnalyticsProvider> | |
| </Suspense> | |
| </QueryProvider> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/layout.tsx` around lines 92 - 97, The Suspense currently
wrapping {children} with AnalyticsProvider causes the whole app tree to suspend;
move {children} out of the Suspense so the app can prerender, and isolate
AnalyticsProvider inside its own Suspense boundary instead. Specifically, update
the block containing QueryProvider, Suspense, AnalyticsProvider,
FeatureFlagProvider and children so that FeatureFlagProvider (and the {children}
tree) are rendered outside the Suspense, and only AnalyticsProvider is wrapped
by Suspense (or its own lightweight boundary) to contain any client-only async
behavior from useSearchParams; keep the existing providers (QueryProvider,
FeatureFlagProvider) intact and ensure AnalyticsProvider still mounts to run its
useEffect side effects.
Comment on lines
55
to
+61
| useEffect(() => { | ||
| if (!GA_TRACKING_ID || typeof window.gtag !== 'function') return; | ||
| const url = pathname + (searchParams.toString() ? '?' + searchParams.toString() : ''); | ||
| const url = pathname + (searchParamsString ? `?${searchParamsString}` : ''); | ||
| window.gtag('config', GA_TRACKING_ID, { | ||
| page_location: url, | ||
| }); | ||
| }, [pathname, searchParams]); | ||
| }, [pathname, searchParamsString]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/components/analytics/AnalyticsProvider.tsxRepository: Forge-Space/siza
Length of output: 4426
Standardize GA page_location to absolute URL format for consistent analytics tracking.
The bootstrap GA config (line 43) reports window.location.href (absolute URL), but the route-change effect (line 57) sends a relative path. This causes the same page to be recorded under different page_location values depending on navigation mode, fragmenting analytics reports.
🛠️ Suggested fix
- const url = pathname + (searchParamsString ? `?${searchParamsString}` : '');
+ const url = new URL(
+ pathname + (searchParamsString ? `?${searchParamsString}` : ''),
+ window.location.origin
+ ).href;
window.gtag('config', GA_TRACKING_ID, {
page_location: url,
});📝 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
| useEffect(() => { | |
| if (!GA_TRACKING_ID || typeof window.gtag !== 'function') return; | |
| const url = pathname + (searchParams.toString() ? '?' + searchParams.toString() : ''); | |
| const url = pathname + (searchParamsString ? `?${searchParamsString}` : ''); | |
| window.gtag('config', GA_TRACKING_ID, { | |
| page_location: url, | |
| }); | |
| }, [pathname, searchParams]); | |
| }, [pathname, searchParamsString]); | |
| useEffect(() => { | |
| if (!GA_TRACKING_ID || typeof window.gtag !== 'function') return; | |
| const url = new URL( | |
| pathname + (searchParamsString ? `?${searchParamsString}` : ''), | |
| window.location.origin | |
| ).href; | |
| window.gtag('config', GA_TRACKING_ID, { | |
| page_location: url, | |
| }); | |
| }, [pathname, searchParamsString]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/analytics/AnalyticsProvider.tsx` around lines 55 -
61, The route-change effect in AnalyticsProvider's useEffect is sending a
relative path (using pathname and searchParamsString) to window.gtag which
fragments page_location; update the effect to construct and send an absolute URL
(e.g., build from window.location.origin + pathname + optional
?searchParamsString) when calling window.gtag('config', GA_TRACKING_ID, {
page_location: ... }), keeping the GA_TRACKING_ID and window.gtag checks intact.
Comment on lines
+31
to
+44
| function resolveLibrary( | ||
| input?: string | ||
| ): 'shadcn' | 'radix' | 'headlessui' | 'primevue' | 'material' | 'none' { | ||
| if (input === 'shadcn') { | ||
| return 'shadcn'; | ||
| } | ||
| if (input === 'mui' || input === 'chakra') { | ||
| return 'material'; | ||
| } | ||
| if (input === 'tailwind') { | ||
| return 'none'; | ||
| } | ||
| return 'none'; | ||
| } |
There was a problem hiding this comment.
Preserve explicit component-library selections in resolveLibrary().
The return type says this helper supports radix, headlessui, primevue, and material, but the implementation only recognizes shadcn, mui/chakra, and tailwind. Passing one of the already-supported tokens currently degrades to 'none', so the local fallback won't respect the caller's selected library.
🛠️ Possible fix
function resolveLibrary(
input?: string
): 'shadcn' | 'radix' | 'headlessui' | 'primevue' | 'material' | 'none' {
- if (input === 'shadcn') {
- return 'shadcn';
- }
- if (input === 'mui' || input === 'chakra') {
- return 'material';
- }
- if (input === 'tailwind') {
- return 'none';
- }
- return 'none';
+ const normalized = input?.toLowerCase();
+
+ switch (normalized) {
+ case 'shadcn':
+ case 'radix':
+ case 'headlessui':
+ case 'primevue':
+ case 'material':
+ return normalized;
+ case 'mui':
+ case 'chakra':
+ return 'material';
+ case 'tailwind':
+ default:
+ return 'none';
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/lib/services/siza-local-agent.ts` around lines 31 - 44, The
function resolveLibrary currently only recognizes 'shadcn', 'mui'/'chakra' and
'tailwind' so inputs like 'radix', 'headlessui', 'primevue' or 'material' fall
through to 'none'; update resolveLibrary to explicitly return 'radix',
'headlessui', 'primevue' and 'material' when those exact tokens are passed (and
still map 'mui'/'chakra' to 'material', 'tailwind' to 'none' and preserve
'shadcn'), and consider normalizing the input (e.g., lowercasing) before the
comparisons so callers' selections are preserved.
|
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 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
lead_signup_started,lead_signup_success,lead_signup_oauth_start,lead_signup_error)AnalyticsProviderat the app root with safe Suspense wrapping for static prerender compatibilitynextredirects/auth/auth-code-errorwith reason codeslead-readiness.spec.ts,marketplace.spec.tsPLAYWRIGHT_WEB_PORT,PLAYWRIGHT_REUSE_SERVER,NEXT_PUBLIC_E2E_DISABLE_TOUR)test:e2e:lead:preflight,test:e2e:lead:chromium,ads:google:prepublishapps/web/marketing/google-ads/siza_br_en_leadtest_v1all|official|mine) end-to-endTest plan
npm run lint(apps/web)npx tsc --noEmit(apps/web)NODE_ENV=production npm run build(apps/web)npm run format:check(repo root)npx jest --runInBand src/__tests__/lib/lead-attribution.test.ts src/__tests__/lib/services/provider-router.test.ts src/__tests__/lib/api/validation/templates.test.ts src/app/api/templates/__tests__/route.test.ts src/app/api/gallery/__tests__/route.test.ts src/__tests__/components/plugins/PluginsClient.test.tsx(apps/web)bash -n scripts/lead-preflight.sh scripts/lead-chromium.sh scripts/google-ads-prepublish.sh(apps/web)shellcheck scripts/lead-preflight.sh scripts/lead-chromium.sh scripts/google-ads-prepublish.sh(apps/web)Notes
Summary by CodeRabbit
New Features
Improvements
Tests & Infrastructure