Fix for LTAR Record Grouping in NocoDB #10902
Conversation
This change fixes an issue in NocoDB where records grouped under a "Link to Another Record" (LTAR) relationship weren't automatically associating with their parent record when using either "New Record - Form" or "New Record - Grid" interfaces. The key improvements include:
Added new helper functions:
isGroupBtLTAR: Detects if a column is a "Belongs To" LTAR type
getBtLTAR: Retrieves parent record information for grouped records
Enhanced empty row creation:
When adding a new row in a grouped view, the system now automatically associates it with the parent record
Initializes an ltarState property to track LTAR relationships
Uses promises to fetch and link parent records asynchronously
Fixed column type filtering:
Updated the condition that determines which columns should receive default values
Properly handles LTAR columns in grouped views
This change ensures that when users add records within a grouped view based on a "Belongs To" relationship (whether using form or grid entry), the parent-child association is automatically established without requiring manual selection.
📝 WalkthroughWalkthroughThis pull request introduces a new function, Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
fixes #9061 |
|
@DarkPhoenix2704 Could you take a look at this? (or whomever would typically review this kind of thing 😄 ) |
![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
🧹 Nitpick comments (1)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue (1)
178-179: Consider clarifying the conditional expression.The negations and nested checks reduce readability, making future troubleshooting harder. Recommend adding parentheses or splitting conditions to clearly convey intent.
- !(curr.column_uidt == UITypes.LinkToAnotherRecord && !isGroupBtLTAR(curr.column_name)) && - ![UITypes.Rollup, UITypes.Lookup, UITypes.Formula, UITypes.Barcode, UITypes.QrCode, UITypes.Links].includes(curr.column_uidt) + // Example of improved readability: + !( + curr.column_uidt === UITypes.LinkToAnotherRecord && + !isGroupBtLTAR(curr.column_name) + ) + && + ![ + UITypes.Rollup, + UITypes.Lookup, + UITypes.Formula, + UITypes.Barcode, + UITypes.QrCode, + UITypes.Links + ].includes(curr.column_uidt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue(4 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(0 hunks)
💤 Files with no reviewable changes (1)
- packages/nc-gui/components/smartsheet/grid/Table.vue
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (5)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue (5)
39-40: Looks good for fetching metadata.These lines cleanly introduce the
getMetafunction fromuseMetas. No immediate issues noted.
190-191: Incrementing group count looks fine.This small update to
group.countaligns with the new row addition logic. No concerns here.
112-167:❓ Verification inconclusive
Potential unvalidated input for
whereclause.This function builds a query with
where: `(${columnWithOrderOne.title},eq,${curr.key})`, which may be risky ifcurr.keyis user-controlled. Confirm that the underlying API safely handles special characters to avoid injection.
🏁 Script executed:
#!/bin/bash #!/bin/bash # Searches for references to validate or sanitize 'curr.key' usage in the codebase rg -A 5 "curr\.key"Length of output: 4613
Action Required: Validate Dynamic Query Input for Injection Vulnerability
The LTAR query in
GroupByTable.vueis constructed as:where: `(${columnWithOrderOne.title},eq,${curr.key})`The shell search shows similar direct usages of
curr.keyin other parts of the codebase (e.g., inuseViewGroupBy.ts), and there’s no evidence of input sanitization or escaping. Please verify that the underlying API (api.dbDataTableRow.list) or any shared query builder logic properly handles special characters to prevent injection attacks. If not, consider implementing input validation or escaping forcurr.key.
- Verify: That
api.dbDataTableRow.listsafely processes dynamic input without risk of injection.- Action: Implement sanitization or use parameterized queries if necessary.
193-235:❓ Verification inconclusive
Validate merging multiple LTAR references for the same column.
If there are multiple Belongs To references in
group.nestedInpointing to the same column, this logic will overwriteltarStateentries. Confirm this is intended. If you'd like to accumulate multiple values, consider appending to the array.
🏁 Script executed:
#!/bin/bash # Searching for references to group.nestedIn where the same column could be repeated rg -A 5 "group.nestedIn"Length of output: 3942
Action Required – Validate LTAR Merging Behavior: The current implementation assigns LTAR references using
ltarState[result.ltarColumn.title] = [result.ltarId];which means if there are multiple LTAR references for the same column in
group.nestedIn, each subsequent reference will overwrite the previous entry. Please confirm whether this overwriting is the intended behavior. If you need multiple LTAR references to accumulate for a given column, consider updating the logic to append additional LTAR IDs (for example, by concatenating arrays) rather than replacing them.
- File:
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue(Lines 193–235)
41-42:❓ Verification inconclusive
API injection point introduced.
You're now using
apifromuseApi()to query data. Verify all usage to ensure correct error handling, and that query parameters are appropriately sanitized.Please run a quick search for all occurrences of
api.dbDataTableRowto confirm that no dynamic string concatenation is occurring without proper validation:
🏁 Script executed:
#!/bin/bash ast-grep --pattern $'api.dbDataTableRow.list($_, $_)'Length of output: 54
Action: Verify API Query Parameter Safety
I checked for dynamic string concatenation in calls to
api.dbDataTableRow.list($_, $_)and didn’t find any matching occurrences. However, to guarantee that all API interactions handle errors robustly and that query parameters are properly sanitized across the codebase, please perform a manual review of all usages ofapi.dbDataTableRow. Additional verification should ensure that no dynamic string concatenation (or similar unsafe operations) is being applied without proper validation.
- Confirm that each use of
api.dbDataTableRowimplements proper error handling.- Ensure query parameters are sanitized prior to being passed to the API.
| if (!skipUpdate && rowObj) { | ||
| saveEmptyRow(rowObj) | ||
| } | ||
There was a problem hiding this comment.
It doesn't seem like this is needed. Deleting it actually fixed the problem I was having with Grid-mode not accepting the new row.
There was a problem hiding this comment.
Currently, newly added rows are not saved unless the user modifies a cell value, which is not the expected behaviour.
cc @dstala
Screen.Recording.2025-04-10.at.11.49.34.AM.mov
There was a problem hiding this comment.
Good catch. I just figured out how to toggle-off the experimental groupby. I'll take a look at this over the weekend and see how to restore the expected behavior.
There was a problem hiding this comment.
I think the problem I had was addEmptyRow really needed to be made async, and then await the callAddEmptyRow. Otherwise ltarstate is empty when the row is saved, since we're doing api calls.
This doesn't work:
async function saveEmptyRow(rowObj: Row) {
// the ltarState will be empty here, but logs properly under rowObj
await updateOrSaveRow?.(rowObj, '', rowObj.rowMeta.ltarState || {})
}
function addEmptyRow(row?: number, skipUpdate = false) {
const rowObj = callAddEmptyRow?.(row)
if (!skipUpdate && rowObj) {
saveEmptyRow(rowObj)
}
Should I ditch this version and see about adding it properly to the canvas-groupby instead? Or will the team look it into it?
There was a problem hiding this comment.
As you can see I'm out of my depth with vue/javascript
Add comment about future optimization Renamed columnWithOrderOne -> display column
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue (3)
39-42: Consider user-facing error handling.
getMetaandapiare retrieved to handle LTAR lookups. If these fail, the code logs errors to the console. Consider displaying user-facing alerts, especially in production, to enhance error visibility.
134-177: Consider optimizing multiple lookups.
Each “nestedIn” item triggers an API call limited to 2 results to ensure uniqueness. Repeated queries may affect performance if there are many items. As noted in the TODO, storing parent record IDs or batching requests could be beneficial.
204-263: Ensure LTAR resolution timing is handled.
Resolving LTAR references asynchronously viaPromise.allis fine, but users might save the row before these promises complete. Consider blocking the save or marking the row as “resolving” to prevent inconsistent data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue (2)
Learnt from: ssweber
PR: nocodb/nocodb#10902
File: packages/nc-gui/components/smartsheet/grid/GroupByTable.vue:105-110
Timestamp: 2025-03-19T13:31:43.506Z
Learning: The `isBt` function is exported from `nocodb-sdk` package (specifically from `packages/nocodb-sdk/src/lib/columnHelper/utils/virtualCell.ts`) and checks if a column is a "Belongs To" type LTAR by examining its column options.
Learnt from: ssweber
PR: nocodb/nocodb#10902
File: packages/nc-gui/components/smartsheet/grid/GroupByTable.vue:105-110
Timestamp: 2025-03-19T13:32:06.905Z
Learning: The `isBt` function is available from the `nocodb-sdk` package and checks if a column is a "Belongs To" type LTAR by examining its column options.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (5)
packages/nc-gui/components/smartsheet/grid/GroupByTable.vue (5)
2-2: Good addition ofisBtimport.
ImportingisBtfromnocodb-sdkproperly aligns with its usage in line 124.
105-112: Helpful docstring.
The documentation clarifies the function’s purpose and references a future optimization (issue #10831). This is a great start for maintainability.
113-117: Confirm skipping nested groups.
The function returns early ifgroup.nestedis true or no rows exist. Verify whether nested groups should indeed be skipped or whether partial LTAR resolution is still desired in that scenario.
118-133: Appropriate filtering of non-BT columns.
Skipping columns that are not Belongs To (BT) or contain__nc_null__is consistent with the docstring’s intent. The code gracefully avoids irrelevant columns.
187-201: Potential concurrency concern ongroup.count.
Incrementinggroup.countlocally might lead to stale data if multiple rows are added in parallel. Consider synchronizing the row count with server updates to prevent mismatch.
|
Working.
|
Merge develop into branch
|
@pranavxc , @dstala I think this is in a good spot now, unless either of you have suggestions on a better way than an api-call to get a Groups parent Id of a Bt LTAR. This feature is highly desired by my team. It was the first “surprise” when we setup a parent Projects -> has many -> Tasks. It’s natural to group-by Projects in the Tasks table and then be confused why the new line disappears after clicking “New Record” underneath the grouping. Only explaining this because I see the team is very busy 😀 Thank you. |
|
Any feedback on this pr? |
|
@DarkPhoenix2704 Do you have 5 minutes to give any feedback on this? I've tested and it's working as intended (and has logic to avoid modifying existing behavior) |
|
Thanks @dstala . I'll poke around the changes brought in via nc-canvas-group and see how to update this pr to bring the same functionality to the canvas version. |
There was a problem hiding this comment.
Check the following - https://github.com/nocodb/nocodb/pull/10902/files#r2036589784
|
I think I'll close this one and reopen with one when I have a bit more time that integrates the LTAR information on the initial Groupby loading, so that there is no delay on Grid/Form creation due to an API call. I'll target the new canvas-groupby. |
3 participants
This change fixes an issue in NocoDB where records grouped under a "Link to Another Record" (LTAR) relationship weren't automatically associating with their parent record when using either "New Record - Form" or "New Record - Grid" interfaces. The key improvements include:
This change ensures that when users add records within a grouped view based on a "Belongs To" relationship (whether using form or grid entry), the parent-child association is automatically established without requiring manual selection.
Change Summary
Provide summary of changes with issue number if any.
Change type
Additional information / screenshots (optional)
I correctly handle when Record isn't Belongs-to.
1) We need a good way of finding quickly when a parent column isn't unique2) Is there a simpler way of identifying parent column by Group.column_text, and then id of parent record?
3) Need help when using 'New Record - Form'. It correctly adds, but how do you 'sync' them so that if you click+it shows it's linked? And sometimes there is a delay before it shows up (like with nested groups).