Conversation
…dating columns view logic
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe AI service's table settings builder now computes required fields without defaults from the schema and enforces their inclusion in column views while preventing them from being marked readonly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Updates AI-generated TableSettingsEntity creation to better handle required DB columns by preventing them from being marked read-only and ensuring they are visible in the configured columns view.
Changes:
- Derive a set of non-nullable columns with no default (excluding autoincrement) from table structure metadata.
- Remove those required fields from
readonly_fields. - Ensure those required fields are present in
columns_view.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const requiredFieldsWithoutDefault = new Set( | ||
| tableInfo?.structure | ||
| .filter((col) => !col.allow_null && col.column_default === null && !checkFieldAutoincrement(col.column_default, col.extra)) |
There was a problem hiding this comment.
requiredFieldsWithoutDefault calls checkFieldAutoincrement(col.column_default, col.extra) in a branch where col.column_default is guaranteed to be null. The helper is typed as checkFieldAutoincrement(defaultValue: string, ...), so this is type-inconsistent and can break compilation under strictNullChecks (and makes the intent unclear). Consider updating the helper signature to accept string | null (and extra?: string | null) or coalescing the argument at the call site.
| .filter((col) => !col.allow_null && col.column_default === null && !checkFieldAutoincrement(col.column_default, col.extra)) | |
| .filter( | |
| (col) => | |
| !col.allow_null && | |
| col.column_default === null && | |
| !checkFieldAutoincrement(col.column_default ?? '', col.extra), | |
| ) |
| for (const requiredField of requiredFieldsWithoutDefault) { | ||
| if (!filteredColumnsView.includes(requiredField)) { | ||
| filteredColumnsView.push(requiredField); |
There was a problem hiding this comment.
The loop that appends requiredFieldsWithoutDefault to filteredColumnsView does an includes check for each required field, resulting in O(n*m) behavior for wide tables. Converting filteredColumnsView to a Set for membership checks (and back to an array at the end) would make this linear and simpler to reason about.
| for (const requiredField of requiredFieldsWithoutDefault) { | |
| if (!filteredColumnsView.includes(requiredField)) { | |
| filteredColumnsView.push(requiredField); | |
| const filteredColumnsViewSet = new Set(filteredColumnsView); | |
| for (const requiredField of requiredFieldsWithoutDefault) { | |
| if (!filteredColumnsViewSet.has(requiredField)) { | |
| filteredColumnsView.push(requiredField); | |
| filteredColumnsViewSet.add(requiredField); |
Summary by CodeRabbit