fix: preserve raw markdown content in markdown mode#12
Conversation
…tion Moved normalization control from MarkdownEditor to EditorComponent to display files exactly as they appear on disk when in markdown mode. Previously, content was being normalized through markdown-it → HTML → turndown pipeline even in markdown mode, causing unwanted whitespace changes and false dirty states. Normalization now only occurs when switching to WYSIWYG mode or initially loading in WYSIWYG mode, ensuring markdown mode preserves all original formatting including multiple blank lines.
Code Review: Fix Markdown Whitespace PreservationSummaryThis PR successfully addresses the issue of markdown content normalization by centralizing normalization control in ✅ Strengths
🔍 Issues & Concerns1. Missing Dependency in useEffect (High Priority)Location: The // Current (line 94)
}, [viewMode, editor, content, onUpdate, onContentLoaded]);Recommendation: Use const onUpdateRef = useRef(onUpdate);
const onContentLoadedRef = useRef(onContentLoaded);
useEffect(() => {
onUpdateRef.current = onUpdate;
onContentLoadedRef.current = onContentLoaded;
}, [onUpdate, onContentLoaded]);
// Then use onUpdateRef.current and onContentLoadedRef.current in the effect
// and remove them from the dependency arrayNote: 2. Logic Complexity in useEffect (Medium Priority)Location: The normalization logic has become quite complex with three different conditions:
Concerns:
Recommendation: Simplify and add comments: // Normalize content when:
// 1. Switching from markdown to WYSIWYG (view mode transition)
// 2. Initial load with content while already in WYSIWYG mode
// 3. Content changes while in WYSIWYG mode
const shouldNormalize =
isViewModeTransition ||
(viewMode === 'wysiwyg' && (isInitialLoad || hasContentChanged));3. Error Handling Inconsistency (Low Priority)Location: If normalization fails, the error is logged but the content still attempts to convert using Recommendation: Fall back to original content on error: } catch (error) {
console.error('Failed to normalize markdown content during view mode switch:', error);
contentToConvert = content; // Use original content on error
}4. Normalization on Initial WYSIWYG Load Modifies File (Medium Priority)Location: When initially loading a file in WYSIWYG mode, the code normalizes the content and calls
Question: Is this intended behavior? Should initial loads in WYSIWYG mode modify the baseline content? Recommendation: Consider only normalizing on view mode transitions, not initial loads. Or add a user preference for "preserve original formatting until first edit". 🧪 Test CoverageCritical Issue: No test coverage exists for these components. Files to test:
Suggested test cases:
🔒 Security ConsiderationsNo security issues identified. The changes don't introduce new attack vectors:
⚡ Performance Considerations
📝 DocumentationThe CLAUDE.md should be updated to reflect this change: Current documentation (line ~90):
Should be updated to:
🎯 Recommendations SummaryMust Fix:
Should Fix: Nice to Have: ✨ ConclusionThis is a well-thought-out fix that correctly identifies and solves the root cause. The main concerns are around the useEffect dependency array (which could cause bugs) and missing test coverage. Once those are addressed, this should be ready to merge. Approval Status: |
Remove normalization on initial WYSIWYG load to preserve original file formatting. Previously, opening a file while in WYSIWYG mode would immediately normalize the content, destructively modifying intentional whitespace without user action. Now normalization only occurs when user explicitly switches from markdown to WYSIWYG mode.
PR Review: Fix Markdown Whitespace PreservationSummaryThis PR addresses a legitimate issue where markdown files weren't displaying their raw content in markdown mode due to unwanted normalization. The solution appropriately centralizes normalization control in ✅ Strengths1. Clear Problem-Solution Architecture
2. Code Quality
3. Performance
|
Remove all normalization on view mode switches and initial loads. Viewing in WYSIWYG mode is now completely non-destructive - files are converted to TipTap JSON for display only. Normalization only occurs when user actually edits in WYSIWYG mode (via onUpdate callback), ensuring intentional whitespace is preserved until user explicitly modifies content.
Code Review: Fix Markdown Whitespace PreservationSummaryThis PR successfully addresses the issue of markdown content normalization in markdown mode. The changes are well-focused and properly centralize normalization control. ✅ Strengths1. Correct Problem Identification
2. Minimal, Focused Changes
3. Good Documentation
|
|
@claude address the high and medium priority recs. For the View Mode State Inconsistency question, you are completely correct. if there hasn't been an edit in wysiwyg mode, the markdown view should reflect the unnormalized state of the file on the filesystem. No edits should have been made. |
|
Claude finished @saadiq's task —— View job ✅ All High and Medium Priority Recommendations AddressedChanges SummaryAll issues fixed in commit
How It Works NowTesting RecommendationThe user should test:
Job Run • Branch: |
- Re-enable onContentLoaded with conditional WYSIWYG-only logic - Track WYSIWYG edits to preserve unnormalized content when viewing - Remove buggy isInitialLoad logic in favor of clear view transitions - Use stable callback refs to prevent useEffect dependency issues - Restore raw content when switching to markdown if no edits made This ensures markdown mode always shows raw file content unless explicit edits were made in WYSIWYG mode. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Saadiq Rodgers-King <saadiq@users.noreply.github.com>
Code ReviewThank you for this PR! The changes effectively address the markdown normalization issue. Here's my detailed feedback: ✅ Strengths
🐛 Potential Issues1. Race Condition with
|
…prehensive tests Address code review feedback with the following improvements: - Fix race condition with rawContentRef by introducing selfTriggeredChangeRef flag to distinguish between external content changes (file loads) and self-triggered normalization updates, preventing raw content from being overwritten - Refactor complex view mode sync logic into separate handler functions for better maintainability: - handleMarkdownToWysiwyg: Manages markdown → WYSIWYG transitions - handleWysiwygToMarkdown: Manages WYSIWYG → markdown transitions - handleContentChangeInWysiwyg: Handles content updates in WYSIWYG mode - Add comprehensive test suite (473 lines) covering: - Raw markdown content preservation in markdown mode - Content normalization in WYSIWYG mode - View mode transitions and content restoration - Race condition prevention with selfTriggeredChangeRef - Content updates and edit tracking - Edge cases (empty content, undefined callbacks, localStorage) - Keyboard shortcuts in different view modes This refactoring improves code organization, prevents subtle timing bugs, and provides test coverage for the markdown whitespace preservation feature.
Code Review: Fix Markdown Whitespace PreservationSummaryThis PR successfully addresses the markdown normalization issue by centralizing content control and preserving raw markdown in markdown mode. The implementation is well-thought-out with comprehensive test coverage. ✅ StrengthsArchitecture & Design
Code Quality
UX Improvements
🔍 Areas for Consideration1. Potential Memory Concerns with
|
Add isExternalChangeRef to distinguish between user typing and external content updates. Only sync external changes to local state if they differ from current local content and aren't from user input. This prevents cursor position jumps that could occur if external updates (e.g., auto-save callbacks) trigger re-renders while user is typing.
PR Review: Fix Preserve Raw Markdown Content in Markdown ModeSummaryThis PR successfully addresses the issue of markdown content being normalized when viewing files in markdown mode. The implementation centralizes normalization control to Code Quality & Best Practices✅ Strengths
🔍 Potential Issues & Concerns1. Complex State Management with Multiple Refs (Medium Priority)The component now manages 5+ refs tracking various states:
Issue: This many refs can make the component harder to reason about and maintain. The interaction between Suggestion: Consider adding a diagram or expanded comment block at the top of the component explaining the state flow and ref interactions: /**
* Content Flow & State Management:
*
* 1. File Load:
* - rawContentRef stores original content
* - hasWysiwygEditsRef reset to false
*
* 2. Markdown Mode:
* - Display rawContentRef as-is
* - No normalization
*
* 3. Switch to WYSIWYG:
* - Content normalized via markdownToJSON
* - onContentLoaded triggers
* - selfTriggeredChangeRef prevents overwriting rawContentRef
*
* 4. Edit in WYSIWYG:
* - hasWysiwygEditsRef = true
* - Content updates flow normally
*
* 5. Switch back to Markdown:
* - IF hasWysiwygEditsRef: keep normalized content
* - ELSE: restore rawContentRef
*/2. Potential Bug in MarkdownEditor (High Priority)In useEffect(() => {
if (isExternalChangeRef.current && content !== localContent) {
setLocalContent(content);
}
// Reset flag after each content prop change
isExternalChangeRef.current = true;
}, [content, localContent]);Issue: Suggestion: Remove useEffect(() => {
if (isExternalChangeRef.current && content !== localContent) {
setLocalContent(content);
}
isExternalChangeRef.current = true;
}, [content]); // Remove localContent from deps3. Race Condition Edge Case (Low Priority)In if (hasContentChanged) {
hasWysiwygEditsRef.current = false;
if (!selfTriggeredChangeRef.current) {
rawContentRef.current = content;
}
selfTriggeredChangeRef.current = false;
}Issue: If two rapid content changes occur (external file change + normalization callback), the second change might incorrectly update Suggestion: Consider using a more robust tracking mechanism, perhaps with a timestamp or content hash to identify self-triggered changes more reliably. 4. Missing Null Check (Low Priority)In const handleWysiwygToMarkdown = useCallback(() => {
if (!hasWysiwygEditsRef.current && onUpdateRef.current) {
onUpdateRef.current(rawContentRef.current);
}
}, []);Suggestion: Add a null/undefined check: if (!hasWysiwygEditsRef.current && onUpdateRef.current && rawContentRef.current !== undefined) {
onUpdateRef.current(rawContentRef.current);
}Security Concerns✅ No security issues identified. The changes:
Performance Considerations✅ Good Performance Practices
|
Add detailed JSDoc block explaining the complex state management in EditorComponent, including: - Purpose and lifecycle of each ref (rawContentRef, hasWysiwygEditsRef, selfTriggeredChangeRef, etc.) - 7 detailed scenarios showing state transitions - Race conditions and how they're handled - Key design decisions This addresses code review feedback about the component's complexity and makes the interaction between multiple refs easier to understand and maintain.
Remove localContent from dependency array to prevent unnecessary effect executions on every keystroke. The comparison 'content !== localContent' uses closure to capture the current localContent value, which is sufficient for detecting external updates. This fixes a performance issue where the effect would run on every user keystroke even though the condition would fail (isExternalChangeRef.current = false during typing).
Code Review: Preserve Raw Markdown Content in Markdown ModeI've reviewed this PR and have feedback on code quality, potential issues, and improvements. Overall, this is a well-thought-out solution to a complex problem with excellent documentation. Strengths
Critical Issues1. Dangerous ESLint Disable in MarkdownEditor (lines 35-36) The comment claims localContent is intentionally NOT in the dependency array and that the closure captures the value, but this is incorrect. The closure captures localContent at the time the effect is created, not the current value during execution. This creates a race condition where content updates may not be detected correctly. Fix: Remove the eslint-disable and add localContent to the dependency array. 2. Memory Leak Risk with Callback Refs (lines 149-156) The React-recommended pattern is to update refs synchronously without useEffect. 3. Inconsistent Error Handling The code doesn't handle errors in markdownToJSON conversions. Consider adding try-catch with toast notifications. Performance Considerations4. Excessive Effect Dependencies (line 252) The three handler functions are recreated whenever editor or content changes. Consider inlining handlers inside the effect. Test Coverage Gaps5. Missing Test Scenarios
Recommended PriorityMust Fix Before Merge:
Should Fix: Overall AssessmentThis PR demonstrates strong engineering with excellent documentation, comprehensive tests, and thoughtful handling of complex state. However, the ESLint disable in MarkdownEditor is a critical bug that must be fixed before merge. Recommendation: Request changes for Issue 1, then approve with the other suggestions as follow-up improvements. |
Replace closure-based comparison with ref pattern to avoid stale closures. The previous implementation used localContent via closure, which could capture stale values. Now using localContentRef.current ensures we always compare against the current value. This fixes the actual bug that the ESLint disable was masking.
…prehensive tests Three key improvements: 1. Fix callback ref pattern: Remove unnecessary useEffect and update refs on every render (standard React pattern). This is more performant and simpler. 2. Add error handling: Wrap all markdownToJSON calls in try-catch blocks with fallback to plain text. This prevents crashes from malformed markdown. 3. Inline handlers: Remove useCallback wrappers and inline logic directly into main effect. This eliminates circular dependencies and reduces complexity. Dependencies reduced from 6 to 3: [viewMode, editor, content] Tests added: - Error handling and recovery - Callback ref updates and stale callback prevention
New test file for MarkdownEditor covering: - Cursor jump prevention during typing - External vs internal update handling - isExternalChangeRef flag behavior - Props and attributes - Content synchronization - Edge cases (empty, long, special characters) Extended EditorComponent tests with: - Error handling for markdown parsing failures - Fallback behavior when conversion fails - Error recovery and continued functionality - Callback ref updates and stale callback prevention Total test coverage expanded significantly for both critical components.
Pull Request Review: Fix Markdown Content PreservationSummaryThis PR successfully addresses the markdown normalization issue by centralizing content control and preventing unwanted whitespace changes. The implementation is solid with excellent test coverage. ✅ Strengths1. Clear Problem DefinitionThe PR description clearly articulates the issue (unwanted normalization) and the solution (centralized control in EditorComponent). 2. Comprehensive DocumentationThe 100+ line JSDoc comment in
This is exemplary documentation for complex state management. 3. Excellent Test Coverage
4. Progressive RefinementThe commit history shows iterative improvement based on review feedback:
🔍 Code Quality ObservationsArchitecture✅ Good separation of concerns: Implementation DetailsEditorComponent.tsx (
MarkdownEditor.tsx (
🐛 Potential Issues1. Minor: Dependency Array Question (MarkdownEditor.tsx:29-35)useEffect(() => {
if (isExternalChangeRef.current && content !== localContentRef.current) {
setLocalContent(content);
}
isExternalChangeRef.current = true;
}, [content]);Question: Should The current implementation uses a ref to avoid stale closures, which is correct. However, the ESLint rule was disabled in an earlier version (commit Verdict: ✅ Current implementation is correct - no action needed. 2. Edge Case: Rapid File SwitchingScenario: User rapidly switches between files while in WYSIWYG mode Current behavior:
Verdict: ✅ Properly handled by existing refs 3. Edge Case: External File ModificationScenario: File changes on disk while open in editor (via file watcher) Current behavior (EditorComponent.tsx:197-203): if (hasContentChanged) {
hasWysiwygEditsRef.current = false;
if (!selfTriggeredChangeRef.current) {
rawContentRef.current = content;
}
selfTriggeredChangeRef.current = false;
}Potential issue: If user has made WYSIWYG edits and the file changes externally, their edits are discarded without warning. Recommendation: Impact: Low priority - rare edge case, but worth considering for future enhancement. 🔒 Security✅ No security concerns - changes are purely client-side state management ⚡ Performance✅ Good performance characteristics:
Potential optimization (future):
🧪 Test CoverageExcellent Coverage ✅
Test Quality✅ Tests are well-structured with clear describe blocks Suggestions for Additional Tests (optional)
Priority: Low - current coverage is already excellent 📋 Best Practices AlignmentChecking against CLAUDE.md guidelines: ✅ TypeScript: Properly typed throughout 🎯 RecommendationsMust AddressNone - this PR is ready to merge as-is. Should Consider (Future Enhancements)
Nice to Have
📊 Final VerdictRating: ⭐⭐⭐⭐⭐ (5/5) Recommendation: ✅ Approve and merge This PR demonstrates:
The implementation is production-ready. The only suggestion (external file change warning) is a nice-to-have for future work, not a blocker. Great work on this fix! 🎉 📝 Minor Nits (optional cleanup)
These are purely stylistic and don't affect functionality. |
1 participant
Summary
Fixes markdown editor to display files exactly as they appear on disk, preserving all whitespace including multiple blank lines when in markdown mode.
Problem
The markdown editor was normalizing content through the markdown-it → HTML → turndown pipeline even when viewing in markdown mode. This caused:
Solution
onContentLoadedcallback fromuseEditorhookResult
✅ Markdown mode displays files exactly as they are on disk
✅ WYSIWYG mode still receives normalized content for proper rendering
✅ No false dirty states - content only changes when explicitly switching to WYSIWYG
✅ Round-trip fidelity maintained where needed
Testing