From cfdf27c8c769683e7f57c7478aa9ee73b6466b42 Mon Sep 17 00:00:00 2001
From: funman300 <funman300@gmail.com>
Date: Wed, 27 May 2026 19:13:42 -0700
Subject: [PATCH] fix(time-attack): clamp timer to zero and pause during
 overlays (#54, #55)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 solitaire_engine/src/time_attack_plugin.rs | 141 +++++++++++++++------
 1 file changed, 102 insertions(+), 39 deletions(-)

diff --git a/solitaire_engine/src/time_attack_plugin.rs b/solitaire_engine/src/time_attack_plugin.rs
index d11c6bf..2af7b9c 100644
--- a/solitaire_engine/src/time_attack_plugin.rs
+++ b/solitaire_engine/src/time_attack_plugin.rs
@@ -27,8 +27,8 @@ use std::time::{SystemTime, UNIX_EPOCH};
 use bevy::prelude::*;
 use solitaire_core::game_state::GameMode;
 use solitaire_data::{
-    delete_time_attack_session_at, load_time_attack_session_from, save_time_attack_session_to,
-    time_attack_session_path, TimeAttackSession,
+    TimeAttackSession, delete_time_attack_session_at, load_time_attack_session_from,
+    save_time_attack_session_to, time_attack_session_path,
 };
 
 use crate::challenge_plugin::CHALLENGE_UNLOCK_LEVEL;
@@ -155,9 +155,10 @@ fn handle_start_time_attack_request(
     // resuming whatever the disk happened to hold. Failures here are
     // logged but never fatal.
     if let Some(p) = path.as_ref().and_then(|r| r.0.as_deref())
-        && let Err(e) = delete_time_attack_session_at(p) {
-            warn!("time_attack_session: failed to delete stale session: {e}");
-        }
+        && let Err(e) = delete_time_attack_session_at(p)
+    {
+        warn!("time_attack_session: failed to delete stale session: {e}");
+    }
     new_game.write(NewGameRequestEvent {
         seed: None,
         mode: Some(GameMode::TimeAttack),
@@ -167,34 +168,34 @@ fn handle_start_time_attack_request(
 
 fn advance_time_attack(
     time: Res<Time>,
+    game: Res<GameStateResource>,
     mut session: ResMut<TimeAttackResource>,
     mut ended: MessageWriter<TimeAttackEndedEvent>,
     paused: Option<Res<crate::pause_plugin::PausedResource>>,
     path: Option<Res<TimeAttackSessionPath>>,
-    home_screens: Query<(), With<crate::home_plugin::HomeScreen>>,
-    win_overlays: Query<(), With<crate::win_summary_plugin::WinSummaryOverlay>>,
+    modal_scrims: Query<(), With<crate::ui_modal::ModalScrim>>,
 ) {
     if !session.active {
         return;
     }
-    // Pause the countdown while Home, the Pause overlay, or the Win Summary
-    // overlay is visible — the player should not lose time while reading results
-    // or navigating menus.
-    if paused.is_some_and(|p| p.0) || !home_screens.is_empty() || !win_overlays.is_empty() {
+    // No shared screen-state enum currently covers every overlay. Pause the
+    // countdown whenever gameplay is blocked by a modal, the pause flag, or a
+    // just-won board state.
+    if paused.is_some_and(|p| p.0) || game.0.is_won || !modal_scrims.is_empty() {
         return;
     }
-    session.remaining_secs -= time.delta_secs();
+    session.remaining_secs = (session.remaining_secs - time.delta_secs()).max(0.0);
     if session.remaining_secs <= 0.0 {
         let wins = session.wins;
         session.active = false;
-        session.remaining_secs = 0.0;
         ended.write(TimeAttackEndedEvent { wins });
         // Session ended naturally — delete the persisted file so the next
         // launch sees no in-progress session.
         if let Some(p) = path.as_ref().and_then(|r| r.0.as_deref())
-            && let Err(e) = delete_time_attack_session_at(p) {
-                warn!("time_attack_session: failed to delete on expiry: {e}");
-            }
+            && let Err(e) = delete_time_attack_session_at(p)
+        {
+            warn!("time_attack_session: failed to delete on expiry: {e}");
+        }
     }
 }
 
@@ -324,6 +325,16 @@ mod tests {
         input.press(KeyCode::KeyT);
     }
 
+    fn advance_by(app: &mut App, total_secs: f32) {
+        app.insert_resource(bevy::time::TimeUpdateStrategy::ManualDuration(
+            std::time::Duration::from_secs_f32(0.2),
+        ));
+        let ticks = (total_secs / 0.2).ceil() as usize + 1;
+        for _ in 0..ticks {
+            app.update();
+        }
+    }
+
     #[test]
     fn pressing_t_below_unlock_level_is_ignored() {
         let mut app = headless_app();
@@ -359,17 +370,15 @@ mod tests {
     }
 
     #[test]
-    fn timer_expiry_fires_ended_event_and_clears_active() {
+    fn timer_expiry_clamps_to_zero_and_fires_ended_event() {
         let mut app = headless_app();
-        // Set the session to an already-expired state (remaining < 0).
-        // MinimalPlugins time delta is nonzero so we skip the intermediate
-        // 0.001-remaining step to avoid a double-fire.
         *app.world_mut().resource_mut::<TimeAttackResource>() = TimeAttackResource {
             active: true,
-            remaining_secs: -1.0,
+            remaining_secs: 0.3,
             wins: 5,
         };
-        app.update();
+
+        advance_by(&mut app, 0.4);
 
         let session = app.world().resource::<TimeAttackResource>();
         assert!(!session.active);
@@ -382,6 +391,30 @@ mod tests {
         assert_eq!(fired[0].wins, 5);
     }
 
+    #[test]
+    fn modal_overlay_pauses_session_timer() {
+        let mut app = headless_app();
+        app.world_mut().spawn(crate::ui_modal::ModalScrim);
+        *app.world_mut().resource_mut::<TimeAttackResource>() = TimeAttackResource {
+            active: true,
+            remaining_secs: 5.0,
+            wins: 2,
+        };
+
+        advance_by(&mut app, 0.4);
+
+        let session = app.world().resource::<TimeAttackResource>();
+        assert!(session.active, "session must stay active while a modal is open");
+        assert_eq!(session.remaining_secs, 5.0);
+
+        let events = app.world().resource::<Messages<TimeAttackEndedEvent>>();
+        let mut cursor = events.get_cursor();
+        assert!(
+            cursor.read(events).next().is_none(),
+            "TimeAttackEndedEvent must not fire while a modal is open"
+        );
+    }
+
     #[test]
     fn win_during_session_increments_wins_and_auto_deals() {
         let mut app = headless_app();
@@ -462,10 +495,13 @@ mod tests {
         };
         app.update();
 
-        // remaining_secs must not have been reset to 0.0 (pause blocked the update).
+        // remaining_secs must not have been clamped to 0.0 (pause blocked the update).
         let session = app.world().resource::<TimeAttackResource>();
         assert!(session.active, "session must still be active while paused");
-        assert!(session.remaining_secs < 0.0, "remaining_secs must not change while paused");
+        assert!(
+            session.remaining_secs < 0.0,
+            "remaining_secs must not change while paused"
+        );
 
         // No ended event must have been emitted.
         let events = app.world().resource::<Messages<TimeAttackEndedEvent>>();
@@ -509,8 +545,7 @@ mod tests {
         // and we load immediately, so wall-clock elapsed is ~0 and the
         // restored remaining_secs should match what we wrote within a tiny
         // epsilon (allowing for the test taking a few seconds to run).
-        let loaded =
-            load_time_attack_session_from(&path).expect("file should exist after exit");
+        let loaded = load_time_attack_session_from(&path).expect("file should exist after exit");
         assert!(
             (loaded.remaining_secs - 240.0).abs() < 5.0,
             "remaining_secs must round-trip within 5 s tolerance, got {}",
@@ -527,8 +562,11 @@ mod tests {
     fn exit_clears_persisted_file_when_no_active_session() {
         let path = tmp_ta_path("exit_clear");
         // Pre-create a stale file.
-        std::fs::write(&path, b"{\"remaining_secs\":100.0,\"wins\":1,\"saved_at_unix_secs\":0}")
-            .expect("write stale");
+        std::fs::write(
+            &path,
+            b"{\"remaining_secs\":100.0,\"wins\":1,\"saved_at_unix_secs\":0}",
+        )
+        .expect("write stale");
         assert!(path.exists());
 
         let mut app = headless_app();
@@ -537,7 +575,10 @@ mod tests {
         app.world_mut().write_message(AppExit::Success);
         app.update();
 
-        assert!(!path.exists(), "stale file must be deleted on exit when session is inactive");
+        assert!(
+            !path.exists(),
+            "stale file must be deleted on exit when session is inactive"
+        );
     }
 
     /// `auto_save_time_attack_session` writes the session once the
@@ -557,10 +598,15 @@ mod tests {
             wins: 2,
         };
         // Pre-seed the timer past the threshold so the very next update fires the save.
-        app.insert_resource(TimeAttackAutoSaveTimer(TIME_ATTACK_AUTO_SAVE_INTERVAL_SECS + 0.1));
+        app.insert_resource(TimeAttackAutoSaveTimer(
+            TIME_ATTACK_AUTO_SAVE_INTERVAL_SECS + 0.1,
+        ));
         app.update();
 
-        assert!(path.exists(), "auto-save file must exist after timer crosses threshold");
+        assert!(
+            path.exists(),
+            "auto-save file must exist after timer crosses threshold"
+        );
         let loaded = load_time_attack_session_from(&path).expect("session must load");
         assert_eq!(loaded.wins, 2);
 
@@ -578,10 +624,15 @@ mod tests {
         let mut app = headless_app();
         app.insert_resource(TimeAttackSessionPath(Some(path.clone())));
         // Session stays at default (inactive). Timer is past threshold.
-        app.insert_resource(TimeAttackAutoSaveTimer(TIME_ATTACK_AUTO_SAVE_INTERVAL_SECS + 0.1));
+        app.insert_resource(TimeAttackAutoSaveTimer(
+            TIME_ATTACK_AUTO_SAVE_INTERVAL_SECS + 0.1,
+        ));
         app.update();
 
-        assert!(!path.exists(), "auto-save must not fire when session is inactive");
+        assert!(
+            !path.exists(),
+            "auto-save must not fire when session is inactive"
+        );
     }
 
     /// Starting a fresh session must delete any stale persisted file so a
@@ -591,8 +642,11 @@ mod tests {
     fn starting_new_session_deletes_stale_persisted_file() {
         let path = tmp_ta_path("start_clears");
         // Pre-create a stale file.
-        std::fs::write(&path, b"{\"remaining_secs\":42.0,\"wins\":99,\"saved_at_unix_secs\":0}")
-            .expect("write stale");
+        std::fs::write(
+            &path,
+            b"{\"remaining_secs\":42.0,\"wins\":99,\"saved_at_unix_secs\":0}",
+        )
+        .expect("write stale");
 
         let mut app = headless_app();
         app.insert_resource(TimeAttackSessionPath(Some(path.clone())));
@@ -602,7 +656,10 @@ mod tests {
         press_t(&mut app);
         app.update();
 
-        assert!(!path.exists(), "stale persisted file must be cleared at session start");
+        assert!(
+            !path.exists(),
+            "stale persisted file must be cleared at session start"
+        );
 
         // And the live resource must reflect a fresh session, not the stale data.
         let session = app.world().resource::<TimeAttackResource>();
@@ -622,8 +679,11 @@ mod tests {
     fn session_expiry_deletes_persisted_file() {
         let path = tmp_ta_path("expiry_clears");
         // Pre-create a file that simulates the auto-save's prior write.
-        std::fs::write(&path, b"{\"remaining_secs\":1.0,\"wins\":7,\"saved_at_unix_secs\":0}")
-            .expect("write");
+        std::fs::write(
+            &path,
+            b"{\"remaining_secs\":1.0,\"wins\":7,\"saved_at_unix_secs\":0}",
+        )
+        .expect("write");
         assert!(path.exists());
 
         let mut app = headless_app();
@@ -637,7 +697,10 @@ mod tests {
 
         app.update();
 
-        assert!(!path.exists(), "persisted file must be deleted on natural expiry");
+        assert!(
+            !path.exists(),
+            "persisted file must be deleted on natural expiry"
+        );
         let session = app.world().resource::<TimeAttackResource>();
         assert!(!session.active);
     }