Skip to content

[Merge only after iOS SDK 7.0 GA] iOS: import the wrapper contract from Bugsee.SPI - #20

Open
dsheikherev wants to merge 2 commits into
nextgenfrom
chore/ios-7-ga-bugsee-spi
Open

dsheikherev wants to merge 2 commits into
nextgenfrom
chore/ios-7-ga-bugsee-spi

Conversation

@dsheikherev

@dsheikherev dsheikherev commented Oct 29, 2026 •

Copy link
Copy Markdown

Merge only after the iOS SDK 7.0 GA. The code builds against the 7.0 betas as well, so merging early breaks nothing. The title is there so this lands with the GA SDK bump.

Why

iOS SDK 7.0 GA trims the public headers before the API freezes. The cross-platform wrapper contract moves out of Bugsee.h into the explicit submodule Bugsee.SPI (BugseeSPI.h). That covers BGSWrapperChannel, BGSReportContract, BGSAttachmentContract, BGSDataRequestResultCallback, BGSCallback, BGSLogEventSource, BugseeWrapper, +[Bugsee setWrapper:] and the rest. @import Bugsee; no longer brings it in. The bugsee-cocoa change is bugsee/bugsee-cocoa#309.

Change

  • ios/bugsee_flutter/Sources/bugsee_flutter_wrapper: BGSFlutterWrapper.m, BGSFlutterReportOps.m, BGSFlutterReportFiles.m, BGSFlutterReportHandlerPipeline.m, BGSFlutterLogOps.m and BGSFlutterNetworkOps.m import <Bugsee/BugseeSPI.h> under __has_include.
  • ios/wrapper_tests: BGSFlutterFakeReport.h, BGSFlutterLogOpsTests.m, BGSFlutterNetworkOpsTests.m and BGSFlutterWrapperTests.m do the same.
  • BGSFlutterLaunchOpsTests.m: GA also keeps data encryption out of the public headers. The option is still read and stays in the option manifest, but BugseeDataEncryptionMode is not visible. Built against GA, the manifest enum test uses its values (0, 1, 2) directly.

No behaviour change.

Checked

  • All 34 wrapper sources and wrapper tests: clang -fsyntax-only against an SDK build with Bugsee.SPI and one without. 34/34 compile in both.
  • Not run: the Swift plugin build, the wrapper test suite and the example app. Those need the GA SDK published to the SPM repo.

🤖 Generated with Claude Code

The iOS SDK 7.0 GA moves the wrapper contract (BGSWrapperChannel,
BGSReportContract, BGSAttachmentContract, BGSCallback, BugseeWrapper,
+setWrapper:, ...) out of Bugsee.h into the Bugsee.SPI submodule
(BugseeSPI.h), which `@import Bugsee;` no longer brings in.

The bugsee_flutter_wrapper sources and the wrapper tests that use those
types import <Bugsee/BugseeSPI.h> under __has_include, so they build
against both the 7.0 betas and GA. GA also keeps data encryption out of
the public headers (the option still works), so the manifest enum test
uses its values directly when built against GA. No behaviour change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 29, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c245e46a-7673-48d3-bcc0-d68bf2d44755
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@claude

claude Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Claude finished @dsheikherev's task in 33s —— View job


Review of PR #20: import the wrapper contract from Bugsee.SPI

  • Read the diff against origin/nextgen
  • Check that every file using SPI symbols gets the import (directly or transitively)
  • Check the BugseeDataEncryptionMode literals against the vendored manifest
  • Post findings

Findings

I found no real bugs. One P3 note is posted inline on BGSFlutterLaunchOpsTests.m: under GA the data-encryption enum check compares the manifest to a copy of itself. I checked the literals (0/1/2) against test/fixtures/option_manifests/ios.json and they match today. The PR already states this trade-off.

Coverage check

I grepped the iOS sources, tests and the feedback package for the SPI symbols (BGSWrapperChannel, BGSReportContract, BGSAttachmentContract, BGSCallback, BGSDataRequestResultCallback, BGSLogEventSource, BugseeWrapper, setWrapper:). Every place that uses one is covered:

  • The six wrapper .m files and the four test files that use the symbols directly are all changed in this PR.
  • BGSFlutterFakeReport.m and BGSFlutterReportHandlerPipelineTests.m use BGSAttachmentContract, BGSReportContract and BGSCallback without importing. They get them through BGSFlutterFakeReport.h, which this PR changes.
  • The public headers under include/ only mention these names in comments, and say they take id so they need no SDK import. The Swift plugin and the other .m files don't reference the symbols.
  • The feedback package does not use any of the moved symbols.

The __has_include guard is a no-op on the 7.0 betas and imports the submodule on GA, as the description says. It does not alter any behaviour.

I did not build or run anything. I only read the diff and grepped, and the Swift build, the XCTest suite and the example app need the GA SDK. Those are the checks I'd want green before merge.

Summary

  1. Overall risk: Low.
  2. Merge recommendation: Approve in substance. The title already holds it until the iOS 7.0 GA SDK bump lands.
  3. Most important item: Before merging, run xcodebuild test for ios/wrapper_tests and flutter build ios against the published GA package. Also consider the optional P3 above.
  4. Positives: The change is small and mechanical, and the comment on each import says why it exists. It is safe on both beta and GA SDKs. The PR description is candid about what was and wasn't verified.

#if __has_include(<Bugsee/BugseeSPI.h>)
// 7.0 GA keeps data encryption out of the public headers (the option is still read), so
// the enum is not visible here; these are its values.
@"BugseeDataEncryptionMode" : @{@"None" : @0, @"OnDevice" : @1, @"EndToEnd" : @2},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: P3 Low
Location: BGSFlutterLaunchOpsTests.m:341-347
Problem: Under GA the BugseeDataEncryptionMode expectation is a literal copy of what test/fixtures/option_manifests/ios.json already says (None 0, OnDevice 1, EndToEnd 2; I checked that they match today). testTheManifestEnumsAreTheFrameworksRealEnums therefore stops guarding that one enum.
Impact: If the SDK renumbers the enum, or the vendored manifest goes stale, nothing fails for this enum on GA. The test's stated purpose, per its doc comment, is to catch exactly that. This only affects this one enum and only when built against GA.
Scenario: A later GA patch changes EndToEnd to 3 and republishes the manifest. The plugin still sends 2, and this test still passes because both sides are the same literal.
Fix: Optional, and probably acceptable since the PR already states the trade-off. Either keep the literal and add a // guarded elsewhere: pointer to whichever check covers it (scripts/verify_option_manifests.dart reads the SDK source), or, if the SDK still exposes the values in a private or SPI header, read them from there. The __has_include(<Bugsee/BugseeSPI.h>) condition is also only a proxy for "encryption enum hidden". It compiles either way, so it is harmless.

….0 stable)

All 11 files conflicted because #21 already moved the wrapper contract to
Bugsee.SPI against the stable 7.0.0 pin; the branch tree now equals nextgen.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Change-Id: I4e93e6adae9859fd936bd8dfa98042bfc3e75b21
@claude

claude Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Claude finished @krassx's task in 17s —— View job


Re-review after the latest push

I found no new issues, because this PR no longer changes anything.

  • Fetched origin/nextgen and compared it with the PR head
  • Checked whether the earlier P3 finding still applies
  • Posted the summary

What changed

The latest commit, fd5dfb6 ("Merge nextgen: take nextgen's @import Bugsee.SPI (#21 adopted iOS 7.0.0 stable)"), resolves the conflict by taking nextgen's side. git diff origin/nextgen HEAD is empty and the trees are identical. git diff origin/nextgen...HEAD is empty too, because the merge base is the nextgen tip f0bdd00. The PR now holds two commits and zero file changes. #21 already put the Bugsee.SPI imports into nextgen. The "SPI" comment in BGSFlutterWrapper.m:10 is there already, as are the references to BugseeSPI.h in the headers and tests.

Earlier P3 finding (BGSFlutterLaunchOpsTests.m, encryption enum literals)

This no longer applies. The __has_include branch with the @0/@1/@2 literals is gone from the tree. Nextgen's encryptionModeFromSdkSource (BGSFlutterLaunchOpsTests.m:335-349) reads the values from test/fixtures/report_enums.json. That fixture is extracted from the SDK source (BGSOptionsPrivate.h at the pinned ref), so it is not a copy of the manifest. That is the better fix for what the P3 asked about.

Summary

  1. Overall risk: None. There is no diff.
  2. Merge recommendation: Merging adds nothing. The goal in the description (the SPI imports landing with the GA bump) is already met by Move the iOS pin from 7.0.0-beta7 to the stable 7.0.0 #21 on nextgen. Close this PR unless the branch is meant to carry something else. If it is, that content was dropped in the conflict resolution and needs re-adding.
  3. Most important item: Confirm the Bugsee.SPI imports are on nextgen (they are in the current tree). The checks I'd still want green are xcodebuild test for ios/wrapper_tests and flutter build ios against the published 7.0.0 package. I did not run either, nor anything else.
  4. Positives: Taking nextgen's side in the merge was the right call, and the earlier P3 has been addressed there.
    · Branch: chore/ios-7-ga-bugsee-spi

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.

2 participants