-
Notifications
You must be signed in to change notification settings - Fork 58
Set sidebar location when no preference #288
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
base: main
Are you sure you want to change the base?
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.
❌ Changes requested. Reviewed everything up to c6e95e3 in 1 minute and 22 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (!sidebar?.globalValue) { | |
if (!sidebar?.globalValue && !sidebar?.workspaceValue) { |
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
activateExtension
inactivate.ts
to set sidebar location only if not user-defined.activate.ts
,activateExtension
now setsworkbench.sideBar.location
to 'left' only if not already set by the user.This description was created by
for c6e95e3. It will automatically update as commits are pushed.