fix(linux): stop the file picker failing silently, in all 68 places - #515
Merged
Conversation
…t a false claim First half of #510. The chooser failure was invisible to every layer above it, so this recovers the one piece of information that is destroyed on the way up. ## What was measured, not assumed The #464 gate produces a host with no xdg-desktop-portal deterministically, and its artefacts already carried the answer: Gtk-WARNING: Can't open portal file chooser: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown GTK notices, reports it, and presents nothing. It does NOT fall back, whatever the comment in lib.rs claimed. Then the information dies one layer down. `rfd 0.16`, which tauri-plugin-dialog uses, exposes `pick_file() -> Option<PathBuf>`: no Result, no error channel at all. So the plugin answers with a SUCCESSFUL null, and the frontend cannot tell "the chooser could not open" from "the user pressed Cancel". That kills the obvious fix. A try/catch around the call sites catches nothing here, because nothing is thrown, and the `if (!filePath) return;` in ExportImportDialog.tsx is not missing error handling: it is code that cannot distinguish the two cases. ## What this adds `portal_chooser::chooser_unavailable_reason()` answers whether a chooser can be presented, before the dialog is ever invoked, using GDBus through the gtk crate that is already a dependency. No new crate reaches the shipped binary. It deliberately does not look for another chooser. On such a host there isn't one: the in-process fallback is the heap corruption GTK_USE_PORTAL=1 exists to avoid. The honest behaviour is to say why there is none. Three things it refuses to get wrong, each pinned by a test that builds a throwaway session bus rather than reasoning about one: - a bus with no portal and nothing activatable reads as MISSING - the case the harness measured; - a bus where the name is owned reads as AVAILABLE, so the check cannot pass by always crying wolf; - an installed-but-not-yet-running portal reads as AVAILABLE. This is the subtle one: a portal is D-Bus activatable, so on a healthy machine that has not opened a chooser yet NOBODY owns the name. Checking only NameHasOwner would warn every user until their first file dialog, and a warning that cries wolf is a warning nobody reads. Two more refusals, both deliberate: an explicit GTK_USE_PORTAL=0 reports no problem, because that user asked for the in-process chooser on purpose; and a bus that cannot be questioned at all reports no problem rather than accusing the host, since a false alarm on a healthy machine is worse than the silence being fixed. The bus timeout is 1s because this sits in front of a user gesture. ## Also The lib.rs comment no longer claims a fallback that does not exist, and records what happens instead. Not done here, and tracked in #510: the frontend still has to consume this, and the ~37 picker call sites with no error handling still want a shared helper for the refusing-portal path, where an error genuinely is raised. cargo fmt, clippy and the four new tests green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second half of #510. The first half recovered the lost information in Rust; this delivers it to the person looking at the button. ## Why a try/catch at the call sites was never the fix Measured by the #464 gate, not assumed. `rfd` exposes `pick_file() -> Option<PathBuf>` with no error channel, so on a host with no xdg-desktop-portal tauri-plugin-dialog answers with a SUCCESSFUL `null` — the same value as "the user pressed Cancel". `open()` is never rejected, so there is nothing to catch, and `if (!filePath) return;` at a call site is not forgetful code: it genuinely cannot tell the two cases apart. So the distinction has to be recreated BEFORE the dialog is invoked, which is what `chooser_unavailable` (fff4670) exists for. ## The count is now exact 68 picker call sites across 34 files: 39 `open`, 29 `save`. The "~37 of 67" figure in the issue came from a heuristic on surrounding try/catch, and that heuristic measures the wrong property here — with nothing thrown, error handling is irrelevant and **all 68 were silent**, not 37. Exactly 34 sat inside a `try` block and 34 did not, which turns out to say nothing about the bug. Two of the 34 files imported the plugin without ever calling it. Those imports are removed rather than pointed at the helper. ## What this adds - `src/utils/pickPath.ts` — `pickFile`/`pickSave`, signature-compatible with the plugin's `open`/`save`. They ask whether a chooser can be presented; when it cannot, they surface a translated message and resolve to `null` without opening anything. A refusing portal, where an error genuinely exists, is caught in the same place instead of at 34 separate call sites. - All 68 call sites migrated, renamed rather than aliased as `pickFile as open`: the defect was that a reader could not tell what a picker call would do, and an `open(...)` that is secretly something else preserves that. - `picker.unavailable.*` in all 47 locales, really translated, not placeheld. `i18n:validate` stays at 0 errors / 0 warnings, 5137 -> 5140 keys. - A hookless `translate()` in the i18n module, because a helper called from a click handler cannot reach the provider's `t`. It shares one lookup function with the provider, so the in-React and out-of-React paths cannot drift. - The message goes out as an `important` toast: that flag is what makes App.tsx bypass `showToastNotifications`, and a user who turned ambient toasts off must still be told why their click did nothing — that IS the bug. ## Pinned at three levels, each verified by breaking the code - `pickPath.test.ts`, 11 tests on the message actually reaching the user. Out of 11: deleting the pre-check fails 4, dropping `important: true` fails 1, pointing a reason at a non-existent translation key fails 3, making the report a no-op fails 7. It also pins a non-English locale, because a portal-less host is most likely a minimal or containerised desktop and those 46 translations are not decoration. - `pickPathIsTheOnlyPicker.test.ts` — nothing but the helper may import the plugin, so the fix cannot decay by addition: the next new call site would go red instead of quietly restoring the silence. It reads sources through `import.meta.glob(..., '?raw')` rather than `node:fs`, because `src/` is typed as a browser (`lib: ES2020, DOM`, no `@types/node`) and `tsc` sits on the production build chain. It asserts on real import statements, so a prose mention does not fail the build, and it checks the helper still wraps the plugin so the rule cannot pass by becoming vacuous. - `portal-chooser-test.sh` case 3 asserted only that no extra window appeared — which the silent build also satisfied. It now also asserts the app SAID why. Case 1 asserts the check runs and stays quiet on a host that has a portal, so the guard cannot pass by crying wolf; a warning that fires everywhere is a warning nobody reads. The window-count assertion stays: it is still true, and the day a window does appear there, somebody has reintroduced the in-process chooser that corrupts the GLib heap. `chooser_unavailable` now logs its verdict, and that is load-bearing rather than decorative: the remedy is a toast inside the WebView, so no window count, D-Bus trace or screenshot can observe it from outside the process. The log line is the only evidence that the frontend consulted the check on the real click path. Refs #510
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds Linux portal availability detection and shared ChangesPortal-aware picker flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Frontend
participant pickPath
participant Tauri
participant SessionBus
participant DialogPlugin
Frontend->>pickPath: pickFile or pickSave
pickPath->>Tauri: invoke chooser_unavailable
Tauri->>SessionBus: check portal ownership/activation
SessionBus-->>Tauri: availability result
Tauri-->>pickPath: optional reason
alt chooser unavailable
pickPath-->>Frontend: translated error toast and null
else chooser available
pickPath->>DialogPlugin: open or save
DialogPlugin-->>pickPath: selected path, cancel, or refusal
pickPath-->>Frontend: path or null
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…Storage Found while wiring the picker message, and it would have shipped the bug the fix is about, just in a different language. `translate()` originally resolved the language from `localStorage`, on the reasoning that `setLanguage` persists before it re-renders so storage and the mounted provider cannot disagree. That is true of the main window and false of the other one: `extract-main.tsx` mounts `I18nProvider` with `initialLanguage` taken from the desktop language Rust injects, deliberately ignoring whatever language the main app was last left in — and it never persists that choice. So in the dedicated extract window every `translate()` call would have used the main app's last language while everything around it rendered in the desktop language. That window's folder picker is one of the 68 migrated call sites, so the very message this branch adds would have come out in the wrong language, which is exactly what that entry point goes out of its way to prevent. It now reads `<html lang>`, which the provider already maintains for accessibility, so there is one published source of truth rather than two derivations that can disagree. `localStorage` and then English remain as fallbacks for code that runs before any provider has mounted. Pinned by `follows the window it is rendered in, not the last language the main app was left in`: storage says Italian, `<html lang>` says German, the message must be German. Restoring the storage-only lookup fails it with Italian. Refs #510
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Tauri runs a synchronous command on the main thread, and on Linux that is the GTK thread. `chooser_unavailable` was sync while doing `bus_get_sync` plus up to two `call_sync` round trips at BUS_TIMEOUT_MS each, so a wedged session bus froze the whole window for about two seconds in front of the very click this module exists to explain, with an unbounded tail if the bus socket accepts a connection and never finishes authenticating. That is the promise the BUS_TIMEOUT_MS comment already made and a sync command cannot keep. Shipping a comment that claims "a wedged session bus must not turn a click into a freeze" next to code that freezes the click is the same defect #510 exists to remove, one file further along: lib.rs:17677 claimed a GTK fallback that measurement showed never happens. The command is now async, which takes it off the main thread, with the blocking round trip on `spawn_blocking` so it does not sit on an async worker either. A failure to join reports "no problem", the same rule the module already applies to a bus that cannot be questioned: a false "your chooser is broken" on a healthy host is worse than the silence being fixed. The pin is a test that `.await`s the command, so it acts at compile time: reverting the command to `pub fn` fails the build with E0277 rather than failing an assertion someone can delete. Verified by doing exactly that before keeping the fix. It also asserts the wrapper only logs the verdict and does not invent one. The two tests that read GTK_USE_PORTAL now share a mutex. The harness runs tests on several threads, so "restored immediately after" was not enough on its own, and the new pin queries the same variable, which would have made the old race reachable from a second place. The class is wider than this command: 37 commands in this crate are still sync, and two of them do filesystem work on the main thread. Audited and opened as #517 rather than left in a paragraph. Refs #510, #517 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/SettingsPanel.tsx (1)
3355-3368: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale
open()comment.Line 3366 still documents the removed dialog API even though this flow now calls
pickFile; replace or remove the comment so future maintenance reflects the shared helper contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/SettingsPanel.tsx` around lines 3355 - 3368, Remove or update the stale comment immediately before loadKeystoreFromPath in the pickFile flow so it describes the shared pickFile contract rather than the removed dialog open() API; preserve the existing filePath guard and keystore-loading behavior.
🧹 Nitpick comments (2)
src-tauri/src/portal_chooser.rs (2)
130-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the
logfacade overeprintln!for these diagnostics.Everything else in the crate goes through
log::*, which tauri-plugin-log fans out to the log files and the in-app DebugPanel;eprintln!reaches raw stderr only, so a user reporting "the picker does nothing" won't have these lines in their exported diagnostics.log::warn!("{CHOOSER_UNAVAILABLE_LOG} {r}")/log::info!("{CHOOSER_CHECKED_LOG}")keeps the literals intact for thetests/portal-chooser/portal-chooser-test.shgrep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/portal_chooser.rs` around lines 130 - 147, Update chooser_unavailable to use the log facade instead of eprintln!, emitting log::warn! with CHOOSER_UNAVAILABLE_LOG and the reason, and log::info! with CHOOSER_CHECKED_LOG when no reason exists. Preserve both message literals unchanged so the existing test grep continues to match.
76-121: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the whole chooser pre-check to the advertised one-second budget.
bus_get_synccan block while connecting to the session bus, andreachable_onstill runs two separate 1scall_syncoperations. A wedged bus can therefore exceed the “caps it at one second” contract documented insrc/utils/pickPath.ts; use a single deadline/timeout around the full check or reduce the per-call timeout so the total stays within one second.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src-tauri/src/portal_chooser.rs` around lines 76 - 121, The portal reachability check in portal_is_reachable and reachable_on must respect the documented one-second total budget, including session-bus connection and both D-Bus calls. Replace the fixed per-call BUS_TIMEOUT_MS usage with a shared deadline or reduced remaining-time calculation, and ensure bus_get_sync is also bounded so a wedged bus cannot make the chooser pre-check exceed one second.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src-tauri/src/portal_chooser.rs`:
- Around line 186-203: Update the private_bus test helper’s temporary
configuration naming to include a unique per-invocation discriminator, rather
than deriving the filename only from servicedir and process ID. Ensure each
parallel private_bus(None) call creates and uses a distinct bus-*.conf path
while preserving the existing servicedir distinction.
- Around line 156-168: Update an_explicit_opt_out_reports_no_problem to remove
the unnecessary unsafe wrappers around GTK_USE_PORTAL environment-variable
operations and delete the misleading single-threaded SAFETY comment. Preserve
the existing save, restore, and assertion behavior; do not claim test-wide
single-threaded execution.
In `@src/i18n/locales/tr.json`:
- Line 5585: Update the Turkish portalMissing translation value to replace “bu
sistem dosya seçici açamıyor” with the grammatically correct “Bu sistemde dosya
seçici açılamıyor.” while preserving the remaining guidance and punctuation.
---
Outside diff comments:
In `@src/components/SettingsPanel.tsx`:
- Around line 3355-3368: Remove or update the stale comment immediately before
loadKeystoreFromPath in the pickFile flow so it describes the shared pickFile
contract rather than the removed dialog open() API; preserve the existing
filePath guard and keystore-loading behavior.
---
Nitpick comments:
In `@src-tauri/src/portal_chooser.rs`:
- Around line 130-147: Update chooser_unavailable to use the log facade instead
of eprintln!, emitting log::warn! with CHOOSER_UNAVAILABLE_LOG and the reason,
and log::info! with CHOOSER_CHECKED_LOG when no reason exists. Preserve both
message literals unchanged so the existing test grep continues to match.
- Around line 76-121: The portal reachability check in portal_is_reachable and
reachable_on must respect the documented one-second total budget, including
session-bus connection and both D-Bus calls. Replace the fixed per-call
BUS_TIMEOUT_MS usage with a shared deadline or reduced remaining-time
calculation, and ensure bus_get_sync is also bounded so a wedged bus cannot make
the chooser pre-check exceed one second.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5465b480-e47f-40ce-9936-51910ae53ac1
📒 Files selected for processing (89)
src-tauri/src/lib.rssrc-tauri/src/portal_chooser.rssrc/App.tsxsrc/components/AeroCryptKeyslotsModal.tsxsrc/components/AeroCryptRecoveryKitModal.tsxsrc/components/AeroCryptUnlock.tsxsrc/components/AeroShare/AeroShareHandshakeBody.tsxsrc/components/AeroSync/CompareTabContent.tsxsrc/components/AeroSync/RemoteSyncResultDialog.tsxsrc/components/AeroSync/SyncTabContent.tsxsrc/components/ArchiveBrowser.tsxsrc/components/BridgeSourcePanel.tsxsrc/components/CloudPairsEditor.tsxsrc/components/CloudPanel.tsxsrc/components/ConnectionScreen.tsxsrc/components/CryptomatorBrowser.tsxsrc/components/CyberToolsModal.tsxsrc/components/DebugPanel.tsxsrc/components/DevTools/CodeBlockActions.tsxsrc/components/DevTools/useAIChatConversations.tssrc/components/DevTools/useAIChatImages.tssrc/components/ExportImportDialog.tsxsrc/components/ExtractWindow.tsxsrc/components/FileVersionsDialog.tsxsrc/components/GitHubReleaseBrowser.tsxsrc/components/GitLabReleaseBrowser.tsxsrc/components/IconPickerDialog.tsxsrc/components/OAuthConnect.tsxsrc/components/RcloneCryptUnlock.tsxsrc/components/SavedServers.tsxsrc/components/SettingsPanel.tsxsrc/components/Sync/SyncTemplateDialog.tsxsrc/components/VaultSyncDialog.tsxsrc/components/vault/VaultCreate.tsxsrc/components/vault/VaultReceipt.tsxsrc/components/vault/useVaultState.tssrc/i18n/I18nContext.tsxsrc/i18n/index.tssrc/i18n/locales/bg.jsonsrc/i18n/locales/bn.jsonsrc/i18n/locales/ca.jsonsrc/i18n/locales/cs.jsonsrc/i18n/locales/cy.jsonsrc/i18n/locales/da.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/el.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/et.jsonsrc/i18n/locales/eu.jsonsrc/i18n/locales/fi.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/gl.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/hr.jsonsrc/i18n/locales/hu.jsonsrc/i18n/locales/hy.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/is.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/ka.jsonsrc/i18n/locales/km.jsonsrc/i18n/locales/ko.jsonsrc/i18n/locales/lt.jsonsrc/i18n/locales/lv.jsonsrc/i18n/locales/mk.jsonsrc/i18n/locales/ms.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/no.jsonsrc/i18n/locales/pl.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ro.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/sk.jsonsrc/i18n/locales/sl.jsonsrc/i18n/locales/sr.jsonsrc/i18n/locales/sv.jsonsrc/i18n/locales/sw.jsonsrc/i18n/locales/th.jsonsrc/i18n/locales/tl.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/uk.jsonsrc/i18n/locales/vi.jsonsrc/i18n/locales/zh.jsonsrc/utils/pickPath.test.tssrc/utils/pickPath.tssrc/utils/pickPathIsTheOnlyPicker.test.tstests/portal-chooser/portal-chooser-test.sh
💤 Files with no reviewable changes (2)
- src/components/OAuthConnect.tsx
- src/components/SavedServers.tsx
…abbit round Three findings from the review on #515, one of which was hiding something larger. Two tests call private_bus(None), so both resolved to the same bus-bare-<pid>.conf and each truncated it with File::create while the other test's dbus-daemon could still be reading it at startup. The consequence is worse than a flake: when the daemon fails to start, private_bus returns None and the test prints a line and returns, so it reports success while asserting nothing. Fixed with an AtomicU32 discriminator per invocation. That is only one way to lose the daemon, so the class is pinned too: a missing dbus-daemon is now fatal when CI is set, and a skip only on a developer machine. build.yml is the only job that runs cargo test on Linux and it did not install dbus, so these three cases were most likely skipping there all along with no trace, because eprintln! from a passing test is captured and the log reads ok either way. dbus is now in that job's apt list, with a comment saying it is there for the tests and not the build. Verified by breaking it: with a dbus-daemon shim that refuses to start, CI=1 turns the three cases red naming the cause, and the same shim without CI still skips. 5/5 pass on a healthy host. Turkish wording corrected: "bu sistemde dosya seçici açılamıyor" instead of "bu sistem dosya seçici açamıyor", locative plus passive. Applied inside the existing sentence so the half that tells the user which packages to install survives, since that is the only actionable content the message has. Rejected, with the measurement: the review also asked to drop the unsafe around std::env::set_var as unnecessary in edition 2021 and warned it would trip unused_unsafe under -D warnings. It does not. clippy --all-targets -D warnings exits 0 with those blocks on the pinned 1.97.0 toolchain, because set_var is an unsafe fn carrying #[rustc_deprecated_safe_2024], which both allows pre-2024 callers to omit unsafe and suppresses unused_unsafe for callers who write it. The block is allowed now and becomes required at edition 2024. The other half of that finding was right and was already fixed in 14e8bb7. Refs #510 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI's clippy caught what my own gate had declared clean: clippy::await_holding_lock on the pin added in 14e8bb7. Both readings in that test must see the same GTK_USE_PORTAL, so the guard spans them, and a std::sync::MutexGuard held across an .await is denied for a real reason: the future can be parked on one worker and resumed on another with the guard still held. Now a blocking #[test] driving its own runtime, so the whole exchange stays on this thread and the lock is held only while this thread runs. The compile-time pin survives unchanged in strength: block_on accepts only a future, so reverting the command to `pub fn` still fails the build with E0277. Re-verified by doing it. Why my gate missed it, since that matters more than the lint. Each cargo step was written as `( cmd 2>&1 | tail -N ); echo "rc=$?"`, and `$?` there is tail's status, not the command's, so every cargo step printed 0 whatever it did; the npm steps were sound because they used ${PIPESTATUS[0]}. On top of that a warm target/ had not re-linted the test target after the edit, and the denial only appeared after `cargo clean -p aeroftp`. A gate that cannot fail is worse than no gate: it turns "unverified" into "verified" in the report. The gate now redirects each step to a file, reads rc with no pipe in the way, and ends with an explicit ALL-GREEN/RED verdict and a non-zero exit. Measured after this change, with those real exit codes: cargo fmt 0, clippy --all-targets -D warnings 0, cargo test --lib portal_chooser 5/5, tsc 0, i18n:validate 0, vitest 545/545 across 56 files. Break-it checks: reverting the command to sync exits 101 with E0277, CI=1 with a dbus-daemon that refuses to start exits 101 with 3 failures, and the same shim without CI exits 0 with 5 passing. Refs #510 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 tasks
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #510.
On a host with no
xdg-desktop-portal, clicking any "Choose file…" control did nothing and said nothing. This makes the app say why, in 68 places.Why the obvious fix was the wrong one
Measured by the #464 gate, not assumed.
rfdexposespick_file() -> Option<PathBuf>with no error channel, sotauri-plugin-dialoganswers a portal-less host with a successfulnull— the same value as "the user pressed Cancel".open()is never rejected, so atry/catchat the call sites catches nothing, andif (!filePath) return;is not forgetful code: it genuinely cannot tell the two cases apart. The distinction has to be recreated before the dialog is invoked, which is whatchooser_unavailable(first half of this branch,fff467092) exists for.The count is now exact, and it changes the conclusion
68 picker call sites across 34 files: 39
open, 29save. The "~37 of 67" figure in the issue came from a heuristic on surroundingtry/catch, and that heuristic measures the wrong property here. Exactly 34 sites sat inside atryand 34 did not — and it makes no difference, because nothing is thrown. All 68 were silent, not 37. Two of the 34 files imported the plugin without ever calling it; those imports are removed rather than repointed.What this adds
src/utils/pickPath.tspickFile/pickSave, signature-compatible with the plugin'sopen/save. They ask whether a chooser can be presented; when it cannot, they surface a translated message and resolve tonullwithout opening anything. A refusing portal — where an error genuinely exists — is caught here too, instead of at 34 separate call sitespickFile as open. The defect was that a reader could not tell what a picker call would do; anopen(...)that is secretly something else preserves exactly thatpicker.unavailable.*i18n:validatestays at 0 errors / 0 warnings, 5137 → 5140 keystranslate()t. It shares one lookup function with the provider so the in-React and out-of-React paths cannot driftThe message goes out as an
importanttoast. That flag is what makesApp.tsxbypassshowToastNotifications, and a user who turned ambient toasts off must still be told why their click did nothing — that is the bug.Pinned at three levels, each verified by breaking the code
pickPath.test.ts— 12 tests on the message actually reaching the user. Mutation counts below are out of the 11 that existed when they were measured: deleting the pre-check fails 4 · droppingimportant: truefails 1 · pointing a reason at a non-existent translation key fails 3 · making the report a no-op fails 7. It also pins a non-English locale, because a portal-less host is most likely a minimal or containerised desktop, and those 46 translations are not decoration. The 12th is the extract-window pin described at the end.pickPathIsTheOnlyPicker.test.ts— nothing but the helper may import the plugin, so the fix cannot decay by addition: the next new call site goes red instead of quietly restoring the silence in one file while 68 others stay correct. It reads sources throughimport.meta.glob(…, '?raw')rather thannode:fs, becausesrc/is typed as a browser (lib: ES2020, DOM, no@types/node) andtscsits on the production build chain (build: tsc && vite build). It asserts on real import statements, so a prose mention does not fail the build, and it checks the helper still wraps the plugin so the rule cannot pass by becoming vacuous.tests/portal-chooser/portal-chooser-test.sh— case 3 asserted only that no extra window appeared, which the silent build also satisfied. It now also asserts the app said why. Case 1 asserts the check runs and stays quiet on a host that has a portal, so the guard cannot pass by crying wolf: a warning that fires everywhere is a warning nobody reads. The window-count assertion stays — it is still true, and the day a window does appear there, somebody has reintroduced the in-process chooser that corrupts the GLib heap.chooser_unavailablenow logs its verdict, and that is load-bearing rather than decorative: the remedy is a toast inside the WebView, so no window count, D-Bus trace or screenshot can observe it from outside the process. The log line is the only evidence that the frontend consulted the check on the real click path.Gate run locally
tsc(TypeScript 7.0.2, after mergingmain) clean ·vitest544/544 ·i18n:validate0 errors / 0 warnings ·cargo fmt --all --checkclean ·cargo clippy --all-targets -D warningsclean ·cargo test --lib portal_chooser4/4.A second commit: the hookless
translate()had to follow the window, not storageFound while wiring the message up, and it would have shipped this branch's own bug in a different form.
translate()first resolved the language fromlocalStorage, reasoning thatsetLanguagepersists before it re-renders so storage and the mounted provider cannot disagree. True of the main window, false of the other one:extract-main.tsxmountsI18nProviderwithinitialLanguagetaken from the desktop language Rust injects, deliberately ignoring whatever language the main app was last left in — and it never persists that choice.The extract window's folder picker is one of the 68 migrated call sites, so the message this PR adds would have appeared there in the main app's last language while everything around it rendered in the desktop language — exactly what that entry point exists to prevent.
It now reads
<html lang>, which the provider already maintains for accessibility: one published source of truth instead of two derivations that can disagree. Storage and then English remain as fallbacks for code running before any provider mounts. Pinned by "follows the window it is rendered in, not the last language the main app was left in" — storage says Italian,<html lang>says German, the message must be German; restoring the storage-only lookup fails it with Italian.Local gate re-run after this commit:
tscclean ·vitest545/545 ·i18n:validate0/0.Summary by CodeRabbit
New Features
Bug Fixes