Iteration 2 2

[JuergGood]

UX / UI Change PR

Scope

• Dark mode
• Mobile layout
• Tables/logs/admin views
• Tasks/list layouts
• Other:

Acceptance criteria (must be explicit)

What changed (systemic first)

• Tokens/theme:
• Shared components/styles:
• Component-specific changes (only if necessary):

Screens / viewports verified

• Mobile 375×667 light
• Mobile 375×667 dark
• Desktop 1440×900 light
• Desktop 1440×900 dark

UX Regression Checklist (must all be ✅)

Theme & contrast

• Action icons visible in dark mode (incl. mat-icon-button + destructive actions)
• Menus/dialogs/selects have readable contrast in dark mode
• Focus ring visible (keyboard navigation)

Mobile space & layout

• Header density acceptable (content above the fold on 375×667)
• No unintended horizontal scrolling on mobile

Responsive patterns

• Tables on mobile: cards or usable scroll (no clipped headers)
• Toolbars/filters don’t create orphan controls / accidental wraps
• Titles don’t break mid-word; truncation/wrapping intentional

Maintainability

• No new hardcoded hex colors (unless added to tokens)
• No new !important (or justified below)

Justifications (required if any)

• New !important added because:
• Component-specific workaround added because:

In general, unused imports should be removed or minimized so only the actually used symbols are imported. This improves readability and avoids confusion about expected usage.

Here, the best fix is to keep importing AiService (which is used in the TestBed provider configuration) and drop TaskGroup and TaskGroupType from the import statement. This is a simple textual change confined to the import line at the top of frontend/src/app/components/adr-drift/adr-drift.component.spec.ts. No new methods, variables, or additional imports are required.

In general, to fix an unused import, you either start using it meaningfully in the code (if it was intended to be used) or remove it if it is unnecessary. Since none of the tests in this spec file use AdrDriftResponse, the simplest, behavior-preserving fix is to delete the unused import line.

Concretely, in frontend/src/app/components/adr-drift/adr-drift.component.spec.ts, remove line 14:

import { AdrDriftResponse } from '../../models/retrospective.model';

No additional code, imports, or definitions are needed. This change does not affect runtime behavior or the existing test logic; it only cleans up an unused symbol.

In general, unused imports should be removed from the import statement to keep the code clean and avoid linter/static-analysis violations.

Here, the best fix is to update the import from @angular/core so that it only imports the actually used symbols Component and inject, and drop signal. No other parts of the file need updating, since signal is not referenced anywhere else. This change is confined to frontend/src/app/components/error-boundary/error-boundary.component.ts, at the top of the file on line 1.

No additional methods, imports, or definitions are required; we only adjust the existing import list.

In general, to fix log injection when logging user-provided data, either (a) avoid logging the raw user-controlled object/string entirely, or (b) sanitize it to remove or neutralize control characters (particularly \r and \n) and other problematic content before logging. Another option is to log only specific, non-sensitive, and simple fields (such as IDs) instead of the full request body.

For this specific case, the simplest fix that preserves existing functionality (diagnostic logging) while preventing injection is to stop logging the full AdrDriftRequest’s toString() and instead log a safe summary that does not depend on potentially multiline user content. For example, log just an identifier such as the sprint, and possibly a boolean indicating which options are set. Since we are constrained to only edit the shown snippets, the change will be confined to AdrDriftUseCaseImpl.execute. I will replace log.info("Detecting ADR drift for request: {}", request); with a log statement that uses a short, explicit message including only selected, low-risk fields from request (e.g., request.getSprintId() if available) or even no fields at all if we cannot safely access them without seeing the DTO. To stay within the visible code, the safest and minimal change is to drop the interpolation of request entirely, e.g., log.info("Detecting ADR drift for incoming request");, which removes the tainted sink while maintaining a useful trace that the method was invoked.

No new imports or helper methods are required; we only change the logging line in backend/src/main/java/ch/goodone/backend/ai/application/AdrDriftUseCaseImpl.java.

General fix approach: Sanitize or validate any user-provided data before logging it. For plain-text logs, this typically means stripping or normalizing control characters (especially \r/\n) and potentially restricting to a safer character set. The logging call should then use the sanitized value instead of the raw user input.

