fix(review): don't block app-version submit on a non-version review in progress

[fjqtt]

Problem

AppStore::Connect#create_review_submission blocks submitting an app version whenever any review submission is in progress:

if app.get_in_progress_review_submission(platform: IOS_PLATFORM)
  raise ReviewAlreadyInProgressError
end

But Spaceship's get_in_progress_review_submission filters only by review state (WAITING_FOR_REVIEW, IN_REVIEW, UNRESOLVED_ISSUES) and platform — it never looks at what the submission contains (app.rb). So an items-only review submission (an In-App Event, custom product page, or product-page experiment) that is in review will make this check truthy and hard-fail an app-version submission — even though the two are allowed to coexist.

App Store Connect explicitly allows this:

Each platform can have one app version submission under review at a time. A platform can have a maximum of two submissions under review at a time: one that includes an app version and one that includes items, like In-App Events or custom product pages, without an app version. … if you have a custom product page under review, you can also submit an app version in a separate submission.

— Overview of submitting for review

The "already in progress" stop is a client-side guard, not an Apple API rejection — fastlane's own deliver carries the identical guard (deliver/lib/deliver/submit_for_review.rb), and its design note even states the assumption "There can only be one open submission per platform per app", which contradicts the two-submission rule above. We hit this in production: an In-App Event was in review, and submitting the app version failed with review_in_progress even though the version had nothing to do with the event.

Fix

Inspect the in-progress submission's items and only raise when it actually contains an appStoreVersion item:

in_progress_submission = app.get_in_progress_review_submission(platform: IOS_PLATFORM)
if in_progress_submission
  raise ReviewAlreadyInProgressError if review_submission_has_app_store_version_item?(in_progress_submission.id)
  log "In-progress review submission has no app version item; proceeding with a separate version submission", {submission_id: in_progress_submission.id}
end

• A real app version in review (including a rejected one sitting in UNRESOLVED_ISSUES) → still raises, unchanged.
• A non-version review in progress (In-App Event etc.) → proceeds; the version is created/submitted as a separate review submission.

The new review_submission_has_app_store_version_item? helper reuses the existing get_review_submission_items and mirrors the per-item iteration already used by get_other_ready_review_items. This extends the same "classify by appStoreVersion item, don't block on non-version items" logic that #33 introduced for the ready-submission path to the in-progress check.

Scope / notes

• Minimal blast radius: only the in-progress check changes. The no-in-progress path and the version-in-progress path behave exactly as before; one extra get_review_submission_items call happens only when an in-progress submission exists.
• I intentionally left get_ready_app_store_version_item untouched, but FWIW it looks like it digs the wrong level — responses.dig(0, "relationships", …) reads the first page body's top-level relationships rather than iterating data[].relationships like get_other_ready_review_items does — so the SubmissionWithItemsExistError guard on the ready path may never fire. Happy to fix that in a follow-up if you agree.
• The repo's CI runs standardrb (passes) + bundler-audit; there's no unit-test harness for connect.rb, so this PR has no automated test. ruby -c is clean.

Validation

Verified against Apple's docs + fastlane source that this is a client-side guard, not an Apple API constraint. Not yet exercised against a live ASC sandbox for the specific path of PATCHing submitted: true on a version submission while an items-only submission is concurrently in review. Note also a separate, pre-existing failure mode (not addressed here): a draft item (not yet in its own in-review submission) can get auto-bundled into the new version submission and rejected — see fastlane #19603. Reviewers with a sandbox that has an in-review In-App Event would be ideal to confirm.

[kitallis]

@fjqtt Good catch 👍🏼

On the change: I do remember handling the non-version items (experiments/CPP/events) to report back before submission (when prepare is clicked in Tramline), but it seems like that's only for reporting (get_other_ready_review_items), in case there is an items-only submission present. But it only does that in specific cases:

• Check if there are items-only submissions during prepare → report to Tramline
• Check if there are items-only submissions during release → report to Tramline (but Tramline discards it)
• Does not check if there is an items-only submission whilst submitting app version

So this change makes sense.

But, due to how Apple APIs and Spacechip is implemented, I've generally tried to reduce API calls we make per path. Fastlane's abstractions are not always efficient in this manner. For example, we could simply used includes in get_other_ready_review_items, but looking at the code, Spaceship throws app_store_version away when inflating model relationships, so even though the API returns correct data, Spaceship does not map it reliably.

We have historically hand-written pieces of Spaceship functions that are inefficiently written, so I'm wondering if we can do something like this:

In create_review_submission:

  in_progress = in_progress_review_submission_with_items
  if in_progress
    raise ReviewAlreadyInProgressError if any_app_store_version_item?(in_progress[:items])
    log "In-progress review submission has no app version item; proceeding with a separate version
  submission",
      {submission_id: in_progress[:submission]["id"]}
  end

def in_progress_review_submission_with_items
    states = [
      api::ReviewSubmission::ReviewSubmissionState::WAITING_FOR_REVIEW,
      api::ReviewSubmission::ReviewSubmissionState::IN_REVIEW,
      api::ReviewSubmission::ReviewSubmissionState::UNRESOLVED_ISSUES
    ].join(",")
    bodies = api.get_review_submissions(
      app_id: app.id,
      filter: {state: states, platform: IOS_PLATFORM},
      includes: "items"
    ).all_pages.map(&:body)

    submission = bodies.flat_map { |body| body["data"] || [] }.first
    return unless submission

    included = bodies.flat_map { |body| body["included"] || [] }
    item_ids = (submission.dig("relationships", "items", "data") || []).map { |d| d["id"] }
    items = included.select { |inc| inc["type"] == "reviewSubmissionItems" && item_ids.include?(inc["id"])
  }

    {submission:, items:}
  end

  # True if any of the submission's items carries an appStoreVersion linkage.
  def any_app_store_version_item?(items)
    items.any? { |item| item.dig("relationships", "appStoreVersion", "data") }
  end

Normally, I wouldn't care about an extra API call, but Apple's APIs are really slow, sometimes taking up to 5 minutes to complete a single call (this is the reason why we have comments describing number of API calls for every public facing method). The aforementioned change should ideally be cheaper and faster overall.

I would need to test this with a live ASC tenant. Give me sometime to confirm, if you are able to validate this, happy to merge this in.

In summary, the conceptual change is solid.

Relatedly, I believe there's another latent bug with cancel_review_submission and ensure_editable_version which both use get_ready_app_store_version_item and hence are susceptible to cancelling an in-progress item-only submission instead of the app version (in both automatic and manual cancellation).

Relatedly, relatedly, your point about accidentally bundling draft app versions seem accurate. But I'm curious, isn't that what will happen if you do it from the ASC portal anyway? That is, if you have a draft app-version and an in-progress item review, creating a review for the draft version will bundle it in ASC as well, right? If yes, then I would argue that behaviour is symmetrical?