ADFA-4739: Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action - #24
ADFA-4739: Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action#24alexmmiller wants to merge 2 commits into
Conversation
…739) Adds the code that loads converted Kotlin website content (build_nav.py, populate_db.py, media insertion) and kotlin-stdlib/-reflect/-test JSON content (sync_kdoc_json_to_db.py) into documentation.db, the local e2e test script for that pipeline, and the CI workflow that runs it end-to-end against a Drive-hosted copy of the database. Split out of the larger Kotlin-docs pipeline PR (#21) so the DB-manipulation side (this ticket) can be reviewed separately from producing the raw JSON data for the Kotlin website (ADFA-5039, PR #23). This PR depends on ADFA-5039 merging first - populate_db.py, build_nav.py, and find_missing_assets.py all import md_to_json.py, which isn't included here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
Review scoped to the files that are NEW in this PR. I deliberately skipped populate_db.py, build_nav.py, insert_optimized_media.py, sync_kdoc_json_to_db.py and build-kotlin-docs.yaml — I verified by blob hash that those are byte-identical to #21, where I've already left 11 findings. Everything below is specific to this PR.
Blocking (2):
CLAUDE.mdmakes a factual claim abouttemplateIdthat the same commit falsifies — and reasons from it to a scope conclusion.nav.pebemits anav-hiddenclass that nothing consumes, so entries meant to be hidden render visible.
Non-blocking (3) — real, but lower severity or arguably intentional; flagged for your judgment rather than as merge blockers.
Context worth stating plainly: this PR is a clean split from #21, and the split itself is well executed — README.md correctly documents the ADFA-5039 dependency, and I checked that this PR is not shipping a stale pre-fix version of anything. A number of things I went looking for came back clean: build-stdlib-json-docs.sh's restore trap genuinely fires on both failure and SIGINT (verified with timeout -s INT) and is registered before the file swap, so there's no race window; optimize_media.py handles pngquant failures and animated images with explicit fallback-and-log rather than silent data loss, and returns a non-zero exit on errors; docs.css/sidebar.js/tabs.js/nav.peb agree on breakpoints and class names; and the README's documented CLI invocations match the argparse definitions exactly.
I also considered and did not file the page.peb "lone <tab>" workaround here — the root cause is md_to_json.py's tag regex, which isn't in this PR, and I've already filed it as blocking on #23.
| scope for this repo. (A repo-wide search for `templateId`, `Templates`, `Bookshelf`, | ||
| `BookCategories`, or `PUCC` turns up zero matches outside `WebServer.kt` itself, which is | ||
| consistent with that division of responsibility.) Concretely, relative to the schema above: |
There was a problem hiding this comment.
Blocking — this claim is false as of the commit that adds it, and it's load-bearing.
The parenthetical asserts a repo-wide search for templateId "turns up zero matches outside WebServer.kt", and the surrounding sentence uses that to conclude the concern "is not a gap to fill; it's out of scope for this repo."
But this same commit adds populate_db.py, which references templateId at lines 87, 98, 145, 261, 266, 276 and 286 — including the actual insert:
INSERT INTO Content (path, languageID, content, contentTypeID, templateId) VALUES (...)and insert_optimized_media.py, which filters on it at lines 214-215 and 256 (... WHERE ... AND templateId != 0). sync_kdoc_json_to_db.py:13 mentions it too.
So the grep this sentence invites the reader to trust returns numerous in-repo hits the moment this PR lands.
What makes this worth blocking rather than a doc nit: CLAUDE.md exists to orient future readers and agents, and this isn't a stale aside — the false premise is used to justify a scope boundary. An agent reading this will conclude the repo doesn't touch templateId and may "helpfully" strip it from exactly the INSERT that needs it.
Suggest either dropping the parenthetical or narrowing it to the Templates/Bookshelf/BookCategories/PUCC names, which I did not find outside WebServer.kt.
There was a problem hiding this comment.
Fixed in 66da59d - dropped templateId from the "zero matches outside WebServer.kt" list, keeping only Templates, Bookshelf, BookCategories, PUCC (the names that actually are absent), and added a clarifying note that populate_db.py/insert_optimized_media.py do read/write templateId directly since it's a plain column on Content they populate.
| </nav> | ||
|
|
||
| {% macro renderNavNode(node) %} | ||
| <li class="nav-item{% if node.hidden %} nav-hidden{% endif %}"> |
There was a problem hiding this comment.
Blocking — the nav-hidden class is inert, so hidden nav entries render fully visible.
This line emits nav-hidden for nodes the tree marks hidden, and the header comment at line 10 documents hidden as "true for entries Writerside hides from the primary tree." But nothing anywhere consumes the class. Verified:
grep -c 'nav-hidden' assets/docs.css -> 0
grep -n 'hidden' assets/sidebar.js -> (no matches)
grep -c 'nav-hidden' templates/nav.html -> 29
I read docs.css in full (227 lines): the only display: none rules are for .nav-item > .nav-subtree, .nav-toggle and .nav-backdrop — none for .nav-hidden. And page.peb links only /assets/docs.css, so there's no other stylesheet that could supply the rule, and nothing injects CSS elsewhere.
Concrete effect: the committed nav.html already carries 29 nav-hidden entries — the Kotlin tour steps, e.g. "Hello world" — and every one of them shows in the sidebar identically to a normal entry. Writerside hides those individual tour steps from the primary nav by design, so the shipped sidebar gets cluttered with dozens of entries that were never meant to be listed.
One .nav-hidden { display: none; } rule in docs.css closes it. Worth deciding deliberately though — if these should actually be visible, the class and its comment are the thing to remove.
There was a problem hiding this comment.
Fixed in 66da59d - added .nav-hidden { display: none; } to assets/docs.css, matching the documented intent ("entries Writerside hides from the primary tree"). Went with hiding them rather than removing the class.
| design plus a flowchart image; there is no code here yet. The actual implementation (the Dokka | ||
| `JsonRenderer`/`ModelMapper`/`LinkPostProcessor` plugin, its test suite, and the | ||
| `kotlin-stdlib-docs` build scripts) exists only on the unmerged branch **`fix/ADFA-4514`**. That | ||
| branch's diff against `main` also shows it removing recent `docdb-studio` work and all of | ||
| `scripts/pdfjs/` — almost certainly because the branch was cut before those were added and hasn't | ||
| been rebased, not because it intends to delete them. **Flagged: rebase `fix/ADFA-4514` onto |
There was a problem hiding this comment.
Non-blocking — this section describes a branch state that is already out of date.
The text says Dokka-plugin-kdoc2json/ on main is "just a README.md describing the intended design plus a flowchart image; there is no code here yet", that the implementation "exists only on the unmerged branch fix/ADFA-4514", and flags "rebase fix/ADFA-4514 onto current main before merging". Line 183 repeats the rebase item in the decisions log.
fix/ADFA-4514 is already merged — 4c6b8aef ("Merge pull request #18 from appdevforall/fix/ADFA-4514") is on main, and git ls-tree -r main shows JsonOutputPlugin.kt, JsonRenderer.kt, ModelMapper.kt, LinkPostProcessor.kt, the test suite and the kotlin-stdlib-docs build scripts all present on main today.
So a reader is told a whole plugin implementation is missing from main and that a rebase is still outstanding, when both are resolved.
I've marked this non-blocking because it's a point-in-time note that was presumably true when drafted, and unlike the templateId claim above nothing reasons from it. But it's the kind of staleness that a repo-orientation doc is specifically supposed to avoid, and it'll mislead the next agent that reads it.
There was a problem hiding this comment.
Fixed in 66da59d - rewrote the Dokka-plugin-kdoc2json/ bullet to describe it as merged (fix/ADFA-4514, 4c6b8aef) with the actual files present on main, and removed the now-resolved rebase item from the decisions log (was line 183).
| - `pip install markdown-it-py Pillow scour brotli` | ||
| - `cairosvg` (only needed if an optimized SVG exceeds `--svg-rasterize-threshold`): `pip install cairosvg` |
There was a problem hiding this comment.
Non-blocking — bare pip install / python3, against the repo's own established pattern.
This is the same objection I left as blocking on #23's review_build_json.sh, so flagging it here for consistency rather than re-litigating it.
Our convention (~/.claude/CLAUDE.md) is explicit: "Always use uv for Python project management. Never use pip, pip3, or the uv pip compatibility shim", and "never invoke python/python3 directly outside a uv run context."
In this PR that shows up in several places:
- these two lines (
pip install markdown-it-py Pillow scour brotli,pip install cairosvg) - README lines 53, 56, 59, 72, 90, 109, 115 — every usage example is
python3 <script>.py ... optimize_media.py:47-48(docstring) and:302(a runtimeRuntimeErrortelling the user topip install cairosvg)run_e2e_pipeline_test.shlines 100, 105, 125, 141, 145, 175 — six barepython3invocations
This isn't only a global-preference thing: docdb-studio/ and check-tools/ in this same repo each ship a pyproject.toml + uv.lock and document uv sync / uv run, and docdb-studio/CLAUDE.md says "Dependencies are managed with uv. Always use uv run."
Related and independently worth fixing: scour and cairosvg are not in requirements.txt (unchanged by this PR), so they exist only in these prose instructions.
I've left this non-blocking because these are developer-facing helper scripts and you may have deliberately kept them dependency-light — but if so, that's worth a line in the README saying so.
There was a problem hiding this comment.
Non-blocking, addressed in 66da59d rather than just documented as intentional - converted every pip install/bare python3 reference (README usage examples, optimize_media.py's docstring + RuntimeError message, and all 6 invocations in run_e2e_pipeline_test.sh) to uv run --with-requirements <repo-root>/requirements.txt, matching the review_build_json.sh fix on #23. Also added scour/cairosvg to requirements.txt since they were only ever in prose.
| except Exception as exc: # noqa: BLE001 - surface which file broke, keep auditing the rest | ||
| print(f"error scanning {md_path}: {exc}", file=sys.stderr) |
There was a problem hiding this comment.
Non-blocking — per-file scan failures are swallowed and the script still exits 0.
The except catches everything from converter.convert_file(...), prints to stderr, and continues — but nothing counts the failures, and main() has no sys.exit(1) path except the directory-validation check at line 82. So if every file in the corpus raised, this still exits 0 and prints a report claiming nothing is broken.
That matters because of how the script is positioned: it's step 1/5 in run_e2e_pipeline_test.sh:100, and the README describes it as the pre-flight gate where you "fix anything broken in the source before converting it." set -euo pipefail in the shell script can't help — exit 0 is exit 0.
Same shape I flagged as blocking on populate_db.py:483 (#21) and md_to_json.py:607 (#23). I've scored it lower here because this tool only writes an advisory markdown report and doesn't mutate the database, so a bad run misleads rather than corrupts.
Counting failures and exiting non-zero — or at minimum printing a N files failed to scan line into the report itself — would make a broken run distinguishable from a clean one.
There was a problem hiding this comment.
Fixed in 66da59d - added a failure counter and --allow-failures flag mirroring md_to_json.py's pattern from #23: sys.exit(1) if any file failed to scan (unless the flag is passed), and a "N file(s) failed to scan" line (with an explicit "this report is incomplete" marker when non-zero) in the emitted report itself. Covered by new tests in tests/test_find_missing_assets.py, which stub the not-yet-merged md_to_json import via PYTHONPATH since that module isn't on this branch until #23 lands.
Blocking: CLAUDE.md's templateId "out of scope" claim was falsified by this same PR (populate_db.py/insert_optimized_media.py both read/write it) - narrowed the claim to the names that are actually absent. The nav-hidden class nav.peb emits was inert (no consuming CSS rule), rendering Writerside-hidden nav entries (e.g. individual tour steps) visible - added the missing docs.css rule. Non-blocking: rewrote the Dokka-plugin-kdoc2json bullet (and decisions log) to reflect that fix/ADFA-4514 is merged, rather than describing it as an outstanding rebase. find_missing_assets.py swallowed per-file scan failures and always exited 0, so a totally broken corpus still looked clean - added a failure counter, --allow-failures flag, and a report line, mirroring md_to_json.py's pattern from #23. Converted README.md, optimize_media.py, and run_e2e_pipeline_test.sh from bare pip/python3 to uv run --with-requirements, and added scour/cairosvg to requirements.txt, matching the repo's established uv convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed 66da59d addressing your review. Replied inline to each of the 5 line comments individually; summary here. Blocking (2)
Non-blocking (3)
TestsAdded Verified the |
Sent prematurely, apologies
Summary
An end-to-end pipeline for loading Kotlin website content and
kotlin-stdlib/-reflect/-test JSON docs into
documentation.db, plus aGitHub Action (
Build Kotlin Docs) that runs it against a Drive-hosted copyof the database.
Scope
Split out of #21
into two ticket-scoped PRs:
md_to_json.py).This PR depends on ADFA-5039 merging first.
populate_db.py,build_nav.py, andfind_missing_assets.pyallimportmd_to_json.pydirectly, and that file isn't included here. Please review this PR's
content, but hold off merging until #23 lands -
README.mdin particularwill very likely conflict with #23's own new
README.mdat the same path(both add a new file there) and need a quick manual merge to combine the two
once both are in.
Changes
ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/:build_nav.py(sidebar nav from
kr.tree),find_missing_assets.py(source QA),populate_db.py(inserts pages/nav/media intodocumentation.db,supports pruning via
--blacklisted-element-titles),optimize_media.py/
insert_optimized_media.py(media optimization + DB update),templates/,assets/.scripts/sync_kotlin_stdlib_docs/sync_kdoc_json_to_db.py- syncsDokka-generated kotlin-stdlib/-reflect/-test JSON into the database.
ProcessDocs/ProcessKotlinDocs/run_e2e_pipeline_test.sh- local e2e testof the whole pipeline against a scratch copy of the database.
.github/workflows/build-kotlin-docs.yaml- CI counterpart: pullsdocumentation.db(and Writerside'swebHelpImages.zip) from GoogleDrive, runs the same pipeline, uploads the result back.
Dokka-plugin-kdoc2json/scripts/kotlin/build-stdlib-json-docs.sh+build.gradle.ktstweak - builds the kdoc-to-json plugin and generatesthe stdlib JSON docs the sync script consumes.
CLAUDE.md- repo-level orientation doc (schema drift between this repo'stooling and the live production database, repo tour, decisions log).
Test plan
run_e2e_pipeline_test.shlocally against a scratch database.Build Kotlin Docswithdry_run: trueand confirm thesummary/blacklist-verification steps pass.
made during the split, aside from
README.mdbeing trimmed to drop themd_to_json.py-only section now covered by ADFA-5039).