Use d3.ticks() for nicer linear scale tick generation#4689
Use d3.ticks() for nicer linear scale tick generation#4689leonardthethird wants to merge 6 commits intomainfrom
Conversation
Replaces evenly-spaced ticks across the raw zoomed domain with d3.ticks,
which picks values that are multiples of {1,2,5}*10^k. Fixes ugly endpoints
like -27.7 / 52.7 on fan charts and other linear scales. forceTickCount
callers keep the evenly-spaced path since d3.ticks treats its count
argument as a hint, not a guarantee.
https://claude.ai/code/session_017gYZRiw2Sq41iLGq7KPJzL
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces linear-scaling tick generation in ChangesAxis Tick Generation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front_end/src/utils/charts/axis.ts`:
- Around line 651-659: The minor-tick density calculation incorrectly uses
majorTicks[1] as an absolute position; change it to use the major step width
instead. Compute a majorStep from majorTicks (e.g., majorStep = (majorTicks[1] -
majorTicks[0]) if available, else (rangeMax - rangeMin) / Math.max(1,
majorTicks.length - 1)), then call findOptimalTickCount(rangeMin, rangeMin +
majorStep, ...) to derive minorTicksPerMajor and keep the minorTickCount
calculation using Math.max(majorTicks.length - 1, 1) * minorTicksPerMajor + 1;
handle the case where majorTicks has fewer than 2 entries by falling back to the
computed step.
- Around line 622-644: The evenly-spaced tick branch fails for forceTickCount
=== 1 due to division by (count - 1); update the logic that builds majorTicks
and minorTicks (the block using forceTickCount, majorTickCount/minorTickCount,
zoomedDomainMin/zoomedDomainMax and range) to special-case count === 1 (or clamp
count to >= 2) so you don't compute i / 0—e.g., when forceTickCount === 1
produce a single rounded tick (use the midpoint of zoomedDomainMin and
zoomedDomainMax or a defined endpoint) and otherwise keep the existing
interpolation and rounding; also add a regression test that exercises
forceTickCount = 1 alongside existing force-count tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c44f129d-472c-4549-99a1-4e679b011b16
📒 Files selected for processing (2)
front_end/src/utils/charts/__tests__/axis.test.tsfront_end/src/utils/charts/axis.ts
| if (forceTickCount) { | ||
| // forceTickCount must be honored exactly. d3.ticks treats its count | ||
| // argument as a hint, so fall back to evenly-spaced ticks here. | ||
| const majorTickCount = forceTickCount; | ||
| majorTicks = range(0, majorTickCount).map( | ||
| (i) => | ||
| Math.round( | ||
| (zoomedDomainMin + | ||
| (i / (majorTickCount - 1)) * | ||
| (zoomedDomainMax - zoomedDomainMin)) * | ||
| 1000000 | ||
| ) / 1000000 | ||
| ); | ||
| const minorTickCount = forceTickCount; | ||
| minorTicks = range(0, minorTickCount).map( | ||
| (i) => | ||
| Math.round( | ||
| (zoomedDomainMin + | ||
| (i / (minorTickCount - 1)) * | ||
| (zoomedDomainMax - zoomedDomainMin)) * | ||
| 1000000 | ||
| ) / 1000000 | ||
| ); |
There was a problem hiding this comment.
Handle forceTickCount === 1 before dividing by count - 1.
With forceTickCount set to 1, Lines 630 and 640 evaluate i / (count - 1) as 0 / 0, so both tick arrays become [NaN]. That breaks the exact-count path on a valid boundary input. Please special-case 1 (or clamp to >= 2) before building the evenly spaced ticks, and add a regression test next to the new force-count coverage.
Suggested fix
if (forceTickCount) {
// forceTickCount must be honored exactly. d3.ticks treats its count
// argument as a hint, so fall back to evenly-spaced ticks here.
+ if (forceTickCount === 1) {
+ const tick = Math.round(zoomedDomainMin * 1000000) / 1000000;
+ majorTicks = [tick];
+ minorTicks = [tick];
+ return {
+ ticks: minorTicks,
+ tickFormat,
+ cursorFormat,
+ };
+ }
+
const majorTickCount = forceTickCount;
majorTicks = range(0, majorTickCount).map(
(i) =>
Math.round(
(zoomedDomainMin +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (forceTickCount) { | |
| // forceTickCount must be honored exactly. d3.ticks treats its count | |
| // argument as a hint, so fall back to evenly-spaced ticks here. | |
| const majorTickCount = forceTickCount; | |
| majorTicks = range(0, majorTickCount).map( | |
| (i) => | |
| Math.round( | |
| (zoomedDomainMin + | |
| (i / (majorTickCount - 1)) * | |
| (zoomedDomainMax - zoomedDomainMin)) * | |
| 1000000 | |
| ) / 1000000 | |
| ); | |
| const minorTickCount = forceTickCount; | |
| minorTicks = range(0, minorTickCount).map( | |
| (i) => | |
| Math.round( | |
| (zoomedDomainMin + | |
| (i / (minorTickCount - 1)) * | |
| (zoomedDomainMax - zoomedDomainMin)) * | |
| 1000000 | |
| ) / 1000000 | |
| ); | |
| if (forceTickCount) { | |
| // forceTickCount must be honored exactly. d3.ticks treats its count | |
| // argument as a hint, so fall back to evenly-spaced ticks here. | |
| if (forceTickCount === 1) { | |
| const tick = Math.round(zoomedDomainMin * 1000000) / 1000000; | |
| majorTicks = [tick]; | |
| minorTicks = [tick]; | |
| return { | |
| ticks: minorTicks, | |
| tickFormat, | |
| cursorFormat, | |
| }; | |
| } | |
| const majorTickCount = forceTickCount; | |
| majorTicks = range(0, majorTickCount).map( | |
| (i) => | |
| Math.round( | |
| (zoomedDomainMin + | |
| (i / (majorTickCount - 1)) * | |
| (zoomedDomainMax - zoomedDomainMin)) * | |
| 1000000 | |
| ) / 1000000 | |
| ); | |
| const minorTickCount = forceTickCount; | |
| minorTicks = range(0, minorTickCount).map( | |
| (i) => | |
| Math.round( | |
| (zoomedDomainMin + | |
| (i / (minorTickCount - 1)) * | |
| (zoomedDomainMax - zoomedDomainMin)) * | |
| 1000000 | |
| ) / 1000000 | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@front_end/src/utils/charts/axis.ts` around lines 622 - 644, The evenly-spaced
tick branch fails for forceTickCount === 1 due to division by (count - 1);
update the logic that builds majorTicks and minorTicks (the block using
forceTickCount, majorTickCount/minorTickCount, zoomedDomainMin/zoomedDomainMax
and range) to special-case count === 1 (or clamp count to >= 2) so you don't
compute i / 0—e.g., when forceTickCount === 1 produce a single rounded tick (use
the midpoint of zoomedDomainMin and zoomedDomainMax or a defined endpoint) and
otherwise keep the existing interpolation and rounding; also add a regression
test that exercises forceTickCount = 1 alongside existing force-count tests.
| const minorTicksPerMajor = findOptimalTickCount( | ||
| rangeMin, | ||
| rangeMin + | ||
| (rangeMax - rangeMin) * (majorTicks[1] ?? 1 / majorTicks.length), | ||
| direction === "horizontal" ? 4 : 2, | ||
| direction === "horizontal" ? 10 : 5 | ||
| ); | ||
| const minorTickCount = | ||
| Math.max(majorTicks.length - 1, 1) * minorTicksPerMajor + 1; |
There was a problem hiding this comment.
Base minor-tick density on the major step, not the second tick value.
majorTicks[1] is an absolute position, but this calculation treats it like a normalized interval. For a domain like [0, 100], the second labeled tick can be 20, which turns the comparison span into 2000 instead of 20. That makes findOptimalTickCount() choose minor density from the wrong range whenever the domain is not normalized.
Suggested fix
- const minorTicksPerMajor = findOptimalTickCount(
- rangeMin,
- rangeMin +
- (rangeMax - rangeMin) * (majorTicks[1] ?? 1 / majorTicks.length),
+ const majorStep =
+ majorTicks.length > 1
+ ? majorTicks[1] - majorTicks[0]
+ : zoomedDomainMax - zoomedDomainMin;
+ const domainSpan = domainMax - domainMin;
+ const scaledMajorStep =
+ domainSpan === 0
+ ? 0
+ : ((rangeMax - rangeMin) * majorStep) / domainSpan;
+ const minorTicksPerMajor = findOptimalTickCount(
+ rangeMin,
+ rangeMin + scaledMajorStep,
direction === "horizontal" ? 4 : 2,
direction === "horizontal" ? 10 : 5
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@front_end/src/utils/charts/axis.ts` around lines 651 - 659, The minor-tick
density calculation incorrectly uses majorTicks[1] as an absolute position;
change it to use the major step width instead. Compute a majorStep from
majorTicks (e.g., majorStep = (majorTicks[1] - majorTicks[0]) if available, else
(rangeMax - rangeMin) / Math.max(1, majorTicks.length - 1)), then call
findOptimalTickCount(rangeMin, rangeMin + majorStep, ...) to derive
minorTicksPerMajor and keep the minorTickCount calculation using
Math.max(majorTicks.length - 1, 1) * minorTicksPerMajor + 1; handle the case
where majorTicks has fewer than 2 entries by falling back to the computed step.
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
|
@claude on preview environment, ticks are still displaying identically to existing code on prod |
|
Claude finished @leonardthethird's task in 2m 9s —— View job Root cause: the d3.ticks() branch is essentially never reachedThe current change splits the linear-scaling path into two branches: if (forceTickCount) {
// evenly-spaced (identical to old behavior)
} else {
// d3.ticks() — the new "nice ticks" path
}The problem: virtually every chart in the codebase passes Confirmed callers that always pass it:
The only callers that can pass What to changeYou need to actually let
Option 1 is the smallest change and matches the spirit of the PR. The risk is that on small feed-card charts ( Want me to push option 1? If yes, I'll also add a regression test that exercises the |
|
@claude yes |
|
Expected with this PR — the fan chart on group cards passes The improvement only shows up on linear-scale charts that don't force a count. Fan charts are a planned follow-up — we'll need to either drop the hard count requirement there or use a Talbot-Lin-style algorithm that can target a count more reliably. Generated by Claude Code |
|
Claude finished @leonardthethird's task in 9m 51s —— View job Done — pushed
|
The previous attempt picked nice values in domain space [0, 1] and only ran when forceTickCount was unset. In production, callers always pass forceTickCount and the actual data range (e.g. [-27.7, 20]) is in rangeScaling, so neither path produced nice display values. Now, for numeric linear scales, generate d3.ticks() in range space and map each result back to domain coordinates. forceTickCount becomes a hint to d3.ticks(); date axes (which would get ugly raw timestamps from d3.ticks) keep evenly-spaced behavior. Also: - minor-tick density is now derived from the major step in range units rather than an absolute tick position - evenly-spaced fallback clamps count to >= 2 to avoid divide-by-zero - new tests cover the forceTickCount + numeric and domain != range cases Co-authored-by: leonardthethird <leonardthethird@users.noreply.github.com>
The test reconstructs range values from domain ticks rounded to 6 decimals, so the recovered step picks up floating-point dust (9.9999711 instead of 10). Snap log10(step) to the nearest integer when it's already within 1e-3 of one, and widen the alignment tolerance to match the same precision floor. https://claude.ai/code/session_017gYZRiw2Sq41iLGq7KPJzL
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
d3.ticks treats its count argument as a hint and picks the nicest step regardless of the resulting tick count. For ranges like [-10, 8] with target=3 it picks step 2, returning 10 ticks — which overflows small feed-card axes and produces overlapping labels. Add a niceTicksAtMost helper that walks from maxCount down until d3.ticks returns a count that fits, turning the parameter into a ceiling instead of a hint. Trades occasional sparser axes for the guarantee that labels never overlap. Minor ticks (gridlines) are unaffected — density there is desirable. https://claude.ai/code/session_017gYZRiw2Sq41iLGq7KPJzL
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
The log branch generated evenly-spaced positions in the warped domain, scaled them to display values, then rounded to fewest sig figs. That preserved the endpoints verbatim — so range_max=52.7 showed up as a literal "52.7" tick label. Apply the same pattern as the linear branch: call niceTicksAtMost in display (range) space and unscale each result back to domain coordinates. Drop minimumSignificantRounding and its sig-fig-cost search loop entirely. Minor ticks still subdivide each major interval evenly in display space — that's what gives the gridlines their log-spaced look. https://claude.ai/code/session_017gYZRiw2Sq41iLGq7KPJzL
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
Caps the d3.ticks count hint at 4 in both branches that pick nice labels. Combined with niceTicksAtMost as a ceiling and a c=1 fallback that always returns at least the endpoints, the visible label count is now bounded between 2 and 4 — small cards no longer overcrowd, and very tall axes don't stretch to a wall of labels. Also fixes a latent bug where minor ticks didn't always include the majors: with the tighter cap, d3 picks different steps for the two calls, so the major positions can be missing from the minor array — and tickFormat would then filter their labels away. Merge majors into minor explicitly to keep that invariant. https://claude.ai/code/session_017gYZRiw2Sq41iLGq7KPJzL
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
leonardthethird
temporarily deployed
to
testing_env
GitHub Actions
Inactive
2 participants
Summary
Improved linear scale tick generation by leveraging d3's
ticks()function to produce "nice" round numbers instead of evenly-spaced ticks. This results in more readable axis labels that follow the pattern of multiples of {1, 2, 5} × 10^k.Key Changes
d3.ticks()to generate major ticks that are mathematically "nice" (round numbers that are easier to read and interpret)forceTickCountis specified, the original evenly-spaced algorithm is used to honor the exact count requirement, since d3.ticks() treats count as a hint rather than a guaranteeImplementation Details
zeroPointis null)maxLabelCountparameter is passed to d3.ticks() to control the approximate number of ticks generatedMath.max(majorTicks.length - 1, 1)to handle edge cases safelyhttps://claude.ai/code/session_017gYZRiw2Sq41iLGq7KPJzL
Summary by CodeRabbit
Improvements
Tests