feat: [MM-69203] Implement collection, storage and attachment of session attributes - #171
Merged
Conversation
enahum
requested changes
Aug 5, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
enahum
approved these changes
Aug 11, 2026
| return null | ||
| } | ||
|
|
||
| private fun getSessionAttributesInterceptor(options: ReadableMap?): SessionAttributesInterceptor? { |
devinbinnie
marked this pull request as ready for review
August 11, 2026 12:26
📝 WalkthroughWalkthroughThis change adds session-attribute APIs, native persistence, device and network collection, TTL-based header generation, and authenticated request integration for Android and iOS. React Native exposes configuration methods and header retrieval. ChangesSession attributes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReactNative
participant ApiClientWrapper
participant SessionAttributesEngine
participant SessionAttributesStore
participant SessionAttributesCollector
participant NetworkClient
ReactNative->>ApiClientWrapper: Configure server manifest and fields
ApiClientWrapper->>SessionAttributesEngine: Store session-attribute configuration
SessionAttributesEngine->>SessionAttributesStore: Persist server state
NetworkClient->>SessionAttributesEngine: Request outbound header
SessionAttributesEngine->>SessionAttributesStore: Load enabled state and TTL timestamps
SessionAttributesEngine->>SessionAttributesCollector: Collect eligible values
SessionAttributesCollector-->>SessionAttributesEngine: Return attribute values
SessionAttributesEngine-->>NetworkClient: Return Base64-encoded header
NetworkClient-->>ReactNative: Send authenticated request with session header
``
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 4 | ❌ 1</summary>
### ❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
| :----------------: | :--------- | :----------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------- |
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 5.22% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
<details>
<summary>✅ Passed checks (4 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :------------------------------------------------------------------------------------------------------------ |
| Title check | ✅ Passed | The title clearly summarizes the implementation of session-attribute collection, storage, and attachment. |
| Description check | ✅ Passed | The description directly relates to the session-attributes implementation and includes the associated ticket. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches 💡 1</summary>
<!-- finishing_touch_suggestion:docstrings -->
<details>
<summary>📝 Generate docstrings 💡</summary>
- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> Create stacked PR
- [ ] <!-- {"checkboxId": "3e1879ae-f29b-4d0d-8e06-d12b7ba33d98"} --> Commit on current branch
</details>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `MM-69203`
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- This is an auto-generated comment: all tool run failures by coderabbit.ai -->
> [!WARNING]
> There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
>
> <details>
> <summary>🔧 ESLint</summary>
>
> > If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
>
> ESLint install failed: dependency version conflict. Check your lock file or package.json.
>
>
>
> </details>
<!-- end of auto-generated comment: all tool run failures by coderabbit.ai -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
ios/SessionAttributes/SessionAttributesCollector.swift (2)
28-55: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffThe
NWPathMonitorruns for the process lifetime, even when no server enables session attributes.
SessionAttributesCollector.sharedstartspathMonitorininit. The singleton is created the first time any code touchesSessionAttributesEngine.shared, andSessionAttributesEngineholdscollectoras a stored property. The monitor then runs continuously, and each Wi-Fi path update triggersNEHotspotNetwork.fetchCurrent, which can present a location-permission requirement.Start the monitor lazily, when the first server enables session attributes, and stop it when no server has the feature enabled.
🤖 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 `@ios/SessionAttributes/SessionAttributesCollector.swift` around lines 28 - 55, Update SessionAttributesCollector initialization so it does not start pathMonitor automatically. Add lifecycle methods to start the monitor when the first server enables session attributes and cancel it when the last enabled server is removed, ensuring SessionAttributesEngine or SessionAttributesCollector only manages monitoring while the feature is in use.
138-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable pre-iOS 14 SSID path.
The podspec and example app target iOS 15.1. Remove
resolveLegacySsid()and the unusedSystemConfiguration.CaptiveNetworkimport.🤖 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 `@ios/SessionAttributes/SessionAttributesCollector.swift` around lines 138 - 159, Remove the pre-iOS 14 fallback from the SSID collection method, including the `resolveLegacySsid()` call and its helper implementation. Delete the now-unused `SystemConfiguration.CaptiveNetwork` import, while preserving the iOS 14+ `NEHotspotNetwork.fetchCurrent` behavior.android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesCollector.kt (1)
44-53: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
getServerFqdncaches the empty failure result permanently.
computeIfAbsentstores the mapping function result, including"". After one parse failure, the cache returns""for that server for the process lifetime, and no retry occurs.Uri.parseis deterministic, so a retry rarely changes the outcome, but the cache also never bounds its size across server URLs.Skip caching the empty result.
♻️ Proposed change
private fun getServerFqdn(serverUrl: String): String { - return fqdnCache.computeIfAbsent(serverUrl) { url -> - try { - Uri.parse(url).host ?: "" - } catch (e: Exception) { - Log.w("NetworkClient", "Failed to resolve server FQDN: ${e.message}") - "" - } - } + fqdnCache[serverUrl]?.let { return it } + val host = try { + Uri.parse(serverUrl).host ?: "" + } catch (e: Exception) { + Log.w("NetworkClient", "Failed to resolve server FQDN: ${e.message}") + "" + } + if (host.isNotEmpty()) { + fqdnCache[serverUrl] = host + } + return host }🤖 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 `@android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesCollector.kt` around lines 44 - 53, Update getServerFqdn so only non-empty parsed host values are stored in fqdnCache; parse failures and empty hosts must return "" without creating a cache entry, allowing subsequent calls to retry while preserving the existing warning behavior.ios/SessionAttributes/SessionAttributesStore.swift (1)
95-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
upsertFieldandremoveFieldfail silently when no state exists.Both methods return without action when
readStatereturnsnilor whenstate.enabledisfalse. The JS caller receives no signal. A caller that invokesupsertSessionAttributesFieldbeforesetSessionAttributesEnabledorsetSessionAttributesManifestsees the field silently dropped.The methods are synchronous and return
void, so an error return is not available. Add a log statement so the drop is observable during debugging.Also applies to: 110-119
🤖 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 `@ios/SessionAttributes/SessionAttributesStore.swift` around lines 95 - 108, Update upsertField and removeField to log when readState(for:) returns nil or the retrieved state is disabled before returning, while preserving their synchronous void behavior and existing state-update logic. Include the serverUrl and field identifier where available so silently dropped operations are observable during debugging.test-runner/src/test/kotlin/com/mattermost/networkclient/SessionAttributesTest.kt (1)
28-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the header payload construction.
The inline reproductions are intentional for the pure-JVM runner. Based on learnings, the
test-runnermodule must not reference production classes that transitively depend oncom.facebook.react.bridge.The current tests cover the TTL predicate, the transport mapping, the interceptor guard, and the cache. Three behaviours from
SessionAttributesEngine.getOutboundHeaderhave no coverage:
- A field whose collected value is empty is skipped and its
lastSentAtis not updated.- An empty payload produces no header at all.
- The payload is JSON, then base64 encoded.
Add inline reproductions of those three rules so a regression in the production ordering is visible.
Based on learnings: in the
test-runnermodule (pure JVM/Kotlin, with no Android SDK and no React Native dependencies), it is intentional to duplicate the relevant logic so the test can validate behaviour in isolation.🤖 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 `@test-runner/src/test/kotlin/com/mattermost/networkclient/SessionAttributesTest.kt` around lines 28 - 45, Add pure-JVM test coverage alongside the existing helpers shouldSend and interfaceType by reproducing getOutboundHeader’s relevant behavior: skip fields with empty collected values without updating lastSentAt, return no header for an entirely empty payload, and serialize non-empty payloads as JSON before Base64 encoding. Keep the reproduction inline and independent of production classes or React Native dependencies.Source: Learnings
android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesStore.kt (1)
197-216: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
setStableValuescan leave the cache and the persisted value out of order.
setStableValuesassignsstableValuesundercacheLockand then callspersistunderpersistLock. The two locks are independent. If two threads callsetStableValuesconcurrently, thread A can win the cache assignment while thread B wins the persist order. The in-memory value then differs from the stored value, and the difference survives a process restart.Hold
persistLockfor the cache assignment and the write together, or build the JSON and update the cache inside the same critical section that orders the write.♻️ Proposed fix to order the cache update with the write
fun setStableValues(values: Map<String, String>) { val json = JSONObject() values.forEach { (key, value) -> json.put(key, value) } - synchronized(cacheLock) { - stableValues = values.toMap() - } - persist(SessionAttributesConstants.STABLE_VALUES_ALIAS, json.toString()) + synchronized(persistLock) { + synchronized(cacheLock) { + stableValues = values.toMap() + } + persistLocked(SessionAttributesConstants.STABLE_VALUES_ALIAS, json.toString()) + } }Extract the body of
persistinto apersistLockedhelper that assumes the caller holdspersistLock, and keeppersistas the wrapper that takes the lock.🤖 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 `@android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesStore.kt` around lines 197 - 216, Update setStableValues so cache assignment and persistence are ordered under the same persistLock critical section. Extract persist’s write logic into a persistLocked helper that assumes persistLock is held, retain persist as the locking wrapper, and invoke persistLocked after updating stableValues while holding persistLock.
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesCollector.kt`:
- Around line 108-126: Update resolveIpAddress to skip interfaces that are down
or virtual before inspecting their addresses, and retain only non-loopback IPv4
candidates. Align the selection with the iOS collector’s physical-interface
behavior where possible, while preserving the existing empty-string fallback and
warning handling.
- Around line 87-106: Update currentNetworkSnapshot to use a safe
ConnectivityManager lookup and guard the network inspection and address/SSID
collection so any failure returns an empty NetworkSnapshot without propagating
into the interceptor. Apply the same failure-safe handling to isMdmEnrolled,
including its system-service and applicationRestrictions access, while
preserving normal results when collection succeeds.
- Around line 29-42: Update SessionAttributesEngine.getOutboundHeader to compute
currentNetworkSnapshot once and pass it into SessionAttributesCollector.collect
for each manifest field. Change collect to accept and reuse that snapshot only
for network attributes, while avoiding snapshot creation for stable values,
MDM_ENROLLED, SERVER_FQDN, and unknown names.
In
`@android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesEngine.kt`:
- Around line 77-115: Make the read, TTL decision, timestamp update, and
persistence in getOutboundHeader atomic per serverUrl, using synchronization or
a store-side operation protected by cacheLock. Prefer a SessionAttributesStore
API such as claimDueFields that atomically identifies due fields without
replacing manifest, then collect values and stamp only fields with non-empty
values; ensure concurrent requests cannot emit the same field within its TTL or
overwrite manifest changes.
In `@ios/SessionAttributes/SessionAttributesCollector.swift`:
- Around line 197-226: Update resolveIpAddress to return the first valid
non-loopback IPv4 address from the interface list instead of overwriting it with
later en0/en1 matches; include other active interfaces as needed to match the
Android client_ip_address behavior, and confirm or preserve the intended iOS
platform scope if broader interface support is not acceptable.
- Around line 197-226: The client_ip_address collectors use inconsistent
interface-selection rules. In
ios/SessionAttributes/SessionAttributesCollector.swift:197-226, define the
shared accepted-interface rule, select the first matching non-zero IPv4 address,
and stop scanning after it is found instead of retaining the last match; in
android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesCollector.kt:108-126,
apply that same interface filter to the existing first-address lookup, excluding
other, VPN, and virtual interfaces.
- Line 20: Synchronize all reads and writes to fqdnCache within
SessionAttributesCollector.getServerFqdn using the existing monitorQueue. Ensure
cache lookup, updates at both write sites, and the returned value execute within
the same serialized access path, preserving the current FQDN resolution
behavior.
- Around line 208-218: Update the IPv4 address handling in the interface
collection logic to reinterpret ifa_addr as sockaddr_in and pass its sin_addr to
inet_ntop instead of the sockaddr header. Preserve the existing en0/en1
filtering and non-zero IP assignment behavior.
In `@ios/SessionAttributes/SessionAttributesEngine.swift`:
- Around line 56-94: Make the read-compute-write sequence in getOutboundHeader
atomic by adding and using a serialized mutateState helper on
SessionAttributesStore. Build the payload and update lastSentAt inside the
mutation closure, persist the state only when a payload is produced, and
preserve the existing nil behavior for disabled, empty, or unchanged state while
ensuring concurrent requests cannot lose timestamp updates.
- Around line 111-119: Update getOutboundHeader to apply the parsed
grace_period_seconds value from SAField when determining outbound session
behavior, preserving the TypeScript contract’s grace-period semantics;
alternatively, remove grace_period_seconds consistently from the cross-platform
contract and related parser/model usage.
In `@ios/SessionAttributes/SessionAttributesStore.swift`:
- Around line 26-29: Replace the UserDefaults-backed persistence in
SessionAttributesStore, including the app-group selection in init(), with
encrypted storage such as the existing iOS Keychain or encrypted-storage helper.
Ensure stable values and session attributes including client_ip_address, ssid,
and client_device_id are written to and read from that encrypted backend,
preserving the current store API and app-group behavior where applicable.
- Around line 31-35: Update serverKey(_:) to canonicalize the URL consistently
before hashing: lowercase only its scheme and host while preserving the path’s
case, then apply the same normalization for both public state updates and
request lookups using their URL string sources. Keep the resulting SHA256 key
generation unchanged.
In `@src/APIClient/NativeApiClient.ts`:
- Around line 170-176: The synchronous getSessionAttributesHeader API must
return the codegen-supported nullable type string | null and must not perform
storage, network, device-attribute collection, or synchronous queue work on the
JS thread. Update the native getter and its implementation to return precomputed
or cached data (or provide an asynchronous path), while preserving any public
wrapper conversion from null to undefined.
---
Nitpick comments:
In
`@android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesCollector.kt`:
- Around line 44-53: Update getServerFqdn so only non-empty parsed host values
are stored in fqdnCache; parse failures and empty hosts must return "" without
creating a cache entry, allowing subsequent calls to retry while preserving the
existing warning behavior.
In
`@android/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesStore.kt`:
- Around line 197-216: Update setStableValues so cache assignment and
persistence are ordered under the same persistLock critical section. Extract
persist’s write logic into a persistLocked helper that assumes persistLock is
held, retain persist as the locking wrapper, and invoke persistLocked after
updating stableValues while holding persistLock.
In `@ios/SessionAttributes/SessionAttributesCollector.swift`:
- Around line 28-55: Update SessionAttributesCollector initialization so it does
not start pathMonitor automatically. Add lifecycle methods to start the monitor
when the first server enables session attributes and cancel it when the last
enabled server is removed, ensuring SessionAttributesEngine or
SessionAttributesCollector only manages monitoring while the feature is in use.
- Around line 138-159: Remove the pre-iOS 14 fallback from the SSID collection
method, including the `resolveLegacySsid()` call and its helper implementation.
Delete the now-unused `SystemConfiguration.CaptiveNetwork` import, while
preserving the iOS 14+ `NEHotspotNetwork.fetchCurrent` behavior.
In `@ios/SessionAttributes/SessionAttributesStore.swift`:
- Around line 95-108: Update upsertField and removeField to log when
readState(for:) returns nil or the retrieved state is disabled before returning,
while preserving their synchronous void behavior and existing state-update
logic. Include the serverUrl and field identifier where available so silently
dropped operations are observable during debugging.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/SessionAttributesTest.kt`:
- Around line 28-45: Add pure-JVM test coverage alongside the existing helpers
shouldSend and interfaceType by reproducing getOutboundHeader’s relevant
behavior: skip fields with empty collected values without updating lastSentAt,
return no header for an entirely empty payload, and serialize non-empty payloads
as JSON before Base64 encoding. Keep the reproduction inline and independent of
production classes or React Native dependencies.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b0372f0-b4cb-410f-b8c3-9cdec683b7c6
📒 Files selected for processing (26)
android/src/main/java/com/mattermost/networkclient/ApiClientModuleImpl.ktandroid/src/main/java/com/mattermost/networkclient/NetworkClient.ktandroid/src/main/java/com/mattermost/networkclient/interceptors/SessionAttributesInterceptor.ktandroid/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributes.ktandroid/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesCollector.ktandroid/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesConstants.ktandroid/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesEngine.ktandroid/src/main/java/com/mattermost/networkclient/sessionattributes/SessionAttributesStore.ktandroid/src/newarch/java/com/ApiClientModule.ktandroid/src/oldarch/java/com/ApiClientModule.ktios/Adapters/SessionAttributesAdapter.swiftios/ApiClient/ApiClient.mmios/ApiClient/ApiClientWrapper.swiftios/NetworkClient.swiftios/SessionAttributes/SessionAttributes.swiftios/SessionAttributes/SessionAttributesCollector.swiftios/SessionAttributes/SessionAttributesConstants.swiftios/SessionAttributes/SessionAttributesEngine.swiftios/SessionAttributes/SessionAttributesStore.swiftreact-native-network-client.podspecsrc/APIClient/NativeApiClient.tssrc/SessionAttributes/index.tsxsrc/index.tsxsrc/schemas.tsxsrc/types/APIClient.tstest-runner/src/test/kotlin/com/mattermost/networkclient/SessionAttributesTest.kt
larkox
approved these changes
Aug 11, 2026
Member
Author
|
@coderabbitai resolve all and approve. this has been reviewed already. |
|
✅ Action performedComments resolved and changes approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DRAFT: Implement collection, storage and attachment of session attributes
Ticket Link
https://mattermost.atlassian.net/browse/MM-69203