Skip to content

feat: decouple notifications panel using widget registry mechanism - #1885

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

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

Conversation

@awais-ansari

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

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Mar 26, 2026

Copy link
Copy Markdown

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
@awais-ansari awais-ansari mentioned this pull request Mar 27, 2026

@arbrandes arbrandes left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread src/external-widgets/learning-upgrade-widget/package.json Outdated
Comment thread jest.config.js Outdated
Comment thread src/courseware/course/sidebar/constants.js Outdated
Comment thread src/courseware/course/sidebar/defaultWidgets.js Outdated
Comment thread src/courseware/course/sidebar/defaultWidgets.js Outdated
Comment thread src/courseware/course/sidebar/ARCHITECTURE.md Outdated
Comment thread src/courseware/course/sidebar/ARCHITECTURE.md Outdated
Comment thread src/courseware/course/sidebar/ARCHITECTURE.md
Comment thread docs/decisions/0010-upgrade-widget-extraction.md Outdated
Comment thread docs/decisions/0010-upgrade-widget-extraction.md Outdated
@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

@arbrandes arbrandes left a comment

Copy link
Copy Markdown
Contributor

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

@arbrandes arbrandes left a comment

Copy link
Copy Markdown
Contributor

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
@awais-ansari
awais-ansari deleted the aansari/hide-upsell-1874 branch April 13, 2026 18:53
@pdpinch

pdpinch commented Apr 16, 2026

Copy link
Copy Markdown

This change broke our build of the MFE. Since this involved changing the name/api for a pluginslot, it should have gone through the DEPR process and gotten some more communication.

In particular, what is the plan for addressing this in the Verawood release?

@arbrandes

arbrandes commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

@pdpinch, you're right, we did not consider backward-compatibility at all, here. I'm going to bring this up with the Axim engineering team, today.

arbrandes added a commit to arbrandes/frontend-app-learning that referenced this pull request Apr 16, 2026
@arbrandes

Copy link
Copy Markdown
Contributor

Revert PR: #1898.

arbrandes added a commit to arbrandes/frontend-app-learning that referenced this pull request Apr 16, 2026
Reverts the following two commits:

- 59113b0 feat: move discussion topic prefetch from trigger to widget
  config lifecycle (openedx#1897)
- 0664dc3 feat: decouple notifications panel using widget registry
  mechanism (openedx#1885)

Co-Authored-By: Claude <noreply@anthropic.com>
@arbrandes

Copy link
Copy Markdown
Contributor

@pdpinch, instead of reverting it, we added backward-compatibility:

#1899

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.

4 participants