Update to latest eslint 9 with neostandard

[vincentfretin]

Update to latest eslint 9 with neostandard, removed eslint-plugin-storybook but you can reintroduce it if you want.

PR similar to what I did in aframe-inspector aframevr/aframe-inspector#842

[claude]

PR Review: Update to ESLint 9 with neostandard

Great modernization work! Migrating to ESLint 9's flat config format is the right move, and using neostandard as the spiritual successor to eslint-config-standard makes sense. The PR is clean and well-scoped. Here are a few observations:

---

Potential Issue: eslint-plugin-react not in package.json

eslint.config.mjs imports eslint-plugin-react directly:

import reactPlugin from "eslint-plugin-react";

But eslint-plugin-react was removed from package.json devDependencies. It's currently only available as a transitive dependency (likely pulled in by one of the Storybook packages). Relying on a transitive dep for a direct import is fragile — if the Storybook packages change their deps, this will break on fresh installs.

Suggestion: Add eslint-plugin-react back to devDependencies explicitly since it's imported directly in the config.

---

React recommended rules may be narrower now

The old config extended plugin:react/recommended and plugin:react/jsx-runtime, which enabled a full suite of React-specific lint rules. The new config registers the react plugin but does not spread reactPlugin.configs.recommended or reactPlugin.configs['jsx-runtime']. The only react rules active are the four explicitly listed (all but react/jsx-uses-vars are disabled).

This is probably intentional to reduce noise while the codebase catches up, but worth noting that rules like react/no-danger, react/no-deprecated, react/display-name, etc. are no longer enforced. If you want to re-enable react recommended rules, you can add:

...reactPlugin.configs.flat.recommended,
...reactPlugin.configs.flat['jsx-runtime'],

---

eslint-disable-next-line react-hooks/exhaustive-deps suppressions

Several files add suppression comments as a quick fix. Most are fine (mount-only effects with [] deps), but a few are worth reviewing when time allows:

• SceneCard.component.jsx: The useEffect runs fetchUsernames() (an async function defined inside the component) whenever scenesData changes, but fetchUsernames isn't in the deps. This could fire with a stale reference. Consider defining fetchUsernames outside the effect or wrapping it in useCallback.
• ReportModal.component.jsx: The useEffect accesses projectInfo on open. If this is intentional (read-once on open), the suppression is fine, but a comment explaining the intent would help future readers.

These aren't blockers — just things to clean up in follow-up PRs.

---

Spurious JSX comment in TimeControls.js

{/* eslint-disable-next-line react-hooks/refs -- reads A-Frame component ref, re-render driven by polling interval */}

react-hooks/refs is not a real ESLint rule name (it would be react/no-refs or similar). This comment has no effect on linting. If it's purely documentation, consider making it a plain comment outside the JSX; if it's intended to suppress a specific rule, the rule name needs to match the actual rule being triggered.

---

Minor: useCallback → useMemo in UsernameEditor.jsx

Swapping useCallback(debounce(...), deps) for useMemo(() => debounce(...), deps) is semantically equivalent here (both recreate the debounced function when originalUsername changes). The useMemo pattern is the more conventional approach for debounced functions in React, so this is a net improvement.

---

What's great about this PR

