Skip to content

forgot to scale#683

Merged
TheJeran merged 1 commit into
mainfrom
jp/timeRatio_fix
Jul 16, 2026
Merged

forgot to scale#683
TheJeran merged 1 commit into
mainfrom
jp/timeRatio_fix

Conversation

@TheJeran

Copy link
Copy Markdown
Collaborator

closes #682

@TheJeran
TheJeran merged commit 8d3878c into main Jul 16, 2026
6 checks passed
@TheJeran
TheJeran deleted the jp/timeRatio_fix branch July 16, 2026 19:30

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the calculation of 'timeRatio' in 'AxisLines.tsx' by wrapping it in 'useMemo' and including it in the dependency array for 'zLine'. The reviewer suggested improving the implementation by adding a safety check for division by zero when 'shape.x' is zero and replacing magic numbers with named constants to improve readability and maintainability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

const depthRatio = useMemo(()=>shape.z/shape.x*timeScale,[shape, timeScale]);
const shapeRatio = useMemo(()=>shape.y/shape.x, [shape])
const timeRatio = Math.max(shape.z/shape.x, 2);
const timeRatio = useMemo(()=>Math.max(shape.z/shape.x * 2, 2),[shape]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This line has a couple of potential improvements:

  1. Edge Case Handling: There's a potential for division by zero if shape.x is 0. This could result in timeRatio being Infinity or NaN, which might cause rendering errors. It's safer to handle this edge case explicitly.

  2. Magic Numbers: The number 2 is used for both a scaling factor and a minimum value. This can be confusing. Using named constants would make the code more readable and maintainable.

Here is a suggestion that addresses both points:

Suggested change
const timeRatio = useMemo(()=>Math.max(shape.z/shape.x * 2, 2),[shape]);
const timeRatio = useMemo(() => {
if (!shape.x) {
return 2; // Fallback to the minimum value if shape.x is 0 or falsy
}
const SCALE_FACTOR = 2;
const MIN_RATIO = 2;
return Math.max((shape.z / shape.x) * SCALE_FACTOR, MIN_RATIO);
}, [shape]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

axis are broken

1 participant