Skip to content

feat: nullable levels#188

Open
edlng wants to merge 1 commit intomainfrom
feature/optional-levels
Open

feat: nullable levels#188
edlng wants to merge 1 commit intomainfrom
feature/optional-levels

Conversation

@edlng
Copy link
Member

@edlng edlng commented Mar 4, 2026

Closes #176

Makes it so that non-exercise classes have nullable levels (i.e., doesn't show the level slider).

Signed-off-by: edlng <edwardliangedli@gmail.com>
@greptile-apps
Copy link

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR successfully makes lowerLevel and upperLevel nullable for non-exercise classes across the full stack: DB schema, Drizzle ORM, domain models, API validation, and UI components. The core feature is well-implemented with correct conditional rendering and form auto-clearing logic.

One actionable finding: The frontend ClassEditSchema enforces that exercise classes must provide levels, but this business rule is not enforced at the API schema layer. Direct API calls can bypass the UI and create exercise classes with null levels, potentially leading to inconsistent data.

Confidence Score: 4/5

  • Safe to merge; the feature works correctly end-to-end with one actionable validation gap at the API layer.
  • This PR implements the nullable levels feature comprehensively across DB, models, API schemas, and UI. The core functionality is sound with proper null guards and conditional rendering. The identified issue—missing server-side validation for the exercise category constraint—is a real business rule gap that could allow inconsistent data via direct API calls, but it doesn't block the feature from working correctly through the UI.
  • src/models/api/class.ts requires a server-side refinement to enforce that exercise categories must have levels (mirroring the frontend validation).

Last reviewed commit: a7a5279

@greptile-apps
Copy link

greptile-apps bot commented Mar 4, 2026

Additional Comments (1)

src/models/api/class.ts, line 20
The frontend ClassEditSchema enforces that exercise classes must provide a levelRange (lines 29-40 in schema.ts). However, the CreateClass API schema (and UpdateClass) only mark the fields as nullable without any cross-field check. This means any caller that bypasses the UI—such as a direct API call or a future script—can create an exercise class with lowerLevel: null, upperLevel: null, and the server will accept it.

Consider adding a server-side refinement to ensure the business rule is enforced end-to-end:

export const CreateClass = z
  .object({
    termId: z.uuid(),
    image: z.string().optional(),
    name: z.string().nonempty(),
    description: z.string().optional(),
    meetingURL: z.url().optional(),
    lowerLevel: z.int().min(1).max(4).nullish(),
    upperLevel: z.int().min(1).max(4).nullish(),
    category: z.string(),
    subcategory: z.string().optional(),
    schedules: z.array(CreateSchedule).default([]),
  })
  .refine(
    (val) => {
      if (val.category.includes("Exercise")) {
        return val.lowerLevel != null && val.upperLevel != null;
      }
      return true;
    },
    {
      error: "Levels are required for exercise classes",
      path: ["lowerLevel"],
    },
  );

check(
"chk_lower_level_bounds",
sql`${table.lowerLevel} >= 1 AND ${table.lowerLevel} <= 4`,
sql`${table.lowerLevel} IS NULL OR (${table.lowerLevel} >= 1 AND ${table.lowerLevel} <= 4)`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make the logic be either both lower and upper are present or the both are not present. Right now I believe the logic is not exactly that.

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.

Make levels optional feature

2 participants