fix: reduce side effect logic#2580
Conversation
|
Looks good to me, if you can get the conflicts resolved I can submit it to the owning team for review. |
duidae
force-pushed
the
fix/fix_side_effect_loop
branch
from
83de3f3 to
28ea30f
Compare
|
@dustin-cowles conflicts are fixed, please have a look & thanks! 🙂 |
|
|
There was a problem hiding this comment.
Pull request overview
This PR refactors course-list processing in the Inbox UI to avoid side effects during array filtering and does a small cleanup to reduce magic-string usage around the “All Courses” option.
Changes:
- Refactor course separation logic to use a single
reducepass instead of mutating an external array insidefilter(). - Introduce constants in
CourseSelectto avoid repeated string literals for the “allCourses” group key and “all_courses” id comparisons.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ui/features/inbox/react/containers/MessageListActionContainer.tsx |
Replaces side-effectful filter + external mutation with reduce to split concluded vs non-concluded courses. |
ui/features/inbox/react/components/CourseSelect/CourseSelect.tsx |
Adds constants for the “All Courses” key/id to reduce magic strings and improve consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { concludedCourses, moreCourses } = uniqueCourses.reduce( | ||
| (acc, course) => { | ||
| if (course.concluded === true) { | ||
| acc.concludedCourses.push(course) | ||
| } else { | ||
| acc.moreCourses.push(course) | ||
| } | ||
| return acc | ||
| }, | ||
| { concludedCourses: [], moreCourses: [] } | ||
| ) |
There was a problem hiding this comment.
The reduce accumulator initializer uses empty array literals ({ concludedCourses: [], moreCourses: [] }). Under this repo’s strict TS settings, those often infer as never[], which makes the subsequent .push(course) lines type-error. Add an explicit accumulator type (or cast the empty arrays to the appropriate element type) so push is type-safe and avoids implicit never[] inference.
| if (key === 'allCourses') { | ||
| // if provided, allCourses should always be present | ||
| if (key === ALL_COURSES_KEY) { | ||
| // if provided, ALL_COURSES_KEY should always be present |
There was a problem hiding this comment.
The inline comment refers to the constant name (ALL_COURSES_KEY) rather than the actual option group (allCourses), which is a bit unclear to readers. Consider rewording it to describe the behavior (e.g., that the “All Courses” group should always be included when present) without referencing the internal constant name.
| // if provided, ALL_COURSES_KEY should always be present | |
| // Always include the "All Courses" group when it is provided. |
thanks!