Repository navigation
fix: refresh browser controls after clearing messages - #158
Merged
Merged
Conversation
[Medium impact] The PR appears safe to merge; the previous test-timing concern is fixed. SummaryThe PR refreshes browser controls after messages are cleared.
The previous test-timing concern is fixed. No new actionable issues were found. Reviews (2) · Last reviewed commit: "test: make clear controls regression det..." · Reviewed by Greptile |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The localized fix addresses the stale-count condition, and the regression test covers the reported failure sequence.
No open issues 🎉
What changed in this PR
Fixes #135 by refreshing Browser controls when new messages arrive after clearing stored messages.
Changes:
- Resynchronizes the tracked root-topic count after clearing.
- Adds a deterministic regression test using an overridable timer factory.
| File | Description |
|---|---|
| tests/MqttProbe.UI.Tests/Components/Browser/BrowserPanelTests.cs | Verifies Clear and Topics recover when traffic restores the previous count. |
| src/MqttProbe.UI/Components/Browser/BrowserPanel.razor | Updates count tracking after clearing and allows tests to disable the timer. |
Review effort: Balanced
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.
Root cause
BrowserPanelgates its timer-drivenStateHasChangedon_lastCounttrackingMessageStoreManager.RootTopicCount. Clearing messages awaitedClearAllMessages()and the button disabled on the resulting rerender, but_lastCountwas left at the pre-clear value. If traffic restored the same root count before the next tick,OnTimerTicksawcount == _lastCount, skipped the guarded render, and the Clear button (and the header Topics chip) stayed stale and disabled.Fix
After the awaited clear, resync the tracked count so the next tick renders:
One line, no layout or style changes.
Regression test
ClearMessages_AfterClear_WhenTrafficRestoresCount_ReenablesButtonOnNextTick(proven failing before the fix, passing after): count 1 → button enabled and Topics chip present → click Clear (count 0) → button disabled and chip absent → restore count 1 before any tick →OnTimerTick()→ button re-enabled and Topics chip present. Also restored the[Test]attribute onBrowserHint_WhenNoStoresAndNotDismissed_IsVisible;BrowserPanelTestsdiscovery is 13 (12 baseline + 1 new), no reduction.Fixes #135.
Validation
MqttProbe.slnxbuild: succeeded, 0 warnings, 0 errorspython scripts/ci/format-check.pyfull tree: cleanNote: validation ran in the working worktree, which also carried unrelated dirty changes; the committed tree contains only this change plus the baseline.