From 086c11e7374ecae7c5123d577c028b51f53e8492 Mon Sep 17 00:00:00 2001 From: Nathaniel Watts Date: Tue, 9 Dec 2025 21:32:30 -0600 Subject: [PATCH 1/2] Update storing of virtual path for deep nesting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 <%= render ActionMenuComponent.new do |menu| %> <% menu.with_list do |list| %> <% list.with_item do %> <%= t(".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 <%= render MyComponent.new do %> <%= t(".title") %> <% end %> ``` But failed for 3+ level nesting: ```erb <%= render ActionMenuComponent.new do |menu| %> <% menu.with_list do |list| %> <% list.with_item do %> <%= t(".menu_action") %> <% 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 <%= render ActionMenuComponent.new do |menu| %> <% menu.with_list do |list| %> <% list.with_item do %> <%= t(".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. --- lib/view_component/base.rb | 11 ++-- lib/view_component/slot.rb | 6 +-- lib/view_component/slotable.rb | 11 +++- .../app/components/action_list_component.rb | 15 ++++++ .../app/components/action_menu_component.rb | 13 +++++ .../action_menu_panel_wrapper_component.rb | 7 +++ .../deep_navigation_wrapper_component.rb | 7 +++ .../app/components/menu_item_component.rb | 11 ++++ test/sandbox/app/components/nav_component.rb | 12 +++++ .../app/components/section_component.rb | 12 +++++ .../views/shared/_action_menu_panel.html.erb | 7 +++ .../views/shared/_deep_navigation.html.erb | 11 ++++ test/sandbox/config/locales/en.yml | 4 ++ .../deeply_nested_translation_test.rb | 51 +++++++++++++++++++ 14 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 test/sandbox/app/components/action_list_component.rb create mode 100644 test/sandbox/app/components/action_menu_component.rb create mode 100644 test/sandbox/app/components/action_menu_panel_wrapper_component.rb create mode 100644 test/sandbox/app/components/deep_navigation_wrapper_component.rb create mode 100644 test/sandbox/app/components/menu_item_component.rb create mode 100644 test/sandbox/app/components/nav_component.rb create mode 100644 test/sandbox/app/components/section_component.rb create mode 100644 test/sandbox/app/views/shared/_action_menu_panel.html.erb create mode 100644 test/sandbox/app/views/shared/_deep_navigation.html.erb create mode 100644 test/sandbox/test/components/deeply_nested_translation_test.rb diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb index 68f15b244..c6d75acaa 100644 --- a/lib/view_component/base.rb +++ b/lib/view_component/base.rb @@ -363,7 +363,7 @@ def content @__vc_content = if __vc_render_in_block_provided? - with_original_virtual_path do + with_captured_virtual_path(@old_virtual_path) do view_context.capture(self, &@__vc_render_in_block) end elsif __vc_content_set_by_with_content_defined? @@ -379,11 +379,14 @@ def content? end # @private - def with_original_virtual_path - @view_context.instance_variable_set(:@virtual_path, @old_virtual_path) + # Temporarily sets the virtual path to the captured value, then restores it. + # This ensures translations and other path-dependent code execute with the correct scope. + def with_captured_virtual_path(captured_path) + old_virtual_path = @view_context.instance_variable_get(:@virtual_path) + @view_context.instance_variable_set(:@virtual_path, captured_path) yield ensure - @view_context.instance_variable_set(:@virtual_path, virtual_path) + @view_context.instance_variable_set(:@virtual_path, old_virtual_path) end private diff --git a/lib/view_component/slot.rb b/lib/view_component/slot.rb index 299ca7210..e70a421e2 100644 --- a/lib/view_component/slot.rb +++ b/lib/view_component/slot.rb @@ -6,7 +6,7 @@ module ViewComponent class Slot include ViewComponent::WithContentHelper - attr_writer :__vc_component_instance, :__vc_content_block, :__vc_content + attr_writer :__vc_component_instance, :__vc_content_block, :__vc_content, :__vc_content_block_virtual_path def initialize(parent) @parent = parent @@ -58,7 +58,7 @@ def to_s if defined?(@__vc_content_block) # render_in is faster than `parent.render` @__vc_component_instance.render_in(view_context) do |*args| - @parent.with_original_virtual_path do + @parent.with_captured_virtual_path(@__vc_content_block_virtual_path) do @__vc_content_block.call(*args) end end @@ -68,7 +68,7 @@ def to_s elsif defined?(@__vc_content) @__vc_content elsif defined?(@__vc_content_block) - @parent.with_original_virtual_path do + @parent.with_captured_virtual_path(@__vc_content_block_virtual_path) do view_context.capture(&@__vc_content_block) end elsif defined?(@__vc_content_set_by_with_content) diff --git a/lib/view_component/slotable.rb b/lib/view_component/slotable.rb index ac78b320e..03f745f20 100644 --- a/lib/view_component/slotable.rb +++ b/lib/view_component/slotable.rb @@ -390,7 +390,12 @@ def __vc_set_slot(slot_name, slot_definition = nil, *args, **kwargs, &block) # 2. Since we have to pass block content to components when calling # `render`, evaluating the block here would require us to call # `view_context.capture` twice, which is slower - slot.__vc_content_block = block if block + if block + slot.__vc_content_block = block + # Capture the virtual path at the time the block is defined, so that + # translations resolve relative to where the block was created, not where it's rendered + slot.__vc_content_block_virtual_path = view_context.instance_variable_get(:@virtual_path) + end # If class if slot_definition[:renderable] @@ -408,7 +413,9 @@ def __vc_set_slot(slot_name, slot_definition = nil, *args, **kwargs, &block) renderable_value = if block renderable_function.call(*args, **kwargs) do |*rargs| - view_context.capture(*rargs, &block) + with_captured_virtual_path(@old_virtual_path) do + view_context.capture(*rargs, &block) + end end else renderable_function.call(*args, **kwargs) diff --git a/test/sandbox/app/components/action_list_component.rb b/test/sandbox/app/components/action_list_component.rb new file mode 100644 index 000000000..eeabca655 --- /dev/null +++ b/test/sandbox/app/components/action_list_component.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# Simplified ActionList component inspired by Primer::Alpha::ActionList +# Used to test deeply nested translation scoping +class ActionListComponent < ViewComponent::Base + renders_many :items, MenuItemComponent + + erb_template <<~ERB + + ERB +end diff --git a/test/sandbox/app/components/action_menu_component.rb b/test/sandbox/app/components/action_menu_component.rb new file mode 100644 index 000000000..5d90c88be --- /dev/null +++ b/test/sandbox/app/components/action_menu_component.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +# Simplified ActionMenu component inspired by Primer::Alpha::ActionMenu +# Used to test deeply nested translation scoping +class ActionMenuComponent < ViewComponent::Base + renders_one :list, ActionListComponent + + erb_template <<~ERB +
+ <%= list %> +
+ ERB +end diff --git a/test/sandbox/app/components/action_menu_panel_wrapper_component.rb b/test/sandbox/app/components/action_menu_panel_wrapper_component.rb new file mode 100644 index 000000000..a18d662ea --- /dev/null +++ b/test/sandbox/app/components/action_menu_panel_wrapper_component.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class ActionMenuPanelWrapperComponent < ViewComponent::Base + def call + render "shared/action_menu_panel" + end +end diff --git a/test/sandbox/app/components/deep_navigation_wrapper_component.rb b/test/sandbox/app/components/deep_navigation_wrapper_component.rb new file mode 100644 index 000000000..cf69fbd99 --- /dev/null +++ b/test/sandbox/app/components/deep_navigation_wrapper_component.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class DeepNavigationWrapperComponent < ViewComponent::Base + def call + render "shared/deep_navigation" + end +end diff --git a/test/sandbox/app/components/menu_item_component.rb b/test/sandbox/app/components/menu_item_component.rb new file mode 100644 index 000000000..49481b0b9 --- /dev/null +++ b/test/sandbox/app/components/menu_item_component.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +# Simplified MenuItem component inspired by Primer::Alpha::ActionList::Item +# Used to test deeply nested translation scoping +class MenuItemComponent < ViewComponent::Base + erb_template <<~ERB + + ERB +end diff --git a/test/sandbox/app/components/nav_component.rb b/test/sandbox/app/components/nav_component.rb new file mode 100644 index 000000000..0ae50bd89 --- /dev/null +++ b/test/sandbox/app/components/nav_component.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +# Level 2: Navigation component that contains an action menu +class NavComponent < ViewComponent::Base + renders_one :action_menu, ActionMenuComponent + + erb_template <<~ERB + + ERB +end diff --git a/test/sandbox/app/components/section_component.rb b/test/sandbox/app/components/section_component.rb new file mode 100644 index 000000000..efb76274e --- /dev/null +++ b/test/sandbox/app/components/section_component.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +# Level 0: Wraps a nav component (for 5-level deep testing) +class SectionComponent < ViewComponent::Base + renders_one :nav, NavComponent + + erb_template <<~ERB +
+ <%= nav %> +
+ ERB +end diff --git a/test/sandbox/app/views/shared/_action_menu_panel.html.erb b/test/sandbox/app/views/shared/_action_menu_panel.html.erb new file mode 100644 index 000000000..5e8d98862 --- /dev/null +++ b/test/sandbox/app/views/shared/_action_menu_panel.html.erb @@ -0,0 +1,7 @@ +<%= render ActionMenuComponent.new do |menu| %> + <% menu.with_list do |list| %> + <% list.with_item do %> + <%= t(".menu_action") %> + <% end %> + <% end %> +<% end %> diff --git a/test/sandbox/app/views/shared/_deep_navigation.html.erb b/test/sandbox/app/views/shared/_deep_navigation.html.erb new file mode 100644 index 000000000..690ad6820 --- /dev/null +++ b/test/sandbox/app/views/shared/_deep_navigation.html.erb @@ -0,0 +1,11 @@ +<%= render SectionComponent.new do |section| %> + <% section.with_nav do |nav| %> + <% nav.with_action_menu do |menu| %> + <% menu.with_list do |list| %> + <% list.with_item do %> + <%= t(".deep_action") %> + <% end %> + <% end %> + <% end %> + <% end %> +<% end %> diff --git a/test/sandbox/config/locales/en.yml b/test/sandbox/config/locales/en.yml index 46f26a502..79172933a 100644 --- a/test/sandbox/config/locales/en.yml +++ b/test/sandbox/config/locales/en.yml @@ -28,6 +28,10 @@ en: shared: partial: title: Partial Title + action_menu_panel: + menu_action: Menu Action from Partial + deep_navigation: + deep_action: Deep Action from Partial (5 levels!) rendering_test: i18n_test_component: diff --git a/test/sandbox/test/components/deeply_nested_translation_test.rb b/test/sandbox/test/components/deeply_nested_translation_test.rb new file mode 100644 index 000000000..96e61cd31 --- /dev/null +++ b/test/sandbox/test/components/deeply_nested_translation_test.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require "test_helper" + +# Tests that translations in deeply nested component blocks (3+ levels) resolve to the +# partial's scope, not an intermediate component's scope. +# +# This pattern is inspired by Primer's ActionMenu → ActionList → Item hierarchy. +class DeeplyNestedTranslationTest < ViewComponent::TestCase + # Tests 3 levels of nesting with typical ViewComponent slot pattern + # + # Structure: + # Partial (_action_menu_panel.html.erb) + # └─> ActionMenuComponent (Level 1) + # └─> menu.with_list → ActionListComponent (Level 2) + # └─> list.with_item → MenuItemComponent (Level 3) + # └─> t(".menu_action") ← Translation should resolve to partial's scope + # + # This uses the standard ViewComponent slot DSL: + # menu.with_list creates an ActionListComponent slot + # list.with_item creates a MenuItemComponent slot + # + # Without the fix, the translation would incorrectly resolve to ActionListComponent's + # scope instead of the partial's scope. + def test_translation_in_3_level_nested_blocks + result = render_inline(ActionMenuPanelWrapperComponent.new) + + assert_includes result.to_html, "Menu Action from Partial" + end + + # Tests 5 levels of nesting to prove the fix works for arbitrary depth + # + # Structure: + # Partial (_deep_navigation.html.erb) + # └─> SectionComponent (Level 1) + # └─> section.with_nav → NavComponent (Level 2) + # └─> nav.with_action_menu → ActionMenuComponent (Level 3) + # └─> menu.with_list → ActionListComponent (Level 4) + # └─> list.with_item → MenuItemComponent (Level 5) + # └─> t(".deep_action") ← Translation should resolve to partial's scope + # + # All using typical ViewComponent slot DSL with renders_one/renders_many. + # + # This demonstrates that the fix isn't just solving "one level deeper" than the original fix, + # but works for any depth of nesting. + def test_translation_in_5_level_nested_blocks + result = render_inline(DeepNavigationWrapperComponent.new) + + assert_includes result.to_html, "Deep Action from Partial (5 levels!)" + end +end From 40d92420356def5fdfa1628110e5d76c5ca020db Mon Sep 17 00:00:00 2001 From: Nathaniel Watts Date: Fri, 19 Dec 2025 19:21:19 -0600 Subject: [PATCH 2/2] Add changlog entry --- docs/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 28362dc41..80fef2040 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -10,6 +10,10 @@ nav_order: 6 ## main +* Fix translation scope resolution in deeply nested component blocks (3+ levels). Translations called inside deeply nested slot blocks using `renders_many`/`renders_one` were incorrectly resolving to an intermediate component's scope instead of the partial's scope where the block was defined. The fix captures the virtual path at block definition time and restores it during block execution, ensuring translations always resolve relative to where the block was created regardless of nesting depth. Builds on #2389. Fixes #2386. + + *Nathaniel Watts* + * Allow `render_inline` with Nokogiri::HTML5 to parse more arbitrary content including bare table content otherwise illegal fragments like ``. *Jonathan Rochkind*