New App - Klipfolio Power Metrics Dashboard with Passcode Access#238
New App - Klipfolio Power Metrics Dashboard with Passcode Access#238salmanfarisvp wants to merge 1 commit intomasterfrom
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new edge app for Screenly that embeds a Klipfolio Power Metrics dashboard with passcode-protected access. The app attempts to load a dashboard via iframe and automatically inject a passcode to bypass the authentication prompt.
Changes:
- New Klipfolio edge app with HTML, JavaScript, and CSS files for iframe-based dashboard embedding
- Configuration files (screenly.yml and instance.yml) to define app settings and metadata
- JavaScript logic to handle CORS proxy URLs and attempt automated passcode injection into the iframe
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| index.html | HTML skeleton with iframe element and script/style links |
| script.js | JavaScript for iframe initialization, CORS proxy handling, and passcode injection attempt |
| styles.css | Full-screen CSS styling for iframe display |
| screenly.yml | App manifest defining settings for dashboard URL and passcode |
| instance.yml | Instance configuration with app ID and name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <link rel="stylesheet" href="styles.css"> | ||
| </head> | ||
| <body> | ||
| <iframe id="dashboard" title="Klipfolio Dashboard"></iframe> | ||
| <script src="screenly.js?version=1" defer></script> | ||
| <script src="script.js"></script> |
There was a problem hiding this comment.
The CSS and JavaScript files should be organized in a static/ folder structure following the codebase convention. All other edge apps in this repository (iframe, clock, weather, etc.) use static/css/, static/js/, and static/images/ subdirectories rather than placing assets in the root directory. This improves organization and maintainability.
| <link rel="stylesheet" href="styles.css"> | |
| </head> | |
| <body> | |
| <iframe id="dashboard" title="Klipfolio Dashboard"></iframe> | |
| <script src="screenly.js?version=1" defer></script> | |
| <script src="script.js"></script> | |
| <link rel="stylesheet" href="static/css/styles.css"> | |
| </head> | |
| <body> | |
| <iframe id="dashboard" title="Klipfolio Dashboard"></iframe> | |
| <script src="static/js/screenly.js?version=1" defer></script> | |
| <script src="static/js/script.js"></script> |
| <body> | ||
| <iframe id="dashboard" title="Klipfolio Dashboard"></iframe> | ||
| <script src="screenly.js?version=1" defer></script> | ||
| <script src="script.js"></script> |
There was a problem hiding this comment.
The custom script should be loaded with the defer attribute. Following the codebase convention seen in other edge apps, scripts that depend on screenly.js should use defer to ensure proper loading order and prevent race conditions where the script tries to access the screenly object before it's available.
| <script src="script.js"></script> | |
| <script src="script.js" defer></script> |
| /* This file is no longer needed as all styles are defined in the HTML */ | ||
| /* The file is kept for reference in case styles need to be extracted later */ |
There was a problem hiding this comment.
The inline comment states this CSS file is no longer needed and kept for reference, but the file is still linked in index.html. Either remove the misleading comment and use this file properly, or remove the file and the link tag. Having a "placeholder" file that claims to not be used creates confusion and technical debt.
| /* This file is no longer needed as all styles are defined in the HTML */ | |
| /* The file is kept for reference in case styles need to be extracted later */ | |
| /* Styles for the Klipfolio Power Metrics dashboard layout */ | |
| /* Defines full-screen behavior for the page and embedded dashboard iframe */ |
| button.click() | ||
| console.log("Clicked submit button") |
There was a problem hiding this comment.
Missing null check for the button element before calling click(). If the submit button with id 'submit' doesn't exist in the iframe document (line 59), the code will throw an error when trying to call button.click() on line 60. Add a null check similar to the one used for the input field.
| button.click() | |
| console.log("Clicked submit button") | |
| if (button) { | |
| button.click() | |
| console.log("Clicked submit button") | |
| } else { | |
| console.log("Submit button not found"); | |
| } |
|
|
||
| // Attempt to inject passcode after iframe loads | ||
| dashboard.onload = function() { | ||
| setTimeout(injectPasscodeIntoIframe, 1000) |
There was a problem hiding this comment.
The script does not call screenly.signalReadyForRendering() to indicate when the app is ready for display. When ready_signal is set to true in the manifest, the app should call this method after the iframe loads successfully. This helps Screenly know when to display the content. Reference the iframe edge app implementation which calls this method in the iframe's onload handler.
| setTimeout(injectPasscodeIntoIframe, 1000) | |
| setTimeout(function() { | |
| // First, try to inject the passcode | |
| injectPasscodeIntoIframe() | |
| // Then signal to Screenly that the app is ready for rendering | |
| try { | |
| if (typeof screenly !== 'undefined' && | |
| typeof screenly.signalReadyForRendering === 'function') { | |
| screenly.signalReadyForRendering() | |
| } | |
| } catch (e) { | |
| console.error("Error calling screenly.signalReadyForRendering:", e) | |
| } | |
| }, 1000) |
| optional: true | ||
| help_text: The URL for the dashboard | ||
| dashboard_passcode: | ||
| type: secret | ||
| title: Dashboard Passcode | ||
| optional: true |
There was a problem hiding this comment.
The dashboard_url and dashboard_passcode settings are marked as optional, but the code does not handle the case where they are not provided. In script.js lines 4-5, fallback messages are set ('Dashboard URL not set', 'Passcode not set'), but these will be used as actual values rather than triggering an error. Since the app cannot function without these values, they should be marked as 'optional: false' in the manifest.
| optional: true | |
| help_text: The URL for the dashboard | |
| dashboard_passcode: | |
| type: secret | |
| title: Dashboard Passcode | |
| optional: true | |
| optional: false | |
| help_text: The URL for the dashboard | |
| dashboard_passcode: | |
| type: secret | |
| title: Dashboard Passcode | |
| optional: false |
| function injectPasscodeIntoIframe() { | ||
| try { | ||
| const iframe = document.getElementById('dashboard') | ||
|
|
||
| // Try to access the iframe document | ||
| let iframeDoc; | ||
| try { | ||
| iframeDoc = iframe.contentDocument || iframe.contentWindow.document | ||
| } catch (e) { | ||
| console.error("Cannot access iframe content due to cross-origin restrictions", e) | ||
| return | ||
| } | ||
|
|
||
| // Get passcode input | ||
| const input = iframeDoc.getElementById('passcode') | ||
|
|
||
| // If passcode input exists, inject passcode and submit | ||
| if (input) { | ||
| console.log("Found passcode field, injecting code"); | ||
| input.value = passcode | ||
| input.dispatchEvent(new Event('input', { bubbles: true })) | ||
|
|
||
| // Get the submit button and click it | ||
| const button = iframeDoc.getElementById('submit') | ||
| button.click() | ||
| console.log("Clicked submit button") | ||
| } else { | ||
| console.log("Passcode input field not found"); | ||
| } | ||
| } catch (e) { | ||
| console.error("Error in injectPasscodeIntoIframe:", e) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The approach of injecting passcode into an iframe will not work due to cross-origin security restrictions. Lines 43-46 attempt to catch cross-origin errors, but even if the iframe loads through the CORS proxy, the Same-Origin Policy prevents accessing or manipulating the iframe's DOM from the parent window. This fundamental security restriction means the passcode injection functionality (lines 36-68) cannot work as designed. The TODO comment in the PR description acknowledges CORS and iframe issues that need to be resolved.
| // Wait a short time for screenly.js to load | ||
| setTimeout(function() { | ||
| // Get the dashboard iframe | ||
| const dashboard = document.getElementById('dashboard') | ||
|
|
||
| // Construct the URL after screenly.js has loaded | ||
| let dashboardUrl | ||
| try { | ||
| // Try to use Screenly's CORS proxy | ||
| dashboardUrl = screenly.cors_proxy_url + encodeURIComponent(dashboardUrlRaw) | ||
| } catch (e) { | ||
| // Fallback to direct URL if screenly is not available | ||
| console.error("Error accessing screenly.cors_proxy_url, using direct URL", e) | ||
| dashboardUrl = dashboardUrlRaw; | ||
| } | ||
|
|
||
| // Set dashboard URL | ||
| dashboard.src = dashboardUrl | ||
|
|
||
| // Attempt to inject passcode after iframe loads | ||
| dashboard.onload = function() { | ||
| setTimeout(injectPasscodeIntoIframe, 1000) | ||
| }; | ||
| }, 500) // 500ms delay for screenly.js to load |
There was a problem hiding this comment.
The 500ms delay for screenly.js to load is arbitrary and may cause race conditions. The screenly.js script is loaded with the defer attribute, which means it will execute after the DOM is parsed but before the load event. Using a setTimeout with a fixed delay is unreliable. Instead, listen for a screenly-specific event or check for the existence of the screenly object, or move the script loading to use proper defer attributes as seen in other edge apps.
| // Wait a short time for screenly.js to load | |
| setTimeout(function() { | |
| // Get the dashboard iframe | |
| const dashboard = document.getElementById('dashboard') | |
| // Construct the URL after screenly.js has loaded | |
| let dashboardUrl | |
| try { | |
| // Try to use Screenly's CORS proxy | |
| dashboardUrl = screenly.cors_proxy_url + encodeURIComponent(dashboardUrlRaw) | |
| } catch (e) { | |
| // Fallback to direct URL if screenly is not available | |
| console.error("Error accessing screenly.cors_proxy_url, using direct URL", e) | |
| dashboardUrl = dashboardUrlRaw; | |
| } | |
| // Set dashboard URL | |
| dashboard.src = dashboardUrl | |
| // Attempt to inject passcode after iframe loads | |
| dashboard.onload = function() { | |
| setTimeout(injectPasscodeIntoIframe, 1000) | |
| }; | |
| }, 500) // 500ms delay for screenly.js to load | |
| // Get the dashboard iframe | |
| const dashboard = document.getElementById('dashboard') | |
| // Construct the URL after screenly.js has loaded | |
| let dashboardUrl | |
| try { | |
| // Try to use Screenly's CORS proxy | |
| dashboardUrl = screenly.cors_proxy_url + encodeURIComponent(dashboardUrlRaw) | |
| } catch (e) { | |
| // Fallback to direct URL if screenly is not available | |
| console.error("Error accessing screenly.cors_proxy_url, using direct URL", e) | |
| dashboardUrl = dashboardUrlRaw; | |
| } | |
| // Set dashboard URL | |
| dashboard.src = dashboardUrl | |
| // Attempt to inject passcode after iframe loads | |
| dashboard.onload = function() { | |
| setTimeout(injectPasscodeIntoIframe, 1000) | |
| }; |
| const dashboardUrlRaw = screenly.settings.dashboard_url || 'Dashboard URL not set' | ||
| const passcode = screenly.settings.dashboard_passcode || 'Passcode not set' |
There was a problem hiding this comment.
The settings are accessed at the module level (lines 4-5) before the window load event, which may execute before screenly.js has fully initialized. This could result in undefined values or errors. The settings should be accessed inside the event handler after ensuring screenly is available, similar to how they're accessed on line 18 within the try-catch block.
| /* global screenly, Sentry */ | ||
| /* eslint-disable-next-line no-unused-vars, no-useless-catch */ |
There was a problem hiding this comment.
The eslint-disable comment on line 2 references 'no-unused-vars' and 'no-useless-catch', but there are no try-catch blocks that simply rethrow errors in this file, making 'no-useless-catch' unnecessary. Additionally, the Sentry global is declared but never used in the code. Either remove the unused eslint-disable rules or implement the Sentry logging mentioned in the PR TODO list.
User description
Access Privated Passcode enabled - Klipfolio Power Metrics Dashboard over Edge App.
ToDo:
PR Type
Enhancement
Description
Add Klipfolio dashboard HTML with iframe
Implement passcode injection and proxy logic
Provide full-screen CSS styling
Add Screenly manifest and instance configurations
Changes walkthrough 📝
styles.css
Add full-screen styling CSSedge-apps/klipfolio-power-metrics-dashboard/styles.css
script.js
Implement iframe URL and passcode injectionedge-apps/klipfolio-power-metrics-dashboard/script.js
index.html
Add HTML skeleton with iframeedge-apps/klipfolio-power-metrics-dashboard/index.html
instance.yml
Add app instance configurationedge-apps/klipfolio-power-metrics-dashboard/instance.yml
screenly.yml
Add Screenly manifest settingsedge-apps/klipfolio-power-metrics-dashboard/screenly.yml