-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add agreement to limits #1892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a new nullable boolean limit agreementOnBusiness to limits schema and server config, derives allowAgreementOnBusiness in link options, and switches AgreementSection gating to this new flag. Changes
Pre-merge checks✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/limits/constants.ts (1)
13-13: Verify default behavior for plan limits.The
agreementOnBusinessfield is added to the type but not set in any of the exported plan constants (FREE_PLAN_LIMITS, PRO_PLAN_LIMITS, BUSINESS_PLAN_LIMITS, etc.). This means:
- The field will be
undefinedfor all plans unless explicitly configured in the database- Combined with the
?? falsedefault inlink-options.tsx(line 105), the agreement feature will be disabled by default for all plansIf certain plans (e.g., Business, Data Rooms) should have this feature enabled by default, consider adding it to their respective plan constants:
export const BUSINESS_PLAN_LIMITS = { users: 3, links: null, documents: null, domains: 5, datarooms: 100, customDomainOnPro: true, customDomainInDataroom: false, advancedLinkControlsOnPro: false, + agreementOnBusiness: true, fileSizeLimits: { maxFiles: 500, }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/links/link-sheet/link-options.tsx(2 hunks)ee/limits/constants.ts(1 hunks)ee/limits/server.ts(1 hunks)
🔇 Additional comments (3)
ee/limits/server.ts (1)
45-45: LGTM!The schema definition is consistent with the existing
watermarkOnBusinesspattern and correctly usesz.boolean().nullish()to allow null or undefined values.components/links/link-sheet/link-options.tsx (2)
105-105: LGTM!The flag derivation follows the same pattern as
allowWatermarkOnBusinesson line 104 and correctly defaults tofalsewhen the limit is not set.
217-221: Verify migration for existing teamsAgreementSection now gates on
allowAgreementOnBusinessinstead ofallowWatermarkOnBusiness. Both flags come from thelimitsJSON field, so teams withwatermarkOnBusinesscurrently enabled will lose agreement access unless you backfillagreementOnBusiness. Confirm:
- A backfill or migration exists to set
agreementOnBusiness: truefor teams wherewatermarkOnBusiness: true.- Or the change is intentional and teams should opt in again.
Summary by CodeRabbit
New Features
Bug Fixes