Improve update logic & Fix update logic issue & Add Input for Query #3502
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the update logic in the main view model by refactoring how running and current queries are tracked and by updating the QueryBuilder.Build method signature to accept separate input and trimmed text parameters.
- Refactored MainViewModel to replace _isQueryRunning with two distinct query state variables.
- Updated QueryBuilder.Build calls in view model, tests, and main window to pass both raw and trimmed query text.
- Added documentation for the new Query.Input property and corresponding changes in QueryBuilder.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Refactored query state tracking and updated QueryBuilder.Build invocations. |
| Flow.Launcher/MainWindow.xaml.cs | Updated QueryBuilder.Build call for consistency in query text handling. |
| Flow.Launcher.Test/QueryBuilderTest.cs | Adjusted tests to accommodate the updated QueryBuilder.Build signature. |
| Flow.Launcher.Plugin/Query.cs | Added Input property documentation. |
| Flow.Launcher.Core/Plugin/QueryBuilder.cs | Changed Build method signature to accept both raw input and trimmed text. |
Comments suppressed due to low confidence (1)
Flow.Launcher/ViewModel/MainViewModel.cs:372
- [nitpick] Ensure that passing the untrimmed QueryText as the first parameter while the second parameter is trimmed is the intended behavior, and document this design choice for clarity.
var query = QueryBuilder.Build(QueryText, QueryText.Trim(), PluginManager.NonGlobalPlugins);
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
This comment has been minimized.
This comment has been minimized.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an untrimmed input field Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant MainWindow
participant MainViewModel
participant QueryBuilder
participant Plugins
User->>MainWindow: type rawInput
MainWindow->>MainViewModel: Submit QueryText (rawInput)
MainViewModel->>QueryBuilder: Build(rawInput, trimmedText, plugins)
Note right of QueryBuilder #eef6ff: Sets Query.Input = rawInput\nand other Query fields
QueryBuilder-->>MainViewModel: Query { Input, Search, RawQuery, ... }
MainViewModel->>MainViewModel: set _progressQuery / _updateQuery
MainViewModel->>Plugins: run plugin queries (async, cancellable)
alt query still current
MainViewModel->>MainWindow: show progress, update results
else cancelled or replaced
MainViewModel->>MainWindow: clear/hide results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)Flow.Launcher/Helper/ResultHelper.cs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
239-265: Redundant guards – can be collapsed for clarityThe same three-part condition (
_currentQuery == null || e.Query.RawQuery != _currentQuery.RawQuery || …) is evaluated twice, once before cloning the results and once after.
Keeping a single guard at the top of the handler avoids duplication and a tiny amount of wasted work.- if (_currentQuery == null || e.Query.RawQuery != _currentQuery.RawQuery || e.Token.IsCancellationRequested) - { - return; - } - - var token = e.Token == default ? _updateSource.Token : e.Token; - … - if (_currentQuery == null || e.Query.RawQuery != _currentQuery.RawQuery || token.IsCancellationRequested) - { - return; - } + var token = e.Token == default ? _updateSource.Token : e.Token; + if (_currentQuery == null || + e.Query.RawQuery != _currentQuery.RawQuery || + token.IsCancellationRequested) + { + return; + }
372-373: Minor readability nit – avoid double-trim
QueryTextis already passed as the raw input. CallingQueryText.Trim()for the second parameter means the string is trimmed twice insideBuild. You can save an allocation by trimming once:-var query = QueryBuilder.Build(QueryText, QueryText.Trim(), PluginManager.NonGlobalPlugins); +var trimmed = QueryText.Trim(); +var query = QueryBuilder.Build(QueryText, trimmed, PluginManager.NonGlobalPlugins);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Flow.Launcher.Core/Plugin/QueryBuilder.cs(3 hunks)Flow.Launcher.Plugin/Query.cs(1 hunks)Flow.Launcher.Test/QueryBuilderTest.cs(3 hunks)Flow.Launcher/MainWindow.xaml.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (11)
Flow.Launcher.Plugin/Query.cs (1)
10-14: Good addition of theInputproperty with clear documentationThe new
Inputproperty with appropriate XML documentation clearly explains its purpose and usage guidance. The warning about usingSearchproperty instead is consistent with the documentation forRawQuery.Flow.Launcher/MainWindow.xaml.cs (1)
411-411: UpdatedQueryBuilder.Buildcall with proper parametersThe call has been correctly updated to pass both the raw query text and the trimmed text as separate parameters, aligning with the updated method signature in
QueryBuilder.cs.Flow.Launcher.Test/QueryBuilderTest.cs (3)
19-19: Test adjusted to use updatedQueryBuilder.BuildsignatureThe test call has been properly updated to include the raw input parameter, maintaining test integrity while accommodating the API change.
42-42: Test adjusted to use updatedQueryBuilder.BuildsignatureThe test call has been properly updated to include the raw input parameter, maintaining test integrity while accommodating the API change.
54-54: Test adjusted to use updatedQueryBuilder.BuildsignatureThe test call has been properly updated to include the raw input parameter, maintaining test integrity while accommodating the API change.
Flow.Launcher.Core/Plugin/QueryBuilder.cs (4)
9-9: Method signature updated to include the raw input parameterThe
Buildmethod signature has been enhanced to accept the raw input as a separate parameter, which will be used to populate the newInputproperty on theQueryclass.
14-15: Minor formatting changeSmall formatting adjustment with no functional impact.
25-26: Additional whitespace added for readabilitySmall formatting improvement to enhance code readability.
Also applies to: 32-33
42-42: Correctly setting the newInputpropertyThe new
Inputproperty is properly populated with the raw input parameter, completing the implementation of this feature.Flow.Launcher/ViewModel/MainViewModel.cs (2)
1274-1281: Potential visibility race on_runningQuery
_runningQueryis written on the UI thread and later read from aTask.Delaycontinuation running on a ThreadPool thread.
Although reference assignments are atomic, withoutvolatile/Interlockedthere is no publish-happens-before relation; the ThreadPool may observe a stale value.If correctness depends on the most recent write, consider:
private volatile Query _runningQuery;or guarding access with a lock/
Interlocked.Exchange.Would you verify if stale reads could surface in practice? If so, I can help prepare a patch.
1396-1397: Verify parameter order matchesQueryBuilder.BuildsignatureJust a sanity check:
Build(rawInput, processedText, nonGlobalPlugins)is expected.
Confirm thatQueryText(raw) andqueryBuilder.ToString().Trim()(processed) are in the correct order to ensureQuery.Inputholds the unmodified user text.
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (2)
35-36: Improved state tracking with dedicated Query objectsThis is a good improvement over using a boolean flag. Tracking queries as objects allows for more precise state management and validation.
The existing suggestion to rename these variables to
_progressQueryand_updateQueryto more clearly indicate their purposes is still valid.
1254-1255:⚠️ Potential issueCompile-time error:
TaskScheduleris not awaitable
await TaskScheduler.Default;will not compile becauseTaskSchedulerdoes not implementGetAwaiter().-// Switch to ThreadPool thread -await TaskScheduler.Default; +// Switch to ThreadPool thread +await Task.Yield(); // or `await Task.Run(() => {}, _updateSource.Token);`
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
260-263: Redundant validation checkThis is a duplicate of the check on line 240. While it's valid to verify conditions haven't changed, consider extracting this validation to a helper method to avoid duplication.
-if (_updateQuery == null || e.Query.RawQuery != _updateQuery.RawQuery || token.IsCancellationRequested) -{ - return; -} +if (IsQueryMismatchOrCancelled(_updateQuery, e.Query, token)) +{ + return; +} // Add this helper method to the class +private bool IsQueryMismatchOrCancelled(Query currentQuery, Query eventQuery, CancellationToken token) +{ + return currentQuery == null || eventQuery.RawQuery != currentQuery.RawQuery || token.IsCancellationRequested; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (9)
Flow.Launcher/ViewModel/MainViewModel.cs (9)
240-240: Fixed update logic comparisonThis fixes the core issue where the update logic incorrectly compared
e.Query.RawQuerywithQueryTextinstead of comparing equivalent objects.
373-373: Updated QueryBuilder.Build call with Input parameterCorrectly updated to pass both raw input and trimmed text to match the updated signature.
1221-1221: Reset query state before processingGood practice to initialize the query state before starting a new query process.
1241-1246: Early return optimization for changed queriesThis is a good optimization to prevent processing outdated queries when user input changes rapidly.
1251-1252: Improved query state trackingSetting both the progress and update query objects provides better tracking of the query lifecycle.
1291-1302: Progress bar visibility linked to query stateGood implementation of delayed progress bar visibility that also checks if the query is still active before showing it.
1325-1332: Reset progress query after completionProperly manages the progress state and progress bar visibility after query execution completes.
1334-1338: Added safety with try-finally blockThe finally block ensures the query state is cleaned up even if an exception occurs, preventing state leakage.
1414-1414: Updated call to match new QueryBuilder.Build signatureCorrectly updated to provide both the original input and the processed query string.
|
Until we can actually figure out what's causing it. |
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: theClueless 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:
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 |
This comment has been minimized.
This comment has been minimized.
|
@jjw24 I failed to reproduce this issue during today's usage and fast typing. But since the first issue you pointed out is related to https://ci.appveyor.com/project/JohnTheGr8/flow-launcher/builds/52791393/artifacts |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/MainWindow.xaml.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-08T21:12:12.060Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
Flow.Launcher.Core/Plugin/QueryBuilder.cs (2)
Query(9-62)QueryBuilder(7-63)Flow.Launcher.Plugin/Query.cs (2)
Query(8-113)ToString(112-112)Flow.Launcher.Core/Plugin/PluginManager.cs (5)
PluginManager(23-821)PluginManager(161-167)ICollection(305-328)ICollection(330-333)PluginPair(459-462)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Flow.Launcher/ViewModel/MainViewModel.cs (6)
39-40: Naming and intent are clear; keep.The split between progress tracking and update gating reads well. Consider brief XML docs to cement intent for future maintainers.
286-293: Harden ResultsUpdated gating and avoid mixed-token checks.Use the resolved token for both the cancellation check and keep comparisons ordinal for perf/correctness on raw text.
- if (_updateQuery == null || e.Query.Input != _updateQuery.Input || e.Token.IsCancellationRequested) + var token = e.Token == default ? _updateToken : e.Token; + if (token.IsCancellationRequested + || _updateQuery == null + || !string.Equals(e.Query.Input, _updateQuery.Input, StringComparison.Ordinal)) { return; } - - var token = e.Token == default ? _updateToken : e.Token;
1417-1436: Progress-bar trigger: compare with ordinal and consider snapshot.Use ordinal comparison for raw text; if you adopt the input snapshot above, compare against it for extra safety.
- if (_progressQuery != null && _progressQuery.Input == query.Input) + if (_progressQuery != null && + string.Equals(_progressQuery.Input, query.Input, StringComparison.Ordinal)) { ProgressBarVisibility = Visibility.Visible; }
1687-1702: Minor:_lastQueryis never null in ctor; simplify null-propagation.You can drop the null-conditional on
_lastQueryfor a tiny clarity bump. Not a blocker.Based on learnings
- if (_lastQuery?.ActionKeyword != query?.ActionKeyword) + if (_lastQuery.ActionKeyword != query?.ActionKeyword)
1350-1415: Potential cross-thread property updates (minor).Property assignments (e.g., PluginIconPath, SearchIconVisibility) happen after the thread hop in the current code. With the compile fix above (use Task.Yield), you’ll continue on the UI context and avoid cross-thread UI-binding surprises.
286-323: Use ordinal comparisons consistently for raw text.All string-equality checks on Input/RawQuery should be
StringComparison.Ordinal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-08T21:12:12.060Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
🧬 Code graph analysis (1)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
Flow.Launcher.Core/Plugin/QueryBuilder.cs (2)
Query(9-62)QueryBuilder(7-63)Flow.Launcher.Plugin/Query.cs (2)
Query(8-113)ToString(112-112)Flow.Launcher.Core/Plugin/PluginManager.cs (3)
PluginManager(23-821)PluginManager(161-167)PluginPair(459-462)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (14)
Flow.Launcher/ViewModel/MainViewModel.cs (14)
444-452: Correct: Build now receives both Input (untrimmed) and text (trimmed).This aligns Backspace with the new QueryBuilder API and preserves user input semantics.
1337-1345: Don’t abort after shortcut expansion (stale guard).ConstructQueryAsync can rewrite QueryText (builtin/custom shortcuts). Comparing
query.Inputwith the liveQueryTextright after that will often differ and prematurely return, yielding an empty/stale result pane after expansions.Apply this to snapshot the input before constructing the query and relax the guard to allow either the original input or the expanded RawQuery:
- var query = await ConstructQueryAsync(QueryText, Settings.CustomShortcuts, Settings.BuiltinShortcuts); + var inputSnapshot = QueryText; + var query = await ConstructQueryAsync(inputSnapshot, Settings.CustomShortcuts, Settings.BuiltinShortcuts); @@ - // Check if the input text matches the query text - if (query.Input != QueryText) return; + // Short-circuit only if user has typed something new unrelated to this query + var currentText = QueryText; + if (!string.Equals(currentText, inputSnapshot, StringComparison.Ordinal) && + !string.Equals(currentText, query.RawQuery, StringComparison.Ordinal)) + { + return; + }
1485-1499: Result-clearing logic looks correct; good early return when no update tasks.This prevents stale results when home-page plugins are all disabled or absent.
If you want to be extra sure, test with:
- ShowHistoryResultsForHomePage = false
- All home plugins disabled
- Enter/clear text to confirm the view clears.
1596-1599: Good: Build for whitespace path sets both Input and RawQuery empty.This keeps IsHomeQuery true and avoids null queries.
1618-1619: Good: pass the method parameter as Input to avoid races.Using
queryTexthere ensures the built Query carries the exact input used for construction.
217-277: Channel consumer resiliency looks fine; continuation restarts on fault.No action needed.
571-588: UI navigation helpers unchanged; LGTM.Selection and preview update wiring remains consistent.
Also applies to: 607-622
1510-1517: ClearResults helper is cohesive and idempotent.Approach to collapse/clear/reset states is sound.
1523-1572: Per-plugin query path is correct; good use of Task.Yield and cloning.No issues spotted.
327-338: Clock/date timer ok.PeriodicTimer loop is simple and cancellation-free as intended.
1951-1958: Dialog focus restore is guarded and logged; ok.No changes requested.
2276-2303: Dispose pattern correct; writers completed and tasks disposed.Looks good.
1379-1381: Compile-time error:TaskScheduleris not awaitable.
await TaskScheduler.Default;won’t compile and also risks property updates off the UI thread. Use an awaitable construct; you don’t need to hop threads here since plugin work is already offloaded per-plugin.- // Switch to ThreadPool thread - await TaskScheduler.Default; + // Yield once without changing context (plugin queries run on background tasks) + await Task.Yield();
1335-1499: No extraawait TaskScheduler.Defaultor stale-query guards found
Only the await at MainViewModel.cs:1380 and the input guard at 1363 exist.
This comment has been minimized.
This comment has been minimized.
Jack251970
force-pushed
the
improve_update
branch
from
7e1c441 to
5f10e02
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Jack251970
changed the title
This comment has been minimized.
This comment has been minimized.
Updated comments across multiple files to standardize the phrasing from "work around" to "workaround" for consistency and clarity. Changes were made in `Directory.Build.props`, `Flow.Launcher.csproj`, and `MainWindow.xaml.cs`. These updates are non-functional and only affect documentation within the codebase.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher/MainWindow.xaml.cs (1)
862-862: Fix forbidden pattern: "work around" → "workaround".The phrase "work around" should be written as one word to comply with the project's style guidelines.
Apply this diff:
- // Initialize call twice to work around multi-display alignment issue- https://github.com/Flow-Launcher/Flow.Launcher/issues/2910 + // Initialize call twice to workaround multi-display alignment issue- https://github.com/Flow-Launcher/Flow.Launcher/issues/2910
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1483-1495: Remove duplicate assignment of_progressQuery = null.The
_progressQueryfield is set tonullat line 1483 within the try block and again at line 1494 in the finally block. The finally block assignment is sufficient to ensure cleanup in all cases.Apply this diff:
if (currentCancellationToken.IsCancellationRequested) return; // this should happen once after all queries are done so progress bar should continue // until the end of all querying - _progressQuery = null; if (!currentCancellationToken.IsCancellationRequested) { // update to hidden if this is still the current query ProgressBarVisibility = Visibility.Hidden; } } finally { // this make sures progress query is null when this query is canceled _progressQuery = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/MainWindow.xaml.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-08T21:12:12.060Z
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
Applied to files:
Flow.Launcher/ViewModel/MainViewModel.cs
🧬 Code graph analysis (2)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
Flow.Launcher.Plugin/Query.cs (2)
Query(8-113)ToString(112-112)Flow.Launcher.Core/Plugin/QueryBuilder.cs (2)
Query(9-62)QueryBuilder(7-63)Flow.Launcher.Core/Plugin/PluginManager.cs (5)
PluginManager(23-821)PluginManager(161-167)ICollection(305-328)ICollection(330-333)PluginPair(459-462)
Flow.Launcher/MainWindow.xaml.cs (2)
Flow.Launcher.Core/Plugin/QueryBuilder.cs (1)
QueryBuilder(7-63)Flow.Launcher.Core/Plugin/PluginManager.cs (2)
PluginManager(23-821)PluginManager(161-167)
🪛 GitHub Actions: Check Spelling
Flow.Launcher/MainWindow.xaml.cs
[error] 862-862: Forbidden pattern: 'work around' matches a line_forbidden.patterns entry: '\bwork[- ]arounds?\b'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
Flow.Launcher/MainWindow.xaml.cs (1)
479-479: LGTM! Correct usage of the new QueryBuilder.Build signature.The call correctly passes the untrimmed
QueryTextBox.Textas the input parameter and the trimmed version as the text parameter, aligning with the new Input property feature.Flow.Launcher/ViewModel/MainViewModel.cs (7)
39-40: LGTM! Query-based tracking improves state management.Replacing the boolean
_isQueryRunningwith Query-based tracking fields provides more precise cancellation and state management by tracking which specific query is in progress.
286-286: LGTM! Correct use of Input property for stale query detection.The guard now compares
e.Query.Inputwith_updateQuery.Input, which correctly identifies stale queries by comparing the untrimmed original input rather than the trimmed/expandedRawQuery.Based on PR objectives and author clarification to reviewer taooceros.
444-444: LGTM! Correct usage of the new QueryBuilder.Build signature.The call correctly passes
QueryText(untrimmed) as the input parameter andQueryText.Trim()as the text parameter.
1375-1376: Verify thatawait TaskScheduler.Default;compiles in your environment.The code
await TaskScheduler.Default;is not standard awaitable syntax.TaskSchedulerdoes not implementGetAwaiter()in the standard .NET libraries.If this compiles in your environment, you may be using a Visual Studio extension or custom awaiter. However, this will fail in most standard .NET environments.
Standard alternatives for switching to ThreadPool:
If the current syntax doesn't compile or you want more portable code, use:
-// Switch to ThreadPool thread -await TaskScheduler.Default; +// Switch to ThreadPool thread +await Task.Run(() => { });Or if you only need to yield execution without guaranteeing a thread switch:
-// Switch to ThreadPool thread -await TaskScheduler.Default; +// Yield execution +await Task.Yield();Note:
Task.Yield()queues continuation but doesn't guarantee thread switch, whileTask.Run(() => { })ensures execution on ThreadPool.Based on past review comments indicating this was discussed with reviewer taooceros.
1424-1434: LGTM! Correct progress bar timing with Input property comparison.The progress bar logic correctly uses
_progressQuery.Input == query.Inputto verify that the current query is still active before showing the progress indicator.
1594-1594: LGTM! Correct handling of empty input.For empty or whitespace input, both the
inputandtextparameters are set tostring.Empty, correctly indicating a home query with no user input.
1614-1614: LGTM! Correctly uses method parameter to avoid race condition.The call correctly passes the immutable
queryTextparameter (not the mutableQueryTextproperty) as the input argument, preventing race conditions where the property value could change between query construction and validation.Based on past review comments indicating this race condition was previously flagged and fixed.
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. Forbidden patterns 🙅 (1)In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves. These forbidden patterns matched content: s.b. workaround(s)If the flagged items are 🤯 false positivesIf items relate to a ...
|
3 participants
From #3314. Resolve #2605.
Improve update logic
Improve main view model update logic.
Fix update logic issue
We should not use
e.Query.RawQuery != QueryTextbecauseQueryText(Input) is different fromRawQuery(Query).Add new property Input for Query
Inputis the property without trimming whitespace and it can help developer get the raw input.Clear results when there are no update tasks
In this function, we need to reset all things because there are no update tasks.
Test