cleanup(legibility): rename Helper/Utils/Manager violators to express intent #414

New Issue

marcel · 2026-05-04T16:17:34+02:00

marcel commented

2026-05-04 16:17:34 +02:00

Context

Part of Epic #411 — Cleanup. This is CLEANUP-3: act on C4.3 findings — file/class names that use generic suffixes (Helper, Utils, Manager, Handler without modifier) where a more specific name would tell the reader what the thing does.

Per the Legibility Rubric, this addresses C4.3 (Minor) — but accumulated naming sloppiness is a strong AI-smell signal.

Inputs

§7 of audit reports lists naming violations
Direct grep pass:

# Backend
find backend/src -name "*Helper.java" -o -name "*Utils.java" -o -name "*Manager.java" -o -name "*Handler.java"

# Frontend
find frontend/src -name "*Helper*" -o -name "*Utils*" -o -name "*Manager*" -o -name "*Handler*"

Method

For each offender:

Read the class/file. Identify what it actually does.
Decide:
- Specific rename — Utils.formatPersonName() → PersonNameFormatter.format() or move method to the class it operates on
- Justified — keep but add a one-line comment explaining why a generic name is correct (e.g., a true catch-all utility module)
- Defer — open follow-up if the rename would touch many call sites and isn't worth this issue's scope
Apply the rename across all references (IDE refactor or git mv + grep)
Run tests after each rename

Known cases (from requirements-phase research, not exhaustive)

Backend:

SecurityUtils.getCurrentUserId() — likely OK to keep as SecurityUtils (genuine utility, not a misnomer)
PersonNameParser — already a good name (class-noun + verb)
DisplayNameFormatter — already a good name
PolygonConverter — already a good name
Any *Helper we find should be reviewed

Frontend:

lib/utils/* — these are genuine utility modules. The folder structure is the concern, not the name. C4.3 mostly applies to the JS side via class-like exports.

Acceptance criteria

All *Helper, *Utils.java, *Manager, *Handler files have been classified (rename / justify / defer)
Each rename applied across all call sites; tests pass
PR opened and merged
Audit re-run (CLEANUP-5) confirms C4.3 PASS

Dependency

Soft dependency on Epic 4 refactor — easier after files are in domain packages.

Definition of Done

PR merged. Closing comment lists each rename in OldName → NewName form.

## Context Part of **Epic #411** — Cleanup. This is **CLEANUP-3**: act on C4.3 findings — file/class names that use generic suffixes (`Helper`, `Utils`, `Manager`, `Handler` without modifier) where a more specific name would tell the reader what the thing does. Per the Legibility Rubric, this addresses **C4.3 (Minor)** — but accumulated naming sloppiness is a strong AI-smell signal. ## Inputs - §7 of audit reports lists naming violations - Direct grep pass: ```bash # Backend find backend/src -name "*Helper.java" -o -name "*Utils.java" -o -name "*Manager.java" -o -name "*Handler.java" # Frontend find frontend/src -name "*Helper*" -o -name "*Utils*" -o -name "*Manager*" -o -name "*Handler*" ``` ## Method For each offender: 1. Read the class/file. Identify what it actually does. 2. Decide: - **Specific rename** — `Utils.formatPersonName()` → `PersonNameFormatter.format()` or move method to the class it operates on - **Justified** — keep but add a one-line comment explaining why a generic name is correct (e.g., a true catch-all utility module) - **Defer** — open follow-up if the rename would touch many call sites and isn't worth this issue's scope 3. Apply the rename across all references (IDE refactor or `git mv` + grep) 4. Run tests after each rename ## Known cases (from requirements-phase research, not exhaustive) Backend: - `SecurityUtils.getCurrentUserId()` — likely OK to keep as `SecurityUtils` (genuine utility, not a misnomer) - `PersonNameParser` — already a good name (class-noun + verb) - `DisplayNameFormatter` — already a good name - `PolygonConverter` — already a good name - Any `*Helper` we find should be reviewed Frontend: - `lib/utils/*` — these are genuine utility modules. The folder structure is the concern, not the name. C4.3 mostly applies to the JS side via class-like exports. ## Acceptance criteria - [ ] All `*Helper`, `*Utils.java`, `*Manager`, `*Handler` files have been classified (rename / justify / defer) - [ ] Each rename applied across all call sites; tests pass - [ ] PR opened and merged - [ ] Audit re-run (CLEANUP-5) confirms C4.3 PASS ## Dependency Soft dependency on Epic 4 refactor — easier after files are in domain packages. ## Definition of Done PR merged. Closing comment lists each rename in `OldName → NewName` form.

marcel added this to the Codebase Legibility milestone 2026-05-04 16:17:34 +02:00

marcel added the P3-later cleanup legibility labels 2026-05-04 16:18:39 +02:00

marcel commented

2026-05-04 21:04:28 +02:00

🏗️ Markus Keller — Senior Application Architect

Observations

Two confirmed naming violators exist in the backend. The frontend utils/ folder is not a violator — it's already purpose-decomposed into single-responsibility modules (date.ts, personFormat.ts, sanitize.ts, etc.), which is the ideal outcome.

SecurityUtils — the issue correctly flags this as "likely OK to keep." Looking at the actual class: it's a final class with a private constructor and a single static method requireUserId(). The "utility" wrapper exists because the Spring Authentication object can't be tested cleanly when it's injected directly into services. The name accurately describes what this is: a security utility that has no better domain home. Justified. No rename needed.

GlobalExceptionHandler — this is actually a legitimate edge case. Spring's @RestControllerAdvice mechanism requires a class that handles exceptions globally — the "Handler" suffix is load-bearing vocabulary from the framework. Compare: AuthenticationEntryPoint, AccessDeniedHandler — Spring's own naming uses "Handler" for cross-cutting concerns. The alternative name would be GlobalExceptionResolver or ApiErrorHandler, neither of which is more expressive. Justified. No rename needed.

RateLimitInterceptor — this one implements HandlerInterceptor (a Spring interface), so "Interceptor" is the correct suffix for the pattern. Not a violator — it follows Spring's naming convention exactly.

Recommendations

Close the loop quickly: The grep pass in the issue is comprehensive and the actual offender list is short — just two classes, both defensible. Document each in the closing PR comment with the OldName → OldName (justified: reason) format the DoD requires.
No rename needed for either class. The issue's own pre-analysis (SecurityUtils — likely OK to keep) is correct; the same reasoning applies to GlobalExceptionHandler.
If there's appetite for a structural improvement, the stronger architectural move is migrating toward the Epic 4 domain-package layout — that would make SecurityUtils's location (security/ flat package) more natural as security/AuthenticationResolver or similar. But that's a separate issue, not this one.
The frontend lib/utils.ts re-exports isoToGerman/germanToIso from lib/utils/date.ts. That barrel export is fine — the underlying file is well-named.

Open Decisions (omit this section entirely if none)

None. Both violators are defensible under the issue's own classification criteria.

## 🏗️ Markus Keller — Senior Application Architect ### Observations Two confirmed naming violators exist in the backend. The frontend `utils/` folder is **not** a violator — it's already purpose-decomposed into single-responsibility modules (`date.ts`, `personFormat.ts`, `sanitize.ts`, etc.), which is the ideal outcome. **`SecurityUtils`** — the issue correctly flags this as "likely OK to keep." Looking at the actual class: it's a `final` class with a private constructor and a single static method `requireUserId()`. The "utility" wrapper exists because the Spring `Authentication` object can't be tested cleanly when it's injected directly into services. The name accurately describes what this is: a security utility that has no better domain home. **Justified. No rename needed.** **`GlobalExceptionHandler`** — this is actually a legitimate edge case. Spring's `@RestControllerAdvice` mechanism requires a class that handles exceptions globally — the "Handler" suffix is load-bearing vocabulary from the framework. Compare: `AuthenticationEntryPoint`, `AccessDeniedHandler` — Spring's own naming uses "Handler" for cross-cutting concerns. The alternative name would be `GlobalExceptionResolver` or `ApiErrorHandler`, neither of which is more expressive. **Justified. No rename needed.** **`RateLimitInterceptor`** — this one `implements HandlerInterceptor` (a Spring interface), so "Interceptor" is the correct suffix for the pattern. Not a violator — it follows Spring's naming convention exactly. ### Recommendations - **Close the loop quickly**: The grep pass in the issue is comprehensive and the actual offender list is short — just two classes, both defensible. Document each in the closing PR comment with the `OldName → OldName (justified: reason)` format the DoD requires. - **No rename needed for either class.** The issue's own pre-analysis (`SecurityUtils — likely OK to keep`) is correct; the same reasoning applies to `GlobalExceptionHandler`. - If there's appetite for a structural improvement, the stronger architectural move is migrating toward the Epic 4 domain-package layout — that would make `SecurityUtils`'s location (`security/` flat package) more natural as `security/AuthenticationResolver` or similar. But that's a separate issue, not this one. - The frontend `lib/utils.ts` re-exports `isoToGerman/germanToIso` from `lib/utils/date.ts`. That barrel export is fine — the underlying file is well-named. ### Open Decisions _(omit this section entirely if none)_ None. Both violators are defensible under the issue's own classification criteria.

marcel commented

2026-05-04 21:04:44 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Ran the exact grep commands from the issue. The actual offender list is lean:

Backend (*.java):

SecurityUtils — final class, private constructor, one method: requireUserId(Authentication, UserService). This name correctly categorizes a cross-cutting security helper with no domain home. The method name itself reveals full intent. Justified.
GlobalExceptionHandler — annotated @RestControllerAdvice. "Handler" is Spring's own vocabulary for this pattern. Renaming to GlobalExceptionResolver gains nothing in clarity. Justified.
RateLimitInterceptor — implements Spring's HandlerInterceptor interface. "Interceptor" suffix is framework-idiomatic. Not a violator.

Frontend (src/):
Zero hits. The lib/utils/ folder is already properly decomposed: date.ts, personFormat.ts, sanitize.ts, debounce.ts, mention.ts, etc. Every file has a single responsibility and a name that reveals it. The top-level lib/utils.ts is a one-line barrel re-export — not a naming concern.

Recommendations

Both backend classes should be classified as "Justified" with inline comments that explain why, per the issue's Method step 2.
For SecurityUtils, add a one-liner: // Cross-cutting auth helper; no domain home — "Utils" is correct here.
For GlobalExceptionHandler, add: // "Handler" is Spring @RestControllerAdvice convention — not a generic suffix.
TDD note: SecurityUtilsTest exists and has 4 tests covering null auth, unauthenticated, user-not-found, and happy path. That's solid coverage for a 13-line class. No new tests needed unless the class grows.
The PR closing comment should list: SecurityUtils → SecurityUtils (justified: cross-cutting auth utility, no domain owner) and GlobalExceptionHandler → GlobalExceptionHandler (justified: Spring @RestControllerAdvice convention).

No open decisions — both classifications are clear from the code.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations Ran the exact grep commands from the issue. The actual offender list is lean: **Backend (`*.java`):** - `SecurityUtils` — `final` class, private constructor, one method: `requireUserId(Authentication, UserService)`. This name correctly categorizes a cross-cutting security helper with no domain home. The method name itself reveals full intent. Justified. - `GlobalExceptionHandler` — annotated `@RestControllerAdvice`. "Handler" is Spring's own vocabulary for this pattern. Renaming to `GlobalExceptionResolver` gains nothing in clarity. Justified. - `RateLimitInterceptor` — implements Spring's `HandlerInterceptor` interface. "Interceptor" suffix is framework-idiomatic. Not a violator. **Frontend (`src/`):** Zero hits. The `lib/utils/` folder is already properly decomposed: `date.ts`, `personFormat.ts`, `sanitize.ts`, `debounce.ts`, `mention.ts`, etc. Every file has a single responsibility and a name that reveals it. The top-level `lib/utils.ts` is a one-line barrel re-export — not a naming concern. ### Recommendations - **Both backend classes should be classified as "Justified"** with inline comments that explain why, per the issue's Method step 2. - For `SecurityUtils`, add a one-liner: `// Cross-cutting auth helper; no domain home — "Utils" is correct here.` - For `GlobalExceptionHandler`, add: `// "Handler" is Spring @RestControllerAdvice convention — not a generic suffix.` - TDD note: `SecurityUtilsTest` exists and has 4 tests covering null auth, unauthenticated, user-not-found, and happy path. That's solid coverage for a 13-line class. No new tests needed unless the class grows. - The PR closing comment should list: `SecurityUtils → SecurityUtils (justified: cross-cutting auth utility, no domain owner)` and `GlobalExceptionHandler → GlobalExceptionHandler (justified: Spring @RestControllerAdvice convention)`. ### No open decisions — both classifications are clear from the code.

marcel commented

2026-05-04 21:05:09 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

Reviewed both naming violators through a security lens.

SecurityUtils.requireUserId() — the static method pattern here deserves a security read independent of the naming question. The method is called in three controllers (DocumentController, TranscriptionBlockController, DashboardController). It validates that authentication != null, isAuthenticated() == true, and that the user exists in the database. The null guard and isAuthenticated() check are both correct — a Spring Authentication object in the context can be non-null but still represent an anonymous user.

One security-relevant observation: findByEmail() returns null for unknown users, and the code guards with if (user == null) throw DomainException.unauthorized(...). This is correct fail-closed behavior. However, findByEmail() returning null rather than Optional<AppUser> is a smell — a future maintainer might accidentally miss the null check. This is worth a low-priority note but not a blocker for this issue.

GlobalExceptionHandler — the handleGeneric(Exception ex) catch-all logs ex via SLF4J parameterized logging (log.error("Unhandled exception", ex)) which is correct. The response body returns only "An unexpected error occurred" — no stack trace leakage. Error codes are from the ErrorCode enum, not raw exception messages. Clean.

RateLimitInterceptor — this class has a well-documented comment explaining the X-Forwarded-For trust model. The trusted proxy check correctly limits to loopback, RFC-1918 ranges (with the 172.16/12 range correctly implemented). This is security-relevant naming: "Interceptor" correctly signals that it runs on every request. Renaming would obscure this.

Recommendations

Keep all three names as-is. The naming in this area is not a security concern.
Low-priority follow-up: consider Optional<AppUser> return type from findByEmail() to make the null check more idiomatic and compiler-enforced. That's a PersonService/UserService cleanup, separate from this issue.
The SecurityUtils test covers the null user path. Good. No regression risk from this cleanup issue.

No open decisions.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations Reviewed both naming violators through a security lens. **`SecurityUtils.requireUserId()`** — the static method pattern here deserves a security read independent of the naming question. The method is called in three controllers (`DocumentController`, `TranscriptionBlockController`, `DashboardController`). It validates that `authentication != null`, `isAuthenticated() == true`, and that the user exists in the database. The `null` guard and `isAuthenticated()` check are both correct — a Spring `Authentication` object in the context can be non-null but still represent an anonymous user. One security-relevant observation: `findByEmail()` returns `null` for unknown users, and the code guards with `if (user == null) throw DomainException.unauthorized(...)`. This is correct fail-closed behavior. However, `findByEmail()` returning `null` rather than `Optional<AppUser>` is a smell — a future maintainer might accidentally miss the null check. This is worth a low-priority note but not a blocker for this issue. **`GlobalExceptionHandler`** — the `handleGeneric(Exception ex)` catch-all logs `ex` via SLF4J parameterized logging (`log.error("Unhandled exception", ex)`) which is correct. The response body returns only `"An unexpected error occurred"` — no stack trace leakage. Error codes are from the `ErrorCode` enum, not raw exception messages. Clean. **`RateLimitInterceptor`** — this class has a well-documented comment explaining the X-Forwarded-For trust model. The trusted proxy check correctly limits to loopback, RFC-1918 ranges (with the 172.16/12 range correctly implemented). This is security-relevant naming: "Interceptor" correctly signals that it runs on every request. Renaming would obscure this. ### Recommendations - Keep all three names as-is. The naming in this area is not a security concern. - Low-priority follow-up: consider `Optional<AppUser>` return type from `findByEmail()` to make the null check more idiomatic and compiler-enforced. That's a `PersonService`/`UserService` cleanup, separate from this issue. - The `SecurityUtils` test covers the `null` user path. Good. No regression risk from this cleanup issue. ### No open decisions.

marcel referenced this issue

2026-05-04 21:05:31 +02:00

epic(legibility): polish — remove smells surfaced by audits #411

marcel commented

2026-05-04 21:05:35 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

This is a classification issue, not a feature issue, so the test strategy is minimal. Here's what exists and what the risk profile looks like:

SecurityUtils — SecurityUtilsTest exists with 4 @ExtendWith(MockitoExtension.class) tests covering all branches of the method. This is the right layer (pure Mockito, no Spring context). If the class were renamed, the test file would need to follow. Since the classification is "Justified / no rename", there's no test work.

GlobalExceptionHandler — I searched for a GlobalExceptionHandlerTest and found none. The handler is @RestControllerAdvice and is effectively integration-tested via any @WebMvcTest slice that exercises the error paths. But there's no dedicated test proving each @ExceptionHandler method maps to the expected HTTP status and response structure. This is a gap independent of this naming issue.

RateLimitInterceptor — no test file found. Rate limiting logic is moderately complex (Caffeine cache, X-Forwarded-For trust chain). The trusted-proxy logic with the 172.16/12 boundary check is the kind of thing that breaks silently under edge cases. This is also independent of the naming issue.

Recommendations

For this issue: no test changes required (no renames happening).
Flag as follow-up issues (out of scope here, but worth tracking):
1. Add GlobalExceptionHandlerTest — a @WebMvcTest slice verifying that DomainException, MethodArgumentNotValidException, and the generic handler each produce the correct HTTP status and ErrorResponse body.
2. Add RateLimitInterceptorTest — unit tests for resolveClientIp() covering: direct connection, trusted-proxy forwarded, untrusted-proxy ignored, and the 172.16–172.31 boundary.
The acceptance criterion "Audit re-run (CLEANUP-5) confirms C4.3 PASS" is well-defined and testable — good DoD.

No open decisions.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations This is a classification issue, not a feature issue, so the test strategy is minimal. Here's what exists and what the risk profile looks like: **`SecurityUtils`** — `SecurityUtilsTest` exists with 4 `@ExtendWith(MockitoExtension.class)` tests covering all branches of the method. This is the right layer (pure Mockito, no Spring context). If the class were renamed, the test file would need to follow. Since the classification is "Justified / no rename", there's no test work. **`GlobalExceptionHandler`** — I searched for a `GlobalExceptionHandlerTest` and found none. The handler is `@RestControllerAdvice` and is effectively integration-tested via any `@WebMvcTest` slice that exercises the error paths. But there's no dedicated test proving each `@ExceptionHandler` method maps to the expected HTTP status and response structure. This is a gap independent of this naming issue. **`RateLimitInterceptor`** — no test file found. Rate limiting logic is moderately complex (Caffeine cache, X-Forwarded-For trust chain). The trusted-proxy logic with the `172.16/12` boundary check is the kind of thing that breaks silently under edge cases. This is also independent of the naming issue. ### Recommendations - For this issue: no test changes required (no renames happening). - Flag as follow-up issues (out of scope here, but worth tracking): 1. Add `GlobalExceptionHandlerTest` — a `@WebMvcTest` slice verifying that `DomainException`, `MethodArgumentNotValidException`, and the generic handler each produce the correct HTTP status and `ErrorResponse` body. 2. Add `RateLimitInterceptorTest` — unit tests for `resolveClientIp()` covering: direct connection, trusted-proxy forwarded, untrusted-proxy ignored, and the 172.16–172.31 boundary. - The acceptance criterion "Audit re-run (CLEANUP-5) confirms C4.3 PASS" is well-defined and testable — good DoD. ### No open decisions.

marcel commented

2026-05-04 21:06:01 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

This is a backend/developer-experience issue with no direct UI impact. No frontend components are being renamed, no user-visible strings or labels are involved, and no Svelte files are in scope.

I checked the frontend lib/utils/ folder since it was listed in the issue inputs. What I found: 24 purpose-specific files (date.ts, personFormat.ts, mention.ts, sanitize.ts, etc.), each with a corresponding *.spec.ts. This is a healthy state — the utils folder is already doing what C4.3 asks for. No component naming violations found.

Recommendations

No UX or accessibility work required for this issue.
From a developer-experience standpoint (which bleeds into UX for the people who maintain this codebase): the frontend naming is in good shape. The audit re-run in CLEANUP-5 should confirm a PASS for the frontend side without any changes.
If this issue surfaces any *Utils* files that get renamed or moved, flag the corresponding i18n keys and translation files to ensure no string keys rely on the old class/module path. In this case, none do.

No concerns from my angle on the UX/accessibility side — the frontend naming is already expressive and domain-oriented.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations This is a backend/developer-experience issue with no direct UI impact. No frontend components are being renamed, no user-visible strings or labels are involved, and no Svelte files are in scope. I checked the frontend `lib/utils/` folder since it was listed in the issue inputs. What I found: 24 purpose-specific files (`date.ts`, `personFormat.ts`, `mention.ts`, `sanitize.ts`, etc.), each with a corresponding `*.spec.ts`. This is a healthy state — the utils folder is already doing what C4.3 asks for. No component naming violations found. ### Recommendations - No UX or accessibility work required for this issue. - From a developer-experience standpoint (which bleeds into UX for the people who maintain this codebase): the frontend naming is in good shape. The audit re-run in CLEANUP-5 should confirm a PASS for the frontend side without any changes. - If this issue surfaces any `*Utils*` files that get renamed or moved, flag the corresponding i18n keys and translation files to ensure no string keys rely on the old class/module path. In this case, none do. No concerns from my angle on the UX/accessibility side — the frontend naming is already expressive and domain-oriented.

marcel referenced this issue

2026-05-04 21:06:27 +02:00

epic(legibility): polish — remove smells surfaced by audits #411

marcel commented

2026-05-04 21:06:29 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

This is a pure source-code refactor with no infrastructure, CI pipeline, or Docker Compose impact. Running the grep commands from the issue confirms the scope is two Java files and the frontend utils/ folder. No build scripts, Dockerfiles, environment variables, or deployment configs are in scope.

A few things I did check:

CI impact: Renaming Java classes triggers a recompile of all files in the same compilation unit (whole Maven module). Since SecurityUtils is imported in 3 controllers and GlobalExceptionHandler is in the controller package, any rename would produce a clean compile cycle. No CI pipeline changes needed.
Build caching: Maven caches compiled output by content hash. A rename invalidates the cache for affected modules. This is expected and harmless — the first post-rename CI run takes slightly longer, subsequent runs are cached again.
Test execution: SecurityUtilsTest exists and runs in the unit test phase. The test is pure Mockito — no Testcontainers, no Spring context, no Docker dependency. Fast and reliable.

Recommendations

No infrastructure changes needed for this issue.
Since both classes are being classified as "Justified" (no rename), there are literally zero build, CI, or deployment implications.
If the Epic 4 domain package restructuring (mentioned as a soft dependency) does happen, that will require careful CI cache invalidation and could briefly break incremental builds. Flag that when the time comes.

No concerns from the platform side.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations This is a pure source-code refactor with no infrastructure, CI pipeline, or Docker Compose impact. Running the grep commands from the issue confirms the scope is two Java files and the frontend `utils/` folder. No build scripts, Dockerfiles, environment variables, or deployment configs are in scope. A few things I did check: - **CI impact**: Renaming Java classes triggers a recompile of all files in the same compilation unit (whole Maven module). Since `SecurityUtils` is imported in 3 controllers and `GlobalExceptionHandler` is in the controller package, any rename would produce a clean compile cycle. No CI pipeline changes needed. - **Build caching**: Maven caches compiled output by content hash. A rename invalidates the cache for affected modules. This is expected and harmless — the first post-rename CI run takes slightly longer, subsequent runs are cached again. - **Test execution**: `SecurityUtilsTest` exists and runs in the unit test phase. The test is pure Mockito — no Testcontainers, no Spring context, no Docker dependency. Fast and reliable. ### Recommendations - No infrastructure changes needed for this issue. - Since both classes are being classified as "Justified" (no rename), there are literally zero build, CI, or deployment implications. - If the Epic 4 domain package restructuring (mentioned as a soft dependency) does happen, _that_ will require careful CI cache invalidation and could briefly break incremental builds. Flag that when the time comes. No concerns from the platform side.

marcel commented

2026-05-04 21:06:53 +02:00

📋 Elicit — Requirements Engineer

Observations

This issue is well-specified for a cleanup task. The Method, Known Cases, and Acceptance Criteria sections give an implementor everything needed to work through the list without ambiguity. The DoD format ("closing comment lists each rename in OldName → NewName form") is measurable and specific.

A few requirement-quality notes:

Scope is clear and bounded. The grep commands are provided. The pre-classified list of Known Cases reduces research overhead. The "Defer" option in the method provides an escape valve for expensive renames — this prevents scope creep during execution.

The acceptance criterion "Audit re-run (CLEANUP-5) confirms C4.3 PASS" depends on an external issue. This is a cross-issue dependency that should be tracked. If CLEANUP-5 doesn't exist yet as a Gitea issue, it should be created before this issue is closed, or the acceptance criterion should be rephrased: "Audit re-run criteria are met (verified by implementor; CLEANUP-5 will confirm formally)."

"Not exhaustive" caveat is important. The Known Cases list says "not exhaustive" — this means the implementor must run the grep commands, not just work from the pre-classified list. The acceptance criterion "All *Helper, *Utils.java, *Manager, *Handler files have been classified" correctly captures this.

Recommendations

The issue is implementation-ready. No refinement needed before starting.
One precision suggestion: the acceptance criterion uses *Utils.java (Java-specific suffix) but the grep command also covers frontend. Consider specifying whether the frontend lib/utils/ directory structure is in scope for classification or explicitly out of scope. Based on the issue body ("The folder structure is the concern, not the name. C4.3 mostly applies to the JS side via class-like exports"), the folder is excluded — but the AC doesn't say this explicitly.
Recommend adding to the closing PR comment format: GlobalExceptionHandler → GlobalExceptionHandler (justified: Spring @RestControllerAdvice naming convention) and SecurityUtils → SecurityUtils (justified: cross-cutting auth utility). This satisfies the DoD without requiring a rename.

No open decisions.

## 📋 Elicit — Requirements Engineer ### Observations This issue is well-specified for a cleanup task. The Method, Known Cases, and Acceptance Criteria sections give an implementor everything needed to work through the list without ambiguity. The DoD format ("closing comment lists each rename in `OldName → NewName` form") is measurable and specific. A few requirement-quality notes: **Scope is clear and bounded.** The grep commands are provided. The pre-classified list of Known Cases reduces research overhead. The "Defer" option in the method provides an escape valve for expensive renames — this prevents scope creep during execution. **The acceptance criterion "Audit re-run (CLEANUP-5) confirms C4.3 PASS" depends on an external issue.** This is a cross-issue dependency that should be tracked. If CLEANUP-5 doesn't exist yet as a Gitea issue, it should be created before this issue is closed, or the acceptance criterion should be rephrased: "Audit re-run criteria are met (verified by implementor; CLEANUP-5 will confirm formally)." **"Not exhaustive" caveat is important.** The Known Cases list says "not exhaustive" — this means the implementor must run the grep commands, not just work from the pre-classified list. The acceptance criterion "All `*Helper`, `*Utils.java`, `*Manager`, `*Handler` files have been classified" correctly captures this. ### Recommendations - The issue is implementation-ready. No refinement needed before starting. - One precision suggestion: the acceptance criterion uses `*Utils.java` (Java-specific suffix) but the grep command also covers frontend. Consider specifying whether the frontend `lib/utils/` directory structure is in scope for classification or explicitly out of scope. Based on the issue body ("The folder structure is the concern, not the name. C4.3 mostly applies to the JS side via class-like exports"), the folder is excluded — but the AC doesn't say this explicitly. - Recommend adding to the closing PR comment format: `GlobalExceptionHandler → GlobalExceptionHandler (justified: Spring @RestControllerAdvice naming convention)` and `SecurityUtils → SecurityUtils (justified: cross-cutting auth utility)`. This satisfies the DoD without requiring a rename. ### No open decisions.

marcel referenced this issue

2026-05-04 21:07:13 +02:00

epic(legibility): polish — remove smells surfaced by audits #411

marcel commented

2026-05-07 09:35:26 +02:00

CLEANUP-3 — Closing summary

Ran the full grep pass as specified in the issue. Results:

Backend (*.java):

Class	Classification	Justification comment added
`SecurityUtils` → `SecurityUtils`	Justified — cross-cutting auth helper, no domain home	✅
`GlobalExceptionHandler` → `GlobalExceptionHandler`	Justified — Spring `@RestControllerAdvice` naming convention	✅
`RateLimitInterceptor`	Not a violator — implements Spring `HandlerInterceptor` interface	—

Frontend (src/): Zero hits. lib/utils/ is already purpose-decomposed into 24 single-responsibility modules (date.ts, personFormat.ts, sanitize.ts, etc.).

No renames needed. Two one-line justification comments added to prevent future confusion.

Backend test suite: 1516 tests ✅

Implemented in commit 0fa90d58 on branch feat/issue-411-legibility-cleanup.

## CLEANUP-3 — Closing summary Ran the full grep pass as specified in the issue. Results: **Backend (`*.java`):** | Class | Classification | Justification comment added | |-------|---------------|----------------------------| | `SecurityUtils` → `SecurityUtils` | **Justified** — cross-cutting auth helper, no domain home | ✅ | | `GlobalExceptionHandler` → `GlobalExceptionHandler` | **Justified** — Spring `@RestControllerAdvice` naming convention | ✅ | | `RateLimitInterceptor` | Not a violator — implements Spring `HandlerInterceptor` interface | — | **Frontend (`src/`):** Zero hits. `lib/utils/` is already purpose-decomposed into 24 single-responsibility modules (`date.ts`, `personFormat.ts`, `sanitize.ts`, etc.). No renames needed. Two one-line justification comments added to prevent future confusion. Backend test suite: 1516 tests ✅ Implemented in commit `0fa90d58` on branch `feat/issue-411-legibility-cleanup`.

marcel closed this issue

2026-05-07 09:35:45 +02:00

marcel referenced this issue from a commit

2026-05-07 09:35:55 +02:00

cleanup(legibility): convert TODOs to issue refs; justify naming violators

marcel referenced this issue

2026-05-07 09:36:25 +02:00

cleanup(legibility): polish — CLEANUP-2, CLEANUP-3, CLEANUP-4 #455

marcel referenced this issue

2026-05-07 09:43:01 +02:00

cleanup(legibility): polish — CLEANUP-2, CLEANUP-3, CLEANUP-4 #455

Sign in to join this conversation.