v1.8.0: Inventory #182
Merged
v1.8.0: Inventory #182
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
Himanshu-Singh-Chauhan
requested review from
nang-dev,
Fryingpannn and
a team
as code owners
![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 5f611d6 in 2 minutes and 0 seconds
More details
- Looked at
123lines of code in2files - Skipped
1files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/vs/workbench/browser/parts/paneCompositePart.ts:421
- Draft comment:
Ensure the insertion order of the icon group relative to the composite bar container is as intended. The icon group is prepended after the composite bar container is added; verify desired layout. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify the layout, which is against the rules as it asks for confirmation of intention. It does not provide a specific suggestion or point out a clear issue.
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:55
- Draft comment:
Duplicate 'border-radius' declaration in .icon-group. The rule first sets it to 6px then to 10px. Remove the redundant declaration to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:70
- Draft comment:
Duplicate 'border-radius' in .codicon. It’s first set to 4px then later to 6px. Consolidate these properties to the intended value. - Reason this comment was not posted:
Marked as duplicate.
4. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:75
- Draft comment:
The 'fill' property is assigned a linear-gradient value, which is not valid. Gradients aren’t acceptable for the fill property; consider using a solid color or a proper SVG gradient. - Reason this comment was not posted:
Marked as duplicate.
5. src/vs/workbench/browser/parts/paneCompositePart.ts:421
- Draft comment:
Consider extracting the icon group creation code into a helper function to improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. src/vs/workbench/browser/parts/paneCompositePart.ts:422
- Draft comment:
The hardcoded icon names array (['bell', 'comment', 'gear', 'globe']) might benefit from more descriptive naming or external configuration if these icons need to be maintained or extended. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_0l20314343TST1jc
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.
| align-items: center; | ||
| gap: 8px; | ||
| /* border: 1px solid; */ | ||
| border-radius: 6px; |
There was a problem hiding this comment.
Avoid duplicate border-radius declarations. The rule sets border-radius twice (6px and later 10px). Choose one.
| border-radius: 4px; | ||
| cursor: pointer; | ||
| opacity: 0.8; | ||
| border-radius: 6px; |
There was a problem hiding this comment.
Avoid duplicate border-radius in .codicon. The declaration on line 69 (4px) is overridden by line 72 (6px).
| border-radius: 6px; | ||
| border: 1px solid rgba(255, 255, 255, 0.50); | ||
| /* background: linear-gradient(180deg, #AFF349 0%, #1B9400 100%); */ | ||
| fill: linear-gradient(180deg, #AFF349 0%, #1B9400 100%); |
There was a problem hiding this comment.
The 'fill' property with a linear-gradient value is not standard CSS. Consider using a different approach (e.g. background/image) for gradient fills in icons.
* Working no move * Working register redirect
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0388ae8 in 1 minute and 38 seconds
More details
- Looked at
118lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. src/vs/workbench/common/views.ts:260
- Draft comment:
Ensure the logic for overriding viewContainerLocation is clearly documented. The override forcing non-allowed IDs from AuxBar to Sidebar is intended, but a comment explaining the rationale would improve maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The existing comment is concise but clear about what the code does. The logic is simple and self-documenting - it's a basic permission check that moves non-allowed containers to a default location. Adding more documentation about the "rationale" would be unnecessary since the purpose is clear from the code and existing comment. The comment also starts with "Ensure that..." which is a red flag per the review rules.
Perhaps there are deeper architectural reasons for this behavior that would benefit from documentation? The rationale for which containers are allowed vs not allowed could be important context.
While there may be deeper reasons, those would belong in architecture/design docs, not inline code comments. The code's behavior is clear and the existing comment is sufficient for maintainers to understand what it does.
Delete this comment. The existing code comment is sufficient, and the request starts with "Ensure that..." which violates the review rules. Additional rationale would be better suited for architecture docs.
2. src/vs/workbench/services/views/pearai/pearaiViews.ts:31
- Draft comment:
Returning early without any feedback when moving to a disallowed location might lead to silent failures. Consider logging or notifying that the move was prevented. - 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 appears to be an intentional UX decision to silently prevent certain view movements. Adding logging/notifications could be noisy and confusing for users who accidentally try to move views to disallowed locations. The code comments clearly document the behavior for developers. This seems like a reasonable UI pattern - silently preventing invalid actions rather than showing errors.
The comment raises a valid point about silent failures being potentially confusing. Maybe users should get feedback when their action fails.
In a UI context, silently preventing invalid actions is often better UX than showing error messages. The behavior is intentional and documented in code comments.
The comment should be deleted as it suggests changing what appears to be an intentional UX decision. Silent prevention of invalid view movements is reasonable UI behavior.
3. src/vs/workbench/workbench.common.main.ts:165
- Draft comment:
The new singleton registration for IViewDescriptorService with PearAIViewDescriptorService looks correct. Confirm that all dependencies expect this override and that no unintended behavior occurs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that all dependencies expect the override and that no unintended behavior occurs. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment does not provide a specific code suggestion or ask for a specific test to be written.
4. src/vs/workbench/common/views.ts:260
- Draft comment:
Avoid duplicating the auxiliaryBar check logic. The same condition exists in the moveViewContainerToLocation override below. Consider extracting a shared utility or comment why each layer enforces it separately. - Reason this comment was not posted:
Marked as duplicate.
5. src/vs/workbench/services/views/pearai/pearaiViews.ts:26
- Draft comment:
The overridden moveViewContainerToLocation uses early returns to silently ignore invalid moves. It may be useful to document (or log) why moves are ignored to aid future debugging. - Reason this comment was not posted:
Marked as duplicate.
6. src/vs/workbench/services/views/pearai/pearaiViewsShared.ts:1
- Draft comment:
Consider adding a brief comment explaining the purpose of the allowed view container IDs array for clarity. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
7. src/vs/workbench/workbench.common.main.ts:165
- Draft comment:
The registration of PearAIViewDescriptorService appears correct. Ensure that its import order guarantees that no calls to IViewDescriptorService occur before its registration. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
Workflow ID: wflow_G4OPCMb0f3mMpkzI
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 2319a90 in 1 minute and 3 seconds
More details
- Looked at
33lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/vs/workbench/browser/parts/overlay/pearOverlayPart.ts:24
- Draft comment:
Renaming constant from 'PEAROVERLAY_ID' to 'PEARAI_CHAT_ID'. Ensure consistency in naming if similar identifiers exist in the project. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/vs/workbench/browser/parts/overlay/pearOverlayPart.ts:73
- Draft comment:
Updated extension identifier to use the new 'PEARAI_CHAT_ID'. Verify that this aligns with the intended extension usage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify if the change aligns with the intended usage, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.
3. src/vs/workbench/browser/parts/overlay/pearOverlayPart.ts:125
- Draft comment:
Ensure that using the updated 'PEARAI_CHAT_ID' in the call to resolve is intentional and consistent with overall design. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that a change is intentional and consistent with the overall design. It does not provide a specific suggestion or point out a clear issue, which violates the rule against asking the author to confirm their intention or ensure behavior is intended.
4. src/vs/workbench/browser/parts/overlay/pearOverlayPart.ts:24
- Draft comment:
Renamed constant 'PEAROVERLAY_ID' to 'PEARAI_CHAT_ID'. Ensure that the new identifier and title ('pearai.chatView' & 'pearai.overlayView') are consistently reflected in all integrations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. src/vs/workbench/browser/parts/overlay/pearOverlayPart.ts:73
- Draft comment:
Updated ExtensionIdentifier usage to use 'PEARAI_CHAT_ID'. Verify that the extension registration aligns with this new identifier. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. src/vs/workbench/browser/parts/overlay/pearOverlayPart.ts:122
- Draft comment:
The call to _webviewViewService.resolve now uses 'PEARAI_CHAT_ID'. Confirm that the provider (e.g., PearInventoryPanel) expects this updated identifier. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_FHL8hyrhNsL8Vawu
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 4e125f8 in 25 seconds
More details
- Looked at
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. extensions/pearai-submodule:1
- Draft comment:
Updated submodule commit; please ensure this change is intentional and tested. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. extensions/pearai-submodule:1
- Draft comment:
Submodule pointer updated to c85d5d314912a926136c7d1a1c4137f374b7b298. Confirm that this update incorporates the intended inventory icons changes and has been fully tested. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm their intention and ensure that the change is tested, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written.
Workflow ID: wflow_0hEOpsBcHCsRZfrl
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 37233d1 in 1 minute and 20 seconds
More details
- Looked at
204lines of code in2files - Skipped
3files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:93
- Draft comment:
Using the CSS :has() pseudo-class (e.g. for hover width adjustments) may have limited browser support. Verify that target environments support it or provide a fallback. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify browser support for the CSS :has() pseudo-class. This falls under asking the author to double-check or ensure something, which is against the rules. The comment does not provide a specific code suggestion or ask for a specific test to be written.
2. src/vs/workbench/browser/parts/paneCompositePart.ts:424
- Draft comment:
Consider adding ARIA attributes to the icon elements for improved accessibility. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:94
- Draft comment:
Using the ':has()' pseudo-class for hover states is a neat solution for expanding the icon container, but note that :has() is relatively new and may not be supported in older browsers. Ensure that our target Chromium version reliably supports it or consider a fallback. - Reason this comment was not posted:
Marked as duplicate.
4. src/vs/workbench/browser/parts/paneCompositePart.ts:421
- Draft comment:
The refactored icon creation using an array (with icon and label) is clean. Please double-check that the order in which the icon group and composite bar container are prepended (lines 451-452) meets the intended UI layout, as the ordering is critical. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to double-check the order of elements in the UI layout, which violates the rule against asking the author to confirm or double-check things. It does not provide a specific suggestion or point out a clear issue, so it should be removed.
Workflow ID: wflow_RP3DUmcx5sYulRNb
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.
|
|
||
| icons.forEach(({ icon, label }) => { | ||
| // Main container | ||
| const container = $('span'); |
There was a problem hiding this comment.
Each icon container has a 'cursor: pointer' style, suggesting interactivity. If these icons are meant to be interactive, please ensure that appropriate click handlers or accessibility roles (e.g. ARIA labels) are added so that their function is clear.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1334ecc in 55 seconds
More details
- Looked at
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/vs/workbench/services/views/pearai/pearaiViewsShared.ts:1
- Draft comment:
Verify container ID case and names. The original used 'PearAI' (uppercase) and now 'pearai' (lowercase), which may break existing references. Ensure consistency across the codebase. - 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%
The comment starts with "Verify..." which is a red flag per the rules. It's asking the author to check something rather than pointing out a definite issue. Without access to other files where these IDs are referenced, we can't be certain this is actually a problem. The author likely changed the case intentionally.
Maybe this really could break existing references and cause runtime issues? The case change seems intentional but could be problematic.
Per the rules, we should not ask authors to verify things or make speculative comments. If this were a real issue, it would likely be caught by the build or tests.
Delete this comment as it violates multiple rules - it's speculative, asks for verification, and requires more context to validate.
2. src/vs/workbench/services/views/pearai/pearaiViewsShared.ts:1
- Draft comment:
Verify container ID 'pearai-roo-cline'—it appears possibly misspelled (perhaps 'pearai-root-cline' was intended). - 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 comment violates several rules. First, it starts with "Verify" which is explicitly called out as likely not useful. Second, it's speculative - the reviewer doesn't have strong evidence that this is actually misspelled. Third, view container IDs are likely referenced elsewhere in the codebase, so without seeing those references, we can't be sure this is wrong.
The ID could genuinely be intended to be 'roo-cline' - maybe it's a feature name or reference we don't understand. Without more context from the broader codebase, we can't be certain this is a mistake.
You're right - we have no strong evidence this is actually misspelled. The author likely chose this ID deliberately to match usage elsewhere in the codebase.
Delete this comment as it asks for verification without strong evidence of an actual issue, and we lack the broader context to know if this ID is correct.
Workflow ID: wflow_5pmca9prZByb0sMs
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 6536bd1 in 54 seconds
More details
- Looked at
49lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. src/vs/workbench/services/views/pearai/pearaiViews.ts:29
- Draft comment:
Using .includes() for matching IDs is intentional to allow dynamic suffixes; consider adding a comment explaining this design choice. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. src/vs/workbench/services/views/pearai/pearaiViewsShared.ts:1
- Draft comment:
Added auxiliaryBarAllowedViewContainerIDs now includes views with dynamic suffixes; ensure this list remains updated as integration patterns evolve. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. src/vs/workbench/services/views/pearai/pearaiViews.ts:29
- Draft comment:
Consider renaming the inline function 'isAllowed' to something more descriptive (e.g. 'isAuxiliaryAllowed') and caching viewContainer.id in a variable for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. src/vs/workbench/services/views/pearai/pearaiViews.ts:33
- Draft comment:
The conditional checks enforcing that only allowed containers can be moved to (or remain in) the AuxiliaryBar are clear. Verify that using 'includes' to match viewContainer IDs covers all intended patterns. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. src/vs/workbench/services/views/pearai/pearaiViewsShared.ts:1
- Draft comment:
Including 'workbench.views.service.auxiliarybar' in allowed IDs properly supports dynamic integration view IDs. Ensure that using 'includes' in the matching logic meets all edge cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_X9V5anpIwsT5dHJl
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 507cc13 in 1 minute and 11 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/vs/workbench/browser/parts/paneCompositePart.ts:445
- Draft comment:
If the label is no longer needed for inventory icons, remove both the labelElement creation and its appending instead of simply commenting out the append call. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_bhBCKqFPqNilsXV9
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.
|
|
||
| // Append in order | ||
| container.appendChild(iconElement); | ||
| // container.appendChild(labelElement); |
There was a problem hiding this comment.
Avoid leaving commented out code. If labelElement is no longer used, remove it instead of commenting out.
Suggested change
| // container.appendChild(labelElement); |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 42b0a0b in 2 minutes and 18 seconds
More details
- Looked at
244lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:83
- Draft comment:
Usage of the ':has' pseudo-class should be verified for compatibility with target browsers/Electron versions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify compatibility, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
2. src/vs/workbench/browser/parts/paneCompositePart.ts:470
- Draft comment:
The refactoring to use an InventoryIcons collection and toggle active classes improves clarity. Verify that the toggle logic aligns with desired state changes. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:68
- Draft comment:
Using 'transition: all' can be overly broad and may animate unintended properties. Consider specifying only the needed properties (e.g. width, box-shadow) to improve performance and avoid unpredictable behavior. - Reason this comment was not posted:
Marked as duplicate.
4. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:83
- Draft comment:
The new CSS rules use the ':has()' pseudo-class to target child icons. Since ':has' is a relatively new selector, confirm that your target Chromium/Electron version fully supports it or prepare fallbacks if needed. - Reason this comment was not posted:
Marked as duplicate.
5. src/vs/workbench/browser/parts/paneCompositePart.ts:109
- Draft comment:
The interface IInventoryIcon includes a 'width' property that isn’t directly used in the TypeScript code (CSS rules set widths based on icon type). Consider either using this value to style the element dynamically or removing it to avoid duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. src/vs/workbench/browser/parts/paneCompositePart.ts:162
- Draft comment:
The property 'activeInventoryIcon' is initialized to 'chat', which makes that icon active by default. Verify that this default state aligns with the intended user experience or consider making it configurable. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. src/vs/workbench/browser/parts/paneCompositePart.ts:474
- Draft comment:
In the click handler for each inventory icon, the active state is toggled and a command is executed. Confirm that deactivating an already active icon (setting activeInventoryIcon to undefined) is intentional and that this behavior remains in sync with the UI. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. src/vs/workbench/browser/parts/paneCompositePart.ts:453
- Draft comment:
The logic for creating inventory icon elements is repeated directly inside the composite bar creation. Consider refactoring this block into a helper function to improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_BccP5btrOcfDYxVA
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.
| cursor: pointer; | ||
| border-radius: 8px; | ||
| position: relative; | ||
| transition: all 0.2s ease-in-out; |
There was a problem hiding this comment.
Considers using 'transition: all ...' may animate unintended properties. Prefer specifying only necessary properties to improve performance.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0846e39 in 2 minutes and 4 seconds
More details
- Looked at
98lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:88
- Draft comment:
Good change: Removing the private qualifier from commandService allows it to be passed to super and used by subclasses. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it simply states what the change does without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
2. src/vs/workbench/browser/parts/paneCompositePart.ts:186
- Draft comment:
Changing commandService from private to protected is appropriate to allow usage in subclasses. - Reason this comment was not posted:
Marked as duplicate.
3. src/vs/workbench/browser/parts/panel/panelPart.ts:78
- Draft comment:
Consistent update: injection of commandService is adjusted (removed private modifier) aligning with other composite parts. - Reason this comment was not posted:
Marked as duplicate.
4. src/vs/workbench/browser/parts/sidebar/sidebarPart.ts:87
- Draft comment:
Injection of commandService has been added here. Ensure that it’s consistently used in SidebarPart as in other parts. - 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 appears to be a speculative comment asking for verification rather than pointing out a specific issue. The code shows commandService is properly injected and passed through - there's no clear problem. The comment is asking for a general consistency check without identifying any specific inconsistency.
The code change could be part of a larger refactoring where consistency is important. There could be established patterns in other parts that this change should follow.
While consistency is important, the comment doesn't identify any specific inconsistency or problem. The code shows proper dependency injection patterns being followed.
Delete this comment as it's asking for verification without identifying any specific issues, and the code shows proper dependency injection usage.
5. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:88
- Draft comment:
Refactored the ICommandService injection by removing the private modifier and passing it to the super call. Confirm that inherited usage (e.g. in fillExtraContextMenuActions) correctly accesses the commandService. - Reason this comment was not posted:
Marked as duplicate.
6. src/vs/workbench/browser/parts/paneCompositePart.ts:186
- Draft comment:
Changing ICommandService from a private to a protected readonly member improves subclass access. Verify that any direct calls (e.g. within composite bar updates) continue to work as expected. - Reason this comment was not posted:
Marked as duplicate.
7. src/vs/workbench/browser/parts/panel/panelPart.ts:81
- Draft comment:
The ICommandService injection has been refactored consistently—now without the private modifier and passed up to the super constructor. This maintains uniform dependency handling across parts. - Reason this comment was not posted:
Marked as duplicate.
8. src/vs/workbench/browser/parts/sidebar/sidebarPart.ts:107
- Draft comment:
ICommandService is now injected and passed to the base class consistently. Ensure that methods (e.g. in fillExtraContextMenuActions) that execute commands can access the commandService via inheritance. - Reason this comment was not posted:
Marked as duplicate.
9. src/vs/workbench/browser/parts/paneCompositePart.ts:117
- Draft comment:
Consider localizing the Inventory icon labels (e.g. 'Chat', 'Creator', 'Search', 'Memory') instead of hardcoding them. This would enhance internationalization support. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_0WqMhsEBNfbw6948
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 99f730e in 1 minute and 21 seconds
More details
- Looked at
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/auxiliaryBarPart.ts:217
- Draft comment:
The function now always returns CompositeBarPosition.TITLE and includes commented-out code. Consider removing the unused code to keep the implementation clean if the configuration check is no longer needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_snKRdG9M4zTvbJqs
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.
| default: return CompositeBarPosition.TITLE; | ||
| } | ||
| // Fix secondary sidebar composite bar position to default | ||
| return CompositeBarPosition.TITLE; |
There was a problem hiding this comment.
Hardcoded return (CompositeBarPosition.TITLE) disables dynamic positioning based on configuration. If intended, please remove or clearly document the removal of configurability and consider deleting the commented-out code.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on da45292 in 1 minute and 51 seconds
More details
- Looked at
171lines of code in2files - Skipped
0files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:85
- Draft comment:
Width values for active icons have been reduced (e.g., chat from 70px to 60px). Confirm these changes align with the design specs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm alignment with design specs, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue with the code.
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:102
- Draft comment:
Glow box-shadow updated to use hex color codes and increased blur/spread. Verify that the lack of opacity control (as opposed to RGBA) meets the intended visual effect. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify the visual effect of using hex color codes instead of RGBA for box-shadow, which implies a lack of opacity control. This is asking for confirmation of intention, which is against the rules.
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:137
- Draft comment:
Icon label position updated (left changed from 28px to 25px with added margin-bottom). Confirm alignment remains as expected with icon sizes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm alignment, which violates the rule against asking for confirmation or verification. It doesn't provide a specific suggestion or point out a clear issue.
4. src/vs/workbench/browser/parts/paneCompositePart.ts:113
- Draft comment:
New 'containerId' property added to IInventoryIcon. Ensure that any consumers of this interface are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that consumers of the interface are updated, which is similar to asking them to double-check or ensure something is tested. This violates the rules.
5. src/vs/workbench/browser/parts/paneCompositePart.ts:120
- Draft comment:
Width for 'chat' icon changed from 70 to 10. Verify that this value is used appropriately; CSS still expands the icon container to 60px when active. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment is about a UI styling change - specifically the width of an icon. The rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic. Assume that if the author changed the UI, they did it correctly." This is exactly the kind of comment we should remove.
Could this width change affect functionality beyond just styling? Could it break layouts or cause UI bugs?
Even if the width could affect layouts, the rules are clear that we should trust the author on UI changes. They must have tested this change and ensured it works correctly.
This comment should be deleted as it violates the rule about not commenting on UI/styling changes.
6. src/vs/workbench/browser/parts/paneCompositePart.ts:134
- Draft comment:
The implementation of getByContainerId depends on the composite id being prefixed with 'workbench.view.extension.'. Confirm that all composite IDs follow this convention. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the author to confirm that all composite IDs follow a specific convention. It is not suggesting a specific code change or improvement, nor is it pointing out a specific issue with the code. It is asking for confirmation, which violates the rule against asking the PR author to confirm their intention or ensure behavior is intended.
7. src/vs/workbench/browser/parts/paneCompositePart.ts:481
- Draft comment:
Click handler logic toggles active state based on current active icon and composite ID. Ensure that the combination of conditions used to compare 'activeInventoryIcon' and 'isViewActive' correctly represents the desired UX. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that the logic correctly represents the desired UX. This falls under asking the author to confirm their intention, which is against the rules. The comment does not provide a specific suggestion or point out a clear issue.
8. src/vs/workbench/browser/parts/paneCompositePart.ts:765
- Draft comment:
updateActiveInventoryIcon uses querySelectorAll and toggles 'active' class based on icon child match. Consider whether this generic approach might cause issues if more than one icon element is nested or if future changes reuse similar class names. - 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%
The comment raises a theoretical concern but:
- The DOM structure is controlled by this code itself (lines 459-472)
- The icon classes are namespaced with 'inventory-' prefix
- The selector hierarchy is specific (.icon-container > .icon-element)
- There's no evidence this is causing actual problems
- The suggestion is speculative ("might cause issues") rather than pointing to a concrete problem
The comment does identify a real potential issue with DOM querying - nested elements and class name conflicts can be problematic in general.
While DOM querying can be fragile, this code has sufficient safeguards through controlled DOM structure and namespaced classes. The comment doesn't suggest a concrete better alternative.
Delete the comment. It raises only theoretical concerns about a well-structured piece of code, without providing actionable suggestions for improvement.
9. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:84
- Draft comment:
Active icon container widths have been adjusted (chat: 70→60, creator: 80→75, search: 75→70, memory: 85→80). Verify these values align with the intended design. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
10. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:101
- Draft comment:
Glow (box-shadow) properties have been updated with new dimensions (57.6px and 9.6px) and hex colors. Confirm the new visual effect is as desired. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
11. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:137
- Draft comment:
Icon label offset has been adjusted (left from 28px to 25px with added margin-bottom). Confirm if this layout shift is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
12. src/vs/workbench/browser/parts/paneCompositePart.ts:120
- Draft comment:
The 'chat' icon width has been changed from 70 to 10 in the InventoryIcons array, but the CSS active width is 60px. Double-check if the reduced TS width value is intended. - Reason this comment was not posted:
Marked as duplicate.
13. src/vs/workbench/browser/parts/paneCompositePart.ts:121
- Draft comment:
The command for the 'creator' icon has been updated to 'pearai-roo-cline.focus'. Verify that this naming is consistent with other view focus commands. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify consistency with other commands, which violates the rule against asking the author to double-check things. It doesn't provide a specific suggestion or point out a clear issue.
14. src/vs/workbench/browser/parts/paneCompositePart.ts:134
- Draft comment:
getByContainerId appends the prefix 'workbench.view.extension.' to the containerId. Consider centralizing this prefix in a constant to avoid duplicating the string across the code. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
15. src/vs/workbench/browser/parts/paneCompositePart.ts:481
- Draft comment:
The click event handler now only activates a non-active icon and does not toggle off an active one. Confirm that omitting the deactivation behavior is intentional. - Reason this comment was not posted:
Marked as duplicate.
16. src/vs/workbench/browser/parts/paneCompositePart.ts:765
- Draft comment:
The updateActiveInventoryIcon method toggles the 'active' class based on the presence of a child element with the icon's class. Consider checking the container's identity directly for more robust activation logic. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Eci9bIvAHGXe6Lug
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 b459a8f in 1 minute and 36 seconds
More details
- Looked at
81lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:8
- Draft comment:
New flex container styles are added for the auxiliary bar. Confirm these rules don't conflict with other layout styles. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the new flex container styles don't conflict with other layout styles. This is asking for confirmation of intention or to double-check something, which violates the rules.
2. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:24
- Draft comment:
Margin-top reduced to 6px and flex layout added for .title. Ensure these changes match the intended UI adjustments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that the changes match the intended UI adjustments, which violates the rule against asking for confirmation of intention. It does not provide a specific suggestion or point out a potential issue with the code.
3. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:42
- Draft comment:
Reformatted .icon-group styles. Consider removing unused commented code (e.g., margin-bottom, z-index) to keep the CSS clean. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment has two parts: 1) Noting reformatting of .icon-group styles - this is just informative and not actionable. 2) Suggesting removal of commented code - while this is actionable, it's not directly related to the changes made in this PR which appear to be mostly about layout and formatting. The commented code existed before and wasn't added or modified in this PR.
The suggestion to remove commented code could be valuable for code cleanliness. Just because the commented code wasn't added in this PR doesn't necessarily mean we shouldn't fix it now.
While cleaning up commented code is good practice, our rules state that we should only comment on changes made in the diff. Making unrelated cleanup suggestions increases review noise and scope creep.
The comment should be deleted because it's partly informational (about reformatting) and partly suggesting changes to code that wasn't modified in this PR.
4. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:37
- Draft comment:
Remove unused commented-out properties (e.g. border, z-index) in the .title block to keep the CSS clean. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:40
- Draft comment:
Verify that reducing margin-top from 12px to 6px in the .title block aligns with the overall design guidelines. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. src/vs/workbench/browser/parts/auxiliarybar/media/auxiliaryBarPart.css:65
- Draft comment:
Consider removing commented-out properties (e.g. margin-bottom and z-index) in the .icon-group block if they are no longer needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_HHk6dxtgJAITLilY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Himanshu-Singh-Chauhan
pushed a commit
to Himanshu-Singh-Chauhan/pearai-app
that referenced
this pull request
Feb 15, 2025
UI Improvements and Model Selection
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.
Important
Renames 'Secondary Side Bar' to 'PearAI Side Bar', updates layout and styles, and modifies configuration for the PearAI Side Bar.
viewsExtensionPoint.ts,layoutActions.ts, andpanelActions.ts.AuxiliaryBarand PearAI integrations from moving out inpearaiViews.ts.auxiliaryBarPart.cssfor new icon styles and layout adjustments.auxiliaryBarPart.tsto include new layout logic for the PearAI Side Bar.workbench.contribution.tsto reflect the PearAI Side Bar changes.PearAIViewDescriptorServiceinworkbench.common.main.ts.This description was created by
for b459a8f. It will automatically update as commits are pushed.