Repository navigation
Move blocking work off tokio workers and fix GUI interaction defects (B-10, B-11, B-36, M-1/2/3/5/7, C-12/16/32) - #175
Merged
Conversation
B-10, five sync-in-async call sites:
- `event::poll(tick_rate)` parked a tokio worker for 100ms every loop
iteration whether or not a key arrived. Poll and read move together to
a blocking thread so the read cannot race another poller.
- `NetworkManager::find_mac` shells out to `arp` on Windows and macOS.
- `pcap_check_support` enumerates devices via a blocking syscall, twice
per /pcap. Uses block_in_place because `ops` is a borrow, not 'static.
- `get_stats` holds a mutex across a `Networks::refresh` syscall and was
called from the draw path. Drawing is synchronous so it cannot yield;
the sample moves to the event loop and the draw path reads cached
numbers.
- `detect_default_ipv4_addr` at startup delayed every path behind it.
B-11: /config wrote settings on every arrow keypress -- a synchronous
fs::write + fs::rename per key event, on the event-loop thread, so key
repeat meant one full file rewrite per repeat tick. Cycling now marks the
settings dirty and the single write happens in exit_config, which every
close path (Esc, Done, toggle) already funnels through. Reset still writes
immediately, being discrete and destructive.
B-36: the spinner advanced an index from the draw path, coupling its speed
to the frame rate and making rendering mutate state. It is now derived from
wall time, so it spins at a constant rate and takes &self.
GUI:
M-1: History menu items keyed on their label, so two runs of the same
command collided and React dropped one.
M-2: keyboard column resize read `column.width` -- the static column
definition, not the live width -- so every press recomputed from the
same base and holding an arrow moved the border one step and stopped.
M-3: NumberField clamped on every keystroke, so in a field with min 10
the "5" of "50" became "10" before the "0" arrived, and the field could
not be cleared. Clamping moves to blur/Enter/steppers.
M-5: the menu strip had no menubar semantics.
M-7: the table and detail pane disagreed on latency -- a closed port read
'' in one and 'refused' in the other, and both were used inside the
detail pane. One function now.
C-12: DetailList keys collided for two identical TXT records.
C-16: the progress line counted `1-1024` as one port and said "1 ports".
C-32: duplicate App.css import; the non-null root assertion now throws a
real message.
94 GUI tests (up from 88) and 134 Rust tests. clippy clean with and
without --features pcap.
Verified in the running GUI: menubar exposes 7 menuitems, and the number
field keeps partial input and stays clearable. NumberField's commit paths
are covered by component tests rather than the browser, because a focus
trap in the app prevented a real blur through the automation harness.
B-28: exerciseDns resolved netscli.com against real public DNS with a 25s budget, while every other scenario is hermetic against the local probe server -- so the suite failed on an air-gapped runner or a slow resolver for reasons that had nothing to do with the app. It now uses `localhost`, which resolves through the system resolver without leaving the host and has both A and AAAA records, so the record-type assertions still exercise what they were written for. NETSCLI_E2E_DNS_HOST overrides it. M-13: teardown called child.kill(), which signals only the direct child. The processes here are supervisors -- tauri-driver spawns the platform WebDriver, which spawns the browser -- so grandchildren survived, kept holding their ports, and the next run failed to bind or attached to a stale session; on CI the job hung until its timeout. Now kills the tree: taskkill /T on Windows, and the process group elsewhere, with tauri-driver spawned detached so it leads one. M-14: roughly a third of assertions pinned pixel measurements, some to within 3px. Those catch real layout regressions but at that precision they also fail for reasons that are not bugs -- font rendering, fractional DPI, platform scrollbar width, a Chrome subpixel-rounding change. A suite that cries wolf gets muted, and a muted suite catches nothing. Alignment checks now pass within tolerance, warn and record beyond it, and fail only on gross misalignment (4x tolerance, i.e. visibly broken). Drift is summarised at the end of the run so the warnings do not scroll past. Deliberately kept strict: a non-numeric or non-finite delta still fails hard, since that means the measurement itself broke -- exactly the "check that cannot fail" shape this is meant to avoid. M-16: assertOperationToastReturnsToTab did `inactiveTab?.click()`. With no inactive tab the optional chain silently did nothing, and the final assertion then passed trivially because the expected tab had never been left. It now asserts a second tab exists first. Checked shell.mjs's `if (!mark) return null` -- that one is already caught by an assert.ok on the result, so it is not a silent skip.
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.
B-10 — five sync-in-async call sites
event::poll(tick_rate)NetworkManager::find_macarpon Windows/macOS — a whole subprocess inline.pcap_check_support/pcap. Usesblock_in_placebecauseopsis a borrow, not'static.get_statsNetworks::refreshsyscall — from the draw path. Drawing is synchronous so it can't yield; the sample moves to the event loop and drawing reads cached numbers.detect_default_ipv4_addrB-11 — a file rewrite per keypress
/configdid a synchronousfs::write+fs::renameon every arrow keypress, on the event-loop thread — so key repeat meant one full file rewrite per repeat tick. Cycling now marks settings dirty; the single write happens inexit_config, which every close path (Esc, Done, toggle) already funnels through. Reset still writes immediately, being discrete and destructive.B-36 — spinner driven by frame rate
It advanced an index from the draw path, coupling its speed to how often the screen redrew and making rendering mutate state. Now derived from wall time, so it spins at a constant rate and takes
&self.GUI
''in one andrefusedin the other, and both were used inside the detail pane. One function now.column.width, the static definition rather than the live width, so holding an arrow moved the border one step and stopped.NumberFieldclamped every keystroke: withmin: 10, the "5" of "50" became "10" before the "0" arrived, and the field couldn't be cleared.1-1024as one port and said "1 ports".App.cssimport; the root assertion now throws a real message.Verification
94 GUI tests (up from 88), 134 Rust tests, clippy clean with and without
--features pcap.Checked live in the running GUI: menubar exposes 7
menuitems; the number field keeps partial input and stays clearable.One honest note: I could not verify
NumberField's blur path through the browser — a focus trap in the app kept focus in the field, and my syntheticblurevents never firedfocusout. Rather than claim it worked, I covered all three commit paths (blur, Enter, steppers) with component tests, which is more durable anyway.One existing test asserted the old M-7 divergence; it's updated with a note explaining why the expected value changed.