Skip to content

fix(react): stale lesson data after navigation #318

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

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

AriPerkkio
Copy link
Member

@AriPerkkio AriPerkkio commented Sep 5, 2024

In #253 the amount of "unnecessary" re-renders were reduced by moving state to components where it was actually needed.

It turns out, those re-renders were indeed needed as <WorkspacePanel /> is accessing non-reactive references of the tutorialStore instead of reactive ones. This is not easily visible from the code though. We should improve that part later. This is now just a quick fix for the high priority bug.

These are the non-reactive methods of tutorialStore that use stale data:

const hasEditor = tutorialStore.hasEditor();
const hasPreviews = tutorialStore.hasPreviews();
const hideTerminalPanel = !tutorialStore.hasTerminalPanel();

Failing test case on CI: https://github.com/stackblitz/tutorialkit/actions/runs/10733493214/job/29766948087?pr=318

Before:

tk-layout-after-nav-bug.webm

After:

tk-layout-after-nav-fixed.webm

Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@AriPerkkio
Copy link
Member Author

/pkg-pr-new

@AriPerkkio AriPerkkio force-pushed the fix/stale-lesson-data branch from e8df55f to dea6330 Compare September 6, 2024 06:21
@AriPerkkio AriPerkkio marked this pull request as ready for review September 6, 2024 06:38
@AriPerkkio AriPerkkio requested a review from Nemikolh September 6, 2024 06:38
Comment on lines 42 to 44
const hasEditor = tutorialStore.hasEditor();
const hasPreviews = tutorialStore.hasPreviews();
const hideTerminalPanel = !tutorialStore.hasTerminalPanel();
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we should make these reactive instead, e.g.

const hasEditor = useStore(tutorialStore.hasEditor);

... but as that would require quite much refactoring on the store, e.g. making lesson reactive, I'd leave that out side of this PR.

@AriPerkkio
Copy link
Member Author

@Nemikolh we have a failing minimal test case now! 🎉 🎉

After yesterday's extensive debugging session I realized what the root cause actually was. The WorkspacePanel is accessing non-reactive references of store here:

const hasEditor = tutorialStore.hasEditor();
const hasPreviews = tutorialStore.hasPreviews();
const hideTerminalPanel = !tutorialStore.hasTerminalPanel();

And as WorkspacePanel is not subscribing to any other changes (as it doesn't really need to), it won't re-render when parts of the store change.

I tried to make lesson reactive like we tried yesterday, but ran into plenty of weird React hydration mismatch errors afterwards. As this bug is critical, I would suggest that we merge this with the work-around and publish new release with the fix.

@Nemikolh
Copy link
Member

Nemikolh commented Sep 6, 2024

Good work on the test! 🤩

Sounds good for the rest! I will review this ASAP. 👍

Copy link
Member

@Nemikolh Nemikolh left a comment

Choose a reason for hiding this comment

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

Looks good!

@Nemikolh Nemikolh merged commit 2b5fc92 into stackblitz:main Sep 10, 2024
10 checks passed
@AriPerkkio AriPerkkio deleted the fix/stale-lesson-data branch September 10, 2024 08:46
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.

2 participants