Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdate virtual-key UI/hooks to read renamed API fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant RBAC as RBAC check
participant API as GraphQL/API
participant DS as DataSource (teams/customers/virtual_keys)
UI->>RBAC: request access for queries
RBAC-->>UI: allowed/denied flags
alt allowed
UI->>API: fetch teams(customers, vks) with search, offset, limit
API->>DS: query datasets
DS-->>API: results + total
API-->>UI: datasets + total
UI->>UI: debounce search (300ms), update offset, poll every 5s
else denied
UI-->>UI: skip corresponding queries (no request)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
|
|
Contributor
Confidence Score: 5/5Safe to merge — all core fixes are correct and the only remaining finding is a minor pre-existing style issue. The field name fixes are verified against the virtualKeyDetailsSheet.tsx — minor pre-existing Important Files Changed
Reviews (7): Last reviewed commit: "docs: add docs for claude code vscode pl..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
akshaydeo
force-pushed
the
04-21-v1.5.0-prerelease4_cut
branch
from
2923c30 to
eecdb93
Compare
danpiths
previously approved these changes
Apr 20, 2026
danpiths
dismissed stale reviews from coderabbitai[bot] and themself
via
e0cf66f
danpiths
force-pushed
the
04-21-v1.5.0-prerelease4_cut
branch
from
eecdb93 to
e0cf66f
Compare
danpiths
force-pushed
the
04-21-v1.5.0-prerelease4_cut
branch
from
e0cf66f to
4fdd841
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ui/app/workspace/governance/teams/page.tsx (2)
61-64: AddteamsDatato dependency array for completeness.The effect reads
teamsDatain the condition but it's not in the dependency array. While this works in practice becauseteamsTotalis derived from it, addingteamsDatawould satisfy the exhaustive-deps rule and make the dependencies explicit.💡 Proposed fix
useEffect(() => { if (!teamsData || offset < teamsTotal) return; setOffset(teamsTotal === 0 ? 0 : Math.floor((teamsTotal - 1) / PAGE_SIZE) * PAGE_SIZE); -}, [teamsTotal, offset]); +}, [teamsData, teamsTotal, offset]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/governance/teams/page.tsx` around lines 61 - 64, The useEffect that adjusts pagination reads teamsData but omits it from the dependency array; update the dependency array of the useEffect (the one calling setOffset and referencing teamsTotal, offset, PAGE_SIZE) to include teamsData alongside teamsTotal and offset so the effect re-runs when teamsData changes and satisfies exhaustive-deps.
18-20: Consider usingnuqsfor search state persistence.Per coding guidelines, persistent or shareable state (like search filters) should use
nuqsfor URL query params. This allows users to bookmark or share filtered views.💡 Example refactor using nuqs
-import { useEffect, useRef, useState } from "react"; +import { useEffect, useRef } from "react"; +import { useQueryState, parseAsString } from "nuqs"; // ... -const [search, setSearch] = useState(""); +const [search, setSearch] = useQueryState("search", parseAsString.withDefault(""));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/governance/teams/page.tsx` around lines 18 - 20, The search state currently uses local React state (search, setSearch) with useDebouncedValue to debounce input, which prevents sharing/bookmarking filters; replace this with a nuqs-backed query param so the search persists in the URL: remove or replace the useState for search and setSearch with the nuqs hook (the project’s query-param hook) to read/write the query key (e.g., "q" or "search") and keep debouncedSearch produced from that value (retain useDebouncedValue on the nuqs value), updating any handlers that reference search/setSearch to use the nuqs getter/setter so filtered views are shareable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/workspace/governance/teams/page.tsx`:
- Around line 61-64: The useEffect that adjusts pagination reads teamsData but
omits it from the dependency array; update the dependency array of the useEffect
(the one calling setOffset and referencing teamsTotal, offset, PAGE_SIZE) to
include teamsData alongside teamsTotal and offset so the effect re-runs when
teamsData changes and satisfies exhaustive-deps.
- Around line 18-20: The search state currently uses local React state (search,
setSearch) with useDebouncedValue to debounce input, which prevents
sharing/bookmarking filters; replace this with a nuqs-backed query param so the
search persists in the URL: remove or replace the useState for search and
setSearch with the nuqs hook (the project’s query-param hook) to read/write the
query key (e.g., "q" or "search") and keep debouncedSearch produced from that
value (retain useDebouncedValue on the nuqs value), updating any handlers that
reference search/setSearch to use the nuqs getter/setter so filtered views are
shareable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbce1533-c92d-477f-9ae3-7ac3bef19c1d
📒 Files selected for processing (6)
docs/enterprise/setting-up-okta.mdxui/app/workspace/governance/teams/page.tsxui/app/workspace/virtual-keys/hooks/useVirtualKeyUsage.tsui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/MultiBudgetLines.tsx
💤 Files with no reviewable changes (1)
- docs/enterprise/setting-up-okta.mdx
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/workspace/virtual-keys/hooks/useVirtualKeyUsage.ts
18 tasks
akshaydeo
force-pushed
the
04-21-v1.5.0-prerelease4_cut
branch
from
4fdd841 to
958d965
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm-charts/bifrost/templates/deployment.yaml (1)
91-97:⚠️ Potential issue | 🟠 MajorEnsure optional encryption-key behavior is consistent across Deployment and StatefulSet
Line 91 correctly gates on
encryptionKeySecret.name, buthelm-charts/bifrost/templates/stateful.yaml(context snippet Lines 91-97) still uses the old object-truthy guard. This leaves sqlite+persistence installs on StatefulSet with different behavior and can still rendersecretKeyRef.nameas empty.Suggested parity fix
--- a/helm-charts/bifrost/templates/stateful.yaml +++ b/helm-charts/bifrost/templates/stateful.yaml @@ -{{- if .Values.bifrost.encryptionKeySecret }} +{{- if .Values.bifrost.encryptionKeySecret.name }} - name: BIFROST_ENCRYPTION_KEY valueFrom: secretKeyRef: name: {{ .Values.bifrost.encryptionKeySecret.name }} key: {{ .Values.bifrost.encryptionKeySecret.key }} {{- end }}As per coding guidelines "
**: always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm-charts/bifrost/templates/deployment.yaml` around lines 91 - 97, The StatefulSet template's conditional for the BIFROST_ENCRYPTION_KEY env var must match the Deployment guard: replace the current object-truthy check with an explicit check for .Values.bifrost.encryptionKeySecret.name so the BIFROST_ENCRYPTION_KEY env var is only rendered when a name is provided; update the conditional around the BIFROST_ENCRYPTION_KEY env var in the stateful template to use the same guard (.Values.bifrost.encryptionKeySecret.name) and ensure secretKeyRef.name and key are only emitted when that value is present to avoid rendering an empty secret name.
🧹 Nitpick comments (2)
ui/app/workspace/governance/teams/page.tsx (1)
18-20: Consider using nuqs for shareable search state.The search state is currently local, which means it won't persist across page refreshes or be shareable via URL. For better UX, consider using nuqs to sync the search query parameter with the URL. This is optional for this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/governance/teams/page.tsx` around lines 18 - 20, Replace the local search useState with a nuqs-backed query state so the query is persisted and shareable: swap the const [search, setSearch] = useState("") to use nuqs' query-state hook (e.g., useQueryState or equivalent) keyed as "search" with default "" and keep setSearch to update that query param; continue to feed debouncedSearch = useDebouncedValue(search, 300) from the nuqs-backed search value so debouncing works the same; optionally apply the same pattern for offset/ setOffset if you want pagination to be shareable.ui/app/workspace/logs/views/logChatMessageView.tsx (1)
188-188: Use a stable key for each tool call.Line 188 still keys the mapped
tool_callsbyindex, even though the log types exposetoolCall.id. Reordering or inserting calls will remount these panels unnecessarily. PrefertoolCall.idwith a deterministic fallback only when it is actually absent.As per coding guidelines, "Always use stable, unique keys in lists; never use array index as key unless unavoidable".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/views/logChatMessageView.tsx` at line 188, The mapped CollapsibleBox currently uses the array index as its React key, causing unnecessary remounts; change the key to use the tool call's stable id (toolCall.id) and only fall back to a deterministic fallback when id is truly absent (e.g., `toolCall.id ?? \`toolcall-${index}\``). Update the key prop on the CollapsibleBox created in the tool_calls mapping (the element with title `Tool Call: ${toolCall.function?.name || \`#${index + 1}\`}` and onCopy returning jsonContent) to use toolCall.id first to ensure stable, unique keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/changelog.md`:
- Line 7: Update the changelog line describing OCR Request Support to use a
hyphenated compound modifier: replace "full body accumulation" with "full-body
accumulation" in the string that begins with "**OCR Request Support** — Added
OCR request type with stream terminal detection, full body accumulation for
passthrough streams, input logging with detail view, and per-request pricing
support" so the wording reads "...full-body accumulation for passthrough
streams..." for improved readability.
In `@ui/app/workspace/governance/teams/page.tsx`:
- Around line 60-64: The useEffect that snaps offset when items shrink
references teamsData in its condition but doesn't include it in the dependency
array; update the effect's dependencies to include teamsData alongside
teamsTotal and offset so React's exhaustive-deps rule is satisfied (locate the
useEffect block that checks "if (!teamsData || offset < teamsTotal) ..." and add
teamsData to the array used by useEffect that calls setOffset(... PAGE_SIZE
...)).
In `@ui/app/workspace/logs/sheets/logDetailView.tsx`:
- Around line 1495-1499: The current summary drops unnamed tool calls because it
maps tool_calls to tc.function?.name and filters falsy values; update the
rendering logic in logDetailView.tsx so that for each item you either use a
per-item fallback (e.g., map to tc.function?.name ?? "unnamed" and join) or
detect whether every tool call has a name and only use the joined names in that
case, otherwise show the count-based label; specifically modify the expression
using message.tool_calls (and the inline map/filter/join) to implement one of
these two behaviors so unnamed calls are not lost.
In `@ui/app/workspace/logs/views/logResponsesMessageView.tsx`:
- Around line 347-384: The current output rendering always stringifies
non-string message.output values, which breaks structured block outputs
(ResponsesMessageContentBlock[]); update the conditional in the message.output
branch to detect when message.output is an array of content blocks
(Array.isArray(message.output) and elements match the block shape) and render
those using the existing ContentBlockView (or the component used elsewhere for
block rendering) inside the CollapsibleBox (mapping each block to a
ContentBlockView with a stable key) before falling back to the JSON CodeEditor;
keep existing handling for string outputs (isJson/cleanJson) and for other
non-block objects to preserve current behavior.
---
Outside diff comments:
In `@helm-charts/bifrost/templates/deployment.yaml`:
- Around line 91-97: The StatefulSet template's conditional for the
BIFROST_ENCRYPTION_KEY env var must match the Deployment guard: replace the
current object-truthy check with an explicit check for
.Values.bifrost.encryptionKeySecret.name so the BIFROST_ENCRYPTION_KEY env var
is only rendered when a name is provided; update the conditional around the
BIFROST_ENCRYPTION_KEY env var in the stateful template to use the same guard
(.Values.bifrost.encryptionKeySecret.name) and ensure secretKeyRef.name and key
are only emitted when that value is present to avoid rendering an empty secret
name.
---
Nitpick comments:
In `@ui/app/workspace/governance/teams/page.tsx`:
- Around line 18-20: Replace the local search useState with a nuqs-backed query
state so the query is persisted and shareable: swap the const [search,
setSearch] = useState("") to use nuqs' query-state hook (e.g., useQueryState or
equivalent) keyed as "search" with default "" and keep setSearch to update that
query param; continue to feed debouncedSearch = useDebouncedValue(search, 300)
from the nuqs-backed search value so debouncing works the same; optionally apply
the same pattern for offset/ setOffset if you want pagination to be shareable.
In `@ui/app/workspace/logs/views/logChatMessageView.tsx`:
- Line 188: The mapped CollapsibleBox currently uses the array index as its
React key, causing unnecessary remounts; change the key to use the tool call's
stable id (toolCall.id) and only fall back to a deterministic fallback when id
is truly absent (e.g., `toolCall.id ?? \`toolcall-${index}\``). Update the key
prop on the CollapsibleBox created in the tool_calls mapping (the element with
title `Tool Call: ${toolCall.function?.name || \`#${index + 1}\`}` and onCopy
returning jsonContent) to use toolCall.id first to ensure stable, unique keys.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1119399c-b2e2-421f-9887-521c96b0c9e9
📒 Files selected for processing (59)
.github/workflows/helm-release.yml.github/workflows/scripts/schemasync/go.modcore/changelog.mdcore/schemas/useragents.gocore/versiondocs/enterprise/setting-up-okta.mdxexamples/plugins/hello-world/go.modframework/changelog.mdframework/go.modframework/versionhelm-charts/bifrost/Chart.yamlhelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/deployment.yamlhelm-charts/index.yamlplugins/compat/changelog.mdplugins/compat/go.modplugins/compat/versionplugins/governance/changelog.mdplugins/governance/go.modplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/go.modplugins/jsonparser/versionplugins/logging/changelog.mdplugins/logging/go.modplugins/logging/operations.goplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/go.modplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/go.modplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/go.modplugins/otel/versionplugins/prompts/changelog.mdplugins/prompts/go.modplugins/prompts/versionplugins/semanticcache/changelog.mdplugins/semanticcache/go.modplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/go.modplugins/telemetry/versiontests/async/go.modtransports/changelog.mdtransports/go.modtransports/versionui/app/workspace/governance/teams/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/app/workspace/logs/views/columns.tsxui/app/workspace/logs/views/logChatMessageView.tsxui/app/workspace/logs/views/logResponsesMessageView.tsxui/app/workspace/model-catalog/views/modelCatalogView.tsxui/app/workspace/virtual-keys/hooks/useVirtualKeyUsage.tsui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/components/ui/MultiBudgetLines.tsx
💤 Files with no reviewable changes (2)
- docs/enterprise/setting-up-okta.mdx
- plugins/logging/operations.go
✅ Files skipped from review due to trivial changes (45)
- plugins/governance/version
- plugins/jsonparser/changelog.md
- framework/version
- core/version
- plugins/mocker/changelog.md
- .github/workflows/helm-release.yml
- plugins/maxim/changelog.md
- plugins/logging/changelog.md
- plugins/telemetry/changelog.md
- helm-charts/bifrost/README.md
- plugins/telemetry/version
- plugins/prompts/version
- plugins/mocker/version
- plugins/logging/version
- plugins/jsonparser/version
- .github/workflows/scripts/schemasync/go.mod
- plugins/otel/version
- plugins/compat/version
- examples/plugins/hello-world/go.mod
- plugins/compat/changelog.md
- plugins/jsonparser/go.mod
- plugins/semanticcache/version
- framework/go.mod
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- transports/version
- helm-charts/bifrost/Chart.yaml
- plugins/governance/changelog.md
- plugins/governance/go.mod
- plugins/prompts/go.mod
- plugins/compat/go.mod
- helm-charts/index.yaml
- ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx
- core/schemas/useragents.go
- plugins/logging/go.mod
- plugins/semanticcache/go.mod
- plugins/telemetry/go.mod
- tests/async/go.mod
- framework/changelog.md
- plugins/maxim/go.mod
- plugins/semanticcache/changelog.md
- plugins/otel/changelog.md
- plugins/maxim/version
- plugins/mocker/go.mod
- plugins/prompts/changelog.md
- transports/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/workspace/virtual-keys/hooks/useVirtualKeyUsage.ts
| - **Anthropic Structured Outputs** — Added `response_format` and structured output support for Anthropic models across chat completions and Responses API, with order-preserving merge of additional model request fields (thanks [@emirhanmutlu-natuvion](https://github.com/emirhanmutlu-natuvion)!) | ||
| - **MCP Tool Annotations** — Preserve MCP tool annotations (`title`, `readOnly`, `destructive`, `idempotent`, `openWorld`) in bidirectional conversion so agents can reason about tool behavior | ||
| - **Anthropic Server Tools** — Expanded Anthropic chat schema and Responses converters to surface server-side tools (web search, code execution, computer use containers) end-to-end | ||
| - **OCR Request Support** — Added OCR request type with stream terminal detection, full body accumulation for passthrough streams, input logging with detail view, and per-request pricing support |
Contributor
There was a problem hiding this comment.
Use a hyphen in the compound modifier for readability.
“full body accumulation” reads cleaner as “full-body accumulation” in this context.
✍️ Suggested wording tweak
-- **OCR Request Support** — Added OCR request type with stream terminal detection, full body accumulation for passthrough streams, input logging with detail view, and per-request pricing support
+- **OCR Request Support** — Added OCR request type with stream terminal detection, full-body accumulation for passthrough streams, input logging with detail view, and per-request pricing support📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **OCR Request Support** — Added OCR request type with stream terminal detection, full body accumulation for passthrough streams, input logging with detail view, and per-request pricing support | |
| - **OCR Request Support** — Added OCR request type with stream terminal detection, full-body accumulation for passthrough streams, input logging with detail view, and per-request pricing support |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...st type with stream terminal detection, full body accumulation for passthrough streams, i...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transports/changelog.md` at line 7, Update the changelog line describing OCR
Request Support to use a hyphenated compound modifier: replace "full body
accumulation" with "full-body accumulation" in the string that begins with
"**OCR Request Support** — Added OCR request type with stream terminal
detection, full body accumulation for passthrough streams, input logging with
detail view, and per-request pricing support" so the wording reads "...full-body
accumulation for passthrough streams..." for improved readability.
18 tasks
akshaydeo
force-pushed
the
04-21-v1.5.0-prerelease4_cut
branch
from
44bb06e to
45cbed9
Compare
akshaydeo
force-pushed
the
04-21-v1.5.0-prerelease4_cut
branch
from
45cbed9 to
8f144bf
Compare
Contributor
Author
Merge activity
|
dominictayloruk
pushed a commit
to dominictayloruk/bifrost
that referenced
this pull request
Apr 21, 2026
* v1.5.0-prerelease4 cut * docs: add docs for claude code vscode plugin --------- Co-authored-by: Anuj Parihar <anujparihar@yahoo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes field name mismatches between the UI and the API contract for access profile budget and rate limit fields. The UI was referencing
budgetsandrate_limiton managing profiles, which did not match the correct field namesbudget_linesandrate_limits.Changes
managingProfile.budgetswithmanagingProfile.budget_linesin both the usage hook and the details sheet view to correctly read budget data from access profiles.managingProfile.rate_limitwithmanagingProfile.rate_limitsin the usage hook to correctly read rate limit data from access profiles.inertattribute on the fieldset from a string""/undefinedto a booleantrue/falsefor correctness.Type of change
Affected areas
How to test
Navigate to a virtual key that is managed by an access profile with budgets and/or rate limits configured. Verify that:
Breaking changes
Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines