[WEB-2500] feat: Product updates modal (What's new in Plane) #5690
Conversation
prateekshourya29
requested review from
SatishGandham,
sriramveeraghanta and
pablohashescobar
WalkthroughThe changes introduce a new environment variable 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://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (17)
web/core/components/global/index.ts (1)
1-1: LGTM! Consider named exports for better maintainability.The export statement aligns with the PR objectives by making the product updates functionality available to other parts of the application. This change effectively introduces the new feature for the product updates modal.
While the current implementation works, consider using named exports instead of a wildcard export for better maintainability and clarity. This approach allows for better tracking of which specific entities are being used elsewhere in the codebase and can prevent potential naming conflicts.
Example of named exports:
export { ProductUpdatesModal, useProductUpdates } from "./product-updates";This suggestion is optional and depends on your team's coding standards and preferences.
web/core/components/global/product-updates/index.ts (1)
1-2: LGTM! Consider adding a brief comment.The export statements look good and align with the PR objective of implementing a product updates modal. This index file will make it easier to import the modal and footer components related to product updates in other parts of the application.
Consider adding a brief comment at the top of the file to describe its purpose:
// Index file for product updates components export * from "./modal"; export * from "./footer";This comment would provide quick context for developers who might work on this file in the future.
web/core/services/instance.service.ts (1)
29-35: LGTM: New method implemented correctly with a minor suggestion.The
getInstanceChangeLogmethod is well-implemented and consistent with the existing code style and error handling pattern. It aligns with the PR objectives of implementing a feature for product updates.Consider enhancing the error handling to provide more context:
async getInstanceChangeLog(): Promise<TPage> { return this.get("/api/instances/changelog/") .then((response) => response.data) .catch((error) => { - throw error; + throw new Error(`Failed to fetch instance changelog: ${error.message}`); }); }This change would make debugging easier by providing more specific error information.
web/ce/components/global/product-updates-header.tsx (2)
1-7: Consider reordering imports for consistency.To improve code organization and address the static analysis hint, consider reordering the imports. Place the
package.jsonimport after the image import.Here's the suggested order:
import { observer } from "mobx-react"; import Image from "next/image"; // assets import PlaneLogo from "@/public/plane-logos/blue-without-text.png"; import packageJson from "package.json"; // helpers import { cn } from "@/helpers/common.helper";Tools
GitHub Check: lint-web
[failure] 3-3:
package.jsonimport should occur after import of@/public/plane-logos/blue-without-text.png
9-25: Approve implementation with a minor accessibility suggestion.The component is well-structured, uses appropriate React and Next.js features, and implements a responsive design. Good job on using the
observerwrapper for reactive updates and dynamically displaying the version number.For improved accessibility, consider adding an
aria-labelto the outerdivto describe the purpose of this section.Here's a suggested modification for line 10:
<div className="flex gap-2 mx-6 my-4 items-center justify-between flex-shrink-0" aria-label="Product updates header">apiserver/plane/license/urls.py (1)
23-27: LGTM: New URL pattern is correctly implemented.The new URL pattern for the changelog feature has been properly added and follows the existing structure in the file. The path and naming are appropriate and consistent with other patterns.
For consistency with other URL patterns in this file, consider adding a trailing comma after the closing parenthesis of
name="instance-changelog",. This is a minor style suggestion and doesn't affect functionality.path( "changelog/", ChangeLogEndpoint.as_view(), - name="instance-changelog", + name="instance-changelog", ),web/core/components/global/product-updates/footer.tsx (4)
1-7: LGTM! Consider grouping imports for better organization.The imports are appropriate for the component's functionality. The use of Next.js Image component is good for optimized image loading, and the custom UI utility and helper function promote code reuse.
Consider grouping the imports for better organization:
import Image from "next/image"; // ui import { getButtonStyling } from "@plane/ui"; // helpers import { cn } from "@/helpers/common.helper"; // assets import PlaneLogo from "@/public/plane-logos/blue-without-text.png";
9-10: LGTM! Consider adding ARIA roles for improved accessibility.The component structure is clean and follows React best practices. The use of Tailwind CSS classes provides a consistent styling approach.
Consider adding ARIA roles to improve accessibility:
- <div className="flex items-center justify-between flex-shrink-0 gap-4 m-6 mb-4"> + <div className="flex items-center justify-between flex-shrink-0 gap-4 m-6 mb-4" role="contentinfo">
11-49: LGTM! Consider adding security attributes and improving accessibility for links.The links section is well-structured and provides useful resources for users. The use of SVG circles as separators is a nice visual touch.
Consider the following improvements:
- Add
rel="noopener noreferrer"to links withtarget="_blank"for security:- target="_blank" + target="_blank" rel="noopener noreferrer"
- Improve accessibility by adding
aria-labelto the SVG separators:- <svg viewBox="0 0 2 2" className="h-0.5 w-0.5 fill-current"> + <svg viewBox="0 0 2 2" className="h-0.5 w-0.5 fill-current" aria-label="separator">
- Consider using a semantic list structure for better accessibility:
<nav aria-label="Footer links"> <ul className="flex items-center gap-2"> <li> <a href="https://go.plane.so/p-docs" target="_blank" rel="noopener noreferrer" className="text-sm text-custom-text-200 hover:text-custom-text-100 hover:underline underline-offset-1 outline-none"> Docs </a> </li> <li aria-hidden="true"> <svg viewBox="0 0 2 2" className="h-0.5 w-0.5 fill-current" aria-label="separator"> <circle cx={1} cy={1} r={1} /> </svg> </li> {/* Repeat for other links */} </ul> </nav>
50-61: LGTM! Consider adding alt text and improving button accessibility.The "Powered by Plane Pages" button is well-implemented, using a custom styling utility for consistency and the Next.js Image component for performance.
Consider the following improvements:
- Add a more descriptive
alttext for the logo image:- <Image src={PlaneLogo} alt="Plane" width={12} height={12} /> + <Image src={PlaneLogo} alt="Plane logo" width={12} height={12} />
- Add
rel="noopener noreferrer"for security:- target="_blank" + target="_blank" rel="noopener noreferrer"
- Improve accessibility by adding an
aria-labelto the anchor tag:className={cn( getButtonStyling("accent-primary", "sm"), "flex gap-1.5 items-center text-center font-medium hover:underline underline-offset-2 outline-none" )} + aria-label="Visit Plane Pages website" >web/core/components/global/product-updates/modal.tsx (2)
23-28: LGTM with a minor suggestion: Component declaration and hooks usage are well-implemented.The component is correctly defined as a functional component, wrapped with MobX's observer HOC for reactive updates. The use of
useReffor the editor reference anduseSWRfor data fetching are good practices.Consider destructuring the props in the function parameters for cleaner code:
-export const ProductUpdatesModal: FC<ProductUpdatesModalProps> = observer((props) => { - const { isOpen, handleClose } = props; +export const ProductUpdatesModal: FC<ProductUpdatesModalProps> = observer(({ isOpen, handleClose }) => {
30-76: LGTM with a minor suggestion: Component structure and conditional rendering are well-implemented.The component's structure is well-organized, using modular components and handling different states appropriately. The conditional rendering for loading and error states enhances the user experience.
Consider adding an
aria-labelto the modal for better accessibility:-<ModalCore isOpen={isOpen} handleClose={handleClose} position={EModalPosition.CENTER} width={EModalWidth.XXL}> +<ModalCore isOpen={isOpen} handleClose={handleClose} position={EModalPosition.CENTER} width={EModalWidth.XXL} aria-label="Product Updates">web/core/components/workspace/sidebar/help-section.tsx (2)
Line range hint
32-32: LGTM: New state and modal for product updatesThe introduction of
isChangeLogOpenstate and theProductUpdatesModalcomponent is well-implemented. The modal is correctly placed and its props are properly set.For consistency, consider renaming
isChangeLogOpentoisProductUpdatesModalOpento match the component name and its purpose more closely.-const [isChangeLogOpen, setIsChangeLogOpen] = useState(false); +const [isProductUpdatesModalOpen, setIsProductUpdatesModalOpen] = useState(false); -<ProductUpdatesModal isOpen={isChangeLogOpen} handleClose={() => setIsChangeLogOpen(false)} /> +<ProductUpdatesModal isOpen={isProductUpdatesModalOpen} handleClose={() => setIsProductUpdatesModalOpen(false)} />Also applies to: 41-41
119-127: LGTM: New menu item for "What's new" featureThe addition of the "What's new" button as a CustomMenu.MenuItem is well-implemented. It correctly triggers the product updates modal when clicked.
To improve accessibility, consider adding an aria-label to the button.
<button type="button" onClick={() => setIsChangeLogOpen(true)} - className="flex w-full items-center justify-start text-xs hover:bg-custom-background-80" + className="flex w-full items-center justify-start text-xs hover:bg-custom-background-80" + aria-label="Open product updates" > <span className="text-xs">What's new</span> </button>apiserver/plane/license/api/views/instance.py (2)
180-181: LGTM! Consider adding a fallback value for INSTANCE_CHANGELOG_URL.The addition of
instance_changelog_urlto the response data is consistent with the existing code structure and aligns with the PR objective of implementing a "What's new" feature. Good job!Consider adding a fallback value for
settings.INSTANCE_CHANGELOG_URLin case it's not defined in the settings. This would make the code more robust:- data["instance_changelog_url"] = settings.INSTANCE_CHANGELOG_URL + data["instance_changelog_url"] = getattr(settings, 'INSTANCE_CHANGELOG_URL', '')This ensures that even if
INSTANCE_CHANGELOG_URLis not defined in the settings, the code won't raise an AttributeError.
Line range hint
1-181: Overall, the change is well-integrated. Consider adding a comment for clarity.The addition of
instance_changelog_urlto the response data is minimal and doesn't affect the overall structure or logic of the file. It enhances the API response with information needed for the "What's new" feature, which aligns well with the PR objectives.Consider adding a brief comment above the new line to explain its purpose:
+ # Add changelog URL for the "What's new" feature data["instance_changelog_url"] = settings.INSTANCE_CHANGELOG_URLThis would improve code readability and make the purpose of this addition clear to other developers.
apiserver/plane/settings/common.py (1)
362-364: LGTM! Consider adding a comment for consistency.The addition of
INSTANCE_CHANGELOG_URLis well-implemented and aligns with the PR objectives. It follows the existing pattern for environment variables in this file.For consistency with other similar configurations in this file, consider adding a brief comment explaining the purpose of this variable. For example:
# Instance Changelog URL INSTANCE_CHANGELOG_URL = os.environ.get("INSTANCE_CHANGELOG_URL", "")This will help maintain the documentation standard throughout the configuration file.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- apiserver/Dockerfile.api (1 hunks)
- apiserver/Dockerfile.dev (1 hunks)
- apiserver/plane/license/api/views/init.py (1 hunks)
- apiserver/plane/license/api/views/changelog.py (1 hunks)
- apiserver/plane/license/api/views/instance.py (1 hunks)
- apiserver/plane/license/urls.py (2 hunks)
- apiserver/plane/settings/common.py (1 hunks)
- web/app/profile/page.tsx (1 hunks)
- web/ce/components/global/index.ts (1 hunks)
- web/ce/components/global/product-updates-header.tsx (1 hunks)
- web/ce/components/global/product-updates-modal.tsx (0 hunks)
- web/ce/components/global/product-updates.tsx (0 hunks)
- web/core/components/common/index.ts (0 hunks)
- web/core/components/common/product-updates-modal.tsx (0 hunks)
- web/core/components/global/index.ts (1 hunks)
- web/core/components/global/product-updates/footer.tsx (1 hunks)
- web/core/components/global/product-updates/index.ts (1 hunks)
- web/core/components/global/product-updates/modal.tsx (1 hunks)
- web/core/components/workspace/sidebar/help-section.tsx (2 hunks)
- web/core/services/instance.service.ts (2 hunks)
Files not reviewed due to no reviewable changes (4)
- web/ce/components/global/product-updates-modal.tsx
- web/ce/components/global/product-updates.tsx
- web/core/components/common/index.ts
- web/core/components/common/product-updates-modal.tsx
Additional context used
Ruff
apiserver/plane/license/api/views/__init__.py
22-22:
.changelog.ChangeLogEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
GitHub Check: lint-web
web/ce/components/global/product-updates-header.tsx
[failure] 3-3:
package.jsonimport should occur after import of@/public/plane-logos/blue-without-text.png
Additional comments not posted (17)
web/ce/components/global/index.ts (1)
1-2: LGTM! Component exports updated.The changes look good and align with the PR objectives. The new export for
product-updates-headerhas been added, which seems to be part of the new product updates modal feature.However, please note that the exports for
product-updatesandproduct-updates-modalhave been removed. Ensure that this doesn't break any existing imports in other parts of the codebase.To verify that no imports are broken due to the removed exports, run the following script:
Also, consider updating the PR description to mention the component restructuring (removal of
product-updatesandproduct-updates-modal, addition ofproduct-updates-header) for better clarity on the changes made.apiserver/plane/license/api/views/__init__.py (1)
22-22:⚠️ Potential issueAddress the unused import of
ChangeLogEndpointThe newly added import of
ChangeLogEndpointis currently flagged as unused by the static analysis tool. This could lead to potential issues or confusion in the future.Consider one of the following actions:
- If
ChangeLogEndpointis intended to be used elsewhere in the codebase, ensure it's properly utilized or re-exported in this init.py file.- If it's meant to be available at the package level, add it to the
__all__list (if one exists) or re-export it explicitly.- If the import is premature and not yet needed, consider removing it until it's actually used to avoid confusion.
To resolve this, you could either remove the import if it's not needed yet:
-from .changelog import ChangeLogEndpointOr, if it should be available at the package level, re-export it:
from .changelog import ChangeLogEndpoint +__all__ = [..., 'ChangeLogEndpoint'] # Add to existing __all__ or create one if it doesn't existPlease clarify the intended use of
ChangeLogEndpointand update the code accordingly.To help verify the usage of
ChangeLogEndpointin the codebase, you can run the following script:This script will help determine if
ChangeLogEndpointis used elsewhere in the codebase or if it's re-exported in the init.py file.Tools
Ruff
22-22:
.changelog.ChangeLogEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
web/core/services/instance.service.ts (2)
2-2: LGTM: Import statement updated correctly.The import statement has been properly updated to include the
TPagetype, which is used in the newgetInstanceChangeLogmethod. This change is consistent with the new functionality being added.
2-2: Summary: Changes align well with PR objectives.The modifications to this file, including the new
getInstanceChangeLogmethod and the updated import statement, are well-implemented and consistent with the existing codebase. These changes directly support the PR objective of implementing a feature for product updates in the Plane application.The new method provides a way to fetch changelog information, which can be used to populate the "What's new in Plane" modal mentioned in the PR summary. This implementation forms a crucial backend service component for the feature.
Also applies to: 29-35
apiserver/plane/license/api/views/changelog.py (3)
1-13: LGTM: Imports are well-organized and appropriate.The imports are correctly grouped, separated by comments, and all seem necessary for the functionality of this file. This organization improves readability and maintainability.
16-19: Verify security implications of AllowAny permission.The
ChangeLogEndpointis currently accessible to all users due to theAllowAnypermission setting. While this may be intentional, it's important to ensure that the changelog doesn't contain any sensitive information that should be restricted to authenticated users or specific user roles.Please confirm if this level of access is appropriate for the changelog content. If the changelog contains any sensitive information, consider implementing appropriate authentication and authorization measures.
1-35: Overall assessment: Good implementation with room for improvement.The
ChangeLogEndpointclass provides a solid foundation for fetching and serving changelog data. The code is well-structured and follows Django REST framework conventions. However, there are a few areas where it can be improved:
- Consider the security implications of using
AllowAnypermission.- Enhance error handling and logging in the
fetch_change_logsmethod.- Improve error handling and status code usage in the
getmethod.- Consider using
ImproperlyConfiguredexception for missing settings.Implementing these suggestions will result in more robust, maintainable, and easier to debug code. Great job on the initial implementation!
apiserver/plane/license/urls.py (2)
14-14: LGTM: Import statement addition is correct.The
ChangeLogEndpointimport has been properly added to the existing list of view imports. It follows the established naming convention and is placed appropriately.
Line range hint
1-78: Summary: Changes align well with PR objectives.The additions to this file successfully implement the URL routing for the new changelog feature, which aligns with the PR objective of introducing a product updates modal. The changes are minimal, focused, and consistent with the existing code structure and conventions.
Key points:
- A new
ChangeLogEndpointhas been imported.- A new URL pattern for "changelog/" has been added, using the
ChangeLogEndpoint.These changes provide the necessary backend routing to support the new "What's new in Plane" feature described in the PR objectives.
web/core/components/global/product-updates/footer.tsx (1)
1-62: Overall, the implementation looks good with room for minor improvements.The
ProductUpdatesFootercomponent is well-structured and follows React best practices. It effectively renders a footer section with useful links and a call-to-action button for Plane Pages. The use of Tailwind CSS for styling and custom utility functions promotes consistency.To further enhance the component:
- Group imports for better organization.
- Improve accessibility by adding ARIA roles, labels, and using semantic HTML structure.
- Enhance security for external links.
- Provide more descriptive alt text for images.
These changes will make the component more robust, accessible, and maintainable.
web/core/components/global/product-updates/modal.tsx (2)
1-16: LGTM: Imports and service initialization look good.The imports cover all necessary dependencies for the component's functionality. Creating the
InstanceServiceinstance outside the component is a good practice to avoid unnecessary recreations.
18-21: LGTM: Props type definition is clear and appropriate.The
ProductUpdatesModalPropstype is well-defined withisOpenandhandleCloseproperties, which align with the component's functionality. This type definition enhances code readability and type safety.web/core/components/workspace/sidebar/help-section.tsx (3)
9-9: LGTM: New import for ProductUpdatesModalThe import for
ProductUpdatesModalis correctly placed and appropriately sourced from the global components. This is suitable for a feature that might be used across different parts of the application.
Line range hint
1-165: Summary: Successfully implemented product updates modalThe changes in this file successfully implement the new feature for displaying product updates, as outlined in the PR objectives. The "What's new" button has been added to the help section sidebar, and it triggers a modal to show the latest product updates.
Key points:
- The
ProductUpdatesModalcomponent is properly imported and integrated.- State management for the modal's visibility is implemented correctly.
- The UI changes are minimal and non-disruptive to the existing sidebar structure.
These changes align well with the PR's goal of enhancing user experience by informing users about recent updates in the Plane application through a dedicated modal interface.
16-16: Verify the new import path for PlaneVersionNumberThe import for
PlaneVersionNumberhas been moved to a new location. While this change appears to be part of a component reorganization, it's important to ensure that the new import path is correct and consistent with the project's structure.To verify the correctness of the new import path, please run the following command:
web/app/profile/page.tsx (2)
38-38: LGTM: FileService import uncommentedThe uncommented import of
FileServiceis necessary for the component's functionality, particularly for handling file-related operations such as deleting user avatars. This change aligns with the component's requirements and doesn't introduce any conflicts or issues.
38-38: Minimal impact: FileService import enables existing functionalityUncommenting the
FileServiceimport doesn't introduce new functionality but enables the existinghandleDeletefunction to work correctly. This change has minimal impact on the overall structure and behavior of the component, ensuring that file-related operations (such as deleting user avatars) function as intended.
| @@ -4,6 +4,7 @@ FROM python:3.12.5-alpine AS backend | |||
| ENV PYTHONDONTWRITEBYTECODE 1 | |||
| ENV PYTHONUNBUFFERED 1 | |||
| ENV PIP_DISABLE_PIP_VERSION_CHECK=1 | |||
| ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/ | |||
There was a problem hiding this comment.
💡 Codebase verification
Hardcoded INSTANCE_CHANGELOG_URL detected in Dockerfiles
The INSTANCE_CHANGELOG_URL environment variable is hardcoded in the following Dockerfiles, which may pose security risks and reduce flexibility across different environments:
apiserver/Dockerfile.devapiserver/Dockerfile.api
Recommendations:
- Replace the hardcoded URL with environment variable placeholders to allow configuration at runtime:
ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL} - Store the actual URL in environment-specific configuration files or secure storage solutions such as Kubernetes Secrets or CI/CD pipeline variables.
- Ensure that sensitive tokens or identifiers within URLs are managed securely and are not exposed in the codebase.
Analysis chain
Approve with suggestions: Consider security and flexibility for INSTANCE_CHANGELOG_URL
The addition of the INSTANCE_CHANGELOG_URL environment variable aligns well with the PR objectives for implementing the "What's new in Plane" feature. However, there are some considerations to improve security and flexibility:
-
Instead of hardcoding the actual URL in the Dockerfile, consider using a placeholder value:
ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL:-https://api.example.com/changelog}This allows for easy overriding of the URL at runtime or through environment-specific configurations.
-
Move the actual URL setting to a more secure and flexible method, such as:
- Environment-specific
.envfiles - Kubernetes ConfigMaps or Secrets
- CI/CD pipeline variables
- Environment-specific
-
Please verify:
- Is this URL sensitive? The long alphanumeric string might be a token or identifier.
- Does this URL change across different environments (dev, staging, production)?
These changes will enhance security and make the configuration more adaptable to different deployment scenarios.
To ensure this URL isn't used elsewhere in the codebase, run:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hardcoded instances of the changelog URL
rg --type-not dockerfile "https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/"
Length of output: 140
Script:
#!/bin/bash
# Search for hardcoded instances of the changelog URL excluding Dockerfiles
rg --ignore-file Dockerfile "https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/"
Length of output: 421
| def fetch_change_logs(self): | ||
| response = requests.get(settings.INSTANCE_CHANGELOG_URL) | ||
| response.raise_for_status() | ||
| return response.json() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and logging to fetch_change_logs method.
While the method correctly raises an exception for HTTP errors, it lacks specific error handling and logging. This could make debugging issues more difficult in production.
Consider adding try-except blocks to handle potential exceptions and log errors. Here's a suggested implementation:
import logging
logger = logging.getLogger(__name__)
def fetch_change_logs(self):
try:
response = requests.get(settings.INSTANCE_CHANGELOG_URL)
response.raise_for_status()
return response.json()
except requests.RequestException as e:
logger.error(f"Error fetching changelog: {str(e)}")
raiseThis change will log any errors that occur during the request, making it easier to diagnose issues in production.
| def get(self, request): | ||
| # Fetch the changelog | ||
| if settings.INSTANCE_CHANGELOG_URL: | ||
| data = self.fetch_change_logs() | ||
| return Response(data, status=status.HTTP_200_OK) | ||
| else: | ||
| return Response( | ||
| {"error": "could not fetch changelog please try again later"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and status codes in get method.
The current implementation has a few areas that could be improved:
- The error message when
INSTANCE_CHANGELOG_URLis not set could be more specific. - Returning HTTP 400 (Bad Request) when the URL is not set might not be the most appropriate status code.
- There's no error handling for potential exceptions from
fetch_change_logs.
Consider the following improvements:
from django.core.exceptions import ImproperlyConfigured
def get(self, request):
if not settings.INSTANCE_CHANGELOG_URL:
raise ImproperlyConfigured("INSTANCE_CHANGELOG_URL is not set in settings.")
try:
data = self.fetch_change_logs()
return Response(data, status=status.HTTP_200_OK)
except requests.RequestException as e:
return Response(
{"error": f"Failed to fetch changelog: {str(e)}"},
status=status.HTTP_503_SERVICE_UNAVAILABLE
)This implementation:
- Raises an
ImproperlyConfiguredexception if the URL is not set, which is more appropriate for configuration issues. - Handles potential exceptions from
fetch_change_logs. - Returns a more appropriate HTTP 503 (Service Unavailable) status when the changelog fetch fails.
| @@ -4,6 +4,7 @@ FROM python:3.12.5-alpine AS backend | |||
| ENV PYTHONDONTWRITEBYTECODE 1 | |||
| ENV PYTHONUNBUFFERED 1 | |||
| ENV PIP_DISABLE_PIP_VERSION_CHECK=1 | |||
| ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/ | |||
There was a problem hiding this comment.
💡 Codebase verification
Hardcoding INSTANCE_CHANGELOG_URL May Expose Sensitive Information
The environment variable INSTANCE_CHANGELOG_URL is currently hardcoded in the Dockerfile with a URL containing an identifier (8e1c2e4c7bc5493eb7731be3862f6960) that appears to be sensitive. To enhance security and flexibility, consider the following changes:
- Use a Placeholder for the URL: Replace the hardcoded URL with a placeholder in the Dockerfile.
- Inject the Actual URL at Runtime: Set the real URL through environment variables during container deployment or runtime configuration.
Suggested Change:
- ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/
+ ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL:-https://api.plane.so/api/public/anchor/PLACEHOLDER/pages/}Benefits:
- Security: Prevents exposure of sensitive identifiers in the codebase.
- Flexibility: Allows different environments (development, staging, production) to use unique URLs without modifying the Dockerfile.
- Maintainability: Simplifies updates to the URL without needing to rebuild the Docker image.
Analysis chain
Consider using a placeholder for the INSTANCE_CHANGELOG_URL
While adding this environment variable aligns with the PR objectives for implementing a product updates modal, hardcoding the full URL in the Dockerfile might pose a security risk, especially since it contains what appears to be an API key or identifier (8e1c2e4c7bc5493eb7731be3862f6960).
Consider replacing the hardcoded URL with a placeholder and injecting the actual URL at runtime or during deployment. This approach enhances security and flexibility. Here's a suggested change:
-ENV INSTANCE_CHANGELOG_URL https://api.plane.so/api/public/anchor/8e1c2e4c7bc5493eb7731be3862f6960/pages/
+ENV INSTANCE_CHANGELOG_URL ${INSTANCE_CHANGELOG_URL:-https://api.plane.so/api/public/anchor/PLACEHOLDER/pages/}This change allows you to:
- Set a default placeholder URL in the Dockerfile.
- Override it with the actual URL during container runtime or in your deployment configuration.
- Avoid exposing potentially sensitive information in your Dockerfile.
To ensure this environment variable is used correctly in the application code, please run the following command:
This will help verify that the application is properly configured to use this new environment variable.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of INSTANCE_CHANGELOG_URL in Python files
rg --type python 'INSTANCE_CHANGELOG_URL' -C 3
Length of output: 77
Script:
#!/bin/bash
# Search for usage of INSTANCE_CHANGELOG_URL in Python files using the correct file type alias
rg --type py 'INSTANCE_CHANGELOG_URL' -C 3
Length of output: 1986
| {!isLoading && !!error ? ( | ||
| <div className="flex flex-col items-center justify-center w-full h-full mb-8"> | ||
| <div className="text-lg font-medium">We are having trouble fetching the updates.</div> | ||
| <div className="text-sm text-custom-text-200"> | ||
| Please visit{" "} | ||
| <a | ||
| href="https://go.plane.so/p-changelog" | ||
| target="_blank" | ||
| className="text-sm text-custom-primary-100 font-medium hover:text-custom-primary-200 underline underline-offset-1 outline-none" | ||
| > | ||
| our changelogs | ||
| </a>{" "} | ||
| for the latest updates. | ||
| </div> | ||
| </div> | ||
| ) : isLoading ? ( | ||
| <div className="flex items-center justify-center w-full h-full"> | ||
| <LogoSpinner /> | ||
| </div> |
There was a problem hiding this comment.
Improve error handling and accessibility for loading state.
While the error and loading states are handled, there are opportunities for improvement:
- The error message could be more specific about the nature of the error.
- The loading spinner lacks an accessibility label.
Consider the following improvements:
- Enhance the error message:
-<div className="text-lg font-medium">We are having trouble fetching the updates.</div>
+<div className="text-lg font-medium">Error: Unable to fetch product updates. {error.message}</div>- Add an accessibility label to the loading spinner:
-<LogoSpinner />
+<LogoSpinner aria-label="Loading product updates" />These changes will provide more context for errors and improve accessibility for screen reader users.
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.
| {!isLoading && !!error ? ( | |
| <div className="flex flex-col items-center justify-center w-full h-full mb-8"> | |
| <div className="text-lg font-medium">We are having trouble fetching the updates.</div> | |
| <div className="text-sm text-custom-text-200"> | |
| Please visit{" "} | |
| <a | |
| href="https://go.plane.so/p-changelog" | |
| target="_blank" | |
| className="text-sm text-custom-primary-100 font-medium hover:text-custom-primary-200 underline underline-offset-1 outline-none" | |
| > | |
| our changelogs | |
| </a>{" "} | |
| for the latest updates. | |
| </div> | |
| </div> | |
| ) : isLoading ? ( | |
| <div className="flex items-center justify-center w-full h-full"> | |
| <LogoSpinner /> | |
| </div> | |
| {!isLoading && !!error ? ( | |
| <div className="flex flex-col items-center justify-center w-full h-full mb-8"> | |
| <div className="text-lg font-medium">Error: Unable to fetch product updates. {error.message}</div> | |
| <div className="text-sm text-custom-text-200"> | |
| Please visit{" "} | |
| <a | |
| href="https://go.plane.so/p-changelog" | |
| target="_blank" | |
| className="text-sm text-custom-primary-100 font-medium hover:text-custom-primary-200 underline underline-offset-1 outline-none" | |
| > | |
| our changelogs | |
| </a>{" "} | |
| for the latest updates. | |
| </div> | |
| </div> | |
| ) : isLoading ? ( | |
| <div className="flex items-center justify-center w-full h-full"> | |
| <LogoSpinner aria-label="Loading product updates" /> | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
web/ce/components/global/product-updates-header.tsx (1)
15-17: Consider simplifying thecnfunction usage.The
cnfunction is typically used for conditional class names, but in this case, it's being used with a single string argument. You could simplify this by removing thecnfunction call.You could replace:
className={cn( "px-2 mx-2 py-0.5 text-center text-xs font-medium rounded-full bg-custom-primary-100/20 text-custom-primary-100" )}with:
className="px-2 mx-2 py-0.5 text-center text-xs font-medium rounded-full bg-custom-primary-100/20 text-custom-primary-100"Unless you plan to add conditional classes in the future, this simplification would be more straightforward.
web/core/components/workspace/sidebar/help-section.tsx (3)
140-148: LGTM: "What's new" feature implemented as per PR objectives.The addition of the "What's new" button within the CustomMenu.MenuItem structure effectively implements the feature described in the PR objectives. The placement and functionality are appropriate for a help section item.
Consider adding an aria-label to the button for improved accessibility:
<button type="button" onClick={() => setIsChangeLogOpen(true)} + aria-label="Open changelog" className="flex w-full items-center justify-start text-xs hover:bg-custom-background-80" > <span className="text-xs">What's new</span> </button>
176-177: LGTM: Improved flexibility in sidebar toggle button styling.The use of template literals for conditional class application enhances the button's ability to adapt to different collapse states. This is a good practice in React for dynamic styling.
Consider extracting the className logic into a separate variable for improved readability:
+ const toggleButtonClassName = cn( + "grid place-items-center rounded-md p-1 text-custom-text-200 outline-none hover:bg-custom-background-90 hover:text-custom-text-100", + { + "w-full": isCollapsed + } + ); <button type="button" - className={`grid place-items-center rounded-md p-1 text-custom-text-200 outline-none hover:bg-custom-background-90 hover:text-custom-text-100 ${isCollapsed ? "w-full" : "" - }`} + className={toggleButtonClassName} onClick={() => toggleSidebar()} >This refactoring would make the component more readable and easier to maintain.
Line range hint
39-39: LGTM: ProductUpdatesModal implementation aligns with PR objectives.The ProductUpdatesModal is correctly implemented and tied to the isChangeLogOpen state, which is toggled by the "What's new" button. This directly fulfills the PR objective of introducing a product updates modal.
Consider adding a loading state to improve user experience:
+ const [isLoading, setIsLoading] = useState(false); // ... other code <ProductUpdatesModal isOpen={isChangeLogOpen} - handleClose={() => setIsChangeLogOpen(false)} + handleClose={() => { + setIsChangeLogOpen(false); + setIsLoading(false); + }} + onOpenChange={(open) => { + if (open) setIsLoading(true); + }} + isLoading={isLoading} /> // ... in the "What's new" button onClick handler onClick={() => { setIsChangeLogOpen(true); + setIsLoading(true); }}This enhancement would provide visual feedback to users while the changelog content is being loaded.
web/app/profile/page.tsx (1)
289-290: LGTM! Minor suggestion for readability.The changes to the email input's className improve consistency and maintain the existing functionality. The error styling is preserved, and the input remains disabled as before.
For improved readability, consider using a template literal for the className:
- className={`w-full cursor-not-allowed rounded-md !bg-custom-background-80 ${errors.email ? "border-red-500" : "" - }`} + className={`w-full cursor-not-allowed rounded-md !bg-custom-background-80 ${ + errors.email ? "border-red-500" : "" + }`}This change would make the conditional styling easier to read and maintain.
web/core/components/global/product-updates/modal.tsx (3)
23-23: Consider removing theobserverwrapper if not using MobX observablesThe
ProductUpdatesModalcomponent is wrapped withobserver, but it doesn't appear to utilize any MobX observable state within this component. If there are no observables being used, you might remove theobserverwrapper to reduce unnecessary overhead.
28-32: Review SWR revalidation settings to ensure data freshnessThe
useSWRhook is configured with:
revalidateIfStale: falserevalidateOnFocus: falseThis means the data won't be revalidated when it becomes stale or when the window gains focus. If it's important for users to see the most recent product updates, consider enabling these options or implementing a manual refresh mechanism to ensure data stays current.
40-50: Localize user-facing strings for internationalizationThe strings displayed to users, such as error messages and informational text, are hardcoded in English:
- Line 40:
"We are having trouble fetching the updates."- Lines 42-51:
"Please visit our changelogs for the latest updates."If your application supports multiple languages or might in the future, consider wrapping these strings with a localization function to support internationalization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/app/profile/page.tsx (1 hunks)
- web/ce/components/global/product-updates-header.tsx (1 hunks)
- web/core/components/global/product-updates/modal.tsx (1 hunks)
- web/core/components/workspace/sidebar/help-section.tsx (3 hunks)
🔇 Additional comments (7)
web/ce/components/global/product-updates-header.tsx (3)
10-26: Component structure looks good!The
ProductUpdatesHeadercomponent is well-structured. It's a functional component wrapped withobserver, which is appropriate for components that need to react to observable state changes. The use of an arrow function with an implicit return is suitable for this simple component.
11-25: Styling approach looks good!The use of Tailwind CSS for styling is consistent with modern React practices. The layout seems responsive and well-structured, using flexbox appropriately. The custom color classes (
custom-primary-100) provide a good way to maintain consistent branding.
1-26: Overall, theProductUpdatesHeadercomponent is well-implemented.This component aligns well with the PR objectives of implementing a modal for product updates. It provides a clean and informative header for the "What's new" feature, displaying the application version and branding effectively. The code is well-structured and follows React best practices.
A few minor suggestions have been made to improve robustness and simplify the code slightly, but these are not critical issues. Good job on implementing this feature!
web/core/components/workspace/sidebar/help-section.tsx (2)
8-10: LGTM: Import changes align with new feature implementation.The changes in the import statements are consistent with the PR objectives of introducing a new product updates modal. The addition of UI components like CustomMenu and Tooltip, along with the shift from ProductUpdates to ProductUpdatesModal, indicates an enhancement in how product updates are presented to the user. The inclusion of PlaneVersionNumber also suggests an improvement in displaying version information.
Also applies to: 17-17
Line range hint
1-185: Overall assessment: Changes successfully implement the "What's new" feature.The modifications in this file effectively implement the product updates modal and "What's new" feature as described in the PR objectives. The code quality is good, and the changes are well-integrated into the existing component structure. Minor suggestions for accessibility and user experience improvements have been provided, but these are not critical for the functionality of the new feature.
Great job on implementing this new feature! The changes are well-structured and align perfectly with the PR objectives. Consider the minor suggestions for further enhancements, but overall, this is ready for merging.
web/app/profile/page.tsx (2)
Line range hint
1-478: LGTM! Changes improve code consistency without altering core functionality.The modifications to this file are minimal and focused on improving code cleanliness and consistency. The core functionality of the ProfileSettingsPage component remains intact, with only minor adjustments to the email input field styling and the removal of the
getLayoutfunction.These changes align with good coding practices and don't introduce any apparent issues. The component continues to handle user profile settings effectively, including form management, image uploads, and account deactivation options.
Line range hint
1-1: Verify the impact of removing thegetLayoutfunction.The
getLayoutfunction has been removed as mentioned in the AI-generated summary. This simplifies the component structure, but we should ensure that this change doesn't negatively impact the page layout or routing.Please confirm that:
- The page still renders correctly within the desired layout.
- Any layout-related functionality previously handled by
getLayoutis now properly managed elsewhere.- This change is consistent with the project's current architecture and routing setup.
Run the following script to check for any remaining references to
getLayoutin the codebase:If any results are found, please review them to ensure that the removal of
getLayoutfrom this component doesn't cause any inconsistencies.✅ Verification successful
Verified:
getLayoutfunction removalThe
getLayoutfunction has been successfully removed with no remaining references found in the codebase. The component structure is simplified, and there are no impacts on page layout or routing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to getLayout rg 'getLayout' --type tsx --type tsLength of output: 63
Script:
#!/bin/bash # Search for any remaining references to getLayout using ast-grep ast-grep --lang typescript --pattern '$_.getLayout($$$)' --pattern '$$_.getLayout$$'Length of output: 253
Script:
#!/bin/bash # Search for any remaining references to getLayout using a combined pattern in ast-grep ast-grep --lang typescript --pattern '$_.getLayout($$$) | $_.getLayout$$'Length of output: 75
| import { observer } from "mobx-react"; | ||
| import Image from "next/image"; | ||
| // helpers | ||
| import { cn } from "@/helpers/common.helper"; | ||
| // assets | ||
| import PlaneLogo from "@/public/plane-logos/blue-without-text.png"; | ||
| // package.json | ||
| import packageJson from "package.json"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using an environment variable for the package version.
The direct import of package.json might cause issues in certain build configurations, especially in production environments. Consider using an environment variable to store and access the package version instead.
You could set up an environment variable in your build process and access it like this:
const version = process.env.NEXT_PUBLIC_APP_VERSION || 'unknown';Then use version instead of packageJson.version in your component.
| <div className="ml-5"> | ||
| {data?.id && ( | ||
| <DocumentReadOnlyEditorWithRef | ||
| ref={editorRef} | ||
| id={data.id} | ||
| initialValue={data.description_html ?? "<p></p>"} | ||
| containerClassName="p-0 border-none" | ||
| mentionHandler={{ | ||
| highlights: () => Promise.resolve([]), | ||
| }} | ||
| embedHandler={{ | ||
| issue: { | ||
| widgetCallback: () => <></>, | ||
| }, | ||
| }} | ||
| /> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Handle cases where no product updates are available
If data exists but data.id is falsy, the content area renders nothing, which might confuse users. To enhance user experience, consider displaying a message indicating that no product updates are available.
Apply this diff to address the issue:
- {data?.id && (
+ {data?.id ? (
<DocumentReadOnlyEditorWithRef
ref={editorRef}
id={data.id}
initialValue={data.description_html ?? "<p></p>"}
containerClassName="p-0 border-none"
mentionHandler={{
highlights: () => Promise.resolve([]),
}}
embedHandler={{
issue: {
widgetCallback: () => <></>,
},
}}
/>
+ ) : (
+ <div className="text-lg font-medium">No product updates are available at this time.</div>
)}📝 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.
| <div className="ml-5"> | |
| {data?.id && ( | |
| <DocumentReadOnlyEditorWithRef | |
| ref={editorRef} | |
| id={data.id} | |
| initialValue={data.description_html ?? "<p></p>"} | |
| containerClassName="p-0 border-none" | |
| mentionHandler={{ | |
| highlights: () => Promise.resolve([]), | |
| }} | |
| embedHandler={{ | |
| issue: { | |
| widgetCallback: () => <></>, | |
| }, | |
| }} | |
| /> | |
| )} | |
| </div> | |
| <div className="ml-5"> | |
| {data?.id ? ( | |
| <DocumentReadOnlyEditorWithRef | |
| ref={editorRef} | |
| id={data.id} | |
| initialValue={data.description_html ?? "<p></p>"} | |
| containerClassName="p-0 border-none" | |
| mentionHandler={{ | |
| highlights: () => Promise.resolve([]), | |
| }} | |
| embedHandler={{ | |
| issue: { | |
| widgetCallback: () => <></>, | |
| }, | |
| }} | |
| /> | |
| ) : ( | |
| <div className="text-lg font-medium">No product updates are available at this time.</div> | |
| )} | |
| </div> |
Media
Issue link - WEB-2500
Summary by CodeRabbit
New Features
INSTANCE_CHANGELOG_URLfor configuration.ProductUpdatesHeadercomponent displaying the current version and updates.ProductUpdatesModalfor viewing product updates.ProductUpdatesFooterwith links to documentation and support.SidebarHelpSectionfor improved user interaction with updates.Bug Fixes
Documentation