Repository navigation
ci(lint): check the formatting of the files a pull request changes, outside the docs and spec lanes - #9264
Conversation
Lint never ran Prettier; only the pre-commit hook does, so a commit made without it landed unformatted shipped source twice (#9148, #9196), and later local merges of main then failed changeset coverage. A Lint step now runs prettier --check on the pull request's added and modified *.ts, *.tsx and *.md files.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
…mmand that fixes them Contributors without the pre-commit hook get the exact pnpm exec prettier --write <files> command in the log and as an error annotation. Prettier failures other than differences fail the step separately.
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Types · interface FieldStatusProps |
message: string |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {message: …} |
| + | core |
. · Types · interface FieldStatusProps |
ref?: React.Ref<HTMLDivElement> |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {ref: …} |
| + | core |
. · Types · interface FieldStatusProps |
type: InputStatusType |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {type: …} |
| + | core |
. · Types · interface FieldStatusProps |
variant?: FieldStatusVariant = 'attached' |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {variant: …} |
| + | core |
. · Types · interface FieldStatusProps |
xstyle?: StyleXStyles |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {xstyle: …} |
| + | core |
. · Types · interface FieldStatusProps |
inherits AriaAttributes, DOMAttributes, HTMLAttributes |
import type {FieldStatusProps} from '@astryxdesign/core' |
| + | core |
. · Types · interface SearchContext |
interface SearchContext<T extends SearchableItem = SearchableItem> |
import type {SearchContext} from '@astryxdesign/core' |
| + | core |
./Typeahead · Also exported from an earlier entry point · SearchContext |
SearchContext (.) |
import {SearchContext} from '@astryxdesign/core/Typeahead' |
| ~ | core |
. · Components · component ComplexSelector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary and ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary and ghost selectors |
<ComplexSelector statusVariant="attached" /> |
| ~ | core |
. · Components · component DateInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<DateInput statusVariant="attached" /> |
| ~ | core |
. · Components · component DateRangeInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<DateRangeInput statusVariant="attached" /> |
| ~ | core |
. · Components · component Field |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Field statusVariant="attached" /> |
| ~ | core |
. · Components · component FieldStatus |
component FieldStatus(props: FieldStatusProps) → component FieldStatus @deprecated |
<FieldStatus /> |
| ~ | core |
. · Components · component FieldStatus |
prop variant?: FieldStatusVariant = 'attached' → prop variant: FieldStatusVariant = 'attached' |
<FieldStatus variant="attached" /> |
| ~ | core |
. · Components · component FileInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<FileInput statusVariant="attached" /> |
| ~ | core |
. · Components · component MultiSelector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary and ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary and ghost selectors |
<MultiSelector statusVariant="attached" /> |
| ~ | core |
. · Components · component NumberInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<NumberInput statusVariant="attached" /> |
| ~ | core |
. · Components · component PowerSearch |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<PowerSearch statusVariant="attached" /> |
| ~ | core |
. · Components · component Selector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary/ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary/ghost selectors |
<Selector statusVariant="attached" /> |
| ~ | core |
. · Components · component TextArea |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TextArea statusVariant="attached" /> |
| ~ | core |
. · Components · component TextInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TextInput statusVariant="attached" /> |
| ~ | core |
. · Components · component TimeInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TimeInput statusVariant="attached" /> |
| ~ | core |
. · Components · component Tokenizer |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Tokenizer statusVariant="attached" /> |
| ~ | core |
. · Components · component Typeahead |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Typeahead statusVariant="attached" /> |
| ~ | core |
. · Types · interface FieldStatusProps |
interface FieldStatusProps extends BaseProps<HTMLDivElement> (props of FieldStatus) → interface FieldStatusProps extends BaseProps<HTMLDivElement> |
import type {FieldStatusProps} from '@astryxdesign/core' |
| ~ | core |
. · Types · interface FieldStatusVariantMap |
tooltip: true → tooltip: true @deprecated |
import type {FieldStatusVariantMap} from '@astryxdesign/core'; const x: FieldStatusVariantMap = {tooltip: …} |
| ~ | core |
. · Types · interface SearchSource |
search: (query: string) => Promise<T[]> | T[] → search: (query: string, context?: SearchContext<T>) => Promise<T[]> | T[] |
import type {SearchSource} from '@astryxdesign/core'; const x: SearchSource = {search: …} |
| ~ | core |
. · Types · interface UseInputStatusIconOptions |
statusVariant?: FieldStatusVariant → statusVariant?: FieldStatusPlacement |
import type {UseInputStatusIconOptions} from '@astryxdesign/core'; const x: UseInputStatusIconOptions = {statusVariant: …} |
| ~ | richtext |
. · Components · component RichTextEditor |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusVariant | 'tooltip' = 'attached' |
<RichTextEditor statusVariant="tooltip" /> |
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
CI impact: 0 job-min per update · pool 0% · time to green −1.6 min · standard runners · 1 other workflow changed (not measured)
| Before | After | Change | |
|---|---|---|---|
| Job-minutes per PR update | 78.6 | 78.6 | 0 |
| Runner pool, runner-minutes a day | 36,679 | 36,679 | 0 (0%) |
| Average concurrent jobs | 25.5 | 25.5 | 0 |
| Time to green (median → this run) | 15.5 min | 14 min | −1.6 min |
| Runs a day | 466 | 466 | 0 |
Other workflows: lint.yml changed: runs on push, pull_request; standard runners; its minutes are not measured here
Assumptions: before is each job's average over the last 20 pull-request runs; after keeps that for unchanged jobs and uses this run's minutes for added or changed ones, weighted by how often each changed job ran in those runs; runs a day averages the seven whole days before today; time to green compares this run with the median of those runs that ran the same jobs.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The formatting step works as described. It flags the two merges that left MultiSelector.tsx and FileInput.tsx unformatted, passes clean merges and recent release merge-backs, and passed on this pull request's own run. Before merging, please update the description:
- Complete the
CI impact:line with what AST-030 FR19 asks for: thelintjob's runner class, job-minutes per update before and after, runner-pool usage and time to green, since the PR report does not measurelint.yml. - Add the docs and spec-lane exception, which the Change section already states, to the Intended invariant and the title (or run the check in those lanes), because spec records and docsite files can still land unformatted.
[Automated review]
| exit 1 | ||
| fi | ||
| if [ "$status" -ne 0 ]; then | ||
| echo "::error title=Prettier failed::prettier --list-different exited with $status" |
There was a problem hiding this comment.
Optional: $status here is xargs's exit code (123 whenever Prettier fails), not Prettier's, so a syntax error reads as "prettier --list-different exited with 123". Something like "Prettier failed (xargs exited with $status); see the error above" would say what happened.
…notes the docs and spec lanes skip it
cixzhang
changed the title
There was a problem hiding this comment.
The formatting check works and the description now covers the CI impact and the docs and spec lane exception, but the fix command it prints needs one change before merging. The paths are joined unquoted at lint.yml:178, so when a listed file sits under a Next.js route group, such as apps/sandbox/src/app/(sandbox)/layout.tsx, pasting the command into bash fails with a syntax error and formats nothing. Quoting each path when the command is built, for example with sed "s/'/'\\\\''/g; s/.*/'&'/" before the tr, fixes it.
[Automated review]
| xargs -0 -r ./node_modules/.bin/prettier --list-different --ignore-unknown \ | ||
| < "$RUNNER_TEMP/changed-files" > "$RUNNER_TEMP/unformatted" || status=$? | ||
| if [ -s "$RUNNER_TEMP/unformatted" ]; then | ||
| fix="pnpm exec prettier --write $(tr '\n' ' ' < "$RUNNER_TEMP/unformatted")" |
There was a problem hiding this comment.
These paths are joined unquoted, so when one sits under a Next.js route group (for example apps/sandbox/src/app/(sandbox)/layout.tsx), pasting the printed command into bash fails with syntax error near unexpected token '(' and formats nothing. Quoting each path fixes it:
fix="pnpm exec prettier --write $(sed "s/'/'\\\\''/g; s/.*/'&'/" "$RUNNER_TEMP/unformatted" | tr '\n' ' ')"A path under a Next.js route group, like app/(sandbox)/layout.tsx, broke the pasted command.
There was a problem hiding this comment.
No further changes are needed: the printed fix command now quotes each path, so it runs for route-group paths such as apps/sandbox/src/app/(sandbox)/layout.tsx. The new step flags the two merges that left FileInput.tsx and MultiSelector.tsx unformatted, passes clean merges and the latest release merge-back, and passed in this pull request's own lint run.
[Automated review]
Maintainer impact
Twice in one day, a merged pull request left shipped source on main unformatted:
MultiSelector.tsxfrom #9148 (fixed by #9247) andFileInput.tsxfrom #9196 (fixed by #9263). Lint never checks formatting. Only the pre-commit hook (lint-staged'sprettier --write) does, and a commit made without the hook skips it. The cost lands on other people. Any later pull request that merges main locally has the hook reformat that file inside its merge commit, and then fails "Check Changeset coverage" for a change to shipped source it never meant to make.Intended invariant
Outside the docs and spec lanes, every file a pull request adds or changes with the hook's patterns (
*.ts,*.tsx,*.md) is Prettier-clean when the pull request merges, so main never gains unformatted source that a later merge would rewrite. The docs-only and spec-only lanes skip full lint, including dependency setup, so they skip this check too: a spec record or docsite file in those lanes can still land unformatted. Those files aren't shipped package source, so they don't trip Changeset coverage.Change
One step in
.github/workflows/lint.yml, "Check formatting of changed files". On pull requests outside the lightweight docs/spec lanes, it runsprettier --check(the repository's Prettier and config, respecting.prettierignore) on the files the merge commit adds or modifies with those patterns. Files the pull request doesn't touch aren't checked, so existing unformatted files elsewhere don't fail unrelated pull requests. A pull request that touches one gets the same fix the hook would apply. On failure, the step lists the unformatted files and prints the exact command to fix them (pnpm exec prettier --write <files>, each path quoted so it pastes into a shell even under a route group likeapp/(sandbox)/), both in the log and as an error annotation, for contributors whose fork has no pre-commit hook.CI impact: the
lintjob runs onubuntu-24.04-arm, unchanged. On this pull request's run the new step took under 1 s (reported as 0 s) of a 235 s job. Full-lint pull request jobs before this change took 234–278 s (median 250 s, 23 recent runs), so job-minutes per update stay about 4.2 and time to green doesn't change. No new jobs or runners, so runner-pool use doesn't change, and nothing is added to the docs and spec lanes.Evidence
MultiSelector.tsxin b56b6d4 (fix(core): one modality call per component, reading the component's own document #9148) andFileInput.tsxin 8f345cb (feat(FieldStatus): deprecate the tooltip variant; field controls keep statusVariant="tooltip" #9196), the two files that caused today's Changeset coverage failures.packages/core/src/FileInput/FileInput.tsxandpnpm exec prettier --write 'packages/core/src/FileInput/FileInput.tsx', then exits 1; at 2a905bb it prints "Every changed file is formatted." and exits 0.Scope
Testing
pnpm exec vitest run --project node .github/scripts/ci-lint-scope.test.mjs .github/scripts/ci-pr-gate.test.mjs .github/scripts/ci-tooling-routing.test.mjs .github/scripts/pr-concurrency.test.mjsactionlint .github/workflows/lint.ymlreports nothing for the new step.Posted by Cindy's agent on her behalf.