Fix ESLint and type-check failures - #76
Conversation
Resolve all 92 ESLint errors reported by the Code Quality workflow: - Convert active @ts-ignore directives to @ts-expect-error, and drop the ones that were no longer suppressing a real type error - Replace explicit `any` with proper types (GameEventData event payload, generic getPreferenceValue, narrowed catch clauses, Storage.setItem) - Replace the `Function` type in render dialog options with `() => void` - Apply prefer-const / no-var fixes (including snow.ts and asset.ts) - Attach `cause` to the re-thrown error in the file-transformer plugin - Remove an unused constant and tidy unused parameters/catch bindings - Add scoped ESLint overrides for CommonJS Jest mocks and Cypress specs (Chai assertions and `any`-typed command declarations)
|
Warning Review limit reached
More reviews will be available in 32 minutes and 55 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR systematically improves TypeScript type safety and code quality across the codebase. The core change introduces a typed ChangesType Safety and Code Quality Improvements
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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 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 |
Deploying 2048-clone with
|
| Latest commit: |
61faa4e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://05f5d0c3.2048-clone-33h.pages.dev |
| Branch Preview URL: | https://claude-lint-failures-fixes-6.2048-clone-33h.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/index.ts (1)
564-574: 💤 Low valueThe
enabledconstant declaration is redundant.The pattern
const enabled = (isAnimationEnabled = !isAnimationEnabled);works correctly but introduces an unnecessary intermediate variable. Sinceenabledis only used in the immediately followingifstatement, you can simplify by checkingisAnimationEnableddirectly.♻️ Proposed simplification
} else if (elem.classList.contains(ANIMATIONS_SETTING_NAME)) { const knob = setting.querySelector(".knob") as HTMLElement; - const enabled = (isAnimationEnabled = !isAnimationEnabled); + isAnimationEnabled = !isAnimationEnabled; animationManager.isAnimationEnabled = isAnimationEnabled; savePreferenceValue( ANIMATIONS_PREFERENCE_NAME, isAnimationEnabled ? SETTING_ENABLED : SETTING_DISABLED, ); - if (enabled) { + if (isAnimationEnabled) { knob.classList.add("enabled"); } else { knob.classList.remove("enabled"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 564 - 574, The const enabled variable is redundant; update the toggle logic in the click handler so you flip isAnimationEnabled directly and use it in the branch: set isAnimationEnabled = !isAnimationEnabled, assign animationManager.isAnimationEnabled = isAnimationEnabled, call savePreferenceValue(ANIMATIONS_PREFERENCE_NAME, isAnimationEnabled ? SETTING_ENABLED : SETTING_DISABLED), and then use if (isAnimationEnabled) { knob.classList.add("enabled"); } else { knob.classList.remove("enabled"); } (remove the const enabled declaration).src/manager/animation.ts (1)
19-24: ⚡ Quick winConsider using definite assignment assertion instead of suppression.
While the directive normalization is good, TypeScript's definite assignment assertion operator (
!) is a cleaner solution for fields initialized via method calls:- // `@ts-expect-error` TODO: This field is assigned in the constructor via resetState but TS is not smart enough to realize that - public newBlocks: Position[]; - // `@ts-expect-error` TODO: This field is assigned in the constructor via resetState but TS is not smart enough to realize that - public movedBlocks: (Position | undefined)[][]; - // `@ts-expect-error` TODO: This field is assigned in the constructor via resetState but TS is not smart enough to realize that - public mergedBlocks: MergedBlock[]; + public newBlocks!: Position[]; + public movedBlocks!: (Position | undefined)[][]; + public mergedBlocks!: MergedBlock[];This tells TypeScript you guarantee initialization without suppressing potential type errors in the field usage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/manager/animation.ts` around lines 19 - 24, Replace the three "// `@ts-expect-error`" suppressions on the class fields newBlocks, movedBlocks, and mergedBlocks with TypeScript definite assignment assertions (append "!" to the property names) so they read "public newBlocks!: Position[];", "public movedBlocks!: (Position | undefined)[][];", and "public mergedBlocks!: MergedBlock[];"; remove the corresponding ts-expect-error lines and keep resetState as the constructor/initializer that guarantees these fields are set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/cli.ts`:
- Around line 74-76: CLIGameStorage.loadFile incorrectly passes a Buffer to
JSON.parse (and suppresses the TS error); update the code in
CLIGameStorage.loadFile to read text not a Buffer (e.g.,
fs.readFileSync(filename, "utf8")) or call .toString("utf8") on the returned
Buffer, remove the `@ts-expect-error`, and then JSON.parse the resulting string;
apply the same change to any other CLI storage file-read sites that parse JSON
from disk to avoid Buffer-to-string type mismatches.
In `@vite.config.ts`:
- Around line 28-29: Replace the incorrect numeric literal and suppressed error
on the terser options: in the terserOptions object (property ecma) change ecma:
6 to a valid Terser ECMA year such as ecma: 2015 and remove the preceding "//
`@ts-expect-error` TODO..." comment so the TypeScript type is satisfied without
suppression.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 564-574: The const enabled variable is redundant; update the
toggle logic in the click handler so you flip isAnimationEnabled directly and
use it in the branch: set isAnimationEnabled = !isAnimationEnabled, assign
animationManager.isAnimationEnabled = isAnimationEnabled, call
savePreferenceValue(ANIMATIONS_PREFERENCE_NAME, isAnimationEnabled ?
SETTING_ENABLED : SETTING_DISABLED), and then use if (isAnimationEnabled) {
knob.classList.add("enabled"); } else { knob.classList.remove("enabled"); }
(remove the const enabled declaration).
In `@src/manager/animation.ts`:
- Around line 19-24: Replace the three "// `@ts-expect-error`" suppressions on the
class fields newBlocks, movedBlocks, and mergedBlocks with TypeScript definite
assignment assertions (append "!" to the property names) so they read "public
newBlocks!: Position[];", "public movedBlocks!: (Position | undefined)[][];",
and "public mergedBlocks!: MergedBlock[];"; remove the corresponding
ts-expect-error lines and keep resetState as the constructor/initializer that
guarantees these fields are set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19be4bb6-74dd-4316-9a95-49f4de4fcb1d
📒 Files selected for processing (19)
cypress.config.tscypress/support/commands/commands.tseslint.config.mjsplugins/app-labels.tsplugins/file-transformer.tssrc/component/how-to-play/index.tssrc/game.tssrc/index.tssrc/manager/animation.tssrc/manager/asset.tssrc/manager/undo.tssrc/preferences.tssrc/render.tssrc/share/browser.tssrc/storage/cli.tstest/browser_storage_test.tstest/cli_storage_test.tsvendor/snow.tsvite.config.ts
💤 Files with no reviewable changes (1)
- src/component/how-to-play/index.ts
- cli.ts: read state files as UTF-8 text so JSON.parse receives a string, removing the Buffer-related @ts-expect-error - vite.config.ts: use a valid Terser ECMA year (2015) instead of `6`, removing the @ts-expect-error - animation.ts: use definite assignment assertions for fields initialized via resetState() instead of @ts-expect-error suppressions - index.ts: drop the redundant `enabled` local and branch on isAnimationEnabled directly
2 participants
Summary
Fixes all 92 ESLint errors reported by the failing Code Quality workflow run. After these changes,
pnpm lint,pnpm type-check, andpnpm type-check:cypressall pass (the unit test suite and Prettier check remain green as well).What changed
@ts-ignore→@ts-expect-error(ban-ts-comment)commands.ts,animation.ts,undo.ts,storage/cli.ts,cli_storage_test.ts, andvite.config.ts.how-to-play, the SentrybeforeSendhandler, and a deadimport.metadirective ingame.ts) — converting these would have produced unused directive type errors instead.Replace
anywith real types (no-explicit-any)GameEventDatatype for the gameEventHandlerpayload.getPreferenceValuegeneric (defaulting tounknown) so call sites infer the right type, and typed thePreferencesindex signature /savePreferenceValuevalue asunknown.catchclauses (share/browser.ts,index.ts) instead of typing the error asany.Storage.setItemvalue asstring.Other rule fixes
no-unsafe-function-type: replacedFunctionwith() => voidin the prompt-dialog options.prefer-const/no-var: applied acrossindex.ts,asset.ts,snow.ts, etc. (mostly viaeslint --fix).no-useless-assignment: scoped theenabledflag into the branch that uses it.preserve-caught-error: attached{ cause: err }to the re-thrown error in the file-transformer plugin.no-unused-vars).Scoped ESLint overrides (
eslint.config.mjs)__mocks__/**/*.js: allow CommonJSrequire/module.exportsfor the Jestfsmock.cypress/**/*.ts: disableno-unused-expressions(Chai assertions likeexpect(x).to.be.true) andno-explicit-any(CypressChainable<any>command declarations).Testing
pnpm lint— 0 errorspnpm type-check— passespnpm type-check:cypress— passespnpm run test-ci— 48/48 passingpnpm format:check— cleanhttps://claude.ai/code/session_016gBdoTccTGigXLvLMMS38W
Generated by Claude Code
Summary by CodeRabbit
Refactor
anytypes tounknownand introducing explicit type signatures.Tests