Add Svelte/Vite frontend with chat UI, streaming, and backend updates - #3
Conversation
When users query for a specific Java version (e.g., "What's new in Java 25"), the retrieval service now prioritizes documents from that version. This improves relevance by filtering vector search results to match the requested JDK version rather than returning mixed results from different Java versions. - Add QueryVersionExtractor utility to parse version patterns from queries - Add RetrievalResult record to encapsulate retrieval quality metadata - Integrate version filtering into RetrievalService with query boosting - Expand search topK when filtering to ensure sufficient version-matched results
…vability LocalSearchService previously returned empty lists silently on failure, making debugging difficult. This refactor introduces SearchOutcome to distinguish between successful searches, missing directories, and I/O errors, enabling callers to respond appropriately and operators to diagnose issues. - Add SearchOutcome class to differentiate success, directory-not-found, and IO error - Add structured logging at debug/info/warn/error levels for search operations - Improve code formatting and readability throughout the service - Skip unreadable files individually rather than failing entire search
During development, the Svelte frontend runs on Vite (port 5173) while the Spring Boot backend runs on port 8085. This creates cross-origin requests that browsers block by default. Configurable CORS support at both the Spring Security and WebMvc layers ensures preflight OPTIONS requests pass. - Add Cors configuration class in AppProperties with sensible defaults - Configure CORS in SecurityConfig at filter chain level for OPTIONS preflight - Add WebMvcConfig for CORS registry and SPA route forwarding - Update application.properties with production CORS defaults - Update application-dev.properties to allow Vite dev server origins - Add /app/** to permitted request matchers in SecurityConfig
Replace monolithic static HTML/CSS/JS frontend with a modern Svelte single-page application using Vite as the build tool. This enables component-based architecture, TypeScript support, hot module replacement during development, and optimized production builds with code splitting and content hashing. - Add Svelte frontend project structure in frontend/ directory - Replace 650-line index.html with minimal SPA shell loading Vite bundles - Remove legacy static HTML pages (chat.html, guided.html, error pages) - Remove legacy CSS and JavaScript utilities - Add built Vite assets in src/main/resources/static/assets/
Update build infrastructure to support the Svelte frontend alongside Spring Boot. The Dockerfile now uses multi-stage builds to compile frontend assets before packaging the Java application. Switch to Debian-based images to resolve Alpine musl networking issues. Add Makefile targets for concurrent frontend+backend development. - Add frontend build stage to Dockerfile using Node 22 bookworm - Switch runtime image from Alpine to eclipse-temurin Debian base - Source all images from public.ecr.aws per project standards - Add `make dev` target to run Vite and Spring Boot concurrently - Add `make dev-backend` for backend-only development - Add `make frontend-install` and `make frontend-build` targets - Add DK1 rule to AGENTS.md requiring ECR image sources
Add node_modules and .svelte-kit directories for the Svelte frontend to prevent dependency caches from being tracked.
The retrieveLimited method's fallback path was not updated when LocalSearchService changed to return SearchOutcome instead of List<Result>. This caused a compilation error. Now properly handles SearchOutcome status, logs failures distinctly, and annotates fallback documents with retrieval source metadata for debugging. - Update fallback to use SearchOutcome.results() and check isFailed() - Add retrievalSource and fallbackReason metadata to fallback documents - Log error context before attempting local search fallback
The cache key was computed after potentially truncating oversized markdown input, which could cause cache misses for identical inputs or incorrect cache hits when different inputs truncated to the same prefix. Capture the original input as the cache key before any mutations to ensure consistent caching behavior.
The Vite-generated assets (JS bundles, CSS) should not be committed since they are built from frontend/ source during Docker image creation. This keeps the repository focused on source files and avoids stale build artifact accumulation. - Add src/main/resources/static/assets/ to gitignore - Add src/main/resources/static/index.html to gitignore (generated by Vite) - Remove previously committed assets from tracking
The frontend build stage was creating an empty static directory, overwriting existing favicon assets. Copy the existing static assets (favicons) before running npm build so they're preserved alongside the Vite output.
The static initializer was catching and ignoring exceptions when loading docs-sources.properties, violating RC1 (no silent degradation) and ER2 (never catch and ignore). Add structured logging to surface configuration loading outcomes for debugging and operational visibility.
LocalSearchService: The toResult() method was returning zombie Result objects with empty data when file reads failed, polluting results lists and masking failures (RC1 violation). Changed to return Optional<Result> and filter out failures using flatMap(Optional::stream). RetrievalService: The truncateDocumentToTokenLimit() method created metadata that was immediately discarded when documentFactory.createLocalDocument() was called (dead code per FS4). Fixed by adding truncation metadata to the document after creation. Removed unused HashMap import.
DocumentFactory: Add createWithPreservedMetadata method with strict null validation using Objects.requireNonNull to preserve document metadata during truncation operations. This prevents loss of retrievalSource and fallbackReason metadata when documents are truncated for token limits. RetrievalService: Add Map import required for Map.of() in truncation metadata. Extract magic numbers into named constants (CS7 compliance): - DEBUG_FIRST_DOC_PREVIEW_LENGTH for log previews - DIAGNOSTIC_PREVIEW_LENGTH for diagnostic outputs - CITATION_SNIPPET_MAX_LENGTH for user-facing snippets
Long words or URLs in chat messages can overflow their containers and break the layout. Adding overflow-wrap and word-break CSS properties ensures text wraps properly within message bubbles for both user and assistant content. - Add overflow-wrap: break-word to user-content class - Add word-break: break-word to user-content class - Add same properties to assistant-content class
Update gRPC dependency alignment to version 1.75.0 to get latest bug fixes and improvements including BackoffPolicyRetryScheduler enhancements. - Update grpc-bom version in dependencyManagement
LocalSearchService: Extract magic numbers into named constants: - MAX_FILES_TO_SCAN (5000) for directory traversal limit - MIN_CONTENT_LENGTH_FOR_SCORING (50) to prevent score inflation RetrievalService: Use CITATION_SNIPPET_MAX_LENGTH constant for citation snippet truncation instead of inline magic number 500.
Add make lint target that runs SpotBugs and PMD static analysis via Maven plugins, providing quick code quality checks.
Extract repeated overflow-wrap and word-break properties into a shared selector for .user-text and .assistant-content, reducing duplication.
Add svelte-check to the lint target for complete static analysis coverage across both Java backend and Svelte frontend.
Update SpotBugs plugin to latest version for improved static analysis coverage and bug detection.
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. WalkthroughMonorepo migration from Maven to Gradle; adds a Svelte/Vite SPA served from Spring static resources; replaces legacy static pages; overhauls markdown processing to an AST pipeline with enrichment placeholders; adds version-aware retrieval with local keyword fallback; expands embedding/cache/rate-limit, CORS/MVC, and numerous service APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Frontend as Frontend (Svelte)
participant Backend as SpringBoot
participant Retrieval as RetrievalService
participant VectorDB as Qdrant
participant LLM as LLM Provider
Browser->>Frontend: user sends message (sessionId, text)
Frontend->>Backend: POST /api/chat/stream (SSE)
Backend->>Retrieval: retrieve(query, sessionId)
Retrieval->>VectorDB: vector search (top-k)
VectorDB-->>Retrieval: search results
alt version filter detected
Retrieval->>Retrieval: apply version filter / post-filter
end
alt vector failure or no results
Retrieval->>Backend: invoke LocalSearchService keyword fallback
end
Retrieval-->>Backend: RetrievalResult (documents + metadata)
Backend->>LLM: stream chat completion (prompt + context)
LLM-->>Backend: streamed chunks
Backend->>Frontend: SSE stream (chunks / status / error)
Frontend->>Frontend: render streamed markdown and update UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
The application was failing to start because it could not create the pipeline.log file. The Logback configuration expects a writable `logs` directory, but the container runs as a non-root `appuser` which lacks permission to create directories in the `/app` workdir. This change pre-creates the directory and assigns proper ownership. - Create `logs` directory explicitly with `mkdir -p` - Set `appuser` ownership for both `logs` directory and `app.jar` - Ensure Logback file appender has a writable target on startup
There was a problem hiding this comment.
Pull request overview
This PR migrates the UI from legacy static HTML/JS pages to a new Svelte/Vite frontend build, while also adding backend improvements for CORS and version-aware retrieval.
Changes:
- Replace legacy static UI pages and client markdown utilities with a Svelte/Vite frontend (new
frontend/workspace) and updated Docker build to compile frontend assets. - Add configurable CORS support via
AppProperties,WebMvcConfig, and Spring Security integration. - Improve retrieval robustness: Java version detection/boosting, safer local-search fallback handling, and metadata preservation during truncation.
Reviewed changes
Copilot reviewed 38 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/static/js/markdown-utils.js | Removes legacy client-side markdown/codeblock utilities. |
| src/main/resources/static/index.html | Removes legacy iframe/tab shell entrypoint. |
| src/main/resources/static/chat.html | Removes legacy chat UI page. |
| src/main/resources/static/guided.html | Removes legacy guided learning UI page. |
| src/main/resources/static/error.html | Removes legacy error page UI. |
| src/main/resources/static/404.html | Removes legacy 404 UI page. |
| src/main/resources/application.properties | Adds production-default CORS properties. |
| src/main/resources/application-dev.properties | Extends CORS origins for Vite dev server. |
| src/main/java/com/williamcallahan/javachat/util/QueryVersionExtractor.java | Adds query parsing for Java/JDK version detection and URL filter patterns. |
| src/main/java/com/williamcallahan/javachat/service/markdown/UnifiedMarkdownService.java | Moves cache lookup earlier using an unmodified cache key. |
| src/main/java/com/williamcallahan/javachat/service/RetrievalService.java | Adds version-aware query boosting/filtering and improves fallback behavior + metadata handling. |
| src/main/java/com/williamcallahan/javachat/service/LocalSearchService.java | Returns structured outcomes + adds logging and IO error handling. |
| src/main/java/com/williamcallahan/javachat/service/DocumentFactory.java | Adds helper to create documents while preserving metadata. |
| src/main/java/com/williamcallahan/javachat/model/RetrievalResult.java | Introduces retrieval-quality wrapper type. |
| src/main/java/com/williamcallahan/javachat/config/WebMvcConfig.java | Adds MVC CORS mapping + forwards / to index.html. |
| src/main/java/com/williamcallahan/javachat/config/SecurityConfig.java | Adds Security-level CORS integration and static route allowances. |
| src/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.java | Adds logging around docs source config loading. |
| src/main/java/com/williamcallahan/javachat/config/AppProperties.java | Adds strongly-typed app.cors configuration. |
| pom.xml | Updates grpc-bom and spotbugs plugin versions. |
| frontend/vite.config.ts | Adds Vite config with proxy + build output into Spring static resources. |
| frontend/tsconfig.json | Adds TypeScript config for Svelte project. |
| frontend/svelte.config.js | Adds Svelte preprocess config. |
| frontend/src/vite-env.d.ts | Adds Vite/Svelte type refs. |
| frontend/src/styles/global.css | Adds new frontend design system + base styles. |
| frontend/src/main.ts | Mounts Svelte app. |
| frontend/src/lib/services/markdown.ts | Adds server-first markdown rendering with client fallback. |
| frontend/src/lib/services/chat.ts | Adds SSE streaming client + citations/enrichment fetch helpers. |
| frontend/src/lib/components/*.svelte | Adds new Svelte UI components (header/chat/input/welcome/message). |
| frontend/src/App.svelte | New app shell + view switching. |
| frontend/package.json | Adds frontend dependencies and scripts. |
| frontend/index.html | Adds frontend entry HTML template. |
| Makefile | Adds dev orchestration (Vite + Spring Boot) and lint/frontend build targets. |
| Dockerfile | Converts to multi-stage build (frontend + backend) using non-DockerHub base images. |
| AGENTS.md | Adds rule disallowing Docker Hub base images. |
| .gitignore | Ignores frontend artifacts and built static assets under Spring resources. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 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.
The project was using outdated versions of FindSecBugs and Maven PMD plugins. Updating these ensures we benefit from the latest security rules, bug fixes, and performance improvements in our static analysis pipeline. - Upgrade `findsecbugs-plugin` from 1.12.0 to 1.14.0 - Upgrade `maven-pmd-plugin` from 3.21.2 to 3.28.0
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.gitignore (1)
58-58: Stray-eartifact should be removed.This line appears to be an accidental remnant—possibly from an
echo -ecommand or similar. It's not a valid ignore pattern and adds noise to the file.🧹 Proposed fix
### Logs ### logs/ *.log --e # Build artifacts BOOT-INF/pom.xml (1)
150-154: Stale comment references old gRPC version.The comment on line 150 mentions
grpc-bom (1.68.1)but you've updated the BOM to1.75.0at line 226. A small housekeeping item to keep future readers from scratching their heads!📝 Proposed fix
- <!-- Ensure grpc-core is present and aligned via grpc-bom (1.68.1) to match Qdrant client -->+ <!-- Ensure grpc-core is present and aligned via grpc-bom (1.75.0) to match Qdrant client --> <dependency> <groupId>io.grpc</groupId> <artifactId>grpc-core</artifactId> </dependency>src/main/java/com/williamcallahan/javachat/service/markdown/UnifiedMarkdownService.java (1)
166-167: Cache key mismatch: storing with mutated key but looking up with original.Here's a sneaky bug! You look up using
cacheKey(the original input at line 113), but store usingmarkdown(which may have been truncated at line 124 or normalized at line 128). This means:
- Long input gets truncated → stored under truncated key
- Same long input comes in → lookup with original key → cache miss!
- Repeat forever 🔄
🐛 Proposed fix
// Cache the result - processCache.put(markdown, result);+ processCache.put(cacheKey, result);
🤖 Fix all issues with AI agents
In `@frontend/src/lib/components/MessageBubble.svelte`:
- Around line 17-23: The $effect block calling renderMarkdown must handle
promise rejections to avoid leaving renderedContent unset; update the logic in
the $effect (where message.role === 'assistant' && message.content) to await or
chain renderMarkdown with a .catch (or use try/catch in an async helper) and on
error set renderedContent to a safe fallback (e.g., a plain-text or error
notice) and log the error; reference the renderMarkdown call, the
renderedContent variable, and the message.role/message.content check when making
the change.
- Around line 74-79: The rendered HTML from renderMarkdown (which calls
marked.parse) is being injected directly into the DOM via {`@html`
renderedContent} in MessageBubble.svelte, creating an XSS risk; fix by running
the HTML output through a sanitizer (e.g., DOMPurify.sanitize) before assigning
to renderedContent (or configure marked with a safe renderer/escape option),
replace uses of unsanitized renderedContent with the sanitized string, and
remove the now-unused escapeHtml() helper; ensure symbols referenced are
renderMarkdown, marked.parse, renderedContent and escapeHtml so reviewers can
locate the changes.
In `@frontend/src/lib/services/chat.ts`:
- Around line 88-95: The SSE parser in frontend/src/lib/services/chat.ts
currently slices lines with line.slice(5) when line.startsWith('data:'), which
preserves an optional space after "data:" and yields values like " hello";
update the branch that handles SSE data lines (the logic around
line.startsWith('data:')) to strip a single optional space per the SSE spec
(i.e., detect "data: " vs "data:" and slice accordingly) so stored data values
do not include a leading space while preserving other whitespace semantics.
In `@frontend/src/lib/services/markdown.ts`:
- Around line 37-39: The empty catch block in
frontend/src/lib/services/markdown.ts (the bare "catch { }" after the
server-side markdown render attempt) silently swallows all errors; change it to
capture the exception (e.g., catch (err)) and record the failure before falling
back to client-side rendering — log a clear contextual message and the error
(console.error or the app's telemetry/logger service such as
reportError/sendToSentry) so network/JSON/500 errors are visible while
preserving the existing fallback behavior.
In `@pom.xml`:
- Around line 222-226: The pom change pins io.grpc:grpc-bom to 1.75.0 claiming
BackoffPolicyRetryScheduler exists, but that class doesn’t exist and this
explicit pin conflicts with the project's intent to avoid fixed gRPC versions
and with io.qdrant:client’s expected gRPC (~1.68.2); revert the grpc-bom version
bump (remove or restore previous/managed version) so gRPC is resolved
transitively by io.qdrant:client and eliminate the incorrect reference to
BackoffPolicyRetryScheduler in comments or commit message.
In `@src/main/java/com/williamcallahan/javachat/config/WebMvcConfig.java`:
- Around line 8-13: The class Javadoc for WebMvcConfig mentions the Vite dev
server running on port 5173, but the CORS defaults are defined in
AppProperties.Cors (e.g., allowedOrigins/defaults) which use port 8085; update
the Javadoc to reflect the actual default dev-server port used by
AppProperties.Cors (or change the AppProperties.Cors default if intent is 5173)
so the doc and configuration align; refer to WebMvcConfig and AppProperties.Cors
when making the change.
- Around line 34-38: addViewControllers currently only forwards "/" to
index.html causing deep links like "/learn" to 404; update
addViewControllers(ViewControllerRegistry registry) to register SPA fallback
patterns on the registry (using
registry.addViewController(...).setViewName("forward:/index.html")) for single-
and multi-segment routes without file extensions (e.g., "/{path:[^\\.]+}",
"/{path:^(?!api$).*$}/**" or similar pattern variants) so client-side routes are
forwarded while leaving assets (with dots) and API controllers intact; modify
the registry.addViewController calls in WebMvcConfig to include these additional
patterns.
In `@src/main/java/com/williamcallahan/javachat/service/LocalSearchService.java`:
- Around line 84-95: toResult currently assumes the filename contains "_"
causing substring(0, file.indexOf("_")) to throw if indexOf returns -1; change
to defensively compute the safeName by first removing the extension (e.g., strip
trailing ".txt"), then find int idx = file.indexOf("_"); if idx > 0 use
file.substring(0, idx) else use the whole nameWithoutExtension as the safeName,
pass that to fromSafeName and construct the Result; keep the existing
IOException catch and log.warn usage (log.warn(...)) for read errors.
♻️ Duplicate comments (2)
frontend/src/lib/components/WelcomeScreen.svelte (1)
48-52: Checkonclickbinding on the suggestion cards.If you’re standardizing on Svelte’s
on:event bindings, swap this toon:clickto avoid relying on DOMonclickproperties. Please verify against Svelte 5 docs.frontend/src/lib/components/Header.svelte (1)
27-34: Checkonclickbindings on the tab buttons.If you’re standardizing on Svelte’s
on:event bindings, update these toon:clickto avoid relying on DOMonclickproperties. Please verify against Svelte 5 docs.Also applies to: 41-48
🧹 Nitpick comments (15)
src/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.java (1)
32-41: Log the throwable and narrow the catch.
You’re already handling the fallback nicely; keeping the stack trace helps future debugging breadcrumbs, andProperties.load(...)only throwsIOException, so catching that avoids masking unexpected bugs. As per coding guidelines, prefer specific exception types.🔧 Suggested tweak
import java.io.InputStream; +import java.io.IOException; @@ - } catch (Exception configLoadError) {- log.warn("Failed to load docs-sources.properties: {} - using default URL mappings",- configLoadError.getMessage());+ } catch (IOException configLoadError) {+ log.warn("Failed to load docs-sources.properties - using default URL mappings",+ configLoadError); }frontend/src/lib/services/markdown.ts (2)
48-54: Regex on HTML conflicts with coding guidelines.The backend explicitly uses Jsoup for DOM-safe manipulation to avoid regex on HTML. These string replacements could misbehave on edge cases (e.g., code content containing
<pre><code>).Consider using a DOM parser like
DOMParserfor consistency with the "no regex for HTML processing" guideline:♻️ DOM-based alternative
functionprocessServerHtml(html: string): string{constparser=newDOMParser()constdoc=parser.parseFromString(html,'text/html')doc.querySelectorAll('pre > code:not([class])').forEach(code=>{code.classList.add('language-java')})doc.querySelectorAll('pre > code[class=""]').forEach(code=>{code.classList.add('language-java')})returndoc.body.innerHTML}As per coding guidelines: "No regex for HTML/Markdown processing; use proper parsers."
59-74: Configuremarkedonce, not on every render.Calling
marked.setOptions()insideclientRenderMarkdownmeans it runs on every single render. Move the configuration to module scope for a tiny performance win:♻️ Move configuration to module scope
import { marked } from 'marked' +// Configure marked once at module load+marked.setOptions({+ gfm: true,+ breaks: false+})+ // ... interface definition ... function clientRenderMarkdown(content: string): string { - // Configure marked for safe, consistent output- marked.setOptions({- gfm: true,- breaks: false- })- // Render markdown let html = marked.parse(content) as stringAlso, the regex for auto-detecting Java code (line 70-71) has the same HTML-regex concern mentioned above.
Dockerfile (1)
64-64: Glob pattern in COPY may match multiple JARs.If the Maven build produces multiple JAR files (e.g.,
-sources.jar,-javadoc.jar), this glob will fail or produce unexpected results. Consider using a more specific pattern or the final artifact name.Fun fact: Docker's COPY with wildcards fails if it matches zero or multiple files when the destination doesn't end with
/! 🎯🔧 Suggested fix
-COPY --from=builder /app/target/*.jar app.jar+COPY --from=builder /app/target/java-chat-*.jar app.jarOr configure Maven to produce a single predictable artifact name.
frontend/tsconfig.json (1)
16-16: Consider adding Vite client types for better IDE support.The
typesarray currently only includes"svelte". For full IntelliSense on Vite-specific features (likeimport.meta.env,import.meta.hot, etc.), you might want to include the Vite client types too!💡 Suggested enhancement
- "types": ["svelte"]+ "types": ["svelte", "vite/client"]src/main/java/com/williamcallahan/javachat/config/AppProperties.java (1)
50-51: Add concise Javadocs for the new public CORS accessors.A one‑liner on
getCors/setCorsand theCorsgetters/setters keeps the API tidy and future‑you from spelunking. As per coding guidelines, public methods should be documented.Also applies to: 192-211
frontend/src/lib/components/ChatView.svelte (1)
97-99: Potential key collision withmessage.timestampUsing
timestampas a key could cause issues if two messages arrive within the same millisecond (e.g., rapid automated testing or edge cases). Consider using a unique ID per message instead.💡 Suggested improvement
You could add an
idfield toChatMessageand generate it when creating messages:messages = [...messages, { + id: crypto.randomUUID(), role: 'user', content: message, timestamp: Date.now() }]Then use
message.idas the key:- {`#each` messages as message, i (message.timestamp)}+ {`#each` messages as message, i (message.id)}frontend/src/lib/components/MessageBubble.svelte (2)
26-44: Highlight.js languages re-registered on every content changeEach time
renderedContentupdates, the effect re-runs and re-registers all languages. While highlight.js handles this gracefully (idempotent registration), it's unnecessary work.♻️ Consider caching the hljs instance
// At module level or use a singleton patternlethljsInstance: typeofimport('highlight.js/lib/core').default|null=nullasyncfunctiongetHighlighter(){if(hljsInstance)returnhljsInstanceconst[hljs,java,xml,json,bash]=awaitPromise.all([import('highlight.js/lib/core'),import('highlight.js/lib/languages/java'),import('highlight.js/lib/languages/xml'),import('highlight.js/lib/languages/json'),import('highlight.js/lib/languages/bash')])hljs.default.registerLanguage('java',java.default)hljs.default.registerLanguage('xml',xml.default)hljs.default.registerLanguage('json',json.default)hljs.default.registerLanguage('bash',bash.default)hljsInstance=hljs.defaultreturnhljsInstance}
46-48: Consider adding user feedback for copy actionThe clipboard write succeeds silently. A brief visual cue (tooltip, icon change, or toast) would improve the UX by confirming the action worked!
💡 Quick enhancement idea
letcopied=$state(false)asyncfunctioncopyToClipboard(text: string){awaitnavigator.clipboard.writeText(text)copied=truesetTimeout(()=>copied=false,2000)}Then update the button to show a checkmark when
copiedis true.frontend/src/lib/services/chat.ts (1)
121-142: Silent error handling in fetch helpersBoth
fetchCitationsandfetchEnrichmentswallow errors silently and return empty results. While this prevents UI crashes, it makes debugging harder when things go wrong.Consider logging errors in development mode or emitting to an error tracking service:
💡 Optional logging enhancement
exportasyncfunctionfetchCitations(query: string): Promise<Citation[]>{try{constresponse=awaitfetch(`/api/chat/citations?q=${encodeURIComponent(query)}`)if(!response.ok)return[]returnawaitresponse.json()}catch(error){if(import.meta.env.DEV){console.warn('Failed to fetch citations:',error)}return[]}}src/main/java/com/williamcallahan/javachat/model/RetrievalResult.java (1)
12-16: Elegant record design! 🏗️Using a record with an enum for quality states is a clean, immutable approach. The factory methods below provide semantic clarity when constructing results.
One consideration: the
documentslist could potentially be passed asnull. Per coding guidelines, collections should never be null. Consider adding a compact constructor for validation:🛡️ Defensive null check
publicrecordRetrievalResult( List<Document> documents, RetrievalQualityquality, StringqualityReason ) { publicRetrievalResult { documents = documents != null ? documents : List.of(); } // ... rest of the code }src/main/java/com/williamcallahan/javachat/service/LocalSearchService.java (1)
118-128: Consider using a Java record forResult.The
Resultclass is a perfect candidate for a record—it's immutable data with public final fields. Records give youequals(),hashCode(), andtoString()for free, plus they're more concise. A fun fact: records were finalized in Java 16!✨ Optional refactor to record
- /**- * Represents a single search result with URL, text content, and relevance score.- */- public static class Result {- public final String url;- public final String text;- public final double score;-- public Result(String url, String text, double score) {- this.url = url;- this.text = text;- this.score = score;- }- }+ /**+ * Represents a single search result with URL, text content, and relevance score.+ */+ public record Result(String url, String text, double score) {}src/main/java/com/williamcallahan/javachat/service/RetrievalService.java (3)
56-112: Version-aware retrieval is a clever enhancement!The boosted query and expanded candidate pool when a Java version is detected should meaningfully improve relevance. The logging is also thorough—super helpful for debugging retrieval quality issues.
However, the magic number
2on line 106 (minimum version-matched docs threshold) deserves a named constant. Per coding guidelines, magic literals should be named constants with intent-revealing names.📝 Extract threshold constant
Add near the other constants:
/** Minimum version-matched documents required before preferring filtered results */privatestaticfinalintMIN_VERSION_MATCHED_DOCS = 2;Then update line 106:
- if (versionMatchedDocs.size() >= 2) {+ if (versionMatchedDocs.size() >= MIN_VERSION_MATCHED_DOCS) {
163-226: Duplicated version filtering logic betweenretrieve()andretrieveWithLimit().The version detection, boosted query creation, and post-filtering logic (lines 168-226) is nearly identical to lines 57-112 in
retrieve(). This duplication violates DRY and creates maintenance risk—if the filtering logic needs adjustment, it must be updated in two places.Consider extracting the version-aware search into a shared helper method that both retrieval paths can call.
♻️ Possible extraction approach
/** * Perform version-aware vector search with optional filtering. */privateList<Document> performVersionAwareSearch(Stringquery, intbaseTopK) { Optional<VersionFilterPatterns> versionFilter = QueryVersionExtractor.extractFilterPatterns(query); StringboostedQuery = QueryVersionExtractor.boostQueryWithVersionContext(query); inttopK = versionFilter.isPresent() ? baseTopK * 2 : baseTopK; // ... logging ...SearchRequestsearchRequest = SearchRequest.builder() .query(boostedQuery) .topK(topK) .build(); List<Document> docs = vectorStore.similaritySearch(searchRequest); // Apply version filtering if applicableif (versionFilter.isPresent()) { docs = applyVersionFilter(docs, versionFilter.get()); } returndocs; }
291-331: Magic numbers in truncation logic should be named constants.Lines 298 and 312 contain magic numbers (
4for chars-per-token estimation and0.8for the break-point threshold). Per coding guidelines, these should be named constants with intent-revealing names.Fun tidbit: the ~4 chars/token heuristic is a reasonable approximation for English text with GPT-style tokenizers, though it varies by language and content type!
📝 Extract constants
+ /** Conservative chars-per-token estimate for truncation calculations */+ private static final int CHARS_PER_TOKEN_ESTIMATE = 4;++ /** Minimum ratio of content to preserve when breaking at sentence boundaries */+ private static final double MIN_CONTENT_PRESERVE_RATIO = 0.8; private Document truncateDocumentToTokenLimit(Document doc, int maxTokens) { // ... - int maxChars = maxTokens * 4;+ int maxChars = maxTokens * CHARS_PER_TOKEN_ESTIMATE; // ... - if (breakPoint > maxChars * 0.8) {+ if (breakPoint > maxChars * MIN_CONTENT_PRESERVE_RATIO) { // ... }
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Store markdown cache under mutated keyYou now check the cache using cacheKey captured before truncation/normalization, but store the result under the mutated markdown value. For inputs that exceed MAX_INPUT_LENGTH or are pre-normalized, repeated requests with the same original string will never hit the cache, forcing reprocessing and increasing latency. Store with cacheKey (or recompute the key after all mutations) to keep cache hits consistent.
ℹ️ 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.
Uh oh!
There was an error while loading. Please reload this page.
The copy button was using width-based media queries to determine hover behavior, causing it to always appear at ~640px regardless of input type. A narrow desktop browser window has a mouse but was being treated as touch. Changed to feature detection using @media (hover: hover) and (pointer: fine) which correctly identifies actual input capabilities. Touch devices see the button always; mouse/trackpad users see it on hover. - Default opacity: 1 for touch accessibility - Hide-on-hover only when device actually supports hover - Remove redundant width-based opacity override
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)
scripts/fetch_all_docs.sh (1)
151-159: Potential double-counting of Java 25 in skip statistics.This block checks
$DOCS_ROOT/java/java25-completefor >= 8000 HTML files and incrementsTOTAL_SKIPPEDon line 157. However, the same directory is already checked via theDOC_SOURCESarray (line 115), which also incrementsTOTAL_SKIPPEDwhen present.If Java 25 docs are already fetched, both checks pass, and
TOTAL_SKIPPEDgets incremented twice for the same docs. This inflates the "Already present" count in the summary.Consider removing this block since the
DOC_SOURCESloop handles it, or guard it to avoid duplicate accounting:🛠️ Option: Remove the redundant check
-# Fetch Java 25 from existing mirror if available-JAVA25_MIRROR="$DOCS_ROOT/java/java25-complete"-if [ -d "$JAVA25_MIRROR" ]; then- count=$(find "$JAVA25_MIRROR" -name "*.html" 2>/dev/null | wc -l | tr -d ' ')- if [ "$count" -ge 8000 ]; then- log "${GREEN}✓ Java 25 Complete API already available: $count HTML files${NC}"- ((TOTAL_SKIPPED++))- fi-fi
🧹 Nitpick comments (4)
src/main/java/com/williamcallahan/javachat/cli/DocumentProcessor.java (3)
267-276: Unusedconfigparameter detected. 🧹The
configparameter is passed tologStartBannerbut never referenced in the method body. Per the coding guidelines, unused code should be removed rather than kept "just in case."If you intended to log environment details (like docs directory or Qdrant settings), now's the time! Otherwise, simplify the signature.
🔧 Option 1: Remove unused parameter
- private void logStartBanner(final EnvironmentConfig config) {+ private void logStartBanner() { if (!LOGGER.isInfoEnabled()) { return; }And update the call site at line 156:
- logStartBanner(config);+ logStartBanner();🔧 Option 2: Use the parameter to log useful context
private void logStartBanner(final EnvironmentConfig config) { if (!LOGGER.isInfoEnabled()) { return; } LOGGER.info(LOG_BANNER_LINE); LOGGER.info(LOG_START_TITLE); LOGGER.info(LOG_BANNER_LINE); LOGGER.info(LOG_DEDUP_ENABLED); + LOGGER.info("Docs directory: {}", config.docsDirectory()); LOGGER.info(LOG_BLANK_LINE); }
291-315: Same story here—configis unused. 🔍The
logSummarymethod also acceptsconfigbut doesn't use it. This is a good candidate for the same cleanup aslogStartBanner.🔧 Remove or use the parameter
- private void logSummary(final EnvironmentConfig config, final IngestionTotals totals) {+ private void logSummary(final IngestionTotals totals) {And update the call site at line 163:
- logSummary(config, totals);+ logSummary(totals);
317-352: SeveralEnvironmentConfigfields are never used. 📦The record reads five environment variables but only
docsDirectoryis actually used in the code. The fieldsqdrantCollection,qdrantHost,qdrantPort, andappPortare populated but never referenced anywhere.This ties back to the unused
configparameters inlogStartBannerandlogSummary—if those methods logged these values, they'd serve a purpose! Otherwise, consider trimming to just what's needed.🔧 Option 1: Simplify to only what's used
private record EnvironmentConfig( - String docsDirectory,- String qdrantCollection,- String qdrantHost,- String qdrantPort,- String appPort+ String docsDirectory ) { private static final String DOCS_DIR_DEFAULT = "data/docs"; - private static final String QDRANT_COLLECTION_DEFAULT = "(not set)";- private static final String QDRANT_HOST_DEFAULT = "localhost";- private static final String QDRANT_PORT_DEFAULT = "8086";- private static final String APP_PORT_DEFAULT = "8085"; private static final String ENV_DOCS_DIR = "DOCS_DIR"; - private static final String ENV_QDRANT_COLLECTION = "QDRANT_COLLECTION";- private static final String ENV_QDRANT_HOST = "QDRANT_HOST";- private static final String ENV_QDRANT_PORT = "QDRANT_PORT";- private static final String ENV_APP_PORT = "PORT"; static EnvironmentConfig fromEnvironment() { return new EnvironmentConfig( - envOrDefault(ENV_DOCS_DIR, DOCS_DIR_DEFAULT),- envOrDefault(ENV_QDRANT_COLLECTION, QDRANT_COLLECTION_DEFAULT),- envOrDefault(ENV_QDRANT_HOST, QDRANT_HOST_DEFAULT),- envOrDefault(ENV_QDRANT_PORT, QDRANT_PORT_DEFAULT),- envOrDefault(ENV_APP_PORT, APP_PORT_DEFAULT)+ envOrDefault(ENV_DOCS_DIR, DOCS_DIR_DEFAULT) ); }🔧 Option 2: Use them in logging for operator visibility
Log the full config in
logStartBannerso operators can verify their environment setup at a glance—especially helpful for debugging misconfiguration!src/main/java/com/williamcallahan/javachat/config/DocsSourceRegistry.java (1)
277-280: Consider the passthrough behavior for non-file:// local-looking paths.Lines 278-280 return
trimmedUrlas-is for paths that aren'thttp://,https://, orfile://, and didn't match the book PDF pattern. If a bare local path like/data/docs/somethingreaches this method, it passes through unchanged.This might be intentional (e.g., relative paths for other processing), but a brief comment explaining the expected input types would help future maintainers understand the design. Just a small tidbit for clarity! 📝
…eption When InterruptedException is caught, the thread's interrupt flag is cleared. Re-interrupt the thread to preserve the interrupt status for callers.
…tion filter
Replace inline magic numbers with named constants documenting security limits:
- FORMAT_DETECTION_MARK_LIMIT, FORMAT_DETECTION_HEADER_SIZE for format sniffing
- JAVA_SERIAL_MAGIC_BYTE_{1,2} for serialized stream detection
- DESERIALIZATION_MAX_{DEPTH,REFERENCES,ARRAY_LENGTH} for filter limits
Also rename filter lambda parameter from 'info' to 'filterContext' per ND2.Replace duplicate min-height declarations with @supports blocks to: - Satisfy Biome's noDuplicateProperties lint rule - Provide 100vh fallback for older browsers - Enable 100dvh for browsers supporting dynamic viewport units
The Makefile's -Dorg.gradle.jvmargs configures the Gradle daemon, not the forked bootRun process. Add jvmArgs and systemProperties directly to the bootRun task so the Spring Boot application receives the intended settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@build.gradle.kts`:
- Around line 206-218: The buildForScripts task description is wrong because it
depends on tasks.build (which runs tests); change its dependency to the Spring
Boot task (tasks.bootJar) and obtain the bootJar artifact directly instead of
scanning build/libs. Update the task "buildForScripts" to
dependsOn(tasks.bootJar) and retrieve the archive file from the bootJar task
(e.g., via tasks.named("bootJar")/tasks.bootJar's archiveFile) to print the
exact built JAR path, matching the existing printJarPath usage and ensuring
tests are not executed.
In `@frontend/src/lib/components/MessageBubble.svelte`:
- Around line 102-108: The copy button in MessageBubble.svelte uses only title
for accessibility which is unreliable for screen readers; update the button
element (class "action-btn", onclick handler calling copyToClipboard) to include
an explicit aria-label (e.g., aria-label="Copy message" or a localized
equivalent) so the icon-only control is announced correctly to assistive
technologies.
🧹 Nitpick comments (3)
src/main/java/com/williamcallahan/javachat/service/OpenAIStreamingService.java (2)
294-305: Consider: "Rate limit" might be doing double duty here. 🤔Currently, I/O errors and
InterruptedExceptionget recorded viarecordRateLimit, but they're not really rate limits—they're transient failures. The current approach works fine for provider availability tracking, but a future reader might scratch their head.Optional tidbit: if you ever refactor
RateLimitManager, consider a more genericrecordProviderUnavailable(provider, reason, exception)that distinguishes rate limits from other transients. No action needed now—just a thought for the road!
367-367: Orphaned comment might confuse future readers. 🧹This line hints at removed code but doesn't explain what was there or why it was problematic. Consider either removing it or expanding to something like:
// Model mapping removed: configured model IDs are used directly to avoid accidental overrides.build.gradle.kts (1)
232-240: Prefer lazy bootJar path resolution for configuration avoidance.
Line 232 resolvestasks.bootJar.get()at configuration time, which can eagerly realize tasks and hinder configuration caching. A small tweak to use a provider keeps things lazy.♻️ Suggested refactor
-val bootJarPath = tasks.bootJar.get().outputs.files.singleFile.absolutePath+val bootJarPath = tasks.named<org.springframework.boot.gradle.tasks.bundling.BootJar>("bootJar")+ .flatMap { it.archiveFile }+ .map { it.asFile.absolutePath } tasks.register("printJarPath") { description = "Print the path to the built boot JAR" group = "help" dependsOn(tasks.bootJar) doLast { - println(bootJarPath)+ println(bootJarPath.get()) } }
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…kip tests The task description claimed to skip tests but dependsOn(tasks.build) includes test in its dependency chain. Changed to dependsOn(tasks.bootJar) and retrieve the archive file directly from the bootJar task output.
The title attribute alone is not reliably announced by screen readers. Adding aria-label ensures icon-only buttons are accessible per WCAG 2.1.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@build.gradle.kts`:
- Around line 61-68: Remove the unused servlet MVC starter dependency
"org.springframework.boot:spring-boot-starter-web" from the dependency list in
build.gradle.kts; this project uses the reactive stack (WebFlux, WebClient,
Flux/Mono, SSE) so keep "org.springframework.boot:spring-boot-starter-webflux"
and other starters (actuator, validation, aop, oauth2-client) and delete the
implementation("org.springframework.boot:spring-boot-starter-web") line to avoid
bringing in servlet MVC artifacts.
🧹 Nitpick comments (2)
build.gradle.kts (2)
166-191: PreferconfigureEachfor Test tasks.
Small Gradle tip:configureEachkeeps configuration lazy and still applies tointegrationTest.♻️ Suggested tweak
-tasks.withType<Test> {+tasks.withType<Test>().configureEach { useJUnitPlatform() maxHeapSize = "1024m" jvmArgs( "--add-opens", "java.base/java.lang=ALL-UNNAMED", "--add-opens", "java.base/java.util=ALL-UNNAMED" ) }
217-226: Prefer Gradle providers forDOCS_DIR.
Usingproviders.environmentVariableis more configuration‑cache friendly and tracked by Gradle.♻️ Suggested tweak
- val docsDir = System.getenv("DOCS_DIR") ?: "${project.rootDir}/data/docs"- systemProperty("DOCS_DIR", docsDir)+ val docsDir = providers.environmentVariable("DOCS_DIR")+ .orElse("${project.rootDir}/data/docs")+ .get()+ systemProperty("DOCS_DIR", docsDir)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
No description provided.