feat(guided): independent lesson chats, streaming citations, smart scrolling, and Zod validation - #9
Conversation
…g behavior ChatService contained 30+ lines of sequential boolean checks to categorize search result quality and generate LLM context messages. This procedural logic violated Tell-Don't-Ask by interrogating document properties instead of letting a domain concept describe itself. The new enum encapsulates both categorization logic (determine) and message formatting (formatMessage), eliminating the boolean chain and making quality levels explicit domain vocabulary. The static describeQuality convenience method maintains the existing API contract while delegating to the new polymorphic design. Also removes deprecated streamResponse(String, double) method from OpenAIStreamingService as all callers now use the StructuredPrompt-based overload. - Add SearchQualityLevel enum with NONE, KEYWORD_SEARCH, HIGH_QUALITY, MIXED_QUALITY - Each level owns its message template and formatting logic - Static determine() replaces procedural if-else chain in ChatService - describeQuality() provides drop-in replacement for existing callers - Remove deprecated OpenAIStreamingService.streamResponse(String, double)
…-aware guidance GuidedLearningService had grown to include 80+ lines of PDF pagination logic (page counting, chunk enumeration, anchor calculation) unrelated to lesson orchestration, plus it lacked awareness of the current lesson context when generating LLM guidance. This commit addresses both issues: 1. Extraction: The new PdfCitationEnhancer component owns the entire citation enhancement workflow (loading PDF for page count, counting chunks, parsing metadata, estimating pages). This removes LocalStoreService from GuidedLearningService and improves testability. 2. Lesson Focus: New buildLessonGuidance() and buildLessonContextDescription() methods construct LLM prompts that include the lesson title, summary, and keywords. The guidance template now includes topic handling rules to redirect greetings and off-topic questions back to the current lesson. - Extract PdfCitationEnhancer to support/ with proper Javadocs - Add THINK_JAVA_GUIDANCE_TEMPLATE with %s placeholder for lesson context - Inject SystemPromptConfig for guided learning mode instructions - Update streamLessonChat and buildLessonPrompt to use lesson-aware guidance
…ion bleeding A single session ID was shared across all lessons in the frontend, causing conversation history from one lesson to bleed into another. For example, asking about loops after studying variables would reference unrelated variable discussion from the shared history. The frontend now maintains a Map<lessonSlug, sessionId> so each lesson gets an isolated backend conversation. The GuidedStreamRequest accessor methods now return Optional<String> instead of defaulting to empty strings, forcing callers to handle missing values explicitly. Frontend: - Replace single sessionId with sessionIdsByLesson Map - Add getSessionIdForLesson() to create/retrieve per-lesson sessions - Use lesson-scoped session ID in streamGuidedChat calls Backend: - Change GuidedStreamRequest.userQuery() to return Optional<String> - Change GuidedStreamRequest.lessonSlug() to return Optional<String> - Filter out blank values in Optional accessors
…tional API Adds test coverage for the SearchQualityLevel enum extracted in commit 6095599, verifying all quality level determinations and message formatting. Also updates GuidedLearningController to use orElseThrow() on the new Optional<String> return types from GuidedStreamRequest, failing fast with clear error messages when required fields are missing. Tests cover: - NONE returned for null/empty document lists - KEYWORD_SEARCH detected from URL metadata patterns - HIGH_QUALITY when all documents have substantial content (>100 chars) - MIXED_QUALITY when some documents have short content - formatMessage() produces correct strings for each level - describeQuality() convenience method integration Controller: - Use orElseThrow() on userQuery() with "User query is required" message - Use orElseThrow() on lessonSlug() with "Lesson slug is required" message
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughImplements lesson-scoped guided chat with per-message streaming and in-chat citation events; adds PDF citation anchoring and search-quality classification; refactors rate-limit/streaming services and SSE support; introduces Zod runtime validation, new frontend chat components and scroll helpers, extensive docs/linting/tooling updates, and associated tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as LearnView/ChatView
participant GuidedClient as guided.ts
participant SSE as streamSse
participant Backend as GuidedLearningController
participant GLS as GuidedLearningService
participant OAI as OpenAIStreamingService
participant ChatMem as ChatMemoryService
User->>Frontend: select lesson & send message
Frontend->>GuidedClient: streamGuidedChat(slug, message, callbacks, {signal})
GuidedClient->>SSE: POST /api/guided/stream (with signal)
SSE->>Backend: SSE connection established
Backend->>GLS: buildStructuredGuidedPromptWithContext(history, slug, message)
GLS-->>Backend: GuidedChatPromptOutcome(structuredPrompt, bookDocs)
Backend->>OAI: streamResponse(structuredPrompt)
loop streaming chunks
OAI-->>Backend: text chunk
Backend->>SSE: SSE event (data/text)
SSE->>GuidedClient: onChunk(text)
GuidedClient->>Frontend: update assistant message by messageId
OAI-->>Backend: citation event (async)
Backend->>SSE: SSE event (citations)
SSE->>GuidedClient: onCitations(citations)
GuidedClient->>Frontend: attach citations to assistant message
end
OAI-->>Backend: [DONE]
Backend->>ChatMem: persist assistant final message
Backend->>SSE: final citation/status events
sequenceDiagram
participant Client
participant SSE as streamSse
participant Validator as Zod
participant Callbacks
Client->>SSE: streamSse(url, body, callbacks, {signal})
SSE->>SSE: fetch(..., {signal})
loop read events
SSE->>Validator: tryParseJson & validate(payload)
alt valid
Validator-->>SSE: validated payload
SSE->>Callbacks: invoke matching callback (onText/onStatus/onCitations/onProvider)
else invalid
Validator-->>SSE: logZodFailure
SSE->>Callbacks: fallback to raw-text handling
end
end
Client-->>SSE: abort()
SSE->>SSE: handle AbortError gracefully (terminate without onError)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR enhances the guided learning experience by isolating chat sessions per lesson, enriching LLM guidance with lesson context and standardized search quality signals, and modularizing PDF citation handling.
Changes:
- Introduces
SearchQualityLeveland associated tests to centralize how search quality is categorized and described to the LLM, and wires it intoChatService. - Refactors guided learning backend to build lesson-aware guidance prompts via
SystemPromptConfig, delegate PDF citation page anchoring toPdfCitationEnhancer, and tighten validation of guided streaming requests. - Updates the guided learning frontend to use per-lesson session IDs when streaming chat, ensuring conversation isolation across lessons.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/williamcallahan/javachat/domain/SearchQualityLevel.java | Adds an enum that classifies search result sets and produces consistent, self-describing quality messages consumed by the chat pipeline. |
| src/test/java/com/williamcallahan/javachat/domain/SearchQualityLevelTest.java | Verifies SearchQualityLevel.determine, formatMessage, and describeQuality behavior across empty, keyword, high-quality, and mixed-quality scenarios. |
| src/main/java/com/williamcallahan/javachat/web/GuidedStreamRequest.java | Changes userQuery and lessonSlug accessors to Optional, encouraging explicit handling of missing/blank values for guided streaming requests. |
| src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java | Uses the new Optional accessors to enforce required user query and lesson slug for guided chat, and continues to build structured prompts for streaming. |
| src/main/java/com/williamcallahan/javachat/support/PdfCitationEnhancer.java | New component that computes and caches Think Java PDF page counts, estimates page numbers from chunk indices, and appends #page=N anchors and anchors fields to PDF citations. |
| src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java | Removes the deprecated string-based streaming API, leaving a single structured-prompt-based streamResponse entry point used by controllers and services. |
| src/main/java/com/williamcallahan/javachat/service/GuidedLearningService.java | Wires in SystemPromptConfig and PdfCitationEnhancer, uses lesson metadata to build targeted guidance strings, and delegates PDF citation enhancement out of the service. |
| src/main/java/com/williamcallahan/javachat/service/ChatService.java | Delegates search quality description to SearchQualityLevel.describeQuality, simplifying logic and keeping messaging consistent with system prompt behavior. |
| frontend/src/lib/components/LearnView.svelte | Replaces a single guided session ID with a per-lesson session ID map and passes the appropriate ID into streamGuidedChat, preventing cross-lesson conversation bleed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/lib/components/LearnView.svelte (1)
213-227: Use the captured slug when starting the stream.Because
doScrollToBottom()is awaited,selectedLessoncan change before the request fires, pairing an old session ID with a new slug. UsingstreamLessonSlugkeeps session isolation intact. Tiny tip: captured values are your safest anchors in async flows.🔧 Suggested fix
- await streamGuidedChat(lessonSessionId, selectedLesson.slug, userQuery, {+ await streamGuidedChat(lessonSessionId, streamLessonSlug, userQuery, {
🤖 Fix all issues with AI agents
In `@src/main/java/com/williamcallahan/javachat/domain/SearchQualityLevel.java`:
- Around line 1-5: SearchQualityLevel currently depends on
org.springframework.ai.document.Document, duplicates the "100" magic literal and
repeats the same high-quality filtering logic; remove the framework import and
refactor by introducing a domain-facing interface (e.g., RetrievedContent with
String getText() and boolean isHighQuality()) and use that in SearchQualityLevel
instead of Document, extract the numeric literal into a named constant (e.g.,
HIGH_QUALITY_LENGTH_THRESHOLD = 100) and replace both occurrences with it, and
move the duplicated filter/count logic into a single helper method (e.g.,
countHighQuality(List<RetrievedContent>) or RetrievedContent.isHighQuality())
inside the enum or a small domain util so both code paths call the same method;
map Document to RetrievedContent in an adapter layer outside the domain.
In `@src/main/java/com/williamcallahan/javachat/support/PdfCitationEnhancer.java`:
- Around line 88-95: Reword the Javadoc opener for the PdfCitationEnhancer
method that returns the Think Java PDF page count (the method currently
documented with "Gets the total page count for the Think Java PDF.") to use a
direct verb or noun phrase instead of "Gets the…"; update the first sentence to
something like "Return the total page count for the Think Java PDF." or "Total
page count of the Think Java PDF." while keeping the rest of the Javadoc and
tags unchanged.
- Around line 45-55: Update the PdfCitationEnhancer Javadoc for the public
method enhanceWithPageAnchors to explicitly document that it may throw an
UncheckedIOException when PDF loading or chunk listing fails; add an `@throws`
UncheckedIOException line describing that the exception is raised if underlying
I/O operations (e.g., opening the PDF, reading chunks, or listing chunk
metadata) fail during page-anchor estimation so callers are aware to handle
runtime I/O errors.
- Around line 56-59: In PdfCitationEnhancer.enhanceWithPageAnchors, don't
silently return the original citations when docs.size() != citations.size();
instead surface the mismatch by failing fast — either throw an
IllegalArgumentException with a clear message including both sizes, or at
minimum log a warning with those sizes and then throw; update the method to
validate the contract up front so callers immediately see the mismatch rather
than getting back unmodified data.
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Around line 187-190: GuidedLearningController currently throws
IllegalArgumentException for missing fields but lacks an `@ExceptionHandler`,
causing 500 responses; add a method annotated with
`@ExceptionHandler`(IllegalArgumentException.class) in GuidedLearningController
that delegates to the shared super.handleValidationException(...) exactly like
IngestionController does so IllegalArgumentException is mapped to a 400
response; reference the controller class name GuidedLearningController and the
existing superclass method handleValidationException to locate where to add the
handler.
🧹 Nitpick comments (6)
src/main/java/com/williamcallahan/javachat/support/PdfCitationEnhancer.java (1)
68-83: Extract PDF-related literals into constants.Strings like
#page=, and the anchor key are repeated and should be named constants for clarity and consistency. As per coding guidelines, avoid magic literals.♻️ Example refactor
+ private static final String PDF_EXTENSION = ".pdf";+ private static final String PAGE_ANCHOR_PREFIX = "#page=";+ private static final String PAGE_ANCHOR_VALUE_PREFIX = "page="; @@ - if (url == null || !url.toLowerCase(Locale.ROOT).endsWith(".pdf")) {+ if (url == null || !url.toLowerCase(Locale.ROOT).endsWith(PDF_EXTENSION)) { continue; } @@ - String withAnchor = url.contains("#page=") ? url : url + "#page=" + page;+ String withAnchor = url.contains(PAGE_ANCHOR_PREFIX) ? url : url + PAGE_ANCHOR_PREFIX + page; citation.setUrl(withAnchor); - citation.setAnchor("page=" + page);+ citation.setAnchor(PAGE_ANCHOR_VALUE_PREFIX + page);src/main/java/com/williamcallahan/javachat/service/GuidedLearningService.java (2)
71-77: Constructor parameter list is getting long.Now at 7 parameters; consider a parameter object or grouping related dependencies to keep injection tidy. Tiny tip: fewer constructor params makes wiring and tests happier. As per coding guidelines, avoid >4 positional parameters.
323-339: Extract lesson-label strings into named constants.Inline labels like “Lesson Summary:” and the default “No specific lesson selected…” are magic literals; constants improve consistency and reuse. Tiny tip: names turn strings into domain vocabulary. As per coding guidelines, avoid magic literals.
♻️ Example extraction
+ private static final String NO_LESSON_CONTEXT_MESSAGE =+ "No specific lesson selected. Provide general Java learning assistance.";+ private static final String LESSON_SUMMARY_LABEL = "Lesson Summary: ";+ private static final String KEY_CONCEPTS_LABEL = "Key concepts to cover: "; @@ - return "No specific lesson selected. Provide general Java learning assistance.";+ return NO_LESSON_CONTEXT_MESSAGE; @@ - contextBuilder.append("\n\nLesson Summary: ").append(lesson.getSummary());+ contextBuilder.append("\n\n").append(LESSON_SUMMARY_LABEL).append(lesson.getSummary()); @@ - contextBuilder.append("\n\nKey concepts to cover: ")+ contextBuilder.append("\n\n").append(KEY_CONCEPTS_LABEL) .append(String.join(", ", lesson.getKeywords()));src/main/java/com/williamcallahan/javachat/web/GuidedStreamRequest.java (1)
28-45: Tighten accessor docs and consider value types.Style-wise, drop “Returns the…” in the opener; also consider introducing a validated
LessonSlug/UserQueryvalue type so these accessors returnOptional<LessonSlug>etc. Tiny tip: small domain types are great bug repellant. As per coding guidelines, avoid filler Javadoc phrases and raw primitives at API boundaries.✍️ Suggested Javadoc wording
- * Returns the user query when present and non-blank.+ * Provides the user query when present and non-blank. @@ - * Returns the lesson slug when present and non-blank.+ * Provides the lesson slug when present and non-blank.src/main/java/com/williamcallahan/javachat/domain/SearchQualityLevel.java (2)
77-83: Extract magic literal 100 to a named constant.The threshold
100appears here and again indescribeQuality(). A named constant likeSUBSTANTIAL_CONTENT_LENGTH_THRESHOLDmakes the intent crystal clear and ensures consistency if you ever need to tune it. Plus, future-you will thank present-you when debugging! 📚✨ Suggested improvement
public enum SearchQualityLevel { + /**+ * Minimum content length (in characters) to consider a document substantial.+ */+ private static final int SUBSTANTIAL_CONTENT_THRESHOLD = 100;+ // ... enum values ... // In determine(): - return content != null && content.length() > 100;+ return content != null && content.length() > SUBSTANTIAL_CONTENT_THRESHOLD; // In describeQuality(): - .filter(doc -> doc.getText() != null && doc.getText().length() > 100)+ .filter(doc -> doc.getText() != null && doc.getText().length() > SUBSTANTIAL_CONTENT_THRESHOLD)
98-108: Duplicate high-quality filtering logic could be consolidated.The same
doc.getText() != null && doc.getText().length() > 100predicate appears in bothdetermine()anddescribeQuality(). Consider extracting a small helper predicate—it keeps things DRY and makes future threshold adjustments safer. Think of it as giving that logic a cozy home of its own! 🏠✨ Suggested consolidation
+ private static boolean isSubstantialContent(Document doc) {+ String content = doc.getText();+ return content != null && content.length() > SUBSTANTIAL_CONTENT_THRESHOLD;+ }+ public static SearchQualityLevel determine(List<Document> docs) { // ... long highQualityCount = docs.stream() - .filter(doc -> {- String content = doc.getText();- return content != null && content.length() > 100;- })+ .filter(SearchQualityLevel::isSubstantialContent) .count(); // ... } public static String describeQuality(List<Document> docs) { SearchQualityLevel level = determine(docs); int totalCount = docs != null ? docs.size() : 0; long highQualityCount = docs != null - ? docs.stream()- .filter(doc -> doc.getText() != null && doc.getText().length() > 100)- .count()+ ? docs.stream().filter(SearchQualityLevel::isSubstantialContent).count() : 0; return level.formatMessage(totalCount, (int) highQualityCount); }
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.
The original README (474 lines) contained everything from quick-start to API reference, configuration, architecture, mobile notes, and troubleshooting. This made it hard to navigate, maintain, and onboard contributors. The refactoring extracts each concern into its own file under docs/ while leaving a concise README as the project landing page. New documentation structure: - docs/README.md: table of contents and entry point - docs/getting-started.md: prerequisites, quick-start, common commands - docs/configuration.md: env vars for LLM, embeddings, Qdrant, RAG tuning - docs/api.md: HTTP endpoints (SSE streaming, guided learning, ingestion) - docs/ingestion.md: fetch/process/dedupe pipeline for RAG indexing - docs/architecture.md: high-level components and request flow Root README now: project summary, feature highlights, quick-start snippet, and a pointer to docs/README.md for comprehensive documentation.
…licate high-quality counting
…tch, reword 'Gets the' opener
…n 400 instead of 500
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:94f27592ed
ℹ️ 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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/configuration.md`:
- Around line 18-19: Update the sentence "If both keys are present, the service
prefers OpenAI for streaming and can fall back to GitHub Models based on
rate-limit/backoff state." to remove any implication of silent provider
switching: document that falling back requires an explicit opt-in configuration
(e.g., a named config flag like ENABLE_PROVIDER_FALLBACK or
prefer_openai_with_fallback) and otherwise the service surfaces a visible error
when the preferred provider fails; also add a short note describing the
user-visible error path and how to enable fallback so readers know how to
control model attribution and debugging.
In `@README.md`:
- Around line 7-12: Update the README's "Documentation ingestion pipeline (fetch
→ chunk → embed → dedupe → index)" bullet to mention that chunking uses
JTokkit's CL100K_BASE tokenizer (GPT‑3.5/4 style) and briefly note how that
affects token counts and overlap behavior; specifically, add a short sentence
after the pipeline bullet explaining "chunks are created using the CL100K_BASE
tokenizer from JTokkit (GPT‑3.5/4 tokenization) so token counts and overlap
should be estimated with that tokenizer" and optionally a one-line tip about
estimating tokens for typical text to predict chunk sizes and overlaps.
In `@src/main/java/com/williamcallahan/javachat/support/PdfCitationEnhancer.java`:
- Around line 64-86: The code uses a global thinkJavaPages (from
getThinkJavaPdfPages()) for every PDF which can mis-anchor non-Think‑Java PDFs;
restrict anchor logic to only the Think Java PDF by checking the URL before
applying page anchors (e.g., compare url to the known Think Java PDF URL or
detect a Think‑Java identifier) and only then call estimatePage and set
citation.setUrl / citation.setAnchor; alternatively compute per-URL page counts
instead of using thinkJavaPages if you need anchors for other PDFs. Ensure this
check is added around the block that uses thinkJavaPages, leaving
parseChunkIndex, countChunksForUrl and estimatePage untouched.
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.
…GuidedLearningController
There was a problem hiding this comment.
Actionable comments posted: 5
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/service/ExternalServiceHealth.java (1)
314-358: Avoid nullable public return values and tighten Javadocs.
timeUntilNextCheck()can return null today. The guideline asks public methods to avoid nulls, so wrapping internally inOptionalkeeps callers safe. Also, the Javadocs use “Returns the …”, which the style guide bans—swap to a verb-led sentence (e.g., “Provides the service identifier.”). Tiny polish, big consistency.✅ Optional-friendly adjustment (no Optional params)
+import java.util.Optional; @@ - private final Duration timeUntilNextCheck;+ private final Optional<Duration> timeUntilNextCheck; @@ - public HealthSnapshot(String name, boolean isHealthy, String message, Duration timeUntilNextCheck) {+ public HealthSnapshot(String name, boolean isHealthy, String message, Duration timeUntilNextCheck) { this.name = name; this.healthy = isHealthy; this.message = message; - this.timeUntilNextCheck = timeUntilNextCheck;+ this.timeUntilNextCheck = Optional.ofNullable(timeUntilNextCheck); } @@ - public Duration timeUntilNextCheck() {+ public Optional<Duration> timeUntilNextCheck() { return timeUntilNextCheck; }Then update callers (e.g.,
QdrantHealthIndicator) to unwrap viamap(...).ifPresent(...).As per coding guidelines “Public methods never return null; singletons use
Optional<T>” and “No filler phrases: ban 'Returns the...', 'Gets the...', 'Sets the...', 'This method...'”.src/main/java/com/williamcallahan/javachat/service/RateLimitService.java (1)
514-546: Auto-fallback across providers may violate the no-silent-switch policy. If OpenAI is rate-limited, selecting GitHub Models (or Local) here can silently change model/provider behavior. Consider returningOptional.empty()and surfacing a clear rate-limit error to the user instead. Based on learnings, Do not auto-fallback or regress models across providers; if rate-limited, surface error to user, never silently switch.
🤖 Fix all issues with AI agents
In `@docs/contracts/code-change.md`:
- Line 25: The Markdown table separator row
"|----------|---------|-------------|" lacks spaces around the pipe characters;
update that row (the table separator line) to include single spaces around each
pipe (e.g. change the separator to have " | " between columns) so it matches the
repository's compact Markdown table style and satisfies the linter.
In `@rules/ast-grep/java-requestbody-requires-valid.yml`:
- Around line 10-14: The current rule flags parameters annotated '@RequestBody
$TYPE $VAR' but only excludes the '@Valid `@RequestBody` $TYPE $VAR' ordering;
update the rule so it also recognizes and excludes the reverse ordering
'@RequestBody `@Valid` $TYPE $VAR' (e.g., change the not.inside check to match
either ordering or supply both patterns) so both annotation orders are treated
as valid and avoid false positives when scanning for '@RequestBody $TYPE $VAR'.
In
`@src/main/java/com/williamcallahan/javachat/config/QdrantHealthIndicator.java`:
- Around line 30-41: The variable named info in QdrantHealthIndicator should be
renamed to a domain-specific identifier (e.g., healthSnapshot) to comply with
the banned-names guideline; update the declaration returned by
externalServiceHealth.getHealthSnapshot(ExternalServiceHealth.SERVICE_QDRANT)
and all subsequent uses (info.isHealthy(), info.message(),
info.timeUntilNextCheck()) to use healthSnapshot, and ensure the type
ExternalServiceHealth.HealthSnapshot and the surrounding logic in
QdrantHealthIndicator remain unchanged.
In
`@src/main/java/com/williamcallahan/javachat/service/ExternalServiceHealth.java`:
- Around line 119-145: Extract the inline user-facing strings in
getHealthSnapshot into private static final constants (e.g.,
HEALTHY_MSG_TEMPLATE, UNHEALTHY_CHECKING_MSG, UNHEALTHY_NEXT_CHECK_TEMPLATE,
UNKNOWN_SERVICE_MSG) and replace the literals used when constructing the message
for HealthSnapshot; update references to status.isHealthy, formatDuration and
timeUntilNextCheck logic remain unchanged, and ensure the constants are declared
near the top of ExternalServiceHealth class for clarity and reuse (affecting
getHealthSnapshot, HealthSnapshot construction, and any tests expecting these
messages).
In `@src/main/java/com/williamcallahan/javachat/service/RateLimitService.java`:
- Around line 1-5: The file RateLimitService.java is failing Spotless
formatting; run the formatter and commit the fixes: execute ./gradlew
spotlessApply, reformat the RateLimitService class and its imports (e.g., the
package declaration and imports like com.openai.core.http.Headers and
com.openai.errors.OpenAIServiceException), verify formatting for any
methods/fields inside RateLimitService, then re-commit the changed file so CI
passes.
🧹 Nitpick comments (10)
Makefile (1)
119-149: Backend-only target completes the trilogy! 🎬The
OPENAI_API_KEYhandling is consistent here too—great job ensuring all three entry points (run,dev,dev-backend) support both credential sources.Optional DRY opportunity: The APP_ARGS construction logic (~15 lines) is now repeated three times. If you find yourself tweaking credential handling frequently, consider extracting it into a shell function or a separate include file. But honestly, for three occurrences that rarely change, the current approach is perfectly readable—this is a "someday maybe" rather than a "must do now."
frontend/oxlintrc.json (1)
3-12: Quick thought on plugin selection 🤔The plugins include
react,react-hooks, andnextjs, but based on the project context (Svelte components likeLearnView.svelte,GuidedLessonChatPanel.svelte), this appears to be a Svelte-based frontend.These React/Next.js plugins won't cause errors, but they add slight overhead and might surface if someone accidentally uses JSX patterns. If you're keeping them for potential future React usage or shared config portability, that's totally fine! Just wanted to flag it in case they slipped in unintentionally.
frontend/package.json (3)
6-8: Exact Node version is quite strict 📌Using
"22.17.0"as an exact match means contributors must have precisely this version—even22.17.1would fail the engine check withengine-strictenabled.Consider using a semver range like
">=22.17.0"or"^22.17.0"to allow compatible patch/minor updates while still ensuring a minimum version.💡 Suggested change
"engines": { - "node": "22.17.0"+ "node": ">=22.17.0" },
17-20: Potential plugin flag redundancy in lint:ox script 🔄The
oxlintrc.jsonalready declares plugins in thepluginsarray (lines 3-12). The CLI flags like--import-plugin,--react-plugin, etc. in thelint:oxscript might be redundant since the config file should handle plugin activation.Worth a quick test: try running
oxlint -c oxlintrc.json .without the explicit plugin flags to see if behavior is identical. If so, you could simplify the script!
53-58: Heads up: type-aware linting in lint-staged can be slow ⏱️Running
oxlint --type-awareon staged files during pre-commit can be noticeably slower than regular linting since it needs to parse the full TypeScript project for type information.For small changesets this is usually fine, but if developers find commits taking too long, you might consider:
- Moving type-aware checks to CI only
- Using a lighter check for pre-commit
Just something to keep an eye on as the codebase grows!
rules/ast-grep/java-no-unchecked-cast.yml (1)
7-7: Consider bumping severity toerrorfor stricter enforcement.The coding guidelines state: "Never use
@SuppressWarningsto resolve lint issues" and "No unchecked casts,@SuppressWarningsin production code." Given this zero-tolerance stance, awarningmight let violations slip through in CI if warnings don't fail the build.That said,
warninggives teams time to migrate existing code—a pragmatic choice if you have legacy usages to clean up first! Just something to revisit once the codebase is compliant. 🧹🔧 Optional: Upgrade to error for strict enforcement
-severity: warning+severity: errorsrc/main/java/com/williamcallahan/javachat/service/RateLimitState.java (1)
97-103: Consider extracting backoff constants for clarity.The exponential backoff logic uses several inline values that could benefit from named constants, making the backoff strategy self-documenting:
- Base 2 for exponential growth
- 7-day maximum backoff ceiling
This is a "nice to have" for future readers who might wonder why these specific values were chosen. The algorithm itself is solid! 📈
💡 Optional: Extract named constants
+ private static final int BACKOFF_EXPONENT_BASE = 2;+ private static final Duration MAX_BACKOFF_DURATION = Duration.ofDays(7);+ // In recordRateLimit method: - Duration additionalBackoff = Duration.ofHours((long) Math.pow(2, failures - 1));- Duration maxBackoff = Duration.ofDays(7); // Never back off more than a week+ Duration additionalBackoff = Duration.ofHours(+ (long) Math.pow(BACKOFF_EXPONENT_BASE, failures - 1));- if (additionalBackoff.compareTo(maxBackoff) > 0) {- additionalBackoff = maxBackoff;+ if (additionalBackoff.compareTo(MAX_BACKOFF_DURATION) > 0) {+ additionalBackoff = MAX_BACKOFF_DURATION; }src/main/java/com/williamcallahan/javachat/service/RateLimitService.java (3)
17-24: Shrink-on-touch: consider extracting a seam from this >500 LOC service. A fun micro-win is movingApiEndpointStateorParsedRateLimitInfointo a smallsupportclass to keep the main service slimmer. As per coding guidelines, Large files (>500 LOC): extract only pieces you touch into clean-architecture roots; avoid broad refactors; Shrink on touch: when editing monoliths, extract at least one seam and net-decrease file size; if unsafe, stop and ask.
41-42: Extract the* 2multiplier into a named constant. It’s a quick readability win and keeps magic numbers out of the logic path.As per coding guidelines, Magic literals: no inline numbers (except 0, 1, -1) or strings; define named constants with intent-revealing names.♻️ Suggested refactor
/** Maximum backoff multiplier to cap exponential growth (32 seconds max). */ private static final int MAX_BACKOFF_MULTIPLIER = 32; + private static final int BACKOFF_MULTIPLIER_STEP = 2; ... - backoffMultiplier = Math.min(backoffMultiplier * 2, MAX_BACKOFF_MULTIPLIER);+ backoffMultiplier = Math.min(backoffMultiplier * BACKOFF_MULTIPLIER_STEP, MAX_BACKOFF_MULTIPLIER);Also applies to: 167-168
73-75: UseApiProviderenum as map key instead of string. This prevents drift between enum values and map entries while improving type safety.♻️ Suggested refactor
- private final Map<String, ApiEndpointState> endpointStates = new ConcurrentHashMap<>();+ private final Map<ApiProvider, ApiEndpointState> endpointStates = new ConcurrentHashMap<>(); ... - return endpointStates.computeIfAbsent(provider.getName(), providerKey -> new ApiEndpointState());+ return endpointStates.computeIfAbsent(provider, ignored -> new ApiEndpointState());Aligns with the guideline: "Ban map/bloated tooling: no stringly helpers." Enums as map keys are naturally hashable, faster to compare, and compiler-checked.
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.
The previous commit had log.warn() calls with long string arguments that spotless requires to be split across lines. This fixes the CI build failure by moving string arguments to their own lines with proper indentation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/williamcallahan/javachat/service/RateLimitService.java (1)
521-554: DeleteselectBestProvider()as dead code; the actual auto-fallback concern applies toselectClientForStreaming()The
selectBestProvider()method is never called anywhere in the codebase and should be removed.However, the underlying policy concern is valid: the no-auto-fallback rule applies to
selectClientForStreaming()in OpenAIStreamingService (lines 461-499). That method silently switches from OpenAI to GitHub Models when OpenAI is rate-limited, without surfacing which provider is responding to the user. Per the coding guidelines, when rate-limited, surface error to user—never silently switch providers.Either: (1) require explicit user approval before trying secondary providers, or (2) always surface which provider fulfilled the request in the response visible to end-users (not just logs).
🧹 Nitpick comments (3)
src/main/java/com/williamcallahan/javachat/service/RateLimitService.java (3)
60-70: Consider renaming to avoid the "Info" suffix 🏷️Per coding guidelines, identifiers containing generic terms like
info,data,valueare discouraged as they don't convey specific intent. The record nameParsedRateLimitInfofalls into this pattern.A more domain-specific name would make its purpose even clearer:
🔧 Suggested rename
- private record ParsedRateLimitInfo(Instant resetTime, long retryAfterSeconds) {+ private record RateLimitTiming(Instant resetTime, long retryAfterSeconds) {This also means updating usages at lines 321, 375, 410, 420, 509, 514, and 518.
321-321: Small naming refinement opportunity 🏷️The variable
rateLimitInfouses the generic "info" term. A more specific name would align better with the coding guidelines and make the code even more self-documenting.🔧 Suggested change
- ParsedRateLimitInfo rateLimitInfo = parseRateLimitFromHeaders(exception.headers());- if (rateLimitInfo.retryAfterSeconds > 0) {+ ParsedRateLimitInfo timing = parseRateLimitFromHeaders(exception.headers());+ if (timing.retryAfterSeconds > 0) {(Or
parsedTiming,resetTiming, etc.)
375-375: Rename theinfovariable 🏷️The variable name
infois on the banned list per coding guidelines. A more descriptive name helps future readers understand what they're looking at.🔧 Suggested change
- ParsedRateLimitInfo info = parseRateLimitHeaders(webError);-- if (info.hasResetTime()) {- applyRateLimit(provider, info.resetTime(), 0);+ ParsedRateLimitInfo timing = parseRateLimitHeaders(webError);++ if (timing.hasResetTime()) {+ applyRateLimit(provider, timing.resetTime(), 0);
The ast-grep rule for @RequestBody validation was rejecting valid code where @Valid appeared after @RequestBody (e.g., @RequestBody@Valid Foo foo). Spring accepts the annotation in either order, but the rule only checked for @Valid before @RequestBody. This caused false positives during linting. - Change from single 'inside' pattern to 'any' with both orderings - Matches: @Valid@RequestBody and @RequestBody@Valid
Standardize markdown table separator to have consistent spacing around dashes. This is a cosmetic change with no functional impact.
Per the no-silent-fallback policy [LM1b], users should know which LLM provider is handling their request when multiple providers are configured. Previously, provider selection was internal and invisible to end-users. This change surfaces the active provider in the SSE stream via a new 'provider' event type. Introduces StreamingResult record that wraps both the content Flux and provider metadata, allowing callers to access provider info after client selection. The ChatController and GuidedLearningController now emit a provider event at stream start, enabling the frontend to display which LLM is responding. - Add StreamingResult record with content flux and ApiProvider - Change streamResponse() return type from Flux<String> to Mono<StreamingResult> - Add EVENT_PROVIDER constant and providerEvent() method to SSE support - Update ChatController to emit provider event before streaming content - Update GuidedLearningController with same provider transparency pattern - Add .flatMapMany() in ChatService to extract content from StreamingResult - Update test mock to return StreamingResult instead of raw Flux
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/main/java/com/williamcallahan/javachat/config/QdrantHealthIndicator.java`:
- Around line 28-42: Add a Javadoc comment to the public health() method in
QdrantHealthIndicator describing its purpose and return contract, and replace
the inline detail key strings ("status" and "nextCheckIn") with intent-revealing
constants (e.g., private static final String STATUS_DETAIL_KEY and
NEXT_CHECK_IN_DETAIL_KEY) declared in the class; update all references inside
health() (the Health.up().withDetail(...) and builder.withDetail(...)) to use
these constants so there are no magic string literals.
In
`@src/main/java/com/williamcallahan/javachat/service/ExternalServiceHealth.java`:
- Line 196: Replace the inline Duration.ofSeconds(5) in ExternalServiceHealth
with a named constant: declare a private static final Duration
EXTERNAL_SERVICE_TIMEOUT (or similarly intent-revealing name) in the
ExternalServiceHealth class and use that constant in the call currently written
as .timeout(Duration.ofSeconds(5)); update any related imports or tests if
needed.
- Around line 201-209: Remove the onErrorResume that masks failures and use an
explicit error subscriber so errors surface and are handled in one place:
replace the current chain that uses
doOnError(...).onErrorResume(...).subscribe() with a subscribe(onNext=null,
onError=...) call that calls status.markUnhealthy() and the same log.warn(...)
(use the existing log message and formatDuration(status.currentBackoff)); this
ensures doOnError is not the only handler or, alternatively, drop doOnError and
perform the status.markUnhealthy() and log.warn(...) inside the subscribe error
handler to avoid duplicated handling.
🧹 Nitpick comments (2)
src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java (1)
213-249: Well-structured streaming flow! 🌟The event sequence is clear and intentional:
- Provider event first — users know who's answering
- Data + heartbeats merged — keeps connection alive while streaming
- Citations at the end — all sources in one final event
Computing citations before streaming (line 222) is smart—it ensures they're ready when the stream completes.
One small observation: the error log at line 244 says "Guided streaming error" but doesn't include the actual exception message. Consider adding
error.getMessage()for easier debugging:- log.error("Guided streaming error");+ log.error("Guided streaming error: {}", error.getMessage());This keeps sensitive stack traces out of logs while still providing actionable context.
src/main/java/com/williamcallahan/javachat/web/ChatController.java (1)
41-42: Double security annotation — belt and suspenders? 🤔Both
@PermitAlland@PreAuthorize("permitAll()")are present. They accomplish the same thing—one is Jakarta EE standard, the other is Spring Security. While harmless, it's a bit redundant. If this is intentional for documentation/clarity, that's fine! Otherwise, you could pick one:`@PermitAll` -@PreAuthorize("permitAll()")Not a blocker—just a tidbit to know!
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.
QdrantHealthIndicator: - Add Javadoc to health() describing return contract - Extract "status" and "nextCheckIn" to DETAIL_KEY_* constants ExternalServiceHealth: - Extract Duration.ofSeconds(5) to HEALTH_CHECK_TIMEOUT constant - Replace doOnError+onErrorResume+subscribe() with subscribe(onSuccess, onError) to consolidate error handling in one place instead of duplicated handling
The backend's provider transparency feature (48d6739) emits a 'provider' SSE event at stream start containing metadata like {"provider":"openai"}. The frontend SSE parser lacked a handler for this event type, causing the raw JSON to fall through to text rendering and appear in chat output. - Add ProviderEventSchema to validation schemas - Add SSE_EVENT_PROVIDER constant and onProvider callback - Add provider event handler in processEvent() to consume the event The onProvider callback is optional, allowing future UI integration to display provider info without requiring immediate changes to callers.
…ng streaming Chat interfaces were hijacking scroll control during streaming, forcing users to the bottom even when they were reading earlier content. This was disruptive UX that made it impossible to review previous messages while a response was still generating. Implements a comprehensive scroll management system with three key components: 1. Intent detection - Distinguishes user scrolling (reading) from programmatic scrolls by tracking scroll direction. Scrolling up disables auto-scroll. 2. Throttled updates - Batches rapid scroll-to-bottom calls during streaming (50ms throttle) to prevent scroll spam and animation conflicts. 3. New content indicator - Shows a floating "New content" pill when user is scrolled up and content arrives. Click to jump to bottom and re-anchor. - Add createScrollAnchor composable with reactive Svelte 5 state - Add NewContentIndicator component with terracotta accent styling - Integrate scroll anchor into ChatView replacing manual shouldAutoScroll - Integrate scroll anchor into LearnView for desktop/mobile chat panels - Update GuidedLessonChatPanel and MobileChatDrawer with indicator props - Remove unused isNearBottom/scrollToBottom imports from updated components
…y behavior The previous scroll anchor implementation fought with users by aggressively auto-scrolling during streaming, even when users scrolled up to read previous content. This created a frustrating "scroll fighting" experience where the UI ignored user intent. This change completely inverts the model: - **Before**: Auto-scroll hijacked control during streaming, tracking "intent" - **After**: User always controls scroll position; indicator appears only when new content streams off-screen, disappearing when user scrolls to ~95% bottom Key changes: - Removed all auto-scroll logic (`isAnchored`, throttling, intent detection) - Changed threshold from 100px to percentage-based (95% = indicator disappears) - Added `scrollOnce()` for single scroll on user send (no ongoing anchoring) - Extracted `clearIndicatorState()` helper to eliminate duplicate code - Updated component comments to reflect "scroll indicator" vs "auto-scroll"
The "New content" indicator had two bugs: 1. Count showed "99+" after just 1 message because onContentAdded() incremented unseenCount on every SSE streaming chunk, not once per message 2. Click to scroll didn't work because jumpToBottom() used this.clearIndicatorState() which lost binding when passed as a callback prop to child components Fix count by separating concerns: add onNewMessageStarted() to increment count once per assistant message, change onContentAdded() to only update visibility. Fix click by adding clearIndicatorStateInternal() as a closure-captured function that all methods call directly, eliminating this dependency. - Add clearIndicatorStateInternal() as internal closure function - Add onNewMessageStarted() method to increment count once per message - Modify onContentAdded() to only update indicator visibility - Remove this.clearIndicatorState() calls in favor of direct internal call - Call onNewMessageStarted() in ChatView and LearnView when assistant message starts
Previously, CSRF was disabled for all API endpoints using
csrf.ignoringRequestMatchers("/api/**"), leaving the application
vulnerable to cross-site request forgery attacks on state-changing
operations like chat streaming and session clearing.
This enables cookie-based CSRF tokens that the SPA frontend can
read via JavaScript and include in POST request headers. The
token is issued on initial page loads via a filter, ensuring
clients have the token before their first state-changing request.
- Add csrf.ts utility to read XSRF-TOKEN cookie and set X-XSRF-TOKEN header
- Integrate CSRF headers into chat.ts clearChatSession POST request
- Integrate CSRF headers into sse.ts streamSse POST request
- Configure CookieCsrfTokenRepository with Lax SameSite policy
- Create CsrfTokenCookieFilter to force token generation on safe requests
- Remove CSRF ignore pattern from API endpoints in SecurityConfigAdded explanatory Javadoc comments to CSRF_COOKIE_NAME and CSRF_HEADER_NAME constants to document their purpose and Spring Security integration. Created prek.toml configuration file for future prek-based hook management.
Fixed Palantir Java Format violations: - Consolidated method parameters onto single line in CsrfTokenCookieFilter - Consolidated csrf configuration chain onto single line in SecurityConfig
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/lib/services/csrf.ts`:
- Around line 7-22: The readCookie function currently calls
decodeURIComponent(tokenText) which can throw on malformed percent-encoding;
wrap the decoding in a try/catch inside readCookie and on error return null (or
the undecoded token as fallback) to avoid bubbling exceptions during header
generation; update the decode step in readCookie to catch any
URIError/RangeError from decodeURIComponent and handle it gracefully so callers
of readCookie (and any header generation logic) are not broken.
🧹 Nitpick comments (1)
src/main/java/com/williamcallahan/javachat/config/SecurityConfig.java (1)
80-105: Extract magic strings into named constants. SameSite and public path literals should be centralized for clarity and to comply with the magic-literals rule.♻️ Example refactor
public class SecurityConfig { private static final String WILDCARD_ORIGIN = "*"; + private static final String CSRF_COOKIE_SAMESITE = "Lax";+ private static final List<String> PUBLIC_APP_PATHS = List.of(+ "/",+ "/index.html",+ "/chat.html",+ "/guided.html",+ "/favicon.ico",+ "/app/**",+ "/assets/**",+ "/static/**"); @@ - csrfTokenRepository.setCookieCustomizer(cookie -> cookie.sameSite("Lax"));+ csrfTokenRepository.setCookieCustomizer(cookie -> cookie.sameSite(CSRF_COOKIE_SAMESITE)); @@ - .authorizeHttpRequests(auth -> auth.requestMatchers(- "/",- "/index.html",- "/chat.html",- "/guided.html",- "/favicon.ico",- "/app/**",- "/assets/**",- "/static/**")+ .authorizeHttpRequests(auth -> auth.requestMatchers(+ PUBLIC_APP_PATHS.toArray(String[]::new)) .permitAll()As per coding guidelines "Magic literals: no inline numbers (except 0, 1, -1) or strings; define named constants with intent-revealing names."
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Delivers independent lesson conversations that preserve context per lesson, real-time citation streaming for instant source visibility, and smart scroll anchoring that respects user reading position. Adds comprehensive Zod runtime validation for API type safety and provider transparency showing which LLM handles requests.
Changes by Category
Features
GuidedStreamRequest.java,guided.ts)GuidedLearningController.java,LessonCitations.svelte)createScrollAnchor.svelte.ts,NewContentIndicator.svelte)StreamingResult.java,SseConstants.java)schemas.ts,validate.ts)chatMessageId.ts)chat.ts)Bug Fixes
docker-compose-qdrant.yml)RateLimitService.java)GuidedTOCProvider.java)MessageBubble.svelte)ErrorTestController.java)Refactoring & Architecture
RetrievedContentinterface to decouple domain from Spring AI (RetrievedContent.java,DocumentContentAdapter.java)SearchQualityLevel.java)LearnViewintoGuidedLessonChatPanelfor desktop chat panel (~150 lines extracted)PdfCitationEnhancer.java)RateLimitManager→RateLimitService,StateData→PersistedState,ServiceInfo→HealthSnapshotList.copyOf()in constructorsBuild & Tooling
make format[DM1f]naming bans and[TY1]type safety (8 rules)Documentation
docs/getting-started.md,docs/api.md,docs/architecture.md, etc.docs/type-safety-zod-validation.mddocumenting [FV1] validation rules[GT1a],[FV1a-h]) for stable rule referencesdocs/contracts/code-change.mdfor contribution standardsTesting
streamSse()handles AbortSignal cancellation without error callbacksBreaking Changes
Technical Details