Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 94 additions & 5 deletions packages/shorebird_cli/lib/src/code_push_client_wrapper.dart
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ class CodePushClientWrapper {
/// The underlying code push client.
final CodePushClient codePushClient;

/// Patch ids this wrapper has promoted during the current command run.
/// Used to suppress the append-after-promotion warning for the second
/// platform of a single `--platforms ios,android` invocation, where the
/// "already on stable" state was created moments earlier by this same run.
final Set<int> _patchesPromotedThisRun = {};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bdero's review bot:

🧹 The name reads as if it tracks global state — it actually tracks "promotions made by this wrapper instance," which has command-scoped lifetime via the scoped ref. A field comment noting the scope (and that cross-invocation reuse correctly prompts because the set starts empty) would save the next reader the trace.


/// Create an app with the given [organizationId] and [appName].
Future<App> createApp({required int organizationId, String? appName}) async {
late final String displayName;
Expand Down Expand Up @@ -872,18 +878,24 @@ aar artifact already exists, continuing...''');
}

/// Creates a patch for the given [appId], [releaseId], and [metadata].
///
/// When [clientPatchId] is supplied, the server treats the call as
/// idempotent: if a patch on this release already has that id, the
/// existing patch is returned instead of creating a new one.
@visibleForTesting
Future<Patch> createPatch({
Future<CreatePatchResponse> createPatch({
required String appId,
required int releaseId,
required Json metadata,
String? clientPatchId,
}) async {
final createPatchProgress = logger.progress('Creating patch');
try {
final patch = await codePushClient.createPatch(
appId: appId,
releaseId: releaseId,
metadata: metadata,
clientPatchId: clientPatchId,
);
createPatchProgress.complete();
return patch;
Expand All @@ -896,7 +908,7 @@ aar artifact already exists, continuing...''');
@visibleForTesting
Future<void> createPatchArtifacts({
required String appId,
required Patch patch,
required CreatePatchResponse patch,
required ReleasePlatform platform,
required Map<Arch, PatchArtifactBundle> patchArtifactBundles,
}) async {
Expand Down Expand Up @@ -943,21 +955,39 @@ aar artifact already exists, continuing...''');

/// Publishes a patch to the Shorebird server. This consists of creating a
/// patch, uploading patch artifacts, and promoting the patch to a specific
/// channel based on the provided [track].
Future<void> publishPatch({
/// channel based on the provided [track]. Returns the resulting
/// [CreatePatchResponse] so callers can compose a final success message
/// that spans every platform in the invocation.
Future<CreatePatchResponse> publishPatch({
required String appId,
required int releaseId,
required Json metadata,
required ReleasePlatform platform,
required DeploymentTrack track,
required Map<Arch, PatchArtifactBundle> patchArtifactBundles,
String? clientPatchId,
}) async {
final patch = await createPatch(
appId: appId,
releaseId: releaseId,
metadata: metadata,
clientPatchId: clientPatchId,
);

// When the create call was idempotent (a clientPatchId hit on an existing
// patch row that's already on the stable channel), uploading this
// platform's artifacts will make them go live immediately to that
// platform's stable users. The server is permissive; the CLI is
// responsible for making sure the developer knows.
if (clientPatchId != null) {
await _confirmAppendToPromotedPatch(
appId: appId,
releaseId: releaseId,
patch: patch,
platform: platform,
);
}

await createPatchArtifacts(
appId: appId,
patch: patch,
Expand All @@ -970,8 +1000,67 @@ aar artifact already exists, continuing...''');
await createChannel(appId: appId, name: track.channel);

await promotePatch(appId: appId, patchId: patch.id, channel: channel);
_patchesPromotedThisRun.add(patch.id);

return patch;
}

logger.success('\n✅ Published Patch ${patch.number}!');
/// Detects the "idempotent hit on an already-promoted patch" case: the
/// caller passed a clientPatchId that matched an existing patch which is
/// already promoted to the stable channel. Uploading this platform's
/// artifacts will go live immediately to that platform's stable users.
///
/// In an interactive terminal we surface that fact and prompt before
/// continuing. In CI we proceed silently — the iOS-promotes-then-Android-
/// completes flow is the canonical use case and prompting would deadlock.
///
/// Skipped entirely when this run is the one that promoted the patch
/// (e.g. the second platform of a `--platforms ios,android` invocation),
/// since the user already opted in by passing both platforms together.
Future<void> _confirmAppendToPromotedPatch({
required String appId,
required int releaseId,
required CreatePatchResponse patch,
required ReleasePlatform platform,
}) async {
// If this run promoted the patch itself (e.g. the first platform of
// `--platforms ios,android`), the "already on stable" state is our own
// doing and the prompt would be spurious.
if (_patchesPromotedThisRun.contains(patch.id)) return;

final List<ReleasePatch> existing;
try {
existing = await codePushClient.getPatches(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bdero's review bot:

💡 The dominant CI flow is "every run has a unique SHA" — so every publishPatch is a genuinely new patch row, but we still fetch the full patch list on every one of them just to confirm "not already on stable." Two server round-trips per platform instead of one, scaling with release-patch-count on the read path.

The cleanest fix lives server-side: have CreatePatchResponse return a created: bool (or echo back the patch's current channel) so the CLI can skip the getPatches lookup when the response says we created a new row. Worth raising with the server PR while it's still in flight.

appId: appId,
releaseId: releaseId,
);
} on Exception {
// If we can't determine the patch's promotion state, fall back to the
// server's permissive behavior — the design treats append-after-
// promotion as allowed; the prompt is a courtesy.
return;
}

final priorState = existing.firstWhereOrNull((p) => p.id == patch.id);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bdero's review bot:

💡 firstWhereOrNull((p) => p.id == patch.id) over the full list — if getPatches ever returns a windowed/recent slice (or starts to), an older patch's promotion state silently reads as "not on stable" and the courtesy prompt vanishes. Not sure if getPatches paginates today — worth confirming, and if there's a "fetch one patch by id" endpoint we should prefer that.

final isOnStable = priorState?.channel == DeploymentTrack.stable.channel;
if (!isOnStable) return;

final platformName = platform.displayName;
final message =
'Patch ${patch.number} is already promoted to the stable track. '
'Uploading $platformName artifacts will go live to $platformName '
'stable users immediately.';

if (shorebirdEnv.canAcceptUserInput) {
logger.warn(message);
if (!logger.confirm('Continue?')) {
logger.info('Aborting.');
throw ProcessExit(ExitCode.success.code);
}
} else {
// CI: log so the action is traceable, but proceed.
logger.info(message);
}
}

/// Returns a GCP download link for measuring download speed.
Expand Down
115 changes: 114 additions & 1 deletion packages/shorebird_cli/lib/src/commands/patch/patch_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'package:shorebird_cli/src/commands/patch/patch.dart';
import 'package:shorebird_cli/src/common_arguments.dart';
import 'package:shorebird_cli/src/config/config.dart';
import 'package:shorebird_cli/src/deployment_track.dart';
import 'package:shorebird_cli/src/executables/executables.dart';
import 'package:shorebird_cli/src/extensions/arg_results.dart';
import 'package:shorebird_cli/src/extensions/string.dart';
import 'package:shorebird_cli/src/formatters/formatters.dart';
Expand All @@ -30,6 +31,7 @@ import 'package:shorebird_cli/src/shorebird_validator.dart';
import 'package:shorebird_cli/src/third_party/flutter_tools/lib/flutter_tools.dart';
import 'package:shorebird_cli/src/version.dart';
import 'package:shorebird_code_push_client/shorebird_code_push_client.dart';
import 'package:uuid/uuid.dart';

/// Signature for a function that returns a [Patcher] for a given [ReleaseType].
typedef ResolvePatcher = Patcher Function(ReleaseType releaseType);
Expand Down Expand Up @@ -98,6 +100,19 @@ To target the latest release (e.g. the release that was most recently updated) u
help: 'The track to publish the patch to.',
defaultsTo: DeploymentTrack.stable.channel,
)
..addOption(
'patch-id',
help: '''
A stable correlation key (e.g. a git SHA) used to unify a logical patch
across platforms. When two invocations on the same release supply the same
--patch-id, the server returns the existing patch instead of allocating a
new number — the iOS and Android halves of the same change end up sharing
one patch number visible to end users.

When omitted on a multi-platform invocation, the CLI generates a fresh
correlation key locally so the platforms in this single command share one
patch.''',
)
..addFlag(
'staging',
negatable: false,
Expand Down Expand Up @@ -215,6 +230,72 @@ NOTE: this is ${styleBold.wrap('not')} recommended. Asset changes cannot be incl
/// The deployment track to publish the patch to.
DeploymentTrack get track => DeploymentTrack(results['track'] as String);

/// The correlation key used to make patch creation idempotent across
/// platforms within this invocation. When the user passes `--patch-id`,
/// that value is used as-is. When the user omits it on a multi-platform
/// invocation, a fresh UUID is generated so the platforms in this single
/// command end up sharing one patch number on the server.
///
/// Returns null only for single-platform invocations with no `--patch-id`,
/// preserving the legacy behavior where the server allocates a fresh patch
/// number per call.
late final String? clientPatchId = _resolveClientPatchId();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bdero's review bot:

🧹 late final String? clientPatchId = _resolveClientPatchId(); works, but _resolveClientPatchId() reads results[...] and calls Uuid().v4(), which means the order of first access decides which Uuid instance is materialized. Tests have to go through runWithOverrides for argResults to be wired before the first access — they do — but a getter (or a method returning a memoized value) reads more obviously than late final-with-side-effect.


String? _resolveClientPatchId() {
final explicit = results['patch-id'] as String?;
if (explicit != null && explicit.isNotEmpty) return explicit;
if (results.releaseTypes.length > 1) return const Uuid().v4();
return null;
}
Comment on lines +249 to +257
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bdero's review bot:

💡 if (explicit != null && explicit.isNotEmpty) return explicit; treats --patch-id="" and --patch-id (omitted) the same. The failure mode: someone writes --patch-id=${{ env.PATCH_SHA }} in their CI template, the env var is undefined, the flag arrives as "", and on a multi-platform invocation we silently generate a UUID — the iOS bot and the Android bot never converge because each generates its own. Loud failure when the flag is present but empty would surface this on the first broken CI run instead of after someone notices the patches aren't unifying.


/// Patches collected from each platform's [createPatch] run, used to emit
/// a single aggregated success message after the fan-out completes.
@visibleForTesting
final platformPatches = <ReleasePlatform, CreatePatchResponse>{};

/// Warns when `--patch-id` is the current `HEAD` commit but the working
/// tree has uncommitted changes — the SHA wouldn't actually identify the
/// code being shipped. Detection is intentionally narrow (literal equality
/// with `git rev-parse HEAD`) so non-SHA ids like `hotfix-login` or CI run
/// IDs never trigger a false positive. Non-blocking: local debug patches
/// over uncommitted code are a legitimate flow.
@visibleForTesting
Future<void> warnIfDirtyTreeMatchesPatchId() async {
final patchId = results['patch-id'] as String?;
if (patchId == null || patchId.isEmpty) return;

final projectRoot = shorebirdEnv.getShorebirdProjectRoot();
if (projectRoot == null) return;

final String head;
try {
head = await git.revParse(
revision: 'HEAD',
directory: projectRoot.path,
);
} on Exception {
return;
}
if (patchId != head) return;

final String porcelain;
try {
porcelain = await git.status(
directory: projectRoot.path,
args: ['--porcelain'],
);
} on Exception {
return;
}
if (porcelain.isEmpty) return;

logger.warn(
'--patch-id is set to HEAD ($patchId), but the working tree has '
'uncommitted changes — this SHA does not identify the code being '
'shipped.',
);
}

@override
Future<int> run() async {
if (results.releaseTypes.isEmpty) {
Expand All @@ -231,6 +312,8 @@ NOTE: this is ${styleBold.wrap('not')} recommended. Asset changes cannot be incl
return ExitCode.usage.code;
}

await warnIfDirtyTreeMatchesPatchId();

final patcherFutures = results.releaseTypes
.map(_resolvePatcher)
.map(createPatch);
Expand All @@ -239,9 +322,37 @@ NOTE: this is ${styleBold.wrap('not')} recommended. Asset changes cannot be incl
await patcherFuture;
}

logUnifiedSuccess();

return ExitCode.success.code;
}

/// Emits one aggregated success line per patch number that ended up with
/// at least one published platform — replaces the old per-platform log
/// that used to live inside the wrapper's `publishPatch`.
@visibleForTesting
void logUnifiedSuccess() {
if (platformPatches.isEmpty) return;

// Group platforms by the patch number they ended up under. Under unified
// numbering this is almost always a single group, but pinning by number
// keeps the message correct in degenerate cases (different releases per
// platform, the user explicitly skipping --patch-id on a multi-platform
// call against a server that hasn't been updated, etc.).
final byPatchNumber = <int, List<ReleasePlatform>>{};
for (final entry in platformPatches.entries) {
byPatchNumber.putIfAbsent(entry.value.number, () => []).add(entry.key);
}

for (final number in byPatchNumber.keys.toList()..sort()) {
final platforms =
byPatchNumber[number]!.map((p) => p.displayName).toList()..sort();
logger.success(
'\n✅ Published Patch $number (${platforms.join(', ')})',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bdero's review bot:

🧹 Lost the ! — was Published Patch X!, now Published Patch X (Android, iOS). The ! matches the celebratory tone of other logger.success lines in the CLI. Published Patch 3 (Android, iOS)! keeps it consistent.

);
}
}

/// Returns a [Patcher] for the given [ReleaseType].
@visibleForTesting
Patcher getPatcher(ReleaseType releaseType) {
Expand Down Expand Up @@ -574,13 +685,15 @@ Building patch with Flutter $flutterVersionString
baseMetadata,
);

await patcher.uploadPatchArtifacts(
final publishedPatch = await patcher.uploadPatchArtifacts(
appId: appId,
releaseId: release.id,
metadata: updateMetadata.toJson(),
track: track,
artifacts: patchArtifactBundles,
clientPatchId: clientPatchId,
);
platformPatches[patcher.releaseType.releasePlatform] = publishedPatch;
},
values: {shorebirdEnvRef.overrideWith(() => releaseFlutterShorebirdEnv)},
);
Expand Down
10 changes: 7 additions & 3 deletions packages/shorebird_cli/lib/src/commands/patch/patcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -134,21 +134,25 @@ More info: ${troubleshootingUrl.toLink()}.
return metadata;
}

/// Uploads the patch artifacts to the CodePush server.
Future<void> uploadPatchArtifacts({
/// Uploads the patch artifacts to the CodePush server. Returns the resulting
/// [CreatePatchResponse] so the orchestrating command can emit one
/// aggregated success message across every platform in the invocation.
Future<CreatePatchResponse> uploadPatchArtifacts({
required String appId,
required int releaseId,
required Map<String, dynamic> metadata,
required Map<Arch, PatchArtifactBundle> artifacts,
required DeploymentTrack track,
String? clientPatchId,
}) async {
await codePushClientWrapper.publishPatch(
return codePushClientWrapper.publishPatch(
appId: appId,
releaseId: releaseId,
metadata: metadata,
platform: releaseType.releasePlatform,
track: track,
patchArtifactBundles: artifacts,
clientPatchId: clientPatchId,
);
}

Expand Down
Loading