Conversation
🦋 Changeset detectedLatest commit: 52d42ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughRefactors fix‑all across CSS, GraphQL, HTML, JavaScript and JSON handlers to a two‑phase flow: Phase 1 repeatedly applies only rules that have fixes, Phase 2 runs a diagnostics‑only pass and then finalises once. Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.changeset/fancy-books-look.md (1)
5-5: Make the note about the user-visible win.The first sentence already says what users care about; the second slips into implementation detail. I’d keep the changeset focused on faster
--writeruns rather than how the passes are split.✍️ Suggested wording
-Improved performance of fix-all operations (`--write`). Biome is now smarter when it runs lint rules and assist actions. First, it runs only rules that have code fixes, and then runs the rest of the rules. +Improved the performance of `--write`, especially on files with many diagnostics that do not have an automatic fix.As per coding guidelines, ".changeset/*.md: Write changeset descriptions for end users, not developers."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/fancy-books-look.md at line 5, Update the changeset to be user-facing: keep or reword the first sentence to emphasize faster fix-all (`--write`) runs and remove the implementation-detail sentence that explains the two-pass behavior (the part that says it “runs only rules that have code fixes, and then runs the rest of the rules”); simply state the user-visible win (e.g., faster `--write`/fix-all operations) so the description targets end users rather than internal behavior.crates/biome_service/src/file_handlers/javascript.rs (1)
1224-1271: Skip phase 1 whenfixable_rulesis empty.Tiny perf nit: diagnostics-only configurations still pay for one
analyze()plus service setup before falling through to phase 2. A small guard here would avoid that extra work and keep the optimisation tidy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/javascript.rs` around lines 1224 - 1271, The phase-1 loop should be skipped when there are no fixable rules to avoid unnecessary service setup and a wasted analyze() call; add a guard before the loop checking if fixable_rules.is_empty() and only run the two-phase fix-all loop when it is false. Locate the block that builds the AnalysisFilter and the loop that constructs JsAnalyzerServices::from(...) and calls analyze(...), and wrap that entire phase-1 logic (filter creation, service setup, analyze invocation, and process_fix_all processing) in a conditional so it is not executed when fixable_rules.is_empty(). Ensure the rest of the function (phase 2) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_service/src/file_handlers/mod.rs`:
- Around line 874-926: The refactor changed phase ordering and error counting in
collect_signal_fixes_only and collect_diagnostic_only but lacks tests; add
handler-level regression tests that exercise the two-phase fix-all pipeline for
each FixFileMode (ApplySuppressions, SafeFixes, SafeAndUnsafeFixes), including
scenarios with mixed fixable and non-fixable rules and at least one test that
validates suppression actions are applied and one that validates skipped
suggested fixes increment skipped_suggested_fixes while errors are tallied only
in the diagnostic phase; target the analyzer/file-handler test harness that
invokes collect_signal_fixes_only and collect_diagnostic_only so the tests
reproduce the exact sequence and assert pending actions,
skipped_suggested_fixes, and errors counters match expectations.
- Around line 874-926: The suppression pre-pass is still invoking the full
collect_signal path which increments ProcessFixAll.errors for each intermediate
suppression iteration; change the suppression flow to call
collect_signal_fixes_only(...) for suppression batches and perform a single
final pass that calls collect_diagnostic_only(...) to compute remaining errors
(or explicitly reset/recompute self.errors = 0 before the final diagnostic
pass). Update callers that currently use collect_signal (or the code path that
handles FixFileMode::ApplySuppressions) to invoke collect_signal_fixes_only for
collection and then invoke collect_diagnostic_only once so FixFileResult.errors
reflects only the final tree.
---
Nitpick comments:
In @.changeset/fancy-books-look.md:
- Line 5: Update the changeset to be user-facing: keep or reword the first
sentence to emphasize faster fix-all (`--write`) runs and remove the
implementation-detail sentence that explains the two-pass behavior (the part
that says it “runs only rules that have code fixes, and then runs the rest of
the rules”); simply state the user-visible win (e.g., faster `--write`/fix-all
operations) so the description targets end users rather than internal behavior.
In `@crates/biome_service/src/file_handlers/javascript.rs`:
- Around line 1224-1271: The phase-1 loop should be skipped when there are no
fixable rules to avoid unnecessary service setup and a wasted analyze() call;
add a guard before the loop checking if fixable_rules.is_empty() and only run
the two-phase fix-all loop when it is false. Locate the block that builds the
AnalysisFilter and the loop that constructs JsAnalyzerServices::from(...) and
calls analyze(...), and wrap that entire phase-1 logic (filter creation, service
setup, analyze invocation, and process_fix_all processing) in a conditional so
it is not executed when fixable_rules.is_empty(). Ensure the rest of the
function (phase 2) remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e0be9fa-1d4e-4dd9-b496-d1831102c283
📒 Files selected for processing (7)
.changeset/fancy-books-look.mdcrates/biome_service/src/file_handlers/css.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/file_handlers/json.rscrates/biome_service/src/file_handlers/mod.rs
dyc3
left a comment
There was a problem hiding this comment.
It looks good but we should probably smoke test it on some real projects just to make sure.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_service/src/file_handlers/css.rs (1)
747-792:⚠️ Potential issue | 🟠 Major
ApplySuppressionsstill over-countserrors.This branch keeps using
collect_signal(), so each suppression pass increments the error tally before the suppression lands. When the loop exits,finish()returns without a final diagnostics-only pass, soFixFileResult.errorscan reflect transient diagnostics rather than the final tree. Same gremlin in the mirrored handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/css.rs` around lines 747 - 792, The ApplySuppressions loop uses process_fix_all.collect_signal during each suppression pass which increments the error tally prematurely; change the suppression-loop signal closure passed to analyze to a no-op collector (do not call process_fix_all.collect_signal) so suppression iterations don't bump errors, and after exiting the loop run one final analyze that uses process_fix_all.collect_signal to collect final diagnostics before calling process_fix_all.finish; update the invocation sites around analyze, process_fix_all.process_batch_actions and the result.is_none() branch accordingly (symbols: FixFileMode::ApplySuppressions, analyze, process_fix_all.collect_signal, process_fix_all.process_batch_actions, process_fix_all.finish).
🧹 Nitpick comments (1)
crates/biome_service/src/file_handlers/mod.rs (1)
1750-1752: KeepSafeFixesphase 1 truly safe-only.
fixable_rulesis currently built fromFixKind != None, soFixKind::Unsaferules join the phase-1 set as well. InSafeFixes, those rules can only yieldMaybeIncorrectactions, which means they still rerun on every intermediate tree without ever mutating it. A separate safe-only subset would keep the hot loop honest.Also applies to: 1952-1957, 2065-2071, 2201-2225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/file_handlers/mod.rs` around lines 1750 - 1752, The SafeFixes phase currently includes any rule with R::METADATA.fix_kind != FixKind::None, which pulls in FixKind::Unsafe rules; change the predicate to only accept truly safe fixes (e.g. R::METADATA.fix_kind == FixKind::Safe) when building the phase-1 set (the places that insert into rules_with_fix / build fixable_rules), so only Safe rules are included; apply the same change to the other occurrences mentioned (the blocks around the other diffs at 1952–1957, 2065–2071, 2201–2225) so all safe-phase selections use the == FixKind::Safe check instead of != FixKind::None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_service/src/file_handlers/css.rs`:
- Around line 747-792: The ApplySuppressions loop uses
process_fix_all.collect_signal during each suppression pass which increments the
error tally prematurely; change the suppression-loop signal closure passed to
analyze to a no-op collector (do not call process_fix_all.collect_signal) so
suppression iterations don't bump errors, and after exiting the loop run one
final analyze that uses process_fix_all.collect_signal to collect final
diagnostics before calling process_fix_all.finish; update the invocation sites
around analyze, process_fix_all.process_batch_actions and the result.is_none()
branch accordingly (symbols: FixFileMode::ApplySuppressions, analyze,
process_fix_all.collect_signal, process_fix_all.process_batch_actions,
process_fix_all.finish).
---
Nitpick comments:
In `@crates/biome_service/src/file_handlers/mod.rs`:
- Around line 1750-1752: The SafeFixes phase currently includes any rule with
R::METADATA.fix_kind != FixKind::None, which pulls in FixKind::Unsafe rules;
change the predicate to only accept truly safe fixes (e.g. R::METADATA.fix_kind
== FixKind::Safe) when building the phase-1 set (the places that insert into
rules_with_fix / build fixable_rules), so only Safe rules are included; apply
the same change to the other occurrences mentioned (the blocks around the other
diffs at 1952–1957, 2065–2071, 2201–2225) so all safe-phase selections use the
== FixKind::Safe check instead of != FixKind::None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2bb6fb4-fa73-4ddc-a758-419d1001f7fa
📒 Files selected for processing (6)
crates/biome_service/src/file_handlers/css.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/file_handlers/json.rscrates/biome_service/src/file_handlers/mod.rs
✅ Files skipped from review due to trivial changes (1)
- crates/biome_service/src/file_handlers/json.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_service/src/file_handlers/javascript.rs
|
Can you trigger a preview release @dyc3 ? |
|
Sure https://github.com/biomejs/biome/actions/runs/23816124849 |
|
I tested it against the big file it was reported in the zed repository, and it works like a charm. Not big saves, but still I didn't see regressions |
Summary
This PR further improves the performance of the fix_all action.
Now, the operation runs in two phases:
This should save us some microseconds because, before, in the loop, we were running ALL rules, even the ones without code fixes, which was wasteful. We were running non-fixable rules on each new tree, even though it wasn't needed because what it counts is the final code.
Now, rules that don't emit code fixes are run later in a second phase, together with the rules that do, because some rules still emit a diagnostic without a code action. It's by design but it shouldn't matter much.
I designed the code myself, and used an agent to implement it.
Test Plan
Current tests should still pass, since this is an internal refactor.
Docs