foreign key fix#1725
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdded null-safety guards and conditional rendering for foreign-key handling across table and record view/edit components, plus corresponding unit tests to cover null, undefined, and falsy values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (5 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Guards dashboard table row actions and preview “Open the record” affordances against rows whose primary key (including FK-backed PKs) is null/empty, and applies pnpm audit --fix-style dependency overrides.
Changes:
- Add primary-key validity checks in the table data source and row preview component.
- Block/skip edit/duplicate/delete actions (single + bulk) and hide action links when PK is invalid.
- Update pnpm overrides/lockfile to address audit findings (ajv, picomatch, path-to-regexp, lodash, @nestjs/core).
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds/updates pnpm overrides and updates resolved versions from audit fixes. |
| package.json | Adds pnpm override rules for several audited transitive deps. |
| frontend/src/app/components/dashboard/db-tables-data-source.ts | Makes FK-backed PK param extraction null-safe; adds hasValidPrimaryKey. |
| frontend/src/app/components/dashboard/db-table-view/db-table-view.component.ts | Adds guards for single and bulk actions to prevent targeting rows with empty PKs. |
| frontend/src/app/components/dashboard/db-table-view/db-table-view.component.html | Hides edit/duplicate/delete controls when PK is invalid. |
| frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.ts | Adds PK validity check for preview sidebar actions. |
| frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.html | Hides “copy link/open record” when PK is invalid; adds type="button" for close button. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
345
to
372
| @@ -355,7 +355,7 @@ <h2 class="mat-h2 table-name">{{ displayName }}</h2> | |||
| (click)="stashUrlParams(); posthog.capture('Dashboard: edit row is clicked')"> | |||
| <mat-icon fontSet="material-symbols-outlined">create</mat-icon> | |||
| </a> | |||
| <a mat-icon-button *ngIf="canAddRow()" | |||
| <a mat-icon-button *ngIf="canAddRow() && tableData.hasValidPrimaryKey(element)" | |||
| routerLink="/dashboard/{{connectionID}}/{{name}}/entry" | |||
| [queryParams]="tableData.getQueryParams(element, 'dub')" | |||
| class="db-table-cell-action-button" | |||
| @@ -368,7 +368,7 @@ <h2 class="mat-h2 table-name">{{ displayName }}</h2> | |||
| (click)="stashUrlParams(); posthog.capture('Dashboard: duplicate row is clicked')"> | |||
| <mat-icon fontSet="material-symbols-outlined">difference</mat-icon> | |||
| </a> | |||
| <button type="button" mat-icon-button *ngIf="canDeleteRow() && tableData.canDelete" | |||
| <button type="button" mat-icon-button *ngIf="canDeleteRow() && tableData.canDelete && tableData.hasValidPrimaryKey(element)" | |||
| class="db-table-cell-action-button" | |||
There was a problem hiding this comment.
tableData.hasValidPrimaryKey(element) is invoked multiple times per row in this template (edit/duplicate/delete). Because template method calls run on every change-detection pass, this can become a hotspot for large tables. Consider computing a per-row flag once (e.g., assign to a local template variable via an outer ng-container, or precompute/attach the value when formatting rows) and reuse it for all action buttons.
| "path-to-regexp@>=8.0.0 <8.4.0": ">=8.4.0", | ||
| "lodash@>=4.0.0 <=4.17.23": ">=4.18.0", | ||
| "lodash@<=4.17.23": ">=4.18.0", | ||
| "@nestjs/core@<=11.1.17": ">=11.1.18" |
There was a problem hiding this comment.
The new pnpm overrides use open-ended ranges (e.g. ">=8.18.0", ">=4.18.0", ">=11.1.18"). Even with a lockfile, this makes future lock regenerations potentially pick up unreviewed major/minor updates (and may produce large, noisy lock diffs). Consider pinning these overrides to specific patched versions (or at least bounding them with an upper limit) to keep installs reproducible and upgrades intentional.
Suggested change
| "@nestjs/core@<=11.1.17": ">=11.1.18" | |
| "@nestjs/core@<=11.1.17": "11.1.18" |
| path-to-regexp@>=8.0.0 <8.4.0: '>=8.4.0' | ||
| lodash@>=4.0.0 <=4.17.23: '>=4.18.0' | ||
| lodash@<=4.17.23: '>=4.18.0' | ||
| '@nestjs/core@<=11.1.17': '>=11.1.18' |
There was a problem hiding this comment.
The lockfile overrides include multiple broad range remaps (ajv/picomatch/path-to-regexp/lodash/@nestjs/core). If these are intended as security fixes, consider pinning them to concrete versions (or adding upper bounds) to reduce the chance of unintended upgrades when the lockfile is regenerated.
Suggested change
| '@nestjs/core@<=11.1.17': '>=11.1.18' | |
| '@nestjs/core@<=11.1.17': '11.1.18' |
Comment on lines
351
to
357
| getQueryParams(row, action) { | ||
| const params = Object.fromEntries( | ||
| this.keyAttributes.map((column) => { | ||
| if (this.foreignKeysList.includes(column.column_name)) { | ||
| const referencedColumnNameOfForeignKey = this.foreignKeys[column.column_name].referenced_column_name; | ||
| return [column.column_name, row[column.column_name][referencedColumnNameOfForeignKey]]; | ||
| return [column.column_name, row[column.column_name]?.[referencedColumnNameOfForeignKey] ?? null]; | ||
| } |
There was a problem hiding this comment.
getQueryParams now safely handles nullable foreign-key objects, but there is still an unguarded foreign-key dereference earlier in this file when building the primary key object for fetchTableRow (it indexes firstRow[column][referencedColumn] without optional chaining). If a primary key column is also a nullable FK, fetchRows() can still throw before the UI guards ever run. Consider applying the same optional chaining / null handling there as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
package.json (1)
4-4:⚠️ Potential issue | 🔴 CriticalFix the pnpm version source-of-truth mismatch.
CI is already failing because
packageManagerpinspnpm@10.33.0while the GitHub Action also sets pnpmversion: "10". Prefer keeping thispackageManagerpin and removing the workflow-levelversionkey, or make both exactly match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 4, The package.json "packageManager" field is pinned to "pnpm@10.33.0" while the GitHub Action workflow also sets pnpm via a "version" key (mismatched source-of-truth), causing CI failures; fix by making them identical: either keep package.json's packageManager "pnpm@10.33.0" and remove or update the workflow's "version" key to match that exact version, or change package.json.packageManager to match the workflow; locate references to packageManager in package.json and the workflow's pnpm "version" key and make both exact matches.frontend/src/app/components/dashboard/db-tables-data-source.ts (1)
162-172:⚠️ Potential issue | 🟠 MajorGuard the first-row related-record lookup too.
A table whose first row has a null FK primary key still hits Line 166 before the new action guards run, causing a null dereference; a non-FK null primary key still calls
fetchTableRowwith null query params.🐛 Proposed fix
- this._tableRow - .fetchTableRow( - connectionID, - tableName, - res.primaryColumns.reduce((keys, column) => { - if (this.foreignKeysList.includes(column.column_name)) { - const referencedColumnNameOfForeignKey = - this.foreignKeys[column.column_name].referenced_column_name; - keys[column.column_name] = firstRow[column.column_name][referencedColumnNameOfForeignKey]; - } else { - keys[column.column_name] = firstRow[column.column_name]; - } - return keys; - }, {}), - ) - .subscribe((res) => (this.relatedRecords = res.referenced_table_names_and_columns[0])); + const firstRowPrimaryKeys = res.primaryColumns.reduce((keys, column) => { + if (this.foreignKeysList.includes(column.column_name)) { + const referencedColumnNameOfForeignKey = + this.foreignKeys[column.column_name].referenced_column_name; + keys[column.column_name] = + firstRow[column.column_name]?.[referencedColumnNameOfForeignKey] ?? null; + } else { + keys[column.column_name] = firstRow[column.column_name]; + } + return keys; + }, {}); + + const hasValidFirstRowPrimaryKey = Object.values(firstRowPrimaryKeys).every( + (value) => value !== null && value !== undefined && value !== '', + ); + + if (hasValidFirstRowPrimaryKey) { + this._tableRow + .fetchTableRow(connectionID, tableName, firstRowPrimaryKeys) + .subscribe((res) => (this.relatedRecords = res.referenced_table_names_and_columns[0])); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/dashboard/db-tables-data-source.ts` around lines 162 - 172, The code that builds the key object from res.primaryColumns can dereference nulls in firstRow and produce null query params; before accessing firstRow[column.column_name][referencedColumnNameOfForeignKey] or passing values to fetchTableRow, add guards: ensure firstRow[column.column_name] is non-null, and for FKs also ensure referencedColumnNameOfForeignKey exists and firstRow[column.column_name][referencedColumnNameOfForeignKey] is non-null; when a value is missing, omit that key or set it to undefined so fetchTableRow is not called with a null param. Update the block that reduces res.primaryColumns (and any call sites to fetchTableRow) to perform these null checks using the symbols res.primaryColumns, this.foreignKeysList, this.foreignKeys, firstRow, and fetchTableRow.frontend/src/app/components/dashboard/db-table-view/db-table-view.component.html (1)
345-371: 🛠️ Refactor suggestion | 🟠 MajorReplace
*ngIfwith@iffor the three control guards.The changes at lines 345, 358, and 371 add necessary primary-key validation checks, but must use Angular 19's built-in control flow instead of structural directives per coding guidelines.
Refactor to `@if`
- <a mat-icon-button *ngIf="canEditRow() && tableData.hasValidPrimaryKey(element)" + `@if` (canEditRow() && tableData.hasValidPrimaryKey(element)) { + <a mat-icon-button routerLink="/dashboard/{{connectionID}}/{{name}}/entry" [queryParams]="tableData.getQueryParams(element)" class="db-table-cell-action-button" @@ (click)="stashUrlParams(); posthog.capture('Dashboard: edit row is clicked')"> <mat-icon fontSet="material-symbols-outlined">create</mat-icon> </a> - <a mat-icon-button *ngIf="canAddRow() && tableData.hasValidPrimaryKey(element)" + } + `@if` (canAddRow() && tableData.hasValidPrimaryKey(element)) { + <a mat-icon-button routerLink="/dashboard/{{connectionID}}/{{name}}/entry" [queryParams]="tableData.getQueryParams(element, 'dub')" class="db-table-cell-action-button" @@ (click)="stashUrlParams(); posthog.capture('Dashboard: duplicate row is clicked')"> <mat-icon fontSet="material-symbols-outlined">difference</mat-icon> </a> - <button type="button" mat-icon-button *ngIf="canDeleteRow() && tableData.canDelete && tableData.hasValidPrimaryKey(element)" + } + `@if` (canDeleteRow() && tableData.canDelete && tableData.hasValidPrimaryKey(element)) { + <button type="button" mat-icon-button class="db-table-cell-action-button" attr.data-testid="table-delete-record-{{i}}-button" angulartics2On="click" @@ (click)="handleDeleteRow($event, element); posthog.capture('Dashboard: delete row is clicked')"> <mat-icon fontSet="material-symbols-outlined">delete</mat-icon> </button> + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/dashboard/db-table-view/db-table-view.component.html` around lines 345 - 371, Replace the three structural directives that use *ngIf with Angular 19 control-flow `@if` constructs: for the edit anchor that currently uses "*ngIf=\"canEditRow() && tableData.hasValidPrimaryKey(element)\"", replace that *ngIf with an `@if` equivalent while keeping the same condition and all attributes (routerLink, [queryParams], stashUrlParams, etc.); do the same for the duplicate anchor that uses "*ngIf=\"canAddRow() && tableData.hasValidPrimaryKey(element)\"" and for the delete button using "*ngIf=\"canDeleteRow() && tableData.canDelete && tableData.hasValidPrimaryKey(element)\"". Locate these by the surrounding symbols canEditRow(), canAddRow(), canDeleteRow(), tableData.hasValidPrimaryKey(element), and the routerLink/queryParams usages and update the template syntax to use `@if` without changing the conditions or other attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.html`:
- Around line 7-13: Replace the deprecated structural *ngIf usage with Angular
19 control-flow `@if` in the db-table-row-view template: for both the button and
the anchor that currently use *ngIf="selectedRow.link && hasValidPrimaryKey()",
wrap the elements (or use the `@if` inline syntax) so the visibility is driven by
`@if` (selectedRow.link && hasValidPrimaryKey()) instead of *ngIf; keep the
existing bindings and handlers (getDedicatedPageLink(), showCopyNotification())
and ensure the rendered element still provides the matTooltip,
[cdkCopyToClipboard], and (cdkCopyToClipboardCopied) behavior unchanged.
In `@frontend/src/app/components/dashboard/db-tables-data-source.ts`:
- Around line 369-377: The hasValidPrimaryKey function's parameter `row` is
untyped; update the method signature for hasValidPrimaryKey to declare `row:
Record<string, unknown>` so dynamic bracket access works with proper typing, and
ensure references to this.keyAttributes, this.foreignKeysList and
this.foreignKeys remain unchanged (they're the unique symbols to locate the
function) — no other logic changes required.
In `@package.json`:
- Around line 26-31: The pnpm overrides use open-ended lower-bound-only
replacements (e.g., the override keys/values for ajv "ajv@>=7.0.0-alpha.0
<8.18.0": ">=8.18.0" and `@nestjs/core` "@nestjs/core@<=11.1.17": ">=11.1.18",
plus picomatch, path-to-regexp and lodash entries) which permit future-major
upgrades; change each replacement value to a bounded semver range that prevents
major bumps (for example, use >=8.18.0 <9.0.0 for ajv, >=11.1.18 <12.0.0 for
`@nestjs/core`, >=4.0.4 <5.0.0 for picomatch, >=8.4.0 <9.0.0 for path-to-regexp,
and consolidate the two lodash selectors into a single override with a bounded
range like >=4.18.0 <5.0.0) so transitive dependencies cannot resolve to
incompatible major versions.
---
Outside diff comments:
In
`@frontend/src/app/components/dashboard/db-table-view/db-table-view.component.html`:
- Around line 345-371: Replace the three structural directives that use *ngIf
with Angular 19 control-flow `@if` constructs: for the edit anchor that currently
uses "*ngIf=\"canEditRow() && tableData.hasValidPrimaryKey(element)\"", replace
that *ngIf with an `@if` equivalent while keeping the same condition and all
attributes (routerLink, [queryParams], stashUrlParams, etc.); do the same for
the duplicate anchor that uses "*ngIf=\"canAddRow() &&
tableData.hasValidPrimaryKey(element)\"" and for the delete button using
"*ngIf=\"canDeleteRow() && tableData.canDelete &&
tableData.hasValidPrimaryKey(element)\"". Locate these by the surrounding
symbols canEditRow(), canAddRow(), canDeleteRow(),
tableData.hasValidPrimaryKey(element), and the routerLink/queryParams usages and
update the template syntax to use `@if` without changing the conditions or other
attributes.
In `@frontend/src/app/components/dashboard/db-tables-data-source.ts`:
- Around line 162-172: The code that builds the key object from
res.primaryColumns can dereference nulls in firstRow and produce null query
params; before accessing
firstRow[column.column_name][referencedColumnNameOfForeignKey] or passing values
to fetchTableRow, add guards: ensure firstRow[column.column_name] is non-null,
and for FKs also ensure referencedColumnNameOfForeignKey exists and
firstRow[column.column_name][referencedColumnNameOfForeignKey] is non-null; when
a value is missing, omit that key or set it to undefined so fetchTableRow is not
called with a null param. Update the block that reduces res.primaryColumns (and
any call sites to fetchTableRow) to perform these null checks using the symbols
res.primaryColumns, this.foreignKeysList, this.foreignKeys, firstRow, and
fetchTableRow.
In `@package.json`:
- Line 4: The package.json "packageManager" field is pinned to "pnpm@10.33.0"
while the GitHub Action workflow also sets pnpm via a "version" key (mismatched
source-of-truth), causing CI failures; fix by making them identical: either keep
package.json's packageManager "pnpm@10.33.0" and remove or update the workflow's
"version" key to match that exact version, or change package.json.packageManager
to match the workflow; locate references to packageManager in package.json and
the workflow's pnpm "version" key and make both exact matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35185774-5303-49cd-b178-c843fdf5a28e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.htmlfrontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.tsfrontend/src/app/components/dashboard/db-table-view/db-table-view.component.htmlfrontend/src/app/components/dashboard/db-table-view/db-table-view.component.tsfrontend/src/app/components/dashboard/db-tables-data-source.tspackage.json
Comment on lines
+7
to
+13
| <button type="button" mat-icon-button *ngIf="selectedRow.link && hasValidPrimaryKey()" | ||
| matTooltip="Copy link to this record" | ||
| [cdkCopyToClipboard]="getDedicatedPageLink()" | ||
| (cdkCopyToClipboardCopied)="showCopyNotification('Link to this record was copied to clipboard.')"> | ||
| <mat-icon>link</mat-icon> | ||
| </button> | ||
| <a mat-icon-button *ngIf="selectedRow.link" | ||
| <a mat-icon-button *ngIf="selectedRow.link && hasValidPrimaryKey()" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the newly guarded primary-key controls no longer use structural directives.
# Expected: no matches after refactoring touched guards to `@if`.
rg -nP '\*ngIf="[^"]*hasValidPrimaryKey' --iglob '*db-table*.component.html'Repository: rocket-admin/rocketadmin
Length of output: 1058
Use @if instead of *ngIf for the primary-key guards.
Angular 19's built-in control flow must be used in all new code. Lines 7 and 13 use the deprecated *ngIf structural directive and must be refactored to use @if.
♻️ Refactoring required
- <button type="button" mat-icon-button *ngIf="selectedRow.link && hasValidPrimaryKey()"
+ `@if` (selectedRow.link && hasValidPrimaryKey()) {
+ <button type="button" mat-icon-button
matTooltip="Copy link to this record"
[cdkCopyToClipboard]="getDedicatedPageLink()"
(cdkCopyToClipboardCopied)="showCopyNotification('Link to this record was copied to clipboard.')">
<mat-icon>link</mat-icon>
</button>
- <a mat-icon-button *ngIf="selectedRow.link && hasValidPrimaryKey()"
+ <a mat-icon-button
[routerLink]="selectedRow.link"
[queryParams]="getDedicatedPageLinkParams()"
matTooltip="Open the record"
(click)="stashUrlParams()">
<mat-icon fontSet="material-symbols-outlined">create</mat-icon>
</a>
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button type="button" mat-icon-button *ngIf="selectedRow.link && hasValidPrimaryKey()" | |
| matTooltip="Copy link to this record" | |
| [cdkCopyToClipboard]="getDedicatedPageLink()" | |
| (cdkCopyToClipboardCopied)="showCopyNotification('Link to this record was copied to clipboard.')"> | |
| <mat-icon>link</mat-icon> | |
| </button> | |
| <a mat-icon-button *ngIf="selectedRow.link" | |
| <a mat-icon-button *ngIf="selectedRow.link && hasValidPrimaryKey()" | |
| `@if` (selectedRow.link && hasValidPrimaryKey()) { | |
| <button type="button" mat-icon-button | |
| matTooltip="Copy link to this record" | |
| [cdkCopyToClipboard]="getDedicatedPageLink()" | |
| (cdkCopyToClipboardCopied)="showCopyNotification('Link to this record was copied to clipboard.')"> | |
| <mat-icon>link</mat-icon> | |
| </button> | |
| <a mat-icon-button | |
| [routerLink]="selectedRow.link" | |
| [queryParams]="getDedicatedPageLinkParams()" | |
| matTooltip="Open the record" | |
| (click)="stashUrlParams()"> | |
| <mat-icon fontSet="material-symbols-outlined">create</mat-icon> | |
| </a> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.html`
around lines 7 - 13, Replace the deprecated structural *ngIf usage with Angular
19 control-flow `@if` in the db-table-row-view template: for both the button and
the anchor that currently use *ngIf="selectedRow.link && hasValidPrimaryKey()",
wrap the elements (or use the `@if` inline syntax) so the visibility is driven by
`@if` (selectedRow.link && hasValidPrimaryKey()) instead of *ngIf; keep the
existing bindings and handlers (getDedicatedPageLink(), showCopyNotification())
and ensure the rendered element still provides the matTooltip,
[cdkCopyToClipboard], and (cdkCopyToClipboardCopied) behavior unchanged.
Comment on lines
+369
to
+377
| hasValidPrimaryKey(row): boolean { | ||
| if (!this.keyAttributes?.length) return false; | ||
| return this.keyAttributes.every((column) => { | ||
| const value = this.foreignKeysList.includes(column.column_name) | ||
| ? row[column.column_name]?.[this.foreignKeys[column.column_name].referenced_column_name] | ||
| : row[column.column_name]; | ||
| return value !== null && value !== undefined && value !== ''; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the newly added method parameter is no longer untyped.
# Expected: no matches after adding an explicit row parameter type.
rg -nP 'hasValidPrimaryKey\s*\(\s*row\s*\)' --iglob 'db-tables-data-source.ts'Repository: rocket-admin/rocketadmin
Length of output: 170
🏁 Script executed:
sed -n '360,390p' frontend/src/app/components/dashboard/db-tables-data-source.tsRepository: rocket-admin/rocketadmin
Length of output: 576
🏁 Script executed:
head -50 frontend/src/app/components/dashboard/db-tables-data-source.ts | tail -40Repository: rocket-admin/rocketadmin
Length of output: 1339
🏁 Script executed:
grep -n "row" frontend/src/app/components/dashboard/db-tables-data-source.ts | head -20Repository: rocket-admin/rocketadmin
Length of output: 1143
🏁 Script executed:
sed -n '98,110p' frontend/src/app/components/dashboard/db-tables-data-source.tsRepository: rocket-admin/rocketadmin
Length of output: 492
🏁 Script executed:
sed -n '351,365p' frontend/src/app/components/dashboard/db-tables-data-source.tsRepository: rocket-admin/rocketadmin
Length of output: 618
🏁 Script executed:
rg -n "formatRow|getQueryParams|hasValidPrimaryKey" frontend/src/app/components/dashboard/db-tables-data-source.ts | head -10Repository: rocket-admin/rocketadmin
Length of output: 257
Add type annotation to the row parameter.
The row parameter lacks a type annotation. Use Record<string, unknown> to match the dynamic key access pattern used throughout the method.
♻️ Proposed refactor
- hasValidPrimaryKey(row): boolean {
+ hasValidPrimaryKey(row: Record<string, unknown>): boolean {
if (!this.keyAttributes?.length) return false;
return this.keyAttributes.every((column) => {
- const value = this.foreignKeysList.includes(column.column_name)
- ? row[column.column_name]?.[this.foreignKeys[column.column_name].referenced_column_name]
- : row[column.column_name];
+ const primaryKeyValue = row[column.column_name];
+ const value =
+ this.foreignKeysList.includes(column.column_name) &&
+ primaryKeyValue !== null &&
+ typeof primaryKeyValue === 'object'
+ ? (primaryKeyValue as Record<string, unknown>)[
+ this.foreignKeys[column.column_name].referenced_column_name
+ ]
+ : primaryKeyValue;
return value !== null && value !== undefined && value !== '';
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hasValidPrimaryKey(row): boolean { | |
| if (!this.keyAttributes?.length) return false; | |
| return this.keyAttributes.every((column) => { | |
| const value = this.foreignKeysList.includes(column.column_name) | |
| ? row[column.column_name]?.[this.foreignKeys[column.column_name].referenced_column_name] | |
| : row[column.column_name]; | |
| return value !== null && value !== undefined && value !== ''; | |
| }); | |
| } | |
| hasValidPrimaryKey(row: Record<string, unknown>): boolean { | |
| if (!this.keyAttributes?.length) return false; | |
| return this.keyAttributes.every((column) => { | |
| const primaryKeyValue = row[column.column_name]; | |
| const value = | |
| this.foreignKeysList.includes(column.column_name) && | |
| primaryKeyValue !== null && | |
| typeof primaryKeyValue === 'object' | |
| ? (primaryKeyValue as Record<string, unknown>)[ | |
| this.foreignKeys[column.column_name].referenced_column_name | |
| ] | |
| : primaryKeyValue; | |
| return value !== null && value !== undefined && value !== ''; | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/app/components/dashboard/db-tables-data-source.ts` around lines
369 - 377, The hasValidPrimaryKey function's parameter `row` is untyped; update
the method signature for hasValidPrimaryKey to declare `row: Record<string,
unknown>` so dynamic bracket access works with proper typing, and ensure
references to this.keyAttributes, this.foreignKeysList and this.foreignKeys
remain unchanged (they're the unique symbols to locate the function) — no other
logic changes required.
Guards the helpers that extract FK cell values against null/undefined and
updates the two FK display components (table cell, row preview) to render
a dash only when the value is genuinely absent. Using `== null` keeps `0`,
empty string, and `false` as real FK values so rows whose FK id is 0 still
render the link, not a dash.
Previously `Object.keys(null)` threw on any row with a null FK column and
`{{ value() || '—' }}` substituted a dash for legitimately-zero ids.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gugu
force-pushed
the
feature/guard-null-primary-key
branch
from
442cc10 to
fc516b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/app/components/dashboard/db-table-view/db-table-view.component.spec.ts (1)
245-270: Add explicit return types to the new spec callbacks.The added
itcallbacks should declare: voidto keep new TypeScript test code aligned with the project rule.Proposed cleanup
-it('should return null (not throw) when foreign key cell is null', () => { +it('should return null (not throw) when foreign key cell is null', (): void => { const foreignKey = { autocomplete_columns: ['FirstName'], column_name: 'CustomerId', @@ -it('should not throw in isForeignKeySelected when record is null', () => { +it('should not throw in isForeignKeySelected when record is null', (): void => { const foreignKey = { autocomplete_columns: ['FirstName'], column_name: 'CustomerId',As per coding guidelines, "
**/*.{ts,tsx}: Always add type annotations to function parameters and return types in TypeScript`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/dashboard/db-table-view/db-table-view.component.spec.ts` around lines 245 - 270, The two new test callbacks are missing explicit return types; update the `it` callback signatures in db-table-view.component.spec.ts to declare a return type of `: void` for both tests (the one exercising `component.getCellValue` and the one exercising `component.isForeignKeySelected`) so they comply with the project's TypeScript rule requiring function return type annotations.frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.spec.ts (1)
26-96: Keep the new spec fixtures typed.The added callbacks need explicit
: voidreturn types, and the repeatedas anycasts can hide drift in theselectedRowshape. A small typed fixture/helper would keep these tests strict without much extra setup.Example direction
-describe('getForeignKeyValue', () => { +describe('getForeignKeyValue', (): void => { const baseRow = { foreignKeys: { @@ - it('returns null when record field is null', () => { - component.selectedRow = { ...baseRow, record: { user_id: null } } as any; + it('returns null when record field is null', (): void => { + component.selectedRow = { ...baseRow, record: { user_id: null } } as NonNullable< + DbTableRowViewComponent['selectedRow'] + >; expect(component.getForeignKeyValue('user_id')).toBeNull(); });As per coding guidelines, "
**/*.{ts,tsx}: Always add type annotations to function parameters and return types in TypeScript" and "**/*.{ts,tsx}: Avoid any types - use specific types or generics instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.spec.ts` around lines 26 - 96, The tests for getForeignKeyValue and getForeignKeyQueryParams use untyped callbacks and many "as any" casts on selectedRow which can hide shape drift; add explicit ": void" return types to each it(...) callback and replace the repeated casts by introducing a small typed fixture/interface (e.g., a LocalSelectedRow or TestRow type) that models foreignKeys and record, then use that fixture to set component.selectedRow without "as any" to keep tests strictly typed while preserving the same test values; update tests referencing component.selectedRow and both methods (getForeignKeyValue, getForeignKeyQueryParams) to use the typed fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.spec.ts`:
- Around line 84-89: The current getForeignKeyQueryParams implementation returns
an object like { id: null } for object FKs whose referenced id is null; change
getForeignKeyQueryParams to guard against null/undefined referenced values
(e.g., if selectedRow.record[field] is an object but its referenced key value is
null/undefined then return null/undefined instead of an object) so no broken
query is generated, and add a unit test in db-table-row-view.component.spec.ts
mirroring the existing positive case but with record: { user_id: { id: null,
name: 'alice' } } and assert that getForeignKeyQueryParams('user_id') returns
null/undefined (matching your chosen sentinel).
In
`@frontend/src/app/components/ui-components/table-display-fields/foreign-key/foreign-key.component.ts`:
- Around line 20-27: Replace the untyped payload and event in
onForeignKeyClick/handleForeignKeyClick by defining a specific interface (e.g.,
ForeignKeyClickPayload with foreignKey: ForeignKeyValue and value: string |
number | ... as appropriate) and change the handler signature to
handleForeignKeyClick(event: MouseEvent): void; inside handleForeignKeyClick
cache const relations = this.relations(); const value = this.value(); before the
null/guard so those narrowed types are used in the emit call
this.onForeignKeyClick.emit({ foreignKey: relations, value }); also update the
ForeignKeyValue type definition to include any additional allowed value types
beyond string|number and remove all uses of any.
---
Nitpick comments:
In
`@frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.spec.ts`:
- Around line 26-96: The tests for getForeignKeyValue and
getForeignKeyQueryParams use untyped callbacks and many "as any" casts on
selectedRow which can hide shape drift; add explicit ": void" return types to
each it(...) callback and replace the repeated casts by introducing a small
typed fixture/interface (e.g., a LocalSelectedRow or TestRow type) that models
foreignKeys and record, then use that fixture to set component.selectedRow
without "as any" to keep tests strictly typed while preserving the same test
values; update tests referencing component.selectedRow and both methods
(getForeignKeyValue, getForeignKeyQueryParams) to use the typed fixture.
In
`@frontend/src/app/components/dashboard/db-table-view/db-table-view.component.spec.ts`:
- Around line 245-270: The two new test callbacks are missing explicit return
types; update the `it` callback signatures in db-table-view.component.spec.ts to
declare a return type of `: void` for both tests (the one exercising
`component.getCellValue` and the one exercising
`component.isForeignKeySelected`) so they comply with the project's TypeScript
rule requiring function return type annotations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cfc22dc-edec-4505-8cc7-c358c99bea68
📒 Files selected for processing (9)
frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.spec.tsfrontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.tsfrontend/src/app/components/dashboard/db-table-view/db-table-view.component.spec.tsfrontend/src/app/components/dashboard/db-table-view/db-table-view.component.tsfrontend/src/app/components/ui-components/record-view-fields/foreign-key/foreign-key.component.htmlfrontend/src/app/components/ui-components/record-view-fields/foreign-key/foreign-key.component.spec.tsfrontend/src/app/components/ui-components/table-display-fields/foreign-key/foreign-key.component.htmlfrontend/src/app/components/ui-components/table-display-fields/foreign-key/foreign-key.component.spec.tsfrontend/src/app/components/ui-components/table-display-fields/foreign-key/foreign-key.component.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/components/dashboard/db-table-view/db-table-view.component.ts
- frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.ts
Comment on lines
+84
to
+89
| it('returns referenced column param when FK is an object', () => { | ||
| component.selectedRow = { | ||
| ...baseRow, | ||
| record: { user_id: { id: 42, name: 'alice' } }, | ||
| } as any; | ||
| expect(component.getForeignKeyQueryParams('user_id')).toEqual({ id: 42 }); |
There was a problem hiding this comment.
Cover object FKs whose referenced value is null.
This positive object case still misses the null-reference branch. With the current implementation in frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.ts:272-277, { user_id: { id: null, name: 'alice' } } returns { id: null }, which can still generate the broken query this PR is meant to prevent.
Suggested regression and guard
it('returns referenced column param when FK is an object', () => {
component.selectedRow = {
...baseRow,
record: { user_id: { id: 42, name: 'alice' } },
} as any;
expect(component.getForeignKeyQueryParams('user_id')).toEqual({ id: 42 });
});
+
+it('returns {} when FK object referenced column is null', () => {
+ component.selectedRow = {
+ ...baseRow,
+ record: { user_id: { id: null, name: 'alice' } },
+ } as any;
+ expect(component.getForeignKeyQueryParams('user_id')).toEqual({});
+});Implementation-side guard:
const cell = this.selectedRow?.record?.[field];
const referencedColumnName = this.selectedRow?.foreignKeys?.[field]?.referenced_column_name;
if (cell == null || !referencedColumnName) return {};
-return { [referencedColumnName]: typeof cell === 'object' ? cell[referencedColumnName] : cell };
+const value = typeof cell === 'object' ? cell[referencedColumnName] : cell;
+if (value == null) return {};
+return { [referencedColumnName]: value };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('returns referenced column param when FK is an object', () => { | |
| component.selectedRow = { | |
| ...baseRow, | |
| record: { user_id: { id: 42, name: 'alice' } }, | |
| } as any; | |
| expect(component.getForeignKeyQueryParams('user_id')).toEqual({ id: 42 }); | |
| it('returns referenced column param when FK is an object', () => { | |
| component.selectedRow = { | |
| ...baseRow, | |
| record: { user_id: { id: 42, name: 'alice' } }, | |
| } as any; | |
| expect(component.getForeignKeyQueryParams('user_id')).toEqual({ id: 42 }); | |
| }); | |
| it('returns {} when FK object referenced column is null', () => { | |
| component.selectedRow = { | |
| ...baseRow, | |
| record: { user_id: { id: null, name: 'alice' } }, | |
| } as any; | |
| expect(component.getForeignKeyQueryParams('user_id')).toEqual({}); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/dashboard/db-table-view/db-table-row-view/db-table-row-view.component.spec.ts`
around lines 84 - 89, The current getForeignKeyQueryParams implementation
returns an object like { id: null } for object FKs whose referenced id is null;
change getForeignKeyQueryParams to guard against null/undefined referenced
values (e.g., if selectedRow.record[field] is an object but its referenced key
value is null/undefined then return null/undefined instead of an object) so no
broken query is generated, and add a unit test in
db-table-row-view.component.spec.ts mirroring the existing positive case but
with record: { user_id: { id: null, name: 'alice' } } and assert that
getForeignKeyQueryParams('user_id') returns null/undefined (matching your chosen
sentinel).
Comment on lines
+20
to
+27
| readonly onForeignKeyClick = output<{ foreignKey: any; value: string }>(); | ||
|
|
||
| handleForeignKeyClick($event): void { | ||
| $event.stopPropagation(); | ||
| if (this.relations() && this.value()) { | ||
| this.onForeignKeyClick.emit({ | ||
| foreignKey: this.relations(), | ||
| value: this.value() | ||
| }); | ||
| } | ||
| } | ||
| handleForeignKeyClick($event): void { | ||
| $event.stopPropagation(); | ||
| if (this.relations() && this.value() != null) { | ||
| this.onForeignKeyClick.emit({ | ||
| foreignKey: this.relations(), | ||
| value: this.value(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the actual base field value input type and FK click consumers before finalizing ForeignKeyValue.
rg -nP -C4 '\b(value|readonly value)\s*=\s*input\b|\bvalue\s*\(\)' --iglob '*.ts'
rg -nP -C3 '\bonForeignKeyClick\b|handleForeignKeyClick' --iglob '*.ts' --iglob '*.html'Repository: rocket-admin/rocketadmin
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full foreign-key.component.ts file
cat frontend/src/app/components/ui-components/table-display-fields/foreign-key/foreign-key.component.ts
# Get the base class definition
fd -t f 'base-table-display-field.component.ts' --type fRepository: rocket-admin/rocketadmin
Length of output: 1491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get base class
cat frontend/src/app/components/ui-components/table-display-fields/base-table-display-field/base-table-display-field.component.ts
# Search for onForeignKeyClick usage in templates
fd -t f -name '*.html' | xargs grep -l 'onForeignKeyClick' 2>/dev/null || trueRepository: rocket-admin/rocketadmin
Length of output: 996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for onForeignKeyClick consumers in HTML and TS
rg -i 'onForeignKeyClick' --type ts --type html -C2
# Also check the template for this component
cat frontend/src/app/components/ui-components/table-display-fields/foreign-key/foreign-key.component.htmlRepository: rocket-admin/rocketadmin
Length of output: 5356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what types are actually passed as element[column] in consumers
rg 'handleForeignKeyView|getCellValue' frontend/src/app/components/dashboard/db-table-view/db-table-view.component.ts -A3 -B1Repository: rocket-admin/rocketadmin
Length of output: 402
Type the FK click payload and event parameter explicitly.
Line 20 uses any for the foreignKey property, and line 22 leaves $event untyped. Create a typed event interface and declare the event parameter as MouseEvent. Also cache relations() and value() before the guard so the narrowed types flow to the emit call.
Suggested fix
+type ForeignKeyValue = string | number;
+
+interface ForeignKeyClickEvent {
+ foreignKey: TableForeignKey;
+ value: ForeignKeyValue;
+}
+
`@Component`({
@@
- readonly onForeignKeyClick = output<{ foreignKey: any; value: string }>();
+ readonly onForeignKeyClick = output<ForeignKeyClickEvent>();
- handleForeignKeyClick($event): void {
- $event.stopPropagation();
- if (this.relations() && this.value() != null) {
+ handleForeignKeyClick(event: MouseEvent): void {
+ event.stopPropagation();
+ const foreignKey = this.relations();
+ const value = this.value();
+ if (foreignKey && value != null) {
this.onForeignKeyClick.emit({
- foreignKey: this.relations(),
- value: this.value(),
+ foreignKey,
+ value,
});
}
}Adjust ForeignKeyValue if the base field's value type supports additional types beyond string | number. Per coding guidelines, **/*.{ts,tsx} must avoid any, use specific types, and annotate function parameters and return types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| readonly onForeignKeyClick = output<{ foreignKey: any; value: string }>(); | |
| handleForeignKeyClick($event): void { | |
| $event.stopPropagation(); | |
| if (this.relations() && this.value()) { | |
| this.onForeignKeyClick.emit({ | |
| foreignKey: this.relations(), | |
| value: this.value() | |
| }); | |
| } | |
| } | |
| handleForeignKeyClick($event): void { | |
| $event.stopPropagation(); | |
| if (this.relations() && this.value() != null) { | |
| this.onForeignKeyClick.emit({ | |
| foreignKey: this.relations(), | |
| value: this.value(), | |
| type ForeignKeyValue = string | number; | |
| interface ForeignKeyClickEvent { | |
| foreignKey: TableForeignKey; | |
| value: ForeignKeyValue; | |
| } | |
| readonly onForeignKeyClick = output<ForeignKeyClickEvent>(); | |
| handleForeignKeyClick(event: MouseEvent): void { | |
| event.stopPropagation(); | |
| const foreignKey = this.relations(); | |
| const value = this.value(); | |
| if (foreignKey && value != null) { | |
| this.onForeignKeyClick.emit({ | |
| foreignKey, | |
| value, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/app/components/ui-components/table-display-fields/foreign-key/foreign-key.component.ts`
around lines 20 - 27, Replace the untyped payload and event in
onForeignKeyClick/handleForeignKeyClick by defining a specific interface (e.g.,
ForeignKeyClickPayload with foreignKey: ForeignKeyValue and value: string |
number | ... as appropriate) and change the handler signature to
handleForeignKeyClick(event: MouseEvent): void; inside handleForeignKeyClick
cache const relations = this.relations(); const value = this.value(); before the
null/guard so those narrowed types are used in the emit call
this.onForeignKeyClick.emit({ foreignKey: relations, value }); also update the
ForeignKeyValue type definition to include any additional allowed value types
beyond string|number and remove all uses of any.
Appends a "— empty" option to the autocomplete suggestions whenever the column is nullable, sourced from either structure.allow_null or widget_params.allow_null. Selecting it emits null, clears the related link, and hides the "open related record" icon. The option sits at the end of the list so mat-autocomplete's autoActiveFirstOption highlights the real current selection, not the empty placeholder. Also threads structure through the widget-branch ndc-dynamic in the row-edit template so the flag reaches the widget when an explicit Foreign_key widget is configured. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gugu
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.ts (3)
176-183: ClearcurrentFieldQueryParamswhen the typed string matches the null option.When
currentDisplayedString === '— empty', this branch setscurrentFieldValue = nulland emitsnull, but leaves any previously setcurrentFieldQueryParamsintact. It is harmless in the UI today because the template hides the link whencurrentFieldValue == null, but it diverges fromupdateRelatedLink, which explicitly clears the query params on null selection. Aligning them prevents a future template change from leaking a stale href.♻️ Proposed fix
if (currentRow !== undefined) { this.currentFieldValue = currentRow.fieldValue; + if (currentRow.isNullOption) { + this.currentFieldQueryParams = undefined; + } else { + this.currentFieldQueryParams = currentRow.primaryKeys; + } this.onFieldChange.emit(this.currentFieldValue); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.ts` around lines 176 - 183, In fetchSuggestions(), when the typed string matches the null option (currentDisplayedString === '— empty') and you set currentFieldValue = null and emit via onFieldChange, also clear currentFieldQueryParams (e.g., set it to undefined or {}) to mirror updateRelatedLink behavior and avoid leaving a stale query param; update the branch in fetchSuggestions() that handles the null-selection path to reset currentFieldQueryParams whenever currentFieldValue is set to null.
272-274: Private method should be underscore-prefixed.Other private members in this class (
_tables,_connections,_debounceTimer) follow the convention;withNullOptionshould too.- private withNullOption(suggestions: Suggestion[]): Suggestion[] { - return this.allowsNull() ? [...suggestions, nullSuggestion] : suggestions; - } + private _withNullOption(suggestions: Suggestion[]): Suggestion[] { + return this.allowsNull() ? [...suggestions, nullSuggestion] : suggestions; + }Update the three call sites (
this.withNullOption(...)at Lines 138, 201, 209) accordingly.As per coding guidelines: "Prefix private members with underscore (e.g.,
_privateMethod,_dataService)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.ts` around lines 272 - 274, Rename the private method withNullOption to _withNullOption and update all internal call sites that invoke this.withNullOption(...) to use this._withNullOption(...); keep the method marked private and preserve its behavior (returning allowsNull() ? [...suggestions, nullSuggestion] : suggestions) so other class members like _tables, _connections, and _debounceTimer follow the underscore-prefixed private naming convention.
261-270: Considernullinstead ofundefinedforcurrentFieldQueryParams.
[queryParams]onrouterLinkexpectsParams | null | undefined, so both work, but the field is typedRecord<string, unknown>(non-optional) and is read in the template as a truthy-ish binding. For internal type consistency, assigningnull(and typing the fieldRecord<string, unknown> | null) is cleaner. Minor; the template already gates visibility oncurrentFieldValue != null, so there is no runtime user-visible issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.ts` around lines 261 - 270, The updateRelatedLink handler and the field currentFieldQueryParams should use null instead of undefined for clearer typing and template checks: in updateRelatedLink (and any other places setting currentFieldQueryParams) assign null when clearing the selection (e.g., when selected?.isNullOption) and set the field to selected.primaryKeys otherwise; change the currentFieldQueryParams declaration/type from Record<string, unknown> to Record<string, unknown> | null and initialize it to null so the template and other code read a consistent nullable value; keep currentFieldValue and onFieldChange logic as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.ts`:
- Around line 176-183: In fetchSuggestions(), when the typed string matches the
null option (currentDisplayedString === '— empty') and you set currentFieldValue
= null and emit via onFieldChange, also clear currentFieldQueryParams (e.g., set
it to undefined or {}) to mirror updateRelatedLink behavior and avoid leaving a
stale query param; update the branch in fetchSuggestions() that handles the
null-selection path to reset currentFieldQueryParams whenever currentFieldValue
is set to null.
- Around line 272-274: Rename the private method withNullOption to
_withNullOption and update all internal call sites that invoke
this.withNullOption(...) to use this._withNullOption(...); keep the method
marked private and preserve its behavior (returning allowsNull() ?
[...suggestions, nullSuggestion] : suggestions) so other class members like
_tables, _connections, and _debounceTimer follow the underscore-prefixed private
naming convention.
- Around line 261-270: The updateRelatedLink handler and the field
currentFieldQueryParams should use null instead of undefined for clearer typing
and template checks: in updateRelatedLink (and any other places setting
currentFieldQueryParams) assign null when clearing the selection (e.g., when
selected?.isNullOption) and set the field to selected.primaryKeys otherwise;
change the currentFieldQueryParams declaration/type from Record<string, unknown>
to Record<string, unknown> | null and initialize it to null so the template and
other code read a consistent nullable value; keep currentFieldValue and
onFieldChange logic as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebb78d32-39cb-48ca-99bc-3735d1119955
📒 Files selected for processing (5)
frontend/src/app/components/db-table-row-edit/db-table-row-edit.component.htmlfrontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.cssfrontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.htmlfrontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.spec.tsfrontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/app/components/ui-components/record-edit-fields/foreign-key/foreign-key.component.css
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prevents edit/duplicate/delete buttons and the preview sidebar's "Open the record" link from producing broken queries (?pk=null) when a row's primary key is null, empty, or a null foreign-key reference. Also applies pnpm audit --fix overrides.
Summary by CodeRabbit
New Features
Bug Fixes
Tests