Fix images being skipped during fetch #102
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
🚀 Preview DeploymentYour documentation preview is ready! Preview URL: https://pr-102.comapeo-docs.pages.dev 🔄 Content: Regenerated 5 pages from Notion (script changes detected)
This preview will update automatically when you push new commits to this PR. Built with commit 3d4ab4e |
Created detailed specification document analyzing the root cause of image download failures due to Notion's 1-hour URL expiration policy. Key findings: - URLs are generated during pageToMarkdown() but downloaded much later - Processing delays can exceed 1-hour expiration window - Affects large page batches and image-heavy pages Proposed solution: - Phase 1: Download images immediately after URL generation - Phase 2: Add URL refresh on expiry detection - Phase 3: Add monitoring and metrics Includes testing strategy, rollout plan, and risk assessment. Related to #94
Revised the specification to reflect a simpler solution: - Phase 1: Reorder operations in processSinglePage() to process images immediately after markdown conversion, before emoji/callout processing - No new functions needed, just moving existing code - Reduces time gap from 3-7 seconds to < 1 second per page Updated Phase 2 to focus on detection/logging rather than complex URL refresh logic, which can be added later if needed. Changes: - Clarified exact code locations and line numbers - Added specific code change examples - Simplified implementation complexity - Reduced risk assessment Related to #94
Implemented two-phase fix for Issue #94: **Phase 1: Reorder image processing (CRITICAL FIX)** - Moved image download to immediately after markdown conversion - Images now process before emoji and callout processing - Reduces time gap from 3-7 seconds to <1 second per page - In generateBlocks.ts:processSinglePage(), moved processAndReplaceImages() from line 320 to line 287 (right after toMarkdownString()) **Phase 2: Expired URL detection and logging** - Added isExpiredUrlError() helper to detect 403 expired signatures - Enhanced error logging to identify expired URLs specifically - Detects AWS S3 "SignatureDoesNotMatch" and other expiration errors - Provides clear diagnostic message when URLs expire **Testing** - Added comprehensive test suite: imageUrlExpiration.test.ts - Added expired URL detection tests: expiredUrlDetection.test.ts - All existing tests pass (downloadImage.test.ts: 9/9) - Expired URL detection tests pass (17/17) **Impact** - Notion image URLs expire after 1 hour - Previous flow: fetch → emoji → callouts → images (3-7s delay) - New flow: fetch → images → emoji → callouts (<1s delay) - With 50 pages at 5 concurrent, saves ~30-70 seconds of cumulative delay - Prevents URLs from expiring during batch processing Fixes #94 See IMAGE_URL_EXPIRATION_SPEC.md for full technical specification
Implemented all suggested improvements from code review: **1. Test Robustness (Timing Assertions)** - Added comments to timing-based test assertions noting potential CI flakiness - Documented generous timeouts (30s, 10min) that account for CI overhead - Tests: imageUrlExpiration.test.ts (lines 302-309, 424-428, 467-471) **2. Documentation: Phase 3 Status** - Clearly marked Phase 3 as "OPTIONAL/FUTURE WORK" in spec - Added status section: "NOT IMPLEMENTED - Future enhancement" - Documented when Phase 3 should be implemented (only if Phase 1/2 insufficient) - Made it clear Phases 1 & 2 solve Issue #94 - Spec: IMAGE_URL_EXPIRATION_SPEC.md (lines 334-355) **3. Type Safety Improvement** - Changed isExpiredUrlError parameter from `any` to `unknown` - Added ErrorWithResponse interface for proper type checking - Added type guard: checks error is object before casting - Better type safety while maintaining flexibility - File: imageProcessing.ts (lines 29-46) **4. Edge Case Test: Callouts with Images** - Added test for callouts containing images after reordering - Verifies both image download and callout transformation work correctly - Ensures images are processed before callouts (order dependency) - Test: imageUrlExpiration.test.ts (lines 530-556) **Test Results:** ✅ All 17 expired URL detection tests pass ✅ All 9 download image tests pass ✅ New edge case test passes ✅ No new type errors introduced **Risk Assessment:** - Low risk: improvements don't change logic, only add clarity/safety - All existing tests continue to pass - Type safety improvement prevents potential runtime errors - Documentation improvements prevent future confusion Related to code review feedback on PR for Issue #94
Fixed a test bug where urlGenerationTime was initialized to 0, causing the timing assertion to compare against 0 instead of the actual URL generation timestamp. This resulted in comparing the full timestamp (~1.7 billion milliseconds) against the 30-second threshold. **Root Cause:** - urlGenerationTime = 0 - downloadTime = Date.now() (e.g., 1764260475398) - timeSinceGeneration = 1764260475398 - 0 = 1764260475398 (HUGE!) - expect(1764260475398).toBeLessThan(30000) → FAIL **Fix:** Initialize urlGenerationTime to Date.now() at test start to provide a valid baseline. The mock still updates it to the exact time when pageToMarkdown is called, but now we have a reasonable fallback. **Test Results:** ✅ 36/36 tests pass (imageUrlExpiration, expiredUrlDetection, downloadImage) ✅ All timing assertions now work correctly ✅ No regressions in other tests Related to Issue #94 - Images being skipped during fetch
Improves code quality by defining expiration indicators array as a module-level constant instead of recreating it on every function call. Changes: - Define EXPIRATION_INDICATORS constant before isExpiredUrlError() - Add JSDoc documentation explaining the constant's purpose - Use 'as const' for better type safety Impact: Negligible performance improvement but more idiomatic code.
Adds a safety net to catch and fix any AWS S3 URLs that persist in the final markdown (e.g. due to callout processing or regex misses), ensuring all expiring URLs are replaced with local images.
Updates incremental sync logic to check existing output files for expiring S3 URLs, forcing regeneration if found. Also exposes hasS3Urls helper for consistent detection logic.
Increases page processing timeout to 5 minutes and overall batch timeout to 10 minutes to prevent failures on pages with many large images. Also bumps individual image download timeout to 5 minutes.
CRITICAL BUG FIXES: - Add progress validation to detect when retry attempts return identical content - Add safety check for null/undefined content before processing - Fix missing currentSource update between retry attempts (the core bug) - Add skipIf guards to real-content-regex-bug tests to prevent CI failures The original retry loop would get stuck when: 1. Image processing returned the same content repeatedly 2. currentSource wasn't updated, causing same expired URLs to be processed again Progress validation now aborts retries when content is identical, preventing wasted processing cycles and infinite loops. Safety checks ensure content is valid before processing begins. Test file dependencies fixed by conditionally skipping tests when the runtime-generated markdown file doesn't exist, preventing CI failures.
TEST IMPROVEMENTS: - Add retry-loop-behavior.test.ts to verify retry logic handles progress correctly - Update test to use realistic mock data showing progressive improvement - Extract buildFetchOneSelection() utility to improve page selection logic - Add comprehensive tests for ancestor/descendant/translation selection The retry test validates that: - Multiple retry attempts are made when progress is detected - Progress validation prevents infinite loops with identical content - Markdown is not re-fetched between retry attempts (efficiency) The buildFetchOneSelection utility improves notion-fetch-one command by selecting ancestors, descendants, translations, and contextual pages based on page relationships and order properties.
CRITICAL WORKAROUND for Bun regex bug on large files (700KB+): - Add manual string parsing fallback when regex.exec() fails on large content - Force String() conversion to avoid proxy/wrapper issues - Add DEBUG_S3_IMAGES environment variable for detailed diagnostics BUG CONTEXT: Bun's regex.exec() returns null on large markdown files (700KB+) even when image markers are clearly present. This causes the retry loop to fail silently because extractImageMatches() returns 0 results, preventing S3 URL detection. DIAGNOSTIC IMPROVEMENTS: - Add getImageDiagnostics() helper to count S3 URLs in content - Add debugS3() logging for detailed image processing flow - Save problematic content to /tmp for debugging when DEBUG_S3_IMAGES=true - Add isExpiringS3Url() helper to centralize S3 URL detection - Refactor hasS3Urls() to use getImageDiagnostics() WORKAROUND STRATEGY: 1. Try regex.exec() first (fast path for normal files) 2. If returns 0 matches AND file >700KB AND contains '
luandro
force-pushed
the
claude/fix-image-fetch-skip-01BnWyLMSiUT6k9cgBGSaHvB
branch
from
b07bf6c to
2d5593f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- Fix indentation from 4 spaces to 2 spaces per .prettierrc.json - ES: Complete truncated text "Preparación para el uso de CoMapeo" - ES: Fix language mix-up "Exchanging Project Data" (was Portuguese) - PT: Remove trailing newlines from "Customizing CoMapeo" and "Troubleshooting" - PT: Fix language mix-up "Exchanging Project Data" (was Spanish) - PT: Replace placeholder with proper translation "Resolução de Problemas"
- Add runtime detection for Bun vs Node environments to prevent false failures - Replace file-based tests with synthetic content for portability - Add afterAll cleanup hooks to prevent test pollution - Add test for HTML 403 body handling in expired URL detection - Add test for explicit error surfacing in processMarkdownWithRetry - Add test for MAX_IMAGE_RETRIES env override - Add test for max retries with no progress scenario - Add tests for image processing error handling and progress tracking - Improve Bun regex bug tests with conditional expectations All tests passing (132 tests across 14 files)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Created detailed specification document analyzing the root cause of image download failures due to Notion's 1-hour URL expiration policy.
Key findings:
Proposed solution:
Includes testing strategy, rollout plan, and risk assessment.
Related to #94