You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: show the block title only when the learner can see it on the page - #263
The drawer labelled itself with blockDisplayName. For most block types that is a Studio-only authoring note ("Wk3 intro copy - REVISED, do not reuse") that never appears in courseware, so the panel was showing learners an internal label.
Reads the new visibleTitle payload field for the subheader instead. The trigger sends it only for block types that actually render their name, so the decision lives with the side that knows the block.
blockDisplayName is still POSTed as block_display_name, unchanged — that's how the content team locates the component.
With no visible title the subheader names the block type, and an unmapped type now falls back to "this content" rather than echoing the raw slug.
Doesn't append a second terminator to a title that already ends a sentence.
The last bullet is outside the ticket's four criteria — it surfaced while testing the first three and is here because it's two lines in the same expression. Say the word if you'd rather it went separately.
Implementation details
FRIENDLY_BLOCK_TYPES previously did FRIENDLY_BLOCK_TYPES[blockType] ?? blockType, which rendered the raw type for anything unmapped — a learner on a custom block would read "about this ol_openedx_chat_xblock block". The lookup now resolves to null when unmapped.
Two things about that lookup are deliberate. The own-key check via hasOwnProperty is needed because a plain object literal returns real values for constructor, __proto__, and toString; without it, blockType: "constructor" would render the Object source text into the question. And both blockType and visibleTitle arrive over a cross-origin postMessage, so the sender controls their type — the typeof checks keep a non-string from throwing on .trim() mid-render, which would blank the panel since the bundle has no error boundary.
FRIENDLY_BLOCK_TYPES gains videoalpha, annotatable, pdf, edx_sga, and staffgradedxblock so it covers every type the trigger's own map can send a title for, plus poll_question — the built-in poll draws its question text rather than its name (its extracted template renders only a JSON config div), so it never sends a title and this wording is the only one it can get. Those blocks land on this wording whenever their title comes through blank, and without the entries they'd read "this content". A test enumerates all 14 of the trigger's types so the two maps can't drift apart silently.
The terminator: an author can write a question into the Studio name, and some types render that name as their heading — the third-party poll puts display_name in an <h3> and keeps the question in a separate field — so a title can arrive as "Which approach did you prefer?" and read "...prefer??".
Screenshots (if appropriate):
Video — title shown.video renders its name as a heading, so the panel reuses the words the learner can already see.
Problem — title shown. Same path for a problem, whose name is the Quiz 1: Base cases heading above the choices.
Poll (the third-party poll) — title shown, and not double-terminated. The <h3> is display_name and the question is a separate field below it, so an author can put a question in the name. The panel reads "…prefer?", not "…prefer??".
Survey — title comes from block_name.survey renders block_name as its heading, and that heading is what the panel names — not display_name.
Text block — falls back to the block type. This block's Studio name is Wk3 intro copy - REVISED, do not reuse. It appears nowhere on the page and nowhere in the panel, which reads "about this text?". This is the leak the ticket is about. ← AC1
Built-in poll — falls back.poll_question draws its question text rather than its name, so it sends no title and the panel names the type.
Problem with an empty Studio name — falls back. A mapped type still degrades to block-type wording when the name is blank.
Unmapped type — generic wording.done is in neither map, so the panel says "about this content?" instead of echoing the raw slug.
The two Storybook stories cover the same split without a harness — Slot (block title in subheader) and Slot (block type in subheader).
How can this be tested?
Automated:nvm use 24 && yarn jest src/bundles/FeedbackDrawer — 80 pass. yarn typecheck passes and yarn build:bundles:feedback builds.
The new tests cover the split directly: FeedbackDrawerManager.test.tsx opens the drawer for an html block whose blockDisplayName is "Wk3 intro copy - REVISED, do not reuse" with no visibleTitle, asserts the panel says "about this text?" and that REVISED appears nowhere in the DOM, then submits and asserts the Studio name is still in the POST body. The shared PAYLOAD now uses different values for the two fields, so the subtitle test can only pass by reading visibleTitle. A parity test enumerates all 14 of the trigger's block types so the two maps can't drift apart silently.
Drawer only (no harness) — nvm use 24 && yarn install && yarn start (Storybook on http://localhost:6006), then smoot-design → Feedback → FeedbackDrawer:
Slot (block title in subheader) — video with a visible title → "…about Lecture 3: Recursion?"
Slot (block type in subheader) — problem with none → "…about this problem?"
Manual — to exercise the full megaphone → drawer loop locally:
Set up a Tutor instance locally and mount frontend-app-learning (run its dev server).
Enable the trigger: install the companion ol_openedx_feedback plugin (open-edx-plugins#876) in the LMS and turn on the ol_openedx_feedback.feedback_enabled CourseWaffleFlag (globally or per course). This branch reads visibleTitle, which only that PR's trigger sends — against the released plugin every block falls back to block-type wording.
Wire the feedback drawer into the Learning MFE's right-sidebar slot (SidebarAIDrawerCoordinator mounts it inline). In .env.development set DEPLOYMENT_NAME='mitxonline' and ENABLE_AI_DRAWER_SLOT='true' (feedback shares AskTIM's sidebar mount).
Build this branch and copy the bundle into the MFE (paths assume the two repos are siblings):
The MFE loads it from /static/smoot-design/feedbackDrawerManager.es.js (override with FEEDBACK_DRAWER_BUNDLE_PATH).
Author a unit with one block per case — a video or word_cloud with a Studio name, a text/HTML block, and a built-in poll. Give the text block a deliberately author-only name like Wk3 intro copy - REVISED, do not reuse so a leak is obvious.
Open the unit as a signed-in learner and click the 📣 on each block:
Title path — the video / word cloud panel reads What kind of feedback do you have about <the block's title>? ← AC2
Fallback path — the text block reads "…about this text?" and the poll "…about this poll?", with no Studio name anywhere in the panel. ← AC1
Record still carries it — submit from the text block and confirm the mit-learn row still has block_display_name set to the Studio name. ← AC4
Steps 5–6 were run on a Tutor LMS with the paired plugin installed — the screenshots above are that run, one shot per case. drag-and-drop-v2's show_title toggle is covered by unit tests in the paired PR only.
The terminator check only permits one trailing quote, so titles whose existing punctuation is followed by a closing bracket or nested quotes still get a second question mark (for example, Recursion (why?) becomes ...why?)?). Treat a sequence of closing punctuation as trailing characters while still allowing Midterm (offline) to receive its question mark.
The drawer named the block with the Studio display_name, which for most block
types is an authoring label the learner never sees ("Wk3 intro copy - REVISED,
do not reuse"). It now reads the new `visibleTitle` payload field, which the
LMS trigger sets only for block types that actually draw a title, and names the
block type when there is none.
Alongside that:
- Name more block types, so the fallback reads "this PDF" or "this assignment"
rather than a raw slug, and never leaks an unmapped slug.
- Treat `subtitle` and `blockType` as untrusted. Both arrive over postMessage
and this bundle has no error boundary, so a non-string `subtitle` threw
mid-render and blanked the panel, and `blockType: "constructor"` resolved to
an Object.prototype member.
- Don't append a second terminator to a title that already ends a sentence.
poll renders the Studio name as its question, so authors write questions
there and the subheader read "...which do you prefer??". A trailing period is
excluded: it's usually numbering ("Ch. 4."), and swallowing the question mark
left the reaction radiogroup's accessible name a statement.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
arslanashraf7
added
Waiting on Author
and removed
Needs Review
What are the relevant tickets?
Part of https://github.com/mitodl/hq/issues/13643
Paired with mitodl/open-edx-plugins#876, which sends the new field.
Description (What does it do?)
The drawer labelled itself with
blockDisplayName. For most block types that is a Studio-only authoring note ("Wk3 intro copy - REVISED, do not reuse") that never appears in courseware, so the panel was showing learners an internal label.visibleTitlepayload field for the subheader instead. The trigger sends it only for block types that actually render their name, so the decision lives with the side that knows the block.blockDisplayNameis still POSTed asblock_display_name, unchanged — that's how the content team locates the component.The last bullet is outside the ticket's four criteria — it surfaced while testing the first three and is here because it's two lines in the same expression. Say the word if you'd rather it went separately.
Implementation details
FRIENDLY_BLOCK_TYPESpreviously didFRIENDLY_BLOCK_TYPES[blockType] ?? blockType, which rendered the raw type for anything unmapped — a learner on a custom block would read "about this ol_openedx_chat_xblock block". The lookup now resolves tonullwhen unmapped.Two things about that lookup are deliberate. The own-key check via
hasOwnPropertyis needed because a plain object literal returns real values forconstructor,__proto__, andtoString; without it,blockType: "constructor"would render theObjectsource text into the question. And bothblockTypeandvisibleTitlearrive over a cross-originpostMessage, so the sender controls their type — thetypeofchecks keep a non-string from throwing on.trim()mid-render, which would blank the panel since the bundle has no error boundary.FRIENDLY_BLOCK_TYPESgainsvideoalpha,annotatable,pdf,edx_sga, andstaffgradedxblockso it covers every type the trigger's own map can send a title for, pluspoll_question— the built-in poll draws its question text rather than its name (its extracted template renders only a JSON config div), so it never sends a title and this wording is the only one it can get. Those blocks land on this wording whenever their title comes through blank, and without the entries they'd read "this content". A test enumerates all 14 of the trigger's types so the two maps can't drift apart silently.The terminator: an author can write a question into the Studio name, and some types render that name as their heading — the third-party
pollputsdisplay_namein an<h3>and keeps the question in a separate field — so a title can arrive as "Which approach did you prefer?" and read "...prefer??".Screenshots (if appropriate):
Video — title shown.
videorenders its name as a heading, so the panel reuses the words the learner can already see.Problem — title shown. Same path for a
problem, whose name is theQuiz 1: Base casesheading above the choices.Poll (the third-party
poll) — title shown, and not double-terminated. The<h3>isdisplay_nameand the question is a separate field below it, so an author can put a question in the name. The panel reads "…prefer?", not "…prefer??".Survey — title comes from
block_name.surveyrendersblock_nameas its heading, and that heading is what the panel names — notdisplay_name.Text block — falls back to the block type. This block's Studio name is
Wk3 intro copy - REVISED, do not reuse. It appears nowhere on the page and nowhere in the panel, which reads "about this text?". This is the leak the ticket is about. ← AC1Built-in poll — falls back.
poll_questiondraws its question text rather than its name, so it sends no title and the panel names the type.Problem with an empty Studio name — falls back. A mapped type still degrades to block-type wording when the name is blank.
Unmapped type — generic wording.
doneis in neither map, so the panel says "about this content?" instead of echoing the raw slug.The two Storybook stories cover the same split without a harness — Slot (block title in subheader) and Slot (block type in subheader).
How can this be tested?
Automated:
nvm use 24 && yarn jest src/bundles/FeedbackDrawer— 80 pass.yarn typecheckpasses andyarn build:bundles:feedbackbuilds.The new tests cover the split directly:
FeedbackDrawerManager.test.tsxopens the drawer for anhtmlblock whoseblockDisplayNameis"Wk3 intro copy - REVISED, do not reuse"with novisibleTitle, asserts the panel says "about this text?" and thatREVISEDappears nowhere in the DOM, then submits and asserts the Studio name is still in the POST body. The sharedPAYLOADnow uses different values for the two fields, so the subtitle test can only pass by readingvisibleTitle. A parity test enumerates all 14 of the trigger's block types so the two maps can't drift apart silently.Drawer only (no harness) —
nvm use 24 && yarn install && yarn start(Storybook on http://localhost:6006), then smoot-design → Feedback → FeedbackDrawer:videowith a visible title → "…about Lecture 3: Recursion?"problemwith none → "…about this problem?"Manual — to exercise the full megaphone → drawer loop locally:
frontend-app-learning(run its dev server).ol_openedx_feedbackplugin (open-edx-plugins#876) in the LMS and turn on theol_openedx_feedback.feedback_enabledCourseWaffleFlag (globally or per course). This branch readsvisibleTitle, which only that PR's trigger sends — against the released plugin every block falls back to block-type wording.SidebarAIDrawerCoordinatormounts it inline). In.env.developmentsetDEPLOYMENT_NAME='mitxonline'andENABLE_AI_DRAWER_SLOT='true'(feedback shares AskTIM's sidebar mount)./static/smoot-design/feedbackDrawerManager.es.js(override withFEEDBACK_DRAWER_BUNDLE_PATH).videoorword_cloudwith a Studio name, a text/HTML block, and a built-in poll. Give the text block a deliberately author-only name likeWk3 intro copy - REVISED, do not reuseso a leak is obvious.What kind of feedback do you have about <the block's title>?← AC2block_display_nameset to the Studio name. ← AC4Steps 5–6 were run on a Tutor LMS with the paired plugin installed — the screenshots above are that run, one shot per case.
drag-and-drop-v2'sshow_titletoggle is covered by unit tests in the paired PR only.Additional Context