docs(16.2): add code review fix report
This commit is contained in:
@@ -0,0 +1,228 @@
|
||||
---
|
||||
phase: 16.2-plugin-provided-field-types-and-the-blog-editor-formwidget
|
||||
fixed_at: 2026-10-10T14:46:56Z
|
||||
review_path: .planning/phases/16.2-plugin-provided-field-types-and-the-blog-editor-formwidget/16.2-REVIEW.md
|
||||
iteration: 1
|
||||
findings_in_scope: 8
|
||||
fixed: 8
|
||||
skipped: 0
|
||||
status: all_fixed
|
||||
---
|
||||
|
||||
# Phase 16.2: Code Review Fix Report
|
||||
|
||||
**Fixed at:** 2026-10-10T14:46:56Z
|
||||
**Source review:** .planning/phases/16.2-plugin-provided-field-types-and-the-blog-editor-formwidget/16.2-REVIEW.md
|
||||
**Iteration:** 1
|
||||
|
||||
**Summary:**
|
||||
- Findings in scope: 8 (CR-01, WR-01, WR-02, WR-03, WR-04, WR-05, WR-06, IN-04, as the orchestrator chose)
|
||||
- Fixed: 8
|
||||
- Skipped: 0 in scope. WR-07, WR-08, IN-01, IN-02, IN-03, IN-05, IN-06, IN-07 and IN-08 are out of scope by decision. Each is listed under Skipped Issues.
|
||||
|
||||
The fixes span three repositories, and each fix is committed in the repository that owns the file:
|
||||
- summercms.go: `/media/nvme/dev/golem15/summercms.io/summercms/summercms.go`
|
||||
- sm-journal-plugin: `/media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin`
|
||||
- vue-journal-layer: no fix touched it.
|
||||
|
||||
## Fixed Issues
|
||||
|
||||
### CR-01: Blog Editor hides a locale's existing `content_html` when its `content` is blank
|
||||
|
||||
**Repository:** sm-journal-plugin
|
||||
**Files modified:** `classes/blog_editor.go`, `classes/blog_editor_test.go`, `controllers/posts_editor_smoke_test.go`
|
||||
**Commit:** 1593231
|
||||
**Applied fix:** `loadBlogHTML` now reads the `htmlColumn` attribute when the stored content is blank and serves it whenever it is not empty. This applies to every locale and to the host row. When it is empty or missing, the blank value is served as stored. FormatHTML never runs on a blank value. An untouched save echoes the served value, which cabana skips. An edit writes the edited HTML to `content`, and FormAfterUpdate writes it to that locale's `content_html`.
|
||||
**Tests:**
|
||||
- Blank or nil content is served as `content_html`, in a translation and on the host row.
|
||||
- Blank content with an empty or missing `content_html` is still served as `""`.
|
||||
- A failing read is the Load error.
|
||||
- A field without `htmlColumn` never reads.
|
||||
- The controller smoke test covers the whole flow: Load serves the API-written body, Decode accepts the edit, and FormAfterUpdate writes `content_html` for that locale.
|
||||
|
||||
### WR-01: Field upload route skips the controller's create/update capability check
|
||||
|
||||
**Repository:** summercms.go
|
||||
**Files modified:** `modules/cabana/field_type_upload.go`, `modules/cabana/field_type_test.go`, `modules/cabana/field_type_smoke_test.go`, `modules/cabana/README.md`, `docs/backend/field-types.md`
|
||||
**Commit:** 3158bb5
|
||||
**Applied fix:** `fieldUpload` now calls `s.operationDeclared(w, r, cc, op)` right after parsing `{id}`, before it reads the body. The operation is `create` for id 0 and `update` otherwise, as on the save routes.
|
||||
**Tests:**
|
||||
- New `TestFieldUpload` case: a controller with no `create` toolbar button answers 403 for id 0 and never calls Upload. The same field with the button answers 201.
|
||||
- The test fixtures for upload (`upFS`, `ftMLPlugin`) now declare `toolbar.buttons: [create]`.
|
||||
**Docs:** the README route row and the field-types page now describe the 403.
|
||||
|
||||
### WR-02: Stored upload objects are orphaned when the route refuses after Upload succeeded
|
||||
|
||||
**Repository:** summercms.go
|
||||
**Files modified:** `modules/cabana/field_type_upload.go`, `modules/cabana/field_type_test.go`, `modules/cabana/README.md`, `docs/backend/field-types.md`
|
||||
**Commit:** c532529
|
||||
**Applied fix:**
|
||||
- The route reads the whole multipart body with `ReadForm` before Upload runs. Files over 8 MiB (`fieldUploadMemory`) spool to a temporary file, which `RemoveAll` deletes.
|
||||
- `singleUploadFile` requires exactly one part: a file part named `file` with a non-blank file name. Only then is Upload called, with the opened file.
|
||||
- An extra part, a value part or an oversized body is refused (422 or 413) without calling Upload.
|
||||
- Two windows remain, and the docs now name them: a record transaction that fails after Upload returns, and an element that stops waiting. In both cases `Upload` should store under a name that is safe to leave behind.
|
||||
**Tests:**
|
||||
- New cases check that a second file part, a later field, a value part before or after the file, and an oversized body never call Upload.
|
||||
- A file larger than the in-memory threshold reaches Upload byte for byte.
|
||||
|
||||
### WR-03: Media key uniqueness is check-then-write
|
||||
|
||||
**Repository:** sm-journal-plugin
|
||||
**Files modified:** `classes/media_store.go`, `classes/media_store_test.go`
|
||||
**Commit:** 2c961b8
|
||||
**Applied fix:**
|
||||
- `StoreMedia` reserves the chosen key in an in-process set, per bucket, while it writes. `uniqueMediaKey` treats reserved keys as taken.
|
||||
- The write uses `blob.WriterOptions{IfNotExist: true}`. A `gcerrors.FailedPrecondition` moves the upload on to the next free name, for at most 10 attempts.
|
||||
- Both parts are needed: fileblob's `IfNotExist` is stat-then-rename under a per-writer mutex, so on its own it would not close the race within one process. The reservation closes it within the process, and `IfNotExist` covers other processes on backends that enforce it atomically, such as S3.
|
||||
**Tests (run with `-race`):**
|
||||
- 16 concurrent uploads of `image.png` get distinct keys and keep their own bytes, and no reservations are left behind.
|
||||
- A test hook (`mediaBeforeWrite`) simulates another process writing the chosen keys. The upload moves to `image-2.png` and leaves the other objects untouched.
|
||||
- When the upload loses every attempt, it gives up with an opaque error.
|
||||
|
||||
### WR-04: `LooksLikeHTML` treats markdown that starts with a tag as HTML
|
||||
|
||||
**Repository:** sm-journal-plugin
|
||||
**Files modified:** `classes/blog_editor.go`, `classes/blog_editor_test.go`, `controllers/posts_editor_smoke_test.go`
|
||||
**Commit:** 5af746f
|
||||
**Status:** fixed: requires human verification (this changes a heuristic)
|
||||
**Applied fix:**
|
||||
- A value that starts with `<` is served as it is only when one of these holds:
|
||||
- `content_html` is empty or missing, or cannot be read (no translation writer).
|
||||
- `content_html` equals the value, ignoring white space at either end.
|
||||
- `content_html` equals `SanitizeHTML(value)`, ignoring white space at either end.
|
||||
- Otherwise the value is markdown that starts with a tag, and `content_html` is served.
|
||||
- `sameStoredHTML` holds the comparison.
|
||||
**Why `FormatHTML(value)` is not a match:** that render also matches markdown which starts with `<img>`, so counting it would bring the bug back.
|
||||
**Side effect:** an imported HTML post whose `content_html` differs from its `content` (for example, Winter's Markdown::parse split an HTML block) now opens with `content_html`. That is what the site renders, so an untouched save still skips and an edit writes both columns from it.
|
||||
**Tests:**
|
||||
- The HTML-verbatim cases now cover a `content_html` that is missing, empty, equal, sanitized, trimmed, or both sanitized and trimmed.
|
||||
- New fixtures cover markdown that starts with `<img>`, `<div>` and `<!-- comment -->`. Each is served as its `content_html`, with no `##` or `**` markers.
|
||||
- A read error on an HTML value is the Load error.
|
||||
- A controller smoke case covers the `<img>` hero.
|
||||
|
||||
### WR-05: Attribute preservation lets `id`, `name`, `style`, `srcset` into the live admin DOM
|
||||
|
||||
**Repository:** sm-journal-plugin
|
||||
**Files modified:** `assets/src/blogeditor/extensions.ts`, `assets/dist/blogeditor.js`, `tests/blogeditor/schema.test.ts`, `tests/blogeditor/smoke.test.ts`, `README.md`
|
||||
**Commit:** f1e6c11
|
||||
**Applied fix:**
|
||||
- `captureAttributes` and `renderCaptured` now use the conservative allowlist from the orchestrator (`KEPT_ATTRIBUTES`, `isKeptAttribute`): `class`, `title`, `alt`, `width`, `height`, `colspan`, `rowspan`, `start`, `rel`, `target` and `data-*`.
|
||||
- `href` and `src` stay modelled on the link and image nodes, which check them.
|
||||
- Every other attribute is never captured, so it is never rendered into the ProseMirror view. This covers `id`, `name`, `style`, `srcset`, `background`, `form`, `contenteditable`, `popover`, `itemprop`, `dir`, `lang` and `on*`. Such attributes drop out of a block once that block is edited.
|
||||
- A `span` is kept as a mark only when it has a kept attribute.
|
||||
- Untouched documents and untouched blocks still return their stored bytes (D-03).
|
||||
**Tests:**
|
||||
- The schema tests use view-safe attributes for the "keeps" cases.
|
||||
- New table cases drop `id`, `name`, `style`, `srcset`, `loading`, `background`, `form`/`contenteditable`/`popover`, `itemprop`/`dir`/`lang` and a cell's `style`.
|
||||
- A live-DOM test asserts that no stored `id`, `name`, `srcset`, `background` or style value reaches `.ProseMirror`, and that the untouched value is still the exact input.
|
||||
- The smoke expectation for edited blocks is updated.
|
||||
- A new test confirms that an untouched block keeps `id`, `style` and `itemprop` byte for byte after another block is edited.
|
||||
**Other changes:** the dist bundle is rebuilt and the plugin README's untouched-values paragraph is updated.
|
||||
|
||||
### WR-06: Translatable Decode can receive non-string values
|
||||
|
||||
**Repository:** summercms.go
|
||||
**Files modified:** `modules/cabana/field_type.go`, `modules/cabana/field_type_test.go`, `modules/pact/capabilities.go`, `modules/pact/README.md`, `docs/backend/field-types.md`
|
||||
**Commit:** f388d27
|
||||
**Applied fix:** before `decodeOne` handles a translatable field's default value, `decodePluginFields` now does the following:
|
||||
- A string passes through.
|
||||
- `nil` becomes `""`.
|
||||
- Any other value is a 422 on the field: "The X field must be text or an object of locale codes to text."
|
||||
**Docs:** the pact `FormFieldValue` doc comment, the pact README row and the field-types page now state the enforced contract.
|
||||
**Tests:** a probe Decode does `in.Value.(string)` and so panics on a broken contract.
|
||||
- A string and null each reach it as a string.
|
||||
- A number, a boolean and an int are refused with a 422 before Decode runs.
|
||||
|
||||
### IN-04: `crypto.randomUUID()` throws outside a secure context
|
||||
|
||||
**Repository:** sm-journal-plugin
|
||||
**Files modified:** `assets/src/blogeditor/element.ts`, `assets/dist/blogeditor.js`, `tests/blogeditor/smoke.test.ts`
|
||||
**Commit:** b997a41
|
||||
**Applied fix:** a new exported `uploadRequestId()` picks the request id in this order:
|
||||
- `crypto.randomUUID` when it exists.
|
||||
- Otherwise, a version 4 UUID built from `crypto.getRandomValues`, which works in insecure contexts.
|
||||
- Otherwise, a time-plus-counter id.
|
||||
The admin host (`PluginField.vue`) echoes any id back, so nothing else changes.
|
||||
**Tests:**
|
||||
- With `randomUUID` stubbed away, ids are unique v4 UUIDs and a full upload round trip still works.
|
||||
- With no `crypto` object, ids are unique counter ids.
|
||||
**Other changes:** the dist bundle is rebuilt.
|
||||
|
||||
## Skipped Issues
|
||||
|
||||
### WR-07: "View site" links to the admin's own origin
|
||||
|
||||
**File:** `admin/src/components/shell/AppShell.vue:25-27, 40-48`
|
||||
**Reason:** By explicit user decision, "View site" defaults to the admin origin. The `SUMMER_FRONTEND__URL` override is a tracked follow-up.
|
||||
**Original issue:** `siteURL` assumes the public site and the admin share an origin, which is wrong for the headless deployment.
|
||||
|
||||
### WR-08: The phase gate prints "Phase 16.2 gate passed" without running the data-loss regression suites
|
||||
|
||||
**File:** `scripts/check-phase16.2.sh:521-544`
|
||||
**Reason:** This is by design. The gate prints a loud NOT RUN block, and `PHASE162_REQUIRE_POSTGRES=1` enforces the Postgres stage. The Postgres run is a tracked Docker-host step.
|
||||
**Original issue:** `--all` exits 0 when Docker is unreachable, without the D-03/D-10 Postgres acceptance tests.
|
||||
|
||||
### IN-01: An embed placeholder that both policies accept may never expand
|
||||
|
||||
**File:** `../vue-journal-layer/app/utils/embeds.ts:41`
|
||||
**Reason:** Deferred. Info findings other than IN-04 are out of scope for this fix pass.
|
||||
**Original issue:** `EMBED_PLACEHOLDER` matches only `data-embed` before `data-id`.
|
||||
|
||||
### IN-02: The expanded YouTube iframe has no `sandbox`, and the exported `/g` regex carries shared `lastIndex` state
|
||||
|
||||
**File:** `../vue-journal-layer/app/utils/embeds.ts:41, 56-60`
|
||||
**Reason:** Deferred. Info findings other than IN-04 are out of scope for this fix pass.
|
||||
**Original issue:** There is no defence-in-depth `sandbox` on the iframe, and the exported global regex is stateful.
|
||||
|
||||
### IN-03: Plugin aliases can shadow list column types
|
||||
|
||||
**File:** `modules/cabana/field_type.go:104-109`
|
||||
**Reason:** Deferred. Info findings other than IN-04 are out of scope for this fix pass.
|
||||
**Original issue:** A plugin alias `date`, `datetime` or `time` is accepted, but its `ListValue` is never called.
|
||||
|
||||
### IN-05: content_html translations are skipped silently when the application is unavailable
|
||||
|
||||
**File:** `../sm-journal-plugin/controllers/posts.go:263-270`
|
||||
**Reason:** Deferred. Info findings other than IN-04 are out of scope for this fix pass.
|
||||
**Original issue:** `writeContentHTMLTranslations` returns nil without a writer, and the columns diverge.
|
||||
|
||||
### IN-06: Untouched detection cannot be byte-exact for legacy content with invalid UTF-8
|
||||
|
||||
**File:** `modules/cabana/field_type.go:641-651`
|
||||
**Reason:** Deferred. Info findings other than IN-04 are out of scope for this fix pass.
|
||||
**Original issue:** JSON transport replaces invalid UTF-8, so an untouched save rewrites such rows.
|
||||
|
||||
### IN-07: "Edit as HTML" selects the first identical block
|
||||
|
||||
**File:** `../sm-journal-plugin/assets/src/blogeditor/element.ts:524-532`
|
||||
**Reason:** Deferred. Info findings other than IN-04 are out of scope for this fix pass.
|
||||
**Original issue:** `indexOf(html)` highlights the first of two identical raw blocks.
|
||||
|
||||
### IN-08: Plugin-typed fields cannot declare `readOnly`/`hidden`/`disabled` in YAML
|
||||
|
||||
**File:** `modules/cabana/field_type.go:33-36`
|
||||
**Reason:** Deferred. Info findings other than IN-04 are out of scope for this fix pass.
|
||||
**Original issue:** `pluginFieldKeys` excludes the general keys that the SPA honours.
|
||||
|
||||
## Verification
|
||||
|
||||
All verification ran in the main checkouts. `workflow.use_worktrees` is `false`, so no worktree was created and the numbers can be reproduced from the trees as committed.
|
||||
|
||||
- **summercms.go:**
|
||||
- `go vet ./...` is clean.
|
||||
- The phase gate's framework set (`FRAMEWORK_RUN` over `./modules/pact ./modules/cabana`) passes.
|
||||
- `go test ./cmd/summer -run TestDocsTree` passes.
|
||||
- The other cabana failures are Docker-only (testcontainers). The failing subtest list is identical before and after these changes; this was checked with `git stash`.
|
||||
- **sm-journal-plugin:**
|
||||
- `go vet ./...` is clean.
|
||||
- `go test ./classes ./controllers ./controllers/api` passes. The root, `models` and `updates` packages need Docker.
|
||||
- vitest passes 232 of 232, and `tsc --noEmit` is clean.
|
||||
- `scripts/check-dist.sh` matches a fresh build.
|
||||
- **vue-journal-layer:** untouched. The gate's layer stage passes 144 of 144, and the corpus copy equals the plugin's.
|
||||
- **Full gate:** `bash scripts/check-phase16.2.sh --all` passes the go, admin (vitest 1218 of 1218), editor, layer and docs stages; docs includes check-admin-openapi, check-admin-dist and the Journal check-dist. Postgres is NOT RUN because no Docker daemon is reachable. Its suites (TestFieldTypeSavePostgres, TestPostsAdminResaveKeepsHTML, TestPostsAdminSaveSanitizesHTML, TestCategoriesAdmin*) still need a Docker-host run. That run matters most for CR-01 and WR-04, whose end-to-end untouched-save behavior those suites cover.
|
||||
|
||||
---
|
||||
|
||||
_Fixed: 2026-10-10T14:46:56Z_
|
||||
_Fixer: Claude (msd-code-fixer)_
|
||||
_Iteration: 1_
|
||||
Reference in New Issue
Block a user