Disable links #84
Disable links #84
Conversation
WalkthroughThe changes introduce an action column in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (5)
src/app/utils/api.class.ts (1)
46-48: LGTM! Consider minor improvements for robustness.The
deactivateLinkmethod is well-implemented and aligns with the PR objective to disable links. It correctly uses thedeletemethod from the base class and follows RESTful conventions.To further enhance the code:
- Consider adding a specific return type for better type safety.
- A brief comment explaining the method's purpose could improve documentation.
Example improvement:
/** * Deactivates a share link with the given ID. * @param id The ID of the share link to deactivate. * @returns A promise that resolves to the API response. */ async deactivateLink(id: string): Promise<AxiosResponse> { return await this.delete({ url: `/share-links/${id}/deactivate` }); }src/mappers/shlink-mapper.ts (1)
71-71: LGTM! Consider a minor improvement for consistency.The addition of the
activeproperty to themapModelToMiniDtofunction aligns well with the PR objective of "Disable links" and is consistent with other mapper functions in this file. This change allows the active status of a link to be included in the mini DTO, which can be used to determine if a link is disabled.For consistency with other properties in this object, consider moving the
activeproperty closer to the top of the object, perhaps right after theurlproperty. This would group similar metadata properties together:url: encodeSHLink(shlinkModel), +active: shlinkModel.getActive(), files: files?.map((x) => { return { location: `${EXTERNAL_URL}/api/v1/share-links/${shlinkModel.getId()}/endpoints/${x.getId()}?ticket=${ticket}`, contentType: 'application/smart-api-access', embedded: null, }; }), -active: shlinkModel.getActive(),src/app/shared-links/Components/LinksTable.tsx (3)
Line range hint
130-148: Ensure consistency by usingcombinedColsinTableHeadCurrently, the action column header is added manually, which can lead to inconsistencies if columns change. Consider iterating over
combinedColsinTableHeadto keep the headers consistent with the data columns.Apply this diff to update the
TableHead:<TableHead> <TableRow> - {columns.map((column) => ( + {combinedCols.map((column) => ( <TableCell key={column.id} align={column.align} style={{ minWidth: column.minWidth }} > {column.label} </TableCell> ))} - <TableCell>Actions</TableCell> </TableRow> </TableHead>
160-160: Improve thekeyprop forTableCellto ensure uniquenessUsing
column.id + row.activeas a key may not guarantee uniqueness and could lead to rendering issues. It's better to use a combination ofrow.idandcolumn.id.Apply this diff to modify the key:
- key={column.id + row.active} + key={`${row.id}-${column.id}`}
166-170: Addaria-labelto theButtonfor better accessibilitySince the
Buttoncontains only an icon, screen readers might not convey its purpose effectively. Adding anaria-labelwill improve accessibility for users relying on assistive technologies.Apply this diff to include the
aria-label:<Button disabled={!row.active} onClick={() => column.action(row.id)} + aria-label="Deactivate Link" > {column.label} </Button>
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/app/shared-links/Components/LinksTable.tsx (6 hunks)
- src/app/utils/api.class.ts (1 hunks)
- src/domain/dtos/shlink.ts (1 hunks)
- src/mappers/shlink-mapper.ts (1 hunks)
Additional comments not posted (2)
src/app/utils/api.class.ts (1)
46-48: Verify usage of the new deactivateLink methodThe new
deactivateLinkmethod has been successfully added to theApiSHLinkclass. To ensure its proper integration:
- Verify that this method is called in the appropriate places where link deactivation is required.
- Check if any existing UI components or other parts of the codebase need to be updated to use this new functionality.
Run the following script to check for potential usage of the new method:
This will help identify areas where the new method should be integrated or where updates might be necessary.
Verification successful
Run updated script to verify usage of
deactivateLinkmethodThe previous script encountered issues recognizing file types. Please execute the following updated script to search for usages of the
deactivateLinkmethod and any related TODO comments:
Verified usage of the new deactivateLink method
The
deactivateLinkmethod has been successfully added to theApiSHLinkclass. It is not referenced elsewhere in the codebase, and there are no pending TODO comments related to its usage. This confirms that the method is properly integrated without requiring further updates.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potential usage or needed updates for the new deactivateLink method # Test: Search for files that might need to use the new deactivateLink method echo "Files that might need to use deactivateLink:" rg --type typescript --type javascript -l 'apiSharedLink|ApiSHLink' # Test: Check if there are any TODO comments related to link deactivation echo "\nTODO comments related to link deactivation:" rg --type typescript --type javascript 'TODO.*deactivate.*link'Length of output: 405
Script:
# Modified script to search for potential usage or needed updates for the new deactivateLink method # Description: Search for .ts and .js files that might need to use the new deactivateLink method echo "Files that might need to use deactivateLink:" rg --glob "*.ts" --glob "*.js" -l 'deactivateLink' # Check if there are any TODO comments related to link deactivation echo "\nTODO comments related to link deactivation:" rg --glob "*.ts" --glob "*.js" 'TODO.*deactivate.*link'Length of output: 335
src/domain/dtos/shlink.ts (1)
146-146: LGTM! Consider updating Swagger documentation.The addition of the
activeproperty toSHLinkMiniDtois a good improvement. It allows for better representation of the share link's current status.Consider updating the Swagger documentation above the class to include the new
activeproperty. You can add it like this:* active: * type: boolean * description: A boolean indicating whether the share link is currently active. * example: trueTo ensure consistency, let's check if this change is reflected in related components or services:
Please review the output of this script to ensure that all necessary updates have been made in related components or services to handle the new
activeproperty.
|
Because once a link is deactivated, it cannot be reactivated, I suggest adding a confirmation dialog to the link deactivation process. This will prevent accidental deactivation and provide clarity to the user. For example, you can display an alert to the user with the following message : |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (4)
src/app/shared-links/Components/ConfirmationDialog.tsx (3)
10-18: LGTM: Component definition and props are well-structured.The component is correctly defined with appropriate TypeScript types for the props. This ensures type safety and improves code maintainability.
Consider using an interface for the props to improve readability and reusability:
interface ConfirmationDialogProps { confirmDeactivate: () => void; confirmDialogOpen: boolean; setConfirmDialogOpen: (arg: boolean) => void; } export const ConfirmationDialog = ({ confirmDeactivate, confirmDialogOpen, setConfirmDialogOpen, }: ConfirmationDialogProps) => { // ... }
19-38: LGTM: Dialog implementation matches requirements and follows best practices.The ConfirmationDialog component is well-implemented using Material-UI components. The content accurately reflects the requirements specified in the PR comments, and the button actions are correctly set up.
To improve accessibility, consider adding an
aria-labelto the Deactivate button to emphasize its destructive nature:- <Button onClick={confirmDeactivate} color="error" variant="contained"> + <Button onClick={confirmDeactivate} color="error" variant="contained" aria-label="Confirm link deactivation"> Deactivate </Button>
41-41: Consider removing the default export.The ConfirmationDialog component is already exported as a named export on line 10. Having both named and default exports for the same component can lead to inconsistent import styles across the project.
Remove the default export to encourage consistent use of named imports:
- export default ConfirmationDialog;This change promotes a more consistent import style throughout the project:
import { ConfirmationDialog } from './ConfirmationDialog';src/app/shared-links/Components/LinksTable.tsx (1)
112-115: LGTM: Action column implementation with room for expansion.The
actionColumnis correctly implemented using thecreateActionColumnfunction. The TODO comment indicates plans for future actions.Would you like me to create a GitHub issue to track the TODO for adding other actions to the
actionColumn?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/app/shared-links/Components/ConfirmationDialog.tsx (1 hunks)
- src/app/shared-links/Components/LinksTable.tsx (8 hunks)
Additional comments not posted (6)
src/app/shared-links/Components/ConfirmationDialog.tsx (2)
1-8: LGTM: Imports are correct and concise.The imports are well-structured, only including the necessary components from Material-UI. This approach helps to keep the bundle size optimized.
1-41: Overall assessment: Well-implemented component meeting PR requirements.The ConfirmationDialog component has been successfully implemented, addressing the need for a confirmation step before deactivating links. The code follows React and TypeScript best practices, and the dialog content accurately reflects the requirements specified in the PR comments.
Key points:
- Proper use of Material-UI components
- TypeScript for type safety
- Adherence to the specified dialog content
- Correct handling of dialog open/close states and actions
The suggested minor improvements (using an interface for props, adding an aria-label, and removing the default export) will further enhance the code quality and maintainability.
Great job on implementing this feature!
src/app/shared-links/Components/LinksTable.tsx (4)
4-4: LGTM: New imports for deactivation functionality.The added imports are appropriate for implementing the link deactivation feature and confirmation dialog.
Also applies to: 16-21, 32-32
44-59: LGTM: Well-structured interface and function for action columns.The
IActionColumninterface andcreateActionColumnfunction are well-defined and promote code reusability for action columns.
132-133: LGTM: Action column successfully integrated into the table.The changes correctly add the "Actions" column to the table header and render the action buttons in the table body. The implementation looks good and consistent with the rest of the table structure.
Also applies to: 166-166, 174-194
213-217: LGTM: ConfirmationDialog integration for link deactivation.The ConfirmationDialog component is correctly integrated, addressing the PR objective to prevent accidental link deactivation.
To ensure the dialog message matches the suggested text in the PR objectives, please run the following script:
|
@amalessid 👍 looks good. I just left a small comment regarding the Tooltip component. Also make sure to resolve the conflict before merging. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
activestatus for links, enhancing clarity on link states.