Wait for VSCode Settings Import to finish before proceeding to next step #250
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lenghuang
requested review from
nang-dev,
Fryingpannn and
a team
as code owners
3 tasks
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e5d829c in 1 minute and 38 seconds
More details
- Looked at
598lines of code in8files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. .prettierignore:4
- Draft comment:
Remove temporary lines before merging to avoid unintended formatting issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is directly addressing changes in the diff. The lines are explicitly marked as temporary in the code itself with "do not merge with this" comments. The comment is essentially redundant since the code already makes this clear. Additionally, this is a .prettierignore file which is a development tool configuration, not application logic.
The comment could be helpful as an extra reminder to prevent accidental merging of temporary changes. Development tool configurations can affect code quality.
While true, the code itself already contains clear "do not merge" comments. An additional PR comment doesn't add meaningful value and could be considered noise.
Delete the comment since it's redundant with the explicit "do not merge" comments in the code itself.
2. core/protocol/ide.ts:94
- Draft comment:
Ensure that the change in return type forimportUserSettingsFromVSCodeis consistently applied across all usages to prevent type errors. - Reason this comment was not posted:
Comment did not seem useful.
3. core/protocol/ideWebview.ts:26
- Draft comment:
Ensure consistent use of semicolons or commas in object type definitions for clarity and to prevent potential errors. - Reason this comment was not posted:
Comment did not seem useful.
4. extensions/vscode/src/commands.ts:262
- Draft comment:
Consider handling the boolean return value ofimportUserSettingsFromVSCodeto manage success or failure scenarios effectively. - Reason this comment was not posted:
Marked as duplicate.
5. extensions/vscode/src/copySettings.ts:142
- Draft comment:
Re-evaluate the necessity of the 1-second delay incopyVSCodeSettingsToPearAIDirto optimize performance. - Reason this comment was not posted:
Confidence changes required:50%
IncopySettings.ts, thecopyVSCodeSettingsToPearAIDirfunction has a hardcoded delay of 1 second. This could lead to unnecessary waiting time if the operation completes faster.
6. extensions/vscode/src/extension/VsCodeMessenger.ts:110
- Draft comment:
Utilize the boolean return value ofimportUserSettingsFromVSCodeto handle success or failure scenarios appropriately. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_dEjVeX7IYumqd8ue
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3e98ca7 in 13 seconds
More details
- Looked at
75lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:62
- Draft comment:
Use double quotes for strings to maintain consistency with the rest of the codebase. - Reason this comment was not posted:
Confidence changes required:50%
The use of single quotes for strings is inconsistent with the rest of the codebase, which uses double quotes. This should be standardized for consistency.
2. gui/src/pages/welcome/setup/ImportExtensions.tsx:83
- Draft comment:
Consider implementing the TODO to hide the skip button for the first five seconds to improve user experience. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comment suggests hiding the skip button for the first five seconds, but this is not implemented. This could be a potential UX improvement.
Workflow ID: wflow_1Ugwui0kg2vu6C06
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f00f34a in 17 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:45
- Draft comment:
Consider adding 'onNext' to the dependency array of the useEffect hook to ensure it captures the latest reference. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_CzmjkJiJ9121AAXi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 04f6d6c in 46 seconds
More details
- Looked at
65lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_UleVrwOjYXvHSYR9
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
…to feat/lh/SyncLoadingOfVsCodeSettingsWithoutPrettier
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2e5c904 in 16 seconds
More details
- Looked at
77lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. extensions/vscode/package.json:186
- Draft comment:
The PR description mentions syncing VS Code settings without Prettier, but the code changes don't seem to address this specific requirement. Ensure that the changes align with the PR's intent. - Reason this comment was not posted:
Comment did not seem useful.
2. gui/src/pages/welcome/setup/ImportExtensions.tsx:27
- Draft comment:
ThesetTimeoutlogic for showing the skip button is duplicated. Consider consolidating this logic to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
ThehandleImportfunction usessetTimeoutto show the skip button after 3 seconds. This logic is duplicated in theuseEffectcleanup function, which is removed. Ensure consistency in handling the skip button visibility.
Workflow ID: wflow_CSDZBVvwdtSZoVss
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 53484bc in 1 minute and 43 seconds
More details
- Looked at
130lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. extensions/vscode/src/extension/VsCodeMessenger.ts:109
- Draft comment:
Consider retaining the type check for 'val' to ensure robustness. Returning 'false' if 'val' is not a boolean can prevent unexpected behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The command being called is "pearai.welcome.importUserSettingsFromVSCode" which is likely a custom command. Since this is a custom command, the implementer would know what type it returns. The try/catch block already handles errors and returns false in that case. The type check seems overly defensive and adds complexity without clear benefit.
The comment raises a valid point about type safety. What if the command implementation changes in the future to return a non-boolean value?
The command is part of the same codebase and its contract should be maintained through normal code review processes. Adding defensive type checks everywhere creates unnecessary complexity. The error case is already handled.
The comment should be deleted. The type check was unnecessary defensive programming that added complexity without clear benefit, and error cases are already handled by the try/catch block.
2. gui/src/pages/gui.tsx:300
- Draft comment:
The removal of 'localStorage.removeItem("importUserSettingsFromVSCode");' is consistent with the changes in ImportExtensions.tsx where this item is no longer used. - Reason this comment was not posted:
Confidence changes required:20%
The removal of the localStorage item 'importUserSettingsFromVSCode' is consistent with the changes in the ImportExtensions component, where this item is no longer used.
3. gui/src/pages/welcome/setup/ImportExtensions.tsx:36
- Draft comment:
Ensure 'isDoneImportingUserSettingsFromVSCode' is consistently used across the codebase to maintain state across sessions. - Reason this comment was not posted:
Confidence changes required:20%
The use of localStorage for 'isDoneImportingUserSettingsFromVSCode' is consistent across the codebase, ensuring the state is preserved across sessions.
Workflow ID: wflow_pgs3xDTpmXg5gAms
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
lenghuang
changed the title
lenghuang
commented
Jan 28, 2025
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3307be3 in 20 seconds
More details
- Looked at
24lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_MFbdfzZczjgb0QOW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
lenghuang
changed the title
nang-dev
reviewed
Jan 29, 2025
nang-dev
reviewed
Jan 29, 2025
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fdc0746 in 11 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .prettierignore:3
- Draft comment:
Ensure that the removal of these lines is intentional and that no local development settings are needed in the main codebase. - Reason this comment was not posted:
Confidence changes required:50%
The changes in the .prettierignore file seem to be removing temporary local dev settings. This is a good practice to ensure that local development settings do not interfere with the main codebase.
Workflow ID: wflow_sgGdK45ghl5KKZr3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
nang-dev
pushed a commit
that referenced
this pull request
Feb 4, 2025
…tep (#250) * copy over what i have from other branch but without less formatting changes * temp * undo some formatting changes * undo some more formatting * only show skip button after 3 seconds * move idemessenger helper back and tweak skip timer not showing up * check is done on local storage * remove comments * remove prettier ignore changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description ✏️
Closes trypear/pearai-app#170
Initially, I got some permissions error when copying.
Then I ran these commands to get it working.
Here we see that it automatically goes to the next step on success, and that if you were to go back for some reason, the "done" status stays there.
Testing Details 🧪
darwin-x86https://drive.google.com/file/d/1YHUXNZfW5ZyGmYxcjCsRKvlqoEmnhvvC/view?usp=sharingv22.13.1for some reason. If not,./scripts/install-and-build.shwould not succeedExtension (VSCode)startOnboardingsince my local was slow and sometimesReset Onboardingwasn't enough.Checklist ✅
Important
Make VSCode settings import synchronous with error handling and retry logic, and update UI to reflect import status.
importUserSettingsFromVSCode()incopySettings.ts.ImportExtensions.tsx.ImportExtensions.tsx.ImportExtensions.tsx.pearai.startOnboardingcommand inpackage.jsonandcommands.ts.pearai.welcome.importUserSettingsFromVSCodeto return a boolean incommands.ts.isDoneImportingUserSettingsFromVSCodetolocalStorage.ts.FinalStep.tsxandImportExtensions.tsxto handle import completion and errors.ImportExtensions.tsx.This description was created by
for fdc0746. It will automatically update as commits are pushed.