Compare commits

..
Author SHA1 Message Date
shadowdaoandClaude Opus 4.8 c712a69c4a test(topbar): cover publish() warnings wiring into PublishWarnings banner
PublishWarnings.tsx had unit tests for the presentational banner, but
nothing asserted that result.warnings from publish() actually flows
through TopBar's handlePublish into it. That 3-line seam is exactly what
this feature exists to fix -- the backend 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.

Adds TopBar.test.tsx asserting: warnings render after a successful
publish with warnings, no banner renders when warnings is absent, a
warning doesn't present as a publish failure, warnings survive the 3s
"Published" flash (fake timers, advanced past 3000ms), and a fresh
publish clears stale warnings from the previous one.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-15 22:12:31 -07:00
shadowdaoandClaude Opus 4.8 d460e8ac33 topbar: surface publish warnings instead of discarding them
handlePublish's JSON response has always included a `warnings` array
(e.g. the contact-form relay's "submissions will not be delivered"
notice), but nothing in the editor ever read it. Adds a PublishWarnings
banner, held in its own state independent of the 3s publishStatus
flash so the customer has time to read it, rendered in both the
desktop and mobile TopBar branches.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-15 21:52:11 -07:00
jknapp 2b1569202a Merge PR #25: bounce + image crop/resize fix 2026-07-14 23:07:58 +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;
}