Skip to content
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

Update the SDK #3196

Merged
merged 6 commits into from
Aug 27, 2024
Merged

Update the SDK #3196

merged 6 commits into from
Aug 27, 2024

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Aug 23, 2024

This PR updates the SDK to the latest version which with it brings the following code changes:

  • New send failure types that will be picked up as part of https://github.com/element-hq/element-internal/issues/614.
  • A new message shield/EncryptionAuthenticity state for users who were previously verified but have now reset their identity.
  • The dedicated event cache store (which will result in all media being deleted from the device).
    • We're storing this in appGroupContainer/Library/Caches/… using the new cachePath on the client builder and we're implementing this at the right time to not require a migration.

And the following SDK changes for free:

  • Various fixes under the hood for the pinned items timeline.
  • A bug fix for edits where their local echoes would show up as Not encrypted
  • Updates to how active calls are computed.

This is definitely one to review commit-by-commit.

Fixes #3202

@pixlwave pixlwave added the pr-misc for other changes label Aug 23, 2024
@pixlwave pixlwave requested a review from a team as a code owner August 23, 2024 19:27
Copy link

Warnings
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against 3b01f1b

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
8.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 72.80702% with 62 lines in your changes missing coverage. Please review.

Project coverage is 77.28%. Comparing base (993fe8c) to head (3b01f1b).
Report is 3 commits behind head on develop.

Files Patch % Lines
...ources/Services/UserSession/UserSessionStore.swift 0.00% 26 Missing ⚠️
...ervices/Authentication/AuthenticationService.swift 0.00% 10 Missing ⚠️
...X/Sources/Services/QRCode/QRCodeLoginService.swift 0.00% 10 Missing ⚠️
...rces/Services/UserSession/SessionDirectories.swift 75.00% 5 Missing ⚠️
...s/Screens/Timeline/View/Style/TimelineStyler.swift 25.00% 3 Missing ⚠️
...e/TimelineItemContent/EncryptionAuthenticity.swift 50.00% 3 Missing ⚠️
...X/Sources/Screens/Timeline/TimelineViewModel.swift 0.00% 2 Missing ⚠️
NSE/Sources/Other/NSEUserSession.swift 0.00% 2 Missing ⚠️
...Sources/Services/Keychain/KeychainController.swift 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3196      +/-   ##
===========================================
- Coverage    77.46%   77.28%   -0.18%     
===========================================
  Files          719      722       +3     
  Lines        57033    57223     +190     
===========================================
+ Hits         44178    44224      +46     
- Misses       12855    12999     +144     
Flag Coverage Δ
unittests 68.27% <72.80%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pixlwave pixlwave requested review from stefanceriu and removed request for a team August 27, 2024 07:41
Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Looks great! 👏
.. just wondering if we can leverage the new types better

case unknown
}

var isSendingFailed: Bool {
Copy link
Member

Choose a reason for hiding this comment

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

Want to rename this into hasSendingFailed so hard but I also get it 🎭

Comment on lines +158 to +162
if FileManager.default.directoryExists(at: sessionDirectories.dataDirectory) {
try? FileManager.default.removeItem(at: sessionDirectories.dataDirectory)
}
if FileManager.default.directoryExists(at: sessionDirectories.cacheDirectory) {
try? FileManager.default.removeItem(at: sessionDirectories.cacheDirectory)
Copy link
Member

Choose a reason for hiding this comment

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

SessionDirectories should maybe handle this on its own and be reused for the QRLoginService below.

let dataDirectory = try container.decodeIfPresent(URL.self, forKey: .sessionDirectory)
let cacheDirectory = try container.decodeIfPresent(URL.self, forKey: .cacheDirectory)

let sessionDirectories = if let dataDirectory {
Copy link
Member

Choose a reason for hiding this comment

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

Continuing from the comment above I wonder if we can or should make the SessionDirectories create itself from a container 🤔

@@ -158,22 +158,36 @@ class UserSessionStore: UserSessionStoreProtocol {
appSettings: appSettings)
}

private func deleteSessionDirectory(for credentials: KeychainCredentials) {
private func deleteSessionDirectories(for credentials: KeychainCredentials) {
Copy link
Member

Choose a reason for hiding this comment

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

Same question here I guess: can we move and hide a bunch of this logic into the new SessionDirectories?

UnitTests/Sources/RestorationTokenTests.swift Show resolved Hide resolved
@pixlwave
Copy link
Member Author

Discussed in a call that I'll do a follow-up PR that refactors this a bit more after we make the RC. We're happy with the serialisation of the RestorationToken so it won't be a breaking refactor.

@pixlwave pixlwave merged commit 812c5d5 into develop Aug 27, 2024
11 of 12 checks passed
@pixlwave pixlwave deleted the doug/update-sdk branch August 27, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-misc for other changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

warning shields appear on local echos of edits
2 participants