Fix potentially uninitialized locals#2885
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR addresses #2845 by removing a self-shadowing local in a rendered collection partial and by preventing a test helper ensure block from referencing locals that may not have been assigned if setup fails.
Changes:
- Render the prompt command partial with
as: :prompt_commandand rename the destructured locals to avoid self-shadowing. - Update
with_default_url_hosttest helper to only restoredefault_url_options[:host]if it was successfully modified.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/models/account/data_transfer/action_text/rich_text_record_set_test.rb | Guards URL host restoration so ensure doesn’t touch potentially uninitialized locals. |
| app/views/prompts/commands/index.html.erb | Passes an explicit local name (prompt_command) when rendering the collection. |
| app/views/prompts/commands/_command.html.erb | Destructures the passed-in local into non-shadowing variable names and updates references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
flavorjones
left a comment
flavorjones
left a comment
There was a problem hiding this comment.
Thanks for taking the time to open a pull request.
There are two changes here, and only one is related to the issue you linked; but I don't think it's necessary.
The other change, which removes command variable shadowing, is something I would merge, if you'd omit the other change.
| options = nil | ||
| had_key = false | ||
| original = nil | ||
| changed_host = false | ||
|
|
||
| options = Rails.application.routes.default_url_options | ||
| had_key = options.key?(:host) | ||
| original = options[:host] | ||
| options[:host] = host | ||
| changed_host = true | ||
| yield | ||
| ensure | ||
| if had_key | ||
| options[:host] = original | ||
| else | ||
| options.delete(:host) | ||
| if changed_host | ||
| if had_key | ||
| options[:host] = original | ||
| else | ||
| options.delete(:host) | ||
| end |
There was a problem hiding this comment.
I think this change is unnecessary in a test that's not (to my knowledge) failing, and it makes the code less readable in my opinion. But please educate me if it's causing a problem in some way.
Summary
Fixes #2845.
Testing
bin/rails test test/models/account/data_transfer/action_text/rich_text_record_set_test.rbbin/rails test