[WEB-4852]chore: views refactor#7729
Conversation
WalkthroughCentralizes view context-menu construction into a new hook-based factory, refactors quick-action components to consume it, removes CE-specific no-op service stubs, standardizes workspace/view service constructors to use API_BASE_URL, simplifies global/project view stores by removing publish/lock/access APIs, updates import/re-export paths, and makes a small header alignment tweak. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Component as QuickActions Component
participant Hook as useViewMenuItems
participant Factory as useMenuItemsFactory
Note over Component,Hook: Build menu at render time
User->>Component: Open context menu
Component->>Hook: useViewMenuItems(props)
Hook->>Factory: create item builders (edit/open/copy/delete)
Factory-->>Hook: item creators (permission-aware)
Hook-->>Component: TContextMenuItem[]
Component->>User: Render menu
alt Edit
User->>Component: Click "Edit"
Component->>Component: captureClick()
Component->>Component: setCreateUpdateViewModal(true)
else Open in new tab
User->>Component: Click "Open in new tab"
Component->>Component: handleOpenInNewTab()
else Copy link
User->>Component: Click "Copy link"
Component->>Component: handleCopyText()
else Delete
User->>Component: Click "Delete"
Component->>Component: setDeleteViewModal(true)
end
sequenceDiagram
autonumber
actor User
participant UVC as UpdateViewComponent
participant Store as View Store
Note over UVC: Locked-state UI removed
User->>UVC: View header
alt isLocked or filtersEqual or unauthorized
UVC-->>User: No action buttons rendered
else
UVC-->>User: Show "Save as" (+ optional "Update view")
User->>UVC: Click "Update view"
UVC->>Store: handleUpdateView()
Store-->>UVC: Updated view
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing touches
🧪 Generate unit tests
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 |
vamsikrishnamathala
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/web/core/store/global-view.store.ts (2)
171-180: Potential crash when prior filters are missingAccessing workspaceIssuesFilter.filters[viewId].filters can be undefined, causing a runtime error.
- const currentGlobalViewFilters: IIssueFilterOptions = this.rootStore.issue.workspaceIssuesFilter.filters[ - viewId - ].filters as IIssueFilterOptions; + const currentGlobalViewFilters = + (this.rootStore.issue.workspaceIssuesFilter.filters?.[viewId]?.filters as IIssueFilterOptions | undefined) ?? + {}; const newFilters: IIssueFilterOptions = {}; - Object.keys(currentGlobalViewFilters ?? {}).forEach((key) => { + Object.keys(currentGlobalViewFilters).forEach((key) => { newFilters[key as keyof IIssueFilterOptions] = []; });
200-204: Don’t swallow errors; wrap revert in runInAction and rethrowSwallowing makes upstream UX inconsistent versus createGlobalView which rethrows.
- } catch { - Object.keys(data).forEach((key) => { - const currentKey = key as keyof IWorkspaceView; - if (currentViewData) set(this.globalViewMap, [viewId, currentKey], currentViewData[currentKey]); - }); + } catch (error) { + runInAction(() => { + Object.keys(data).forEach((key) => { + const currentKey = key as keyof IWorkspaceView; + if (currentViewData) set(this.globalViewMap, [viewId, currentKey], currentViewData[currentKey]); + }); + }); + throw error; }apps/web/core/store/project-view.store.ts (1)
26-26: Fix type mismatch: getViewById may return null but interface disallows itThe interface promises
IProjectView, while the implementation returnsIProjectView | null. This will fail TS interface conformance.Apply:
- getViewById: (viewId: string) => IProjectView; + getViewById: (viewId: string) => IProjectView | null;Also applies to: 132-132
apps/web/core/local-db/utils/load-workspace.ts (1)
2-2: Remove unused API_BASE_URL importAfter switching to parameterless
WorkspaceService, this import is unused and may fail lint.-import { API_BASE_URL } from "@plane/constants";apps/web/core/services/view.service.ts (1)
12-26: Tighten return types for stronger contracts
createView/patchViewreturnanybut callers treat them asIProjectView. Make the types explicit.- async createView(workspaceSlug: string, projectId: string, data: Partial<IProjectView>): Promise<any> { + async createView(workspaceSlug: string, projectId: string, data: Partial<IProjectView>): Promise<IProjectView> { ... - async patchView(workspaceSlug: string, projectId: string, viewId: string, data: Partial<IProjectView>): Promise<any> { + async patchView(workspaceSlug: string, projectId: string, viewId: string, data: Partial<IProjectView>): Promise<IProjectView> {
🧹 Nitpick comments (14)
apps/web/core/hooks/store/use-project-view.ts (1)
5-5: Type import path shift — IProjectViewStore exported from file; no barrel re-exportIProjectViewStore is declared/exported in apps/web/core/store/project-view.store.ts (export interface at line 14); use-project-view.ts imports it from '@/plane-web/store/project-view.store' — the import is valid and no type drift observed. No store-level barrel re-export of IProjectViewStore was found; add a re-export if you want consumers to import from '@/plane-web/store' instead of the file path.
apps/web/core/components/views/update-view-component.tsx (2)
16-17: Unused prop: remove lockedTooltipContent from Props
lockedTooltipContentis no longer destructured or used. Drop it fromPropsto avoid dead API surface.Apply:
type Props = { isLocked: boolean; areFiltersEqual: boolean; isOwner: boolean; isAuthorizedUser: boolean; setIsModalOpen: (value: SetStateAction<boolean>) => void; handleUpdateView: () => void; - lockedTooltipContent?: string; trackerElement: string; };
48-61: Locked views now hide all actions — confirm intended UX (Save as may be desirable while locked)Current condition hides both “Save as” and “Update view” when
isLocked. If “Save as” should remain available for authorized users on locked views, decouple its visibility fromisLocked.Optionally:
- {!isLocked && !areFiltersEqual && isAuthorizedUser && ( + {!areFiltersEqual && isAuthorizedUser && ( <> <Button variant="outline-primary" size="md" className="flex-shrink-0" data-ph-element={trackerElement} onClick={() => setIsModalOpen(true)} > Save as </Button> - {isOwner && <>{updateButton}</>} + {isOwner && !isLocked && <>{updateButton}</>} </> )}apps/web/core/components/views/quick-actions.tsx (1)
57-71: Avoid brittle fixed-index splice for publish itemInserting at index 2 assumes a specific base order from the factory. Prefer injecting via the factory or compute position relative to a known key.
Apply:
- if (publishContextMenu) MENU_ITEMS.splice(2, 0, publishContextMenu); + if (publishContextMenu) { + const anchorIdx = MENU_ITEMS.findIndex((i) => i.key === "copy-link" || i.key === "open-in-new-tab"); + const insertAt = anchorIdx >= 0 ? anchorIdx + 1 : MENU_ITEMS.length; + MENU_ITEMS.splice(insertAt, 0, publishContextMenu); + }Or expose a
withPublishItem()from the menu factory to control placement centrally.apps/web/ce/components/views/helper.tsx (2)
5-5: Mark TWorkspaceLayoutProps as a type-only import to avoid runtime cyclesHelps tree-shaking and prevents potential circular deps at runtime.
-import { TWorkspaceLayoutProps } from "@/components/views/helper"; +import type { TWorkspaceLayoutProps } from "@/components/views/helper";
17-28: Trim or document unused props (workspaceSlug, projectId, viewId)They’re not consumed by the factory. Either remove for now or add a comment indicating upcoming usage to avoid confusion.
apps/web/core/components/workspace/views/quick-action.tsx (1)
36-45: Localize toast messages"Link Copied!" and "View link copied to clipboard." are user-facing. Prefer i18n for consistency.
- copyUrlToClipboard(viewLink).then(() => { - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Link Copied!", - message: "View link copied to clipboard.", - }); - }); + copyUrlToClipboard(viewLink).then(() => { + setToast({ + type: TOAST_TYPE.SUCCESS, + title: t("link_copied_title"), + message: t("view_link_copied_message"), + }); + });If these keys don’t exist, I can add them to i18n. Want me to open a PR?
apps/web/core/store/project-view.store.ts (3)
159-176: Return fetched views and always reset loaderCurrently the success path implicitly returns
undefined. Also, considerfinallyto ensure loader reset.- fetchViews = async (workspaceSlug: string, projectId: string) => { - try { - this.loader = true; - await this.viewService.getViews(workspaceSlug, projectId).then((response) => { - runInAction(() => { - response.forEach((view) => { - set(this.viewMap, [view.id], view); - }); - set(this.fetchedMap, projectId, true); - this.loader = false; - }); - return response; - }); - } catch (error) { - this.loader = false; - return undefined; - } - }; + fetchViews = async (workspaceSlug: string, projectId: string) => { + try { + this.loader = true; + const response = await this.viewService.getViews(workspaceSlug, projectId); + runInAction(() => { + response.forEach((view) => set(this.viewMap, [view.id], view)); + set(this.fetchedMap, projectId, true); + }); + return response; + } catch { + return undefined; + } finally { + runInAction(() => { + this.loader = false; + }); + } + };
200-208: Avoid mutating caller’s data in createView
getValidatedViewFiltersmutates its input; clone before validating to avoid side effects.-import { set } from "lodash"; +import { set, cloneDeep } from "lodash"; ... - async createView(workspaceSlug: string, projectId: string, data: Partial<IProjectView>): Promise<IProjectView> { - const response = await this.viewService.createView(workspaceSlug, projectId, getValidatedViewFilters(data)); + async createView(workspaceSlug: string, projectId: string, data: Partial<IProjectView>): Promise<IProjectView> { + const response = await this.viewService.createView( + workspaceSlug, + projectId, + getValidatedViewFilters(cloneDeep(data)) + );
103-109: Minor naming consistencyPrefer
viewsList(camelCase) overViewsList.- const ViewsList = Object.values(this.viewMap ?? {}); + const viewsList = Object.values(this.viewMap ?? {}); - let filteredViews = ViewsList.filter((view) => view?.project === projectId); + let filteredViews = viewsList.filter((view) => view?.project === projectId);Apply similarly in
getFilteredProjectViews.Also applies to: 116-127
apps/web/core/services/view.service.ts (1)
12-18: Preserve error details when response is absentRe-throwing
error?.response?.datacan yieldundefinedon network errors. Prefer fallback to the original error.- .catch((error) => { - throw error?.response?.data; - }); + .catch((error) => { + throw (error?.response?.data ?? error); + });Consider centralizing this in
APIServiceto avoid repetition.Also applies to: 20-26, 28-34, 36-42, 44-50, 52-58, 60-72, 74-80
apps/web/core/services/workspace.service.ts (3)
27-29: Prefer a backward-compatible constructor to avoid breakages and keep testabilityRetain an optional baseUrl param with a default to API_BASE_URL. This keeps overrides/mocking simple and cushions any missed call-site updates.
-export class WorkspaceService extends APIService { - constructor() { - super(API_BASE_URL); - } -} +export class WorkspaceService extends APIService { + constructor(baseUrl: string = API_BASE_URL) { + super(baseUrl); + } +}
31-37: Normalize thrown error shapes across methodsSome methods throw error.response while others throw error.response?.data. This inconsistency bleeds into UI error handling. Consider centralizing error extraction in APIService (e.g., an interceptor or helper) and always throwing a typed APIError.
Example approach in APIService:
protected extractError(e: any) { const res = e?.response; return { status: res?.status, data: res?.data ?? res, message: res?.data?.message ?? e?.message ?? "Request failed", }; } // Usage in services: .catch((e) => { throw this.extractError(e); })Also applies to: 39-45, 113-119, 290-298, 336-347, 349-356, 358-368, 370-376, 378-388
323-329: Guard query_type joining to avoid runtime errorsIf query_type is unexpectedly missing or not an array, join() will throw. Make it tolerant without changing the API.
- return this.get(`/api/workspaces/${workspaceSlug}/entity-search/`, { - params: { - ...params, - query_type: params.query_type.join(","), - }, - }) + return this.get(`/api/workspaces/${workspaceSlug}/entity-search/`, { + params: { + ...params, + ...(params.query_type + ? { query_type: Array.isArray(params.query_type) ? params.query_type.join(",") : params.query_type } + : {}), + }, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/web/app/(all)/[workspaceSlug]/(projects)/workspace-views/header.tsx(1 hunks)apps/web/ce/components/views/helper.tsx(2 hunks)apps/web/ce/services/index.ts(1 hunks)apps/web/ce/services/project/index.ts(1 hunks)apps/web/ce/services/project/view.service.ts(0 hunks)apps/web/ce/services/workspace.service.ts(0 hunks)apps/web/ce/store/global-view.store.ts(1 hunks)apps/web/ce/store/project-view.store.ts(1 hunks)apps/web/core/components/views/quick-actions.tsx(2 hunks)apps/web/core/components/views/update-view-component.tsx(2 hunks)apps/web/core/components/workspace/views/quick-action.tsx(2 hunks)apps/web/core/hooks/store/use-global-view.ts(1 hunks)apps/web/core/hooks/store/use-project-view.ts(1 hunks)apps/web/core/local-db/utils/load-workspace.ts(1 hunks)apps/web/core/services/view.service.ts(1 hunks)apps/web/core/services/workspace.service.ts(2 hunks)apps/web/core/store/global-view.store.ts(4 hunks)apps/web/core/store/project-view.store.ts(4 hunks)apps/web/core/store/user/base-permissions.store.ts(1 hunks)apps/web/ee/services/index.ts(1 hunks)apps/web/ee/services/project/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/web/ce/services/workspace.service.ts
- apps/web/ce/services/project/view.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-14T13:16:23.323Z
Learnt from: vamsikrishnamathala
PR: makeplane/plane#7061
File: web/core/components/workspace-notifications/root.tsx:18-18
Timestamp: 2025-05-14T13:16:23.323Z
Learning: In the Plane project codebase, the path alias `@/plane-web` resolves to the `ce` directory, making imports like `@/plane-web/hooks/use-notification-preview` correctly resolve to files in `web/ce/hooks/`.
Applied to files:
apps/web/core/hooks/store/use-project-view.ts
🧬 Code graph analysis (6)
apps/web/core/local-db/utils/load-workspace.ts (1)
apps/web/core/services/workspace.service.ts (1)
WorkspaceService(26-389)
apps/web/ce/components/views/helper.tsx (3)
packages/i18n/src/hooks/use-translation.ts (1)
useTranslation(23-35)packages/ui/src/dropdowns/context-menu/root.tsx (1)
TContextMenuItem(10-23)packages/types/src/views.ts (1)
IProjectView(9-30)
apps/web/core/components/workspace/views/quick-action.tsx (2)
packages/ui/src/dropdowns/context-menu/root.tsx (1)
TContextMenuItem(10-23)apps/web/ce/components/views/helper.tsx (1)
useViewMenuItems(73-77)
apps/web/core/components/views/quick-actions.tsx (2)
packages/ui/src/dropdowns/context-menu/root.tsx (1)
TContextMenuItem(10-23)apps/web/ce/components/views/helper.tsx (1)
useViewMenuItems(73-77)
apps/web/core/store/global-view.store.ts (1)
packages/types/src/workspace-views.ts (1)
IWorkspaceView(9-33)
apps/web/core/store/project-view.store.ts (2)
packages/types/src/views.ts (1)
IProjectView(9-30)packages/utils/src/project-views.ts (1)
getValidatedViewFilters(87-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and lint web apps
🔇 Additional comments (21)
apps/web/core/store/user/base-permissions.store.ts (1)
21-21: Switch to plane-web services barrel — confirm export & constructor compatibilityConfirm WorkspaceService is re-exported from @/plane-web/services and that its constructor accepts no required args as used in apps/web/core/store/user/base-permissions.store.ts:21. Per prior learnings, @/plane-web resolves to CE paths — ensure the CE barrel exposes WorkspaceService identically.
apps/web/ce/store/global-view.store.ts (1)
1-1: CE barrel looks goodRe-export keeps CE aligned with the centralized store. No issues spotted.
apps/web/app/(all)/[workspaceSlug]/(projects)/workspace-views/header.tsx (1)
194-194: Centering fix for RightItemAdding
items-centertoHeader.RightItemis a safe visual improvement. LGTM.apps/web/core/hooks/store/use-global-view.ts (1)
5-5: Resolved — IGlobalViewStore is exported from global-view.store
Confirmed: apps/web/core/store/global-view.store.ts containsexport interface IGlobalViewStore(line 15), so the type is available from that module.apps/web/core/components/views/quick-actions.tsx (1)
15-16: Good move to hook-based menu factoryImporting
useViewMenuItemscentralizes menu logic and reduces duplication. Nice.apps/web/ce/components/views/helper.tsx (2)
73-77: Factory composition LGTMThe hook-based composition is clean and simplifies consumers.
33-33: Translations present — no action requiredKeys "edit", "open_in_new_tab", "copy_link", and "delete" are defined in packages/i18n/src/locales/en/translations.json and mirrored across other locale files.
apps/web/core/components/workspace/views/quick-action.tsx (1)
47-57: Good delegation to useViewMenuItems; ensure helper enforces lockingHook usage looks correct. After applying the helper changes to respect isLocked, the menu will align with permissions.
Please validate that locked views disable Edit/Delete for non-owner/admin as intended after the helper change.
apps/web/core/store/global-view.store.ts (1)
136-148: createGlobalView: pattern looks goodOptimistic write + rethrow on failure is consistent. No changes needed.
apps/web/core/store/project-view.store.ts (1)
86-87: Method binding looks goodBinding class methods prevents context loss when passed as callbacks.
apps/web/ce/store/project-view.store.ts (1)
1-1: CE barrel re-export aligns with core storeRe-exporting the core project-view store via CE path is consistent with the refactor.
apps/web/ee/services/project/index.ts (1)
1-1: No callers import ViewService fromee/services/project— removing the re-export is safe.Repo search found no imports from
ee/services/project;apps/web/ee/services/project/index.tsonly exportsestimate.service, whileViewServiceis defined atapps/web/core/services/view.service.tsand consumers import it via@/plane-web/services(no migration required).Likely an incorrect or invalid review comment.
apps/web/ce/services/index.ts (1)
2-2: Switch to alias-based workspace service export looks correctMatches centralization to
@/services/workspace.service.apps/web/ce/services/project/index.ts (1)
2-2: Re-exporting view.service from shared alias is consistentGood consolidation.
apps/web/core/local-db/utils/load-workspace.ts (1)
102-105: Constructor change usage is correctInstantiating
new WorkspaceService()matches the updated service API.apps/web/ee/services/index.ts (2)
2-2: EE workspace service re-export aligns with centralizationLooks good.
2-2: Ensure "@/services" alias resolves across packagesFound alias usages; confirm your TypeScript path mappings (or the extended @plane/typescript-config/nextjs.json) map "@/services/*" to the shared services package (packages/services/src) or replace with package-relative/relative imports.
Locations:
- apps/web/ee/services/index.ts:2 — export * from "@/services/workspace.service";
- apps/web/ce/services/index.ts:2 — export * from "@/services/workspace.service";
- apps/web/core/local-db/utils/load-workspace.ts:9 — import { WorkspaceService } from "@/services/workspace.service";
- packages/services/src/workspace/index.ts:5 — export * from "./workspace.service";
apps/web/core/services/view.service.ts (2)
8-10: Constructor update is consistent with centralized API_BASE_URLNo issues.
1-11: No legacy constructor usages found — callers instantiate ViewService with no args.
- apps/web/core/store/project-view.store.ts:84 — this.viewService = new ViewService();
- apps/web/core/store/issue/project-views/filter.store.ts:70 — this.issueFilterService = new ViewService();
apps/web/core/services/workspace.service.ts (2)
1-1: Centralizing base URL import looks goodUsing API_BASE_URL aligns this service with the new convention and reduces constructor noise.
27-29: Verify no callers still pass a base URL or import the CE/EE serviceAutomated grep attempts here couldn't confirm (ripgrep skipped files); run the checks below locally and update any matches to use new WorkspaceService() and import from '@/services/workspace.service'.
# (preferred) in a git repo git grep -n --no-color -E 'new[[:space:]]+WorkspaceService[[:space:]]*\([[:space:]]*[^)\s]' || true git grep -n --no-color -E "from ['\"][^'\"]*(ce|ee)/services/workspace\.service['\"]" || true # fallback using ripgrep rg -n --hidden -S 'new\s+WorkspaceService\s*\(\s*[^)\s]' || true rg -n --hidden -S "from\s+['\"][^'\"]*(ce|ee)/services/workspace\.service['\"]" || true
vamsikrishnamathala
dismissed stale reviews from pushya22 and prateekshourya29
via
065035f
* chore: refactored view store and services * chore: removed unused import * chore: refactored update view component * fix: lint errors
4 participants
Description
This PR implements a significant refactoring of the views-related code across the application, focusing on improving code organization, reducing duplication, and centralizing view-related functionality.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Changes
Style