Repository navigation
revert(*): back out the web settings timezone write fix - #525
gloryfromca wants to merge 1 commit into
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: preserve the working timezone save and keep the retired forward-channel setting removed.
This exact revert reintroduces both failures corrected by #506. Calling settings.set at this head with ordinary values produced KeyError for both keys and left the config unchanged: the cron writer accepts Python field names while the restored RPC path passes defaultTimezone, and forwardChannels has no schema field at all. The latter also conflicts with the fire-at-origin architecture and the loader migration that strips this retired key. The focused suites remain green because this revert deletes the five regression cases that covered these paths.
Coverage: I reviewed the full github/main...HEAD diff; AGENTS.md and CLAUDE.md; CONTEXT-MAP.md and the canonical fire-at-origin guidance; the settings UI caller, RPC boundary, cron writer, schema, loader, scheduling consumers, relevant history and #506 record; backward compatibility; test strength; architecture boundaries; and current mergeability.
Verification: uv run pytest tests/test_rpc_settings.py tests/test_config_update.py tests/test_config_loader.py tests/test_cli_cron_commands.py -x (232 passed); npm test --prefix ui-web -- --run src/features/settings/SettingsPage.test.tsx (123 passed); npm run type-check --prefix ui-web (passed). A direct isolated-config invocation of settings_set reproduced KeyError for both cron.defaultTimezone = "UTC" and cron.forwardChannels = ["telegram"], with {} still on disk.
This reverts commit 908666d. The fix edits ui-web/src/features/settings/SettingsPage.tsx, a file the ui-web rebuild on refactor/ui_web_architecture replaces with SettingsApp and its pages/ tree, so the change is redone on that branch rather than carried across the rewrite. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
51482da to
9f1fd7c
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The tree is unchanged from the previously reviewed revision, so both measured regressions remain real. The amended PR record now explicitly accepts the temporary timezone internal_error and defers the whole #506 repair to the replacement settings implementation on #475. That author-level scope decision means neither item blocks this revert; both earlier threads have been replied to and resolved.
Named follow-up: #475's current head already omits the retired forwardChannels row from the replacement UI, but its RPC backend still passes defaultTimezone without conversion and still whitelists cron.forwardChannels. The resubmission there still needs the backend conversion/refusal and regression coverage before that branch merges.
Coverage for this revision: I reviewed the full github/main...HEAD diff and the delta from 51482dacc338; rechecked the AGENTS.md/CLAUDE.md constraints, canonical fire-at-origin guidance, callers, schema, loader, history, backward-compatibility impact, removed tests, architecture boundaries, amended PR record, and the stated #475 follow-up branch. The new commit changes attribution/message metadata, not the source tree.
Verification: uv run pytest tests/test_rpc_settings.py tests/test_config_update.py tests/test_config_loader.py tests/test_cli_cron_commands.py -x (232 passed); npm test --prefix ui-web -- --run src/features/settings/SettingsPage.test.tsx (123 passed); npm run type-check --prefix ui-web (passed).
Summary
Reverts #506. The fix it carried edits
ui-web/src/features/settings/SettingsPage.tsx,a file the ui-web rebuild on
refactor/ui_web_architecture(#475) replaces withSettingsApp.tsxand itspages/tree, so the frontend half cannot travel acrossthat rewrite. The backend half goes with it so the change stays one reviewable unit:
it is redone on the refactor branch instead, where the settings page it has to edit
actually exists. #506 carries a note asking its author to resubmit there.
Nothing else landed on
mainbetween #506 and this revert, so the result is thepre-#506 tree exactly.
Type
Verification
git diff --stat 04eb8a31d 9f1fd7c73is empty: this head's tree(
00f113b27b7443562f83e89f27adcf8874c1dc91) is byte-identical to the tree of04eb8a31d, the commitmainsat on before #506 merged. The unit suite,lint, and the contract checks ran on this head in CI rather than locally.
coverage gatesis red and stays red: the diff-coverage gate scores the 11restored lines in the
cron.forwardChannelsvalidation branch, which had no testcovering them before #506 deleted them. A revert cannot raise that number without
writing tests for code it is removing from the writable set.
Risk
The web settings timezone control goes back to failing its write with
-32603 internal_error, which is the behaviour every release before #506 shipped.Rollback is re-applying #506, or the resubmission on
refactor/ui_web_architecture.Related Issues
Reverts #506.