From ac94cdcce943e4f468e156f14cec759ca0d9dbee Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 2 Oct 2026 21:20:05 +0200 Subject: [PATCH] docs(12.2): add code review report --- .../12.2-REVIEW.md | 270 ++++++++++++++++++ 1 file changed, 270 insertions(+) create mode 100644 .planning/phases/12.2-admin-form-fields-date-file-upload-relation-editing-with-def/12.2-REVIEW.md diff --git a/.planning/phases/12.2-admin-form-fields-date-file-upload-relation-editing-with-def/12.2-REVIEW.md b/.planning/phases/12.2-admin-form-fields-date-file-upload-relation-editing-with-def/12.2-REVIEW.md new file mode 100644 index 0000000..7ede179 --- /dev/null +++ b/.planning/phases/12.2-admin-form-fields-date-file-upload-relation-editing-with-def/12.2-REVIEW.md @@ -0,0 +1,270 @@ +--- +phase: 12.2-admin-form-fields-date-file-upload-relation-editing-with-def +reviewed: 2026-10-02T19:12:38Z +depth: standard +files_reviewed: 137 +files_reviewed_list: + - ../fonoteka.go/parity/migrate_test.go + - ../fonoteka.go/parity/schema_diff_test.go + - admin/openapi/admin.json + - admin/package.json + - admin/src/api/client.ts + - admin/src/api/files.ts + - admin/src/api/schema.d.ts + - admin/src/api/types.ts + - admin/src/app/dateFormat.ts + - admin/src/app/sessionKey.ts + - admin/src/components/form/fields/DatepickerField.vue + - admin/src/components/form/fields/FileCaptionModal.vue + - admin/src/components/form/fields/FileuploadField.vue + - admin/src/components/form/formContext.ts + - admin/src/components/form/registry.ts + - admin/src/components/list/CellValue.vue + - admin/src/components/list/DataTable.vue + - admin/src/components/relation/RelationChildModal.vue + - admin/src/components/relation/RelationManager.vue + - admin/src/components/relation/RelationPickerModal.vue + - admin/src/components/relation/RelationPivotModal.vue + - admin/src/components/ui/ConfirmDialog.vue + - admin/src/components/ui/confirm.ts + - admin/src/views/FormView.vue + - admin/tests/app/dateFormat.test.ts + - admin/tests/app/sessionKey.test.ts + - admin/tests/fixtures/deferred.files.json + - admin/tests/fixtures/deferred.form-schema.json + - admin/tests/fixtures/deferred.relation-schema.json + - admin/tests/fixtures/typed.ts + - admin/tests/fixtures/widgets.relation-schema.json + - admin/tests/form/DatepickerField.test.ts + - admin/tests/form/FileuploadField.test.ts + - admin/tests/form/registry.test.ts + - admin/tests/list/CellValue.test.ts + - admin/tests/relation/RelationChildModal.test.ts + - admin/tests/relation/RelationManager.test.ts + - admin/tests/relation/RelationPickerModal.test.ts + - admin/tests/relation/RelationPivotModal.test.ts + - admin/tests/smoke/deferred.smoke.test.ts + - docs/backend/admin-spa.md + - docs/backend/forms.md + - docs/backend/lists-and-filters.md + - docs/backend/relation-manager.md + - docs/console/setup-and-maintenance.md + - docs/database/attachments.md + - docs/database/casts-and-validation.md + - docs/database/models.md + - docs/plugins/scheduling.md + - internal/tools/swagger2openapi/main.go + - internal/tools/swagger2openapi/main_test.go + - modules/cabana/README.md + - modules/cabana/admin_openapi.go + - modules/cabana/auth.go + - modules/cabana/contracts.go + - modules/cabana/crud.go + - modules/cabana/datepicker_smoke_test.go + - modules/cabana/datepicker_test.go + - modules/cabana/deferred.go + - modules/cabana/deferred_commit_test.go + - modules/cabana/example_relation_test.go + - modules/cabana/field_date.go + - modules/cabana/field_file.go + - modules/cabana/fileupload_smoke_test.go + - modules/cabana/fileupload_test.go + - modules/cabana/form_schema.go + - modules/cabana/http.go + - modules/cabana/list_schema.go + - modules/cabana/messages.go + - modules/cabana/openapi_conformance_test.go + - modules/cabana/phase10_coverage_test.go + - modules/cabana/phase10_csrf_test.go + - modules/cabana/phase122_fixture_test.go + - modules/cabana/protected_file_test.go + - modules/cabana/registry.go + - modules/cabana/relation.go + - modules/cabana/relation_child.go + - modules/cabana/relation_child_scope_test.go + - modules/cabana/relation_child_smoke_test.go + - modules/cabana/relation_child_test.go + - modules/cabana/relation_form.go + - modules/cabana/schema.go + - modules/cabana/schema_types.go + - modules/cabana/security_coverage_test.go + - modules/cabana/settings.go + - modules/cabana/testdata/deferred/controllers/gadgets/config_form.yaml + - modules/cabana/testdata/deferred/controllers/gadgets/config_list.yaml + - modules/cabana/testdata/deferred/controllers/gadgets/config_relation.yaml + - modules/cabana/testdata/deferred/controllers/locked/config_form.yaml + - modules/cabana/testdata/deferred/controllers/locked/config_list.yaml + - modules/cabana/testdata/deferred/controllers/locked/config_relation.yaml + - modules/cabana/testdata/deferred/models/gadget/columns.yaml + - modules/cabana/testdata/deferred/models/gadget/fields.yaml + - modules/cabana/testdata/deferred/models/gadget/locked_fields.yaml + - modules/cabana/testdata/deferred/models/member/fields.yaml + - modules/cabana/testdata/deferred/models/member/pivot_fields.yaml + - modules/cabana/testdata/deferred/models/part/fields.yaml + - modules/conga/README.md + - modules/conga/schedule_test.go + - modules/conga/scheduler.go + - modules/lagoon/README.md + - modules/lagoon/attach/file.go + - modules/lagoon/attach/file_test.go + - modules/lagoon/attach/guard.go + - modules/lagoon/attach/guard_test.go + - modules/lagoon/attach/relation.go + - modules/lagoon/attach/store.go + - modules/lagoon/attach/store_test.go + - modules/lagoon/attach/thumb.go + - modules/lagoon/attach/url_test.go + - modules/lagoon/commands.go + - modules/lagoon/date.go + - modules/lagoon/date_test.go + - modules/lagoon/deferred.go + - modules/lagoon/deferred_migrations.go + - modules/lagoon/deferred_test.go + - modules/lagoon/fill.go + - modules/lagoon/fill_test.go + - modules/lagoon/migrations.go + - modules/lagoon/migrations_test.go + - modules/lagoon/purge.go + - modules/lagoon/purge_test.go + - modules/lagoon/schedule.go + - modules/lagoon/validate.go + - modules/lagoon/validate_request_test.go + - modules/lagoon/validate_rules.go + - modules/lagoon/validate_rules_test.go + - modules/lagoon/validate_test.go + - modules/pact/README.md + - modules/pact/capabilities.go + - modules/pact/capabilities_test.go + - modules/phrasebook/backend/lang/en/lang.yaml + - modules/phrasebook/backend/lang/pl/lang.yaml + - modules/tide/multipart_test.go + - modules/tide/normalize_upload_test.go + - scripts/check-phase12.2.sh + - scripts/check-phase12.sh +findings: + critical: 3 + warning: 4 + info: 0 + total: 7 +status: issues_found +--- + +# Phase 12.2: Code Review Report + +**Reviewed:** 2026-10-02T19:12:38Z +**Depth:** standard +**Files Reviewed:** 137 +**Status:** issues_found + +## Summary + +The phase adds substantial form, deferred-binding, file, date, and relation functionality, but it is not ready to ship. Three blocking defects can lose or silently misapply upload state or permit memory-exhaustion requests, and four additional correctness defects affect upload limits, datetime validation, file ordering, and pending pivot data. + +The admin suite passed (60 files, 768 tests), `vue-tsc --noEmit` passed, and both phase gate scripts passed `bash -n`. Targeted Go tests were attempted: `modules/pact` and `internal/tools/swagger2openapi` passed, while other packages could not complete in this environment because Docker/listener access was denied and the linker reported a disk-quota error. Those infrastructure failures are not counted as findings. + +## Narrative Findings (AI reviewer) + +### Critical Issues + +### CR-01: Form saves can commit before an in-flight upload reaches the deferred session + +**Classification:** BLOCKER + +**Files:** + +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/FileuploadField.vue:375-424` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/formContext.ts:31-49` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/FormView.vue:205-214` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/FormView.vue:238-297` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/relation/RelationChildModal.vue:167-193` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/relation/RelationChildModal.vue:321-359` + +**Issue:** Upload activity is local to `FileuploadField`; the shared `FormSession` exposes dirty state but no active-upload state. Both the record form and relation-child form therefore allow submit while `uploadWithProgress` is still running. The save commits only bindings already present at that instant, clears `pendingChanges`, and can navigate away. An upload completing just afterwards creates a deferred binding that was not part of the successful save. On create/save-and-close this appears as a successful save with the selected file missing and leaves an invisible pending attachment until purge; on update it silently requires another save that the user is never told to perform. + +**Fix:** Extend `FormSession` with upload registration (for example `beginUpload(): () => void` plus a readonly active counter). Register before starting the XHR and release in `finally`. Disable or await both record and child submission while the counter is non-zero, and prevent navigation until all uploads have reached a known terminal state. Add a test whose upload promise is held pending while Save is clicked. + +### CR-02: Aborted or network-failed uploads have no idempotency or reconciliation path + +**Classification:** BLOCKER + +**Files:** + +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/api/files.ts:128-193` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/FileuploadField.vue:387-434` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_file.go:619-706` + +**Issue:** Once an upload request has reached the server, an XHR abort or lost response does not prove the transaction failed. The server may already have stored the blob, file row, and deferred bind, but the client discards the item on abort or marks it retryable on a network error. A later form save can then attach a file the user believes was cancelled, while Retry posts a second independent upload (and can attach both on an `attachMany` field). There is no client request identifier, unique server constraint, or list reconciliation to distinguish “not stored” from “stored but response lost.” This is a data-integrity and cancellation-semantics failure. + +**Fix:** Generate a stable client upload ID per queued item, send it on every retry, persist it with a uniqueness constraint scoped to admin/session/field, and return the already-created item for duplicate requests. On abort or an ambiguous network failure, reconcile the field list by that ID and explicitly delete a server-side result before treating cancellation as complete. Cover response-loss-after-commit and abort-after-commit cases. + +### CR-03: Core CRUD, settings, and relation mutation JSON bodies are uncapped + +**Classification:** BLOCKER + +**Files:** + +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:51-53` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:467-480` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:585-594` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:751-805` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/crud.go:648-658` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:613-625` + +**Issue:** The service explicitly records that Surf does not apply body limits to raw routes, yet `decodeObject` and `decodeRelationMutation` decode `r.Body` directly. Settings update, record create/update, and relation link/unlink can therefore stream arbitrarily large JSON into `encoding/json`, allowing any authenticated, CSRF-valid admin request (or a compromised admin session) to consume unbounded memory. The new child and file JSON routes already demonstrate the required capped pattern with `http.MaxBytesReader`, so the protection is inconsistent across the same API. + +**Fix:** Replace these helpers with service methods that accept `http.ResponseWriter`, wrap `r.Body` in `http.MaxBytesReader(w, r.Body, s.jsonCap())`, reject trailing JSON consistently, and map `*http.MaxBytesError` to 413. Prefer a single raw-route JSON decoder so every current and future handler inherits the cap. Add oversized-body tests for settings, create/update, and link/unlink. + +### Warnings + +### WR-01: A field may advertise a maximum file size that its request cap cannot carry + +**Classification:** WARNING + +**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_file.go:367-381,709-724` + +**Issue:** Boot validation accepts `maxFilesize == http.body_limits.upload_bytes`, but `uploadCap` caps the complete multipart request at `upload_bytes`. Multipart framing consumes part of that budget, so a file at (or near) the documented field maximum is rejected with 413 before the file-size validator can accept it. The same problem occurs whenever the gap between the two configured limits is smaller than the multipart overhead. Configuration that passes boot therefore cannot deliver the promised maximum. + +**Fix:** Define the two limits unambiguously. If `upload_bytes` is a file-content limit, cap the wire body at `upload_bytes + multipartOverhead` and keep the exact file limit in `attach.Store`. If it is a request-body limit, make boot reject `maxBytes + multipartOverhead > uploadBytes` and document the required headroom. Test equality and a gap smaller than 64 KiB. + +### WR-02: Datetime picker bounds use local days while the server validates UTC days + +**Classification:** WARNING + +**Files:** + +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/app/dateFormat.ts:272-290` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/DatepickerField.vue:66-85` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_date.go:270-345` + +**Issue:** For a normal `datetime`, the SPA constructs min/max at midnight/end-of-day in the browser's local time zone and compares the displayed local value to those bounds. The server deliberately compares the saved instant's UTC calendar date. In a positive-offset zone, for example, `2000-01-01 00:30 +01:00` is enabled and considered in range by the picker for `minDate: 2000-01-01`, but it emits `1999-12-31T23:30:00Z` and the server rejects it. Around both boundaries the UI can allow values the server refuses and disable values the server accepts. + +**Fix:** Make the SPA comparison use the same calendar basis as the server: convert non-`ignoreTimezone` min/max to UTC-day instant bounds before comparing, or change the server contract to validate the displayed local day and pass a trusted zone explicitly. Add tests in positive and negative offsets around midnight for both min and max. + +### WR-03: Concurrent reorder requests can overwrite the latest order or create a mixed order + +**Classification:** WARNING + +**Files:** + +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/FileuploadField.vue:481-524` +- `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_file.go:1247-1289` + +**Issue:** Debouncing limits requests within one burst but `sendOrder` does not serialize requests. It clears `orderBefore` before awaiting, so another drag can dispatch a second reorder while the first is in flight. Responses may complete out of order and leave the database in the older order. The server also reads without locking the whole visible set and updates each row separately, so concurrent transactions can interleave row updates into an order that matches neither request. A failure from an older request can additionally restore an obsolete client order over a newer successful move. + +**Fix:** Keep one reorder request in flight and retain only the latest pending ID snapshot; after completion, send that latest snapshot. On the server, lock all visible file rows before deriving and updating sort positions (or use a relation-scoped version/CAS). Ignore stale responses by generation and add a test with deliberately reversed promise completion. + +### WR-04: Pending pivot hydration discards type-conversion errors + +**Classification:** WARNING + +**File:** `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/relation_child.go:442-473,553-568` + +**Issue:** Both pending-pivot show and update explicitly discard `lagoon.Fill` errors while rebuilding the pivot model from its deferred envelope. If a deferred row is stale after a schema/type change or otherwise contains a value that no longer fits the pivot column, the API returns a successful response with that field silently replaced by its zero value. Update may then present a response inconsistent with the envelope that will later fail during parent commit. This hides corrupted/incompatible pending state and makes it difficult for the user to recover safely. + +**Fix:** Check the `lagoon.Fill` result in both paths and return `lifecycleFailure(cc, err)` (or a field-level 422 if recovery through editing is intended). Add a test with an out-of-range or wrong-type value in a deferred pivot envelope. + +--- + +_Reviewed: 2026-10-02T19:12:38Z_ +_Reviewer: the agent (gsd-code-reviewer)_ +_Depth: standard_