Skip to content

Conversation

matthilo96
Copy link
Contributor

@matthilo96 matthilo96 commented Aug 22, 2025

Pull Request Checklist (Feature Branch to next):

  • Ich habe die neuesten Änderungen aus dem next Branch in meinen Feature-Branch gemergt.
  • Das Code-Review wurde abgeschlossen.
  • Fachliche Tests wurden durchgeführt und sind abgeschlossen.

Summary by CodeRabbit

  • New Features

    • Exposes reservationDuration (minutes) in scope data across API responses and adds a client-side reservation timer that disables booking actions when expired.
  • Error Handling

    • Recognizes session timeout and shows a session-expired error callout.
  • Localization

    • Added EN/DE translations for session timeout header and message.
  • Schema

    • thinnedScope schema now includes reservationDuration.
  • Tests

    • Updated tests to cover reservationDuration propagation and timeout behavior.

Copy link
Contributor

coderabbitai bot commented Aug 22, 2025

Warning

Rate limit exceeded

@matthilo96 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 49 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between f264486 and 05b79af.

📒 Files selected for processing (1)
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (4 hunks)

Walkthrough

Adds an optional integer reservationDuration to ThinnedScope, propagates it through mappers and the API facade, updates schema and many tests, and implements a frontend reservation timer (provide/inject + useReservationTimer) that gates CustomerInfo and AppointmentSummary with session-timeout translations and an error state.

Changes

Cohort / File(s) Summary
Model: ThinnedScope
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php
Adds public ?int $reservationDuration = null, extends constructor to accept ?int $reservationDuration, adds getReservationDuration(): ?int, and includes reservationDuration in toArray() serialization.
Core mapping/services
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php, zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
Adds MapperService::extractReservationDuration(...); mapping flows call it and pass reservationDuration into constructed ThinnedScope and Office objects and facade outputs.
API tests (controllers)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/*ControllerTest.php, zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/*ControllerTest.php, zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/*ControllerTest.php
Updated expected API fixtures to include scope.reservationDuration (null or explicit value) across many tests.
Frontend components (expiration gating)
zmscitizenview/src/components/Appointment/AppointmentSummary.vue, zmscitizenview/src/components/Appointment/CustomerInfo.vue
Import and use useReservationTimer; add isExpired gating and MucCallout; hide forms/actions when expired.
Frontend reservation timer wiring
zmscitizenview/src/components/Appointment/AppointmentView.vue, zmscitizenview/src/utils/useReservationTimer.ts
Provide reservationStartMs via provide; new useReservationTimer() computes deadline, remainingMs, isExpired, and timeLeftString from appointment.scope.reservationDuration.
Frontend errors and translations
zmscitizenview/src/utils/errorHandler.ts, zmscitizenview/src/utils/en-US.json, zmscitizenview/src/utils/de-DE.json
Add apiErrorSessionTimeout error state and translation keys apiErrorSessionTimeoutHeader / apiErrorSessionTimeoutText in EN/DE (note: DE keys appear duplicated in diff).
Frontend unit tests
zmscitizenview/tests/unit/AppointmentSummary.spec.ts, zmscitizenview/tests/unit/CustomerInfo.spec.ts
Tests updated with mocks/provide for reservationStartMs, improved stubs/helpers, and new session-timeout tests asserting expired-state UI and behavior.
Schema
zmsentities/schema/citizenapi/thinnedScope.json
Adds optional reservationDuration property with type ["integer","null"] and description "Duration of the reservation in minutes".

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant View as AppointmentView.vue
  participant Hook as useReservationTimer
  participant Cust as CustomerInfo.vue
  participant Sum as AppointmentSummary.vue

  User->>View: Reserve appointment
  View->>View: reservationStartMs = Date.now()
  View->>Cust: provide appointment, reservationStartMs
  View->>Sum: provide appointment, reservationStartMs

  rect rgba(200,220,255,0.12)
  note over Hook: compute deadline = reservationStartMs + reservationDuration (minutes)
  Cust->>Hook: useReservationTimer()
  Sum->>Hook: useReservationTimer()
  Hook-->>Cust: isExpired, remainingMs, timeLeftString
  Hook-->>Sum: isExpired, remainingMs, timeLeftString
  end

  alt isExpired == true
    Cust-->>User: show session expired callout (only back)
    Sum-->>User: show session expired callout (actions hidden)
  else
    Cust-->>User: show form and Next button
    Sum-->>User: show summary and action buttons
  end
Loading
sequenceDiagram
  autonumber
  participant Src as Source Scope
  participant Map as MapperService
  participant Facade as ZmsApiFacadeService
  participant Thin as ThinnedScope
  participant Office as Office

  Src-->>Map: input Scope or ThinnedScope
  Map->>Map: extractReservationDuration(scope)
  Map-->>Facade: reservationDuration value
  Facade->>Thin: construct(..., reservationDuration)
  Facade->>Office: construct(..., reservationDuration)
  Thin-->>Facade: toArray() includes reservationDuration
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Poem

I hopped through scopes and mapped a time,
A ticking minute—reservation’s chime.
When clocks run out, the callout shows,
Actions hide where quiet grows.
Tests and translations snug in line—this rabbit keeps your bookings fine. 🐇⏱️

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix-zmskvr-615-expiry-of-blocked-time

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php (1)

81-89: Fix typo in provider payload key in AppointmentUpdateControllerTest

Our search shows the only occurrence of the misspelled key is in the test itself, and no mappings in the controller emit “displayame”. All other controller tests use the correct “displayName”. Please update the stub in zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php to match the rest of the suite:

• File: zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php
Lines: 85–89

-                    'displayame'=> '',
+                    'displayName'=> '',

This will align the test with the actual API contract and ensure it fails if the implementation ever regresses.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php (1)

53-57: Guard against undefined index for captchaToken

Accessing $responseBody['captchaToken'] before checking it exists can trigger notices. Prefer isset() to avoid noise in tests.

Apply this diff:

-        if($responseBody['captchaToken']) {
+        if (isset($responseBody['captchaToken'])) {
             $this->assertArrayHasKey('captchaToken', $responseBody);
             $this->assertIsString($responseBody['captchaToken']);
             unset($responseBody['captchaToken']);
         }
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php (1)

63-67: Use isset() when handling optional captchaToken

Same rationale as in AppointmentById test: avoid undefined index by guarding with isset().

Apply this diff:

-        if($responseBody['captchaToken']) {
+        if (isset($responseBody['captchaToken'])) {
             $this->assertArrayHasKey('captchaToken', $responseBody);
             $this->assertIsString($responseBody['captchaToken']);
             unset($responseBody['captchaToken']);
         }
🧹 Nitpick comments (24)
zmscitizenview/src/utils/en-US.json (1)

128-129: Align terminology with domain model (“reservation” vs “session”) and tighten copy

Frontend logic keys off reservationDuration (a reservation timer), while the header says “session”. Consider aligning the wording and making the body more concise.

If the German translations use “Reservierung,” mirror that for consistency. Suggested copy:

-  "apiErrorSessionTimeoutHeader": "Your session has expired.",
-  "apiErrorSessionTimeoutText": "Too much time has passed since you started booking your appointment. Please select a new date to continue with the booking.",
+  "apiErrorSessionTimeoutHeader": "Your reservation has expired.",
+  "apiErrorSessionTimeoutText": "Your reservation expired due to inactivity. Please select a new date to continue booking.",
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (1)

151-154: Add a simple guard to prevent negative durations

If the schema allows only non-negative durations, enforce it defensively before validation.

         $this->whitelistedMails = $whitelistedMails;
         $this->reservationDuration = $reservationDuration;
+        if ($this->reservationDuration !== null && $this->reservationDuration < 0) {
+            throw new InvalidArgumentException('reservationDuration must be >= 0');
+        }
         $this->ensureValid();
zmscitizenview/tests/unit/CustomerInfo.spec.ts (4)

11-13: Avoid any: add precise Ref types for stronger checks

Tighten typings to catch regressions at compile time.

-  let mockAppointmentProvider: any;
-  let mockReservationStartMs: any;
+  import type { Ref } from "vue";
+  let mockAppointmentProvider: { appointment: Ref<{ scope?: { reservationDuration?: number | null } } | null> };
+  let mockReservationStartMs: Ref<number | null>;

69-73: Button stub: add type="button" to avoid implicit form submit side effects

Prevents accidental submits in environments where a form wrapper exists.

-              '<button class="muc-button" :disabled="disabled" @click="$emit(\'click\')"><slot /></button>',
+              '<button type="button" class="muc-button" :disabled="disabled" @click="$emit(\'click\')"><slot /></button>',

84-87: Reduce brittleness in button lookup

Index-based selection is fragile. Prefer data-testid or role-based selectors exposed by the component.

Example:

const findNextButton = (wrapper: any) => wrapper.find('[data-testid="next"]');
const findBackButton = (wrapper: any) => wrapper.find('[data-testid="back"]');

If component changes are undesirable, consider using more specific class selectors per button variant.


336-353: Stabilize time-dependent test and fix typos

Freezing time removes flakiness. Also fix “Reservationtime” and “sre” typos in comments.

-    it("displays the timeout message when the reservation time has expired", async () => {
-      // Reservationtime: 1 minute
-      mockAppointmentProvider.appointment.value = { scope: { reservationDuration: 1 } };
-      mockReservationStartMs.value = Date.now() - 2 * 60 * 1000;
+    it("displays the timeout message when the reservation time has expired", async () => {
+      // Reservation time: 1 minute
+      // Freeze time to avoid drift
+      vi.useFakeTimers();
+      vi.setSystemTime(new Date("2025-01-01T12:00:00Z"));
+      mockAppointmentProvider.appointment.value = { scope: { reservationDuration: 1 } };
+      mockReservationStartMs.value = Date.now() - 2 * 60 * 1000;
 
       const wrapper = createWrapper();
       await nextTick();
 
       expect(wrapper.html()).toContain("apiErrorSessionTimeoutHeader");
       expect(wrapper.html()).toContain("apiErrorSessionTimeoutText");
 
-      // form and next-button sre not displayed
+      // form and next-button are not displayed
       expect(wrapper.find(".m-form").exists()).toBe(false);
       const buttons = wrapper.findAll(".m-button-group .muc-button");
       expect(buttons.length).toBe(1); // only back-button
+      vi.useRealTimers();
     });
zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php (1)

111-113: Add non-null reservationDuration test coverage
It’s great that you mirrored the first (null) case in your second scope entry. However, to fully exercise the non-null path, consider adding at least one test where reservationDuration is a concrete integer—similar to the existing positive case in the appointment tests.

We’ve already confirmed a positive case here:

// zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php:106
"reservationDuration" => 15

Please add a similar assertion in your ScopesListControllerTest. For example:

File: zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php
Lines: 111–113

                     "whitelistedMails"   => null,
-                    "reservationDuration" => null
+                    "reservationDuration" => 30

This will ensure both the null and non-null branches of reservationDuration are covered.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php (1)

143-145: Optional: Add a non-null reservationDuration case to OfficesListControllerTest

A quick search shows only one non-null fixture for reservationDuration in your tests—and it’s in the AppointmentByIdControllerTest, not in the OfficesListControllerTest:

  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php:106:
    "reservationDuration" => 15
  • In OfficesListControllerTest, both fixtures set "reservationDuration" => null (no other positive cases found)

If you’d like to catch any future mapping regressions in the OfficesList endpoint, consider adding at least one office fixture with a non-null integer value for reservationDuration alongside the existing null cases.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php (1)

117-119: Align assertion style with other tests (optional)

Other tests often use assertEqualsCanonicalizing for array responses. Not mandatory here, but consider consistency unless key order strictness is intentional.

Apply this diff if you want consistent canonicalizing across the suite:

-        $this->assertEquals($expectedResponse, $responseBody);
+        $this->assertEqualsCanonicalizing($expectedResponse, $responseBody);
zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php (1)

68-71: Reduce duplication for captcha token stripping across tests (optional)

Multiple tests repeat the “if present, assert string and unset” pattern. Consider a small helper in the base test case to DRY this up.

I can draft a helper like stripCaptchaToken(array &$body): void in ControllerTestCase and update call sites if you want.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php (1)

115-116: Reduce brittleness: prefer canonicalizing equality for associative arrays

Switching to assertEqualsCanonicalizing here would decouple the test from key order within associative arrays, aligning with other tests in this suite that already use canonicalizing assertions.

-        $this->assertEquals($expectedResponse, $responseBody);
+        $this->assertEqualsCanonicalizing($expectedResponse, $responseBody);
zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php (1)

89-91: Propagate reservationDuration in office scope responses — consistent with schema/model

All four insertions look correct and keep reservationDuration adjacent to whitelistedMails. Given these arrays are asserted via assertEqualsCanonicalizing later, ordering is less fragile here, but keeping consistent placement aids readability and cross-file grepping.

  • Consider adding a single positive test case (non-null reservationDuration) for an office to exercise MapperService::mapOfficesWithScope and verify propagation end-to-end.
  • Optional: Extract a small helper to build the expected “scope” shape for offices to DRY up repeated structures across multiple tests.

Also applies to: 143-145, 253-255, 307-309

zmsentities/schema/citizenapi/thinnedScope.json (1)

116-123: Schema: add basic bounds to reservationDuration

Adding reservationDuration is spot-on. To prevent invalid data, recommend constraining it to non-negative (or strictly positive) minutes. If “0” is not meaningful, use minimum: 1; otherwise minimum: 0.

         "reservationDuration": {
           "type": [
             "integer",
             "null"
           ],
-          "description": "Duration of the reservation in minutes"
+          "description": "Duration of the reservation in minutes",
+          "minimum": 1
         }

If “0” should mean “no time left” or “disabled,” change minimum to 0 and document the semantics.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php (1)

108-110: Reservation duration added to reserved appointment scope — matches API evolution

Looks good and consistent with the thinnedScope schema. This test already uses assertEqualsCanonicalizing (Line 119), which avoids order-coupling — keep that pattern.

If possible, add a variant where reservationDuration is a concrete integer to cover the happy-path countdown scenario.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php (1)

87-89: Include reservationDuration across service-filtered office scopes

The additions are consistent across both test blocks and align with the schema/model changes. Nice placement directly after whitelistedMails for consistency.

Given the repeated scope structures, consider a shared builder or fixture helper to construct the expected scope array. This keeps tests focused on behavior while easing future field additions.

Also applies to: 161-163, 212-214

zmscitizenview/src/utils/errorHandler.ts (2)

122-124: Normalize API error codes with underscores/hyphens before mapping (future-proof).

If the backend ever emits session_timeout, session-timeout, or mixed-case codes, the current mapping won’t hit. Normalizing to camelCase first makes the mapping more robust without affecting existing camelCase codes.

Apply this diff to normalize the incoming error code prior to mapping:

-  const errorStateKey = `apiError${errorCode.charAt(0).toUpperCase() + errorCode.slice(1)}`;
+  // Normalize snake_case/kebab-case to camelCase before building the state key
+  const normalized = errorCode.replace(/[_-](\w)/g, (_, c) =>
+    c ? c.toUpperCase() : ""
+  );
+  const errorStateKey =
+    "apiError" + normalized.charAt(0).toUpperCase() + normalized.slice(1);

156-171: Simplify redundant error-array checks.

The three branches for data.errors can be collapsed to one fallback when firstErrorCode is falsy. This is optional, but it shortens the function and follows KISS.

A minimal refactor could be:

   const firstErrorCode = data?.errors?.[0]?.errorCode ?? "";
-  if (firstErrorCode) {
-    handleApiError(firstErrorCode, errorStates);
-  } else if (data.errors && data.errors.length > 0) {
-    errorStates.apiErrorGenericFallback.value = true;
-  } else if (data.errors && data.errors.length === 0) {
-    // Handle case where errors array exists but is empty
-    errorStates.apiErrorGenericFallback.value = true;
-  }
+  if (firstErrorCode) {
+    handleApiError(firstErrorCode, errorStates);
+  } else if (Array.isArray(data.errors)) {
+    errorStates.apiErrorGenericFallback.value = true;
+  }
zmscitizenview/src/components/Appointment/AppointmentView.vue (1)

509-547: Optional: clear previous timer when starting a new reservation.

If users reserve repeatedly within one session (e.g., rebooking loops), overwriting is fine. As a small polish, explicitly resetting reservationStartMs.value = null right before the new reservation starts can avoid any transient calculations if a consumer evaluates before the promise resolves.

 const nextReserveAppointment = () => {
   if (isReservingAppointment.value) {
     return;
   }
 
   isReservingAppointment.value = true;
   clearContextErrors(errorStateMap.value);
   captchaError.value = false;
   rebookOrCanelDialog.value = false;
+  // Reset any previous reservation timestamp until we get a new one
+  reservationStartMs.value = null;
zmscitizenview/src/utils/useReservationTimer.ts (2)

4-4: Import watch to manage interval lifecycle based on expiry.

We will use watch to stop the interval once expired. This also supports the new timeLeftString addition.

-import { computed, inject, onBeforeUnmount, onMounted, ref } from "vue";
+import { computed, inject, onBeforeUnmount, onMounted, ref, watch } from "vue";

7-10: Defensive injects (optional): fail fast with clearer error if not provided.

The non-null assertions will crash on destructuring if a provider is missing. Consider throwing an explicit error to aid debugging in non-embedded usage (e.g., isolated component tests).

-  const { appointment } = inject<SelectedAppointmentProvider>("appointment")!;
-  const reservationStartMs = inject<Ref<number | null>>("reservationStartMs")!;
+  const apptProvider = inject<SelectedAppointmentProvider>("appointment");
+  const reservationStartMs = inject<Ref<number | null>>("reservationStartMs");
+  if (!apptProvider || !reservationStartMs) {
+    throw new Error("useReservationTimer requires 'appointment' and 'reservationStartMs' providers.");
+  }
+  const { appointment } = apptProvider;
zmscitizenview/src/components/Appointment/AppointmentSummary.vue (1)

279-292: Hook usage is aligned; consider surfacing the countdown in the UI (optional).

You already destructure timeLeftString from the hook. If desired, surface the remaining time in a subtle hint before expiration (e.g., as a small text near the booking button). If not needed, remove the unused binding to keep the file tidy.

Minimal removal if you decide not to show the countdown:

-const { isExpired, timeLeftString } = useReservationTimer();
+const { isExpired } = useReservationTimer();
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (1)

95-127: Consider also exposing reservationDuration on Office for parity with other mappers

MapperService::mapOfficesWithScope already sets reservationDuration on Office. For consistency of API projections across entry points, add it here too so consumers don’t have to special-case different endpoints.

Apply this minimal diff inside the Office constructor:

                 geo: $provider->data['geo'] ?? null,
+                reservationDuration: $rd,
                 scope: $matchingScope ? new ThinnedScope(
zmscitizenview/src/components/Appointment/CustomerInfo.vue (1)

121-121: Remove commented-out timer scaffold and unused imports

Per Clean Code guidelines, avoid commented-out code and unused symbols. The logic now lives in useReservationTimer; keeping the scaffold increases noise.

Apply this diff to drop unused imports and the commented block:

-import { computed, inject, onBeforeUnmount, onMounted, ref, watch } from "vue";
+import { computed, inject, ref } from "vue";
@@
-// const { appointment } = inject<SelectedAppointmentProvider>("appointment")!;
-// const reservationStartMs = inject<Ref<number | null>>("reservationStartMs")!
-//
-// /** Hilfsfunktion: beliebige Zeitrepräsentation -> ms seit Epoche */
-// const toMs = (ts: unknown): number | null => {
-//   if (ts == null) return null;
-//   if (typeof ts === "number") return ts < 1e12 ? ts * 1000 : ts; // Sekunden -> ms
-//   if (typeof ts === "string") {
-//     if (/^\d+$/.test(ts)) {
-//       const n = Number(ts);
-//       return Number.isFinite(n) ? (n < 1e12 ? n * 1000 : n) : null;
-//     }
-//     const d = new Date(ts).getTime(); // ISO-String
-//     return Number.isFinite(d) ? d : null;
-//   }
-//   return null;
-// };
-//
-// // Minuten aus appointment.scope.reservationDuration (ohne Defaults/Minima)
-// const reservationDurationMinutes = computed<number | undefined>(() => {
-//   const raw: unknown = (appointment.value as any)?.scope?.reservationDuration;
-//   const n = typeof raw === "string" ? Number.parseInt(raw, 10) : (raw as number | undefined);
-//   return Number.isFinite(n as number) ? (n as number) : undefined;
-// });
-//
-// // Deadline = Start + rd * 60_000
-// const deadlineMs = computed<number | null>(() => {
-//   if (reservationStartMs.value == null || reservationDurationMinutes.value == null) return null;
-//   return reservationStartMs.value + reservationDurationMinutes.value * 60_000;
-// });
-//
-// // Sekündlicher Ticker
-// const nowMs = ref(Date.now());
-// let timer: number | undefined;
-//
-// onMounted(() => { timer = window.setInterval(() => nowMs.value = Date.now(), 1000); });
-// onBeforeUnmount(() => { if (timer) window.clearInterval(timer); });
-//
-// // Restzeit & Darstellung
-// const remainingMs = computed<number | null>(() =>
-//   deadlineMs.value == null ? null : Math.max(0, deadlineMs.value - nowMs.value)
-// );
-//
-// const timeLeftString = computed<string>(() => {
-//   if (remainingMs.value == null) return "";
-//   const total = Math.floor(remainingMs.value / 1000);
-//   const m = Math.floor(total / 60);
-//   const s = total % 60;
-//   return `${m}:${s.toString().padStart(2, "0")}`;
-// });
-//
-// const isExpired = computed<boolean>(() => remainingMs.value !== null && remainingMs.value <= 0);

