Conversation
…r handling Agent-Logs-Url: https://github.com/Flow-Launcher/Flow.Launcher/sessions/bfa04540-ad54-4f04-a7cf-fed5dbc06dcc Co-authored-by: VictoriousRaptor <10308169+VictoriousRaptor@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
197-240:⚠️ Potential issue | 🔴 CriticalThe file has a critical syntax error: missing final return statement and unmatched brace.
The method
EverythingHighlightStringToHighlightList()is missing areturn highlightData;statement after the for loop. The entire file contains only 1 occurrence ofreturn highlightData;(the early return for empty strings), but 2 are required. Additionally, the file has 40 opening braces and only 39 closing braces, confirming it will not compile as written.The function must include
return highlightData;after the loop closes to return the processed highlight data for non-empty inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs` around lines 197 - 240, The method EverythingHighlightStringToHighlightList is missing the final return and a closing brace; after the for loop inside EverythingHighlightStringToHighlightList add "return highlightData;" and ensure you close the method/block so braces are balanced (matching the opening brace for the method/class), keeping the existing early return for empty strings intact.
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
17-21:⚠️ Potential issue | 🔴 CriticalKeep
IsFastSortOption()under_semaphore.This is still the only public native-call path here that bypasses the shared lock. That leaves the
Everything_IsFastSort()/CheckAndThrowExceptionOnError()pair exposed to interleaving native calls from the other methods, so the result and error check are no longer atomic.Verify that
IsFastSortOption()is the only publicEverythingApiDllImportpath without a matching semaphore acquisition. Expected:IsEverythingRunningAsync,SearchAsync, andIncrementRunCounterAsyncall acquire_semaphore;IsFastSortOption()does not.#!/bin/bash rg -n -C2 'public bool IsFastSortOption|public async ValueTask<bool> IsEverythingRunningAsync|public async IAsyncEnumerable<SearchResult> SearchAsync|public async Task IncrementRunCounterAsync|_semaphore\.(Wait|WaitAsync)|EverythingApiDllImport\.' Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs` around lines 17 - 21, IsFastSortOption currently calls EverythingApiDllImport.Everything_IsFastSort(...) and CheckAndThrowExceptionOnError() without acquiring the shared _semaphore, allowing interleaved native calls; wrap the body of IsFastSortOption in a synchronous semaphore acquisition/release (use _semaphore.Wait() before calling Everything_IsFastSort and ensure _semaphore.Release() in a finally block) so the Everything_IsFastSort + CheckAndThrowExceptionOnError pair is executed atomically with the other methods that use _semaphore.
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs (1)
283-283: Remove dead code.
await Task.CompletedTask;at the end of the method serves no purpose and can be removed.Suggested fix
} } - - await Task.CompletedTask; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs` at line 283, Remove the redundant await Task.CompletedTask call at the end of the method in the EverythingApiV3 class; locate the method containing the trailing "await Task.CompletedTask;" (in EverythingApiV3) and delete that statement so the method ends normally without the no-op await. Ensure no other logic depends on that awaited task and run tests/compile to confirm no async signature changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs`:
- Around line 145-166: IsFastSortOption lacks synchronization: wrap the native
DLL interaction in a semaphore acquisition/release using the existing _semaphore
to prevent concurrent native calls; specifically, acquire the semaphore before
calling TryConnectEverything3 and before invoking
Everything3ApiDllImport.Everything3_IsPropertyFastSort, and ensure the semaphore
is released in a finally block. Keep the existing IPCErrorException behavior if
TryConnectEverything3 fails, ensure
Everything3ApiDllImport.Everything3_DestroyClient(client) is still called in
cleanup, and remove the inner CheckAndThrowExceptionOnErrorFromEverything3()
call after DestroyClient (leave only the necessary error check where
appropriate) to avoid rethrowing based on destroy semantics. Ensure exception
safety by releasing _semaphore even when exceptions are thrown.
- Around line 90-94: The code mutates the caller's EverythingSearchOption by
assigning to option.Keyword; instead, read option.Keyword into a local variable
(e.g., keyword or localKeyword), set useRegex = true if
localKeyword.StartsWith("@"), and strip the leading "@" from that local variable
(localKeyword = localKeyword[1..]) before using it in the rest of
EverythingApiV3 logic; do not assign back to option.Keyword so the
EverythingSearchOption object remains unchanged.
---
Outside diff comments:
In `@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs`:
- Around line 197-240: The method EverythingHighlightStringToHighlightList is
missing the final return and a closing brace; after the for loop inside
EverythingHighlightStringToHighlightList add "return highlightData;" and ensure
you close the method/block so braces are balanced (matching the opening brace
for the method/class), keeping the existing early return for empty strings
intact.
---
Duplicate comments:
In `@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs`:
- Around line 17-21: IsFastSortOption currently calls
EverythingApiDllImport.Everything_IsFastSort(...) and
CheckAndThrowExceptionOnError() without acquiring the shared _semaphore,
allowing interleaved native calls; wrap the body of IsFastSortOption in a
synchronous semaphore acquisition/release (use _semaphore.Wait() before calling
Everything_IsFastSort and ensure _semaphore.Release() in a finally block) so the
Everything_IsFastSort + CheckAndThrowExceptionOnError pair is executed
atomically with the other methods that use _semaphore.
---
Nitpick comments:
In `@Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs`:
- Line 283: Remove the redundant await Task.CompletedTask call at the end of the
method in the EverythingApiV3 class; locate the method containing the trailing
"await Task.CompletedTask;" (in EverythingApiV3) and delete that statement so
the method ends normally without the no-op await. Ensure no other logic depends
on that awaited task and run tests/compile to confirm no async signature changes
are needed.
🪄 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: 43220eec-d73e-4a0f-a23d-f1e76991c8d8
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingSearchManager.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingSearchManager.cs:27">
P2: `EnableEverything15Support` is cached in a readonly field, so runtime setting changes won’t switch between legacy and v3 Everything APIs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingSearchManager.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingSearchManager.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs:111">
P2: Destroy the Everything3 client after incrementing run count; the current code leaks unmanaged client handles.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiV3.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
@cubic-dev-ai review |
@jjw24 I have started the AI code review. It will take a few minutes to complete. |
jjw24
left a comment
There was a problem hiding this comment.
@VictoriousRaptor If cubic doesn't update the PR description after the review, could you do so please.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAvailabilityService.cs">
<violation number="1" location="Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAvailabilityService.cs:29">
P2: Use the standard “Everything is not running” message in the non‑1.5 path; the current string reports a 1.5‑specific unavailability even when 1.5 support is disabled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| throw new EngineNotAvailableException( | ||
| Enum.GetName(Settings.IndexSearchEngineOption.Everything)!, | ||
| Localize.flowlauncher_plugin_everything_15_resolution(), | ||
| Localize.flowlauncher_plugin_everything_15_unavailable(), |
There was a problem hiding this comment.
P2: Use the standard “Everything is not running” message in the non‑1.5 path; the current string reports a 1.5‑specific unavailability even when 1.5 support is disabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAvailabilityService.cs, line 29:
<comment>Use the standard “Everything is not running” message in the non‑1.5 path; the current string reports a 1.5‑specific unavailability even when 1.5 support is disabled.</comment>
<file context>
@@ -20,12 +20,26 @@ public async ValueTask EnsureAvailableAsync(IEverythingApi api, CancellationToke
+ throw new EngineNotAvailableException(
+ Enum.GetName(Settings.IndexSearchEngineOption.Everything)!,
+ Localize.flowlauncher_plugin_everything_15_resolution(),
+ Localize.flowlauncher_plugin_everything_15_unavailable(),
+ Constants.EverythingErrorImagePath,
+ ClickToInstallEverythingAsync);
</file context>
| Localize.flowlauncher_plugin_everything_15_unavailable(), | |
| Localize.flowlauncher_plugin_everything_is_not_running(), |
There was a problem hiding this comment.
Do we still use x86? Maybe these can be removed?
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.