Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Trans } from "@lingui/react/macro";
import { Gear, House, Plus, PushPin } from "@phosphor-icons/react";
import { useMemo } from "react";
import { useParams } from "react-router";
import { isAdminRole } from "@/lib/roles";
import { useWorkspace } from "@/hooks/useWorkspace";
import { useWorkspaceProjects } from "@/hooks/useWorkspaceProjects";
import { BackButton } from "../../primitives/BackButton";
Expand Down Expand Up @@ -29,6 +30,13 @@ export const WorkspaceHomeView = () => {
const backLabel = workspace?.org_name ?? "Home";
const isExternal = workspace?.role === "external";
const canCreateProject = !isExternal;
const isAdmin = isAdminRole(workspace?.role);
const isBilling = workspace?.role === "billing";
const settingsPath = isAdmin
? `${base}/settings/general`
: isBilling
? `${base}/settings/billing`
: `${base}/settings/members`;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

return (
<nav className="flex h-full flex-col gap-0.5 p-1.5">
Expand Down Expand Up @@ -71,7 +79,7 @@ export const WorkspaceHomeView = () => {
<>
<div className="mt-auto" />
<NavItem
to={`${base}/settings/general`}
to={settingsPath}
label={<Trans>Settings</Trans>}
icon={Gear}
pushes
Expand Down
Original file line number Diff line number Diff line change
@@ -1,26 +1,34 @@
import { Trans } from "@lingui/react/macro";
import { CreditCard, Gear, Users, Warning } from "@phosphor-icons/react";
import { useParams } from "react-router";
import { useWorkspace } from "@/hooks/useWorkspace";
import { isAdminRole } from "@/lib/roles";
import { BackButton } from "../../primitives/BackButton";
import { NavItem } from "../../primitives/NavItem";

export const WorkspaceSettingsView = () => {
const { workspaceId } = useParams<{ workspaceId: string }>();
const { workspaces } = useWorkspace();

if (!workspaceId) return null;
const base = `/w/${workspaceId}/settings`;

const workspace = workspaces.find((w) => w.id === workspaceId);
const isAdmin = isAdminRole(workspace?.role);

return (
<nav className="flex h-full flex-col gap-0.5 p-1.5">
<BackButton
to={`/w/${workspaceId}/home`}
label={<Trans>Settings</Trans>}
/>
<NavItem
to={`${base}/general`}
label={<Trans>General</Trans>}
icon={Gear}
/>
{isAdmin && (
<NavItem
to={`${base}/general`}
label={<Trans>General</Trans>}
icon={Gear}
/>
)}
<NavItem
to={`${base}/members`}
label={<Trans>Members</Trans>}
Expand All @@ -31,11 +39,13 @@ export const WorkspaceSettingsView = () => {
label={<Trans>Usage and billing</Trans>}
icon={CreditCard}
/>
<NavItem
to={`${base}/danger`}
label={<Trans>Danger zone</Trans>}
icon={Warning}
/>
{isAdmin && (
<NavItem
to={`${base}/danger`}
label={<Trans>Danger zone</Trans>}
icon={Warning}
/>
)}
</nav>
);
};
11 changes: 7 additions & 4 deletions echo/frontend/src/routes/workspaces/WorkspaceSettingsRoute.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -464,8 +464,13 @@ export const WorkspaceSettingsRoute = () => {
// a second time with the role-correct default if the URL is bare.
const callerCanManage =
settings?.my_policies?.includes("member:manage") ?? false;
const defaultTab: TabValue =
settings?.my_role === "billing" && !callerCanManage ? "billing" : "general";
const canEditSettings =
settings?.my_policies?.includes("settings:manage") ?? false;
const defaultTab: TabValue = canEditSettings
? "general"
: settings?.my_role === "billing"
? "billing"
: "members";
Comment on lines +467 to +473
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Defer default-tab redirect until settings are loaded.

Line 469 computes a fallback before settings exists, and Line 485 then locks in a valid but wrong segment. Billing/admin users can land on members from /settings and never get corrected.

Suggested fix
-	const defaultTab: TabValue = canEditSettings
-		? "general"
-		: settings?.my_role === "billing"
-			? "billing"
-			: "members";
+	const defaultTab: TabValue | null = settings
+		? canEditSettings
+			? "general"
+			: settings.my_role === "billing"
+				? "billing"
+				: "members"
+		: null;
 	const activeTab: TabValue = segmentIsValid
 		? (segment as TabValue)
-		: defaultTab;
+		: (defaultTab ?? "members");
@@
 	useEffect(() => {
 		if (!workspaceId) return;
-		if (segment !== activeTab) {
-			navigate(`/w/${workspaceId}/settings/${activeTab}${urlSearch}`, {
+		if (!segmentIsValid) {
+			if (!defaultTab) return;
+			navigate(`/w/${workspaceId}/settings/${defaultTab}${urlSearch}`, {
 				replace: true,
 			});
 		}
-	}, [workspaceId, segment, activeTab, navigate, urlSearch]);
+	}, [workspaceId, segmentIsValid, defaultTab, navigate, urlSearch]);

Also applies to: 483-490

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@echo/frontend/src/routes/workspaces/WorkspaceSettingsRoute.tsx` around lines
467 - 473, The current default-tab computation and redirect run before
`settings` is loaded causing wrong tab lock-in; update `WorkspaceSettingsRoute`
to defer computing `canEditSettings`/`defaultTab` and performing the redirect
until `settings` is non-null (or a loading flag is false). Specifically, guard
or delay the logic that reads `settings?.my_policies` and `settings?.my_role`
(the `canEditSettings` and `defaultTab` calculations) and the subsequent
redirect code block (the code around lines handling tab selection/redirect) so
it only executes when `settings` is available, leaving the route idle or showing
a loader until then. Ensure `defaultTab: TabValue` is derived from `settings`
inside that guarded branch to avoid landing billing/admin users on `members`
prematurely.

const activeTab: TabValue = segmentIsValid
? (segment as TabValue)
: defaultTab;
Expand Down Expand Up @@ -599,8 +604,6 @@ export const WorkspaceSettingsRoute = () => {

const canManage = callerCanManage;
const myAppUserId = meV2?.id ?? null;
const canEditSettings =
settings.my_policies?.includes("settings:manage") ?? false;
const seesFinancials =
settings.my_policies?.includes("workspace:view_invoices") ?? false;

Expand Down
Loading