Support Multiple Topmost Records #3500
Conversation
|
🚨 gitStream Monthly Automation Limit Reached 🚨 Your organization has exceeded the number of pull requests allowed for automation with gitStream. To continue automating your PR workflows and unlock additional features, please contact LinearB. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the topmost record storage to support multiple topmost records while preserving the original order. Key changes include:
- Replacing the single-record storage with a new FlowLauncherJsonStorageTopMostRecord class.
- Migrating old topmost record data to a new structure supporting multiple records.
- Extending JsonStorage functionality with Exists and Delete methods.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Updated to use FlowLauncherJsonStorageTopMostRecord for topmost data. |
| Flow.Launcher/Storage/TopMostRecord.cs | Introduced a new storage class with migration logic and multiple records handling. |
| Flow.Launcher.Infrastructure/Storage/JsonStorage.cs | Added utility methods for file existence check and deletion. |
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
📝 Walkthrough""" WalkthroughThis update introduces new methods to the generic JSON storage class for existence checking and deletion of storage files. It adds a new class, Changes
Sequence Diagram(s)sequenceDiagram
participant MainViewModel
participant TopMostRecordStorage
participant JsonStorage
MainViewModel->>TopMostRecordStorage: Instantiate FlowLauncherJsonStorageTopMostRecord
TopMostRecordStorage->>JsonStorage: Check if old data exists (Exists())
alt Old data exists
TopMostRecordStorage->>JsonStorage: Load old TopMostRecord
TopMostRecordStorage->>TopMostRecordStorage: Migrate to MultipleTopMostRecord
TopMostRecordStorage->>JsonStorage: Delete old data (Delete())
else New data exists
TopMostRecordStorage->>JsonStorage: Load MultipleTopMostRecord
TopMostRecordStorage->>JsonStorage: Delete old data if any (Delete())
end
MainViewModel->>TopMostRecordStorage: Save()
TopMostRecordStorage->>JsonStorage: Save new MultipleTopMostRecord
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
Flow.Launcher.Infrastructure/Storage/JsonStorage.cs (1)
48-52: Consider broadening “existence” semantics
Exists()currently returnsFile.Exists(FilePath)only.
In day-to-day use we normally only care about the primary json, but the consumer of this API may interpret “storage exists” as “any artefact exists”. If a previous run crashed while writing, only the.bakor.tmpfile may be present, yetExists()will misleadingly return false.Either (a) clarify the XML-doc comment to state that only the main file is checked, or (b) extend the check to the backup / temp paths so that migration code can make a more reliable decision.
Flow.Launcher/ViewModel/MainViewModel.cs (3)
40-44: Field naming can be misleading
_topMostRecordactually holds the storage wrapper (FlowLauncherJsonStorageTopMostRecord), not the record collection itself.
Because the same identifier is later used for model operations (AddOrUpdate,IsTopMost) it still works, but the mental model is harder to follow.
A name such as_topMostRecordStorage(old name) or_topMostRecordManagerwould better convey responsibility.
1613-1614: Missing await/async variant ofSave()
JsonStorage.Save()is synchronous and can block the UI thread when Flow is closed via UI. You already haveSaveAsync()upstream – consider exposing an async counterpart inFlowLauncherJsonStorageTopMostRecordand calling it from an asyncSaveAsyncinMainViewModel, or invoking the sync save on a background thread.
1646-1651:IsTopMostinvoked per-result may impact large result sets
UpdateResultViewiterates every incomingResultand calls_topMostRecord.IsTopMost(result); under the new design this does anO(n)bag scan per call.
In the worst case (large multi-plugin query + many top-most records) this becomes quadratic.An inexpensive optimisation is to cache
_topMostRecordlook-ups once per query key:var isTopMostCache = new Dictionary<string, bool>(); ... var key = result.OriginQuery?.RawQuery; if (key != null && isTopMostCache.TryGetValue(key, out var isTop)) { if (isTop) result.Score = Result.MaxScore; } else { var top = _topMostRecord.IsTopMost(result); if (key != null) isTopMostCache[key] = top; if (top) result.Score = Result.MaxScore; }Flow.Launcher/Storage/TopMostRecord.cs (2)
252-277: Custom converter should null-check & reuse options
Read()assumesdictionaryis non-null. A corrupted file will throw NRE instead of propagatingJsonException, losing diagnostic information.
Add a guard:if (dictionary == null) return new();Also, consider passing the existing
optionswhen re-serialising inWrite()to preserve caller settings (e.g., indentation).
279-299:Record.EqualslacksGetHashCodeoverride
Equalsis overridden for logical comparison butGetHashCodeisn’t, breaking hash-based collections, and may lead to subtle bugs if aRecordis ever placed in aHashSet/dictionary. Either addGetHashCodeor seal the class and document the behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Infrastructure/Storage/JsonStorage.cs(1 hunks)Flow.Launcher/Storage/TopMostRecord.cs(2 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
136-138: Constructor no longer pre-loads data – confirm this is intendedOld code loaded records eagerly (
_topMostRecordStorage.Load()); the new wrapper loads lazily inside its own ctor.
That is fine, but be aware it now occurs beforeMainViewModel’s constructor body runs only if the wrapper’s constructor does synchronous IO. If startup performance matters, consider lazy-loading on first use.
This comment has been minimized.
This comment has been minimized.
Docstrings generation was requested by @Jack251970. * #3500 (comment) The following files were modified: * `Flow.Launcher.Infrastructure/Storage/JsonStorage.cs` * `Flow.Launcher/Storage/TopMostRecord.cs` * `Flow.Launcher/ViewModel/MainViewModel.cs`
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for multiple topmost records by migrating from a single-record storage mechanism to a new storage system that allows multiple records per query. Key changes include refactoring the storage API in MainViewModel, implementing a new data structure (MultipleTopMostRecord) with conversion logic from the old format, and adding file existence and deletion functionality in JsonStorage.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Migrates the topmost record storage to the new FlowLauncherJsonStorageTopMostRecord implementation. |
| Flow.Launcher/Storage/TopMostRecord.cs | Refactors the storage mechanism to support multiple topmost records and introduces a new data structure with migration logic. |
| Flow.Launcher/Infrastructure/Storage/JsonStorage.cs | Adds helper methods (Exists and Delete) for file management, supporting the migration process. |
Comments suppressed due to low confidence (1)
Flow.Launcher/Storage/TopMostRecord.cs:90
- [nitpick] The internal class name 'TopMostRecord' may cause confusion with the new multi-record storage and the legacy naming in the migration logic. Consider renaming it to a name that clearly indicates its role as the legacy data structure.
internal class TopMostRecord
|
Note Generated docstrings for this pull request at #3501 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for multiple topmost records by migrating from a single-record storage design to a multiple-record model.
- Replaces the old FlowLauncherJsonStorage with a new FlowLauncherJsonStorageTopMostRecord in MainViewModel.
- Implements migration logic and new data structures (using ConcurrentBag) in the storage layer to accommodate multiple records.
- Adds file existence and deletion capabilities in JsonStorage to support the migration process.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Updated storage field and usage to support multiple topmost records. |
| Flow.Launcher/Storage/TopMostRecord.cs | Introduced new classes for multiple topmost records and provided migration logic from old data. |
| Flow.Launcher/Infrastructure/Storage/JsonStorage.cs | Added methods to check file existence and delete files for storage management. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/Storage/TopMostRecord.cs (1)
146-148: ConcurrentBag usage is appropriate for this implementationUsing a
ConcurrentBagfor storing multiple records per query fits the requirements, as the retrieved learnings confirm that sequence order preservation is not needed in this context. This implementation choice appropriately balances thread safety with performance.
🧹 Nitpick comments (1)
Flow.Launcher/Storage/TopMostRecord.cs (1)
196-199: Check for empty bag may be redundantThe check for
value.IsEmptyafter already rebuilding the bag seems redundant since we're replacing the entire bag with filtered elements on line 193. The bag would be empty only if it contained just the one element being removed.- // if the bag is empty, remove the bag from the dictionary - if (value.IsEmpty) - { - records.TryRemove(result.OriginQuery.RawQuery, out _); - } + // if the bag is empty, remove the bag from the dictionary + if (records[result.OriginQuery.RawQuery].IsEmpty) + { + records.TryRemove(result.OriginQuery.RawQuery, out _); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/Storage/TopMostRecord.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/Storage/TopMostRecord.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3500
File: Flow.Launcher/Storage/TopMostRecord.cs:191-197
Timestamp: 2025-05-01T05:35:21.827Z
Learning: ConcurrentBag.TryTake(out T) in C# removes an arbitrary item from the bag, not necessarily the one matching the out parameter. This can lead to removing the wrong element when trying to remove a specific item. A safer approach is to reconstruct the collection using filtering or use an ordered collection with proper removal methods.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3500
File: Flow.Launcher/Storage/TopMostRecord.cs:145-149
Timestamp: 2025-05-01T05:38:25.645Z
Learning: For the MultipleTopMostRecord implementation in Flow.Launcher, sequence order of records in the ConcurrentBag does not need to be preserved, as confirmed by the developer. The unordered nature of ConcurrentBag is acceptable for this implementation.
🧬 Code Graph Analysis (1)
Flow.Launcher/Storage/TopMostRecord.cs (2)
Flow.Launcher.Infrastructure/Storage/JsonStorage.cs (3)
Exists(48-51)Delete(53-62)Save(202-213)Flow.Launcher/ViewModel/MainViewModel.cs (3)
Save(1609-1614)Result(1372-1410)Result(1412-1438)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
Flow.Launcher/Storage/TopMostRecord.cs (5)
12-85: Good implementation of the new storage class for managing topmost recordsThe new
FlowLauncherJsonStorageTopMostRecordclass provides a clean facade that handles migration between the old single-record format and the new multiple-records format. The constructor logic properly addresses all possible scenarios (new data exists, only old data exists, neither exists) with appropriate exception handling during file deletion operations.
191-194: Good fix for the ConcurrentBag.TryTake issueThis implementation correctly addresses the issue with
ConcurrentBag.TryTake()removing arbitrary items. Instead of usingTryTake(), the code creates a new bag from filtered elements, ensuring the exact matching record is removed.
229-233: Good fix for record updates using filtered approachSimilar to the
Removemethod, theAddOrUpdatemethod correctly handles updating records by creating a new collection from filtered elements rather than attempting to useTryTake(), which would remove arbitrary items. This implementation ensures that the specific record is properly updated.
240-262: Well-implemented JSON converter for concurrent collectionsThe
ConcurrentDictionaryConcurrentBagConverterproperly handles the serialization and deserialization of the concurrent collections by converting them to standard collections that can be serialized. This is a good practice for working with concurrent collections that don't have built-in serialization support.
266-269: Good choice to make Record properties immutableUsing
initaccessors instead ofsetmakes theRecordproperties immutable after initialization, which is a good practice for value objects. This prevents accidental modification of records after creation and helps maintain data integrity.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR enables Flow to support multiple topmost records by migrating existing single-record data and updating the storage logic.
- Refactors storage for topmost records to use a new JSON storage class and data structure.
- Adds migration logic to convert old topmost records into a new multiple-record format.
- Enhances JSON storage by adding existence checks and file deletion support.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Updates references to topmost record storage to use the new storage class for multiple topmost records. |
| Flow.Launcher/Storage/TopMostRecord.cs | Introduces the new storage class (FlowLauncherJsonStorageTopMostRecord) and data structure (MultipleTopMostRecord) for migration. |
| Flow.Launcher/Infrastructure/Storage/JsonStorage.cs | Adds new utility methods to check file existence and delete storage files. |
Comments suppressed due to low confidence (1)
Flow.Launcher/Infrastructure/Storage/JsonStorage.cs:57
- Wrap the File.Delete call in a try-catch block to gracefully handle possible exceptions during file deletion and enhance fault tolerance.
if (File.Exists(path))
|
Just in case, I’ll mention this now — I haven’t tested this PR and I'm not fully aware of its intent. But from a UX perspective, if there’s already a top-most item A and you add another one B, the display order should be B, then A. When there are three top-most item — A, B, and C — the one that was activated most recently should be displayed in front. Otherwise, users may feel like their top-most action didn’t work, and they’ll be forced to deactivate other top-most item to perform their intended task. |
Yeah, that makes sense. But I think the impact may be very small. Users typically will not set so much records as topmost, so ABC, CBA really does differ too much. The interface still allows them to select the correct option easily. |
|
Noooo~ ABC and CBA is different! (I actually tested this PR.) I use the Top Most feature all the time, and since our search often doesn’t show the best results first, a true “pin to top” is essential. This PR isn’t Top Most—it’s more like “add to top-group.” If there’s already a Top Most item, clicking Top Most again doesn’t move it to the first position, so you’d have to rename the menu—and it’s not intuitive. Isn’t it just a matter of sorting by insertion order? If I pin A, then B, then C, C was last, so it should appear first—C, B, A. That’s trivial. Items with TopMost = true will still be grouped at the top, matching your PR’s intention. We can’t be sure there won’t be requests like “Please allow ordering within the top group.” If someone files a bug titled “I clicked Top Most but it sorted to second place,” replying “Oh, unpin the first item” would be ridiculous. Users won’t remember what they pinned—they have no way to know unless they try—so when they click Top Most, their only expectation is “show this at the very top.” It’s fine to group all pinned items together, but that group’s order needs to reflect the user’s intent (most recently pinned first). Anyway, this implementation fails to meet the functional expectation of “Top Most,” so it can’t stay like this. If you’re going to add this feature, please change it so that items are displayed in the order in which “Top Most” was applied—i.e., the item most recently marked as Top Most should appear first. |
Well, I get it. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR updates Flow to support multiple topmost records by migrating from a legacy single-record structure to a new data structure that supports multiple records and adjusts record ranking accordingly. Key changes include:
- Replacing the single topmost record storage with a new FlowLauncherJsonStorageTopMostRecord class.
- Updating the scoring logic in MainViewModel to factor in record positioning.
- Implementing migration logic in the storage layer to convert old data to the new format.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Updates to use the new topmost record structure and scoring adjustment. |
| Flow.Launcher/Storage/TopMostRecord.cs | Introduces the new FlowLauncherJsonStorageTopMostRecord and migration logic. |
| Flow.Launcher/Infrastructure/Storage/JsonStorage.cs | Adds supportive file-check and deletion methods. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Flow.Launcher/Storage/TopMostRecord.cs (3)
46-47: Add logging for old data migration.Consider adding a log message here to indicate that an old data migration is being performed. This would improve maintainability by providing better visibility into the migration process.
- // Migrate old data to new data + // Migrate old data to new data + System.Diagnostics.Debug.WriteLine("Migrating from TopMostRecord to MultipleTopMostRecord format.");
167-167: Consider if ConcurrentQueue is the right data structure for user expectations.The PR objectives mention that users expect the most recently pinned item to appear first (i.e., at the very top). However,
ConcurrentQueueis FIFO (First In, First Out), meaning the oldest items will be enumerated first.This may create a user experience issue where the most recently pinned item appears at the bottom of the topmost group, not at the top as users would expect when they pin an item.
Do you want to reconsider using a data structure that naturally places the most recent items first, such as
ConcurrentStack(LIFO), or manually reversing the collection during enumeration?
271-280: Consider preserving order for user experience.When deserializing the records, the order within each queue is preserved. However, if order is important for user experience (especially for the multiple topmost items feature), consider adding a comment that explains how ordering works in the deserialized data.
This is particularly important since there was a discussion in the PR about the appropriate ordering of topmost items.
public override ConcurrentDictionary<string, ConcurrentQueue<Record>> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { var dictionary = JsonSerializer.Deserialize<Dictionary<string, List<Record>>>(ref reader, options); var concurrentDictionary = new ConcurrentDictionary<string, ConcurrentQueue<Record>>(); foreach (var kvp in dictionary) { + // Preserve the order of records as they were stored in the JSON concurrentDictionary.TryAdd(kvp.Key, new ConcurrentQueue<Record>(kvp.Value)); } return concurrentDictionary; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/Storage/TopMostRecord.cs(2 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/ViewModel/MainViewModel.cs
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/Storage/TopMostRecord.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3500
File: Flow.Launcher/Storage/TopMostRecord.cs:191-197
Timestamp: 2025-05-01T05:35:21.827Z
Learning: ConcurrentBag.TryTake(out T) in C# removes an arbitrary item from the bag, not necessarily the one matching the out parameter. This can lead to removing the wrong element when trying to remove a specific item. A safer approach is to reconstruct the collection using filtering or use an ordered collection with proper removal methods.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3500
File: Flow.Launcher/Storage/TopMostRecord.cs:145-149
Timestamp: 2025-05-01T05:38:25.645Z
Learning: For the MultipleTopMostRecord implementation in Flow.Launcher, sequence order of records in the ConcurrentBag does not need to be preserved, as confirmed by the developer. The unordered nature of ConcurrentBag is acceptable for this implementation.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/Storage/TopMostRecord.cs (1)
183-207: Consider clarifying the topmost index calculation logic.The current implementation assumes newer items (at the end of the queue) should have higher priority, which conflicts with the PR objective that states "the most recently pinned item should appear first."
In the comments discussion, users expect that when clicking "Top Most" on an item, it should move to the first position, which is intuitive. The current logic might not align with this expectation.
- // since the latter items should be more recent, we should return the smaller index for score to subtract - // which can make them more topmost - // A, B, C => 2, 1, 0 => (max - 2), (max - 1), (max - 0) + // Items are stored in queue order, but for display we need to consider user expectations + // Users expect their most recently pinned item to appear first (at the top) + // The topmost index affects result ordering, with smaller values appearing higher
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
Support Multiple Topmost Records
Currently, for one query, Flow can only support one topmost record. We should support multiple topmost records because users may want to set many records to be topmost.
The latter record is, the topper it be.
Test
TopMostRecord.jsonwill be converted into new dataMultipleTopMostRecord.json.e.g.
MultipleTopMostRecord.json:
Screenshot:
