refactor(site): modularize homepage demo runtime#478
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughExtracts homepage demo scenario configuration into a typed registry ( ChangesHomepage Demo Scenario Registry, CSS Scoping, and Locale Sync
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (user switches language)
participant index as index.astro setLanguage
participant syncLocale as syncHomepageDemoLocale
participant registry as homepageDemoScenarios
participant frame as ComponentDemo frame (DOM)
Browser->>index: language toggle event
index->>syncLocale: call(lang, replayLabels, replaySuffix, callbacks)
syncLocale->>registry: lookup replayCardSelector for each replay scenario
syncLocale->>frame: update replay card aria-label
syncLocale->>registry: lookup locale variant src/title per scenario
syncLocale->>frame: query by data-scenario-id
alt src differs from current
syncLocale->>frame: resetDemoProgressDedupe()
syncLocale->>frame: update dataset.src + dataset.title
syncLocale->>frame: frame.reload()
frame-->>syncLocale: reload complete
syncLocale->>frame: syncComponentBackground()
syncLocale->>frame: restoreFeatureDemoProgress() (if restoreProgressOnReload)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/site/src/scripts/component-demo-runtime.ts (1)
80-171:⚠️ Potential issue | 🟡 MinorRemove dead code:
scopeCssandscopeSelectorListare unused.Both functions are defined but never called.
scopeSelectorList(line 73) is only referenced withinscopeCss(line 80), which itself has no callers in the codebase. Since the file imports and usesscopeHomepageDemoCssfrom the shared module instead, these local functions should be deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/site/src/scripts/component-demo-runtime.ts` around lines 80 - 171, The functions scopeCss and scopeSelectorList are dead code that should be removed. Delete both function definitions entirely since scopeCss has no callers in the codebase and scopeSelectorList is only referenced within scopeCss. The file already imports and uses scopeHomepageDemoCss from the shared module instead, making these local functions unnecessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/site/src/components/homepage-demo/host-css.ts`:
- Around line 8-9: The regex pattern `/^\d+%$/` in the condition checking for
keyframe percentages only matches integer values like 50%, but fails to match
decimal percentages like 33.33% or 12.5%, causing them to be incorrectly
prefixed. Update the regex pattern to support optional decimal values by
allowing for a decimal point followed by digits after the initial digits,
ensuring both integer and decimal percentages are correctly recognized in CSS
keyframes.
In `@apps/site/src/pages/index.astro`:
- Around line 2645-2653: The setLanguage() function call at line 2659 executes
synchronously and references syncComponentBackground through the
syncHomepageDemoLocale() call within setLanguage()'s body, but
syncComponentBackground is not declared until line 2784, violating the temporal
dead zone. Move the setLanguage(savedLanguage === 'en' ? 'en' : 'zh') call to
execute after the syncComponentBackground declaration is complete, either by
placing it at the end of the script file after line 2784 or by wrapping it in a
deferred execution mechanism like setTimeout with zero delay to push it to the
end of the event loop.
In `@apps/site/src/scripts/component-demo-runtime.ts`:
- Around line 537-541: The runtime code in the styleNode.textContent assignment
is inconsistently processing CSS compared to ComponentDemo.astro. Instead of
passing both scopedCss and createHomepageDemoHostCss(instanceSelector) through
scopeHomepageDemoCss, modify the code to concatenate them directly without
re-scoping, matching the approach used in ComponentDemo.astro. This eliminates
unnecessary processing since the host CSS selectors already begin with
instanceSelector and will pass through unchanged.
- Around line 1-6: The imports in the component-demo-runtime.ts file are not
sorted according to ESLint rules. Reorder the imports by sorting them
alphabetically by their source path while keeping type imports grouped
separately from regular imports. The type import from scenario-types and the
regular import from host-css need to be reorganized to satisfy the linter's sort
order requirements.
In `@apps/site/src/scripts/homepage-demo-locale.ts`:
- Around line 79-80: The demo host element's accessible name is not being
updated when the localized title changes during a locale switch. After the line
that sets frame.dataset.title to variant.title, add a line to also update the
frame element's aria-label attribute to the same variant.title value. This
ensures that screen reader users will hear the correct localized label when the
language is switched, keeping the accessible name synchronized with the visible
title.
In `@apps/site/src/utils/homepage-demo-src.ts`:
- Around line 1-5: The import statement is importing HomepageDemoScenarioId from
the wrong module. The type HomepageDemoScenarioId is not exported from
../data/homepage-demo-scenarios but rather from
../components/homepage-demo/scenario-types. Remove HomepageDemoScenarioId from
the import statement on the line importing from ../data/homepage-demo-scenarios,
and add a separate import statement to import HomepageDemoScenarioId from
../components/homepage-demo/scenario-types instead.
---
Outside diff comments:
In `@apps/site/src/scripts/component-demo-runtime.ts`:
- Around line 80-171: The functions scopeCss and scopeSelectorList are dead code
that should be removed. Delete both function definitions entirely since scopeCss
has no callers in the codebase and scopeSelectorList is only referenced within
scopeCss. The file already imports and uses scopeHomepageDemoCss from the shared
module instead, making these local functions unnecessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ee2e2011-f151-45df-b74f-e34b1648dc08
📒 Files selected for processing (9)
apps/site/src/components/ComponentDemo.astroapps/site/src/components/homepage-demo/README.mdapps/site/src/components/homepage-demo/host-css.tsapps/site/src/components/homepage-demo/scenario-types.tsapps/site/src/data/homepage-demo-scenarios.tsapps/site/src/pages/index.astroapps/site/src/scripts/component-demo-runtime.tsapps/site/src/scripts/homepage-demo-locale.tsapps/site/src/utils/homepage-demo-src.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (rust)
🧰 Additional context used
🪛 ESLint
apps/site/src/utils/homepage-demo-src.ts
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
apps/site/src/scripts/homepage-demo-locale.ts
[error] 1-8: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 1-4: Replace ⏎····homepageDemoScenarios,⏎····type·HomepageDemoLocale,⏎ with ·homepageDemoScenarios,·type·HomepageDemoLocale·
(prettier/prettier)
[error] 72-72: Replace [data-scenario-id="${scenarioId}"] with ⏎············[data-scenario-id="${scenarioId}"]⏎········
(prettier/prettier)
apps/site/src/scripts/component-demo-runtime.ts
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
🪛 LanguageTool
apps/site/src/components/homepage-demo/README.md
[style] ~7-~7: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... - src/components/ComponentDemo.astro is the compatibility host that still loads...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (6)
apps/site/src/components/homepage-demo/host-css.ts (1)
1-2: LGTM!Also applies to: 32-123, 125-350
apps/site/src/components/homepage-demo/README.md (1)
1-10: LGTM!apps/site/src/components/ComponentDemo.astro (1)
4-9: LGTM!Also applies to: 15-15, 19-19, 78-81, 91-91
apps/site/src/scripts/component-demo-runtime.ts (1)
20-22: LGTM!Also applies to: 607-607
apps/site/src/components/homepage-demo/scenario-types.ts (1)
1-3: LGTM!apps/site/src/data/homepage-demo-scenarios.ts (1)
1-97: LGTM!
| if (trimmed.startsWith('from') || trimmed.startsWith('to') || /^\d+%$/.test(trimmed)) | ||
| return trimmed; |
There was a problem hiding this comment.
Keyframe percentage regex doesn't match decimal values.
The pattern /^\d+%$/ only matches integer percentages (e.g., 50%) but not decimal percentages like 33.33% or 12.5%, which are valid in CSS keyframes. If demo CSS contains decimal percentages, they would be incorrectly prefixed with the instance selector.
Proposed fix
- if (trimmed.startsWith('from') || trimmed.startsWith('to') || /^\d+%$/.test(trimmed))
+ if (trimmed.startsWith('from') || trimmed.startsWith('to') || /^\d+(?:\.\d+)?%$/.test(trimmed))📝 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.
| if (trimmed.startsWith('from') || trimmed.startsWith('to') || /^\d+%$/.test(trimmed)) | |
| return trimmed; | |
| if (trimmed.startsWith('from') || trimmed.startsWith('to') || /^\d+(?:\.\d+)?%$/.test(trimmed)) | |
| return trimmed; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/site/src/components/homepage-demo/host-css.ts` around lines 8 - 9, The
regex pattern `/^\d+%$/` in the condition checking for keyframe percentages only
matches integer values like 50%, but fails to match decimal percentages like
33.33% or 12.5%, causing them to be incorrectly prefixed. Update the regex
pattern to support optional decimal values by allowing for a decimal point
followed by digits after the initial digits, ensuring both integer and decimal
percentages are correctly recognized in CSS keyframes.
| syncHomepageDemoLocale({ | ||
| document, | ||
| lang, | ||
| replayLabels: localizedLabels.replayLabels, | ||
| replaySuffix: localizedLabels.replaySuffix, | ||
| resetDemoProgressDedupe, | ||
| syncComponentBackground, | ||
| restoreFeatureDemoProgress, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify call-site order vs const declaration order.
rg -n "const setLanguage|setLanguage\\(savedLanguage|const syncComponentBackground" apps/site/src/pages/index.astro
sed -n '2555,2668p' apps/site/src/pages/index.astro | nl -ba
sed -n '2778,2810p' apps/site/src/pages/index.astro | nl -baRepository: TouchAI-org/TouchAI
Length of output: 314
🏁 Script executed:
# Get context around the setLanguage definition, call, and syncComponentBackground declaration
sed -n '2555,2800p' apps/site/src/pages/index.astro | head -250Repository: TouchAI-org/TouchAI
Length of output: 12409
🏁 Script executed:
# Get the rest of the context to see where syncComponentBackground and restoreFeatureDemoProgress are fully declared
sed -n '2784,2850p' apps/site/src/pages/index.astroRepository: TouchAI-org/TouchAI
Length of output: 3798
🏁 Script executed:
# Find where restoreFeatureDemoProgress is declared
rg -n "const restoreFeatureDemoProgress|let restoreFeatureDemoProgress" apps/site/src/pages/index.astroRepository: TouchAI-org/TouchAI
Length of output: 45
🏁 Script executed:
# Search more broadly for restoreFeatureDemoProgress
rg -n "restoreFeatureDemoProgress" apps/site/src/pages/index.astro | head -20Repository: TouchAI-org/TouchAI
Length of output: 186
🏁 Script executed:
# Check if setLanguage call at line 2659 is wrapped in any async/deferred mechanism
sed -n '2655,2662p' apps/site/src/pages/index.astroRepository: TouchAI-org/TouchAI
Length of output: 442
setLanguage() throws a ReferenceError immediately on page load due to accessing syncComponentBackground before its declaration.
At line 2659, setLanguage(savedLanguage === 'en' ? 'en' : 'zh') executes synchronously. The function body (lines 2561-2653) references syncComponentBackground at line 2648 when calling syncHomepageDemoLocale(). However, syncComponentBackground is not declared until line 2784, causing a temporal dead zone violation.
💡 Proposed fix (defer initial call until declarations are initialized)
const languageToggle = document.querySelector('.lang-toggle');
const savedLanguage = localStorage.getItem('touchai-language');
-setLanguage(savedLanguage === 'en' ? 'en' : 'zh');
+queueMicrotask(() => {
+ setLanguage(savedLanguage === 'en' ? 'en' : 'zh');
+});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/site/src/pages/index.astro` around lines 2645 - 2653, The setLanguage()
function call at line 2659 executes synchronously and references
syncComponentBackground through the syncHomepageDemoLocale() call within
setLanguage()'s body, but syncComponentBackground is not declared until line
2784, violating the temporal dead zone. Move the setLanguage(savedLanguage ===
'en' ? 'en' : 'zh') call to execute after the syncComponentBackground
declaration is complete, either by placing it at the end of the script file
after line 2784 or by wrapping it in a deferred execution mechanism like
setTimeout with zero delay to push it to the end of the event loop.
| import type { HomepageDemoScenarioId } from '../components/homepage-demo/scenario-types'; | ||
| import { | ||
| createHomepageDemoHostCss, | ||
| createHomepageDemoInstanceSelector, | ||
| scopeHomepageDemoCss, | ||
| } from '../components/homepage-demo/host-css'; |
There was a problem hiding this comment.
Sort imports to satisfy linter.
ESLint reports these imports need to be sorted.
Proposed fix
-import type { HomepageDemoScenarioId } from '../components/homepage-demo/scenario-types';
import {
createHomepageDemoHostCss,
createHomepageDemoInstanceSelector,
scopeHomepageDemoCss,
} from '../components/homepage-demo/host-css';
+import type { HomepageDemoScenarioId } from '../components/homepage-demo/scenario-types';🧰 Tools
🪛 ESLint
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/site/src/scripts/component-demo-runtime.ts` around lines 1 - 6, The
imports in the component-demo-runtime.ts file are not sorted according to ESLint
rules. Reorder the imports by sorting them alphabetically by their source path
while keeping type imports grouped separately from regular imports. The type
import from scenario-types and the regular import from host-css need to be
reorganized to satisfy the linter's sort order requirements.
Source: Linters/SAST tools
| if (styleNode) { | ||
| styleNode.textContent = scopeCss( | ||
| `${scopedCss}\n${hostCssFor(host.dataset.demoId || '')}`, | ||
| `touchai-component-demo[data-demo-id="${host.dataset.demoId || ''}"]` | ||
| styleNode.textContent = scopeHomepageDemoCss( | ||
| `${scopedCss}\n${createHomepageDemoHostCss(instanceSelector)}`, | ||
| instanceSelector | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Inconsistent CSS generation with ComponentDemo.astro.
The runtime passes createHomepageDemoHostCss(instanceSelector) through scopeHomepageDemoCss, but ComponentDemo.astro concatenates them directly without re-scoping the host CSS. While this works (selectors already starting with instanceSelector pass through unchanged), it's unnecessary processing and creates an inconsistency between build-time and runtime behavior.
Proposed fix to match ComponentDemo.astro approach
if (styleNode) {
- styleNode.textContent = scopeHomepageDemoCss(
- `${scopedCss}\n${createHomepageDemoHostCss(instanceSelector)}`,
- instanceSelector
- );
+ styleNode.textContent = `${scopeHomepageDemoCss(scopedCss, instanceSelector)}\n${createHomepageDemoHostCss(instanceSelector)}`;
}📝 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.
| if (styleNode) { | |
| styleNode.textContent = scopeCss( | |
| `${scopedCss}\n${hostCssFor(host.dataset.demoId || '')}`, | |
| `touchai-component-demo[data-demo-id="${host.dataset.demoId || ''}"]` | |
| styleNode.textContent = scopeHomepageDemoCss( | |
| `${scopedCss}\n${createHomepageDemoHostCss(instanceSelector)}`, | |
| instanceSelector | |
| ); | |
| if (styleNode) { | |
| styleNode.textContent = `${scopeHomepageDemoCss(scopedCss, instanceSelector)}\n${createHomepageDemoHostCss(instanceSelector)}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/site/src/scripts/component-demo-runtime.ts` around lines 537 - 541, The
runtime code in the styleNode.textContent assignment is inconsistently
processing CSS compared to ComponentDemo.astro. Instead of passing both
scopedCss and createHomepageDemoHostCss(instanceSelector) through
scopeHomepageDemoCss, modify the code to concatenate them directly without
re-scoping, matching the approach used in ComponentDemo.astro. This eliminates
unnecessary processing since the host CSS selectors already begin with
instanceSelector and will pass through unchanged.
| frame.dataset.src = variant.src; | ||
| frame.dataset.title = variant.title; |
There was a problem hiding this comment.
Keep the demo host aria-label synchronized with localized title.
This updates data-title but not the host’s accessible name. After language switch, screen-reader region labels can stay in the previous locale.
💡 Proposed fix
frame.dataset.src = variant.src;
frame.dataset.title = variant.title;
+ frame.setAttribute('aria-label', variant.title);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/site/src/scripts/homepage-demo-locale.ts` around lines 79 - 80, The demo
host element's accessible name is not being updated when the localized title
changes during a locale switch. After the line that sets frame.dataset.title to
variant.title, add a line to also update the frame element's aria-label
attribute to the same variant.title value. This ensures that screen reader users
will hear the correct localized label when the language is switched, keeping the
accessible name synchronized with the visible title.
| import { | ||
| homepageDemoScenarios, | ||
| type HomepageDemoLocale, | ||
| type HomepageDemoScenarioId, | ||
| } from '../data/homepage-demo-scenarios'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify where HomepageDemoScenarioId is declared/exported vs imported.
rg -n "HomepageDemoScenarioId" \
apps/site/src/components/homepage-demo/scenario-types.ts \
apps/site/src/data/homepage-demo-scenarios.ts \
apps/site/src/utils/homepage-demo-src.tsRepository: TouchAI-org/TouchAI
Length of output: 1043
Import HomepageDemoScenarioId from the defining module, not the data module.
HomepageDemoScenarioId is not exported by ../data/homepage-demo-scenarios (only imported from ../components/homepage-demo/scenario-types), so this import breaks TypeScript symbol resolution.
💡 Proposed fix
import {
homepageDemoScenarios,
type HomepageDemoLocale,
- type HomepageDemoScenarioId,
} from '../data/homepage-demo-scenarios';
+import type { HomepageDemoScenarioId } from '../components/homepage-demo/scenario-types';📝 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.
| import { | |
| homepageDemoScenarios, | |
| type HomepageDemoLocale, | |
| type HomepageDemoScenarioId, | |
| } from '../data/homepage-demo-scenarios'; | |
| import { | |
| homepageDemoScenarios, | |
| type HomepageDemoLocale, | |
| } from '../data/homepage-demo-scenarios'; | |
| import type { HomepageDemoScenarioId } from '../components/homepage-demo/scenario-types'; |
🧰 Tools
🪛 ESLint
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/site/src/utils/homepage-demo-src.ts` around lines 1 - 5, The import
statement is importing HomepageDemoScenarioId from the wrong module. The type
HomepageDemoScenarioId is not exported from ../data/homepage-demo-scenarios but
rather from ../components/homepage-demo/scenario-types. Remove
HomepageDemoScenarioId from the import statement on the line importing from
../data/homepage-demo-scenarios, and add a separate import statement to import
HomepageDemoScenarioId from ../components/homepage-demo/scenario-types instead.
1 participant
Summary
Test Plan
pnpm --filter @touchai/site build