Conversation
WalkthroughThis PR introduces a complete DemoTime browser extension for Chrome and Edge, featuring HTML pages for navigation, scene management, and settings configuration, along with JavaScript modules for theme handling, API integration, CSS styling, and configuration files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExtensionUI as Extension UI
participant ChromeStorage as Chrome Storage
participant DemoTimeAPI as DemoTime API
User->>ExtensionUI: Opens extension popup
ExtensionUI->>ChromeStorage: Load stored demotimeUrl & theme
ChromeStorage-->>ExtensionUI: Return settings
ExtensionUI->>ExtensionUI: Apply theme
ExtensionUI-->>User: Display home page with buttons
User->>ExtensionUI: Click Previous button
ExtensionUI->>ChromeStorage: Read demotimeUrl
ChromeStorage-->>ExtensionUI: Return URL
ExtensionUI->>DemoTimeAPI: GET /api/previous?bringToFront=true
DemoTimeAPI-->>ExtensionUI: Redirect to slide
ExtensionUI->>User: Open slide in new tab
sequenceDiagram
participant User
participant ScenesPage as Scenes Page
participant ChromeStorage as Chrome Storage
participant DemoTimeAPI as DemoTime API
User->>ScenesPage: Navigate to "See all scenes"
ScenesPage->>ChromeStorage: Load demotimeUrl
ChromeStorage-->>ScenesPage: Return URL
ScenesPage->>DemoTimeAPI: GET /api/demos
DemoTimeAPI-->>ScenesPage: Return scenes list
ScenesPage->>ScenesPage: Normalize & flatten scenes
ScenesPage-->>User: Render scene items
User->>ScenesPage: Click scene
ScenesPage->>DemoTimeAPI: GET /api/runById?id=...&bringToFront=true
DemoTimeAPI-->>ScenesPage: Redirect to scene
ScenesPage->>User: Open scene in new tab
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Add DemoTime Browser Extension dev popup with Home, Scenes, and Settings pages that open DemoTime API tabs and persist configuration via
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| link.addEventListener('click', (e) => { | ||
| e.preventDefault(); | ||
| window.location.href = chrome.runtime.getURL('pages/' + href); | ||
| }); |
There was a problem hiding this comment.
Suggestion: Guard navigation targets (check href/id); if missing, skip the handler to avoid broken URLs.
| link.addEventListener('click', (e) => { | |
| e.preventDefault(); | |
| window.location.href = chrome.runtime.getURL('pages/' + href); | |
| }); | |
| if (!href) return; | |
| link.addEventListener('click', (e) => { | |
| e.preventDefault(); | |
| window.location.href = chrome.runtime.getURL('pages/' + href); | |
| }); |
🚀 Want me to fix this? Reply ex: "fix it for me".
| @@ -0,0 +1,124 @@ | |||
| // Scenes page functionality | |||
| export default function main() { | |||
| const scenesList = document.getElementById('scenesList'); | |||
There was a problem hiding this comment.
Suggestion: Null‑check queried elements before calling appendChild, addEventListener, or reading children; early‑return/skip cleanup to prevent TypeErrors.
| const scenesList = document.getElementById('scenesList'); | |
| const scenesList = document.getElementById('scenesList'); | |
| if (!scenesList) { | |
| console.warn('Missing #scenesList element'); | |
| return; | |
| } |
🚀 Want me to fix this? Reply ex: "fix it for me".
| if (document.readyState === 'loading') { | ||
| document.addEventListener('DOMContentLoaded', loadTheme); | ||
| } else { | ||
| loadTheme(); | ||
| } | ||
|
|
||
| // Cleanup function | ||
| return () => { | ||
| // No cleanup needed for theme management | ||
| }; |
There was a problem hiding this comment.
main() adds a DOMContentLoaded listener but never removes it. If called during loading, repeated calls will stack handlers and duplicate loadTheme() runs. Consider storing the handler and removing it in the cleanup.
| if (document.readyState === 'loading') { | |
| document.addEventListener('DOMContentLoaded', loadTheme); | |
| } else { | |
| loadTheme(); | |
| } | |
| // Cleanup function | |
| return () => { | |
| // No cleanup needed for theme management | |
| }; | |
| let onReady; | |
| if (document.readyState === 'loading') { | |
| onReady = () => loadTheme(); | |
| document.addEventListener('DOMContentLoaded', onReady); | |
| } else { | |
| loadTheme(); | |
| } | |
| // Cleanup function | |
| return () => { | |
| if (onReady) document.removeEventListener('DOMContentLoaded', onReady); | |
| }; |
🚀 Want me to fix this? Reply ex: "fix it for me".
| } else { | ||
| themeSelect.value = 'system'; | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
The saved theme isn’t applied on load. Consider calling applyTheme(themeSelect.value) after setting themeSelect.value so the theme takes effect immediately.
| } catch (error) { | |
| applyTheme(themeSelect.value); | |
| } catch (error) { |
🚀 Want me to fix this? Reply ex: "fix it for me".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/browser-extension/manifest.json`:
- Line 5: Update the manifest.json "description" value to use v3 terminology by
replacing the word "slides" with "Scene" (e.g., change "Navigate back to
DemoTime slides in Visual Studio Code" to "Navigate back to DemoTime Scene in
Visual Studio Code") so the user-facing description uses the v3 naming; edit the
"description" field in manifest.json accordingly.
- Around line 22-24: The manifest currently grants "<all_urls>" in
host_permissions which is too broad; update the manifest.json to restrict
host_permissions to only the needed schemes (e.g., "http://*/*" and
"https://*/*") or move "<all_urls>" into optional_host_permissions and request
it at runtime; modify the host_permissions array (referencing manifest.json and
the host_permissions key) to use the tightened patterns matching the
user-configured DemoTime endpoint or implement optional_host_permissions and a
runtime permission request flow in the extension code that contacts the DemoTime
endpoint.
In `@apps/browser-extension/README.md`:
- Line 11: Fix the typo in the README line that currently reads "Overview of all
scenes from you DemoTime..." by changing "you DemoTime" to "your DemoTime" so
the sentence reads "Overview of all scenes from your DemoTime..." (locate the
line containing "Scenes Page: Overview of all scenes from you DemoTime" in
apps/browser-extension/README.md).
- Around line 81-99: Update the README project structure to match the actual
files added in the PR: replace references to navigation.html/navigation.js and
common.css/navigation.css with the real pages and assets (home.html,
scenes.html, settings.html), scripts (home.js, scenes.js, settings.js, theme.js)
and single stylesheet (main.css), while keeping manifest.json and the assets
list; ensure the "pages", "scripts", and "styles" sections list the exact
filenames (home.html, scenes.html, settings.html; home.js, scenes.js,
settings.js, theme.js; main.css) and remove the outdated navigation/common
entries so the README accurately reflects the repository.
In `@apps/browser-extension/styles/main.css`:
- Around line 50-61: The dark theme block [data-theme="dark"] is missing the CSS
variable --color-demo-time-black, causing fallbacks to the light value; add a
--color-demo-time-black declaration inside [data-theme="dark"] (matching the
intended dark-mode black used elsewhere, e.g., a near-black hex) so dark mode
contexts referencing that variable use the correct dark value instead of falling
back to :root.
🧹 Nitpick comments (10)
apps/browser-extension/scripts/theme.js (1)
40-49: Avoid dangling DOMContentLoaded listener.Line 40-49 adds a listener but the cleanup is empty. If
main()is ever invoked multiple times, this can accumulate listeners. Consider{ once: true }and/or removing it in cleanup.♻️ Suggested adjustment
export default function main() { + const onReady = () => loadTheme(); // Initialize theme on page load if (document.readyState === 'loading') { - document.addEventListener('DOMContentLoaded', loadTheme); + document.addEventListener('DOMContentLoaded', onReady, { once: true }); } else { - loadTheme(); + onReady(); } // Cleanup function return () => { - // No cleanup needed for theme management + document.removeEventListener('DOMContentLoaded', onReady); }; }apps/browser-extension/scripts/settings.js (1)
11-19: Reuse shared theme helper from theme.js.Line 11-19 duplicates the logic already exported from
apps/browser-extension/scripts/theme.js. Import and reuseapplyTheme(orsaveTheme) to keep behavior consistent.♻️ Suggested refactor
+import { applyTheme } from './theme.js'; + export default function main() { const urlInput = document.getElementById('urlInput'); const themeSelect = document.getElementById('themeSelect'); const saveBtn = document.getElementById('saveBtn'); const errorMsg = document.getElementById('errorMsg'); // Setup navbar navigation setupNavbar(); - // Apply theme function - const applyTheme = (theme) => { - const root = document.documentElement; - if (theme === 'system') { - root.removeAttribute('data-theme'); - } else { - root.setAttribute('data-theme', theme); - } - }; + // applyTheme imported from theme.jsapps/browser-extension/scripts/home.js (1)
65-75: Consider extractingsetupNavbarinto a shared helper.Line 65-75 duplicates the same navigation wiring found in settings/scenes. A small shared module would reduce duplication and make it easier to update navigation behavior consistently.
apps/browser-extension/scripts/scenes.js (3)
17-17: Remove debug console.log statements before release.Multiple
console.logstatements (Lines 17, 28, 33, 41-42) appear to be development artifacts. Consider removing or gating them behind a debug flag for production.Also applies to: 28-28, 33-33, 41-42
48-57: Confusing variable naming:scenesList_datashadows the DOM elementscenesList.The variable
scenesList_datais misleading becausescenesListalready refers to the DOM element. Using a clearer name likescenesDataordemosArraywould improve readability.♻️ Suggested rename
- let scenesList_data = scenes; + let scenesData = scenes; if (!Array.isArray(scenes)) { // Check if it's an object with demos/scenes/data property - scenesList_data = scenes.demos || scenes.scenes || scenes.data || scenes; + scenesData = scenes.demos || scenes.scenes || scenes.data || scenes; // If still not an array, try to convert object to array - if (!Array.isArray(scenesList_data)) { - scenesList_data = Object.values(scenes); + if (!Array.isArray(scenesData)) { + scenesData = Object.values(scenes); } } - if (!scenesList_data || scenesList_data.length === 0) { + if (!scenesData || scenesData.length === 0) { scenesList.innerHTML = '<div class="loading-message">No scenes available</div>'; return; } // Flatten the scenes by iterating through demo groups and their children - scenesList_data.forEach(demoGroup => { + scenesData.forEach(demoGroup => {
110-111: Cleanup function doesn't remove event listeners.Event listeners are added to
sceneItemelements (Line 94) andnavLinks(Line 119), but the cleanup function returns a no-op. While these listeners are scoped to elements that get removed on page unload, ifmain()is called multiple times (e.g., SPA-style navigation), listeners could accumulate.apps/browser-extension/demotime-custom.css (1)
89-95: Avoid!importantwith@applydirectives.Using
!importantin Tailwind@applyrules (Lines 90, 93) is a code smell that indicates specificity issues. Consider restructuring the CSS selectors or using more specific targeting instead.apps/browser-extension/styles/main.css (3)
64-66: Universal transition selector may impact performance.Applying transitions to
*affects every element and pseudo-element, which can cause layout thrashing during theme switches. For a small extension popup this is likely acceptable, but consider scoping transitions to specific elements that need them (e.g.,.nav-icon,.scene-item,.btn-*) if you notice jank.
346-360: Consider adding explicit focus styles for keyboard navigation.The
.btn-primaryand.btn-secondarybuttons have:hoverand:activestates but no explicit:focusor:focus-visiblestyles. Keyboard users may not have a visible focus indicator beyond the browser default.Proposed addition
.btn-primary:focus-visible, .btn-secondary:focus-visible { outline: 2px solid var(--color-demo-time-accent); outline-offset: 2px; }
528-551: WebKit-only scrollbar styles - Firefox will show default scrollbar.The
::-webkit-scrollbarpseudo-elements only work in Chromium-based browsers. If you plan to support Firefox as a target browser for this extension, consider adding standard scrollbar properties.Firefox scrollbar support
.scenes-list { /* Standard properties for Firefox */ scrollbar-width: thin; scrollbar-color: rgba(209, 213, 219, 0.8) transparent; } [data-theme="dark"] .scenes-list { scrollbar-color: rgba(75, 85, 99, 0.8) transparent; }
| "manifest_version": 3, | ||
| "name": "DemoTime Browser Extension", | ||
| "version": "1.0.0", | ||
| "description": "Navigate back to DemoTime slides in Visual Studio Code", |
There was a problem hiding this comment.
Align manifest description with v3 terminology.
Line 5 says “slides”; v3 uses “Scene”. Please update the wording for consistency. Based on learnings, use the v3 naming in user‑facing text.
✏️ Suggested wording
- "description": "Navigate back to DemoTime slides in Visual Studio Code",
+ "description": "Navigate back to DemoTime Scenes in Visual Studio Code",📝 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.
| "description": "Navigate back to DemoTime slides in Visual Studio Code", | |
| "description": "Navigate back to DemoTime Scenes in Visual Studio Code", |
🤖 Prompt for AI Agents
In `@apps/browser-extension/manifest.json` at line 5, Update the manifest.json
"description" value to use v3 terminology by replacing the word "slides" with
"Scene" (e.g., change "Navigate back to DemoTime slides in Visual Studio Code"
to "Navigate back to DemoTime Scene in Visual Studio Code") so the user-facing
description uses the v3 naming; edit the "description" field in manifest.json
accordingly.
| "host_permissions": [ | ||
| "<all_urls>" | ||
| ] |
There was a problem hiding this comment.
Reduce host permissions scope.
Line 22-24 grants <all_urls>, which is broader than necessary and weakens least‑privilege. Since the extension only calls a user‑configured DemoTime endpoint, restrict to http/https wildcards (or move to optional_host_permissions with a runtime request).
🔒 Proposed tightening
- "host_permissions": [
- "<all_urls>"
- ]
+ "host_permissions": [
+ "https://*/*",
+ "http://*/*"
+ ]📝 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.
| "host_permissions": [ | |
| "<all_urls>" | |
| ] | |
| "host_permissions": [ | |
| "https://*/*", | |
| "http://*/*" | |
| ] |
🤖 Prompt for AI Agents
In `@apps/browser-extension/manifest.json` around lines 22 - 24, The manifest
currently grants "<all_urls>" in host_permissions which is too broad; update the
manifest.json to restrict host_permissions to only the needed schemes (e.g.,
"http://*/*" and "https://*/*") or move "<all_urls>" into
optional_host_permissions and request it at runtime; modify the host_permissions
array (referencing manifest.json and the host_permissions key) to use the
tightened patterns matching the user-configured DemoTime endpoint or implement
optional_host_permissions and a runtime permission request flow in the extension
code that contacts the DemoTime endpoint.
|
|
||
| - **Navigation Page**: Quick access to return to your DemoTime presentation with a single click (previous or next scene) | ||
| - **Settings Page**: Configure your DemoTime API URL | ||
| - **Scenes Page**: Overview of all scenes from you DemoTime where you can navigate to |
There was a problem hiding this comment.
Typo: "from you DemoTime" → "from your DemoTime".
✏️ Fix
-- **Scenes Page**: Overview of all scenes from you DemoTime where you can navigate to
+- **Scenes Page**: Overview of all scenes from your DemoTime where you can navigate to📝 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.
| - **Scenes Page**: Overview of all scenes from you DemoTime where you can navigate to | |
| - **Scenes Page**: Overview of all scenes from your DemoTime where you can navigate to |
🧰 Tools
🪛 LanguageTool
[grammar] ~11-~11: Ensure spelling is correct
Context: ...nes Page**: Overview of all scenes from you DemoTime where you can navigate to - **...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@apps/browser-extension/README.md` at line 11, Fix the typo in the README line
that currently reads "Overview of all scenes from you DemoTime..." by changing
"you DemoTime" to "your DemoTime" so the sentence reads "Overview of all scenes
from your DemoTime..." (locate the line containing "Scenes Page: Overview of all
scenes from you DemoTime" in apps/browser-extension/README.md).
| ``` | ||
| demotime-browser-extension/ | ||
| ├── manifest.json # Extension configuration | ||
| ├── assets/ # Logo images | ||
| │ ├── demotime.png | ||
| │ ├── logo-16.png | ||
| │ ├── logo-48.png | ||
| │ └── logo-128.png | ||
| ├── pages/ # HTML pages | ||
| │ ├── navigation.html # Main navigation page | ||
| │ └── settings.html # Settings page | ||
| ├── scripts/ # JavaScript files | ||
| │ ├── navigation.js # Navigation logic | ||
| │ └── settings.js # Settings logic | ||
| └── styles/ # CSS files | ||
| ├── common.css # Shared styles | ||
| ├── navigation.css # Navigation page styles | ||
| └── settings.css # Settings page styles | ||
| ``` |
There was a problem hiding this comment.
Project structure is outdated/inconsistent with actual files.
The documented structure shows navigation.html, navigation.js, common.css, etc., but the actual PR adds home.html, scenes.html, home.js, scenes.js, main.css, etc. Update the structure to reflect the actual implementation.
📁 Suggested project structure update
demotime-browser-extension/
├── manifest.json # Extension configuration
├── assets/ # Logo images
│ ├── demotime.png
│ ├── logo-16.png
│ ├── logo-48.png
│ └── logo-128.png
├── pages/ # HTML pages
-│ ├── navigation.html # Main navigation page
-│ └── settings.html # Settings page
+│ ├── home.html # Home page with next/previous navigation
+│ ├── scenes.html # All scenes overview
+│ └── settings.html # Settings page
├── scripts/ # JavaScript files
-│ ├── navigation.js # Navigation logic
-│ └── settings.js # Settings logic
+│ ├── home.js # Home page logic
+│ ├── scenes.js # Scenes page logic
+│ ├── settings.js # Settings logic
+│ └── theme.js # Theme management
└── styles/ # CSS files
- ├── common.css # Shared styles
- ├── navigation.css # Navigation page styles
- └── settings.css # Settings page styles
- └── main.css # Main styles
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@apps/browser-extension/README.md` around lines 81 - 99, Update the README
project structure to match the actual files added in the PR: replace references
to navigation.html/navigation.js and common.css/navigation.css with the real
pages and assets (home.html, scenes.html, settings.html), scripts (home.js,
scenes.js, settings.js, theme.js) and single stylesheet (main.css), while
keeping manifest.json and the assets list; ensure the "pages", "scripts", and
"styles" sections list the exact filenames (home.html, scenes.html,
settings.html; home.js, scenes.js, settings.js, theme.js; main.css) and remove
the outdated navigation/common entries so the README accurately reflects the
repository.
| [data-theme="dark"] { | ||
| --color-demo-time-accent-low: #1a1815; | ||
| --color-demo-time-accent: #ffd43b; | ||
| --color-demo-time-accent-high: #ffd43b; | ||
| --color-demo-time-gray-1: #1e1e1e; | ||
| --color-demo-time-gray-2: #2d2d30; | ||
| --color-demo-time-gray-3: #3e3e42; | ||
| --color-demo-time-gray-4: #565656; | ||
| --color-demo-time-gray-5: #6e6e6e; | ||
| --color-demo-time-gray-6: #8e8e8e; | ||
| --color-demo-time-gray-7: #cccccc; | ||
| } |
There was a problem hiding this comment.
Missing --color-demo-time-black in dark theme.
The [data-theme="dark"] block is missing the --color-demo-time-black variable that exists in other theme definitions. If this variable is referenced in dark mode contexts, it will fall back to the :root light mode value.
Proposed fix
[data-theme="dark"] {
--color-demo-time-accent-low: `#1a1815`;
--color-demo-time-accent: `#ffd43b`;
--color-demo-time-accent-high: `#ffd43b`;
--color-demo-time-gray-1: `#1e1e1e`;
--color-demo-time-gray-2: `#2d2d30`;
--color-demo-time-gray-3: `#3e3e42`;
--color-demo-time-gray-4: `#565656`;
--color-demo-time-gray-5: `#6e6e6e`;
--color-demo-time-gray-6: `#8e8e8e`;
--color-demo-time-gray-7: `#cccccc`;
+ --color-demo-time-black: `#ffffff`;
}📝 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.
| [data-theme="dark"] { | |
| --color-demo-time-accent-low: #1a1815; | |
| --color-demo-time-accent: #ffd43b; | |
| --color-demo-time-accent-high: #ffd43b; | |
| --color-demo-time-gray-1: #1e1e1e; | |
| --color-demo-time-gray-2: #2d2d30; | |
| --color-demo-time-gray-3: #3e3e42; | |
| --color-demo-time-gray-4: #565656; | |
| --color-demo-time-gray-5: #6e6e6e; | |
| --color-demo-time-gray-6: #8e8e8e; | |
| --color-demo-time-gray-7: #cccccc; | |
| } | |
| [data-theme="dark"] { | |
| --color-demo-time-accent-low: `#1a1815`; | |
| --color-demo-time-accent: `#ffd43b`; | |
| --color-demo-time-accent-high: `#ffd43b`; | |
| --color-demo-time-gray-1: `#1e1e1e`; | |
| --color-demo-time-gray-2: `#2d2d30`; | |
| --color-demo-time-gray-3: `#3e3e42`; | |
| --color-demo-time-gray-4: `#565656`; | |
| --color-demo-time-gray-5: `#6e6e6e`; | |
| --color-demo-time-gray-6: `#8e8e8e`; | |
| --color-demo-time-gray-7: `#cccccc`; | |
| --color-demo-time-black: `#ffffff`; | |
| } |
🤖 Prompt for AI Agents
In `@apps/browser-extension/styles/main.css` around lines 50 - 61, The dark theme
block [data-theme="dark"] is missing the CSS variable --color-demo-time-black,
causing fallbacks to the light value; add a --color-demo-time-black declaration
inside [data-theme="dark"] (matching the intended dark-mode black used
elsewhere, e.g., a near-black hex) so dark mode contexts referencing that
variable use the correct dark value instead of falling back to :root.
1 participant
Description
Added DemoTime Browser extension (still in dev) to apps folders
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.