Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WEB-2678]feat: added functionality to add labels directly from dropdown #6208

Closed
wants to merge 7 commits into from

Conversation

mathalav55
Copy link
Collaborator

@mathalav55 mathalav55 commented Dec 16, 2024

Description

Added functionality to add labels directly from drop-down if the label does not exist.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Improvement (change that would cause existing functionality to not work as expected)
  • Code refactoring
  • Performance improvements
  • Documentation update

References

WEB-2678

Summary by CodeRabbit

  • New Features

    • Users can now create new labels directly from the label selection interface.
    • Enhanced submission tracking for label creation with visual feedback (loading spinner).
    • New functionality to add labels when pressing the "Enter" key during label input.
    • Added support for internationalization with multiple language translations available.
  • Bug Fixes

    • Improved error handling for label creation, ensuring appropriate notifications for users.
  • Documentation

    • Updated component props to reflect new functionalities related to label creation.

@mathalav55 mathalav55 added 🌟enhancement New feature or request 🌐frontend labels Dec 16, 2024
@mathalav55 mathalav55 added this to the v0.24.2 milestone Dec 16, 2024
@mathalav55 mathalav55 self-assigned this Dec 16, 2024
Copy link
Contributor

coderabbitai bot commented Dec 16, 2024

Walkthrough

The pull request introduces changes to the label management components across multiple files in the web application. The modifications primarily focus on simplifying label creation and handling by removing explicit user permission checks and adding a more streamlined mechanism for creating labels directly from various issue-related components. The changes enhance the label selection and creation process by introducing a new onAddLabel method and improving the user interaction flow for adding labels.

Changes

File Change Summary
web/core/components/issues/issue-detail/label/root.tsx Removed useUserPermissions import and related permission checks, simplifying label creation logic.
web/core/components/issues/issue-detail/label/select/label-select.tsx Added onAddLabel method, new submitting state, and enhanced label creation logic.
web/core/components/issues/issue-detail/label/select/root.tsx Added onAddLabel prop to IssueLabelSelect component.
web/core/components/issues/issue-layouts/properties/labels.tsx Introduced handleAddLabel function, updated useLabel hook, added label creation capabilities.
packages/i18n/.eslintignore Added entries to ignore generated files during linting.
packages/i18n/.eslintrc.js Introduced ESLint configuration for the i18n package.
packages/i18n/.prettierignore Created .prettierignore file to specify files and directories to be ignored by Prettier.
packages/i18n/.prettierrc Added Prettier configuration file with specific formatting options.
packages/i18n/package.json Introduced package.json for the @plane/i18n package with dependencies and scripts.
packages/i18n/src/components/index.tsx Added TranslationProvider and TranslationContext for managing translations.
packages/i18n/src/components/store.ts Introduced TranslationStore class for managing language translations.
packages/i18n/src/config/index.ts Defined language support and translations with constants and types.
packages/i18n/src/hooks/use-translation.ts Added useTranslation custom hook for accessing translation context.
web/app/[workspaceSlug]/(projects)/layout.tsx Updated import path for WorkspaceAuthWrapper.
web/app/profile/page.tsx Replaced ProfileSettingsPage with ProfileForm component for profile management.
web/app/profile/sidebar.tsx Updated component to use translation functionality.
web/core/lib/wrappers/store-wrapper.tsx Added language management functionality to StoreWrapper.

Possibly related PRs

  • [WEB-2467] fix: platform bug #5621: This PR involves modifications to permissions related to project labels, which connects to the changes in the main PR that removed user permission checks from the IssueLabel component, indicating a shift in how label creation is managed.
  • [WEB-2108] refactor: enhance workspace and project wrapper modularity #6207: This PR focuses on enhancing the modularity of workspace and project wrappers, which may relate to the overall structure and organization of components, including the IssueLabel component, although it does not directly address permissions or label management.

Suggested reviewers

  • sriramveeraghanta
  • SatishGandham
  • pablohashescobar

Poem

🐰 Hop, hop, labels take flight!
Permissions fade, simplicity's might
Enter key pressed, magic unfurls
New labels dance, our workflow twirls
Code rabbit sings of streamlined delight! 🏷️

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 438cc33 and 00c15da.

📒 Files selected for processing (4)
  • web/core/components/issues/issue-detail/label/root.tsx (1 hunks)
  • web/core/components/issues/issue-detail/label/select/label-select.tsx (3 hunks)
  • web/core/components/issues/issue-detail/label/select/root.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/properties/labels.tsx (7 hunks)
🔇 Additional comments (2)
web/core/components/issues/issue-detail/label/select/root.tsx (1)

29-29: LGTM!

The addition of the onAddLabel prop correctly passes the createLabel function to the IssueLabelSelect component, enhancing label creation functionality.

web/core/components/issues/issue-detail/label/root.tsx (1)

10-13: Simplification by removing permission checks is acceptable

The removal of useUserPermissions import and related permission checks simplifies the IssueLabel component. Assuming that permission checks are enforced elsewhere, this change is appropriate.

Comment on lines +106 to +111
const handleAddLabel = async (labelName: string) => {
setSubmitting(true);
const label = await onAddLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
onSelect([...values, label.id]);
setQuery("");
setSubmitting(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling in handleAddLabel to prevent unhandled exceptions

The handleAddLabel function does not handle potential exceptions from onAddLabel. If an error occurs during label creation, setSubmitting(false) may not be called, leaving the component in a loading state, and the user won't receive feedback about the failure.

Apply this diff to add error handling:

const handleAddLabel = async (labelName: string) => {
+  try {
    setSubmitting(true);
    const label = await onAddLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
    onSelect([...values, label.id]);
    setQuery("");
+  } catch (error) {
+    // Handle error, possibly show error message to user
+    console.error(error);
+  } finally {
    setSubmitting(false);
+  }
};
📝 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.

Suggested change
const handleAddLabel = async (labelName: string) => {
setSubmitting(true);
const label = await onAddLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
onSelect([...values, label.id]);
setQuery("");
setSubmitting(false);
const handleAddLabel = async (labelName: string) => {
try {
setSubmitting(true);
const label = await onAddLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
onSelect([...values, label.id]);
setQuery("");
} catch (error) {
// Handle error, possibly show error message to user
console.error(error);
} finally {
setSubmitting(false);
}
};

Comment on lines +265 to +273
const handleAddLabel = async (labelName: string) => {
if (!projectId) return;
setSubmitting(true);
const label = await createLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
onChange([...value, label.id]);
setQuery("");
setSubmitting(false);
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling in handleAddLabel to prevent unhandled exceptions

The handleAddLabel function does not handle potential exceptions from createLabel. If an error occurs during label creation, setSubmitting(false) may not be called, leaving the component in a loading state, and the user won't receive feedback about the failure.

Apply this diff to add error handling:

const handleAddLabel = async (labelName: string) => {
  if (!projectId) return;
+  try {
    setSubmitting(true);
    const label = await createLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
    onChange([...value, label.id]);
    setQuery("");
+  } catch (error) {
+    // Handle error, possibly show error message to user
+    console.error(error);
+  } finally {
    setSubmitting(false);
+  }
};
📝 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.

Suggested change
const handleAddLabel = async (labelName: string) => {
if (!projectId) return;
setSubmitting(true);
const label = await createLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
onChange([...value, label.id]);
setQuery("");
setSubmitting(false);
};
const handleAddLabel = async (labelName: string) => {
if (!projectId) return;
try {
setSubmitting(true);
const label = await createLabel(workspaceSlug, projectId, { name: labelName, color: getRandomLabelColor() });
onChange([...value, label.id]);
setQuery("");
} catch (error) {
// Handle error, possibly show error message to user
console.error(error);
} finally {
setSubmitting(false);
}
};

@mathalav55 mathalav55 marked this pull request as draft December 17, 2024 08:17
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (15)
web/core/components/issues/issue-layouts/kanban/block.tsx (1)

Line range hint 29-43: Enhance type safety for issue service types

Consider improving type safety by extending the IssueBlockProps interface to include a type discriminator for issue service types.

 interface IssueBlockProps {
   issueId: string;
   groupId: string;
   subGroupId: string;
   issuesMap: IIssueMap;
   displayProperties: IIssueDisplayProperties | undefined;
   draggableId: string;
   canDropOverIssue: boolean;
   canDragIssuesInCurrentGrouping: boolean;
   updateIssue: ((projectId: string | null, issueId: string, data: Partial<TIssue>) => Promise<void>) | undefined;
   quickActions: TRenderQuickActions;
   canEditProperties: (projectId: string | undefined) => boolean;
   scrollableContainerRef?: MutableRefObject<HTMLDivElement | null>;
   shouldRenderByDefault?: boolean;
-  isEpic?: boolean;
+  serviceType: EIssueServiceType;
 }

This change would:

  1. Make the service type explicit and required
  2. Prevent potential mismatches between service types
  3. Improve code maintainability
web/ce/layouts/project-wrapper.tsx (2)

6-8: Add JSDoc documentation for the interface.

Consider adding documentation to explain the purpose and usage of this interface.

+/**
+ * Interface for ProjectAuthWrapper props
+ * @property children - React nodes to be rendered within the wrapper
+ */
 export type IProjectAuthWrapper = {
   children: React.ReactNode;
 };

10-15: Consider simplifying the component hierarchy.

This wrapper component doesn't add any functionality over CoreProjectAuthWrapper. Consider either:

  1. Adding meaningful functionality to justify this abstraction layer, or
  2. Directly exporting CoreProjectAuthWrapper if no additional functionality is needed.
web/core/layouts/auth-layout/workspace-wrapper.tsx (1)

29-32: Consider standardizing loading state pattern

The addition of isLoading props to both wrapper components establishes a pattern for loading state propagation. Consider:

  1. Documenting this pattern for other wrapper components
  2. Creating a higher-order component or hook to standardize this behavior
  3. Adding this pattern to your component guidelines

This would ensure consistency as more wrapper components are added or modified.

Also applies to: 37-37

web/app/profile/page.tsx (2)

6-6: Remove unused import of PageHead.

The PageHead component is imported but not used in this file. Removing unused imports can help reduce bundle size and improve code readability.

Apply this diff to remove the unused import:

-import { PageHead } from "@/components/core";

26-26: Ensure currentUser and userProfile.data are necessary props for ProfileForm.

Passing both currentUser and userProfile.data to ProfileForm might be redundant if ProfileForm can access useUser() internally. Consider simplifying prop passing for better maintainability.

web/core/components/profile/form.tsx (4)

3-3: Review the necessity of importing Controller from react-hook-form.

The Controller component is imported but not used until later in the code. Ensure that all imports are necessary and consider organizing them logically.


55-55: Consider moving t outside the component for optimization.

The useTranslation hook is called inside the component. If the translation context does not depend on any props or state, consider moving it outside the component to prevent unnecessary re-renders.


106-129: Handle errors in handleProfilePictureDelete function.

Currently, the function only catches errors but does not handle specific cases or provide detailed feedback. Consider handling different error scenarios, such as network issues or permission errors, to improve user experience.


416-416: Fix incorrect className property.

The className property has an extra ! which may not have the intended effect and could lead to styling issues.

Apply this diff to correct the className:

-  className="rounded-md border-[0.5px] !border-custom-border-200"
+  className="rounded-md border-[0.5px] border-custom-border-200"
packages/i18n/src/config/index.ts (1)

7-11: Consider strengthening type safety for translations

The current Translations type allows any string key. Consider using a union type of known translation keys to catch typos and missing translations at compile time.

 export type Translations = {
-  [key: string]: {
-    [key: string]: string;
+  [K in Language]: {
+    [key: string]: string;
   };
 };
packages/i18n/src/components/store.ts (2)

27-37: Consider adding error handling for language initialization

The language initialization logic could benefit from error handling for cases where localStorage or navigator APIs are unavailable.

   initializeLanguage() {
-    if (typeof window === "undefined") return;
+    try {
+      if (typeof window === "undefined") return;
 
-    const savedLocale = localStorage.getItem("userLanguage") as Language;
-    if (savedLocale && languages.includes(savedLocale)) {
-      this.setLanguage(savedLocale);
-    } else {
-      const browserLang = navigator.language.split("-")[0] as Language;
-      const newLocale = languages.includes(browserLang as Language) ? (browserLang as Language) : fallbackLng;
-      this.setLanguage(newLocale);
+      const savedLocale = localStorage.getItem("userLanguage") as Language;
+      if (savedLocale && languages.includes(savedLocale)) {
+        this.setLanguage(savedLocale);
+        return;
+      }
+
+      const browserLang = navigator?.language?.split("-")[0] as Language;
+      const newLocale = languages.includes(browserLang as Language) ? browserLang : fallbackLng;
+      this.setLanguage(newLocale);
+    } catch (error) {
+      console.error('Failed to initialize language:', error);
+      this.setLanguage(fallbackLng);
     }
   }

22-25: Add error handling for setLanguage method

The setLanguage method should handle cases where localStorage is unavailable.

   setLanguage(lng: Language) {
-    localStorage.setItem("userLanguage", lng);
-    this.currentLocale = lng;
+    try {
+      localStorage.setItem("userLanguage", lng);
+      this.currentLocale = lng;
+    } catch (error) {
+      console.error('Failed to save language preference:', error);
+      this.currentLocale = lng;
+    }
   }
web/core/lib/wrappers/store-wrapper.tsx (1)

54-57: Consider optimizing the language change implementation.

A few suggestions to improve the language change logic:

  1. The changeLanguage function from useTranslation is likely stable and could be removed from the dependency array
  2. Consider adding type validation before casting to Language type

Here's a suggested improvement:

-  useEffect(() => {
-    if (!userProfile?.language) return;
-    changeLanguage(userProfile?.language as Language);
-  }, [userProfile?.language, changeLanguage]);
+  useEffect(() => {
+    if (!userProfile?.language) return;
+    
+    // Validate language type before casting
+    const isValidLanguage = (lang: string): lang is Language => 
+      ['en', 'fr', 'es', 'ja'].includes(lang);
+    
+    if (isValidLanguage(userProfile.language)) {
+      changeLanguage(userProfile.language);
+    }
+  }, [userProfile?.language]); // changeLanguage is stable from useTranslation
web/core/components/workspace/settings/invitations-list-item.tsx (1)

123-127: LGTM! Consider standardizing error message format.

The error handling improvements look good. However, for consistency with other error messages in the codebase, consider removing "Please try again." from the fallback message since it's already implied.

-                  message: error?.error || "An error occurred while updating member role. Please try again.",
+                  message: error?.error || "An error occurred while updating member role.",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00c15da and afd2d6b.

📒 Files selected for processing (41)
  • packages/i18n/.eslintignore (1 hunks)
  • packages/i18n/.eslintrc.js (1 hunks)
  • packages/i18n/.prettierignore (1 hunks)
  • packages/i18n/.prettierrc (1 hunks)
  • packages/i18n/package.json (1 hunks)
  • packages/i18n/src/components/index.tsx (1 hunks)
  • packages/i18n/src/components/store.ts (1 hunks)
  • packages/i18n/src/config/index.ts (1 hunks)
  • packages/i18n/src/hooks/index.ts (1 hunks)
  • packages/i18n/src/hooks/use-translation.ts (1 hunks)
  • packages/i18n/src/index.ts (1 hunks)
  • packages/i18n/src/locales/en/translations.json (1 hunks)
  • packages/i18n/src/locales/es/translations.json (1 hunks)
  • packages/i18n/src/locales/fr/translations.json (1 hunks)
  • packages/i18n/src/locales/ja/translations.json (1 hunks)
  • packages/i18n/tsconfig.json (1 hunks)
  • packages/types/src/users.d.ts (1 hunks)
  • packages/ui/src/modals/constants.ts (1 hunks)
  • web/app/[workspaceSlug]/(projects)/layout.tsx (1 hunks)
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx (0 hunks)
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(list)/header.tsx (0 hunks)
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/header.tsx (0 hunks)
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/layout.tsx (1 hunks)
  • web/app/onboarding/page.tsx (1 hunks)
  • web/app/profile/page.tsx (2 hunks)
  • web/app/profile/sidebar.tsx (9 hunks)
  • web/app/provider.tsx (2 hunks)
  • web/ce/components/de-dupe/de-dupe-button.tsx (1 hunks)
  • web/ce/layouts/project-wrapper.tsx (1 hunks)
  • web/ce/layouts/workspace-wrapper.tsx (1 hunks)
  • web/core/components/issues/issue-layouts/kanban/block.tsx (1 hunks)
  • web/core/components/profile/form.tsx (1 hunks)
  • web/core/components/profile/index.ts (1 hunks)
  • web/core/components/workspace/settings/invitations-list-item.tsx (1 hunks)
  • web/core/layouts/auth-layout/project-wrapper.tsx (3 hunks)
  • web/core/layouts/auth-layout/workspace-wrapper.tsx (4 hunks)
  • web/core/lib/wrappers/store-wrapper.tsx (3 hunks)
  • web/ee/layouts/project-wrapper.tsx (1 hunks)
  • web/ee/layouts/workspace-wrapper.tsx (1 hunks)
  • web/next.config.js (1 hunks)
  • web/package.json (1 hunks)
💤 Files with no reviewable changes (3)
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/cycles/(list)/header.tsx
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/views/(list)/header.tsx
  • web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/modules/(list)/header.tsx
✅ Files skipped from review due to trivial changes (15)
  • web/ee/layouts/project-wrapper.tsx
  • web/ee/layouts/workspace-wrapper.tsx
  • packages/i18n/.eslintignore
  • packages/i18n/.prettierrc
  • packages/i18n/.prettierignore
  • packages/i18n/src/hooks/index.ts
  • packages/i18n/tsconfig.json
  • packages/i18n/src/locales/en/translations.json
  • packages/i18n/.eslintrc.js
  • packages/i18n/src/index.ts
  • packages/i18n/src/locales/ja/translations.json
  • web/ce/components/de-dupe/de-dupe-button.tsx
  • web/core/components/profile/index.ts
  • packages/i18n/package.json
  • packages/i18n/src/locales/es/translations.json
🔇 Additional comments (23)
web/core/components/issues/issue-layouts/kanban/block.tsx (2)

9-9: LGTM: Clean import addition

The addition of EIssueServiceType import follows the existing import organization pattern and is necessary for type-safe issue service differentiation.


Line range hint 123-123: Verify the impact of service type differentiation

The hook now differentiates between epics and issues using EIssueServiceType. Let's verify this change doesn't affect existing peek functionality.

✅ Verification successful

Based on the search results, I can now generate the final response as I have gathered sufficient information about the usage of useIssueDetail hook and peek functionality across the codebase.

Service type differentiation is consistently implemented

The changes to the useIssueDetail hook's service type parameter are properly reflected across the codebase. Key findings:

  • All peek-related components correctly handle the service type differentiation:
    • useIssuePeekOverviewRedirection.tsx uses the correct service type based on isEpic
    • Issue layouts (kanban, list, spreadsheet) consistently pass the service type parameter
    • The peek functionality remains intact with proper service type handling

The implementation maintains consistent behavior for both epics and regular issues, with no adverse effects on the existing peek functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent usage of useIssueDetail hook with service types

# Search for other useIssueDetail hook usages
echo "Checking useIssueDetail hook usage patterns:"
rg "useIssueDetail\(" -A 2

# Search for potential peek-related components that might need similar updates
echo "\nChecking related peek components:"
rg "issuePeek|getIsIssuePeeked" -A 2

Length of output: 47007

web/app/[workspaceSlug]/(projects)/projects/(detail)/layout.tsx (1)

4-5: Verify the architectural reorganization impact.

The import path change from @/layouts/auth-layout to @/plane-web/layouts/project-wrapper suggests a significant architectural reorganization. While the change itself is clean, it seems broader than what's needed for adding label dropdown functionality.

✅ Verification successful

Import path change is part of a broader architectural reorganization

The change from @/layouts/auth-layout to @/plane-web/layouts appears to be part of a systematic architectural reorganization:

  • The old path @/layouts/auth-layout is still used in CE-specific components (web/ce/layouts/)
  • The new path @/plane-web/layouts is consistently used in the main app components
  • The project wrapper component exists in multiple variants:
    • Core implementation: web/core/layouts/auth-layout/project-wrapper.tsx
    • CE implementation: web/ce/layouts/project-wrapper.tsx
    • EE implementation: web/ee/layouts/project-wrapper.tsx

This architectural change appears intentional, organizing components based on their edition (CE/EE) and maintaining proper separation of concerns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if this architectural change is consistent across the codebase
echo "Checking for remaining references to old auth-layout path..."
rg "from \"@/layouts/auth-layout\"" --type ts --type tsx

echo "Checking for adoption of new plane-web layout path..."
rg "from \"@/plane-web/layouts/" --type ts --type tsx

Length of output: 415


Script:

#!/bin/bash
# Let's try without file type restrictions since tsx might not be recognized
echo "Checking for remaining references to old auth-layout path..."
rg "from ['\"]@/layouts/auth-layout"

echo "Checking for adoption of new plane-web layout path..."
rg "from ['\"]@/plane-web/layouts"

# Also check for any files with similar names in both locations
echo "Checking files in layouts directory..."
fd "project.*wrapper" 

echo "Checking files in plane-web/layouts directory..."
fd "project.*wrapper" "plane-web"

Length of output: 1305

web/ce/layouts/workspace-wrapper.tsx (1)

1-1: Verify PR scope alignment.

The changes reviewed appear to be focused on architectural reorganization of wrapper components, which seems unrelated to the stated PR objective of "adding functionality to add labels directly from dropdown". Please clarify if these changes are necessary for the label functionality or if they should be in a separate PR.

web/app/[workspaceSlug]/(projects)/layout.tsx (2)

5-6: Verify the behavior of the new WorkspaceAuthWrapper implementation.

The import source has changed from @/layouts/auth-layout to @/plane-web/layouts/workspace-wrapper. Since this component handles workspace authentication, we should ensure the new implementation maintains the same security guarantees.

✅ Verification successful

Based on the search results, I can now generate a final response. The code shows that the WorkspaceAuthWrapper is properly implemented with security checks:

  1. The core implementation in web/core/layouts/auth-layout/workspace-wrapper.tsx contains essential security hooks like useUser, useWorkspace, and useUserPermissions.
  2. The CE (Community Edition) version in web/ce/layouts/workspace-wrapper.tsx wraps the core implementation, maintaining all security guarantees.

The new WorkspaceAuthWrapper implementation maintains proper security checks

The import change is part of a well-structured refactoring where:

  1. The core authentication logic remains in the core module
  2. The CE version extends it by wrapping the core component
  3. All necessary security hooks and permissions checks are preserved
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Compare the old and new WorkspaceAuthWrapper implementations
# to ensure security checks are preserved

# Check the new implementation
echo "New Implementation:"
ast-grep --pattern 'export const WorkspaceAuthWrapper = observer(({ children }: { children: React.ReactNode }) => {
  $$$
})'

# Check for any security-related functions or hooks used
echo "\nSecurity-related function usage:"
rg -A 5 'useWorkspace|useUser|isAuthorized|checkPermission' 'web/ce/layouts/workspace-wrapper.tsx'

Length of output: 354


Script:

#!/bin/bash
# Let's try searching for the WorkspaceAuthWrapper in the new location
echo "Searching for WorkspaceAuthWrapper in plane-web directory:"
rg -l "WorkspaceAuthWrapper" --type tsx

# Get the implementation details
echo "\nImplementation details:"
rg -A 10 "WorkspaceAuthWrapper" --type tsx

# Check for workspace-related hooks and security checks
echo "\nChecking workspace-related hooks:"
rg -A 5 "useWorkspace|useUser|isAuthorized|checkPermission|workspaceService" --type tsx

Length of output: 525


Script:

#!/bin/bash
# Let's try without specifying file type
echo "Searching for WorkspaceAuthWrapper:"
rg -l "WorkspaceAuthWrapper"

# Get the implementation details
echo "\nImplementation details:"
rg -B 2 -A 10 "export.*WorkspaceAuthWrapper"

# Find the old implementation for comparison
echo "\nOld implementation:"
git log -p --all -- "**/auth-layout.tsx" "**/auth-layout.ts"

# Check for workspace-related hooks and security checks
echo "\nChecking workspace-related hooks:"
rg "useWorkspace|useUser|isAuthorized|checkPermission|workspaceService"

Length of output: 74877


Line range hint 1-24: Clarify the relationship between layout changes and label functionality.

This layout change appears to be a structural refactoring, but the PR's objective is to add label dropdown functionality. Could you clarify how this layout change supports or relates to the label management enhancement described in PR #6208?

web/core/layouts/auth-layout/project-wrapper.tsx (3)

37-37: LGTM: Clean interface extension

The addition of the optional isLoading prop follows TypeScript best practices and maintains backward compatibility.


41-41: LGTM: Props destructuring with default value

Good practice using default value for the optional prop, ensuring backward compatibility.


155-155: Verify loading state behavior

The modified loading condition now includes parent loading state. While this is logically correct, we should ensure it doesn't cause unnecessary loading states or flickering.

✅ Verification successful

Loading state implementation is well-handled

The loading state implementation is correct and won't cause unnecessary flickering because:

  1. The isParentLoading prop is optional and defaults to false
  2. The loading state is only shown when either:
    • Parent component explicitly requests loading state (isParentLoading)
    • OR when essential project permission data is being fetched (!projectMemberInfo && projectId && hasPermissionToCurrentProject === null)
  3. The implementation uses a clean loading spinner with proper layout and styling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other components that might be affected by this loading state change
rg -l "ProjectAuthWrapper.*isLoading" 

Length of output: 39


Script:

#!/bin/bash
# Let's search for the component and its usage patterns
rg "ProjectAuthWrapper" -A 5 -B 5

# Also search for loading state patterns in the same file
rg "isLoading|loading" "web/core/layouts/auth-layout/project-wrapper.tsx"

Length of output: 3565


Script:

#!/bin/bash
# Let's look at the complete implementation of the loading state logic in the file
cat "web/core/layouts/auth-layout/project-wrapper.tsx"

Length of output: 7097

web/core/layouts/auth-layout/workspace-wrapper.tsx (3)

29-32: LGTM: Consistent interface pattern

The interface changes mirror those in ProjectAuthWrapper, maintaining consistency across wrapper components.


35-35: LGTM: Consistent props handling

Props destructuring with default value matches the pattern used in ProjectAuthWrapper.


122-122: Verify combined loading conditions

The loading condition now includes multiple checks including parent loading and DB initialization. While comprehensive, we should verify the order of these checks is optimal.

✅ Verification successful

Let me gather more context about the loading conditions and their usage.


Let me check one more thing about the loading states and their initialization order.


Let me check one final thing about the loading conditions initialization.


Based on the analysis of the loading conditions, I can now provide the final response:

Loading conditions order is correctly implemented

The loading checks are properly ordered for optimal performance:

  • isParentLoading checks parent component loading state first
  • allWorkspaces === undefined verifies workspace data availability
  • loader handles any additional loading states
  • isDBInitializing is appropriately placed last as it depends on workspace initialization

The order ensures proper cascade of dependencies and prevents unnecessary DB initialization attempts before required data is available.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for components using WorkspaceAuthWrapper with loading prop
rg -l "WorkspaceAuthWrapper.*isLoading"

# Check for other DB initialization patterns
rg "isDBInitializing" 

Length of output: 310


Script:

#!/bin/bash
# Check the full context of the loading conditions in workspace-wrapper
rg -A 10 -B 10 "isParentLoading.*allWorkspaces.*loader.*isDBInitializing" web/core/layouts/auth-layout/workspace-wrapper.tsx

# Check how allWorkspaces is defined
ast-grep --pattern 'const { $_, allWorkspaces } = $_'

# Look for loader definition
ast-grep --pattern 'const loader = $_'

Length of output: 936


Script:

#!/bin/bash
# Get more context about how these loading states are initialized
rg -B 20 "if \(isParentLoading.*allWorkspaces.*loader.*isDBInitializing\)" web/core/layouts/auth-layout/workspace-wrapper.tsx

# Check for any useSWR hooks that might affect loading states
ast-grep --pattern 'useSWR($$$)'

Length of output: 66962


Script:

#!/bin/bash
# Get the full context around the loading conditions
rg -B 30 "if \(isParentLoading.*allWorkspaces.*loader.*isDBInitializing\)" web/core/layouts/auth-layout/workspace-wrapper.tsx

Length of output: 1061

web/app/profile/page.tsx (1)

13-13: Verify whether userProfile is necessary in this component.

The userProfile data is destructured from useUser(), but it appears to be used only when passed to ProfileForm. Ensure that userProfile is required here or if it can be fetched within the ProfileForm component to reduce prop drilling.

Run the following script to check the usage of userProfile:

✅ Verification successful

Let me check the implementation of ProfileForm to understand if userProfile could be fetched within it.


Based on the analysis of the code, I can now generate the final response:

userProfile is necessary in this component as it's correctly used to pass profile data to ProfileForm.

The userProfile data is properly utilized in this component as it's passed to the ProfileForm component which requires this data to:

  • Set default form values for user role and language preferences
  • Handle profile updates through the updateUserProfile function
  • Display and manage user profile settings
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the usage of `userProfile` in this file.

# Test: Find all occurrences of `userProfile` in the file.
rg 'userProfile' web/app/profile/page.tsx

Length of output: 167


Script:

#!/bin/bash
# Find ProfileForm implementation
ast-grep --pattern 'const ProfileForm = $_'

# Also check if useUser hook is used elsewhere in the codebase
rg 'useUser\(' -A 2

Length of output: 63553

packages/i18n/src/hooks/use-translation.ts (1)

5-17: Confirm that useTranslation is used within a TranslationProvider.

The useTranslation hook throws an error if used outside of a TranslationProvider. Ensure that all components using this hook are properly wrapped with the provider to prevent runtime errors.

Run the following script to check for components using useTranslation without TranslationProvider:

✅ Verification successful

Let me check if these components are wrapped by a higher-level component that provides the TranslationProvider. Let's examine the store-wrapper.tsx first since it seems to be a top-level wrapper component.


Let's check the provider.tsx since it uses StoreWrapper, and also check the actual implementation of TranslationProvider to understand the application's translation setup.


Based on the results, I can now generate the final response since we have sufficient information. The provider.tsx shows that TranslationProvider is properly set up at the application root level, wrapping the StoreWrapper and other components.

No issues found with useTranslation provider setup

The useTranslation hook is safely used within the application as the TranslationProvider is properly configured in the root provider hierarchy (web/app/provider.tsx). The provider wraps all application components including:

  • StoreWrapper
  • InstanceWrapper
  • IntercomProvider
  • PostHogProvider

This ensures that all components using useTranslation will have access to the translation context.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find components using useTranslation without TranslationProvider.

# Test: List all files using useTranslation.
files=$(rg -l 'useTranslation')

# For each file, check if TranslationProvider is imported.
for file in $files; do
  if ! rg -q 'TranslationProvider' "$file"; then
    echo "Potential issue in $file: useTranslation used without TranslationProvider."
  fi
done

Length of output: 1176


Script:

#!/bin/bash
# Check the content of store-wrapper.tsx to see if it provides the TranslationProvider
rg -A 10 -B 10 'useTranslation' 'web/core/lib/wrappers/store-wrapper.tsx'

# Also check if store-wrapper is used to wrap the application
rg -l 'StoreWrapper'

Length of output: 1402


Script:

#!/bin/bash
# Check the content of provider.tsx to understand the provider hierarchy
rg -A 10 -B 10 'StoreWrapper' 'web/app/provider.tsx'

# Check where TranslationProvider is defined and how it's used
rg -l 'TranslationProvider'

# Check if there's a root layout or similar file that might include TranslationProvider
fd -e tsx -e ts layout

Length of output: 6337

packages/i18n/src/config/index.ts (1)

1-39: Verify PR scope: i18n implementation vs label functionality

This appears to be an i18n implementation, but the PR description mentions adding label functionality from dropdowns. Please clarify if this i18n work is intended to be part of this PR or should be in a separate PR.

✅ Verification successful

Let me gather more context about the PR to understand the relationship between i18n and label changes.


Based on the PR description and file changes, I can now generate the final response:

The i18n implementation is a valid part of this PR's scope

The PR adds functionality for creating labels from dropdowns, and the i18n implementation is a supporting feature for this work. This is confirmed by:

  • PR description mentions adding label functionality from dropdowns as the primary change
  • The codebase changes show both label-related components and i18n package modifications
  • CodeRabbit's summary explicitly mentions "Added support for internationalization" as part of the new features
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are other label-related changes in this PR
gh pr view 6208 --json files | jq -r '.files[].path' | grep -i label

Length of output: 320


Script:

#!/bin/bash
# Get PR description and title
gh pr view 6208 --json title,body

# Check for i18n related changes
gh pr view 6208 --json files | jq -r '.files[].path' | grep -i "i18n\|translations\|locale"

# Look for label-related code in the new files
rg -n "i18n" "web/core/components/issues/issue-detail/label/"

Length of output: 2273

packages/i18n/src/components/index.tsx (1)

14-26: LGTM: Event listener cleanup is properly handled

The useEffect hook correctly sets up and cleans up the storage event listener, preventing memory leaks.

web/app/provider.tsx (1)

7-8: LGTM: TranslationProvider integration looks good.

The TranslationProvider is correctly implemented and properly wrapped around the application components to provide translation context throughout the app.

Also applies to: 45-55

web/package.json (1)

34-34: Consider using explicit version for @plane/i18n

Using "*" for package versions can lead to unexpected breaking changes. Even for internal packages, it's recommended to use explicit versions or version ranges to ensure consistent builds.

Let's check if other internal @plane packages use explicit versions:

Consider using an explicit version or version range like other @plane packages.

web/next.config.js (1)

32-32: LGTM: Correct configuration for internal package transpilation

The addition of @plane/i18n to transpilePackages is necessary and correctly configured for Next.js to properly handle the internal package.

web/app/onboarding/page.tsx (1)

47-47: LGTM! Consistent null check pattern.

The explicit null check before calling fetchWorkspaces aligns with the pattern used in the invitations fetch logic, preventing unnecessary function calls.

web/app/profile/sidebar.tsx (3)

9-11: LGTM! Clean import organization.

The imports are well-organized, with the i18n hook properly imported from the @plane/i18n package.


121-121: Verify translation key definitions.

The translation keys being used (e.g., 'profile_settings', 'your_account', etc.) need to be verified to ensure they exist in the translation files.

Also applies to: 127-127, 136-136, 149-149, 160-160, 210-210, 222-222

✅ Verification successful

Translation keys are properly defined and used

All the translation keys mentioned in the review comment (profile_settings, your_account, create_workspace, invitations) are properly defined in the English translations file at packages/i18n/src/locales/en/translations.json. The keys are also correctly used in the sidebar component with the appropriate t() function calls.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for translation key definitions
fd -e json -e yaml --exec rg -l "profile_settings|your_account|create_workspace|invitations"

# Check if there are any other components using these translation keys
rg "t\([\"']profile_settings[\"']\)" --type ts --type tsx
rg "t\([\"']your_account[\"']\)" --type ts --type tsx

Length of output: 470


Script:

#!/bin/bash
# Let's check the content of translation files to verify the keys
echo "Checking English translations:"
cat ./packages/i18n/src/locales/en/translations.json | jq '. | select(.profile_settings != null or .your_account != null or .create_workspace != null or .invitations != null)'

# Search for translation key usage with correct ripgrep syntax
echo "Checking translation key usage:"
rg "t\([\"']profile_settings[\"']\)"
rg "t\([\"']your_account[\"']\)"
rg "t\([\"']create_workspace[\"']\)"
rg "t\([\"']invitations[\"']\)"

Length of output: 2194


26-26: Verify the impact of key format change.

The change from hyphenated to underscore format in WORKSPACE_ACTION_LINKS keys could affect other components that might be using these keys.

Comment on lines +308 to +315
validate: (value) => {
if (value.trim().length < 1) return "Display name can't be empty.";
if (value.split(" ").length > 1) return "Display name can't have two consecutive spaces.";
if (value.replace(/\s/g, "").length < 1) return "Display name must be at least 1 character long.";
if (value.replace(/\s/g, "").length > 20)
return "Display name must be less than 20 characters long.";
return true;
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify validation logic for display_name.

The validation rules for display_name can be optimized for readability and efficiency. Consider combining conditions or using regex patterns.

Refactored validation:

validate: (value) => {
  const trimmedValue = value.trim();
  if (trimmedValue.length < 1) return "Display name can't be empty.";
  if (/\s{2,}/.test(value)) return "Display name can't have two consecutive spaces.";
  if (trimmedValue.length > 20) return "Display name must be less than 20 characters long.";
  return true;
},

Comment on lines +451 to +453
<Button variant="primary" type="submit" loading={isLoading}>
{isLoading ? t("saving...") : t("save_changes")}
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Disable the save button when the form is not dirty or valid.

Currently, the save button is always enabled, which allows users to submit without making changes or with invalid data. Use isDirty and isValid from react-hook-form to control the button's disabled state.

Update the button component:

-  <Button variant="primary" type="submit" loading={isLoading}>
+  <Button variant="primary" type="submit" loading={isLoading} disabled={!isDirty || !isValid}>

Ensure you extract isDirty and isValid from the form state:

const {
  handleSubmit,
  watch,
  control,
  setValue,
  formState: { errors, isDirty, isValid },
} = useForm<TUserProfileForm>({
  mode: "onChange",
  defaultValues: { ... },
});

Comment on lines +240 to +242
<span>{`${watch("first_name")} ${watch("last_name")}`}</span>
</div>
<span className="text-sm text-custom-text-300 tracking-tight">{watch("email")}</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential undefined values in watch("first_name") and watch("last_name").

When displaying the user's full name, if first_name or last_name are undefined, it may result in "undefined undefined". Consider providing default values or handling empty strings.

Apply this diff to provide default values:

-  <span>{`${watch("first_name")} ${watch("last_name")}`}</span>
+  <span>{`${watch("first_name") || ""} ${watch("last_name") || ""}`}</span>
📝 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.

Suggested change
<span>{`${watch("first_name")} ${watch("last_name")}`}</span>
</div>
<span className="text-sm text-custom-text-300 tracking-tight">{watch("email")}</span>
<span>{`${watch("first_name") || ""} ${watch("last_name") || ""}`}</span>
</div>
<span className="text-sm text-custom-text-300 tracking-tight">{watch("email")}</span>

email: user.email || "",
role: profile.role || "Product / Project Manager",
language: profile.language || "en",
user_timezone: "Asia/Kolkata",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use user's actual timezone if available.

The default value for user_timezone is hardcoded as "Asia/Kolkata". If the user's timezone is available from user.user_timezone, it would be better to use that to personalize the experience.

Apply this diff to use the user's timezone:

-  user_timezone: "Asia/Kolkata",
+  user_timezone: user.user_timezone || "Asia/Kolkata",

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +131 to +168
const onSubmit = async (formData: TUserProfileForm) => {
setIsLoading(true);
const userPayload: Partial<IUser> = {
first_name: formData.first_name,
last_name: formData.last_name,
avatar_url: formData.avatar_url,
display_name: formData?.display_name,
user_timezone: formData.user_timezone,
};
// if unsplash or a pre-defined image is uploaded, delete the old uploaded asset
if (formData.cover_image_url?.startsWith("http")) {
userPayload.cover_image = formData.cover_image_url;
userPayload.cover_image_asset = null;
}

const profilePayload: Partial<TUserProfile> = {
role: formData.role,
language: formData.language,
};

const updateCurrentUserDetail = updateCurrentUser(userPayload).finally(() => setIsLoading(false));
const updateCurrentUserProfile = updateUserProfile(profilePayload).finally(() => setIsLoading(false));

const promises = [updateCurrentUserDetail, updateCurrentUserProfile];
const updateUserAndProfile = Promise.all(promises);

setPromiseToast(updateUserAndProfile, {
loading: "Updating...",
success: {
title: "Success!",
message: () => `Profile updated successfully.`,
},
error: {
title: "Error!",
message: () => `There was some error in updating your profile. Please try again.`,
},
});
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure concurrent updates complete before proceeding.

When updating user data and profile data concurrently using Promise.all, if one promise rejects, the other may still be pending. Ensure that you properly handle rejections and provide feedback to the user.

Consider updating the code to handle individual promise rejections:

-  const updateUserAndProfile = Promise.all(promises);
+  const updateUserAndProfile = Promise.allSettled(promises);

And adjust the toast messages accordingly based on which operation failed.

Committable suggestion skipped: line range outside the PR's diff.

@@ -13,4 +13,5 @@ export enum EModalWidth {
XXXXL = "sm:max-w-4xl",
VXL = "sm:max-w-5xl",
VIXL = "sm:max-w-6xl",
VIIXL = "sm:max-w-7xl",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

The new modal width VIIXL is not being used in the codebase

The newly added VIIXL modal width constant is defined in packages/ui/src/modals/constants.ts but is not being used anywhere in the codebase. The search results show that other modal widths like XXL, VIXL, and XXXXL are being actively used, but there are no occurrences of VIIXL.

  • Consider removing the unused VIIXL constant if it's not needed
  • If the width is intended to be used, implement it in the components that require the sm:max-w-7xl width
🔗 Analysis chain

Ensure the new modal width VIIXL is utilized where necessary.

The addition of VIIXL to EModalWidth provides an extra-large modal width option. Verify that this new size is actually needed and applied in components that require larger modals.

Run the following script to find usages of EModalWidth.VIIXL:


The previous search might have failed because the enum might be used differently. These commands will help us:

  1. Find any occurrence of VIIXL string
  2. Find how EModalWidth is imported and used
  3. Find any components using the tailwind class directly
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the usage of EModalWidth.VIIXL in the codebase.

# Test: Find all occurrences of EModalWidth.VIIXL.
rg 'EModalWidth\.VIIXL'

Length of output: 25


Script:

#!/bin/bash
# Let's first check if there are any direct string occurrences of 'VIIXL'
rg "VIIXL"

# Also search for any modal width related imports and usages
rg -A 2 "EModalWidth"

# Check for any recent changes or additions to modal components
fd -e tsx -e ts "Modal" --exec rg -l "max-w-7xl"

Length of output: 12454

// Create Context
export const TranslationContext = createContext<TranslationStore>(translationStore);

export const TranslationProvider = observer(({ children }: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add proper typing for children prop

Instead of using any, consider using proper React types for the children prop.

-export const TranslationProvider = observer(({ children }: any) => {
+export const TranslationProvider = observer(({ children }: { children: React.ReactNode }) => {
📝 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.

Suggested change
export const TranslationProvider = observer(({ children }: any) => {
export const TranslationProvider = observer(({ children }: { children: React.ReactNode }) => {

Comment on lines +18 to +20
t(key: string) {
return translations[this.currentLocale]?.[key] || translations[fallbackLng][key] || key;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance translation lookup robustness

The current translation lookup could be improved in several ways:

  1. Add logging for missing translations
  2. Handle nested translation keys
  3. Add type safety for translation keys
-  t(key: string) {
-    return translations[this.currentLocale]?.[key] || translations[fallbackLng][key] || key;
+  t(key: string) {
+    const translation = translations[this.currentLocale]?.[key] || translations[fallbackLng][key];
+    if (!translation && process.env.NODE_ENV === 'development') {
+      console.warn(`Missing translation for key: ${key} in locale: ${this.currentLocale}`);
+    }
+    return translation || key;
   }
📝 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.

Suggested change
t(key: string) {
return translations[this.currentLocale]?.[key] || translations[fallbackLng][key] || key;
}
t(key: string) {
const translation = translations[this.currentLocale]?.[key] || translations[fallbackLng][key];
if (!translation && process.env.NODE_ENV === 'development') {
console.warn(`Missing translation for key: ${key} in locale: ${this.currentLocale}`);
}
return translation || key;
}

Comment on lines +1 to +49
{
"submit": "Soumettre",
"cancel": "Annuler",
"loading": "Chargement",
"error": "Erreur",
"success": "Succès",
"warning": "Avertissement",
"info": "Info",
"close": "Fermer",
"yes": "Oui",
"no": "Non",
"ok": "OK",
"name": "Nom",
"description": "Description",
"search": "Rechercher",
"add_member": "Ajouter un membre",
"remove_member": "Supprimer un membre",
"add_members": "Ajouter des membres",
"remove_members": "Supprimer des membres",
"add": "Ajouter",
"remove": "Supprimer",
"add_new": "Ajouter nouveau",
"remove_selected": "Supprimer la sélection",
"first_name": "Prénom",
"last_name": "Nom de famille",
"email": "Email",
"display_name": "Nom d'affichage",
"role": "Rôle",
"timezone": "Fuseau horaire",
"avatar": "Avatar",
"cover_image": "Image de couverture",
"password": "Mot de passe",
"change_cover": "Modifier la couverture",
"language": "Langue",
"saving": "Enregistrement...",
"save_changes": "Enregistrer les modifications",
"deactivate_account": "Désactiver le compte",
"deactivate_account_description": "Lors de la désactivation d'un compte, toutes les données et ressources de ce compte seront définitivement supprimées et ne pourront pas être récupérées.",
"profile_settings": "Paramètres du profil",
"your_account": "Votre compte",
"profile": "Profil",
"security": " Sécurité",
"activity": "Activité",
"appearance": "Apparence",
"notifications": "Notifications",
"workspaces": "Workspaces",
"create_workspace": "Créer un workspace",
"invitations": "Invitations"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add missing label-related translations.

Given that this PR adds functionality for label management, please add translations for label-related terms such as:

  • "Add label"
  • "Create label"
  • "Label name"
  • "Label color"
  • "Delete label"

Also, consider these improvements:

  • Line 38: The account deactivation message could be more concise in French
  • Line 42: Remove extra space in "Sécurité"

@@ -62,6 +61,7 @@ export type TUserProfile = {
billing_address_country: string | undefined;
billing_address: string | undefined;
has_billing_address: boolean;
language: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making language field optional and type-safe

The language field is marked as required which could break existing code. Additionally, using string type allows any value to be assigned.

Consider these improvements:

-  language: string;
+  language?: TLanguageCode;

And add a type definition for supported languages:

export type TLanguageCode = 'en' | 'es' | 'fr' | 'ja' | string; // Add other supported languages

💡 Codebase verification

Language field is part of i18n functionality and unrelated to label dropdown

Based on the evidence found:

  1. The label dropdown implementation in web/core/components/dropdowns/layout.tsx only handles display of labels and icons
  2. The language field in users.d.ts appears alongside other user profile fields and is related to the i18n functionality found in packages/i18n
  3. There's no connection between the label dropdown functionality and the language field addition

This change should indeed be moved to a separate PR focused on internationalization features.

🔗 Analysis chain

Verify PR scope: Language field seems unrelated

The addition of the language field appears unrelated to the PR's objective of "adding functionality to add labels directly from dropdown". Consider moving this change to a separate PR focused on internationalization.

Let's check if there are any label-related changes that justify this addition:


Let me gather more context about the PR and language-related changes.


Let me check the actual content of the language-related changes and label dropdown implementations to understand if there's a connection.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for label-related changes
rg -l 'label' --type ts --type tsx

# Search for i18n usage in label components
ast-grep --pattern 'useTranslation($$$)' 

Length of output: 334


Script:

#!/bin/bash
# Check PR description and related files
gh pr view

# Look for language-related changes
rg -l "language" --type ts

# Look specifically for label dropdown related changes
rg -l "dropdown.*label|label.*dropdown" --type ts

Length of output: 799


Script:

#!/bin/bash
# Check language-related changes in dropdown and user-related files
rg "language" web/core/components/dropdowns/layout.tsx -C 2
rg "language" web/helpers/dashboard.helper.ts -C 2
rg "language" packages/types/src/users.d.ts -C 2

# Check label dropdown implementation
rg "label" web/core/components/dropdowns/layout.tsx -C 2

Length of output: 932

@mathalav55 mathalav55 closed this Dec 17, 2024
@mathalav55 mathalav55 deleted the feat-label_select_add branch December 17, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟enhancement New feature or request 🌐frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants