LocalStorage

[BayerC]

new test

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Consider passing a LocalStorage instance into load_or_create_session_id (or SessionState) rather than constructing it inside the function to make it easier to swap implementations or use alternative storage strategies without monkeypatching.
• The fake_browser_session_id fixture is autouse and globally patches load_or_create_session_id; if any tests need to exercise the real browser-backed behavior (e.g., integration tests), you may want to narrow its scope or provide an opt-out mechanism.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider passing a `LocalStorage` instance into `load_or_create_session_id` (or `SessionState`) rather than constructing it inside the function to make it easier to swap implementations or use alternative storage strategies without monkeypatching.
- The `fake_browser_session_id` fixture is `autouse` and globally patches `load_or_create_session_id`; if any tests need to exercise the real browser-backed behavior (e.g., integration tests), you may want to narrow its scope or provide an opt-out mechanism.

## Individual Comments

### Comment 1
<location path="src/open_cups/session_state.py" line_range="22-23" />
<code_context>
-            existing = st.query_params.get("session_id")
-            st.session_state.session_id = existing or str(uuid.uuid4())
-        st.query_params["session_id"] = st.session_state.session_id
+        if SESSION_ID_STORAGE_KEY not in st.session_state:
+            st.session_state.session_id = load_or_create_session_id()

     @property
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use the shared key constant consistently for `st.session_state` to avoid divergence.

You’re checking for `SESSION_ID_STORAGE_KEY` in `st.session_state` but assigning via the `session_id` attribute. If the constant changes, the check and assignment will diverge. Please write to `st.session_state[SESSION_ID_STORAGE_KEY] = load_or_create_session_id()` and have the `session_id` property read from that key so there’s a single source of truth.

Suggested implementation:

```python
    def __init__(self) -> None:
        if SESSION_ID_STORAGE_KEY not in st.session_state:
            st.session_state[SESSION_ID_STORAGE_KEY] = load_or_create_session_id()

    @property
    def session_id(self) -> str:
        return str(st.session_state[SESSION_ID_STORAGE_KEY])

```

You should also search the rest of the codebase for any direct uses of `st.session_state.session_id` and update them to use `st.session_state[SESSION_ID_STORAGE_KEY]` or this `session_id` property, to ensure everything consistently uses the shared key constant.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (bug_risk): Use the shared key constant consistently for st.session_state to avoid divergence.

You’re checking for SESSION_ID_STORAGE_KEY in st.session_state but assigning via the session_id attribute. If the constant changes, the check and assignment will diverge. Please write to st.session_state[SESSION_ID_STORAGE_KEY] = load_or_create_session_id() and have the session_id property read from that key so there’s a single source of truth.

Suggested implementation:

    def __init__(self) -> None:
        if SESSION_ID_STORAGE_KEY not in st.session_state:
            st.session_state[SESSION_ID_STORAGE_KEY] = load_or_create_session_id()

    @property
    def session_id(self) -> str:
        return str(st.session_state[SESSION_ID_STORAGE_KEY])

You should also search the rest of the codebase for any direct uses of st.session_state.session_id and update them to use st.session_state[SESSION_ID_STORAGE_KEY] or this session_id property, to ensure everything consistently uses the shared key constant.

[BayerC]

still doenst work!