Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: tinyMCELanguageSelectorPlugin error related to Button ID #991

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

AfaqShuaib09
Copy link
Contributor

@AfaqShuaib09 AfaqShuaib09 commented Oct 30, 2024

This PR fixes the TinyMCE editor Language Selector failed to find and set button ID in TinyMceLanguageSelectorPlugin after 3 attempts. It adds a 0.1-sec timeout to prevent TinyMCE from appearing immediately after the state change, as the editor reloads afterward, causing it to disappear and triggering the error above.

Error Reproduction Steps:

  • Pull the code from the master branch.
  • Create a new course run for any existing course.
  • Add the relevant details and move the course to the review state. You should see the message failed to find and set button ID in TinyMceLanguageSelectorPlugin after 3 attempts in the console log.
  • Checkout to this branch.
  • Create a new course run for any existing course.
  • You will no longer see this error in the console logs.

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.76%. Comparing base (c73567b) to head (4cca100).

Files with missing lines Patch % Lines
...components/EditCoursePage/CollapsibleCourseRun.jsx 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #991      +/-   ##
==========================================
- Coverage   67.77%   67.76%   -0.01%     
==========================================
  Files         128      128              
  Lines        3230     3233       +3     
  Branches      937      937              
==========================================
+ Hits         2189     2191       +2     
- Misses        993      994       +1     
  Partials       48       48              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AfaqShuaib09 AfaqShuaib09 marked this pull request as ready for review October 30, 2024 19:51
if (IN_REVIEW_STATUS.includes(courseRun.status) !== isCourseRunInReview) {
setTimeout(() => {
this.setState({ isCourseRunInReview: IN_REVIEW_STATUS.includes(courseRun.status) });
}, 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

this 0.1 value -- what was the reason for selecting this value?

Copy link
Contributor Author

@AfaqShuaib09 AfaqShuaib09 Oct 31, 2024

Choose a reason for hiding this comment

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

I tried random values for the timeout, i.e. 500, 200, 100, 50. It's minimum value at which this error no longer appears.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is from your local testing, I would suggest keeping the deployment environment in mind when deciding this.

Copy link
Contributor Author

@AfaqShuaib09 AfaqShuaib09 Oct 31, 2024

Choose a reason for hiding this comment

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

Hmm, maybe we can increase the timeout a bit, or let's try this out on staging first to check if the error still appears (cause I don't want to add unnecessary delay in rendering of review related fields)

Copy link
Contributor

@zawan-ila zawan-ila left a comment

Choose a reason for hiding this comment

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

Can you please add instructions to reproduce the issue locally in the PR description? Some details are present on our internal ticket, but would be great to add them here as well.

@AfaqShuaib09
Copy link
Contributor Author

Can you please add instructions to reproduce the issue locally in the PR description? Some details are present on our internal ticket, but would be great to add them here as well.

Done, I've added the steps to reproduce this error locally, you can test it out on stage as well.

Copy link
Contributor

@zawan-ila zawan-ila left a comment

Choose a reason for hiding this comment

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

I can repro the error by simply opening a course. And checkout out this code does not seem to fix it. Could it be that my local data is simply messed up?

@AfaqShuaib09
Copy link
Contributor Author

AfaqShuaib09 commented Nov 1, 2024

I can repro the error by simply opening a course. And checkout out this code does not seem to fix it. Could it be that my local data is simply messed up?

Ah, it’s reproducible for me now, even after reloading the page. However it’s sort of flaky behavior, sometimes it appears on page refresh, and sometimes it doesn’t. Two days ago, it didn’t appear for me at all after refreshing the page. I’m not sure what the issue is. The PR above will resolve the error for the review state transition scenario. I think it would be better to ignore this error, as we did for support-tools

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