Fix #9: Add manually reviewed URLs feature with CSV file support - #15
Conversation
- Added manuallyReviewedPath and manuallyReviewedExpirySeconds configuration options - Support for CSV file with format: url,last_approved_timestamp - URLs in CSV are treated as valid if not expired (current time < approved_time + expiry) - Exact URL matching required (prefix matching does NOT work) - Time-limited approvals ensure periodic re-verification - Test dynamically generates CSV with current timestamp to avoid expiration issues - Added test verifying exact match requirement (prefix match correctly fails) - Comprehensive documentation explaining the feature and CSV format - All 24 tests passing This solves the problem of websites that resist automated checking (anti-scraping, rate limiting) by allowing manual verification with time-limited approvals.
There was a problem hiding this comment.
Pull request overview
This PR adds support for manually reviewed URLs via a CSV file, addressing websites that resist automated checking due to anti-scraping measures, rate limiting, or CAPTCHAs. URLs in the CSV file are approved if they match exactly and haven't expired based on their approval timestamp.
Key Changes
- Added
manuallyReviewedPathandmanuallyReviewedExpirySecondsconfiguration options to the external-links rule - Implemented CSV parsing logic with time-based expiry validation (default 365 days)
- URLs require exact matching (prefix matching does not work) using normalized URL comparison
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/rules/ExternalLinksRule.ts |
Added CSV loading logic, manual approval checking, and new configuration schema options |
README.md |
Documented the new manually reviewed URLs feature with configuration examples and CSV format specification |
tests/fixtures.test.ts |
Added dynamic CSV generation with current timestamps to prevent expiration issues in tests |
tests/fixtures/external-links-manually-reviewed.csv |
Added CSV fixture file with approved URLs (regenerated dynamically by tests) |
tests/fixtures/ExternalLinkRule-manually-reviewed.html |
Added test fixture to validate exact matching behavior and expiry logic |
tests/fixtures/ExternalLinkRule-skip-regexes.html |
Added test fixture for skipRegexes functionality |
tests/fixtures/required-reports.json |
Updated expected test outputs for new test fixtures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface ManuallyReviewedUrl { | ||
| url: string | ||
| lastApprovedTime: number | ||
| } | ||
|
|
There was a problem hiding this comment.
The ManuallyReviewedUrl interface is defined but never used in the codebase. The loadManuallyReviewedUrls() method returns Map<string, number> directly instead of using this interface. Consider removing this interface or using it to type the map entries if it provides value for documentation purposes.
| interface ManuallyReviewedUrl { | |
| url: string | |
| lastApprovedTime: number | |
| } |
| if (!line) continue | ||
|
|
||
| // Simple CSV parsing: split by comma and handle quoted fields | ||
| const parts = line.split(',') |
There was a problem hiding this comment.
The CSV parsing logic uses a simple split(',') which will fail if URLs contain commas (e.g., in query parameters). While the comment mentions handling quoted fields, the current implementation only removes quotes from the first and last positions and doesn't properly handle CSV quoting rules. If a URL contains a comma, it should be quoted in the CSV, but this parser will still split on that comma.
Consider using a proper CSV parsing library or implementing RFC 4180-compliant parsing that handles commas inside quoted fields. For example, "https://example.com?a=1,2",1234567890 would be incorrectly parsed as three parts instead of two.
| if (!url || !timestampStr) continue | ||
|
|
||
| const timestamp = parseInt(timestampStr, 10) | ||
| if (isNaN(timestamp)) continue |
There was a problem hiding this comment.
The timestamp parsing doesn't validate that the timestamp is positive or within a reasonable range. Negative timestamps (e.g., -1) or timestamps far in the future (e.g., 9999999999999) would be accepted and could cause unexpected behavior in the expiry calculation at line 216.
Consider adding validation:
const timestamp = parseInt(timestampStr, 10)
if (isNaN(timestamp) || timestamp < 0 || timestamp > Date.now() / 1000 + 365 * 24 * 60 * 60) continue| if (isNaN(timestamp)) continue | |
| const nowSec = Math.floor(Date.now() / 1000) | |
| const maxAllowed = nowSec + 365 * 24 * 60 * 60 // 1 year in the future | |
| if (isNaN(timestamp) || timestamp < 0 || timestamp > maxAllowed) continue |
Closes #9
Overview
Adds support for manually reviewed URLs via CSV file, solving the problem of websites that resist automated checking (anti-scraping, rate limiting, CAPTCHAs).
Changes
Core Feature
Implementation Details
Testing
Documentation