🪲 Don't crash the "view program" page if the program has a syntax error #5985

rix0rrr · 2024-11-30T19:50:39Z

It used to be that the "view program" page would crash if a program has syntax errors. The reason is that we try to translate programs from the language of the author to the language of the viewer, but that operation may fail due to parse errors, which would cause the page to fail loading.

We used to only check for blanks and avoid translating a program because we knew that programs with blanks would fail to parse; but there are many more reasons that would case a parse failure, so instead of trying to predict them I'm now just catching the error.

In this PR I want to address a failure case as quickly as possible. There are some open issues/questions that someone else could/should have a look at later:

I have not written a test yet to make sure this doesn't re-occur.
We do translation in 2 steps: first to English, then to the target language. I don't know if that's on purpose or not; I imagine so, it's what the original code was doing and it must be for a reason.
The original code used to always normalize to English first, then translate to the target language. I've changed it to only translate if the source and target languages are different in order to save compute time. I hope that's doesn't break any use cases I'm not aware of.
We have no way to surface the parse error the user. That may be fine because they can try running the program and then seeing the parse error, but just something to note.

How to test

Write a syntactically invalid program and save it.

Use the /hedy/.../view_program link to view it, and observe that the page loads successfully.

It used to be that the "view program" page would crash if a program has syntax errors. The reason is that we try to translate programs from the language of the author to the language of the viewer, but that operation may fail due to parse errors, which aren't caught. In this PR I want to address a failure case as quickly as possible. There are some open issues/questions that someone else could/should have a look at later: - I have not written a test yet to make sure this doesn't re-occur. - We do translation in 2 steps: first to English, then to the target language. I don't know if that's on purpose or not; I imagine so, it's what the original code was doing and it must be for a reason. - The original code used to always normalize to English first, then translate to the target language. I've changed it to only translate if the source and target languages are different. - We have no way to surface the parse error the user. That may be fine because they can try running the program and then seeing the parse error, but just something to note.

for more information, see https://pre-commit.ci

jpelay · 2024-12-02T16:59:01Z

Hello Rico, thanks for the PR.

I have not written a test yet to make sure this doesn't re-occur.

I've just added a test for this case! Tried in main and it fails, and in this branch it succeeds.

We do translation in 2 steps: first to English, then to the target language. I don't know if that's on purpose or not; I imagine so, it's what the original code was doing and it must be for a reason.

We can't translate two arbitrary languages with each other, so we use English as a bridge, we can update the code, but I don't feel that's worth it right now.

The original code used to always normalize to English first, then translate to the target language. I've changed it to only translate if the source and target languages are different in order to save compute time. I hope that's doesn't break any use cases

I'm not aware of.
This is great! Thanks!

We have no way to surface the parse error the user. That may be fine because they can try running the program and then seeing the parse error, but just something to note.

I think it's apparent that this program doesn't work, but indeed we are not letting the user know that there's a problem here. I think we can send a flag to the front end that the program contains an error and then show an alert somewhere. What do you think?

rix0rrr · 2024-12-02T17:18:28Z

We can't translate two arbitrary languages with each other, so we use English as a bridge, we can update the code, but I don't feel that's worth it right now.

If this is the case: translate_keywords has a from_lang and a to_lang.

Does it check that at least one of those two arguments has the string "en" and throw an error otherwise? It should (but a quick look at the code didn't show that)
If it can do that, it can also call itself recursively twice if it detects that NOT one of the arguments is "en". That moves the burden of being aware of what translate_keywords can do from the consumer of translate_keywords() to a performance question for the implementor of translate_keywords().

What do you think?

I think it's not worth it to invest time into. The error will show itself if the viewer clicks "Run program".

rix0rrr · 2024-12-02T17:20:05Z

tests/cypress/e2e/hedy_page/invalid_program.cy.js

-        cy.get('#editor > .cm-editor > .cm-scroller > .cm-content').click();
+        codeMirrorContent().click();


rix0rrr · 2024-12-02T17:47:32Z

Thanks for the test!

jpelay · 2024-12-02T18:08:32Z

Thanks for the test!

No worries, I'm actually just now seeing that we can translate any arbitrary languages indeed (I think we couldn't before, but may be I misremember) so I'm adapting that code as well.

rix0rrr · 2024-12-02T18:12:27Z

Lovely, thanks! Feel free to merge when you feel good about it.

for more information, see https://pre-commit.ci

jpelay

All good now!

rix0rrr

Dont add waits it’s never the answer

jpelay · 2024-12-02T20:38:28Z

Dont add waits it’s never the answer

😭 I added a nice wait for an intercepted link and it's not working, so I'm debugging the issue!

mergify · 2024-12-02T20:55:29Z

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

jpelay

Dont merge until i figure out the test

jpelay · 2024-12-02T21:32:41Z

Instead of a wait I added a timeout, still not the best, but a bit better I think!

mergify · 2024-12-02T21:58:31Z

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

rix0rrr requested a review from jpelay November 30, 2024 19:50

pre-commit-ci bot and others added 3 commits November 30, 2024 19:50

[pre-commit.ci] auto fixes from pre-commit.com hooks

260a8ef

for more information, see https://pre-commit.ci

update data for testing to include program

244d182

add codeMirrorContent function to utils

d28c37b

rix0rrr commented Dec 2, 2024

View reviewed changes

jpelay and others added 3 commits December 2, 2024 14:30

stop using english as bridge when translating

619c2ff

add tests for translating languages not involving english

529ddb4

[pre-commit.ci] auto fixes from pre-commit.com hooks

94f7868

for more information, see https://pre-commit.ci

jpelay approved these changes Dec 2, 2024

View reviewed changes

jpelay added 2 commits December 2, 2024 14:32

add the programs to the database

a277267

add wait

533bc98

rix0rrr commented Dec 2, 2024

View reviewed changes

Merge branch 'main' into crash-on-untranslatable-program

03a715c

jpelay requested changes Dec 2, 2024

View reviewed changes

use timeout instead of a wait

c232b17

jpelay approved these changes Dec 2, 2024

View reviewed changes

mergify bot merged commit f031876 into main Dec 2, 2024
11 checks passed

mergify bot deleted the crash-on-untranslatable-program branch December 2, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪲 Don't crash the "view program" page if the program has a syntax error #5985

🪲 Don't crash the "view program" page if the program has a syntax error #5985

rix0rrr commented Nov 30, 2024 •

edited

Loading

jpelay commented Dec 2, 2024

rix0rrr commented Dec 2, 2024 •

edited

Loading

rix0rrr Dec 2, 2024

rix0rrr commented Dec 2, 2024

jpelay commented Dec 2, 2024

rix0rrr commented Dec 2, 2024

jpelay left a comment

rix0rrr left a comment

jpelay commented Dec 2, 2024

mergify bot commented Dec 2, 2024

jpelay left a comment

jpelay commented Dec 2, 2024

mergify bot commented Dec 2, 2024

		cy.get('#editor > .cm-editor > .cm-scroller > .cm-content').click();
		codeMirrorContent().click();

🪲 Don't crash the "view program" page if the program has a syntax error #5985

🪲 Don't crash the "view program" page if the program has a syntax error #5985

Conversation

rix0rrr commented Nov 30, 2024 • edited Loading

jpelay commented Dec 2, 2024

rix0rrr commented Dec 2, 2024 • edited Loading

rix0rrr Dec 2, 2024

Choose a reason for hiding this comment

rix0rrr commented Dec 2, 2024

jpelay commented Dec 2, 2024

rix0rrr commented Dec 2, 2024

jpelay left a comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

jpelay commented Dec 2, 2024

mergify bot commented Dec 2, 2024

jpelay left a comment

Choose a reason for hiding this comment

jpelay commented Dec 2, 2024

mergify bot commented Dec 2, 2024

rix0rrr commented Nov 30, 2024 •

edited

Loading

rix0rrr commented Dec 2, 2024 •

edited

Loading