Skip to content

Add check/check:ci scripts and standardize lint naming#7755

Open
gilluminate wants to merge 5 commits intomainfrom
gill/add-check-scripts
Open

Add check/check:ci scripts and standardize lint naming#7755
gilluminate wants to merge 5 commits intomainfrom
gill/add-check-scripts

Conversation

@gilluminate
Copy link
Contributor

@gilluminate gilluminate commented Mar 25, 2026

Ticket: N/A (developer experience improvement)

Description Of Changes

Add combined check and check:ci scripts to all client workspaces, and standardize lint script naming to match the format convention (lint = fix, lint:ci = check-only, mirroring format/format:ci).

Also simplifies admin-ui and privacy-center lint commands by removing redundant next lint + separate cypress eslint calls in favor of plain eslint . (cypress dirs have their own .eslintrc.cjs with root: true).

Code Changes

  • Add check (lint fix + format + typecheck) and check:ci (lint check + format check + typecheck) scripts to admin-ui, privacy-center, fidesui, and fides-js
  • Rename lintlint:ci (check-only) and lint:fixlint (fix) across all 4 workspaces, root package.json, and turbo.json
  • Simplify admin-ui and privacy-center lint commands from next lint && eslint ./cypress/... to eslint .
  • Update CI workflow to use npm run check:ci instead of separate typecheck/lint/format steps
  • Update pre-commit hook to use npm run check instead of separate format/lint hooks
  • Add cache: false to the format turbo task (drive-by fix: prettier --write mutates files)

Steps to Confirm

  1. Run cd clients && npm run check:ci -- lint, format, and typecheck all execute for each workspace
  2. Run cd clients/fidesui && npm run check -- runs lint fix, format, and typecheck in sequence
  3. Run cd clients && npm run lint -- runs eslint --fix across all workspaces
  4. Run cd clients && npm run lint:ci -- runs eslint check-only across all workspaces

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link
Contributor

vercel bot commented Mar 25, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Mar 25, 2026 6:59pm
fides-privacy-center Ignored Ignored Mar 25, 2026 6:59pm

Request Review

@github-actions
Copy link

github-actions bot commented Mar 25, 2026

Dependency Review

✅ No vulnerabilities found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA cb83fb4.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

gilluminate and others added 2 commits March 25, 2026 11:30
Combine lint, format, and typecheck into a single `check` (fix) and
`check:ci` (check-only) script across all client workspaces. Rename
lint scripts to match format convention: `lint` = fix, `lint:ci` =
check-only. Simplify admin-ui and privacy-center lint commands by
removing redundant next lint + separate cypress eslint calls. Update
CI workflow and pre-commit hook to use the new combined scripts.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gilluminate gilluminate force-pushed the gill/add-check-scripts branch from 1d7d976 to c44e143 Compare March 25, 2026 17:30
The check scripts call typecheck internally via npm run, so turbo
doesn't resolve the ^build dependency that typecheck normally has.
Add ^build to check and check:ci so fides-js is built before
privacy-center typechecks against it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gilluminate gilluminate marked this pull request as ready for review March 25, 2026 18:34
@gilluminate gilluminate requested a review from a team as a code owner March 25, 2026 18:34
@gilluminate gilluminate requested review from lucanovera and removed request for a team March 25, 2026 18:34
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This is a clean developer-experience improvement that standardizes script naming across all four client workspaces (admin-ui, privacy-center, fidesui, fides-js). It introduces combined check/check:ci scripts, renames lint/lint:fix to lint:ci/lint (mirroring the existing format/format:ci convention), and simplifies the CI workflow from three separate steps into one. The turbo task graph is correctly updated with cache: false on mutating tasks (lint, format, check) and ^build dependencies where typecheck needs upstream build artifacts.

