-
Notifications
You must be signed in to change notification settings - Fork 1
chore: cherry-pick codebase cleanup
#35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: cherry-pick codebase cleanup
#35
Conversation
- Add ESLint configuration with TypeScript support - Remove unused variables, imports, and parameters - Fix TypeScript any types and improve type safety - Update preflight script to include linting - Fix test script to run in CI mode - Improve code quality and maintainability
|
@bniladridas is attempting to deploy a commit to the harpertoken Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @bniladridas, 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 focuses on a comprehensive codebase cleanup, re-applying previous linting and formatting changes to enhance code quality and maintainability. Key updates include the migration to a new, modern ESLint configuration with robust TypeScript support, refactoring of components to eliminate unused code, and improvements to type safety. Additionally, configuration files have been modernized, and dependency declarations have been streamlined. Highlights
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
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a number of cleanup and linting changes across the codebase. The introduction of ESLint is a great step towards maintaining code quality. I've found a few issues, including a critical bug in the server's error handling middleware that needs to be addressed. There are also some smaller improvements that can be made to enhance type safety and fix some misplaced comments. Overall, good progress on cleaning up the codebase.
apps/server/index.ts
Outdated
| configuredApp.use((err: any, _req: Request, res: Response) => { | ||
| const status = err.status || err.statusCode || 500; | ||
| const message = err.message || 'Internal Server Error'; | ||
|
|
||
| res.status(status).json({ message }); | ||
| } | ||
| ); | ||
| res.status(status).json({ message }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error handling middleware has an incorrect signature. In Express, an error handling middleware must have four arguments: (err, req, res, next). By removing the next parameter, this function will be treated as a regular middleware and will not be called by Express to handle errors. This will cause errors to be unhandled.
To fix this, you must add the next parameter back, even if it's unused. You will also need to re-import NextFunction from express.
| configuredApp.use((err: any, _req: Request, res: Response) => { | |
| const status = err.status || err.statusCode || 500; | |
| const message = err.message || 'Internal Server Error'; | |
| res.status(status).json({ message }); | |
| } | |
| ); | |
| res.status(status).json({ message }); | |
| }); | |
| configuredApp.use((err: any, _req: Request, res: Response, _next: NextFunction) => { | |
| const status = err.status || err.statusCode || 500; | |
| const message = err.message || 'Internal Server Error'; | |
| res.status(status).json({ message }); | |
| }); |
|
|
||
| console.log('Generated Gemini suggestion:', suggestion); | ||
| return suggestion.startsWith(' ') ? suggestion : ' ' + suggestion; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| console.log('Generated Gemini suggestion:', suggestion); | ||
| return suggestion.startsWith(' ') ? suggestion : ' ' + suggestion; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| timeoutRef.current = setTimeout(() => { | ||
| generateSuggestion(newText); | ||
| }, 500); | ||
| }, 500) as unknown as number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type cast as unknown as number is used here to align the return type of setTimeout with the timeoutRef. While this works, it's a bit of a code smell and can be avoided. Using window.setTimeout explicitly tells TypeScript to use the browser's implementation, which correctly returns a number, removing the need for a cast and making the code cleaner and more type-safe.
| timeoutRef.current = setTimeout(() => { | |
| generateSuggestion(newText); | |
| }, 500); | |
| }, 500) as unknown as number; | |
| timeoutRef.current = window.setTimeout(() => { | |
| generateSuggestion(newText); | |
| }, 500); |
| action?: ToastActionElement; | ||
| }; | ||
|
|
||
| const actionTypes = { | ||
| ADD_TOAST: 'ADD_TOAST', | ||
| UPDATE_TOAST: 'UPDATE_TOAST', | ||
| DISMISS_TOAST: 'DISMISS_TOAST', | ||
| REMOVE_TOAST: 'REMOVE_TOAST', | ||
| } as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actionTypes constant object has been removed. While this might seem like a cleanup, using a constant object for action types is a common and recommended pattern in Redux-style state management. It helps prevent typos by providing a single source of truth for action type strings and allows for autocompletion in IDEs. Reverting to magic strings (e.g., "ADD_TOAST") directly in the code can make it more error-prone and harder to maintain. Consider restoring the actionTypes constant object.
codebase cleanup
|
Deployment failed with the following error: Learn More: https://vercel.com/harpertoken?upgradeToPro=build-rate-limit |
|
Deployment failed with the following error: Learn More: https://vercel.com/harpertoken?upgradeToPro=build-rate-limit |
|
🎉 This PR is included in version 1.4.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Re-applying the linting and cleanup changes from commit 662e69e.