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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
type: lesson
title: Layout change from
template: default
mainCommand: node index.mjs
previews:
- title: "Custom preview"
port: 8000
---

# Navigation test - Layout change from

This page should show previw
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
type: lesson
title: Layout change to
previews: false
terminal:
panels:
- ["terminal", "Custom Terminal"]
---

# Navigation test - Layout change to

This page should not show previw. It should show terminal instead.
2 changes: 2 additions & 0 deletions e2e/src/content/tutorial/tests/navigation/meta.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ lessons:
- page-one
- page-two
- page-three
- layout-change-from
- layout-change-to
mainCommand: ''
prepareCommands: []
---
28 changes: 25 additions & 3 deletions e2e/test/navigation.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { test, expect } from '@playwright/test';

const BASE_URL = '/tests/navigation/page-one';
const BASE_URL = '/tests/navigation';

test('user can navigate between lessons using nav bar links', async ({ page }) => {
await page.goto(BASE_URL);
await page.goto(`${BASE_URL}/page-one`);
await expect(page.getByRole('heading', { level: 1, name: 'Navigation test - Page one' })).toBeVisible();

// navigate forwards
Expand All @@ -21,10 +21,32 @@ test('user can navigate between lessons using nav bar links', async ({ page }) =
});

test('user can navigate between lessons using breadcrumbs', async ({ page }) => {
await page.goto(BASE_URL);
await page.goto(`${BASE_URL}/page-one`);

await page.getByRole('button', { name: 'Tests / Navigation / Page one' }).click({ force: true });
await page.getByRole('region', { name: 'Navigation' }).getByRole('link', { name: 'Page three' }).click();

await expect(page.getByRole('heading', { level: 1, name: 'Navigation test - Page three' })).toBeVisible();
});

test("user should see metadata's layout changes after navigation (#318)", async ({ page }) => {
await page.goto(`${BASE_URL}/layout-change-from`);

// first page should have preview visible
await expect(page.getByRole('heading', { level: 1, name: 'Navigation test - Layout change from' })).toBeVisible();
await expect(page.getByText('Custom preview')).toBeVisible();

await page.getByRole('link', { name: 'Layout change to' }).click();
await expect(page.getByRole('heading', { level: 1, name: 'Navigation test - Layout change to' })).toBeVisible();

// second page should have preview hidden, terminal visible

/* eslint-disable multiline-comment-style */
// TODO: Requires #245
// await expect(page.getByText('Preparing Environment')).not.toBeVisible();

await expect(page.getByRole('tab', { name: 'Custom Terminal', selected: true })).toBeVisible();
await expect(page.getByRole('tabpanel', { name: 'Custom Terminal' })).toContainText('~/tutorial', {
useInnerText: true,
});
});
15 changes: 12 additions & 3 deletions packages/react/src/Panels/WorkspacePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ interface TerminalProps extends PanelProps {
* This component is the orchestrator between various interactive components.
*/
export function WorkspacePanel({ tutorialStore, theme }: Props) {
/**
* Re-render when lesson changes.
* The `tutorialStore.hasEditor()` and other methods below access
* stale data as they are not reactive.
*/
useStore(tutorialStore.ref);

const hasEditor = tutorialStore.hasEditor();
const hasPreviews = tutorialStore.hasPreviews();
const hideTerminalPanel = !tutorialStore.hasTerminalPanel();
Comment on lines 42 to 44
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.

Expand Down Expand Up @@ -89,6 +96,7 @@ function EditorSection({ theme, tutorialStore, hasEditor }: PanelProps) {
const selectedFile = useStore(tutorialStore.selectedFile);
const currentDocument = useStore(tutorialStore.currentDocument);
const lessonFullyLoaded = useStore(tutorialStore.lessonFullyLoaded);
const storeRef = useStore(tutorialStore.ref);

const lesson = tutorialStore.lesson!;

Expand Down Expand Up @@ -116,7 +124,7 @@ function EditorSection({ theme, tutorialStore, hasEditor }: PanelProps) {
} else {
setHelpAction('reset');
}
}, [tutorialStore.ref]);
}, [storeRef]);

return (
<Panel
Expand All @@ -128,7 +136,7 @@ function EditorSection({ theme, tutorialStore, hasEditor }: PanelProps) {
className="transition-theme bg-tk-elements-panel-backgroundColor text-tk-elements-panel-textColor"
>
<EditorPanel
id={tutorialStore.ref}
id={storeRef}
theme={theme}
showFileTree={tutorialStore.hasFileTree()}
editorDocument={currentDocument}
Expand Down Expand Up @@ -157,6 +165,7 @@ function PreviewsSection({
const previewRef = useRef<ImperativePreviewHandle>(null);
const lesson = tutorialStore.lesson!;
const terminalConfig = useStore(tutorialStore.terminalConfig);
const storeRef = useStore(tutorialStore.ref);

function showTerminal() {
const { current: terminal } = terminalPanelRef;
Expand Down Expand Up @@ -206,7 +215,7 @@ function PreviewsSection({
});

return () => unsubscribe();
}, [tutorialStore.ref]);
}, [storeRef]);

return (
<Panel
Expand Down
6 changes: 3 additions & 3 deletions packages/runtime/src/store/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class TutorialStore {
private _lessonFilesFetcher: LessonFilesFetcher;
private _lessonTask: Task<unknown> | undefined;
private _lesson: Lesson | undefined;
private _ref: number = 1;
private _ref = atom(1);
private _themeRef = atom(1);

private _lessonFiles: Files | undefined;
Expand Down Expand Up @@ -135,7 +135,7 @@ export class TutorialStore {

this._lessonTask?.cancel();

this._ref += 1;
this._ref.set(1 + (this._ref.value || 0));
this._lesson = lesson;
this.lessonFullyLoaded.set(false);

Expand Down Expand Up @@ -219,7 +219,7 @@ export class TutorialStore {
return this._lesson;
}

get ref(): unknown {
get ref(): ReadableAtom<unknown> {
return this._ref;
}

Expand Down