plane icon indicating copy to clipboard operation
plane copied to clipboard

Published 20 hours ago • verified publisher icon makeplane

[WEB-5429] refactor: conditional hooks

Open aaryan610 opened this issue 3 weeks ago • 2 comments

Description

This PR refactors instances where hooks are initialized conditionally.

Type of Change

  • [x] Code refactoring

Summary by CodeRabbit

  • Refactor

    • Improved filter initialization logic for consistent handling of undefined states across multiple components.
    • Optimized conditional hook invocation patterns to ensure filters are properly defined in all code paths.

  • Chores

    • Added comprehensive codebase analysis documenting hook usage patterns and conditional filter initialization approaches.

aaryan610 avatar Nov 14 '25 08:11 aaryan610

Linked to Plane Work Item(s)

This comment was auto-generated by Plane

makeplane[bot] avatar Nov 14 '25 08:11 makeplane[bot]

Walkthrough

This PR introduces a useSWR hooks usage analysis report and systematically refactors work-item filter initialization across empty state components. The changes move undefined-checks from component call sites into the useWorkItemFilterInstance hook itself, enabling unconditional hook invocations with the hook handling undefined entity IDs.

Changes

Cohort / File(s) Change Summary
New Report
CONDIDENTIAL_USESWR_REPORT.md		 Introduces Markdown report analyzing useSWR hook usage patterns; catalogs 4 files violating Rules of Hooks (hooks before early returns) and ~92 files following correct conditional-key patterns; includes remediation recommendations.
Hook Signature Update
apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts		 Modified useWorkItemFilterInstance to accept entityId: string | undefined and return undefined when entityId is undefined; previously always invoked getFilter().
Empty State Components
apps/web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx,
apps/web/core/components/issues/issue-layouts/empty-states/cycle.tsx,
apps/web/core/components/issues/issue-layouts/empty-states/module.tsx,
apps/web/core/components/issues/issue-layouts/empty-states/project-issues.tsx		 Replaced conditional hook invocations with unconditional calls; filters are now always initialized via hooks regardless of ID availability, delegating undefined-check logic to the hook.
Calendar Component Refactor
apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx		 Introduced local fallbackStoreType variable and refactored storeType selection logic; functionally equivalent with improved readability.

Sequence Diagram

sequenceDiagram
    participant Comp as Empty State Component
    participant Hook as useWorkItemFilterInstance
    participant Filter as Filter Store

    rect rgba(100, 150, 200, 0.3)
    Note over Comp,Filter: Before: Conditional Hook Invocation
    alt ID exists
        Comp->>Hook: call hook(id)
        Hook->>Filter: getFilter(id)
        Filter-->>Hook: filter instance
        Hook-->>Comp: filter instance
    else ID undefined
        Note over Comp: skip hook call
        Comp->>Comp: filterInstance = undefined
    end
    end

    rect rgba(100, 150, 200, 0.3)
    Note over Comp,Filter: After: Unconditional Hook Invocation
    Comp->>Hook: call hook(id or undefined)
    alt ID exists
        Hook->>Filter: getFilter(id)
        Filter-->>Hook: filter instance
    else ID undefined
        Hook->>Hook: early return undefined
    end
    Hook-->>Comp: filter instance or undefined
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Hook signature change: Verify that the new undefined-handling logic in useWorkItemFilterInstance correctly propagates through dependent components and doesn't introduce unintended side effects.
  • Systematic component updates: Ensure all four empty state components handle undefined filter returns safely (note: code shows optional chaining is used, but confirm no downstream assumptions of non-null filters).
  • New report artifact: Review the useSWR violations and recommended patterns to confirm accuracy and assess implications of flagged files.

Poem

🐰 Hooks now hop where paths align,
No early leaps—just conditional signs,
Filters initialize without a fuss,
Rules of Hooks kept righteous for us!
✨ thump thump

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description is incomplete; it lacks required sections including detailed explanation, test scenarios, and references, though it does specify the type of change correctly. Expand the description to include: specific examples of changed components, test scenarios validating the refactoring, and links to related issues (e.g., WEB-5429).
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main refactoring objective—converting conditionally initialized hooks to unconditionally initialized ones—which aligns with the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch refactor/conditional-hooks
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e357c4ad0b3f04514028455744105a9a82ff6ef and a54f0c41132f450b4aa259659a861f2280c44e2c.

