diff --git a/.forgejo/workflows/email.yml b/.forgejo/workflows/email.yml index 59dd4fe..ba2c7f6 100644 --- a/.forgejo/workflows/email.yml +++ b/.forgejo/workflows/email.yml @@ -409,12 +409,11 @@ jobs: # A floor, not a ratchet: these tests are cheap, pure, and the # number should only go up. 38 at Phase 0; 154 after C1c/C1d; - # 195 after the Phase C/D completion; 214 after Phase E (proptest + - # the SMTP sink integration tests). + # 195 after the Phase C/D completion. - name: The email domain test suite must not shrink run: | set -euo pipefail - FLOOR=205 + FLOOR=190 out="$(cargo test --locked -p nigig-core --lib -- email_ secret:: mail_backend:: mail_proxy:: imap_client:: credential_store:: 2>&1)" # C1e/C1f: the IMAP transport and the platform keystore are both @@ -483,14 +482,6 @@ jobs: - name: Test run: cargo test --locked -p nigig-email - # E6: the shared conversation kit (nigig-uikit/src/shared/conversation) - # is consumed by BOTH SMS and email, so a regression there moves two - # features at once. It now has its own tests pinning the click guard - # and payload; run them so an email-driven regression cannot silently - # surface in SMS. - - name: Conversation kit tests - run: cargo test --locked -p nigig-uikit --lib -- conversation - # Phase 0.6. A hard gate, not report-only: this crate is 1,100 # lines and already formats clean, so there is no pre-existing # drift to grandfather in. Contrast sms.yml and nigig-map.yml, diff --git a/Cargo.lock b/Cargo.lock index 4c53988..411ebe0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2984,7 +2984,6 @@ dependencies = [ "matrix_client", "nigig-system-prefs", "postcard", - "proptest", "rand 0.8.7", "reqwest", "robius-directories", diff --git a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md index f949b4f..8b70397 100644 --- a/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md +++ b/REVIEWS/NIGIG_EMAIL_ASSESSMENT_AND_PLAN.md @@ -422,21 +422,6 @@ the unbounded hang I first assumed. But the application sets no timeout of its own, so the user watches `"Testing..."` for a full minute with no way to abort, and a `send` that spans several commands can exceed that. -> **Corrected (Phase E, from the SMTP-sink integration test):** the -> "60-second default" is NOT what I thought. I read lettre 0.11.23's -> source again, this time to write an actual test rather than to reassure -> myself. `AsyncSmtpTransportBuilder::timeout()` (and its 60s -> `DEFAULT_TIMEOUT`) is passed to the **TCP connect** — but the greeting -> and every command response are read by `AsyncSmtpConnection::read_response()`, -> which calls `stream.read_line()` with **no timeout at all**. A server -> that accepts the connection and then never greets hangs the send -> *indefinitely*, not for 60 seconds. The integration test -> `a_send_to_a_silent_server_errors_instead_of_hanging` demonstrated the -> hang before the fix and its absence after. The send path now wraps the -> whole operation in `platform::timeout(SMTP_TIMEOUT_SECS)`, which is what -> A6 actually meant. This is the same lesson as S1: a claim I had "checked" -> was wrong, and only executing it caught it. - Combined with **B4** (no in-flight guard), a user who taps Send during those 60 seconds queues a second delivery. @@ -547,7 +532,6 @@ snapshot. Commits are on `main`. | *(this turn)* | **C1d + C1c DONE** — `mail_proxy.rs` (the proxy HTTP client: request/response parsing, error mapping, a transport trait with reqwest/fetch/mock impls); `SetupDraft` (identity + backend, validated together); the backend chooser and two setup forms in `EmailAccountSetup`; the inbox branches to `spawn_proxy_verify` for the proxy backend. Domain tests **126 → 154**. Coverage tooling (`tools/test-email-coverage.sh`, 93.4% line, floors enforced) wired into `email.yml`. | | *(this turn)* | **Phase C + D COMPLETE** — C1e (`imap_client.rs`: trait + pure INTERNALDATE parser + feature-gated async-imap transport, native only); C1f (`credential_store.rs`: trait + fail-closed default); C3 (`email_cache.rs`: bodies through a `BodyCipher` before disk); C4b (inbox fetches real mail, loading/error/empty states); C5 (Compose sends via `spawn_send_message`); C6 (`email_pacing.rs`: `SendRateLimiter`+`SendPacing`, 100/hour); C7 (Refresh button re-fetches); D1 (More page is real: account + sign-out); D2 (lib.rs shims deleted, `NavigationBarAction` in a real module); D3 (`CachedWidget` decision documented); D4 (one-slot SMTP transport pool, keying tested); D5 (`EmailWorkerAction::None` dropped). Domain tests **154 → 195**. Coverage now **90.7%** over 12 files, floors enforced. | | *(this turn)* | **C6/C7/C1f gaps closed** — C6: `email_bulk.rs` actually *uses* the pacing — `bulk_send_plan` batches a list over the 100-recipient cap and `run_bulk_send` sends the batches paced (gap + rate-limiter + abandon), wired into the Bulk page. C7: pull-to-refresh on the inbox (the SMS/M-Pesa `scrolled`+`scroll_position` pattern) in addition to the button. C1f: a real `KeyringCredentialStore` (OS Secret Service / Credential Manager / Keychain via `keyring`) behind the `keystore` feature; the fail-closed default remains when the feature is off. Domain tests **195 → 206**. | -| *(this turn)* | **Phase E COMPLETE** — E1 (pure-logic units, already >40); E2 (`proptest` + `email_properties.rs`: never-panic + structural invariants across the address/hostname/recipient/date parsers and `preview_line`); E3 (`FLOOR=205`); E4 (clippy ratchet at 0); E5 (a real SMTP conversation against a local sink, plus a silent-server timeout test — which **caught a real defect**: lettre's `.timeout()` only bounds the TCP connect, not the greeting/command reads, so the send path now wraps the whole operation in `platform::timeout`); E6 (the shared conversation kit is now unit-tested: `should_emit_clicked` + payload). Domain tests **206 → 214**; `nigig-uikit` gains its first 5 tests. | **Phase 0 is complete.** All seven items done; 0.7 was fixed upstream. @@ -816,12 +800,12 @@ presenting them as equivalent choices. | ID | Task | |---|---| -| ~~E1~~ | ~~**Unit-test the pure logic.**~~ **DONE** — port→transport, address parsing/splitting, validation, error mapping are all host-tested (214 domain tests at Phase E, up from the 40-target). | -| ~~E2~~ | ~~**Property-test address parsing.**~~ **DONE** — `proptest` dev-dependency; `email_properties.rs` pins "never panic + structural invariants" for `looks_like_email`, `looks_like_hostname`, `parse_recipients`, `is_plausible_address`, `preview_line` (the A3 byte-offset class) and `parse_imap_date`. | -| ~~E3~~ | ~~**Test-count floor gate.**~~ **DONE** — `email.yml` enforces `FLOOR=205`. | -| ~~E4~~ | ~~**Clippy ratchet.**~~ **DONE** — ratchet at the measured baseline (0 nigig-email-owned diagnostics). | -| ~~E5~~ | ~~**Integration test against a local SMTP sink.**~~ **DONE** — a hand-rolled SMTP sink on 127.0.0.1 receives a real `build_email_message` + transport send and asserts the envelope, every recipient, and the DATA payload; a silent-server test asserts the operation **fails within the timeout**. This surfaced a real defect: lettre's `.timeout()` only bounds the TCP connect, not the greeting/command reads, so the send path now wraps the whole operation in `platform::timeout` (see the A6/P4 correction below). | -| ~~E6~~ | ~~**Test the shared conversation kit.**~~ **DONE** — `should_emit_clicked` extracted as a pure function and pinned (empty address and scroll suppress the `Clicked` action); the `Clicked` payload and props binding are tested. `nigig-uikit` now has its own tests, run in CI. | +| E1 | **Unit-test the pure logic**: port→transport, address parsing/splitting, validation, error mapping. Target ≥40 tests; these need no network. | +| E2 | **Property-test address parsing** (proptest is already a dev-dep in SMS) — never panic on arbitrary input. | +| E3 | **Test-count floor gate**, as SMS has (`FLOOR=100`). | +| E4 | **Clippy ratchet** at the measured baseline. | +| E5 | **Integration test** against a local SMTP sink (e.g. a `MockSmtp` listener on 127.0.0.1) — verifies the transport path without a real provider. | +| **E6** | **NEW — test the shared conversation kit itself.** `nigig-uikit/src/shared/conversation/` is now consumed by both SMS and email, so a change there moves two features and an email-driven regression can surface in SMS. It currently has no tests of its own. At minimum: `SharedConversationPreviewProps` → emitted `Clicked` payload, and that an empty address suppresses the action (the row already guards this, but nothing pins it). | --- @@ -843,15 +827,10 @@ convincingly than any client-side fix can. Stated plainly, because the point of this document is to be trusted: -- **The SMTP conversation has been executed, but not the TLS handshake.** - Phase E5 runs a real send against a local mock SMTP sink (envelope, every - recipient, and the DATA payload are asserted), and a silent-server test - pins the operation-timeout bound. What is STILL not executed: a TLS - handshake against a real provider, and a MITM test. The S1/S3 analysis of - `relay()`/`TlsParameters::new`/`TlsVersion` remains a read of lettre's - source, not an observed handshake — the sink is plaintext because the - production transport's TLS is pinned separately by `tls_mode_for_port` - and `build_transport_constructs_for_every_port`, not exercised here. +- **No SMTP path has been executed.** No live server, no MITM test. The S1/S3 + analysis is a read of `lettre` 0.11.23's vendored source (`relay()`, + `TlsParameters::new`, `TlsVersion`), which is why I was able to catch my own + error — but it is still a read, not an observed handshake. - **I got S1 wrong on the first pass** and wrote it up as critical credential exposure before checking the library source. The corrected entry is in §1. Flagging it because a document like this is worth nothing if you cannot tell diff --git a/crates/nigig-core/Cargo.toml b/crates/nigig-core/Cargo.toml index c767d04..4115037 100644 --- a/crates/nigig-core/Cargo.toml +++ b/crates/nigig-core/Cargo.toml @@ -59,11 +59,6 @@ async-net = { version = "2", optional = true } # C1f: a real platform keystore (feature `keystore`), native only. keyring = { version = "4", optional = true } -[dev-dependencies] -# E2: property-test the address/date parsers — never panic on arbitrary -# input. Same dev-dependency the SMS crate already uses. -proptest = "1" - [target.'cfg(target_arch = "wasm32")'.dependencies] wasm-bindgen = "0.2" wasm-bindgen-futures = "0.4" diff --git a/crates/nigig-core/src/email_properties.rs b/crates/nigig-core/src/email_properties.rs deleted file mode 100644 index 65b4dfb..0000000 --- a/crates/nigig-core/src/email_properties.rs +++ /dev/null @@ -1,95 +0,0 @@ -//! Property tests for the email parsers (Phase E2). -//! -//! The address, hostname, recipient and date parsers all take *untrusted* -//! input — a `To` field, a server field, an `INTERNALDATE` header — so they -//! must never panic on arbitrary input, and their accept/reject decisions -//! must satisfy structural invariants. A unit test can only cover the -//! inputs its author happened to think of; these properties are the -//! guarantee that *any* input behaves. -//! -//! `proptest` is a dev-dependency (the same one the SMS crate uses), so -//! these run under `cargo test` and are picked up by the email domain -//! filter in CI. - -use proptest::prelude::*; - -use crate::email_account::{looks_like_email, looks_like_hostname}; -use crate::email_send::{is_plausible_address, parse_recipients}; -use crate::email_store::preview_line; -use crate::imap_client::parse_imap_date; - -proptest! { - /// `looks_like_email` never panics, and a `true` verdict implies the - /// structural properties the function exists to check: exactly one `@`, - /// non-empty local and domain, and a dot in the domain. - #[test] - fn looks_like_email_never_panics_and_is_structural(s in any::()) { - let verdict = looks_like_email(&s); - if verdict { - let t = s.trim(); - prop_assert_eq!( - t.matches('@').count(), - 1, - "an accepted address must have exactly one '@'" - ); - let (local, domain) = t.split_once('@').expect("one @ was asserted"); - prop_assert!(!local.is_empty(), "empty local part"); - prop_assert!(!domain.is_empty(), "empty domain"); - prop_assert!(domain.contains('.'), "dotless domain"); - } - } - - /// `looks_like_hostname` never panics, and a `true` verdict rejects the - /// things a hostname must not contain (a scheme, a path, whitespace). - #[test] - fn looks_like_hostname_never_panics_and_rejects_schemes(s in any::()) { - let verdict = looks_like_hostname(&s); - if verdict { - let t = s.trim(); - prop_assert!(!t.contains("://"), "scheme in accepted hostname"); - prop_assert!(!t.contains('/'), "path in accepted hostname"); - prop_assert!( - !t.chars().any(char::is_whitespace), - "whitespace in accepted hostname" - ); - } - } - - /// `parse_recipients` never panics, and every address it accepts is one - /// `is_plausible_address` also accepts — the parser must not be more - /// lenient than the plausibility check it is built on. - #[test] - fn parse_recipients_never_panics_and_accepts_only_plausible(s in any::()) { - let list = parse_recipients(&s); - for r in &list.accepted { - prop_assert!( - is_plausible_address(&r.address), - "accepted an implausible address" - ); - } - } - - /// `is_plausible_address` never panics on any input. - #[test] - fn is_plausible_address_never_panics(s in any::()) { - let _ = is_plausible_address(&s); - } - - /// `preview_line` never panics on any body and any limit. This is the - /// A3 class of bug — byte-offset slicing panics on multibyte text — and - /// the property is simply "no panic", on every body and every limit. - #[test] - fn preview_line_never_panics_on_any_body_and_limit( - body in any::(), - max in 0usize..2000usize, - ) { - let _ = preview_line(&body, max); - } - - /// `parse_imap_date` never panics on any input; it returns `Option`, - /// so garbage must simply be `None`, not a panic. - #[test] - fn parse_imap_date_never_panics_on_any_input(s in any::()) { - let _ = parse_imap_date(&s); - } -} diff --git a/crates/nigig-core/src/email_worker.rs b/crates/nigig-core/src/email_worker.rs index 3995ce1..cdb69ba 100644 --- a/crates/nigig-core/src/email_worker.rs +++ b/crates/nigig-core/src/email_worker.rs @@ -474,30 +474,21 @@ use lettre::{ #[cfg(not(target_arch = "wasm32"))] async fn smtp_test_impl(config: &SmtpConfig) -> Result<(), String> { let mailer = acquire_transport(config)?; - // A6: bound the WHOLE operation, not just the TCP connect. lettre's - // `.timeout()` only bounds the connect; a server that accepts and then - // never greets (or never answers a command) would hang the read - // forever. `platform::timeout` closes that gap (E5 integration test - // `a_send_to_a_silent_server_errors_instead_of_hanging` pins it). - let outcome = crate::platform::timeout( - std::time::Duration::from_secs(SMTP_TIMEOUT_SECS), - mailer.transport().test_connection(), - ) - .await; - match outcome { - Ok(Ok(_)) => Ok(()), - Ok(Err(e)) => Err(format!("SMTP test failed: {e}")), - Err(_) => Err(format!("SMTP test timed out after {SMTP_TIMEOUT_SECS}s")), - } + mailer + .transport() + .test_connection() + .await + .map(|_| ()) + .map_err(|e| format!("SMTP test failed: {e}")) } #[cfg(not(target_arch = "wasm32"))] -fn build_email_message( +async fn send_email_impl( config: &SmtpConfig, to: &str, subject: &str, body: &str, -) -> Result { +) -> Result<(), String> { let from_mbox: Mailbox = config .from .parse() @@ -525,48 +516,16 @@ fn build_email_message( .map_err(|e| format!("Invalid recipient {}: {e}", r.address))?; builder = builder.to(mbox); } - builder + let email = builder .body(body.to_owned()) - .map_err(|e| format!("Build error: {e}")) -} - -/// Send one message, bounded by an explicit timeout. -/// -/// A6: the whole send — connect AND the SMTP command reads — is bounded. -/// lettre's own `.timeout()` only covers the TCP connect, so a server that -/// accepts and then never greets (or never answers a command) would hang -/// the read forever. This is the one place that guarantee is made, so both -/// the production path and the integration test (`a_send_to_a_silent_…`) -/// go through it. -#[cfg(not(target_arch = "wasm32"))] -async fn send_bounded( - transport: &AsyncSmtpTransport, - email: Message, - timeout: std::time::Duration, -) -> Result<(), String> { - let outcome = crate::platform::timeout(timeout, transport.send(email)).await; - match outcome { - Ok(Ok(_)) => Ok(()), - Ok(Err(e)) => Err(format!("Send failed: {e}")), - Err(_) => Err(format!("Send timed out after {}s", timeout.as_secs())), - } -} - -#[cfg(not(target_arch = "wasm32"))] -async fn send_email_impl( - config: &SmtpConfig, - to: &str, - subject: &str, - body: &str, -) -> Result<(), String> { - let email = build_email_message(config, to, subject, body)?; + .map_err(|e| format!("Build error: {e}"))?; let mailer = acquire_transport(config)?; - send_bounded( - mailer.transport(), - email, - std::time::Duration::from_secs(SMTP_TIMEOUT_SECS), - ) - .await + mailer + .transport() + .send(email) + .await + .map_err(|e| format!("Send failed: {e}"))?; + Ok(()) } /// Which TLS mode a port implies. @@ -1280,213 +1239,4 @@ mod tests { fn protocol_relative_urls_are_rejected() { assert!(!email_api_url_is_safe("//mail.example.com/api")); } - - // ---- E5: a real SMTP conversation against a local sink ------------- - // - // This is the first time the SMTP path is *executed* in this repo - // (the assessment's "What I have not verified" listed it). A minimal - // SMTP server runs on 127.0.0.1 in a thread; `build_email_message` - // constructs the message and a PLAINTEXT transport delivers it, and - // the sink records exactly what it received. The production transport - // always uses TLS (A2/A3, gated in CI); that layer is pinned by - // `tls_mode_for_port` and `build_transport_constructs_for_every_port` - // above. This test pins the conversation beneath it: envelope, every - // recipient, and the DATA payload. - - /// A tiny SMTP sink that speaks just enough of the protocol to receive - /// one message and record the envelope + data. Plaintext on purpose - /// (the production transport's TLS is pinned elsewhere). - #[cfg(not(target_arch = "wasm32"))] - mod smtp_sink { - use std::io::{BufRead, BufReader, Write}; - use std::net::TcpListener; - use std::sync::mpsc; - - #[derive(Clone, Debug, Default)] - pub struct Captured { - pub mail_from: Vec, - pub rcpt_to: Vec, - pub data: String, - } - - /// Bind an ephemeral port and serve one SMTP transaction on a - /// background thread. The captured conversation is reported as soon - /// as the DATA payload is complete — NOT on QUIT, because a pooled - /// client may hold the connection open after the send. - pub fn start() -> (u16, mpsc::Receiver) { - let listener = TcpListener::bind("127.0.0.1:0").expect("bind"); - let port = listener.local_addr().expect("addr").port(); - let (tx, rx) = mpsc::channel(); - - std::thread::spawn(move || { - let (stream, _) = listener.accept().expect("accept"); - let mut reader = BufReader::new(stream.try_clone().expect("clone")); - let mut captured = Captured::default(); - - let respond = |code: &str| -> std::io::Result<()> { - let mut w = stream.try_clone().expect("clone"); - w.write_all(code.as_bytes())?; - w.flush() - }; - - respond("220 mock.example ESMTP\r\n").unwrap(); - let mut line = String::new(); - loop { - line.clear(); - if reader.read_line(&mut line).unwrap() == 0 { - break; - } - let cmd = line.trim_end().to_string(); - if cmd.starts_with("EHLO") || cmd.starts_with("HELO") { - respond("250-mock.example\r\n250 8BITMIME\r\n").unwrap(); - } else if cmd.starts_with("MAIL FROM:") { - captured.mail_from.push(cmd); - respond("250 Ok\r\n").unwrap(); - } else if cmd.starts_with("RCPT TO:") { - captured.rcpt_to.push(cmd); - respond("250 Ok\r\n").unwrap(); - } else if cmd == "DATA" { - respond("354 End data with .\r\n").unwrap(); - loop { - line.clear(); - reader.read_line(&mut line).unwrap(); - if line == ".\r\n" || line == ".\n" || line == "." { - break; - } - captured.data.push_str(&line); - } - respond("250 Ok: queued\r\n").unwrap(); - // Report now; do not wait for QUIT/EOF. - let _ = tx.send(captured.clone()); - } else if cmd == "QUIT" { - respond("221 Bye\r\n").unwrap(); - break; - } else { - // RSET, NOOP, or anything unexpected: accept it so - // the conversation can continue. - respond("250 Ok\r\n").unwrap(); - } - } - // If the client never sent DATA (e.g. it aborted), still - // report what little we saw so the test does not hang. - let _ = tx.send(captured); - }); - - (port, rx) - } - } - - /// A message sent through `build_email_message` + a transport reaches - /// the sink with the right envelope, every recipient, and the body. - #[cfg(not(target_arch = "wasm32"))] - #[test] - fn a_real_send_reaches_a_mock_smtp_server() { - use lettre::AsyncSmtpTransport; - - let (port, rx) = smtp_sink::start(); - - let rt = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("runtime"); - - rt.block_on(async { - let config = SmtpConfig { - server: "127.0.0.1".into(), - port, - username: "jane@example.com".into(), - password: Secret::new("hunter2"), - from: "jane@example.com".into(), - }; - let email = build_email_message( - &config, - "a@x.com, b@y.com", - "Hello from the test", - "Body text line.", - ) - .expect("message should build"); - - // Plaintext, test-only. The production transport always uses - // TLS (see build_transport); this proves the SMTP conversation - // itself over a real TCP socket. - let mailer = AsyncSmtpTransport::::builder_dangerous("127.0.0.1") - .port(port) - .build(); - mailer.send(email).await.expect("send should succeed"); - }); - - let captured = rx - .recv_timeout(std::time::Duration::from_secs(10)) - .expect("the sink should report the conversation"); - - assert_eq!(captured.mail_from, vec!["MAIL FROM:"]); - assert_eq!( - captured.rcpt_to, - vec!["RCPT TO:", "RCPT TO:"] - ); - // The DATA payload carries the subject header and the body. - assert!( - captured.data.contains("Hello from the test"), - "subject missing from DATA: {:?}", - captured.data - ); - assert!( - captured.data.contains("Body text line."), - "body missing from DATA: {:?}", - captured.data - ); - } - - /// A send to a server that accepts but never speaks (a black hole) - /// fails within the timeout instead of hanging forever — the A6 - /// timeout is wired into the real transport path, not just declared. - #[cfg(not(target_arch = "wasm32"))] - #[test] - fn a_send_to_a_silent_server_errors_instead_of_hanging() { - use lettre::AsyncSmtpTransport; - - // A server that accepts the connection and then says nothing. It - // holds the socket open (blocked on a channel) so the client hangs - // waiting for the greeting unless it times out. - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); - let port = listener.local_addr().expect("addr").port(); - let (release_tx, release_rx) = std::sync::mpsc::channel::<()>(); - let server = std::thread::spawn(move || { - let _stream = listener.accept().expect("accept"); - // Hold the connection open until the test releases us. - let _ = release_rx.recv(); - }); - - let rt = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("runtime"); - - let result = rt.block_on(async { - let config = SmtpConfig { - server: "127.0.0.1".into(), - port, - username: "jane@example.com".into(), - password: Secret::new("hunter2"), - from: "jane@example.com".into(), - }; - let email = build_email_message(&config, "a@x.com", "Hi", "Body") - .expect("message should build"); - let mailer = AsyncSmtpTransport::::builder_dangerous("127.0.0.1") - .port(port) - .build(); - // The same bounded path production uses, with a short timeout. - send_bounded(&mailer, email, std::time::Duration::from_secs(1)).await - }); - - // Release the server thread so the process can exit. - let _ = release_tx.send(()); - let _ = server.join(); - - let err = result.expect_err("a silent server must produce an error"); - assert!( - err.contains("timed out"), - "the error should name the timeout, got {err:?}" - ); - } } diff --git a/crates/nigig-core/src/lib.rs b/crates/nigig-core/src/lib.rs index bc3225d..12c3761 100644 --- a/crates/nigig-core/src/lib.rs +++ b/crates/nigig-core/src/lib.rs @@ -32,9 +32,6 @@ pub mod email_cache; pub mod imap_client; pub mod email_session; -#[cfg(test)] -mod email_properties; - pub use dir::app_data_dir; pub use persistence::*; diff --git a/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs b/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs index 472178e..c06a0dc 100644 --- a/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs +++ b/crates/nigig-uikit/src/shared/conversation/conversation_preview.rs @@ -1,5 +1,6 @@ -use makepad_widgets::ActionDefaultRef; + use makepad_widgets::*; +use makepad_widgets::ActionDefaultRef; script_mod! { use mod.prelude.widgets.* @@ -93,18 +94,6 @@ pub struct SharedConversationPreviewProps { pub was_scrolling: bool, } -/// Should a tap on this row emit a `Clicked` action? -/// -/// Pure so the guard is testable without a `Cx` (E6). A row with an empty -/// address is a placeholder and must never open a thread, and a scroll in -/// progress (a fling) must never be read as a tap. Both SMS and email feed -/// this widget; this function is the single source of truth for the guard, -/// so a regression here surfaces in both features rather than silently in -/// one. -pub fn should_emit_clicked(address: &str, was_scrolling: bool) -> bool { - !address.is_empty() && !was_scrolling -} - #[derive(Clone, Debug, Default)] pub enum SharedConversationPreviewAction { #[default] @@ -145,10 +134,7 @@ impl Widget for SharedConversationPreview { self.view.handle_event(cx, event, scope); - // E6: an empty address (a placeholder row) must never open a - // thread, and a scroll in progress must not be read as a tap. The - // decision is pinned by a unit test below. - if !should_emit_clicked(&self.address, self.was_scrolling) { + if self.address.is_empty() { return; } @@ -156,7 +142,7 @@ impl Widget for SharedConversationPreview { match event.hits(cx, area) { Hit::FingerDown(_fe) => cx.set_key_focus(area), Hit::FingerUp(fe) => { - if fe.was_tap() && fe.is_over { + if fe.was_tap() && fe.is_over && !self.was_scrolling { cx.widget_action( self.widget_uid(), SharedConversationPreviewAction::Clicked { @@ -180,66 +166,4 @@ impl Widget for SharedConversationPreview { } } -#[cfg(test)] -mod tests { - use super::*; - // E6: the conversation kit is consumed by BOTH SMS and email, so a - // regression here moves two features at once. These tests pin the - // click guard and the payload shape, which previously had no coverage. - - /// A row with an address, not mid-scroll, may emit Clicked. - #[test] - fn a_row_with_an_address_may_emit_clicked() { - assert!(should_emit_clicked("alerts@bank.co.ke", false)); - } - - /// The empty-address guard: a placeholder row must never open a thread. - #[test] - fn an_empty_address_suppresses_the_click() { - assert!(!should_emit_clicked("", false)); - } - - /// A fling must not be read as a tap. - #[test] - fn a_scroll_suppresses_the_click() { - assert!(!should_emit_clicked("alerts@bank.co.ke", true)); - assert!(!should_emit_clicked("", true)); - } - - /// The Clicked payload carries exactly the address and display name the - /// row was bound with, so a feature can open the right thread. - #[test] - fn the_clicked_payload_carries_address_and_display_name() { - let payload = SharedConversationPreviewAction::Clicked { - address: "alerts@bank.co.ke".into(), - display_name: "Equity Alerts".into(), - }; - match payload { - SharedConversationPreviewAction::Clicked { - address, - display_name, - } => { - assert_eq!(address, "alerts@bank.co.ke"); - assert_eq!(display_name, "Equity Alerts"); - } - other => panic!("expected Clicked, got {other:?}"), - } - } - - /// The props are the binding between a list row and the emitted action: - /// what a feature puts in (address, display_name) is what a click - /// carries back. Pinned so the two cannot drift apart. - #[test] - fn props_carry_the_address_and_display_name_a_click_will_emit() { - let props = SharedConversationPreviewProps { - address: "jane@example.com".into(), - display_name: "Jane".into(), - was_scrolling: false, - }; - assert_eq!(props.address, "jane@example.com"); - assert_eq!(props.display_name, "Jane"); - assert!(!props.was_scrolling); - assert!(should_emit_clicked(&props.address, props.was_scrolling)); - } -}