feat: add new client structure rfc #2770
Conversation
ogzhanolguncu
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces an RFC (Request for Comments) document proposing a new feature-based file structure for client-side components in a Next.js application. The document outlines a comprehensive approach to reorganizing code from a flat directory structure to a more modular, feature-centric architecture. The proposal aims to improve code maintainability, developer onboarding, and feature development by establishing clear organizational boundaries and colocating related code components. Changes
Suggested reviewers
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
apps/engineering/content/rfcs/0007-client-file-structure.mdx (8)
40-62: Enhance example documentationWhile the current structure is well-illustrated, consider adding brief descriptions of each component's responsibility to better demonstrate the organizational challenges.
For example:
├── authorization/ │ ├── permissions/ │ │ ├── [permissionId]/ -│ │ │ ├── client.tsx +│ │ │ ├── client.tsx # Handles client-side permission logic -│ │ │ ├── delete-permission.tsx +│ │ │ ├── delete-permission.tsx # Permission deletion UI and logic
65-97: Clarify shared code organizationThe proposed structure effectively organizes feature-specific code, but consider adding guidance on:
- How to handle types shared between multiple features
- Where to place utilities used across multiple permissions/roles
- Strategy for sharing schemas between related features
112-146: Consider further code organization improvementsThe example demonstrates good practices, but consider these additional improvements:
- Move the
Propstype to a separatetypes.tsfile- Extract the workspace lookup logic into a custom hook (e.g.,
useWorkspace)+// types.ts +export type AuditPageProps = { + params: { + bucket: string; + }; + searchParams: SearchParams; +}; + +// hooks/use-workspace.ts +export function useWorkspace(tenantId: string) { + return getWorkspace(tenantId); +} + // page.tsx -type Props = { - params: { - bucket: string; - }; - searchParams: SearchParams; -}; +import { type AuditPageProps } from "./types"; +import { useWorkspace } from "./hooks/use-workspace"; -export default async function AuditPage(props: Props) { +export default async function AuditPage(props: AuditPageProps) { const tenantId = getTenantId(); - const workspace = await getWorkspace(tenantId); + const workspace = await useWorkspace(tenantId);
151-193: Consider adding quantifiable metricsThe impact analysis could be strengthened by including:
- Average time spent on structural discussions in PRs
- Number of revision cycles due to structural issues
- Time spent on refactoring due to inconsistent organization
🧰 Tools
🪛 LanguageTool
[uncategorized] ~155-~155: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... our ecosystem: Developer Community: - Open source contributors face a learning curve when...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~191-~191: Consider using a different verb for a more formal wording.
Context: ...ructure makes it easier to identify and fix architectural issues ## Proposed Solu...(FIX_RESOLVE)
194-220: Add guidance for test organizationThe proposed structure should include guidelines for organizing test files. Consider addressing:
- Location of test files (co-located vs separate test directory)
- Organization of test utilities and fixtures
- Strategy for integration tests
281-285: Enhance migration strategyConsider adding:
- Step-by-step migration guide
- Timeline recommendations
- Strategy for handling partially migrated features
- Tooling suggestions for automated migration
289-296: Expand naming conventionsConsider adding conventions for:
- Component file naming (e.g., PascalCase vs kebab-case)
- Test file suffixes (e.g.,
.test.ts,.spec.ts)- Barrel file usage (
index.ts)- Shared type file naming
300-304: Enhance version trackingConsider adding:
- Links to related discussions or PRs
- Status column (Draft, Review, Approved)
- Reviewers column
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/engineering/content/rfcs/0007-client-file-structure.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/rfcs/0007-client-file-structure.mdx
[uncategorized] ~19-~19: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ndardized contribution patterns for our open source community** ## Problem Statement ### C...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~99-~99: Possible missing comma found.
Context: ... actual page files will look like this. Note this is audit component refactored from...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~155-~155: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... our ecosystem: Developer Community: - Open source contributors face a learning curve when...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~191-~191: Consider using a different verb for a more formal wording.
Context: ...ructure makes it easier to identify and fix architectural issues ## Proposed Solu...
(FIX_RESOLVE)
[style] ~255-~255: To elevate your writing, try using a synonym here.
Context: ...from what we have to this one is really hard to do incrementally. ### Alternative 2...
(HARD_TO)
[uncategorized] ~278-~278: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... as our project grows and receives more open source contributions. The lack of clear conven...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/engineering/content/rfcs/0007-client-file-structure.mdx (7)
1-7: Consider adding version number to frontmatterFor better document versioning and tracking, consider adding a
versionfield to the frontmatter metadata.--- title: 0007 Client-side file structure description: File structure for our client apps date: 2024-12-20 +version: 0.1 authors: - Oguzhan Olguncu ---
42-62: Consider adding a component relationship diagramWhile the directory structure is helpful, a visual representation (e.g., component relationship diagram) would better illustrate the dependencies and data flow between components in the current structure. This would make it easier to understand the problems with the current approach.
Would you like me to help create a Mermaid diagram showing the component relationships?
151-193: Add success metrics for measuring impactThe impact and motivation sections would benefit from concrete metrics to measure success, such as:
- Average time spent in code reviews
- Number of revision cycles for new contributors
- Time to first contribution
- Code quality metrics
This will help track the effectiveness of the new structure over time.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~155-~155: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... our ecosystem: Developer Community: - Open source contributors face a learning curve when...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~190-~190: Possible missing preposition found.
Context: ...to test and maintain - Clear boundaries prevent unwanted coupling between features - Co...(AI_HYDRA_LEO_MISSING_TO)
[style] ~191-~191: Consider using a different verb for a more formal wording.
Context: ...ructure makes it easier to identify and fix architectural issues ## Proposed Solu...(FIX_RESOLVE)
214-216: Clarify guidelines for cross-feature communicationWhile the document states "Feature-specific components shouldn't be imported by other features", it doesn't address:
- How features should communicate with each other
- Where to place shared types used across features
- How to handle feature composition
Consider adding patterns for cross-feature interactions and communication.
281-284: Provide detailed migration strategyThe future work section would benefit from:
- A step-by-step migration plan
- Timeline estimates for each phase
- Rollback procedures if issues arise
- Impact on existing features during migration
This will help teams prepare for and execute the transition smoothly.
289-296: Expand file naming conventionsConsider adding conventions for:
- Test files (e.g.,
.test.ts,.spec.ts)- Documentation files (e.g.,
.docs.mdx)- Storybook files (e.g.,
.stories.tsx)- Mock data files (e.g.,
.mock.ts)- Constants files (e.g.,
.constants.ts)This will ensure consistency across all file types in the project.
302-304: Enhance version trackingConsider adding more details to the version history:
- Type of changes (Added, Changed, Deprecated, Removed)
- Link to related discussions or PRs
- Reviewers or approvers
- Status (Draft, Review, Approved, Implemented)
This will help track the evolution of the RFC and its implementation status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/engineering/content/rfcs/0007-client-file-structure.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/rfcs/0007-client-file-structure.mdx
[uncategorized] ~19-~19: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ndardized contribution patterns for our open source community** ## Problem Statement ### C...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~155-~155: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... our ecosystem: Developer Community: - Open source contributors face a learning curve when...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~190-~190: Possible missing preposition found.
Context: ...to test and maintain - Clear boundaries prevent unwanted coupling between features - Co...
(AI_HYDRA_LEO_MISSING_TO)
[style] ~191-~191: Consider using a different verb for a more formal wording.
Context: ...ructure makes it easier to identify and fix architectural issues ## Proposed Solu...
(FIX_RESOLVE)
[style] ~255-~255: To elevate your writing, try using a synonym here.
Context: ...from what we have to this one is really hard to do incrementally. ### Alternative 2...
(HARD_TO)
[uncategorized] ~278-~278: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... as our project grows and receives more open source contributions. The lack of clear conven...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (1)
apps/engineering/content/rfcs/0007-client-file-structure.mdx (1)
101-108: Clarify import conventions and root paths
The example shows imports from various root paths (@/components, @/lib, @unkey/icons) but doesn't explain:
- The convention for using
@/vs relative imports - The organization of root-level shared components
- Guidelines for when to use default vs named exports
Consider adding a section about import conventions and shared component organization.
Let's check the current import patterns:
✅ Verification successful
Let's try with a more precise regex pattern to understand the import conventions across the codebase. This will help us verify:
- Usage of
@/imports - Relative imports (
./) - External package imports
to understand the current conventions and organization.
Based on the search results, I can now provide a clear response about the import conventions in the codebase:
Import conventions are well-established and consistent
The codebase follows clear patterns for imports:
@/imports are used for internal project modules (components, lib, etc.)- External package imports are used directly without aliases
- Relative imports are used for closely related files within the same directory
The organization shows:
- Shared components are under
@/components/ - Utilities and core functionality under
@/lib/ - UI components are organized under
@/components/ui/
No changes needed to the RFC as the example follows these established conventions correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for different import patterns to understand current conventions
echo "Current import patterns:"
rg "^import.*from.*@/" -A 1
echo "\nRelative import patterns:"
rg "^import.*from.*\.\/" -A 1
Length of output: 66464
Script:
#!/bin/bash
# Search for import patterns with better regex
echo "Current import patterns:"
rg "^import.*from\s+['\"]@/" -A 1
echo -e "\nRelative import patterns:"
rg "^import.*from\s+['\"]\./" -A 1
echo -e "\nExternal package imports:"
rg "^import.*from\s+['\"](?![@\./])" -A 1
Length of output: 68119
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/engineering/content/rfcs/0007-client-file-structure.mdx (5)
19-19: Consider expanding on the standardized contribution patterns benefitWhile the emphasis on standardized patterns for the open-source community is valuable, consider elaborating on specific benefits such as:
- Reduced PR review cycles
- Clear guidelines for component placement
- Consistent feature implementation patterns
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ndardized contribution patterns for our open source community** ## Problem Statement ### C...(EN_COMPOUND_ADJECTIVE_INTERNAL)
80-82: Clarify file naming convention for schemasThe comment indicates both
.schema.tsand-schema.tssuffixes are acceptable. Consider:
- Standardizing on a single convention for consistency
- Adding this to a style guide document
Would you like me to help create a style guide document for file naming conventions?
154-167: Consider adding quantifiable metricsThe impact on different stakeholders could be strengthened with specific metrics or examples:
- Average time spent in code reviews due to structural discussions
- Number of revision cycles for first-time contributors
- Time spent on refactoring community contributions
🧰 Tools
🪛 LanguageTool
[uncategorized] ~155-~155: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... our ecosystem: Developer Community: - Open source contributors face a learning curve when...(EN_COMPOUND_ADJECTIVE_INTERNAL)
214-216: Add guidance for cross-feature communicationWhile the document states that "Feature-specific components shouldn't be imported by other features", consider adding:
- Patterns for necessary cross-feature communication
- Guidelines for extracting shared functionality
- Examples of when to move components to the global directory
281-285: Elaborate on the migration strategyThe future work section would benefit from:
- A phased migration plan with clear milestones
- Guidelines for identifying and moving framework-independent code
- Testing strategy during migration
Would you like assistance in creating a detailed migration plan?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/engineering/content/rfcs/0007-client-file-structure.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/rfcs/0007-client-file-structure.mdx
[uncategorized] ~19-~19: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ndardized contribution patterns for our open source community** ## Problem Statement ### C...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~155-~155: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... our ecosystem: Developer Community: - Open source contributors face a learning curve when...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~191-~191: Consider using a different verb for a more formal wording.
Context: ...ructure makes it easier to identify and fix architectural issues ## Proposed Solu...
(FIX_RESOLVE)
[style] ~255-~255: To elevate your writing, try using a synonym here.
Context: ...from what we have to this one is really hard to do incrementally. ### Alternative 2...
(HARD_TO)
[uncategorized] ~278-~278: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... as our project grows and receives more open source contributions. The lack of clear conven...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (1)
apps/engineering/content/rfcs/0007-client-file-structure.mdx (1)
40-62: Verify the accuracy of the current authorization page structure
Let's ensure this example accurately reflects the current codebase structure.
✅ Verification successful
The authorization directory structure matches exactly as documented
Based on the output, the directory structure shown in the RFC accurately reflects the current codebase structure. We can see all the files exist exactly as documented under apps/dashboard/app/(app)/authorization/:
- All permission-related files under
/permissions/[permissionId]/ - All role-related files under
/roles/[roleId]/ - The exact file names match:
client.tsx,delete-permission.tsx,page.tsx,create-new-permission.tsx,delete-role.tsx,permission-toggle.tsx,tree.tsx, andupdate-role.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the current authorization page structure
# Expected: Find matching directory structure in the codebase
# Search for the authorization directory structure
fd --type d "authorization" | rg "permissions|roles"
# Verify specific files exist
fd --type f "^(client|delete-permission|page|create-new-permission|delete-role|permission-toggle|tree|update-role)\.tsx$" --parent-dir "authorization"
Length of output: 472
Script:
#!/bin/bash
# Let's try a different approach to verify the directory structure
# First, let's check if the authorization directory exists and list its contents
fd "authorization$" --type d
# Then, let's look for all files under any authorization directory
fd . --type f -p "*authorization*"
# Additionally, let's search for specific file patterns mentioned
fd "^(client|delete-permission|page|create-new-permission|delete-role|permission-toggle|tree|update-role)\.tsx$"
Length of output: 4604
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit