Add startup credential validation, Qdrant TLS mapping, OG images - #22
Merged
Conversation
The Qdrant REST URL builder dropped the port when TLS was enabled, defaulting to 443 via HTTPS convention. Qdrant Cloud exposes REST on port 6333 even over TLS, so the health check, audit, and index initializer all built incorrect URLs and failed with connection errors causing /actuator/health to return 503 in production. Unify all three buildQdrantRestBaseUrl methods to always include the gRPC-to-REST mapped port regardless of TLS. The scheme handles encryption; the port handles routing. - ExternalServiceHealth: replace TLS branch with unified scheme + port - AuditService: same pattern, update stale Javadoc about port 443 - QdrantIndexInitializer: add mapGrpcToRestPort helper, use in TLS branch - Add 4 URL-builder tests covering TLS/non-TLS with default and Docker ports
The app previously started with empty LLM API keys, deferring failure to the first chat request. Add a @PostConstruct validator that halts startup immediately with a clear error message when neither GITHUB_TOKEN nor OPENAI_API_KEY is set, or when Qdrant TLS is enabled without QDRANT_API_KEY. - Add RequiredCredentialValidation @configuration bean - Add 6 unit tests covering all credential combination scenarios - Set GITHUB_TOKEN=test-token in test properties for @SpringBootTest
…t code SpotBugs reported 7 main-source and 9 test-source warnings. Remove redundant null checks on API methods with @nonnull contracts, narrow overly broad throws clauses, and fix a null parameter violation in a test. - ExceptionResponseBuilder: drop null guards on getStatusText(), getResponseBodyAsString(), getHeaders(), headers(), body() - ExceptionResponseBuilderTest: use empty string instead of null for statusText (API parameter is @nonnull) - ChatMemoryServiceTest: use execute() instead of submit() when Future is unused - RateLimitStateTest: narrow throws Exception to ReflectiveOperationException
…diom The Qdrant health checks used subscribe(onSuccess, onError) callbacks for logging, which places side effects outside the reactive chain and makes future operator composition (retry, fallback) harder. Success logging was at DEBUG level, making it invisible in production when diagnosing connectivity issues, and log messages lacked a consistent prefix for filtering. Switch to doOnSuccess/doOnError side-effect operators before a bare subscribe(), add [HEALTH] prefix to all Qdrant health-check log messages, promote success logging from debug to info, and include the error message in the connectivity-check failure log. - Replace subscribe(onSuccess, onError) with doOnSuccess/doOnError/subscribe in both checkQdrantConnectivity and checkQdrantHealth - Add [HEALTH] prefix to all six log statements for grep-friendly filtering - Promote checkQdrantHealth success log from debug to info - Include error.getMessage() in connectivity check failure log - Log collection count on health check success for quick verification
The Gradle `-q` flag still emits substantial output during dependency resolution on large projects, inflating Docker build logs and slowing CI feedback. Redirect all output to /dev/null and tolerate resolution failures with `|| true` since this layer is a cache-warming optimisation — the subsequent compilation step will fail fast if any dependency is genuinely missing. - Replace `-q` flag with full output redirection to /dev/null - Add `|| true` to prevent cache-warming layer from aborting the build
Social previews previously used the 310x310 mstile as the OG image, which renders poorly on platforms expecting landscape cards (Facebook, LinkedIn, Slack). This adds a proper 1200x630 branded OG image — both as a static fallback for CDN/build-time crawling and as a server-rendered endpoint with cache headers — and updates all meta tags to match: og:image dimensions, og:image:type, and twitter:card upgraded from "summary" to "summary_large_image". The SeoController is also hardened: escapeJson now delegates to Jackson's JsonStringEncoder instead of naive quote replacement, and null inputs are rejected via Objects.requireNonNull rather than silently producing empty strings. - Add static frontend/public/og-image.png (1200x630 branded image) - Add OpenGraphImageRenderer to render OG image at startup from 1024px logo - Add OpenGraphImageController to serve /og-image.png with cache headers - Update SeoController: default image path, OG dimensions/type meta, twitter:card=summary_large_image, null safety, Jackson JSON escaping - Update index.html: og:image path, dimensions, type, alt text, twitter:card, SEO script imagePath - Add tests for controller, renderer, and new SeoController assertions
describeException accepted null and returned null, violating the project's null discipline ([NO1a]). Callers should never pass null since an exception is required context for building an error description. Replace the defensive null check with Objects.requireNonNull to fail fast on misuse. - Replace null guard with Objects.requireNonNull in describeException - Update Javadoc @return to document actual behavior
Social shares used the 310x310 mstile icon as the OG image, producing generic-looking previews. The twitter:card was set to "summary" (small thumbnail) and og:image:width/height/type tags were missing or wrong. This adds a server-rendered 1200x630 branded PNG served at /og-image.png with proper OG and Twitter Card metadata on all public routes. - Add OpenGraphImageRenderer that renders the branded image at startup using Java AWT with the existing high-res logo, dark gradient, and title/tagline text; caches bytes in a field - Add OpenGraphImageController serving GET /og-image.png with Cache-Control: public, max-age=86400, s-maxage=604800 - Update SeoController to emit og:image:width, og:image:height, og:image:type, and twitter:card=summary_large_image - Fix escapeJson() to use Jackson JsonStringEncoder for comprehensive JSON escaping with caller-side requireNonNull validation - Fix ExceptionResponseBuilder.describeException() null return by replacing dead null guard with requireNonNull precondition - Update frontend/index.html static meta tags and inline SEO script - Add unit and WebMvcTest coverage for renderer and controller
Title positioning and title-tagline spacing used inline numeric literals (170 and 30) instead of named constants, violating the no-magic-literals rule already followed by the rest of the class. - Extract 170 as TITLE_TOP_MARGIN for vertical title positioning - Extract 30 as TITLE_TAGLINE_SPACING for gap between title and tagline
Spring component constructors should not throw checked exceptions. The IOException from logo loading and PNG encoding is now caught and wrapped in UncheckedIOException with the resource description for clear startup failure diagnostics.
The OG image had a visible white outline tracing the icon's rounded rectangle. The source logo contains semi-transparent white pixels (#FFFFFF36) along its border that produce a visible fringe when composited on any dark background. The previous 54px crop only removed transparent padding but left the border stroke and corner curves intact. Additionally, the logo was loaded from the gitignored static/assets/ directory (Vite build output), causing FileNotFoundException on CI where the file is never present. Increase the crop inset from 54px to 110px on the 1024px source to fully clear the rounded-rectangle corners (which extend to ~100px diagonal) and the white fringe pixels on all straight edges. Move the logo to src/main/resources/branding/ which is committed to git and not covered by the static/assets/ gitignore rule. Regenerate the static fallback OG image with the same 110px crop. - Increase LOGO_CROP_INSET from 54 to 110 in OpenGraphImageRenderer - Move logo classpath from static/assets/ to branding/ - Add src/main/resources/branding/javachat_brace_cup_star_1024.png - Replace gradient background with solid #25263C favicon blue - Update title to "JavaChat.ai", tagline to multi-line wrapped format - Increase font sizes (title 82pt, tagline 34pt) - Regenerate static og-image.png from 1024px source with 110px crop
Contributor
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a cached 1200×630 Open Graph PNG renderer and controller, centralizes Qdrant REST connection and gRPC→REST port mapping, introduces startup credential validation, refactors health-check/retry APIs and logging to use the new Qdrant connection, updates frontend SEO metadata, tightens exception formatting, and adds/adjusts tests. Changes
Sequence Diagram(s)sequenceDiagram
participant HealthIndicator as QdrantHealthIndicator
participant ExternalHealth as ExternalServiceHealth
participant QdrantHTTP as Qdrant (HTTP)
HealthIndicator->>ExternalHealth: triggerRetryIfDue(SERVICE_QDRANT)
ExternalHealth->>QdrantHTTP: HTTP connectivity check (candidateRestBaseUrls + apiKey)
alt success
QdrantHTTP-->>ExternalHealth: 200 / collections info
ExternalHealth-->>ExternalHealth: mark healthy, reset backoff
else failure
QdrantHTTP-->>ExternalHealth: error/timeout
ExternalHealth-->>ExternalHealth: mark unhealthy, schedule backoff/retry
end
ExternalHealth-->>HealthIndicator: getHealthSnapshot(SERVICE_QDRANT)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/main/java/com/williamcallahan/javachat/web/ExceptionResponseBuilder.java`:
- Line 7: Add a named constant for the null error message and document the
non‑null contract in the method Javadoc: define a private static final String
(e.g. NULL_PARAM_MESSAGE) and replace any inline string literals used in
Objects.requireNonNull or explicit null checks in ExceptionResponseBuilder
(including occurrences around the block referenced at lines 58–65) with that
constant, and add an `@throws` NullPointerException line to the method/class
Javadoc explaining when callers must not pass null.
In
`@src/test/java/com/williamcallahan/javachat/web/ExceptionResponseBuilderTest.java`:
- Around line 37-48: Replace inline literals in the test method
describeException_handlesBlankStatusTextWithoutThrowing with named constants:
declare constants (e.g. BLANK_STATUS_TEXT = "", RESPONSE_BODY = "problem", and
EXPECTED_HTTP_STATUS = "httpStatus=400") at the top of the test class and use
them in the HttpClientErrorException.create call and in the assertTrue check;
keep the HttpStatus.BAD_REQUEST and StandardCharsets references as-is and ensure
the test still calls builder.describeException(exception) and asserts the
EXPECTED_HTTP_STATUS substring is present.
In
`@src/test/java/com/williamcallahan/javachat/web/OpenGraphImageControllerTest.java`:
- Around line 33-52: Tests in OpenGraphImageControllerTest use magic literals
for the OG image path, cache-control fragments, dimensions, and assertion
messages; extract these into UPPER_SNAKE_CASE domain constants (e.g.,
OG_IMAGE_PATH, OG_CACHE_CONTROL_MAX_AGE, OG_CACHE_CONTROL_PUBLIC,
OG_IMAGE_WIDTH, OG_IMAGE_HEIGHT, OG_ASSERT_MSG_DECODE, OG_ASSERT_MSG_WIDTH,
OG_ASSERT_MSG_HEIGHT) declared at the top of the OpenGraphImageControllerTest
class and replace the inline strings/numbers in
serves_og_image_with_correct_content_type_and_cache_headers and
serves_og_image_with_correct_dimensions with those constants so all specs live
in one place and follow the project guideline for domain-qualifying constant
names.
In
`@src/test/java/com/williamcallahan/javachat/web/OpenGraphImageRendererTest.java`:
- Around line 18-43: The test contains magic literals; update
OpenGraphImageRendererTest to centralize literals into UPPER_SNAKE_CASE
constants (e.g., define private static final int OG_IMAGE_WIDTH = 1200 and
OG_IMAGE_HEIGHT = 630, and private static final String SPRING_BANNER_PROP =
"spring.main.banner-mode=off", plus any repeated assertion message strings if
desired) and replace inline uses in `@TestPropertySource`,
renders_valid_png_with_correct_dimensions (getWidth/getHeight asserts) and
returns_same_cached_bytes_on_repeated_calls (any repeated messages) to reference
these constants so literals are not hard-coded.
🧹 Nitpick comments (3)
Dockerfile (1)
42-44: Add a tiny failure breadcrumb while still continuing.Fully silencing and ignoring failures makes later CI triage harder; a one‑line warning keeps logs tidy but still leaves a hint. Fun tidbit: a single breadcrumb now often saves a full re-run later.
💡 Minimal logging tweak
-RUN --mount=type=cache,target=/root/.gradle \- ./gradlew dependencies --no-daemon > /dev/null 2>&1 || true+RUN --mount=type=cache,target=/root/.gradle \+ ./gradlew dependencies --no-daemon > /dev/null 2>&1 || { \+ echo "Gradle dependency warmup failed; continuing" >&2; \+ true; \+ }src/main/java/com/williamcallahan/javachat/service/AuditService.java (1)
47-50: Port mapping logic is duplicated across multiple services.The
GRPC_TO_REST_PORTmapping (and equivalentmapGrpcToRestPorthelper) appears inAuditService,QdrantIndexInitializer, andExternalServiceHealth. While each implementation is correct, this creates a maintenance burden if port mappings change.Consider extracting a shared utility (e.g.,
QdrantPortMapperor adding it toAppProperties) to centralize this mapping. This is a nice-to-have for future maintainability rather than a blocking issue! 🗺️Also applies to: 324-328
src/main/java/com/williamcallahan/javachat/web/OpenGraphImageRenderer.java (1)
87-89: Consider returning a defensive copy of the byte array.The cached
byte[]is returned directly. While the current consumers (controller) don't mutate it, returning the internal array allows callers to accidentally (or intentionally) corrupt the cached image. A quick.clone()would add a small safety net.That said, since this is an internal component and the only consumer immediately writes to the HTTP response, this is a minor defensive coding suggestion rather than a pressing concern.
🛡️ Optional defensive copy
public byte[] openGraphPngBytes() { - return openGraphPngBytes;+ return openGraphPngBytes.clone(); }
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:567d96a67f
ℹ️ 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.
…ve dead constructor param isHealthy() is now a pure query returning cached health state. Retry logic extracted to triggerRetryIfDue() which callers invoke explicitly. Removed unused ServiceStatus(String name) parameter (YAGNI).
…ect in health indicator QdrantHealthIndicator.health() previously called isHealthy() and discarded the return value purely for its retry side-effect. Now calls the explicit triggerRetryIfDue() command method.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/com/williamcallahan/javachat/config/RequiredCredentialValidation.java`:
- Around line 22-38: The class currently uses inline property keys and log
message strings; extract those literals into named constants and reference them
from the `@Value` annotations and any logging use sites. Create constants for the
property keys (e.g., GITHUB_TOKEN_KEY, OPENAI_API_KEY_KEY, QDRANT_TLS_KEY,
QDRANT_API_KEY_KEY) and reuse the existing message constants
(MISSING_LLM_API_KEY_MESSAGE, MISSING_QDRANT_API_KEY_MESSAGE) or move any
remaining inline messages (notably the string at line 59) into named constants;
update the fields githubToken, openaiApiKey, qdrantTlsEnabled, qdrantApiKey and
any logger calls to use the new constants instead of magic literals.
- Around line 28-38: RequiredCredentialValidation uses field injection for
githubToken, openaiApiKey, qdrantTlsEnabled, and qdrantApiKey; switch to
constructor injection by adding a constructor that takes these four parameters
and annotate the class as a Spring component/bean (or ensure it's constructed by
the framework), remove the `@Value` annotations from fields, assign the incoming
parameters to final fields so the bean is immutable, and update any tests or
callers to use the constructor if necessary.
🧹 Nitpick comments (4)
src/test/java/com/williamcallahan/javachat/web/OpenGraphImageRendererTest.java (1)
49-55: ConsiderassertSamefor reference equality checks.Fun fact! JUnit provides
assertSame(expected, actual, message)specifically for verifying two references point to the same object. It's semantically clearer and produces a more helpful failure message (showing both object identities) if the assertion ever fails.♻️ Proposed refactor
+import static org.junit.jupiter.api.Assertions.assertSame;`@Test` void returns_same_cached_bytes_on_repeated_calls() { byte[] firstCall = renderer.openGraphPngBytes(); byte[] secondCall = renderer.openGraphPngBytes(); - assertTrue(firstCall == secondCall, OG_IMAGE_CACHED_REFERENCE_ASSERTION_MESSAGE);+ assertSame(firstCall, secondCall, OG_IMAGE_CACHED_REFERENCE_ASSERTION_MESSAGE); }src/main/java/com/williamcallahan/javachat/service/ExternalServiceHealth.java (3)
378-386: Consider adding a debug log for the fallback case.The fallback assumption that an unrecognized port is already a REST port works for known configurations, but could cause head-scratching moments if someone uses a non-standard port. A quick debug log would help troubleshooting without adding noise to normal operations.
💡 Optional: Add debug log for fallback
// Assume caller configured the REST port directly + log.debug("[HEALTH] Unrecognized gRPC port {}; assuming REST port was configured directly", grpcPort); return grpcPort;
398-400: Remove unused constructor parameter.The
nameparameter is kept "for future use" but the coding guidelines advise against keeping unused code "just in case"—YAGNI (You Ain't Gonna Need It) is the guiding principle here. If you need it later, adding a parameter is a small change!As per coding guidelines: "Delete unused code instead of keeping it 'just in case'."
♻️ Remove unused parameter
- ServiceStatus(String name) {- // Name parameter kept for future use if needed+ ServiceStatus() { }And update the call site at line 98:
- serviceStatuses.put(SERVICE_QDRANT, new ServiceStatus(SERVICE_QDRANT));+ serviceStatuses.put(SERVICE_QDRANT, new ServiceStatus());
468-508: Consider converting to a record.
HealthSnapshotis an immutable data carrier with 4 fields—a perfect candidate for a Java record! Records reduce boilerplate and clearly communicate "this is just data." The current implementation works fine, so this is purely a nice-to-have.Fun fact: Records were introduced in Java 14 as a preview and became standard in Java 16. They're basically Java's way of saying "I just want a simple data holder without writing 40 lines of code!"
💡 Optional: Convert to record
- /**- * Immutable snapshot of service health status for UI and diagnostics.- */- public static class HealthSnapshot {- private final String name;- private final boolean healthy;- private final String message;- private final Optional<Duration> timeUntilNextCheck;-- /**- * Creates a snapshot of service health status.- *- * `@param` name service identifier- * `@param` isHealthy current health state- * `@param` message human-readable status description- * `@param` timeUntilNextCheck time until next check (null wraps to empty Optional)- */- public HealthSnapshot(String name, boolean isHealthy, String message, Duration timeUntilNextCheck) {- this.name = name;- this.healthy = isHealthy;- this.message = message;- this.timeUntilNextCheck = Optional.ofNullable(timeUntilNextCheck);- }-- /** Provides the service identifier. */- public String name() {- return name;- }-- /** Indicates whether the service is currently healthy. */- public boolean isHealthy() {- return healthy;- }-- /** Describes the current health state in human-readable form. */- public String message() {- return message;- }-- /** Provides the time until the next scheduled check, if applicable. */- public Optional<Duration> timeUntilNextCheck() {- return timeUntilNextCheck;- }- }+ /**+ * Immutable snapshot of service health status for UI and diagnostics.+ *+ * `@param` name service identifier+ * `@param` isHealthy current health state+ * `@param` message human-readable status description+ * `@param` timeUntilNextCheck time until next check (empty if not applicable)+ */+ public record HealthSnapshot(+ String name,+ boolean isHealthy,+ String message,+ Optional<Duration> timeUntilNextCheck) {++ /**+ * Creates a snapshot, wrapping nullable duration in Optional.+ */+ public static HealthSnapshot of(String name, boolean isHealthy, String message, Duration timeUntilNextCheck) {+ return new HealthSnapshot(name, isHealthy, message, Optional.ofNullable(timeUntilNextCheck));+ }+ }Note: You'd need to update call sites to use
HealthSnapshot.of(...)or passOptional.ofNullable(timeUntilNextCheck)directly.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Required credential validation used field-level @value injection and inline placeholder literals, which hid dependencies and forced tests to set private fields through reflection. This change makes dependencies explicit and immutable while centralizing property/log literals used by the validator. - Extract @value placeholder strings into named constants - Extract success log message into a named constant - Replace mutable field injection with constructor injection and final fields - Keep validation behavior unchanged for missing LLM and Qdrant credentials - Update unit test to construct RequiredCredentialValidation directly - Remove reflection-based test helper and private-field mutation
Qdrant REST connection details were duplicated inside ExternalServiceHealth (host, TLS mode, API key, and gRPC-to-REST port mapping), which risked drift and repeated configuration logic. This change introduces a dedicated connection component and reuses it in health checks as the single source of truth. - Add QdrantRestConnection component for Qdrant host/port/TLS/API-key config - Move gRPC-to-REST port mapping logic into the new component - Inject QdrantRestConnection into ExternalServiceHealth - Replace local URL and API-key assembly with shared component accessors - Remove duplicated @value fields/constants and obsolete helper method
Qdrant connection and gRPC-to-REST port mapping logic was duplicated across initializer and audit paths, with each class maintaining its own @value-based state and URL construction. This created maintenance drift risk and inconsistent REST endpoint resolution. This change aligns both classes with the shared QdrantRestConnection component introduced earlier and removes duplicate logic. - Inject QdrantRestConnection into QdrantIndexInitializer - Replace local @value fields (host, port, useTls, apiKey) with shared connection accessors - Replace local restBaseUrls() calls with candidateRestBaseUrls() - Replace local API-key header wiring with qdrantRestConnection.apiKey() - Remove duplicated port constants and mapGrpcToRestPort/restBaseUrls helper code - Inject QdrantRestConnection into AuditService - Replace local base URL and API-key logic in audit scroll requests - Remove duplicated GRPC_TO_REST_PORT map and buildQdrantRestBaseUrl() helper
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/ExternalServiceHealth.java (1)
119-176:⚠️ Potential issue | 🟡 MinorAdd null guard for
serviceNameparameter in public methods to prevent unexpected NPE.
ConcurrentHashMaprejects null keys and throwsNullPointerExceptiononget(null). Add a pre-check usingObjects.requireNonNull()(as done in the constructor) to keep the API predictable—or explicitly document that null is unsupported. Applies toisHealthy(),triggerRetryIfDue(), andgetHealthSnapshot().
🤖 Fix all issues with AI agents
In `@src/main/java/com/williamcallahan/javachat/config/QdrantRestConnection.java`:
- Around line 52-75: Extract the "api-key" header name into a public constant
(e.g. API_KEY_HEADER) on QdrantRestConnection and replace the three string
occurrences in AuditService and ExternalServiceHealth with that constant; leave
scheme ("http"/"https") and separator("://") literals scoped to
QdrantRestConnection only (optionally introduce private constants used by
restBaseUrl() and candidateRestBaseUrls()) since other classes
(QdrantIndexInitializer, AppProperties) consume or validate full URLs rather
than constructing them.
- Around line 31-41: The fields host, configuredPort, useTls, and apiKey in
QdrantRestConnection are currently field-injected with `@Value`; convert them to
immutable constructor-injected values by making those fields final, removing the
`@Value` annotations from the fields, and adding a single constructor for
QdrantRestConnection that accepts parameters annotated with `@Value`("${...}") for
each setting (or rely on a single constructor for autowiring) and assigns them
to the final fields so the component is immutable and testable.
In `@src/main/java/com/williamcallahan/javachat/web/OpenGraphImageRenderer.java`:
- Around line 154-156: The encodePng method currently calls
ImageIO.write(canvas, "png", outputStream) without checking its boolean result
and uses a hardcoded "png" string; change this by extracting the format string
into a named constant (e.g., PNG_FORMAT) and capture the boolean returned by
ImageIO.write in encodePng, then throw an IOException (or log and throw) if it
returns false to surface missing PNG writers and avoid returning empty bytes.
Ensure the constant and the guard clause are placed near the encodePng method
and reference the BufferedImage canvas and outputStream variables already in
scope.
- Around line 70-78: The constructor OpenGraphImageRenderer currently reads
logoResource without closing the InputStream, doesn't guard against
ImageIO.read(...) returning null, and uses magic literals; fix by using a
try-with-resources to open logoResource.getInputStream() and pass that stream to
ImageIO.read, immediately check the BufferedImage (e.g., logoSource) for null
and throw a clear UncheckedIOException with the logoResource description if
null, and extract the classpath string, the "png" format and any error message
fragments into private static final constants (reuse the class's existing
constant naming style); ensure you still call renderOpenGraphImage(logoSource)
and assign to openGraphPngBytes inside the try block.
In
`@src/test/java/com/williamcallahan/javachat/config/QdrantRestConnectionTest.java`:
- Around line 87-90: Rename the generic parameter fieldValue in method setField
to a domain-specific name such as fieldSetting (update the method signature:
setField(Object target, String fieldName, Object fieldSetting)) and replace all
uses of fieldValue inside the method with the new name; also update any local
callers in QdrantRestConnectionTest that pass or reference that parameter to use
the new identifier so compilation and intent remain correct.
In
`@src/test/java/com/williamcallahan/javachat/config/RequiredCredentialValidationTest.java`:
- Around line 14-20: Replace inline literal values in the
bothKeysBlank_throwsIllegalStateException test with named constants: introduce
constants for the blank API key inputs (e.g., BLANK_KEY), the boolean flag (if
applicable, e.g., USE_SOME_FLAG), and the expected error message substring
(e.g., EXPECTED_NO_KEY_MESSAGE), then use those constants in the call to
createValidation and in the assertTrue that checks thrown.getMessage(). Also
ensure the constants are declared as static final fields on the test class
(RequiredCredentialValidationTest) so other tests can reuse them and comply with
the no-magic-literals rule.
In
`@src/test/java/com/williamcallahan/javachat/web/OpenGraphImageRendererTest.java`:
- Around line 36-37: The test currently uses field injection for
OpenGraphImageRenderer via the `@Autowired` renderer field; change the test to use
constructor injection instead by removing the `@Autowired` field and adding a
constructor that accepts an OpenGraphImageRenderer parameter which assigns it to
a private final field, updating any references to use that field in
OpenGraphImageRendererTest to match the repo's constructor-injection standard.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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 fixes across the backend and frontend, focusing on robust credential validation, improved Open Graph image handling for social sharing, enhanced error diagnostics, and more reliable Qdrant connectivity and port handling. The main changes are grouped below:
Backend: Credential Validation and Robustness
RequiredCredentialValidationto enforce that at least one LLM API key is configured and, if Qdrant TLS is enabled, a Qdrant API key is also set. This ensures the application fails fast on startup if critical credentials are missing.QdrantIndexInitializerandAuditServiceto always include the explicit port, even under TLS/cloud, to support correct connectivity in all environments. [1][2][3]ExternalServiceHealthwith clearer log messages and better error reporting, making diagnostics and monitoring more actionable. [1][2][3]Backend: API and Error Handling
ExceptionResponseBuilder:Backend: Open Graph Image Serving
OpenGraphImageControllerto serve the pre-rendered Open Graph image (/og-image.png) with aggressive cache headers, ensuring fast and cache-friendly delivery for social media previews.Frontend: Social Sharing Metadata
index.htmlto use the new/og-image.png(1200x630) for richer social sharing previews, replacing the old 310x310 image. Also updated alt text, image dimensions, and card type for better SEO and appearance on social platforms. [1][2]DevOps: Build Process
/dev/null(instead of using-q), preventing excessive log output during dependency resolution.