Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to override the base URL for specific channel endpoints and adds an API Format column to the request logs. Key updates include frontend dialog enhancements for managing endpoint base URLs, schema modifications to support the new field, and backend logic to apply these overrides during LLM calls. Review feedback pointed out a bug in the dialog's state management where background refreshes could cause data loss and recommended using the Badge component in the request logs for better UI consistency.
| useEffect(() => { | ||
| if (open) { | ||
| setEndpoints(channel.endpoints ?? []); | ||
| setNewApiFormat(''); | ||
| setNewPath(''); | ||
| setNewBaseURL(''); | ||
| setError(null); | ||
| } | ||
| }, [open, channel.endpoints]); |
There was a problem hiding this comment.
The useEffect hook that resets the dialog state includes channel.endpoints in its dependency array. Since the channels are refetched every 5 seconds in the background (as seen in useQueryChannels), this dependency will trigger a state reset whenever a background refresh occurs, causing any unsaved changes the user has made in the dialog to be lost.
You should change the dependency to channel.id so that the state only resets when the dialog is opened or when the active channel changes, but not on background data refreshes.
| useEffect(() => { | |
| if (open) { | |
| setEndpoints(channel.endpoints ?? []); | |
| setNewApiFormat(''); | |
| setNewPath(''); | |
| setNewBaseURL(''); | |
| setError(null); | |
| } | |
| }, [open, channel.endpoints]); | |
| useEffect(() => { | |
| if (open) { | |
| setEndpoints(channel.endpoints ?? []); | |
| setNewApiFormat(''); | |
| setNewPath(''); | |
| setNewBaseURL(''); | |
| setError(null); | |
| } | |
| }, [open, channel.id]); |
| cell: ({ row }) => { | ||
| const format = row.original.format; | ||
| if (!format) { | ||
| return <div className='text-muted-foreground text-xs'>-</div>; | ||
| } | ||
| return ( | ||
| <span className='inline-flex items-center rounded-md border border-zinc-200 bg-zinc-50 px-2 py-0.5 text-xs font-medium text-zinc-700 dark:border-zinc-700 dark:bg-zinc-800/50 dark:text-zinc-300'> | ||
| {format} | ||
| </span> | ||
| ); | ||
| }, |
There was a problem hiding this comment.
For better UI consistency and maintainability, consider using the Badge component to display the API format, similar to how it is displayed in the channel endpoints dialog. The current implementation uses hardcoded zinc colors which might not align with the project's theme variables or other parts of the UI.
| cell: ({ row }) => { | |
| const format = row.original.format; | |
| if (!format) { | |
| return <div className='text-muted-foreground text-xs'>-</div>; | |
| } | |
| return ( | |
| <span className='inline-flex items-center rounded-md border border-zinc-200 bg-zinc-50 px-2 py-0.5 text-xs font-medium text-zinc-700 dark:border-zinc-700 dark:bg-zinc-800/50 dark:text-zinc-300'> | |
| {format} | |
| </span> | |
| ); | |
| }, | |
| cell: ({ row }) => { | |
| const format = row.original.format; | |
| if (!format) { | |
| return <div className='text-muted-foreground text-xs'>-</div>; | |
| } | |
| return ( | |
| <Badge variant='secondary' className='font-mono text-[10px]'> | |
| {format} | |
| </Badge> | |
| ); | |
| }, |
Greptile SummaryThis PR extends channel endpoints with a per-endpoint
Confidence Score: 4/5Safe to merge for normal users who go through the UI, but a direct API call with a malformed baseURL bypasses all validation and can corrupt a channel's endpoint data. The frontend correctly validates baseURL with The gap is in Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Frontend Dialog
participant GQL as GraphQL Mutation
participant Biz as ChannelService.SaveChannelEndpoints
participant Val as ValidateEndpoints
participant DB as Database
participant Route as buildNonDefaultEndpointOutbound
UI->>UI: z.url().optional().or(z.literal('')) validates baseURL
UI->>GQL: "saveChannelEndpoints({endpoints: [{apiFormat, path, baseURL}]})"
GQL->>Biz: SaveChannelEndpoints(input)
Biz->>Val: ValidateEndpoints(endpoints)
Note over Val: Checks api_format + path<br/>does NOT check baseURL
Val-->>Biz: ok
Biz->>DB: SetEndpoints(endpoints).Save()
DB-->>Biz: channel
Note over Route: At request time...
Route->>Route: "baseURL = channel.BaseURL"
Route->>Route: "if ep.BaseURL != empty then baseURL = ep.BaseURL"
Route->>Route: build outbound transformer with baseURL
Reviews (3): Last reviewed commit: "feat: customized channel endpoints with ..." | Re-trigger Greptile |
| type ChannelEndpoint struct { | ||
| APIFormat string `json:"api_format"` | ||
| Path string `json:"path,omitempty"` | ||
| BaseURL string `json:"base_url,omitempty"` | ||
| } |
There was a problem hiding this comment.
BaseURL
The existing ValidateEndpoints function in channel_endpoint.go validates api_format (non-empty, supported, unique) and path (must start with /, must not be a full URL), but it was not updated to validate the new BaseURL field. If a caller bypasses the frontend and submits an invalid string (e.g. "not-a-url") via the GraphQL mutation, it is persisted to the database without error. When the channel is subsequently fetched, the frontend's channelEndpointSchema — which uses z.url('Invalid URL').optional().or(z.literal('')) — will reject the malformed string, causing channelSchema.parse() to throw and breaking the channel list entirely for that channel. Adding a url.ParseRequestURI check on ep.BaseURL in ValidateEndpoints (similar to how path guards against accidental full URLs) would prevent invalid data from ever reaching the database.
No description provided.