Skip to content

feat: record breadcrumbs for events that implement ReportsBreadcrumb - #2

Merged
anilcancakir merged 5 commits into
mainfrom
feature/event-breadcrumbs
Sep 27, 2026
Merged

anilcancakir merged 5 commits into
mainfrom
feature/event-breadcrumbs

Conversation

@anilcancakir

Copy link
Copy Markdown
Member

Summary

  • EventBreadcrumbs.breadcrumbFor maps any dispatched magic event that implements ReportsBreadcrumb (breadcrumbCategory, breadcrumbMessage, breadcrumbData) to a Sentry breadcrumb, and answers null for every other event.
  • The service provider registers it through magic's Event.listenAny, so a plugin (magic_deeplink's DeeplinkOpened, for example) gets a breadcrumb trail without depending on Sentry. Opt-in by contract: an event that does not implement ReportsBreadcrumb never reaches Sentry.

Verification

flutter analyze clean, flutter test 37 passing, format clean.

Part of a coordinated set

This PR is one of eight that move reusable, app-agnostic pieces out of Uptizm into the magic framework and its plugins, so any magic app can use them the Laravel way.

Merge order:

  1. fluttersdk/magic first: every plugin PR compiles against its new API (SessionScope, Repository, MagicAction, Event.listenAny, ReportsBreadcrumb).
  2. Then magic_deeplink, magic_notifications, magic_payments and magic_sentry, in any order.
  3. Then magic_starter, which uses magic core's SessionScope, magic_payments' StoreIdentitySync and magic_deeplink's gate.
  4. magic-starter-laravel is independent of the Flutter side.
  5. anilcancakir/uptizm last.

Until magic merges and ships, CI on the plugin PRs resolves the published magic and is expected to be red. Each branch was verified locally against the sibling working trees (analyze, the full test suite, format check).

No version bump, publish or tag is included; a release is a separate step.

@kodizm

kodizm Bot commented Sep 26, 2026

Copy link
Copy Markdown

Note

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

The change looks correct and small, but CI is red on this commit; I can't confirm the cause from the annotations.

Minor

lib/src/sentry_service_provider.dart:63 (maintainability): _registerEventBreadcrumbs() runs on every boot(). A second boot, for example in tests or on a hot restart, would add a duplicate wildcard listener and every breadcrumb would be recorded twice. Nothing in the diff shows a guard. Worth confirming that Event.listenAny in magic dedupes, or that boot() runs once.

Tests

test/event_breadcrumbs_test.dart covers the opt-in mapping and the null case for a plain event. Nothing covers the Event.listenAny wiring in the provider.

CI

@kodizm

kodizm Bot commented Sep 27, 2026

Copy link
Copy Markdown

Note

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

Fixes the previous minor finding; nothing new to flag.

This commit addresses my earlier note about _registerEventBreadcrumbs() double-registering on a second boot(): it now stores the remover callback returned by Event.listenAny() in a static field and calls it before re-registering, so a hot restart or repeated boot() no longer records breadcrumbs twice. The fix is covered by a new test (test/sentry_service_provider_test.dart) that boots twice, dispatches one event, and asserts exactly one breadcrumb is recorded.

One thing worth noting rather than flagging: the remover is stored in a static field shared across all SentryServiceProvider instances, so if two separate provider instances were ever booted concurrently for different apps in the same process, the second boot would remove the first app's listener. Given magic's single-app-per-process model this is not a real scenario, so I'm not raising it as a finding.

Tests

The new test in test/sentry_service_provider_test.dart directly exercises the double-boot fix and passes. test/event_breadcrumbs_test.dart (already reviewed) covers the mapping logic itself.

CI

  • Lint & Test: failure, same as the prior commit. The annotation still only says "Process completed with exit code 1" (https://github.com/fluttersdk/magic_sentry/actions/runs/36281965082/job/108515388520). pubspec.yaml pins magic: ^0.0.21 (published, not a git ref), which is consistent with the PR description's note that plugin CI resolves the published magic package and is expected to be red until fluttersdk/magic ships Event.listenAny and ReportsBreadcrumb. I cannot confirm the failure is only that from the annotation alone.

@kodizm

kodizm Bot commented Sep 27, 2026

Copy link
Copy Markdown

Note

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

Comment-only update since the last review; nothing new to flag.

This commit only rewords the CHANGELOG entry and the doc comments in sentry_service_provider.dart, correcting the earlier "hot restart" claim to "same isolate" (a hot restart is a new isolate, so the static remover field would not survive it anyway; the accurate scope is a second boot() within the same isolate, e.g. a test suite). No behaviour changed. My earlier findings for this PR either stand as previously noted or were already addressed; nothing further to add.

Tests

No test changes in this commit; the existing double-boot test still covers the behaviour.

CI

  • Lint & Test: failure, same as prior commits. The annotation still only says "Process completed with exit code 1" (https://github.com/fluttersdk/magic_sentry/actions/runs/36285445879/job/108525223040). Consistent with the PR description's note that CI resolves the published magic package and is expected to be red until fluttersdk/magic ships Event.listenAny and ReportsBreadcrumb, but I cannot confirm that's the only cause from the annotation alone.

@anilcancakir

Copy link
Copy Markdown
Member Author

Floor only, in 7844b5b. There were no open findings from your last round.

pubspec.yaml: magic moves ^0.0.21 to ^0.0.22.

  • Event.listenAny and ReportsBreadcrumb ship in magic#205, which releases as magic 0.0.22. At ^0.0.21 a solver could hand this package a magic without them, and it would fail to compile.
  • The comment above the constraint now names both requirements.
  • The comment also pointed at "the published API check under doc/getting-started/installation.md", which does not exist in that file, so I dropped that pointer.
  • CHANGELOG.md gains a ### Changed line under [Unreleased].

Gates, run locally against magic#205's head (89c4daa) through a temporary pubspec_overrides.yaml:

Formatted 11 files (0 changed)
flutter analyze --no-fatal-infos: No issues found!
flutter test: +38: All tests passed!

CI: Lint & Test stays red until magic 0.0.22 is on pub.dev. CI resolves the hosted magic, which does not have Event.listenAny yet. That is expected; CI will be re-run once the release is live.

@kodizm please re-review.

@kodizm

kodizm Bot commented Sep 27, 2026

Copy link
Copy Markdown

Note

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

Floor-only update since the last review; all prior findings addressed, nothing new.

7844b5b raises the magic dependency from ^0.0.21 to ^0.0.22, which is where Event.listenAny and ReportsBreadcrumb actually ship, so the solver can no longer hand this plugin a magic version that lacks the API it depends on. The comment above the constraint now correctly names both requirements, and the stale pointer to a non-existent section of doc/getting-started/installation.md is gone. CHANGELOG.md gains a matching ### Changed entry. No behaviour changed; this only tightens the version constraint to match reality.

Tests

No test changes in this commit. The double-boot fix from the prior commit is still covered by test/sentry_service_provider_test.dart, and test/event_breadcrumbs_test.dart still covers the mapping logic.

CI

  • Lint & Test: failure, same as prior commits. The annotation still only says "Process completed with exit code 1" (https://github.com/fluttersdk/magic_sentry/actions/runs/36311298733/job/108597610984). This is consistent with the PR description and the author's note: CI resolves the hosted magic package from pub.dev, which does not yet have 0.0.22 published, so the constraint bump alone can't turn CI green until that release lands. I cannot independently confirm that's the only cause from the annotation alone.

@codecov

codecov Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@anilcancakir

Copy link
Copy Markdown
Member Author

Round 5: CI after magic 0.0.22 published.

Lint & Test is green now that magic ^0.0.22 resolves from pub.dev. codecov/patch was red at 92.31%, one line short: lib/src/event_breadcrumbs.dart:13, the EventBreadcrumbs._(); private constructor, which nothing calls by design (local flutter test --coverage showed DA:13,0 and no other miss in the file).

Fixed in ecd0f0c: EventBreadcrumbs is an abstract final class (Dart 3, within the package's >=3.6.0 SDK floor). It still cannot be instantiated or extended, and there is no constructor left to be uncovered. No behaviour moved: breadcrumbFor is unchanged.

Gates on ecd0f0c: dart format 11 files, 0 changed; flutter analyze no issues; flutter test --coverage 38 passed, 0 uncovered lines in event_breadcrumbs.dart.

@kodizm please re-review.

@kodizm

kodizm Bot commented Sep 27, 2026

Copy link
Copy Markdown

Note

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

All prior findings addressed; CI is now green.

ecd0f0c changes EventBreadcrumbs from a class with an uncalled private constructor to abstract final class EventBreadcrumbs, which is uninstantiable and unextendable without a dead constructor. This closes the codecov/patch gap the author flagged (the uncovered EventBreadcrumbs._(); line no longer exists) and matches the description exactly: breadcrumbFor itself is unchanged.

Tests

No new tests needed here; nothing behavioural changed. test/event_breadcrumbs_test.dart and test/sentry_service_provider_test.dart continue to cover the mapping logic and the double-boot fix from earlier rounds.

CI

  • Lint & Test: success.
  • codecov/patch: success, 100.00% of diff hit (target 96.19%).

No further findings.

@anilcancakir
anilcancakir merged commit fc2228b into main Sep 27, 2026
3 checks passed
@anilcancakir anilcancakir mentioned this pull request Sep 27, 2026
@anilcancakir
anilcancakir deleted the feature/event-breadcrumbs branch September 27, 2026 15:47
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