Skip to content

fix(cli): error when --viewport is passed without configured [viewports] - #124

Merged
aram-devdocs merged 1 commit into
mainfrom
fix/119-viewport-no-config
Apr 25, 2026
Merged

fix(cli): error when --viewport is passed without configured [viewports]#124
aram-devdocs merged 1 commit into
mainfrom
fix/119-viewport-no-config

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Closes #119.

Summary

  • plumb lint <url> --viewport NAME now errors with exit code 2 when config.viewports is empty (no plumb.toml, or one without [viewports.*]), instead of silently discarding the flag and falling back to a single desktop 1280×800 target.
  • Quickstart path (plumb lint plumb-fake://hello with no --viewport) is unchanged — still falls back to default desktop.

Why error vs. warn vs. magic

Issue #119 listed three options: error, magic-default sizes, or stderr warning. Picked error because:

  • Matches the populated-config path (LintError::UnknownViewports already errors on bad names) — no more inconsistency between the two paths.
  • Phase 1 ticket feat(cli,core): --viewport flag + viewport multi-run orchestration #16 acceptance criterion ("--viewport mobile --viewport desktop runs against exactly those two viewports") fails silently under the old behavior; under the new behavior it fails loudly.
  • "Magic" default sizes (option 2) introduces non-deterministic naming and was rejected by the issue itself.
  • A stderr warning (option 3) leaves the run completing with the wrong viewport — easy to miss in CI.

Error message

--viewport mobile requested but no [viewports] are configured. add [viewports.mobile] to plumb.toml or remove the flag to use the default 1280x800 desktop.

Actionable: tells the user both how to opt in (add config) and how to opt out (drop the flag).

Test plan

  • New integration test lint_with_viewport_flag_and_no_config_exits_input_error — TempDir with no plumb.toml, asserts exit 2 + stderr matches.
  • Renamed unit test asserts the new LintError::ViewportFlagWithoutConfig variant and preserves input order.
  • Quickstart unit test empty_config_yields_single_default_desktop_target still passes — no regression on the no-flag path.
  • just validate clean locally (fmt + clippy -Dwarnings + nextest + determinism + cargo-deny).

…ts] (#119)

Previously, running `plumb lint <url> --viewport NAME` in a fresh
checkout (or any directory without a `[viewports]` section in
`plumb.toml`) silently dropped the flag and ran a single 1280x800
desktop viewport. The user got no signal that their flag was ignored —
inconsistent with the populated-config path, which errors via
`LintError::UnknownViewports`.

`resolve_targets` now returns a new `LintError::ViewportFlagWithoutConfig`
variant whenever `config.viewports` is empty AND `viewports_arg` is
non-empty. The empty-flag, empty-config quickstart path
(`plumb lint plumb-fake://hello`) is unchanged: it still falls back to
the default desktop target.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

All context collected. Here is the structured review:


PR #124fix(cli): error when --viewport is passed without configured [viewports]

Files touched: crates/plumb-cli/src/commands/lint.rs, crates/plumb-cli/tests/cli_integration.rs
Bucket: plumb-cli only — correct layer for this change.


1. Determinism

No regressions. ViewportFlagWithoutConfig { names } is populated from viewports_arg.to_vec(), which preserves the CLI flag order. The error message is a pure function of that slice — no wall-clock, no HashMap, no env reads. The join in the #[error] macro is deterministic. Clean.

2. Workspace layering

Only plumb-cli is touched. No new internal deps, no unsafe. #[forbid(unsafe_code)] is unaffected. Clean.

3. Error handling

LintError::ViewportFlagWithoutConfig is a thiserror-derived variant, propagated via .map_err(anyhow::Error::from)? in run, and surfaces as ExitCode::from(2) in main — consistent with UnknownViewports and correct per PRD §13.3. No unwrap/expect outside test code. Clean.

One observation (not a blocker): the #[error] format uses .names.first().map_or("<name>", String::as_str) to produce the [viewports.X] example, which means multi-flag invocations (--viewport mobile --viewport tablet) show only the first name in the suggestion. That's reasonable UX — the message also lists all names in the preamble — but it could surprise someone who reads only the second half of the message.

4. Test coverage

  • Unit test empty_config_with_viewport_flag_errors (renamed from the old ignore path): exhaustive match arms, asserts input order preserved. Good.
  • empty_config_yields_single_default_desktop_target (lint.rs:236) still present and unchanged — quickstart regression coverage confirmed.
  • Integration test lint_with_viewport_flag_and_no_config_exits_input_error: real TempDir, asserts code(2) and stderr contains "mobile" and "no [viewports]" — both substrings match the error message literally. Good.
  • Existing UnknownViewports unit tests updated with exhaustive arms for the new variant — no silent match widening. Clean.

5. Documentation

LintError::ViewportFlagWithoutConfig has a doc comment explaining intent and linking #119. The resolve_targets doc block is renumbered from 3 branches to 4 with accurate prose for each. Clean.


Punch list

Nothing actionable that warrants a change request. The single stylistic note above (first-name-only in the [viewports.X] suggestion) is an observation, not a defect, and matches the precedent set by the existing UnknownViewports message shape.


Verdict: APPROVE

@aram-devdocs
aram-devdocs merged commit 5849499 into main Apr 25, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): --viewport <name> silently ignored when config has no [viewports]

1 participant