Skip to content

fix: normalize feed URL protocols #34

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −4
Draft

fix: normalize feed URL protocols #34

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion scripts/pretest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const modules = [
];
try {
execSync(`rm -rf compiled-tests && mkdir compiled-tests`);
execSync(`npx tsc ${modules.join(' ')} --module commonjs --target es2017 --outDir compiled-tests`, { stdio: 'inherit' });
execSync(`npx tsc ${modules.join(' ')} --module commonjs --target es2017 --skipLibCheck --outDir compiled-tests`, { stdio: 'inherit' });
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Confirm this flag is not masking issues elsewhere

If any downstream task relies on stricter type-checking (e.g., CI builds), ensure skipLibCheck is limited to tests only and not bleeding into production builds.

Run this script to see where skipLibCheck is used across the repo:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching for 'skipLibCheck' usage across the repository..."
rg -n --no-heading "skipLibCheck" -A2 -B2 || true

Length of output: 781

Limit skipLibCheck to test compilation only

The search shows skipLibCheck is enabled globally in your root tsconfig.json (line 9), in addition to being passed via CLI in scripts/pretest.js (line 8). This means production builds may also be skipping lib checks and masking type errors.

Recommended changes:

  • Remove "skipLibCheck": true from tsconfig.json.
  • Create a dedicated test config (e.g. tsconfig.test.json) that sets "skipLibCheck": true.
  • Update scripts/pretest.js to compile tests with that project file: 
    - execSync(`npx tsc ${modules.join(' ')} --module commonjs --target es2017 --skipLibCheck --outDir compiled-tests`, { stdio: 'inherit' });
+ execSync(`npx tsc --project tsconfig.test.json ${modules.join(' ')} --module commonjs --target es2017 --outDir compiled-tests`, { stdio: 'inherit' });
  • Ensure your CI and production build steps use the root tsconfig.json without skipLibCheck.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
execSync(`npx tsc ${modules.join(' ')} --module commonjs --target es2017 --skipLibCheck --outDir compiled-tests`, { stdio: 'inherit' });
execSync(
`npx tsc --project tsconfig.test.json ${modules.join(' ')} --module commonjs --target es2017 --outDir compiled-tests`,
{ stdio: 'inherit' }
);
🤖 Prompt for AI Agents 
In scripts/pretest.js around line 8, currently passing --skipLibCheck on the CLI
(and you also have "skipLibCheck": true in the root tsconfig.json), limit
skipLibCheck to test compilation by removing "skipLibCheck" from the root
tsconfig.json, adding a tsconfig.test.json that extends the root and sets
"skipLibCheck": true, and update scripts/pretest.js to compile tests using that
project file (e.g. use -p tsconfig.test.json or --project tsconfig.test.json) so
CI and production builds continue to use the root tsconfig.json without
skipLibCheck.

} catch (e) {
console.error('Failed to compile test modules', e);
process.exit(1);
Expand Down
4 changes: 4 additions & 0 deletions tests/urlHelpers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ test('removes protocol and trailing slash', () => {
assert.strictEqual(normalizeFeedUrl('https://example.com/'), 'example.com');
});

test('removes protocol regardless of case', () => {
assert.strictEqual(normalizeFeedUrl('HTTP://Example.com'), 'Example.com');
});
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Broaden test coverage for edge protocols and formats

Add a couple more cases to lock in expected behavior for other common inputs.

Consider adding:

test('removes mixed-case HTTPS', () => {
  assert.strictEqual(normalizeFeedUrl('hTtPs://Example.com/'), 'Example.com');
});

test('decodes and removes encoded protocol', () => {
  assert.strictEqual(normalizeFeedUrl('HTTP%3A%2F%2FExample.com'), 'Example.com');
});

If you adopt the generalized scheme stripping, also add:

test('removes protocol-relative URLs', () => {
  assert.strictEqual(normalizeFeedUrl('//example.com/a//b/'), 'example.com/a/b');
});
🤖 Prompt for AI Agents 
In tests/urlHelpers.test.js around lines 9 to 11, the current test only covers
an uppercase HTTP scheme; add additional unit tests to broaden coverage: add a
test for mixed-case HTTPS expecting 'Example.com' (input
'hTtPs://Example.com/'), a test for encoded protocol expecting 'Example.com'
(input 'HTTP%3A%2F%2FExample.com'), and if normalizeFeedUrl implements
generalized scheme stripping also add a protocol-relative URL test expecting
'example.com/a/b' (input '//example.com/a//b/'); include these assertions using
the same test/assert style as existing tests.


test('normalizes multiple slashes', () => {
assert.strictEqual(normalizeFeedUrl('https://example.com//a//b'), 'example.com/a/b');
});
Expand Down
6 changes: 3 additions & 3 deletions utils/url-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ export const normalizeFeedUrl = (url: string | null): string => {
// First decode to handle any encoded URLs
const decoded = decodeURIComponent(url);
return decoded
// Remove protocol
.replace(/^https?:\/\//, '')
// Remove protocol (case-insensitive)
.replace(/^https?:\/\//i, '')
// Remove trailing slashes
.replace(/\/+$/, '')
// Normalize multiple slashes
Expand All @@ -14,4 +14,4 @@ export const normalizeFeedUrl = (url: string | null): string => {
console.warn('Error normalizing URL:', error);
return url.replace(/\/+$/, '');
}
};
};