Better cursor rules #10431
Better cursor rules #10431
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR reorganizes the development guidelines by splitting the monolithic .cursorrules file into granular, topic-specific documentation under the .cursor/rules directory.
- Added comprehensive
README.mdin.cursor/rulesserving as a central index for all development guidelines and commands - Created detailed
architecture.mddocumenting Twenty's monorepo structure, tech stack, and infrastructure components - Added focused state management guidelines in
react-state-management-guidelines.mdcovering Recoil and Apollo Client best practices - Introduced clear code style rules in
code-style-guidelines.mdfor control flow, error handling, and organization - Removed the monolithic
.cursorrulesfile in favor of this more maintainable structure
10 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
| const isValidName = (name: string) => { | ||
| return name.length >= 2 && /^[a-zA-Z\s]*$/.test(name); | ||
| }; |
There was a problem hiding this comment.
style: The regex pattern ^[a-zA-Z\s]*$ doesn't handle international names. Consider using Unicode properties for name validation.
| logger.error('Failed to fetch user data', { | ||
| userId, | ||
| error: error instanceof Error ? error.message : 'Unknown error', | ||
| }); | ||
| throw new UserFetchError('Failed to fetch user data'); |
There was a problem hiding this comment.
style: Error message is duplicated in logger.error and UserFetchError. Consider using a single error message source.
| logger.error('Failed to fetch user data', { | |
| userId, | |
| error: error instanceof Error ? error.message : 'Unknown error', | |
| }); | |
| throw new UserFetchError('Failed to fetch user data'); | |
| const errorMessage = 'Failed to fetch user data'; | |
| logger.error(errorMessage, { | |
| userId, | |
| error: error instanceof Error ? error.message : 'Unknown error', | |
| }); | |
| throw new UserFetchError(errorMessage); |
| const statusColors: Record<Status, string> = { | ||
| success: 'green', | ||
| warning: 'yellow', | ||
| error: 'red', | ||
| default: 'gray', | ||
| }; |
There was a problem hiding this comment.
logic: The lookup object example is missing type safety for the default case if Status is a union type.
| const handleScroll = useCallback((event: UIEvent) => { | ||
| // Complex scroll handling | ||
| }, []); |
There was a problem hiding this comment.
logic: empty dependency array with event parameter usage could cause stale closure issues
|
|
||
| // Local imports | ||
| import { UserAvatar } from './UserAvatar'; | ||
| import { type UserProfileProps } from './types'; |
There was a problem hiding this comment.
style: type import should be before component import to maintain consistent ordering (types before components)
|
|
||
| ### Test File Location | ||
| - Place test files next to implementation | ||
| - Use `.test.ts` or `.spec.ts` extension |
There was a problem hiding this comment.
logic: inconsistent file extension guidance - earlier examples use .tsx for React components but here suggests .ts for test files
| async login(email: string, password: string) { | ||
| await this.page.fill('[data-testid="email-input"]', email); | ||
| await this.page.fill('[data-testid="password-input"]', password); | ||
| await this.page.click('[data-testid="login-button"]'); | ||
| } |
There was a problem hiding this comment.
logic: LoginPage class is missing page initialization in constructor
| async login(email: string, password: string) { | |
| await this.page.fill('[data-testid="email-input"]', email); | |
| await this.page.fill('[data-testid="password-input"]', password); | |
| await this.page.click('[data-testid="login-button"]'); | |
| } | |
| constructor(private page: Page) {} | |
| async login(email: string, password: string) { | |
| await this.page.fill('[data-testid="email-input"]', email); | |
| await this.page.fill('[data-testid="password-input"]', password); | |
| await this.page.click('[data-testid="login-button"]'); | |
| } |
| it('matches visual snapshot', async () => { | ||
| const image = await page.screenshot(); | ||
| expect(image).toMatchImageSnapshot(); | ||
| }); |
There was a problem hiding this comment.
style: Visual snapshot test missing viewport size configuration which could lead to inconsistent results
| ``` | ||
|
|
||
| #### Provide Context | ||
| - Lingui provides powerfulway to add context for translators but we don't use them as of today. |
There was a problem hiding this comment.
syntax: 'powerfulway' should be 'powerful way'
| - Lingui provides powerfulway to add context for translators but we don't use them as of today. | |
| - Lingui provides powerful way to add context for translators but we don't use them as of today. |
| function handleResult(result: Result) { | ||
| if (result.type === 'success') { | ||
| // TypeScript knows result.data exists | ||
| console.log(result.data); | ||
| } | ||
| } |
There was a problem hiding this comment.
style: Type guard example should handle both success and error cases to demonstrate proper exhaustive checking.
| function handleResult(result: Result) { | |
| if (result.type === 'success') { | |
| // TypeScript knows result.data exists | |
| console.log(result.data); | |
| } | |
| } | |
| function handleResult(result: Result) { | |
| if (result.type === 'success') { | |
| // TypeScript knows result.data exists | |
| console.log(result.data); | |
| } else { | |
| // TypeScript knows this is the error case | |
| console.error(result.message); | |
| } | |
| } |
Move to the new cursor rule folder style and make it more granular