feat(seo): add Clicky analytics injection with config policy and lint fixes - #25
Merged
Conversation
This repo deploys via Coolify containers (BuildKit secrets) and runs locally with `.env`. Codify the rule that secrets never belong in tracked Spring property/YAML files, while keeping local/dev ergonomics intact. - Add [EV1] policy for secrets + env var handling (OpenAI/GitHub Models, Qdrant) - Clarify [TL1e] secrets storage expectations - Clarify [LM1c] that only provider connectivity comes from `.env`/env vars
Add optional Clicky analytics injection to the SEO-rendered SPA shell so SPA routes are tracked without a frontend rebuild. This follows the standard Clicky pattern (clicky_site_ids + async loader) and is disabled for local dev. - Inject Clicky initializer + async script loader when enabled - Remove any Clicky tags when disabled to avoid double-injection - Validate app.clicky.site-id digits-only when enabled - Enable Clicky by default in prod; disable in dev profile
Type-aware oxlint flagged two issues: unsafe stringification of unknown stream failures (Object default "[object Object]") and unnecessary type assertions on marked.parse() results. This keeps lint green without changing app behavior. - Format unknown stream failure diagnostics safely (JSON/string-aware) - Remove unnecessary `as string` casts on marked.parse() return values
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Clicky analytics support and config to the backend; enforces secrets-from-environment guidance in AGENTS.md; hardens frontend markdown parsing and stream diagnostics; and applies large-scale frontend stylistic, typing, and test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant SeoController as SeoController
participant AppProperties as AppProperties
participant Document as index.html
Browser->>SeoController: GET /
SeoController->>AppProperties: read app.clicky.enabled, app.clicky.site-id
SeoController->>Document: updateDocumentMetadata(...)
SeoController->>Document: clickyAnalyticsInjector.applyTo(Document)
alt app.clicky.enabled == true and loader missing
Document-->>Browser: include Clicky initializer (inline script)
Document-->>Browser: include Clicky external script tag
else app.clicky.enabled == false
Document-->>Browser: remove Clicky initializer and script tags if present
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/williamcallahan/javachat/web/SeoController.java`:
- Around line 47-57: Replace the scattered `@Value` parameters in the
SeoController constructor with a single AppProperties.ClickyProperties instance:
add a nested ClickyProperties class inside AppProperties (with fields
enabled:boolean and siteId:String plus any validation/@PostConstruct checks),
bind it under app.clicky via `@ConfigurationProperties`, and then change the
SeoController constructor to accept AppProperties (or
AppProperties.ClickyProperties) instead of the two `@Value` params; update the
constructor to set this.clickyEnabled = clickyProps.isEnabled() and
this.clickySiteId = clickyProps.isEnabled() ?
parseClickySiteId(clickyProps.getSiteId()) : -1L and remove the old `@Value`
imports/usages so configuration is centralized and validated in AppProperties.
Uh oh!
There was an error while loading. Please reload this page.
SeoController was injecting Clicky settings through ad-hoc @value annotations and duplicating the site-ID parsing/validation logic locally. This moves the configuration into AppProperties.Clicky, validated at startup alongside all other app.* sections, so parsing runs once and SeoController receives an already-validated value object through constructor injection. - Add AppProperties.Clicky inner class with enabled/siteId/parsedSiteId fields - Wire Clicky into requireConfiguredSection startup validation - Replace @value injections in SeoController with AppProperties dependency - Remove SeoController.parseClickySiteId (logic now lives in Clicky.validateConfiguration)
Run oxfmt to enforce the frontend's canonical formatting: double quotes, trailing commas, statement semicolons, and consistent line wrapping across TypeScript sources, tests, HTML, CSS, JSON config, and build configs. Spotless also reformatted a few long lines in the Java files touched by the previous commit. No behavioral changes — only whitespace and punctuation normalization.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/web/SeoController.java (1)
118-170: 🛠️ Refactor suggestion | 🟠 MajorRename
doctodocumentfor intent-revealing naming.Since this section was touched, align the parameter name with the naming rules for clarity. Tiny rename, big readability win.
✏️ Suggested rename
- private void updateDocumentMetadata(Document doc, PageMetadata metadata, String fullUrl, String imageUrl) {+ private void updateDocumentMetadata(Document document, PageMetadata metadata, String fullUrl, String imageUrl) { @@ - updateClickyAnalytics(doc);+ updateClickyAnalytics(document); } - private void updateClickyAnalytics(Document doc) {- Element existingClickyLoader = doc.head().selectFirst("script[src=\"" + CLICKY_SCRIPT_URL + "\"]");+ private void updateClickyAnalytics(Document document) {+ Element existingClickyLoader = document.head().selectFirst("script[src=\"" + CLICKY_SCRIPT_URL + "\"]"); @@ - doc.head().select("script").forEach(scriptTag -> {+ document.head().select("script").forEach(scriptTag -> { @@ - doc.head().appendElement("script").text(initializer);- doc.head().appendElement("script").attr("async", "").attr("src", CLICKY_SCRIPT_URL);+ document.head().appendElement("script").text(initializer);+ document.head().appendElement("script").attr("async", "").attr("src", CLICKY_SCRIPT_URL); }As per coding guidelines, “Banned abbreviations:
doc… Every identifier must be domain-specific and intent-revealing.”src/main/java/com/williamcallahan/javachat/config/AppProperties.java (1)
108-310: 🛠️ Refactor suggestion | 🟠 MajorAdd Javadocs for the new public Clicky accessors.
Public methods in
AppProperties.Clicky(andgetClicky/setClicky) need Javadocs to satisfy the project’s documentation rule. Small docs now prevent future guesswork.As per coding guidelines, “Javadocs are mandatory on public/protected classes, methods, and enums.”
🤖 Fix all issues with AI agents
In `@frontend/src/lib/services/markdown.ts`:
- Around line 376-401: parseMarkdown currently calls DOMPurify.sanitize(rawHtml,
...) directly which will throw during SSR because DOMPurify needs a DOM; make
parseMarkdown SSR-safe by replacing the direct DOMPurify.sanitize call with an
SSR-compatible initialization: either switch to using the isomorphic-dompurify
package and call its sanitize method, or create a DOMPurify instance bound to a
jsdom window before sanitizing (initialize once and reuse), and then call that
instance's sanitize with the same options; update references in parseMarkdown
(and any helper usages such as the marked.parse → rawHtml flow) so
DOMPurify.sanitize is only invoked via the SSR-safe instance.
In `@frontend/src/styles/global.css`:
- Around line 51-66: Remove the stylelint failures by unquoting the single-word
family name and normalizing the system font casing: in the `@font-face` block and
the --font-serif variable remove the surrounding quotes from Fraunces (use
Fraunces without quotes) and adjust the font stack in --font-sans to use the
expected BlinkMacSystemFont token/casing that your linter expects (ensure the
BlinkMacSystemFont entry matches the project's canonical casing/token), then run
the linter to confirm the errors are gone.
🧹 Nitpick comments (1)
frontend/package.json (1)
23-36: Consider moving@types/dompurifyto devDependencies.
Tiny tidbit:@types/*packages are compile‑time only, so keeping them indevDependenciestrims production installs.♻️ Suggested tweak
"dependencies": { - "@types/dompurify": "^3.0.5", "dompurify": "^3.3.1", "highlight.js": "^11.10.0", "marked": "^15.0.0", "zod": "^3.25.76" }, "devDependencies": { + "@types/dompurify": "^3.0.5", "@sveltejs/vite-plugin-svelte": "^5.0.0",
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
SeoController's IOException handler silently discarded the exception with no log statement, making production failures invisible to operators. Additionally, the Clicky JavaScript initializer was an inline magic string rather than a named constant, reducing readability and violating naming discipline. - Add SLF4J Logger to SeoController for operational visibility - Log IOException with full stack trace before returning 500 response - Extract CLICKY_INITIALIZER_TEMPLATE constant for the site ID push script - Use String.format() instead of string concatenation for the initializer
Clicky site ID validation used a manual char-by-char loop with raw ASCII ordinal comparisons (character < '0' || character > '9'), which is harder to read than the idiomatic Java alternative. Replace with a single-expression stream using Character.isDigit() for clarity while preserving identical validation behavior — Long.parseLong() on the next line still rejects any non-ASCII digits that isDigit() might accept. - Replace 6-line manual loop with trimmedSiteId.chars().allMatch(Character::isDigit) - Extract boolean allDigits for self-documenting conditional
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/williamcallahan/javachat/web/SeoController.java`:
- Around line 151-176: The Clicky DOM mutation logic inside
SeoController.updateClickyAnalytics should be moved into a new
single-responsibility class (e.g., ClickyAnalyticsInjector) that encapsulates
detection/removal/insertion using CLICKY_SCRIPT_URL and
CLICKY_INITIALIZER_TEMPLATE and reads the clickyEnabled and clickySiteId flags;
create ClickyAnalyticsInjector with a public method injectIfEnabled(Document)
(or removeIfDisabled(Document)), have SeoController call that method from
updateClickyAnalytics (or replace updateClickyAnalytics with a one-line
delegate), and ensure the new class owns all Jsoup DOM changes so the controller
only delegates and contains no analytics DOM logic.
🧹 Nitpick comments (1)
src/main/java/com/williamcallahan/javachat/web/SeoController.java (1)
156-176: Replace new magic string literals with named constants.
The new selector/token/tag/attribute strings should be constants to align with the no‑magic‑literals rule.🔧 Example refactor (constants)
+ private static final String CLICKY_LOADER_SELECTOR =+ "script[src=\"" + CLICKY_SCRIPT_URL + "\"]";+ private static final String CLICKY_SITE_IDS_TOKEN = "clicky_site_ids";+ private static final String CLICKY_SCRIPT_TAG = "script";+ private static final String CLICKY_SCRIPT_ATTR_ASYNC = "async";+ private static final String CLICKY_SCRIPT_ATTR_SRC = "src"; ... - Element existingClickyLoader = doc.head().selectFirst("script[src=\"" + CLICKY_SCRIPT_URL + "\"]");+ Element existingClickyLoader = doc.head().selectFirst(CLICKY_LOADER_SELECTOR); ... - doc.head().select("script").forEach(scriptTag -> {+ doc.head().select(CLICKY_SCRIPT_TAG).forEach(scriptTag -> { String scriptBody = scriptTag.html(); - if (scriptBody != null && scriptBody.contains("clicky_site_ids")) {+ if (scriptBody != null && scriptBody.contains(CLICKY_SITE_IDS_TOKEN)) { scriptTag.remove(); } }); ... - doc.head().appendElement("script").attr("async", "").attr("src", CLICKY_SCRIPT_URL);+ doc.head()+ .appendElement(CLICKY_SCRIPT_TAG)+ .attr(CLICKY_SCRIPT_ATTR_ASYNC, "")+ .attr(CLICKY_SCRIPT_ATTR_SRC, CLICKY_SCRIPT_URL);As per coding guidelines, "No inline numbers (except 0, 1, -1) or strings; define named constants (no magic literals)."
Uh oh!
There was an error while loading. Please reload this page.
SeoController was performing Clicky script detection, removal, and insertion directly in its updateClickyAnalytics method, mixing analytics DOM mutation with HTTP-to-HTML translation. This violates controller single responsibility (adapters/in/web should only delegate, not own business logic). - Create ClickyAnalyticsInjector @component with applyTo(Document) method - Move CLICKY_SCRIPT_URL and CLICKY_INITIALIZER_TEMPLATE constants - Move clickyEnabled/clickySiteId fields and AppProperties.Clicky wiring - Extract removeClickyTags private helper for disabled-state cleanup
SeoController now delegates all Clicky DOM mutations to the new ClickyAnalyticsInjector component via a single applyTo(doc) call, removing the updateClickyAnalytics method and its analytics-specific constants/fields from the controller. - Replace updateClickyAnalytics with clickyAnalyticsInjector.applyTo(doc) - Remove CLICKY_SCRIPT_URL, CLICKY_INITIALIZER_TEMPLATE constants - Remove clickyEnabled, clickySiteId fields - Replace AppProperties constructor dependency with ClickyAnalyticsInjector
SeoControllerTest uses @WebMvcTest which only loads the controller slice. After extracting ClickyAnalyticsInjector from SeoController, the test context needs the new component imported alongside AppProperties. - Add ClickyAnalyticsInjector.class to @import annotation
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.
Summary
.env/env vars only, non-secret defaults in Spring property filesmake lintcleanClicky Analytics
<head>devprofile; enabled in production@ConfigurationPropertiesinAppProperties.Clickywith startup validationClickyAnalyticsInjector— controller only delegatesConfiguration
app.clicky.enabled/app.clicky.site-idin Spring propertiesSeoControllerreceives already-validated config through constructor injectionVerification
./gradlew test— 242 tests pass (0 failures)make lint— 0 errors, 0 warnings (oxlint, ast-grep, svelte-check, spotbugs, pmd)Commits
53bc987docs: codify secrets and config source policycdae404feat(seo): inject Clicky analytics into SPA shell9712df1fix(frontend): satisfy type-aware lint diagnostics99ab4e2refactor(seo): bind Clicky analytics via @ConfigurationProperties95b4a17style: apply oxfmt and Spotless formatting across codebasea98116ffix(seo): add error logging and extract Clicky initializer constant4607510refactor(config): replace manual ASCII loop with Character.isDigit()f8f816frefactor(seo): extract Clicky DOM injection into ClickyAnalyticsInjectorfce1142refactor(seo): delegate Clicky analytics from SeoController to injectorc1fe8abtest(seo): add ClickyAnalyticsInjector to SeoControllerTest context