feat: Add Advanced Form #149
Conversation
WalkthroughAdds an "Advance" form feature: new locale entries, menu item, route, API client and mock endpoint, plus TinyVue components (advance parent, basic-info form, editable process-grid and renderers) implementing data fetch, validation, grid editing, reset and submit flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as TinyVue Advance UI
participant Parent as Advance Parent Component
participant Basic as BasicInfo Component
participant Grid as ProcessGrid Component
participant API as getAdvanceData()
participant Mock as Mock Server (/api/advance/getdata)
UI->>Parent: mount
Parent->>API: call getAdvanceData()
API->>Mock: HTTP GET /api/advance/getdata
Mock-->>API: return init data (positions, hr, teacher, status, department)
API-->>Parent: resolved data
Parent->>Basic: populate options (positions/hr/teacher)
Parent->>Grid: populate options (status/department)
User->>Basic: fill fields
User->>Grid: add/edit process rows
Parent->>Basic: Basic.validForm()
Parent->>Grid: Grid.saveRow() / validations
Parent->>Parent: on success show success modal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
template/nestJs/src/menu/menu.service.ts (1)
114-142: Consider refactoring hardcoded array indices.The
handleMenuParentIdmethod uses hardcoded array indices to establish parent-child menu relationships. While the new line 137 follows the established pattern, this approach is brittle and makes the code difficult to maintain when menu items are added, removed, or reordered.Consider refactoring to use a more maintainable approach, such as:
- Using menu names or unique identifiers instead of array indices
- Building a lookup map by menu name/id
- Or defining parent relationships directly in the menu data structure
Example approach:
async handleMenuParentId(menuId: number[]) { const menu = await this.menu.find(); const menuMap = new Map(menu.map(m => [m.name, m])); // Define relationships by name instead of index const relationships = { 'Home': 'Board', 'Work': 'Board', 'Base': 'Form', 'Step': 'Form', 'Advance': 'Form', // ... other relationships }; for (const [childName, parentName] of Object.entries(relationships)) { const child = menuMap.get(childName); const parent = menuMap.get(parentName); if (child && parent) { child.parentId = parent.id; } } for (const item of menu) { await this.menu.update(item.id, { parentId: item.parentId }); } }template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
125-157: Consider fetching options from API instead of hardcoding.The dropdown options (
departmentOptions,statusOptions,runningStatusOptions) are currently hardcoded. For production use, these should typically be fetched from the backend to ensure consistency and maintainability.Consider fetching options in
onMounted:import { onMounted } from 'vue'; const departmentOptions = ref([]); const statusOptions = ref([]); const runningStatusOptions = ref([]); onMounted(async () => { try { // Fetch options from API const { data } = await getProcessGridOptions(); departmentOptions.value = data.departments; statusOptions.value = data.statuses; runningStatusOptions.value = data.runningStatuses; } catch (error) { console.error('Failed to load options:', error); } });template/tinyvue/src/views/form/advance/basic-info/index.vue (1)
141-158: Reconsider requiring all fields.All nine form fields use
commonRule, making them all required. Consider whether fields likeremarkshould truly be mandatory, as remarks are typically optional.Consider making optional fields less strict:
const rules = ref({ projectName: [...commonRule], position: [...commonRule], hr: [...commonRule], teacher: [...commonRule], startTime: [...commonRule], endTime: [...commonRule], phone: [...commonRule], address: [...commonRule], - remark: [...commonRule], + remark: [], // Optional field });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
template/nestJs/locales.json(1 hunks)template/nestJs/src/menu/init/menuData.ts(1 hunks)template/nestJs/src/menu/menu.service.ts(1 hunks)template/tinyvue/src/locales.json(1 hunks)template/tinyvue/src/router/routes/modules/form.ts(1 hunks)template/tinyvue/src/views/form/advance/basic-info/index.vue(1 hunks)template/tinyvue/src/views/form/advance/index.vue(1 hunks)template/tinyvue/src/views/form/advance/process-grid/index.vue(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
template/tinyvue/src/router/routes/modules/form.ts (1)
template/tinyvue/src/store/modules/user/types.ts (1)
RoleType(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-test
🔇 Additional comments (6)
template/tinyvue/src/router/routes/modules/form.ts (1)
40-51: LGTM! Route configuration follows established patterns.The new Advance route is correctly configured with:
- Consistent path and component structure
- Appropriate lazy loading
- Correct metadata (locale key, authentication, role restriction)
- Alignment with existing Base and Step form routes
template/nestJs/src/menu/init/menuData.ts (1)
292-301: Menu item configuration looks correct.The new "Advance" menu item follows the established pattern for form child menu items:
- Correct order value (3, following Base:1 and Step:2)
parentId: nullis expected (will be set byhandleMenuParentId)- Component and path values align with the Vue router configuration
- Locale key matches the translations added in locale files
Note: The
iconis set to an empty string, which is consistent with other form child items (Base, Step).template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
125-136: Review the initial sample data.The grid is initialized with a hardcoded sample row containing what appears to be test data (name: '黄芊义', etc.). Consider whether this should:
- Start empty for production
- Load real data from an API
- Remain as a placeholder for demonstration purposes
If this is production code, the initial data should likely be removed or loaded from the backend.
template/tinyvue/src/locales.json (2)
870-895: Locale key organization looks good.The new locale keys follow a clear hierarchical structure:
menu.form.advancefor navigationadvanceForm.form.basicInfo.*for basic info form fieldsadvanceForm.form.process.*for process grid labels and actionsadvanceForm.form.validError.*for validation messagesadvanceForm.form.delete.titleandadvanceForm.form.nodatafor UI feedbackThis organization makes the keys easy to maintain and understand.
870-895: Verify corresponding enUS translations exist for all advanceForm locale keys.The Chinese translations for the advance form are added in the zhCN section (lines 870-895), but ensure all corresponding keys exist in the enUS section. Both language sections should include all new locale keys for consistency.
template/nestJs/locales.json (1)
930-930: Verify that the corresponding enUS translation exists.The Chinese translation for
menu.form.advanceis added at line 930, but a corresponding entry in theenUSsection of this file should also be included to maintain consistency.
| const validForm = () => { | ||
| let baseValidate = false; | ||
| formRef.value.validate((valid) => { | ||
| if (!valid) { | ||
| Modal.message({ | ||
| message: t('baseForm.form.submit.error'), | ||
| status: 'error', | ||
| }); | ||
| } | ||
| baseValidate = valid; | ||
| }); | ||
|
|
||
| return baseValidate; | ||
| }; |
There was a problem hiding this comment.
validForm has confusing synchronous/asynchronous behavior.
The validForm method returns a boolean immediately, but form validation via formRef.value.validate() is callback-based. The return value may not reflect the actual validation result if validation is asynchronous.
Consider refactoring to use an async/Promise-based approach:
const validForm = () => {
return new Promise<boolean>((resolve) => {
formRef.value.validate((valid) => {
if (!valid) {
Modal.message({
message: t('baseForm.form.submit.error'),
status: 'error',
});
}
resolve(valid);
});
});
};Then update the parent component's handleSubmit to use await:
const handleSubmit = async () => {
const baseValid = await basicInfoRef.value.validForm();
// ...
};🤖 Prompt for AI Agents
In template/tinyvue/src/views/form/advance/basic-info/index.vue around lines 164
to 177, validForm currently returns a boolean synchronously while
formRef.value.validate() is callback-based and may be asynchronous; change
validForm to return a Promise<boolean> that wraps
formRef.value.validate((valid)=>{...; resolve(valid);}), keep the Modal.message
call when valid is false, and update all callers (e.g., parent handleSubmit) to
await basicInfoRef.value.validForm() (make handleSubmit async) so the true
validation result is used.
| const fetchData = async () => { | ||
| loadingState.value = Loading.service({ | ||
| text: 'loading...', | ||
| target: document.getElementById('project-form'), | ||
| background: 'rgba(0, 0, 0, 0.7)', | ||
| }); | ||
| try { | ||
| const { data } = await getBaseData(); | ||
| projectData.positionOptions = data.position; | ||
| projectData.hrOptions = data.HR; | ||
| projectData.teacherOptions = data.mentor.map((item) => ({ | ||
| label: item, | ||
| value: item, | ||
| })); | ||
| } finally { | ||
| loadingState.value.close(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for API failures.
The fetchData method doesn't handle API errors. If getBaseData() fails, the loading indicator will close, but users won't know why the dropdowns are empty and have no way to retry.
Apply this diff to add error handling:
const fetchData = async () => {
loadingState.value = Loading.service({
text: 'loading...',
target: document.getElementById('project-form'),
background: 'rgba(0, 0, 0, 0.7)',
});
try {
const { data } = await getBaseData();
projectData.positionOptions = data.position;
projectData.hrOptions = data.HR;
projectData.teacherOptions = data.mentor.map((item) => ({
label: item,
value: item,
}));
+ } catch (error) {
+ console.error('Failed to load form data:', error);
+ Modal.message({
+ message: t('baseForm.form.get.error'),
+ status: 'error',
+ });
} finally {
loadingState.value.close();
}
};🤖 Prompt for AI Agents
In template/tinyvue/src/views/form/advance/basic-info/index.vue around lines 184
to 201, fetchData currently awaits getBaseData() inside try/finally but does not
handle API errors; wrap the await in a try/catch/finally, on catch call the UI
notification (e.g., Message.error or this.$message.error) with a clear
user-facing message and the error, optionally set projectData.* options to empty
arrays or previous values, and expose a retry path (e.g., enable a retry button
or return a boolean) so callers can retry; ensure loadingState.value.close()
remains in finally so the spinner always hides.
| const handleSubmit = () => { | ||
| const baseValid = basicInfoRef.value.validForm(); | ||
| if (baseValid) { | ||
| Modal.message({ | ||
| message: t('baseForm.form.submit.success'), | ||
| status: 'success', | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add validation for ProcessGrid and implement actual form submission.
The handleSubmit method has several concerns:
-
Missing ProcessGrid validation: Only
basicInfoRefis validated, butprocessGridis not checked. If the grid has required data or validation rules, they should be enforced here. -
No actual form submission: The method shows a success message without submitting data to any API endpoint. This appears to be a placeholder implementation.
-
Missing null safety: No check if
basicInfoRef.valueexists before callingvalidForm().
Apply this diff to address these concerns:
const handleSubmit = () => {
+ if (!basicInfoRef.value || !processGrid.value) {
+ Modal.message({
+ message: t('baseForm.form.submit.error'),
+ status: 'error',
+ });
+ return;
+ }
+
const baseValid = basicInfoRef.value.validForm();
- if (baseValid) {
+ const gridValid = processGrid.value.getTableData().length > 0; // Or add validation method to ProcessGrid
+
+ if (baseValid && gridValid) {
+ // TODO: Implement actual form submission
+ // const formData = {
+ // basicInfo: basicInfoRef.value.getFormData(),
+ // processData: processGrid.value.getTableData()
+ // };
+ // await submitAdvanceForm(formData);
+
Modal.message({
message: t('baseForm.form.submit.success'),
status: 'success',
});
}
};📝 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.
| const handleSubmit = () => { | |
| const baseValid = basicInfoRef.value.validForm(); | |
| if (baseValid) { | |
| Modal.message({ | |
| message: t('baseForm.form.submit.success'), | |
| status: 'success', | |
| }); | |
| } | |
| }; | |
| const handleSubmit = () => { | |
| if (!basicInfoRef.value || !processGrid.value) { | |
| Modal.message({ | |
| message: t('baseForm.form.submit.error'), | |
| status: 'error', | |
| }); | |
| return; | |
| } | |
| const baseValid = basicInfoRef.value.validForm(); | |
| const gridValid = processGrid.value.getTableData().length > 0; // Or add validation method to ProcessGrid | |
| if (baseValid && gridValid) { | |
| // TODO: Implement actual form submission | |
| // const formData = { | |
| // basicInfo: basicInfoRef.value.getFormData(), | |
| // processData: processGrid.value.getTableData() | |
| // }; | |
| // await submitAdvanceForm(formData); | |
| Modal.message({ | |
| message: t('baseForm.form.submit.success'), | |
| status: 'success', | |
| }); | |
| } | |
| }; |
| const saveRow = (row) => { | ||
| gridRef.value.clearActived(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add row validation before saving.
The saveRow method clears the active state without validating the row data. This allows users to save incomplete or invalid rows.
Apply this diff to add validation:
const saveRow = (row) => {
+ // Validate required fields
+ if (!row.name || !row.number || !row.department) {
+ Modal.message({
+ message: t('advanceForm.form.validError.null'),
+ status: 'error',
+ });
+ return;
+ }
+
+ // TODO: Add API call to persist the row data
+ // try {
+ // await saveProcessRow(row);
+ // } catch (error) {
+ // Modal.message({
+ // message: t('advanceForm.form.save.error'),
+ // status: 'error',
+ // });
+ // return;
+ // }
+
gridRef.value.clearActived();
+ Modal.message({
+ message: t('advanceForm.form.process.save') + ' ' + t('baseForm.form.submit.success'),
+ status: 'success',
+ });
};📝 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.
| const saveRow = (row) => { | |
| gridRef.value.clearActived(); | |
| }; | |
| const saveRow = (row) => { | |
| // Validate required fields | |
| if (!row.name || !row.number || !row.department) { | |
| Modal.message({ | |
| message: t('advanceForm.form.validError.null'), | |
| status: 'error', | |
| }); | |
| return; | |
| } | |
| // TODO: Add API call to persist the row data | |
| // try { | |
| // await saveProcessRow(row); | |
| // } catch (error) { | |
| // Modal.message({ | |
| // message: t('advanceForm.form.save.error'), | |
| // status: 'error', | |
| // }); | |
| // return; | |
| // } | |
| gridRef.value.clearActived(); | |
| Modal.message({ | |
| message: t('advanceForm.form.process.save') + ' ' + t('baseForm.form.submit.success'), | |
| status: 'success', | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In template/tinyvue/src/views/form/advance/process-grid/index.vue around lines
173-175, the saveRow function currently clears the active row without validating
its data; update saveRow to run validation on the provided row (synchronously or
await an existing grid/form validation method), and only call
gridRef.value.clearActived() when validation passes; if validation fails,
prevent clearing, surface validation errors to the user (e.g., mark
fields/errors or show a message), and handle async validators with try/catch so
saveRow waits for validation to finish before proceeding.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
200-202: Add row validation before clearing active state insaveRow
saveRowimmediately clears the active row without checking that required fields (name/number/department, etc.) are filled, so users can “save” incomplete rows. It would be safer to validate the row first (either manually checking required fields or using any grid-level validation API you have) and only callgridRef.value.clearActived()when validation passes, surfacing an error message otherwise.
🧹 Nitpick comments (4)
template/tinyvue/src/views/form/advance/process-grid/status-render.vue (1)
1-25: Status renderer is clear; consider a safe fallback for unknown valuesThe mapping of
running/finished/delayedto tag types is straightforward and matches the mock data. If there’s any chance of other status values appearing, you may want to defaulttype(e.g., empty string or a neutral type) whentagType[props.value]is undefined so TinyTag never receives an unexpected value.template/tinyvue/src/views/form/advance/process-grid/select-render.vue (1)
1-17: Guard against missing options and fix thetranformValuetypoCurrent logic assumes
props.optionsis always defined; if this prop is ever omitted or null,props.options.find(...)will throw. Consider defaulting to an empty array in the computed (e.g., using a localconst options = props.options || []) before searching. Also, renamingtranformValue→transformValuewill make the intent clearer and avoid carrying a typo into future usages.template/tinyvue/src/views/form/advance/process-grid/index.vue (2)
154-162: Correct theoptionsprop definition to use a constructor type and factory default
options.typeis currently an object literal withstatus/departmentfields, anddefaultis a bare object. This doesn’t follow Vue’s expected prop option shape and can lead to confusing runtime checks and shared mutable defaults. Consider something like:- defineProps({ - options: { - type: { - status: [], - department: [], - }, - default: {}, - }, - }); + defineProps({ + options: { + type: Object, + default: () => ({ + status: [], + department: [], + }), + }, + });This keeps the runtime contract clear and avoids shared state across component instances.
168-183: Consider formattingcreateTimevalues instead of relying on raw Date output
createTimeis initialized withnew Date()and rendered viadefaultRender, which will display the default stringification of the Date object. If you want a stable, user-friendly format, it would be better to format this field (e.g., when setting the value or indefaultRender), especially sinceuseDateFormatis already imported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
template/nestJs/src/mock/data/forms.ts(2 hunks)template/tinyvue/src/api/form.ts(1 hunks)template/tinyvue/src/views/form/advance/basic-info/index.vue(1 hunks)template/tinyvue/src/views/form/advance/index.vue(1 hunks)template/tinyvue/src/views/form/advance/process-grid/index.vue(1 hunks)template/tinyvue/src/views/form/advance/process-grid/select-render.vue(1 hunks)template/tinyvue/src/views/form/advance/process-grid/status-render.vue(1 hunks)template/tinyvue/src/views/form/step/mock.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- template/tinyvue/src/views/form/advance/index.vue
- template/tinyvue/src/views/form/advance/basic-info/index.vue
🔇 Additional comments (6)
template/nestJs/src/mock/data/forms.ts (2)
39-53: Status and department mock options look consistent with frontend usageThe added
statusstrings anddepartmentoption objects are well-suited for select/tag renderers and match the expected simple value/label shape.
84-91: /api/advance/getdata mock endpoint correctly reuses initBaseThe new advance init endpoint reusing
initBasekeeps the data contract aligned with base/step mocks and the newgetAdvanceDataAPI.template/tinyvue/src/api/form.ts (1)
30-33: Advance form API wiring matches existing pattern
submitStepFormkeeps the original payload semantics, andgetAdvanceDatais consistent withgetBaseData/getStepDataand the new/api/advance/getdatamock. No issues from an API-shape standpoint.Also applies to: 41-45
template/tinyvue/src/views/form/step/mock.ts (3)
1-4: Expanded initBase mock supports new process fieldsThe shared
initBasenow includesstatusanddepartment, which lines up with the new advance form grid’s select/tag fields. Keeping this in a single base structure will simplify maintaining option lists across base/step/advance flows.Also applies to: 39-53
66-73: /api/step/getdata now backed by initBase for consistencySwitching the step init endpoint to return
initBasekeeps the step form options in sync with the base/advance mocks. This should make it easier to reuse option sets across forms.
84-91: New /api/advance/getdata mock aligns with frontend getAdvanceDataThe added
init-advancemock endpoint reusesinitBaseand matches the/api/advance/getdataURL consumed bygetAdvanceData, so the TinyVue advance form can run entirely against the local mock data.
|
/cmd docker --check |
🐳 Docker Check Status
Status: ⏳ Building... Preparing environment and running Docker health checks. |
✅ Docker Check Status
Status: SUCCESS! Service is built, running, and responded to health check on |
|
@zhaoxiaofeng876 主要有以下优化点：
|
| }" | ||
| > | ||
| <template #default="data"> | ||
| <selectRender |
There was a problem hiding this comment.
| <selectRender | |
| <select-render |
| import { useDateFormat } from '@vueuse/core'; | ||
| import { ref } from 'vue'; | ||
| import statusRender from './status-render.vue'; | ||
| import selectRender from './select-render.vue'; |
There was a problem hiding this comment.
一般组件名称用烤串命名法或者大驼峰（建议烤串，因为TinyVue组件是烤串，统一风格）
| </tiny-grid> | ||
| </div> | ||
| </template> | ||
| <script setup> |
There was a problem hiding this comment.
建议加上 lang="ts"，获得更好的 TypeScript 类型提示
| <script setup> | |
| <script setup lang="ts"> |
| ); | ||
| }); | ||
| </script> | ||
| <style lang=""></style> |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
template/tinyvue/src/views/form/advance/index.vue (2)
61-64: Reset handlers now safely guard child refsSwitching to optional chaining (
basicInfoRef.value?.resetForm(),processGrid.value?.resetGrid()) addresses the earlier concern about calling methods on potentially undefined refs.
66-74: Submit flow still only validates BasicInfo and stays client‑only
handleSubmitonly checksbasicInfoRef.value.validForm()and then shows a success message; it doesn’t validate/process the grid data or actually submit a combined payload. If the grid is intended to be required for submission, consider exposing a validation/data method fromProcessGrid(e.g., viadefineExpose) and using it here, and later wiring this to a real API call.template/tinyvue/src/views/form/advance/process-grid/index.vue (3)
8-17: Using only the#emptyslot resolves the previous render-empty concernDropping the unused
:render-emptybinding and relying solely on the#emptyslot avoids passing an undefined renderer into TinyGrid and keeps the empty-state implementation straightforward.Also applies to: 142-145
200-233: Consider adding basic row validation and feedback in saveRow
saveRowcurrently just clears the active state:const saveRow = (row) => { gridRef.value.clearActived(); };To prevent saving obviously incomplete rows, you could perform a simple check on required fields (e.g., name/number/department) and show a localized error message when validation fails, only clearing the active state when validation passes.
35-55: Align component naming and script language with existing project conventionsMinor polish items:
- The imported
selectRenderfrom./select-render.vueis used as<selectRender>. For consistency with other TinyVue components and prior review feedback, you may prefer kebab-case in the template, e.g.<select-render ...>.- Adding
lang="ts"to the script block (if the rest of the project uses TypeScript) will improve type hints and catch more issues at build time:-<script setup> +<script setup lang="ts">These are stylistic but help keep the codebase consistent.
Also applies to: 68-75, 148-166
🧹 Nitpick comments (3)
template/tinyvue/src/views/form/step/mock.ts (1)
134-141: Consider decoupling step and advance mock payloadsReusing
initStepfor/api/advance/getdataworks now, but it couples the step and advance forms’ payload shapes. If the two forms diverge later (extra fields, validations), consider introducing a dedicatedinitAdvancemock to avoid cross-impact.template/tinyvue/src/views/form/advance/index.vue (1)
76-99: Harden fetchData with error feedback and a defensive closeRight now, failures from
getAdvanceDataare silently swallowed andloadingState.value.close()is called unguarded. Consider:
- Wrapping the call with a
catchthat shows an error message (for example, using an existing i18n key likebaseForm.form.get.error).- Changing the finally block to
loadingState.value?.close()to avoid potential issues if the loading instance is ever missing.This will make the initial load path more resilient without changing the happy path behavior.
template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
168-176: Normalize theoptionsprop definition to standard Vue patternsThe current
definePropsconfig uses an object astypeand a plain object asdefault, which is unconventional for Vue props and can lead to shared default instances:defineProps({ options: { type: { status: [], department: [], }, default: {}, }, });Consider switching to the usual object-prop pattern with a factory default:
- defineProps({ - options: { - type: { - status: [], - department: [], - }, - default: {}, - }, - }); + defineProps({ + options: { + type: Object, + default: () => ({ + status: [], + department: [], + }), + }, + });This keeps the shape you expect while avoiding shared mutable defaults.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
template/nestJs/locales.json(2 hunks)template/tinyvue/src/locales.json(2 hunks)template/tinyvue/src/views/form/advance/index.vue(1 hunks)template/tinyvue/src/views/form/advance/process-grid/index.vue(1 hunks)template/tinyvue/src/views/form/step/mock.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template/tinyvue/src/locales.json
🔇 Additional comments (5)
template/tinyvue/src/views/form/step/mock.ts (1)
89-103: Status and department options match advanced form usageAdding
statusas string values anddepartmentas{ value, label }objects aligns with howfetchDatain the advance form maps and consumes these options; the mock shape looks consistent and ready for frontend use.template/nestJs/locales.json (2)
281-307: Advanced form enUS translations are coherent and aligned with usageThe new
menu.form.advanceandadvanceForm.form.*keys in enUS look consistent and match the component usages (basic info title, grid columns, actions, validation, empty text).
957-983: Advanced form zhCN translations mirror enUS semanticsThe zhCN entries for
menu.form.advanceandadvanceForm.form.*accurately mirror the English meanings (section titles, grid fields, actions, validation, delete prompt, empty state), ensuring i18n coverage for the new form.template/tinyvue/src/views/form/advance/index.vue (1)
1-31: Advanced form layout and i18n wiring look solidThe page structure (breadcrumb, two content cards, footer actions) and use of
advanceForm.*/stepForm.*locale keys integrate cleanly with the new BasicInfo and ProcessGrid components.Also applies to: 101-138
template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
105-141: Guard$refs.gridRefin the Edit link condition to maintain consistency with the Save linkThe Save link safely guards
$refs.gridRef:v-if="$refs.gridRef && $refs.gridRef.hasActiveRow(data.row)"However, the Edit link directly calls
hasActiveRowwithout checking if$refs.gridRefexists:v-if="!$refs.gridRef.hasActiveRow(data.row)"This creates an asymmetry in null-safety. Using
v-elsewould both eliminate the second condition evaluation and ensure the guard is maintained:- <a - v-if="!$refs.gridRef.hasActiveRow(data.row)" - class="mr-2" - @click="editRow(data.row)" - > + <a + v-else + class="mr-2" + @click="editRow(data.row)" + > <IconEdit class="operation-icon"></IconEdit >{{ $t('advanceForm.form.process.edit') }} </a>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
template/tinyvue/src/views/form/advance/index.vue (1)
64-77: AlignhandleSubmitwith async validation and consider grid validationRight now
handleSubmit:
- Calls
basicInfoRef.value.validForm()synchronously.- Ignores any validation/state from
processGrid.- Always just shows a success toast without actually submitting data.
Once
validFormis changed to return aPromise<boolean>, this should be made async and null‑safe, and ideally wired to grid validation as well, e.g.:- const handleSubmit = () => { - const baseValid = basicInfoRef.value.validForm(); - if (baseValid) { - Modal.message({ - message: t('baseForm.form.submit.success'), - status: 'success', - }); - } - }; + const handleSubmit = async () => { + if (!basicInfoRef.value || !processGrid.value) { + Modal.message({ + message: t('baseForm.form.submit.error'), + status: 'error', + }); + return; + } + + const baseValid = await basicInfoRef.value.validForm(); + // TODO: expose a validation method from ProcessGrid (e.g. validGrid()) + // and include it here before treating the form as successfully submitted. + + if (baseValid) { + Modal.message({ + message: t('baseForm.form.submit.success'), + status: 'success', + }); + } + };This also revisits an earlier comment about incorporating ProcessGrid into the overall validation flow.
template/tinyvue/src/views/form/advance/process-grid/index.vue (2)
215-217: Add basic row validation and feedback before clearing edit state
saveRowcurrently just clears the active row:const saveRow = (row) => { gridRef.value.clearActived(); };Per the earlier review, this lets users “save” incomplete rows with no validation or feedback. Even a simple client‑side check on required fields would improve UX:
- const saveRow = (row) => { - gridRef.value.clearActived(); - }; + const saveRow = (row) => { + if (!row.name || !row.number || !row.department) { + Modal.message({ + message: t('advanceForm.form.validError.null'), + status: 'error', + }); + return; + } + + // TODO: hook up an API call here when backend is ready. + + gridRef.value.clearActived(); + Modal.message({ + message: + t('advanceForm.form.process.save') + + ' ' + + t('baseForm.form.submit.success'), + status: 'success', + }); + };This keeps the grid in edit mode when data is invalid and gives clear success/error messaging when saving.
68-75: FixSelectRendertag casing for the status columnIn the status column slot you’re using
<selectRender>:<template #default="data"> <selectRender :data="data" :options="options.status" field="status" ></selectRender> </template>Since the component is imported as
SelectRender, Vue will resolve it asSelectRender(PascalCase) orselect-render(kebab‑case);<selectRender>is lower‑cased by the HTML parser toselectrenderand likely won’t match the registered component, so this slot will not render as intended.Align this with the department column and earlier naming advice by switching to kebab‑case:
- <template #default="data"> - <selectRender - :data="data" - :options="options.status" - field="status" - ></selectRender> - </template> + <template #default="data"> + <select-render + :data="data" + :options="options.status" + field="status" + ></select-render> + </template>or use
<SelectRender>consistently.template/tinyvue/src/views/form/advance/basic-info/index.vue (1)
143-156:validForm’s synchronous return is unreliable with callback‑based validation
validFormreturnsbaseValidateimmediately after callingformRef.value.validate((valid) => ...). With TinyForm’s callback‑based (and often async) validation, there’s a real risk thatbaseValidateis stillfalsewhen returned, sohandleSubmitin the parent will almost never seetrueeven when the form is valid. This matches an earlier bot comment on a similar pattern.To make this robust, have
validFormreturn aPromise<boolean>and await it from the parent:- const validForm = () => { - let baseValidate = false; - formRef.value.validate((valid) => { - if (!valid) { - Modal.message({ - message: t('baseForm.form.submit.error'), - status: 'error', - }); - } - baseValidate = valid; - }); - - return baseValidate; - }; + const validForm = () => { + if (!formRef.value) return Promise.resolve(false); + + return new Promise<boolean>((resolve) => { + formRef.value.validate((valid: boolean) => { + if (!valid) { + Modal.message({ + message: t('baseForm.form.submit.error'), + status: 'error', + }); + } + resolve(valid); + }); + }); + };Then in the parent
handleSubmitmake itasyncand useconst baseValid = await basicInfoRef.value?.validForm();.This ensures the submit flow uses the actual validation result instead of a stale default.
🧹 Nitpick comments (3)
template/tinyvue/src/views/form/advance/basic-info/index.vue (1)
102-107: Prop definition forprojectDatashould use a proper type and factory default
type: { positionOptions: [], hrOptions: [], teacherOptions: [] }is not a valid Vue prop type (it expects a constructor orPropType), anddefault: {}for an object prop will reuse the same object across component instances.Consider tightening this to something like:
- defineProps({ - projectData: { - type: { positionOptions: [], hrOptions: [], teacherOptions: [] }, - default: {}, - }, - }); + import type { PropType } from 'vue'; + + const props = defineProps({ + projectData: { + type: Object as PropType<{ + positionOptions: any[]; + hrOptions: any[]; + teacherOptions: any[]; + }>, + default: () => ({ + positionOptions: [], + hrOptions: [], + teacherOptions: [], + }), + }, + });This avoids shared mutable defaults and gives you better type checking.
template/tinyvue/src/views/form/advance/index.vue (1)
79-102: HandlegetAdvanceDatafailures and guardloadingStatecleanup
fetchDataalways closes the loading overlay infinally, but ifgetAdvanceData()throws, users just see empty selects with no explanation and no retry hint. Also, ifLoading.serviceever fails or returns nothing,loadingState.value.close()could throw.Consider:
const fetchData = async () => { - loadingState.value = Loading.service({ + loadingState.value = Loading.service({ text: 'loading...', target: document.getElementById('content-container'), background: 'rgba(0, 0, 0, 0.7)', }); try { const { data } = await getAdvanceData(); projectData.positionOptions = data.position; projectData.hrOptions = data.HR; projectData.teacherOptions = data.mentor.map((item) => ({ label: item, value: item, })); processOptions.status = data.status.map((item) => ({ label: item, value: item, })); processOptions.department = data.department; - } finally { - loadingState.value.close(); + } catch (error) { + console.error('Failed to load advance form data:', error); + Modal.message({ + message: t('baseForm.form.get.error'), + status: 'error', + }); + } finally { + loadingState.value && loadingState.value.close(); } };This keeps the UX responsive when the API fails and avoids a potential null dereference on
loadingState.value.template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
110-121: Use the Composition API ref instead of$refsand guardhasActiveRowcallsThe operation column mixes
$refs.gridRefwith Composition API refs:<a v-if="$refs.gridRef && $refs.gridRef.hasActiveRow(data.row)" ... > ... <a v-if="!$refs.gridRef.hasActiveRow(data.row)" ... >The second condition can throw when
$refs.gridRefis undefined (e.g., during initial render), and$refsis generally discouraged in new Composition API code when you already havegridRef.Since template unwrapping exposes
gridRefdirectly, you can simplify and make this safe:- <a - v-if="$refs.gridRef && $refs.gridRef.hasActiveRow(data.row)" - class="mr-2" - @click="saveRow(data.row)" - > + <a + v-if="gridRef && gridRef.hasActiveRow(data.row)" + class="mr-2" + @click="saveRow(data.row)" + > <IconSave class="operation-icon"></IconSave >{{ $t('advanceForm.form.process.save') }} </a> - <a - v-if="!$refs.gridRef.hasActiveRow(data.row)" - class="mr-2" - @click="editRow(data.row)" - > + <a + v-else + class="mr-2" + @click="editRow(data.row)" + >This avoids direct
$refsusage, removes the null‑dereference risk, and guarantees only one of “save” / “edit” is shown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
template/tinyvue/src/views/form/advance/basic-info/index.vue(1 hunks)template/tinyvue/src/views/form/advance/index.vue(1 hunks)template/tinyvue/src/views/form/advance/process-grid/index.vue(1 hunks)template/tinyvue/src/views/form/advance/process-grid/status-render.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template/tinyvue/src/views/form/advance/process-grid/status-render.vue
已修改： |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
216-218: Validation missing before saving row.The
saveRowmethod clears the active state without validating row data, allowing incomplete or invalid rows to be saved. This issue was previously flagged and should be addressed.
🧹 Nitpick comments (1)
template/tinyvue/src/views/form/advance/process-grid/index.vue (1)
159-160: Remove unused imports.
TinyPagerandTinyTagare imported but never used in this component.Apply this diff:
TinyPopconfirm, Modal, TinyDatePicker, - TinyPager, - TinyTag, } from '@opentiny/vue';
| @@ -0,0 +1,241 @@ | |||
| <template lang=""> | |||
There was a problem hiding this comment.
Remove or set the empty lang attribute.
The lang="" attribute on the template tag serves no purpose. Either remove it entirely or set it to a meaningful value.
Apply this diff:
-<template lang="">
+<template>📝 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.
| <template lang=""> | |
| <template> |
🤖 Prompt for AI Agents
In template/tinyvue/src/views/form/advance/process-grid/index.vue around line 1,
the <template> tag contains an empty lang attribute (lang="") which is
unnecessary; remove the lang attribute entirely or set it to a valid value (for
example lang="pug" or lang="html") depending on whether a template preprocessor
is used. Update the tag accordingly so it is either <template> or <template
lang="..."> ensuring consistency with any configured preprocessors in the
project.
| }" | ||
| > | ||
| <template #default="data"> | ||
| <selectRender |
There was a problem hiding this comment.
Fix inconsistent component name casing.
Line 70 uses selectRender (camelCase) while line 49 uses SelectRender (PascalCase) for the same component. Since the import at line 167 is SelectRender, line 70 is incorrect and may cause rendering issues.
Apply this diff:
- <selectRender
+ <SelectRender
:data="data"
:options="options.status"
field="status"
- ></selectRender>
+ ></SelectRender>📝 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.
| <selectRender | |
| <SelectRender | |
| :data="data" | |
| :options="options.status" | |
| field="status" | |
| ></SelectRender> |
🤖 Prompt for AI Agents
template/tinyvue/src/views/form/advance/process-grid/index.vue around line 70:
the component is incorrectly cased as `selectRender` (camelCase) while the
component is imported/used elsewhere as `SelectRender` (PascalCase) and the
import at line 167 is `SelectRender`; change the tag at line 70 from
`selectRender` to `SelectRender` so casing matches the import and the other
usage to avoid rendering issues.
| v-if="$refs.gridRef && $refs.gridRef.hasActiveRow(data.row)" | ||
| class="mr-2" | ||
| @click="saveRow(data.row)" | ||
| > | ||
| <IconSave class="operation-icon"></IconSave | ||
| >{{ $t('advanceForm.form.process.save') }} | ||
| </a> | ||
| <a | ||
| v-if="!$refs.gridRef.hasActiveRow(data.row)" | ||
| class="mr-2" | ||
| @click="editRow(data.row)" | ||
| > | ||
| <IconEdit class="operation-icon"></IconEdit | ||
| >{{ $t('advanceForm.form.process.edit') }} | ||
| </a> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid using $refs in template conditionals.
Lines 113 and 121 access $refs.gridRef directly in the template, which is fragile because refs may not be available during initial render and won't trigger proper reactivity.
Refactor to use a method or computed property:
+// In script setup, add a helper method:
+const isRowActive = (row) => {
+ return gridRef.value?.hasActiveRow?.(row) ?? false;
+};Then update the template:
<template #default="data">
<a
- v-if="$refs.gridRef && $refs.gridRef.hasActiveRow(data.row)"
+ v-if="isRowActive(data.row)"
class="mr-2"
@click="saveRow(data.row)"
>
<IconSave class="operation-icon"></IconSave
>{{ $t('advanceForm.form.process.save') }}
</a>
<a
- v-if="!$refs.gridRef.hasActiveRow(data.row)"
+ v-if="!isRowActive(data.row)"
class="mr-2"
@click="editRow(data.row)"
>📝 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.
| v-if="$refs.gridRef && $refs.gridRef.hasActiveRow(data.row)" | |
| class="mr-2" | |
| @click="saveRow(data.row)" | |
| > | |
| <IconSave class="operation-icon"></IconSave | |
| >{{ $t('advanceForm.form.process.save') }} | |
| </a> | |
| <a | |
| v-if="!$refs.gridRef.hasActiveRow(data.row)" | |
| class="mr-2" | |
| @click="editRow(data.row)" | |
| > | |
| <IconEdit class="operation-icon"></IconEdit | |
| >{{ $t('advanceForm.form.process.edit') }} | |
| </a> | |
| v-if="isRowActive(data.row)" | |
| class="mr-2" | |
| @click="saveRow(data.row)" | |
| > | |
| <IconSave class="operation-icon"></IconSave | |
| >{{ $t('advanceForm.form.process.save') }} | |
| </a> | |
| <a | |
| v-if="!isRowActive(data.row)" | |
| class="mr-2" | |
| @click="editRow(data.row)" | |
| > | |
| <IconEdit class="operation-icon"></IconEdit | |
| >{{ $t('advanceForm.form.process.edit') }} | |
| </a> |
| defineProps({ | ||
| options: { | ||
| type: { | ||
| status: [], | ||
| department: [], | ||
| }, | ||
| default: {}, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Fix invalid TypeScript prop definition.
The current type definition syntax is invalid. For TypeScript, use a proper interface or type definition.
Apply this diff:
+ interface Options {
+ status: Array<{ label: string; value: string }>;
+ department: Array<{ label: string; value: string }>;
+ }
+
defineProps({
options: {
- type: {
- status: [],
- department: [],
- },
- default: {},
+ type: Object as () => Options,
+ default: () => ({ status: [], department: [] }),
},
});Alternatively, use TypeScript's defineProps with type parameters:
interface Props {
options?: {
status: Array<{ label: string; value: string }>;
department: Array<{ label: string; value: string }>;
};
}
const props = withDefaults(defineProps<Props>(), {
options: () => ({ status: [], department: [] }),
});🤖 Prompt for AI Agents
In template/tinyvue/src/views/form/advance/process-grid/index.vue around lines
169-177, the prop uses an invalid TypeScript `type` object syntax; replace it by
declaring a Props interface (specifying options with status and department
arrays of {label:string;value:string}) and use defineProps with that type plus
withDefaults to provide the default options factory returning empty arrays;
update references to use the returned props variable if needed.
| }, | ||
| }); | ||
|
|
||
| const gridRef = ref('gridRef'); |
There was a problem hiding this comment.
Critical: Fix incorrect ref initialization.
gridRef is initialized with the string 'gridRef' instead of null. This will cause runtime errors when methods like gridRef.value.getActiveRow() are called.
Apply this diff:
- const gridRef = ref('gridRef');
+ const gridRef = ref<InstanceType<typeof TinyGrid>>();Or if you prefer simpler typing:
- const gridRef = ref('gridRef');
+ const gridRef = ref(null);📝 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.
| const gridRef = ref('gridRef'); | |
| const gridRef = ref(null); |
🤖 Prompt for AI Agents
In template/tinyvue/src/views/form/advance/process-grid/index.vue around line
179, gridRef is incorrectly initialized to the string 'gridRef' which will break
runtime calls like gridRef.value.getActiveRow(); change the initialization to a
null-based ref (e.g., ref(null) or typed ref of null such as
ref<InstanceType<typeof SomeGridComponent> | null>(null)) so gridRef.value is
null until the component mounts and method calls check for existence before
invoking.
2 participants
PR
效果图：
1920px
1366px
手机端
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.