fix: avoid round trip loss of annotation data#7730
Open
sid-bruno wants to merge 1 commit intousebruno:mainfrom
Open
fix: avoid round trip loss of annotation data#7730sid-bruno wants to merge 1 commit intousebruno:mainfrom
sid-bruno wants to merge 1 commit intousebruno:mainfrom
Conversation
sid-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughThis PR adds first-class support for annotations across the Bruno application and Electron packages. Changes include Redux store mappings for query parameters and headers, serialization utilities for filesystem persistence, TypeScript schema type definitions for annotations on variables and key-value pairs, and Yup validation schemas to enforce annotation structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. 🔧 ast-grep (0.42.1)packages/bruno-app/src/providers/ReduxStore/slices/collections/index.jsThanks 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reintroduces/extends support for preserving annotation metadata across schema validation and save/export transformations, aiming to prevent annotation loss during read → write round trips.
Changes:
- Extend
@usebruno/schemaYup validators to acceptannotationson multiple pair-like structures (headers, params, vars, env vars, multipart entries, file entries). - Propagate
annotationsthrough request/collection transformation utilities inbruno-appandbruno-electron. - Add schema and transformation tests, and introduce
Annotationtypings inbruno-schema-types.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bruno-schema/src/collections/index.js | Adds annotationSchema and allows annotations on multiple collection/request structures. |
| packages/bruno-schema/src/collections/annotationsSchema.spec.js | New Jest spec verifying schemas accept annotations in several locations. |
| packages/bruno-schema-types/src/common/variables.ts | Adds annotations to Variable. |
| packages/bruno-schema-types/src/common/key-value.ts | Adds annotations to KeyValue (covers headers/params via inheritance). |
| packages/bruno-schema-types/src/common/index.ts | Re-exports Annotation type. |
| packages/bruno-schema-types/src/common/annotation.ts | New Annotation interface. |
| packages/bruno-schema-types/src/collection/environment.ts | Adds annotations to EnvironmentVariable. |
| packages/bruno-electron/src/utils/collection.js | Preserves annotations when projecting request params/headers to filesystem shape. |
| packages/bruno-electron/src/utils/tests/collection-utils.spec.js | Updates transformation expectations to include annotations. |
| packages/bruno-app/src/utils/collections/index.js | Preserves annotations in multiple save/export projections (headers/params and request transforms). |
| packages/bruno-app/src/utils/collections/index.spec.js | Adds test asserting request transform preserves header/param annotations. |
| packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js | Ensures headers/query params reducers carry annotations through set operations. |
Comments suppressed due to low confidence (1)
packages/bruno-app/src/utils/collections/index.js:201
transformCollectionToSaveToExportAsFilenow preservesannotationsfor request headers/params, but the other body pair projections (copyFormUrlEncodedParams,copyMultipartFormParams,copyFileParams) still omitannotations. Since the schema now accepts annotations on formUrlEncoded/multipart/file entries, exporting will still drop annotation data for those sections; these copy helpers should also pass throughannotationsto fully avoid round-trip loss.
export const transformCollectionToSaveToExportAsFile = (collection, options = {}) => {
const copyHeaders = (headers) => {
return map(headers, (header) => {
return {
uid: header.uid,
name: header.name,
value: header.value,
description: header.description,
annotations: header.annotations,
enabled: header.enabled
};
});
};
const copyParams = (params) => {
return map(params, (param) => {
return {
uid: param.uid,
name: param.name,
value: param.value,
description: param.description,
annotations: param.annotations,
type: param.type,
enabled: param.enabled
};
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+137
to
+142
| Yup.object({ | ||
| name: Yup.string().min(1).required('annotation name is required'), | ||
| value: Yup.string().nullable() | ||
| }) | ||
| .noUnknown(true) | ||
| .strict() |
Comment on lines
1
to
6
| export type { UID } from './uid'; | ||
| export type { KeyValue } from './key-value'; | ||
| export type { Variable, Variables } from './variables'; | ||
| export type { Annotation } from './annotation'; | ||
| export type { MultipartFormEntry, MultipartForm } from './multipart-form'; | ||
| export type { FileEntry, FileList } from './file'; |
Comment on lines
+1
to
+4
| const { itemSchema, environmentSchema, collectionSchema } = require('./index'); | ||
|
|
||
| describe('annotation acceptance', () => { | ||
| test('itemSchema accepts annotations on headers and params', async () => { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/bruno-schema/src/collections/index.js (1)
134-144: DRY violation: duplicate annotation schema definition.This inline object schema duplicates
annotationSchemaexactly. All other usages (environmentVariablesSchema, keyValueSchema, varsSchema, fileSchemaWithAnnotations, requestParamsSchema) referenceannotationSchemadirectly, butmultipartFormSchemadefines it inline.♻️ Use annotationSchema reference
// Optional annotations on multipart entries annotations: Yup.array() .of( - Yup.object({ - name: Yup.string().min(1).required('annotation name is required'), - value: Yup.string().nullable() - }) - .noUnknown(true) - .strict() + annotationSchema ) .nullable(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-schema/src/collections/index.js` around lines 134 - 144, The multipartFormSchema currently inlines the same object as annotationSchema, violating DRY; update multipartFormSchema to reference the existing annotationSchema (used by environmentVariablesSchema, keyValueSchema, varsSchema, fileSchemaWithAnnotations, requestParamsSchema) instead of redefining the Yup.object(...) inline so all annotation validations come from the single annotationSchema symbol.packages/bruno-app/src/utils/collections/index.spec.js (1)
42-66: UID lengths are inconsistent with schema requirements.The schema expects 21-character alphanumeric UIDs, but
requestuid123456789012andheaderuid123456789012are 22 characters. While this test exercises the transform function directly and may not invoke schema validation, it's good practice to use valid fixtures.🔧 Suggested fix
const item = { - uid: 'requestuid123456789012', + uid: 'requestuid12345678901', type: 'http-request', ... { - uid: 'headeruid123456789012', + uid: 'headeruid12345678901', name: 'X-Test',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/collections/index.spec.js` around lines 42 - 66, Fixture UIDs in the test object are 22 (or otherwise non-21) characters and must be adjusted to match the 21-character alphanumeric schema; update the uid fields used in this spec (specifically the values 'requestuid123456789012', 'headeruid123456789012', and 'paramuid1234567890123') to be 21 characters (e.g., remove one trailing character or otherwise trim to 21 chars) and verify any other fixture uid strings in the same test also conform to the 21-character requirement so the test data matches the schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-app/src/utils/collections/index.spec.js`:
- Around line 42-66: Fixture UIDs in the test object are 22 (or otherwise
non-21) characters and must be adjusted to match the 21-character alphanumeric
schema; update the uid fields used in this spec (specifically the values
'requestuid123456789012', 'headeruid123456789012', and 'paramuid1234567890123')
to be 21 characters (e.g., remove one trailing character or otherwise trim to 21
chars) and verify any other fixture uid strings in the same test also conform to
the 21-character requirement so the test data matches the schema.
In `@packages/bruno-schema/src/collections/index.js`:
- Around line 134-144: The multipartFormSchema currently inlines the same object
as annotationSchema, violating DRY; update multipartFormSchema to reference the
existing annotationSchema (used by environmentVariablesSchema, keyValueSchema,
varsSchema, fileSchemaWithAnnotations, requestParamsSchema) instead of
redefining the Yup.object(...) inline so all annotation validations come from
the single annotationSchema symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66e5177e-dc6d-4ad0-b744-49988cce39f1
📒 Files selected for processing (12)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/collections/index.jspackages/bruno-app/src/utils/collections/index.spec.jspackages/bruno-electron/src/utils/collection.jspackages/bruno-electron/src/utils/tests/collection-utils.spec.jspackages/bruno-schema-types/src/collection/environment.tspackages/bruno-schema-types/src/common/annotation.tspackages/bruno-schema-types/src/common/index.tspackages/bruno-schema-types/src/common/key-value.tspackages/bruno-schema-types/src/common/variables.tspackages/bruno-schema/src/collections/annotationsSchema.spec.jspackages/bruno-schema/src/collections/index.js
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
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
Reopening #7711
Contribution Checklist:
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
Release Notes
New Features
Tests