Skip to content

Commit 665cffd

Browse files
ivyolamitivyolamit
committed
[LEMS-3953/pr4-logarithm-graph-component] fix claude review comment
1 parent d74aaff commit 665cffd

File tree

6 files changed

+297
-14
lines changed

6 files changed

+297
-14
lines changed

‎packages/perseus/src/widgets/interactive-graphs/graphs/exponential.test.tsx‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,10 @@ describe("getExponentialKeyboardConstraint", () => {
200200
];
201201
const asymptote = 1;
202202
const snapStep: vec.Vector2 = [1, 1];
203+
const range: [vec.Vector2, vec.Vector2] = [
204+
[-10, 10],
205+
[-10, 10],
206+
];
203207

204208
it("moves point up by one snap step when valid", () => {
205209
// Arrange, Act
@@ -208,6 +212,7 @@ describe("getExponentialKeyboardConstraint", () => {
208212
asymptote,
209213
snapStep,
210214
0,
215+
range,
211216
);
212217

213218
// Assert
@@ -227,6 +232,7 @@ describe("getExponentialKeyboardConstraint", () => {
227232
asymptote,
228233
snapStep,
229234
0,
235+
range,
230236
);
231237

232238
// Assert — skips y=1 (asymptote) and lands on y=0
@@ -246,11 +252,84 @@ describe("getExponentialKeyboardConstraint", () => {
246252
asymptote,
247253
snapStep,
248254
0,
255+
range,
249256
);
250257

251258
// Assert — skips x=2 and lands on x=3
252259
expect(constraint.right).toEqual([3, 3]);
253260
});
261+
262+
it("rejects positions where the clamped coord collides with the other point's x", () => {
263+
// Arrange — points at [9,3] and [8,6], asymptote at 1.
264+
// Moving point 0 right: x=10 clamps to 9 (inset max),
265+
// which equals otherPoint[X]=... wait, otherPoint is point 1.
266+
// Actually let's use: point 0 at [8,3], point 1 at [9,6].
267+
// Moving right: x=9 shares x with point 1 (skip).
268+
// x=10 clamps to 9 (same as point 1). All further clamp to 9.
269+
const edgeCoords: [vec.Vector2, vec.Vector2] = [
270+
[8, 3],
271+
[9, 6],
272+
];
273+
274+
// Act
275+
const constraint = getExponentialKeyboardConstraint(
276+
edgeCoords,
277+
asymptote,
278+
snapStep,
279+
0,
280+
range,
281+
);
282+
283+
// Assert — no valid right move, falls back to original position
284+
expect(constraint.right).toEqual([8, 3]);
285+
});
286+
287+
it("stays put when all upward positions would cause a clamped reflection collision", () => {
288+
// Arrange — asymptote at y=8, points at [3,7] and [1,4].
289+
// Moving point 0 up: y=8 is the asymptote (skip), y=9 crosses
290+
// the asymptote so reflectedY = 2*8-4 = 12, which clamps to 9
291+
// (yMax - snapStep). clampedReflectedY(9) === clampedY(9). Skip.
292+
// y=10 clamps to 9 too. No valid upward position.
293+
const edgeCoords: [vec.Vector2, vec.Vector2] = [
294+
[3, 7],
295+
[1, 4],
296+
];
297+
298+
// Act
299+
const constraint = getExponentialKeyboardConstraint(
300+
edgeCoords,
301+
8,
302+
snapStep,
303+
0,
304+
range,
305+
);
306+
307+
// Assert — no valid up move, falls back to original position
308+
expect(constraint.up).toEqual([3, 7]);
309+
});
310+
311+
it("skips positions where the clamped asymptote check catches out-of-bounds coord", () => {
312+
// Arrange — asymptote at y=9 (one step from edge).
313+
// Point 0 at [3,8], moving up: y=9 is asymptote (skip).
314+
// y=10 clamps to 9 which also equals asymptote. Skip.
315+
// No valid upward position.
316+
const edgeCoords: [vec.Vector2, vec.Vector2] = [
317+
[3, 8],
318+
[1, 6],
319+
];
320+
321+
// Act
322+
const constraint = getExponentialKeyboardConstraint(
323+
edgeCoords,
324+
9,
325+
snapStep,
326+
0,
327+
range,
328+
);
329+
330+
// Assert — no valid up move
331+
expect(constraint.up).toEqual([3, 8]);
332+
});
254333
});
255334

256335
describe("constrainAsymptoteKeyboard", () => {

‎packages/perseus/src/widgets/interactive-graphs/graphs/exponential.tsx‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ import {
99
usePerseusI18n,
1010
type I18nContextType,
1111
} from "../../../components/i18n-context";
12-
import {X, Y} from "../math";
12+
import {X, Y, snap} from "../math";
1313
import {actions} from "../reducer/interactive-graph-action";
1414
import useGraphConfig from "../reducer/use-graph-config";
15+
import {bound} from "../utils";
1516

1617
import {MovableAsymptote} from "./components/movable-asymptote";
1718
import {MovablePoint} from "./components/movable-point";
@@ -30,7 +31,7 @@ import type {
3031
InteractiveGraphElementSuite,
3132
} from "../types";
3233
import type {Coord} from "@khanacademy/perseus-core";
33-
import type {vec} from "mafs";
34+
import type {Interval, vec} from "mafs";
3435

3536
const {getExponentialCoefficients} = kmathCoefficients;
3637

@@ -136,6 +137,7 @@ function ExponentialGraph(props: ExponentialGraphProps) {
136137
asymptote,
137138
snapStep,
138139
i,
140+
range,
139141
)}
140142
onMove={(destination) =>
141143
dispatch(actions.exponential.movePoint(i, destination))
@@ -161,6 +163,7 @@ export const getExponentialKeyboardConstraint = (
161163
asymptote: number,
162164
snapStep: vec.Vector2,
163165
pointIndex: number,
166+
range: [Interval, Interval],
164167
): {
165168
up: vec.Vector2;
166169
down: vec.Vector2;
@@ -175,14 +178,45 @@ export const getExponentialKeyboardConstraint = (
175178
snapStep,
176179
pointIndex,
177180
(coord) => {
181+
// The reducer clamps the destination via boundAndSnapToGrid
182+
// before applying its own collision checks. We must predict
183+
// the clamped position to avoid accepting coords that the
184+
// reducer will silently reject.
185+
const clamped = snap(
186+
snapStep,
187+
bound({snapStep, range, point: coord}),
188+
);
189+
const clampedX = clamped[X];
190+
const clampedY = clamped[Y];
191+
178192
// Point cannot land on the horizontal asymptote
179-
if (coord[Y] === asymptoteY) {
193+
if (coord[Y] === asymptoteY || clampedY === asymptoteY) {
180194
return false;
181195
}
182196
// Both points must have different x-values
183-
if (coord[X] === otherPoint[X]) {
197+
if (coord[X] === otherPoint[X] || clampedX === otherPoint[X]) {
184198
return false;
185199
}
200+
// When the move crosses the asymptote, the reducer will
201+
// reflect the other point. Check that the reflected Y
202+
// doesn't collide with the proposed coord's Y.
203+
const currentPoint = coords[pointIndex];
204+
const currentSide = currentPoint[Y] > asymptoteY;
205+
const proposedSide = coord[Y] > asymptoteY;
206+
if (currentSide !== proposedSide) {
207+
const reflectedY = 2 * asymptoteY - otherPoint[Y];
208+
const clampedReflectedY = snap(
209+
snapStep,
210+
bound({snapStep, range, point: [0, reflectedY]}),
211+
)[Y];
212+
if (
213+
reflectedY === coord[Y] ||
214+
clampedReflectedY === coord[Y] ||
215+
clampedReflectedY === clampedY
216+
) {
217+
return false;
218+
}
219+
}
186220
return true;
187221
},
188222
);

‎packages/perseus/src/widgets/interactive-graphs/graphs/logarithm.test.tsx‎

Lines changed: 66 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,10 @@ describe("getLogarithmKeyboardConstraint", () => {
200200
];
201201
const asymptote = -6;
202202
const snapStep: vec.Vector2 = [1, 1];
203+
const range: [vec.Vector2, vec.Vector2] = [
204+
[-10, 10],
205+
[-10, 10],
206+
];
203207

204208
it("moves point up by one snap step when valid", () => {
205209
// Arrange, Act
@@ -208,6 +212,7 @@ describe("getLogarithmKeyboardConstraint", () => {
208212
asymptote,
209213
snapStep,
210214
0,
215+
range,
211216
);
212217

213218
// Assert
@@ -227,6 +232,7 @@ describe("getLogarithmKeyboardConstraint", () => {
227232
asymptote,
228233
snapStep,
229234
0,
235+
range,
230236
);
231237

232238
// Assert — skips x=-6 (asymptote) and lands on x=-7
@@ -246,6 +252,7 @@ describe("getLogarithmKeyboardConstraint", () => {
246252
asymptote,
247253
snapStep,
248254
0,
255+
range,
249256
);
250257

251258
// Assert — skips y=-7 and lands on y=-8
@@ -265,10 +272,67 @@ describe("getLogarithmKeyboardConstraint", () => {
265272
asymptote,
266273
snapStep,
267274
0,
275+
range,
268276
);
269277

270-
// Assert — skips x=-5 (same x as point 1) and x=-6 (asymptote), lands on x=-7
271-
expect(constraint.left).toEqual([-7, -3]);
278+
// Assert — skips x=-5 (same x as point 1), x=-6 (asymptote),
279+
// and x=-7 (reflected other point would collide), lands on x=-8
280+
expect(constraint.left).toEqual([-8, -3]);
281+
});
282+
283+
it("stays put when all rightward positions would cause a clamped collision", () => {
284+
// Arrange — asymptote at x=8, points at [7,3] and [4,1].
285+
// Moving point 0 right: x=8 is the asymptote, x=9 crosses the
286+
// asymptote so reflectedX = 2*8-4 = 12 which clamps to 9
287+
// (same as clamped coord), and x>=10 all clamp to 9 as well.
288+
// No valid rightward position exists, so the point stays put.
289+
const edgeCoords: [vec.Vector2, vec.Vector2] = [
290+
[7, 3],
291+
[4, 1],
292+
];
293+
const edgeRange: [vec.Vector2, vec.Vector2] = [
294+
[-10, 10],
295+
[-10, 10],
296+
];
297+
298+
// Act
299+
const constraint = getLogarithmKeyboardConstraint(
300+
edgeCoords,
301+
8,
302+
snapStep,
303+
0,
304+
edgeRange,
305+
);
306+
307+
// Assert — no valid right move, falls back to original position
308+
expect(constraint.right).toEqual([7, 3]);
309+
});
310+
311+
it("rejects positions where the clamped coord collides with the other point", () => {
312+
// Arrange — points at [8,3] and [9,1], asymptote at -5.
313+
// Moving point 0 right: x=9 shares x with otherPoint (skip).
314+
// x=10 clamps to 9 (inset max), which also equals otherPoint[X].
315+
// All further attempts clamp to 9 too. Point stays put.
316+
const edgeCoords: [vec.Vector2, vec.Vector2] = [
317+
[8, 3],
318+
[9, 1],
319+
];
320+
const edgeRange: [vec.Vector2, vec.Vector2] = [
321+
[-10, 10],
322+
[-10, 10],
323+
];
324+
325+
// Act
326+
const constraint = getLogarithmKeyboardConstraint(
327+
edgeCoords,
328+
-5,
329+
snapStep,
330+
0,
331+
edgeRange,
332+
);
333+
334+
// Assert — no valid right move, falls back to original position
335+
expect(constraint.right).toEqual([8, 3]);
272336
});
273337
});
274338

‎packages/perseus/src/widgets/interactive-graphs/graphs/logarithm.tsx‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ import {
99
usePerseusI18n,
1010
type I18nContextType,
1111
} from "../../../components/i18n-context";
12-
import {X, Y} from "../math";
12+
import {X, Y, snap} from "../math";
1313
import {actions} from "../reducer/interactive-graph-action";
1414
import useGraphConfig from "../reducer/use-graph-config";
15+
import {bound} from "../utils";
1516

1617
import {MovableAsymptote} from "./components/movable-asymptote";
1718
import {MovablePoint} from "./components/movable-point";
@@ -30,7 +31,7 @@ import type {
3031
InteractiveGraphElementSuite,
3132
} from "../types";
3233
import type {Coord} from "@khanacademy/perseus-core";
33-
import type {vec} from "mafs";
34+
import type {Interval, vec} from "mafs";
3435

3536
const {getLogarithmCoefficients} = kmathCoefficients;
3637

@@ -147,6 +148,7 @@ function LogarithmGraph(props: LogarithmGraphProps) {
147148
asymptote,
148149
snapStep,
149150
i,
151+
range,
150152
)}
151153
onMove={(destination) =>
152154
dispatch(actions.logarithm.movePoint(i, destination))
@@ -172,6 +174,7 @@ export const getLogarithmKeyboardConstraint = (
172174
asymptote: number,
173175
snapStep: vec.Vector2,
174176
pointIndex: number,
177+
range: [Interval, Interval],
175178
): {
176179
up: vec.Vector2;
177180
down: vec.Vector2;
@@ -186,19 +189,50 @@ export const getLogarithmKeyboardConstraint = (
186189
snapStep,
187190
pointIndex,
188191
(coord) => {
192+
// The reducer clamps the destination via boundAndSnapToGrid
193+
// before applying its own collision checks. We must predict
194+
// the clamped position to avoid accepting coords that the
195+
// reducer will silently reject.
196+
const clamped = snap(
197+
snapStep,
198+
bound({snapStep, range, point: coord}),
199+
);
200+
const clampedX = clamped[X];
201+
const clampedY = clamped[Y];
202+
189203
// Point cannot land on the vertical asymptote
190-
if (coord[X] === asymptoteX) {
204+
if (coord[X] === asymptoteX || clampedX === asymptoteX) {
191205
return false;
192206
}
193207
// Both points must have different x-values
194208
// (same x makes the coefficient computation degenerate)
195-
if (coord[X] === otherPoint[X]) {
209+
if (coord[X] === otherPoint[X] || clampedX === otherPoint[X]) {
196210
return false;
197211
}
198212
// Both points must have different y-values
199-
if (coord[Y] === otherPoint[Y]) {
213+
if (coord[Y] === otherPoint[Y] || clampedY === otherPoint[Y]) {
200214
return false;
201215
}
216+
// When the move crosses the asymptote, the reducer will
217+
// reflect the other point. Check that the reflected X
218+
// doesn't collide with the proposed coord's X.
219+
const currentPoint = coords[pointIndex];
220+
const currentSide = currentPoint[X] > asymptoteX;
221+
const proposedSide = coord[X] > asymptoteX;
222+
if (currentSide !== proposedSide) {
223+
const reflectedX = 2 * asymptoteX - otherPoint[X];
224+
const clampedReflectedX = snap(
225+
snapStep,
226+
bound({snapStep, range, point: [reflectedX, 0]}),
227+
)[X];
228+
if (
229+
reflectedX === coord[X] ||
230+
clampedReflectedX === coord[X] ||
231+
clampedReflectedX === clampedX
232+
) {
233+
return false;
234+
}
235+
}
202236
return true;
203237
},
204238
);

0 commit comments

Comments
 (0)