[kcjonson]

Summary

• Add 30x30 toggle button in bottom-right corner of the sandbox
• Menu expands upward when the toggle button is clicked
• Menu automatically closes after selecting a scene
• Menu starts collapsed by default, reducing UI clutter

Test plan

• Launch sandbox without --scene argument
• Verify toggle button appears in bottom-right corner
• Click toggle button to expand menu
• Select a scene and verify menu closes
• Verify menu position updates correctly on window resize

[Copilot]

Pull request overview

This PR refactors the navigation menu in the UI sandbox application to improve UX by relocating it to the bottom-right corner with a collapsible toggle mechanism. The menu now starts collapsed by default to reduce UI clutter and expands upward when the user clicks the toggle button.

Key Changes:

• Added a 30x30 toggle button anchored to the bottom-right corner that controls menu visibility
• Modified menu positioning to expand upward from the bottom-right instead of being fixed at top-left
• Implemented auto-collapse behavior when a scene is selected from the menu

Comments suppressed due to low confidence (1)

apps/ui-sandbox/main.cpp:141

• The header text position is only set once during initialization and won't update when the window is resized. While the menu background rectangles are redrawn at the correct positions in RenderNavigationMenu, the UI::Text object has a fixed position that won't align with the new menu position after a resize. This will cause the "Scenes" header to appear disconnected from the menu background.

	g_menuState.headerText = Text{
		.position = {menuX + 10, menuY + 8},
		.text = "Scenes",
		.style = {.color = Color(0.9F, 0.9F, 0.9F, 1.0F), .fontSize = 16.0F},
		.zIndex = 110.0F,
		.id = "menu_header_text"

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The toggle button and menu button positions are only set once during initialization and won't update when the window is resized. While RenderNavigationMenu recalculates menu background positions dynamically, the UI buttons (created via UI::Button) have fixed positions from initialization. This means the toggle button will remain at its initial position after a window resize, breaking the intended bottom-right anchoring. Consider calling InitializeNavigationMenu() in FramebufferSizeCallback, or add a method to update button positions on resize.

[Copilot]

Magic number 5.0F is used for the gap between menu and toggle button. Consider extracting this to a named constant (e.g., kMenuToggleGap) for better maintainability and to ensure consistency with the duplicate calculation in RenderNavigationMenu (line 203).

[Copilot]

The menu dimension constants (kMargin, kToggleSize, kMenuWidth, kLineHeight, kHeaderHeight) and positioning calculations are duplicated between InitializeNavigationMenu and RenderNavigationMenu. This creates a maintenance burden as changes must be synchronized in both places. Consider extracting these constants to a shared location (e.g., file-level constants) or creating a helper function to calculate menu positions.

[kcjonson]

This is too much in the main file here, lets refactor so that all the menu bits are in their own class.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The .c_str() call on a temporary string creates a dangling pointer. The temporary string ("menu_button_" + std::to_string(i)) is destroyed at the end of this expression, leaving id pointing to freed memory.

Consider storing the ID string and then passing its c_str(), or pass the ID string directly if the Button::Args accepts std::string. For example:

std::string buttonId = "menu_button_" + std::to_string(i);
m_menuButtons.push_back(
    Button{Button::Args{
        .label = sceneName,
        .position = {m_menuX + 2, itemY + 2},
        .size = {kMenuWidth - 4, kLineHeight - 4},
        .type = Button::Type::Secondary,
        .onClick = [this, sceneName]() { /* ... */ },
        .zIndex = 100.0F,
        .id = buttonId.c_str()
    }}
);

Or if Button stores a copy of the string internally, this may work as-is depending on the Button constructor implementation.

[Copilot]

[nitpick] Calling Update(0.0F) on buttons inside HandleInput() is inconsistent with the typical lifecycle pattern used elsewhere (e.g., button_scene.cpp). The standard pattern is:

• HandleInput() processes input events only
• Update(deltaTime) updates state based on time

Currently, buttons are updated here with hardcoded 0.0F delta time, but NavigationMenu::Update() doesn't update the buttons. Consider moving these Update() calls to the Update() method and passing through the actual deltaTime parameter to maintain consistency with other components.

Suggested change

	m_toggleButton->Update(0.0F);
	}
	// Only handle menu buttons when expanded
	if (m_expanded) {
	for (auto& button : m_menuButtons) {
	button.HandleInput();
	button.Update(0.0F);
	}
	}
	}
	void NavigationMenu::Update(float /*deltaTime*/) {
	// Currently no time-based updates needed
	}
	// Only handle menu buttons when expanded
	if (m_expanded) {
	for (auto& button : m_menuButtons) {
	button.HandleInput();
	}
	}
	}
	void NavigationMenu::Update(float deltaTime) {
	// Update toggle button
	if (m_toggleButton) {
	m_toggleButton->Update(deltaTime);
	}
	// Update menu buttons if expanded
	if (m_expanded) {
	for (auto& button : m_menuButtons) {
	button.Update(deltaTime);
	}
	}