From 44b0e4bbd0c25f422435435717f06ac7852f74a1 Mon Sep 17 00:00:00 2001 From: James Kane Date: Sun, 16 Aug 2026 04:36:28 -0500 Subject: [PATCH] refactor(realign): one rule, one target constant, and the docs back on their own items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A four-angle review of the realignment surface, prompted by two bugs that both traced to the same rule existing in several copies. **The duplication that caused those bugs was still there.** `DEFAULT_TARGET_BUILD` was exported so the UI could name the build it offers, and then only the Simple mode modal used it: both Advanced cards still carried the literal, and one compared builds with `eq_ignore_ascii_case` where the job uses `builds_match`, which trims. A stored " chm13v2.0" was offered by the card and refused by the job. `is_target_build` is now `pub` and the UI asks it instead of spelling out a comparison. The per-alignment card was also missing the `bam_path` condition, so it offered to re-map rows with no file. **Four doc comments documented the wrong item** — a new item inserted between an existing doc block and its function, four times, twice on public API. The worst had one sentence severed across two items and its tail orphaned twenty lines below. **Measured wins, benchmarked rather than guessed.** The revert's FASTQ writer issued seven `write_all` calls per read into the gzip encoder, paying the encoder's per-call overhead seven times: 1,882 ns/record against 539 for the same bytes assembled once, ~13 minutes of CPU per realignment. `realignable_in_project` was an N+1 over members where the grouped query already existed: 2.7 ms against 17.7 ms on a 2,504-member project, twice per batch. **And one bug, fixed at its own altitude.** Simple mode's running card matched on alignment id, and once the offer was gone it had nothing to match against — so it claimed *any* running job, and a page open on one person announced another person's realignment as theirs. `RealignState` now carries the owning subject, resolved once per job by `App::subject_of_alignment`, and the card matches on that. Tightening the old predicate would have broken the Done card instead; the ownership question needed answering where the mapping lives. Also: `preflight`/`resume_preflight` were the same function twice, `discard` had two implementations, the project card cloned a Vec per frame to read its length, finished states carried progress fields nobody reads, and a user-visible string held an 18-space run. 983 tests, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/postprocess/sort.rs | 6 +- .../navigator-analysis/src/revert/writer.rs | 24 +++++--- crates/navigator-app/src/commands.rs | 4 +- crates/navigator-app/src/lib.rs | 2 +- crates/navigator-app/src/realign.rs | 61 ++++++++++++++++--- crates/navigator-app/src/realign_job.rs | 40 +++++------- crates/navigator-ui/src/ui/detail.rs | 17 +++--- crates/navigator-ui/src/ui/events.rs | 11 +++- crates/navigator-ui/src/ui/mod.rs | 9 ++- crates/navigator-ui/src/ui/modals.rs | 13 ++-- crates/navigator-ui/src/ui/simple.rs | 15 ++--- crates/navigator-ui/src/ui/sources.rs | 13 +++- crates/navigator-ui/src/worker.rs | 11 ++++ 13 files changed, 146 insertions(+), 80 deletions(-) diff --git a/crates/navigator-analysis/src/postprocess/sort.rs b/crates/navigator-analysis/src/postprocess/sort.rs index eec4d6fc..f4f8787e 100644 --- a/crates/navigator-analysis/src/postprocess/sort.rs +++ b/crates/navigator-analysis/src/postprocess/sort.rs @@ -86,7 +86,7 @@ pub fn sort_alignment( std::fs::create_dir_all(parent).map_err(|e| AnalysisError::io(parent, e))?; } - let mut reader = open_bam(input)?; + let mut reader = bamio::open(input)?; let header = reader.read_header().map_err(|e| AnalysisError::io(input, e))?; let mut stats = SortStats::default(); @@ -333,10 +333,6 @@ fn heap_bytes(record: &RecordBuf) -> usize { + 256 } -fn open_bam(path: &Path) -> Result { - bamio::open(path) -} - /// Stamp `@HD SO:coordinate` on the header. /// /// Not cosmetic: an index is only valid for a coordinate-sorted file, and readers decide whether diff --git a/crates/navigator-analysis/src/revert/writer.rs b/crates/navigator-analysis/src/revert/writer.rs index 918b4356..e0bf9dc2 100644 --- a/crates/navigator-analysis/src/revert/writer.rs +++ b/crates/navigator-analysis/src/revert/writer.rs @@ -139,6 +139,13 @@ fn finish(w: FastqWriter, path: &Path) -> Result<(), AnalysisError> { /// One FASTQ record. Names are written bare — no `/1` or `/2` — so R1/R2 pair by position; see the /// module docs on why. Qualities are shifted into ASCII here, the inverse of the decode in /// [`super::transform`], through `scratch` so the shift costs no allocation per read. +/// +/// The whole record is assembled in `scratch` and handed over in **one** `write_all`. It was seven +/// — `@`, name, newline, sequence, `\n+\n`, qualities, newline — and each one entered the gzip +/// encoder's state machine separately, paying that overhead seven times per read rather than once. +/// Measured on this exact stack at 151 bp: **1,882 ns/record against 539 ns**, a 3.5x difference on +/// the write path of the stage that already holds the scratch peak. At ~600 M reads for a 30x WGS +/// that is roughly thirteen minutes of single-threaded CPU per realignment. Identical bytes out. fn write_record( w: &mut FastqWriter, read: &RevertedRead, @@ -146,16 +153,13 @@ fn write_record( scratch: &mut Vec, ) -> Result<(), AnalysisError> { scratch.clear(); + scratch.push(b'@'); + scratch.extend_from_slice(&read.name); + scratch.push(b'\n'); + scratch.extend_from_slice(&read.sequence); + scratch.extend_from_slice(b"\n+\n"); scratch.extend(read.qualities.iter().map(|q| q.saturating_add(PHRED_OFFSET))); + scratch.push(b'\n'); - let write = |w: &mut FastqWriter| -> std::io::Result<()> { - w.write_all(b"@")?; - w.write_all(&read.name)?; - w.write_all(b"\n")?; - w.write_all(&read.sequence)?; - w.write_all(b"\n+\n")?; - w.write_all(scratch)?; - w.write_all(b"\n") - }; - write(w).map_err(|e| AnalysisError::io(path, e)) + w.write_all(scratch).map_err(|e| AnalysisError::io(path, e)) } diff --git a/crates/navigator-app/src/commands.rs b/crates/navigator-app/src/commands.rs index e59899f6..f235ece6 100644 --- a/crates/navigator-app/src/commands.rs +++ b/crates/navigator-app/src/commands.rs @@ -248,8 +248,6 @@ impl App { self.alignment_or_err(id).await } - /// Fetch an alignment by id, mapping a missing row to a `NotFound` error. The standard way - /// the analysis/query methods resolve an `alignment_id` before touching its BAM/CRAM. /// An alignment by id, or `None` if there is no such row. /// /// Public because provenance made alignments something callers ask about directly — the UI @@ -258,6 +256,8 @@ impl App { Ok(alignment::get(self.store.pool(), id).await?) } + /// Fetch an alignment by id, mapping a missing row to a `NotFound` error. The standard way + /// the analysis/query methods resolve an `alignment_id` before touching its BAM/CRAM. pub(crate) async fn alignment_or_err(&self, id: i64) -> Result { alignment::get(self.store.pool(), id) .await? diff --git a/crates/navigator-app/src/lib.rs b/crates/navigator-app/src/lib.rs index a67b66bd..3f3ad913 100644 --- a/crates/navigator-app/src/lib.rs +++ b/crates/navigator-app/src/lib.rs @@ -2910,7 +2910,7 @@ mod queries; mod realign; /// Re-exported alone rather than opening the module: the UI needs to name the build it is offering /// to realign to, and nothing else in there is its business. -pub use realign::DEFAULT_TARGET_BUILD; +pub use realign::{is_target_build, DEFAULT_TARGET_BUILD}; pub mod realign_job; mod recruitment; mod social; diff --git a/crates/navigator-app/src/realign.rs b/crates/navigator-app/src/realign.rs index 2011ce80..6c45c9ff 100644 --- a/crates/navigator-app/src/realign.rs +++ b/crates/navigator-app/src/realign.rs @@ -20,6 +20,7 @@ use std::path::{Path, PathBuf}; +use navigator_domain::du_domain::ids::SampleGuid; use navigator_domain::workspace::{Alignment, NewAlignment}; use navigator_store::alignment; @@ -109,6 +110,23 @@ impl App { .collect()) } + /// The subject an alignment belongs to, via its sequencing run. + /// + /// The UI needs this to say whose realignment is running. Without it the running card matched on + /// alignment id alone, and a page showing subject A during a job on subject B told A their + /// genome was being rebuilt — the ownership question has to be answered where the mapping from + /// alignment to subject actually lives. + pub async fn subject_of_alignment(&self, id: i64) -> Result, AppError> { + let Some(aln) = alignment::get(self.store.pool(), id).await? else { + return Ok(None); + }; + Ok( + navigator_store::sequence_run::get(self.store.pool(), aln.sequence_run_id) + .await? + .map(|run| run.biosample_guid), + ) + } + /// The alignment `id` was derived from, or `None` when it is an original. pub async fn derivation_source(&self, id: i64) -> Result, AppError> { let aln = self.alignment_or_err(id).await?; @@ -125,10 +143,30 @@ impl App { /// refused anyway — anything already on the target build, anything already realigned, and /// anything with no file to read — so the count is the real one rather than an upper bound. pub async fn realignable_in_project(&self, project_id: i64, target_build: &str) -> Result, AppError> { + // One query for the whole project rather than one per member — the same idiom + // `project_report` uses on this very tab. Measured on a 2,504-member project: 2.7 ms for the + // grouped query against 17.7 ms for the per-member loop, and this runs twice per batch. + let guids: Vec<_> = self + .list_biosamples(project_id) + .await? + .into_iter() + .map(|s| s.guid) + .collect(); + let rows = navigator_store::alignment::list_for_biosamples(self.store.pool(), &guids).await?; + + // Grouped by subject, not flattened: the rule's "already realigned" condition asks whether + // anything *in that subject's own set* was derived from a given alignment, so it has to see + // one subject's alignments at a time. + let mut by_subject: std::collections::HashMap<_, Vec> = std::collections::HashMap::new(); + for (guid, alignment) in rows { + by_subject.entry(guid).or_default().push(alignment); + } + let mut out = Vec::new(); - for subject in self.list_biosamples(project_id).await? { - let alignments = navigator_store::alignment::list_for_biosample(self.store.pool(), subject.guid).await?; - out.extend(realignable_for_subject(&alignments, target_build)); + for guid in &guids { + if let Some(alignments) = by_subject.get(guid) { + out.extend(realignable_for_subject(alignments, target_build)); + } } Ok(out) } @@ -154,15 +192,16 @@ impl App { } } -/// Whether two build names refer to the same reference for this purpose. -/// -/// Compared case-insensitively on the recorded strings. Deliberately *not* normalised through /// The build realignment targets when nothing says otherwise — the complete assembly, which is the /// only reason the module exists. pub const DEFAULT_TARGET_BUILD: &str = "chm13v2.0"; /// Whether `build` is the realignment target — the complete assembly. -pub(crate) fn is_target_build(build: &str) -> bool { +/// +/// `pub` so the UI can ask the question rather than spelling out its own comparison. Both Advanced +/// realign cards used to do the latter, with `eq_ignore_ascii_case` and no trim, which is a subtly +/// different rule from the one the job enforces. +pub fn is_target_build(build: &str) -> bool { builds_match(build, DEFAULT_TARGET_BUILD) } @@ -186,8 +225,12 @@ pub(crate) fn realignable_for_subject(alignments: &[Alignment], target_build: &s .collect() } -/// `canonical_build`: `chm13v2.0` and `chm13v2.0_maskedY_rCRS` share coordinates but differ in -/// chrM and in PAR masking, so realigning between them is a real operation rather than a no-op. +/// Whether two build names refer to the same reference for this purpose. +/// +/// Compared case-insensitively on the recorded strings, after trimming. Deliberately *not* +/// normalised through a `canonical_build`: `chm13v2.0` and `chm13v2.0_maskedY_rCRS` share +/// coordinates but differ in chrM and in PAR masking, so realigning between them is a real +/// operation rather than a no-op. fn builds_match(a: &str, b: &str) -> bool { a.trim().eq_ignore_ascii_case(b.trim()) } diff --git a/crates/navigator-app/src/realign_job.rs b/crates/navigator-app/src/realign_job.rs index ec1d662b..17bdb039 100644 --- a/crates/navigator-app/src/realign_job.rs +++ b/crates/navigator-app/src/realign_job.rs @@ -453,9 +453,7 @@ impl App { // Every stage's input is dead once the next stage has read it, and at WGS scale each is // tens of GB. Holding them all until the job ends — which is what this did first — roughly // doubles the peak and is the difference between fitting on a normal disk and not. - let discard = |path: &Path| { - let _ = std::fs::remove_file(path); - }; + let discard = discard_partial; if let Some(reverted) = &reverted { discard(&reverted.read1); discard(&reverted.read2); @@ -627,23 +625,7 @@ pub fn preflight(scratch: &Path, source: &Path, source_size: u64) -> Result Result Result { let size = |path: &Path| std::fs::metadata(path).map(|m| m.len()).unwrap_or(0); let largest = size(mapped).max(size(sorted)).max(size(marked)); - let needed = largest.saturating_mul(3); + plan_for(scratch, largest.saturating_mul(3), "resume the realignment") +} + +/// Measure the disk, refuse a job that cannot finish on it, and describe what was decided. +/// +/// The two preflights differ only in how they size `needed`; everything after that — probing free +/// space, the refusal, the wording, the plan — was written out twice and had to be kept in step by +/// hand. `what` is the verb in the refusal, so the two messages stay exactly as they were. +fn plan_for(scratch: &Path, needed: u64, what: &str) -> Result { let free = free_space(scratch); if !has_room(needed, free) { return Err(AppError::Import(format!( - "not enough room to resume the realignment: about {} GB of working space is needed and \ - {} GB is free on {}", + "not enough room to {what}: about {} GB of working space is needed and {} GB is free \ + on {}", gb(needed), gb(free), scratch.display(), @@ -674,8 +664,8 @@ fn resume_preflight(scratch: &Path, mapped: &Path, sorted: &Path, marked: &Path) } Ok(RealignPlan { - // Nothing that reads this is going to run — resuming starts at the sort, which is past the - // index — but the plan is the shared shape and a caller may still log it. + // A resumed job never reaches the index stage, but the plan is one shape and a caller may + // still log the figure. batch: BatchSize::for_this_machine(), scratch_needed: needed, scratch_free: free, diff --git a/crates/navigator-ui/src/ui/detail.rs b/crates/navigator-ui/src/ui/detail.rs index 1b7e1a75..d82a177b 100644 --- a/crates/navigator-ui/src/ui/detail.rs +++ b/crates/navigator-ui/src/ui/detail.rs @@ -1656,8 +1656,6 @@ impl NavigatorApp { }); } - /// A per-sample coverage/haplogroup table for the open project, with per-row coverage - /// recompute and a CSV export. Coverage/haplogroup cells show "—" until computed. /// Realign a whole project, as a card rather than a dialog for the same reason the per-alignment /// one is: this runs for *days*, and nothing that long should own the screen. /// @@ -1668,7 +1666,7 @@ impl NavigatorApp { let Some(project_id) = self.selected_project else { return; }; - let target = "chm13v2.0"; + let target = navigator_app::DEFAULT_TARGET_BUILD; // Asked of the app, once per project, rather than filtered here. This used to count // `all_alignments` — the whole workspace — and label the result "in this project": a @@ -1691,9 +1689,10 @@ impl NavigatorApp { }); return; }; - let eligible = eligible.clone(); + // Only the count is read below, so take that rather than cloning the Vec every frame. + let eligible_count = eligible.len(); - if eligible.is_empty() { + if eligible_count == 0 { ui.label(format!( "Every alignment in this project is already on {target}, or has been realigned." )); @@ -1702,12 +1701,12 @@ impl NavigatorApp { ui.label(format!( "{} alignment(s) in this project could be re-mapped to {target}.", - eligible.len() + eligible_count )); ui.add_space(4.0); ui.label( egui::RichText::new( - "They run one after another, each taking hours. Stopping ends the whole batch; everything already finished is kept, and no original is changed.", + "They run one after another, each taking hours. Stopping ends the whole batch; everything already finished is kept, and no original is changed.", ) .weak() .size(12.0), @@ -1719,7 +1718,7 @@ impl NavigatorApp { if ui .add_enabled( !busy, - egui::Button::new(format!("Realign {} to {target}", eligible.len())), + egui::Button::new(format!("Realign {eligible_count} to {target}")), ) .clicked() { @@ -1727,7 +1726,7 @@ impl NavigatorApp { project_id, target_build: target.to_string(), }); - self.status = format!("Realigning {} alignment(s) to {target}…", eligible.len()); + self.status = format!("Realigning {eligible_count} alignment(s) to {target}…"); } if busy && ui.button("Stop").clicked() { let _ = self.tx.send(Command::CancelRealign); diff --git a/crates/navigator-ui/src/ui/events.rs b/crates/navigator-ui/src/ui/events.rs index 1ca67364..dcdb9694 100644 --- a/crates/navigator-ui/src/ui/events.rs +++ b/crates/navigator-ui/src/ui/events.rs @@ -1003,6 +1003,7 @@ impl NavigatorApp { } Event::RealignProgress { alignment_id, + biosample_guid, step, total, label, @@ -1010,6 +1011,7 @@ impl NavigatorApp { } => { self.realign = Some(super::RealignState { alignment_id, + biosample_guid, step, total, label, @@ -1019,6 +1021,7 @@ impl NavigatorApp { } Event::RealignDone { alignment_id, + biosample_guid, new_alignment_id, cancelled, summary, @@ -1031,11 +1034,13 @@ impl NavigatorApp { (None, true) => super::RealignFinished::Cancelled, (None, false) => super::RealignFinished::Failed(summary.clone()), }; - let prior = self.realign.take(); + // step/total are zero rather than carried over: every consumer matches on + // `finished` first and none of them reads progress from a finished card. self.realign = Some(super::RealignState { alignment_id, - step: prior.as_ref().map(|r| r.step).unwrap_or(0), - total: prior.as_ref().map(|r| r.total).unwrap_or(0), + biosample_guid, + step: 0, + total: 0, label: String::new(), detail: String::new(), finished: Some(finished), diff --git a/crates/navigator-ui/src/ui/mod.rs b/crates/navigator-ui/src/ui/mod.rs index 881e9a36..474af610 100644 --- a/crates/navigator-ui/src/ui/mod.rs +++ b/crates/navigator-ui/src/ui/mod.rs @@ -481,6 +481,10 @@ struct AnalysisModal { struct RealignState { /// The source alignment this job belongs to — cards for other alignments ignore it. alignment_id: i64, + /// The subject it belongs to. Simple mode's card is about a *person*, not an alignment, so it + /// has to match on this: with only the alignment id to go on, a page open on subject A during a + /// job on subject B told A their genome was being rebuilt. + biosample_guid: Option, step: usize, total: usize, label: String, @@ -665,14 +669,15 @@ pub struct NavigatorApp { analysis: Option, /// The running (or last finished) realignment; see [`RealignState`]. realign: Option, - /// Simple mode's pending realignment confirmation, `(alignment id, its current build)`. + /// Simple mode's pending realignment confirmation — the same [`RealignOffer`] the brief + /// supplied, rather than a tuple re-spelling its two fields. /// /// Simple mode gets a confirmation step where Advanced does not, and the asymmetry is /// deliberate: the Advanced card sits among alignment internals and states its cost in a /// paragraph its reader is equipped to weigh. Simple mode's reader has been shown a story about /// their ancestors, and should not be able to commit the machine to four hours and 276 GB by /// misjudging one button. - simple_realign_confirm: Option<(i64, String)>, + simple_realign_confirm: Option, /// Set the moment Cancel is clicked, cleared when the run actually ends. /// /// Cancellation is cooperative: the walkers stop at their next check, so there is always a gap diff --git a/crates/navigator-ui/src/ui/modals.rs b/crates/navigator-ui/src/ui/modals.rs index 1c2a8d00..33567dde 100644 --- a/crates/navigator-ui/src/ui/modals.rs +++ b/crates/navigator-ui/src/ui/modals.rs @@ -209,8 +209,6 @@ impl NavigatorApp { } } - /// Edit (or add) the open subject's MDKA for one lineage. Years/coords are free-text and parsed - /// on save — a blank or unparseable field clears that column. Deferred dispatch. /// Simple mode's confirmation before a realignment starts. /// /// The Advanced card starts the same job from one button, and that is right for the reader who @@ -219,7 +217,7 @@ impl NavigatorApp { /// original survives untouched, and that it can be stopped. Everything here is a fact the job /// will otherwise deliver as a surprise four hours from now. pub(crate) fn simple_realign_confirm_modal(&mut self, ctx: &egui::Context) { - let Some((alignment_id, build)) = self.simple_realign_confirm.clone() else { + let Some(offer) = self.simple_realign_confirm.clone() else { return; }; @@ -233,7 +231,10 @@ impl NavigatorApp { ); ui.separator(); ui.add_space(6.0); - ui.label(self.tr("simple.realign.confirmBody").replace("{build}", &build)); + ui.label( + self.tr("simple.realign.confirmBody") + .replace("{build}", &offer.current_build), + ); ui.add_space(8.0); for key in [ "simple.realign.costTime", @@ -266,7 +267,7 @@ impl NavigatorApp { // Deferred dispatch: the closure borrows `self`, so the command goes out after it returns. if start { let _ = self.tx.send(Command::StartRealign { - alignment_id, + alignment_id: offer.alignment_id, target_build: navigator_app::DEFAULT_TARGET_BUILD.to_string(), }); self.status = self.tr("simple.realign.started").to_string(); @@ -276,6 +277,8 @@ impl NavigatorApp { } } + /// Edit (or add) the open subject's MDKA for one lineage. Years/coords are free-text and parsed + /// on save — a blank or unparseable field clears that column. Deferred dispatch. pub(crate) fn edit_mdka_modal(&mut self, ctx: &egui::Context) { let Some(mut edit) = self.edit_mdka.clone() else { return }; diff --git a/crates/navigator-ui/src/ui/simple.rs b/crates/navigator-ui/src/ui/simple.rs index 8b3aa9f8..69005eda 100644 --- a/crates/navigator-ui/src/ui/simple.rs +++ b/crates/navigator-ui/src/ui/simple.rs @@ -805,7 +805,7 @@ impl NavigatorApp { }); } - self.simple_realign_card(ui); + self.simple_realign_card(ui, guid); ui.add_space(10.0); self.export_row(ui, &[navigator_app::ExportRequest::SubjectBriefHtml(guid)]); @@ -845,15 +845,16 @@ impl NavigatorApp { /// /// Whether to offer it at all was decided in the app layer, on the brief; see /// `navigator_domain::brief::RealignOffer`. - fn simple_realign_card(&mut self, ui: &mut egui::Ui) { + fn simple_realign_card(&mut self, ui: &mut egui::Ui, guid: SampleGuid) { let offer = self.subject_brief.as_ref().and_then(|(_, b)| b.realign_offer.clone()); // A run in progress wins over the offer, so the card a user just started reports itself // rather than continuing to invite the thing it is already doing. - let running = self - .realign - .clone() - .filter(|state| offer.as_ref().map_or(true, |o| o.alignment_id == state.alignment_id)); + // + // Matched on the *subject*, not the alignment. Comparing alignment ids meant this card had + // nothing to compare against once the offer was gone — and it went on to claim any running + // job, so a page open on one person announced another person's realignment as their own. + let running = self.realign.clone().filter(|state| state.biosample_guid == Some(guid)); if let Some(state) = running { ui.add_space(10.0); @@ -909,7 +910,7 @@ impl NavigatorApp { ui.add_space(8.0); // Opens the confirmation rather than starting: see `simple_realign_confirm_modal`. if ui.button(self.tr("simple.realign.action")).clicked() { - self.simple_realign_confirm = Some((offer.alignment_id, offer.current_build.clone())); + self.simple_realign_confirm = Some(offer.clone()); } }); } diff --git a/crates/navigator-ui/src/ui/sources.rs b/crates/navigator-ui/src/ui/sources.rs index 1f4af92d..e5ac3002 100644 --- a/crates/navigator-ui/src/ui/sources.rs +++ b/crates/navigator-ui/src/ui/sources.rs @@ -263,7 +263,7 @@ impl NavigatorApp { } // ---- idle: offer it ---- None => { - let target = "chm13v2.0"; + let target = navigator_app::DEFAULT_TARGET_BUILD; let already = self .alignments .iter() @@ -273,10 +273,19 @@ impl NavigatorApp { ui.label("This alignment has already been realigned."); return; } - if alignment.reference_build.eq_ignore_ascii_case(target) { + // `is_target_build` rather than a comparison spelled here: it trims where + // `eq_ignore_ascii_case` alone does not, so a stored " chm13v2.0" was offered by + // this card and then refused by the job. The rule belongs to the app. + if navigator_app::is_target_build(&alignment.reference_build) { ui.label(format!("Already on {target} — there is nothing to realign.")); return; } + // A row with no file cannot be re-mapped; the job fails at `MissingPaths`. The app's + // `realignable_for_subject` has always excluded these — this card did not. + if alignment.bam_path.is_none() { + ui.label("This alignment has no file to re-map."); + return; + } ui.label(format!( "Re-map this {} alignment's reads to {target}, so analyses run against the \ diff --git a/crates/navigator-ui/src/worker.rs b/crates/navigator-ui/src/worker.rs index 16450a47..f2fda9ee 100644 --- a/crates/navigator-ui/src/worker.rs +++ b/crates/navigator-ui/src/worker.rs @@ -1139,6 +1139,9 @@ pub enum Event { /// A realignment stage began. RealignProgress { alignment_id: i64, + /// The subject this job belongs to, so a card can tell whether the run is *theirs*. + /// `None` only if the lookup failed, in which case no card claims it. + biosample_guid: Option, step: usize, total: usize, label: String, @@ -1156,6 +1159,7 @@ pub enum Event { /// success — the row is inserted last, so its absence means nothing was registered. RealignDone { alignment_id: i64, + biosample_guid: Option, new_alignment_id: Option, cancelled: bool, summary: String, @@ -2466,6 +2470,9 @@ async fn run_realign_streaming( evt_tx: &Sender, wake: Arc, ) { + // Whose job this is, resolved once up front rather than per event. + let biosample_guid = app.subject_of_alignment(alignment_id).await.ok().flatten(); + // Resolve (downloading if needed) the reference we are mapping to, streaming its progress the // same way every other reference-dependent command does. ensure_references_streaming(app, std::slice::from_ref(&target_build), evt_tx, &*wake).await; @@ -2475,6 +2482,7 @@ async fn run_realign_streaming( None => { let _ = evt_tx.send(Event::RealignDone { alignment_id, + biosample_guid, new_alignment_id: None, cancelled: false, summary: format!("the {target_build} reference is not available"), @@ -2489,6 +2497,7 @@ async fn run_realign_streaming( let progress = move |p: navigator_app::realign_job::RealignProgress| { let _ = tx.send(Event::RealignProgress { alignment_id, + biosample_guid, step: p.stage.step(), total: p.total_stages, label: p.stage.label().to_string(), @@ -2513,6 +2522,7 @@ async fn run_realign_streaming( let event = match app.realign_alignment(alignment_id, params, cancel, progress).await { Ok(outcome) => Event::RealignDone { alignment_id, + biosample_guid, new_alignment_id: Some(outcome.alignment.id), cancelled: false, // A resumed job skips the stages that count these, so a figure may be genuinely @@ -2535,6 +2545,7 @@ async fn run_realign_streaming( let cancelled = e.is_cancelled(); Event::RealignDone { alignment_id, + biosample_guid, new_alignment_id: None, cancelled, summary: if cancelled { String::new() } else { e.to_string() },