Display user's shared links table + Add new links #64
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/app/shared-links/LinksTable.tsx (1)
49-85: Consider moving the mock data to a separate file.The
apiDataMockconstant provides a good representation of the expected data structure, and therowsconstant is created correctly by mapping the mock data using thecreateDatafunction.However, as the component grows, it might be beneficial to move the mock data to a separate file (e.g.,
mockData.ts) to keep the component file focused on the UI logic and improve maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/app/components/Header.tsx (1 hunks)
- src/app/shared-links/LinksTable.tsx (1 hunks)
- src/app/shared-links/page.tsx (1 hunks)
Additional comments not posted (7)
src/app/shared-links/page.tsx (2)
1-3: LGTM!The imports are correctly used for layout, styling, and rendering the links data.
16-16: LGTM!Rendering the
LinksTablecomponent inside theContaineris appropriate for displaying the shared links data. It keeps theSharedLinksPagecomponent focused on layout and styling, while delegating the data fetching and rendering to theLinksTablecomponent.src/app/components/Header.tsx (1)
43-45: LGTM!The new "Dashboard" menu item is correctly implemented and enhances the navigation options in the header menu. The
MenuItemcomponent is properly configured with the necessary props, and the label accurately reflects the purpose of the link. This addition improves the user experience by providing direct access to the dashboard.src/app/shared-links/LinksTable.tsx (4)
1-23: LGTM!The imports and type definitions are appropriate and well-structured.
25-47: LGTM!The
columnsconstant andcreateDatafunction are well-defined and serve their purposes effectively.
86-99: LGTM!The component's state and event handlers are implemented correctly to manage the pagination functionality of the table.
101-151: LGTM!The
LinksTablecomponent's rendering logic is well-structured and follows the Material-UI table components' API correctly. The use ofmapfunctions to render the table header and body based on thecolumnsandrowsdata is efficient and readable. The pagination functionality is correctly integrated with theTablePaginationcomponent, allowing users to navigate through the data.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/app/shared-links/Components/LinksTable.tsx (1)
58-90: TODO: Replace the mock data with real data from an API.The table is currently using mock data, which is fine for testing and development purposes. However, before releasing this feature to production, the mock data should be replaced with real data fetched from an API.
Do you want me to open a GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/app/shared-links/Components/BooleanIcon.tsx (1 hunks)
- src/app/shared-links/Components/LinksTable.tsx (1 hunks)
- src/app/shared-links/page.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/app/shared-links/page.tsx
Additional comments not posted (2)
src/app/shared-links/Components/BooleanIcon.tsx (1)
5-15: LGTM!The
BooleanIconcomponent is well-implemented and follows best practices:
- It correctly renders the appropriate icon based on the boolean status prop.
- It uses a concise and readable ternary operator to conditionally render the icons.
- It uses the Material-UI colors to style the icons, maintaining consistency with the Material-UI theme.
- It has a clear and descriptive name that conveys its purpose.
- It has a clear and descriptive prop type definition.
- It returns a JSX element, which is the expected return type for a React component.
src/app/shared-links/Components/LinksTable.tsx (1)
94-157: LGTM! TheLinksTablecomponent is a good starting point for the shared links feature.The component is well-structured and uses Material-UI components effectively for styling and layout. The pagination functionality is also implemented correctly.
|
@amalessid overall looks great 👍 Either we can have another column which defines the status, or maybe we can do some kind of color code? or maybe grey out the inactive ones to indicate its inactive? |
Yes, I agree. Link's status is definitely important in this scope. I'll be adding that. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
src/app/utils/api.class.ts (1)
8-12: Consider using a more specific type forAxiosRequestConfig.Instead of using
anyfor theAxiosRequestConfigtype, consider using a more specific type to improve type safety. This will help catch potential type-related issues at compile time.src/app/shared-links/Components/LinksTable.tsx (2)
64-155: Consider adding sorting, filtering, and searching capabilities to the table.To enhance the user experience and make it easier to find specific shared links, consider implementing the following features:
- Sorting: Allow users to sort the table by clicking on the column headers.
- Filtering: Allow users to filter the table by specific criteria, such as expiry date or passcode enabled.
- Searching: Allow users to search for specific shared links by name or URL.
64-155: Consider making the table responsive for smaller screens.The table may not look good on smaller screens, such as mobile devices. Consider using Material-UI's responsive table components or implementing custom styles to make the table more mobile-friendly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (6)
- package.json (1 hunks)
- src/app/hooks/useSession.ts (1 hunks)
- src/app/shared-links/Components/AddLinkDialog.tsx (1 hunks)
- src/app/shared-links/Components/LinksTable.tsx (1 hunks)
- src/app/shared-links/page.tsx (1 hunks)
- src/app/utils/api.class.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/app/shared-links/page.tsx
Additional context used
Biome
src/app/shared-links/Components/AddLinkDialog.tsx
[error] 17-17: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
Additional comments not posted (10)
src/app/hooks/useSession.ts (1)
1-19: LGTM!The custom
useSessionhook is a clean and reusable way to manage the session state in the application. It encapsulates the session fetching logic using thegetSessionfunction fromnext-auth/reactand stores the fetched session in the component state using theuseStatehook.The
fetchSessionfunction is memoized using theuseCallbackhook to avoid unnecessary re-creation on re-renders, which is a good optimization. TheuseEffecthook is used to fetch the session when the component mounts, ensuring that the session is available when the component is rendered.The hook returns the
sessionstate, allowing consuming components to easily access the session data.Overall, the implementation is clean, efficient, and promotes code reusability.
src/app/utils/api.class.ts (3)
14-19: LGTM!The
Apiclass and itscreatemethod are implemented correctly. The constructor sets therequestproperty as protected, and thecreatemethod provides a convenient way to make POST requests with the providedurl,data, andparams.
21-31: LGTM!The
find,update, anddeletemethods in theApiclass are implemented correctly. The use ofOmitensures that thedataproperty is not required for thefindanddeletemethods, and the corresponding HTTP methods are used correctly.
34-34: LGTM!Exporting an instance of the
Apiclass asapiClientis a good practice. It allows other parts of the application to use the API client without having to create a new instance each time.package.json (1)
27-27: Verify compatibility and behavior of the updatedaxiospackage.The minor version update to the
axiospackage from^1.7.5to^1.7.7is unlikely to introduce breaking changes. However, it's important to ensure that the update does not introduce any unexpected behavior or compatibility issues with other parts of the codebase.Run the following script to verify the
axiospackage usage and compatibility:Verification successful
Axios usage confirmed. Update approved with testing recommendation.
The axios package is indeed used in the project, with imports and usage found in multiple files:
- src/services/base-service.service.ts
- src/app/utils/api.class.ts
- src/app/shared-links/Components/LinksTable.tsx
The minor version update from 1.7.5 to 1.7.7 is unlikely to cause issues with the current usage. However, it's always a good practice to test the application after updating dependencies to ensure full compatibility.
- Run your test suite to verify that all axios-related functionality works as expected with the new version.
- Manually test any features that involve API calls or use axios directly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify `axios` package usage and compatibility. # Test 1: Search for `axios` imports. Expect: Only valid imports from the `axios` package. rg --type typescript $'import .* from \'axios\'' # Test 2: Search for `axios` usage. Expect: Only valid usage of `axios` methods and properties. rg --type typescript -A 5 $'axios\.'Length of output: 161
Script:
#!/bin/bash # Description: Verify `axios` package usage and compatibility. # Find all JavaScript and TypeScript files echo "JavaScript and TypeScript files:" fd -e js -e jsx -e ts -e tsx # Search for axios imports echo -e "\nAxios imports:" fd -e js -e jsx -e ts -e tsx | xargs rg 'import .* from [\'"]axios[\'"]' # Search for axios usage with more context echo -e "\nAxios usage:" fd -e js -e jsx -e ts -e tsx | xargs rg -A 10 'axios\.'Length of output: 8267
Script:
#!/bin/bash # Description: Verify `axios` package usage and compatibility (attempt 2). # Search for axios imports echo "Axios imports:" rg "import.*from ['\"]axios['\"]" --type js --type ts --type jsx --type tsx # Search for axios usage with context echo -e "\nAxios usage:" rg "axios\." -A 5 --type js --type ts --type jsx --type tsxLength of output: 279
Script:
#!/bin/bash # Description: Final attempt to verify `axios` package usage and compatibility. # Search for axios imports without specifying file types echo "Axios imports:" rg "import.*from ['\"]axios['\"]" # Search for axios usage without specifying file types echo -e "\nAxios usage:" rg "axios\." -A 5Length of output: 1590
src/app/shared-links/Components/AddLinkDialog.tsx (4)
1-16: LGTM!The imports are well-organized and follow a consistent structure. The necessary dependencies from React, Material-UI, custom hooks, and utilities are imported correctly.
20-36: LGTM!The styled components for the dialog title, content, and actions are created using the
styledfunction from Material-UI. The styles are applied consistently and follow a similar pattern for each component.
38-48: LGTM!The
createSHLinkfunction is implemented correctly, making an API call to create a shared link using the providedurlanddata. The optionalcallbackfunction is invoked after a successful API call.The
TCreateSHLinkDtotype is a valid extension ofCreateSHLinkDtowith an optionalconfigExpproperty.
50-127: LGTM!The
AddLinkDialogcomponent is implemented correctly:
- It uses the
useSessionhook to retrieve user data for the read-only user ID field.- The form state is managed using the
useStatehook and thepayloadstate variable.- The
handleChangefunction updates thepayloadstate based on form input changes.- The dialog is rendered with the necessary form fields, including user ID, name, PIN code, and expiration date.
- The "Create" button triggers the
createSHLinkfunction with the form data and invokes thecallbackandonClosefunctions.src/app/shared-links/Components/LinksTable.tsx (1)
64-155: LGTM!The
LinksTablecomponent is well-implemented and follows the best practices for React components. It uses the Material-UI components, which are well-tested and accessible. It also uses theuseSessionhook to keep the data in sync with the session, and theuseEffecthook to fetch the data when the user's ID changes. The component is modular and reusable, and it uses theuseStatehook to manage the state of the pagination and the dialog. The component is also using theaxioslibrary to make the API request, which is a good choice for a simple API request. The component is using theSHLinkMiniDtointerface to type the data, which is a good way to keep the data typed and avoid runtime errors. The component is using theBooleanIconcomponent to display the boolean values, which is a good way to keep the table readable and accessible. The component is using theCalendarTodayIconcomponent to display the expiry date, which is a good way to keep the table readable and accessible. The component is using thePapercomponent to wrap the table, which is a good way to keep the table responsive and accessible. The component is using theGridcomponent to align the button to the right, which is a good way to keep the layout responsive and accessible. The component is using theTableContainercomponent to set a max height for the table, which is a good way to keep the table responsive and accessible. The component is using theTableHeadcomponent to display the table headers, which is a good way to keep the table readable and accessible. The component is using theTableBodycomponent to display the table rows, which is a good way to keep the table readable and accessible. The component is using theTableRowcomponent to display the table rows, which is a good way to keep the table readable and accessible. The component is using theTableCellcomponent to display the table cells, which is a good way to keep the table readable and accessible. The component is using theTablePaginationcomponent to display the pagination, which is a good way to keep the table readable and accessible. The component is using theButtoncomponent to display the button, which is a good way to keep the button accessible. The component is using thesxprop to set the styles, which is a good way to keep the styles local to the component. The component is using thearia-labelprop to set the accessibility label, which is a good way to keep the table accessible. The component is using thetabIndexprop to set the tab index, which is a good way to keep the table accessible. The component is using thehoverprop to set the hover effect, which is a good way to keep the table readable and accessible. The component is using thekeyprop to set the key for the table rows, which is a good way to keep the table performant. The component is using thealignprop to set the alignment for the table cells, which is a good way to keep the table readable and accessible. The component is using thestyleprop to set the styles for the table cells, which is a good way to keep the styles local to the component. The component is using therowsPerPageOptionsprop to set the options for the pagination, which is a good way to keep the pagination readable and accessible. The component is using thecomponentprop to set the component for the pagination, which is a good way to keep the pagination accessible. The component is using thecountprop to set the total number of rows for the pagination, which is a good way to keep the pagination readable and accessible. The component is using therowsPerPageprop to set the number of rows per page for the pagination, which is a good way to keep the pagination readable and accessible. The component is using thepageprop to set the current page for the pagination, which is a good way to keep the pagination readable and accessible. The component is using theonPageChangeprop to set the callback for the page change for the pagination, which is a good way to keep the pagination readable and accessible. The component is using theonRowsPerPageChangeprop to set the callback for the rows per page change for the pagination, which is a good way to keep the pagination readable and accessible.
src/app/utils/api.class.ts
Outdated
| const instance = axios.create({ | ||
| baseURL: 'http://localhost:3000', | ||
| withCredentials: true, | ||
| }); |
There was a problem hiding this comment.
Consider using environment variables for the base URL and ensure CSRF protection.
Please consider the following suggestions:
- Use environment variables to set the base URL based on the deployment environment. Hardcoding the base URL could lead to issues when deploying the application to different environments.
- Ensure that proper CSRF protection measures are in place when using
withCredentials. EnablingwithCredentialswithout proper security measures could expose the application to CSRF attacks.
amalessid
changed the title
yassinedorbozgithub
force-pushed
the
61-show-user-shared-links-in-material-ui-tab-structure
branch
from
eb142ea
to
05e3e9b
Compare
src/app/utils/api.class.ts
Outdated
| import axios, { type AxiosInstance, type AxiosRequestConfig } from 'axios'; | ||
|
|
||
| const instance = axios.create({ | ||
| baseURL: 'http://localhost:3000/api/v1', |
There was a problem hiding this comment.
perhaps let's make this an environment variable which is substitutable at runtime. We can follow this article as a reference https://blog.innei.ren/nextjs-runtime-env-and-build-once-deploy-many?locale=en.
src/app/utils/api.class.ts
Outdated
| params?: AxiosRequestConfig<any>; | ||
| } | ||
|
|
||
| class Api { |
There was a problem hiding this comment.
Perhaps we can make this as a base class and make service specifics classes for shlinks, shlink-endpoints, etc so that we do not need to pass the url in the components. This will also help with refactoring should we look to use different endpoint names in future
There was a problem hiding this comment.
It's a good idea 🚀
src/app/utils/api.class.ts
Outdated
| } | ||
| } | ||
|
|
||
| export const apiClient = new Api(instance); |
There was a problem hiding this comment.
Should we not define an interceptor that populates the bearer token before sending a request?
There was a problem hiding this comment.
The cookies set it after the login allow us to interact with the API endpoints without the bearer token
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (3)
src/app/utils/api.types.ts (1)
3-7: LGTM: Well-structured IApiProps interface with a minor suggestionThe
IApiPropsinterface is well-defined with appropriate required and optional properties for API requests. However, consider using a more specific type instead ofanyforAxiosRequestConfig<any>to improve type safety.Consider replacing
AxiosRequestConfig<any>with a more specific type if possible, e.g.,AxiosRequestConfig<object>or a custom type that fits your use case. This will provide better type checking and reduce the risk of runtime errors..env.example (1)
19-19: LGTM! Consider adding a comment for clarity.The addition of
NEXT_PUBLIC_API_URLis appropriate and aligns with the PR objectives. It provides a configurable API endpoint that will be useful for implementing the shared links feature.Consider adding a brief comment above this line to explain its purpose:
+# API endpoint for client-side requests NEXT_PUBLIC_API_URL=${NEXTAUTH_URL}/api/v1This will help other developers understand the intended use of this variable.
.env (1)
19-19: LGTM! Consider adding a comment for clarity.The addition of
NEXT_PUBLIC_API_URLis a good approach for centralizing the API endpoint configuration. It correctly uses theNEXTAUTH_URLas a base, ensuring consistency with the authentication setup.Consider adding a brief comment above this line to explain its purpose:
+# Public API URL for client-side requests NEXT_PUBLIC_API_URL=${NEXTAUTH_URL}/api/v1This will help other developers quickly understand the intention behind this environment variable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (8)
- .env (1 hunks)
- .env.example (1 hunks)
- envConfig.ts (1 hunks)
- package.json (1 hunks)
- src/app/shared-links/Components/AddLinkDialog.tsx (1 hunks)
- src/app/shared-links/Components/LinksTable.tsx (1 hunks)
- src/app/utils/api.class.ts (1 hunks)
- src/app/utils/api.types.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- package.json
- src/app/shared-links/Components/LinksTable.tsx
Additional context used
Biome
src/app/shared-links/Components/AddLinkDialog.tsx
[error] 18-18: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
Additional comments not posted (7)
src/app/utils/api.types.ts (3)
1-1: LGTM: Correct import of AxiosRequestConfig typeThe import statement is correctly using the
typekeyword for a type-only import from the 'axios' library, which is a good TypeScript practice.
1-11: Overall, well-structured API types with room for minor improvementsThe
api.types.tsfile introduces well-defined types for API interactions, which aligns with the PR objectives of implementing a feature to display and add shared links. The use of TypeScript interfaces and enums provides a good foundation for type-safe API calls.Consider the following improvements:
- Use more specific types instead of
anywhere possible.- Standardize naming conventions, especially for enum members.
- As the application grows, ensure to add other endpoints to the
EEndpointenum or consider splitting endpoints into separate enums if they become numerous.These types will contribute to a more robust and maintainable codebase as you implement the shared links feature.
9-11: LGTM: EEndpoint enum with suggestions for improvementThe use of an enum for endpoints is a good practice. However, there are a few points to consider:
The enum currently contains only one endpoint. If this is intentional for the current feature, that's fine, but ensure to add other endpoints as the application grows.
The naming convention for the enum member uses camelCase ('shareLinks'), which is unconventional in TypeScript. Consider using PascalCase for enum members.
Consider updating the enum member to use PascalCase:
export enum EEndpoint { ShareLinks = 'share-links', }To ensure consistency across the codebase, let's check for other potential endpoint definitions:
Verification successful
LGTM: EEndpoint enum with minor suggestions
The use of an enum for endpoints is appropriate. Currently, only one endpoint is defined.
Suggestions:
Naming Convention: The enum member
shareLinksuses camelCase, which is unconventional in TypeScript. Consider renaming it to PascalCase for consistency.export enum EEndpoint { ShareLinks = 'share-links', }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other endpoint definitions or usages rg --type typescript -i '(enum|const).*endpoint'Length of output: 85
Script:
#!/bin/bash # Corrected search for other endpoint definitions or usages in TypeScript files rg -i '(enum|const).*endpoint' --glob '*.ts' --glob '*.tsx'Length of output: 3784
src/app/utils/api.class.ts (2)
1-47: Overall implementation looks great with room for minor enhancements.The API handling implementation in this file is well-structured and addresses many of the concerns raised in previous comments. Here's a summary of the strengths and areas for potential improvement:
Strengths:
- Use of environment variables for the base URL.
- Implementation of a base API class with derived service-specific classes.
- Use of TypeScript for improved type safety.
- Consistent method signatures and naming conventions.
Areas for potential enhancement:
- Implement CSRF protection measures.
- Add error handling and request/response interceptors.
- Use more specific types for method parameters (e.g., in
createLink).- Consider a factory function for the exported API instance to facilitate testing.
Overall, this is a solid foundation for API interactions in the application. Addressing the suggested enhancements will further improve the robustness and maintainability of the code.
1-9: Approve use of environment variables for base URL and suggest CSRF protection.Great job on using environment variables for the base URL. This addresses the previous comment about making it configurable for different environments.
However, as mentioned in a past comment, ensure that proper CSRF protection measures are in place, especially when using
withCredentials(which isn't visible in this file but might be used elsewhere).To verify CSRF protection implementation, run the following script:
src/app/shared-links/Components/AddLinkDialog.tsx (2)
21-37: Styled components look goodThe styled components for the dialog title, content, and actions are well-defined and provide a consistent custom appearance for the dialog.
39-47: Type definitions are well-structuredThe
TCreateSHLinkDtotype andAddLinkDialogPropsinterface are well-defined and provide clear type information for the component.
| import { loadEnvConfig } from '@next/env'; | ||
|
|
||
| const projectDir = process.cwd(); | ||
| loadEnvConfig(projectDir); |
There was a problem hiding this comment.
Add error handling and logging.
While the current implementation is functional, it doesn't handle potential errors or provide any logging. This could make debugging issues related to environment configuration more difficult.
Consider adding error handling and logging:
import { loadEnvConfig } from '@next/env';
const projectDir = process.cwd();
try {
const { combinedEnv, loadedEnvFiles } = loadEnvConfig(projectDir);
console.log('Environment configuration loaded successfully');
console.log('Loaded env files:', loadedEnvFiles);
// You can add more detailed logging here if needed
// For example, logging specific environment variables (be careful not to log sensitive information)
// console.log('API_URL:', combinedEnv.API_URL);
export { combinedEnv, loadedEnvFiles };
} catch (error) {
console.error('Failed to load environment configuration:', error);
// You might want to throw the error here or handle it in some other way
throw error;
}This modification adds error handling and basic logging, which can help with debugging and provide more information about the environment configuration loading process.
LGTM! Consider exporting the loaded configuration.
The implementation looks correct. It imports the necessary function, gets the current working directory, and loads the environment configuration. However, you might want to consider exporting the result of loadEnvConfig for use in other parts of the application.
Consider modifying the code as follows:
import { loadEnvConfig } from '@next/env';
const projectDir = process.cwd();
-loadEnvConfig(projectDir);
+export const { combinedEnv, loadedEnvFiles } = loadEnvConfig(projectDir);This change would allow other parts of your application to import and use the loaded environment configuration.
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.
| import { loadEnvConfig } from '@next/env'; | |
| const projectDir = process.cwd(); | |
| loadEnvConfig(projectDir); | |
| import { loadEnvConfig } from '@next/env'; | |
| const projectDir = process.cwd(); | |
| export const { combinedEnv, loadedEnvFiles } = loadEnvConfig(projectDir); |
| class BaseApi { | ||
| constructor(protected readonly instance: AxiosInstance) {} | ||
|
|
||
| create({ url, data, params = {} }: IApiProps) { | ||
| return this.instance.post(url, data, params); | ||
| } | ||
|
|
||
| find({ url, params = {} }: Omit<IApiProps, 'data'>) { | ||
| return this.instance.get(url, { params }); | ||
| } | ||
|
|
||
| update({ url, data, params = {} }: IApiProps) { | ||
| return this.instance.put(url, data, params); | ||
| } | ||
|
|
||
| delete({ url, params = {} }: Omit<IApiProps, 'data'>) { | ||
| return this.instance.delete(url, params); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding error handling and request/response interceptors.
The BaseApi class provides a good abstraction for API operations. To enhance its robustness and functionality, consider the following suggestions:
- Implement error handling in each method to provide consistent error responses.
- Add request and response interceptors to the axios instance for tasks like adding authentication headers or handling common error scenarios.
Example of adding an interceptor:
instance.interceptors.response.use(
(response) => response,
(error) => {
// Handle common errors (e.g., 401 Unauthorized)
return Promise.reject(error);
}
);| export class ApiSHLink extends BaseApi { | ||
| constructor(protected readonly instance: AxiosInstance) { | ||
| super(instance); | ||
| } | ||
|
|
||
| async findLinks() { | ||
| return await this.find({ | ||
| url: `/${EEndpoint.shareLinks}`, | ||
| }); | ||
| } | ||
|
|
||
| async createLink(data: object) { | ||
| return await this.create({ url: `/${EEndpoint.shareLinks}`, data }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Approve ApiSHLink implementation and suggest type improvement.
Great job implementing a service-specific class for share links. This addresses the previous suggestion and improves the overall structure of the API handling.
To further enhance type safety, consider defining a more specific type for the data parameter in the createLink method instead of using the generic object type. This will provide better documentation and catch potential errors at compile-time.
Example:
interface CreateLinkData {
// Define the expected properties here
url: string;
// Add other relevant properties
}
async createLink(data: CreateLinkData) {
// ...
}| return ( | ||
| <Dialog open={!!open} fullWidth maxWidth="xs" onClose={() => onClose?.()}> | ||
| <StyledDialogTitle>Create a new Link</StyledDialogTitle> | ||
| <DialogContent style={{ padding: '5px 8px' }}> | ||
| <form onSubmit={handleSubmit(onSubmitForm)}> | ||
| <StyledDialogContent> | ||
| <TextField | ||
| label="User id" | ||
| error={!!errors.userId} | ||
| disabled | ||
| required | ||
| inputProps={{ readOnly: true }} | ||
| {...register('userId', {})} | ||
| /> | ||
| <TextField | ||
| label="Name" | ||
| error={!!errors.name} | ||
| required | ||
| helperText={errors.name ? errors.name.message : null} | ||
| placeholder="Name" | ||
| {...register('name', { required: 'Required field' })} | ||
| /> | ||
| <TextField | ||
| type="number" | ||
| label="PIN Code" | ||
| {...register('configPasscode', { | ||
| setValueAs: (value) => value || undefined, | ||
| })} | ||
| /> | ||
| <TextField | ||
| type="date" | ||
| label="Expiration Date" | ||
| InputLabelProps={{ shrink: true }} | ||
| {...register('configExp', { | ||
| setValueAs: (value) => value || undefined, | ||
| })} | ||
| /> | ||
| </StyledDialogContent> | ||
| </form> | ||
| </DialogContent> | ||
| <StyledDialogActions> | ||
| <Button color="inherit" variant="contained" onClick={onClose}> | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| type="submit" | ||
| color="success" | ||
| variant="contained" | ||
| onClick={handleSubmit(onSubmitForm)} | ||
| > | ||
| Create | ||
| </Button> | ||
| </StyledDialogActions> | ||
| </Dialog> | ||
| ); |
There was a problem hiding this comment.
Enhance accessibility for form inputs
The form inputs could benefit from improved accessibility attributes.
Consider adding aria-describedby attributes to connect error messages with their respective inputs:
<TextField
label="Name"
error={!!errors.name}
required
helperText={errors.name ? errors.name.message : null}
placeholder="Name"
aria-describedby={errors.name ? "name-error" : undefined}
{...register('name', { required: 'Required field' })}
/>
{errors.name && <span id="name-error" style={{ display: 'none' }}>{errors.name.message}</span>}Apply similar changes to other form inputs to improve screen reader support.
Note : Currently working with mock data
Summary by CodeRabbit
Summary by CodeRabbit
LinksTablecomponent to display shared links with pagination and formatted data.AddLinkDialogfor creating new shareable links through a user-friendly interface.NEXT_PUBLIC_API_URLfor enhanced API configuration.react-hook-formfor improved form management.These enhancements improve user experience by providing direct access to important features and organizing shared links effectively.