feat(securities): express vesting & cliff in months instead of years#562
Open
maxgubitosi wants to merge 1 commit into
Open
feat(securities): express vesting & cliff in months instead of years#562maxgubitosi wants to merge 1 commit into
maxgubitosi wants to merge 1 commit into
Conversation
Vesting and cliff durations for shares and stock options were entered and stored in years; switch the unit to months (e.g. a 4-year / 1-year-cliff grant is now 48 months / 12 months). - Rename the Share/Option `vestingYears`/`cliffYears` fields to `vestingMonths`/`cliffMonths`, with a Prisma migration that renames the columns and converts existing values from years to months (x12). - Update the Zod mutation schemas, the public OpenAPI schema, the add/get procedures for shares and options, and the share/option creation forms (the numeric input suffix now reads "month(s)"). - No vesting math exists in the app, so this is purely a unit/label/storage change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughVesting and cliff duration fields are renamed from year-based ( ChangesVesting/Cliff Duration: Years → Months
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
prisma/migrations/20260619131445_vesting_cliff_in_months/migration.sql (1)
10-20: ⚡ Quick winVerify the rename and conversion run atomically.
If the migration runner does not wrap this file in one transaction, a failure after the renames but before both
UPDATEs can leave month-named columns containing year-based values. Please verify the runner behavior or add an explicit transaction around these statements.Atomic migration shape
+BEGIN; + -- AlterTable ALTER TABLE "Share" RENAME COLUMN "vestingYears" TO "vestingMonths"; ALTER TABLE "Share" RENAME COLUMN "cliffYears" TO "cliffMonths"; @@ -- Convert existing durations from years to months UPDATE "Share" SET "vestingMonths" = "vestingMonths" * 12, "cliffMonths" = "cliffMonths" * 12; UPDATE "Option" SET "vestingMonths" = "vestingMonths" * 12, "cliffMonths" = "cliffMonths" * 12; + +COMMIT;🤖 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 `@prisma/migrations/20260619131445_vesting_cliff_in_months/migration.sql` around lines 10 - 20, The migration contains column renames followed by value conversions that could leave the database in an inconsistent state if execution fails between the ALTER TABLE and UPDATE statements. Wrap all the ALTER TABLE statements (for both "Share" and "Option" tables renaming "vestingYears" to "vestingMonths" and "cliffYears" to "cliffMonths") and the subsequent UPDATE statements (that multiply the values by 12 for both tables) within an explicit BEGIN TRANSACTION and COMMIT block to ensure all operations execute atomically, so that either all changes succeed or all roll back together.
🤖 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 `@prisma/schema.prisma`:
- Around line 693-694: The comment for cliffMonths incorrectly conflates "no
cliff" with "immediate vesting". Update the cliffMonths comment to clarify that
a value of 0 means "no cliff" (not immediate vesting), while keeping the
vestingMonths comment unchanged since it correctly states that 0 means immediate
vesting. Apply the same comment fix to the duplicate cliffMonths and
vestingMonths fields that appear later in the schema around lines 745-746.
In `@src/server/api/schema/shares.ts`:
- Around line 60-66: Update the schema validation for the month fields in
ShareSchema to enforce both integer and non-negative constraints. For both
cliffMonths and vestingMonths fields, change z.number() to
z.number().int().min(0) to match the Prisma model's Int type definition and
align with the internal tRPC/UI validation that already enforces these
constraints. This ensures the public API schema is consistent with the storage
layer and prevents invalid negative or fractional month values from being
accepted.
---
Nitpick comments:
In `@prisma/migrations/20260619131445_vesting_cliff_in_months/migration.sql`:
- Around line 10-20: The migration contains column renames followed by value
conversions that could leave the database in an inconsistent state if execution
fails between the ALTER TABLE and UPDATE statements. Wrap all the ALTER TABLE
statements (for both "Share" and "Option" tables renaming "vestingYears" to
"vestingMonths" and "cliffYears" to "cliffMonths") and the subsequent UPDATE
statements (that multiply the values by 12 for both tables) within an explicit
BEGIN TRANSACTION and COMMIT block to ensure all operations execute atomically,
so that either all changes succeed or all roll back together.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a1debd1-503d-44a4-81b2-a12279e33dc5
📒 Files selected for processing (10)
prisma/migrations/20260619131445_vesting_cliff_in_months/migration.sqlprisma/schema.prismasrc/components/securities/options/steps/vesting-details.tsxsrc/components/securities/shares/steps/general-details.tsxsrc/server/api/schema/shares.tssrc/trpc/routers/securities-router/procedures/add-option.tssrc/trpc/routers/securities-router/procedures/add-share.tssrc/trpc/routers/securities-router/procedures/get-options.tssrc/trpc/routers/securities-router/procedures/get-shares.tssrc/trpc/routers/securities-router/schema.ts
Comment on lines
+693
to
+694
| cliffMonths Int @default(0) // vesting cliff in months; 0 means immediate vesting | ||
| vestingMonths Int @default(0) // total vesting period in months; 0 means immediate vesting |
There was a problem hiding this comment.
Clarify the zero-cliff semantics.
cliffMonths = 0 means “no cliff”, not necessarily immediate vesting when vestingMonths is still positive. Keep “immediate vesting” on vestingMonths = 0 only.
Proposed wording fix
- cliffMonths Int `@default`(0) // vesting cliff in months; 0 means immediate vesting
+ cliffMonths Int `@default`(0) // vesting cliff in months; 0 means no cliff
vestingMonths Int `@default`(0) // total vesting period in months; 0 means immediate vesting- cliffMonths Int `@default`(0) // vesting cliff in months; 0 means immediate vesting
+ cliffMonths Int `@default`(0) // vesting cliff in months; 0 means no cliff
vestingMonths Int `@default`(0) // total vesting period in months; 0 means immediate vestingAlso applies to: 745-746
🤖 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 `@prisma/schema.prisma` around lines 693 - 694, The comment for cliffMonths
incorrectly conflates "no cliff" with "immediate vesting". Update the
cliffMonths comment to clarify that a value of 0 means "no cliff" (not immediate
vesting), while keeping the vestingMonths comment unchanged since it correctly
states that 0 means immediate vesting. Apply the same comment fix to the
duplicate cliffMonths and vestingMonths fields that appear later in the schema
around lines 745-746.
Comment on lines
+60
to
+66
| cliffMonths: z.number().openapi({ | ||
| description: "Vesting cliff in months", | ||
| example: 12, | ||
| }), | ||
| vestingYears: z.number().openapi({ | ||
| description: "Vesting Years", | ||
| example: 4, | ||
| vestingMonths: z.number().openapi({ | ||
| description: "Total vesting period in months", | ||
| example: 48, |
There was a problem hiding this comment.
Add integer and non-negative constraints to month fields in public API schema.
The ShareSchema uses z.number() for cliffMonths and vestingMonths, which accepts negative and fractional values. However, the Prisma model defines both as Int type, and internal tRPC/UI validation already enforces .min(0). Update the public API schema to match the storage and internal validation layer.
Proposed validation fix
- cliffMonths: z.number().openapi({
+ cliffMonths: z.number().int().min(0).openapi({
description: "Vesting cliff in months",
example: 12,
}),
- vestingMonths: z.number().openapi({
+ vestingMonths: z.number().int().min(0).openapi({
description: "Total vesting period in months",
example: 48,🤖 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 `@src/server/api/schema/shares.ts` around lines 60 - 66, Update the schema
validation for the month fields in ShareSchema to enforce both integer and
non-negative constraints. For both cliffMonths and vestingMonths fields, change
z.number() to z.number().int().min(0) to match the Prisma model's Int type
definition and align with the internal tRPC/UI validation that already enforces
these constraints. This ensures the public API schema is consistent with the
storage layer and prevents invalid negative or fractional month values from
being accepted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Vesting and cliff durations for shares and stock options were previously entered and stored in years. This PR switches the unit to months (e.g. a 4-year grant with a 1-year cliff is now 48 months / 12 months), which is the granularity most grants actually use.
Changes
Share.{vestingYears,cliffYears}andOption.{vestingYears,cliffYears}to{vestingMonths,cliffMonths}.20260619131445_vesting_cliff_in_months) — renames the columns and converts existing values from years to months (* 12), so existing rows keep the same real-world duration.Notes
cliffYears/vestingYearsfields are renamed tocliffMonths/vestingMonths(src/server/api/schema/shares.ts).Testing
prisma generateregenerates cleanly with the renamed fields.pnpm build(Next.js production build) passes with no type errors.Summary by CodeRabbit