Skip to content

Commit 6f90c24

Browse files
fix(review): reconcile deck write-root with panel read-root (Issue #1)
Generated decks never appeared in the Quiz/Flashcard panels. Root cause was TWO independent bugs, not the single "3-level layout" hypothesis in the handoff: A. Read-root unconfigured (dominant). The panels read decks from `review.directories`; the generator WRITES them under `content.base_path`. These are two separate config keys that must agree, but the live config sets only `content`, so `_web.py` handed `discover_directories([])` and the panels showed nothing regardless of disk contents. Fix: new `settings.resolve_study_dirs()` falls back to `content.base_path` when `review.directories` is unset. Wired into `_web.py` and `mcp/tools.py`. B. Single-level descent. The real vault is 3 levels deep (`base/<publisher>/<course>/{flashcards,quizzes}/`) but `discover_directories` only walked one child level (the publisher), never reaching course dirs. Fix: recursive descent to depth 4, stopping at the first content-bearing dir on each branch (a course is a leaf — never recurse into its deck subdirs, and skip empty deck dirs that `get_course_dir` eagerly mkdirs). Proven end-to-end (scripts/prove_gen_readpath.py + live server + Playwright): generated a real Ollama/gemma4 deck into CodeWithMosh/Complete_SQL_Mastery, confirmed files land where panels read, `/api/courses` lists it (9 fc / 6 quiz), `/api/cards` returns quality content, and the Flashcard panel renders card 1/9. Tests: +9 (4 loader, 4 resolver, 1 empty-dir guard). 87 passed, no regressions.
1 parent ddf5737 commit 6f90c24

7 files changed

Lines changed: 326 additions & 13 deletions

File tree

packages/studyloop/src/studyloop/cli/_web.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,13 @@ def web(port: int, lan: bool, password: str, ttyd_port: int, dev: bool) -> None:
3939

4040
import secrets
4141

42-
from studyloop.settings import load_raw_config
42+
from studyloop.settings import resolve_study_dirs
4343

4444
study_dirs: list[str] = []
4545
with contextlib.suppress(Exception):
46-
study_dirs = load_raw_config().get("review", {}).get("directories", [])
46+
# Falls back to content.base_path when review.directories is unset, so
47+
# the review panels discover decks the generator just wrote.
48+
study_dirs = resolve_study_dirs()
4749

4850
# Resolve credentials: always read username from config; password from CLI > config > auto
4951
username = "study"

packages/studyloop/src/studyloop/mcp/tools.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
from mcp.server.fastmcp.exceptions import ToolError
1616

1717
from studyloop.services.review import get_due, get_stats, record_review
18-
from studyloop.settings import load_raw_config, load_settings
18+
from studyloop.settings import load_settings
1919

2020
if TYPE_CHECKING:
2121
from pathlib import Path
@@ -46,8 +46,10 @@ def list_courses() -> dict[str, Any]:
4646
Each course has: name, card_count, quiz_count, due_count.
4747
"""
4848
from studyloop.services.review import list_course_summaries
49+
from studyloop.settings import resolve_study_dirs
4950

50-
study_dirs = load_raw_config().get("review", {}).get("directories", [])
51+
# Falls back to content.base_path when review.directories is unset.
52+
study_dirs = resolve_study_dirs()
5153

5254
return {"courses": list_course_summaries(study_dirs)}
5355

packages/studyloop/src/studyloop/review_loader.py

Lines changed: 70 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,19 @@ def load_quizzes(directory: Path) -> list[QuizQuestion]:
286286

287287
_GENERIC_DIR_NAMES = {"downloads", "content", "data", "files", "output", "generated"}
288288

289+
# Course-internal subdirs created by storage.get_course_dir. These are NEVER
290+
# courses themselves — a ``flashcards/`` dir contains ``*flashcards.json`` which
291+
# would otherwise match _has_review_content and surface as a bogus "flashcards"
292+
# course. Discovery must never recurse into or report these.
293+
_DECK_SUBDIRS = frozenset(
294+
{"flashcards", "quizzes", "audio", "chapters", "video", "slides"}
295+
)
296+
297+
# Depth guard for the recursive course search. The real vault is
298+
# ``base/<publisher>/<course>/`` (3 levels); 4 leaves headroom without risking
299+
# a runaway walk of an arbitrarily deep tree.
300+
_MAX_DISCOVERY_DEPTH = 4
301+
289302

290303
def _course_name(directory: Path) -> str:
291304
"""Derive a display name from a directory path.
@@ -302,36 +315,84 @@ def discover_directories(config_dirs: list[str] | None = None) -> list[tuple[str
302315
"""Discover course directories with flashcard/quiz content.
303316
304317
Returns list of (course_name, directory_path) tuples.
305-
Searches configured directories and their subdirectories.
318+
319+
The real study vault is a 3-level tree --
320+
``base/<publisher>/<course>/{flashcards,quizzes}/`` -- so a flat
321+
"base + one child level" scan never reached the course dirs that
322+
``content.job`` writes to (the publisher level is one hop too shallow).
323+
Discovery therefore walks down to ``_MAX_DISCOVERY_DEPTH``, stopping at the
324+
FIRST content-bearing directory on each branch (a course is a leaf: we must
325+
not descend into its ``flashcards/`` deck subdir and report that as a
326+
second, bogus course).
327+
328+
The legacy ``downloads/`` special-case is preserved for flat vaults.
306329
"""
307330
if not config_dirs:
308331
return []
309332

310333
courses: list[tuple[str, Path]] = []
334+
seen: set[Path] = set()
311335
for dir_str in config_dirs:
312336
base = Path(dir_str).expanduser()
313337
if not base.is_dir():
314338
continue
315339

316-
# Check if this directory itself has content
340+
# Check if this directory itself has content (flat single-course layout).
317341
if _has_review_content(base):
318-
courses.append((_course_name(base), base))
342+
_append_course(courses, seen, _course_name(base), base)
319343
continue
320344

321-
# Check subdirectories (e.g. downloads/flashcards/)
345+
# Legacy: a downloads/ holding the decks directly under the root.
322346
downloads = base / "downloads"
323347
if downloads.is_dir() and _has_review_content(downloads):
324-
courses.append((_course_name(base), downloads))
348+
_append_course(courses, seen, _course_name(base), downloads)
325349
continue
326350

327-
# Check immediate children
328-
for child in sorted(base.iterdir()):
329-
if child.is_dir() and _has_review_content(child):
330-
courses.append((_course_name(child), child))
351+
# Otherwise descend the publisher/course tree.
352+
_discover_recursive(base, courses, seen, depth=0)
331353

332354
return courses
333355

334356

357+
def _discover_recursive(
358+
directory: Path,
359+
courses: list[tuple[str, Path]],
360+
seen: set[Path],
361+
depth: int,
362+
) -> None:
363+
"""Walk ``directory``'s children, collecting content-bearing course dirs.
364+
365+
A content-bearing directory is treated as a leaf course -- recursion stops
366+
there so its ``flashcards/`` / ``quizzes/`` deck subdirs are never reported
367+
as their own courses. Deck subdirs and dot-dirs are skipped entirely.
368+
"""
369+
if depth >= _MAX_DISCOVERY_DEPTH:
370+
return
371+
for child in sorted(directory.iterdir()):
372+
if not child.is_dir() or child.name.startswith("."):
373+
continue
374+
if child.name in _DECK_SUBDIRS:
375+
continue
376+
if _has_review_content(child):
377+
_append_course(courses, seen, _course_name(child), child)
378+
continue
379+
_discover_recursive(child, courses, seen, depth + 1)
380+
381+
382+
def _append_course(
383+
courses: list[tuple[str, Path]],
384+
seen: set[Path],
385+
name: str,
386+
path: Path,
387+
) -> None:
388+
"""Append (name, path) once, de-duplicating on the resolved path."""
389+
resolved = path.resolve()
390+
if resolved in seen:
391+
return
392+
seen.add(resolved)
393+
courses.append((name, path))
394+
395+
335396
def _has_review_content(directory: Path) -> bool:
336397
"""Check if a directory has flashcard or quiz JSON files."""
337398
fc_dir = directory / "flashcards"

packages/studyloop/src/studyloop/settings.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,32 @@ def load_raw_config() -> dict[str, Any]:
347347
return loaded
348348

349349

350+
def resolve_study_dirs() -> list[str]:
351+
"""Resolve the directories the review panels scan for decks.
352+
353+
The web/MCP layers read decks from ``review.directories``; the content
354+
generator WRITES them under ``content.base_path``. These are two separate
355+
config keys that MUST agree, but a typical config sets only ``content``
356+
(the generator's root) and omits ``review`` entirely — so the panels were
357+
handed ``[]`` and showed nothing, even though decks were on disk.
358+
359+
Resolution order:
360+
1. ``review.directories`` when explicitly set (verbatim — power users
361+
may point the panels at extra roots).
362+
2. Fallback to ``content.base_path`` (the generator's write root), so a
363+
freshly generated deck is discoverable with zero extra config.
364+
365+
Always returns at least one entry (the default ``content.base_path`` when
366+
nothing is configured) so the panels have a root to scan on a fresh install.
367+
"""
368+
raw = load_raw_config()
369+
review_dirs = raw.get("review", {}).get("directories") or []
370+
if review_dirs:
371+
return [str(d) for d in review_dirs]
372+
base_path = load_settings().content.base_path
373+
return [str(Path(base_path).expanduser())]
374+
375+
350376
def write_raw_config(data: dict[str, Any]) -> Path:
351377
"""Write raw YAML config to the active config path and return the path."""
352378
config_path = get_config_path()

packages/studyloop/tests/test_review_loader.py

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,80 @@ def test_nonexistent_directory(self) -> None:
154154

155155
assert discover_directories(["/nonexistent/path"]) == []
156156

157+
def test_finds_course_three_levels_deep(self, tmp_path: Path) -> None:
158+
# The real vault is base/<publisher>/<course>/flashcards/ — discovery
159+
# must descend past the publisher level to find the course.
160+
from studyloop.review_loader import discover_directories
161+
162+
course = tmp_path / "CodeWithMosh" / "Complete_SQL_Mastery"
163+
fc_dir = course / "flashcards"
164+
fc_dir.mkdir(parents=True)
165+
(fc_dir / "getting-started-flashcards.json").write_text(
166+
json.dumps({"title": "Getting Started", "cards": [{"front": "Q", "back": "A"}]})
167+
)
168+
169+
courses = discover_directories([str(tmp_path)])
170+
assert len(courses) == 1
171+
name, path = courses[0]
172+
assert name == "Complete_SQL_Mastery"
173+
assert path == course
174+
175+
def test_finds_multiple_courses_under_multiple_publishers(self, tmp_path: Path) -> None:
176+
from studyloop.review_loader import discover_directories
177+
178+
for pub, crs in [
179+
("CodeWithMosh", "Complete_SQL_Mastery"),
180+
("CodeWithMosh", "The_Ultimate_Git_Course"),
181+
("ArjanCodes", "The_Software_Designer_Mindset"),
182+
]:
183+
quiz_dir = tmp_path / pub / crs / "quizzes"
184+
quiz_dir.mkdir(parents=True)
185+
(quiz_dir / f"{crs}-quiz.json").write_text(
186+
json.dumps(
187+
{
188+
"title": crs,
189+
"questions": [
190+
{"question": "Q?", "answerOptions": [{"text": "A", "isCorrect": True}]}
191+
],
192+
}
193+
)
194+
)
195+
196+
courses = discover_directories([str(tmp_path)])
197+
names = sorted(n for n, _ in courses)
198+
assert names == [
199+
"Complete_SQL_Mastery",
200+
"The_Software_Designer_Mindset",
201+
"The_Ultimate_Git_Course",
202+
]
203+
204+
def test_does_not_descend_into_deck_subdirs(self, tmp_path: Path) -> None:
205+
# A content-bearing course is a leaf: its flashcards/ subdir must not
206+
# be reported as a separate course.
207+
from studyloop.review_loader import discover_directories
208+
209+
course = tmp_path / "Pub" / "Course"
210+
fc_dir = course / "flashcards"
211+
fc_dir.mkdir(parents=True)
212+
(fc_dir / "x-flashcards.json").write_text(
213+
json.dumps({"title": "X", "cards": [{"front": "Q", "back": "A"}]})
214+
)
215+
216+
courses = discover_directories([str(tmp_path)])
217+
assert len(courses) == 1
218+
assert courses[0][1] == course
219+
220+
def test_empty_deck_dirs_not_discovered(self, tmp_path: Path) -> None:
221+
# get_course_dir eagerly mkdirs flashcards/ + quizzes/; an course with
222+
# only empty deck dirs (no JSON yet) must NOT surface as ready content.
223+
from studyloop.review_loader import discover_directories
224+
225+
course = tmp_path / "Pub" / "EmptyCourse"
226+
(course / "flashcards").mkdir(parents=True)
227+
(course / "quizzes").mkdir(parents=True)
228+
229+
assert discover_directories([str(tmp_path)]) == []
230+
157231

158232
class TestFindContentDirs:
159233
def test_finds_subdirectories(self, tmp_path: Path) -> None:

packages/studyloop/tests/test_settings_custom.py

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,63 @@ def test_load_raw_config_reads_env_override(monkeypatch, tmp_path):
6666
assert load_raw_config() == {"browser": "firefox"}
6767

6868

69+
def test_resolve_study_dirs_uses_explicit_review_directories(monkeypatch, tmp_path):
70+
# When review.directories is set, it wins verbatim.
71+
from studyloop.settings import resolve_study_dirs
72+
73+
config_path = tmp_path / "config.yaml"
74+
config_path.write_text(
75+
yaml.dump(
76+
{
77+
"content": {"base_path": str(tmp_path / "study")},
78+
"review": {"directories": [str(tmp_path / "explicit")]},
79+
}
80+
)
81+
)
82+
monkeypatch.setenv("STUDYLOOP_CONFIG", str(config_path))
83+
84+
assert resolve_study_dirs() == [str(tmp_path / "explicit")]
85+
86+
87+
def test_resolve_study_dirs_falls_back_to_content_base_path(monkeypatch, tmp_path):
88+
# The real bug: review.directories unset → read root must default to the
89+
# write root (content.base_path) so generated decks are discoverable.
90+
from studyloop.settings import resolve_study_dirs
91+
92+
study = tmp_path / "Study"
93+
study.mkdir()
94+
config_path = tmp_path / "config.yaml"
95+
config_path.write_text(yaml.dump({"content": {"base_path": str(study)}}))
96+
monkeypatch.setenv("STUDYLOOP_CONFIG", str(config_path))
97+
98+
assert resolve_study_dirs() == [str(study)]
99+
100+
101+
def test_resolve_study_dirs_expands_user_in_fallback(monkeypatch, tmp_path):
102+
from studyloop.settings import resolve_study_dirs
103+
104+
config_path = tmp_path / "config.yaml"
105+
config_path.write_text(yaml.dump({"content": {"base_path": "~/Obsidian/Personal/Study"}}))
106+
monkeypatch.setenv("STUDYLOOP_CONFIG", str(config_path))
107+
108+
resolved = resolve_study_dirs()
109+
assert resolved == [str(Path.home() / "Obsidian" / "Personal" / "Study")]
110+
assert "~" not in resolved[0]
111+
112+
113+
def test_resolve_study_dirs_empty_when_nothing_configured(monkeypatch, tmp_path):
114+
from studyloop.settings import resolve_study_dirs
115+
116+
missing = tmp_path / "none.yaml"
117+
monkeypatch.setenv("STUDYLOOP_CONFIG", str(missing))
118+
119+
# No config at all → default content.base_path (~/study-materials) is used;
120+
# resolver always yields exactly one root (never empty), so panels have a
121+
# root to scan even on a fresh install.
122+
resolved = resolve_study_dirs()
123+
assert len(resolved) == 1
124+
125+
69126
def test_write_raw_config_creates_parent_and_round_trips(monkeypatch, tmp_path):
70127
from studyloop.settings import load_raw_config, write_raw_config
71128

0 commit comments

Comments
 (0)