[fix]: testset sync related issues#3958
[fix]: testset sync related issues#3958ashrafchowdury wants to merge 8 commits intorelease/v0.93.1from
testset sync related issues#3958Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
bot
added
size:L
Railway Preview Environment
|
…estset-sync-confirmation [feat]: Added warning UIs for `testset` disconnect & change
dosubot
bot
added
size:XL
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ? revisionVersion | ||
| ? `${testsetName} (v${revisionVersion})` |
There was a problem hiding this comment.
🟡 Truthy check on revisionVersion drops version 0 from display name
The old code used revisionVersion != null to gate the version suffix in the display name, which correctly allowed version 0 through. The new code uses a bare truthy check (revisionVersion ? ...), which treats 0 as falsy. If a v0 (draft) revision is ever connected, the display name will omit the version suffix entirely, producing just testsetName instead of testsetName (v0). While the PR also filters v0 from the selection UI (testsetRelationAdapter.ts:95), other code paths (e.g. snapshot hydration, direct connectToTestset calls) could still pass revisionVersion: 0.
| ? revisionVersion | |
| ? `${testsetName} (v${revisionVersion})` | |
| ? revisionVersion != null | |
| ? `${testsetName} (v${revisionVersion})` |
Was this helpful? React with 👍 or 👎 to provide feedback.
| hasSuccessfulResults, | ||
| handleSyncOpen, | ||
| handleDisconnect, | ||
| handleChangeTestset, | ||
| handleAddToTestset, | ||
| ]) |
There was a problem hiding this comment.
🟡 handleManageTestcasesClick missing from menuItems useMemo dependency array
The menuItems useMemo at TestsetDropdown/index.tsx:479 references handleManageTestcasesClick in the onClick handler for the "manage" menu item (line 512), but handleManageTestcasesClick is not listed in the dependency array (lines 536-544). This means the memoized menu will capture a stale closure of handleManageTestcasesClick if its dependencies (loadableId, connectedSource?.id, store, initSelectionDraft) change. This was a pre-existing issue but is still present after the PR's changes to the dependency array.
(Refers to lines 536-544)
Was this helpful? React with 👍 or 👎 to provide feedback.
web/oss/src/components/Playground/Components/Modals/TestsetDisconnectConfirmModal/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
🟡 mutations.ts SYSTEM_FIELDS replacement expands filtering of entity.data keys from 6 to 20+ fields
In web/packages/agenta-entities/src/testset/state/mutations.ts:76-79, the currentColumnsAtom iterates over entity.data keys and filters using SYSTEM_FIELDS. The old local constant only had 6 entries (id, __id, __isSkeleton, key, created_at, updated_at). The replacement SYSTEM_FIELDS from @agenta/entities/testcase has 20+ entries including flags, tags, meta, set_id, testset_id, etc.
While entity.data (nested testcase format) normally only contains user columns, if a user's column data happens to be named tags, meta, or flags (all plausible column names), those columns would now be silently excluded from currentColumnsAtom derivation. This was not the case before.
(Refers to lines 76-79)
Prompt for agents
In web/packages/agenta-entities/src/testset/state/mutations.ts, the currentColumnsAtom (around line 59-99) reads keys from entity.data (the nested data sub-object which is user column space). The old local SYSTEM_FIELDS only had 6 entries: 'id', '__id', '__isSkeleton', 'key', 'created_at', 'updated_at'. The new imported SYSTEM_FIELDS has 20+ entries. Consider keeping a smaller set for this specific use case (filtering within entity.data), since fields like 'tags', 'meta', 'flags' could be legitimate user-defined column names. The broader SYSTEM_FIELDS set is correct for top-level entity filtering but too aggressive for filtering within the data sub-object.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const isSystemColumnPath = (columnKey: string): boolean => { | ||
| if (!columnKey) return false | ||
|
|
||
| return columnKey.split(".").some((segment) => SYSTEM_COLUMN_SET.has(segment)) |
There was a problem hiding this comment.
🔴 isSystemColumnPath over-filters nested user data columns by checking every path segment
The new isSystemColumnPath function splits a column key on . and returns true if ANY segment is in SYSTEM_COLUMN_SET. This means legitimate nested user data columns like event.id, response.meta, user.tags, item.key, or data.created_at would be incorrectly filtered from the table display. Fields like id, key, meta, tags, flags, created_at, updated_at are extremely common in nested API response objects that users store as testcase data.
This function is called in web/oss/src/components/TestcasesTableNew/hooks/useTestcasesTable.ts:193-198 to filter both baseColumns and columns (which include expanded/nested columns from expandedColumnsAtom). The top-level system field filtering is already handled by currentColumnKeysAtom in columnState.ts, so this function only needs to handle nested paths — but it incorrectly applies system field matching to intermediate and leaf segments of user data.
Prompt for agents
In web/oss/src/components/TestcasesTableNew/hooks/constants.ts, the isSystemColumnPath function (lines 32-35) needs to be changed so it only checks the FIRST segment (top-level key) against the system column set, not every segment. Nested sub-keys like 'id', 'meta', 'tags', 'key' etc. are common in user data objects and should not be filtered. For example:
export const isSystemColumnPath = (columnKey: string): boolean => {
if (!columnKey) return false
const segments = columnKey.split(".")
return SYSTEM_COLUMN_SET.has(segments[0])
}
Alternatively, only check the leaf segment for internal-only fields (like __dedup_id__, __isSkeleton) while checking the root segment for all system fields.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Never expose internal/system fields as nested columns. | ||
| if (SYSTEM_FIELDS.has(subKey) || subKey.startsWith("_")) return |
There was a problem hiding this comment.
🔴 collectObjectSubKeysRecursive hides legitimate nested fields named id, key, meta, etc.
The new guard if (SYSTEM_FIELDS.has(subKey) || subKey.startsWith("_")) return in collectObjectSubKeysRecursive filters out nested object properties that share names with system fields. SYSTEM_FIELDS includes id, key, meta, tags, flags, created_at, updated_at, etc. — all common field names in user data objects.
For example, a testcase with data {response: {id: "resp-123", text: "Hello"}} would only expand to column response.text; response.id would be silently hidden. The subKey.startsWith("_") check is also too broad, filtering legitimate fields like _id (MongoDB), _type, _source (Elasticsearch).
Previously this function had no such filter, so all nested properties were expanded into sub-columns. This is a regression.
| // Never expose internal/system fields as nested columns. | |
| if (SYSTEM_FIELDS.has(subKey) || subKey.startsWith("_")) return | |
| // Never expose double-underscore internal fields as nested columns. | |
| if (subKey.startsWith("__")) return | |
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
What's fixed??
testcase_dedup_idand parentdatacolumn from the testset modalQA
testcase_dedup_idand the parentdatacolumn on the testset table