Repository navigation
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe application adds startup diagnostics, error recovery UI, deferred optional-runtime loading, fatal boot handling, and general UI integrity auditing. CI now runs startup-hardening validation, and smoke tests verify startup and controller behavior. ChangesStartup and UI hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant main.tsx
participant startupDiagnostics
participant AppErrorBoundary
participant ApplicationFailureScreen
Browser->>main.tsx: initialize application
main.tsx->>startupDiagnostics: install diagnostics
main.tsx->>AppErrorBoundary: render application
AppErrorBoundary-->>startupDiagnostics: record render failure
main.tsx->>ApplicationFailureScreen: render fatal fallback
main.tsx->>startupDiagnostics: mark application interactive
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Deploying network-map with
|
| Latest commit: |
ee473a0
|
| Status: | 🚫 Build failed. |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
occu-med-map/src/generalUiIntegrityController.ts (1)
210-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
occumedUiOverflowfrom a flag, not from message text.Line 220 detects overflow by searching the human-readable failure strings for
"overflow". The messages are display text. A future wording change ("clipped", "exceeds width") silently sets the dataset attribute to"false".occu-med-map/scripts/production-ui-smoke.mjsreadsdataset.occumedUiOverflow, so the break stays invisible until a smoke run fails.Track overflow with an explicit boolean when you add the failure.
♻️ Proposed explicit overflow tracking
const failures = new Set<string>(); + let overflowDetected = false; const documentWidth = Math.max(document.documentElement.scrollWidth, document.body?.scrollWidth || 0); - if (documentWidth > window.innerWidth + 3) failures.add(`document overflow ${documentWidth - window.innerWidth}px`); + if (documentWidth > window.innerWidth + 3) { + overflowDetected = true; + failures.add(`document overflow ${documentWidth - window.innerWidth}px`); + }- if (element.scrollWidth > element.clientWidth + 3) failures.add(`horizontal overflow: ${name}`); + if (element.scrollWidth > element.clientWidth + 3) { + overflowDetected = true; + failures.add(`horizontal overflow: ${name}`); + }- document.documentElement.dataset.occumedUiOverflow = failureList.some((failure) => failure.includes("overflow")) - ? "true" - : "false"; + document.documentElement.dataset.occumedUiOverflow = overflowDetected ? "true" : "false";🤖 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 `@occu-med-map/src/generalUiIntegrityController.ts` around lines 210 - 222, Track overflow detection with an explicit boolean in the audit flow where failures are added, and update it whenever an overflow failure is recorded. Use that boolean to set document.documentElement.dataset.occumedUiOverflow instead of searching failure message text, while preserving the existing "true"/"false" dataset values.occu-med-map/scripts/general-ui-hardening-smoke.ts (1)
62-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSource-text assertions do not verify the behaviour named in their messages.
Each assertion greps the controller source for an identifier. The failure messages state behavioural guarantees that the assertions cannot check:
- Line 62 passes even when
resizeObservernever observes a target.- Line 63 passes even when
MIN_AUDIT_INTERVAL_MSis declared and never read.- Line 64 and Line 65 pass even when the filter accepts every mutation. That is the current state, as noted in the review of
occu-med-map/src/generalUiIntegrityController.tsLine 246-250.The assertions also pin internal identifier names, so a rename breaks CI without any behaviour change.
occu-med-map/scripts/production-ui-smoke.mjsalready callswindow.__NETWORK_MAP_GENERAL_UI__?.audit?.()in a real browser. Add the throttle and geometry checks there: drive a resize, count audit invocations over a fixed window, and assert the observed interval againstMIN_AUDIT_INTERVAL_MS. Keep the grep assertions as a cheap secondary guard if you want the fast signal.Do you want me to draft the behavioural assertions for the browser smoke path?
🤖 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 `@occu-med-map/scripts/general-ui-hardening-smoke.ts` around lines 62 - 65, Replace the identifier-only assertions in the smoke test with behavioural checks in the browser flow of production-ui-smoke.mjs: trigger a geometry resize, count __NETWORK_MAP_GENERAL_UI__.audit invocations during a fixed observation window, and verify the observed spacing respects MIN_AUDIT_INTERVAL_MS. Exercise unrelated and relevant DOM mutations to confirm only relevant changes trigger audits, while retaining the existing source-text checks only as optional secondary guards.
🤖 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.
Inline comments:
In `@occu-med-map/src/generalUiIntegrityController.ts`:
- Around line 325-334: Update the focus-trap logic around the active element
checks so forward Tab also redirects focus when active is outside dialog.
Preserve the existing Shift+Tab behavior, and for non-shift Tab focus first when
active is outside the dialog or at the last focusable element.
- Around line 146-149: Update the focus restoration in the activeDialogs forEach
callback to call state.opener.focus only when document.activeElement is
document.body, null, or still contained within the removed dialog state.dialog;
otherwise preserve the user’s current focus. Keep the existing isConnected and
preventScroll checks.
- Around line 246-250: Update elementAffectsAudit to split container selectors
from overlay/dialog descendant selectors: retain AUDIT_RELEVANT_SELECTOR for
direct matches only, and use a separate selector set for closest and
querySelector checks. Ensure container ancestors such as .app-body, .sidebar,
and .map-wrap do not make every mutation relevant, while preserving the
AUDIT_RELEVANT_SELECTOR identifier or updating the associated smoke assertion.
- Around line 391-396: Replace the beforeunload registration near the install
flow with a pagehide handler that invokes cleanup only when the event is not
persisted, preserving bfcache state. Add a pageshow handler that detects
restored pages and reruns the existing install/audit path, ensuring observers
and listeners are re-established after restoration.
- Around line 273-280: Update refreshResizeTargets to track all elements
currently observed by resizeObserver, unobserve tracked targets that are no
longer connected or matched, and replace the tracked set with the current root
and RESIZE_TARGET_SELECTOR matches. Clear this tracking set in cleanup so
detached dialog elements are released.
In `@occu-med-map/src/main.tsx`:
- Around line 139-140: Move the interactivity transition and optional runtime
scheduling out of the immediate post-root.render flow and into App’s mount
effect, so they run only after React commits the mounted tree. Update the logic
around markApplicationInteractive and scheduleOptionalRuntimes while preserving
their existing behavior and ensuring the effect runs for the actual rendered
application.
In `@occu-med-map/src/startupDiagnostics.ts`:
- Around line 143-151: Update markApplicationInteractive and
markOptionalRuntimesComplete so they only call setBootPhase when the current
phase is not "failed"; preserve the existing degraded, interactive, and ready
phase selection otherwise, ensuring fatal failures remain terminal.
---
Nitpick comments:
In `@occu-med-map/scripts/general-ui-hardening-smoke.ts`:
- Around line 62-65: Replace the identifier-only assertions in the smoke test
with behavioural checks in the browser flow of production-ui-smoke.mjs: trigger
a geometry resize, count __NETWORK_MAP_GENERAL_UI__.audit invocations during a
fixed observation window, and verify the observed spacing respects
MIN_AUDIT_INTERVAL_MS. Exercise unrelated and relevant DOM mutations to confirm
only relevant changes trigger audits, while retaining the existing source-text
checks only as optional secondary guards.
In `@occu-med-map/src/generalUiIntegrityController.ts`:
- Around line 210-222: Track overflow detection with an explicit boolean in the
audit flow where failures are added, and update it whenever an overflow failure
is recorded. Use that boolean to set
document.documentElement.dataset.occumedUiOverflow instead of searching failure
message text, while preserving the existing "true"/"false" dataset values.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13b3fd2b-765a-46fb-bed0-62c3031a3e02
📒 Files selected for processing (10)
.github/workflows/validate.ymloccu-med-map/package.jsonoccu-med-map/scripts/general-ui-hardening-smoke.tsoccu-med-map/scripts/startup-hardening-smoke.tsoccu-med-map/src/AppErrorBoundary.tsxoccu-med-map/src/generalUiIntegrityController.tsoccu-med-map/src/generalUiIntegrityRuntime.tsoccu-med-map/src/main.tsxoccu-med-map/src/startup-hardening.cssoccu-med-map/src/startupDiagnostics.ts
| activeDialogs.forEach((state) => { | ||
| if (current.includes(state.dialog)) return; | ||
| if (state.opener?.isConnected) state.opener.focus({ preventScroll: true }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore focus only when focus was lost.
Line 148 moves focus to the recorded opener whenever a tracked dialog disappears. The audit runs on a throttle, so the user may already have moved focus to a different control by then. In that case this call takes focus away from the current element.
Restore focus only when the active element is document.body, is null, or is still inside the removed dialog.
♿ Proposed guard for focus restore
activeDialogs.forEach((state) => {
if (current.includes(state.dialog)) return;
- if (state.opener?.isConnected) state.opener.focus({ preventScroll: true });
+ const active = document.activeElement;
+ const focusLost = !active || active === document.body || state.dialog.contains(active);
+ if (focusLost && state.opener?.isConnected) state.opener.focus({ preventScroll: true });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| activeDialogs.forEach((state) => { | |
| if (current.includes(state.dialog)) return; | |
| if (state.opener?.isConnected) state.opener.focus({ preventScroll: true }); | |
| }); | |
| activeDialogs.forEach((state) => { | |
| if (current.includes(state.dialog)) return; | |
| const active = document.activeElement; | |
| const focusLost = !active || active === document.body || state.dialog.contains(active); | |
| if (focusLost && state.opener?.isConnected) state.opener.focus({ preventScroll: true }); | |
| }); |
🤖 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 `@occu-med-map/src/generalUiIntegrityController.ts` around lines 146 - 149,
Update the focus restoration in the activeDialogs forEach callback to call
state.opener.focus only when document.activeElement is document.body, null, or
still contained within the removed dialog state.dialog; otherwise preserve the
user’s current focus. Keep the existing isConnected and preventScroll checks.
| function elementAffectsAudit(element: Element): boolean { | ||
| return element.matches(AUDIT_RELEVANT_SELECTOR) | ||
| || Boolean(element.closest(AUDIT_RELEVANT_SELECTOR)) | ||
| || Boolean(element.querySelector(AUDIT_RELEVANT_SELECTOR)); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
closest on container selectors defeats the mutation filter.
AUDIT_RELEVANT_SELECTOR (Line 39-47) contains the layout containers .app-body, .sidebar, and .map-wrap. Line 248 accepts any element that has one of those containers as an ancestor. The whole application tree lives inside .app-body, so almost every mutation in the application passes the filter.
Two consequences follow:
- The filter does not narrow anything. Map panning, marker updates, and list re-renders all schedule an audit. The 90 ms throttle bounds the audit rate, but the audit still runs continuously during interaction.
- When a batch genuinely contains no relevant mutation, Line 249 runs
querySelector(AUDIT_RELEVANT_SELECTOR)over each mutated subtree. That is the most expensive path and it executes on every observer flush.
Split the selector sets. Use the container selectors for matches only. Use the overlay and dialog selectors for closest and the subtree scan.
⚡ Proposed split between container and descendant selectors
+const CONTAINER_SELECTOR = [
+ ".command-header",
+ ".app-body",
+ ".sidebar",
+ ".map-wrap",
+].join(", ");
+const DESCENDANT_RELEVANT_SELECTOR = [
+ DIALOG_SELECTOR,
+ OVERLAY_SELECTOR,
+ ".occumed-sidebar-workspace-tab",
+].join(", ");
const AUDIT_RELEVANT_SELECTOR = [
DIALOG_SELECTOR,
OVERLAY_SELECTOR,
".command-header",
".app-body",
".sidebar",
".map-wrap",
".occumed-sidebar-workspace-tab",
].join(", "); function elementAffectsAudit(element: Element): boolean {
- return element.matches(AUDIT_RELEVANT_SELECTOR)
- || Boolean(element.closest(AUDIT_RELEVANT_SELECTOR))
- || Boolean(element.querySelector(AUDIT_RELEVANT_SELECTOR));
+ if (element.matches(AUDIT_RELEVANT_SELECTOR)) return true;
+ if (element.closest(DESCENDANT_RELEVANT_SELECTOR)) return true;
+ return Boolean(element.querySelector(DESCENDANT_RELEVANT_SELECTOR));
}Note: occu-med-map/scripts/general-ui-hardening-smoke.ts Line 65 asserts that the source contains AUDIT_RELEVANT_SELECTOR. Keep that identifier, or update the assertion together with this change.
🤖 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 `@occu-med-map/src/generalUiIntegrityController.ts` around lines 246 - 250,
Update elementAffectsAudit to split container selectors from overlay/dialog
descendant selectors: retain AUDIT_RELEVANT_SELECTOR for direct matches only,
and use a separate selector set for closest and querySelector checks. Ensure
container ancestors such as .app-body, .sidebar, and .map-wrap do not make every
mutation relevant, while preserving the AUDIT_RELEVANT_SELECTOR identifier or
updating the associated smoke assertion.
| function refreshResizeTargets(): void { | ||
| if (!resizeObserver) return; | ||
| const root = document.getElementById("root"); | ||
| if (root) resizeObserver.observe(root); | ||
| document.querySelectorAll<HTMLElement>(RESIZE_TARGET_SELECTOR).forEach((element) => { | ||
| resizeObserver?.observe(element); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prune detached targets from the ResizeObserver.
refreshResizeTargets calls observe() on every match, but the code never calls unobserve(). The observer keeps its observation targets reachable while the observer itself stays reachable from module scope. Dialog elements matched through DIALOG_SELECTOR in RESIZE_TARGET_SELECTOR are re-created on each open, so the observation list grows for the lifetime of the page. A long session with repeated modal opens retains every detached dialog element.
Track the observed elements and unobserve the ones that left the document.
🧹 Proposed pruning of detached observation targets
+let observedTargets = new Set<Element>();
+
function refreshResizeTargets(): void {
if (!resizeObserver) return;
+ observedTargets.forEach((element) => {
+ if (element.isConnected) return;
+ resizeObserver?.unobserve(element);
+ observedTargets.delete(element);
+ });
const root = document.getElementById("root");
- if (root) resizeObserver.observe(root);
+ if (root && !observedTargets.has(root)) {
+ resizeObserver.observe(root);
+ observedTargets.add(root);
+ }
document.querySelectorAll<HTMLElement>(RESIZE_TARGET_SELECTOR).forEach((element) => {
- resizeObserver?.observe(element);
+ if (observedTargets.has(element)) return;
+ resizeObserver?.observe(element);
+ observedTargets.add(element);
});
}Clear the set in cleanup:
resizeObserver?.disconnect();
resizeObserver = null;
+ observedTargets = new Set();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function refreshResizeTargets(): void { | |
| if (!resizeObserver) return; | |
| const root = document.getElementById("root"); | |
| if (root) resizeObserver.observe(root); | |
| document.querySelectorAll<HTMLElement>(RESIZE_TARGET_SELECTOR).forEach((element) => { | |
| resizeObserver?.observe(element); | |
| }); | |
| } | |
| let observedTargets = new Set<Element>(); | |
| function refreshResizeTargets(): void { | |
| if (!resizeObserver) return; | |
| observedTargets.forEach((element) => { | |
| if (element.isConnected) return; | |
| resizeObserver?.unobserve(element); | |
| observedTargets.delete(element); | |
| }); | |
| const root = document.getElementById("root"); | |
| if (root && !observedTargets.has(root)) { | |
| resizeObserver.observe(root); | |
| observedTargets.add(root); | |
| } | |
| document.querySelectorAll<HTMLElement>(RESIZE_TARGET_SELECTOR).forEach((element) => { | |
| if (observedTargets.has(element)) return; | |
| resizeObserver?.observe(element); | |
| observedTargets.add(element); | |
| }); | |
| } |
🤖 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 `@occu-med-map/src/generalUiIntegrityController.ts` around lines 273 - 280,
Update refreshResizeTargets to track all elements currently observed by
resizeObserver, unobserve tracked targets that are no longer connected or
matched, and replace the tracked set with the current root and
RESIZE_TARGET_SELECTOR matches. Clear this tracking set in cleanup so detached
dialog elements are released.
| const first = focusable[0]; | ||
| const last = focusable[focusable.length - 1]; | ||
| const active = document.activeElement; | ||
| if (event.shiftKey && (active === first || !dialog.contains(active))) { | ||
| event.preventDefault(); | ||
| last.focus(); | ||
| } else if (!event.shiftKey && active === last) { | ||
| event.preventDefault(); | ||
| first.focus(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward Tab escapes the focus trap when focus is outside the dialog.
Line 328 handles !dialog.contains(active) only for Shift+Tab. Line 331 handles forward Tab only when active === last. When focus sits outside the dialog and the user presses Tab without Shift, neither branch runs. The browser then moves focus to the next element in document order, which is outside the modal. The trap leaks.
Focus can sit outside the dialog in normal use. syncDialogs moves focus into a new dialog only once, and the application may move focus afterwards.
Redirect focus into the dialog for both directions when the active element is outside it.
⌨️ Proposed fix for the focus trap
const first = focusable[0];
const last = focusable[focusable.length - 1];
const active = document.activeElement;
- if (event.shiftKey && (active === first || !dialog.contains(active))) {
+ const outside = !dialog.contains(active);
+ if (event.shiftKey && (active === first || outside)) {
event.preventDefault();
last.focus();
- } else if (!event.shiftKey && active === last) {
+ } else if (!event.shiftKey && (active === last || outside)) {
event.preventDefault();
first.focus();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const first = focusable[0]; | |
| const last = focusable[focusable.length - 1]; | |
| const active = document.activeElement; | |
| if (event.shiftKey && (active === first || !dialog.contains(active))) { | |
| event.preventDefault(); | |
| last.focus(); | |
| } else if (!event.shiftKey && active === last) { | |
| event.preventDefault(); | |
| first.focus(); | |
| } | |
| const first = focusable[0]; | |
| const last = focusable[focusable.length - 1]; | |
| const active = document.activeElement; | |
| const outside = !dialog.contains(active); | |
| if (event.shiftKey && (active === first || outside)) { | |
| event.preventDefault(); | |
| last.focus(); | |
| } else if (!event.shiftKey && (active === last || outside)) { | |
| event.preventDefault(); | |
| first.focus(); | |
| } |
🤖 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 `@occu-med-map/src/generalUiIntegrityController.ts` around lines 325 - 334,
Update the focus-trap logic around the active element checks so forward Tab also
redirects focus when active is outside dialog. Preserve the existing Shift+Tab
behavior, and for non-shift Tab focus first when active is outside the dialog or
at the last focusable element.
| if (document.readyState === "loading") { | ||
| document.addEventListener("DOMContentLoaded", install, { once: true }); | ||
| } else { | ||
| install(); | ||
| } | ||
| window.addEventListener("beforeunload", cleanup, { once: true }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does a beforeunload listener prevent back/forward cache in Chrome, Firefox, and Safari?
💡 Result:
No, in modern versions of Chrome, Firefox, and Safari, a beforeunload event listener does not prevent a page from being stored in the back/forward cache (bfcache) [1][2]. Historically, some browsers treated beforeunload listeners as a signal to disqualify a page from the bfcache [3]. However, this is no longer the case. Official documentation and browser implementation updates confirm that modern browsers support bfcache even when a beforeunload listener is present [1][4][2]. This is distinct from the unload event, which remains a primary blocker for bfcache compatibility on many platforms and is actively discouraged in favor of more reliable alternatives like pagehide or visibilitychange [1][5][2][6]. While beforeunload is technically compatible with bfcache, it remains an unreliable event because it is not guaranteed to fire (e.g., if a browser process is killed or a tab is closed unexpectedly) [5]. Consequently, developers are still advised to use beforeunload sparingly, typically by adding the listener conditionally only when the user has unsaved changes and removing it once those changes are saved [7][1].
Citations:
- 1: https://web.dev/articles/bfcache
- 2: https://groups.google.com/a/chromium.org/g/bfcache-dev/c/zTIMx7u4uxo/m/-M4IS6LDBgAJ
- 3: https://stackoverflow.com/questions/40727989/is-onbeforeunload-cached-on-safari-macos
- 4: https://bugzilla.mozilla.org/show_bug.cgi?id=1837045
- 5: https://developer.chrome.com/docs/web-platform/deprecating-unload
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Window/unload_event
- 7: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced file and relevant handler/cleanup bootstrap code.
if [ -f occu-med-map/src/generalUiIntegrityController.ts ]; then
echo "FILE FOUND: occu-med-map/src/generalUiIntegrityController.ts"
wc -l occu-med-map/src/generalUiIntegrityController.ts
sed -n '300,410p' occu-med-map/src/generalUiIntegrityController.ts | nl -ba -v300
echo "--- broader relevant search ---"
rg -n "event\.persisted|beforeunload|pagehide|pageshow|mutationObserver|window\.__NETWORK_MAP_GENERAL_UI__|function cleanup|export \{|const installation|scheduleAudit|install\(" occu-med-map/src/generalUiIntegrityController.ts
else
echo "Referenced file not found"
fd -i 'generalUiIntegrityController\.ts|production-ui-smoke' .
fiRepository: Occumed79/Network-Map
Length of output: 313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- lines 300-398 ---"
sed -n '300,398p' occu-med-map/src/generalUiIntegrityController.ts | awk '{printf "%d\t%s\n", 300+NR-1, $0}'
echo "--- relevant identifiers ---"
grep -n "event\.persisted|beforeunload|pagehide|pageshow|mutationObserver|window\.__NETWORK_MAP_GENERAL_UI__|cleanup\(|function cleanup|install\(|scheduleAudit" occu-med-map/src/generalUiIntegrityController.tsRepository: Occumed79/Network-Map
Length of output: 3649
🌐 Web query:
MDN beforeunload event back/forward cache bfcache
💡 Result:
The relationship between the beforeunload event and the back/forward cache (bfcache) has evolved as browsers have shifted toward prioritizing performance [1][2][3]. In modern browsers, the presence of a beforeunload event listener does not automatically make a page ineligible for the bfcache, although its behavior remains complex and can still be a factor in reliability [1]. Historical Context and Current Behavior: Historically, beforeunload listeners were often treated similarly to unload listeners, which strictly prevented pages from entering the bfcache [4][5]. Modern specifications and browser implementations have moved away from this rigid requirement [6]. While some browsers may still treat pages with these listeners with caution, they are no longer universally excluded from the bfcache [1]. Key Considerations for Developers: 1. Conditional Usage: It is highly recommended to add beforeunload listeners only when necessary (e.g., when the user has unsaved changes) and to remove them immediately once those changes are saved or discarded [4][1]. This practice minimizes the risk of the page being excluded from the bfcache and ensures the event only fires when intended [4][1]. 2. Reliability: The beforeunload event is inherently unreliable; it may not fire if a background tab is discarded by the browser to save memory, or during certain types of navigation [1][7]. 3. Alternatives: For tasks like saving state or analytics, the pagehide and visibilitychange events are preferred because they are bfcache-compatible [1][8]. The pagehide event, in particular, fires when a page is put into the bfcache, whereas unload does not [1][8]. 4. Monitoring: You can identify if your pages are being blocked from using the bfcache by utilizing the PerformanceNavigationTiming.notRestoredReasons API, which reports specific reasons for bfcache blocking [2][3]. Additionally, tools like Chrome DevTools include audits to help developers identify issues preventing bfcache usage [7]. In summary, while the presence of a beforeunload listener is not a hard "blocker" for bfcache in modern browsers in the same way it once was, it is still best practice to use it sparingly and conditionally to maintain optimal performance and reliability [4][1].
Citations:
- 1: https://web.dev/articles/bfcache
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Performance_API/Monitoring_bfcache_blocking_reasons
- 3: https://developer.mozilla.org/en-US/docs/Glossary/bfcache
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event
- 5: back-forward cache: specify interaction with unload & beforeunload whatwg/html#5748
- 6: Make beforeunload not affect 'salvageable' & fire unload event only if document is no longer salvageable whatwg/html#5889
- 7: https://developer.chrome.com/docs/web-platform/deprecating-unload
- 8: https://developer.mozilla.org/en-US/docs/Web/API/Window/unload_event
🌐 Web query:
MDN pagehide page lifecycle API document hidden beforeunload
💡 Result:
The pagehide, beforeunload, and visibilitychange events each play distinct roles in the browser page lifecycle, and modern web development standards prioritize specific events for reliability and performance [1][2][3]. visibilitychange (Recommended for Session End) The visibilitychange event is the best way to determine when a page is hidden and effectively serves as the most reliable signal for the end of a user's session [1][2][3]. When the document's visibilityState becomes "hidden," it indicates the user has switched tabs, minimized the window, or navigated away [4][3]. Developers should use this event—typically in conjunction with the Navigator.sendBeacon API—to save application state or send analytics data [3]. pagehide The pagehide event is fired when the browser hides the current page during navigation to another page [1]. Unlike unload and beforeunload, pagehide is compatible with the back/forward cache (bfcache) [1][2]. If you must detect when a page is being unloaded or placed into the bfcache, pagehide is the preferred alternative to the unreliable unload event [1][5][2]. The event's persisted property indicates whether the page is entering the bfcache (true) or being terminated (false) [5]. beforeunload The beforeunload event is used exclusively to warn users about potential data loss before they leave a page (e.g., if there is unsaved form data) [6]. It allows the page to trigger a browser-controlled confirmation dialog [6][7]. It should not be used for general session termination or analytics, as it is not bfcache-friendly—its presence can prevent the browser from caching the page entirely, which negatively impacts performance [6]. It is best practice to add this listener only when unsaved changes exist and remove it when they are saved [6]. Summary of Recommendations - For saving state or analytics: Use visibilitychange [2][3]. - For detecting navigation away: Use pagehide instead of unload [1][2]. - For preventing data loss: Use beforeunload only when necessary [6].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Window/pagehide_event
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Window/unload_event
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Document/visibilitychange%5Fevent
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Page_Visibility_API
- 5: https://developer.chrome.com/docs/web-platform/page-lifecycle-api
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event
- 7: https://developer.mozilla.org/en-US/docs/Web/API/BeforeUnloadEvent
Use pagehide/pageshow instead of beforeunload for bfcache-aware cleanup.
Modern back/forward-cache behavior does not guarantee that beforeunload blocks caching, but beforeunload can still fire without running before a process is killed or the tab is discarded. If it runs, cleanup() disconnects observers and removes listeners while leaving window.__NETWORK_MAP_GENERAL_UI__ defined and no re-install path exists. Replace beforeunload with pagehide (skip cleanup when event.persisted is true), and reinstall/audit on restored pageshow.
🤖 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 `@occu-med-map/src/generalUiIntegrityController.ts` around lines 391 - 396,
Replace the beforeunload registration near the install flow with a pagehide
handler that invokes cleanup only when the event is not persisted, preserving
bfcache state. Add a pageshow handler that detects restored pages and reruns the
existing install/audit path, ensuring observers and listeners are re-established
after restoration.
| window.requestAnimationFrame(() => markApplicationInteractive(rootElement)); | ||
| scheduleOptionalRuntimes(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For React 19 createRoot, does root.render() guarantee that React has committed the rendered DOM tree before root.render() returns?
💡 Result:
No, root.render does not guarantee that React has committed the rendered DOM tree before the method returns [1][2]. While the rendering process is synchronous once it begins, the root.render call itself is scheduled in a microtask [1][2]. Consequently, code executing immediately after root.render may run before the DOM updates have been committed and before any associated effects (such as useEffect or useLayoutEffect) have fired [1][3]. If you require the DOM to be updated synchronously—for example, if you need to perform operations that depend on the rendered DOM immediately after the call—you should wrap the root.render call in flushSync [1][3]: import { flushSync } from 'react-dom'; import { createRoot } from 'react-dom/client'; const root = createRoot(document.getElementById('root')); flushSync( => { root.render(); }); // The DOM is now updated and effects have been processed.
Citations:
- 1: https://react.dev/reference/react-dom/client/createRoot
- 2: Add note about root.render microtask scheduling reactjs/react.dev#7717
- 3: Bug: createRoot().render() is not fully synchronous unless wrapped in flushSync, unlike legacy ReactDOM.render react/react#32811
🏁 Script executed:
#!/bin/bash
set -euo pipefan # intentionally leave it to wait for no commandRepository: Occumed79/Network-Map
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching main.tsx:"
fd -a 'main\.tsx$' . || true
echo
echo "Inspect target file outline:"
ast-grep outline occu-med-map/src/main.tsx --view expanded 2>/dev/null || true
echo
echo "Relevant lines 1-220:"
sed -n '1,220p' occu-med-map/src/main.tsx
echo
echo "Search for markup recovery/ready/busy/runtime helpers:"
rg -n "markApplicationInteractive|scheduleOptionalRuntimes|aria-busy|recovery|main:root|mark\(" occu-med-map/src/main.tsx occu-med-map/src -S || trueRepository: Occumed79/Network-Map
Length of output: 9386
🏁 Script executed:
#!/bin/bash
set -u
echo "Startup diagnostics target helpers:"
sed -n '1,190p' occu-med-map/src/startupDiagnostics.ts
echo
echo "App mounting points / root render target search:"
rg -n "rootElement|getElementById\\(\"root\"\\)|main:root|`#root`|document\\.getElementById|addEventListener\\(|useEffect\\(" occu-med-map/src/App.tsx occu-med-map/src -S || trueRepository: Occumed79/Network-Map
Length of output: 27751
Signal interactivity after React commits the root.
root.render() can return before the React DOM update and mounted component effects commit. Calling requestAnimationFrame immediately leaves aria-busy, the boot phase, and optional runtime scheduling able to run before the application tree is present. Mark the app interactive and schedule optional runtimes from a mount effect in App, or use a synchronous commit signal that only fires for each actual rendered tree.
🤖 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 `@occu-med-map/src/main.tsx` around lines 139 - 140, Move the interactivity
transition and optional runtime scheduling out of the immediate post-root.render
flow and into App’s mount effect, so they run only after React commits the
mounted tree. Update the logic around markApplicationInteractive and
scheduleOptionalRuntimes while preserving their existing behavior and ensuring
the effect runs for the actual rendered application.
| export function markApplicationInteractive(rootElement: HTMLElement): void { | ||
| rootElement.setAttribute("aria-busy", "false"); | ||
| if (readyAt === null) readyAt = elapsed(); | ||
| setBootPhase(failures.length ? "degraded" : "interactive"); | ||
| } | ||
|
|
||
| export function markOptionalRuntimesComplete(): void { | ||
| setBootPhase(failures.length ? "degraded" : "ready"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep failed as a terminal boot phase.
AppErrorBoundary records fatal render failures at occu-med-map/src/AppErrorBoundary.tsx Lines 54-60. The scheduled calls to markApplicationInteractive and markOptionalRuntimesComplete then overwrite failed with degraded. The snapshot phase conflicts with the fatal health state.
Only update these phases when phase !== "failed".
Proposed fix
export function markApplicationInteractive(rootElement: HTMLElement): void {
rootElement.setAttribute("aria-busy", "false");
if (readyAt === null) readyAt = elapsed();
- setBootPhase(failures.length ? "degraded" : "interactive");
+ if (phase !== "failed") {
+ setBootPhase(failures.length ? "degraded" : "interactive");
+ }
}
export function markOptionalRuntimesComplete(): void {
- setBootPhase(failures.length ? "degraded" : "ready");
+ if (phase !== "failed") {
+ setBootPhase(failures.length ? "degraded" : "ready");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function markApplicationInteractive(rootElement: HTMLElement): void { | |
| rootElement.setAttribute("aria-busy", "false"); | |
| if (readyAt === null) readyAt = elapsed(); | |
| setBootPhase(failures.length ? "degraded" : "interactive"); | |
| } | |
| export function markOptionalRuntimesComplete(): void { | |
| setBootPhase(failures.length ? "degraded" : "ready"); | |
| } | |
| export function markApplicationInteractive(rootElement: HTMLElement): void { | |
| rootElement.setAttribute("aria-busy", "false"); | |
| if (readyAt === null) readyAt = elapsed(); | |
| if (phase !== "failed") { | |
| setBootPhase(failures.length ? "degraded" : "interactive"); | |
| } | |
| } | |
| export function markOptionalRuntimesComplete(): void { | |
| if (phase !== "failed") { | |
| setBootPhase(failures.length ? "degraded" : "ready"); | |
| } | |
| } |
🤖 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 `@occu-med-map/src/startupDiagnostics.ts` around lines 143 - 151, Update
markApplicationInteractive and markOptionalRuntimesComplete so they only call
setBootPhase when the current phase is not "failed"; preserve the existing
degraded, interactive, and ready phase selection otherwise, ensuring fatal
failures remain terminal.
Final hardening statusThe branch is ready for review at Completed
Validation performed
Validation limitationGitHub Actions did not start for the connector-authored commits, so I am not claiming the full repository workflow, production browser smoke suite, or Render runtime verification passed. Those remain merge gates. No Render settings, environment variables, Neon data/schema, provider data, or production route behavior were changed. |
|
Superseded by #206, which surgically ported the useful startup recovery/diagnostics work onto the finalized #205 runtime-ownership stack and passed the exact desktop Validate, UI Acceptance, Observability, Post Idle Browser Probe, and Hardening Acceptance gates. The obsolete mutation-controller portions of this divergent branch were intentionally not carried forward. |
1 participant
Scope
This pass hardens the application beneath the existing visual and map-specific fixes rather than stacking another cosmetic override.
Startup and failure containment
window.__NETWORK_MAP_BOOT__Runtime performance and stability
UI recovery and accessibility
Regression protection
Risk controls
Summary by CodeRabbit
New Features
Tests