Bug fix: Atom effect receives context with unintended scope #179
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the atom effect received an unintended context by ensuring that updates and side‐effects always use the initialized scope context. Key changes include:
- Adding a new test (testEffectCrossScopeBoundary) under a compiler condition to exercise cross‐scope behavior.
- Updating StoreContext functions to call switchContext with the atom’s initialized scope during state updates, refresh, reset, and transactional operations.
- Renaming a test function from testTransitiveUpdate to testUpdatePropagation and updating associated comments for clarity.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/AtomsTests/Core/StoreContextTests.swift | Added a test for cross-scope behavior and renamed a test function. |
| Sources/Atoms/Core/StoreContext.swift | Revised update, refresh, reset, and release implementations to use switchContext with the correct scope. |
| Sources/Atoms/AtomScope.swift | Updated documentation to clarify that only atoms initialized in the current scope are observed. |
Comments suppressed due to low confidence (1)
Tests/AtomsTests/Core/StoreContextTests.swift:1295
- [nitpick] Consider renaming the local helper function 'assert' to a more descriptive name (e.g. 'verifyAtomState') to avoid confusion with XCTest's assertions.
func assert<Node: Atom>(
| @@ -640,4 +643,12 @@ private extension StoreContext { | |||
| observer.onUpdate(snapshot) | |||
| } | |||
| } | |||
|
|
|||
| func switchContext(scope: Scope?) -> StoreContext { | |||
There was a problem hiding this comment.
Consider adding a doc comment to switchContext to explain its purpose, how it constructs a new context with the provided scope, and its role in ensuring the correct update propagation.
Copilot uses AI. Check for mistakes.
| @@ -306,22 +317,27 @@ private extension StoreContext { | |||
| updatedScopes[currentScope.key] = currentScope | |||
| } | |||
|
|
|||
| func transitiveUpdate(for key: AtomKey, cache: some AtomCacheProtocol) { | |||
| func updatePropagation(for key: AtomKey, cache: some AtomCacheProtocol) { | |||
There was a problem hiding this comment.
Review the inline comment 'Overridden atoms don't get updated transitively.' and update it to reflect the new terminology ('propagation') to improve clarity and consistency.
Copilot uses AI. Check for mistakes.
Pull Request Type
Issue for this PR
Link: #178