Skip to content

fix(formatter/sort-imports): Do not move leading comment if empty line found#16676

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-10-fix_formatter_sort-imports_check_comment_after_newline
Dec 10, 2025
Merged

fix(formatter/sort-imports): Do not move leading comment if empty line found#16676
graphite-app[bot] merged 1 commit intomainfrom
12-10-fix_formatter_sort-imports_check_comment_after_newline

Conversation

@leaysgur
Copy link
Copy Markdown
Member

@leaysgur leaysgur commented Dec 10, 2025 •
edited
Loading

In short, this PR fixes #16655

Input:

// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";
import { useMutation, UseMutationOptions } from "@tanstack/react-query";

🆖 Before fix: Comment is attached to import { apiClient line.

import { useMutation, UseMutationOptions } from "@tanstack/react-query";

// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";

🆗 After fix: Comment with empty line is not attached.

// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { useMutation, UseMutationOptions } from "@tanstack/react-query";

import { apiClient } from "../../apiClient";

However, the implementation itself was not straightforward; it required a concept to represent OrphanContent.

@leaysgur leaysgur requested a review from Dunqing as a code owner December 10, 2025 03:31
Copilot AI review requested due to automatic review settings December 10, 2025 03:31
@github-actions github-actions bot added A-formatter Area - Formatter C-bug Category - Bug labels Dec 10, 2025
@leaysgur Graphite App
Copy link
Copy Markdown
Member Author

leaysgur commented Dec 10, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@leaysgur leaysgur changed the title fix(formatter/sort-imports): Check comment after newline fix(formatter/sort-imports): Do not move leading comment if empty line found Dec 10, 2025
Copilot AI reviewed Dec 10, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the import sorting behavior to correctly handle comments that are separated from imports by newlines. The key change ensures that comments followed by an empty line don't incorrectly attach to the subsequent import, but instead remain associated with the preceding import or become leading orphan lines if they appear before any imports.

Key changes:

  • Enhanced orphan comment detection logic to distinguish between comments directly adjacent to imports versus those separated by empty lines
  • Modified return signature of into_sorted_import_units to include leading orphan lines as a separate output
  • Added comprehensive test cases covering various comment placement scenarios

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
crates/oxc_formatter/tests/ir_transform/sort_imports.rs Added test should_not_attach_comment_if_newline with three test cases validating correct comment handling when separated by newlines
crates/oxc_formatter/src/ir_transform/sort_imports/partitioned_chunk.rs Implemented orphan comment detection logic using two pending buffers (orphan_pending for separated comments, current_pending for adjacent ones) and changed return type to 3-tuple including leading orphan lines
crates/oxc_formatter/src/ir_transform/sort_imports/mod.rs Updated to handle new 3-tuple return value and output leading orphan lines before sorted imports

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

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Dec 10, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #16676 will not alter performance

Comparing 12-10-fix_formatter_sort-imports_check_comment_after_newline (ab35571) with main (8babdf9)

Summary

✅ 38 untouched
⏩ 7 skipped1

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@leaysgur leaysgur marked this pull request as draft December 10, 2025 03:51
@leaysgur leaysgur force-pushed the 12-10-fix_formatter_sort-imports_check_comment_after_newline branch 4 times, most recently from e1bb39d to ab35571 Compare December 10, 2025 05:17
@leaysgur leaysgur marked this pull request as ready for review December 10, 2025 05:23
@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Dec 10, 2025
@Dunqing Graphite App
Copy link
Copy Markdown
Member

Dunqing commented Dec 10, 2025 •
edited by graphite-app bot
Loading

Merge activity

@leaysgur
fix(formatter/sort-imports): Do not move leading comment if empty lin…
2495a39 
…e found (#16676)

In short, this PR fixes #16655

Input:

```js
// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";
import { useMutation, UseMutationOptions } from "@tanstack/react-query";
```

🆖 Before fix: Comment is attached to `import { apiClient` line.

```js
import { useMutation, UseMutationOptions } from "@tanstack/react-query";

// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";
```

🆗 After fix: Comment with empty line is not attached.

```js
// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { useMutation, UseMutationOptions } from "@tanstack/react-query";

import { apiClient } from "../../apiClient";
```

However, the implementation itself was not straightforward; it required a concept to represent `OrphanContent`.
@graphite-app graphite-app bot force-pushed the 12-10-fix_formatter_sort-imports_check_comment_after_newline branch from ab35571 to 2495a39 Compare December 10, 2025 08:29
@graphite-app graphite-app bot merged commit 2495a39 into main Dec 10, 2025
20 checks passed
@graphite-app graphite-app bot deleted the 12-10-fix_formatter_sort-imports_check_comment_after_newline branch December 10, 2025 08:35
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 10, 2025
Copilot AI pushed a commit that referenced this pull request Dec 10, 2025
@leaysgur
fix(formatter/sort-imports): Do not move leading comment if empty lin…
ab018c4 
…e found (#16676)

In short, this PR fixes #16655

Input:

```js
// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";
import { useMutation, UseMutationOptions } from "@tanstack/react-query";
```

🆖 Before fix: Comment is attached to `import { apiClient` line.

```js
import { useMutation, UseMutationOptions } from "@tanstack/react-query";

// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";
```

🆗 After fix: Comment with empty line is not attached.

```js
// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { useMutation, UseMutationOptions } from "@tanstack/react-query";

import { apiClient } from "../../apiClient";
```

However, the implementation itself was not straightforward; it required a concept to represent `OrphanContent`.
taearls pushed a commit to taearls/oxc that referenced this pull request Dec 11, 2025
@leaysgur @taearls
fix(formatter/sort-imports): Do not move leading comment if empty lin…
46e3572 
…e found (oxc-project#16676)

In short, this PR fixes oxc-project#16655

Input:

```js
// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";
import { useMutation, UseMutationOptions } from "@tanstack/react-query";
```

🆖 Before fix: Comment is attached to `import { apiClient` line.

```js
import { useMutation, UseMutationOptions } from "@tanstack/react-query";

// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { apiClient } from "../../apiClient";
```

🆗 After fix: Comment with empty line is not attached.

```js
// THIS IS A GENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

import { useMutation, UseMutationOptions } from "@tanstack/react-query";

import { apiClient } from "../../apiClient";
```

However, the implementation itself was not straightforward; it required a concept to represent `OrphanContent`.
@Boshen Boshen mentioned this pull request Dec 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Dunqing Dunqing Dunqing approved these changes

Assignees

@Dunqing Dunqing

Labels

A-formatter Area - Formatter C-bug Category - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

oxfmt: Header Comments / Preambles are moved when using sort-imports

3 participants

@leaysgur @Dunqing