fix(ui): omit unchanged allowed_routes on key update#27070
Conversation
|
|
Greptile SummaryThis PR fixes a UX bug where the key-edit form always included Confidence Score: 4/5Safe to merge; findings are P2 suggestions that don't affect correctness of the core fix. Only P2 findings: an order-sensitive list comparison that could misfire if the form reorders routes, and a minor test-coverage gap for undefined allowed_routes. The core logic is sound and well-tested for the three key scenarios. No files require special attention beyond the inline suggestions.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/templates/key_info_view.tsx | Adds areStringListsEqual and normalizeStringList helpers; strips unchanged allowed_routes from key-update payloads to avoid tripping the backend permission check on no-op saves. |
| ui/litellm-dashboard/src/components/templates/key_info_view.test.tsx | Adds three new tests for unchanged, already-empty, and clear-to-empty allowed_routes flows; all use mocked keyUpdateCall and do not make real network calls. |
Reviews (1): Last reviewed commit: "fix(ui): omit unchanged allowed_routes o..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const areStringListsEqual = (left: unknown, right: unknown): boolean => { | ||
| const normalizedLeft = normalizeStringList(left); | ||
| const normalizedRight = normalizeStringList(right); | ||
|
|
||
| return ( | ||
| normalizedLeft.length === normalizedRight.length && | ||
| normalizedLeft.every((entry, index) => entry === normalizedRight[index]) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
areStringListsEqual uses positional comparison (every((entry, index) => entry === right[index])), so two lists with the same routes in different order are treated as distinct. If the multi-select component in KeyEditView ever returns routes in a different order than how they were stored (e.g., it sorts alphabetically or the user reselects in a different sequence), a non-admin editor would still trip the backend permission check on what they perceive as an unchanged save. A set/sorted comparison is more robust for unordered identifiers like routes.
| const areStringListsEqual = (left: unknown, right: unknown): boolean => { | |
| const normalizedLeft = normalizeStringList(left); | |
| const normalizedRight = normalizeStringList(right); | |
| return ( | |
| normalizedLeft.length === normalizedRight.length && | |
| normalizedLeft.every((entry, index) => entry === normalizedRight[index]) | |
| ); | |
| }; | |
| const areStringListsEqual = (left: unknown, right: unknown): boolean => { | |
| const normalizedLeft = normalizeStringList(left).sort(); | |
| const normalizedRight = normalizeStringList(right).sort(); | |
| return ( | |
| normalizedLeft.length === normalizedRight.length && | |
| normalizedLeft.every((entry, index) => entry === normalizedRight[index]) | |
| ); | |
| }; |
| it("should drop empty allowed_routes when the key previously had no route override", async () => { | ||
| const keyData: KeyResponse = { | ||
| ...MOCK_KEY_DATA, | ||
| user_id: "proxy-admin-user", | ||
| allowed_routes: [], | ||
| } as KeyResponse; | ||
|
|
||
| await enterEditMode(keyData); | ||
| await editViewMocks.onSubmit!({ | ||
| key: keyData.token, | ||
| token: keyData.token, | ||
| allowed_routes: [], | ||
| }); | ||
|
|
||
| expect(keyUpdateCall).toHaveBeenCalledWith( | ||
| expect.anything(), | ||
| expect.not.objectContaining({ allowed_routes: expect.anything() }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
currentKeyData.allowed_routes is null or undefined
The most common case for a key that never had allowed_routes set is undefined (the field simply isn't present on KeyResponse), yet only [] is covered by the "previously had no route override" test. If normalizeStringList handles undefined correctly (it does — returns []), a test for that value makes the coverage explicit and guards against future regressions in the helper.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Testing
Closes #27005