Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refacto(invite|signin): remove unused code + fix signin on invite page. #9745

Merged

Conversation

AMoreaux
Copy link
Contributor

@AMoreaux AMoreaux commented Jan 20, 2025

  • Replace window.location.replace by useRedirect hook.
  • Remove unused code: switchWorkspace, addUserByInviteHash...
  • Refacto Invite component.
  • Fix signin on invite modal.

@AMoreaux AMoreaux self-assigned this Jan 20, 2025
Eliminates the SwitchWorkspace functionality, including its service, DTO, and related resolver logic. This cleanup removes unused code and simplifies the auth module structure.
Removed the `switchWorkspace` mutation and its associated arguments from the GraphQL schema. This code was unused and its removal helps in cleaning up the generated types and mutations.
Introduced the `timeoutSeconds` field in various types and mutations related to ServerlessFunction. This allows setting a timeout for serverless functions, providing more configurability for function execution limits.
Removed obsolete mutations and related logic for adding users to workspaces via invite tokens or hashes. Replaced old `window.location.replace` calls with the `redirect` hook across affected files to improve maintainability and consistency.
…-signin-on-invite-page' into chore/remove-unused-code-and-fix-signin-on-invite-page
@AMoreaux AMoreaux requested a review from Weiko January 20, 2025 16:51
Removed unused `workspacePublicData` from `useSignInUp` hook and cleaned up a debug log in SettingsDomain. These changes improve code readability and maintainability by eliminating unnecessary code.
@AMoreaux AMoreaux marked this pull request as ready for review January 20, 2025 17:00
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR significantly refactors authentication and workspace management, focusing on cleaning up unused code and improving the sign-in flow for invited users.

  • Removes switchWorkspace mutation and related services/tests, replacing direct workspace switching with useRedirect hook for navigation
  • Removes unused workspace invitation mutations (addUserToWorkspace, addUserToWorkspaceByInviteToken) and consolidates invite handling in sign-in flow
  • Fixes password validation in validatePassword method by removing redundant password hashing
  • Simplifies Invite component by removing complex workspace switching logic and relying on SignInUpWorkspaceScopeForm
  • Makes subdomain parameter required in buildWorkspaceUrl function and adds it to workspace invite hash query

28 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -6,13 +6,13 @@ export const useBuildWorkspaceUrl = () => {
const domainConfiguration = useRecoilValue(domainConfigurationState);

const buildWorkspaceUrl = (
subdomain?: string,
subdomain: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Making subdomain required could break existing code that passes undefined. Consider keeping it optional or documenting the breaking change.

Comment on lines +15 to 17
if (subdomain.length !== 0) {
url.hostname = `${subdomain}.${domainConfiguration.frontDomain}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider handling invalid/malformed subdomains to prevent invalid URLs

Comment on lines +69 to 71
const handleChange = async (workspace: Workspaces[0]) => {
redirectToWorkspaceDomain(workspace.subdomain);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: handleChange is now async but doesn't await the redirect call

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing this hook could break workspace switching functionality in other components that depend on it, particularly MultiWorkspaceDropdownButton.tsx which is also modified in this PR. Verify that all consumers of this hook have been properly updated to use alternative methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The removed code handled error cases and showed error messages to users. Ensure the new implementation has proper error handling and user feedback mechanisms.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing this DTO could break GraphQL operations that depend on it. Verify that all references to SwitchWorkspaceInput in auth.resolver.ts and other resolvers have been properly updated or removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Removing this service eliminates critical workspace access validation. Verify this logic exists elsewhere to prevent unauthorized workspace access.

Cleaned up unused `useRecoilValue` and related state to simplify and optimize imports in `useSignInUp`. This enhances maintainability by reducing unnecessary code.

export const workspacesState = createState<Workspaces[] | null>({
export const workspacesState = createState<Workspaces | null>({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove | null

Introduce `publicFeatureFlags` and `PublicFeatureFlag` types to GraphQL schema. Added `updateLabPublicFeatureFlag` mutation and its input type, while removing `addUserToWorkspace` and `addUserToWorkspaceByInviteToken` mutations. Also adjusted enum casing for `WorkspaceActivationStatus` for consistency.
Changed the default value of workspacesState from null to an empty array to ensure proper handling of workspaces initialization. This prevents potential null-related errors in dependent components.
fix(apollo): handle empty workspaces state correctly

Replaced null with an empty array for workspaces to ensure consistent state handling. Updated the logic that checks for multiple workspaces to align with the new change. This prevents potential errors when working with workspace data.
```
@charlesBochet charlesBochet merged commit 34afd73 into main Jan 21, 2025
47 checks passed
@charlesBochet charlesBochet deleted the chore/remove-unused-code-and-fix-signin-on-invite-page branch January 21, 2025 15:33
Copy link
Contributor

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-a993bb2e.json

Generated by 🚫 dangerJS against c74a19f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants