fix(frontend): detect auto-login disabled from FastAPI nested detail field#13770
fix(frontend): detect auto-login disabled from FastAPI nested detail field#13770Eltortilla1 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughExtends ChangesAuto-login detection for FastAPI nested error shape
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5071d37 to
bc73224
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/frontend/src/controllers/API/queries/auth/__tests__/autologin-disabled-detection.test.ts (2)
23-27: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the detection logic into a testable function.
The helper duplicates the production logic from
use-get-autologin.ts. While the comment states this is intentional to make tests break when logic changes, this creates a maintenance risk: if someone incorrectly modifies both the production code and this test helper, the tests will pass even with broken behavior.A more robust approach would be to extract the detection logic into a separate exported function in the production code and import it here. This ensures tests validate the actual production code path.
🤖 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 `@src/frontend/src/controllers/API/queries/auth/__tests__/autologin-disabled-detection.test.ts` around lines 23 - 27, The helper function `isAutoLoginDisabledByBackend` in the test file duplicates the production logic from `use-get-autologin.ts`, creating maintenance risk. Extract the detection logic that checks if `data?.detail?.auto_login === false || data?.auto_login === false` into a separate exported function in the production code file `use-get-autologin.ts`, then remove the duplicate implementation from the test file and instead import and use that exported function. This ensures the test validates the actual production code path rather than a duplicated copy.Source: Coding guidelines
29-78: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftTest coverage validates the isolated logic but not the production code path.
These tests validate the helper function's logic but don't test the actual
useGetAutoLoginhook where the detection logic lives inline (lines 67-69 ofuse-get-autologin.ts). Consider adding integration tests that mock axios responses and verify the hook's behavior end-to-end.This would provide stronger confidence that the fix works in the actual production flow, especially since the detection logic isn't extracted as a separate function.
🤖 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 `@src/frontend/src/controllers/API/queries/auth/__tests__/autologin-disabled-detection.test.ts` around lines 29 - 78, Add integration tests for the useGetAutoLogin hook that mock axios error responses and verify the end-to-end behavior of the auto login disabled detection. The current test file validates the isAutoLoginDisabledByBackend helper function in isolation, but the actual detection logic lives inline within the useGetAutoLogin hook (in use-get-autologin.ts at lines 67-69). Create tests that mock axios to return FastAPI error responses with the nested detail shape and flat shape, then assert that the hook correctly handles these responses and sets the autoLoginDisabledByBackend state appropriately.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@src/frontend/src/controllers/API/queries/auth/__tests__/autologin-disabled-detection.test.ts`:
- Around line 53-60: The test in the "Previous buggy path — data.auto_login
directly (undefined for FastAPI errors)" describe block only verifies that the
bug exists (data.auto_login is undefined) but does not actually test the
function behavior. Enhance the test by calling the isAutoLoginDisabledByBackend
function with the data object and asserting that it returns true, since the
nested detail.auto_login property is false. This will verify that the function
correctly handles the FastAPI nested error response shape despite the old code's
incorrect direct data.auto_login access.
---
Nitpick comments:
In
`@src/frontend/src/controllers/API/queries/auth/__tests__/autologin-disabled-detection.test.ts`:
- Around line 23-27: The helper function `isAutoLoginDisabledByBackend` in the
test file duplicates the production logic from `use-get-autologin.ts`, creating
maintenance risk. Extract the detection logic that checks if
`data?.detail?.auto_login === false || data?.auto_login === false` into a
separate exported function in the production code file `use-get-autologin.ts`,
then remove the duplicate implementation from the test file and instead import
and use that exported function. This ensures the test validates the actual
production code path rather than a duplicated copy.
- Around line 29-78: Add integration tests for the useGetAutoLogin hook that
mock axios error responses and verify the end-to-end behavior of the auto login
disabled detection. The current test file validates the
isAutoLoginDisabledByBackend helper function in isolation, but the actual
detection logic lives inline within the useGetAutoLogin hook (in
use-get-autologin.ts at lines 67-69). Create tests that mock axios to return
FastAPI error responses with the nested detail shape and flat shape, then assert
that the hook correctly handles these responses and sets the
autoLoginDisabledByBackend state appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff1ac53d-2abd-4576-a6f1-c5c884850af4
📒 Files selected for processing (2)
src/frontend/src/controllers/API/queries/auth/__tests__/autologin-disabled-detection.test.tssrc/frontend/src/controllers/API/queries/auth/use-get-autologin.ts
| describe("Previous buggy path — data.auto_login directly (undefined for FastAPI errors)", () => { | ||
| it("returns false when only data is present without detail (old incorrect read)", () => { | ||
| // This simulates what the OLD code read: data.auto_login === undefined | ||
| const data = { detail: { auto_login: false } } as AutoLoginErrorResponse; | ||
| // Accessing data.auto_login (old code) gives undefined, which !== false | ||
| expect(data.auto_login).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Complete the test to verify the function behavior.
This test only documents that data.auto_login is undefined (the bug), but it doesn't verify that isAutoLoginDisabledByBackend correctly handles the FastAPI nested shape. The test should call the function and assert it returns true for this case.
🧪 Suggested fix
describe("Previous buggy path — data.auto_login directly (undefined for FastAPI errors)", () => {
it("returns false when only data is present without detail (old incorrect read)", () => {
- // This simulates what the OLD code read: data.auto_login === undefined
const data = { detail: { auto_login: false } } as AutoLoginErrorResponse;
- // Accessing data.auto_login (old code) gives undefined, which !== false
+ // The OLD code read data.auto_login (undefined), which !== false, causing the bug
+ // The FIXED code reads data.detail.auto_login (false), correctly detecting disabled state
expect(data.auto_login).toBeUndefined();
+ expect(isAutoLoginDisabledByBackend(data)).toBe(true);
});
});🤖 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
`@src/frontend/src/controllers/API/queries/auth/__tests__/autologin-disabled-detection.test.ts`
around lines 53 - 60, The test in the "Previous buggy path — data.auto_login
directly (undefined for FastAPI errors)" describe block only verifies that the
bug exists (data.auto_login is undefined) but does not actually test the
function behavior. Enhance the test by calling the isAutoLoginDisabledByBackend
function with the data object and asserting that it returns true, since the
nested detail.auto_login property is false. This will verify that the function
correctly handles the FastAPI nested error response shape despite the old code's
incorrect direct data.auto_login access.
Source: Coding guidelines
…field
When LANGFLOW_AUTO_LOGIN=false the backend returns HTTP 403 with:
{ "detail": { "message": "Auto login is disabled.", "auto_login": false } }
The previous check read `error.response?.data?.auto_login`, which is
`undefined` because FastAPI nests error payloads under `detail`. This caused
`autoLoginDisabledByBackend` to always evaluate to `false`, triggering an
infinite retry loop and preventing the login screen from ever rendering.
Fix: read `error.response?.data?.detail?.auto_login === false` first
(FastAPI error shape), with a fallback to the top-level field for
forward-compatibility. Also extends `AutoLoginErrorResponse` to correctly
type the `detail` field.
Fixes langflow-ai#13766
refactor(frontend): extract isAutoLoginDisabled as exported utility
Extract the auto-login disabled detection predicate into a named exported
function so regression tests can import it directly rather than duplicating
the logic. No behaviour change.
test(frontend): assert isAutoLoginDisabled handles FastAPI nested shape
Extend the regression test to call isAutoLoginDisabled() and assert it
returns true when auto_login is nested under detail, confirming the fix
works end-to-end and not just that the old read returned undefined.
aa3dc3e to
bba75a0
Compare
Summary
Fixes #13766 — login screen never displayed (infinite loading spinner) when
LANGFLOW_AUTO_LOGIN=false.AUTO_LOGIN=falsethe backend returns HTTP 403 with{ "detail": { "auto_login": false } }. The previous check readerror.response?.data?.auto_login, which isundefinedbecause FastAPI wraps error payloads underdetail. As a resultautoLoginDisabledByBackendwas alwaysfalse, causing an infinite retry loop inhandleAutoLoginErrorand the login page never rendering.error.response?.data?.detail?.auto_login === falsefirst (FastAPI error shape), with a fallback to the top-level field for forward-compatibility.AutoLoginErrorResponseto correctly type thedetailfield.Test plan
LANGFLOW_AUTO_LOGIN=false,LANGFLOW_SUPERUSER,LANGFLOW_SUPERUSER_PASSWORDsethttp://localhost:3000— login screen appears immediately (no infinite spinner)auto_loginorrefreshrequests visible in Network tabLANGFLOW_AUTO_LOGIN=truestill auto-logs in without credentials (no regression)autologin-disabled-detection.test.ts(7 tests)Summary by CodeRabbit
Bug Fixes
Tests