From 6cafb9218b5091c6e3af28f755ba108f539fdc85 Mon Sep 17 00:00:00 2001 From: StillHammer Date: Mon, 13 Oct 2025 09:33:16 +0800 Subject: [PATCH] Fix StoryReader compatibility and memory leak issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🔧 Changes: - StoryReader now handles both old/new text structures (original_language vs content) - Fixed memory leak: properly remove global event listeners on destroy - Added null check in _hideWordPopup() to prevent errors after DOM cleanup - Fixed chapter list display (was showing only one chapter instead of all) - Smart routing: only load chapter content when needed - Use module ContentLoader for proper content loading with vocabulary 🐛 Bugs Fixed: 1. "Cannot read properties of undefined (reading 'split')" - StoryReader couldn't handle texts with 'content' field 2. "Cannot read properties of null (reading 'style')" - Event listeners firing after game cleanup 3. Chapter list showing only book ID instead of all chapters 4. Game compatibility scores dropping to 0.00 after navigation ✨ Architecture Improvements: - Event listener cleanup follows best practices - Proper handler reference storage for removeEventListener - Defensive programming with null checks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- index.html | 94 ++++++++++++++++++++++++++------------- src/Application.js | 43 +++++++++++------- src/core/ContentLoader.js | 14 +++--- src/core/GameLoader.js | 9 ++++ src/games/StoryReader.js | 94 ++++++++++++++++++++++++++++++--------- 5 files changed, 179 insertions(+), 75 deletions(-) diff --git a/index.html b/index.html index 720e908..1a02e89 100644 --- a/index.html +++ b/index.html @@ -272,6 +272,30 @@ // Load content using ContentLoader to get reports const contentData = await contentLoader.loadContent(bookId); + // Generate chapter cards from contentData.chapters array + const chapters = contentData.chapters || []; + const chapterCards = chapters.map(chapter => ` +
+
${chapter.chapter_number || '?'}
+
+

${chapter.name || chapter.title}

+

${chapter.description || ''}

+
+ ${chapter.difficulty} + ${chapter.language || contentData.language} +
+
+
+
+
+ 0% Complete +
+
+
+ `).join(''); + updateMainContent(`
@@ -287,25 +311,7 @@
-
-
7-8
-
-

${contentData.name}

-

${contentData.description}

-
- ${contentData.difficulty} - ${contentData.language} -
-
-
-
-
- 0% Complete -
-
-
+ ${chapterCards || '

No chapters available

'}
`); @@ -339,19 +345,22 @@ throw new Error('GameLoader not available'); } - // Load current content for compatibility scoring - const content = contentLoader.getContent(chapterId); - if (content) { - eventBus.emit('content:loaded', { content }, 'Bootstrap'); - - // Wait a bit for the event to be processed, then get games - setTimeout(() => { - window._renderGamesInterface(gameLoader, chapterId); - }, 50); - return; + // Load CHAPTER content (not book) for compatibility scoring + // Use module-based ContentLoader which loads chapter JSON + const moduleContentLoader = app.getModule('contentLoader'); + if (moduleContentLoader) { + const chapterContent = await moduleContentLoader.getContent(chapterId); + if (chapterContent) { + // Module ContentLoader already emits content:loaded, no need to emit again + // Just wait for it to be processed + setTimeout(() => { + window._renderGamesInterface(gameLoader, chapterId); + }, 50); + return; + } } - // If no content, render immediately with no games + // If no module content loader or content, render immediately window._renderGamesInterface(gameLoader, chapterId); } catch (error) { console.error('Error loading games:', error); @@ -738,6 +747,21 @@ } }; + window.selectChapter = function(bookId, chapterId) { + try { + // Set global chapter ID for Games/Smart Guide + window.currentBookId = bookId; + window.currentChapterId = chapterId; + console.log(`✅ Chapter selected: ${bookId}/${chapterId}`); + + // Navigate to games for this chapter + const { router } = app.getCore(); + router.navigate(`/games/${chapterId}`); + } catch (error) { + console.error('Chapter selection error:', error); + } + }; + window.openPlaceholder = function() { alert('Quick Play feature coming soon!'); }; @@ -787,8 +811,14 @@ throw new Error('GameLoader not available'); } - // Get current content - const content = contentLoader.getContent(currentChapterId); + // Get current content using module ContentLoader (async) + const moduleContentLoader = app.getModule('contentLoader'); + if (!moduleContentLoader) { + throw new Error('ContentLoader not available'); + } + + // Load chapter content asynchronously + const content = await moduleContentLoader.getContent(currentChapterId || window.currentChapterId); if (!content) { throw new Error('No content loaded for current chapter'); } diff --git a/src/Application.js b/src/Application.js index 2141d3e..fbcc796 100644 --- a/src/Application.js +++ b/src/Application.js @@ -332,30 +332,41 @@ class Application { } async _handleGamesRoute(path, state) { - this._eventBus.emit('navigation:games', { path, state }, 'Application'); - // Simple approach: Force re-render by emitting the chapter navigation event console.log('🔄 Games route - path:', path, 'state:', state); - // Extract chapter ID from path or use current one + // Extract ID from path const pathParts = path.split('/'); - let chapterId = pathParts[2] || window.currentChapterId || 'sbs'; + const pathId = pathParts[2]; - console.log('🔄 Games route - using chapterId:', chapterId); + // IMPORTANT: Use window.currentChapterId if available (set by Smart Guide/Chapter selection) + // If not available, check if we should use the path ID + let chapterId = window.currentChapterId; - // Make sure currentChapterId is set - if (!window.currentChapterId) { - window.currentChapterId = chapterId; + if (!chapterId && pathId) { + // Path has an ID, but we don't know if it's a book or chapter + // Try to use it, but warn the user + console.warn(`⚠️ Using ID from path: ${pathId} (may be book ID, should be chapter ID)`); + chapterId = pathId; } - // Force navigation to the chapter games - this will trigger the content loading - setTimeout(() => { - console.log('🔄 Games route - forcing navigation event'); - this._eventBus.emit('navigation:games', { - path: `/games/${chapterId}`, - data: { path: `/games/${chapterId}` } - }, 'Application'); - }, 100); + if (!chapterId) { + console.warn('⚠️ No chapter selected. Please select a chapter first.'); + // Redirect to home - user needs to select a chapter + this._router.navigate('/'); + return; + } + + console.log('🔄 Games route - using chapterId:', chapterId, '(from', window.currentChapterId ? 'window.currentChapterId' : 'path', ')'); + + // Keep currentChapterId consistent + window.currentChapterId = chapterId; + + // Emit navigation event ONCE - don't emit twice + this._eventBus.emit('navigation:games', { + path: `/games/${chapterId}`, + data: { path: `/games/${chapterId}` } + }, 'Application'); } async _handleDynamicRevisionRoute(path, state) { diff --git a/src/core/ContentLoader.js b/src/core/ContentLoader.js index 564926b..cfe00cc 100644 --- a/src/core/ContentLoader.js +++ b/src/core/ContentLoader.js @@ -141,12 +141,8 @@ class ContentLoader extends Module { // Cache the result this._contentCache.set(cacheKey, content); - // Emit success event - this._eventBus.emit('content:loaded', { - request, - content, - cached: false - }, this.name); + // DON'T emit content:loaded for exercise content - that's for chapter content only + // (exercises are ephemeral and shouldn't trigger game compatibility recalculation) console.log(`✅ Content loaded: ${content.title || cacheKey}`); return content; @@ -2491,6 +2487,12 @@ Return ONLY valid JSON: vocabCount: content.vocabulary ? Object.keys(content.vocabulary).length : 0 }); + // Emit content:loaded event for GameLoader + this._eventBus.emit('content:loaded', { + content, + chapterId + }, this.name); + return content; } catch (error) { diff --git a/src/core/GameLoader.js b/src/core/GameLoader.js index e4a8f62..a58c6b6 100644 --- a/src/core/GameLoader.js +++ b/src/core/GameLoader.js @@ -279,6 +279,15 @@ class GameLoader extends Module { _handleContentUpdate(event) { console.log('🔄 GameLoader: Content updated, recalculating compatibility scores'); + const structure = { + hasContent: !!event.data.content, + contentKeys: Object.keys(event.data.content || {}), + hasVocabulary: !!event.data.content?.vocabulary, + vocabularyCount: event.data.content?.vocabulary ? Object.keys(event.data.content.vocabulary).length : 0, + hasTexts: !!event.data.content?.texts, + hasSentences: !!event.data.content?.sentences + }; + console.log('📦 GameLoader: event.data structure:', JSON.stringify(structure, null, 2)); this._currentContent = event.data.content; // Recalculate compatibility scores for all games diff --git a/src/games/StoryReader.js b/src/games/StoryReader.js index 61d5e83..3e7cced 100644 --- a/src/games/StoryReader.js +++ b/src/games/StoryReader.js @@ -40,6 +40,10 @@ class StoryReader extends Module { this._startTime = null; this._totalReadingTime = 0; + // Event listener references for cleanup + this._boundKeydownHandler = null; + this._boundClickOutsideHandler = null; + Object.seal(this); } @@ -64,17 +68,33 @@ class StoryReader extends Module { * @returns {Object} Compatibility score and details */ static getCompatibilityScore(content) { + const info = { + hasStory: !!content?.story, + hasAdditionalStories: !!content?.additionalStories, + hasTexts: !!content?.texts, + hasSentences: !!content?.sentences, + contentKeys: Object.keys(content || {}) + }; + console.log('🔍 StoryReader: Checking compatibility with content:', JSON.stringify(info, null, 2)); + let storyCount = 0; - let hasMainStory = !!(content?.story?.title || content?.rawContent?.story?.title); - let hasAdditionalStories = !!(content?.additionalStories?.length || content?.rawContent?.additionalStories?.length); - let hasTexts = !!(content?.texts?.length || content?.rawContent?.texts?.length); - let hasSentences = !!(content?.sentences?.length || content?.rawContent?.sentences?.length); + let hasMainStory = !!(content?.story?.title); + let hasAdditionalStories = !!(content?.additionalStories?.length); + let hasTexts = !!(content?.texts?.length); + let hasSentences = !!(content?.sentences?.length); if (hasMainStory) storyCount++; - if (hasAdditionalStories) storyCount += (content?.additionalStories?.length || content?.rawContent?.additionalStories?.length); - if (hasTexts) storyCount += (content?.texts?.length || content?.rawContent?.texts?.length); + if (hasAdditionalStories) storyCount += content.additionalStories.length; + if (hasTexts) storyCount += content.texts.length; if (hasSentences) storyCount++; + console.log('🎯 StoryReader: Story count =', storyCount, { + hasMainStory, + hasAdditionalStories, + hasTexts, + hasSentences + }); + if (storyCount === 0) { return { score: 0, @@ -114,7 +134,7 @@ class StoryReader extends Module { // Select initial story this._selectStory(0); - this._vocabulary = this._content?.vocabulary || this._content?.rawContent?.vocabulary || {}; + this._vocabulary = this._content?.vocabulary || {}; // Set up event listeners this._eventBus.on('game:pause', this._handlePause.bind(this), this.name); @@ -161,6 +181,16 @@ class StoryReader extends Module { this._readingTimer = null; } + // Remove global event listeners + if (this._boundKeydownHandler) { + document.removeEventListener('keydown', this._boundKeydownHandler); + this._boundKeydownHandler = null; + } + if (this._boundClickOutsideHandler) { + document.removeEventListener('click', this._boundClickOutsideHandler); + this._boundClickOutsideHandler = null; + } + // Clean up container if (this._config.container) { this._config.container.innerHTML = ''; @@ -204,7 +234,7 @@ class StoryReader extends Module { this._availableStories = []; // Check main story field - const mainStory = this._content.rawContent?.story || this._content.story; + const mainStory = this._content.story; if (mainStory && mainStory.title) { this._availableStories.push({ id: 'main', @@ -215,7 +245,7 @@ class StoryReader extends Module { } // Check additional stories - const additionalStories = this._content.rawContent?.additionalStories || this._content.additionalStories; + const additionalStories = this._content.additionalStories; if (additionalStories && Array.isArray(additionalStories)) { additionalStories.forEach((story, index) => { if (story && story.title) { @@ -230,7 +260,7 @@ class StoryReader extends Module { } // Check texts and convert to stories - const texts = this._content.rawContent?.texts || this._content.texts; + const texts = this._content.texts; if (texts && Array.isArray(texts)) { texts.forEach((text, index) => { if (text && (text.title || text.original_language)) { @@ -246,7 +276,7 @@ class StoryReader extends Module { } // Check sentences and create story from them - const sentences = this._content.rawContent?.sentences || this._content.sentences; + const sentences = this._content.sentences; if (sentences && Array.isArray(sentences) && sentences.length > 0 && this._availableStories.length === 0) { const sentencesStory = this._convertSentencesToStory(sentences); this._availableStories.push({ @@ -280,7 +310,18 @@ class StoryReader extends Module { } _convertTextToStory(text, index) { - const sentences = this._splitTextIntoSentences(text.original_language, text.user_language); + // Handle different text structures + // 1. Old structure: text.original_language + text.user_language + // 2. New structure: text.content (no translation yet) + let originalText = text.original_language || text.content || ''; + let translationText = text.user_language || text.translation || ''; + + // If no translation, use empty string + if (!translationText) { + translationText = ''; + } + + const sentences = this._splitTextIntoSentences(originalText, translationText); return { title: text.title || `Text ${index + 1}`, totalSentences: sentences.length, @@ -309,9 +350,15 @@ class StoryReader extends Module { }; } - _splitTextIntoSentences(originalText, translationText) { + _splitTextIntoSentences(originalText, translationText = '') { + // Validate inputs + if (!originalText || typeof originalText !== 'string') { + console.warn('StoryReader: Invalid originalText provided to _splitTextIntoSentences', originalText); + return []; + } + const originalSentences = originalText.split(/[.!?]+/).filter(s => s.trim().length > 0); - const translationSentences = translationText.split(/[.!?]+/).filter(s => s.trim().length > 0); + const translationSentences = translationText ? translationText.split(/[.!?]+/).filter(s => s.trim().length > 0) : []; const sentences = []; const maxSentences = Math.max(originalSentences.length, translationSentences.length); @@ -758,8 +805,8 @@ class StoryReader extends Module { // TTS button in popup document.getElementById('popup-tts-btn').addEventListener('click', () => this._speakWordFromPopup()); - // Keyboard shortcuts - document.addEventListener('keydown', (e) => { + // Keyboard shortcuts - store handler reference for cleanup + this._boundKeydownHandler = (e) => { if (e.target.tagName === 'INPUT' || e.target.tagName === 'SELECT') return; switch (e.key) { @@ -780,14 +827,16 @@ class StoryReader extends Module { this._playSentenceTTS(); break; } - }); + }; + document.addEventListener('keydown', this._boundKeydownHandler); - // Click outside to close popup - document.addEventListener('click', (e) => { + // Click outside to close popup - store handler reference for cleanup + this._boundClickOutsideHandler = (e) => { if (!e.target.closest('.word-popup') && !e.target.closest('.clickable-word')) { this._hideWordPopup(); } - }); + }; + document.addEventListener('click', this._boundClickOutsideHandler); } _getCurrentSentenceData() { @@ -938,7 +987,10 @@ class StoryReader extends Module { } _hideWordPopup() { - document.getElementById('word-popup').style.display = 'none'; + const popup = document.getElementById('word-popup'); + if (popup) { + popup.style.display = 'none'; + } } _previousSentence() {