UiStack: per-child layout and optional ownership of a focus scope

[humanapp]

Problem: A screen that wants several rows of controls, each centered on its own width with headers between them, could not express that. UiGrid places every row from the same left edge with uniform widths, and rows added to a screen separately each own a focus scope, making navigation between them an app-layer, hand-wired concern.

Fix: UiStack gains new functionality:

• Layout. Children are records rather than bare views, so alignment and spacing can belong to an individual child instead of the whole stack.
• Focus. Given a scopeId, the stack owns one focus scope for its children and answers arrow keys for all of them, so a column of independently sized rows navigates as a single ragged grid.

UiRow, UiGrid and UiButton implement the new composable view interface from ui-core, which is what lets a stack drive them. Their own scopeId becomes optional, so a screen names the scope once on the stack rather than once per row.

Also adds a README section and two sample screens illustrating the feature.

[Copilot]

🟡 Not ready to approve

Several views now allow scopeId to be omitted, but their types and focusDefault() behavior don’t consistently handle undefined, which can cause type-safety issues and unclear runtime focus behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR enhances UiStack to support per-child layout options and (optionally) own a single focus scope that composes navigation across its children, enabling “ragged grid” navigation for independently-sized rows without app-layer wiring.

Changes:

• UiStack children become records (UiStackChild) with per-child alignment and spacing, plus new justify behavior.
• UiStack can own a focus scope (scopeId) and provide composed directional navigation; UiRow, UiGrid, and UiButton implement the composable focus interface to participate.
• Adds README guidance and new sample screens demonstrating stacked layout + composed focus, and bumps ui-core dependency.

File summaries

File	Description
stack.ts	Adds per-child layout options, justify, and optional focus-scope ownership with composed navigation.
row.ts	Makes scopeId optional for stack-owned scopes and adds composable focus support helpers.
grid.ts	Makes scopeId optional for stack-owned scopes and adds composable focus support helpers.
button.ts	Adds composable focus support and split render passes (controls vs focus overlay).
samples.ts	Adds sample screens demonstrating stack-owned scope navigation and layout.
README.md	Documents how to stack rows/grids into a single focus scope and use per-child layout.
pxt.json	Bumps ui-core dependency to v0.0.8.

Review details

Suppressed comments (6)

stack.ts:142

• The scopeId getter is documented as possibly undefined for layout-only stacks, but its return type is UiFocusScopeId (non-optional). This should be UiFocusScopeId | undefined to match behavior and UiStackOptions.scopeId?:.

        public get scopeId(): UiFocusScopeId {
            return this.scopeId_
        }

stack.ts:460

