Fixed collection variable sync on cloud for empty row#4475
Fixed collection variable sync on cloud for empty row#4475rohitneharabrowserstack merged 11 commits intomasterfrom
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:
WalkthroughSanitization for COLLECTION records was changed: when building the variables map each entry's map key and the variable's Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 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 unit tests (beta)
📝 Coding Plan
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 the current code and only fix it if needed.
Inline comments:
In `@app/src/backend/apiClient/upsertApiRecord.ts`:
- Around line 27-40: The map over sanitizedRecord.data.variables can destructure
a null/undefined variable causing a TypeError; in the map callback that computes
entryKeyTrimmed/valueKeyTrimmed/finalKey and then does const { localValue, key:
_ignored, ...rest } = variable, first guard that variable is truthy (e.g., if
(!variable) return null) or replace with a safe default before destructuring
(e.g., const v = variable ?? {}; then destructure v) so you never destructure
null/undefined; ensure the callback still returns null for skipped entries so
the subsequent .filter((pair): pair is [string, any] => Array.isArray(pair))
continues to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9bde200-870f-4960-a39f-bed3a619f22b
📒 Files selected for processing (1)
app/src/backend/apiClient/upsertApiRecord.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/backend/apiClient/upsertApiRecord.ts (2)
32-34: Minor: Redundant optional chaining.On line 33,
variable?.keyuses optional chaining, butvariableis already guaranteed to be truthy due to the guard at lines 29-31. This is harmless but slightly redundant.- const valueKeyTrimmed = (variable?.key || "").toString().trim(); + const valueKeyTrimmed = (variable.key || "").toString().trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/backend/apiClient/upsertApiRecord.ts` around lines 32 - 34, The optional chaining in the expression used to build valueKeyTrimmed is redundant because variable is already guaranteed truthy by the earlier guard; replace variable?.key with variable.key when computing valueKeyTrimmed (the lines computing entryKeyTrimmed, valueKeyTrimmed, and finalKey) so the code reads valueKeyTrimmed = (variable.key || "").toString().trim() and keep finalKey = entryKeyTrimmed || valueKeyTrimmed unchanged.
46-50: Potential order loss when empty entry keys are normalized tovariable.key.When an entry has an empty
entryKeybut a non-emptyvariable.key, the sanitized variables map usesvalueKeyTrimmedas thefinalKey. However,variablesOrderstill contains the original empty key, which gets filtered out here (empty string is falsy). This causes the renamed variable to lose its position in the order.Example scenario:
- Original:
entryKey = "",variable.key = "myVar",variablesOrder = ["", "other"]- After sanitization:
variables = { "myVar": {...} },variablesOrder = ["other"]- Result:
"myVar"is orphaned from the orderThe downstream
mapToEnvironmentArray(inenvironment/utils.ts) appends unordered keys at the end, so data isn't lost, but the variable's original position is not preserved. If this is acceptable for the empty-row edge case, consider adding a brief comment explaining the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/backend/apiClient/upsertApiRecord.ts` around lines 46 - 50, The current filter removes falsy/empty entries from sanitizedRecord.data.variablesOrder which drops variables that were renamed from an empty entryKey (e.g., entryKey = "" -> variable.key = "myVar"), losing their original position; update the variablesOrder normalization so that for each entry you trim k, and if the trimmed key is empty but there exists a variable in sanitizedRecord.data.variables whose original entryKey was falsy (or whose valueKeyTrimmed/variable.key was used as the final key) then replace the empty slot with that variable's final key instead of filtering it out; implement this by mapping sanitizedRecord.data.variablesOrder to trimmed keys, and when encountering a falsy trimmed key, find the next unused variable in sanitizedRecord.data.variables with an original/falsy entryKey (or matching valueKeyTrimmed) and insert its final key, leaving other keys unchanged so original ordering is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/backend/apiClient/upsertApiRecord.ts`:
- Around line 32-34: The optional chaining in the expression used to build
valueKeyTrimmed is redundant because variable is already guaranteed truthy by
the earlier guard; replace variable?.key with variable.key when computing
valueKeyTrimmed (the lines computing entryKeyTrimmed, valueKeyTrimmed, and
finalKey) so the code reads valueKeyTrimmed = (variable.key ||
"").toString().trim() and keep finalKey = entryKeyTrimmed || valueKeyTrimmed
unchanged.
- Around line 46-50: The current filter removes falsy/empty entries from
sanitizedRecord.data.variablesOrder which drops variables that were renamed from
an empty entryKey (e.g., entryKey = "" -> variable.key = "myVar"), losing their
original position; update the variablesOrder normalization so that for each
entry you trim k, and if the trimmed key is empty but there exists a variable in
sanitizedRecord.data.variables whose original entryKey was falsy (or whose
valueKeyTrimmed/variable.key was used as the final key) then replace the empty
slot with that variable's final key instead of filtering it out; implement this
by mapping sanitizedRecord.data.variablesOrder to trimmed keys, and when
encountering a falsy trimmed key, find the next unused variable in
sanitizedRecord.data.variables with an original/falsy entryKey (or matching
valueKeyTrimmed) and insert its final key, leaving other keys unchanged so
original ordering is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d96419fd-fb39-4384-b4de-90dc72be115f
📒 Files selected for processing (1)
app/src/backend/apiClient/upsertApiRecord.ts
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 `@app/src/backend/apiClient/upsertApiRecord.ts`:
- Around line 46-49: The trimmed variablesOrder entries can collapse into
duplicates (e.g. " foo " -> "foo") and cause duplicate rendering; update the
sanitization in upsertApiRecord by deduplicating the normalized keys after
trimming/filtering: transform sanitizedRecord.data.variablesOrder with .map(k =>
(k||'').trim()).filter(k => k && sanitizedRecord.data?.variables[k]) then remove
duplicates while preserving order (e.g., using a Set or seen map) so
mapToEnvironmentArray() receives a unique, trimmed variablesOrder array; ensure
the final value remains an array on sanitizedRecord.data.variablesOrder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6864068-044d-4e99-a4d0-4bde267fc303
📒 Files selected for processing (1)
app/src/backend/apiClient/upsertApiRecord.ts
| return null; // will be filtered out | ||
| } | ||
|
|
||
| const { localValue, key: _ignored, ...rest } = variable; |
There was a problem hiding this comment.
Do we still get this key in the object?
There was a problem hiding this comment.
Yes we do get it
Co-authored-by: Nafees Nehar <[email protected]>
2 participants
Closes issue:
📜 Summary of changes:
🎥 Demo Video:
Video/Demo:
✅ Checklist:
🧪 Test instructions:
🔗 Other references:
Summary by CodeRabbit