Allow vehicle walkaround in both directions by gabrieltmonkai · Pull Request #990 · monkvision/monkjs

gabrieltmonkai · 2026-03-27T11:44:12Z

Overview

Jira Ticket Reference : MN-834

Implemented vehicle walkaround support in both directions based on segments

Checklist before requesting a review

I have updated the unit tests based on the changes I made
I have updated the docs (TSDoc / README / global doc) to reflect my changes
I have updated the local app configs if needed
I have performed self-QA of my feature by testing the apps and packages and made sure that :
- No regression or new bug has occurred
- The acceptance criteria listed in the ticket are met
- Self-QA was made on both desktop and mobile

dlymonkai · 2026-04-09T09:16:31Z

+  /**
+   * Array of covered segments to display on the indicator.
+   * If provided, multiple progress bars will be rendered for each segment.
+   * If not provided, falls back to showing a single progress bar from 0 to alpha.
+   */
+  coveredSegments?: CoveredSegment[];


Question: What's the purpose of having multiple segments?

If not necessary, we probably could simplify this by: start?: number

dlymonkai · 2026-04-09T09:29:50Z

+  const smoothedAlphaRef = useRef<number | null>(null);
+  const justStartedRef = useRef<boolean>(false);
+
+  const smoothedAlpha = useMemo(() => {


I think we can get rid of useMemo for low/medium computation (handled by react 19 compiler)

dlymonkai · 2026-04-09T09:31:02Z

  const [startingAlpha, setStartingAlpha] = useState<number | null>(null);
-  const [checkpoint, setCheckpoint] = useState(45);
-  const [nextCheckpoint, setNextCheckpoint] = useState(90);
+  const coveredSegmentsRef = useRef<Set<number>>(new Set());


Why reference is needed in this component?

dlymonkai · 2026-04-23T13:10:18Z

+  coveredSegments: CoveredSegment[];
+}
+
+const DEGREE_GRANULARITY = 5;


Could you move this to the top of the file, right after the imports? By convention, constants should be declared at the beginning.

dlymonkai · 2026-04-23T13:46:27Z

   * - 180: POV is behind the back of the car
   */
-  alpha: number;
+  walkaroundPosition: number;


This prop rename could be a breaking change for our clients, so we should keep the current name.

dlymonkai · 2026-04-23T14:10:03Z

   */
  showProgressBar?: boolean;
+  /**
+   * Boolean indicating if segment tracking is active.
+   * When true, the component will track and display covered segments.
+   *
+   * @default false
+   */


I’m not sure about the isTracking prop. Could you use showProgressBar instead?

dlymonkai · 2026-04-23T14:14:59Z

+  coveragePercentage: number;
+}
+
+const DEGREE_GRANULARITY = 5;


This value is declared twice in VideoCapture and VehicleWalkaroundIndicator. Could you share this value somewhere ? Is DEGREE_GRANULARITY really necessary?

Right, I've left it in useVehicleWalkaroundIndicatorState and exported it.

It's necessary because useVehicleWalkaround displays the visual indicator of coverage and VehicleWalkaroundIndicator tracks the actual position and calculates which segments are covered.

If they use different granularity values, the visual display would be incorrect.

dlymonkai · 2026-04-23T14:17:15Z

+function normalizeAngle(angle: number): number {
+  const normalized = angle % 360;
+  return normalized < 0 ? normalized + 360 : normalized;


There is the same function in VehicleWalkaroundIndicator, could you move this function in the common package then import it here?

dlymonkai · 2026-04-23T14:32:40Z

+  const smoothedAlphaRef = useRef<number | null>(null);
+  const prevStartingAlphaRef = useRef<number | null>(null);


I’m not sure about the purpose of all these refs. Why do we need a ref for alpha? We already have the startingAlpha state.

That's a good question, I tried to get rid of them, but couldn't figure out another way to solve those 2 out of 10 cases when at the beginning of video recording, the alpha started offset or coveredSegments was 2 or 3 already.

We're using prevStartingAlphaRef to know whether or not we've had previous covered segments. It's the rare case when at resume or retaking the video after discard, we would have the alpha slightly offset, plus a couple of segments already covered.

Even though we reset the alpha, this is extra safety to do a "hard reset" if the user will fall in those 2 out of 10 cases when alpha starts offset.

For the other refs, this is just a non-rendering pattern when we want to do calculations that reflect in real-time, without triggering a render, but while the hook is heavy triggered by the nature of often the alpha updates once the phone is moving.

They are more specific to the beginning of the video tracking after resume or discard video, to prevent those 2 issues:

alpha starting offset

coveredSegments starting with a value between 2-4 by default

To be honest, I’m not a fan of using these four refs. It feels like an anti-pattern.

If the alpha is offset or segments are covered, it means something isn’t working properly in the code. This hook likely needs a full rework.

dlymonkai · 2026-04-23T16:20:45Z

@@ -32,34 +61,94 @@ export function useVehicleWalkaround({
  alpha,
 }: UseVehicleWalkaroundParams): VehicleWalkaroundHandle {
  const [startingAlpha, setStartingAlpha] = useState<number | null>(null);


What do you think about having this state updated if the alpha go through the startingalpha in the opposite side (walking back).

I'm not sure about the direction, do you point towards tracking the covered segments to the left/right based on startingAlpha?

Yes this will be updated if alpha is going clockwise.

dlymonkai · 2026-04-23T19:58:00Z

@@ -32,34 +61,94 @@ export function useVehicleWalkaround({
  alpha,
 }: UseVehicleWalkaroundParams): VehicleWalkaroundHandle {
  const [startingAlpha, setStartingAlpha] = useState<number | null>(null);


What do you think about having this state updated if the alpha go through the startingalpha in the opposite side (walking back).

…gments

…ass before starting a video record

Co-authored-by: Copilot <copilot@github.com>

dlymonkai · 2026-04-30T09:07:56Z

+ * Granularity (in degrees) used for dividing the 360-degree circle into segments for vehicle walkaround tracking.
+ * This constant determines the precision of position tracking during vehicle inspections.
+ */
+export const DEGREE_GRANULARITY = 5;


This constant is defined in common-ui-web but imported in inspection-capture-web. This creates an inverted dependency, it should not depend on UI packages.

Is this const really necessary? The original implementation had not granularity.

If really needed, it should be passed as a prop to the component

dlymonkai reviewed Apr 9, 2026

View reviewed changes

dlymonkai force-pushed the main branch from 57a7a3d to e105b5d Compare April 9, 2026 15:48

dlymonkai reviewed Apr 23, 2026

View reviewed changes

gabrieltmonkai and others added 6 commits April 30, 2026 10:33

Implemented vehicle walkaround support in both directions based on se…

5214d4a

…gments

Removed coveredSegmentsRef

00b6c85

Separated concers of coveredSegments; addressed stabilization of comp…

af2d8f8

…ass before starting a video record

Addressed PR comments

03d158e

Co-authored-by: Copilot <copilot@github.com>

Added a safe check hook for alpha calculations

8dcea2c

Co-authored-by: Copilot <copilot@github.com>

Simplified ref logic

dc310f0

gabrieltmonkai force-pushed the feat/MN-834/compass-in-both-directions branch from 5206f08 to dc310f0 Compare April 30, 2026 08:34

dlymonkai reviewed Apr 30, 2026

View reviewed changes

		const smoothedAlphaRef = useRef<number \| null>(null);
		const prevStartingAlphaRef = useRef<number \| null>(null);

Conversation

gabrieltmonkai commented Mar 27, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Checklist before requesting a review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gabrieltmonkai commented Mar 27, 2026 •

edited by atlassian Bot

Loading