Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: improved-image-Type-detection in build-rss.js #3359

Closed
wants to merge 1 commit into from
Closed

feat: improved-image-Type-detection in build-rss.js #3359

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions scripts/build-rss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,23 @@
const fs = require('fs')
const json2xml = require('jgexml/json2xml')

const MIME_TYPES = {
'.jpg': 'image/jpeg',
'.jpeg': 'image/jpeg',
'.png': 'image/png',
'.svg': 'image/svg+xml',
'.webp': 'image/webp',
'.gif': 'image/gif',
'.bmp': 'image/bmp',
'.tiff': 'image/tiff',
'.tif': 'image/tiff'
};

function getMimeType(url) {
const ext = path.extname(url).toLowerCase();
return MIME_TYPES[ext] || 'application/octet-stream'; // Fallback MIME type

Check warning on line 18 in scripts/build-rss.js

View check run for this annotation

Codecov / codecov/patch

scripts/build-rss.js#L17-L18 

Added lines #L17 - L18 were not covered by tests
}
Comment on lines +16 to +19
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing 'path' module import.

The function uses path.extname() but the path module is not imported.

Add this import at the top of the file:

const fs = require('fs')
const json2xml = require('jgexml/json2xml')
+const path = require('path')

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 17-18: scripts/build-rss.js#L17-L18
Added lines #L17 - L18 were not covered by tests

💡 Codebase verification

Missing path module import and validation needed in getMimeType function

  • The path module is used but not imported in scripts/build-rss.js
  • The function lacks input validation for null/undefined URLs
  • The function is used only internally for RSS feed generation with controlled inputs
  • Test coverage is required as per jest.config.js settings

Recommended changes:

  1. Add const path = require('path') at the top of the file
  2. Add input validation:
function getMimeType(url) {
  if (!url || typeof url !== 'string') return 'application/octet-stream';
  const ext = path.extname(url).toLowerCase();
  return MIME_TYPES[ext] || 'application/octet-stream';
}
  1. Create test file at tests/build-rss.test.js covering:
    • Valid image URLs with different extensions
    • Invalid/empty URLs
    • URLs without extensions
    • URLs with unknown extensions
🔗 Analysis chain

Add input validation and tests for getMimeType function.

The function needs defensive programming and test coverage:

  1. URL parameter should be validated
  2. Function is not covered by tests according to codecov

Would you like me to help with:

  1. Adding input validation to handle null/undefined URLs?
  2. Creating unit tests for various scenarios (valid URLs, invalid extensions, null inputs)?

Let's verify the current usage patterns:

Let me gather more context about the usage and implementation to provide a comprehensive verification.

Let me check the test configuration and examine if there are any existing test utilities that could be helpful for testing the getMimeType function.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for getMimeType usage patterns
rg "getMimeType\(" --type js

Length of output: 189

Script:

#!/bin/bash
# Check the full context of build-rss.js
cat scripts/build-rss.js

# Look for any existing test files
fd "test" -e js

# Check if there's a test configuration file
fd "jest.config" -e js

Length of output: 3157

Script:

#!/bin/bash
# Check jest configuration
cat jest.config.js

# Check test utilities
cat tests/utils.test.js

# Check if path module is properly imported
rg "require.*path" scripts/build-rss.js

Length of output: 1365

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 17-18: scripts/build-rss.js#L17-L18
Added lines #L17 - L18 were not covered by tests


function getAllPosts() {
return require('../config/posts.json')
}
Expand Down Expand Up @@ -53,16 +70,11 @@
const link = `${base}${post.slug}${tracking}`;
const item = { title: post.title, description: clean(post.excerpt), link, category: type, guid: { '@isPermaLink': true, '': link }, pubDate: new Date(post.date).toUTCString() }
if (post.cover) {
const enclosure = {};
enclosure["@url"] = base+post.cover;
enclosure["@length"] = 15026; // dummy value, anything works
enclosure["@type"] = 'image/jpeg';
if (typeof enclosure["@url"] === 'string') {
let tmp = enclosure["@url"].toLowerCase();
if (tmp.indexOf('.png')>=0) enclosure["@type"] = 'image/png';
if (tmp.indexOf('.svg')>=0) enclosure["@type"] = 'image/svg+xml';
if (tmp.indexOf('.webp')>=0) enclosure["@type"] = 'image/webp';
}
const enclosure = {

Check warning on line 73 in scripts/build-rss.js

View check run for this annotation

Codecov / codecov/patch

scripts/build-rss.js#L73 

Added line #L73 was not covered by tests
"@url": `${base}${post.cover}`,
"@length": 15026, // Dummy value, replace with actual size if available
"@type": getMimeType(`${base}${post.cover}`) // Pass the full URL to getMimeType
};
Comment on lines +73 to +77
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve enclosure object creation.

Several improvements can be made to this section:

  1. The hard-coded length (15026) should be replaced with actual file size
  2. Base URL concatenation is duplicated

Consider this improvement:

+      const fullUrl = `${base}${post.cover}`;
       const enclosure = {
-        "@url": `${base}${post.cover}`,
+        "@url": fullUrl,
         "@length": 15026, // Dummy value, replace with actual size if available
-        "@type": getMimeType(`${base}${post.cover}`) // Pass the full URL to getMimeType
+        "@type": getMimeType(fullUrl)
       };

Would you like help implementing a function to determine the actual file size of the images? This could involve:

  1. Using fs.statSync for local files
  2. Making an HTTP HEAD request for remote files

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 73-73: scripts/build-rss.js#L73
Added line #L73 was not covered by tests

item.enclosure = enclosure;
}
rss.channel.item.push(item)
Expand Down
Loading