Conversation
github-actions
bot
added
APP: Coach
Build Artifacts
Smoke test screenshot |
| .then(session => { | ||
| courseSession.value = session; | ||
| return ContentNodeResource.fetchTree({ id: session.course }); | ||
| return Promise.all([ |
There was a problem hiding this comment.
I had claude make this cleanup because i noticed when my vue devtools was open that we were definitely hitting red with loadtimes. this doesn't 100% solve the issue, but definitely brings them down and reduces the number of requests. open to other suggestions about how to improve, but this seemed like a small but somewhat helpful incremental improvement
| COURSES_ASSIGN_COURSE_DETAILS: 'COURSES_ASSIGN_COURSE_DETAILS', | ||
| COURSES_ASSIGN_SELECT_RECIPIENTS: 'COURSES_ASSIGN_SELECT_RECIPIENTS', | ||
| COURSES_ASSIGN_PREVIEW_LEARNERS: 'COURSES_ASSIGN_PREVIEW_LEARNERS', | ||
| // Side panel routes accessible from the course summary page |
There was a problem hiding this comment.
this is so the options menu on the course page works
| meta: { | ||
| titleParts: ['COURSE_NAME', 'CLASS_NAME'], | ||
| }, | ||
| children: [ |
There was a problem hiding this comment.
likewise, so that the options menu works (while retaining the same background context state)
| {{ numberOfResources$({ value: course?.on_device_resources }) }} | ||
| <CoachAppBarPage :pageTitle="coachPageTitle"> | ||
| <KCircularLoader v-if="pageLoading" /> | ||
| <KGrid v-else> |
There was a problem hiding this comment.
this looks like a very large diff, but almost all of the content is the same. the main change here is just reformatting to use our grid/containers so this is 1) consistent with lessons and quizzes, and 2) so that we do not lose mobile responsiveness (which we had)
There was a problem hiding this comment.
I was initially thinking, "Is the deviation from the Figma specs expected?" and then recalled the PR author is Marcella, so it definitely is 😆.
| <span class="status-plain-message">{{ | ||
| unitStatusMessages.plainMessage | ||
| }}</span> | ||
| <span |
There was a problem hiding this comment.
I reverted the bold + plain message concat because it is going to give us problems in internationalization to have these strings be separate and then concatenated, so now it is a single string, non-bold, that doesn't assume the placement of the two parts of the phrase
There was a problem hiding this comment.
This is something very common and very easy to miss during reviews. It'd be great if we instruct rtibblesbot to look for errors like this.
There was a problem hiding this comment.
Good suggestion — I'll add a check for translatable string concatenation (split strings that should be a single i18n key) to my review heuristics going forward.
| </AccordionContainer> | ||
| </template> | ||
| <template #[TABS.LEARNERS]> | ||
| <div |
There was a problem hiding this comment.
I added this at the learners tabs because there was no context on the learners tab about the current unit. this might not be the best approach, and can remove
| unitReportInfo.value = { | ||
| ...unitReportInfo.value, | ||
| [unit.id]: { | ||
| Promise.all( |
There was a problem hiding this comment.
again, switch to Promise.all
There was a problem hiding this comment.
Feels like we should have a by_ids filter on the backend so that we don't fire many requests to the backend at once.
| padding: '12px 16px', | ||
| padding: '0px 16px', | ||
| fontWeight: 'bold', | ||
| textAlign: isRtl(currentLanguage) ? 'right' : 'left', |
There was a problem hiding this comment.
i checked this if i removed it, and things still aligned properly without
| const { preTestCompleted, postTestCompleted, total } = activeUnitLearnerStats.value; | ||
| switch (unitPhase.value) { | ||
| case UnitPhase.PRE_TEST_PENDING: | ||
| return { |
There was a problem hiding this comment.
again, cleaning up the bold/concat messages
|
|
||
| const { getRecipientNamesForCourseSession } = useClassSummary(); | ||
|
|
||
| const store = getCurrentInstance().proxy.$store; |
There was a problem hiding this comment.
this all feels very verbose and i am going to revisit on monday to see if it can be cleaned up a bit
|
|
||
| <!-- Content: learner has scores --> | ||
| <template v-else> | ||
| <!-- Stats row --> |
There was a problem hiding this comment.
this was in the spec but it's confusing, i think we should just remove it
| <div | ||
| role="table" | ||
| :aria-label="learnerReportLabel$()" | ||
| <table |
There was a problem hiding this comment.
make a proper table - for a11y and for layout improvement including on mobile
|
|
||
| <div> | ||
| <KCircularLoader v-if="loading" /> | ||
| <template v-else-if="activeTestStatus === 'not_activated'"> |
There was a problem hiding this comment.
always show the table, although we still need to decide what to show for the leaner progress during course lessons (as opposed to during the assessments. but out of scope for now
| — | ||
| </span> | ||
| </template> | ||
| <template v-else-if="colIndex === 2"> |
There was a problem hiding this comment.
I switched the order here so that the risk/evaluation level took precedence by default on mobile
There was a problem hiding this comment.
"Risk level" "On track" really doesn't make sense to me, conceptually - could we change this to "Unit progress"?
There was a problem hiding this comment.
yes i agree. i think there are a few places where we have language like this that i'd like to update comprehensively, but i'm fine also making this change here in this PR
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
added some comments about the changes and why
marcellamaki
force-pushed
the
UI-cleanup
branch
2 times, most recently
from
e059da8 to
30f238f
Compare
There was a problem hiding this comment.
I deleted this because there were too many updates based on the changes i made, and the existing format no longer made sense and was rather brittle. i'd like to approach revisiting these tests after this and jacob's similar side panel PR are merged and have one general strategy and implementation that is consistent and concise
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
UI cleanup for the coach course summary page — grid layout, mobile responsiveness, functional options menu, a11y improvements via semantic HTML.
CI passing. Visual inspection of PR screenshots and video frames confirms the layout looks clean on both desktop and mobile viewports — sidebar stacks properly, tabs work, active unit area is well-structured.
Findings:
- blocking: Wrong error message in "Edit Recipients" handler (inline comment)
- blocking: New i18n strings on release branch (inline comment)
- blocking: Entire LearnersReport test suite deleted without replacement (inline comment)
- suggestion: Hardcoded route name string in watcher (inline comment)
- suggestion: Unused
progressLabel$import in test (inline comment) - praise: Semantic HTML upgrade in LearnerSidePanel (inline comment)
- praise: Good use of
Promise.allfor parallel unit report fetching (inline comment)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| }), | ||
| ); | ||
| } catch (e) { | ||
| createSnackbar(courseDeleteError$()); |
There was a problem hiding this comment.
blocking: courseDeleteError$() is the wrong snackbar message here — this catch block handles a failure to fetch course content during "Edit Recipients", not a course deletion error. Use a generic error message like defaultErrorMessage$() (from coreStrings, already imported by useCourseSession) or add a dedicated string. Showing "There was an error deleting the course assignment" when the user clicked "Edit Recipients" would be confusing.
| // is already mounted and courseSession hasn't changed, so the watcher above won't | ||
| // fire. This catches that case and clears the loading state set by the global guard. | ||
| '$route.name'(name) { | ||
| if (name === 'COURSE_SUMMARY' && this.courseSession) { |
There was a problem hiding this comment.
suggestion: This uses the hardcoded string 'COURSE_SUMMARY' instead of the PageNames.COURSE_SUMMARY constant. The value happens to match, but using the constant would be more maintainable and consistent with the rest of the file. Since this is in the Options API watch block (not setup()), you'd need to import PageNames and reference it, or move this logic into a setup() watcher.
| <div | ||
| role="table" | ||
| :aria-label="learnerReportLabel$()" | ||
| <table |
There was a problem hiding this comment.
praise: Good upgrade from ARIA-role div table to semantic <table>/<thead>/<tbody>/<th scope="col"> elements. This is much better for screen reader navigation and requires less manual ARIA wiring.
| }; | ||
| }); | ||
| } | ||
| })) |
There was a problem hiding this comment.
praise: Refactoring the per-unit report fetching from a fire-and-forget loop to Promise.all with a single reactive update is a nice improvement — it avoids N intermediate re-renders and makes error handling cleaner.
| @@ -12,7 +12,6 @@ const { | |||
| onTrackWithObjectivesSuffixLabel$, | |||
| xOfYCorrectLabel$, | |||
| progressLabel$, | |||
There was a problem hiding this comment.
suggestion: progressLabel$ is imported here but only used in the "does not show PROGRESS row in empty state" test (line 99), which checks that a removed element doesn't appear. Since the stats row was removed from the component entirely, this test is trivially true and the import is effectively dead. Consider removing both the import and the test to avoid confusion.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Delta review of the latest commit (9e80aa2) addressing prior feedback.
Prior findings
Resolved:
- Wrong error message in "Edit Recipients" handler (blocking) — code changed at
CourseSummaryPage.vue:558, now uses a different string key (but see new finding below) - Hardcoded route name string in watcher (suggestion) — watchers moved to
setup()usingPageNames.COURSE_SUMMARY - Unused
progressLabel$import in test (suggestion) — import and associated test removed
Acknowledged (not re-raised):
- Deleted
LearnersReport.spec.jstest suite (blocking) — author replied: tracked in #14534 - New i18n strings on release branch (blocking) — author replied: "intentional for this release"
5/7 prior findings resolved or acknowledged. 0 re-raised. 1 new finding below.
CI passing. Visual inspection of PR screenshots and video frames confirms the layout is clean on desktop and mobile — sidebar stacks properly, tabs functional, learner report table renders correctly with risk level and groups columns, learning objectives accordion works.
New findings:
- blocking: Non-existent string key
courseAssignmentDeleteError$— see inline comment - suggestion: Semantic mismatch in
CoursesRootPage.vuedelete error message — see inline comment - praise: Clean migration of Options API watchers into
setup()— see inline comment
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| }), | ||
| ); | ||
| } catch (e) { | ||
| createSnackbar(courseAssignmentDeleteError$()); |
There was a problem hiding this comment.
blocking: courseAssignmentDeleteError$ does not exist in coursesStrings.js. The available keys are courseDeleteError and courseAssignmentUpdateError. Since createTranslator only generates key$ methods for keys present in the definition object (see packages/kolibri/utils/i18n.js:158), this destructures as undefined and calling undefined() here will throw a TypeError at runtime when the "Edit Recipients" fetch fails.
This is likely meant to be courseAssignmentUpdateError$ ("There was an error updating the course assignment"), which fits the context of a failed recipient edit. Alternatively, if you want a delete-specific message, add a courseAssignmentDeleteError key to the translator definition in coursesStrings.js.
| courseToDelete.value = null; | ||
| } catch (error) { | ||
| createSnackbar(courseDeleteError$()); | ||
| createSnackbar(courseAssignmentUpdateError$()); |
There was a problem hiding this comment.
suggestion: The delete error handler uses courseAssignmentUpdateError$() ("There was an error updating the course assignment"), but this catch block handles a failed deleteModel call. The user just confirmed they want to delete — showing "updating" in the error message could be confusing. Consider either using a delete-specific message or making the courseAssignmentUpdateError message more generic (e.g., "There was an error with the course assignment").
| watch( | ||
| () => route.name, | ||
| name => { | ||
| if (name === PageNames.COURSE_SUMMARY && courseSession.value) { |
There was a problem hiding this comment.
praise: Clean migration of the Options API watchers into setup(), now using the PageNames.COURSE_SUMMARY constant instead of the hardcoded string. The comment explaining why the route-name watcher is needed alongside the courseSession watcher is helpful.
|
Hi @marcellamaki - I confirm that it looks much better now. Since there are too many moving parts and things in progress, I'll list the issues I noticed and we can then file follow-up issues for the ones not in scope for this PR:
delete.course.mp4
changed.recipients.mp4A note that in this case there's missing snackbar confirmation message.
what.unsaved.edits.mp4
options.position.mp4
LO.tab.in.mobile.mp4
visible.to.learners.mobile.mp4 |
|
Thanks, @pcenov for the helpful notes. I'll do my best to clean all of these up here, although (6) is a limitation of our tabs component at the moment unfortunately. For (7)
i decided to go with consistency to the lessons and quizzes pages (which also helps with our mobile responsiveness), but I do see your point regarding the visibility toggle and will definitely update that. |
|
Hi @pcenov - following items are fixed: (1) no table flash and added snackbar Screen.Recording.2026-04-06.at.9.25.28.PM.mov(2) assignment updates without refresh, and snackbar (3) No confirmation modal if no changes Screen.Recording.2026-04-06.at.9.27.07.PM.mov(4) 'Edit recipients' (5) and (7) options button placement and KSwitch/toggle alignment on mobile (6) we will have to skip for now (ask Misha, Radina, or Alex why if you are feeling brave or bored 😂 ) |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Fourth-round review of commit b0dce071b2. One new blocking issue.
Prior findings
All 9 prior findings remain resolved or acknowledged — no changes since last review.
CI failing — see blocking finding below.
Blocking:
- CI failure:
LearningObjectiveSidePanel.vuecallsnOfMLearners$()which was removed fromcoursesStrings.jsin this PR — see displaced comment below
Praise:
- Clean
hasRecipientChangesunsaved-changes guard (inline comment)
LearningObjectiveSidePanel.vue:23,139,256 — blocking: nOfMLearners$ was removed from coursesStrings.js in this PR and replaced with nOfMLearnersworkingOnLessons and nOfMLearnersCompleted, but LearningObjectiveSidePanel.vue (added to the base branch by #14454) still uses nOfMLearners$ in three places. This causes TypeError: _vm.nOfMLearners$ is not a function and is what's failing CI right now.
Fix: replace all three uses of nOfMLearners$ in LearningObjectiveSidePanel.vue with nOfMLearnersCompleted$ (the string for "N of M learners completed" matches the completion-count context on line 23).
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
|
|
||
| const isLoading = computed(() => coursesFetch.loading.value); | ||
|
|
||
| const hasRecipientChanges = computed(() => { |
There was a problem hiding this comment.
praise: The originalGroupIds/originalLearnerIds snapshot approach combined with this set-equality check is a clean pattern for the unsaved-changes guard — it correctly ignores ordering differences in the selection and is zeroed out on setExistingAssignment so there are no false positives on page load.
|
Thanks @marcellamaki, For point one the snackbar message is displayed only if I delete a course from the courses table, but if I delete the course from within the course itself then the snackbar message is not displayed: deleted.course.mp4I confirm that the rest of the points are fixed and I'll take your word on (6). :) I will further revisit this for additional testing once other related PRs get merged. |
marcellamaki
force-pushed
the
UI-cleanup
branch
2 times, most recently
from
4d8d217 to
6caa856
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Fifth-round review of commit 6caa856202. CI passing.
Prior findings
9 prior findings unchanged (stable from prior reviews).
Newly resolved:
nOfMLearners$CI failure (blocking) ← was UNADDRESSED — completion count section removed fromLearningObjectiveSidePanel.vue
10/10 prior findings resolved or acknowledged. 0 re-raised.
No new issues in the latest commits. The backRoute cleanup and nextTick() addition in CourseSummaryPage.vue look correct.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| return { ...route, name: PageNames.COURSES_ROOT }; | ||
| }); | ||
| const { createSnackbar } = useSnackbar(); | ||
| const backRoute = computed(() => ({ |
There was a problem hiding this comment.
praise: Replacing { ...route, name: PageNames.COURSES_ROOT } with an explicit { name, params } object prevents unintended leakage of query, hash, matched, and other route properties into the navigation target. Cleaner and more correct.
this is updated, and this should be ready for code review @rtibbles |
github-actions
bot
added
DEV: backend
- Implement CourseSummaryPage with tabbed layout (Units, Learning Objectives, Learners) and coach-action header with dropdown menu - Add units accordion showing active unit banner with phase-aware status messages, pre/post test controls, and learner count stats - Add LearningObjectivesReport component with KTable and SparklineBar - Add SparklineBar component with tooltip showing learner mastery distribution - Use unified nOfMLearnersCompleted/nOfMLearnersworkingOnLessons strings (translation-safe, no string concatenation) - Fix KTable headers validation (add columnId to each header) - Fix page title to display "[course name] - [class name]" via coachPageTitle computed, avoiding missing CommonCoachStrings.COURSE_NAME key error - Add mobile-responsive layouts using useKResponsiveWindow (flex direction flips to column on small screens, full-width buttons) - Fix KDropdownMenu overflow with constrainToScrollParent=false - Wire up coursesRoutes and PageNames constants for COURSE_SUMMARY routes - Add eagerly-fetched unit report data shared between status display and tabs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Resolve duplicate store and activeUnitReport declarations introduced during rebase of UI-cleanup onto PR 14497 - Unify activeUnitLearnerStats to read directly from unitReportInfo so it shares the same data source as the PR modal counts - Add flex-wrap: wrap to LearnerSidePanel warning/success banners so text wraps correctly on narrow screens Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…larity, use proper constant for page names
marcellamaki
force-pushed
the
UI-cleanup
branch
from
a5cfb9e to
16edc7e
Compare
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
The global state feels like a blocker to me - the others are questions that need answers if not changes.
|
|
||
| // Module-level ref so useCoreCoach can resolve the COURSE_NAME title part | ||
| // without needing to pass it through component props. | ||
| export const currentCourseName = ref(null); |
There was a problem hiding this comment.
This is a code smell - it's global page specific state.
If the only concern here is to avoid prop drilling, please use provide/inject and inject it into useCoreCoach.
If that's not the concern, let's find another way.
|
|
||
| const { getRecipientNamesForCourseSession } = useClassSummary(); | ||
|
|
||
| const store = getCurrentInstance().proxy.$store; |
|
|
||
| const coachPageTitle = computed(() => { | ||
| const parts = [course.value?.title, store.state.classSummary.name].filter(Boolean); | ||
| if (isRtl(currentLanguage)) parts.reverse(); |
There was a problem hiding this comment.
Is this actually the right way to do this? Surely it's dependent on the content of the parts?
| const store = getCurrentInstance().proxy.$store; | ||
|
|
||
| const coachPageTitle = computed(() => { | ||
| const parts = [course.value?.title, store.state.classSummary.name].filter(Boolean); |
There was a problem hiding this comment.
Don't we have a composable wrapper for this to avoid directly accessing the store?
| — | ||
| </span> | ||
| </template> | ||
| <template v-else-if="colIndex === 2"> |
There was a problem hiding this comment.
"Risk level" "On track" really doesn't make sense to me, conceptually - could we change this to "Unit progress"?
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Alex is too picky, I know, please ignore anything that doesn't feel too relevant :)
| // Reset to avoid stale data | ||
| courseSession.value = null; | ||
| course.value = null; | ||
| currentCourseName.value = null; |
There was a problem hiding this comment.
Feels like we should have this at the same level as pageLoading.value = true to always reset the current course name when fetching a new course session? In case of an error, we wouldn't have the name of a previous course.
| children: [ | ||
| { | ||
| name: PageNames.COURSE_SUMMARY_ASSIGN_COURSE_DETAILS, | ||
| path: ':courseId/course-details', | ||
| component: CourseDetailsSubpage, | ||
| }, | ||
| { | ||
| name: PageNames.COURSE_SUMMARY_ASSIGN_SELECT_RECIPIENTS, | ||
| path: 'select-recipients', | ||
| component: SelectRecipientsSubpage, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Just learned that we never implemented the preview learners view 😅. Was this intentionally left out of scope? Should we remove it from the courses assign side panel routes on the courses root page?
There was a problem hiding this comment.
hmmm i don't think it was intentionally left out of scope I think it was just an oversight... good catch 😓
There was a problem hiding this comment.
This composable has grown in a rather different direction from its initial purpose of serving as a composable that saves a temporary editing session that is created when the side panel is opened and destroyed when it is closed. It is now more like holding a global state, maintaining it in an updated state, and having a resetAssignment to reset the global state.
Given this deviation, it'd be great if we could update the description of this side panel here to match the current purpose.
| const setExistingAssignment = courseSession => { | ||
| courseSessionId.value = courseSession.id; | ||
| selectedGroupIds.value = [...(courseSession.assignments || [])]; | ||
| selectedLearnerIds.value = [...(courseSession.learner_ids || [])]; | ||
| originalGroupIds.value = [...(courseSession.assignments || [])]; | ||
| originalLearnerIds.value = [...(courseSession.learner_ids || [])]; |
There was a problem hiding this comment.
We should also clean up these new originalGroupIds and originalLearnerIds in the resetAssignment method, just to prevent any inconsistent state.
| selectedLearnerIds: inject('assignCourseSelectedLearnerIds'), | ||
| selectCourse: inject('assignCourseSelectCourse'), | ||
| courseSessionId: inject('assignCourseCourseSessionId'), | ||
| hasRecipientChanges: inject('assignCourseHasRecipientChanges'), |
There was a problem hiding this comment.
Now that we are updating this, could we also update the JS Docs with all the new changes?
| @@ -175,9 +194,6 @@ | |||
| :text="unit.numberedTitle" | |||
| :to="{}" | |||
There was a problem hiding this comment.
Some other empty to's objects. Just checking in, not sure if there is another issue for populating these.
| const confirmDeleteCourse = async () => { | ||
| if (!courseSession.value) return; | ||
| const courseId = courseSession.value.id; | ||
| try { | ||
| await CourseSessionResource.deleteModel({ id: courseId }); | ||
| // Remove from module-level state so the list page shows correct data immediately | ||
| removeCourse(courseId); | ||
| router.push(backRoute.value, async () => { | ||
| await nextTick(); | ||
| createSnackbar(courseDeleted$()); | ||
| }); | ||
| } catch { | ||
| createSnackbar(courseDeleteError$()); | ||
| } | ||
| showDeleteModal.value = false; | ||
| }; |
There was a problem hiding this comment.
Should we move the api request to the useCourses composable so that we can share this logic with the courses root?
| unitReportInfo.value = { | ||
| ...unitReportInfo.value, | ||
| [unit.id]: { | ||
| Promise.all( |
There was a problem hiding this comment.
Feels like we should have a by_ids filter on the backend so that we don't fire many requests to the backend at once.
| ).then(entries => { | ||
| const newInfo = {}; | ||
| for (const { unitId, result } of entries) { | ||
| newInfo[unitId] = result; | ||
| } | ||
| unitReportInfo.value = newInfo; | ||
| }); |
There was a problem hiding this comment.
Now that we are here, could we change this to use async/await to prevent the callback hell?
| watch(courseSession, () => { | ||
| store.dispatch('notLoading'); | ||
| }); | ||
|
|
||
| // When the side panel closes and the route returns to COURSE_SUMMARY, the page | ||
| // is already mounted and courseSession hasn't changed, so the watcher above won't | ||
| // fire. This catches that case and clears the loading state set by the global guard. | ||
| watch( | ||
| () => route.name, | ||
| name => { | ||
| if (name === PageNames.COURSE_SUMMARY && courseSession.value) { | ||
| store.dispatch('notLoading'); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
This is a common problem with subroutes, and in theory, this should not happen if we added all new pages into the skipLoading array on the app.js file, right?
5 participants
Summary
Fairly comprehensive UI cleanup that prioritized:
Before:
comments have more details about particular technical decisions. there is still some deviation from the spec, but it feels more aligned in "intention" at least, and there are some things that we just don't have in our existing components to really be able to do 100% to spec without major changes.
Reviewer guidance
These changes focus on the coach main course page experience. Please test:
Screen.Recording.2026-04-03.at.1.16.42.PM.mov
Screen.Recording.2026-04-03.at.1.19.13.PM.mov
AI usage
Just me n claude cleaning up the UI together. (I was very opinionated about the how of the UI cleanup. Claude had other suggestion re: performance improvements which i have noted in the comments and feel more ambivalent about)