Conversation
|
Visit the preview URL for this PR (updated for commit dece4ac): https://extendafamily-7613e--pr169-jimmy-finish-module-3qyhxqcb.web.app (expires Thu, 08 Jan 2026 02:46:38 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f8bb7b5cd838ab636347dc54fd0ab08ab3715d31 |
ji-mmyliu
force-pushed
the
jimmy/finish-module-editor
branch
from
0d5ae20 to
43ac855
Compare
ji-mmyliu
force-pushed
the
jimmy/finish-module-editor
branch
from
43ac855 to
7f31359
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive module editor UI that enables administrators to manage course modules through drag-and-drop reordering, context menus for page manipulation, and an empty state for new modules. The changes span both frontend and backend, introducing new APIs for page deletion, reordering, and enhanced lesson uploads with insertion at specific indices.
Key Changes:
- Added drag-and-drop page reordering with visual feedback
- Implemented context menu for inserting pages (above/below), creating activities, and deleting pages
- Created empty module editing state with initial page creation options
- Backend support for page reordering, deletion, and lesson uploads at specific positions
- Migrated from single PDF URL per module to individual PDF URLs per lesson page
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types/CourseTypes.ts | Changed QuestionType from "Custom" to "Input" and added pdfUrl to LessonPage |
| frontend/src/constants/ActivityLabels.tsx | New file defining icons and labels for each question type |
| frontend/src/components/pages/ViewModulePage.tsx | Major refactor adding drag-and-drop, context menus, and empty module state |
| frontend/src/components/pages/DeletePageModal.tsx | New modal component for confirming page deletion |
| frontend/src/components/help/modal/types.ts | Removed lessonPdfUrl from module type |
| frontend/src/components/courses/moduleViewing/Thumbnail.tsx | Added drag-and-drop props and context menu support |
| frontend/src/components/course_authoring/EmptyModuleEditing.tsx | New component for empty module state with page creation options |
| frontend/src/components/bookmarks/*.tsx | Updated to use individual PDF URLs per page instead of module-level URL |
| frontend/src/APIClients/CourseAPIClient.ts | Added deletePage, reorderPages methods; updated lessonUpload signature |
| frontend/src/APIClients/ActivityAPIClient.ts | Added createActivity method |
| backend/types/courseTypes.ts | Removed lessonPdfUrl from CourseModuleDTO, added pdfUrl to LessonPageDTO |
| backend/services/interfaces/*.ts | Updated interfaces for new uploadFile, deletePage, and reorderPages methods |
| backend/services/implementations/fileStorageService.ts | Added uploadFile method with overwrite support |
| backend/services/implementations/coursePageService.ts | Enhanced deletePage with transaction and module update |
| backend/services/implementations/courseModuleService.ts | Updated uploadLessons for insertIdx support, added reorderPages |
| backend/services/implementations/activityService.ts | Updated createActivity to return full module and support position index |
| backend/scripts/update-module-pages-pdf.ts | Migration script to add pdfUrl to existing pages |
| backend/rest/courseRoutes.ts | Added DELETE and PATCH endpoints for page operations |
| backend/models/coursepage.mgmodel.ts | Added pdfUrl field to LessonPage schema |
| backend/models/coursemodule.mgmodel.ts | Updated status type to use ModuleStatus enum |
| backend/models/activity.mgmodel.ts | Made Media context field optional |
| backend/middlewares/validators/courseValidators.ts | Enhanced pageBelongsToModuleValidator for hydrated/lean results |
Comments suppressed due to low confidence (1)
backend/rest/courseRoutes.ts:144
- The file upload endpoint lacks validation for the uploaded file. There's no check for file type (should be PDF), file size limits, or whether moduleId is provided. Malicious users could upload non-PDF files or excessively large files. Add validation to ensure the file is a valid PDF and has a reasonable size limit before processing.
courseRouter.post(
"/uploadLessons",
upload.single("lessonPdf"),
isAuthorizedByRole(new Set(["Administrator"])),
async (req, res) => {
try {
const {
file: lessonPdf,
body: { moduleId, insertIdx },
} = req;
if (!lessonPdf) {
throw new Error("No lessonPdf file uploaded.");
}
const result = await courseModuleService.uploadLessons(
moduleId,
lessonPdf.buffer,
insertIdx ? parseInt(insertIdx, 10) : undefined,
);
res.status(200).json(result);
} catch (e: unknown) {
res.status(500).send(getErrorMessage(e));
}
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!files || files.length === 0) { | ||
| // eslint-disable-next-line no-alert | ||
| alert("Error: No file selected to upload"); | ||
| return; | ||
| } | ||
| const file = files[0]; | ||
| CourseAPIClient.lessonUpload(file, moduleId) | ||
| .then(refreshModule) | ||
| .catch((error) => { | ||
| /* eslint-disable-next-line no-console */ | ||
| console.error("Failed to upload lesson page:", error); | ||
| /* eslint-disable-next-line no-alert */ | ||
| alert( | ||
| `Failed to upload lesson page, please try again. If the issue persists, please contact us.`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Using the native alert() function for error handling is not a good user experience pattern in modern web applications. Consider using a toast notification system, snackbar, or custom error modal for more professional and less intrusive error messaging. The alert also blocks user interaction until dismissed.
| Table = "Table", | ||
| Matching = "Matching", | ||
| Custom = "Custom", | ||
| Input = "Input", |
There was a problem hiding this comment.
There's a naming inconsistency between frontend and backend for the question type. The frontend uses QuestionType.Input (in CourseTypes.ts) while the backend uses QuestionType.TextInput (in activityTypes.ts). This mismatch will cause the activity creation to fail when users try to create a "Short Answer" activity from the UI, as the backend won't recognize "Input" as a valid question type.
| Input = "Input", | |
| Input = "TextInput", |
| export const questionTypeIcons: Record<QuestionType, React.ReactNode> = { | ||
| [QuestionType.MultipleChoice]: <AccountTreeOutlined />, | ||
| [QuestionType.MultiSelect]: <AccountTreeOutlined />, | ||
| [QuestionType.Table]: <TableChartOutlined />, | ||
| [QuestionType.Matching]: <AccountTreeOutlined />, | ||
| [QuestionType.Input]: <Subject />, | ||
| }; | ||
|
|
||
| export const questionTypeLabels: Record<QuestionType, string> = { | ||
| [QuestionType.MultipleChoice]: "Multiple Choice", | ||
| [QuestionType.MultiSelect]: "Multi Select", | ||
| [QuestionType.Table]: "Table", | ||
| [QuestionType.Matching]: "Matching", | ||
| [QuestionType.Input]: "Short Answer", |
There was a problem hiding this comment.
The frontend code enables users to create "Input" (Short Answer) activities via the UI, but the backend activityService.ts will reject this question type because it's not implemented in the activityModelMapper. This will result in an "Unsupported question type" error when users attempt to create this activity type. Either remove this option from the frontend UI (ActivityLabels.tsx) until the backend implementation is complete, or ensure the backend has the necessary model and handler for TextInput activities.
| export const questionTypeIcons: Record<QuestionType, React.ReactNode> = { | |
| [QuestionType.MultipleChoice]: <AccountTreeOutlined />, | |
| [QuestionType.MultiSelect]: <AccountTreeOutlined />, | |
| [QuestionType.Table]: <TableChartOutlined />, | |
| [QuestionType.Matching]: <AccountTreeOutlined />, | |
| [QuestionType.Input]: <Subject />, | |
| }; | |
| export const questionTypeLabels: Record<QuestionType, string> = { | |
| [QuestionType.MultipleChoice]: "Multiple Choice", | |
| [QuestionType.MultiSelect]: "Multi Select", | |
| [QuestionType.Table]: "Table", | |
| [QuestionType.Matching]: "Matching", | |
| [QuestionType.Input]: "Short Answer", | |
| export const questionTypeIcons: Partial<Record<QuestionType, React.ReactNode>> = { | |
| [QuestionType.MultipleChoice]: <AccountTreeOutlined />, | |
| [QuestionType.MultiSelect]: <AccountTreeOutlined />, | |
| [QuestionType.Table]: <TableChartOutlined />, | |
| [QuestionType.Matching]: <AccountTreeOutlined />, | |
| }; | |
| export const questionTypeLabels: Partial<Record<QuestionType, string>> = { | |
| [QuestionType.MultipleChoice]: "Multiple Choice", | |
| [QuestionType.MultiSelect]: "Multi Select", | |
| [QuestionType.Table]: "Table", | |
| [QuestionType.Matching]: "Matching", |
| <VisuallyHiddenInput | ||
| type="file" | ||
| onChange={handleFileChange} | ||
| multiple |
There was a problem hiding this comment.
The file upload input accepts any file type (no accept attribute is specified on the VisuallyHiddenInput). While the UI text suggests PDF uploads, users could select and attempt to upload any file type. Add an accept=".pdf" attribute to the file input to restrict file selection to PDF files only, similar to how it's done in the context menu handlers in ViewModulePage.tsx.
| multiple | |
| multiple | |
| accept=".pdf" |
| async reorderPages( | ||
| moduleId: string, | ||
| fromIndex: number, | ||
| toIndex: number, | ||
| ): Promise<CourseModuleDTO> { | ||
| try { | ||
| const courseModule: CourseModule | null = await MgCourseModule.findById( | ||
| moduleId, | ||
| ); | ||
|
|
||
| if (!courseModule) { | ||
| throw new Error(`Course module with id ${moduleId} not found.`); | ||
| } | ||
|
|
||
| // Validate indices | ||
| if ( | ||
| fromIndex < 0 || | ||
| fromIndex >= courseModule.pages.length || | ||
| toIndex < 0 || | ||
| toIndex >= courseModule.pages.length | ||
| ) { | ||
| throw new Error( | ||
| `Invalid indices: fromIndex=${fromIndex}, toIndex=${toIndex}, pages.length=${courseModule.pages.length}`, | ||
| ); | ||
| } | ||
|
|
||
| // If indices are the same, no reordering needed | ||
| if (fromIndex === toIndex) { | ||
| const result = await this.getCourseModule(moduleId); | ||
| return result as CourseModuleDTO; | ||
| } | ||
|
|
||
| // Reorder pages array | ||
| const pages = [...courseModule.pages]; | ||
| const [movedPage] = pages.splice(fromIndex, 1); | ||
| pages.splice(toIndex, 0, movedPage); | ||
|
|
||
| // Update module with reordered pages | ||
| const updatedModule = await MgCourseModule.findByIdAndUpdate( | ||
| moduleId, | ||
| { pages }, | ||
| { new: true }, | ||
| ); | ||
|
|
||
| if (!updatedModule) { | ||
| throw new Error( | ||
| `Failed to update course module with id ${moduleId} after reordering`, | ||
| ); | ||
| } | ||
|
|
||
| // Return the fully populated module | ||
| const result = await this.getCourseModule(moduleId); | ||
| return result as CourseModuleDTO; | ||
| } catch (error) { | ||
| Logger.error( | ||
| `Failed to reorder pages for module ${moduleId}. Reason = ${getErrorMessage( | ||
| error, | ||
| )}`, | ||
| ); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
The reorderPages method lacks transaction handling despite making database mutations. Unlike other methods in this service (uploadLessons, changeStatus, deleteCoursePage), this method doesn't use a MongoDB session/transaction. If the operation fails partway through, it could leave the database in an inconsistent state. Consider wrapping this method in a transaction for data integrity.
| if ( | ||
| typeof fromIndex !== "number" || | ||
| typeof toIndex !== "number" || | ||
| fromIndex < 0 || | ||
| toIndex < 0 | ||
| ) { | ||
| res.status(400).send("Invalid fromIndex or toIndex"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The validation for fromIndex and toIndex only checks if they are non-negative numbers, but doesn't validate if they are within the bounds of the module's pages array. This could lead to errors when the service method validates the indices. Consider adding a check that validates the indices against the module's page count, or ensure the error handling properly communicates this to the client.
| if (typeof page === "string") return page; | ||
| if (typeof page === "object" && page !== null) { | ||
| // @ts-expect-error allow fallback to _id | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| return (page.id || page._id || "").toString(); | ||
| } |
There was a problem hiding this comment.
The validator is suppressing TypeScript errors with @ts-expect-error and has complex logic to handle both hydrated and lean MongoDB results. This indicates inconsistent data handling patterns. The type system should be able to properly represent the different data shapes. Consider defining proper TypeScript types or interfaces for the different forms of data (hydrated vs lean) and using type guards instead of suppressing errors.
| if (typeof page === "string") return page; | |
| if (typeof page === "object" && page !== null) { | |
| // @ts-expect-error allow fallback to _id | |
| // eslint-disable-next-line no-underscore-dangle | |
| return (page.id || page._id || "").toString(); | |
| } | |
| if (typeof page === "string") { | |
| return page; | |
| } | |
| if (typeof page === "object" && page !== null) { | |
| // Narrow to an object that may have id or _id properties | |
| const candidate = page as { id?: unknown; _id?: unknown }; | |
| const value = candidate.id ?? candidate._id; | |
| return value != null ? String(value) : ""; | |
| } |
| const handleUploadPdfAbove = () => { | ||
| if (contextMenu === null) return; | ||
| const input = document.createElement("input"); | ||
| input.type = "file"; | ||
| input.accept = ".pdf"; | ||
| input.onchange = async (e) => { | ||
| const file = (e.target as HTMLInputElement).files?.[0]; | ||
| if (file && module) { | ||
| try { | ||
| const updatedModule = await CourseAPIClient.lessonUpload( | ||
| file, | ||
| module.id, | ||
| contextMenu.pageIndex, | ||
| ); | ||
| setModule(updatedModule); | ||
| } catch (error) { | ||
| /* eslint-disable-next-line no-console */ | ||
| console.error("Failed to upload PDF:", error); | ||
| } | ||
| } | ||
| }; | ||
| input.click(); | ||
| handleCloseContextMenu(); | ||
| }; | ||
|
|
||
| const handleUploadPdfBelow = () => { | ||
| if (contextMenu === null) return; | ||
| const input = document.createElement("input"); | ||
| input.type = "file"; | ||
| input.accept = ".pdf"; | ||
| input.onchange = async (e) => { | ||
| const file = (e.target as HTMLInputElement).files?.[0]; | ||
| if (file && module) { | ||
| try { | ||
| const updatedModule = await CourseAPIClient.lessonUpload( | ||
| file, | ||
| module.id, | ||
| contextMenu.pageIndex + 1, | ||
| ); | ||
| setModule(updatedModule); | ||
| } catch (error) { | ||
| /* eslint-disable-next-line no-console */ | ||
| console.error("Failed to upload PDF:", error); | ||
| } | ||
| } | ||
| }; | ||
| input.click(); | ||
| handleCloseContextMenu(); | ||
| }; | ||
|
|
||
| const handleCreateActivity = (event: React.MouseEvent<HTMLElement>) => { | ||
| if (contextMenu === null) return; | ||
| setSelectedPageIndexForActivity(contextMenu.pageIndex); | ||
| setActivityMenuAnchor(event.currentTarget); | ||
| handleCloseContextMenu(); | ||
| }; | ||
|
|
||
| const handleActivityTypeSelect = async (questionType: QuestionType) => { | ||
| if (selectedPageIndexForActivity === null || !module) return; | ||
| try { | ||
| const updatedModule = await ActivityAPIClient.createActivity( | ||
| module.id, | ||
| questionType, | ||
| selectedPageIndexForActivity + 1, | ||
| ); | ||
| if (updatedModule) { | ||
| setModule(updatedModule); | ||
| } | ||
| } catch (error) { | ||
| /* eslint-disable-next-line no-console */ | ||
| console.error("Failed to create activity:", error); | ||
| } | ||
| setActivityMenuAnchor(null); | ||
| setSelectedPageIndexForActivity(null); | ||
| }; | ||
|
|
||
| const handleDeletePageFromContext = async () => { | ||
| if (contextMenu === null || !module) return; | ||
| const pageToDelete = module.pages[contextMenu.pageIndex]; | ||
| if (!pageToDelete) return; | ||
|
|
||
| try { | ||
| const deletedPageId = await CourseAPIClient.deletePage( | ||
| module.id, | ||
| pageToDelete.id, | ||
| ); | ||
|
|
||
| if (deletedPageId) { | ||
| const refreshedModule = await fetchModule(); | ||
| setModule(refreshedModule); | ||
|
|
||
| setCurrentPage((prevPage) => { | ||
| const updatedPagesLength = refreshedModule?.pages.length || 0; | ||
| if (updatedPagesLength === 0) return 0; | ||
| return Math.min(prevPage, updatedPagesLength - 1); | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| /* eslint-disable-next-line no-console */ | ||
| console.error("Failed to delete page:", error); | ||
| } | ||
| handleCloseContextMenu(); |
There was a problem hiding this comment.
Error handling only logs to console without providing user feedback. When PDF upload, activity creation, or page deletion fails, users receive no visual indication of the failure. Consider adding toast notifications, error modals, or other UI feedback to inform users when these operations fail, similar to the pattern used in the DeletePageModal with loading states.
| /** | ||
| * Uploads a PDF file and creates lesson pages for each page in the PDF | ||
| * @param moduleId the id of the module to add the lessons to | ||
| * @param pdfPath the path to the temporary uploaded PDF file | ||
| * @param pdfBuffer the buffer of the uploaded PDF file | ||
| * @returns Updated course module | ||
| * @throws Error if upload fails or module not found | ||
| */ | ||
| uploadLessons(moduleId: string, pdfPath: string): Promise<CourseModuleDTO>; | ||
| uploadLessons(moduleId: string, pdfBuffer: Buffer): Promise<CourseModuleDTO>; |
There was a problem hiding this comment.
The interface documentation doesn't match the implementation. The interface documentation only mentions pdfBuffer parameter, but the actual implementation in courseModuleService.ts also accepts an optional insertIdx parameter. Update the interface documentation to include the insertIdx parameter and its purpose.
| ), | ||
| [ | ||
| currentPage, | ||
| theme.palette.Neutral, |
There was a problem hiding this comment.
The useMemo dependency array includes theme.palette.Neutral but the code uses theme.palette.Neutral[200] in the component. This means the memo will only recompute if the entire palette.Neutral object changes, not when specific values like Neutral[200] change. Consider adding the specific theme values that are actually used, or use the entire theme object as a dependency.
| theme.palette.Neutral, | |
| theme.palette.Neutral[200], |
ji-mmyliu
force-pushed
the
jimmy/finish-module-editor
branch
from
7f31359 to
dece4ac
Compare
1 participant
Notion ticket link
Empty Module Editing UI
Editing Module Pages
Implementation description
Steps to test
What should reviewers focus on?
Checklist