#3767 onboarding impreovements #3863
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements onboarding improvements to enhance the user experience for new members, addressing issue #3767. The changes introduce the ability to display both checkboxes and information blocks in checklists, add drag-and-drop reordering functionality for admin users, implement optimistic UI updates for checkbox interactions, and add support for a new member events image.
Key Changes:
- Added support for checklist items to be either TASK (checkbox) or INFO (information block) types with a new
itemTypefield anddisplayOrderfield for custom ordering - Implemented optimistic UI updates when toggling checkboxes to eliminate perceived lag
- Added drag-and-drop functionality for admins to reorder tasks and subtasks/info blocks
- Added support for uploading and displaying a "New Member Events" image on the onboarding page
- Made parent task checkboxes non-interactive (disabled) since they auto-check when all subtasks are complete
- Updated all links in markdown to open in new tabs
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/src/types/checklist-types.ts | Added ChecklistItemType enum (TASK/INFO) and displayOrder field to Checklist type |
| src/shared/src/types/user-types.ts | Added newMemberImageId to Organization type |
| src/backend/src/prisma/schema.prisma | Added displayOrder, itemType, and newMemberImageId fields to database schema |
| src/backend/src/prisma/migrations/20260106000000_add_checklist_ordering_and_type/migration.sql | Migration to add new fields with backfill logic for existing data |
| src/backend/src/services/onboarding.services.ts | Added reorderTasks and reorderChecklistItems methods, updated queries to order by displayOrder, filtered INFO blocks from validation logic |
| src/backend/src/services/organizations.services.ts | Added setNewMemberImage and getNewMemberImage methods |
| src/backend/src/controllers/onboarding.controllers.ts | Added controller methods for reordering and updated create/edit to handle itemType |
| src/backend/src/controllers/organizations.controllers.ts | Added controller methods for new member image upload/retrieval |
| src/backend/src/routes/onboarding.routes.ts | Added routes for task and checklist item reordering |
| src/backend/src/routes/organizations.routes.ts | Added routes for new member image operations |
| src/backend/src/transformers/organizationTransformer.ts | Updated to include image IDs in organization preview |
| src/frontend/src/apis/onboarding.api.ts | Added API functions for reordering tasks and checklist items |
| src/frontend/src/apis/organizations.api.ts | Added API functions for new member image operations |
| src/frontend/src/utils/urls.ts | Added URL definitions for new endpoints |
| src/frontend/src/hooks/onboarding.hook.ts | Implemented optimistic updates for checkbox toggling and mutations for reordering |
| src/frontend/src/hooks/organizations.hooks.ts | Added hooks for new member image upload and retrieval |
| src/frontend/src/components/NERMarkdown.tsx | Updated to make all links open in new tabs |
| src/frontend/src/pages/HomePage/components/SubtaskSection.tsx | Refactored to display both TASK and INFO items in a single list with backward compatibility |
| src/frontend/src/pages/HomePage/components/ParentTask.tsx | Made parent checkbox disabled and visually distinct |
| src/frontend/src/pages/HomePage/components/ChecklistSection.tsx | Added display of new member events image after General checklist |
| src/frontend/src/pages/HomePage/components/Checklist.tsx | Changed initial showTasks state to true (expanded by default) |
| src/frontend/src/pages/AdminToolsPage/OnboardingConfig/OnboardingInfoSection.tsx | Added UI for uploading new member events image |
| src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/AdminSubtaskSection.tsx | Added drag-and-drop for reordering subtasks and info blocks |
| src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/AdminChecklist.tsx | Added drag-and-drop for reordering parent tasks |
| src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/CreateChecklistModal.tsx | Refactored to support creating both TASK and INFO items with drag-and-drop ordering |
| src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/CreateInfoBlockModal.tsx | New modal for creating information blocks with markdown preview |
| src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/EditInfoBlockModal.tsx | New modal for editing information blocks with markdown preview |
| src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/SubtaskFormModal.tsx | Updated to set itemType to TASK when creating subtasks |
| src/frontend/src/pages/AdminToolsPage/RecruitmentConfig/AdminToolsRecruitmentConfig.tsx | Fixed alt text from "Apply Interest" to "Explore As Guest" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useState(() => { | ||
| setLocalItems(allItems); | ||
| }); |
There was a problem hiding this comment.
This is using useState incorrectly. The useState hook is meant to initialize state, not to create side effects. To synchronize local state with props changes, you should use the useEffect hook instead.
Replace this pattern with:
useEffect(() => {
setLocalItems(allItems);
}, [allItems]);This ensures the local items update when allItems changes, which happens when parentTask.subtasks updates.
| }) | ||
| ) | ||
| ); | ||
| }) |
There was a problem hiding this comment.
Missing semicolon after the transaction block. This will likely cause a syntax error or unexpected behavior.
| const allItems = [ | ||
| ...subtasks.map((subtask) => ({ | ||
| ...subtask, | ||
| itemType: subtask.itemType ?? 'TASK', | ||
| displayOrder: subtask.displayOrder ?? 999 | ||
| })), | ||
| // Backward compatibility: show old descriptions only if they exist and aren't already in subtasks as info blocks | ||
| ...(parentTask.descriptions && parentTask.descriptions.length > 0 | ||
| ? parentTask.descriptions.map((description, index) => ({ | ||
| checklistId: `info-${index}`, | ||
| name: description, | ||
| itemType: 'INFO' as const, | ||
| displayOrder: 1000 + index, | ||
| isOptional: false | ||
| })) | ||
| : []) | ||
| ].sort((a, b) => a.displayOrder - b.displayOrder); |
There was a problem hiding this comment.
The backward compatibility logic for old descriptions could cause duplicate items to be shown. If the data was migrated from old descriptions to new INFO-type checklist items but the descriptions array wasn't cleared, users would see both the old descriptions and the new INFO blocks.
Consider adding a filter to prevent showing old descriptions if there are already INFO-type subtasks:
const hasInfoBlocks = subtasks.some(s => s.itemType === 'INFO');
const allItems = [
...subtasks.map((subtask) => ({
...subtask,
itemType: subtask.itemType ?? 'TASK',
displayOrder: subtask.displayOrder ?? 999
})),
// Only show old descriptions if no INFO blocks exist
...(!hasInfoBlocks && parentTask.descriptions && parentTask.descriptions.length > 0
? parentTask.descriptions.map((description, index) => ({
checklistId: `info-${index}`,
name: description,
itemType: 'INFO' as const,
displayOrder: 1000 + index,
isOptional: false
}))
: [])
].sort((a, b) => a.displayOrder - b.displayOrder);| useState(() => { | ||
| setLocalTasks(parentChecklists); | ||
| }); |
There was a problem hiding this comment.
This is using useState incorrectly. The useState hook is meant to initialize state, not to create side effects. To synchronize local state with props changes, you should use the useEffect hook instead.
Replace this pattern with:
useEffect(() => {
setLocalTasks(parentChecklists);
}, [parentChecklists]);This ensures the local tasks update when parentChecklists changes.
| }) | ||
| ) | ||
| ); | ||
| }) |
There was a problem hiding this comment.
Missing semicolon after the transaction block. This will likely cause a syntax error or unexpected behavior.
| }) | |
| }); |
| await Promise.all( | ||
| filteredSubtasks.map((subtask) => | ||
| createChecklist({ | ||
| name: subtask.name, | ||
| descriptions: [], | ||
| teamId, | ||
| teamTypeId, | ||
| parentChecklistId: parentChecklist.checklistId, | ||
| isOptional: subtask.isOptional | ||
| }) | ||
| ) | ||
| items.map((item) => { | ||
| if (item.type === 'TASK' && item.name) { | ||
| return createChecklist({ | ||
| name: item.name, | ||
| descriptions: [], | ||
| teamId, | ||
| teamTypeId, | ||
| parentChecklistId: parentChecklist.checklistId, | ||
| isOptional: item.isOptional || false, | ||
| itemType: 'TASK' | ||
| }); | ||
| } else if (item.type === 'INFO' && item.content) { | ||
| return createChecklist({ | ||
| name: 'Info Block', | ||
| descriptions: [item.content], | ||
| teamId, | ||
| teamTypeId, | ||
| parentChecklistId: parentChecklist.checklistId, | ||
| isOptional: false, | ||
| itemType: 'INFO' | ||
| }); | ||
| } | ||
| return Promise.resolve(); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The Promise.all will create all items in parallel, which means the displayOrder assigned by the backend may not match the actual order in the items array. This happens because the backend assigns displayOrder based on the order items are created, not on any provided order.
Consider either:
- Creating items sequentially (using a for loop instead of Promise.all) to ensure proper ordering
- Or better, pass the desired displayOrder to the backend as part of the creation payload and have the backend honor it
For example:
for (let i = 0; i < items.length; i++) {
const item = items[i];
if (item.type === 'TASK' && item.name) {
await createChecklist({
name: item.name,
descriptions: [],
teamId,
teamTypeId,
parentChecklistId: parentChecklist.checklistId,
isOptional: item.isOptional || false,
itemType: 'TASK'
// TODO: Add displayOrder: i + 1 once backend supports it
});
} else if (item.type === 'INFO' && item.content) {
await createChecklist({
name: 'Info Block',
descriptions: [item.content],
teamId,
teamTypeId,
parentChecklistId: parentChecklist.checklistId,
isOptional: false,
itemType: 'INFO'
// TODO: Add displayOrder: i + 1 once backend supports it
});
}
}| const schema: yup.ObjectSchema<ChecklistFormValues> = yup.object().shape({ | ||
| name: yup.string().required('Name is Required') | ||
| }) as any; |
There was a problem hiding this comment.
Using as any to bypass TypeScript type checking is an anti-pattern that defeats the purpose of using TypeScript. This suggests there might be a type incompatibility between the yup schema definition and the expected type.
Consider properly typing the schema without the as any cast, or if there's a genuine type issue with the library, add a more specific comment explaining why this is necessary.
| const schema: yup.ObjectSchema<ChecklistFormValues> = yup.object().shape({ | |
| name: yup.string().required('Name is Required') | |
| }) as any; | |
| const schema: yup.ObjectSchema<ChecklistFormValues> = yup | |
| .object({ | |
| name: yup.string().required('Name is Required') | |
| }) | |
| .required(); |
| @@ -1,10 +1,12 @@ | |||
| import { Box, Grid, Link, Typography } from '@mui/material'; | |||
| import React from 'react'; | |||
| import { Box, Grid, Link, Typography, useTheme } from '@mui/material'; | |||
There was a problem hiding this comment.
Unused import Link.
| import { Box, Grid, Link, Typography, useTheme } from '@mui/material'; | |
| import { Box, Grid, Typography, useTheme } from '@mui/material'; |
| import { Box } from '@mui/system'; | ||
| import React from 'react'; | ||
| import { Checklist } from 'shared'; | ||
| import { Checklist, ChecklistItemType } from 'shared'; |
There was a problem hiding this comment.
Unused import ChecklistItemType.
| import { Checklist, ChecklistItemType } from 'shared'; | |
| import { Checklist } from 'shared'; |
| component="img" | ||
| sx={{ display: 'block', maxWidth: '100%', maxHeight: '200px', mb: 1, objectFit: 'contain' }} | ||
| alt="New Member Event" | ||
| src={newMemberImageBlob ? URL.createObjectURL(newMemberImageBlob) : undefined} |
There was a problem hiding this comment.
This use of variable 'newMemberImageBlob' always evaluates to true.
| src={newMemberImageBlob ? URL.createObjectURL(newMemberImageBlob) : undefined} | |
| src={URL.createObjectURL(newMemberImageBlob)} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/CreateInfoBlockModal.tsx
Outdated
Show resolved
Hide resolved
src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/CreateChecklistModal.tsx
Outdated
Show resolved
Hide resolved
src/frontend/src/pages/AdminToolsPage/OnboardingConfig/Checklists/CreateChecklistModal.tsx
Outdated
Show resolved
Hide resolved
Changes
Test Cases
Screenshots
If you made UI changes you must post a screenshot of the whole page for each change in 1) a normal sized window and 2) the smallest possible window
If you did any manual testing (e.g., with Postman), put screenshots of the http request and before and after of the db
If none of this applies, you can delete this section
To Do
Any remaining things that need to get done
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes #3767