Skip to content

Add missing loading to /[tenantId] pages#3032

Open
dimaMachina wants to merge 1 commit intomainfrom
dimaMachina-patch-5
Open

Add missing loading to /[tenantId] pages#3032
dimaMachina wants to merge 1 commit intomainfrom
dimaMachina-patch-5

Conversation

@dimaMachina
Copy link
Copy Markdown
Collaborator

No description provided.

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 6, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agents-api Ready Ready Preview, Comment Apr 6, 2026 6:26pm
agents-manage-ui Ready Ready Preview, Comment Apr 6, 2026 6:26pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
agents-docs Skipped Skipped Apr 6, 2026 6:26pm

Request Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 6, 2026

⚠️ No Changeset found

Latest commit: 24537d5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pullfrog
Copy link
Copy Markdown
Contributor

pullfrog bot commented Apr 6, 2026

TL;DR — Adds a loading skeleton to the /[tenantId] route segment by re-exporting the existing [projectId]/loading.tsx component. Without this, navigating to the tenant root showed no loading indicator while the page data resolved.

Key changes

  • Add loading.ts for the [tenantId] route — re-exports the project-level loading skeleton so the tenant root page displays the same shimmer UI during navigation.

Summary | 1 file | 1 commit | base: maindimaMachina-patch-5

The new file is a single re-export (export { default } from './projects/[projectId]/loading'), reusing the grid skeleton already defined at the [projectId] level. This ensures Next.js renders an instant loading state for the [tenantId] segment without duplicating any UI code.

Before: No loading.ts at the [tenantId] level — navigating to the tenant root showed a blank page or stale content while data loaded.
After: The existing skeleton grid from [projectId]/loading.tsx is re-exported, giving immediate visual feedback during route transitions.

agents-manage-ui/src/app/[tenantId]/loading.ts

Pullfrog  | View workflow run | Triggered by Pullfrog | Using Claude Opus𝕏

Copy link
Copy Markdown
Contributor

@pullfrog pullfrog bot left a comment

Choose a reason for hiding this comment

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

Clean, correct change. The [tenantId]/page.tsx performs an async redirect via redirectToProject(), and without a loading.ts at this segment, users see no loading indicator during that transition. This re-export follows the exact same pattern already established in [tenantId]/projects/loading.tsx.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Copy link
Copy Markdown
Contributor

@claude claude 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 Review Summary

(0) Total Issues | Risk: Low

This PR adds a missing loading state to the /[tenantId] route segment. When users navigate to /{tenantId}, the page performs an async operation (fetchProjects) before redirecting. Without a loading state, users would see a blank page during this fetch.

Analysis

What this PR does:

  • Adds loading.ts at [tenantId] level that re-exports the skeleton component from [projectId]/loading.tsx
  • Provides visual feedback during the async redirect operation in [tenantId]/page.tsx

Correctness:

  • ✅ Follows the established pattern already used in [tenantId]/projects/loading.tsx
  • ✅ Re-uses the existing skeleton component rather than duplicating code
  • ✅ Correctly positioned to catch the async operation in [tenantId]/page.tsx

Minor observation (non-blocking):
The similar file at [tenantId]/projects/loading.tsx uses .tsx extension while this PR uses .ts. Both work correctly for re-exports without JSX, but using .tsx would be more consistent with the existing sibling file. This is purely stylistic and not worth blocking on.


✅ APPROVE

Summary: Clean, minimal change that improves UX by showing a loading skeleton during the redirect operation. The implementation follows established patterns in the codebase.

@itoqa
Copy link
Copy Markdown

itoqa bot commented Apr 6, 2026

Ito Test Report ✅

11 test cases ran. 3 additional findings, 8 passed.

Overall, 11 test cases ran with 8 passing and 3 failing, confirming that tenant-root navigation is generally stable (including redirects to projects, loading-skeleton behavior, back/forward and refresh recovery, rapid interaction, and multi-tab consistency) with no persistent loading loops in the validated happy paths. The key issues were a high-severity auth boundary flaw where a forced-logout cookie is cleared/ignored if a session cookie exists (allowing protected tenant routing), plus two medium-severity routing defects where unauthorized or nonexistent tenants are incorrectly redirected to /{tenantId}/projects and encoded traversal-style tenant input (e.g., /..%2Fdefault) can trigger an in-app 500 instead of safe no-access/not-found handling.

✅ Passed (8)
Category Summary Screenshot
Adversarial Rapid /default navigation and back/forward stress recovered to a stable /default/projects UI without a persistent 500 state. ADV-1
Adversarial Three concurrent tabs consistently resolved from /default to valid tenant destination without loading or auth bounce loops. ADV-3
Edge Back/forward stress repeatedly settled at /default/projects; observed hook-order error aligns with Fast Refresh, not redirect logic. EDGE-1
Edge Interrupted reload cycles consistently recovered to /default/projects; no redirect loop or stuck loading state observed. EDGE-2
Happy-path From /default, app resolved to allowed fallback /default/projects (no seeded project target), rendered stable destination UI, and avoided blank/dead loading state. ROUTE-1
Happy-path /empty-tenant redirected cleanly to /empty-tenant/projects and rendered the empty-state CTA without loading loops. ROUTE-2
Screen Loading skeleton rendered during delayed tenant-root navigation, and mobile loading state showed no horizontal overflow before redirect settled. SCREEN-1
Screen After navigating to /default and allowing redirect completion, loading fallback was no longer present and destination controls remained interactive. SCREEN-2
ℹ️ Additional Findings (3)

These findings are unrelated to the current changes but were observed during testing.

Category Summary Screenshot
Adversarial 🟠 Encoded traversal tenant path /..%2Fdefault resolves to an in-app 500 error state instead of a controlled safe route outcome. ADV-2
Adversarial ⚠️ With dev-logged-out set, requests with an existing session still proceed to tenant project routing instead of redirecting to login. ADV-4
Happy-path 🟠 Unauthorized tenant errors are broadly caught and redirected to /${tenantId}/projects instead of a safe auth/no-access/not-found route. ROUTE-3
🟠 Encoded traversal tenant path triggers 500 instead of safe handling
  • What failed: The app renders a Server Error 500 state for encoded traversal-style tenant input, but expected behavior is controlled safe handling (for example no-access/not-found) without a backend 500.
  • Impact: Malformed tenant paths can force a server-error experience instead of safe rejection, reducing reliability and exposing an avoidable error surface. Users or scanners hitting crafted URLs can repeatedly trigger this failure mode.
  • Steps to reproduce:
    1. Open the manage UI in a local authenticated session.
    2. Navigate directly to http://localhost:3000/..%2Fdefault.
    3. Wait for the route to resolve to /..%2Fdefault/projects.
    4. Observe that the terminal state is an in-app 500 error instead of controlled no-access/not-found handling.
  • Stub / mock context: The same local deterministic auth setup and bypassed tenant/project access checks were active while running this scenario, but the malformed-tenant 500 is still supported by production source paths that accept and propagate unsanitized tenant IDs into ref resolution.
  • Code analysis: I traced tenant path handling across schema validation and ref resolution. Tenant IDs are accepted as unconstrained strings, then used to construct Dolt branch names, and tenant-scoped requests resolve refs from those constructed names; this allows malformed tenant strings to propagate into backend branch resolution and fail as 500s.
  • Why this is likely a bug: Production code accepts unsanitized tenantId path input and uses it directly in tenant ref construction/resolution, creating a plausible deterministic path to backend branch-resolution failures and 500 responses.

Relevant code:

packages/agents-core/src/validation/schemas.ts (lines 2981-2988)

const TenantId = z.string().openapi('TenantIdPathParam', {
  param: {
    name: 'tenantId',
    in: 'path',
  },
  description: 'Tenant identifier',
  example: 'tenant_123',
});

packages/agents-core/src/dolt/ref-helpers.ts (lines 22-24)

export const getTenantScopedRef = (tenantId: string, ref: string): string => {
  return `${tenantId}_${ref}`;
};

agents-api/src/middleware/ref.ts (lines 182-186)

if (projectId) {
  resolvedRef = await resolveProjectRef(db, c, tenantId, projectId, ref);
} else {
  resolvedRef = await resolveTenantRef(db, tenantId, ref);
}
⚠️ Session-state abuse attempt via forced logout cookie
  • What failed: The app continues into tenant/project routing and exposes authenticated destination pages instead of forcing a login redirect while the logout marker is active.
  • Impact: Users with an active session cookie can continue into tenant pages despite a forced-logout marker. This weakens logout enforcement and can expose tenant context when re-authentication should be required.
  • Steps to reproduce:
    1. Sign in as an authorized user and ensure a valid session cookie exists.
    2. Set dev-logged-out=true for localhost.
    3. Navigate to /default and observe whether the app redirects to login or continues into tenant project routes.
  • Stub / mock context: The run intentionally set a forced-logout browser cookie to validate auth-boundary handling, then cleared it after verification; no route interception or test-only code patching was used for this case.
  • Code analysis: I reviewed tenant-root request handling in agents-manage-ui/src/proxy.ts plus downstream tenant redirect flow in agents-manage-ui/src/app/[tenantId]/page.tsx and agents-manage-ui/src/lib/utils/project-redirect.ts; the middleware deletes dev-logged-out and allows the request when any session cookie exists, which directly enables the observed route exposure.
  • Why this is likely a bug: The forced-logout cookie is treated as authoritative only in the no-session dev path, but explicitly ignored and deleted when a session cookie is present, creating an auth-state contradiction that permits protected tenant navigation.

Relevant code:

agents-manage-ui/src/proxy.ts (lines 139-147)

const hasSession = request.cookies.getAll().some((c) => isSessionCookie(c.name));

if (hasSession) {
  if (request.cookies.has(LOGGED_OUT_COOKIE)) {
    const response = NextResponse.next();
    response.cookies.delete(LOGGED_OUT_COOKIE);
    return applySecurityHeaders(response);
  }
  return applySecurityHeaders(NextResponse.next());
}

agents-manage-ui/src/proxy.ts (lines 150-153)

if (isDevelopment()) {
  if (request.cookies.has(LOGGED_OUT_COOKIE)) {
    return applySecurityHeaders(redirectToLogin(request));
  }

agents-manage-ui/src/lib/utils/project-redirect.ts (lines 16-22)

const result = await fetchProjects(tenantId);

if (result.data && result.data.length > 0) {
  // Redirect to the first project in the list
  const firstProject = result.data[0];
  redirect(`/${tenantId}/projects/${firstProject.projectId}/${targetPath}`);
}
🟠 Unauthorized tenant root redirects to projects fallback instead of safe no-access handling
  • What failed: The app routes unauthorized/nonexistent tenant access to /${tenantId}/projects instead of a safe login/no-access/not-found destination, so authorization failures are treated like normal project-list fallbacks.
  • Impact: Unauthorized-tenant access is handled with the wrong destination and can mislead users into an invalid tenant context. This weakens expected authz boundary behavior and safe error routing.
  • Steps to reproduce:
    1. Authenticate as a user who does not have access to a target tenant slug.
    2. Navigate directly to /nonexistent-or-unauthorized-tenant (or another unauthorized tenant id).
    3. Observe that the app resolves to //projects instead of sending the user to a safe login/no-access/not-found route.
  • Stub / mock context: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code analysis: src/app/[tenantId]/page.tsx always calls redirectToProject, and src/lib/utils/project-redirect.ts catches all non-redirect errors and force-redirects to tenant projects. src/lib/api/api-config.ts throws ApiError for non-2xx responses, so auth/not-found API failures are swallowed into that fallback path instead of being surfaced to a safe boundary route.
  • Why this is likely a bug: The production redirect helper conflates authorization/not-found API errors with empty-project success fallback, creating an incorrect route outcome by code path design.

Relevant code:

agents-manage-ui/src/app/[tenantId]/page.tsx (lines 3-6)

async function TenantPage({ params }: PageProps<'/[tenantId]'>) {
  const { tenantId } = await params;
  await redirectToProject(tenantId, 'agents');
}

agents-manage-ui/src/lib/utils/project-redirect.ts (lines 15-38)

try {
  const result = await fetchProjects(tenantId);

  if (result.data && result.data.length > 0) {
    const firstProject = result.data[0];
    redirect(`/${tenantId}/projects/${firstProject.projectId}/${targetPath}`);
  } else {
    redirect(`/${tenantId}/projects`);
  }
} catch (error) {
  if (
    error instanceof Error &&
    (error.message === 'NEXT_REDIRECT' || (error as any).digest?.startsWith('NEXT_REDIRECT'))
  ) {
    throw error;
  }
  redirect(`/${tenantId}/projects`);
}

agents-manage-ui/src/lib/api/api-config.ts (lines 81-88)

if (!response.ok) {
  let errorData: any;
  try {
    const text = await response.text();
    errorData = text ? JSON.parse(text) : null;
  } catch {
    errorData = null;
  }

Commit: 24537d5

View Full Run


Tell us how we did: Give Ito Feedback

@github-actions
Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 7 days if no further activity occurs.

If this PR is still relevant:

  • Rebase it on the latest main branch
  • Add a comment explaining its current status
  • Request a review if it's ready

Thank you for your contributions!

@github-actions github-actions bot added the stale label Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant