View as selector and tests

[ericokuma]

This PR addresses the follow-up feedback for the "View As" feature (APP-765) by improving code structure, efficiency, and test coverage.

Key Changes:

• Shared View As Project Permissions Selector: Extracted the GetDeploymentCredentials → GetProjectWithBearerToken → effective permissions query chain into a new shared selector (useViewAsProjectPermissions). This deduplicates logic previously duplicated in ProjectLayout and TopNavigationBar, making the code more maintainable and reducing component complexity.
• E2E Tests for View As: Added comprehensive Playwright tests to cover critical scenarios for the "View As" feature, including UI visibility, state persistence across refreshes and navigation, and state clearing. This significantly improves test coverage for a security-adjacent feature. (Note: Effective permissions assertions are commented out pending viewerPage fixture setup).
• Optimized initForProject: Implemented an early return in initForProject to prevent redundant sessionStorage reads when the project scope has not changed, improving efficiency.

Checklist:

• Covered by tests
• Ran it and it works as intended
• Reviewed the diff before requesting a review
• Checked for unhandled edge cases
• Linked the issues it closes
• Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
• Intend to cherry-pick into the release branch
• I'm proud of this work!

---

Linear Issue: APP-765

  

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[ericpgreen2]

Issues

1. E2E tests don't validate the core behavior of View As (High)

The test suite tests UI mechanics (popover opens, chip renders, sessionStorage persists) but not the actual purpose of View As — that an admin sees restricted permissions when impersonating a less-privileged user.

The e2e environment only has one user (the admin), so every test selects the admin to "view as" themselves. The comment on line 72 makes this explicit: // Select the first user in the list (the admin user itself). Since the effective permissions are identical to the admin's real permissions, the query chain (GetDeploymentCredentials → GetProjectWithBearerToken → effective permissions) is never exercised in a way that produces observable differences.

The skipped test ("Effective permissions update correctly when viewing as a viewer") is the one that would cover this, but it requires a second user with viewer-level permissions to be provisioned in setup.ts. Without it, the tests give confidence that the UI plumbing works but not that security policy enforcement actually functions through View As.

Consider either:

• Provisioning a viewer user in the e2e setup and enabling the skipped test, or
• Adding a unit/integration test that asserts the effective permissions reflect the mocked user's restrictions (not the admin's) when View As is active.

2. The shared selectors are thin wrappers — consider a compound hook instead (Medium)

view-as-project-permissions.ts extracts three functions, but both consumers (+layout.svelte:138-158, TopNavigationBar.svelte:80-99) still repeat the same multi-step chain: read viewAsUserStore → create credentials query → pipe access token into project query → derive effective permissions. The wrappers move individual steps behind functions without actually reducing the duplication.

Worse, the two consumers compute effective permissions differently — the layout uses computeEffectivePermissions (switches on the whole object), while the navbar uses inline ?? fallback per field (falls through to the admin's permission if a mocked field is undefined).

A single compound hook that encapsulates the entire chain would eliminate both problems:

// Returns the mocked user's project permissions (or undefined when View As is inactive)
export function createViewAsPermissions(org: string, project: string) {
  const mockedUserId = get(viewAsUserStore)?.id; // or accept as param
  const credentialsQuery = createAdminServiceGetDeploymentCredentials(...);
  const projectQuery = createAdminServiceGetProjectWithBearerToken(...);
  // return derived store with projectPermissions
}

Both consumers would call one thing and get back the mocked permissions, then merge with their actual permissions in a single consistent place.

3. Silent test skip inside "Admin can change the viewed user" (Low)

view-as.spec.ts:247-259: The if (userCount > 1) block means the core assertion (selecting a different user) is silently skipped when only one user exists — which is always the case in the current e2e environment. The test passes without testing its stated purpose. Consider asserting userCount > 1 so CI fails loudly if the fixture is misconfigured, or skip the test with a message.

Minor

• test.describe.configure({ mode: "serial" }) may not be needed — each test navigates fresh and doesn't depend on prior test state. Parallel mode would be faster in CI.

---

Developed in collaboration with Claude Code

[ericokuma]

The compound hook addresses the duplication well, and the test fixes are clean. One issue in the new implementation:

Subscription leak in createViewAsState (Medium)

Inside the credentialsQuery.subscribe callback (line 67), a new projectQuery subscription is created every time the credentials store emits — refetches, cache updates, background polling, etc. The return unsubProject on line 95 is the return value of the .subscribe callback, which Svelte ignores (only derived's outer function return is treated as a cleanup). So previous project subscriptions are never torn down and accumulate over the lifetime of the component.

Each leaked subscription calls set() on the derived store, causing redundant updates. TanStack Query deduplication prevents duplicate network requests, but the subscription count still grows unboundedly.

One way to fix it:

const unsubCredentials = credentialsQuery.subscribe(($credentialsQuery) => {
  const accessToken = $credentialsQuery.data?.accessToken;

  if (!accessToken) {
    unsubProject?.();
    unsubProject = undefined;
    set({ ... });
    return;
  }

  // Only re-subscribe if the token actually changed
  if (accessToken === prevAccessToken) return;
  prevAccessToken = accessToken;

  unsubProject?.();
  const projectQuery = createAdminServiceGetProjectWithBearerToken(...);
  unsubProject = projectQuery.subscribe(($projectQuery) => {
    set({ ... });
  });
});

return () => {
  unsubProject?.();
  unsubCredentials();
};

The key ideas: (1) track and clean up the inner subscription, (2) skip re-subscribing when the token hasn't changed, (3) clean up both subscriptions in the outer cleanup.

---

Developed in collaboration with Claude Code