Fix null origin query issue #3530
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the OriginQuery null issue by updating the handling of OriginQuery in topmost results and plugin info context menus so that the previous null checks can be removed.
- Removed conditional null-checks around result.OriginQuery in history recording and record management.
- Updated ContextMenuPluginInfo signature to accept a Result instead of a plugin ID.
- Propagated OriginQuery values in context menu result objects.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Flow.Launcher/ViewModel/MainViewModel.cs | Removed null-check on result.OriginQuery and added its propagation to context menus |
| Flow.Launcher/Storage/TopMostRecord.cs | Removed null-checks for result.OriginQuery in record lookup, removal and update logic |
Comments suppressed due to low confidence (2)
Flow.Launcher/ViewModel/MainViewModel.cs:447
- Removing the null-check for result.OriginQuery may lead to a NullReferenceException if result.OriginQuery is unexpectedly null. Ensure that result.OriginQuery is always non-null at this point or add alternative error handling.
_history.Add(result.OriginQuery.RawQuery);
Flow.Launcher/Storage/TopMostRecord.cs:15
- The removal of the null-check for result.OriginQuery could cause a null dereference when accessing RawQuery. Confirm that result.OriginQuery is guaranteed to be non-null or implement a safeguard.
if (records.IsEmpty || !records.TryGetValue(result.OriginQuery.RawQuery, out var value))
@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 ...
|
|
🥷 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: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis change removes explicit null checks for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainViewModel
participant TopMostRecord
User->>MainViewModel: Selects a result (possibly with context menu)
MainViewModel->>TopMostRecord: AddOrUpdate(result)
Note right of TopMostRecord: No null check on result.OriginQuery<br/>Directly uses result.OriginQuery.RawQuery
MainViewModel->>MainViewModel: Add result.OriginQuery.RawQuery to history (no null check)
Possibly related PRs
Suggested labels
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: 0
🔭 Outside diff range comments (2)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1616-1643: 💡 Verification agent🧩 Analysis chain
Robustness concerns in
ContextMenuPluginInfo
PluginManager.GetPluginForId(id)may returnnullif the plugin is disabled/uninstalled between query & click.metadata.Websitecan benull→API.OpenUrl(null)might throw or open an empty tab.- No glyph/icon is set; consider adding one for UI consistency.
- var metadata = PluginManager.GetPluginForId(id).Metadata; + var pluginPair = PluginManager.GetPluginForId(id); + if (pluginPair is null) + return new Result { Title = App.API.GetTranslation("pluginNotFound"), OriginQuery = result.OriginQuery }; + var metadata = pluginPair.Metadata; ... - Action = _ => - { - App.API.OpenUrl(metadata.Website); - return true; - }, + Action = _ => + { + if (!string.IsNullOrWhiteSpace(metadata.Website)) + App.API.OpenUrl(metadata.Website); + else + App.API.ShowMsg(metadata.Name, App.API.GetTranslation("websiteNotProvided")); + return true; + },
I’ll verify the nullability and property definitions before updating the comment.
🏁 Script executed:
#!/bin/bash set -e echo "1) Signature and nullability of GetPluginForId:" rg -n "GetPluginForId" -C 2 echo echo "2) Definition of PluginMetadata and its Website, IcoPath, PluginDirectory properties:" rg -n "class PluginMetadata" -C 3 rg -n "Website" -C 2 rg -n "IcoPath" -C 2 rg -n "PluginDirectory" -C 2Length of output: 71390
Digging into
PluginMetadatato confirm nullability ofWebsiteandIcoPath:#!/bin/bash # Show PluginMetadata definition sed -n '1,200p' Flow.Launcher.Plugin/PluginMetadata.cs
Handle missing plugin and website in ContextMenuPluginInfo
Ensure you don’t dereference a null plugin or open a null/empty URL:
• Add a null‐check for
GetPluginForId(id)and return a “plugin not found” result.
• Guardmetadata.Websiteagainst null/whitespace before callingOpenUrl.File: Flow.Launcher/ViewModel/MainViewModel.cs (around lines 1616–1643)
- var metadata = PluginManager.GetPluginForId(id).Metadata; + var pluginPair = PluginManager.GetPluginForId(id); + if (pluginPair is null) + return new Result + { + Title = App.API.GetTranslation("pluginNotFound"), + OriginQuery = result.OriginQuery + }; + var metadata = pluginPair.Metadata; … - Action = _ => - { - App.API.OpenUrl(metadata.Website); - return true; - }, + Action = _ => + { + if (!string.IsNullOrWhiteSpace(metadata.Website)) + App.API.OpenUrl(metadata.Website); + return true; + },Flow.Launcher/Storage/TopMostRecord.cs (1)
14-39:⚠️ Potential issueEliminated null-checks can crash when
OriginQueryis absent
result.OriginQuery.RawQueryis now dereferenced in three places without guarding againstnull.
While the PR’s intent is to guarantee this property, the guarantee relies on every code path (including 3rd-party plugins) honouring it. A single oversight will take down the app.Suggested defensive code (minimal overhead):
- if (records.IsEmpty || !records.TryGetValue(result.OriginQuery.RawQuery, out var value)) + var raw = result.OriginQuery?.RawQuery; + if (string.IsNullOrEmpty(raw) || records.IsEmpty || !records.TryGetValue(raw, out var value))Replicate the same pattern in
RemoveandAddOrUpdate.Alternatively, mark
Result.OriginQueryas non-nullable in the model and enforce it via constructor or factory methods.
🧹 Nitpick comments (1)
Flow.Launcher/ViewModel/MainViewModel.cs (1)
1154-1157:ContextMenuPluginInfoinsertion looks good, but mind the orderingAdding the plugin-info entry after the Top-Most item changes the order that users see items. If UI order matters (e.g., consistency with previous releases), consider inserting it before
results.Add(ContextMenuTopMost(selected))or document the new order in release notes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/Storage/TopMostRecord.cs(1 hunks)Flow.Launcher/ViewModel/MainViewModel.cs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Flow.Launcher/ViewModel/MainViewModel.cs (3)
445-449: PotentialNullReferenceExceptionwhen adding to history
result.OriginQueryis now assumed to be non-null (_history.Add(result.OriginQuery.RawQuery)), but 3rd-party plugins (or future internal code paths) can still create aResultwithout populatingOriginQuery.
If that happens Flow crashes as soon as a user executes such a result.- _history.Add(result.OriginQuery.RawQuery); + if (result.OriginQuery is not null) + _history.Add(result.OriginQuery.RawQuery);Alternatively, add a contract/assertion earlier in the pipeline (e.g., when building
Result) to guarantee non-nullness and fail fast during development rather than at runtime.Please grep for
new Result(patterns that omitOriginQueryto confirm whether any remain.
1590-1592: 👍 OriginQuery now propagated to Top-Most “remove” menuPropagating
OriginQueryensures downstream consumers (TopMostRecord, history, etc.) can rely on its presence. No issues spotted.
1608-1610: 👍 OriginQuery now propagated to Top-Most “add” menuSame remark as above – this removes the need for null guards elsewhere.
2 participants
Follow on with #3203.
Add
OriginQueryfor topmost results & plugin info results so that we can removeresult.OriginQuerynull check.