feat: add Biome linting with unused variables as errors #275
Conversation
- Replace existing linting setup with Biome - Configure Biome to treat unused variables as errors - Add lint and lint:fix scripts to package.json - Configure Biome with recommended rules and formatting
- Format code according to Biome rules - Fix linting issues automatically resolved by Biome
- Update deploy workflow to run npm run lint instead of Prettier - Ensures linting rules (including unused variables) are enforced in CI
- Remove conflicting position: relative from .simulationsummary class - Add explicit position: fixed to .simulationsummary-expanded - Fixes issue where expanded summary was hiding canvas instead of overlaying - Also fix Simulation component fragment and Figure test accessibility
- Fix non-null assertion in AutoStartSimulation.tsx - Fix template literals and hook dependencies in Figure.tsx - Fix isFinite usage in ColorLegend.tsx - Add button types to ResponsiveSimulationSummary.test.tsx - Fix accessibility in Figure.test.tsx - Apply Biome auto-fixes across codebase (formatting, imports, etc.) - Fix CSS positioning issue with simulation summary overlay
Summary of ChangesHello @andeplane, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the project's developer tooling by migrating from Prettier to Biome for linting and formatting. The primary goal is to enhance code quality and consistency across the repository by enforcing stricter rules, particularly for unused variables. This transition involved a comprehensive application of auto-fixes and targeted manual adjustments to ensure the codebase fully complies with the new, more robust standards set by Biome. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step forward for the project's code quality by introducing Biome for linting and formatting. The automated fixes have cleaned up a lot of the codebase, improving consistency and readability. The manual fixes for accessibility and logic are also valuable improvements. I've found one regression in the console's auto-scrolling behavior that should be addressed. Overall, this is a solid contribution to the developer experience and maintainability of the project.
- Fix accessibility issues: convert divs with role=button to actual button elements - Add type='button' to all button elements - Fix globalThis shadowing with biome-ignore comment - Fix JSX key issues in iterables - All lint errors resolved, only warnings remain
- Biome incorrectly removed lammpsOutput from dependency array - Effect needs to run when output changes to auto-scroll console - Fixes regression in console auto-scrolling behavior
andeplane
force-pushed
the
feat/add-biome-linting
branch
from
da53568 to
e30d5e3
Compare
- Button element with all:unset was breaking dark theme CSS - Use div with proper accessibility attributes instead - Add biome-ignore for useSemanticElements since styling requires div
- Button element with all:unset was breaking CSS styling - Use div with proper accessibility attributes instead
andeplane
force-pushed
the
feat/add-biome-linting
branch
from
fc7a5aa to
70a7985
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on introducing Biome.js for code linting and formatting, updating dependencies, and improving code quality and accessibility across the application. Key changes include adding biome.json and corresponding npm scripts, updating mixpanel-browser, and reordering imports for consistency. Many files received minor formatting adjustments, such as using template literals and improving whitespace. Accessibility was enhanced by adding role, tabIndex, and onKeyDown attributes to interactive elements, particularly in App.tsx and Examples.tsx. The Figure.tsx component now correctly manages onToggleSyncDataPoints for modifiers, ensuring proper cleanup and setup. Additionally, isFinite checks were replaced with Number.isFinite for improved robustness in several components. A review comment highlighted that the simulation dependency was incorrectly removed from a useEffect hook in View.tsx, which could lead to atom selections persisting between simulations. Another comment noted that renderSettings.showSimulationBox was missing from a useMemo dependency array in SimulationSummaryContent.tsx, causing the UI to not update correctly when this setting changed.
- Add renderSettings.showSimulationBox to dependency array in SimulationSummaryContent - This ensures the checkbox state updates correctly when renderSettings changes - Add biome-ignore comment explaining why this dependency is needed - Addresses PR review feedback from gemini-code-assist
- Add biome-ignore comment explaining why simulation dependency is needed - The simulation dependency is intentionally included to clear selection when simulation changes - This was a recent bug fix (commit 054bad2) and must remain in dependency array
- Add public/jupyter to excludes (generated by JupyterLite build in CI) - Set ignoreUnknown: true to skip unknown file types - Prevents Biome from checking generated files and binary files in CI
- Memoize simulationSummaryColumns with correct dependencies - Use useCallback for setSyncFrequency and setUIUpdateFrequency - Reference simulation in useEffect body to satisfy exhaustive dependencies rule - Fixes CI linting errors while maintaining correct behavior
This reverts commit 5fc9083.
- Change useExhaustiveDependencies from error to warn in biome.json - Allows important dependencies (like renderSettings.showSimulationBox) to remain - CI will pass with warnings instead of failing on false positives - Keeps biome-ignore comments for documentation purposes
|
/gemini review |
- Place biome-ignore comment before hook call, not in dependency array - Use dependency name in parentheses: useExhaustiveDependencies(dependencyName) - Remove useExhaustiveDependencies: warn override from biome.json - Rule is now error-level, but properly suppressed with biome-ignore - Fixes CI linting errors while maintaining required dependencies
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the codebase's quality and consistency by introducing Biome for linting and formatting. The automated fixes across the project are extensive and well-executed, leading to cleaner and more readable code. I'm particularly impressed with the manual fixes that were also included, such as improving accessibility with ARIA attributes and keyboard handlers, fixing a tricky CSS positioning issue, and making the code more robust by removing non-null assertions and cleaning up hook dependencies. The attention to detail is evident throughout the changes. Great work!
Phase 1 - Quick fixes: - Remove non-null assertion (!) from View.tsx prevBonds - Replace explicit any with unknown in View.tsx type guard - Fix iterable callback returns in Examples.tsx and processing.ts (wrap in blocks) Phase 2 - Array keys: - Replace array index keys with stable identifiers (measurement.label) in SelectedAtomsInfo.tsx Phase 3 - CSS !important: - Disable noImportantStyles rule in biome.json (CSS biome-ignore comments don't work) - Remove unused CSS suppression comments All planned warnings from the linting plan are now fixed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant step forward for the project's code quality and developer experience by introducing Biome for linting and formatting. The changes are extensive, touching many files, but they are primarily for code style, consistency, and fixing minor issues like unused variables and accessibility, which is excellent. I've found one critical issue where fixing a linting error seems to have inadvertently introduced a bug that prevents graphs from updating. Other than that, the changes look solid and well-executed.
| if (graph && plotConfig?.data1D) { | ||
| graph.updateOptions({ file: plotConfig.data1D.data }); | ||
| } | ||
| }, [graph, plotConfig, timesteps]); | ||
| }, [graph, plotConfig]); | ||
|
|
There was a problem hiding this comment.
This useEffect hook is missing _timesteps in its dependency array. The component subscribes to timesteps from the store, which causes it to re-render when the simulation progresses. However, since _timesteps is not in the dependency array, this effect won't re-run, and the graph will not be updated with new data points.
This seems to be an incorrect fix for a no-unused-vars linting error. The _timesteps variable (which should probably be renamed back to timesteps) is essential for triggering graph updates.
Please add it back to the dependency array. You might also need to add a biome-ignore comment for the useExhaustiveDependencies rule if the linter complains, similar to what was done in src/containers/Console.tsx.
useEffect(() => {
if (graph && plotConfig?.data1D) {
graph.updateOptions({ file: plotConfig.data1D.data });
}
}, [graph, plotConfig, _timesteps]);
The graph was not updating when new data points arrived during simulation because timesteps was missing from the dependency array. Since data1D.data arrays are mutated in place by modifiers, plotConfig reference doesn't change, so the effect needs timesteps to trigger updates.
Summary
This PR replaces the existing linting setup with Biome and configures it to treat unused variables as errors.
Changes
Scripts Added
npm run lint- Check for linting issuesnpm run lint:fix- Auto-fix linting issuesTesting
Breaking Changes
None - this is a developer tooling change only.