📒 Files selected for processing (7)
  • CONDITIONAL_USESWR_REPORT.md (1 hunks)
  • apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx (1 hunks)
  • apps/web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx (1 hunks)
  • apps/web/core/components/issues/issue-layouts/empty-states/cycle.tsx (1 hunks)
  • apps/web/core/components/issues/issue-layouts/empty-states/module.tsx (1 hunks)
  • apps/web/core/components/issues/issue-layouts/empty-states/project-issues.tsx (1 hunks)
  • apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-16T07:23:39.497Z 
Learnt from: vamsikrishnamathala
Repo: makeplane/plane PR: 7214
File: web/core/store/issue/helpers/base-issues.store.ts:117-117
Timestamp: 2025-06-16T07:23:39.497Z
Learning: In the updateIssueDates method of BaseIssuesStore (web/core/store/issue/helpers/base-issues.store.ts), the projectId parameter is intentionally made optional to support override implementations in subclasses. The base implementation requires projectId and includes an early return check, but making it optional allows derived classes to override the method with different parameter requirements.

Applied to files:

  • apps/web/core/components/issues/issue-layouts/empty-states/project-issues.tsx
  • apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx
  • apps/web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx
📚 Learning: 2025-07-08T13:41:01.659Z 
Learnt from: prateekshourya29
Repo: makeplane/plane PR: 7363
File: apps/web/core/components/issues/select/dropdown.tsx:9-11
Timestamp: 2025-07-08T13:41:01.659Z
Learning: The `getProjectLabelIds` function in the label store handles undefined projectId internally, so it's safe to pass undefined values to it without explicit checks in the calling component.

Applied to files:

  • apps/web/core/components/issues/issue-layouts/empty-states/project-issues.tsx
  • apps/web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx
📚 Learning: 2025-06-18T09:46:08.566Z 
Learnt from: prateekshourya29
Repo: makeplane/plane PR: 7188
File: web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx:40-45
Timestamp: 2025-06-18T09:46:08.566Z
Learning: When reviewing breadcrumb components that accept projectId or similar props, check if empty strings are being passed during loading states, which can result in invalid URLs. The preferred approach is to handle these loading states internally within the component rather than requiring each consumer to manage the loading logic.

Applied to files:

  • apps/web/core/components/issues/issue-layouts/empty-states/project-issues.tsx
📚 Learning: 2025-10-21T17:22:05.204Z 
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7989
File: apps/web/app/(all)/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(detail)/[pageId]/page.tsx:45-46
Timestamp: 2025-10-21T17:22:05.204Z
Learning: In the makeplane/plane repository, the refactor from useParams() to params prop is specifically scoped to page.tsx and layout.tsx files in apps/web/app (Next.js App Router pattern). Other components (hooks, regular client components, utilities) should continue using the useParams() hook as that is the correct pattern for non-route components.

Applied to files:

  • apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx
🧬 Code graph analysis (6)
apps/web/core/components/issues/issue-layouts/empty-states/project-issues.tsx (1)
apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts (1)
  • useWorkItemFilterInstance (7-13)
apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts (2)
packages/shared-state/src/store/work-item-filters/shared.ts (1)
  • IWorkItemFilterInstance (8-8)
apps/web/core/hooks/store/work-item-filters/use-work-item-filters.ts (1)
  • useWorkItemFilters (7-11)
apps/web/core/components/issues/issue-layouts/empty-states/module.tsx (2)
apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts (1)
  • useWorkItemFilterInstance (7-13)
apps/web/core/store/issue/helpers/base-issues.store.ts (1)
  • moduleId (264-266)
apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx (1)
apps/web/core/hooks/use-issue-layout-store.ts (1)
  • useIssueStoreType (8-37)
apps/web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx (1)
apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts (1)
  • useWorkItemFilterInstance (7-13)
apps/web/core/components/issues/issue-layouts/empty-states/cycle.tsx (2)
apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts (1)
  • useWorkItemFilterInstance (7-13)
apps/web/core/store/issue/helpers/base-issues.store.ts (1)
  • cycleId (269-271)
🪛 LanguageTool
CONDITIONAL_USESWR_REPORT.md

[uncategorized] ~202-~202: The official name of this software platform is spelled with a capital “H”. Context: ... - apps/web/core/components/integration/github/root.tsx - apps/web/core/components/int...

(GITHUB)

[uncategorized] ~203-~203: The official name of this software platform is spelled with a capital “H”. Context: ... - apps/web/core/components/integration/github/select-repository.tsx - apps/web/core/c...

(GITHUB)

[uncategorized] ~207-~207: The official name of this software platform is spelled with a capital “H”. Context: ... - apps/web/core/components/integration/github/single-user-select.tsx - apps/web/core/...

(GITHUB)

[uncategorized] ~208-~208: The official name of this software platform is spelled with a capital “H”. Context: ... - apps/web/core/components/integration/github/repo-details.tsx - apps/web/core/compon...

(GITHUB)

[grammar] ~236-~236: Ensure spelling is correct Context: ...nal keys properly The vast majority of useSWR usage in the codebase follows the corre...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

[grammar] ~291-~291: Ensure spelling is correct Context: ...>; }; ``` --- ## Key Takeaway Most useSWR usage is correct! The issue is specif...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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). (4)
  • GitHub Check: Agent
  • GitHub Check: CodeQL analysis (javascript-typescript)
  • GitHub Check: Build and lint web apps
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
apps/web/core/components/issues/issue-layouts/calendar/base-calendar-root.tsx (2)

55-56: LGTM: Cleaner extraction of hook invocation.

Extracting the useIssueStoreType() call to a separate variable improves readability and ensures unconditional hook invocation, which aligns with React's Rules of Hooks.

1-299: Excellent documentation of useSWR patterns.

This report provides comprehensive analysis distinguishing correct conditional key usage from Rules of Hooks violations. The examples and remediation patterns are clear and actionable.

Note: The 4 violations documented (in apps/space and workspace/project wrappers) are not addressed in this PR, which focuses on work-item filter hooks. Consider tracking those violations separately if not already planned.

apps/web/core/components/issues/issue-layouts/empty-states/cycle.tsx (1)

39-39: LGTM: Correct application of unconditional hook pattern.

The hook is now invoked unconditionally with cycleId (which may be undefined), and the undefined check is handled within useWorkItemFilterInstance. Downstream usage correctly employs optional chaining (cycleWorkItemFilter?.hasActiveFilters, cycleWorkItemFilter?.clearFilters).

apps/web/core/components/issues/issue-layouts/empty-states/project-issues.tsx (1)

25-25: LGTM: Consistent unconditional hook pattern.

The hook is invoked unconditionally with potentially undefined projectId, with downstream code using optional chaining appropriately (projectWorkItemFilter?.hasActiveFilters, projectWorkItemFilter?.clearFilters).

apps/web/core/components/issues/issue-layouts/empty-states/module.tsx (1)

37-37: LGTM: Consistent unconditional hook pattern.

The hook is invoked unconditionally with potentially undefined moduleId, with downstream code using optional chaining appropriately (moduleWorkItemFilter?.hasActiveFilters, moduleWorkItemFilter?.clearFilters).

apps/web/core/components/issues/issue-layouts/empty-states/archived-issues.tsx (1)

24-24: LGTM: Consistent unconditional hook pattern.

The hook is invoked unconditionally with potentially undefined projectId, with downstream code using optional chaining appropriately (archivedWorkItemFilter?.hasActiveFilters, archivedWorkItemFilter?.clearFilters).

apps/web/core/hooks/store/work-item-filters/use-work-item-filter-instance.ts (1)

7-13: LGTM: Core hook correctly updated and all call sites already follow the new pattern.

All call sites already pass potentially undefined entityId parameters (derived from router params with conditional checks), and all properly handle the undefined return type using optional chaining:

  • moduleWorkItemFilter?.hasActiveFilters
  • projectWorkItemFilter?.clearFilters
  • Nullish checks: !archivedWorkItemFilter

Type safety is maintained throughout the codebase. No additional changes needed at call sites.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 14 '25 08:11 coderabbitai[bot]