Skip to content

Fix NaN handling in responseAdSlotCount - #1254

Open
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/response-ad-positions-validation
Open

Fix NaN handling in responseAdSlotCount#1254
pavankumar-vh wants to merge 1 commit into
CodebuffAI:mainfrom
pavankumar-vh:fix/response-ad-positions-validation

Conversation

@pavankumar-vh

Copy link
Copy Markdown

Overview

Fix NaN handling in the responseAdSlotCount function in common/src/util/response-ad-positions.ts.

Bug Description

The function didn't validate that params.step and params.firstAdAfterNodes are finite numbers. If they were NaN or Infinity, Math.max(1, NaN) would return NaN.

Fix

Added Number.isFinite() checks to default to safe values for invalid numbers.

Testing

No existing tests for this function, but the fix prevents incorrect behavior with invalid inputs.

Files Changed

  • common/src/util/response-ad-positions.ts - Added NaN validation

Scope

This change only touches common/ which is an approved contribution area per the Contributing Guide.

The function didn't validate that params.step and params.firstAdAfterNodes are
finite numbers. If they were NaN or Infinity, Math.max(1, NaN) would return NaN.

Added Number.isFinite() checks to default to safe values for invalid numbers.
@codebuff-team

Copy link
Copy Markdown
Contributor

Good catch on the NaN propagation issue — Math.max(1, NaN) silently returning NaN would have broken responseAdSlotCount downstream in a way that's hard to trace. The fix itself (checking Number.isFinite before falling back to defaults) is a sensible approach.

A few gaps though:

  1. params.nodeCount is used directly in the final calculation (params.nodeCount - firstAdAfterNodes - 1) without any finiteness check. If nodeCount is NaN or Infinity, the function still returns garbage. Since this PR's whole premise is defensive NaN handling, leaving the primary input unguarded is an inconsistency worth fixing in the same patch.
  2. No test was added. The PR body acknowledges there are no existing tests for this function, but given this is exactly the kind of pure function that's trivial and valuable to test (bad inputs, boundary conditions), a maintainer will likely want at least one test case covering NaN/Infinity inputs before merging this to a private tree by hand.
  3. Minor style: the two safe-value lines are quite long and would trip most lint configs (line length); consider breaking them up as the original code did.

The core idea is right and worth porting, but as-is it fixes half the surface area of the bug it identifies and has no test coverage. Please extend the guard to nodeCount and add a small unit test, and this should be straightforward to land.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants