feat(core): toggle slide canvas between 16:9 and 4:3#191
Open
itskylechung wants to merge 4 commits into
Open
Conversation
Adds a meta.aspect field on slides ('16:9' default, '4:3' opt-in)
and a DEV-only toolbar button that flips it. The new ratio flows
through the editor canvas, thumbnail rails, presenter, present
mode, and HTML/PDF exports.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
updateMetaAspectInSource was stripping the `aspect: '4:3',` substring but leaving the surrounding indentation + newline, so the meta object grew a blank, indented line on each round-trip. The removal path now eats the whole source line. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@itskylechung 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 (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds slide aspect-ratio support (16:9 and 4:3): new SlideAspect type and getCanvasDims, source rewrite and persistence, dev PUT route, component prop threading (Player/SlideCanvas/thumbnails/presenter), export sizing for HTML/PDF, localization, and a Changeset release note. ChangesSlide Aspect Ratio Toggle
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 821eaf23d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const exportDefaultIdx = source.search(/export\s+default\b/); | ||
| if (exportDefaultIdx === -1) return null; | ||
| const insertion = `export const meta: SlideMeta = { aspect: ${newLiteral} };\n\n`; |
There was a problem hiding this comment.
When toggling a valid slide module that has no export const meta block (for example, just export default [Cover] satisfies Page[];) to 4:3, this branch inserts export const meta: SlideMeta = ... without adding or ensuring a SlideMeta import. That leaves the slide with an unresolved type name and breaks the dev/build type pipeline immediately after using the new aspect toggle on such slides.
Useful? React with 👍 / 👎.
…lue mirror DEFAULT_ASPECT was exported but never consumed. Document why the editor-side SlideAspectValue union is kept separate from the client SlideAspect type. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![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 (1)
packages/core/src/editing/slide-ops.ts (1)
257-260: ⚡ Quick winReuse the shared aspect contract here.
This redefines the same union/default that
packages/core/src/app/lib/sdk.tsalready exports, so the persistence layer can drift from the runtime/UI contract over time. Importtype SlideAspectandDEFAULT_ASPECTinstead of duplicating the literals here.Also applies to: 269-270
🤖 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 257 - 260, Replace the duplicated union with the shared SDK contract: import the exported type SlideAspect and DEFAULT_ASPECT from the shared module and use SlideAspect in place of the local SlideAspectValue type, and update validateSlideAspect to accept unknown and return SlideAspect | null using the shared literals (and fallback/default where appropriate); also update the other occurrences that repeat the literals (previously at the same block) to reference DEFAULT_ASPECT instead of hardcoded '16:9'/'4:3' so persistence and runtime use the single source of truth.
🤖 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/home.tsx`:
- Line 463: The outer preview shell is hardcoded to "aspect-video" while
SlideCanvas gets slide?.meta?.aspect, causing mismatched thumbnails; update the
outer preview container (the element wrapping SlideCanvas near the SlideCanvas
invocation) to use the slide's aspect (e.g., compute a className based on
slide?.meta?.aspect with a sensible default like "aspect-video") so the wrapper
and SlideCanvas share the same aspect ratio; look for the wrapper element around
SlideCanvas and replace the fixed "aspect-video" with a dynamic class derived
from slide?.meta?.aspect (or map common ratio strings to Tailwind aspect
classes) so 4:3 slides render correctly.
In `@packages/core/src/editing/slide-ops.ts`:
- Around line 298-312: When removing the matched property in the remove branch,
extend the deletion span to also consume same-line trailing whitespace and
comments: after computing lineEnd = idx + matchLen, advance lineEnd over
spaces/tabs, then if you see '//' advance to the next newline, or if you see
'/*' advance until the closing '*/' (inclusive); finally if the next char is a
newline consume it as well before building before/after and stripping the
dangling comma. Update the block around lineEnd (the variables body, lineEnd,
idx, match, openBrace, closeBrace) to perform these extra consumes so trailing
inline comments are removed with the property.
---
Nitpick comments:
In `@packages/core/src/editing/slide-ops.ts`:
- Around line 257-260: Replace the duplicated union with the shared SDK
contract: import the exported type SlideAspect and DEFAULT_ASPECT from the
shared module and use SlideAspect in place of the local SlideAspectValue type,
and update validateSlideAspect to accept unknown and return SlideAspect | null
using the shared literals (and fallback/default where appropriate); also update
the other occurrences that repeat the literals (previously at the same block) to
reference DEFAULT_ASPECT instead of hardcoded '16:9'/'4:3' so persistence and
runtime use the single source of truth.
🪄 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: ec0ffbda-6731-4426-9756-3154b81c3748
📒 Files selected for processing (20)
.changeset/aspect-toggle.mdpackages/core/src/app/components/player.tsxpackages/core/src/app/components/present/overview-grid.tsxpackages/core/src/app/components/slide-canvas.tsxpackages/core/src/app/components/thumbnail-rail.tsxpackages/core/src/app/lib/export-html.tspackages/core/src/app/lib/export-pdf.tspackages/core/src/app/lib/sdk.test.tspackages/core/src/app/lib/sdk.tspackages/core/src/app/routes/home.tsxpackages/core/src/app/routes/presenter.tsxpackages/core/src/app/routes/slide.tsxpackages/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/routes/slides.ts
- Emit untyped `export const meta` when inserting into a slide with no meta block, so a missing SlideMeta import can't dangle (Codex). - Eat a same-line trailing comment when removing the aspect property so it isn't orphaned inside meta (CodeRabbit). - Make the home-card frame aspect-aware so 4:3 previews aren't pillarboxed in a 16:9 shell (CodeRabbit). Co-Authored-By: Claude Opus 4.7 <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
Adds a per-slide aspect-ratio toggle so a deck can mix 16:9 and 4:3 slides. A
meta.aspectfield ('16:9'default,'4:3'opt-in) drives the canvas dimensions everywhere a slide is rendered, and a DEV-only toolbar button flips it.How
meta.aspect— new optionalSlideAspectfield onSlideMeta.getCanvasDims()insdk.tsmaps it to dimensions (16:9 → 1920×1080, 4:3 → 1440×1080; height stays fixed so existing 1080-tall content isn't rescaled, the 4:3 frame just narrows).SlideCanvastakes anaspectprop; threaded through the editor canvas, thumbnail rails (vertical + horizontal), presenter, present mode + overview grid, and home-page card previews.export-pdf(@page size, frame + supersample dims) andexport-html(.os-framesize, fit script) read the slide's aspect.setSlideAspect/updateMetaAspectInSourceeditmeta.aspectinindex.tsxvia a newPUT /__slides/:id/aspectdev endpoint. Resetting to 16:9 removes the field (it's the default) and cleans up the line so the file doesn't accrete blank lines.16:9/4:3control next to the Inspect toggle.Tests
getCanvasDimsunit tests (default / 16:9 / 4:3).updateMetaAspectInSourcetests covering insert / replace / remove / no-op / fresh-meta, including the no-stray-blank-line invariant.typecheck+biome checkclean.A changeset (
minor) is included.Summary by CodeRabbit
New Features
Localization