feat!: Support SPM for kiosk_mode - #905
Conversation
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 4/10 — medium
This is a packaging change with no Dart-side behavioural delta, and the additive approach (podspec retained alongside Package.swift) keeps the CocoaPods path alive. The risk that keeps this above a 2: plugin registration now depends on the @objc(KioskModePlugin) name resolving through the generated registrant's @import kiosk_mode fallback instead of the deleted ObjC shim, and no CI job builds iOS — so the MethodChannel/EventChannel round-trip after the rename rests on local simulator builds, with on-device verification still outstanding by your own note.
Review Summary
Verdict: COMMENT
1 warning, 3 nits.
The migration itself checks out against the repo:
pluginClass: KioskModePlugininkiosk_mode/pubspec.yaml:26is unchanged, and@objc(KioskModePlugin)keeps the ObjC-visible name stable, so the generated registrant's@import kiosk_modefallback resolves the class now that the header is gone.register(with:)still maps toregisterWithRegistrar:via the@objcFlutterPluginconformance.- No stale references anywhere in the repo — grepping
SwiftKioskModePlugin,Classes/, andkiosk_mode-Swiftreturns nothing outside the deleted files. s.source_files = 'kiosk_mode/Sources/kiosk_mode/**/*.swift'resolves correctly relative to the podspec's directory, and matches the relocated source without picking upPackage.swift.s.license = { :type => 'BSD-3-Clause' }matches the actualkiosk_mode/LICENSE(BSD 3-Clause, Mews).Package.swiftfollows the plugin-author guide's shape: package name matching the plugin, hyphenated product name, explicitFlutterFrameworkproduct dependency.- The CI Flutter bump is safe constraint-wise: no package in the monorepo declares a Flutter upper bound, and nothing depends on
kiosk_modeexcept its own example. - The new
.vscode/launch.jsonentry points at a realkiosk_mode/example/lib/main.dart, and incidentally drops a stray trailing comma.
PR template: all four sections are present and filled, with no leftover placeholders. Every removal/replacement claimed in the description is reflected in the diff (ObjC shim, ios/Assets/, EXCLUDED_ARCHS, podspec boilerplate metadata, Flutter/ephemeral/ gitignore entry).
Leaving the example CocoaPods-integrated is a reasonable call given the hard-fail it avoids for users with SPM disabled — the trade-off is that nothing in the repo exercises the SPM path automatically, which lines up with the CI gap you already logged as a follow-up.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- Release semantics (
kiosk_mode/pubspec.yamlline 8): the Flutter floor raise>=3.38.0→>=3.44.0plus the podspecs.platformbump to iOS 13 are consumer-facing breaking changes, but melos maps a plainfeat:on a pre-1.0 package to a patch bump (0.8.1). Mark the change breaking — retitle the PR/squash commit tofeat!: Support SPM for kiosk_modeor add aBREAKING CHANGE:footer — so melos cuts0.9.0and the generated changelog carries the breaking-change note, matching how the Flutter 3.38 migration (#864) was released. - In
kiosk_mode/ios/.gitignore: add Swift Package Manager artifacts so they can't be committed accidentally —.build/,.swiftpm/, andPackage.resolved. - In
kiosk_mode/example/ios/Flutter/AppFrameworkInfo.plist(line 23): confirm theMinimumOSVersionkey removal actually came from the Flutter tool. The known deployment-target migrator rewrites the value rather than deleting the key, andoptimus_widgetbook/ios/Flutter/AppFrameworkInfo.pliststill has it. If it was a hand edit, restore the key with13.0. - Before merging, run
melos exec --scope="optimus_widgetbook" -- "flutter build web"on Flutter 3.44.8. The shared.github/actions/setupbump changes the toolchain for the widgetbook deploy workflows, which only run on push tomain, so a regression there would not surface on this PR.
After all changes, run the project's linter/formatter.
Add a Package.swift so the plugin can be consumed via Swift Package Manager, letting consumer apps drop CocoaPods entirely. The podspec is kept, so CocoaPods-only apps keep working. Drop the legacy ObjC shim and register the Swift class directly, move the sources to ios/kiosk_mode/Sources/kiosk_mode/, and repoint the podspec at the new location. Closes #799 BREAKING CHANGE: The minimum Flutter version is raised from 3.38.0 to 3.44.0, and the podspec's iOS platform floor from 8.0 to 13.0. The explicit FlutterFramework package dependency required by Package.swift was only introduced in Flutter 3.44.0.
5b68fd7 to
49dbe9b
Compare
|
✅ Review posted. View review · run |
moxly
left a comment
There was a problem hiding this comment.
Risk Assessment
Score: 3/10 — low
The one consumer-facing concern from the last round — the release being cut as a patch despite a Flutter-floor and iOS-deployment-target raise — is resolved now that the change is marked feat!, so melos will cut 0.9.0 with a breaking-change note. What keeps this off a 1-2 is unchanged: no CI job builds iOS, so the @objc(KioskModePlugin) registration rename and both integration paths rest entirely on the local verification in the description. The remaining two carry-forwards are nit-level and don't affect the plugin's runtime behaviour.
Review Summary
Verdict: APPROVE
Re-review of commit 49dbe9b. The incremental diff could not be computed (the previously-reviewed commit 5b68fd7 is no longer reachable — the branch was force-pushed), so I re-analysed the full diff and reconciled it against the previous review rather than scoping to new lines only.
0 new findings. 2 resolved — 2 fixed, 0 dismissed; 2 carried forward (both nits).
Resolved since the last review
feat!marker (was a warning,kiosk_mode/pubspec.yaml:8). The PR title and head commit are nowfeat!: Support SPM for kiosk_mode. Withversion: 0.8.0+1inkiosk_mode/pubspec.yaml:3, melos will map the breaking marker to a minor bump (0.9.0) and emit the breaking-change note in the changelog — matching how the Flutter 3.38 migration shipped (kiosk_mode/CHANGELOG.md:10,0.8.0). Both tightening constraints (flutter: >=3.44.0, podspecs.platformiOS 13) are now covered by that marker.- SPM artifact hygiene (was a nit).
kiosk_mode/ios/.gitignore:39-42now carries.build/,.swiftpm/, andPackage.resolved. The patterns have no leading slash, so they match insidekiosk_mode/ios/kiosk_mode/wherePackage.swiftlives — correct placement.
Unresolved from previous review (2 nits — neither blocks)
kiosk_mode/example/ios/Flutter/AppFrameworkInfo.plist—MinimumOSVersionis still deleted rather than rewritten. I re-read the current file: the key is absent, and the siblingoptimus_widgetbook/ios/Flutter/AppFrameworkInfo.pliststill carries it. Every other deployment-target change in this PR bumps its value (Podfileand threeIPHONEOS_DEPLOYMENT_TARGETentries,9.0→13.0), which is why the deletion still looks unlike the tool's migrator. Example-app only, so impact is low..github/actions/setup/action.yml:9— the shared composite action is still bumped 3.38.5 → 3.44.8, and the widgetbook web-build workflows (deploy-widgetbook.yml,deploy-widgetbook-azure.yml) remainpush: main/workflow_dispatchonly, so a regression there would surface only after merge.
Re-verified as still clean
- No stale references to the removed ObjC shim:
SwiftKioskModePluginreturns 0 matches repo-wide, and nothing underkiosk_mode/referencesClasses/or the old iOS8.0/9.0floors (the only3.38hit is the historical changelog entry). @objc(KioskModePlugin)onkiosk_mode/ios/kiosk_mode/Sources/kiosk_mode/KioskModePlugin.swift:4-5keeps the ObjC-visible name aligned with the unchangedpluginClass: KioskModePlugininkiosk_mode/pubspec.yaml:26, and the method/event channel names are byte-identical to before the rename..vscode/launch.jsonparses as valid JSON with the newkiosk_mode_example_appentry pointing at a realkiosk_mode/example/lib/main.dart.- The SPM layout is consistent: target
kiosk_moderesolves toSources/kiosk_mode/by SPM's default convention, and the podspec'ssource_filesglob targets the same relocated directory without picking upPackage.swift.
Fix All — prompt for AI agent
Fix the following issues in this PR:
- In
kiosk_mode/example/ios/Flutter/AppFrameworkInfo.plist: confirm theMinimumOSVersionkey deletion actually came from the Flutter 3.44 tool migration. If it was a hand edit, restore the key with<string>13.0</string>—AppFrameworkInfo.plistbecomesApp.framework'sInfo.plist, and an embedded framework missingMinimumOSVersioncan trip App Store validation. - Before merging, run
melos exec --scope="optimus_widgetbook" -- "flutter build web"on Flutter 3.44.8. The.github/actions/setupbump changes the toolchain for the widgetbook deploy workflows, which only run on push tomain, so a regression there would not surface on this PR.
After all changes, run the project's linter/formatter.
JulioSarmientoMews
left a comment
There was a problem hiding this comment.
APPROVE — thorough, well-documented packaging change. Verified in an isolated checkout of 49dbe9b.
What I independently confirmed
Package.swiftmatches the official plugin-author template exactly: layoutios/kiosk_mode/Package.swift,.iOS("13.0"), hyphenatedkiosk-modeproduct over underscoredkiosk_modetarget, and.package(name: "FlutterFramework", path: "../FlutterFramework").- The shim removal is safe.
@objc(KioskModePlugin)preserves the ObjC name thatpluginClass: KioskModePlugin(kiosk_mode/pubspec.yaml:26) resolves,DEFINES_MODULE => YESis retained so the registrant's@import kiosk_modefallback works, and the method/event channel names are byte-identical across Swift, Kotlin and Dart. Zero stale references toSwiftKioskModePluginorClasses/remain. source_files = 'kiosk_mode/Sources/kiosk_mode/**/*.swift'resolves relative to the podspec and matches the relocated sources without picking upPackage.swift. Thepod lib lintpass is the most load-bearing item in your testing list.- The toolchain bump can't break the other packages: none has a Flutter upper bound, none has native code, and
melos bsbootstraps all of them inside the greencheckjob. feat!is right — melos will cut0.9.0and carry the breaking note, covering both the Flutter floor and the podspec iOS floor.
Resolving a nit from the earlier review round
The MinimumOSVersion deletion in kiosk_mode/example/ios/Flutter/AppFrameworkInfo.plist is genuine Flutter tool behaviour, not a hand edit — see flutter/flutter#185039, "MinimumOSVersion in ios/Flutter/AppFrameworkInfo.plist is auto removed when running on iOS". The tool strips the key so it can inject the value from IPHONEOS_DEPLOYMENT_TARGET at framework-copy time. optimus_widgetbook still has it only because it hasn't been built on 3.44. Nothing to restore here. Heads-up for downstream though: real apps moving to 3.44 may see the same removal and hit the App Store upload issues in that thread.
Validation gap worth naming
Beyond "CI doesn't build iOS": CI runs no automated tests at all. The 12 checks are check (format + analyze), dcm, PR size, CodeQL and six Wiz scanners — there is no flutter test in .github/. So flutter test ✅ 4/4 is local-only, same bucket as both iOS builds. Pre-existing, not introduced here, but it means the registration rename rests entirely on your local runs. Since Guided Access can't be exercised on the simulator, a physical-device smoke test of watchKioskMode() is the one thing I'd want before publishing — it's what actually proves @objc(KioskModePlugin) resolves at registration.
Cross-repo: nothing in this monorepo consumes kiosk_mode except its own example. The real consumer is the Kiosk app in tech-mo; worth confirming it's on Flutter ≥3.44 before 0.9.0 ships. Low risk, since dropping CocoaPods requires 3.44+ SPM anyway.
Two more nits, neither touching lines this PR changed so noted here instead of inline:
optimus/pubspec.yaml:8andoptimus_icons/pubspec.yaml:8still declareflutter: ">=3.38.0". Before this PR the CI pin (3.38.5) matched that floor exactly; now CI only ever exercises 3.44.8, so the declared floor of both flagship published packages is tested nowhere. Not this PR's job to fix, but worth raising the floors in a follow-up or accepting the drift consciously.kiosk_mode/ios/kiosk_mode/Sources/kiosk_mode/KioskModePlugin.swift— pre-existing, unrelated to this diff (outside the patch hunk, so I can't anchor it inline):onCancelnils the sink but never callsremoveObserver, so each listen → cancel → listen cycle registers anotherNotificationCenterobserver andhandleNotificationfires N times per status change afterward.kiosk_mode.dartdoesn't.distinct()the stream, so the duplicates do reach consumers. One-line fix (NotificationCenter.default.removeObserver(self)inonCancel), happy for it to be a separate issue.
Two non-blocking notes inline below. Nothing here needs to change before merge — the only ask is dispatching the widgetbook web build on this branch first.
Closes #799
Summary
kiosk_modewas the last plugin in some consumer apps still forcing CocoaPods — Flutter only drops CocoaPods from an app when every plugin ships aPackage.swift, so one holdout keepspod installin the build. This adds Swift Package Manager support following the official plugin-author migration guide.The migration is additive — the podspec stays, so CocoaPods-only apps keep working.
Plugin (
kiosk_mode/ios/)kiosk_mode/Package.swift(iOS 13, one target, explicitFlutterFrameworkdependency).Classes/KioskModePlugin.h/.m). SPM has no bridging-header dance, so the Swift class is now the registered plugin directly:SwiftKioskModePlugin→KioskModePlugin, annotated@objc(KioskModePlugin)so lookup by ObjC name at registration still resolves. This matches the existingpluginClass: KioskModePlugininpubspec.yaml, so no Dart-side or consumer-facing API change.ios/kiosk_mode/Sources/kiosk_mode/(viagit mv, history preserved) and repointedsource_filesin the podspec.platformfrom8.0to13.0(stale — Flutter already enforces iOS 13+) and removed the deadEXCLUDED_ARCHS => i386workaround.flutter createboilerplate (homepagewashttp://example.com, author was "Your Company").ios/Assets/, which held only a.gitkeepand was never referenced by the podspec.Minimum Flutter version:
>=3.38.0→>=3.44.0The explicit
FlutterFrameworkpackage dependency inPackage.swiftlanded in Flutter 3.44.0. Without it, 3.44+ emitsPlugin kiosk_mode has a Package.swift for ios but is missing a dependency on FlutterFramework. Declaring it is the forward-looking choice, but it does raise the floor, so CI (.github/actions/setup+publish) moves off 3.38.5 to 3.44.8 — otherwiseflutter pub geton this package would fail in CI. Other packages in the monorepo are untouched: they have no native code and are unaffected. The Dart SDK constraint is deliberately left at>=3.10.1— nothing here needs newer Dart, and raising it would only narrow compatibility for no gain.Example app
Mostly Flutter tool auto-migrations triggered by building on 3.44 (
Podfile,AppDelegate.swiftUIScene lifecycle,Info.plist,Runner.xcscheme,Podfile.lock, deployment target 9.0 → 13.0). These re-apply on any build, so they're committed rather than fought.Note the example is intentionally left CocoaPods-integrated. Committing the SPM integration that the tool bakes into
project.pbxprojmakes the example hard-fail withthe package at '.../FlutterFramework' cannot be accessedfor anyone who has SPM disabled. With it left out, the integration is added automatically on build for anyone who has the flag on. AddedFlutter/ephemeral/to the example's.gitignoreso the generated packages stay untracked.Testing steps
Both integration paths were verified locally on Flutter 3.44.8 — note that CI does not build iOS at all (
check.ymlis ubuntu-only,dart format+flutter analyze), so this cannot be confirmed by the pipeline.flutter config --enable-swift-package-manager, then fromkiosk_mode/example:flutter clean && flutter build ios --no-codesign. ✅ Build succeeds, log reportsAll plugins found for ios are Swift Packages, and theFlutterFrameworkadvisory warning is gone.flutter config --no-enable-swift-package-manager,flutter clean,flutter build ios --no-codesignagainst a pristineproject.pbxproj. ✅ Build succeeds with the relocated sources.pod lib lint kiosk_mode.podspecfromkiosk_mode/ios. ✅ Passes (catches a badsource_filesglob).flutter analyze .✅ no issues;dart format✅ 0 files changed;flutter test✅ 4/4.Follow-up issues
None.
Check during review