fix: ln support for views constants #6431
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of view-related constants and imports across multiple files. The primary changes involve moving Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
🔭 Outside diff range comments (1)
web/core/store/global-view.store.ts (1)
Line range hint
132-136: Consider adding error recovery for failed view access updates.The view access update in
createGlobalViewlacks error handling. If the access update fails after view creation, the view's state might become inconsistent.Consider wrapping the access update in a try-catch block and implementing a rollback mechanism:
if (data.access === EViewAccess.PRIVATE) { - await this.updateViewAccess(workspaceSlug, response.id, EViewAccess.PRIVATE); + try { + await this.updateViewAccess(workspaceSlug, response.id, EViewAccess.PRIVATE); + } catch (error) { + // Rollback view creation + await this.deleteGlobalView(workspaceSlug, response.id); + throw error; + } }
🧹 Nitpick comments (5)
web/helpers/views.helper.ts (1)
4-7: Consider using Record type for icon mapping.The icon mapping implementation is correct, but could be more type-safe using Record.
-const VIEW_ACCESS_ICONS = { +const VIEW_ACCESS_ICONS: Record<EViewAccess, LucideIcon> = { [EViewAccess.PUBLIC]: Globe2, [EViewAccess.PRIVATE]: Lock, };packages/constants/src/views.ts (1)
1-4: Consider using string enum for explicit values.The current numeric enum implementation might lead to serialization issues. Consider using string values for better maintainability and explicit serialization.
export enum EViewAccess { - PRIVATE, - PUBLIC, + PRIVATE = "PRIVATE", + PUBLIC = "PUBLIC", }web/core/components/views/filters/filter-selection.tsx (1)
Line range hint
31-45: Simplify filter handling logic.The current implementation could be simplified to improve readability and maintainability. Consider using Set operations or array methods like filter/includes.
- if (Array.isArray(currValues)) { - if (Array.isArray(value)) { - value.forEach((val) => { - if (!currValues.includes(val)) currValues.push(val); - else currValues.splice(currValues.indexOf(val), 1); - }); - } else if (typeof value !== "boolean") { - if (currValues?.includes(value)) currValues.splice(currValues.indexOf(value), 1); - else currValues.push(value); - } - } + if (Array.isArray(currValues)) { + const valueArray = Array.isArray(value) ? value : [value]; + if (typeof value !== "boolean") { + const uniqueValues = new Set(currValues); + valueArray.forEach(val => + uniqueValues.has(val) ? uniqueValues.delete(val) : uniqueValues.add(val) + ); + currValues = Array.from(uniqueValues); + } + }web/core/components/workspace/views/form.tsx (1)
Line range hint
29-37: Consider adding JSDoc documentation for defaultValues.Adding documentation would help explain the significance of the default access level and other properties.
+/** + * Default values for workspace view creation/update + * @property {string} name - Empty string as default name + * @property {string} description - Empty string as default description + * @property {EViewAccess} access - Default to PUBLIC access for better discoverability + * @property {IIssueDisplayProperties} display_properties - Computed display properties + * @property {IIssueDisplayFilterOptions} display_filters - Computed display filters with spreadsheet layout + */ const defaultValues: Partial<IWorkspaceView> = { name: "", description: "", access: EViewAccess.PUBLIC, display_properties: getComputedDisplayProperties(), display_filters: getComputedDisplayFilters({ layout: EIssueLayoutTypes.SPREADSHEET, order_by: "-created_at", }), };web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(detail)/[viewId]/header.tsx (1)
Line range hint
132-138: Consider improving accessibility for private view indicator.The private view indicator needs accessibility improvements.
Add ARIA attributes to improve accessibility:
{viewDetails?.access === EViewAccess.PRIVATE ? ( - <div className="cursor-default text-custom-text-300"> + <div className="cursor-default text-custom-text-300" role="status" aria-label="Private view"> <Tooltip tooltipContent={"Private"}> - <Lock className="h-4 w-4" /> + <Lock className="h-4 w-4" aria-hidden="true" /> </Tooltip> </div> ) : ( <></> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
packages/constants/src/index.ts(1 hunks)packages/constants/src/views.ts(1 hunks)packages/i18n/src/locales/en/translations.json(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(detail)/[viewId]/header.tsx(1 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/page.tsx(1 hunks)web/ce/services/project/view.service.ts(1 hunks)web/ce/services/workspace.service.ts(1 hunks)web/core/components/issues/issue-layouts/filters/applied-filters/roots/global-view-root.tsx(1 hunks)web/core/components/issues/issue-layouts/filters/applied-filters/roots/project-view-root.tsx(1 hunks)web/core/components/stickies/layout/stickies-list.tsx(1 hunks)web/core/components/views/applied-filters/access.tsx(1 hunks)web/core/components/views/applied-filters/root.tsx(1 hunks)web/core/components/views/filters/filter-selection.tsx(1 hunks)web/core/components/views/filters/order-by.tsx(4 hunks)web/core/components/views/form.tsx(1 hunks)web/core/components/views/view-list-item-action.tsx(1 hunks)web/core/components/workspace/views/form.tsx(1 hunks)web/core/components/workspace/views/quick-action.tsx(1 hunks)web/core/constants/empty-state.tsx(1 hunks)web/core/constants/views.ts(0 hunks)web/core/store/global-view.store.ts(1 hunks)web/core/store/project-view.store.ts(1 hunks)web/helpers/views.helper.ts(1 hunks)
💤 Files with no reviewable changes (1)
- web/core/constants/views.ts
✅ Files skipped from review due to trivial changes (6)
- web/ce/services/workspace.service.ts
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/page.tsx
- web/ce/services/project/view.service.ts
- web/core/store/project-view.store.ts
- web/core/constants/empty-state.tsx
- web/core/components/workspace/views/quick-action.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (17)
packages/constants/src/index.ts (1)
17-17: LGTM! Export follows consistent pattern.The addition of the views module export aligns with the existing pattern and supports the centralization of view-related constants.
web/helpers/views.helper.ts (1)
8-15: LGTM! Type-safe implementation of access specifiers.The implementation correctly combines the base constants with icons while maintaining type safety.
packages/constants/src/views.ts (1)
20-23: Verify i18n key existence for order options.The order options use common i18n keys. Let's verify their existence.
✅ Verification successful
i18n keys verified successfully
The translation keys
common.order_by.ascandcommon.order_by.descexist in the English translations with appropriate values "Ascending" and "Descending" respectively.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the common order by keys in translation files rg -l "common\.order_by\.(asc|desc)" --type yamlLength of output: 50
Script:
#!/bin/bash # Find potential translation files echo "=== Looking for translation files ===" fd -e json -e yaml -e yml -e ts "locale|translation|i18n|lang" echo -e "\n=== Searching for order_by keys with variations ===" rg "order_by|orderBy" -lLength of output: 7088
Script:
#!/bin/bash # Check the English translation file for the specific keys and their context echo "=== Checking English translations ===" cat packages/i18n/src/locales/en/translations.json | jq '.common.order_by // empty' echo -e "\n=== Checking full common namespace for similar keys ===" cat packages/i18n/src/locales/en/translations.json | jq '.common | keys' 2>/dev/nullLength of output: 438
web/core/components/views/applied-filters/access.tsx (1)
5-7: LGTM! Import changes align with constant centralization.The updated imports correctly reference the new locations while maintaining type safety.
web/core/components/views/filters/order-by.tsx (3)
5-6: LGTM! Good improvement in modularity.Moving constants to @plane/constants and adding i18n support improves maintainability and internationalization.
63-80: Good improvement in sort options handling.The refactoring to use VIEW_SORT_BY_OPTIONS with proper type checking and selected state handling is a good improvement. The code is now more maintainable and type-safe.
39-39: Verify translations are available for all labels.The code now uses the translation function for all labels. Ensure that all the required translation keys exist in the i18n files.
Also applies to: 54-54, 58-58, 76-76
✅ Verification successful
All required translation keys are present in the i18n files.
The translation keys used in the component:
project_view.sort_by.nameproject_view.sort_by.created_atproject_view.sort_by.updated_atcommon.order_by.asccommon.order_by.descAll these keys are properly defined in the translation files with appropriate translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all required translation keys exist rg -A 1 't\((.*?)\)' web/core/components/views/filters/order-by.tsx | grep -o '".*"' | tr -d '"' | while read -r key; do echo "Checking translation key: $key" rg "\"$key\":" packages/i18n/src/locales/ doneLength of output: 538
Script:
#!/bin/bash # First, let's see the actual content around these lines to understand the translation keys echo "=== Checking lines 39, 54, 58, 76 ===" rg -A 1 -B 1 --line-number "t\(" web/core/components/views/filters/order-by.tsx echo -e "\n=== Checking translation function calls ===" # Use ast-grep to find all t() function calls ast-grep --pattern 't($_)' web/core/components/views/filters/order-by.tsx echo -e "\n=== Checking orderByDetails usage ===" # Look for orderByDetails object structure rg "orderByDetails" web/core/components/views/filters/order-by.tsxLength of output: 1449
Script:
#!/bin/bash # Find VIEW_SORTING_KEY_OPTIONS definition echo "=== Finding VIEW_SORTING_KEY_OPTIONS definition ===" rg -A 10 "VIEW_SORTING_KEY_OPTIONS\s*=" --type ts echo -e "\n=== Looking for translation files ===" fd . packages/i18n/src/locales/ -e json -x cat {}Length of output: 80155
web/core/components/views/applied-filters/root.tsx (1)
2-2: Good type safety improvement.The enhanced type definition for
handleRemoveFilterto includenullimproves type safety and makes the filter removal handling more robust.Also applies to: 16-16
web/core/components/views/view-list-item-action.tsx (1)
6-6: LGTM! Consistent access control implementation.The use of centralized EViewAccess constant and its implementation in the access control UI is correct and consistent with the rest of the codebase.
Also applies to: 89-91
web/core/components/issues/issue-layouts/filters/applied-filters/roots/project-view-root.tsx (1)
9-9: LGTM! Import consolidation improves maintainability.The migration of
EViewAccessto@plane/constantshelps centralize view-related constants.web/core/components/stickies/layout/stickies-list.tsx (1)
15-15: LGTM! Clean import organization.The import statement is properly placed in the components section.
web/core/components/issues/issue-layouts/filters/applied-filters/roots/global-view-root.tsx (1)
9-9: LGTM! Consistent import consolidation.The migration of
EViewAccessto@plane/constantsmaintains consistency with other files.web/core/components/workspace/views/form.tsx (1)
7-7: LGTM! Import consolidation maintains consistency.The migration of
EViewAccessto@plane/constantsaligns with the codebase-wide consolidation.web/core/store/global-view.store.ts (1)
7-7: LGTM! Import statement updated correctly.The import statement has been updated to include
EViewAccessfrom@plane/constants, aligning with the centralization of view-related constants.web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(detail)/[viewId]/header.tsx (1)
9-9: LGTM! Import statement updated correctly.The import statement has been updated to include
EViewAccessfrom@plane/constants, maintaining consistency with the centralization of view-related constants.web/core/components/views/form.tsx (1)
8-8: LGTM! Import statement updated correctly.The import statement has been updated to include
EViewAccessfrom@plane/constants, maintaining consistency across the codebase.packages/i18n/src/locales/en/translations.json (1)
320-336: LGTM! Translations added for all required strings.The translations have been properly added for:
- View access levels (public/private)
- Sort by options (created_at/updated_at/name)
- Common order by options (ascending/descending)
packages/constants/src/views.ts
Outdated
| export const VIEW_SORTING_KEY_OPTIONS = [ | ||
| { key: "name", label: "project_view.sort_by.name" }, | ||
| { key: "created_at", label: "project_view.sort_by.created_at" }, | ||
| { key: "updated_at", label: "project_view.sort_by.updated_at" }, | ||
| ]; |
There was a problem hiding this comment.
💡 Codebase verification
Missing translations for sorting options in non-English locales
The i18n keys exist in English translations but appear to be missing in other language files (es, fr, ja, zh-CN). This could lead to fallback text being displayed for non-English users.
packages/i18n/src/locales/es/translations.json: Missing sort_by translationspackages/i18n/src/locales/fr/translations.json: Missing sort_by translationspackages/i18n/src/locales/ja/translations.json: Missing sort_by translationspackages/i18n/src/locales/zh-CN/translations.json: Missing sort_by translations
🔗 Analysis chain
Verify i18n key existence for sorting options.
The sorting options use i18n keys. Let's verify their existence in the translation files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the i18n keys in translation files
rg -l "project_view\.sort_by\.(name|created_at|updated_at)" --type yaml
Length of output: 73
Script:
#!/bin/bash
# First, let's find translation files
echo "=== Looking for translation files ==="
fd -e yml -e yaml -e json .
echo -e "\n=== Searching for project_view keys in YAML files ==="
rg "project_view" --type yaml -A 5 -B 5
echo -e "\n=== Searching for project_view keys in JSON files ==="
rg "project_view" --type json -A 5 -B 5
Length of output: 2850
Script:
#!/bin/bash
# Search for sort_by keys in all translation files with more context
echo "=== Checking sort_by keys in all translation files ==="
rg "sort_by" packages/i18n/src/locales/*/translations.json -A 10 -B 2
Length of output: 1233
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/helpers/views.helper.ts (1)
8-15: Consider removing unnecessary type assertion.The type assertion in
option.key as keyof typeof VIEW_ACCESS_ICONSmight be redundant sinceoption.keyis already of typeEViewAccess, which matches the keys inVIEW_ACCESS_ICONS.Try this simpler version:
export const VIEW_ACCESS_SPECIFIERS: { key: EViewAccess; i18n_label: string; icon: LucideIcon; }[] = VIEW_ACCESS_SPECIFIERS_CONSTANTS.map((option) => ({ ...option, - icon: VIEW_ACCESS_ICONS[option.key as keyof typeof VIEW_ACCESS_ICONS], + icon: VIEW_ACCESS_ICONS[option.key], }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/constants/src/views.ts(1 hunks)web/core/components/views/applied-filters/access.tsx(2 hunks)web/core/components/views/filters/order-by.tsx(4 hunks)web/helpers/views.helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/constants/src/views.ts
- web/core/components/views/applied-filters/access.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
web/core/components/views/filters/order-by.tsx (2)
5-6: LGTM! Improved modularity by centralizing constants.The imports have been properly reorganized to use centralized constants from
@plane/constantsand added i18n support.
21-21: LGTM! Consistent internationalization implementation.All user-facing strings have been properly internationalized using the translation hook.
Also applies to: 39-39, 58-58, 76-76
web/helpers/views.helper.ts (3)
1-2: LGTM! Clean and well-organized imports.Good practice using explicit imports and clear constant renaming to avoid confusion.
4-7: LGTM! Type-safe and intuitive icon mapping.The constant uses enum values as keys, ensuring type safety, and the chosen icons clearly represent their respective access levels.
1-15: Verify the constants migration and usage.Since this is part of a larger refactoring to centralize view-related constants, let's verify the setup and usage.
Run this script to check the constants and their usage:
✅ Verification successful
Constants migration is properly implemented ✅
The view-related constants are correctly centralized in
@plane/constantsand are being consistently used across the codebase. The helper file appropriately extends these constants with UI-specific icon mappings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the constants exist in @plane/constants echo "Checking @plane/constants for view constants..." ast-grep --pattern 'export const VIEW_ACCESS_SPECIFIERS' ast-grep --pattern 'export enum EViewAccess' # Check for usage of the new constants echo -e "\nChecking usage of the new constants..." rg -t typescript "VIEW_ACCESS_SPECIFIERS.*from.*@plane/constants" --glob '!node_modules' rg -t typescript "EViewAccess.*from.*@plane/constants" --glob '!node_modules'Length of output: 1521
Script:
#!/bin/bash # Check for usage of the new constants without typescript flag echo "Checking usage of the new constants..." rg "VIEW_ACCESS_SPECIFIERS.*from.*@plane/constants" --glob '!node_modules' rg "EViewAccess.*from.*@plane/constants" --glob '!node_modules' # Check for the enum definition echo -e "\nChecking for EViewAccess enum definition..." rg "enum EViewAccess" --glob '!node_modules'Length of output: 2478
| {VIEW_SORT_BY_OPTIONS.map((option) => { | ||
| const isSelected = (option.key === "asc" && !isDescending) || (option.key === "desc" && isDescending); | ||
| return ( | ||
| <CustomMenu.MenuItem | ||
| key={option.key} | ||
| className="flex items-center justify-between gap-2" | ||
| onClick={() => { | ||
| if (!isSelected) | ||
| onChange({ | ||
| order: option.key as TViewFiltersSortBy, | ||
| }); | ||
| }} | ||
| > | ||
| {t(option.i18n_label)} | ||
| {isSelected && <Check className="h-3 w-3" />} | ||
| </CustomMenu.MenuItem> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary condition in onClick handler.
The current implementation prevents toggling back to the default state when a user clicks on an already selected sort option. This creates a suboptimal user experience.
Consider this improvement:
{VIEW_SORT_BY_OPTIONS.map((option) => {
const isSelected = (option.key === "asc" && !isDescending) || (option.key === "desc" && isDescending);
return (
<CustomMenu.MenuItem
key={option.key}
className="flex items-center justify-between gap-2"
onClick={() => {
- if (!isSelected)
onChange({
order: option.key as TViewFiltersSortBy,
});
}}
>
{t(option.i18n_label)}
{isSelected && <Check className="h-3 w-3" />}
</CustomMenu.MenuItem>
);
})}This change allows users to toggle between ascending and descending sort orders freely.
📝 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.
| {VIEW_SORT_BY_OPTIONS.map((option) => { | |
| const isSelected = (option.key === "asc" && !isDescending) || (option.key === "desc" && isDescending); | |
| return ( | |
| <CustomMenu.MenuItem | |
| key={option.key} | |
| className="flex items-center justify-between gap-2" | |
| onClick={() => { | |
| if (!isSelected) | |
| onChange({ | |
| order: option.key as TViewFiltersSortBy, | |
| }); | |
| }} | |
| > | |
| {t(option.i18n_label)} | |
| {isSelected && <Check className="h-3 w-3" />} | |
| </CustomMenu.MenuItem> | |
| ); | |
| })} | |
| {VIEW_SORT_BY_OPTIONS.map((option) => { | |
| const isSelected = (option.key === "asc" && !isDescending) || (option.key === "desc" && isDescending); | |
| return ( | |
| <CustomMenu.MenuItem | |
| key={option.key} | |
| className="flex items-center justify-between gap-2" | |
| onClick={() => { | |
| onChange({ | |
| order: option.key as TViewFiltersSortBy, | |
| }); | |
| }} | |
| > | |
| {t(option.i18n_label)} | |
| {isSelected && <Check className="h-3 w-3" />} | |
| </CustomMenu.MenuItem> | |
| ); | |
| })} |
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
Release Notes
New Features
Improvements
@plane/constants.Localization
These changes improve view management and provide more granular control over project and workspace views.