Conversation
kaichaosun
requested review from
jazzz,
osmaczko and
plopezlpz
and removed request for
jazzz
osmaczko
left a comment
osmaczko
left a comment
There was a problem hiding this comment.
Nice 👍
Works for me as well
| /// Get the data directory (in project folder). | ||
| fn get_data_dir() -> PathBuf { | ||
| // Use the directory where the binary is or current working directory | ||
| let manifest_dir = env!("CARGO_MANIFEST_DIR"); |
There was a problem hiding this comment.
CARGO_MANIFEST_DIR is baked in at compile time and is never empty in practice, making the "." fallback a dead code. Please consider using simply std::env::current_dir() instead.
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
Overall this looks clean no blockers. I think it will be hard to keep this up to date, but as long as we are cool letting this lag a bit, then I don't see any issues.
My only notes for future consideration:
- Seems like this should be using crates/client as that is the primary interface for developers.
- This introduces some language which is in conflict with our existing codebase e.g. Sessions, Alice/Bob. As this resides in this repo the contents become canonical, which means we'll need to update it at some point to bring it in line with the project.
|
|
||
| use anyhow::{Context, Result}; | ||
| use arboard::Clipboard; | ||
| use libchat::{ChatStorage, Context as ChatManager, Introduction, StorageConfig}; |
There was a problem hiding this comment.
If this is supposed to mimic a developer example; Rather than using Context, this should use crates/client/src/client.rs
There was a problem hiding this comment.
Good point, will figure out the latest client crate status and how to integrate with it in new PR.
| Ok(Some(format!("Connected to {}", remote_user))) | ||
| } | ||
| "/chats" => { | ||
| let sessions: Vec<_> = self.state.sessions.keys().cloned().collect(); |
There was a problem hiding this comment.
Dust: Collecting this iterator doesn't immediately seem necessary.
if self.state.session.is_empty() {
...
}
for name in map.keys() {
...
}
There was a problem hiding this comment.
Tried this, but got immutable / mutable borrow conflict in for loop.
There was a problem hiding this comment.
Ah I see. The state is being mutated while being looped over - I missed that.
| fn now() -> u64 { | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_millis() as u64 | ||
| } |
There was a problem hiding this comment.
A duplicate function exists in app.rs, its not immediately clear if things would break if the implementations differed. I'd consider unifying them or labelling appropriately
| ## Features | ||
|
|
||
| - End-to-end encrypted messaging using libchat | ||
| - File-based transport for local simulation (no network required) |
There was a problem hiding this comment.
Dust: Consider using Multicast instead of file-based transport. Files are inherently reliable which hides a class of usage failures.
There was a problem hiding this comment.
Multicast sounds a good idea, I remembered tried it before, but switch to file based transport for some reason. Maybe still worth to try it later.
Make the transport switchable looks cool.
|
3 participants
Why this change:
What changes:
Ignore the double ratchet ffi removal, it's in #84
Screenshot of cli app