[WEB-2542] Fix: display filter and tooltip issues in list layout. #5696
Conversation
* fix: issue type display filter not working. * fix: layout shift when hovered on bulkops checkbox.
WalkthroughThe pull request includes multiple modifications across various files. Key changes involve the reintroduction of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/app/profile/page.tsx (1 hunks)
- web/core/components/issues/issue-layouts/list/block.tsx (3 hunks)
🔇 Additional comments not posted (5)
web/core/components/issues/issue-layouts/list/block.tsx (3)
133-133: Improved efficiency in keyMinWidth calculationThe modification to calculate
keyMinWidthonly whendisplayProperties.keyis present is a good optimization. It prevents unnecessary calculations and aligns with the PR objective of fixing display filter issues.
296-296: Minor formatting adjustmentThe change on this line appears to be a minor formatting adjustment, likely for improved code readability. This doesn't affect the component's functionality.
Line range hint
1-296: Overall assessment of changesThe modifications to the IssueBlock component align well with the PR objectives of fixing display filter issues. The changes improve efficiency in calculations, enhance rendering flexibility, and maintain good coding practices. No major issues were identified, and the adjustments contribute positively to the component's functionality and maintainability.
web/app/profile/page.tsx (2)
38-38: LGTM: FileService import reintroducedThe reintroduction of the
FileServiceimport is necessary and correct. This service is used in thehandleDeletefunction to manage user file operations, specifically for deleting the user's avatar. The change aligns with the profile settings functionality and allows users to properly manage their profile pictures.
Line range hint
1-489: Verify PR objectives and scopeThe changes in this file (reintroducing the
FileServiceimport) don't directly address the PR objectives mentioned in the summary, which are fixing display filter and tooltip issues in the list layout. While the change in this file is valid and necessary for the profile settings functionality, it's important to ensure that the PR includes other files that address the mentioned display filter and tooltip issues.To verify the PR scope and objectives, please run the following script:
This script will help identify files that might be related to the PR objectives. Please review the output and ensure that the necessary changes for fixing display filter and tooltip issues in the list layout are included in the PR.
✅ Verification successful
PR objectives are adequately addressed across multiple files.
The updated shell script results show that several files related to list layout, display filter, and tooltips are included in this PR. This indicates that the PR comprehensively addresses the objectives of fixing display filter and tooltip issues in the list layout across the codebase, not just in
web/app/profile/page.tsx.
List Layout:
web/core/components/modules/quick-actions.tsxweb/core/components/ui/loader/layouts/index.ts- (and others)
Display Filter:
web/helpers/issue.helper.tsweb/core/components/issues/issue-layouts/root.tsx- (and others)
Tooltips:
space/core/components/editor/toolbar.tsxweb/core/components/workspace/sidebar/user-menu.tsx- (and others)
These changes collectively fulfill the PR's goals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for files related to list layout, display filter, and tooltips echo "Files potentially related to list layout:" rg -l "list.*layout" --type tsx --type ts echo "\nFiles potentially related to display filter:" rg -l "display.*filter" --type tsx --type ts echo "\nFiles potentially related to tooltips:" rg -l "tooltip" --type tsx --type tsLength of output: 498
Script:
#!/bin/bash # Description: Check for files related to list layout, display filter, and tooltips echo "Files potentially related to list layout:" rg -l "list.*layout" --glob "*.tsx" --glob "*.ts" echo "\nFiles potentially related to display filter:" rg -l "display.*filter" --glob "*.tsx" --glob "*.ts" echo "\nFiles potentially related to tooltips:" rg -l "tooltip" --glob "*.tsx" --glob "*.ts"Length of output: 11420
| {displayProperties && (displayProperties.key || displayProperties.issue_type) && ( | ||
| <div className="flex-shrink-0" style={{ minWidth: `${keyMinWidth}px` }}> | ||
| {issue.project_id && ( | ||
| <IssueIdentifier | ||
| issueId={issueId} | ||
| projectId={issue.project_id} | ||
| textContainerClassName="text-xs font-medium text-custom-text-300" | ||
| displayProperties={displayProperties} |
There was a problem hiding this comment.
💡 Codebase verification
IssueIdentifier does not utilize displayProperties.issue_type
The IssueIdentifier component currently only uses displayProperties.key to determine rendering. The displayProperties.issue_type property is not referenced or utilized within the component. Therefore, the change to include issue_type in the conditional rendering does not have any effect.
IssueIdentifiercomponent (web/ce/components/issues/issue-details/issue-identifier.tsx) only referencesdisplayProperties.key.
🔗 Analysis chain
Improved flexibility in IssueIdentifier rendering
The modification to render IssueIdentifier when either displayProperties.key or displayProperties.issue_type is present improves the component's flexibility. Passing the entire displayProperties object to IssueIdentifier is a good practice for maintaining consistency.
Please verify that the IssueIdentifier component correctly handles the new displayProperties prop. Run the following script to check its implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the IssueIdentifier component's handling of displayProperties
# Test: Search for the IssueIdentifier component definition
rg --type typescript -A 10 'export const IssueIdentifier'
Length of output: 94
Script:
#!/bin/bash
# Description: Verify the IssueIdentifier component's handling of displayProperties
# Test: Search for the IssueIdentifier component definition in .ts and .tsx files
rg -g "*.ts" -g "*.tsx" -A 10 'export const IssueIdentifier'
Length of output: 1212
Script:
#!/bin/bash
# Description: Verify the usage of displayProperties within IssueIdentifier component
# Test: Search for usage of displayProperties in issue-identifier.tsx
rg -g 'issue-identifier.tsx' 'displayProperties'
Length of output: 439
Script:
#!/bin/bash
# Description: Verify all usages of displayProperties within IssueIdentifier component
# Test: Search for specific properties of displayProperties in issue-identifier.tsx
rg -g 'issue-identifier.tsx' 'displayProperties\.(key|issue_type)'
Length of output: 206
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
web/core/local-db/utils/query.utils.ts (1)
Line range hint
1-383: Consider refactoring for improved maintainability and readability.While the specific change in this PR is good, I've noticed some areas where the overall file could be improved:
Some functions, like
getFilteredRowsForGrouping, are quite long and complex. Consider breaking them down into smaller, more focused functions to improve readability and maintainability.There's repeated logic in functions like
getMetaKeysFragmentandgetMetaKeys. Consider extracting common operations (e.g., checking ARRAY_FIELDS) into helper functions to reduce duplication.The file lacks comments explaining the purpose and behavior of complex SQL-generating functions. Adding documentation for these functions would greatly improve code understandability.
Consider using TypeScript interfaces or types for function parameters to improve type safety and self-documentation.
These suggestions are outside the scope of the current PR but could be considered for future refactoring efforts to improve the overall code quality of this file.
web/core/local-db/storage.sqlite.ts (2)
58-58: Approved: Improved comment clarity, minor suggestion for formattingThe added comment provides helpful context about the purpose of this operation, which is to clear the local issue cache. This improvement in documentation is beneficial for code maintainability.
Consider adjusting the comment formatting for better readability:
-//@ts-expect-error , clear local issue cache +// @ts-expect-error - Clear local issue cacheThis format:
- Adds a space after the double slash for consistency with typical comment styles.
- Uses a hyphen to separate the TypeScript directive from the explanation.
- Removes the comma and extra space for cleaner formatting.
Line range hint
55-62: Consider improving type safety and documentation for File System Access API usageThe
clearStoragemethod uses the File System Access API, which is a relatively new web API. To improve type safety and code clarity:
- Consider adding type definitions for
fileSystemDirectoryHandleif they're not available in your current TypeScript setup.- Add a brief comment explaining the use of the File System Access API and any browser compatibility considerations.
- If possible, replace the
@ts-expect-errorwith more specific type annotations or assertions.Here's a suggested improvement:
interface FileSystemDirectoryHandle { remove(options?: { recursive: boolean }): Promise<void>; } clearStorage = async () => { try { const storageManager = window.navigator.storage; // Using File System Access API to get the root directory const fileSystemDirectoryHandle = await storageManager.getDirectory() as FileSystemDirectoryHandle; // Clear the entire directory recursively await fileSystemDirectoryHandle.remove({ recursive: true }); } catch (e) { console.error("Error clearing sqlite sync storage", e); } };This change adds type safety and removes the need for
@ts-expect-error. Remember to check browser compatibility and add appropriate fallbacks if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- web/core/layouts/auth-layout/project-wrapper.tsx (1 hunks)
- web/core/layouts/auth-layout/workspace-wrapper.tsx (1 hunks)
- web/core/lib/polyfills/requestIdleCallback.ts (1 hunks)
- web/core/local-db/storage.sqlite.ts (1 hunks)
- web/core/local-db/utils/query.utils.ts (1 hunks)
- web/core/store/project/project.store.ts (1 hunks)
- web/core/store/user/index.ts (1 hunks)
- web/core/store/user/profile.store.ts (2 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/core/layouts/auth-layout/project-wrapper.tsx
- web/core/layouts/auth-layout/workspace-wrapper.tsx
🔇 Additional comments not posted (7)
web/core/lib/polyfills/requestIdleCallback.ts (2)
6-6: Approved: Good use ofconstfor immutable variable.The change from
vartoconstfor thestartvariable is a positive improvement. It aligns with modern JavaScript best practices by usingconstfor variables that aren't reassigned. This change enhances code clarity by explicitly declaring the immutability ofstart, which accurately reflects its usage within the function.While this modification doesn't alter the functionality of the
requestIdleCallbackpolyfill, it contributes to better code quality and maintainability.
Line range hint
1-24: Approved: Well-implemented polyfill forrequestIdleCallback.The overall implementation of the
requestIdleCallbackandcancelIdleCallbackpolyfills is well-done. Here are some positive aspects of the implementation:
- It correctly checks for the existence of
windowand only applies the polyfill if necessary.- The use of the nullish coalescing operator (
??) for fallback is a modern and clean approach.- The
timeRemainingfunction provides a reasonable approximation of idle time.- The implementation is properly encapsulated as a module with the
export {}statement.While this polyfill doesn't perfectly mimic the behavior of native
requestIdleCallback(as it doesn't consider actual browser idle time), it provides a good approximation that should work well for most use cases.web/core/store/user/profile.store.ts (1)
84-88: LGTM: Semicolon added for consistency.The addition of the semicolon at the end of the
mutateUserProfilemethod is a minor stylistic improvement that enhances code consistency.web/core/store/user/index.ts (2)
Line range hint
1-324: LGTM! The import change is well-integrated and doesn't introduce any issues.The change in the import statement for ENABLE_LOCAL_DB_CACHE is the only modification in this file. The usage of this constant in the
localDBEnabledcomputed property remains unchanged and correct. The rest of the file, including the UserStore class implementation and its methods, is not affected by this change.
12-12: LGTM! Verify usage of ENABLE_LOCAL_DB_CACHE in the codebase.The import change for ENABLE_LOCAL_DB_CACHE looks good. It's now imported from "@/plane-web/constants/issues", which suggests a reorganization of the codebase. The functionality using this constant remains unchanged in this file.
To ensure consistency across the codebase, let's verify the usage of ENABLE_LOCAL_DB_CACHE:
This script will help us confirm that all occurrences of ENABLE_LOCAL_DB_CACHE are imported from the correct location and used consistently throughout the codebase.
✅ Verification successful
Verified: The import and usage of
ENABLE_LOCAL_DB_CACHEare consistent across the codebase. All instances correctly import from@/plane-web/constants/issues, and the constant is used as intended in the respective files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of ENABLE_LOCAL_DB_CACHE across the codebase # Search for import statements of ENABLE_LOCAL_DB_CACHE echo "Searching for import statements of ENABLE_LOCAL_DB_CACHE:" rg --type typescript --type javascript "import.*ENABLE_LOCAL_DB_CACHE.*from" # Search for usage of ENABLE_LOCAL_DB_CACHE echo "\nSearching for usage of ENABLE_LOCAL_DB_CACHE:" rg --type typescript --type javascript "ENABLE_LOCAL_DB_CACHE"Length of output: 441
Script:
#!/bin/bash # Description: Verify the usage of ENABLE_LOCAL_DB_CACHE across the codebase # Search for import statements of ENABLE_LOCAL_DB_CACHE echo "Searching for import statements of ENABLE_LOCAL_DB_CACHE in TypeScript files:" rg "import\s+.*ENABLE_LOCAL_DB_CACHE.*from" --glob "*.ts" --glob "*.tsx" # Search for usage of ENABLE_LOCAL_DB_CACHE echo -e "\nSearching for usages of ENABLE_LOCAL_DB_CACHE in TypeScript files:" rg "ENABLE_LOCAL_DB_CACHE" --glob "*.ts" --glob "*.tsx"Length of output: 1062
web/core/local-db/utils/query.utils.ts (1)
80-80: Appropriate use ofconstfor immutable variable.The change from
lettoconstfor thesqlvariable declaration is a good improvement. Since the variable is not reassigned within the function, usingconstis more appropriate. This change:
- Prevents accidental reassignment of the variable.
- Clearly communicates the intent that this variable should not be modified after initialization.
- Aligns with modern JavaScript best practices for variable declarations.
This modification enhances code quality without affecting the functionality.
web/core/store/project/project.store.ts (1)
Line range hint
395-411: Approve the change with a suggestion to verify error handling.The addition of
throw error;after logging and fetching projects is a good improvement in error handling. It allows the error to propagate up the call stack, which can be beneficial for error reporting and handling at higher levels of the application.However, this change might affect how errors are handled by the calling code. Please verify that any code calling the
deleteProjectmethod is prepared to handle the thrown error. Run the following script to find potential callers of this method:
| @@ -129,7 +129,7 @@ export class ProfileStore implements IUserProfileStore { | |||
| } | |||
| const userProfile = await this.userService.updateCurrentUserProfile(data); | |||
| return userProfile; | |||
| } catch (error) { | |||
| } catch { | |||
There was a problem hiding this comment.
Consider keeping the error parameter for debugging.
While the current implementation still updates the error state with a generic message, removing the error parameter from the catch block means that specific error information is lost. This could make it more difficult to diagnose issues if they occur.
Consider keeping the error parameter and logging it for debugging purposes. Here's a suggested change:
- } catch {
+ } catch (error) {
+ console.error("Error updating user profile:", error);
if (currentUserProfileData) {
this.mutateUserProfile(currentUserProfileData);
}This way, you maintain the generic user-facing error message while still having access to the specific error details for debugging.
📝 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.
| } catch { | |
| } catch (error) { | |
| console.error("Error updating user profile:", error); |
Summary by CodeRabbit
New Features
Bug Fixes