Skip to content

fix: correct paths being passed to fix streaming bug #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 11, 2025
Merged

fix: correct paths being passed to fix streaming bug #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32e0fdb
fix: correct paths being passed
maschad Jun 10, 2025
212b89e
fix: fixed issue with streaming paths
maschad Jun 11, 2025
31a6d13
chore: resolve clippy issues
maschad Jun 11, 2025
b9c0d0f
chore: added more tests
maschad Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@
**/*.rs.bk
.env
logs/
test_share/
.DS_Store
128 changes: 97 additions & 31 deletions src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ pub struct App {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct DirectoryItem {
pub name: String,
pub path: PathBuf,
pub path: PathBuf, // This will be the absolute path for file operations
pub display_path: PathBuf, // This will be used for UI display
pub is_dir: bool,
pub index: usize,
pub depth: usize,
Expand Down Expand Up @@ -100,12 +101,33 @@ impl App {
} else {
self.current_path.clone()
};
let mut children: Vec<DirectoryItem> = self
.all_shared_items
.iter()
.filter(|item| item.path.parent().unwrap_or(&PathBuf::new()) == current)
.cloned()
.collect();
tracing::info!("Current path in download mode: {:?}", current);
tracing::info!("All shared items: {:?}", self.all_shared_items);

// In download mode, we want to show all items at the root level
let mut children: Vec<DirectoryItem> = if current.as_os_str().is_empty() {
// At root level, show all items
self.all_shared_items.clone()
} else {
// For subdirectories, filter by parent
self.all_shared_items
.iter()
.filter(|item| {
let binding = &PathBuf::new();
let parent = item.path.parent().unwrap_or(binding);
tracing::info!(
"Comparing paths - parent: {:?}, current: {:?}",
parent,
current
);
parent == current
})
.cloned()
.collect()
};

tracing::info!("Found {} items in current directory", children.len());

children.sort_by(|a, b| match (a.is_dir, b.is_dir) {
(true, false) => std::cmp::Ordering::Less,
(false, true) => std::cmp::Ordering::Greater,
Expand Down Expand Up @@ -153,13 +175,13 @@ impl App {
let abs_selected = self.current_path.join(selected_path);
path == abs_selected
}),
AppState::Download => self.directory_items.iter().any(|item| {
item.path
.strip_prefix(&self.current_path)
.is_ok_and(|rel_path| {
let abs_selected = self.current_path.join(rel_path);
path == abs_selected
})
AppState::Download => self.items_to_download.iter().any(|selected_path| {
tracing::info!(
"Comparing download paths - selected: {:?}, current: {:?}",
selected_path,
path
);
selected_path == &path
}),
};

Expand All @@ -168,6 +190,16 @@ impl App {
.map(|rel_path| rel_path.components().count())
.unwrap_or(0);

let display_path = if self.state == AppState::Download {
// In download mode, use just the name for display
PathBuf::from(&name)
} else {
// In share mode, use the relative path for display
path.strip_prefix(&self.current_path)
.unwrap_or(&path)
.to_path_buf()
};

let preview = if is_dir {
format!("Directory: {name}")
} else {
Expand All @@ -176,18 +208,15 @@ impl App {
|file| {
let reader = BufReader::new(file);
let mut buffer = String::new();
// Read up to 1000 UTF-8 characters (not bytes)
reader
.take(4000) // Read up to 4000 bytes, adjust as needed for long UTF-8 chars
.read_to_string(&mut buffer)
.ok();
reader.take(4000).read_to_string(&mut buffer).ok();
buffer.chars().take(1000).collect()
},
)
};
DirectoryItem {
name,
path,
path, // Keep the absolute path
display_path, // Use the display path for UI
is_dir,
index,
depth,
Expand Down Expand Up @@ -340,6 +369,7 @@ impl App {
pub fn select_item(&mut self) {
if let Some(index) = self.selected_index {
if let Some(item) = self.directory_items.get_mut(index) {
tracing::info!("Attempting to select item: {:?}", item);
match self.state {
AppState::Share | AppState::Download => {
let items_set = match self.state {
Expand All @@ -351,6 +381,7 @@ impl App {
// Add the directory itself with its relative path
if let Ok(rel_path) = item.path.strip_prefix(&self.current_path) {
let path_buf = rel_path.to_path_buf();
tracing::info!("Adding directory to selection: {:?}", path_buf);
items_set.insert(path_buf);

// Add all files and subdirectories with their relative paths
Expand All @@ -361,6 +392,10 @@ impl App {
if let Ok(entry_rel_path) =
entry.path().strip_prefix(&self.current_path)
{
tracing::info!(
"Adding sub-item to selection: {:?}",
entry_rel_path
);
items_set.insert(entry_rel_path.to_path_buf());
}
}
Expand All @@ -371,19 +406,42 @@ impl App {
// For share mode, store relative to current directory
if let Ok(rel_path) = item.path.strip_prefix(&self.current_path)
{
tracing::info!(
"Adding file to share selection: {:?}",
rel_path
);
items_set.insert(rel_path.to_path_buf());
} else {
tracing::info!(
"Adding file to share selection with name: {:?}",
item.name
);
items_set.insert(PathBuf::from(&item.name));
}
}
AppState::Download => {
// For download mode, use the path directly since it's already relative
tracing::info!("inserting item is: {:?}", item);
// For download mode, store the absolute path from the sharing node
tracing::info!("Download mode - item to insert: {:?}", item);
tracing::info!("Current path: {:?}", self.current_path);

// Ensure we have a valid path
if item.path.as_os_str().is_empty() {
tracing::warn!("Empty path for item: {:?}", item);
return;
}

// Use the absolute path for downloads
tracing::info!(
"Adding file to download selection with absolute path: {:?}",
item.path
);
items_set.insert(item.path.clone());
}
}
}
item.selected = true;
tracing::info!("Item selected. Current selection: {:?}", items_set);

// Update the cached version
if let Some(cached_items) = self.directory_cache.get_mut(&self.current_path)
{
Expand Down Expand Up @@ -412,14 +470,9 @@ impl App {
}
}
AppState::Download => {
if let Ok(rel_path) = item.path.strip_prefix(&self.current_path) {
let path_buf = rel_path.to_path_buf();
self.items_to_download.remove(&path_buf);
} else {
// If we can't strip the prefix, try removing by filename
let path_buf = PathBuf::from(&item.name);
self.items_to_download.remove(&path_buf);
}
// In download mode, we use the absolute path
tracing::info!("Unselecting item in download mode: {:?}", item);
self.items_to_download.remove(&item.path);
}
}
item.selected = false;
Expand Down Expand Up @@ -497,10 +550,23 @@ impl App {
self.items_being_downloaded
.clone_from(&self.items_to_download);

// Get the absolute paths from all_shared_items
let file_names: Vec<String> = self
.items_to_download
.iter()
.map(|path| path.to_string_lossy().to_string())
.filter_map(|path| {
// Find the original item in all_shared_items to get the absolute path
self.all_shared_items
.iter()
.find(|item| {
item.name == path.file_name().unwrap_or_default().to_string_lossy()
})
.map(|item| {
let path_str = item.path.to_string_lossy().to_string();
tracing::info!("Using absolute path for download: {}", path_str);
path_str
})
})
Comment on lines 554 to +569
Copy link

@coderabbitai coderabbitai bot Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential issue with file name matching

The current implementation matches files by name only, which could select the wrong file if multiple files have the same name in different directories.

Consider using a more robust matching strategy:

-                    .find(|item| {
-                        item.name == path.file_name().unwrap_or_default().to_string_lossy()
-                    })
+                    .find(|item| {
+                        // Match by the full path or a unique identifier
+                        &item.path == path || 
+                        (item.name == path.file_name().unwrap_or_default().to_string_lossy() 
+                         && item.path.parent() == path.parent())
+                    })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let file_names: Vec<String> = self
.items_to_download
.iter()
.map(|path| path.to_string_lossy().to_string())
.filter_map(|path| {
// Find the original item in all_shared_items to get the absolute path
self.all_shared_items
.iter()
.find(|item| {
item.name == path.file_name().unwrap_or_default().to_string_lossy()
})
.map(|item| {
let path_str = item.path.to_string_lossy().to_string();
tracing::info!("Using absolute path for download: {}", path_str);
path_str
})
})
let file_names: Vec<String> = self
.items_to_download
.iter()
.filter_map(|path| {
// Find the original item in all_shared_items to get the absolute path
self.all_shared_items
.iter()
.find(|item| {
// Match by the full path or a unique identifier
&item.path == path
|| (item.name == path.file_name().unwrap_or_default().to_string_lossy()
&& item.path.parent() == path.parent())
})
.map(|item| {
let path_str = item.path.to_string_lossy().to_string();
tracing::info!("Using absolute path for download: {}", path_str);
path_str
})
})
🤖 Prompt for AI Agents 
In src/app.rs around lines 554 to 569, the code matches files by name only when
filtering items_to_download against all_shared_items, which can cause incorrect
matches if multiple files share the same name in different directories. To fix
this, update the matching logic to compare the full relative or absolute paths
instead of just the file names, ensuring that the correct file is selected by
uniquely identifying it with its complete path.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open a follow-up GitHub issue for this discussion

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@maschad Sure thing — I’ll open a follow-up issue that captures the file-matching concern raised in the review discussion.

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue documenting the path-matching bug discussed in PR #17.
gh issue create \
  --title "Follow-up: Improve file matching when resolving download paths" \
  --body "$(cat <<'EOF'
### Context
During code review of pull request #17, a potential bug was identified in `src/app.rs` (see comment https://github.com/maschad/junkanoo/pull/17#discussion_r2140895565).  
The current implementation resolves files to download by matching **only the file name**, which can select the wrong file when multiple shared files have identical names in different directories.

### Expected
Files chosen for download should be matched unambiguously, e.g. by comparing full relative/absolute paths or another unique identifier.

### Impact
If duplicate file names exist in separate directories, users may receive the wrong file.

### Acceptance Criteria
- Matching logic uniquely identifies the intended file (full path or equivalent).
- Unit/integration tests cover scenarios with duplicate file names.

Referenced PR: https://github.com/maschad/junkanoo/pull/17
EOF
)" \
  --assignee "maschad"

Length of output: 1036

@maschad ✅ Follow-up issue created: #18 — it captures the file-matching bug noted in the review and includes context, expected behaviour, impact, and acceptance criteria. Let me know if you’d like any tweaks or extra details added.

🧬 Code Graph Analysis Results

Relevant snippets from src/service/node.rs (lines 55-126)

pub fn new() -> Result<(Client, impl Stream<Item = Event>, EventLoop, PeerId), Box<dyn Error>> {
    // Create a public/private key pair, either random or based on a seed.
    // let id_keys = Keypair::generate_ed25519();
    // let peer_id = id_keys.public().to_peer_id();
    let peer_id = PeerId::random();

    let mut swarm = SwarmBuilder::with_new_identity()
        .with_tokio()
        .with_tcp(
            tcp::Config::default(),
            noise::Config::new,
            yamux::Config::default,
        )?
        .with_quic()
        .with_behaviour(|key| Behaviour {
            kademlia: kad::Behaviour::new(
                peer_id,
                kad::store::MemoryStore::new(key.public().to_peer_id()),
            ),
            request_response: request_response::cbor::Behaviour::new(
                [(JUNKANOO_REQUEST_RESPONSE_PROTOCOL, ProtocolSupport::Full)],
                request_response::Config::default(),
            ),
            file_stream: stream::Behaviour::new(),
        })?
        .with_swarm_config(|c| {
            c.with_idle_connection_timeout(Duration::from_secs(CONNECTION_TIMEOUT))
        })
        .build();

    // Set up file transfer protocol listener
    let incoming_streams = swarm
        .behaviour_mut()
        .file_stream
        .new_control()
        .accept(JUNKANOO_FILE_PROTOCOL)
        .unwrap();
    tracing::info!(
        "Listening for file transfer streams on protocol: {}",
        JUNKANOO_FILE_PROTOCOL
    );

    // Set Kademlia into server mode before adding bootnodes
    swarm
        .behaviour_mut()
        .kademlia
        .set_mode(Some(kad::Mode::Server));

    // Then add the bootnodes
    for peer in &BOOTNODES {
        if let Ok(peer_id) = peer.parse() {
            swarm
                .behaviour_mut()
                .kademlia
                .add_address(&peer_id, "/dnsaddr/bootstrap.libp2p.io".parse()?);
        }
    }

    let (command_sender, command_receiver) = mpsc::channel(0);
    let (event_sender, event_receiver) = mpsc::channel(0);

    let local_peer_id = *swarm.local_peer_id();

    Ok((
        Client {
            sender: command_sender,
        },
        event_receiver,
        EventLoop::new(swarm, command_receiver, event_sender, incoming_streams),
        local_peer_id,
    ))
}

Relevant snippet from src/main.rs (lines 352-464)

async fn handle_download_mode(
    client: &mut Client,
    target_peer_addr: Multiaddr,
    app: Arc<Mutex<App>>,
) -> Result<(), &'static str> {
    let target_peer_id = target_peer_addr
        .iter()
        .find_map(|p| match p {
            Protocol::P2p(peer_id) => Some(peer_id),
            _ => None,
        })
        .ok_or("Peer address must contain a peer ID component (/p2p/...)")?;

    client.dial(target_peer_id, target_peer_addr).await.unwrap();
    tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;

    // Initial directory request
    match client.request_directory(target_peer_id).await {
        Ok(display_response) => {
            let mut items = display_response.items;
            // For download mode, we need to ensure both paths are properly set
            for item in &mut items {
                tracing::info!("path before setting paths is: {:?}", item.path);
                // Keep the absolute path for file operations
                let abs_path = item.path.clone();
                // Use just the name for display
                let display_path = PathBuf::from(&item.name);
                item.path = abs_path;
                item.display_path = display_path;
                tracing::info!(
                    "Set paths - absolute: {:?}, display: {:?}",
                    item.path,
                    item.display_path
                );
            }
            items.sort_by(|a, b| match (a.is_dir, b.is_dir) {
                (true, false) => std::cmp::Ordering::Less,
                (false, true) => std::cmp::Ordering::Greater,
                _ => match a.depth.cmp(&b.depth) {
                    std::cmp::Ordering::Equal => a.name.to_lowercase().cmp(&b.name.to_lowercase()),
                    other => other,
                },
            });

            {
                let mut app = app.lock();
                app.all_shared_items.clone_from(&items);
                app.directory_items = items;
                app.current_path = PathBuf::new();
                app.populate_directory_items();
                tracing::info!("Initial directory items: {:?}", app.directory_items);
            }

            // Start a background task to handle directory updates
            let mut client_clone = client.clone();
            let app_clone = app.clone();
            tokio::spawn(async move {
                loop {
                    tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;
                    match client_clone.request_directory(target_peer_id).await {
                        Ok(display_response) => {
                            let mut items = display_response.items;
                            // For download mode, we need to ensure both paths are properly set
                            for item in &mut items {
                                // Keep the absolute path for file operations
                                let abs_path = item.path.clone();
                                // Use just the name for display
                                let display_path = PathBuf::from(&item.name);
                                item.path = abs_path;
                                item.display_path = display_path;
                            }
                            items.sort_by(|a, b| match (a.is_dir, b.is_dir) {
                                (true, false) => std::cmp::Ordering::Less,
                                (false, true) => std::cmp::Ordering::Greater,
                                _ => match a.depth.cmp(&b.depth) {
                                    std::cmp::Ordering::Equal => {
                                        a.name.to_lowercase().cmp(&b.name.to_lowercase())
                                    }
                                    other => other,
                                },
                            });

                            let mut app = app_clone.lock();
                            if app.all_shared_items != items {
                                app.all_shared_items.clone_from(&items);
                                app.directory_items = items;
                                app.populate_directory_items();
                                tracing::info!(
                                    "Updated directory items: {:?}",
                                    app.directory_items
                                );
                                // Notify UI to refresh
                                if let Some(refresh_sender) = &app.refresh_sender {
                                    let _ = refresh_sender.try_send(());
                                }
                            }
                        }
                        Err(e) => {
                            tracing::error!("Failed to request directory: {}", e);
                            break;
                        }
                    }
                }
            });

            Ok(())
        }
        Err(e) => {
            tracing::error!("Failed to request directory: {}", e);
            Err("Failed to request directory")
        }
    }
}

Summary of relevant code in src/app.rs (the provided file)

  • The App struct manages application state including directory items, shared/downloaded items, connection state, and UI state.
  • It supports two modes: AppState::Share and AppState::Download.
  • The method populate_directory_items populates the directory listing based on the current path and mode.
  • The method handle_download_mode filters and sorts directory items when in download mode, showing items from all_shared_items.
  • The select_item and unselect_item methods manage selection of files/directories for sharing or downloading, storing relative paths for sharing and absolute paths for downloading.
  • The start_download async method initiates file download requests via the Client using absolute paths resolved from all_shared_items.
  • The App maintains a client: Option<Client> to communicate with peers.
  • The refresh_sender is used to notify the UI to refresh when selections or directory items change.

These snippets and summaries provide context on how the app manages file sharing and downloading, peer connections, directory listings, and UI updates, which are relevant for understanding the file and any related discussions or issues.

.collect();

tracing::info!("Starting download of files: {:?}", file_names);
Expand Down
71 changes: 62 additions & 9 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::path::PathBuf;
use std::{io::Stdout, sync::Arc};

use app::{App, ConnectionState, DirectoryItem};
Expand Down Expand Up @@ -235,8 +236,8 @@ fn render_loop(terminal: &mut Terminal<CrosstermBackend<Stdout>>, app: &Arc<Mute
// Clone the app before dropping the lock
let mut app_clone = app.clone();
tracing::debug!(
"Starting download with {} items selected",
app.items_to_download.len()
"Starting download with {:#?} items selected",
app.items_to_download
);
// Start the download in a new task
tokio::spawn(async move {
Expand Down Expand Up @@ -271,17 +272,37 @@ async fn handle_host_mode(client: &mut Client, peer_id: PeerId, app: Arc<Mutex<A
.unwrap_or(&virtual_root)
.to_path_buf();
}
tracing::info!("Virtual root path: {:?}", virtual_root);
all_paths
.iter()
.enumerate()
.map(|(index, path)| {
let rel_path = path.strip_prefix(&virtual_root).unwrap_or(path);
let name = rel_path
tracing::info!("Processing path: {:?}", path);

// Get the absolute path for file operations
let abs_path = std::fs::canonicalize(path).unwrap_or_else(|_| path.clone());
tracing::info!("Absolute path: {:?}", abs_path);

// Get the name from the path
let name = path
.file_name()
.or_else(|| path.file_name())
.unwrap_or_default()
.to_string_lossy()
.to_string();

// Get the relative path for UI display
let rel_path = path
.strip_prefix(&virtual_root)
.unwrap_or(path)
.to_path_buf();

tracing::info!(
"Name: {}, Relative path: {:?}, Absolute path: {:?}",
name,
rel_path,
abs_path
);

let is_dir = path.is_dir();
let depth = rel_path.components().count();
let preview = if is_dir {
Expand All @@ -297,15 +318,18 @@ async fn handle_host_mode(client: &mut Client, peer_id: PeerId, app: Arc<Mutex<A
},
)
};
DirectoryItem {
let item = DirectoryItem {
name,
path: rel_path.to_path_buf(),
path: abs_path, // Use the absolute path for file operations
display_path: rel_path, // Use the relative path for UI display
is_dir,
index,
depth,
selected: true,
preview,
}
};
tracing::info!("Created DirectoryItem: {:?}", item);
item
})
.collect()
}
Expand Down Expand Up @@ -345,6 +369,21 @@ async fn handle_download_mode(
match client.request_directory(target_peer_id).await {
Ok(display_response) => {
let mut items = display_response.items;
// For download mode, we need to ensure both paths are properly set
for item in &mut items {
tracing::info!("path before setting paths is: {:?}", item.path);
// Keep the absolute path for file operations
let abs_path = item.path.clone();
// Use just the name for display
let display_path = PathBuf::from(&item.name);
item.path = abs_path;
item.display_path = display_path;
tracing::info!(
"Set paths - absolute: {:?}, display: {:?}",
item.path,
item.display_path
);
}
items.sort_by(|a, b| match (a.is_dir, b.is_dir) {
(true, false) => std::cmp::Ordering::Less,
(false, true) => std::cmp::Ordering::Greater,
Expand All @@ -358,8 +397,9 @@ async fn handle_download_mode(
let mut app = app.lock();
app.all_shared_items.clone_from(&items);
app.directory_items = items;
app.current_path = std::path::PathBuf::new();
app.current_path = PathBuf::new();
app.populate_directory_items();
tracing::info!("Initial directory items: {:?}", app.directory_items);
}

// Start a background task to handle directory updates
Expand All @@ -371,6 +411,15 @@ async fn handle_download_mode(
match client_clone.request_directory(target_peer_id).await {
Ok(display_response) => {
let mut items = display_response.items;
// For download mode, we need to ensure both paths are properly set
for item in &mut items {
// Keep the absolute path for file operations
let abs_path = item.path.clone();
// Use just the name for display
let display_path = PathBuf::from(&item.name);
item.path = abs_path;
item.display_path = display_path;
}
items.sort_by(|a, b| match (a.is_dir, b.is_dir) {
(true, false) => std::cmp::Ordering::Less,
(false, true) => std::cmp::Ordering::Greater,
Expand All @@ -387,6 +436,10 @@ async fn handle_download_mode(
app.all_shared_items.clone_from(&items);
app.directory_items = items;
app.populate_directory_items();
tracing::info!(
"Updated directory items: {:?}",
app.directory_items
);
// Notify UI to refresh
if let Some(refresh_sender) = &app.refresh_sender {
let _ = refresh_sender.try_send(());
Expand Down
Loading