Skip to content

[compliments] refactor: optimize loadComplimentFile method and add unit tests #3969

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.

Sign up for GitHub

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

Jump to bottom
Merged
rejas merged 3 commits into from
Nov 23, 2025
Merged

[compliments] refactor: optimize loadComplimentFile method and add unit tests #3969

rejas merged 3 commits into from
Nov 23, 2025

Conversation

@KristjanESPERANTO
Copy link
Collaborator

@KristjanESPERANTO KristjanESPERANTO commented Nov 19, 2025 •
edited
Loading

Changes

  • Replace indexOf() with startsWith() for cleaner protocol detection
  • Use URL API for robust cache-busting parameter handling
  • Add HTTP response validation and improved error logging
  • Add JSDoc type annotations for better documentation
  • Remove unused urlSuffix instance variable
  • Add unit tests
  • Fix .gitignore pattern

Motivation

After merging #3967, I noticed some potential for improving reliability and user experience related to the method loadComplimentFile. With these changes the method now validates URLs upfront to catch configuration errors early, checks HTTP status codes to detect server issues (404/500), and provides specific error messages that help users troubleshoot problems.

The complexity of the code does not really increase with the changes. On the contrary, the method should now be more intuitive to understand.

Testing

Added unit tests for loadComplimentFile() to validate the improvements:

  • HTTP error handling
  • Cache-busting

Since E2E tests already cover the happy path, these unit tests focus on error cases and edge cases.

Additional Fix

While adding the test file, I discovered that the .gitignore pattern modules was incorrectly matching tests/unit/modules/, preventing test files from being tracked. Changed to /modules to only match the root directory.

@KristjanESPERANTO
refactor(compliments): optimize loadComplimentFile method
c5ba952 
- Replace `indexOf()` with `startsWith()` for cleaner protocol detection
- Use `URL` API for robust cache-busting parameter handling
- Add HTTP response validation and improved error logging
- Add JSDoc type annotations for better documentation
- Remove unused `urlSuffix` instance variable

Improves reliability and user experience: Validates URLs upfront to
catch configuration errors early, checks HTTP status codes to detect
server issues (404/500), and provides specific error messages that
help users troubleshoot problems independently.
@KristjanESPERANTO
fix: correct gitignore pattern for modules directory
bc7018c 
Use leading slash to only ignore /modules in root directory,
preventing test files in tests/unit/modules/ from being ignored.
@KristjanESPERANTO
test(compliments): add unit tests for loadComplimentFile
90643e1 
- Test HTTP error handling (404, 500)
- Test cache-busting with existing query parameters
- Test cache-busting configuration behavior
@rejas rejas merged commit 7934e7a into MagicMirrorOrg:develop Nov 23, 2025
9 checks passed
@KristjanESPERANTO
Copy link
Collaborator Author

KristjanESPERANTO commented Nov 23, 2025

@rejas Thanks for reviewing and accepting! 🙂

@KristjanESPERANTO KristjanESPERANTO deleted the compliments branch November 23, 2025 17:12
@sdetweil sdetweil mentioned this pull request Jan 1, 2026
Merged
sdetweil added a commit that referenced this pull request Jan 1, 2026
@sdetweil @khassel
@KristjanESPERANTO @ryan-d-williams @rejas @bugsounet @MarcLandis @dependabot @HeikoGr @pedrolamas @MagMar94 @eltociear @Developer-Incoming @n8many @mixasgr @savvadam @geraki @OWL4C @bughaver @plebcity @dathbe @m-idler @Crazylegstoo @jboucly @jarnoml @JHWelch @Blackspirits @xsorifc28
Release 2.34.0 (#3999)
b742e83 
Thanks to: @Blackspirits, @Crazylegstoo, @jarnoml, @jboucly, @JHWelch,
@khassel, @KristjanESPERANTO, @rejas, @sdetweil, @xsorifc28

⚠️ This release needs nodejs version >=22.21.1 <23 || >=24

[Compare to previous Release
v2.33.0](v2.33.0...develop)

[core]
Prepare Release 2.34.0
(#3998)
dependency update + adjust Playwright setup + fix linter issue
(#3994)
demo with gif (#3995)
[core] fix: allow browser globals in config files
(#3992)
[core] fix: restore --ozone-platform=wayland flag for reliable Wayland
support (#3989)
[core] auto create release notes with every push on develop
(#3985)
[core] chore: simplify Wayland start script
(#3974)
[gitignore] restore the old Git behavior for the default modules
(#3968)
[core] configure cspell to check default modules only and fix typos
(#3955)
[gitignore] restoring the old Git behavior for the CSS directory
(#3954)
feat(core): add server:watch script with automatic restart on file
changes (#3920)
[check_config] refactor: improve error handling
(#3927)
refactor: replace express-ipfilter with lightweight custom middleware
(#3917)
refactor: replace module-alias dependency with internal alias resolver
(#3893)

[dependencies]

[chore] update dependencies and min. node version
(#3986)
[core] bump dependencies into december
(#3982)
Bump actions/checkout from 5 to 6
(#3972)
Update deps, unpin parse5
(#3934)
[core] Update deps and pin jsdom to v27.0.0
(#3925)
chore: update dependencies
(#3921)
update deps, exclude node v23
(#3916)
remove eslint warnings, add npm publish process to Collaboration.md
(#3913)
feat: add ESlint rule no-sparse-arrays for config check
(#3911)
chore: bump dependencies into october
(#3909)

[logging]

logger: add calling filename as prefix on server side
(#3926)
[logger] Add prefixes to most Log messages
(#3923)

[modules/alert]
Add new pt and pt-BR translations for Alert module and update global PT
strings (#3965)

[modules/calendar]
add checksum to test whether calendar event list changed
(#3988)
[calendar] fix: prevent excessive fetching on client reload and refactor
calendarfetcherutils.js
(#3976)
[calendar] refactor: migrate CalendarFetcher to ES6 class and improve
error handling (#3959)
[calendar] Show repeatingCountTitle only if yearDiff > 0
(#3949)
[tests] suppress debug logs in CI environment + improve calendar symbol
test stability (#3941)
[calendar] chore: remove requiresVersion: "2.1.0"
(#3932)
[calendar] test: remove "Recurring event per timezone" test
(#3929)

[modules/compliments]
[compliments] refactor: optimize loadComplimentFile method and add unit
tests (#3969)
fix: set compliments remote file minimum delay to 15 minutes
(#3970)
[compliments] fix: duplicate query param "?" in compliments module
refresh url (#3967)

[modules/newsfeed]
[newsfeed] fix header layout issue
(#3946)

[modules/weather]
[weatherprovider] update subclass language use override
(#3914)
[weather] fix wind-icon not showing in pirateweather
(#3957)
[weather] add error handling to weather fetch functions, including cors
(#3791)
remove deprecated ukmetoffice datapoint provider, cleanup .gitignore
(#3952)
fixes problems with daylight-saving-time in weather provider openmeteo
(#3931)
Fix for envcanada Provider to use updated Env Canada URL
(#3919)
[weather] feat: add configurable forecast date format option
(#3918)

[testing]
testing: update "Enforce Pull-Request Rules" workflow
(#3987)
[core] refactor: replace XMLHttpRequest with fetch and migrate e2e tests
to Playwright (#3950)
[test] replace node-libgpiod with serialport in electron-rebuild
workflow (#3945)
[ci] Add concurrency to automated tests workflow to cancel outdated runs
(#3943)
[tests] migrate from jest to vitest
(#3940)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Karsten Hassel <hassel@gmx.de>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: Ryan Williams <65094007+ryan-d-williams@users.noreply.github.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: Marc Landis <dirk.rettschlag@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: HeikoGr <20295490+HeikoGr@users.noreply.github.com>
Co-authored-by: Pedro Lamas <pedrolamas@gmail.com>
Co-authored-by: veeck <gitkraken@veeck.de>
Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com>
Co-authored-by: Ikko Eltociear Ashimine <eltociear@gmail.com>
Co-authored-by: DevIncomin <56730075+Developer-Incoming@users.noreply.github.com>
Co-authored-by: Nathan <n8nyoung@gmail.com>
Co-authored-by: mixasgr <mixasgr@users.noreply.github.com>
Co-authored-by: Savvas Adamtziloglou <savvas-gr@greeklug.gr>
Co-authored-by: Konstantinos <geraki@gmail.com>
Co-authored-by: OWL4C <124401812+OWL4C@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+bughaver@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+lsaadeh@users.noreply.github.com>
Co-authored-by: Koen Konst <koenspero@gmail.com>
Co-authored-by: Koen Konst <c.h.konst@avisi.nl>
Co-authored-by: dathbe <github@beffa.us>
Co-authored-by: Marcel <m-idler@users.noreply.github.com>
Co-authored-by: Kevin G. <crazylegstoo@gmail.com>
Co-authored-by: Jboucly <33218155+jboucly@users.noreply.github.com>
Co-authored-by: Jboucly <contact@jboucly.fr>
Co-authored-by: Jarno <54169345+jarnoml@users.noreply.github.com>
Co-authored-by: Jordan Welch <JordanHWelch@gmail.com>
Co-authored-by: Blackspirits <blackspirits@gmail.com>
Co-authored-by: Samed Ozdemir <samed@xsor.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rejas rejas rejas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KristjanESPERANTO @rejas