Skip to content

🛠️ Check tool imports with Husky#2584

Open
JFWooten4 wants to merge 2 commits into
stellar:mainfrom
JFWooten4:improve-pre-commit-setup-guidance
Open

🛠️ Check tool imports with Husky#2584
JFWooten4 wants to merge 2 commits into
stellar:mainfrom
JFWooten4:improve-pre-commit-setup-guidance

Conversation

@JFWooten4

Copy link
Copy Markdown
Contributor

This follows up on #2392 (comment) to expand the pre-commit to a whole lot of error checking while it formats the mdx files. I'm still somehow partial to a future implementation that defers to exit 0 for compatibility and closed checks afterward, over immediate strict style enforcement. As I said originally, I think it's better to have more contributions, even if some of them require the maintainers to do a 20-second formatting run.

Notwithstanding this, it throws an error and provides much more detailed reusable instructions rather than most of the old one-off yarn hardcoded fetches, which are no longer functional or at least compatible with the pnpm style. In any case, it's simple enough to commit with --no verify on edge-case machines. Lastly, since we're getting rid of the command-line color code, I'll also say that those were not particularly cross-platform compatible with different shells and error box notifications.

Co-authored-by: Codex <noreply@openai.com>
Copilot AI review requested due to automatic review settings July 14, 2026 09:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates the Husky pre-commit hook to be more self-diagnosing and less dependent on colored/one-off package-manager commands, while still enforcing MD/MDX formatting checks consistently with the repo’s Prettier configuration.

Changes:

  • Replaces the old pnpm check:mdx call with a standalone hook that verifies repo root, Node presence/version (from .nvmrc), and installed dependencies before running Prettier.
  • Runs Prettier --check across {docs,src/pages,meeting-notes}/**/*.{md,mdx} and prints actionable setup/formatting instructions on failure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .husky/pre-commit Outdated
Co-authored-by: Codex <noreply@openai.com>
@ElliotFriend

ElliotFriend commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@JFWooten4 Wanted to ask your thoughts about a PR that proposes an additional check in the pre-commit hook: #2550 it's slated to check for any [absolute](/docs/urls) and report accordingly. Wanted to get your sense of:

  • how well would that change fit in with this strategy of running the checks?
  • what order do you think might work best to merge these two?

@ElliotFriend ElliotFriend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for expanding this — the preflight checks and the actionable corepack pnpm install hints are a real upgrade over the old color-coded one-off, and you're right that the ANSI codes weren't portable. CI's green and the approach is good.

On strict-vs-lenient: let's keep it as exit 1. I hear the argument for exit 0 + format-after to lower contribution friction, but since mdx-format runs in CI regardless, a lenient hook doesn't remove the check — it just pushes the failure to a CI round-trip and a manual formatting run on our end. Strict keeps it self-regulating, the failure message already points straight at pnpm format:mdx, and anyone genuinely stuck on an edge-case machine has --no-verify.

One change before merge: the hook hardcodes the prettier config and glob, which duplicates the check:mdx script — I'd rather not have the two able to drift. Can you run corepack pnpm check:mdx after the preflight instead, so there's a single source of truth for what gets checked? The preflight/version-guard logic all stays; just swap the final direct-prettier invocation for the script call.

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.

3 participants