Fix StoryReader compatibility and memory leak issues

🔧 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 <noreply@anthropic.com>
This commit is contained in:
StillHammer 2025-10-13 09:33:16 +08:00
parent 3da40c0d73
commit 6cafb9218b
5 changed files with 179 additions and 75 deletions

View File

@ -272,6 +272,30 @@
// Load content using ContentLoader to get reports // Load content using ContentLoader to get reports
const contentData = await contentLoader.loadContent(bookId); const contentData = await contentLoader.loadContent(bookId);
// Generate chapter cards from contentData.chapters array
const chapters = contentData.chapters || [];
const chapterCards = chapters.map(chapter => `
<div class="chapter-card" onclick="selectChapter('${bookId}', '${chapter.id}')"
onmouseenter="showTooltip(event, '${chapter.id}')"
onmouseleave="hideTooltip()">
<div class="chapter-number">${chapter.chapter_number || '?'}</div>
<div class="chapter-info">
<h3>${chapter.name || chapter.title}</h3>
<p class="chapter-description">${chapter.description || ''}</p>
<div class="chapter-meta">
<span class="difficulty-badge difficulty-${chapter.difficulty}">${chapter.difficulty}</span>
<span class="language-badge">${chapter.language || contentData.language}</span>
</div>
<div class="chapter-progress">
<div class="progress">
<div class="progress-bar" style="width: 0%"></div>
</div>
<span class="progress-text">0% Complete</span>
</div>
</div>
</div>
`).join('');
updateMainContent(` updateMainContent(`
<div class="chapters-container"> <div class="chapters-container">
<div class="chapters-header"> <div class="chapters-header">
@ -287,25 +311,7 @@
</div> </div>
<div class="chapters-grid"> <div class="chapters-grid">
<div class="chapter-card" onclick="navigateToGames('${bookId}')" ${chapterCards || '<p>No chapters available</p>'}
onmouseenter="showTooltip(event, '${bookId}')"
onmouseleave="hideTooltip()">
<div class="chapter-number">7-8</div>
<div class="chapter-info">
<h3>${contentData.name}</h3>
<p class="chapter-description">${contentData.description}</p>
<div class="chapter-meta">
<span class="difficulty-badge difficulty-${contentData.difficulty}">${contentData.difficulty}</span>
<span class="language-badge">${contentData.language}</span>
</div>
<div class="chapter-progress">
<div class="progress">
<div class="progress-bar" style="width: 0%"></div>
</div>
<span class="progress-text">0% Complete</span>
</div>
</div>
</div>
</div> </div>
</div> </div>
`); `);
@ -339,19 +345,22 @@
throw new Error('GameLoader not available'); throw new Error('GameLoader not available');
} }
// Load current content for compatibility scoring // Load CHAPTER content (not book) for compatibility scoring
const content = contentLoader.getContent(chapterId); // Use module-based ContentLoader which loads chapter JSON
if (content) { const moduleContentLoader = app.getModule('contentLoader');
eventBus.emit('content:loaded', { content }, 'Bootstrap'); if (moduleContentLoader) {
const chapterContent = await moduleContentLoader.getContent(chapterId);
// Wait a bit for the event to be processed, then get games if (chapterContent) {
// Module ContentLoader already emits content:loaded, no need to emit again
// Just wait for it to be processed
setTimeout(() => { setTimeout(() => {
window._renderGamesInterface(gameLoader, chapterId); window._renderGamesInterface(gameLoader, chapterId);
}, 50); }, 50);
return; return;
} }
}
// If no content, render immediately with no games // If no module content loader or content, render immediately
window._renderGamesInterface(gameLoader, chapterId); window._renderGamesInterface(gameLoader, chapterId);
} catch (error) { } catch (error) {
console.error('Error loading games:', 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() { window.openPlaceholder = function() {
alert('Quick Play feature coming soon!'); alert('Quick Play feature coming soon!');
}; };
@ -787,8 +811,14 @@
throw new Error('GameLoader not available'); throw new Error('GameLoader not available');
} }
// Get current content // Get current content using module ContentLoader (async)
const content = contentLoader.getContent(currentChapterId); 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) { if (!content) {
throw new Error('No content loaded for current chapter'); throw new Error('No content loaded for current chapter');
} }

View File

