feat(core): warn on shared-image edits and add split-shared-images skill#236
Open
ridemountainpig wants to merge 2 commits into
Open
feat(core): warn on shared-image edits and add split-shared-images skill#236ridemountainpig wants to merge 2 commits into
ridemountainpig wants to merge 2 commits into
Conversation
Warn in the inspector's replace/crop dialogs when an image is rendered from a shared component or map body, add a split-shared-images skill to refactor those slides, and update the slide-authoring skill to keep each <img> at its own call site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@ridemountainpig 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 (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds guardrails for shared-image editing in the inspector. A new ChangesShared Image Guardrails
Sequence DiagramsequenceDiagram
participant User as User
participant Inspector as InspectorProvider
participant useEditor as useEditor
participant Vite as GET /__edit/element-info
participant Probe as probeElementSharing
participant Dialog as AssetPickerDialog / ImageCropDialog
participant Notice as SharedImageNotice
User->>Inspector: Click Replace or Crop on an image
Inspector->>useEditor: fetchElementSharing(slideId, line, col)
useEditor->>Vite: GET ?slideId=&line=&col=
Vite->>Probe: probeElementSharing(source, line, col)
Probe-->>Vite: {instances: N, viaMap: bool} | null
Vite-->>useEditor: 200 JSON / 422 no element
useEditor-->>Inspector: ElementSharing | null
Inspector->>Inspector: update sharing state via probeSharing
Inspector->>Dialog: sharedNotice = sharingFor(target)
Dialog->>Notice: render with notice={instances, viaMap}
Notice-->>User: Warning message + split-shared-images skill hint
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 3
🧹 Nitpick comments (1)
.changeset/shared-image-guardrails.md (1)
6-6: 💤 Low valueTrim the changeset summary.
This line is more detailed than the repo’s changeset rule allows. Keep it as a short, direct, one-line user-facing summary; the warning/split-skill details belong in the PR body, not the changeset. As per coding guidelines, “Changeset descriptions must be short and direct: one line, present-tense, describing what changed from a user's perspective.”
🤖 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 @.changeset/shared-image-guardrails.md at line 6, The changeset summary in the shared-image-guardrails.md file is too detailed and violates the repository's changeset guidelines. Replace the current multi-part technical description with a single, short, one-line present-tense summary that describes what changed from a user's perspective only. Remove all implementation details like the specific skill names and technical refactoring information—those details belong in the PR body, not in the changeset file.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/skills/split-shared-images/SKILL.md`:
- Around line 18-19: The skill documentation at
packages/core/skills/split-shared-images/SKILL.md (lines 18-19) and
packages/cli/template/.agents/skills/split-shared-images/SKILL.md (lines 18-19)
includes both `<img>` and `<video>` in the trigger pattern list, but the rest of
the guide only describes image-specific refactoring without video-specific
guidance. Remove `<video>` references from the pattern list at both locations
(in both the core SKILL.md and template SKILL.md files) to keep the skill
documentation focused on images only and aligned with the actual inspector
behavior and refactoring steps documented.
In `@packages/core/src/app/components/inspector/shared-image-notice.tsx`:
- Around line 9-12: The message selection logic in the variable assignment is
using the wrong condition to determine which warning text to display. Replace
the condition `notice.instances > 1` with `notice.viaMap` as the discriminator
to properly select the map-specific warning text, while still retaining the use
of `notice.instances` for count formatting in the format function call when
appropriate.
In `@packages/core/src/vite/routes/edit.ts`:
- Around line 34-47: The column parameter is being coerced to 0 for invalid
values instead of being validated and rejected. Add validation for the columnRaw
variable after line 34, similar to the existing line validation, to check that
columnRaw is a finite number and greater than or equal to 0. If validation
fails, return a json error response with status 400 and an appropriate error
message. Then update line 46 where probeElementSharing is called to use the
validated columnRaw directly instead of the ternary operator that defaults
invalid values to 0.
---
Nitpick comments:
In @.changeset/shared-image-guardrails.md:
- Line 6: The changeset summary in the shared-image-guardrails.md file is too
detailed and violates the repository's changeset guidelines. Replace the current
multi-part technical description with a single, short, one-line present-tense
summary that describes what changed from a user's perspective only. Remove all
implementation details like the specific skill names and technical refactoring
information—those details belong in the PR body, not in the changeset file.
🪄 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: 4cfcad29-f4b7-48b3-ab62-62c5b1855d92
📒 Files selected for processing (22)
.changeset/shared-image-guardrails.mdapps/demo/.agents/skills/split-shared-imagesapps/demo/.claude/skills/split-shared-imagespackages/cli/template/.agents/skills/slide-authoring/SKILL.mdpackages/cli/template/.agents/skills/split-shared-images/SKILL.mdpackages/cli/template/.claude/skills/split-shared-imagespackages/cli/template/AGENTS.mdpackages/core/skills/slide-authoring/SKILL.mdpackages/core/skills/split-shared-images/SKILL.mdpackages/core/src/app/components/inspector/asset-picker-dialog.tsxpackages/core/src/app/components/inspector/image-crop-dialog.tsxpackages/core/src/app/components/inspector/inspector-provider.tsxpackages/core/src/app/components/inspector/shared-image-notice.tsxpackages/core/src/app/lib/inspector/use-editor.tspackages/core/src/editing/edit-ops.test.tspackages/core/src/editing/edit-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/routes/edit.ts
- Reject invalid `column` query param in /element-info instead of coercing to 0 - Use `viaMap` (not instance count) to pick the map-specific warning text - Drop `<video>` from the split-shared-images trigger list (image-only skill) - Trim the changeset summary to one user-facing line Co-Authored-By: Claude Opus 4.8 <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.
What
split-shared-imagesskill: refactors slides where one<img>is reused across multiple call sites so each image can be edited independently.slide-authoringskill update: guidance to keep each<img>at its own call site (pass the image as children rather than asrcprop) so the inspector can replace one at a time.Close #213.
Notes
patchfor@open-slide/coreand@open-slide/cli).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Localization