From 353d65c7eeb8e30fbf2a250b3e388a5943b3828c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Wed, 6 May 2026 13:26:29 +0200 Subject: [PATCH] fix: address fresh-scaffold + background_tasks papercuts (#116, #119, #123, #124, #126, #127, #128, #129) Pulls together the post-0.0.8 follow-up fixes: - #129: `repo_root_from_client_app` now walks up to the nearest workspace `package.json` (falling back to the nearest plain `package.json`) instead of hardcoding two levels up. This unblocks `sm host sync-js-deps` in the flat `sm new` scaffold layout where `client_app/` sits one level under the project root. - #123: scaffolded `vite.config.ts` now also seeds the optimizeDeps walk with each wheel-installed module's `package.json`. Without it, deps a module imports directly (`sonner`, `lucide-react`, ...) were never pre-bundled and Node module resolution from inside `.venv/site-packages` could not reach `host/client_app/node_modules`. - #124: `background_tasks.sync_db` now exposes `set_database_url(url)`, called from `BackgroundTasksModule.on_startup` with the host's resolved `settings.database_url`. The old `os.environ["SM_DATABASE_URL"]` read silently fell back to SQLite in the web process (uvicorn doesn't load `.env` into the env). Falls back to the env var when no override is set so the worker's existing flow keeps working. - Adds regression tests for #126, #127, #128 (already fixed in source but not previously asserted in CI), #129 layout-resolution edges, and the new `set_database_url` URL-resolution contract. --- .../templates/host/client_app/vite.config.ts | 13 +++- .../cli/tests/test_cli_new_regressions.py | 61 +++++++++++++++++++ .../hosting/simple_module_hosting/manifest.py | 29 ++++++++- framework/hosting/tests/test_manifest.py | 60 ++++++++++++++++++ .../background_tasks/module.py | 6 ++ .../background_tasks/sync_db.py | 34 ++++++++++- .../background_tasks/tests/test_sync_db.py | 60 ++++++++++++++++++ 7 files changed, 257 insertions(+), 6 deletions(-) create mode 100644 framework/cli/tests/test_cli_new_regressions.py create mode 100644 framework/hosting/tests/test_manifest.py create mode 100644 modules/background_tasks/tests/test_sync_db.py diff --git a/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts b/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts index 6fb200e5..ccc0ea48 100644 --- a/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts +++ b/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts @@ -35,14 +35,25 @@ const fsRoot = findNodeModulesRoot(__dirname); // dev server can read files outside the host root, and in // optimizeDeps.entries so its dependency scanner discovers bare imports // from wheel-installed pages and pre-bundles them. +// +// We also collect each module's package.json (one level up from pages/, +// where Hatch's force-include drops it) so the dependency walk in +// `collectOptimizeIncludes` reaches packages a wheel-installed page imports +// directly (`sonner`, `lucide-react`, ...). Without this seed, Vite's +// pre-bundler never sees those bare specifiers and Node module resolution +// walks up from inside .venv/site-packages — never reaching +// host/client_app/node_modules. const manifestPath = path.resolve(__dirname, 'modules.manifest.json'); const moduleFsAllow: string[] = []; const moduleOptimizeEntries: string[] = []; +const modulePkgJsonPaths: string[] = []; if (fs.existsSync(manifestPath)) { const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf-8')) as Record; for (const pagesDir of Object.values(manifest)) { moduleFsAllow.push(path.dirname(pagesDir)); moduleOptimizeEntries.push(path.join(pagesDir, '**/*.tsx')); + const modulePkgJson = path.join(path.dirname(pagesDir), 'package.json'); + if (fs.existsSync(modulePkgJson)) modulePkgJsonPaths.push(modulePkgJson); } } @@ -99,7 +110,7 @@ function collectOptimizeIncludes(): string[] { 'use-sync-external-store/shim/with-selector', ]); const visited = new Set(); - const queue: string[] = [path.join(__dirname, 'package.json')]; + const queue: string[] = [path.join(__dirname, 'package.json'), ...modulePkgJsonPaths]; while (queue.length > 0) { const pkgJsonPath = queue.shift(); if (!pkgJsonPath || visited.has(pkgJsonPath)) continue; diff --git a/framework/cli/tests/test_cli_new_regressions.py b/framework/cli/tests/test_cli_new_regressions.py new file mode 100644 index 00000000..cdebf268 --- /dev/null +++ b/framework/cli/tests/test_cli_new_regressions.py @@ -0,0 +1,61 @@ +"""Regression tests for ``sm new`` scaffold bugs filed against released wheels. + +Each test pins a specific issue's repro so the bug can't sneak back in +without a CI failure pointing at the fix. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +from simple_module_cli.cli import app +from typer.testing import CliRunner + + +def test_sm_new_flat_pins_inertia_react_to_v2(tmp_path: Path) -> None: + """Issue #128: flat scaffold's root package.json must peer-match + @simple-module-py/ui's ``@inertiajs/react: ^2.0.0`` peer dep.""" + runner = CliRunner() + target = tmp_path / "demo" + runner.invoke( + app, + ["new", "demo", "--yes", "--flat", "--no-install", "--dest", str(target)], + ) + data = json.loads((target / "package.json").read_text()) + inertia = data.get("dependencies", {}).get("@inertiajs/react") + assert inertia is not None + assert inertia.startswith("^2."), f"expected ^2.x, got {inertia!r}" + + +def test_sm_new_sample_module_pins_match_framework_version(tmp_path: Path) -> None: + """Issue #126/#119: sample hello module must pin framework deps to the + actual published framework version, not the future ``>=1.0,<2.0`` range.""" + from importlib.metadata import version + + expected = version("simple_module_cli") + runner = CliRunner() + target = tmp_path / "demo" + runner.invoke( + app, + ["new", "demo", "--yes", "--db", "sqlite", "--no-install", "--dest", str(target)], + ) + text = (target / "modules" / "hello" / "pyproject.toml").read_text() + for pkg in ("simple_module_core", "simple_module_db", "simple_module_hosting"): + assert f"{pkg}=={expected}" in text, f"{pkg} should be pinned to =={expected}" + # Dev pin: simple_module_test was the original >=0.1,<1.0 unsatisfiable case. + assert f"simple_module_test=={expected}" in text + + +def test_sm_new_sample_module_seeds_static_dist_placeholder(tmp_path: Path) -> None: + """Issue #127: hatch's force-include resolves at uv-sync time. A fresh + scaffold must ship an empty ``static/dist/`` so the build doesn't fail + before vite has had a chance to run.""" + runner = CliRunner() + target = tmp_path / "demo" + runner.invoke( + app, + ["new", "demo", "--yes", "--db", "sqlite", "--no-install", "--dest", str(target)], + ) + static_dist = target / "modules" / "hello" / "hello" / "static" / "dist" + assert static_dist.is_dir(), "static/dist/ must exist for hatch force-include" diff --git a/framework/hosting/simple_module_hosting/manifest.py b/framework/hosting/simple_module_hosting/manifest.py index c4fb8f6d..f28bbdfe 100644 --- a/framework/hosting/simple_module_hosting/manifest.py +++ b/framework/hosting/simple_module_hosting/manifest.py @@ -45,13 +45,38 @@ def repo_root_from_client_app(client_app_dir: Path) -> Path: - """Repo root is two levels above ``host/client_app/``. + """Locate the workspace/repo root that contains ``client_app_dir``. Both ``write_module_pages_manifest`` and the ``sync-js-deps`` CLI derive the workspace root from the host's client_app directory. Centralized here so the heuristic lives in exactly one place. + + Walks up looking for the nearest ``package.json`` that declares + ``workspaces`` (the workspace root npm uses), falling back to any + ``package.json``. This handles both the framework repo's + ``host/client_app/`` layout and the flat ``sm new`` scaffold's + ``client_app/`` layout (where the parent IS the workspace root). """ - return client_app_dir.resolve().parent.parent + here = client_app_dir.resolve() + fallback: Path | None = None + for parent in (here.parent, *here.parents): + pkg = parent / "package.json" + if not pkg.is_file(): + continue + if fallback is None: + fallback = parent + try: + data = json.loads(pkg.read_text(encoding="utf-8")) + except json.JSONDecodeError: + continue + if "workspaces" in data: + return parent + if fallback is not None: + return fallback + # No package.json found anywhere on the way up — preserve the legacy + # two-levels-up shape so framework-internal callers still get a valid + # path even before any npm setup has happened. + return here.parent.parent def compute_module_pages(modules: Sequence[ModuleBase]) -> dict[str, Path]: diff --git a/framework/hosting/tests/test_manifest.py b/framework/hosting/tests/test_manifest.py new file mode 100644 index 00000000..821ae0cd --- /dev/null +++ b/framework/hosting/tests/test_manifest.py @@ -0,0 +1,60 @@ +"""Tests for ``simple_module_hosting.manifest`` helpers.""" + +from __future__ import annotations + +import json +from pathlib import Path + +from simple_module_hosting.manifest import repo_root_from_client_app + + +def test_repo_root_finds_workspace_root_in_framework_layout(tmp_path: Path) -> None: + """``host/client_app/`` shape: walk up to the workspace package.json.""" + workspace = tmp_path / "ws" + (workspace / "host" / "client_app").mkdir(parents=True) + (workspace / "package.json").write_text( + json.dumps({"name": "ws", "workspaces": ["host/client_app", "modules/*"]}) + ) + (workspace / "host" / "package.json").write_text(json.dumps({"name": "ws-host"})) + + assert repo_root_from_client_app(workspace / "host" / "client_app") == workspace.resolve() + + +def test_repo_root_finds_root_in_flat_scaffold_layout(tmp_path: Path) -> None: + """Flat scaffold: ``client_app/`` directly under the host root.""" + host = tmp_path / "my-app" + (host / "client_app").mkdir(parents=True) + (host / "package.json").write_text(json.dumps({"name": "my-app"})) + + assert repo_root_from_client_app(host / "client_app") == host.resolve() + + +def test_repo_root_prefers_workspace_package_json_over_nearer_one(tmp_path: Path) -> None: + """When a non-workspace package.json sits between client_app and the + workspace root, the workspace one wins so npm install runs at the right cwd.""" + workspace = tmp_path / "ws" + (workspace / "host" / "client_app").mkdir(parents=True) + (workspace / "package.json").write_text( + json.dumps({"name": "ws", "workspaces": ["host/client_app"]}) + ) + (workspace / "host" / "package.json").write_text(json.dumps({"name": "ws-host"})) + + assert repo_root_from_client_app(workspace / "host" / "client_app") == workspace.resolve() + + +def test_repo_root_falls_back_to_nearest_package_json(tmp_path: Path) -> None: + """No workspaces field anywhere — settle on the nearest package.json found.""" + root = tmp_path / "root" + (root / "client_app").mkdir(parents=True) + (root / "package.json").write_text(json.dumps({"name": "root"})) + + assert repo_root_from_client_app(root / "client_app") == root.resolve() + + +def test_repo_root_falls_back_to_two_levels_up_if_no_package_json(tmp_path: Path) -> None: + """No package.json exists yet — preserve the legacy two-up behaviour so + framework-internal callers still get a path before any npm setup.""" + deep = tmp_path / "outer" / "inner" / "client_app" + deep.mkdir(parents=True) + + assert repo_root_from_client_app(deep) == (tmp_path / "outer").resolve() diff --git a/modules/background_tasks/background_tasks/module.py b/modules/background_tasks/background_tasks/module.py index d73edb53..70cf2bae 100644 --- a/modules/background_tasks/background_tasks/module.py +++ b/modules/background_tasks/background_tasks/module.py @@ -96,8 +96,14 @@ async def on_startup(self, app: FastAPI) -> None: from background_tasks.celery_app import build_celery from background_tasks.signals import bind_event_bus + from background_tasks.sync_db import set_database_url services = app.state.background_tasks + # Pin the sync engine to the same URL the host's async settings + # resolved — pydantic-settings reads ``.env`` but never propagates + # to ``os.environ``, so signals would otherwise fall back to the + # SQLite default and silently drop ``TaskExecution`` rows. + set_database_url(app.state.sm.settings.database_url) # build_celery imports `signals` for side effects and runs # `autodiscover_tasks` across every installed module. services.celery = build_celery(services.settings) diff --git a/modules/background_tasks/background_tasks/sync_db.py b/modules/background_tasks/background_tasks/sync_db.py index d578ee5c..d087c495 100644 --- a/modules/background_tasks/background_tasks/sync_db.py +++ b/modules/background_tasks/background_tasks/sync_db.py @@ -26,6 +26,7 @@ _engine: Engine | None = None _session_factory: sessionmaker[Session] | None = None +_url_override: str | None = None def _sync_url(async_url: str) -> str: @@ -37,9 +38,35 @@ def _sync_url(async_url: str) -> str: return async_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") +def set_database_url(url: str | None) -> None: + """Pin the URL used to build the sync engine. + + The web process loads ``.env`` via pydantic-settings, but those values + never land in ``os.environ`` — so reading ``SM_DATABASE_URL`` directly + from the env can silently drop us back to the SQLite default while the + rest of the app uses Postgres. ``BackgroundTasksModule.on_startup`` + calls this with the resolved ``settings.database_url`` so signals use + the same DB the app is on. Pass ``None`` to clear the override (used in + tests + on shutdown). + """ + global _url_override, _engine, _session_factory + if _url_override == url: + return + _url_override = url + if _engine is not None: + _engine.dispose() + _engine = None + _session_factory = None + + +def _resolve_url() -> str: + if _url_override is not None: + return _url_override + return os.environ.get("SM_DATABASE_URL", "sqlite:///./app.db") + + def _build_engine() -> Engine: - url = os.environ.get("SM_DATABASE_URL", "sqlite:///./app.db") - sync_url = _sync_url(url) + sync_url = _sync_url(_resolve_url()) # Small pool — signals fire sequentially per worker process. return create_engine(sync_url, pool_pre_ping=True, pool_size=2, max_overflow=3) @@ -60,11 +87,12 @@ def dispose_sync_engine() -> None: restarts within one process (test runners, uvicorn dev reload) don't accumulate engines against the old DB URL. """ - global _engine, _session_factory + global _engine, _session_factory, _url_override if _engine is not None: _engine.dispose() _engine = None _session_factory = None + _url_override = None @contextmanager diff --git a/modules/background_tasks/tests/test_sync_db.py b/modules/background_tasks/tests/test_sync_db.py new file mode 100644 index 00000000..f70b5763 --- /dev/null +++ b/modules/background_tasks/tests/test_sync_db.py @@ -0,0 +1,60 @@ +"""Tests for the sync session factory's URL resolution. + +The web process loads ``.env`` via pydantic-settings but never propagates the +result to ``os.environ``. ``set_database_url`` lets the module's +``on_startup`` pin the sync engine to whatever the host settings resolved, +so signals don't silently fall back to SQLite while the rest of the app +talks to Postgres. +""" + +from __future__ import annotations + +from collections.abc import Iterator + +import pytest +from background_tasks import sync_db + + +@pytest.fixture(autouse=True) +def _reset_sync_db() -> Iterator[None]: + sync_db.dispose_sync_engine() + yield + sync_db.dispose_sync_engine() + + +def test_resolve_url_falls_back_to_env(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("SM_DATABASE_URL", "sqlite:///./from-env.db") + sync_db.set_database_url(None) + + assert sync_db._resolve_url() == "sqlite:///./from-env.db" + + +def test_set_database_url_overrides_env(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("SM_DATABASE_URL", "sqlite:///./from-env.db") + sync_db.set_database_url("postgresql+asyncpg://u:p@h/db") + + assert sync_db._resolve_url() == "postgresql+asyncpg://u:p@h/db" + + +def test_set_database_url_resets_engine_when_url_changes(tmp_path) -> None: + db_a = tmp_path / "a.db" + db_b = tmp_path / "b.db" + sync_db.set_database_url(f"sqlite:///{db_a}") + factory_a = sync_db.get_sync_session_factory() + sync_db.set_database_url(f"sqlite:///{db_a}") + factory_a_again = sync_db.get_sync_session_factory() + assert factory_a is factory_a_again, "same URL must reuse the cached factory" + + sync_db.set_database_url(f"sqlite:///{db_b}") + factory_b = sync_db.get_sync_session_factory() + assert factory_b is not factory_a, "URL change must dispose the old engine" + + +def test_dispose_sync_engine_clears_url_override(tmp_path) -> None: + url = f"sqlite:///{tmp_path / 'pinned.db'}" + sync_db.set_database_url(url) + assert sync_db._url_override == url + + sync_db.dispose_sync_engine() + + assert sync_db._url_override is None