[WIP] Openid #2945
[WIP] Openid #2945
Conversation
The server has a backwards incompatible change of no longer returning a valid token during bootstrap. This is an intentional choice to force the user to log in after configuring the server, in order to make sure the configuration is valid.
…tch options with accents
Create schedules from csv import
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (10)
packages/loot-core/src/server/admin/types/handlers.ts (5)
7-57: LGTM: Well-structured interface with comprehensive admin operations.The
AdminHandlersinterface provides a robust set of admin operations with strong typing, which is excellent for maintainability and type safety. However, consider adding JSDoc comments to each method to explain their purpose, parameters, and return values. This would enhance code readability and make it easier for other developers to understand and use these methods.Example of adding JSDoc comments:
export interface AdminHandlers { /** * Retrieves all users in the system. * @returns A promise that resolves to an array of UserEntity objects or null if no users are found. */ 'users-get': () => Promise<UserEntity[] | null>; // Add similar comments for other methods }
10-12: Consider documenting the null case for 'users-get-access' method.The 'users-get-access' method efficiently retrieves user access information for multiple files. However, it's not clear when the method might return null. Consider adding a comment to explain the circumstances under which null would be returned, as this would help developers handle this case appropriately.
Example:
/** * Retrieves user access information for multiple files. * @param fileIds An array of file IDs to check access for. * @returns A Promise that resolves to a Map of file IDs to UserAccessEntity arrays, or null if [explain null case here]. */ 'users-get-access': ( fileIds: string[], ) => Promise<Map<string, UserAccessEntity[]> | null>;
14-16: Clarify the purpose of the returned IDs in 'user-delete-all' method.The 'user-delete-all' method handles bulk deletion of users and appropriately accounts for partial success scenarios. However, the purpose of the returned
idsarray is ambiguous. It's unclear whether these IDs represent successfully deleted users or those that failed to delete.Consider updating the method signature with a more descriptive name for the IDs array:
'user-delete-all': ( ids: string[], ) => Promise<{ someDeletionsFailed: boolean; failedDeletionIds?: string[] }>;Also, add a comment explaining the meaning of the returned data:
/** * Attempts to delete multiple users. * @param ids An array of user IDs to delete. * @returns A Promise that resolves to an object indicating if any deletions failed, * and an optional array of user IDs for which deletion failed. */
18-24: LGTM: 'user-add' and 'user-update' methods are well-defined.The 'user-add' and 'user-update' methods are correctly implemented with appropriate error handling and consistent return types. The use of
Omit<UserEntity, 'id'>ensures that the 'id' field is not included in the input, which is correct for these operations.For improved consistency and type safety, consider using a union type for the return value:
type UserOperationResult = { error: string } | { id: string }; 'user-add': ( user: Omit<UserEntity, 'id'>, ) => Promise<UserOperationResult>; 'user-update': ( user: Omit<UserEntity, 'id'>, ) => Promise<UserOperationResult>;This change ensures that either 'error' or 'id' is always present in the return value, but never both.
26-57: LGTM: Comprehensive set of admin operations with strong typing.The remaining methods in the
AdminHandlersinterface provide a wide range of admin operations with appropriate use of TypeScript for type safety. Here are some suggestions for minor improvements:
- For the 'check-file-access' method, consider renaming it to 'is-file-access-granted' for clarity.
- The 'access-add' method returns
Record<string, never>on success, which is equivalent to an empty object. Consider usingvoidinstead for clarity.- Add comments to methods with complex parameter or return types, such as 'access-delete-all' and 'transfer-ownership'.
Example of improved method signature with comments:
/** * Deletes access for specified users to a file. * @param fileId The ID of the file. * @param ids An array of user IDs whose access should be revoked. * @returns A Promise that resolves to an object indicating if some deletions failed, * and an optional array of IDs for which deletion failed. */ 'access-delete-all': ({ fileId, ids, }: { fileId: string; ids: string[]; }) => Promise<{ someDeletionsFailed: boolean; failedIds?: string[] }>;packages/loot-core/src/server/post.ts (2)
70-116: LGTM with suggestions for improvementThe
delfunction is well-implemented and consistent with the existingpostfunction. It correctly handles DELETE requests, timeouts, and error scenarios. However, consider the following improvements:
- Add type annotations to the function parameters and return type for better type safety.
- Refactor the error handling logic into a separate function to reduce code duplication across
post,del, andpatch.- Extract the API call failure logging into a separate function to reduce duplication.
Here's an example of how you could refactor the error handling and logging:
function handleApiResponse(res: Response, text: string, url: string, data: any) { if (res.status !== 'ok') { console.log( `API call failed: ${url}\nData: ${JSON.stringify(data, null, 2)}\nResponse: ${JSON.stringify(res, null, 2)}` ); throw new PostError(res.description || res.reason || 'unknown'); } return res.data; } // Then in your functions: return handleApiResponse(res, text, url, data);This refactoring would reduce duplication and make the code more maintainable.
118-164: Approve with recommendation for significant refactoringThe
patchfunction is correctly implemented and consistent with the existingpostanddelfunctions. However, there's a significant amount of code duplication across these three functions. Consider the following refactoring suggestions:
- Create a generic
sendRequestfunction that can handle different HTTP methods (POST, DELETE, PATCH).- Add type annotations to improve type safety.
- Refactor error handling and logging into separate functions.
Here's an example of how you could refactor to reduce duplication:
type HttpMethod = 'POST' | 'DELETE' | 'PATCH'; async function sendRequest(method: HttpMethod, url: string, data: any, headers: Record<string, string> = {}, timeout: number | null = null): Promise<any> { let text: string; let res: Response; try { const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), timeout); const signal = timeout ? controller.signal : null; res = await fetch(url, { method, body: JSON.stringify(data), signal, headers: { ...headers, 'Content-Type': 'application/json', }, }); clearTimeout(timeoutId); text = await res.text(); } catch (err) { throw new PostError('network-failure'); } throwIfNot200(res, text); try { res = JSON.parse(text); } catch (err) { throw new PostError('parse-json', { meta: text }); } return handleApiResponse(res, text, url, data); } function handleApiResponse(res: any, text: string, url: string, data: any): any { if (res.status !== 'ok') { console.log( `API call failed: ${url}\nData: ${JSON.stringify(data, null, 2)}\nResponse: ${JSON.stringify(res, null, 2)}` ); throw new PostError(res.description || res.reason || 'unknown'); } return res.data; } export const post = (url: string, data: any, headers?: Record<string, string>, timeout?: number | null) => sendRequest('POST', url, data, headers, timeout); export const del = (url: string, data: any, headers?: Record<string, string>, timeout?: number | null) => sendRequest('DELETE', url, data, headers, timeout); export const patch = (url: string, data: any, headers?: Record<string, string>, timeout?: number | null) => sendRequest('PATCH', url, data, headers, timeout);This refactoring significantly reduces code duplication, improves type safety, and makes the code more maintainable. It also makes it easier to add new HTTP methods in the future if needed.
packages/desktop-client/src/components/manager/subscribe/Login.tsx (2)
71-161: OpenID login component implementation is comprehensive.The
OpenIdLogincomponent effectively handles the OpenID login process, including appropriate warnings and configuration options. The use ofuseEffectto check for owner creation is a good practice.Consider adding a brief comment explaining the purpose of the
warnMasterCreationstate, as it might not be immediately clear to other developers why this warning is necessary.
Line range hint
191-301: Main Login component refactoring looks great.The refactoring of the main
Logincomponent significantly improves its flexibility and maintainability. The use of theuseLoginMethodhook and conditional rendering of login methods enhances the user experience.Consider using a constant for the 'internal-error' string in the
getErrorMessagefunction for consistency with other error types.packages/loot-core/src/server/main.ts (1)
1566-1568: Consider using strict equality checksIn the condition checking
loginInfo.loginMethod, using strict equality (===and!==) can prevent unintended type coercion and improve code reliability.Apply this diff to use strict equality checks:
- if ( - typeof loginInfo.loginMethod !== 'string' || - loginInfo.loginMethod == null + if ( + typeof loginInfo.loginMethod !== 'string' || + loginInfo.loginMethod === null ) { loginInfo.loginMethod = 'password'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/desktop-client/src/components/manager/subscribe/Login.tsx (4 hunks)
- packages/desktop-client/vite.config.mts (1 hunks)
- packages/loot-core/src/server/admin/app.ts (1 hunks)
- packages/loot-core/src/server/admin/types/handlers.ts (1 hunks)
- packages/loot-core/src/server/main.ts (10 hunks)
- packages/loot-core/src/server/post.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/vite.config.mts
🧰 Additional context used
🔇 Additional comments (9)
packages/loot-core/src/server/admin/types/handlers.ts (2)
1-5: LGTM: Import statements are appropriate and follow best practices.The import statements correctly import the necessary types from relative paths, which is consistent with TypeScript best practices.
1-57: Overall: Well-structured and comprehensive admin handlers interface.This file defines a robust
AdminHandlersinterface that covers a wide range of administrative operations. The use of TypeScript for strong typing enhances code safety and maintainability. The interface serves as a crucial contract for implementing admin functionality in the system.Key strengths:
- Comprehensive coverage of admin operations
- Consistent use of async/await pattern
- Strong typing for parameters and return values
Suggestions for improvement:
- Add JSDoc comments to all methods for better documentation
- Clarify some method names and return types for improved readability
- Consider using union types for more precise error handling
Overall, this interface provides a solid foundation for the admin functionality of the application. Great job!
packages/loot-core/src/server/admin/app.ts (2)
1-254: Consider a comprehensive refactor for consistency and improved error handlingAfter reviewing all methods in this file, here are the main points to address:
Inconsistent return types: Most methods return different types based on the outcome. Consider standardizing return types across all methods for consistency.
Lack of error handling: Many methods don't handle potential exceptions, especially during JSON parsing. Add try-catch blocks to handle these cases.
Inconsistent patterns: Some methods (e.g.,
multiuser-getandowner-created) don't follow the same pattern as others, particularly regarding user token checks. Evaluate whether this is intentional or if they should be aligned with other methods.Repetitive code: Many methods follow a similar structure. Consider extracting common logic into utility functions to reduce duplication.
Suggested approach for refactoring:
- Define a standard return type interface that all methods will use:
interface ApiResult<T> { success: boolean; error?: string; data?: T; }
- Create utility functions for common operations:
async function getWithAuth<T>(url: string): Promise --- `1-12`: _:warning: Potential issue_ **Consider removing `@ts-strict-ignore` and addressing type issues** The file starts with a `// @ts-strict-ignore` comment, which suppresses strict type checking. It's generally better to address type issues rather than ignoring them. Consider removing this comment and fixing any underlying type problems to improve type safety and catch potential errors early. To identify potential type issues, run the following command: </blockquote></details> <details> <summary>packages/desktop-client/src/components/manager/subscribe/Login.tsx (3)</summary><blockquote> `4-24`: **Improved component structure and imports.** The refactoring of the login functionality into separate components (`PasswordLogin`, `OpenIdLogin`, and `HeaderLogin`) improves the overall structure and maintainability of the code. The new imports, including `useSearchParams` and `OpenIdConfig`, support these changes and enhance type safety. --- `25-69`: **Password login component implementation looks good.** The `PasswordLogin` component is well-implemented, handling state management, error handling, and form submission effectively. The `onEnter` prop has been added to the `BigInput` component, addressing the previous review comment and allowing users to submit the form by pressing enter. --- `163-189`: **Header login component looks good.** The `HeaderLogin` component effectively handles the header-based login process, displaying either a loading state with an animated icon or an error message with a link to password login. This improves the user experience during the automatic login attempt. </blockquote></details> <details> <summary>packages/loot-core/src/server/main.ts (2)</summary><blockquote> `1947-1949`: **Ensure `userToken` is securely transmitted** When making requests that include sensitive tokens or authentication headers, it's important to ensure that the token is securely transmitted and handled. Verify that the `post` function properly handles headers and consider any potential security implications. Please confirm that the `post` function correctly incorporates the headers and that the `userToken` is transmitted over a secure connection. --- `2171-2171`: **Verify that `adminApp` is correctly integrated** The `adminApp` is added to the application using `app.combine(adminApp)`. Ensure that `adminApp` is correctly initialized and that all its routes and middleware are properly configured within the application. Please confirm that `adminApp` functions as expected within the application context, and all administrative routes are accessible and secure. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| app.method('access-delete-all', async function ({ fileId, ids }) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
| if (userToken) { | ||
| try { | ||
| const res = await del( | ||
| getServer().BASE_SERVER + `/admin/access?fileId=${fileId}`, | ||
| { | ||
| token: userToken, | ||
| ids, | ||
| }, | ||
| ); | ||
|
|
||
| if (res) { | ||
| return res; | ||
| } | ||
| } catch (err) { | ||
| return { error: err.reason }; | ||
| } | ||
| } | ||
|
|
||
| return { someDeletionsFailed: true }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize return type for consistency
The access-delete-all method, like other methods in this file, returns different types based on the outcome. To maintain consistency across the codebase and make it easier for consumers to handle responses, consider standardizing the return type.
Here's a suggested refactor:
type AccessDeleteAllResult = {
success: boolean;
error?: string;
someDeletionsFailed?: boolean;
data?: any; // Or a more specific type based on the expected server response
};
app.method('access-delete-all', async function ({ fileId, ids }): Promise<AccessDeleteAllResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const res = await del(
getServer().BASE_SERVER + `/admin/access?fileId=${fileId}`,
{
token: userToken,
ids,
},
);
if (res) {
return { success: true, data: res };
} else {
return { success: false, someDeletionsFailed: true };
}
} catch (err) {
return { success: false, error: err.reason || 'An error occurred' };
}
});This approach provides a consistent structure for the return value, making it easier to handle responses in a uniform way across all methods. It also preserves the someDeletionsFailed flag, which seems to be important information for this operation.
| res = await fetch(getServer().SIGNUP_SERVER + '/login-methods').then(res => | ||
| res.json(), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the get helper function instead of fetch for consistency
In the subscribe-get-login-methods handler, consider replacing the direct fetch call with the existing get helper function to maintain consistency and leverage any built-in error handling and configurations.
Apply this diff to implement the change:
- res = await fetch(getServer().SIGNUP_SERVER + '/login-methods').then(res =>
- res.json(),
- );
+ res = await get(getServer().SIGNUP_SERVER + '/login-methods');📝 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.
| res = await fetch(getServer().SIGNUP_SERVER + '/login-methods').then(res => | |
| res.json(), | |
| ); | |
| res = await get(getServer().SIGNUP_SERVER + '/login-methods'); |
| const res = await get(getServer().BASE_SERVER + '/openid/config'); | ||
|
|
||
| if (res) { | ||
| return JSON.parse(res) as OpenIdConfig; | ||
| } | ||
|
|
||
| return {}; | ||
| }; |
There was a problem hiding this comment.
Handle potential JSON parsing errors in get-openid-config
The JSON.parse(res) call might throw an exception if the response is not valid JSON. Wrapping it in a try-catch block will prevent unhandled exceptions and allow the function to return a meaningful error.
Apply this diff to enhance error handling:
const res = await get(getServer().BASE_SERVER + '/openid/config');
if (res) {
+ try {
return JSON.parse(res) as OpenIdConfig;
+ } catch (e) {
+ return { error: 'parse-failure' };
+ }
}
return {};📝 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.
| const res = await get(getServer().BASE_SERVER + '/openid/config'); | |
| if (res) { | |
| return JSON.parse(res) as OpenIdConfig; | |
| } | |
| return {}; | |
| }; | |
| const res = await get(getServer().BASE_SERVER + '/openid/config'); | |
| if (res) { | |
| try { | |
| return JSON.parse(res) as OpenIdConfig; | |
| } catch (e) { | |
| return { error: 'parse-failure' }; | |
| } | |
| } | |
| return {}; | |
| }; |
| } = JSON.parse(res) || {}; | ||
|
|
||
| if (status === 'error') { | ||
| if (reason === 'unauthorized') { | ||
| return null; | ||
| } else if (reason === 'token-expired') { | ||
| tokenExpired = true; | ||
| } else { | ||
| return { offline: true }; |
There was a problem hiding this comment.
Add error handling when parsing JSON in subscribe-get-user
The JSON.parse(res) call may throw an exception if res is not valid JSON. Wrapping the parsing logic in a try-catch block ensures that parsing errors are handled gracefully, and appropriate error messages are returned.
Apply this diff to add error handling:
let tokenExpired = false;
+ let parsedRes;
try {
- const {
+ parsedRes = JSON.parse(res) || {};
+ } catch (e) {
+ return { error: 'parse-failure' };
+ }
+
+ const {
status,
reason,
data: {
userName = null,
permissions = [],
userId = null,
displayName = null,
loginMethod = null,
} = {},
- } = JSON.parse(res) || {};
+ } = parsedRes;
if (status === 'error') {
if (reason === 'unauthorized') {
return null;
} else if (reason === 'token-expired') {
tokenExpired = true;
} else {
return { offline: true };
}
}📝 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.
| } = JSON.parse(res) || {}; | |
| if (status === 'error') { | |
| if (reason === 'unauthorized') { | |
| return null; | |
| } else if (reason === 'token-expired') { | |
| tokenExpired = true; | |
| } else { | |
| return { offline: true }; | |
| let tokenExpired = false; | |
| let parsedRes; | |
| try { | |
| parsedRes = JSON.parse(res) || {}; | |
| } catch (e) { | |
| return { error: 'parse-failure' }; | |
| } | |
| const { | |
| status, | |
| reason, | |
| data: { | |
| userName = null, | |
| permissions = [], | |
| userId = null, | |
| displayName = null, | |
| loginMethod = null, | |
| } = {}, | |
| } = parsedRes; | |
| if (status === 'error') { | |
| if (reason === 'unauthorized') { | |
| return null; | |
| } else if (reason === 'token-expired') { | |
| tokenExpired = true; | |
| } else { | |
| return { offline: true }; |
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (7)
packages/desktop-client/src/components/LoggedInUser.tsx (4)
41-52: LGTM: Improved state management and type safetyThe changes to the component's props and state management are well-implemented:
- Explicit typing of
triggerRefasHTMLButtonElementenhances type safety.- New state variables (
isOwner,multiuserEnabled) and hooks provide better user management and routing capabilities.- The use of
useMetadataPrefforbudgetIdandcloudFileIdimproves metadata handling.For consistency, consider destructuring the
useAuthhook:const { hasPermission } = useAuth();This change would align with the destructuring pattern used for other hooks in the component.
55-67: LGTM: Improved effect hooks for better data managementThe changes to the
useEffecthooks are well-implemented:
- Including
getUserDatain the dependency array ensures the effect runs when the function changes, improving reliability.- The new effect for updating
isOwnerenhances the component's ability to manage user permissions based on the current file.To optimize the new effect, consider memoizing the
isOwnercalculation:const isOwner = useMemo(() => { return cloudFileId && currentFile ? currentFile.usersWithAccess.some(u => u.userId === userData?.userId) : false; }, [cloudFileId, currentFile, userData?.userId]);This change would eliminate the need for the separate
isOwnerstate and effect, potentially improving performance.
69-114: LGTM: Enhanced menu handling with improved navigationThe changes to the
handleMenuSelectfunction (previouslyonChangePassword) are well-implemented:
- The function now handles a variety of menu selections, improving the component's versatility.
- The use of
window.__navigateensures compatibility with different navigation methods.To improve readability and maintainability, consider extracting the navigation logic into a separate function:
const navigateTo = (path: string) => { if (window.__navigate) { window.__navigate(path); } else { window.location.href = path; } }; // Then in handleMenuSelect: case 'change-password': await closeBudget(); navigateTo('/change-password'); break; // ... (apply similar changes to other cases)This refactoring would reduce repetition and make the function easier to maintain.
145-201: LGTM: Improved menu item generation with enhanced type safetyThe addition of the
MenuItemtype and thegetMenuItemsfunction significantly improves the component's structure and maintainability:
- The
MenuItemtype enhances type safety for menu items.- The
getMenuItemsfunction encapsulates complex logic for determining which menu items to display based on user permissions and application state.To improve flexibility, consider adding an optional
disabledproperty to theMenuItemtype:type MenuItem = { name: string; text: string; disabled?: boolean; };This would allow for more dynamic control over menu item availability without changing the menu structure.
packages/loot-core/src/server/cloud-storage.ts (2)
Line range hint
351-359: LGTM: Improved error handling in upload functionThe enhanced error handling in the
uploadfunction is a significant improvement. It provides more specific error messages based on the error reason, which will help in debugging and user communication.Consider using a switch statement or object lookup for mapping error reasons to messages, which could make the code more maintainable if more error types are added in the future. For example:
const errorMessages = { unauthorized: 'unauthorized', default: 'network' }; throw FileUploadError( errorMessages[err.reason] || errorMessages.default );This approach would make it easier to add or modify error messages in the future.
Line range hint
463-466: LGTM: Enhanced error handling in download functionThe addition of error handling for fetching user file information in the
downloadfunction is a good improvement. It provides better visibility into download failures and includes the fileId in the error, which will be helpful for troubleshooting.Consider using a more robust logging mechanism instead of
console.logfor production environments. You might want to integrate a logging library or use a custom logging function that can be easily configured for different environments. For example:import { logger } from '../utils/logger'; // ... if (userFileInfoRes.status !== 'ok') { logger.error('Could not download file from the server', { fileId, response: userFileInfoRes }); throw FileDownloadError('internal', { fileId }); }This approach would provide more flexibility in how errors are logged and processed in different environments.
packages/desktop-client/src/components/manager/BudgetList.tsx (1)
688-695: Enhance accessibility for UserAccessForFile component.The
SvgUserGroupicon lacks an accessible label, which may cause issues for users relying on screen readers.Consider adding an
aria-labelto theSvgUserGroupicon:<SvgUserGroup aria-label="Show users with file access" style={{ width: 15, height: 15, alignSelf: 'flex-end', opacity: 0.7, }} />This will improve the component's accessibility for users relying on screen readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/desktop-client/src/auth/ProtectedRoute.tsx (1 hunks)
- packages/desktop-client/src/components/LoggedInUser.tsx (4 hunks)
- packages/desktop-client/src/components/manager/BudgetList.tsx (13 hunks)
- packages/loot-core/src/client/reducers/budgets.ts (5 hunks)
- packages/loot-core/src/server/admin/app.ts (1 hunks)
- packages/loot-core/src/server/admin/types/handlers.ts (1 hunks)
- packages/loot-core/src/server/api-models.ts (1 hunks)
- packages/loot-core/src/server/cloud-storage.ts (1 hunks)
- packages/loot-core/src/types/file.d.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/desktop-client/src/auth/ProtectedRoute.tsx
- packages/loot-core/src/server/admin/types/handlers.ts
- packages/loot-core/src/server/api-models.ts
- packages/loot-core/src/types/file.d.ts
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/admin/app.ts
[error] 155-155: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🔇 Additional comments (8)
packages/loot-core/src/server/admin/app.ts (2)
1-255: Overall recommendations for improving consistency and robustnessAfter reviewing the entire file, here are some general recommendations to improve the consistency and robustness of
🧰 Tools
🪛 Biome
[error] 155-155: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
1-12:⚠️ Potential issueConsider removing
// @ts-strict-ignoreand addressing type issuesThe use of
// @ts-strict-ignoreat the top of the file disables strict type checking. This might hide potential type-related issues. Consider removing this directive and addressing any resulting type errors to improve type safety throughout the file.To identify potential type issues, you can run the following command:
This will help pinpoint any type-related issues that need to be addressed.
packages/desktop-client/src/components/LoggedInUser.tsx (1)
4-4: LGTM: Enhanced imports for improved functionality and type safetyThe new imports and type definitions are well-chosen and contribute to the component's enhanced functionality:
useLocation,useNavigate: Improved routing capabilities.useAuth,Permissions: Enhanced authentication and authorization.useMetadataPref: Better management of metadata preferences.useMultiuserEnabled,useServerURL: Support for multi-user environments.RemoteFile,SyncedLocalFile: Improved type safety for file operations.These additions set a solid foundation for the component's expanded capabilities.
Also applies to: 7-7, 9-10, 12-13, 21-21
packages/loot-core/src/server/cloud-storage.ts (2)
25-29: LGTM: Improved type safety for user access managementThe addition of the
UsersWithAccessinterface and the update to theRemoteFileinterface enhance type safety and provide a clear structure for managing user access information. This change aligns well with the PR's objective of improving authentication and authorization mechanisms.Also applies to: 37-38
Line range hint
280-286: LGTM: Enhanced metadata managementThe updates to the metadata handling in the
importBufferfunction are well-implemented. The inclusion oflastUploadedusingmonthUtils.currentDay()and the conditional assignment ofencryptKeyIdimprove the accuracy and completeness of the file metadata.packages/loot-core/src/client/reducers/budgets.ts (3)
51-51: Verify the handling of 'owner' when 'remoteFiles' is unavailableIn the case where
remoteFilesisnull, theownerproperty is set to an empty string'':owner: '',Ensure that downstream components can handle an empty string for the
ownerproperty without issues. Ifnullorundefinedis the standard for unknown owners in your application, consider using one of those values instead.
83-84: Ensure consistent handling in the 'detached' stateIn the 'detached' state, you assign
ownerandusersWithAccesssimilar to the 'synced' state:owner: remote.owner, usersWithAccess: remote.usersWithAccess,Verify that in the 'detached' state, these properties are valid and that any logic depending on them can handle this state correctly.
95-95: Consistent 'owner' assignment when the file is 'broken'When handling a 'broken' file state, the
owneris set to an empty string:owner: '',Confirm that representing the
owneras an empty string aligns with the application's conventions for a 'broken' file. If a different placeholder or value is more appropriate (e.g.,nullor a specific indicator), consider updating it accordingly.
| app.method('user-delete-all', async function (ids) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
| if (userToken) { | ||
| try { | ||
| const res = await post( | ||
| getServer().BASE_SERVER + '/admin/users/delete-all', | ||
| { | ||
| token: userToken, | ||
| ids, | ||
| }, | ||
| ); | ||
|
|
||
| if (res) { | ||
| return res; | ||
| } | ||
| } catch (err) { | ||
| return { error: err.reason }; | ||
| } | ||
| } | ||
|
|
||
| return { someDeletionsFailed: true }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve return type consistency and error handling
The user-delete-all method has inconsistent return types and could benefit from improved error handling. Consider the following improvements:
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
resis falsy but no error is thrown.
Here's a suggested refactor:
type DeleteResult = {
success: boolean;
error?: string;
deletionsFailed?: boolean;
data?: any;
};
app.method('user-delete-all', async function (ids): Promise<DeleteResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const res = await post(
getServer().BASE_SERVER + '/admin/users/delete-all',
{ token: userToken, ids }
);
if (res) {
return { success: true, data: res };
} else {
return { success: false, deletionsFailed: true };
}
} catch (err) {
return { success: false, error: err.reason || 'An error occurred' };
}
});This refactoring provides a consistent return type and improves error handling.
| {!loading && multiuserEnabled && userData?.userName && ( | ||
| <small> | ||
| (logged as: <span>{userData?.displayName}</span>) | ||
| </small> | ||
| )} |
There was a problem hiding this comment.
LGTM: Improved rendering logic with a note on internationalization
The changes to the component's JSX improve its flexibility and maintainability:
- Conditional rendering of user information for multi-user environments.
- Use of
getMenuItemsfunction in the Menu component simplifies the JSX.
However, as noted in a previous review comment, the text "logged as:" is not wrapped with a translation function. To ensure proper internationalization, please update the code as follows:
{!loading && multiuserEnabled && userData?.userName && (
<small>
<Trans>
(logged as: <span>{{displayName: userData?.displayName}}</span>)
</Trans>
</small>
)}This change will ensure that the text can be properly translated in localized versions of the application.
Also applies to: 226-227
| } | ||
| baseMenu.push({ | ||
| name: 'config-server', | ||
| text: serverUrl ? t('Change server URL') : t('Start using a server'), |
There was a problem hiding this comment.
Wrap 'Change server URL' with the translation function
As mentioned in a previous review comment, the string 'Change server URL' is not wrapped with the translation function t(). This may prevent it from being translated in localized versions of the application. Please update the code as follows:
text: serverUrl ? t('Change server URL') : t('Start using a server'),This change will ensure proper internationalization of the menu item text.
| const fetchUsers = useCallback(async () => { | ||
| try { | ||
| setCurrentUserId(userData?.userId ?? ''); | ||
| } catch (error) { | ||
| console.error('Failed to fetch users:', error); | ||
| } | ||
| }, [userData?.userId]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize fetchUsers function.
The fetchUsers function can be simplified since it's only setting the currentUserId based on userData.
Consider simplifying the function:
const fetchUsers = useCallback(() => {
setCurrentUserId(userData?.userId ?? '');
}, [userData?.userId]);This change removes the try-catch block, which isn't necessary for a simple state update. If error handling is needed, it should be implemented where userData is fetched.
| const sortedUsersAccess = [...usersAccess].sort((a, b) => { | ||
| const textA = | ||
| a.userId === currentUserId ? 'You' : (a.displayName ?? a.userName); | ||
| const textB = | ||
| b.userId === currentUserId ? 'You' : (b.displayName ?? b.userName); | ||
| return textA.localeCompare(textB); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize sorting logic with useMemo.
The sorting logic for sortedUsersAccess is recalculated on every render. This can be optimized using useMemo.
Consider using useMemo to memoize the sorted array:
const sortedUsersAccess = useMemo(() => {
return [...usersAccess].sort((a, b) => {
const textA = a.userId === currentUserId ? 'You' : (a.displayName ?? a.userName);
const textB = b.userId === currentUserId ? 'You' : (b.displayName ?? b.userName);
return textA.localeCompare(textB);
});
}, [usersAccess, currentUserId]);This will ensure that the sorting only happens when usersAccess or currentUserId changes, potentially improving performance.
| case 'broken': | ||
| ownerName = 'unknown'; | ||
| Icon = SvgFileDouble; | ||
| status = t('Local'); | ||
| break; |
There was a problem hiding this comment.
Remove duplicate 'broken' case in switch statement.
There's a duplicate case for the 'broken' state. This is likely an oversight during the implementation of the new changes.
Please remove the duplicate case:
case 'local':
Icon = SvgFileDouble;
status = 'Local';
break;
- case 'broken':
- ownerName = 'unknown';
- Icon = SvgFileDouble;
- status = t('Local');
- break;
case 'broken':
ownerName = 'unknown';
Icon = SvgFileDouble;
status = t('Local');
break;📝 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.
| case 'broken': | |
| ownerName = 'unknown'; | |
| Icon = SvgFileDouble; | |
| status = t('Local'); | |
| break; | |
| case 'local': | |
| Icon = SvgFileDouble; | |
| status = 'Local'; | |
| break; | |
| case 'broken': | |
| ownerName = 'unknown'; | |
| Icon = SvgFileDouble; | |
| status = t('Local'); | |
| break; |
| owner: remote.owner, | ||
| usersWithAccess: remote.usersWithAccess, |
There was a problem hiding this comment.
Check for null or undefined values in 'remote.owner' and 'remote.usersWithAccess'
You are assigning owner and usersWithAccess from the remote object:
owner: remote.owner,
usersWithAccess: remote.usersWithAccess,Ensure that remote.owner and remote.usersWithAccess are always defined. If there is a possibility that these properties are null or undefined, consider adding default values or handling those cases to prevent potential runtime errors.
| owner: f.owner, | ||
| usersWithAccess: f.usersWithAccess, |
There was a problem hiding this comment.
Validate 'owner' and 'usersWithAccess' from remote files
When adding remote files not present locally, you include:
owner: f.owner,
usersWithAccess: f.usersWithAccess,Ensure that f.owner and f.usersWithAccess are reliably provided by the remote source. If there's a chance these properties could be missing or undefined, implement checks or provide default values to maintain robustness.
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/loot-core/src/server/admin/app.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/admin/app.ts
[error] 155-155: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🔇 Additional comments (2)
packages/loot-core/src/server/admin/app.ts (2)
247-255: 🛠️ Refactor suggestionEnhance error handling, standardize return type, and consider adding authentication
The
owner-createdmethod could benefit from improved error handling, a more consistent return type, and potentially adding authentication. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Consider adding authentication to align with other methods in this file.
Here's a suggested refactor:
type OwnerCreatedResult = { success: boolean; error?: string; data?: boolean; }; app.method('owner-created', async function (): Promise<OwnerCreatedResult> { const userToken = await asyncStorage.getItem('user-token'); // Consider if authentication is needed for this method // if (!userToken) { // return { success: false, error: 'No user token found' }; // } try { const res = await get(getServer().BASE_SERVER + '/admin/ownerCreated/', { // headers: { 'X-ACTUAL-TOKEN': userToken }, // If authentication is needed }); if (res) { const isOwnerCreated = JSON.parse(res) as boolean; return { success: true, data: isOwnerCreated }; } else { return { success: false, error: 'Empty response from server' }; } } catch (err) { return { success: false, error: err.message || 'An error occurred' }; } });This refactoring provides better error handling, including for JSON parsing, and a consistent return type. It also includes commented-out authentication code, which you should consider implementing if this method requires authentication like other methods in this file.
To verify if authentication is needed for this endpoint, you can check the server-side implementation:
#!/bin/bash # Search for the ownerCreated endpoint implementation rg --type typescript "('/admin/ownerCreated/'|/admin\/ownerCreated\/)" -C 5
237-245: 🛠️ Refactor suggestionEnhance error handling, standardize return type, and consider adding authentication
The
multiuser-getmethod could benefit from improved error handling, a more consistent return type, and potentially adding authentication. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Consider adding authentication to align with other methods in this file.
Here's a suggested refactor:
type MultiUserResult = { success: boolean; error?: string; data?: boolean; }; app.method('multiuser-get', async function (): Promise<MultiUserResult> { const userToken = await asyncStorage.getItem('user-token'); // Consider if authentication is needed for this method // if (!userToken) { // return { success: false, error: 'No user token found' }; // } try { const res = await get(getServer().BASE_SERVER + '/admin/multiuser/', { // headers: { 'X-ACTUAL-TOKEN': userToken }, // If authentication is needed }); if (res) { const isMultiUser = JSON.parse(res) as boolean; return { success: true, data: isMultiUser }; } else { return { success: false, error: 'Empty response from server' }; } } catch (err) { return { success: false, error: err.message || 'An error occurred' }; } });This refactoring provides better error handling, including for JSON parsing, and a consistent return type. It also includes commented-out authentication code, which you should consider implementing if this method requires authentication like other methods in this file.
To verify if authentication is needed for this endpoint, you can check the server-side implementation:
| app.method('user-delete-all', async function (ids) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
| if (userToken) { | ||
| try { | ||
| const res = await del( | ||
| getServer().BASE_SERVER + '/admin/users', | ||
| { | ||
| token: userToken, | ||
| ids, | ||
| }, | ||
| ); | ||
|
|
||
| if (res) { | ||
| return res; | ||
| } | ||
| } catch (err) { | ||
| return { error: err.reason }; | ||
| } | ||
| } | ||
|
|
||
| return { someDeletionsFailed: true }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and return type consistency
The user-delete-all method has inconsistent return types and could benefit from improved error handling. Consider the following improvements:
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
userTokenis falsy explicitly. - Use a try-catch block to handle potential errors from the
delfunction.
Here's a suggested refactor:
type DeleteResult = {
success: boolean;
error?: string;
deletedIds?: string[];
};
app.method('user-delete-all', async function (ids): Promise<DeleteResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const res = await del(
getServer().BASE_SERVER + '/admin/users',
{ token: userToken, ids }
);
return { success: true, deletedIds: res };
} catch (err) {
return { success: false, error: err.reason || 'An error occurred' };
}
});This refactoring provides a consistent return type and improves error handling.
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/admin/app.ts (2)
1-1: Consider removing@ts-strict-ignoreand addressing type issuesThe use of
@ts-strict-ignoreat the top of the file suggests that there might be type-related issues that need to be addressed. It's generally better to fix these issues rather than ignoring them, as it improves type safety and can prevent potential runtime errors.Consider removing the
@ts-strict-ignorecomment and addressing any resulting type errors. This will improve the overall type safety of the code.
1-252: Overall improvements for consistency and robustnessAfter reviewing all methods in this file, here are some general recommendations for improvement:
- Implement consistent error handling and return types across all methods. This will make the API more predictable and easier to use.
- Consider creating utility functions for common operations like token retrieval and API calls to reduce code duplication.
- Add proper TypeScript types for all method parameters and return values to improve type safety.
- Implement proper error logging to aid in debugging and monitoring.
- Consider implementing a more robust authentication mechanism, possibly using a higher-order function to wrap all authenticated methods.
- Remove the
@ts-strict-ignoreat the top of the file and address any underlying type issues.To implement these improvements, consider the following architectural changes:
- Create a base
ApiResult<T>type that all methods can use for their return values:type ApiResult<T> = { success: boolean; error?: string; data?: T; };
- Implement a higher-order function for authenticated API calls:
async function withAuth<T>( operation: (token: string) => Promise<ApiResult<T>> ): Promise<ApiResult<T>> { const userToken = await asyncStorage.getItem('user-token'); if (!userToken) { return { success: false, error: 'No user token found' }; } return operation(userToken); }
- Create utility functions for API calls:
async function apiGet<T>(url: string, token: string): Promise<ApiResult<T>> { try { const res = await get(url, { headers: { 'X-ACTUAL-TOKEN': token } }); return res ? { success: true, data: JSON.parse(res) } : { success: false, error: 'Empty response' }; } catch (err) { return { success: false, error: err.message || 'An error occurred' }; } } // Similar functions for post, patch, and delThese changes will significantly improve the consistency, robustness, and maintainability of the code.
🧰 Tools
🪛 Biome
[error] 152-152: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/loot-core/src/server/admin/app.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/loot-core/src/server/admin/app.ts
[error] 152-152: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
| app.method('users-get', async function () { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| const res = await get(getServer().BASE_SERVER + '/admin/users/', { | ||
| headers: { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }, | ||
| }); | ||
|
|
||
| if (res) { | ||
| const list = JSON.parse(res) as UserEntity[]; | ||
| return list; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and standardize return type
The users-get method could benefit from improved error handling and a more consistent return type. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
resis falsy but no error is thrown.
Here's a suggested refactor:
type UsersGetResult = {
success: boolean;
error?: string;
data?: UserEntity[];
};
app.method('users-get', async function (): Promise<UsersGetResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const res = await get(getServer().BASE_SERVER + '/admin/users/', {
headers: { 'X-ACTUAL-TOKEN': userToken }
});
if (res) {
const list = JSON.parse(res) as UserEntity[];
return { success: true, data: list };
} else {
return { success: false, error: 'Empty response from server' };
}
} catch (err) {
return { success: false, error: err.message || 'An error occurred' };
}
});This refactoring provides better error handling, including for JSON parsing, and a consistent return type.
| app.method('users-get-access', async function (fileIds) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| const res = await post( | ||
| getServer().BASE_SERVER + '/admin/access/get-bulk', | ||
| fileIds, | ||
| { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }, | ||
| ); | ||
|
|
||
| if (res) { | ||
| return new Map<string, UserAccessEntity[]>(res); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and return type consistency
The users-get-access method has minimal error handling and an inconsistent return type. Consider the following improvements:
- Add a try-catch block to handle potential errors, including Map creation errors.
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
resis falsy but no error is thrown.
Here's a suggested refactor:
type UsersGetAccessResult = {
success: boolean;
error?: string;
data?: Map<string, UserAccessEntity[]>;
};
app.method('users-get-access', async function (fileIds): Promise<UsersGetAccessResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const res = await post(
getServer().BASE_SERVER + '/admin/access/get-bulk',
fileIds,
{ 'X-ACTUAL-TOKEN': userToken }
);
if (res) {
const accessMap = new Map<string, UserAccessEntity[]>(res);
return { success: true, data: accessMap };
} else {
return { success: false, error: 'Empty response from server' };
}
} catch (err) {
return { success: false, error: err.message || 'An error occurred' };
}
});This refactoring provides better error handling, including for Map creation, and a consistent return type.
| app.method('user-add', async function (user) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| try { | ||
| const res = await post(getServer().BASE_SERVER + '/admin/users/', user, { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }); | ||
|
|
||
| return res as UserEntity; | ||
| } catch (err) { | ||
| return { error: err.reason }; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize return type and improve error handling
The user-add method has inconsistent return types and could benefit from improved error handling. Consider the following improvements:
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
userTokenis falsy explicitly. - Improve error handling to provide more informative error messages.
Here's a suggested refactor:
type UserAddResult = {
success: boolean;
error?: string;
data?: UserEntity;
};
app.method('user-add', async function (user): Promise<UserAddResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const res = await post(getServer().BASE_SERVER + '/admin/users/', user, {
'X-ACTUAL-TOKEN': userToken,
});
return { success: true, data: res as UserEntity };
} catch (err) {
return { success: false, error: err.reason || 'An error occurred while adding the user' };
}
});This refactoring provides a consistent return type and improves error handling.
| app.method('user-update', async function (user) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| try { | ||
| const res = await patch(getServer().BASE_SERVER + '/admin/users/', user, { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }); | ||
|
|
||
| return res as UserEntity; | ||
| } catch (err) { | ||
| return { error: err.reason }; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize return type and consider combining with user-add
The user-update method has inconsistent return types and is very similar to the user-add method. Consider the following improvements:
- Standardize the return type for consistency.
- Improve error handling to provide more informative error messages.
- Combine
user-addanduser-updateinto a single function to reduce code duplication.
Here's a suggested refactor:
type UserModifyResult = {
success: boolean;
error?: string;
data?: UserEntity;
};
async function modifyUser(user: UserEntity, isUpdate: boolean): Promise<UserModifyResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const method = isUpdate ? patch : post;
const res = await method(getServer().BASE_SERVER + '/admin/users/', user, {
'X-ACTUAL-TOKEN': userToken,
});
return { success: true, data: res as UserEntity };
} catch (err) {
const action = isUpdate ? 'updating' : 'adding';
return { success: false, error: err.reason || `An error occurred while ${action} the user` };
}
}
app.method('user-add', async function (user) {
return modifyUser(user, false);
});
app.method('user-update', async function (user) {
return modifyUser(user, true);
});This refactoring reduces code duplication, provides a consistent return type, and improves error handling for both adding and updating users.
| app.method('access-get', async function (fileId) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| const res = await get( | ||
| `${getServer().BASE_SERVER + '/admin/access/'}?fileId=${fileId}`, | ||
| { | ||
| headers: { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (res) { | ||
| return JSON.parse(res) as UserAccessEntity[]; | ||
| } | ||
| } | ||
|
|
||
| return []; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and standardize return type
The access-get method could benefit from improved error handling and a more consistent return type. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
resis falsy but no error is thrown.
Here's a suggested refactor:
type AccessGetResult = {
success: boolean;
error?: string;
data: UserAccessEntity[];
};
app.method('access-get', async function (fileId): Promise<AccessGetResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found', data: [] };
}
try {
const res = await get(
`${getServer().BASE_SERVER}/admin/access/?fileId=${fileId}`,
{
headers: {
'X-ACTUAL-TOKEN': userToken,
},
},
);
if (res) {
const accessData = JSON.parse(res) as UserAccessEntity[];
return { success: true, data: accessData };
} else {
return { success: false, error: 'Empty response from server', data: [] };
}
} catch (err) {
return { success: false, error: err.message || 'An error occurred', data: [] };
}
});This refactoring provides better error handling, including for JSON parsing, and a consistent return type.
| app.method('access-get-available-users', async function (fileId) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| const res = await get( | ||
| `${getServer().BASE_SERVER + '/admin/access/users'}?fileId=${fileId}`, | ||
| { | ||
| headers: { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (res) { | ||
| return JSON.parse(res) as UserAvailable[]; | ||
| } | ||
| } | ||
|
|
||
| return []; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and standardize return type
The access-get-available-users method could benefit from improved error handling and a more consistent return type. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
resis falsy but no error is thrown.
Here's a suggested refactor:
type AvailableUsersResult = {
success: boolean;
error?: string;
data: UserAvailable[];
};
app.method('access-get-available-users', async function (fileId): Promise<AvailableUsersResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found', data: [] };
}
try {
const res = await get(
`${getServer().BASE_SERVER}/admin/access/users?fileId=${fileId}`,
{
headers: {
'X-ACTUAL-TOKEN': userToken,
},
},
);
if (res) {
const availableUsers = JSON.parse(res) as UserAvailable[];
return { success: true, data: availableUsers };
} else {
return { success: false, error: 'Empty response from server', data: [] };
}
} catch (err) {
return { success: false, error: err.message || 'An error occurred', data: [] };
}
});This refactoring provides better error handling, including for JSON parsing, and a consistent return type.
| app.method('transfer-ownership', async function ({ fileId, newUserId }) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| try { | ||
| await post( | ||
| getServer().BASE_SERVER + '/admin/access/transfer-ownership/', | ||
| { fileId, newUserId }, | ||
| { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }, | ||
| ); | ||
| } catch (err) { | ||
| return { error: err.reason }; | ||
| } | ||
| } | ||
|
|
||
| return {}; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize return type and return meaningful data on success
The transfer-ownership method has inconsistent return types and doesn't return any meaningful data on success. Consider the following improvements:
- Standardize the return type for consistency with other methods.
- Handle the case where
userTokenis falsy explicitly. - Return meaningful data on successful ownership transfer.
Here's a suggested refactor:
type TransferOwnershipResult = {
success: boolean;
error?: string;
data?: {
fileId: string;
newOwnerId: string;
};
};
app.method('transfer-ownership', async function ({ fileId, newUserId }): Promise<TransferOwnershipResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
await post(
getServer().BASE_SERVER + '/admin/access/transfer-ownership/',
{ fileId, newUserId },
{
'X-ACTUAL-TOKEN': userToken,
},
);
return {
success: true,
data: {
fileId,
newOwnerId: newUserId
}
};
} catch (err) {
return { success: false, error: err.reason || 'An error occurred while transferring ownership' };
}
});This refactoring provides a consistent return type and includes confirmation details in the success case, making it more informative for the consumer of this method.
| app.method('file-owner-get', async function (fileId) { | ||
| const userToken = await asyncStorage.getItem('user-token'); | ||
|
|
||
| if (userToken) { | ||
| const res = await get( | ||
| `${getServer().BASE_SERVER + '/admin/file/owner'}?fileId=${fileId}`, | ||
| { | ||
| headers: { | ||
| 'X-ACTUAL-TOKEN': userToken, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (res) { | ||
| return JSON.parse(res) as UserEntity; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and standardize return type
The file-owner-get method could benefit from improved error handling and a more consistent return type. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Handle the case where
resis falsy but no error is thrown.
Here's a suggested refactor:
type FileOwnerResult = {
success: boolean;
error?: string;
data?: UserEntity;
};
app.method('file-owner-get', async function (fileId): Promise<FileOwnerResult> {
const userToken = await asyncStorage.getItem('user-token');
if (!userToken) {
return { success: false, error: 'No user token found' };
}
try {
const res = await get(
`${getServer().BASE_SERVER}/admin/file/owner?fileId=${fileId}`,
{
headers: {
'X-ACTUAL-TOKEN': userToken,
},
},
);
if (res) {
const owner = JSON.parse(res) as UserEntity;
return { success: true, data: owner };
} else {
return { success: false, error: 'Empty response from server' };
}
} catch (err) {
return { success: false, error: err.message || 'An error occurred' };
}
});This refactoring provides better error handling, including for JSON parsing, and a consistent return type.
| app.method('multiuser-get', async function () { | ||
| const res = await get(getServer().BASE_SERVER + '/admin/multiuser/'); | ||
|
|
||
| if (res) { | ||
| return (JSON.parse(res) as boolean) || false; | ||
| } | ||
|
|
||
| return null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling, standardize return type, and consider adding authentication
The multiuser-get method could benefit from improved error handling, a more consistent return type, and potentially adding authentication. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Consider adding authentication to align with other methods in this file.
Here's a suggested refactor:
type MultiUserResult = {
success: boolean;
error?: string;
data?: boolean;
};
app.method('multiuser-get', async function (): Promise<MultiUserResult> {
const userToken = await asyncStorage.getItem('user-token');
// Consider if authentication is needed for this method
// if (!userToken) {
// return { success: false, error: 'No user token found' };
// }
try {
const res = await get(getServer().BASE_SERVER + '/admin/multiuser/', {
// headers: { 'X-ACTUAL-TOKEN': userToken }, // If authentication is needed
});
if (res) {
const isMultiUser = JSON.parse(res) as boolean;
return { success: true, data: isMultiUser };
} else {
return { success: false, error: 'Empty response from server' };
}
} catch (err) {
return { success: false, error: err.message || 'An error occurred' };
}
});This refactoring provides better error handling, including for JSON parsing, and a consistent return type. It also includes commented-out authentication code, which you should consider implementing if this method requires authentication like other methods in this file.
| app.method('owner-created', async function () { | ||
| const res = await get(getServer().BASE_SERVER + '/admin/ownerCreated/'); | ||
|
|
||
| if (res) { | ||
| return JSON.parse(res) as boolean; | ||
| } | ||
|
|
||
| return null; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling, standardize return type, and consider adding authentication
The owner-created method could benefit from improved error handling, a more consistent return type, and potentially adding authentication. Consider the following improvements:
- Add a try-catch block to handle potential errors, including JSON parsing errors.
- Standardize the return type to always return an object with a consistent structure.
- Consider adding authentication to align with other methods in this file.
Here's a suggested refactor:
type OwnerCreatedResult = {
success: boolean;
error?: string;
data?: boolean;
};
app.method('owner-created', async function (): Promise<OwnerCreatedResult> {
const userToken = await asyncStorage.getItem('user-token');
// Consider if authentication is needed for this method
// if (!userToken) {
// return { success: false, error: 'No user token found' };
// }
try {
const res = await get(getServer().BASE_SERVER + '/admin/ownerCreated/', {
// headers: { 'X-ACTUAL-TOKEN': userToken }, // If authentication is needed
});
if (res) {
const isOwnerCreated = JSON.parse(res) as boolean;
return { success: true, data: isOwnerCreated };
} else {
return { success: false, error: 'Empty response from server' };
}
} catch (err) {
return { success: false, error: err.message || 'An error occurred' };
}
});This refactoring provides better error handling, including for JSON parsing, and a consistent return type. It also includes commented-out authentication code, which you should consider implementing if this method requires authentication like other methods in this file.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx (2)
41-42: Simplify mouse event handlers using optional chaining.The current implementation of
onMouseEnterandonMouseLeavecan be simplified using optional chaining.Consider refactoring these lines as follows:
onMouseEnter={() => onHover?.(user.id)} onMouseLeave={() => onHover?.(null)}This change makes the code more concise while maintaining the same functionality.
🧰 Tools
🪛 Biome
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
143-143: Update component display name for consistency.The current display name 'UserRow' doesn't match the component name 'UserDirectoryRow'.
For better consistency and easier debugging, consider updating the display name:
-UserDirectoryRow.displayName = 'UserRow'; +UserDirectoryRow.displayName = 'UserDirectoryRow';This change will ensure that the display name accurately reflects the component name.
packages/desktop-client/src/components/modals/TransferOwnership.tsx (1)
140-166: Action buttons and error handling are well-implemented.The implementation of action buttons and error display is correct and functional. The additional logic in the confirm button ensures proper cleanup after the transfer.
Consider extracting the onPress logic of the confirm button into a separate function for better readability:
const handleConfirm = async () => { await onSave(); close(); await dispatch(closeAndLoadBudget((currentFile as Budget).id)); };Then update the button:
<Button isDisabled={availableUsers.length === 0} onPress={handleConfirm} > Transfer ownership </Button>This change would improve the code's readability and maintainability.
packages/desktop-client/src/components/LoggedInUser.tsx (3)
42-52: LGTM: Enhanced state management and hooksThe new state variables and hooks improve the component's functionality. The updated
useEffectforgetUserDataand the newuseEffectfor checking file access permissions are well-implemented.One minor suggestion:
Consider adding
userDatato the dependency array of the seconduseEffectto ensure it updates when the user data changes:useEffect(() => { if (cloudFileId && currentFile) { setIsOwner( currentFile.usersWithAccess.some( u => u.userId === userData?.userId && u.owner, ), ); } else { setIsOwner(false); } - }, [cloudFileId]); + }, [cloudFileId, userData, currentFile]);Also applies to: 56-59, 61-71
73-118: LGTM: Improved menu selection handlingThe renamed and expanded
handleMenuSelectfunction enhances the component's flexibility and navigation capabilities. The use ofwindow.__navigatewith a fallback towindow.location.hrefis a good approach for compatibility.One minor suggestion:
Consider using a
switchstatement withcaseblocks instead ofif-elsefor better readability and maintainability:switch (type) { case 'change-password': case 'sign-in': case 'config-server': await closeBudget(); if (window.__navigate) { window.__navigate(`/${type}`); } else { window.location.href = `/${type}`; } break; // ... other cases }
Line range hint
28-235: LGTM: Improved component structure and organizationThe overall restructuring and enhancement of the
LoggedInUsercomponent have significantly improved its maintainability and readability. The separation of concerns (e.g., menu item generation, menu selection handling) is well-implemented, making the code more modular and easier to understand.One minor suggestion for further improvement:
Consider extracting the
serverMessagefunction outside of the component to improve readability and potentially make it reusable:const getServerMessage = (serverUrl: string | null, userData: UserData | null) => { if (!serverUrl) { return t('No server'); } if (userData?.offline) { return t('Server offline'); } return t('Server online'); }; // Then in the component: // ... {getServerMessage(serverUrl, userData)} // ...This change would make the main component body more concise and potentially allow the
serverMessagelogic to be reused elsewhere if needed.packages/desktop-client/src/components/modals/EditUser.tsx (1)
258-340: RoleDescription component is well-implementedThe
RoleDescriptioncomponent provides clear and detailed explanations of the different user roles. The use of styled Text components for formatting is appropriate and consistent.Minor suggestion: Consider extracting the repeated style objects into constants to reduce duplication and improve maintainability. For example:
const descriptionTextStyle = { ...styles.verySmallText, color: theme.pageTextLight, }; // Then use it like this: <Text style={descriptionTextStyle}> // ... text content ... </Text>This would make the component more DRY (Don't Repeat Yourself) and easier to maintain.
packages/loot-core/src/server/cloud-storage.ts (3)
Line range hint
359-367: Improved error handling in upload functionThe enhanced error handling in the
uploadfunction is a great improvement. It provides more specific error messages based on the error reason, which aligns well with the PR objectives of enhancing the robustness of the file management system.For consistency, consider using a switch statement or object lookup for error reasons:
if (err instanceof PostError) { const errorReasons = { unauthorized: 'unauthorized', default: 'network' }; throw FileUploadError(errorReasons[err.reason] || errorReasons.default); }This approach would make it easier to add more specific error reasons in the future if needed.
Line range hint
264-269: Enhanced metadata handling in importBuffer functionThe updates to the metadata handling in the
importBufferfunction are well-implemented. The addition oflastUploadedandencryptKeyIdproperties aligns with the PR objectives of enhancing file management functionalities.For improved clarity, consider destructuring the
fileDataobject:const { fileId, groupId, encryptMeta } = fileData; meta = { ...meta, cloudFileId: fileId, groupId, lastUploaded: monthUtils.currentDay(), encryptKeyId: encryptMeta ? encryptMeta.keyId : null, };This approach makes it clearer which properties are being used from the
fileDataobject.
Line range hint
478-481: Improved error handling in download functionThe enhanced error handling for fetching user file information in the
downloadfunction is a valuable improvement. It provides better debugging information and aligns well with the PR objectives of enhancing the robustness of the file management system.For consistency with other error handling in the file, consider using a more specific error reason:
}).catch(err => { console.log('Error fetching file info', err); throw FileDownloadError('file-info-fetch-failure', { fileId }); });This approach would make the error reason more specific and consistent with other error handling patterns in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/desktop-client/src/components/LoggedInUser.tsx (4 hunks)
- packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx (1 hunks)
- packages/desktop-client/src/components/manager/BudgetList.tsx (13 hunks)
- packages/desktop-client/src/components/modals/EditUser.tsx (1 hunks)
- packages/desktop-client/src/components/modals/TransferOwnership.tsx (1 hunks)
- packages/loot-core/src/server/cloud-storage.ts (1 hunks)
- packages/loot-core/src/types/models/user.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/types/models/user.ts
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (20)
packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx (2)
21-25: Good use of memoization and dynamic styling.The component is well-structured with appropriate use of memoization for performance optimization. The dynamic styling based on component state is a good practice.
124-129:⚠️ Potential issueFix duplicate 'name' attribute in Cell component.
The Cell component for the edit button has a duplicate 'name' attribute set to "enabled", which is incorrect.
Update the 'name' attribute to accurately reflect its content:
<Cell - name="enabled" + name="edit" width={80} plain style={{ padding: 0, paddingLeft: 5 }} >This change will improve the semantic accuracy of your component structure.
Likely invalid or redundant comment.
packages/desktop-client/src/components/modals/TransferOwnership.tsx (5)
1-25: Imports and type definitions look good.The necessary dependencies and types are correctly imported, and the
TransferOwnershipPropstype is well-defined. This provides a solid foundation for the component.
75-135: Modal rendering is well-implemented.The modal structure, user selection dropdown, and informative labels are correctly implemented. The component provides clear instructions and warnings to the user, enhancing the user experience. The handling of cases when no users are available is also appropriate.
1-166: Overall, the TransferOwnership component is well-implemented.The component successfully implements the functionality for transferring ownership of a budget file. It has a clear structure, good error handling, and provides a user-friendly interface. The suggested improvements, including fixing the typo in the error setter name, enhancing type safety in the onSave function, and extracting the confirm button logic, will further improve the component's robustness and maintainability.
Great job on implementing this feature! After addressing the mentioned issues, this component will be ready for integration into the main codebase.
27-41:⚠️ Potential issueFix typo in error state setter name.
There's a typo in the error state setter name. Please update it as follows:
- const [error, setSetError] = useState<string | null>(null); + const [error, setError] = useState<string | null>(null);This will correct the naming convention for the error state setter.
Likely invalid or redundant comment.
58-73:⚠️ Potential issueImprove error handling and type safety in onSave function.
The onSave function's logic is correct, but there are a couple of improvements we can make:
- Use the corrected error setter name.
- Improve type safety when handling the cloudFileId.
Please apply the following changes:
async function onSave() { - if (cloudFileId) { + if (typeof cloudFileId === 'string') { const response = await send('transfer-ownership', { - fileId: cloudFileId as string, + fileId: cloudFileId, newUserId: userId, }); const { error } = response || {}; if (!error) { originalOnSave?.(); } else { - setSetError(getUserAccessErrors(error)); + setError(getUserAccessErrors(error)); } } else { - setSetError('Cloud file ID is missing.'); + setError('Cloud file ID is missing or invalid.'); } }These changes will improve type safety and use the correct error setter.
Likely invalid or redundant comment.
packages/desktop-client/src/components/LoggedInUser.tsx (4)
4-4: LGTM: Enhanced imports and type definitionsThe new imports and type definitions improve the component's functionality, type safety, and maintainability. The
MenuItemtype is a good addition for better code organization.Also applies to: 7-7, 9-10, 12-13, 21-21, 28-28, 41-41, 149-152
154-205: LGTM: Well-structured menu item generationThe new
getMenuItemsfunction significantly improves code organization and maintainability by centralizing the menu item generation logic. It correctly handles different user roles and application states, enhancing the component's flexibility.Addressing a past review comment:
As previously noted, the 'Change server URL' text should be wrapped with the translation function. Please update line 170 as follows:
- text: serverUrl ? 'Change server URL' : t('Start using a server'), + text: serverUrl ? t('Change server URL') : t('Start using a server'),
218-222: LGTM: Improved rendering logic with a note on internationalizationThe changes to the component's JSX improve its flexibility and maintainability:
- Conditional rendering of user information for multi-user environments.
- Use of
getMenuItemsfunction in the Menu component simplifies the JSX.Addressing a past review comment:
As previously noted, the text "logged as:" is not wrapped with a translation function. To ensure proper internationalization, please update the code as follows:
{!loading && multiuserEnabled && userData?.userName && ( <small> <Trans> (logged as: <span>{{displayName: userData?.displayName}}</span>) </Trans> </small> )}This change will ensure that the text can be properly translated in localized versions of the application.
Also applies to: 230-231
41-41: LGTM: Improved type safety for triggerRefThe explicit typing of
triggerRefasHTMLButtonElementenhances type safety and helps prevent potential runtime errors. This is a good practice for improving code reliability.packages/desktop-client/src/components/modals/EditUser.tsx (4)
1-35: Imports and type definitions look goodThe imports are appropriate for the component's functionality, and the type definitions for
EditUserPropsandEditUserFinanceAppPropsare well-structured. This provides a solid foundation for type safety in the component.
93-118: EditUserFinanceApp component looks goodThe
EditUserFinanceAppcomponent is well-structured. It correctly wraps theEditUsercomponent with aModaland handles saving and closing operations appropriately. The props are passed correctly, and the component follows React best practices.
1-340: Overall assessment: Well-implemented user management interface with minor improvements neededThis new file implements a comprehensive user management interface with good type safety and clear component structure. The main components (
EditUserFinanceApp,EditUser, andRoleDescription) are well-organized and follow React best practices.Key points:
- The utility functions are helpful, but there's a minor typo to fix.
- Consider improving type safety in error handling.
- The
EditUsercomponent could benefit from some UX improvements, particularly in role selection.- There are a few opportunities to improve code maintainability through constant extraction.
After addressing these minor issues, the implementation will be even more robust and user-friendly.
196-196:⚠️ Potential issueFix typo in htmlFor attribute
There's a typo in the
htmlForattribute of the FormLabel for the display name:- <FormLabel title="Display Name" htmlFor="dispalyname-field" /> + <FormLabel title="Display Name" htmlFor="displayname-field" />Ensure that the corresponding Input component has a matching
idattribute.Likely invalid or redundant comment.
packages/loot-core/src/server/cloud-storage.ts (2)
25-30: Excellent addition of user access management structuresThe new
UsersWithAccessinterface and the updates to theRemoteFileinterface are well-structured and provide improved type safety for user access management. These changes align perfectly with the PR objectives of enhancing user and access management functionalities.Also applies to: 38-39
Line range hint
1-524: Overall assessment: Significant improvements in user access management and error handlingThis PR introduces valuable enhancements to the
cloud-storage.tsfile, aligning well with the objectives of improving user and access management functionalities. The newUsersWithAccessinterface, updates to theRemoteFileinterface, and improved error handling across various functions contribute to a more robust and maintainable codebase.While some minor improvements have been suggested, the overall changes represent a substantial step forward in the file's functionality and error management capabilities.
packages/desktop-client/src/components/manager/BudgetList.tsx (3)
71-73: LGTM: Useful type guard function addedThe
isLocalFilefunction is a well-implemented type guard that will improve type safety and make it easier to work with theFiletype union. This is a good practice in TypeScript.
Line range hint
257-312: LGTM: FileItem component updated correctly for multi-user supportThe
FileItemcomponent has been successfully updated to support multi-user functionality. The conditional rendering of theUserAccessForFilecomponent and the passing ofcurrentUserIdtoFileStateare implemented correctly.
Line range hint
341-377: LGTM: BudgetFiles component updated correctlyThe
BudgetFilescomponent has been correctly updated to accept and pass down thecurrentUserIdprop to its childFileItemcomponents.
| @@ -0,0 +1,143 @@ | |||
| // @ts-strict-ignore | |||
There was a problem hiding this comment.
Consider removing the // @ts-strict-ignore directive.
The presence of this directive suggests that there might be underlying TypeScript issues in the file. It's generally better to address these issues directly rather than suppressing them.
To improve type safety and maintainability:
- Remove the
// @ts-strict-ignoredirective. - Address any TypeScript errors that emerge.
- If you encounter difficulties resolving these errors, please let me know, and I'd be happy to assist you in fixing them.
| function getUserDirectoryErrors(reason: string): string { | ||
| switch (reason) { | ||
| case 'unauthorized': | ||
| return 'You are not logged in.'; | ||
| case 'token-expired': | ||
| return 'Login expired, please login again.'; | ||
| case 'user-cant-be-empty': | ||
| return 'Please enter a value for the username; the field cannot be empty.'; | ||
| case 'role-cant-be-empty': | ||
| return 'Select a role; the field cannot be empty.'; | ||
| case 'user-already-exists': | ||
| return 'The username you entered already exists. Please choose a different username.'; | ||
| case 'not-all-deleted': | ||
| return 'Not all users were deleted. Check if one of the selected users is the server owner.'; | ||
| case 'role-does-not-exists': | ||
| return 'Selected role does not exists, possibly a bug? Visit https://actualbudget.org/contact/ for support.'; | ||
| default: | ||
| return `An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: ${reason})`; | ||
| } | ||
| } | ||
|
|
||
| async function saveUser( | ||
| method: 'user-add' | 'user-update', | ||
| user: User, | ||
| setError: (error: string) => void, | ||
| actions: BoundActions, | ||
| ): Promise<boolean> { | ||
| const { error, id: newId } = (await send(method, user)) || {}; | ||
| if (!error) { | ||
| if (newId) { | ||
| user.id = newId; | ||
| } | ||
| } else { | ||
| setError(getUserDirectoryErrors(error)); | ||
| if (error === 'token-expired') { | ||
| actions.addNotification({ | ||
| type: 'error', | ||
| id: 'login-expired', | ||
| title: 'Login expired', | ||
| sticky: true, | ||
| message: getUserDirectoryErrors(error), | ||
| button: { | ||
| title: 'Go to login', | ||
| action: () => { | ||
| actions.signOut(); | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix typo in error message and consider improving error handling
There's a typo in the error message for the 'role-does-not-exists' case:
- return 'Selected role does not exists, possibly a bug? Visit https://actualbudget.org/contact/ for support.';
+ return 'Selected role does not exist, possibly a bug? Visit https://actualbudget.org/contact/ for support.';Also, consider using a more specific type for the reason parameter in getUserDirectoryErrors to ensure all possible error cases are handled exhaustively.
type ErrorReason = 'unauthorized' | 'token-expired' | 'user-cant-be-empty' | 'role-cant-be-empty' | 'user-already-exists' | 'not-all-deleted' | 'role-does-not-exist';
function getUserDirectoryErrors(reason: ErrorReason): string {
// ... existing switch statement ...
}This change would help catch potential missing error cases at compile-time.
| const [enabled, setEnabled] = useState<boolean>(defaultUser.enabled); | ||
| const [role, setRole] = useState<string>( | ||
| defaultUser.role ?? '213733c1-5645-46ad-8784-a7b20b400f93', | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a constant for the default role ID
The default role ID is hardcoded as a UUID string. It would be better to define this as a constant, preferably imported from a central configuration file. This would make it easier to update if needed and improve code readability.
import { DEFAULT_ROLE_ID } from '../config/roles';
// ...
const [role, setRole] = useState<string>(defaultUser.role ?? DEFAULT_ROLE_ID);| <Select | ||
| disabled={defaultUser.owner} | ||
| options={Object.entries(PossibleRoles)} | ||
| value={role} | ||
| onChange={newValue => setRole(newValue)} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving role selection UX
The current implementation of the role selection uses raw object entries, which might not provide the best user experience. Consider mapping the PossibleRoles to an array of { value, label } objects for clearer role names:
const roleOptions = Object.entries(PossibleRoles).map(([id, name]) => ({
value: id,
label: name,
}));
// ...
<Select
disabled={defaultUser.owner}
options={roleOptions}
value={role}
onChange={newValue => setRole(newValue)}
/>This would display more user-friendly labels in the dropdown while maintaining the correct role IDs for data management.
| } else if (res.status === 403) { | ||
| const data = JSON.parse(res.text())?.data; | ||
| if (data && data.reason === 'token-expired') { | ||
| asyncStorage.removeItem('user-token'); | ||
| window.location.href = '/'; | ||
| } |
There was a problem hiding this comment.
Improve error handling for 403 status
While the addition of specific handling for the 403 status is a good improvement, there are a few issues to address:
- The use of
window.location.hrefmight not be appropriate in a server-side context. Consider using a more generic approach or passing this responsibility to the caller. - The JSON parsing of
res.text()might cause issues if the response is not valid JSON. Consider using a try-catch block to handle potential parsing errors. - The condition
res.status === 403is currently unreachable due to the previous conditionres.status !== 200.
Consider refactoring this section as follows:
if (res.status !== 200) {
if (res.status === 403) {
try {
const data = await res.json();
if (data && data.reason === 'token-expired') {
await asyncStorage.removeItem('user-token');
throw new HTTPError(403, 'Token expired');
}
} catch (error) {
// Handle JSON parsing error or rethrow the original error
throw new HTTPError(403, 'Unauthorized');
}
}
return res.text().then(str => {
throw new HTTPError(res.status, str);
});
}
return res;This approach provides more robust error handling and allows the caller to decide how to handle the token expiration.
| break; | ||
| case 'remote': | ||
| Icon = SvgCloudDownload; | ||
| status = t('Available for download'); | ||
| ownerName = multiuserEnabled ? getOwnerDisplayName() : ''; | ||
| break; | ||
| case 'local': | ||
| Icon = SvgFileDouble; | ||
| status = 'Local'; | ||
| break; | ||
| case 'broken': | ||
| ownerName = 'unknown'; | ||
| Icon = SvgFileDouble; | ||
| status = t('Local'); | ||
| break; | ||
| default: | ||
| Icon = SvgCloudCheck; | ||
| status = t('Syncing'); | ||
| ownerName = multiuserEnabled ? getOwnerDisplayName() : ''; | ||
| break; | ||
| } | ||
|
|
||
| const showOwnerContent = multiuserEnabled && file.owner !== currentUserId; | ||
|
|
||
| return ( | ||
| <View | ||
| style={{ | ||
| color, | ||
| alignItems: 'center', | ||
| flexDirection: 'row', | ||
| marginTop: 8, | ||
| }} | ||
| > | ||
| <Icon | ||
| <View style={{ width: '100%' }}> | ||
| <View | ||
| style={{ | ||
| width: 18, | ||
| height: 18, | ||
| color: 'currentColor', | ||
| color, | ||
| alignItems: 'center', | ||
| flexDirection: 'row', | ||
| marginTop: 8, | ||
| }} | ||
| /> | ||
| > | ||
| <Icon | ||
| style={{ | ||
| width: 18, | ||
| height: 18, | ||
| color: 'currentColor', | ||
| }} | ||
| /> | ||
|
|
||
| <Text style={{ marginLeft: 5 }}>{status}</Text> | ||
| </View> | ||
|
|
||
| <Text style={{ marginLeft: 5 }}>{status}</Text> | ||
| <View style={{ paddingTop: 10, flexDirection: 'row', width: '100%' }}> | ||
| {showOwnerContent && ( | ||
| <View style={{ flexDirection: 'row' }}> | ||
| <Text | ||
| style={{ | ||
| ...styles.altMenuHeaderText, | ||
| ...styles.verySmallText, | ||
| color: theme.pageTextLight, | ||
| }} | ||
| > | ||
| Owner: | ||
| </Text> | ||
| <Text | ||
| style={{ | ||
| ...styles.verySmallText, | ||
| color: theme.pageTextLight, | ||
| paddingLeft: 10, | ||
| }} | ||
| > | ||
| {ownerName} | ||
| </Text> | ||
| </View> | ||
| )} | ||
| </View> | ||
| </View> | ||
| ); | ||
|
|
||
| function getOwnerDisplayName() { | ||
| if ( | ||
| !( | ||
| file.state === 'remote' || | ||
| file.state === 'synced' || | ||
| file.state === 'detached' | ||
| ) | ||
| ) { | ||
| return ''; | ||
| } | ||
|
|
||
| const userFound = file.usersWithAccess?.find(f => f.owner); | ||
| return userFound?.displayName ?? userFound?.userName ?? 'unknown'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Multi-user functionality well implemented
The changes to the FileState component correctly implement the multi-user functionality. The owner information is displayed conditionally based on the multi-user setting and the current user.
However, there's room for optimization in the getOwnerDisplayName function:
Consider memoizing the getOwnerDisplayName function to avoid unnecessary recalculations:
const getOwnerDisplayName = useCallback(() => {
if (
!(
file.state === 'remote' ||
file.state === 'synced' ||
file.state === 'detached'
)
) {
return '';
}
const userFound = file.usersWithAccess?.find(f => f.owner);
return userFound?.displayName ?? userFound?.userName ?? 'unknown';
}, [file.state, file.usersWithAccess]);This optimization will prevent the function from being recreated on every render, potentially improving performance.
| const [currentUserId, setCurrentUserId] = useState(''); | ||
| const userData = useSelector(state => state.user.data); | ||
|
|
||
| const fetchUsers = useCallback(async () => { | ||
| try { | ||
| setCurrentUserId(userData?.userId ?? ''); | ||
| } catch (error) { | ||
| console.error('Failed to fetch users:', error); | ||
| } | ||
| }, [userData?.userId]); | ||
|
|
||
| useEffect(() => { | ||
| if (multiuserEnabled && !userData?.offline) { | ||
| fetchUsers(); | ||
| } | ||
| }, [multiuserEnabled, userData?.offline, fetchUsers]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM with suggestions: User state management implemented
The changes correctly implement state management for the current user. However, the fetchUsers function can be simplified:
Consider simplifying the fetchUsers function:
const fetchUsers = useCallback(() => {
setCurrentUserId(userData?.userId ?? '');
}, [userData?.userId]);This simplification removes the unnecessary try-catch block and async/await, as the function is not actually performing any asynchronous operations.
| type UserAccessForFileProps = { | ||
| fileId: string; | ||
| currentUserId: string; | ||
| }; | ||
|
|
||
| function UserAccessForFile({ fileId, currentUserId }: UserAccessForFileProps) { | ||
| const allFiles = useSelector(state => state.budgets.allFiles || []); | ||
| const remoteFiles = allFiles.filter( | ||
| f => f.state === 'remote' || f.state === 'synced' || f.state === 'detached', | ||
| ) as (SyncedLocalFile | RemoteFile)[]; | ||
| const currentFile = remoteFiles.find(f => f.cloudFileId === fileId); | ||
| const multiuserEnabled = useMultiuserEnabled(); | ||
|
|
||
| let usersAccess = currentFile?.usersWithAccess ?? []; | ||
| usersAccess = usersAccess?.filter(user => user.userName !== '') ?? []; | ||
|
|
||
| const sortedUsersAccess = [...usersAccess].sort((a, b) => { | ||
| const textA = | ||
| a.userId === currentUserId ? 'You' : (a.displayName ?? a.userName); | ||
| const textB = | ||
| b.userId === currentUserId ? 'You' : (b.displayName ?? b.userName); | ||
| return textA.localeCompare(textB); | ||
| }); | ||
|
|
||
| return ( | ||
| <View> | ||
| {multiuserEnabled && usersAccess.length > 0 && ( | ||
| <View | ||
| style={{ | ||
| marginLeft: '5px', | ||
| alignSelf: 'center', | ||
| }} | ||
| > | ||
| <Tooltip | ||
| content={ | ||
| <View | ||
| style={{ | ||
| margin: 5, | ||
| }} | ||
| > | ||
| <Text | ||
| style={{ | ||
| ...styles.altMenuHeaderText, | ||
| ...styles.verySmallText, | ||
| color: theme.pageTextLight, | ||
| }} | ||
| > | ||
| File shared with: | ||
| </Text> | ||
| <View | ||
| style={{ | ||
| padding: 0, | ||
| }} | ||
| > | ||
| {sortedUsersAccess.map(user => ( | ||
| <View key={user.userId} style={{ flexDirection: 'row' }}> | ||
| <SvgUser | ||
| style={{ | ||
| width: 10, | ||
| height: 10, | ||
| opacity: 0.7, | ||
| marginTop: 3, | ||
| marginRight: 5, | ||
| }} | ||
| /> | ||
| <View | ||
| style={{ | ||
| ...styles.verySmallText, | ||
| color: theme.pageTextLight, | ||
| margin: 0, | ||
| listStylePosition: 'inside', | ||
| }} | ||
| > | ||
| {user.userId === currentUserId | ||
| ? 'You' | ||
| : (user.displayName ?? user.userName)} | ||
| </View> | ||
| </View> | ||
| ))} | ||
| </View> | ||
| </View> | ||
| } | ||
| placement="bottom end" | ||
| > | ||
| <SvgUserGroup | ||
| style={{ | ||
| width: 15, | ||
| height: 15, | ||
| alignSelf: 'flex-end', | ||
| opacity: 0.7, | ||
| }} | ||
| /> | ||
| </Tooltip> | ||
| </View> | ||
| )} | ||
| </View> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM with suggestions: UserAccessForFile component well-implemented
The UserAccessForFile component is a good addition, providing clear information about file access. However, there are two areas for potential improvement:
Memoize the sorting logic to avoid unnecessary recalculations:
const sortedUsersAccess = useMemo(() => {
return [...usersAccess].sort((a, b) => {
const textA = a.userId === currentUserId ? 'You' : (a.displayName ?? a.userName);
const textB = b.userId === currentUserId ? 'You' : (b.displayName ?? b.userName);
return textA.localeCompare(textB);
});
}, [usersAccess, currentUserId]);Improve accessibility:
- Add an
aria-labelto theSvgUserGroupicon:
<SvgUserGroup
aria-label="Show users with file access"
style={{
width: 15,
height: 15,
alignSelf: 'flex-end',
opacity: 0.7,
}}
/>- Ensure the
Tooltipcomponent supports keyboard navigation. If it doesn't already, consider implementing this functionality or using an accessible tooltip library.
These changes will improve performance and make the component more accessible to all users.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/LoggedInUser.tsx (2)
41-41: Consider using a more specific type for triggerRefThe
triggerRefcould benefit from a more specific type that includes the expected button interface.- const triggerRef = useRef<HTMLButtonElement>(null); + const triggerRef = useRef<HTMLButtonElement & { getBoundingClientRect(): DOMRect }>(null);
154-205: Simplify menu items generation logicThe
getMenuItemsfunction has deeply nested conditions that make it hard to maintain. Consider breaking it down into smaller functions for each menu section.+ const getBaseMenuItems = (): MenuItem[] => { + const items: MenuItem[] = []; + if (serverUrl && !userData?.offline && userData?.loginMethod === 'password') { + items.push({ name: 'change-password', text: t('Change password') }); + } + if (serverUrl) { + items.push({ name: 'sign-out', text: t('Sign out') }); + } + items.push({ + name: 'config-server', + text: serverUrl ? t('Change server URL') : t('Start using a server'), + }); + return items; + }; + const getAdminMenuItems = (): MenuItem[] => { + if (!multiuserEnabled || !isAdmin) return []; + const items: MenuItem[] = []; + // ... admin menu logic + return items; + }; const getMenuItems = (): (MenuItem | typeof Menu.line)[] => { - // current implementation + const adminItems = getAdminMenuItems(); + const baseItems = getBaseMenuItems(); + return [...adminItems, adminItems.length > 0 ? Menu.line : null, ...baseItems].filter(Boolean); };packages/desktop-client/src/components/ManageRules.tsx (1)
Line range hint
103-157: Consider user-scoping for rules in multi-user context.Given that this PR implements OpenID authentication, consider adding user-scoping to rules management. The
loadRulesfunction might need to handle user-specific rules in a multi-user context.Consider:
- Adding user context to rule loading
- Implementing access control for rule management
- Adding user filtering capabilities
Example enhancement for the loadRules function:
async function loadRules() { setLoading(true); let loadedRules = null; if (payeeId) { loadedRules = await send('payees-get-rules', { id: payeeId, userId: currentUser?.id, // Add user context }); } else { loadedRules = await send('rules-get', { userId: currentUser?.id, // Add user context }); } setAllRules(loadedRules); return loadedRules; }packages/desktop-client/src/components/Modals.tsx (1)
604-612: Consider adding a comment to explain the manager-specific backup modal.The new case appears to be a specialized version of the backup modal for manager context, with different prop values. Adding a brief comment would help explain why this variation exists and how it differs from the regular 'load-backup' case.
+ // Manager-specific backup modal with disabled backup creation and update watching case 'manager-load-backup': return ( <LoadBackupModal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/desktop-client/src/components/App.tsx(4 hunks)packages/desktop-client/src/components/FinancesApp.tsx(4 hunks)packages/desktop-client/src/components/LoggedInUser.tsx(4 hunks)packages/desktop-client/src/components/ManageRules.tsx(1 hunks)packages/desktop-client/src/components/Modals.tsx(4 hunks)
🔇 Additional comments (9)
packages/desktop-client/src/components/LoggedInUser.tsx (1)
218-222: ** Wrap 'logged as:' with translation function**
As noted in previous reviews, the text needs to be wrapped with translation function.
packages/desktop-client/src/components/App.tsx (3)
12-12: LGTM: Imports are properly structured.
The new imports are correctly added to support the user session management functionality.
Also applies to: 23-23, 30-30
55-56: LGTM: Clean Redux state and actions setup.
The implementation follows Redux best practices with proper typing and hooks usage.
131-165: 🛠️ Refactor suggestion
Remove duplicate useEffect hooks and optimize dependencies.
This issue was previously identified. Additionally:
- The dependency array
[userData, userData?.tokenExpired]is redundant sinceuserData?.tokenExpiredis derived fromuserData. Using just[userData]would be sufficient.
Apply this optimized version:
- useEffect(() => {
- if (userData?.tokenExpired) {
- addNotification({
- type: 'error',
- id: 'login-expired',
- title: 'Login expired',
- sticky: true,
- message: 'Login expired, please login again.',
- button: {
- title: 'Go to login',
- action: () => {
- signOut();
- },
- },
- });
- }
- }, [userData, userData?.tokenExpired]);
-
- useEffect(() => {
- if (userData?.tokenExpired) {
- addNotification({
- type: 'error',
- id: 'login-expired',
- title: 'Login expired',
- sticky: true,
- message: 'Login expired, please login again.',
- button: {
- title: 'Go to login',
- action: () => {
- signOut();
- },
- },
- });
- }
- }, [userData, userData?.tokenExpired]);
+ useEffect(() => {
+ if (userData?.tokenExpired) {
+ addNotification({
+ type: 'error',
+ id: 'login-expired',
+ title: 'Login expired',
+ sticky: true,
+ message: 'Login expired, please login again.',
+ button: {
+ title: 'Go to login',
+ action: () => {
+ signOut();
+ },
+ },
+ });
+ }
+ }, [userData]);packages/desktop-client/src/components/ManageRules.tsx (2)
Line range hint 1-300: Implementation looks solid and well-structured.
The component demonstrates good practices:
- Proper state management with React hooks
- Clear separation of concerns
- Comprehensive error handling
- Well-implemented pagination with loadMore
31-31: Verify the SimpleTable component move.
The SimpleTable component has been moved from rules/ to common/, suggesting it's been generalized for reuse.
Let's verify the component move and its impact:
✅ Verification successful
Let me verify if the SimpleTable component exists in the common directory and check its implementation to ensure it's properly moved.
SimpleTable component move is properly handled
The git history and file search results confirm that:
- The SimpleTable component was intentionally moved from
rules/tocommon/in commit bd66917 ("change location of component") - All imports have been properly updated to use the new path in
common/ - The old location in
rules/no longer exists - The component is now being reused across different features (UserAccess, UserDirectory, ManageRules)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of SimpleTable to ensure they've been updated
# and verify the component has been properly moved
# Check if the old location still exists
echo "Checking if old SimpleTable location exists:"
fd --type f "SimpleTable" packages/desktop-client/src/components/rules
# Find all imports of SimpleTable
echo -e "\nChecking all SimpleTable imports:"
rg "import.*SimpleTable.*from" packages/desktop-client/src/components
Length of output: 691
Script:
#!/bin/bash
# Check if SimpleTable exists in the common directory
echo "Checking SimpleTable in common directory:"
fd --type f "SimpleTable" packages/desktop-client/src/components/common
# Look at the git history to verify the move
echo -e "\nChecking git history for SimpleTable moves:"
git log --follow --patch --all -- "*SimpleTable*"
Length of output: 13732
packages/desktop-client/src/components/Modals.tsx (2)
30-30: LGTM: Import statements are well-organized.
The new imports for user management components follow the established naming conventions and are properly organized with other modal imports.
Also applies to: 33-33, 57-57, 59-59, 70-70
616-642: LGTM: User management modal cases are well-implemented.
The implementation aligns with the PR objectives for OpenID integration and follows consistent patterns for modal rendering.
Let's verify the prop types for the new components:
✅ Verification successful
Based on the search results, I can now verify the prop types for all the components mentioned in the review comment. The search results show:
- EditUserAccess component accepts props:
type EditUserAccessProps = {
defaultUserAccess: UserAccessEntity;
onSave?: (userAccess: UserAccessEntity) => void;
};- EditUserFinanceApp component accepts props:
type EditUserFinanceAppProps = {
defaultUser: User;
onSave: (user: User) => void;
};- TransferOwnership component accepts props:
type TransferOwnershipProps = {
onSave?: () => void;
};- OpenIDEnableModal component accepts props:
type OpenIDEnableModalProps = {
onSave?: () => void;
};- PasswordEnableModal component accepts props:
type PasswordEnableModalProps = {
onSave?: () => void;
};Props are properly typed and match the usage in the modal cases
All components have properly typed props that match how they are being used in the modal cases. The props passed in the switch statement (options.access, options.user, options.onSave) align with the prop type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify prop types for new user management components
# Expected: Find prop type definitions in component files
# Check EditUserAccess props
ast-grep --pattern 'interface $name {
defaultUserAccess: $_
onSave: $_
}'
# Check EditUserFinanceApp props
ast-grep --pattern 'interface $name {
defaultUser: $_
onSave: $_
}'
# Check TransferOwnership props
ast-grep --pattern 'interface $name {
onSave: $_
}'
# Check auth modal props
ast-grep --pattern 'interface $name {
onSave: $_
}'
Length of output: 265
Script:
#!/bin/bash
# Let's try a different approach to find the component files and their prop types
# First, let's locate the component files
fd "EditUserAccess|EditUserFinanceApp|TransferOwnership|OpenIDEnableModal|PasswordEnableModal" .tsx
# Then let's examine their content with proper context
rg -A 5 "interface.*Props" --type typescript
# Also check for type definitions using 'type' keyword
rg -A 5 "type.*Props.*=" --type typescript
# Look for prop type definitions in a different pattern
ast-grep --pattern 'export $_ function $name({ $props }: { $props_type }) {'
Length of output: 434
Script:
#!/bin/bash
# Let's try one more time with corrected commands
# Find the component files
fd "EditUserAccess|EditUserFinanceApp|TransferOwnership|OpenIDEnableModal|PasswordEnableModal"
# Look for prop definitions with ripgrep
rg -A 5 "interface.*Props"
rg -A 5 "type.*Props.*="
# Check component definitions
rg -A 3 "function (EditUserAccess|EditUserFinanceApp|TransferOwnership|OpenIDEnableModal|PasswordEnableModal)"
# Look for Props usage in the components
ast-grep --pattern 'function $name(props: $type) {
$$$
}'
Length of output: 203916
packages/desktop-client/src/components/FinancesApp.tsx (1)
283-305: Verify that ProtectedRoute correctly handles permission and validateOwner props for new routes
The new routes /user-directory and /user-access are conditionally rendered based on multiuserEnabled and use ProtectedRoute with permission={Permissions.ADMINISTRATOR}. The route for /user-access also includes validateOwner={true}.
Please ensure that:
- The
ProtectedRoutecomponent accepts and correctly handles both thepermissionandvalidateOwnerprops. - The authentication and authorization logic within
ProtectedRouteproperly enforces administrator permissions and owner validation as expected.
Run the following script to check if ProtectedRoute accepts the permission and validateOwner props:
✅ Verification successful
ProtectedRoute component correctly handles both permission and validateOwner props
The implementation shows that:
- The component properly accepts both
permissionand optionalvalidateOwnerprops as defined inProtectedRouteProps - It correctly enforces administrator permissions through
hasPermission(permission) - When
validateOwneris true, it additionally verifies user ownership by checkingusersWithAccessagainst the current user's ID - The component appropriately renders either the protected content or an access denied message based on these checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `ProtectedRoute` component accepts `permission` and `validateOwner` props.
# Test: Search for the component definition of `ProtectedRoute` to check for `permission` and `validateOwner` in its props.
ast-grep --pattern $'function ProtectedRoute({ $$$ permission, validateOwner, $$$ }) {\n $$$\n}'
Length of output: 96
Script:
#!/bin/bash
# Let's try different approaches to locate and analyze the ProtectedRoute component
# Search for files containing ProtectedRoute
echo "=== Files containing ProtectedRoute ==="
rg -l "ProtectedRoute"
# Search for the component definition with more flexible pattern
echo -e "\n=== ProtectedRoute component definition ==="
rg -A 10 "(?:export function|export const|function|const) ProtectedRoute"
# Search for type definitions that might define props
echo -e "\n=== ProtectedRoute type/interface definitions ==="
rg "type.*ProtectedRoute|interface.*ProtectedRoute"
Length of output: 1743
Script:
#!/bin/bash
# Let's examine the complete ProtectedRoute component and its type definition
# Get the full content of ProtectedRouteProps type
echo "=== ProtectedRouteProps type definition ==="
rg -A 5 "type ProtectedRouteProps = {" packages/desktop-client/src/auth/ProtectedRoute.tsx
# Get the complete implementation of ProtectedRoute component
echo -e "\n=== Complete ProtectedRoute implementation ==="
cat packages/desktop-client/src/auth/ProtectedRoute.tsx
Length of output: 2265
| useEffect(() => { | ||
| if (cloudFileId && currentFile) { | ||
| setIsOwner( | ||
| currentFile.usersWithAccess.some( | ||
| u => u.userId === userData?.userId && u.owner, | ||
| ), | ||
| ); | ||
| } else { | ||
| setIsOwner(false); | ||
| } | ||
| }, [cloudFileId]); |
There was a problem hiding this comment.
Add missing dependencies to useEffect
The effect is missing currentFile and userData in its dependency array, which could lead to stale closures.
useEffect(() => {
if (cloudFileId && currentFile) {
setIsOwner(
currentFile.usersWithAccess.some(
u => u.userId === userData?.userId && u.owner,
),
);
} else {
setIsOwner(false);
}
- }, [cloudFileId]);
+ }, [cloudFileId, currentFile, userData?.userId]);📝 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.
| useEffect(() => { | |
| if (cloudFileId && currentFile) { | |
| setIsOwner( | |
| currentFile.usersWithAccess.some( | |
| u => u.userId === userData?.userId && u.owner, | |
| ), | |
| ); | |
| } else { | |
| setIsOwner(false); | |
| } | |
| }, [cloudFileId]); | |
| useEffect(() => { | |
| if (cloudFileId && currentFile) { | |
| setIsOwner( | |
| currentFile.usersWithAccess.some( | |
| u => u.userId === userData?.userId && u.owner, | |
| ), | |
| ); | |
| } else { | |
| setIsOwner(false); | |
| } | |
| }, [cloudFileId, currentFile, userData?.userId]); |
| const handleMenuSelect = async (type: string) => { | ||
| setMenuOpen(false); | ||
|
|
||
| switch (type) { | ||
| case 'change-password': | ||
| onChangePassword(); | ||
| await closeBudget(); | ||
| if (window.__navigate) { | ||
| window.__navigate('/change-password'); | ||
| } else { | ||
| window.location.href = '/change-password'; | ||
| } | ||
| break; | ||
| case 'sign-in': | ||
| await closeBudget(); | ||
| window.__navigate('/login'); | ||
| if (window.__navigate) { | ||
| window.__navigate('/login'); | ||
| } else { | ||
| window.location.href = '/login'; | ||
| } | ||
| break; | ||
| case 'user-access': | ||
| navigate('/user-access'); | ||
| break; | ||
| case 'user-directory': | ||
| navigate('/user-directory'); | ||
| break; | ||
| case 'index': | ||
| navigate('/'); | ||
| break; | ||
| case 'sign-out': | ||
| signOut(); | ||
| if (signOut) { | ||
| signOut(); | ||
| } | ||
| break; | ||
| case 'config-server': | ||
| await closeBudget(); | ||
| window.__navigate('/config-server'); | ||
| if (window.__navigate) { | ||
| window.__navigate('/config-server'); | ||
| } else { | ||
| window.location.href = '/config-server'; | ||
| } | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor navigation logic to reduce duplication
The navigation logic using window.__navigate is repeated multiple times. Consider extracting it into a helper function.
+ const navigateTo = (path: string) => {
+ if (window.__navigate) {
+ window.__navigate(path);
+ } else {
+ window.location.href = path;
+ }
+ };
const handleMenuSelect = async (type: string) => {
setMenuOpen(false);
switch (type) {
case 'change-password':
await closeBudget();
- if (window.__navigate) {
- window.__navigate('/change-password');
- } else {
- window.location.href = '/change-password';
- }
+ navigateTo('/change-password');
break;
// Apply similar changes to other casesCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (22)
packages/desktop-client/src/auth/types.ts (1)
1-3: Consider planning for more granular permissions.While the current
ADMINISTRATORpermission is sufficient for basic access control, consider planning for more granular permissions to support different user roles and access levels in the future (e.g.,READ_ONLY,MANAGE_USERS,MANAGE_BUDGETS).Example expansion:
export enum Permissions { ADMINISTRATOR = 'ADMIN', MANAGE_USERS = 'MANAGE_USERS', MANAGE_BUDGETS = 'MANAGE_BUDGETS', READ_ONLY = 'READ_ONLY' }packages/loot-core/src/types/models/user.ts (3)
8-11: Address type inconsistency and add documentationThe
ownerfield is boolean here but numeric inUserAvailable. This inconsistency could lead to confusion. Also, consider adding JSDoc comments to document the purpose of these fields.+/** + * Represents a user entity with complete information including ownership status. + * @property id - Unique identifier for the user + * @property owner - Indicates whether this user is the owner of the system/resource + */ export interface UserEntity extends NewUserEntity { id: string; owner: boolean; }
13-17: Maintain consistent property naming across interfacesConsider using consistent property naming between interfaces.
UserEntityusesidwhile this interface usesuserId.+/** + * Represents user data optimized for dropdown UI components. + */ export interface UserEntityDropdown { - userId: string; + id: string; userName: string; displayName?: string; }
27-30: Enhance type safety for roles definitionConsider using TypeScript's
as constassertion or an enum to make the roles type-safe and immutable. Also, add documentation about role capabilities.+/** + * Defines possible user roles and their corresponding display values. + * - Admin: Full system access with user management capabilities + * - Basic: Standard user access without administrative privileges + */ -export const PossibleRoles = { +export const PossibleRoles = { ADMIN: 'Admin', BASIC: 'Basic', -}; +} as const;packages/desktop-client/src/auth/AuthProvider.tsx (1)
8-10: Consider enhancing the permission system for future scalability.The current implementation uses a simple single-permission model. Consider these architectural improvements for future scalability:
- Support multiple permissions per user (e.g.,
hasAnyPermission,hasAllPermissions)- Implement role-based access control (RBAC)
- Add permission inheritance/hierarchy
Example enhancement:
type AuthContextType = { hasPermission: (permission: Permissions | Permissions[]) => boolean; hasRole: (role: Roles) => boolean; // Add more granular permission checks hasAllPermissions: (permissions: Permissions[]) => boolean; hasAnyPermission: (permissions: Permissions[]) => boolean; };packages/loot-core/src/server/admin/types/handlers.ts (2)
8-24: Consider standardizing error handling patterns.The user management methods use different error handling patterns:
users-getreturnsnullfor errorsuser-add/updateuse{error?: string}
Consider adopting a consistent error handling pattern across all methods for better maintainability.
26-28: Improve return type definition for access-add.Using
Record<string, never>for an empty object is verbose. Consider using a more explicit success type.- ) => Promise<{ error?: string } | Record<string, never>>; + ) => Promise<{ error?: string } | { success: true }>;packages/desktop-client/src/components/manager/subscribe/common.tsx (2)
Line range hint
33-91: Add missing dependencies to useEffectThe useEffect hook uses
setServerURL,setMultiuserEnabled, andsetLoginMethods, but these aren't included in the dependencies array. This could lead to stale closures.- }, [location]); + }, [location, setServerURL, setMultiuserEnabled, setLoginMethods, navigate, redirect]);
Line range hint
56-60: Consider enhancing error handling for better UXThe current error handling only logs the error and redirects to the config server. Consider:
- Adding specific error messages for different failure scenarios (network issues, invalid OpenID configuration, etc.)
- Implementing a retry mechanism for transient failures
- Adding error boundaries to gracefully handle rendering failures
This would improve the user experience when setting up OpenID authentication.
packages/loot-core/src/server/admin/app.ts (2)
14-107: Extract common token validation and request handling logic.All user management methods share similar patterns for token validation and request handling. Consider creating a higher-order function to reduce code duplication:
type AuthenticatedRequestResult<T> = { success: boolean; error?: string; data?: T; }; async function withAuthenticatedRequest<T>( request: (token: string) => Promise<T> ): Promise<AuthenticatedRequestResult<T>> { const userToken = await asyncStorage.getItem('user-token'); if (!userToken) { return { success: false, error: 'No user token found' }; } try { const result = await request(userToken); return { success: true, data: result }; } catch (err) { return { success: false, error: err.reason }; } } // Usage example: app.method('user-add', async function(user) { return withAuthenticatedRequest(token => post(getServer().BASE_SERVER + '/admin/users/', user, { 'X-ACTUAL-TOKEN': token, }) ); });
109-190: Improve URL management for API endpoints.The API endpoints are constructed using string concatenation, which is error-prone and hard to maintain. Consider creating a centralized endpoint configuration:
const API_ENDPOINTS = { access: { base: '/admin/access', get: (fileId: string) => `${API_ENDPOINTS.access.base}?fileId=${fileId}`, bulk: `${API_ENDPOINTS.access.base}/get-bulk`, users: (fileId: string) => `${API_ENDPOINTS.access.base}/users?fileId=${fileId}`, }, // ... other endpoint configurations } as const; // Usage example: app.method('access-get', async function(fileId) { return withAuthenticatedRequest(token => get( getServer().BASE_SERVER + API_ENDPOINTS.access.get(fileId), { headers: { 'X-ACTUAL-TOKEN': token }, } ) ); });packages/desktop-client/src/components/modals/EditUser.tsx (2)
37-56: Consider improving error messages for better user guidanceWhile the error messages are generally good, some could be more helpful:
- For 'user-cant-be-empty', consider adding length requirements if any
- For 'role-cant-be-empty', consider explaining available roles
- For default case, consider adding steps to report the issue
case 'user-cant-be-empty': - return 'Please enter a value for the username; the field cannot be empty.'; + return 'Please enter a username (minimum 3 characters required).'; case 'role-cant-be-empty': - return 'Select a role; the field cannot be empty.'; + return 'Please select a role (Basic or Admin) to set user permissions.'; default: - return `An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: ${reason})`; + return `An internal error occurred (ref: ${reason}). Please: \n1. Try refreshing the page\n2. If the error persists, visit https://actualbudget.org/contact/ with the reference number.`;
58-91: Consider enhancing error handling and success case clarityThe function handles errors well but could be improved:
- Consider adding input validation before the API call
- Make the success case more explicit with a type
+type SaveUserResult = { + success: boolean; + userId?: string; + error?: string; +}; async function saveUser( method: 'user-add' | 'user-update', user: User, setError: (error: string) => void, actions: BoundActions, -): Promise<boolean> { +): Promise<SaveUserResult> { + // Validate input + if (!user.userName?.trim()) { + setError(getUserDirectoryErrors('user-cant-be-empty')); + return { success: false, error: 'user-cant-be-empty' }; + } const { error, id: newId } = (await send(method, user)) || {}; if (!error) { if (newId) { user.id = newId; } + return { success: true, userId: newId }; } else { setError(getUserDirectoryErrors(error)); if (error === 'token-expired') { // ... notification code ... } - return false; + return { success: false, error }; } - return true; }packages/loot-core/src/server/cloud-storage.ts (1)
25-30: Add documentation and clarify owner fieldsThe new interfaces look well-structured, but could benefit from some improvements:
- Add JSDoc comments explaining the purpose of each interface and its properties
- Clarify the difference between
owner: booleaninUsersWithAccessandowner: stringinRemoteFile+/** + * Represents a user who has access to a file + */ export interface UsersWithAccess { userId: string; userName: string; displayName: string; + /** Indicates if this user is the owner of the file */ owner: boolean; } +/** + * Represents a file stored in the cloud + */ export interface RemoteFile { // ... existing properties ... + /** The userId of the file owner */ owner: string; + /** List of users who have access to this file */ usersWithAccess: UsersWithAccess[]; }Also applies to: 38-39
packages/desktop-client/src/components/ServerContext.tsx (1)
13-17: Rename 'LoginMethods' type to 'LoginMethod' for consistencyThe type
LoginMethodsrepresents a single login method. Renaming it toLoginMethodwould improve readability and follow standard naming conventions. Update all occurrences accordingly.Apply this diff to rename
LoginMethodstoLoginMethod:-type LoginMethods = { +type LoginMethod = { method: string; displayName: string; active: boolean; }; type ServerContextValue = { url: string | null; version: string; multiuserEnabled: boolean; - availableLoginMethods: LoginMethods[]; + availableLoginMethods: LoginMethod[]; setURL: ( url: string, opts?: { validate?: boolean }, ) => Promise<{ error?: string }>; refreshLoginMethods: () => Promise<void>; setMultiuserEnabled: (enabled: boolean) => void; - setLoginMethods: (methods: LoginMethods[]) => void; + setLoginMethods: (methods: LoginMethod[]) => void; }; const ServerContext = createContext<ServerContextValue>({ url: null, version: '', multiuserEnabled: false, - availableLoginMethods: [], + availableLoginMethods: [], setURL: () => Promise.reject(new Error('ServerContext not initialized')), refreshLoginMethods: () => Promise.reject(new Error('ServerContext not initialized')), setMultiuserEnabled: () => {}, setLoginMethods: () => {}, }); export const useMultiuserEnabled = () => { const { multiuserEnabled } = useContext(ServerContext); const loginMethod = useLoginMethod(); return multiuserEnabled && loginMethod === 'openid'; }; export const useLoginMethod = () => { - const availableLoginMethods = useContext(ServerContext).availableLoginMethods; + const availableLoginMethods = useContext(ServerContext).availableLoginMethods; export const useAvailableLoginMethods = () => useContext(ServerContext).availableLoginMethods; export const useRefreshLoginMethods = () => useContext(ServerContext).refreshLoginMethods; export const useSetMultiuserEnabled = () => useContext(ServerContext).setMultiuserEnabled; export const useSetLoginMethods = () => useContext(ServerContext).setLoginMethods; const [availableLoginMethods, setAvailableLoginMethods] = useState< - LoginMethods[] + LoginMethod[] >([]); return ( <ServerContext.Provider value={{ url: serverURL, multiuserEnabled, availableLoginMethods, setURL, version: version ? `v${version}` : 'N/A', refreshLoginMethods, setMultiuserEnabled, setLoginMethods: setAvailableLoginMethods, }} > {children} </ServerContext.Provider> );Also applies to: 22-23, 28-30, 36-37, 49-49, 55-55, 64-64, 74-75, 77-78, 80-81, 88-89, 135-136
packages/desktop-client/src/components/manager/subscribe/Login.tsx (4)
26-70: UseuseDispatchinsidePasswordLoginfor better encapsulationInstead of passing
dispatchas a prop to thePasswordLogincomponent, you can use theuseDispatchhook directly within the component. This enhances encapsulation and adheres to React best practices.Apply this diff to implement the change:
+ import { useDispatch } from 'react-redux'; function PasswordLogin({ setError, dispatch }) { + const dispatch = useDispatch(); const [password, setPassword] = useState(''); const [loading, setLoading] = useState(false); async function onSubmitPassword() { // ... } return ( // ... ); } - <PasswordLogin setError={setError} dispatch={dispatch} /> + <PasswordLogin setError={setError} />
90-91: Add error handling forsend('owner-created')callIn the
OpenIdLogincomponent, theuseEffecthook doesn't handle potential errors from thesend('owner-created')call. Adding error handling will prevent unhandled promise rejections and improve robustness.Apply this diff to handle errors:
useEffect(() => { - send('owner-created').then(created => setWarnMasterCreation(!created)); + send('owner-created') + .then(created => setWarnMasterCreation(!created)) + .catch(err => { + // Handle the error appropriately + console.error('Error fetching owner-created status:', err); + }); }, []);
183-187: Use consistent components instead of HTML elementsIn the
HeaderLogincomponent, consider using<Text>and<View>instead of<span>. This promotes consistency with the React components used throughout the application.Apply this diff to adjust the components:
- <span> + <View style={{ flexDirection: 'row', alignItems: 'center' }}> Checking Header Token Login...{' '} <AnimatedLoading style={{ width: 20, height: 20 }} /> - </span> + </View>
Based on the code analysis, I can now generate the final response:
The
getErrorMessagefunction needs to handle additional error codesThe current implementation is missing some error codes that can be returned by the subscribe-sign-in handler:
network-failure: Already handledunauthorized: Missingnot-logged-in: MissingThe error codes are returned in these patterns:
{ error: err.reason || 'network-failure' }from network errors{ error: 'unauthorized' }from various auth checks in main.ts{ error: 'not-logged-in' }from auth state checks🔗 Analysis chain
Line range hint
223-236: Ensure all possible error messages are handled ingetErrorMessageVerify that the
getErrorMessagefunction covers all error codes returned by thesubscribe-sign-inAPI to provide accurate feedback to users.Run the following script to list all unique error codes returned by
subscribe-sign-inhandlers:This will help identify any error codes not currently handled in
getErrorMessage.
These scripts will help us:
- Locate the file containing the subscribe-sign-in handler
- Find error patterns in authentication flows
- Look for handler registrations in the codebase
These scripts will help us:
- Find the actual implementation of the subscribe-sign-in handler
- Check the type definitions for possible error codes in the response type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List all unique error codes from 'subscribe-sign-in' handlers. # Search for error codes in 'subscribe-sign-in' handlers. rg "handlers\['subscribe-sign-in'\]" -A 10 packages/loot-core/src/ | grep -Eo "'error': '[^']+'" | sort | uniqLength of output: 133
Script:
#!/bin/bash # Let's try a broader search to find the subscribe-sign-in handler implementation rg -l "subscribe-sign-in" packages/loot-core/src/Length of output: 137
Script:
#!/bin/bash # Search for error related code in authentication/sign-in flows rg "'error':|throw new Error|return \{.*error:" -A 2 packages/loot-core/src/ | grep -v "test"Length of output: 40745
Script:
#!/bin/bash # Look for the actual handler registration ast-grep --pattern 'handlers[$_] = async function($$$) { $$ }'Length of output: 64
Script:
#!/bin/bash # Let's check the server handler implementation in main.ts cat packages/loot-core/src/server/main.ts | grep -A 20 "subscribe-sign-in"Length of output: 1118
Script:
#!/bin/bash # Let's also check the type definition for possible error codes cat packages/loot-core/src/types/server-handlers.d.ts | grep -A 10 "subscribe-sign-in"Length of output: 305
packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx (1)
165-165: Correct the extra period in the displayed textThere's an extra period at the end of the sentence in the
Textcomponent. The sentence should end with a single period.Apply this diff to fix the punctuation:
- Determine which users can view and manage your budgets..{' '} + Determine which users can view and manage your budgets.{` `}packages/loot-core/src/types/server-handlers.d.ts (2)
293-296: Enforce mutual exclusivity betweenpasswordandopenIdinsubscribe-bootstrapCurrently, both
passwordandopenIdare optional parameters. To prevent ambiguous usage, consider enforcing that eitherpasswordoropenIdmust be provided, but not both.You can achieve this by using a union type:
'subscribe-bootstrap': ( arg: | { password: string } | { openId: OpenIdConfig } ) => Promise<{ error?: string }>;This ensures clear intent when calling the method and enhances type safety.
398-401: Consider makingopenIda required parameter inenable-openidThe
openIdparameter is currently optional, which might lead to confusion if the method is called without it. Consider makingopenIda required parameter to ensure proper configuration.Update the method signature:
'enable-openid': (arg: { openId: OpenIdConfig; }) => Promise<{ error?: string }>;This enforces that necessary configuration is provided when enabling OpenID.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/desktop-client/src/auth/AuthProvider.tsx(1 hunks)packages/desktop-client/src/auth/types.ts(1 hunks)packages/desktop-client/src/components/ServerContext.tsx(4 hunks)packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(4 hunks)packages/desktop-client/src/components/manager/subscribe/common.tsx(4 hunks)packages/desktop-client/src/components/modals/EditUser.tsx(1 hunks)packages/loot-core/src/server/admin/app.ts(1 hunks)packages/loot-core/src/server/admin/types/handlers.ts(1 hunks)packages/loot-core/src/server/cloud-storage.ts(2 hunks)packages/loot-core/src/server/main.ts(11 hunks)packages/loot-core/src/types/models/user.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(4 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx
[error] 104-104: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🔇 Additional comments (24)
packages/desktop-client/src/auth/types.ts (1)
1-3: LGTM! Clean and well-structured enum implementation.
The Permissions enum is correctly exported and follows TypeScript naming conventions.
packages/desktop-client/src/auth/AuthProvider.tsx (3)
1-16: LGTM! Well-structured type definitions and imports.
The type definitions are clear, properly typed, and follow TypeScript best practices.
39-45: LGTM! Well-implemented custom hook.
The useAuth hook follows React best practices with proper error handling and type safety.
21-30:
Consider security implications of offline user permissions.
The current implementation allows offline users to bypass all permission checks. This could potentially lead to security issues if the offline mode is unintentionally activated or exploited.
Additionally, consider these type-safety improvements:
- userData?.permission?.toUpperCase() === permission?.toUpperCase()
+ Object.values(Permissions).includes(userData?.permission as Permissions) &&
+ userData.permission === permissionThis change would:
- Validate that the user's permission is a valid enum value
- Remove unnecessary case-insensitive comparison since we're using an enum
packages/loot-core/src/server/admin/types/handlers.ts (3)
1-6: LGTM! Clean and well-organized imports.
The imports are properly structured and use specific type imports rather than importing entire modules.
7-53: Verify completeness of multi-user support methods.
Given this is part of OpenID implementation, consider if additional methods are needed for:
- User session management
- Role-based access control
- Group management
#!/bin/bash
# Check for existing session/role management
rg -g "*.ts" -A 5 "session|role|permission" packages/loot-core/src/42-49: Improve transfer-ownership return type and add verification.
- Consider using a consistent success response type.
- This is a sensitive operation that should be verified for proper access control.
✅ Verification successful
Let me try to find the implementation of the transfer-ownership handler with a different search strategy.
Based on the implementation found, I can now generate the final response:
Access control is properly implemented, but return type can be improved
The transfer-ownership operation already includes proper access control:
- Requires a valid user token for authentication
- Makes request to a protected admin endpoint
- Handles and returns appropriate error messages for unauthorized access
However, the return type can be more specific:
- Current:
Promise<{ error?: string } | Record<string, never>> - Consider:
Promise<{ success: true } | { error: string }>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for access control verification in transfer-ownership implementation
rg -A 10 "transfer-ownership.*implementation" --type ts
Length of output: 2199
Script:
#!/bin/bash
# Search for transfer-ownership implementation using ast-grep
ast-grep --pattern 'transfer-ownership: async function($$$) { $$$ }'
# Also try searching with ripgrep for any mentions of transfer-ownership
rg -A 10 "transfer-ownership" --type ts
Length of output: 7315
packages/desktop-client/src/components/manager/subscribe/common.tsx (3)
6-6: LGTM: Clean import organization with proper type definitions
The new imports properly support the multiuser and login method management requirements while maintaining type safety.
Also applies to: 10-14
50-52: LGTM: Improved type safety with proper response typing
The addition of proper type annotations for the API responses enhances type safety and maintainability.
Also applies to: 73-75
Line range hint 95-114: LGTM: Well-structured Title component
The Title component is properly typed and follows React best practices with proper styling integration.
packages/loot-core/src/server/admin/app.ts (1)
233-241: Consider authentication requirement for owner-created.
Unlike other methods in this file, owner-created doesn't require authentication. Consider whether this is intentional and document the reason if it is.
✅ Verification successful
Let me search for more context about this endpoint and its usage.
Let me search for more context about the login flow and authentication patterns.
From the Login component implementation, I can now understand the context better. Let me check one more thing about the server setup.
Based on the context gathered, I can now provide the final response:
The owner-created endpoint's lack of authentication is intentional and correct
The owner-created endpoint is specifically used during the initial login flow to determine if a server owner exists, which affects the UI warning message about the first OpenID login becoming the server owner. This check must occur before authentication since it's part of the login process itself.
All other admin endpoints in the codebase consistently require authentication via the user-token, as evidenced by the code patterns:
file-owner-getrequiresX-ACTUAL-TOKENusers-getrequiresX-ACTUAL-TOKENaccessrelated endpoints requireX-ACTUAL-TOKEN
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if owner-created endpoint is protected in the server implementation
rg -A 5 "'/admin/owner-created/'"
Length of output: 433
Script:
#!/bin/bash
# Search for the server-side implementation of the owner-created endpoint
ast-grep --pattern 'app.$_("/admin/owner-created/"'
# Search for any references to owner-created in the codebase
rg -B 3 -A 3 "owner-created"
# Look for authentication middleware or route protection patterns
ast-grep --pattern 'app.use(auth'
Length of output: 1667
Script:
#!/bin/bash
# Look for the Login component implementation to understand the context
cat "packages/desktop-client/src/components/manager/subscribe/Login.tsx"
# Search for authentication middleware patterns in different formats
rg -B 3 -A 3 "middleware.*auth"
rg -B 3 -A 3 "requireAuth"
rg -B 3 -A 3 "isAuthenticated"
Length of output: 8411
Script:
#!/bin/bash
# Look for the server setup and route definitions
rg -B 5 -A 5 "BASE_SERVER.*admin"
rg -B 5 -A 5 "'/admin/.*owner'"
# Check for any auth middleware or protected routes
ast-grep --pattern 'router.use($$$)'
ast-grep --pattern 'app.use($$$)'
Length of output: 10248
packages/desktop-client/src/components/modals/EditUser.tsx (1)
1-35: LGTM! Clean type definitions and imports structure
The type definitions and imports are well-organized, with clear interfaces for component props.
packages/loot-core/src/server/cloud-storage.ts (2)
47-52: Fix unreachable code and improve error handling
The previous review comment about improving error handling for 403 status is still valid. The issues with unreachable code, window.location.href usage, and JSON parsing remain unaddressed.
Line range hint 1-423: Verify security-sensitive operations
Let's verify the handling of sensitive operations across the codebase:
- Token management
- Encryption key handling
- File access controls
✅ Verification successful
Based on the results, I can now provide a comprehensive security analysis of the codebase. Let me generate the final response:
Security implementation verified - token and encryption handling are properly managed
The security implementation across the codebase follows secure practices:
-
Token Management:
- User tokens are consistently handled via
asyncStorage - Tokens are properly validated before operations
- Token expiration is handled with proper cleanup (
removeItem('user-token'))
- User tokens are consistently handled via
-
Encryption:
- Encryption keys are managed through a dedicated encryption module
- Key validation is performed before operations (
hasKeychecks) - Proper error handling for encryption failures
-
Access Controls:
- File ownership is properly tracked
- User access is verified through
usersWithAccesschecks - Owner-specific operations are properly gated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential security issues in token and key management
# Look for token usage patterns
echo "Checking token handling..."
rg -A 2 "user-token" --type ts
# Check encryption key management
echo "Checking encryption key handling..."
rg -A 2 "encryptKeyId|hasKey" --type ts
# Look for file access control patterns
echo "Checking file access controls..."
rg -A 2 "usersWithAccess|owner.*string" --type ts
Length of output: 24177
packages/desktop-client/src/components/ServerContext.tsx (1)
61-61: Verify defaulting to 'password' login method
In useLoginMethod, if no login methods are active, the default is set to 'password'. Confirm that this fallback is appropriate, especially if the 'password' method is not intended to be available. This could lead to authentication issues if 'password' login is disabled on the server.
packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx (1)
142-144:
Include setHoveredUserAccess in onHover dependencies
The useCallback hook for onHover uses setHoveredUserAccess but has an empty dependency array. For consistency and to prevent stale references, include setHoveredUserAccess in the dependencies.
Apply this diff to include setHoveredUserAccess:
const onHover = useCallback(id => {
setHoveredUserAccess(id);
- }, []);
+ }, [setHoveredUserAccess]);Likely invalid or redundant comment.
packages/loot-core/src/types/server-handlers.d.ts (7)
20-20: Import statement for OpenIdConfig is correct
The import statement correctly brings in OpenIdConfig from './models/openid'.
288-291: subscribe-get-login-methods method definition looks good
The method is well-defined, and the return types for methods and error are appropriate.
298-306: subscribe-get-user method definition is appropriate
The method signature accurately reflects the possible return values and optional fields.
326-327: subscribe-set-token method is well-defined
The method signature is clear, and types are appropriately specified.
346-347: get-user-file-info method definition looks appropriate
The method correctly defines the input parameter and return type.
402-403: enable-password method is properly defined
The method signature clearly specifies the required password parameter and return type.
404-406: get-openid-config method definition is acceptable
The method correctly returns the OpenID configuration when available.
packages/loot-core/src/server/main.ts (1)
881-881:
Avoid exposing internal error details to the client
Returning err.reason or error.reason directly to the client may expose sensitive internal information, which can be a security risk. Consider returning a generic error message or sanitizing the error details before sending them to the client to prevent potential information leakage.
Apply the following diff to handle errors securely:
881
- return { error: 'failed', reason: error.reason };
+ return { error: 'failed' };
1556
- return { error: err.reason || 'network-failure' };
+ return { error: 'network-failure' };
1663
- return { error: err.reason || 'network-failure' };
+ return { error: 'network-failure' };
2044
- return { error: err.reason || 'network-failure' };
+ return { error: 'network-failure' };
2061
- return { error: err.reason || 'network-failure' };
+ return { error: 'network-failure' };Also applies to: 1556-1556, 1663-1663, 2044-2044, 2061-2061
⛔ Skipped due to learnings
Learnt from: matt-fidd
PR: actualbudget/actual#3581
File: packages/loot-core/src/server/main.ts:1096-1102
Timestamp: 2024-10-25T00:17:33.196Z
Learning: In this codebase, including `err.stack` in error responses returned to clients is acceptable to maintain consistent behavior.
| export interface NewUserEntity { | ||
| userName: string; | ||
| displayName: string; | ||
| role: string; | ||
| enabled: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety for the role field
The role field should be constrained to valid values from PossibleRoles to prevent invalid role assignments.
export interface NewUserEntity {
userName: string;
displayName: string;
- role: string;
+ role: typeof PossibleRoles[keyof typeof PossibleRoles];
enabled: boolean;
}Committable suggestion skipped: line range outside the PR's diff.
| export interface UserAvailable { | ||
| userId: string; | ||
| displayName?: string; | ||
| userName: string; | ||
| haveAccess?: number; | ||
| owner?: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type definitions for boolean concepts
The interface uses numeric types (number) for what appear to be boolean concepts (haveAccess, owner). Consider using boolean types for clarity and add documentation explaining the distinction between these fields.
+/**
+ * Represents a user with their access status to a specific resource.
+ * @property haveAccess - Indicates if the user has access to the resource
+ * @property owner - Indicates if the user owns the resource
+ */
export interface UserAvailable {
userId: string;
displayName?: string;
userName: string;
- haveAccess?: number;
- owner?: number;
+ haveAccess?: boolean;
+ owner?: boolean;
}📝 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.
| export interface UserAvailable { | |
| userId: string; | |
| displayName?: string; | |
| userName: string; | |
| haveAccess?: number; | |
| owner?: number; | |
| } | |
| /** | |
| * Represents a user with their access status to a specific resource. | |
| * @property haveAccess - Indicates if the user has access to the resource | |
| * @property owner - Indicates if the user owns the resource | |
| */ | |
| export interface UserAvailable { | |
| userId: string; | |
| displayName?: string; | |
| userName: string; | |
| haveAccess?: boolean; | |
| owner?: boolean; | |
| } |
| 'user-delete-all': ( | ||
| ids: string[], | ||
| ) => Promise<{ someDeletionsFailed: boolean; ids?: number[] }>; |
There was a problem hiding this comment.
Fix type inconsistency in user-delete-all return type.
The method accepts string[] as input but returns number[] in the response. This could lead to type conversion issues.
- ) => Promise<{ someDeletionsFailed: boolean; ids?: number[] }>;
+ ) => Promise<{ someDeletionsFailed: boolean; ids?: string[] }>;📝 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.
| 'user-delete-all': ( | |
| ids: string[], | |
| ) => Promise<{ someDeletionsFailed: boolean; ids?: number[] }>; | |
| 'user-delete-all': ( | |
| ids: string[], | |
| ) => Promise<{ someDeletionsFailed: boolean; ids?: string[] }>; |
| 'access-delete-all': ({ | ||
| fileId, | ||
| ids, | ||
| }: { | ||
| fileId: string; | ||
| ids: string[]; | ||
| }) => Promise<{ someDeletionsFailed: boolean; ids?: number[] }>; |
There was a problem hiding this comment.
Fix type inconsistency in access-delete-all return type.
Similar to user-delete-all, this method has mismatched ID types between input and output.
- }) => Promise<{ someDeletionsFailed: boolean; ids?: number[] }>;
+ }) => Promise<{ someDeletionsFailed: boolean; ids?: string[] }>;📝 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.
| 'access-delete-all': ({ | |
| fileId, | |
| ids, | |
| }: { | |
| fileId: string; | |
| ids: string[]; | |
| }) => Promise<{ someDeletionsFailed: boolean; ids?: number[] }>; | |
| 'access-delete-all': ({ | |
| fileId, | |
| ids, | |
| }: { | |
| fileId: string; | |
| ids: string[]; | |
| }) => Promise<{ someDeletionsFailed: boolean; ids?: string[] }>; |
| // @ts-strict-ignore | ||
| import * as asyncStorage from '../../platform/server/asyncStorage'; | ||
| import { UserAvailable, UserEntity } from '../../types/models/user'; | ||
| import { UserAccessEntity } from '../../types/models/userAccess'; | ||
| import { createApp } from '../app'; | ||
| import { del, get, patch, post } from '../post'; | ||
| import { getServer } from '../server-config'; | ||
|
|
||
| import { AdminHandlers } from './types/handlers'; | ||
|
|
||
| // Expose functions to the client | ||
| export const app = createApp<AdminHandlers>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing @ts-strict-ignore and addressing type issues.
The use of @ts-strict-ignore suggests potential type safety issues. Consider addressing these issues to improve code reliability and maintainability.
| <Button | ||
| variant="bare" | ||
| aria-label="Menu" | ||
| onPress={() => | ||
| dispatch( | ||
| pushModal('transfer-ownership', { | ||
| onSave: async () => { | ||
| await loadAccess(); | ||
| setLoading(false); | ||
| }, | ||
| }), | ||
| ) | ||
| } | ||
| > | ||
| <LockToggle style={{ width: 16, height: 16 }} /> | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility by updating the aria-label
The Button triggering the transfer of ownership has an aria-label of "Menu", which may not accurately describe its purpose. For better accessibility, update the aria-label to "Transfer Ownership".
Apply this diff to update the aria-label:
<Button
variant="bare"
- aria-label="Menu"
+ aria-label="Transfer Ownership"
onPress={() =>
dispatch(
pushModal('transfer-ownership', {
onSave: async () => {
await loadAccess();
setLoading(false);
},
}),
)
}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| variant="bare" | |
| aria-label="Menu" | |
| onPress={() => | |
| dispatch( | |
| pushModal('transfer-ownership', { | |
| onSave: async () => { | |
| await loadAccess(); | |
| setLoading(false); | |
| }, | |
| }), | |
| ) | |
| } | |
| > | |
| <LockToggle style={{ width: 16, height: 16 }} /> | |
| </Button> | |
| <Button | |
| variant="bare" | |
| aria-label="Transfer Ownership" | |
| onPress={() => | |
| dispatch( | |
| pushModal('transfer-ownership', { | |
| onSave: async () => { | |
| await loadAccess(); | |
| setLoading(false); | |
| }, | |
| }), | |
| ) | |
| } | |
| > | |
| <LockToggle style={{ width: 16, height: 16 }} /> | |
| </Button> |
| const onSearchChange = useCallback( | ||
| (value: string) => { | ||
| setFilter(value); | ||
| setPage(0); | ||
| }, | ||
| [setFilter], | ||
| ); |
There was a problem hiding this comment.
Include setPage in onSearchChange dependencies
The useCallback hook for onSearchChange uses the setPage function but does not include it in the dependency array. To ensure the callback updates correctly when setPage changes, include it in the dependencies.
Apply this diff to include setPage in the dependencies:
[setFilter],
+ [setFilter, setPage],Committable suggestion skipped: line range outside the PR's diff.
| style={{ | ||
| paddingBlock: 15, | ||
| paddingInline: isModal ? 13 : 0, | ||
| borderTop: isModal && '1px solid ' + theme.pillBorder, |
There was a problem hiding this comment.
Avoid assigning 'false' as a style value
The borderTop style property is conditionally set using isModal && '1px solid ' + theme.pillBorder. If isModal is false, borderTop will be assigned false, which is not a valid CSS value. This may lead to styling issues.
Apply this diff to set borderTop to undefined when isModal is false:
- borderTop: isModal && '1px solid ' + theme.pillBorder,
+ borderTop: isModal ? '1px solid ' + theme.pillBorder : undefined,📝 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.
| borderTop: isModal && '1px solid ' + theme.pillBorder, | |
| borderTop: isModal ? '1px solid ' + theme.pillBorder : undefined, |
| 'subscribe-needs-bootstrap': (args: { url }) => Promise< | ||
| | { error: string } | ||
| | { | ||
| bootstrapped: boolean; | ||
| hasServer: false; | ||
| } | ||
| | { | ||
| bootstrapped: boolean; | ||
| hasServer: true; | ||
| loginMethods: { | ||
| method: string; | ||
| displayName: string; | ||
| active: boolean; | ||
| }[]; | ||
| multiuser: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define explicit types for the return value of subscribe-needs-bootstrap
The return type of subscribe-needs-bootstrap is a union of three different object types. To enhance readability and maintainability, consider defining explicit interfaces or type aliases for each return type.
Example:
interface NeedsBootstrapError {
error: string;
}
interface NeedsBootstrapNoServer {
bootstrapped: boolean;
hasServer: false;
}
interface NeedsBootstrapWithServer {
bootstrapped: boolean;
hasServer: true;
loginMethods: {
method: string;
displayName: string;
active: boolean;
}[];
multiuser: boolean;
}
'subscribe-needs-bootstrap': (args: { url }) => Promise<
NeedsBootstrapError | NeedsBootstrapNoServer | NeedsBootstrapWithServer
>;This clarification aids in type checking and code comprehension.
| 'subscribe-sign-in': ( | ||
| arg: | ||
| | { | ||
| password; | ||
| loginMethod?: string; | ||
| } | ||
| | { | ||
| return_url; | ||
| loginMethod?: 'openid'; | ||
| }, | ||
| ) => Promise<{ error?: string }>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine argument types in subscribe-sign-in for clarity
To improve type safety and eliminate ambiguity, explicitly type the arguments and adjust the usage of loginMethod.
Suggestion:
'subscribe-sign-in': (
arg:
| {
password: string;
loginMethod?: string;
}
| {
return_url: string;
loginMethod: 'openid';
},
) => Promise<{ error?: string }>;- Specify
passwordandreturn_urlasstringtypes. - Set
loginMethodto'openid'explicitly in the second case, and consider making it required to clarify intent.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/manager/BudgetList.tsx (2)
75-77: Consider moving type guard to shared utilities.The
isLocalFiletype guard function could be useful across different components. Consider moving it to a shared utilities file for better code organization and reusability.
225-238: Optimize getOwnerDisplayName with useMemo.The
getOwnerDisplayNamefunction is recreated on every render and performs object lookups. Consider memoizing it to improve performance.const getOwnerDisplayName = useMemo(() => { if ( !( file.state === 'remote' || file.state === 'synced' || file.state === 'detached' ) ) { return ''; } const userFound = file.usersWithAccess?.find(f => f.owner); return userFound?.displayName ?? userFound?.userName ?? 'unknown'; }, [file.state, file.usersWithAccess]);
| <UserAccessForFile | ||
| fileId={(file as RemoteFile).cloudFileId} | ||
| currentUserId={currentUserId} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type guard for cloudFileId access.
Casting to RemoteFile might fail if the file is of a different type. Consider adding a type guard to safely access cloudFileId.
const cloudFileId = 'state' in file &&
(file.state === 'remote' || file.state === 'synced' || file.state === 'detached')
? file.cloudFileId
: undefined;
{cloudFileId && (
<UserAccessForFile
fileId={cloudFileId}
currentUserId={currentUserId}
/>
)}| function UserAccessForFile({ fileId, currentUserId }: UserAccessForFileProps) { | ||
| const allFiles = useSelector(state => state.budgets.allFiles || []); | ||
| const remoteFiles = allFiles.filter( | ||
| f => f.state === 'remote' || f.state === 'synced' || f.state === 'detached', | ||
| ) as (SyncedLocalFile | RemoteFile)[]; | ||
| const currentFile = remoteFiles.find(f => f.cloudFileId === fileId); | ||
| const multiuserEnabled = useMultiuserEnabled(); | ||
|
|
||
| let usersAccess = currentFile?.usersWithAccess ?? []; | ||
| usersAccess = usersAccess?.filter(user => user.userName !== '') ?? []; | ||
|
|
||
| const sortedUsersAccess = [...usersAccess].sort((a, b) => { | ||
| const textA = | ||
| a.userId === currentUserId ? 'You' : (a.displayName ?? a.userName); | ||
| const textB = | ||
| b.userId === currentUserId ? 'You' : (b.displayName ?? b.userName); | ||
| return textA.localeCompare(textB); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error boundary and optimize user sorting.
The component could benefit from error handling and performance optimizations:
- Wrap the component in an error boundary to gracefully handle rendering errors
- Memoize the filtered and sorted users list
const filteredAndSortedUsers = useMemo(() => {
const filtered = (currentFile?.usersWithAccess ?? [])
.filter(user => user.userName !== '');
return [...filtered].sort((a, b) => {
const textA = a.userId === currentUserId ? 'You' : (a.displayName ?? a.userName);
const textB = b.userId === currentUserId ? 'You' : (b.displayName ?? b.userName);
return textA.localeCompare(textB);
});
}, [currentFile?.usersWithAccess, currentUserId]);| <Tooltip | ||
| content={ | ||
| <View | ||
| style={{ | ||
| margin: 5, | ||
| }} | ||
| > | ||
| <Text | ||
| style={{ | ||
| ...styles.altMenuHeaderText, | ||
| ...styles.verySmallText, | ||
| color: theme.pageTextLight, | ||
| }} | ||
| > | ||
| File shared with: | ||
| </Text> | ||
| <View | ||
| style={{ | ||
| padding: 0, | ||
| }} | ||
| > | ||
| {sortedUsersAccess.map(user => ( | ||
| <View key={user.userId} style={{ flexDirection: 'row' }}> | ||
| <SvgUser | ||
| style={{ | ||
| width: 10, | ||
| height: 10, | ||
| opacity: 0.7, | ||
| marginTop: 3, | ||
| marginRight: 5, | ||
| }} | ||
| /> | ||
| <View | ||
| style={{ | ||
| ...styles.verySmallText, | ||
| color: theme.pageTextLight, | ||
| margin: 0, | ||
| listStylePosition: 'inside', | ||
| }} | ||
| > | ||
| {user.userId === currentUserId | ||
| ? 'You' | ||
| : (user.displayName ?? user.userName)} | ||
| </View> | ||
| </View> | ||
| ))} | ||
| </View> | ||
| </View> | ||
| } | ||
| placement="bottom end" | ||
| > | ||
| <SvgUserGroup | ||
| style={{ | ||
| width: 15, | ||
| height: 15, | ||
| alignSelf: 'flex-end', | ||
| opacity: 0.7, | ||
| }} | ||
| /> | ||
| </Tooltip> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance tooltip accessibility.
The tooltip implementation needs accessibility improvements:
- Add aria-label to the trigger icon
- Add keyboard navigation support
- Add role and aria-expanded attributes
<Tooltip
+ role="tooltip"
content={...}
placement="bottom end"
>
<SvgUserGroup
+ aria-label="Show users with file access"
+ role="button"
+ tabIndex={0}
+ aria-expanded={false}
style={{
width: 15,
height: 15,
alignSelf: 'flex-end',
opacity: 0.7,
}}
/>
</Tooltip>Committable suggestion skipped: line range outside the PR's diff.
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
Closing this PR and open a new one after a rebase. |
Following the PR #1164
Instruction on how to test this.
Clone actual server PR and this PR locally, at the server, in the packages.json, point the @actual/web to link to this PR clone (this is important because the pages will come from this package)
This is how I do:
It's important to remember to BUILD the desktop-client package before starting the server.
I suggest to start testing this using a empty server instead of an server already configured.
Once you have actual server running, you can open the URL and start configuring it using this preview doc https://deploy-preview-447.www.actualbudget.org/docs/advanced/oauth-auth
I suggest start with configuring from bootstrap. Use passwordless.id for testing, probably the easiest way to test.
You can test the openid provider login now. But to test multiple users, you have to configure multiuser support