Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Discover Services UI and a multi-step Services Quiz (UI, form primitives, providers, flow primitives), localized strings for multiple languages, a server action to append quiz submissions to Google Sheets (optional Zapier webhook), and integrates the Discover block into the Services page; also migrates prediction-flow header/progress/exit UI to the new flow primitives. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant UI as "Services Quiz UI (Providers/Stepper)"
participant Server as "appendServicesQuizRow (Server)"
participant Sheets as "Google Sheets"
participant Zapier as "Zapier Webhook (optional)"
User->>Browser: open /services -> click category
Browser->>UI: render quiz (ServicesQuizRootProvider + Flow primitives)
User->>UI: complete steps & submit
UI->>Server: call appendServicesQuizRow(payload)
Server->>Sheets: ensure header row -> append row
Server->>Zapier: POST payload (if configured)
Server-->>UI: return success
UI-->>Browser: show final screen (auto-redirect)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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 |
Cleanup: Preview Environment RemovedThe preview environment for this PR has been destroyed.
Cleanup triggered by PR close at 2026-02-18T08:48:56Z |
ncarazon
force-pushed
the
feat/services-page
branch
4 times, most recently
from
8a8b885 to
e3cf376
Compare
ncarazon
force-pushed
the
feat/services-page-quiz
branch
2 times, most recently
from
052296e to
803773d
Compare
ncarazon
force-pushed
the
feat/services-page-quiz
branch
from
93657fc to
bd2a6ed
Compare
ncarazon
force-pushed
the
feat/services-page-quiz
branch
from
bd2a6ed to
49396a3
Compare
ncarazon
force-pushed
the
feat/services-page-quiz
branch
from
49396a3 to
85eaffa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In `@front_end/messages/en.json`:
- Line 1972: The translation key organizationPlaceholder currently has the wrong
text ("Additional notes")—replace its value with a company/organization-style
placeholder such as "Your organization" or "Acme Corp" so the Organization field
displays an appropriate hint; update the value for the organizationPlaceholder
entry in the en.json translations (keep the key name organizationPlaceholder
unchanged).
In `@front_end/messages/es.json`:
- Around line 1976-1979: The translation for organizationPlaceholder is
incorrect: it currently reads "Notas adicionales" (notes) but should reflect the
organization name; update the value of the key organizationPlaceholder in
front_end/messages/es.json to a proper Spanish label such as "Nombre de la
empresa", "Organización" or "Nombre de la organización" so the placeholder
matches the field meaning.
In `@front_end/messages/pt.json`:
- Line 1976: The translation for the key organizationPlaceholder is incorrect
("Notas adicionais"); update the value for the organizationPlaceholder JSON key
to a proper Portuguese organization placeholder such as "Nome da organização"
(or simply "Organização") so it matches the Organization field shown in the PR
images; ensure the organizationPlaceholder key is updated consistently with
other locale files if present.
In `@front_end/messages/zh.json`:
- Around line 1977-1981: The JSON key organizationPlaceholder currently has the
wrong Chinese text "附加备注"; update its value to a proper organization/company
placeholder (e.g., "组织名称" or "公司名称") so it matches the field purpose—edit the
organizationPlaceholder entry in front_end/messages/zh.json to replace "附加备注"
with an appropriate translation such as "公司名称" or "组织/公司名称".
In
`@front_end/src/app/`(main)/services/components/discover_series/discover_series_block.tsx:
- Around line 1-50: The directory/name mismatch is a typo: rename the folder
discover_series to discover_services, the file discover_series_block.tsx to
discover_services_block.tsx, and the exported constant DISCOVER_SERIES_CARDS to
DISCOVER_SERVICES_CARDS; update the component file symbol references if needed
(the component is DiscoverServicesBlock), change the import of
DISCOVER_SERIES_CARDS in the component to DISCOVER_SERVICES_CARDS, and update
any other imports/usages (routes, tests, constants imports) across the repo so
all paths and symbol names match the new discover_services naming.
In
`@front_end/src/app/`(main)/services/components/templates/services_page_template.tsx:
- Line 19: Rename or align the "series" naming to "services" consistently:
update the directory and file names (discover_series → discover_services and any
files exporting DISCOVER_SERIES_CARDS → DISCOVER_SERVICES_CARDS) or rename the
component DiscoverServicesBlock to DiscoverSeriesBlock and change translation
keys (discoverServicesTitle/discoverServicesSubtitle →
discoverSeriesTitle/discoverSeriesSubtitle) so the component name
DiscoverServicesBlock, the folder discover_series, the constant
DISCOVER_SERIES_CARDS, and translation keys all use the same term; ensure
imports/exports and any references in services_page_template.tsx and the
discover_series module are updated accordingly to avoid mismatches.
In `@front_end/src/app/`(services-quiz)/append_services_quiz_row.ts:
- Around line 38-60: ensureHeaderRow has a race where two concurrent callers can
both detect an empty sheet and append duplicate HEADER_ROW; fix by making the
header write idempotent: after getSheetFirstRow shows empty, either (A) write
the header with a deterministic overwrite API call (use the Sheets batch update
/ spreadsheets.values.update to set row 1 explicitly instead of append) so you
replace whatever is at row 1 atomically, or (B) keep append but immediately
re-check getSheetFirstRow and if you find duplicates remove the extra header(s)
(compare against HEADER_ROW and delete rows via spreadsheets.batchUpdate).
Modify ensureHeaderRow to use one of these approaches referencing
ensureHeaderRow, getSheetFirstRow, appendSheetRow and HEADER_ROW.
- Around line 91-110: The Zapier webhook call using zapierWebhookUrl should be
wrapped in a try/catch and check the fetch response so failures don't bubble up
and fail the server action after the sheet row is written; update the block that
calls fetch(zapierWebhookUrl, ...) to await the call inside a try, verify
response.ok (and log non-2xx with response status/text), catch network or other
errors and log them (use the existing logging utility or console.error) but do
not rethrow so the user flow continues; keep the payload assembly with
normalizeWhoForecasts(payload.whoForecasts) and the contact fields as-is.
- Line 66: The env var name used in the mustEnv call (credentialsBase64 assigned
from "GOOGLE_CREDEBTIALS_FAB_SHEET_B64") contains a typo; update the string to
"GOOGLE_CREDENTIALS_FAB_SHEET_B64" wherever it's used (e.g., in
append_services_quiz_row.ts where credentialsBase64 is set, and also in
settings.py and fab_management/utils.py) and coordinate updating the deployed
environment variables in dev/staging/production to match the corrected name so
the mustEnv lookups succeed.
In
`@front_end/src/app/`(services-quiz)/components/fields/services_quiz_notes_input.tsx:
- Around line 9-75: The clear button in ServicesQuizNotesInput currently uses a
hardcoded aria-label "Clear"—replace it with a localized label by sourcing the
string from your i18n solution (for example call t('clear') or pass a clearLabel
prop) and use that value for aria-label (with a safe fallback like 'Clear' if
translation is missing); update the clear button code where aria-label="Clear"
to aria-label={localizedClearLabel} and ensure you import/use the appropriate
translation hook or prop in the ServicesQuizNotesInput component.
In
`@front_end/src/app/`(services-quiz)/components/fields/services_quiz_radio_card.tsx:
- Around line 29-38: In handleClick, clicking an already-selected radio should
be a no-op when no onDeselect is provided: change the control flow so that if
isSelected is true you either call onDeselect() if it exists, then return, or
simply return when onDeselect is undefined; only call onSelect() when isSelected
is false. Update the handleClick function (references: handleClick, disabled,
isSelected, onDeselect, onSelect) to perform this early return path for selected
items.
In
`@front_end/src/app/`(services-quiz)/components/quiz_state/services_quiz_flow_provider.tsx:
- Around line 83-100: The goNext handler currently awaits onSubmit and lets
rejections bubble up; add a submitError state (e.g., const [submitError,
setSubmitError] = useState<Error | null>(null)) in
services_quiz_flow_provider.tsx, wrap the await onSubmit(payload) in try/catch
inside goNext, on success clear submitError and setStep(6), on failure
setSubmitError(err) and do not advance the step (still clear isSubmitting in
finally), and expose submitError (and optionally a clearSubmitError function)
via the flow context so UI components can render user-facing error feedback;
update references to goNext, onSubmit, setIsSubmitting, setStep, and the
provider value accordingly.
In `@front_end/src/app/`(services-quiz)/components/services_quiz_header.tsx:
- Around line 46-52: The icon-only mobile exit Button (the Button element with
onClick={requestExit} rendering <FontAwesomeIcon icon={faRightFromBracket} />)
lacks an accessible label; update that Button to include an explicit accessible
name (e.g., add aria-label="Exit" or aria-label="Close quiz" or include
visually-hidden text for screen readers) and ensure the label is localized if
needed so assistive technologies can convey the button's purpose while
preserving the icon-only visual.
In `@front_end/src/app/`(services-quiz)/components/services_quiz_screen.tsx:
- Around line 50-59: ServicesQuizScreenInner currently casts step as
ServicesQuizStepId and directly reads STEP_COMPONENTS[step], which can be
undefined and crash when rendering <ActiveStep />; update
ServicesQuizScreenInner to validate the step returned from useServicesQuizFlow
(or check STEP_COMPONENTS[step]) and provide a safe fallback (e.g., render a
default component, a NotFound/StepError component, or null) when the lookup
yields undefined, and optionally log or assert the invalid step value to aid
debugging.
In `@front_end/src/app/`(services-quiz)/components/steps/services_quiz_step_1.tsx:
- Around line 24-31: The challenge labels from SERVICES_QUIZ_CHALLENGES are
being rendered directly in ServicesQuizToggleChip (see the map over challenges
and props label={ch}); update the rendering so the label is localized via the
i18n helper (use t(ch) or map challenge identifiers to translation keys and call
t(key)) and ensure state.selectedChallenges and toggleChallenge still use the
same identifier logic (compare/emit identifiers, not translated strings), or if
these strings must remain untranslated, add a clear code comment above the
SERVICES_QUIZ_CHALLENGES reference explaining why they are intentionally kept in
English.
In `@front_end/src/app/`(services-quiz)/components/steps/services_quiz_step_5.tsx:
- Around line 43-50: The Zod schema for contact fields returns Zod's default
error strings, so the errors computed in the useMemo (errors in
services_quiz_step_5.tsx) end up with messages that don't match your translation
keys; update the contactSchema in services_quiz_completion_provider.tsx to
provide custom error messages (e.g., for contactName use .min(1, { message:
"fieldRequired" }) or similar, and for contactEmail use .email({ message:
"invalidEmail" }) and .min(1, { message: "fieldRequired" }) as needed) so the
f.contactName._errors[0] / f.contactEmail._errors[0] values are the translation
keys expected by t(); apply the same custom-message fix to the other schema
fields referenced around the other occurrences (the blocks you noted at 116–117
and 131–134) so all validation returns the correct translation keys.
- Around line 160-171: The submit button is disabled by checking !canSubmit
which prevents users from clicking to reveal validation errors (showErrors is
only toggled in handleSubmit); change the button's disabled prop to only depend
on isSubmitting (remove !canSubmit) so the user can click Submit to trigger
handleSubmit, and ensure handleSubmit still sets showErrors = true and aborts
when canSubmit is false (so validation guard remains in handleSubmit).
Reference: button's disabled prop, canSubmit, isSubmitting, handleSubmit, and
showErrors.
In `@front_end/src/app/`(services-quiz)/constants.ts:
- Around line 9-41: The challenge strings in the exported object (keys like
enterprise, government, "non-profit", academia in constants.ts) are hardcoded
English labels shown in the UI; switch these to translation keys (e.g.,
"challenges.enterprise.product_feature_bets") and use next-intl to render them,
or keep the existing values as stable IDs and add a parallel displayLabels map
that maps those IDs to i18n keys so UI components use t(displayLabels[...])
while any backend/Sheet submissions continue to use the stable IDs.
In `@front_end/src/app/`(services-quiz)/helpers.ts:
- Around line 15-20: The contact object construction in helpers.ts currently
converts empty name/email to null via state.contactName.trim() || null and
state.contactEmail.trim() || null, which can break downstream code expecting
required values; update the logic in the function that builds contact
(reference: contact, state.contactName, state.contactEmail) to enforce required
fields by validating trim() results and either (a) throw a descriptive
Error/assert when name or email are empty, or (b) return a validation error
object upstream so the caller can block submission—pick one approach and
implement it consistently (do not silently convert to null).
In `@front_end/src/components/flow/flow_stepper.tsx`:
- Around line 113-137: FlowStepperSegment renders an empty Button which is
inaccessible to screen readers; update the component (FlowStepperSegment) to
provide a meaningful accessible label by passing an aria-label (and optionally
title) to the Button using the step index and/or step data (e.g.,
`aria-label={`Step ${index + 1}${step?.title ? `: ${step.title}` : ''}`}`),
ensuring the onClick handler (onSelectStep) and existing props remain unchanged
and that useFlowStepper-provided step/title are used when available.
🧹 Nitpick comments (10)
front_end/src/app/(services-quiz)/constants.ts (1)
43-48:SERVICES_QUIZ_CATEGORIESduplicates keys fromSERVICES_QUIZ_CHALLENGES.Consider deriving it to avoid drift:
♻️ Suggested refactor
-export const SERVICES_QUIZ_CATEGORIES: ServicesQuizCategory[] = [ - "enterprise", - "government", - "non-profit", - "academia", -]; +export const SERVICES_QUIZ_CATEGORIES = Object.keys( + SERVICES_QUIZ_CHALLENGES +) as ServicesQuizCategory[];front_end/src/app/(prediction-flow)/components/header.tsx (1)
36-78: Dual navigation paths to the tournament page — verify no redundancy.
useExitGuard.onExit(Line 38) navigates to the tournament page whencanExitImmediatelyis true. When false,ExitFlowModal'sprimaryAction(inexit_flow_modal.tsxLine 41) also navigates to the same URL. This works correctly as two mutually exclusive paths, but the tournament URL is now duplicated in two places. Consider centralizing it if this becomes harder to maintain.front_end/src/app/(services-quiz)/components/services_quiz_stepper.tsx (2)
39-40: Magic step IDs5and6should reference shared constants.
ServicesQuizStepIdis defined as1 | 2 | 3 | 4 | 5 | 6in the answers provider, but the semantic meaning of each step isn't captured. Consider extracting named constants (e.g.,SUBMIT_STEP = 5,FINAL_STEP = 6) to avoid scattered magic numbers.
52-56:isFinalStepbranch innextLabelis dead code givenhideOnFinalStepdefaults totrue.When
hideOnFinalStepistrue(the default), the component returnsnullon Line 47 before reaching this label logic. TheisFinalStep ? t("next")branch only executes ifhideOnFinalStepis explicitly set tofalse, which may be an untested/unintended path.front_end/src/app/(services-quiz)/components/quiz_state/services_quiz_answers_provider.tsx (1)
84-140:useMemorecreates the entire API on any state change.Since every state value is in the dependency array, any single field change (e.g., typing a character in
contactName) causes all callbacks and the state object to be recreated. For a quiz form this is unlikely to cause performance issues, but if consumers of this context start multiplying, consider splitting the state from the action callbacks (e.g., separateuseReducer+ stable dispatch, or separate contexts for state vs. actions).front_end/src/components/flow/flow_stepper.tsx (1)
50-70: Consider stabilizing callback identity to prevent unnecessary context re-renders.
useMemoon Line 58 includesonToggleMenuandonSelectStepin its dependency array. If the parent creates these inline (e.g.,() => setIsMenuOpen(!isMenuOpen)), the context value changes every render, defeating the memo and re-rendering all consumers.The parent (
ProgressSection) passes() => setIsMenuOpen(!isMenuOpen)inline at Line 69, which will cause this.♻️ Suggested fix in progress_section.tsx (consumer side)
Wrap callbacks with
useCallbackin the parent, or accept that this is a low-impact issue given the small component tree.+ import { useCallback } from "react"; ... + const handleToggleMenu = useCallback(() => setIsMenuOpen(prev => !prev), [setIsMenuOpen]); + const handleSelectStep = useCallback((id: FlowStepId) => changeActivePost(id as number), [changeActivePost]); ... <FlowStepperRoot steps={steps} activeStepId={currentPostId} isMenuOpen={isMenuOpen} - onToggleMenu={() => setIsMenuOpen(!isMenuOpen)} - onSelectStep={(id) => changeActivePost(id as number)} + onToggleMenu={handleToggleMenu} + onSelectStep={handleSelectStep} >front_end/src/app/(services-quiz)/components/quiz_state/services_quiz_progress_provider.tsx (1)
31-42:useMemodependency onstateobject likely defeats memoization.If
stateis a new object reference on each render (e.g., from a reducer), thisuseMemorecomputes every time. Consider destructuring the individual fields used in the computation into the dependency array for effective memoization:♻️ Suggested improvement
+ const { + selectedChallenges, + notes, + timing, + whoForecasts, + privacy, + contactName, + contactEmail, + } = state; + const hasProgress = useMemo(() => { return ( step > 1 || - state.selectedChallenges.length > 0 || - state.notes.trim().length > 0 || - !!state.timing || - !!state.whoForecasts || - !!state.privacy || - state.contactName.trim().length > 0 || - state.contactEmail.trim().length > 0 + selectedChallenges.length > 0 || + notes.trim().length > 0 || + !!timing || + !!whoForecasts || + !!privacy || + contactName.trim().length > 0 || + contactEmail.trim().length > 0 ); - }, [step, state]); + }, [step, selectedChallenges, notes, timing, whoForecasts, privacy, contactName, contactEmail]);front_end/src/app/(services-quiz)/components/steps/services_quiz_step_3.tsx (1)
22-58: Consider disabling toggle chips when "Not Sure" is selected.When
isNotSureis true, the toggle chips still appear interactive (not disabled), even thoughselectedis empty. If the provider allows toggling a chip while in "not_sure" mode, the UX intent may be unclear. Passingdisabled={isNotSure}toServicesQuizToggleChipwould make the mutual exclusivity visually explicit.front_end/src/app/(services-quiz)/components/steps/services_quiz_step_5.tsx (1)
86-88:useLayoutEffectmissingsyncCommentsHeightin dependency array.React will warn about the missing dependency. Since
syncCommentsHeightis recreated on every render, consider either wrapping it inuseCallbackor inlining the logic directly in the effect.front_end/messages/en.json (1)
1936-1977: Minor duplication:"continue"(Line 1949) vs existing"continueButton"(Line 112).Both map to
"Continue". Consider reusing the existing key to keep the translation surface smaller, unless the distinction is intentional (e.g., different casing or context in other locales).
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@front_end/messages/cs.json`:
- Around line 1959-1984: The Czech pluralization for "stepsLeft" and
"automaticRedirectInSeconds" currently uses only one/other and must be changed
to Czech categories one/few/other; update the "stepsLeft" message key to use
plural forms one {# krok zbývá}, few {# kroky zbývají}, other {# kroků zbývá}
and update "automaticRedirectInSeconds" to one {# sekunda}, few {# sekundy},
other {# sekund} so the UI shows grammatically correct Czech for 1, 2–4, and 5+
counts.
In `@front_end/messages/en.json`:
- Line 1977: The "automaticRedirectInSeconds" message currently uses a fixed
string and needs ICU pluralization; update the value for the key
"automaticRedirectInSeconds" to an ICU plural form using the {count, plural, one
{Automatic redirect in # sec} other {Automatic redirect in # secs}} pattern
(keeping the {count} placeholder) so the label uses "sec" for singular and
"secs" for plural.
In `@front_end/messages/es.json`:
- Around line 1954-1976: Several message keys in this block mix formal and
informal Spanish (e.g., "selectChallengesYouAreCurrentlyFacing",
"submitYourAnswersTitle", "weHaveReceivedYourRequest", "privacyQuestionTitle",
"semiConfidentialityDescription"); standardize them to a single register (choose
informal as suggested) by updating the formal strings ("Seleccione", "Envíe",
"Hemos recibido", "¿Qué tan confidencial es este trabajo?") to informal
equivalents ("Selecciona", "Envía", "Hemos recibido tu solicitud" already
informal? adjust to "Hemos recibido tu solicitud" keep, change others
accordingly) — specifically edit the values for keys:
selectChallengesYouAreCurrentlyFacing, submitYourAnswersTitle,
weHaveReceivedYourRequest (verify tone), privacyQuestionTitle,
semiConfidentialityDescription, fullConfidentialityDescription and any other
lines using "usted/tu" inconsistency so all use the informal "tú/tu" forms
consistently across the block. Ensure punctuation and ellipses (e.g., "Escribe
aquí…") remain unchanged.
In
`@front_end/src/app/`(services-quiz)/components/quiz_state/services_quiz_flow_provider.tsx:
- Around line 85-107: The call to aggregateServicesQuizAnswers(state) is outside
the try/catch and can throw; move that call inside the try block (after
setIsSubmitting(true)) so any errors are caught and handled by the catch which
sets setSubmitError, keep setIsSubmitting(true) before the try so the UI shows
submitting, then await onSubmit?.(payload) as before and restore
setIsSubmitting(false) in finally; update references in goNext to use the
payload created inside the try and preserve the step transition to setStep(6).
In `@front_end/src/app/`(services-quiz)/components/steps/services_quiz_step_5.tsx:
- Around line 80-84: The component's handleSubmit calls await goNext() but
doesn't surface submission failures because it never reads submitError from
useServicesQuizFlow(); destructure submitError (e.g., const { goNext,
submitError, ... } = useServicesQuizFlow()) and render a visible error message
under the submit button (use the existing translation key t("submissionFailed")
or similar) so users see failures; ensure the error UI is conditional on
submitError and styled/placed as suggested (after the submit button) and keep
existing showErrors/validation logic intact.
In `@front_end/src/services/google_spreadsheets.ts`:
- Around line 125-142: The setSheetRow function currently builds the range using
`${sheetName}!A${rowIndex}` without validating rowIndex; add a defensive
validation in setSheetRow that ensures rowIndex is a positive integer (> 0) (and
optionally coerce/validate numeric input) and throw a clear Error if invalid
before calling getSheetsClient or sheets.spreadsheets.values.update; reference
the existing range construction and opts destructure to locate where to add the
check.
- Around line 72-90: The four exported functions (appendSheetRow,
appendSheetRows, setSheetRow, getSheetFirstRow) currently let upstream errors
bubble to the caller; wrap each function's body in a try/catch, catch any error
from getSheetsClient or sheets API calls, and either (a) return a safe fallback
value consistent with getAllSheetsData (e.g., null/empty result) or (b) rethrow
a new Error with added contextual info (include spreadsheetId, sheetName and the
original error.message) so the server action can handle it; ensure you reference
the same function names when updating behavior so callers can be updated or
tests adjusted.
- Around line 46-50: The ranges built in getAllSheetsData use raw sheetName
values and will break for names with spaces/special characters; add a small
helper (e.g., quoteSheetName or toQuotedSheetRange) that wraps sheet names in
single quotes and escapes any existing single quotes, then use that helper
wherever ranges are constructed (the sheetNames.map callback and the other four
places that build `${sheetName}!A1:Z`-style ranges before calling
sheets.spreadsheets.values.get/batchGet). Update all five range expressions to
call the helper so every API range is correctly quoted.
🧹 Nitpick comments (8)
front_end/messages/zh-TW.json (1)
1973-1973: Consider using a locale-appropriate placeholder name.
"約翰·多伊"is a direct transliteration of "John Doe." For zh-TW users, a familiar example name like"王小明"would feel more natural and immediately recognizable as placeholder text.Suggested change
- "yourNamePlaceholder": "約翰·多伊", + "yourNamePlaceholder": "王小明",front_end/src/components/flow/flow_stepper.tsx (2)
81-98: Menu toggle button lacks an accessible label.The
FlowStepperMenuTogglerenders an icon-only<Button>without anaria-labelor visible text. Screen readers will announce it as an unlabeled button.♿ Proposed fix
<Button variant="tertiary" onClick={onToggleMenu} className="h-8 w-8 rounded-full px-2 py-2" + aria-label={isMenuOpen ? "Close menu" : "Open menu"} >
50-70: Context value stability depends on caller memoizing callbacks.
onToggleMenuandonSelectStepare included in theuseMemodeps. If the parent passes unstable (non-memoized) callbacks, the context value will be recreated on every render, potentially causing unnecessary re-renders in all consumers. This is fine if callers are disciplined aboutuseCallback, just worth noting for maintainability.front_end/src/app/(services-quiz)/components/steps/services_quiz_step_5.tsx (1)
86-88:syncCommentsHeightmissing fromuseLayoutEffectdependency array.The
syncCommentsHeightfunction is referenced inside the effect but not listed as a dependency. This is functionally safe here (it only reads a stable ref and a constant), but it would fail thereact-hooks/exhaustive-depslint rule. Wrapping it inuseCallbackor inlining it in the effect would satisfy the rule.front_end/src/app/(services-quiz)/components/quiz_state/services_quiz_flow_provider.tsx (1)
119-147:setStepis exposed in context but missing fromuseMemodeps.
setStepis included in thevalueobject (line 122) but omitted from theuseMemodependency array (lines 134-146). React'suseStatesetter is stable across renders so this won't cause bugs, but it's technically an incomplete dependency list.front_end/src/services/google_spreadsheets.ts (3)
17-29: A newGoogleAuthinstance is created on every function call.
getSheetsClientconstructs a freshGoogleAuthand Sheets client for each invocation. IfappendSheetRowandgetSheetFirstRoware called in sequence (as the quiz submission flow likely does), this creates two separate auth instances and potentially two token exchanges.For the current quiz use case the overhead is negligible, but if this module is reused for higher-throughput scenarios, consider caching clients keyed by
(credentialsBase64, mode).
1-9:decodeCredentialswill throw opaque errors on malformed input.If
credentialsBase64is not valid base64 or not valid JSON,Buffer.from(...).toString()silently produces garbage, andJSON.parsethrows a genericSyntaxError. A descriptive error message would improve debuggability.♻️ Proposed improvement
function decodeCredentials(credentialsBase64: string) { - return JSON.parse(Buffer.from(credentialsBase64, "base64").toString()); + try { + return JSON.parse(Buffer.from(credentialsBase64, "base64").toString()); + } catch (e) { + throw new Error("Failed to decode Google Sheets credentials from base64", { + cause: e, + }); + } }
40-43: Empty sheet name fallback could produce an invalid API range.On Line 41, if
s.properties?.titleis falsy, the fallback|| ""produces an empty string. This empty name is then used in the range`${sheetName}!A1:Z`on Line 49, which becomes"!A1:Z"— an invalid range that will cause an API error (caught by the outer try/catch, so it's safe, but the entire function silently returns[]hiding the real issue).Consider filtering out sheets with empty/missing titles instead:
♻️ Proposed fix
const sheetNames = - spreadsheet.data.sheets?.map((s) => s.properties?.title || "") ?? []; + spreadsheet.data.sheets + ?.map((s) => s.properties?.title) + .filter((title): title is string => !!title) ?? [];
ncarazon
force-pushed
the
feat/services-page-quiz
branch
from
505f808 to
c67b9ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@front_end/messages/zh-TW.json`:
- Line 1973: Replace the current Traditional Chinese placeholder value for the
"yourNamePlaceholder" key (currently "約翰·多伊") with a more culturally appropriate
Taiwanese example such as "王小明"; update the JSON value for "yourNamePlaceholder"
in zh-TW locale so the placeholder reads idiomatically for Traditional Chinese
users.
In
`@front_end/src/app/`(services-quiz)/components/fields/services_quiz_radio_card.tsx:
- Around line 43-44: The component services_quiz_radio_card.tsx is hardcoding
role="radio" which is wrong for multi-select uses; update the component to
derive the ARIA role dynamically (e.g., use "checkbox" when an onDeselect prop
exists or accept a new prop like ariaRole/inputType) and set aria-checked
accordingly, ensure any consumers pass onDeselect (or the new prop) so
multi-select instances render role="checkbox" while single-select keep
role="radio"; adjust prop types/signature (and usage of isSelected/onDeselect)
to reflect the new prop and preserve existing behavior.
In
`@front_end/src/app/`(services-quiz)/components/quiz_state/services_quiz_flow_provider.tsx:
- Around line 108-116: selectStep currently lets users jump directly to the
final step (6) by only checking TOTAL_STEPS and isSubmitting; modify selectStep
to block navigation to the final step unless the quiz has been successfully
submitted/completed: inside selectStep (the callback referencing isSubmitting,
TOTAL_STEPS, and setStep) add a guard that if next equals the final step id (6
or the ServicesQuizFinal/FinalStep constant) then return unless a
submitted/completed flag (e.g., isSubmitted or quizCompleted) is true; keep the
existing checks for TOTAL_STEPS and isSubmitting and only call setStep(next)
when all checks pass.
In `@front_end/src/components/flow/flow_stepper.tsx`:
- Around line 81-98: FlowStepperMenuToggle currently renders a Button with only
an icon so screen readers can't tell its purpose; update the Button in the
FlowStepperMenuToggle component to include an accessible label (e.g., add an
aria-label or aria-pressed attribute) that reflects state using isMenuOpen (such
as "Open menu" when isMenuOpen is false and "Close menu" when true), leaving
onToggleMenu and the FontAwesomeIcon usage unchanged; ensure the label text is
kept in sync with the icon state so assistive tech announces the correct action.
🧹 Nitpick comments (2)
front_end/src/services/google_spreadsheets.ts (1)
87-87: Null-to-empty-string mapping is duplicated in three places.The
v === null ? "" : vtransform appears identically at lines 87, 113, and 143. Consider extracting a small helper to DRY this up.♻️ Suggested helper
+type CellValue = string | number | boolean | null; + +function sanitizeRow(row: CellValue[]): (string | number | boolean)[] { + return row.map((v) => (v === null ? "" : v)); +}Then replace each inline
.map(...):- values: [row.map((v) => (v === null ? "" : v))], + values: [sanitizeRow(row)],Also applies to: 113-113, 143-143
front_end/src/app/(services-quiz)/components/steps/services_quiz_step_5.tsx (1)
84-86:syncCommentsHeightmissing fromuseLayoutEffectdependency array.The
eslint-plugin-react-hooksexhaustive-depsrule would flag this. It's not a runtime bug here since the function only reads from a ref and the constantMAX_ROWS, but wrappingsyncCommentsHeightinuseCallback(with no deps) and adding it to the effect's dependency array would satisfy the linter and make the intent explicit.♻️ Proposed fix
- const syncCommentsHeight = () => { + const syncCommentsHeight = useCallback(() => { const el = commentsRef.current; if (!el) return; const styles = window.getComputedStyle(el); const lineHeight = Number.parseFloat(styles.lineHeight || "20"); const paddingY = Number.parseFloat(styles.paddingTop || "0") + Number.parseFloat(styles.paddingBottom || "0"); const borderY = Number.parseFloat(styles.borderTopWidth || "0") + Number.parseFloat(styles.borderBottomWidth || "0"); const maxHeight = lineHeight * MAX_ROWS + paddingY + borderY; el.style.height = "0px"; const nextHeight = Math.min(el.scrollHeight, maxHeight); el.style.height = `${nextHeight}px`; el.style.overflowY = el.scrollHeight > maxHeight ? "auto" : "hidden"; - }; + }, []); useLayoutEffect(() => { syncCommentsHeight(); - }, [state.contactComments]); + }, [syncCommentsHeight, state.contactComments]);
There was a problem hiding this comment.
I ran this locally and it worked as expected. I looked over the code briefly and didn't see any red flags. Another should comment on the actual code quality though.
I have 2 notes:
- I can see is that this adds a few new env variables, but they aren't referenced in the
.env.example. Can you add them?
I found at least:
SERVICES_QUIZ_GOOGLE_SHEETS_SPREADSHEET_ID
GOOGLE_CREDEBTIALS_FAB_SHEET_B64
SERVICES_QUIZ_ZAPIER_WEBHOOK_URL
there might be others? - We don't fail gracefully if
SERVICES_QUIZ_GOOGLE_SHEETS_SPREADSHEET_IDdoesn't exist. If it doesn't exist, you shouldn't even render the quiz section.
ncarazon
force-pushed
the
feat/services-page-quiz
branch
from
c67b9ca to
44c7eb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@front_end/src/app/`(services-quiz)/append_services_quiz_row.ts:
- Around line 64-91: The appendServicesQuizRow server action currently lets
errors from ensureHeaderRow or appendSheetRow bubble up with raw details; wrap
the body of appendServicesQuizRow in a try/catch, catch any thrown error from
ensureHeaderRow or appendSheetRow, log the full error server-side (including
credentials/spreadsheetId) and then either return a structured sanitized error
object or re-throw a new Error with a generic message (e.g., "Failed to submit
quiz") so internal details are not sent to the client; make sure to reference
appendServicesQuizRow, ensureHeaderRow, and appendSheetRow when locating where
to add the try/catch and where to sanitize the error before returning or
re-throwing.
🧹 Nitpick comments (6)
front_end/src/services/google_spreadsheets.ts (2)
7-8:decodeCredentialscan throw confusing errors on malformed input.If
credentialsBase64is not valid base64 or not valid JSON,JSON.parse(Buffer.from(...))will throw a genericSyntaxError. Consider wrapping with a more descriptive error message to aid debugging in production.Proposed improvement
function decodeCredentials(credentialsBase64: string) { - return JSON.parse(Buffer.from(credentialsBase64, "base64").toString()); + try { + return JSON.parse(Buffer.from(credentialsBase64, "base64").toString()); + } catch (err) { + throw new Error( + `Failed to decode Google credentials from base64: ${err instanceof Error ? err.message : err}` + ); + } }
17-29: Client factory creates a new auth instance per call.
getSheetsClientinstantiates a freshGoogleAuthand Sheets client on every invocation. InappendServicesQuizRow, this means 2–3 separate client creations per submission (forgetSheetFirstRow, possiblysetSheetRow, andappendSheetRow). For the current low-traffic quiz use case this is acceptable, but if this service is reused in higher-throughput contexts, consider caching the client per credentials+scope pair.front_end/src/app/(services-quiz)/components/quiz_state/services_quiz_answers_provider.tsx (2)
84-140: All callbacks are recreated on any state change due touseMemodependencies.The
useMemodepends on all 10 state values, so every keystroke in any field (e.g., contact name) recreates all callbacks includingtoggleChallenge,toggleWhoForecastsSelection, etc. For this small quiz form, the performance impact is negligible — but if you notice unnecessary re-renders in child components later, consider usinguseCallbackfor the toggle functions (which use functional updaters and don't need external state in their closures) and composing the API object separately.
14-14:ServicesQuizStepIdis defined here but not used within this file.This type seems more related to the flow/progress providers than the answers state. Consider co-locating it with the step/flow logic (e.g., in
constants.tsor the flow provider) for better discoverability.front_end/src/app/(services-quiz)/components/steps/services_quiz_step_shell.tsx (1)
5-31:classNameis applied to the<h2>title, not the root element — consider renaming for clarity.Consumers of
ServicesQuizStepShellmight expectclassNameto apply to the outer wrapper. Since it targets the title's<h2>, a name liketitleClassNamewould make the intent immediately clear and prevent surprises.♻️ Suggested rename
type Props = PropsWithChildren<{ title: string; - className?: string; + titleClassName?: string; }>; const ServicesQuizStepShell: React.FC<Props> = ({ children, title, - className, + titleClassName, }) => { return ( <div className="flex flex-1 flex-col sm:justify-center"> <div className="mx-4 mt-4 sm:-mt-[170px] md:mx-0"> <h2 className={cn( "m-0 mb-4 text-[20px] font-bold leading-[25px] text-blue-700 dark:text-blue-700-dark sm:mb-9 sm:text-center sm:text-[28px] sm:leading-[34px]", - className + titleClassName )} >front_end/src/app/(services-quiz)/components/steps/services_quiz_step_1.tsx (1)
30-30:disabled={!state.category}is unreachable — challenges array is empty when category is null.Line 17 returns
[]whenstate.categoryis falsy, so no chips render anddisablednever takes effect. This is harmless but unnecessary.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@front_end/.env.example`:
- Line 23: Fix the typo in the environment variable name by renaming
GOOGLE_CREDEBTIALS_FAB_SHEET_B64 to GOOGLE_CREDENTIALS_FAB_SHEET_B64 in
front_end/.env.example and update any future usages or docs that reference
GOOGLE_CREDEBTIALS_FAB_SHEET_B64 to the corrected
GOOGLE_CREDENTIALS_FAB_SHEET_B64 to ensure consistency (search for the old
symbol to replace any occurrences).
In `@front_end/messages/zh.json`:
- Around line 1962-1980: Several message values in the Chinese locale remain in
English; update the JSON values for the keys "howSoonDoYouNeedForecasts",
"learnAboutMetaculusPros", "metaculusProsDescription",
"semiConfidentialityDescription", "fullConfidentialityDescription",
"noConfidentiality", and "noConfidentialityDescription" to proper Chinese
translations so the zh.json locale is fully localized; locate and replace the
English strings for those exact keys in front_end/messages/zh.json with concise
Chinese equivalents that match tone and meaning of the surrounding entries
(e.g., translate the question, the Metaculus Pro description paragraph, and the
confidentiality option labels/descriptions).
🧹 Nitpick comments (1)
front_end/.env.example (1)
21-21: Consider expanding the comment to document all three variables.The comment only explains the behavior when the spreadsheet ID is missing, but doesn't describe the purpose of the credentials or Zapier webhook variables. Adding brief descriptions would improve maintainability.
📝 Suggested documentation enhancement
-# Services Quiz (optional — quiz section hidden if spreadsheet ID is missing) +# Services Quiz (optional — quiz section hidden if spreadsheet ID is missing) +# - SERVICES_QUIZ_GOOGLE_SHEETS_SPREADSHEET_ID: Target spreadsheet for quiz submissions +# - GOOGLE_CREDENTIALS_FAB_SHEET_B64: Base64-encoded Google service account credentials +# - SERVICES_QUIZ_ZAPIER_WEBHOOK_URL: Optional webhook for additional integrations
ncarazon
force-pushed
the
feat/services-page-quiz
branch
from
3a9aa19 to
c7b76b0
Compare
ncarazon
force-pushed
the
feat/services-page-quiz
branch
from
9565bd7 to
f03a6e0
Compare
3 participants
This PR adds a services quiz page — a multi-step guided flow that helps prospective clients discover Metaculus services based on their organization type and needs. It also adds a "Discover Services" entry-point section to the services page, and extracts reusable flow components from the existing prediction flow.
Changes:
/serviceswith 4 organization-type cards (Enterprise, Government, Non-Profit, Academia)/services/quizcollecting challenges, timeline, forecaster preferences, confidentiality, and contact infogoogle_spreadsheets.tsserviceFlowHeader,FlowStepper,FlowExitConfirmModal,useExitGuard) intosrc/components/flow/Discover Services section on
/servicesQuiz Step 1 – Challenge selection
Quiz Step 2 — Timeline
Quiz Step 3 — Forecaster selection
Quiz Step 4 — Confidentiality level
Quiz Step 5 — Contact form
Final screen
Summary by CodeRabbit
New Features
Refactor
Chores
Fixes