fix: CSS source map line offsets when wrapping extracted CSS with @cssId#2514
fix: CSS source map line offsets when wrapping extracted CSS with @cssId#2514
@cssId#2514Conversation
🦋 Changeset detectedLatest commit: 17bb077 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughFixes CSS source-map line offsets when extracted CSS is wrapped with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts (1)
175-211: Add a regression case for the@cssIdwrapper offset.This validates the new pipeline, but it does not reproduce the PR’s stated failure mode: generated CSS lines shifted by wrapping extracted CSS with
@cssId. Please add a case where the reported diagnostic line includes the wrapper offset and assert it maps back to the original CSS line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts` around lines 175 - 211, Add a regression test that verifies processTasmCSSDiagnostics correctly removes the `@cssId` wrapper offset when mapping diagnostics back to original source: create a new case in cssDiagnostics.test.ts that uses processTasmCSSDiagnostics and supplies cssDiagnostics JSON containing a diagnostic whose "line" value includes the wrapper offset (e.g., line = originalLine + wrapperOffset), reuse the existing sourceMap and context '/workspace/app', and assert the returned diagnostic's sourceLine/sourceColumn correspond to the original CSS location (originalLine and originalColumn) and not the wrapped line; reference processTasmCSSDiagnostics, mainCSSSourceMap, cssDiagnostics, and emittedWarnings to implement the test.packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts (1)
9-9: Use the sharedgetMainCSSSourceMaphelper here too.
cssDiagnostics.tsnow exports the same helper, but this plugin still calls its local duplicate. Importing the shared helper keeps future source-map fixes covered by one implementation.♻️ Suggested refactor
-import type * as CSS from '@lynx-js/css-serializer'; - -import { processTasmCSSDiagnostics } from './cssDiagnostics.js'; +import { + getMainCSSSourceMap, + processTasmCSSDiagnostics, +} from './cssDiagnostics.js';Then remove the now-duplicated local
Asset,normalizeCSSSourceMap, andgetMainCSSSourceMapdefinitions near the bottom of this file.Also applies to: 239-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts` at line 9, The plugin currently imports processTasmCSSDiagnostics but still uses a duplicated local implementation of getMainCSSSourceMap (and related Asset and normalizeCSSSourceMap) instead of the shared helper; replace the local helper usage by importing getMainCSSSourceMap from cssDiagnostics.js alongside processTasmCSSDiagnostics in LynxEncodePlugin.ts (use the shared getMainCSSSourceMap where the plugin currently calls its duplicate), then remove the duplicated local definitions Asset, normalizeCSSSourceMap, and getMainCSSSourceMap at the bottom of the file so the plugin uses the single shared implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts`:
- Line 9: The plugin currently imports processTasmCSSDiagnostics but still uses
a duplicated local implementation of getMainCSSSourceMap (and related Asset and
normalizeCSSSourceMap) instead of the shared helper; replace the local helper
usage by importing getMainCSSSourceMap from cssDiagnostics.js alongside
processTasmCSSDiagnostics in LynxEncodePlugin.ts (use the shared
getMainCSSSourceMap where the plugin currently calls its duplicate), then remove
the duplicated local definitions Asset, normalizeCSSSourceMap, and
getMainCSSSourceMap at the bottom of the file so the plugin uses the single
shared implementation.
In `@packages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts`:
- Around line 175-211: Add a regression test that verifies
processTasmCSSDiagnostics correctly removes the `@cssId` wrapper offset when
mapping diagnostics back to original source: create a new case in
cssDiagnostics.test.ts that uses processTasmCSSDiagnostics and supplies
cssDiagnostics JSON containing a diagnostic whose "line" value includes the
wrapper offset (e.g., line = originalLine + wrapperOffset), reuse the existing
sourceMap and context '/workspace/app', and assert the returned diagnostic's
sourceLine/sourceColumn correspond to the original CSS location (originalLine
and originalColumn) and not the wrapped line; reference
processTasmCSSDiagnostics, mainCSSSourceMap, cssDiagnostics, and emittedWarnings
to implement the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94900325-fb2a-4026-9035-2889daeaf289
📒 Files selected for processing (8)
.changeset/fix-css-id-source-map-offset.mdpackages/webpack/css-extract-webpack-plugin/src/loader.tspackages/webpack/css-extract-webpack-plugin/test/loader.test.tspackages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.mdpackages/webpack/template-webpack-plugin/src/LynxEncodePlugin.tspackages/webpack/template-webpack-plugin/src/cssDiagnostics.tspackages/webpack/template-webpack-plugin/src/index.tspackages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts
There was a problem hiding this comment.
Pull request overview
Fixes incorrect CSS source map line mapping when extracted CSS is wrapped with @cssId metadata, and refactors TASM CSS diagnostics handling into a single helper for parsing + resolving + deduping.
Changes:
- Offset extracted CSS source map
mappingswhen wrapping emitted CSS with an@cssIdheader. - Introduce
processTasmCSSDiagnosticshelper and add tests for end-to-end diagnostic processing/deduping. - Export the new diagnostics helper/types from
@lynx-js/template-webpack-pluginentrypoint and update API docs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts | Adds coverage for processTasmCSSDiagnostics from raw input to deduped, source-mapped output. |
| packages/webpack/template-webpack-plugin/src/index.ts | Re-exports processTasmCSSDiagnostics and related types from the package entrypoint. |
| packages/webpack/template-webpack-plugin/src/cssDiagnostics.ts | Adds processTasmCSSDiagnostics helper and a getMainCSSSourceMap utility. |
| packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts | Switches encode-time diagnostics pipeline to use processTasmCSSDiagnostics. |
| packages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.md | Updates extracted public API to include the new diagnostics helper/types. |
| packages/webpack/css-extract-webpack-plugin/test/loader.test.ts | Adds unit tests for sourcemap line-offset behavior. |
| packages/webpack/css-extract-webpack-plugin/src/loader.ts | Implements offsetSourceMapLines and applies it when wrapping with @cssId. |
| .changeset/fix-css-id-source-map-offset.md | Adds a patch changeset for @lynx-js/css-extract-webpack-plugin. |
Comments suppressed due to low confidence (1)
packages/webpack/css-extract-webpack-plugin/src/loader.ts:235
- The
@cssIdwrapper template literal includes indentation whitespace before${content}and before the closing}(and a trailing newline). That changes the emitted CSS (adds a leading space to the first content line) and can skew source map column expectations. Consider building the wrapper string with explicit\nseparators (no indentation) or otherwise ensuring no incidental whitespace is introduced.
? `@cssId "${cssId}" "${filePath}" {
${content}
}
`
: content,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this PR will degrade performance by 12.96%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | transform 1000 view elements |
39.9 ms | 45.9 ms | -12.96% |
Comparing fix/css-diagnostics-source-map (17bb077) with main (30f0277)
Footnotes
-
26 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
luhc228
force-pushed
the
fix/css-diagnostics-source-map
branch
from
8bc0d64 to
5ebaf8d
Compare
React External#673 Bundle Size — 679.93KiB (0%).17bb077(current) vs 30f0277 main#664(baseline) Bundle metrics
|
| Current #673 |
Baseline #664 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch fix/css-diagnostics-source-map Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7555 Bundle Size — 225.23KiB (0%).17bb077(current) vs 30f0277 main#7546(baseline) Bundle metrics
|
| Current #7555 |
Baseline #7546 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
69 |
69 |
| Duplicate Code |
44.57% |
44.57% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7555 |
Baseline #7546 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
79.47KiB |
79.47KiB |
Bundle analysis report Branch fix/css-diagnostics-source-map Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#687 Bundle Size — 196.39KiB (0%).17bb077(current) vs 30f0277 main#678(baseline) Bundle metrics
|
| Current #687 |
Baseline #678 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
66 |
66 |
| Duplicate Code |
44.07% |
44.07% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #687 |
Baseline #678 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
85.15KiB |
85.15KiB |
Bundle analysis report Branch fix/css-diagnostics-source-map Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9127 Bundle Size — 900.11KiB (0%).17bb077(current) vs 30f0277 main#9118(baseline) Bundle metrics
|
| Current #9127 |
Baseline #9118 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
 Cache Invalidation |
0% |
11.72% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229(+0.44%) |
228 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.29% |
27.29% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9127 |
Baseline #9118 |
|
|---|---|---|
| JS |
495.97KiB |
495.97KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch fix/css-diagnostics-source-map Project dashboard
Generated by RelativeCI Documentation Report issue
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/webpack/css-extract-webpack-plugin/src/loader.ts (1)
309-321:offsetSourceMapLinessilently no-ops on indexed source maps.The function only shifts when
sourceMap.mappingsis truthy. For indexed source maps (which use a top-levelsectionsarray instead ofmappings), the map is returned unchanged and the offset is not applied — any diagnostics against an indexed map would still point to the wrong lines. In practice, css-loader emits non-indexedRawSourceMapobjects, so this is unlikely to be hit today, but a short comment (or an explicit guard) would make the assumption clear for future callers.🧹 Suggested hardening
export function offsetSourceMapLines<T extends DependencySourceMap>( sourceMap: T, lineOffset: number, ): T { - if (lineOffset <= 0 || !sourceMap.mappings) { + // Note: indexed source maps (with `sections`) are not supported here; + // css-loader emits standard RawSourceMap objects with a `mappings` string. + if (lineOffset <= 0 || typeof sourceMap.mappings !== 'string' || sourceMap.mappings.length === 0) { return sourceMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/css-extract-webpack-plugin/src/loader.ts` around lines 309 - 321, The function offsetSourceMapLines currently only adjusts source maps that have a top-level mappings string and silently no-ops for indexed source maps (those with a top-level sections array); update offsetSourceMapLines to explicitly detect indexed maps (check for a sections property on the incoming DependencySourceMap) and either apply an appropriate handling strategy (e.g., throw a clear error/warning indicating indexed maps are unsupported) or document the assumption with a concise comment near offsetSourceMapLines and add a guard that returns early while making the unsupported case explicit; reference the function name offsetSourceMapLines and the DependencySourceMap type when adding the guard/comment so future maintainers see the intended behavior for indexed maps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/webpack/css-extract-webpack-plugin/src/loader.ts`:
- Around line 309-321: The function offsetSourceMapLines currently only adjusts
source maps that have a top-level mappings string and silently no-ops for
indexed source maps (those with a top-level sections array); update
offsetSourceMapLines to explicitly detect indexed maps (check for a sections
property on the incoming DependencySourceMap) and either apply an appropriate
handling strategy (e.g., throw a clear error/warning indicating indexed maps are
unsupported) or document the assumption with a concise comment near
offsetSourceMapLines and add a guard that returns early while making the
unsupported case explicit; reference the function name offsetSourceMapLines and
the DependencySourceMap type when adding the guard/comment so future maintainers
see the intended behavior for indexed maps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ecbe8c4-e402-4fff-84cb-d87bbc7e1b81
📒 Files selected for processing (8)
.changeset/fix-css-id-source-map-offset.mdpackages/webpack/css-extract-webpack-plugin/src/loader.tspackages/webpack/css-extract-webpack-plugin/test/loader.test.tspackages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.mdpackages/webpack/template-webpack-plugin/src/LynxEncodePlugin.tspackages/webpack/template-webpack-plugin/src/cssDiagnostics.tspackages/webpack/template-webpack-plugin/src/index.tspackages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts
✅ Files skipped from review due to trivial changes (3)
- .changeset/fix-css-id-source-map-offset.md
- packages/webpack/css-extract-webpack-plugin/test/loader.test.ts
- packages/webpack/template-webpack-plugin/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts
- packages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.md
- packages/webpack/template-webpack-plugin/src/cssDiagnostics.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/webpack/css-extract-webpack-plugin/src/loader.ts:235
- The
@cssIdwrapper template literal includes indentation whitespace before${content}and before the closing}. That alters the emitted CSS and can shift generated column positions, whileoffsetSourceMapLinesonly adjusts line mappings. Build the wrapper string without incidental indentation (or explicitly control it) so the wrapped output and source-map offsets stay accurate.
? `@cssId "${cssId}" "${filePath}" {
${content}
}
`
: content,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
luhc228
force-pushed
the
fix/css-diagnostics-source-map
branch
from
5ebaf8d to
17bb077
Compare
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)
packages/webpack/css-extract-webpack-plugin/src/loader.ts (1)
178-188:⚠️ Potential issue | 🟠 MajorUse the effective dependency
cssIdfor wrapping and source-map offsetting.Line 178 preserves an existing
cssIdon the dependency, but Lines 186 and 231 still use the parentcssId. That can emit an identifier with one CSS id while wrapping with another, or skip wrapping/offsetting when only the dependency hascssId.🐛 Proposed fix
if (params.get('cssId') === null) { params.set('cssId', cssId); } + const dependencyCssId = params.get('cssId') ?? ''; const filePath = path.relative( this.rootContext, extractPathFromIdentifier(identifier)!, ); - const shouldWrapCSSId = Boolean(cssId) + const shouldWrapCSSId = Boolean(dependencyCssId) && (params.get('common') === null || params.get('common') === 'false');- ? `@cssId "${cssId}" "${filePath}" { + ? `@cssId "${dependencyCssId}" "${filePath}" { ${content} } `Also applies to: 231-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webpack/css-extract-webpack-plugin/src/loader.ts` around lines 178 - 188, Compute and use the effective dependency cssId (prefer the existing params.get('cssId') if present, otherwise fall back to the parent cssId) instead of always using the parent cssId; i.e., derive a variable like effectiveCssId = params.get('cssId') ?? cssId and use effectiveCssId wherever the code currently uses cssId for decisions and transformations (specifically for shouldWrapCSSId logic and the source-map offsetting/wrapping code referenced around the later block that currently uses cssId), ensuring you update uses in the shouldWrapCSSId calculation and the source-map offset/wrap section (the code using params, cssId, extractPathFromIdentifier and the wrapping/offset logic) to reference effectiveCssId.
🤖 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 `@packages/webpack/css-extract-webpack-plugin/src/loader.ts`:
- Around line 178-188: Compute and use the effective dependency cssId (prefer
the existing params.get('cssId') if present, otherwise fall back to the parent
cssId) instead of always using the parent cssId; i.e., derive a variable like
effectiveCssId = params.get('cssId') ?? cssId and use effectiveCssId wherever
the code currently uses cssId for decisions and transformations (specifically
for shouldWrapCSSId logic and the source-map offsetting/wrapping code referenced
around the later block that currently uses cssId), ensuring you update uses in
the shouldWrapCSSId calculation and the source-map offset/wrap section (the code
using params, cssId, extractPathFromIdentifier and the wrapping/offset logic) to
reference effectiveCssId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7008649-c377-4f3a-aad1-7c2f2e7626a5
📒 Files selected for processing (9)
.changeset/fix-css-id-source-map-offset.mdpackages/webpack/css-extract-webpack-plugin/src/loader.tspackages/webpack/css-extract-webpack-plugin/test/loader.test.tspackages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.mdpackages/webpack/template-webpack-plugin/src/LynxEncodePlugin.tspackages/webpack/template-webpack-plugin/src/cssDiagnostics.tspackages/webpack/template-webpack-plugin/src/index.tspackages/webpack/template-webpack-plugin/test/cases/code-splitting/initial-css-order/rspack.config.jspackages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts
✅ Files skipped from review due to trivial changes (5)
- packages/webpack/template-webpack-plugin/test/cases/code-splitting/initial-css-order/rspack.config.js
- .changeset/fix-css-id-source-map-offset.md
- packages/webpack/template-webpack-plugin/src/index.ts
- packages/webpack/css-extract-webpack-plugin/test/loader.test.ts
- packages/webpack/template-webpack-plugin/src/cssDiagnostics.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/webpack/template-webpack-plugin/test/cssDiagnostics.test.ts
- packages/webpack/template-webpack-plugin/etc/template-webpack-plugin.api.md
- packages/webpack/template-webpack-plugin/src/LynxEncodePlugin.ts
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
Checklist