Pin feature#124
Merged
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
|
Warning Review limit reached
More reviews will be available in 41 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces an entity pinning feature allowing users to pin Azure Service Bus queues, topics, and subscriptions within a workspace scope. Pinned entities persist to preferences, reorder filtered lists, and appear in a dedicated UI section with toggle and selection commands. ChangesEntity Pinning Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
BusLane/Models/PinnedEntity.cs (1)
8-19: ⚡ Quick winConsider adding validation to enforce TopicName requirements by entity type.
The record allows invalid states: a Subscription without a TopicName, or a Queue/Topic with a TopicName set. While DisplayName handles the null case defensively, validating at construction time would prevent inconsistent data.
🛡️ Proposed validation using init-only properties
public record PinnedEntity( string WorkspaceId, PinnedEntityType Type, string Name, string? TopicName) { + public PinnedEntity(string WorkspaceId, PinnedEntityType Type, string Name, string? TopicName) + : this(WorkspaceId, Type, Name, TopicName) + { + if (Type == PinnedEntityType.Subscription && string.IsNullOrWhiteSpace(TopicName)) + throw new ArgumentException("TopicName is required for Subscription entities.", nameof(TopicName)); + + if (Type != PinnedEntityType.Subscription && !string.IsNullOrWhiteSpace(TopicName)) + throw new ArgumentException("TopicName should only be set for Subscription entities.", nameof(TopicName)); + } + public string DisplayName => Type == PinnedEntityType.Subscription && !string.IsNullOrWhiteSpace(TopicName) ? $"{TopicName}/{Name}" : Name;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BusLane/Models/PinnedEntity.cs` around lines 8 - 19, The PinnedEntity record allows inconsistent states (Subscription missing TopicName, or Queue/Topic having TopicName) — add validation in the record's constructor/body to enforce rules: when Type == PinnedEntityType.Subscription require non-empty TopicName; when Type != PinnedEntityType.Subscription require TopicName be null or empty; implement this by converting the positional record to include a validation block (a compact constructor or explicit parameter checks) inside PinnedEntity (referencing the Type and TopicName parameters) and throw ArgumentException/ArgumentNullException with clear messages if validation fails; keep DisplayName and TypeLabel unchanged.BusLane/ViewModels/Core/NavigationState.cs (1)
98-103: ⚡ Quick winDocument the new public pinning API.
The added public constructor and pinning methods are missing XML doc comments, unlike the rest of the public surface in this file.
As per coding guidelines, "Use XML doc comments for public classes, methods, and key regions".
Also applies to: 204-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BusLane/ViewModels/Core/NavigationState.cs` around lines 98 - 103, The new public API lacks XML documentation: add XML doc comments for the public NavigationState(IPreferencesService? preferencesService) constructor (and the parameter), the parameterless NavigationState() constructor, and all newly added public pinning methods referenced around the pinning section (methods that manage pin/unpin state); include <summary> for purpose, <param> describing the preferencesService (nullable behavior), and any <returns> or <remarks> as appropriate so the public surface matches existing documentation style in this file. Ensure the comments match the existing XML doc format used elsewhere in NavigationState and cover threading/side-effect notes if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BusLane/ViewModels/Core/NavigationState.cs`:
- Around line 218-229: TogglePin currently mutates _allPinnedEntities before
calling PersistPins()/Save, so a persistence failure leaves in-memory state
inconsistent; modify TogglePin to make the operation atomic by either (a)
performing the persistence first and only mutating _allPinnedEntities and
calling ReloadScopedPins() on success, or (b) wrapping the current mutation +
PersistPins() in try/catch and rolling back the in-memory change to
_allPinnedEntities if PersistPins() throws; ensure references to
_allPinnedEntities, PersistPins(), ReloadScopedPins(), and the TogglePin method
are used so the fix is applied in both occurrences (around lines shown and the
second block at 298-306).
- Line 37: PinnedEntities is currently a public mutable ObservableCollection
which allows external callers to modify it and cause state/persistence desync;
change the public API to expose a ReadOnlyObservableCollection<PinnedEntity>
(e.g., public ReadOnlyObservableCollection<PinnedEntity> PinnedEntities) backed
by a private ObservableCollection<PinnedEntity> (e.g., _pinnedEntities) that
only this class mutates. Update TogglePin, PersistPins, and ReloadScopedPins to
operate on _allPinnedEntities and the private _pinnedEntities (raising
collection changes) and ensure PinnedEntitiesJson is only updated via
PersistPins so external code cannot mutate the public collection directly.
---
Nitpick comments:
In `@BusLane/Models/PinnedEntity.cs`:
- Around line 8-19: The PinnedEntity record allows inconsistent states
(Subscription missing TopicName, or Queue/Topic having TopicName) — add
validation in the record's constructor/body to enforce rules: when Type ==
PinnedEntityType.Subscription require non-empty TopicName; when Type !=
PinnedEntityType.Subscription require TopicName be null or empty; implement this
by converting the positional record to include a validation block (a compact
constructor or explicit parameter checks) inside PinnedEntity (referencing the
Type and TopicName parameters) and throw ArgumentException/ArgumentNullException
with clear messages if validation fails; keep DisplayName and TypeLabel
unchanged.
In `@BusLane/ViewModels/Core/NavigationState.cs`:
- Around line 98-103: The new public API lacks XML documentation: add XML doc
comments for the public NavigationState(IPreferencesService? preferencesService)
constructor (and the parameter), the parameterless NavigationState()
constructor, and all newly added public pinning methods referenced around the
pinning section (methods that manage pin/unpin state); include <summary> for
purpose, <param> describing the preferencesService (nullable behavior), and any
<returns> or <remarks> as appropriate so the public surface matches existing
documentation style in this file. Ensure the comments match the existing XML doc
format used elsewhere in NavigationState and cover threading/side-effect notes
if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49d114a7-6b16-4252-9d7b-e1e46a404275
📒 Files selected for processing (13)
BusLane.Tests/Services/Infrastructure/PreferencesServiceTests.csBusLane.Tests/ViewModels/Core/ConnectionTabViewModelTests.csBusLane.Tests/ViewModels/Core/NavigationStatePinningTests.csBusLane.Tests/ViewModels/MainWindowViewModelTests.csBusLane.Tests/Views/EntityTreeViewTests.csBusLane/Models/PinnedEntity.csBusLane/Services/Abstractions/IPreferencesService.csBusLane/Services/Infrastructure/PreferencesService.csBusLane/ViewModels/Core/ConnectionTabViewModel.csBusLane/ViewModels/Core/NavigationState.csBusLane/ViewModels/MainWindowViewModel.csBusLane/Views/Controls/AzureEntityTreeView.axamlBusLane/Views/Controls/EntityTreeView.axaml
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
1 participant
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.
This pull request introduces support for pinning Service Bus entities (queues, topics, subscriptions) to workspaces, allowing users to persist and quickly access their favorite entities across sessions. The changes include a new
PinnedEntitymodel, updates to the preferences service for storing pinned entities, enhancements to the navigation state for pin management, and comprehensive tests to ensure correct behavior.Pinning Feature Implementation:
PinnedEntityrecord andPinnedEntityTypeenum inPinnedEntity.csto represent pinned entities and their types.IPreferencesServiceinterface and its implementations (PreferencesService,DummyPreferencesService, and test stubs) to include aPinnedEntitiesJsonproperty for persisting pinned entities. [1] [2] [3] [4] [5] [6] [7]Navigation State Enhancements:
NavigationStateto support pinning logic, including storing all pins, filtering entities to show pinned items first, and scoping pins to the current workspace. Added public APIs for toggling pins, checking pin status, and exposing pinned entities for UI binding. [1] [2] [3] [4] [5] [6]ConnectionTabViewModelto initializeNavigationStatewith the preferences service and set the pin scope when connecting to a workspace or Azure namespace. [1] [2] [3]Testing and Verification:
These changes collectively enable robust pinning functionality for Service Bus entities, improve user productivity, and ensure reliable persistence and retrieval of pinned items across sessions.
Summary by CodeRabbit
Release Notes