fix(ui+http): timezone validator + system-configs 404 noise

[vanducng]

Summary

Two small quick-fixes that surfaced together on a fork dashboard but apply identically to upstream/dev (both files unchanged from upstream).

1. Timezone save rejected with "Invalid timezone"

Saving a cron job with `Asia/Ho_Chi_Minh` (or any IANA name picked from the dropdown) failed validator and showed an "Invalid timezone" toast.

Root cause: `isValidIanaTimezone` (in `ui/web/src/lib/timezone-utils.ts`) built a `Set` from `Intl.supportedValuesOf("timeZone")` then canonicalized each name via `new Intl.DateTimeFormat(...).resolvedOptions().timeZone`. Across browser/OS combos this drops or remaps some names — e.g. on Chrome 110+ `Asia/Saigon` canonicalizes to `Asia/Ho_Chi_Minh` but on older Safari/Firefox it stays `Asia/Saigon`. When the user's stored value happens to be the absent form, save fails despite the value being a valid IANA name.

Fix: validate via direct `Intl.DateTimeFormat(...)` try/catch. That's the canonical IANA validation path — accepts every name the browser's tz database understands, deprecated aliases included.

2. `/v1/system-configs/{key}` 404 console spam

`background-error-banner.tsx` polls `/v1/system-configs/alert.background.provider_error` to recover alert state after page refresh. When no alert exists (the common case), backend returned 404. The frontend's `.catch()` handled it semantically — but Chrome still logs "Failed to load resource" for every 404. Devtools console fills with hundreds of these entries.

Fix: `handleGet` returns 200 with empty `value` for missing keys. Settings-store "missing" == "empty" is the natural semantics here; the frontend's existing `if (res.value)` guard already handles both shapes. No behavior change for other callers.

Files changed

• `ui/web/src/lib/timezone-utils.ts` — replace Set-based validator with Intl try/catch
• `internal/http/system_configs.go` — `handleGet` returns 200 with empty value on miss

Test plan

• `go build ./...` + `go vet ./...` green
• `pnpm build` in `ui/web/` green
• Manual: open a cron-job detail → pick `Asia/Ho_Chi_Minh` → Save → no toast
• Manual: open dashboard with no active provider alert → console clean (no `/v1/system-configs/*` 404)

Notes

Tested in production on a downstream fork (dataplanelabs/goclaw v3.23.11) since 2026-05-25. Both fixes resolved the reported user-visible issues; no regressions observed across the trace/cron/alert paths.

[mrgoonie]

Backlog review: merge-candidate. The timezone validator change uses Intl directly, which handles canonical/deprecated IANA aliases better than the Set-based check, and the system-config missing-key behavior is intentionally fail-soft. go and web checks are passing.

[mrgoonie]

review-pr --fix --reply result

Verdict: Request changes until the prepared fix can be applied to the PR branch.

Summary: The timezone validator change is sound. The system-configs 404 noise fix needs one guard so missing config keys are treated as empty without masking real store failures.

Iterations: 1 review loop, 1 local fix attempt.
Important finding fixed locally: SystemConfigsHandler.handleGet treats every store.Get error as a missing key and returns 200 {"value":""}. That hides database, tenant-context, or store failures from callers. Expected behavior: missing key can be empty 200 for the alert poller, but non-not-found store errors should still return 500.

Prepared local commit: a1c20d65 fix: preserve system config store errors
Prepared fix scope: added store.ErrSystemConfigNotFound, returned it from PG/SQLite SystemConfigStore.Get no-row paths, made the HTTP handler fail-soft only for that sentinel, and added handler tests for missing vs store-failure responses.
Merge/conflict state: GitHub reports CLEAN.
CI state: Existing PR checks are green (go, web), but no new checks ran for the prepared fix because push was blocked.

Local verification run after the fix:

• go test ./internal/http ./internal/store/pg
• go test -tags sqliteonly ./internal/store/sqlitestore
• go build ./...
• go vet ./...
• go build -tags sqliteonly ./...
• pnpm install --frozen-lockfile in ui/web/
• pnpm build in ui/web/

External blocker: GitHub rejected pushing to dataplanelabs/goclaw:upstream-fix/timezone-validator-sysconfig-404 as mrgoonie (Permission denied). Branch owner or maintainer needs to apply the prepared store-error guard, or grant push access.

Unresolved blockers/questions: fork push permission blocked applying the prepared fix to the PR head.

[mrgoonie]

Summary: This is a useful pair of small fixes, but the system-config GET change currently swallows every store error and returns an empty successful config response.

Risk level: Medium

Mandatory gates:

• Duplicate/prior implementation: clear — repo search for timezone/system-configs found only this PR.
• Project standards: issue found — HTTP handler must not hide backend/tenant errors as successful API responses.
• Strategic necessity: clear value — the timezone validation fix and alert 404-noise reduction both address real UX friction.
• CI/checks: green (go, web)

Findings:

• Critical: none
• Important: internal/http/system_configs.go now treats any h.store.Get error as a missing value and returns 200 {"value":""}. The store can return non-not-found failures too, including tenant-context failures (system config get: tenant_id required) and database/query errors. Those should remain errors; otherwise the API silently masks real backend/auth/tenant bugs as an empty config. Please distinguish missing-key/no-rows from operational errors (for example by making the store expose a typed not-found error or by adding a specific GetOptional path for alert-state reads) and only return empty value for the intended missing-alert case.
• Suggestion: consider scoping the fail-soft behavior to alert.background.provider_error or another explicit optional-read path. Changing all /v1/system-configs/{key} misses from 404 to 200 is an API contract change that may be okay, but it should be intentional and tested.

Verdict: REQUEST_CHANGES

[clark-cant]

🦸‍♂️ Maintainer follow-up — This PR has been in CHANGES_REQUESTED state for 6 days (since @mrgoonie's review on 2026-06-20). The review feedback is specific and actionable: 1) Distinguish not-found/no-rows from operational errors in system_configs.go handleGet — don't swallow tenant-context or DB errors as empty 200 responses, 2) Consider scoping the fail-soft behavior to an explicit optional-read path or the specific alert key. The timezone validation fix looks good as-is. CI is green, the PR is mergeable — once the system-configs error handling is addressed, this should be ready for re-review. @vanducng are you still planning to iterate on this? Posted by github-maintain at 2026-06-26T10:50:00Z

[clark-cant]

🦸‍♂️ Maintainer follow-up — Branch is stale (BLOCKED merge state, needs rebase onto current dev). CI is green on the current head, which is a good sign.

Review feedback from @mrgoonie (2026-06-15) still pending:

• SystemConfigsHandler.handleGet treats every store.Get error as a missing key → returns 200 with empty value. This masks real DB/tenant-context failures.
• Fix: distinguish store.ErrSystemConfigNotFound (→ 200 empty) from operational errors (→ 500). A prepared fix was described in the review comment but could not be pushed due to fork permissions.

Timezone validator fix — looks good as-is, no changes needed.

Action needed from @vanducng:

1. Rebase onto current dev
2. Apply the store-error guard (or equivalent)
3. Request re-review

This is a small, clean PR — should be quick to close out. 3 days since last maintainer check-in.

Posted by github-maintain at 2026-06-29T17:54:00Z

[vanducng]

@clark-cant addressed both follow-up items in 919a19f9:

• Rebasing: PR head is now rebased onto current dev.
• Store-error guard: SystemConfigsHandler.handleGet now returns empty 200 only for store.ErrSystemConfigNotFound; operational store errors return 500 instead of being masked. PG and SQLite stores now wrap missing rows with the shared sentinel, and internal/http/system_configs_test.go covers missing-key vs store-error behavior.

Verification:

• env -u GOROOT go test ./internal/http ./internal/store/pg
• env -u GOROOT go test -tags sqliteonly ./internal/store/sqlitestore
• GitHub checks are green on the pushed head: go, web, release-versioning.

Ready for re-review.

[clark-cant]

LGTM — the fix commit directly addresses the previous review feedback.

Summary of changes:

1. Timezone validator (isValidIanaTimezone): Replaced fragile Set-based check with direct Intl.DateTimeFormat try/catch — the canonical IANA validation path. Accepts deprecated aliases the browser knows. Clean.

2. System-configs GET: Now properly distinguishes not-found from operational errors:

   • store.ErrSystemConfigNotFound → 200 with empty value (correct: alert key legitimately absent most of the time)
   • Other errors (tenant_id required, DB failures) → 500 with error message (correct: don't mask real bugs)
   • Sentinel error ErrSystemConfigNotFound added to store package, both PG and SQLite stores wrap it correctly
   • Tests cover both paths: missing key returns 200, store error returns 500

Risk: Low. Both changes are well-scoped, well-tested, and the system-configs fix is the right approach.

Verdict: Approve.

[clark-cant]

🦸‍♂️ Maintainer re-review — github-maintain

All previous feedback addressed. Approving.

Changes since last review:

• ✅ Rebased onto current dev
• ✅ ErrSystemConfigNotFound sentinel added to internal/store/system_config_store.go
• ✅ PG and SQLite stores wrap missing rows with the shared sentinel
• ✅ handleGet now distinguishes not-found (→ 200 empty) from operational errors (→ 500)
• ✅ Handler tests cover both paths (missing key vs store error)
• ✅ Timezone validator uses Intl try/catch directly — clean and correct

Verification:

• CI green: go, web, release-versioning all SUCCESS on head 919a19f9
• mergeStateStatus: BLOCKED only due to stale CHANGES_REQUESTED (this review resolves it)
• Diff scope matches stated purpose: 6 files, +97/-12, no unrelated changes

Mandatory gates:

• Duplicate/prior implementation: clear
• Project standards: compliant (targets dev, proper error handling, tests included)
• Strategic necessity: clear value (fixes timezone save + eliminates console 404 spam)

Verdict: Approve — ready for merge.

Posted by github-maintain at $(date -u +'%Y-%m-%dT%H:%M:%SZ')