Skip to content

Update rules table with category information - #28651

Merged
ntBre merged 9 commits into
mainfrom
brent/rule-docs-experiments
Sep 21, 2026
Merged

ntBre merged 9 commits into
mainfrom
brent/rule-docs-experiments

Conversation

@ntBre

@ntBre ntBre commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR updates our rules page with a big table inspired by oxlint and clippy. I'm afraid this
might be a bit of a pain to review, as Codex added a ton of JS and CSS to achieve our desired
functionality. I iterated with it a few times trying to shrink the diff but without much luck.

Supersedes #28099

Test Plan

Mostly local testing:

rule-docs-demo.mp4

Summary
--

This PR updates our rules page with a big table inspired by [oxlint] and [clippy]. I'm afraid this
might be a bit of a pain to review, as Codex added a ton of JS and CSS to achieve our desired
functionality. I iterated with it a few times trying to shrink the diff but without much luck.

Supersedes #28099

[clippy]: https://rust-lang.github.io/rust-clippy/master/
[oxlint]: https://oxc.rs/docs/guide/usage/linter/rules.html

Test Plan
--

Mostly local testing:

TODO
@ntBre ntBre added the documentation Improvements or additions to public-facing documentation label Sep 16, 2026
@astral-sh-bot

astral-sh-bot Bot commented Sep 16, 2026

Copy link
Copy Markdown

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre
ntBre marked this pull request as ready for review September 17, 2026 14:42
@MichaReiser
MichaReiser self-requested a review September 17, 2026 16:53
@MichaReiser

Copy link
Copy Markdown
Member
Screen.Recording.2026-09-17.at.20.19.48.mov

In a dropdown, using arrow down scrolls the main page. This seems like a bug to me

@MichaReiser

Copy link
Copy Markdown
Member
Screenshot 2026-09-17 at 20 21 45 Can we change the word break in category and linter to only break after `-` (or other word separators but not cut of single characters

@MichaReiser

Copy link
Copy Markdown
Member
Screenshot 2026-09-17 at 20 22 43

The sort keys for linter and categories is on its own line, when it is to the right for rule code and name

@MichaReiser

Copy link
Copy Markdown
Member

The placement of the sort arrows also feels slightly off. The down arrow is below the font's baseline but the top arrow is lower than some capital characters

@MichaReiser

Copy link
Copy Markdown
Member

What's the meaning of the Default value under Status? Should it also be a dedicated option as for Categories?

@ntBre

ntBre commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

What's the meaning of the Default value under Status? Should it also be a dedicated option as for Categories?

It means the rule is enabled by default. I think in this case it's orthogonal to the other Status entries. It doesn't fill in a selection of other fields like the Default categories.

@MichaReiser

Copy link
Copy Markdown
Member

It means the rule is enabled by default. I think in this case it's orthogonal to the other Status entries. It doesn't fill in a selection of other fields like the Default categories.

I'm not sure I understand this. And a value that's orthogonal to other entries seems problematic?

Some preliminary codex findings

[Non-blocking] Use the form as the source of filter values.
The controls already have names, so new FormData(form).getAll("category") can replace the custom values() machinery and the positional [category, linter, status] dependency. Current abstraction (line 59), FormData documentation.
Likewise, allow the existing reset button to restore the HTML defaults, then refresh filtering after reset completes. The current reset handler (line 426) cancels that browser behavior and recreates it manually. Preserve the URL distinction between “all” and “none”; FormData alone does not solve that.

For sorting, could we somehow plugin to mkdocs-material table? https://squidfunk.github.io/mkdocs-material/reference/data-tables/#sortable-tables

@ntBre

ntBre commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

In the Categories dropdown, Default is a shorthand selector that selects correctness, suspicious, complexity, performance, and style (other entries in the dropdown). I think that justifies making it a special case like All and None. In the Status dropdown, Default is not special, it selects one of several statuses that overlap with each other. It doesn't prefill other options like Preview or Stable, so it doesn't make sense to me to make it special in that dropdown.

It's the same word, and even the same rules I guess, but very different selector behavior. Maybe I should just drop that from the Status filter? But we do mark it as a Status still, with the ✅ icon.

@MichaReiser

Copy link
Copy Markdown
Member

Okay, I don't know what the Default status is 😅 Is it that it's on by Default? But that seems different than Stable?

Not saying you should do this, but you could consider using the HTML popover + multiple select. Not sure how the browser support for it is today, but it looks like much less JS :)

https://gist.github.com/MichaReiser/d4d353e87c8f88f5c7852a0a17045a78

@ntBre

ntBre commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Yes, Status = Default means the rule is enabled by default.

Ahh, this isn't just our JSON status field (stable | preview | deprecated | removed). Codex included everything we currently have in our Status table columns, denoted by the emojis in the legend (we previously hid the Status heading from the tables, but that's its internal name). So it covers the JSON status, as well as fixability and whether it's a default rule. I see now why that was confusing 😅

I told Codex to monitor this thread for your suggestions. I'll look into the table especially. We're currently doing some special sorting based on the category severity rather than alphabetical or something, though. Not sure if that's supported.

Comment thread docs/.overrides/main.html Outdated
Comment on lines +33 to +35
<script src="https://unpkg.com/tablesort@5.3.0/dist/tablesort.min.js"
integrity="sha384-vhcBfNt762rNlbnxZsIJQSPWX7b+8NzToPmcW6J5cJzXSnOjZKS2LQ/O2uqLlHcX"
crossorigin="anonymous"></script>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm also not sure about this. The suggestion from the sortable table docs you linked was this in mkdocs.yml:

extra_javascript:
  - https://unpkg.com/tablesort@5.3.0/dist/tablesort.min.js

When I asked Codex if we could hash-pin, it switched to the code here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, sounds like a dependency update nightmare. Do we need sorting? Should we add sorting if users asks for it or what's the use case you had in mind?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, I'll just drop sorting for now. I think it was more like a feature Codex included without me asking and then it seemed like we had to keep it. I guess oxlint has it, which is probably where it got the idea.

Sorting by category is probably the most interesting to me, but filtering down to a single category at a time probably also gets you close enough.

@MichaReiser

Copy link
Copy Markdown
Member

Yes, Status = Default means the rule is enabled by default.

Seems confusing to me. Especially because it overlaps with categories - default. I think we should remove it.

<div class="rule-filters__controls">
<label class="rule-filters__search">
Search rules
<input type="search" name="rule-search" placeholder="Code, name, or message"

@MichaReiser MichaReiser Sep 18, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if searching by message might be annoying for cases where message/name overlap. Maybe let's leave message to Ctrl+F. I guess, only searching rule names is sort of the main benefit that I'd see of the custom search over just using Ctrl+F. We can also remove it (I'm overall leaning towards simplyfing this page)

let _ = write!(
table_out,
"| {ss}{explanation}{se} | {ss}{message}{se} | <div {SYMBOLS_CONTAINER}>{status_token}{fix_token}{default_token}</div>|",
"| <span class='rule-identity'>{ss}{explanation}<br><span class='rule-message'>{message}</span>{se}</span> \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we need to escape message here. and in more cases now, because it is used in HTML and not markdown context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My Codex claims this is okay because message is still being put into Markdown at this stage and that the inline span doesn't prevent MkDocs from handing the message normally.

@@ -0,0 +1,376 @@
.md-typeset .rule-filters {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Codex thinks that this can be drastically cut, especially. if we accept more default styling from mdmkdocs (which I think we should)


No. Some declarations are redundant; much of the rest supports the chosen appearance rather than filtering or sorting. I see three non-blocking simplifications:

  1. Simplify the mobile table design. The responsive rules repeatedly adjust column widths, padding, and font size, hide messages, and stack status icons. Keeping Material’s horizontal scrolling on small screens could replace much of this. That would change the mobile presentation.

  2. Remove duplicated declarations. For example, display: table at line 199 already exists in Ruff’s extra.css. vertical-align: top at line 209 is already supplied for both cells and headers by Material.

  3. Consolidate table styling. The code and rule-name styles repeat background and padding resets. Column widths could target existing .rule-code, .rule-category, etc. classes instead of repeated positional nth-child() selectors.

I would retain:

  • Popover positioning, bounded height, and scrolling.
  • Visible keyboard focus styles.
  • Sticky positioning and the associated overflow overrides.
  • The Status-header overrides: Tablesort adds role="columnheader" even to unsortable columns, which otherwise triggers Material’s pointer cursor and sort indicator.

Material supplies [base table styling and sorting integration](https://squidfunk.github.io/mkdocs-material/reference/data-tables/#sortable-tables), but not this complete filter layout. The biggest reduction would come from accepting more of its default table presentation. These conclusions are based on the CSS and markup; the proposed mobile changes need visual checks.

Comment thread docs/stylesheets/rule-filters.css Outdated
}

#rule-catalog th:nth-child(1) {
width: 10%;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need the explicit widths? Can't we use one column that simply fills and uses whatever remaining space is available?

Comment thread docs/js/rule-filters.js Outdated
Comment on lines +35 to +48
const rows = [...body.rows].map((element) => ({
element,
name: element.querySelector(".rule-identity code").textContent.trim(),
text: [".rule-code", ".rule-identity"]
.map((selector) => element.querySelector(selector).textContent)
.join(" ")
.toLowerCase(),
category: element.querySelector(".rule-category").textContent.trim(),
linter: element.querySelector(".rule-linter").dataset.linter,
status: element.querySelector(".rule-status").dataset.status,
fixable: element.querySelector(".rule-status").dataset.fixable === "true",
isDefault:
element.querySelector(".rule-status").dataset.default === "true",
}));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Codex tells me you could do

const rows = Array.from(body.rows, (element) => {
  const [code, identity, category, linter, status] = element.cells;

  return {
    element,
    name: identity.querySelector("code").textContent.trim(),
    text: `${code.textContent} ${identity.textContent}`.toLowerCase(),
    category: category.textContent.trim(),
    linter: linter.dataset.linter,
    status: status.dataset.status,
    fixable: status.dataset.fixable === "true",
    isDefault: status.dataset.default === "true",
  };
});

@MichaReiser

Copy link
Copy Markdown
Member

The sticky headers don't seem to work on mobile. I actually think this is good, because you don't want to waste the limited screen space. However, it's also where it would be most useful. Overall, I'm not sure we need sticky headers. It introduces a fair amount of complexity, it's not a feature we get for free, the columns are mostly self-explanatory (it's not like a table with 10 columns full of numbers and you don't know which column is which), and you probably want to use filters anyways.

@MichaReiser

Copy link
Copy Markdown
Member

Codex also thinks that we could use the default md-mkdocs table scroll behavior even on mobile? I think the bar for me is that it's not worse than our current rules table, and, if there are easy way to make the line breaks slightly better, we should do that. But not add the cost of introducing a lot of complexity

@MichaReiser MichaReiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you. I really like the new table.

I'd try to simplify the implementation a bit. Can we reduce some of the feature or reuse more of what mkdocs material already provides?

@ntBre

ntBre commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

I think the horizontal scrolling behavior on mobile is now a little worse, but it's at least consistent with our current page, as you said:

Screen.Recording.2026-09-18.at.10.33.48.mov
Screen.Recording.2026-09-18.at.10.34.29.mov

The now-wider table makes it a little more noticeable, but the behavior is the same.

@MichaReiser

Copy link
Copy Markdown
Member

Hmm, Something is off with our rules page on mobile (main). Why is there so much white nothing...

@MichaReiser

MichaReiser commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

I have codex look into the mobile issue.

I don't think the fixable in Status works as expected. Selecting all options except Fixable shows me both fixable and unfixable rules. I think fixable would have to be its own filter, because it's a separate dimension (or it must be a tri-state that you can toggle between on/off/don't care). I'd remove it.

other codex findings


P3, non-blocking] Remove obsolete leftovers. The rule-filter-change event has no remaining consumers, and the category documentation still claims the order controls sorting.

"No originating linter".to_string(),
)
};
// Preserve links to the old linter headings on their first table row.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Credit to Codex there, but I was happy we could do this too!

@ntBre

ntBre commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

My Codex blames "invisible screen-reader labels escaping the table’s scroll area" and says the new version is also affected. Not sure why it didn't show in my video, but I can reproduce now.

I can remove Fixable. I think the weirdness is caused by the selectors being combined with OR rather than AND, which is generally what you want but does make Fixable weird.

@MichaReiser MichaReiser left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice. I didn't review all the JS or CSS.

I think there's one more simplification. I don't think the old linter group anchors need to reset the filters.

I’d simplify the behavior before changing APIs. Keep shareable query filters, but let old linter anchors simply scroll to their preserved targets instead of also resetting and selecting filters. Keep the small amount of handling needed to reveal hidden targets. Remove the unused custom event.

We can expect that an old linter anchor is a shared link and not something you paste after playing with different filters.

We should test that this PR does not regress the fix of https://github.com/astral-sh/ruff/actions/runs/35372508810?pr=28699

let _ = write!(table_out, "| {ss}—{se} {{ .rule-code }} ");
}

// Message placeholders can be mistaken for an attribute list on the table cell.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, do we need to do something about this? I think this comes back to the escaping discussion. Codex assumes that message is markdown. Which. istrue, but I don't think we guarantee that it is correct markdown or does not contain any html tags. So a b<a outside of. acode block could be misinterpreted as HTML. ty's CLI uses markdown::to_html(&about.to_string()) but I think that does a little more than we want?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry just to keep quoting my Codex here, but it says basically that you're right, this could happen, but that it's preexisting and doesn't affect any of our current rules. Using markdown::to_html would supposedly parse twice, such that:

`__str__`

is converted to:

<code><strong>str</strong></code>

(first to <code>__str__</code> and then expanded again to convert the underscores to bold)

Codex thinks we'd essentially need to render HTML directly (and use markdown::to_html) instead of leaving it to MkDocs to fix this. It does seem worth maybe following up on, I'm kind of surprised we've never used < outside of code in a message. Apparently we have one > in long-sleep-not-forever (ASYNC116), but that doesn't cause the same problem.

@ntBre

ntBre commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Applied the anchor refresh simplification and merged the changes from your PR. Looks good now:

Screen.Recording.2026-09-18.at.14.26.36.mov

@ntBre
ntBre merged commit 67b3bd4 into main Sep 21, 2026
51 checks passed
@ntBre
ntBre deleted the brent/rule-docs-experiments branch September 21, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to public-facing documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants