Skip to content

Ensure basalRateSchedule card is updated#555

Closed
motinis wants to merge 1 commit into
LoopKit:devfrom
motinis:issue-2267
Closed

Ensure basalRateSchedule card is updated#555
motinis wants to merge 1 commit into
LoopKit:devfrom
motinis:issue-2267

Conversation

@motinis

@motinis motinis commented Jan 6, 2025 •
edited by marionbarker
Loading

Copy link
Copy Markdown

This PR is in response to Loop Issue 2267: Basal Rate Therapy Settings do not update automatically after changes

@marionbarker marionbarker requested a review from ps2 March 21, 2025 13:54
@marionbarker marionbarker mentioned this pull request Mar 21, 2025
Merged
ps2
ps2 reviewed Mar 21, 2025
View reviewed changes
Comment thread LoopKitUI/Views/Settings Editors/TherapySettingsView.swift
private var basalRatesSection: Card {
card(for: .basalRate) {
if let schedule = viewModel.therapySettings.basalRateSchedule,
@State var basalRateSchedule = viewModel.therapySettings.basalRateSchedule // work-around for issue 2267 where basalRateSchedule doesn't update in the card

@ps2 ps2 Mar 21, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Declaring @State inside a computed var is a misuse of the @State wrapper. It won't provide the state persistence expected because computed properties don't maintain storage between calculations. This might work coincidentally due to the onAppear update but this needs to be updated.

@motinis motinis Mar 22, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes - I was aware, that’s why I explicitly commented that it’s a workaround (a proper fix would not need such a comment). I couldn’t find a reason why the current implementation stopped working correctly. If you have a cleaner way to fix then that’s better. Otherwise I preferred a benign workaround to the current buggy state.

@motinis

motinis commented Apr 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

I have tried different alternatives, but to no avail. I cannot replicate this now in the simulator but it does happen consistently on iPhone SE.

It does not happen on iPhone 11 Pro, iOS 18.3.2 (and after upgrading also not on 18.4)

@marionbarker

marionbarker commented Apr 18, 2025

Copy link
Copy Markdown
Contributor

Test

Bottom line - this PR is not a reliable fix.

Configuration

  • SE 3rd gen phone, iOS 18.4 phone
  • Pump: rPi DASH simulator; in-app pump simulator (tried both)
  • CGM: Nightscout
  • macOS 15.4
  • Xcode 16.3

dev branch

LoopWorkspace built from dev (commit f3021a5)

  • confirmed that the basal is saved but stale basal rate value is displayed initially

tidepool-merge branch

LoopWorkspace built from tidepool-merge (commit 17aa656)

  • confirmed that the basal is updated properly (never stale)

dev branch with this PR

LoopWorkspace built from dev (commit f3021a5) with this PR (LoopKit commit a4d21161)

  • This did not work to fix the Issue in my testing
  • I rebuilt twice, confirmed the change; tried rPi and in-app pump
  • The screenshots below are from left to right
    • Initial Basal Rates on Therapy Settings
    • Screen where basal rates were modified and saved to pod
    • Basal Rates on Therapy Settings screen after save (stale value)
    • Basal Rates on Therapy Settings screen after going up to Settings and then returning to Therapy Settings (current value)

issue-2267

tidepool-merge branch - repeat

LoopWorkspace built from tidepool-merge (commit 17aa656) - repeat the test to confirm bug is fixed

  • confirmed that the basal is updated properly (never stale)

Code Review

  • I confirmed the tidepool-merge and dev branch code is identical (except for import LoopAlgorithm is added to tidepool-merge) for LoopKit/LoopKitUI/Views/Settings Editors/TherapySettingsView.swift (which is the file modified in this PR.

  • So then I looked at other files associated with the Therapy Settings:

find . -name "*Therapy*.swift"
./LoopKit/LoopKitUI/ViewModels/TherapySettingsViewModel.swift
./LoopKit/LoopKitUI/Views/Settings Editors/TherapySetting+Settings.swift
./LoopKit/LoopKitUI/Views/Settings Editors/TherapySettingsView.swift
./LoopKit/LoopKitUI/Views/Information Screens/GlucoseTherapySettingInformationView.swift
./LoopKit/LoopKitTests/TherapySettingsTests.swift
./LoopKit/LoopKit/TherapySetting.swift
./LoopKit/LoopKit/TherapySettings.swift

I then looked for diffs between tidepool-merge and dev branches for those files. The only one with substantive differences was for diff LoopKit/LoopKitUI/ViewModels/TherapySettingsViewModel.swift. The difference was not in the BasalRates part of the code but instead in syncDeliveryLimits.

Details of the diff:

13d12
< import LoopAlgorithm
17c16
<     func syncDeliveryLimits(deliveryLimits: DeliveryLimits) async throws -> DeliveryLimits
---
>     func syncDeliveryLimits(deliveryLimits: DeliveryLimits, completion: @escaping (Result<DeliveryLimits, Error>) -> Void)
102,103c101,102
<     public func syncDeliveryLimits(deliveryLimits: DeliveryLimits) async throws -> DeliveryLimits {
<         return try await delegate?.syncDeliveryLimits(deliveryLimits: deliveryLimits) ?? deliveryLimits
---
>     public func syncDeliveryLimits(deliveryLimits: DeliveryLimits, completion: @escaping (Result<DeliveryLimits, Error>) -> Void) {
>         delegate?.syncDeliveryLimits(deliveryLimits: deliveryLimits, completion: completion)

I stopped there.

@motinis

motinis commented Apr 18, 2025

Copy link
Copy Markdown
Author

@marionbarker I don’t think that change is relevant. Perhaps there is some difference in LoopKit or Loop project files?

@marionbarker

marionbarker commented Apr 19, 2025

Copy link
Copy Markdown
Contributor

Close this PR. Refer to #558 as an alternative.

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

Reviewers

@ps2 ps2 ps2 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@motinis @marionbarker @ps2