Skip to content

feat: decouple notifications panel using widget registry mechanism#1885

Merged
arbrandes merged 7 commits intoopenedx:masterfrom
awais-ansari:aansari/hide-upsell-1874
Apr 8, 2026
Merged

feat: decouple notifications panel using widget registry mechanism#1885
arbrandes merged 7 commits intoopenedx:masterfrom
awais-ansari:aansari/hide-upsell-1874

Conversation

@awais-ansari
Copy link
Copy Markdown
Contributor

@awais-ansari awais-ansari commented Mar 26, 2026

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:

  1. Misleading naming: "Notifications" implied platform-level notifications, conflicting with the separate notification tray in the header.
  2. Non-Core: The upgrade panel is a non-core feature. It should not be bundled into the Open edX core.
  3. Empty panel UX problem: In courses without a paid track, the panel rendered empty but remained visible.

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:

{
  id: string,
  priority: number, 
  Sidebar: ReactComponent,   
  Trigger: ReactComponent, 
  isAvailable: (context) => boolean,
  enabled: boolean,
  Provider?: ReactComponent, 
}

Rename: notifications → upgrade

All internal references to the old "Notifications" panel are renamed for clarity:

  1. WIDGETS.NOTIFICATIONS → WIDGETS.UPGRADE
  2. NotificationTray → UpgradePanel
  3. NotificationTrigger → UpgradeTrigger
  4. NotificationIcon → UpgradeIcon
  5. notificationStatus context → upgradeWidgetStatus
  6. LocalStorage keys updated accordingly

Extracted Upgrade Widget Package

The upgrade panel is extracted from the sidebar core into upgrade widget. It is no longer bundled by default. Instances that need it register it via env.config.jsx.

Configuration

import { upgradeWidgetConfig } from './src/widgets/upgrade/src/index';

const config = {
  SIDEBAR_WIDGETS: [upgradeWidgetConfig],
};

export default config;

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:

  • Priority cascade: DISCUSSIONS (10) → UPGRADE (20) → external widgets → COURSE_OUTLINE (fallback)
  • Auto-open logic on initial load
  • Unit shift behavior for each scenario
  • State persistence via localStorage / sessionStorage

Removed

  • src/courseware/course/new-sidebar - Entire unused new-sidebar directory
  • src/courseware/course/sidebar/sidebars/notifications - Built-in notification widget
  • NotificationTraySlot
  • NotificationWidgetSlot
  • NotificationsDiscussionsSidebarSlot - Replaced by RightSidebarSlot
  • NotificationsDiscussionsTriggerSlot - Replaced by RightSidebarTriggerSlot

Testing

  • 12 new test suites covering hooks, defaultWidgets, storage utils, SidebarContextProvider, SidebarTriggers, SidebarBase, UpgradePanel, UpgradeTrigger, UpgradeIcon, UpgradeWidgetContext, and upgrade utils
  • All use cases are documented in USE_CASE_VERIFICATION.md

Migration Notes

  • No breaking changes for instances that don't use the upgrade widget. Discussions and course outline work are identical.
  • Instances using the upgrade panel: Add SIDEBAR_WIDGETS: [upgradeWidgetConfig] to env.config.jsx.
  • Plugin slot consumers:
    - Replace NotificationTraySlot / NotificationsDiscussionsSidebarSlot with RightSidebarSlot. Replace
    - Replace NotificationsDiscussionsTriggerSlot with RightSidebarTriggerSlot.
  • localStorage: Old notificationStatus.{courseId} keys are not migrated to new key format upgradeWidget.{courseId} starts fresh.

@awais-ansari awais-ansari force-pushed the aansari/hide-upsell-1874 branch from 005f525 to e50a017 Compare March 26, 2026 19:18
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 98.07074% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.27%. Comparing base (d77f39f) to head (8959969).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...eware/course/sequence/lock-paywall/LockPaywall.jsx 84.61% 2 Missing ⚠️
src/courseware/course/sidebar/SidebarContext.js 50.00% 1 Missing ⚠️
...urseware/course/sidebar/SidebarContextProvider.jsx 97.29% 1 Missing ⚠️
src/courseware/course/sidebar/utils/storage.js 96.55% 1 Missing ⚠️
src/widgets/upgrade/src/widgetConfig.js 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@awais-ansari
Copy link
Copy Markdown
Contributor Author

@brian-smith-tcril Please review this PR.

@awais-ansari awais-ansari marked this pull request as ready for review March 26, 2026 20:35
@awais-ansari awais-ansari force-pushed the aansari/hide-upsell-1874 branch from e50a017 to 1b6e9df Compare March 27, 2026 11:45
Copy link
Copy Markdown
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@awais-ansari awais-ansari force-pushed the aansari/hide-upsell-1874 branch from 42d3e87 to 1cf6eb5 Compare March 30, 2026 19:17
@awais-ansari awais-ansari requested a review from arbrandes March 30, 2026 19:29
@awais-ansari awais-ansari force-pushed the aansari/hide-upsell-1874 branch from a635248 to 42e6974 Compare March 30, 2026 22:58
Copy link
Copy Markdown
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-op

setCurrentSidebar(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

useSidebarSync.js:39:

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.)

@awais-ansari awais-ansari force-pushed the aansari/hide-upsell-1874 branch from 1cd5047 to f69d24d Compare April 6, 2026 13:01
@awais-ansari awais-ansari force-pushed the aansari/hide-upsell-1874 branch from df0c9b5 to d041eb4 Compare April 7, 2026 08:23
@awais-ansari awais-ansari requested a review from arbrandes April 8, 2026 12:19
Copy link
Copy Markdown
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I think this is good enough to go. Thanks for bearing with me!

@arbrandes arbrandes merged commit 0664dc3 into openedx:master Apr 8, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants