Skip to content

Harden ESLint rules for react-web-cli package#252

Open
AndyTWF wants to merge 15 commits intomainfrom
eslint-harden-react-web-cli
Open

Harden ESLint rules for react-web-cli package#252
AndyTWF wants to merge 15 commits intomainfrom
eslint-harden-react-web-cli

Conversation

@AndyTWF
Copy link
Copy Markdown
Contributor

@AndyTWF AndyTWF commented Apr 1, 2026

Summary

Incrementally enables strict TypeScript and code quality ESLint rules for packages/react-web-cli, closing gaps that allowed dead code, type safety holes, and stale closures to slip through undetected.

  • Enable no-unused-vars — removed 30 unused imports, variables, and state declarations
  • Enable unicorn rulesprefer-string-slice, prefer-code-point, no-negated-condition (fixes surrogate pair bugs, improves readability)
  • Install eslint-plugin-react-hooks — catches stale closures and hook ordering bugs in the 3,600-line main component
  • Enable no-console — migrated ~25 console.log calls to gated debugLog() helper
  • Enable no-explicit-any — created types.ts with proper discriminated unions (ControlMessage, AblyCliGlobals, WebSocketMessageData) replacing ~24 any annotations
  • Upgrade to recommended-type-checked — full type-aware linting with no-unsafe-* family, no-misused-promises, no-floating-promises
  • Promote all rules to error — source files enforced strictly, test files exempt for mocking flexibility

Final state: 0 errors, 22 warnings (16 exhaustive-deps + 6 no-disabled-tests — both intentionally left as warnings).

🤖 Generated with Claude Code

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cli-web-cli Ready Ready Preview, Comment Apr 1, 2026 1:01pm

Request Review

AndyTWF and others added 10 commits April 1, 2026 13:22
Remove dead code: unused imports (safeFit, CONTROL_MESSAGE_PREFIX,
isHijackMetaChunk, colour), unused state (sessionIdInitialized,
secondarySocket value, secondaryConnectionStatus value,
secondarySpinnerPrefixReference, secondaryReconnectScheduledThisCycleReference),
unused locals (colour object, wasReconnecting, wasConnecting, latestStatus,
TERMINAL_PROMPT_IDENTIFIER), and unused variables in terminal-shared.ts
(bufferLength, baseY, viewportY, cursorX).

Also fixes a lonely-if lint error exposed by the cleanup.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the "off" override and replace two substring() calls with slice()
in the control message parsing logic.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace all charCodeAt() calls with codePointAt() for proper
surrogate pair handling. Fixes potential bugs with emoji/wide characters
in TerminalOverlay.tsx and AblyCliTerminal.tsx.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Flip negated if/else conditions to positive-first style in
AblyCliTerminal.tsx (handlePtyData, handleWebSocketOpen) and
use-terminal-visibility.ts (recompute ternaries).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add rules-of-hooks (error) to catch hook ordering bugs, and
exhaustive-deps (warn) to flag stale closures in the large
AblyCliTerminal component. This is the highest-value lint addition
for catching React-specific bugs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate ~25 console.log calls to debugLog() so they only fire when
ABLY_CLI_DEBUG is enabled. console.warn and console.error remain
allowed. Update two tests that asserted on console.log to enable
debug mode and match the "[AblyCLITerminal DEBUG]" prefix.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create types.ts with ControlMessage union, AblyCliGlobals interface,
TerminalWithConnectingState, and WebSocketMessageData type to replace
all ~24 explicit 'any' annotations in source files with proper types.

Key changes:
- Control message handler now uses discriminated union type
- globalThis casts use AblyCliGlobals interface instead of 'any'
- Terminal extension properties use TerminalWithConnectingState
- messageDataToUint8Array uses WebSocketMessageData type
- debounce utility uses unknown[] instead of any[]
- terminal-box stub uses unknown instead of any

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e-* as warn

Switch from tsPlugin.configs.recommended to recommended-type-checked for full
type-aware linting. Fix errors from additional rules (no-unnecessary-type-assertion,
no-misused-promises, no-floating-promises) and remove dead code checking for
stream/hijack properties on typed ControlMessage. Enable no-unsafe-* family as
warnings. Disable require-await and unbound-method in test config.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add type assertions for JSON.parse calls (signedConfig parsing, control
message parsing) and cast MessageEvent.data to WebSocketMessageData.
Remove dead code checking for stream/hijack on typed ControlMessage.
Source files now have zero no-unsafe-* warnings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All no-unsafe-*, no-explicit-any, no-base-to-string, and no-console rules
now enforced as errors in source files. Test files disable these rules since
tests legitimately use `any` for mocking. Only react-hooks/exhaustive-deps
remains as warn (non-trivial to fix without risking behavior changes).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AndyTWF and others added 5 commits April 1, 2026 13:40
…web-cli

Cherry-pick no-deprecated (warn), no-useless-constructor, unified-signatures,
return-await, and consistent-type-imports (all error) from the main CLI config.
Auto-fix converts 5 value imports to type imports across AblyCliTerminal.tsx
and use-terminal-visibility.ts.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- handleCloseSplit: suppress — deps are stable but defined later (hoisting)
- handleCloseSecondary: suppress — same hoisting constraint
- handlePtyData: remove unused deps (clearInstallInstructionsTimer,
  updateConnectionStatusAndExpose, updateSessionActive)
- handleControlMessage: add activateSessionAndSendCommand and
  clearPromptDetectionTimeout; remove unused clearPtyBuffer, updateSessionActive
- activateSecondarySessionAndSendCommand: suppress hoisting constraint
- startConnectingAnimation: move spinnerFrames to module scope
- handleWebSocketOpen: remove unused deps (clearAnimationMessages,
  credentialHash, resumeOnReload, updateConnectionStatusAndExpose)
- termCleanupReference: capture .current in local var before cleanup
- useEffect (initial connection): add resumeOnReload
- startInactivityTimer: add clearInactivityTimer, remove module-scope deps

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- handleClosePrimary: suppress — deps defined later in component (hoisting)
- connectWebSocket: use credentialsInitializedRef for debug-only state read;
  remove unnecessary ref dep (term)
- handleWebSocketMessage: use handleControlMessageRef to avoid deep dep chain
  through sessionId state; remove all stale deps from array
- connectSecondaryWebSocket: suppress — already accessed via ref; missing deps
  include hoisted callbacks and state causing cascading recreations

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two useEffects intentionally use minimal dep arrays to avoid destroying
and recreating xterm Terminal instances on state changes:
- Primary terminal init (empty []) — 12 missing deps
- Secondary terminal init ([isSplit]) — 4 missing deps

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All 16 warnings have been resolved — either by fixing deps, using ref
patterns, or suppressing with documented rationale. Promote to error to
prevent regressions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@AndyTWF AndyTWF marked this pull request as ready for review April 1, 2026 14:46
@AndyTWF AndyTWF requested a review from umair-ably April 1, 2026 14:46
@claude-code-ably-assistant
Copy link
Copy Markdown

Walkthrough

This PR incrementally hardens ESLint and TypeScript rules for the packages/react-web-cli package, eliminating dead code, type safety holes, and potential stale closure bugs that were previously undetected. It progresses from enabling no-unused-vars through unicorn rules, eslint-plugin-react-hooks, no-console, no-explicit-any, and finally full recommended-type-checked + strict enforcement — arriving at 0 errors with only 16 exhaustive-deps (suppressed with documented rationale) and 6 no-disabled-tests remaining as warnings.

Changes

Area Files Summary
Config eslint.config.js Adds react-hooks plugin, upgrades to recommended-type-checked, promotes all new rules to error in source, exempts test files from no-unsafe-* / no-explicit-any
Config package.json, pnpm-lock.yaml Adds eslint-plugin-react-hooks dev dependency
Components packages/react-web-cli/src/AblyCliTerminal.tsx Removes 30+ dead variables/state, migrates console.logdebugLog(), fixes exhaustive-deps via ref patterns and documented suppressions, converts value imports to type imports
Components packages/react-web-cli/src/TerminalOverlay.tsx Replaces charCodeAt() with codePointAt() for correct surrogate pair handling
Services packages/react-web-cli/src/global-reconnect.ts Type safety fixes; removes any annotations
Utils packages/react-web-cli/src/terminal-shared.ts, terminal-box.ts Removes unused variables; replaces any with unknown
Utils packages/react-web-cli/src/use-terminal-visibility.ts Flips negated conditions to positive-first; converts value imports to type imports
New File packages/react-web-cli/src/types.ts Introduces ControlMessage discriminated union, AblyCliGlobals interface, TerminalWithConnectingState, and WebSocketMessageData type — replaces ~24 any annotations
Tests packages/react-web-cli/src/AblyCliTerminal.test.tsx, AblyCliTerminal.resize.test.tsx Updates assertions to match debugLog() output format (prefix + ABLY_CLI_DEBUG env var)

Review Notes

  • Behavioral change — debugLog() gating: ~25 debug console.log calls are now only emitted when ABLY_CLI_DEBUG is set. Any developer relying on those logs without that env var will no longer see them.
  • Surrogate pair fix: charCodeAt()codePointAt() in TerminalOverlay.tsx and AblyCliTerminal.tsx is a real bug fix for emoji/wide characters — reviewers should check the context is correct (no off-by-one from the index difference).
  • exhaustive-deps suppressions: 6 eslint-disable-next-line comments suppress the rule for mount-only useEffect blocks (terminal init) and hoisted callback refs. Each has a documented rationale in the commit messages — reviewers should verify the suppression comments in source are similarly clear.
  • New types.ts: The ControlMessage discriminated union now removes dead branches that checked for stream/hijack properties — confirm those message types are truly retired upstream.
  • No runtime logic changes: All changes are dead-code removal, type annotations, or code style fixes. The only functional change is debugLog() gating and the codePointAt() fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant