fix(settings): preserve OpenAI form input on validation failure#1862
fix(settings): preserve OpenAI form input on validation failure#1862dripsmvcp wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughSubmitting OpenAI hosting settings now preserves user-entered ChangesOpenAI Form Input Preservation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Fixes we-promise#1824. The OpenAI settings form auto-submits on blur, so typing the URI base before the model triggers cross-field validation. The rescue re-renders the page with values read from Setting.openai_*, which is still blank because the failed save was rejected — so the user's input disappears and they see 'OpenAI model is required' with no value to fix. Stash the submitted uri_base and model on rescue and prefer them over the saved Setting when rendering, so the user can finish typing the missing field and re-submit.
3bdd9d3 to
a00fc07
Compare
|
Clean, minimal fix. The instance-variable fallback pattern ( The test covers the specific regression scenario well. One small suggestion: it might also be worth a test for the Generated by Claude Code |
|
It's been fixed, ty |
|
@jjmata Would you check it again? |
…e-promise#1862) jjmata asked for symmetric coverage of the model field. Add a test where the user changes the URI base and clears the model in the same submit: the cross-field validation fails and the re-rendered model input must reflect the submitted (cleared) value rather than reverting to the saved model. Complements the existing uri_base preservation test.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/controllers/settings/hostings_controller_test.rb (1)
137-140: ⚡ Quick winMake the assertion more explicit for consistency and robustness.
The current assertion verifies the model field doesn't revert to
"saved-model", but it would pass for any other value (empty string, nil, or something unexpected). For clarity and consistency with the uri_base assertion on line 135, explicitly assert the expected cleared value.✨ Suggested refinement
assert_select "input[name=?]", "setting[openai_model]" do |inputs| - assert_not_equal "saved-model", inputs.first["value"].to_s, - "model field must reflect the submitted (cleared) value, not the saved model" + assert_equal "", inputs.first["value"].to_s, + "model field must reflect the submitted (cleared) value, not revert to the saved model" endThis makes the expected behavior explicit and would catch unexpected values beyond just the saved model.
🤖 Prompt for 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. In `@test/controllers/settings/hostings_controller_test.rb` around lines 137 - 140, The test currently only asserts the model field is not "saved-model"; change it to assert the exact expected cleared value instead (match the style used for the uri_base assertion). Locate the assert_select block that references "input[name=?]", "setting[openai_model]" and replace the negative assertion with an explicit equality check that inputs.first["value"].to_s equals the expected cleared value (empty string) so the test fails for nil/unexpected values as well.
🤖 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.
Nitpick comments:
In `@test/controllers/settings/hostings_controller_test.rb`:
- Around line 137-140: The test currently only asserts the model field is not
"saved-model"; change it to assert the exact expected cleared value instead
(match the style used for the uri_base assertion). Locate the assert_select
block that references "input[name=?]", "setting[openai_model]" and replace the
negative assertion with an explicit equality check that
inputs.first["value"].to_s equals the expected cleared value (empty string) so
the test fails for nil/unexpected values as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c65e95ac-3d1b-4767-b2b2-52401c9c3d5e
📒 Files selected for processing (1)
test/controllers/settings/hostings_controller_test.rb
Summary
Fixes #1824.
The OpenAI settings form (Settings → General → OpenAI) auto-submits on blur. If a user types the URI Base before entering the Model, the partial submission hits the cross-field validation (
OpenAI model is required when custom URI base is configured) and the controller re-renders the page. The fields read theirvalue:fromSetting.openai_*, which is still blank because the validation prevented the save — so the user's input is silently wiped and they're left staring at an empty field with an error message and no way to recover other than starting over.This is also the root cause of the symptoms reported in #1825: the auto-submit was persisting partial (URI-base-only or model-only) configs intermittently, which made
User#ai_available?returnfalseand locked the AI Chat behind a misleading "set theOPENAI_ACCESS_TOKENenvironment variable" message.What changed
app/controllers/settings/hostings_controller.rb— in therescue Setting::ValidationErrorblock, stash the submittedopenai_uri_base/openai_modelvalues into instance variables before re-rendering.app/views/settings/hostings/_openai_settings.html.erb— each of the two fields now readsvalue: @openai_*_input || Setting.openai_*, so when the rescue path fires the user sees what they just typed (and the inline error tells them what's missing).test/controllers/settings/hostings_controller_test.rb— new regression test asserting the submitted URI base round-trips back into the rendered<input value="…">after validation failure.No change to the
Setting.validate_openai_config!rule or to any of the existing tests around it (cross-field validation still rejects an incomplete save — we just don't punish the user for it in the UI).29 LOC across 3 files.
Test plan
preserves submitted openai uri base in form when validation failsfails onmain(Expected: "https://api.example.com/v1" Actual: nil) and passes on this branchcannot update openai uri base without model …andcannot clear openai model when custom uri base is setstill pass — DB invariants unchangedbin/rails test test/controllers/settings/hostings_controller_test.rb test/models/setting_test.rb— 41 runs, 114 assertions, 0 failuresbin/rails test— 4307 runs, 0 failures, 26 skips (1 pre-existing env error inactive_storage_authorization_test.rbre: missinglibvipson local host, reproduces on stockmain, unrelated)bin/rubocop -a— cleanbundle exec erb_lint app/views/settings/hostings/_openai_settings.html.erb -a— cleanbin/brakeman --no-pager— no new warningsSummary by CodeRabbit
Bug Fixes
Tests
Before

After

