chore: updated the ssr rendering on sites #6145
Conversation
WalkthroughThe pull request introduces a new API endpoint for accessing project metadata via the URL pattern Changes
Possibly related PRs
Poem
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 (
|
![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
🧹 Outside diff range and nitpick comments (9)
space/core/lib/instance-provider.tsx (1)
Line range hint
22-82: Consider optimizing data fetching for SSR.While the current implementation works, you could potentially improve the SSR experience by:
- Pre-fetching instance info during SSR
- Implementing proper fallback UI during hydration
Consider updating the SWR configuration to include SSR options:
useSWR("INSTANCE_INFO", () => fetchInstanceInfo(), { revalidateOnFocus: false, revalidateIfStale: false, errorRetryCount: 0, + fallbackData: initialData, // Pass from getServerSideProps + suspense: true, });space/app/provider.tsx (1)
13-23: Consider adding error boundariesThe provider hierarchy is well-structured, but consider wrapping the providers with an error boundary to gracefully handle potential runtime errors in the provider chain.
+import { ErrorBoundary } from "@/components/error-boundary"; export const AppProvider: FC<IAppProvider> = (props) => { const { children } = props; return ( + <ErrorBoundary fallback={<div>Something went wrong</div>}> <StoreProvider> <ToastProvider> <InstanceProvider>{children}</InstanceProvider> </ToastProvider> </StoreProvider> + </ErrorBoundary> ); };space/core/components/common/powered-by.tsx (2)
8-8: Review z-index valueThe z-index value of 999999 seems excessive and might cause layering issues with other fixed position elements. Consider using a lower value that still achieves the desired stacking context.
- className="fixed bottom-2.5 right-5 !z-[999999] flex items-center gap-1 rounded border border-custom-border-200 bg-custom-background-100 px-2 py-1 shadow-custom-shadow-2xs" + className="fixed bottom-2.5 right-5 !z-50 flex items-center gap-1 rounded border border-custom-border-200 bg-custom-background-100 px-2 py-1 shadow-custom-shadow-2xs"
13-13: Optimize image loadingConsider adding priority={true} to the Image component since it's above the fold and part of the initial viewport.
- <Image src={planeLogo} alt="Plane logo" className="h-6 w-6" height="24" width="24" /> + <Image src={planeLogo} alt="Plane logo" className="h-6 w-6" height="24" width="24" priority />space/app/issues/[anchor]/client-layout.tsx (2)
24-40: Consider optimizing SWR configuration for better performanceThe SWR implementation could benefit from some optimizations:
- Add revalidation configuration
- Implement error retry strategy
- Consider adding data preloading
const { error } = useSWR( anchor ? `PUBLISH_SETTINGS_${anchor}` : null, anchor ? async () => { const response = await fetchPublishSettings(anchor); if (response.view_props) { updateLayoutOptions({ list: !!response.view_props.list, kanban: !!response.view_props.kanban, calendar: !!response.view_props.calendar, gantt: !!response.view_props.gantt, spreadsheet: !!response.view_props.spreadsheet, }); } } : null, + { + revalidateOnFocus: false, + revalidateOnReconnect: false, + retry: 3, + dedupingInterval: 5000 + } );
47-53: Consider adding aria-labels for accessibilityThe layout structure should include proper accessibility attributes for better screen reader support.
- <div className="relative flex h-screen min-h-[500px] w-screen flex-col overflow-hidden"> + <div className="relative flex h-screen min-h-[500px] w-screen flex-col overflow-hidden" role="main" aria-label="Issues Layout"> - <div className="relative flex h-[60px] flex-shrink-0 select-none items-center border-b border-custom-border-300 bg-custom-sidebar-background-100"> + <div className="relative flex h-[60px] flex-shrink-0 select-none items-center border-b border-custom-border-300 bg-custom-sidebar-background-100" role="navigation" aria-label="Issues Navigation"> <IssuesNavbarRoot publishSettings={publishSettings} /> </div> - <div className="relative h-full w-full overflow-hidden bg-custom-background-90">{children}</div> + <div className="relative h-full w-full overflow-hidden bg-custom-background-90" role="region" aria-label="Issues Content">{children}</div>space/next.config.js (1)
67-71: Consider adding validation for Sentry environment variablesThe Sentry integration could benefit from additional validation to ensure all required environment variables are present.
+const validateSentryConfig = () => { + const required = ['SENTRY_ORG_ID', 'SENTRY_PROJECT_ID', 'SENTRY_AUTH_TOKEN']; + const missing = required.filter(key => !process.env[key]); + if (missing.length > 0) { + console.warn(`Missing required Sentry environment variables: ${missing.join(', ')}`); + return false; + } + return true; +}; -if (parseInt(process.env.SENTRY_MONITORING_ENABLED || "0", 10)) { +if (parseInt(process.env.SENTRY_MONITORING_ENABLED || "0", 10) && validateSentryConfig()) { module.exports = withSentryConfig(nextConfig, sentryConfig); } else { module.exports = nextConfig; }space/app/issues/[anchor]/layout.tsx (2)
17-19: Use server-only environment variables in server-side codeIn server-side code, using
process.env.NEXT_PUBLIC_API_BASE_URLis unnecessary and could inadvertently expose environment variables to the client. Consider using a server-only environment variable without theNEXT_PUBLIC_prefix, such asprocess.env.API_BASE_URL, to ensure sensitive data remains secure.
25-30: Removeasynckeyword fromIssuesLayoutif not neededThe
IssuesLayoutfunction is declared asasyncbut does not use any asynchronous operations inside. If there is no need for asynchronous logic, you can remove theasynckeyword to improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
apiserver/plane/space/urls/project.py(1 hunks)apiserver/plane/space/views/__init__.py(1 hunks)apiserver/plane/space/views/meta.py(1 hunks)space/app/issues/[anchor]/client-layout.tsx(1 hunks)space/app/issues/[anchor]/layout.tsx(2 hunks)space/app/layout.tsx(2 hunks)space/app/provider.tsx(1 hunks)space/core/components/common/index.ts(1 hunks)space/core/components/common/powered-by.tsx(1 hunks)space/core/components/views/auth.tsx(1 hunks)space/core/lib/instance-provider.tsx(1 hunks)space/core/lib/store-provider.tsx(2 hunks)space/helpers/common.helper.ts(0 hunks)space/next.config.js(2 hunks)
💤 Files with no reviewable changes (1)
- space/helpers/common.helper.ts
✅ Files skipped from review due to trivial changes (1)
- space/core/components/common/index.ts
🔇 Additional comments (10)
space/core/components/views/auth.tsx (1)
7-8: Verify the import path configurations for SSR compatibility.
The changes to use absolute imports and the @plane/constants package look good, but let's ensure the setup is SSR-friendly.
Also applies to: 14-15
✅ Verification successful
Let me check the Next.js configurations and workspace setup to better understand the import configuration.
Let me verify the @plane/constants package configuration in the workspace.
Import changes are correctly configured and SSR-compatible
The changes are verified to be working correctly because:
@plane/constantsexists as a workspace package in the monorepo setupSPACE_BASE_PATHis properly defined inpackages/constants/src/endpoints.ts- The import is compatible with Next.js SSR as it's using environment variables and the package is part of the workspace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SSR compatibility of imports
# Check if @plane/constants is configured in the project
cat package.json | jq '.dependencies["@plane/constants"]'
# Verify Next.js configuration for the import aliases
fd -t f -e js -e ts -e json "next.config.*"
Length of output: 171
Script:
#!/bin/bash
# Check space/next.config.js for import alias configuration
cat space/next.config.js
# Check if this is a monorepo setup with workspace packages
cat package.json | jq '.workspaces'
# Check if @plane/constants exists in workspace packages
fd -t f package.json packages/
Length of output: 2723
Script:
#!/bin/bash
# Check the package.json of constants package
cat packages/constants/package.json
# Check if SPACE_BASE_PATH is defined in constants package
rg "SPACE_BASE_PATH" packages/constants/
Length of output: 407
space/core/lib/instance-provider.tsx (1)
9-10: LGTM! Import changes are consistent with the codebase.
The changes align well with the auth.tsx updates and maintain good organization.
Also applies to: 17-20
space/app/provider.tsx (1)
1-1: LGTM: Proper use of client directive
The "use client" directive is correctly placed at the top of the file, indicating this component will be rendered on the client side, which is necessary for context providers.
space/core/lib/store-provider.tsx (2)
Line range hint 12-18: LGTM: Proper singleton store initialization
The store initialization logic correctly handles both client and server-side scenarios while maintaining the singleton pattern.
27-33: 🛠️ Refactor suggestion
Improve type safety for initialState
The current typing of initialState as any reduces type safety. Consider creating a specific type for the initial state structure.
+interface StoreInitialState {
+ // Define your store state structure here
+ // Example:
+ // user?: UserState;
+ // settings?: SettingsState;
+}
export type StoreProviderProps = {
children: ReactNode;
- initialState?: any;
+ initialState?: Partial<StoreInitialState>;
};Also, verify that the hydration logic handles partial state correctly:
✅ Verification successful
Based on the shell script results, I can now provide a final response. The codebase shows that the store follows a specific structure where each store has its own interface defining the state and actions. Looking at the hydration implementation, particularly in space/core/store/root.store.ts and other store files, I can see that the hydration logic is properly typed for each store.
Type safety for initialState should match RootStore structure
The initialState should match the root store's structure. Here's the corrected suggestion:
+interface StoreInitialState {
+ instance?: {
+ instance?: IInstance;
+ config?: IInstanceConfig;
+ };
+ user?: {
+ data?: IUser;
+ };
}
export type StoreProviderProps = {
children: ReactNode;
- initialState?: any;
+ initialState?: StoreInitialState;
};The hydration logic in the root store properly handles partial state through null checks and optional chaining, so no changes are needed there.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for store hydration usage patterns
ast-grep --pattern 'store.hydrate($_)'
# Look for potential hydration-related tests
rg -l "hydrate.*test" --type ts
Length of output: 144
Script:
#!/bin/bash
# Let's check the store implementation to understand the hydration logic
rg -A 10 "hydrate" --type ts
# Find store-related test files
fd "store.*test" --type f
# Look for store type definitions
rg -A 5 "interface.*Store|type.*Store" --type ts
# Check store initialization
ast-grep --pattern 'initializeStore'
Length of output: 65713
space/app/layout.tsx (2)
3-3: Verify SPACE_BASE_PATH configuration across environments
The switch from ASSET_PREFIX to SPACE_BASE_PATH for asset paths is good for SSR compatibility. However, we should ensure this constant is properly configured across all environments.
Also applies to: 28-32
✅ Verification successful
SPACE_BASE_PATH is properly configured across environments
The constant is consistently configured:
- Defined in
packages/constants/src/endpoints.tsas a central source - Properly set in all Docker environments with default value "/spaces"
- Correctly used in Next.js configuration via
next.config.js - No conflicting ASSET_PREFIX usage in the space app (only found in admin app where it's intentionally different)
- Consistently used for asset paths across the space application components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if SPACE_BASE_PATH is consistently used across the codebase
rg "SPACE_BASE_PATH" --type ts --type tsx
# Verify no remaining ASSET_PREFIX usage
rg "ASSET_PREFIX" --type ts --type tsx
Length of output: 136
Script:
#!/bin/bash
# Check if SPACE_BASE_PATH is consistently used across the codebase
rg "SPACE_BASE_PATH"
# Verify no remaining ASSET_PREFIX usage
rg "ASSET_PREFIX"
Length of output: 4622
7-7: Verify AppProvider implementation includes all necessary contexts
The consolidation of providers into AppProvider improves maintainability. However, we should verify that all necessary contexts from the removed providers (StoreProvider, ToastProvider, InstanceProvider) are included in the new AppProvider.
Also applies to: 35-35
✅ Verification successful
AppProvider implementation includes all necessary contexts and additional improvements
The new AppProvider in space/app/provider.tsx includes all the required providers and even adds more functionality:
- StoreProvider (MobX store context)
- ThemeProvider (theme management)
- Toast notifications
- Progress bar
- SWR configuration
- Instance wrapper
- Intercom integration
- PostHog analytics
- Polyfills
The implementation not only maintains but enhances the functionality of the removed providers by consolidating them into a more organized structure with additional features like analytics, intercom support, and proper theme management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check AppProvider implementation
ast-grep --pattern 'export const AppProvider = ({ children }: { children: React.ReactNode }) => {
$$$
}'
Length of output: 108
Script:
#!/bin/bash
# Let's try to find the AppProvider implementation with a more flexible pattern
ast-grep --pattern 'export $_ AppProvider = $_'
# Also search for the file content directly
rg -l "AppProvider"
# And check the specific provider file content
fd "provider.tsx" --exec cat {}
Length of output: 15715
space/next.config.js (1)
Line range hint 27-64: Review Sentry configuration security settings
The Sentry configuration includes sensitive information. Consider:
- Moving sensitive values to environment variables
- Reviewing the tunnelRoute setting for potential conflicts
- Verifying the security implications of widenClientFileUpload
✅ Verification successful
Based on the search results, I can generate the final response since I have enough information to verify the security concerns raised in the review comment:
Sentry configuration security review completed
The configuration is secure and follows best practices:
- All sensitive values including
authTokenare properly configured through environment variables - The
/monitoringtunnelRoute does not have any conflicts in the codebase - The
widenClientFileUploadsetting is a standard Sentry configuration for improved debugging and does not pose security risks when used with proper source map settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any hardcoded sensitive values in Sentry config
rg -i "auth.*token|secret|key" --type js --type ts
# Check for potential route conflicts
rg "tunnelRoute|/monitoring" --type ts --type js
Length of output: 66099
apiserver/plane/space/views/__init__.py (1)
29-29: Import statement added correctly
The ProjectMetaDataEndpoint has been successfully imported from the .meta module, making it available for use within the application.
apiserver/plane/space/urls/project.py (1)
13-13: New API endpoint added correctly to URL patterns
The ProjectMetaDataEndpoint has been properly added to the URL patterns, allowing access to project metadata via the specified anchor parameter.
Also applies to: 17-21
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (17)
space/core/services/module.service.ts (1)
Line range hint
10-17: Consider enhancing error handling verbosity.While the current error handling aligns with the standardization effort, consider adding more detailed error information for debugging purposes.
Here's a suggested enhancement:
async getModules(anchor: string): Promise<TPublicModule[]> { return this.get(`/api/public/anchor/${anchor}/modules/`) .then((response) => response?.data) .catch((error) => { - throw error?.response?.data; + throw { + message: `Failed to fetch modules for anchor: ${anchor}`, + status: error?.response?.status, + details: error?.response?.data + }; }); }space/core/services/cycle.service.ts (1)
1-5: Consider standardizing import path prefixes.The file mixes different import path prefixes:
- @plane/constants
- @/services
- @/types
Consider standardizing these for better maintainability.
space/core/services/member.service.ts (2)
Line range hint
11-18: Consider centralizing error handling in APIService base class.The error handling pattern is duplicated across all services:
.catch((error) => { throw error?.response?.data; });Consider moving this to the base APIService class to promote DRY principles.
Line range hint
1-18: LGTM! Service implementation maintains architectural consistency.The MemberService follows the established patterns:
- Extends APIService
- Uses typed responses (TPublicMember[])
- Follows consistent endpoint structure
- Maintains proper error handling
The consistent service architecture across label, cycle, and member services promotes maintainability and predictability. Consider documenting these patterns in a service architecture guide.
space/core/components/common/powered-by.tsx (1)
24-26: Optimize image loading and accessibilityThe Image component could be optimized:
- Add priority prop since this is likely above the fold
- Add explicit alt text instead of "Plane logo"
<div className="relative grid h-6 w-6 place-items-center"> - <Image src={planeLogo} alt="Plane logo" className="h-6 w-6" height="24" width="24" /> + <Image + src={planeLogo} + alt="Plane - Modern project management tool" + className="h-6 w-6" + height="24" + width="24" + priority + /> </div>space/core/services/auth.service.ts (1)
Line range hint
11-24: Standardize error handling across methodsThe error handling patterns are inconsistent across methods:
requestCSRFTokenthrows raw erroremailCheckandgenerateUniqueCodethrow error?.response?.dataThis inconsistency could lead to unpredictable error handling in consuming code.
Consider standardizing to a consistent pattern:
async requestCSRFToken(): Promise<ICsrfTokenData> { return this.get("/auth/get-csrf-token/") .then((response) => response.data) .catch((error) => { - throw error; + throw error?.response?.data; }); }space/core/services/user.service.ts (1)
Line range hint
11-39: Standardize error handling across user methodsThe error handling is inconsistent across methods:
currentUserandgetCurrentUserProfilethrowerror?.responseupdateUserthrowserror?.response?.dataupdateCurrentUserProfilethrowserror?.responseSimilar operations (profile-related methods) should handle errors consistently.
Consider standardizing the error handling:
async updateUser(data: Partial<IUser>): Promise<IUser> { return this.patch("/api/users/me/", data) .then((response) => response?.data) .catch((error) => { - throw error?.response?.data; + throw error?.response; }); }space/core/components/account/oauth/gitlab-button.tsx (1)
Line range hint
21-23: Improve URL construction and navigation handlingThe current implementation directly manipulates window.location and uses string concatenation for URL construction.
Consider using the URL API for safer URL construction and Next.js router for navigation:
const handleSignIn = () => { - window.location.assign(`${API_BASE_URL}/auth/spaces/gitlab/${nextPath ? `?next_path=${nextPath}` : ``}`); + const url = new URL('/auth/spaces/gitlab', API_BASE_URL); + if (nextPath) url.searchParams.set('next_path', nextPath); + window.location.assign(url.toString()); };space/helpers/file.helper.ts (1)
2-2: LGTM! Consider enhancing URL validationThe import path change is consistent with the codebase-wide refactoring. While the implementation is sound, consider enhancing the URL validation in
getFileURLfunction to use a more robust check for valid URLs.export const getFileURL = (path: string): string | undefined => { if (!path) return undefined; - const isValidURL = path.startsWith("http"); + const isValidURL = path.startsWith("http://") || path.startsWith("https://"); if (isValidURL) return path; return `${API_BASE_URL}${path}`; };space/core/services/file.service.ts (1)
Line range hint
22-28: Consider standardizing error handling across service methodsThe current error handling pattern could be improved for better error propagation and debugging.
Consider implementing a centralized error handling utility:
// error.utils.ts export const handleServiceError = (error: any) => { const errorData = error?.response?.data; if (!errorData) throw new Error('Unknown error occurred'); throw errorData; }; // Usage in service methods async updateAssetUploadStatus(anchor: string, assetId: string): Promise<void> { return this.patch(`/api/public/assets/v2/anchor/${anchor}/${assetId}/`) .then((response) => response?.data) .catch(handleServiceError); }Also applies to: 33-39, 43-57, 60-66, 69-75, 78-84, 87-93
space/core/services/issue.service.ts (1)
Line range hint
11-93: Consider implementing response type safetyThe service methods could benefit from stronger type safety for API responses.
Consider defining explicit response types:
interface IssueVoteResponse { id: string; user_id: string; created_at: string; // ... other fields } async getIssueVotes(anchor: string, issueID: string): Promise<IssueVoteResponse[]> { return this.get(`/api/public/anchor/${anchor}/issues/${issueID}/votes/`) .then((response) => response?.data) .catch((error) => { throw error?.response; }); }space/core/components/issues/navbar/user-avatar.tsx (2)
Line range hint
39-42: Consider implementing CSRF token error handlingThe CSRF token request lacks error handling and retry logic.
Consider implementing error handling and retry mechanism:
useEffect(() => { const fetchCSRFToken = async () => { try { const data = await authService.requestCSRFToken(); if (data?.csrf_token) setCsrfToken(data.csrf_token); } catch (error) { console.error('Failed to fetch CSRF token:', error); // Implement retry logic or user notification } }; if (csrfToken === undefined) fetchCSRFToken(); }, [csrfToken]);
Line range hint
95-107: Enhance accessibility for sign-out functionalityThe sign-out button could benefit from improved accessibility attributes.
Consider adding ARIA attributes and keyboard navigation:
<button type="submit" + aria-label="Sign out" + role="menuitem" className="flex items-center gap-2 rounded p-2 whitespace-nowrap hover:bg-custom-background-80 text-sm min-w-36 cursor-pointer" > <LogOut size={12} className="flex-shrink-0 text-red-500" /> <div>Sign out</div> </button>space/core/components/account/auth-forms/unique-code.tsx (2)
Line range hint
89-93: Consider enhancing form error handlingThe form's error handling could be improved to provide better user feedback.
Consider adding error state management and user notification:
- onError={() => setIsSubmitting(false)} + onError={(error) => { + setIsSubmitting(false); + // Add error notification/message state + setErrorMessage(error.message); + }}
Line range hint
142-146: Improve accessibility for password strength indicatorThe password strength indicator message should be more accessible.
Add ARIA attributes for screen readers:
<p className="flex items-center gap-1 font-medium text-green-700"> <CircleCheck height={12} width={12} /> - Paste the code sent to your email + <span role="status" aria-live="polite"> + Paste the code sent to your email + </span> </p>space/core/components/account/auth-forms/password.tsx (2)
Line range hint
133-142: Enhance CSRF token handling securityThe current CSRF token handling could be more robust.
Consider adding validation and error handling:
const handleCSRFToken = async () => { if (!formRef || !formRef.current) return; const token = await csrfPromise; - if (!token?.csrf_token) return; + if (!token?.csrf_token) { + throw new Error('Failed to obtain CSRF token'); + } const csrfElement = formRef.current.querySelector("input[name=csrfmiddlewaretoken]"); + if (!csrfElement) { + throw new Error('CSRF token element not found'); + } csrfElement?.setAttribute("value", token?.csrf_token); };
Line range hint
82-89: Optimize password strength validationThe password strength validation logic could be memoized for better performance.
Consider using useMemo for the validation:
+ const passwordStrength = useMemo( + () => getPasswordStrength(passwordFormData.password), + [passwordFormData.password] + ); const passwordSupport = passwordFormData.password.length > 0 && mode === EAuthModes.SIGN_UP && - getPasswordStrength(passwordFormData.password) != E_PASSWORD_STRENGTH.STRENGTH_VALID + passwordStrength != E_PASSWORD_STRENGTH.STRENGTH_VALID
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (24)
space/app/issues/[anchor]/client-layout.tsx(1 hunks)space/app/views/[anchor]/layout.tsx(0 hunks)space/core/components/account/auth-forms/password.tsx(1 hunks)space/core/components/account/auth-forms/unique-code.tsx(1 hunks)space/core/components/account/oauth/github-button.tsx(1 hunks)space/core/components/account/oauth/gitlab-button.tsx(1 hunks)space/core/components/account/oauth/google-button.tsx(1 hunks)space/core/components/common/powered-by.tsx(1 hunks)space/core/components/issues/navbar/user-avatar.tsx(1 hunks)space/core/lib/instance-provider.tsx(2 hunks)space/core/services/auth.service.ts(1 hunks)space/core/services/cycle.service.ts(1 hunks)space/core/services/file.service.ts(1 hunks)space/core/services/instance.service.ts(1 hunks)space/core/services/issue.service.ts(1 hunks)space/core/services/label.service.ts(1 hunks)space/core/services/member.service.ts(1 hunks)space/core/services/module.service.ts(1 hunks)space/core/services/project-member.service.ts(1 hunks)space/core/services/publish.service.ts(1 hunks)space/core/services/state.service.ts(1 hunks)space/core/services/user.service.ts(1 hunks)space/helpers/common.helper.ts(0 hunks)space/helpers/file.helper.ts(1 hunks)
💤 Files with no reviewable changes (2)
- space/app/views/[anchor]/layout.tsx
- space/helpers/common.helper.ts
✅ Files skipped from review due to trivial changes (2)
- space/core/services/instance.service.ts
- space/core/services/state.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- space/app/issues/[anchor]/client-layout.tsx
- space/core/lib/instance-provider.tsx
🔇 Additional comments (20)
space/core/services/module.service.ts (2)
Line range hint 1-8: LGTM! Import standardization looks good.
The updated import path for API_BASE_URL aligns with the standardization effort across service files, maintaining consistency in the codebase.
Line range hint 10-17: Verify the consistency of public API endpoint usage.
Let's ensure this public endpoint is consistently used across the codebase.
✅ Verification successful
The public API endpoint is used consistently and exclusively in the module service
Based on the search results, the /api/public/anchor/${anchor}/modules/ endpoint is only used in space/core/services/module.service.ts and there are no other variations or duplicate implementations of this endpoint across the codebase. The endpoint is properly encapsulated within the module service and follows a consistent pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the public modules endpoint
# Search for any variations of the modules endpoint usage
rg -l "anchor/.+/modules" --type ts --type tsx
# Check for any potential duplicate implementations
ast-grep --pattern 'get(`/api/public/anchor/${$_}/modules/`)'
Length of output: 138
Script:
#!/bin/bash
# Search for any variations of the modules endpoint usage with proper escaping and file type
rg -l "anchor/.*/modules" --type ts
# Check for any API calls related to modules
rg -A 2 "modules" --type ts
# Look for similar service patterns
ast-grep --pattern 'getModules($$$) {
$$$
}'
Length of output: 62310
space/core/services/label.service.ts (2)
1-2: LGTM! Import refactoring aligns with centralized constants management.
The change to import API_BASE_URL from @plane/constants instead of helpers follows good practices by centralizing configuration constants.
Line range hint 11-18: Verify API endpoint pattern consistency across services.
The endpoint pattern /api/public/anchor/${anchor}/labels/ should be consistent with other public anchor endpoints.
✅ Verification successful
API endpoint pattern /api/public/anchor/${anchor}/labels/ follows consistent structure with other endpoints
The endpoint follows the consistent pattern used across the codebase:
- All endpoints start with
/api/public/anchor/${anchor}/ - Resource identifiers are in plural form (e.g.,
labels,issues,comments,reactions) - Nested resources use appropriate ID parameters (e.g.,
issueID,commentId) - The
labelsendpoint follows the same pattern as other collection endpoints likemembers,modules,states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistency of public anchor API endpoints across the codebase
# Search for all public anchor API endpoint patterns
rg -g '*.ts' -g '*.js' '/api/public/anchor/.*/' --no-filename | sort | uniq
Length of output: 1641
space/core/services/cycle.service.ts (1)
Line range hint 11-18: LGTM! Well-typed API response handling.
The use of TPublicCycle[] type ensures type safety for the API response data.
space/core/services/project-member.service.ts (1)
2-2: Verify the constant's availability in the new location
The import path for API_BASE_URL has been updated. While this aligns with the broader refactoring effort, we should ensure the constant is properly exported from @plane/constants.
✅ Verification successful
Import path change is consistent with the codebase structure
The constant API_BASE_URL is properly exported from packages/constants/src/endpoints.ts and multiple services in the space directory are consistently using the new import path @plane/constants. This change aligns with the codebase's structure and appears to be part of a successful refactoring effort.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify API_BASE_URL export in @plane/constants
# Expected: Find the export statement for API_BASE_URL
# Search for the API_BASE_URL export
rg -t ts -t js "export.*API_BASE_URL"
# Verify all other services are using the same import path
rg -t ts "import.*API_BASE_URL.*from.*@plane/constants"
Length of output: 2249
space/core/services/publish.service.ts (1)
2-2: LGTM! Consistent with the codebase-wide refactoring
The import path update aligns with the changes made in other service files, maintaining consistency across the codebase.
space/core/components/common/powered-by.tsx (1)
20-20: Review z-index value for potential conflicts
The component uses a very high z-index value (!z-[999999]). This might cause issues with other fixed position elements or modals.
✅ Verification successful
Let me try a different approach to find z-index usage in the codebase. The previous command had an issue with the file type. This new command should help us identify potential z-index conflicts by:
- Finding Tailwind z-index utilities like
z-[number] - Finding standard Tailwind z-index classes like
z-10,z-20 - Finding CSS z-index properties
Let me try without specifying file types, as it seems the sandbox environment might have different file type configurations. This command should help us find:
- Tailwind z-index utilities with square brackets
- CSS z-index properties
- Any other z-index related patterns
High z-index value is appropriate for this component
The powered-by component's z-index of 999999 is appropriate for its use case. Here's why:
- It's a fixed position component that needs to stay above all other elements
- Other high z-index values in the codebase are lower:
- Bubble menus and selectors: z-[99999]
- Dropdown menus: z-[21]
- Headers and navigation: z-[15] to z-[18]
- Content layers: z-[1] to z-[5]
The powered-by component is intentionally designed to be the topmost element, and its z-index doesn't conflict with other UI components' layering hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other high z-index values that might conflict
# Expected: Find potential z-index conflicts
# Search for z-index declarations in CSS/SCSS/TSX files
rg -t css -t scss -t tsx "z-\[.*\]|z-index"
Length of output: 74
Script:
#!/bin/bash
# Search for z-index related classes and inline styles in tsx/jsx/css files
rg -t tsx -t jsx -t css "z-\[.*?\]|\bz-[0-9]+\b|z-index"
Length of output: 86
Script:
#!/bin/bash
# Search for z-index patterns without specifying file types, letting ripgrep auto-detect
rg "z-\[[0-9]+\]|z-index:|z-\[.*?\]"
Length of output: 18769
space/core/services/auth.service.ts (1)
1-1: LGTM: Import path update aligns with codebase refactoring
The change to import API_BASE_URL from "@plane/constants" is consistent with the broader refactoring effort across the codebase.
space/core/services/user.service.ts (1)
2-2: LGTM: Import path update is consistent
The change to import API_BASE_URL from "@plane/constants" aligns with the codebase-wide refactoring.
space/core/components/account/oauth/gitlab-button.tsx (1)
5-5: LGTM: Import path update matches pattern
The change to import API_BASE_URL from "@plane/constants" is consistent with the codebase refactoring.
space/core/components/account/oauth/google-button.tsx (1)
5-5: Verify constant availability across builds
The import path change from @/helpers/common.helper to @plane/constants appears to be part of a broader refactoring effort. The change itself is clean and aligns with the codebase's direction.
space/core/components/account/oauth/github-button.tsx (1)
5-5: LGTM! Consistent with other OAuth components
The import path change maintains consistency with other OAuth components and follows the same pattern.
space/core/services/file.service.ts (2)
2-2: LGTM: Import refactoring aligns with codebase standards
The change to import API_BASE_URL from @plane/constants is consistent with the broader refactoring effort across the codebase.
Line range hint 43-57: Verify file upload security measures
The file upload implementation should be reviewed for potential security vulnerabilities.
Run the following script to check for file upload security measures:
✅ Verification successful
Let me gather more information about the file upload security implementation.
Based on the results, I can now provide a comprehensive response about the file upload security measures.
File upload security measures are properly implemented
The codebase has multiple security measures in place for file uploads:
- File type validation:
- Server-side validation enforces allowed MIME types (
image/jpeg,image/png,image/webp,image/jpg,image/gif) - Client-side validation through
isFileValidchecks for allowed image types
- Server-side validation enforces allowed MIME types (
- File size restrictions:
- Enforced through
maxFileSizeconfiguration - Size limits are checked both client-side and server-side
- User-friendly error messages display size limits in MB
- Enforced through
- Upload handling:
- Uses signed URLs for secure upload paths
- Implements proper error handling and status updates
- Validates upload status through
updateAssetUploadStatus
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for file type validation and size limits
rg -A 5 "upload.*file|file.*upload" --type typescript
# Check for file extension validation
ast-grep --pattern 'getFileMetaDataForUpload($_)'
Length of output: 634
Script:
#!/bin/bash
# Search for the implementation of getFileMetaDataForUpload
ast-grep --pattern 'function getFileMetaDataForUpload($_) { $$$ }'
# Search for file type and size validation patterns
rg -A 10 "file\.(size|type)" --type-add 'typescript:*.ts'
# Look for file extension validation
rg -A 5 "\.extension|mimetype|mime.*type" --type-add 'typescript:*.ts'
# Check for security-related configurations
rg -A 5 "maxFileSize|allowedTypes|allowed.*types" --type-add 'typescript:*.ts'
Length of output: 38617
space/core/services/issue.service.ts (1)
1-1: LGTM: Import refactoring aligns with codebase standards
The change to import API_BASE_URL from @plane/constants is consistent with the broader refactoring effort.
space/core/components/issues/navbar/user-avatar.tsx (1)
11-11: LGTM: Import refactoring aligns with codebase standards
The change to import API_BASE_URL from @plane/constants is consistent with the broader refactoring effort.
space/core/components/account/auth-forms/unique-code.tsx (1)
6-6: LGTM: Import standardization
The import of API_BASE_URL from @plane/constants aligns with the PR's objective of standardizing imports across the application.
space/core/components/account/auth-forms/password.tsx (2)
7-7: LGTM: Consistent import pattern
The import of API_BASE_URL from @plane/constants maintains consistency with other files.
Line range hint 91-100: LGTM: Efficient button state management
The use of useMemo for button disabled state is a good optimization to prevent unnecessary re-renders.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/constants/src/endpoints.ts (1)
16-18: Consider documenting the constant and centralizing the default URLWhile the constant follows the established pattern, consider:
- Adding a JSDoc comment explaining its purpose and usage
- Moving the default URL to a centralized defaults configuration file to maintain consistency if the website URL changes
// plane website url +/** + * The main Plane website URL used for branding and external links + * @default https://plane.so + */ export const WEBSITE_URL = - process.env.NEXT_PUBLIC_WEBSITE_URL || "https://plane.so"; + process.env.NEXT_PUBLIC_WEBSITE_URL || DEFAULT_WEBSITE_URL;space/core/components/common/powered-by.tsx (1)
26-28: Consider optimizing the image loadingThe image is wrapped in a relative/grid container which might be unnecessary. Also, consider adding loading="lazy" since this is a non-critical UI element.
- <div className="relative grid h-6 w-6 place-items-center"> <Image src={planeLogo} alt="Plane logo" className="h-6 w-6" height="24" width="24" + loading="lazy" /> - </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
apiserver/plane/space/views/meta.py(1 hunks)packages/constants/src/endpoints.ts(1 hunks)space/app/views/[anchor]/layout.tsx(0 hunks)space/core/components/account/auth-forms/password.tsx(1 hunks)space/core/components/account/auth-forms/unique-code.tsx(1 hunks)space/core/components/common/powered-by.tsx(1 hunks)space/core/components/issues/navbar/user-avatar.tsx(1 hunks)space/core/components/views/auth.tsx(1 hunks)space/core/lib/instance-provider.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- space/app/views/[anchor]/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- space/core/components/issues/navbar/user-avatar.tsx
- apiserver/plane/space/views/meta.py
- space/core/lib/instance-provider.tsx
- space/core/components/account/auth-forms/password.tsx
- space/core/components/views/auth.tsx
- space/core/components/account/auth-forms/unique-code.tsx
🔇 Additional comments (2)
space/core/components/common/powered-by.tsx (2)
1-1: CSP headers missing for external resource protection
The Next.js configuration files show that while there are some security headers configured (X-Frame-Options), there are no Content Security Policy (CSP) headers defined to protect against potentially malicious external resources.
20-25: 🛠️ Refactor suggestion
Enhance accessibility of the external link
The link should have an aria-label for better screen reader support.
<a
href={WEBSITE_URL}
+ aria-label="Visit Plane website - opens in new tab"
className="fixed bottom-2.5 right-5 !z-[999999] flex items-center gap-1 rounded border border-custom-border-200 bg-custom-background-100 px-2 py-1 shadow-custom-shadow-2xs"
target="_blank"
rel="noreferrer noopener"Likely invalid or redundant comment.
Summary by CodeRabbit
New Features
IssuesClientLayoutcomponent for managing issue-related views.PoweredBycomponent linking to the Plane website.WEBSITE_URLfor the Plane website.Refactor
IssuesLayoutfrom a client-side to a server-side component for improved performance.Bug Fixes
Documentation