Skip to content

feat(perf): wire MagicPerfHooks, interaction links and insight rules - #22

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

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

Conversation

@anilcancakir

Copy link
Copy Markdown
Member

What

  • Wires MagicPerfHooks into dusk and telescope: id-paired HTTP, interaction links, wind and magic insight rules.
  • A query reload, action or event dispatch links to the interaction it STARTED in, not the one open when it ended.

Why

  • The perf report attributes cost to the gesture and the framework site; a span linked at its end blamed the wrong tap.

Testing

  • flutter test green (135) against magic, dusk and telescope from this set. Release after them.

A query reload, an action and an event dispatch are reported when they end,
so linking them then dropped a reload that outlived its tap and gave one that
ended during the next tap to the wrong interaction. They now resolve from
their startUs: the zone handle counts when its window holds the start, closed
or not, then dusk's perfInteractionAt, then the window. The mediaQuerySize
insight now describes the MediaQuery.sizeOf reads wind makes.
@kodizm

kodizm Bot commented Sep 29, 2026

Copy link
Copy Markdown

Note

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

Don't merge this yet: it compiles against magic, dusk and telescope APIs that the committed pubspec.yaml can't resolve, and CI is red.

Critical

pubspec.yaml:15: The dependency constraints are unchanged (magic: ^0.0.22, fluttersdk_dusk: ^0.0.16, fluttersdk_telescope: ^0.0.7). A 0.0.x caret pins the patch version, so pub can never pick up the releases this code needs. Those releases provide MagicPerfHooks, PerfMode, perfInteractionAt, perfTimelineReader, perfInsightContributors, MagicRequest.id and the interactionId/linkedBy/startUs fields on telescope records. Without them master stops compiling for anyone who doesn't have a local pubspec_overrides.yaml. The PR says "release after them", but the constraints still have to be raised in this PR (or a follow-up that lands first) before it can merge. (correctness)

Minor

lib/src/dusk_integration.dart:11: The host example now gates with !kReleaseMode, so profile builds carry the tools. The rest of the package still says kDebugMode: CLAUDE.md ("guarded by kDebugMode AT THE CALL SITE"), README.md:61-91, lib/dusk.dart:16 and lib/src/magic_devtools.dart:29-38. A consumer now gets two conflicting instructions. If profile support is intended, update those places too; otherwise revert this doc change. (maintainability)

lib/src/telescope_integration.dart:246: A response without an id (an Http.fake answer) takes _pending[0] even when that entry is a real request that has an id. That request's own response then finds no match at index == -1 and is silently dropped. This only happens when fake and real requests are in flight together, so I'm not fully sure how often it bites. Taking the oldest pending entry whose id == null would avoid it. (correctness)

Tests

New tests cover the insight rules (test/perf_insight_rules_test.dart), the session sink and interaction linking (test/perf_integration_test.dart) and id-paired HTTP (test/telescope_integration_test.dart). I only read the test manifest, not the test bodies. The rule code itself is guarded against zero denominators (painted == 0, builds == 0) as far as I read.

CI

  • Lint & Test: failure. The annotation only says "Process completed with exit code 1"; details are in the job log. This fits the unresolved dependency constraints above, but I can't read the log to confirm it.
  • Auto-merge low-risk Dependabot PRs: skipped.

@anilcancakir

Copy link
Copy Markdown
Member Author

