Conversation
|
@omribz156 is attempting to deploy a commit to the Ever Co Team on Vercel. A member of the Team first needs to authorize it. |
Summary by CodeRabbit
WalkthroughThe pull request fixes a security bug where setting ChangesProject Limit Zero Handling
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
|
Greptile SummaryThis PR fixes a bug where setting
Confidence Score: 4/5The config fix is correct and narrow in scope; the only concern is the test's direct mutation of the shared config singleton, which is guarded by try/finally. The core one-line change is correct and the getNumberOrDefault helper is already proven by its existing use for throttle settings. The new e2e test properly exercises the zero-limit path with a confirming GET assertion. No files require special attention; the change is well-contained to config loading and a single new test case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /api/v1/projects] --> B{Authenticate user}
B -- Unauthorized --> C[401 Unauthorized]
B -- OK --> D{numProjectsCreated >= maxProjectsPerUser?}
D -- "maxProjectsPerUser = 0" --> E[429 TooManyRequests]
D -- "maxProjectsPerUser = 100" --> F[Create project in transaction]
F --> G[201 Created]
Reviews (1): Last reviewed commit: "fix(api): preserve zero project limit" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('/api/v1/projects (POST) should not create a project if the project limit is zero', async () => { | ||
| const maxProjectsPerUser = config.maxProjectsPerUser; | ||
| config.maxProjectsPerUser = 0; |
There was a problem hiding this comment.
config singleton mid-test works because Jest runs test cases within the same file sequentially, and the controller reads config.maxProjectsPerUser at request time. However, a future move to parallel test workers could leave the value permanently set to 0, causing every subsequent test in the file to reject all project creation requests. The try/finally pattern is the correct approach for plain object properties.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/test/project.e2e-spec.ts">
<violation number="1" location="api/test/project.e2e-spec.ts:230">
P2: Regression test mutates in-memory config after the app is already bootstrapped, so it does not cover the original zero-limit config-loading bug.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| it('/api/v1/projects (POST) should not create a project if the project limit is zero', async () => { | ||
| const maxProjectsPerUser = config.maxProjectsPerUser; | ||
| config.maxProjectsPerUser = 0; |
There was a problem hiding this comment.
P2: Regression test mutates in-memory config after the app is already bootstrapped, so it does not cover the original zero-limit config-loading bug.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/test/project.e2e-spec.ts, line 230:
<comment>Regression test mutates in-memory config after the app is already bootstrapped, so it does not cover the original zero-limit config-loading bug.</comment>
<file context>
@@ -224,6 +225,31 @@ describe('ProjectController (e2e)', () => {
+ it('/api/v1/projects (POST) should not create a project if the project limit is zero', async () => {
+ const maxProjectsPerUser = config.maxProjectsPerUser;
+ config.maxProjectsPerUser = 0;
+
+ try {
</file context>
There was a problem hiding this comment.
Thanks, good catch. I replaced that e2e singleton-mutation check with a config-loading unit test in api/src/config.spec.ts, setting TR_MAX_PROJECTS_PER_USER=0 before importing ./config. Verified with git diff --check; local Jest could not run because this checkout has no installed node_modules (cross-env missing), so I'll watch the remote CI run.
|
![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
🤖 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 `@api/src/config.spec.ts`:
- Around line 14-21: Add tests covering parsing of TR_MAX_PROJECTS_PER_USER
beyond the zero case: create additional it blocks in config.spec.ts that (1) set
process.env.TR_MAX_PROJECTS_PER_USER = '5', call jest.resetModules();
import('./config') and assert config.maxProjectsPerUser === 5, (2) delete
process.env.TR_MAX_PROJECTS_PER_USER, reset modules, import('./config') and
assert it falls back to the default value used by config.maxProjectsPerUser, (3)
set process.env.TR_MAX_PROJECTS_PER_USER = 'invalid', reset modules,
import('./config') and assert it falls back to the default, and optionally (4)
set a negative string like '-1' and assert behavior (default or clamped)
expected by the config implementation; reuse jest.resetModules() and the dynamic
import('./config') pattern to ensure environment changes are picked up.
- Around line 4-12: Move the jest.resetModules() call into a beforeEach block so
each test starts with a clean module cache: add beforeEach(() =>
jest.resetModules()); remove the duplicate jest.resetModules() invocations
inside individual tests and from afterEach; keep the existing afterEach logic
that restores process.env.TR_MAX_PROJECTS_PER_USER using maxProjectsPerUser (and
delete when undefined) so environment cleanup remains intact while module resets
are centralized in beforeEach.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66b31d81-24aa-45d7-9e09-77386d66affe
📒 Files selected for processing (1)
api/src/config.spec.ts
| afterEach(() => { | ||
| if (maxProjectsPerUser === undefined) { | ||
| delete process.env.TR_MAX_PROJECTS_PER_USER; | ||
| } else { | ||
| process.env.TR_MAX_PROJECTS_PER_USER = maxProjectsPerUser; | ||
| } | ||
|
|
||
| jest.resetModules(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider moving jest.resetModules() to beforeEach for idiomatic test structure.
The current pattern calls jest.resetModules() both inside the test (line 15) and in afterEach (line 11). The idiomatic Jest pattern is to place module resets in beforeEach, ensuring every test starts with a clean module cache without requiring individual tests to explicitly reset.
♻️ Proposed refactor to use beforeEach pattern
describe('config', () => {
const maxProjectsPerUser = process.env.TR_MAX_PROJECTS_PER_USER;
+ beforeEach(() => {
+ jest.resetModules();
+ });
+
afterEach(() => {
if (maxProjectsPerUser === undefined) {
delete process.env.TR_MAX_PROJECTS_PER_USER;
} else {
process.env.TR_MAX_PROJECTS_PER_USER = maxProjectsPerUser;
}
-
- jest.resetModules();
});
it('keeps a zero max project limit from the environment', async () => {
- jest.resetModules();
process.env.TR_MAX_PROJECTS_PER_USER = '0';
const { config } = await import('./config');🤖 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 `@api/src/config.spec.ts` around lines 4 - 12, Move the jest.resetModules()
call into a beforeEach block so each test starts with a clean module cache: add
beforeEach(() => jest.resetModules()); remove the duplicate jest.resetModules()
invocations inside individual tests and from afterEach; keep the existing
afterEach logic that restores process.env.TR_MAX_PROJECTS_PER_USER using
maxProjectsPerUser (and delete when undefined) so environment cleanup remains
intact while module resets are centralized in beforeEach.
| it('keeps a zero max project limit from the environment', async () => { | ||
| jest.resetModules(); | ||
| process.env.TR_MAX_PROJECTS_PER_USER = '0'; | ||
|
|
||
| const { config } = await import('./config'); | ||
|
|
||
| expect(config.maxProjectsPerUser).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding test cases for other scenarios to increase coverage.
The current test validates the zero case (the specific fix), but additional test cases would increase confidence in the config parsing logic:
- Positive numbers (e.g.,
'5'→5) - Undefined/not set (should use default value)
- Invalid strings (e.g.,
'invalid') - Negative numbers (if applicable)
📝 Example additional test cases
it('parses positive max project limit from the environment', async () => {
process.env.TR_MAX_PROJECTS_PER_USER = '5';
const { config } = await import('./config');
expect(config.maxProjectsPerUser).toBe(5);
});
it('uses default when max project limit is not set', async () => {
delete process.env.TR_MAX_PROJECTS_PER_USER;
const { config } = await import('./config');
expect(config.maxProjectsPerUser).toBe(100); // or whatever the default is
});
it('uses default when max project limit is invalid', async () => {
process.env.TR_MAX_PROJECTS_PER_USER = 'invalid';
const { config } = await import('./config');
expect(config.maxProjectsPerUser).toBe(100); // or whatever the default is
});🤖 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 `@api/src/config.spec.ts` around lines 14 - 21, Add tests covering parsing of
TR_MAX_PROJECTS_PER_USER beyond the zero case: create additional it blocks in
config.spec.ts that (1) set process.env.TR_MAX_PROJECTS_PER_USER = '5', call
jest.resetModules(); import('./config') and assert config.maxProjectsPerUser ===
5, (2) delete process.env.TR_MAX_PROJECTS_PER_USER, reset modules,
import('./config') and assert it falls back to the default value used by
config.maxProjectsPerUser, (3) set process.env.TR_MAX_PROJECTS_PER_USER =
'invalid', reset modules, import('./config') and assert it falls back to the
default, and optionally (4) set a negative string like '-1' and assert behavior
(default or clamped) expected by the config implementation; reuse
jest.resetModules() and the dynamic import('./config') pattern to ensure
environment changes are picked up.
|
Thanks @omribz156 — reviewed, and the fix is correct and well-scoped. Swapping The only remaining blocker is our CAA (Contributor Assignment Agreement — the "CLA" check above from cla-assistant is still |
2 participants
Fixes #525
What changed
TR_MAX_PROJECTS_PER_USER=0when loading API config instead of falling back to100through0 || 100.Verification
git diff --checknode -e "const f=(v,d)=>{if(!v)return d;const p=Number.parseInt(v,10);return Number.isNaN(p)?d:p}; console.log(['0','1','',undefined,'abc'].map(v=>String(f(v,100))).join(','))"->0,1,100,100,100I could not complete the focused e2e run locally because
yarn --cwd api install --frozen-lockfiletimed out before installingcross-env;yarn --cwd api test:e2e ...then failed withcross-env is not recognized.yarn --cwd api buildalso hit the existing TypeScript 6baseUrldeprecation before compiling project files.Before submitting the PR, please make sure you do the following
Contributor license agreement
For us it's important to have the agreement of our contributors to use their work, whether it be code or documentation. Therefore, we are asking all contributors to sign a contributor license agreement (CLA) as commonly accepted in most open source projects. Just open the pull request and our CLA bot will prompt you briefly.
Please check our contribution guidelines for some help in the process.
This was implemented with Codex assistance, with the patch manually reviewed and kept to the project-limit boundary.
Summary by cubic
Preserves zero project limit set via
TR_MAX_PROJECTS_PER_USER=0in API config instead of defaulting to 100, so users cannot create a project when the limit is zero. Adds a unit test for the config to ensure zero is preserved.Written for commit cc740d6. Summary will update on new commits. Review in cubic