[wip] Feat/lh/synchronous loading of vs code settings #249
Conversation
lenghuang
requested review from
nang-dev,
Fryingpannn and
a team
as code owners
…to feat/lh/SynchronousLoadingOfVsCodeSettings
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 299053b in 1 minute and 44 seconds
More details
- Looked at
1560lines of code in8files - Skipped
7files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. extensions/vscode/src/extension/VsCodeMessenger.ts:131
- Draft comment:
EnsureimportUserSettingsFromVSCodealways returns a boolean.
return !!val;
- Reason this comment was not posted:
Comment looked like it was already resolved.
2. extensions/vscode/src/commands.ts:294
- Draft comment:
EnsureimportUserSettingsFromVSCodereturns a boolean.
return !!(await importUserSettingsFromVSCode());
- Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_Tq3gEJx0IuSQYJvE
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.
| new Promise((resolve) => setTimeout(resolve, 3000)), // Take at least three seconds | ||
| copyVSCodeSettingsToPearAIDir(), | ||
| ]); | ||
| throw new Error("Test error message"); |
There was a problem hiding this comment.
Remove the test error throw in importUserSettingsFromVSCode to prevent it from always failing.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bf96fbf in 15 seconds
More details
- Looked at
63lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. extensions/vscode/src/copySettings.ts:120
- Draft comment:
The functioncopyDirectoryRecursiveSyncis called withawait, but its name suggests it might be synchronous. Consider renaming it to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%
ThecopyDirectoryRecursiveSyncfunction is called withawait, but its name suggests it might be synchronous. This could be misleading and should be clarified.
2. extensions/vscode/src/copySettings.ts:87
- Draft comment:
The functioncopyDirectoryRecursiveSyncis called withawait, but its name suggests it might be synchronous. Consider renaming it to avoid confusion. This also applies to the call on line 120. - Reason this comment was not posted:
Confidence changes required:50%
ThecopyDirectoryRecursiveSyncfunction is called withawait, but its name suggests it might be synchronous. This could be misleading and should be clarified.
Workflow ID: wflow_xfgLLWM2VFD6ytqr
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 0ddb7c5 in 15 seconds
More details
- Looked at
219lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. gui/src/inventory/pages/InventoryPage.tsx:1
- Draft comment:
Consider organizing import statements alphabetically for better readability. - Reason this comment was not posted:
Confidence changes required:10%
The import statements are not organized alphabetically, which is a common best practice for readability.
2. gui/src/pages/welcome/setup/ImportExtensions.tsx:24
- Draft comment:
Good addition of type checking forsettingsLoadedto ensure it's a boolean. - Reason this comment was not posted:
Confidence changes required:0%
The check forsettingsLoadedbeing a boolean is a good addition for type safety.
Workflow ID: wflow_VsA4HQWmTxyfTTnR
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 30724d1 in 17 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:25
- Draft comment:
Usingconsole.dirfor logging is not recommended in production code. Consider using a proper logging mechanism or removing it before deploying. - Reason this comment was not posted:
Confidence changes required:50%
The use of console.dir for logging is not ideal for production code.
2. gui/src/pages/welcome/setup/ImportExtensions.tsx:29
- Draft comment:
Usingconsole.dirfor logging is not recommended in production code. Consider using a proper logging mechanism or removing it before deploying. - Reason this comment was not posted:
Confidence changes required:50%
The use of console.dir for logging is not ideal for production code.
Workflow ID: wflow_0U4Z66vhmAeC7H5g
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 97280f7 in 34 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:28
- Draft comment:
TheisDonestate seems unnecessary asonNext()is called immediately after setting it to true, which likely changes the component state or unmounts it. Consider removingisDoneand usingisImportingto control the UI state. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is incorrect. While isDone is set right before onNext() in the success case, it's also set to false in the error case and is used to control meaningful UI state differences. isImporting and isDone serve different purposes - one shows loading state, the other shows completion state. Using isImporting instead would not correctly handle the error case UI.
Maybe there's a more elegant way to handle these states with a single enum/state variable instead of two booleans? The comment might be hinting at that.
While using an enum could be cleaner, the current implementation with two separate boolean states is perfectly valid and clear. The comment specifically suggests using isImporting which would be incorrect.
The comment should be deleted because it incorrectly suggests removing isDone state which is actually necessary for proper UI state management.
Workflow ID: wflow_KIaZOXRvlM3vonIC
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 0f3ebda in 35 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:32
- Draft comment:
Consider using a consistent key for localStorage operations. The keyimportUserSettingsFromVSCodeis used for both setting and checking import status, which can lead to confusion. It might be better to use a separate key for completion status, likeimportUserSettingsFromVSCodeCompleted. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests using a different key for completion status, but the code actually uses a single key consistently throughout. The change in the diff shows removal of "importUserSettingsFromVSCodeCompleted" in favor of the consistent "importUserSettingsFromVSCode" key. This appears to be an improvement in consistency, not a problem.
The comment might be pointing out a legitimate architectural concern about mixing status and completion flags in one key.
While separating concerns is generally good, the current implementation is consistent and simple. The comment is actually criticizing an improvement in consistency.
The comment should be deleted because it's criticizing a change that actually improves key consistency, and the current implementation is clear and consistent.
Workflow ID: wflow_tttowekd2GukdJAa
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 cb02313 in 13 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:21
- Draft comment:
Good practice to resetimportErrorbefore starting a new import process. - Reason this comment was not posted:
Confidence changes required:0%
The addition ofsetImportError("")is a good practice to reset the error state before starting a new import process.
Workflow ID: wflow_AmM9AMBcbIb17xwC
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 f1dac41 in 18 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:36
- Draft comment:
Ensure consistency in punctuation for user-facing messages. The added period is a good practice. - Reason this comment was not posted:
Confidence changes required:20%
The error message update is a minor change, but it is important to ensure consistency in punctuation for user-facing messages.
Workflow ID: wflow_iCg7izyePFmmXwXt
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 d78205d in 13 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. extensions/vscode/src/copySettings.ts:182
- Draft comment:
Remove the commented-out error throwing line if it's no longer needed to keep the code clean. - Reason this comment was not posted:
Confidence changes required:50%
The commented-out error throwing line should be removed if it's not needed, as commented-out code can clutter the codebase.
Workflow ID: wflow_QsCeqFXWuIduK8tr
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 f8bc880 in 15 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:92
- Draft comment:
Consider implementing the feature to hide the skip button for the first few seconds or remove the TODO comment if it's not going to be addressed in this PR. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comment suggests a feature that is not yet implemented. It would be better to implement this feature or remove the comment if it's not going to be addressed in this PR.
Workflow ID: wflow_x5FbD74sC2qckFqi
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 ea1ffc1 in 13 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. gui/src/pages/welcome/setup/ImportExtensions.tsx:92
- Draft comment:
Consider implementing the feature to hide the skip button for the first few seconds or remove the TODO comment if it's not going to be addressed in this PR. - Reason this comment was not posted:
Confidence changes required:50%
The TODO comment suggests a feature that is not yet implemented. It would be better to implement this feature or remove the comment if it's not going to be addressed in this PR.
Workflow ID: wflow_4YCVuzX2XrqwzQhk
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Hey @lenghuang, let's keep the linting/formatting changes in a separate pr, |
|
@Himanshu-Singh-Chauhan Hey Himanshu, totally agree. Closing this PR and working here instead: #250 |
Description ✏️
Closes #xxx
What changed? Feel free to be brief.
Checklist ✅
Important
Synchronously load VS Code settings into PearAI with updated commands and UI components to handle import success and errors.
importUserSettingsFromVSCodenow returns abooleanindicating success or failure inide.tsandideWebview.ts.ImportExtensions.tsx.pearai.welcome.importUserSettingsFromVSCodecommand incommands.tsto return a boolean.importUserSettingsFromVSCodeinVsCodeMessenger.ts.ImportExtensions.tsxto handle import state and errors.FinalStep.tsxandSetupPage.tsxto integrate the import process.commands.tsandVsCodeMessenger.ts.This description was created by
for ea1ffc1. It will automatically update as commits are pushed.