Round 1, in 1d4735b:

  • Critical, pubspec.yaml:15 (floors cannot resolve the APIs this uses): agreed, and left open on purpose. The floors can only name versions that exist on pub.dev, and magic (with #210 merged), dusk (#48) and telescope (#38 merged) have not been released yet; the maintainer asked to hold releases for now. The floors move in this batch's release, after those three publish, and this PR merges only then. CI stays red until that point for the same reason.
  • Minor, lib/src/dusk_integration.dart:11 (!kReleaseMode here, kDebugMode everywhere else): the whole package now says !kReleaseMode, matching dusk#48, which moves dusk:install and its docs the same way. Changed: CLAUDE.md (the call-site rule and Off-limits 5), README.md (overview, dependency note and every install snippet above the preview section), lib/src/magic_devtools.dart:29-42, lib/dusk.dart:16,20, lib/telescope.dart:16,20, lib/src/telescope_integration.dart:14, lib/magic_devtools.dart:5, plus a CHANGELOG ### Changed bullet. The preview catalog keeps its own gate (kReleaseMode plus PREVIEW_ENABLED, defaulting to kDebugMode); that is a separate switch and did not change.
  • Minor, lib/src/telescope_integration.dart:246 (an id-less answer takes a real request): fixed. _record now takes the oldest pending entry whose id equals the answer's, so an id-less answer pairs only with an id-less request. The class docblock also said an Http.fake answer takes this path; FakeNetworkDriver.addInterceptor is a no-op, so fake traffic never reaches the interceptor, and the docblock and CHANGELOG now say that. Reproduced first: an answer without an id never takes a request that has one in test/telescope_integration_test.dart failed (Expected: <201> Actual: <null>) and passes now.

Gates, against the local perf set through a gitignored pubspec_overrides.yaml (magic at #210's head, telescope at #38's head, dusk at #48's head, wind at #225's head): flutter analyze no issues, dart format 0 changed, flutter test 136 passed.

@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.

Round 1 is resolved. The code in 1d4735b looks correct, and the one thing still blocking the merge is the dependency floors, which you're holding open on purpose.

Since my last review, 1d4735b fixes both Minor findings. The Critical one was answered.

  • pubspec.yaml:15 (constraints can't resolve the new APIs): your reply settles it. The floors move in the release batch after magic, dusk and telescope publish, and this PR merges only then. I won't raise it again. Until then, master would not compile if this merged early.
  • The guard is now !kReleaseMode everywhere: I checked CLAUDE.md (the call-site paragraph and golden rule 5), README.md, lib/magic_devtools.dart, lib/dusk.dart, lib/telescope.dart, lib/src/magic_devtools.dart and lib/src/telescope_integration.dart. The only places that still say kDebugMode are in the preview catalog (README.md:111, lib/preview.dart:18, preview_routes.dart). That catalog has its own kReleaseMode + PREVIEW_ENABLED switch, so those are correct as they are. There's a CHANGELOG ### Changed bullet, as rule 3 asks.
  • Pairing an answer without an id (lib/src/telescope_integration.dart:249): _pending.indexWhere((p) => p.id == id) now covers both cases with one rule. An answer with no id matches only the oldest request that also has no id, so a request that has an id can no longer be taken by mistake and then lose its own response. The updated docblock is also accurate: Http.fake traffic never reaches this interceptor.

Tests

an answer without an id never takes a request that has one in test/telescope_integration_test.dart covers the pairing fix. It checks that each record got the right URL and status, that the id-less one is flagged attributedHeuristically, and that nothing is left pending. The guard change only touches docs, so it has no test.

CI

  • Lint & Test: failure on 1d4735b, which matches the local checkout. The annotation only says "Process completed with exit code 1", so I can't see the log. That's consistent with the unresolved dependency floors above, but I haven't confirmed it.
  • Auto-merge low-risk Dependabot PRs: skipped.

The adapter stringified bodies with toString(), and a Dart Map prints as
{email: a@b.test, password: hunter2}, which telescope's JSON masking
cannot read, so a login body and a Sanctum login answer reached the
agent-facing buffer in the clear. Request, response and error bodies are
now masked with telescope's hidden parameter lists before they are cut
to 8 KB, without touching the request object the driver sends on.

install() also hides the header auth.token.header names (default
Authorization), the one magic's AuthInterceptor writes the token under,
on every call since a store reset drops the addition.
@kodizm

kodizm Bot commented Sep 29, 2026

Copy link
Copy Markdown

Note

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

The new commit 675a2d4 looks correct and I have no new findings. The one thing still blocking the merge is the dependency floors, which you're holding open on purpose.

This was an incremental review of 675a2d4 only: CHANGELOG.md, README.md, lib/src/telescope_integration.dart and test/telescope_integration_test.dart. The pubspec.yaml floors were settled in the last round, so I'm not raising them again.

What I checked in the credential masking:

  • Request bodies are masked on a copy (lib/src/telescope_integration.dart:214). The masking runs before _truncate. For a JSON string that order matters: once a string is cut at 8 KB it no longer parses, so masking afterwards would miss it. The test a JSON string body is masked before it is cut to size covers that case. _redactBody works on a copy, so the body the driver actually sends is unchanged. The test checks this both ways: body['password'] is still hunter2 afterwards, and the loopback server receives the real body.
  • Responses and errors use the response key list (:229 and :242). Error answers use the same list.
  • The auth header is hidden before the install guard (:54). Because the call sits ahead of the _installed early return, a second install() after the telescope store is reset adds the header name back. That fits the idempotency rule in CLAUDE.md ("a consumer with a lazy debug toggle will call twice"). Both install paths (MagicDevtools.installPost at lib/src/magic_devtools.dart:103 and the README snippet) go through MagicTelescopeIntegration.install(), so neither skips it.
  • The CHANGELOG has a ### Security bullet, as golden rule 3 asks.

One limit: TelescopeRedaction comes from an unreleased telescope version that isn't in this checkout. I'm relying on the tests for how it handles key matching and body types that aren't a Map or a String, such as a Dio FormData. I did not read its source.

Tests

Five new cases in the HTTP credential redaction group of test/telescope_integration_test.dart cover:

  • a real loopback login with the default Authorization header and with a renamed X-Auth header;
  • masking of an error answer;
  • a JSON string body longer than 8 KB;
  • hiding the header again after a store reset.

CI

  • Lint & Test: failure on 675a2d4, which matches the local checkout. The annotation only says "Process completed with exit code 1", so I can't see the log. That fits the unresolved dependency floors (this commit also needs the unreleased TelescopeRedaction), but I haven't confirmed it.
  • Auto-merge low-risk Dependabot PRs: skipped.

magic ^0.0.24 (MagicPerfHooks, request ids, onRefreshUI removed),
fluttersdk_dusk ^0.0.17 (PerfMode, interaction readers),
fluttersdk_telescope ^0.0.9 (TelescopeRedaction, record link fields)
and fluttersdk_wind ^1.8.0 (the size-only MediaQuery read the
mediaQuerySize insight describes).

Co-Authored-By: Anılcan Çakır <anilcan.cakir@gmail.com>
Co-Authored-By: Anılcan Çakır <anilcan.cakir@gmail.com>
@anilcancakir

Copy link
Copy Markdown
Member Author

Floors raised for the release batch, as agreed in round 3 (the one open point was the floors, held until the upstream releases).

  • 039e9c6 raises the floors in pubspec.yaml:
    • magic ^0.0.22 to ^0.0.24: MagicPerfHooks, PerfMode consumers, the request ids, and the removal of onRefreshUI.
    • fluttersdk_dusk ^0.0.16 to ^0.0.17: PerfMode and the interaction readers.
    • fluttersdk_telescope ^0.0.7 to ^0.0.9: TelescopeRedaction and the record link fields.
    • fluttersdk_wind ^1.7.0 to ^1.8.0: the size-only MediaQuery read the mediaQuerySize insight describes.
  • 5130681 carries the dusk and telescope floors into the README install snippet and adds a ### Changed CHANGELOG bullet. The magic_devtools: ^0.0.7 line in the snippet is this package's own version, so it moves in the 0.0.8 release PR.

Publish state: artisan 0.0.17, wind 1.8.0 and contracts 1.2.0 are live, telescope 0.0.9 is publishing, and dusk 0.0.17 and magic 0.0.24 follow in this batch. CI stays red on the unresolved magic / fluttersdk_dusk versions until those two are live; I re-run it then.

Local gates against the batch's release branches (a gitignored pubspec_overrides.yaml with absolute paths): dart format --set-exit-if-changed lib test 0 changed, flutter analyze no issues, flutter test +141 all passed.

@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.

The new floors fix the last open point, and I have no new findings. This can merge once magic 0.0.24 and dusk 0.0.17 are on pub.dev and CI is green.

This review covers only what changed since 675a2d4: 039e9c6 (pubspec.yaml) and 5130681 (README.md and CHANGELOG.md).

  • Dependency floors (pubspec.yaml:15-18): each is raised to the release that ships the APIs this code uses:

    • magic: ^0.0.24
    • fluttersdk_dusk: ^0.0.17
    • fluttersdk_telescope: ^0.0.9
    • fluttersdk_wind: ^1.8.0

    This resolves the Critical finding from round 1. The committed file still uses hosted caret constraints, and pubspec_overrides.yaml stays gitignored (.gitignore:30), as the sibling-development section of CLAUDE.md asks.

  • README install snippet (README.md:46-47): it now names the same dusk and telescope floors as pubspec.yaml. Leaving magic_devtools: ^0.0.7 for the release PR is correct: that line is this package's own version, and CLAUDE.md says release is where pubspec.yaml gets bumped.

  • CHANGELOG: a ### Changed bullet under [Unreleased] explains each floor, as golden rule 3 asks.

Tests

These commits only change constraints and docs, so they need no new tests. The earlier rounds covered the behaviour tests.

CI

  • Lint & Test: failure on 5130681, which matches the local checkout. The annotation only says "Process completed with exit code 1", and I can't read the job log. That fits your note that magic 0.0.24 and fluttersdk_dusk 0.0.17 aren't published yet, so the floors can't resolve. I haven't confirmed that's the cause. Re-run it once they are live before merging.
  • Auto-merge low-risk Dependabot PRs: skipped.

@anilcancakir
anilcancakir merged commit a5f8566 into master Sep 29, 2026
3 of 4 checks passed
@codecov

codecov Bot commented Sep 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.90148% with 41 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
lib/src/perf_integration.dart 80.00% 39 Missing ⚠️
lib/src/perf_insight_rules.dart 99.43% 1 Missing ⚠️
lib/src/telescope_integration.dart 96.96% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

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