Browse Source

Merge pull request #11110 from growilabs/fix/183153-183154-restore-renderer-options-null-guard

fix: GrowiSlides throws unhandled exception when the rendererOptions is undefined
mergify[bot] 1 week ago
parent
commit
3b7c2192c9

+ 35 - 0
.kiro/specs/presentation/design.md

@@ -185,6 +185,8 @@ type SlidesProps = {
 - Render slide content via ReactMarkdown with section extraction
 - Render slide content via ReactMarkdown with section extraction
 - Apply Marp container CSS from pre-extracted constants (no runtime Marp dependency)
 - Apply Marp container CSS from pre-extracted constants (no runtime Marp dependency)
 - Use `MARP_CONTAINER_CLASS_NAME` from shared constants module
 - Use `MARP_CONTAINER_CLASS_NAME` from shared constants module
+- Treat the incoming `rendererOptions` as a read-only shared reference (see Risks & Mitigations); derive a new options object via `useMemo` and never mutate the input
+- Guard for `rendererOptions == null` (and missing `remarkPlugins` / `components`) with an early return; the `?` on `PresentationOptions.rendererOptions` is intentional and reflects SWR loading state
 
 
 **Dependencies**
 **Dependencies**
 - Inbound: Slides — rendering delegation (P0)
 - Inbound: Slides — rendering delegation (P0)
@@ -282,3 +284,36 @@ export const presentationMarpit: Marp;
 - Validation: Script exits with error if CSS extraction produces empty output
 - Validation: Script exits with error if CSS extraction produces empty output
 - Risks: Marp options must stay synchronized with `growi-marpit.ts`
 - Risks: Marp options must stay synchronized with `growi-marpit.ts`
 
 
+## Risks & Mitigations
+
+### `rendererOptions` undefined during SWR loading
+
+Callers in `apps/app` obtain `rendererOptions` from `usePresentationViewOptions()`, which is an SWR hook. During the loading window the value is `undefined`. `PresentationOptions.rendererOptions` is therefore declared optional, and `GrowiSlides` performs an early-return null guard.
+
+Defense is layered across four points; removing any single layer has historically caused regressions (PR #11110 / Redmine #183154):
+
+| Layer | Where | Purpose |
+|---|---|---|
+| Type signature | `consts/index.ts` — `rendererOptions?: ReactMarkdownOptions` | Force callers to acknowledge the loading state at compile time |
+| Caller guard | `apps/app` `SlideRenderer.tsx`, `PagePresentationModal.tsx` — no `as ReactMarkdownOptions` casts, must propagate `undefined` honestly | Prevent the loading `undefined` from masquerading as a value |
+| Component guard | `GrowiSlides.tsx` — early return when `rendererOptions == null` | Last line of defense for inline `slide: true` route, which has no caller-side `<RendererErrorMessage />` |
+| E2E | `apps/app/playwright/20-basic-features/presentation.spec.ts` — reload step | Exercises the SWR loading path that unit tests with mocked modules cannot reach |
+
+**Do not remove the optional `?`, the null guard, or add an `as` cast on grounds of "the type is required" — the optionality is intentional and reflects runtime reality.**
+
+### `rendererOptions` is a shared SWR cache reference
+
+The same `rendererOptions` object is returned by SWR to both `SlideRenderer` and `PagePresentationModal`. `GrowiSlides` must **not** mutate it; doing so leaks state across components and causes pushed remark plugins to accumulate across re-renders (incompatible with React StrictMode and concurrent rendering).
+
+Derive a new options object with `useMemo`, spreading `remarkPlugins` and `components` into fresh arrays/objects before adding `extractSections.remarkPlugin` and the section component. Keep the memo dependency list aligned with all derived inputs (`rendererOptions`, `isDarkMode`, `disableSeparationByHeader`, `presentation`).
+
+### Revalidation Triggers
+
+Re-run validation if any of the following change:
+
+- `PresentationOptions.rendererOptions` type (especially making it required again)
+- `usePresentationViewOptions` loading semantics
+- Null guards in `GrowiSlides`, `SlideRenderer`, or `PagePresentationModal`
+- Any `as ReactMarkdownOptions` cast added in `@growi/presentation` callers
+- Marp library major upgrade (regenerate `marpit-base-css.vendor-styles.prebuilt.ts`)
+

+ 1 - 1
.kiro/specs/presentation/spec.json

@@ -1,7 +1,7 @@
 {
 {
   "feature_name": "presentation",
   "feature_name": "presentation",
   "created_at": "2026-03-05T12:00:00Z",
   "created_at": "2026-03-05T12:00:00Z",
-  "updated_at": "2026-03-23T00:00:00Z",
+  "updated_at": "2026-05-12T00:00:00Z",
   "language": "en",
   "language": "en",
   "phase": "implementation-complete",
   "phase": "implementation-complete",
   "cleanup_completed": true,
   "cleanup_completed": true,

+ 35 - 0
apps/app/playwright/20-basic-features/presentation.spec.ts

@@ -15,3 +15,38 @@ test('Presentation', async ({ page }) => {
     page.getByRole('application').getByRole('heading', { level: 1 }),
     page.getByRole('application').getByRole('heading', { level: 1 }),
   ).toHaveText(/Welcome to GROWI/);
   ).toHaveText(/Welcome to GROWI/);
 });
 });
+
+test('Slide page (slide: true frontmatter) renders without crashing', async ({
+  page,
+}) => {
+  await page.goto('/Sandbox/slide-test');
+
+  // open the editor
+  await page.getByTestId('editor-button').click();
+  await expect(page.getByTestId('grw-editor-navbar-bottom')).toBeVisible();
+
+  // fill the editor with slide content
+  await page
+    .locator('.cm-content')
+    .fill('---\nslide: true\n---\n# Slide 1\n---\n# Slide 2');
+
+  // The editor preview must finish rendering both slides through the marpit
+  // pipeline before saving — this is the slide-mode observable contract and
+  // also proves the preview did not crash on slide content.
+  const previewSlides = page
+    .getByTestId('page-editor-preview-body')
+    .locator('svg[data-marpit-svg]');
+  await expect(previewSlides).toHaveCount(2);
+
+  // save
+  await page.keyboard.press('Control+s');
+
+  // view mode must render the slide deck after save
+  await page.getByTestId('view-button').click();
+  await expect(page.locator('.slides')).toBeVisible();
+
+  // reload exercises the SWR loading path where rendererOptions is briefly
+  // undefined; the slide page must still render without crashing.
+  await page.reload();
+  await expect(page.locator('.slides')).toBeVisible();
+});

+ 1 - 5
apps/app/src/client/components/Page/SlideRenderer.tsx

@@ -1,5 +1,4 @@
 import type { JSX } from 'react';
 import type { JSX } from 'react';
-import type { Options as ReactMarkdownOptions } from 'react-markdown';
 
 
 import { usePresentationViewOptions } from '~/stores/renderer';
 import { usePresentationViewOptions } from '~/stores/renderer';
 
 
@@ -16,10 +15,7 @@ export const SlideRenderer = (props: SlideRendererProps): JSX.Element => {
   const { data: rendererOptions } = usePresentationViewOptions();
   const { data: rendererOptions } = usePresentationViewOptions();
 
 
   return (
   return (
-    <Slides
-      hasMarpFlag={marp}
-      options={{ rendererOptions: rendererOptions as ReactMarkdownOptions }}
-    >
+    <Slides hasMarpFlag={marp} options={{ rendererOptions }}>
       {markdown}
       {markdown}
     </Slides>
     </Slides>
   );
   );

+ 1 - 2
apps/app/src/client/components/PagePresentationModal/PagePresentationModal.tsx

@@ -5,7 +5,6 @@ import type { PresentationProps } from '@growi/presentation/dist/client';
 import { useSlidesByFrontmatter } from '@growi/presentation/dist/services';
 import { useSlidesByFrontmatter } from '@growi/presentation/dist/services';
 import { LoadingSpinner } from '@growi/ui/dist/components';
 import { LoadingSpinner } from '@growi/ui/dist/components';
 import { useFullScreen } from '@growi/ui/dist/utils';
 import { useFullScreen } from '@growi/ui/dist/utils';
-import type { Options as ReactMarkdownOptions } from 'react-markdown';
 import { Modal, ModalBody } from 'reactstrap';
 import { Modal, ModalBody } from 'reactstrap';
 
 
 import { useCurrentPageData } from '~/states/page';
 import { useCurrentPageData } from '~/states/page';
@@ -87,7 +86,7 @@ const PagePresentationModalSubstance: React.FC = () => {
         {rendererOptions != null && isEnabledMarp != null && (
         {rendererOptions != null && isEnabledMarp != null && (
           <Presentation
           <Presentation
             options={{
             options={{
-              rendererOptions: rendererOptions as ReactMarkdownOptions,
+              rendererOptions,
               revealOptions: {
               revealOptions: {
                 embedded: true,
                 embedded: true,
                 hash: true,
                 hash: true,

+ 2 - 1
packages/presentation/package.json

@@ -37,7 +37,8 @@
     "lint:biome": "biome check",
     "lint:biome": "biome check",
     "lint:styles": "stylelint --allow-empty-input \"src/**/*.scss\" \"src/**/*.css\"",
     "lint:styles": "stylelint --allow-empty-input \"src/**/*.scss\" \"src/**/*.css\"",
     "lint:typecheck": "tsgo --noEmit",
     "lint:typecheck": "tsgo --noEmit",
-    "lint": "run-p lint:*"
+    "lint": "run-p lint:*",
+    "test": "vitest run"
   },
   },
   "dependencies": {
   "dependencies": {
   },
   },

+ 58 - 0
packages/presentation/src/client/components/GrowiSlides.spec.tsx

@@ -0,0 +1,58 @@
+import { render, screen } from '@testing-library/react';
+import { describe, expect, it, vi } from 'vitest';
+
+import { GrowiSlides } from './GrowiSlides';
+
+vi.mock('next/head', () => ({
+  default: ({ children }: { children: React.ReactNode }) => <>{children}</>,
+}));
+
+vi.mock('../consts/marpit-base-css.vendor-styles.prebuilt', () => ({
+  PRESENTATION_MARPIT_CSS: '',
+  SLIDE_MARPIT_CSS: '',
+}));
+
+vi.mock('../services/renderer/extract-sections', () => ({
+  remarkPlugin: vi.fn(),
+}));
+
+vi.mock('./RichSlideSection', () => ({
+  RichSlideSection: () => <div />,
+  PresentationRichSlideSection: () => <div />,
+}));
+
+const validRendererOptions = {
+  remarkPlugins: [],
+  rehypePlugins: [],
+  components: {},
+};
+
+describe('GrowiSlides', () => {
+  it('does not throw when rendererOptions is undefined', () => {
+    expect(() =>
+      render(
+        <GrowiSlides options={{ rendererOptions: undefined }}>
+          {'# Slide'}
+        </GrowiSlides>,
+      ),
+    ).not.toThrow();
+  });
+
+  it('renders nothing when rendererOptions is undefined', () => {
+    const { container } = render(
+      <GrowiSlides options={{ rendererOptions: undefined }}>
+        {'# Slide'}
+      </GrowiSlides>,
+    );
+    expect(container.firstChild).toBeNull();
+  });
+
+  it('renders slides content when rendererOptions is valid', () => {
+    render(
+      <GrowiSlides options={{ rendererOptions: validRendererOptions }}>
+        {'# Slide 1'}
+      </GrowiSlides>,
+    );
+    expect(screen.queryByText('Slide 1')).toBeTruthy();
+  });
+});

+ 30 - 18
packages/presentation/src/client/components/GrowiSlides.tsx

@@ -1,6 +1,7 @@
-import type { JSX } from 'react';
+import { type JSX, useMemo } from 'react';
 import Head from 'next/head';
 import Head from 'next/head';
 import ReactMarkdown from 'react-markdown';
 import ReactMarkdown from 'react-markdown';
+import type { PluggableList } from 'unified';
 
 
 import { MARP_CONTAINER_CLASS_NAME, type PresentationOptions } from '../consts';
 import { MARP_CONTAINER_CLASS_NAME, type PresentationOptions } from '../consts';
 import {
 import {
@@ -23,25 +24,36 @@ export const GrowiSlides = (props: Props): JSX.Element => {
   const { options, children, presentation } = props;
   const { options, children, presentation } = props;
   const { rendererOptions, isDarkMode, disableSeparationByHeader } = options;
   const { rendererOptions, isDarkMode, disableSeparationByHeader } = options;
 
 
-  if (
-    rendererOptions.remarkPlugins == null ||
-    rendererOptions.components == null
-  ) {
-    // biome-ignore lint/complexity/noUselessFragments: This is for type checking only. The actual code will never reach here.
+  // Derive a new options object instead of mutating `rendererOptions`:
+  // it is a shared SWR cache reference also consumed by PagePresentationModal,
+  // so mutation here would leak into other components and accumulate on re-render.
+  const slideRendererOptions = useMemo(() => {
+    if (
+      rendererOptions == null ||
+      rendererOptions.remarkPlugins == null ||
+      rendererOptions.components == null
+    ) {
+      return null;
+    }
+    const remarkPlugins: PluggableList = [
+      ...rendererOptions.remarkPlugins,
+      [extractSections.remarkPlugin, { isDarkMode, disableSeparationByHeader }],
+    ];
+    return {
+      ...rendererOptions,
+      remarkPlugins,
+      components: {
+        ...rendererOptions.components,
+        section: presentation ? PresentationRichSlideSection : RichSlideSection,
+      },
+    };
+  }, [rendererOptions, isDarkMode, disableSeparationByHeader, presentation]);
+
+  if (slideRendererOptions == null) {
+    // biome-ignore lint/complexity/noUselessFragments: early return when rendererOptions is null
     return <></>;
     return <></>;
   }
   }
 
 
-  rendererOptions.remarkPlugins.push([
-    extractSections.remarkPlugin,
-    {
-      isDarkMode,
-      disableSeparationByHeader,
-    },
-  ]);
-  rendererOptions.components.section = presentation
-    ? PresentationRichSlideSection
-    : RichSlideSection;
-
   const css = presentation ? PRESENTATION_MARPIT_CSS : SLIDE_MARPIT_CSS;
   const css = presentation ? PRESENTATION_MARPIT_CSS : SLIDE_MARPIT_CSS;
   return (
   return (
     <>
     <>
@@ -49,7 +61,7 @@ export const GrowiSlides = (props: Props): JSX.Element => {
         <style>{css}</style>
         <style>{css}</style>
       </Head>
       </Head>
       <div className={`slides ${MARP_CONTAINER_CLASS_NAME}`}>
       <div className={`slides ${MARP_CONTAINER_CLASS_NAME}`}>
-        <ReactMarkdown {...rendererOptions}>
+        <ReactMarkdown {...slideRendererOptions}>
           {children ?? '## No Contents'}
           {children ?? '## No Contents'}
         </ReactMarkdown>
         </ReactMarkdown>
       </div>
       </div>

+ 1 - 1
packages/presentation/src/client/consts/index.ts

@@ -4,7 +4,7 @@ import type { Options as RevealOptions } from 'reveal.js';
 export const MARP_CONTAINER_CLASS_NAME = 'marpit';
 export const MARP_CONTAINER_CLASS_NAME = 'marpit';
 
 
 export type PresentationOptions = {
 export type PresentationOptions = {
-  rendererOptions: ReactMarkdownOptions;
+  rendererOptions?: ReactMarkdownOptions;
   revealOptions?: RevealOptions;
   revealOptions?: RevealOptions;
   isDarkMode?: boolean;
   isDarkMode?: boolean;
   disableSeparationByHeader?: boolean;
   disableSeparationByHeader?: boolean;

+ 10 - 0
packages/presentation/vitest.config.ts

@@ -0,0 +1,10 @@
+import react from '@vitejs/plugin-react';
+import { defineConfig } from 'vitest/config';
+
+export default defineConfig({
+  plugins: [react()],
+  test: {
+    environment: 'happy-dom',
+    globals: true,
+  },
+});