Validate pseudo-class argument arity consistently#1769
Validate pseudo-class argument arity consistently#1769RedZapdos123 wants to merge 8 commits intofb55:masterfrom
Conversation
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds explicit pseudo-class argument validation: classifies filters by arity, validates presence/absence of arguments early in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pseudo-selectors/index.ts`:
- Around line 23-39: The hardcoded filtersWithArguments and
filtersWithoutArguments sets can drift from the canonical filters map; update
index.ts to derive validation sets from the exported filters object (from
filters.ts) instead of duplicating values: build one set from
Object.keys(filters) and split it into argument-taking vs no-argument categories
(or keep one hardcoded category like filtersWithArguments and compute the other
as the difference), and add a runtime/compile-time assertion that
filtersWithArguments.size + filtersWithoutArguments.size ===
Object.keys(filters).length to fail fast if they diverge; reference the symbols
filtersWithArguments, filtersWithoutArguments and the exported filters object in
filters.ts when making the change.
In `@test/pseudo-classes.ts`:
- Around line 131-147: Add test assertions in the existing "should throw when
pseudo-classes are missing required arguments" spec to cover all entries in
filtersWithArguments and subselects: call CSSselect.selectAll for each
additional filter that requires an argument (e.g., ":contains", ":icontains",
":nth-last-child", ":nth-of-type", ":nth-last-of-type") and for each subselect
(e.g., ":is", ":matches", ":where") and assert they throw with the same
"Pseudo-class <name> requires an argument" pattern; locate the spec using the
test name and the CSSselect.selectAll calls to extend the list so the test fully
mirrors the filtersWithArguments and subselects collections.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd672eec-d9d5-44af-a453-4079e0a6b0e2
📒 Files selected for processing (2)
src/pseudo-selectors/index.tstest/pseudo-classes.ts
RedZapdos123
changed the title
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
|
Addressed the CodeRabbit feedback in commit 488d27d. Changes:
Validation ReRun:
@coderabbitai review |
🧩 Analysis chainThe PR description image shows a terminal session where Let me fetch the latest state of the changed files before reviewing. 🏁 Script executed: Length of output: 4696 🏁 Script executed: Length of output: 7923
✅
|
| Category | Members | Count |
|---|---|---|
filtersWithArguments |
contains, icontains, nth-child, nth-last-child, nth-of-type, nth-last-of-type, lang |
7 |
filtersWithoutArguments (derived) |
root, scope, hover, visited, active |
5 |
allFilterNames |
all 12 exports | 12 |
7 + 5 = 12 ✅
Validation guards in compilePseudoSelector
The three guards are placed correctly — all before the delegate calls — so no path reaches downstream code with invalid data:
data === null && name in subselects→ covers:has,:is,:matches,:where,:not.data === null && filtersWithArguments.has(name)→ covers the 7 filters above.data !== null && filtersWithoutArguments.has(name)→ covers:root,:scope,:hover,:visited,:activewith spurious arguments.
One thing worth confirming: for :scope without arguments (data === null), neither guard fires, so execution reaches filters["scope"](next, null as unknown as string, ...). Looking at the scope implementation — it uses rule only in the no-context branch where it's forwarded to filters["root"], and root ignores _rule entirely — so the null cast is safe and is pre-existing behaviour (no regression introduced by this PR).
✅ test/pseudo-classes.ts
"requires an argument" block — covers all 7 filtersWithArguments members plus all 5 subselect pseudo-classes. Complete.
"doesn't have any arguments" block — covers :scope(foo) and :active(foo).
Minor observation: the remaining three filtersWithoutArguments members (:root(foo), :hover(foo), :visited(foo)) are not exercised. The validation logic for them is identical, so this isn't a correctness gap, but it slightly reduces coverage confidence. You may want to add:
expect(() => CSSselect.selectAll(":root(foo)", dom)).toThrow(
"Pseudo-class :root doesn't have any arguments",
);
expect(() => CSSselect.selectAll(":hover(foo)", dom)).toThrow(
"Pseudo-class :hover doesn't have any arguments",
);
expect(() => CSSselect.selectAll(":visited(foo)", dom)).toThrow(
"Pseudo-class :visited doesn't have any arguments",
);That said, this is non-blocking — the two existing cases are sufficient to validate the code path.
Summary
| Concern | Status |
|---|---|
| Fail-fast set integrity check | ✅ Correct |
filtersWithoutArguments derived from Object.keys |
✅ Robust & future-proof |
| Subselect no-arg guard | ✅ Correct |
| Filter missing-arg guard | ✅ Correct |
| Filter extra-arg guard | ✅ Correct |
null cast for :scope without args |
✅ Safe (pre-existing, filters ignore rule) |
| Test coverage for missing-arg path | ✅ Complete (all 12 affected pseudo-classes) |
| Test coverage for extra-arg path | |
| Build & test suite | ✅ 214 passed, 0 failures |
The implementation is clean and the logic is sound. The only actionable follow-up is the three missing :root/:hover/:visited test cases, which you can add now or defer.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pseudo-selectors/index.ts`:
- Around line 23-47: Remove the mathematically redundant consistency check that
compares filtersWithArguments.size + filtersWithoutArguments.size to
allFilterNames.size; since filtersWithoutArguments is computed as the complement
of filtersWithArguments against allFilterNames, that equality always holds. Edit
the block referencing filtersWithoutArguments, filtersWithArguments and
allFilterNames to either delete the final if/throw check entirely or replace it
with a more useful assertion (for example ensure filtersWithArguments.size > 0)
so the code retains meaningful runtime validation while avoiding the redundant
set-size comparison.
In `@test/pseudo-classes.ts`:
- Around line 181-189: Expand the existing test that verifies argument-less
pseudo-classes by adding the missing cases from filtersWithoutArguments: add
assertions that calling CSSselect.selectAll(":root(foo)", dom),
CSSselect.selectAll(":hover(foo)", dom), and
CSSselect.selectAll(":visited(foo)", dom) each throw with the respective
messages "Pseudo-class :root doesn't have any arguments", "Pseudo-class :hover
doesn't have any arguments", and "Pseudo-class :visited doesn't have any
arguments" (follow the same pattern used for :scope and :active in the test).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8531de04-fe3e-483c-a000-76c44a59453c
📒 Files selected for processing (2)
src/pseudo-selectors/index.tstest/pseudo-classes.ts
|
The PR description image shows a PowerShell terminal confirming 214 tests passed (1 skipped) across all 8 test files via
✅ Actions performedReview triggered.
|
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
|
Addressed the latest CodeRabbit nitpicks in commit 816ae4d. Updates:
Validation ReRun:
@coderabbitai review |
🧩 Analysis chainThe PR description image shows a PowerShell terminal with Let me pull the latest diff before reviewing. 🏁 Script executed: Length of output: 4965
✅
|
| Guard | Covers |
|---|---|
data === null && name in subselects |
:has, :is, :matches, :where, :not |
data === null && filtersWithArguments.has(name) |
7 filter pseudo-classes |
data !== null && filtersWithoutArguments.has(name) |
:root, :scope, :hover, :visited, :active |
✅ test/pseudo-classes.ts
"requires an argument" block — 12 pseudo-classes covered (all 7 filtersWithArguments + all 5 subselect pseudos). Complete.
"doesn't have any arguments" block — now covers all 5 filtersWithoutArguments members (:scope, :active, :root, :hover, :visited). Complete.
Summary
| Concern | Status |
|---|---|
| Fail-fast set integrity check | ✅ |
filtersWithoutArguments derived from Object.keys |
✅ |
| Redundant size-parity assertion removed | ✅ |
| All three validation guards | ✅ |
| Missing-arg test coverage (12/12) | ✅ |
| Extra-arg test coverage (5/5) | ✅ |
| Build & test suite | ✅ 214 passed, 0 failures |
Everything looks clean and complete. Triggering a final review now.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
🧩 Analysis chainThe PR description image shows a PowerShell terminal where 🏁 Script executed: Length of output: 4965
✅
|
| Item | Status |
|---|---|
filtersWithArguments (7 members) |
✅ |
Fail-fast loop verifying each name exists in filters |
✅ |
filtersWithoutArguments derived from Object.keys(filters) |
✅ |
| Redundant size-parity assertion removed | ✅ |
Guard: data === null && name in subselects |
✅ |
Guard: data === null && filtersWithArguments.has(name) |
✅ |
Guard: data !== null && filtersWithoutArguments.has(name) |
✅ |
✅ test/pseudo-classes.ts
| Block | Coverage |
|---|---|
| "requires an argument" | 12/12 (7 filters + 5 subselects: :has, :is, :matches, :where, :not) |
| "doesn't have any arguments" | 5/5 (:scope, :active, :root, :hover, :visited) |
The implementation is complete and correct. Triggering the final review now.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pseudo-selectors/index.ts`:
- Around line 62-64: Replace all membership checks that use the "in" operator
with Object.hasOwn to ensure only own properties of the pseudo maps are
considered: change occurrences where you test membership against subselects,
filters, and pseudos (e.g., the check in the block that throws `Pseudo-class
:${name} requires an argument` and the other checks at the sites referencing
subselects, filters, and pseudos) to use Object.hasOwn(subselects, name),
Object.hasOwn(filters, name), and Object.hasOwn(pseudos, name) respectively so
prototype properties cannot be mistaken for valid pseudos.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aded63c8-d119-42a8-9ffe-a3176d43df51
📒 Files selected for processing (2)
src/pseudo-selectors/index.tstest/pseudo-classes.ts
🧩 Analysis chainThe PR description image shows a PowerShell terminal session running in 🏁 Script executed: Length of output: 4965 The PR description image confirms 214 tests passed (1 skipped) across all 8 test files via Vitest v4.1.2, and
✅
|
| Set | Members | Count |
|---|---|---|
filtersWithArguments |
contains, icontains, nth-child, nth-last-child, nth-of-type, nth-last-of-type, lang |
7 |
filtersWithoutArguments (derived) |
root, scope, hover, visited, active |
5 |
allFilterNames |
all exports | 12 ✅ |
Three validation guards in compilePseudoSelector
All three guards are placed before any delegate call, so no downstream code can receive invalid data:
| Guard | Pseudo-classes covered |
|---|---|
data === null && name in subselects |
:has, :is, :matches, :where, :not |
data === null && filtersWithArguments.has(name) |
all 7 filters above |
data !== null && filtersWithoutArguments.has(name) |
:root, :scope, :hover, :visited, :active |
The :scope without-arguments path (falling through to filters["scope"](next, null as unknown as string, ...)) is safe — scope forwards to filters["root"] which ignores _rule entirely. No regression introduced.
✅ test/pseudo-classes.ts
| Test block | Coverage |
|---|---|
| "requires an argument" | 12/12 (7 filtersWithArguments + :has, :is, :matches, :where, :not) |
| "doesn't have any arguments" | 5/5 (:scope, :active, :root, :hover, :visited) |
Both blocks are complete.
Summary
| Concern | Status |
|---|---|
| Fail-fast integrity loop | ✅ |
filtersWithoutArguments derived from Object.keys |
✅ |
| Guard: subselects missing arg | ✅ |
| Guard: filters missing arg | ✅ |
| Guard: filters receiving unexpected arg | ✅ |
null cast for :scope without args |
✅ Safe |
| Missing-arg test coverage | ✅ 12/12 |
| Extra-arg test coverage | ✅ 5/5 |
| Build & test suite | ✅ 214 passed, 0 failures |
The implementation is clean, consistent, and complete. Triggering the review now.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
🧩 Analysis chainThe PR description image shows a PowerShell terminal where Let me fetch the latest diff before reviewing. 🏁 Script executed: Length of output: 4965
✅
|
| Set | Members | Count |
|---|---|---|
filtersWithArguments |
contains, icontains, nth-child, nth-last-child, nth-of-type, nth-last-of-type, lang |
7 |
filtersWithoutArguments (derived) |
root, scope, hover, visited, active |
5 |
allFilterNames |
all exports from filters.ts |
12 |
7 + 5 = 12 ✅
Validation guards in compilePseudoSelector
All three guards are placed before their respective delegate calls — no downstream code can receive invalid data:
| Guard | Triggers for |
|---|---|
data === null && name in subselects |
:has, :is, :matches, :where, :not |
data === null && filtersWithArguments.has(name) |
7 filters above |
data !== null && filtersWithoutArguments.has(name) |
:root, :scope, :hover, :visited, :active |
The :scope without-arguments path (data === null, neither filter guard fires) safely reaches filters["scope"](next, null as unknown as string, ...) — the scope filter forwards to filters["root"] in the no-context branch, and root ignores the _rule parameter entirely, so the null cast is harmless pre-existing behaviour. No regression introduced here.
✅ test/pseudo-classes.ts
| Block | Coverage |
|---|---|
| "requires an argument" | 12/12 — all 7 filtersWithArguments + all 5 subselect pseudos (:has, :is, :matches, :where, :not) |
| "doesn't have any arguments" | 5/5 — :scope, :active, :root, :hover, :visited |
Both blocks are complete. No gaps remain.
Summary
| Concern | Status |
|---|---|
| Fail-fast set integrity check | ✅ |
filtersWithoutArguments derived from Object.keys |
✅ |
| Subselect no-arg guard | ✅ |
| Filter missing-arg guard | ✅ |
| Filter extra-arg guard | ✅ |
null cast for :scope without args |
✅ Safe (pre-existing, root ignores rule) |
| Missing-arg test coverage | ✅ 12/12 |
| Extra-arg test coverage | ✅ 5/5 |
| Build & test suite | ✅ 214 passed, 0 failures |
The implementation is clean, the logic is sound, and the test coverage is complete. Triggering a review now.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
|
Addressed the latest CodeRabbit change request in commit c083585. Update:
Validation rerun:
@coderabbitai review |
|
Correction: the commit SHA for the latest fix is 3f1e6ec (not c083585). This commit contains the Object.hasOwn membership-check hardening. |
🧩 Analysis chainThe PR description image shows a PowerShell terminal confirming 214 tests passed (1 skipped) across all 8 test files via Vitest v4.1.2, and Let me fetch the latest diff to verify the 🏁 Script executed: Length of output: 5430
✅
|
| Location | Before | After |
|---|---|---|
| New subselect no-arg guard (line 59) | name in subselects |
Object.hasOwn(subselects, name) |
| Unknown-subselect guard (line 68) | !(name in subselects) |
!Object.hasOwn(subselects, name) |
| Filter branch entry (line 92) | name in filters |
Object.hasOwn(filters, name) |
| Pseudos branch entry (line 110) | name in pseudos |
Object.hasOwn(pseudos, name) |
The Set.prototype.has() calls for filtersWithArguments, filtersWithoutArguments, and allFilterNames are unaffected — those are already prototype-safe by design.
Why this matters: the in operator walks the prototype chain, meaning a pseudo-class named "toString", "hasOwnProperty", or "constructor" would incorrectly match against subselects, filters, or pseudos when using in. Object.hasOwn restricts the check to own enumerable properties only, which is the correct semantics for these plain-object dispatch tables.
✅ test/pseudo-classes.ts — unchanged and complete
| Block | Coverage |
|---|---|
| "requires an argument" | 12/12 (7 filtersWithArguments + 5 subselect pseudos) |
| "doesn't have any arguments" | 5/5 (:scope, :active, :root, :hover, :visited) |
Summary
| Concern | Status |
|---|---|
| Fail-fast integrity loop | ✅ |
filtersWithoutArguments derived from Object.keys |
✅ |
in → Object.hasOwn for subselects (both sites) |
✅ |
in → Object.hasOwn for filters |
✅ |
in → Object.hasOwn for pseudos |
✅ |
| All three validation guards | ✅ |
| Missing-arg test coverage | ✅ 12/12 |
| Extra-arg test coverage | ✅ 5/5 |
| Build & test suite | ✅ 214 passed, 0 failures |
The implementation is clean, complete, and correct. Triggering the review now.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
I feel the current code changes in the commit are fine, and the CodeRabbit is just being too nitpicky. |
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pseudo-selectors/index.ts (1)
74-88:⚠️ Potential issue | 🟠 MajorHarden the alias and user-pseudo lookups to match built-in map protection.
Object.hasOwnnow protects lookups ofsubselects,filters, andpseudos, but lines 74–77 still use unprotected property access onoptions.pseudosandaliases. This creates an inconsistency in the security hardening: inherited properties from a polluted prototype chain can still resolve pseudo handlers or aliases, bypassing the fail-fast validation added for built-in maps.Apply
Object.hasOwnto bothoptions.pseudos?.[name]andaliases[name]to complete the defense-in-depth hardening:🔧 Proposed fix
- const userPseudo = options.pseudos?.[name]; + const userPseudo = + options.pseudos && Object.hasOwn(options.pseudos, name) + ? options.pseudos[name] + : undefined; - const stringPseudo = - typeof userPseudo === "string" ? userPseudo : aliases[name]; + const stringPseudo = + typeof userPseudo === "string" + ? userPseudo + : Object.hasOwn(aliases, name) + ? aliases[name] + : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pseudo-selectors/index.ts` around lines 74 - 88, The lookup for user and alias pseudos currently uses unprotected property access (options.pseudos?.[name] and aliases[name]); change them to use Object.hasOwn to avoid inherited-prototype lookups: resolve userPseudo by checking options.pseudos exists and Object.hasOwn(options.pseudos, name) ? options.pseudos[name] : undefined, and resolve alias similarly with Object.hasOwn(aliases, name) ? aliases[name] : undefined before assigning stringPseudo and proceeding to parse(stringPseudo) and call subselects["is"](next, alias, ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pseudo-selectors/index.ts`:
- Around line 74-88: The lookup for user and alias pseudos currently uses
unprotected property access (options.pseudos?.[name] and aliases[name]); change
them to use Object.hasOwn to avoid inherited-prototype lookups: resolve
userPseudo by checking options.pseudos exists and Object.hasOwn(options.pseudos,
name) ? options.pseudos[name] : undefined, and resolve alias similarly with
Object.hasOwn(aliases, name) ? aliases[name] : undefined before assigning
stringPseudo and proceeding to parse(stringPseudo) and call
subselects["is"](next, alias, ...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac0164ac-1e66-47bc-9634-27c2f8be3f2d
📒 Files selected for processing (2)
src/pseudo-selectors/index.tstest/pseudo-classes.ts
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/pseudo-classes.ts`:
- Around line 181-217: Remove the duplicated assertions that re-check the same
argument-less pseudo-classes; in the test case using CSSselect.selectAll, delete
the repeated expect blocks for :root, :hover, and :visited that duplicate
earlier checks so each pseudo-class (:scope, :active, :root, :hover, :visited,
:enabled) is asserted only once that it throws when given arguments.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa8ead26-2ee5-4426-9b3e-0997a8f26a92
📒 Files selected for processing (1)
test/pseudo-classes.ts
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
Signed-off-by: Mridankan Mandal <xerontitan90@gmail.com>
2 participants
Description:
Verification:
Fixes #1766
The screenshot of the tests run, for validation:
Summary by CodeRabbit
Bug Fixes
Tests