fix(ZMSKVR-1121): citizen view hash routing and deep-link test stability#2245
fix(ZMSKVR-1121): citizen view hash routing and deep-link test stability#2245ThomasAFink wants to merge 8 commits into
Conversation
- Sync hash routes on hashchange and clear stale params in zms-appointment.ce.vue - Watch confirmAppointmentHash and appointmentHash in AppointmentView for same-tab navigation - Reset confirm success when loading appointment deep link - ATAF: use in-page location for same-document citizen view URLs (Safari timeout); drop redundant refresh - CLI: remove stray skip_gateway_trust callback param; gitignore .venv for local Python venv
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request refactors deep-link navigation and initialization across the citizen view stack. Navigation logic in Java now uses same-document checks before updating URLs. Route state initialization is centralized into a single sync function. Vue components migrate initialization from mount hooks to hash-driven watchers with concurrency guards to prevent stale results. ChangesDeep-Link Navigation & Initialization Refactoring
Sequence Diagram(s)sequenceDiagram
participant Browser as User Browser
participant View as AppointmentView
participant API as Backend API
participant Router as Client Router
Browser->>View: hashchange / appointmentHash set
View->>View: runAppointmentInitializationFromHash(hash)
View->>API: fetch services/providers
API-->>View: services/providers
View->>API: fetch appointment
API-->>View: appointment data
View->>Router: route to init/reschedule/cancel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.java (1)
1645-1645:⚠️ Potential issue | 🟡 MinorStale javadoc: navigation no longer refreshes.
The comment still says "then refresh so the app loads it", but the refresh call was removed and replaced by
navigateCitizenViewUrl(url)(which relies on the Vuehashchangelistener inzms-appointment.ce.vueinstead). Update the doc to reflect the new behavior, e.g. "hash updates are picked up by the app'shashchangelistener — no refresh needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.java` at line 1645, Update the stale Javadoc on CitizenViewPage: replace the sentence that says "then refresh so the app loads it" with a note that no refresh is required because hash updates are picked up by the app's hashchange listener; mention the method used (navigateCitizenViewUrl(url)) so readers know navigation now relies on the Vue hashchange handler instead of a manual refresh.zmscitizenview/src/components/Appointment/AppointmentView.vue (1)
1292-1345:⚠️ Potential issue | 🟡 MinorLocalStorage restoration runs even during confirm deep links.
The guard on line 1293 checks only
!props.appointmentHash. When the user lands on a#/appointment/confirm/...URL,props.confirmAppointmentHashis truthy butprops.appointmentHashis not, so this block still fires a secondfetchServicesAndProviderscall and restores staleselectedService/appointment/currentViewfrom localStorage — which then races againstnextConfirmAppointment'scurrentView.value = 5(line 1109). The lastthen()to resolve wins, producing flaky UI state on the confirmation page (and is likely contributing to the deep-link test flakiness this PR is trying to fix).🛡️ Proposed fix
onMounted(() => { - if (!props.appointmentHash) { + if (!props.appointmentHash && !props.confirmAppointmentHash) { const localStorageAppointment = localStorage.getItem(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmscitizenview/src/components/Appointment/AppointmentView.vue` around lines 1292 - 1345, The localStorage restoration in onMounted runs whenever props.appointmentHash is falsy, which also triggers during confirmation deep-links; update the guard to also check props.confirmAppointmentHash (or otherwise detect confirm deep-link state) and skip the localStorage restore/fetch when confirmAppointmentHash is truthy to prevent the extra fetchServicesAndProviders call and the race with nextConfirmAppointment's currentView assignment; locate the onMounted block and adjust the initial if condition (and any related early returns) so restoration only happens when neither appointmentHash nor confirmAppointmentHash indicate a confirm deep-link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.java`:
- Around line 1668-1682: The navigateCitizenViewUrl method currently swallows
all exceptions and logs a vague warning; change it so navigation failures are
not silently ignored: catch only to log a detailed message (include the target
URL and the caught exception) using ScenarioLogManager.getLogger().error or
warn, then rethrow the exception (or wrap it in a RuntimeException) so callers
see the failure; retain the existing logic around
isSameDocumentCitizenViewNavigation and ensureAbsoluteCitizenViewUrl but do not
suppress errors from DriverUtil.getDriver(), driver.navigate().to(...) or
((JavascriptExecutor) driver).executeScript(...).
In `@zmscitizenview/src/components/Appointment/AppointmentView.vue`:
- Around line 1259-1290: The watchers for props.appointmentHash and
props.confirmAppointmentHash start async flows (via
runAppointmentInitializationFromHash and nextConfirmAppointment) without
guarding against stale responses; add a shared "latestAppointmentHash" (or
separate latestConfirmHash) variable that you set to the current hash at the
start of each watcher callback, then in runAppointmentInitializationFromHash
(and any async helpers like fetchServicesAndProviders and fetchAppointment)
check that the stored latest hash still equals the hash being processed after
each await/then and bail early if it does not so late-resolving promises cannot
overwrite services.value, offices.value, appointment.value, or currentView.value
for a newer hash.
In `@zmscitizenview/src/zms-appointment.ce.vue`:
- Around line 126-136: The debug statements currently emit sensitive base64
payloads (window.location.hash, appointmentHash.value,
confirmAppointmentHash.value) — replace these raw logs in the
syncRouteFromLocation/extractRouteParams area with non-sensitive indicators: log
only presence booleans or masked values (e.g.,
hasAppointmentHash/hasConfirmHash) and keep descriptive text, or remove the
after-sync log entirely; ensure no code paths call console.debug with the full
hash values.
---
Outside diff comments:
In
`@zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.java`:
- Line 1645: Update the stale Javadoc on CitizenViewPage: replace the sentence
that says "then refresh so the app loads it" with a note that no refresh is
required because hash updates are picked up by the app's hashchange listener;
mention the method used (navigateCitizenViewUrl(url)) so readers know navigation
now relies on the Vue hashchange handler instead of a manual refresh.
In `@zmscitizenview/src/components/Appointment/AppointmentView.vue`:
- Around line 1292-1345: The localStorage restoration in onMounted runs whenever
props.appointmentHash is falsy, which also triggers during confirmation
deep-links; update the guard to also check props.confirmAppointmentHash (or
otherwise detect confirm deep-link state) and skip the localStorage
restore/fetch when confirmAppointmentHash is truthy to prevent the extra
fetchServicesAndProviders call and the race with nextConfirmAppointment's
currentView assignment; locate the onMounted block and adjust the initial if
condition (and any related early returns) so restoration only happens when
neither appointmentHash nor confirmAppointmentHash indicate a confirm deep-link.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6501e60-e03f-4171-9795-7e27325c8aff
📒 Files selected for processing (5)
.gitignorecli_test.pyzmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.javazmscitizenview/src/components/Appointment/AppointmentView.vuezmscitizenview/src/zms-appointment.ce.vue
💤 Files with no reviewable changes (1)
- cli_test.py
…onsole Replace syncRouteFromLocation console.debug payloads with segment count and presence flags only.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
zmscitizenview/src/zms-appointment.ce.vue (3)
127-130: Optional: gate theconsole.debugbehind a dev flag (or drop it).Now that the payload is just a segment count plus presence booleans there's no credential leak, but an unconditional
console.debugin a production component is still noise for downstream consumers embedding the custom element. Consider wrapping it inimport.meta.env.DEV(Vite) or removing it before release.♻️ Suggested tweak
- console.debug("[ZMS] syncRouteFromLocation: segments=", urlElements.length, { - hasAppointmentHash: !!appointmentHash.value, - hasConfirmAppointmentHash: !!confirmAppointmentHash.value, - }); + if (import.meta.env.DEV) { + console.debug("[ZMS] syncRouteFromLocation: segments=", urlElements.length, { + hasAppointmentHash: !!appointmentHash.value, + hasConfirmAppointmentHash: !!confirmAppointmentHash.value, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmscitizenview/src/zms-appointment.ce.vue` around lines 127 - 130, The console.debug call in syncRouteFromLocation is unconditional and should be gated or removed for production; wrap the debug call (the console.debug that logs urlElements.length and the appointmentHash/confirmAppointmentHash booleans) in a dev-only guard such as checking import.meta.env.DEV (or remove the statement entirely) so it only runs in development builds and avoids noisy logs for consumers of the custom element.
93-116: Minor:extractRouteParamshas a hidden dependency onwindow.location.The function takes
urlElementsas input but silently readswindow.location.hreffor theexclusiveLocationquery param. That mixes two input sources and makes the function harder to test in isolation. SincesyncRouteFromLocationis the sole caller, consider reading the query params there and passing them in (or folding both parses intosyncRouteFromLocation). Non-blocking — pre-existing pattern, just now more visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmscitizenview/src/zms-appointment.ce.vue` around lines 93 - 116, extractRouteParams mixes inputs by reading window.location.href internally for query params (exclusiveLocation) while taking urlElements as an argument; make it pure by moving the URLSearchParams creation and exclusiveLocation check out of extractRouteParams and into its caller (syncRouteFromLocation), then pass the extracted exclusiveLocation value (or a searchParams object) into extractRouteParams so extractRouteParams only depends on its parameters and no longer reads window.location.
118-145: Hash sync +hashchangewiring looks correct.The clear-then-reparse approach works well with the
appointmentHash/confirmAppointmentHashwatchers inAppointmentView.vue(both early-return on empty), so transitioning between deep links won't trigger spurious initialization. Listener teardown inonUnmountedis in place.One edge case to be aware of: when navigating same-tab from a confirm-success URL (
/appointment/confirm/HASH) to a neutral URL with no appointment hash,syncRouteFromLocationwill clear both refs, butconfirmAppointmentSuccesswill remaintruebecause it only resets insiderunAppointmentInitializationFromHash()(which runs only when a new appointment hash loads). If the user journey requires clearing success state on any hash change, consider resetting it duringsyncRouteFromLocationor whenever hash clears, rather than only on appointment-hash load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmscitizenview/src/zms-appointment.ce.vue` around lines 118 - 145, syncRouteFromLocation clears appointment-related refs but doesn't reset the confirmAppointmentSuccess flag, so navigating from a confirm-success hash to a neutral hash leaves confirmAppointmentSuccess true; update syncRouteFromLocation to also reset confirmAppointmentSuccess (or invoke a clear function) when confirmAppointmentHash and appointmentHash are cleared so the UI state matches the absence of a hash, referencing syncRouteFromLocation, confirmAppointmentHash, appointmentHash, confirmAppointmentSuccess and runAppointmentInitializationFromHash to ensure consistency with existing initialization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@zmscitizenview/src/zms-appointment.ce.vue`:
- Around line 127-130: The console.debug call in syncRouteFromLocation is
unconditional and should be gated or removed for production; wrap the debug call
(the console.debug that logs urlElements.length and the
appointmentHash/confirmAppointmentHash booleans) in a dev-only guard such as
checking import.meta.env.DEV (or remove the statement entirely) so it only runs
in development builds and avoids noisy logs for consumers of the custom element.
- Around line 93-116: extractRouteParams mixes inputs by reading
window.location.href internally for query params (exclusiveLocation) while
taking urlElements as an argument; make it pure by moving the URLSearchParams
creation and exclusiveLocation check out of extractRouteParams and into its
caller (syncRouteFromLocation), then pass the extracted exclusiveLocation value
(or a searchParams object) into extractRouteParams so extractRouteParams only
depends on its parameters and no longer reads window.location.
- Around line 118-145: syncRouteFromLocation clears appointment-related refs but
doesn't reset the confirmAppointmentSuccess flag, so navigating from a
confirm-success hash to a neutral hash leaves confirmAppointmentSuccess true;
update syncRouteFromLocation to also reset confirmAppointmentSuccess (or invoke
a clear function) when confirmAppointmentHash and appointmentHash are cleared so
the UI state matches the absence of a hash, referencing syncRouteFromLocation,
confirmAppointmentHash, appointmentHash, confirmAppointmentSuccess and
runAppointmentInitializationFromHash to ensure consistency with existing
initialization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23ad858f-f2f6-4250-bf05-91fda444bf46
📒 Files selected for processing (1)
zmscitizenview/src/zms-appointment.ce.vue
Track latestAppointmentHash and latestConfirmAppointmentHash; bail in fetchServicesAndProviders / fetchAppointment and confirm-appointment then callbacks when a newer hash (or cleared hash) superseded the request.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.java (1)
1645-1661:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale Javadoc — the "then refresh" wording no longer matches the implementation.
The method no longer issues
driver.navigate().refresh(); it now routes throughnavigateCitizenViewUrl(...), which deliberately avoids a full reload for same-document hash changes (that's the whole point of the Safari fix). The current Javadoc misleads future readers and contradicts the helper's own Javadoc on Line 1663.📝 Suggested doc fix
- /** Navigate to the appointment view URL extracted from the confirmation mail (link without /confirm/), then refresh so the app loads it. */ + /** + * Navigate to the appointment view URL extracted from the confirmation mail (link without /confirm/). + * Routes through {`@link` `#navigateCitizenViewUrl`(String)} so same-document hash changes update via + * {`@code` window.location.href} (Safari / eager page-load) instead of a full {`@code` navigate().to()}. + */As per coding guidelines: "Comments rules — Don't be redundant" and "Use as clarification of code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.java` around lines 1645 - 1661, The Javadoc for openAppointmentViewDeepLinkInBrowser is stale— it says "then refresh" but the implementation calls navigateCitizenViewUrl(...) (which intentionally avoids a full reload) and uses ensureAbsoluteCitizenViewUrl(...) to normalize the URL; update the method Javadoc to accurately describe that it navigates to the appointment view deep link using navigateCitizenViewUrl and does not perform a full browser refresh, and mention that it waits briefly for client-side routing to settle (the Thread.sleep) and that the URL is obtained via zms.ataf.rest.steps.CitizenApiSteps.getBookingAppointmentUrl().zmscitizenview/src/components/Appointment/AppointmentView.vue (1)
1315-1373:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
onMountedlocalStorage restore can race with the confirm-hash flow.The gate at Line 1316 only checks
!props.appointmentHash. When a user opens a#/appointment/confirm/...deep link in a browser that still has fresh (<30 min)lhm-appointment-datafrom a prior session, both flows fire in parallel:
- The
confirmAppointmentHashwatcher (Line 1292) callsnextConfirmAppointment, which setsappointment.value/currentView = 5on success.- This
onMountedblock callsfetchServicesAndProvidersand then overwritesappointment.value/currentViewwith the stored data.Whichever resolves last wins, which can leave
confirmAppointmentSuccess === truewhileappointment.valueis the unrelated stored appointment, or pull the user back out of the success view.🛡️ Suggested gate
onMounted(() => { - if (!props.appointmentHash) { + if (!props.appointmentHash && !props.confirmAppointmentHash) { const localStorageAppointment = localStorage.getItem( LOCALSTORAGE_PARAM_APPOINTMENT_DATA );As per coding guidelines: "Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class." — the restore should explicitly opt out for any deep-link entry, not rely on the watchers winning the race.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmscitizenview/src/components/Appointment/AppointmentView.vue` around lines 1315 - 1373, The onMounted restore path races with the confirm-hash flow; change the initial guard in the onMounted block so it explicitly opts out when any deep-link confirm flow is present (not just props.appointmentHash). Add an early return if props.appointmentHash or props.confirmAppointmentHash (the prop watched by the confirmAppointmentHash watcher that calls nextConfirmAppointment) is truthy, so the restore logic in onMounted will not call fetchServicesAndProviders nor overwrite appointment.value or currentView when a confirm deep-link is active.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.java`:
- Around line 1645-1661: The Javadoc for openAppointmentViewDeepLinkInBrowser is
stale— it says "then refresh" but the implementation calls
navigateCitizenViewUrl(...) (which intentionally avoids a full reload) and uses
ensureAbsoluteCitizenViewUrl(...) to normalize the URL; update the method
Javadoc to accurately describe that it navigates to the appointment view deep
link using navigateCitizenViewUrl and does not perform a full browser refresh,
and mention that it waits briefly for client-side routing to settle (the
Thread.sleep) and that the URL is obtained via
zms.ataf.rest.steps.CitizenApiSteps.getBookingAppointmentUrl().
In `@zmscitizenview/src/components/Appointment/AppointmentView.vue`:
- Around line 1315-1373: The onMounted restore path races with the confirm-hash
flow; change the initial guard in the onMounted block so it explicitly opts out
when any deep-link confirm flow is present (not just props.appointmentHash). Add
an early return if props.appointmentHash or props.confirmAppointmentHash (the
prop watched by the confirmAppointmentHash watcher that calls
nextConfirmAppointment) is truthy, so the restore logic in onMounted will not
call fetchServicesAndProviders nor overwrite appointment.value or currentView
when a confirm deep-link is active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0f40933-cc57-4225-ae2c-d1ab83555051
📒 Files selected for processing (2)
zmsautomation/src/test/java/zms/ataf/ui/pages/citizenview/CitizenViewPage.javazmscitizenview/src/components/Appointment/AppointmentView.vue
|
Actionable comments posted: 0 |
1 participant
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores