Skip to content

fix(datasets): route Inertia CUD through view endpoints + SM018 diagnostic - #48

Merged
antosubash merged 4 commits into
mainfrom
feature/gracious-meitner-439ba1
Apr 21, 2026
Merged

antosubash merged 4 commits into
mainfrom
feature/gracious-meitner-439ba1

Conversation

@antosubash

Copy link
Copy Markdown
Owner

Summary

  • Bug: Inertia's client-side router.post/patch/delete in Datasets pages targeted /api/datasets/*, which returns plain JSON. Inertia rejects non-Inertia responses with "All Inertia requests must receive a valid Inertia response" — so Create, Edit, and Browse's delete button were all broken in the same way.
  • Fix: Added view-layer CUD endpoints at /datasets/, /datasets/{id} (PATCH), /datasets/{id} (DELETE) that return RedirectResponse(303, constants.REDIRECT_BROWSE) — which Inertia follows correctly. Frontend now posts to these view routes. Extracted the upload body into a shared perform_upload() helper so both the REST API and the Inertia view endpoint reuse one implementation.
  • Prevention: New SM018 module diagnostic that regex-scans pages/**/*.tsx for router.{post,patch,put,delete}('/api/...') across every module and warns at make doctor / dev boot. Catches this class of bug before it ships.

Changes

Datasets module

  • endpoints/api.py — extracted perform_upload(...); the existing POST /api/datasets/ now delegates to it (unchanged behavior for REST clients).
  • endpoints/views.py — new POST /, PATCH /{id}, DELETE /{id} that redirect to the browse page. All three use constants.REDIRECT_BROWSE (already defined for this purpose).
  • pages/{Create,Edit,Browse}.tsx — target /datasets/* instead of /api/datasets/*. No props / UI changes.
  • tests/test_datasets.py — new TestDatasetsViews class with 6 tests: 303 redirect + row-landed on create, unknown-kind rejection, update round-trip, delete removes row, and 404 paths for update/delete.

Framework diagnostic (SM018)

  • diagnostics/_inertia_api.py (new) — pre-compiled regex that flags router.{post,patch,put,delete}('/api/...') in TSX with file:line, suggests using a view endpoint that redirects, and skips router.get (non-mutation) and non-string URLs.
  • diagnostics/_module.py — invokes the check per-module alongside the existing orphan-page/phantom-render checks.
  • tests/test_module_diagnostics.pyTestSM018InertiaApiCalls with 5 tests: fires on the anti-pattern for all four verbs, silent on view-path / router.get / no-pages-dir.
  • CLAUDE.md — documented SM018 in the diagnostic-codes table.

Test plan

  • uv run pytest modules/datasets/tests/ — 45 passed
  • uv run pytest framework/core/tests/test_module_diagnostics.py — 9 passed (4 existing SM017 + 5 new SM018)
  • make doctor — 0 errors, 0 SM018 warnings across all modules after the frontend fix
  • Manually confirmed SM018 fires by temporarily reintroducing router.post('/api/...') in a tmp page
  • Ruff + ruff format clean on all touched files
  • Ty type-check clean on modules/datasets/datasets/

Reviewer notes

  • The REST API endpoints under /api/datasets/* are untouched and still return JSON for external consumers (tests covering them still pass).
  • perform_upload takes 9 positional args — this is deliberate. FastAPI Depends can't be applied inside a plain helper, and packaging the args into a dataclass would just shift boilerplate without reducing coupling.
  • delete_view mirrors the pre-existing double-fetch pattern in delete_dataset (get-by-id then service.delete which re-fetches internally). Fixing that properly requires a service-signature change that affects both routes; left as a separate concern.
  • SM018 uses text regex rather than an AST walk — TSX isn't parseable by Python's ast module, and the pattern is unambiguous enough that string matching catches it reliably. Same tradeoff as the existing _js_workspace.py check.

…M018 diagnostic

Inertia's router rejects plain JSON responses, so the datasets Create/Edit/Browse
pages broke when posting to /api/datasets/*. Added view-layer POST/PATCH/DELETE
endpoints that return RedirectResponse(303) and repointed the frontend. The upload
body is factored into a shared `perform_upload` helper so both the JSON API and
the Inertia view reuse it.

Added SM018: a module diagnostic that regex-scans every module's pages/**/*.tsx
for `router.{post,patch,put,delete}('/api/...')` and warns. Prevents this class
of bug from recurring in other modules.

- views.py: new POST / + PATCH /{id} + DELETE /{id}, all redirect to
  constants.REDIRECT_BROWSE
- api.py: extract perform_upload(); existing /api/datasets/ endpoint now delegates
- Create/Edit/Browse.tsx: target /datasets/* instead of /api/datasets/*
- test_datasets.py: 6 new tests covering the CUD view redirects + 404 paths
- _inertia_api.py (new): SM018 check + 5 tests
- CLAUDE.md: document SM018
…e cap)

Adding the TestDatasetsViews class pushed test_datasets.py to 370 lines. Move
the CUD view tests into test_views.py, split into TestUploadView /
TestUpdateView / TestDeleteView for readability. Main file drops back to 285
lines; new file is 95 lines.
Pytest collision: modules/users/tests/test_views.py already uses that basename,
and pytest requires unique test module basenames across the suite. Rename to
test_datasets_views.py. Also apply ruff format.
@antosubash
antosubash merged commit 273c3d1 into main Apr 21, 2026
10 checks passed
antosubash added a commit that referenced this pull request Apr 21, 2026
Resolves three conflicts with main's db-backed-settings feature (PR #47)
and datasets view-routing fix (PR #48):

- modules/settings/pyproject.toml: kept both new blocks added on either
  side — [project.scripts] sm-settings entry (from main) AND [project.urls]
  metadata (from this branch).
- modules/settings/settings/pages/Modules.tsx: accepted main's deletion
  (replaced by ModulesEdit.tsx under the new admin UI). Our only edit
  here was the mechanical @simple-module → @simple-module-py scope
  rename, which no longer applies to a deleted file.
- modules/settings/settings/pages/ModulesEdit.tsx (new in main): updated
  its @simple-module/* imports to @simple-module-py/* to stay consistent
  with the npm scope rename on this branch.

Post-merge validators green:
  - scripts/check_metadata.py: All package metadata OK.
  - scripts/check_readmes.py: All READMEs OK.
  - scripts/bump_version.py 0.0.1 --check: All 17 packages at 0.0.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant