tasks: switch dashboard-pie-chart spec to inline eslint-disable-line for charts CSS import by dognose24 · Pull Request #36 · dognose24/jetpack

dognose24 · 2026-05-15T14:57:24Z

Fixes RSM-3355

Proposed changes

Switch the dashboard-pie-chart task spec's ESLint disable-comment form for the @automattic/charts/style.css import from eslint-disable-next-line (comment on the line above) to inline eslint-disable-line (comment at end of import line). Keep the CSS import itself — it's load-bearing.

Imports section JSX updated to:

import '@automattic/charts/style.css'; // eslint-disable-line import/no-unresolved -- ...

"Why the CSS import is required" section's reference to the disable comment updated to match.
New "Why the disable comment must be inline" section documenting the failure mode of the next-line form so future contributors don't revert it.
Changelog: Comment-only patch entry, internal tooling.

Evidence the CSS import is needed (vs my earlier suspicion that it was redundant)

Built the package twice on host and grepped routes/dashboard/content.min.js:

Build	grep `svg ?\{.display.block`
Without `import '@automattic/charts/style.css'`	0 matches
With the import	2 matches — both reference `.a8ccharts-... svg{display:block}` and one is inside a runtime `style.appendChild(document.createTextNode('...'))` injection

The CSS rule is genuinely missing without the import. The task spec's existing "Why the CSS import is required" rationale is correct and stays.

What was actually failing

Both PR #30 and PR #35 sandbox runs of /premium-analytics-implement-task hit the same recurring friction at commit time:

Agent adds // eslint-disable-next-line ... above the import per spec.
Pre-commit prettier inserts a blank line between the @automattic/charts and @wordpress/i18n import groups — separating the disable comment from the import it was annotating.
Local ESLint auto-fix sees the disable comment as targeting a rule that doesn't fire (because the charts package is built locally → dist/index.css exists) → auto-removes the comment.
Agent commits.
CI lint job runs without the charts package built → import/no-unresolved fires → comment is gone → CI fail.

Each run the agent burned 2–3 commit attempts fighting prettier + ESLint before either removing the import (wrong fix; loses the CSS rule) or switching to the inline form (right fix). Fixing the spec saves every future run that same cycle.

Why inline works where next-line doesn't

Prettier doesn't move trailing comments — they stay glued to their statement.
ESLint won't auto-remove an inline disable that targets a rule that actually fires (in CI the file is unresolved so the rule is live → the comment is "used").

Does this pull request change what data or activity we track or use?

No — task spec / docs only, no code or behavior change.

Testing instructions

Docs-only change to a task spec. Read through projects/packages/premium-analytics/tasks/dashboard-pie-chart.md and confirm:

The Imports JSX example uses inline // eslint-disable-line ... at the end of the CSS import line.
The "Why the CSS import is required" section still recommends the inline form.
The new "Why the disable comment must be inline" section explains the rationale.

…for charts CSS import The CSS import itself is required — build comparison verified (PR #30 / PR #35 dogfood): without it the `.a8ccharts-* svg{display:block}` chart-layout rule is missing from the bundle, exposing the resize-loop class of bug the rule guards against. Keep the import. But the `eslint-disable-next-line` form (comment on the line above) doesn't survive this repo's tooling: * Prettier inserts a blank line between import groups → separates the comment from the import * Local ESLint auto-fix sees the comment as unused (charts package built locally → dist/index.css exists) → removes it * CI's lint job runs without the charts package built → import is unresolved AND the disable comment is gone → fail Switch to inline `eslint-disable-line` at the end of the import line. Prettier doesn't move trailing comments and ESLint won't auto-remove a comment that actually disables an active rule (the rule fires in CI where the file is unresolved). Adds a "Why the disable comment must be inline" section so future contributors understand why the cleaner-looking next-line form was rejected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions · 2026-05-15T14:58:06Z

@copilot review

github-actions · 2026-05-15T14:58:29Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
🔴 Add a "[Status]" label (In Progress, Needs Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
You can use GitHub's Reviewers functionality to request a review.
When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

Copilot

Pull request overview

Updates the premium-analytics “dashboard-pie-chart” task spec to use an inline ESLint disable comment for the @automattic/charts/style.css import so it survives the repo’s prettier + ESLint auto-fix workflow, and documents why this is required.

Changes:

Switch the spec’s CSS import example from eslint-disable-next-line to inline eslint-disable-line.
Update and expand documentation explaining why the CSS import is required and why the disable comment must be inline.
Add an internal tooling-only changelog entry for the spec update.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
projects/packages/premium-analytics/tasks/dashboard-pie-chart.md	Updates the import example and adds rationale for using inline `eslint-disable-line`.
projects/packages/premium-analytics/changelog/update-pie-chart-task-inline-eslint-disable	Internal changelog entry documenting the spec/tooling adjustment.

dognose24 · 2026-05-18T13:01:04Z

+* Locally the charts package is usually built, so `dist/index.css` exists and ESLint's auto-fix sees the disable comment as "unused" and removes it.
+* CI's lint job runs without the charts package built, so the import is unresolved — but the disable comment was already deleted locally and committed away, so CI fails.
+
+The inline `disable-line` form has none of these failure modes: prettier doesn't move trailing comments, and ESLint won't auto-remove it because the rule it disables actually fires (in the CI environment where the file is unresolved).


Good catch — fixed in 0b867cb. The trailing paragraph now uses the full eslint-disable-line directive, matching every other reference in the spec.

Addresses Copilot review on PR #36: the trailing rationale paragraph used the shortened `disable-line`; everywhere else uses the full `eslint-disable-line`. Use the full directive name for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

dognose24 · 2026-05-18T13:24:10Z

+Use `eslint-disable-line` on the same line as the import — **not** `eslint-disable-next-line` on the line above. The next-line form looks cleaner but doesn't survive this repo's lint/format toolchain end-to-end:
+
+* Prettier (run by the pre-commit hook) inserts a blank line between import groups, which separates the next-line comment from the import it was annotating.
+* Locally the charts package is usually built, so `dist/index.css` exists and ESLint's auto-fix sees the disable comment as "unused" and removes it.
+* CI's lint job runs without the charts package built, so the import is unresolved — but the disable comment was already deleted locally and committed away, so CI fails.
+
+The inline `eslint-disable-line` form has none of these failure modes: prettier doesn't move trailing comments, and ESLint won't auto-remove it because the rule it disables actually fires (in the CI environment where the file is unresolved).


Good catch — you're right, the Prettier attribution was wrong. Confirmed locally: .prettierrc.js only loads prettier-plugin-svelte (tools/js-tools/prettier-plugins.js), no import-organizing plugin. The real driver is ESLint import/order per tools/js-tools/eslintrc/base.mjs:318-325 enforcing 'newlines-between': 'never' + alphabetic ordering — that reorder detaches the next-line disable comment, then --fix removes the detached comment as unused (locally dist/index.css exists so the rule doesn't fire).

Rewrote the failure-chain bullets in 734fcf6 to describe the actual mechanism (ESLint reorders → comment detaches → --fix removes it → CI fails) instead of misattributing the cause to Prettier. The practical takeaway (use inline eslint-disable-line) is unchanged.

…ust be inline" Addresses Copilot review on PR #36: the prior wording blamed Prettier for inserting blank lines between import groups, but the repo's Prettier config only loads `prettier-plugin-svelte` — no import-organizing plugin. The real driver is ESLint's `import/order` rule (`tools/js-tools/eslintrc/base.mjs:318-325`) which enforces `'newlines-between': 'never'` and alphabetic ordering; that reordering plus `--fix` removing the now-detached "unused" disable comment is what produces the observed failure mode. Rewrite the failure-chain bullets to describe the actual mechanism (lint reorders → comment detaches → --fix removes it → CI fails) rather than misattributing the cause to Prettier. Practical recommendation (inline `eslint-disable-line`) is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

dognose24 · 2026-05-18T13:31:07Z

 `import { __ } from '@wordpress/i18n';` line unchanged — do not re-add it. The
-`eslint-disable-next-line` comment is required — see "Why the CSS import" below.
+inline `eslint-disable-line` comment on the CSS import line is required — see
+"Why the CSS import" below for the rationale, and "Why the disable comment must


Fixed in 01a9d88 — the Imports section now references the full heading "Why the CSS import is required" verbatim, matching the actual section title.

Addresses Copilot review on PR #36: the Imports section pointed to "Why the CSS import" but the actual heading later in the doc is "Why the CSS import is required". Match the full heading text so readers find the referenced section without ambiguity and future edits don't silently drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

dognose24 · 2026-05-18T13:37:34Z

+* Once the comment is no longer immediately above the unresolved import, ESLint's `--fix` sees the comment as "unused" — locally the `@automattic/charts/dist/index.css` file exists (charts package is built), so the `import/no-unresolved` rule doesn't fire there, and the disable comment gets removed.
+* On CI the lint job runs without the charts package built. The import is unresolved, but the disable comment was already deleted locally in the previous step and committed away — so CI lint fails.
+
+The inline `eslint-disable-line` form has none of these failure modes: trailing comments don't move during import reordering, and ESLint won't auto-remove a comment whose target rule actually fires (in CI where the file is genuinely unresolved).


You're right, my second iteration still encoded an unverified mechanism. Confirmed: pnpm run lint-file is just eslint --flag v10_config_lookup_from_file with no --report-unused-disable-directives, and reportUnusedDisableDirectives doesn't appear in the ESLint configs.

Fixed in 7788e43. The failure-chain bullets now describe the observed outcome (pre-commit reorders imports → disable comment ends up missing from the committed file → CI lint fails on import/no-unresolved) and explicitly note we haven't reverse-engineered which rule/interaction drops the comment. The practical fix (inline eslint-disable-line) and the reason it works are unchanged; the spec just no longer makes claims about internal lint behavior it can't substantiate.

Good discipline catch — the spec is doc-as-source-of-truth for future agent runs, encoding a wrong mechanism would propagate.

…ionale Addresses Copilot review on PR #36: prior wording asserted ESLint --fix removes the detached disable comment as "unused", but `pnpm run lint-file --fix` doesn't set `--report-unused-disable-directives` and no `reportUnusedDisableDirectives` setting is present in the repo's ESLint configs. The exact rule interaction that drops the comment isn't pinned down by available config evidence. Rewrite the failure-chain to describe the observed outcome ("pre-commit reorders imports → disable comment ends up missing from the committed file → CI lint fails") and explicitly note we haven't reverse-engineered the mechanism. The recommendation (inline `eslint-disable-line`) and its justification are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

github-actions Bot added [Package] Premium Analytics Docs labels May 15, 2026

github-actions Bot added the [Status] Needs Author Reply label May 15, 2026

dognose24 self-assigned this May 15, 2026

dognose24 requested a review from Copilot May 15, 2026 15:03

Copilot started reviewing on behalf of dognose24 May 15, 2026 15:03 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

dognose24 requested a review from Copilot May 18, 2026 13:01

Copilot started reviewing on behalf of dognose24 May 18, 2026 13:02 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

dognose24 requested a review from Copilot May 18, 2026 13:25

Copilot started reviewing on behalf of dognose24 May 18, 2026 13:26 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

dognose24 requested a review from Copilot May 18, 2026 13:31

Copilot started reviewing on behalf of dognose24 May 18, 2026 13:32 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

dognose24 requested a review from Copilot May 18, 2026 13:46

Copilot started reviewing on behalf of dognose24 May 18, 2026 13:47 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

dognose24 merged commit 57a61dd into trunk May 18, 2026
61 of 63 checks passed

github-actions Bot removed the [Status] Needs Author Reply label May 18, 2026

dognose24 mentioned this pull request May 19, 2026

implement-task: add Step 10 invariant capture audit #40

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tasks: switch dashboard-pie-chart spec to inline eslint-disable-line for charts CSS import#36

tasks: switch dashboard-pie-chart spec to inline eslint-disable-line for charts CSS import#36
dognose24 merged 5 commits into
trunkfrom
tools/pie-chart-task-inline-eslint-disable

dognose24 commented May 15, 2026

Uh oh!

github-actions Bot commented May 15, 2026

Uh oh!

github-actions Bot commented May 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

dognose24 May 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

dognose24 May 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

dognose24 May 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

dognose24 May 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dognose24 commented May 15, 2026

Proposed changes

Evidence the CSS import is needed (vs my earlier suspicion that it was redundant)

What was actually failing

Why inline works where next-line doesn't

Does this pull request change what data or activity we track or use?

Testing instructions

Related

Uh oh!

github-actions Bot commented May 15, 2026

Uh oh!

github-actions Bot commented May 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

dognose24 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dognose24 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dognose24 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dognose24 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants