Fix crash when opening urls with non-existent local file #4004
Conversation
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.
@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: PatternIf the flagged items are 🤯 false positivesIf items relate to a ...
|
Jack251970
changed the title
|
🥷 Code experts: Jack251970, onesounds Jack251970, onesounds have 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: ✨ Comment |
📝 WalkthroughWalkthroughAdds a new localized string key for file-not-found and a helper to check file-or-directory existence; updates PublicAPIInstance.OpenUri to pre-check existence for file URIs, show a localized error if missing, and wrap non-browser Process.Start in try/catch with logging and user-visible error. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as PublicAPIInstance.OpenUri
participant FS as FileSystem (File/Directory)
participant Proc as Process.Start
participant UI as ShowMsgError
participant Log as Logger
User->>API: OpenUri(uri)
alt uri points to file system path
API->>FS: FileOrLocationExists(path)?
alt not exists
API-->>UI: Show "fileNotFoundError" (localized)
note right of API: return early
else exists
API->>Proc: Start(path) [non-browser]
alt throws
API->>Log: Log exception
API-->>UI: Show error message
end
end
else other URI (browser)
API->>Proc: Start(uri) [browser]
alt throws
API->>Log: Log exception
API-->>UI: Show error message
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (2)
Flow.Launcher/PublicAPIInstance.cs (2)
415-420: Pre-check fixes the crash; add log and minor tidy.Log the miss to aid diagnostics and avoid re-evaluating uri.LocalPath twice.
- if (uri.IsFile && !File.Exists(uri.LocalPath) && !Directory.Exists(uri.LocalPath)) - { - ShowMsgError(GetTranslation("errorTitle"), string.Format(GetTranslation("fileNotFoundError"), uri.LocalPath)); - return; - } + if (uri.IsFile) + { + var localPath = uri.LocalPath; + if (!File.Exists(localPath) && !Directory.Exists(localPath)) + { + LogWarn(ClassName, $"OpenUri path not found: {localPath}"); + ShowMsgError(GetTranslation("errorTitle"), + string.Format(GetTranslation("fileNotFoundError"), localPath)); + return; + } + }
450-463: Use LocalPath for file URIs; optionally specialize error UX.Using AbsoluteUri for file: targets can mis-handle percent‑encoding; prefer LocalPath. Optional: catch Win32Exception 1155 (“no association”) and show a friendly, localized message.
Minimal, safe tweak:
- Process.Start(new ProcessStartInfo() - { - FileName = uri.AbsoluteUri, - UseShellExecute = true - })?.Dispose(); + var psi = new ProcessStartInfo + { + FileName = uri.IsFile ? uri.LocalPath : uri.AbsoluteUri, + UseShellExecute = true + }; + Process.Start(psi)?.Dispose();Optional (requires new locale keys, e.g., fileOpenError and fileNoAssociationError):
- catch (Exception e) - { - LogException(ClassName, $"Failed to open: {uri.AbsoluteUri}", e); - ShowMsgError(GetTranslation("errorTitle"), e.Message); - } + catch (Win32Exception ex) when (ex.NativeErrorCode == 1155) // no association + { + LogWarn(ClassName, $"No association to open: {uri}"); + ShowMsgError(GetTranslation("errorTitle"), GetTranslation("fileNoAssociationError")); + } + catch (Exception e) + { + LogException(ClassName, $"Failed to open: {uri}", e); + ShowMsgError(GetTranslation("errorTitle"), + string.Format(GetTranslation("fileOpenError"), e.Message)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/PublicAPIInstance.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-20T07:28:28.092Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3850
File: Flow.Launcher.Core/Resource/Internationalization.cs:0-0
Timestamp: 2025-07-20T07:28:28.092Z
Learning: In Flow Launcher's Internationalization class, when the Flow Launcher language directory or default language file is missing, the only viable approach is to log an error and return early - there are no fallback mechanisms or alternative recovery strategies available due to architectural constraints.
Applied to files:
Flow.Launcher/Languages/en.xaml
📚 Learning: 2025-09-05T11:56:27.267Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3593
File: Flow.Launcher/HotkeyControlDialog.xaml:6-6
Timestamp: 2025-09-05T11:56:27.267Z
Learning: In Flow.Launcher's migration to iNKORE.UI.WPF.Modern UI framework, dialog resource keys like PopuBGColor, PopupButtonAreaBGColor, PopupButtonAreaBorderColor, and PopupTextColor are provided by the iNKORE.UI.WPF.Modern framework itself, not defined locally in the codebase theme files.
Applied to files:
Flow.Launcher/Languages/en.xaml
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher/PublicAPIInstance.cs
🧬 Code graph analysis (1)
Flow.Launcher/PublicAPIInstance.cs (4)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (3)
ShowMsgError(85-85)GetTranslation(171-171)LogException(311-311)Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (2)
ShowMsgError(62-65)GetTranslation(97-100)Flow.Launcher.Core/Resource/Internationalization.cs (1)
GetTranslation(348-360)Flow.Launcher.Infrastructure/Logger/Log.cs (1)
Exception(93-103)
⏰ 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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher/PublicAPIInstance.cs (1)
1-1: No review needed for BOM/using change.Flow.Launcher/Languages/en.xaml (1)
593-593: Resolved — fileNotFoundError present in all locale filesAll Flow.Launcher/Languages/*.xaml contain x:Key="fileNotFoundError"; no action required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs (1)
178-178: Consider refactoring to use the new helper method.Line 178 currently uses
LocationExists(fileOrFolderPath) || FileExists(fileOrFolderPath), which is exactly what the newFileOrLocationExistsmethod does. This would be a good opportunity to use the new helper method for consistency.Apply this diff to use the new helper method:
- if (LocationExists(fileOrFolderPath) || FileExists(fileOrFolderPath)) + if (fileOrFolderPath.FileOrLocationExists())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/PublicAPIInstance.cs
🧰 Additional context used
🪛 GitHub Actions: Check Spelling
Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs
[warning] Spell check warning: 'temppath' is not a recognized word.
[warning] Spell check warning: 'temppath' is not a recognized word.
[warning] Spell check warning: 'temppath' is not a recognized word.
⏰ 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). (8)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher.Plugin/SharedCommands/FilesFolders.cs (1)
153-161: LGTM! Clean helper method implementation.The new
FileOrLocationExistsmethod provides a convenient way to check for both file and directory existence in a single call. The implementation correctly delegates to the existingLocationExistsandFileExistsmethods, maintaining consistency with the existing codebase patterns.
Crash when opening non-existent local file
Fix crash when opening urls with non-existent local file in public api functions