docs: update image stabilization spec with implementation clarifications#141
Conversation
Address all issues identified in the spec review by verifying against actual codebase: - Fix function name to processSinglePageTranslation (not processPageTranslation) - Fix slug helper to include page ID suffix for deterministic filenames - Replace validateAndFixRemainingImages (warns only) with getImageDiagnostics + throw - Add totalFailures > 0 check to fail on broken placeholder images - Defer per-language image overrides to future enhancement (EN-only initially) - Downgrade prompt enhancement to P2 (already 3 existing instructions) - Fix import list (remove unused validateAndFixRemainingImages) - Remove incorrect metrics.skippedSmallSize reference in logging - Add Prerequisites section for EN fetch recommendation - Document frontmatter images as out of scope - Update review document with resolution status https://claude.ai/code/session_01RMuRBsneMpCL1cBVLuVcmx
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| | 3.3 | Test `/images/` path preservation | Same | | ||
| | 3.4 | Test HTML image tag handling | Same | | ||
| | 3.5 | Test idempotency | Same | | ||
| | 3.6 | Test per-language image overrides | Same | |
There was a problem hiding this comment.
Phase 3 testing table still lists override tests as P0
Task 3.6 "Test per-language image overrides" is listed in the Phase 3 (P0) testing table. Since per-language overrides are explicitly deferred to a future enhancement, this task should either be removed from P0 or re-labeled (e.g., "Test per-language image overrides (future — skip for initial implementation)").
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 555:555
Comment:
**Phase 3 testing table still lists override tests as P0**
Task 3.6 "Test per-language image overrides" is listed in the Phase 3 (P0) testing table. Since per-language overrides are explicitly deferred to a future enhancement, this task should either be removed from P0 or re-labeled (e.g., "Test per-language image overrides (future — skip for initial implementation)").
How can I resolve this? If you propose a fix, please make it concise.| // Fail page if any images failed to download (no broken placeholders in translations) | ||
| if (imageResult.stats.totalFailures > 0) { | ||
| throw new Error( | ||
| `Image stabilization failed for "${title}": ${imageResult.stats.totalFailures} image(s) failed to download. ` + | ||
| `Cannot proceed with translation — images would be broken.` | ||
| ); | ||
| } | ||
|
|
||
| const markdownContent = imageResult.markdown; | ||
|
|
||
| // Single diagnostic line per page | ||
| console.log( | ||
| chalk.blue( | ||
| ` Images: processed=${imageResult.stats.successfulImages} reused=${imageResult.metrics.skippedSmallSize} failed=${imageResult.stats.totalFailures}` | ||
| ` Images: processed=${imageResult.stats.successfulImages} failed=${imageResult.stats.totalFailures}` | ||
| ) | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
Diagnostic logging placed after potential throw
The diagnostic console.log for image stats (lines 280-284) is placed after the imageResult.stats.totalFailures > 0 check that throws. This means that when image processing succeeds (the common path), the log appears. However, when it fails, the error throw at line 271 means no diagnostic line is logged for the failure case — only the error message is thrown. Consider moving the diagnostic log before the failure check so operators always get a status line regardless of outcome:
| // Fail page if any images failed to download (no broken placeholders in translations) | |
| if (imageResult.stats.totalFailures > 0) { | |
| throw new Error( | |
| `Image stabilization failed for "${title}": ${imageResult.stats.totalFailures} image(s) failed to download. ` + | |
| `Cannot proceed with translation — images would be broken.` | |
| ); | |
| } | |
| const markdownContent = imageResult.markdown; | |
| // Single diagnostic line per page | |
| console.log( | |
| chalk.blue( | |
| ` Images: processed=${imageResult.stats.successfulImages} reused=${imageResult.metrics.skippedSmallSize} failed=${imageResult.stats.totalFailures}` | |
| ` Images: processed=${imageResult.stats.successfulImages} failed=${imageResult.stats.totalFailures}` | |
| ) | |
| ); | |
| ``` | |
| // Single diagnostic line per page (logged before failure check for visibility) | |
| console.log( | |
| chalk.blue( | |
| ` Images: processed=${imageResult.stats.successfulImages} failed=${imageResult.stats.totalFailures}` | |
| ) | |
| ); | |
| // Fail page if any images failed to download (no broken placeholders in translations) | |
| if (imageResult.stats.totalFailures > 0) { | |
| throw new Error( | |
| `Image stabilization failed for "${title}": ${imageResult.stats.totalFailures} image(s) failed to download. ` + | |
| `Cannot proceed with translation — images would be broken.` | |
| ); | |
| } | |
| const markdownContent = imageResult.markdown; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 269:285
Comment:
**Diagnostic logging placed after potential throw**
The diagnostic `console.log` for image stats (lines 280-284) is placed after the `imageResult.stats.totalFailures > 0` check that throws. This means that when image processing succeeds (the common path), the log appears. However, when it fails, the error throw at line 271 means no diagnostic line is logged for the failure case — only the error message is thrown. Consider moving the diagnostic log before the failure check so operators always get a status line regardless of outcome:
```suggestion
// Single diagnostic line per page (logged before failure check for visibility)
console.log(
chalk.blue(
` Images: processed=${imageResult.stats.successfulImages} failed=${imageResult.stats.totalFailures}`
)
);
// Fail page if any images failed to download (no broken placeholders in translations)
if (imageResult.stats.totalFailures > 0) {
throw new Error(
`Image stabilization failed for "${title}": ${imageResult.stats.totalFailures} image(s) failed to download. ` +
`Cannot proceed with translation — images would be broken.`
);
}
const markdownContent = imageResult.markdown;
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
Test case 5 "Per-Language Image Overrides" describes testing localized image overrides with active assertions (e.g., "EN markdown references image A, localized markdown references image B → Result: uses image B rewritten to This test case should either be:
Currently, an implementer following the spec would write tests for functionality that isn't supposed to exist yet in Phase 1. Prompt To Fix With AIThis is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 392:397
Comment:
**Test case conflicts with deferred scope**
Test case 5 "Per-Language Image Overrides" describes testing localized image overrides with active assertions (e.g., "EN markdown references image A, localized markdown references image B → Result: uses image B rewritten to `/images/...`"). However, the spec repeatedly and correctly states that per-language overrides are "out of scope for the initial implementation" and deferred to a "future enhancement."
This test case should either be:
- Removed from the P0 testing phase, or
- Explicitly marked as a test for the future enhancement only (e.g., wrapped in a `describe.skip` or noted as "Future: test only when per-language overrides are implemented")
Currently, an implementer following the spec would write tests for functionality that isn't supposed to exist yet in Phase 1.
How can I resolve this? If you propose a fix, please make it concise.
The post-translation validation code here uses const diagnostics = getImageDiagnostics(translatedContent);But the simplified spec ( const diagnostics = getImageDiagnostics(translated.markdown);These should be consistent to avoid confusion for implementers. Since Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 300:300
Comment:
**Variable name mismatch across spec documents**
The post-translation validation code here uses `translatedContent` as the variable name:
```typescript
const diagnostics = getImageDiagnostics(translatedContent);
```
But the simplified spec (`TRANSLATION_IMAGE_STABILIZATION_SPEC_SIMPLIFIED.md:238`) uses `translated.markdown`:
```typescript
const diagnostics = getImageDiagnostics(translated.markdown);
```
These should be consistent to avoid confusion for implementers. Since `translateText()` returns an object with a `.markdown` property, the simplified spec's `translated.markdown` is more accurate for direct usage. The main spec should either use the same pattern or clearly show the intermediate variable assignment.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
Machine-readable implementation plan with atomic tasks, exact code locations, dependency graph, and complete test specifications. Ready for autonomous execution. https://claude.ai/code/session_01RMuRBsneMpCL1cBVLuVcmx
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 392:395
Comment:
Add note indicating this is optional/future work. The spec states overrides are "out of scope for the initial implementation" (line 86) and a "Future Enhancement" (section at line 113), but testing requirement #5 doesn't indicate this is optional. Consider adding "*(Future enhancement — skip for initial implementation)*" to align with the rest of the spec.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
- Fix __tests__/ path to flat path in simplified spec - Fix title -> originalTitle variable name consistency - Add note about invoking processSinglePageTranslation in tests 🤖 Generated with [Qoder][https://qoder.com]
| | 3.3 | Test `/images/` path preservation | Same | | ||
| | 3.4 | Test HTML image tag handling | Same | | ||
| | 3.5 | Test idempotency | Same | | ||
| | 3.6 | Test per-language image overrides | Same | |
There was a problem hiding this comment.
Task 3.6 label inconsistent with PLAN document.
In TRANSLATION_IMAGE_STABILIZATION_PLAN.md, TASK-3.6 is "Test title page bypass", but here it's listed as "Test per-language image overrides". Since per-language overrides are explicitly deferred to future enhancement (not P0), this should match the PLAN:
| | 3.6 | Test per-language image overrides | Same | | |
| | 3.6 | Test title page bypass | Same | |
Note: The per-language override tests can be removed from P0 scope entirely or added as a separate future task.
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 555:555
Comment:
Task 3.6 label inconsistent with PLAN document.
In `TRANSLATION_IMAGE_STABILIZATION_PLAN.md`, TASK-3.6 is "Test title page bypass", but here it's listed as "Test per-language image overrides". Since per-language overrides are explicitly deferred to future enhancement (not P0), this should match the PLAN:
```suggestion
| 3.6 | Test title page bypass | Same |
```
Note: The per-language override tests can be removed from P0 scope entirely or added as a separate future task.
How can I resolve this? If you propose a fix, please make it concise.| console.log( | ||
| chalk.blue( | ||
| ` Images: processed=${imageResult.stats.successfulImages} reused=${imageResult.metrics.skippedSmallSize} failed=${imageResult.stats.totalFailures}` | ||
| ` Images: processed=${imageResult.stats.successfulImages} failed=${imageResult.stats.totalFailures}` | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Inconsistent diagnostic logging between SPEC and PLAN.
SPEC shows unconditional logging (logs on success, skipped on failure due to throw), but TRANSLATION_IMAGE_STABILIZATION_PLAN.md:178-184 conditionally logs only when successfulImages > 0:
if (imageResult.stats.successfulImages > 0) {
console.log(
chalk.blue(
` Images: processed=${imageResult.stats.successfulImages} failed=${imageResult.stats.totalFailures}`
)
);
}Consider aligning both documents. The conditional approach in PLAN avoids logging "processed=0 failed=0" for pages without images.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 280:284
Comment:
Inconsistent diagnostic logging between SPEC and PLAN.
SPEC shows unconditional logging (logs on success, skipped on failure due to throw), but `TRANSLATION_IMAGE_STABILIZATION_PLAN.md:178-184` conditionally logs only when `successfulImages > 0`:
```typescript
if (imageResult.stats.successfulImages > 0) {
console.log(
chalk.blue(
` Images: processed=${imageResult.stats.successfulImages} failed=${imageResult.stats.totalFailures}`
)
);
}
```
Consider aligning both documents. The conditional approach in PLAN avoids logging "processed=0 failed=0" for pages without images.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.- Make diagnostic logging conditional (only log when successfulImages > 0) - Fix TASK-3.6 label to match PLAN (Test title page bypass)
| | 3.7 | Update existing translation tests if needed | `scripts/notion-translate/index.test.ts` | | ||
| | Task | Description | File(s) | | ||
| | ---- | ------------------------------------------- | --------------------------------------------------------------- | | ||
| | 3.1 | Create image stabilization test file | `scripts/notion-translate/__tests__/imageStabilization.test.ts` | |
There was a problem hiding this comment.
Test file path inconsistency with PLAN
The Phase 3 test table lists the test file as scripts/notion-translate/__tests__/imageStabilization.test.ts (with __tests__/ subfolder), but the PLAN document explicitly states the file should be at scripts/notion-translate/imageStabilization.test.ts (flat directory) to follow the existing project convention. The SIMPLIFIED spec also uses the flat path.
This inconsistency could confuse the implementer about where to create the test file.
| | 3.1 | Create image stabilization test file | `scripts/notion-translate/__tests__/imageStabilization.test.ts` | | |
| | 3.1 | Create image stabilization test file | `scripts/notion-translate/imageStabilization.test.ts` | |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 552:552
Comment:
**Test file path inconsistency with PLAN**
The Phase 3 test table lists the test file as `scripts/notion-translate/__tests__/imageStabilization.test.ts` (with `__tests__/` subfolder), but the PLAN document explicitly states the file should be at `scripts/notion-translate/imageStabilization.test.ts` (flat directory) to follow the existing project convention. The SIMPLIFIED spec also uses the flat path.
This inconsistency could confuse the implementer about where to create the test file.
```suggestion
| 3.1 | Create image stabilization test file | `scripts/notion-translate/imageStabilization.test.ts` |
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| **Location:** `scripts/notion-translate/translateFrontMatter.ts` | ||
|
|
||
| **Add to constraints section:** | ||
| The prompt already contains two instructions about preserving image URLs: |
There was a problem hiding this comment.
"Two" instructions but three are listed
The text says "The prompt already contains two instructions about preserving image URLs:" but then lists three items (lines 317, 318, 319). Should say "three instructions" to match the list.
| The prompt already contains two instructions about preserving image URLs: | |
| The prompt already contains three instructions about preserving image URLs: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 315:315
Comment:
**"Two" instructions but three are listed**
The text says "The prompt already contains two instructions about preserving image URLs:" but then lists three items (lines 317, 318, 319). Should say "three instructions" to match the list.
```suggestion
The prompt already contains three instructions about preserving image URLs:
```
How can I resolve this? If you propose a fix, please make it concise.| // 3. Fail if any images couldn't be downloaded (no broken placeholders!) | ||
| if (imageResult.stats.totalFailures > 0) { | ||
| throw new Error( | ||
| `Image stabilization failed for "${title}": ${imageResult.stats.totalFailures} image(s) failed` |
There was a problem hiding this comment.
Inconsistent variable name: title vs originalTitle
The code example uses "${title}" in the error message (line 229) and title in translateText (line 235), but uses originalTitle in generateSafeFilename (line 223). In the SPEC and PLAN, this variable is consistently originalTitle. The simplified spec should use the same variable name to avoid confusion during implementation.
| `Image stabilization failed for "${title}": ${imageResult.stats.totalFailures} image(s) failed` | |
| `Image stabilization failed for "${originalTitle}": ${imageResult.stats.totalFailures} image(s) failed` |
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC_SIMPLIFIED.md
Line: 229:229
Comment:
**Inconsistent variable name: `title` vs `originalTitle`**
The code example uses `"${title}"` in the error message (line 229) and `title` in `translateText` (line 235), but uses `originalTitle` in `generateSafeFilename` (line 223). In the SPEC and PLAN, this variable is consistently `originalTitle`. The simplified spec should use the same variable name to avoid confusion during implementation.
```suggestion
`Image stabilization failed for "${originalTitle}": ${imageResult.stats.totalFailures} image(s) failed`
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // 3. Translate (now with stable paths) | ||
| // 4. Translate (now with stable paths) | ||
| const translated = await translateText(markdownContent, title, language); |
There was a problem hiding this comment.
Variable name mismatch with SPEC and PLAN
translateText(markdownContent, title, language) uses title and language, but the SPEC and PLAN use originalTitle and config.language. Since this is a developer-facing code example, matching the actual variable names from the implementation reduces ambiguity.
| const translated = await translateText(markdownContent, title, language); | |
| const translated = await translateText(markdownContent, originalTitle, config.language); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC_SIMPLIFIED.md
Line: 235:235
Comment:
**Variable name mismatch with SPEC and PLAN**
`translateText(markdownContent, title, language)` uses `title` and `language`, but the SPEC and PLAN use `originalTitle` and `config.language`. Since this is a developer-facing code example, matching the actual variable names from the implementation reduces ambiguity.
```suggestion
const translated = await translateText(markdownContent, originalTitle, config.language);
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Same Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: TRANSLATION_IMAGE_STABILIZATION_SPEC.md
Line: 376:376
Comment:
**Test file path also inconsistent here**
Same `__tests__/` subfolder path mismatch as in the Phase 3 table. The unit test section references `scripts/notion-translate/__tests__/imageStabilization.test.ts` but should be `scripts/notion-translate/imageStabilization.test.ts` per the PLAN's convention note.
```suggestion
Create `scripts/notion-translate/imageStabilization.test.ts`:
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
luandro
merged commit 9f73b74
into
feat/translation-image-stabilization-spec
🧹 Preview Deployment CleanupThe preview deployment for this PR has been cleaned up. Preview URL was: Note: Cloudflare Pages deployments follow automatic retention policies. Old previews are cleaned up automatically. |
2 participants
Summary
Updated the translation image stabilization specification documents to clarify the initial implementation approach and correct several inaccuracies identified during review.
Key Changes
Clarified initial implementation scope: The initial phase always uses English page markdown for image processing. Per-language image overrides (using localized Notion page images) are documented as a future enhancement, not part of the current implementation.
Corrected function naming and behavior:
processPageTranslation()toprocessSinglePageTranslation()throughoutgetImageDiagnostics()is used for post-translation validation (with custom throw logic), notvalidateAndFixRemainingImages()which only warnsgenerateSafeFilename(title, pageId)that includes page ID suffix for deterministic, collision-free filenamesStrengthened failure handling:
imageResult.stats.totalFailures > 0before proceeding to translationImproved documentation clarity:
Updated test requirements:
/images/...paths (no-op case)Simplified companion documents:
TRANSLATION_IMAGE_STABILIZATION_SPEC_SIMPLIFIED.mdto match clarified scopeTRANSLATION_IMAGE_STABILIZATION_SPEC_REVIEW.mdwith corrected analysisImplementation Details
The specification now accurately reflects that:
/images/...paths)getImageDiagnostics()+ custom throw, not the existingvalidateAndFixRemainingImages()functionThese clarifications align the specification with the actual implementation requirements and reduce ambiguity for developers.
https://claude.ai/code/session_01RMuRBsneMpCL1cBVLuVcmx
Greptile Summary
This PR updates the translation image stabilization specification documents to clarify the initial implementation approach and resolve inconsistencies found during review. The key changes are:
validateAndFixRemainingImages(which only warns) withgetImageDiagnostics+ custom throw logic for stricter post-translation validation; renamedprocessPageTranslationtoprocessSinglePageTranslation; updatedgenerateSafeFilenameto include page ID for collision-free filenamestotalFailures > 0check before proceedingIssues found:
__tests__/subfolder for test files in two places, while the PLAN explicitly notes the project uses flat directory convention — this will confuse the implementertitle/languagevsoriginalTitle/config.language) in code examples compared to the SPEC and PLANConfidence Score: 4/5
Important Files Changed
__tests__/subfolder while PLAN uses flat directory; "two instructions" text lists three items.titlevsoriginalTitle,languagevsconfig.language) in code examples that diverge from SPEC and PLAN, which could confuse implementers.Flowchart
flowchart TD A["processSinglePageTranslation()"] --> B{Is title page?} B -- Yes --> C["translatedContent = # title\n(skip image processing)"] B -- No --> D["convertPageToMarkdown(englishPage.id)"] D --> E["generateSafeFilename(title, pageId)"] E --> F["processAndReplaceImages(markdown, safeFilename)"] F --> G{totalFailures > 0?} G -- Yes --> H["❌ Throw: Image stabilization failed"] G -- No --> I["Log image stats (if any processed)"] I --> J["translateText(stabilizedMarkdown, title, language)"] J --> K["getImageDiagnostics(translatedContent)"] K --> L{s3Matches > 0?} L -- Yes --> M["❌ Throw: S3 URLs remain post-translation"] L -- No --> N["✅ Continue to saveTranslatedContentToDisk()"] C --> NLast reviewed commit: 7a955c2