refactor(document): drop the 5-minute Cache-Control TTL on /density (#709) #711

Merged

marcel merged 1 commits from feat/issue-709-remove-density-cache-ttl into main 2026-06-01 20:01:32 +02:00

Conversation 7 Commits 1 Files Changed 3 +9 -11

marcel commented 2026-06-01 19:57:12 +02:00

Owner

Copy Link

Closes #709.

What

Removes the explicit Cache-Control: max-age=300, private header from GET /api/documents/density. The density chart is an interactive filter control (click-to-select month, drag-to-select range); a 5-minute browser cache could show stale month counts for up to 5 minutes after a user edits, uploads, transcribes, re-dates, or re-tags a document — actively misleading on a control the user acts on.

The in-memory aggregation is sub-200ms p95 over ~5k docs and there is no CDN/proxy cache (DevOps confirmed no load reason to cache at this scale), so the trade-off rightly tilts toward freshness.

Implementation note (deviation from the literal AC)

The issue's AC called for header().doesNotExist("Cache-Control") ("omit, not no-store"). In practice that assertion is impossible: our explicit header was overriding Spring Security's default HeaderWriterFilter, which writes Cache-Control: no-cache, no-store, max-age=0, must-revalidate on every secured response. Removing our header lets that framework default apply.

This over-satisfies the issue's goal — the browser never caches the density response at all (stronger than plain omission), and no-store is strictly safer than private for the per-user filtered data (no caching ⇒ no cross-user leak, addressing Nora's concern for free). Confirmed with the user before proceeding; the test now asserts this real contract.

Changes

• DocumentController.density() → return ResponseEntity.ok(result) (explicit header removed; unused CacheControl / TimeUnit imports dropped).
• DocumentControllerTest: density_emitsPrivateCacheControlHeader → density_isNeverBrowserCached, updated red-first, now asserts the exact no-cache, no-store, max-age=0, must-revalidate contract.
• DocumentService.getDensity() Javadoc: dropped the stale max-age=300 … absorbs repeated browse loads claim.

No change to getDensity() computation logic (per out-of-scope). #481 (SQL aggregation) and ETag modeling remain parked.

Verification

• DocumentControllerTest — 105/105 green (red-first confirmed before the controller change).
• ./mvnw clean package -DskipTests — BUILD SUCCESS.

🤖 Generated with Claude Code

Closes #709. ## What Removes the explicit `Cache-Control: max-age=300, private` header from `GET /api/documents/density`. The density chart is an interactive filter control (click-to-select month, drag-to-select range); a 5-minute browser cache could show stale month counts for up to 5 minutes after a user edits, uploads, transcribes, re-dates, or re-tags a document — actively misleading on a control the user acts on. The in-memory aggregation is sub-200ms p95 over ~5k docs and there is no CDN/proxy cache (DevOps confirmed no load reason to cache at this scale), so the trade-off rightly tilts toward freshness. ## Implementation note (deviation from the literal AC) The issue's AC called for `header().doesNotExist("Cache-Control")` ("omit, not `no-store`"). In practice that assertion is **impossible**: our explicit header was *overriding* Spring Security's default `HeaderWriterFilter`, which writes `Cache-Control: no-cache, no-store, max-age=0, must-revalidate` on every secured response. Removing our header lets that framework default apply. This **over-satisfies** the issue's goal — the browser never caches the density response at all (stronger than plain omission), and `no-store` is strictly safer than `private` for the per-user filtered data (no caching ⇒ no cross-user leak, addressing Nora's concern for free). Confirmed with the user before proceeding; the test now asserts this real contract. ## Changes - `DocumentController.density()` → `return ResponseEntity.ok(result)` (explicit header removed; unused `CacheControl` / `TimeUnit` imports dropped). - `DocumentControllerTest`: `density_emitsPrivateCacheControlHeader` → `density_isNeverBrowserCached`, updated **red-first**, now asserts the exact `no-cache, no-store, max-age=0, must-revalidate` contract. - `DocumentService.getDensity()` Javadoc: dropped the stale `max-age=300 … absorbs repeated browse loads` claim. No change to `getDensity()` computation logic (per out-of-scope). #481 (SQL aggregation) and ETag modeling remain parked. ## Verification - `DocumentControllerTest` — 105/105 green (red-first confirmed before the controller change). - `./mvnw clean package -DskipTests` — BUILD SUCCESS. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 1 commit 2026-06-01 19:57:12 +02:00

refactor(document): drop the 5-minute Cache-Control TTL on /density (#709) ... 50f554680c

The density chart is an interactive filter control; a 5-minute private
browser cache let it show stale month counts after an edit/upload/re-tag.
The in-memory aggregation is sub-200ms p95 over ~5k docs, so there is no
load reason to cache. Removing the explicit header lets Spring Security's
default no-store directive apply, so the response is always fresh.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

marcel referenced this pull request 2026-06-01 19:57:32 +02:00

Revisit the 5-minute Cache-Control TTL on GET /api/documents/density #709

marcel commented 2026-06-01 19:58:33 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

I reviewed this through the clean-code and TDD lens. It is a small, disciplined refactor and it reads correctly.

What I checked

• TDD evidence — the PR body states the test was updated red-first before the controller change, and the test name was renamed (density_emitsPrivateCacheControlHeader → density_isNeverBrowserCached) to describe the new behavior. The rename is the right move — a test name is documentation, and the old name would have lied after this change.
• Dead code — the unused CacheControl and java.util.concurrent.TimeUnit imports were dropped along with the header. Good. Removing the behavior without removing its imports is the most common leftover in a change like this, and it was caught.
• Guard clauses / function size — density() is unchanged in shape; return ResponseEntity.ok(result) is the simplest correct form. No nesting introduced.
• Comments explain why, not what — the updated getDensity() Javadoc now explains the reason for freshness (interactive chart, imperceptible recompute, issue #709) rather than restating a header value. That is exactly the kind of "why" comment I want; the old comment was a stale "what" that contradicted the code.

One thing I want to name (not a blocker)

The test now hard-codes the framework default string "no-cache, no-store, max-age=0, must-revalidate". That is a deliberate, honest assertion of the real contract — I prefer this over doesNotExist(...) because the AC's version would have been a false assertion (the header is present, written by Spring Security's HeaderWriterFilter). The deviation from the literal AC is justified and was confirmed with the user.

The only fragility: if the Spring Security default directive string ever changes on a framework upgrade, this test breaks. That is acceptable — it breaks loudly with a clear diff, which is precisely when you want to re-confirm the caching contract. I would not weaken it to a containsString("no-store").

Clean change. Tests precede code. Names reveal intent. Ship it.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** I reviewed this through the clean-code and TDD lens. It is a small, disciplined refactor and it reads correctly. ### What I checked - **TDD evidence** — the PR body states the test was updated red-first before the controller change, and the test name was renamed (`density_emitsPrivateCacheControlHeader` → `density_isNeverBrowserCached`) to describe the new behavior. The rename is the right move — a test name is documentation, and the old name would have lied after this change. - **Dead code** — the unused `CacheControl` and `java.util.concurrent.TimeUnit` imports were dropped along with the header. Good. Removing the behavior without removing its imports is the most common leftover in a change like this, and it was caught. - **Guard clauses / function size** — `density()` is unchanged in shape; `return ResponseEntity.ok(result)` is the simplest correct form. No nesting introduced. - **Comments explain why, not what** — the updated `getDensity()` Javadoc now explains the *reason* for freshness (interactive chart, imperceptible recompute, issue #709) rather than restating a header value. That is exactly the kind of "why" comment I want; the old comment was a stale "what" that contradicted the code. ### One thing I want to name (not a blocker) The test now hard-codes the framework default string `"no-cache, no-store, max-age=0, must-revalidate"`. That is a deliberate, honest assertion of the real contract — I prefer this over `doesNotExist(...)` because the AC's version would have been a **false** assertion (the header is present, written by Spring Security's `HeaderWriterFilter`). The deviation from the literal AC is justified and was confirmed with the user. The only fragility: if the Spring Security default directive string ever changes on a framework upgrade, this test breaks. That is acceptable — it breaks loudly with a clear diff, which is precisely when you *want* to re-confirm the caching contract. I would not weaken it to a `containsString("no-store")`. Clean change. Tests precede code. Names reveal intent. Ship it.

marcel commented 2026-06-01 19:58:46 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This change is a net security improvement, and I want to give credit for that explicitly.

Security analysis

The old header was Cache-Control: max-age=300, private. The density response is per-user filtered data (it reflects the caller's search/filter state). Under the old header, that response was cacheable in the user's own browser for 5 minutes. private correctly kept it out of shared caches, so this was never a cross-user leak (CWE-525) on its own — but it was the weaker of the available options.

By removing the explicit header, Spring Security's HeaderWriterFilter default takes over: Cache-Control: no-cache, no-store, max-age=0, must-revalidate. That means the response is not written to disk or memory cache at all. For data derived from a user's session/filter context, no-store is the strictly safer posture — it removes even the same-device, shared-machine residual-cache concern. This over-satisfies the issue's "just omit it" goal in the right direction.

On the deviation from the AC

The issue asked for header().doesNotExist("Cache-Control"). As the author correctly diagnosed, that assertion is impossible here — the framework always writes the header on secured responses. From a security-testing standpoint I strongly prefer the chosen approach: the test now asserts the actual caching contract the browser receives (no-store ...), which is the security-relevant fact. A doesNotExist test would have given false confidence that "nothing controls caching" when in fact no-store does. Codifying the real contract as a permanent regression test is exactly right — if someone later re-adds a weaker private, max-age=... header, this test fails.

One consistency note (not a blocker, not in scope)

The neighboring getDocumentThumbnail() endpoint deliberately sets private, max-age=31536000, immutable with a documented threat-model comment (CWE-525, cache-buster in the URL). That is the correct pattern for an immutable, URL-versioned binary. It is fine and unrelated — I mention it only so a future reader doesn't "harmonize" the two endpoints. They have different threat models: the thumbnail is immutable+versioned (cache aggressively), the density JSON is mutable per-filter (don't cache). Both are now correct.

No injection, no auth, no data-exposure concerns. Approved.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This change is a net security improvement, and I want to give credit for that explicitly. ### Security analysis The old header was `Cache-Control: max-age=300, private`. The density response is **per-user filtered data** (it reflects the caller's search/filter state). Under the old header, that response was cacheable in the user's own browser for 5 minutes. `private` correctly kept it out of shared caches, so this was never a cross-user leak (CWE-525) on its own — but it was the weaker of the available options. By removing the explicit header, Spring Security's `HeaderWriterFilter` default takes over: `Cache-Control: no-cache, no-store, max-age=0, must-revalidate`. That means **the response is not written to disk or memory cache at all**. For data derived from a user's session/filter context, `no-store` is the strictly safer posture — it removes even the same-device, shared-machine residual-cache concern. This over-satisfies the issue's "just omit it" goal in the right direction. ### On the deviation from the AC The issue asked for `header().doesNotExist("Cache-Control")`. As the author correctly diagnosed, that assertion is **impossible** here — the framework always writes the header on secured responses. From a security-testing standpoint I strongly prefer the chosen approach: the test now asserts the *actual* caching contract the browser receives (`no-store ...`), which is the security-relevant fact. A `doesNotExist` test would have given false confidence that "nothing controls caching" when in fact `no-store` does. Codifying the real contract as a permanent regression test is exactly right — if someone later re-adds a weaker `private, max-age=...` header, this test fails. ### One consistency note (not a blocker, not in scope) The neighboring `getDocumentThumbnail()` endpoint deliberately sets `private, max-age=31536000, immutable` with a documented threat-model comment (CWE-525, cache-buster in the URL). That is the correct pattern *for an immutable, URL-versioned binary*. It is fine and unrelated — I mention it only so a future reader doesn't "harmonize" the two endpoints. They have different threat models: the thumbnail is immutable+versioned (cache aggressively), the density JSON is mutable per-filter (don't cache). Both are now correct. No injection, no auth, no data-exposure concerns. Approved.

marcel commented 2026-06-01 19:58:59 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

A small, correct refactor that pushes a caching decision to where it belongs and documents the why. No layering or boundary concerns.

Layer boundaries

The change lives entirely in the controller (HTTP-transport concern) and a Javadoc note in the service. Correct placement: cache headers are a transport detail, not business logic. density() still delegates aggregation to documentService.getDensity() — the service contract is untouched, as the issue's "no computation change" constraint requires. No service reaches into another domain. Clean.

On the architectural decision and its record

The interesting part of this PR is the discovery that the explicit header was masking Spring Security's default no-store. That is a genuine architectural fact about how the HTTP layer behaves on secured endpoints, and the PR captures it in both the test (as an executable assertion) and the PR description. I'm satisfied that this is documented where it will be found.

I considered whether this warrants an ADR. My judgment: no. This is a localized, reversible config decision on one endpoint, not a lasting structural choice (no new tech, transport, or pattern). The issue #709 thread already records the context, alternatives (omit vs no-store vs ETag), and consequences. Parking ETag/conditional-requests in #481/#709 rather than building it now is the correct "simplest tool for the actual problem" call — a sub-200ms recompute does not justify conditional-request machinery. Boring, right-sized decision.

Documentation currency

I ran the doc-impact checklist. This PR adds no migration, no table/column, no join/FK, no new package, controller, service, route, Docker service, external system, ErrorCode, Permission, or domain term. The auth-flow and upload-flow sequence diagrams are unaffected (no auth logic changed — only a response header default surfaced). No diagram or doc update is owed. No blocker.

Approved.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** A small, correct refactor that pushes a caching decision to where it belongs and documents the *why*. No layering or boundary concerns. ### Layer boundaries The change lives entirely in the controller (HTTP-transport concern) and a Javadoc note in the service. Correct placement: cache headers are a transport detail, not business logic. `density()` still delegates aggregation to `documentService.getDensity()` — the service contract is untouched, as the issue's "no computation change" constraint requires. No service reaches into another domain. Clean. ### On the architectural decision and its record The interesting part of this PR is the discovery that the explicit header was **masking** Spring Security's default `no-store`. That is a genuine architectural fact about how the HTTP layer behaves on secured endpoints, and the PR captures it in both the test (as an executable assertion) and the PR description. I'm satisfied that this is documented where it will be found. I considered whether this warrants an ADR. My judgment: **no.** This is a localized, reversible config decision on one endpoint, not a lasting structural choice (no new tech, transport, or pattern). The issue #709 thread already records the context, alternatives (omit vs `no-store` vs ETag), and consequences. Parking ETag/conditional-requests in #481/#709 rather than building it now is the correct "simplest tool for the actual problem" call — a sub-200ms recompute does not justify conditional-request machinery. Boring, right-sized decision. ### Documentation currency I ran the doc-impact checklist. This PR adds no migration, no table/column, no join/FK, no new package, controller, service, route, Docker service, external system, `ErrorCode`, `Permission`, or domain term. The auth-flow and upload-flow sequence diagrams are unaffected (no auth logic changed — only a response header default surfaced). **No diagram or doc update is owed.** No blocker. Approved.

marcel commented 2026-06-01 19:59:16 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

The change is correct and the test was updated red-first, which I appreciate. My concerns are about test layer and coverage, not about blocking the merge.

What is good

• The test asserts the exact header string (no-cache, no-store, max-age=0, must-revalidate) rather than a loose containsString. Exact assertions catch silent widening — if someone re-introduces private, max-age=300, this fails. One logical behavior, one assertion. Name reads as a sentence. Good.
• Updated red-first per the PR body, so it is a meaningful test, not one written green-after-the-fact.

Concern 1 — this is a @WebMvcTest slice; verify the security filter is actually in the slice (not a blocker, but verify)

The new assertion depends on Spring Security's HeaderWriterFilter running in the test context. DocumentControllerTest must @Import the security config (the persona reference shows the canonical @WebMvcTest(DocumentController.class) @Import({SecurityConfig.class, PermissionAspect.class})). If the security config is imported, the no-store default is real and the test is valid — the fact that the author confirms 105/105 green with this exact string strongly implies it is. I just want it on record that this test is only meaningful because the security filter is in the slice; if a future refactor drops SecurityConfig from the import, this test would assert a default that no longer applies. A one-line comment in the test pointing at which filter writes the header would harden it. The author did add a comment naming "Spring Security's default no-store directive" — good enough.

Concern 2 — the assertion couples to a framework-internal string

This is the right trade-off given the AC was impossible, but I want it logged: the test now fails on a Spring Security upgrade that reorders or rewords the default directive. That is acceptable and arguably desirable (it forces a human to re-confirm the caching contract on upgrade), but it means this is a known "expected to break on framework bump" test. Worth a mental note for whoever does the next Spring Boot upgrade.

Coverage gap (minor, out of scope)

There is no integration-level test (full stack, real Postgres via Testcontainers) proving an actual edit is immediately reflected in /density — i.e. the user-facing behavior the issue cares about ("no 5-minute staleness after an edit"). The MockMvc test proves the header contract; it does not prove freshness end-to-end. That end-to-end behavior is now guaranteed by no-store + no server cache, so I am not asking for it here, but if the density chart ever grows a server-side cache (#481), that integration test becomes mandatory.

No flaky-test, no H2, no Thread.sleep concerns. Approved with the notes above.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** The change is correct and the test was updated red-first, which I appreciate. My concerns are about test layer and coverage, not about blocking the merge. ### What is good - The test asserts the **exact** header string (`no-cache, no-store, max-age=0, must-revalidate`) rather than a loose `containsString`. Exact assertions catch silent widening — if someone re-introduces `private, max-age=300`, this fails. One logical behavior, one assertion. Name reads as a sentence. Good. - Updated red-first per the PR body, so it is a meaningful test, not one written green-after-the-fact. ### Concern 1 — this is a `@WebMvcTest` slice; verify the security filter is actually in the slice (not a blocker, but verify) The new assertion depends on Spring Security's `HeaderWriterFilter` running in the test context. `DocumentControllerTest` must `@Import` the security config (the persona reference shows the canonical `@WebMvcTest(DocumentController.class) @Import({SecurityConfig.class, PermissionAspect.class})`). If the security config is imported, the `no-store` default is real and the test is valid — the fact that the author confirms 105/105 green with this exact string strongly implies it is. I just want it on record that **this test is only meaningful because the security filter is in the slice**; if a future refactor drops `SecurityConfig` from the import, this test would assert a default that no longer applies. A one-line comment in the test pointing at *which filter* writes the header would harden it. The author did add a comment naming "Spring Security's default no-store directive" — good enough. ### Concern 2 — the assertion couples to a framework-internal string This is the right trade-off given the AC was impossible, but I want it logged: the test now fails on a Spring Security upgrade that reorders or rewords the default directive. That is **acceptable and arguably desirable** (it forces a human to re-confirm the caching contract on upgrade), but it means this is a known "expected to break on framework bump" test. Worth a mental note for whoever does the next Spring Boot upgrade. ### Coverage gap (minor, out of scope) There is no integration-level test (full stack, real Postgres via Testcontainers) proving an *actual edit* is immediately reflected in `/density` — i.e. the user-facing behavior the issue cares about ("no 5-minute staleness after an edit"). The MockMvc test proves the *header contract*; it does not prove *freshness end-to-end*. That end-to-end behavior is now guaranteed by `no-store` + no server cache, so I am **not** asking for it here, but if the density chart ever grows a server-side cache (#481), that integration test becomes mandatory. No flaky-test, no H2, no `Thread.sleep` concerns. Approved with the notes above.

marcel commented 2026-06-01 19:59:25 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

I was cited in the issue as confirming there is no load reason to cache this at current scale, and nothing in the diff changes that assessment.

Operational impact

• No infrastructure touched — no Compose file, no CI workflow, no Caddyfile, no env var. Pure application-code change. Nothing for me to size or operate.
• No CDN / reverse-proxy cache in front of this endpoint. Caddy does not cache by default and we have no caching directive for /api/documents/density. So the old private, max-age=300 header only ever affected individual browsers — removing it has zero impact on origin load from shared caches (there were none).
• Load delta is negligible. Sub-200ms in-memory aggregation over ~5k docs, a handful of concurrent family users. Even with every density request now hitting the backend fresh, this is noise against the VPS budget (CX32, 4 vCPU / 8GB). No new failure mode, no new resource pressure. If anything this is less state to reason about.
• No monitoring/alerting change needed. If #481's SQL-aggregation work ever lands and density volume grows, that is the moment to revisit a server-side cache and an accompanying Grafana panel — not now. Profiling-before-scaling discipline holds; we are not pre-optimizing.

Nothing to flag from the platform side. Approved.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** I was cited in the issue as confirming there is no load reason to cache this at current scale, and nothing in the diff changes that assessment. ### Operational impact - **No infrastructure touched** — no Compose file, no CI workflow, no Caddyfile, no env var. Pure application-code change. Nothing for me to size or operate. - **No CDN / reverse-proxy cache in front of this endpoint.** Caddy does not cache by default and we have no caching directive for `/api/documents/density`. So the old `private, max-age=300` header only ever affected individual browsers — removing it has **zero** impact on origin load from shared caches (there were none). - **Load delta is negligible.** Sub-200ms in-memory aggregation over ~5k docs, a handful of concurrent family users. Even with every density request now hitting the backend fresh, this is noise against the VPS budget (CX32, 4 vCPU / 8GB). No new failure mode, no new resource pressure. If anything this is *less* state to reason about. - **No monitoring/alerting change needed.** If #481's SQL-aggregation work ever lands and density volume grows, that is the moment to revisit a server-side cache and an accompanying Grafana panel — not now. Profiling-before-scaling discipline holds; we are not pre-optimizing. Nothing to flag from the platform side. Approved.

marcel commented 2026-06-01 19:59:43 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer & Business Analyst

Verdict: ⚠️ Approved with concerns

Working in Brownfield mode. I assessed this against the issue's acceptance criteria and the underlying user need. The change is sound and well-justified; my concern is purely about AC traceability hygiene, which matters for a solo, issue-driven, LLM-assisted workflow.

Requirement satisfied — root need is met

The real requirement (problem language, not solution language): "While a user is acting on the interactive density chart, the system shall show month counts that reflect the user's most recent edits, without a multi-minute staleness window." This maps to Nielsen heuristic #1 (visibility of system status), correctly flagged in the issue. The chosen implementation (no-store via framework default) satisfies this even more strongly than plain header omission would have — staleness window is now zero. ✅ The benefit is delivered.

Concern — one written AC was not met as literally specified

The issue lists, as a checkbox AC:

DocumentControllerTest.density_emitsPrivateCacheControlHeader updated red-first to assert the header is absent: header().doesNotExist("Cache-Control")

The PR does not do this — it asserts the exact no-store string instead. The PR body explains, credibly, that doesNotExist is technically impossible because Spring Security always writes the header. I agree with the engineering decision. But from a requirements-discipline standpoint this is an unmet written AC, and the correct close-out is not to silently override it — it is to amend the issue body so the AC matches reality before merge. Right now the issue and the PR disagree on what "done" means. For a backlog that aims to be spec-dense and the source of truth, that drift is the thing to fix.

Recommendation (lightweight): edit issue #709's acceptance criteria to replace the doesNotExist line with: "assert the response carries the framework-default no-cache, no-store, max-age=0, must-revalidate (the explicit header was masking this default; omission yields a stronger no-cache contract than the AC originally assumed)." Then the merge closes a fully-satisfied issue. This is a 30-second edit, not a code change.

Ambiguity I want to name

The issue's own "Decision" section says "'Remove' = omit the header, not no-store" and even argues no-store "adds no real protection here." The PR delivers exactly no-store. So the delivered outcome contradicts a sentence in the agreed decision. The PR's reasoning (the default was already no-store, omission can't undo it) resolves the contradiction correctly — but again, this is precisely the kind of decision-vs-reality drift that should be folded back into the issue so the next reader isn't confused. The author notes it was "confirmed with the user," which is good; capture that confirmation in the issue body.

Other AC items — header removal (✅), Javadoc updated (✅), no computation change (✅), out-of-scope items parked (✅) — are all met. No NFR gaps: performance is unaffected (sub-200ms), no a11y/security/observability implications.

Approved, conditional on tidying the issue's ACs to match what shipped. Not a code blocker.

## 📋 Elicit — Requirements Engineer & Business Analyst **Verdict: ⚠️ Approved with concerns** Working in Brownfield mode. I assessed this against the issue's acceptance criteria and the underlying user need. The change is sound and well-justified; my concern is purely about **AC traceability hygiene**, which matters for a solo, issue-driven, LLM-assisted workflow. ### Requirement satisfied — root need is met The real requirement (problem language, not solution language): *"While a user is acting on the interactive density chart, the system shall show month counts that reflect the user's most recent edits, without a multi-minute staleness window."* This maps to Nielsen heuristic #1 (visibility of system status), correctly flagged in the issue. The chosen implementation (`no-store` via framework default) satisfies this even more strongly than plain header omission would have — staleness window is now zero. ✅ The benefit is delivered. ### Concern — one written AC was not met as literally specified The issue lists, as a checkbox AC: > `DocumentControllerTest.density_emitsPrivateCacheControlHeader` updated red-first to assert the header is absent: `header().doesNotExist("Cache-Control")` The PR does **not** do this — it asserts the exact `no-store` string instead. The PR body explains, credibly, that `doesNotExist` is technically impossible because Spring Security always writes the header. **I agree with the engineering decision.** But from a requirements-discipline standpoint this is an unmet written AC, and the correct close-out is not to silently override it — it is to **amend the issue body** so the AC matches reality before merge. Right now the issue and the PR disagree on what "done" means. For a backlog that aims to be spec-dense and the source of truth, that drift is the thing to fix. **Recommendation (lightweight):** edit issue #709's acceptance criteria to replace the `doesNotExist` line with: *"assert the response carries the framework-default `no-cache, no-store, max-age=0, must-revalidate` (the explicit header was masking this default; omission yields a stronger no-cache contract than the AC originally assumed)."* Then the merge closes a fully-satisfied issue. This is a 30-second edit, not a code change. ### Ambiguity I want to name The issue's own "Decision" section says **"'Remove' = omit the header, not `no-store`"** and even argues `no-store` "adds no real protection here." The PR delivers exactly `no-store`. So the *delivered outcome* contradicts a sentence in the *agreed decision*. The PR's reasoning (the default was already `no-store`, omission can't undo it) resolves the contradiction correctly — but again, this is precisely the kind of decision-vs-reality drift that should be folded back into the issue so the next reader isn't confused. The author notes it was "confirmed with the user," which is good; capture that confirmation in the issue body. Other AC items — header removal (✅), Javadoc updated (✅), no computation change (✅), out-of-scope items parked (✅) — are all met. No NFR gaps: performance is unaffected (sub-200ms), no a11y/security/observability implications. Approved, conditional on tidying the issue's ACs to match what shipped. Not a code blocker.

marcel commented 2026-06-01 19:59:52 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This is a backend-only change with no markup, styling, or component diff — so there is nothing for me to check on contrast, touch targets, focus rings, semantic HTML, or responsive breakpoints. But it is not UX-neutral, and I want to register why it is a positive for users.

Why this matters for the user

The density chart is an interactive filter control — users click a month or drag a range to filter. Under the old 5-minute browser cache, a user who uploaded, re-dated, transcribed, or re-tagged a document could click that chart and act on stale month counts for up to five minutes, with no indication anything was out of date. That is a direct violation of visibility of system status (Nielsen #1): the interface would have been confidently wrong on a control the user manipulates. Removing the cache means the chart always reflects the current archive state. The control is now honest about what it represents.

This is exactly the "design for the hardest constraint first" instinct applied to data freshness: the 60+ researcher who just corrected a document's date should see that correction reflected immediately, not wonder why the timeline still shows the old bucket.

No accessibility regression

No visual state, no animation, no color, no timing affordance changed. prefers-reduced-motion, contrast, and keyboard reachability are untouched. The freshness improvement happens transparently — the user is not asked to refresh or dismiss anything.

Nothing to fix. Approved from the UX/accessibility side.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This is a backend-only change with no markup, styling, or component diff — so there is nothing for me to check on contrast, touch targets, focus rings, semantic HTML, or responsive breakpoints. But it is not UX-neutral, and I want to register why it is a *positive* for users. ### Why this matters for the user The density chart is an **interactive filter control** — users click a month or drag a range to filter. Under the old 5-minute browser cache, a user who uploaded, re-dated, transcribed, or re-tagged a document could click that chart and act on **stale month counts** for up to five minutes, with no indication anything was out of date. That is a direct violation of *visibility of system status* (Nielsen #1): the interface would have been confidently wrong on a control the user manipulates. Removing the cache means the chart always reflects the current archive state. The control is now honest about what it represents. This is exactly the "design for the hardest constraint first" instinct applied to data freshness: the 60+ researcher who just corrected a document's date should see that correction reflected immediately, not wonder why the timeline still shows the old bucket. ### No accessibility regression No visual state, no animation, no color, no timing affordance changed. `prefers-reduced-motion`, contrast, and keyboard reachability are untouched. The freshness improvement happens transparently — the user is not asked to refresh or dismiss anything. Nothing to fix. Approved from the UX/accessibility side.

marcel merged commit 50f554680c into main 2026-06-01 20:01:32 +02:00

marcel deleted branch feat/issue-709-remove-density-cache-ttl 2026-06-01 20:01:32 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

needs-discussion

Has an open decision or design question that must be resolved before implementation can start.

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#711