chore: Adding NoConsoleLog rule to Biome linter #1348
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes to multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
package.json (1)
12-12: Document version bump reason in package.json.Consider adding a comment in package.json to document that this version bump is required for the NoConsoleLog rule support.
"devDependencies": { - "@biomejs/biome": "1.9.4", + "@biomejs/biome": "1.9.4", // Required for NoConsoleLog rule support "lint-staged": "^15.2.7",biome.jsonc (1)
62-62: Consider adding JSON schema validation to prevent syntax errors.While fixing the JSON structure is good, we should prevent such issues in the future.
Consider:
- Adding pre-commit hooks to validate JSON files
- Ensuring your IDE is configured to validate JSON schema
- Adding JSON validation to your CI pipeline
"scripts": { + "validate:config": "biome check biome.jsonc" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
biome.jsonc(2 hunks)package.json(1 hunks)
🔇 Additional comments (1)
package.json (1)
12-12: Verify @biomejs/biome version bump compatibility.
The version bump from 1.9.2 to 1.9.4 looks good, but let's verify there are no breaking changes.
✅ Verification successful
Version bump is compatible with NoConsoleLog rule
The version bump from 1.9.2 to 1.9.4 is safe as:
- The NoConsoleLog rule is already configured in biome.jsonc
- The repository's CHANGELOG.md shows no breaking changes in recent versions
- The rule configuration in biome.jsonc is compatible with both versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the changelog between versions 1.9.2 and 1.9.4 for breaking changes
# and verify if the NoConsoleLog rule requires this version
# Search for changelog or release notes in the repository
rg -i "changelog|release.*notes" -g "!node_modules"
# Check for any existing Biome configuration using NoConsoleLog rule
# to ensure the version requirement is consistent
rg -l "noConsoleLog" -g "*.{json,jsonc}"
Length of output: 18207
Script:
#!/bin/bash
# Check the content of biome.jsonc to see if NoConsoleLog rule is already configured
cat biome.jsonc
# Check the content of CHANGELOG.md to see if there are any breaking changes
cat CHANGELOG.md
Length of output: 13022
biome.jsonc
Outdated
| "noConsoleLog": { | ||
| "level": "error" | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
The new noConsoleLog rule will cause build failures due to existing console.log statements in the codebase. Here are the locations that need to be addressed:
packages/docusaurus-tldraw-plugin/src/tldr.jsx- Error loggingpackages/frontend/src/analytics.ts- Initialization loggingpackages/frontend/src/components/FilterBar/FilterBar.tsx- Debug loggingpackages/frontend/src/components/InboxItem/InboxItemsHeader.tsx- Debug loggingpackages/frontend/src/components/InboxItem/InboxItem.tsx- Debug loggingpackages/frontend/src/components/InboxItem/GuiActions.tsx- Error and success loggingpackages/frontend/src/auth/api.ts- Error loggingpackages/frontend/src/main.tsx- Error loggingpackages/fastify-graphiql/Readme.md- Example codepackages/storybook/src/stories/GuiActions/guiActions.stories.tsx- Multiple debug logs
Consider:
- Replacing error logging with proper error handling/logging utilities
- Removing debug console.logs or replacing with appropriate logging mechanisms
- Using environment-based logging for development-only logs
🔗 Analysis chain
LGTM! Let's verify the impact of this new rule.
The addition of the noConsoleLog rule as an error is appropriate for maintaining code quality. However, we should verify the impact on the existing codebase.
Let's check for any existing console.log statements that might break the build:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for console.log statements in the codebase
# Exclude test files, configuration files, and build artifacts
# Search for console.log statements
echo "Searching for console.log statements..."
rg "console\.log\(" \
--type ts \
--type tsx \
--type js \
--type jsx \
-g '!*.test.*' \
-g '!*.spec.*' \
-g '!**/test/**' \
-g '!**/tests/**'
Length of output: 245
Script:
#!/bin/bash
# Let's try a different approach using ripgrep without file type filters
# but with explicit exclusions for test and build files
echo "Searching for console.log statements..."
rg "console\.log\(" \
--type-not test \
--type-not spec \
-g '!*.test.*' \
-g '!*.spec.*' \
-g '!**/test/**' \
-g '!**/tests/**' \
-g '!**/dist/**' \
-g '!**/build/**' \
-g '!**/node_modules/**' \
-g '!**/.git/**'
Length of output: 313
Script:
#!/bin/bash
# Simplify the search using basic ripgrep without type filters
# Just exclude common test and build directories
echo "Searching for console.log statements..."
rg "console\.log\(" \
--glob '!node_modules' \
--glob '!dist' \
--glob '!build' \
--glob '!.git' \
--glob '!test' \
--glob '!tests' \
--glob '!*.test.*' \
--glob '!*.spec.*'
Length of output: 1595
|
Smeller på en del checks der, ja :thinking_face: Kanskje kun ha "error" på det som kjøres på lint-staged, og ikke ha det i global config? f.eks. i Hva tenker du? |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
packages/frontend/src/auth/api.ts (1)
Line range hint
1-11: Consider implementing proper error handling.This is an authentication-related function where errors should be properly handled. Consider implementing proper error tracking/monitoring instead of just returning false silently.
Some suggestions:
- Implement error tracking (e.g., Sentry, Application Insights)
- Add error boundaries at the React component level
- Consider adding a notification system to inform users of authentication issues
Would you like me to help implement any of these suggestions?
packages/frontend/src/main.tsx (1)
48-48: Appropriate use of console.error, but error handling could be enhanced.Using
console.erroris correct here since this is a fatal error condition. However, consider enhancing the error handling to provide more context and better developer experience.Consider this improvement:
- console.error(`element with id "root" is not in DOM`); + const errorMessage = 'Failed to initialize application: DOM element with id "root" not found. ' + + 'Please ensure index.html contains <div id="root"></div>'; + console.error(errorMessage); + throw new Error(errorMessage);This change would:
- Provide more context about the error
- Include troubleshooting guidance
- Halt execution to prevent undefined behavior
packages/frontend/src/components/InboxItem/GuiActions.tsx (2)
Line range hint
53-58: Enhance error handling with user feedback.The current error handling only logs to console without providing any feedback to users. Consider implementing proper error handling with user notifications.
Here's a suggested improvement:
if (!response.ok) { - console.error(`Error: ${response.statusText}`); + const errorMessage = `Failed to perform action: ${response.statusText}`; + // Use your application's error handling utility/service + handleError(errorMessage); + // Show user feedback (assuming you have a notification system) + showErrorNotification(errorMessage); } } catch (error) { - console.error('Error performing action:', error); + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + handleError('Error performing action: ' + errorMessage); + showErrorNotification('Failed to perform action. Please try again later.'); }
Line range hint
6-8: Fix interface inconsistency between GuiActionProps and GuiActionsProps.There's an inconsistency in how actions are typed across the component:
GuiActionPropsexpects an array of actionsGuiActionsPropsexpects a single actionGuiActionscomponent maps over the actions array but passes single actions toGuiActionButtonThis needs to be unified. Here's a suggested fix:
export interface GuiActionProps { actions: GuiActionButtonProps[]; dialogToken: string; } interface GuiActionsProps { - actions: GuiActionButtonProps; + actions: GuiActionButtonProps[]; dialogToken: string; } // Update the GuiActionButton component signature -const GuiActionButton = ({ actions, dialogToken }: GuiActionsProps) +const GuiActionButton = ({ action, dialogToken }: { action: GuiActionButtonProps; dialogToken: string }) // Update the usage in GuiActions export const GuiActions = ({ actions, dialogToken }: GuiActionProps): JSX.Element => { return ( <section className={styles.guiActions} data-id="dialog-gui-actions"> {actions.map((actionProps) => ( - <GuiActionButton key={actionProps.id} actions={actionProps} dialogToken={dialogToken} /> + <GuiActionButton key={actionProps.id} action={actionProps} dialogToken={dialogToken} /> ))} </section> ); };Also applies to: 17-20, 121-127
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
.hooks/pre-push(1 hunks)biome.jsonc(1 hunks)package.json(1 hunks)packages/docusaurus-tldraw-plugin/src/tldr.jsx(1 hunks)packages/frontend/src/analytics.ts(1 hunks)packages/frontend/src/auth/api.ts(1 hunks)packages/frontend/src/components/FilterBar/index.ts(1 hunks)packages/frontend/src/components/InboxItem/GuiActions.tsx(1 hunks)packages/frontend/src/main.tsx(1 hunks)packages/storybook/src/stories/GuiActions/guiActions.stories.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- .hooks/pre-push
- packages/frontend/src/analytics.ts
- packages/frontend/src/components/FilterBar/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- biome.jsonc
- package.json
🔇 Additional comments (1)
packages/frontend/src/components/InboxItem/GuiActions.tsx (1)
54-54: Verify if console.error is allowed by the NoConsoleLog rule.
While the PR aims to implement the NoConsoleLog rule, this line introduces console.error for error logging. We should verify if this is intentionally allowed by the rule configuration.
Consider implementing a proper error logging service/utility instead of using console.error directly. This would:
- Provide consistent error handling across the application
- Enable better error tracking and monitoring
- Make it easier to switch logging implementations in the future
| @@ -6,7 +6,7 @@ export const getIsAuthenticated = async (): Promise<boolean> => { | |||
| }); | |||
| return response.ok; | |||
| } catch (error) { | |||
| console.log('error', error); | |||
| console.error('error', error); | |||
There was a problem hiding this comment.
Remove console.error as per NoConsoleLog rule.
While changing from console.log to console.error is more semantically correct for error logging, it still conflicts with the PR's objective of removing console logging via the NoConsoleLog rule.
- console.error('error', error);
+ // Consider implementing proper error tracking/monitoring
+ return false;Committable suggestion skipped: line range outside the PR's diff.
| @@ -10,7 +10,7 @@ export const Tldr = ({ children, height }) => { | |||
| ); | |||
|
|
|||
| if (err) { | |||
| console.log(result.err); | |||
| console.error(result.err); | |||
There was a problem hiding this comment.
Fix undefined variable reference
The code references result.err but the destructured variable is named err. This will cause a runtime error as result is undefined.
Apply this fix:
- console.error(result.err);
+ console.error(err);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error(result.err); | |
| console.error(err); |
| console.error(result.err); | ||
| return <div>Error loading tldr file</div>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling pattern
Given the PR's objective to remove console logging, consider implementing proper error handling:
- The error details would be lost once console logging is removed
- The generic error message "Error loading tldr file" provides little context for debugging
Consider this implementation:
- console.error(err);
- return <div>Error loading tldr file</div>;
+ return (
+ <div role="alert" className="tldr-error">
+ <h3>Failed to load TLDR file</h3>
+ <p>{err.message || 'An unexpected error occurred'}</p>
+ </div>
+ );This approach:
- Removes console logging as per the new linter rule
- Preserves error details for users
- Adds semantic HTML with proper ARIA attributes
- Can be styled appropriately using the
tldr-errorclass
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error(result.err); | |
| return <div>Error loading tldr file</div>; | |
| return ( | |
| <div role="alert" className="tldr-error"> | |
| <h3>Failed to load TLDR file</h3> | |
| <p>{err.message || 'An unexpected error occurred'}</p> | |
| </div> | |
| ); |
| @@ -60,7 +60,7 @@ Default.args = { | |||
| }, | |||
| ], | |||
| dialogToken: 'your-dialog-token', | |||
| onDeleteSuccess: () => console.log('Deleted'), | |||
| onDeleteSuccess: () => console.info('Deleted'), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the use of console.info as it doesn't align with the PR's intent
While replacing console.log with console.info technically bypasses the noConsoleLog rule, it doesn't align with the PR's objective of removing console logging. For Storybook stories, consider these better alternatives:
- Use Storybook's built-in action logger:
-onDeleteSuccess: () => console.info('Deleted'),
+onDeleteSuccess: action('onDeleteSuccess'),- Or use a mock callback that updates component state to demonstrate the behavior visually:
const DeleteCallback = () => {
const [deleted, setDeleted] = useState(false);
return (
<div>
<GuiActions
{...args}
onDeleteSuccess={() => setDeleted(true)}
/>
{deleted && <div>Item was deleted!</div>}
</div>
);
};Also applies to: 89-89, 106-106
Hva er endret?
Dokumentasjon / Storybook / testdekning
storiesiStorybook, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)
Summary by CodeRabbit
New Features
Chores
@biomejs/biomepackage to improve stability and performance.Bug Fixes
Refactor
Filtertype for better clarity in the codebase.GuiActionsPropsinterface to accept a single action instead of an array, streamlining the component's structure.