fix(lint): repair no-case-declarations errors and gate lint in CI - #108
Open
Ravi chandu Edru (RaviChanduEdru) wants to merge 2 commits into
Open
Ravi chandu Edru (RaviChanduEdru) wants to merge 2 commits into
Ravi chandu Edru (RaviChanduEdru) wants to merge 2 commits into
Conversation
`routeToHash` declared `hash`, `queryParams`, and `query` directly inside the `case 'catalogue':` block, tripping `no-case-declarations` three times. Those bindings leak into the sibling `case` clauses in the temporal dead zone, so `npm run lint` failed on main. CI never ran `npm run lint` - it covers catalogue build, learn build, RDF validation, tsc, a11y, tests, and build - which is how the errors landed. Add a Lint step before the type check so this cannot regress again. The remaining react-hooks/exhaustive-deps warning in OntologyGraph is left alone: eslint exits 0 on warnings, and changing that effect's dependency array would be a behavior change out of scope here.
The `npm run catalogue:build succeeds with the real catalogue` test passed `timeout: 30000` to `execSync`, but that only bounds the child process - the test itself still ran under Vitest's 5s default `testTimeout`, so the intended 30s never applied. The step spawns `npx tsx` and compiles the whole catalogue, which measured 4.6s to 7.5s locally on Windows - already over the 5s default, and it fails outright on a cold cache. Hoist the value into a shared constant and pass it as the `it()` timeout so the child-process bound and the test bound agree.
Contributor
Author
@microsoft-github-policy-service agree |
This was referenced Aug 11, 2026
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.
Two small repo-hygiene fixes:
npm run lintcurrently fails onmain, and the end-to-end catalogue test runs under a much shorter timeout than it intends.1.
npm run lintfails onmainrouteToHashdeclareshash,queryParams, andquerydirectly inside thecase 'catalogue':block, which tripsno-case-declarationsthree times:Those bindings leak into the sibling
caseclauses in the temporal dead zone. Wrapping the clause in braces scopes them correctly — no behavior change.CI never runs
npm run lint(it covers catalogue build, learn build, RDF validation,tsc, a11y, tests, and build), which is how the errors landed unnoticed. This PR adds aLintstep before the type check so it cannot regress again.The one remaining
react-hooks/exhaustive-depswarning inOntologyGraph.tsxis deliberately left alone — eslint exits 0 on warnings so the new gate passes, and changing that effect's dependency array would be a behavior change that doesn't belong in this PR. Happy to add--max-warnings 0in a follow-up if you'd prefer the stricter gate.2. Catalogue build test doesn't get the timeout it asks for
compile-catalogue.test.tspassestimeout: 30000toexecSync, but that only bounds the child process — the test itself still ran under Vitest's 5s defaulttestTimeout, so the intended 30s never applied.That step spawns
npx tsxand compiles the whole catalogue. It measured 4.6s and 7.5s across two local runs on Windows, both over the 5s default, and it fails outright on a cold cache:The fix hoists the value into a shared constant and passes it as the
it()timeout, so the child-process bound and the test bound agree. As the catalogue grows this gets closer to biting CI too.Verification
npm run lint— exit 0 (was 3 errors)npx tsc -b— exit 0npm test— 394/394 passed (was 393 passed, 1 failed)