feat: Add Unit Setting - Metric or Imperial

[salmanfarisvp]

User description

• Added Unit Type Setting
• Removed Debug Console Message
• Improve Caching and Refresh token logic

Preview.

Metric Unit

Imperial Unit.

---

PR Type

Enhancement, Bug fix, Documentation

---

Description

• Add unit_type setting for metric/imperial

• Respect unit setting in formatting

• Add Strava 429 retry/backoff handling

• Improve leaderboard sorting and validation

---

Diagram Walkthrough

flowchart LR
  S1["`screenly.yml` / `screenly_qc.yml`: `unit_type` setting"]
  U1["`static/js/utils.js`: unit selection + formatting"]
  UI1["`static/js/ui.js`: renders formatted stats"]
  A1["`static/js/api.js`: rate-limit retries + sorting"]
  C1["`static/js/cache.js`: longer cache duration"]
  M1["`static/js/main.js`: slower refresh interval"]
  S1 -- "configures units" --> U1
  U1 -- "formats distance/elevation" --> UI1
  A1 -- "fetch + retry 429/401" --> UI1
  C1 -- "cache responses longer" --> A1
  M1 -- "schedule refresh" --> A1

Loading

File Walkthrough

	Relevant files
Enhancement	4 files api.js Add 429 backoff and deterministic ranking                                +149/-144 cache.js Increase cache TTL and reduce logging                                        +49/-72  main.js Slow refresh cadence and trim debug output                              +27/-28  utils.js Add `unit_type` override for imperial/metric                          +66/-40
Formatting	1 files ui.js Lint/formatting cleanups in UI helpers                                      +34/-22
Documentation	1 files README.md Document unit system configuration feature                              +1/-0
Configuration changes	2 files screenly.yml Add `unit_type` select setting options                                      +17/-1    screenly_qc.yml Mirror `unit_type` setting in QC config                                    +16/-0

---

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f3371be)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Retry Handling The 429 retry/backoff logic uses the Retry-After header with parseInt. If Strava ever returns an HTTP-date or a non-numeric value, waitTime can become NaN (effectively sleeping 0ms). Consider validating the parsed value (fallback to default delay when invalid) and/or logging the computed wait time to avoid tight retry loops. // Handle 429 Rate Limit Exceeded if (response.status === 429) { const retryAfter = response.headers.get('Retry-After') const waitTime = retryAfter ? parseInt(retryAfter, 10) * 1000 : CONFIG.RATE_LIMIT_RETRY_DELAY console.warn( `⚠️ Rate limit exceeded (429). Retry attempt ${rateLimitRetry + 1}/${CONFIG.RATE_LIMIT_MAX_RETRIES}`, ) if (rateLimitRetry < CONFIG.RATE_LIMIT_MAX_RETRIES) { await sleep(waitTime) return makeStravaRequest(url, options, rateLimitRetry + 1) } else { throw new Error( 'Rate Limit Exceeded. Strava API limits reached. Please wait a few minutes and try again.', ) } } Type Safety usesImperialUnits assumes screenly.settings.unit_type is a string and calls .toLowerCase(). If the setting is missing, null, or unexpectedly structured, this can throw at runtime. Consider guarding with a typeof === 'string' check (and possibly trimming) before lowercasing. function usesImperialUnits(locale) { // Check if unit_type setting is configured if ( typeof screenly !== 'undefined' && screenly.settings && screenly.settings.unit_type ) { const unitType = screenly.settings.unit_type.toLowerCase() if (unitType === 'imperial') { return true } if (unitType === 'metric') { return false } } Config Schema The newly added unit_type setting uses a nested help_text.properties structure to define a select UI. Please validate this matches Screenly's expected schema for settings definitions (including indentation and whether help_text supports objects vs. strings), otherwise the setting may not render or may be ignored at runtime. unit_type: type: string title: Unit System default_value: metric optional: true help_text: properties: type: select help_text: | Choose the unit system for displaying distance and elevation. Options: "metric" (km, m) or "imperial" (mi, ft). options: - label: Metric value: metric - label: Imperial value: imperial schema_version: 1

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f3371be
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Make rate-limit backoff robust Handle invalid or non-numeric Retry-After values; parseInt() can yield NaN, causing sleep(NaN) to behave unexpectedly and retries to hammer the API. Clamp the computed wait time to a sane minimum/maximum before sleeping. edge-apps/strava-club-leaderboard/static/js/api.js [242-260] if (response.status === 429) { const retryAfter = response.headers.get('Retry-After') - const waitTime = retryAfter - ? parseInt(retryAfter, 10) * 1000 - : CONFIG.RATE_LIMIT_RETRY_DELAY + const parsedSeconds = retryAfter ? Number.parseInt(retryAfter, 10) : NaN + const waitTime = + Number.isFinite(parsedSeconds) && parsedSeconds > 0 + ? parsedSeconds * 1000 + : CONFIG.RATE_LIMIT_RETRY_DELAY + + const clampedWaitTime = Math.min(Math.max(waitTime, 1000), 5 * 60 * 1000) console.warn( `⚠️ Rate limit exceeded (429). Retry attempt ${rateLimitRetry + 1}/${CONFIG.RATE_LIMIT_MAX_RETRIES}`, ) if (rateLimitRetry < CONFIG.RATE_LIMIT_MAX_RETRIES) { - await sleep(waitTime) + await sleep(clampedWaitTime) return makeStravaRequest(url, options, rateLimitRetry + 1) } else { throw new Error( 'Rate Limit Exceeded. Strava API limits reached. Please wait a few minutes and try again.', ) } } Suggestion importance[1-10]: 7 __ Why: If Retry-After is missing or malformed, parseInt() can yield NaN, and sleep(NaN) effectively becomes a 0ms delay, causing immediate retry bursts against the Strava API. Validating/clamping the computed wait time makes the 429 handling in makeStravaRequest() more correct and safer under real-world responses.	Medium
	Fix unit setting schema structure The unit_type schema is likely malformed: help_text is typically a string, so nesting properties/options/schema_version under it may prevent the setting from rendering or being parsed. Define the select/options using the expected top-level keys for a setting and keep help_text as a string. edge-apps/strava-club-leaderboard/screenly.yml [39-54] unit_type: type: string title: Unit System default_value: metric optional: true - help_text: - properties: - type: select - help_text: | - Choose the unit system for displaying distance and elevation. Options: "metric" (km, m) or "imperial" (mi, ft). - options: - - label: Metric - value: metric - - label: Imperial - value: imperial - schema_version: 1 + help_text: | + Choose the unit system for displaying distance and elevation. Options: "metric" (km, m) or "imperial" (mi, ft). + properties: + type: select + options: + - label: Metric + value: metric + - label: Imperial + value: imperial + schema_version: 1 Suggestion importance[1-10]: 7 __ Why: The unit_type definition nests properties, options, and schema_version under help_text, which is very likely to be misinterpreted since help_text is typically a string field. Restructuring so help_text remains a string and properties/options are top-level under unit_type should prevent the setting UI/config parsing from breaking.	Medium
	Prevent crash on invalid settings Guard against non-string values for screenly.settings.unit_type before calling toLowerCase(), otherwise the app can crash at runtime if the setting is missing, null, or not a string. Trim the value to handle accidental whitespace. edge-apps/strava-club-leaderboard/static/js/utils.js [21-35] function usesImperialUnits(locale) { // Check if unit_type setting is configured if ( typeof screenly !== 'undefined' && screenly.settings && - screenly.settings.unit_type + typeof screenly.settings.unit_type === 'string' ) { - const unitType = screenly.settings.unit_type.toLowerCase() + const unitType = screenly.settings.unit_type.trim().toLowerCase() if (unitType === 'imperial') { return true } if (unitType === 'metric') { return false } } Suggestion importance[1-10]: 6 __ Why: The current code calls toLowerCase() on screenly.settings.unit_type after only a truthiness check, so a non-string value can throw at runtime. Adding a typeof === 'string' guard (and trim()) is a small but real robustness improvement in usesImperialUnits().	Low

---

Previous suggestions

Suggestions up to commit 82c394a

Category	Suggestion	Impact
Possible issue	Handle rate limits on retries The post-refresh retry path doesn't handle 429, so a rate-limited response after token refresh will throw instead of backing off. Apply the same 429 handling here to avoid spurious failures when Strava throttles during retries. edge-apps/strava-club-leaderboard/static/js/api.js [261-269] const retryResponse = await fetch(url, { ...options, headers: retryHeaders }) + +if (retryResponse.status === 429) { + const retryAfter = retryResponse.headers.get('Retry-After') + const retryAfterSeconds = retryAfter != null ? Number(retryAfter) : NaN + const waitTime = Number.isFinite(retryAfterSeconds) && retryAfterSeconds > 0 + ? retryAfterSeconds * 1000 + : CONFIG.RATE_LIMIT_RETRY_DELAY + + console.warn(`⚠️ Rate limit exceeded (429). Retry attempt ${rateLimitRetry + 1}/${CONFIG.RATE_LIMIT_MAX_RETRIES}`) + + if (rateLimitRetry < CONFIG.RATE_LIMIT_MAX_RETRIES) { + await sleep(waitTime) + return makeStravaRequest(url, options, rateLimitRetry + 1) + } + throw new Error('Rate Limit Exceeded. Strava API limits reached. Please wait a few minutes and try again.') +} if (!retryResponse.ok) { const errorData = await retryResponse.json().catch(() => ({})) throw new Error(`Strava API error: ${retryResponse.status} - ${errorData.message || retryResponse.statusText}`) } return await retryResponse.json() Suggestion importance[1-10]: 7 __ Why: The post-refresh retry path (retryResponse = await fetch(...)) currently bypasses the new 429 backoff logic, so it can fail unnecessarily under throttling. Reusing the same rate-limit handling improves correctness and resilience when refreshAccessToken() is followed by another rate-limited response.	Medium
	Validate rate-limit retry delay Guard against invalid/empty Retry-After values, since parseInt() can yield NaN and cause a zero-delay retry loop. Clamp to a sane default when the header is missing or not a positive number. edge-apps/strava-club-leaderboard/static/js/api.js [232-246] if (response.status === 429) { const retryAfter = response.headers.get('Retry-After') - const waitTime = retryAfter - ? parseInt(retryAfter, 10) * 1000 + const retryAfterSeconds = retryAfter != null ? Number(retryAfter) : NaN + const waitTime = Number.isFinite(retryAfterSeconds) && retryAfterSeconds > 0 + ? retryAfterSeconds * 1000 : CONFIG.RATE_LIMIT_RETRY_DELAY console.warn(`⚠️ Rate limit exceeded (429). Retry attempt ${rateLimitRetry + 1}/${CONFIG.RATE_LIMIT_MAX_RETRIES}`) if (rateLimitRetry < CONFIG.RATE_LIMIT_MAX_RETRIES) { await sleep(waitTime) return makeStravaRequest(url, options, rateLimitRetry + 1) } else { throw new Error('Rate Limit Exceeded. Strava API limits reached. Please wait a few minutes and try again.') } } Suggestion importance[1-10]: 6 __ Why: The current Retry-After parsing can produce NaN, which effectively becomes a 0ms delay in sleep() and can cause rapid retries. Adding a finite/positive-number guard makes the new 429 handling in makeStravaRequest() more reliable.	Low
	Make timestamp tracking robust Avoid truthiness checks for timestamps (0/NaN) and make the update condition explicit, so invalid dates don't silently poison ordering. This prevents unpredictable sorting when start_date(_local) is missing or unparsable. edge-apps/strava-club-leaderboard/static/js/api.js [451-458] // Track the latest activity time for tiebreaker (Strava logic: who achieved distance first) const activityTime = activity.start_date_local || activity.start_date if (activityTime) { const activityTimestamp = new Date(activityTime).getTime() - if (!stats.latestActivityTime || activityTimestamp > stats.latestActivityTime) { + if (Number.isFinite(activityTimestamp) && (stats.latestActivityTime == null || activityTimestamp > stats.latestActivityTime)) { stats.latestActivityTime = activityTimestamp } } Suggestion importance[1-10]: 5 __ Why: As written, an unparsable start_date(_local) can yield NaN and get stored into stats.latestActivityTime, undermining the intended tie-break sorting. Checking Number.isFinite() and using an explicit null check makes processLeaderboard() ordering more stable.	Low

[github-actions]

Persistent review updated to latest commit f3371be