Add Notifications page

[AlexVelezLl]

Summary

• Added Notifications page.
  • Displays this page as a StudioImmersivePage.
  • To persist the state when reloading the page, I have implemented this so that it is opened if a Modal=NOTIFICATIONS query param is set in the route. Since this modal will be shown across the entire application, I chose this option rather than creating a new route for this page on all app routers.
  • Built the NotificationsList component in a way that each row corresponds to a notification type, and each notification type has its own renderer. So that we can extend this later if we want to support more notification types.
• Ended up implementing the "red dot" by adding two new datetime fields in the User model: last_read_notification_date and newest_notification_date, the first representing the date of the last notification read by the user, the second the date of the most recent notification. If newest_notification_date > last_read_notification_date, the user has a new notification :).
• Composables
  • Moved useFilter, useFetch, useKeywordSearch, and useQueryParams to the shared folder.
  • Modified useFilter composable to comply with the current KSelect API.
  • Added useSnackbar as a wrapper of the current Vuex Snackbar module, until this gets migrated.
  • Added useStore as sugar syntax to get the store from the currentInstance.
  • Added useCommunityLibraryUpdates composable to manage the data fetching and transformation of submissions into updates/notifications.

Grabacion.de.pantalla.2025-12-17.a.la.s.10.41.42.a.m.mov

References

Closes #5457
Closes #5458

Reviewer guidance

• Check the new notifications page on the profile menu in the Appbar or by opening the main menu.
• To create new notifications, create new Submissions.
• If you want to see approved submissions/rejected submissions notifications, you can use the admin_communitylibrary_submission/{id}/resolve endpoint.

Tech debt introduced

• Right now, notification filter selections don't close automatically when clicking outside the select input. This is a bug due to some weird interaction of VDialog and Popper, I did not want to spend too much time trying to solve this because we will end up replacing Vuetify in Studio anyways, but noting that this bug will be present until then. We can coordinate later if we can prioritize migrating the ImmersivePage into Studio before releasing the Community Library, or if we prefer spending more time trying to fix this bug. There is now a StudioImmersivePage! (Thanks @MisRob 😄 )

[AlexVelezLl]

These tests were moved to be their own test files

[AlexVelezLl]

Follow up: learningequality/kolibri-design-system#1187

[AlexVelezLl]

This was a weird error, the key field in the session indexedDB table is CURRENT_USER and its only value could be CURRENT_USER so it doesnt make sense to look at the id and if id === state.currentUser.id for this table. Therefore, until now, this function wasn't doing anything.

[MisRob]

Hi @AlexVelezLl, I didn't review, just noticed the note about FullscreenModal (assuming you refer to shared/views/FullscreenModal.vue). If so, would you instead please use non-Vuetify alternative StudioImmersiveModal? We built it recently as a replacement for FullscreenModal. Of course, feel free to adjust StudioImmersiveModal to this new use-case if it's needed.

If there are any other places like that, please let me know and we can chat. This tracker should be quite up-to-date so it may too help.

[MisRob]

And also StudioPage + StudioOfflineAlert will be handy :) The first one takes care of consistent paddings on all screens - I hope we get to using it everywhere for these kinds of layouts in Studio.

[AlexVelezLl]

Thanks a lot @MisRob! I did look for an alternative on Studio, but didn't find it. Will use the StudioImmersiveModal instead. Thanks!

[MisRob]

Yeah the naming doesn't help - when possible I try to rename in line with Kolibri conventions :) Glad it helps.

[rtibblesbot]

Great work on this notifications feature! This is a well-structured PR with clean composable architecture, good test coverage, and thoughtful design decisions (like using query params for modal persistence and the extensible notification type renderer pattern). The video demo looks polished.

There are a few issues to address before merging:

Blocking issues:

• Variable shadowing bug in useFetch.js — the catch (error) parameter shadows the outer error ref, so error state is never written to the ref
• notify_update_to_channel_editors() is called before super().save() for new submissions, but date_updated is auto_now=True and hasn't been set yet — the notification date will be None
• notify_users() reads user.newest_notification_date from stale in-memory objects after .update() — the change events will push the old values
• Approval/rejection notification components pass notification.date_created instead of notification.date to NotificationBase, showing the wrong date
• NotificationFilters receives a lastReadFilter prop from the parent but never declares or uses it

Visual inspection: The UI video shows proper layout, working tabs, filters, and notification list rendering. The immersive modal and responsive design look good.

CI: All checks pass.

Looking forward to the updates — this is a great addition to Studio!

[rtibblesbot]

Solid feature implementation — the notifications page, composables migration, and red dot indicator are well-architected.

CI passing. Video demo verified: layout, filters, chips, and tab switching look correct.

Blocking:

• Variable shadowing bug in useFetch.js — catch (error) shadows the outer error ref (see inline)
• notify_update_to_channel_editors() called before super().save() on creation — date_updated is None for auto_now fields until saved (see inline)
• Approval/rejection notification components pass notification.date_created instead of notification.date to NotificationBase (see inline)

Suggestions:

• lastReadFilter prop is passed to NotificationFilters but never declared or used in that component (see inline)

[rtibblesbot]

Solid implementation of the notifications page and red dot indicator. Architecture is well-considered — the composable-based approach, extensible notification type rendering, and timestamp-based unread tracking are good design choices. CI passing. Video demo verified.

• blocking: Wrong date displayed for approval/rejection notifications (shows creation date instead of status-change date)
• blocking: Variable shadowing bug in useFetch — the error ref is never set on catch
• suggestion: Unused prop lastReadFilter passed to NotificationFilters
• nitpick: Using array index as v-for key in notification list

[rtibblesbot]

Solid implementation of the notifications page and red-dot indicator. The architecture — timestamp-based unread tracking, composable-driven filtering, per-type notification renderers — is well thought out and extensible.

CI passing. Video demo reviewed; UI looks clean with proper layout, filter behavior, and empty states.

Findings:

• blocking (1): Race condition in notify_users — queryset is read after a bulk update(), so newest_notification_date in the change events may be stale. See inline comment on models.py:706.
• suggestion (3): useFetch.js catch block shadows outer error variable; mark_notifications_read uses Greatest on an in-memory assignment instead of DB-level update; notify_update_to_channel_editors is called before super().save() so date_updated may be None for new submissions.
• nitpick (2): Mutating input argument in getSubmissionsUpdates; minor typo in comment.
• strings: This PR adds new i18n strings and targets the unstable branch (which is the default branch), so that's appropriate.

[rtibblesbot]

Well-structured PR that adds a solid notifications system. The composable architecture is extensible and the UI is clean.

CI passing. Video demo reviewed — layout, filters, tabs, and red dot all look correct.

Findings:

• suggestion (x3): Date filter eagerness, self-notification on submission creation, View more buttons not wired up
• nitpick (x1): getStartOfWeek locale assumption

See inline comments for details.

[rtibblesbot]

Solid implementation of the notifications page and red dot indicator. The architecture — composable-based data fetching, extensible notification type renderers, and timestamp-based unread tracking — is well-considered.

CI passing. Video demo verified: notifications page layout, tabs, filters, chips, and empty states all look correct.

Findings:

• suggestion: Date filter values are computed at component setup time, not at selection time (NotificationFilters.vue)
• suggestion: mark_notifications_read leaves the in-memory model field as a Greatest expression after save
• nitpick: flaggedNotification string uses { variable } (with spaces) inconsistent with existing {variable} pattern
• nitpick: getStartOfWeek() treats Sunday as week start (US convention) — may not match expectations for an international platform
• nitpick: New strings are added targeting unstable (the default branch), which is appropriate

[rtibblesbot]

Good work on the notifications page. The architecture is clean: notification types map to dedicated renderer components, the composable handles data transformation well, and the Vuex session bug fix is correct.

CI: Python tests in progress. Video demo verified — layout, filters, tabs, and chips all look correct.

Suggestions:

• useCommunityLibraryUpdates.js:75-76: Direct mutation of submission objects from API response
• MainNavigationDrawer.vue:238: Drawer not closed when opening notifications modal (unlike language modal)
• CommunityLibrarySubmissionRejection.vue:54: resolved_by_name may be null, could render literal "null" in title
• index.vue:25,32: Redundant @click handlers on VTabs when v-model already manages selection

Nitpicks:

• useCommunityLibraryUpdates.js:125: Redundant new Date() wrapping on values already of type Date
• index.vue:167: Typo "precisision" → "precision" (now on line 167 after latest commit)

[rtibblesbot]

Thorough implementation of the notifications page and red dot indicator for community library submissions.

CI passing. Video demo verified — layout, filters, tabs, and status chips all look correct.

Suggestions:

• useFetch.js: the data.value = null move changes behavior for all consumers (see inline)
• mark_notifications_read: unused pk parameter
• useCommunityLibraryUpdates.js: isLoadingMore state could be stale on error path
• NotificationFilters.vue: date filter values computed once at component creation

Nitpicks:

• CommunityLibraryStatusChip.vue: props declared after setup (non-standard order)
• useStore.js: no guard for missing instance

[rtibblesbot]

Well-structured notifications feature. The architecture — composable-based data fetching, extensible notification type renderers, timestamp-based unread tracking — is solid. CI passing. Video demo verified.

Delta from prior reviews

• [blocking] Variable shadowing bug in useFetch.js — RESOLVED (renamed to caughtError)
• [blocking] notify_update_to_channel_editors() called before super().save() for new submissions — RESOLVED (line 2935 calls super().save() before line 2939 calls notify_update_to_channel_editors)
• [blocking] notify_users() reads stale in-memory objects after .update() — RESOLVED (line 703-705 refreshes queryset from DB before creating change events)
• [blocking] Approval/rejection components pass notification.date_created instead of notification.date — RESOLVED (both now use notification.date)
• [blocking] Greatest(NULL, value) returns NULL on PostgreSQL — RESOLVED (Coalesce wraps F() to handle NULL)
• [blocking] NotificationFilters receives unused lastReadFilter prop — RESOLVED (prop removed; lastRead is now passed via queryParams to the composable)
• [suggestion] useFetch.js data.value = null behavior change — RESOLVED (moved inside catch block with clear comment)
• [suggestion] mark_notifications_read leaves in-memory field as Greatest expression — UNRESOLVED (after save(), self.last_read_notification_date is a Greatest expression, not a datetime; low risk since the callsite discards the instance, but fragile if reused)
• [suggestion] WithNotificationIndicator screen reader support — RESOLVED (added visuallyhidden span with newNotificationsNotice$() text)
• [suggestion] useStore missing guard for getCurrentInstance — RESOLVED (throws descriptive error)
• [suggestion] notify_update_to_channel_editors on resolve doesn't exclude the resolving admin — UNRESOLVED (admin self-notification if they're also an editor; design choice, not a bug)
• [suggestion] Date filter values computed once at component creation — UNRESOLVED (see inline)
• [suggestion] handleNotificationsRead timeout path leaves stale data — RESOLVED (timeout resolves the promise, allowing the flow to continue)
• [suggestion] resolved_by_name may be null rendering literal "null" — RESOLVED (get_resolved_by_name returns None, and rejection template falls back to empty string)
• [suggestion] Drawer not closed when opening notifications modal — UNRESOLVED (see inline)
• [nitpick] Array index as v-for key — RESOLVED (key is now ${notification.id}-${notification.type})
• [nitpick] CommunityLibraryStatusChip mixes Options API with Composition API / props after setup — UNRESOLVED (minor, not worth blocking)
• [nitpick] getStartOfWeek locale assumption — RESOLVED (comment updated: "Setting first day of the week to Monday")
• [nitpick] Typo "precisision" — unable to locate in current code (likely resolved)
• [nitpick] Test description mismatch — unable to verify (not re-raised)

New findings

• suggestion (x1): "View more" buttons are non-functional (see inline)
• suggestion (x1): Drawer not closed when opening notifications modal (carried from prior, see inline)
• suggestion (x1): Date filters computed eagerly at setup time (carried from prior, see inline)

---

@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, UNRESOLVED, PARTIALLY_ADDRESSED, 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

[rtibblesbot]

suggestion: The "View more" KButton has no @click handler — it renders but does nothing when clicked. Same in the rejection and creation notification components. If this is intentionally deferred, consider hiding the button until it's wired up, to avoid a confusing dead click for users.

[rtibbles]

He said he would do it later!

[rtibblesbot]

suggestion: Unlike openLanguageModal() (line 234) which sets this.drawer = false before opening the modal, showNotificationsModal leaves the drawer open. The notifications modal renders as a full-screen immersive page, so the drawer would be visible underneath or require the user to dismiss it separately.

[rtibblesbot]

suggestion: The date filter values (DateFilterMap) are computed once at component setup time. If the notifications modal stays open across a day boundary, "Today" will still use the date from when the component was first created. Consider computing the date strings lazily when the filter is selected, or recomputing when the modal opens.

