fix: fix searching the linked records of a new record in expanded/form #10250
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue (1)
324-332: Consider optimizing the search filter implementation.The current implementation recalculates lowercase values on every filter operation. Consider these optimizations:
- Memoize the lowercase query
- Handle special characters in search
const filteredStateLinkedItems = computed(() => { const list = state.value?.[colTitle.value] ?? [] - const query = childrenListPagination.query.toLocaleLowerCase() + const query = useMemo(() => childrenListPagination.query.toLocaleLowerCase().trim(), [childrenListPagination.query]) if (!query || !list.length) return list return list.filter((record: Record<string, any>) => - `${record[relatedTableDisplayValueProp.value] ?? ''}`.toLocaleLowerCase().includes(query), + `${record[relatedTableDisplayValueProp.value] ?? ''}` + .toLocaleLowerCase() + .normalize('NFD') + .replace(/[\u0300-\u036f]/g, '') + .includes(query), ) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (2)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue (2)
394-394: LGTM! The template changes correctly implement the search functionality.The modification properly integrates the new filtered list while maintaining compatibility with existing functionality.
324-332: Verify search functionality with special characters.Please ensure the search functionality works correctly with:
- Special characters (é, ñ, etc.)
- Mixed case strings
- Numbers and symbols
Also applies to: 394-394
|
Uffizzi Preview |
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nc-gui/composables/useLTARStore.ts (1)
323-328: Consider debouncing the search filterThe local search implementation might cause performance issues with large datasets as it filters on every query change.
Consider implementing debounce:
import { debounce } from 'lodash-es' const debouncedFilter = debounce((query: string, rawList: Record<string, any>[]) => { return query ? rawList.filter((record) => `${record[relatedTableDisplayValueProp.value] ?? ''}`.toLowerCase().includes(query.toLowerCase()), ) : rawList }, 300)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue(1 hunks)packages/nc-gui/composables/useLTARStore.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nc-gui/components/virtual-cell/components/LinkedItems.vue
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: release-docker / buildx
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (5)
packages/nc-gui/composables/useLTARStore.ts (5)
12-12: LGTM! Type safety improvementThe change from
Record<string, any>toRecord<string, any>[]better reflects the actual data structure and improves type safety.
226-229: LGTM! Efficient duplicate preventionThe implementation efficiently prevents duplicate records by using a Set for O(1) lookups. This ensures that records already in the children list are not shown in the excluded list.
572-572: LGTM! Fixes the core issueThe change correctly passes the form state to
loadChildrenList, fixing the issue with searching linked records in expanded/form mode as mentioned in the PR objectives.
332-334: Review pagination logic for edge casesThe pagination implementation assumes fixed values:
isFirstPageis always truepageis hardcoded to 1
This might cause issues with larger datasets or when navigating through pages.Consider implementing proper pagination calculation:
- isFirstPage: true, - page: 1, + isFirstPage: offset === 0, + page: Math.floor(offset / pageSize) + 1,
320-338: Verify error handling for form stateWhile the implementation looks good, we should ensure proper error handling when accessing form state properties.
Consider adding null checks:
- const rawList = newRowState.state?.[colTitle] ?? [] + if (!newRowState.state) { + console.warn(`Form state is missing for column: ${colTitle}`) + } + const rawList = newRowState.state?.[colTitle] ?? []
rameshmane7218
force-pushed
the
nc-fix/searching-linked-records
branch
from
d730d73 to
f0ea159
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/nc-gui/composables/useLTARStore.ts (3)
226-229: Consider using case-insensitive ID field comparison.The filtering logic is good, but the hard-coded 'Id' field might be case-sensitive. Consider using a constant or checking both 'Id' and 'id' to make it more robust.
-const ids = new Set(childrenList.value?.list?.map((item) => item.Id) ?? []) +const ids = new Set(childrenList.value?.list?.map((item) => item.Id || item.id) ?? []) -childrenExcludedList.value.list = childrenExcludedList.value.list.filter((item) => !ids.has(item.Id)) +childrenExcludedList.value.list = childrenExcludedList.value.list.filter((item) => !ids.has(item.Id || item.id))
329-338: Consider extracting pagination constants.The hardcoded values (10) for page size could be moved to constants for better maintainability.
+const DEFAULT_PAGE_SIZE = 10 + childrenList.value = { list, pageInfo: { isFirstPage: true, - isLastPage: list.length <= 10, + isLastPage: list.length <= DEFAULT_PAGE_SIZE, page: 1, - pageSize: 10, + pageSize: DEFAULT_PAGE_SIZE, totalRows: list.length, }, }
572-572: Document the resetOffset parameter.Consider adding a comment explaining why
resetOffsetis set tofalsein the watch handler to improve code maintainability.+ // Don't reset offset when pagination changes to maintain scroll position await loadChildrenList(false, newRowState.state)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue(2 hunks)packages/nc-gui/composables/useLTARStore.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nc-gui/components/virtual-cell/components/LinkedItems.vue
🔇 Additional comments (3)
packages/nc-gui/composables/useLTARStore.ts (3)
12-12: LGTM! Type definition accurately reflects API response.The change from
Record<string, any>toRecord<string, any>[]better represents the list of records returned by the API.
320-328: LGTM! Robust search implementation for new records.The search implementation is well done with case-insensitive matching and proper null handling.
305-306: Verify all loadChildrenList calls are updated.Let's verify that all calls to
loadChildrenListare updated to handle the newactiveStateparameter correctly.✅ Verification successful
All loadChildrenList calls are correctly implemented
The function calls are compatible with the new parameter structure, as both active calls in the codebase properly handle the optional parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to loadChildrenList to verify parameter usage rg "loadChildrenList\(" --type tsLength of output: 297
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nc-gui/composables/useLTARStore.ts (1)
322-340: Consider implementing virtual pagination for better performance.While the local filtering works correctly, loading all records in memory might cause performance issues with large datasets. Consider implementing virtual pagination that only loads the required subset of records.
const colTitle = column.value?.title || '' const rawList = newRowState.state?.[colTitle] ?? [] const query = childrenListPagination.query.toLocaleLowerCase() +const start = (childrenListPagination.page - 1) * childrenListPagination.size +const end = start + childrenListPagination.size const list = query ? rawList.filter((record: Record<string, any>) => `${record[relatedTableDisplayValueProp.value] ?? ''}`.toLocaleLowerCase().includes(query), - ) + ).slice(start, end) : rawList
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/composables/useLTARStore.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: release-docker / buildx
- GitHub Check: unit-tests-pg
- GitHub Check: unit-tests
🔇 Additional comments (2)
packages/nc-gui/composables/useLTARStore.ts (2)
12-12: LGTM! Type definition accurately reflects API response structure.The change from
Record<string, any>toRecord<string, any>[]better represents the actual API response structure where the list is always an array of records.
574-574: LGTM! Watch handler correctly maintains state consistency.The watch handler properly passes the state to
loadChildrenListensuring state consistency during pagination changes.
| const ids = new Set(childrenList.value?.list?.map((item) => item.Id) ?? []) | ||
| if (childrenExcludedList.value.list && ids.size) { | ||
| childrenExcludedList.value.list = childrenExcludedList.value.list.filter((item) => !ids.has(item.Id)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use primary key from metadata instead of hardcoded 'Id'.
The current implementation assumes 'Id' as the primary key field, which might not be true for all tables. Consider using the primary key field from the table metadata.
-const ids = new Set(childrenList.value?.list?.map((item) => item.Id) ?? [])
-if (childrenExcludedList.value.list && ids.size) {
- childrenExcludedList.value.list = childrenExcludedList.value.list.filter((item) => !ids.has(item.Id))
+const primaryKeyField = relatedTableMeta.value?.columns?.find((c) => c.pk)?.title
+if (!primaryKeyField) return
+const ids = new Set(childrenList.value?.list?.map((item) => item[primaryKeyField]) ?? [])
+if (childrenExcludedList.value.list && ids.size) {
+ childrenExcludedList.value.list = childrenExcludedList.value.list.filter((item) => !ids.has(item[primaryKeyField]))
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue (1)
159-169: Consider adding error handling for record creation.While the implementation is solid, consider adding try-catch blocks in the
onCreatedRecordfunction to handle potential failures in record creation and linking.const onCreatedRecord = async (record: any) => { if (!isNewRecord.value) return + try { if (!isNew.value) { vModel.value = false } else { await addLTARRef(record, injectedColumn?.value as ColumnType) loadChildrenList(false, state.value) } + } catch (error) { + message.error(t('error.recordCreationFailed')) + console.error('Failed to create/link record:', error) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (3)
packages/nc-gui/components/virtual-cell/components/LinkedItems.vue (3)
159-169: LGTM! Improved record creation handling.The async implementation with proper state management for both new and existing records is well-structured. The changes align with the PR objective of fixing linked record searching.
208-211: LGTM! Proper state handling in watch.The watch handler correctly passes the state parameter to
loadChildrenListfor new records, maintaining consistency with the record creation flow.
Line range hint
341-356: Verify keyboard event handling in search input.The search input implementation looks good, but please verify that:
- The
@keydown.capture.stopmodifier doesn't interfere with essential browser shortcuts- The keyboard event handling works correctly with the global
linkedShortcutshandler✅ Verification successful
Keyboard event handling implementation is correct and safe
The
@keydown.capture.stopmodifier is properly used here as it:
- Prevents conflicts with parent handlers while searching
- Allows proper handling of Escape (blur) and Enter (quick link) keys
- Works harmoniously with the list items' keyboard shortcuts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other keyboard event handlers that might conflict rg -A 5 "@keydown|addEventListener.*keydown" --type vueLength of output: 85
Script:
#!/bin/bash # First find all Vue files and then search for keyboard event handlers fd -e vue -x rg -l -A 5 "(@keydown|addEventListener.*keydown|@keyup|@keypress)" # Also search for any global shortcut handlers or keyboard utilities fd -e vue -e js -e ts -x rg -l "linkedShortcuts|keyboardShortcuts|keyboard"Length of output: 10196
Script:
#!/bin/bash # Check the implementation of keyboard handling in LinkedItems.vue rg -B 5 -A 10 "handleKeyDown|@keydown" ./packages/nc-gui/components/virtual-cell/components/LinkedItems.vue # Also check for any keyboard-related utilities or mixins that might be imported rg -B 2 "import.*keyboard" ./packages/nc-gui/components/virtual-cell/components/LinkedItems.vueLength of output: 2626
Fix searching the linked records of a new record in expanded/form mode
Change Summary
For a new record created in expanded/form mode, searching the linked records did not work. I have made it work by filtering the local ephemeral state.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of