build: migrate to biome #41
Conversation
…config.js there's an issue in the release ci where the env vars aren't found by the build. opting for hardcoding as the values are public and this way is more straightforward.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughThis pull request involves the removal of several configuration files, including Changes
Possibly related PRs
Suggested reviewers
🪧 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range comments (5)
src/lib/cardinal-provider.tsx (1)
Line range hint
1-150: Consider enhancing error handling and type safetyWhile the overall structure of the file is well-organized, consider the following improvements:
Error Handling: Enhance error handling in the
pingfunction. Instead of just logging errors, consider implementing a more robust error handling strategy.Type Safety: Use more specific types instead of
stringfor theviewproperty. Consider using a union type like'card' | 'list'.Configuration Management: The current approach of managing configuration through local storage is functional, but consider using a more robust state management solution for larger applications.
Environment Variables: Instead of using
__CARDINAL_PROJECT_ID__, consider using environment variables for project-specific constants.Would you like assistance in implementing any of these suggestions?
src/routes/__root.tsx (1)
Line range hint
40-101: Approved: useEffect dependency array update and linter ignore comment.The changes improve the correctness of the useEffect hook by including all used variables in the dependency array. The added comment explains why
personasis intentionally left out.Consider moving the ignore comment to the line where
personasis used to make it more specific:// Inside the useEffect const newPersonas = personas.filter((p) => { // biome-ignore lint: we don't want to rerun whenever `personas` changes // ... rest of the code });This change would make the ignored lint rule more targeted and easier to understand in the future.
src/components/sidebar/messages.tsx (3)
Line range hint
45-173: Consider refactoring the Message component for improved maintainability.While not directly related to the current changes, the
Messagecomponent is quite large and handles multiple responsibilities. Consider breaking it down into smaller, more focused components. This would improve readability and maintainability.For example, you could extract the form rendering logic into a separate component:
function MessageForm({ message, onSubmit }) { // Form logic here }And use it in the
Messagecomponent:function Message({ message, namespace }: MessageProp) { // ... other logic ... return ( <AccordionItem> {/* ... */} <AccordionContent> <MessageForm message={message} onSubmit={handleSubmit} /> </AccordionContent> </AccordionItem> ); }
Line range hint
86-98: Add error handling for when no personas are available.The current implementation disables the persona selection when no personas are available, but it doesn't provide any guidance to the user on how to proceed. Consider adding an error message or a link to create a persona when none are available.
Example:
{personas.length === 0 ? ( <FormItem> <FormLabel>Persona tag</FormLabel> <p className="text-sm text-muted-foreground">No personas available. <a href="/create-persona" className="text-primary">Create one here</a>.</p> </FormItem> ) : ( <FormField control={form.control} name="persona" render={({ field }) => ( // ... existing Select component ... )} /> )}
Line range hint
45-173: Consider extracting form submission logic into a custom hook.The form submission logic in the
handleSubmitfunction could be extracted into a custom hook for better reusability and separation of concerns. This would make the component cleaner and easier to test.Example:
function useMessageSubmission(message, namespace) { const { cardinalUrl, isCardinalConnected, personas, setPersonas } = useCardinal(); const queryClient = useQueryClient(); const { toast } = useToast(); return async (values: ComponentProperty) => { // ... existing submission logic ... }; } function Message({ message, namespace }: MessageProp) { const handleSubmit = useMessageSubmission(message, namespace); // ... rest of the component ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
- .env.example (0 hunks)
- .eslintrc.cjs (0 hunks)
- .github/workflows/build.yaml (0 hunks)
- .prettierignore (0 hunks)
- .prettierrc (0 hunks)
- biome.json (1 hunks)
- package.json (2 hunks)
- src/components/sidebar/messages.tsx (1 hunks)
- src/components/ui/badge.tsx (1 hunks)
- src/components/ui/button.tsx (1 hunks)
- src/components/ui/label.tsx (1 hunks)
- src/components/ui/sheet.tsx (1 hunks)
- src/components/ui/toast.tsx (1 hunks)
- src/index.css (2 hunks)
- src/lib/cardinal-provider.tsx (1 hunks)
- src/main.tsx (2 hunks)
- src/routes/__root.tsx (5 hunks)
- src/vite-env.d.ts (1 hunks)
- vite.config.ts (2 hunks)
💤 Files with no reviewable changes (5)
- .env.example
- .eslintrc.cjs
- .github/workflows/build.yaml
- .prettierignore
- .prettierrc
🧰 Additional context used
🪛 Gitleaks
vite.config.ts
31-31: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (23)
src/vite-env.d.ts (1)
3-4: LGTM! Constants added as per PR objectives.The addition of
__SENTRY_DSN__and__POSTHOG_KEY__constants is consistent with the PR's goal of migrating to hardcoded constants for Sentry DSN and Posthog key. The declarations follow the same naming convention and type as the existing__CARDINAL_PROJECT_ID__constant.Let's verify the usage of these new constants in the codebase:
This will help ensure that the new constants are being used as intended throughout the project.
✅ Verification successful
Verified! The
__SENTRY_DSN__and__POSTHOG_KEY__constants are properly declared and utilized within the codebase, aligning with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of the new constants in the codebase echo "Checking usage of __SENTRY_DSN__:" rg "__SENTRY_DSN__" --type ts --type tsx echo "\nChecking usage of __POSTHOG_KEY__:" rg "__POSTHOG_KEY__" --type ts --type tsxLength of output: 298
Script:
#!/bin/bash # Description: Check for the usage of the new constants in the codebase echo "Checking usage of __SENTRY_DSN__:" rg "__SENTRY_DSN__" --type typescript --type typescriptreact echo "\nChecking usage of __POSTHOG_KEY__:" rg "__POSTHOG_KEY__" --type typescript --type typescriptreactLength of output: 352
Script:
#!/bin/bash # Description: Check for the usage of the new constants in the codebase echo "Checking usage of __SENTRY_DSN__:" rg "__SENTRY_DSN__" -g "*.ts" -g "*.tsx" echo "\nChecking usage of __POSTHOG_KEY__:" rg "__POSTHOG_KEY__" -g "*.ts" -g "*.tsx"Length of output: 654
src/components/ui/label.tsx (1)
2-2: LGTM: Import order change likely due to Biome.The change in the import statement order is minor and doesn't affect the functionality of the code. This modification is likely a result of automatic import sorting by the new linter (Biome) mentioned in the PR objectives.
src/components/ui/badge.tsx (2)
1-1: LGTM: Import order change likely due to Biome migration.The change in the import statement order is minor and doesn't affect the functionality. This modification is likely a result of the migration to Biome for formatting, as mentioned in the PR objectives.
Line range hint
1-37: File consistency maintained after import change.The modification to the import statement order does not impact the rest of the file. Both
VariantPropsandcvaare correctly used throughout the component, and no further changes are necessary.vite.config.ts (2)
1-1: LGTM: Improved import statementThe change to use
import path from 'node:path'is a good practice. It explicitly indicates that this is a Node.js built-in module, improving code clarity and following Node.js recommendations.
31-31: Verify the public nature of the PostHog keyA static analysis tool has flagged the PostHog key as a potential security risk. While the comment suggests this key is intentionally public, it's crucial to verify this and understand any associated risks.
Please confirm:
- Is this PostHog key truly intended to be public?
- Are there any risks associated with exposing this key in the source code?
- Does PostHog have any guidelines or best practices for handling their API keys?
Run the following script to check if this key is used elsewhere in the project:
If this key is confirmed to be public and safe to expose, consider adding a comment explicitly stating this to prevent future concerns.
✅ Verification successful
PostHog Key Exposure Verified
The PostHog key appears solely in
vite.config.tsand is not found elsewhere in the project, mitigating the risk of unintended exposure. If this key is intended to be public, consider adding an explicit comment to clarify its purpose and prevent future concerns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of PostHog key across the project # Test: Search for the PostHog key. Expect: Only this occurrence. rg --type-not json "phc_nkoe1cRzoBD3JUNX7ZKFC1M9wUV189UMc9ZhXAq56ts"Length of output: 157
🧰 Tools
🪛 Gitleaks
31-31: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
package.json (2)
Line range hint
1-56: Overall LGTM! Verify the migration impact.The changes in
package.jsonaccurately reflect the migration from ESLint and Prettier to Biome. The scripts have been updated appropriately, and the necessary dependencies have been added/removed.To ensure a smooth transition, please run the following verification script:
#!/bin/bash # Description: Verify the impact of migrating to Biome # Test: Check if any ESLint or Prettier config files still exist config_files=(".eslintrc.js" ".eslintrc.cjs" ".eslintrc.json" ".prettierrc" ".prettierrc.js" ".prettierrc.json") for file in "${config_files[@]}"; do if [ -f "$file" ]; then echo "Warning: $file still exists and should be removed" fi done # Test: Check for any remaining ESLint or Prettier dependencies if grep -qE "eslint|prettier" package.json; then echo "Warning: ESLint or Prettier related dependencies found in package.json" fi # Test: Verify that Biome runs without errors npm run lint npm run format echo "Verification complete. Please review the output and address any warnings."
10-11: LGTM! Verify Biome configuration.The changes to the
lintandformatscripts correctly reflect the migration from ESLint and Prettier to Biome. This is in line with the PR objectives.To ensure that Biome is correctly configured, please run the following script:
src/components/ui/button.tsx (2)
2-2: LGTM: Import order change is stylistic.The reordering of imports from
class-variance-authorityis a minor stylistic change that doesn't affect functionality. The new order alphabetizes the imports, which is a common convention and improves readability.
Line range hint
1-54: File is consistent with the summary and changes are approved.I've reviewed the entire file, and it's consistent with the AI-generated summary. The only change is the import order on line 2, which doesn't affect the functionality of the
Buttoncomponent or any other part of the file. The component structure, props, and logic remain unchanged.src/index.css (2)
6-10: Improved quote consistency in font-face declaration.The change from single quotes to double quotes in the
font-familyname andsrcURLs aligns with common CSS style guides. This improves code consistency without affecting functionality.
87-87: Consistent quoting in font-family declaration.The change to double quotes for font names with spaces in the
font-familyproperty is correct and consistent with earlier changes. The font stack remains well-structured, maintaining the intended fallback order.src/main.tsx (1)
30-30: Consider the security and flexibility implications of hardcoding the Sentry DSN.While using a global constant (
__SENTRY_DSN__) simplifies the build process, it may introduce security and flexibility concerns:
- Security: Hardcoding the DSN in the source code could potentially expose it if the repository is made public.
- Flexibility: This approach makes it more challenging to manage different configurations for various environments (development, staging, production).
Consider using a build-time injection method that keeps the DSN out of the source code while still avoiding runtime environment variables. This could involve using a build script to inject the value or utilizing Vite's define feature for compile-time constants.
To ensure the DSN is not exposed in the public repository, run the following script:
#!/bin/bash # Description: Check if Sentry DSN is exposed in the repository # Test: Search for Sentry DSN patterns in the codebase rg --type-add 'config:*.{js,ts,json}' --type config 'dsn.*[0-9a-f]{32}'If this script returns any results, it indicates that the Sentry DSN might be exposed in the codebase.
src/components/ui/sheet.tsx (1)
2-2: LGTM: Import order change is acceptable.The change in the import order for
cvaandVariantPropsfrom 'class-variance-authority' is likely a result of the migration to Biome for linting and formatting. This change doesn't affect the functionality and aligns with common practices of sorting imports alphabetically.src/components/ui/toast.tsx (4)
2-2: LGTM: Import order change.The change in the order of imports from 'class-variance-authority' is acceptable. This modification likely results from the migration to Biome for linting and formatting, as mentioned in the PR objectives.
Line range hint
46-59: Excellent enhancement to the Toast component.The addition of
VariantProps<typeof toastVariants>to theToastcomponent's props is a valuable improvement. This change:
- Enhances type safety by ensuring that only valid variant props can be passed to the component.
- Improves the component's flexibility by allowing it to accept different style variants.
- Aligns well with the
toastVariantsfunction defined earlier in the file.The implementation correctly uses the
variantprop in thecnfunction call, ensuring that the appropriate styles are applied based on the variant.
Line range hint
118-120: Beneficial addition of exported types.The export of
ToastPropsandToastActionElementtypes is a valuable addition:
ToastProps(type alias forReact.ComponentPropsWithoutRef<typeof Toast>) allows other components to easily use and extend the props of theToastcomponent.ToastActionElement(type alias forReact.ReactElement<typeof ToastAction>) provides a specific type for the action element within a toast, improving type safety when working with toast actions.These exports enhance the usability of the Toast components throughout the application, improving type safety and developer experience.
Also applies to: 124-125
Line range hint
1-133: Overall assessment: Excellent improvements to the Toast component system.The changes in this file align well with the PR objectives of migrating to Biome and enhancing the codebase. The modifications to the
Toastcomponent and the addition of exported types improve type safety, component flexibility, and developer experience. These changes are well-implemented and maintain the existing functionality while providing better tooling support.biome.json (2)
5-16: Formatter configuration looks good.The formatter settings are well-configured with reasonable defaults. Ignoring the lock file and generated route tree is a good practice.
1-149: Overall, the Biome configuration is well-structured and comprehensive.The migration to Biome appears to be thorough, with a detailed configuration covering formatting and linting aspects. The suggested improvements aim to enhance type safety, consistency, and error detection. Consider implementing these changes to further refine your development workflow.
src/routes/__root.tsx (2)
3-3: LGTM: Import order change.The change in import order doesn't affect functionality and is likely due to automatic import sorting by Biome.
Line range hint
103-130: Review needed: useEffect dependency array change and error handling.
Dependency array change:
The removal ofisCardinalConnectedfrom the dependency array might lead to missed updates if its value changes. Please verify if this was intentional and if the effect should indeed not re-run whenisCardinalConnectedchanges.Error handling:
The renaming oferrorto_errorin the catch block is a good practice to indicate that the error is caught but not used. However, consider logging this error for debugging purposes:} catch (_error) { console.error('Error parsing event data:', _error); // ... rest of the code }To ensure
isCardinalConnectedis not used within the effect, run:If the pattern is not found, the removal from the dependency array is likely correct.
✅ Verification successful
Verified: The removal of
isCardinalConnectedfrom the dependency array is correct, as it is no longer used within theuseEffect. Additionally, the renaming oferrorto_erroris appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
ast-grep --lang typescript --pattern $'useEffect(() => { $$$ $isCardinalConnected $$$ }, [$$$])'Length of output: 103
src/components/sidebar/messages.tsx (1)
3-3: LGTM: Import order change likely due to Biome migration.The change in the import order for
MessageSquareCodeis minor and doesn't affect the functionality. This reordering is likely a result of the migration to Biome for linting and formatting, possibly enforcing alphabetical ordering of imports.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/routes/__root.tsx (5 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/routes/__root.tsx (3)
3-3: LGTM: Import order change.The change in import order doesn't affect functionality and is likely due to automatic import sorting by Biome.
Line range hint
40-101: Improved useEffect dependencies and added lint ignore comment.The changes in this useEffect hook are beneficial:
- The lint ignore comment prevents unnecessary reruns when
personaschanges.- Adding
queryClientto the dependency array ensures the effect updates when the query client changes, improving the hook's correctness.These modifications enhance the component's behavior and maintainability.
Line range hint
103-130: Review dependency array and error handling changes.
The renaming of
errorto_errorin the catch block is a good practice, indicating that the error is intentionally unused.However, removing
isCardinalConnectedfrom the dependency array might lead to missed updates if this value changes. Can you confirm if this was intentional? If so, please explain the reasoning behind this change.To verify the usage of
isCardinalConnected, please run the following script:If the script returns no results, it suggests that
isCardinalConnectedis not used within the effect, justifying its removal from the dependency array.
Closes: WORLD-1194
Overview
This PR introduces changes to how buildtime constants are handled and a migration to using Biome for formatting and linting from Eslint and Prettier.
TODO:
Brief Changelog
Sentry DSN and Posthog key
Testing and Verifying
Manually tested and verified
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
pingfunction of the CardinalProvider.Documentation
Chores