fix: gRPC request loses all messages except the first on save for yaml collection#7910
Open
sharan-bruno wants to merge 8 commits intousebruno:mainfrom
Conversation
sharan-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughParser and serializer now treat ChangesgRPC Multi-Message Support
Sequence Diagram(s)sequenceDiagram
participant UI as Test UI (Playwright)
participant App as App (Request Editor)
participant FS as Filestore (YML/BRU serializer)
participant Parser as Parser (parseGrpcRequest)
UI->>App: create collection (format) & open request editor
UI->>App: select gRPC, choose method, add messages, generate samples
App->>FS: stringify request (stringifyGrpcRequest) -> write file
FS->>Parser: read file -> parseGrpcRequest
Parser-->>App: parsed brunoRequest.body.grpc entries (multiple)
App-->>UI: file saved and verified
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/grpc/multi-message-yml/multi-message.spec.ts (1)
46-58: 💤 Low valueWrap the verification steps in
test.stepfor better report output.The second test has no
test.stepwrapping, so all assertion failures will appear flat in the test report. Per coding guidelines, steps should be used as much as possible.♻️ Proposed refactor
test('verifies all messages are saved in the request .yml file', async () => { - const requestFilePath = path.join(collectionPath, COLLECTION_NAME, `${REQUEST_NAME}.yml`); - expect(fs.existsSync(requestFilePath)).toBe(true); - - const parsed = yaml.load(fs.readFileSync(requestFilePath, 'utf8')) as GrpcRequestYml; - const messages = parsed.grpc?.message ?? []; - - expect(messages).toHaveLength(MESSAGE_COUNT); - for (const variant of messages) { - expect(variant.title?.length ?? 0).toBeGreaterThan(0); - expect(variant.message?.trim().length ?? 0).toBeGreaterThan(0); - } + const requestFilePath = path.join(collectionPath, COLLECTION_NAME, `${REQUEST_NAME}.yml`); + + await test.step('yml file exists on disk', async () => { + expect(fs.existsSync(requestFilePath)).toBe(true); + }); + + await test.step(`parsed yml contains ${MESSAGE_COUNT} messages with non-empty title and body`, async () => { + const parsed = yaml.load(fs.readFileSync(requestFilePath, 'utf8')) as GrpcRequestYml; + const messages = parsed.grpc?.message ?? []; + expect(messages).toHaveLength(MESSAGE_COUNT); + for (const variant of messages) { + expect(variant.title?.length ?? 0).toBeGreaterThan(0); + expect(variant.message?.trim().length ?? 0).toBeGreaterThan(0); + } + }); });🤖 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 `@tests/grpc/multi-message-yml/multi-message.spec.ts` around lines 46 - 58, Wrap the assertions inside the 'verifies all messages are saved in the request .yml file' test into one or more test.step calls so failures are reported as steps; specifically, place the filesystem existence check for requestFilePath, the yaml parsing into parsed (GrpcRequestYml) and extraction of messages, the expect(messages).toHaveLength(MESSAGE_COUNT), and the per-variant assertions on variant.title and variant.message inside a test.step (using a descriptive step name like "validate saved gRPC messages") so the checks for requestFilePath, parsed, messages, MESSAGE_COUNT, and each variant validation are executed under that step.
🤖 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 `@packages/bruno-filestore/src/formats/yml/items/parseGrpcRequest.ts`:
- Around line 60-65: The code assumes grpc?.message is an array by casting to
GrpcMessageVariant[] and then calling variants.map, which crashes if
grpc.message is a legacy string; update the logic in parseGrpcRequest to check
Array.isArray(grpc?.message) before treating it as variants (keep using the
existing variables grpc, variants, brunoRequest.body.grpc and ensureString), and
skip the mapping when it's not an array so legacy string messages are treated as
“no messages” instead of invoking variants.map on a non-array.
---
Nitpick comments:
In `@tests/grpc/multi-message-yml/multi-message.spec.ts`:
- Around line 46-58: Wrap the assertions inside the 'verifies all messages are
saved in the request .yml file' test into one or more test.step calls so
failures are reported as steps; specifically, place the filesystem existence
check for requestFilePath, the yaml parsing into parsed (GrpcRequestYml) and
extraction of messages, the expect(messages).toHaveLength(MESSAGE_COUNT), and
the per-variant assertions on variant.title and variant.message inside a
test.step (using a descriptive step name like "validate saved gRPC messages") so
the checks for requestFilePath, parsed, messages, MESSAGE_COUNT, and each
variant validation are executed under that step.
🪄 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: 4c9f2891-4a7f-478a-81bb-db18253f2cb0
📒 Files selected for processing (4)
packages/bruno-filestore/src/formats/yml/items/parseGrpcRequest.tspackages/bruno-filestore/src/formats/yml/items/stringifyGrpcRequest.tstests/grpc/multi-message-yml/multi-message.spec.tstests/utils/page/actions.ts
… improve message handling
sharan-bruno
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/grpc/multi-message-yml/multi-message.spec.ts (2)
67-71: 💤 Low valueBRU verification only checks block count, not message content
Unlike the YML path, the
brubranch doesn't assert that each message block has non-empty body content. A bug that persists structurally correct but emptybody:grpcblocks would pass this test.Consider adding a content-presence check, e.g.:
♻️ Proposed refactor
} else { // .bru stores each gRPC message as its own `body:grpc { ... }` block. const blockCount = (fileContent.match(/^body:grpc\b/gm) ?? []).length; expect(blockCount).toBe(MESSAGE_COUNT); + // Also verify each block has non-empty content (not just the structural wrapper) + const nonEmptyBlocks = (fileContent.match(/^body:grpc\b[\s\S]*?\{[^}]+\}/gm) ?? []).length; + expect(nonEmptyBlocks).toBe(MESSAGE_COUNT); }🤖 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 `@tests/grpc/multi-message-yml/multi-message.spec.ts` around lines 67 - 71, The test currently only counts occurrences of "body:grpc" using fileContent and blockCount against MESSAGE_COUNT; update the bru branch to also verify each "body:grpc { ... }" block contains non-empty message content by parsing all matches (e.g., using a global regex to capture the block bodies) and asserting that every captured body string is not empty/whitespace. Locate the block where blockCount is computed and add an assertion loop that iterates over the captured blocks from fileContent, checking each block's trimmed content is truthy and failing the test if any are empty.
34-50: ⚡ Quick winWrap logical phases with
test.stepfor readable test reportsPer coding guidelines,
test.stepshould be promoted as much as possible. Grouping the create/configure/save phases makes CI report failures much easier to locate.♻️ Proposed refactor
test('creates a gRPC request with multiple messages and saves it', async ({ page, createTmpDir }) => { collectionPath = await createTmpDir(tmpDirPrefix); - await createCollection(page, collectionName, collectionPath, { format }); - await createRequest(page, REQUEST_NAME, collectionName, { url: GRPC_URL, requestType: 'gRPC' }); - - await selectGrpcMethod(page, GRPC_METHOD); - - await addGrpcMessage(page); - await addGrpcMessage(page); - - await generateGrpcSampleMessage(page, 0); - await generateGrpcSampleMessage(page, 1); - await generateGrpcSampleMessage(page, 2); - - await saveRequest(page); + await test.step('Setup collection and request', async () => { + await createCollection(page, collectionName, collectionPath, { format }); + await createRequest(page, REQUEST_NAME, collectionName, { url: GRPC_URL, requestType: 'gRPC' }); + await selectGrpcMethod(page, GRPC_METHOD); + }); + + await test.step(`Add ${MESSAGE_COUNT - 1} extra messages and generate samples`, async () => { + await addGrpcMessage(page); + await addGrpcMessage(page); + await generateGrpcSampleMessage(page, 0); + await generateGrpcSampleMessage(page, 1); + await generateGrpcSampleMessage(page, 2); + }); + + await test.step('Save request', async () => { + await saveRequest(page); + }); });As per coding guidelines: "Promote the use of
test.stepas much as possible so the generated reports are easier to read."🤖 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 `@tests/grpc/multi-message-yml/multi-message.spec.ts` around lines 34 - 50, Wrap the test's logical phases in test.step to improve report readability: group collection setup (createTmpDir + createCollection) in one test.step, request creation/configuration (createRequest + selectGrpcMethod + addGrpcMessage x2) in another test.step, sample generation (generateGrpcSampleMessage calls) in a third, and saving (saveRequest) in a final step; update the test named 'creates a gRPC request with multiple messages and saves it' to call test.step with descriptive titles around those symbol sets so failures are scoped to the appropriate phase.
🤖 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 `@tests/utils/page/actions.ts`:
- Around line 1252-1256: generateGrpcSampleMessage currently clicks the
regenerate button and returns immediately, which can race with async sample
generation; update generateGrpcSampleMessage to, after clicking
`#regenerate-msg-${index} button`, wait/assert that the gRPC message editor is
populated (e.g., wait for the editor/content locator to contain non-empty text
or a specific DOM change) before returning so subsequent saveRequest won't race;
also change the locator to use a data-testid like
`data-testid="regenerate-msg-${index}"` (and update the component) instead of
the CSS ID to make the selector more robust.
---
Nitpick comments:
In `@tests/grpc/multi-message-yml/multi-message.spec.ts`:
- Around line 67-71: The test currently only counts occurrences of "body:grpc"
using fileContent and blockCount against MESSAGE_COUNT; update the bru branch to
also verify each "body:grpc { ... }" block contains non-empty message content by
parsing all matches (e.g., using a global regex to capture the block bodies) and
asserting that every captured body string is not empty/whitespace. Locate the
block where blockCount is computed and add an assertion loop that iterates over
the captured blocks from fileContent, checking each block's trimmed content is
truthy and failing the test if any are empty.
- Around line 34-50: Wrap the test's logical phases in test.step to improve
report readability: group collection setup (createTmpDir + createCollection) in
one test.step, request creation/configuration (createRequest + selectGrpcMethod
+ addGrpcMessage x2) in another test.step, sample generation
(generateGrpcSampleMessage calls) in a third, and saving (saveRequest) in a
final step; update the test named 'creates a gRPC request with multiple messages
and saves it' to call test.step with descriptive titles around those symbol sets
so failures are scoped to the appropriate phase.
🪄 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: c595b337-7c8f-45a7-b4ba-5f3120143c99
📒 Files selected for processing (2)
tests/grpc/multi-message-yml/multi-message.spec.tstests/utils/page/actions.ts
…ated and improve request creation button interaction
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/utils/page/actions.ts (1)
1241-1245: ⚡ Quick win
addGrpcMessageshould assert the new entry appeared before returning.A click with no follow-up assertion means a missed click or slow render will surface as a confusing
#regenerate-msg-2 not foundfailure insidegenerateGrpcSampleMessagerather than here where the add actually failed.✅ Proposed fix
const addGrpcMessage = async (page: Page) => { await test.step('Add gRPC message', async () => { + const countBefore = await page.getByTestId('grpc-message-item').count(); await page.getByTestId('grpc-add-message-button').click(); + await expect(page.getByTestId('grpc-message-item')).toHaveCount(countBefore + 1); }); };Adjust
grpc-message-itemto whateverdata-testidis on the per-message container in the production component.🤖 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 `@tests/utils/page/actions.ts` around lines 1241 - 1245, The addGrpcMessage helper doesn't verify that a new message row appears after clicking; update the addGrpcMessage function to wait/assert for the per-message container to be present (e.g., using the test id used by the production component, such as 'grpc-message-item' or the actual per-message data-testid) after clicking the 'grpc-add-message-button' so the step fails here on add problems rather than later in generateGrpcSampleMessage; ensure you use the same locator used elsewhere (e.g., getByTestId('grpc-message-item') or the real per-message test id) and await a visibility/existence assertion before returning from addGrpcMessage.tests/grpc/multi-message-yml/multi-message.spec.ts (1)
30-69: ⚡ Quick winAdd
afterAllteardown to remove created collections from the app sidebar.Neither
describe.serialblock removes its collection after the tests complete. If these suites run before other tests that expect a clean sidebar (or if parallel workers share state), the leftovergrpc-yml-multi-msg/grpc-bru-multi-msgcollections can cause interference.✅ Proposed addition
+import { removeCollection } from '../../utils/page/actions'; for (const { format, collectionName, tmpDirPrefix } of FORMATS) { test.describe.serial(`grpc multi-message (${format} format)`, () => { let collectionPath: string; + let sharedPage: Parameters<Parameters<typeof test>[1]>[0]['page']; + test.afterAll(async () => { + if (sharedPage) { + await removeCollection(sharedPage, collectionName); + } + }); test('creates a gRPC request with multiple messages and saves it', async ({ page, createTmpDir }) => { + sharedPage = page; collectionPath = await createTmpDir(tmpDirPrefix); ...🤖 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 `@tests/grpc/multi-message-yml/multi-message.spec.ts` around lines 30 - 69, Add a teardown to each describe.serial to remove the created collection from the app sidebar: inside the `test.describe.serial` block add a `test.afterAll(async ({ page }) => { ... })` that uses the existing UI helper to remove the created collection by `collectionName` (e.g., call your project’s `deleteCollection`/`removeCollection` helper via `await deleteCollection(page, collectionName)`), and optionally remove the temporary directory using `collectionPath`/`createTmpDir` cleanup if a helper exists; place this afterAll next to the tests so the `grpc-yml-multi-msg` / `grpc-bru-multi-msg` collections do not persist between suites.
🤖 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.
Nitpick comments:
In `@tests/grpc/multi-message-yml/multi-message.spec.ts`:
- Around line 30-69: Add a teardown to each describe.serial to remove the
created collection from the app sidebar: inside the `test.describe.serial` block
add a `test.afterAll(async ({ page }) => { ... })` that uses the existing UI
helper to remove the created collection by `collectionName` (e.g., call your
project’s `deleteCollection`/`removeCollection` helper via `await
deleteCollection(page, collectionName)`), and optionally remove the temporary
directory using `collectionPath`/`createTmpDir` cleanup if a helper exists;
place this afterAll next to the tests so the `grpc-yml-multi-msg` /
`grpc-bru-multi-msg` collections do not persist between suites.
In `@tests/utils/page/actions.ts`:
- Around line 1241-1245: The addGrpcMessage helper doesn't verify that a new
message row appears after clicking; update the addGrpcMessage function to
wait/assert for the per-message container to be present (e.g., using the test id
used by the production component, such as 'grpc-message-item' or the actual
per-message data-testid) after clicking the 'grpc-add-message-button' so the
step fails here on add problems rather than later in generateGrpcSampleMessage;
ensure you use the same locator used elsewhere (e.g.,
getByTestId('grpc-message-item') or the real per-message test id) and await a
visibility/existence assertion before returning from addGrpcMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42167a8c-8d89-493d-96ee-15c25280f2ac
📒 Files selected for processing (2)
tests/grpc/multi-message-yml/multi-message.spec.tstests/utils/page/actions.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/utils/page/actions.ts`:
- Around line 133-141: The createRequest routine advertises four request types
but only switches the UI when requestType === 'gRPC', causing GraphQL/WebSocket
to fall back to the default; update createRequest to handle any non-default
CreateRequestOptions['requestType'] by looking up
REQUEST_TYPE_TESTID[requestType] and clicking that test-id before filling the
form (mirror the existing gRPC branch logic), and apply the same change to the
other occurrence referenced around the second block (the similar branch at lines
~309-311) so GraphQL and WebSocket selections properly trigger their UI.
- Around line 110-117: The file-format toggle click is non-deterministic: before
selecting `bru` ensure the "show-file-format-toggle" is enabled rather than
blindly clicking it. In the block inside tests/utils/page/actions.ts that
references createCollectionModal and
page.getByTestId('show-file-format-toggle'), read the toggle's current state
(e.g., check aria-pressed / checked attribute or use isChecked/isPressed helper)
and only click the toggle if it is not already enabled, then proceed to
createCollectionModal.locator('select#format').selectOption(format); this
guarantees the format control is visible before selecting `bru`.
🪄 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: c0251c95-5ec7-48a4-9c0b-8e00064f7bc9
📒 Files selected for processing (4)
packages/bruno-app/src/components/Sidebar/CreateCollection/index.jspackages/bruno-filestore/src/formats/yml/items/parseGrpcRequest.tspackages/bruno-filestore/src/formats/yml/items/stringifyGrpcRequest.tstests/utils/page/actions.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-app/src/components/Sidebar/CreateCollection/index.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-filestore/src/formats/yml/items/stringifyGrpcRequest.ts
sanish-bruno
suggested changes
May 7, 2026
| */ | ||
| const generateGrpcSampleMessage = async (page: Page, index: number = 0) => { | ||
| await test.step(`Generate sample for gRPC message #${index}`, async () => { | ||
| await page.locator(`#regenerate-msg-${index} button`).click(); |
Contributor
Author
There was a problem hiding this comment.
addressed - added test id for this locator
| */ | ||
| const saveRequest = async (page: Page) => { | ||
| await test.step('Save request', async () => { | ||
| await page.evaluate(() => (document.activeElement as HTMLElement | null)?.blur()); |
Contributor
Author
There was a problem hiding this comment.
yes we need this line, without that the playwrite test cases are failing
|
|
||
| const REQUEST_TYPE_TESTID: Record<NonNullable<CreateRequestOptions['requestType']>, string> = { | ||
| HTTP: 'http-request', | ||
| GraphQL: 'graphql-request', |
Collaborator
There was a problem hiding this comment.
@sharan-bruno can we use all capital for keys? also export this, can use object.freeze as this behaves more like an ENUM
Contributor
Author
There was a problem hiding this comment.
Thank you for the suggestion, addressed this - added object.freeze
| collectionPath = await createTmpDir(tmpDirPrefix); | ||
|
|
||
| await createCollection(page, collectionName, collectionPath, { format }); | ||
| await createRequest(page, REQUEST_NAME, collectionName, { url: GRPC_URL, requestType: 'gRPC' }); |
Collaborator
There was a problem hiding this comment.
can we use imported requestType object and use it to add requestType rather than hardcoding it?
Contributor
Author
There was a problem hiding this comment.
addressed this, removed this hardcoded requestType
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
2 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.
Description
Jira link: https://usebruno.atlassian.net/browse/BRU-3271
gRPC request loses all messages except the first on save for yaml collection
In YAML collections, saving a gRPC request with multiple client messages only persisted the first message. On reload, all additional messages were lost.
This happened because both the YAML stringifier and parser handled grpc.message as a single string (legacy behavior from single-message support), instead of supporting multiple messages.
Fix
Updated stringifier to save grpc.message as an array (GrpcMessageVariant[])
Updated parser to read and restore all messages correctly
Added an end-to-end test to verify multiple messages are persisted
Minor test utility improvements for gRPC request handling
Compatibility
The fix matches the shape already used by the .bru format and by the
opencollection schema (
GrpcMessageVariant[]), so requests round-trip cleanlybetween formats. Existing yml requests written by older versions (with
grpc.messageas a string) won't auto-migrate — they'll be treated as havingno messages until re-saved. If we want to preserve those, we can add a fallback
branch in the parser.
Contribution Checklist:
Before:
After:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Tests
Chores