Repository navigation
Desktop app: no console windows, dialogs that don't freeze, updater timeouts, screen-reader announcements - #556
Merged
Merged
Conversation
…race trace.rs read tracert/traceroute output with tokio's strict UTF-8 line reader. A non-English Windows writes its tracert messages in the OEM code page, so one byte that is not UTF-8 (for example the u-umlaut in a German timed-out hop) ended the whole trace with 'trace stdout read failed', hops already printed included. The output is now read as bytes and decoded lossily, the same way reverse.rs already decodes ping -a. The hop number and timings are ASCII, so the text that gets a replacement character is only the localized words after them. The new test runs a real child process that prints a non-UTF-8 byte; an English runner cannot hit this any other way.
…ndow on Windows The installed desktop app is a GUI-subsystem process with no console, so every console tool it starts gets a new one. Measured on Windows 11 with a small windows_subsystem=windows program that called the core's reverse_lookup_best_effort_timeout and trace_route and polled the top-level windows. Each call opened new visible windows (a Windows Terminal window titled C:\WINDOWS\System32\ping.exe, and one titled ...\tracert.exe), 4 per run. With CREATE_NO_WINDOW on the same two calls, 0 new windows appeared and the trace still returned its 9 lines. Discover and Sweep call the ping -a lookup once for every host that answered, up to 32 at a time. The Settings probe that runs 'netscli --version' for each candidate on PATH gets the same flag. arp already set it. The constant is repeated in each file because the shared place for it (common/system_tools.rs) is outside this change.
open_result_bundle, choose_file_save_default_directory, export_text_file and save_result_bundle were plain fns, which Tauri runs on the main thread, and they called the dialog plugin's blocking_* functions. The plugin says those must not be used on the main thread. Reading rfd and tauri-runtime-wry, the main thread waits on a channel for the dialog while the dialog needs the main thread, so on Windows the window cannot repaint while a dialog is open and on macOS the dialog should deadlock. That macOS part is from the code and has not been run on a Mac. The four commands are now async and use the plugin's callback calls through one small helper, dialog::ask, which waits on a channel without holding a thread. The pcap-only dialogs (open_pcap_file and the capture save path) already run off the main thread and are unchanged. The tests pin that the wait leaves the thread free (a current-thread runtime must run another task meanwhile) and that an unanswered dialog reads as cancelled.
…he dialog The updater plugin applies no timeout unless one is passed, and UpdateDialog cannot be closed while an install runs (Escape, the backdrop, Close, Later and Skip are all disabled). A download that stalled left a modal the user could only leave by quitting the app. checkForUpdate now passes 30 seconds and installUpdate 10 minutes. The plugin enforces them as a limit on the whole request, not on silence between chunks, so the download one is generous: the largest installer, the Linux AppImage, is 84 MB in 0.3.4. When the limit passes the plugin rejects, installUpdate passes the rejection on, and the dialog lands in its existing failed state with the release page link. Escape stays locked during a download on purpose. Closing the dialog would not stop the download, and on Windows a download that finished later would start the installer and exit the app.
A failed run put its message in a plain div, the progress bar had no live text, and a finished run on the visible tab was deliberately not announced. The only live regions were the toasts, which are opt-in and off by default, so a screen reader user got no news of a run at stock settings. The error strip is now a role=alert. A new visually hidden polite status region (RunAnnouncer) in the workspace says that a run started, a quarter-way message at 25, 50 and 75 percent of a phase, and that it completed with its summary or was stopped. A failure is left to the alert so it is not spoken twice. It follows the visible tab only, says nothing when the person switches tabs, and gives a trace no percentages because its progress is the hop reached out of the maximum. Not tested with a screen reader, only through the live region's text.
Escape cancelled the active run before checking whether anything was open. The About dialog, the update dialog and the context menus close from document keydown listeners that are added after the global one, so they run after it and the press had already stopped the run. Dismissing a menu mid-scan lost the scan. The cancel is now skipped while a dialog or menu is in the document. Found by reading the listener order, and the new test is the first one for this hook.
On Windows the updater plugin exits the app right after it starts msiexec. If UAC is declined or the installer fails, no app is running and nothing says so. The update dialog now says that NetsCLI closes to finish, that it should reopen by itself, and to open it again if it does not. The real fix for the declined-UAC case is a per-user install, which is tracked as #512.
updates.rs works out a reason for the installs that must not replace themselves (Scoop, the AUR package, a .deb) and the frontend mirrored the field, but nothing ever displayed it. Those installs got the same 'Update available' notice as everyone, which opens the GitHub release page and so invites a manual MSI download over a copy that Scoop manages. The reason now goes into the update toast, for example 'Update available: v0.4.0. Installed with Scoop, so update it there: scoop update netscli-gui'. The notice is persistent, so its message now wraps instead of being cut off at one line.
… would cause it When a render throws, the boundary's fallback drew outside .container, which is the only place the colour tokens are defined. Its text took the OS theme's colour on a page that is dark either way, so on a light theme it was black on near-black. The window has no native title bar, so with the app's frame gone there was nothing to minimise, move or close it with. The fallback (now CrashScreen) sits inside a dark .container with the app's own frame and window controls above it. The usual trigger is a shared result bundle that passes the shape checks and then breaks a row builder (an mDNS service without addresses threw on addresses.join inside a useMemo). parseResultBundle now runs buildRows and resultSummary on the result and refuses the file with a message, which shows in the error strip, instead of ending the session. The ErrorBoundary had no test; the new ones cover the screen's message, its window controls and that it is drawn inside .container (jsdom has no stylesheet, so the structure is what can be checked).
…ell fragment The command shown in the strip, copied with the button or Ctrl+Shift+C, and recorded in History is built from the tab's form values. Opening a result bundle merges its form strings into the tab without checking them, so a host of '1.1.1.1; curl ... | sh' reached the clipboard as a working command. The audit named host, subnet, ports and interface, but the numeric fields (count, max hops, timeouts) and the capture filter went through the same unquoted interpolation. buildCommand now passes every value through one helper. Plain values (hosts, ports, subnets, numbers) come out exactly as before. A value with spaces or shell syntax becomes one double-quoted argument, which keeps it a single argument in bash, PowerShell and cmd. A value containing a character no quoting makes safe for all three ($, backtick, a double quote, %, !, a control character, a trailing backslash) is left out like an empty one. That changes one earlier behaviour on purpose. A capture filter containing a double quote used to be escaped with a backslash, which is wrong in PowerShell, and is now left out of the preview.
…s empty Clearing the Packets field sent no limit to open_pcap_file. The core then summarised every packet up to its ten million ceiling and returned them all as one JSON value, while the progress text said it was parsing up to 1000. The import now reads 1000 when the field is empty and never more than the field's own maximum of 100000. This only matters in builds made with --features pcap, which the published installers are not. The audit also suggested an operation id so Stop works on an import. That is not done. run_json_operation can only abort the async task, and parsing a file is blocking work that an abort does not stop, so Stop would have looked like it worked while the parse carried on.
…r passes lint
The control-character range in the command quoting helper tripped no-control-regex. \p{Cc} matches the same characters, and C1 controls as well.
…r Stop DNS Lookup with record ALL makes ten backend calls one after another under its run's operation id. Stop aborts the call that is registered, and between two calls there is none, so a Stop that landed there cancelled nothing and the loop went on to ask for every record type that was left. A run replaced by a newer one did the same. executeTool now takes an isCurrent callback, which runWorkspaceTab answers from its own bookkeeping, and the loop stops when it turns false. The audit suggested minting an id per call. That would leave Stop aimed at an id nobody registered, so the id stays shared. The two Rust comments that said op ids are unique per run now name this exception and what a late removal could do to it, which is lose one Stop for one lookup.
The signature fits on one line, which cargo fmt --check wants.
…ally A gui-save-settings.json that did not parse made every export and capture fail until the file was deleted by hand. Each of them reads the settings first, and so did every Settings control, so the controls could not repair it either. A file that does not parse now reads as the defaults, which lets the next change write a good one over it. The cost is that a save folder the user had chosen is forgotten, which beats nothing being saveable. The write goes to a temporary file beside the settings and is renamed over them, so an interruption leaves the old settings rather than a truncated file.
The GUI wrote CSV as UTF-8 with no byte-order mark. Excel takes the encoding from that mark and, on Windows, reads a file without one in the legacy code page, so a device named with a typographic apostrophe came out as mojibake (the three UTF-8 bytes of the apostrophe read as three legacy characters). Only the two file exports get the mark. The command line's CSV is for pipes and keeps none, and JSON exports are untouched. This is the audit's recommendation and it is a trade-off: a script that reads these files as plain UTF-8 will see the mark at the start of the first header, and needs utf-8-sig or the equivalent. Not checked in Excel here.
…e search Tab close buttons sat in the tab order. A button is a tab stop by default, so each tab added a stop to a strip that is meant to have one. The tab's own children are presentational in ARIA, so a screen reader had nothing to offer there either. They are now tabindex -1, and Delete still closes the focused tab. The close buttons are still nested inside the tabs. Moving them out would change the markup the drag-to-reorder code and the styles rely on, and was not done. Workspace search keeps focus in its input while the arrow keys move through the results, so nothing told a screen reader which result was current. The input is now a combobox that controls the result list and names the current option with aria-activedescendant. Option ids are by position because an item's own id can hold spaces.
…he toast live region Every toast, errors included, left after 1.8 seconds, which is too short to read the one message that says something failed. An error toast now stays until it is clicked. The toast region is polite and atomic, so it reads out everything inside it whenever anything inside it changes. The update dialog sat in it, and its status line changes with every step of a download, so the whole dialog was read out again each time. The dialog now sits beside the region, and keeps its own status line for the progress text.
The progress bar already honours prefers-reduced-motion and the tab spinner did not. With the setting on, the ring is now still, which still marks the tab as busy. CSS only, so there is no test.
sanitize_export_filename, export_path_in_directory, has_supported_artifact_extension and validate_saved_artifact_path decide what the webview can write to disk and ask the OS to open, and none of them had a test. They are correct as read, which is the point of pinning them. The tests cover path separators, traversal and drive prefixes in a filename, names with nothing usable in them, the save folder being created and refused when it is a file, the extension allowlist, and the registry path. The save-folder check in validate_saved_artifact_path is left out on purpose, because it reads the user's real settings and creates the default folder.
main.json explained the window permissions by the buttons in TitleBar.tsx, which was replaced by AppFrame.tsx. Description text only.
…image sources The webview's CSP now has base-uri, form-action and object-src set to 'none', and img-src is 'self' only. Nothing in the app uses any of what was removed. The index.html has no base, form, object or embed, the source has none either, the built CSS has no url(), and the built JavaScript has no data: or blob: URI. The icons are inline SVG. The frontend was built and searched for that. The packaged app was not run with the new policy, so a CSP error would show only there.
The GUI compiles against the ES2020 library, which has no Array.prototype.at. Vitest does not type-check, so only npm run build noticed.
The empty state for a build without the pcap feature told users to use a PCAP-enabled NetsCLI Desktop build, which suggests a download that does not exist. RELEASE.md says the published GUI installers are intentionally built without --features pcap, and the packet-capture docs say the same. The text now says that and that it needs a build made from source with the pcap feature, and points at the setup docs button below it.
The toolbar built every row of the result and scanned each column for its values in its render body to get the filter box's hints, the workspace recomputed the column list each render, and the raw JSON preview tokenised its text each render. The app re-renders on every progress event, selection change and status poll, and none of these depend on anything that changes then. They are now memoised on what they read (the tab's kind and result, the rows, the text). filterHintsFor takes just the kind and result so the dependencies are exact. This is the audit's recommendation and is not measured. The audit's own estimate was that the cost is probably tolerable for the largest published result, and no timing was taken here, so the claim is only that repeated work is gone, which the toolbar test counts. The other two memoisations have no test of their own.
Checked in the browser with the real stylesheet, with the OS theme set to light. The first version of the new crash screen drew its Reload button as the browser's light grey face carrying the light text colour it inherits from .container, rgb(228, 231, 239) on rgb(240, 240, 240). Nothing global styles a bare button. It now takes the same border, background and text tokens as the dialogs' action buttons.
…ze cap presentation.test.ts went over the 300 line limit with the new tests and has no transition exception. They now live in presentation.commands.test.ts, with one more case for the mDNS service type list, which goes through the same helper from a single comma-separated field. The test for escaping a quote in a capture filter is gone because that behaviour was replaced on purpose in the quoting change.
…trace changes The comments claimed a Windows Terminal window on every Windows 11 and a freeze, and named three languages for the OEM code page problem. What was measured is a Windows Terminal window with Windows Terminal as the default terminal, and the German ü in code page 850 is a byte that is not UTF-8. Comment text only.
The target is handed to tracert, traceroute or tracepath as a plain argument, so 'netscli trace -- -d' reached the tool as a flag. Windows' tracert also reads a leading slash as an option ('tracert /d /h 1 127.0.0.1' runs as '-d -h 1', checked) and has no -- to end its options ('-- is not a valid command option', checked), so refusing is the fix that works on every platform. trace_route now returns an invalid-input error for an empty target or one that starts with a dash or a slash, before anything is spawned. A host name or an address never starts with either. The core has no hostname validator to reuse, and a stricter check would turn away names that resolve, internationalised ones among them, so only the leading character is checked.
…behaviour was The test comment now states what trace_route returned without the check, which was observed, and the changelog gets an entry.
Measured on Windows with debug builds of the desktop app, started with a throwaway WebView2 profile, with a small program sending WM_NULL to the main window every 200 ms and a script invoking open_result_bundle from the page. The build from the base commit answered none of 25 probes over 8 seconds while the Open NetsCLI Result dialog was open, and IsHungAppWindow turned true after about 5 seconds. The build from this branch answered 40 of 40 with the same dialog open, and 20 of 20 with the Choose NetsCLI Save Folder dialog open. Both builds showed the dialog window. The macOS half of the claim is still from reading rfd and has not been run on a Mac.
trace.rs went over the 300 line limit with the option refusal and its tests, and has no transition exception. The tests now live in trace/tests.rs, the same arrangement as common/ports/tests.rs. They are unchanged.
Contributor
|
Site preview: https://pr-556.netscli-site-preview.pages.dev Built from eda298b with Production is unaffected: netscli.com is served from GitHub Pages via |
# Conflicts: # CHANGELOG.md
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.
Audit fixes for the desktop app (
areas/desktop-app.md, local only), plus the trace fixes in the core library.What changes for users
ping -a, used to look up names during Discover and Sweep,tracert, and the MCP--versionprobe were started withoutCREATE_NO_WINDOW. Measured with a windowless program calling the real core: 4 visible console windows per call before, 0 after. Every other child process was checked.-or/).data:,blob:andasset:for images; the built app uses none.Checked (by the fixing agent, in its own target dir)
cargo fmt --all --check.cargo clippy -p netscli-core -p netscli-gui --all-targets -- -D warnings.cargo test: core 131 unit tests plus its integration tests, gui 39.apps/netscli-gui, all clean:npm run lintnpm run test:unit: 292 tests in 52 filesnpm run test:maintainabilitynpm run builddata:orblob:images under the new CSP and found none.Please check by hand before release