Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 21, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 21, 2025 5:25am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 21, 2025 5:25am
rivet-inspector Ignored Ignored Preview Nov 21, 2025 5:25am
rivet-site Ignored Ignored Preview Nov 21, 2025 5:25am

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 5:25 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 5:25 AM UTC: CI is running for this pull request on a draft pull request (#3520) due to your merge queue CI optimization settings.
  • Nov 21, 5:26 AM UTC: Merged by the Graphite merge queue via draft PR: #3520.

Copy link
Member Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

graphite-app bot pushed a commit that referenced this pull request Nov 21, 2025
@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-chore_disable_biome_in_lefthook branch November 21, 2025 05:26
@claude
Copy link

claude bot commented Nov 21, 2025

Code Review

Summary

This PR disables the biome formatter in lefthook pre-commit hooks and includes some automatic import reordering changes. The PR is already CLOSED.

Changes Overview

  1. lefthook.yml: Commented out the biome-fmt pre-commit hook with a TODO note
  2. Import reordering: Several Rust files had their imports automatically reordered by rustfmt

Detailed Review

✅ Positive Aspects

  1. Clear Intent: The TODO comment explains why biome is being disabled ("big dumb errors")
  2. Consistent Formatting: The Rust code changes follow proper rustfmt formatting conventions
  3. Non-Breaking Changes: Import reordering is purely cosmetic and doesn't affect functionality

⚠️ Concerns & Recommendations

1. Temporary Solution Without Timeline
  • The TODO comment suggests this is temporary, but there's no tracking issue or follow-up plan
  • Recommendation: Create a tracking issue for re-enabling biome and link it in the TODO comment
2. Import Ordering Inconsistency

Looking at the import changes, there seems to be inconsistency with the project's stated conventions:

In pegboard-gateway/src/lib.rs, the changes move single imports before module imports, and reorder items within module imports. While this follows rustfmt's default behavior, the CLAUDE.md mentions:

"Follow existing patterns in neighboring files"

Recommendation: Verify this is the desired import style across the codebase. If not, configure rustfmt accordingly.

3. Missing Context on "Big Dumb Errors"
  • The commit message and TODO don't specify what the biome errors are
  • Recommendation: Document the specific errors in:
    • A tracking issue
    • The PR description (for future reference)
    • Or at minimum, add more context to the TODO comment
4. Formatting Inconsistency Risk
  • Disabling biome means TypeScript/JavaScript files won't be auto-formatted on commit
  • This could lead to inconsistent formatting in future commits to those files
  • Recommendation:
    • Add a note to CLAUDE.md or contributing docs about manual biome formatting requirements
    • Consider running biome manually before merging large changesets
5. Test Coverage
  • No tests were added or modified
  • Note: Since this is a formatting/tooling change, test changes aren't expected, but consider:
    • Adding a CI check that fails if biome errors exist (separate from pre-commit)
    • This would prevent the issue from getting worse while disabled in pre-commit

🔍 Code Quality Observations

File: engine/packages/engine/tests/common/actors.rs

  • The formatting change improves readability by breaking the long format! call across multiple lines
  • Follows Rust formatting best practices

Files: pegboard-gateway/src/lib.rs and shared_state.rs

  • Import reordering follows alphabetical ordering within groups
  • No functional changes, purely cosmetic

Security & Performance

  • ✅ No security concerns
  • ✅ No performance impact (formatting changes only)

Final Assessment

Overall: This is a reasonable temporary fix to unblock development, but needs follow-through.

Action Items for Follow-up:

  1. Create a tracking issue for the biome errors
  2. Document the specific errors that need fixing
  3. Set a timeline or milestone for re-enabling biome
  4. Consider adding CI checks to prevent biome errors from accumulating
  5. Update documentation about manual formatting requirements while biome is disabled

Verdict: Acceptable as a temporary measure, but requires follow-up work to ensure code quality doesn't degrade over time.


Note: This PR is already closed. If reopening or creating a follow-up, please address the recommendations above.

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.

2 participants