Openid

[lelemm]

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

[coderabbitai]

Actionable comments posted: 20

🧹 Outside diff range and nitpick comments (9)

packages/desktop-client/src/components/modals/TransferOwnership.tsx (1)

66-149: Render logic is well-implemented, with a minor suggestion for accessibility.

The component's render logic is well-structured, using appropriate layout components and conditional rendering. The error display and button states are handled correctly.

Consider adding an aria-label to the Select component to improve accessibility:

<Select
  options={availableUsers}
  onChange={(newValue: string) => {
    setUserId(newValue);
  }}
  value={userId}
+ aria-label="Select new budget owner"
/>

This will provide better context for screen reader users.

packages/desktop-client/src/components/LoggedInUser.tsx (2)

47-48: LGTM: Improved state management and effects.

The addition of isOwner and multiuserEnabled state variables, along with the new effect to check file access permissions, enhances the component's ability to handle user permissions and multi-user functionality. The inclusion of getUserData in the dependency array of its effect ensures proper updates.

Consider adding error handling to the file access check:

useEffect(() => {
  if (cloudFileId) {
    send('check-file-access', cloudFileId)
      .then(({ granted }: { granted: boolean }) => setIsOwner(granted))
      .catch(error => {
        console.error('Error checking file access:', error);
        setIsOwner(false);
      });
  }
}, [cloudFileId]);

This will ensure that any errors during the file access check are logged and the user is not incorrectly set as an owner.

Also applies to: 51-54, 56-62

---

64-109: LGTM: Improved menu handling with a suggestion for refactoring.

The handleMenuSelect function has been significantly enhanced to handle more menu options and integrate better with the application's navigation system. The use of window.__navigate when available is a good approach.

To reduce code duplication, consider refactoring 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 will make the code more DRY and easier to maintain.

packages/desktop-client/src/components/manager/BudgetList.tsx (2)

Line range hint 256-328: Consider memoizing UserAccessForFile component.

The changes to the FileItem component look good overall. However, to optimize performance, consider memoizing the UserAccessForFile component, especially if the list of files is large.

You can use React.memo to prevent unnecessary re-renders:

const MemoizedUserAccessForFile = React.memo(UserAccessForFile);

// Then in the JSX:
{isOpenID && (
  <MemoizedUserAccessForFile
    fileId={(file as RemoteFile).cloudFileId}
    ownerId={file.owner}
    currentUserId={currentUserId}
    usersPerFile={usersPerFile}
  />
)}

This will ensure that the UserAccessForFile component only re-renders when its props change, potentially improving performance for large lists of files.

---

Line range hint 1-753: Consider further code organization and reusability improvements.

The implementation of multi-user functionality is generally well done. However, there are a few areas where the code organization could be improved:

1. Some components, particularly BudgetList, have grown quite large. Consider splitting it into smaller, more focused components to improve readability and maintainability.

2. There's some repeated logic around fetching and managing user data that could potentially be extracted into a custom hook, e.g., useUserData. This would help to keep the components cleaner and more focused on their primary responsibilities.

Here's an example of how you might create a custom hook for user data:

function useUserData(multiuserEnabled: boolean, userData: { offline: boolean, userId: string }) {
  const [users, setUsers] = useState<UserEntity[]>([]);
  const [currentUserId, setCurrentUserId] = useState('');
  const [usersPerFile, setUsersPerFile] = useState(new Map<string, UserAccessEntity[]>());

  // Include the fetchUsers and fetchFileAccess logic here

  useEffect(() => {
    if (multiuserEnabled && !userData?.offline) {
      fetchUsers();
      fetchFileAccess();
    }
  }, [multiuserEnabled, userData?.offline, fetchUsers, fetchFileAccess]);

  return { users, currentUserId, usersPerFile };
}

This would allow you to simplify the BudgetList component and potentially reuse this logic elsewhere in the application.

packages/desktop-client/src/components/Modals.tsx (1)

606-613: LGTM: 'edit-user' case implemented correctly

The 'edit-user' case is implemented correctly, rendering the EditUserFinanceApp component with the appropriate props.

Consider using a more generic name for the EditUserFinanceApp component, such as EditUser, to improve its potential for reuse across different contexts.

packages/desktop-client/src/components/modals/EditUser.tsx (3)

149-150: Missing 'id' attribute in 'Input' component for 'Username' field

The FormLabel uses htmlFor="name-field", but the associated Input component lacks an id attribute. This may impact accessibility and label association.

Add the id attribute to the Input component:

- <Input value={userName} onChangeValue={text => setUserName(text)} />
+ <Input id="name-field" value={userName} onChangeValue={text => setUserName(text)} />

---

224-224: Unintended fixed width on 'Stack' component

The Stack component has a style with width: '100px', which may conflict with the FormField's flex: 1 style, potentially causing layout issues.

Consider removing or adjusting the width to ensure responsive design:

- <Stack direction="row" style={{ marginTop: 10, width: '100px' }}>
+ <Stack direction="row" style={{ marginTop: 10 }}>

---

149-151: Ensure consistent use of 'htmlFor' and 'id' attributes

For better accessibility, ensure that all FormLabel components have matching htmlFor attributes and that the corresponding inputs have the id attribute set.

Apply this pattern to the 'Username' field:

  <FormLabel title="Username" htmlFor="name-field" />
- <Input value={userName} onChangeValue={text => setUserName(text)} />
+ <Input id="name-field" value={userName} onChangeValue={text => setUserName(text)} />

