Skip to content

Assert unrecognized Cloudinary URL variant throws in transformCloudinaryImage#25

Merged
ooloth merged 1 commit into
mainfrom
claude/issue-22
May 15, 2026
Merged

Assert unrecognized Cloudinary URL variant throws in transformCloudinaryImage#25
ooloth merged 1 commit into
mainfrom
claude/issue-22

Conversation

@ooloth
Copy link
Copy Markdown
Owner

@ooloth ooloth commented May 15, 2026

Summary

  • After the three known path-segment branches (upload/, fetch/, youtube/), adds a throw new Error(...) when a Cloudinary URL matches none of the recognized variants.
  • Updates the existing test case that previously expected the URL to be returned unchanged to instead expect the new error to be thrown.

Test plan

  • transformCloudinaryImage throws for a Cloudinary URL with an unrecognized path segment
  • All 51 test files continue to pass

Closes #22

https://claude.ai/code/session_01LWPHUTNZkL6Ww9RungK9Hg


Generated by Claude Code

Adds a throw statement after the three known path-segment branches so
that a Cloudinary URL that matches none of upload/, fetch/, or youtube/
fails loudly instead of silently returning the untransformed URL.
Updates the existing test case to expect the new error.

Closes #22

https://claude.ai/code/session_01LWPHUTNZkL6Ww9RungK9Hg
@ooloth ooloth marked this pull request as ready for review May 15, 2026 11:16
Copilot AI review requested due to automatic review settings May 15, 2026 11:16
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an explicit error throw in transformCloudinaryImage when a Cloudinary URL does not match any recognized path segment (upload/, fetch/, youtube/), replacing the previous silent fallthrough. The corresponding test is updated to assert the new throw behavior.

Changes:

  • Throw a descriptive Error after the three known path-segment branches.
  • Update the matching unit test to expect the thrown error instead of an unchanged URL.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
io/cloudinary/transformCloudinaryImage.ts Adds throw new Error(...) for unrecognized Cloudinary URL variants.
io/cloudinary/transformCloudinaryImage.test.ts Updates test to assert the new error is thrown.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

Code Review

Overview

This PR converts a silent no-op (returning the URL unchanged) into a fail-fast error when transformCloudinaryImage encounters a Cloudinary URL with an unrecognized path segment. It's a small, focused improvement: rather than silently serving un-optimized images, the function now surfaces the misconfiguration immediately.


What works well

  • Fail-fast is the right call. Returning the URL unchanged was a quiet bug — the image would be served without optimization and nothing would indicate the problem. An explicit throw surfaces this immediately during development or at build time.
  • Error message is actionable. Including the full URL in transformCloudinaryImage: unrecognized Cloudinary URL variant: <url> makes it easy to diagnose.
  • Callers are safe. Both existing callers (fetchTmdbList.ts:113 and fetchItunesItems.ts:123) construct fetch/ URLs, so neither will hit the new throw path.
  • Test is accurate. The updated test correctly asserts the throw with the exact error message.

Minor observations (non-blocking)

1. Hostname check is loose

if (url.includes('cloudinary')) {

This matches any URL with the word "cloudinary" anywhere in it (path, query, etc.), not just res.cloudinary.com. A URL like https://example.com/cloudinary-backup/image.jpg would enter the branch and then hit the new throw. Replacing with a parsed hostname check would be strictly safer:

if (new URL(url).hostname === 'res.cloudinary.com') {

This is a pre-existing issue, not introduced here, and unlikely to cause real-world problems given the URL patterns in use — just worth flagging.

2. Path-segment checks have the same looseness

if (url.includes('upload/')) {
if (url.includes('fetch/')) {
if (url.includes('youtube/')) {

A fetch/ URL whose destination path contains upload/ (e.g., fetch/https://example.com/upload/image.jpg) would match the upload/ branch instead. Again, pre-existing and unlikely in practice, but the same hostname-parsed + pathname approach would eliminate the ambiguity.


Summary

LGTM. The change is correct, minimal, and clearly improves the function's behaviour. The two pre-existing loose checks are worth addressing eventually (perhaps a follow-up issue), but they don't block this PR.

@ooloth ooloth merged commit d1fb6bb into main May 15, 2026
14 of 15 checks passed
@ooloth ooloth deleted the claude/issue-22 branch May 15, 2026 23:20
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.

Assert that a Cloudinary URL contains a recognized path segment in transformCloudinaryImage

3 participants