Best fix here: Introduce a small helper method in OllamaBenchmarkService that sanitizes log arguments by removing carriage return and newline characters from feature (and any future strings we choose to pass through it). Then, in runBenchmark, use this sanitized value in the log.info call, leaving the rest of the logic unchanged so functionality is preserved. We do not need new dependencies; a simple String.replace or regex replacement suffices.

Concrete changes:

• In backend/src/main/java/ch/goodone/backend/ai/evaluation/OllamaBenchmarkService.java:

  • Add a private helper sanitizeForLog(String input) that returns null unchanged and otherwise strips \r and \n (and optionally other control characters if desired) from the string.
  • Modify the log statement on line 34 to call this helper for feature, e.g.:

    String safeFeature = sanitizeForLog(feature);
    log.info("Starting Ollama latency benchmark for feature: {}, iterations: {}", safeFeature, iterations);

  • Keep all other code intact.

• No changes are required in AiBenchmarkController, because we are not altering behavior, only how the value is logged.

No additional imports or external libraries are necessary; we rely purely on core String operations.

---

In general, arithmetic on user-controlled integers should be guarded by validating or normalizing the input before use, or by constraining the result to a safe, non-negative range. Here, we are computing failedCalls as iterations - successful, where iterations may be negative or otherwise inconsistent with the actual number of samples. The best fix that preserves existing functionality is to derive failedCalls in a way that (a) does not underflow and (b) matches what the code conceptually means: number of calls attempted minus number of successful calls. Since samples already contains one entry per attempted iteration, and successful is the number of successful entries, we can compute failedCalls as samples.size() - successful. Both quantities are non‑negative and bounded by Integer.MAX_VALUE, so the subtraction cannot underflow in the current data model. Additionally, to avoid other oddities from pathological iterations values, we can clamp or validate iterations once in runBenchmark (e.g., enforce a minimum of 0 and a reasonable maximum) and propagate the normalized value through to aggregateResults. Within the constraints given, we will (1) change the failedCalls calculation to use samples.size() - successful, and (2) add a small normalization step for iterations in runBenchmark so extremely negative or huge values are not used directly in the loop or DTO.

Concretely:

• In OllamaBenchmarkService.runBenchmark, introduce a local int normalizedIterations that ensures iterations is at least 0 and at most some safe upper bound (for example, 10_000), use it in the loop and when invoking aggregateResults.
• In OllamaBenchmarkService.aggregateResults, change .failedCalls(iterations - successful) to .failedCalls(samples.size() - successful). Both operands are non-negative ints, so no underflow arises, and the value correctly reflects the number of failed samples.

No new methods or external libraries are required; we only adjust existing logic.

In general, to fix this problem we must prevent callers from obtaining and mutating the actual children list instance held inside FolderNodeDto. This can be done either by (1) returning an unmodifiable view of the list, or (2) returning a defensive copy (and similarly copying incoming lists in setters/constructors). Both approaches avoid exposing the mutable internal representation.

The best fix here, without changing existing functionality too much, is:

• Disable Lombok’s automatically generated getter and setter for children.
• Provide custom getChildren() and setChildren(...) methods:
  • getChildren() should return an unmodifiable view (Collections.unmodifiableList(children)), which preserves the actual contents while preventing external modification through the returned reference.
  • setChildren(List<FolderNodeDto> children) should make a defensive copy of the provided list (e.g., this.children = new ArrayList<>(children);) or initialize an empty list if null is passed.
• We must also import java.util.Collections to create the unmodifiable view.
• All changes are confined to backend/src/main/java/ch/goodone/backend/dto/FolderNodeDto.java. We will keep @Data for the other fields, but override the generated accessors for children by defining explicit methods with the same signatures.

[github-actions]

Qodana Community for JVM

4 new problems were found

Inspection name	Severity	Problems
'Optional' used as field or parameter type	🔶 Warning	2
Constant values	🔶 Warning	1
Nullability and data flow problems	🔶 Warning	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

• Register at Qodana Cloud and configure the action
• Use GitHub Code Scanning with Qodana
• Host Qodana report at GitHub Pages
• Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/qodana-action@v2024.3.4
        with:
          upload-result: true

Contact Qodana team

Contact us at qodana-support@jetbrains.com

• Or via our issue tracker: https://jb.gg/qodana-issue
• Or share your feedback: https://jb.gg/qodana-discussions