Fix/remove current room artifact

[PythonFZ]

Summary by CodeRabbit

• Refactor

  • Improved per-user room isolation to provide dedicated connections for enhanced stability.
  • Enhanced session management on disconnect to ensure proper cleanup and prevent orphaned connections.
  • Streamlined room membership tracking for more reliable user state management.

• Tests

  • Updated test infrastructure for improved test reliability.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request refactors session and room management by removing the user's current room storage concept and transitioning to a per-user room architecture. Helper functions that tracked room-to-session mappings are eliminated, socket event handlers now use user-specific rooms, and disconnect cleanup retrieves room association from persisted session data instead of in-memory state. Tests are updated to reflect atomic room membership updates and visited rooms tracking.

Changes

Cohort / File(s)	Summary
Event handling refactor src/zndraw/app/events.py	Removed _get_len() and get_project_room_from_session() helper functions. Modified handle_connect() to join user-specific room ("user:<user_name>") instead of restoring previous room state. Updated handle_disconnect() to retrieve room_name from session data (SessionKeys.session_data) for cleanup operations, shifting from in-memory room lookup to persisted session information.
Service layer updates src/zndraw/services/client_service.py	Removed public method update_user_room(). Updated update_user_and_room_membership() to remove Redis hset operation persisting currentRoom, retaining only room membership addition and visited room tracking via pipeline. Updated class docstring to reflect room membership and visit tracking management.
Service tests tests/services/test_client_service.py	Removed tests for direct currentRoom updates, user hash creation, and per-user room updates. Replaced with tests validating atomic updates via pipeline and visited_rooms tracking. Updated assertions to verify room membership and visited room records instead of currentRoom field state.
Socket event tests tests/test_lock_socket_events.py	Replaced separate socket client (sio_client) with the connect_room fixture's persistent sio connection. Removed explicit connect/join calls and sleep-based synchronization. Updated authenticated_session fixture to yield the provided sio connection, with cleanup handled by the fixture rather than explicit disconnection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Fix/redis scan iter optimization #823: Modifies src/zndraw/app/events.py connect/disconnect handling with session_locks-based cleanup and structured logging, directly overlapping with this PR's socket event refactoring and session-based cleanup architecture.

Poem

🐰 Each user hops into their own cozy room,
No more rejoining yesterday's gloom,
Sessions guide us home with care so true,
Per-user isolation, fresh and new! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.42% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix/remove current room artifact' clearly and concisely summarizes the main change: removing the currentRoom storage mechanism and related code across multiple files (events.py, client_service.py, and tests).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

src/zndraw/app/events.py (1)

243-250: Consider defensive error handling for JSON parsing.

If session_data_raw somehow contains malformed JSON (e.g., due to data corruption), json.loads will raise JSONDecodeError, causing the disconnect handler to fail and potentially leaving resources uncleaned.

While the risk is low since this data is written internally via json.dumps, adding a try/except would make cleanup more resilient.

♻️ Suggested defensive handling

     if session_id:
         session_data_raw = r.get(SessionKeys.session_data(session_id))
         if session_data_raw:
-            session_data = json.loads(session_data_raw)
-            room_name = session_data.get("roomId")
+            try:
+                session_data = json.loads(session_data_raw)
+                room_name = session_data.get("roomId")
+            except json.JSONDecodeError:
+                log.warning(f"Failed to parse session data for {session_id}")

tests/test_lock_socket_events.py (1)

108-110: Unused variable user_name should be prefixed with underscore.

Static analysis correctly identifies that user_name is unpacked but never used in this test. Prefix with underscore to indicate it's intentionally unused.

♻️ Suggested fix

 def test_lock_release_emits_lock_update(authenticated_session):
     """Test that releasing a lock emits lock:update event with action=released."""
-    server, room, session_id, auth_headers, sio, user_name = authenticated_session
+    server, room, session_id, auth_headers, sio, _user_name = authenticated_session

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4047f7d and ea11962.

📒 Files selected for processing (4)

• src/zndraw/app/events.py
• src/zndraw/services/client_service.py
• tests/services/test_client_service.py
• tests/test_lock_socket_events.py

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: You MUST NEVER @pytest.mark.xfail or similar - all tests must pass!
All default values (e.g., camera, particles) must be defined exclusively within the Pydantic model. Do not scatter fallback logic throughout the codebase.
Do not perform null checks combined with hardcoded literals for default values. Rely entirely on the schema to populate default values during initialization.
You can not use LUA scripts with Redis!
If sensible, implement collections.abc interfaces for your classes, such as MutableMapping or MutableSequence.
Use numpy style docstrings in Python code.
Docstrings must be concise and to the point.
Use type hints wherever possible in Python. Use list[int|float] | None instead of t.Optional[t.List[int|float]]!
Imports should always be at the top of the file in Python.

Files:

• tests/services/test_client_service.py
• src/zndraw/services/client_service.py
• src/zndraw/app/events.py
• tests/test_lock_socket_events.py

**/test_*.py

📄 CodeRabbit inference engine (AGENTS.md)

When designing new tests, read the old tests first to understand the existing patterns. Use pytest.mark.parametrize to avoid code duplication. Tests should be very specific and test only one thing. Avoid complex test setups. Each test must be a function, not a method of a class!

Files:

• tests/services/test_client_service.py
• tests/test_lock_socket_events.py

🧬 Code graph analysis (3)

tests/services/test_client_service.py (2)

src/zndraw/services/client_service.py (2)

• update_user_and_room_membership (59-76)
• get_room_users (43-57)

src/zndraw/app/redis_keys.py (4)

• UserKeys (554-600)
• RoomKeys (242-550)
• users (304-306)
• visited_rooms (578-580)

src/zndraw/app/events.py (1)

src/zndraw/app/redis_keys.py (4)

• session_id (617-619)
• SessionKeys (604-669)
• session_data (622-637)
• UserKeys (554-600)

tests/test_lock_socket_events.py (3)

tests/conftest.py (2)

• connect_room (281-306)
• server (431-437)

src/zndraw/app/redis_keys.py (1)

• session_id (617-619)

tests/test_room_lock_enforcement.py (1)

• auth_headers (44-46)

🪛 Ruff (0.14.11)

tests/test_lock_socket_events.py

110-110: Unpacked variable user_name is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: pytest (3.13, ubuntu-latest)
• GitHub Check: pytest (3.12, ubuntu-latest)
• GitHub Check: pytest (3.11, ubuntu-latest)

🔇 Additional comments (9)

src/zndraw/app/events.py (1)

212-214: LGTM!

Clean refactor to join a user-specific room for user-targeted events (e.g., extension notifications). The naming convention user:{user_name} is clear and consistent.

src/zndraw/services/client_service.py (2)

59-76: LGTM!

Clean implementation of atomic room membership and visit tracking using Redis pipeline. The removal of currentRoom storage simplifies the data model while the visited_rooms set provides historical tracking.

---

16-21: LGTM!

Docstring accurately reflects the updated service responsibility.

tests/test_lock_socket_events.py (2)

10-21: LGTM!

Good refactor to leverage the connect_room fixture for cleaner test setup. The fixture now properly delegates cleanup, avoiding duplicate connection management code.

---

24-57: LGTM!

Test correctly uses the refactored fixture and validates all expected fields in the lock:update event.

tests/services/test_client_service.py (4)

120-128: LGTM!

Test correctly verifies both room membership and visit tracking, reflecting the new atomic behavior of update_user_and_room_membership.

---

152-170: LGTM!

Test correctly validates that users can join multiple rooms sequentially and that all visits are tracked. The new behavior where users remain in all room sets (rather than being moved) is properly tested.

---

290-304: LGTM!

Good improvement to test through the service API (update_user_and_room_membership) rather than directly manipulating Redis. This provides better coverage of the actual service behavior with special characters.

---

172-180: LGTM!

Idempotency test correctly verifies that duplicate room joins don't create duplicate entries due to Redis set semantics.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov-commenter]

Codecov Report

❌ Patch coverage is 65.51724% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.96%. Comparing base (aaeda38) to head (ea11962).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/zndraw/app/events.py	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   79.97%   79.96%   -0.01%     
==========================================
  Files         165      165              
  Lines       20108    20000     -108     
==========================================
- Hits        16081    15993      -88     
+ Misses       4027     4007      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.