Skip to content

Conversation

@MohamadJaara
Copy link
Member

@MohamadJaara MohamadJaara commented Feb 12, 2026

https://wearezeta.atlassian.net/browse/WPB-23463

PR Submission Checklist for internal contributors

  • The PR Title
    • conforms to the style of semantic commit messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³

What's new in this PR?

Issues

  • Read receipts and typing indicator were not consistently synced during fresh login (slow sync).
  • Screenshot censoring setting was not included in slow-sync property sync.
  • Screenshot censoring toggle was persisted only locally and did not trigger backend property API updates.

Solutions

  • Added a dedicated user properties sync step in slow sync flow.
  • Added screenshot censoring user property key and synced it during slow sync.
  • Switched user-properties slow-sync fetch to a single call to GET /properties-values.
  • Updated parsing to handle the actual payload shape:
    • { "WIRE_TYPING_INDICATOR_MODE": ..., ... }
  • Updated screenshot censoring toggle persistence to use UserPropertyRepository backend property APIs:
    • enable -> setProperty(WIRE_SCREENSHOT_CENSORING_MODE, 1)
    • disable -> deleteProperty(WIRE_SCREENSHOT_CENSORING_MODE)
  • Added/updated unit tests for:
    • slow-sync user properties step behavior
    • properties API request/response handling
    • repository sync logic
    • screenshot toggle backend write behavior

Needs releases with:

  • GitHub link to other pull request

Testing

How to Test

  • Functional verification:
    1. Fresh login/slow sync: verify one request to GET /properties-values and local update of:
      • read receipts
      • typing indicator
      • screenshot censoring
    2. Toggle screenshot censoring ON: verify backend property set call.
    3. Toggle screenshot censoring OFF: verify backend property delete call.

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 12, 2026

Test Results

0 tests   - 4 347   0 ✅  - 4 230   0s ⏱️ - 5m 1s
0 suites  -   726   0 💤  -   117 
0 files    -   726   0 ❌ ±    0 

Results for commit e7adc3a. ± Comparison against base commit 9e0501a.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 12, 2026

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2026

Codecov Report

❌ Patch coverage is 80.18018% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.77%. Comparing base (9e0501a) to head (e7adc3a).

Files with missing lines Patch % Lines
...um/logic/data/properties/UserPropertyRepository.kt 77.27% 7 Missing and 13 partials ⚠️
...nsoring/PersistScreenshotCensoringConfigUseCase.kt 87.50% 0 Missing and 1 partial ⚠️
.../com/wire/kalium/logic/sync/slow/SlowSyncWorker.kt 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3862      +/-   ##
===========================================
+ Coverage    59.71%   59.77%   +0.05%     
===========================================
  Files         1902     1903       +1     
  Lines        59535    59637     +102     
  Branches      6463     6489      +26     
===========================================
+ Hits         35550    35646      +96     
+ Misses       21013    21003      -10     
- Partials      2972     2988      +16     
Files with missing lines Coverage Δ
...etwork/api/authenticated/properties/PropertyKey.kt 100.00% <100.00%> (ø)
...um/network/api/v0/authenticated/PropertiesApiV0.kt 93.75% <100.00%> (+93.75%) ⬆️
.../com/wire/kalium/logic/data/sync/SlowSyncStatus.kt 100.00% <100.00%> (ø)
...um/logic/feature/user/SyncUserPropertiesUseCase.kt 100.00% <100.00%> (ø)
...nsoring/PersistScreenshotCensoringConfigUseCase.kt 91.66% <87.50%> (-8.34%) ⬇️
.../com/wire/kalium/logic/sync/slow/SlowSyncWorker.kt 72.60% <83.33%> (+0.96%) ⬆️
...um/logic/data/properties/UserPropertyRepository.kt 68.90% <77.27%> (+23.45%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e0501a...e7adc3a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MohamadJaara MohamadJaara force-pushed the mo/feat/sync--read-receipts-and-typing-indicator branch from 69ee398 to 6356464 Compare February 12, 2026 13:24
@MohamadJaara MohamadJaara changed the title feat: implement user properties synchronization in slow sync feat: implement user properties synchronization in slow sync [WPB-23463] Feb 12, 2026
…eceipts, typing indicator, and screenshot censoring
@MohamadJaara MohamadJaara force-pushed the mo/feat/sync--read-receipts-and-typing-indicator branch from f840d16 to e7adc3a Compare February 12, 2026 14:52
@sonarqubecloud
Copy link

@MohamadJaara
Copy link
Member Author

@claude review

@github-actions
Copy link
Contributor

github-actions bot commented Feb 13, 2026

Pull Request Review: User Properties Synchronization in Slow Sync

Status: ✅ Approved with minor suggestions


Summary

This PR implements a comprehensive solution for syncing user properties (read receipts, typing indicator, screenshot censoring) during the slow sync flow. The implementation is well-structured with proper error handling, good test coverage, and follows the project's architectural patterns.


Strengths

1. Architecture & Design

  • Interface segregation: Breaking down UserPropertyRepository into focused interfaces (ReadReceiptsPropertyRepository, TypingIndicatorPropertyRepository, etc.) is excellent. This follows the Interface Segregation Principle and makes the code more maintainable.
  • Delegation pattern: Using Kotlin's delegation (by) to compose the implementation is clean and reduces boilerplate.
  • Separation of concerns: Each property type has its own data source class, making the code modular and testable.

2. Robust Error Handling

  • 404 handling with defaults: The flatMapLeft pattern to handle missing properties (404) and apply sensible defaults is well-implemented.
    • Read receipts: defaults to disabled (0)
    • Typing indicator: defaults to enabled (1)
    • Screenshot censoring: defaults to disabled (0)
  • Non-blocking sync in slow sync: The user properties sync step intentionally ignores failures and continues with .flatMapLeft { Unit.right() } (line 722 in SlowSyncWorker.kt). This is the right approach for a non-critical sync step.
  • Fallback mechanism: When the bulk endpoint (/properties-values) returns 404, it falls back to individual property calls. Smart compatibility handling.

3. API Design

  • Bulk fetch optimization: Using a single GET /properties-values call instead of multiple individual calls significantly reduces network overhead during slow sync.
  • Flexible JSON parsing: The toIntOrNull() extension handles both numeric and string JSON values, which provides good resilience against API variations.

4. Test Coverage

  • Comprehensive testing: 385 new lines in UserPropertyRepositoryTest.kt covering:
    • Bulk fetch success/failure scenarios
    • Individual property fetch with 404 handling
    • Fallback chain behavior
    • String vs numeric JSON value parsing
    • All three property types
  • Network layer tests: PropertiesApiV0Test.kt verifies HTTP request configuration properly.
  • Use case tests: SyncUserPropertiesUseCaseTest.kt and updated SlowSyncWorkerTest.kt ensure integration works correctly.

Issues & Suggestions

1. Minor: Inconsistent Property Semantics 🟡

The typing indicator has inverted semantics compared to read receipts and screenshot censoring:

  • Read receipts: Set=enabled (1), Delete=disabled
  • Screenshot censoring: Set=enabled (1), Delete=disabled
  • Typing indicator: Delete=enabled, Set=disabled (0) ⚠️

Location:

  • UserPropertyRepository.kt:82-91 (setTypingIndicatorEnabled/removeTypingIndicatorProperty)

While this is likely intentional (backend quirk), it could be confusing for future maintainers. Consider adding a comment explaining why typing indicator works differently.

2. Potential Bug: Fallback Chain Behavior 🔴

Location: UserPropertyRepository.kt:493-511

In syncUsingFallbackCalls(), if the first property fails with a non-404 error, the entire chain stops due to flatMap chaining. However, this might not be the desired behavior during slow sync where we want to continue despite individual failures.

Current behavior:

syncReadReceipts()
    .flatMap { syncTypingIndicator() }  // Won't run if syncReadReceipts fails
    .flatMap { syncScreenshotCensoring() }  // Won't run if either above fails

Question: Should individual property sync failures stop the entire fallback chain, or should each property be attempted independently?

Suggested fix (if independent syncing is desired):

private suspend fun syncUsingFallbackCalls(): Either<CoreFailure, Unit> {
    // Attempt all syncs independently and combine results
    val results = listOf(
        syncReadReceipts(),
        syncTypingIndicator(), 
        syncScreenshotCensoring()
    )
    
    // Return first failure or success if all succeeded
    return results.firstOrNull { it.isLeft() } ?: Either.Right(Unit)
}

However, since the slow sync worker already ignores failures from the entire user properties sync step, this may be acceptable as-is. Just wanted to raise it for consideration.

3. Code Quality: Magic Numbers 🟡

Location: Multiple files

The property value encoding (0=disabled, 1=enabled) is scattered throughout without constants:

  • UserPropertyRepository.kt:384-486 (multiple occurrences)
  • PersistScreenshotCensoringConfigUseCase.kt:664-665

Suggestion: Define constants for clarity:

private const val PROPERTY_ENABLED = 1
private const val PROPERTY_DISABLED = 0

4. Documentation: Missing KDoc 🟡

The new interfaces and functions lack KDoc comments explaining:

  • When to use syncPropertiesStatuses() vs individual sync methods
  • The fallback behavior when the bulk endpoint is unavailable
  • Default values when properties are not found (404)

Suggestion: Add KDoc to at least the public repository interfaces.


Security Considerations

No security issues identified

  • Screenshot censoring now properly syncs with backend, preventing privacy leaks
  • No injection vulnerabilities (using property enums, not raw strings)
  • No sensitive data logged in error messages

Performance Considerations

Performance improved

  • Before: 3 separate API calls during slow sync (read receipts, typing indicator, + missing screenshot censoring)
  • After: 1 bulk API call with fallback to 3 individual calls only if bulk endpoint unavailable
  • Impact: Reduced slow sync latency and network overhead

Testing Recommendations

Before merging, verify:

  1. ✅ Fresh login with all properties set on backend
  2. ✅ Fresh login with no properties set (all defaults)
  3. ✅ Fresh login with only some properties set
  4. ✅ Toggle screenshot censoring and verify backend API calls (set/delete)
  5. ✅ Old backend compatibility (bulk endpoint returns 404) → should fall back to individual calls
  6. ✅ Property values returned as strings vs numbers (both should work)

Detailed Code Review Notes

UserPropertyRepository.kt

Lines 318-367: Interface segregation pattern - well done! ✅

Lines 474-491: The bulk properties sync logic is clean, but the default value logic could be clearer:

val typingIndicatorEnabled = properties.findIntValue(PropertyKey.WIRE_TYPING_INDICATOR_MODE)?.let { it != 0 } ?: true

This is correct (typing indicator defaults to enabled=true) but dense. Consider extracting to a named function for readability.

Lines 526-540: Helper functions are well-implemented. The isPropertyNotFound() extension is particularly good for encapsulating the error checking logic.

SlowSyncWorker.kt

Lines 716-722: The user properties sync step implementation is correct:

.continueWithStep(SlowSyncStep.USER_PROPERTIES) {
    syncUserProperties.invoke()
        .onFailure {
            kaliumLogger.e("Failure during User Properties Sync $it")
        }.flatMapLeft { Unit.right() }
}

The failure is logged but doesn't stop slow sync. This is appropriate for a non-critical sync step. ✅

PersistScreenshotCensoringConfigUseCase.kt

Lines 662-675: Good change from local-only to backend sync:

  • Enabling now calls setProperty(WIRE_SCREENSHOT_CENSORING_MODE, 1)
  • Disabling now calls deleteProperty(WIRE_SCREENSHOT_CENSORING_MODE)

This ensures screenshot censoring is properly synced across devices. ✅

Test Files

UserPropertyRepositoryTest.kt: Excellent coverage! Tests for:

  • ✅ Bulk fetch with all properties present
  • ✅ Bulk fetch with missing properties (defaults)
  • ✅ String vs numeric JSON values
  • ✅ Fallback to individual calls when bulk endpoint unavailable
  • ✅ Error propagation

SlowSyncWorkerTest.kt: The new test givenSyncUserPropertiesFails_whenPerformingSlowSync_thenContinueWithNextSteps properly verifies that sync continues despite user properties failure. ✅


Conclusion

This is a well-implemented feature with proper architecture, error handling, and test coverage. The code follows the project's conventions (Either pattern, repository pattern, use cases, etc.) and integrates cleanly into the existing slow sync flow.

The minor issues noted are mostly about code clarity and documentation rather than functional problems. The implementation is solid and ready for merge.

Recommendation:Approve (with optional follow-up for the suggestions above)


Reviewed by: Claude Sonnet 4.5
Review Date: 2026-02-13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants