feat(ui): add visual drag-drop editor for training management page #1115
Conversation
Replace the raw JSON/YAML editor with a modern React-based visual editor for training management. The new UI provides: - Drag-and-drop reordering for sections and problems - Inline editing for section titles - Problem search with auto-complete and validation - Cross-section problem movement via drag-and-drop - Collapsible sections for better organization - Advanced JSON editor with manual apply button Also fixes infinite loop issue caused by useEffect dependencies and adds i18n support for zh, zh_TW, and ko locales.
WalkthroughAdds a React-based training editor: new TrainingEditor and SectionItem components, training types and cycle-detection helper, a TSX page bootstrap (training_edit.page.tsx) and template changes (training_edit.html), removes the old jQuery training_edit.page.ts, and adds many new localization keys in ko, zh, and zh_TW locale files. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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
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 |
DrSmoothl
changed the title
Add prerequisite section selector with circular dependency detection, problem deduplication on cross-section drag, and fix delete path handling. - Add checkbox-based prerequisite selector with collapse for many sections - Implement cycle detection (DFS) to prevent circular dependencies - Add problem deduplication check when moving between sections - Fix training delete endpoint path extraction - Add i18n strings for zh, zh_TW, ko locales
|
Thank you for your submission, we really appreciate it. Comment |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Pull request overview
This pull request replaces the raw JSON editor for training management with a modern visual drag-and-drop interface. The changes significantly improve the user experience for creating and editing training plans by providing intuitive visual editing capabilities.
Key Changes:
- Replaces the old text-based JSON editor with a React-based visual editor featuring drag-and-drop functionality
- Adds comprehensive i18n support with translations for Chinese (Simplified), Chinese (Traditional), and Korean locales
- Implements section and problem management with inline editing, collapsible sections, and prerequisite configuration
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-default/templates/training_edit.html | Updated template to render React component and pass context data via UiContext |
| packages/ui-default/pages/training_edit.page.tsx | New React page component handling initialization, form submission, and deletion logic |
| packages/ui-default/pages/training_edit.page.ts | Removed old jQuery-based implementation |
| packages/ui-default/components/training/editor/types.ts | Type definitions and utility functions for training data structures |
| packages/ui-default/components/training/editor/index.tsx | Export barrel for training editor components |
| packages/ui-default/components/training/editor/TrainingEditor.tsx | Main editor component with form handling, validation, and advanced JSON editor |
| packages/ui-default/components/training/editor/SectionItem.tsx | Section component with drag-drop, title editing, and prerequisite management |
| packages/ui-default/components/training/editor/ProblemSelector.tsx | Problem search and selection component with validation |
| packages/ui-default/components/training/editor/ProblemItem.tsx | Individual problem item with drag-drop reordering |
| packages/ui-default/locales/zh.yaml | Added Chinese (Simplified) translations for new UI strings |
| packages/ui-default/locales/zh_TW.yaml | Added Chinese (Traditional) translations for new UI strings |
| packages/ui-default/locales/ko.yaml | Added Korean translations for new UI strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | ||
| continue; | ||
| } | ||
| onSelect(pid, pdocs[0]); | ||
| } catch { | ||
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); |
There was a problem hiding this comment.
Using .replace('{0}', String(pid)) for string interpolation is fragile and doesn't follow the i18n pattern used elsewhere in the codebase. The i18n function should handle parameter substitution. Verify that the i18n utility supports parameter substitution, and if so, use the standard approach like i18n('Problem {0} not found', pid).
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | |
| continue; | |
| } | |
| onSelect(pid, pdocs[0]); | |
| } catch { | |
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | |
| Notification.error(i18n('Problem {0} not found', pid)); | |
| continue; | |
| } | |
| onSelect(pid, pdocs[0]); | |
| } catch { | |
| Notification.error(i18n('Problem {0} not found', pid)); |
packages/ui-default/locales/zh.yaml
Outdated
| Enter training title: 输入训练标题 | ||
| Brief introduction (max 500 characters): 简要介绍（最多500字） | ||
| Detailed description (supports Markdown): 详细说明（支持Markdown） | ||
| Pin level: 置顶等级 | ||
| Search and add problem...: 搜索并添加题目... | ||
| Training updated successfully: 训练更新成功 | ||
| Training created successfully: 训练创建成功 | ||
| Training deleted successfully: 训练删除成功 | ||
| Failed to save training: 保存训练失败 | ||
| Failed to delete training: 删除训练失败 | ||
| Collapse: 收起 | ||
| Expand: 展开 | ||
| Advanced: 高级 | ||
| JSON Editor: JSON 编辑器 | ||
| Apply to Editor: 应用到编辑器 | ||
| JSON applied to editor: 已应用到编辑器 | ||
| Invalid JSON format: JSON 格式无效 | ||
| 'Invalid JSON format: must be an array': JSON 格式无效：必须是数组 | ||
| 'Edit JSON and click "Apply to Editor" to update the visual editor above. This does not save to server - use "Update" or "Create" button below to save.': 编辑 JSON 后点击「应用到编辑器」更新上方的可视化编辑器。此操作不会保存到服务器，请使用下方的「更新」或「创建」按钮保存。 | ||
| Basic Information: 基础信息 | ||
| 'Confirm deleting this training? Its files and status will be deleted as well.': 确认删除此训练计划吗？其文件和状态也将被删除。 | ||
| Prerequisite Sections: 前置章节 | ||
| 'Users must complete selected sections before accessing this one.': 用户必须完成所选章节后才能访问本章节。 | ||
| This problem already exists in the target section: 该题目已存在于目标章节中 | ||
| 'Cannot add this prerequisite: it would create a circular dependency': 无法添加此前置章节：会产生循环依赖 | ||
| selected: 已选 | ||
| Show less: 收起 | ||
| '{0} more sections': 还有 {0} 个章节 | ||
| Hide: 隐藏 |
There was a problem hiding this comment.
The zh.yaml locale file includes translations that are missing from zh_TW.yaml and ko.yaml. Specifically, the following translations are present in zh.yaml but missing from the other locale files:
- "Enter training title"
- "Brief introduction (max 500 characters)"
- "Detailed description (supports Markdown)"
- "Search and add problem..."
This inconsistency means users of Traditional Chinese and Korean locales will see English text for these strings. All locale files should have the same set of translation keys for feature parity.
| const container = document.getElementById('TrainingEditor'); | ||
| if (!container) return; | ||
|
|
||
| const isEdit = window.location.pathname.includes('/edit'); |
There was a problem hiding this comment.
The isEdit detection using window.location.pathname.includes('/edit') is fragile and could produce false positives if '/edit' appears elsewhere in the URL path. Consider using a more robust method, such as checking the page name from the NamedPage registration or passing isEdit as a prop from the context.
| const isEdit = window.location.pathname.includes('/edit'); | |
| const isEdit = /\/training\/[^/]+\/edit\/?$/.test(window.location.pathname); |
| const path = window.location.pathname; | ||
| const tidMatch = path.match(/\/training\/([^/]+)\/edit/); | ||
| if (!tidMatch) throw new Error('Training ID not found'); | ||
| const tid = tidMatch[1]; | ||
| const baseUrl = path.replace(/\/training\/[^/]+\/edit.*$/, ''); |
There was a problem hiding this comment.
The URL parsing logic using regex /\/training\/([^/]+)\/edit/ could fail if the URL structure is different than expected (e.g., different base paths, query parameters). Consider using a more robust URL parsing approach or extracting the training ID from the UiContext instead of parsing the URL.
| const path = window.location.pathname; | |
| const tidMatch = path.match(/\/training\/([^/]+)\/edit/); | |
| if (!tidMatch) throw new Error('Training ID not found'); | |
| const tid = tidMatch[1]; | |
| const baseUrl = path.replace(/\/training\/[^/]+\/edit.*$/, ''); | |
| const url = new URL(window.location.href); | |
| const segments = url.pathname.split('/').filter(Boolean); | |
| const trainingIndex = segments.lastIndexOf('training'); | |
| if (trainingIndex === -1 || trainingIndex + 1 >= segments.length) { | |
| throw new Error('Training ID not found'); | |
| } | |
| const tid = segments[trainingIndex + 1]; | |
| const basePathSegments = segments.slice(0, trainingIndex); | |
| const baseUrl = basePathSegments.length ? `/${basePathSegments.join('/')}` : ''; |
| } catch (error: any) { | ||
| Notification.error(error.message || i18n('Failed to delete training')); |
There was a problem hiding this comment.
The error handler catches any error but only uses the message property. If the error is not an Error object, accessing error.message might be undefined. Consider adding a type guard or using a default error message more safely.
| } catch (error: any) { | ||
| Notification.error(error.message || i18n('Failed to save training')); | ||
| throw error; |
There was a problem hiding this comment.
The error handler catches any error but only uses the message property. If the error is not an Error object (e.g., a string or other type), accessing error.message might be undefined. Consider adding a type guard or using a default error message more safely.
| const addSection = () => { | ||
| const newId = formData.dag.length > 0 | ||
| ? Math.max(...formData.dag.map((n) => n._id)) + 1 | ||
| : 1; |
There was a problem hiding this comment.
The addSection function generates section IDs by finding the max ID and adding 1. If sections are deleted and recreated, this could potentially lead to ID collisions or inconsistent numbering. Consider using a more robust ID generation strategy, such as a timestamp or UUID, especially if sections can be deleted and re-added.
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | ||
| continue; | ||
| } | ||
| onSelect(pid, pdocs[0]); | ||
| } catch { | ||
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); |
There was a problem hiding this comment.
Using .replace('{0}', String(pid)) for string interpolation is fragile and doesn't follow the i18n pattern used elsewhere in the codebase. The i18n function should handle parameter substitution. Verify that the i18n utility supports parameter substitution, and if so, use the standard approach like i18n('Problem {0} not found', pid).
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | |
| continue; | |
| } | |
| onSelect(pid, pdocs[0]); | |
| } catch { | |
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | |
| Notification.error(i18n('Problem {0} not found', pid)); | |
| continue; | |
| } | |
| onSelect(pid, pdocs[0]); | |
| } catch { | |
| Notification.error(i18n('Problem {0} not found', pid)); |
| <div | ||
| className="training-empty-state" | ||
| style={{ | ||
| textAlign: 'center', | ||
| padding: '2rem', | ||
| background: '#f8f9fa', | ||
| borderRadius: '4px', | ||
| border: '2px dashed #ddd', | ||
| }} | ||
| > | ||
| <p style={{ color: '#666', marginBottom: '1rem' }}> | ||
| {i18n('No sections yet. Click the button above to add your first section.')} | ||
| </p> | ||
| <button | ||
| type="button" | ||
| className="rounded primary button" | ||
| onClick={addSection} | ||
| > | ||
| <span className="icon icon-add" /> {i18n('Add First Section')} | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
The inline style objects are repeated across multiple elements and components. Consider extracting these styles into CSS classes or constants to improve maintainability and reduce code duplication. This applies to styles like the empty state container, section headers, and problem zones.
| const processingRef = React.useRef(false); | ||
|
|
||
| const handleChange = async (value: string) => { | ||
| if (processingRef.current) return; | ||
| if (!value.trim()) return; | ||
|
|
||
| const keys = value.split(',').filter((v) => v.trim()); | ||
| if (keys.length === 0) return; | ||
|
|
||
| processingRef.current = true; | ||
| try { | ||
| for (const key of keys) { | ||
| const pid = Number.parseInt(key, 10); | ||
| if (Number.isNaN(pid)) continue; | ||
| if (existingPids.includes(pid)) { | ||
| Notification.warn(i18n('Problem already added')); | ||
| continue; | ||
| } | ||
| try { | ||
| const pdocs = await api('problems', { ids: [pid] }, ['docId', 'pid', 'title']); | ||
| if (pdocs.length === 0) { | ||
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | ||
| continue; | ||
| } | ||
| onSelect(pid, pdocs[0]); | ||
| } catch { | ||
| Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); | ||
| } | ||
| } | ||
| } finally { | ||
| processingRef.current = false; | ||
| ref.current?.clear(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The processingRef flag is used to prevent duplicate processing, but if an error occurs between setting it to true and the finally block, subsequent attempts to add problems will be blocked. While the finally block should handle this, consider adding error recovery logic or using a more robust state management approach.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @packages/ui-default/components/training/editor/ProblemSelector.tsx:
- Around line 29-31: The duplicate-check fails when existingPids contains string
IDs because existingPids is (number | string)[] while pid is a number; in the
ProblemSelector component normalize types before comparing by either mapping
existingPids to numbers (e.g., Number(...) for each entry) or by converting pid
to a string and comparing against existingPids as strings; update the check
around existingPids.includes(pid) to perform the normalized comparison (refer to
existingPids and pid in ProblemSelector.tsx) and keep
Notification.warn(i18n('Problem already added')) behavior unchanged.
In @packages/ui-default/components/training/editor/TrainingEditor.tsx:
- Around line 74-80: The advanced JSON textarea is using defaultValue so it
stays stale; make it a controlled input: change the textarea to use
value={advancedJson} and add an onChange handler that calls
setAdvancedJson(e.target.value). Also update the effect that sets
dagJsonRef.current to run when either showAdvanced or formData.dag changes (use
[showAdvanced, formData.dag]) and when showAdvanced is true
setAdvancedJson(JSON.stringify(formData.dag, null, 2)) so reopening the editor
reflects the latest formData.dag; keep dagJsonRef, useEffect, showAdvanced,
setAdvancedJson and advancedJson as the referenced symbols.
- Around line 35-52: The effect drops string pids by filtering only numbers;
change the collection/filtering in loadProblems to accept both numbers and
strings (e.g., typeof p === 'number' || typeof p === 'string'), dedupe those IDs
into uniquePids, pass uniquePids to the api call, and when building the cache
use the pid field from returned ProblemDoc (e.g., key by pdoc.pid) so
string-based problem IDs are stored and retrievable by hasProblem/SectionItem;
update references inside loadProblems and the newPdict construction and keep
setPdict((prev) => ({ ...prev, ...newPdict })) as is.
- Around line 192-212: handleSubmit currently mutates the formData object
directly by assigning to formData.description; instead create an immutable copy
for submission (e.g., construct a payload by shallow-copying formData and
overriding description from descriptionRef.current.value) and use that payload
when performing validation and calling onSubmit; keep using setLoading and
finally block as-is and do not assign into formData directly to avoid React
state mutation issues.
🧹 Nitpick comments (9)
packages/ui-default/pages/training_edit.page.tsx (2)
64-67: Inconsistent error handling between submit and delete handlers.
handleSubmitre-throws the error after showing a notification (line 66), whilehandleDelete(lines 80-82) only shows a notification without re-throwing. This inconsistency may cause unexpected behavior if callers expect errors to propagate.Consider making the error handling consistent:
Suggested fix
const handleDelete = async () => { try { const path = window.location.pathname; const tidMatch = path.match(/\/training\/([^/]+)\/edit/); if (!tidMatch) throw new Error('Training ID not found'); const tid = tidMatch[1]; const baseUrl = path.replace(/\/training\/[^/]+\/edit.*$/, ''); const res = await request.post(`${baseUrl}/training/${tid}`, { operation: 'delete' }); Notification.success(i18n('Training deleted successfully')); window.location.href = res.url || `${baseUrl}/training`; } catch (error: any) { Notification.error(error.message || i18n('Failed to delete training')); + throw error; } };
36-36: Fragile edit mode detection based on URL substring.Using
window.location.pathname.includes('/edit')is fragile and could match unintended paths (e.g.,/training/editor/...). SinceNamedPagealready receives['training_edit', 'training_create']as page names, consider using that context if available, or use a more specific regex pattern.packages/ui-default/components/training/editor/ProblemSelector.tsx (3)
26-28: String problem IDs (alphanumeric) are silently ignored.The code parses input as integers with
parseInt(key, 10), so alphanumeric problem IDs like'P1001'will be skipped (returns NaN). If the system supports string PIDs (as suggested by the type(number | string)[]forexistingPids), this could cause user confusion.Consider handling both numeric and string PIDs:
Suggested enhancement
for (const key of keys) { - const pid = Number.parseInt(key, 10); - if (Number.isNaN(pid)) continue; + const trimmedKey = key.trim(); + const numericPid = Number.parseInt(trimmedKey, 10); + const pid = Number.isNaN(numericPid) ? trimmedKey : numericPid; + if (!pid) continue;
33-42: N+1 API calls pattern - consider batching.Each problem ID triggers a separate API call inside the loop. For inputs with multiple comma-separated IDs, this results in N sequential requests. Consider batching all IDs into a single API call for better performance.
Suggested optimization
+ // Batch fetch all problems at once + const validPids = keys + .map((k) => Number.parseInt(k, 10)) + .filter((p) => !Number.isNaN(p) && !existingPids.includes(p)); + + if (validPids.length === 0) return; + + try { + const pdocs = await api('problems', { ids: validPids }, ['docId', 'pid', 'title']); + const pdocMap = new Map(pdocs.map((p) => [p.docId, p])); + + for (const pid of validPids) { + const pdoc = pdocMap.get(pid); + if (pdoc) { + onSelect(pid, pdoc); + } else { + Notification.error(i18n('Problem {0} not found', pid)); + } + } + } catch { + Notification.error(i18n('Failed to fetch problems')); + }
36-36: Use i18n's native parameter substitution instead of manual replace.The
i18nfunction fromvj/utils/base.tsalready supports parameter substitution. Replace manual.replace('{0}', ...)with the native syntax:- Notification.error(i18n('Problem {0} not found').replace('{0}', String(pid))); + Notification.error(i18n('Problem {0} not found', pid));This also applies to line 41.
packages/ui-default/components/training/editor/ProblemItem.tsx (1)
54-58: Consider handling missingpdoc.titlegracefully.If
pdocexists butpdoc.titleis an empty string or undefined, the display may look incomplete (e.g., just showing the pid prefix with no title).Suggested improvement
const displayTitle = pdoc - ? `${pdoc.pid ? `${pdoc.pid} ` : ''}${pdoc.title}` + ? `${pdoc.pid ? `${pdoc.pid} ` : ''}${pdoc.title || i18n('Untitled')}` : `${i18n('Problem')} ${pid}`;packages/ui-default/components/training/editor/SectionItem.tsx (2)
31-34:titleValuestate may become stale ifnode.titleis updated externally.If the parent updates
node.title(e.g., via JSON editor apply), this local state won't reflect the change since it's only initialized once.Suggested fix using useEffect to sync
const [titleValue, setTitleValue] = React.useState(node.title); + + React.useEffect(() => { + setTitleValue(node.title); + }, [node.title]);
222-249: Consider memoizing section index lookup for better render performance.The
findIndexcall on line 224 runs for every prerequisite on every render. With many sections, this could become a performance concern.Suggested optimization
+ const sectionIndexMap = React.useMemo( + () => new Map(allSections.map((s, i) => [s._id, i])), + [allSections] + ); {visiblePrereqs.map((s) => { const isChecked = node.requireNids.includes(s._id); - const sectionLabel = s.title || `${i18n('Section')} ${allSections.findIndex((x) => x._id === s._id) + 1}`; + const sectionLabel = s.title || `${i18n('Section')} ${(sectionIndexMap.get(s._id) ?? 0) + 1}`;packages/ui-default/components/training/editor/TrainingEditor.tsx (1)
176-190: JSON validation only checks for array type; individual node structure is not validated.Malformed node objects (missing
_id,pids, etc.) could cause runtime errors in downstream components.Suggested validation enhancement
const handleApplyJson = () => { const jsonValue = advancedEditorRef.current?.value; if (!jsonValue) return; try { const parsed = JSON.parse(jsonValue); if (!Array.isArray(parsed)) { Notification.error(i18n('Invalid JSON format: must be an array')); return; } + for (const node of parsed) { + if (typeof node._id !== 'number' || !Array.isArray(node.pids)) { + Notification.error(i18n('Invalid node format: each node must have _id (number) and pids (array)')); + return; + } + } updateFormField('dag', parsed); Notification.success(i18n('JSON applied to editor')); } catch { Notification.error(i18n('Invalid JSON format')); } };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/ui-default/components/training/editor/ProblemItem.tsxpackages/ui-default/components/training/editor/ProblemSelector.tsxpackages/ui-default/components/training/editor/SectionItem.tsxpackages/ui-default/components/training/editor/TrainingEditor.tsxpackages/ui-default/components/training/editor/index.tsxpackages/ui-default/components/training/editor/types.tspackages/ui-default/locales/ko.yamlpackages/ui-default/locales/zh.yamlpackages/ui-default/locales/zh_TW.yamlpackages/ui-default/pages/training_edit.page.tspackages/ui-default/pages/training_edit.page.tsxpackages/ui-default/templates/training_edit.html
💤 Files with no reviewable changes (1)
- packages/ui-default/pages/training_edit.page.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/ui-default/components/training/editor/ProblemSelector.tsx (2)
packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/utils/index.ts (1)
api(5-9)
packages/ui-default/pages/training_edit.page.tsx (4)
packages/ui-default/components/training/editor/types.ts (2)
TrainingNode(3-8)TrainingFormData(10-16)packages/ui-default/misc/Page.ts (1)
NamedPage(37-37)packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/training/editor/TrainingEditor.tsx (1)
TrainingEditor(19-431)
packages/ui-default/components/training/editor/SectionItem.tsx (4)
packages/ui-default/components/training/editor/types.ts (3)
TrainingNode(3-8)DND_TYPES(29-32)wouldCreateCycle(54-78)packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/training/editor/ProblemItem.tsx (1)
ProblemItem(23-93)packages/ui-default/components/training/editor/ProblemSelector.tsx (1)
ProblemSelector(13-66)
packages/ui-default/components/training/editor/TrainingEditor.tsx (4)
packages/ui-default/components/training/editor/types.ts (3)
TrainingFormData(10-16)TrainingNode(3-8)hasProblem(46-52)packages/ui-default/utils/index.ts (1)
api(5-9)packages/ui-default/utils/base.ts (1)
i18n(14-17)packages/ui-default/components/dialog/index.tsx (1)
confirm(253-258)
packages/ui-default/components/training/editor/ProblemItem.tsx (3)
packages/ui-default/components/training/editor/types.ts (1)
DND_TYPES(29-32)packages/ui-default/components/training/editor/SectionItem.tsx (1)
drop(124-128)packages/ui-default/utils/base.ts (1)
i18n(14-17)
⏰ 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: Agent
🔇 Additional comments (26)
packages/ui-default/locales/zh.yaml (1)
995-1037: LGTM! Training editor localization keys are well-structured.The Chinese Simplified translations for the new training editor UI are complete and consistent with the existing translation style in the file. Key coverage includes section management, problem handling, JSON editor interactions, prerequisite sections, and cycle detection messaging.
packages/ui-default/locales/ko.yaml (1)
721-753: LGTM! Korean localization keys align with other locale files.The Korean translations for the training editor UI are complete and consistent with the keys added in other locale files (zh.yaml, zh_TW.yaml).
packages/ui-default/components/training/editor/index.tsx (1)
1-2: LGTM! Clean barrel export pattern.The index file provides a clean public API surface by exporting
TrainingEditorand re-exporting all types. This is a standard and appropriate pattern for component packages.packages/ui-default/pages/training_edit.page.tsx (1)
85-93: LGTM! React 18+ createRoot usage is correct.The component rendering with
createRootis appropriate for React 18+/19 and correctly passes all required props toTrainingEditor.packages/ui-default/templates/training_edit.html (2)
15-18: LGTM! TrainingEditor container setup is clean.The container div with noscript fallback and loading indicator provides good UX while the React component initializes.
29-30: No changes needed—the current code is correct as-is.The original review comment suggests adding
|jsonfilter to line 30 for consistency with line 29, but this misses a crucial implementation detail:dagis pre-serialized to a JSON string on the server side (inpackages/hydrooj/src/handler/training.tsline 203:this.response.body.dag = JSON.stringify(this.tdoc.dag, null, 2)), whiletdocis passed as a raw object. Therefore:
- Line 29 correctly uses
{{ tdoc|json|safe }}to serialize the object- Line 30 correctly uses
{{ dag|safe }}for the pre-serialized stringAdding
|jsonto line 30 would double-serialize the string and break the code. The current implementation is correct and follows proper Jinja2 filtering practices.packages/ui-default/components/training/editor/ProblemSelector.tsx (1)
50-65: LGTM! Component rendering is clean.The JSX structure with labeled autocomplete input and appropriate props is well-organized.
packages/ui-default/locales/zh_TW.yaml (1)
997-1029: LGTM! Traditional Chinese translations are complete and consistent.The zh_TW translations for the training editor UI appropriately use Traditional Chinese terminology (e.g., 新增, 套用, 章節) and maintain consistency with the Simplified Chinese (zh.yaml) translations in meaning.
packages/ui-default/components/training/editor/ProblemItem.tsx (4)
1-5: LGTM!Imports are clean and appropriate. The component correctly imports react-dnd hooks, the i18n utility, and local DND_TYPES constant.
7-21: LGTM!Interface definitions are well-typed.
ProblemItemPropscorrectly defines the component's contract, andDragItemcaptures the drag state needed for react-dnd operations.
28-50: LGTM!The drag-and-drop logic is correctly implemented:
useDragcorrectly sets up the draggable item with type, item data, and isDragging state.useDropproperly handles hover for same-section reordering with index mutation pattern (line 45) which is a standard react-dnd optimization.- The
isOvercollector correctly checks for same-section drops only.
60-92: LGTM!The render output is well-structured with:
- Appropriate visual feedback for drag (
isDraggingopacity) and drop (isOverbackground)- Accessible aria-label on the remove button
- Clean layout with flexbox
packages/ui-default/components/training/editor/SectionItem.tsx (7)
1-8: LGTM!Imports are appropriate. The component correctly imports all necessary dependencies including the
wouldCreateCycleutility for prerequisite validation.
39-60: LGTM!Section drag-and-drop is correctly implemented with:
- Proper use of
previewanddragrefs for handle-based dragging- Standard hover-based reordering with index mutation
64-79: LGTM!Title editing handlers correctly:
- Validate non-empty titles before saving
- Revert to original on empty input or Escape
- Support both blur and Enter key to save
81-97: LGTM!Problem management handlers are straightforward and correct. The reorder logic properly creates a new array and splices elements.
99-108: LGTM!The prerequisite change handler correctly uses
wouldCreateCycleto prevent circular dependencies before updating. Good defensive coding with cycle detection fromtypes.ts.
118-132: LGTM!The problem drop zone correctly:
- Accepts only
PROBLEMtype- Only triggers
onMoveProblemfor cross-section drops (line 125 check)- Provides visual feedback via
isOverProblemZone
280-298: LGTM!The problem list rendering correctly:
- Shows empty state message when no problems
- Uses stable composite key (
${node._id}-${pid})- Passes correct props to
ProblemItempackages/ui-default/components/training/editor/TrainingEditor.tsx (4)
1-9: LGTM!Imports are well-organized with type-only imports separated, and all necessary dependencies are included.
92-103: LGTM!The
addSectionfunction correctly generates unique IDs by finding the max existing ID and incrementing, with a fallback for empty arrays.
124-170: LGTM!The
moveProblemBetweenSectionsfunction correctly:
- Handles same-section reordering separately (no dedup needed)
- Checks for duplicates using
hasProblembefore cross-section moves- Shows user-friendly notification when duplicate is detected
299-352: LGTM!The sections rendering correctly handles the empty state with a helpful message and CTA button, and renders
SectionItemcomponents with all necessary props.packages/ui-default/components/training/editor/types.ts (3)
1-32: LGTM!Type definitions are well-structured:
TrainingNodecorrectly models section data with prerequisites and problemsTrainingFormDatacaptures the full form stateDND_TYPESusesas constfor type safety
34-52: LGTM!The
isSameProblemfunction correctly handles the dual identity case where problems can be referenced by numeric ID or string PID, usingdocIdas the canonical identifier when both docs are available.hasProblemprovides a clean wrapper for array membership checks.
54-78: LGTM!The
wouldCreateCyclefunction correctly implements cycle detection:
- Uses iterative DFS (stack-safe for deep graphs)
- Builds a lookup map for O(1) section access
- Detects if adding
newPrereqIdas a prerequisite ofcurrentNodeIdwould create a cycle by checking ifcurrentNodeIdis reachable fromnewPrereqId
| if (existingPids.includes(pid)) { | ||
| Notification.warn(i18n('Problem already added')); | ||
| continue; |
There was a problem hiding this comment.
Type mismatch risk in duplicate check.
existingPids is typed as (number | string)[], but pid is always a number (from parseInt). The includes() check uses strict equality, so if existingPids contains '123' (string) and pid is 123 (number), the duplicate won't be detected.
Consider normalizing types for comparison:
Suggested fix
- if (existingPids.includes(pid)) {
+ if (existingPids.some((p) => String(p) === String(pid))) {📝 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.
| if (existingPids.includes(pid)) { | |
| Notification.warn(i18n('Problem already added')); | |
| continue; | |
| if (existingPids.some((p) => String(p) === String(pid))) { | |
| Notification.warn(i18n('Problem already added')); | |
| continue; |
🤖 Prompt for AI Agents
In @packages/ui-default/components/training/editor/ProblemSelector.tsx around
lines 29 - 31, The duplicate-check fails when existingPids contains string IDs
because existingPids is (number | string)[] while pid is a number; in the
ProblemSelector component normalize types before comparing by either mapping
existingPids to numbers (e.g., Number(...) for each entry) or by converting pid
to a string and comparing against existingPids as strings; update the check
around existingPids.includes(pid) to perform the normalized comparison (refer to
existingPids and pid in ProblemSelector.tsx) and keep
Notification.warn(i18n('Problem already added')) behavior unchanged.
| React.useEffect(() => { | ||
| const loadProblems = async () => { | ||
| const allPids = formData.dag.flatMap((node) => node.pids).filter((p) => typeof p === 'number'); | ||
| if (allPids.length === 0) return; | ||
| const uniquePids = [...new Set(allPids)]; | ||
| try { | ||
| const pdocs = await api('problems', { ids: uniquePids }, ['docId', 'pid', 'title']); | ||
| const newPdict: Record<number, ProblemDoc> = {}; | ||
| for (const pdoc of pdocs) { | ||
| newPdict[pdoc.docId] = pdoc; | ||
| } | ||
| setPdict((prev) => ({ ...prev, ...newPdict })); | ||
| } catch (e) { | ||
| console.error('Failed to load problems:', e); | ||
| } | ||
| }; | ||
| loadProblems(); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the provided search for string problem IDs
rg -n "pids.*string" --type=ts -C2 packages/ui-default/Repository: hydro-dev/Hydro
Length of output: 1836
🏁 Script executed:
#!/bin/bash
# Find the type definition for pids to confirm it allows strings
# First locate the file and search for the type or interface definition
rg -n "pids\s*[:?]" --type=ts packages/ui-default/components/training/ -B2 -A2Repository: hydro-dev/Hydro
Length of output: 4389
🏁 Script executed:
#!/bin/bash
# Look for the formData type definition and dag structure
rg -n "interface.*dag|type.*dag" --type=ts packages/ui-default/Repository: hydro-dev/Hydro
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Check the api call to see what it expects and how problem IDs are used
rg -n "api\(" --type=ts packages/ui-default/components/training/editor/TrainingEditor.tsx -A2 -B2Repository: hydro-dev/Hydro
Length of output: 319
String problem IDs are filtered out and silently ignored when loading problem metadata.
Line 37 filters to only typeof p === 'number', but the pids type definition allows (number | string)[]. String-based problem IDs are accepted throughout the rest of the codebase (see SectionItem.tsx and the hasProblem helper), so their metadata should also be fetched. String IDs will currently be silently dropped, leaving them without cached documentation.
🤖 Prompt for AI Agents
In @packages/ui-default/components/training/editor/TrainingEditor.tsx around
lines 35 - 52, The effect drops string pids by filtering only numbers; change
the collection/filtering in loadProblems to accept both numbers and strings
(e.g., typeof p === 'number' || typeof p === 'string'), dedupe those IDs into
uniquePids, pass uniquePids to the api call, and when building the cache use the
pid field from returned ProblemDoc (e.g., key by pdoc.pid) so string-based
problem IDs are stored and retrievable by hasProblem/SectionItem; update
references inside loadProblems and the newPdict construction and keep
setPdict((prev) => ({ ...prev, ...newPdict })) as is.
| const dagJsonRef = React.useRef(JSON.stringify(initialData.dag, null, 2)); | ||
| React.useEffect(() => { | ||
| if (showAdvanced) { | ||
| dagJsonRef.current = JSON.stringify(formData.dag, null, 2); | ||
| setAdvancedJson(dagJsonRef.current); | ||
| } | ||
| }, [showAdvanced]); |
There was a problem hiding this comment.
Advanced JSON textarea won't reflect changes after initial display.
The defaultValue on line 376 won't update when advancedJson state changes since React only uses defaultValue for the initial render. If users toggle the advanced editor, close it, modify sections visually, then reopen it, they'll see stale JSON.
Suggested fix: use controlled value with onChange
<textarea
ref={advancedEditorRef}
className="textbox"
- defaultValue={advancedJson}
+ value={advancedJson}
+ onChange={(e) => setAdvancedJson(e.target.value)}
style={{ height: '400px' }}
data-json
/>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/ui-default/components/training/editor/TrainingEditor.tsx around
lines 74 - 80, The advanced JSON textarea is using defaultValue so it stays
stale; make it a controlled input: change the textarea to use
value={advancedJson} and add an onChange handler that calls
setAdvancedJson(e.target.value). Also update the effect that sets
dagJsonRef.current to run when either showAdvanced or formData.dag changes (use
[showAdvanced, formData.dag]) and when showAdvanced is true
setAdvancedJson(JSON.stringify(formData.dag, null, 2)) so reopening the editor
reflects the latest formData.dag; keep dagJsonRef, useEffect, showAdvanced,
setAdvancedJson and advancedJson as the referenced symbols.
| const handleSubmit = async (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| if (descriptionRef.current) { | ||
| formData.description = descriptionRef.current.value; | ||
| } | ||
| if (formData.dag.length === 0) { | ||
| Notification.error(i18n('Please add at least one section')); | ||
| return; | ||
| } | ||
| const emptySection = formData.dag.find((node) => node.pids.length === 0); | ||
| if (emptySection) { | ||
| Notification.error(i18n('Each section must have at least one problem')); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| try { | ||
| await onSubmit(formData); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Direct mutation of formData object before submit.
Line 195 directly mutates formData.description which violates React's immutable state pattern. While it may work due to the subsequent onSubmit call, this could cause subtle bugs or stale closures.
Suggested fix: create a copy for submission
const handleSubmit = async (e: React.FormEvent) => {
e.preventDefault();
+ const submitData = { ...formData };
if (descriptionRef.current) {
- formData.description = descriptionRef.current.value;
+ submitData.description = descriptionRef.current.value;
}
- if (formData.dag.length === 0) {
+ if (submitData.dag.length === 0) {
Notification.error(i18n('Please add at least one section'));
return;
}
- const emptySection = formData.dag.find((node) => node.pids.length === 0);
+ const emptySection = submitData.dag.find((node) => node.pids.length === 0);
if (emptySection) {
Notification.error(i18n('Each section must have at least one problem'));
return;
}
setLoading(true);
try {
- await onSubmit(formData);
+ await onSubmit(submitData);
} finally {
setLoading(false);
}
};🤖 Prompt for AI Agents
In @packages/ui-default/components/training/editor/TrainingEditor.tsx around
lines 192 - 212, handleSubmit currently mutates the formData object directly by
assigning to formData.description; instead create an immutable copy for
submission (e.g., construct a payload by shallow-copying formData and overriding
description from descriptionRef.current.value) and use that payload when
performing validation and calling onSubmit; keep using setLoading and finally
block as-is and do not assign into formData directly to avoid React state
mutation issues.
… components instead of creating own; Added circular dependency detection functionality; Performed performance optimizations.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/ui-default/components/training/TrainingEditor.tsx:
- Around line 49-51: The textarea uses defaultValue={advancedJson}, so updates
to advancedJson (from React.useEffect watching showAdvanced and formData.dag)
won't update the textarea once mounted; either make the textarea controlled by
replacing defaultValue with value={advancedJson} and ensure onChange updates
advancedJson, or when you want to keep it uncontrolled reset
advancedEditorInitialized whenever formData.dag changes so the textarea is
re-mounted with the new defaultValue; target the React.useEffect that sets
advancedJson, the showAdvanced/formData.dag deps, the advancedJson state, the
advancedEditorInitialized flag, and the textarea currently using
defaultValue={advancedJson} to implement the chosen fix.
- Around line 71-74: The deleteSection callback removes the node from dag but
leaves its _id in other nodes' requireNids, causing orphaned prerequisite
references; update deleteSection (the React.useCallback named deleteSection) to,
after filtering out the deleted node, also map the remaining dag nodes and
remove nodeId from each node.requireNids (e.g., set requireNids =
requireNids.filter(id => id !== nodeId) or clear to [] if none remain) so all
references to the deleted section are cleaned in the setFormData update.
- Around line 86-100: handleApplyJson currently only checks that parsed JSON is
an array but doesn't validate each item's shape, which can lead to runtime
errors in SectionItem; update handleApplyJson to iterate over parsed array and
validate required fields (_id, title, requireNids, pids) and their expected
types (e.g., _id and title strings, requireNids/pids arrays) before calling
setFormData, collect and surface any validation errors via Notification.error,
and only call setFormData/Notification.success when all items pass validation;
reference advancedEditorRef for the source, parsed variable for the data,
setFormData for applying it, and ensure you bail out on validation failure to
prevent invalid state.
🧹 Nitpick comments (5)
packages/ui-default/pages/training_edit.page.tsx (3)
32-34: React root is not unmounted on page cleanup.When using
createRoot, the root should be unmounted when the page is destroyed to prevent memory leaks, especially in SPA-like navigation. TheNamedPagecallback doesn't return a cleanup function.Recommended cleanup pattern
const page = new NamedPage(['training_edit', 'training_create'], () => { const container = document.getElementById('TrainingEditor'); if (!container) return; + + const root = createRoot(container); // ... existing setup code ... - createRoot(container).render( + root.render( <TrainingEditor initialData={initialData} isEdit={isEdit} onSubmit={handleSubmit} onDelete={isEdit ? handleDelete : undefined} canDelete={UiContext.canDelete} />, ); + + return () => { + root.unmount(); + }; });Verify if
NamedPagesupports cleanup callbacks; if not, this may be acceptable given the application's navigation pattern.
36-36: URL-based edit detection may be fragile.Using
window.location.pathname.includes('/edit')to determine edit mode could break if the URL structure changes or if 'edit' appears elsewhere in the path.Consider using the page name from the
NamedPageregistration instead, or passingisEditviaUiContextfrom the server template (which already distinguishes viapage_name == 'training_edit').
70-83: URL manipulation for delete could be simplified.The manual URL extraction and reconstruction (lines 72-77) is complex and error-prone. Consider having the server provide the delete endpoint URL via
UiContext, similar to howcanDeleteis provided.packages/ui-default/components/training/SectionItem.tsx (1)
33-49: Autocomplete initialization dependency array may cause issues.The
onUpdatefunction is in the dependency array. IfonUpdateis recreated on each parent render (non-memoized), this effect would re-run, thoughautocompleteInitialized.currentguards against re-initialization. However, theonChangehandler captures the firstonUpdatereference and won't update ifonUpdatechanges.Consider using a ref for
onUpdateto ensure the latest callback is always called:Alternative pattern using ref for callback
const onUpdateRef = React.useRef(onUpdate); React.useEffect(() => { onUpdateRef.current = onUpdate; }, [onUpdate]); // Then in autocomplete onChange: autocompleteRef.current.onChange((val: string) => { // ... parse pids ... onUpdateRef.current(node._id, { pids }); });packages/ui-default/components/training/TrainingEditor.tsx (1)
200-221: Consider clarifying the advanced editor's one-way sync behavior.The current implementation allows JSON → visual editor sync (via "Apply to Editor"), but visual editor changes don't reflect in the JSON textarea while it's open. This is a deliberate design choice, but users might expect bidirectional sync.
Consider adding a "Refresh from Editor" button or updating the help text to clarify this behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/ui-default/components/training/SectionItem.tsxpackages/ui-default/components/training/TrainingEditor.tsxpackages/ui-default/components/training/index.tsxpackages/ui-default/components/training/types.tspackages/ui-default/locales/ko.yamlpackages/ui-default/locales/zh.yamlpackages/ui-default/locales/zh_TW.yamlpackages/ui-default/pages/training_edit.page.tsxpackages/ui-default/templates/training_edit.html
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui-default/components/training/SectionItem.tsx (2)
packages/ui-default/components/training/types.ts (2)
TrainingNode(1-6)wouldCreateCycle(16-35)packages/ui-default/utils/base.ts (1)
i18n(14-17)
🔇 Additional comments (18)
packages/ui-default/locales/ko.yaml (1)
721-744: LGTM!The Korean translations for the new training section UI are comprehensive and consistent with the existing localization patterns. The placeholder
{0}in'Show all {0} sections'is correctly used.packages/ui-default/components/training/index.tsx (1)
1-2: LGTM!Clean barrel file that correctly exposes
TrainingEditorand type definitions for the public API.packages/ui-default/templates/training_edit.html (2)
16-71: Good accessibility pattern with noscript fallback.The noscript fallback ensures the training editor remains functional for users with JavaScript disabled, maintaining form submission capability.
84-90: The current code is correct as written—no changes needed.On line 203 of the handler,
dagis explicitly serialized viaJSON.stringify(this.tdoc.dag, null, 2)before being passed to the template. By the timedagreaches line 87 of the template, it's already a valid JSON string. Using|json|safe(as suggested) would double-encode it, breaking the JavaScript. The|safefilter alone is appropriate here because the value is server-generated, not user-controlled. This is different fromtdocon line 86, which is a raw object requiring|jsonserialization.Likely an incorrect or invalid review comment.
packages/ui-default/pages/training_edit.page.tsx (1)
20-30: LGTM on parseDag defensive handling.The function correctly handles undefined, string (with JSON parsing), and array inputs with appropriate fallback to
defaultDagon parse errors.packages/ui-default/locales/zh.yaml (1)
995-1018: LGTM!The Simplified Chinese translations for training section management are comprehensive and well-translated. The placeholder usage in
'Show all {0} sections'is correct.packages/ui-default/components/training/SectionItem.tsx (3)
219-225: InputdefaultValuewon't reflect externalnode.pidschanges.Using
defaultValuemeans the input shows the initial value but won't update ifnode.pidschanges externally (e.g., via the JSON editor's "Apply to Editor"). Since the autocomplete widget manages its own state, this may be intentional, but could cause UI inconsistency.Verify if the autocomplete widget needs to be updated when
node.pidschanges externally, or if the current behavior (sync only via user interaction) is acceptable.
80-89: Good implementation of circular dependency detection.The
handleRequireNidsChangecorrectly useswouldCreateCycleto prevent users from creating circular prerequisites, with appropriate error notification.
107-170: LGTM on section header UI.The header provides intuitive controls for reordering, inline title editing, collapse/expand, and delete. Proper disabled states and visual feedback are implemented.
packages/ui-default/components/training/types.ts (2)
1-14: LGTM on type definitions.The
TrainingNodeandTrainingFormDatainterfaces are well-structured with appropriate field types. Thepids: (number | string)[]type correctly accommodates both numeric and string-based problem IDs.
16-35: Correct cycle detection algorithm.The
wouldCreateCyclefunction correctly detects if addingnewPrereqIdas a prerequisite tocurrentNodeIdwould create a cycle. The DFS approach with a visited set handles:
- Direct cycles (A → B → A)
- Transitive cycles (A → B → C → A)
- Existing cycles in the graph (defensive)
Time complexity is O(V+E), which is efficient for typical training plan sizes.
packages/ui-default/components/training/TrainingEditor.tsx (6)
1-8: LGTM!Imports are well-organized and the threshold constant is appropriately defined at module scope.
10-31: LGTM!Props interface is well-typed with appropriate optional markers. The use of
initialData.dag.lengthforisLargeTrainingensures collapsed state remains stable as users add/remove sections, which is good UX.
102-120: LGTM!The submit handler properly:
- Reads the description from the uncontrolled textarea ref
- Validates minimum requirements (sections and problems)
- Manages loading state with
try/finallyto ensure cleanupThe error handling is delegated to the parent
onSubmitcallback, which is an appropriate separation of concerns.
130-164: LGTM!The Basic Information form section is well-structured with:
- Proper controlled inputs for title, pin, and introduce
- Uncontrolled description textarea (appropriate for external markdown editor integration)
- Good accessibility with labels wrapping inputs
- Appropriate
requiredandautoFocusattributes
166-198: LGTM!Training sections rendering is well-implemented:
- Clear empty state with actionable guidance
- Proper key usage with
node._id- Callbacks correctly bound for section operations
defaultCollapsedoptimization for large trainings
223-230: LGTM!Action buttons are appropriately implemented with:
- Loading state disabling the submit button
- Conditional delete button based on edit mode and permissions
- Clear button labels with i18n support
packages/ui-default/locales/zh_TW.yaml (1)
997-1020: LGTM!The Traditional Chinese translations are well-crafted:
- Accurate translations that match the UI context
- Proper use of Traditional Chinese characters (e.g., 章節, 編輯器, 套用)
- Keys align with the strings used in
TrainingEditor.tsx- Special character handling is correct (e.g., line 1013 with quotes around the key)
| React.useEffect(() => { | ||
| if (showAdvanced) setAdvancedJson(JSON.stringify(formData.dag, null, 2)); | ||
| }, [showAdvanced, formData.dag]); |
There was a problem hiding this comment.
State update won't sync to uncontrolled textarea.
This effect updates advancedJson state when showAdvanced or formData.dag changes. However, the textarea at line 213 uses defaultValue={advancedJson}, which only sets the initial value. Subsequent state changes won't update the textarea content.
This means if the user keeps the advanced editor open while making changes in the visual editor, those changes won't be reflected in the JSON textarea until they close and reopen it.
If this is intentional (treating the advanced editor as a separate editing context), consider adding a UI hint. Otherwise, switch to a controlled component or reset advancedEditorInitialized when dag changes.
🤖 Prompt for AI Agents
In @packages/ui-default/components/training/TrainingEditor.tsx around lines 49 -
51, The textarea uses defaultValue={advancedJson}, so updates to advancedJson
(from React.useEffect watching showAdvanced and formData.dag) won't update the
textarea once mounted; either make the textarea controlled by replacing
defaultValue with value={advancedJson} and ensure onChange updates advancedJson,
or when you want to keep it uncontrolled reset advancedEditorInitialized
whenever formData.dag changes so the textarea is re-mounted with the new
defaultValue; target the React.useEffect that sets advancedJson, the
showAdvanced/formData.dag deps, the advancedJson state, the
advancedEditorInitialized flag, and the textarea currently using
defaultValue={advancedJson} to implement the chosen fix.
| const deleteSection = React.useCallback(async (nodeId: number) => { | ||
| if (!await confirm(i18n('Are you sure you want to delete this section?'))) return; | ||
| setFormData((prev) => ({ ...prev, dag: prev.dag.filter((node) => node._id !== nodeId) })); | ||
| }, []); |
There was a problem hiding this comment.
Orphaned prerequisite references after section deletion.
When a section is deleted, other sections may still reference its _id in their requireNids arrays. This could cause issues with cycle detection or UI display of prerequisites.
Consider cleaning up references:
🔧 Proposed fix
const deleteSection = React.useCallback(async (nodeId: number) => {
if (!await confirm(i18n('Are you sure you want to delete this section?'))) return;
- setFormData((prev) => ({ ...prev, dag: prev.dag.filter((node) => node._id !== nodeId) }));
+ setFormData((prev) => ({
+ ...prev,
+ dag: prev.dag
+ .filter((node) => node._id !== nodeId)
+ .map((node) => ({
+ ...node,
+ requireNids: node.requireNids.filter((nid) => nid !== nodeId),
+ })),
+ }));
}, []);📝 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 deleteSection = React.useCallback(async (nodeId: number) => { | |
| if (!await confirm(i18n('Are you sure you want to delete this section?'))) return; | |
| setFormData((prev) => ({ ...prev, dag: prev.dag.filter((node) => node._id !== nodeId) })); | |
| }, []); | |
| const deleteSection = React.useCallback(async (nodeId: number) => { | |
| if (!await confirm(i18n('Are you sure you want to delete this section?'))) return; | |
| setFormData((prev) => ({ | |
| ...prev, | |
| dag: prev.dag | |
| .filter((node) => node._id !== nodeId) | |
| .map((node) => ({ | |
| ...node, | |
| requireNids: node.requireNids.filter((nid) => nid !== nodeId), | |
| })), | |
| })); | |
| }, []); |
🤖 Prompt for AI Agents
In @packages/ui-default/components/training/TrainingEditor.tsx around lines 71 -
74, The deleteSection callback removes the node from dag but leaves its _id in
other nodes' requireNids, causing orphaned prerequisite references; update
deleteSection (the React.useCallback named deleteSection) to, after filtering
out the deleted node, also map the remaining dag nodes and remove nodeId from
each node.requireNids (e.g., set requireNids = requireNids.filter(id => id !==
nodeId) or clear to [] if none remain) so all references to the deleted section
are cleaned in the setFormData update.
| const handleApplyJson = React.useCallback(() => { | ||
| const jsonValue = advancedEditorRef.current?.value; | ||
| if (!jsonValue) return; | ||
| try { | ||
| const parsed = JSON.parse(jsonValue); | ||
| if (!Array.isArray(parsed)) { | ||
| Notification.error(i18n('Invalid JSON format: must be an array')); | ||
| return; | ||
| } | ||
| setFormData((prev) => ({ ...prev, dag: parsed })); | ||
| Notification.success(i18n('JSON applied to editor')); | ||
| } catch { | ||
| Notification.error(i18n('Invalid JSON format')); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
JSON validation is insufficient for runtime safety.
The validation only checks if the parsed JSON is an array, but doesn't validate the structure of each element. Invalid objects (missing _id, title, requireNids, or pids) could cause runtime errors in SectionItem components.
🔧 Proposed fix with schema validation
const handleApplyJson = React.useCallback(() => {
const jsonValue = advancedEditorRef.current?.value;
if (!jsonValue) return;
try {
const parsed = JSON.parse(jsonValue);
if (!Array.isArray(parsed)) {
Notification.error(i18n('Invalid JSON format: must be an array'));
return;
}
+ const isValidNode = (node: unknown): node is TrainingNode =>
+ typeof node === 'object' &&
+ node !== null &&
+ typeof (node as TrainingNode)._id === 'number' &&
+ typeof (node as TrainingNode).title === 'string' &&
+ Array.isArray((node as TrainingNode).requireNids) &&
+ Array.isArray((node as TrainingNode).pids);
+ if (!parsed.every(isValidNode)) {
+ Notification.error(i18n('Invalid JSON format: each section must have _id, title, requireNids, and pids'));
+ return;
+ }
setFormData((prev) => ({ ...prev, dag: parsed }));
Notification.success(i18n('JSON applied to editor'));
} catch {
Notification.error(i18n('Invalid JSON format'));
}
}, []);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/ui-default/components/training/TrainingEditor.tsx around lines 86 -
100, handleApplyJson currently only checks that parsed JSON is an array but
doesn't validate each item's shape, which can lead to runtime errors in
SectionItem; update handleApplyJson to iterate over parsed array and validate
required fields (_id, title, requireNids, pids) and their expected types (e.g.,
_id and title strings, requireNids/pids arrays) before calling setFormData,
collect and surface any validation errors via Notification.error, and only call
setFormData/Notification.success when all items pass validation; reference
advancedEditorRef for the source, parsed variable for the data, setFormData for
applying it, and ensure you bail out on validation failure to prevent invalid
state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui-default/templates/training_edit.html:
- Line 74: Replace the unsafe template interpolation for UiContext.dag so dag is
JSON-encoded before being marked safe; instead of rendering {{ dag|safe if dag
else '[]' }}, apply the JSON filter to the dag variable (e.g., use the
template's JSON/tojson filter) and then output that (still falling back to '[]'
when dag is falsy) so UiContext.dag receives properly encoded JSON and XSS risks
are mitigated.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui-default/pages/training_edit.page.tsxpackages/ui-default/templates/training_edit.html
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui-default/pages/training_edit.page.tsx
🔇 Additional comments (1)
packages/ui-default/templates/training_edit.html (1)
7-8: LGTM!The structural changes correctly establish mount points for the React-based TrainingEditor component and provide the necessary form wrapper.
| {% block script %} | ||
| <script> | ||
| UiContext.tdoc = {{ tdoc|json|safe if tdoc else 'null' }}; | ||
| UiContext.dag = {{ dag|safe if dag else '[]' }}; |
There was a problem hiding this comment.
Security: Add |json filter to prevent XSS.
Line 74 uses the safe filter without the |json filter, which bypasses HTML escaping and could lead to XSS if dag contains user-controlled content (problem IDs, training structure, etc.). The dag data should be properly JSON-encoded before rendering.
🔒 Proposed fix to add JSON encoding
- UiContext.dag = {{ dag|safe if dag else '[]' }};
+ UiContext.dag = {{ dag|json|safe if dag else '[]' }};📝 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.
| UiContext.dag = {{ dag|safe if dag else '[]' }}; | |
| UiContext.dag = {{ dag|json|safe if dag else '[]' }}; |
🤖 Prompt for AI Agents
In @packages/ui-default/templates/training_edit.html at line 74, Replace the
unsafe template interpolation for UiContext.dag so dag is JSON-encoded before
being marked safe; instead of rendering {{ dag|safe if dag else '[]' }}, apply
the JSON filter to the dag variable (e.g., use the template's JSON/tojson
filter) and then output that (still falling back to '[]' when dag is falsy) so
UiContext.dag receives properly encoded JSON and XSS risks are mitigated.
- Use controlled ProblemSelectAutoCompleteFC component in SectionItem - Use set(UiContext) pattern instead of script block in template - Parse dag and defaultDag from existing HTML elements
|
@undefined-moe addressed your PR review suggestions |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/ui-default/pages/training_edit.page.tsx:
- Around line 61-65: The redirect currently builds a relative path with
"./training/${res.tid}" which can resolve incorrectly; change the else branch
that sets window.location.href to use an absolute path like
`/training/${res.tid}` (or otherwise construct the full absolute URL) so that
when res.tid is present you do window.location.href = `/training/${res.tid}`
instead of the relative "./training/..." to avoid incorrect nesting.
- Line 34: The isEdit detection using window.location.pathname.includes('/edit')
can yield false positives; replace that check with a robust segment-based or
endpoint check—e.g., derive isEdit by splitting window.location.pathname and
confirming the last path segment equals 'edit' or by using a route/NamedPage
context if available (prefer using the NamedPage/context route name over string
checks); update the isEdit variable assignment (currently computed from
window.location.pathname.includes('/edit')) to use pathname.endsWith('/edit') or
the path-segment comparison or the NamedPage route name to avoid matching
substrings inside IDs or titles.
- Around line 72-85: The handleDelete function is brittle due to pathname regex
parsing; instead add an optional tid?: string to the UiContext declaration
(alongside canDelete) and update handleDelete to use UiContext.tid directly
(e.g., post to /training/{UiContext.tid} with the same payload), removing the
regex matching and baseUrl computation; keep the success/error notifications and
redirect logic (use res.url || '/training') unchanged and throw a clear error if
UiContext.tid is missing.
🧹 Nitpick comments (3)
packages/ui-default/pages/training_edit.page.tsx (3)
18-25: Consider logging parse errors in development mode.The
parseDagfunction silently swallows JSON parsing errors, which could hide issues with malformed data during development.📝 Optional enhancement: add development logging
function parseDag(dagStr: string | undefined, defaultDag: TrainingNode[]): TrainingNode[] { if (!dagStr) return defaultDag; try { return JSON.parse(dagStr); } catch { + if (process.env.NODE_ENV !== 'production') { + console.warn('Failed to parse DAG, using default:', dagStr); + } return defaultDag; } }
36-48: Consider clarifying the DAG initialization logic.The ternary expression on line 47 is correct but somewhat complex. A brief comment or intermediate variable would improve readability.
♻️ Optional clarity improvement
+ // In create mode, prefer non-empty dag over defaultDag; in edit mode, always use dag const initialData: TrainingFormData = { title: UiContext.tdoc?.title || '', content: UiContext.tdoc?.content || '', description: UiContext.tdoc?.description || '', pin: UiContext.tdoc?.pin || 0, dag: isEdit ? dag : (dag.length > 0 ? dag : defaultDag), };
66-69: Consider typing the error more precisely.Using
anyfor the error type bypasses type safety. Consider usingunknownand type-narrowing instead.♻️ Optional type safety improvement
- } catch (error: any) { - Notification.error(error.message || i18n('Failed to save training')); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : i18n('Failed to save training'); + Notification.error(message); throw error; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ui-default/components/training/SectionItem.tsxpackages/ui-default/pages/training_edit.page.tsxpackages/ui-default/templates/training_edit.html
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui-default/templates/training_edit.html
- packages/ui-default/components/training/SectionItem.tsx
🔇 Additional comments (4)
packages/ui-default/pages/training_edit.page.tsx (4)
1-6: LGTM!Import statements are clean and all dependencies are used appropriately. The React 19 import pattern using
createRootfrom 'react-dom/client' is correct.
8-16: LGTM!The
UiContexttype declaration appropriately models the server-provided context for both create and edit modes.
87-95: LGTM!React rendering logic is correctly implemented using React 19's
createRootAPI. Props are appropriately passed, and the conditionalonDeletehandler follows the correct pattern for edit vs. create modes.
27-27: LGTM!The page structure correctly uses the
NamedPagepattern with appropriate route registration for both create and edit modes.Also applies to: 98-98
|
|
||
| if (fallbackForm) fallbackForm.style.display = 'none'; | ||
|
|
||
| const isEdit = window.location.pathname.includes('/edit'); |
There was a problem hiding this comment.
Use more robust edit mode detection.
The current implementation uses pathname.includes('/edit'), which could produce false positives if a training ID or title contains the substring 'edit' (e.g., /training/reddit/edit or /training/my-edited-notes/edit).
🔧 Recommended fix: check path segments or route name
Option 1: Check the actual route name from the NamedPage context:
- const isEdit = window.location.pathname.includes('/edit');
+ const isEdit = page.name === 'training_edit';Option 2: Check if pathname ends with '/edit':
- const isEdit = window.location.pathname.includes('/edit');
+ const isEdit = window.location.pathname.endsWith('/edit');Option 3: Add to UiContext (requires backend change):
declare const UiContext: {
tdoc?: { ... };
canDelete: boolean;
+ isEdit: boolean;
};🤖 Prompt for AI Agents
In @packages/ui-default/pages/training_edit.page.tsx at line 34, The isEdit
detection using window.location.pathname.includes('/edit') can yield false
positives; replace that check with a robust segment-based or endpoint
check—e.g., derive isEdit by splitting window.location.pathname and confirming
the last path segment equals 'edit' or by using a route/NamedPage context if
available (prefer using the NamedPage/context route name over string checks);
update the isEdit variable assignment (currently computed from
window.location.pathname.includes('/edit')) to use pathname.endsWith('/edit') or
the path-segment comparison or the NamedPage route name to avoid matching
substrings inside IDs or titles.
| if (res.url) { | ||
| window.location.href = res.url; | ||
| } else if (res.tid) { | ||
| window.location.href = `./training/${res.tid}`; | ||
| } |
There was a problem hiding this comment.
Fix relative URL construction that may produce incorrect paths.
Line 64 uses ./training/${res.tid} which constructs a path relative to the current location. This will fail when the current path has multiple segments.
For example:
- Current path:
/domain/123/training/create - Redirect:
./training/456 - Resolves to:
/domain/123/training/training/456❌
🔧 Recommended fix: use absolute path construction
if (res.url) {
window.location.href = res.url;
} else if (res.tid) {
- window.location.href = `./training/${res.tid}`;
+ const baseUrl = window.location.pathname.replace(/\/training\/.*$/, '');
+ window.location.href = `${baseUrl}/training/${res.tid}`;
}📝 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.
| if (res.url) { | |
| window.location.href = res.url; | |
| } else if (res.tid) { | |
| window.location.href = `./training/${res.tid}`; | |
| } | |
| if (res.url) { | |
| window.location.href = res.url; | |
| } else if (res.tid) { | |
| const baseUrl = window.location.pathname.replace(/\/training\/.*$/, ''); | |
| window.location.href = `${baseUrl}/training/${res.tid}`; | |
| } |
🤖 Prompt for AI Agents
In @packages/ui-default/pages/training_edit.page.tsx around lines 61 - 65, The
redirect currently builds a relative path with "./training/${res.tid}" which can
resolve incorrectly; change the else branch that sets window.location.href to
use an absolute path like `/training/${res.tid}` (or otherwise construct the
full absolute URL) so that when res.tid is present you do window.location.href =
`/training/${res.tid}` instead of the relative "./training/..." to avoid
incorrect nesting.
| const handleDelete = async () => { | ||
| try { | ||
| const path = window.location.pathname; | ||
| const tidMatch = path.match(/\/training\/([^/]+)\/edit/); | ||
| if (!tidMatch) throw new Error('Training ID not found'); | ||
| const tid = tidMatch[1]; | ||
| const baseUrl = path.replace(/\/training\/[^/]+\/edit.*$/, ''); | ||
| const res = await request.post(`${baseUrl}/training/${tid}`, { operation: 'delete' }); | ||
| Notification.success(i18n('Training deleted successfully')); | ||
| window.location.href = res.url || `${baseUrl}/training`; | ||
| } catch (error: any) { | ||
| Notification.error(error.message || i18n('Failed to delete training')); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the file
find . -name "training_edit.page.tsx" -type fRepository: hydro-dev/Hydro
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Check repository structure
git ls-files | grep -E "training|Training" | head -20Repository: hydro-dev/Hydro
Length of output: 994
🏁 Script executed:
#!/bin/bash
# Search for TrainingEditor component
rg -l "TrainingEditor" --type=tsx --type=ts | head -10Repository: hydro-dev/Hydro
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Read the training_edit.page.tsx file to see the handleDelete function
cat -n ./packages/ui-default/pages/training_edit.page.tsx | sed -n '60,100p'Repository: hydro-dev/Hydro
Length of output: 1652
🏁 Script executed:
#!/bin/bash
# Read TrainingEditor component
cat -n ./packages/ui-default/components/training/TrainingEditor.tsx | head -100Repository: hydro-dev/Hydro
Length of output: 4730
🏁 Script executed:
#!/bin/bash
# Search for confirmation dialogs and onDelete patterns
rg "onDelete|confirm|Confirmation" -i ./packages/ui-default/components/training/ -A 3 -B 1Repository: hydro-dev/Hydro
Length of output: 5053
🏁 Script executed:
#!/bin/bash
# Search for UiContext usage and training ID
rg "UiContext|initialData" ./packages/ui-default/pages/training_edit.page.tsx -B 2 -A 2Repository: hydro-dev/Hydro
Length of output: 732
🏁 Script executed:
#!/bin/bash
# Check the full training_edit.page.tsx file to see what initialData contains
cat -n ./packages/ui-default/pages/training_edit.page.tsx | head -75Repository: hydro-dev/Hydro
Length of output: 3112
🏁 Script executed:
#!/bin/bash
# Look for types to understand TrainingFormData structure
rg -A 10 "interface TrainingFormData|type TrainingFormData" ./packages/ui-default/components/training/types.tsRepository: hydro-dev/Hydro
Length of output: 414
🏁 Script executed:
#!/bin/bash
# Check the training handler to see how delete endpoint works
cat -n ./packages/hydrooj/src/handler/training.ts | head -100Repository: hydro-dev/Hydro
Length of output: 4831
🏁 Script executed:
#!/bin/bash
# Search for delete operation handling
rg "delete|operation" ./packages/hydrooj/src/handler/training.ts -B 2 -A 3 | head -50Repository: hydro-dev/Hydro
Length of output: 316
🏁 Script executed:
#!/bin/bash
# Search more specifically for delete operation in training handler
rg -n "post|DELETE|delete|operation" ./packages/hydrooj/src/handler/training.ts | head -30Repository: hydro-dev/Hydro
Length of output: 565
🏁 Script executed:
#!/bin/bash
# Get the full training handler file to understand the API structure
wc -l ./packages/hydrooj/src/handler/training.tsRepository: hydro-dev/Hydro
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Look at the postDelete method
cat -n ./packages/hydrooj/src/handler/training.ts | sed -n '175,190p'Repository: hydro-dev/Hydro
Length of output: 779
🏁 Script executed:
#!/bin/bash
# Also check the post method to see how it handles operations
cat -n ./packages/hydrooj/src/handler/training.ts | sed -n '213,270p'Repository: hydro-dev/Hydro
Length of output: 3032
🏁 Script executed:
#!/bin/bash
# Verify the exact endpoint structure by checking how delete is called
rg -n "training_delete|/delete" ./packages/ui-default/pages/training_edit.page.tsx -B 2 -A 2Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if there are any other patterns for delete in the codebase
rg "operation.*delete|postDelete" ./packages/hydrooj/src/handler/training.ts -B 2 -A 2Repository: hydro-dev/Hydro
Length of output: 567
🏁 Script executed:
#!/bin/bash
# Check the server routing pattern to understand how postDelete maps to endpoint
rg -n "class TrainingEditHandler|@post|postDelete" ./packages/hydrooj/src/handler/training.ts -B 1 -A 8 | head -60Repository: hydro-dev/Hydro
Length of output: 2144
🏁 Script executed:
#!/bin/bash
# Verify the actual endpoint by checking URL patterns
rg "training.*delete" ./packages/hydrooj/src/handler/training.ts -iRepository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the server decorator implementation to understand routing
rg -n "export.*TrainingEditHandler|export.*TrainingDetailHandler" ./packages/hydrooj/src/handler/training.ts -A 2Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check if the operation parameter is handled elsewhere or if there's a bug
rg "operation" ./packages/hydrooj/src/handler/training.tsRepository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look at the complete TrainingDetailHandler class to see postDelete context
cat -n ./packages/hydrooj/src/handler/training.ts | sed -n '99,184p'Repository: hydro-dev/Hydro
Length of output: 4891
🏁 Script executed:
#!/bin/bash
# Check how request.post handles operation/action parameters
rg -n "request\.post|operation|action" ./packages/ui-default/pages/training_edit.page.tsx | head -20Repository: hydro-dev/Hydro
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Look at similar patterns in other files for POST with operation
rg "request\.post.*operation|request\.post.*action" --type=tsx --type=ts -B 2 -A 2 | head -40Repository: hydro-dev/Hydro
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Check if there's a DELETE endpoint or if postDelete is routed via operation parameter
rg -rn "postDelete|delete.*route" ./packages/hydrooj/src/service/ | grep -E "training|handler|route" | head -20Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the server framework to understand method routing
rg -n "async post\|function post" ./packages/hydrooj/src/service/server.ts | head -10Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Look for how postDelete method name maps to routes
fd "server.ts" --type f -x head -100 {}Repository: hydro-dev/Hydro
Length of output: 7255
🏁 Script executed:
#!/bin/bash
# Try to understand how postDelete is actually called
# Check if there's a /delete route or if operation parameter routing exists
rg "operation.*delete\|_delete\|/delete" ./packages/hydrooj/src/handler/training.tsRepository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the framework's router to understand how postXxx methods map to endpoints
grep -n "postDelete\|route\|/delete" ./packages/hydrooj/src/handler/training.tsRepository: hydro-dev/Hydro
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Test the actual endpoint path by checking how other delete operations work
rg -B5 -A5 "async postDelete" ./packages/hydrooj/src/handler/*.ts | head -40Repository: hydro-dev/Hydro
Length of output: 3101
🏁 Script executed:
#!/bin/bash
# Check if there are examples of how to properly call delete in other parts of the codebase
rg "request\.post.*tid\|request\.delete" ./packages/ui-default -A 2 -B 2 | head -50Repository: hydro-dev/Hydro
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Verify the actual endpoint by checking the router configuration
rg "training.*route|/training" ./packages/hydrooj/src -A 1 | grep -E "Handler|route|/training" | head -30Repository: hydro-dev/Hydro
Length of output: 1728
🏁 Script executed:
#!/bin/bash
# Check if TrainingDetailHandler actually has a handler that processes requests
grep -n "class TrainingDetailHandler\|async post\|async get\|@param" ./packages/hydrooj/src/handler/training.ts | sed -n '8,35p'Repository: hydro-dev/Hydro
Length of output: 1176
🏁 Script executed:
#!/bin/bash
# Verify which handler is responsible for the /training/:tid route
grep -n "Route.*training" ./packages/hydrooj/src/handler/training.tsRepository: hydro-dev/Hydro
Length of output: 655
🏁 Script executed:
#!/bin/bash
# Final verification: Check what happens when operation parameter is passed
# Check if TrainingDetailHandler.postDelete ignores extra parameters
cat -n ./packages/hydrooj/src/handler/training.ts | sed -n '174,184p'Repository: hydro-dev/Hydro
Length of output: 616
Simplify URL parsing by storing the training ID in context instead of extracting from pathname.
The current implementation uses regex to extract the training ID and base URL from the pathname, which is fragile and will break if:
- URL structure changes
- Query parameters are present
- Additional path segments exist
The confirmation dialog is already shown by the TrainingEditor component before calling onDelete, so no changes needed there.
Recommended fix: Store the training ID in UiContext during page initialization (similar to how canDelete is already passed), then use it directly instead of parsing from the pathname:
// In training_edit.page.tsx - modify UiContext declaration
declare const UiContext: {
tdoc?: { /* ... */ };
tid?: string; // Add this
canDelete: boolean;
};
// In handleDelete
const res = await request.post(`/training/${UiContext.tid}`, {});This eliminates regex fragility while making the code clearer and more maintainable.
🤖 Prompt for AI Agents
In @packages/ui-default/pages/training_edit.page.tsx around lines 72 - 85, The
handleDelete function is brittle due to pathname regex parsing; instead add an
optional tid?: string to the UiContext declaration (alongside canDelete) and
update handleDelete to use UiContext.tid directly (e.g., post to
/training/{UiContext.tid} with the same payload), removing the regex matching
and baseUrl computation; keep the success/error notifications and redirect logic
(use res.url || '/training') unchanged and throw a clear error if UiContext.tid
is missing.
| dag: isEdit ? dag : (dag.length > 0 ? dag : defaultDag), | ||
| }; | ||
|
|
||
| const handleSubmit = async (data: TrainingFormData) => { |
There was a problem hiding this comment.
use existing form element to handle submit. hide the textbox and mount an editor that syncs all the changes to the original text field like other rich text editors do so that you don't need to re-implement these logic.
| ); | ||
|
|
||
| return ( | ||
| <div className="training-section" style={{ border: '1px solid #ddd', borderRadius: '4px', marginBottom: '12px', background: '#fff' }}> |
There was a problem hiding this comment.
put these styles into a seperate file, e.g. training.page.styl
2 participants
Summary
Changes
New Features
Visual Training Editor
i18n
Added translations for all new UI strings in:
Screenshots
Testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.