Skip to content

feat(validate): report screenshot sets over the 10-screenshot cap - #2125

Open
rudrankriyam wants to merge 1 commit into
mainfrom
feat/screenshot-set-count-validation
Open

feat(validate): report screenshot sets over the 10-screenshot cap#2125
rudrankriyam wants to merge 1 commit into
mainfrom
feat/screenshot-set-count-validation

Conversation

@rudrankriyam

@rudrankriyam rudrankriyam commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

App Store Connect accepts at most 10 screenshots per screenshot set (one display type within one localization). asc's validation layer checks dimensions, platform/display-type fit, unknown display types, and set nonemptiness, but had no maximum-count check anywhere in internal/validation/.

The result: an eleventh screenshot fails server-side mid-batch, after earlier uploads have already landed, leaving the set in a partially-updated state. asc validate reported the version as clean right up until that point.

Change

Adds a screenshots.count.exceeds_max check (severity error) to screenshotChecks in internal/validation/screenshots.go, so asc validate — and every other consumer of these checks via internal/validation/report.go — reports over-full sets before submission.

  • New exported constant LimitScreenshotsPerSet = 10 in internal/validation/limits.go, alongside the existing Limit* constants, so the follow-up upload-preflight wiring can reuse it.
  • Message names the display type and count: screenshot set APP_IPHONE_65 has 11 screenshots, exceeding the maximum of 10
  • Remediation names locale, display type, and count: Reduce the en-US APP_IPHONE_65 screenshot set from 11 to 10 screenshots or fewer
  • The count check runs before the display type is resolved, so an over-full set is still reported when its display type is unknown or blank (those paths continue early). An 11-shot set fails server-side regardless of whether asc recognizes its display type.

Follows the existing CheckResult ID/severity/remediation conventions in the file and the "X exceeds N unit" / imperative-remediation phrasing used by internal/validation/metadata.go.

Ground truth for the limit

  • docs/openapi/latest.json carries no maxItems (0 occurrences across the entire snapshot) and no maxLength on AppScreenshotSet / AppScreenshotSetCreateRequest — the cap is not expressed in the schema and is enforced server-side at upload time. Cited here as Apple's documented App Store constraint.
  • docs/API_NOTES.md has no existing statement about screenshot counts.
  • The repo already encodes the same value independently: appScreenshotSetMaxScreenshots = 10 in internal/cli/assets/assets_screenshots.go, used by the --max-screenshots discovery/limiting logic.
  • No evidence found — in the OpenAPI snapshot, internal/screenshotcatalog, or the docs — that the cap differs per display type. A flat 10 matches the repo's own upload-path constant.

Scope

Validation layer only. No changes under internal/cli/assets/ or the screenshot upload preflights, which are owned by in-flight PRs #2078 / #2097 / #2102.

Follow-up: wiring this same cap into the upload preflight — so asc refuses the over-full batch before the first byte goes up, rather than only reporting it at validation time — is deferred until those in-flight screenshot PRs land, at which point validation.LimitScreenshotsPerSet can replace the private appScreenshotSetMaxScreenshots constant.

Tests

TDD, RED first. Added to internal/validation/screenshot_test.go:

  • TestScreenshotChecks_ExceedsMaxScreenshots — 11 valid-dimension screenshots produce exactly one check: screenshots.count.exceeds_max, error severity, correct locale/resource, message naming display type + count, remediation naming locale + display type + count + limit.
  • TestScreenshotChecks_PassAtMaxScreenshots — exactly 10 is clean (no checks).
  • TestScreenshotChecks_ExceedsMaxAlongsideDimensionMismatch — an over-full set containing one bad-dimension file reports both checks, exactly two.
  • TestScreenshotChecks_ExceedsMaxForUnknownDisplayType — over-full set with an unrecognized display type reports both count.exceeds_max and display_type_unknown.

RED confirmed before implementation (3 assertion failures; the "10 is clean" case passed from the start, guarding against an off-by-one). All green after.

Compatibility

Additive. A new error-severity check can newly fail asc validate for a version that has an over-full set — which is the point, since that version could not have been submitted successfully anyway. No existing check IDs, messages, or output shapes changed. No command help changed, so docs/COMMANDS.md is untouched.

Validation

make build, make format, make check-docs, GOLANGCI_LINT_TIMEOUT="10m --allow-parallel-runners" make lint (0 issues), and ASC_BYPASS_KEYCHAIN=1 make test all pass — full suite exit 0, zero failures, no flakes hit. The pre-commit hook re-ran docs/format/lint/tests on commit. No live API calls.

Summary by CodeRabbit

  • New Features

    • Added validation to ensure each App Store screenshot set contains no more than 10 screenshots.
    • Exceeding the limit now produces a clear error identifying the affected screenshot set.
  • Bug Fixes

    • Screenshot-count validation now works even when display type information is missing or unrecognized.
    • Existing dimension validation continues to run alongside count checks.

App Store Connect accepts at most 10 screenshots per screenshot set (one
display type within one localization), but the validation layer only
checked dimensions, platform/display-type fit, and set nonemptiness. An
eleventh screenshot failed server-side mid-batch, after earlier uploads
had already landed.

Add a screenshots.count.exceeds_max error check so validation reports
over-full sets before submission. The check runs before the display type
is resolved, so sets with unknown or missing display types are still
reported, and its remediation names the locale, display type, and count.
@mintlify

mintlify Bot commented Aug 19, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
rudrankriyam-app-store-connect-cli-67 🟡 Building Aug 19, 2026, 2:27 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 62987f36-28ac-4cc3-96ee-1a012f1651c2

📥 Commits

Reviewing files that changed from the base of the PR and between bbfeff8 and 4636041.

📒 Files selected for processing (3)
  • internal/validation/limits.go
  • internal/validation/screenshot_test.go
  • internal/validation/screenshots.go

Limit details: You’ve used the included review currently available. Your 70 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.


📝 Walkthrough

Walkthrough

The change adds a maximum of 10 screenshots per set. Validation reports oversized sets before display-type checks and retains dimension and unknown-display-type checks.

Changes

Screenshot validation

Layer / File(s) Summary
Screenshot limit contract
internal/validation/limits.go
Adds the exported LimitScreenshotsPerSet constant with a value of 10.
Screenshot count validation and coverage
internal/validation/screenshots.go, internal/validation/screenshot_test.go
Reports oversized sets with locale and display-type context. Tests cover the limit boundary, combined dimension errors, and unknown display types.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 46360

This localized validation change reports screenshot sets above the 10-screenshot limit before submission; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant ScreenshotSet
  participant ScreenshotValidation
  participant DisplayTypeValidation
  participant ValidationChecks
  ScreenshotSet->>ScreenshotValidation: provide screenshot set
  ScreenshotValidation->>ValidationChecks: report count error when more than 10 screenshots exist
  ScreenshotValidation->>DisplayTypeValidation: continue display-type validation
  DisplayTypeValidation->>ValidationChecks: report display-type and dimension checks
Loading

Suggested labels: medium, p2

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the new validation for screenshot sets that exceed the 10-screenshot limit.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/screenshot-set-count-validation

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. Track spend and usage in your billing settings.


Comment @coderabbitai help to get the list of available commands.

@rudrankriyam rudrankriyam added p1 High priority: important workflow issue or high-impact bug medium Moderate scope with some cross-file or design work labels Aug 19, 2026
@rudrankriyam rudrankriyam modified the milestones: 4.8.2, 4.12.0 Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

medium Moderate scope with some cross-file or design work p1 High priority: important workflow issue or high-impact bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant