From 17713303bbc5b529d664e898e3b61da88740da06 Mon Sep 17 00:00:00 2001 From: a2nr Date: Sun, 6 Sep 2026 18:08:58 +0700 Subject: [PATCH] fix(review): address code review findings for dev to master merge - Optimize N+1 queries in get_all_students_progress via bulk fetch - Backfill legacy progress in migration 0005 and startup sync - Use arrayBuffer in hooks.server proxy for binary payload safety - Restrict postMessage targetOrigin from wildcard to window.location.origin - Harden iframe polling in TeacherVelxioEditor and reduce frequency - Improve markdown section extractor line-boundary detection - Clean up child subprocesses on exception in CLI runner - Add console.error in contentEditor catch blocks and parameterize repro script - Configure sequential vitest execution to optimize test runs --- app.py | 3 +- cli/runner.py | 20 ++++++ frontend/src/hooks.server.test.ts | 35 ++++++++++ frontend/src/hooks.server.ts | 2 +- .../lib/components/TeacherVelxioEditor.svelte | 21 +++--- .../src/lib/services/contentEditor.svelte.ts | 10 ++- .../lib/services/markdown/extractSections.ts | 34 ++++++++-- .../lib/services/markdown/markdown.test.ts | 7 ++ frontend/src/lib/services/velxio-bridge.ts | 3 +- frontend/src/lib/services/velxio-manager.ts | 7 +- frontend/vitest.config.ts | 4 +- .../0005_student_progress_composite.py | 14 ++-- scripts/repro_quiz_options.py | 13 ++-- services/lesson_registry.py | 66 +++++++++++++++++++ services/storage/postgres_backend.py | 19 ++++-- services/tests/test_lesson_registry.py | 14 ++++ 16 files changed, 229 insertions(+), 43 deletions(-) diff --git a/app.py b/app.py index f85fe7b..3c3c339 100644 --- a/app.py +++ b/app.py @@ -55,9 +55,10 @@ def create_app(): # ── Startup tasks ───────────────────────────────────────────────── # Sync metadata lesson ke PostgreSQL (auto-skip bila DB belum aktif). # Idempotent; kegagalan hanya di-log — tidak mematikan app. - from services.lesson_registry import maybe_sync_on_startup + from services.lesson_registry import maybe_backfill_on_startup, maybe_sync_on_startup maybe_sync_on_startup() + maybe_backfill_on_startup() return app diff --git a/cli/runner.py b/cli/runner.py index 3150bb3..6e45888 100644 --- a/cli/runner.py +++ b/cli/runner.py @@ -154,6 +154,7 @@ def execute_process( # Redirect to binary file directly (e.g. pg_dump) if stdout_file: + proc = None try: stdin_src = stdin_file if stdin_file else (subprocess.PIPE if stdin_data else None) proc = subprocess.Popen( @@ -181,11 +182,21 @@ def execute_process( stderr_thread.join() return proc.returncode, "", "".join(captured_stderr) except Exception as e: + if proc and proc.poll() is None: + try: + proc.terminate() + proc.wait(timeout=2) + except Exception: + try: + proc.kill() + except Exception: + pass return 1, "", str(e) # Capturing or streaming output stdout_lines: List[str] = [] stderr_lines: List[str] = [] + proc = None try: stdin_pipe = stdin_file if stdin_file else (subprocess.PIPE if stdin_data else subprocess.DEVNULL) @@ -236,6 +247,15 @@ def execute_process( return proc.returncode, "".join(stdout_lines), "".join(stderr_lines) except Exception as e: + if proc and proc.poll() is None: + try: + proc.terminate() + proc.wait(timeout=2) + except Exception: + try: + proc.kill() + except Exception: + pass if verbose: sys.stderr.write(f"Process execution error: {e}\n") return 1, "", str(e) diff --git a/frontend/src/hooks.server.test.ts b/frontend/src/hooks.server.test.ts index 0882709..cfefc10 100644 --- a/frontend/src/hooks.server.test.ts +++ b/frontend/src/hooks.server.test.ts @@ -53,4 +53,39 @@ describe('hooks.server handle', () => { expect(resolve).toHaveBeenCalledWith(mockEvent); expect(response).toBe(mockResponse); }); + + it('should read body as arrayBuffer when proxying POST requests', async () => { + const arrayBufferMock = vi.fn().mockResolvedValue(new Uint8Array([1, 2, 3]).buffer); + const mockEvent: any = { + request: { + method: 'POST', + headers: new Headers({ 'content-type': 'application/octet-stream' }), + arrayBuffer: arrayBufferMock + }, + url: new URL('https://sinau-c-dev.manakin-gentoo.ts.net/api/upload') + }; + + // Mock global fetch + const origFetch = global.fetch; + global.fetch = vi.fn().mockResolvedValue( + new Response(JSON.stringify({ success: true }), { + status: 200, + headers: { 'content-type': 'application/json' } + }) + ) as any; + + try { + const resolve = vi.fn(); + const response = await handle({ event: mockEvent, resolve }); + + expect(arrayBufferMock).toHaveBeenCalled(); + expect(global.fetch).toHaveBeenCalled(); + const fetchInit = (global.fetch as any).mock.calls[0][1]; + expect(fetchInit.body).toBeInstanceOf(ArrayBuffer); + expect(response.status).toBe(200); + } finally { + global.fetch = origFetch; + } + }); }); + diff --git a/frontend/src/hooks.server.ts b/frontend/src/hooks.server.ts index 554a293..7ee67f9 100644 --- a/frontend/src/hooks.server.ts +++ b/frontend/src/hooks.server.ts @@ -59,7 +59,7 @@ export const handle: Handle = async ({ event, resolve }) => { }; if (event.request.method !== 'GET' && event.request.method !== 'HEAD') { - init.body = await event.request.text(); + init.body = await event.request.arrayBuffer(); } const res = await fetch(backendUrl, init); diff --git a/frontend/src/lib/components/TeacherVelxioEditor.svelte b/frontend/src/lib/components/TeacherVelxioEditor.svelte index f7ad9a4..c783e5c 100644 --- a/frontend/src/lib/components/TeacherVelxioEditor.svelte +++ b/frontend/src/lib/components/TeacherVelxioEditor.svelte @@ -28,6 +28,7 @@ export function loadStateToSimulator(code: string, circuit: string) { if (!iframeRef?.contentWindow) return; const win = iframeRef.contentWindow; + const targetOrigin = typeof window !== 'undefined' ? window.location.origin : '*'; // Teacher mode: embed mode with components and editor fully editable win.postMessage( @@ -37,13 +38,13 @@ hideComponentPicker: false, lockComponents: false }, - '*' + targetOrigin ); if (circuit) { try { const circuitData = typeof circuit === 'string' ? JSON.parse(circuit) : circuit; - win.postMessage({ type: 'elemes:load_circuit', ...circuitData }, '*'); + win.postMessage({ type: 'elemes:load_circuit', ...circuitData }, targetOrigin); } catch (err) { console.error('[TeacherVelxioEditor] Error parsing circuit JSON:', err); } @@ -55,7 +56,7 @@ type: 'elemes:load_code', files: [{ name: 'sketch.ino', content: code }] }, - '*' + targetOrigin ); } } @@ -97,18 +98,20 @@ } function checkIframeReady() { + if (isReady) return; try { const win = iframeRef?.contentWindow as any; - if (!win || !win.document) return; - const root = win.document.getElementById('root'); - if (root && root.children.length > 0) { - if (!isReady) { + if (!win) return; + // Safely check origin before touching document + if (win.location?.origin === window.location.origin) { + const root = win.document?.getElementById('root'); + if (root && root.children?.length > 0) { isReady = true; loadStateToSimulator(initialCode, initialCircuit); } } } catch { - // Cross-origin fallback or not yet loaded + // Cross-origin fallback or still navigating } } @@ -128,7 +131,7 @@ return; } checkIframeReady(); - }, 600); + }, 1000); const pollTimeout = setTimeout(() => { clearInterval(poll); diff --git a/frontend/src/lib/services/contentEditor.svelte.ts b/frontend/src/lib/services/contentEditor.svelte.ts index 4b87a42..47ebdb0 100644 --- a/frontend/src/lib/services/contentEditor.svelte.ts +++ b/frontend/src/lib/services/contentEditor.svelte.ts @@ -65,7 +65,8 @@ export class ContentEditorManager { } else { this.showMessage('error', res.message || 'Gagal memuat file'); } - } catch { + } catch (err) { + console.error('[contentEditor] loadDraft error:', err); this.showMessage('error', 'Gagal terhubung ke server'); } finally { this.loading = false; @@ -107,6 +108,7 @@ export class ContentEditorManager { this.previewError = res.message || 'Preview gagal'; } } catch (e: any) { + console.error('[contentEditor] runPreview error:', e); this.previewError = e.message || 'Gagal merender preview'; } finally { this.previewLoading = false; @@ -125,7 +127,8 @@ export class ContentEditorManager { } else { this.showMessage('error', res.message || 'Gagal menyimpan draft'); } - } catch { + } catch (err) { + console.error('[contentEditor] handleSave error:', err); this.showMessage('error', 'Gagal terhubung ke server'); } finally { this.saving = false; @@ -166,7 +169,8 @@ export class ContentEditorManager { this.showMessage('error', res.message || 'Gagal mempublikasikan'); } } - } catch { + } catch (err) { + console.error('[contentEditor] handlePublish error:', err); this.showMessage('error', 'Gagal terhubung ke server'); } finally { this.publishing = false; diff --git a/frontend/src/lib/services/markdown/extractSections.ts b/frontend/src/lib/services/markdown/extractSections.ts index 916b132..a703120 100644 --- a/frontend/src/lib/services/markdown/extractSections.ts +++ b/frontend/src/lib/services/markdown/extractSections.ts @@ -1,3 +1,24 @@ +/** + * Extract content between startMarker and endMarker, returning the extracted content + * and the remaining content with the markers and extracted block removed. + */ +function findMarkerIndex(content: string, marker: string, fromIndex = 0): number { + let idx = content.indexOf(marker, fromIndex); + let fallbackIdx = -1; + while (idx !== -1) { + const prevNewline = content.lastIndexOf('\n', idx - 1); + const linePrefix = content.slice(prevNewline + 1, idx); + if (/^[ \t]*$/.test(linePrefix)) { + return idx; + } + if (fallbackIdx === -1) { + fallbackIdx = idx; + } + idx = content.indexOf(marker, idx + marker.length); + } + return fallbackIdx; +} + /** * Extract content between startMarker and endMarker, returning the extracted content * and the remaining content with the markers and extracted block removed. @@ -11,10 +32,13 @@ export function extractSection( return ['', content]; } - const startIdx = content.indexOf(startMarker); - const endIdx = content.indexOf(endMarker); + const startIdx = findMarkerIndex(content, startMarker); + if (startIdx === -1) { + return ['', content]; + } - if (startIdx === -1 || endIdx === -1 || endIdx <= startIdx) { + const endIdx = findMarkerIndex(content, endMarker, startIdx + startMarker.length); + if (endIdx === -1 || endIdx <= startIdx) { return ['', content]; } @@ -35,8 +59,8 @@ export function upsertSection( newInnerContent: string ): string { const trimmedInner = newInnerContent.trim(); - const startIdx = content.indexOf(startMarker); - const endIdx = content.indexOf(endMarker); + const startIdx = findMarkerIndex(content, startMarker); + const endIdx = startIdx !== -1 ? findMarkerIndex(content, endMarker, startIdx + startMarker.length) : -1; if (startIdx !== -1 && endIdx !== -1 && endIdx > startIdx) { return ( diff --git a/frontend/src/lib/services/markdown/markdown.test.ts b/frontend/src/lib/services/markdown/markdown.test.ts index f286c93..b80bbb3 100644 --- a/frontend/src/lib/services/markdown/markdown.test.ts +++ b/frontend/src/lib/services/markdown/markdown.test.ts @@ -21,6 +21,13 @@ describe('Markdown Renderer & Extractor Tests', () => { expect(extracted).toBe(''); expect(remaining).toBe(input); }); + + it('should prefer line-boundary markers when marker name is mentioned in prose', () => { + const input = 'This tutorial explains ---EXPECTED_OUTPUT--- in body.\n---EXPECTED_OUTPUT---\nactual content\n---END_EXPECTED_OUTPUT---\nAfter'; + const [extracted, remaining] = extractSection(input, '---EXPECTED_OUTPUT---', '---END_EXPECTED_OUTPUT---'); + expect(extracted).toBe('actual content'); + expect(remaining).toBe('This tutorial explains ---EXPECTED_OUTPUT--- in body.\n\nAfter'); + }); }); describe('upsertSection', () => { diff --git a/frontend/src/lib/services/velxio-bridge.ts b/frontend/src/lib/services/velxio-bridge.ts index 92e7659..e44522d 100644 --- a/frontend/src/lib/services/velxio-bridge.ts +++ b/frontend/src/lib/services/velxio-bridge.ts @@ -186,7 +186,8 @@ export class VelxioBridge { // === Internal === private send(type: string, payload: Record = {}) { - this.iframe.contentWindow?.postMessage({ type, ...payload }, '*'); + const targetOrigin = typeof window !== 'undefined' ? window.location.origin : '*'; + this.iframe.contentWindow?.postMessage({ type, ...payload }, targetOrigin); } private request(sendType: string, expectType: string): Promise { diff --git a/frontend/src/lib/services/velxio-manager.ts b/frontend/src/lib/services/velxio-manager.ts index 96882e7..357e598 100644 --- a/frontend/src/lib/services/velxio-manager.ts +++ b/frontend/src/lib/services/velxio-manager.ts @@ -204,12 +204,13 @@ export function initVelxioBridge( onReady(new VelxioBridge(iframe)); // Just provide dummy or initialize if (data) { + const targetOrigin = typeof window !== 'undefined' ? window.location.origin : '*'; win.postMessage({ type: 'elemes:set_embed_mode', hideAuth: true, hideComponentPicker: true, lockComponents: true - }, '*'); + }, targetOrigin); const restoredCircuit = resolveRestoredContent(arduinoCircuitKey, data.velxio_circuit, isTeacher); if (restoredCircuit.content) { @@ -218,7 +219,7 @@ export function initVelxioBridge( console.log('[Velxio Fallback] Restoring circuit from draft'); } const circuitData = JSON.parse(restoredCircuit.content); - win.postMessage({ type: 'elemes:load_circuit', ...circuitData }, '*'); + win.postMessage({ type: 'elemes:load_circuit', ...circuitData }, targetOrigin); } catch {} } @@ -227,7 +228,7 @@ export function initVelxioBridge( if (restoredCode.shouldRestore) { console.log('[Velxio Fallback] Restoring code from draft'); } - win.postMessage({ type: 'elemes:load_code', files: [{ name: 'sketch.ino', content: restoredCode.content }] }, '*'); + win.postMessage({ type: 'elemes:load_code', files: [{ name: 'sketch.ino', content: restoredCode.content }] }, targetOrigin); } } } catch { /* cross-origin or not ready yet */ } diff --git a/frontend/vitest.config.ts b/frontend/vitest.config.ts index 2332903..ccc4365 100644 --- a/frontend/vitest.config.ts +++ b/frontend/vitest.config.ts @@ -14,7 +14,9 @@ export default defineConfig({ }, test: { environment: 'node', - include: ['src/**/*.test.ts'] + include: ['src/**/*.test.ts'], + testTimeout: 15000, + fileParallelism: false } }); diff --git a/migrations/versions/0005_student_progress_composite.py b/migrations/versions/0005_student_progress_composite.py index 1c25325..adffac3 100644 --- a/migrations/versions/0005_student_progress_composite.py +++ b/migrations/versions/0005_student_progress_composite.py @@ -57,22 +57,18 @@ def upgrade() -> None: "state IN ('not_started','in_progress','completed','scored','done')", ) - # Backfill: completed → exercise lulus; scored → ambil skor kuis lama. + # Backfill: completed → exercise lulus & composite 100%; scored → ambil skor kuis lama & hitung composite. op.execute( - "UPDATE student_progress SET exercise_passed = true " + "UPDATE student_progress SET exercise_passed = true, composite_percent = 100.0, state = 'done' " "WHERE state = 'completed'" ) op.execute( "UPDATE student_progress " - "SET quiz_score_earned = score_earned, quiz_score_total = score_total " + "SET quiz_score_earned = score_earned, quiz_score_total = score_total, " + " composite_percent = CASE WHEN score_total > 0 THEN ROUND((score_earned::numeric / score_total) * 100, 2) ELSE NULL END, " + " state = CASE WHEN score_total > 0 AND (score_earned::numeric / score_total) * 100 >= 75.0 THEN 'done' ELSE 'in_progress' END " "WHERE state = 'scored'" ) - # Legacy 'completed'/'scored' belum punya composite → turunkan ke - # 'in_progress' agar backend menghitung ulang jadi 'done'/'in_progress'. - op.execute( - "UPDATE student_progress SET state = 'in_progress' " - "WHERE state IN ('completed', 'scored')" - ) def downgrade() -> None: diff --git a/scripts/repro_quiz_options.py b/scripts/repro_quiz_options.py index a9e4d2f..205c5cd 100644 --- a/scripts/repro_quiz_options.py +++ b/scripts/repro_quiz_options.py @@ -7,13 +7,16 @@ Reproduce quiz option-display bug via Playwright. panjang options dari server quiz_data (cocokkan by question text/prose) - Log ke /tmp/repro_quiz.log """ -import sys, json, urllib.request +import json +import os +import sys +import urllib.request from pathlib import Path -BASE = "https://sinau-c-dev.manakin-gentoo.ts.net" -TOKEN = "141214" -SLUG = "quiz_test" -LOG = "/tmp/repro_quiz.log" +BASE = os.environ.get("ELEMES_TEST_BASE_URL", "https://sinau-c-dev.manakin-gentoo.ts.net") +TOKEN = os.environ.get("ELEMES_TEST_TOKEN", "141214") +SLUG = os.environ.get("ELEMES_TEST_SLUG", "quiz_test") +LOG = os.environ.get("ELEMES_TEST_LOG", "/tmp/repro_quiz.log") def log(msg): with open(LOG, "a") as f: diff --git a/services/lesson_registry.py b/services/lesson_registry.py index ac86744..b380a34 100644 --- a/services/lesson_registry.py +++ b/services/lesson_registry.py @@ -97,3 +97,69 @@ def maybe_sync_on_startup(): db.close() except Exception as exc: # noqa: BLE001 — startup tidak boleh crash karena DB logger.warning("Sync lesson registry gagal (akan dicoba lagi nanti): %s", exc) + + +def backfill_legacy_progress(db: Session) -> int: + """Hitung ulang composite score & state untuk record progress yang state='in_progress' + tetapi sudah memiliki data kelulusan (exercise_passed atau quiz_score_earned). + Ini memastikan siswa yang datanya dimigrasi dari schema lama tidak kehilangan + status 'done' / skor kelulusan di UI. + """ + from sqlalchemy import select + from config import LESSON_EXERCISE_WEIGHT, LESSON_QUIZ_WEIGHT, LESSON_DONE_MIN_PERCENT + from services.models import Lesson, StudentProgress + from services import repositories as repo + + stmt = select(StudentProgress).where( + StudentProgress.state == "in_progress", + (StudentProgress.exercise_passed.is_(True)) | (StudentProgress.quiz_score_earned.is_not(None)), + ) + records = list(db.scalars(stmt)) + if not records: + return 0 + + recomputed = 0 + lessons_cache = {l.id: l for l in db.scalars(select(Lesson))} + + for p in records: + lesson = lessons_cache.get(p.lesson_id) + if not lesson: + continue + try: + has_exercise, has_quiz = lesson_service.get_lesson_components(lesson.slug) + except Exception: + has_exercise, has_quiz = True, False + + repo.recompute_progress( + db, + user_id=p.user_id, + lesson_id=p.lesson_id, + has_exercise=has_exercise, + has_quiz=has_quiz, + exercise_weight=LESSON_EXERCISE_WEIGHT, + quiz_weight=LESSON_QUIZ_WEIGHT, + done_min_percent=LESSON_DONE_MIN_PERCENT, + ) + recomputed += 1 + + db.commit() + return recomputed + + +def maybe_backfill_on_startup(): + """Auto-backfill progress state saat app init jika ada legacy in_progress records.""" + from services.database import SessionLocal + + if SessionLocal is None: + return + try: + db = SessionLocal() + try: + count = backfill_legacy_progress(db) + if count > 0: + logger.info("Backfill legacy progress selesai: %d record dihitung ulang", count) + finally: + db.close() + except Exception as exc: # noqa: BLE001 + logger.warning("Backfill legacy progress gagal (akan dicoba lagi nanti): %s", exc) + diff --git a/services/storage/postgres_backend.py b/services/storage/postgres_backend.py index 29b4db8..675447a 100644 --- a/services/storage/postgres_backend.py +++ b/services/storage/postgres_backend.py @@ -11,7 +11,7 @@ from sqlalchemy import select from services import repositories as repo from services.database import SessionLocal -from services.models import User +from services.models import QuizAttempt, StudentProgress, User from services.progress_status import format_progress_status, parse_progress_status from services.repositories import ( count_completed_lessons, @@ -164,12 +164,21 @@ def get_all_students_progress(all_lessons_func): .order_by(User.created_at, User.id) ) ) + # Bulk-fetch semua progress & quiz attempts dalam 2 query (hindari N+1). + all_progress = db.scalars(select(StudentProgress)).all() + progress_by_user = {} + for p in all_progress: + progress_by_user.setdefault(p.user_id, {})[p.lesson_id] = p + + all_attempts = db.scalars(select(QuizAttempt)).all() + attempts_by_user = {} + for a in all_attempts: + attempts_by_user.setdefault(a.user_id, {})[a.lesson_id] = a + students = [] for user in users: - rows = {p.lesson_id: p for p in list_progress_for_user(db, user_id=user.id)} - attempts = { - a.lesson_id: a for a in list_quiz_attempts_for_user(db, user_id=user.id) - } + rows = progress_by_user.get(user.id, {}) + attempts = attempts_by_user.get(user.id, {}) student = {'nama_siswa': user.display_name, 'id': user.id, 'role': user.role} for lesson in lessons: student[lesson.slug] = _status_to_string(rows.get(lesson.id)) diff --git a/services/tests/test_lesson_registry.py b/services/tests/test_lesson_registry.py index f62683b..22bcb01 100644 --- a/services/tests/test_lesson_registry.py +++ b/services/tests/test_lesson_registry.py @@ -172,3 +172,17 @@ def test_specs_union_root_and_sub_bab_no_db(tmp_path, monkeypatch): title_by_slug = {s: t for s, t, _ in specs} assert title_by_slug["shared"] == "Shared" assert title_by_slug["sub_only"] == "Sub Only" + + +@pytest.mark.skipif(not DB_REQUIRED, reason="butuh DATABASE_URL (PostgreSQL nyata)") +def test_backfill_legacy_progress(): + from services.database import SessionLocal + from services.lesson_registry import backfill_legacy_progress + + db = SessionLocal() + try: + count = backfill_legacy_progress(db) + assert isinstance(count, int) + finally: + db.close() +