Skip to content

perf: widget counters, conditional Focus, Container primitives, size-aspect MediaQuery - #225

Merged
anilcancakir merged 6 commits into
masterfrom
feat/llm-perf-tracing
Sep 29, 2026
Merged

anilcancakir merged 6 commits into
masterfrom
feat/llm-perf-tracing

Conversation

@anilcancakir

@anilcancakir anilcancakir commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

What

  • Perf counters: W-widget builds, wrapper emissions and inherited reads, published through profile-safe resolvers.
  • A gestureless WAnchor around a WDiv installs Focus only when the className carries focus: (WAnchor.trackFocus).
  • WDiv's non-animated box is built from Container.build's own primitives, in the same order; geometry is pinned against Container.
  • Every size read is MediaQuery.sizeOf: WindContext, the h-full paths, the grid fallback, wScreenIs, wScreenCurrent, WPopover, WSelect.

Why

  • Measured in uptizm: nearly half the anchors carried a focus node nothing could show, every decorated div paid one extra element, and each frame of a keyboard animation rebuilt every styled widget because the whole MediaQueryData was a dependency.

Testing

  • flutter test green (1892), coverage 95.5%, tool/check-docs.py clean.
  • Before/after counts on Chrome and an Android profile build: wrapperEmissions.Focus per WAnchor 1.00 to 0.62, wrapperEmissions.Container per WDiv 1.14 to 0.10 on the dashboard.
  • Downstream: tests that found a fill through find.byType(Container) must read DecoratedBox (fixed in magic_starter in this set).

Summary by CodeRabbit

  • New Features
    • Added focus-tracking controls for anchors. Hover- and active-only elements remain outside keyboard and remote-control navigation, while focusable descendants can still focus an ancestor wrapper.
    • Expanded performance diagnostics with per-widget build, wrapper-emission, and inherited-read counters, available in profile builds.
  • Improvements
    • Refined box layout and decoration rendering while preserving animated layouts and sizing behavior.
    • Screen-size lookups avoid unnecessary rebuilds when unrelated media settings, such as keyboard insets, change.

…m primitives, size-aspect MediaQuery

A gestureless WAnchor around a WDiv installs Focus only when the className
carries focus:. WDiv's non-animated Container is replaced by Container.build's
own primitives in the same order, skipping a zero Padding. WindContext and
WDiv read MediaQuery.sizeOf, so a keyboard inset no longer rebuilds every
styled widget.
… only

Both read MediaQuery.of(context).size, so a shell that swaps its tree on
wScreenIs(context, 'lg') rebuilt on every frame of a keyboard animation.
They now read MediaQuery.sizeOf, as do WPopover's and WSelect's flip checks;
wind no longer reads the whole MediaQueryData anywhere.
The widgets rule still described the Container WDiv no longer builds, and
w-anchor did not say that adding or dropping a focus: class at runtime
remounts the children below.
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 520bf349-8655-4ef0-af62-e58342e6662b

📥 Commits

Reviewing files that changed from the base of the PR and between df6b5a2 and a8cdf75.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • doc/widgets/w-anchor.md
  • lib/src/debug_resolver.dart
  • skills/wind-ui/references/widgets.md
  • test/debug_resolver_test.dart
  • test/utils/wind_perf_counters_test.dart
  • test/widgets/w_anchor/dpad_activation_test.dart
🚧 Files skipped from review as they are similar to previous changes (3)
  • skills/wind-ui/references/widgets.md
  • CHANGELOG.md
  • doc/widgets/w-anchor.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The pull request adds performance counters across Wind widgets, changes WDiv focus tracking and non-animated box composition, uses size-specific MediaQuery reads, and enables debug and performance resolvers in profile builds.

Changes

Widget layout, focus, and performance

Layer / File(s) Summary
Performance counter contract and widget instrumentation
lib/src/utils/wind_perf_counters.dart, lib/src/parser/wind_context.dart, lib/src/utils/wind_helpers.dart, lib/src/widgets/*, test/utils/*, test/parser/wind_context_test.dart, doc/core-concepts/debugging.md, CHANGELOG.md
Adds per-widget build and wrapper-emission maps and typed inherited-read counts. Wind widgets record these events. Screen-size reads use MediaQuery.sizeOf, with tests for size and keyboard-inset updates.
WDiv focus and box composition
lib/src/widgets/w_anchor.dart, lib/src/widgets/w_div.dart, test/widgets/w_anchor/*, test/widgets/w_div/*, test/parser/parsers/shadow_parser_test.dart, doc/widgets/*, skills/wind-ui/*, .claude/rules/widgets.md
Adds WAnchor.trackFocus and has WDiv set it from its focus class. Non-animated WDiv boxes use Flutter primitives instead of Container; tests cover focus, geometry, decoration, shadows, and transitions.
Resolver availability and documentation
lib/src/wind_facade.dart, lib/src/debug_resolver.dart, test/debug_resolver_test.dart, test/wind_facade_test.dart, doc/core-concepts/debugging.md, skills/wind-ui/references/debug.md, CHANGELOG.md
Resolver installation is gated by release mode, so profile builds can install the resolvers. Debug resolution temporarily disables performance counters and restores their prior enabled state.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to a8cdf

No actionable issue remains from the supplied evidence; the PR is ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to a8cdf

Profile builds can now opt into widget-state inspection that was previously limited to debug builds. Release builds remain excluded, but access to diagnostics in deployed profile builds needs an explicit policy. No externally reachable attack path was established.

Retained concerns

  • Medium · security · inferred: Profile builds can now register a resolver that exposes per-widget diagnostic state to registry consumers. This is an intentional expansion from debug-only behavior, but whether profile hosts restrict inspector access is unverified; exposure depends on host installation and access controls.
Security review details

Security Blast Radius

  • inferred — The newly eligible scope is an opted-in profile-build app process and its inspectable widget Elements, not release builds. No cross-process or remote reachability is demonstrated by the inspected library code.

Security Findings and Attack Paths

  • inferred — A consumer able to access an installed registry and supply a widget Element could obtain its diagnostic fields in profile mode. The repository evidence does not establish that an untrusted consumer has those capabilities.

Trust Boundaries and Controls

  • observed — Host opt-in and the release-mode refusal remain local controls. The PR relaxes the former debug-only build-mode boundary; authorization at any downstream inspector boundary is not shown.

Resilience and Maintainability Implications

  • inferred — The saved-flag and finally pattern preserves measurement-session state after normal resolution or an exception. A synchronously reentrant counter consumer would be suppressed during resolution, but no such production callback was identified.

Hardening Proposals

  • proposed — For any deployed profile host that enables snapshots, restrict who can invoke the inspector and document the intended access policy for widget-state diagnostics.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: performance counters, conditional Focus, primitive-based WDiv boxes, and MediaQuery.sizeOf usage. It is concise and specific, although the phrase "siz…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kodizm

kodizm Bot commented Sep 29, 2026

Copy link
Copy Markdown

Note

Kodizm (AI-generated). May contain mistakes; verify before acting.

Looks correct overall: _buildBox matches Container.build step for step, and the sizeOf swaps are safe. One behaviour change in the conditional Focus goes further than the PR describes.

Major

lib/src/widgets/w_div.dart:151: correctness. trackFocus looks only at the div's own className. But every W-widget below the div reads isFocused from this anchor's provider through WindContext.build (wind_context.dart:85), and that value came from _focusNode.hasFocus, which is true while any descendant is focused. Here is a case that breaks: WDiv('hover:bg-gray-50', children: [WText('Email', className: 'focus:text-blue-600'), WInput(...)]). Before this PR the label turned blue while the input had focus. Now the anchor never attaches a node, _isFocused stays false, and the label never lights. A top-level hover-only div also stops being a Tab stop, since it used to get canRequestFocus: true when inherited == null. If dropping both is intended, the CHANGELOG / doc/widgets/w-anchor.md should say so. If not, the gate would have to consider descendants too, which a string check can't do.

Minor

lib/src/wind_facade.dart:53: installDebugResolver now also runs in profile builds, and WindDebugResolverImpl.resolve calls WindContext.build(element), which now calls recordInheritedRead. With counters enabled during a profiling session, every diagnostic snapshot inflates inheritedReads.windTheme / mediaQuerySize with reads that no build caused.

Tests

The new counters (wind_perf_counters_test.dart), the sizeOf dependency (wind_context_test, wind_helpers_test), the Container geometry parity (w_div/sizing_test.dart) and the no-focus: anchor (dpad_activation_test.dart) are covered. Nothing covers the focus-within loss described above.

CI

  • Lint & Test: success
  • Internal Links & Previews: success
  • Workflows lint: success
  • Diff CVEs on PR: success
  • CodeRabbit: success
  • codecov/patch: failure, 93.58% of the diff is covered against a 95.45% target. This is not a required check, but CLAUDE.md asks for tests in the same change set when coverage drops.
  • Demo build/deploy, External Links, Dependabot: skipped

Not reviewed line by line: the one-line recordWidgetBuild / recordWrapperEmission additions in the other W-widgets, the docs/skill/CHANGELOG edits, and the test-file rewrites from Container to DecoratedBox.

…ing, keep resolver snapshots out of the perf counters
@anilcancakir

Copy link
Copy Markdown
Member Author

Round 1 fixes, all in a8cdf75:

  • Major, lib/src/widgets/w_div.dart:151 (focus-within lost under a hover-only div): confirmed, and kept as the intended trade rather than reverted. A className string cannot see what its descendants style, so a gate that considers them is not available, and restoring the node on every hover-only div gives back the cost this PR measured (about 193 of 399 anchors per frame). The Tab and D-pad stop drop was already documented in the CHANGELOG and doc/widgets/w-anchor.md:145; the sibling-label case was not. Now:
    • CHANGELOG.md (the ### Changed entry for hover-only divs) names your exact example and says the label now stays unlit, and that adding any focus: class to the row brings the node back.
    • doc/widgets/w-anchor.md gains a paragraph with the example and the one-class way back (hover:bg-gray-50 focus:bg-gray-50).
    • skills/wind-ui/references/widgets.md:266 says the same.
    • Pinned by two tests in test/widgets/w_anchor/dpad_activation_test.dart: a sibling label under "p-2 hover:bg-gray-50" does not read the field's focus and ... under "p-2 hover:bg-gray-50 focus:bg-gray-50" reads the field's focus.
  • Minor, lib/src/wind_facade.dart:53 (snapshots inflate the counters in profile): fixed in lib/src/debug_resolver.dart. resolve suspends WindPerfCounters.enabled around its own WindContext.build and WindParser.parse and restores it in finally, which also keeps the snapshot's parse out of the cache counters. Reproduced first: test/debug_resolver_test.dart resolving a widget records nothing in the counters failed before the change and passes now.
  • codecov/patch 93.58%: the 12 uncovered diff lines were the rarer wrapper and read sites (WText padding, margin, align-self, Expanded, Flexible; the clipped-padding path; the two fractional max-* clamps; the grid's unbounded-width size read; WInput's platform-brightness fallback). A new group in test/utils/wind_perf_counters_test.dart drives each one and asserts its counter. Measured locally from coverage/lcov.info against git diff origin/master -- lib: 189 of 189 diff lines hit.

Gates: flutter analyze no issues, dart format 0 changed, ./tool/coverage.sh 90 all tests passed at 96.0%, python3 tool/check-docs.py 0 issues.

@kodizm please re-review.

@kodizm

kodizm Bot commented Sep 29, 2026

Copy link
Copy Markdown

Note

Kodizm (AI-generated). May contain mistakes; verify before acting.

Looks good to merge: a8cdf75 deals with both earlier findings, and I found nothing new in it.

Since the last review, I read only a8cdf75 (df6b5a2 to head).

  • The focus-within change (earlier Major): you've explained that this is a deliberate trade, so it's settled. CHANGELOG.md, doc/widgets/w-anchor.md and skills/wind-ui/references/widgets.md now describe the unlit label and how to get it back with one focus: class. The two new cases in dpad_activation_test.dart test both sides.
  • Snapshots inflating the counters (earlier Minor): fixed. lib/src/debug_resolver.dart:35 saves WindPerfCounters.enabled, turns it off for the WindContext.build and WindParser.parse calls, and puts the saved value back in finally. So a throwing parse cannot leave counting switched off, and the test checks that enabled is still true afterwards.

Tests

debug_resolver_test.dart checks that resolve adds nothing to the inherited reads or the cache hit/miss counters. The label cases are tested in both directions. The new group in wind_perf_counters_test.dart covers the rarer wrapper and read sites.

CI

  • Lint & Test: success
  • codecov/patch: success, 100.00% of diff hit (target 95.45%)
  • Internal Links & Previews: success
  • Workflows lint: success
  • Diff CVEs on PR: success
  • CodeRabbit: success
  • Skipped: Deploy Demo, Build Demo, Detect demo-affecting changes, External Links, Dependabot auto-merge

I did not read this commit's new wind_perf_counters_test.dart cases line by line, only the other files it changed.

@anilcancakir
anilcancakir merged commit e9bad1b into master Sep 29, 2026
12 checks passed
@anilcancakir
anilcancakir deleted the feat/llm-perf-tracing branch September 29, 2026 10:35
@anilcancakir anilcancakir mentioned this pull request Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant