Skip to content

fix: changed upper bound to be able to be null so it can be set blank…#167

Merged
wfelliss merged 1 commit into
mainfrom
upper-bound-null
Jun 29, 2026
Merged

fix: changed upper bound to be able to be null so it can be set blank…#167
wfelliss merged 1 commit into
mainfrom
upper-bound-null

Conversation

@wfelliss

Copy link
Copy Markdown
Owner

… and db:generation time

@wfelliss
wfelliss merged commit 8e5934e into main Jun 29, 2026
2 checks passed
@wfelliss
wfelliss deleted the upper-bound-null branch June 29, 2026 14:17

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

Copy link
Copy Markdown
Contributor

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 modifies the database schema to make the upper_bound_idx column nullable. However, the review feedback points out that corresponding backend changes are required to support this, including updating DTO validation, service type definitions, and adjusting nullish coalescing logic to handle explicit null values correctly.

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.

profile: integer("profile").references(() => profiles.id), // associated profile
lower_bound_idx: integer("lower_bound_idx").notNull().default(0), // inclusive trim start index (default: 0)
upper_bound_idx: integer("upper_bound_idx").notNull(), // inclusive trim end index (set by service layer to length-1)
upper_bound_idx: integer("upper_bound_idx"), // inclusive trim end index (set by service layer to length-1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

While making upper_bound_idx nullable in the database schema is necessary to allow it to be set to blank, the current backend implementation will prevent this from working as expected:

  1. DTO Validation Failure: In apps/backend/src/runs/dto/update-run.dto.ts, upper_bound_idx is decorated with @IsInt() and @Min(0). If a client sends null, validation will fail with a 400 Bad Request. You should update the DTO to allow null, for example:
    @IsOptional()
    @IsInt()
    @Min(0)
    @ValidateIf((object, value) => value !== null)
    upper_bound_idx?: number | null;
  2. Service Type Mismatch: In RunsService (runs.service.ts), the updates parameter type for updateRun and data for createRun define upper_bound_idx?: number, which does not permit null. These should be updated to number | null.
  3. Nullish Coalescing Logic: In RunsService.updateRun, the line:
    const effectiveUpperBound = updates.upper_bound_idx ?? currentRun.upper_bound_idx ?? Math.max(effectiveLength - 1, 0);
    uses the nullish coalescing operator (??). If updates.upper_bound_idx is passed as null to clear the bound, updates.upper_bound_idx ?? ... will fall back to currentRun.upper_bound_idx (the old non-null value), preventing the field from ever being cleared/set to null in the database. You will need to adjust this logic to distinguish between undefined (no update) and null (explicitly clearing the bound).

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.

1 participant