Surface publish warnings in the editor instead of discarding them #26

Merged
jknapp merged 2 commits from feat/publish-warnings into main 2026-07-16 12:34:33 +00:00
5 changed files with 323 additions and 0 deletions
@@ -0,0 +1,57 @@
import { describe, test, expect, vi, afterEach } from 'vitest';
import React from 'react';
import { createRoot, Root } from 'react-dom/client';
import { act } from 'react-dom/test-utils';
import { PublishWarnings } from './PublishWarnings';
/* ---------- DOM test harness (no @testing-library/react in this repo, see
src/ui/AssetPicker.test.tsx for the same pattern: react-dom/client +
react-dom/test-utils `act`, both transitive deps of react-dom already). ---------- */
let container: HTMLDivElement;
let root: Root;
function render(ui: React.ReactElement) {
container = document.createElement('div');
document.body.appendChild(container);
act(() => {
root = createRoot(container);
root.render(ui);
});
}
function click(el: Element | null) {
if (!el) throw new Error('element not found');
act(() => { (el as HTMLElement).dispatchEvent(new MouseEvent('click', { bubbles: true })); });
}
afterEach(() => {
if (container) {
act(() => { root.unmount(); });
container.remove();
}
});
describe('PublishWarnings', () => {
test('renders nothing when there are no warnings', () => {
render(<PublishWarnings warnings={[]} onDismiss={() => {}} />);
expect(container.innerHTML).toBe('');
});
test('renders each warning', () => {
render(
<PublishWarnings
warnings={['First problem.', 'Second problem.']}
onDismiss={() => {}}
/>,
);
expect(container.textContent).toContain('First problem.');
expect(container.textContent).toContain('Second problem.');
});
test('dismiss fires the callback', () => {
const onDismiss = vi.fn();
render(<PublishWarnings warnings={['A problem.']} onDismiss={onDismiss} />);
click(container.querySelector('[data-testid="publish-warnings-dismiss"]'));
expect(onDismiss).toHaveBeenCalledTimes(1);
});
});
@@ -0,0 +1,31 @@
import React from 'react';
export interface PublishWarningsProps {
warnings: string[];
onDismiss: () => void;
}
/** Non-blocking banner shown after a successful publish. The site IS live --
* these are things the customer should fix and re-publish, not failures. */
export const PublishWarnings: React.FC<PublishWarningsProps> = ({ warnings, onDismiss }) => {
if (!warnings.length) return null;
return (
<div className="publish-warnings" role="status" data-testid="publish-warnings">
<i className="fa fa-exclamation-triangle" aria-hidden="true" />
<ul>
{warnings.map((w, i) => (
<li key={i}>{w}</li>
))}
</ul>
<button
type="button"
onClick={onDismiss}
aria-label="Dismiss"
data-testid="publish-warnings-dismiss"
>
<i className="fa fa-times" aria-hidden="true" />
</button>
</div>
);
};
+191
View File
@@ -0,0 +1,191 @@
import { describe, test, expect, vi, afterEach } from 'vitest';
import React from 'react';
import { createRoot, Root } from 'react-dom/client';
import { act } from 'react-dom/test-utils';
/**
* Closes the gap flagged by the whole-branch review: PublishWarnings.test.tsx
* covers the presentational banner in isolation, but nothing asserted that
* `result.warnings` from `publish()` actually reaches it through TopBar. That
* 3-line seam (handlePublish -> setPublishWarnings -> <PublishWarnings>) is
* exactly what regressed before this feature existed: the backend has always
* returned `warnings`, and TopBar discarded them by only checking
* `result.success` -- so the contact-form-relay warning was dead code for its
* entire life. This suite drives the real `<TopBar>` through a real button
* click and asserts the warnings show up, survive the 3s "Published" flash,
* don't taint the success/error status, and get cleared by a fresh publish.
*
* Mocks (same DOM-harness + `vi.mock('@craftjs/core', ...)` pattern as
* RenderNode.test.tsx / useWhpApi.load.test.tsx -- no @testing-library/react
* in this repo):
* - `@craftjs/core`'s `useEditor`: TopBar only needs inert undo/redo/query
* stubs, not a real Craft.js tree.
* - `useWhpApi`: this IS the seam under test -- `publish` is a controllable
* mock so each test can choose exactly what the "backend" returns.
* - TemplateModal / HeadCodeModal / SitesmithButton: sibling chrome
* unrelated to the warnings seam (portals, CodeMirror lazy-load,
* useSitesmith's own fetch calls) -- stubbed out so this suite stays
* focused, same as MobilePanelBar.test.tsx stubbing its sibling panels.
* `usePages`/`useSiteDesign`/`useMobileChrome` are left un-mocked and
* un-provided -- their default context values (defined in each context
* module) are harmless no-op stubs, and TopBar's desktop render path never
* needs more than that.
*/
vi.mock('@craftjs/core', () => ({
useEditor: (collector?: (state: unknown, query: unknown) => Record<string, unknown>) => {
const query = {
serialize: () => '{}',
history: { canUndo: () => false, canRedo: () => false },
};
const actions = { history: { undo: vi.fn(), redo: vi.fn() } };
const collected = collector ? collector({}, query) : {};
return { actions, query, ...collected };
},
}));
const publishMock = vi.fn();
vi.mock('../../hooks/useWhpApi', () => ({
useWhpApi: () => ({
save: vi.fn().mockResolvedValue({ success: true }),
publish: publishMock,
load: vi.fn().mockResolvedValue(null),
uploadAsset: vi.fn(),
isWHP: true,
}),
}));
vi.mock('./TemplateModal', () => ({ TemplateModal: () => null }));
vi.mock('./HeadCodeModal', () => ({ HeadCodeModal: () => null }));
vi.mock('../sitesmith/SitesmithButton', () => ({ SitesmithButton: () => null }));
import { TopBar } from './TopBar';
import { EditorConfigProvider } from '../../state/EditorConfigContext';
import { WhpConfig } from '../../types';
const whpConfig: WhpConfig = {
user: 'testuser',
apiUrl: '/panel/api/site-builder',
csrfToken: 'tok',
siteId: 1,
siteDomain: 'example.com',
siteName: 'Example Site',
backUrl: '/panel/sites',
isRoot: false,
};
let container: HTMLDivElement;
let root: Root;
function render() {
container = document.createElement('div');
document.body.appendChild(container);
act(() => {
root = createRoot(container);
root.render(
<EditorConfigProvider config={whpConfig}>
<TopBar device="desktop" onDeviceChange={() => {}} showGuides={false} onToggleGuides={() => {}} />
</EditorConfigProvider>,
);
});
}
function publishButton(): HTMLButtonElement {
const btn = container.querySelector<HTMLButtonElement>('.topbar-btn.publish');
if (!btn) throw new Error('Publish button not found');
return btn;
}
/** handlePublish does exactly one `await publish()` before touching state;
* two microtask flushes inside the same act() batch is enough to carry that
* through to the resulting re-render. */
async function clickPublish() {
await act(async () => {
publishButton().dispatchEvent(new MouseEvent('click', { bubbles: true }));
await Promise.resolve();
await Promise.resolve();
});
}
afterEach(() => {
if (container) {
act(() => { root.unmount(); });
container.remove();
}
publishMock.mockReset();
vi.useRealTimers();
});
describe('TopBar publish-warnings wiring', () => {
test('warnings from publish() flow through into the rendered banner', async () => {
publishMock.mockResolvedValue({ success: true, warnings: ['Warning one.', 'Warning two.'] });
render();
await clickPublish();
expect(container.textContent).toContain('Warning one.');
expect(container.textContent).toContain('Warning two.');
});
test('a clean publish (no warnings key) renders no banner at all', async () => {
publishMock.mockResolvedValue({ success: true });
render();
await clickPublish();
expect(container.querySelector('[data-testid="publish-warnings"]')).toBeNull();
});
test('a warning is non-blocking -- publish still reports success, not a failure', async () => {
publishMock.mockResolvedValue({
success: true,
warnings: ['Submissions will not be delivered until an administrator enables it.'],
});
render();
await clickPublish();
expect(container.querySelector('.publish-badge.published')).not.toBeNull();
expect(container.querySelector('.save-indicator.error')).toBeNull();
expect(container.textContent).toContain('Submissions will not be delivered until an administrator enables it.');
});
test('warnings survive the 3-second "Published" flash timer', async () => {
vi.useFakeTimers();
publishMock.mockResolvedValue({ success: true, warnings: ['Sticks around after the flash.'] });
render();
await act(async () => {
publishButton().dispatchEvent(new MouseEvent('click', { bubbles: true }));
await Promise.resolve();
await Promise.resolve();
});
// Sanity: both the flash and the warning are up before the timer fires.
expect(container.querySelector('.publish-badge.published')).not.toBeNull();
expect(container.textContent).toContain('Sticks around after the flash.');
act(() => {
vi.advanceTimersByTime(3000);
});
// The 3s timer resets publishStatus -> the "Published" flash is gone...
expect(container.querySelector('.publish-badge.published')).toBeNull();
// ...but publishWarnings lives in its own state and must NOT have been
// cleared by that same timer.
expect(container.textContent).toContain('Sticks around after the flash.');
});
test('a new publish attempt clears warnings left over from the previous one', async () => {
publishMock.mockResolvedValueOnce({ success: true, warnings: ['Old warning.'] });
render();
await clickPublish();
expect(container.textContent).toContain('Old warning.');
publishMock.mockResolvedValueOnce({ success: true });
await clickPublish();
expect(container.textContent).not.toContain('Old warning.');
expect(container.querySelector('[data-testid="publish-warnings"]')).toBeNull();
});
});
+9
View File
@@ -10,6 +10,7 @@ import { DeviceMode } from '../../types';
import { TemplateModal } from './TemplateModal'; import { TemplateModal } from './TemplateModal';
import { HeadCodeModal } from './HeadCodeModal'; import { HeadCodeModal } from './HeadCodeModal';
import { TopBarOverflowMenu } from './TopBarOverflowMenu'; import { TopBarOverflowMenu } from './TopBarOverflowMenu';
import { PublishWarnings } from './PublishWarnings';
import { SitesmithButton } from '../sitesmith/SitesmithButton'; import { SitesmithButton } from '../sitesmith/SitesmithButton';
import { useSitesmithModal } from '../../state/SitesmithContext'; import { useSitesmithModal } from '../../state/SitesmithContext';
@@ -33,6 +34,7 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
const [saveStatus, setSaveStatus] = useState<'idle' | 'saving' | 'saved' | 'error'>('idle'); const [saveStatus, setSaveStatus] = useState<'idle' | 'saving' | 'saved' | 'error'>('idle');
const [publishStatus, setPublishStatus] = useState<'idle' | 'publishing' | 'published' | 'error'>('idle'); const [publishStatus, setPublishStatus] = useState<'idle' | 'publishing' | 'published' | 'error'>('idle');
const [publishWarnings, setPublishWarnings] = useState<string[]>([]);
const [isDraft, setIsDraft] = useState(false); const [isDraft, setIsDraft] = useState(false);
// Mobile-A2: lifted from private useState into MobileChromeContext so // Mobile-A2: lifted from private useState into MobileChromeContext so
// opening a mobile sheet can close these modals (item 3) -- behavior is // opening a mobile sheet can close these modals (item 3) -- behavior is
@@ -100,11 +102,16 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
const handlePublish = useCallback(async () => { const handlePublish = useCallback(async () => {
setPublishStatus('publishing'); setPublishStatus('publishing');
setPublishWarnings([]);
try { try {
const result = await publish(); const result = await publish();
if (result?.success) { if (result?.success) {
setPublishStatus('published'); setPublishStatus('published');
setIsDraft(false); setIsDraft(false);
// The site published; these are fixable problems, not failures. Held
// independently of publishStatus so the 3s "Published" flash doesn't
// take the warning down with it.
setPublishWarnings(Array.isArray(result.warnings) ? result.warnings : []);
if (publishTimeoutRef.current) clearTimeout(publishTimeoutRef.current); if (publishTimeoutRef.current) clearTimeout(publishTimeoutRef.current);
publishTimeoutRef.current = setTimeout(() => setPublishStatus('idle'), 3000); publishTimeoutRef.current = setTimeout(() => setPublishStatus('idle'), 3000);
} else { } else {
@@ -220,6 +227,7 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
if (isMobile) { if (isMobile) {
return ( return (
<nav className="topbar topbar-mobile"> <nav className="topbar topbar-mobile">
<PublishWarnings warnings={publishWarnings} onDismiss={() => setPublishWarnings([])} />
<div className="topbar-left"> <div className="topbar-left">
{isWHP && ( {isWHP && (
<a <a
@@ -307,6 +315,7 @@ export const TopBar: React.FC<TopBarProps> = ({ device, onDeviceChange, showGuid
return ( return (
<nav className="topbar"> <nav className="topbar">
<PublishWarnings warnings={publishWarnings} onDismiss={() => setPublishWarnings([])} />
<div className="topbar-left"> <div className="topbar-left">
{isWHP && ( {isWHP && (
<a href={whpConfig!.backUrl} className="topbar-btn back-btn" aria-label="Back to Panel"> <a href={whpConfig!.backUrl} className="topbar-btn back-btn" aria-label="Back to Panel">
+35
View File
@@ -106,6 +106,11 @@ body {
border-bottom: 1px solid var(--color-border); border-bottom: 1px solid var(--color-border);
z-index: 100; z-index: 100;
gap: 12px; gap: 12px;
/* Positioned ancestor for .publish-warnings (position: absolute; top:
100%), which is rendered as this <nav>'s first child in both the
desktop and mobile branches -- without this, it would anchor to the
viewport instead of sitting directly under the topbar. */
position: relative;
} }
.topbar-left, .topbar-left,
@@ -1884,3 +1889,33 @@ body {
height: 44px !important; height: 44px !important;
} }
} }
/* --------------------------------------------------------------------------
Publish warnings banner -- non-blocking; the site DID publish. Anchored
to .topbar's `position: relative` (see above) so it drops down directly
beneath the bar in both the desktop and mobile branches.
-------------------------------------------------------------------------- */
.publish-warnings {
position: absolute;
top: 100%;
left: 0;
right: 0;
z-index: 40;
display: flex;
align-items: flex-start;
gap: 8px;
padding: 10px 12px;
background: #422006;
border-bottom: 1px solid #a16207;
color: #fde68a;
font-size: 12px;
}
.publish-warnings ul { margin: 0; padding-left: 16px; flex: 1; }
.publish-warnings li { margin: 2px 0; }
.publish-warnings button {
background: none;
border: none;
color: #fde68a;
cursor: pointer;
padding: 0 4px;
}