Skip to content

testRunningModes#633

Open
Kwame Yeboah (Yeboahmedia) wants to merge 4 commits into
mainfrom
testRunningModes
Open

testRunningModes#633
Kwame Yeboah (Yeboahmedia) wants to merge 4 commits into
mainfrom
testRunningModes

Conversation

@Yeboahmedia

@Yeboahmedia Kwame Yeboah (Yeboahmedia) commented May 22, 2026

Copy link
Copy Markdown
Contributor

User description

Diagnostic-only PR. Adds three buttons (IMAGE / VIDEO / LIVE_STREAM) to the smart-selfie capture screen that rebuild the FaceLandmarker with the selected runningMode and surface creation errors inline.

Notes:

  • JS bindings only declare RunningMode = 'IMAGE' | 'VIDEO'. LIVE_STREAM exists in the Java API but not the web build, so it is passed through as a probe and is expected to error at create time.
  • IMAGE uses detect(); VIDEO/LIVE_STREAM use detectForVideo().
  • Singleton cache is now keyed by runningMode; old instance is closed before rebuilding.

PR Type

Tests, Enhancement


Description

  • Add diagnostic RunningMode toggle (IMAGE/VIDEO/LIVE_STREAM) to selfie capture UI

  • Rebuild FaceLandmarker singleton keyed by selected runningMode

  • Dispatch detection via detect() or detectForVideo() based on mode

  • Surface runningMode creation errors inline on the capture screen


Diagram Walkthrough

flowchart LR
  UI["RunningMode Toggle UI"]
  Hook["useFaceCapture hook"]
  Manager["mediapipeManager"]
  FaceLandmarker["FaceLandmarker Instance"]
  UI -- "setRunningMode(mode)" --> Hook
  Hook -- "getMediapipeInstance(mode)" --> Manager
  Manager -- "close old / create new" --> FaceLandmarker
  Hook -- "detect() or detectForVideo()" --> FaceLandmarker
Loading

File Walkthrough

Relevant files
Enhancement
SmartSelfieCapture.tsx
Add RunningMode toggle UI and error display                           

packages/web-components/lib/components/selfie/src/smartselfie-capture/SmartSelfieCapture.tsx

  • Added a three-button toggle bar (IMAGE / VIDEO / LIVE_STREAM) for
    selecting MediaPipe running mode
  • Display inline error message when runningMode creation fails
  • Added CSS styles for the toggle bar, buttons, active state, and error
    text
+55/-0   
useFaceCapture.ts
Support dynamic RunningMode switching in face capture hook

packages/web-components/lib/components/selfie/src/smartselfie-capture/hooks/useFaceCapture.ts

  • Added runningMode and runningModeError signals to hook state
  • Added setRunningMode function that tears down and rebuilds the
    FaceLandmarker
  • Modified initializeFaceLandmarker to pass runningMode and check cache
    validity
  • Changed detection dispatch to use detect() for IMAGE mode and
    detectForVideo() otherwise
+36/-6   
mediapipeManager.ts
Key MediaPipe singleton by runningMode with teardown logic

packages/web-components/lib/components/selfie/src/smartselfie-capture/utils/mediapipeManager.ts

  • Exported MediapipeRunningMode type and DEFAULT_MEDIAPIPE_RUNNING_MODE
    constant
  • Extended global singleton cache with runningMode field for cache
    invalidation
  • Modified getMediapipeInstance to accept a runningMode parameter and
    close/rebuild on mode change
  • Pass runningMode (with cast for LIVE_STREAM) to
    FaceLandmarker.createFromOptions
+50/-3   


Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Diagnostic-only PR. Adds three buttons (IMAGE / VIDEO / LIVE_STREAM) to
    the smart-selfie capture screen that rebuild the FaceLandmarker with the
    selected runningMode and surface creation errors inline.
    
    Notes:
    - JS bindings only declare RunningMode = 'IMAGE' | 'VIDEO'. LIVE_STREAM
      exists in the Java API but not the web build, so it is passed through
      as a probe and is expected to error at create time.
    - IMAGE uses detect(); VIDEO/LIVE_STREAM use detectForVideo().
    - Singleton cache is now keyed by runningMode; old instance is closed
      before rebuilding.
    @github-actions

    github-actions Bot commented May 22, 2026

    Copy link
    Copy Markdown

    🔍 Semgrep Security Scan Results

    ✅ No security findings detected by p/security-audit ruleset.

    @prfectionist

    prfectionist Bot commented May 22, 2026

    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🏅 Score: 55
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 No multiple PR themes
    ⚡ Recommended focus areas for review

    Race Condition

    setRunningMode calls initializeFaceLandmarker() (async) followed immediately by setupCanvas() and startDetectionLoop() without awaiting proper sequencing. If initializeFaceLandmarker fails or the running mode error is set, the code still proceeds to setupCanvas() and startDetectionLoop() with a null faceLandmarkerRef.current, which will cause a null dereference in the detection loop.

    const setRunningMode = async (mode: MediapipeRunningMode) => {
      if (mode === runningMode.value) return;
      stopDetectionLoop();
      resetFaceDetectionState();
      runningMode.value = mode;
      faceLandmarkerRef.current = null;
      await initializeFaceLandmarker();
      setupCanvas();
      startDetectionLoop();
    };
    Null Dereference

    The detection dispatch code accesses landmarker.detect(detectionSource) or landmarker.detectForVideo(...) without a null check on faceLandmarkerRef.current. If initialization failed (e.g., LIVE_STREAM mode), the landmarker will be null and this will throw an unhandled TypeError.

    const landmarker = faceLandmarkerRef.current;
    const results =
      runningMode.value === 'IMAGE'
        ? landmarker.detect(detectionSource)
        : landmarker.detectForVideo(detectionSource, performance.now());
    Race Condition

    When the running mode changes and a mediapipeGlobal.loading promise is already in flight (from a previous mode switch), the code falls through to if (mediapipeGlobal.loading) and awaits the old promise which was building with the previous running mode. The returned instance will have the wrong running mode but the caller won't know.

    if (mediapipeGlobal.loading) {
    Unsafe Cast

    Casting runningMode as 'IMAGE' | 'VIDEO' when it could be 'LIVE_STREAM' silences the type system but passes an invalid value to the WASM runtime. While this is intentional for diagnostics, it bypasses TypeScript's safety guarantees and could mask other bugs if this code is accidentally left in production.

    runningMode: runningMode as 'IMAGE' | 'VIDEO',
    Duplicate Call

    startFallbackTimer() is called both inside the try block (after successful initialization) and again unconditionally after the try/catch block, resulting in two fallback timers being started on success.

      startFallbackTimer();
    } catch (error) {
      console.error('Failed to initialize MediaPipe:', error);
      runningModeError.value =
        error instanceof Error ? error.message : String(error);
      isInitializing.value = false;
      // MediaPipe failed — start the fallback timer so the button eventually
      // enables and the user isn't permanently stuck.
      startFallbackTimer();
    }
    startFallbackTimer();

    Comment on lines +153 to +162
    const setRunningMode = async (mode: MediapipeRunningMode) => {
    if (mode === runningMode.value) return;
    stopDetectionLoop();
    resetFaceDetectionState();
    runningMode.value = mode;
    faceLandmarkerRef.current = null;
    await initializeFaceLandmarker();
    setupCanvas();
    startDetectionLoop();
    };

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: setRunningMode is async but has no error handling. If initializeFaceLandmarker() throws (e.g. when selecting LIVE_STREAM), the subsequent setupCanvas() and startDetectionLoop() will still execute with a null faceLandmarkerRef, likely causing a runtime crash in the detection loop. [possible issue, importance: 7]

    Suggested change
    const setRunningMode = async (mode: MediapipeRunningMode) => {
    if (mode === runningMode.value) return;
    stopDetectionLoop();
    resetFaceDetectionState();
    runningMode.value = mode;
    faceLandmarkerRef.current = null;
    await initializeFaceLandmarker();
    setupCanvas();
    startDetectionLoop();
    };
    const setRunningMode = async (mode: MediapipeRunningMode) => {
    if (mode === runningMode.value) return;
    stopDetectionLoop();
    resetFaceDetectionState();
    runningMode.value = mode;
    faceLandmarkerRef.current = null;
    try {
    await initializeFaceLandmarker();
    if (!faceLandmarkerRef.current) return;
    setupCanvas();
    startDetectionLoop();
    } catch (error) {
    runningModeError.value =
    error instanceof Error ? error.message : String(error);
    }
    };

    Comment on lines +259 to +262
    const results =
    runningMode.value === 'IMAGE'
    ? landmarker.detect(detectionSource)
    : landmarker.detectForVideo(detectionSource, performance.now());

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: When runningMode is LIVE_STREAM, calling detectForVideo on a landmarker configured with LIVE_STREAM will throw because MediaPipe expects a callback-based API for that mode. There's no guard or try/catch around this dispatch, so the detection loop will crash and stop entirely without surfacing the error to the user. [possible issue, importance: 7]

    Suggested change
    const results =
    runningMode.value === 'IMAGE'
    ? landmarker.detect(detectionSource)
    : landmarker.detectForVideo(detectionSource, performance.now());
    let results;
    try {
    results =
    runningMode.value === 'IMAGE'
    ? landmarker.detect(detectionSource)
    : landmarker.detectForVideo(detectionSource, performance.now());
    } catch (detectionError) {
    runningModeError.value =
    detectionError instanceof Error ? detectionError.message : String(detectionError);
    stopDetectionLoop();
    return;
    }

    Copilot AI left a comment

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    Pull request overview

    Adds a diagnostic-only running-mode selector to the Smart Selfie capture flow to rebuild the MediaPipe FaceLandmarker as IMAGE, VIDEO, or a probe LIVE_STREAM mode, and surfaces running-mode creation errors inline. This fits into the web-components Smart Selfie capture pipeline by modifying the MediaPipe singleton lifecycle and the face-detection loop dispatch method.

    Changes:

    • Introduces a MediapipeRunningMode type + default mode and stores the selected mode in the MediaPipe singleton cache.
    • Adds a running-mode button bar + inline error surface on the SmartSelfie capture screen.
    • Updates useFaceCapture to rebuild the landmarker on mode changes and to use detect() for IMAGE vs detectForVideo() for VIDEO/LIVE_STREAM.

    Reviewed changes

    Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

    File Description
    packages/web-components/lib/components/selfie/src/smartselfie-capture/utils/mediapipeManager.ts Adds runningMode typing/default and attempts to key the singleton instance by runningMode (with teardown/rebuild).
    packages/web-components/lib/components/selfie/src/smartselfie-capture/SmartSelfieCapture.tsx Adds diagnostic UI controls (mode buttons + error text) and corresponding styles.
    packages/web-components/lib/components/selfie/src/smartselfie-capture/hooks/useFaceCapture.ts Tracks runningMode state, reinitializes the landmarker on toggle, and dispatches detection calls based on mode.
    Comments suppressed due to low confidence (1)

    packages/web-components/lib/components/selfie/src/smartselfie-capture/utils/mediapipeManager.ts:266

    • getMediapipeInstance returns an in-flight mediapipeGlobal.loading promise without checking whether it was started with the requested runningMode. If the user toggles modes while the first create is still loading, the caller can receive an instance configured for the previous mode, leaving mediapipeGlobal.runningMode / runningMode.value out of sync and potentially calling the wrong detect API for the configured graph. Consider keying loading by runningMode (or storing loadingMode and rebuilding once the prior load resolves) so each mode request is guaranteed to receive a matching instance.
      if (
        mediapipeGlobal.loaded &&
        mediapipeGlobal.instance &&
        mediapipeGlobal.runningMode === runningMode
      ) {
        return mediapipeGlobal.instance;
      }
    
      if (mediapipeGlobal.loading) {
        return mediapipeGlobal.loading;
      }
    

    💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

    Comment on lines 138 to 149
    @@ -138,6 +148,19 @@ export const useFaceCapture = ({
    startFallbackTimer();
    };
    Comment on lines +183 to +184
    <div className="running-mode-bar" role="group" aria-label="MediaPipe running mode">
    {(['IMAGE', 'VIDEO', 'LIVE_STREAM'] as const).map((mode) => (
    Comment on lines 19 to +21
    import { getMediapipeInstance } from '../utils/mediapipeManager';
    import type { MediapipeRunningMode } from '../utils/mediapipeManager';
    import { DEFAULT_MEDIAPIPE_RUNNING_MODE } from '../utils/mediapipeManager';
    {(['IMAGE', 'VIDEO', 'LIVE_STREAM'] as const).map((mode) => (
    <button
    key={mode}
    type="button"
    @github-actions

    github-actions Bot commented May 22, 2026

    Copy link
    Copy Markdown

    This branch has been deployed to s3 / cloudfront.

    ✅ Preview URL for Smart Camera Web:

    https://cdn.smileidentity.com/js/preview-testRunningModes/smart-camera-web.js
    

    ✅ Preview URL for Embed:

    https://cdn.smileidentity.com/inline/preview-testRunningModes/js/script.min.js
    

    ✅ Preview URL for Web Client (Sandbox):

    https://d272vhyct1c386.cloudfront.net
    

    ✅ Preview URL for Web Client (Production):

    https://d7xctdv6ops6v.cloudfront.net
    

    @github-actions

    github-actions Bot commented Jun 5, 2026

    Copy link
    Copy Markdown

    This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

    @github-actions github-actions Bot added the Stale label Jun 5, 2026
    @github-actions github-actions Bot removed the Stale label Jun 8, 2026
    @github-actions

    Copy link
    Copy Markdown

    This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

    @github-actions github-actions Bot added the Stale label Jun 22, 2026
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants