fix: background_tasks: settings form accepts unbounded numeric values and gives no save feedback (#279)

* fix: background_tasks: settings form accepts unbounded numeric values and gives no save feedback (#270)

* fix: address review feedback for #270

* fix: address review feedback for #270

* fix: address review feedback for #270

Author: antosubash

biome.json

@@ -6,6 +6,7 @@
       "packages/**",
       "modules/*/*/pages/**",
       "modules/*/*/components/**",
+      "modules/*/tests-js/**",
       "!host/client_app/modules.generated.ts",
       "!host/client_app/modules.manifest.json",
       "!host/client_app/modules.generated.css",

docs/modules/background_tasks.md

@@ -148,8 +148,8 @@ DB-backed; defaults are in `BackgroundTasksSettings`. Several are marked `requir
 | `stuck_after_seconds` | `300` | heartbeat-staleness threshold |
 | `stuck_sweep_interval_seconds` | `60` | beat cadence for the stuck sweep |
 | `purge_interval_seconds` | `86_400` | beat cadence for old-row purge |
-| `retention_days` | `14` | how long to keep terminal rows |
-| `max_retries` | `3` | informational; tasks define their own policies |
+| `retention_days` | `14` | how long to keep terminal rows; range 1-3650 |
+| `max_retries` | `3` | informational; tasks define their own policies; range 0-100 |
 
 Bootstrap env-var equivalents (`SM_BG_TASKS_*`) only seed pydantic defaults at first boot — once a value lives in the DB, it's authoritative.
 

modules/background_tasks/background_tasks/settings.py

@@ -80,8 +80,18 @@ class BackgroundTasksSettings(BaseSettings):
     stuck_sweep_interval_seconds: int = DEFAULT_STUCK_SWEEP_INTERVAL_SECONDS
     purge_interval_seconds: int = DEFAULT_PURGE_INTERVAL_SECONDS
 
-    retention_days: int = DEFAULT_RETENTION_DAYS
-    max_retries: int = DEFAULT_MAX_RETRIES
+    retention_days: int = Field(
+        default=DEFAULT_RETENTION_DAYS,
+        ge=1,
+        le=3650,
+        description="Days to keep terminal task execution records (1-3650).",
+    )
+    max_retries: int = Field(
+        default=DEFAULT_MAX_RETRIES,
+        ge=0,
+        le=100,
+        description="Configured retry ceiling; individual tasks define their own policies (0-100).",
+    )
 
     @model_validator(mode="after")
     def _forbid_localhost_broker_in_production(self) -> BackgroundTasksSettings:

modules/background_tasks/tests/test_background_tasks_settings.py

@@ -0,0 +1,27 @@
+from __future__ import annotations
+
+import pytest
+from background_tasks.settings import BackgroundTasksSettings
+from pydantic import ValidationError
+
+
+@pytest.mark.parametrize("retention_days", [1, 3650])
+def test_retention_days_accepts_supported_boundaries(retention_days: int) -> None:
+    assert BackgroundTasksSettings(retention_days=retention_days).retention_days == retention_days
+
+
+@pytest.mark.parametrize("retention_days", [0, -5, 3651, 999_999_999])
+def test_retention_days_rejects_values_outside_supported_range(retention_days: int) -> None:
+    with pytest.raises(ValidationError):
+        BackgroundTasksSettings(retention_days=retention_days)
+
+
+@pytest.mark.parametrize("max_retries", [0, 100])
+def test_max_retries_accepts_supported_boundaries(max_retries: int) -> None:
+    assert BackgroundTasksSettings(max_retries=max_retries).max_retries == max_retries
+
+
+@pytest.mark.parametrize("max_retries", [-1, 101, 999_999_999])
+def test_max_retries_rejects_values_outside_supported_range(max_retries: int) -> None:
+    with pytest.raises(ValidationError):
+        BackgroundTasksSettings(max_retries=max_retries)

modules/settings/package.json

@@ -12,5 +12,7 @@
   "devDependencies": {
     "@simple-module-py/tsconfig": "*"
   },
-  "dependencies": {}
+  "dependencies": {
+    "sonner": "^2.0.7"
+  }
 }

modules/settings/settings/locales/en.json

@@ -91,6 +91,7 @@
   "modules_form": {
     "save": "Save",
     "saving": "Saving…",
+    "saved_toast": "Settings saved",
     "default_group": "General",
     "requires_restart": "Requires restart",
     "reset_to_default": "Reset to default",

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

@@ -38,23 +38,27 @@ export function ModuleForm({ module: m, testable = false }: Props) {
   }, [m.fields]);
 
   const [values, setValues] = useState<Record<string, unknown>>(initial);
+  const [baseline, setBaseline] = useState<Record<string, unknown>>(initial);
   const [errors, setErrors] = useState<Record<string, string>>({});
   const [busy, setBusy] = useState(false);
+  const [saved, setSaved] = useState(false);
 
   // Reset the edit buffer whenever the underlying module changes (package
   // switch, or server-reloaded props after a save/reset).
   useEffect(() => {
     setValues(initial);
+    setBaseline(initial);
     setErrors({});
+    setSaved(false);
   }, [initial]);
 
   const modifiedFields = useMemo(() => {
     const s = new Set<string>();
     for (const name of Object.keys(values)) {
-      if (notEqual(values[name], initial[name])) s.add(name);
+      if (notEqual(values[name], baseline[name])) s.add(name);
     }
     return s;
-  }, [values, initial]);
+  }, [values, baseline]);
 
   const defaultByName = useMemo(() => {
     const o: Record<string, unknown> = {};
@@ -76,6 +80,7 @@ export function ModuleForm({ module: m, testable = false }: Props) {
 
   async function onSave() {
     setBusy(true);
+    setSaved(false);
     setErrors({});
     const changed: Record<string, unknown> = {};
     for (const name of modifiedFields) changed[name] = values[name];
@@ -92,12 +97,16 @@ export function ModuleForm({ module: m, testable = false }: Props) {
       }
       setErrors(fieldErrs);
     } else if (resp.ok) {
-      router.reload({ only: ['modules'] });
+      // Keeping the form mounted lets the confirmation remain visible instead
+      // of being discarded by an immediate Inertia reload.
+      setBaseline({ ...values });
+      setSaved(true);
     }
     setBusy(false);
   }
 
   async function onReset(name: string) {
+    setSaved(false);
     await fetch(`/api/settings/modules/${m.package}/${name}`, { method: 'DELETE' });
     router.reload({ only: ['modules'] });
   }
@@ -109,7 +118,12 @@ export function ModuleForm({ module: m, testable = false }: Props) {
           <h2 className="text-xl font-semibold">{m.module_name}</h2>
           <p className="text-xs font-mono text-muted-foreground">{m.package}</p>
         </div>
-        <div className="flex items-start gap-2">
+        <div className="flex items-center gap-2">
+          {saved && (
+            <p role="status" className="text-sm font-medium text-emerald-700">
+              {t(keys.settings.modules_form.saved_toast)}
+            </p>
+          )}
           {testable && <TestConnectionButton pkg={m.package} />}
           <button
             type="button"
@@ -148,7 +162,10 @@ export function ModuleForm({ module: m, testable = false }: Props) {
                     id={`field-${m.package}-${f.name}`}
                     field={f}
                     value={values[f.name]}
-                    onChange={(name, v) => setValues((prev) => ({ ...prev, [name]: v }))}
+                    onChange={(name, v) => {
+                      setSaved(false);
+                      setValues((prev) => ({ ...prev, [name]: v }));
+                    }}
                   />
                   {isModified && (
                     <button

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

@@ -0,0 +1,63 @@
+import '@testing-library/jest-dom/vitest';
+import { configureI18n } from '@simple-module-py/i18n';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { afterEach, describe, expect, test, vi } from 'vitest';
+
+configureI18n({
+  locale: 'en',
+  messages: {
+    'settings.modules_form.saved_toast': 'Settings saved',
+  },
+});
+
+const mocks = vi.hoisted(() => ({ reload: vi.fn() }));
+
+vi.mock('@inertiajs/react', () => ({
+  router: { reload: mocks.reload },
+}));
+
+import { ModuleForm, type ModuleView } from '../settings/pages/components/ModuleForm';
+
+const moduleView: ModuleView = {
+  module_name: 'BackgroundTasks',
+  package: 'background_tasks',
+  env_prefix: 'SM_BG_TASKS_',
+  class_name: 'BackgroundTasksSettings',
+  fields: [
+    {
+      name: 'retention_days',
+      type: 'int',
+      value: 14,
+      default: 14,
+      description: '',
+      is_secret: false,
+      requires_restart: false,
+      group: null,
+      env_var: 'SM_BG_TASKS_RETENTION_DAYS',
+    },
+  ],
+};
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+  vi.clearAllMocks();
+});
+
+describe('ModuleForm', () => {
+  test('shows success feedback after settings are saved', async () => {
+    const fetchMock = vi.fn().mockResolvedValue({ ok: true, status: 200 });
+    vi.stubGlobal('fetch', fetchMock);
+    render(<ModuleForm module={moduleView} />);
+
+    fireEvent.change(screen.getByLabelText('retention_days'), { target: { value: '30' } });
+    fireEvent.click(screen.getByRole('button', { name: /modules_form\.save$/ }));
+
+    await waitFor(() => expect(screen.getByRole('status')).toBeVisible());
+    expect(fetchMock).toHaveBeenCalledWith(
+      '/api/settings/modules/background_tasks',
+      expect.objectContaining({ body: JSON.stringify({ retention_days: 30 }) }),
+    );
+    expect(mocks.reload).not.toHaveBeenCalled();
+    expect(screen.getByRole('status')).toHaveTextContent('Settings saved');
+  });
+});

modules/settings/tests/test_module_api.py

@@ -32,6 +32,28 @@ async def test_put_validation_error_surfaces_422(authenticated_client, app):
     assert "i18n_default_locale" in resp.text
 
 
+@pytest.mark.asyncio
+async def test_put_rejects_and_does_not_persist_unbounded_background_task_values(
+    authenticated_client, app, db_session
+):
+    from settings.service import SettingService
+    from settings.store import SettingsStore
+
+    original = app.state.background_tasks.settings
+    resp = await authenticated_client.put(
+        "/api/settings/modules/background_tasks",
+        json={"retention_days": -5, "max_retries": 999_999_999},
+    )
+
+    assert resp.status_code == 422
+    assert {error["loc"][-1] for error in resp.json()["detail"]} == {
+        "retention_days",
+        "max_retries",
+    }
+    assert app.state.background_tasks.settings is original
+    assert await SettingsStore(SettingService(db_session)).get_overrides("background_tasks") == {}
+
+
 @pytest.mark.asyncio
 async def test_delete_field_resets_to_default(authenticated_client, app):
     await authenticated_client.put("/api/settings/modules/host", json={"multi_tenant": True})

modules/settings/tsconfig.json

@@ -6,5 +6,5 @@
       "@simple-module-py/ui/*": ["../../packages/ui/src/*"]
     }
   },
-  "include": ["settings/**/*.ts", "settings/**/*.tsx"]
+  "include": ["settings/**/*.ts", "settings/**/*.tsx", "tests-js/**/*.ts", "tests-js/**/*.tsx"]
 }