Improve update logic & Fix update logic issue & 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.
|
Warning Rate limit exceeded@Jack251970 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change updates the query construction and management logic across the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant MainViewModel
participant QueryBuilder
participant Query
User->>MainWindow: Types or edits query
MainWindow->>MainViewModel: Passes QueryText (raw input)
MainViewModel->>QueryBuilder: Build(raw input, processed text, plugins)
QueryBuilder->>Query: Constructs Query (with Input property)
Query-->>MainViewModel: Returns Query object
MainViewModel->>MainViewModel: Tracks _progressQuery and _updateQuery
MainViewModel->>MainWindow: Updates UI/results
Suggested reviewers
Poem
✨ 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: 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.
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)
36-37: Consider more descriptive names for query tracking variables.While
_progressQueryand_updateQueryare good names with helpful comments, consider renaming them to even more descriptive names to make their purposes clearer.-private Query _progressQuery; // Used for QueryResultAsync -private Query _updateQuery; // Used for ResultsUpdated +private Query _progressQuery; // Tracks active query for progress bar visibility +private Query _updateQuery; // Used to validate incoming ResultsUpdated events
1286-1286:⚠️ Potential issueFix the TaskScheduler usage - it's not awaitable.
TaskScheduler.Defaultis not awaitable and this will cause a compilation error. You need to use a construct that actually returns a Task.-// Switch to ThreadPool thread -await TaskScheduler.Default; +// Switch to ThreadPool thread +await Task.Yield();
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
247-270: Consider extracting duplicate validation logic.The validation check at line 247 is repeated at line 267. Consider extracting this into a helper method to avoid duplication and make future maintenance easier.
+private bool IsQueryStillValid(Query eventQuery, CancellationToken token) +{ + return _updateQuery != null && eventQuery.RawQuery == _updateQuery.RawQuery && !token.IsCancellationRequested; +} plugin.ResultsUpdated += (s, e) => { - if (_updateQuery == null || e.Query.RawQuery != _updateQuery.RawQuery || e.Token.IsCancellationRequested) + if (!IsQueryStillValid(e.Query, e.Token)) { return; } // processing logic... - if (_updateQuery == null || e.Query.RawQuery != _updateQuery.RawQuery || token.IsCancellationRequested) + if (!IsQueryStillValid(e.Query, token)) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/QueryBuilder.cs(2 hunks)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 (2)
- Flow.Launcher/MainWindow.xaml.cs
- Flow.Launcher.Core/Plugin/QueryBuilder.cs
🧰 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 (1)
- GitHub Check: build
🔇 Additional comments (10)
Flow.Launcher/ViewModel/MainViewModel.cs (10)
36-37: Good replacement of boolean flag with more descriptive Query objects.Replacing the single boolean
_isQueryRunningflag with two separateQueryobjects (_progressQueryand_updateQuery) provides better state tracking and clarity about which query is being used for which purpose.
247-250: Improved update logic validation with proper Query comparison.The updated condition now correctly compares
e.Query.RawQuerywith_updateQuery.RawQueryinstead of comparing withQueryText, addressing the core issue mentioned in the PR objective. This ensures that the comparison is between the same types of data.
267-270: Good addition of secondary validation check after processing.Adding a second check after processing the results ensures that the query is still valid before writing the results to the channel, providing an extra layer of protection against race conditions.
380-380: Updated Build method call to include both raw and processed query.This change correctly passes both the original
QueryTextand its trimmed version to align with the updated signature ofQueryBuilder.Build.
1244-1244: Good defensive programming to initialize _progressQuery.Explicitly setting
_progressQueryto null at the start of the method ensures a clean query state before processing begins.
1272-1273: Effective early return to prevent processing outdated queries.Adding this check prevents running queries that no longer match the current user input, which should improve performance and responsiveness when typing quickly.
1282-1283: Clear assignment of tracking queries to improve state management.Setting both
_progressQueryand_updateQueryat the same point in the code flow makes it easier to understand and maintain the query state.
1343-1351: Improved progress bar handling based on _progressQuery state.The progress bar visibility is now controlled by checking if
_progressQueryis not null and matches the current query, which provides a more accurate representation of the current processing state than the previous boolean flag.
1268-1405: Added try-finally block for robust cleanup of _progressQuery.Wrapping the query execution in a try-finally block ensures that
_progressQueryis always cleared even if exceptions occur, preventing the progress bar from getting stuck in a visible state.
1505-1505: Updated ConstructQueryAsync return to include original input.The method now correctly returns the result of
QueryBuilder.Buildwith both the originalQueryTextand the expanded query string, which aligns with the updated method signature.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1285-1286:⚠️ Potential issueTaskScheduler.Default is not awaitable
await TaskScheduler.Default;will not compile becauseTaskSchedulerdoesn't implementGetAwaiter().-// Switch to ThreadPool thread -await TaskScheduler.Default; +// Switch to ThreadPool thread +await Task.Yield(); // or `await Task.Run(() => {}, token);`
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
36-1506: Consider renaming variables for better clarityWhile the code logic has improved significantly, the variable names
_progressQueryand_updateQuerycould be more descriptive to better indicate their specific purposes.-private Query _progressQuery; // Used for QueryResultAsync -private Query _updateQuery; // Used for ResultsUpdated +private Query _executingQuery; // Query currently being executed in QueryResultAsync +private Query _activeQuery; // Query currently being used to validate result updates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/ViewModel/MainViewModel.cs(10 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 (1)
- GitHub Check: build
🔇 Additional comments (12)
Flow.Launcher/ViewModel/MainViewModel.cs (12)
36-37: Improved query state tracking with dedicated objectsThis change replaces the boolean flag
_isQueryRunningwith two explicit Query objects that provide better state tracking for different scenarios. This is a good design improvement as it allows for more precise query state management.
247-250: Fixed update logic to properly compare query objectsThis fixes the core issue mentioned in the PR where the update logic was incorrectly comparing
e.Query.RawQuerywithQueryText. Now it properly comparese.Query.RawQuerywith_updateQuery.RawQuery, ensuring that only relevant updates are processed.
267-270: Added additional validation check to prevent stale updatesThis second validation check ensures that by the time results are processed, the query is still valid. This prevents potential race conditions where the query changed between the initial check and when the results are about to be added to the update channel.
380-380: Updated Backspace command to pass both raw and trimmed queryThe change aligns with the updated QueryBuilder.Build signature that now requires both the raw input and the processed query text. This ensures consistency with the rest of the codebase.
1244-1244: Explicitly reset query state at the beginning of query executionThis ensures that the progress query state starts clean before each query execution, preventing potential state leakage from previous queries.
1268-1273: Added early return to prevent processing outdated queriesThis important check prevents processing queries that no longer match the current user input, which helps with responsiveness when typing quickly. This directly addresses the PR objective of improving interface updates for rapid input.
1282-1283: Set both query tracking objects after token creationSetting both
_progressQueryand_updateQueryat this point ensures they're properly initialized with the current query before any async operations, which is crucial for proper state tracking.
1341-1348: Improved progress bar visibility logicThe progress bar visibility is now based on checking if the current query matches the
_progressQueryreference instead of a boolean flag. This is more robust as it prevents showing the progress bar for outdated queries.
1401-1405: Added try-finally block to ensure cleanupThe try-finally block ensures that
_progressQueryis always reset, even if an exception occurs during query execution. This prevents the UI from getting stuck in a loading state.
1408-1409: Updated QueryTaskAsync with isHomeQuery parameterThis parameter addition clarifies whether the query is a home query, which allows for different handling logic. The method now makes the distinction explicit rather than relying on other contextual clues.
1485-1486: Updated QueryBuilder.Build call to use consistent parametersThis change aligns with the updated method signature to accept both raw input and processed text, maintaining consistency across the codebase.
1505-1506: Passing both raw and processed query to QueryBuilder.BuildSimilarly to the previous comment, this ensures the Build method receives the complete context it needs - both the original user input (QueryText) and the expanded/processed query text.
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.
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.
@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 ...
|
Jack251970
changed the title
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.Test