Skip to content

Commit 387bc15

Browse files
committed
Update storing of virtual path for deep nesting
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 #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 #2389. Fixes #2386.
1 parent 642bbce commit 387bc15

File tree

14 files changed

+183
-9
lines changed

14 files changed

+183
-9
lines changed

lib/view_component/base.rb

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ def content
363363

364364
@__vc_content =
365365
if __vc_render_in_block_provided?
366-
with_original_virtual_path do
366+
with_captured_virtual_path(@old_virtual_path) do
367367
view_context.capture(self, &@__vc_render_in_block)
368368
end
369369
elsif __vc_content_set_by_with_content_defined?
@@ -379,11 +379,14 @@ def content?
379379
end
380380

381381
# @private
382-
def with_original_virtual_path
383-
@view_context.instance_variable_set(:@virtual_path, @old_virtual_path)
382+
# Temporarily sets the virtual path to the captured value, then restores it.
383+
# This ensures translations and other path-dependent code execute with the correct scope.
384+
def with_captured_virtual_path(captured_path)
385+
old_virtual_path = @view_context.instance_variable_get(:@virtual_path)
386+
@view_context.instance_variable_set(:@virtual_path, captured_path)
384387
yield
385388
ensure
386-
@view_context.instance_variable_set(:@virtual_path, virtual_path)
389+
@view_context.instance_variable_set(:@virtual_path, old_virtual_path)
387390
end
388391

389392
private

lib/view_component/slot.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ module ViewComponent
66
class Slot
77
include ViewComponent::WithContentHelper
88

9-
attr_writer :__vc_component_instance, :__vc_content_block, :__vc_content
9+
attr_writer :__vc_component_instance, :__vc_content_block, :__vc_content, :__vc_content_block_virtual_path
1010

1111
def initialize(parent)
1212
@parent = parent
@@ -58,7 +58,7 @@ def to_s
5858
if defined?(@__vc_content_block)
5959
# render_in is faster than `parent.render`
6060
@__vc_component_instance.render_in(view_context) do |*args|
61-
@parent.with_original_virtual_path do
61+
@parent.with_captured_virtual_path(@__vc_content_block_virtual_path) do
6262
@__vc_content_block.call(*args)
6363
end
6464
end
@@ -68,7 +68,7 @@ def to_s
6868
elsif defined?(@__vc_content)
6969
@__vc_content
7070
elsif defined?(@__vc_content_block)
71-
@parent.with_original_virtual_path do
71+
@parent.with_captured_virtual_path(@__vc_content_block_virtual_path) do
7272
view_context.capture(&@__vc_content_block)
7373
end
7474
elsif defined?(@__vc_content_set_by_with_content)

lib/view_component/slotable.rb

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,12 @@ def __vc_set_slot(slot_name, slot_definition = nil, *args, **kwargs, &block)
390390
# 2. Since we have to pass block content to components when calling
391391
# `render`, evaluating the block here would require us to call
392392
# `view_context.capture` twice, which is slower
393-
slot.__vc_content_block = block if block
393+
if block
394+
slot.__vc_content_block = block
395+
# Capture the virtual path at the time the block is defined, so that
396+
# translations resolve relative to where the block was created, not where it's rendered
397+
slot.__vc_content_block_virtual_path = view_context.instance_variable_get(:@virtual_path)
398+
end
394399

395400
# If class
396401
if slot_definition[:renderable]
@@ -408,7 +413,9 @@ def __vc_set_slot(slot_name, slot_definition = nil, *args, **kwargs, &block)
408413
renderable_value =
409414
if block
410415
renderable_function.call(*args, **kwargs) do |*rargs|
411-
view_context.capture(*rargs, &block)
416+
with_captured_virtual_path(@old_virtual_path) do
417+
view_context.capture(*rargs, &block)
418+
end
412419
end
413420
else
414421
renderable_function.call(*args, **kwargs)
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# frozen_string_literal: true
2+
3+
# Simplified ActionList component inspired by Primer::Alpha::ActionList
4+
# Used to test deeply nested translation scoping
5+
class ActionListComponent < ViewComponent::Base
6+
renders_many :items, MenuItemComponent
7+
8+
erb_template <<~ERB
9+
<ul class="action-list">
10+
<% items.each do |item| %>
11+
<%= item %>
12+
<% end %>
13+
</ul>
14+
ERB
15+
end
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
# frozen_string_literal: true
2+
3+
# Simplified ActionMenu component inspired by Primer::Alpha::ActionMenu
4+
# Used to test deeply nested translation scoping
5+
class ActionMenuComponent < ViewComponent::Base
6+
renders_one :list, ActionListComponent
7+
8+
erb_template <<~ERB
9+
<div class="action-menu">
10+
<%= list %>
11+
</div>
12+
ERB
13+
end
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# frozen_string_literal: true
2+
3+
class ActionMenuPanelWrapperComponent < ViewComponent::Base
4+
def call
5+
render "shared/action_menu_panel"
6+
end
7+
end
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# frozen_string_literal: true
2+
3+
class DeepNavigationWrapperComponent < ViewComponent::Base
4+
def call
5+
render "shared/deep_navigation"
6+
end
7+
end
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# frozen_string_literal: true
2+
3+
# Simplified MenuItem component inspired by Primer::Alpha::ActionList::Item
4+
# Used to test deeply nested translation scoping
5+
class MenuItemComponent < ViewComponent::Base
6+
erb_template <<~ERB
7+
<li class="menu-item">
8+
<%= content %>
9+
</li>
10+
ERB
11+
end
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
# frozen_string_literal: true
2+
3+
# Level 2: Navigation component that contains an action menu
4+
class NavComponent < ViewComponent::Base
5+
renders_one :action_menu, ActionMenuComponent
6+
7+
erb_template <<~ERB
8+
<nav class="nav">
9+
<%= action_menu %>
10+
</nav>
11+
ERB
12+
end
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
# frozen_string_literal: true
2+
3+
# Level 0: Wraps a nav component (for 5-level deep testing)
4+
class SectionComponent < ViewComponent::Base
5+
renders_one :nav, NavComponent
6+
7+
erb_template <<~ERB
8+
<section class="section">
9+
<%= nav %>
10+
</section>
11+
ERB
12+
end

0 commit comments

Comments
 (0)