Skip to content

Fix override banner in Live activity#653

Merged
dnzxy merged 7 commits intonightscout:devfrom
marv-out:overridesLA
Jul 31, 2025
Merged

Fix override banner in Live activity#653
dnzxy merged 7 commits intonightscout:devfrom
marv-out:overridesLA

Conversation

@marv-out
Copy link
Contributor

I've noticed that the LA banner disappears in the following situation:

  • start an Override preset -> banner is there as it is supposed to be
  • now edit the running override (not the preset) and save -> banner is gone

Debugging this showed me that we are

  1. missing the ID when we are performing our copy override logic and
  2. (the "actual fix) are setting the same date as the copied override. This leads to the fetch only showing the first (now) disabled override that we've copied previously. And because of the fact that its disabled, it isn't shown in the UI. Also updating the date to Date() fixes this as this override will now be the latest override.

I really hope that this PR doesn't break anything related to overrides, as this logic is pretty well tested and works. I don't think that this small change will cause any issues, but nevertheless, it needs to be tested thoroughly.

@dnzxy
Copy link
Contributor

dnzxy commented Jun 11, 2025

@dsnallfot @bjorkert @bjornoleh @flyingpie101 you guys are the override hardcore users. Let's go 🤓

@dnzxy
Copy link
Contributor

dnzxy commented Jun 17, 2025

I understand why you are adding an actual override date when the new override gets stored (based on your description of the issue and the intended fix). Why are you removing the duration helper function? No longer needed or mistakenly removed, @marv-out ?

@marv-out
Copy link
Contributor Author

It wasn't in use. We apparently use a different one

MikePlante1
MikePlante1 previously approved these changes Jun 22, 2025
Copy link
Contributor

@MikePlante1 MikePlante1 left a comment

Choose a reason for hiding this comment

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

Tested this in a simulator. I confirmed it fixes the problem of the OR banner in LA being hidden when a running OR is edited. I did not notice any adverse effects.

I also started a preset 10 min OR, edited the target after 6 minutes, and it expired 4 minutes later as expected.

I'll note that the Adjustments History page only shows the "Custom Override" for the full duration of the OR and does not show the initial preset, but that's the same as it is in current dev, so not anything introduced by this PR.

@MikePlante1 MikePlante1 dismissed their stale review June 22, 2025 03:43

Further testing revealed a change when altering the duration of a running override. For example:

  • Previously if you were 20 minutes into a 1 hour override and edited its duration to 10 minutes, the override would be cancelled. This PR makes it so it will continue to run for 10 minutes after the override was edited.
  • Previously, if you were 20 minutes into a 1 hour override and edited its duration to 40 minutes, the override would continue for another 20 minutes (40-20). This PR makes it so it will continue to run for 40 minutes after the override was edited.

This only happens when you have a preset running and you edit the current running override. It does not happen when you edit a current running override that has already been edited. When editing a previously edited override, it acts as it does in current dev.

@marv-out
Copy link
Contributor Author

marv-out commented Jul 2, 2025

Ok I think this makes sense from looking at my changes. Hmmm, will have another look later how to avoid this behaviour

@marv-out
Copy link
Contributor Author

marv-out commented Jul 2, 2025

I pushed a fix for the behaviour you described @MikePlante1. Pls test again!

Copy link

@kingst kingst left a comment

Choose a reason for hiding this comment

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

LGTM based on code review

@dnzxy
Copy link
Contributor

dnzxy commented Jul 29, 2025

@MikePlante1 have you had the chance to test this again based on the comment by Marvin from July 2nd?

Copy link
Contributor

@MikePlante1 MikePlante1 left a comment

Choose a reason for hiding this comment

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

Re-tested with the latest code & now it LGTM

@dnzxy dnzxy merged commit 8675383 into nightscout:dev Jul 31, 2025
3 checks passed
mountrcg referenced this pull request in mountrcg/Tai Aug 28, 2025
Fix override banner in Live activity
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.

4 participants