feat: share themes by URL — CLI theme add, gallery import + delete - #286
Open
ridemountainpig wants to merge 6 commits into
Open
feat: share themes by URL — CLI theme add, gallery import + delete#286ridemountainpig wants to merge 6 commits into
ridemountainpig wants to merge 6 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@ridemountainpig is attempting to deploy a commit to the open-slide Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds URL-based theme sharing across the CLI, dev server, and themes UI, including remote discovery/import, copying and deleting themes, slide reference cleanup, host restrictions, build manifests, localization, and documentation. ChangesTheme import/sharing feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ThemeImportDialog
participant registerThemeRoutes
participant discoverThemes
participant fetchTheme
participant writeTheme
ThemeImportDialog->>registerThemeRoutes: POST /__themes/import
registerThemeRoutes->>discoverThemes: discover source URL
registerThemeRoutes->>fetchTheme: fetch selected themes
registerThemeRoutes->>writeTheme: write themes locally
writeTheme-->>registerThemeRoutes: written results
registerThemeRoutes-->>ThemeImportDialog: import response
sequenceDiagram
participant ThemesGallery
participant registerThemeRoutes
participant clearThemeFromSlides
ThemesGallery->>registerThemeRoutes: DELETE /__themes/:id
registerThemeRoutes->>clearThemeFromSlides: clear slide references
clearThemeFromSlides-->>registerThemeRoutes: cleared slide IDs
registerThemeRoutes-->>ThemesGallery: deletion response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 9
🧹 Nitpick comments (3)
apps/web/content/docs/reference/config.mdx (1)
44-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: type definition order is inconsistent with existing pattern.
OpenSlideThemeImportConfigis used before its definition, whileOpenSlideBuildConfigabove follows define-then-use ordering. Consider moving the new type block beforeOpenSlideConfigfor consistency.🤖 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/web/content/docs/reference/config.mdx` around lines 44 - 55, The type declaration order is inconsistent because OpenSlideThemeImportConfig is referenced before it is defined. Move the OpenSlideThemeImportConfig block so it appears before OpenSlideConfig, keeping the define-then-use pattern used by OpenSlideBuildConfig and preserving the existing type names and references.packages/core/src/cli/run.ts (1)
143-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider test coverage for the new
theme addcommand.
themeAddcovers host-trust confirmation, interactive selection, and fetch/write flows — all security/UX-sensitive paths — but no corresponding test file was found alongsidetheme.ts(only an unrelated import shown inrun.test.ts).🤖 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 `@packages/core/src/cli/run.ts` around lines 143 - 156, Add test coverage for the new theme add command and its `themeAdd` flow, since it exercises security- and UX-sensitive behavior. Create or extend tests around `themeAdd` in `theme.ts` to cover host-trust confirmation, interactive selection, and fetch/write success and failure paths. Also verify the `theme` command wiring in `run.ts` invokes `themeAdd` correctly from the `add` subcommand. Reuse the existing `themeAdd` symbol and the `theme` command setup to place the tests near the changed behavior.packages/core/src/vite/routes/themes.ts (1)
10-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the endpoint-list comment.
This block restates the routes; the later comment on Lines 17-19 already captures the non-obvious reason for the GET routes.
Proposed cleanup
-// POST /__themes/import { url, id?, ids?, force? } fetch themes from a URL into themes/ -// — multi-theme sources answer { discovered } and import -// nothing until the client re-posts with ids -// DELETE /__themes/:id remove a theme's files + clear meta.theme from slides using it -// GET /themes/index.json theme manifest (mirrors the build output) -// GET /themes/:file raw theme .md / .demo.* source -//As per coding guidelines,
**/*.{ts,tsx,js,jsx}comments should not explain WHAT the code does or add module-header descriptions.🤖 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 `@packages/core/src/vite/routes/themes.ts` around lines 10 - 16, The module-header endpoint list comment in themes.ts should be removed because it restates the routes and violates the comment guidelines. Delete the block near the top of the file and keep the later inline comment that explains the non-obvious GET route behavior; use the surrounding route definitions in the routes module to locate it.Source: Coding guidelines
🤖 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 `@packages/core/src/app/components/themes/theme-detail.tsx`:
- Around line 79-100: The copied share link in handleCopyUrl currently
interpolates theme.id directly, which can break for spaces or other unsafe
characters. Update the URL construction in theme-detail.tsx so the /themes/<id>
segment uses an encoded theme.id while keeping the same route shape, and leave
the rest of the clipboard/toast flow unchanged.
In `@packages/core/src/app/components/themes/themes-gallery.tsx`:
- Around line 62-70: The theme gallery menu trigger stays hidden for keyboard
users because it only reveals on hover or when expanded; update the button in
ThemesGallery to also become visible on keyboard focus. Add a focus-based reveal
class such as focus-visible or group-focus-within to the same trigger styles so
tabbing to the DropdownMenuTrigger button shows it before activation, while
keeping the existing hover and aria-expanded behavior intact.
In `@packages/core/src/cli/theme.ts`:
- Around line 34-80: The cancel path in selectEntries is inconsistent with the
trust-confirmation flow in themeAdd: blank input currently throws
ThemeImportError('invalid', 'Cancelled.') instead of aborting cleanly. Update
selectEntries to return a sentinel value such as null or an empty result when
the user cancels, then handle that case in themeAdd the same way the
confirmation decline is handled so it prints the dim “Cancelled.” message and
returns normally. Keep the change localized around selectEntries and the
themeAdd caller, preserving the existing selection behavior for valid inputs.
In `@packages/core/src/editing/slide-ops.ts`:
- Around line 226-232: The theme-removal logic in slide-ops.ts is matching any
theme property inside the meta object, so update the edit path in the relevant
helper to only target top-level meta.theme and ignore nested properties like
nested.theme. Replace the regex-based search in the code around the propRe/match
check with a top-level-aware scan or AST-based edit so the deletion only occurs
when the actual top-level theme field matches the deleted theme ID.
In `@packages/core/src/themes/import.ts`:
- Around line 321-372: `writeTheme` has a TOCTOU race because `availableThemeId`
probes with `existsSync` before `fs.writeFile` writes the theme file, so
concurrent imports can clobber each other. Update the `writeTheme` flow to avoid
the separate existence check when not forcing: attempt the write with an
exclusive create mode and, on `EEXIST`, retry with the next numbered suffix
until a free id is successfully written. Keep `availableThemeId` and the
`writeTheme` rename logic aligned with this retry-based approach.
- Around line 297-299: The formatFrontmatterValue helper only quotes values in a
couple of cases, so YAML-significant characters like colons or # can still be
emitted unescaped into frontmatter. Update formatFrontmatterValue in import.ts
to always serialize the value safely for YAML, and make sure the renaming path
that writes name: <value> uses this helper so theme names with special
characters stay valid.
In `@packages/core/src/themes/scan.ts`:
- Around line 71-76: The demo candidate lookup in scanTheme should not rely on
existsSync alone, since it can match directories and incorrectly set demoAbs.
Update the DEMO_EXTS loop in scan.ts to verify each candidate is a file before
assigning demoAbs, using the same path-building logic and filtering out only
non-file or missing paths so the build plugin only receives valid demo files.
In `@packages/core/src/vite/routes/themes.ts`:
- Around line 126-171: The catch path in the themes import handler is treating
malformed JSON from readBody as a server error, so update the request handling
in the themes route to detect body-parse failures and return a 400 instead of
falling through to the generic 500 response. Use the existing ThemeImportError
branch and the surrounding try/catch in the route that calls readBody,
discoverThemes, and fetchTheme to add a specific malformed-body case before the
final fallback.
- Around line 46-49: The theme route currently calls decodeURIComponent before
the try block, so malformed percent-encoded paths can escape the handler’s error
handling. Move the decode logic into the try in the themes route handler (the
code that builds url and rel) and catch URIError explicitly so invalid theme
paths return a 400 response instead of failing outside the route.
---
Nitpick comments:
In `@apps/web/content/docs/reference/config.mdx`:
- Around line 44-55: The type declaration order is inconsistent because
OpenSlideThemeImportConfig is referenced before it is defined. Move the
OpenSlideThemeImportConfig block so it appears before OpenSlideConfig, keeping
the define-then-use pattern used by OpenSlideBuildConfig and preserving the
existing type names and references.
In `@packages/core/src/cli/run.ts`:
- Around line 143-156: Add test coverage for the new theme add command and its
`themeAdd` flow, since it exercises security- and UX-sensitive behavior. Create
or extend tests around `themeAdd` in `theme.ts` to cover host-trust
confirmation, interactive selection, and fetch/write success and failure paths.
Also verify the `theme` command wiring in `run.ts` invokes `themeAdd` correctly
from the `add` subcommand. Reuse the existing `themeAdd` symbol and the `theme`
command setup to place the tests near the changed behavior.
In `@packages/core/src/vite/routes/themes.ts`:
- Around line 10-16: The module-header endpoint list comment in themes.ts should
be removed because it restates the routes and violates the comment guidelines.
Delete the block near the top of the file and keep the later inline comment that
explains the non-obvious GET route behavior; use the surrounding route
definitions in the routes module to locate it.
🪄 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: CHILL
Plan: Pro
Run ID: 32668155-c0ea-4ae6-b850-45a169ee190d
📒 Files selected for processing (29)
.changeset/theme-import-from-url.mdapps/web/content/docs/cli/meta.jsonapps/web/content/docs/cli/overview.mdxapps/web/content/docs/cli/theme-add.mdxapps/web/content/docs/core-feature/themes.mdxapps/web/content/docs/reference/config.mdxpackages/core/README.mdpackages/core/src/app/components/themes/theme-detail.tsxpackages/core/src/app/components/themes/theme-import-dialog.tsxpackages/core/src/app/components/themes/themes-gallery.tsxpackages/core/src/app/routes/themes.tsxpackages/core/src/app/virtual.d.tspackages/core/src/cli/run.tspackages/core/src/cli/theme.tspackages/core/src/config.tspackages/core/src/editing/slide-ops.test.tspackages/core/src/editing/slide-ops.tspackages/core/src/locale/en.tspackages/core/src/locale/ja.tspackages/core/src/locale/types.tspackages/core/src/locale/zh-cn.tspackages/core/src/locale/zh-tw.tspackages/core/src/themes/import.tspackages/core/src/themes/scan.tspackages/core/src/vite/api-plugin.tspackages/core/src/vite/config.tspackages/core/src/vite/routes/context.tspackages/core/src/vite/routes/themes.tspackages/core/src/vite/themes-plugin.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/core/src/editing/slide-ops.ts`:
- Around line 196-213: The scanner in nestingDepthAt currently ignores comments,
so matches inside // or /* */ can be treated as top-level and rewrite comment
text instead of real metadata. Update nestingDepthAt to track line and block
comment state before counting a match as depth 0, and apply the same fix
anywhere else in slide-ops.ts that reuses this scanner logic so only actual code
tokens are considered. Reference the nestingDepthAt helper and the related
match-handling logic around the affected slide operations when making the
change.
🪄 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: CHILL
Plan: Pro
Run ID: c139629b-529c-4498-afbb-9a35351e5684
📒 Files selected for processing (9)
apps/web/content/docs/reference/config.mdxpackages/core/src/app/components/themes/theme-detail.tsxpackages/core/src/app/components/themes/themes-gallery.tsxpackages/core/src/cli/theme.tspackages/core/src/editing/slide-ops.test.tspackages/core/src/editing/slide-ops.tspackages/core/src/themes/import.tspackages/core/src/themes/scan.tspackages/core/src/vite/routes/themes.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/core/src/app/components/themes/theme-detail.tsx
- apps/web/content/docs/reference/config.mdx
- packages/core/src/editing/slide-ops.test.ts
- packages/core/src/cli/theme.ts
- packages/core/src/app/components/themes/themes-gallery.tsx
- packages/core/src/vite/routes/themes.ts
- packages/core/src/themes/scan.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/editing/slide-ops.ts (1)
246-259: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBrace matching needs to ignore strings and comments.
A}inside a valid string or comment truncatesbodyearly, soremoveMetaThemeFromSourcereturnsnullandclearThemeFromSlidesskips removingmeta.themefor that slide. Reuse the quote/comment-aware scan fromsyntaxStateAthere.🤖 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 `@packages/core/src/editing/slide-ops.ts` around lines 246 - 259, The brace scan in removeMetaThemeFromSource currently counts every “{” and “}”, so a brace inside a string or comment can end the body too early. Update this scan to reuse the quote/comment-aware logic from syntaxStateAt, so only real code braces affect depth and closeBrace detection. Keep the behavior in clearThemeFromSlides intact by ensuring removeMetaThemeFromSource returns the correct body instead of null when braces appear inside strings or comments.
🤖 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.
Outside diff comments:
In `@packages/core/src/editing/slide-ops.ts`:
- Around line 246-259: The brace scan in removeMetaThemeFromSource currently
counts every “{” and “}”, so a brace inside a string or comment can end the body
too early. Update this scan to reuse the quote/comment-aware logic from
syntaxStateAt, so only real code braces affect depth and closeBrace detection.
Keep the behavior in clearThemeFromSlides intact by ensuring
removeMetaThemeFromSource returns the correct body instead of null when braces
appear inside strings or comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f0cbac0-d5a4-4d36-be6e-d937474dade3
📒 Files selected for processing (2)
packages/core/src/editing/slide-ops.test.tspackages/core/src/editing/slide-ops.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/editing/slide-ops.test.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves the config.mdx conflict by expressing themeImport in the new TypeTable docs format, and migrates the theme import dialog + gallery menu triggers to the Base UI render prop API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keeps both host options in OpenSlideConfig: upstream's top-level allowedHosts (Vite dev-server host check) and this PR's themeImport.allowedHosts (theme import restriction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds theme sharing between open-slide sites via URL:
open-slide theme add <url>— new CLI command that imports a theme from another open-slide site into the local project.Includes new theme scan/import modules in core, a
/api/themesVite route, locale strings (en/ja/zh-cn/zh-tw), docs for the new CLI command and theme sharing, and a minor changeset for@open-slide/core.Test plan
pnpm check— clean (one pre-existing warning on main, untouched)pnpm typecheck— passespnpm test— 284 tests passing🤖 Generated with Claude Code
Summary by CodeRabbit
open-slide theme add <url>with flags for--id,--all,--force, and-y/--yes.