Skip to content

color() value parser#815

Merged
dahlia merged 12 commits into
mainfrom
feature/color
May 14, 2026
Merged

color() value parser#815
dahlia merged 12 commits into
mainfrom
feature/color

Conversation

@dahlia
Copy link
Copy Markdown
Owner

@dahlia dahlia commented May 14, 2026

Adds a color() value parser to @optique/core that accepts CSS color strings and returns a typed Color object with r, g, b (0–255 integers) and a (0–1) fields.

The parser accepts four notations, all case-insensitive: hex (#f00, #ff0000, #ff0000ff), RGB/RGBA (rgb(255, 0, 0), rgba(255, 0, 0, 0.5)), HSL/HSLA (hsl(0, 100%, 50%)), and all 148 CSS Level 4 named colors including transparent and rebeccapurple. The formats option restricts which notations are accepted. The format() method always outputs canonical lowercase hex, so parse(format(x)) round-trips reliably. Named colors appear as shell completion suggestions via suggest().

A Codex review caught three edge cases in the initial implementation. The RGB/HSL regexes used [\d.]+ for numeric components, which matched strings like . and 1..; parseFloat then returned NaN or silently truncated, bypassing the range checks. The pattern is now (?:\d+(?:\.\d*)?|\d*\.\d+), with Number.isFinite() guards added after each parseFloat() call. Named color parsing was returning a reference to the shared lookup table entry rather than a copy, so a caller using a type cast could mutate it and affect every subsequent parse of that name. And format() accepted any number for each channel, meaning { r: 300, g: 0, b: 0, a: 1 } silently formatted as "#12c0000" and a: NaN produced "#000000NaN"; the method now throws RangeError for non-integer r/g/b, out-of-range, or non-finite values.

Files changed: packages/core/src/valueparser.ts, packages/core/src/valueparser.test.ts, docs/concepts/valueparsers.md, CHANGES.md.

Closes #810.

dahlia added 2 commits May 15, 2026 00:09
Adds a color() value parser that accepts CSS color strings and
returns a structured Color object with r, g, b (0-255) and a (0-1)
fields.

Supported input notations:
- Hex: #rgb, #rrggbb, #rgba, #rrggbbaa (case-insensitive)
- RGB/RGBA: rgb(r, g, b) / rgba(r, g, b, a)
- HSL/HSLA: hsl(h, s%, l%) / hsla(h, s%, l%, a)
- Named: all 148 CSS Level 4 named colors including transparent
  and rebeccapurple (case-insensitive lookup)

The formats option restricts which notations are accepted.  The
format() method always outputs canonical lowercase hex (#rrggbb
or #rrggbbaa when alpha < 1).  Named colors are surfaced as shell
completion suggestions via suggest().

Closes #810

Assisted-by: Claude Code:claude-sonnet-4-6
Three issues found and fixed in the color() parser:

- Tighten RGB/HSL regexes so that malformed numeric strings like "."
  and "1.." no longer match.  The previous [\d.]+ pattern allowed
  parseFloat() to return NaN or silently truncate, bypassing range
  checks.  The new pattern requires at least one digit on at least one
  side of any decimal point.  Number.isFinite() guards are also added
  after each parseFloat() call as a second line of defense.

- Named color parsing now returns a shallow copy of the stored Color
  object instead of a direct reference.  This prevents a caller from
  mutating the shared table entry and affecting all future parses.

- format() now validates all Color components before formatting.
  It throws a RangeError if r, g, or b are non-integer or outside
  0–255, or if a is non-finite or outside 0–1.  Previously,
  out-of-range values produced corrupt hex strings (e.g. "#12c0000"
  for r=300) and NaN components produced literal "NaN" in the output.

Regression tests added for all three cases.

Assisted-by: Claude Code:claude-sonnet-4-6
Assisted-by: Codex:gpt-5.5
@dahlia dahlia added this to the Optique 1.1 milestone May 14, 2026
@dahlia dahlia self-assigned this May 14, 2026
@dahlia dahlia added the enhancement New feature or request label May 14, 2026
@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@codex review

@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

❌ Patch coverage is 97.09763% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.23%. Comparing base (b7bb457) to head (85ae195).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/core/src/valueparser.ts 97.09% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #815      +/-   ##
==========================================
+ Coverage   95.21%   95.23%   +0.01%     
==========================================
  Files          50       50              
  Lines       30184    30563     +379     
  Branches     7834     7909      +75     
==========================================
+ Hits        28740    29106     +366     
- Misses       1383     1396      +13     
  Partials       61       61              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new color() value parser to the @optique/core package, enabling the parsing of CSS color strings—including hex, RGB/RGBA, HSL/HSLA, and 148 named colors—into structured objects. The implementation includes a format() method for canonical hex output and support for shell completion. Reviewers identified an opportunity to better align HSL hue parsing with the CSS specification by allowing wrapping values and suggested a performance optimization in the suggest() method to avoid redundant array allocations.

Comment thread packages/core/src/valueparser.ts Outdated
Comment thread packages/core/src/valueparser.ts Outdated
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR adds a new color() CSS value parser to Optique. The implementation parses hex, rgb/rgba, hsl/hsla, and W3C named color strings into a normalized {r, g, b, a} object. It supports optional format restrictions, outputs canonical lowercase hex notation, and provides prefix-based suggestions for named colors. The feature includes comprehensive tests covering parsing logic, format filtering, error messaging, and a complete user documentation section.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly summarizes the main change: adding a color() value parser. It is concise and specific.
Description check ✅ Passed The description comprehensively relates to the changeset, detailing the parser's functionality, accepted formats, edge-case fixes, and affected files.
Linked Issues check ✅ Passed All objectives from issue #810 are met: hex, RGB/RGBA, HSL/HSLA, named colors, structured Color object, formats option, and canonical formatting for round-tripping.
Out of Scope Changes check ✅ Passed All changes are in scope: valueparser implementation/tests, documentation updates, and changelog entry for the color() parser feature.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/color

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.

❤️ Share

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/src/valueparser.test.ts`:
- Around line 16730-16732: The tests that currently call assert.throws(() =>
color({ metavar: "" as NonEmptyString }), TypeError) should be strengthened to
also assert the exact error message: capture the thrown error (e.g., const err =
assert.throws(...)) and assert.strictEqual(err.message, "<expected diagnostic
string>"); do the same for the invalid formats-entry test that constructs color
with a bad formats value (replace its assert.throws(TypeError) with capture +
assert.strictEqual on err.message). Reference the color constructor and the
NonEmptyString/formats option names to locate the two failing test cases and
update their assertions to check both error type and exact diagnostic string.
- Around line 17070-17073: The test currently uses a loose threshold
(suggestions.length > 100) which can mask regressions; update the assertion to
require the exact CSS Level 4 named-color count by computing the unique
suggestions (use the suggestions array / variable and dedupe via a Set or
equivalent) and assert strict equality against the expected constant (the known
exact number of CSS Level 4 named colors) instead of a greater-than check;
locate the assertion around the test block that references suggestions and
replace the loose threshold with a strict equality assertion (e.g., compare
suggestions' unique count to the expected constant).

In `@packages/core/src/valueparser.ts`:
- Around line 2311-2329: The JSDoc for the exported color() parser only lists
TypeError but omits that format(value: Color) can throw a RangeError for invalid
channel ranges; update the JSDoc on the color() factory and the format() method
documentation to add an `@throws` {RangeError} entry describing that format(value:
Color) will throw when any RGBA/HSL channel is outside its valid range (e.g.,
r/g/b not in 0–255, a not in 0–1, hue/saturation/lightness out of expected
bounds), and mirror the same `@throws` RangeError note in the other documentation
block where format() is described (the secondary JSDoc mentioned in the
comment).
- Around line 2238-2246: The regexes currently use optional "a" (e.g., /^rgba?/
and /^hsla?/) which allows mismatched arity (rgb with alpha or rgba without);
replace them with strict separate patterns for the two variants: one for rgb (no
alpha) and one for rgba (requires alpha), and likewise one for hsl (no alpha)
and one for hsla (requires alpha). Update usages that reference COLOR_RGB_REGEX
and COLOR_HSL_REGEX (and the other occurrences around 2392-2427) to use the new
pair of constants (e.g., COLOR_RGB_REGEX vs COLOR_RGBA_REGEX, and
COLOR_HSL_REGEX vs COLOR_HSLA_REGEX) so that "rgb(...)"/ "hsl(...)" reject an
alpha argument and "rgba(...)"/ "hsla(...)" require it.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 24156368-f414-4934-ac39-33250fafcd78

📥 Commits

Reviewing files that changed from the base of the PR and between b7bb457 and deca48a.

📒 Files selected for processing (4)
  • CHANGES.md
  • docs/concepts/valueparsers.md
  • packages/core/src/valueparser.test.ts
  • packages/core/src/valueparser.ts

Comment thread packages/core/src/valueparser.test.ts
Comment thread packages/core/src/valueparser.test.ts
Comment thread packages/core/src/valueparser.ts
Comment thread packages/core/src/valueparser.ts
dahlia added 3 commits May 15, 2026 00:46
hslToRgb already normalises any angle via modulo, so restricting the
parsed hue to [0, 360] was more conservative than CSS Level 4 requires.
Hue values above 360 (e.g. 361, 720) now parse successfully and wrap
correctly. The regex still rejects negative hue input (it has no leading-
minus branch), so hsl(-1, ...) continues to fail as before.

Updated the hsl(361) test to expect success, and clarified the hsl(-1)
test description to reflect the actual rejection reason.

#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
Extract Object.keys(CSS_NAMED_COLORS) into a module-level constant
CSS_NAMED_COLOR_KEYS so that suggest() iterates a pre-built array
instead of allocating 149 strings on every tab-completion keystroke.

Add a @throws {RangeError} tag to the color() JSDoc describing the
condition under which the returned parser's format() method throws,
as required by the project coding guidelines ("functions or methods
that throw exceptions must include the @throws tag").

#815 (comment)
#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
Constructor-validation tests now assert the exact error message string
rather than only the error class, so a silent change to a diagnostic
is caught as a regression.

The suggest() test for empty-prefix completion replaces the loose
`suggestions.length > 100` guard with an exact equality check against
149 (the 148 CSS Level 4 named colors plus transparent), matching
what the parser actually promises.

#815 (comment)
#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@coderabbitai review

@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

/gemini review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

✅ 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.

@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 82402d3964

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/src/valueparser.ts Outdated
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new color() value parser to the @optique/core package, allowing CSS color strings to be parsed into structured Color objects. The implementation supports hex, RGB/RGBA, HSL/HSLA, and CSS Level 4 named colors, and includes a format() method for canonical hex output. The changes also provide comprehensive documentation and extensive unit tests. I have no feedback to provide.

The COLOR_HSL_REGEX pattern used an unsigned-only numeric pattern for
the hue component, so inputs like hsl(-120, 100%, 50%) were rejected
even though CSS <angle> values allow negative hues and hslToRgb already
normalises any angle via modulo arithmetic.

Added COLOR_HUE_PATTERN = "(?:-?<num>)" and used it for the first
capture group of COLOR_HSL_REGEX only; saturation, lightness, and alpha
remain non-negative. Updated the negative-hue test to expect success and
assert the wrapped result: hsl(-120, 100%, 50%) → blue (same as 240°).

#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@codex review

@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

/gemini review

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/src/valueparser.ts`:
- Around line 2360-2368: The fallback error in invalidFormatError always lists
hex, rgb, and named examples; update invalidFormatError (in function
invalidFormatError) to build the fallback message from options.formats so it
only advertises allowed notations. Check options.formats (and default if
missing), map each allowed format string to a representative example (e.g.,
"hex" -> "#ff0000", "rgb" -> "rgb(255, 0, 0)", "named" -> "red"), join them into
the message template used when options.errors?.invalidFormat is not provided,
and return that constructed message instead of the hard-coded examples.
- Around line 2398-2464: The parser currently preserves arbitrary float alphas
(set in the COLOR_RGB_REGEX and COLOR_HSL_REGEX branches) while format() encodes
alpha as 8-bit hex, causing non-roundtripable values; fix by either quantizing
parsed alpha to 8-bit or rejecting non-8-bit alphas. Concretely, inside the
COLOR_RGB_REGEX branch (where a is set from parseFloat(m[4])) and the
COLOR_HSL_REGEX branch (where a is set from parseFloat(m[4]) before conversion),
replace the raw a with a quantized value (a = Math.round(a * 255) / 255) or
validate and return invalidFormatError(input) if Math.round(a * 255) / 255 !==
a, so parsed Color objects will match format(Color) output; ensure the same
behavior is applied to any other code paths that set a (e.g., any named-color
handling if/where alpha can be present).
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e2112ea3-278a-4d7a-becf-8e15f7b32bae

📥 Commits

Reviewing files that changed from the base of the PR and between 82402d3 and 9ca7308.

📒 Files selected for processing (2)
  • packages/core/src/valueparser.test.ts
  • packages/core/src/valueparser.ts

Comment thread packages/core/src/valueparser.ts
Comment thread packages/core/src/valueparser.ts Outdated
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new color() value parser to the @optique/core package, enabling the parsing of CSS color strings—including hex, RGB, HSL, and 148 named colors—into structured Color objects. The implementation includes comprehensive documentation, extensive unit tests, and support for shell completion of named colors. Feedback was provided to improve the robustness of named color lookups by using the in operator to better handle potential proxy objects and align with the repository's type-guarding patterns.

Comment thread packages/core/src/valueparser.ts
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

dahlia added 2 commits May 15, 2026 01:40
Two fixes to parse() in color():

The default error message now lists only the notations that the
formats option allows. Previously, color({ formats: ["hex"] }).parse("red")
told the user to try "rgb(255, 0, 0)" or "red", both of which that
parser then rejects. The message is now built dynamically from the
set of enabled formats using valueSet() with type "disjunction", so
the examples always match what parse() actually accepts.

Named color lookup now uses the `in` operator to test property
existence rather than !== undefined. This is more semantically
correct for a plain-object lookup table and consistent with the
TypeScript patterns used elsewhere in the codebase.

#815 (comment)
#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
parse("rgba(0,0,0,0.5)") previously produced a Color with a === 0.5,
but format() always encodes alpha as an 8-bit hex byte (0x80/255 ≈
0.502), so parse(format(parse(rgb_input))) would differ from the
original parsed value. This made the round-trip guarantee inconsistent
between hex-originated and rgb/hsl-originated Colors.

Alpha is now quantized immediately after range validation:
  a = Math.round(aRaw * 255) / 255
so every Color object produced by parse() carries alpha at 1/255
granularity, matching what format() would recover. The three test
assertions that expected exact 0.5 are updated to use 128/255.

#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@codex review

@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

/gemini review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5e6ec8ca2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/src/valueparser.ts Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/src/valueparser.ts`:
- Around line 2450-2453: The lookup for named colors uses the `in` operator
which matches inherited keys (e.g., "constructor"); change the check to an
own-property test such as
`Object.prototype.hasOwnProperty.call(CSS_NAMED_COLORS, key)` (or
`CSS_NAMED_COLORS.hasOwnProperty(key)`) in the block where `const key =
trimmed.toLowerCase();` so only real named colors trigger `return { success:
true, value: { ...CSS_NAMED_COLORS[key] } };` and avoid accepting non-color
inputs.
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c78a23b7-cc94-4dd4-92ae-c700937cd28d

📥 Commits

Reviewing files that changed from the base of the PR and between 9ca7308 and a5e6ec8.

📒 Files selected for processing (2)
  • packages/core/src/valueparser.test.ts
  • packages/core/src/valueparser.ts

Comment thread packages/core/src/valueparser.ts
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new color() value parser to @optique/core, enabling the parsing of CSS color strings into structured Color objects. The implementation supports hex, RGB/RGBA, HSL/HSLA, and CSS Level 4 named colors, including shell completion and canonical hex formatting. Feedback on the implementation highlights that the rgb() and hsl() parsers are currently limited to legacy comma-separated syntax and do not support modern CSS Color Level 4 features like space-separated values or slash-separated alpha. Additionally, redundant Math.round() calls were identified in the format() method, as the inputs are already validated as integers.

Comment thread packages/core/src/valueparser.ts
Comment thread packages/core/src/valueparser.ts Outdated
dahlia added 2 commits May 15, 2026 01:58
The previous `key in CSS_NAMED_COLORS` check walks the prototype chain,
so inherited keys like "constructor", "toString", and "__proto__" all
returned true. CSS_NAMED_COLORS["constructor"] resolves to
Object.prototype.constructor (a function), and spreading it produces an
object without valid r/g/b/a fields, so the parser returned
{ success: true } with a malformed Color for those inputs.

Object.hasOwn() restricts the check to the object's own properties,
which is the correct guard for a plain lookup table. Added regression
tests for the three canonical prototype-inherited keys to prevent this
class of bug from regressing silently.

#815 (comment)
#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
The validation guard at the top of format() already ensures that r, g,
and b are integers via Number.isInteger(), so wrapping each channel in
Math.round() was a no-op. Removing it makes the code match the project's
guideline against adding validation for scenarios that cannot occur.

The alpha channel keeps Math.round(a * 255) since a is a float whose
product with 255 may not be an exact integer.

#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@codex review

@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

/gemini review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/src/valueparser.ts`:
- Around line 2352-2356: The color parser allows configuring allowed formats
(allowedFormats / allowHex / allowRgb / allowHsl / allowNamed) but format() can
still emit a notation (e.g., hex) that parse() will reject when that branch is
disabled; fix by ensuring format() only emits a notation included in
options.formats or by validating options at construction time and throwing if
formats would prevent round-trip (e.g., disallow omitting "hex" when format()
might output hex), update the code paths that pick output notation to consult
allowedFormats and adjust VALID_COLOR_FORMATS/allowedFormats normalization so
parse() and format() are consistent (identify the responsible functions format()
and parse() and the config handling around allowedFormats to change).
- Around line 2383-2491: The parser currently quantizes alpha in parse() but
lacks a normalize() hook, so defaults/placeholders can carry non-canonical
values; add a normalize(value: Color) function on the returned parser object
(next to parse(), format(), suggest()) that: validates/clamps r,g,b to integers
in 0..255, quantizes a to Math.round(a * 255) / 255 while ensuring a is finite
and in 0..1, and returns the normalized Color (or throws/returns an error
consistent with this value parser API on invalid input); reference parse(),
format(), and the returned parser object when locating where to add normalize().
🪄 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: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 41bc57bb-adc6-480c-b832-0c45b131e5fb

📥 Commits

Reviewing files that changed from the base of the PR and between a5e6ec8 and 7f46dc6.

📒 Files selected for processing (2)
  • packages/core/src/valueparser.test.ts
  • packages/core/src/valueparser.ts

Comment thread packages/core/src/valueparser.ts
Comment thread packages/core/src/valueparser.ts
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new color() value parser to @optique/core, enabling the parsing of CSS color strings—including hex, RGB/RGBA, HSL/HSLA, and 148 named colors—into structured Color objects. The implementation includes comprehensive documentation, extensive unit tests, and support for shell completion of named colors. Review feedback identified that the rgb()/rgba() parsing logic should be updated to use more flexible numeric patterns and robust validation guards to ensure consistency with CSS Level 4 standards and the PR's stated objectives.

Comment thread packages/core/src/valueparser.ts
Comment thread packages/core/src/valueparser.ts
format() previously always emitted hex, so color({ formats: ["rgb"] })
would produce "#ff0000" from format() while parse() would reject that
same string. format() now picks the first notation that formats allows:
hex (unchanged) → rgb → hsl (using a new rgbToHsl() helper) → named
color name via reverse lookup → hex fallback. This ensures that
format() output can always be fed back into parse() on the same parser.

normalize() is a new optional method that applies the alpha quantization
parse() already performs — Math.round(a * 255) / 255 — so programmatic
Color values and placeholders reach the same canonical form as values
that came through parse(). Values with out-of-range channels are
returned unchanged, matching the ValueParser interface contract.

#815 (comment)
#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@codex review

@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

/gemini review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a379f940b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/core/src/valueparser.ts Outdated
Comment thread packages/core/src/valueparser.ts Outdated
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new color() value parser to the @optique/core package, which parses CSS color strings into structured Color objects. The parser supports hex, RGB/RGBA, HSL/HSLA, and 148 CSS Level 4 named colors, providing normalized RGBA components and a canonical hex format() method. The implementation is accompanied by comprehensive unit tests and updated documentation. I have no feedback to provide.

rgbToHsl() rounded h/s/l to integers, so colors with very low lightness
lost information. For example, {r:0,g:0,b:1} formatted as
"hsl(240, 100%, 0%)" which re-parses to black {r:0,g:0,b:0} instead of
the original value. rgbToHsl() now returns unrounded floats, and
format() outputs them via parseFloat(x.toFixed(4)) — enough precision
for parse() to recover the exact same r/g/b integer values.

In named-only mode, format() fell back to hex when no keyword matched,
but parse() rejects hex when formats excludes "hex". The silent fallback
is replaced with a RangeError so the misconfiguration is caught
explicitly rather than producing output the same parser cannot consume.

#815 (comment)
#815 (comment)

Assisted-by: Claude Code:claude-sonnet-4-6
@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

@codex review

@dahlia
Copy link
Copy Markdown
Owner Author

dahlia commented May 14, 2026

/gemini review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new color() value parser to the @optique/core package, allowing CSS color strings to be parsed into structured Color objects. The implementation supports hex notation, RGB/RGBA, HSL/HSLA, and all 148 CSS Level 4 named colors, complete with documentation and extensive tests. Review feedback suggests improving the RGB/RGBA parser to support floating-point values for color channels as specified in CSS Color Level 4, which would require updating the regex patterns and switching from parseInt to parseFloat during component extraction.

Comment thread packages/core/src/valueparser.ts
Comment thread packages/core/src/valueparser.ts
@dahlia dahlia merged commit 01bc5b1 into main May 14, 2026
8 checks passed
@dahlia dahlia deleted the feature/color branch May 14, 2026 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

color() value parser for CSS-like color notations

1 participant