Open
Conversation
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to c6e95e3 in 1 minute and 22 seconds
More details
- Looked at
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. extensions/vscode/src/activation/activate.ts:111
- Draft comment:
Consider checking both global and workspace settings. Currently, only globalValue is checked. If a user has set a workspace-specific setting for sidebar location, it might be overlooked. - 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 comment makes a valid technical point - workspaceValue could indeed be set independently of globalValue. However, looking at the broader context, this code is trying to set a default sidebar location if the user hasn't explicitly set it anywhere. The third parameter 'true' in the update() call indicates this is explicitly setting it globally. The current approach of only checking globalValue seems intentional - if there's a workspace setting, we probably want to override it globally anyway.
I could be wrong about the intention - maybe we really do want to preserve workspace-specific settings. The code comment "If the user has not set the sidebar location" is ambiguous about whether it means globally or anywhere.
The code is clearly setting a global value (note the 'true' parameter), suggesting it intentionally wants to establish a global default regardless of workspace settings. The current implementation aligns with this goal.
The comment should be deleted. The current implementation appears intentional in only checking globalValue since it's setting a global default.
2. extensions/vscode/src/activation/activate.ts:113
- Draft comment:
Minor typographical suggestion: consider adding a comma in the comment, e.g. "If the user has not set the sidebar location, set it" to improve readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is an extremely minor stylistic suggestion about punctuation in a comment. It doesn't affect code functionality or even code quality in any meaningful way. According to the rules, we should not make purely informative comments or comments that are obvious/unimportant.
Perhaps clear and readable comments are important for maintainability, and this suggestion does technically improve readability slightly.
While readable comments are good, this change is too minor to warrant a PR comment. It's more noise than signal in the review process.
Delete this comment as it's too minor and doesn't meaningfully improve code quality.
3. extensions/vscode/src/activation/activate.ts:162
- Draft comment:
There is an inconsistency in naming: the comment on line 162 refers to 'PearAPP' while the rest of the file and project name use 'PearAI'. Please confirm and correct if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_nLiP3enspKP7SLYE
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.
| const sidebar = vscode.workspace.getConfiguration().inspect("workbench.sideBar.location"); | ||
|
|
||
| // If the user has not set the sidebar location set it | ||
| if (!sidebar?.globalValue) { |
There was a problem hiding this comment.
Consider checking both global and workspace values. Relying solely on globalValue can override a user’s workspace setting. For example, use if (!sidebar?.globalValue && !sidebar?.workspaceValue) to determine if no preference was set.
Suggested change
| if (!sidebar?.globalValue) { | |
| if (!sidebar?.globalValue && !sidebar?.workspaceValue) { |
This file contains hidden or 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
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 ✏️
In a recent change the sidebar location was being override on extension mount. This PR changes this logic to only set the sidebar location if the user has not set it already.
If the indented change was to force a "default layout" I would suggest creating some setting within the PearAI configuration, or include some flag that overrides this behavior.
Checklist ✅
Important
Modify
activateExtensioninactivate.tsto set sidebar location only if not user-defined.activate.ts,activateExtensionnow setsworkbench.sideBar.locationto 'left' only if not already set by the user.This description was created by
for c6e95e3. It will automatically update as commits are pushed.