• Clean migration from legacy .eslintrc + .eslintignore to the new flat config format
• Removal of @babel/eslint-parser (no longer needed in ESLint 9)
• Removal of the redundant braces in switch case blocks (assetsUtils.js, TextureWidget.js)
• Moving generateCacheBust outside the component in ViewerSidebar.js (correct fix — pure function didn't need to be inside the component)
• Removal of now-unnecessary inline // eslint-disable-line comments in src/tested/ files

The main ask before merging would be explicitly pinning eslint-plugin-react in devDependencies to avoid a latent dependency issue.

[vincentfretin]

eslint-plugin-react is a dependency of neostandard, but I'll add it explicitely in next commit.
You're right about the the react recommended rules, I thought neostandard included those but that doesn't seem to be the case, I'll add them.
All the remaining "eslint-disable-next-line react-hooks/exhaustive-deps" were checked by your twin Claude Code cli to be necessary, those dependencies are not included on purpose to not trigger a render loop.
The react-hooks/refs rule is a correct eslint rule, you may not be aware of the latest rules in eslint-plugin-react-hooks

[claude]

PR Review: ESLint 9 + Neostandard Migration

Great work! This is a clean, well-executed migration to ESLint 9's flat config format. The approach of turning off stylistically opinionated rules (rather than mass-reformatting all code) is the right call for a tooling-only PR.

---

✅ Positives

• Flat config format in eslint.config.mjs is the correct ESLint 9 approach and removes the need for @babel/eslint-parser (which was only required by the legacy parser).
• neostandard is the natural successor to eslint-config-standard and a solid choice.
• generateCacheBust moved outside component in ViewerSidebar.js — good refactor; pure utility functions with no dependency on component state don't belong inside the component body.
• useCallback → useMemo in UsernameEditor.jsx — useMemo(() => debounce(...), deps) is actually the more semantically correct pattern for creating stable debounced callbacks (the debounce timer isn't recreated on every render, only when deps change).
• Removing stale inline // eslint-disable-line comments from src/tested/ files is a nice cleanup.

---

⚠️ Things to Verify

Node.js version requirement
ESLint 9 requires ^18.18.0 || ^20.9.0 || >=21.1.0. Please confirm this aligns with the project's CI Node version (check .nvmrc, engines in package.json, or the GitHub Actions workflow). This will silently fail in older environments.

eslint-plugin-storybook removed
Intentional per the PR description, but this means Storybook story files will no longer be linted with Storybook-specific rules (e.g., no-redundant-story-name, prefer-story-title). Worth re-adding once there's a neostandard-compatible version or when it supports ESLint 9 flat config. A short TODO comment in eslint.config.mjs would help future contributors know this was intentional:

// TODO: Re-add eslint-plugin-storybook when it supports ESLint 9 flat config

TransformControls.js is now partially linted
The old .eslintignore excluded src/editor/lib/TransformControls.js entirely. The new config does not — it only ignores src/lib. The file still has /* eslint-disable curly */ at the top, so linting will run but with curly disabled. This is fine, just worth being aware of.

---

💡 Minor Suggestions

eslint-disable-next-line comments lack explanations
Several files now suppress react-hooks/exhaustive-deps with no rationale. Suppressions without context are maintenance traps. Consider adding a brief explanation in the same comment:

// eslint-disable-next-line react-hooks/exhaustive-deps -- intentional: fetchUsernames not stable, scenesData is the only trigger needed

The TimeControls.js suppression is a good example of the right pattern — it already includes an explanation. Apply the same to the others.

Global ignores: .storybook vs .storybook/
Minor nit: the old .eslintignore had .storybook/ (trailing slash, directory-only). The new config has .storybook (no trailing slash). ESLint 9 flat config treats these identically for directories, so this is fine — just noting the change is safe.

---

Summary

This is a solid, low-risk dependency upgrade with minimal source code changes. The source file edits are all either necessary lint compliance or good side-effect refactors. No functional regressions introduced. The main pre-merge action item is verifying the Node.js version constraint in CI.

[claude]

PR Review: ESLint 9 + neostandard migration

Overall this is a well-structured tooling upgrade. The flat config approach is cleaner and easier to maintain. Here are my observations:

Positives

• Good modernization: Migrating from deprecated eslintrc to the ESLint 9 flat config is the right move. eslint.config.mjs is clean and well-organized.
• neostandard is a solid choice as the community-maintained successor to eslint-config-standard.
• @babel/eslint-parser removal is appropriate. ESLint 9 with neostandard can parse modern JS/JSX natively without the Babel parser dependency.
• useCallback to useMemo in UsernameEditor.jsx: Correct fix. The old useCallback(debounce(...), deps) pattern evaluated debounce() on every render before passing to useCallback, creating a new debounced function instance each render. useMemo(() => debounce(...), deps) only creates the debounced function when dependencies change.
• generateCacheBust hoisted to module scope in ViewerSidebar.js: Good -- this function has no closure dependencies and does not need to be recreated on each render.
• Switch statement cleanup in TextureWidget.js and assetsUtils.js: Removing redundant braces around case bodies is cleaner.

---

Issues

1. JSX comment will not suppress ESLint rule (bug) in TimeControls.js

JSX expression comments ({/* */}) are not parsed as ESLint disable directives -- only JavaScript comments (// or /* */) work. The current code:

{/* eslint-disable-next-line react-hooks/refs -- reads A-Frame component ref */}
<span>{formatTime()}</span>

...will not actually suppress the lint warning. A working form would be to use a JS line comment in JSX context.

Also worth confirming: is react-hooks/refs the correct rule name? Standard rules in eslint-plugin-react-hooks are react-hooks/exhaustive-deps and react-hooks/rules-of-hooks. Double-check the actual rule name being triggered here.

2. Debounce not cancelled on unmount in UsernameEditor.jsx

The useMemo change is correct, but the debounced function is not cancelled when the component unmounts or when originalUsername changes. If the component unmounts while a debounce timer is pending, it will attempt state updates after unmount. This was pre-existing, but since this PR touches the code it is worth fixing by adding a cleanup effect that calls checkUsername.cancel().

3. eslint-plugin-storybook silently dropped

The PR description mentions this but it is worth tracking as a follow-up. Storybook lint rules help catch issues invisible at runtime. eslint-plugin-storybook supports ESLint 9 flat config and can be re-added with minimal effort.

4. Node.js globals for CommonJS files

The old config had node: true globally, covering files like src/tested/*.js that use CommonJS (require, module.exports). The new config uses neostandard({ env: ['browser'] }). Worth confirming Node.js globals are still available for those files and that npm run lint passes cleanly on src/tested/.

---

Minor

• react-hooks/set-state-in-effect disabled globally: This is a new rule in react-hooks v7 that guards against a real class of bugs. Consider using targeted eslint-disable-next-line at specific call sites where the pattern is intentional, rather than a project-wide disable.
• The no-var, prefer-const, and object-shorthand rules remain off, which is reasonable given the mix of legacy A-Frame code and modern React.
• node_modules is not listed in ignores but ESLint 9 ignores it by default, so this is not an omission.

---

Overall this is a solid upgrade. The main concrete issue to address before merging is the ineffective JSX eslint-disable comment in TimeControls.js. Everything else is correct or a minor follow-up item.

[claude]

PR Review: ESLint 9 Migration with neostandard

This is a clean and well-structured ESLint migration. Overall the approach is sound. A few observations below.

Positives

• The migration from legacy .eslintrc to flat eslint.config.mjs is done correctly and follows ESLint 9 best practices.
• neostandard({ noStyle: true }) is the right call for a Prettier project — avoids style rule conflicts.
• Removing dead // eslint-disable-line no-unused-vars comments from src/tested/ is a nice cleanup.
• Moving generateCacheBust in ViewerSidebar.js to module scope is good refactoring — the function has no dependency on component state.
• Cleaning up the /* eslint-disable react/no-danger */ comments is correct: react/no-danger was never in plugin:react/recommended, so those suppressions were no-ops.

---

Concerns

1. useCallback → useMemo in UsernameEditor.jsx

Both approaches are functionally equivalent for wrapping a debounced function, but useCallback is the more semantically appropriate hook for memoizing functions. The change likely came from a new ESLint rule preference — not a blocker, just worth noting.

Also a pre-existing issue (not introduced here): when originalUsername changes, a new debounced function is created but the old one is never explicitly .cancel()-ed on unmount. A useEffect cleanup would prevent any stale async calls.

2. Multiple react-hooks/exhaustive-deps suppressions added

Six new // eslint-disable-next-line react-hooks/exhaustive-deps suppressions were added in:
AddLayerPanel, SceneCard, ReportModal, AIChatPanel, PurchaseModal, Auth.context.js

Some of these are legitimate (intentional mount-only effects, A-Frame integration patterns). But a second pass to confirm each is truly intentional rather than just silencing the linter would be worthwhile. Auth.context.js and PurchaseModal.jsx in particular would benefit from a short inline comment explaining why the dep is intentionally omitted.

3. Likely incorrect eslint-disable rule name in TimeControls.js

// eslint-disable-next-line react-hooks/refs -- reads A-Frame component ref, re-render driven by polling interval

react-hooks/refs is not a real rule in eslint-plugin-react-hooks. This comment may be silently doing nothing. If a rule was triggering here, it was most likely react-hooks/exhaustive-deps — the comment should use that name to actually suppress anything.

4. react-hooks/set-state-in-effect disabled globally

'react-hooks/set-state-in-effect': 'off'

This is a new rule from react-hooks v7. Disabling it globally hides cases where setState is called in effects in ways that could cause subtle bugs. It might be worth enabling it and fixing any violations, or at least leaving a comment explaining the rationale for the global disable.

5. eslint-plugin-storybook removed

The PR notes this can be reintroduced. Since @storybook/addon-docs is still a dev dependency and the project has Storybook stories, re-adding the plugin (which supports flat config via storybook/flat/recommended) would keep Storybook-specific anti-patterns caught at lint time. A follow-up PR would be fine.

---

Minor

• src/editor/lib/TransformControls.js was excluded via the old .eslintignore but is now under src/**/*.{js,jsx} in the new config. Net positive coverage — just confirm npm run lint passes cleanly on it.

---

Summary

The migration is solid. Before merging, worth addressing:

1. Verify react-hooks/refs in TimeControls.js is actually the right rule name (likely should be react-hooks/exhaustive-deps)
2. Consider re-enabling react-hooks/set-state-in-effect or documenting why it's globally disabled
3. Consider reintroducing eslint-plugin-storybook (follow-up PR is fine)
4. Verify npm run lint passes cleanly end-to-end