Feature/verse on image#963
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces the complete "verse on image" feature: users can now select/crop images, overlay and edit verse text with typography controls and image filters, and export as PNG for sharing. New pages at ChangesVerse on Image Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
Some of it still needs to be updated to Svelte 5 as well. |
FyreByrd
requested changes
May 27, 2026
FyreByrd
left a comment
FyreByrd
left a comment
Collaborator
There was a problem hiding this comment.
Well. The code itself works, and it works pretty well. I was able to generate an image following largely the same process that is present in the native app.
However, there are definitely quite a few design issues, most of which were inherited from the original PR, so I wouldn't feel too bad about it.
Here's what I noticed in addition to the individual review comments:
- The image page could use a little bit of padding/margin on the top and bottom.
- I used a really large image on the image upload page
/crop. It may be good to have a maximum size for the image, at least when rendering to the page? It may also be beneficial to display the image dimensions somewhere on the page (or just the selection's dimensions) - I think it would be helpful to disable native text selection within the image in the verse on image page. (I found it really annoying when trying to drag the text around that it kept trying to highlight).
Any additional thoughts @chrisvire ? I know you had said you had some issues with the original PR.
Collaborator
There was a problem hiding this comment.
Please convert this to use TypeScript
Contributor
Author
There was a problem hiding this comment.
I think I've gotten it pretty well converted. Is there anything else you want me to change on the TypeScript front?
Collaborator
There was a problem hiding this comment.
Several variables are shown as being possibly null/undefined and are showing errors in the editor.
TheNonPirate
force-pushed
the
feature/verse-on-image
branch
from
2eb3131 to
eecf3aa
Compare
Rebased on main
Rebased on main and fixed merge conflicts
Rebased and fixed merge conflicts
Rebased and fixed merge conflict
…to match android app
…reference editor pane, start work on color picker editor. Rebased and dealt with merge conflicts in package-lock.json and package.json
…mple-color-picker with svelte-awesome-color-picker. Fixed merge conflicts
…tSize slider next.
(Because it isn't implemented yet)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/lib/icons/VideoIcon.svelte (1)
7-7: ⚡ Quick winUse typed
$props()here as well for Svelte 5 consistency.Line 7 still uses
export let; aligning with the typed$props()pattern used across the migrated icons will keep the layer consistent and reduce migration drift.🤖 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 `@src/lib/icons/VideoIcon.svelte` at line 7, Replace the old Svelte 3 export pattern for the color prop with the typed Svelte 5 $props() pattern: remove "export let color = 'black';" and instead use the $props() helper to declare a typed props object (e.g., const { color = 'black' } = $props<{ color?: string }>();), so the component uses the typed $props() call and preserves the default value for the color prop.
🤖 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 `@src/lib/components/VerseOnImage.svelte`:
- Around line 312-327: The export clone appended to document.body inside
VerseOnImage.svelte (the variable clone used with toPng and later
fetch/shareImage) is only removed on success; modify the flow to always remove
the clone in all exit paths by moving document.body.removeChild(clone) into a
finally-style cleanup (or ensure both the toPng.catch and the fetch.catch call
remove the clone) so the DOM node is never left behind even if toPng or the
fetch fails; keep existing error logging but guarantee cleanup around the
toPng(...).then(...) chain that calls shareImage(reference, verses, reference +
'.png', blob).
- Around line 850-853: Replace the placeholder user-facing label "Test Label For
Now" passed to the ColorPicker component: locate the ColorPicker usage (props:
toRight, label, isInput) and change the label prop to the intended descriptive
string or an i18n key (e.g., use a translation function or a constant like
t('verseOnImage.colorLabel') or COLOR_PICKER_LABEL) so the UI shows the correct
production text instead of the placeholder.
- Around line 188-204: The component attaches global pointer listeners via
window.addEventListener('pointermove', resize) and
window.addEventListener('pointerup', stopResize) (see functions resize and
stopResize) but never cleans them up on unmount; add a Svelte onDestroy handler
(import onDestroy from 'svelte') that removes those same listeners (and sets
resizing = false if needed) to ensure no handlers remain after navigation or
component tear-down; ensure the onDestroy cleanup mirrors the removals in
stopResize so listeners attached around pointerdown/pointermove are always
removed.
- Line 497: The CSS rule in VerseOnImage.svelte uses max-height:
{textboxMaxHeight}; which is invalid when textboxMaxHeight is a numeric
value—ensure the value includes a unit by appending one (e.g.,
`${textboxMaxHeight}px`) or convert textboxMaxHeight to a string with units
before use; update the usage wherever max-height is set in the component (look
for textboxMaxHeight) so the rendered CSS always contains a valid unit (px, rem,
%, etc.) or bind a style object/inline style that formats the value with a unit.
- Around line 160-161: The pinch-resize math can produce Infinity/NaN when
resizeStartDistance is zero/uninitialized; in the pinch handler where scale is
computed (using currentDistance / resizeStartDistance) add a guard that checks
resizeStartDistance is a positive number (e.g., > 0) before dividing — if not,
set scale = 1 or skip the resize update — then compute newWidth from
resizeStartSize * scale (using the guarded scale) and bail out early if
resizeStartSize is also invalid; update the logic around the symbols scale,
currentDistance, resizeStartDistance, and resizeStartSize in VerseOnImage.svelte
accordingly.
In `@src/routes/crop/`+page.svelte:
- Around line 32-37: The onMount block and the confirm handler assume the DOM
image exists; guard against voiCustomImage.original being null by checking the
image variable before accessing properties or calling methods: in the onMount
handler (where you call image?.complete and image.addEventListener(...)) only
access image if it's truthy (e.g., early-return if !image), and in the confirm
action (the function that finalizes the crop) also short-circuit if image is
undefined or not complete; ensure you still attach the load listener only when
image is present and use { once: true } as before so initCropBox is called when
available. Reference: onMount, initCropBox, image, and the confirm action to
find and update the checks.
- Around line 39-43: initCropBox currently sets cropLeft/cropTop from
getImageRect(image) but doesn’t clamp cropSize to the rendered image, causing
negative positions when the image is smaller than the default 100; update
initCropBox to compute imageRect = getImageRect(image), then clamp cropSize =
Math.min(cropSize, imageRect.width, imageRect.height) and ensure cropLeft =
Math.max(imageRect.left, Math.min(cropLeft, imageRect.right - cropSize)) and
cropTop = Math.max(imageRect.top, Math.min(cropTop, imageRect.bottom -
cropSize)) (use the existing cropSize, cropLeft, cropTop variables) so the
initial overlay always fits inside the image.
In `@vite.config.js.timestamp-1689265782083-934aea1ea46f.mjs`:
- Around line 2-5: The committed timestamped vite config contains
machine-specific file:// imports (e.g., the imports that reference
NodeGlobalsPolyfillPlugin, NodeModulesPolyfillPlugin, rollupNodePolyFill and
sveltekit) and embedded sourcemap/metadata that leak local paths; remove this
generated file from the repo (git rm), add the filename pattern (e.g.,
vite.config.js.timestamp-*.mjs) to .gitignore, and restore/commit only the
canonical vite.config.js (or regenerate it via your build tool) so the hardcoded
file:/// imports and the sourcemap metadata entry are not tracked; ensure
CI/build steps produce these artifacts locally but they are not committed.
---
Nitpick comments:
In `@src/lib/icons/VideoIcon.svelte`:
- Line 7: Replace the old Svelte 3 export pattern for the color prop with the
typed Svelte 5 $props() pattern: remove "export let color = 'black';" and
instead use the $props() helper to declare a typed props object (e.g., const {
color = 'black' } = $props<{ color?: string }>();), so the component uses the
typed $props() call and preserves the default value for the color prop.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea6239f8-efe7-4bd6-8354-026db09a5403
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
convert/convertConfig.tspackage.jsonsrc/lib/components/DownloadSelector.sveltesrc/lib/components/TextSelectionToolbar.sveltesrc/lib/components/VerseOnImage.sveltesrc/lib/data/stores/view.tssrc/lib/icons/DownloadIcon.sveltesrc/lib/icons/TextAppearanceIcon.sveltesrc/lib/icons/VideoIcon.sveltesrc/lib/icons/image/BlurIcon.sveltesrc/lib/icons/image/BrightnessIcon.sveltesrc/lib/icons/image/ContrastIcon.sveltesrc/lib/icons/image/FontChoiceIcon.sveltesrc/lib/icons/image/FormatAlignCenterIcon.sveltesrc/lib/icons/image/FormatAlignLeftIcon.sveltesrc/lib/icons/image/FormatAlignRightIcon.sveltesrc/lib/icons/image/FormatBoldIcon.sveltesrc/lib/icons/image/FormatColorFillIcon.sveltesrc/lib/icons/image/FormatItalicIcon.sveltesrc/lib/icons/image/FormatLineSpacingIcon.sveltesrc/lib/icons/image/ImageSingleIcon.sveltesrc/lib/icons/image/ImagesIcon.sveltesrc/lib/icons/image/LetterSpacingIcon.sveltesrc/lib/icons/image/OpacityIcon.sveltesrc/lib/icons/image/ReferenceIcon.sveltesrc/lib/icons/image/ResizeArrowsIcon.sveltesrc/lib/icons/image/SaturationIcon.sveltesrc/lib/icons/image/TextGlowIcon.sveltesrc/lib/icons/image/TextShadowIcon.sveltesrc/lib/icons/image/TextShadowNoneIcon.sveltesrc/lib/icons/image/TextWidthIcon.sveltesrc/lib/icons/index.tssrc/routes/+layout.sveltesrc/routes/crop/+page.sveltesrc/routes/image/+page.sveltevite.config.js.timestamp-1689265782083-934aea1ea46f.mjsvite.config.js.timestamp-1692163299587-ad359ade3a45c.mjs
💤 Files with no reviewable changes (1)
- src/routes/+layout.svelte
FyreByrd
requested changes
May 28, 2026
FyreByrd
left a comment
FyreByrd
left a comment
Collaborator
There was a problem hiding this comment.
Shouldn't be too much left to change. Just what I have here and also the CR comments.
Collaborator
There was a problem hiding this comment.
Several variables are shown as being possibly null/undefined and are showing errors in the editor.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/components/VerseOnImage.svelte (3)
677-680:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove invalid
//comment syntax in template.HTML/Svelte markup doesn't support
//comments. This line should be removed.Proposed fix
<div class="flex items-center justify-center image_selector_pane_box" - //style="height: {this.width};" >🤖 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 `@src/lib/components/VerseOnImage.svelte` around lines 677 - 680, In the Svelte template inside the VerseOnImage component remove the invalid JavaScript-style comment fragment "//style="height: {this.width};"" from the <div class="flex items-center justify-center image_selector_pane_box"> block (it isn't valid in HTML/Svelte templates); if you intended to keep a commented note, replace it with a proper HTML comment <!-- ... --> or remove it entirely and, if the inline style is needed, convert it to a valid attribute style="height: {this.width};" on that div.
93-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd null check for canvas context.
getContext('2d')can returnnull. AccessingfillStyleon null would throw a runtime error.Proposed fix
function standardize_color(str: string) { var ctx = document.createElement('canvas').getContext('2d'); + if (!ctx) return str; // Fallback to original string if context unavailable ctx.fillStyle = str; return ctx.fillStyle; }🤖 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 `@src/lib/components/VerseOnImage.svelte` around lines 93 - 97, The function standardize_color currently assumes document.createElement('canvas').getContext('2d') always returns a CanvasRenderingContext2D; add a null check for ctx in standardize_color and handle the null case (e.g., return the original str or a safe fallback) before accessing ctx.fillStyle; ensure you still set ctx.fillStyle = str when ctx is non-null and return ctx.fillStyle, and keep variable names (ctx, standardize_color) unchanged so the fix integrates cleanly.
359-383:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
downloadCanvasis missing the.finally()cleanup added toshareCanvas.The clone appended to
document.bodyis only removed on success (line 362). IftoPngor the fetch chain fails, the hidden clone remains in the DOM.Proposed fix
document.body.appendChild(clone); toPng(clone) .then(function (dataUrl) { document.body.removeChild(clone); //Get rid of the clone once we're done fetch(dataUrl) .then((response) => response.blob()) .then((blob) => { const filename = reference + '.png'; const file = new File([blob], filename, { type: 'image/png' }); const url = URL.createObjectURL(file); const anchor = document.createElement('a'); anchor.href = url; anchor.download = filename; anchor.click(); URL.revokeObjectURL(url); }) .catch((error) => { console.error('Error fetching data:', error); }); }) .catch(function (error) { console.error('oops, something went wrong!', error); + }) + .finally(() => { + if (clone.isConnected) { + document.body.removeChild(clone); + } }); }🤖 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 `@src/lib/components/VerseOnImage.svelte` around lines 359 - 383, downloadCanvas currently only removes the DOM clone on successful toPng()->fetch() resolution so a failed toPng or fetch leaves the hidden clone in the document; update downloadCanvas to always remove the clone in a .finally() after the toPng promise (or wrap the async flow in try/finally) and also ensure any created object URL (URL.createObjectURL) is revoked in a finally block so resources are cleaned up even on errors; target the clone variable, the toPng(...) call, the fetch(...) chain, and the URL.revokeObjectURL(...) usage when applying this change.
🤖 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.
Outside diff comments:
In `@src/lib/components/VerseOnImage.svelte`:
- Around line 677-680: In the Svelte template inside the VerseOnImage component
remove the invalid JavaScript-style comment fragment "//style="height:
{this.width};"" from the <div class="flex items-center justify-center
image_selector_pane_box"> block (it isn't valid in HTML/Svelte templates); if
you intended to keep a commented note, replace it with a proper HTML comment
<!-- ... --> or remove it entirely and, if the inline style is needed, convert
it to a valid attribute style="height: {this.width};" on that div.
- Around line 93-97: The function standardize_color currently assumes
document.createElement('canvas').getContext('2d') always returns a
CanvasRenderingContext2D; add a null check for ctx in standardize_color and
handle the null case (e.g., return the original str or a safe fallback) before
accessing ctx.fillStyle; ensure you still set ctx.fillStyle = str when ctx is
non-null and return ctx.fillStyle, and keep variable names (ctx,
standardize_color) unchanged so the fix integrates cleanly.
- Around line 359-383: downloadCanvas currently only removes the DOM clone on
successful toPng()->fetch() resolution so a failed toPng or fetch leaves the
hidden clone in the document; update downloadCanvas to always remove the clone
in a .finally() after the toPng promise (or wrap the async flow in try/finally)
and also ensure any created object URL (URL.createObjectURL) is revoked in a
finally block so resources are cleaned up even on errors; target the clone
variable, the toPng(...) call, the fetch(...) chain, and the
URL.revokeObjectURL(...) usage when applying this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77d32bf9-85b7-4ee6-83bd-07a0c5c0172a
📒 Files selected for processing (2)
src/lib/components/VerseOnImage.sveltesrc/routes/crop/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/crop/+page.svelte
Contributor
|
Actionable comments posted: 0 |
FyreByrd
approved these changes
May 29, 2026
FyreByrd
left a comment
FyreByrd
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me! All of my feedback has been addressed. Thanks for the good work.
Closed
This was referenced Jun 11, 2026
This was referenced Jun 26, 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
4 participants
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.
Implementing #300 and building on #399. This implements the verse on image feature. Currently, everything is implemented except for the video download option.
Summary by CodeRabbit
Release Notes