fix(datatype): Variable type validation flag reference mismatch issue - #8878
fix(datatype): Variable type validation flag reference mismatch issue#8878sharan-bruno wants to merge 21 commits into
Conversation
|
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:
Walkthrough
ChangesDataTypeSelector validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Playwright
participant EnvironmentPanel
participant DataTypeSelector
participant CollectionVariables
Playwright->>EnvironmentPanel: open global or collection environment
EnvironmentPanel->>DataTypeSelector: render variable data type
DataTypeSelector->>CollectionVariables: resolve {{reference}}
CollectionVariables-->>DataTypeSelector: return inferred type
DataTypeSelector-->>Playwright: show or hide mismatch icon
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-app/src/components/DataTypeSelector/index.js`:
- Around line 20-26: Update the variable matching logic around the
value-resolution helper to require the entire trimmed value to consist of a
single {{...}} reference, rather than matching embedded references. Return null
for mixed templates such as prefix {{enabled}}, so getDataTypeFromValue is only
used for complete variable references.
🪄 Autofix
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 Plus
Run ID: 1dce5156-1d5f-49c5-88e5-eac0d05a59e3
📒 Files selected for processing (1)
packages/bruno-app/src/components/DataTypeSelector/index.js
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/variable-datatypes/reference-types/fixtures/workspace/collections/reference-types/opencollection.yml`:
- Around line 1-3: Add typed collection variables to the reference-types
OpenCollection fixture, then extend reference-variable-type.spec.ts with
assertions covering both matching and mismatched references to those collection
variables. Preserve the existing global-variable cases while ensuring
collection-variable resolution and type validation are exercised.
In
`@tests/variable-datatypes/reference-types/fixtures/workspace/environments/global-environment.yml`:
- Around line 23-69: Add matching and mismatched fixture entries alongside the
existing reference cases for a source variable named globalEnv.boolean, ensuring
references resolve the dotted variable name itself rather than treating it as a
nested object path. Include appropriate typed cases to verify both valid and
invalid type resolution, while preserving the existing globalEnvNestedObject
path cases.
In `@tests/variable-datatypes/reference-types/fixtures/workspace/workspace.yml`:
- Around line 6-8: Update the workspace fixture declaration for the single
collection named reference-types: move its fixture directory from
collections/reference-types to the singular collection/reference-types location
and change the workspace.yml path accordingly.
In `@tests/variable-datatypes/reference-types/reference-variable-type.spec.ts`:
- Around line 1-78: Update the test file’s TypeScript statements to terminate
with semicolons, including the imports, helper function declarations and
returns, locator/assertion calls, and test-step statements throughout the
describe block. Preserve the existing test behavior and structure while applying
the repository’s semicolon style consistently.
🪄 Autofix
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 Plus
Run ID: 3b5e9d1c-9ecd-47ed-9baf-2426014b7981
📒 Files selected for processing (7)
tests/utils/page/actions.tstests/variable-datatypes/reference-types/fixtures/workspace/collections/reference-types/environments/collection-environment.ymltests/variable-datatypes/reference-types/fixtures/workspace/collections/reference-types/opencollection.ymltests/variable-datatypes/reference-types/fixtures/workspace/environments/global-environment.ymltests/variable-datatypes/reference-types/fixtures/workspace/workspace.ymltests/variable-datatypes/reference-types/init-user-data/preferences.jsontests/variable-datatypes/reference-types/reference-variable-type.spec.ts
| opencollection: "1.0.0" | ||
| info: | ||
| name: reference-types |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add collection-variable reference fixtures.
This collection defines no variables. The collection-environment cases reference only global variables. Add typed collection variables here and add matching and mismatched reference assertions in reference-variable-type.spec.ts.
Without these cases, the test does not cover collection-variable resolution added by this PR.
🤖 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/variable-datatypes/reference-types/fixtures/workspace/collections/reference-types/opencollection.yml`
around lines 1 - 3, Add typed collection variables to the reference-types
OpenCollection fixture, then extend reference-variable-type.spec.ts with
assertions covering both matching and mismatched references to those collection
variables. Preserve the existing global-variable cases while ensuring
collection-variable resolution and type validation are exercised.
Source: Coding guidelines
| collections: | ||
| - name: "reference-types" | ||
| path: "collections/reference-types" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the singular fixture directory for one collection.
This workspace declares one collection. Move the fixture to fixtures/workspace/collection/reference-types and change path to collection/reference-types.
Based on learnings: use fixtures/collection when there is one collection.
🤖 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/variable-datatypes/reference-types/fixtures/workspace/workspace.yml`
around lines 6 - 8, Update the workspace fixture declaration for the single
collection named reference-types: move its fixture directory from
collections/reference-types to the singular collection/reference-types location
and change the workspace.yml path accordingly.
Source: Learnings
| - name: "reference-types" | ||
| path: "collections/reference-types" | ||
|
|
||
| specs: |
There was a problem hiding this comment.
remove spec: , docs: ?
| import { Tooltip } from 'react-tooltip'; | ||
| import { BRUNO_VARIABLE_DATATYPES, parseValueByDataType, validateDataTypeValue } from '@usebruno/common/utils'; | ||
| import { BRUNO_VARIABLE_DATATYPES, parseValueByDataType, validateDataTypeValue, getDataTypeFromValue } from '@usebruno/common/utils'; | ||
| import get from 'lodash/get'; |
There was a problem hiding this comment.
nit: external lib imports always at the top. then @usebruno scoped libraries, then utils from package, then component imports
|
|
||
| const DataTypeSelector = ({ variable, onChange, compact = false }) => { | ||
| const DataTypeSelector = ({ variable, collection, onChange, compact = false }) => { | ||
| const resolvableVariables = useMemo(() => getAllVariables(collection), [collection]); |
There was a problem hiding this comment.
getAllVariables re-runs on every mounted row whenever collection, this will cause performance issues, can we hoist this to the parent EnvironmentVariablesTable, add this const resolvableVariables = useMemo(() => getAllVariables(_collection), [_collection]); as a sibling memo to the _collection, and pass it down, so that each row doesn't have to call getAllVariables which is very computationally complex! Also DataTypeSelector shouldn't know about collection at all, Change the prop to resolvableVariables and drop collection
This should be done at all the places we need to have this. so more files needs changes. Be mindful about where all we need to add this.
| }; | ||
|
|
||
| const DataTypeSelector = ({ variable, onChange, compact = false }) => { | ||
| const DataTypeSelector = ({ variable, collection, onChange, compact = false }) => { |
There was a problem hiding this comment.
Also i don't see any files that changed to pass down collection to this component.
|
|
||
| const referencedType = getReferencedVariableType(variable.value, resolvableVariables); | ||
| const selectedType = variable.dataType || 'string'; | ||
| const referencedTypeError = referencedType === selectedType ? null : `Value is not a valid ${selectedType}`; |
There was a problem hiding this comment.
Better to give the responsibility to validateDataTypeValue that having referencedTypeError here , keep the concerns together may be something like this
export const validateTypedVariableValue = (value, dataType, resolvableVariables) => {
const referencedType = getReferencedVariableType(value, resolvableVariables);
if (referencedType !== null) {
return referencedType === dataType ? null : `Value is not a valid ${dataType}`;
}
return validateDataTypeValue(parseValueByDataType(value, dataType), dataType);
};| const handleTypeChange = (type) => { | ||
| onChange({ dataType: type === 'string' ? undefined : type }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Taking a tangent from existing implementation , considering unit testability and responsibility, why can't we think of something like this
In packages/bruno-common/src/utils/datatype.ts
// Resolves `path` on `obj`, matching lodash.get semantics we rely on:
// a literal key that contains dots wins over walking the same-looking dotted path.
// (Env vars can be named e.g. "globalEnvObject.port" as a flat key.)
const getByPath = (obj: Record<string, any>, path: string): any => {
if (obj == null) return undefined;
if (Object.prototype.hasOwnProperty.call(obj, path)) return obj[path];
return path.split('.').reduce<any>((acc, key) => (acc == null ? undefined : acc[key]), obj);
};
// Whole-string template reference only: '{{a.b}}' resolves, 'x-{{a}}-y' does not.
const resolveWholeReference = (value: any, resolvableVariables: Record<string, any>): any => {
if (typeof value !== 'string') return undefined;
const match = value.trim().match(/^\{\{([^}]+)\}\}$/);
if (!match) return undefined;
return getByPath(resolvableVariables, match[1].trim());
};
// string-form → typed JS value, or raw on failure. When `resolvableVariables`
// is provided and `value` is a whole-string template reference, the resolved
// JS value is returned as-is (its declared type is authoritative — we don't
// re-coerce it).
export const parseValueByDataType = (
value: any,
dataType?: BrunoVariableDataType,
resolvableVariables?: Record<string, any>
): any => {
if (resolvableVariables) {
const resolved = resolveWholeReference(value, resolvableVariables);
if (resolved !== undefined) return resolved;
}
if (!dataType || dataType === 'string') return value;
// ... existing number / boolean / object branches unchanged ...
};in packages/bruno-app/src/components/DataTypeSelector/index.js
const DataTypeSelector = ({ variable, resolvableVariables, onChange, compact = false }) => {
const selectedType = variable.dataType || 'string';
const coercedValue = parseValueByDataType(variable.value, selectedType, resolvableVariables);
const typeError = validateDataTypeValue(coercedValue, selectedType);
// ... rest unchanged
};and unit test cases in packages/bruno-common/src/utils/datatype.spec.ts
| // Returns an error message when post-coerce value's JS type doesn't match dataType. | ||
| export const validateDataTypeValue = (value: any, dataType?: BrunoVariableDataType): string | null => { | ||
| if (!dataType || dataType === 'string') return null; | ||
| if (!dataType) return null; |
There was a problem hiding this comment.
We were returning null previously when dataType === 'string' , please check the consumers of this function, and ensure that we are not introducing any regressions
| dataType?: BrunoVariableDataType, | ||
| resolvableVariables?: Record<string, any> | ||
| ): any => { | ||
| if (resolvableVariables) { |
There was a problem hiding this comment.
we will need to pass down the resolved value to check if they are parsable as a number, boolean, object, than return the value as we were doing before, we should try to parse/coerce the value to a a certain type then return it. Thoughts on this!
| describe('parseValueByDataType — {{var}} references', () => { | ||
| const variables = { count: 7, flag: true, payload: { a: 1 }, label: 'hi', nested: { count: 3 } }; | ||
|
|
||
| it('resolves a lone reference to the referenced variable value', () => { |
There was a problem hiding this comment.
coercion test cases should also be part of the test suite, especially variableReferences, where
obj : {
"count": 42
}
{{obj.count}} should get parsed as string as well as numberPlease also add the basic coercion test cases if they are missing
| it('returns empty string for functions and symbols', () => { | ||
| expect(valueToString(() => 42)).toBe(''); | ||
| expect(valueToString(function named() {})).toBe(''); | ||
| expect(valueToString(function named() { })).toBe(''); |
There was a problem hiding this comment.
@sharan-bruno this is intentional right, Linting?
There was a problem hiding this comment.
formatting issue, i will remove it
| "defaultWorkspacePath": "{{workspacePath}}" | ||
| } | ||
| } | ||
| } No newline at end of file |
|
|
||
| // string-form → typed JS value, or raw on failure. | ||
| export const parseValueByDataType = (value: any, dataType?: BrunoVariableDataType): any => { | ||
| export const parseValueByDataType = ( |
There was a problem hiding this comment.
may need to update the title as there is an additional change including resolvableVariables
| }); | ||
|
|
||
| await test.step('Valid literal values are not flagged', async () => { | ||
| await expectNotFlagged(page, 'globalEnvString'); |
There was a problem hiding this comment.
@sharan-bruno In the current state of the test cases, it is very difficult to track which paths are tested, which are not, i think we may need to tweak the test cases do that it is easier to review, maintain. One should be able to understand, what is being tested, what is missed quickly and should be able to decisively add things so that the testing is robust.
Now there is a lot of mental overhead to read each of the variable names, and keeping all those context in mind and work with these files will be difficult to pull through.
Similar approach can also be adopted for unit test cases
Please check #8943 , Please use discretion, while making these changes
There was a problem hiding this comment.
@sanish-bruno Agreed. I’ve refactored the test cases using a matrix-based approach, making the coverage and expected behavior easier to understand and maintain
| const mismatchIcon = async (page: Page, name: string) => { | ||
| const locators = buildCommonLocators(page); | ||
| const row = locators.environment.varRow(name); | ||
| await scrollVirtuosoRowIntoView(page, row); |
There was a problem hiding this comment.
This flaked on CI. The row was missing because the table had not rendered yet. Can we wait for it first?
There was a problem hiding this comment.
added wait before checking this
…across components and addressed review comments
…ironment configurations
d787f77 to
19aaec4
Compare
| const onSave = () => dispatch(saveRequest(item.uid, collection.uid)); | ||
| const handleRun = () => dispatch(sendRequest(item, collection.uid)); | ||
|
|
||
| const resolvableVariables = useMemo(() => getAllVariables(collection), [collection]); |
There was a problem hiding this comment.
Should this pass item too?
getAllVariables(collection, item)
|
|
||
| const onSave = () => dispatch(saveFolderRoot(collection.uid, folder.uid)); | ||
|
|
||
| const resolvableVariables = useMemo(() => getAllVariables(collection), [collection]); |
There was a problem hiding this comment.
3 participants
BRU-3849
Description
Fixes the false data-type warning shown on variable values that reference other variables. DataTypeSelector now resolves the reference before validating.
Problem
variable.value was type-checked literally, so {{port}} typed as number was seen as the string "{{port}}". Result: a bogus "Value is not a valid number" flag even when port is a number.
Fix
Added getReferencedVariableType — extracts the {{...}} name, resolves it via getAllVariables, and returns that variable's real type (supports dotted paths and dotted names). References compare against the referenced type; non-references still go through validateDataTypeValue. Adds a collection prop, already passed by all four call sites.
Screenshots
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
New Features
Tests