Feat/additional info apps - #4
Conversation
c33fb8a to
e582461
Compare
e582461 to
b98533b
Compare
…in/async-debugger into feat/additional-info-apps
…in/async-debugger into feat/additional-info-apps
valexandru
left a comment
There was a problem hiding this comment.
Inca nu am terminat review-ul pe toate componentele, dar ca si intrebari/sugestii generale:
- Avem cumva vreun mod de management de log-uri? Adica pe baza unui flag sa afisam doar error-urile de exemplu?
- Ar fi super daca pentru macar unul din exemplele pe care le avem daca putem face un screen recording de cum se foloseste tool-ul pe acel exemplu. Si il putem include ca un gif de exemplu in README
There was a problem hiding this comment.
In fisierul asta as mai vrea sa mai vad urmatoarele sectiuni si modificari:
- Requirements - care sa specifice pe ce sisteme de operare functioneaza, daca trebuie sa ai ceva anume instalat si ce anume
- In momentul in care vom avea release-uri pe repo-ul asta, vom avea direct applicatiile generate iar utilizatorii vor putea direct descarca si instala release-urile respective. Putem face un release cu un tag 0.1.0 sau ceva de genul asta pe branch-ul asta si sa scriem aici in readme cum se instaleaza/foloseste
There was a problem hiding this comment.
De asemenea, as vrea sa mai vad niste screenshot-uri reprezentative cu aplicatia ca sa fie ceva mai de impact pentru utilizator
There was a problem hiding this comment.
De asemenea, vreau sa mai avem un mic fisier gen Architecture.md care sa descrie componentele aplicatiei si cum comunica intre ele, dar si ierarhia repository-ului la modul:
--- src-tauri
|------ commands -> comenzile apelabile de catre componenta de front-end. + descriere sumara de care sunt ele
|
Plus daca poti sa rezolvi conflictele care sunt fata de branch-ul main |
| use std::sync::Arc; | ||
| use tokio::sync::{Barrier, RwLock}; | ||
| use tokio::time::{Duration, Instant, sleep}; | ||
|
|
There was a problem hiding this comment.
Short docs description here at the beginning of this file for what it does and its purpose.
There was a problem hiding this comment.
Let's choose a better format to display the time here in the "ReceivedAt" column. Something similar to what you did in the other screens
There was a problem hiding this comment.
Yup, the reason why it loads so slowly is the size of the gif. Let's compress it a bit more to improve the loading performance
There was a problem hiding this comment.
Let's change the "Target" column name into something more suggestive
There was a problem hiding this comment.
Besides application names, let's also put actual names at "Name" instead of the "No Name" value
| # Architecture | ||
| ``` | ||
| ┌── common.rs/get_pid_hosting_at ────────┐ | ||
| ┌── domain/application.rs/new ─> │ │ |
There was a problem hiding this comment.
Change this into a draw.io with a bit more explanations about what does components do.
| debug!("Enabling app {}", app.title()); | ||
| self.state.enable_app(*app.id(), connection).await; | ||
| } else { | ||
| // TODO |
There was a problem hiding this comment.
We need to replace this with at least logging on error.
| self.state.get_current_applications_list().await | ||
| } | ||
|
|
||
| pub async fn delete_connection(&self, uuid: Uuid) { | ||
| self.connection_manager.disconnect_app(uuid).await; | ||
| self.state.delete_app(uuid).await | ||
| // self.state.delete_app(uuid).await |
There was a problem hiding this comment.
| // self.state.delete_app(uuid).await |
| } | ||
| // Update app connection status | ||
| if app.state() == ApplicationState::Disabled { | ||
| // If app is disabled we dont save anything |
There was a problem hiding this comment.
| // If app is disabled we dont save anything | |
| // If app is disabled, we don't save anything |
| ) { | ||
| // debug for missed resources_updates | ||
| if resources_update.dropped_events > 0 { | ||
| println!( |
There was a problem hiding this comment.
let's use debug, info and error everywhere instead of println!
| // Use netstat to list all TCP/UDP connections with PIDs. | ||
| let output = Command::new("netstat").args(["-ano"]).output().ok()?; | ||
| // This needs to be lossy, because the netstat output sometimes contains non UTF-8 characters | ||
| let stdout = String::from_utf8_lossy(&output.stdout); |
There was a problem hiding this comment.
Add comment that this is needed for running on windows
There was a problem hiding this comment.
We need to restructure all the Vue component files. Here is an example of the component:
// The template that contains the html code comes first
<template>
<div class="counter">
<h2>Count: {{ count }}</h2>
<p>Double: {{ doubleCount }}</p>
<button @click="increment">Increment</button>
</div>
</template>
// Then, we have the typescript/javascript code
<script>
// Exporting the component
export default {
name: 'Counter',
// parameters of the components -> you can think of them as the global variables
data() {
return {
count: 0
}
},
// values that are computed by a function
computed: {
doubleCount() {
return this.count * 2
}
},
// these are the more complex functions that you use in the code, for example actions on buttons
methods: {
increment() {
this.count++
}
}
}
</script>
// Finally, if there is needed, local styles used in the file
<style scoped>
.counter {
padding: 1em;
border-radius: 4px;
background: #f5f5f5;
}
button {
margin-top: 10px;
}
</style>
| {title: "Location", key: "location", align: "center"}, | ||
| ]); | ||
|
|
||
| const appList = computed(() => { |
There was a problem hiding this comment.
For example, this computed parameter should be in the computed parameter of the Vue component (see the main file).
| </v-data-table> | ||
| </v-card-text> | ||
| </v-card> | ||
| </template> No newline at end of file |
There was a problem hiding this comment.
| </template> | |
| </template> | |
There was a problem hiding this comment.
This applies to all files
| appToEdit.id = uuid as string; | ||
| } | ||
| ).catch( | ||
| (e) => console.log("Failed to edit application due to " + e) |
There was a problem hiding this comment.
For consistency, when you have an error, use console.err()
| ).catch( | ||
| (e) => console.log("Failed to edit application due to " + e) | ||
| ); | ||
| //appToEdit.connection_status = "Connected"; |
There was a problem hiding this comment.
| //appToEdit.connection_status = "Connected"; |
There was a problem hiding this comment.
If this is not needed, let's just delete it
|
We also need workflows for checking rust and typescript code. Maybe move them from the code in the Andrei's PR |
DTeodor-Alexaandru
left a comment
There was a problem hiding this comment.
Overall review:
- Each ramification should be logged, otherwise the app debugging will be hard
- Each operation that may fail should be error handled and logged..
- In many places Option is used instead of Result and there is no log
- In frontend all the errors should be handled or at least displayed, there shouldn t be any user interaction with no outcome to the user
- Let's replace everywhere
println!with logger - The thread panic should be handled properly, if a thread panics and other work fine, what will be the outcome? The app is fine to continue or it should be restarted?
There was a problem hiding this comment.
I would add in prerequisites section all the prerequisites:)))
Eg deno, webkit2gtk-4.1, libsoup-3.0 etc... for the final version i would start with a fresh os and note all the dependencies
| cargo run --example <example_name> | ||
| ``` | ||
|
|
||
| - `0s1s` spawns three asynchronous tasks: |
There was a problem hiding this comment.
bullet point listing is a bit unclear, I would enumerate
| if zero_tx.send(chunk).await.is_err() { | ||
| break; | ||
| } | ||
| println!("Sent 1 chunk of 0s"); |
There was a problem hiding this comment.
I would use logger, instead of println!
| use crate::error::Error; | ||
| use crate::state_manager::StateManager; | ||
| #[derive(Serialize, Deserialize)] | ||
| pub struct ExportEntry { |
There was a problem hiding this comment.
Let's have a separate structs dir and splitted into domains
There was a problem hiding this comment.
Also, doc for all the structs
| let conn: Connection = state_manager | ||
| .connection_manager | ||
| .connect_app(*app.id(), app.url().clone(), app.pid()) | ||
| .await?; |
There was a problem hiding this comment.
The error is handled properly in the frontend? Or just logged?
There was a problem hiding this comment.
I would also log the error before throwing it
| if let Some(app) = apps.get_mut(&app_id) { | ||
| app.writeable().set_pid(new_pid); | ||
| } | ||
| // drop guard |
There was a problem hiding this comment.
| // drop guard |
| /// and update each poll’s `task_name` and `task_color`. | ||
| /// 3. If there is an entry in the task‐operations store matching | ||
| /// the same key, update its `task_name` and `task_color` as well. | ||
| pub async fn edit_state_task( |
There was a problem hiding this comment.
error handling, if the task id doesn't exist
| /// – Otherwise, append a new `CPUOverview` entry (with `started_at` and optional `stopped_at`). | ||
| /// – If no `TaskOp` record exists, create one from scratch using any known | ||
| /// task metadata (name/color) and the new CPU overview. | ||
| pub async fn handle_async_op_update(&self, app_id: Uuid, async_op_update: AsyncOpUpdate) { |
There was a problem hiding this comment.
same idea, error handling
| appToEdit.id = uuid as string; | ||
| } | ||
| ).catch( | ||
| (e) => console.log("Failed to edit application due to " + e) |
There was a problem hiding this comment.
the error should be handled or at least displayed
* wip * fix lint and refactor sysinfo * fix rust code * remove cpu * fix navitems * fix sidebar * update workflow --------- Co-authored-by: Cristiana Precup <cristianaprecup65@gmail.com>
There was a problem hiding this comment.
As face un modul warning, in mod.rs sa pastram trait-ul Warning, iar in sub-module separate sa facem cate un fisier pentru fieacare tip de warning
* move computation from tauri-commands to backend * remove unused function * fix edit task method * remove any
No description provided.