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
This commit is contained in:
a2nr 2026-09-06 18:08:58 +07:00
parent 74423eee49
commit 17713303bb
16 changed files with 229 additions and 43 deletions

3
app.py
View File

@ -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

View File

@ -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)

View File

@ -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;
}
});
});

View File

@ -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);

View File

@ -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);

View File

@ -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;

View File

@ -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 (

View File

@ -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', () => {

View File

@ -186,7 +186,8 @@ export class VelxioBridge {
// === Internal ===
private send(type: string, payload: Record<string, any> = {}) {
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<any | null> {

View File

@ -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 */ }

View File

@ -14,7 +14,9 @@ export default defineConfig({
},
test: {
environment: 'node',
include: ['src/**/*.test.ts']
include: ['src/**/*.test.ts'],
testTimeout: 15000,
fileParallelism: false
}
});

View File

@ -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:

View File

@ -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:

View File

@ -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)

View File

@ -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))

View File

@ -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()