feat(learning): ground guided lessons with durable citations - #110
Merged
Conversation
The packaged recursion trace requested a syntax language that is not loaded in the production highlighter, emitting a console warning on normal lesson navigation. Render the trace as an unclassified code block so its formatting remains intact without invoking an unavailable grammar. - Remove the unsupported text language marker from the recursion trace fence
Closing the mobile lesson-chat drawer removed the focused close control without returning keyboard focus to the control that opened it. Preserve the invoking button and restore focus after Svelte completes the drawer's DOM update. - Capture the mobile chat trigger when opening the drawer - Return focus after closing and cover the keyboard flow with a regression test
Regenerate the manifest-bound seed before accepting an existing Java API mirror and quarantine unseeded HTML outside the ingestion tree. This prevents stale flat Javadoc pages from recreating noncanonical citations.
Documentation mirrors now derive discovery behavior, field positions, and allowed seed types from canonical manifests instead of parallel script inventories. Existing mirrors are reconciled before they are accepted. - Share manifest parsing across Java, Bash, and Python - Preserve extensionless pages with bounded retry and timeout behavior - Quarantine Java API files absent from the canonical seed
…mmediately Learners can browse the complete curriculum, receive lesson-specific guidance from canonical sources, and see immediate stream progress. Invalid lesson requests now fail before retrieval or model inference. - Add source-scoped lessons across Java, JVM languages, Spring, and Quarkus - Make citation discovery deterministic and remove obsolete PDF anchoring - Reject false Javadoc anchors and stale lesson slugs - Restore focus and syntax highlighting across the expanded lesson UI
Navigation now keeps the active view, browser history, canonical URL, title, and social metadata aligned for direct visits and in-app route changes. - Share one route metadata owner between bootstrap and Svelte navigation - Normalize supported trailing-slash routes - Preserve back and forward navigation across Chat and Learn
Guided lesson catalogs and retrieval constraints now own detached immutable state, preventing callers from mutating cached metadata or source scopes. - Return deep lesson snapshots from the TOC provider - Defensively copy retrieval source constraints - Prove both boundaries reject external mutation
Guided lesson snapshots now have the copy constructor used by the TOC cache and response boundaries, keeping the defensive-ownership commit buildable.
Contributor
📝 WalkthroughWalkthroughThe PR expands guided learning with canonical documentation scopes, curated lesson content, and new JVM/framework lessons. It adds documentation seed discovery and selective fetching, refactors retrieval and citations, synchronizes SPA routes with SEO metadata, improves accessibility, and emits preparation status events before streaming work. ChangesFrontend navigation and presentation
Documentation ingestion and lessons
Guided learning and retrieval
Configuration and validation cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates guided learning to ground lessons in manifest-owned canonical documentation sources, emit citations earlier and deterministically, and improve both backend streaming UX (early “preparing” status) and frontend learning experience (SEO sync, accessibility, syntax highlighting, mobile focus).
Changes:
- Introduces manifest-governed documentation source scoping (including seed discovery) and projects guided lessons onto canonical
docSetconstraints. - Emits an immediate SSE
statuspreparation event for chat/guided streams and refactors retrieval/citation handling around context documents. - Improves Learn UX: route/metadata synchronization, guided lesson heading accessibility, mobile focus restoration, and expanded syntax highlighting for JVM/config languages.
Reviewed changes
Copilot reviewed 94 out of 94 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/williamcallahan/javachat/web/GuidedSseCitationEventTest.java | Updates guided SSE tests for context-document citations and revised error-stream assertions. |
| src/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerTest.java | Adds tests ensuring unknown/blank/missing slugs 404 before any guided work starts. |
| src/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerStreamingFailureTest.java | Adjusts controller failure tests to new citation API and lesson lookup behavior. |
| src/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerCuratedContentTest.java | Updates curated-content tests for revised GuidedLesson construction. |
| src/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerBackpressureOverflowTest.java | Adds preparation-status assertion and updates citation API usage. |
| src/test/java/com/williamcallahan/javachat/web/ChatControllerStreamingFailureTest.java | Adds StepVerifier test that preparation status emits before deferred work begins. |
| src/test/java/com/williamcallahan/javachat/util/JavadocTypeCanonicalizerTest.java | Expands tests for anchor/type canonicalization edge cases. |
| src/test/java/com/williamcallahan/javachat/service/RetrievalConstraintTest.java | Adds tests for immutable, normalized docSet handling in retrieval constraints. |
| src/test/java/com/williamcallahan/javachat/service/QdrantRetrievalConstraintBuilderTest.java | Extends Qdrant filter tests for docSet any-of keyword matching. |
| src/test/java/com/williamcallahan/javachat/service/GuidedTOCProviderTest.java | Tests manifest-projected guided lesson scoping and immutability snapshots. |
| src/test/java/com/williamcallahan/javachat/service/GuidedLessonCuratedContentTest.java | Adds TOC invariants test and adjusts curated lesson resource missing behavior. |
| src/main/resources/guided/lessons/variables-and-types.md | Refreshes lesson content with clearer structure, runnable steps, and checks. |
| src/main/resources/guided/lessons/testing-with-junit.md | Adds new guided lesson content for JUnit testing fundamentals. |
| src/main/resources/guided/lessons/strings.md | Refreshes string lessons with runnable examples, outputs, and exercises. |
| src/main/resources/guided/lessons/spring-boot-vs-quarkus.md | Adds new lesson comparing Spring Boot vs Quarkus with cited references. |
| src/main/resources/guided/lessons/scala-on-the-jvm.md | Adds new Scala JVM lesson (versioned) with references. |
| src/main/resources/guided/lessons/recursion.md | Refreshes recursion lesson with runnable examples and exercises. |
| src/main/resources/guided/lessons/records.md | Adds new lesson explaining Java records with examples and references. |
| src/main/resources/guided/lessons/packages-and-imports.md | Adds new lesson about packages/imports and compilation commands. |
| src/main/resources/guided/lessons/modules.md | Adds new lesson on JPMS modules and boundaries. |
| src/main/resources/guided/lessons/modern-java-pattern-matching.md | Adds new lesson on pattern matching, sealed types, and switch guards. |
| src/main/resources/guided/lessons/methods.md | Refreshes methods lesson with runnable examples and clearer structure. |
| src/main/resources/guided/lessons/loops.md | Refreshes loops lesson with runnable examples, outputs, and exercises. |
| src/main/resources/guided/lessons/lambdas-and-streams.md | Adds new lesson on lambdas/streams with deterministic examples. |
| src/main/resources/guided/lessons/kotlin-on-the-jvm.md | Adds new Kotlin JVM lesson (versioned) with references. |
| src/main/resources/guided/lessons/introduction-to-java.md | Refreshes intro lesson with runnable steps and comprehension checks. |
| src/main/resources/guided/lessons/interfaces-and-inheritance.md | Adds new lesson on interfaces/inheritance and polymorphism. |
| src/main/resources/guided/lessons/groovy-on-the-jvm.md | Adds new Groovy JVM lesson (versioned) with references. |
| src/main/resources/guided/lessons/generics.md | Adds new lesson on generics, variance, and erasure. |
| src/main/resources/guided/lessons/garbage-collection-and-memory.md | Adds new lesson on GC vs resource ownership and diagnosis. |
| src/main/resources/guided/lessons/file-io.md | Adds new lesson on file I/O, charset choice, and try-with-resources. |
| src/main/resources/guided/lessons/exceptions.md | Adds new lesson on exceptions, causes, and recovery boundaries. |
| src/main/resources/guided/lessons/enums-and-sealed-types.md | Adds new lesson on enums and sealed hierarchies. |
| src/main/resources/guided/lessons/conditionals.md | Refreshes conditionals lesson with runnable examples and boundary focus. |
| src/main/resources/guided/lessons/concurrency-and-virtual-threads.md | Adds new lesson on concurrency and virtual threads with references. |
| src/main/resources/guided/lessons/collections.md | Adds new lesson on core collection interfaces and contracts. |
| src/main/resources/guided/lessons/clojure-on-the-jvm.md | Adds new Clojure JVM lesson (versioned) with references. |
| src/main/resources/guided/lessons/classes-and-objects.md | Refreshes classes/objects lesson with clearer object independence example. |
| src/main/resources/guided/lessons/choosing-a-jvm-language.md | Adds new lesson comparing JVM languages and boundary costs. |
| src/main/resources/guided/lessons/arrays.md | Refreshes arrays lesson with runnable examples and indexing guidance. |
| src/main/resources/documentation-sources.manifest | Expands canonical documentation-source manifest schema with seed discovery + policies. |
| src/main/resources/documentation-seed-document-types.manifest | Adds canonical catalog of supported seed discovery document types. |
| src/main/resources/application.properties | Removes guided Think Java PDF path property. |
| src/main/java/com/williamcallahan/javachat/web/SseSupport.java | Adds response-preparation SSE status helper for early stream admission visibility. |
| src/main/java/com/williamcallahan/javachat/web/SseConstants.java | Adds stable status code + stage for “preparing” retrieval phase. |
| src/main/java/com/williamcallahan/javachat/web/ChatController.java | Emits preparation status immediately and defers work until after first event. |
| src/main/java/com/williamcallahan/javachat/util/JavadocMemberAnchorResolver.java | Rejects invocation-like tokens when canonicalizing declaration parameter anchors. |
| src/main/java/com/williamcallahan/javachat/service/RetrievalConstraint.java | Adds immutable, normalized docSet constraint support and official-docSet factory. |
| src/main/java/com/williamcallahan/javachat/service/QdrantRetrievalConstraintBuilder.java | Adds docSet any-of keyword filter and centralizes metadata field names. |
| src/main/java/com/williamcallahan/javachat/service/GuidedTOCProvider.java | Projects lessons onto manifest-owned source scopes and returns immutable snapshots. |
| src/main/java/com/williamcallahan/javachat/model/GuidedLesson.java | Adds technology, sourceReferences (write-only), docSet, and snapshot-copy ctor + validation. |
| src/main/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifest.java | Moves shared field rules into a shared validator and updates parsing accordingly. |
| src/main/java/com/williamcallahan/javachat/config/DocumentationSourceManifest.java | Parses/serializes expanded manifest schema; enforces unique docSet. |
| src/main/java/com/williamcallahan/javachat/config/DocumentationSeedDocumentTypeCatalog.java | Loads and validates manifest-owned seed document types for structured discovery. |
| src/main/java/com/williamcallahan/javachat/config/DocumentationManifestFieldRules.java | Adds shared boolean/unsigned-int parsing and HTTP/HTTPS URL validation helpers. |
| src/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.java | Extends documentation source records with seed discovery + policy fields. |
| src/main/java/com/williamcallahan/javachat/config/AppProperties.java | Removes guided-learning Think Java PDF configuration section. |
| scripts/testdata/documentation_seed/sitemap.xml | Adds sitemap test data for seed discovery scenarios. |
| scripts/testdata/documentation_seed/sitemap-line-injection.xml | Adds sitemap test data for injection/whitespace rejection. |
| scripts/testdata/documentation_seed/navigation.html | Adds HTML navigation test data for seed discovery scenarios. |
| scripts/test_java_api_fetch_projection.sh | Expands fetch projection tests to cover seed reconciliation/quarantine behavior. |
| scripts/documentation_seed.py | Adds deterministic seed builder for xml-sitemap and html-links discovery sources. |
| frontend/src/lib/validation/schemas.ts | Extends GuidedLesson schema with technology and docSet. |
| frontend/src/lib/utils/highlight.ts | Expands highlight.js language support for curriculum JVM/config languages. |
| frontend/src/lib/utils/highlight.test.ts | Adds tests ensuring supported lesson languages are highlighted (and text/properties are quiet). |
| frontend/src/lib/services/guided.ts | Updates citations fetch comment to reflect canonical official scope (not Think Java PDF). |
| frontend/src/lib/services/guided.test.ts | Adds TOC validation tests for projected lesson source-scope contract. |
| frontend/src/lib/components/LearnView.test.ts | Adds mobile focus restoration test for guided chat drawer close behavior. |
| frontend/src/lib/components/LearnView.svelte | Restores mobile focus on close using tick() and tracks trigger element. |
| frontend/src/lib/components/GuidedLessonHeader.test.ts | Adds accessibility test for a single H1 lesson title. |
| frontend/src/lib/components/GuidedLessonHeader.svelte | Changes lesson title heading from H2 to H1. |
| frontend/src/App.test.ts | Adds route/metadata synchronization tests and resets head metadata between runs. |
| frontend/src/App.svelte | Synchronizes view with path and browser history; centralizes metadata sync. |
| frontend/index.html | Replaces inline SEO script with module import of route metadata synchronization. |
| docs/pipeline-commands.md | Documents expanded manifest ownership + seed discovery and adds --doc-sets flag docs. |
| docs/api.md | Documents initial stream.preparing status event for chat and guided streaming endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:6e791410a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java (1)
285-308: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSurface partial citation conversion failures to the client.
citationsForContextDocumentslogs conversion failures but returns only the surviving citations, and this controller now emits them as fully successful. Preserve the failure count and emit the existing citation-warning status, asChatControllerdoes.As per coding guidelines, “catching and logging an exception while continuing as if the operation succeeded” is prohibited.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java` around lines 285 - 308, Update GuidedLearningController’s citationsForContextDocuments flow to preserve partial citation-conversion failures instead of treating surviving citations as fully successful. Capture the failure count/result alongside finalCitations and emit the existing citation-warning status event, matching ChatController’s behavior, before or with the citation event; do not silently continue after logged conversion failures.Source: Coding guidelines
src/main/java/com/williamcallahan/javachat/config/DocumentationSourceManifest.java (1)
119-134: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftReplace the 14-position constructor train with named domain groups.
Most columns are
String, so an adjacent-field swap compiles and silently corrupts manifest meaning. SplitDocumentationSourceinto cohesive typed components such as identity, fetch policy, and seed discovery, then construct those by name.As per coding guidelines, constructors with low-legibility argument trains are prohibited, and methods or constructors exceeding four parameters require parameter objects or builders.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/williamcallahan/javachat/config/DocumentationSourceManifest.java` around lines 119 - 134, Replace the positional 14-argument DocumentationSource construction in the manifest reader with named domain-group parameter objects or a builder, such as identity, fetch policy, and seed discovery. Update DocumentationSource and its call site to construct each group explicitly by field name, preserving existing parsing and validation behavior; ensure no constructor or method exceeds four parameters.Source: Coding guidelines
🧹 Nitpick comments (7)
src/test/java/com/williamcallahan/javachat/service/RetrievalConstraintTest.java (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a factory method for default constraints.
That's a lot of empty strings! 🚂 As a fun little tidbit, our coding guidelines mention avoiding "positional-null sludge" or repeated placeholder arguments.
If you find yourself creating blank constraints often in tests, throwing a small test helper method like
RetrievalConstraint.emptyWithDocSets(requestedDocSets)into the mix can keep our test setups sparkling clean and easy to read. ✨🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/service/RetrievalConstraintTest.java` at line 32, Replace the repeated empty-string constructor arguments in RetrievalConstraintTest with a clearly named factory or test helper such as emptyWithDocSets, passing requestedDocSets through it while preserving the resulting default constraint values.Source: Coding guidelines
src/test/java/com/williamcallahan/javachat/config/DocumentationSourceManifestTest.java (1)
166-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify array searching with
Arrays.asList.Did you know that
java.util.Arrays.asList(...).indexOf(...)is a super fun and idiomatic way to find an index without needing a manualforloop? It makes the code a tiny bit sweeter and easier to read! 🍬✨ Proposed refactor
- private static int canonicalFieldIndex(List<String> manifestLines, String canonicalFieldName) {- String[] manifestHeaderFields = manifestLines.getFirst().split("\\|", -1);- for (int fieldIndex = 0; fieldIndex < manifestHeaderFields.length; fieldIndex++) {- if (manifestHeaderFields[fieldIndex].equals(canonicalFieldName)) {- return fieldIndex;- }- }- throw new IllegalArgumentException(- "Canonical documentation source manifest has no field " + canonicalFieldName);- }+ private static int canonicalFieldIndex(List<String> manifestLines, String canonicalFieldName) {+ String[] manifestHeaderFields = manifestLines.getFirst().split("\\|", -1);+ int fieldIndex = java.util.Arrays.asList(manifestHeaderFields).indexOf(canonicalFieldName);+ if (fieldIndex >= 0) {+ return fieldIndex;+ }+ throw new IllegalArgumentException(+ "Canonical documentation source manifest has no field " + canonicalFieldName);+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/config/DocumentationSourceManifestTest.java` around lines 166 - 175, Update canonicalFieldIndex to use Arrays.asList(manifestHeaderFields).indexOf(canonicalFieldName) instead of the manual loop, while preserving the existing IllegalArgumentException when the field is not found.src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java (2)
165-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a dedicated unknown slug for clarity.
Fun testing tip! 🎩 Reusing
LESSON_SLUGhere works perfectly because thetocProvidermock isn't stubbed for it and returns empty. However, using a dedicated string like"unknown-lesson"might make the test's intent a bit clearer at first glance without relying on the mock's default behavior.💡 Proposed tweak
- for (String invalidLessonSlug : List.of(LESSON_SLUG, "")) {+ for (String invalidLessonSlug : List.of("unknown-lesson", "")) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java` at line 165, Update the invalidLessonSlug values in GuidedLearningServiceCitationTest to use a dedicated unknown slug such as "unknown-lesson" instead of reusing LESSON_SLUG, while retaining the empty-string case and existing test behavior.
106-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid manual instantiation of
ObjectMapper.Did you know that our coding guidelines explicitly prohibit manually instantiating
ObjectMapper? 🚀 Even in unit tests that lack a Spring context, it's a great habit to reuse a single instance (like aprivate static finalfield) or inject it, so we don't accidentally miss any awesome Spring Jackson configuration modules in our setups!Consider updating these calls to use a shared static instance or a Spring-injected bean to keep our DI practices top-notch:
src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java#L106-L106: Use a sharedObjectMapper.src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java#L134-L134: Use a sharedObjectMapper.src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java#L190-L190: Use a sharedObjectMapper.src/test/java/com/williamcallahan/javachat/service/GuidedLessonCuratedContentTest.java#L122-L122: Use a sharedObjectMapper.src/test/java/com/williamcallahan/javachat/service/GuidedTOCProviderTest.java#L26-L26: Use a sharedObjectMapper.src/test/java/com/williamcallahan/javachat/service/GuidedTOCProviderTest.java#L41-L41: Use a sharedObjectMapper.src/test/java/com/williamcallahan/javachat/service/GuidedTOCProviderTest.java#L62-L62: Use a sharedObjectMapper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java` at line 106, The tests manually instantiate ObjectMapper when constructing GuidedTOCProvider. Replace each occurrence with one shared ObjectMapper instance, such as a private static final field or injected bean, across src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java lines 106, 134, and 190, GuidedLessonCuratedContentTest.java line 122, and GuidedTOCProviderTest.java lines 26, 41, and 62.Source: Coding guidelines
frontend/src/lib/utils/highlight.ts (1)
51-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a tiny helper function to DRY up language registrations.
Here's a fun little tidbit: you can DRY up these language registrations with a tiny helper function! It keeps this block incredibly readable and saves a bit of repetition as your supported languages continue to grow. 🚀
💡 Proposed refactor for maximum sleekness
- if (!hljsInstance.getLanguage("plaintext")) {- hljsInstance.registerLanguage("plaintext", plaintext.default);- }- if (!hljsInstance.getLanguage("properties")) {- hljsInstance.registerLanguage("properties", properties.default);- }- if (!hljsInstance.getLanguage("kotlin")) {- hljsInstance.registerLanguage("kotlin", kotlin.default);- }- if (!hljsInstance.getLanguage("scala")) hljsInstance.registerLanguage("scala", scala.default);- if (!hljsInstance.getLanguage("groovy")) {- hljsInstance.registerLanguage("groovy", groovy.default);- }- if (!hljsInstance.getLanguage("clojure")) {- hljsInstance.registerLanguage("clojure", clojure.default);- }+ const register = (name: string, lang: any) => {+ if (!hljsInstance!.getLanguage(name)) {+ hljsInstance!.registerLanguage(name, lang);+ }+ };++ register("plaintext", plaintext.default);+ register("properties", properties.default);+ register("kotlin", kotlin.default);+ register("scala", scala.default);+ register("groovy", groovy.default);+ register("clojure", clojure.default);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/utils/highlight.ts` around lines 51 - 66, Extract the repeated language-registration logic in the highlight initialization block into a small helper that checks hljsInstance.getLanguage and registers the language only when absent. Update the plaintext, properties, kotlin, scala, groovy, and clojure registrations to use this helper while preserving their current language names and module defaults.Source: Coding guidelines
src/main/java/com/williamcallahan/javachat/service/GuidedTOCProvider.java (1)
40-56: 🚀 Performance & Scalability | 🔵 TrivialSmall scalability tidbit: every read pays a full deep-copy while holding the lock. 🙂
getTOC()is fullysynchronizedand rebuilds a fresh copy of all lessons (immutableLessonSnapshots) on each call, andfindBySlugruns this per lookup — so each guided request deep-copies every lesson while serialized on the monitor. SincetocLoadedandcachedLessonsare alreadyvolatile, the post-load path doesn't actually need the lock; avolatilefast-return keeps the detached-snapshot guarantee without serializing concurrent reads. Not urgent, just a friendly win under load.♻️ Optional lock-free read fast path
public synchronized List<GuidedLesson> getTOC() { - if (tocLoaded) return immutableLessonSnapshots(cachedLessons);+ return tocLoaded ? immutableLessonSnapshots(cachedLessons) : loadAndProjectTOC();+ }++ private synchronized List<GuidedLesson> loadAndProjectTOC() {+ if (tocLoaded) {+ return immutableLessonSnapshots(cachedLessons);+ } try { ... } tocLoaded = true; return immutableLessonSnapshots(cachedLessons); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/williamcallahan/javachat/service/GuidedTOCProvider.java` around lines 40 - 56, Update GuidedTOCProvider.getTOC() to avoid synchronizing already-loaded reads: check the volatile tocLoaded state and return an immutableLessonSnapshots(cachedLessons) fast path before entering the existing load lock. Preserve synchronized single-load behavior for the initial load and retain detached immutable snapshots for every caller.src/main/java/com/williamcallahan/javachat/model/GuidedLesson.java (1)
199-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA tiny naming adventure! 🕵️
It looks like the local variable
retainedDocSetsmight be a small copy-paste traveler from thedocSetlogic, but since this helper method now also validatessourceReferences, a more general name likeretainedTokensorseenTokenswould be perfectly intent-revealing for both use cases!Feel free to rename it if you'd like to keep the local variable name in sync with the method's parameter names.
✨ Proposed rename
private static void requireValidTokens(List<String> sourceTokens, String tokenName) { if (sourceTokens.isEmpty()) { throw new IllegalStateException("Guided lesson " + tokenName + " cannot be empty"); } - Set<String> retainedDocSets = new HashSet<>();+ Set<String> retainedTokens = new HashSet<>(); for (String sourceToken : sourceTokens) { if (sourceToken.isBlank()) { throw new IllegalStateException("Guided lesson " + tokenName + " cannot contain blank tokens"); } if (!sourceToken.equals(sourceToken.trim())) { throw new IllegalStateException("Guided lesson " + tokenName + " tokens must be trimmed"); } - if (!retainedDocSets.add(sourceToken)) {+ if (!retainedTokens.add(sourceToken)) { throw new IllegalStateException("Guided lesson " + tokenName + " cannot contain duplicate tokens"); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/williamcallahan/javachat/model/GuidedLesson.java` around lines 199 - 215, Rename the local set variable in requireValidTokens from retainedDocSets to a generic token-oriented name such as retainedTokens or seenTokens, and update its add/check usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/lib/services/pageMetadata.ts`:
- Around line 172-176: Update canonicalUrlForPath to resolve noncanonical route
aliases to their declared canonical paths before constructing the URL: map /chat
to / and /guided to /learn, while preserving normalized paths for canonical
routes and other paths. Keep the existing pageOrigin handling unchanged.
In `@scripts/documentation_seed.py`:
- Line 10: Update the XML parser import used by read_sitemap_urls() to use
defusedxml.ElementTree instead of xml.etree.ElementTree, and add defusedxml to
the project’s dependency configuration. Preserve the existing sitemap parsing
behavior while ensuring network-fetched XML is processed through the hardened
parser.
In `@scripts/fetch_all_docs.sh`:
- Around line 290-300: Update generate_java_api_javadoc_seed to write the
generator output to a temporary file, check the Python/tee pipeline status and
seed validation, and return nonzero before mirror reconciliation on any failure.
Only after successful validation should the temporary seed be atomically renamed
to seed_file; ensure the callers around the Java mirror flow at the referenced
status checks explicitly propagate this failure instead of continuing.
- Around line 365-369: Update the cached-return branch in the fetch flow around
count_html_files so it restores the caller’s original working directory before
returning success. Preserve the existing cache validation and log behavior, and
ensure subsequent sources and metadata processing run from the original
directory.
In `@scripts/lib/documentation_fetch_sources.sh`:
- Around line 115-142: The validate_fetch_result function currently accepts Wget
exit code 8 for all fetch strategies, allowing seed fetches to pass with stale
or incomplete HTML. Add an explicit strategy or mirror-mode flag to distinguish
recursive mirror fetches, and only permit exit code 8 on that path; seed fetches
must treat it as failure while preserving the existing completeness checks.
In `@scripts/lib/documentation_sources.sh`:
- Around line 92-115: Update load_documentation_source_manifest_header and the
related Bash validation paths to consume the shared canonical manifest field
catalog instead of accepting arbitrary field names or order. Align
DocumentationSourceManifest.validateHeader() with that same catalog so both
implementations reject unknown, missing, reordered, and duplicate fields
identically. Keep the governed inventory defined in exactly one module and
ensure all projections preserve the same field names and order.
- Around line 186-204: Update is_absolute_https_remote_url and
is_absolute_http_remote_base_url to reject remote URLs or base URLs containing
query (?) or fragment (#) components before validation or normalization;
preserve acceptance of valid HTTP/HTTPS values and the existing HTTPS conversion
behavior.
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Around line 258-263: Move the sseSupport.configureStreamingHeaders(response)
call before the openAIStreamingService.isAvailable() check in the guided-session
streaming flow, so both unavailable-service errors and successful streams
receive the required SSE headers.
In `@src/main/resources/documentation-sources.manifest`:
- Around line 3-4: Update the manifest entries for Kotlin and Scala, along with
the referenced entries at rows 6 and 8, so their source URLs are immutable
version-specific endpoints when retaining versioned paths and metadata;
alternatively remove the pinned version identity and mark them as rolling
documentation. Ensure no unversioned documentation URL is stored under a
specific release label.
In `@src/main/resources/guided/lessons/data-access-and-testing-in-spring-boot.md`:
- Around line 60-70: Update the StudyNote(String title) constructor to reject
titles exceeding the `@Column` limit of 200 characters, while preserving the
existing null and blank validation. Add or update a test covering an overlong
title if validation tests are present.
- Around line 94-97: Align StudyNoteRepository.findByTitle with the database’s
duplicate-title policy: either add and test a uniqueness constraint for
StudyNote.title to preserve Optional<StudyNote>, or change findByTitle to return
a collection if duplicate titles are allowed. Ensure the repository contract and
database behavior are consistent.
In `@src/main/resources/guided/lessons/dependency-injection-and-configuration.md`:
- Around line 72-74: Update the StudyReminderProperties record invariant to
reject negative leadTime values while allowing zero, using validation
appropriate for Duration. Add coverage for zero and negative durations,
including the expected rejection at the negative boundary.
In `@src/main/resources/guided/lessons/groovy-on-the-jvm.md`:
- Line 24: Replace the authoring-environment disclaimer in
src/main/resources/guided/lessons/groovy-on-the-jvm.md:24 with a reader-facing
instruction explaining how to compile the Groovy snippet, and make the
corresponding replacement in
src/main/resources/guided/lessons/kotlin-on-the-jvm.md:26 with Kotlin
compilation guidance; do not retain workspace-specific statements.
In `@src/main/resources/guided/lessons/records.md`:
- Line 54: Rewrite the `List.copyOf` explanation in the records lesson to state
that it creates an unmodifiable snapshot of the elements, so later additions or
removals to the original list do not affect the record. Do not claim that it
makes the caller’s original list unmodifiable; preserve the clarification that
mutable topic objects are not deep-copied.
---
Outside diff comments:
In
`@src/main/java/com/williamcallahan/javachat/config/DocumentationSourceManifest.java`:
- Around line 119-134: Replace the positional 14-argument DocumentationSource
construction in the manifest reader with named domain-group parameter objects or
a builder, such as identity, fetch policy, and seed discovery. Update
DocumentationSource and its call site to construct each group explicitly by
field name, preserving existing parsing and validation behavior; ensure no
constructor or method exceeds four parameters.
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Around line 285-308: Update GuidedLearningController’s
citationsForContextDocuments flow to preserve partial citation-conversion
failures instead of treating surviving citations as fully successful. Capture
the failure count/result alongside finalCitations and emit the existing
citation-warning status event, matching ChatController’s behavior, before or
with the citation event; do not silently continue after logged conversion
failures.
---
Nitpick comments:
In `@frontend/src/lib/utils/highlight.ts`:
- Around line 51-66: Extract the repeated language-registration logic in the
highlight initialization block into a small helper that checks
hljsInstance.getLanguage and registers the language only when absent. Update the
plaintext, properties, kotlin, scala, groovy, and clojure registrations to use
this helper while preserving their current language names and module defaults.
In `@src/main/java/com/williamcallahan/javachat/model/GuidedLesson.java`:
- Around line 199-215: Rename the local set variable in requireValidTokens from
retainedDocSets to a generic token-oriented name such as retainedTokens or
seenTokens, and update its add/check usage accordingly.
In `@src/main/java/com/williamcallahan/javachat/service/GuidedTOCProvider.java`:
- Around line 40-56: Update GuidedTOCProvider.getTOC() to avoid synchronizing
already-loaded reads: check the volatile tocLoaded state and return an
immutableLessonSnapshots(cachedLessons) fast path before entering the existing
load lock. Preserve synchronized single-load behavior for the initial load and
retain detached immutable snapshots for every caller.
In
`@src/test/java/com/williamcallahan/javachat/config/DocumentationSourceManifestTest.java`:
- Around line 166-175: Update canonicalFieldIndex to use
Arrays.asList(manifestHeaderFields).indexOf(canonicalFieldName) instead of the
manual loop, while preserving the existing IllegalArgumentException when the
field is not found.
In
`@src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java`:
- Line 165: Update the invalidLessonSlug values in
GuidedLearningServiceCitationTest to use a dedicated unknown slug such as
"unknown-lesson" instead of reusing LESSON_SLUG, while retaining the
empty-string case and existing test behavior.
- Line 106: The tests manually instantiate ObjectMapper when constructing
GuidedTOCProvider. Replace each occurrence with one shared ObjectMapper
instance, such as a private static final field or injected bean, across
src/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.java
lines 106, 134, and 190, GuidedLessonCuratedContentTest.java line 122, and
GuidedTOCProviderTest.java lines 26, 41, and 62.
In
`@src/test/java/com/williamcallahan/javachat/service/RetrievalConstraintTest.java`:
- Line 32: Replace the repeated empty-string constructor arguments in
RetrievalConstraintTest with a clearly named factory or test helper such as
emptyWithDocSets, passing requestedDocSets through it while preserving the
resulting default constraint values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 217be38d-98a6-4772-bfdd-43f6e0fac1c4
📒 Files selected for processing (94)
docs/api.mddocs/pipeline-commands.mdfrontend/index.htmlfrontend/src/App.sveltefrontend/src/App.test.tsfrontend/src/lib/components/GuidedLessonHeader.sveltefrontend/src/lib/components/GuidedLessonHeader.test.tsfrontend/src/lib/components/LearnView.sveltefrontend/src/lib/components/LearnView.test.tsfrontend/src/lib/services/guided.test.tsfrontend/src/lib/services/guided.tsfrontend/src/lib/services/pageMetadata.tsfrontend/src/lib/utils/highlight.test.tsfrontend/src/lib/utils/highlight.tsfrontend/src/lib/validation/schemas.tsscripts/documentation_seed.pyscripts/fetch_all_docs.shscripts/lib/documentation_fetch_sources.shscripts/lib/documentation_sources.shscripts/test_documentation_fetch_projection.shscripts/test_java_api_fetch_projection.shscripts/testdata/documentation_seed/navigation.htmlscripts/testdata/documentation_seed/sitemap-line-injection.xmlscripts/testdata/documentation_seed/sitemap.xmlsrc/main/java/com/williamcallahan/javachat/config/AppProperties.javasrc/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.javasrc/main/java/com/williamcallahan/javachat/config/DocumentationManifestFieldRules.javasrc/main/java/com/williamcallahan/javachat/config/DocumentationSeedDocumentTypeCatalog.javasrc/main/java/com/williamcallahan/javachat/config/DocumentationSourceManifest.javasrc/main/java/com/williamcallahan/javachat/config/JavaApiDocumentationManifest.javasrc/main/java/com/williamcallahan/javachat/model/GuidedLesson.javasrc/main/java/com/williamcallahan/javachat/service/GuidedLearningService.javasrc/main/java/com/williamcallahan/javachat/service/GuidedTOCProvider.javasrc/main/java/com/williamcallahan/javachat/service/QdrantRetrievalConstraintBuilder.javasrc/main/java/com/williamcallahan/javachat/service/RetrievalConstraint.javasrc/main/java/com/williamcallahan/javachat/service/RetrievalService.javasrc/main/java/com/williamcallahan/javachat/support/PdfCitationEnhancer.javasrc/main/java/com/williamcallahan/javachat/util/JavadocMemberAnchorResolver.javasrc/main/java/com/williamcallahan/javachat/web/ChatController.javasrc/main/java/com/williamcallahan/javachat/web/GuidedLearningController.javasrc/main/java/com/williamcallahan/javachat/web/SseConstants.javasrc/main/java/com/williamcallahan/javachat/web/SseSupport.javasrc/main/resources/application.propertiessrc/main/resources/documentation-seed-document-types.manifestsrc/main/resources/documentation-sources.manifestsrc/main/resources/guided/lessons/arrays.mdsrc/main/resources/guided/lessons/building-rest-apis-with-spring-boot.mdsrc/main/resources/guided/lessons/choosing-a-jvm-language.mdsrc/main/resources/guided/lessons/classes-and-objects.mdsrc/main/resources/guided/lessons/clojure-on-the-jvm.mdsrc/main/resources/guided/lessons/collections.mdsrc/main/resources/guided/lessons/concurrency-and-virtual-threads.mdsrc/main/resources/guided/lessons/conditionals.mdsrc/main/resources/guided/lessons/data-access-and-testing-in-spring-boot.mdsrc/main/resources/guided/lessons/dependency-injection-and-configuration.mdsrc/main/resources/guided/lessons/enums-and-sealed-types.mdsrc/main/resources/guided/lessons/exceptions.mdsrc/main/resources/guided/lessons/file-io.mdsrc/main/resources/guided/lessons/garbage-collection-and-memory.mdsrc/main/resources/guided/lessons/generics.mdsrc/main/resources/guided/lessons/groovy-on-the-jvm.mdsrc/main/resources/guided/lessons/interfaces-and-inheritance.mdsrc/main/resources/guided/lessons/introduction-to-java.mdsrc/main/resources/guided/lessons/kotlin-on-the-jvm.mdsrc/main/resources/guided/lessons/lambdas-and-streams.mdsrc/main/resources/guided/lessons/loops.mdsrc/main/resources/guided/lessons/methods.mdsrc/main/resources/guided/lessons/modern-java-pattern-matching.mdsrc/main/resources/guided/lessons/modules.mdsrc/main/resources/guided/lessons/packages-and-imports.mdsrc/main/resources/guided/lessons/quarkus-fundamentals.mdsrc/main/resources/guided/lessons/records.mdsrc/main/resources/guided/lessons/recursion.mdsrc/main/resources/guided/lessons/scala-on-the-jvm.mdsrc/main/resources/guided/lessons/spring-boot-fundamentals.mdsrc/main/resources/guided/lessons/spring-boot-vs-quarkus.mdsrc/main/resources/guided/lessons/strings.mdsrc/main/resources/guided/lessons/testing-with-junit.mdsrc/main/resources/guided/lessons/variables-and-types.mdsrc/main/resources/guided/toc.jsonsrc/test/java/com/williamcallahan/javachat/config/DocumentationSourceManifestTest.javasrc/test/java/com/williamcallahan/javachat/service/GuidedLearningServiceCitationTest.javasrc/test/java/com/williamcallahan/javachat/service/GuidedLessonCuratedContentTest.javasrc/test/java/com/williamcallahan/javachat/service/GuidedTOCProviderTest.javasrc/test/java/com/williamcallahan/javachat/service/QdrantRetrievalConstraintBuilderTest.javasrc/test/java/com/williamcallahan/javachat/service/RetrievalConstraintTest.javasrc/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.javasrc/test/java/com/williamcallahan/javachat/util/JavadocTypeCanonicalizerTest.javasrc/test/java/com/williamcallahan/javachat/web/ChatControllerStreamingFailureTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerBackpressureOverflowTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerCuratedContentTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerStreamingFailureTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedLearningControllerTest.javasrc/test/java/com/williamcallahan/javachat/web/GuidedSseCitationEventTest.java
💤 Files with no reviewable changes (3)
- src/main/java/com/williamcallahan/javachat/support/PdfCitationEnhancer.java
- src/main/resources/application.properties
- src/main/java/com/williamcallahan/javachat/config/AppProperties.java
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
make lintmake buildmake test— 619 tests passed, 0 failures, 0 skippednpm run validateandnpm run test— 158 frontend tests passedDeployment verification
The dev deployment is pinned to commit
09c3612341e0d8204283f7696aca94e3e9b0b4d8. After review checks pass and this PR is merged, production verification will confirm exact runtime commit identity, health/readiness, canonical Java citation paths, stream latency, unknown-slug rejection, responsive guided navigation, and a fresh black-box dogfood pass.