Fix block page showing cycle rollover time during manual lockdown#712
Open
absurd wants to merge 1 commit into
Open
Fix block page showing cycle rollover time during manual lockdown#712absurd wants to merge 1 commit into
absurd wants to merge 1 commit into
Conversation
When a manual lockdown is in effect and the current time-limit cycle has not been exceeded, getUnblockTime() in background.js was unconditionally using the cycle rollover time, then only raising unblockTime to the lockdown end if the latter was later. The effect: a manual lockdown ending before the next cycle rollover was obscured on the block page, which showed the cycle rollover time instead of the actual lockdown end. Fix: gate the Case 2 and Case 4 cycle-rollover assignments on an afterTimeLimit check, matching the existing one already computed inside Case 4. When the time limit is not actually exceeded, leave unblockTime null so the subsequent lockdown / minimum-block-time checks install the correct end time.
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
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.
Bug
When a manual lockdown is active on a block set that also has a time-limit cycle (e.g. "20 minutes in every 4 hours"), and the lockdown ends before the next cycle rollover, the block page shows the rollover time instead of the lockdown end time.
Reproduction:
limitMins=20,limitPeriod=14400(4 h), no time periods.lockdown.html, select the set and activate a short lockdown (e.g. 10 minutes) that ends before the next cycle rollover.Expected: block page shows the lockdown end time.
Actual: block page shows the next cycle rollover time (e.g. "4:00:00 PM" when the 4-hour cycle rolls over at 4 PM, even though the lockdown ends at ~3:10 PM).
Before (pre-fix): block page shows
4:00:00 PM— the cycle rollover, despite the lockdown ending 24 minutes earlier.After (with this fix): same state, block page shows
3:36:49 PM— the actual lockdown end time.Root cause
In
getUnblockTime(), Case 2 (!timePeriods && timeLimit, lines 1283–1287) and the Case 4 fallback (lines 1333–1336) unconditionally set:…even when the time limit had not actually been exceeded. The lockdown check below (lines 1340–1346) only raises
unblockTimeiflockdownEnd > unblockTime, so an earlier lockdown end was masked by the later cycle rollover. The min-block-time check (lines 1348–1354) has the same shape.Change
Gate the Case 2 and Case 4 cycle-rollover assignments on an
afterTimeLimitcheck — the same computation already present inside Case 4. When the time limit is not actually exceeded, leaveunblockTimenullso the existing lockdown / minimum-block-time checks install the correct end time.The existing
endTime > unblockTimecomparisons in the lockdown and min-block checks already tolerate anullunblockTime(Date coerces to its epoch-ms value,nullcoerces to 0), so no changes there are required.Case 3 (
conjMode) is deliberately left alone — fixing the analogous edge case there would require broader changes and is out of scope for a minimal bug fix.Diff:
background.js+9/-2,VERSIONS.md+3.Verification
I tested by directly calling
getUnblockTime()against a synthesizedtimedata[]state for each of: lockdown-only (the bug repro), limit exhausted + no lockdown, lockdown-after-rollover, min-block only, limit exhausted + lockdown-before-rollover, and a "nothing active" degenerate case. All produce the expected unblock time. Also confirmed end-to-end by navigating to a blocked URL in Firefox with the bug-repro state applied — blocked.html now shows the lockdown end time instead of the cycle rollover (screenshots above).Note on behavior change
In the "no block reason is actually active" edge case (time limit not exceeded, no lockdown, no min-block, no time period — a state that shouldn't normally arise when the block page is shown),
getUnblockTime()now returnsnullinstead of falling back to the cycle rollover.blocked.htmlrenders this as "the end of time", matching its default placeholder. I believe this is correct — there is no principled unblock time to show in that case — but happy to revisit if you'd rather preserve the old behavior.