Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughSelective committing of session messages was added: the frontend lets users pick message indices to include, and the backend accepts an optional Changes
Sequence DiagramsequenceDiagram
participant User
participant ViewHeader as "View Header"
participant CommitSheet as "Commit Sheet UI"
participant Backend as "Backend Handler"
User->>ViewHeader: Click "Commit Version"
ViewHeader->>CommitSheet: Open commit sheet (init selection)
User->>CommitSheet: Toggle message checkboxes
alt No changes (hasChanges == false)
ViewHeader->>ViewHeader: Find selected session
ViewHeader-->>User: Call onSessionSaved(selectedSession) (no backend request)
else Has changes
User->>CommitSheet: Submit commit
CommitSheet->>CommitSheet: Build payload (trim message)
CommitSheet->>CommitSheet: Include message_indices only if not all selected
CommitSheet->>Backend: POST /commit with payload
Backend->>Backend: Validate message_indices (if present)
alt Invalid indices or empty selection
Backend-->>CommitSheet: 400 BadRequest (error)
CommitSheet->>User: Show error toast
else Valid
Backend->>Backend: Construct versions from selected/all messages
Backend-->>CommitSheet: 200 OK (success)
CommitSheet->>ViewHeader: onSessionSaved(result.session)
ViewHeader->>User: Update UI state
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
f89f479 to
9367bf9
Compare
f49ca80 to
5013139
Compare
9367bf9 to
8d5d81d
Compare
8d5d81d to
7735ceb
Compare
37a9003 to
c41876d
Compare
6947a3d to
a2340f6
Compare
35c9daf to
37bf719
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/components/prompts/components/promptsViewHeader.tsx (1)
111-136:⚠️ Potential issue | 🟠 MajorMissing dependencies in
useCallbackcause stale closure.The callback references
sessionsandselectedSessionId(lines 114-115) but they're missing from the dependency array. This can cause the callback to reference stale values when these change.🐛 Proposed fix
- }, [selectedPrompt?.id, messages, buildSaveParams, provider, model, createSession, setUrlState, onSessionSaved, hasChanges]); + }, [selectedPrompt?.id, messages, buildSaveParams, provider, model, createSession, setUrlState, onSessionSaved, hasChanges, sessions, selectedSessionId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/components/promptsViewHeader.tsx` around lines 111 - 136, The handleCommitVersion callback closes over sessions and selectedSessionId but they are not listed in the useCallback dependency array, which can lead to stale values; update the dependency array for useCallback that defines handleCommitVersion to include sessions and selectedSessionId (in addition to the existing dependencies like selectedPrompt?.id, messages, buildSaveParams, provider, model, createSession, setUrlState, onSessionSaved, hasChanges) so the closure always sees the latest session list and selected session id.ui/components/prompts/sheets/commitVersionSheet.tsx (1)
111-135:⚠️ Potential issue | 🟡 MinorEmpty selection would commit a version with no messages.
If the user deselects all messages and submits,
message_indiceswould be[]andallSelectedwould befalse, sending an empty array. The backend would create a version with zero messages. Consider disabling submit when no messages are selected.🛡️ Proposed fix - disable submit button when no messages selected
- <Button type="submit" data-testid="commit-version-submit" disabled={isLoading}> + <Button type="submit" data-testid="commit-version-submit" disabled={isLoading || selectedIndices.size === 0}> {isLoading ? 'Committing...' : 'Commit Version'} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/sheets/commitVersionSheet.tsx` around lines 111 - 135, Prevent submitting an empty selection by adding a selection check in onSubmit and disabling the form submit control: inside onSubmit (function onSubmit) early-return with a toast.error if neither allSelected is true nor selectedIndices has any entries (e.g., selectedIndices.size === 0), and also update the submit button in the CommitVersionSheet UI to be disabled when there are no selected messages (use the same condition: !allSelected && selectedIndices.size === 0) so the user cannot submit a commit with zero messages.
🧹 Nitpick comments (4)
ui/components/prompts/sheets/commitVersionSheet.tsx (3)
81-87: Consider usingsession.messages.lengthin dependency instead of the array reference.The
session.messagesarray reference may change even if the content is the same, potentially causing unnecessary resets. Usingsession.messages.lengthwould be more stable while still reacting to actual message count changes.💡 Proposed fix
useEffect(() => { if (open) { reset({ commitMessage: '' }) setSelectedIndices(new Set(session.messages.length > 0 ? [0] : [])) } - }, [open, reset, session.messages]) + }, [open, reset, session.messages.length])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/sheets/commitVersionSheet.tsx` around lines 81 - 87, The effect that resets the form and selection uses session.messages in its dependency array which can change by reference and trigger unnecessary runs; update the dependency to session.messages.length and keep the body using session.messages (i.e., in the useEffect that calls reset and setSelectedIndices) so the effect only reruns when the number of messages actually changes.
28-66: Adddata-testidtoMessagePreviewcheckbox for testability.Per coding guidelines, new interactive UI elements should have
data-testidattributes following the<entity>-<element>-<qualifier>pattern.💡 Proposed fix
<Checkbox checked={selected} onCheckedChange={onToggle} className="mt-1 shrink-0" + data-testid={`message-preview-checkbox-${sessionMessage.order_index}`} />As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/sheets/commitVersionSheet.tsx` around lines 28 - 66, Add a data-testid to the interactive Checkbox inside the MessagePreview component so tests can target it; specifically, update the Checkbox in MessagePreview to include a data-testid attribute using the prescribed pattern (e.g., "messagepreview-checkbox-selected" or similar that follows <entity>-<element>-<qualifier>), ensuring the attribute stays on the Checkbox element that uses checked={selected} and onCheckedChange={onToggle} so test authors can reliably locate and toggle it.
152-159: Adddata-testidto the toggle all button.💡 Proposed fix
<button type="button" onClick={toggleAll} className="text-xs text-muted-foreground hover:text-foreground transition-colors" + data-testid="commit-version-toggle-all" > {allSelected ? 'Deselect all' : 'Select all'} </button>As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/sheets/commitVersionSheet.tsx` around lines 152 - 159, The toggle-all button rendered in the CommitVersionSheet component (the button using onClick={toggleAll} and label derived from allSelected) needs a data-testid attribute for testing; update that button element to include a descriptive test id such as data-testid="commit-version-toggle-all" (or similar consistent naming) so automated tests can target it.transports/bifrost-http/handlers/prompts.go (1)
1013-1016: Error message is confusing when session has no messages.When
session.Messagesis empty, the error becomes"message index X out of range (0--1)". Consider a clearer message for this edge case.💡 Proposed improvement
for _, idx := range req.MessageIndices { - if idx < 0 || idx >= len(session.Messages) { - SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("message index %d out of range (0-%d)", idx, len(session.Messages)-1)) + if len(session.Messages) == 0 { + SendError(ctx, fasthttp.StatusBadRequest, "session has no messages") + return + } + if idx < 0 || idx >= len(session.Messages) { + SendError(ctx, fasthttp.StatusBadRequest, fmt.Sprintf("message index %d out of range (0-%d)", idx, len(session.Messages)-1)) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/prompts.go` around lines 1013 - 1016, The current range error builds "(0-%d)" using len(session.Messages)-1 which yields "(0--1)" when session.Messages is empty; update the error handling in the block around session.Messages, idx and SendError so you check len(session.Messages) == 0 first and return a clear message like "no messages in session" (or "message index %d out of range: session has no messages"), otherwise keep the existing fmt.Sprintf path that prints the valid range 0-(len(session.Messages)-1); reference session.Messages, idx, SendError and fmt.Sprintf in your change.
🤖 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/bifrost-http/handlers/prompts.go`:
- Around line 1010-1030: The code builds version messages from
req.MessageIndices but does not check for duplicate indices so repeated indices
produce duplicate TablePromptVersionMessage entries; update the handler to
deduplicate indices before appending by tracking seen indices (map[int]bool)
when iterating req.MessageIndices, validate bounds with the same checks using
SendError, skip any index already seen, and only append messages for the first
occurrence (use req.MessageIndices, session.Messages, SendError, and
tables.TablePromptVersionMessage to locate the relevant code).
---
Outside diff comments:
In `@ui/components/prompts/components/promptsViewHeader.tsx`:
- Around line 111-136: The handleCommitVersion callback closes over sessions and
selectedSessionId but they are not listed in the useCallback dependency array,
which can lead to stale values; update the dependency array for useCallback that
defines handleCommitVersion to include sessions and selectedSessionId (in
addition to the existing dependencies like selectedPrompt?.id, messages,
buildSaveParams, provider, model, createSession, setUrlState, onSessionSaved,
hasChanges) so the closure always sees the latest session list and selected
session id.
In `@ui/components/prompts/sheets/commitVersionSheet.tsx`:
- Around line 111-135: Prevent submitting an empty selection by adding a
selection check in onSubmit and disabling the form submit control: inside
onSubmit (function onSubmit) early-return with a toast.error if neither
allSelected is true nor selectedIndices has any entries (e.g.,
selectedIndices.size === 0), and also update the submit button in the
CommitVersionSheet UI to be disabled when there are no selected messages (use
the same condition: !allSelected && selectedIndices.size === 0) so the user
cannot submit a commit with zero messages.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/prompts.go`:
- Around line 1013-1016: The current range error builds "(0-%d)" using
len(session.Messages)-1 which yields "(0--1)" when session.Messages is empty;
update the error handling in the block around session.Messages, idx and
SendError so you check len(session.Messages) == 0 first and return a clear
message like "no messages in session" (or "message index %d out of range:
session has no messages"), otherwise keep the existing fmt.Sprintf path that
prints the valid range 0-(len(session.Messages)-1); reference session.Messages,
idx, SendError and fmt.Sprintf in your change.
In `@ui/components/prompts/sheets/commitVersionSheet.tsx`:
- Around line 81-87: The effect that resets the form and selection uses
session.messages in its dependency array which can change by reference and
trigger unnecessary runs; update the dependency to session.messages.length and
keep the body using session.messages (i.e., in the useEffect that calls reset
and setSelectedIndices) so the effect only reruns when the number of messages
actually changes.
- Around line 28-66: Add a data-testid to the interactive Checkbox inside the
MessagePreview component so tests can target it; specifically, update the
Checkbox in MessagePreview to include a data-testid attribute using the
prescribed pattern (e.g., "messagepreview-checkbox-selected" or similar that
follows <entity>-<element>-<qualifier>), ensuring the attribute stays on the
Checkbox element that uses checked={selected} and onCheckedChange={onToggle} so
test authors can reliably locate and toggle it.
- Around line 152-159: The toggle-all button rendered in the CommitVersionSheet
component (the button using onClick={toggleAll} and label derived from
allSelected) needs a data-testid attribute for testing; update that button
element to include a descriptive test id such as
data-testid="commit-version-toggle-all" (or similar consistent naming) so
automated tests can target it.
🪄 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: 5a553f81-bad1-4a0a-a933-9296cbbc4c3d
📒 Files selected for processing (4)
transports/bifrost-http/handlers/prompts.goui/components/prompts/components/promptsViewHeader.tsxui/components/prompts/sheets/commitVersionSheet.tsxui/lib/types/prompts.ts
b3e7ea8 to
a426c21
Compare
dd314d0 to
9ef5b72
Compare
a426c21 to
c7ddf8c
Compare
c7ddf8c to
3732c49
Compare
9ef5b72 to
ae921da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/components/prompts/components/promptsViewHeader.tsx (1)
111-136:⚠️ Potential issue | 🟡 MinorMissing dependencies in
useCallbackmay cause stale closure bugs.The
handleCommitVersioncallback referencessessionsandselectedSessionIdin the!hasChangesbranch, but these are not included in the dependency array. This can cause the callback to use stale values.🛡️ Proposed fix to add missing dependencies
- }, [selectedPrompt?.id, messages, buildSaveParams, provider, model, createSession, setUrlState, onSessionSaved, hasChanges]); + }, [selectedPrompt?.id, messages, buildSaveParams, provider, model, createSession, setUrlState, onSessionSaved, hasChanges, sessions, selectedSessionId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/components/promptsViewHeader.tsx` around lines 111 - 136, The useCallback for handleCommitVersion captures sessions and selectedSessionId in the early-return branch but they are missing from the dependency array, which can produce stale closures; update the dependency list for handleCommitVersion to include sessions and selectedSessionId (in addition to the existing selectedPrompt?.id, messages, buildSaveParams, provider, model, createSession, setUrlState, onSessionSaved, hasChanges) so the callback rebinds when those values change.
🧹 Nitpick comments (2)
ui/components/prompts/sheets/commitVersionSheet.tsx (2)
44-49: Adddata-testidto the Checkbox for test discoverability.Per coding guidelines, new interactive UI elements should have
data-testidattributes following the<entity>-<element>-<qualifier>pattern.🧪 Proposed fix
<Checkbox checked={selected} onCheckedChange={onToggle} className="mt-1 shrink-0" + data-testid="message-preview-checkbox" />As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/sheets/commitVersionSheet.tsx` around lines 44 - 49, The Checkbox component instance in commitVersionSheet.tsx (the JSX element with props checked, onCheckedChange and className) needs a data-testid attribute for test discoverability; add a data-testid following the pattern "<entity>-<element>-<qualifier>" (e.g., "commit-version-sheet-checkbox-selected" or similar) to the Checkbox JSX so tests can reliably query it.
156-162: Adddata-testidto the toggle all button.The "Select all / Deselect all" button is an interactive element that should have a
data-testidfor E2E testing.🧪 Proposed fix
<button type="button" onClick={toggleAll} className="text-xs text-muted-foreground hover:text-foreground transition-colors" + data-testid="commit-version-toggle-all" >As per coding guidelines:
ui/**/*.{tsx,ts}: Add new interactive UI elements withdata-testidattributes following the pattern:data-testid="<entity>-<element>-<qualifier>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/prompts/sheets/commitVersionSheet.tsx` around lines 156 - 162, Add a data-testid attribute to the "Select all / Deselect all" toggle button so E2E tests can target it: update the button in the CommitVersionSheet component (the button that calls toggleAll and reads allSelected) to include data-testid following the project pattern, e.g. data-testid="commit-version-toggle-all" (replace with exact entity-element-qualifier if your naming conventions require a different entity).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/components/prompts/sheets/commitVersionSheet.tsx`:
- Line 101: The computed allSelected is true when session.messages.length is 0
because selectedIndices.size === 0; update the logic in CommitVersionSheet to
check that there are messages first (e.g., make allSelected depend on
session.messages.length > 0) so it becomes false for empty sessions; locate the
expression using allSelected (and the symbols selectedIndices and
session.messages) and change it to require a non-zero message count before
treating all items as selected.
---
Outside diff comments:
In `@ui/components/prompts/components/promptsViewHeader.tsx`:
- Around line 111-136: The useCallback for handleCommitVersion captures sessions
and selectedSessionId in the early-return branch but they are missing from the
dependency array, which can produce stale closures; update the dependency list
for handleCommitVersion to include sessions and selectedSessionId (in addition
to the existing selectedPrompt?.id, messages, buildSaveParams, provider, model,
createSession, setUrlState, onSessionSaved, hasChanges) so the callback rebinds
when those values change.
---
Nitpick comments:
In `@ui/components/prompts/sheets/commitVersionSheet.tsx`:
- Around line 44-49: The Checkbox component instance in commitVersionSheet.tsx
(the JSX element with props checked, onCheckedChange and className) needs a
data-testid attribute for test discoverability; add a data-testid following the
pattern "<entity>-<element>-<qualifier>" (e.g.,
"commit-version-sheet-checkbox-selected" or similar) to the Checkbox JSX so
tests can reliably query it.
- Around line 156-162: Add a data-testid attribute to the "Select all / Deselect
all" toggle button so E2E tests can target it: update the button in the
CommitVersionSheet component (the button that calls toggleAll and reads
allSelected) to include data-testid following the project pattern, e.g.
data-testid="commit-version-toggle-all" (replace with exact
entity-element-qualifier if your naming conventions require a different entity).
🪄 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: 0f85e2c5-cb62-4557-a884-05815d6996cc
📒 Files selected for processing (3)
transports/bifrost-http/handlers/prompts.goui/components/prompts/components/promptsViewHeader.tsxui/components/prompts/sheets/commitVersionSheet.tsx
ae921da to
54c7bb4
Compare
3732c49 to
a5c5d4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bifrost-http/handlers/prompts.go`:
- Around line 1037-1054: The current branch iterates client-supplied
req.MessageIndices and appends messages in that order, which reorders the
session; instead build a set from req.MessageIndices (deduplicating and
validating indices) and then iterate session.Messages in their original order,
appending any message whose index is present in the set into messages (use the
same validation logic and SendError on out-of-range indices); update the code
paths using session.Messages, tables.TablePromptVersionMessage, and
req.MessageIndices accordingly so output preserves original session order while
filtering.
🪄 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: 8cd6dca8-2b4e-41d0-9f7d-b3722b5f9ea4
📒 Files selected for processing (4)
transports/bifrost-http/handlers/prompts.goui/components/prompts/components/promptsViewHeader.tsxui/components/prompts/sheets/commitVersionSheet.tsxui/lib/types/prompts.ts
✅ Files skipped from review due to trivial changes (2)
- ui/lib/types/prompts.ts
- ui/components/prompts/sheets/commitVersionSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/components/prompts/components/promptsViewHeader.tsx
Merge activity
|
54c7bb4 to
086b264
Compare
a5c5d4a to
075f66e
Compare
075f66e to
1fa8785
Compare
086b264 to
a808635
Compare
The base branch was changed.

Summary
Add selective message inclusion when committing prompt sessions as versions. Users can now choose which specific messages from a session to include in the committed version instead of always including all messages.
Changes
message_indicesoptional field toCommitSessionRequestto specify which messages to include (0-based indices)Type of change
Affected areas
How to test
Screenshots/Recordings
The commit version sheet now displays a scrollable list of messages with checkboxes, allowing users to selectively include messages in the version.
Breaking changes
The
message_indicesfield is optional, maintaining backward compatibility.Related issues
Enables more granular version control for prompt engineering workflows.
Security considerations
Added validation to ensure message indices are within valid bounds to prevent out-of-range access.
Checklist
docs/contributing/README.mdand followed the guidelines