feat: decouple notifications panel using widget registry mechanism#1885
feat: decouple notifications panel using widget registry mechanism#1885arbrandes merged 7 commits intoopenedx:masterfrom
Conversation
005f525 to
e50a017
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1885 +/- ##
==========================================
+ Coverage 91.16% 91.27% +0.11%
==========================================
Files 349 343 -6
Lines 5830 5766 -64
Branches 1389 1387 -2
==========================================
- Hits 5315 5263 -52
+ Misses 498 484 -14
- Partials 17 19 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@brian-smith-tcril Please review this PR. |
e50a017 to
1b6e9df
Compare
arbrandes
left a comment
There was a problem hiding this comment.
First off: this is amazingly well documented. Congrats, and thank you! I'm probably going to start using this PR as a benchmark for how complicated frontend architectural decisions should be implemented.
Second: this is mostly an architecture review based on the ADR, ARCHITECTURE.md,and a glance at USE_CASE_VERIFICATION.md. I have not looked closely at the code, yet.
Lastly, I'm coming at this "from the future". Prior to the Willow cutoff in October of this year, frontend-app-learning will be converted to frontend-base, and this is one of the situations where the new slot architecture will help (or, at least, affect). Some of the inline comments are just FYIs related to that, and a couple are meant to help with the transition later. Either way, we should keep this in mind.
42d3e87 to
1cf6eb5
Compare
a635248 to
42e6974
Compare
There was a problem hiding this comment.
Thanks for addressing my previous concerns (rename to widgets/, drop separate package, etc.). I tested this and it works - at least for the Dicussions widget and the outline. Nice!
Looking at the code as it stands now (with some help from Claude), this is what I found:
1. Edge-case bug: Mismatch between what sidebar is "rendered" vs what is "open" for layout purposes
Sidebar.jsx:8 renders any widget in the SIDEBARS registry (all enabled widgets), and SidebarTriggers.jsx:23 renders triggers for all of them too. But useIsSidebarOpen() and upgradePanelVisible check availableSidebarIds, which only includes widgets whose isAvailable() returns true.
So if a widget's trigger doesn't self-hide (like UpgradeTrigger), a user can open a panel that's physically rendered but that layout code considers "closed". Example: Upgrade is registered but the course has no verified track - the trigger shows, clicking it opens the panel, but sequence navigation tabs don't shrink to accommodate it.
Fix: either filter SidebarTriggers by availableSidebarIds, or have useIsSidebarOpen() check SIDEBARS instead. The first is simpler.
2. Warning: The old notificationTrayVisible was dead code - this PR activates that layout logic
On master, LockPaywall destructures notificationTrayVisible from SidebarContext, but the old provider never sets it - it was always undefined. The layout variables that depended on it (shouldDisplayGatedContentOneColumn, etc.) were dead code.
The new code replaces it with upgradePanelVisible = availableSidebarIds.includes(currentSidebar), which CAN be true, activating layout adjustments that previously never triggered. The variable name is also misleading: it's true for any open sidebar widget (including Discussions), not specifically the upgrade panel.
I have not tested this: have you? (Is there even a way for us to test it?)
3. Nit: No-op state call in useUnitShiftBehavior.js
useUnitShiftBehavior.js:122-124:
} else if (currentWidget) {
// Current RIGHT sidebar panel still available and no higher priority - keep it
setCurrentSidebar(currentSidebar); // no-opsetCurrentSidebar(currentSidebar) with the same value is a no-op (React bails out). The adjacent branches call setSidebarId too, making this one look like a missing call. Removing the line or adding a comment would clarify intent.
4. Nit: Dead guard in useSidebarSync.js
if (shouldDisplayFullScreen || previousUnitIdRef.current === null) {
return;
}React runs effects in hook declaration order. Since useUnitShiftBehavior is called before useSidebarSync, the ref is always set to unitId before this check runs. The === null branch is unreachable.
5. Nit: misleading variable name in SequenceNavigation.jsx
shouldDisplayUpgradeTriggerInSequence is just a small-screen breakpoint check unrelated to the upgrade widget. Consider a generic name like isSmallScreen.
6. Codebase layout: course-outline doesn't belong in src/widgets/
src/widgets/course-outline/ is tightly coupled to the sidebar framework via WIDGETS.COURSE_OUTLINE constants, dedicated plugin slots, and special-case logic in all three sidebar hooks. It's not pluggable - removing the directory would break the core layout. Placing it in src/widgets/ alongside truly decoupled widgets like Upgrade implies a level of decoupling that doesn't exist.
(It's fine to leave Discussions in there, as it is a properly independent widget - just a default one.)
1cd5047 to
f69d24d
Compare
df0c9b5 to
d041eb4
Compare
arbrandes
left a comment
There was a problem hiding this comment.
Alright, I think this is good enough to go. Thanks for bearing with me!
Summary
Removes the tightly coupled upgrade/upsell ("notifications") panel from the Learning MFE core and replaces it with a pluggable widget registry system. The right sidebar now supports dynamically-registered external widgets, making it easy to add, remove, or replace sidebar panels without forking the MFE. Proposal
Motivation
The built-in upgrade panel (NotificationTray) caused several issues:
Architecture: Pluggable Widget Registry
The right sidebar now operates on a widget registry (SIDEBARS) built at runtime from:
Built-in widgets: only DISCUSSIONS are included by default.
External widgets: installed via npm and registered via SIDEBAR_WIDGETS in env.config.jsx
Each widget provides:
Rename: notifications → upgrade
All internal references to the old "Notifications" panel are renamed for clarity:
Extracted Upgrade Widget Package
The upgrade panel is extracted from the sidebar core into
upgradewidget. It is no longer bundled by default. Instances that need it register it via env.config.jsx.Configuration
When the upgrade widget is promoted to its own npm package @edx/learning-upgrade-widget, operators will install it and update the import path. No other changes needed.
Two-Sidebar Collaboration
The architecture docs in ARCHITECTURE.md fully describe the shared currentSidebar state between the LEFT (Course Outline) and RIGHT (widget panels) sidebars, including:
Removed
Testing
Migration Notes
SIDEBAR_WIDGETS: [upgradeWidgetConfig]to env.config.jsx.- Replace NotificationTraySlot / NotificationsDiscussionsSidebarSlot with RightSidebarSlot. Replace
- Replace NotificationsDiscussionsTriggerSlot with RightSidebarTriggerSlot.