Implement Timeline Visualization in GUI#63
Conversation
Replaced the placeholder text in `src/bin/gui.rs` with a visual representation of events. - Added `get_events` method to `ReplayEngine` in `src/replay.rs` to expose session events. - Updated `TimeLoopGui` to store and load `replay_events`. - Implemented timeline drawing in `show_session_details` using `egui::Painter`. - Visualized different event types (Command, FileChange, etc.) with distinct colors. - Added a playback position indicator on the timeline. Co-authored-by: nur-srijan <198181700+nur-srijan@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 64b2feb in 8 seconds. Click for details.
- Reviewed
146lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_Tb6Zm09gFnQqNOtS
You can customize 
Summary of ChangesHello @nur-srijan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a new timeline visualization feature into the GUI application. It allows users to graphically observe the sequence and timing of various events within a selected replay session, enhancing the understanding of session activity and playback progression. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements the timeline visualization in the GUI. The changes are well-structured, introducing a new get_events method in the ReplayEngine and using it to render a timeline with egui. My review focuses on a performance issue in data loading, improving UI state consistency, and some minor improvements for maintainability and user experience in the new visualization code. Overall, a great addition to the GUI.
| if let Ok(engine) = ReplayEngine::new(session_id) { | ||
| if let Ok(events) = engine.get_events() { | ||
| self.replay_events = events; | ||
| } else { | ||
| self.replay_events.clear(); | ||
| } | ||
|
|
||
| if let Ok(rs) = engine.get_session_summary() { | ||
| self.replay_summary = Some(rs); | ||
| } | ||
| } else { | ||
| self.replay_events.clear(); | ||
| self.replay_summary = None; | ||
| } |
There was a problem hiding this comment.
This block for loading session data has a significant performance issue and can be made more robust.
Performance: engine.get_events() and engine.get_session_summary() both internally read and parse all events from storage. This means you are doing the same expensive work twice every time a session is selected. For large sessions, this will be slow. I recommend refactoring ReplayEngine to calculate the summary from the already-loaded events to avoid this.
Robustness: The error handling can lead to an inconsistent UI state and can be simplified. The suggested change improves robustness, but the performance issue will remain until ReplayEngine is refactored.
if let Ok(engine) = ReplayEngine::new(session_id) {
if let Ok(events) = engine.get_events() {
self.replay_summary = engine.get_session_summary().ok();
self.replay_events = events;
} else {
self.replay_events.clear();
self.replay_summary = None;
}
} else {
self.replay_events.clear();
self.replay_summary = None;
}| let (color, height_fraction, y_offset) = match event.event_type { | ||
| EventType::Command { .. } => (egui::Color32::from_rgb(100, 149, 237), 0.8, 0.1), // Cornflower Blue | ||
| EventType::FileChange { .. } => (egui::Color32::from_rgb(255, 99, 71), 0.8, 0.1), // Tomato Red | ||
| EventType::TerminalState { .. } => (egui::Color32::from_gray(100), 0.4, 0.3), | ||
| EventType::KeyPress { .. } => (egui::Color32::from_gray(60), 0.2, 0.4), | ||
| EventType::SessionMetadata { .. } => (egui::Color32::WHITE, 0.5, 0.25), | ||
| }; |
There was a problem hiding this comment.
The colors and layout properties for different event types are hardcoded as magic values within this match statement. To improve readability and maintainability, it's better to define these as named constants, perhaps at the top of the file or in a dedicated constants module. This makes it easier to see all visualization parameters in one place and change them consistently.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@jules Comment on lines 332 to 345 of Performance: Robustness: The error handling can lead to an inconsistent UI state and can be simplified. The suggested change improves robustness, but the performance issue will remain until if let Ok(engine) = ReplayEngine::new(session_id) {
if let Ok(events) = engine.get_events() {
self.replay_summary = engine.get_session_summary().ok();
self.replay_events = events;
} else {
self.replay_events.clear();
self.replay_summary = None;
}
} else {
self.replay_events.clear();
self.replay_summary = None;
}Lines +514 to +520: The colors and layout properties for different event types are hardcoded as magic values within this |
Thank you for the detailed feedback. I have addressed both points:
|
- Refactored `ReplayEngine` to expose `calculate_summary` as a static method, allowing session summary calculation from already loaded events. - Updated `src/bin/gui.rs` to compute session summary from in-memory events instead of re-reading from disk, improving performance for large sessions. - Extracted magic constants for timeline colors and dimensions into named constants in `src/bin/gui.rs` for better maintainability. Co-authored-by: nur-srijan <198181700+nur-srijan@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| sessions: Vec<timeloop_terminal::session::Session>, | ||
| selected: Option<String>, | ||
| replay_summary: Option<timeloop_terminal::replay::ReplaySummary>, | ||
| replay_events: Vec<Event>, |
Check failure
Code scanning / clippy
cannot find type Event in this scope Error
| let x = rect.min.x + t * rect.width(); | ||
|
|
||
| let (color, height_fraction, y_offset) = match event.event_type { | ||
| EventType::Command { .. } => (COLOR_COMMAND, 0.8, 0.1), |
Check failure
Code scanning / clippy
failed to resolve: use of undeclared type EventType Error
|
|
||
| let (color, height_fraction, y_offset) = match event.event_type { | ||
| EventType::Command { .. } => (COLOR_COMMAND, 0.8, 0.1), | ||
| EventType::FileChange { .. } => (COLOR_FILE_CHANGE, 0.8, 0.1), |
Check failure
Code scanning / clippy
failed to resolve: use of undeclared type EventType Error
| let (color, height_fraction, y_offset) = match event.event_type { | ||
| EventType::Command { .. } => (COLOR_COMMAND, 0.8, 0.1), | ||
| EventType::FileChange { .. } => (COLOR_FILE_CHANGE, 0.8, 0.1), | ||
| EventType::TerminalState { .. } => (COLOR_TERMINAL, 0.4, 0.3), |
Check failure
Code scanning / clippy
failed to resolve: use of undeclared type EventType Error
| EventType::Command { .. } => (COLOR_COMMAND, 0.8, 0.1), | ||
| EventType::FileChange { .. } => (COLOR_FILE_CHANGE, 0.8, 0.1), | ||
| EventType::TerminalState { .. } => (COLOR_TERMINAL, 0.4, 0.3), | ||
| EventType::KeyPress { .. } => (COLOR_KEYPRESS, 0.2, 0.4), |
Check failure
Code scanning / clippy
failed to resolve: use of undeclared type EventType Error
| EventType::FileChange { .. } => (COLOR_FILE_CHANGE, 0.8, 0.1), | ||
| EventType::TerminalState { .. } => (COLOR_TERMINAL, 0.4, 0.3), | ||
| EventType::KeyPress { .. } => (COLOR_KEYPRESS, 0.2, 0.4), | ||
| EventType::SessionMetadata { .. } => (COLOR_METADATA, 0.5, 0.25), |
Check failure
Code scanning / clippy
failed to resolve: use of undeclared type EventType Error
| let y_end = y_start + rect.height() * height_fraction; | ||
|
|
||
| // Use thinner lines for keypresses to avoid clutter | ||
| let stroke_width = if matches!(event.event_type, EventType::KeyPress { .. }) { |
Check failure
Code scanning / clippy
failed to resolve: use of undeclared type EventType Error
Updated the summarize_session call to include api_key_opt.
| async fn run_ai_summarize(session_id: &str, model: Option<&str>) -> Result<(), TimeLoopError> { | ||
| let model = model.unwrap_or("openrouter/auto"); | ||
| let summary = timeloop_terminal::ai::summarize_session(session_id, model, None).await?; | ||
| let summary = timeloop_terminal::ai::summarize_session(session_id, model, api_key_opt).await?; |
Check failure
Code scanning / clippy
cannot find value api_key_opt in this scope Error
| let _ = Self::save_to_path(path, self); | ||
| } else if self.inner.is_none() { | ||
| let _ = Self::save_to_disk(false); | ||
| let _ = Self::save_to_disk(); |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
General fix: Avoid initializing cryptographic salts with hard‑coded constants. Instead, allocate the buffer directly using a CSPRNG so that no constant value is ever associated with the salt variable, even transiently.
Best targeted fix here: Replace the two‑step pattern
let mut salt = vec![0u8; 16];
let mut osrng = rand::rngs::OsRng;
osrng.fill_bytes(&mut salt);with a single expression that generates the salt from random bytes, e.g. using rand::RngCore::next_u32 in a small loop or, more simply, using rand::Rng’s gen::<[u8; 16]>() (if available in this file), then collecting into a Vec<u8>. Since we cannot assume traits are already imported, a minimal, explicit approach that only uses OsRng and standard library methods is to allocate an uninitialized [u8; 16] array, fill it via fill_bytes, and convert it to Vec<u8>. This keeps the same salt length and preserves the existing call to derive_key_with_params, so functionality (random salt, same parameters) remains unchanged.
Concretely, in src/storage.rs, in the block starting at line 398 where a new salt is generated, change the code around line 400–405 to allocate a [u8; 16] array, fill it with OsRng, then convert it into a Vec<u8> that is passed to derive_key_with_params and stored in encryption_salt. No new external dependencies are required; we can continue using rand::rngs::OsRng and fill_bytes as before.
| @@ -397,9 +397,10 @@ | ||
|
|
||
| // If file didn't exist or wasn't encrypted, generate a salt now | ||
| if encryption_salt.is_none() { | ||
| let mut salt = vec![0u8; 16]; | ||
| let mut osrng = rand::rngs::OsRng; | ||
| osrng.fill_bytes(&mut salt); | ||
| let mut salt_array = [0u8; 16]; | ||
| osrng.fill_bytes(&mut salt_array); | ||
| let salt = salt_array.to_vec(); | ||
| let key = Self::derive_key_with_params(passphrase, &salt, Some(params)); | ||
| encryption_key = Some(key); | ||
| encryption_salt = Some(salt); |
2 participants
Implemented the timeline visualization feature in the GUI application.
This involved:
src/replay.rsto expose aget_eventsmethod onReplayEngine, allowing access to the raw event data needed for visualization.src/bin/gui.rs:replay_eventsto theTimeLoopGuistate.replay_eventswhen a session is selected.egui.Verified the changes by running
cargo check --features guiandcargo clippy --features guito ensure no compilation errors or lints were introduced in the modified files. Also ran existing tests withcargo test --lib.PR created automatically by Jules for task 14868285181004966445 started by @nur-srijan
Important
Adds timeline visualization to GUI, rendering events from
ReplayEngineas color-coded lines usingegui.get_events()method toReplayEngineinsrc/replay.rsto retrieve event data for visualization.TimeLoopGuiinsrc/bin/gui.rsto includereplay_eventsstate and populate it when a session is selected.src/bin/gui.rsusingegui, rendering events as color-coded lines based on their type.cargo check --features guiandcargo clippy --features gui.cargo test --lib.This description was created by
for 64b2feb. You can customize this summary. It will automatically update as commits are pushed.