fix: two medium bugs from the final adversarial review
- Retry was broken for the common case: retry_selected_download removed the failed download only from the UI list, not the client's DownloadStore. Since Client::download re-derives the same md5 token from the unchanged filename, retrying a still-Failed download left a duplicate-token entry; the token-migration path then deleted both, misrouting completion so the retried file downloaded to disk but the UI showed 'Queued' forever. Add Client::remove_download / DownloadStore::remove_by_file (status-agnostic) and clear the stale entry before re-issuing. Regression-tested. - Chat-room member selection could point out of range: RoomEvent::UserLeft shrank the active room's member list without clamping user_selected, so the highlight silently shifted to the wrong user (or vanished, making 'b'/'m' no-ops, and blanking the member pane). Clamp user_selected after every applied room event. Regression-tested.
This commit is contained in:
@@ -1101,6 +1101,23 @@ impl Client {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Remove every download for `username`/`filename` regardless of status.
|
||||||
|
/// Call this before re-issuing [`Client::download`] for a failed download,
|
||||||
|
/// otherwise the stale entry (whose md5-derived token equals the retry's)
|
||||||
|
/// shadows the fresh one and its completion is misrouted.
|
||||||
|
///
|
||||||
|
/// Returns whether anything was removed.
|
||||||
|
#[must_use]
|
||||||
|
pub fn remove_download(&self, username: &str, filename: &str) -> bool {
|
||||||
|
match self.context.write_safe() {
|
||||||
|
Ok(mut ctx) => ctx.downloads.remove_by_file(username, filename),
|
||||||
|
Err(e) => {
|
||||||
|
error!("[client] remove_download: {}", e);
|
||||||
|
false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
pub fn download(
|
pub fn download(
|
||||||
&self,
|
&self,
|
||||||
filename: String,
|
filename: String,
|
||||||
|
|||||||
@@ -84,6 +84,17 @@ impl DownloadStore {
|
|||||||
true
|
true
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Remove every download matching `username`/`filename` regardless of
|
||||||
|
/// status. Used before retrying a failed download so the stale entry (whose
|
||||||
|
/// md5-derived token collides with the retry's) can't shadow the fresh one.
|
||||||
|
/// Returns whether anything was removed.
|
||||||
|
pub fn remove_by_file(&mut self, username: &str, filename: &str) -> bool {
|
||||||
|
let before = self.downloads.len();
|
||||||
|
self.downloads
|
||||||
|
.retain(|d| !(d.username == username && d.filename == filename));
|
||||||
|
self.downloads.len() != before
|
||||||
|
}
|
||||||
|
|
||||||
pub fn pause_by_file(&mut self, username: &str, filename: &str) -> bool {
|
pub fn pause_by_file(&mut self, username: &str, filename: &str) -> bool {
|
||||||
let Some(download) = self.get_by_file_mut(username, filename) else {
|
let Some(download) = self.get_by_file_mut(username, filename) else {
|
||||||
return false;
|
return false;
|
||||||
@@ -272,6 +283,28 @@ mod tests {
|
|||||||
assert!(store.get_by_token(456).is_some());
|
assert!(store.get_by_token(456).is_some());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn remove_by_file_removes_regardless_of_status() {
|
||||||
|
let mut store = DownloadStore::new();
|
||||||
|
// A failed download (the retry case) plus a same-name duplicate that a
|
||||||
|
// token-migration could have left behind — both must go.
|
||||||
|
let mut failed = make_download(1, DownloadStatus::Failed(None));
|
||||||
|
failed.filename = "song.mp3".to_string();
|
||||||
|
store.add(failed);
|
||||||
|
let mut dup = make_download(2, DownloadStatus::Queued);
|
||||||
|
dup.filename = "song.mp3".to_string();
|
||||||
|
store.add(dup);
|
||||||
|
let mut other = make_download(3, DownloadStatus::Queued);
|
||||||
|
other.filename = "other.mp3".to_string();
|
||||||
|
store.add(other);
|
||||||
|
|
||||||
|
assert!(store.remove_by_file("peer", "song.mp3"));
|
||||||
|
assert!(store.get_by_token(1).is_none());
|
||||||
|
assert!(store.get_by_token(2).is_none());
|
||||||
|
assert!(store.get_by_token(3).is_some(), "other file untouched");
|
||||||
|
assert!(!store.remove_by_file("peer", "song.mp3"), "idempotent");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn collect_failed_tokens_notifies_and_lists_matching() {
|
fn collect_failed_tokens_notifies_and_lists_matching() {
|
||||||
let mut store = DownloadStore::new();
|
let mut store = DownloadStore::new();
|
||||||
|
|||||||
@@ -284,6 +284,16 @@ impl RoomsState {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// The active room's member list may have grown/shrunk (join/leave or a
|
||||||
|
// wholesale replace on Joined); keep the selection highlight in range so
|
||||||
|
// 'b'/'m' never act on the wrong member or a phantom one.
|
||||||
|
self.clamp_user_selected();
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Keep `user_selected` within the active room's member list.
|
||||||
|
fn clamp_user_selected(&mut self) {
|
||||||
|
let len = self.active_room().map_or(0, |r| r.users.len());
|
||||||
|
self.user_selected = self.user_selected.min(len.saturating_sub(1));
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Index of the open room `name`, creating an empty tab if necessary.
|
/// Index of the open room `name`, creating an empty tab if necessary.
|
||||||
@@ -526,6 +536,54 @@ mod tests {
|
|||||||
assert_eq!(state.selected_user(), None); // blues has no members yet
|
assert_eq!(state.selected_user(), None); // blues has no members yet
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn user_selection_stays_in_range_when_members_leave() {
|
||||||
|
let mut state = RoomsState::new();
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::Joined {
|
||||||
|
room: "jazz".to_string(),
|
||||||
|
users: vec![
|
||||||
|
"alice".to_string(),
|
||||||
|
"bob".to_string(),
|
||||||
|
"carol".to_string(),
|
||||||
|
],
|
||||||
|
},
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
state.focus_or_open("jazz");
|
||||||
|
state.user_selected = 2; // carol highlighted (last)
|
||||||
|
|
||||||
|
// carol leaves: selection must clamp to the new last index, not point
|
||||||
|
// past the end (which would make 'b'/'m' silent no-ops).
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::UserLeft {
|
||||||
|
room: "jazz".to_string(),
|
||||||
|
username: "carol".to_string(),
|
||||||
|
},
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
assert_eq!(state.user_selected, 1);
|
||||||
|
assert_eq!(state.selected_user().as_deref(), Some("bob"));
|
||||||
|
|
||||||
|
// Everyone leaves: selection collapses to 0 and yields no user.
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::UserLeft {
|
||||||
|
room: "jazz".to_string(),
|
||||||
|
username: "bob".to_string(),
|
||||||
|
},
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::UserLeft {
|
||||||
|
room: "jazz".to_string(),
|
||||||
|
username: "alice".to_string(),
|
||||||
|
},
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
assert_eq!(state.user_selected, 0);
|
||||||
|
assert_eq!(state.selected_user(), None);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn user_joined_and_left_update_membership() {
|
fn user_joined_and_left_update_membership() {
|
||||||
let mut state = RoomsState::new();
|
let mut state = RoomsState::new();
|
||||||
|
|||||||
@@ -1324,9 +1324,13 @@ impl MainTui {
|
|||||||
let client = self.client.clone();
|
let client = self.client.clone();
|
||||||
let sender = self.state.downloads_sender_channel.clone().unwrap();
|
let sender = self.state.downloads_sender_channel.clone().unwrap();
|
||||||
|
|
||||||
// Drop the old failed entry; the retry pushes a fresh one.
|
// Drop the old failed entry from both the UI list and the client's
|
||||||
|
// download store; otherwise the stale entry (same md5-derived token and
|
||||||
|
// filename) shadows the retry and its completion is misrouted, leaving
|
||||||
|
// the retried download stuck showing "Queued".
|
||||||
self.state.downloads.remove(index);
|
self.state.downloads.remove(index);
|
||||||
self.select_download_after_removal(index);
|
self.select_download_after_removal(index);
|
||||||
|
let _ = self.client.remove_download(&username, &filename);
|
||||||
|
|
||||||
thread::spawn(move || {
|
thread::spawn(move || {
|
||||||
match client.download(filename.clone(), username, size, directory) {
|
match client.download(filename.clone(), username, size, directory) {
|
||||||
|
|||||||
Reference in New Issue
Block a user