Key changes:

  • New check (lint --fix + format --write + typecheck) and check:ci (lint-check + format-check + typecheck) scripts added consistently to all workspaces and the root clients/package.json
  • lint:fixlint and old lintlint:ci renamed everywhere, including turbo.json and CI workflow
  • admin-ui and privacy-center simplify from next lint && eslint ./cypress/... to eslint . — safe because admin-ui/.eslintrc.cjs already extends next/core-web-vitals and both cypress subdirectories carry their own root: true config
  • format turbo task gains cache: false (drive-by correctness fix for prettier --write)
  • Pre-commit hook consolidates two hooks into one npm run check call, which now additionally runs typecheck — a possible DX trade-off worth considering

Confidence Score: 5/5

  • This PR is safe to merge — all changes are tooling/DX improvements with no application logic or runtime behavior affected.
  • All changes are confined to package.json scripts, turbo.json task configuration, CI workflow, and pre-commit hooks. The ESLint simplification from next lint to eslint . is functionally equivalent given the existing config structure (next/core-web-vitals in each workspace config, root: true on cypress configs). The only non-trivial side effect is that the pre-commit hook now also runs typecheck, which is a deliberate tradeoff. No production code is touched.
  • .pre-commit-config-with-static-check.yml — the added typecheck in the pre-commit hook may slow down commits; worth confirming this is the desired DX tradeoff.

Important Files Changed

Filename Overview
.github/workflows/frontend_checks.yml Consolidates three separate CI steps (typecheck, lint, format) into a single npm run check:ci call — clean simplification with no behavioral regressions.
.pre-commit-config-with-static-check.yml Merges two separate pre-commit hooks (format + lint:fix) into a single npm run check call; this now also runs typecheck on every commit, which may noticeably slow down the pre-commit experience.
clients/turbo.json Adds check/check:ci tasks with correct ^build dependencies; adds cache: false to format (drive-by fix for prettier --write mutation); lint keeps cache: false while lint:ci is cacheable — all configurations are correct.
clients/admin-ui/package.json Replaces next lint && eslint ./cypress/... with eslint .; functionally equivalent because admin-ui/.eslintrc.cjs already extends next/core-web-vitals and the cypress subdirectory has root:true in its own config.

Comments Outside Diff (1)

  1. .pre-commit-config-with-static-check.yml, line 53-59 (link)

    P2 Pre-commit now runs typecheck on every commit

    The merged npm-run-check hook now executes npm run check, which runs lint --fix && format && typecheck across all workspaces. Previously, the two separate hooks only ran format and lint:fix.

    The typecheck step (and its ^build turbo dependency, which will first build fidesui and fides-js) is considerably heavier than a format/lint pass. This could noticeably slow down every commit for all contributors using this pre-commit config.

    Consider whether check (which includes typecheck) is the right default here, or whether check:ci (lint-check + format-check, no typecheck) would be a lighter alternative for the pre-commit hook — reserving the full typecheck for CI:

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Fix check/check:ci to depend on ^build f..." | Re-trigger Greptile

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, well-scoped DX improvement. The consolidation of separate typecheck/lint/format steps into unified check/check:ci scripts is a nice ergonomic win, the naming convention (lint = fix, lint:ci = check-only) is consistent and mirrors the existing format/format:ci pattern, and the drive-by cache: false fix on the format turbo task is correct.

One thing to verify: The pre-commit hook now runs npm run check, which includes tsc --noEmit — something the previous hooks did not do. This will noticeably slow down every commit (potentially 30–90s depending on machine/cache state). CI already catches type errors via check:ci, so it's worth deciding whether that tradeoff is intentional. See inline comment for details.

Everything else looks solid — the next linteslint . simplification is safe (confirmed ESLint configs extend next/core-web-vitals directly and no eslint.dirs override is present), cypress files continue to be linted correctly via their local root: true config, and the turbo caching choices (cache: false on check, cacheable check:ci) are correct.

Keep typecheck out of the pre-commit hook to avoid 30-90s slowdown
on every commit. CI handles typecheck via check:ci.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

1 participant