Skip to content

fix: crash in no-important on non-standard important values - #541

Merged
DMartens merged 2 commits into
mainfrom
fix/no-important-crash
Sep 5, 2026
Merged

fix: crash in no-important on non-standard important values#541
DMartens merged 2 commits into
mainfrom
fix/no-important-crash

Conversation

@Pixel998

@Pixel998 Pixel998 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Prerequisites checklist

AI acknowledgment

  • I did not use AI to generate this PR.
  • (If the above is not checked) I have reviewed the AI-generated content before submitting.

What did you do?

/* eslint css/no-important: "error" */
a { color: red !ie; }

What did you expect to happen?

No error. !ie is a non-standard hack, not an !important flag.

What actually happened?

TypeError: Cannot read properties of null (reading 'index')

What is the purpose of this pull request?

This PR fixes the crash above. important holds the raw identifier for hacks like !ie, which is truthy, so the rule went on to locate the flag in the source text, found no match, and dereferenced null.

What changes did you make? (Give an overview)

Narrowed the check to important === true, so only real !important flags are reported.

Depends on eslint/csstree#152, which makes !IMPORTANT parse as true.

Related Issues

Is there anything you'd like reviewers to focus on?

Summary by CodeRabbit

  • Bug Fixes

    • Corrected priority detection so only declarations explicitly marked as important are reported.
    • Legacy Internet Explorer priority annotations are now accepted without triggering violations.
  • Tests

    • Added coverage for supported !ie, !IE7, and !important\9 annotations.

@github-project-automation github-project-automation Bot moved this to Needs Triage in Triage Aug 24, 2026
@eslint-github-bot eslint-github-bot Bot added the bug Something isn't working label Aug 24, 2026
@Pixel998 Pixel998 moved this from Needs Triage to Blocked in Triage Aug 24, 2026
DMartens
DMartens previously approved these changes Aug 24, 2026

@DMartens DMartens 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.

Changes make sense to me, thanks. Marking as approved waiting for the csstree PR to land.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions Bot added the Stale label Sep 3, 2026
@DMartens

DMartens commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Can you please rebase on main as there was a @eslint/csstree release with the required fix?

@DMartens DMartens moved this from Blocked to Implementing in Triage Sep 4, 2026
@DMartens DMartens removed the Stale label Sep 4, 2026
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 60cc2550-9074-44e9-b384-53d58e18ca35

📥 Commits

Reviewing files that changed from the base of the PR and between 33e7314 and 4b0178b.

📒 Files selected for processing (2)
  • src/rules/no-important.js
  • tests/rules/no-important.test.js

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The no-important rule now checks for an explicit true important flag. Tests confirm that legacy IE priority annotations do not report violations.

Changes

No-important rule handling

Layer / File(s) Summary
Explicit important-flag validation
src/rules/no-important.js, tests/rules/no-important.test.js
The rule uses node.important === true. Tests cover lowercase, uppercase, and escaped IE priority syntax.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 4b017

The rule now ignores non-standard priority annotations while continuing to report actual !important declarations, preventing the prior crash without introducing an identified merge risk.

Suggested reviewers: lumirlumir

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the no-important crash and the non-standard important values that cause it. It matches the primary change and objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/no-important-crash

Comment @coderabbitai help to get the list of available commands.

@Pixel998
Pixel998 marked this pull request as ready for review September 5, 2026 19:24

@DMartens DMartens 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.

Changes LGTM, thanks.

@DMartens
DMartens merged commit 2985ad5 into main Sep 5, 2026
39 checks passed
@DMartens
DMartens deleted the fix/no-important-crash branch September 5, 2026 20:30
@github-project-automation github-project-automation Bot moved this from Implementing to Complete in Triage Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

2 participants