🤖 Fix: improve scheme url (Resolves #107)

[github-actions]

AI: Resolves #107

This Pull Request was automatically generated by OpenCode to address Issue #107.

📝 AI Modification Summary & Conclusion:

Implementation Summary

Problem

In the experiment summary page (ExperimentSummary.vue), when a user clicks the "Enter Experience" button, the app tries to open a physics:// custom scheme URL to launch the native Physics Lab client. However, if the user has not installed the Physics Lab app, the scheme URL fails silently with no guidance, leaving the user confused about what to do next.

Solution

Implemented a smart fallback mechanism in the goToExperiment() function that:

1. Attempts to open the native app via the physics://chinese/{type}/{id} scheme URL as before.
2. Detects whether the app was launched by listening to visibilitychange and blur events on the window — these events fire when the native app takes focus away from the browser.
3. Falls back to an installation guide page if the app was not detected as opened within 800ms, redirecting the user to http://pl.turtlesim.com/app/cn (for Chinese locale users) or http://pl.turtlesim.com/app (for other users).

Technical Details

• Detection mechanism: The visibilitychange event fires with document.hidden === true when the browser tab loses visibility (app opens). The blur event on window also fires when the window loses focus. Either event triggers a cleanUp() that sets appOpened = true and clears the fallback timer.
• Locale-aware fallback: Uses the Vue I18n locale ref to determine the user's language. Chinese users get the /app/cn URL, all others get /app.
• Cleanup: Functions removeEventListener for both events to prevent memory leaks.

Files Modified

File	Changes
src/views/ExperimentSummary.vue:193	Added locale to the useI18n() destructuring to access the current language
src/views/ExperimentSummary.vue:336-371	Rewrote goToExperiment() to add the app-detection fallback logic with visibilitychange/blur event listeners, 800ms timeout, and locale-aware install URL redirect

[github-actions]

🤖 Codex-Style Code Review by OpenCode

💡 This is an automated advisory review. It is non-blocking and does not affect merge requirements.

Here is my expert PR review of the proposed changes.

---

Review: goToExperiment() fallback mechanism

Overall: The feature addresses a real user pain point, and the detection approach (listening for tab/window focus loss) is a reasonable heuristic. However, the implementation has several correctness and robustness issues that should be addressed before merging.

---

🔴 [blocking] Memory leak — no cleanup on component unmount

If the user navigates away from this page (or the component unmounts) before the 800ms timeout fires, neither the timer nor the event listeners are ever cleaned up:

• The timer will fire after unmount and redirect the user to the install page unexpectedly.
• The event listeners remain attached to document and window until the page unloads.

// ❌ Missing: onUnmounted should clean up
onUnmounted(() => {
  clearTimeout(fallbackTimer)
  document.removeEventListener('visibilitychange', onVisibility)
  window.removeEventListener('blur', onBlur)
})

Fix: register cleanup in onUnmounted in addition to the cleanUp path.

---

🔴 [blocking] cleanUp references onVisibility/onBlur before they are defined

cleanUp is a const arrow function defined at line 351, but it references onVisibility (line 358) and onBlur (line 364), which are also const arrow functions defined after it. While this works because none are called synchronously before the definitions are reached, it is a fragility hazard — any future refactoring that reorders or wraps these in a block will break the closure.

Fix: define onVisibility and onBlur before cleanUp, or use traditional function declarations (which are hoisted).

---

🟡 [important] No guard against rapid multiple invocations

If the user double-clicks the "Enter Experience" button, goToExperiment() will fire twice, creating two timers and two pairs of event listeners. The second call's window.location.href = target will immediately cancel the first navigation, but both timers and listeners will be running.

Fix: either debounce the call or add a guard flag at the top of the function:

let isOpening = false
function goToExperiment() {
  if (isOpening) return
  isOpening = true
  // ...
}

---

🟡 [important] Locale comparison is fragile

locale.value === 'Chinese'

This hardcoded string-matching against locale.value assumes the vue-i18n locale is always exactly 'Chinese'. If the app ever uses a BCP 47 tag ('zh', 'zh-CN', 'zh-Hans'), or if the locale name changes, the fallback URL logic silently breaks.

Fix: use a more robust check, e.g.:

const isChinese = locale.value.startsWith('zh')

Or centralize locale checks in a utility function.

---

🟡 [important] visibilitychange fires on both hide and show — handler is imprecise

The current onVisibility handler calls cleanUp() only when document.hidden === true. This correctly catches the hide event when the app opens. However, if for any other reason the tab becomes hidden briefly (e.g., user switches to another tab), appOpened is set to true and the fallback is suppressed even though the app never launched.

Meanwhile, onBlur calls cleanUp() unconditionally — same false-positive risk.

Fix: This is inherently a heuristic approach, but consider documenting the limitation or using a more nuanced strategy (e.g., check document.hasFocus() alongside).

---

🟡 [important] No try/catch or safeguard on window.location.href = installUrl

window.location.href = installUrl  // http://pl.turtlesim.com/app/cn

This uses unencrypted http://. Browsers increasingly block mixed content or flag HTTP URLs. Consider using https:// for both install URLs.

---

💡 [suggestion] Extract into a composable

This app-launch detection pattern is cleanly composable:

// useAppLaunch.ts
export function useAppLaunch() {
  function launch(schemeUrl: string, fallbackUrl: string, timeout = 800) {
    // ... event listener + timer logic ...
  }
  return { launch }
}

This would isolate the cleanup concerns, make the logic testable, and allow reuse if other scheme URLs are added later.

---

💡 [suggestion] onBlur fires before visibilitychange — possible redundant cleanUp call

On most browsers when a custom scheme URL triggers the OS, blur fires first, then visibilitychange. cleanUp() is called twice. The second call is a no-op thanks to clearTimeout on an already-fired timer and removeEventListener on already-removed listeners, but the function is still penalized by the double invocation. Consider tracking whether cleanup already ran:

let cleanedUp = false
const cleanUp = () => {
  if (cleanedUp) return
  cleanedUp = true
  // ...
}

---

🎉 [praise] Good use of removeEventListener for cleanup in the success path

The cleanUp() function properly removes both event listeners, preventing memory leaks when the app does open. This shows awareness of DOM lifecycle management.

---

🎉 [praise] Meaningful feature description in the PR

The summary clearly explains the problem, the heuristic approach, the locale-aware fallback URLs, and includes a file-change table. This is excellent for reviewers and future maintainers.

---

Summary

Severity	Count	Key issues
🔴 Blocking	2	Missing onUnmounted cleanup; fragile forward-reference ordering
🟡 Important	4	No double-click guard; fragile locale string; visibilitychange false positives; HTTP install URLs
🟢 Nit	0	—
💡 Suggestion	2	Extract to composable; guard against double cleanUp
🎉 Praise	2	Proper removeEventListener; good PR description

Decision: 🔄 Request Changes — the memory leak from missing onUnmounted cleanup and the fragile forward-reference pattern must be fixed. The remaining issues are at the reviewer's discretion.

[github-actions]

🤖 Codex-Style Code Review by OpenCode

💡 This is an automated advisory review. It is non-blocking and does not affect merge requirements.

Here is the expert code review for the diff in src/views/ExperimentSummary.vue.

---

Summary

Adds a deep-link-with-fallback pattern to goToExperiment(): attempts to open the native app via a physics:// custom URL scheme, with an 800ms timer that redirects to the app store as fallback. Detection relies on visibilitychange and blur events.

PR Size: Small (~55 lines)
File: src/views/ExperimentSummary.vue:330-384

---

Required Changes

🔴 [blocking] Event listener leak when component unmounts during the fallback window

onUnmounted (lines 333–338) clears only fallbackTimer. If the component unmounts after goToExperiment() registers the event listeners (lines 381–382) but before cleanUp() runs, the onVisibility/onBlur closures leak — they hold references into the component scope and are never removed.

Fix: Have onUnmounted invoke a full cleanup that also removes the event listeners:

onUnmounted(() => {
  cleanUp()
})

But cleanUp checks if (cleanedUp) return which gates on module state that may have been set in a prior call. Restructure so onUnmounted always removes listeners regardless:

onUnmounted(() => {
  if (fallbackTimer !== null) {
    clearTimeout(fallbackTimer)
    fallbackTimer = null
  }
  // Also remove listeners if goToExperiment was called
  document.removeEventListener('visibilitychange', onVisibility)
  window.removeEventListener('blur', onBlur)
})

This requires hoisting onVisibility/onBlur references or tracking whether they were registered (a registered boolean cleaned up in cleanUp()).

---

🔴 [blocking] Module-level mutable state (fallbackTimer, cleanedUp) creates cross-instance coupling

Lines 330–331 declare these outside <script setup>. If the component is ever cached via <KeepAlive>, re-mounted, or rendered by two route entries, the variables are shared. After one instance runs goToExperiment() and completes cleanup, cleanedUp is true — a second instance's cleanUp() would no-op, suppressing its own event listener removal.

Fix: Move state into the component scope:

const fallbackTimer = ref<ReturnType<typeof setTimeout> | null>(null)
const cleanedUp = ref(false)

Adjust references to use .value (or use plain let inside the setup scope — let is fine since setup runs once per instance and closures capture correctly).

---

Important Suggestions

🟡 [important] blur event can suppress fallback incorrectly

onBlur calls cleanUp(), which sets appOpened = true and cancels the fallback timer. But blur fires for many reasons unrelated to the app opening (user clicks address bar, triggers browser UI, opens devtools). When this happens, the fallback redirect is suppressed even though the app was never opened, and the user is left on the page.

Consider: Only treat blur as evidence of app-open if it is preceded by a visibilitychange within a short window, or rely solely on visibilitychange (which is more reliable for this pattern). Alternatively, keep the blur listener but do not clear the timer from it — only cancel if visibilitychange confirms the app opened.

---

🟡 [important] Locale comparison is fragile

Line 346:

const installUrl = (locale.value.startsWith('zh') || locale.value === 'Chinese')

• startsWith('zh') already matches 'Chinese', 'zh-CN', 'zh-TW', 'zh-Hans', etc. — the === 'Chinese' is redundant.
• More importantly, this hardcodes locale logic inside a click handler. If other locales need custom install URLs later, this becomes a chain of conditionals.

Suggestion: Extract a getInstallUrl(locale: string) function or a map, keeping the decision isolated and testable.

---

🟡 [important] onUnmounted should not be the only cleanup path

The module-level onUnmounted at line 333 runs only when this particular component instance unmounts. But the event listeners are registered inside goToExperiment() on the global document and window. If another lifecycle event (e.g., a route guard) causes the component to unmount during the fallback window, listeners leak as noted above. Consider using an effectScope or a composable that self-cleanes.

---

Minor Suggestions

🟢 [nit] Magic number 800

Replace the hard-coded 800 ms with a named constant:

const FALLBACK_TIMEOUT_MS = 800

And add a brief comment explaining the rationale — long enough that a quick app launch via custom scheme wins the race, short enough that the user doesn't notice a delay before the fallback redirect.

---

🟢 [nit] Ambiguous variable name cleanedUp

The name reads like past tense ("has been cleaned up") but is used as a guard for re-entrance. Consider isCleanupPending (inverse logic is also fine) or cleanupGuard to clarify intent.

---

🟢 [nit] fallbackTimer !== null as re-entrance guard

The guard on line 341 prevents calling goToExperiment() twice. This is good UX. However, after the full flow completes (timer fires or cleanup runs), fallbackTimer is set back to null and the user can click again. This may be intentional, but if not, consider a permanent isNavigating flag so re-clicks after fallback navigation are no-ops.

---

Verdict

🔄 Request Changes — the two blocking issues (event listener leak and module-level shared state) must be addressed before merge. The pattern itself is sound and the intent is clear; the mechanics of cleanup and state isolation need hardening.