feat(core): add optional PPTX export via slide-to-pptx plugin#156
feat(core): add optional PPTX export via slide-to-pptx plugin#156Luotee wants to merge 2 commits into
Conversation
…o-pptx
Adds a third entry to the slide download dropdown ("Download PPTX")
alongside the existing HTML and PDF options. PPTX rendering is
delegated to the optional npm peer dependency
`@helping-ai-workflow/slide-to-pptx`; the core itself never imports
it directly. The dropdown item works in two phases:
1. Plugin not installed: server returns JSON
`{ status: 'plugin-missing', packageManager, command }` and the
client surfaces a card-style toast with the detected
`cd <userCwd> && <pm> add -D @helping-ai-workflow/slide-to-pptx`
command + an inline copy button (`pptx-plugin-missing-toast`).
2. Plugin installed: server opens a `text/event-stream`, forwards
the plugin's `onProgress` events as SSE `progress` frames, then
one `done` frame carrying the base64-encoded pptx. The client
reassembles into a Blob and triggers a download. A
`pptx-progress-toast` mirrors the existing PDF toast UX
(per-page count, progress bar interpolated across the
`rendering` phase between 10% and 80%).
## Files added
- `packages/core/src/vite/routes/export-pptx-handler.ts`
Pure handler that detects the plugin via dynamic ESM `import()`
walking `node_modules` from `userCwd` (works under npm `file:`
symlinks), shell-quotes the install command, and rejects any
`slideId` whose resolved path escapes `<userCwd>/slides/`.
- `packages/core/src/vite/routes/export-pptx.ts`
Registers `POST /__os/api/export/pptx` and wires the handler to
the project's `userCwd`.
- `packages/core/src/vite/routes/pm-detect.ts`
Detects pnpm / npm / yarn / bun from lockfile presence and
`packageManager` field; builds the per-pm install command.
- `packages/core/src/app/lib/export-pptx.ts`
Browser-side SSE consumer with `try/finally` reader cancellation
on every error path, switch-default in `computePercent` to keep
the progress bar advancing on unknown phases. Naming mirrors the
existing `export-pdf.ts` / `export-html.ts` siblings.
- `packages/core/src/app/components/pptx-progress-toast.tsx`
Card-shaped toast mirroring `pdf-progress-toast.tsx`: shows
current/total page count + interpolated percent.
- `packages/core/src/app/components/pptx-plugin-missing-toast.tsx`
Inline copy-button toast for the install command.
- Tests: `export-pptx-handler.test.ts` (6 cases incl. path-traversal
blocks), `pm-detect.test.ts` (10 cases).
- `.changeset/pptx-export-optional-plugin.md` (minor bump).
## Files modified
- `packages/core/package.json`
Adds `@helping-ai-workflow/slide-to-pptx` to
`peerDependenciesMeta` with `optional: true`. No runtime
dependency on the plugin; bundle size unaffected for users who
don't install it.
- `packages/core/src/app/routes/slide.tsx`
Adds the PPTX dropdown item next to the existing PDF item, with
a Date.now()-suffixed toast session id so rapid double-clicks
produce distinct toast ids instead of stomping each other.
- `packages/core/src/locale/{en,zh-tw,zh-cn,ja,types}.ts`
Adds `pptxToast.*` and `exportAsPptx` / `pptxExportFailed` keys.
- `packages/core/src/vite/api-plugin.ts`
One-line route registration.
## Scope discipline
The diff is strictly additive — no existing PDF / HTML / route code
is refactored. The PPTX path mirrors the PDF path file-for-file
(handler ↔ handler, toast ↔ toast, lib ↔ lib) so the two exports
are symmetric.
All 236 vitest cases pass; tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@Luotee is attempting to deploy a commit to the Yiwei Ho Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds optional "Export as PPTX" to slide downloads: client export API and toasts, locale strings, server SSE export route with plugin dynamic import and package-manager detection, tests, and package.json marking the plugin as an optional peer dependency. ChangesPPTX Export Feature
Sequence DiagramsequenceDiagram
participant Client
participant DevServer
participant PluginModule
participant Renderer
Client->>DevServer: POST /__os/api/export/pptx { slideId }
DevServer->>DevServer: validate slideId, resolve slideDir
DevServer->>PluginModule: dynamic import register/render entry
alt module not found
DevServer-->>Client: 200 JSON { status: "plugin-missing", packageManager, command }
else plugin loaded
DevServer->>Renderer: renderSlideToPptx(slideDir, onProgress)
Renderer-->>DevServer: progress events (phase,current,total)
DevServer-->>Client: SSE events (progress ...)
Renderer-->>DevServer: done (Buffer)
DevServer-->>Client: SSE done (base64 payload)
Client->>Client: decode base64 -> download blob
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🤖 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 @.changeset/pptx-export-optional-plugin.md:
- Line 5: Replace the current line 5 in
.changeset/pptx-export-optional-plugin.md with a single short present-tense
user-facing change sentence (e.g., "Add optional 'Export as PPTX' item to the
slide download dropdown."), removing the install instructions and rationale so
the changeset contains only a one-line description of what changed from the
user's perspective.
In `@packages/core/package.json`:
- Around line 66-70: Add the missing peer dependency entry for
"`@helping-ai-workflow/slide-to-pptx`" in package.json so the existing
"peerDependenciesMeta" optional flag applies; specifically, add a
"peerDependencies" object (or update it) to include
"`@helping-ai-workflow/slide-to-pptx`": "*" (or an appropriate semver) alongside
the existing "peerDependenciesMeta" entry so npm treats it as an optional peer
dependency. Ensure the symbol names to edit are the JSON keys "peerDependencies"
and "peerDependenciesMeta" and the package name
"`@helping-ai-workflow/slide-to-pptx`".
In `@packages/core/src/app/components/pptx-plugin-missing-toast.tsx`:
- Around line 23-27: The onClick handler currently calls
navigator.clipboard?.writeText(command) and immediately shows a success toast
and dismisses toastId even if the copy failed or the Clipboard API is missing;
change the onClick logic in the component to first check for navigator.clipboard
and call navigator.clipboard.writeText(command) (which returns a Promise) and
only call toast.success(...) and toast.dismiss(toastId) in the Promise .then()
(or await the promise), and handle failures in .catch() by either attempting a
fallback copy (e.g., temporary textarea) or showing an error toast; ensure you
reference the existing onClick handler, navigator.clipboard.writeText, command,
toast.success and toast.dismiss(toastId) when applying the fix.
In `@packages/core/src/app/components/pptx-progress-toast.tsx`:
- Around line 16-38: The switch over progress.phase (in the pptx progress toast
component) lacks a default, so if progress.phase contains an unexpected value
`text` may remain unset; add a default branch in the switch that assigns a
sensible runtime fallback to `text` (e.g., t.pptxToast.unknown or a generic
message) and optionally log or warn including the unexpected `progress.phase`
value to aid debugging; update the case switch that references progress.phase
and the variable text accordingly.
In `@packages/core/src/app/routes/slide.tsx`:
- Around line 507-513: Replace the millisecond-precision session id using
Date.now() with a collision-safe unique id to avoid toast key clashes: update
the code that constructs session (currently `const session =
`${slideId}-${Date.now()}``) so it uses a UUID (e.g., `crypto.randomUUID()`) or
combines `performance.now()` entropy, then use that new session to build
`progressToastId` and `errorToastId` (the variables `session`,
`progressToastId`, and `errorToastId` are the identifiers to change); ensure the
fallback is available for environments without crypto.randomUUID().
In `@packages/core/src/vite/routes/pm-detect.ts`:
- Around line 17-20: The bun detection only checks for 'bun.lockb' so Bun
projects with 'bun.lock' get misidentified; update the PM detection logic in the
function that uses fs.existsSync(path.join(cwd, ...)) to check for both
'bun.lock' and 'bun.lockb' (or check 'bun.lock' first) and return { name: 'bun'
} when either exists, ensuring the branch that returns { name: 'bun' } is
reached when a Bun lockfile is present.
🪄 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: 56b34478-7224-41d4-9fcd-b32363fcea43
📒 Files selected for processing (17)
.changeset/pptx-export-optional-plugin.mdpackages/core/package.jsonpackages/core/src/app/components/pptx-plugin-missing-toast.tsxpackages/core/src/app/components/pptx-progress-toast.tsxpackages/core/src/app/lib/export-pptx.tspackages/core/src/app/routes/slide.tsxpackages/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/vite/api-plugin.tspackages/core/src/vite/routes/export-pptx-handler.test.tspackages/core/src/vite/routes/export-pptx-handler.tspackages/core/src/vite/routes/export-pptx.tspackages/core/src/vite/routes/pm-detect.test.tspackages/core/src/vite/routes/pm-detect.ts
| "peerDependenciesMeta": { | ||
| "@helping-ai-workflow/slide-to-pptx": { | ||
| "optional": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In npm package.json, does "peerDependenciesMeta" have any effect if the same package is not listed in "peerDependencies"?
💡 Result:
Yes and no—depends on what you mean by “effect,” and which package manager. For npm itself: peerDependenciesMeta only applies to entries under peerDependencies. npm’s documentation describes peerDependenciesMeta as providing metadata (currently: marking peer dependencies as optional) for “peer dependencies” declared in peerDependencies, e.g., “Marking a peer dependency as optional…” [1]. The npm docs and examples don’t define any behavior for keys in peerDependenciesMeta that are not present in peerDependencies, so npm does not have an advertised/useful effect for that mismatched case. For real-world behavior in the npm ecosystem: some tools treat peerDependenciesMeta entries as if they were peers even when they are missing from peerDependencies, which can result in installing/handling those packages anyway. For example, pnpm issue reports describe a case where a package present only in peerDependenciesMeta (and not peerDependencies) still affected peer handling/installation behavior [2]. That indicates that non-npm implementations may interpret peerDependenciesMeta more broadly than npm’s documented contract. So, if you’re asking specifically about npm: peerDependenciesMeta is meant to annotate peerDependencies, and keys not listed in peerDependencies aren’t part of the documented mechanism—so you should not rely on any effect there [1]. If you’re using pnpm/yarn or a custom installer, confirm with a test because behavior can differ [2].
Citations:
- 1: https://docs.npmjs.com/cli/v10/configuring-npm/package-json/
- 2: Should not install packages from peerDependenciesMeta, if they are only in meta, but not in peerDependencies pnpm/pnpm#6065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/core/package.json"
echo "== package.json lines 50-90 =="
nl -ba "$FILE" | sed -n '50,90p'
echo
echo "== peerDependencies occurrences =="
rg -n "peerDependencies" "$FILE" || true
echo
echo "== slide-to-pptx occurrences =="
rg -n "slide-to-pptx" "$FILE" || trueRepository: 1weiho/open-slide
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/core/package.json"
echo "== package.json lines 50-90 =="
awk 'NR>=50 && NR<=90 {printf "%6d %s\n", NR, $0}' "$FILE" || true
echo
echo "== peerDependencies occurrences =="
rg -n "peerDependencies" "$FILE" || true
echo
echo "== slide-to-pptx occurrences =="
rg -n "slide-to-pptx" "$FILE" || trueRepository: 1weiho/open-slide
Length of output: 1665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/core/package.json"
echo "== exact \"peerDependencies\": key matches =="
rg -n '"peerDependencies"\s*:' "$FILE" || true
echo
echo "== keys containing \"peerDependencies\" (for context) =="
rg -n '"peerDependencies[^"]*"\s*:' "$FILE" || trueRepository: 1weiho/open-slide
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la .changeset || true
find .changeset -maxdepth 1 -type f -name '*.md' | sort | sed -n '1,200p'
find .changeset -maxdepth 1 -type f -name 'pptx-export-optional-plugin.md' -print || trueRepository: 1weiho/open-slide
Length of output: 473
Declare the peer dependency explicitly—peerDependenciesMeta alone isn’t sufficient.
In packages/core/package.json (lines 66-70), peerDependenciesMeta marks @helping-ai-workflow/slide-to-pptx as optional, but the package isn’t declared in peerDependencies, so the “optional peer” metadata won’t be applied as intended by npm.
Suggested fix
+ "peerDependencies": {
+ "`@helping-ai-workflow/slide-to-pptx`": "*"
+ },
"peerDependenciesMeta": {
"`@helping-ai-workflow/slide-to-pptx`": {
"optional": true
}
},📝 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.
| "peerDependenciesMeta": { | |
| "@helping-ai-workflow/slide-to-pptx": { | |
| "optional": true | |
| } | |
| }, | |
| "peerDependencies": { | |
| "`@helping-ai-workflow/slide-to-pptx`": "*" | |
| }, | |
| "peerDependenciesMeta": { | |
| "`@helping-ai-workflow/slide-to-pptx`": { | |
| "optional": true | |
| } | |
| }, |
🤖 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/package.json` around lines 66 - 70, Add the missing peer
dependency entry for "`@helping-ai-workflow/slide-to-pptx`" in package.json so the
existing "peerDependenciesMeta" optional flag applies; specifically, add a
"peerDependencies" object (or update it) to include
"`@helping-ai-workflow/slide-to-pptx`": "*" (or an appropriate semver) alongside
the existing "peerDependenciesMeta" entry so npm treats it as an optional peer
dependency. Ensure the symbol names to edit are the JSON keys "peerDependencies"
and "peerDependenciesMeta" and the package name
"`@helping-ai-workflow/slide-to-pptx`".
| onClick={() => { | ||
| navigator.clipboard?.writeText(command).catch(() => {}); | ||
| toast.success(t.common.copyCommand, { duration: 1500 }); | ||
| toast.dismiss(toastId); | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact snippet around the referenced lines
FILE="packages/core/src/app/components/pptx-plugin-missing-toast.tsx"
nl -ba "$FILE" | sed -n '1,120p'
# Extract just the onClick handler for quick review
nl -ba "$FILE" | sed -n '15,60p'
# Search for other clipboard usage patterns in packages/core
rg -n "navigator\.clipboard|clipboard\?\.writeText|writeText\(" packages/core/src/app -S || true
rg -n "toast\.success|toast\.dismiss" packages/core/src/app/components/pptx-plugin-missing-toast.tsx -S
# Search for "copyCommand" usage to see expected semantics elsewhere
rg -n "copyCommand" packages/core/src -S || trueRepository: 1weiho/open-slide
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/core/src/app/components/pptx-plugin-missing-toast.tsx"
# Print file with line numbers via awk (no nl dependency)
awk '{print NR "\t" $0}' "$FILE" | sed -n '1,120p'
# Show the exact region around the prior referenced lines (23-27) with context
awk '{print NR "\t" $0}' "$FILE" | sed -n '15,45p'
# Search for clipboard usage patterns in core
rg -n "navigator\.clipboard|clipboard\?\.writeText|writeText\(" packages/core/src -S || true
# Search for toast usage patterns in the file
rg -n "toast\.success|toast\.dismiss" "$FILE" -S || true
# Check how copyCommand is used elsewhere (semantics)
rg -n "copyCommand" packages/core/src -S || trueRepository: 1weiho/open-slide
Length of output: 3874
Guard Clipboard API usage and only show success after an actual copy.
navigator.clipboard?.writeText(command)can evaluate toundefined; calling.catch(...)on that result can throw at runtime.- The success toast +
toast.dismiss(toastId)currently run even if the copy didn’t happen.
🐛 Proposed fix
<button
type="button"
onClick={() => {
- navigator.clipboard?.writeText(command).catch(() => {});
- toast.success(t.common.copyCommand, { duration: 1500 });
- toast.dismiss(toastId);
+ const copyPromise = navigator.clipboard?.writeText(command);
+ if (!copyPromise) return;
+ copyPromise
+ .then(() => {
+ toast.success(t.common.copyCommand, { duration: 1500 });
+ toast.dismiss(toastId);
+ })
+ .catch(() => {});
}}📝 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.
| onClick={() => { | |
| navigator.clipboard?.writeText(command).catch(() => {}); | |
| toast.success(t.common.copyCommand, { duration: 1500 }); | |
| toast.dismiss(toastId); | |
| }} | |
| <button | |
| type="button" | |
| onClick={() => { | |
| const copyPromise = navigator.clipboard?.writeText(command); | |
| if (!copyPromise) return; | |
| copyPromise | |
| .then(() => { | |
| toast.success(t.common.copyCommand, { duration: 1500 }); | |
| toast.dismiss(toastId); | |
| }) | |
| .catch(() => {}); | |
| }} |
🤖 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/app/components/pptx-plugin-missing-toast.tsx` around lines
23 - 27, The onClick handler currently calls
navigator.clipboard?.writeText(command) and immediately shows a success toast
and dismisses toastId even if the copy failed or the Clipboard API is missing;
change the onClick logic in the component to first check for navigator.clipboard
and call navigator.clipboard.writeText(command) (which returns a Promise) and
only call toast.success(...) and toast.dismiss(toastId) in the Promise .then()
(or await the promise), and handle failures in .catch() by either attempting a
fallback copy (e.g., temporary textarea) or showing an error toast; ensure you
reference the existing onClick handler, navigator.clipboard.writeText, command,
toast.success and toast.dismiss(toastId) when applying the fix.
| switch (progress.phase) { | ||
| case 'loading': | ||
| text = t.pptxToast.loading; | ||
| break; | ||
| case 'measuring': | ||
| text = t.pptxToast.loading; | ||
| break; | ||
| case 'rendering': | ||
| text = format(t.pptxToast.rendering, { | ||
| current: progress.current.toString().padStart(2, '0'), | ||
| total: progress.total.toString().padStart(2, '0'), | ||
| }); | ||
| break; | ||
| case 'building': | ||
| text = t.pptxToast.building; | ||
| break; | ||
| case 'postprocessing': | ||
| text = t.pptxToast.postprocessing; | ||
| break; | ||
| case 'done': | ||
| text = t.pptxToast.done; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Add a runtime fallback for unexpected progress phases.
Line 16 to Line 38 has no default branch. Since phase is parsed from JSON at runtime, an unknown phase can leave text unset.
♻️ Proposed fix
switch (progress.phase) {
@@
case 'done':
text = t.pptxToast.done;
break;
+ default:
+ text = t.pptxToast.loading;
+ break;
}📝 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.
| switch (progress.phase) { | |
| case 'loading': | |
| text = t.pptxToast.loading; | |
| break; | |
| case 'measuring': | |
| text = t.pptxToast.loading; | |
| break; | |
| case 'rendering': | |
| text = format(t.pptxToast.rendering, { | |
| current: progress.current.toString().padStart(2, '0'), | |
| total: progress.total.toString().padStart(2, '0'), | |
| }); | |
| break; | |
| case 'building': | |
| text = t.pptxToast.building; | |
| break; | |
| case 'postprocessing': | |
| text = t.pptxToast.postprocessing; | |
| break; | |
| case 'done': | |
| text = t.pptxToast.done; | |
| break; | |
| } | |
| switch (progress.phase) { | |
| case 'loading': | |
| text = t.pptxToast.loading; | |
| break; | |
| case 'measuring': | |
| text = t.pptxToast.loading; | |
| break; | |
| case 'rendering': | |
| text = format(t.pptxToast.rendering, { | |
| current: progress.current.toString().padStart(2, '0'), | |
| total: progress.total.toString().padStart(2, '0'), | |
| }); | |
| break; | |
| case 'building': | |
| text = t.pptxToast.building; | |
| break; | |
| case 'postprocessing': | |
| text = t.pptxToast.postprocessing; | |
| break; | |
| case 'done': | |
| text = t.pptxToast.done; | |
| break; | |
| default: | |
| text = t.pptxToast.loading; | |
| break; | |
| } |
🤖 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/app/components/pptx-progress-toast.tsx` around lines 16 -
38, The switch over progress.phase (in the pptx progress toast component) lacks
a default, so if progress.phase contains an unexpected value `text` may remain
unset; add a default branch in the switch that assigns a sensible runtime
fallback to `text` (e.g., t.pptxToast.unknown or a generic message) and
optionally log or warn including the unexpected `progress.phase` value to aid
debugging; update the case switch that references progress.phase and the
variable text accordingly.
| // Suffix with a high-resolution timestamp so two | ||
| // rapid clicks (before React re-renders to disable | ||
| // the dropdown item) produce distinct toast ids | ||
| // instead of stomping each other's state. | ||
| const session = `${slideId}-${Date.now()}`; | ||
| const progressToastId = `pptx-export-${session}`; | ||
| const errorToastId = `pptx-export-${session}-error`; |
There was a problem hiding this comment.
Use a collision-safe session id for toast keys.
Date.now() is millisecond precision, so two very fast selects can still generate the same session and overwrite toast state. Prefer crypto.randomUUID() (or append performance.now() entropy) for guaranteed uniqueness.
Suggested fix
- const session = `${slideId}-${Date.now()}`;
+ const session = `${slideId}-${crypto.randomUUID()}`;📝 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.
| // Suffix with a high-resolution timestamp so two | |
| // rapid clicks (before React re-renders to disable | |
| // the dropdown item) produce distinct toast ids | |
| // instead of stomping each other's state. | |
| const session = `${slideId}-${Date.now()}`; | |
| const progressToastId = `pptx-export-${session}`; | |
| const errorToastId = `pptx-export-${session}-error`; | |
| // Suffix with a high-resolution timestamp so two | |
| // rapid clicks (before React re-renders to disable | |
| // the dropdown item) produce distinct toast ids | |
| // instead of stomping each other's state. | |
| const session = `${slideId}-${crypto.randomUUID()}`; | |
| const progressToastId = `pptx-export-${session}`; | |
| const errorToastId = `pptx-export-${session}-error`; |
🤖 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/app/routes/slide.tsx` around lines 507 - 513, Replace the
millisecond-precision session id using Date.now() with a collision-safe unique
id to avoid toast key clashes: update the code that constructs session
(currently `const session = `${slideId}-${Date.now()}``) so it uses a UUID
(e.g., `crypto.randomUUID()`) or combines `performance.now()` entropy, then use
that new session to build `progressToastId` and `errorToastId` (the variables
`session`, `progressToastId`, and `errorToastId` are the identifiers to change);
ensure the fallback is available for environments without crypto.randomUUID().
| if (fs.existsSync(path.join(cwd, 'pnpm-lock.yaml'))) return { name: 'pnpm' }; | ||
| if (fs.existsSync(path.join(cwd, 'bun.lockb'))) return { name: 'bun' }; | ||
| if (fs.existsSync(path.join(cwd, 'yarn.lock'))) return { name: 'yarn' }; | ||
| if (fs.existsSync(path.join(cwd, 'package-lock.json'))) return { name: 'npm' }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What lockfile filename does Bun currently generate by default (bun.lockorbun.lockb), and what compatibility expectations exist for tooling that detects Bun projects?
💡 Result:
By default, Bun currently generates a lockfile named bun.lock (not bun.lockb) when you run bun install. [1] Compatibility expectations for tooling that detects Bun projects: - Many tools historically looked specifically for bun.lockb; Bun changed the default lockfile format to the text-based bun.lock, and it kept support for existing bun.lockb lockfiles (with an automatic migration path when running bun install). [1] - Tooling should therefore detect both bun.lock and bun.lockb during a transition period, because Bun supports both and will use bun.lock when present (as reflected by community/tooling guidance). [1][2] - If a tool relies on the lockfile’s contents/format, it should expect bun.lock to be the canonical “text-based lockfile” going forward, since Bun notes that features will target bun.lock after the migration. [1][3] Net: prefer bun.lock as the current default, but detection logic should treat bun.lockb as a fallback for older repos and/or during migration, and avoid assuming only one filename. [1][3][2]
Citations:
- 1: https://bun.com/docs/pm/lockfile
- 2: https://answers.netlify.com/t/support-new-bun-lock-text-lockfile-for-bun/134716
- 3: Implement a text-based lockfile format oven-sh/bun#11863
Add bun.lock fallback detection to avoid misidentifying Bun projects
Bun generates bun.lock by default, so checking only bun.lockb can cause Bun repos to fall through and be detected as another package manager (leading to the wrong install command).
Suggested patch
if (fs.existsSync(path.join(cwd, 'pnpm-lock.yaml'))) return { name: 'pnpm' };
+ if (fs.existsSync(path.join(cwd, 'bun.lock'))) return { name: 'bun' };
if (fs.existsSync(path.join(cwd, 'bun.lockb'))) return { name: 'bun' };
if (fs.existsSync(path.join(cwd, 'yarn.lock'))) return { name: 'yarn' };
if (fs.existsSync(path.join(cwd, 'package-lock.json'))) return { name: 'npm' };📝 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 (fs.existsSync(path.join(cwd, 'pnpm-lock.yaml'))) return { name: 'pnpm' }; | |
| if (fs.existsSync(path.join(cwd, 'bun.lockb'))) return { name: 'bun' }; | |
| if (fs.existsSync(path.join(cwd, 'yarn.lock'))) return { name: 'yarn' }; | |
| if (fs.existsSync(path.join(cwd, 'package-lock.json'))) return { name: 'npm' }; | |
| if (fs.existsSync(path.join(cwd, 'pnpm-lock.yaml'))) return { name: 'pnpm' }; | |
| if (fs.existsSync(path.join(cwd, 'bun.lock'))) return { name: 'bun' }; | |
| if (fs.existsSync(path.join(cwd, 'bun.lockb'))) return { name: 'bun' }; | |
| if (fs.existsSync(path.join(cwd, 'yarn.lock'))) return { name: 'yarn' }; | |
| if (fs.existsSync(path.join(cwd, 'package-lock.json'))) return { name: 'npm' }; |
🤖 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/pm-detect.ts` around lines 17 - 20, The bun
detection only checks for 'bun.lockb' so Bun projects with 'bun.lock' get
misidentified; update the PM detection logic in the function that uses
fs.existsSync(path.join(cwd, ...)) to check for both 'bun.lock' and 'bun.lockb'
(or check 'bun.lock' first) and return { name: 'bun' } when either exists,
ensuring the branch that returns { name: 'bun' } is reached when a Bun lockfile
is present.
Applied with a small tweak — kept "(requires `@helping-ai-workflow/slide-to-pptx` plugin)" so the "optional" part is clear to users reading the changelog. Install command and toast behavior moved out as suggested. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Hey @Luotee, thanks for this PR! The implementation is really well done, and I appreciate the effort you put into it. I do want to raise one architectural concern before we move forward. I'd prefer to have the PPTX rendering logic built directly into open-slide rather than depending on an external package (
Would you be open to co-working on this as a built-in feature? I'd love to collaborate on landing it together. Let me know what you think! |
|
Hi @1weiho , thanks for the kind words and the thoughtful concerns. Both points totally make sense. One thing worth flagging though: even if we merge this directly into open-slide, users will still need to run That said, I think it's worth exploring whether we can hit the same precision without the browser dependency. Would make the whole thing cleaner. I'm traveling this week, so let me poke at this when I'm back and we can sync on a plan then. Excited to collaborate on landing this properly! 🙌 |
2 participants
Summary
Adds an optional Download PPTX entry to the slide download dropdown, alongside the existing HTML and PDF options. PPTX rendering is delegated to the optional npm peer dependency
@helping-ai-workflow/slide-to-pptx;@open-slide/coreitself never imports it directly, so users who never install the plugin pay zero bundle-size cost.Two phases:
{ status: 'plugin-missing', packageManager, command }. Client surfaces a card-style toast with the detectedcd <userCwd> && <pm> add -D @helping-ai-workflow/slide-to-pptxcommand + an inline copy button.progressframes, then a finaldoneframe carrying the base64-encoded pptx. Client reassembles into a Blob and triggers a download. The progress toast mirrors the existing PDF toast UX (per-page count + an interpolated progress bar across the rendering phase).Scope discipline
Diff is strictly additive — no existing PDF / HTML / route code is refactored. The PPTX path mirrors the PDF path file-for-file (handler ↔ handler, toast ↔ toast, lib ↔ lib) so the two exports are symmetric.
Files
9 added (
packages/core/src/):vite/routes/export-pptx-handler.ts— pure handler (plugin-resolution via ESMimport()walkingnode_modulesfromuserCwd, shell-quoted install command,slideIdpath-traversal guard)vite/routes/export-pptx.ts— registersPOST /__os/api/export/pptxvite/routes/pm-detect.ts— pnpm / npm / yarn / bun detection from lockfile +packageManagerfieldapp/lib/export-pptx.ts— SSE consumer withtry/finallyreader cancellationapp/components/pptx-progress-toast.tsx— card-shaped progress toast mirroringpdf-progress-toast.tsxapp/components/pptx-plugin-missing-toast.tsx— inline copy-button toastvite/routes/export-pptx-handler.test.ts(6 cases incl. 3 path-traversal blocks)vite/routes/pm-detect.test.ts(10 cases).changeset/pptx-export-optional-plugin.md(minor bump)8 modified:
packages/core/package.json— adds@helping-ai-workflow/slide-to-pptxtopeerDependenciesMetawithoptional: truepackages/core/src/app/routes/slide.tsx— new dropdown item next to PDFpackages/core/src/locale/{en,zh-tw,zh-cn,ja,types}.ts—pptxToast.*keys +exportAsPptx/pptxExportFailedpackages/core/src/vite/api-plugin.ts— one-line route registrationTest plan
pnpm test— 236 / 236 passingtsc --noEmitcleanOpen for maintainer preference
A few defensive notes I left out to keep the diff additive — happy to address any of these in this PR or a follow-up:
SSE write on client disconnect:
res.writeinside the plugin'sonProgresscallback can throw EPIPE if the user closes the tab mid-stream. The throw currently lands in the outer try/catch (writes an error event, thenres.end()— both no-ops on a closed socket). User-impact is a noisy server log line per cancelled export. Wrappable in a silent try/catch if you'd prefer.PDF dropdown shares the same theoretical toast-id race that the PPTX side closes via a
Date.now()session suffix. Did NOT touch the existing PDF dropdown to keep the diff additive, but happy to surface a sibling fix here or in a follow-up if you'd like the convention uniform.shellQuotenot applied to the plugin name inpm-detect.installCommand— the package name is a constant today, but if you'd prefer defensive coverage for any future templating, I can add the wrap.🤖 Drafted with assistance from Claude Code
Summary by CodeRabbit
New Features
Chores
Tests