Feature/add follower counter #4970
Closed
Conversation
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
…ED into feature/add-follower-counter
WalkthroughIntroduces a new Social Counter usermod with strategy-based social network metric fetching and 7-segment display rendering, adds its supporting headers and library manifest, updates documentation for both this usermod and the existing seven-segment display, and applies formatting-only edits to wled00/const.h. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
usermods/seven_segment_display/readme.md (2)
16-18: Normalize heading levels under “Settings”
##### Examplejumps two levels below the## Settingsheading and trips markdownlint (MD001). Promote it to an H4 so the hierarchy stays consistent.-##### Example +#### Example
53-55: Remove trailing spaces inside code spansThe inline code examples include trailing spaces (
\HHMMSS ``) which markdownlint (MD038) flags and which render oddly. Trim the spaces inside the backticks.-`HHMMSS ` -`hh:MM:SS ` +`HHMMSS` +`hh:MM:SS`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
usermods/seven_segment_display/readme.md(2 hunks)usermods/usermod_v2_social_counter/library.json(1 hunks)usermods/usermod_v2_social_counter/readme.md(1 hunks)usermods/usermod_v2_social_counter/social_network_factory.h(1 hunks)usermods/usermod_v2_social_counter/social_network_types.h(1 hunks)usermods/usermod_v2_social_counter/strategies/instagram_strategy.h(1 hunks)usermods/usermod_v2_social_counter/strategies/mock_strategy.h(1 hunks)usermods/usermod_v2_social_counter/strategies/social_network_strategy.h(1 hunks)usermods/usermod_v2_social_counter/strategies/tiktok_strategy.h(1 hunks)usermods/usermod_v2_social_counter/strategies/twitch_strategy.h(1 hunks)usermods/usermod_v2_social_counter/strategies/youtube_strategy.h(1 hunks)usermods/usermod_v2_social_counter/usermod_v2_social_counter.cpp(1 hunks)wled00/const.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/const.h
🧠 Learnings (10)
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/const.h
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/const.h
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/const.h
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Applied to files:
wled00/const.h
📚 Learning: 2025-02-19T12:43:34.200Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Applied to files:
wled00/const.h
📚 Learning: 2025-09-02T02:27:39.220Z
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S:8-16
Timestamp: 2025-09-02T02:27:39.220Z
Learning: The ESP32 macro is defined when compiling code on all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3, etc.) in the Arduino framework, providing consistent library compatibility across the entire ESP32 family. For target-specific detection, CONFIG_IDF_TARGET_* macros should be used instead.
Applied to files:
wled00/const.h
📚 Learning: 2025-02-19T12:43:34.199Z
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
wled00/const.h
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/const.h
📚 Learning: 2025-08-29T01:34:34.358Z
Learnt from: willmmiles
PR: wled/WLED#4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.
Applied to files:
wled00/const.h
📚 Learning: 2025-04-27T10:06:22.545Z
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-137
Timestamp: 2025-04-27T10:06:22.545Z
Learning: In the Seven Segment Display Reloaded usermod, the dimension mismatch between the default array (umSSDRNumbers[11][7]) and the override (umSSDRNumbers[11][10] = umSSDR_NUMBERS) is intentional by design, allowing for different use cases with varying numbers of segments per digit.
Applied to files:
usermods/seven_segment_display/readme.md
🧬 Code graph analysis (8)
usermods/usermod_v2_social_counter/strategies/social_network_strategy.h (5)
usermods/usermod_v2_social_counter/strategies/instagram_strategy.h (2)
getName(28-31)getType(33-36)usermods/usermod_v2_social_counter/strategies/mock_strategy.h (2)
getName(26-29)getType(31-34)usermods/usermod_v2_social_counter/strategies/tiktok_strategy.h (2)
getName(26-29)getType(31-34)usermods/usermod_v2_social_counter/strategies/twitch_strategy.h (2)
getName(20-23)getType(25-28)usermods/usermod_v2_social_counter/strategies/youtube_strategy.h (2)
getName(20-23)getType(25-28)
wled00/const.h (1)
usermods/usermod_v2_social_counter/usermod_v2_social_counter.cpp (1)
USERMOD_ID_SOCIAL_COUNTER(560-563)
usermods/usermod_v2_social_counter/social_network_factory.h (6)
usermods/usermod_v2_social_counter/strategies/social_network_strategy.h (1)
SocialNetworkStrategy(5-14)usermods/usermod_v2_social_counter/strategies/mock_strategy.h (1)
MockStrategy(8-35)usermods/usermod_v2_social_counter/strategies/instagram_strategy.h (1)
InstagramStrategy(11-56)usermods/usermod_v2_social_counter/strategies/youtube_strategy.h (1)
YouTubeStrategy(8-70)usermods/usermod_v2_social_counter/strategies/tiktok_strategy.h (1)
TikTokStrategy(8-67)usermods/usermod_v2_social_counter/strategies/twitch_strategy.h (1)
TwitchStrategy(8-51)
usermods/usermod_v2_social_counter/strategies/youtube_strategy.h (1)
usermods/usermod_v2_social_counter/strategies/social_network_strategy.h (1)
SocialNetworkStrategy(5-14)
usermods/usermod_v2_social_counter/strategies/tiktok_strategy.h (2)
usermods/usermod_v2_social_counter/strategies/social_network_strategy.h (1)
SocialNetworkStrategy(5-14)usermods/usermod_v2_social_counter/strategies/instagram_strategy.h (3)
extractUsername(61-70)getName(28-31)getType(33-36)
usermods/usermod_v2_social_counter/strategies/mock_strategy.h (6)
usermods/usermod_v2_social_counter/strategies/social_network_strategy.h (1)
SocialNetworkStrategy(5-14)usermods/usermod_v2_social_counter/usermod_v2_social_counter.cpp (3)
Serial(291-299)Serial(313-326)Serial(365-393)usermods/usermod_v2_social_counter/strategies/instagram_strategy.h (2)
getName(28-31)getType(33-36)usermods/usermod_v2_social_counter/strategies/tiktok_strategy.h (2)
getName(26-29)getType(31-34)usermods/usermod_v2_social_counter/strategies/twitch_strategy.h (2)
getName(20-23)getType(25-28)usermods/usermod_v2_social_counter/strategies/youtube_strategy.h (2)
getName(20-23)getType(25-28)
usermods/usermod_v2_social_counter/strategies/instagram_strategy.h (2)
usermods/usermod_v2_social_counter/strategies/social_network_strategy.h (1)
SocialNetworkStrategy(5-14)usermods/usermod_v2_social_counter/usermod_v2_social_counter.cpp (3)
Serial(291-299)Serial(313-326)Serial(365-393)
usermods/usermod_v2_social_counter/strategies/twitch_strategy.h (2)
usermods/usermod_v2_social_counter/strategies/social_network_strategy.h (1)
SocialNetworkStrategy(5-14)usermods/usermod_v2_social_counter/strategies/instagram_strategy.h (3)
getName(28-31)getType(33-36)extractUsername(61-70)
🪛 markdownlint-cli2 (0.18.1)
usermods/seven_segment_display/readme.md
14-14: Heading levels should only increment by one level at a time
Expected: h3; Actual: h5
(MD001, heading-increment)
53-53: Spaces inside code span elements
(MD038, no-space-in-code)
54-54: Spaces inside code span elements
(MD038, no-space-in-code)
🔇 Additional comments (2)
usermods/usermod_v2_social_counter/library.json (1)
2-2: Fix invalid JSON key name
"name:"is not a valid JSON key, so this manifest will fail to parse and the usermod cannot be built. Replace it with"name"(without the colon in the string).- "name:": "social_counter", + "name": "social_counter",Likely an incorrect or invalid review comment.
usermods/usermod_v2_social_counter/readme.md (1)
25-39: Nice UI setup walkthroughThe step-by-step guidance for configuring the usermod in the WLED UI is concise and matches the new features. Helps users get productive quickly.
Comment on lines
+13
to
+24
| static int values[4] = {0, 1000, 5000, 100}; | ||
|
|
||
| // Simula cada métrica com incremento independente | ||
| values[metric] += (metric + 1) * 111; // muda a taxa por tipo | ||
| if (values[metric] > 999999) | ||
| values[metric] = 0; | ||
|
|
||
| count = values[metric]; | ||
|
|
||
| Serial.printf("[MOCK] Métrica %d simulada: %d\n", metric, count); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Guard the metric index before touching values
selectedMetric ultimately comes from user-configurable JSON/UI input. If someone saves an out-of-range value (or a future metric constant exceeds 3), values[metric] writes past the four-element array, corrupting memory and crashing the MCU. Add a bounds check before using metric, bail out on invalid input, and keep the rest of the logic unchanged.
bool fetchMetric(int metric, const String &link, int &count) override
{
static int values[4] = {0, 1000, 5000, 100};
+ if (metric < 0 || metric >= (int)(sizeof(values) / sizeof(values[0])))
+ {
+ Serial.printf("[MOCK] Métrica inválida: %d\n", metric);
+ return false;
+ }
+
// Simula cada métrica com incremento independente
values[metric] += (metric + 1) * 111; // muda a taxa por tipo📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static int values[4] = {0, 1000, 5000, 100}; | |
| // Simula cada métrica com incremento independente | |
| values[metric] += (metric + 1) * 111; // muda a taxa por tipo | |
| if (values[metric] > 999999) | |
| values[metric] = 0; | |
| count = values[metric]; | |
| Serial.printf("[MOCK] Métrica %d simulada: %d\n", metric, count); | |
| return true; | |
| } | |
| bool fetchMetric(int metric, const String &link, int &count) override | |
| { | |
| static int values[4] = {0, 1000, 5000, 100}; | |
| if (metric < 0 || metric >= (int)(sizeof(values) / sizeof(values[0]))) | |
| { | |
| Serial.printf("[MOCK] Métrica inválida: %d\n", metric); | |
| return false; | |
| } | |
| // Simula cada métrica com incremento independente | |
| values[metric] += (metric + 1) * 111; // muda a taxa por tipo | |
| if (values[metric] > 999999) | |
| values[metric] = 0; | |
| count = values[metric]; | |
| Serial.printf("[MOCK] Métrica %d simulada: %d\n", metric, count); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In usermods/usermod_v2_social_counter/strategies/mock_strategy.h around lines 13
to 24, the code indexes the static array `values[4]` with `metric` without
validating it; add a bounds check to ensure 0 <= metric < 4 before any access,
log or Serial.printf an error and return false (or otherwise bail) on invalid
index, and only then proceed with the existing increment, wrap/reset and
assignment logic so out-of-range inputs cannot corrupt memory.
Comment on lines
+11
to
+18
| bool fetchMetric(int metric, const String &link, int &count) override | ||
| { | ||
| // Implementação simplificada para evitar dependências | ||
|
|
||
| // Simula a resposta da API | ||
| count = random(10000, 1000000); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Do not return random follower counts
fetchMetric() currently ignores metric and link, returns a random number, and always reports success. That turns the usermod into noise—each refresh shows a different fake value for a real YouTube channel. Either implement the actual YouTube request/parse flow here or fail fast so the caller can surface “metric unavailable” rather than bogus data. At minimum, drop the random stub until real fetching is wired in.
- // Simula a resposta da API
- count = random(10000, 1000000);
- return true;
+ // TODO: replace with real YouTube API call.
+ count = 0;
+ return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool fetchMetric(int metric, const String &link, int &count) override | |
| { | |
| // Implementação simplificada para evitar dependências | |
| // Simula a resposta da API | |
| count = random(10000, 1000000); | |
| return true; | |
| } | |
| bool fetchMetric(int metric, const String &link, int &count) override | |
| { | |
| // Implementação simplificada para evitar dependências | |
| // TODO: replace with real YouTube API call. | |
| count = 0; | |
| return false; | |
| } |
🤖 Prompt for AI Agents
usermods/usermod_v2_social_counter/strategies/youtube_strategy.h lines 11-18:
fetchMetric currently ignores metric and link and returns a randomized count and
true; remove the random stub and either implement the real YouTube fetch/parse
flow or return a deterministic failure. Replace the stub with one of two
options: (1) implement HTTP request to YouTube API or scrape the page, parse the
correct metric based on the metric parameter, set count and return true on
success and false on any error (propagate/log errors); or (2) if the real
implementation is not ready, remove the random value, set count = 0 (or leave
unchanged), and return false so callers can surface "metric unavailable". Ensure
input validation of link and metric and avoid side effects when failing.
Comment on lines
+246
to
+269
| for (int digit = 0; digit < maxDigits; digit++) | ||
| { | ||
| int fromDigit = (from / (int)pow(10, digit)) % 10; | ||
| int toDigit = (to / (int)pow(10, digit)) % 10; | ||
|
|
||
| int valueToShow = (progress < 0.5) ? fromDigit : toDigit; | ||
|
|
||
| for (int seg = 0; seg < 7; seg++) | ||
| { | ||
| bool on = digitSegments[valueToShow][seg]; | ||
| uint16_t ledIndex = getSegmentLedIndex(digit, seg); | ||
|
|
||
| for (int i = 0; i < ledsPerSegment; i++) | ||
| { | ||
| if (on) | ||
| { | ||
| strip.setPixelColor(ledIndex + i, WS2812FX::Color(r, g, b)); | ||
| } | ||
| else | ||
| { | ||
| strip.setPixelColor(ledIndex + i, 0); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Clamp animation inputs before indexing digitSegments
On the first refresh we have lastDisplayedCount == -1, so Line 351 sets previousCount to -1. When the loop reaches Lines 248‑255, fromDigit becomes -1, which is then used as an index into digitSegments. That immediately walks off the front of the array, so the very first animation step hits undefined behaviour (and in practice reads garbage, potentially crashing on some targets).
Please normalise the from/to values to >= 0 before deriving the per-digit segments. A minimal safe fix is to clamp both inputs and reuse an integer divisor instead of recomputing pow each time, e.g.:
- for (int digit = 0; digit < maxDigits; digit++)
+ int divisor = 1;
+ int safeFrom = from < 0 ? 0 : from;
+ int safeTo = to < 0 ? 0 : to;
+ for (int digit = 0; digit < maxDigits; digit++)
{
- int fromDigit = (from / (int)pow(10, digit)) % 10;
- int toDigit = (to / (int)pow(10, digit)) % 10;
+ int fromDigit = (safeFrom / divisor) % 10;
+ int toDigit = (safeTo / divisor) % 10;
int valueToShow = (progress < 0.5) ? fromDigit : toDigit;
+ divisor *= 10;(Any equivalent guard that keeps the index in the 0‑9 range works.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (int digit = 0; digit < maxDigits; digit++) | |
| { | |
| int fromDigit = (from / (int)pow(10, digit)) % 10; | |
| int toDigit = (to / (int)pow(10, digit)) % 10; | |
| int valueToShow = (progress < 0.5) ? fromDigit : toDigit; | |
| for (int seg = 0; seg < 7; seg++) | |
| { | |
| bool on = digitSegments[valueToShow][seg]; | |
| uint16_t ledIndex = getSegmentLedIndex(digit, seg); | |
| for (int i = 0; i < ledsPerSegment; i++) | |
| { | |
| if (on) | |
| { | |
| strip.setPixelColor(ledIndex + i, WS2812FX::Color(r, g, b)); | |
| } | |
| else | |
| { | |
| strip.setPixelColor(ledIndex + i, 0); | |
| } | |
| } | |
| } | |
| int divisor = 1; | |
| int safeFrom = from < 0 ? 0 : from; | |
| int safeTo = to < 0 ? 0 : to; | |
| for (int digit = 0; digit < maxDigits; digit++) | |
| { | |
| int fromDigit = (safeFrom / divisor) % 10; | |
| int toDigit = (safeTo / divisor) % 10; | |
| int valueToShow = (progress < 0.5) ? fromDigit : toDigit; | |
| for (int seg = 0; seg < 7; seg++) | |
| { | |
| bool on = digitSegments[valueToShow][seg]; | |
| uint16_t ledIndex = getSegmentLedIndex(digit, seg); | |
| for (int i = 0; i < ledsPerSegment; i++) | |
| { | |
| if (on) | |
| { | |
| strip.setPixelColor(ledIndex + i, WS2812FX::Color(r, g, b)); | |
| } | |
| else | |
| { | |
| strip.setPixelColor(ledIndex + i, 0); | |
| } | |
| } | |
| } | |
| divisor *= 10; | |
| } |
🤖 Prompt for AI Agents
In usermods/usermod_v2_social_counter/usermod_v2_social_counter.cpp around lines
246 to 269, the code can compute a negative fromDigit when previousCount is -1
which is then used to index digitSegments; clamp the `from` and `to` values to
be >= 0 before extracting digits, and ensure each per-digit index is constrained
to the 0..9 range (e.g., using max(0, min(9, digitValue))). Replace repeated
pow(10, digit) calls with an integer divisor variable initialized to 1 and
multiplied by 10 each loop iteration to derive digits more efficiently and
safely; apply the clamp to the digit after computing it and then use that
clamped digit to access digitSegments.
|
this appears to be an accidental PR. I will close this in 24h. |
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.
Summary by CodeRabbit
New Features
Documentation
Chores/Style