Add reveal plugin #917
Add reveal plugin #917
Conversation
✅ Deploy Preview for livecodes ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a Reveal.js SDK integration: new SDK module, tests, build targets, package export and dependencies, documentation and sidebar entry. Integration discovers Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Reveal as Reveal.js
participant LiveCodes as LiveCodes SDK
participant Container as [data-livecodes]<br/>Container
participant Playground as createPlayground()
participant IFrame as iframe.livecodes
Note over LiveCodes,Reveal: Init and config discovery
Reveal->>LiveCodes: init(deck)
LiveCodes->>Reveal: deck.getConfig()
Reveal-->>LiveCodes: globalConfig
LiveCodes->>Container: querySelectorAll([data-livecodes])
loop per container
Container->>LiveCodes: read data-config (JSON)
LiveCodes->>Playground: createPlayground(mergedConfig)
Playground-->>LiveCodes: Promise<Playground>
end
LiveCodes->>LiveCodes: Promise.all(playgroundPromises)
alt all succeed
LiveCodes->>IFrame: apply default styles + customStyle
opt sdkReady provided
LiveCodes->>Playground: sdkReady(playground)
end
LiveCodes->>Window: attach window.LiveCodes
else failure
LiveCodes-->>Reveal: propagate or log error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
src/sdk/reveal.ts (1)
19-24: Replace @ts-ignore with proper typing.Using
@ts-ignoresuppresses type checking and can hide type errors. The style property assignment can be properly typed.Apply this diff:
const initIframeStyle = (iframe: HTMLIFrameElement, styles: Partial<CSSStyleDeclaration>) => { for (const [key, value] of Object.entries(styles)) { - // @ts-ignore - iframe.style[key as any] = value; + if (value !== undefined) { + iframe.style.setProperty(key, String(value)); + } } };Alternatively, if you prefer direct assignment:
const initIframeStyle = (iframe: HTMLIFrameElement, styles: Partial<CSSStyleDeclaration>) => { - for (const [key, value] of Object.entries(styles)) { - // @ts-ignore - iframe.style[key as any] = value; - } + Object.assign(iframe.style, styles); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
docs/docs/languages/javascript.mdx(1 hunks)docs/docs/sdk/reveal.mdx(1 hunks)docs/sidebars.ts(1 hunks)package.json(3 hunks)scripts/build.js(1 hunks)src/sdk/__tests__/reveal.test.ts(1 hunks)src/sdk/package.sdk.json(1 hunks)src/sdk/reveal.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sdk/__tests__/reveal.test.ts (1)
src/sdk/index.ts (1)
createPlayground(33-375)
src/sdk/reveal.ts (1)
src/sdk/index.ts (1)
createPlayground(33-375)
🪛 ast-grep (0.39.9)
src/sdk/__tests__/reveal.test.ts
[warning] 13-13: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 LanguageTool
docs/docs/sdk/reveal.mdx
[style] ~7-~7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ds directly within your slides. It has a very simple [implementation](https://github.com/liv...
(EN_WEAK_ADJECTIVE)
⏰ 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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
🔇 Additional comments (5)
docs/sidebars.ts (1)
88-88: LGTM!The navigation entry for the Reveal.js SDK documentation is correctly added to the sidebar configuration.
scripts/build.js (1)
148-160: LGTM!The build configuration correctly adds two output formats for the Reveal.js integration:
reveal.umd.js(IIFE format for browser globals/UMD)reveal.js(default ESM format)This follows the existing pattern used for other SDK builds.
src/sdk/__tests__/reveal.test.ts (2)
13-16: Static analysis warning can be safely ignored.The static analysis tool flags
document.body.innerHTML = ''as a potential XSS risk. However, this is a test cleanup operation using a static empty string, which is completely safe. The warning is a false positive in this context.
37-42: Verify test expectations match the fixed config merging logic.These test assertions verify that both
script(from local config) andmarkup(from global config) are present in the merged configuration. However, the current implementation insrc/sdk/reveal.ts(line 40) has a config merging bug that may cause these assertions to pass incorrectly or fail unexpectedly.After applying the fix suggested in the
src/sdk/reveal.tsreview comment (correcting the config merging logic), please re-run these tests to ensure they still pass and accurately validate the intended behavior.The test expectations appear correct for the desired behavior—just ensure the implementation matches.
package.json (1)
94-94: No issues found with reveal.js version.The latest reveal.js version is 5.2.1, which matches the PR. Known vulnerabilities affect only versions < 4.3.0 and <= 3.9.1, so the specified version is secure.
docs/docs/languages/javascript.mdx
Outdated
| # JavaScript | ||
|
|
||
| TODO... | ||
| TODO... |
There was a problem hiding this comment.
Complete or remove placeholder documentation.
This file contains only "TODO..." which appears to be incomplete. If this is unrelated to the Reveal.js integration PR, consider removing this change from the PR to keep it focused.
If you need the JavaScript documentation completed, I can help generate appropriate content for this page.
🤖 Prompt for AI Agents
In docs/docs/languages/javascript.mdx lines 1-3, the file contains only a
placeholder "TODO..." which is incomplete and unrelated to the Reveal.js
integration PR; either remove this file change from the PR or replace the
placeholder with minimal, relevant JavaScript documentation for the docs site.
If removing: revert the file or remove it from the commit/PR. If keeping:
replace the placeholder with a short page summary describing the JavaScript
language docs purpose, primary supported features, and links to related pages
(e.g., setup, examples, Reveal.js notes) so the page is meaningful and
review-ready.
src/sdk/reveal.ts
Outdated
| const promises = containers.map((container) => { | ||
| const localOptions = container.dataset.config || "{}"; | ||
| const finalOptions = { config: { ...globalOptions, ...JSON.parse(localOptions) } } | ||
| return createPlayground(container, finalOptions); |
There was a problem hiding this comment.
Fix incorrect config merging logic.
The config merging on lines 39-40 has a critical bug. globalOptions is of type LiveOptions which extends EmbedOptions and may already contain a nested config property. Simply spreading globalOptions and the parsed localOptions into a new config object will result in incorrect nesting.
For example, if globalOptions = { config: { theme: 'dark' }, loading: 'eager' }, the current code produces:
{ config: { config: { theme: 'dark' }, loading: 'eager', ...localOptions } }Apply this diff to correctly merge configurations:
const promises = containers.map((container) => {
const localOptions = container.dataset.config || "{}";
- const finalOptions = { config: { ...globalOptions, ...JSON.parse(localOptions) } }
+ let parsedLocalOptions: LiveOptions = {};
+ try {
+ parsedLocalOptions = JSON.parse(localOptions);
+ } catch (e) {
+ console.error('Failed to parse data-config:', e);
+ }
+ const finalOptions: EmbedOptions = {
+ ...globalOptions,
+ ...parsedLocalOptions,
+ config: {
+ ...globalOptions.config,
+ ...parsedLocalOptions.config,
+ }
+ };
return createPlayground(container, finalOptions);
});This also adds error handling for invalid JSON in data-config attributes.
🤖 Prompt for AI Agents
In src/sdk/reveal.ts around lines 38 to 41, the current merge nests
globalOptions under config; instead build finalOptions by spreading
globalOptions at top-level and setting config to the merge of
(globalOptions.config || {}) with the parsed localOptions. Parse
container.dataset.config inside a try/catch: on parse error log or warn (use
existing logger or console.warn) and fall back to an empty object, then set
finalOptions = { ...globalOptions, config: { ...(globalOptions.config || {}),
...parsedLocalConfig } } and return createPlayground(container, finalOptions).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/sdk/reveal.ts (1)
39-42: Fix incorrect config merging logic.The config merging has a critical bug that was flagged in a previous review.
globalOptionsis of typeLiveOptions(which extendsEmbedOptions) and may contain properties likeconfig,loading,view, etc. The current code incorrectly nests all ofglobalOptionsunder theconfigproperty, which will cause malformed options to be passed tocreatePlayground.For example, if
globalOptions = { config: { theme: 'dark' }, loading: 'eager' }, the current code produces:{ config: { config: { theme: 'dark' }, loading: 'eager', ...localOptions } }Additionally, the code lacks error handling for invalid JSON in
data-configattributes.Apply this diff to correctly merge configurations with error handling:
const promises = containers.map((container) => { - const localOptions = container.dataset.config || '{}'; - const finalOptions = { config: { ...globalOptions, ...JSON.parse(localOptions) } }; + let parsedLocalConfig: Record<string, any> = {}; + try { + parsedLocalConfig = JSON.parse(container.dataset.config || '{}'); + } catch (e) { + console.error('Failed to parse data-config:', e); + } + const finalOptions: EmbedOptions = { + ...globalOptions, + config: { + ...(globalOptions.config || {}), + ...parsedLocalConfig, + }, + }; return createPlayground(container, finalOptions); });
🧹 Nitpick comments (2)
src/sdk/reveal.ts (1)
20-25: Consider removing@ts-ignoreand using proper type assertion.The
@ts-ignoredirective bypasses type safety. Consider using a type-safe approach instead.Apply this diff to improve type safety:
const initIframeStyle = (iframe: HTMLIFrameElement, styles: Partial<CSSStyleDeclaration>) => { for (const [key, value] of Object.entries(styles)) { - // @ts-ignore - iframe.style[key as any] = value; + if (value !== undefined) { + iframe.style[key as keyof CSSStyleDeclaration] = value as string; + } } };src/sdk/__tests__/reveal.test.ts (1)
24-51: Consider adding test coverage for EmbedOptions-level properties.The test validates that
configproperties (script, markup) are merged correctly, but doesn't verify that top-levelEmbedOptionsproperties (likeloading,view,headless) fromglobalOptionsare passed through correctly tocreatePlayground. Given the config merging bug inreveal.ts(lines 39-41), adding a test that checks for these properties would help catch the issue.Consider adding assertions like:
// In the test setup, add a top-level option to globalOptions: livecodes: { markup: { language: 'markdown', content: '# Hello world' }, sdkReady, loading: 'eager' // Add this } // Then assert in the test: expect(calledWith.loading).toBe('eager'); expect(calledWith.config).not.toHaveProperty('loading'); // Should not be nested in configThis would expose the incorrect nesting bug where
loadingends up insideconfiginstead of at the top level ofEmbedOptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sdk/__tests__/reveal.test.ts(1 hunks)src/sdk/reveal.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sdk/reveal.ts (1)
src/sdk/index.ts (1)
createPlayground(33-375)
src/sdk/__tests__/reveal.test.ts (2)
src/sdk/reveal.ts (1)
LiveCodes(27-54)src/sdk/index.ts (1)
createPlayground(33-375)
🪛 ast-grep (0.39.9)
src/sdk/__tests__/reveal.test.ts
[warning] 13-13: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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). (11)
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (18.x)
- GitHub Check: tests (18.x, 3)
- GitHub Check: tests (18.x, 4)
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 1)
- GitHub Check: tests (18.x, 2)
- GitHub Check: build
🔇 Additional comments (4)
src/sdk/reveal.ts (2)
1-18: LGTM! Well-defined type interfaces.The type definitions are clean and correctly extend the necessary base types.
LiveOptionsappropriately extendsEmbedOptionswith thesdkReadycallback, andGlobalLiveCodesOptionsproperly extendsReveal.Optionswith LiveCodes-specific configuration.
56-58: LGTM! Standard global attachment pattern.The window attachment is properly guarded with a runtime check and follows common patterns for exposing a global API in browser environments.
src/sdk/__tests__/reveal.test.ts (2)
1-22: LGTM! Well-structured test setup and coverage.The mock setup correctly simulates
createPlaygroundbehavior, and the first test appropriately verifies the no-op path when no[data-livecodes]elements are present.
13-13: Static analysis false positive - test cleanup is safe.The static analysis tool flagged
document.body.innerHTML = ''as a potential XSS risk. However, this is safe in the test context as we're assigning a string literal (empty string) to clean up the DOM between tests, not using user input.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
docs/docs/sdk/reveal.mdx (3)
59-59: Fix TypeScript import path — references node_modules directly.The import should use the published package name, not the node_modules path. This is incorrect for documentation examples that users will follow.
Apply this diff:
-import type { LiveOptions, GlobalLiveCodesOptions, LiveCodesInstance } from './node_modules/livecodes/reveal.js'; +import type { LiveOptions, GlobalLiveCodesOptions, LiveCodesInstance } from 'livecodes';
85-85: Fix import path — references node_modules directly.Similar to line 59, this should import from the published package, not node_modules.
Apply this diff:
-import { LiveCodes } from "./node_modules/livecodes/reveal.js"; +import { LiveCodes } from "livecodes";
110-110: Fix syntax error in import path — misplaced quotation mark.The quotation mark is in the wrong position, breaking the import statement syntax. This code example will fail if copy-pasted.
Apply this diff:
-import { LiveCodes } from "./node_modules/livecodes"/reveal.js"; +import { LiveCodes } from "livecodes";
🧹 Nitpick comments (1)
docs/docs/sdk/reveal.mdx (1)
7-7: Consider rephrasing to avoid intensifier.The phrase "very simple" is a generic intensifier. Consider using more specific language that describes the actual characteristics (e.g., "lightweight," "straightforward," or "minimal").
Example alternatives:
- "It has a straightforward [implementation]..."
- "It has a lightweight [implementation]..."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/docs/sdk/reveal.mdx(1 hunks)src/sdk/package.sdk.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdk/package.sdk.json
🧰 Additional context used
🪛 LanguageTool
docs/docs/sdk/reveal.mdx
[style] ~7-~7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ds directly within your slides. It has a very simple [implementation](https://github.com/liv...
(EN_WEAK_ADJECTIVE)
⏰ 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). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
- GitHub Check: tests (18.x, 4)
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 2)
- GitHub Check: tests (18.x, 3)
- GitHub Check: tests (18.x, 1)
- GitHub Check: build (18.x)
- GitHub Check: build
🔇 Additional comments (1)
docs/docs/sdk/reveal.mdx (1)
27-27: Verify CDN URLs and package references are consistent with current/published versions.The documentation references
[email protected]in CDN URLs and imports from"reveal.js-plugin-livecodes"(line 39). Ensure these versions and package names are accurate for the published SDK.Please confirm:
- The published npm package version matches the CDN version referenced (0.12.0)
- Line 39's import path
"reveal.js-plugin-livecodes"is the correct published package name- The build process produces both
reveal.umd.jsandreveal.jsfiles referenced in the CDN URLsAlso applies to: 38-39, 147-149
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/docs/sdk/reveal.mdx (1)
7-7: Consider replacing the weak adjective "very simple".Per style guidelines, the phrase "very simple" uses an over-used intensifier. Consider rewording for stronger, more direct language (e.g., "straightforward implementation" or "simple implementation").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/docs/sdk/reveal.mdx(1 hunks)src/sdk/package.sdk.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdk/package.sdk.json
🧰 Additional context used
🪛 LanguageTool
docs/docs/sdk/reveal.mdx
[style] ~7-~7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ds directly within your slides. It has a very simple [implementation](https://github.com/liv...
(EN_WEAK_ADJECTIVE)
⏰ 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). (11)
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (18.x)
- GitHub Check: tests (18.x, 2)
- GitHub Check: tests (18.x, 4)
- GitHub Check: build
- GitHub Check: tests (18.x, 3)
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 1)
🔇 Additional comments (5)
docs/docs/sdk/reveal.mdx (5)
27-27: Verify the CDN version references are current.The CDN links reference
[email protected]. Please ensure this version aligns with the release version for this PR. Once merged, these version strings should be updated if a newer version is published.Also applies to: 148-148
59-59: Import paths have been corrected.The three import statements now correctly reference the
livecodespackage:
- Line 59: TypeScript types import ✓
- Line 85: ES module LiveCodes import ✓
- Line 110: ES module LiveCodes import ✓
This resolves the previous review feedback about incorrect paths. The documentation is now consistent with the published package exports.
Also applies to: 85-85, 110-110
13-44: Usage section is well-structured and provides clear import methods.Both import approaches (script tag and ES module) are correctly documented with proper examples and syntax.
46-76: TypeScript support section is clear and comprehensive.The type definitions (
LiveOptions,GlobalLiveCodesOptions,LiveCodesInstance) are well-described with a practical example demonstrating their use together.
78-143: Configuration and examples are thorough and correct.The Config section properly demonstrates:
- Global configuration via the
livecodesplugin option withsdkReadycallback- Per-container configuration using
data-configattributecustomStyleCSS customization for iframesCode examples are syntactically correct and illustrate the concepts well.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/sdk/reveal.ts (1)
54-56: Add error handling for JSON.parse to prevent initialization failure.Line 56 parses
container.dataset.configwithout error handling. If any container has invalid JSON in itsdata-configattribute, the entire initialization will fail for all playgrounds due to an uncaught exception inside the map function.Apply this diff to add error handling:
const promises = containers.map((container) => { const localOptions = container.dataset.config || '{}'; - const parsedLocalOptions = JSON.parse(localOptions); + let parsedLocalOptions: any = {}; + try { + parsedLocalOptions = JSON.parse(localOptions); + } catch (e) { + console.error('Failed to parse data-config for container:', container, e); + } let finalOptions: EmbedOptions;This ensures that one malformed config won't prevent other playgrounds from initializing.
🧹 Nitpick comments (2)
docs/docs/sdk/reveal.mdx (1)
27-27: Consider using a version placeholder or latest tag.The hardcoded version
[email protected]will require manual updates with each release. Consider using@latestor a version placeholder that can be automatically updated during documentation builds.The same issue appears on line 152 in the demo code.
src/sdk/reveal.ts (1)
20-25: Type suppressions are acceptable here but could be improved.The
@ts-ignoreandas anytype assertions are used to bypass TypeScript's type checking when dynamically applying CSS properties. While this works and is a common pattern for dynamic property access, it bypasses type safety.Consider a more type-safe approach:
const initIframeStyle = (iframe: HTMLIFrameElement, styles: Partial<CSSStyleDeclaration>) => { for (const [key, value] of Object.entries(styles)) { - // @ts-ignore - iframe.style[key as any] = value; + if (key in iframe.style && typeof value === 'string') { + (iframe.style as any)[key] = value; + } } };This at least checks that the property exists before attempting to set it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/sdk/reveal.mdx(1 hunks)src/sdk/__tests__/reveal.test.ts(1 hunks)src/sdk/reveal.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sdk/reveal.ts (1)
src/sdk/index.ts (1)
createPlayground(33-375)
src/sdk/__tests__/reveal.test.ts (2)
src/sdk/reveal.ts (1)
LiveCodes(42-94)src/sdk/index.ts (1)
createPlayground(33-375)
🪛 ast-grep (0.39.9)
src/sdk/__tests__/reveal.test.ts
[warning] 13-13: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 LanguageTool
docs/docs/sdk/reveal.mdx
[style] ~7-~7: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ds directly within your slides. It has a very simple [implementation](https://github.com/liv...
(EN_WEAK_ADJECTIVE)
⏰ 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). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
- GitHub Check: build (18.x)
- GitHub Check: build
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 4)
- GitHub Check: tests (18.x, 3)
- GitHub Check: tests (18.x, 2)
- GitHub Check: tests (18.x, 1)
🔇 Additional comments (7)
docs/docs/sdk/reveal.mdx (2)
85-103: Example code looks good and demonstrates proper usage.The configuration example correctly shows how to initialize Reveal with the LiveCodes plugin, configure global options, and use the sdkReady callback.
136-147: Clear documentation of per-container configuration.The example effectively demonstrates how to override global configuration on a per-container basis using the
data-configattribute with a JSON-stringified object.src/sdk/__tests__/reveal.test.ts (3)
4-11: Mock implementation correctly simulates playground creation.The mock properly creates an iframe element and appends it to the container, matching the expected behavior of the real
createPlaygroundfunction.
24-51: Comprehensive test validates full initialization flow.This test effectively covers the complete scenario: global config, per-container config, config merging (with local config taking precedence), sdkReady callback invocation, and custom styling application.
100-127: Test confirms correct config precedence.Good validation that custom per-container configuration (
data-config) properly overrides global configuration while still triggering the sdkReady callback.src/sdk/reveal.ts (2)
58-75: Config merging logic correctly handles string and object configs.The implementation properly differentiates between string configs (remote URLs) and object configs, merging them appropriately while respecting the precedence of local (per-container) config over global config.
84-92: Proper sequencing of async operations.The code correctly waits for all playgrounds to be created via
Promise.allbefore applying iframe styles and invoking sdkReady callbacks, ensuring that the DOM elements exist before modification.
| const applyConfigAndSdkFn = async function ( | ||
| sdkItem: Playground, | ||
| sdkReadyfn?: (sdk: Playground) => void, | ||
| config?: string | Partial<Config>, | ||
| ) { | ||
| if (typeof config === 'string') { | ||
| await fetch(config) | ||
| .then((res) => res.json()) | ||
| .then((json) => sdkItem.setConfig(json)); | ||
| } | ||
| if (typeof sdkReadyfn === 'function') { | ||
| sdkReadyfn(sdkItem); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for remote config fetch.
Lines 32-35 fetch a remote config URL without error handling. Network failures or invalid JSON responses will result in unhandled promise rejections, causing the sdkReady callback to not be invoked for that playground.
Apply this diff to add error handling:
const applyConfigAndSdkFn = async function (
sdkItem: Playground,
sdkReadyfn?: (sdk: Playground) => void,
config?: string | Partial<Config>,
) {
if (typeof config === 'string') {
- await fetch(config)
- .then((res) => res.json())
- .then((json) => sdkItem.setConfig(json));
+ try {
+ const res = await fetch(config);
+ if (!res.ok) {
+ throw new Error(`Failed to fetch config: ${res.status} ${res.statusText}`);
+ }
+ const json = await res.json();
+ await sdkItem.setConfig(json);
+ } catch (e) {
+ console.error('Failed to load remote config from', config, e);
+ }
}
if (typeof sdkReadyfn === 'function') {
sdkReadyfn(sdkItem);
}
};📝 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.
| const applyConfigAndSdkFn = async function ( | |
| sdkItem: Playground, | |
| sdkReadyfn?: (sdk: Playground) => void, | |
| config?: string | Partial<Config>, | |
| ) { | |
| if (typeof config === 'string') { | |
| await fetch(config) | |
| .then((res) => res.json()) | |
| .then((json) => sdkItem.setConfig(json)); | |
| } | |
| if (typeof sdkReadyfn === 'function') { | |
| sdkReadyfn(sdkItem); | |
| } | |
| }; | |
| const applyConfigAndSdkFn = async function ( | |
| sdkItem: Playground, | |
| sdkReadyfn?: (sdk: Playground) => void, | |
| config?: string | Partial<Config>, | |
| ) { | |
| if (typeof config === 'string') { | |
| try { | |
| const res = await fetch(config); | |
| if (!res.ok) { | |
| throw new Error(`Failed to fetch config: ${res.status} ${res.statusText}`); | |
| } | |
| const json = await res.json(); | |
| await sdkItem.setConfig(json); | |
| } catch (e) { | |
| console.error('Failed to load remote config from', config, e); | |
| } | |
| } | |
| if (typeof sdkReadyfn === 'function') { | |
| sdkReadyfn(sdkItem); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/sdk/reveal.ts around lines 27 to 40, the remote config fetch is done
without error handling which can cause unhandled promise rejections and prevent
sdkReadyfn from being called; wrap the async config load in a try/catch (or
append a .catch) to handle network and JSON parse errors, log or surface the
error (console.error or a logger), and ensure sdkReadyfn(sdkItem) is invoked
regardless of fetch success (i.e., call sdkReadyfn in finally or after the
catch), leaving the existing config unchanged on failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/sdk/__tests__/reveal.test.ts (2)
102-102: Inconsistent test name capitalization.The test name uses title case inconsistently. Consider using lowercase for consistency with other test descriptions.
Apply this diff:
-test('should Apply Custom Config Over Global Config And Trigger Sdk Ready', async () => { +test('should apply custom config over global config and trigger sdkReady', async () => {
35-135: Consider adding error handling tests.While the current coverage is solid, consider adding tests for error scenarios such as:
- Invalid JSON in
data-configattributecreatePlaygroundpromise rejectionThese tests would improve robustness and help catch edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/__tests__/reveal.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sdk/__tests__/reveal.test.ts (2)
src/sdk/index.ts (1)
createPlayground(33-375)src/sdk/reveal.ts (1)
LiveCodes(42-94)
🪛 ast-grep (0.39.9)
src/sdk/__tests__/reveal.test.ts
[warning] 30-30: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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). (11)
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build (18.x)
- GitHub Check: tests (18.x, 2)
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 4)
- GitHub Check: tests (18.x, 1)
- GitHub Check: tests (18.x, 3)
- GitHub Check: build
🔇 Additional comments (3)
src/sdk/__tests__/reveal.test.ts (3)
1-19: LGTM! Well-structured test helpers.The imports and helper functions are well-organized and improve test readability. The helpers appropriately encapsulate common assertions and DOM setup logic.
21-28: LGTM! Mock implementation correctly simulates createPlayground behavior.The mock appropriately creates and appends an iframe element, matching the expected behavior of the real implementation.
30-33: Static analysis warning is a false positive.The innerHTML assignment on line 31 is safe test cleanup code with no user input. This is a standard pattern for DOM reset in Jest tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/sdk/__tests__/reveal.test.ts (1)
39-139: Add test coverage for multiple containers.As noted in a previous review, the implementation in
reveal.tsexplicitly supports multiple containers usingcontainers.map()andPromise.all(), but there's no test verifying this behavior. This remains a gap in test coverage for a core feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/__tests__/reveal.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sdk/__tests__/reveal.test.ts (2)
src/sdk/index.ts (1)
createPlayground(33-375)src/sdk/reveal.ts (1)
LiveCodes(42-94)
🪛 ast-grep (0.39.9)
src/sdk/__tests__/reveal.test.ts
[warning] 34-34: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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). (11)
- GitHub Check: build (18.x)
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Header rules - livecodes
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Pages changed - livecodes
- GitHub Check: build
- GitHub Check: tests (18.x, 4)
- GitHub Check: tests (18.x, 2)
- GitHub Check: tests (18.x, 3)
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 1)
🔇 Additional comments (4)
src/sdk/__tests__/reveal.test.ts (4)
1-23: LGTM! Well-structured test helpers.The helper functions are clean, focused, and reduce duplication across test cases effectively.
25-37: LGTM! Mock and cleanup setup is appropriate.The mock implementation correctly simulates
createPlaygroundbehavior for unit testing. ThebeforeEachcleanup ensures proper test isolation.Regarding the static analysis warning on line 34: this is a false positive. Setting
innerHTML = ''in test cleanup code does not pose an XSS risk.
39-104: LGTM! Good coverage of single-container configuration scenarios.These tests comprehensively cover:
- No containers present
- Local config precedence over global config
- Global config as fallback
- Custom styles application
The assertions are clear and verify the expected merge behavior.
129-139: LGTM! Validates the no-config scenario.This test correctly verifies that a playground can be initialized without any configuration, relying on defaults.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sdk/__tests__/reveal.test.ts (1)
68-83: Consider adding a test for local config overriding global config.The current tests cover:
- No containers (lines 39-43)
- Single container with global config only (lines 45-66)
- Multiple containers with local config only (lines 68-83)
However, there's no test verifying the config merge precedence when both global and local configs are present. This is core functionality in
reveal.ts(lines 54-71) where local config should override/merge with global config. The removed duplicate test from the previous review may have covered this scenario.Consider adding a test like:
test('should merge local config with global config, with local taking precedence', async () => { const sdkReady = jest.fn(); createContainer('{"config":{"script":{"language":"javascript","content":"console.log(123)"}}}'); const mockDeck = { getConfig: jest.fn().mockReturnValue({ livecodes: { config: { script: { language: 'javascript', content: 'console.log(456)' } }, sdkReady, }, }), } as any; LiveCodes.init(mockDeck); await new Promise(process.nextTick); expectCreatePlaygroundAndSdkFn(sdkReady); const calledWith = (createPlayground as jest.Mock).mock.calls[0][1]; expect(calledWith.config.script.language).toBe('javascript'); expect(calledWith.config.script.content).toBe('console.log(123)'); // local overrides global });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/__tests__/reveal.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/sdk/__tests__/reveal.test.ts (2)
src/sdk/index.ts (1)
createPlayground(33-375)src/sdk/reveal.ts (1)
LiveCodes(42-94)
🪛 ast-grep (0.39.9)
src/sdk/__tests__/reveal.test.ts
[warning] 34-34: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ 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). (11)
- GitHub Check: Redirect rules - livecodes
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Header rules - livecodes
- GitHub Check: Pages changed - livecodes
- GitHub Check: build (18.x)
- GitHub Check: tests (18.x, 3)
- GitHub Check: tests (18.x, 2)
- GitHub Check: build
- GitHub Check: tests (18.x, 5)
- GitHub Check: tests (18.x, 1)
- GitHub Check: tests (18.x, 4)
|
Thank you @m2y11138 I think this should be in a separate repo. You can then add We can still refer to this plugin in LiveCodes docs (also similar to markdown-to-livecodes) If you wish, we can add the new repo to live-codes organization on github, while keeping you as its main maintainer. Once you have done that please let me know. I may have some suggestions for usage. |
|
Thank you for reply! @hatemhosny |
|
Hello @hatemhosny |
2 participants
What type of PR is this? (check all applicable)
Description
This PR adds a LiveCodes SDK for Reveal.js, including implementation, tests for the code, and updated documentation explaining its usage.
Related Tickets & Documents
Fixes #841
Mobile & Desktop Screenshots/Recordings
Added tests?
Added to documentations?
[optional] Are there any post-deployment tasks we need to perform?
Yes, we need to rebuild the SDK and publish it to the npm registry.
[optional] What gif best describes this PR or how it makes you feel?
This is truly a magical experience. More detailed SDK instructions are documented in the docs, and I look forward to any feedback or suggestions.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores