fix: normalize feed URL protocols #34
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Summary by CodeRabbit
WalkthroughAdded --skipLibCheck to TypeScript compilation in pretest script. Updated normalizeFeedUrl to remove protocols case-insensitively. Introduced a new test verifying protocol removal regardless of case. No public APIs changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
utils/url-helpers.ts (1)
7-12: Consider supporting any scheme and protocol-relative URLsIf inputs may contain feed://, ftp://, or protocol-relative //example.com, you can generalize the scheme strip and handle // at start. Safe and backward compatible with current behavior.
Apply this diff:
- // Remove protocol (case-insensitive) - .replace(/^https?:\/\//i, '') + // Remove any scheme (case-insensitive), e.g., http(s)://, feed://, ftp:// + .replace(/^[a-z][a-z0-9+.-]*:\/\//i, '') + // Handle protocol-relative URLs (e.g., //example.com) + .replace(/^\/\//, '')Optionally, add tests covering:
- 'HTTPS://Example.com' -> 'Example.com'
- 'feed://example.com' -> 'example.com'
- '//example.com/path' -> 'example.com/path'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
scripts/pretest.js(1 hunks)tests/urlHelpers.test.js(1 hunks)utils/url-helpers.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/urlHelpers.test.js (1)
utils/url-helpers.ts (1)
normalizeFeedUrl(1-17)
🔇 Additional comments (3)
scripts/pretest.js (1)
8-8: Skip lib checks: good call to reduce test compile frictionAdding --skipLibCheck makes pretest faster and avoids noisy third-party type errors during test compilation.
utils/url-helpers.ts (1)
7-8: Case-insensitive protocol removal is correctThe i flag is the right fix to strip HTTP(S) regardless of case. The behavior matches the new test.
tests/urlHelpers.test.js (1)
9-11: Nice regression test for case-insensitive protocol strippingThis directly validates the updated regex behavior and guards against future regressions.
| 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' }); |
There was a problem hiding this comment.
💡 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 || trueLength 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": truefrom 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.jsonwithoutskipLibCheck.
📝 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.
| 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.
| test('removes protocol regardless of case', () => { | ||
| assert.strictEqual(normalizeFeedUrl('HTTP://Example.com'), 'Example.com'); | ||
| }); |
There was a problem hiding this comment.
🧹 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.
There was a problem hiding this comment.
Pull Request Overview
This PR adds case-insensitive protocol handling to the normalizeFeedUrl function to properly handle feed URLs with mixed-case HTTP protocols like "HTTP://" or "Https://".
- Updated the protocol removal regex to be case-insensitive by adding the
iflag - Added a test case to verify mixed-case protocol handling works correctly
- Added
--skipLibCheckflag to TypeScript compilation to ignore library type errors during test compilation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| utils/url-helpers.ts | Modified regex to remove HTTP/HTTPS protocols case-insensitively |
| tests/urlHelpers.test.js | Added test case for case-insensitive protocol removal |
| scripts/pretest.js | Added skipLibCheck flag to TypeScript compilation options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary
Testing
npm testhttps://chatgpt.com/codex/tasks/task_e_689c091a04b883279516a22bb4ca6403