And similarly for other form fields.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ce1b8b7 and 5a23f3f.

📒 Files selected for processing (10)

• packages/desktop-client/src/auth/ProtectedRoute.tsx (1 hunks)
• packages/desktop-client/src/components/LoggedInUser.tsx (4 hunks)
• packages/desktop-client/src/components/Modals.tsx (4 hunks)
• packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx (1 hunks)
• packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx (1 hunks)
• packages/desktop-client/src/components/common/Button2.tsx (3 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/PasswordEnableModal.tsx (1 hunks)
• packages/desktop-client/src/components/modals/TransferOwnership.tsx (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• packages/desktop-client/src/auth/ProtectedRoute.tsx
• packages/desktop-client/src/components/common/Button2.tsx
• packages/desktop-client/src/components/modals/PasswordEnableModal.tsx

🧰 Additional context used

🪛 Biome

packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx

[error] 94-94: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 95-95: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (26)

packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx (4)

1-19: LGTM: Imports and type definition are well-structured.

The imports are appropriate for the component's functionality, and the UserAccessProps type is well-defined. The use of strict TypeScript checking is a good practice for maintaining code quality.

---

59-82: LGTM: Error handling is well-implemented.

The handleError function effectively manages different error scenarios, including a special case for token expiration. The use of the addNotification action for displaying errors is consistent and user-friendly.

---

141-143: LGTM: Component export and display name are correctly implemented.

The memoized export of the component and the setting of the display name for debugging purposes are both good practices.

---

1-143: Overall, well-implemented component with room for minor improvements.

The UserAccessRow component is well-structured, follows React best practices, and includes proper error handling and performance optimizations. To further enhance the code quality and maintainability, consider implementing the following suggestions:

1. Simplify the marked state initialization.
2. Create a custom hook for cloudFileId.
3. Refactor the handleAccessToggle function for improved readability.
4. Use optional chaining for onHover event handlers.

These changes will make the component more readable, maintainable, and potentially more reusable across the application.

🧰 Tools

🪛 Biome

[error] 94-94: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 95-95: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

packages/desktop-client/src/components/modals/TransferOwnership.tsx (4)

1-22: Imports and type definitions look good.

The necessary dependencies are imported, and the TransferOwnershipProps type is well-defined. The code structure follows best practices for React and TypeScript.

---

34-47: User fetching logic is well-implemented.

The useEffect hook correctly fetches users, filters out the current user, and formats the data for the Select component. The dependency array is properly set to re-fetch when the current user changes.

---

1-149: Overall, the TransferOwnership component is well-implemented.

The component effectively handles the process of transferring budget ownership. It follows React best practices, uses appropriate hooks for state management and side effects, and has a clear and user-friendly UI.

A few minor improvements have been suggested:

1. Fixing a typo in the error state setter name.
2. Enhancing type safety and error handling in the onSave function.
3. Adding an aria-label to the Select component for better accessibility.

After addressing these points, the component will be in excellent shape for production use.

---

49-64: ⚠️ Potential issue

Improve 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:

1. Use the corrected error setter name.
2. 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 (3)

4-4: LGTM: Enhanced imports and improved type safety.

The new imports (useLocation, send, useAuth, Permissions, useMetadataPref, useNavigate, useMultiuserEnabled, useServerURL) suggest improved routing, authentication, and server-related functionality. The explicit typing of triggerRef as HTMLButtonElement enhances type safety.

Also applies to: 6-6, 9-10, 12-13, 21-21, 41-41

---

140-196: LGTM: Improved menu item generation and type safety.

The addition of the MenuItem type and the getMenuItems function significantly improves the component's structure and maintainability:

1. The MenuItem type enhances type safety for menu items.
2. The getMenuItems function encapsulates complex logic for determining which menu items to display based on user permissions and application state.

These changes make the code more readable and easier to maintain.

---

Line range hint 1-226: Overall LGTM: Significant improvements aligned with PR objectives

This PR has made substantial improvements to the LoggedInUser component, aligning well with the stated objectives:

1. Enhanced multi-user support through conditional rendering and permission checks.
2. Improved handling of different user roles (admin, file owner).
3. Better integration with the application's navigation system.
4. Refactored code for improved maintainability and readability.

These changes contribute to a more robust user management system, which is crucial for the OpenID implementation mentioned in the PR objectives.

There are a couple of minor internationalization issues to address:

1. Wrapping "logged as:" text with a translation function.
2. Wrapping "Change server URL" text with a translation function.

Once these are resolved, the component will be well-prepared for multi-user scenarios and internationalization.

Great work on this significant refactoring!

packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx (3)

271-276: LGTM: UserAccess component is well-structured.

The UserAccess component is a clean and simple wrapper for UserAccessContent. It properly passes down the required props and provides a default value for setLoading. This structure allows for easy reuse and potential extension in the future.

---

1-336: Summary: Solid implementation with room for improvements

Overall, the UserAccess.tsx file implements a comprehensive user access management system with good structure and functionality. Here's a summary of the key points and suggestions for improvement:

1. Address TypeScript strict mode violations to enhance type safety.
2. Improve error handling in data loading functions.
3. Consider breaking down the large UserAccessContent component into smaller, more focused sub-components.
4. Optimize performance by memoizing the UserAccessList component.
5. Enhance the LockToggle component with proper TypeScript typing and consistent naming conventions.

Implementing these suggestions will further improve the code's robustness, maintainability, and performance. Great job on the implementation, and these refinements will take it to the next level!

---

1-35: ⚠️ Potential issue

Consider addressing TypeScript strict mode violations.

The @ts-strict-ignore comment at the beginning of the file suggests potential TypeScript strict mode violations. It's recommended to address these issues to improve type safety and maintainability of the code.

To resolve this:

1. Remove the @ts-strict-ignore comment.
2. Address any TypeScript errors that appear after removing the comment.
3. If there are specific reasons for ignoring certain TypeScript checks, use more specific ignore comments for those particular lines or blocks of code.

To identify potential TypeScript issues, run the following command:

This will help pinpoint specific areas that need attention to comply with strict mode.

packages/desktop-client/src/components/manager/BudgetList.tsx (4)

21-31: LGTM: New imports and type definitions.

The new imports and type definitions are appropriate for implementing the multi-user functionality. They provide the necessary types and functions for handling remote files, user entities, and user access.

---

147-254: LGTM: FileState component updates for multi-user support.

The changes to the FileState component appropriately handle the display of file ownership information for multi-user scenarios. The new props and conditional rendering logic enhance the component's functionality for OpenID-enabled files.

---

Line range hint 353-407: LGTM: BudgetFiles component updates for multi-user support.

The changes to the BudgetFiles component correctly pass down the new props related to multi-user functionality to the FileItem components. This ensures that each file item can display the appropriate user access information.

---

181-185: ⚠️ Potential issue

Remove duplicate 'broken' case in switch statement.

There's a duplicate case for the 'broken' state in the switch statement. This is likely an oversight and should be removed to avoid potential issues.

Apply this diff to 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;

Likely invalid or redundant comment.

packages/desktop-client/src/components/Modals.tsx (6)

30-30: LGTM: New imports added correctly

The new component imports are correctly placed and follow the existing import style. They correspond to the new modal cases added later in the file, which is good for maintaining code organization.

Also applies to: 33-33, 54-55, 66-66

---

597-604: LGTM: 'edit-access' case implemented correctly

The 'edit-access' case is implemented correctly, rendering the EditUserAccess component with the appropriate props.

Note: The past review comment about a duplicate 'edit-access' case is no longer applicable, as there's only one such case in the current code.

---

615-616: LGTM: 'transfer-ownership' case implemented correctly

The 'transfer-ownership' case is implemented correctly, rendering the TransferOwnership component.

Verify if the TransferOwnership component requires any additional props, such as defaultUser, similar to other user-related modals. If not needed, the current implementation is correct.

---

618-619: LGTM: 'enable-openid' case implemented correctly

The 'enable-openid' case is implemented correctly, rendering the OpenIDEnableModal component with the appropriate onSave prop. This aligns with the OpenID functionality mentioned in the PR objectives.

---

621-622: LGTM: 'enable-password-auth' case implemented correctly

The 'enable-password-auth' case is implemented correctly, rendering the PasswordEnableModal component with the appropriate onSave prop. This seems to be part of the authentication system improvements alongside the OpenID functionality.

---

30-30: Overall: Changes look good and align with PR objectives

The new modal cases for user access, user editing, ownership transfer, OpenID, and password authentication have been implemented correctly and consistently with the existing code structure. These changes align well with the PR objectives, particularly the introduction of OpenID functionality.

A few minor suggestions were made:

1. Consider using a more generic name for the EditUserFinanceApp component.
2. Verify if the TransferOwnership component requires any additional props.

These changes enhance the application's authentication and user management capabilities while maintaining code quality and consistency.

Also applies to: 33-33, 54-55, 66-66, 597-622

packages/desktop-client/src/components/modals/EditUser.tsx (2)

170-174: Verify styling of the 'Checkbox' component

The Checkbox component includes a style prop setting the color property, which may not correctly affect the appearance of a disabled checkbox.

Please verify if the color style properly affects the Checkbox when disabled. Consider using the disabledStyle prop or adjusting the styling approach if needed.

---

172-172: Verify existence of 'master' property on 'User' type

The defaultUser.master property is used to conditionally disable inputs, but it's essential to ensure that this property exists on the User type.

Please confirm that the UserEntity type includes the master property to prevent TypeScript errors or runtime issues.

[coderabbitai]

🛠️ Refactor suggestion

Consider simplifying state initialization and creating a custom hook for cloudFileId.

1. Simplify the marked state initialization:

const [marked, setMarked] = useState(access.owner === 1 || access.haveAccess === 1);

2. Consider creating a custom hook for cloudFileId:

function useCloudFileId() {
  return useMetadataPref('cloudFileId')[0];
}

// In the component:
const cloudFileId = useCloudFileId();

These changes would improve readability and potentially increase reusability across different components.

[coderabbitai]

🛠️ Refactor suggestion

Refactor handleAccessToggle for improved readability and error handling.

1. Consider extracting the access addition and deletion logic into separate functions for better readability:

const addAccess = async () => {
  const { error } = await send('access-add', {
    fileId: cloudFileId as string,
    userId: access.userId,
  });
  if (error) handleError(error);
  return !error;
};

const deleteAccess = async () => {
  const { someDeletionsFailed } = await send('access-delete-all', {
    fileId: cloudFileId as string,
    ids: [access.userId],
  });
  if (someDeletionsFailed) {
    actions.addNotification({
      type: 'error',
      title: 'Access Revocation Incomplete',
      message: 'Some access permissions were not revoked successfully.',
      sticky: true,
    });
  }
  return !someDeletionsFailed;
};

const handleAccessToggle = async () => {
  const newValue = !marked;
  const success = newValue ? await addAccess() : await deleteAccess();
  if (success) setMarked(newValue);
};

This refactoring improves code readability and makes it easier to maintain and test the access toggle logic.

[coderabbitai]

🛠️ Refactor suggestion

Improve event handling with optional chaining.

As suggested by static analysis, you can improve the onMouseEnter and onMouseLeave event handlers by using optional chaining:

onMouseEnter={() => onHover?.(access.userId)}
onMouseLeave={() => onHover?.(null)}

This change simplifies the code and ensures that onHover is called only if it's defined.

🧰 Tools

🪛 Biome

[error] 94-94: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 95-95: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

[coderabbitai]

⚠️ Potential issue

Fix typo in error state setter name.

The component initialization and state setup are well-implemented. However, there's a typo in the error state setter name.

Please apply the following change:

- 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.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	export function TransferOwnership({
	onSave: originalOnSave,
	}: TransferOwnershipProps) {
	const userData = useSelector((state: State) => state.user.data);
	const actions = useActions();
	const [userId, setUserId] = useState('');
	const [error, setSetError] = useState<string | null>(null);
	const [availableUsers, setAvailableUsers] = useState<[string, string][]>([]);
	const [cloudFileId] = useMetadataPref('cloudFileId');
	export function TransferOwnership({
	onSave: originalOnSave,
	}: TransferOwnershipProps) {
	const userData = useSelector((state: State) => state.user.data);
	const actions = useActions();
	const [userId, setUserId] = useState('');
	const [error, setError] = useState<string | null>(null);
	const [availableUsers, setAvailableUsers] = useState<[string, string][]>([]);
	const [cloudFileId] = useMetadataPref('cloudFileId');

[coderabbitai]

⚠️ Potential issue

Add error handling to the loadAccess function.

The loadAccess function doesn't have any error handling. Consider adding a try-catch block to handle potential errors gracefully:

const loadAccess = useCallback(async () => {
  setLoading(true);
  try {
    const users: UserAvailable[] = await send(
      'access-get-available-users',
      cloudFileId as string,
    );

    // ... rest of the function

    return loadedAccess;
  } catch (error) {
    console.error('Error loading user access:', error);
    // Consider showing an error message to the user
    return [];
  } finally {
    setLoading(false);
  }
}, [cloudFileId, setLoading]);

This change ensures that any errors during data loading are caught and handled appropriately, improving the robustness of the component.

[coderabbitai]

🛠️ Refactor suggestion

Optimize UserAccessForFile component with useMemo.

The sorting logic in the UserAccessForFile component could be optimized by moving it into a useMemo hook. This would prevent unnecessary re-computations on each render.

Here's how you could implement this:

const sortedUsersAccess = useMemo(() => {
  const filteredUsers = usersPerFile?.get(fileId)?.filter(user => user.userId !== ownerId) ?? [];
  return [...filteredUsers].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);
  });
}, [fileId, ownerId, currentUserId, usersPerFile]);

This will ensure that the sorting only happens when the relevant data changes.

---

🛠️ Refactor suggestion

Enhance accessibility in UserAccessForFile component.

The UserAccessForFile component could benefit from some accessibility improvements. Consider adding appropriate ARIA attributes and ensuring that the tooltip can be navigated using a keyboard.

Here are some suggestions:

1. Add an aria-label to the SvgUserGroup icon:

<SvgUserGroup
  aria-label="Show users with file access"
  style={{
    width: 15,
    height: 15,
    alignSelf: 'flex-end',
    opacity: 0.7,
  }}
/>

2. Ensure the Tooltip component supports keyboard navigation. If it doesn't already, consider implementing this functionality or using an accessible tooltip library.

3. Add a tabIndex to make the tooltip focusable:

<Tooltip
  tabIndex={0}
  content={...}
>
  ...
</Tooltip>

These changes will improve the component's accessibility for users relying on screen readers or keyboard navigation.

[coderabbitai]

🛠️ Refactor suggestion

Adjust role selection component for better user experience

The role selection Select component might be improved by displaying user-friendly labels instead of raw object entries.

Map the PossibleRoles entries to an array of { value, label } objects for clearer role names.

Example:

const roleOptions = Object.entries(PossibleRoles).map(([id, name]) => ({
  value: id,
  label: name,
}));

Then update the Select component:

- <Select
-   disabled={defaultUser.master}
-   options={Object.entries(PossibleRoles)}
-   value={role}
-   onChange={newValue => setRole(newValue)}
- />
+ <Select
+   disabled={defaultUser.master}
+   options={roleOptions}
+   value={role}
+   onChange={newValue => setRole(newValue)}
+ />

[coderabbitai]

🛠️ Refactor suggestion

Consider centralizing error messages for reusability

The getUserDirectoryErrors function contains hardcoded error messages. Centralizing these messages in a constants file can improve maintainability.

Extract error messages into a separate module or constants object to facilitate reuse and localization.

[coderabbitai]

⚠️ Potential issue

Typo in error message: 'does not exists' should be 'does not exist'

In the getUserDirectoryErrors function, there's a grammatical error in the error message for 'role-does-not-exists'.

Apply this diff to fix the typo:

- 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.';

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	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.';

[coderabbitai]

⚠️ Potential issue

Typo in 'htmlFor' attribute: 'dispalyname-field' should be 'displayname-field'

The FormLabel component has a typo in the htmlFor attribute, which may affect label association with the input field.

Apply this diff to correct the typo:

- <FormLabel title="Display Name" htmlFor="dispalyname-field" />
+ <FormLabel title="Display Name" htmlFor="displayname-field" />

Also, ensure the corresponding Input component has the matching id attribute:

- <Input
+ <Input id="displayname-field"

Committable suggestion was skipped due to low confidence.

[coderabbitai]

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 AdminHandlers interface 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 ids array 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 AdminHandlers interface provide a wide range of admin operations with appropriate use of TypeScript for type safety. Here are some suggestions for minor improvements:

1. For the 'check-file-access' method, consider renaming it to 'is-file-access-granted' for clarity.
2. The 'access-add' method returns Record<string, never> on success, which is equivalent to an empty object. Consider using void instead for clarity.
3. 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 improvement

The del function is well-implemented and consistent with the existing post function. It correctly handles DELETE requests, timeouts, and error scenarios. However, consider the following improvements:

1. Add type annotations to the function parameters and return type for better type safety.
2. Refactor the error handling logic into a separate function to reduce code duplication across post, del, and patch.
3. 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 refactoring

The patch function is correctly implemented and consistent with the existing post and del functions. However, there's a significant amount of code duplication across these three functions. Consider the following refactoring suggestions:

1. Create a generic sendRequest function that can handle different HTTP methods (POST, DELETE, PATCH).
2. Add type annotations to improve type safety.
3. 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 OpenIdLogin component effectively handles the OpenID login process, including appropriate warnings and configuration options. The use of useEffect to check for owner creation is a good practice.

Consider adding a brief comment explaining the purpose of the warnMasterCreation state, 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 Login component significantly improves its flexibility and maintainability. The use of the useLoginMethod hook and conditional rendering of login methods enhances the user experience.

Consider using a constant for the 'internal-error' string in the getErrorMessage function for consistency with other error types.

packages/loot-core/src/server/main.ts (1)

1566-1568: Consider using strict equality checks

In 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

📥 Commits

Files that changed from the base of the PR and between 5a23f3f and 5cb9613.

📒 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 AdminHandlers interface 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:

1. Comprehensive coverage of admin operations
2. Consistent use of async/await pattern
3. Strong typing for parameters and return values

Suggestions for improvement:

1. Add JSDoc comments to all methods for better documentation
2. Clarify some method names and return types for improved readability
3. 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 handling

After reviewing all methods in this file, here are the main points to address:

1. Inconsistent return types: Most methods return different types based on the outcome. Consider standardizing return types across all methods for consistency.

2. Lack of error handling: Many methods don't handle potential exceptions, especially during JSON parsing. Add try-catch blocks to handle these cases.

3. Inconsistent patterns: Some methods (e.g., multiuser-get and owner-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.

4. Repetitive code: Many methods follow a similar structure. Consider extracting common logic into utility functions to reduce duplication.

Suggested approach for refactoring:

1. Define a standard return type interface that all methods will use:

interface ApiResult<T> {
  success: boolean;
  error?: string;
  data?: T;
}

2. 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 -->

[coderabbitai]

🛠️ 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.

[coderabbitai]

🛠️ 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.

Suggested change

	res = await fetch(getServer().SIGNUP_SERVER + '/login-methods').then(res =>
	res.json(),
	);
	res = await get(getServer().SIGNUP_SERVER + '/login-methods');

[coderabbitai]

⚠️ Potential issue

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.

Suggested change

	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 {};
	};

[coderabbitai]

⚠️ Potential issue

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.

Suggested change

	} = 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 };

[coderabbitai]

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 safety

The changes to the component's props and state management are well-implemented:

• Explicit typing of triggerRef as HTMLButtonElement enhances type safety.
• New state variables (isOwner, multiuserEnabled) and hooks provide better user management and routing capabilities.
• The use of useMetadataPref for budgetId and cloudFileId improves metadata handling.

For consistency, consider destructuring the useAuth hook:

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 management

The changes to the useEffect hooks are well-implemented:

• Including getUserData in the dependency array ensures the effect runs when the function changes, improving reliability.
• The new effect for updating isOwner enhances the component's ability to manage user permissions based on the current file.

To optimize the new effect, consider memoizing the isOwner calculation:

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 isOwner state and effect, potentially improving performance.

---

69-114: LGTM: Enhanced menu handling with improved navigation

The changes to the handleMenuSelect function (previously onChangePassword) are well-implemented:

• The function now handles a variety of menu selections, improving the component's versatility.
• The use of window.__navigate ensures 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 safety

The addition of the MenuItem type and the getMenuItems function significantly improves the component's structure and maintainability:

1. The MenuItem type enhances type safety for menu items.
2. The getMenuItems function encapsulates complex logic for determining which menu items to display based on user permissions and application state.

To improve flexibility, consider adding an optional disabled property to the MenuItem type:

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 function

The enhanced error handling in the upload function 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 function

The addition of error handling for fetching user file information in the download function 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.log for 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 SvgUserGroup icon lacks an accessible label, which may cause issues for users relying on screen readers.

Consider adding an aria-label to the SvgUserGroup icon:

<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

📥 Commits

Files that changed from the base of the PR and between 5cb9613 and 97ebff9.

📒 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 robustness

After 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 issue

Consider removing // @ts-strict-ignore and addressing type issues

The use of // @ts-strict-ignore at 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 safety

The 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 management

The addition of the UsersWithAccess interface and the update to the RemoteFile interface 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 management

The updates to the metadata handling in the importBuffer function are well-implemented. The inclusion of lastUploaded using monthUtils.currentDay() and the conditional assignment of encryptKeyId improve 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 unavailable

In the case where remoteFiles is null, the owner property is set to an empty string '':

owner: '',

Ensure that downstream components can handle an empty string for the owner property without issues. If null or undefined is the standard for unknown owners in your application, consider using one of those values instead.

---

83-84: Ensure consistent handling in the 'detached' state

In the 'detached' state, you assign owner and usersWithAccess similar 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 owner is set to an empty string:

owner: '',

Confirm that representing the owner as an empty string aligns with the application's conventions for a 'broken' file. If a different placeholder or value is more appropriate (e.g., null or a specific indicator), consider updating it accordingly.

[coderabbitai]

🛠️ 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:

1. Standardize the return type to always return an object with a consistent structure.
2. Handle the case where res is 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.

[coderabbitai]

⚠️ Potential issue

LGTM: Improved rendering logic with a note on internationalization

The changes to the component's JSX improve its flexibility and maintainability:

1. Conditional rendering of user information for multi-user environments.
2. Use of getMenuItems function 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

[coderabbitai]

⚠️ Potential issue

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.

[coderabbitai]

🛠️ 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.

[coderabbitai]

🛠️ 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.

[coderabbitai]

⚠️ Potential issue

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.

Suggested change

	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;

[coderabbitai]

⚠️ Potential issue

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.

[coderabbitai]

⚠️ Potential issue

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.

[coderabbitai]

Actionable comments posted: 12

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 97ebff9 and 43b49e2.

📒 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 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. 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 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. 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:

[coderabbitai]

🛠️ 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:

1. Standardize the return type to always return an object with a consistent structure.
2. Handle the case where userToken is falsy explicitly.
3. Use a try-catch block to handle potential errors from the del function.

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.

[coderabbitai]

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-ignore and addressing type issues

The use of @ts-strict-ignore at 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-ignore comment and addressing any resulting type errors. This will improve the overall type safety of the code.

---

1-252: Overall improvements for consistency and robustness

After reviewing all methods in this file, here are some general recommendations for improvement:

1. Implement consistent error handling and return types across all methods. This will make the API more predictable and easier to use.
2. Consider creating utility functions for common operations like token retrieval and API calls to reduce code duplication.
3. Add proper TypeScript types for all method parameters and return values to improve type safety.
4. Implement proper error logging to aid in debugging and monitoring.
5. Consider implementing a more robust authentication mechanism, possibly using a higher-order function to wrap all authenticated methods.
6. Remove the @ts-strict-ignore at the top of the file and address any underlying type issues.

To implement these improvements, consider the following architectural changes:

1. Create a base ApiResult<T> type that all methods can use for their return values:

type ApiResult<T> = {
  success: boolean;
  error?: string;
  data?: T;
};

2. 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);
}

3. 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 del

These 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

📥 Commits

Files that changed from the base of the PR and between 43b49e2 and 7bdff09.

📒 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)

[coderabbitai]

🛠️ 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. Handle the case where res is 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.

[coderabbitai]

🛠️ 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:

1. Add a try-catch block to handle potential errors, including Map creation errors.
2. Standardize the return type to always return an object with a consistent structure.
3. Handle the case where res is 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.

[coderabbitai]

🛠️ 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:

1. Standardize the return type to always return an object with a consistent structure.
2. Handle the case where userToken is falsy explicitly.
3. 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.

[coderabbitai]

🛠️ 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:

1. Standardize the return type for consistency.
2. Improve error handling to provide more informative error messages.
3. Combine user-add and user-update into 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.

[coderabbitai]

🛠️ 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. Handle the case where res is 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.

[coderabbitai]

🛠️ 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. Handle the case where res is 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.

[coderabbitai]

🛠️ 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:

1. Standardize the return type for consistency with other methods.
2. Handle the case where userToken is falsy explicitly.
3. 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.

[coderabbitai]

🛠️ 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. Handle the case where res is 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.

[coderabbitai]

🛠️ 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. 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.

[coderabbitai]

🛠️ 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:

1. Add a try-catch block to handle potential errors, including JSON parsing errors.
2. Standardize the return type to always return an object with a consistent structure.
3. 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.

[coderabbitai]

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 onMouseEnter and onMouseLeave can 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 hooks

The new state variables and hooks improve the component's functionality. The updated useEffect for getUserData and the new useEffect for checking file access permissions are well-implemented.

One minor suggestion:

Consider adding userData to the dependency array of the second useEffect to 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 handling

The renamed and expanded handleMenuSelect function enhances the component's flexibility and navigation capabilities. The use of window.__navigate with a fallback to window.location.href is a good approach for compatibility.

One minor suggestion:

Consider using a switch statement with case blocks instead of if-else for 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 organization

The overall restructuring and enhancement of the LoggedInUser component 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 serverMessage function 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 serverMessage logic to be reused elsewhere if needed.

packages/desktop-client/src/components/modals/EditUser.tsx (1)

258-340: RoleDescription component is well-implemented

The RoleDescription component 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 function

The enhanced error handling in the upload function 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 function

The updates to the metadata handling in the importBuffer function are well-implemented. The addition of lastUploaded and encryptKeyId properties aligns with the PR objectives of enhancing file management functionalities.

For improved clarity, consider destructuring the fileData object:

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 fileData object.

---

Line range hint 478-481: Improved error handling in download function

The enhanced error handling for fetching user file information in the download function 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

📥 Commits

Files that changed from the base of the PR and between 7bdff09 and 3ec6cc1.

📒 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 issue

Fix 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 TransferOwnershipProps type 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 issue

Fix 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 issue

Improve 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:

1. Use the corrected error setter name.
2. 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 definitions

The new imports and type definitions improve the component's functionality, type safety, and maintainability. The MenuItem type 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 generation

The new getMenuItems function 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 internationalization

The changes to the component's JSX improve its flexibility and maintainability:

1. Conditional rendering of user information for multi-user environments.
2. Use of getMenuItems function 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 triggerRef

The explicit typing of triggerRef as HTMLButtonElement enhances 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 good

The imports are appropriate for the component's functionality, and the type definitions for EditUserProps and EditUserFinanceAppProps are well-structured. This provides a solid foundation for type safety in the component.

---

93-118: EditUserFinanceApp component looks good

The EditUserFinanceApp component is well-structured. It correctly wraps the EditUser component with a Modal and 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 needed

This new file implements a comprehensive user management interface with good type safety and clear component structure. The main components (EditUserFinanceApp, EditUser, and RoleDescription) are well-organized and follow React best practices.

Key points:

1. The utility functions are helpful, but there's a minor typo to fix.
2. Consider improving type safety in error handling.
3. The EditUser component could benefit from some UX improvements, particularly in role selection.
4. 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 issue

Fix typo in htmlFor attribute

There's a typo in the htmlFor attribute 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 id attribute.

Likely invalid or redundant comment.

packages/loot-core/src/server/cloud-storage.ts (2)

25-30: Excellent addition of user access management structures

The new UsersWithAccess interface and the updates to the RemoteFile interface 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 handling

This PR introduces valuable enhancements to the cloud-storage.ts file, aligning well with the objectives of improving user and access management functionalities. The new UsersWithAccess interface, updates to the RemoteFile interface, 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 added

The isLocalFile function is a well-implemented type guard that will improve type safety and make it easier to work with the File type union. This is a good practice in TypeScript.

---

Line range hint 257-312: LGTM: FileItem component updated correctly for multi-user support

The FileItem component has been successfully updated to support multi-user functionality. The conditional rendering of the UserAccessForFile component and the passing of currentUserId to FileState are implemented correctly.

---

Line range hint 341-377: LGTM: BudgetFiles component updated correctly

The BudgetFiles component has been correctly updated to accept and pass down the currentUserId prop to its child FileItem components.

[coderabbitai]

⚠️ Potential issue

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:

1. Remove the // @ts-strict-ignore directive.
2. Address any TypeScript errors that emerge.
3. If you encounter difficulties resolving these errors, please let me know, and I'd be happy to assist you in fixing them.

[coderabbitai]

🛠️ Refactor suggestion

⚠️ Potential issue

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.

[coderabbitai]

🛠️ 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);

[coderabbitai]

🛠️ 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.

[coderabbitai]

⚠️ Potential issue

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:

1. The use of window.location.href might not be appropriate in a server-side context. Consider using a more generic approach or passing this responsibility to the caller.
2. 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.
3. The condition res.status === 403 is currently unreachable due to the previous condition res.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.

[coderabbitai]

🛠️ 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.

[coderabbitai]

🛠️ 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.

[coderabbitai]

🛠️ 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-label to the SvgUserGroup icon:

<SvgUserGroup
  aria-label="Show users with file access"
  style={{
    width: 15,
    height: 15,
    alignSelf: 'flex-end',
    opacity: 0.7,
  }}
/>

• Ensure the Tooltip component 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.

[coderabbitai]

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 triggerRef

The triggerRef could 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 logic

The getMenuItems function 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 loadRules function might need to handle user-specific rules in a multi-user context.

Consider:

1. Adding user context to rule loading
2. Implementing access control for rule management
3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3ec6cc1 and d4b0e49.

📒 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 since userData?.tokenExpired is derived from userData. 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:

1. The SimpleTable component was intentionally moved from rules/ to common/ in commit bd66917 ("change location of component")
2. All imports have been properly updated to use the new path in common/
3. The old location in rules/ no longer exists
4. 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:

1. EditUserAccess component accepts props:

type EditUserAccessProps = {
  defaultUserAccess: UserAccessEntity;
  onSave?: (userAccess: UserAccessEntity) => void;
};

2. EditUserFinanceApp component accepts props:

type EditUserFinanceAppProps = {
  defaultUser: User;
  onSave: (user: User) => void;
};

3. TransferOwnership component accepts props:

type TransferOwnershipProps = {
  onSave?: () => void;
};

4. OpenIDEnableModal component accepts props:

type OpenIDEnableModalProps = {
  onSave?: () => void;
};

5. 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 ProtectedRoute component accepts and correctly handles both the permission and validateOwner props.
• The authentication and authorization logic within ProtectedRoute properly 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 permission and optional validateOwner props as defined in ProtectedRouteProps
• It correctly enforces administrator permissions through hasPermission(permission)
• When validateOwner is true, it additionally verifies user ownership by checking usersWithAccess against 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

[coderabbitai]

⚠️ Potential issue

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.

Suggested change

	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]);

[coderabbitai]

🛠️ 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 cases

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

[coderabbitai]

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 ADMINISTRATOR permission 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 documentation

The owner field is boolean here but numeric in UserAvailable. 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 interfaces

Consider using consistent property naming between interfaces. UserEntity uses id while this interface uses userId.

+/**
+ * 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 definition

Consider using TypeScript's as const assertion 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:

1. Support multiple permissions per user (e.g., hasAnyPermission, hasAllPermissions)
2. Implement role-based access control (RBAC)
3. 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-get returns null for errors
• user-add/update use {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 useEffect

The useEffect hook uses setServerURL, setMultiuserEnabled, and setLoginMethods, 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 UX

The current error handling only logs the error and redirects to the config server. Consider:

1. Adding specific error messages for different failure scenarios (network issues, invalid OpenID configuration, etc.)
2. Implementing a retry mechanism for transient failures
3. 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 guidance

While the error messages are generally good, some could be more helpful:

1. For 'user-cant-be-empty', consider adding length requirements if any
2. For 'role-cant-be-empty', consider explaining available roles
3. 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 clarity

The function handles errors well but could be improved:

1. Consider adding input validation before the API call
2. 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 fields

The new interfaces look well-structured, but could benefit from some improvements:

1. Add JSDoc comments explaining the purpose of each interface and its properties
2. Clarify the difference between owner: boolean in UsersWithAccess and owner: string in RemoteFile

+/**
+ * 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 consistency

The type LoginMethods represents a single login method. Renaming it to LoginMethod would improve readability and follow standard naming conventions. Update all occurrences accordingly.

Apply this diff to rename LoginMethods to LoginMethod:

-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: Use useDispatch inside PasswordLogin for better encapsulation

Instead of passing dispatch as a prop to the PasswordLogin component, you can use the useDispatch hook 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 for send('owner-created') call

In the OpenIdLogin component, the useEffect hook doesn't handle potential errors from the send('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 elements

In the HeaderLogin component, 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 getErrorMessage function needs to handle additional error codes

The current implementation is missing some error codes that can be returned by the subscribe-sign-in handler:

• network-failure: Already handled
• unauthorized: Missing
• not-logged-in: Missing

The 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 in getErrorMessage

Verify that the getErrorMessage function covers all error codes returned by the subscribe-sign-in API to provide accurate feedback to users.

Run the following script to list all unique error codes returned by subscribe-sign-in handlers:

This will help identify any error codes not currently handled in getErrorMessage.

---

These scripts will help us:

1. Locate the file containing the subscribe-sign-in handler
2. Find error patterns in authentication flows
3. Look for handler registrations in the codebase

---

These scripts will help us:

1. Find the actual implementation of the subscribe-sign-in handler
2. 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 | uniq

Length 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 text

There's an extra period at the end of the sentence in the Text component. 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 between password and openId in subscribe-bootstrap

Currently, both password and openId are optional parameters. To prevent ambiguous usage, consider enforcing that either password or openId must 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 making openId a required parameter in enable-openid

The openId parameter is currently optional, which might lead to confusion if the method is called without it. Consider making openId a 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

📥 Commits

Reviewing files that changed from the base of the PR and between d4b0e49 and 30afb0e.

📒 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: ⚠️ Potential issue

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 === permission

This change would:

1. Validate that the user's permission is a valid enum value
2. 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.

1. Consider using a consistent success response type.
2. 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-get requires X-ACTUAL-TOKEN
• users-get requires X-ACTUAL-TOKEN
• access related endpoints require X-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:

1. Token management
2. Encryption key handling
3. 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:

1. 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'))

2. Encryption:

   • Encryption keys are managed through a dedicated encryption module
   • Key validation is performed before operations (hasKey checks)
   • Proper error handling for encryption failures

3. Access Controls:

   • File ownership is properly tracked
   • User access is verified through usersWithAccess checks
   • 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: ⚠️ Potential issue

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: ⚠️ Potential issue

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.

[coderabbitai]

🛠️ 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.

[coderabbitai]

🛠️ 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.

Suggested change

	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;
	}

[coderabbitai]

⚠️ Potential issue

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.

Suggested change

	'user-delete-all': (
	ids: string[],
	) => Promise<{ someDeletionsFailed: boolean; ids?: number[] }>;
	'user-delete-all': (
	ids: string[],
	) => Promise<{ someDeletionsFailed: boolean; ids?: string[] }>;

[coderabbitai]

⚠️ Potential issue

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.

Suggested change

	'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[] }>;

[coderabbitai]

🛠️ 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.

[coderabbitai]

🛠️ 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.

Suggested change

	<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>

[coderabbitai]

⚠️ Potential issue

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.

[coderabbitai]

⚠️ Potential issue

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.

Suggested change

	borderTop: isModal && '1px solid ' + theme.pillBorder,
	borderTop: isModal ? '1px solid ' + theme.pillBorder : undefined,

[coderabbitai]

🛠️ 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.

[coderabbitai]

🛠️ 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 password and return_url as string types.
• Set loginMethod to 'openid' explicitly in the second case, and consider making it required to clarify intent.

[coderabbitai]

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 isLocalFile type 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 getOwnerDisplayName function 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]);

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 30afb0e and ae2506f.

📒 Files selected for processing (1)

• packages/desktop-client/src/components/manager/BudgetList.tsx (13 hunks)

[coderabbitai]

🛠️ 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}
  />
)}

[coderabbitai]

🛠️ Refactor suggestion

Add error boundary and optimize user sorting.

The component could benefit from error handling and performance optimizations:

1. Wrap the component in an error boundary to gracefully handle rendering errors
2. 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]);

[coderabbitai]

🛠️ Refactor suggestion

Enhance tooltip accessibility.

The tooltip implementation needs accessibility improvements:

1. Add aria-label to the trigger icon
2. Add keyboard navigation support
3. 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.