Skip to content

Finish the lint baseline and widen the CI check to the whole repo - #119

Merged
juanmaguitar merged 1 commit into
trunkfrom
juanmaguitar/issue-117-finish-lint-baseline
Aug 5, 2026
Merged

Finish the lint baseline and widen the CI check to the whole repo#119
juanmaguitar merged 1 commit into
trunkfrom
juanmaguitar/issue-117-finish-lint-baseline

Conversation

@juanmaguitar

@juanmaguitar juanmaguitar commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Closes #117.

CI linted only the files a PR touched. That was the right call when the linter landed on top of a backlog, but it meant new code was held to the standard while everything else drifted, and the check could not be made required.

The console findings are not debt

17 of the 27 remaining findings were no-console in the runner modules and the build scripts. Those runners are spawned as child processes (process.execPath + ELECTRON_RUN_AS_NODE=1) and main.js reads their stdout/stderr back to stream it to the renderer — their console output is the transport. Deleting a call would delete the message.

So this is a lint-config gap, fixed with one config block scoped to those four runners plus scripts/**/*.cjs. The rule stays on in main.js, preload.js and the renderer, where a stray console statement really is a leftover. Verified both directions: a temporary console.log in main.js still fails lint while server-runner.js passes.

The genuine debt

  • @param/@return tags on kill-tree.js (7), dev-server-command.cjs (1), path-basename.cjs (1).
  • One loose equality in npm-runner.js. It becomes a truthiness check rather than !== null, because !== null would change behaviour for an undefined signal (early return instead of falling through). No caller passes undefined today — the only production call site is a close handler — but truthiness keeps the fix behaviourally identical to what was there.

The workflow

Now npx eslint . --max-warnings=0, and it runs on push to trunk as well as on pull requests so the default branch carries a status of its own — a protection rule has nothing to require otherwise.

The changed-files job is dropped rather than kept behind a flag; it stays in git log -- .github/workflows/lint.yml, which the header comment points at. It is the right shape again if a future rule ever lands with a backlog behind it.

Testing

  • npm run lint exits 0 with no output.
  • npm test — 147/147.
  • The narrowness check described above.

Follow-ups, not in this PR

🤖 Generated with Claude Code

@juanmaguitar @claude
Finish the lint baseline and widen the CI check to the whole repo
d895893 
CI linted only the files a PR touched, because the linter arrived on top of a
backlog. New code was held to the standard while everything else drifted, and
the check could not be made required.

What was left split in two. The bulk — 17 "unexpected console statement"
findings — was never debt: the runner modules are spawned as child processes
and main.js reads their stdout back to stream it to the renderer, so their
console output is the transport. Removing those calls would break the app.
That is a gap in the lint config, scoped here to just those files and the
scripts/ entry points, so the rule keeps catching a stray console statement in
main.js, preload.js and the renderer.

The genuine findings were ten: missing @PARAM tags, mostly in kill-tree.js,
and one loose equality. The equality becomes a truthiness check rather than
!== null so behaviour is unchanged for an undefined signal.

The workflow now runs `eslint .` and also runs on push to trunk, so the branch
has a status a protection rule can require. The changed-files job is left in
git history rather than behind a flag — it is the right shape again if a future
rule ever lands with a backlog behind it.

Closes #117

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown

0 findings across the five dimensions (architecture, security, performance, cross-platform, tests).

This PR finishes the lint baseline (#117) and widens CI: `eslint.config.mjs` gets a scoped `no-console: off` for the four stdout-is-the-interface runner files (`src/install-runner.js`, `src/script-runner.js`, `src/server-runner.js`, `src/playground-web-runner.js`) plus `scripts/**/*.cjs` - verified all five glob targets exist and none overlap `main.js`/`preload.js`/renderer, so the rule still catches stray `console` calls there. `.github/workflows/lint.yml` drops the changed-files scoping for `eslint . --max-warnings=0` on every PR and on push to `trunk`; it stays read-only, uses `--ignore-scripts`, and never executes PR code, so there's no secrets exposure.

The one behavioral-looking diff, `src/npm-runner.js`'s `shouldRetryWithRelaxedEngines` (`signal != null` -> `signal`), is a no-op for real inputs - Node's `close`/`exit` signal argument is always either `null` or a non-empty signal-name string (e.g. `'SIGTERM'`), so both checks agree in every reachable case. Already exercised by `test/npm-runner.test.cjs` (null-signal and `'SIGTERM'`/`'SIGKILL'` cases both present), so no new test needed.

Everything else in the diff (`src/kill-tree.js`, `src/renderer/dev-server-command.cjs`, `src/renderer/path-basename.cjs`) is JSDoc-only - out of scope per the review rules (ESLint's job).

No reconciliation needed - first review run on this PR.

@juanmaguitar
juanmaguitar merged commit a3f46c4 into trunk Aug 5, 2026
4 checks passed
@juanmaguitar
juanmaguitar deleted the juanmaguitar/issue-117-finish-lint-baseline branch August 5, 2026 16:35
This was referenced Aug 5, 2026
Merged
Merged
Merged
juanmaguitar added a commit that referenced this pull request Aug 7, 2026
@juanmaguitar @claude
Let a site know which Trac ticket it is being used for (#123)
c81c9c5 
Closes #109. First step of #110, following [Option
A](#110 (comment)).

## Why

The app knows *where* a site is and *what state its build is in*, but
not *what the contributor is working on* — and Core work is organised
around Trac tickets, not directories. That gap is why a generated patch
carries no ticket number (#107), why there is nowhere to show a ticket's
existing patches, and why applying someone else's patch (#11) has no
context to start from.

## What this does

A site can now be associated with a Trac ticket:

- asked optionally at creation — *"What are you working on?"*
- set, changed or cleared later from a new panel on the site screen
- stored in `siteMeta`, so it survives restarts

Parsing accepts what a contributor actually types or pastes: a bare
number, a number with a `#`, and any core Trac ticket URL with a comment
anchor, trailing slash or `?format=` query still attached. Anything else
is rejected with a message aimed at the contributor rather than at a
log.

The panel sits between the action row and the Terminal; nothing else on
the page moves.

## What this deliberately does not do

**No network.** Linking a ticket never depends on Trac being reachable.

That is a design decision, not an omission. While building this I
measured every documented Trac read path — `?format=csv`, the ticket
HTML, `raw-attachment/ticket/<id>/<file>`, and even `robots.txt` — and
all of them currently return **403 behind a proof-of-work interstitial**
for any client that is not a browser (SHA-256 hashcash, `_hcc` cookie,
escalating to an "I am human" checkbox on repeat hits; a spoofed browser
User-Agent gets a bare nginx 403 instead). This also breaks core's own
`grunt patch`, which has parsed that page for years.

So *showing* a ticket's patches needs a fetch strategy of its own and
lands separately. Two things shape it:

- A Contributor Day room shares one NAT IP — exactly the pattern that
escalates Trac's challenge to interactive, and that exhausts
unauthenticated GitHub's 60 requests/hour. Any network path has to
degrade to opening the contributor's own browser, where the challenge
clears itself.
- The real fix is upstream: [meta
#8202](https://meta.trac.wordpress.org/ticket/8202) proposes a `ticket/`
JSON endpoint returning exactly the attachment data (filename, author,
date, size) that panel wants. The follow-up should keep its fetch source
swappable so it can adopt that endpoint when it lands.

Also out of scope here: applying patches (#11), the ticket number in the
patch header (#107), tickets as branches (#108), opening PRs from the
app (#118).

## Testing

- `test/trac-ticket.test.cjs` — 14 tests over every accepted and
rejected input form. `parseTicketRef` is a pure, dependency-free module
so it runs under `node --test` with no DOM, following the
`setup-steps.cjs` / `update-plan.cjs` convention.
- `npm test` and `npm run test:electron`: **172/172 pass** on both.
- `npm run lint` (now repo-wide after #119): clean.

Manual: create a site with and without a ticket; link, unlink and
re-link from the panel; paste `62281`, `#62281` and a URL with
`#comment:3`; restart the app and confirm the ticket survives; confirm
"Open in Trac" opens the external browser.

### Fixed after manual testing

Manual testing surfaced a wrong-message bug: a bare word like `abc` (and
`62281abc`, `#abc`) was rejected with *"Only core.trac.wordpress.org
tickets are supported"* — pointing the contributor at a host they never
named. `new URL('https://abc')` succeeds because a bare word is a legal
hostname, so non-URL input slipped past into the host check instead of
falling to the generic message. Fixed to only treat input as a URL when
it has a scheme, a path separator or a dotted host; everything else now
gets *"Enter a ticket number like 62281, or a core.trac.wordpress.org
ticket URL."* A github.com URL still correctly names the host as the
reason. The reproducing test asserts the message, not just rejection —
the gap the original suite missed.

Rebased onto trunk after #122, so no generated bundle is included — the
diff is five source files.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Finish the lint baseline so the whole repo can be linted in CI

1 participant

@juanmaguitar