[CORE-181] Form filling front-end API integration#16
Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates the form detail page with the Core System SDK to load form metadata/sections, auto-save answers, and submit responses, plus updates local dev proxy + dependency lockfiles to support the integration.
Changes:
- Replaced the static form UI with SDK-driven loading of form/sections/questions, answer autosave, and submission.
- Updated Vite dev proxy to point at the mock Core System API.
- Updated dependencies/lockfile (Core System SDK snapshot, TypeScript/ESLint ecosystem bumps).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| vite.config.ts | Switches /api proxy target to mock endpoint and changes TLS verification behavior. |
| src/features/form/components/FormDetailPage.tsx | Implements SDK-backed form rendering, autosave, preview, and submit flow. |
| package.json | Pins Core System SDK snapshot and bumps tooling versions/package manager metadata. |
| pnpm-lock.yaml | Reflects resolved dependency updates required by the SDK integration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resolution: {integrity: sha512-iZrhL1fSklfCCVn68IYHaAoKfcby3RakUTn2tRPyHBkhr2tkYqeQbjJWf+NizIYBzKBn2IarDJXmTdXd6CuEfw==} | ||
| '@icons-pack/react-simple-icons@13.11.2': | ||
| resolution: {integrity: sha512-IiYWH7Dlp4HvTxsL9Mh/1tjKKhLikKzL+578H1ngQkoREhfXaS9yNl1SXkjNGEAYqNH2w5/tBQc0uXLWAkcSFA==} | ||
| engines: {node: '>=24', pnpm: '>=10'} |
There was a problem hiding this comment.
@icons-pack/react-simple-icons@13.11.2 declares engines: { node: ">=24" } in the lockfile, but this repo’s CI uses Node 22 and the Docker build stage uses Node 23. This mismatch is likely to break installs/builds; pin @icons-pack/react-simple-icons to a Node-22/23 compatible version or upgrade the project’s Node version everywhere.
| engines: {node: '>=24', pnpm: '>=10'} | |
| engines: {node: '>=22', pnpm: '>=10'} |
| .map(([questionId, value]) => { | ||
| const questionType = questionTypeMap[questionId]; | ||
| const valueArray = value.includes(",") ? value.split(",") : [value]; | ||
|
|
||
| return { | ||
| questionId, | ||
| questionType: questionType as ResponsesAnswerJSON["questionType"], | ||
| value: valueArray | ||
| }; |
There was a problem hiding this comment.
saveAnswers() splits any answer containing a comma into multiple values (value.includes(",") ? value.split(",") : ...). This will incorrectly break normal text inputs (SHORT_TEXT/LONG_TEXT) when users type commas, producing the wrong payload.
|
|
||
| const renderQuestion = (question: FormsQuestionResponse) => { | ||
| const value = answers[question.id] || ""; | ||
| console.log("Rendering question:", question, "with value:", value); |
There was a problem hiding this comment.
console.log inside renderQuestion will run on every render for every question and will spam the console in production builds unless stripped. Please remove the debug log (or guard it behind an explicit dev flag).
| console.log("Rendering question:", question, "with value:", value); |
| label={choice.name} | ||
| checked={value.includes(choice.id)} | ||
| onCheckedChange={checked => { | ||
| const currentValues = value ? value.split(",") : []; | ||
| const newValues = checked ? [...currentValues, choice.id] : currentValues.filter(v => v !== choice.id); | ||
| updateAnswer(question.id, newValues.join(",")); | ||
| }} |
There was a problem hiding this comment.
The MULTIPLE_CHOICE checked state uses value.includes(choice.id), which can produce false positives when one choice id is a substring of another (e.g., "1" matches "10"). It also allows duplicates when adding (no de-dupe). Consider storing multi-select answers as an array/Set and checking membership by exact match.
| {question.choices?.map(choice => ( | ||
| <Checkbox | ||
| key={choice.id} | ||
| id={`${question.id}-${choice.id}`} | ||
| label={choice.name} | ||
| checked={value.includes(choice.id)} | ||
| onCheckedChange={checked => { | ||
| const currentValues = value ? value.split(",") : []; | ||
| const newValues = checked ? [...currentValues, choice.id] : currentValues.filter(v => v !== choice.id); | ||
| updateAnswer(question.id, newValues.join(",")); | ||
| }} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ); | ||
|
|
||
| case "DETAILED_MULTIPLE_CHOICE": | ||
| return ( | ||
| <div key={question.id} style={{ display: "flex", flexDirection: "column", gap: "0.5rem" }}> | ||
| {question.choices?.map(choice => ( | ||
| <DetailedCheckbox | ||
| key={choice.id} | ||
| id={`${question.id}-${choice.id}`} | ||
| title={choice.name} | ||
| description={choice.description || ""} | ||
| checked={value.includes(choice.id)} | ||
| onCheckedChange={checked => { | ||
| const currentValues = value ? value.split(",") : []; | ||
| const newValues = checked ? [...currentValues, choice.id] : currentValues.filter(v => v !== choice.id); | ||
| updateAnswer(question.id, newValues.join(",")); | ||
| }} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
The DETAILED_MULTIPLE_CHOICE checked state uses value.includes(choice.id), which has the same substring false-positive issue as above and can also create duplicate ids when toggling. Using an array/Set of selected choice ids avoids both problems.
| {question.choices?.map(choice => ( | |
| <Checkbox | |
| key={choice.id} | |
| id={`${question.id}-${choice.id}`} | |
| label={choice.name} | |
| checked={value.includes(choice.id)} | |
| onCheckedChange={checked => { | |
| const currentValues = value ? value.split(",") : []; | |
| const newValues = checked ? [...currentValues, choice.id] : currentValues.filter(v => v !== choice.id); | |
| updateAnswer(question.id, newValues.join(",")); | |
| }} | |
| /> | |
| ))} | |
| </div> | |
| ); | |
| case "DETAILED_MULTIPLE_CHOICE": | |
| return ( | |
| <div key={question.id} style={{ display: "flex", flexDirection: "column", gap: "0.5rem" }}> | |
| {question.choices?.map(choice => ( | |
| <DetailedCheckbox | |
| key={choice.id} | |
| id={`${question.id}-${choice.id}`} | |
| title={choice.name} | |
| description={choice.description || ""} | |
| checked={value.includes(choice.id)} | |
| onCheckedChange={checked => { | |
| const currentValues = value ? value.split(",") : []; | |
| const newValues = checked ? [...currentValues, choice.id] : currentValues.filter(v => v !== choice.id); | |
| updateAnswer(question.id, newValues.join(",")); | |
| }} | |
| /> | |
| ))} | |
| {question.choices?.map(choice => { | |
| const selectedIds = value ? value.split(",").filter(Boolean) : []; | |
| const isChecked = selectedIds.includes(choice.id); | |
| return ( | |
| <Checkbox | |
| key={choice.id} | |
| id={`${question.id}-${choice.id}`} | |
| label={choice.name} | |
| checked={isChecked} | |
| onCheckedChange={checked => { | |
| const currentValues = value ? value.split(",").filter(Boolean) : []; | |
| let newValues: string[]; | |
| if (checked) { | |
| newValues = currentValues.includes(choice.id) | |
| ? currentValues | |
| : [...currentValues, choice.id]; | |
| } else { | |
| newValues = currentValues.filter(v => v !== choice.id); | |
| } | |
| updateAnswer(question.id, newValues.join(",")); | |
| }} | |
| /> | |
| ); | |
| })} | |
| </div> | |
| ); | |
| case "DETAILED_MULTIPLE_CHOICE": | |
| return ( | |
| <div key={question.id} style={{ display: "flex", flexDirection: "column", gap: "0.5rem" }}> | |
| {question.choices?.map(choice => { | |
| const selectedIds = value ? value.split(",").filter(Boolean) : []; | |
| const isChecked = selectedIds.includes(choice.id); | |
| return ( | |
| <DetailedCheckbox | |
| key={choice.id} | |
| id={`${question.id}-${choice.id}`} | |
| title={choice.name} | |
| description={choice.description || ""} | |
| checked={isChecked} | |
| onCheckedChange={checked => { | |
| const currentValues = value ? value.split(",").filter(Boolean) : []; | |
| let newValues: string[]; | |
| if (checked) { | |
| newValues = currentValues.includes(choice.id) | |
| ? currentValues | |
| : [...currentValues, choice.id]; | |
| } else { | |
| newValues = currentValues.filter(v => v !== choice.id); | |
| } | |
| updateAnswer(question.id, newValues.join(",")); | |
| }} | |
| /> | |
| ); | |
| })} |
| progress: "DRAFT" as const, | ||
| questions: [] | ||
| }); | ||
| setSections(loadedSections); |
There was a problem hiding this comment.
If formsListSections returns a non-200 status, the code silently leaves sections empty and still completes loading without setting an error. This can put the page into a broken state (navigation works but there are no sections/questions). Consider treating non-200 as an error (setError) or providing a fallback UI.
| setSections(loadedSections); | |
| setSections(loadedSections); | |
| } else { | |
| throw new Error(`載入表單區段失敗: HTTP ${sectionsResponse.status}`); |
| cookieDomainRewrite: { | ||
| "dev.core-system.sdc.nycu.club": "localhost" | ||
| } | ||
| secure: false |
There was a problem hiding this comment.
secure: false disables TLS certificate verification for the dev proxy. Even in dev, it’s safer to keep verification on and only disable it conditionally (e.g., behind an env flag) when you know you’re hitting a self-signed endpoint.
| secure: false | |
| secure: process.env.VITE_DEV_PROXY_INSECURE === "true" |
| {isLastStep ? ( | ||
| <Button type="submit" disabled={!formData.agree}> | ||
| 送出 | ||
| <Button type="submit" disabled={responseProgress === "DRAFT"}> |
There was a problem hiding this comment.
responsesSubmitFormResponse is never reachable because the submit button is disabled when responseProgress === "DRAFT" (which is the normal pre-submit state). This prevents users from ever submitting a draft response; the disabled condition should instead block only already-submitted responses (or be based on validation state).
| <Button type="submit" disabled={responseProgress === "DRAFT"}> | |
| <Button type="submit" disabled={responseProgress === "SUBMITTED"}> |
| const questionTypeMap: Record<string, string> = {}; | ||
| sections.forEach(section => { | ||
| section.questions?.forEach(question => { | ||
| questionTypeMap[question.id] = question.type; | ||
| }); | ||
| }); | ||
|
|
||
| const answersArray = Object.entries(answers) | ||
| .filter(([, value]) => value !== "") | ||
| .map(([questionId, value]) => { | ||
| const questionType = questionTypeMap[questionId]; | ||
| const valueArray = value.includes(",") ? value.split(",") : [value]; | ||
|
|
||
| return { | ||
| questionId, | ||
| questionType: questionType as ResponsesAnswerJSON["questionType"], | ||
| value: valueArray |
There was a problem hiding this comment.
questionTypeMap[questionId] can be undefined (e.g., before sections/questions are loaded), but it’s cast to ResponsesAnswerJSON["questionType"] and sent anyway. This can lead to invalid update requests during the initial load autosave; filter out unknown questionIds or only save once sections/questions are available.
| <h2 className={styles.sectionHeader}>{sections[currentStep].title}</h2> | ||
| )} | ||
| <h1 className={styles.title}>{form.title}</h1> | ||
| {currentStep === 0 ? <p className={styles.description}>{form.description}</p> : <h2 className={styles.sectionHeader}>{sections[currentStep].title}</h2>} |
There was a problem hiding this comment.
sections[currentStep].title is read without a guard when currentStep !== 0. If formsListSections fails (or returns non-200) sections can remain empty, and clicking "下一頁" will make this access throw. Either block navigation until sections are loaded or render defensively when sections[currentStep] is undefined.
| {currentStep === 0 ? <p className={styles.description}>{form.description}</p> : <h2 className={styles.sectionHeader}>{sections[currentStep].title}</h2>} | |
| {currentStep === 0 ? ( | |
| <p className={styles.description}>{form.description}</p> | |
| ) : sections[currentStep] ? ( | |
| <h2 className={styles.sectionHeader}>{sections[currentStep].title}</h2> | |
| ) : null} |
No description provided.