fix: include filename and line numbers in CSS parse errors #19282
Conversation
- Add getLineAndColumn helper to calculate position from buffer index - Add formatError helper to format errors with source location - Update all CSS parser error messages to include filename:line:column - Add comprehensive test coverage for error reporting - Maintain backward compatibility when no filename provided Fixes tailwindlabs#19236
WalkthroughAdds an exported `CssSyntaxError` and threads an optional source context through the CSS parser so parse errors include filename, line, and column when a source is provided. `parseString` now accepts an optional `source` parameter. Multiple error paths that previously threw generic `Error` now throw `CssSyntaxError` with attached location data, AST nodes carry source locations, and tests (including a CLI test) are updated to assert file/line reporting. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integrations/cli/index.test.ts(1 hunks)packages/tailwindcss/src/css-parser.test.ts(2 hunks)packages/tailwindcss/src/css-parser.ts(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
integrations/cli/index.test.ts (1)
integrations/utils.ts (3)
test(80-455)json(523-523)css(518-518)
packages/tailwindcss/src/css-parser.ts (1)
packages/tailwindcss/src/source-maps/source.ts (1)
Source(6-18)
🔇 Additional comments (13)
packages/tailwindcss/src/css-parser.ts (8)
39-53: LGTM - Clean line/column calculation.The function correctly computes 1-indexed line and column numbers by scanning the input. The approach is straightforward and appropriate for error reporting.
55-62: LGTM - Error formatting is clear and consistent.The function properly handles both cases: returning the plain message when source is unavailable, and appending location info in the standard
at file:line:columnformat when source is provided.
166-166: LGTM - Source propagation is correct.The
parseStringfunction signature correctly accepts an optionalsourceparameter, and both call sites properly propagate the source context for accurate error reporting.Also applies to: 220-220, 622-622
297-297: LGTM - Accurate error location for custom properties.The error correctly uses the
startposition (captured at line 207) to point to the beginning of the invalid custom property.
362-362: LGTM - Error locations point to declaration start.Both error sites correctly use
bufferStartto point to the beginning of the invalid declaration, providing users with a clear starting point for debugging.Also applies to: 481-481
419-419: LGTM - Bracket mismatch errors point to the problem location.Both errors correctly use the current position
i(the unexpected closing bracket) to help users identify where the bracket mismatch occurs.Also applies to: 520-520
562-565: LGTM - End-of-file position is appropriate for unclosed blocks.Using
input.lengthcorrectly points to the end of the file where the closing brace is missing. Including the selector or at-rule name in the message provides helpful context.
664-686: LGTM - Unterminated string errors are well-formatted.The errors correctly point to the opening quote position and helpfully append the closing quote character to show users what the complete string should look like.
packages/tailwindcss/src/css-parser.test.ts (4)
1219-1225: LGTM - Single-line error test is correct.The test properly verifies that parse errors include the filename and line number when the
fromoption is provided.
1227-1238: LGTM - Multi-line error test validates line number calculation.The test correctly verifies that line numbers are accurately calculated for errors in multi-line CSS files.
1240-1253: LGTM - Missing brace error test is accurate.The test correctly validates that bracket mismatch errors include precise location information.
1279-1285: LGTM - Validates backward compatibility without source.The test correctly verifies that when the
fromoption is not provided, error messages omit the filename and location, maintaining backward compatibility.integrations/cli/index.test.ts (1)
2108-2134: LGTM - Comprehensive CLI integration test.The test validates the complete workflow from CLI input through parsing to error output, ensuring that parse errors include filename and precise location information in real-world usage.
There was a problem hiding this comment.
- I would use an error subclass that stores the
Source+offset— you can format the message in the constructor instead of wrapping every message in a call toformatError() - Maybe call the error subclass
CssSyntaxError— I think PostCSS uses the same name for their errors and it feels like a good one imo. - We already have a
getLineAndColumn-like helper we use for source maps. Replace uses of that withcreateLineTable(source.code).find(offset)
(I'll make these changes on Monday unless you get to them before I do)
|
@thecrypticace I already made the changes shall i commit? |
- Use simpler CSS that actually triggers parse errors - Fix regex patterns to match actual error output - Remove inline snapshots from describe.each blocks - Update CLI test to match actual error line numbers All tests should now pass with the CssSyntaxError implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integrations/cli/index.test.ts(2 hunks)packages/tailwindcss/src/css-parser.test.ts(2 hunks)packages/tailwindcss/src/css-parser.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- integrations/cli/index.test.ts
- packages/tailwindcss/src/css-parser.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/css-parser.ts (2)
packages/tailwindcss/src/source-maps/source.ts (1)
Source(6-18)packages/tailwindcss/src/source-maps/line-table.ts (1)
createLineTable(50-100)
createLineTable().find() returns 0-based columns per source map spec, but users expect 1-based column numbers. Add 1 to column before formatting error message so both line and column are 1-based and accurate. Addresses reviewer feedback about off-by-one column numbers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tailwindcss/src/css-parser.ts (1)
549-554: Consider pointing to block start for unclosed block errors.Currently, these errors report position
input.length(EOF), which is where the error is detected. For better developer experience, consider pointing to where the unclosed block started. Theparent.src[1]contains the start position when source tracking is enabled.Example improvement:
if (parent.kind === 'rule') { - throw new CssSyntaxError(`Missing closing } at ${parent.selector}`, source, input.length) + throw new CssSyntaxError(`Missing closing } at ${parent.selector}`, source, parent.src?.[1] ?? input.length) } if (parent.kind === 'at-rule') { - throw new CssSyntaxError(`Missing closing } at ${parent.name} ${parent.params}`, source, input.length) + throw new CssSyntaxError(`Missing closing } at ${parent.name} ${parent.params}`, source, parent.src?.[1] ?? input.length) }This would show developers exactly where the unclosed block began rather than just pointing to EOF.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/tailwindcss/src/css-parser.test.ts(2 hunks)packages/tailwindcss/src/css-parser.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tailwindcss/src/css-parser.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/css-parser.ts (2)
packages/tailwindcss/src/source-maps/source.ts (1)
Source(6-18)packages/tailwindcss/src/source-maps/line-table.ts (1)
createLineTable(50-100)
🔇 Additional comments (3)
packages/tailwindcss/src/css-parser.ts (3)
40-50: LGTM! Column offset correctly adjusted.The CssSyntaxError class properly formats error messages with file location. The
column + 1adjustment at line 46 correctly converts the 0-based column returned bycreateLineTable().find()to a 1-based column for display, making both line and column 1-based in the output as expected.
154-154: Excellent error reporting improvements.All error throw sites now correctly use
CssSyntaxErrorwith source and position information. The positions chosen are appropriate:
- Invalid declarations/properties point to the start of the construct (
bufferStart,start)- Mismatched brackets point to where the mismatch was detected (
i)This provides actionable error messages for developers.
Also applies to: 208-208, 285-285, 350-350, 407-407, 469-469, 508-508
610-610: LGTM! parseString signature and error handling are correct.The signature change adds an optional
sourceparameter with a default value ofnull, maintaining backward compatibility. The unterminated string errors correctly usestartIdx(the start of the string literal) as the position, which helps developers quickly locate the problematic string.Also applies to: 652-654, 671-673
Add minimal JSDoc documentation to satisfy docstring coverage requirement for the exported CssSyntaxError class.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/tailwindcss/src/css-parser.ts (1)
40-53: The column off-by-one issue has been resolved.The implementation correctly adds 1 to the column at line 49, converting the 0-based column from
createLineTable().find()to a 1-based column for user-facing error messages. The CssSyntaxError class properly formats errors with filename and position context.
🧹 Nitpick comments (1)
packages/tailwindcss/src/css-parser.ts (1)
288-288: Consider including the buffer content in the error message.For consistency with other invalid declaration errors (lines 353, 472) and to aid debugging, include the parsed content in the error message.
Apply this diff:
- if (!declaration) throw new CssSyntaxError(`Invalid custom property, expected a value`, source, start) + if (!declaration) throw new CssSyntaxError(`Invalid custom property, expected a value: \`${buffer.trim()}\``, source, start)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/tailwindcss/src/css-parser.ts(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/css-parser.ts (2)
packages/tailwindcss/src/source-maps/source.ts (1)
Source(6-18)packages/tailwindcss/src/source-maps/line-table.ts (1)
createLineTable(50-100)
🔇 Additional comments (2)
packages/tailwindcss/src/css-parser.ts (2)
157-157: LGTM: parseString properly threads source context.The signature update and call sites correctly propagate the optional source parameter, enabling location-aware error reporting for unterminated strings.
Also applies to: 211-211, 613-613
655-677: LGTM: Unterminated string errors provide helpful context.Including the string slice in the error message aids debugging. For typical CSS, the content length is reasonable; extreme cases are rare enough not to warrant truncation.
|
Don't worry about the test failures. I'm gonna make some changes. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/tailwindcss/src/css-parser.ts (1)
46-50: Fix the reported column number off-by-one.The
find()method returns a 0-based column (per the source map spec), so the error message reports a column that is one less than the actual character position. Add 1 before formatting the message so both line and column are 1-based.Apply this diff to fix the column offset:
- let start = createLineTable(source.code).find(loc[1]) - message = `${source.file}: ${start.line}:${start.column}: ${message}` + let start = createLineTable(source.code).find(loc[1]) + message = `${source.file}: ${start.line}:${start.column + 1}: ${message}`
🧹 Nitpick comments (1)
packages/tailwindcss/src/css-parser.ts (1)
50-50: Prefer standard error format without space after filename.The current format
${source.file}: ${start.line}:${start.column}: ${message}includes a space after the filename. Standard error formats (GCC, Clang, ESLint) usefile:line:column: messagewithout that space, which improves consistency with tooling expectations.Apply this diff:
- message = `${source.file}: ${start.line}:${start.column}: ${message}` + message = `${source.file}:${start.line}:${start.column}: ${message}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/tailwindcss/src/css-parser.test.ts(6 hunks)packages/tailwindcss/src/css-parser.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tailwindcss/src/css-parser.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/css-parser.ts (2)
packages/tailwindcss/src/source-maps/source.ts (2)
SourceLocation(27-27)Source(6-18)packages/tailwindcss/src/source-maps/line-table.ts (1)
createLineTable(50-100)
🔇 Additional comments (5)
packages/tailwindcss/src/css-parser.ts (5)
12-13: LGTM!The new imports correctly support error location tracking.
297-302: LGTM!The error sites correctly throw
CssSyntaxErrorwith accurate source locations. Position tracking appropriately uses content spans for syntax errors and point locations for unexpected closing brackets.Also applies to: 367-371, 427-427, 489-494, 533-533
166-166: LGTM!Both
parseStringcall sites correctly pass thesourceparameter for location-aware error reporting.Also applies to: 220-220
642-717: LGTM!The
parseStringsignature change is backward compatible, and both unterminated string error paths correctly throwCssSyntaxErrorwith accurate location spans from the opening quote to the error point.
53-61: LGTM!The error class correctly follows standard Error subclass patterns: calls super, sets custom name, stores location data, and captures stack traces when available.
2 participants
Summary
Fixes CSS parse errors to include filename and line numbers, making debugging much easier for developers.
Currently, CSS parsing errors only show generic messages like:
This gives no clue about the file or line causing the issue.
This change adds filename and line/column info to error messages, allowing developers to quickly locate and fix syntax problems.
Key updates include:
getLineAndColumn()helper to calculate line and column positionsformatError()for standardized error outputparseString()to pass source metadataExample output improvement:
Before:
After:
Test plan
No breaking changes.
Fixes #19236