Skip to content

fix(admin): serialize unset config fields using their defaults - #3

Open
jmagar wants to merge 3 commits into
mainfrom
fix/admin-config-defaults
Open

fix(admin): serialize unset config fields using their defaults#3
jmagar wants to merge 3 commits into
mainfrom
fix/admin-config-defaults

Conversation

@jmagar

@jmagar jmagar commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator
  • serializeConfig now falls back to a field's default (with type-appropriate last resorts: 0/false/"") instead of emitting null for unset number/boolean/text values
  • Admin setup failure now surfaces the real error message instead of a generic "Try again."
  • Adds a test covering effective-default serialization
  • Includes rebuilt bundled frontend assets (public/)

jmagar added 3 commits August 4, 2026 23:40
- serializeConfig now falls back to a field's default (0/false/"" as
  type-appropriate last resorts) instead of emitting null for unset
  number/boolean/text values
- surface the real error message when admin setup fails instead of a
  generic "Try again."
- add test covering effective-default serialization
- rebuild bundled frontend assets
Consolidated fixes from review agents on serializeConfig:

- number/boolean/enable/text fields with no value and no default are
  now omitted from the serialized payload instead of coercing to
  0/false/"". The Go ApplyPatch handler (server/common/config.go:325)
  already skips absent keys and preserves existing/dynamic defaults,
  so silently pinning e.g. force_ssl=false on save was a real
  regression risk. The recursive object walk explicitly drops
  undefined leaves so omission is unambiguous.
- guard the remaining unguarded `(error as Error).message` casts in
  admin.tsx (session, setup, save) with the `instanceof Error` idiom
  already used elsewhere, with sensible fallback copy.
- extend admin.test.ts with cases for value/default omission,
  falsy-but-set booleans, zero-but-set numbers, and enable defaults.
- add component-level tests in admin-screen.test.tsx covering the
  setup error path for both Error and non-Error rejections.

Rebuilt and staged public/ via `make frontend-stage` so the embedded
assets match the new admin.tsx bundle.
Two renamed chunk files were left out of the previous commit's
explicit path list; complete the frontend-stage output.
@jmagar

jmagar commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Applied fixes from the three converging review passes (commits 58b35089, d928df80):

Critical semantics fixserializeConfig in web/src/screens/admin.tsx no longer invents 0/false/"" for fields with no value and no default. Those keys are now omitted from the serialized payload entirely, matching the Go server's ApplyPatch (server/common/config.go:325), which skips absent keys and preserves existing/dynamic-default values via Configuration element Interface(). This still fixes the original fresh-install bug (no more null values aborting ApplyPatch) without the side effect of silently pinning real settings like force_ssl=false on save. Falsy-but-set values (false, 0) and enable-type defaults are preserved correctly.

Error message guards — replaced the remaining unguarded (error as Error).message casts (session load, setup, save) with the instanceof Error idiom already used elsewhere in the file, each with a sensible fallback string for non-Error rejections.

Test coverage — extended admin.test.ts with cases for: value+default both unset (omitted), falsy-but-set boolean over a truthy default, zero-but-set number over a non-zero default, enable-type default resolution, and omission for both boolean and text types. Updated the existing "effective defaults" test since host (null value, null default) is now omitted rather than coerced to "". Added two component-level tests in admin-screen.test.tsx covering the setup error path for both Error and non-Error rejections.

All touched tests pass (14/14), plus tsc --noEmit and eslint on the changed files are clean.

Also rebuilt and staged public/ via make frontend-stage so the embedded assets match the updated admin.tsx bundle (small, expected diff — two chunk renames plus updated HTML references).

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