Also applies to: 152-205

zmscitizenview/tests/unit/AppointmentSummary.spec.ts (1)

108-125: Stabilize date rendering by fixing the time zone for the date formatter

The time formatter already pins Europe/Berlin, but the date formatter relies on the environment’s default TZ. Pin it to avoid flakes around midnight or CI TZ differences.

Apply this diff:

-      const formatterDate = new Intl.DateTimeFormat("de-DE", {
+      const formatterDate = new Intl.DateTimeFormat("de-DE", {
+        timeZone: "Europe/Berlin",
         weekday: "long",
         year: "numeric",
         month: "numeric",
         day: "numeric",
       });
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 12335fe and 9b34109.

📒 Files selected for processing (24)
  • zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (4 hunks)
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (4 hunks)
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (11 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php (1 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php (1 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php (1 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php (1 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php (1 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php (1 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php (3 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php (2 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php (4 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopeByIdControllerTest.php (1 hunks)
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php (2 hunks)
  • zmscitizenview/src/components/Appointment/AppointmentSummary.vue (7 hunks)
  • zmscitizenview/src/components/Appointment/AppointmentView.vue (3 hunks)
  • zmscitizenview/src/components/Appointment/CustomerInfo.vue (4 hunks)
  • zmscitizenview/src/utils/de-DE.json (1 hunks)
  • zmscitizenview/src/utils/en-US.json (1 hunks)
  • zmscitizenview/src/utils/errorHandler.ts (1 hunks)
  • zmscitizenview/src/utils/useReservationTimer.ts (1 hunks)
  • zmscitizenview/tests/unit/AppointmentSummary.spec.ts (7 hunks)
  • zmscitizenview/tests/unit/CustomerInfo.spec.ts (17 hunks)
  • zmsentities/schema/citizenapi/thinnedScope.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*

⚙️ CodeRabbit configuration file

**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:

Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.

General rules

  1. Follow standard conventions.
  2. Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
  3. Boy scout rule. Leave the campground cleaner than you found it.
  4. Always find root cause. Always look for the root cause of a problem.

Design rules

  1. Keep configurable data at high levels.
  2. Prefer polymorphism to if/else or switch/case.
  3. Separate multi-threading code.
  4. Prevent over-configurability.
  5. Use dependency injection.
  6. Follow Law of Demeter. A class should know only its direct dependencies.

Understandability tips

  1. Be consistent. If you do something a certain way, do all similar things in the same way.
  2. Use explanatory variables.
  3. Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
  4. Prefer dedicated value objects to primitive type.
  5. Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
  6. Avoid negative conditionals.

Names rules

  1. Choose descriptive and unambiguous names.
  2. Make meaningful distinction.
  3. Use pronounceable names.
  4. Use searchable names.
  5. Replace magic numbers with named constants.
  6. Avoid encodings. Don't append prefixes or type information.

Functions rules

  1. Small.
  2. Do one thing.
  3. Use descriptive names.
  4. Prefer fewer arguments.
  5. Have no side effects.
  6. Don't use flag arguments. Split method into several independent methods that can b...

Files:

  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopeByIdControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php
  • zmsentities/schema/citizenapi/thinnedScope.json
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php
  • zmscitizenview/src/utils/en-US.json
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php
  • zmscitizenview/src/utils/useReservationTimer.ts
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php
  • zmscitizenview/src/utils/errorHandler.ts
  • zmscitizenview/src/components/Appointment/AppointmentSummary.vue
  • zmscitizenview/src/components/Appointment/CustomerInfo.vue
  • zmscitizenview/tests/unit/AppointmentSummary.spec.ts
  • zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php
  • zmscitizenview/tests/unit/CustomerInfo.spec.ts
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
  • zmscitizenview/src/components/Appointment/AppointmentView.vue
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php
  • zmscitizenview/src/utils/de-DE.json
**/*.php

⚙️ CodeRabbit configuration file

**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:

  1. For error handling: Use the proper Monolog logging framework with error levels
  2. For application info logs: Use the proper Monolog logging framework with info levels
  3. For debugging: Use a dedicated debug logger or remove debug statements
  4. For CLI output: Use a CLI output handler or symfony/console
  5. The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY

Example replacement:

// Instead of:
error_log("Import failed - " . $e->getMessage());

// Use:
$log->error("Import failed", ['error' => $e->getMessage()]);

Flag specific logging violations:

  1. error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
  2. Any debug output that should use proper Monolog logging
  3. Debug constants like DEBUG = true
  4. Debug logging that should be removed in production
  5. Commented debug code that should be cleaned up

Example replacements:

// Instead of:
error_log("Error occurred");
var_dump($data);
die('debug point');

// Use:
$log->error("Error occurred", ['context' => 'processing']);
$log->debug('Data dump', ['data' => $data]);
// Remove die() statements entirely

Exception handling should use proper logging:

// Instead of:
try {
    $result = $this->process();
} catch (Exception $e) {
    error_log("Processing failed: " . $e->getMessage());
}

// Use:
try {
    $result = $this->process();
} catch (Exception $e) {
    $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]);
}
```...

Files:

  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentPreconfirmControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesServicesRelationsControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentReserveControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopeByIdControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficeListByServiceControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php
  • zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php
  • zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php
**/*.{js,jsx,ts,tsx}

⚙️ CodeRabbit configuration file

**/*.{js,jsx,ts,tsx}: Flag any usage of console.log() as it should be replaced with proper logging or removed:

  1. For development: console.debug()
  2. For production: Remove console.log() statements or use structured logging
  3. For errors: Use error console.error()

Example replacement:

// Instead of:
console.log('User data:', userData);

// Use:
console.debug('Processing user data', { userData });
// or for development only:
Remove the console.log entirely

Flag specific logging violations:

  1. console.log(), console.debug(), console.warn() usage (except console.error in catch blocks)
  2. Any debug output that should use proper logging frameworks

Example replacements:

// Instead of:
console.log('User data:', userData);
console.debug('Processing...');

// Use:
// Remove console.log entirely or use proper logging
// Only console.error in catch blocks is acceptable
try {
  processData(userData);
} catch (error) {
  console.error('Processing failed:', error);
}

Flag JavaScript security and UX issues:

  1. alert(), confirm(), prompt() usage (poor UX)
  2. eval() usage (security risk)
  3. innerHTML with user input (XSS risk)
  4. Unfiltered user input in DOM manipulation

Example replacements:

// Instead of:
alert('Error occurred');
eval(userInput);

// Use:
// Use proper error handling and UI components
this.showErrorNotification('Error occurred');
// Avoid eval() entirely

Flag TODO/FIXME comments that indicate technical debt:

  1. TODO comments without clear action items
  2. FIXME comments indicating broken functionality
  3. HACK comments indicating temporary workarounds
  4. XXX comments indicating problematic code

These should be converted to proper issues or addressed:

// Instead of:
// TODO: fix this later
// FIXME: this is broken

// Use:
// Create proper issue and reference it
// @see Issue #123: Refactor validation logic

Flag code complexity issues:

  1. Functions longer than 50 lines
  2. Deep nesting (...

Files:

  • zmscitizenview/src/utils/useReservationTimer.ts
  • zmscitizenview/src/utils/errorHandler.ts
  • zmscitizenview/tests/unit/AppointmentSummary.spec.ts
  • zmscitizenview/tests/unit/CustomerInfo.spec.ts
🧬 Code graph analysis (4)
zmscitizenview/src/utils/useReservationTimer.ts (1)
zmscitizenview/src/types/ProvideInjectTypes.ts (1)
  • SelectedAppointmentProvider (22-24)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (4)
zmscitizenapi/src/Zmscitizenapi/Models/Office.php (1)
  • __construct (29-58)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedProvider.php (2)
  • __construct (28-38)
  • ThinnedProvider (11-69)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedProcess.php (1)
  • __construct (52-73)
zmscitizenapi/src/Zmscitizenapi/Models/Collections/ThinnedScopeList.php (1)
  • __construct (17-32)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (2)
  • MapperService (35-489)
  • extractReservationDuration (54-59)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (3)
zmsentities/src/Zmsentities/Scope.php (1)
  • Scope (11-375)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (1)
  • ThinnedScope (12-185)
zmsentities/src/Zmsentities/Schema/Entity.php (1)
  • toProperty (264-267)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (34)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (3)

33-33: New nullable property looks correct and is backwards compatible

Adding public ?int $reservationDuration = null; is fine and matches the intended semantics.


35-56: Constructor extension verified—no action needed
All new ThinnedScope(...) invocations either use named arguments or rely on the no-arg default constructor. Appending ?int $reservationDuration = null at the end preserves backward compatibility, and there are no positional-style calls to refactor.


156-178: reservationDuration in schema confirmed
The zmsentities/schema/citizenapi/thinnedScope.json file already declares the reservationDuration property with:

  • type: integer | null
  • description: “Duration of the reservation in minutes”

No further schema changes are needed—serialization and schema remain in lockstep.

zmscitizenview/tests/unit/CustomerInfo.spec.ts (1)

48-59: Injection keys align with component usage

I’ve verified that CustomerInfo.vue uses plain string keys for injection—"customerData", "selectedTimeslot", and "loadingStates"—and the unit test’s provide block uses those exact same strings. The commented-out injections for appointment and reservationStartMs in the component don’t impact resolution (they’re currently unused), so no changes to the spec’s provide keys are needed.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopesListControllerTest.php (1)

74-76: Expected shape updated correctly with reservationDuration => null

Matches the model change; assertion uses canonicalizing equality, so order is fine.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Office/OfficesListControllerTest.php (1)

89-91: Office.scope now includes reservationDuration; looks consistent

The added field aligns with ThinnedScope serialization.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentUpdateControllerTest.php (1)

105-106: Addition of scope.reservationDuration in expected response looks correct

The new field is placed consistently after whitelistedMails and matches the schema/type (integer|null). No issues spotted.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentByIdControllerTest.php (1)

105-107: Verify the origin of the hard-coded “15” for reservationDuration

I wasn’t able to find a JSON fixture or mapping in zmscitizenapi tests that explicitly defines "reservationDuration" => 15. That value likely comes from a default in your mapping or facade layer (e.g. an AppointmentMapper or the fixture setup in zmscitizenapi/src/...). To avoid brittle tests, please:

• Locate where reservationDuration is set to 15 in the production code
– e.g. inspect src/Zmscitizenapi/Mappers/AppointmentMapper.php (or similar) for a default 15
• Ensure that fixture data (if used) and mapping defaults are versioned/stable
– if it’s a config value, consider extracting it into a constant (e.g. const DEFAULT_RESERVATION_DURATION = 15;) and reference that in both code and tests
• Update the test to reference the same constant or to load the fixture so the source of truth is unified

By confirming the source of 15 and tying your test to that constant/fixture, you’ll prevent future breaks if the default ever changes.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentConfirmControllerTest.php (1)

106-108: reservationDuration added to expected scope — aligned with schema and other tests

Placement and null expectation are consistent. Looks good.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Scope/ScopeByIdControllerTest.php (1)

73-75: Confirmed: reservationDuration property correctly defined in JSON schema

The thinnedScope.json schema at schema/citizenapi/thinnedScope.json includes a reservationDuration property with type ["integer","null"] and the expected description, matching the null expectation in the test fixture. No further changes required.

zmscitizenapi/tests/Zmscitizenapi/Controllers/Appointment/AppointmentCancelControllerTest.php (1)

109-111: reservationDuration addition in expected scope — consistent and correct

Matches the updated contract; test remains clear.

zmscitizenview/src/utils/errorHandler.ts (1)

65-65: Add session-timeout error key — looks correct and consistent.

The new apiErrorSessionTimeout key aligns with the dynamic mapping in handleApiError and with the translation keys pattern (<ErrorStateName>Header/Text). No issues.

zmscitizenview/src/utils/de-DE.json (2)

128-129: German translations for session timeout — good wording and tone.

The keys and phrasing match the error state and are consistent with the rest of the locale file.


1-224: Localization parity verified for apiErrorSessionTimeout keys
The keys apiErrorSessionTimeoutHeader and apiErrorSessionTimeoutText are present exactly once in both de-DE.json and en-US.json, with no duplicates detected.

zmscitizenview/src/components/Appointment/AppointmentView.vue (3)

365-366: Introduce reservationStartMs state — correct typing and initialization.

Providing a Ref<number|null> here is appropriate and keeps the timer opt-in until a reservation is made.


435-436: Provide reservationStartMs to descendants — enables timer hook integration.

Providing via the injection key is consistent with the existing provide/inject usage in this component.


527-535: Start timer upon successful reservation — timing is correctly captured.

Setting reservationStartMs immediately after receiving a valid processId is the right trigger. This ensures CustomerInfo/AppointmentSummary can gate UI on expiration right away.

zmscitizenview/src/components/Appointment/AppointmentSummary.vue (2)

2-9: Clear, user-friendly session-timeout callout.

Showing a dedicated callout on expiry with the new translation keys is exactly what users need here.


10-13: Correct gating of interactive UI when expired.

Wrapping the details and action groups with !isExpired prevents invalid interactions post-timeout while still allowing navigation (e.g., Back) where appropriate.

Also applies to: 210-216, 231-232, 264-264

zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.php (5)

94-125: LGTM: Correctly extracting and threading reservationDuration into ThinnedScope for offices

Computing $rd once and passing it through to the nested ThinnedScope is clean and avoids repeated property traversal.


161-182: LGTM: Scopes projection now includes reservationDuration

Good reuse of MapperService::extractReservationDuration and consistent serialization alongside whitelistedMails.


276-318: LGTM: getScopeByOfficeId returns reservationDuration consistently

The intermediate $result array keeps the mapping readable while adding reservationDuration. No concerns.


429-450: LGTM: getScopeById includes reservationDuration in ThinnedScope

Straightforward propagation; matches other endpoints.


712-733: LGTM: ThinnedProcess.scope carries reservationDuration

Correctly extracts from process->scope and threads through to the client-visible model.

zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (3)

81-82: LGTM: Precomputing reservationDuration for Office mapping

Reads once and passes through to both Office and nested ThinnedScope mapping. Once the extractor is fixed, this is robust.


113-115: LGTM: ThinnedScope in Office mapping now includes reservationDuration

Consistent with schema and other mappers.


255-257: LGTM: scopeToThinnedScope adds reservationDuration via MapperService extractor

Keeps model construction centralized and consistent. After the extractor fix, this covers both entity origins.

zmscitizenview/src/components/Appointment/CustomerInfo.vue (3)

2-9: Nice UX: Explicit timeout callout when the session expires

Clear, localized messaging via MucCallout aligns with the PR’s objective. Good separation from the main form.


11-20: Solid gating of interactive UI on expiration

Guarding the heading, form, and Next button with v-if="!isExpired" prevents unintended interactions post-timeout.

Also applies to: 96-96


123-151: Adoption of useReservationTimer is appropriate

Injecting isExpired/timeLeftString keeps the component lean and shifts time logic to a dedicated composable.

zmscitizenview/tests/unit/AppointmentSummary.spec.ts (4)

14-49: LGTM: Comprehensive test scaffolding for services, provider, appointment, and timer start

The beforeEach setup is clear and supports the new timeout scenarios without over-mocking.


73-82: LGTM: Improved muc-button and muc-callout stubs

Richer stubs (data-icon, disabled) make downstream assertions precise and resilient.


88-90: LGTM: Helper for icon-based button queries

findButtonByIcon reduces duplication and improves test intent readability.


318-336: LGTM: Session timeout test validates both UX messaging and UI gating

Good coverage: asserts header/text visibility, hides main content, and ensures only back navigation is available.

ThomasAFink
ThomasAFink previously approved these changes Aug 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (1)

54-66: Resolved: safe extraction for both Scope and ThinnedScope

The previous fatal due to calling toProperty() on ThinnedScope is correctly fixed with an instanceof split. Union types and imports are in order. Nice.

🧹 Nitpick comments (5)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (5)

63-65: Drop redundant nullsafe on $scope in the Scope branch

At this point $scope cannot be null (you returned earlier and handled ThinnedScope). Using ?-> on $scope slightly obscures intent.

Apply:

-        $v = $scope?->toProperty()?->preferences?->appointment?->reservationDuration?->get();
+        $v = $scope->toProperty()?->preferences?->appointment?->reservationDuration?->get();

87-87: Use a descriptive variable name

rd is cryptic. Use reservationDuration for readability and consistency with the constructor argument below.

-            $rd = self::extractReservationDuration($providerScope);
+            $reservationDuration = self::extractReservationDuration($providerScope);

246-263: Prefer self:: over MapperService:: for intra-class static calls

Line 263 uses the class name while Line 87 uses self::. Pick one convention; self:: is idiomatic and eases refactors.

-            reservationDuration: MapperService::extractReservationDuration($scope),
+            reservationDuration: self::extractReservationDuration($scope),

246-263: Avoid casting before null-coalescing: emailFrom becomes '' instead of null

Line 247 casts to string before ?? null, which turns null into '' and defeats the null default. If emailFrom is absent you probably want null, not an empty string.

-            emailFrom: (string) $scope->getEmailFrom() ?? null,
+            emailFrom: $scope->getEmailFrom(),

Rationale: ThinnedScope::emailFrom is ?string, so passing through the nullable getter is sufficient and clearer.


119-121: Use a descriptive variable name for reservationDuration
Rename the short $rd to $reservationDuration at the callsite to make the intent clear.

• File: zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php around line 120
• Change the constructor call to pass the renamed variable:

-                    whitelistedMails: isset($providerScope->whitelistedMails) ? ((string) $providerScope->whitelistedMails === '' ? null : (string) $providerScope->whitelistedMails) : null,
-                    reservationDuration: $rd,
+                    whitelistedMails: isset($providerScope->whitelistedMails) ? ((string) $providerScope->whitelistedMails === '' ? null : (string) $providerScope->whitelistedMails) : null,
+                    reservationDuration: $reservationDuration,
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8a82272 and f264486.

📒 Files selected for processing (2)
  • zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (4 hunks)
  • zmscitizenview/src/utils/useReservationTimer.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • zmscitizenview/src/utils/useReservationTimer.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*

⚙️ CodeRabbit configuration file

**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:

Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.

General rules

  1. Follow standard conventions.
  2. Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
  3. Boy scout rule. Leave the campground cleaner than you found it.
  4. Always find root cause. Always look for the root cause of a problem.

Design rules

  1. Keep configurable data at high levels.
  2. Prefer polymorphism to if/else or switch/case.
  3. Separate multi-threading code.
  4. Prevent over-configurability.
  5. Use dependency injection.
  6. Follow Law of Demeter. A class should know only its direct dependencies.

Understandability tips

  1. Be consistent. If you do something a certain way, do all similar things in the same way.
  2. Use explanatory variables.
  3. Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
  4. Prefer dedicated value objects to primitive type.
  5. Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
  6. Avoid negative conditionals.

Names rules

  1. Choose descriptive and unambiguous names.
  2. Make meaningful distinction.
  3. Use pronounceable names.
  4. Use searchable names.
  5. Replace magic numbers with named constants.
  6. Avoid encodings. Don't append prefixes or type information.

Functions rules

  1. Small.
  2. Do one thing.
  3. Use descriptive names.
  4. Prefer fewer arguments.
  5. Have no side effects.
  6. Don't use flag arguments. Split method into several independent methods that can b...

Files:

  • zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php
**/*.php

⚙️ CodeRabbit configuration file

**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:

  1. For error handling: Use the proper Monolog logging framework with error levels
  2. For application info logs: Use the proper Monolog logging framework with info levels
  3. For debugging: Use a dedicated debug logger or remove debug statements
  4. For CLI output: Use a CLI output handler or symfony/console
  5. The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY

Example replacement:

// Instead of:
error_log("Import failed - " . $e->getMessage());

// Use:
$log->error("Import failed", ['error' => $e->getMessage()]);

Flag specific logging violations:

  1. error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
  2. Any debug output that should use proper Monolog logging
  3. Debug constants like DEBUG = true
  4. Debug logging that should be removed in production
  5. Commented debug code that should be cleaned up

Example replacements:

// Instead of:
error_log("Error occurred");
var_dump($data);
die('debug point');

// Use:
$log->error("Error occurred", ['context' => 'processing']);
$log->debug('Data dump', ['data' => $data]);
// Remove die() statements entirely

Exception handling should use proper logging:

// Instead of:
try {
    $result = $this->process();
} catch (Exception $e) {
    error_log("Processing failed: " . $e->getMessage());
}

// Use:
try {
    $result = $this->process();
} catch (Exception $e) {
    $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]);
}
```...

Files:

  • zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php
🧬 Code graph analysis (1)
zmscitizenapi/src/Zmscitizenapi/Services/Core/MapperService.php (2)
zmsdb/src/Zmsdb/Query/Scope.php (1)
  • Scope (5-457)
zmscitizenapi/src/Zmscitizenapi/Models/ThinnedScope.php (2)
  • ThinnedScope (12-185)
  • getReservationDuration (151-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: call-php-unit-tests / zmsdb-test
  • GitHub Check: call-php-unit-tests / zmsapi-test

@ThomasAFink ThomasAFink added the feature Just a normal feature label Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Just a normal feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants