feat: add optional viewport field to plan files + --viewport CLI flag for responsive/mobile testing - #306
Conversation
Adds an optional viewport field to the plan file schema (<width>x<height> format, e.g. "390x844") and a --viewport <WxH> CLI flag to test create --plan-from. This allows users to test responsive/mobile-only UI by telling the frontend browser runner what viewport size to use. - CliPlanInput: add viewport?: string - plan.schema.json: add viewport property (pattern: ^[1-9]\d*x[1-9]\d*$) - assertPlanShape / collectPlanIssues: validate viewport format - runCreateFromPlan: pass viewport in POST /tests body - test create: add --viewport <WxH> flag (overrides plan JSON value) - DOCUMENTATION.md: add viewport to plan field table - Tests: 2 new viewport tests (valid + invalid) Closes TestSprite#174
WalkthroughChangesFrontend viewport support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestCreateCommand
participant runCreateFromPlan
participant PlanValidator
participant CreateRequest
TestCreateCommand->>runCreateFromPlan: pass --viewport value
runCreateFromPlan->>PlanValidator: validate plan and viewport
PlanValidator-->>runCreateFromPlan: return validated viewport
runCreateFromPlan->>CreateRequest: include resolved viewport
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
DOCUMENTATION.md (1)
169-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the CLI override precedence.
This row documents the plan value, but it does not state that
test create --plan-from --viewport <WxH>overrides the JSON value. Add this rule to the note or link the command reference.Suggested documentation update
-| `viewport` | no | string | Browser viewport for the frontend runner, in `<width>x<height>` form. Forwarded to the backend. Absent means the runner's desktop default. | +| `viewport` | no | string | Browser viewport for the frontend runner, in `<width>x<height>` form. Forwarded to the backend. A `--viewport <WxH>` value on `test create --plan-from` overrides this field. Absent means the runner's desktop default. |As per path instructions,
test create --plan-fromsupports a CLI--viewport <WxH>override, and the CLI value should take precedence over the plan value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DOCUMENTATION.md` at line 169, Update the viewport documentation row in DOCUMENTATION.md to state that the test create --plan-from --viewport <WxH> CLI value overrides the viewport specified in the JSON plan, or link to the command reference documenting this precedence.Source: Path instructions
src/lib/plan-schema.spec.ts (1)
137-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd boundary cases for positive viewport dimensions.
The test rejects
"abc"but does not cover0x844,390x0, or0390x0844. Add these cases and assert that bothvalidateandpassesRealValidatorreject them. This protects the schema and runtime validator contract.As per path instructions, plan validation and
test lintare local/offline and should reject invalid plans before network requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/plan-schema.spec.ts` around lines 137 - 145, Extend the viewport validation test around validate and passesRealValidator to cover zero dimensions and leading-zero dimensions: reject 0x844, 390x0, and 0390x0844 with both validators. Keep the existing valid 390x844 and invalid abc assertions, and ensure these cases remain local validation checks before any network-dependent path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/test.ts`:
- Around line 9601-9605: Handle the shared test create command’s viewport option
in both execution paths: if viewport is supported only with --plan-from, reject
its use before runCreate via localValidationError so the command returns
VALIDATION_ERROR with exit code 5; otherwise pass viewport through runCreate and
include it in the request body. Update the --plan-from forwarding at the
existing action branch while preserving the thin client’s existing error
handling.
- Around line 2449-2458: Define a shared VIEWPORT_PATTERN matching positive,
non-zero dimensions without leading zeroes, then replace the duplicated viewport
regex checks in the CLI override, assertPlanShape, and collectPlanIssues. Ensure
all local validation paths, including test create --plan-from and test lint,
reject values such as 0x844, 390x0, and 0390x0844 consistently with
schemas/plan.schema.json.
---
Nitpick comments:
In `@DOCUMENTATION.md`:
- Line 169: Update the viewport documentation row in DOCUMENTATION.md to state
that the test create --plan-from --viewport <WxH> CLI value overrides the
viewport specified in the JSON plan, or link to the command reference
documenting this precedence.
In `@src/lib/plan-schema.spec.ts`:
- Around line 137-145: Extend the viewport validation test around validate and
passesRealValidator to cover zero dimensions and leading-zero dimensions: reject
0x844, 390x0, and 0390x0844 with both validators. Keep the existing valid
390x844 and invalid abc assertions, and ensure these cases remain local
validation checks before any network-dependent path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d13ea3cf-b63e-41e4-ae2e-76d4663e6c2d
📒 Files selected for processing (4)
DOCUMENTATION.mdschemas/plan.schema.jsonsrc/commands/test.tssrc/lib/plan-schema.spec.ts
| if (opts.viewport !== undefined) { | ||
| if (!/^\d+x\d+$/.test(opts.viewport)) { | ||
| throw localValidationError( | ||
| 'viewport', | ||
| 'must be a string in `<width>x<height>` format (e.g. "390x844")', | ||
| undefined, | ||
| 'flag', | ||
| ); | ||
| } | ||
| plan.viewport = opts.viewport; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one positive-dimension validator for every viewport input.
schemas/plan.schema.json requires each dimension to match [1-9]\d*, but these runtime checks use \d+. Therefore 0x844, 390x0, and 0390x0844 pass --viewport, assertPlanShape, and test lint, even though the schema rejects them. test create --plan-from can send a value that local validation should reject.
Define one VIEWPORT_PATTERN and use it in the CLI override, assertPlanShape, and collectPlanIssues.
Proposed fix
+const VIEWPORT_PATTERN = /^[1-9]\d*x[1-9]\d*$/;
- if (!/^\d+x\d+$/.test(opts.viewport)) {
+ if (!VIEWPORT_PATTERN.test(opts.viewport)) {
- if (typeof obj.viewport !== 'string' || !/^\d+x\d+$/.test(obj.viewport)) {
+ if (typeof obj.viewport !== 'string' || !VIEWPORT_PATTERN.test(obj.viewport)) {As per path instructions, plan validation and test lint are local/offline and should reject invalid plans before network requests.
Also applies to: 2763-2772, 2854-2858
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/test.ts` around lines 2449 - 2458, Define a shared
VIEWPORT_PATTERN matching positive, non-zero dimensions without leading zeroes,
then replace the duplicated viewport regex checks in the CLI override,
assertPlanShape, and collectPlanIssues. Ensure all local validation paths,
including test create --plan-from and test lint, reject values such as 0x844,
390x0, and 0390x0844 consistently with schemas/plan.schema.json.
Source: Path instructions
| .option( | ||
| '--viewport <WxH>', | ||
| 'optional browser viewport for the frontend runner (e.g. "390x844" for mobile). ' + | ||
| 'With --plan-from, overrides the viewport in the plan JSON.', | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently ignore --viewport outside --plan-from.
The option is registered on the shared test create command. The action forwards it only in the --plan-from branch at Line [9708]. The regular runCreate call at Lines [9722-9748] does not receive it. A valid code-file invocation can therefore succeed without applying the requested viewport.
If viewport is plan-only, reject the flag before runCreate with localValidationError so the command returns VALIDATION_ERROR with exit code 5. Otherwise, thread viewport through runCreate and its request body.
As per path instructions, this thin client must preserve correctness and clear error handling.
Also applies to: 9708-9708
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/test.ts` around lines 9601 - 9605, Handle the shared test create
command’s viewport option in both execution paths: if viewport is supported only
with --plan-from, reject its use before runCreate via localValidationError so
the command returns VALIDATION_ERROR with exit code 5; otherwise pass viewport
through runCreate and include it in the request body. Update the --plan-from
forwarding at the existing action branch while preserving the thin client’s
existing error handling.
Source: Path instructions
|
Thanks for building this, and CI is now approved (it had never run — our fault). Two separate things to say: one about the code, one about sequencing. Code: Sequencing, which is the bigger issue: the backend does not currently accept a This is the line So I'm not going to merge this yet, and I don't want to leave you guessing either. Concretely:
@lxcario's original report (#174) is what put this on the list; the desktop-only viewport is a genuine gap for anyone testing responsive layouts. |
|
Status, so this isn't sitting on an open-ended pause: this stays on hold, and there's now a dated decision point rather than an indefinite one. Nothing about my 08-13 read has changed — I re-verified it today rather than repeating it. The backend still accepts no What's new is that the platform side finally has a live owner and a date. There's an internal effort that just kicked off on mobile testing as a first-class test type, with a design review on 2026-08-22. I've put the viewport question into that review explicitly, framed as its own decision rather than an implied one: native device testing and desktop-browser viewport emulation are different features that want the same backend plumbing, and somebody has to say which one is in scope — because right now nobody has, and the default outcome of not deciding is that your PR sits here forever. So: I'll come back to this thread after the 22nd with a real answer — either "the backend is taking a viewport field, rebase and this lands as the CLI surface for it," or "viewport emulation is out of scope in favour of device testing, here's why," which would be a decline with a reason instead of a pause. One thing worth fixing regardless of that outcome, since it's a real defect independent of the sequencing: the viewport validation in the CLI and the shape documented for plan files don't agree on the accepted format, so a plan file the documented schema would accept can be rejected by the flag and vice versa. Those need to be one definition. To be explicit about what I'm not saying: the feature is wanted and the need behind it is real and well-documented internally. This isn't a rejection, and it isn't your code being wrong about anything except the schema mismatch above. |
2 participants
Summary
Adds an optional
viewportfield to the plan file schema (<width>x<height>, e.g."390x844") and a--viewport <WxH>CLI flag totest create --plan-from. This lets users test responsive/mobile-only UI (e.g.md:hiddenbottom nav) by telling the frontend browser runner what viewport to use before executing plan steps.Closes #174
Changes
src/commands/test.tsCliPlanInput— added optionalviewport?: stringfieldassertPlanShape— validates viewport format (^\d+x\d+$)collectPlanIssues— same viewport format check fortest lintrunCreateFromPlan— passesviewportthrough toPOST /testsbody; also supports--viewportCLI flag as an override of the plan file's viewporttest createcommand — added--viewport <WxH>flagCreateFromPlanOptions— addedviewport?: stringfor the override pathCreateFlagOpts— addedviewport?: stringfor the flag parsingschemas/plan.schema.jsonviewportproperty (string, pattern^[1-9]\d*x[1-9]\d*$)DOCUMENTATION.mdviewportto the plan file field tablesrc/lib/plan-schema.spec.tsTesting
src/lib/plan-schema.spec.ts— 17 tests pass (2 new): schema + validator both accept valid viewport, reject invalidsrc/commands/test.test.ts— 324 tests passsrc/commands/test.run.spec.ts— 92 tests passUsage
Plan file (JSON):
{ "projectId": "prj_abc123", "type": "frontend", "name": "Mobile bottom-nav test", "viewport": "390x844", "planSteps": [ { "type": "action", "description": "Navigate to dashboard" }, { "type": "assertion", "description": "Verify the mobile bottom navigation bar is visible" } ] }CLI flag override:
testsprite test create --plan-from plan.json --viewport 390x844When
--viewportis supplied alongside--plan-from, it overrides the plan file's viewport value — useful for running the same plan at different breakpoints (e.g. mobile smoke pass on a desktop-authored plan).Summary by CodeRabbit
New Features
WIDTHxHEIGHTformat.--viewportoption when creating tests from plans.Bug Fixes
Documentation