fix(chat): keep the model picker on-screen and make set_config type-safe - #4839
Conversation
- The model dropdown opened upward with a fixed max-height and no viewport awareness, so on a short window it rendered above the page's fixed top nav (.menu-bar, z-index: 1000) and was clipped/painted over. openModelList() now clamps the list's height to the space actually free below that bar, using its live rendered position rather than a hardcoded offset. - set_config accepted its value as an opaque string (the tool schema has no way to know a setting's real type) and handed it straight to set_state_external(), whose switch branch picks turn_on/turn_off by Python truthiness - so a request to turn a switch off (the string "False") always evaluated truthy and turned it on instead, while still reporting success. Values are now coerced to the target CONFIG_ITEMS entry's real type (bool for switch, float/int for input_number/number) before writing, and the tool refuses to report success unless the write actually landed. - Raised the plain completion retry budget from 3 to 10 attempts, with the backoff settling at 5s after the first two retries (1s, 3s, then 5s repeating) via the existing retry_delay_for() schedule mechanism. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new numeric coercion path in set_config can silently truncate non-integer values for step==1 items (e.g. "2.9" → 2) and then report success for the truncated value.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves the Chat UI and tool reliability by (1) making the model picker dropdown respect the usable viewport (accounting for the fixed .menu-bar) and (2) making set_config type-safe so switch/number writes behave correctly and only report success when the value actually changes, alongside expanding the completion retry budget and updating tests accordingly.
Changes:
- Clamp the chat model picker list height to the space below the fixed menu bar, and clear stale “thinking bubble” state when the client transitions to idle.
- Add type coercion + applied-value verification to
set_configto prevent truthiness bugs (notably"False"for switches) and avoid reporting false success. - Increase plain completion retry attempts/delays and update tests to validate the retry schedule and new
set_configbehavior.
File summaries
| File | Description |
|---|---|
| apps/predbat/web_chat.py | Adjusts model picker dropdown sizing to stay visible under the fixed nav; clears thinking bubble on idle. |
| apps/predbat/tests/test_chat.py | Updates retry-backoff assertions and canned-response sizing for the higher retry budget. |
| apps/predbat/tests/test_agent_tools.py | Adds coverage ensuring set_config coerces switch values and rejects invalid boolean strings. |
| apps/predbat/chat.py | Raises completion retry budget and defines the retry-delay schedule with tail-repeat semantics. |
| apps/predbat/agent_tools.py | Implements set_config coercion + read-back verification to ensure writes are real and type-correct. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Flooring a fractional value via int() would silently write something other than what was requested (2.9 becomes 2) while still reporting success against the read-back - the same class of bug as the switch truthiness issue this PR already fixes, just for numbers. Reject instead of guessing. Addresses Copilot review feedback on PR #4839. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ingfall2008/batpred into fix/chat-model-picker-and-set-config
Summary
.menu-bar,z-index: 1000, shared across every Predbat page) and was clipped/painted over.openModelList()now measures the space actually free below that bar and clamps the list's height to it.set_configaccepted itsvalueargument as an opaque JSON string (the schema has no way to know a setting's real type) and passed it straight toset_state_external(), whose switch handling picksturn_on/turn_offby Python truthiness - so a request to turn a switch off (the string"False") always evaluated truthy and turned it on instead, while still reporting success. This was reproduced live during this session:set_configon an already-on switch withvalue: "False"returned{"success": true, "previous_value": true, "new_value": true}- no change, reported as success. Values are now coerced to the target CONFIG_ITEMS entry's real type (boolfor switch,float/intforinput_number/number) before writing, and the tool refuses to report success unless the write actually took effect.retry_delay_for()schedule mechanism.Test plan
./run_all --test chat --test web_chat --test agent_tools --test hainterface_service(addedtest_set_config_coerces_switch_value; updatedtest_retry_backoff_sequence_is_one_then_three_secondsandtest_turn_error_is_detailed_and_stored_but_never_replayedfor the new retry constants)./run_all --quick(full quick suite)./run_pre_commit🤖 Generated with Claude Code