Conversation
…gement improvements
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive form flow editor feature with visual workflow management capabilities. The implementation includes a new Popover shared component and extensive updates to form editing functionality.
Changes:
- Adds a visual form flow editor with node-based workflow representation including START, END, SECTION, and CONDITION node types
- Introduces a new Popover shared component using Radix UI primitives for contextual menus
- Adds development proxy configuration for API requests and wraps the app with ToastProvider
Reviewed changes
Copilot reviewed 24 out of 35 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Adds proxy configuration for API requests to development server |
| package.json / pnpm-lock.yaml | Adds @radix-ui/react-popover dependency |
| src/App.tsx | Wraps application with ToastProvider for global toast notifications |
| src/shared/components/index.ts | Exports new Popover component |
| src/shared/components/Popover/* | New Popover component implementation with animations |
| src/shared/components/Select/* | Adds "text" variant for inline select styling |
| src/features/form/components/AdminFormDetailPages/EditPage.tsx | Implements complete flow editor with node management logic |
| src/features/form/components/AdminFormDetailPages/components/FormEditor/* | FlowRenderer and Arrow components for visual workflow |
| src/features/form/components/AdminFormDetailPages/components/SectionEditor/* | Various question editor components moved to subdirectory |
| src/features/form/components/AdminFormDetailPages/types/workflow.ts | Type definitions for workflow nodes |
| src/features/dashboard/components/AdminSettingsPage.tsx | Updates orgSlug to uppercase and adds debug logging |
| src/features/dashboard/components/ComponentsDemo.tsx | Adds Popover demo |
| src/features/form/components/AdminFormsPage.tsx | Adds commented import statement |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div>Section Edit Page Content</div> | ||
| <button onClick={() => handleEditForm("test")}>Edit Form</button> | ||
| <h2>表單結構</h2> | ||
| <blockquote className={styles.description}>點擊區塊已新增或編輯條件與問題</blockquote> |
There was a problem hiding this comment.
There is a typo in the Chinese text. The word "已" should be "以" (meaning "to" or "in order to"). The current text reads "點擊區塊已新增或編輯條件與問題" which should be "點擊區塊以新增或編輯條件與問題" (Click on blocks to add or edit conditions and questions).
| <blockquote className={styles.description}>點擊區塊已新增或編輯條件與問題</blockquote> | |
| <blockquote className={styles.description}>點擊區塊以新增或編輯條件與問題</blockquote> |
| import type { NodeItem } from "./types/workflow"; | ||
|
|
||
| const postProcessNodes = (nodes: NodeItem[]): NodeItem[] => { | ||
| console.log("Post-processing nodes:", nodes); |
There was a problem hiding this comment.
The console.log statement should be removed before merging to production. Debug logging statements should not be committed to the codebase.
| console.log("Post-processing nodes:", nodes); |
| server: { | ||
| proxy: { | ||
| "/api": { | ||
| target: "https://dev.core-system.sdc.nycu.club", |
There was a problem hiding this comment.
The proxy configuration includes a hardcoded development URL "https://dev.core-system.sdc.nycu.club". This should be moved to an environment variable to allow different environments (dev, staging, production) to be configured without code changes. Consider using an environment variable like "VITE_API_BASE_URL" instead.
| content={ | ||
| <div className={styles.popoverContent}> | ||
| {node.type === "SECTION" && <Button variant="secondary">編輯</Button>} | ||
| {node.type != "END" && node.type != "CONDITION" && ( |
There was a problem hiding this comment.
Inconsistent comparison operators are used: line 104 uses != while line 109 uses !==. For consistency and best practices in JavaScript/TypeScript, always use strict equality operators (=== and !==) instead of loose equality operators (== and !=).
| {node.type != "END" && node.type != "CONDITION" && ( | |
| {node.type !== "END" && node.type !== "CONDITION" && ( |
| {node.type === "SECTION" && <Button variant="secondary">編輯</Button>} | ||
| {node.type != "END" && node.type != "CONDITION" && ( | ||
| <Button variant="secondary" onClick={onAddSection}> | ||
| 新增區域 | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
The "編輯" (Edit) button on line 103 does not have an onClick handler. This appears to be a placeholder button that does nothing when clicked, which could confuse users. Either add the appropriate handler or remove this button until the functionality is implemented.
| {node.type === "SECTION" && <Button variant="secondary">編輯</Button>} | |
| {node.type != "END" && node.type != "CONDITION" && ( | |
| <Button variant="secondary" onClick={onAddSection}> | |
| 新增區域 | |
| </Button> | |
| )} | |
| {node.type != "END" && node.type != "CONDITION" && ( | |
| <Button variant="secondary" onClick={onAddSection}> | |
| 新增區域 | |
| </Button> | |
| )} |
| if (nodeToMerge && nodeToDelete.type === "CONDITION" && nodeToDelete.nextTrue !== nodeToDelete.mergeId && nodeToDelete.nextFalse !== nodeToDelete.mergeId) { | ||
| setToastOpen(true); | ||
| setToastTitle("無法刪除條件節點"); | ||
| setToastDescription("請確保只有一個可辨識的分支路後再嘗試刪除。"); |
There was a problem hiding this comment.
There is a typo in the error message. "可辨識的分支路" should be "可辨識的分支路徑" (with the complete word "路徑" meaning "path"). The current text is incomplete.
| setToastDescription("請確保只有一個可辨識的分支路後再嘗試刪除。"); | |
| setToastDescription("請確保只有一個可辨識的分支路徑後再嘗試刪除。"); |
| export const AdminSettingsPage = () => { | ||
| // NOTE: current routes are hard-coded to /orgs/sdc/* | ||
| const orgSlug = "sdc"; | ||
| const orgSlug = "SDC"; |
There was a problem hiding this comment.
The orgSlug value has been changed from lowercase "sdc" to uppercase "SDC". This may cause issues if the API is case-sensitive, as all the hardcoded routes throughout the codebase use lowercase "sdc" (e.g., "/orgs/sdc/forms"). If this change is intentional, the associated comment on line 43 should be updated to reflect that routes use lowercase but the API expects uppercase. Otherwise, this should be reverted to "sdc" to maintain consistency.
| const orgSlug = "SDC"; | |
| const orgSlug = "sdc"; |
| ); | ||
| }, [nodes]); | ||
|
|
||
| const renderedIds = new Set<string>(); |
There was a problem hiding this comment.
The renderedIds Set is declared inside the component but outside the renderNode function. This means it persists across renders but doesn't get reset when the component re-renders. This could cause nodes to not be rendered properly after a state update. The Set should be initialized inside a useMemo hook or the renderNode function should be restructured to handle this correctly.
| return; | ||
| } | ||
|
|
||
| if (!nodeToDelete) return; |
There was a problem hiding this comment.
There is redundant code - the variable nodeToDelete is checked twice for null/undefined. The check on line 260 is unnecessary since the same check already exists on line 252 with an early return. This second check can be removed.
| if (!nodeToDelete) return; |
| return { ...node, nextFalse: nodeToDelete.next || (nodeToDelete.mergeId === nodeToDelete.nextTrue ? nodeToDelete.nextFalse : nodeToDelete.nextFalse) }; | ||
| } | ||
| if (node.nextTrue === id) { | ||
| return { ...node, nextTrue: nodeToDelete.next || (nodeToDelete.mergeId === nodeToDelete.nextTrue ? nodeToDelete.nextFalse : nodeToDelete.nextFalse) }; |
There was a problem hiding this comment.
The condition logic on lines 268, 270, and 271 is complex and appears to have duplicated ternary expressions. The expression (nodeToDelete.mergeId === nodeToDelete.nextTrue ? nodeToDelete.nextFalse : nodeToDelete.nextFalse) evaluates to nodeToDelete.nextFalse in both cases, making the ternary operator redundant. This suggests either a logic error or unnecessary complexity that should be simplified to just nodeToDelete.next || nodeToDelete.nextFalse.
| return { ...node, nextFalse: nodeToDelete.next || (nodeToDelete.mergeId === nodeToDelete.nextTrue ? nodeToDelete.nextFalse : nodeToDelete.nextFalse) }; | |
| } | |
| if (node.nextTrue === id) { | |
| return { ...node, nextTrue: nodeToDelete.next || (nodeToDelete.mergeId === nodeToDelete.nextTrue ? nodeToDelete.nextFalse : nodeToDelete.nextFalse) }; | |
| return { ...node, nextFalse: nodeToDelete.next || nodeToDelete.nextFalse }; | |
| } | |
| if (node.nextTrue === id) { | |
| return { ...node, nextTrue: nodeToDelete.next || nodeToDelete.nextFalse }; |
…d improve FlowNode interaction
…in AdminFormEditPage
Type of changes
Purpose
Additional Information