Skip to content

fix(core): accept Unicode/CJK slide ids for discovery and mutations - #366

Open
stantheman0128 wants to merge 2 commits into
1weiho:mainfrom
stantheman0128:fix/269-cjk-slide-id
Open

fix(core): accept Unicode/CJK slide ids for discovery and mutations#366
stantheman0128 wants to merge 2 commits into
1weiho:mainfrom
stantheman0128:fix/269-cjk-slide-id

Conversation

@stantheman0128

@stantheman0128 stantheman0128 commented Jul 23, 2026

Copy link
Copy Markdown

Summary

Fixes #269 (option B).

After #271, non-ASCII slide folders were dropped from discovery so they no longer appeared half-broken. This goes further: Unicode letter/digit slide ids (including CJK such as 推薦系統) are first-class for both listing and mutations.

  • Broaden SLIDE_ID_RE to /^[\p{L}\p{N}_-]+$/u (still rejects spaces, dots, separators; path-traversal checks unchanged)
  • Discovery keeps the same filter, so CJK ids now list instead of being warned away
  • Percent-encode slide id segments in client /__slides and /__assets fetches; decode path segments on the server
  • Sibling paths covered: folder assign, rename, duplicate, delete, asset list

Evidence (Windows 11, demo at http://localhost:5173, slide folder slides/推薦系統/)

listedInSlidesModule: true
createFolder: { status: 200, id: "f-2e9222cb" }
assign: { status: 200, body: { ok: true } }
assignedInManifest: true
assetsList: { status: 200, count: 0 }
rename: { status: 200, body: { ok: true, slideId: "推薦系統", name: "推薦系統演示" } }
duplicate: { status: 200, slideId: "推薦系統-copy" }
deleteDuplicate: { status: 200 }

Unit tests:

pnpm exec vitest run packages/core/src/editing/slide-ops.test.ts packages/core/src/vite/open-slide-plugin.test.ts
Test Files  2 passed (2)
Tests  67 passed (67)

AI assistance

This change was prepared with AI assistance (Cursor/Grok). I reviewed the diff, ran the commands above, and verified the product path with a live CJK slide id on Windows.

Test plan

  • Unit: CJK id accepted by SLIDE_ID_RE, listed by generateSlidesModule, duplicate/rm/resolve work
  • Live: assign / rename / duplicate / delete / asset list for 推薦系統
  • Maintainer: optional UI drag-into-folder smoke on a CJK-named slide

Summary by CodeRabbit

  • New Features
    • Slide folder IDs now support Unicode letters and digits (including CJK), enabling discovery and edit operations with those IDs.
    • Asset and slide links are generated and returned using safe encoding for slide IDs and asset names.
  • Bug Fixes
    • Improved handling of percent-encoded URL path segments with safer decoding and clearer 400 errors for invalid slide/asset identifiers.
    • Updated route behavior ensures decoded IDs are validated consistently across slide operations.

CJK and other non-ASCII folder names were either ignored after 1weiho#271 or
rejected by SLIDE_ID_RE on every edit path. Broaden the id regex, keep
path-traversal guards, and percent-encode/decode slide id URL segments.

Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

@stantheman0128 is attempting to deploy a commit to the open-slide Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8c651066-2133-45c2-96e8-0980cd5a1422

📥 Commits

Reviewing files that changed from the base of the PR and between 956c208 and 2f20927.

📒 Files selected for processing (3)
  • packages/core/src/vite/routes/assets.ts
  • packages/core/src/vite/routes/context.test.ts
  • packages/core/src/vite/routes/context.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/core/src/vite/routes/context.ts
  • packages/core/src/vite/routes/assets.ts

Walkthrough

The PR enables Unicode slide folder IDs, including CJK characters, across discovery, filesystem operations, slide routes, and asset URLs. It adds safe path-segment decoding, encoded request construction, validation updates, and tests for accepted and rejected IDs.

Changes

Unicode slide ID support

Layer / File(s) Summary
Unicode slide ID contract
packages/core/src/editing/slide-ops.ts, packages/core/src/editing/slide-ops.test.ts, packages/core/src/vite/open-slide-plugin.*, .changeset/cjk-slide-ids.md
Slide ID validation now accepts Unicode letters and digits, with discovery and filesystem tests covering CJK IDs and invalid names.
Encoded slide and asset operation URLs
packages/core/src/app/lib/assets.ts, packages/core/src/app/lib/folders.ts
Slide and asset operations consistently URL-encode IDs and path segments through shared URL construction.
Route decoding and validation
packages/core/src/vite/routes/context.ts, packages/core/src/vite/routes/context.test.ts, packages/core/src/vite/routes/slides.ts, packages/core/src/vite/routes/assets.ts
Routes safely decode path segments, reject malformed or invalid IDs, and encode asset URLs in responses.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Poem

I’m a rabbit with a CJK slide,
Through encoded paths I safely glide.
Folders bloom, mutations run,
Unicode names are no longer shunned.
Hop, hop—tests confirm the ride!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: supporting Unicode/CJK slide IDs for discovery and mutations.
Linked Issues check ✅ Passed The PR addresses #269 by broadening slide-id validation and adding URL encoding/decoding so non-ASCII ids work in discovery and mutation routes.
Out of Scope Changes check ✅ Passed The decoder tests and warning text changes are directly tied to the slide-id fix, with no clearly unrelated changes.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/vite/routes/assets.ts`:
- Around line 36-39: Update the asset route’s filename decoding to use
decodePathSegment, matching the scope decoding, and reject a null result before
calling validateAssetName or resolving the filesystem path. Ensure malformed
filename percent-encoding follows the existing invalid-path response with status
400 rather than the outer 500 handler, including the corresponding alternate
route handling.

In `@packages/core/src/vite/routes/context.ts`:
- Line 78: Update the JSDoc adjacent to the URL path-segment decoding logic by
removing the behavior-only description or replacing it with a concise WHY
explaining that malformed percent-encoding must fail closed before route
validation and filesystem resolution; do not add comments elsewhere.
🪄 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 Plus

Run ID: 62b03740-1209-4761-95dd-3f8a55748d60

📥 Commits

Reviewing files that changed from the base of the PR and between c408ba0 and 956c208.

📒 Files selected for processing (10)
  • .changeset/cjk-slide-ids.md
  • packages/core/src/app/lib/assets.ts
  • packages/core/src/app/lib/folders.ts
  • packages/core/src/editing/slide-ops.test.ts
  • packages/core/src/editing/slide-ops.ts
  • packages/core/src/vite/open-slide-plugin.test.ts
  • packages/core/src/vite/open-slide-plugin.ts
  • packages/core/src/vite/routes/assets.ts
  • packages/core/src/vite/routes/context.ts
  • packages/core/src/vite/routes/slides.ts

Comment thread packages/core/src/vite/routes/assets.ts
Comment thread packages/core/src/vite/routes/context.ts Outdated
…opes

The scope and slideId segments were routed through decodePathSegment,
which reports malformed percent-encoding as null, while the filename
segment beside them still called decodeURIComponent directly. A truncated
escape in a filename therefore threw URIError inside the request handler
and surfaced as a 500, where the identical damage in the neighbouring
segment produced a clean 400.

Route both segments through the guard and reject on null. The filename
checks compare against null explicitly because an empty segment decodes to
an empty string, which is falsy but not an error.

Add unit coverage for the decoder, including the truncated escape and lone
percent sign that motivated it, and rewrite its docstring to explain why
throwing is the wrong answer here rather than restating the signature.
@stantheman0128

Copy link
Copy Markdown
Author

Addressed CodeRabbit feedback on b6a8538: asset filename segments now use decodePathSegment (fail-closed 400 on malformed percent-encoding), matching scope decoding. pnpm exec vitest run packages/core/src/vite/open-slide-plugin.test.ts packages/core/src/editing/slide-ops.test.ts — 67 passed.

Stan Shih (@stantheman0128)

@stantheman0128

Copy link
Copy Markdown
Author

Follow-up: fork already had the same fix at \2f20927\ before this session's local commit; no additional push needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Non-ASCII slide id is listed but rejected by folder/slide mutations (can't move to a folder)

2 participants