• focusChild returns undefined when a child isn't composable-focus aware, but the return type is declared as UiComposableFocusView<any> (non-optional). This should be an optional return type to accurately represent the function contract.

        private focusChild(index: number): UiComposableFocusView<any> {

row.ts:124

• UiRowOptions.scopeId is optional, but the scopeId getter is typed as non-optional. Returning UiFocusScopeId | undefined better reflects actual state before a parent assigns a scope via setScopeId().

        public get scopeId(): UiFocusScopeId {
            return this.scopeId_
        }

        /**

row.ts:293

• Now that scopeId can be omitted (for stack-owned scopes), focusDefault() should handle scopeId_ === undefined the same way registerFocusTargets() does; otherwise callers can end up calling setActiveScope(undefined) with unclear downstream behavior.

        public registerNavigation(controller: UiFocusInputController): void {
            if (this.scopeId_ === undefined) return
            controller.setNavigation(this.scopeId_, {
                kind: "row",
                targets: this.navigationTargets(),

grid.ts:102

• UiGridOptions.scopeId is optional, but the scopeId getter is typed as non-optional. Returning UiFocusScopeId | undefined better reflects actual state before a parent assigns a scope via setScopeId().

        public get scopeId(): UiFocusScopeId {
            return this.scopeId_
        }

        /**

grid.ts:229

• Now that scopeId can be omitted (for stack-owned scopes), focusDefault() should handle scopeId_ === undefined the same way registerFocusTargets() does; otherwise callers can end up calling setActiveScope(undefined) with unclear downstream behavior.

        public registerNavigation(controller: UiFocusInputController): void {
            if (this.scopeId_ === undefined) return
            controller.setNavigation(this.scopeId_, {
                kind: "raggedGrid",
                rows: this.navigationRows(),

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

UiStack currently short-circuits focus registration/navigation when scopeId is omitted, which can silently disable focus for focusable children used under a layout-only stack.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

stack.ts:221

• arrange() measures every child twice when justify is left at its default of "start" (once via contentMainSize() and again inside the placement loop). Skipping the contentMainSize() pass when no justification is applied avoids a full extra measurement pass in the common case.

            const available = row ? this.finalRect.width : this.finalRect.height
            const content = this.contentMainSize()
            const slack = Math.max(0, available - content)
            let pos = (row ? this.finalRect.x : this.finalRect.y) +
                this.justifyOffset(slack)

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

There are API/type-safety issues that can break downstream consumers (e.g., required children typing and undefined scopeId_ use in preferred-target resolution) that should be corrected before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

stack.ts:227

• arrange() measures each child twice: once inside contentMainSize() (called earlier in this method) and again in the per-child loop. If measure() is non-trivial (common for composite views), this doubles layout work every time the stack is arranged. Consider caching the measured sizes from the first pass (e.g., into reusable arrays) and reusing them during arrangement.

                const child = this.children_[i]
                const alignment = child.alignment || this.alignment_
                child.view.measure(this.childConstraints_, this.childSize_)
                const childWidth = this.childSize_.preferredWidth
                const childHeight = this.childSize_.preferredHeight

stack.ts:53

• UiStackOptions.children is declared as required, but UiStack still treats it as optional (this.setChildren(options.children) and children || []). This is an API-breaking type change for callers that previously relied on the default empty children list; the type should reflect the runtime behavior by making children optional.

        children: UiStackChild[]

row.ts:306

• Because scopeId_ can now be undefined, _uiControls.preferredControlId(scopeId: string, ...) can be called with an undefined scope via resolvePreferredTargetId() (immediately below this method). This can fail type-checking under strictNullChecks and can produce invalid target ids at runtime if called before a parent assigns a scope. Guard resolvePreferredTargetId() to return undefined when scopeId_ is missing.

        public focusDefault(focus: UiFocusState): UiFocusSetResult {
            if (this.scopeId_ === undefined)
                return { kind: "rejected", reason: "missingScope" }
            return focus.setActiveScope(this.scopeId_)
        }

grid.ts:242

• Because scopeId_ can now be undefined, _uiControls.preferredControlId(scopeId: string, ...) can be called with an undefined scope via resolvePreferredTargetId() (immediately below this method). This can fail type-checking under strictNullChecks and can produce invalid target ids at runtime if called before a parent assigns a scope. Guard resolvePreferredTargetId() to return undefined when scopeId_ is missing.

        public focusDefault(focus: UiFocusState): UiFocusSetResult {
            if (this.scopeId_ === undefined)
                return { kind: "rejected", reason: "missingScope" }
            return focus.setActiveScope(this.scopeId_)
        }

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

There are correctness/perf gaps in the new stack composition path (runtime guard robustness and avoidable extra measurement work) plus missing automated coverage for the new composed-focus behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

stack.ts:499

• focusChild() only checks for navigationRows and registerFocusTargets, but UiStack later calls resolvePreferredTargetId() on the returned child (via resolvePreferredTargetId() and also constructs stack scope options). If a view partially implements the composable focus surface, this will throw at runtime. Consider tightening the guard to ensure the full required surface is present before treating a child as focus-composable.

        private focusChild(
            index: number,
        ): UiComposableFocusView<any> | undefined {
            const child = <any>this.children_[index].view
            if (!child.navigationRows || !child.registerFocusTargets)
                return undefined
            return <UiComposableFocusView<any>>child

stack.ts:222

• arrange() always calls contentMainSize(), which measures every child once, and then measures every child again inside the arrangement loop. For the common/default case (justify: "start"), the slack/offset/spacing are always zero, so the extra full measurement pass can be skipped to avoid an avoidable per-frame cost.

            const row = this.orientation_ == "row"
            const available = row ? this.finalRect.width : this.finalRect.height
            const content = this.contentMainSize()
            const slack = Math.max(0, available - content)
            let pos = (row ? this.finalRect.x : this.finalRect.y) +
                this.justifyOffset(slack)
            const spacing = this.justifySpacing(slack)

stack.ts:296

• UiStack now owns an optional focus scope and composes child focus surfaces (target registration + ragged-grid navigation + wrapping/verticalStrategy), but there are no automated tests exercising this new behavior. Adding a smoke/unit test that builds a stack with a mix of UiRow/UiGrid/UiButton and verifies cross-child directional movement (including hidden/disabled targets) would help prevent regressions.

        public registerFocusTargets(
            focus: UiFocusState,
            scopeOptions?: UiFocusScopeOptions,
        ): void {
            if (this.scopeId_ === undefined) {

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

UiStack.focusChild() currently silently drops focusable-but-non-composable children in a scoped stack (contradicting its own comment), which can make such children unreachable without an explicit failure mode.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

stack.ts:516

• In a scope-owning stack, children are only registered/navigated when focusChild() returns them. As written, focusChild() silently returns undefined for any focusable child that lacks one of the required composable members, which means that child won’t register focus targets at all (so it becomes unreachable) even though the comment says it’s “left to itself rather than half driven”. Consider failing fast with an assertion when a child looks focusable (has registerFocusTargets) but doesn’t implement the full UiComposableFocusView surface, so this doesn’t degrade silently.

            const child = <any>this.children_[index].view
            // The whole composable surface is required, since this stack calls
            // every part of it. A view offering only some of it is left to
            // itself rather than half driven.
            if (
                !child.navigationRows ||
                !child.registerFocusTargets ||
                !child.resolvePreferredTargetId ||
                !child.setScopeId
            )
                return undefined

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

It changes core layout and focus-scope/navigation behavior across multiple fundamental controls (and bumps ui-core), so it warrants final human validation in a real app flow beyond the added smoke tests.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.