Restore statistics tracking with verified Qt lifecycle - #44

src/squid_statistics.py (2)

123-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider having update_distance delegate here.

add_distance handles multi-rollover correctly via divmod, while the older update_distance only subtracts one limit. Routing update_distance through add_distance(math.sqrt(dx*dx + dy*dy)) removes the duplicate rollover logic and the behavioral divergence between the two entry points.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/squid_statistics.py` around lines 123 - 134, Update update_distance to
calculate the Euclidean distance and delegate to add_distance, removing its
duplicate rollover handling. Preserve the existing input behavior while ensuring
multi-rollover accumulation uses add_distance’s divmod logic.

---

17-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Annotate the lookup tables as ClassVar to silence RUF012.

These are immutable-intent lookup tables; ClassVar documents that and clears the three Ruff warnings.

♻️ Proposed annotation

+from typing import ClassVar, Dict
+
 class SquidStatistics:
@@
-    _PERSISTENCE_KEYS = {
+    _PERSISTENCE_KEYS: ClassVar[Dict[str, str]] = {
@@
-    _EVENT_KEYS = {
+    _EVENT_KEYS: ClassVar[Dict[str, str]] = {
@@
-    _NEURON_BIRTH_KEYS = {
+    _NEURON_BIRTH_KEYS: ClassVar[Dict[str, str]] = {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/squid_statistics.py` around lines 17 - 64, Annotate the class-level
lookup tables _PERSISTENCE_KEYS, _EVENT_KEYS, and _NEURON_BIRTH_KEYS as ClassVar
with their existing dictionary value types. Add the required ClassVar import if
needed, while preserving all table contents and behavior.

Source: Linters/SAST tools

tests/test_squid_statistics.py (1)

81-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for add_distance rollover and increment legacy-name mapping.

Both are new/reworked public APIs in this layer with branch logic (multi-rollover via divmod, unknown-name False return) and no test here. load_statistics / to_dict round-tripping a partial payload is also worth a case unless it is fully covered by the persistence tests in the later layer.

💚 Suggested additional cases

    def test_add_distance_rolls_over_multiple_times(self):
        from src.squid_statistics import DISTANCE_ROLLOVER_LIMIT

        self.statistics.add_distance(DISTANCE_ROLLOVER_LIMIT * 2 + 5)

        self.assertEqual(self.statistics.distance_swam, 5)
        self.assertEqual(self.statistics.distance_swam_multiplier, 3)

    def test_add_distance_rejects_invalid_values(self):
        with self.assertRaises(ValueError):
            self.statistics.add_distance(-1)
        with self.assertRaises(ValueError):
            self.statistics.add_distance(float("inf"))

    def test_increment_maps_legacy_names_and_reports_unknown(self):
        self.assertTrue(self.statistics.increment("cheese_eaten"))
        self.assertEqual(self.statistics.cheese_consumed, 1)
        self.assertFalse(self.statistics.increment("not_a_real_event"))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_squid_statistics.py` around lines 81 - 89, Add tests covering the
public APIs in the statistics test suite: verify add_distance handles multiple
DISTANCE_ROLLOVER_LIMIT rollovers, rejects negative and non-finite values, and
verify increment maps the legacy cheese_eaten name while returning False for
unknown event names. Add a load_statistics/to_dict partial-payload round-trip
case only if persistence tests do not already cover it.

tests/test_statistics_tab_lifecycle.py (2)

74-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

FakeSquid omits the core stat attributes.

It only defines anxiety, happiness, satisfaction, is_sleeping, is_sick. That is enough for the statistics path, but any future assertion that reaches SquidBrainWindow.sync_state_from_squid (which reads hunger, cleanliness, sleepiness, curiosity, squid_x/y) will fail with AttributeError rather than a meaningful assertion. Worth adding the missing defaults now.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_statistics_tab_lifecycle.py` around lines 74 - 81, Update the
FakeSquid fixture to define default values for hunger, cleanliness, sleepiness,
curiosity, squid_x, and squid_y alongside its existing attributes. Ensure these
defaults satisfy SquidBrainWindow.sync_state_from_squid so future assertions
fail on behavior rather than missing attributes.

---

89-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Optional: split this into three tests (initial bind / live event refresh / unbind+rebind).

At ~210 lines it exercises four distinct phases behind one name, so a failure in the rebind assertions reports as a failure of "late logic wiring". A shared helper that builds the patched window plus one logic factory would keep the setup cost identical.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_statistics_tab_lifecycle.py` around lines 89 - 298, Split
test_late_logic_wiring_projects_model_statistics_into_real_tab into focused
tests for initial binding, live event refresh, and unbind/rebind behavior.
Extract the repeated patched-window setup and TamagotchiLogic construction into
shared helpers, preserving the existing assertions and setup behavior while
giving each phase its own descriptive test name.

src/brain_statistics_tab.py (2)

267-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nit: "Max Neurons" is the only unlocalised row in the export.

Every other line uses loc.get(...). Consider adding a key (e.g. stat_max_neurons) and reusing it for the label at Line 153 too.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/brain_statistics_tab.py` around lines 267 - 270, Localize the “Max
Neurons” export label in the statistics export code by adding and using a
localization key such as stat_max_neurons through loc.get(...). Reuse the same
key for the corresponding label near the existing line-153 display so both
occurrences remain consistent with the other localized rows.

---

187-194: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Optional: gate the 1 Hz refresh on is_visible.

is_visible is now tracked in showEvent/hideEvent but never read, so the timer keeps re-syncing and repainting 17 labels while the tab is hidden. Cheap, but the flag suggests the intent was to skip hidden updates.

♻️ Proposed change

     def update_statistics(self):
         """Refresh the UI from the canonical squid statistics model."""
         if not self.tamagotchi_logic or not self.tamagotchi_logic.squid:
             return
 
         self._sync_from_squid_statistics()
         self.statistics['last_update_time'] = time.time()
-        self.update_display()
+        if self.is_visible:
+            self.update_display()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/brain_statistics_tab.py` around lines 187 - 194, Optionally update
update_statistics to return early when is_visible is false, while preserving the
existing tamagotchi_logic and squid guard. This should prevent
_sync_from_squid_statistics, timestamp updates, and update_display from running
while the tab is hidden; continue the current refresh flow when the tab is
visible.

tests/test_statistics_persistence.py (1)

15-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Good schema-lock test. Mirroring the mapping as a literal and asserting equality means any accidental rename or dropped persisted field fails loudly. Note it does couple the suite to the private _PERSISTENCE_KEYS; if the model ever exposes a public accessor, prefer that here and at Line 400.

Also applies to: 91-107

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_statistics_persistence.py` around lines 15 - 44, The schema-lock
test currently depends directly on the private _PERSISTENCE_KEYS mapping. If a
public accessor for the persistence schema exists or is introduced, update
EXPECTED_PERSISTENCE_KEYS and the corresponding assertions around the additional
referenced range to use that public accessor instead, while preserving the exact
mapping equality check.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/squid_statistics.py (3)

123-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider routing update_distance through add_distance.

Rollover logic now exists in two places. update_distance at lines 107-121 keeps the older single-subtraction form, which under-counts if one step crosses more than one rollover limit. add_distance handles that case with divmod. Delegation removes the duplication and makes both paths behave the same.

♻️ Proposed delegation

     def update_distance(self, dx, dy):
         '''Track distance traveled by squid with rollover protection'''
         distance = math.sqrt(dx*dx + dy*dy)
-        self.distance_swam += distance
-
-        # Check for rollover and reset with multiplier
-        if self.distance_swam >= DISTANCE_ROLLOVER_LIMIT:
-            self.distance_swam = self.distance_swam - DISTANCE_ROLLOVER_LIMIT
-            self.distance_swam_multiplier += 1
-
+        previous_multiplier = self.distance_swam_multiplier
+        self.add_distance(distance)
+        if self.distance_swam_multiplier != previous_multiplier:
             # Log the rollover event
             if hasattr(self.squid, 'tamagotchi_logic'):
                 self.squid.tamagotchi_logic.show_message(
                     f"🌊 Distance counter rolled over! Now at {self.distance_swam_multiplier}x"
                 )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/squid_statistics.py` around lines 123 - 134, Update update_distance to
delegate its distance accumulation and rollover handling to add_distance,
removing the duplicated single-subtraction rollover logic while preserving any
update_distance-specific state or behavior that is still required.

---

192-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract counter initialization instead of calling __init__ again.

load_statistics and reset both call self.__init__(self.squid) to clear counters. That call also resets self.squid and self.start_time, and it will silently clear any non-counter attribute added to __init__ later. Move the counter assignments into a private method and call it from __init__, load_statistics, and reset. The reset semantics stay the same, and the intent becomes explicit.

♻️ Proposed extraction

     def __init__(self, squid):
         self.squid = squid
         self.start_time = time.time()
+        self._reset_counters()
+        self._last_sickness_state = bool(getattr(squid, 'is_sick', False))
+
+    def _reset_counters(self):
+        """Return every tracked counter to its initial value."""
         self.total_age_seconds = 0
         self.sushi_consumed = 0
         # ... remaining counter assignments ...
         self.max_neurons_reached = DEFAULT_NEURON_COUNT
         self.current_neurons = DEFAULT_NEURON_COUNT
-        self._last_sickness_state = bool(getattr(squid, 'is_sick', False))

     def load_statistics(self, data):
         """Load canonical statistics from a desktop ``statistics.json`` dict."""
         # Loading replaces the current model. Without this reset, fields absent
         # from an older or partial save leak in from the live session and create
         # a hybrid of two different states.
-        self.__init__(self.squid)
+        self._reset_counters()
+        self.start_time = time.time()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/squid_statistics.py` around lines 192 - 197, Extract the counter-field
initialization from __init__ into a private helper, then invoke that helper from
__init__, load_statistics, and reset. Replace the self.__init__(self.squid)
calls in load_statistics and reset so they clear only counters while preserving
self.squid, self.start_time, and other non-counter state.

---

291-300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The max_poops_cleaned name does not match what observe_poop_count records.

The method records the largest number of poop items present at one time. The attribute name states cleaned poops. Because max_poops_cleaned is a persisted key and a statistics tab label, renaming it needs a save-schema and UI change. If you keep the name, add a short comment that states the value is a simultaneous-count maximum.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/squid_statistics.py` around lines 291 - 300, Keep the persisted
max_poops_cleaned name unchanged for compatibility, and add a concise comment
near its update in observe_poop_count clarifying that it stores the maximum
simultaneous poop-item count, not the number cleaned.

tests/test_statistics_persistence.py (2)

243-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert against DEFAULT_NEURON_COUNT instead of the literal 8.

The expected fallback is the model default. src/squid_statistics.py exports DEFAULT_NEURON_COUNT, and tests/test_statistics_tab_lifecycle.py already imports it. If the default changes, these assertions and the ones at lines 163-164 and 180-181 fail for a reason unrelated to the behavior under test.

♻️ Proposed change

-                self.assertEqual(statistics.current_neurons, 8)
-                self.assertEqual(statistics.max_neurons_reached, 8)
+                self.assertEqual(
+                    statistics.current_neurons,
+                    DEFAULT_NEURON_COUNT,
+                )
+                self.assertEqual(
+                    statistics.max_neurons_reached,
+                    DEFAULT_NEURON_COUNT,
+                )

Add the import near the existing SquidStatistics import:

from src.squid_statistics import DEFAULT_NEURON_COUNT, SquidStatistics

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_statistics_persistence.py` around lines 243 - 244, Replace the
hard-coded expected value 8 in the statistics persistence assertions with
DEFAULT_NEURON_COUNT, importing it alongside SquidStatistics from
src.squid_statistics. Apply the same update to the corresponding assertions at
lines 163-164 and 180-181.

---

246-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Split the two loader scenarios into separate tests.

This method verifies _apply_save_data and load_game in sequence. If the first block of assertions fails at line 280, the ZIP loader path never runs, so one defect hides the other. Two test methods that share the malformed_statistics fixture, or a subTest per loader, report both paths independently.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_statistics_persistence.py` around lines 246 - 317, Split
test_both_public_loaders_accept_malformed_numeric_statistics into separate tests
covering _apply_save_data and load_game independently. Share the
malformed_statistics fixture/setup without duplicating its definition, and
preserve all existing assertions so each loader reports failures even when the
other path is broken.

tests/test_statistics_event_wiring.py (1)

562-568: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Do not mask StatisticsTab mutations in the rock test.

statistics_tab.increment_stat currently delegates to squid.statistics.increment. If RockInteractionManager.throw_rock() regresses to the obsolete statistics-tab mutation path, this test still records one event and passes. Replace the lambda with a Mock and assert that it is not called. Keep the model counter assertion.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_statistics_event_wiring.py` around lines 562 - 568, Update the
statistics_tab fixture in the rock interaction test so increment_stat is a Mock
rather than delegating to squid.statistics.increment, then assert that this mock
is not called after RockInteractionManager.throw_rock(). Preserve the existing
model counter assertion.

tests/test_squid_statistics.py (1)

122-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert that no event counter changes for an unknown event.

The test checks only cheese_consumed. A regression that changes sushi_consumed, poops_created, or another mapped counter can still pass. Snapshot all event-backed counters before and after the call.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_squid_statistics.py` around lines 122 - 128, Update
test_unknown_event_is_logged_without_mutating_counters to snapshot all
event-backed counters before calling increment("misspelled_event"), then assert
the complete counter state is unchanged afterward instead of checking only
cheese_consumed.