[jochenkirstaetter]

Prepares nectarlink-core and desktop/app for non-Windows desktop targets while tightening session and reconnect reliability across all platforms.

core/nectarlink-core & core/nectarlink-ffi

• features.rs: Map (DeviceKind::Desktop, "linux") to Role::Desktop alongside "windows" and "macos", with a feature-matrix parity test against Windows.
• identity.rs: Create identity.key with 0o600 permissions on Unix (#[cfg(unix)]).
• deck.rs: Extend is_valid_app_path with a deterministic shape check that accepts .desktop and .appimage launchers as well as absolute (/...) and home-relative (~/..., expanded in execute_action) Unix paths across all OSes, while rejecting .sh scripts, shell binaries (sh, bash, dash, zsh, fish, csh, tcsh, ksh), .. traversal, and non-executable system/temp trees (/etc, /tmp, /var/tmp, /dev, /proc, /sys).
• handoff.rs: Clamp cursor and selection_end to valid UTF-8 character boundaries before slicing.
• session.rs: Apply REQUEST_TIMEOUT across both outbox enqueue and reply wait in Session::request. Add try_send_realtime (with OUTBOX_REALTIME_MAX_FILL = 96) for droppable real-time PC audio frames so control RPCs (FindDeviceRing, etc.) are never starved by audio backpressure.
• node.rs:
  • Add CloseOnDrop guard in dial and dial_with_usb_interrupt so cancelled or raced LAN dials close cleanly with CLOSE_NORMAL and interrupt promptly when a USB tunnel arrives.
  • Filter loopback addresses out of last_addrs and preserve prior LAN addresses when a session only reports USB loopback addresses.
  • Check session.is_alive() in publish_online and skip already-closed connections in register_session.
  • Have on_usb_tunnel_accepted start and wake the peer supervisor instead of dialing in parallel, and skip DEFER_DIAL when a USB tunnel is ready while keeping the 750 ms wait intact for LAN discovery.
  • Increase EVENT_CAPACITY from 512 to 2048 to absorb startup/reconnect event bursts.
• core/nectarlink-ffi/src/lib.rs: Re-emit a full state snapshot (DevicesChanged, PendingPairingsChanged, TransfersChanged, NearbyChanged, TrustedNetworksChanged) when the broadcast event stream lags.

desktop/app

• icons.rs: Normalize Unix absolute paths (/path/to/icon.png -> file:///path/to/icon.png) in file_url while keeping Windows drive and UNC (\\server\share\...) path formatting unchanged.
• qml/MainWindow.qml: Render caption bar minimize/maximize/restore/close icons with crisp vector ShapePath glyphs on non-Windows platforms while keeping the native Windows font glyphs and Snap Layouts hit-testing on Windows.

[thtbee]

Hi @jochenkirstaetter, thank you for splitting this out so quickly. It's much easier to review on its own, and several points from the last round are sorted: the key file is now plain with 0600 and named for what it is, Windows keeps its own title-bar glyphs, .sh launch tiles are refused, and the clipboard garbage is fixed where it starts in PhoneClipboard.kt.

I went through every file and checked each change against how the code behaves today. Here's what needs to change before this can go in.

Must change before merge

1. The PC's folders get shared without the PC's sharing setting (desktop/app/src/pc_storage.rs). ensure_enabled now also allows access when any paired phone has the per-device storage toggle on. On a PC that toggle means "browse that phone here" (the phone's storage in File Explorer), not "let it browse this PC". So someone who only mounts their phone's storage would open their PC's folders to every paired phone, with Your PC in your pocket off. The PC's folders need to stay behind share_pc_storage alone; that split was made on purpose in ffad687.

2. Turning on PC sharing switches on phone storage for every device, at every start (desktop/app/src/settings.rs, set_share_pc_storage, and the new call in core_host.rs). It now sets the per-device storage toggle on all paired devices, and runs on each launch through apply_runtime and core_host::run. That turns on a separate, sensitive permission as a side effect, and turns it back on after someone switched it off for a device. Please remove it; the two settings are independent.

3. The control stream's write timeout can drop healthy sessions (core/nectarlink-core/src/session.rs, writer). Each write now gets REQUEST_TIMEOUT (10 s), and the stream closes if it takes longer. Control messages can be up to 1 MiB: an MMS part in a reply is up to 900 KB, and photo and SMS lists are filled to the frame limit. Over Connect over mobile data at around 1 Mbit/s one of those takes 8 to 16 s, so the session would be torn down mid-reply, over and over. A dead connection is already caught by QUIC's idle timeout and the heartbeat. If the aim is to catch a stalled peer, please base it on progress (nothing written for a while) rather than a fixed time per frame, with a test for a large frame on a slow link.

4. The duplicate-session rule needs to come out the same on both devices (core/nectarlink-core/src/node.rs, register_session). In the last review I listed preferring USB here as good. Looking at it more closely, both devices only agree on which connection is USB once both have registered the tunnel (usb_bridge_ports on the phone, usb_peers on the PC). In the moment where one side has and the other hasn't, each can keep a different connection and close the other's, which is the connection churn fixed in 0.1.4. The new test brings USB up before any LAN session exists, so it doesn't cover that moment. Please add a two-device test that brings the tunnel up while a LAN session is live, or keep the existing rule in register_session and leave the USB preference to maintain, which dial_with_usb_interrupt already handles.

5. A maximized window doesn't come to the front any more (desktop/app/qml/MainWindow.qml, restoreAndBringToFront). For a maximized window it calls showMaximized(), raise() and requestActivate() instead of bringToFront(). On Windows, NativeWindow::bringToFront is what gets past the foreground lock, so a maximized window now stays behind other apps when opened from the tray or after an update. Please call bringToFront() on that path too.

6. Timeline icons go back to matching English text (desktop/app/qml/pages/TimelinePage.qml, iconForRow). It looks for "Copied image", "Copied file " and "Link · " in the title. The titles are translated now, so this quietly falls back in Simplified Chinese; 054bafb moved the timeline off English text for this reason. Please base the icon on the entry's kind and file name, as the file-extension part already does.

7. A new test fails on CI (desktop/app/src/settings.rs, desktop_i18n_catalog_is_complete_preserves_placeholders_and_compiles_to_qm). It reads nectarlink_zh_CN.qm from OUT_DIR, which isn't there in the CI build, so "Windows app (x64)" fails. The catalog is already checked in CI by cargo xtask strings, and the Android strings by Android lint, so I'd remove this test and android_i18n_catalog_matches_all_translatable_keys_and_placeholders (the desktop crate shouldn't read Android resources) rather than fix them.

8. The description doesn't match the code. It mentions replacing an .expect in upgrade_to, a oneshot in spawn_session and a 15 s timeout on Session::rpc. None of those exist on main or in this branch. Please describe what the PR actually changes; it's the record of why each change was made.

Worth changing

• Scope. A few parts aren't Linux preparation and change Windows or Android: remembering the window's position and size, the Android clipboard fix, thumbnails for any image file in the Timeline, and ThemeAndLocalizationTest. Some are good (the clipboard one fixes a real bug), but in their own PRs, each with its own description and tests, they're easier to review and to trace if something regresses. The window geometry also still needs the two-monitor check from last time.
• Older Qt in shared QML. if ("topLeftRadius" in this), the Qt.ColorScheme guards, variableAxes set from handlers, preferredRendererType = 2 and the Theme.errorContainer || ... fallbacks all make the shared QML run on Qt older than the 6.12 we build with. That's a decision for the Linux port as a whole: ship the pinned Qt (an AppImage or Flatpak), or support distribution Qt. Please decide and document it first. If it's distribution Qt, I'd rather keep the version checks in one place than spread through the components.
• file_url (desktop/app/src/icons.rs) now trims every leading /, which turns a Windows network path (\\server\share\a.png) into file:///server/share/a.png. Please keep the Windows behavior and only normalize Unix paths.
• CloseOnDrop closes a cancelled dial with CLOSE_DUPLICATE; CLOSE_NORMAL, or a code of its own, would describe it better.
• EVENT_CAPACITY from 512 to 2048: fine together with the resync, but a comment on why would help.
• is_valid_app_path accepts any binary directly in /usr/bin and friends, shells included. Fine as a guard since tiles are set up on the PC, but sh, bash and the like could be refused for the same reason as .sh.

Good as they are

• session.rs request: the timeout now covers a full outbox too, so a call can't hang.
• node.rs: cancelled dials closed, loopback addresses kept out of the stored ones, publish_online checking the session is alive, connected() and USB tunnels waking the supervisor instead of dialing in parallel.
• FFI: sending state again after the event channel lags.
• try_send_realtime for PC audio frames.
• handoff.rs: the multi-byte text fixes, with a test.
• features.rs: Linux as a desktop OS, with a test that the matrix matches Windows.
• identity.rs: 0600 on the key file, with a test.

How to move this forward

Once the eight points above are sorted, I'll do a quick second pass. Happy to help with 3 and 4 in particular, since they touch code that's had a lot of tuning. #12 can then go on top as planned.

[jochenkirstaetter]

Hi @thtbee

Again, thanks for the great, detailed feedback. That's highly appreciated.
Yeah, I see that the code is still too invasive and needs better descriptions and the reasoning. I'm working on that now to get this branch ready as quickly as possible. I'm also going to revert a few bits and pieces given your arguments.

Apologies for any inconvenience caused.

Also, I'm going to isolate those non-Linux preparations into their own PR stacked on top of this branch, or probably will rebase them to the future main branch anyway and then place them back into the code. Meanwhile, I'm going to create individual issues based on those.

Cheers

[jochenkirstaetter]

Hi @thtbee

Branch has been rebased onto latest main and is ready for review.
Meanwhile I stacked the other features, fixes, and changes into their own branches.

Hope this helps.

Cheers

[thtbee]

Hi @jochenkirstaetter, thanks for turning this around so quickly, and no apology needed. Most of what I raised came from history that isn't written down anywhere, so it was hard to know from the outside.

This version is much easier to follow. The PC storage changes, the write timeout, the USB preference in register_session, the window and Timeline changes and the i18n tests are all out, file_url keeps network paths working, and cancelled dials close with CLOSE_NORMAL. Splitting the window, clipboard and thumbnail work into #26, #27 and #28 is a good call too.

I went through the new diff file by file. A few things are left.

Must change before merge

1. connected() waking the supervisor gets around the reconnect backoff (core/nectarlink-core/src/node.rs). Node::connected is called from about 50 places, many on events: each phone notification forwarded to a PC, each clipboard copy, battery and media updates. When a paired device is off for a few hours, every one of those calls now wakes maintain out of its backoff sleep and starts a dial, so the backoff that grows to BACKOFF_MAX stops applying while anything is happening. On a phone with a PC that's switched off, that's a dial on almost every notification, which costs battery. Discovery and the USB tunnel already wake the supervisor when the device actually shows up. If there's a case where neither does, could you tell me which one? I'd rather wake from that event than from every call.

2. Waking during DEFER_DIAL brings back the dial race it exists to avoid (maintain). When two devices come up together, the higher ID waits 750 ms so the lower one's dial usually wins. LAN discovery wakes the supervisor at exactly that moment, so with wake.notified() in that select! both now dial at once, and one connection gets dropped. Skipping the wait when a USB tunnel is ready makes sense; please leave the wake out of the wait itself.

3. The description should match the code, since it becomes the record of the change. Some items in it don't appear in the diff:

   • endpoint.network_change() on a USB tunnel to a paired peer is already on main.
   • Serializing RemoteStorageEntry::modified as a u64 isn't in this branch.

   And some changes aren't mentioned:

   • the FFI resync after a lagged event stream;
   • register_session skipping connections that are already closed;
   • publish_online checking is_alive();
   • on_usb_tunnel_accepted starting and waking the supervisor instead of dialing itself;
   • the UTF-8 boundary fixes in handoff.rs.

Worth changing

• Code that only builds with #12. settings.rs calls crate::pc_notifications::start_listener() on Linux, which doesn't exist in this branch, and try_data_dir gets #[allow(dead_code)] although Windows uses it. Both are only needed by the Linux backend, so I'd move them to #12. If one has to stay here, #[cfg_attr(not(windows), allow(dead_code))] keeps the warning on Windows.
• try_send_realtime returns Ok(()) when it drops a frame. That's right for audio, but the doc comment should say so, so nobody uses it later for something that must arrive.
• The new test checks that a ring works during a burst of 128 frames. With the writer draining as it goes, 128 frames stay at or under the drop threshold, so the test passes with or without try_send_realtime. A burst larger than OUTBOX_CAPACITY would show that frames get dropped and the ring still gets through.

Good as they are

• CloseOnDrop in dial, and dial_with_usb_interrupt leaving a slow LAN dial when a tunnel comes up.
• Keeping loopback addresses out of last_addrs, and keeping the earlier LAN addresses when a session has only USB ones.
• Timing out Session::request across both enqueue and reply.
• The Linux role, .desktop and AppImage tiles with shells refused, the 0600 key file, and file_url.
• Vector caption glyphs off Windows, with Snap Layouts input kept on Windows.

Once the first two are sorted, this looks ready to me.

[jochenkirstaetter]

Hi @thtbee

I'm learning, I'm learning...

Reviewing your feedback and getting those changes done ASAP. This is really a solid base for all the other stacked, pending branches.I really like the dynamics in this project and how "parallel" development across multiple OS is coming together nicely.

Be right back with mitigated changes.

Cheers

[thtbee]

Thanks, Jochen, this addresses everything: connected() no longer wakes the supervisor, DEFER_DIAL keeps its wait, the Linux-only bits moved to #12, and the test now shows frames being dropped while the ring gets through. No need to apologize for the rounds either; this is how it should go, and the result is a solid base for the other platforms.

One small thing in the description: it says launch tiles also accept binaries under /usr/games/ and /opt/ and refuse .bash, .zsh, .fish and .py files, but is_valid_app_path doesn't do either (only .AppImage files can be in /opt). Please update that line so the record matches the code.

[jochenkirstaetter]

Hi @thtbee

Minor change has been applied to is_valid_app_path - I hope I got you right.
It's now checking whether a Shell binary has been passed in, and rejects launching it.

I'm wondering whether it might not be better to evaluate $PATH environment var instead of providing a list of discrete folder paths. Checking things and reverting back.

Cheers

[jochenkirstaetter]

Hi @thtbee

OK, that looks way better now.
In hindsight, I'm going to review the follow-ups on Linux for $PATH handling as well, and see that tests are better gated across the code base.

Crossing fingers this is ready now.

Cheers

[thtbee]

Thanks for trying the PATH idea. I think it causes a problem though, so I've moved this back to changes requested.

Saved tiles are checked again every time the Deck loads: DeckConfig::load runs sanitized(), which drops any tile whose action fails is_valid(), and the next save writes the file without it. With the check now depending on PATH, whether a tile survives depends on how NectarLink was started. Autostart, a systemd user unit or a Flatpak usually get a shorter PATH than a terminal, often without ~/.local/bin. So a tile for ~/.local/bin/obsidian could disappear for good after a reboot, with no message.

Tiles are only ever set up on the PC itself, and the phone only gets the wire layout without paths, so this check is a sanity guard rather than a security boundary. I'd keep it to the shape of the path and leave the rest to launch time:

• absolute, with no .. segments and no control characters;
• not a shell (sh, bash and the others) or a .sh script;
• .desktop and .AppImage as before.

If the app is gone when the tile is pressed, launch_app fails and the PC can say so, which is easier to understand than a tile that vanished.

One smaller thing: the Windows asserts in the test now only run on Windows, but .exe and .lnk don't depend on the OS. With the check above, all of them can stay unconditional again.

[jochenkirstaetter]

Hi @thtbee

Checking the CI issue(s). No worries, gonna sort it out.

Cheers

[jochenkirstaetter]

Hi @thtbee

I'm not too sure about your findings and conclusion about the PATH handling. Autostart, systemd and Flatpak are in the context of the logged in user. As for systemd it would depend whether it's installed as a global service or --user entry. This would also allow to run multiple instances of NectarLink using different users, and still getting the right PATH information. Similar to RunAs under Windows.

As for the asserts, I introduced them specifically because those Unix paths, e.g. /usr/bin/firefox, and others, do not exist under Windows and would cause havoc, or?

No issues to revert things, but I'm under the impression that it might go the wrong way.

What's your POV?

Cheers

[thtbee]

Fair questions, and I'm not sure I explained the problem well. It's less about PATH being wrong and more about what happens when the check fails.

Saved tiles are checked again every time the Deck loads: DeckConfig::load runs sanitized(), and a tile that fails is_valid() is dropped, then gone for good at the next save. So the check has to give the same answer for the same tile no matter how or where NectarLink was started. With PATH it doesn't:

• Flatpak: the app sees the sandbox's PATH, /app/bin:/usr/bin, and that /usr/bin is the runtime's, not the host's. A tile for the host's /usr/local/bin/foo or ~/.local/bin/foo fails, even though flatpak-spawn --host could launch it.
• systemd --user: units get systemd's default PATH (/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin) unless environment.d adds to it, so no ~/.local/bin.
• Graphical sessions: many people add ~/.local/bin or a custom bin folder in .bashrc or .zshrc, which a session started by GDM or SDDM never reads. It's the reason editors like VS Code resolve the shell environment themselves.
• Different OS: feat(linux): Linux desktop & CLI implementation (stacked on #23) #12's CI shows it from the other side. On Windows, build_action("launch_app", "/usr/bin/firefox", ...) in deck.rs now fails ("Application path must end with .exe or .lnk.") because /usr/bin isn't on Windows' PATH, while the same test passes on Linux.

On the asserts: is_valid_app_path never checks that a file exists, only that the path looks like something we're willing to launch. /usr/bin/firefox is a fine-looking path on any OS; on Windows it would simply fail to launch, and Windows never produces it anyway. Once the check is about shape only, the same asserts hold everywhere and don't need cfg. Needing to gate them is a hint that the result depends on the machine.

So my suggestion is still to check the shape (absolute, no .., no control characters, not a shell or a .sh script, plus .desktop and .AppImage), and leave existence to launch time, where launch_app fails and the PC can say so. Multiple users each running their own NectarLink keeps working that way too, since each launch uses that user's environment.

Happy to be convinced otherwise if you've seen a case where shape alone lets through something it shouldn't.

[jochenkirstaetter]

Hi @thtbee

Got it, and already have the "comprise solution" coming up. I was also digging deeper into the PATH situation, and I was partially wrong. Which aligns with your explanation very well.

The latest push shall cover both ends nicely now. I also added the ~ (tilde) handling which represents the HOME folder under Linux and macOS.

Let me know if that's suitable now.

Cheers

[thtbee]

Thanks for digging into it, and for being open about it. This version does exactly what I hoped: the check is about the shape of the path, so a saved tile gives the same answer however and wherever NectarLink starts, and all the asserts run on every OS again. Supporting ~/ and expanding it in execute_action is a nice touch, and keeping /etc, /tmp, /dev, /proc and /sys out makes sense as a sanity guard.

It should also fix #12's Windows failure in builds_and_validates_editor_actions, since /usr/bin/firefox no longer depends on Windows' PATH.

One small thing: the deck.rs line in the description still describes the PATH version. Could you update it to the shape check, since the description ends up as the record of the change?