-
Notifications
You must be signed in to change notification settings - Fork 485
Update storing of virtual path for deep nesting #2520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update storing of virtual path for deep nesting #2520
Conversation
When translations are called inside deeply nested component blocks
(3+ levels) using renders_many/renders_one, they were incorrectly
resolving to an intermediate component's scope instead of the
partial's scope where the block was defined.
Example of the bug:
```erb
<!-- In app/views/shared/_action_menu_panel.html.erb -->
<%= render ActionMenuComponent.new do |menu| %>
<% menu.with_list do |list| %>
<% list.with_item do %>
<%= t(".menu_action") %> <!-- Should resolve to shared.action_menu_panel.menu_action -->
<% end %>
<% end %>
<% end %>
```
The translation would incorrectly resolve to `action_list_component.menu_action`
instead of `shared.action_menu_panel.menu_action`.
Previous Fix:
While a fix was added in ViewComponent#2389 to solve translation scope issues, the
`with_original_virtual_path` method only restored the virtual path
one level up to the immediate parent component, not to the original
partial where the block was defined.
This worked for 2-level nesting:
```erb
<!-- Partial → Component → Block (2 levels) -->
<%= render MyComponent.new do %>
<%= t(".title") %> <!-- Works correctly -->
<% end %>
```
But failed for 3+ level nesting:
```erb
<!-- Partial → Component1 → Component2 → Block (3 levels) -->
<%= render ActionMenuComponent.new do |menu| %>
<% menu.with_list do |list| %>
<% list.with_item do %>
<%= t(".menu_action") %> <!-- Resolved to wrong scope -->
<% end %>
<% end %>
<% end %>
```
Solution:
This change captures the virtual path at block definition time (when
`with_*` slot methods are called) and stores it on the slot. When the
block executes, it restores to the captured virtual path.
Implementation:
1. In slotable.rb: Capture `@virtual_path` when a block is provided to
a slot method and store it as `__vc_content_block_virtual_path`
2. In slot.rb: When executing the block, call
`@parent.with_captured_virtual_path(@__vc_content_block_virtual_path)`
to restore the captured path
3. In base.rb: The `with_captured_virtual_path` method temporarily sets
the virtual path to the captured value during block execution
After this fix, deeply nested translations work correctly:
```erb
<!-- Now works at any nesting depth -->
<%= render ActionMenuComponent.new do |menu| %>
<% menu.with_list do |list| %>
<% list.with_item do %>
<%= t(".menu_action") %> <!-- Correctly resolves to shared.action_menu_panel.menu_action -->
<% end %>
<% end %>
<% end %>
```
For test coverage: Tests for 3-level and 5-level nesting scenarios
using simplified test components inspired by Primer's ActionMenu pattern.
Builds on ViewComponent#2389.
Fixes ViewComponent#2386.
f1c372b to
40d9242
Compare
joelhawksley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏🏻 excellent work! Thank you for the clean PR to fix such a gnarly issue ❤️
Thank YOU for all that ya'll do to make this project a success! |
|
@joelhawksley thanks for merging this! Is it possible to make a new release including this fix? Currently we can't upgrade to Rails 8.1 because of this 😄 Thanks for your hard work on this excellent gem! |
What are you trying to accomplish?
Fix translation scope resolution in deeply nested component blocks (3+ levels). Currently, translations called inside deeply nested slot blocks using
renders_many/renders_oneincorrectly resolve to an intermediate component's scope instead of the partial's scope where the block was defined.Example of the bug:
The translation incorrectly resolves to
action_list_component.menu_actioninstead ofshared.action_menu_panel.menu_action.Builds on #2389.
Fixes #2386.
What approach did you choose and why?
The previous fix in #2389 added
with_original_virtual_path, which restored the virtual path one level up to the immediate parent component.This worked for 2-level nesting:
But failed for 3+ level nesting:
The issue: The method only went one level up to the immediate parent, not all the way back to the original partial.
Approach: Capture the virtual path at block definition time instead of trying to walk back up the component hierarchy at execution time.
Implementation details:
with_item), capture the current@virtual_pathand store it on the slot as__vc_content_block_virtual_path@parent.with_captured_virtual_path(@__vc_content_block_virtual_path)to temporarily restore the captured pathwith_captured_virtual_pathmethod takes the captured path as an explicit parameter and temporarily sets the virtual path during block executionWhy this approach:
Alternative approaches considered:
Tradeoffs:
__vc_content_block_virtual_path) to each slot that has a blockAfter this fix, deeply nested translations work correctly:
Anything you want to highlight for special attention from reviewers?
Test coverage: Added comprehensive tests for both 3-level and 5-level nesting scenarios using simplified test components inspired by Primer's ActionMenu pattern. The tests verify that translations resolve to the partial's scope, not any intermediate component's scope.
Implementation note: This also consolidates the virtual path restoration logic by having all three execution contexts (main component content blocks, lambda slots, and component class slots) use the same
with_captured_virtual_pathmethod with an explicit parameter, making the behavior consistent everywhere.