Admin editor + settings-webhook fixes found in production use - #3
Conversation
`skelpoRepeaterSerialize` built its hidden-input selector with string
concatenation *inside* the quoted selector:
'input[type=hidden][name="f_"+wrap.dataset.name]'
That is not a valid attribute selector, so `querySelector` threw before the
`||` fallback could run and the serializer aborted — the hidden JSON field
kept its pre-edit value and every repeater change was lost on submit.
Concatenate outside the selector string and keep the fallback, so the result
is actually assigned to `hidden`.
`multiselect` rendered as `<select multiple name="f_x">`. Hono's `parseBody` collapses repeated keys to the last value unless the name ends in `[]`, so a three-option selection round-tripped as one option — and the ctrl-click UX made that easy to miss. Render the options as checkboxes posting `f_<name>[]` and read the array back in `parseContentForm`. The comma-separated string form is still accepted for API/legacy posts, and the empty-value skip now checks the `[]` key too so clearing every box saves an empty array instead of being ignored. `select` keeps its existing `<select>` rendering, split out of the shared branch.
A single-language deployment (e.g. a German-only editorial team) had no way to set the admin UI language short of touching every user row — browser Accept-Language negotiation decided it, so editors landed on English. `ADMIN_DEFAULT_LOCALE` sits between the two: it beats negotiation but still loses to an explicit per-user `locale` or the language-switcher cookie, so nobody's own choice is overridden. Invalid values fall through to negotiation via the existing `coerceLocale`. Documented in `.env.example`.
Two defaults in the admin lists that fought the rest of the CMS: * The content list and the new-entry form hardcoded `'en'`, ignoring the `site.defaultLocale` setting. On a de-default site every list opened empty and every new row had to have its locale corrected by hand. Both now read `getDefaultLocale()`; an explicit `?locale=` still wins. * "Delete" in the admin only sets `status='archived'`, but archived rows kept showing in the dashboard, the content list and the translation-sibling picker — so deleting appeared to do nothing, and deleting again was a silent no-op. Archived rows are now excluded from all three. Archived rows remain reachable over the API (`?status=archived`) for restore or purge; surfacing them behind an admin filter is a follow-up.
…tted `setting.changed` is declared in the `WebhookEvent` union, offered in the admin webhook subscription form, and documented in both README and docs/api-spec.md — but no settings writer ever called `fireEvent`. Anyone subscribed to it got nothing, so a consumer using webhooks for cache invalidation served stale settings until it restarted. Emit it from all three writers: the admin settings screen, `PUT /settings/:key`, and the bulk `PUT /settings` (one event per key). Dep keys match the `invalidate()` calls already there.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdmin locale defaults, archived-content filtering, content-editor form handling, and ChangesAdmin locale and content visibility
Content editor form handling
Setting change event dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/admin/contentEditor.tsx`:
- Around line 366-374: Update the option selection logic in the select rendering
branch of the content editor so each option is selected only when its value
strictly equals the stored value, replacing the substring-based
value.includes(o) check while preserving the existing multi-select behavior
elsewhere.
- Around line 705-707: Update the skip condition in the form-processing logic
around raw and body[f_${def.name}[]] so optional multiselect fields are not
skipped when all checkboxes are unchecked and the browser omits the array key.
Let the existing coercion logic at the multiselect handling block assign [] so
prior selections are cleared, while preserving the current skip behavior for
non-multiselect fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10ed9036-6186-49d6-8b25-737c6c104697
📒 Files selected for processing (6)
.env.examplesrc/admin/contentEditor.tsxsrc/admin/i18n/middleware.tssrc/admin/routes.tsxsrc/admin/screens.tsxsrc/routes/api/settings.ts
Two issues raised in review: * `select` used `value.includes(o)`, so an option that is a substring of the stored value (`en` vs `en-US`) also rendered `selected`. With several options marked selected the browser keeps the last one, so an untouched save could silently rewrite the value. Compare with `===`. * An optional multiselect with every box unchecked submits neither `f_<name>` nor `f_<name>[]`, so it hit the empty-and-optional skip and was left out of `fields`. `updateContent` replaces the fields blob wholesale, so the key vanished instead of becoming `[]` — consumers doing `fields.tags.map(...)` get `undefined` rather than an empty array. Exclude multiselect from the skip; the existing coercion then stores `[]`.
1 participant
Five fixes surfaced while running the CMS behind a real editorial site. All are
generic — nothing project-specific — and each was reproducible in the admin UI.
Admin content editor
skelpoRepeaterSerializebuiltits selector with concatenation inside the quoted string
(
'input[name="f_"+wrap.dataset.name]'), which throws — the serializeraborted before writing the hidden JSON field, so every repeater change was
lost on submit.
<select multiple name="f_x">+Hono
parseBodycollapses repeated keys unless the name ends in[].Now rendered as checkboxes posting
f_<name>[]; the comma-string form isstill accepted for API/legacy posts.
Admin defaults
'en'instead of readingsite.defaultLocale, so ade-default site opened every content list empty and every new row had the
wrong locale.
translation-sibling picker. Since admin "Delete" only sets
status='archived', deleting appeared to do nothing and deleting again was asilent no-op. They remain reachable over the API (
?status=archived) forrestore/purge — an admin-side filter is a reasonable follow-up.
Webhooks
setting.changedwas advertised but never fired. It is in theWebhookEventunion, in the admin subscription form, and in both README anddocs/api-spec.md— but no settings writer calledfireEvent. Consumersusing it for cache invalidation served stale settings until restart. Now
emitted from the admin screen,
PUT /settings/:key, and bulkPUT /settings.New config
ADMIN_DEFAULT_LOCALEsets a deployment-wide admin UI language. It beatsAccept-Language negotiation but still loses to a per-user
localeor thelanguage-switcher cookie, so it never overrides someone's own choice.
Documented in
.env.example.Verification
npx tsc --noEmitcleannpm test— 77 unit + 34 integration, 0 failuresSummary by CodeRabbit
New Features
Bug Fixes
Documentation