Skip to content

Conversation

@salmanfarisvp
Copy link
Collaborator

@salmanfarisvp salmanfarisvp commented Dec 3, 2025

User description

The user wants to control the refresh time of the Power BI dashboard, which is currently hardcoded to 30 minutes. This PR introduces a minimum refresh interval of 1 minute, with the default remaining at 30 minutes.

  • Update Refresh Logic as per user input
  • Introduce a new user input field to enter the refresh time under advanced.

image


PR Type

Enhancement


Description

  • Add configurable refresh interval setting

  • Enforce 1-minute minimum interval

  • Default refresh stays at 30 minutes

  • Wire setting into token refresh loop


Diagram Walkthrough

flowchart LR
  Settings["New setting: `refresh_interval` (minutes)"]
  MainJS["Compute interval from settings"]
  TokenLoop["Token refresh loop uses interval"]
  Defaults["Default 30m, minimum 1m"]

  Settings -- "read value" --> MainJS
  MainJS -- "validate & to seconds" --> TokenLoop
  Defaults -- "fallbacks" --> MainJS
Loading

File Walkthrough

Relevant files
Enhancement
main.js
Configurable token refresh interval in JS                               

edge-apps/powerbi/static/js/main.js

  • Add getTokenRefreshInterval with min/default logic
  • Replace hardcoded 30m with configurable interval
  • Use interval for initial and subsequent timeouts
+15/-4   
screenly.yml
Add refresh interval setting to app config                             

edge-apps/powerbi/screenly.yml

  • Introduce refresh_interval setting (minutes)
  • Set default 30, advanced, min explained
+14/-0   
screenly_qc.yml
Add refresh interval to QC config                                               

edge-apps/powerbi/screenly_qc.yml

  • Mirror refresh_interval setting for QC profile
  • Include defaults and help text
+14/-0   

@github-actions
Copy link

github-actions bot commented Dec 3, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Validation

Missing upper-bound and non-integer handling for refresh_interval. Non-positive, extremely large, or fractional inputs (e.g., "0", "100000", "2.5") are not constrained; consider clamping, rounding, and documenting behavior.

function getTokenRefreshInterval() {
  // User provides interval in minutes, convert to seconds
  var intervalMinutes = parseInt(screenly.settings.refresh_interval, 10);
  if (isNaN(intervalMinutes) || intervalMinutes < MIN_TOKEN_REFRESH_MIN) {
    return DEFAULT_TOKEN_REFRESH_MIN * 60; // convert to seconds
  }
  return intervalMinutes * 60; // convert minutes to seconds
}
Consistency

Exponential backoff may permanently increase the next timeout; successful token refresh does not reset tokenRefreshInterval or nextTimeout beyond currentErrorStep = 0. Confirm intended behavior or reset nextTimeout after success.

  async function run() {
    var nextTimeout = tokenRefreshInterval;
    try {
      var newToken = await getEmbedToken();
      await report.setAccessToken(newToken);
      currentErrorStep = 0;
    } catch {
      nextTimeout = Math.min(initErrorDelaySec * Math.pow(2, currentErrorStep), nextTimeout);
      if (currentErrorStep >= maxErrorStep) {
        return;
      }
      currentErrorStep += 1;
    }
    setTimeout(run, nextTimeout * 1000);
  }

  setTimeout(run, tokenRefreshInterval * 1000);
}
Schema Type

Setting refresh_interval is declared as type: string but help_text type: number. Consider aligning to a single numeric type to avoid parsing issues across UIs.

refresh_interval:
  type: string
  default_value: '30'
  title: Refresh Interval
  optional: true
  help_text:
    properties:
      advanced: true
      help_text: |
        Set the interval (in minutes) for periodically refreshing the Power BI dashboard or report.
        The default value is 30 minutes, the minimum allowed is 1 minute.
        Note: The embed token will be refreshed each time the dashboard or report is refreshed.
      type: number
    schema_version: 1

@github-actions
Copy link

github-actions bot commented Dec 3, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Clamp refresh interval range

Guard against extremely large intervals that could overflow setTimeout (max ~24.8
days) and cause no refresh. Add an upper bound in minutes and clamp the value within
[MIN, MAX] before converting to seconds.

edge-apps/powerbi/static/js/main.js [7-14]

 function getTokenRefreshInterval() {
-  // User provides interval in minutes, convert to seconds
+  // User provides interval in minutes, clamp and convert to seconds
   var intervalMinutes = parseInt(screenly.settings.refresh_interval, 10);
-  if (isNaN(intervalMinutes) || intervalMinutes < MIN_TOKEN_REFRESH_MIN) {
-    return DEFAULT_TOKEN_REFRESH_MIN * 60; // convert to seconds
+  var MAX_TOKEN_REFRESH_MIN = 24 * 60; // cap at 24 hours
+  if (isNaN(intervalMinutes)) {
+    intervalMinutes = DEFAULT_TOKEN_REFRESH_MIN;
   }
-  return intervalMinutes * 60; // convert minutes to seconds
+  intervalMinutes = Math.max(MIN_TOKEN_REFRESH_MIN, Math.min(intervalMinutes, MAX_TOKEN_REFRESH_MIN));
+  return intervalMinutes * 60; // seconds
 }
Suggestion importance[1-10]: 6

__

Why: Adding an upper bound prevents excessively large intervals that could delay refresh indefinitely and risk setTimeout limitations. The change is reasonable and accurate but is a defensive improvement rather than a critical fix.

Low
Validate timeout before scheduling

Ensure nextTimeout remains a finite positive number to avoid passing NaN/Infinity to
setTimeout on unexpected errors. Add validation with a safe fallback and cap to a
reasonable maximum.

edge-apps/powerbi/static/js/main.js [48-62]

 async function run() {
   var nextTimeout = tokenRefreshInterval;
   try {
     var newToken = await getEmbedToken();
     await report.setAccessToken(newToken);
     currentErrorStep = 0;
   } catch {
-    nextTimeout = Math.min(initErrorDelaySec * Math.pow(2, currentErrorStep), nextTimeout);
+    var backoff = initErrorDelaySec * Math.pow(2, currentErrorStep);
+    if (!(isFinite(backoff) && backoff > 0)) backoff = initErrorDelaySec;
+    nextTimeout = Math.min(backoff, tokenRefreshInterval);
     if (currentErrorStep >= maxErrorStep) {
       return;
     }
     currentErrorStep += 1;
   }
+  if (!(isFinite(nextTimeout) && nextTimeout > 0)) nextTimeout = DEFAULT_TOKEN_REFRESH_MIN * 60;
   setTimeout(run, nextTimeout * 1000);
 }
Suggestion importance[1-10]: 5

__

Why: Validating nextTimeout adds robustness against unexpected NaN/Infinity values and uses a sensible fallback, but such cases are unlikely here; impact is moderate. The patch aligns with the shown code and maintains behavior.

Low
General
Reflect dynamic setting changes

Recompute the interval each cycle to honor runtime setting changes (e.g., user
updates). Update tokenRefreshInterval inside run before use instead of capturing a
potentially stale value.

edge-apps/powerbi/static/js/main.js [42-65]

 function initTokenRefreshLoop(report) {
   var currentErrorStep = 0;
   var initErrorDelaySec = 15;
   var maxErrorStep = 7;
-  var tokenRefreshInterval = getTokenRefreshInterval();
-...
-  setTimeout(run, tokenRefreshInterval * 1000);
+  function currentIntervalSec() {
+    return getTokenRefreshInterval();
+  }
+  async function run() {
+    var tokenRefreshInterval = currentIntervalSec();
+    var nextTimeout = tokenRefreshInterval;
+    try {
+      var newToken = await getEmbedToken();
+      await report.setAccessToken(newToken);
+      currentErrorStep = 0;
+    } catch {
+      var backoff = initErrorDelaySec * Math.pow(2, currentErrorStep);
+      if (!(isFinite(backoff) && backoff > 0)) backoff = initErrorDelaySec;
+      nextTimeout = Math.min(backoff, tokenRefreshInterval);
+      if (currentErrorStep >= maxErrorStep) {
+        return;
+      }
+      currentErrorStep += 1;
+    }
+    if (!(isFinite(nextTimeout) && nextTimeout > 0)) nextTimeout = DEFAULT_TOKEN_REFRESH_MIN * 60;
+    setTimeout(run, nextTimeout * 1000);
+  }
+  setTimeout(run, currentIntervalSec() * 1000);
 }
Suggestion importance[1-10]: 6

__

Why: Recomputing the interval each run ensures runtime setting updates take effect, improving flexibility without major complexity. It's accurate with the PR context but not strictly necessary for correctness.

Low

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you reformat the code? Let's not touch it until the global config

Copy link
Collaborator Author

@salmanfarisvp salmanfarisvp Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rusko124 to fix the lint errors. D

Would you like me to revert it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@salmanfarisvp
Copy link
Collaborator Author

@rusko124 As we discussed, reverted code that refreshes the token.

salmanfarisvp and others added 4 commits December 12, 2025 15:16
Co-authored-by: rusko124 <psafronov@screenly.io>
Co-authored-by: rusko124 <psafronov@screenly.io>
Co-authored-by: rusko124 <psafronov@screenly.io>
@salmanfarisvp
Copy link
Collaborator Author

salmanfarisvp commented Dec 19, 2025

@rusko124, could you please check again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants