Add session ID crypto, refactor chat memory, deduplication, simple analytics - #21
Merged
Conversation
Chunk deduplication only checked hash markers derived from URL, chunk index, and chunk text. When title or package metadata changed without content changes, ingestion skipped those chunks and left stale metadata in the vector layer. This change persists title/package metadata in hash marker payloads and compares stored marker metadata during dedup checks. Existing hashes are now reprocessed when metadata changes, while unchanged hashes still skip as before. - Add metadata-aware hash marker read/write logic in LocalStoreService - Add hasHashMetadataChanged(...) and metadata parsing for existing markers - Update ChunkProcessingService to reprocess hash hits on metadata drift - Pass title/package metadata when marking ingested hashes in ingestion flows - Add regression tests for both metadata-drift reingest and unchanged skip
Health recovery logic could stall after transient outages: health snapshots did not trigger retries, unhealthy scheduled checks were effectively bypassed, and parallel retry paths could inflate failure state while risking backoff overflow. This change ensures retry evaluation happens when health is polled, gates health checks to one in-flight check per service status, and uses overflow-safe capped backoff progression. - Trigger retry evaluation from QdrantHealthIndicator.health() - Run unhealthy retry evaluation in scheduled health checks - Add in-progress check gating in ExternalServiceHealth.ServiceStatus - Replace Math.pow duration growth with overflow-safe capped doubling - Add tests for retry trigger behavior and concurrency/overflow handling
Chat memory maintained two separate per-session structures (messages and turns) with non-atomic updates. Under concurrency, ordering could diverge, and session validation semantics relied on side-effect-prone access patterns. This refactor introduces a single per-session conversation object with synchronized atomic updates, plus explicit session recognition checks for validation endpoints. - Replace dual-map storage with SessionConversation aggregate per session - Update user/assistant writes to atomically update history and turns together - Add hasSession(String) for side-effect-free recognition checks - Update /api/chat/session/validate to avoid unknown-session creation side effects - Clarify session response docs and add concurrency/session validation tests
Some HTTP and provider exception types can expose null status text, headers, or response payload values. The previous formatter called methods on those values without guarding for null, which could throw while we were already handling an upstream failure. This change hardens exception detail assembly so error reporting stays reliable under malformed or partial exception metadata. - Guard RestClient status text and response body before blank checks - Guard WebClient status text, response body, and headers before use - Guard OpenAI exception headers/body values before appending - Add regression test covering null status text handling
Rate-limit window expiry was clearing consecutive failure counters during availability checks, and rate-limit events were not always incrementing total failure metrics. That made resilience telemetry and backoff behavior less trustworthy after throttling events. This change keeps failure history intact across expiry boundaries and records every rate-limit event consistently. - Increment total failure count when recording rate-limit events - Stop resetting consecutive failures when availability window expires - Add focused tests for expiry behavior and total failure increments
Embedding configuration default dimensions and request hint behavior were out of sync for mixed provider/model usage. The client also sent dimensions indiscriminately, which can break providers that do not accept that field for their embedding models. This change aligns defaults with configured expectations and applies dimension hints only for models that support override semantics. - Set embedding default dimensions to 4096 in AppProperties - Add model-aware dimension override gating for text-embedding-3 models - Keep request construction typed while conditionally applying dimensions - Add regression tests for include/omit dimension request behavior
Session identifiers were built from timestamp plus Math.random-only suffixes, which are weaker and less stable across runtimes with varying crypto support. This could increase collision risk and reduce entropy in high-throughput client sessions. This change introduces a deterministic fallback chain that uses the strongest available browser/runtime primitive first, then degrades explicitly. - Add random segment generator with crypto.randomUUID primary path - Fallback to crypto.getRandomValues hex encoding when UUID is unavailable - Keep explicit padded Math.random fallback for legacy runtimes - Add unit tests covering all three entropy-source branches
Pre-commit formatting (spotlessApply) reformatted two files during earlier commits, leaving non-functional line-wrap and trailing-line diffs unstaged. These diffs are noise-only and should be isolated so functional commits remain clean and fully focused on behavior changes. This commit captures only formatter output and does not alter runtime behavior or test intent. - Wrap long method-call lines to formatter-compliant style - Normalize chained append formatting in marker payload builder - Remove trailing blank line in test file - Keep logic and assertions unchanged
The session utility tests repeated identical fake-timer initialization in each case. This was introduced by formatter/hook interactions and left as an unstaged follow-up change. This commit centralizes shared timer setup in beforeEach to keep tests clear and reduce repetition without changing assertions or coverage. - Add beforeEach with fixed test clock initialization - Remove duplicated timer setup lines from individual test cases - Keep existing test scenarios and expectations unchanged
The site needs privacy-friendly analytics tracking. Rather than hardcoding a script tag in index.html, this uses Vite's built-in transformIndexHtml hook to inject the correct script at build time — latest.dev.js in development (no-op) and latest.js in production (real tracking). - Switch defineConfig from object to function form to access ConfigEnv.mode - Add simple-analytics plugin using transformIndexHtml to return HtmlTagDescriptor - Extract CDN base URL to SIMPLE_ANALYTICS_CDN named constant - Remove stale "Serve from root" comment on base property
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughConsolidates session-random logic and tests, adds mode-aware Vite analytics injection, centralizes per-session chat state, implements metadata-aware ingestion deduplication, adds robust external-service retry/backoff and health checks, hardens exception null-safety, and expands tests across these areas. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/williamcallahan/javachat/service/ingestion/LocalDocsFileIngestionProcessor.java (1)
369-389: 🛠️ Refactor suggestion | 🟠 MajorUse constants for metadata keys.
Avoid new magic strings for"title"and"package".♻️ Suggested refactor
public class LocalDocsFileIngestionProcessor { private static final Logger log = LoggerFactory.getLogger(LocalDocsFileIngestionProcessor.class); private static final Logger INDEXING_LOG = LoggerFactory.getLogger("INDEXING"); private static final String FILE_URL_PREFIX = "file://"; private static final String API_PATH_SEGMENT = "${app.ingestion.api-path-segment:/api/}"; + private static final String METADATA_KEY_TITLE = "title";+ private static final String METADATA_KEY_PACKAGE = "package"; @@ - String title = metadataText(doc, "title");- String packageName = metadataText(doc, "package");+ String title = metadataText(doc, METADATA_KEY_TITLE);+ String packageName = metadataText(doc, METADATA_KEY_PACKAGE);As per coding guidelines, “No inline numbers (except 0, 1, -1) or strings; define named constants (no magic literals).”
src/main/java/com/williamcallahan/javachat/service/DocsIngestionService.java (1)
154-176:⚠️ Potential issue | 🟡 MinorAlign metadata handling with naming/constant rules.
docis a banned abbreviation, and the new metadata keys/empty string should be named constants. A small refactor keeps this compliant and clearer.♻️ Suggested fix
@@ public class DocsIngestionService { private static final Logger INDEXING_LOG = LoggerFactory.getLogger("INDEXING"); private static final String API_PATH_SEGMENT = "/api/"; private static final Duration HTTP_CONNECT_TIMEOUT = Duration.ofSeconds(30); + private static final String METADATA_HASH_KEY = "hash";+ private static final String METADATA_TITLE_KEY = "title";+ private static final String METADATA_PACKAGE_KEY = "package";+ private static final String METADATA_EMPTY_TEXT = ""; @@ - for (org.springframework.ai.document.Document doc : documents) {- Object hashMetadata = doc.getMetadata().get("hash");+ for (org.springframework.ai.document.Document document : documents) {+ Object hashMetadata = document.getMetadata().get(METADATA_HASH_KEY); if (hashMetadata == null) { continue; } - String title = metadataText(doc, "title");- String packageName = metadataText(doc, "package");+ String title = metadataText(document, METADATA_TITLE_KEY);+ String packageName = metadataText(document, METADATA_PACKAGE_KEY); try { localStore.markHashIngested(hashMetadata.toString(), title, packageName); @@ private String metadataText(org.springframework.ai.document.Document document, String metadataKey) { Object metadataRaw = document.getMetadata().get(metadataKey); if (metadataRaw == null) { - return "";+ return METADATA_EMPTY_TEXT; } return metadataRaw.toString(); }As per coding guidelines: “Banned abbreviations: … doc …” and “No inline numbers (except 0, 1, -1) or strings; define named constants (no magic literals).”
🤖 Fix all issues with AI agents
In `@frontend/vite.config.ts`:
- Around line 4-24: Extract the inline Simple Analytics plugin from
vite.config.ts into a new module by moving SIMPLE_ANALYTICS_CDN and the plugin
object into a dedicated exported function simpleAnalyticsPlugin(mode: string):
Plugin; implement the same transformIndexHtml logic there (including selecting
latest.dev.js vs latest.js and returning the script tag with async and src),
export the function, then import and call simpleAnalyticsPlugin(mode) in the
plugins array inside defineConfig to replace the current inline plugin.
In
`@src/main/java/com/williamcallahan/javachat/service/ChunkProcessingService.java`:
- Around line 186-190: The local variable named doc in the block using
hashAlreadyIngested/hashIngestionLookup and
documentFactory.createDocumentWithPages should be renamed to a domain-specific
identifier (e.g., document) to follow the banned-abbreviations rule; update the
declaration and every usage (the variable passed to pageDocuments.add and any
later references) so that document replaces doc consistently in the method
containing hashAlreadyIngested, hashIngestionLookup.hasMetadataChanged,
documentFactory.createDocumentWithPages, and pageDocuments.add.
In `@src/main/java/com/williamcallahan/javachat/web/ChatController.java`:
- Around line 313-321: The inline user-facing strings in ChatController (e.g.,
"Session not found on server", "Session found", "Session found but empty")
violate the no-inline-strings rule; replace them with named constants (e.g.,
SESSION_NOT_FOUND_MSG, SESSION_FOUND_MSG, SESSION_FOUND_EMPTY_MSG) defined
either as private static final String fields inside ChatController or in a
centralized Messages/constants class, then use those constants in the
SessionValidationResponse constructors (keep existing variables sessionId,
turnCount, exists and the ResponseEntity return logic unchanged).
In
`@src/test/java/com/williamcallahan/javachat/web/ChatControllerSessionValidationTest.java`:
- Around line 19-57: Rename the generic local variable names (e.g., change
"response" to a specific name like "badRequestResponse" or
"unknownSessionResponse") in the tests
validateSession_returnsBadRequestWhenSessionIdIsBlank and
validateSession_doesNotCreateUnknownSessionAndReportsRecognizedSessionHistory,
and move repeated string literals and session id values (e.g., "Session ID is
required", "Session not found on server", "Session found", "unknown-session-id",
"recognized-session-id", and the stored message) into private static final
constants at the top of ChatControllerSessionValidationTest; update all usages
(assertEquals, validateSession calls, addUser call, etc.) to reference those
constants and adjust variable names to be descriptive rather than the banned
generic "response".
🧹 Nitpick comments (2)
src/test/java/com/williamcallahan/javachat/service/RateLimitStateTest.java (1)
24-29: Solid test setup! 🧪Registering
JavaTimeModuleis essential for Jackson to serializeInstantfields correctly. Without it, you'd get cryptic serialization errors.One small consideration:
RateLimitStatewrites to./data/rate-limit-state.jsononsafeSaveState()calls (triggered byrecordRateLimit). In CI or parallel test runs, this could cause flaky behavior if tests share the same working directory. You might want to consider:
- Mocking the file I/O
- Using a temp directory per test
- Or ensuring cleanup in
@AfterEachThat said, for isolated test runs this works fine — just something to keep in mind as the test suite grows!
src/main/java/com/williamcallahan/javachat/service/ExternalServiceHealth.java (1)
384-389: Minor consideration: ordering of state updates in markHealthy()The current order is:
isHealthy.set(true)consecutiveFailures.set(0)currentBackoff = ...lastCheck = ...checkInProgress.set(false)There's a brief window where
isHealthyis true butcheckInProgressis still true. This is benign becauseisHealthy(serviceName)returns early on line 130 when healthy, but if you ever add code that checks both flags, you might see slightly inconsistent reads.Not a bug — just something to keep in mind if this class evolves!
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
Here are some automated review suggestions for this pull request.
Reviewed commit:5cb023cff5
ℹ️ 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.
…alServiceHealth Move URL construction and request spec building inside try blocks so pre-subscription failures reset checkInProgress via markUnhealthy. Add status.reset() before verifyQdrantCollectionsAfterStartup to prevent ApplicationReadyEvent from being blocked by in-flight check. Inline anemic checkQdrantHealthAsync wrapper.
Replace inline string literals with SESSION_NOT_FOUND_MESSAGE, SESSION_FOUND_MESSAGE, and SESSION_FOUND_EMPTY_MESSAGE constants to comply with no-magic-literals rule.
…nkProcessingService
…ctory Move duplicated private metadataText methods from DocsIngestionService and LocalDocsFileIngestionProcessor into DocumentFactory as a static utility to eliminate DRY violation.
…ests for LocalStoreService
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 13, 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
1 participant
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.
This pull request introduces several improvements and refactorings across both the frontend and backend, focusing on session management, document ingestion deduplication, chat memory handling, and health check reliability. The changes enhance robustness, thread safety, and maintainability, while also improving test coverage and analytics integration.
Backend Improvements
Chat Memory Service Refactor:
ChatMemoryServiceto use a newSessionConversationclass, ensuring thread-safe and consistent management of per-session chat history and turns. This replaces multiple concurrent maps with a single map, and synchronizes updates for each session. Also, added ahasSessionmethod for session existence checks. [1][2][3][4][5]Document Ingestion Deduplication:
ChunkProcessingServiceto not only skip already-ingested chunks, but also re-ingest if associated metadata (title or package) has changed. This is achieved by expanding theHashIngestionLookupinterface and updating its usage. [1][2][3][4]DocsIngestionServiceto mark hashes as ingested with metadata, and added a helper to extract metadata for this purpose.Health Check Robustness:
ExternalServiceHealthto clarify unhealthy/retry-due states, prevent duplicate health checks, and ensure scheduled checks work correctly even if the service is unhealthy. [1][2][3][4][5]QdrantHealthIndicatorwhen the service is unhealthy and backoff has elapsed.Configuration:
AppProperties.Embeddings.Frontend Improvements
Session ID Generation and Testing:
crypto.randomUUIDif available, falling back tocrypto.getRandomValuesor a paddedMath.randomoutput. Added comprehensive tests for all fallback paths. [1][2][3]Analytics Integration:
These changes collectively improve reliability, maintainability, and observability across the application.