[rtibbles]

I know it's an edge case, but might be worth filing a follow up issue for these values to get refreshed (similar to how we do for the ElapsedTime component in Kolibri). Exact cadence doesn't have to be hugely frequent - I just know some people keep their Studio page open in the browser for long periods (because of specific issues we've seen with the vuex and data syncing functionality that only happens in long lived sessions).

[AlexVelezLl]

Hey @rtibblesbot, could you please create a follow-up issue to remind us to fix it later?

[rtibbles]

@rtibblesbot any update on this?

[rtibblesbot]

Well-architected notifications feature. The composable-based data fetching, extensible notification type renderers, and timestamp-based unread tracking are solid design choices.

CI: Frontend tests, linting, Python unit tests, and build still in progress. Build message files and path checks passing.

Delta from prior reviews

• [blocking] Variable shadowing bug in useFetch.js — RESOLVED (renamed to caughtError)
• [blocking] notify_update_to_channel_editors() called before super().save() for new submissions — RESOLVED (line 2935 calls super().save() before line 2939 calls notify_update_to_channel_editors)
• [blocking] notify_users() reads stale in-memory objects after .update() — RESOLVED (lines 703-705 refresh queryset from DB before creating change events)
• [blocking] Approval/rejection components pass notification.date_created instead of notification.date — RESOLVED (all three components now use notification.date)
• [blocking] Greatest(NULL, value) returns NULL on PostgreSQL — RESOLVED (Coalesce wraps F() to handle NULL in both mark_notifications_read and notify_users)
• [blocking] NotificationFilters receives unused lastReadFilter prop — RESOLVED (prop removed; lastRead is now passed via queryParams to the composable)
• [blocking] "View more" buttons have no click handler — RESOLVED as intentional deferral (developer confirmed: "Will implement this page later")
• [suggestion] useFetch.js data.value = null behavior change — RESOLVED (moved inside catch block with explanatory comment)
• [suggestion] mark_notifications_read leaves in-memory field as Greatest expression — UNRESOLVED (low risk since callsite returns HTTP 204 immediately, but fragile if method is reused)
• [suggestion] WithNotificationIndicator screen reader support — RESOLVED (added visuallyhidden span with newNotificationsNotice$() text)
• [suggestion] useStore missing guard for getCurrentInstance — RESOLVED (throws descriptive error)
• [suggestion] notify_update_to_channel_editors on resolve doesn't exclude the resolving admin — CONTESTED (developer explained: "In the rare case that the resolver is an editor of the channel being resolved, it's okay if they get the notification, too")
• [suggestion] Date filter values computed once at component creation — UNRESOLVED (developer acknowledged: "I think we can live with that"; very minor edge case)
• [suggestion] handleNotificationsRead timeout path leaves stale data — RESOLVED (waitForLastReadUpdate removed entirely; flow now awaits markNotificationsRead dispatch directly)
• [suggestion] resolved_by_name may be null rendering literal "null" — RESOLVED (falls back to empty string: props.notification.resolved_by_name || '')
• [suggestion] Drawer not closed when opening notifications modal — UNRESOLVED (see inline)
• [suggestion] Redundant @click handlers on VTabs — RESOLVED (removed; v-model handles selection)
• [suggestion] AppBar showNotificationsModal missing trackClick analytics — RESOLVED (line 237: this.$analytics.trackClick('general', 'Notifications'))
• [suggestion] NotificationList has no fallback for unknown notification types — RESOLVED (line 28: || 'div' fallback)
• [suggestion] Direct mutation of submission objects from API response — RESOLVED (lines 75-79: creates new sub object via spread before mutating)
• [suggestion] isLoadingMore state could be stale on error path — RESOLVED (line 155: wasLoadingMore captured before try block)
• [suggestion] Unused pk parameter in mark_notifications_read — UNRESOLVED (minor; pk=None still present but unused since detail=False)
• [nitpick] Array index as v-for key — RESOLVED (key is now ${notification.id}-${notification.type})
• [nitpick] CommunityLibraryStatusChip mixes Options API with Composition API — UNRESOLVED (minor style inconsistency)
• [nitpick] getStartOfWeek locale assumption — RESOLVED (comment updated and logic changed: "Setting first day of the week to Monday")
• [nitpick] Typo "precisision" — RESOLVED (line 146 now reads "precision")
• [nitpick] Redundant new Date() wrapping in sort — RESOLVED (line 125: b.date - a.date without wrapping)
• [nitpick] flaggedNotification string uses { variable } with spaces — UNRESOLVED (minor i18n inconsistency)
• [nitpick] Test description mismatch — unable to verify (not re-raised)

No new blocking issues found. The majority of prior findings have been addressed. The remaining unresolved items are low-risk suggestions and nitpicks.

---

@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, UNRESOLVED, PARTIALLY_ADDRESSED, 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

[rtibblesbot]

suggestion: Carried from prior review — openLanguageModal() (line 234) sets this.drawer = false before opening the language modal, but showNotificationsModal does not close the drawer. Since the notifications modal is a full-screen immersive page, the drawer may remain visible underneath. Consider adding this.drawer = false; for consistency:

showNotificationsModal() {
  this.drawer = false;
  this.$router.push({
    query: {
      ...this.$route.query,
      modal: Modals.NOTIFICATIONS,
    },
  });
  this.trackClick('Notifications');
},

[rtibblesbot]

Well-implemented notifications feature. The majority of prior findings have been addressed.

CI: Python tests in progress. Video demo previously verified.

Delta from prior reviews

• [blocking] Variable shadowing bug in useFetch.js — RESOLVED (renamed to caughtError)
• [blocking] notify_update_to_channel_editors() called before super().save() for new submissions — RESOLVED (super().save() on line 2935 precedes notify_update_to_channel_editors on line 2939)
• [blocking] notify_users() reads stale in-memory objects after .update() — RESOLVED (lines 703-705 refresh queryset from DB)
• [blocking] Approval/rejection components pass notification.date_created instead of notification.date — RESOLVED (all three components now use notification.date)
• [blocking] Greatest(NULL, value) returns NULL on PostgreSQL — RESOLVED (Coalesce wraps F() to handle NULL)
• [blocking] NotificationFilters receives unused lastReadFilter prop — RESOLVED (prop removed; lastRead passed via queryParams)
• [blocking] "View more" buttons have no click handler — RESOLVED as intentional deferral
• [suggestion] useFetch.js data.value = null behavior change — RESOLVED (moved inside catch block)
• [suggestion] mark_notifications_read leaves in-memory field as Greatest expression — UNRESOLVED (low risk since viewset returns HTTP 204 immediately, but fragile if method is reused)
• [suggestion] WithNotificationIndicator screen reader support — RESOLVED (added visuallyhidden span)
• [suggestion] useStore missing guard for getCurrentInstance — RESOLVED (throws descriptive error)
• [suggestion] notify_update_to_channel_editors on resolve doesn't exclude the resolving admin — CONTESTED (developer explained acceptable for rare edge case)
• [suggestion] Date filter values computed once at component creation — UNRESOLVED (developer acknowledged as acceptable edge case)
• [suggestion] handleNotificationsRead timeout path leaves stale data — RESOLVED (timeout approach removed entirely)
• [suggestion] resolved_by_name may be null rendering literal "null" — RESOLVED (falls back to empty string)
• [suggestion] Drawer not closed when opening notifications modal — UNRESOLVED (see inline)
• [suggestion] Redundant @click handlers on VTabs — RESOLVED
• [suggestion] AppBar showNotificationsModal missing trackClick analytics — RESOLVED (line 237)
• [suggestion] NotificationList no fallback for unknown notification types — RESOLVED (|| 'div' fallback)
• [suggestion] Direct mutation of submission objects from API response — RESOLVED (spread copy)
• [suggestion] isLoadingMore state stale on error path — RESOLVED (wasLoadingMore captured)
• [suggestion] Unused pk parameter in mark_notifications_read — RESOLVED (not present in method signature)
• [nitpick] Array index as v-for key — RESOLVED (key is ${notification.id}-${notification.type})
• [nitpick] CommunityLibraryStatusChip mixes Options API with Composition API — RESOLVED (now fully <script setup>)
• [nitpick] getStartOfWeek locale assumption — RESOLVED (Monday-based, with comment)
• [nitpick] Typo "precisision" — RESOLVED
• [nitpick] Redundant new Date() wrapping in sort — RESOLVED
• [nitpick] flaggedNotification string uses { variable } with spaces — RESOLVED (uses {variable})
• [nitpick] Test description mismatch — not re-raised

No new blocking issues. The remaining unresolved items are low-risk suggestions carried from prior reviews.

---

@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, UNRESOLVED, PARTIALLY_ADDRESSED, 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

[rtibblesbot]

suggestion: (Carried from prior review) The drawer isn't closed when opening the notifications modal. openLanguageModal() on line 234 sets this.drawer = false before showing its modal, but showNotificationsModal() does not. The drawer will remain visible behind the immersive modal.

showNotificationsModal() {
  this.drawer = false;
  this.$router.push({

[rtibblesbot]

Well-implemented notifications feature. The architecture — composable-based data fetching, extensible notification type renderers, timestamp-based unread tracking — remains solid. The vast majority of prior findings have been addressed.

CI: Frontend tests passing. Build, linting, and Python unit tests in progress.

Delta from prior reviews

• [blocking] Variable shadowing bug in useFetch.js — RESOLVED (renamed to caughtError)
• [blocking] notify_update_to_channel_editors() called before super().save() for new submissions — RESOLVED (super().save() precedes notify_update_to_channel_editors)
• [blocking] notify_users() reads stale in-memory objects after .update() — RESOLVED (refreshed queryset from DB before creating change events)
• [blocking] Approval/rejection components pass notification.date_created instead of notification.date — RESOLVED (all use notification.date)
• [blocking] Greatest(NULL, value) returns NULL on PostgreSQL — RESOLVED (Coalesce wraps F() to handle NULL)
• [blocking] NotificationFilters receives unused lastReadFilter prop — RESOLVED (prop removed)
• [blocking] "View more" buttons have no click handler — RESOLVED as intentional deferral
• [suggestion] useFetch.js data.value = null behavior change — RESOLVED (moved inside catch block)
• [suggestion] mark_notifications_read leaves in-memory field as Greatest expression — UNRESOLVED (low risk: viewset returns HTTP 204 immediately and frontend optimistically updates session state, so the stale expression is never read)
• [suggestion] WithNotificationIndicator screen reader support — RESOLVED (added visuallyhidden span)
• [suggestion] useStore missing guard for getCurrentInstance — RESOLVED (throws descriptive error)
• [suggestion] notify_update_to_channel_editors on resolve doesn't exclude the resolving admin — CONTESTED (developer explained acceptable for rare edge case)
• [suggestion] Date filter values computed once at component creation — UNRESOLVED (developer acknowledged as acceptable; see inline)
• [suggestion] handleNotificationsRead timeout path leaves stale data — RESOLVED (timeout approach removed entirely)
• [suggestion] resolved_by_name may be null rendering literal "null" — RESOLVED (falls back to empty string)
• [suggestion] Drawer not closed when opening notifications modal — RESOLVED (MainNavigationDrawer sets this.drawer = false)
• [suggestion] Redundant @click handlers on VTabs — RESOLVED
• [suggestion] AppBar showNotificationsModal missing trackClick analytics — RESOLVED
• [suggestion] NotificationList no fallback for unknown notification types — RESOLVED (|| 'div' fallback)
• [suggestion] Direct mutation of submission objects from API response — RESOLVED (spread copy)
• [suggestion] isLoadingMore state stale on error path — RESOLVED (wasLoadingMore captured)
• [suggestion] Unused pk parameter in mark_notifications_read — RESOLVED
• [nitpick] Array index as v-for key — RESOLVED
• [nitpick] CommunityLibraryStatusChip mixes Options API with Composition API — RESOLVED (now fully <script setup>)
• [nitpick] getStartOfWeek locale assumption — RESOLVED (Monday-based, with comment)
• [nitpick] Typo "precisision" — RESOLVED
• [nitpick] Redundant new Date() wrapping in sort — RESOLVED
• [nitpick] flaggedNotification string uses { variable } with spaces — RESOLVED
• [nitpick] Test description mismatch — not re-raised

New findings

• suggestion (x1): useFetch isFinished never set to true on error — affects existing consumers (see inline)
• suggestion (x1): useFetch comment contradicts code in catch block (see inline)

No new blocking issues. The remaining unresolved items are low-risk suggestions from prior reviews.

---

@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, UNRESOLVED, PARTIALLY_ADDRESSED, 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

[rtibblesbot]

Well-implemented notifications feature. All prior blocking issues have been addressed.

CI: Python tests pending. Video demo previously verified across multiple reviews.

~28 prior finding(s) resolved.

Carried-forward unresolved:

• [suggestion] mark_notifications_read leaves in-memory field as Greatest expression — UNRESOLVED (low risk: viewset returns HTTP 204 immediately; fragile only if method is reused elsewhere)
• [suggestion] Date filter values computed once at component setup — UNRESOLVED (developer acknowledged as acceptable edge case)
• [suggestion] notify_update_to_channel_editors on resolve doesn't exclude resolving admin — CONTESTED (developer provided rationale: acceptable for rare edge case where resolver is also a channel editor)

No new blocking issues found.

---

@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, UNRESOLVED, PARTIALLY_ADDRESSED, 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

[rtibbles]

Some questions and comments, nothing blocking.

[rtibbles]

Nice that the effort of turning this into a composable from the previous mixin now means that it is also more broadly useful across the app!

[rtibbles]

Nice that the effort of turning this into a composable from a mixin, now means that we can reuse this in other parts of the app too!

[AlexVelezLl]

Yes!

[rtibbles]

Muy elegante!

[rtibbles]

How does registering components work with script setup? You just import it and it knows?

[AlexVelezLl]

Yes, it's beautiful!

[rtibbles]

Presumably the name is now just auto derived from the filename?

[AlexVelezLl]

Yes. Although it had some issues in Vue test utils if we wanted to do something like wrapper.findComponent({ name: 'NotificationsFilter' }), but the name appears correctly in the Vue browser plugin.

[rtibbles]

Good job we're switching over to Vue Testing Library then!

[rtibbles]

He said he would do it later!

[rtibbles]

I know it's an edge case, but might be worth filing a follow up issue for these values to get refreshed (similar to how we do for the ElapsedTime component in Kolibri). Exact cadence doesn't have to be hugely frequent - I just know some people keep their Studio page open in the browser for long periods (because of specific issues we've seen with the vuex and data syncing functionality that only happens in long lived sessions).

[rtibbles]

Not a blocker, and it was kind of like this when you got here, but using the DjangoJSONEncoder class might clean this up a little:

https://docs.djangoproject.com/en/3.2/topics/serialization/#djangojsonencoder

[AlexVelezLl]

Something like this? f0b08e3. Just tried it, and it seems to be working fine

[rtibbles]

Yes!

[rtibbles]

What viewset does this map to? Not sure I'm completely understanding the data flow here.

[AlexVelezLl]

Added!

[rtibblesbot]

Well-implemented notifications feature. All prior blocking issues resolved.

CI: JS tests, linting, build passing. Python tests in progress.

~30 prior finding(s) resolved.

Carried-forward:

• [suggestion] mark_notifications_read leaves in-memory field as Greatest expression — UNRESOLVED (low risk: viewset returns HTTP 204 immediately and the instance is discarded)
• [suggestion] Date filter values computed once at component setup — CONTESTED (maintainer acknowledged as edge case, requested follow-up issue be filed)
• [suggestion] notify_update_to_channel_editors on resolve doesn't exclude resolving admin — CONTESTED (developer explained, maintainer did not re-raise)
• [suggestion] useFetch isFinished never set to true on error — CONTESTED (developer confirmed intentional behavior)

No new blocking issues. Recent commits (DjangoJSONEncoder in _json_dumps, SESSION comment in ALL_TABLES) are maintainer-requested changes and 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, UNRESOLVED, PARTIALLY_ADDRESSED, 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

[rtibblesbot]

Well-implemented notifications feature. All prior blocking issues resolved; recent commits address @rtibbles' feedback (DjangoJSONEncoder, SESSION comment).

CI passing.

~30 prior finding(s) resolved.

Carried-forward unresolved:

• [suggestion] mark_notifications_read leaves in-memory field as Greatest expression — UNRESOLVED (low risk: viewset returns HTTP 204 immediately and the instance is discarded; fragile only if method is reused elsewhere)
• [suggestion] Date filter values computed once at component setup — UNRESOLVED (developer acknowledged as acceptable; @rtibbles asked for a follow-up issue to refresh values in long-lived sessions — developer requested bot file it)

Carried-forward contested:

• [suggestion] notify_update_to_channel_editors on resolve doesn't exclude resolving admin — CONTESTED (developer provided rationale, accepted by team)

No new blocking issues found.

---

@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, UNRESOLVED, PARTIALLY_ADDRESSED, 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