Tech-debt batch 18: frontend — settings drafts and connection tests, VerifyEmail states, native downloads, inert mobile drawer (#637 #638)

CHANGELOG.md

@@ -21,6 +21,14 @@ All notable changes to this project are documented in this file. The format is b
   an owner are suspended. Existing roles no longer emit `RoleAssigned` (#627).
   `UserDeleted` now lives in `simple_module_core.user_events` (still re-exported from
   `users.contracts.events`) so modules can subscribe without depending on `users`.
+- Frontend: verification failures show a generic error unless the server reports a bad
+  token or an already-verified account; file downloads bypass Inertia so signed-URL
+  redirects work; closed mobile sidebars are inert and hidden from assistive technology,
+  while desktop sidebars remain accessible (#638).
+- Settings: reverting a field preserves other unsaved edits; connection-test results
+  stay with their module, and unreadable or empty check responses report failure (#637).
+  `@simple-module-py/ui`: `readBody` now rethrows a body-read failure instead of returning
+  `null`; `apiFetch` reports it as a network failure (status 0).
 - Release tooling converts Python prereleases to npm SemVer and publishes them
   with the `next` tag; metadata checks catch missing/stale npm matrix entries.
   Memray loadtests refuse occupied ports and stop only their own process group (#629).

modules/file_storage/file_storage/pages/components/FileTable.tsx

@@ -141,7 +141,9 @@ export function FileTable({
                   size="sm"
                   className="h-auto p-0 text-primary-700 max-lg:min-h-11"
                 >
-                  <a href={ROUTES.apiDownload(file.id)}>{t(keys.file_storage.actions.download)}</a>
+                  <a href={ROUTES.apiDownload(file.id)} data-native-link>
+                    {t(keys.file_storage.actions.download)}
+                  </a>
                 </Button>
                 {canPublish && (
                   <Button

modules/file_storage/tests-js/FileTable.test.tsx

@@ -1,5 +1,6 @@
 import '@testing-library/jest-dom/vitest';
 import { configureI18n } from '@simple-module-py/i18n';
+import { shouldInterceptNavigation } from '@simple-module-py/ui/lib/spa-links';
 import { fireEvent, render, screen } from '@testing-library/react';
 import { describe, expect, test, vi } from 'vitest';
 
@@ -101,3 +102,12 @@ describe('FileTable visibility', () => {
     expect(onTogglePublic).toHaveBeenCalledWith(FILES[0]);
   });
 });
+
+describe('FileTable downloads (#638)', () => {
+  test('leaves the download endpoint to the browser for signed-URL redirects', () => {
+    renderTable();
+    const link = screen.getByRole<HTMLAnchorElement>('link', { name: 'Download' });
+    expect(link.getAttribute('href')).toBe('/api/file-storage/files/f1/download');
+    expect(shouldInterceptNavigation(link, new URL('http://localhost:3000/files'))).toBe(false);
+  });
+});

modules/settings/settings/pages/ModulesEdit.tsx

@@ -142,7 +142,11 @@ function ModulesEdit({ modules, testable = {} }: Props) {
               </div>
             </Card>
           ) : current ? (
-            <ModuleForm module={current} checks={testable[current.package] ?? []} />
+            <ModuleForm
+              key={current.package}
+              module={current}
+              checks={testable[current.package] ?? []}
+            />
           ) : (
             <p className="text-muted-foreground">{t(keys.settings.modules.empty_title)}</p>
           )}

modules/settings/settings/pages/components/ModuleForm.tsx

@@ -42,18 +42,23 @@ export function ModuleForm({ module: m, checks = [] }: Props) {
     return o;
   }, [m.fields]);
 
-  const [values, setValues] = useState<Record<string, unknown>>(initial);
-  const [baseline, setBaseline] = useState<Record<string, unknown>>(initial);
+  const [{ values, baseline }, setBuffer] = useState({ values: initial, baseline: initial });
   const [errors, setErrors] = useState<Record<string, string>>({});
   const [busy, setBusy] = useState(false);
   const [saved, setSaved] = useState(false);
   const [formErrors, setFormErrors] = useState<string[]>([]);
 
-  // Reset the edit buffer whenever the underlying module changes (package
-  // switch, or server-reloaded props after a save/reset).
+  // Refresh saved values without discarding drafts in other fields (#637).
   useEffect(() => {
-    setValues(initial);
-    setBaseline(initial);
+    setBuffer((prev) => ({
+      values: Object.fromEntries(
+        Object.entries(initial).map(([name, value]) => [
+          name,
+          notEqual(prev.values[name], prev.baseline[name]) ? prev.values[name] : value,
+        ]),
+      ),
+      baseline: initial,
+    }));
     setErrors({});
     setSaved(false);
   }, [initial]);
@@ -107,7 +112,7 @@ export function ModuleForm({ module: m, checks = [] }: Props) {
     } else {
       // Keeping the form mounted lets the confirmation remain visible instead
       // of being discarded by an immediate Inertia reload.
-      setBaseline({ ...values });
+      setBuffer((prev) => ({ ...prev, baseline: { ...values } }));
       setSaved(true);
     }
     setBusy(false);
@@ -121,6 +126,10 @@ export function ModuleForm({ module: m, checks = [] }: Props) {
         params: { path: { package: m.package, field: name } },
       }),
     );
+    setBuffer((prev) => ({
+      ...prev,
+      values: { ...prev.values, [name]: prev.baseline[name] },
+    }));
     router.reload({ only: ['modules'] });
   }
 
@@ -187,7 +196,10 @@ export function ModuleForm({ module: m, checks = [] }: Props) {
                   onReset={() => onReset(f.name)}
                   onChange={(name, v) => {
                     setSaved(false);
-                    setValues((prev) => ({ ...prev, [name]: v }));
+                    setBuffer((prev) => ({
+                      ...prev,
+                      values: { ...prev.values, [name]: v },
+                    }));
                   }}
                 />
               ))}

modules/settings/settings/pages/components/TestConnectionButton.tsx

@@ -3,7 +3,7 @@ import { Button } from '@simple-module-py/ui/components/ui/button';
 import { useRelativeTime } from '@simple-module-py/ui/hooks/use-relative-time';
 import { apiFetch } from '@simple-module-py/ui/lib/api';
 import { CheckCircle2, PlugZap, XCircle } from 'lucide-react';
-import { useEffect, useState } from 'react';
+import { useEffect, useRef, useState } from 'react';
 import { ROUTES } from '../routes';
 import { type LastTest, readLastTest, writeLastTest } from './last-test';
 
@@ -35,9 +35,19 @@ export function TestConnectionButton({ pkg, checks }: Props) {
   const [error, setError] = useState<string | null>(null);
   const [last, setLast] = useState<LastTest | null>(null);
 
+  const requestId = useRef(0);
+
   // Read on mount rather than during render: the result is per-tab session
   // state, and touching sessionStorage while rendering breaks SSR hydration.
-  useEffect(() => setLast(readLastTest(pkg)), [pkg]);
+  useEffect(() => {
+    setLast(readLastTest(pkg));
+    setResults(null);
+    setError(null);
+    setBusy(false);
+    return () => {
+      requestId.current++;
+    };
+  }, [pkg]);
 
   // "users.mailer" → "mailer". Naming the check is the difference between
   // "Test connection" and knowing an SMTP dial is about to happen; with two
@@ -50,15 +60,17 @@ export function TestConnectionButton({ pkg, checks }: Props) {
       : t(keys.settings.modules.test_connection_generic);
 
   async function run() {
+    const id = ++requestId.current;
     setBusy(true);
     setError(null);
     setResults(null);
     // A view route (`/admin/settings/test-connection/…`), not in the OpenAPI document.
     const result = await apiFetch<{ checks?: CheckResult[] }>(ROUTES.testConnection(pkg), {
       method: 'POST',
     });
-    if (result.ok) {
-      const checkResults = result.data?.checks ?? [];
+    if (id !== requestId.current) return;
+    const checkResults = result.data?.checks;
+    if (result.ok && Array.isArray(checkResults) && checkResults.length > 0) {
       setResults(checkResults);
       setLast(
         writeLastTest(

modules/settings/tests-js/settings_ModuleFormRevert.test.tsx

@@ -0,0 +1,88 @@
+import '@testing-library/jest-dom/vitest';
+import { configureI18n } from '@simple-module-py/i18n';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { afterEach, expect, test, vi } from 'vitest';
+import type { FieldMeta } from '../settings/pages/components/FieldInput';
+import { ModuleForm, type ModuleView } from '../settings/pages/components/ModuleForm';
+import ModulesEdit from '../settings/pages/ModulesEdit';
+
+const mocks = vi.hoisted(() => ({ reload: vi.fn() }));
+vi.mock('@inertiajs/react', () => ({ router: { reload: mocks.reload }, Head: () => null }));
+configureI18n({
+  locale: 'en',
+  messages: {
+    'settings.modules_form.save': 'Save',
+    'settings.modules_form.reset_to_default': 'Revert',
+    'settings.modules.unsaved_count_one': '{count} unsaved',
+  },
+});
+
+function field(name: string, value: string, db_override = false): FieldMeta {
+  return {
+    name,
+    value,
+    db_override,
+    type: 'string',
+    default: '',
+    description: '',
+    is_secret: false,
+    requires_restart: false,
+    group: null,
+    env_var: 'SM_TEST_FIELD',
+    env_set: false,
+    env_readable: false,
+    choices: null,
+  };
+}
+function view(fields: FieldMeta[]): ModuleView {
+  return {
+    module_name: 'Users',
+    package: 'users',
+    env_prefix: 'SM_USERS_',
+    class_name: 'UsersSettings',
+    fields,
+  };
+}
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+  vi.clearAllMocks();
+});
+
+test('reverting one field preserves other drafts and their saved baseline after props reload', async () => {
+  const fetchMock = vi.fn(async () => new Response('{}', { status: 200 }));
+  vi.stubGlobal('fetch', fetchMock);
+  const { rerender } = render(
+    <ModuleForm
+      module={view([field('smtp_host', 'override', true), field('sender', 'original')])}
+    />,
+  );
+  fireEvent.change(screen.getByLabelText('sender'), { target: { value: 'draft' } });
+  fireEvent.change(screen.getByLabelText('smtp_host'), { target: { value: 'discard me' } });
+  fireEvent.click(screen.getByRole('button', { name: 'Revert' }));
+  await waitFor(() => expect(mocks.reload).toHaveBeenCalled());
+  rerender(
+    <ModuleForm module={view([field('smtp_host', 'default'), field('sender', 'original')])} />,
+  );
+  expect(screen.getByLabelText('smtp_host')).toHaveValue('default');
+  expect(screen.getByLabelText('sender')).toHaveValue('draft');
+  expect(screen.getByText('1 unsaved')).toBeVisible();
+  fireEvent.click(screen.getByRole('button', { name: 'Save' }));
+  await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2));
+  const [request] = fetchMock.mock.calls[1] as unknown as [Request];
+  expect(await request.json()).toEqual({ sender: 'draft' });
+});
+
+test('switching modules starts a fresh edit buffer even when field names match', () => {
+  const users = view([field('sender', 'users original')]);
+  const mail = {
+    ...view([field('sender', 'mail original')]),
+    module_name: 'Mail',
+    package: 'mail',
+  };
+  render(<ModulesEdit modules={[users, mail]} />);
+  fireEvent.change(screen.getByLabelText('sender'), { target: { value: 'users draft' } });
+  fireEvent.click(screen.getByRole('button', { name: /mail/ }));
+  expect(screen.getByLabelText('sender')).toHaveValue('mail original');
+  expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled();
+});

modules/settings/tests-js/settings_TestConnectionButton.test.tsx

@@ -1,6 +1,6 @@
 import '@testing-library/jest-dom/vitest';
 import { configureI18n, keys } from '@simple-module-py/i18n';
-import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { act, fireEvent, render, screen, waitFor } from '@testing-library/react';
 import { afterEach, describe, expect, it, vi } from 'vitest';
 
 configureI18n({ locale: 'en', messages: {} });
@@ -24,6 +24,70 @@ describe('TestConnectionButton', () => {
     window.sessionStorage.clear();
   });
 
+  it('clears the displayed result when the package changes', async () => {
+    stub(200, { checks: [{ name: 'a.b', status: 'healthy', detail: 'users connected' }] });
+    const { rerender } = render(<TestConnectionButton pkg="users" checks={['a.b', 'c.d']} />);
+    click();
+    expect(await screen.findByText('users connected')).toBeVisible();
+    rerender(<TestConnectionButton pkg="mail" checks={['a.b', 'c.d']} />);
+    expect(screen.queryByText('users connected')).not.toBeInTheDocument();
+  });
+
+  it('ignores a pending result after switching packages, including switching back', async () => {
+    let finish!: (response: Response) => void;
+    vi.stubGlobal(
+      'fetch',
+      vi.fn(
+        () =>
+          new Promise<Response>((resolve) => {
+            finish = resolve;
+          }),
+      ),
+    );
+    const { rerender } = render(<TestConnectionButton pkg="users" checks={['a.b', 'c.d']} />);
+    click();
+    rerender(<TestConnectionButton pkg="mail" checks={['a.b', 'c.d']} />);
+    expect(screen.getByRole('button')).toBeEnabled();
+    rerender(<TestConnectionButton pkg="users" checks={['a.b', 'c.d']} />);
+    await act(async () =>
+      finish(
+        new Response(
+          JSON.stringify({
+            checks: [{ name: 'a.b', status: 'healthy', detail: 'stale result' }],
+          }),
+          { status: 200 },
+        ),
+      ),
+    );
+    expect(screen.queryByText('stale result')).not.toBeInTheDocument();
+    expect(window.sessionStorage.getItem('sm.settings.last-test.users')).toBeNull();
+  });
+
+  it.each([null, {}, { checks: [] }, { checks: 'invalid' }])(
+    'does not record success for an invalid check response: %j',
+    async (body) => {
+      stub(200, body);
+      render(<TestConnectionButton pkg="users" checks={['a.b', 'c.d']} />);
+      click();
+      expect(await screen.findByText(keys.settings.modules.test_failed)).toBeVisible();
+      expect(
+        JSON.parse(window.sessionStorage.getItem('sm.settings.last-test.users') ?? '{}').ok,
+      ).toBe(false);
+    },
+  );
+
+  it('does not record success when the response body cannot be read', async () => {
+    const response = new Response('{}', { status: 200 });
+    vi.spyOn(response, 'text').mockRejectedValue(new TypeError('stream interrupted'));
+    vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response));
+    render(<TestConnectionButton pkg="users" checks={['a.b', 'c.d']} />);
+    click();
+    expect(await screen.findByText(keys.settings.modules.test_failed)).toBeVisible();
+    expect(
+      JSON.parse(window.sessionStorage.getItem('sm.settings.last-test.users') ?? '{}').ok,
+    ).toBe(false);
+  });
+
   it('POSTs to the view route and shows each check detail', async () => {
     const fetchMock = stub(200, {
       checks: [{ name: 'users.mailer', status: 'unhealthy', detail: 'connection refused' }],