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

Assessment Workspace: Module tab reloading on each keystroke in editor #2332

Open
martin-henz opened this issue Jan 29, 2023 · 7 comments · Fixed by #2338
Open

Assessment Workspace: Module tab reloading on each keystroke in editor #2332

martin-henz opened this issue Jan 29, 2023 · 7 comments · Fixed by #2338
Assignees
Labels
Bug Something isn't working important Fixing this is important, but not mission-critical

Comments

@martin-henz
Copy link
Member

Expected Behavior

Editor keystrokes should not affect module loading.

Current Behavior

Currently each editor keystroke reloads the currently loaded modules in mobile workspace.

Steps to Reproduce

  1. Load: https://share.sourceacademy.org/oxhil
  2. shrink screen to mobile mode
  3. press run
  4. open the runes tab
  5. start typing in editor

you will see that each keystroke restarts the animation and thus the keyboard entry will be slow.

Context

This is a critical issue in modules. Fixing it is necessary for current CS1101S.

@martin-henz martin-henz added Bug Something isn't working important Fixing this is important, but not mission-critical labels Jan 29, 2023
@martin-henz
Copy link
Member Author

(Moved here from modules repo: source-academy/modules#168)

@martin-henz
Copy link
Member Author

See also: #2331 (review)

@ianyong
Copy link
Member

ianyong commented Jan 29, 2023

One of the things that causes a re-render every time the editor value updates is the session button. This is because the session button takes in the current editor value as a prop so that it can initialise new sessions with it:

// No point memoing this, it uses props.editorValue
const sessionButtons = (
<ControlBarSessionButtons
editorSessionId={props.editorSessionId}
// TODO: Hardcoded to make use of the first editor tab. Rewrite after editor tabs are added.
editorValue={props.editorTabs[0].value}
handleSetEditorSessionId={id => dispatch(setEditorSessionId(workspaceLocation, id))}
sharedbConnected={props.sharedbConnected}
key="session"
/>
);

This isn't an issue for the desktop workspace because the control bar is not a parent or child component of the side content, so the re-rendering of the control bar does not affect the side content component at all. However in the mobile workspace, the control bar is a child of the mobile side content component.

I think it should be possible to retrieve the editor value from the Redux store only when it is needed to prevent these unnecessary re-renders.

Not sure how close I am to a fix because I'm more or less playing whack-a-mole using the React profiler.

@ianyong ianyong self-assigned this Feb 19, 2023
@martin-henz martin-henz changed the title Mobile Workspace: Module tab reloading on each keystroke in editor Assessment Workspace: Module tab reloading on each keystroke in editor Oct 16, 2023
@martin-henz martin-henz reopened this Oct 16, 2023
@martin-henz
Copy link
Member Author

I've renamed this issue from "Mobile Workspace" to "Assessment Workspace", just to keep the context. I've reopened the issue, because the problem currently arises in the desktop version in the deployed Source Academy (sourceacademy.nus.edu.sg).

Screen.Recording.2023-10-16.at.11.55.05.AM.mov

To reproduce this example, here is the program:

const WIDTH = 400;
const HEIGHT = 300;
const FPS = 15; 

// let x = 1; let direction = 1;  // for extra fun
function my_first_filter(ignore, dest) {
    // for extra fun
    // direction = x &gt; 0 &amp;&amp; x &lt; 9 ? direction : - direction;
    // x = x + direction;
    const width = image_width();    
    const height = image_height();

    for (let i = 0; i < height; i = i + 1) {
        for (let j = 0; j < width; j = j + 1) {
            dest[i][j][0] = math_min(i, 255);       // for fun: + 10 * x;
            dest[i][j][1] = math_min(j, 255);       // for fun: + 10 * x;
            dest[i][j][2] = math_max(0, 255 - (i + j) / 2);
            dest[i][j][3] = 255;
        }
    }
}

install_filter(my_first_filter);
set_dimensions(WIDTH, HEIGHT);
keep_aspect_ratio(true);
set_fps(FPS);
start();

@sayomaki
Copy link
Contributor

As previously discussed, this issue is likely related to how the various props are being created within the related pages (Playground.tsx, Assessment.tsx, ... etc) and causes unnecessary re-renders when modifying the program in the editor.

The issue may also be affecting Workspace.tsx as well, as the current way of defining the props appears to be an anti-pattern, which in turn causes these side effects.

Unfortunately, this would require a not-so-trivial refactor of the aforementioned components in order to fix/resolve the problem.

@martin-henz
Copy link
Member Author

Just confirming that this issue is still present in the current SA @ NUS.

@RichDom2185
Copy link
Member

Just confirming that this issue is still present in the current SA @ NUS.

How about SA.org? SA @ NUS deployment is frozen so it is expected that the issue still persists there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working important Fixing this is important, but not mission-critical
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants