From d5e7abdbd6243df1c299a5e647e331820eb2f740 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 20:31:48 +0200 Subject: [PATCH 1/2] settings: revert keeps other drafts, connection tests stay with their module, unreadable check responses fail (#637) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 4 + .../settings/settings/pages/ModulesEdit.tsx | 6 +- .../settings/pages/components/ModuleForm.tsx | 28 ++++-- .../pages/components/TestConnectionButton.tsx | 20 ++++- .../settings_ModuleFormRevert.test.tsx | 88 +++++++++++++++++++ .../settings_TestConnectionButton.test.tsx | 66 +++++++++++++- packages/ui/src/lib/api-result.ts | 2 +- packages/ui/src/lib/api.test.ts | 7 ++ packages/ui/src/lib/api.ts | 7 +- 9 files changed, 209 insertions(+), 19 deletions(-) create mode 100644 modules/settings/tests-js/settings_ModuleFormRevert.test.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index b58962012..73f4c7a89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ 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`. +- 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). - Database: discard `on_commit` callbacks registered inside a rolled-back savepoint, including released child savepoints, while preserving callbacks from surviving work (#631). - Role-permission edits, module settings updates/clears and system/tenant feature-flag diff --git a/modules/settings/settings/pages/ModulesEdit.tsx b/modules/settings/settings/pages/ModulesEdit.tsx index 586724361..0db34f4bb 100644 --- a/modules/settings/settings/pages/ModulesEdit.tsx +++ b/modules/settings/settings/pages/ModulesEdit.tsx @@ -142,7 +142,11 @@ function ModulesEdit({ modules, testable = {} }: Props) { ) : current ? ( - + ) : (

{t(keys.settings.modules.empty_title)}

)} diff --git a/modules/settings/settings/pages/components/ModuleForm.tsx b/modules/settings/settings/pages/components/ModuleForm.tsx index 09bad0035..0b4e8794a 100644 --- a/modules/settings/settings/pages/components/ModuleForm.tsx +++ b/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>(initial); - const [baseline, setBaseline] = useState>(initial); + const [{ values, baseline }, setBuffer] = useState({ values: initial, baseline: initial }); const [errors, setErrors] = useState>({}); const [busy, setBusy] = useState(false); const [saved, setSaved] = useState(false); const [formErrors, setFormErrors] = useState([]); - // 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 }, + })); }} /> ))} diff --git a/modules/settings/settings/pages/components/TestConnectionButton.tsx b/modules/settings/settings/pages/components/TestConnectionButton.tsx index f5124d27f..5b43c0a88 100644 --- a/modules/settings/settings/pages/components/TestConnectionButton.tsx +++ b/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(null); const [last, setLast] = useState(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,6 +60,7 @@ 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); @@ -57,8 +68,9 @@ export function TestConnectionButton({ pkg, checks }: Props) { 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( diff --git a/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx b/modules/settings/tests-js/settings_ModuleFormRevert.test.tsx new file mode 100644 index 000000000..4c2a3c46b --- /dev/null +++ b/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( + , + ); + 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( + , + ); + 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(); + 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(); +}); diff --git a/modules/settings/tests-js/settings_TestConnectionButton.test.tsx b/modules/settings/tests-js/settings_TestConnectionButton.test.tsx index 6adb03f0e..848f317c9 100644 --- a/modules/settings/tests-js/settings_TestConnectionButton.test.tsx +++ b/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(); + click(); + expect(await screen.findByText('users connected')).toBeVisible(); + rerender(); + 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((resolve) => { + finish = resolve; + }), + ), + ); + const { rerender } = render(); + click(); + rerender(); + expect(screen.getByRole('button')).toBeEnabled(); + rerender(); + 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(); + 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(); + 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' }], diff --git a/packages/ui/src/lib/api-result.ts b/packages/ui/src/lib/api-result.ts index fa0c9db2d..5276c2e47 100644 --- a/packages/ui/src/lib/api-result.ts +++ b/packages/ui/src/lib/api-result.ts @@ -66,7 +66,7 @@ export async function apiResult( /** Parse a raw response body: JSON when it is JSON, the text otherwise, null when empty. */ export async function readBody(response: Response): Promise { - const text = await response.text().catch(() => ''); + const text = await response.text(); if (!text) return null; try { return JSON.parse(text); diff --git a/packages/ui/src/lib/api.test.ts b/packages/ui/src/lib/api.test.ts index f941c317c..f62a9a348 100644 --- a/packages/ui/src/lib/api.test.ts +++ b/packages/ui/src/lib/api.test.ts @@ -134,6 +134,13 @@ describe('apiResult', () => { }); describe('apiFetch', () => { + it('reports a failed response body read as a network failure', async () => { + const response = new Response('{}', { status: 200 }); + vi.spyOn(response, 'text').mockRejectedValue(new TypeError('stream interrupted')); + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response)); + expect(await apiFetch('/x')).toMatchObject({ ok: false, status: 0, data: null }); + }); + it('serialises json and normalises the result', async () => { const fetchMock = stubFetch(200, { saved: true }); const result = await apiFetch<{ saved: boolean }>('/setup/save', { diff --git a/packages/ui/src/lib/api.ts b/packages/ui/src/lib/api.ts index 4d27bd7f5..1714b393a 100644 --- a/packages/ui/src/lib/api.ts +++ b/packages/ui/src/lib/api.ts @@ -71,7 +71,6 @@ export async function apiFetch( const { json, ...rest } = init; const headers = new Headers(rest.headers); if (json !== undefined) headers.set('Content-Type', 'application/json'); - let response: Response; try { const request = new Request(new URL(url, globalThis.location?.href), { credentials: 'same-origin', @@ -79,10 +78,10 @@ export async function apiFetch( headers, body: json !== undefined ? JSON.stringify(json) : rest.body, }); - response = await globalThis.fetch(prepare(request, currentCsrfToken)); + const response = await globalThis.fetch(prepare(request, currentCsrfToken)); + const body = await readBody(response); + return response.ok ? success(response.status, body as T) : failure(response.status, body); } catch { return failure(NETWORK_FAILURE, null); } - const body = await readBody(response); - return response.ok ? success(response.status, body as T) : failure(response.status, body); } From 25ba2ac59b7ceeb51f7c85ff515ea33ecc75cd62 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 24 Oct 2026 20:39:46 +0200 Subject: [PATCH 2/2] frontend: VerifyEmail distinguishes failures, downloads bypass Inertia, closed mobile drawer is inert (#638) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 4 ++ .../pages/components/FileTable.tsx | 4 +- .../file_storage/tests-js/FileTable.test.tsx | 10 +++ .../users/tests-js/users_VerifyEmail.test.tsx | 65 +++++++++++++++++++ modules/users/users/pages/VerifyEmail.tsx | 19 +++++- .../src/layouts/SidebarLayout.drawer.test.tsx | 61 ++++++++++++++++- .../ui/src/layouts/useDrawerA11y.test.tsx | 3 +- packages/ui/src/layouts/useDrawerA11y.ts | 25 ++++--- 8 files changed, 177 insertions(+), 14 deletions(-) create mode 100644 modules/users/tests-js/users_VerifyEmail.test.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index 73f4c7a89..886f35ea6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ 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 diff --git a/modules/file_storage/file_storage/pages/components/FileTable.tsx b/modules/file_storage/file_storage/pages/components/FileTable.tsx index 7faec28e2..fcdff3433 100644 --- a/modules/file_storage/file_storage/pages/components/FileTable.tsx +++ b/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" > - {t(keys.file_storage.actions.download)} + + {t(keys.file_storage.actions.download)} + {canPublish && (