@ -332,30 +332,41 @@ class Application {
} }
async _handleGamesRoute(path, state) { async _handleGamesRoute(path, state) {
this._eventBus.emit('navigation:games', { path, state }, 'Application');
// Simple approach: Force re-render by emitting the chapter navigation event // Simple approach: Force re-render by emitting the chapter navigation event
console.log('🔄 Games route - path:', path, 'state:', state); 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('/'); 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 (!chapterId && pathId) {
if (!window.currentChapterId) { // Path has an ID, but we don't know if it's a book or chapter
window.currentChapterId = chapterId; // 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 if (!chapterId) {
setTimeout(() => { console.warn('⚠️ No chapter selected. Please select a chapter first.');
console.log('🔄 Games route - forcing navigation event'); // 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', { this._eventBus.emit('navigation:games', {
path: `/games/${chapterId}`, path: `/games/${chapterId}`,
data: { path: `/games/${chapterId}` } data: { path: `/games/${chapterId}` }
}, 'Application'); }, 'Application');
}, 100);
} }
async _handleDynamicRevisionRoute(path, state) { async _handleDynamicRevisionRoute(path, state) {

View File

@ -141,12 +141,8 @@ class ContentLoader extends Module {
// Cache the result // Cache the result
this._contentCache.set(cacheKey, content); this._contentCache.set(cacheKey, content);
// Emit success event // DON'T emit content:loaded for exercise content - that's for chapter content only
this._eventBus.emit('content:loaded', { // (exercises are ephemeral and shouldn't trigger game compatibility recalculation)
request,
content,
cached: false
}, this.name);
console.log(`✅ Content loaded: ${content.title || cacheKey}`); console.log(`✅ Content loaded: ${content.title || cacheKey}`);
return content; return content;
@ -2491,6 +2487,12 @@ Return ONLY valid JSON:
vocabCount: content.vocabulary ? Object.keys(content.vocabulary).length : 0 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; return content;
} catch (error) { } catch (error) {

View File

@ -279,6 +279,15 @@ class GameLoader extends Module {
_handleContentUpdate(event) { _handleContentUpdate(event) {
console.log('🔄 GameLoader: Content updated, recalculating compatibility scores'); 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; this._currentContent = event.data.content;
// Recalculate compatibility scores for all games // Recalculate compatibility scores for all games

View File

@ -40,6 +40,10 @@ class StoryReader extends Module {
this._startTime = null; this._startTime = null;
this._totalReadingTime = 0; this._totalReadingTime = 0;
// Event listener references for cleanup
this._boundKeydownHandler = null;
this._boundClickOutsideHandler = null;
Object.seal(this); Object.seal(this);
} }
@ -64,17 +68,33 @@ class StoryReader extends Module {
* @returns {Object} Compatibility score and details * @returns {Object} Compatibility score and details
*/ */
static getCompatibilityScore(content) { 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 storyCount = 0;
let hasMainStory = !!(content?.story?.title || content?.rawContent?.story?.title); let hasMainStory = !!(content?.story?.title);
let hasAdditionalStories = !!(content?.additionalStories?.length || content?.rawContent?.additionalStories?.length); let hasAdditionalStories = !!(content?.additionalStories?.length);
let hasTexts = !!(content?.texts?.length || content?.rawContent?.texts?.length); let hasTexts = !!(content?.texts?.length);
let hasSentences = !!(content?.sentences?.length || content?.rawContent?.sentences?.length); let hasSentences = !!(content?.sentences?.length);
if (hasMainStory) storyCount++; if (hasMainStory) storyCount++;
if (hasAdditionalStories) storyCount += (content?.additionalStories?.length || content?.rawContent?.additionalStories?.length); if (hasAdditionalStories) storyCount += content.additionalStories.length;
if (hasTexts) storyCount += (content?.texts?.length || content?.rawContent?.texts?.length); if (hasTexts) storyCount += content.texts.length;
if (hasSentences) storyCount++; if (hasSentences) storyCount++;
console.log('🎯 StoryReader: Story count =', storyCount, {
hasMainStory,
hasAdditionalStories,
hasTexts,
hasSentences
});
if (storyCount === 0) { if (storyCount === 0) {
return { return {
score: 0, score: 0,
@ -114,7 +134,7 @@ class StoryReader extends Module {
// Select initial story // Select initial story
this._selectStory(0); this._selectStory(0);
this._vocabulary = this._content?.vocabulary || this._content?.rawContent?.vocabulary || {}; this._vocabulary = this._content?.vocabulary || {};
// Set up event listeners // Set up event listeners
this._eventBus.on('game:pause', this._handlePause.bind(this), this.name); this._eventBus.on('game:pause', this._handlePause.bind(this), this.name);
@ -161,6 +181,16 @@ class StoryReader extends Module {
this._readingTimer = null; 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 // Clean up container
if (this._config.container) { if (this._config.container) {
this._config.container.innerHTML = ''; this._config.container.innerHTML = '';
@ -204,7 +234,7 @@ class StoryReader extends Module {
this._availableStories = []; this._availableStories = [];
// Check main story field // Check main story field
const mainStory = this._content.rawContent?.story || this._content.story; const mainStory = this._content.story;
if (mainStory && mainStory.title) { if (mainStory && mainStory.title) {
this._availableStories.push({ this._availableStories.push({
id: 'main', id: 'main',
@ -215,7 +245,7 @@ class StoryReader extends Module {
} }
// Check additional stories // Check additional stories
const additionalStories = this._content.rawContent?.additionalStories || this._content.additionalStories; const additionalStories = this._content.additionalStories;
if (additionalStories && Array.isArray(additionalStories)) { if (additionalStories && Array.isArray(additionalStories)) {
additionalStories.forEach((story, index) => { additionalStories.forEach((story, index) => {
if (story && story.title) { if (story && story.title) {
@ -230,7 +260,7 @@ class StoryReader extends Module {
} }
// Check texts and convert to stories // 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)) { if (texts && Array.isArray(texts)) {
texts.forEach((text, index) => { texts.forEach((text, index) => {
if (text && (text.title || text.original_language)) { if (text && (text.title || text.original_language)) {
@ -246,7 +276,7 @@ class StoryReader extends Module {
} }
// Check sentences and create story from them // 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) { if (sentences && Array.isArray(sentences) && sentences.length > 0 && this._availableStories.length === 0) {
const sentencesStory = this._convertSentencesToStory(sentences); const sentencesStory = this._convertSentencesToStory(sentences);
this._availableStories.push({ this._availableStories.push({
@ -280,7 +310,18 @@ class StoryReader extends Module {
} }
_convertTextToStory(text, index) { _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 { return {
title: text.title || `Text ${index + 1}`, title: text.title || `Text ${index + 1}`,
totalSentences: sentences.length, 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 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 sentences = [];
const maxSentences = Math.max(originalSentences.length, translationSentences.length); const maxSentences = Math.max(originalSentences.length, translationSentences.length);
@ -758,8 +805,8 @@ class StoryReader extends Module {
// TTS button in popup // TTS button in popup
document.getElementById('popup-tts-btn').addEventListener('click', () => this._speakWordFromPopup()); document.getElementById('popup-tts-btn').addEventListener('click', () => this._speakWordFromPopup());
// Keyboard shortcuts // Keyboard shortcuts - store handler reference for cleanup
document.addEventListener('keydown', (e) => { this._boundKeydownHandler = (e) => {
if (e.target.tagName === 'INPUT' || e.target.tagName === 'SELECT') return; if (e.target.tagName === 'INPUT' || e.target.tagName === 'SELECT') return;
switch (e.key) { switch (e.key) {
@ -780,14 +827,16 @@ class StoryReader extends Module {
this._playSentenceTTS(); this._playSentenceTTS();
break; break;
} }
}); };
document.addEventListener('keydown', this._boundKeydownHandler);
// Click outside to close popup // Click outside to close popup - store handler reference for cleanup
document.addEventListener('click', (e) => { this._boundClickOutsideHandler = (e) => {
if (!e.target.closest('.word-popup') && !e.target.closest('.clickable-word')) { if (!e.target.closest('.word-popup') && !e.target.closest('.clickable-word')) {
this._hideWordPopup(); this._hideWordPopup();
} }
}); };
document.addEventListener('click', this._boundClickOutsideHandler);
} }
_getCurrentSentenceData() { _getCurrentSentenceData() {
@ -938,7 +987,10 @@ class StoryReader extends Module {
} }
_hideWordPopup() { _hideWordPopup() {
document.getElementById('word-popup').style.display = 'none'; const popup = document.getElementById('word-popup');
if (popup) {
popup.style.display = 'none';
}
} }
_previousSentence() { _previousSentence() {