Skip to content

chore(docs): turn on no-unused-vars, and pin the analytics split - #416

Merged
jfrench9 merged 1 commit into
mainfrom
chore/docs-housekeeping
Sep 20, 2026
Merged

jfrench9 merged 1 commit into
mainfrom
chore/docs-housekeeping

Conversation

@jfrench9

Copy link
Copy Markdown
Member

Two findings from the post-merge review, both really about a guard that wasn't there.

The dead variable was the symptom

A const domain in the GraphQL field page was assigned and never read — the neighbour nav computes its own. Lint couldn't catch it because @typescript-eslint/no-unused-vars was off repo-wide, which is the actual finding.

The rule is on now. It found 24 violations across 14 files, all fixed:

  • genuinely dead locals and two dead helpers in usage/content.tsx — deleted
  • arguments whose position callers rely on — marked _
  • catch (error) with an unused binding → optional catch binding
  • GET(request: Request) in a route handler that ignores it → GET()

Two destructures exist precisely to keep a prop off the DOM element, so they're renamed rather than removed. Progress in the flowbite mocks reads color via data-color; only size was unused there — worth saying because my first pass renamed both and broke it.

The analytics split is load-bearing copy

The extensions hub decided its three sections with tag.name.includes('Analytical Views'), inline in the page component and untested.

The failure mode isn't a missing section. A tag renamed away from that string doesn't disappear — it lands in Writes, beside copy promising every operation "takes a typed request, returns an operation envelope" and accepts an Idempotency-Key. Said of read-only queries against the materialized graph, that's wrong copy about what a call does.

It's now partitionExtensionTags in lib/openapi.ts with four tests, including that an unrecognised tag still lands in writes rather than being dropped, and that analytics are recognised for any domain rather than RoboLedger only. Renaming the match fails three of them — mutation-verified.

Verification

npm run test:all: 580 passed (70 files), format, lint, typecheck clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01C3sS2qq4GbycMSSKr6vsf3

Two review findings, both about a guard that was not there.

**The dead variable was the symptom.** A `const domain` in the GraphQL field
page was assigned and never read — the neighbour nav computes its own. Lint
could not catch it because `@typescript-eslint/no-unused-vars` is off across
the repo, so the rule is on now and the 24 violations it found are fixed.
Most were genuinely dead; the rest are arguments whose position callers rely
on, marked `_`. Two of those destructures exist precisely to keep a prop off
the DOM element, so they are renamed rather than removed — `Progress` in the
flowbite mocks reads `color` and only `size` was unused.

**The analytics split is load-bearing copy.** The extensions hub decided its
three sections with `tag.name.includes('Analytical Views')` inline in the page,
untested. The failure mode is not a missing section: a tag renamed away from
that string lands in *Writes*, beside copy promising each operation "takes a
typed request, returns an operation envelope" and accepts an Idempotency-Key —
said of read-only queries against the materialized graph. It is now
`partitionExtensionTags` in `lib/openapi.ts` with four tests, including that an
unrecognised tag still lands somewhere rather than being dropped. Renaming the
match fails three of them.

`npm run test:all`: 580 passed, lint, format, typecheck clean.

Claude-Session: https://claude.ai/code/session_01C3sS2qq4GbycMSSKr6vsf3
@github-actions github-actions Bot added the change:normal Normal change: automated review documented on the pull request before merge label Sep 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

@jfrench9

Copy link
Copy Markdown
Member Author

@claude please review this PR

@claude

claude Bot commented Sep 20, 2026

Copy link
Copy Markdown

Claude finished @jfrench9's task in 1m 18s —— View job


Review complete

  • Gather context (PR description, diff, changed files)
  • Review eslint.config.js no-unused-vars change
  • Review the 14 files with lint-driven fixes
  • Review lib/openapi.ts partitionExtensionTags + tests
  • Review extensions/page.tsx usage of the new partition helper
  • Post final review feedback

Went through the diff against origin/main file by file. The description is accurate — I traced each "genuinely dead" claim back to the code rather than taking it at face value.

partitionExtensionTags (src/lib/openapi.ts:482-506) — solid extraction. The old inline ANALYTICS/GRAPHQL predicates in extensions/page.tsx are gone and replaced with the tested helper; extensions/page.tsx:65-69 now destructures { graphql, writes, analytics } from it. The four new tests in openapi.test.ts genuinely cover the stated failure mode (unrecognised tag → writes, not dropped; analytics recognised across domains, not just RoboLedger). Renaming the match would indeed fail the "any domain" and default-fallback tests, so this isn't decorative coverage.

Lint fixes — spot-checked the riskiest-looking ones (removed props/destructures, not just renamed args):

  • EntityInfoStep.tsx: removed entityDetails prop and handleDetailChange — confirmed neither is referenced anywhere else in the component, so this was genuinely dead.
  • checkout/[session_id]/content.tsx and dashboard/content.tsx: setCurrentGraph / currentGraph removed from destructures — confirmed no remaining references.
  • useRestoreMonitoring.ts: backupId/graphId_backupId/_graphId in startProgressPolling — correct, the only usages are inside commented-out code waiting on an SDK export, so they're honestly unused right now rather than a hidden bug.
  • flowbite-mocks.tsx Progress mock: confirmed color is still read (via data-color={color}), only size was renamed to _size — matches what the description called out as the near-miss from the first pass.

One loose end (non-blocking): in billing-panels.tsx, router was dropped from OverviewTab's destructured params and offerings from SubscriptionsTab's, but both are still listed as required in the inline prop type annotations (router: any at line ~214, offerings: any at line ~435) and both are still passed by the caller in content.tsx:907-933. Not a bug — TS allows partial destructuring and the caller still supplies them — but since this PR's whole point is scrubbing unused bindings, it'd be more complete to drop router/offerings from the type objects (and the caller's props) too, rather than leaving now-pointless required props in the public signature.

Nothing else stood out — no behavior changes, no silently-broken JSX, no places where the "unused" var was actually a prop DOM elements needed. Config change (no-unused-vars on, with ^_ ignore patterns for args/vars/caught errors) is a sensible, low-risk repo-wide default.

I wasn't able to run npm run test:all/typecheck/lint myself in this session (Bash execution requires interactive approval I don't have here), so I relied on manual tracing of each removed/renamed identifier through its file instead of tool output. The PR description's own report (580 passed, format/lint/typecheck clean) is consistent with what I found by inspection.

Verdict: looks good to merge as-is; the billing-panels.tsx prop-type cleanup is an optional nit, not a blocker.

@jfrench9
jfrench9 merged commit b301068 into main Sep 20, 2026
7 checks passed
@jfrench9
jfrench9 deleted the chore/docs-housekeeping branch September 20, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:normal Normal change: automated review documented on the pull request before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant