improvement: enhance activity components and types modularity #6262
Conversation
prateekshourya29
requested review from
SatishGandham,
sriramveeraghanta and
rahulramesha
WalkthroughThis pull request introduces comprehensive changes to activity-related type definitions and components across the Plane project. The modifications primarily focus on enhancing type safety and restructuring activity-related types, with a shift from project-specific to workspace-based activity representations. The changes span multiple files in the Changes
Sequence DiagramsequenceDiagram
participant Types as Activity Types
participant Components as Activity Components
participant Store as MobX Store
Types->>Components: Provide structured activity data
Components->>Store: Request workspace/member details
Store-->>Components: Return workspace/member information
Components->>Components: Render activity with enhanced type safety
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 1
🧹 Nitpick comments (8)
packages/types/src/activity.d.ts (2)
1-20: Consider marking optional fields with "?" for better TypeScript clarity
The fields like “field”, “comment”, “old_value”, etc., are allowed to be undefined. Converting them into optional properties (e.g., “field?: TFieldKey”) may better convey their optionality to consumers of the type.
36-36: Enum-like union for verbs
Using a union type for CRUD-like action verbs is concise and safe. You might consider enumerations or a larger union if more verbs appear in the future.web/core/components/common/activity/user.tsx (2)
16-18: Integration with store hooks
Using hooks like useMember and useWorkspace is a cleaner approach than directly accessing store properties, significantly improving testability and reusability.
32-32: Fallback handling on display_name
Ensuring a consistent display format protects against undefined or incomplete data, improving user experience.web/core/components/common/activity/activity-block.tsx (2)
18-18: Use more descriptive prop names if possible.
If “activity” is more specifically a “workspaceActivity,” consider clarifying the prop name for readability.
36-36: Consider adding alt text.
If the icon is purely decorative, no action needed. Otherwise, consider using an accessible alternative or aria-label for the icon.web/core/components/common/activity/helper.tsx (2)
29-29: Centralize store usage.
Importing and directly accessing the store is fine. If you find multiple files executing the same logic, consider creating a helper function in a common utility to retrieve the workspace detail.
278-278: Default case fallback message.
The template literal for “verb + activityType” could be refined by gracefully handling undefined or empty responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/types/src/activity.d.ts(1 hunks)packages/types/src/enums.ts(2 hunks)packages/types/src/index.d.ts(1 hunks)web/ce/types/projects/project-activity.ts(1 hunks)web/core/components/common/activity/activity-block.tsx(3 hunks)web/core/components/common/activity/activity-item.tsx(1 hunks)web/core/components/common/activity/helper.tsx(4 hunks)web/core/components/common/activity/user.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/ce/types/projects/project-activity.ts
🔇 Additional comments (18)
web/core/components/common/activity/activity-item.tsx (1)
22-23: Use of null check on activityType is a good defensive strategy
Returning null when activityType is missing prevents unexpected errors later in the rendering logic.
packages/types/src/activity.d.ts (2)
22-27: TWorkspaceBaseActivity extends TBaseActivity effectively
The addition of the “workspace” property is clearly separated at this layer, promoting good modular design. This looks correct.
29-34: TProjectBaseActivity nicely reuses TWorkspaceBaseActivity
This layered approach allows further specialization with minimal duplication, which aligns well with DRY principles. Great job.
web/core/components/common/activity/user.tsx (8)
2-2: Observer import is appropriate for MobX React
Ensuring reactivity is crucial for dynamic UI updates when observable state changes.
5-5: Transition to TWorkspaceBaseActivity
Switching from TProjectActivity to TWorkspaceBaseActivity promotes broader applicability of activity definitions across multiple contexts.
10-10: Precise prop typing enhances clarity
Using the more generic TWorkspaceBaseActivity ensures this component remains flexible for various workspace-based activities.
14-14: Wrapping component with observer
This allows the component to update automatically as the MobX observables change. Excellent approach.
20-21: Enhanced clarity by extracting actor and workspace details
Deriving actorDetail and workspaceDetail from the store fosters more maintainable code by grouping logic in one place.
25-25: Conditional user name rendering
Good that you are gracefully handling custom user names and placeholders, preventing possible runtime errors if the property is missing.
29-29: Dynamic link generation
Linking to a user’s profile based on workspace slug and actorId is a clear solution for user attribution.
37-37: Ending the component structure
The overall structure is coherent and pairs well with the store-based logic.
packages/types/src/index.d.ts (1)
38-38: Exporting activity types
Adding “export * from './activity';” ensures activity-related types are readily available throughout the codebase.
packages/types/src/enums.ts (2)
7-7: Consolidate multiline type declaration.
The single-line format is more compact and aligned with TypeScript best practices.
62-62: New enum member added.
The addition of "TEAM_SPACE_COMMENT_DESCRIPTION" is consistent with the naming convention of the other enum members. Ensure that any relevant logic that depends on file asset types is updated accordingly.
✅ Verification successful
Let me verify if this enum addition impacts other parts of the codebase by searching for its usage and related patterns.
Let me search for any type-related usage patterns and file asset type handling to ensure we haven't missed any implementations.
Let me check the complete enum definition to understand the context better.
Based on the gathered information, I can now provide the final response:
The addition of TEAM_SPACE_COMMENT_DESCRIPTION is safe and follows existing patterns
The new enum member follows the established pattern in the codebase, where similar entries like TEAM_SPACE_DESCRIPTION and COMMENT_DESCRIPTION already exist. From the codebase analysis, we can see that file asset types are properly handled in the existing file upload and management logic, and this new addition maintains consistency with the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any usage of TEAM_SPACE_COMMENT_DESCRIPTION
rg "TEAM_SPACE_COMMENT_DESCRIPTION" -A 2
# Search for similar enum patterns to understand the context
ast-grep --pattern 'enum $_ {
$$$
TEAM_SPACE_$$$
$$$
}'
# Look for switch statements or type checks that might need updates
ast-grep --pattern 'switch ($x) {
$$$
case $_.TEAM_SPACE_$$$:
$$$
}'
Length of output: 310
Script:
#!/bin/bash
# Search for type definitions or interfaces using FileAssetType
rg "FileAssetType" -A 3
# Look for any functions or methods that handle file asset types
ast-grep --pattern 'function $_($$_: FileAssetType) {
$$$
}'
# Search for object literals or mappings involving file asset types
rg "type.*=.*FileAssetType" -A 2
Length of output: 8187
Script:
#!/bin/bash
# Get the complete enum definition
cat packages/types/src/enums.ts | grep -A 20 "export enum EFileAssetType"
# Check if there are any type mappings or interfaces that might need updates
rg "interface.*{[^}]*FileAssetType" -A 5
Length of output: 855
web/core/components/common/activity/activity-block.tsx (2)
46-46: Tooltip’s user feedback.
Good use of cursor-help. Confirm that the tooltip remains clear and functional on both mobile and desktop.
32-33: Recheck spacing changes.
Swapping from gap-3 to gap-2 and adjusting padding may subtly alter layout. Ensure these changes align with intended design updates.
✅ Verification successful
Let me gather information about the spacing changes by examining the git history and related files.
Let me search for more specific information about the spacing patterns and any related components.
Let me search for the previous version of this file to see the exact change in spacing.
Based on the git history, I can now see the exact changes. The spacing was changed from gap-3 to gap-2 and the padding values were changed from pb-2/pt-2/py-2 to pb-3/pt-3/py-3. Let me check the tailwind configuration for any spacing guidelines.
Based on the gathered information, I can now provide a final response. The tailwind configuration shows that both gap and padding values are scaled to 90% of the default, where gap-2 is 0.45rem and gap-3 is 0.675rem, while py-3 is 0.675rem. The change from gap-3 to gap-2 and the padding adjustments are consistent with the design system's spacing scale.
The spacing changes are aligned with the design system.
The modifications from gap-3 (0.675rem) to gap-2 (0.45rem) and the padding adjustments to py-3 (0.675rem) follow the project's standardized spacing scale as defined in the tailwind configuration. These values are intentionally scaled to 90% of the default Tailwind values for consistent spacing across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check git history for the specific file and lines to see the changes
git log -L32,33:web/core/components/common/activity/activity-block.tsx
# Search for similar spacing patterns in related files
rg "gap-[23]" web/core/components/common/activity/
# Look for design system or style guide files that might define spacing
fd "style|theme|spacing" web/core
Length of output: 501
Script:
#!/bin/bash
# Search for other activity block components and their spacing patterns
rg -A 2 -B 2 "activity.*block" web/core/
# Look for similar components with gap and padding classes
rg "gap-[23].*p[tby]-3" web/core/
# Check if there are any style guidelines or design tokens
cat web/core/components/core/theme/custom-theme-selector.tsx
Length of output: 14290
Script:
#!/bin/bash
# Get the previous version of the activity-block file
git show HEAD^:web/core/components/common/activity/activity-block.tsx
# Look for any design system documentation
fd "design-system|tailwind" . --type f --exec rg -l "gap|spacing"
Length of output: 2078
Script:
#!/bin/bash
# Check tailwind config for spacing definitions
cat ./packages/tailwind-config-custom/tailwind.config.js
# Look for other instances of this component being used
rg -A 2 "ActivityBlockComponent" web/core/
Length of output: 39327
web/core/components/common/activity/helper.tsx (2)
74-74: Enhanced data fetching approach.
Retrieving workspace details via store is preferred over previously direct references to workspace_detail. This approach ensures the logic is consistent with updated data flows.
76-76: Updated function signature for getBooleanActionText.
Allowing undefined value handling is a good improvement. Double-check any upstream code where non-string values may flow into this function.
✅ Verification successful
Let me gather information about the usage of getBooleanActionText function to verify if there are any potential issues with non-string values flowing into it.
Let me search for where this newValue is coming from to verify the type safety.
Let me search for where newValue is being passed to getBooleanActionText to understand its type safety.
Let me check the type definition of TProjectActivity to understand the type of new_value.
Let me check the TProjectBaseActivity type definition to understand the type of new_value.
Let me check the TWorkspaceBaseActivity type and the actual implementation of getBooleanActionText.
Let me check the TBaseActivity type to complete the chain of type definitions.
Based on all the gathered information, I can now provide a final response about the function signature change:
Function signature change for getBooleanActionText is type-safe and well-implemented.
The string | undefined type aligns perfectly with the new_value field from TBaseActivity type definition, and the function properly handles both cases by checking for specific string values ("true"/"false") and falling back to a default. No upstream issues are expected.
Reasoning:
- The function accepts
string | undefinedwhich matches the type ofnew_valuefrom the activity type chain (TProjectActivity -> TProjectBaseActivity -> TWorkspaceBaseActivity -> TBaseActivity) - The implementation safely handles both string values and undefined through explicit checks
- All usages are passing
newValuewhich comes fromactivity.new_valuethat matches the expected type
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usages of getBooleanActionText function
rg "getBooleanActionText" -A 2
# Also search for the function definition to understand its implementation
ast-grep --pattern 'getBooleanActionText = ($$$) => {
$$$
}'
Length of output: 1918
Script:
#!/bin/bash
# Search for the switch case and surrounding context to understand where newValue comes from
rg "case \"is_epic_enabled\":" -B 10 -A 2
# Also look for any type definitions related to this activity
ast-grep --pattern 'type $_ = {
$$$
}'
Length of output: 67581
Script:
#!/bin/bash
# Search for the context around where newValue is being used with getBooleanActionText
rg "getBooleanActionText\(newValue\)" -B 10
# Also look for where newValue is being set/defined
rg "newValue" -B 5 web/core/components/common/activity/helper.tsx
Length of output: 6858
Script:
#!/bin/bash
# Search for TProjectActivity type definition
rg "type TProjectActivity" -A 10
# Also search for any interfaces extending or types including TProjectActivity
rg "TProjectActivity"
Length of output: 702
Script:
#!/bin/bash
# Search for TProjectBaseActivity type definition
rg "type TProjectBaseActivity" -A 15
# Also look at the implementation of getBooleanActionText to understand how it handles the values
ast-grep --pattern 'getBooleanActionText = ($$$) => {
$$$
}'
Length of output: 587
Script:
#!/bin/bash
# Search for TWorkspaceBaseActivity type definition
rg "type TWorkspaceBaseActivity" -A 15
# Look for the full implementation of getBooleanActionText function
rg "getBooleanActionText" -A 5 web/core/components/common/activity/helper.tsx
Length of output: 2036
Script:
#!/bin/bash
# Search for TBaseActivity type definition
rg "type TBaseActivity" -A 15
# Also search for any activity-related type imports in the helper file
rg "import.*activity" web/core/components/common/activity/helper.tsx
Length of output: 1142
| // types | ||
| import { TWorkspaceBaseActivity } from "@plane/types"; | ||
| // ui |
There was a problem hiding this comment.
💡 Codebase verification
Type change requires updates in dependent files
The type hierarchy shows that TProjectActivity (which is an alias for TProjectBaseActivity) extends TWorkspaceBaseActivity with an additional project field. The change from TProjectActivity to TWorkspaceBaseActivity in activity-block.tsx could lead to missing project information in the following files that still use TProjectActivity:
web/core/components/common/activity/activity-item.tsxweb/core/components/common/activity/helper.tsx
These files need to be reviewed and potentially updated to handle the absence of project information or maintain consistency with the type being used.
🔗 Analysis chain
Type definition upgraded to TWorkspaceBaseActivity.
Switching from TProjectActivity to TWorkspaceBaseActivity ensures consistency with the new workspace-driven approach. Confirm that all references to the old type are removed or updated across the codebase.
Let me gather more information about the TWorkspaceBaseActivity type and its relationship with TProjectActivity.
Let me check the type definitions to understand the relationship between these types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for references to TProjectActivity to ensure they were removed or updated
rg "TProjectActivity" -A 5
Length of output: 2140
Script:
#!/bin/bash
# Check for TWorkspaceBaseActivity definition and usage
rg "TWorkspaceBaseActivity" -A 5
# Check the content of the activity-block.tsx file to understand the usage
cat web/core/components/common/activity/activity-block.tsx
Length of output: 4384
Script:
#!/bin/bash
# Check the content of the project activity type definition
cat web/ce/types/projects/project-activity.ts
# Check the base activity type definitions
cat packages/types/src/activity.d.ts
Length of output: 1031
Description
Updated activity-related components and types to modularize them for sharing across multiple components and platforms.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
EFileAssetTypeenum for expanded functionality.Usercomponent to use hooks for improved state management and reactivity.Bug Fixes
ActivityItemcomponent to prevent rendering with undefined activity types.Refactor
TProjectActivityinterface with a type alias for better structure.TWorkspaceBaseActivity, reflecting a shift in expected data structure.Chores