-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Added ESLint for linting service as a Toolcall for linting the output #1799
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
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to 31de4f0 in 2 minutes and 5 seconds. Click for details.
- Reviewed
258
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/studio/electron/main/create/index.ts:69
- Draft comment:
Consider adding error handling if linting fails on generated page. The catch in lintAndFix logs error, but user feedback might be beneficial. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. apps/studio/electron/main/create/index.ts:227
- Draft comment:
Consider checking for undefined instead of falsey when using lintResult.output, as an empty string output might be valid. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a valid concern - using || could indeed cause issues if lintResult.output is an empty string, as that would be treated as falsy and the original content would be used instead. However, we need to consider if this is actually a real issue in practice. Looking at the context, this is a linting service output - would it ever return an empty string as valid output? Most linting services would either return null/undefined for no changes or the actual content. I don't have access to the LintingService implementation or its API contract. The output type could be designed to never return empty strings. While technically correct, without seeing the LintingService implementation, this feels like a speculative edge case that may never occur in practice. Delete the comment as it's speculating about an edge case without strong evidence that it's actually an issue in practice.
3. packages/foundation/src/linting/index.ts:105
- Draft comment:
Consider verifying that result.fixed status and output handling correctly distinguish between an empty output and a no-output scenario. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment suggests verifying the handling ofresult.fixed
status and output, which is a request for confirmation or verification. This violates the rule against asking the PR author to confirm or ensure behavior. The comment does not provide a specific code suggestion or point out a specific issue, making it not useful according to the guidelines.
4. apps/studio/electron/main/create/index.ts:69
- Draft comment:
Double linting: The generated page is linted in applyGeneratedPage and again via lintProject. If this redundancy is intentional, please document the rationale. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The two linting operations serve different purposes - one is focused on fixing the generated content before saving, while the other ensures the entire project is consistent after all changes. This is actually a reasonable pattern - fix issues in new content first, then verify the whole project. The comment suggests documenting this but documentation isn't strictly necessary since the code is clear and the pattern makes sense. I could be wrong about whether this pattern is intentional - maybe it really is accidental duplication. And even if intentional, documentation could still be valuable for future maintainers. The code structure and comments clearly show the two different linting purposes. The first lint fixes generated content, the second verifies project-wide consistency. This is a logical pattern that doesn't require additional documentation. The comment should be deleted because it questions a reasonable code pattern and requests unnecessary documentation. The code is clear as-is.
5. packages/foundation/src/linting/index.ts:103
- Draft comment:
Avoid double type casting with 'as unknown as ESLintResult'. Consider using proper types or generics to improve type safety. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The double casting is likely necessary here due to ESLint's type definitions not matching exactly with the custom type. The ESLint.lintFiles() method returns a type that needs to be transformed. Using a single cast might not work due to TypeScript's type system rules. This seems like a necessary evil rather than a problem to fix. The comment might have a point about type safety - there could be a way to properly type this using generics or by extending ESLint's types. We might be missing a better typing solution. While better typing might be possible, the current solution is a common pattern when dealing with external libraries whose types don't exactly match our needs. The risk of type mismatch is mitigated by the explicit type definition. This comment should be deleted as the double casting appears to be a necessary workaround when dealing with ESLint's types, and the suggested alternatives might not actually improve type safety in this case.
Workflow ID: wflow_tE6piaI2YMylN6Z7
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@Kitenite can you review the changes and tell if any modifications or changes needed |
Hello, I am migrating the desktop app to a new repository. This repository will now focus on the web version. This PR has been migrated to the new repository: onlook-dev#12. Sorry for the inconvenience, rest assure your work here will also be adapted for the web version (if it hasn't already). |
Description
Added ESLint for linting the output of the llm response
Related Issues
fixes #1570
Type of Change
Added a file for linting script in the packages/foundation/src/linting
Important
Adds ESLint integration for linting and auto-fixing LLM-generated project files using a new
LintingService
.LintingService
inindex.ts
to lint and fix generated project files.eslint
,@typescript-eslint/parser
,@typescript-eslint/eslint-plugin
,eslint-plugin-react
, andeslint-plugin-react-hooks
topackage.json
.LintingService
class inindex.ts
to perform linting and auto-fixing using ESLint.lintAndFix()
andlintProject()
methods for file and project-level linting.This description was created by
for 31de4f0. You can customize this summary. It will automatically update as commits are pushed.