chore: merge helpers and layouts#8624
Conversation
📝 WalkthroughWalkthroughConsolidates CE helpers and wrappers into core layouts, replaces helper-based workspace-creation checks with a config-backed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/issues/issue-layouts/filters/header/display-filters/display-properties.tsx (1)
69-87:⚠️ Potential issue | 🟡 Minor
keyshould be on the outermost element returned from.map(), not the inner<button>.The Fragment (
<>) is the top-level element returned from the.map()callback, butkeyis placed on the child<button>. React will warn about missing keys. Since the Fragment serves no purpose here, remove it and return the<button>directly.Proposed fix
{filteredDisplayProperties.map((displayProperty) => ( - <> - <button - key={displayProperty.key} - type="button" - className={`rounded-sm border px-2 py-0.5 text-11 transition-all ${ - displayProperties?.[displayProperty.key] - ? "border-accent-strong bg-accent-primary text-on-color" - : "border-subtle hover:bg-layer-1" - }`} - onClick={() => - handleUpdate({ - [displayProperty.key]: !displayProperties?.[displayProperty.key], - }) - } - > - {t(displayProperty.titleTranslationKey)} - </button> - </> + <button + key={displayProperty.key} + type="button" + className={`rounded-sm border px-2 py-0.5 text-11 transition-all ${ + displayProperties?.[displayProperty.key] + ? "border-accent-strong bg-accent-primary text-on-color" + : "border-subtle hover:bg-layer-1" + }`} + onClick={() => + handleUpdate({ + [displayProperty.key]: !displayProperties?.[displayProperty.key], + }) + } + > + {t(displayProperty.titleTranslationKey)} + </button> ))}
🤖 Fix all issues with AI agents
In `@apps/web/core/components/onboarding/create-or-join-workspaces.tsx`:
- Line 42: The local flag is inverted: isWorkspaceCreationEnabled is set to
config?.is_workspace_creation_disabled which causes CreateWorkspace to render
when creation is actually disabled; change the assignment to invert the boolean
so it reflects "enabled" (e.g., set isWorkspaceCreationEnabled to the negation
of config?.is_workspace_creation_disabled ?? false) and keep the existing
ternary that renders <CreateWorkspace> when isWorkspaceCreationEnabled is true
and the restriction/notice component when false; update references to
isWorkspaceCreationEnabled, config?.is_workspace_creation_disabled, and the
CreateWorkspace/notice render branch accordingly.
- Line 35: Remove the unused totalSteps property from the Props type for the
CreateOrJoinWorkspaces component: update the Props type declaration to no longer
include totalSteps and ensure the component signature and destructuring (const {
invitations, stepChange, finishOnboarding } = props) remain unchanged; verify no
other references to totalSteps exist in CreateOrJoinWorkspaces (or its exports)
so callers aren't affected and TypeScript errors are avoided.
In `@apps/web/core/components/onboarding/steps/workspace/create.tsx`:
- Line 54: The boolean is inverted: change how isWorkspaceCreationEnabled is
derived so it reflects creation being allowed (i.e., true when
config.is_workspace_creation_disabled is false). Update the assignment for
isWorkspaceCreationEnabled to negate config?.is_workspace_creation_disabled (or
rename to isWorkspaceCreationDisabled and adjust usage), and then leave the
render check that currently uses if (!isWorkspaceCreationEnabled) unchanged so
the restriction notice only shows when creation is actually disabled; reference
the variable isWorkspaceCreationEnabled and the config property
is_workspace_creation_disabled as the loci to fix.
In `@apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx`:
- Line 46: The boolean is inverted: isWorkspaceCreationEnabled is set to
config?.is_workspace_creation_disabled (no negation), so when admins disable
creation the UI still shows the "Create workspace" link; change the assignment
of isWorkspaceCreationEnabled to the negation of
config?.is_workspace_creation_disabled (e.g.,
!config?.is_workspace_creation_disabled ?? true/false as appropriate) so the
"Create workspace" link rendering logic (the "Create workspace" link branch)
correctly hides when disabled and shows when enabled.
🧹 Nitpick comments (4)
apps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx (1)
11-13: Nit: section comment is slightly misleading.The
// componentscomment on line 11 now groups both a component import (SPREADSHEET_COLUMNS) and a helper utility (shouldRenderColumn). Consider adding a// helperscomment before line 13 for clarity, or moving it above the component import.apps/web/helpers/views.helper.ts (1)
18-26: Formatting change looks fine; minor pre-existing type-safety note on the cast.The only change in this file is a blank-line formatting fix (line 18), which is fine.
As a small optional note on the pre-existing code: the
as keyof typeof VIEW_ACCESS_ICONScast on line 25 silently suppresses type errors ifEViewAccessever gains a new member beyondPUBLIC/PRIVATE. The result would beicon: undefinedat runtime while the type still claims a valid icon. A safer approach would be to typeVIEW_ACCESS_ICONSasRecord<EViewAccess, ...>so the compiler forces you to add icons for new members.♻️ Optional: tighten the icon map type
-const VIEW_ACCESS_ICONS = { +const VIEW_ACCESS_ICONS: Record<EViewAccess, LucideIcon | React.FC<ISvgIcons>> = { [EViewAccess.PUBLIC]: GlobeIcon, [EViewAccess.PRIVATE]: LockIcon, };This would also let you drop the
ascast on line 25:- icon: VIEW_ACCESS_ICONS[option.key as keyof typeof VIEW_ACCESS_ICONS], + icon: VIEW_ACCESS_ICONS[option.key],apps/web/app/(all)/[workspaceSlug]/layout.tsx (1)
7-13: Nit: import grouping is inconsistent with other files in this PR.Other files in this PR place the
@/layouts/…import under a// layoutscomment group, separate from@/plane-webimports. Here it's interleaved without a section comment. Consider reordering for consistency:Suggested grouping
import { Outlet } from "react-router"; import { AuthenticationWrapper } from "@/lib/wrappers/authentication-wrapper"; +// layouts +import { WorkspaceAuthWrapper } from "@/layouts/auth-layout/workspace-wrapper"; +// plane web imports import { WorkspaceContentWrapper } from "@/plane-web/components/workspace/content-wrapper"; import { AppRailVisibilityProvider } from "@/plane-web/hooks/app-rail"; import { GlobalModals } from "@/plane-web/components/common/modal/global"; -import { WorkspaceAuthWrapper } from "@/layouts/auth-layout/workspace-wrapper"; import type { Route } from "./+types/layout";apps/web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/layout.tsx (1)
13-17: Misplaced import:ProjectSettingsSidebarRootsits after the// typescomment.
ProjectSettingsSidebarRootis a component import but it's listed after the// typessection on line 17. It should be grouped with the other component imports (lines 10-12).Suggested fix
// components import { getProjectActivePath } from "@/components/settings/helper"; import { SettingsMobileNav } from "@/components/settings/mobile/nav"; +import { ProjectSettingsSidebarRoot } from "@/components/settings/project/sidebar"; // layouts import { ProjectAuthWrapper } from "@/layouts/auth-layout/project-wrapper"; // types import type { Route } from "./+types/layout"; -import { ProjectSettingsSidebarRoot } from "@/components/settings/project/sidebar";
|
|
||
| export const CreateOrJoinWorkspaces = observer(function CreateOrJoinWorkspaces(props: Props) { | ||
| const { invitations, totalSteps, stepChange, finishOnboarding } = props; | ||
| const { invitations, stepChange, finishOnboarding } = props; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the target file to see the current state
cat -n apps/web/core/components/onboarding/create-or-join-workspaces.tsx | head -50Repository: makeplane/plane
Length of output: 2170
🏁 Script executed:
# Check if totalSteps is still passed to CreateOrJoinWorkspaces anywhere
rg -n 'totalSteps' --type=tsx --type=ts -g '!**/create-or-join-workspaces.tsx'Repository: makeplane/plane
Length of output: 86
🏁 Script executed:
# Also search within the file itself to see if totalSteps is referenced anywhere
rg -n 'totalSteps' apps/web/core/components/onboarding/create-or-join-workspaces.tsxRepository: makeplane/plane
Length of output: 83
🏁 Script executed:
# Find where CreateOrJoinWorkspaces is imported and used
rg -n 'CreateOrJoinWorkspaces' --type ts --type tsxRepository: makeplane/plane
Length of output: 86
🏁 Script executed:
# Alternative search without type specification
rg -n '<CreateOrJoinWorkspaces'Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
# Search for props being passed to CreateOrJoinWorkspaces
rg -n 'totalSteps.*=' --type ts --type tsx -B 3 -A 3Repository: makeplane/plane
Length of output: 86
🏁 Script executed:
# Search for CreateOrJoinWorkspaces usage without type flags
rg 'CreateOrJoinWorkspaces' apps/web/core/components/onboarding/Repository: makeplane/plane
Length of output: 219
🏁 Script executed:
# Search for totalSteps in the onboarding directory
rg 'totalSteps' apps/web/core/components/onboarding/Repository: makeplane/plane
Length of output: 2120
🏁 Script executed:
# Find parent component that uses CreateOrJoinWorkspaces
fd -e tsx -e ts apps/web/core/components/onboarding/ | xargs grep -l 'CreateOrJoinWorkspaces'Repository: makeplane/plane
Length of output: 510
🏁 Script executed:
# Search for imports of CreateOrJoinWorkspaces across entire repo
rg 'import.*CreateOrJoinWorkspaces'Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
# Search for component usage (opening tag)
rg '<CreateOrJoinWorkspaces'Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
# Look for files that reference this component
rg 'CreateOrJoinWorkspaces' apps/web/Repository: makeplane/plane
Length of output: 219
🏁 Script executed:
# Search entire repository for CreateOrJoinWorkspaces
rg 'CreateOrJoinWorkspaces'Repository: makeplane/plane
Length of output: 219
🏁 Script executed:
# Check if it's used in any test files
find . -name '*.test.*' -o -name '*.spec.*' | xargs grep -l 'CreateOrJoinWorkspaces' 2>/dev/nullRepository: makeplane/plane
Length of output: 43
🏁 Script executed:
# Look at the end of the create-or-join-workspaces.tsx file to understand its full scope
tail -50 apps/web/core/components/onboarding/create-or-join-workspaces.tsxRepository: makeplane/plane
Length of output: 2141
Remove unused totalSteps prop from the Props type.
totalSteps is declared in the Props type on line 29 but is never destructured or used in the component. Since other onboarding components use this prop for step display, but CreateOrJoinWorkspaces doesn't need it, it should be removed from the type definition to keep the interface clean and prevent confusion for callers.
♻️ Proposed fix
type Props = {
invitations: IWorkspaceMemberInvitation[];
- totalSteps: number;
stepChange: (steps: Partial<TOnboardingSteps>) => Promise<void>;
finishOnboarding: () => Promise<void>;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { invitations, stepChange, finishOnboarding } = props; | |
| type Props = { | |
| invitations: IWorkspaceMemberInvitation[]; | |
| stepChange: (steps: Partial<TOnboardingSteps>) => Promise<void>; | |
| finishOnboarding: () => Promise<void>; | |
| }; | |
| const { invitations, stepChange, finishOnboarding } = props; |
🤖 Prompt for AI Agents
In `@apps/web/core/components/onboarding/create-or-join-workspaces.tsx` at line
35, Remove the unused totalSteps property from the Props type for the
CreateOrJoinWorkspaces component: update the Props type declaration to no longer
include totalSteps and ensure the component signature and destructuring (const {
invitations, stepChange, finishOnboarding } = props) remain unchanged; verify no
other references to totalSteps exist in CreateOrJoinWorkspaces (or its exports)
so callers aren't affected and TypeScript errors are avoided.
apps/web/core/components/onboarding/create-or-join-workspaces.tsx
Outdated
Show resolved
Hide resolved
apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR refactors the web app to rely more on core hooks/layouts and removes several CE “shim” helpers/layout wrappers, consolidating behavior into the main codepaths.
Changes:
- Removed CE layout/helper shims (auth wrappers, instance/workspace helpers, issue actions/analytics stubs, etc.) and updated imports to point at core layouts/hooks.
- Switched workspace-creation gating to use
useInstance().configinstead of the removedgetIsWorkspaceCreationDisabledhelper. - Simplified/relocated issue display-property rendering logic (e.g., removing
issue_typefrom the display-properties list and removingshouldRenderDisplayProperty).
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/constants/src/issue/common.ts | Removes issue_type from ISSUE_DISPLAY_PROPERTIES configuration. |
| apps/web/helpers/views.helper.ts | Formatting-only change (blank line). |
| apps/web/helpers/issue-filter.helper.ts | Removes shouldRenderDisplayProperty; keeps shouldRenderColumn. |
| apps/web/core/hooks/use-issues-actions.tsx | Drops TEAM-related action handlers and associated imports. |
| apps/web/core/components/workspace/sidebar/workspace-menu-root.tsx | Replaces workspace-creation helper with useInstance().config and adjusts React import. |
| apps/web/core/components/work-item-filters/filters-hoc/project-level.tsx | Removes CE helper spread; passes workspaceSlug directly. |
| apps/web/core/components/power-k/ui/pages/open-entity/workspace-settings-menu.tsx | Removes CE helper gating for settings links. |
| apps/web/core/components/power-k/config/creation/command.ts | Replaces workspace-creation helper with useInstance().config. |
| apps/web/core/components/onboarding/steps/workspace/create.tsx | Replaces workspace-creation helper with useInstance().config. |
| apps/web/core/components/onboarding/create-or-join-workspaces.tsx | Replaces workspace-creation helper with useInstance().config and adjusts React import. |
| apps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.tsx | Updates shouldRenderColumn import to non-CE helper path. |
| apps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.tsx | Updates shouldRenderColumn import to non-CE helper path. |
| apps/web/core/components/issues/issue-layouts/filters/header/display-filters/display-properties.tsx | Removes router-based filtering and the removed helper call for display properties. |
| apps/web/core/components/issues/issue-detail-widgets/sub-issues/helper.ts | Removes CE analytics helper usage and related routing/state logic; simplifies delete flow. |
| apps/web/ce/layouts/workspace-wrapper.tsx | Deleted CE wrapper layout shim. |
| apps/web/ce/layouts/project-wrapper.tsx | Deleted CE wrapper layout shim. |
| apps/web/ce/helpers/workspace.helper.ts | Deleted CE helper shim. |
| apps/web/ce/helpers/work-item-filters/project-level.ts | Deleted CE helper shim. |
| apps/web/ce/helpers/project-settings.ts | Deleted CE helper shim. |
| apps/web/ce/helpers/pi-chat.helper.ts | Deleted CE helper shim. |
| apps/web/ce/helpers/issue-action-helper.ts | Deleted CE helper shim for TEAM-related issue actions. |
| apps/web/ce/helpers/instance.helper.ts | Deleted CE helper shim (replaced by useInstance().config). |
| apps/web/ce/helpers/epic-analytics.ts | Deleted CE helper shim. |
| apps/web/ce/helpers/command-palette.ts | Deleted CE helper shim. |
| apps/web/app/(all)/create-workspace/page.tsx | Replaces workspace-creation helper with useInstance().config. |
| apps/web/app/(all)/[workspaceSlug]/layout.tsx | Switches WorkspaceAuthWrapper import from CE shim path to core layout path. |
| apps/web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/members/page.tsx | Removes CE helper passthrough; uses direct i18n key. |
| apps/web/app/(all)/[workspaceSlug]/(settings)/settings/projects/[projectId]/layout.tsx | Switches ProjectAuthWrapper import from CE shim path to core layout path. |
| apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/layout.tsx | Switches ProjectAuthWrapper import from CE shim path to core layout path. |
| apps/web/app/(all)/[workspaceSlug]/(projects)/browse/[workItem]/page.tsx | Switches ProjectAuthWrapper import from CE shim path to core layout path. |
Comments suppressed due to low confidence (1)
apps/web/core/hooks/use-issues-actions.tsx:79
useIssuesActionsno longer handlesEIssuesStoreType.TEAM,TEAM_VIEW, orTEAM_PROJECT_WORK_ITEMS, even though these store types still exist (e.g.useIssueStoreTypecan return them) and are included in layout components’ supported store-type unions. Falling back touseProjectIssueActionswill be incorrect on team/teamspace routes (noprojectId), resulting in fetch/update actions becoming no-ops or targeting the wrong store. Either restore explicit handlers for these store types (even if they are intentional no-ops in CE) or remove/guard the corresponding store types upstream so callers can’t hit the default branch silently.
export const useIssuesActions = (storeType: EIssuesStoreType): IssueActions => {
const projectIssueActions = useProjectIssueActions();
const projectEpicsActions = useProjectEpicsActions();
const cycleIssueActions = useCycleIssueActions();
const moduleIssueActions = useModuleIssueActions();
const projectViewIssueActions = useProjectViewIssueActions();
const globalIssueActions = useGlobalIssueActions();
const profileIssueActions = useProfileIssueActions();
const archivedIssueActions = useArchivedIssueActions();
const workspaceDraftIssueActions = useWorkspaceDraftIssueActions();
switch (storeType) {
case EIssuesStoreType.PROJECT_VIEW:
return projectViewIssueActions;
case EIssuesStoreType.PROFILE:
return profileIssueActions;
case EIssuesStoreType.ARCHIVED:
return archivedIssueActions;
case EIssuesStoreType.CYCLE:
return cycleIssueActions;
case EIssuesStoreType.MODULE:
return moduleIssueActions;
case EIssuesStoreType.GLOBAL:
return globalIssueActions;
case EIssuesStoreType.WORKSPACE_DRAFT:
//@ts-expect-error type mismatch
return workspaceDraftIssueActions;
case EIssuesStoreType.EPIC:
return projectEpicsActions;
case EIssuesStoreType.PROJECT:
default:
return projectIssueActions;
}
| const { config } = useInstance(); | ||
| const { updateUserProfile } = useUserProfile(); | ||
| const { fetchCurrentUserSettings } = useUserSettings(); | ||
| const { createWorkspace, fetchWorkspaces } = useWorkspace(); | ||
|
|
||
| const isWorkspaceCreationEnabled = getIsWorkspaceCreationDisabled() === false; | ||
| const isWorkspaceCreationDisabled = config?.is_workspace_creation_disabled ?? false; | ||
|
|
There was a problem hiding this comment.
isWorkspaceCreationEnabled is derived from config?.is_workspace_creation_disabled, which inverts the meaning. With the current code, workspace creation will be treated as disabled when is_workspace_creation_disabled is false, and the component will incorrectly show the “restricted creation” message (and block the form) on instances where creation is allowed. Derive this as !is_workspace_creation_disabled (or rename the variable to isWorkspaceCreationDisabled and update the conditional accordingly).
| const { config } = useInstance(); | ||
| // derived values | ||
| const isWorkspaceCreationEnabled = getIsWorkspaceCreationDisabled() === false; | ||
| const isWorkspaceCreationDisabled = config?.is_workspace_creation_disabled ?? false; | ||
|
|
There was a problem hiding this comment.
isWorkspaceCreationEnabled is currently set to config?.is_workspace_creation_disabled ?? false, which flips the intended meaning. This will show the Create Workspace flow when creation is disabled, and show the restriction warning when creation is enabled. Compute this as !is_workspace_creation_disabled (or rename to isWorkspaceCreationDisabled and invert the JSX condition).
| const { config } = useInstance(); | ||
| const { data: currentUser } = useUser(); | ||
| const { signOut } = useUser(); | ||
| const { updateUserProfile } = useUserProfile(); | ||
| const { currentWorkspace: activeWorkspace, workspaces } = useWorkspace(); | ||
| // derived values | ||
| const isWorkspaceCreationEnabled = getIsWorkspaceCreationDisabled() === false; | ||
| const isWorkspaceCreationDisabled = config?.is_workspace_creation_disabled ?? false; |
There was a problem hiding this comment.
isWorkspaceCreationEnabled is assigned from config?.is_workspace_creation_disabled ?? false, which inverts the flag’s meaning. This will hide the “Create workspace” menu item when creation is allowed and show it when it’s disabled. Use const isWorkspaceCreationEnabled = !(config?.is_workspace_creation_disabled ?? false) (or rename to isWorkspaceCreationDisabled and update the condition).
2 participants
Type of Change
Summary by CodeRabbit
Refactor
Removals
UI Improvements