Skip to content

Commit e0733f6

Browse files
DavidBabinecDavidBabinec
committed
refactor: optimize React components using best practices
1 parent 14dddb9 commit e0733f6

41 files changed

Lines changed: 1526 additions & 422 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎eslint.config.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@ import js from '@eslint/js'
22
import globals from 'globals'
33
import reactHooks from 'eslint-plugin-react-hooks'
44
import reactRefresh from 'eslint-plugin-react-refresh'
5+
import reactCompiler from 'eslint-plugin-react-compiler'
56
import tseslint from 'typescript-eslint'
67
import { defineConfig, globalIgnores } from 'eslint/config'
78

8-
// React Compiler ESLint rule is NOT enabled — see vite.config.ts for why
9-
// the compiler itself was rolled back. The eslint plugin is meaningless
10-
// without the compiler running.
9+
// React Compiler ESLint rule surfaces functions the compiler can't safely
10+
// memoize (Rules-of-React violations, mutating render-time state, etc.) so
11+
// they're caught at lint time rather than as a build-time bailout. The
12+
// compiler itself is wired in `vite.config.ts`.
1113

1214
export default defineConfig([
1315
globalIgnores(['dist', '.worktrees']),
@@ -18,6 +20,7 @@ export default defineConfig([
1820
tseslint.configs.recommended,
1921
reactHooks.configs.flat.recommended,
2022
reactRefresh.configs.vite,
23+
reactCompiler.configs.recommended,
2124
],
2225
languageOptions: {
2326
globals: globals.browser,

‎src/__tests__/admin/accountMenuButton.test.tsx‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,10 +187,11 @@ describe('AccountMenuButton', () => {
187187
// name persists) and runs the View Transitions fade. Hard navigation
188188
// would reboot the React app and is reserved for sign-out.
189189
const assignSpy = window.location.assign as ReturnType<typeof mock>
190-
let observedPathname = ''
190+
// Render the live pathname into the DOM and assert via querying it. This
191+
// keeps the probe pure (no closure-variable reassignment from inside the
192+
// component — flagged by react-compiler as a side effect during render).
191193
function PathProbe() {
192-
observedPathname = useLocation().pathname
193-
return null
194+
return <span data-testid="probe-pathname">{useLocation().pathname}</span>
194195
}
195196

196197
render(
@@ -211,7 +212,7 @@ describe('AccountMenuButton', () => {
211212
fireEvent.click(screen.getByTestId('account-menu-go-to-account'))
212213

213214
await waitFor(() => {
214-
expect(observedPathname).toBe('/admin/account')
215+
expect(screen.getByTestId('probe-pathname').textContent).toBe('/admin/account')
215216
})
216217
expect(assignSpy).not.toHaveBeenCalled()
217218
})

‎src/__tests__/architecture/bundle-size-budgets.test.ts‎

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -103,16 +103,35 @@ const BUDGETS: ChunkBudget[] = [
103103
rationale: 'dompurify + immer (current ~32 KB raw / 12 KB gzipped)',
104104
},
105105

106-
// Second-tier eager chunk (shared by all admin routes through
107-
// @admin/layouts/AdminCanvasLayout and AdminPageLayout).
106+
// Editor shell — loaded eagerly only on canvas pages (Site / Content /
107+
// Data / Media). Contains the canvas, every panel, the property
108+
// controls, every first-party module, and the publisher graph. Plugin /
109+
// Users / Account / plugin admin pages do NOT pull this chunk.
108110
{
109-
prefix: 'layouts-',
110-
maxBytes: 540_000,
111+
prefix: 'AdminCanvasLayout-',
112+
maxBytes: 700_000,
111113
rationale:
112-
'shared admin layouts chunk — currently bloated by editor-store ' +
113-
'(usePersistence) being pulled into AdminPageLayout. Current ~507 KB raw / ' +
114-
'143 KB gzipped. Cap should be REDUCED when AdminPageLayout drops the ' +
115-
'editor-store dependency.',
114+
'editor shell (canvas + panels + modules + publisher). Current ' +
115+
'~621 KB raw / 199 KB gzipped. Includes React Compiler overhead ' +
116+
'(`useMemoCache` calls per component, ~30% bundle growth). Only the ' +
117+
'four canvas-capable routes import this chunk via the direct deep ' +
118+
'import `@admin/layouts/AdminCanvasLayout`.',
119+
},
120+
121+
// Admin shell — the lightweight layout for non-canvas admin pages.
122+
// Must stay TINY: every byte added here ships on every non-editor admin
123+
// page (Plugins / Users / Account / plugin admin pages).
124+
{
125+
prefix: 'AdminPageLayout-',
126+
maxBytes: 12_000,
127+
rationale:
128+
'lightweight admin shell — toolbar + page header + settings modal ' +
129+
'mount gate. Current ~4 KB raw / 2 KB gzipped. Reads adminUi (tiny ' +
130+
'Zustand store) for site name + settings modal flag, fetched via ' +
131+
'`useSiteSummary` (lightweight cmsAdapter call) instead of ' +
132+
'`usePersistence`. This chunk MUST NOT pull `@site/store/store` ' +
133+
'— if it grows past ~12 KB, an admin-shell consumer almost ' +
134+
'certainly re-introduced the editor store dependency.',
116135
},
117136

118137
// Heaviest lazy chunk — protected by the codemirror-lazy-only gate at the

‎src/__tests__/layout/editorLayoutPersistence.test.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
1010
import React from 'react'
1111
import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'
1212
import { MemoryRouter } from '@admin/lib/routing'
13-
import { AdminCanvasLayout } from '@admin/layouts'
13+
import { AdminCanvasLayout } from '@admin/layouts/AdminCanvasLayout'
1414
import { AdminSessionProvider } from '@admin/session'
1515
import { StepUpProvider } from '@admin/shared/StepUp'
1616
import { useEditorStore } from '@site/store/store'

‎src/__tests__/settings/settingsModal.test.tsx‎

Lines changed: 24 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,8 @@ function resetStore() {
5555
selectedNodeId: null,
5656
selectedNodeIds: [],
5757
hoveredNodeId: null,
58-
settingsModalOpen: false,
59-
settingsModalSection: 'pages',
58+
isSettingsOpen: false,
59+
activeSection: 'pages',
6060
domTreePanel: { collapsed: false, x: 0, y: 0, width: 280 },
6161
propertiesPanel: { collapsed: false, x: 0, y: 0, width: 280 },
6262
focusedPanel: 'canvas',
@@ -76,8 +76,8 @@ function openModal(section = 'pages', withSite = false) {
7676
useEditorStore.setState({ site, activePageId: 'page-1' } as Parameters<typeof useEditorStore.setState>[0])
7777
}
7878
useEditorStore.setState({
79-
settingsModalOpen: true,
80-
settingsModalSection: section,
79+
isSettingsOpen: true,
80+
activeSection: section,
8181
} as Parameters<typeof useEditorStore.setState>[0])
8282
}
8383

@@ -89,13 +89,13 @@ afterEach(cleanup)
8989
// ---------------------------------------------------------------------------
9090

9191
describe('SettingsModal — render gating', () => {
92-
it('renders nothing when settingsModalOpen is false', () => {
92+
it('renders nothing when isSettingsOpen is false', () => {
9393
render(<SettingsModal />)
9494
// null render — no dialog in the DOM
9595
expect(document.querySelector('[role="dialog"]')).toBeNull()
9696
})
9797

98-
it('renders the dialog when settingsModalOpen is true', () => {
98+
it('renders the dialog when isSettingsOpen is true', () => {
9999
openModal()
100100
render(<SettingsModal />)
101101
expect(document.querySelector('[role="dialog"]')).not.toBeNull()
@@ -106,7 +106,7 @@ describe('SettingsModal — render gating', () => {
106106
const { unmount } = render(<SettingsModal />)
107107
expect(document.querySelector('[role="dialog"]')).not.toBeNull()
108108
act(() => {
109-
useEditorStore.setState({ settingsModalOpen: false } as Parameters<typeof useEditorStore.setState>[0])
109+
useEditorStore.setState({ isSettingsOpen: false } as Parameters<typeof useEditorStore.setState>[0])
110110
})
111111
unmount()
112112
expect(document.querySelector('[role="dialog"]')).toBeNull()
@@ -194,15 +194,15 @@ describe('SettingsModal — backdrop', () => {
194194
expect(backdrop).not.toBeNull()
195195
})
196196

197-
it('clicking the backdrop closes the modal (calls closeSettingsModal)', () => {
197+
it('clicking the backdrop closes the modal (calls closeSettings)', () => {
198198
openModal()
199199
render(<SettingsModal />)
200200
// Find the backdrop — it has aria-hidden="true" and the onClick handler
201201
const backdrop = document.querySelector('[aria-hidden="true"]') as HTMLElement
202202
expect(backdrop).not.toBeNull()
203203
fireEvent.click(backdrop)
204-
// After click, store should have settingsModalOpen: false
205-
expect(useEditorStore.getState().settingsModalOpen).toBe(false)
204+
// After click, store should have isSettingsOpen: false
205+
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
206206
})
207207
})
208208

@@ -367,15 +367,15 @@ describe('SettingsModal — close behaviours', () => {
367367
render(<SettingsModal />)
368368
const closeBtn = screen.getByLabelText('Close settings')
369369
fireEvent.click(closeBtn)
370-
expect(useEditorStore.getState().settingsModalOpen).toBe(false)
370+
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
371371
})
372372

373373
it('pressing Escape closes the modal', () => {
374374
openModal()
375375
render(<SettingsModal />)
376376
const dialog = screen.getByRole('dialog')
377377
fireEvent.keyDown(dialog, { key: 'Escape', code: 'Escape' })
378-
expect(useEditorStore.getState().settingsModalOpen).toBe(false)
378+
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
379379
})
380380

381381
it('pressing Tab does NOT close the modal', () => {
@@ -384,7 +384,7 @@ describe('SettingsModal — close behaviours', () => {
384384
const dialog = screen.getByRole('dialog')
385385
fireEvent.keyDown(dialog, { key: 'Tab', code: 'Tab' })
386386
// Modal should still be open
387-
expect(useEditorStore.getState().settingsModalOpen).toBe(true)
387+
expect(useEditorStore.getState().isSettingsOpen).toBe(true)
388388
})
389389
})
390390

@@ -400,7 +400,7 @@ describe('SettingsModal — focus trap keyboard logic', () => {
400400
// The dialog should handle keydown (for Esc + Tab trap).
401401
// We verify the Escape path works as a proxy for the handler being present.
402402
fireEvent.keyDown(dialog, { key: 'Escape' })
403-
expect(useEditorStore.getState().settingsModalOpen).toBe(false)
403+
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
404404
})
405405

406406
it('pressing Escape on a child element inside the dialog closes the modal', () => {
@@ -409,7 +409,7 @@ describe('SettingsModal — focus trap keyboard logic', () => {
409409
const closeBtn = screen.getByLabelText('Close settings')
410410
// Escape on a child — should bubble to dialog's onKeyDown
411411
fireEvent.keyDown(closeBtn, { key: 'Escape' })
412-
expect(useEditorStore.getState().settingsModalOpen).toBe(false)
412+
expect(useEditorStore.getState().isSettingsOpen).toBe(false)
413413
})
414414
})
415415

@@ -598,14 +598,14 @@ describe('SettingsModal — Guideline #225 focus return (source enforcement)', (
598598
// uiSlice.ts store default — FIXED by Performance Engineer (#358) ✅
599599
// ---------------------------------------------------------------------------
600600

601-
describe('SettingsButton + uiSlice — section ID alignment (source enforcement)', () => {
601+
describe('SettingsButton + settingsSlice — section ID alignment (source enforcement)', () => {
602602
const btnSrc = readFileSync(
603603
new URL('../../admin/pages/site/toolbar/SettingsButton.tsx', import.meta.url),
604604
'utf-8',
605605
)
606606

607-
const uiSliceSrc = readFileSync(
608-
new URL('../../admin/pages/site/store/slices/uiSlice.ts', import.meta.url),
607+
const settingsSliceSrc = readFileSync(
608+
new URL('../../admin/pages/site/store/slices/settingsSlice.ts', import.meta.url),
609609
'utf-8',
610610
)
611611

@@ -615,10 +615,12 @@ describe('SettingsButton + uiSlice — section ID alignment (source enforcement)
615615
expect(btnSrc).not.toContain("openSettings('general')")
616616
})
617617

618-
it('uiSlice settingsModalSection default is a valid section ID (not "general")', () => {
619-
// Fixed in Contribution #358: settingsModalSection: 'general' → 'pages'.
620-
// Also openSettingsModal default parameter: section = 'general' → 'pages'.
621-
expect(uiSliceSrc).not.toContain("settingsModalSection: 'general'")
618+
it('settingsSlice activeSection default is a valid section ID', () => {
619+
// The default literal must be one of the NAV_ITEMS ids — picking an
620+
// invalid one (e.g. retired "typography" / "colors") would silently
621+
// fall through to "general" via `normalizeSection` in SettingsModal,
622+
// hiding what the user is supposed to see on first open.
623+
expect(settingsSliceSrc).toMatch(/DEFAULT_SECTION: SettingsSection = '(general|pages|breakpoints|shortcuts|publishing|preferences|modules)'/)
622624
})
623625
})
624626

‎src/__tests__/toolbar/openPageInNewTabButton.test.tsx‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
22
import React, { type ReactNode } from 'react'
33
import { cleanup, fireEvent, render, screen, within } from '@testing-library/react'
44
import { MemoryRouter } from '@admin/lib/routing'
5-
import { Toolbar } from '@site/toolbar'
5+
import { Toolbar } from '@site/toolbar/Toolbar'
6+
import { PublishButton } from '@site/toolbar/PublishButton'
67
import { AdminSessionProvider } from '@admin/session'
78
import { StepUpProvider } from '@admin/shared/StepUp'
89
import { useEditorStore } from '@site/store/store'
@@ -87,7 +88,15 @@ describe('Toolbar publishing actions', () => {
8788
}) as typeof window.open
8889

8990
try {
90-
render(<Wrapper><Toolbar /></Wrapper>)
91+
// Toolbar is now prop-driven — the editor-only buttons (zoom,
92+
// publish, settings) are passed in via the `rightSlot` by
93+
// AdminCanvasLayout. This test exercises the PublishButton's
94+
// "more actions" menu, so it has to provide the same mount.
95+
render(
96+
<Wrapper>
97+
<Toolbar rightSlot={<PublishButton enabled={true} />} />
98+
</Wrapper>,
99+
)
91100

92101
const toolbar = screen.getByTestId('toolbar')
93102
fireEvent.click(within(toolbar).getByRole('button', { name: /more publishing actions/i }))

‎src/__tests__/toolbar/toolbar.test.ts‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -425,21 +425,36 @@ describe('Toolbar — structural requirements', () => {
425425
expect(src).toContain('data-testid="toolbar"')
426426
})
427427

428-
it('Toolbar imports and renders all required sub-components', () => {
429-
const { readFileSync } = require('fs')
430-
const src = readFileSync(
428+
it('Toolbar is a prop-driven shell — editor-only buttons live in AdminCanvasLayout', () => {
429+
// After the Toolbar refactor, Toolbar.tsx itself no longer references
430+
// editor-specific sub-components (ZoomControls / PublishButton /
431+
// SettingsButton / save status). Those are passed in via the
432+
// `rightSlot` prop by AdminCanvasLayout, which keeps Toolbar shareable
433+
// with AdminPageLayout (Plugins / Users / Account) without dragging
434+
// the editor store into the non-editor admin bundle.
435+
const { readFileSync } = require('fs')
436+
const toolbarSrc = readFileSync(
431437
new URL('../../admin/pages/site/toolbar/Toolbar.tsx', import.meta.url),
432438
'utf-8',
433439
)
434-
// Undo/Redo lives in the canvas notch, not the toolbar — verified separately.
435-
expect(src).not.toContain('UndoRedoButtons')
436-
expect(src).toContain('ZoomControls')
437-
expect(src).not.toContain('ModulePickerDropdown')
438-
expect(src).toContain('PublishButton')
439-
expect(src).not.toContain('ExportButton')
440-
expect(src).toContain('SettingsButton')
441-
expect(src).not.toContain('SaveIndicator')
442-
expect(src).toContain('saveStatus={saveStatus}')
440+
// Toolbar.tsx must not import the editor-only sub-components.
441+
expect(toolbarSrc).not.toContain('UndoRedoButtons')
442+
expect(toolbarSrc).not.toContain('ModulePickerDropdown')
443+
expect(toolbarSrc).not.toContain('ExportButton')
444+
expect(toolbarSrc).not.toContain('SaveIndicator')
445+
expect(toolbarSrc).not.toContain("from './ZoomControls'")
446+
expect(toolbarSrc).not.toContain("from './PublishButton'")
447+
expect(toolbarSrc).not.toContain('saveStatus={saveStatus}')
448+
449+
// The editor-only buttons must be mounted from AdminCanvasLayout.
450+
const layoutSrc = readFileSync(
451+
new URL('../../admin/layouts/AdminCanvasLayout/AdminCanvasLayout.tsx', import.meta.url),
452+
'utf-8',
453+
)
454+
expect(layoutSrc).toContain('ZoomControls')
455+
expect(layoutSrc).toContain('PublishButton')
456+
expect(layoutSrc).toContain('SettingsButton')
457+
expect(layoutSrc).toContain('saveStatus={persistence.saveStatus}')
443458
})
444459

445460
it('module picker trigger has data-testid for Playwright', () => {
@@ -603,7 +618,12 @@ describe('Toolbar — structural requirements', () => {
603618
expect(src).toContain('const persistence = usePersistence(')
604619
expect(src).toContain("'default'")
605620
expect(src).toContain('cmsAdapter')
606-
expect(src).toContain('publishEnabled')
621+
// PublishButton's `enabled` prop carries the publish gating now
622+
// (the old `publishEnabled` toolbar prop was dropped when Toolbar
623+
// became prop-driven — AdminCanvasLayout mounts PublishButton itself
624+
// inside the toolbar's rightSlot).
625+
expect(src).toContain('<PublishButton')
626+
expect(src).toContain('enabled={workspace === \'site\' && canPublishPages}')
607627
})
608628

609629
it('touch targets: all toolbar buttons have a defined compact height (Guideline #357)', () => {

‎src/__tests__/toolbar/vcBreadcrumb.test.tsx‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -365,12 +365,23 @@ describe('VCBreadcrumb — structural source checks', () => {
365365
expect(src).toContain('validateComponentName')
366366
})
367367

368-
it('Toolbar.tsx imports and renders VCBreadcrumb', () => {
369-
const src = require('fs').readFileSync(
368+
it('Toolbar exposes a breadcrumb region that AdminCanvasLayout mounts VCBreadcrumb into', () => {
369+
// After the Toolbar refactor, the lazy VCBreadcrumb mount lives in
370+
// AdminCanvasLayout (the only layout that ever enters VC mode), and
371+
// Toolbar exposes a `breadcrumbSlot` prop that it renders into a
372+
// dedicated DOM region. Verifies both ends of the seam.
373+
const toolbarSrc = require('fs').readFileSync(
370374
new URL('../../admin/pages/site/toolbar/Toolbar.tsx', import.meta.url),
371375
'utf-8',
372376
)
373-
expect(src).toContain('VCBreadcrumb')
374-
expect(src).toContain('breadcrumbRegion')
377+
expect(toolbarSrc).toContain('breadcrumbSlot')
378+
expect(toolbarSrc).toContain('breadcrumbRegion')
379+
380+
const layoutSrc = require('fs').readFileSync(
381+
new URL('../../admin/layouts/AdminCanvasLayout/AdminCanvasLayout.tsx', import.meta.url),
382+
'utf-8',
383+
)
384+
expect(layoutSrc).toContain('VCBreadcrumb')
385+
expect(layoutSrc).toContain('breadcrumbSlot')
375386
})
376387
})

0 commit comments

Comments
 (0)