Skip to content

Conversation

@Lunyachek
Copy link
Contributor

Description

In Course Overview field we can see incorrect text typing direction

Screen.Recording.2025-09-08.at.16.30.55.mov

Fix was found in tinymce-react issues section - tinymce/tinymce-react#267

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 9, 2025
@openedx-webhooks
Copy link

Thanks for the pull request, @Lunyachek!

This repository is currently maintained by @bradenmacdonald.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.


Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@codecov
Copy link

codecov bot commented Sep 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.64%. Comparing base (720b591) to head (db663c2).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #2437    +/-   ##
========================================
  Coverage   94.64%   94.64%            
========================================
  Files        1187     1187            
  Lines       26220    26222     +2     
  Branches     5679     5821   +142     
========================================
+ Hits        24817    24819     +2     
+ Misses       1344     1333    -11     
- Partials       59       70    +11     

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

Comment on lines 113 to 135
<WysiwygEditor
initialValue={overview}
value={overview}
onChange={(value) => onChange(value, 'overview')}
/>
Copy link
Contributor

@jacobo-dominguez-wgu jacobo-dominguez-wgu Sep 10, 2025

Choose a reason for hiding this comment

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

I tested on my local and the typing direction issue was fixed, but it adds a different issue: the editor does not show the initial value even the passed data on the value param is set.
The docs indicate that the initualValue should not be modified by the onChange so by doing the next we can fix the issue and keep the initialValue:

const [initialOverview] = React.useState(overview);

<WysiwygEditor
    initialValue={initialOverview}
    value={overview}
    onChange={(value) => onChange(value, 'overview')}
/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! You were right, I fixed initialValue with useState and added separate variables for both overview and aboutSidebarHtml, so the editors now correctly display their initial values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing it!

<Form.Group className="form-group-custom">
<Form.Label>{intl.formatMessage(messages.courseAboutSidebarLabel)}</Form.Label>
<WysiwygEditor
initialValue={aboutSidebarHtml}
Copy link
Contributor

Choose a reason for hiding this comment

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

The same applies here.

@mphilbrick211 mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Sep 10, 2025
@jacobo-dominguez-wgu
Copy link
Contributor

jacobo-dominguez-wgu commented Sep 11, 2025

The check is failing because the last commit is not following the commit convention, can you please edit it?

@Lunyachek Lunyachek force-pushed the lunyachek/fix/course-overview-field-wrong-text-direction branch from a9705ce to db663c2 Compare September 11, 2025 21:54
@Lunyachek
Copy link
Contributor Author

The check is failing because the last commit is not following the commit convention, can you please edit it?

Fixed

Copy link
Contributor

@jacobo-dominguez-wgu jacobo-dominguez-wgu left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Sep 13, 2025

Thanks for tackling this bug! It seems like a serious issue. Unfortunately, this fix is causing a different bug for me: The Course Overview field is now usable, but it's blank - the initial contents are missing completely.

Screenshot 2025-09-12 at 5 27 42 PM

I believe what's happening is this:

  • The <IntroducingSection> component is rendered while the course data is still loading. It stores "" into the initialOverview state.
  • The course data loads and the overview prop of <IntroducingSection> is updated.
  • The WYSIWYG editor is not updated, because its initialValue has not changed. It stays empty.

I'm not sure why that seems to be only happening on my machine though.

I think that to fix this, we need to have some information about whether or not overview has been fully loaded yet.

@Lunyachek
Copy link
Contributor Author

@bradenmacdonald Hello! I've created a new course and I can see the initial value for the Course Overview field. Or did I misunderstand what you meant?

2025-09-22.10.56.57.mov

@bradenmacdonald
Copy link
Contributor

@Lunyachek Yep, that's it. It seems to be working for you, but not for me. I put an explanation of what I think the bug is above, and it could result in a flaky bug that sometimes happens and sometimes doesn't. Or otherwise there is something wrong with my environment. Maybe you can rebase the PR and we can both try again? If you also log the value of initialOverview at different points in the rendering I could compare it to mine and figure out why it's happening.

@Lunyachek
Copy link
Contributor Author

Lunyachek commented Sep 29, 2025

@bradenmacdonald Here are my logs from running the component locally:

[IntroducingSection] initialOverview (init): 
[IntroducingSection] initialOverview on mount: 
[IntroducingSection] overview changed: <section class="about">...</section>
[IntroducingSection] initialOverview vs overview equal?: false

On init/mount, initialOverview is indeed empty (''), since it takes the snapshot before the async overview data arrives.

When overview is later passed down with the real HTML, initialOverview !== overview (as expected, because useState keeps the first snapshot).

The WysiwygEditor still receives the correct value prop with the loaded HTML.

So on my side the behavior is stable — the editor initializes and then updates correctly once overview is populated.

I rebased and temporarily pushed my logs to this PR (I'll delete them later)

@Lunyachek Lunyachek force-pushed the lunyachek/fix/course-overview-field-wrong-text-direction branch from db663c2 to 79cb2b4 Compare September 29, 2025 17:05
@bradenmacdonald
Copy link
Contributor

Thanks for pushing those logs @Lunyachek. Here's what I see:

[IntroducingSection] initialOverview (init): 
hook.js:377 [IntroducingSection] initialOverview (init): 
[IntroducingSection] initialOverview on mount: 
index.jsx:82 [IntroducingSection] overview changed: 
index.jsx:83 [IntroducingSection] initialOverview vs overview equal?: true
index.jsx:78 [IntroducingSection] initialOverview on mount: 
index.jsx:82 [IntroducingSection] overview changed: 
index.jsx:83 [IntroducingSection] initialOverview vs overview equal?: true
index.jsx:73 [IntroducingSection] initialOverview (init): 
hook.js:377 [IntroducingSection] initialOverview (init): 
[IntroducingSection] overview changed: <section class="about"><h2>About This Course</h2>...</section>
index.jsx:83 [IntroducingSection] initialOverview vs overview equal?: false
index.jsx:73 [IntroducingSection] initialOverview (init): 
hook.js:377 [IntroducingSection] initialOverview (init): 
index.jsx:73 [IntroducingSection] initialOverview (init): 
hook.js:377 [IntroducingSection] initialOverview (init): 
index.jsx:73 [IntroducingSection] initialOverview (init): 
hook.js:377 [IntroducingSection] initialOverview (init): 
Screenshot 2025-09-29 at 4 09 26 PM Screenshot 2025-09-29 at 4 10 27 PM

Whereas here's what it looked like before this PR (the text direction is wrong, but the content does appear):

Screenshot 2025-09-29 at 4 12 34 PM Screenshot 2025-09-29 at 4 13 30 PM

initialValue={overview}
onChange={(value) => onChange(value, 'overview')}
initialValue={initialOverview}
value={overview}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong - as far as I can tell (and TypeScript can tell), WysiwygEditor does not accept a value prop.

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Sep 29, 2025

I still think the problem is initialOverview - the current value of initialOverview needs to be captured/saved when the courseDetails data is loaded, not when the IntroducingSection component happens to first render.

If IntroducingSection renders before courseDetails is loaded: you get the bug.
If the courseDetails is loaded before IntroducingSection renders: no bug.

Notice that on your system we see these logs:

[IntroducingSection] initialOverview on mount: 
[IntroducingSection] overview changed: <section class="about">...</section>

So the value of initialOverview gets frozen (in the state) correctly.

But on mine we see:

[IntroducingSection] initialOverview on mount: 
[IntroducingSection] overview changed: 
[IntroducingSection] overview changed: <section class="about"><h2>About This Course</h2>...</section>

So the value of initialOverview gets frozen (in the state) wrong to be an empty string.

Sometimes the bug is gone, for example if I hot-reload just the IntroducingSection component (so that courseDetails is already loaded), I don't see the bug. But if I refresh the page, I do see the bug.

@jacobo-dominguez-wgu
Copy link
Contributor

I could not reproduce @bradenmacdonald's problem neither but I think something like this may solve it, give it a try on your env @Lunyachek.
File: src/schedule-and-details/index.jsx line 280

<IntroducingSection
    overview={loadingDetailsStatus !== RequestStatus.LOADING ? courseDetails.overview ?? '' : null}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: In Eng Review

Development

Successfully merging this pull request may close these issues.

4 participants