fix(website): correct link to account edit if Keycloak has subpath

[chaoran-chen]

resolves #5796

This fixes a bug that the link to the account edit page on Keycloak was wrong if the Keycloak instance is not at the root path but under a different pathname.

PR Checklist

• [ ] All necessary documentation has been adapted.
• [ ] The implemented feature is covered by appropriate, automated tests.
• Any manual testing that has been done is documented (i.e. what exactly was tested?)
  • I tested it on a VM where Keycloak is at /auth
  • Also checked that the link works correctly in the preview.

🚀 Preview: https://fix-edit-account-link.loculus.org

[claude]

Code Review

Thank you for fixing this bug! The approach of extracting the base URL from the authorization URL is sound. Here are my findings:

Code Quality & Best Practices ✅

Positive aspects:

• Good refactoring: Consolidating the account page URL logic into getAuthUrl.ts alongside related authentication URL functions makes sense
• Cleaner code: Removing the dependency on passing keycloakClient around simplifies the call sites
• Proper use of existing helper: Leverages getAuthBaseUrl() which already handles the pathname extraction logic

Issues Found 🔍

1. Critical: Leftover debug console.log (website/src/components/User/UserPage.astro:22)

There is a console.log statement on line 22 that should be removed before merging

2. Missing test coverage ⚠️

The old implementation had unit tests (urlForKeycloakAccountPage.spec.ts) that were removed. The new getUrlForKeycloakAccountPage() function has no test coverage. Consider:

• Adding tests to verify the function constructs the correct URL with subpaths
• Testing edge cases (e.g., when getAuthBaseUrl() returns null)

3. Potential null reference bug 🐛

In getAuthUrl.ts:35, if getAuthBaseUrl() returns null (when '/realms' is not found in the auth URL), the function will concatenate null with the realm path, resulting in a malformed URL string.

Recommendation: Add null checking to handle this edge case gracefully.

4. Unnecessary .ts extension in import (minor)

The import statements include .ts extension which is inconsistent with other imports in the codebase. Consider removing for consistency.

Performance Considerations ✅

No concerns. The function is async and used in server-side rendering contexts where the additional call is negligible.

Security Considerations ✅

No security issues identified. The URL construction is safe and does not introduce injection vulnerabilities.

Testing

As noted in the PR description, manual testing was done on a VM with Keycloak at /auth. This is good, but automated tests would help prevent regressions.

Summary

The core fix is correct and addresses the issue effectively. The main items to address are:

1. Remove the debug console.log statement (blocking)
2. Add null checking to prevent runtime errors (recommended)
3. Consider adding unit tests to replace the removed test coverage (recommended)

Overall this is a solid fix that simplifies the code architecture. Nice work!

[claude]

Code Review

Summary

This PR fixes a bug where the Keycloak account edit page link was incorrect when Keycloak is hosted at a subpath (e.g., /auth) rather than at the root path. The fix correctly preserves the pathname by reusing the existing getAuthBaseUrl() helper function.

Positive Aspects

✅ Root Cause Fix: The solution addresses the core issue by reusing getAuthBaseUrl(), which correctly extracts the base URL including any pathname up to /realms. This is more robust than manually constructing URLs from url.protocol and url.host which drops the pathname.

✅ Code Consolidation: Removing the separate urlForKeycloakAccountPage.ts file and consolidating the logic into getAuthUrl.ts improves maintainability. All auth URL construction is now in one place.

✅ Consistency: The new implementation aligns with how the codebase handles Keycloak URLs elsewhere (see loculus-info/index.ts:18 which also uses getAuthBaseUrl()).

✅ Manual Testing: The author tested on a VM with Keycloak at /auth, which directly validates the fix.

Issues & Concerns

🔴 Missing Test Coverage (Important)

The PR removes the unit tests (urlForKeycloakAccountPage.spec.ts) without replacing them. While manual testing was performed, automated tests are important for:

• Preventing regressions
• Documenting expected behavior
• Ensuring the fix works for various subpath scenarios

Recommendation: Add unit tests for getUrlForKeycloakAccountPage() in a new getAuthUrl.spec.ts file. Test cases should cover:

describe('getUrlForKeycloakAccountPage', () => {
  test('handles Keycloak at root path', async () => {
    // Mock getAuthBaseUrl to return 'https://kc.example.com'
    expect(await getUrlForKeycloakAccountPage()).toBe('https://kc.example.com/realms/loculus/account');
  });
  
  test('handles Keycloak at subpath', async () => {
    // Mock getAuthBaseUrl to return 'https://kc.example.com/auth'
    expect(await getUrlForKeycloakAccountPage()).toBe('https://kc.example.com/auth/realms/loculus/account');
  });
});

🟡 Missing Null Check (Medium Priority)

In getAuthUrl.ts:36, the new function doesn't handle the case where getAuthBaseUrl() returns null (which can happen if authUrl.indexOf('/realms') returns -1 on line 28).

Current code:

export const getUrlForKeycloakAccountPage = async () => {
    const baseUrl = await getAuthBaseUrl();
    return `${baseUrl}${realmPath}/account`;  // baseUrl could be null
};

Recommendation: Add null handling:

export const getUrlForKeycloakAccountPage = async () => {
    const baseUrl = await getAuthBaseUrl();
    if (baseUrl === null) {
        return null; // or throw an error, depending on desired behavior
    }
    return `${baseUrl}${realmPath}/account`;
};

Then update the calling code to handle the null case appropriately (or document why it can't happen in practice).

🟡 File Extension Inconsistency (Minor)

In the imports, .ts extension is included inconsistently:

• Line 2 in UserPage.astro: from '../../utils/getAuthUrl.ts' (with .ts)
• Line 5 in seqsets/index.astro: from '../../utils/getAuthUrl.ts' (with .ts)

While this works, the codebase generally omits .ts extensions in imports (as seen in the removed files and other imports). Consider:

import { getUrlForKeycloakAccountPage } from '../../utils/getAuthUrl';

Security & Performance

✅ No security concerns identified
✅ No performance impact (same async pattern as before)

Suggested Checklist (from website/AGENTS.md)

Before merging, please ensure:

• Run CI=1 npm run test (after adding tests)
• Run npm run check-types
• Run npm run format

Verdict

The core fix is solid and correct 👍. However, the missing test coverage is a significant gap that should be addressed before merging to prevent future regressions. The null check is also important for robustness.

Recommendation: Add unit tests and null handling, then this is good to merge.

[corneliusroemer]

This used to be unit tested and now isn't anymore unless I missed something. It's quite a trivial change but still.

Do we maybe have an integration test for going to account page? Otherwise might be worth adding to improve coverage rather than reduce.

[theosanderson]

Fwiw the unit test looks to have been added by codex while AI coding for me, and wasn't something I actively decided we needed. I don't really think we need it (no strong feelings).

[theosanderson]

thanks (obvs if cornelius feels anything is blocking, do flag)

[chaoran-chen]

Thanks for the reviews! I'll propose a few tests in a separate PR (not doing it here because they will be more about getAuthUrl() than getUrlForKeycloakAccountPage()).