feat(app): add guided learning, inline citations, and unified streaming - #6
Merged
Conversation
…nses Assistant responses now show their source citations, enabling users to verify information and explore related documentation. The panel categorizes citations by type (PDF, API docs, repository, external links) with distinctive icons and styling for each. Citations are fetched asynchronously based on the user's query and deduplicated by URL to avoid showing the same source multiple times. - Create CitationPanel component with type-aware icons and styling - Track user query on assistant messages for citation lookup - Integrate CitationPanel below non-error assistant messages in ChatView - Add wrapper styling to support citation panel layout
Implement the service layer for guided learning functionality. This provides typed interfaces and API functions for fetching the lesson table of contents, individual lesson metadata, lesson content as markdown, and streaming chat responses scoped to a specific lesson context. - Define GuidedLesson and LessonContentResponse interfaces - Add fetchTOC, fetchLesson, and fetchLessonContent for lesson data - Implement streamGuidedChat with SSE parsing, error handling, and cleanup - Add GuidedCitation type and fetchGuidedCitations for lesson-specific sources
Replace the "coming soon" placeholder with a full guided learning interface. Users can browse lessons from a table of contents, view lesson content with syntax-highlighted code blocks, and ask questions in a contextual chat panel that provides lesson-specific responses. - Create LearnView component with TOC grid and lesson reader - Add two-column layout: lesson content panel with markdown rendering - Integrate chat panel scoped to selected lesson context - Support code highlighting with highlight.js for Java, XML, JSON, bash - Wire up App.svelte to render LearnView instead of placeholder - Add placeholder prop to ChatInput for contextual prompts
Contributor
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a guided learning UI with per-lesson streaming chat and inline citations, introduces unified SSE streaming utilities on frontend and backend, new citation and thinking UI components, and multiple frontend/backend utilities and parsing improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend (LearnView / ChatView)
participant SSE as SSE Stream (streamSse)
participant Backend as Backend (SseSupport + Controller)
participant OpenAI as OpenAI
participant Vector as Vector Store
User->>Frontend: Open LearnView / select lesson
Frontend->>Backend: GET TOC / fetchLessonContent
Backend->>Vector: query lessons/content
Vector-->>Backend: lessons/markdown
Backend-->>Frontend: GuidedLesson / Markdown
User->>Frontend: Send chat message
Frontend->>Frontend: append user message, set streaming UI
Frontend->>SSE: POST streamGuidedChat (streamSse)
rect rgba(100, 200, 255, 0.5)
SSE->>Backend: deliver SSE POST
Backend->>OpenAI: stream with retrieval/context
OpenAI-->>Backend: text chunks + citation info
Backend->>Backend: prepare text/status/citation events via SseSupport
Backend-->>SSE: emit SSE events (text/status/citation + heartbeats)
end
SSE-->>Frontend: processEvent -> onText/onStatus/onCitations
Frontend->>Frontend: render chunks, accumulate citations, show ThinkingIndicator
Frontend->>Frontend: finalize assistant message and show CitationPanel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/src/lib/components/LearnView.svelte`:
- Around line 66-89: The selectLesson function can suffer a race where an
earlier fetch resolves after a later one and overwrites state; fix by capturing
the requested slug locally (e.g., const requestSlug = lesson.slug) at the top of
selectLesson and, after each async response (both fetchLessonContent and
fetchCitations), check that the current selected identifier
(selectedLesson?.slug or a dedicated selectedSlug store) still equals
requestSlug before assigning lessonMarkdown, lessonCitations, lessonError, or
changing loadingLesson; alternatively implement a cancellable token that is
compared before applying results so only the latest request updates state.
In `@frontend/src/lib/services/guided.ts`:
- Around line 149-154: The error extraction assumes a trailing space by using
payload.slice(8) after the startsWith('[ERROR]') check, which will drop the
first character when the server sends '[ERROR]No space here'; update the
extraction to use payload.slice(7).trim() (keep the same startsWith('[ERROR]')
guard and the existing onError?.(serverError) and throw serverError behavior) so
you correctly capture the message whether or not there is a space after the
marker.
🧹 Nitpick comments (3)
frontend/src/lib/components/ChatView.svelte (1)
58-61: Quick observation oncurrentUserQuery🤔I notice
currentUserQueryis set on line 61 but doesn't appear to be used anywhere in the template or other functions—only reset in thefinallyblock. If it's reserved for future features (like showing "Answering: {query}" during streaming), that's cool! Otherwise, you could simplify by removing it sinceuserQuerylocal variable already tracks the query through the async flow.frontend/src/lib/components/CitationPanel.svelte (1)
101-131: Potential race condition on rapid query changes 🏃♂️Here's a fun learning moment! If the user types quickly (triggering multiple query changes), older fetch responses could arrive after newer ones and overwrite the correct citations. Consider using an
AbortControllerto cancel stale requests:🔧 Optional enhancement with AbortController
$effect(() => { const currentQuery = query if (!currentQuery || !currentQuery.trim()) { citations = [] hasFetched = false return } + const controller = new AbortController() loading = true hasFetched = false - fetchCitations(currentQuery)+ fetchCitations(currentQuery, { signal: controller.signal }) .then((result) => { // Deduplicate by URL const seen = new Set<string>() citations = result.filter((citation) => { const key = citation.url?.toLowerCase() ?? '' if (seen.has(key)) return false seen.add(key) return true }) }) .catch((fetchError) => { + if (fetchError.name === 'AbortError') return console.warn('Failed to fetch citations:', fetchError) citations = [] }) .finally(() => { loading = false hasFetched = true }) + return () => controller.abort() })This is a nice-to-have—in practice, citation fetches are usually fast enough that users won't notice!
frontend/src/lib/components/LearnView.svelte (1)
11-14: DuplicateMessageWithQuerytype 📋I spy with my little eye... this same interface in
ChatView.svelte! Consider extracting it to a shared types file (liketypes.tsor alongside thechatservice) to keep things DRY. Not urgent, but it'll save future-you from updating two places!💡 Example shared location
// In frontend/src/lib/services/chat.ts or a new types.tsexportinterfaceMessageWithQueryextendsChatMessage{queryForCitations?: string}Then import it in both components.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…cking Citation fetching was masking failures by silently returning empty arrays, making it impossible to distinguish "no results" from "fetch failed". This refactors error handling to surface failures explicitly while extracting magic strings into named constants and simplifying type detection logic. - Add fetchError state to CitationPanel with error indicator UI - Return Result type from fetchGuidedCitations for explicit success/failure - Extract URL protocol constants (HTTP, HTTPS, LOCAL_PATH_PREFIX, etc.) - Replace 6+ case if-else with pattern-matching helpers (matchesPatterns) - Log lesson citation errors with context instead of swallowing silently - Remove unused currentUserQuery from ChatView (dead code cleanup)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:3cb16b978b
ℹ️ 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.
Pull request overview
Adds a Guided Learning experience to the frontend, including lesson TOC browsing, lesson content rendering, and an in-context chat UI with per-answer citations.
Changes:
- Introduces a guided-learning API client (
guided.ts) for TOC, lesson content, citations, and streaming chat. - Adds a new
LearnViewUI for selecting lessons, viewing lesson markdown, and chatting about a lesson. - Adds a reusable
CitationPaneland integrates it into bothChatViewandLearnView; enhancesChatInputto accept a configurable placeholder.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/lib/services/guided.ts | New guided-learning service wrapper, including an SSE streaming helper for guided chat. |
| frontend/src/lib/components/LearnView.svelte | New guided learning UI (TOC + lesson + chat layout) with markdown rendering and code highlighting. |
| frontend/src/lib/components/CitationPanel.svelte | New component to fetch and render citations for a query. |
| frontend/src/lib/components/ChatView.svelte | Adds citations rendering per assistant message and tracks query per response. |
| frontend/src/lib/components/ChatInput.svelte | Adds placeholder prop support with a default value. |
| frontend/src/App.svelte | Replaces “coming soon” guided-learning placeholder with the real LearnView. |
💡 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.
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 guided learning stream endpoint was sending plain text SSE events while ChatController uses JSON-wrapped events. This inconsistency caused whitespace to be stripped from guided chat responses. By matching the ChatController pattern, whitespace in code blocks and formatted content is now preserved. Backend changes (GuidedLearningController): - Inject ObjectMapper and add JSON serialization helpers - Return Flux<ServerSentEvent<String>> instead of Flux<String> - Wrap text chunks in ChunkMessage records with proper event types - Use structured ErrorMessage records for error events - Fix backpressure handling (onBackpressureBuffer + share) - Add SSE event type constants for consistency Frontend changes (guided.ts): - Remove unused GuidedCitation type and CitationFetchResult (dead code) - Add event type tracking and multi-line event buffering - Parse JSON-wrapped payloads via tryParseJson helper - Handle "error" event type with proper error propagation
…son sources The citation panel was always expanded, taking up vertical space even when users didn't need to reference sources. The lesson-level citation fetch was also discarding results without displaying them. This redesigns the panel as a collapsible pill and completes the lesson sources feature. CitationPanel.svelte: - Convert from always-visible panel to collapsible disclosure button - Replace if/else type detection with rule-based TYPE_DETECTION_RULES array - Add expand/collapse state with animated chevron rotation - Modernize styling with smaller badges, refined spacing, dark mode support - Remove rarely-used snippet display for cleaner UI LearnView.svelte: - Add lessonCitations state to store fetched results (was discarding them) - Add lessonCitationsLoaded flag for proper loading state - Add lessonCitationsError for error visibility (not silent) - Display "Lesson Sources" section below lesson content - Reset citation state on lesson change and back navigation markdown.ts: - Clarify whitespace preservation comment in enrichment extension
railway-appBottemporarily deployed
to
insightful-intuition / production
January 24, 2026 09:00
Inactive
CitationPanel.svelte: - Restore API_DOC_PATTERNS and REPO_PATTERNS named constants (CS7) - Replace TYPE_DETECTION_RULES engine with simple if/else (KISS principle) - Remove duplicate function definitions GuidedLearningController.java: - Add comment explaining why SSE streams need error events - Sanitize error details sent to client for security
railway-appBottemporarily deployed
to
insightful-intuition / production
January 24, 2026 09:01
Inactive
When SSE streams end, multi-byte UTF-8 characters (emoji, CJK) may be split across network chunks. The TextDecoder holds partial bytes internally when using stream mode. Without a final flush via decode() without arguments, these trailing bytes would be silently dropped, corrupting the last character. - Add decoder.decode() call on stream completion in chat.ts - Add decoder.decode() call on stream completion in guided.ts - Append flushed bytes to buffer before processing remaining content
PR review identified multiple issues: async citation fetches could resolve out of order causing stale data to overwrite fresh results; URLs from the backend lacked scheme validation allowing potential XSS via javascript:, data:, or vbscript: URIs; highlight.js effect cleanup didn't properly detect unmount during async load. Race condition fix: - Add pendingRequestId counter to track active requests - Ignore responses from superseded requests in CitationPanel XSS prevention: - Add sanitizeUrlScheme() to validate URLs use http/https or relative paths - Apply sanitization in buildFullUrl() before rendering href attributes - Add sanitizeCitationUrl() to LearnView for lesson source links Effect cleanup fix: - Add isCancelled flag with cleanup function in highlight.js effect - Verify element reference hasn't changed during async load
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@frontend/src/lib/components/CitationPanel.svelte`:
- Around line 153-174: The fixed id "citation-list" causes duplicate IDs when
multiple CitationPanel instances render; change to a unique per-instance id
(e.g., build an id string in the component and use it where "citation-list"
appears). Generate the id in the component (module-level counter or random
suffix) and replace the hard-coded id in the <ul id="citation-list"> and the
button's aria-controls="citation-list" so both reference the generated id; keep
all existing bindings (isExpanded, citations) untouched and ensure the same
generated id is used for both attributes.
In `@frontend/src/lib/components/LearnView.svelte`:
- Around line 111-184: When starting a stream in handleSend, capture the lesson
slug and an AbortController (or similar cancel token) and pass it into
streamGuidedChat; in the onChunk/onError handlers and before appending the final
assistant message check that the current selectedLesson.slug (or the captured
slug) still matches and/or that the controller hasn't been aborted so chunks
aren't applied to a different view, and abort the controller in goBack to cancel
any in‑flight stream and also reset isStreaming/currentStreamingContent there;
update references to selectedLesson.slug, currentStreamingContent, isStreaming,
streamGuidedChat, handleSend and goBack accordingly to implement the guard and
cancellation.
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Around line 77-85: Rename the jsonSerialize method parameter from the banned
identifier "value" to a domain-specific name like "payload" and replace the
generic RuntimeException with a more specific exception (e.g.,
IllegalStateException) to improve diagnostics; update the method signature for
jsonSerialize(Object payload), keep the objectMapper.writeValueAsString(payload)
call, and change the catch block to throw new IllegalStateException("Failed to
serialize SSE data", e); update any local usages/call sites of jsonSerialize
accordingly to use the new parameter name.
- Around line 43-46: The SSE error event constant SSE_EVENT_ERROR uses the
non-standard value "error" which isn't in the allowed event taxonomy; change the
constant name/value to align with the taxonomy (e.g., rename SSE_EVENT_ERROR to
SSE_EVENT_STATUS and set its value to "status") and update all references in
this class (and any callers) to use SSE_EVENT_STATUS; keep SSE_EVENT_TEXT as-is
and ensure emitted SSE events use the standardized names: "text", "citation",
"code", "enrichment", "suggestion", "status".
- Around line 244-248: The Flux created in GuidedLearningController (variable
dataStream from openAIStreamingService.streamResponse) uses
onBackpressureBuffer() with no bound—change it to onBackpressureBuffer(512,
/*queueSupplier*/ null, BufferOverflowStrategy.DROP_OLDEST) (or DROP_LATEST if
you prefer) to cap queued chunks and avoid unbounded memory growth; apply the
same change to the identical stream creation in ChatController so both
controllers use a 512-capacity bounded buffer with an explicit overflow
strategy.
♻️ Duplicate comments (3)
frontend/src/lib/components/LearnView.svelte (1)
68-108: Guard against stale lesson fetches when switching.
A slower response can overwrite a newer selection; capture the target slug and verify before updating state (and mirror the guard in the citation fetch). Fun tidbit: async races love fast clickers.🔧 Suggested guard using the selected slug
async function selectLesson(lesson: GuidedLesson): Promise<void> { + const targetSlug = lesson.slug // Reset state atomically before async operation selectedLesson = lesson loadingLesson = true lessonMarkdown = '' lessonError = null lessonCitations = [] lessonCitationsError = null lessonCitationsLoaded = false messages = [] try { const response = await fetchLessonContent(lesson.slug) + if (selectedLesson?.slug !== targetSlug) return lessonMarkdown = response.markdown ... } catch (error) { + if (selectedLesson?.slug !== targetSlug) return lessonError = error instanceof Error ? error.message : 'Failed to load lesson' lessonMarkdown = '' } finally { - loadingLesson = false+ if (selectedLesson?.slug === targetSlug) {+ loadingLesson = false+ } } }frontend/src/lib/components/CitationPanel.svelte (2)
104-110: Block unsafe URL schemes before rendering links.
Citation URLs cross a trust boundary; without scheme validation,javascript:links can slip through. Fun tidbit: validatinghrefis a quick win against stored XSS.🛡️ Suggested URL scheme guard
+function isSafeCitationUrl(url: string): boolean {+ const lower = url.toLowerCase()+ return lower.startsWith(URL_SCHEME_HTTP)+ || lower.startsWith(URL_SCHEME_HTTPS)+ || lower.startsWith(LOCAL_PATH_PREFIX)+}+ function buildFullUrl(citation: Citation): string { if (!citation.url) return FALLBACK_LINK_TARGET + if (!isSafeCitationUrl(citation.url)) return FALLBACK_LINK_TARGET if (citation.anchor && !citation.url.includes(ANCHOR_SEPARATOR)) { return `${citation.url}${ANCHOR_SEPARATOR}${citation.anchor}` } return citation.url }
116-149: Guard citation state against out‑of‑order fetches.
Rapid query changes can let older results overwrite newer ones. Fun tidbit: a simple request counter is a reliable async seatbelt.🧵 Suggested request guard
let citations = $state<Citation[]>([]) let hasFetched = $state(false) let fetchError = $state<string | null>(null) let isExpanded = $state(false) +let requestId = 0 $effect(() => { const currentQuery = query + const currentRequest = ++requestId if (!currentQuery || !currentQuery.trim()) { citations = [] hasFetched = false fetchError = null isExpanded = false return } hasFetched = false fetchError = null isExpanded = false fetchCitations(currentQuery) .then((result) => { + if (currentRequest !== requestId) return // Deduplicate by URL const seen = new Set<string>() citations = result.filter((citation) => { const key = citation.url?.toLowerCase() ?? '' if (seen.has(key)) return false seen.add(key) return true }) }) .catch((error: unknown) => { + if (currentRequest !== requestId) return const errorMessage = error instanceof Error ? error.message : 'Failed to fetch citations' fetchError = errorMessage citations = [] }) .finally(() => { - hasFetched = true+ if (currentRequest === requestId) {+ hasFetched = true+ } }) })
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.
railway-appBottemporarily deployed
to
insightful-intuition / production
January 24, 2026 09:09
Inactive
Multiple rapid lesson selections could cause stale fetch responses to overwrite fresh content. Similarly, navigating away during streaming could leave orphaned chunks updating the wrong lesson context. This adds slug-based guards throughout the lesson loading and chat streaming workflows. Lesson selection guards: - Capture targetSlug before async operations - Check selectedLesson.slug matches before applying content, citations, errors - Skip state updates if user switched lessons during fetch Stream navigation guards: - Capture streamLessonSlug at stream start - Guard onChunk handler to ignore chunks after navigation - Guard message append and finally block with slug comparison - Reset streaming state in goBack() to cancel in-flight streams
…licts Multiple CitationPanel instances on the same page shared a hardcoded "citation-list" ID, breaking ARIA associations and causing DOM conflicts. Each component instance now generates a unique ID using a random suffix, ensuring correct aria-controls binding between trigger and list elements.
…ffer PR review identified several issues: SSE error events used non-standard "error" type instead of the allowed "status" taxonomy; jsonSerialize used banned identifier "value" and generic RuntimeException; unbounded onBackpressureBuffer could cause memory growth with slow clients. - Rename SSE_EVENT_ERROR to SSE_EVENT_STATUS per allowed taxonomy - Rename jsonSerialize parameter from "value" to "payload" - Replace RuntimeException with IllegalStateException for specificity - Add STREAM_BACKPRESSURE_BUFFER_SIZE constant (512) matching codebase convention - Apply bounded buffer to streaming Flux
The streaming response Flux used unbounded onBackpressureBuffer() which could accumulate indefinitely if SSE clients consume slowly. This adds a 512-element bound matching the convention used elsewhere in the codebase (SitemapController, RobotsController). The existing onErrorResume handler provides graceful overflow handling.
railway-appBottemporarily deployed
to
insightful-intuition / production
January 24, 2026 09:14
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Around line 76-105: Extract the inline fallback JSON in sseError into a named
constant (e.g., private static final String SSE_ERROR_FALLBACK_JSON =
"{\"message\":\"Error serialization failed\",\"details\":\"See server logs\"}"),
replace the literal in sseError(...) with that constant, and keep the constant
near the other helper records (ChunkMessage, ErrorMessage) so intent is clear
and reusable; ensure the constant is used only for the serialization fallback
path and update any references to the literal accordingly.
♻️ Duplicate comments (1)
frontend/src/lib/components/CitationPanel.svelte (1)
163-171: Invalidate pending requests when the query clears.
Right now, an in‑flight fetch can still populate citations after the query becomes empty.🔧 Suggested fix
if (!currentQuery || !currentQuery.trim()) { + pendingRequestId += 1 citations = [] hasFetched = false fetchError = null isExpanded = false return }Also applies to: 174-207
Uh oh!
There was an error while loading. Please reload this page.
Per CS7 (magic literals), the inline JSON string for SSE error fallback was extracted to SSE_ERROR_FALLBACK_JSON constant for clarity and reuse.
railway-appBottemporarily deployed
to
insightful-intuition / production
January 24, 2026 09:23
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java`:
- Line 251: Extract the magic literal 0.7 used in the call to
openAIStreamingService.streamResponse(fullPrompt, 0.7) into a clearly named
constant (e.g., LLM_TEMPERATURE or GUIDED_LEARNING_TEMPERATURE) declared near
other LLM configuration constants or at the top of GuidedLearningController;
replace the inline literal with that constant to make intent and tuning easier
and keep all LLM settings grouped for future adjustments.
🧹 Nitpick comments (1)
src/main/java/com/williamcallahan/javachat/web/GuidedLearningController.java (1)
217-217: Consider a more domain-specific variable name.The variable
textis a bit generic—per coding guidelines, identifiers should be intent-revealing. Something likemarkdownContentorlessonMarkdownwould make it instantly clear what this holds. Tiny learning nugget: descriptive names save future-you (and teammates) precious brain cycles! 🧠🔧 Suggested tweak
- String text = String.join("", chunks);- guidedService.putLessonCache(slug, text);- return text;+ String markdownContent = String.join("", chunks);+ guidedService.putLessonCache(slug, markdownContent);+ return markdownContent;
Uh oh!
There was an error while loading. Please reload this page.
Exception response formatting was guarded with null checks that don’t apply to current API contracts. This simplifies the formatting code and uses Optional to normalize missing headers. - Assume non-blank status/body strings only after blank checks - Normalize headers with Optional - Always append OpenAI response body
railway-appBottemporarily deployed
to
insightful-intuition / production
January 25, 2026 01:54
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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/DocumentFactory.java (1)
38-50: Unintended NPE risk:Map.of()rejects null beforecreateDocumentWithOptionalIdcan handle it gracefully.
createDocumentWithOptionalIdalready checks for null/blank hash, but the metadata map built just before tries to insert hash unconditionally viaMap.of(), which throws NPE if hash is null. While call sites appear to always generate hash, the method signature accepts nullableString, signaling this pattern should be defensive.Switch to a mutable map and only add hash when it's present—aligns with the learning that
Map<String, Object>is acceptable for Document metadata:💡 Suggested fix
- Map<String, ?> metadata = Map.of(- "url", url,- "title", title,- "chunkIndex", chunkIndex,- "package", packageName,- "hash", hash- );+ Map<String, Object> metadata = new HashMap<>();+ metadata.put("url", url);+ metadata.put("title", title);+ metadata.put("chunkIndex", chunkIndex);+ metadata.put("package", packageName);+ if (hash != null && !hash.isBlank()) {+ metadata.put("hash", hash);+ }Also applies to: 117-121
🤖 Fix all issues with AI agents
In `@docs/domains/local-store-directories.md`:
- Around line 3-5: Clarify the wording in the LocalStoreService documentation so
it distinguishes startup failures from runtime errors: update the text that
currently says the app "fails to start and endpoints like `/api/guided/toc`
return 500" to state that invalid/unwritable configured directories cause the
app to fail to start (making endpoints unreachable), and separately note that if
permissions change after startup the service may return 500s for endpoints such
as `/api/guided/toc`; keep reference to LocalStoreService, configured
directories, and the `/api/guided/toc` endpoint for clarity.
In `@spotbugs-exclude.xml`:
- Around line 78-85: The current SpotBugs suppression is too broad; either
narrow it to only the verified classes (replace the wildcard Match with explicit
<Class> entries for RetrySupport, RateLimitManager, GuidedLearningService and
RateLimitState if needed) or introduce a single LogSanitizer/LogHelper utility
that centralizes sanitizeLogValue(), sanitizeForLogText(), sanitizeLogMessage()
and update all logging call sites to use that helper, then remove the broad
com.williamcallahan.javachat.* suppression so SpotBugs can still catch
CRLF_INJECTION_LOGS in new classes.
In
`@src/main/java/com/williamcallahan/javachat/service/markdown/InlineListParser.java`:
- Around line 149-159: The parser currently splits trailing text on the first
punctuation even inside tokens (e.g., "1.8") because extractTrailingText /
findTrailingTextStart splits on any punctuation; update findTrailingTextStart so
it only treats a punctuation as a split point when it is followed by whitespace
(or end-of-string) to avoid breaking decimals/abbreviations, then use that
improved logic where EntryTextSplit is created (the branch around
markers/markerIndex and the other places that call extractTrailingText or rely
on trailingText, e.g., the other parsing blocks that build EntryTextSplit and
set trailingText) so digits and abbreviations remain in the main entryText
instead of being moved into trailingText.
In
`@src/main/java/com/williamcallahan/javachat/service/markdown/OrderedMarkerScanner.java`:
- Around line 113-129: finalizeMarker currently only advances afterIndex by one
when it sees a single space, which makes markers like "1. Item" invalid; update
the logic in finalizeMarker to skip all contiguous whitespace characters after
sequenceEnd (use a loop advancing afterIndex while afterIndex < text.length()
and Character.isWhitespace(text.charAt(afterIndex))). Ensure this change
preserves downstream checks (e.g., calls to isContentStartValid and the
MarkerMatch return) and still handles end-of-string correctly for
InlineListOrderedKind and MarkerMatch creation.
In `@src/main/java/com/williamcallahan/javachat/service/RateLimitManager.java`:
- Around line 352-369: The applyRateLimit method can NPE if resetTime is null
and retryAfterSecondsOverride == 0; update applyRateLimit(ApiProvider provider,
Instant resetTime, long retryAfterSecondsOverride) to compute retryAfterSeconds
as: if retryAfterSecondsOverride > 0 use it, else if resetTime != null compute
Duration.between(Instant.now(), resetTime).getSeconds(), else set
retryAfterSeconds = 0. Leave the
rateLimitState.recordRateLimit(provider.getName(), resetTime, ...) call as-is
(it can receive a null resetTime) and continue to call
state.recordRateLimit(retryAfterSeconds) and the log using the sanitized
providerName.
In `@src/main/java/com/williamcallahan/javachat/web/ChatController.java`:
- Around line 56-60: Replace the scattered `@Value` fields in ChatController
(localEmbeddingServerUrl and localEmbeddingEnabled) by injecting the existing
AppProperties and using its LocalEmbedding config; remove the two `@Value` fields,
add an AppProperties dependency to the ChatController (constructor or field
injection), and replace usages of localEmbeddingEnabled and
localEmbeddingServerUrl with appProperties.getLocalEmbedding().isEnabled() and
appProperties.getLocalEmbedding().getServerUrl() respectively so all
app.local-embedding.* reads come from the centralized LocalEmbedding config
object.
🧹 Nitpick comments (5)
src/test/java/com/williamcallahan/javachat/service/OpenAIStreamingServiceTest.java (1)
19-57: Consider testing observable behavior rather than private internals via reflection.A fun tidbit: tests that poke at private methods are sometimes called "white-box" tests—they peer inside the box! The trade-off is that they can shatter when you refactor internals, even if external behavior stays rock solid.
Per the coding guidelines ("Assert observable behavior" and "Refactor-resilient"), these tests would be more durable if they verified retry behavior through the public streaming API—e.g., confirming that a 429 triggers a retry attempt or that an IO error results in fallback behavior. That way, renaming or restructuring
isRetryablePrimaryFailurewon't break your tests.That said, if isolating this classification logic is intentional for focused unit coverage, this approach is pragmatic—just be aware of the coupling.
src/main/java/com/williamcallahan/javachat/support/RetrySupport.java (1)
120-125: Solid CRLF sanitization for log safety.This handles the primary log injection attack vector. For extra defense-in-depth (optional), you could consider escaping additional control characters like
\t,\b,\f, or even ANSI escape sequences (\u001B) that could affect log readability or formatting in certain terminals. That said, CRLF is the critical vector for log forging, so this is already effective!♻️ Optional: broader control character sanitization
private static String sanitizeLogValue(String rawValue) { if (rawValue == null) { return "null"; } - return rawValue.replace("\r", "\\r").replace("\n", "\\n");+ return rawValue+ .replace("\r", "\\r")+ .replace("\n", "\\n")+ .replace("\t", "\\t")+ .replace("\u001B", "\\u001B"); // ANSI escape }src/main/java/com/williamcallahan/javachat/service/markdown/MarkdownNormalizer.java (1)
113-172: Replace the inline digit cap with a named constant.
The3limit inisNumericHeaderreads as a magic literal. A named constant will clarify intent and align with style rules.♻️ Suggested refactor
final class MarkdownNormalizer { private MarkdownNormalizer() {} private static final int INDENTED_CODE_BLOCK_SPACES = 4; + private static final int MAX_NUMERIC_HEADER_DIGITS = 3; @@ - if (digitIndex == 0 || digitIndex > 3 || digitIndex >= trimmedLine.length()) {+ if (digitIndex == 0 || digitIndex > MAX_NUMERIC_HEADER_DIGITS || digitIndex >= trimmedLine.length()) { return false; }As per coding guidelines, avoid magic literals by introducing named constants.
src/main/java/com/williamcallahan/javachat/service/markdown/InlineListParser.java (1)
38-50: Name the HTML tag literals as constantsThe
"li","ol", and"ul"literals are domain policy knobs and repeated; naming them improves clarity and keeps the “no magic literals” rule crisp. Tiny tip: HTML tags are part of your domain vocabulary, so naming them pays off.♻️ Suggested refactor
final class InlineListParser { private static final int MAX_NESTED_DEPTH = 3; private static final int MIN_MARKER_COUNT = 2; private static final int BULLET_MARKER_WIDTH = 2; private static final int COLON_BACKTRACK_OFFSET = 2; + private static final String TAG_ORDERED_LIST = "ol";+ private static final String TAG_UNORDERED_LIST = "ul";+ private static final String TAG_LIST_ITEM = "li"; @@ - for (String entryLabel : parse.primaryBlock().entryLabels()) {- listElement.appendChild(new Element("li").text(entryLabel));+ for (String entryLabel : parse.primaryBlock().entryLabels()) {+ listElement.appendChild(new Element(TAG_LIST_ITEM).text(entryLabel)); } @@ - for (String entryLabel : nestedParse.primaryBlock().entryLabels()) {- nestedListElement.appendChild(new Element("li").text(entryLabel));+ for (String entryLabel : nestedParse.primaryBlock().entryLabels()) {+ nestedListElement.appendChild(new Element(TAG_LIST_ITEM).text(entryLabel)); } @@ - Block primaryBlock = new Block("ol", entryLabels);+ Block primaryBlock = new Block(TAG_ORDERED_LIST, entryLabels); @@ - Block primaryBlock = new Block("ul", entryLabels);+ Block primaryBlock = new Block(TAG_UNORDERED_LIST, entryLabels);As per coding guidelines, avoid magic literals in production code.
Also applies to: 66-69, 164-201
src/main/java/com/williamcallahan/javachat/service/RateLimitManager.java (1)
163-168: Minor inconsistency: mixed static and instance usage ofRateLimitHeaderParser.The constructor instantiates
headerParseras an instance field (line 166), butformatDurationis called statically (line 451). Both approaches work, but it's a bit inconsistent. IfformatDurationis a static utility method while other parser methods require instance state, this is intentional—just something to be aware of for future readers.No action needed unless you want to align on one pattern.
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@spotbugs-exclude.xml`:
- Around line 73-83: The SpotBugs suppression includes a blanket <Bug
pattern="SPRING_ENDPOINT"/> Match that overrides the targeted controller
suppressions; remove the redundant global Match (the standalone <Bug
pattern="SPRING_ENDPOINT"/> block) so the regex-scoped Match for controllers
(Class name="~com\\.williamcallahan\\.javachat\\.web\\.(.*Controller)$") and the
specific test controller entry (Class
name="com.williamcallahan.javachat.web.MarkdownApiIntegrationTest$TestMarkdownController")
remain effective, or alternatively merge the test controller into the regex
Match if you intend a single rule.
- Around line 97-110: The spotbugs-exclude.xml contains global <Match> entries
that suppress EI_EXPOSE_REP and EI_EXPOSE_REP2 without class filters; remove the
two blanket <Match> blocks that have <Bug pattern="EI_EXPOSE_REP"/> and <Bug
pattern="EI_EXPOSE_REP2"/> (the ones with no <Class> child) or replace them with
scoped <Class
name="~com\\.williamcallahan\\.javachat\\.(config|domain|web)\\..*"/> entries to
match the package-limited rules already present, ensuring only the intended
packages are excluded rather than suppressing these warnings project-wide.
In `@src/main/java/com/williamcallahan/javachat/service/RetrievalService.java`:
- Around line 241-243: The access to docs.get(0).getMetadata() in
RetrievalService can return null and cause an exception; replace direct usage
with a null-safe check (e.g., wrap getMetadata() with
Optional.ofNullable(...).orElse(Collections.emptyMap()) or an explicit null
check) so that metadataSize is computed from a non-null Map and docText remains
unaffected; update the variables metadata and metadataSize (and any downstream
usage that assumes non-null metadata) to use the safe Map instead of risking a
null pointer from getMetadata().
In
`@src/main/java/com/williamcallahan/javachat/web/ExceptionResponseBuilder.java`:
- Around line 107-110: The code calls exception.getStatusText() without checking
for null, risking an NPE in ExceptionResponseBuilder; change the logic around
the statusText variable so you null-check (or use a safe string check) before
calling isBlank(), e.g., retrieve statusText from exception.getStatusText(),
ensure it's not null and not blank, then append it to details (reference:
ExceptionResponseBuilder, the exception.getStatusText() call, the statusText
local variable, and details.append invocation).
- Line 127: The current ExceptionResponseBuilder unconditionally appends
exception.body() via details.append(", body=").append(exception.body()), which
can produce noisy ", body=null" or empty values; update the code in
ExceptionResponseBuilder to check that exception.body() is non-null and
non-empty (e.g., not blank) before appending the ", body=" segment so it only
adds body information when meaningful.
- Around line 89-92: The call to exception.getStatusText() in
ExceptionResponseBuilder can return null, so calling statusText.isBlank() may
throw an NPE; update the code that builds the details string to null-safe check
the status text (e.g., test for statusText != null && !statusText.isBlank() or
use a utility like StringUtils.hasText/hasLength) before appending, referencing
the local variable statusText and the method
RestClientResponseException.getStatusText() so only non-null, non-blank status
text is appended to details.
♻️ Duplicate comments (2)
spotbugs-exclude.xml (1)
85-91: Global CRLF_INJECTION_LOGS suppression is overly broad.This blanket suppression hides potential log injection vulnerabilities across the entire codebase. As noted in a prior review, sanitization utilities exist but are scattered—future logging additions may inadvertently skip sanitization without SpotBugs catching it.
Consider narrowing this to the specific classes where sanitization has been verified (e.g.,
RetrySupport,RateLimitManager,GuidedLearningService), or centralize log sanitization into a single helper so the broad exclusion becomes defensible.src/main/java/com/williamcallahan/javachat/service/RateLimitManager.java (1)
344-369: Guard against nullresetTimebefore Duration.between.Duration.between(Instant.now(), resetTime)will NPE ifresetTimeis ever null. Even if callers guard today, a defensive branch keeps future changes safe and avoids a hard crash.🛡️ Defensive fix
- long retryAfterSeconds = retryAfterSecondsOverride > 0- ? retryAfterSecondsOverride- : Math.max(0, Duration.between(Instant.now(), resetTime).getSeconds());+ long retryAfterSeconds;+ if (retryAfterSecondsOverride > 0) {+ retryAfterSeconds = retryAfterSecondsOverride;+ } else if (resetTime != null) {+ retryAfterSeconds = Math.max(0, Duration.between(Instant.now(), resetTime).getSeconds());+ } else {+ retryAfterSeconds = 0;+ }
🧹 Nitpick comments (3)
spotbugs-exclude.xml (1)
112-121: Global IMPROPER_UNICODE suppression duplicates the scoped rule.Lines 115-118 already suppress
IMPROPER_UNICODEfor all classes undercom.williamcallahan.javachat.*. The second global match at lines 119-121 (no class filter) is redundant and would suppress warnings from any package—including third-party code or test helpers where Unicode issues might actually matter.Keeping just the scoped rule preserves intent while maintaining SpotBugs coverage elsewhere.
🧹 Remove the redundant global match
<Match> <Bug pattern="IMPROPER_UNICODE"/> <Class name="~com\\.williamcallahan\\.javachat\\..*"/> </Match> - <Match>- <Bug pattern="IMPROPER_UNICODE"/>- </Match>src/main/java/com/williamcallahan/javachat/service/RateLimitManager.java (1)
305-315: Rename generic variableinfofor clarity.
Since this method was touched, consider renaminginfo→rateLimitInfoto comply with the banned generic-name list. As per coding guidelines, please use domain-specific naming.♻️ Suggested rename
- ParsedRateLimitInfo info = parseRateLimitHeaders(webError);+ ParsedRateLimitInfo rateLimitInfo = parseRateLimitHeaders(webError);- if (info.hasResetTime()) {- applyRateLimit(provider, info.resetTime(), 0);+ if (rateLimitInfo.hasResetTime()) {+ applyRateLimit(provider, rateLimitInfo.resetTime(), 0); } else { recordRateLimit(provider, webError.getMessage()); }src/main/java/com/williamcallahan/javachat/service/OpenAiCompatibleEmbeddingModel.java (1)
70-79: Great testability seam! 🧪This package-private overload is exactly what makes unit testing pleasant—you can inject a mock
OpenAIClientwithout hitting the network. TheObjects.requireNonNullcall with an explicit message is a nice touch for debugging.A small suggestion: a brief single-line comment (e.g.,
// For testing with mock clients) would help future maintainers immediately understand why this overload exists. Totally optional though!
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.
When lesson citations loaded after the initial content render, the browser's scroll anchoring behavior would unexpectedly shift the viewport, causing a jarring user experience. This was particularly noticeable when users had scrolled partway through lesson content before citations finished loading. The fix manually preserves and restores scroll position around citation updates, and disables browser scroll anchoring on the content panel to prevent the browser from fighting the manual restoration. - Add lessonContentPanelEl ref to track scroll container - Capture scrollTop before citation state updates - Restore scroll position via requestAnimationFrame after DOM update - Disable overflow-anchor CSS to prevent browser interference
The header height was hardcoded as a magic number (57px) in App.svelte, making responsive adjustments impossible and violating DRY when multiple components need to account for header offset. This extracts the value to a CSS custom property with responsive breakpoints that match the actual header sizing at different viewport widths. - Define --header-height CSS variable in global.css (57px default) - Add responsive overrides for mobile (60px @ 640px, 52px @ 380px) - Replace hardcoded padding-top in App.svelte with var(--header-height)
railway-appBottemporarily deployed
to
insightful-intuition / production
January 25, 2026 02:04
Inactive
…undant global rules
railway-appBottemporarily deployed
to
insightful-intuition / production
January 25, 2026 02:20
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/RateLimitManager.java (1)
309-314: Rename local variableinfoto a domain‑specific name.
Generic identifiers are banned;rateLimitInfofits and is already used elsewhere.As per coding guidelines, avoid banned generic identifiers.🔧 Suggested rename
- ParsedRateLimitInfo info = parseRateLimitHeaders(webError);+ ParsedRateLimitInfo rateLimitInfo = parseRateLimitHeaders(webError);- if (info.hasResetTime()) {- applyRateLimit(provider, info.resetTime(), 0);+ if (rateLimitInfo.hasResetTime()) {+ applyRateLimit(provider, rateLimitInfo.resetTime(), 0); } else { recordRateLimit(provider, webError.getMessage()); }
🤖 Fix all issues with AI agents
In
`@src/main/java/com/williamcallahan/javachat/service/markdown/OrderedMarkerScanner.java`:
- Around line 113-131: The finalizeMarker method currently accepts markers where
the delimiter (markerChar '.' or ')') is immediately followed by non-whitespace
(e.g., "1.Foo"); update finalizeMarker to require at least one whitespace
character after the delimiter before treating it as a list marker: after
computing afterIndex (first non-whitespace after sequenceEnd), if no whitespace
was skipped (i.e., afterIndex == sequenceEnd + 1) return null; keep existing
numeric-version and bounds checks and return new MarkerMatch(startIndex,
afterIndex, kind) only when that mandatory whitespace is present so downstream
checks like isContentStartValid won't misclassify "1.Foo".
In
`@src/main/java/com/williamcallahan/javachat/web/ExceptionResponseBuilder.java`:
- Around line 124-126: In ExceptionResponseBuilder, guard against a null return
from exception.headers() before calling isEmpty(): check that
exception.headers() != null && !exception.headers().isEmpty() (similar to
RateLimitManager.parseRateLimitFromHeaders()), and only then append the headers
to details; update the conditional referencing exception.headers() so it won’t
NPE when headers() returns null.
- Around line 93-95: The current code calls exception.getResponseBodyAsString()
and then responseBody.isBlank(), which can NPE for WebClientResponseException
instances that may return null; update ExceptionResponseBuilder to only call
getResponseBodyAsString() and perform the null/blank check when exception is an
instance of WebClientResponseException (use "exception instanceof
WebClientResponseException"), assign the result to a local String responseBody,
then if responseBody != null && !responseBody.isBlank() append it to details;
reference getResponseBodyAsString(), the exception variable, and the local
responseBody in your change.
🧹 Nitpick comments (3)
spotbugs-exclude.xml (1)
116-125: Avoid the global IMPROPER_UNICODE blanket.
The second<Match>without a<Class>suppresses all IMPROPER_UNICODE findings, which makes the scoped rule redundant. Tiny tidbit: SpotBugs filters are independent, so broad matches mute everything. Consider removing the global match or narrowing it.🧹 Suggested cleanup
<Match> <Bug pattern="IMPROPER_UNICODE"/> </Match> - <Match>- <Bug pattern="IMPROPER_UNICODE"/>- </Match>src/main/java/com/williamcallahan/javachat/service/RateLimitManager.java (1)
345-374: Avoid sentinel0for “optional” retry‑after.
Using a nullableLong(or an overload) makes intent explicit and avoids encoding “unknown” in a value.As per coding guidelines, optional parameters shouldn’t be encoded via sentinel values.♻️ One possible refactor (nullable override)
-private void applyRateLimit(ApiProvider provider, Instant resetTime, long retryAfterSecondsOverride) {+private void applyRateLimit(ApiProvider provider, Instant resetTime, Long retryAfterSecondsOverride) { String providerName = sanitizeLogValue(provider.getName()); ApiEndpointState state = getOrCreateEndpointState(provider); long retryAfterSeconds; - if (retryAfterSecondsOverride > 0) {+ if (retryAfterSecondsOverride != null && retryAfterSecondsOverride > 0) { retryAfterSeconds = retryAfterSecondsOverride; } else if (resetTime != null) { retryAfterSeconds = Math.max(0, Duration.between(Instant.now(), resetTime).getSeconds()); } else { retryAfterSeconds = 0; }Also update call sites that pass
0to passnullinstead:-applyRateLimit(provider, rateLimitInfo.resetTime(), 0);+applyRateLimit(provider, rateLimitInfo.resetTime(), null);src/main/java/com/williamcallahan/javachat/web/ChatController.java (1)
286-306: Extract health-check literals into named constants.Per the project guidelines, avoid inline strings like
"/v1/models"and"UNREACHABLE: "—constants make intent clearer and prevent drift. As per coding guidelines, please use intent-revealing constants.♻️ Suggested refactor
+ private static final String EMBEDDINGS_MODELS_PATH = "/v1/models";+ private static final String EMBEDDINGS_UNREACHABLE_PREFIX = "UNREACHABLE: ";- String healthUrl = serverUrl + "/v1/models";+ String healthUrl = serverUrl + EMBEDDINGS_MODELS_PATH; restTemplate.getForEntity(healthUrl, String.class); return ResponseEntity.ok(EmbeddingsHealthResponse.healthy(serverUrl)); } catch (RestClientException httpError) { log.debug("Embedding server health check failed", httpError); String details = describeException(httpError); return ResponseEntity.ok(EmbeddingsHealthResponse.unhealthy( - serverUrl, "UNREACHABLE: " + details));+ serverUrl, EMBEDDINGS_UNREACHABLE_PREFIX + details)); }
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.
Loading and error states in LearnView were not centered within their container. Using absolute positioning with inset: 0 fills the parent while flexbox centers the content both horizontally and vertically. - Add position: absolute and inset: 0 to loading-state and error-state - Add justify-content: center for vertical centering - Add position: relative to lesson-content-panel as positioning context - Add explicit grid-template-rows to lesson-view-content
Fixed positioning required manual padding-top on the app container to prevent content from being hidden under the header. Sticky positioning keeps the header in the document flow, eliminating the need for compensating padding and simplifying the layout. The header still stays visible at the top when scrolling. - Change header from position: fixed to position: sticky - Remove left/right declarations (unnecessary for sticky in flex container) - Remove padding-top: var(--header-height) from app container
railway-appBottemporarily deployed
to
insightful-intuition / production
January 25, 2026 02:40
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/lib/components/LearnView.svelte`:
- Around line 99-120: The fetchCitations promise chain currently lacks a
rejection handler so if fetchCitations rejects lessonCitationsLoaded never gets
set and the error surfaces; add a .catch() (or use try/catch if converting to
async/await) on the fetchCitations(lesson.title) call to handle rejections by
setting lessonCitationsError to the caught error and setting
lessonCitationsLoaded = true, keeping the same stale-response guard
(selectedLesson?.slug !== targetSlug) and the scroll-preservation/restore logic
around lessonContentPanelEl so UI state remains consistent; update the promise
chain where fetchCitations is invoked and ensure deduplicateCitations is only
used on successful results.
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 was referenced Jan 25, 2026
This was referenced Feb 7, 2026
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
Introduces a comprehensive Guided Learning module with interactive lessons and scoped chat, refactors the citation system to stream sources inline with responses, and unifies SSE streaming logic across frontend and backend for improved reliability and performance. Includes a new phase-aware Thinking Indicator and significant clean code improvements.
Changes by Category
Features
LearnViewwith lesson table of contents, markdown content rendering, and context-aware chat.CitationPanel.ThinkingIndicatorwith "Warm Precision" aesthetic (connecting, searching, generating phases) and smooth cursor fade-out animations.Refactoring
sse.ts), scrolling (scroll.ts), URL sanitization (url.ts), and syntax highlighting (highlight.ts).SseSupportandSseConstants, implementing consistent error handling and backpressure boundaries.RateLimitHeaderParser,OpenAiSdkUrlNormalizer, andDocsIngestionServiceto reduce duplication and improve testability.InlineListParserandOrderedMarkerScannerfor better separation of concerns.Bug Fixes
javascript:,data:, and protocol-relative URLs).Testing
InlineListParser,OpenAiSdkUrlNormalizer, andGuidedLearningController.Technical Details
Related Issues