feat(core): free-form slide tags with list filter and per-slide editor - #361
Open
D4n1984 wants to merge 1 commit into
Open
feat(core): free-form slide tags with list filter and per-slide editor#361D4n1984 wants to merge 1 commit into
D4n1984 wants to merge 1 commit into
Conversation
|
@D4n1984 is attempting to deploy a commit to the open-slide Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds free-form slide tags end to end: source metadata parsing and rewriting, virtual-module exposure, persisted list filtering, development-mode editing, server normalization, localization, and release metadata. ChangesSlide tag data flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SlideSource
participant OpenSlidePlugin
participant HomeRoute
participant SlideRoute
participant SlidesRoute
SlideSource->>OpenSlidePlugin: export meta.tags
OpenSlidePlugin->>HomeRoute: slideTags virtual-module map
HomeRoute->>HomeRoute: filter slides by active tags
SlideRoute->>SlidesRoute: PATCH slide tags
SlidesRoute->>SlideSource: rewrite meta.tags
SlidesRoute->>OpenSlidePlugin: invalidate virtual module
OpenSlidePlugin->>HomeRoute: refreshed tag metadata
Suggested reviewers: Possibly related PRs
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: 2
🧹 Nitpick comments (4)
packages/core/src/app/components/tag-combobox.tsx (1)
116-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCombobox listbox semantics are incomplete for screen readers.
The input is
role="combobox"witharia-controls={listId}, but the popup<ul>has norole="listbox", the option buttons have norole="option"/aria-selected, and there's noaria-activedescendanttrackingactiveIndex. Keyboard navigation works visually, but assistive tech won't announce the highlighted option as the user arrows through the list.Consider adding
role="listbox"to the<ul>,role="option"+aria-selectedto each option, stable ids, andaria-activedescendanton the input.🤖 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/tag-combobox.tsx` around lines 116 - 193, Complete the combobox accessibility wiring around the input and popup: add aria-activedescendant based on activeIndex, give the <ul> a listbox role, and assign each filtered and create option a stable unique id with role="option" and aria-selected reflecting whether it is active. Ensure the active-descendant id matches the currently highlighted option while preserving existing keyboard and mouse behavior.packages/core/src/vite/routes/slides.ts (1)
33-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the comment to WHY, not WHAT.
The comment enumerates the exact transformation steps the code already makes obvious (trim/lowercase/dash/strip/cap). As per coding guidelines,
**/*.{ts,tsx,js,jsx}should default to no comments and only explain non-obvious WHY; here it's describing WHAT the code does.✏️ Suggested trim
-// Normalise a client-supplied tag server-side: trim, lowercase, collapse -// whitespace to dashes, strip anything outside letters/numbers/._-, and cap the -// length. Returns null for anything that ends up empty or too long. +// Mirrors the client-side tag normalization so PATCH stays authoritative even if the client sends raw input. function sanitizeTag(v: unknown): string | null {🤖 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/slides.ts` around lines 33 - 44, Remove the step-by-step implementation description above sanitizeTag and replace it with a concise comment only if needed to explain the non-obvious reason this client-supplied tag normalization is required; otherwise remove the comment entirely. Keep sanitizeTag’s existing behavior unchanged.Source: Coding guidelines
packages/core/src/editing/slide-ops.test.ts (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the added coverage overall — solid scenarios for replace, inject, escape, clear, and decoy-string handling.
One gap: no test exercises the
'unsafe'branch offindMetaTagsArrayRange(i.e.tagspresent but not an array literal), which is the path that preventsupdateMetaTagsInSourcefrom silently mangling non-arraytagsvalues.✅ Suggested additional test
it('returns null when tags is not an array literal', () => { const source = `export const meta = { tags: getDefaultTags() };\nexport default [];\n`; expect(updateMetaTagsInSource(source, ['x'])).toBeNull(); });Also applies to: 154-221
🤖 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.test.ts` at line 13, The test coverage for updateMetaTagsInSource is missing the unsafe findMetaTagsArrayRange path. Add a test where tags is assigned a non-array expression such as getDefaultTags(), and assert that updateMetaTagsInSource returns null while preserving existing scenarios.packages/core/src/editing/slide-ops.ts (1)
356-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared brace-matching/injection logic to avoid duplicating
updateMetaTitleInSource.The
metaobject location, brace-matching, and "inject as first property preserving indentation" logic (lines 359-379, 387-395) are copy-pasted fromupdateMetaTitleInSource. A shared helper (e.g.locateMetaObjectBraces(source)+ a genericinjectFirstMetaProperty(body, keyLiteral)) would reduce duplication and let both bug fixes and future meta fields live in one place.🤖 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 356 - 403, Extract the duplicated meta-object lookup, brace matching, and first-property indentation-preserving injection from updateMetaTagsInSource and updateMetaTitleInSource into shared helpers such as locateMetaObjectBraces and injectFirstMetaProperty. Update both functions to use the helpers while preserving their existing replacement, insertion, and null-return behavior.
🤖 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/routes/slide.tsx`:
- Around line 598-604: Update the DEV-only SlideTagsControl integration around
setSlideTags so rejected tag-save promises are caught and surfaced through the
existing toast/error-feedback mechanism, rather than remaining unhandled. Keep
the onSave callback compatible with SlideTagsControl’s void callback contract
and preserve successful saves.
In `@packages/core/src/editing/slide-ops.ts`:
- Around line 366-379: Update the brace-scanning loop in the changed meta-object
rewrite flow to skip quoted string literals using the existing skipStringLiteral
helper before evaluating braces, preserving index advancement for the skipped
span. Apply the same string-aware scanning fix to updateMetaTitleInSource, whose
matching loop has the identical vulnerability, so braces inside string values
cannot affect depth or rewrite ranges.
---
Nitpick comments:
In `@packages/core/src/app/components/tag-combobox.tsx`:
- Around line 116-193: Complete the combobox accessibility wiring around the
input and popup: add aria-activedescendant based on activeIndex, give the <ul> a
listbox role, and assign each filtered and create option a stable unique id with
role="option" and aria-selected reflecting whether it is active. Ensure the
active-descendant id matches the currently highlighted option while preserving
existing keyboard and mouse behavior.
In `@packages/core/src/editing/slide-ops.test.ts`:
- Line 13: The test coverage for updateMetaTagsInSource is missing the unsafe
findMetaTagsArrayRange path. Add a test where tags is assigned a non-array
expression such as getDefaultTags(), and assert that updateMetaTagsInSource
returns null while preserving existing scenarios.
In `@packages/core/src/editing/slide-ops.ts`:
- Around line 356-403: Extract the duplicated meta-object lookup, brace
matching, and first-property indentation-preserving injection from
updateMetaTagsInSource and updateMetaTitleInSource into shared helpers such as
locateMetaObjectBraces and injectFirstMetaProperty. Update both functions to use
the helpers while preserving their existing replacement, insertion, and
null-return behavior.
In `@packages/core/src/vite/routes/slides.ts`:
- Around line 33-44: Remove the step-by-step implementation description above
sanitizeTag and replace it with a concise comment only if needed to explain the
non-obvious reason this client-supplied tag normalization is required; otherwise
remove the comment entirely. Keep sanitizeTag’s existing behavior unchanged.
🪄 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: 787a92f2-586d-4274-886e-f81857bf22de
📒 Files selected for processing (18)
.changeset/slide-tags.mdpackages/core/src/app/components/tag-combobox.tsxpackages/core/src/app/lib/folders.tspackages/core/src/app/lib/sdk.tspackages/core/src/app/lib/slides.tspackages/core/src/app/routes/home.tsxpackages/core/src/app/routes/slide.tsxpackages/core/src/app/virtual.d.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/vite/open-slide-plugin.test.tspackages/core/src/vite/open-slide-plugin.tspackages/core/src/vite/routes/slides.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/routes/slide.tsx`:
- Around line 1077-1105: Update the tag editor around the Popover onOpenChange
save flow to track whether onSave is in progress, keep the PopoverTrigger button
unavailable while that save is pending, and clear the pending state on both
success and failure. Preserve the existing changed-check and error toast,
ensuring subsequent edits cannot start until the current save settles.
In `@packages/core/src/vite/open-slide-plugin.ts`:
- Line 117: Update the metadata-boundary scanning logic in the manifest
generation flow to ignore braces inside quoted string literals, including titles
like `title: 'contains }'`, so scanning continues to the actual closing meta
brace and preserves the slide’s tags. Reuse the string-aware scanning behavior
from updateMetaTagsInSource rather than maintaining a separate parsing rule.
In `@packages/core/src/vite/routes/slides.ts`:
- Around line 33-35: Remove the implementation-narrating comment above
sanitizeTag. Keep the code unchanged, unless replacing it with a concise comment
documenting the non-obvious policy rationale for the tag normalization rules.
- Around line 229-236: Update the shared findMetaTagsArrayRange scanner to
ignore both line and block comments when locating meta.tags, so it only matches
actual metadata properties. Apply this root-cause fix for the PATCH flow at
packages/core/src/vite/routes/slides.ts#L229-L236 and generated slideTags
handling at packages/core/src/vite/open-slide-plugin.ts#L72-L84; both sites
require no separate logic changes beyond using the corrected scanner, and cover
both paths with tests.
🪄 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: b1efb27c-3bda-4b04-88e5-4f983f4241e7
📒 Files selected for processing (18)
.changeset/slide-tags.mdpackages/core/src/app/components/tag-combobox.tsxpackages/core/src/app/lib/folders.tspackages/core/src/app/lib/sdk.tspackages/core/src/app/lib/slides.tspackages/core/src/app/routes/home.tsxpackages/core/src/app/routes/slide.tsxpackages/core/src/app/virtual.d.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/vite/open-slide-plugin.test.tspackages/core/src/vite/open-slide-plugin.tspackages/core/src/vite/routes/slides.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/core/src/app/virtual.d.ts
- .changeset/slide-tags.md
- packages/core/src/locale/en.ts
- packages/core/src/locale/zh-tw.ts
- packages/core/src/app/lib/sdk.ts
- packages/core/src/locale/zh-cn.ts
- packages/core/src/locale/ja.ts
- packages/core/src/app/lib/slides.ts
- packages/core/src/locale/types.ts
- packages/core/src/app/routes/home.tsx
- packages/core/src/editing/slide-ops.test.ts
- packages/core/src/app/lib/folders.ts
- packages/core/src/app/components/tag-combobox.tsx
- packages/core/src/editing/slide-ops.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/core/src/locale/types.ts (1)
140-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the WHAT-only template comment.
This comment describes the expected translation format rather than a non-obvious constraint or rationale. Remove it, or rewrite it to explain why the
{name}placeholder must be preserved.🤖 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/locale/types.ts` around lines 140 - 141, Remove the template-only comment immediately above the createTag property in the locale types definition, leaving the createTag declaration unchanged.Source: Coding guidelines
packages/core/src/editing/slide-ops.ts (1)
257-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove comments that restate code or test assertions. Keep only concise rationale for genuinely surprising scanner constraints.
packages/core/src/editing/slide-ops.ts#L257-L257: remove the helper-description comment.packages/core/src/editing/slide-ops.ts#L273-L278: reduce to a brief non-obvious rationale, if needed.packages/core/src/editing/slide-ops.ts#L295-L299: reduce to a brief non-obvious rationale, if needed.packages/core/src/editing/slide-ops.ts#L326-L335: remove the API/process description.packages/core/src/editing/slide-ops.ts#L409-L421: remove the rewrite-step description.packages/core/src/vite/open-slide-plugin.test.ts#L75-L76: remove the assertion-restating comment.As per coding guidelines,
**/*.{ts,tsx,js,jsx}must default to no comments unless documenting a non-obvious WHY.🤖 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` at line 257, Remove the helper-description comment at packages/core/src/editing/slide-ops.ts:257-257, the API/process description at 326-335, the rewrite-step description at 409-421, and the assertion-restating comment at packages/core/src/vite/open-slide-plugin.test.ts:75-76. At slide-ops.ts:273-278 and 295-299, retain comments only if they concisely explain a genuinely non-obvious scanner constraint; otherwise remove them. Keep code behavior unchanged and follow the no-comments-by-default guideline.Source: Coding guidelines
packages/core/src/vite/routes/slides.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the implementation-narrating route comment.
The added text describes what the PATCH endpoint writes rather than a non-obvious constraint. As per coding guidelines, “Default to writing no comments. Only add one when the WHY is non-obvious.”
🤖 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/slides.ts` at line 27, Remove the implementation-narrating comment above the PATCH route in the slides routing definition; leave the route and its behavior unchanged.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/editing/slide-ops.ts`:
- Around line 345-375: Update the tags-key detection in the surrounding source
scan used by updateMetaTagsInSource to recognize quoted keys such as 'tags' and
"tags", not only bare identifiers. Ensure quoted tags properties are treated as
existing and handled safely; if they cannot be parsed reliably, return 'unsafe'
instead of inserting a duplicate tags key.
In `@packages/core/src/vite/open-slide-plugin.ts`:
- Around line 70-84: The parseTags function currently matches quoted text inside
comments; replace the TAG_ELEMENT_RE-based loop with comment-aware array-element
scanning consistent with findMetaTagsArrayRange, so values such as commented-out
strings are ignored while active quoted tags are preserved. Add a manifest test
covering tags: [/* 'internal' */ 'public'] and verify only public is returned.
In `@packages/core/src/vite/routes/slides.ts`:
- Around line 217-224: Update updateMetaTitleInSource to locate the metadata
object’s closing brace with the string-aware matchMetaBrace scanner, matching
the boundary handling already used by updateMetaTagsInSource. Ensure titles
containing braces inside quoted strings do not cause duplicate insertion or
leave the original title taking precedence.
---
Nitpick comments:
In `@packages/core/src/editing/slide-ops.ts`:
- Line 257: Remove the helper-description comment at
packages/core/src/editing/slide-ops.ts:257-257, the API/process description at
326-335, the rewrite-step description at 409-421, and the assertion-restating
comment at packages/core/src/vite/open-slide-plugin.test.ts:75-76. At
slide-ops.ts:273-278 and 295-299, retain comments only if they concisely explain
a genuinely non-obvious scanner constraint; otherwise remove them. Keep code
behavior unchanged and follow the no-comments-by-default guideline.
In `@packages/core/src/locale/types.ts`:
- Around line 140-141: Remove the template-only comment immediately above the
createTag property in the locale types definition, leaving the createTag
declaration unchanged.
In `@packages/core/src/vite/routes/slides.ts`:
- Line 27: Remove the implementation-narrating comment above the PATCH route in
the slides routing definition; leave the route and its behavior unchanged.
🪄 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: 53c9d655-9a7b-4141-a810-06116ee1a7fa
📒 Files selected for processing (18)
.changeset/slide-tags.mdpackages/core/src/app/components/tag-combobox.tsxpackages/core/src/app/lib/folders.tspackages/core/src/app/lib/sdk.tspackages/core/src/app/lib/slides.tspackages/core/src/app/routes/home.tsxpackages/core/src/app/routes/slide.tsxpackages/core/src/app/virtual.d.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/vite/open-slide-plugin.test.tspackages/core/src/vite/open-slide-plugin.tspackages/core/src/vite/routes/slides.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- .changeset/slide-tags.md
- packages/core/src/app/virtual.d.ts
- packages/core/src/locale/zh-cn.ts
- packages/core/src/app/lib/slides.ts
- packages/core/src/locale/ja.ts
- packages/core/src/app/lib/sdk.ts
- packages/core/src/locale/en.ts
- packages/core/src/editing/slide-ops.test.ts
- packages/core/src/app/routes/home.tsx
- packages/core/src/app/components/tag-combobox.tsx
- packages/core/src/app/lib/folders.ts
- packages/core/src/app/routes/slide.tsx
- Add SlideMeta.tags and surface it end to end: the vite plugin parses a string-literal tags array from each slide's meta and emits slideTags in virtual:open-slide/slides (typed + re-exported from app/lib/slides). - New TagCombobox token-input component (chips inside the box, keyboard nav, optional create) used by both the home filter and the slide editor. - Home list filter: header combobox whose suggestions are the union of tags in the current view, AND-combined with the text search, persisted in localStorage, stale selections ignored, folio reflects filtering. - Meta write: updateMetaTagsInSource mirrors updateMetaTitleInSource, with unit tests; PATCH /__slides/:id now accepts optional sanitized tags alongside name. - Client patchSlideTags + useFolders.setSlideTags; DEV-only SlideTagsControl in the slide top bar persists once on popover close. - Locale keys (filter/editor) across en/ja/zh-cn/zh-tw + types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/open-slide-plugin.test.ts`:
- Around line 75-76: Remove the implementation-narrating comment above the
relevant test in the open-slide plugin test; leave the test name, assertions,
and behavior unchanged.
In `@packages/core/src/vite/routes/slides.ts`:
- Around line 23-28: Remove the endpoint inventory comment block above the slide
route handlers in slides.ts, leaving the route implementations and request types
unchanged.
- Around line 183-186: Validate that the parsed body in the slide patch route is
a non-null, non-array object before reading name or tags. Return the existing
400 “nothing to update” response for primitives, null, arrays, and objects
without name/tags; only then evaluate hasName and hasTags.
- Around line 235-245: Move the slides virtual-module invalidation and server.ws
full-reload logic into the updated !== source branch so PATCH requests that make
no changes do not notify or reload clients. Keep the existing write behavior and
reload sequence unchanged when the source actually changes.
🪄 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: 69bc1e29-8fce-4c52-9b04-7768adb315a3
📒 Files selected for processing (18)
.changeset/slide-tags.mdpackages/core/src/app/components/tag-combobox.tsxpackages/core/src/app/lib/folders.tspackages/core/src/app/lib/sdk.tspackages/core/src/app/lib/slides.tspackages/core/src/app/routes/home.tsxpackages/core/src/app/routes/slide.tsxpackages/core/src/app/virtual.d.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/vite/open-slide-plugin.test.tspackages/core/src/vite/open-slide-plugin.tspackages/core/src/vite/routes/slides.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/core/src/app/lib/slides.ts
- packages/core/src/app/virtual.d.ts
- packages/core/src/app/lib/sdk.ts
- packages/core/src/app/lib/folders.ts
- .changeset/slide-tags.md
- packages/core/src/locale/en.ts
- packages/core/src/locale/zh-cn.ts
- packages/core/src/locale/ja.ts
- packages/core/src/locale/types.ts
- packages/core/src/editing/slide-ops.test.ts
- packages/core/src/app/components/tag-combobox.tsx
- packages/core/src/locale/zh-tw.ts
- packages/core/src/vite/open-slide-plugin.ts
- packages/core/src/app/routes/home.tsx
- packages/core/src/editing/slide-ops.ts
- packages/core/src/app/routes/slide.tsx
Comment on lines
+75
to
+76
| // The escaped quote is decoded and the [decoy] inside the title string is | ||
| // not mistaken for the tags array; slides without tags are omitted. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the implementation-narrating test comment.
Lines 75-76 restate the test name and assertion rather than documenting a hidden constraint. As per coding guidelines, “Default to writing no comments. Only add one when the WHY is non-obvious.”
🤖 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/open-slide-plugin.test.ts` around lines 75 - 76,
Remove the implementation-narrating comment above the relevant test in the
open-slide plugin test; leave the test name, assertions, and behavior unchanged.
Source: Coding guidelines
Comment on lines
23
to
28
| // PUT /__slides/:id/reorder reorder pages { order: number[] } | ||
| // DELETE /__slides/:id/pages/:i remove page | ||
| // POST /__slides/:id/pages/:i/duplicate duplicate page | ||
| // POST /__slides/:id/duplicate duplicate slide directory { newId? } | ||
| // PATCH /__slides/:id rename slide (writes meta.title) | ||
| // PATCH /__slides/:id rename slide + edit tags (writes meta.title/meta.tags) | ||
| // DELETE /__slides/:id delete slide directory + folder assignment |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the endpoint inventory comment block.
This documents what each route does rather than a non-obvious constraint; the route handlers and request types are the source of truth. As per coding guidelines, “Don't explain WHAT the code does” and “don't write 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/slides.ts` around lines 23 - 28, Remove the
endpoint inventory comment block above the slide route handlers in slides.ts,
leaving the route implementations and request types unchanged.
Source: Coding guidelines
Comment on lines
183
to
+186
| const body = (await readBody(req)) as SlidePatchBody; | ||
| const name = validateSlideName(body.name); | ||
| if (!name) return json(res, 400, { error: 'invalid name' }); | ||
| const hasName = body.name !== undefined; | ||
| const hasTags = body.tags !== undefined; | ||
| if (!hasName && !hasTags) return json(res, 400, { error: 'nothing to update' }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/core/src/vite/routes/context.ts --match readBody --view expanded
sed -n '1,140p' packages/core/src/vite/routes/context.tsRepository: 1weiho/open-slide
Length of output: 2433
Reject non-object bodies before patching.
readBody parses valid JSON directly, so null, true, "x", 1, [...], or { "a": 1 } all pass through and cause the route to read missing properties instead of returning 400 “nothing to update”. Add an object check before deconstructing body.
🤖 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/slides.ts` around lines 183 - 186, Validate
that the parsed body in the slide patch route is a non-null, non-array object
before reading name or tags. Return the existing 400 “nothing to update”
response for primitives, null, arrays, and objects without name/tags; only then
evaluate hasName and hasTags.
Comment on lines
235
to
245
| if (updated !== source) { | ||
| await fs.writeFile(entry, updated, 'utf8'); | ||
| } | ||
| // The TSX edit lands through Vite's normal HMR pipeline, but the | ||
| // React state holding `slide.meta` in the editor won't re-fetch on | ||
| // its own — tell every client to refresh so the new title shows up. | ||
| // its own — tell every client to refresh so the new title/tags show up. | ||
| // Invalidate the slides virtual module first so the reload rebuilds it | ||
| // with the new meta.tags rather than racing the debounced file watcher. | ||
| const slidesMod = server.moduleGraph.getModuleById(`\0${SLIDES_VMOD}`); | ||
| if (slidesMod) server.moduleGraph.invalidateModule(slidesMod); | ||
| server.ws.send({ type: 'full-reload' }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid reloading clients when the PATCH is a no-op.
Lines 235-237 correctly skip the write, but Lines 243-245 still invalidate and fully reload every connected dev client. Keep invalidation and reload inside the changed-source branch.
Proposed fix
if (updated !== source) {
await fs.writeFile(entry, updated, 'utf8');
+ const slidesMod = server.moduleGraph.getModuleById(`\0${SLIDES_VMOD}`);
+ if (slidesMod) server.moduleGraph.invalidateModule(slidesMod);
+ server.ws.send({ type: 'full-reload' });
}
-const slidesMod = server.moduleGraph.getModuleById(`\0${SLIDES_VMOD}`);
-if (slidesMod) server.moduleGraph.invalidateModule(slidesMod);
-server.ws.send({ type: 'full-reload' });🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 235-235: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(entry, updated, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/slides.ts` around lines 235 - 245, Move the
slides virtual-module invalidation and server.ws full-reload logic into the
updated !== source branch so PATCH requests that make no changes do not notify
or reload clients. Keep the existing write behavior and reload sequence
unchanged when the source actually changes.
This was referenced Jul 23, 2026
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.
Closes #360
Summary
Adds free-form slide tags end to end: declare
tags: ['en', 'architecture']in a deck'smeta, filter the slide list with a token-input combobox, and edit a slide's tags from the slide view with autocomplete of the existing vocabulary — persisted back into the slide'sindex.tsx.What's included
SlideMeta.tags?: string[]— plain string tags inexport const meta. Backward compatible; decks without tags behave as before.slideTagsmap viavirtual:open-slide/slides(mirroringslideThemes), so the list can filter without loading slide modules.localStorage. Stale selections are ignored rather than hiding everything.allowCreatemode: it autocompletes existing tags across the repo and creates new ones as you type (normalized: trim, lowercase, whitespace→dashes). Changes persist once on close, writingmeta.tagsviaPATCH /__slides/:id— mirroring the rename flow — with server-side sanitization (charset/length caps, dedupe).updateMetaTagsInSourceinediting/slide-ops.ts, mirroringupdateMetaTitleInSource, with unit tests (replace / insert / create-meta / unsafe→null / escaping).TagComboboxcomponent built on existing primitives — no new dependencies.en,ja,zh-CN,zh-TW; changeset included (minor).Independent of (and composable with) the folder-nesting work in #199 — tags cover the cross-cutting dimensions (language, topic) that folders don't.
Testing
pnpm typecheck✅ ·pnpm check(biome) ✅ ·pnpm test316/316 ✅ (includes 10 newslide-opstag tests and a plugin tags-extraction test)pnpm --filter @open-slide/core build✅Summary by CodeRabbit
New Features
Bug Fixes
Tests