From 37edfc42333c2838a312477ce56bf207bad3fcd8 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 27 Aug 2026 10:50:19 +0200 Subject: [PATCH 01/13] feat(admin): complete the admin sidebar, kill double-edits, wire Doctor to real data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three duplications and a fake panel, all in the admin section: - Branding and Feature Flags registered into the main app sidebar while their pages live under /admin in AdminLayout — the app sidebar grew orphan "Appearance"/"System" groups (one floating above Dashboard) and the admin sidebar/index were missing both screens. They now register into ADMIN_SIDEBAR; branding gets order=105 so groups read Access → Appearance → System. - Branding was editable in two places: its own page and the generic Settings → Modules editor. register_module_settings() gains an optional manage_url; a module that declares one renders as a "Managed on its own page" link card in the generic editor instead of a second, raw editor for the same fields. Branding declares it. - The Branding page repeated its own title and description inside the form card, 40px under the identical page header. The card now starts at the first field. - The Doctor page was hardcoded fiction (doctor-data.ts: a fake billing module, migrations that never existed, "Vite :5173" when dev runs on 5050). It now runs the real diagnostics engine per request, reads the boot-time migration state plus recent alembic revisions, and reports live environment facts. The fake dev-server and env-vars panels are gone; the stat row is errors/warnings/modules/health, all real. Fixing Doctor surfaced a false SM003 against audit_log: the render-call resolver skipped annotated assignments (NAME: Final = ...) and f-string constants, so PAGE_BROWSE = f"{MODULE_NAME}/Browse" never resolved. The resolver now handles both shapes and refuses genuinely dynamic values. Polish alongside: Feature Flags' near-empty scope card collapses into one toolbar row; Background Tasks table headers adopt the uppercase style every other admin table uses; "1 fields set" in Audit Log gets CLDR plural forms. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui --- .../simple_module_core/diagnostics/_pages.py | 67 +++++- .../core/tests/test_module_diagnostics.py | 36 --- .../core/tests/test_sm003_page_render.py | 94 ++++++++ modules/audit_log/audit_log/locales/en.json | 4 +- .../background_tasks/pages/Index.tsx | 19 +- modules/branding/branding/module.py | 9 +- modules/branding/branding/pages/Manage.tsx | 16 +- modules/dashboard/dashboard/doctor.py | 112 +++++++++ .../dashboard/dashboard/endpoints/views.py | 7 +- modules/dashboard/dashboard/locales/en.json | 27 ++- modules/dashboard/dashboard/pages/Doctor.tsx | 212 +++++------------- .../pages/components/DemoPlaceholders.tsx | 2 +- .../pages/components/DiagnosticsCard.tsx | 83 +++++++ .../pages/components/MigrationsCard.tsx | 55 +++++ .../dashboard/pages/components/doctor-data.ts | 68 ------ modules/dashboard/tests/test_view_routes.py | 29 +++ modules/feature_flags/feature_flags/module.py | 3 +- .../feature_flags/pages/Browse.tsx | 31 +-- modules/settings/settings/_module_settings.py | 60 ++--- .../settings/_module_settings_props.py | 52 +++++ .../settings/settings/endpoints/module_api.py | 2 +- modules/settings/settings/endpoints/views.py | 2 +- modules/settings/settings/locales/en.json | 5 +- modules/settings/settings/module_registry.py | 21 +- .../settings/settings/pages/ModulesEdit.tsx | 29 ++- .../settings/pages/components/ModuleForm.tsx | 2 + modules/settings/settings/registration.py | 10 +- .../tests/test_module_settings_render.py | 19 ++ .../tests/test_module_settings_serialize.py | 2 +- packages/i18n/src/generated-resources.ts | 28 ++- packages/i18n/src/keys.generated.ts | 28 ++- 31 files changed, 735 insertions(+), 399 deletions(-) create mode 100644 framework/core/tests/test_sm003_page_render.py create mode 100644 modules/dashboard/dashboard/doctor.py create mode 100644 modules/dashboard/dashboard/pages/components/DiagnosticsCard.tsx create mode 100644 modules/dashboard/dashboard/pages/components/MigrationsCard.tsx delete mode 100644 modules/dashboard/dashboard/pages/components/doctor-data.ts create mode 100644 modules/settings/settings/_module_settings_props.py diff --git a/framework/core/simple_module_core/diagnostics/_pages.py b/framework/core/simple_module_core/diagnostics/_pages.py index 2023b94c..d3358ea8 100644 --- a/framework/core/simple_module_core/diagnostics/_pages.py +++ b/framework/core/simple_module_core/diagnostics/_pages.py @@ -12,17 +12,62 @@ from simple_module_core.module import ModuleBase +def _assignment(s: ast.stmt) -> tuple[ast.Name, ast.expr] | None: + """Return ``(target, value)`` for a single-target top-level assignment. + + Covers both ``NAME = ...`` and the annotated ``NAME: Final = ...`` form + module constants are conventionally written in. + """ + if isinstance(s, ast.Assign) and len(s.targets) == 1 and isinstance(s.targets[0], ast.Name): + return s.targets[0], s.value + if isinstance(s, ast.AnnAssign) and isinstance(s.target, ast.Name) and s.value is not None: + return s.target, s.value + return None + + def _module_level_str_consts(tree: ast.Module) -> dict[str, str]: """Return ``{name: literal}`` for top-level ``NAME = "string"`` assignments.""" - return { - s.targets[0].id: s.value.value - for s in tree.body - if isinstance(s, ast.Assign) - and len(s.targets) == 1 - and isinstance(s.targets[0], ast.Name) - and isinstance(s.value, ast.Constant) - and isinstance(s.value.value, str) - } + consts: dict[str, str] = {} + for s in tree.body: + assignment = _assignment(s) + if assignment is None: + continue + target, value = assignment + if isinstance(value, ast.Constant) and isinstance(value.value, str): + consts[target.id] = value.value + return consts + + +def _resolve_fstring_consts(tree: ast.Module, consts: dict[str, str]) -> dict[str, str]: + """Resolve top-level ``NAME = f"{CONST}/lit"`` against already-known consts. + + Only plain interpolations of known string constants count — a conversion, + format spec, or unknown name makes the value non-static, so it is skipped + rather than guessed at. + """ + resolved: dict[str, str] = {} + for s in tree.body: + assignment = _assignment(s) + if assignment is None or not isinstance(assignment[1], ast.JoinedStr): + continue + target, value = assignment + parts: list[str] = [] + for piece in value.values: + if isinstance(piece, ast.Constant) and isinstance(piece.value, str): + parts.append(piece.value) + elif ( + isinstance(piece, ast.FormattedValue) + and isinstance(piece.value, ast.Name) + and piece.value.id in consts + and piece.conversion == -1 + and piece.format_spec is None + ): + parts.append(consts[piece.value.id]) + else: + break + else: + resolved[target.id] = "".join(parts) + return resolved def _iter_render_components(tree: ast.Module, consts: dict[str, str]) -> list[str]: @@ -79,6 +124,10 @@ def find_render_calls(mod: ModuleBase, src_dir: Path) -> set[str]: consts: dict[str, str] = {} for tree in trees: consts.update(_module_level_str_consts(tree)) + # Second pass: f-string constants built from the plain ones above + # (``PAGE = f"{MODULE_NAME}/Browse"`` is the conventional shape). + for tree in trees: + consts.update(_resolve_fstring_consts(tree, consts)) prefix = f"{mod.meta.name}/" rendered: set[str] = set() diff --git a/framework/core/tests/test_module_diagnostics.py b/framework/core/tests/test_module_diagnostics.py index 6b103d77..c84e62eb 100644 --- a/framework/core/tests/test_module_diagnostics.py +++ b/framework/core/tests/test_module_diagnostics.py @@ -33,42 +33,6 @@ def _mk_module_tree(root: Path, name: str, *, with_pkg_json: bool, with_tsconfig return src_dir -class TestSm003PageRenderResolution: - """SM003 must resolve PAGE_X constants imported from sibling files.""" - - def _diags(self, src_dir: Path, mod_name: str): - from simple_module_core.diagnostics._pages import check_pages, find_render_calls - - mod = _FakeModule(meta=_FakeMeta(name=mod_name)) - rendered = find_render_calls(mod, src_dir) # pyright: ignore[reportArgumentType] - return [d for d in check_pages(mod, src_dir, rendered) if d.code == "SM003"] # pyright: ignore[reportArgumentType] - - async def test_resolves_constant_imported_from_sibling_file(self, tmp_path: Path): - src_dir = tmp_path / "feature_flags" / "feature_flags" - (src_dir / "pages").mkdir(parents=True) - (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}") - (src_dir / "constants.py").write_text('PAGE_BROWSE = "FeatureFlags/Browse"\n') - endpoints = src_dir / "endpoints" - endpoints.mkdir() - (endpoints / "views.py").write_text( - "from feature_flags.constants import PAGE_BROWSE\n" - "async def view(inertia):\n" - " return await inertia.render(PAGE_BROWSE, {})\n" - ) - assert self._diags(src_dir, "FeatureFlags") == [] - - async def test_still_flags_truly_orphan_pages(self, tmp_path: Path): - src_dir = tmp_path / "m" / "m" - (src_dir / "pages").mkdir(parents=True) - (src_dir / "pages" / "Ghost.tsx").write_text("export default function G() {}") - (src_dir / "endpoints.py").write_text( - 'async def view(inertia):\n return await inertia.render("M/Other", {})\n' - ) - results = self._diags(src_dir, "M") - assert [r.code for r in results] == ["SM003"] - assert "Ghost.tsx" in results[0].message - - class TestSm017JsWorkspaceFiles: async def test_fires_when_both_missing(self, tmp_path: Path): src_dir = _mk_module_tree(tmp_path, "orders", with_pkg_json=False, with_tsconfig=False) diff --git a/framework/core/tests/test_sm003_page_render.py b/framework/core/tests/test_sm003_page_render.py new file mode 100644 index 00000000..dc9b5c26 --- /dev/null +++ b/framework/core/tests/test_sm003_page_render.py @@ -0,0 +1,94 @@ +"""SM003 page-render resolution tests, split from test_module_diagnostics. + +The resolver reads inertia.render() targets out of module source; these tests +cover the constant shapes modules actually use (plain, imported, annotated +f-string) and the dynamic shapes it must refuse to guess at. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + + +@dataclass +class _FakeMeta: + name: str + + +@dataclass +class _FakeModule: + meta: _FakeMeta + + +class TestSm003PageRenderResolution: + """SM003 must resolve PAGE_X constants imported from sibling files.""" + + def _diags(self, src_dir: Path, mod_name: str): + from simple_module_core.diagnostics._pages import check_pages, find_render_calls + + mod = _FakeModule(meta=_FakeMeta(name=mod_name)) + rendered = find_render_calls(mod, src_dir) # pyright: ignore[reportArgumentType] + return [d for d in check_pages(mod, src_dir, rendered) if d.code == "SM003"] # pyright: ignore[reportArgumentType] + + async def test_resolves_constant_imported_from_sibling_file(self, tmp_path: Path): + src_dir = tmp_path / "feature_flags" / "feature_flags" + (src_dir / "pages").mkdir(parents=True) + (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}") + (src_dir / "constants.py").write_text('PAGE_BROWSE = "FeatureFlags/Browse"\n') + endpoints = src_dir / "endpoints" + endpoints.mkdir() + (endpoints / "views.py").write_text( + "from feature_flags.constants import PAGE_BROWSE\n" + "async def view(inertia):\n" + " return await inertia.render(PAGE_BROWSE, {})\n" + ) + assert self._diags(src_dir, "FeatureFlags") == [] + + async def test_still_flags_truly_orphan_pages(self, tmp_path: Path): + src_dir = tmp_path / "m" / "m" + (src_dir / "pages").mkdir(parents=True) + (src_dir / "pages" / "Ghost.tsx").write_text("export default function G() {}") + (src_dir / "endpoints.py").write_text( + 'async def view(inertia):\n return await inertia.render("M/Other", {})\n' + ) + results = self._diags(src_dir, "M") + assert [r.code for r in results] == ["SM003"] + assert "Ghost.tsx" in results[0].message + + async def test_resolves_annotated_fstring_constant(self, tmp_path: Path): + """The conventional shape: ``PAGE: Final = f"{MODULE_NAME}/Browse"``. + + Regression test for a false SM003 against audit_log — the resolver + skipped both annotated assignments and f-strings, so every module + writing its page name this way was flagged as an orphan. + """ + src_dir = tmp_path / "audit_log" / "audit_log" + (src_dir / "pages").mkdir(parents=True) + (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}") + (src_dir / "constants.py").write_text( + "from typing import Final\n" + 'MODULE_NAME: Final = "AuditLog"\n' + 'PAGE_BROWSE: Final = f"{MODULE_NAME}/Browse"\n' + ) + endpoints = src_dir / "endpoints" + endpoints.mkdir() + (endpoints / "views.py").write_text( + "from audit_log.constants import PAGE_BROWSE\n" + "async def view(inertia):\n" + " return await inertia.render(PAGE_BROWSE, {})\n" + ) + assert self._diags(src_dir, "AuditLog") == [] + + async def test_fstring_with_unknown_name_stays_flagged(self, tmp_path: Path): + """An f-string over a runtime value is not static — don't guess.""" + src_dir = tmp_path / "m" / "m" + (src_dir / "pages").mkdir(parents=True) + (src_dir / "pages" / "Browse.tsx").write_text("export default function B() {}") + (src_dir / "endpoints.py").write_text( + 'PAGE = f"{dynamic()}/Browse"\n' + "async def view(inertia):\n" + " return await inertia.render(PAGE, {})\n" + ) + results = self._diags(src_dir, "M") + assert [r.code for r in results] == ["SM003"] diff --git a/modules/audit_log/audit_log/locales/en.json b/modules/audit_log/audit_log/locales/en.json index 629536e0..5e4894f8 100644 --- a/modules/audit_log/audit_log/locales/en.json +++ b/modules/audit_log/audit_log/locales/en.json @@ -50,7 +50,9 @@ "show_less": "Show less", "system_user": "System", "no_changes": "—", - "unresolved_user": "No matching account for this id" + "unresolved_user": "No matching account for this id", + "fields_set_one": "{count} field set", + "fields_set_other": "{count} fields set" }, "nav": { "audit_log": "Audit Log" diff --git a/modules/background_tasks/background_tasks/pages/Index.tsx b/modules/background_tasks/background_tasks/pages/Index.tsx index 056b0525..ffa90124 100644 --- a/modules/background_tasks/background_tasks/pages/Index.tsx +++ b/modules/background_tasks/background_tasks/pages/Index.tsx @@ -47,6 +47,9 @@ interface Props { /** Task, Status, Queue, Queued, Duration, Worker, Actions. */ const COLUMN_COUNT = 7; +// Same header treatment as the other admin tables (users, audit log, flags). +const TH = 'text-[11px] font-semibold uppercase tracking-[0.08em] text-muted-foreground'; + const STATUS_ALL = '__all__'; function pushFilters(filters: { status: string; task_name: string }, page: number): void { @@ -181,23 +184,23 @@ function Index() { - + - {t(keys.background_tasks.table.task)} - {t(keys.background_tasks.table.status)} - + {t(keys.background_tasks.table.task)} + {t(keys.background_tasks.table.status)} + - + - + - + - + {t(keys.background_tasks.table.actions)} diff --git a/modules/branding/branding/module.py b/modules/branding/branding/module.py index e92693be..4a0d1477 100644 --- a/modules/branding/branding/module.py +++ b/modules/branding/branding/module.py @@ -11,7 +11,7 @@ from pathlib import Path from fastapi import APIRouter, FastAPI -from simple_module_core.menu import MenuItem, MenuRegistry +from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry from simple_module_core.public_routes import PublicRouteRegistry @@ -42,6 +42,9 @@ def register_settings(self, app: FastAPI) -> None: constants.PACKAGE, BrandingSettings, lambda s: BrandingServices(settings=s), + # Branding ships its own management page; the generic module-settings + # editor links there instead of double-editing the same fields. + manage_url=MENU_URL, ) def register_permissions(self, registry: PermissionRegistry) -> None: @@ -68,6 +71,10 @@ def register_menu_items(self, registry: MenuRegistry) -> None: label_key="branding.nav.branding", url=MENU_URL, icon="palette", + # Between Access (100) and System (110) so the admin sidebar + # reads Access → Appearance → System. + order=105, + section=MenuSection.ADMIN_SIDEBAR, group="Appearance", group_key="ui.nav_groups.appearance", roles=["admin"], diff --git a/modules/branding/branding/pages/Manage.tsx b/modules/branding/branding/pages/Manage.tsx index 359f9510..82530043 100644 --- a/modules/branding/branding/pages/Manage.tsx +++ b/modules/branding/branding/pages/Manage.tsx @@ -2,13 +2,7 @@ import { Head, router, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Button } from '@simple-module-py/ui/components/ui/button'; -import { - Card, - CardContent, - CardDescription, - CardHeader, - CardTitle, -} from '@simple-module-py/ui/components/ui/card'; +import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; import { Label } from '@simple-module-py/ui/components/ui/label'; import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; @@ -136,12 +130,10 @@ function Manage() { description={t(keys.branding.manage.description)} >
+ {/* The PageShell above already carries the title + description; + repeating them inside the card read as a glitch. */} - - {t(keys.branding.manage.title)} - {t(keys.branding.manage.description)} - - +
str | None: + """Trim the working directory off absolute finding paths for display.""" + if not file: + return file + try: + return str(Path(file).relative_to(Path.cwd())) + except ValueError: + return file + + +_ALEMBIC_INI = "host/alembic.ini" +_RECENT_LIMIT = 5 + + +def collect_diagnostics(app: FastAPI) -> list[dict[str, Any]]: + """Run the module + i18n diagnostics and serialize the findings. + + Mirrors the dev-boot run in ``app_builder.create_app`` (minus the host/ui + locale extras, which only the builder can see). Sorted errors-first so the + screen leads with what needs fixing. + """ + sm = app.state.sm + diagnostics = run_diagnostics( + list(sm.modules), + i18n_supported_locales=sm.settings.i18n_supported_locales, + i18n_default_locale=sm.settings.i18n_default_locale, + ) + order = {"error": 0, "warning": 1, "info": 2} + diagnostics.sort(key=lambda d: (order.get(d.level.value, 3), d.code)) + return [ + { + "level": d.level.value, + "code": d.code, + "message": d.message, + "module": d.module_name, + "file": _relative(d.file), + "suggestion": d.suggestion, + } + for d in diagnostics + ] + + +def migration_overview(app: FastAPI) -> dict[str, Any]: + """Migration state from the boot check, plus the most recent revisions. + + ``app.state.migration`` exists because the lifespan refuses to start a + behind-head app, so a running app is at head by construction — the value + of this panel is showing *which* head, and what recently changed. + """ + state = getattr(app.state, "migration", None) or {} + return { + "current_revision": state.get("current_revision"), + "head_revision": state.get("head_revision"), + "is_current": state.get("is_current", True), + "recent": _recent_revisions(), + } + + +def _recent_revisions(limit: int = _RECENT_LIMIT) -> list[dict[str, Any]]: + """Newest ``limit`` alembic revisions (head first), or ``[]`` when the + script directory isn't present (e.g. a deployment without host/).""" + try: + from alembic.config import Config as AlembicConfig + from alembic.script import ScriptDirectory + + script = ScriptDirectory.from_config(AlembicConfig(_ALEMBIC_INI)) + revisions = [] + for rev in script.walk_revisions(): + revisions.append( + { + "revision": rev.revision[:12], + "message": rev.doc or "", + "modules": sorted(rev.branch_labels or ()), + } + ) + if len(revisions) >= limit: + break + return revisions + except Exception as exc: # pragma: no cover - depends on deploy layout + logger.debug("Alembic script directory unavailable: %s", exc) + return [] + + +def environment_info(app: FastAPI) -> dict[str, Any]: + """Live environment facts: mode, database backend, locales.""" + sm = app.state.sm + return { + "environment": sm.settings.environment, + "database": sm.db.engine.dialect.name, + "locales": list(sm.settings.i18n_supported_locales), + "default_locale": sm.settings.i18n_default_locale, + } diff --git a/modules/dashboard/dashboard/endpoints/views.py b/modules/dashboard/dashboard/endpoints/views.py index 1c72319d..747fd050 100644 --- a/modules/dashboard/dashboard/endpoints/views.py +++ b/modules/dashboard/dashboard/endpoints/views.py @@ -60,12 +60,17 @@ async def doctor( inertia: InertiaDep, db: AsyncSession = Depends(get_db), ) -> InertiaResponse: - """`make doctor` mirror — static checks, modules, dev server, env.""" + """`make doctor` mirror — live diagnostics, migrations, modules, env.""" + from dashboard.doctor import collect_diagnostics, environment_info, migration_overview + stats = await fetch_dashboard_stats(db, request.app) return await inertia.render( _PAGE_DOCTOR, { "module_count": stats["module_count"], "system_info": stats["system_info"], + "diagnostics": collect_diagnostics(request.app), + "migration": migration_overview(request.app), + "environment": environment_info(request.app), }, ) diff --git a/modules/dashboard/dashboard/locales/en.json b/modules/dashboard/dashboard/locales/en.json index cc9ccae2..7a5660a6 100644 --- a/modules/dashboard/dashboard/locales/en.json +++ b/modules/dashboard/dashboard/locales/en.json @@ -31,30 +31,33 @@ }, "doctor": { "title": "Doctor", - "description": "Static checks, migrations, dev server, and module health. Mirrors `make doctor` output.", + "description": "Live module diagnostics, migration state and environment. Mirrors `make doctor`.", "rerun": "Re-run", - "stat_checks_passed": "Checks passed", "stat_modules": "Modules", - "stat_pending_migrations": "Pending mig.", "stat_health": "Health", "ok": "OK", "review": "review", - "clean": "clean", "alert": "{count} alert", - "static_checks": "Static checks", - "just_now": "just now", "recent_migrations": "Recent migrations", - "generate": "Generate", - "apply": "Apply", "applied": "applied", - "pending": "pending", "installed_modules": "Installed modules", "loaded": "loaded", "active": "active", - "dev_server": "Dev server", - "running": "running", "run_command": "Run a command", - "environment": "Environment" + "environment": "Environment", + "stat_errors": "Errors", + "stat_warnings": "Warnings", + "diagnostics": "Diagnostics", + "all_clear_title": "All checks pass", + "all_clear_hint": "No findings from the module, i18n and migration checks.", + "at_head": "DB at head", + "behind": "behind", + "suggestion": "Suggestion", + "env_mode": "Mode", + "env_database": "Database", + "env_python": "Python", + "env_locales": "Locales", + "env_revision": "Revision" }, "nav": { "dashboard": "Dashboard", diff --git a/modules/dashboard/dashboard/pages/Doctor.tsx b/modules/dashboard/dashboard/pages/Doctor.tsx index b18fedfc..db2af827 100644 --- a/modules/dashboard/dashboard/pages/Doctor.tsx +++ b/modules/dashboard/dashboard/pages/Doctor.tsx @@ -1,4 +1,4 @@ -import { Head, usePage } from '@inertiajs/react'; +import { Head, router, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; @@ -7,20 +7,11 @@ import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; -import { - Activity, - AlertTriangle, - CheckCircle2, - Database, - GitBranch, - Package, - Play, - RefreshCw, - Stethoscope, - Terminal, - XCircle, -} from 'lucide-react'; -import { DEV_SERVER, ENV_VARS, MIGRATIONS, STATIC_CHECKS, TONE } from './components/doctor-data'; +import { TONE } from '@simple-module-py/ui/lib/tone'; +import { Activity, AlertTriangle, Package, RefreshCw, Stethoscope, XCircle } from 'lucide-react'; +import type React from 'react'; +import { type Diagnostic, DiagnosticsCard } from './components/DiagnosticsCard'; +import { type Migration, MigrationsCard } from './components/MigrationsCard'; interface SystemModule { name: string; @@ -32,52 +23,44 @@ interface HealthCheck { status: 'healthy' | 'degraded' | 'unhealthy'; } +interface Environment { + environment: string; + database: string; + locales: string[]; + default_locale: string; +} + interface Props { - total_users: number; - active_users_7d: number; module_count: number; system_info: { modules: SystemModule[]; python_version: string; health_checks: HealthCheck[]; }; -} - -const STATUS_VISUALS = { - pass: { Icon: CheckCircle2, color: 'text-primary-600', tone: TONE.success }, - warn: { Icon: AlertTriangle, color: 'text-amber-600', tone: TONE.warning }, - fail: { Icon: XCircle, color: 'text-red-600', tone: TONE.destructive }, -} as const; - -function CheckRow({ check }: { check: (typeof STATIC_CHECKS)[number] }) { - const { Icon, color, tone } = STATUS_VISUALS[check.status]; - return ( -
-
- ); + diagnostics: Diagnostic[]; + migration: Migration; + environment: Environment; } function Doctor() { - const { system_info, module_count } = usePage<{ props: Props }>().props as unknown as Props; + const props = usePage<{ props: Props }>().props as unknown as Props; + const { system_info, module_count, diagnostics, migration, environment } = props; const { t } = useT(); - const passed = STATIC_CHECKS.filter((c) => c.status === 'pass').length; - const pending = MIGRATIONS.filter((m) => !m.applied).length; + const errors = diagnostics.filter((d) => d.level === 'error').length; + const warnings = diagnostics.filter((d) => d.level === 'warning').length; const unhealthy = system_info.health_checks.filter((c) => c.status !== 'healthy').length; + const envRows: [string, string][] = [ + [t(keys.dashboard.doctor.env_mode), environment.environment], + [t(keys.dashboard.doctor.env_database), environment.database], + [t(keys.dashboard.doctor.env_python), system_info.python_version], + [t(keys.dashboard.doctor.env_locales), environment.locales.join(', ')], + ]; + if (migration.head_revision) { + envRows.push([t(keys.dashboard.doctor.env_revision), migration.head_revision.slice(0, 12)]); + } + return ( <> @@ -85,40 +68,31 @@ function Doctor() { title={t(keys.dashboard.doctor.title)} description={t(keys.dashboard.doctor.description)} actions={ - <> - - - + } >
+ -
- - - - {t(keys.dashboard.doctor.just_now)} - - } - > - {t(keys.dashboard.doctor.static_checks)} - -
- {STATIC_CHECKS.map((c) => ( - - ))} -
- - - - - - - - -
- } - > - {t(keys.dashboard.doctor.recent_migrations)} - -
- {MIGRATIONS.map((m) => ( -
- - {m.id} - - - {m.module} - -
{m.msg}
- {m.when} - - {m.applied - ? t(keys.dashboard.doctor.applied) - : t(keys.dashboard.doctor.pending)} - -
- ))} -
- - + + @@ -226,24 +143,18 @@ function Doctor() {
- - - {t(keys.dashboard.doctor.running)} - - } - > - {t(keys.dashboard.doctor.dev_server)} + +
- {DEV_SERVER.map(([k, v, tone]) => ( + {envRows.map(([k, v]) => (
{k} - + {v}
@@ -267,25 +178,6 @@ function Doctor() {
- - - - - -
- {ENV_VARS.map(([k, v]) => ( -
- {k} - - {v} - -
- ))} -
- -
diff --git a/modules/dashboard/dashboard/pages/components/DemoPlaceholders.tsx b/modules/dashboard/dashboard/pages/components/DemoPlaceholders.tsx index edea57fb..a588ba6b 100644 --- a/modules/dashboard/dashboard/pages/components/DemoPlaceholders.tsx +++ b/modules/dashboard/dashboard/pages/components/DemoPlaceholders.tsx @@ -6,6 +6,7 @@ import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; import { Badge } from '@simple-module-py/ui/components/ui/badge'; import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; +import { TONE } from '@simple-module-py/ui/lib/tone'; import { Box, ChevronRight, @@ -15,7 +16,6 @@ import { ShoppingCart, Users, } from 'lucide-react'; -import { TONE } from './doctor-data'; type Tone = keyof typeof TONE; diff --git a/modules/dashboard/dashboard/pages/components/DiagnosticsCard.tsx b/modules/dashboard/dashboard/pages/components/DiagnosticsCard.tsx new file mode 100644 index 00000000..eb8a1810 --- /dev/null +++ b/modules/dashboard/dashboard/pages/components/DiagnosticsCard.tsx @@ -0,0 +1,83 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; +import { Badge } from '@simple-module-py/ui/components/ui/badge'; +import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; +import { TONE } from '@simple-module-py/ui/lib/tone'; +import { AlertTriangle, CheckCircle2, Info, XCircle } from 'lucide-react'; + +export interface Diagnostic { + level: 'error' | 'warning' | 'info'; + code: string; + message: string; + module: string; + file: string | null; + suggestion: string | null; +} + +const LEVEL_VISUALS = { + error: { Icon: XCircle, color: 'text-red-600', tone: TONE.destructive }, + warning: { Icon: AlertTriangle, color: 'text-amber-600', tone: TONE.warning }, + info: { Icon: Info, color: 'text-muted-foreground', tone: TONE.default }, +} as const; + +function DiagnosticRow({ d, suggestionLabel }: { d: Diagnostic; suggestionLabel: string }) { + const { Icon, color, tone } = LEVEL_VISUALS[d.level]; + return ( +
+
+ ); +} + +export function DiagnosticsCard({ diagnostics }: { diagnostics: Diagnostic[] }) { + const { t } = useT(); + return ( + + + {t(keys.dashboard.doctor.diagnostics)} + {diagnostics.length === 0 ? ( +
+
+ ) : ( +
+ {diagnostics.map((d) => ( + + ))} +
+ )} + + + ); +} diff --git a/modules/dashboard/dashboard/pages/components/MigrationsCard.tsx b/modules/dashboard/dashboard/pages/components/MigrationsCard.tsx new file mode 100644 index 00000000..c9339a7d --- /dev/null +++ b/modules/dashboard/dashboard/pages/components/MigrationsCard.tsx @@ -0,0 +1,55 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { SectionTitle } from '@simple-module-py/ui/components/SectionTitle'; +import { Badge } from '@simple-module-py/ui/components/ui/badge'; +import { Card, CardContent } from '@simple-module-py/ui/components/ui/card'; +import { TONE } from '@simple-module-py/ui/lib/tone'; + +export interface Migration { + current_revision: string | null; + head_revision: string | null; + is_current: boolean; + recent: { revision: string; message: string; modules: string[] }[]; +} + +export function MigrationsCard({ migration }: { migration: Migration }) { + const { t } = useT(); + if (migration.recent.length === 0) return null; + return ( + + + + {migration.is_current + ? t(keys.dashboard.doctor.at_head) + : t(keys.dashboard.doctor.behind)} + + } + > + {t(keys.dashboard.doctor.recent_migrations)} + +
+ {migration.recent.map((m) => ( +
+ + {m.revision} + + {m.modules.map((mod) => ( + + {mod} + + ))} +
{m.message}
+ + {t(keys.dashboard.doctor.applied)} + +
+ ))} +
+ + + ); +} diff --git a/modules/dashboard/dashboard/pages/components/doctor-data.ts b/modules/dashboard/dashboard/pages/components/doctor-data.ts deleted file mode 100644 index 4ad83348..00000000 --- a/modules/dashboard/dashboard/pages/components/doctor-data.ts +++ /dev/null @@ -1,68 +0,0 @@ -export const STATIC_CHECKS = [ - { name: 'Module imports', status: 'pass' as const, hint: 'All ModuleBase subclasses load.' }, - { name: 'Migration drift', status: 'pass' as const, hint: 'Alembic head matches DB.' }, - { name: 'Orphan pages', status: 'pass' as const, hint: 'Every Inertia page has a route.' }, - { - name: 'Permission registry', - status: 'pass' as const, - hint: 'All declared perms reachable from a role.', - }, - { - name: 'Coupling check', - status: 'warn' as const, - hint: 'Cross-module imports detected — modules should depend via the registry.', - file: 'modules/billing/router.py:14', - }, - { - name: 'Schema isolation', - status: 'pass' as const, - hint: 'No cross-schema foreign keys on Postgres.', - }, -]; - -export const MIGRATIONS = [ - { - id: '0024', - module: 'billing', - msg: 'create subscriptions table', - when: 'just now', - applied: false, - }, - { - id: '0023', - module: 'orders', - msg: 'add fulfilled_at column', - when: '3h ago', - applied: true, - }, - { - id: '0022', - module: 'users', - msg: 'add invited_by foreign key', - when: '1d ago', - applied: true, - }, - { - id: '0021', - module: 'audit', - msg: 'partition events by month', - when: '2d ago', - applied: true, - }, -]; - -export const ENV_VARS: [string, string][] = [ - ['SM_ENVIRONMENT', 'development'], - ['SM_DATABASE_URL', 'sqlite+aiosqlite'], - ['SM_USERS_MAILER', 'console'], - ['SM_USERS_ALLOW_SIGNUP', 'false'], -]; - -export const DEV_SERVER: [string, string, 'success' | 'default'][] = [ - ['FastAPI', ':8000', 'success'], - ['Vite HMR', ':5173', 'success'], - ['Postgres', ':5432', 'success'], - ['Worker', 'idle', 'default'], -]; - -export { TONE } from '@simple-module-py/ui/lib/tone'; diff --git a/modules/dashboard/tests/test_view_routes.py b/modules/dashboard/tests/test_view_routes.py index fd8c979c..969ab53a 100644 --- a/modules/dashboard/tests/test_view_routes.py +++ b/modules/dashboard/tests/test_view_routes.py @@ -94,3 +94,32 @@ async def test_dashboard_index_redirects_anon_to_login(client): resp = await client.get("/dashboard/", follow_redirects=False) assert resp.status_code == 302 assert "/users/login" in resp.headers["location"] + + +@pytest.mark.anyio +async def test_doctor_reports_real_diagnostics(authenticated_client): + """The doctor page ships live diagnostics, migration state and env facts. + + Guards against the panel regressing to hardcoded demo data: the values + asserted here can only come from the running app. + """ + resp = await authenticated_client.get( + "/admin/doctor/", + headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, + ) + assert resp.status_code == 200, resp.text + props = resp.json()["props"] + + assert isinstance(props["diagnostics"], list) + for finding in props["diagnostics"]: + assert finding["code"].startswith("SM") + assert finding["level"] in {"error", "warning", "info"} + + migration = props["migration"] + # The test fixtures stamp alembic at head, so the page must agree. + assert migration["is_current"] is True + assert migration["current_revision"] == migration["head_revision"] + + env = props["environment"] + assert env["database"] == "sqlite" + assert env["default_locale"] in env["locales"] diff --git a/modules/feature_flags/feature_flags/module.py b/modules/feature_flags/feature_flags/module.py index ae3a85dd..ffe5d3c5 100644 --- a/modules/feature_flags/feature_flags/module.py +++ b/modules/feature_flags/feature_flags/module.py @@ -7,7 +7,7 @@ from pathlib import Path from fastapi import APIRouter, FastAPI -from simple_module_core.menu import MenuItem, MenuRegistry +from simple_module_core.menu import MenuItem, MenuRegistry, MenuSection from simple_module_core.module import ModuleBase, ModuleMeta from simple_module_core.permissions import PermissionRegistry @@ -48,6 +48,7 @@ def register_menu_items(self, registry: MenuRegistry) -> None: url=MENU_URL, icon=MENU_ICON, order=MENU_ORDER, + section=MenuSection.ADMIN_SIDEBAR, group="System", group_key="ui.nav_groups.system", # Mirrors the view router's guard. Without it the entry shows diff --git a/modules/feature_flags/feature_flags/pages/Browse.tsx b/modules/feature_flags/feature_flags/pages/Browse.tsx index 7fb23eb3..20e00045 100644 --- a/modules/feature_flags/feature_flags/pages/Browse.tsx +++ b/modules/feature_flags/feature_flags/pages/Browse.tsx @@ -105,22 +105,23 @@ function Browse() { title={t(keys.feature_flags.browse.title)} description={t(keys.feature_flags.browse.description)} > - - router.visit(buildPath(next))} - /> -

- {tenant_id - ? t(keys.feature_flags.browse.viewing_tenant, { tenant_id }) - : t(keys.feature_flags.browse.viewing_system)} -

- - -
+ {/* One toolbar row instead of a near-empty card: picker, its hint, + and the flag count share the line the table sits under. */} +
+
+ router.visit(buildPath(next))} + /> +

+ {tenant_id + ? t(keys.feature_flags.browse.viewing_tenant, { tenant_id }) + : t(keys.feature_flags.browse.viewing_system)} +

+
{flags.length > 0 && ( -

+

{t(keys.feature_flags.browse.count, { count: flags.length })}

)} diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index 01e936be..b44b6592 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -12,7 +12,6 @@ from typing import Any from fastapi import FastAPI -from fastapi.encoders import jsonable_encoder from pydantic_settings import BaseSettings from settings.env_vars import env_prefix_for @@ -84,6 +83,9 @@ class ModuleSettingsView: env_prefix: str class_name: str fields: list[ModuleSettingField] + manage_url: str | None = None + """The module's own management page. When set, the generic editor renders + a link there instead of a second editor for the same fields.""" def _mask(value: Any) -> Any: @@ -194,16 +196,22 @@ def collect_module_settings( views: list[ModuleSettingsView] = [] seen: set[str] = set() + settings_services = getattr(app.state, "settings", None) + registry = getattr(settings_services, "module_registry", None) + + def _manage_url(package: str) -> str | None: + return registry.manage_url(package) if registry is not None else None + for mod in getattr(app.state.sm, "modules", ()): package = _package_of(mod) settings = _extract_settings(app, package) if settings is None: continue - views.append(_build_view(mod.meta.name, package, settings, by_package)) + views.append( + _build_view(mod.meta.name, package, settings, by_package, _manage_url(package)) + ) seen.add(package) - settings_services = getattr(app.state, "settings", None) - registry = getattr(settings_services, "module_registry", None) if registry is not None: for package in registry.all_packages(): if package in seen: @@ -211,7 +219,9 @@ def collect_module_settings( settings = _extract_settings(app, package) if settings is None: continue - views.append(_build_view(package.title(), package, settings, by_package)) + views.append( + _build_view(package.title(), package, settings, by_package, _manage_url(package)) + ) seen.add(package) views.sort(key=lambda v: v.module_name) @@ -223,6 +233,7 @@ def _build_view( package: str, settings: BaseSettings, overrides: dict[str, frozenset[str]] | None = None, + manage_url: str | None = None, ) -> ModuleSettingsView: prefix = env_prefix_for(package) overridden = (overrides or {}).get(package, frozenset()) @@ -235,6 +246,7 @@ def _build_view( env_prefix=prefix, class_name=type(settings).__name__, fields=fields, + manage_url=manage_url, ) @@ -249,41 +261,3 @@ async def overrides_by_package(service: SettingService) -> dict[str, frozenset[s from settings.store import SettingsStore return await SettingsStore(service).all_override_fields() - - -def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: - """Convert dataclass views to plain dicts for Inertia props. - - Field values arrive as whatever type the module declared — pydantic has - already coerced ``media_root: Path`` to a ``PosixPath``, ``timeout: - timedelta`` to a ``timedelta`` — and this screen reflects every installed - module's settings, so the set of types is open-ended by design. They are - encoded here rather than handed on as-is: this is the boundary where a - settings object stops being Python and becomes a prop. - """ - return [ - { - "module_name": v.module_name, - "package": v.package, - "env_prefix": v.env_prefix, - "class_name": v.class_name, - "fields": [ - { - "name": f.name, - "env_var": f.env_var, - "value": jsonable_encoder(f.value), - "default": jsonable_encoder(f.default), - "description": f.description, - "is_secret": f.is_secret, - "type": f.type, - "requires_restart": f.requires_restart, - "group": f.group, - "env_set": f.env_set, - "db_override": f.db_override, - "source": f.source, - } - for f in v.fields - ], - } - for v in views - ] diff --git a/modules/settings/settings/_module_settings_props.py b/modules/settings/settings/_module_settings_props.py new file mode 100644 index 00000000..8e626d82 --- /dev/null +++ b/modules/settings/settings/_module_settings_props.py @@ -0,0 +1,52 @@ +"""Serialize module-settings views into Inertia props. + +Split from ``_module_settings`` (collection) so each file keeps one +responsibility: that one discovers and shapes the views, this one is the +boundary where a settings object stops being Python and becomes a prop. +""" + +from __future__ import annotations + +from typing import Any + +from fastapi.encoders import jsonable_encoder + +from settings._module_settings import ModuleSettingsView + + +def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: + """Convert dataclass views to plain dicts for Inertia props. + + Field values arrive as whatever type the module declared — pydantic has + already coerced ``media_root: Path`` to a ``PosixPath``, ``timeout: + timedelta`` to a ``timedelta`` — and this screen reflects every installed + module's settings, so the set of types is open-ended by design. They are + encoded here rather than handed on as-is. + """ + return [ + { + "module_name": v.module_name, + "package": v.package, + "env_prefix": v.env_prefix, + "class_name": v.class_name, + "manage_url": v.manage_url, + "fields": [ + { + "name": f.name, + "env_var": f.env_var, + "value": jsonable_encoder(f.value), + "default": jsonable_encoder(f.default), + "description": f.description, + "is_secret": f.is_secret, + "type": f.type, + "requires_restart": f.requires_restart, + "group": f.group, + "env_set": f.env_set, + "db_override": f.db_override, + "source": f.source, + } + for f in v.fields + ], + } + for v in views + ] diff --git a/modules/settings/settings/endpoints/module_api.py b/modules/settings/settings/endpoints/module_api.py index adab6709..8f4f8780 100644 --- a/modules/settings/settings/endpoints/module_api.py +++ b/modules/settings/settings/endpoints/module_api.py @@ -18,8 +18,8 @@ collect_module_settings, is_secret_field, overrides_by_package, - serialize, ) +from settings._module_settings_props import serialize from settings.constants import MODULE_PACKAGE, PERM_DELETE, PERM_EDIT, PERM_VIEW from settings.contracts.events import SettingsReloaded from settings.deps import get_setting_service diff --git a/modules/settings/settings/endpoints/views.py b/modules/settings/settings/endpoints/views.py index 3509b078..bc3abe85 100644 --- a/modules/settings/settings/endpoints/views.py +++ b/modules/settings/settings/endpoints/views.py @@ -23,8 +23,8 @@ _package_of, collect_module_settings, overrides_by_package, - serialize, ) +from settings._module_settings_props import serialize from settings.constants import ( ERR_SETTING_NOT_FOUND, PERM_CREATE, diff --git a/modules/settings/settings/locales/en.json b/modules/settings/settings/locales/en.json index 13562939..542c212d 100644 --- a/modules/settings/settings/locales/en.json +++ b/modules/settings/settings/locales/en.json @@ -83,7 +83,10 @@ "source_env": "From environment", "source_env_hint": "{env_var} is set in this deployment", "source_default": "Default", - "head_title": "Modules" + "head_title": "Modules", + "managed_title": "Managed on its own page", + "managed_description": "{module} ships a purpose-built settings page; edit it there so validation and previews apply.", + "managed_open": "Open {module} settings" }, "modules_form": { "save": "Save", diff --git a/modules/settings/settings/module_registry.py b/modules/settings/settings/module_registry.py index bd28c7e8..4a80147b 100644 --- a/modules/settings/settings/module_registry.py +++ b/modules/settings/settings/module_registry.py @@ -17,11 +17,28 @@ class ModuleSettingsRegistry: """In-memory map of ``package`` → ``BaseSettings`` subclass.""" _classes: dict[str, type[BaseSettings]] = field(default_factory=dict) - - def register(self, package: str, cls: type[BaseSettings]) -> None: + _manage_urls: dict[str, str] = field(default_factory=dict) + + def register( + self, + package: str, + cls: type[BaseSettings], + manage_url: str | None = None, + ) -> None: if package in self._classes: raise ValueError(f"{package!r} already registered") self._classes[package] = cls + if manage_url: + self._manage_urls[package] = manage_url + + def manage_url(self, package: str) -> str | None: + """URL of the module's own management page, if it declared one. + + Modules with a purpose-built settings screen (e.g. Branding) declare it + so the generic module-settings editor links there instead of offering a + second, raw editor for the same fields. + """ + return self._manage_urls.get(package) def get(self, package: str) -> type[BaseSettings] | None: return self._classes.get(package) diff --git a/modules/settings/settings/pages/ModulesEdit.tsx b/modules/settings/settings/pages/ModulesEdit.tsx index f76cfeae..f5eec358 100644 --- a/modules/settings/settings/pages/ModulesEdit.tsx +++ b/modules/settings/settings/pages/ModulesEdit.tsx @@ -1,9 +1,10 @@ -import { Head } from '@inertiajs/react'; +import { Head, Link } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; +import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card } from '@simple-module-py/ui/components/ui/card'; import { Input } from '@simple-module-py/ui/components/ui/input'; import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; -import { Box, Search } from 'lucide-react'; +import { ArrowRight, Box, Search } from 'lucide-react'; import type React from 'react'; import { useMemo, useState } from 'react'; import { ModuleForm, type ModuleView } from './components/ModuleForm'; @@ -96,7 +97,29 @@ function ModulesEdit({ modules, testable = [] }: Props) {
- {current ? ( + {current?.manage_url ? ( + + {/* No second editor for these fields — the module's own page is + the one place they're edited. */} +
+ + +

+ {t(keys.settings.modules.managed_title)} +

+

+ {t(keys.settings.modules.managed_description, { module: current.module_name })} +

+ +
+ + ) : current ? ( diff --git a/modules/settings/settings/pages/components/ModuleForm.tsx b/modules/settings/settings/pages/components/ModuleForm.tsx index 50122046..6b28ee1f 100644 --- a/modules/settings/settings/pages/components/ModuleForm.tsx +++ b/modules/settings/settings/pages/components/ModuleForm.tsx @@ -11,6 +11,8 @@ export type ModuleView = { env_prefix: string; class_name: string; fields: FieldMeta[]; + /** The module's own management page; when set, the generic editor links there. */ + manage_url?: string | null; }; type Props = { diff --git a/modules/settings/settings/registration.py b/modules/settings/settings/registration.py index 2ab3196f..3456ead1 100644 --- a/modules/settings/settings/registration.py +++ b/modules/settings/settings/registration.py @@ -26,9 +26,15 @@ def register_module_settings( package: str, settings_cls: type[BaseSettings], services_factory: Callable[[BaseSettings], Any], + manage_url: str | None = None, ) -> None: - """Register a module's BaseSettings class and mount its services on app.state.""" + """Register a module's BaseSettings class and mount its services on app.state. + + ``manage_url`` points at the module's own management page when it has one; + the generic module-settings editor then links there instead of rendering a + second editor for the same fields. + """ registry = getattr(app.state, MODULE_PACKAGE).module_registry - registry.register(package, settings_cls) + registry.register(package, settings_cls, manage_url=manage_url) defaults = settings_cls() setattr(app.state, package, services_factory(defaults)) diff --git a/modules/settings/tests/test_module_settings_render.py b/modules/settings/tests/test_module_settings_render.py index 0dcbf3ca..87c73bc6 100644 --- a/modules/settings/tests/test_module_settings_render.py +++ b/modules/settings/tests/test_module_settings_render.py @@ -88,3 +88,22 @@ async def test_full_page_load_still_works( assert resp.status_code == _OK assert resp.headers["content-type"].startswith("text/html") + + async def test_dedicated_page_modules_link_instead_of_double_editing( + self, + app_with_path_setting: FastAPI, + authenticated_client: httpx.AsyncClient, + ) -> None: + """Branding declares its own page; generic modules don't. + + The editor uses ``manage_url`` to link there instead of rendering a + second editor for the same fields. + """ + resp = await authenticated_client.get("/admin/settings/", headers=_INERTIA) + modules = resp.json()["props"]["modules"] + + branding = next(m for m in modules if m["package"] == "branding") + assert branding["manage_url"] == "/admin/branding/" + + demo = next(m for m in modules if m["package"] == "pathdemo") + assert demo["manage_url"] is None diff --git a/modules/settings/tests/test_module_settings_serialize.py b/modules/settings/tests/test_module_settings_serialize.py index cd3cf69e..9f50cedd 100644 --- a/modules/settings/tests/test_module_settings_serialize.py +++ b/modules/settings/tests/test_module_settings_serialize.py @@ -20,8 +20,8 @@ from settings._module_settings import ( ModuleSettingField, ModuleSettingsView, - serialize, ) +from settings._module_settings_props import serialize def _view_with(value: Any, default: Any = "") -> ModuleSettingsView: diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index 478fe935..27dff893 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -17,6 +17,8 @@ export default { 'audit_log.browse.showing': '', 'audit_log.browse.title': '', 'audit_log.changes.fields_set': '', + 'audit_log.changes.fields_set_one': '', + 'audit_log.changes.fields_set_other': '', 'audit_log.changes.no_changes': '', 'audit_log.changes.show_less': '', 'audit_log.changes.show_more': '', @@ -184,28 +186,31 @@ export default { 'branding.nav.branding': '', 'dashboard.doctor.active': '', 'dashboard.doctor.alert': '', + 'dashboard.doctor.all_clear_hint': '', + 'dashboard.doctor.all_clear_title': '', 'dashboard.doctor.applied': '', - 'dashboard.doctor.apply': '', - 'dashboard.doctor.clean': '', + 'dashboard.doctor.at_head': '', + 'dashboard.doctor.behind': '', 'dashboard.doctor.description': '', - 'dashboard.doctor.dev_server': '', + 'dashboard.doctor.diagnostics': '', + 'dashboard.doctor.env_database': '', + 'dashboard.doctor.env_locales': '', + 'dashboard.doctor.env_mode': '', + 'dashboard.doctor.env_python': '', + 'dashboard.doctor.env_revision': '', 'dashboard.doctor.environment': '', - 'dashboard.doctor.generate': '', 'dashboard.doctor.installed_modules': '', - 'dashboard.doctor.just_now': '', 'dashboard.doctor.loaded': '', 'dashboard.doctor.ok': '', - 'dashboard.doctor.pending': '', 'dashboard.doctor.recent_migrations': '', 'dashboard.doctor.rerun': '', 'dashboard.doctor.review': '', 'dashboard.doctor.run_command': '', - 'dashboard.doctor.running': '', - 'dashboard.doctor.stat_checks_passed': '', + 'dashboard.doctor.stat_errors': '', 'dashboard.doctor.stat_health': '', 'dashboard.doctor.stat_modules': '', - 'dashboard.doctor.stat_pending_migrations': '', - 'dashboard.doctor.static_checks': '', + 'dashboard.doctor.stat_warnings': '', + 'dashboard.doctor.suggestion': '', 'dashboard.doctor.title': '', 'dashboard.home.description': '', 'dashboard.home.description_body': '', @@ -484,6 +489,9 @@ export default { 'settings.modules.env_var_hint': '', 'settings.modules.field_count_suffix': '', 'settings.modules.head_title': '', + 'settings.modules.managed_description': '', + 'settings.modules.managed_open': '', + 'settings.modules.managed_title': '', 'settings.modules.no_fields': '', 'settings.modules.search_placeholder': '', 'settings.modules.secret_badge': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index 1043e40b..30e8cba7 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -23,6 +23,8 @@ export const keys = { }, changes: { fields_set: 'audit_log.changes.fields_set', + fields_set_one: 'audit_log.changes.fields_set_one', + fields_set_other: 'audit_log.changes.fields_set_other', no_changes: 'audit_log.changes.no_changes', show_less: 'audit_log.changes.show_less', show_more: 'audit_log.changes.show_more', @@ -241,28 +243,31 @@ export const keys = { doctor: { active: 'dashboard.doctor.active', alert: 'dashboard.doctor.alert', + all_clear_hint: 'dashboard.doctor.all_clear_hint', + all_clear_title: 'dashboard.doctor.all_clear_title', applied: 'dashboard.doctor.applied', - apply: 'dashboard.doctor.apply', - clean: 'dashboard.doctor.clean', + at_head: 'dashboard.doctor.at_head', + behind: 'dashboard.doctor.behind', description: 'dashboard.doctor.description', - dev_server: 'dashboard.doctor.dev_server', + diagnostics: 'dashboard.doctor.diagnostics', + env_database: 'dashboard.doctor.env_database', + env_locales: 'dashboard.doctor.env_locales', + env_mode: 'dashboard.doctor.env_mode', + env_python: 'dashboard.doctor.env_python', + env_revision: 'dashboard.doctor.env_revision', environment: 'dashboard.doctor.environment', - generate: 'dashboard.doctor.generate', installed_modules: 'dashboard.doctor.installed_modules', - just_now: 'dashboard.doctor.just_now', loaded: 'dashboard.doctor.loaded', ok: 'dashboard.doctor.ok', - pending: 'dashboard.doctor.pending', recent_migrations: 'dashboard.doctor.recent_migrations', rerun: 'dashboard.doctor.rerun', review: 'dashboard.doctor.review', run_command: 'dashboard.doctor.run_command', - running: 'dashboard.doctor.running', - stat_checks_passed: 'dashboard.doctor.stat_checks_passed', + stat_errors: 'dashboard.doctor.stat_errors', stat_health: 'dashboard.doctor.stat_health', stat_modules: 'dashboard.doctor.stat_modules', - stat_pending_migrations: 'dashboard.doctor.stat_pending_migrations', - static_checks: 'dashboard.doctor.static_checks', + stat_warnings: 'dashboard.doctor.stat_warnings', + suggestion: 'dashboard.doctor.suggestion', title: 'dashboard.doctor.title', }, home: { @@ -638,6 +643,9 @@ export const keys = { env_var_hint: 'settings.modules.env_var_hint', field_count_suffix: 'settings.modules.field_count_suffix', head_title: 'settings.modules.head_title', + managed_description: 'settings.modules.managed_description', + managed_open: 'settings.modules.managed_open', + managed_title: 'settings.modules.managed_title', no_fields: 'settings.modules.no_fields', search_placeholder: 'settings.modules.search_placeholder', secret_badge: 'settings.modules.secret_badge', From 42b0639f29a0ee4efb90e1549ef27e9214e99705 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Thu, 27 Aug 2026 14:58:40 +0200 Subject: [PATCH 02/13] style(admin): drop the alarm-red admin skin for the app's primary accent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Admin Panel shell used red everywhere — sidebar tint, badge, logo mark, avatar, active states — which read as a warning, not a place. The admin area now speaks the same visual language as the app sidebar: primary-accent active states, the branding-driven logo mark, and an emerald Admin Panel badge. The badge and the panel's own menu are the wayfinding; a color no longer shouts it. Because the accent rides the branding primary tokens, a deployment's custom brand color now restyles the admin area along with the app. The red-tinted --color-admin-* tokens had no other consumers, so they go too. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui --- packages/ui/src/layouts/AdminLayout.tsx | 24 +++++++++++++----------- packages/ui/src/styles/globals.css | 7 ------- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/packages/ui/src/layouts/AdminLayout.tsx b/packages/ui/src/layouts/AdminLayout.tsx index ba1ed4db..9da8ebc0 100644 --- a/packages/ui/src/layouts/AdminLayout.tsx +++ b/packages/ui/src/layouts/AdminLayout.tsx @@ -3,15 +3,17 @@ import { keys, useT } from '@simple-module-py/i18n'; import type React from 'react'; import { SidebarLayout } from './SidebarLayout'; +// Same visual language as the app sidebar — the admin area announces itself +// through the panel badge and its own menu, not through an alarm color. const THEME = { - sidebarBg: 'bg-admin-bg', - accentColor: 'bg-gradient-to-br from-red-500 to-red-700', - avatarBg: 'bg-red-700', - hoverBg: 'hover:bg-admin-hover', - activeClass: 'bg-red-600/15 text-red-300 border-l-2 border-red-400', + sidebarBg: 'bg-app-sidebar', + accentColor: 'bg-gradient-to-br from-primary-400 to-primary-600', + avatarBg: 'bg-primary-700', + hoverBg: 'hover:bg-app-sidebar-hover', + activeClass: 'bg-primary-600/15 text-primary-300 border-l-2 border-primary-400', inactiveClass: - 'text-admin-text hover:bg-admin-hover hover:text-white border-l-2 border-transparent', - mutedTextClass: 'text-admin-text-muted', + 'text-app-sidebar-text hover:bg-app-sidebar-hover hover:text-white border-l-2 border-transparent', + mutedTextClass: 'text-app-sidebar-text-muted', mobileTitleLabel: 'Admin', } as const; @@ -25,11 +27,11 @@ function AdminBadge() {
- + {t(keys.ui.admin.panel_badge)} @@ -55,7 +57,7 @@ function BackToApp() {
{diagnostics.map((d) => ( diff --git a/modules/users/tests/test_settings.py b/modules/users/tests/test_settings.py index bc628197..31151710 100644 --- a/modules/users/tests/test_settings.py +++ b/modules/users/tests/test_settings.py @@ -165,3 +165,30 @@ def test_real_secrets_accepted_in_production(self, monkeypatch): ) assert s.reset_password_token_secret == "real-reset-secret" assert s.verification_token_secret == "real-verify-secret" + + +class TestLoginRedirectUrl: + """A blanked ``login_redirect_url`` must never reach a consumer. + + Nothing stops an admin clearing it in the generic module-settings editor, + and every consumer treats it as a destination — the login view hands it to + Inertia (``router.visit("")`` reloads the current page), while the Keycloak + and OAuth callbacks put it directly into a ``Location`` header. Normalising + on the settings class covers all three, since hydration and + ``apply_changes_and_reload`` both reconstruct through it. + """ + + def test_blank_falls_back_to_the_default(self): + from users.settings import UsersSettings + + assert UsersSettings(login_redirect_url="").login_redirect_url == "/dashboard/" + + def test_whitespace_only_falls_back_to_the_default(self): + from users.settings import UsersSettings + + assert UsersSettings(login_redirect_url=" ").login_redirect_url == "/dashboard/" + + def test_a_real_value_is_left_alone(self): + from users.settings import UsersSettings + + assert UsersSettings(login_redirect_url="/home/").login_redirect_url == "/home/" diff --git a/modules/users/tests/test_views.py b/modules/users/tests/test_views.py index 5b2bd550..1e2cd78b 100644 --- a/modules/users/tests/test_views.py +++ b/modules/users/tests/test_views.py @@ -124,10 +124,17 @@ async def test_login_redirect_url_is_dashboard_when_installed(self, anon_client) @pytest.mark.anyio async def test_login_redirect_url_falls_back_when_setting_is_blanked(self, anon_client): """An admin can blank the DB-backed setting via the generic module - editor (no non-empty validator on the field) — the login page must - never hand the frontend "" as a navigation target.""" - anon_client._transport.app.state.users.settings.login_redirect_url = "" + editor — no consumer may then receive "". + Normalisation lives on the settings class (unit-tested in + test_settings.py), so it applies wherever the value is constructed. + Assigning the attribute here would bypass pydantic and test nothing, + so this goes through the real path. + """ + from users.settings import UsersSettings + + app = anon_client._transport.app + app.state.users.settings = UsersSettings(login_redirect_url="") resp = await anon_client.get( "/users/login", headers={"X-Inertia": "true", "X-Inertia-Version": "1.0"}, diff --git a/modules/users/users/auth_local/views.py b/modules/users/users/auth_local/views.py index 50e973ef..ca3956df 100644 --- a/modules/users/users/auth_local/views.py +++ b/modules/users/users/auth_local/views.py @@ -62,13 +62,10 @@ async def login_page(request: Request, inertia: InertiaDep) -> InertiaResponse: # handler clears it once login actually succeeds. "login_redirect_url": ( safe_next_or_none(request.session.get(SESSION_NEXT_KEY)) + # Never "" — UsersSettings normalises a blanked value back to + # the default, so every consumer (here, Keycloak, OAuth) gets + # a usable target rather than each guarding for itself. or users_settings.login_redirect_url - # An admin can blank the DB-backed setting via the generic - # module-settings editor (no non-empty validator on the - # field) — never hand the frontend "" as a navigation - # target (Inertia's router.visit("") just reloads the - # current page, stranding the user on /login). - or "/dashboard/" ), "oauth_providers": users_state.oauth_providers, }, diff --git a/modules/users/users/settings.py b/modules/users/users/settings.py index 83143b73..9007d46d 100644 --- a/modules/users/users/settings.py +++ b/modules/users/users/settings.py @@ -12,7 +12,7 @@ import os -from pydantic import Field, model_validator +from pydantic import Field, field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict from simple_module_core.dotenv import env_str from simple_module_core.environments import NON_PROD_ENVIRONMENTS @@ -20,6 +20,8 @@ _PLACEHOLDER_RESET_SECRET = "dev-reset-token-secret-change-me" _PLACEHOLDER_VERIFY_SECRET = "dev-verify-token-secret-change-me" +DEFAULT_LOGIN_REDIRECT_URL = "/dashboard/" + class UsersSettings(BaseSettings): """Local user management configuration.""" @@ -33,7 +35,21 @@ class UsersSettings(BaseSettings): # Where the login page sends a successful sign-in. Sites without the # bundled ``dashboard`` module (``smpy new --preset minimal``) override # this to wherever their post-login landing lives. - login_redirect_url: str = "/dashboard/" + login_redirect_url: str = DEFAULT_LOGIN_REDIRECT_URL + + @field_validator("login_redirect_url") + @classmethod + def _non_empty_redirect(cls, value: str) -> str: + """Blank is never a usable navigation target. + + Nothing stops an admin clearing this in the generic module-settings + editor, and every consumer treats it as a destination: the login view + hands it to Inertia (``router.visit("")`` silently reloads the current + page) while the Keycloak and OAuth callbacks put it straight into a + ``Location`` header. Normalising here fixes all three at once — + hydration runs every value through this class. + """ + return value.strip() or DEFAULT_LOGIN_REDIRECT_URL # Token secrets — MUST be set in production. Dev default is a deterministic # placeholder that's obvious in logs so it can't be mistaken for a real key. From c9d3d325c5ed7ba4b09c9ec83d49e347baed1208 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 28 Aug 2026 10:45:37 +0200 Subject: [PATCH 06/13] fix: normalise a blanked login_redirect_url in the keycloak provider too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous pass fixed only UsersSettings. Keycloak has its own settings class with its own login_redirect_url, and its callback reads that copy into a Location header — so blanking it there still produced an empty redirect, and the last commit message overclaimed by saying Keycloak was covered. The shared normalisation lives in simple_module_core.redirect_safety (already the home for safe_next/safe_next_or_none and imported by both providers) rather than in one module, since the two provider modules must not import each other. Each provider keeps its own field validator, which is where hydration and apply_changes_and_reload both run. Verified: make lint clean, 2167 python + 127 js tests pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui --- .../simple_module_core/redirect_safety.py | 24 +++++++++++++++++- modules/keycloak/keycloak/settings.py | 19 ++++++++++++-- .../keycloak/tests/test_keycloak_module.py | 25 +++++++++++++++++++ modules/users/users/settings.py | 14 +++++------ 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/framework/core/simple_module_core/redirect_safety.py b/framework/core/simple_module_core/redirect_safety.py index 82e09546..62af4bff 100644 --- a/framework/core/simple_module_core/redirect_safety.py +++ b/framework/core/simple_module_core/redirect_safety.py @@ -53,4 +53,26 @@ def safe_next_or_none(raw: str | None) -> str | None: return result or None -__all__ = ["DEFAULT_FALLBACK", "SESSION_NEXT_KEY", "safe_next", "safe_next_or_none"] +def non_empty_redirect(value: str, *, default: str) -> str: + """Normalise a configured redirect destination, never returning ``""``. + + Any auth provider can expose a ``login_redirect_url``-style setting, and + nothing stops an admin clearing it in the generic module-settings editor. + Every consumer treats the value as a destination — Inertia's + ``router.visit("")`` silently reloads the current page, and an empty + ``Location`` header is a broken redirect — so providers normalise on their + settings class, where hydration and ``apply_changes_and_reload`` both run. + It lives here rather than in one provider because the providers must not + import each other (cross-module coupling), and this is the same concern as + the rest of this module. + """ + return value.strip() or default + + +__all__ = [ + "DEFAULT_FALLBACK", + "SESSION_NEXT_KEY", + "non_empty_redirect", + "safe_next", + "safe_next_or_none", +] diff --git a/modules/keycloak/keycloak/settings.py b/modules/keycloak/keycloak/settings.py index e9e9745c..b9f2b286 100644 --- a/modules/keycloak/keycloak/settings.py +++ b/modules/keycloak/keycloak/settings.py @@ -2,10 +2,13 @@ from __future__ import annotations -from pydantic import Field, model_validator +from pydantic import Field, field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict from simple_module_core.dotenv import env_str from simple_module_core.environments import NON_PROD_ENVIRONMENTS +from simple_module_core.redirect_safety import non_empty_redirect + +DEFAULT_LOGIN_REDIRECT_URL = "/dashboard/" class KeycloakSettings(BaseSettings): @@ -20,13 +23,25 @@ class KeycloakSettings(BaseSettings): roles_claim_path: str = "realm_access.roles" admin_role: str = "admin" - login_redirect_url: str = "/dashboard/" + login_redirect_url: str = DEFAULT_LOGIN_REDIRECT_URL jwks_cache_ttl_seconds: int = 3600 role_mapping: dict[str, str] = Field( default_factory=lambda: {"admin": "admin", "user": "user"}, ) + @field_validator("login_redirect_url") + @classmethod + def _non_empty_redirect(cls, value: str) -> str: + """Blank is never a usable navigation target. + + The callback puts this straight into a ``Location`` header, and an + admin can clear it in the generic module-settings editor. Normalising + on the class covers hydration and ``apply_changes_and_reload`` alike. + Users' provider has its own copy of this field and does the same. + """ + return non_empty_redirect(value, default=DEFAULT_LOGIN_REDIRECT_URL) + @model_validator(mode="after") def _check_required_in_production(self) -> KeycloakSettings: import os diff --git a/modules/keycloak/tests/test_keycloak_module.py b/modules/keycloak/tests/test_keycloak_module.py index 7b073d11..2b5164e2 100644 --- a/modules/keycloak/tests/test_keycloak_module.py +++ b/modules/keycloak/tests/test_keycloak_module.py @@ -20,3 +20,28 @@ def test_keycloak_provider_satisfies_protocol(): provider = KeycloakAuthProvider() assert isinstance(provider, AuthProvider) assert provider.name == "keycloak" + + +class TestKeycloakLoginRedirectUrl: + """A blanked ``login_redirect_url`` must never reach the OIDC callback. + + ``endpoints/api.py`` puts this value straight into a ``Location`` header, + and an admin can clear it in the generic module-settings editor. This is a + second copy of the field — ``users`` has its own — so it needs its own + guard; the two provider modules must not import each other. + """ + + def test_blank_falls_back_to_the_default(self): + from keycloak.settings import KeycloakSettings + + assert KeycloakSettings(login_redirect_url="").login_redirect_url == "/dashboard/" + + def test_whitespace_only_falls_back_to_the_default(self): + from keycloak.settings import KeycloakSettings + + assert KeycloakSettings(login_redirect_url=" ").login_redirect_url == "/dashboard/" + + def test_a_real_value_is_left_alone(self): + from keycloak.settings import KeycloakSettings + + assert KeycloakSettings(login_redirect_url="/home/").login_redirect_url == "/home/" diff --git a/modules/users/users/settings.py b/modules/users/users/settings.py index 9007d46d..22ba66f9 100644 --- a/modules/users/users/settings.py +++ b/modules/users/users/settings.py @@ -16,6 +16,7 @@ from pydantic_settings import BaseSettings, SettingsConfigDict from simple_module_core.dotenv import env_str from simple_module_core.environments import NON_PROD_ENVIRONMENTS +from simple_module_core.redirect_safety import non_empty_redirect _PLACEHOLDER_RESET_SECRET = "dev-reset-token-secret-change-me" _PLACEHOLDER_VERIFY_SECRET = "dev-verify-token-secret-change-me" @@ -42,14 +43,13 @@ class UsersSettings(BaseSettings): def _non_empty_redirect(cls, value: str) -> str: """Blank is never a usable navigation target. - Nothing stops an admin clearing this in the generic module-settings - editor, and every consumer treats it as a destination: the login view - hands it to Inertia (``router.visit("")`` silently reloads the current - page) while the Keycloak and OAuth callbacks put it straight into a - ``Location`` header. Normalising here fixes all three at once — - hydration runs every value through this class. + Covers this module's consumers — the login view (which hands it to + Inertia, where ``router.visit("")`` silently reloads the current page) + and the generic OAuth callback (which puts it in a ``Location`` + header). Keycloak has its own settings class with its own copy of this + field and normalises it the same way. """ - return value.strip() or DEFAULT_LOGIN_REDIRECT_URL + return non_empty_redirect(value, default=DEFAULT_LOGIN_REDIRECT_URL) # Token secrets — MUST be set in production. Dev default is a deterministic # placeholder that's obvious in logs so it can't be mistaken for a real key. From 02c6a5e98bb60b3fec3004f754fa0c752fbb123a Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 28 Aug 2026 11:00:41 +0200 Subject: [PATCH 07/13] perf(admin): run doctor diagnostics off the event loop; unify sidebar theme MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - /admin/doctor did blocking filesystem walks and AST parses (module coupling/page checks, the alembic script directory) inline on the event loop, serially after the DB fetch. They now run via asyncio.to_thread alongside it, so one doctor request no longer stalls every other coroutine on the worker for a full framework scan. Still live data — no caching added. - AdminLayout's THEME was byte-for-byte AuthenticatedLayout's after this branch dropped the red admin skin, which is exactly the drift that commit set out to remove. Both now spread DEFAULT_SIDEBAR_THEME and override only mobileTitleLabel. The theme moved to its own layouts/sidebar-theme.ts (SidebarLayout was over the 300-line cap) and is re-exported from SidebarLayout so existing importers are unaffected. Verified: make lint clean, 2167 python + 127 js tests pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui --- .../dashboard/dashboard/endpoints/views.py | 17 +++++++++-- packages/ui/src/layouts/AdminLayout.tsx | 11 ++----- .../ui/src/layouts/AuthenticatedLayout.tsx | 11 ++----- packages/ui/src/layouts/SidebarLayout.tsx | 15 +++------- packages/ui/src/layouts/sidebar-theme.ts | 30 +++++++++++++++++++ 5 files changed, 52 insertions(+), 32 deletions(-) create mode 100644 packages/ui/src/layouts/sidebar-theme.ts diff --git a/modules/dashboard/dashboard/endpoints/views.py b/modules/dashboard/dashboard/endpoints/views.py index 747fd050..c5948c6d 100644 --- a/modules/dashboard/dashboard/endpoints/views.py +++ b/modules/dashboard/dashboard/endpoints/views.py @@ -6,6 +6,8 @@ from __future__ import annotations +import asyncio + from fastapi import APIRouter, Depends, HTTPException, Request from inertia import InertiaResponse from simple_module_core.permissions import is_admin @@ -63,14 +65,23 @@ async def doctor( """`make doctor` mirror — live diagnostics, migrations, modules, env.""" from dashboard.doctor import collect_diagnostics, environment_info, migration_overview - stats = await fetch_dashboard_stats(db, request.app) + # collect_diagnostics/migration_overview do blocking filesystem walks + + # AST parses (module coupling/page checks, the alembic script directory) — + # run them off the event loop and alongside the DB stats fetch instead of + # serially after it, so one doctor request doesn't stall every other + # coroutine on this worker for the duration of a full framework scan. + stats, diagnostics, migration = await asyncio.gather( + fetch_dashboard_stats(db, request.app), + asyncio.to_thread(collect_diagnostics, request.app), + asyncio.to_thread(migration_overview, request.app), + ) return await inertia.render( _PAGE_DOCTOR, { "module_count": stats["module_count"], "system_info": stats["system_info"], - "diagnostics": collect_diagnostics(request.app), - "migration": migration_overview(request.app), + "diagnostics": diagnostics, + "migration": migration, "environment": environment_info(request.app), }, ) diff --git a/packages/ui/src/layouts/AdminLayout.tsx b/packages/ui/src/layouts/AdminLayout.tsx index 9da8ebc0..1334d7bd 100644 --- a/packages/ui/src/layouts/AdminLayout.tsx +++ b/packages/ui/src/layouts/AdminLayout.tsx @@ -1,19 +1,12 @@ import { Link } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import type React from 'react'; -import { SidebarLayout } from './SidebarLayout'; +import { DEFAULT_SIDEBAR_THEME, SidebarLayout } from './SidebarLayout'; // Same visual language as the app sidebar — the admin area announces itself // through the panel badge and its own menu, not through an alarm color. const THEME = { - sidebarBg: 'bg-app-sidebar', - accentColor: 'bg-gradient-to-br from-primary-400 to-primary-600', - avatarBg: 'bg-primary-700', - hoverBg: 'hover:bg-app-sidebar-hover', - activeClass: 'bg-primary-600/15 text-primary-300 border-l-2 border-primary-400', - inactiveClass: - 'text-app-sidebar-text hover:bg-app-sidebar-hover hover:text-white border-l-2 border-transparent', - mutedTextClass: 'text-app-sidebar-text-muted', + ...DEFAULT_SIDEBAR_THEME, mobileTitleLabel: 'Admin', } as const; diff --git a/packages/ui/src/layouts/AuthenticatedLayout.tsx b/packages/ui/src/layouts/AuthenticatedLayout.tsx index aaf08491..879cdeef 100644 --- a/packages/ui/src/layouts/AuthenticatedLayout.tsx +++ b/packages/ui/src/layouts/AuthenticatedLayout.tsx @@ -1,16 +1,9 @@ import { Toaster } from '@simple-module-py/ui/components/ui/sonner'; import type React from 'react'; -import { SidebarLayout } from './SidebarLayout'; +import { DEFAULT_SIDEBAR_THEME, SidebarLayout } from './SidebarLayout'; const THEME = { - sidebarBg: 'bg-app-sidebar', - accentColor: 'bg-gradient-to-br from-primary-400 to-primary-600', - avatarBg: 'bg-primary-700', - hoverBg: 'hover:bg-app-sidebar-hover', - activeClass: 'bg-primary-600/15 text-primary-300 border-l-2 border-primary-400', - inactiveClass: - 'text-app-sidebar-text hover:bg-app-sidebar-hover hover:text-white border-l-2 border-transparent', - mutedTextClass: 'text-app-sidebar-text-muted', + ...DEFAULT_SIDEBAR_THEME, mobileTitleLabel: 'SimpleModule', } as const; diff --git a/packages/ui/src/layouts/SidebarLayout.tsx b/packages/ui/src/layouts/SidebarLayout.tsx index c0fdbb72..8164b745 100644 --- a/packages/ui/src/layouts/SidebarLayout.tsx +++ b/packages/ui/src/layouts/SidebarLayout.tsx @@ -21,6 +21,7 @@ import { darkSurfaceLogo } from '../lib/brand'; import type { MenuItem, SharedProps } from '../types'; import { AdminSectionLink } from './AdminSectionLink'; import { SidebarUserMenu } from './SidebarUserMenu'; +import { DEFAULT_SIDEBAR_THEME, type SidebarTheme } from './sidebar-theme'; // A stable reference for "no items" — `menus?.[key] ?? []` would otherwise // mint a fresh empty array every render, and that array flows into @@ -44,17 +45,6 @@ function groupMenuItems(items: MenuItem[]): { group: string; items: MenuItem[] } return groups; } -interface SidebarTheme { - sidebarBg: string; - accentColor: string; - avatarBg: string; - hoverBg: string; - activeClass: string; - inactiveClass: string; - mutedTextClass: string; - mobileTitleLabel: string; -} - interface SidebarLayoutProps { children: React.ReactNode; menuKey: 'sidebar' | 'adminSidebar'; @@ -289,3 +279,6 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S ); } + +export type { SidebarTheme }; +export { DEFAULT_SIDEBAR_THEME }; diff --git a/packages/ui/src/layouts/sidebar-theme.ts b/packages/ui/src/layouts/sidebar-theme.ts new file mode 100644 index 00000000..0358486b --- /dev/null +++ b/packages/ui/src/layouts/sidebar-theme.ts @@ -0,0 +1,30 @@ +/** + * The sidebar's visual language, shared by every sidebar-driven layout. + * + * Split from SidebarLayout so the palette is a data concern with one home: + * AdminLayout and AuthenticatedLayout both spread the default and override + * only `mobileTitleLabel`, so a tweak lands once instead of drifting between + * the two shells. + */ + +export interface SidebarTheme { + sidebarBg: string; + accentColor: string; + avatarBg: string; + hoverBg: string; + activeClass: string; + inactiveClass: string; + mutedTextClass: string; + mobileTitleLabel: string; +} + +export const DEFAULT_SIDEBAR_THEME: Omit = { + sidebarBg: 'bg-app-sidebar', + accentColor: 'bg-gradient-to-br from-primary-400 to-primary-600', + avatarBg: 'bg-primary-700', + hoverBg: 'hover:bg-app-sidebar-hover', + activeClass: 'bg-primary-600/15 text-primary-300 border-l-2 border-primary-400', + inactiveClass: + 'text-app-sidebar-text hover:bg-app-sidebar-hover hover:text-white border-l-2 border-transparent', + mutedTextClass: 'text-app-sidebar-text-muted', +}; From fc59c6c46b5813d933b01637df97a634a252b3cf Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 28 Aug 2026 11:28:57 +0200 Subject: [PATCH 08/13] fix(qa): type-aware secret masking + a visible focus ring on sidebar toggles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Browser QA of the admin area found two defects worth fixing here: - Module settings masked `reset_password_token_lifetime_seconds` as a secret, so an admin could neither read nor edit a plain duration. The name pattern already dodges the bare words "token" and "key", but it cannot dodge "password" — and that field is an int. Credential material is always a string, so the declared type now gates the match, which closes the whole class rather than this one name. The mask-sentinel stripper is unaffected: it already requires the value to equal the string sentinel. - The sidebar's icon-only open/close buttons showed no focus ring under keyboard navigation (WCAG 2.4.7). Button's default ring-ring/50 is invisible on the near-black sidebar, so those two get a light ring. Text links beside them already read via the UA outline. Not changed, with reasons: the Users "You're the only account" prompt renders above a one-row table by design — it is an onboarding nudge, not an empty state, and UsersEmpty.tsx documents why hiding the row would be wrong. Branding's silent handling of server 422s, the uncounted maxlength truncation, and BackgroundTasks accepting a negative max_retries are all real but pre-existing, outside this branch's changes, and want their own change with a validation design behind it. Verified: make lint clean, 2169 python + 127 js tests pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui --- modules/settings/settings/_module_settings.py | 9 ++++-- .../tests/test_settings_field_sources.py | 32 +++++++++++++++++++ packages/ui/src/layouts/SidebarLayout.tsx | 11 +++++-- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index b44b6592..e577251d 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -158,7 +158,12 @@ def _field_view( cls = type(settings) info = cls.model_fields[name] raw_value = getattr(settings, name) - secret = is_secret_field(name) + value_type = value_type_for_field(cls, name) + # Credential material is always a string. Without this, a numeric field + # whose name merely contains a secret-ish word gets masked and becomes + # uneditable — `reset_password_token_lifetime_seconds` is an int, but it + # matches on "password" the same way the real secrets do. + secret = value_type == "string" and is_secret_field(name) extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} default = _resolve_default(info) env_var = f"{prefix}{name.upper()}" @@ -170,7 +175,7 @@ def _field_view( default=_mask(default) if secret else default, description=info.description or "", is_secret=secret, - type=value_type_for_field(cls, name), + type=value_type, requires_restart=bool(extra.get("requires_restart", False)), group=extra.get("group"), env_set=live_env_var is not None and live_env_var in os.environ, diff --git a/modules/settings/tests/test_settings_field_sources.py b/modules/settings/tests/test_settings_field_sources.py index e4888fcd..9ad1199a 100644 --- a/modules/settings/tests/test_settings_field_sources.py +++ b/modules/settings/tests/test_settings_field_sources.py @@ -120,3 +120,35 @@ async def test_failing_check_still_returns_200_with_the_reason(self, authenticat for check in body["checks"]: assert check["status"] in ("healthy", "degraded", "unhealthy") assert "detail" in check + + +class TestSecretMaskingIsTypeAware: + """Only string fields can hold credential material. + + The name-based pattern deliberately avoids the bare words "token" and + "key", but it cannot avoid "password" — and + ``reset_password_token_lifetime_seconds`` is an int that contains it. + Masking it made a plain duration uneditable in the admin UI, so the + declared type gates the match. + """ + + def _field(self, cls, name: str): + from settings._module_settings import _field_view + + return _field_view(name, cls(), "SM_USERS_", frozenset()) + + def test_an_int_named_like_a_secret_is_not_masked(self): + from users.settings import UsersSettings + + field = self._field(UsersSettings, "reset_password_token_lifetime_seconds") + assert field.is_secret is False + assert field.type == "int" + assert isinstance(field.value, int) + + def test_a_real_string_secret_is_still_masked(self): + from settings._module_settings import SECRET_MASK + from users.settings import UsersSettings + + field = self._field(UsersSettings, "reset_password_token_secret") + assert field.is_secret is True + assert field.value == SECRET_MASK diff --git a/packages/ui/src/layouts/SidebarLayout.tsx b/packages/ui/src/layouts/SidebarLayout.tsx index 8164b745..d5e961c8 100644 --- a/packages/ui/src/layouts/SidebarLayout.tsx +++ b/packages/ui/src/layouts/SidebarLayout.tsx @@ -29,6 +29,13 @@ import { DEFAULT_SIDEBAR_THEME, type SidebarTheme } from './sidebar-theme'; // render where a menu is absent instead of only when its contents change. const NO_ITEMS: MenuItem[] = []; +// The sidebar is near-black in every theme, where Button's default +// `ring-ring/50` is effectively invisible — these icon-only toggles are +// reachable by keyboard, so they get a light ring that actually shows +// (WCAG 2.4.7). Text links beside them fall back to the UA outline, which +// already reads on this surface. +const ICON_BUTTON_FOCUS = 'focus-visible:ring-white/70 focus-visible:border-white/70'; + function groupMenuItems(items: MenuItem[]): { group: string; items: MenuItem[] }[] { const groups: { group: string; items: MenuItem[] }[] = []; const indexByGroup = new Map(); @@ -124,7 +131,7 @@ function SidebarShell({ children, menuKey, theme, headerSlot, footerNavSlot }: S size="icon-sm" onClick={() => setSidebarOpen(true)} aria-label={t(keys.ui.sidebar.open)} - className="text-sidebar-icon hover:text-white hover:bg-white/10" + className={`text-sidebar-icon hover:text-white hover:bg-white/10 ${ICON_BUTTON_FOCUS}`} >