-
Notifications
You must be signed in to change notification settings - Fork 3
feat(progress): Add debouncing to prevent spam from rapid updates #2
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
feat(progress): Add debouncing to prevent spam from rapid updates #2
Conversation
marvin-roesch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this submission! There's only a few things I have to note, one of which will require some work on my part first (unless you also want to tackle the offset parsing).
f037b1d to
f60d484
Compare
|
Summary of changes:
|
marvin-roesch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes, especially the unit tests! I think the logic is all good now, but I still find a little hard to follow. See my suggestion to maybe improve that.
|
|
||
| // Setup Discord expectations | ||
| if tt.expectedPosts > 0 { | ||
| mockDiscord.On("Send", "Progress updated!", mock.Anything, mock.Anything, mock.Anything).Return(nil).Times(tt.expectedPosts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests, I was a bit too lazy to introduce them so far, but it definitely makes sense at this level of complexity.
However, I think it would be good to actually check the content of the posted messages here to verify that the progress reports are correctly done. Could separate the actual reporting into its own struct with a method that accepts a ProgressDiff so that that can be mocked for easier testing, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added discord message validation. I'm sure there is a better way to do it, but this is what I could come up with.
f60d484 to
ce298ab
Compare
|
Made a bunch of changes to make the logic clearer. Mainly
|
ce298ab to
6e62c47
Compare
|
Fixed a stupid mistake I made in the previous revision and added comments in the test scenarios to better explain what it is testing. |
Add debouncing to progress plugin to prevent notification spam
Problem
The progress plugin currently posts to Discord immediately when changes are detected. This causes spam when Brandon's website updates multiple times in quick succession (e.g., update → revert → update), flooding the Discord channel with
notifications.
Solution
Added a configurable debounceDelay option that waits for a specified period after the last detected change before posting. This ensures only the final state is posted after rapid changes settle.
Changes
• New config option: debounceDelay (e.g., "2m", "30s") - optional, defaults to immediate posting
• Enhanced offset format: Stores timing and pending change information for debouncing
• Backward compatibility: Automatically handles legacy offset format
• Updated documentation: README reflects new configuration options
Usage
yaml
connectors:
brandon-progress:
plugin: progress
config:
url: https://brandonsanderson.com
message: The progress bars on Brandon's website were updated!
debounceDelay: "2m" # Wait 2 minutes after last change
Testing
Tested with rapid successive runs (modifying the saved offset in between) - no immediate posts during changes, only posts after delay expires.