[#342] Allow editing responses in Blade forms#315
Conversation
d0196b8 to
d821480
Compare
apps/blade/src/app/forms/[formName]/_components/form-view-edit-client.tsx
Outdated
Show resolved
Hide resolved
be00a54 to
e14456a
Compare
f0a2818 to
9e4e85b
Compare
📝 WalkthroughWalkthroughAdds an "allow edit" flow across DB, API, admin UI, and client UI: new Form.allowEdit and FormResponse.editedAt, an Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Server as Next.js
participant API as TRPC
participant DB
User->>Browser: Open form page
Browser->>Server: Request page (getForm)
Server->>API: getForm(slug)
API->>DB: SELECT form (includes allowEdit)
DB-->>API: form + allowEdit
API-->>Server: form data
Server-->>Browser: Render FormRunner (respond mode)
User->>Browser: Submit response
Browser->>Browser: normalizeResponses + Zod validation
Browser->>API: createResponse(payload)
API->>DB: INSERT response (submittedAt, editedAt)
DB-->>API: inserted row
API-->>Browser: success (responseId)
Browser->>Browser: show SubmissionSuccessCard, redirect countdown
alt allowEdit = true
User->>Browser: Click "Edit" on response
Browser->>Server: Request response view
Server->>API: getUserResponse(responseId)
API->>DB: SELECT response + form (allowEdit)
DB-->>API: response + allowEdit
API-->>Server: payload
Server-->>Browser: Render FormRunner (isReview, editable)
User->>Browser: Submit edits
Browser->>API: editResponse(id, payload)
API->>DB: UPDATE response (editedAt)
DB-->>API: update result
API-->>Browser: updated timestamp
else
User->>Browser: View response (read-only FormRunner)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
0848650 to
842d71d
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/blade/src/app/admin/forms/[slug]/client.tsx (1)
329-331:⚠️ Potential issue | 🟡 MinorMissing
allowEditin auto-save trigger.The other toggle switches (
duesOnly,allowResubmission) auto-save when changed, butallowEditis missing from this effect's dependency array. This creates inconsistent UX—users must manually save or wait for periodic auto-save.Proposed fix
// auto save trigger when toggle switches are changed useEffect(() => { if (!isLoading) handleSaveForm(); - }, [duesOnly, allowResubmission, responseRoleIds, isLoading]); // removed handleSaveForm to prevent save-on-every-render + }, [duesOnly, allowResubmission, allowEdit, responseRoleIds, isLoading]); // removed handleSaveForm to prevent save-on-every-renderpackages/api/src/routers/forms.ts (1)
586-605:⚠️ Potential issue | 🟠 MajorInconsistent
allowEditvalue — always returnsfalsefor single-response queries.When fetching by
responseId,allowEditis hardcoded tofalse(line 595). However, the "all responses" path (line 637) correctly returnsFormsSchemas.allowEdit. This inconsistency could cause the UI to incorrectly hide the edit button when viewing a single response.🐛 Proposed fix: Use actual form's allowEdit value
return await db .select({ submittedAt: FormResponse.editedAt, responseData: FormResponse.responseData, formName: FormsSchemas.name, formSlug: FormsSchemas.slugName, id: FormResponse.id, hasSubmitted: sql<boolean>`true`, - allowEdit: sql<boolean>`false`, + allowEdit: FormsSchemas.allowEdit, }) .from(FormResponse) .leftJoin(FormsSchemas, eq(FormResponse.form, FormsSchemas.id)) .where( and( eq(FormResponse.id, responseId), eq(FormResponse.userId, userId), ), );
🤖 Fix all issues with AI agents
In `@apps/blade/src/app/admin/forms/`[slug]/client.tsx:
- Around line 580-592: The Switch and Label for the "Allow Response Edit" toggle
have mismatched identifiers (Switch id="allow-resubmit" vs Label
htmlFor="allow-edit"), breaking the label-input association; update one so both
match (e.g., set the Switch id to "allow-edit" or the Label htmlFor to
"allow-resubmit") so the Label correctly targets the Switch used by the
allowEdit state and setAllowEdit handler (look for the Switch and Label
components around the allowEdit/setAllowEdit usage).
In
`@apps/blade/src/app/dashboard/_components/member-dashboard/forms/form-responses.tsx`:
- Around line 38-44: The Link wrapping a Button creates nested interactive
elements (Link > Button) which is invalid; update the FormResponses row to
render a single interactive element: either make Button render the anchor by
using Button's asChild prop (if supported) so Button acts as the Link, passing
href={`/forms/${formResponse.formSlug}/${formResponse.id}`}, or remove Button
and apply the button styling/ARIA to the Link itself so the Link becomes the
sole interactive element that shows "Edit" or "View" based on
formResponse.allowEdit; ensure focus/role/aria-labels remain correct.
In `@apps/blade/src/app/forms/`[formName]/_components/form-runner.tsx:
- Around line 124-128: The conditional {form.banner && <div
className="overflow-hidden rounded-lg"></div>} renders an empty container;
update the FormRunner rendering so that if form.banner is present it actually
displays content (e.g., if form.banner is a URL render an <img> with
src=form.banner and appropriate alt/className, or if form.banner can be a
ReactNode render it directly), or remove the conditional entirely if you do not
intend to show a banner; locate the banner usage in the form-runner.tsx
component and replace the empty div with the appropriate rendering logic for
form.banner.
- Around line 104-187: The code uses any casts and eslint-disable around the
instructions processing and when rendering InstructionResponseCard; replace the
casts by reading the typed instructions from form.instructions (which exists on
FormType) and map them into InstructionWithOrder (same as questions mapping)
inside the useMemo for allItems (function: useMemo that builds questionsWithType
and instructionsWithType), remove the eslint-disable comments and the (form as
any).instructions casting as well as the cast when passing to
InstructionResponseCard, and pass the properly-typed item (InstructionWithOrder)
directly to InstructionResponseCard so TypeScript and ESLint no longer require
escape hatches.
In `@apps/blade/src/app/forms/`[formName]/_components/form-view-edit-client.tsx:
- Around line 40-95: The editResponse mutation currently only checks ownership
and doesn't respect the form's allowEdit flag, so update the editResponse
protectedProcedure (the mutation handler that queries FormResponse) to fetch the
related form.allowEdit when finding the response (e.g., include form: { columns:
{ allowEdit: true } } or join the form) and after fetching, reject the request
if either no response is found or response.form?.allowEdit is false by throwing
a TRPCError (BAD_REQUEST or FORBIDDEN) before performing the update; ensure the
subsequent update logic (update(FormResponse).set(...)) only runs when the
allowEdit check passes and set editedAt when updating.
In `@packages/api/src/routers/forms.ts`:
- Around line 607-626: The query branch for when input.form is present currently
hardcodes allowEdit to false; update the select projection so allowEdit reflects
the form's actual column value (same logic used in the responseId branch) by
selecting the FormsSchemas.allowEdit column (or a boolean expression checking
eq(FormsSchemas.allowEdit, true)) instead of sql<boolean>`false`, keeping the
rest of the query using FormResponse, FormsSchemas, and input.form as-is.
- Around line 506-530: editResponse currently skips checking the form's
allowEdit flag and does not validate responseData against the form's JSON
schema; fetch the existing FormResponse together with its Form (e.g., join
FormResponse with Form to get formId, allowEdit, and jsonSchema), verify
form.allowEdit is true (throw TRPCError with code "FORBIDDEN" or "BAD_REQUEST"
if false), validate input.responseData against the form's jsonSchema using the
same validator used by createResponse (or the shared validate function), and
only then perform the db.update(FormResponse)...set(...) operation; return the
updated record as before and keep the ownership check eq(FormResponse.userId,
userId).
In `@packages/db/src/schemas/knight-hacks.ts`:
- Around line 503-505: The schema change adds allowEdit to the form_response
table; instead of using drizzle-kit push, create a SQL migration file that ALTER
TABLE form_response ADD COLUMN allowEdit boolean NOT NULL DEFAULT false and
include a DOWN migration to DROP COLUMN allowEdit; for editedAt avoid backdating
existing rows — either (A) add editedAt nullable, backfill existing rows SET
editedAt = createdAt for rows that should be considered edited and then ALTER
COLUMN editedAt SET NOT NULL for future enforcement, or (B) add editedAt NOT
NULL only for new inserts by keeping it NULLABLE initially and adding
application-level logic to populate it going forward, then create a follow-up
migration to make it NOT NULL; follow Drizzle migrations format and include
descriptive up/down SQL and transaction safety.
🧹 Nitpick comments (5)
packages/api/src/routers/forms.ts (1)
113-113: Remove debugconsole.logstatement.This logs the entire input object (including form data) to stdout. Remove it or use the structured
logutility if logging is intentional.🧹 Proposed fix
- console.log(input);packages/db/src/schemas/knight-hacks.ts (1)
543-546: Consider a clearer pattern for tracking edits: makeeditedAtnullable.The schema defaults
editedAttonow()on all inserts, which semantically conflates "new response" with "edited response." The update logic correctly setseditedAt: new Date()on actual edits, but the default creates ambiguity: you can't distinguish whether a timestamp is from creation or editing.Recommend making
editedAtnullable (omit the default), then explicitly set it only during updates. This makes the intent clear: null = never edited, otherwise = last edit time.- editedAt: t.timestamp().notNull().defaultNow(), + editedAt: t.timestamp(),Existing responses will have null, matching their actual state (created once, never edited). Future edits will populate it as they do now.
apps/blade/src/app/forms/[formName]/_hooks/useSubmissionSuccess.ts (1)
28-40: Clamp/reset the redirect countdown to avoid stale or negative values.
IfisSubmittedtoggles or delays change, the countdown can retain old values or dip below zero. Reset it on submit and clamp at 0.💡 Suggested tweak
useEffect(() => { - if (!isSubmitted) return; + if (!isSubmitted) { + setShowCheckmark(false); + setShowText(false); + setRedirectCountdown(Math.ceil(redirectDelayMs / 1000)); + return; + } + + setRedirectCountdown(Math.ceil(redirectDelayMs / 1000)); const checkTimer = setTimeout( () => setShowCheckmark(true), checkmarkDelayMs, ); const textTimer = setTimeout(() => setShowText(true), textDelayMs); const countdownInterval = setInterval(() => { - setRedirectCountdown((prev) => prev - 1); + setRedirectCountdown((prev) => Math.max(prev - 1, 0)); }, 1000);apps/blade/src/app/forms/[formName]/_components/utils.ts (1)
15-24: Consider moving validators to a keyed registry for defense-in-depth.The current approach uses
new Functionto evaluate zodValidator strings from the form database. While the risk is lower than typical code execution (validators come from your server, not user input), a registry pattern eliminates the eval entirely and protects against database compromise:Safer pattern (registry lookup)
// validators.ts export const VALIDATOR_REGISTRY = { questionnaire_v1: z.object({ /* ... */ }), survey_v2: z.object({ /* ... */ }), // ... } as const; // utils.ts - updated getValidatorResponse export const getValidatorResponse = ( validatorKey: string, responses: FormResponseUI, form: FormType, ) => { const zodSchema = VALIDATOR_REGISTRY[validatorKey as keyof typeof VALIDATOR_REGISTRY]; if (!zodSchema) { return { success: false, error: new z.ZodError([ { code: "custom", message: "Unknown validator", path: [] }, ]), }; } const payload = normalizeResponses(responses, form); return zodSchema.safeParse(payload); };This avoids
new Functionentirely and makes validators auditable in code. No immediate risk (validators are server-provided), but this is a good refactor for maintainability.apps/blade/src/app/forms/[formName]/_components/form-submitted-success.tsx (1)
26-48: Add live-region semantics for the redirect countdown (and hide the decorative icon).The countdown changes dynamically; a polite live region ensures it’s announced, and hiding the icon avoids screen‑reader noise.
As per coding guidelines, "Accessibility (alt text, ARIA, semantic HTML)".💡 Suggested update
- <CheckCircle2 className="mx-auto h-16 w-16 text-green-500" /> + <CheckCircle2 + className="mx-auto h-16 w-16 text-green-500" + aria-hidden="true" + /> ... - <p className="mt-4 text-sm text-muted-foreground"> + <p + className="mt-4 text-sm text-muted-foreground" + role="status" + aria-live="polite" + aria-atomic="true" + > Redirecting in {redirectCountdown}... </p>
apps/blade/src/app/dashboard/_components/member-dashboard/forms/form-responses.tsx
Outdated
Show resolved
Hide resolved
apps/blade/src/app/forms/[formName]/_components/form-runner.tsx
Outdated
Show resolved
Hide resolved
apps/blade/src/app/forms/[formName]/_components/form-view-edit-client.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/blade/src/app/forms/`[formName]/_hooks/useSubmissionSuccess.ts:
- Around line 24-26: The countdown initial value uses plain division which can
produce fractional seconds; make it consistent with the reset logic by
initializing redirectCountdown using the same rounding approach (e.g., use
Math.ceil on redirectDelayMs / 1000) so redirectCountdown and
setRedirectCountdown behavior matches; update the initialization expression
where redirectCountdown is declared to mirror the Math.ceil used elsewhere
(referencing redirectCountdown, setRedirectCountdown, and redirectDelayMs).
🧹 Nitpick comments (3)
apps/blade/src/app/admin/forms/[slug]/client.tsx (1)
329-331: Consider addressing the missinghandleSaveFormdependency.The linter flags that
handleSaveFormis used inside this effect but isn't in the dependency array. The inline comment explains this was intentional to prevent save-on-every-render, but omitting it creates a stale closure—the effect may call an outdated version ofhandleSaveForm.A safer pattern is to use a ref to track "should save" intent, or wrap the save trigger in a stable callback. Since this pattern pre-dates your change and you're just adding
allowEdit, this is low-priority—but worth addressing to silence the lint warning and avoid subtle bugs.♻️ One possible fix using a ref guard
+ const initialLoadComplete = React.useRef(false); + // auto save trigger when toggle switches are changed useEffect(() => { - if (!isLoading) handleSaveForm(); - }, [duesOnly, allowResubmission, responseRoleIds, isLoading, allowEdit]); // removed handleSaveForm to prevent save-on-every-render + if (!isLoading && initialLoadComplete.current) { + handleSaveForm(); + } + initialLoadComplete.current = true; + }, [duesOnly, allowResubmission, responseRoleIds, isLoading, allowEdit, handleSaveForm]);apps/blade/src/app/forms/[formName]/_components/form-runner.tsx (2)
39-39: Remove unuseduserNameprop.This prop is declared in the component's props interface but never referenced in the component body. Unused props add confusion and increase the component's API surface without benefit.
🧹 Suggested fix
}: { isReview?: boolean; form: FormType; formId: string; - userName?: string; - zodValidator: string;
123-209: Consider wrapping in a semantic<form>element.Using a
<form>element instead of a plain<div>improves accessibility by:
- Providing proper form semantics to screen readers
- Enabling native keyboard submission (Enter key)
- Allowing
type="submit"buttons to work naturallyIf you adopt this, use
event.preventDefault()in the handler to maintain your custom submit logic:💡 Example approach
return ( - <div className="min-h-screen overflow-x-visible bg-primary/5 p-6"> + <form + className="min-h-screen overflow-x-visible bg-primary/5 p-6" + onSubmit={(e) => { + e.preventDefault(); + if (canSubmit) handleSubmit(); + }} + > <div className="mx-auto max-w-3xl space-y-4"> {/* ... content ... */} - <Button onClick={handleSubmit} disabled={!canSubmit} size="lg"> + <Button type="submit" disabled={!canSubmit} size="lg">
Why
allow users to edit their form response if allowed by that form
What
Test Plan
tested with all input types and with multiple submissions
Summary by CodeRabbit
New Features
Improvements