Make npm run lint work in fast-qa - #258
Open
Hotragn wants to merge 1 commit into
Open
Conversation
The seventh recipe from tinyfish-io#248, and the one tinyfish-io#249 and tinyfish-io#250 could not cover. npm run lint fails because fast-qa is on Next 16.1.3 and next lint was removed in Next 16 -- the CLI reads "lint" as a directory argument: Invalid project directory provided, no such directory: .../fast-qa/lint Renaming the script is not enough here. fast-qa already ships an eslint.config.mjs importing eslint/config, eslint-config-next/core-web-vitals and eslint-config-next/typescript, but declares neither eslint nor eslint-config-next, so the config references three packages the recipe has no dependency on. Changing only the script moves the failure from "command not found" to "cannot resolve eslint". Adds eslint ^9 and eslint-config-next 16.1.3 as devDependencies, matching silicon-signal, which is also on Next 16.1.3 and pins the same version. The existing config file is untouched and now resolves. Turning lint on for the first time surfaced 4 errors, fixed here: - execute-tests/route.ts destructured five names as let when only parallelLimit is reassigned; split so the rest are const. - project-dialog.tsx pushed four pieces of form state from an effect whenever the project prop changed, tripping react-hooks/set-state-in-effect. There is one call site, so the dialog now takes a key and React remounts it, which is what the existing useState initialisers already did. The effect and its import are gone. Checked all four paths -- create, edit, switching projects, close -- and 'new' cannot collide with a real id since generateId() starts with a timestamp. Verified from scratch: rm -rf node_modules && npm ci, then npm run lint and tsc --noEmit, all exit 0. One pre-existing exhaustive-deps warning in lib/hooks.ts left alone, as adding the dependency risks changing render behaviour. The lock grows by 369 entries because this recipe is getting ESLint for the first time. It was already in sync beforehand, and --package-lock-only produces the same result, so the churn is the toolchain rather than drift.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes #248. The seventh recipe, and the one with a different root cause from the six in #249 and #250.
The bug
fast-qais on Next 16.1.3, andnext lintwas removed in Next 16 — the CLI parseslintas a directory argument. Same walltenders-finderhit.But swapping the script isn't enough here.
fast-qaalready ships aneslint.config.mjsthat importseslint/config,eslint-config-next/core-web-vitalsandeslint-config-next/typescript, while declaring neithereslintnoreslint-config-next:So renaming the script alone just moves the failure from "command not found" to "cannot resolve eslint". I tried exactly that while working on #250, backed it out, and reported it on #248 rather than shipping a half-fix.
The fix
eslint: ^9andeslint-config-next: 16.1.3added as devDependencies, matchingsilicon-signal— which is also on Next 16.1.3 and pinseslint-config-nextto the same version. Script moves toeslint, the idiom in the other Next 16 recipes. The existingeslint.config.mjsis untouched and now actually resolves.The 4 errors its first-ever lint run surfaced
app/api/execute-tests/route.ts:43— threeprefer-constviolations on one destructure. OnlyparallelLimitis reassigned (clamped on the next line), so that staysletand the rest becomeconst:components/qa/project-dialog.tsx:39—react-hooks/set-state-in-effect. An effect was pushing four pieces of form state whenever theprojectprop changed:There's exactly one call site, so rather than the render-phase adjustment dance I gave the dialog a
keyand let React remount it:The
useState(project?.name || '')initialisers already did this work; the effect existed only because the component never remounted. With the key, the effect and its import both go away.I walked the four paths to confirm behaviour is unchanged — open-for-create, open-for-edit, switching directly between two projects, and close (which resets
editingProjecttoundefined). Each changes the key and remounts with the right values.'new'can't collide with a real id either:generateId()returns`${Date.now()}-${random}`, so ids always start with a digit.The manual field reset at the end of
handleSaveis now redundant but harmless, so I left it rather than widen the diff.Verification
One pre-existing warning left alone —
lib/hooks.ts:305,exhaustive-depsmissingmodifiers. Adding it risks changing render behaviour, which isn't a call to make inside a lint repair. Same treatment as thesummer-school-finderwarning in #250.On the lockfile
The lock diff is large — 369 package entries added — because this recipe is getting ESLint for the first time and that pulls in the whole toolchain (typescript-eslint, the Next plugin, the import/jsx-a11y/react-hooks plugins and their transitives). I checked it isn't hiding anything else: the lock was already in sync with
package.jsonbefore this change, unliketutor-finderin #253, and regenerating with--package-lock-onlyinstead of a full install produced the same result (369 vs 363), so the churn is inherent rather than incidental drift I introduced.Independent of #249, #250 and #253.