fix: prevent redirect loop when already on login page

[eliorh]

No description provided.

[claude]

Claude finished @eliorh's task —— View job

---

PR Review: Prevent Redirect Loop on Login Page

I've reviewed the changes in this PR. Here's my feedback:

Todo List

• Read and analyze the changes in src/client.ts
• Check for CLAUDE.md or similar repo guidelines
• Review for code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage

---

Summary

This PR adds a check to prevent redirect loops when users are already on the login page. The fix is simple and targeted.

Code Quality ✅

The implementation follows existing code patterns and is consistent with the codebase style. The variable naming (isOnLoginPage) is clear and descriptive.

Potential Issues 🤔

1. Hardcoded Login Path: The code assumes the login page is always at /login. This might not be flexible enough if:

   • The app uses a different login path
   • The app supports multiple login pages (e.g., /admin/login, /user/login)
   • The login path is configurable

   Consider making this configurable through the CreateClientConfig:

   const loginPath = config.loginPath || '/login';
   const isOnLoginPage = 
     typeof window !== "undefined" && window.location.pathname === loginPath;

2. Exact Path Matching: The current implementation only checks for exact path match. It won't handle:

   • Query parameters (e.g., /login?redirect=/dashboard)
   • Hash fragments (e.g., /login#forgot-password)
   • Trailing slashes (e.g., /login/)

   Consider using pathname.startsWith('/login') or a more robust URL parsing approach.

Performance ✅

No performance concerns. The additional check is minimal and only adds a simple string comparison.

Security ✅

No security issues identified. The fix appropriately prevents redirect loops which could potentially be exploited for DoS attacks.

Test Coverage ❌

Missing Tests: The PR doesn't include tests for the new functionality. The existing test file (tests/unit/client.test.js) doesn't cover the requiresAuth redirect behavior at all.

Recommended test cases:

1. Test that no redirect occurs when on /login page with requiresAuth: true
2. Test that redirect still occurs on other pages with requiresAuth: true
3. Test edge cases like /login/, /login?param=value

Example test structure:

describe('Authentication redirect behavior', () => {
  test('should not redirect when already on login page', () => {
    // Mock window.location.pathname = '/login'
    // Create client with requiresAuth: true
    // Verify no redirect occurs
  });
  
  test('should redirect when on non-login page', () => {
    // Mock window.location.pathname = '/dashboard'
    // Create client with requiresAuth: true
    // Verify redirect occurs
  });
});

Recommendation

The fix addresses the immediate issue effectively. However, I recommend:

1. Adding comprehensive tests for this behavior
2. Consider making the login path configurable for better flexibility
3. Consider using a more robust path matching strategy

Overall, this is a good fix for a real issue. With tests added, it would be ready to merge.

[github-actions]

🚀 Package Preview Available!

---

Install this PR's preview build with npm:

npm i @base44-preview/[email protected]

Prefer not to change any import paths? Install using npm alias so your code still imports @base44/sdk:

npm i "@base44/sdk@npm:@base44-preview/[email protected]"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/[email protected]"
  }
}

• 📦 Preview Package: @base44-preview/[email protected]
• 🔗 View this commit on GitHub

---

_{Preview published to npm registry — try new features instantly!}

[claude]

Claude finished @eliorh's task —— View job

---

PR Review: Prevent Redirect Loop on Login Page

I've reviewed the changes in this PR. Here's my feedback:

---

Summary

This PR adds checks in two places to prevent redirect loops when users are already on the login page:

1. In src/client.ts: Prevents automatic redirect on client initialization when requiresAuth: true
2. In src/modules/auth.ts: Prevents redirect in the redirectToLogin() method

Code Quality ✅

The implementation is clean and follows existing code patterns. The logic is straightforward and the variable naming is clear.

Potential Issues 🤔

1. Hardcoded Login Path: Both implementations assume the login page is always at /login. This could be problematic if:

   • Apps use different login paths (e.g., /auth/login, /signin)
   • Apps have multiple login pages (e.g., /admin/login, /user/login)
   • The login path is configurable

   Consider making this configurable through CreateClientConfig:

   const loginPath = config.loginPath || '/login';
   const isOnLoginPage = window.location.pathname === loginPath;

2. Exact Path Matching: The checks use exact path matching (===), which won't handle:

   • Query parameters (e.g., /login?redirect=/dashboard)
   • Trailing slashes (e.g., /login/)
   • Hash fragments (e.g., /login#forgot-password)

   Consider using pathname.startsWith('/login') for more robust matching.

3. Duplicate Logic: The login page check is implemented in two places. Consider extracting this to a shared utility function to maintain DRY principles.

Performance ✅

No performance concerns. The additional checks are minimal and only add simple string comparisons.

Security ✅

No security issues identified. The fix appropriately prevents redirect loops which could potentially be exploited.

Test Coverage ❌

Missing Tests: The PR doesn't include tests for the new functionality.

Based on my review of the test files:

• tests/unit/client.test.js doesn't test the requiresAuth redirect behavior at all
• tests/unit/auth.test.js has a test that mocks window.location.pathname but doesn't test the new skip logic

Recommended test cases:

1. Test that no redirect occurs when on /login page with requiresAuth: true
2. Test that redirect still occurs on other pages with requiresAuth: true
3. Test the redirectToLogin() method skips when already on /login
4. Test edge cases like /login/, /login?param=value

Recommendations

1. Add comprehensive tests for both changes
2. Consider making the login path configurable for better flexibility
3. Extract the login check to a shared utility to avoid duplication
4. Use more robust path matching to handle edge cases

Conclusion

This is a good fix for a real redirect loop issue. The implementation is simple and effective, though it could benefit from being more configurable and having test coverage. With tests added and the minor improvements suggested, this would be ready to merge.

[claude]

Claude finished @eliorh's task —— View job

---

PR Review: Prevent Redirect Loop on Login Page

I've reviewed the changes in this PR. Here's my comprehensive feedback:

Todo List

• Check for CLAUDE.md or repository guidelines
• Review changes in src/client.ts
• Review changes in src/modules/auth.ts
• Assess code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Check security concerns
• Review test coverage
• Provide comprehensive feedback

---

Summary

This PR addresses a redirect loop issue when users are already on the login page by adding checks in two places:

1. src/client.ts:201-203: Prevents automatic redirect when requiresAuth: true and already on /login
2. src/modules/auth.ts:47-49: Prevents redirect in redirectToLogin() method when already on /login

Additionally, the PR includes a fix for preview environment URLs to redirect to the main app's login page.

Code Quality ✅

The implementation is clean and follows existing code patterns. The variable naming (isOnLoginPage) is clear and descriptive. The logic is straightforward and easy to understand.

Potential Bugs & Issues 🐛

1. Hardcoded Login Path: Both implementations assume the login page is always at /login. This could be problematic if:

   • Apps use different login paths (e.g., /auth/login, /signin, /account/login)
   • Apps have multiple login pages (e.g., /admin/login, /user/login)
   • The login path is configurable

   Recommendation: Consider making the login path configurable:

   const loginPath = config.loginPath || '/login';
   const isOnLoginPage = window.location.pathname === loginPath;

2. Exact Path Matching: The checks use strict equality (===), which won't handle:

   • Query parameters (e.g., /login?redirect=/dashboard)
   • Trailing slashes (e.g., /login/)
   • Hash fragments (e.g., /login#forgot-password)

   Recommendation: Use more flexible matching:

   const isOnLoginPage = window.location.pathname === '/login' || 
                        window.location.pathname === '/login/';

3. Duplicate Logic: The login page check is duplicated in two places (client.ts:202 and auth.ts:47).

   Recommendation: Extract to a shared utility function:

   function isOnLoginPage(): boolean {
     return typeof window !== "undefined" && 
            window.location.pathname === "/login";
   }

Performance Considerations ✅

No performance concerns. The additional checks are minimal - just simple string comparisons that execute in O(1) time. The impact on bundle size is negligible.

Security Concerns ✅

No security issues identified. The fix appropriately prevents redirect loops which could potentially be exploited for denial-of-service attacks. The implementation doesn't expose any sensitive information or create new attack vectors.

Test Coverage ❌

Critical Issue: The PR lacks test coverage for the new functionality.

The existing tests don't cover:

• requiresAuth redirect behavior in client.ts
• The new skip logic in redirectToLogin() when already on /login
• Preview environment URL handling

Recommended Test Cases:

1. Test that no redirect occurs when on /login with requiresAuth: true
2. Test that redirect still occurs on other pages with requiresAuth: true
3. Test the redirectToLogin() skip behavior
4. Test edge cases: /login/, /login?param=value, /login#section
5. Test preview URL redirect logic (preview--* hostnames)

Example test structure:

test('should not redirect when already on login page', () => {
  // Mock window.location.pathname = '/login'
  global.window = {
    location: { pathname: '/login' }
  };
  
  const client = createClient({
    appId: 'test-app',
    requiresAuth: true
  });
  
  // Verify no redirect occurs
  // (setTimeout callback should not call redirectToLogin)
});

Additional Comments

1. Preview URL Fix: The addition of preview environment handling (auth.ts:56-65) is a good enhancement that wasn't mentioned in the PR description. This ensures preview environments redirect to the main app's login page while preserving the return URL.

2. Async Check: The authentication check in client.ts uses setTimeout(..., 0) to avoid blocking client creation, which is a good practice.

Recommendations

1. Add comprehensive test coverage - This is the most critical improvement needed
2. Consider making the login path configurable for better flexibility
3. Use more robust path matching to handle edge cases
4. Extract duplicate logic to maintain DRY principles
5. Update PR description to mention the preview URL fix

Conclusion

This is a valid fix for a real redirect loop issue. The implementation is functional but could benefit from:

• Test coverage (critical)
• More flexible configuration options
• Better edge case handling

With tests added and the minor improvements suggested, this would be ready to merge.

[claude]

Claude finished @eliorh's task —— View job

---

PR Review: Prevent Redirect Loop on Login Page

I've reviewed the changes in this PR. Here's my comprehensive feedback:

Todo List

• Check for CLAUDE.md or repository guidelines
• Review changes in src/client.ts
• Review changes in src/modules/auth.ts
• Assess code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Check security concerns
• Review test coverage
• Provide comprehensive feedback

---

Summary

This PR addresses a redirect loop issue when users are already on the login page by adding checks in two places:

1. src/client.ts:201-210: Prevents automatic redirect when requiresAuth: true and already on /login
2. src/modules/auth.ts:47-52: Prevents redirect in redirectToLogin() method when already on /login

Additionally, the PR includes a fix for preview environment URLs to redirect to the main app's login page.

Code Quality ✅

The implementation is clean and follows existing code patterns. The variable naming (isOnLoginPage, isPreview) is clear and descriptive. The logic is straightforward and easy to understand.

Potential Bugs & Issues 🐛

1. Hardcoded Login Path: Both implementations assume the login page is always at /login. This could be problematic if:

   • Apps use different login paths (e.g., /auth/login, /signin, /account/login)
   • Apps have multiple login pages (e.g., /admin/login, /user/login)
   • The login path is configurable

   Recommendation: Consider making the login path configurable:

   const loginPath = config.loginPath || '/login';
   const isOnLoginPage = window.location.pathname === loginPath;

2. Exact Path Matching: The checks use strict equality (===), which won't handle:

   • Query parameters (e.g., /login?redirect=/dashboard)
   • Trailing slashes (e.g., /login/)
   • Hash fragments (e.g., /login#forgot-password)

   Recommendation: Use more flexible matching:

   const isOnLoginPage = window.location.pathname === '/login' || 
                        window.location.pathname === '/login/';

3. Duplicate Logic: The login page check is duplicated in two places (client.ts:202 and auth.ts:50).

   Recommendation: Extract to a shared utility function:

   function isOnLoginPage(): boolean {
     return typeof window !== "undefined" && 
            window.location.pathname === "/login";
   }

Performance Considerations ✅

No performance concerns. The additional checks are minimal - just simple string comparisons that execute in O(1) time. The impact on bundle size is negligible.

Security Concerns ✅

No security issues identified. The fix appropriately prevents redirect loops which could potentially be exploited for denial-of-service attacks. The implementation doesn't expose any sensitive information or create new attack vectors.

Test Coverage ❌

Critical Issue: The PR lacks test coverage for the new functionality.

The existing tests don't cover:

• requiresAuth redirect behavior in client.ts
• The new skip logic in redirectToLogin() when already on /login
• Preview environment URL handling

Recommended Test Cases:

1. Test that no redirect occurs when on /login with requiresAuth: true
2. Test that redirect still occurs on other pages with requiresAuth: true
3. Test the redirectToLogin() skip behavior
4. Test edge cases: /login/, /login?param=value, /login#section
5. Test preview URL redirect logic (preview--* hostnames)

Example test structure:

test('should not redirect when already on login page', () => {
  // Mock window.location.pathname = '/login'
  global.window = {
    location: { pathname: '/login' }
  };
  
  const client = createClient({
    appId: 'test-app',
    requiresAuth: true
  });
  
  // Verify no redirect occurs
  // (setTimeout callback should not call redirectToLogin)
});

Additional Comments

1. Preview URL Fix: The addition of preview environment handling (auth.ts:56-65) is a good enhancement that wasn't mentioned in the PR description. This ensures preview environments redirect to the main app's login page while preserving the return URL.

2. Async Check: The authentication check in client.ts uses setTimeout(..., 0) to avoid blocking client creation, which is a good practice.

Recommendations

1. Add comprehensive test coverage - This is the most critical improvement needed
2. Consider making the login path configurable for better flexibility
3. Use more robust path matching to handle edge cases
4. Extract duplicate logic to maintain DRY principles
5. Update PR description to mention the preview URL fix

Conclusion

This is a valid fix for a real redirect loop issue. The implementation is functional but could benefit from:

• Test coverage (critical)
• More flexible configuration options
• Better edge case handling

With tests added and the minor improvements suggested, this would be ready to merge.

[claude]

Claude finished @eliorh's task —— View job

---

PR Review: Prevent Redirect Loop on Login Page

I've reviewed the changes in this PR. Here's my comprehensive feedback:

Todo List

• Check for CLAUDE.md or repository guidelines
• Read and analyze changes in src/modules/auth.ts
• Assess code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Check security concerns
• Review test coverage
• Provide comprehensive feedback

---

Summary

This PR adds logic to the redirectToLogin() method in src/modules/auth.ts to:

1. Check if already on the login page to prevent redirect loops (lines 50-52)
2. Handle preview environment URLs specially to redirect to the main app's login page (lines 47-48, 62-67)

Note: Based on the commit history, the changes to src/client.ts that were mentioned in previous reviews appear to have been reverted in the latest commit.

Code Quality ✅

The implementation is clean and follows existing patterns in the codebase. The variable naming (isPreview, isOnLoginPage) is clear and self-documenting. The logic flow is easy to understand.

Potential Bugs & Issues 🐛

1. Hardcoded Login Path: The implementation assumes the login page is always at /login (line 50). This could be problematic if:

   • Apps use different login paths (e.g., /auth/login, /signin, /account/login)
   • Apps have multiple login pages (e.g., /admin/login, /user/login)
   • The login path needs to be configurable

   Recommendation: Consider making the login path configurable through AuthModuleOptions:

   const loginPath = options.loginPath || '/login';
   const isOnLoginPage = window.location.pathname === loginPath && !isPreview;

2. Exact Path Matching: The check uses strict equality (===), which won't handle:

   • Trailing slashes (e.g., /login/)
   • Query parameters are preserved in the redirect, but the check doesn't account for them
   • Hash fragments (e.g., /login#forgot-password)

   Recommendation: Consider more flexible matching:

   const pathname = window.location.pathname;
   const isOnLoginPage = (pathname === '/login' || pathname === '/login/') && !isPreview;

3. Preview Logic Ordering: The current logic checks !isPreview in the condition (line 50), which is good - preview environments should always redirect to the main app. However, the comment could be clearer about this intentional behavior.

Performance Considerations ✅

No performance concerns. The additional checks are minimal:

• Two simple string operations (startsWith and equality check)
• String replacements for preview URL handling
• All operations are O(1) complexity

Security Concerns ✅

No security issues identified:

• The fix prevents redirect loops which could be exploited for denial-of-service
• The preview environment handling properly redirects to the main app while preserving the return URL
• No sensitive information is exposed

Test Coverage ❌

Critical Issue: The PR lacks test coverage for the new functionality.

Looking at the existing tests in tests/unit/auth.test.js, I can see that redirectToLogin() has some tests, but they don't cover:

1. The new skip behavior when already on /login
2. The preview environment URL handling
3. Edge cases like /login/ or /login?param=value

Recommended Test Cases:

test('should not redirect when already on login page', () => {
  const mockLocation = { 
    href: '',
    pathname: '/login',
    hostname: 'app.example.com'
  };
  global.window = { location: mockLocation };
  
  base44.auth.redirectToLogin();
  
  // Verify no redirect occurred (href remains empty)
  expect(mockLocation.href).toBe('');
});

test('should redirect from preview environment even when on /login', () => {
  const mockLocation = {
    href: '',
    pathname: '/login',
    hostname: 'preview--feature.example.com',
    protocol: 'https:',
    port: ''
  };
  global.window = { location: mockLocation };
  
  base44.auth.redirectToLogin();
  
  // Should redirect to main app's login
  expect(mockLocation.href).toContain('https://feature.example.com/login');
});

test('should handle preview URLs with custom ports', () => {
  const mockLocation = {
    href: '',
    pathname: '/dashboard',
    hostname: 'preview--feature.example.com',
    protocol: 'http:',
    port: '3000'
  };
  global.window = { location: mockLocation };
  
  base44.auth.redirectToLogin();
  
  expect(mockLocation.href).toBe(
    'http://feature.example.com:3000/login?from_url=' +
    encodeURIComponent('http://preview--feature.example.com:3000/dashboard')
  );
});

Additional Comments

1. Good Design Choice: The logic to always redirect preview environments (even from /login) is smart, as it ensures users authenticate against the main app rather than preview instances.

2. Missing client.ts Changes: The commit history shows changes to client.ts were reverted. If the redirect loop was also happening from the requiresAuth check in client initialization, you may need to add similar logic there.

Recommendations

1. Add comprehensive test coverage - This is the most critical improvement needed
2. Consider making the login path configurable for better flexibility
3. Handle edge cases like trailing slashes in path matching
4. Document the preview redirect behavior in code comments for clarity
5. Consider if client.ts also needs the redirect loop prevention

Conclusion

This is a good fix for a real redirect loop issue. The implementation is functional and addresses the core problem. The preview environment handling is a thoughtful addition. However, the PR would benefit from:

• Test coverage (critical)
• More flexible path configuration
• Better edge case handling

With tests added and the minor improvements suggested, this would be ready to merge.