Skip to content

Allow keeping Atoms alive within a scope until it's deallocation #188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 3, 2025

Conversation

ra1028
Copy link
Owner

@ra1028 ra1028 commented May 29, 2025

Pull Request Type

  • Bug fix
  • New feature
  • Refactoring
  • Documentation update
  • Chore

Issue for this PR

Link: #165

@ra1028 ra1028 marked this pull request as ready for review June 3, 2025 08:59
@Copilot Copilot AI review requested due to automatic review settings June 3, 2025 08:59
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for retaining atoms for the lifetime of a view scope, introducing new scope tracking structures and updating cache/subscription management to use them.

  • Replace initializedScope with scopeValues in AtomCache and adapt initializer calls.
  • Add ScopeValues, ScopeState, and scopes in StoreState to track active scopes and unregister atoms when scopes deallocate.
  • Update StoreContext to register/unregister scopes, use state.subscribed instead of the old graph field for subscriptions, and adjust related logic.
  • Revise tests and documentation to reflect the new scope-based keep-alive behavior.

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Tests/AtomsTests/Utilities/Utilities.swift Renamed initializer argument from initializedScope to scopeValues.
Tests/AtomsTests/Core/StoreContextTests.swift Adjusted assertions to use store.state.subscribed.
Tests/AtomsTests/Core/ScopeKeyTests.swift Added @MainActor to testDescription.
Tests/AtomsTests/Core/AtomCacheTests.swift Updated test to use scopeValues in AtomCache initializer.
Tests/AtomsTests/Attribute/KeepAliveTests.swift Expanded KeepAlive tests to cover scoped and overridden scopes.
Sources/Atoms/Core/StoreState.swift Added subscribed and scopes dictionaries.
Sources/Atoms/Core/StoreContext.swift Refactored to use ScopeValues, register/unregister scopes, and switch to state.subscribed.
Sources/Atoms/Core/ScopeValues.swift Introduced ScopeValues to capture scope parameters.
Sources/Atoms/Core/ScopeState.swift Added ScopeState class to automatically unregister on deinit.
Sources/Atoms/Core/ScopeKey.swift Made Token actor-isolated with @MainActor.
Sources/Atoms/Core/Scope.swift Reduced Scope to only track atom keys.
Sources/Atoms/Core/Graph.swift Removed deprecated subscribed field.
Sources/Atoms/Core/AtomKey.swift Exposed scopeKey to public, removed isScoped.
Sources/Atoms/Core/AtomCache.swift Replaced initializedScope with scopeValues, added shouldKeepAlive.
Sources/Atoms/Attribute/KeepAlive.swift Clarified documentation for scoped/override retention behavior.
Sources/Atoms/AtomScope.swift Updated AtomScope to register new scopes via ScopeState.
README.md Added scoped retention note under KeepAlive.

@Environment(\.store)
private var environmentStore
private var store
Copy link
Preview

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store property is missing its @Environment(\.store) property wrapper, so it will never be injected and will always be nil. It should be declared as @Environment(\.store) private var store.

Copilot uses AI. Check for mistakes.

Comment on lines +183 to +185
scopedStore?.registerScope(state: state.scopeState)

return content.environment(\.store, scopedStore)
Copy link
Preview

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Registering a scope via side effects directly in the SwiftUI View body can lead to unpredictable behaviors. Consider moving registerScope to an onAppear modifier or the view initializer to avoid side effects during view recomputation.

Suggested change
scopedStore?.registerScope(state: state.scopeState)
return content.environment(\.store, scopedStore)
return content
.environment(\.store, scopedStore)
.onAppear {
scopedStore?.registerScope(state: state.scopeState)
}

Copilot uses AI. Check for mistakes.

@ra1028 ra1028 merged commit 002f10d into main Jun 3, 2025
6 checks passed
@ra1028 ra1028 deleted the scoped-keep-alive branch June 3, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant