Second Look
GitHub

Real pull requests, after review

This is the reviewer's page for a pull request that has been through the whole flow: the AI's first pass, an engineer's decisions, a fix, and a second review. It is read-only because the reviews are published. What you see matches what the author got on GitHub. Green-edged findings were confirmed and sent. Greyed ones were rejected and the author never saw them.

To confirm and reject findings yourself, use the playground.

Rename customer Phone to PrimaryPhone

Changliu53/fieldops-platform-sample#2 by Changliu53, commit daf9d20

Summary for the author

high risk

This change is meant to rename the customer's "Phone" field to "PrimaryPhone". The database update does not actually rename it. It deletes the existing Phone column, which erases every customer's phone number, and then creates a new, empty PrimaryPhone column. Undoing the update has the same problem in reverse.

Ask them to double check

  • Run the migration against a copy of production data. Confirm that customers who had a phone number still have it under PrimaryPhone afterwards.
  • Make sure the migration's designer file and the updated model snapshot are part of this pull request.
  • Add a note to the description saying what happens to existing customer rows, as the team rules require.
  • Search the whole codebase, including raw SQL, reports, API responses and the front end, for anything that still uses "Phone". The customer list loading does not prove every screen or export still works.
  1. criticalThe cause: this is a rename done as "delete the old column, then add a new one", and deleting a column deletes the data in it. When this runs in production, every existing customer's phone number is permanently erased, and PrimaryPhone starts empty for everyone. The team rules say renames must use RenameColumn so existing rows keep their data.confirmed
  2. warningThe migration is added without its designer file (20261120100000_RenameCustomerPhone.Designer.cs), and the model snapshot is not updated. Without the designer file, EF may not discover or apply the migration. A stale snapshot also makes the next generated migration try to make this change again. The team rules require both files.confirmed
  3. warningThe rollback (Down) has the same problem. It drops PrimaryPhone and adds an empty Phone column. If the team ever rolls this migration back, all phone numbers entered since the deploy are lost too.confirmed

src/FieldOps.Api/Data/Entities/Customer.csmodifiedWhole file on GitHub

@@ -5,6 +5,6 @@ public class Customer : ITenantOwned
5 public int Id { get; set; }
6 public int TenantId { get; set; }
7 public string Name { get; set; } = "";
- public string? Phone { get; set; }
8+ public string? PrimaryPhone { get; set; }
9 public List<Well> Wells { get; set; } = new();
10 }

src/FieldOps.Api/Data/Migrations/20261120100000_RenameCustomerPhone.csaddedWhole file on GitHub

@@ -0,0 +1,40 @@
1+using Microsoft.EntityFrameworkCore.Migrations;
2+
3+#nullable disable
4+
5+namespace FieldOps.Api.Data.Migrations
6+{
7+ /// <inheritdoc />
8+ public partial class RenameCustomerPhone : Migration
9+ {
10+ /// <inheritdoc />
11+ protected override void Up(MigrationBuilder migrationBuilder)
12+ {
13+ migrationBuilder.DropColumn(
14+ name: "Phone",
15+ table: "Customers");
16+
17+ migrationBuilder.AddColumn<string>(
18+ name: "PrimaryPhone",
19+ table: "Customers",
20+ type: "nvarchar(32)",
21+ maxLength: 32,
22+ nullable: true);
criticaldata-loss20261120100000_RenameCustomerPhone.cs:13-2297% sureconfirmed by Changliu53

The cause: this is a rename done as "delete the old column, then add a new one", and deleting a column deletes the data in it. When this runs in production, every existing customer's phone number is permanently erased, and PrimaryPhone starts empty for everyone. The team rules say renames must use RenameColumn so existing rows keep their data.

Suggested fix: Replace the DropColumn/AddColumn pair in Up with migrationBuilder.RenameColumn(name: "Phone", table: "Customers", newName: "PrimaryPhone");.

23+ }
24+
25+ /// <inheritdoc />
26+ protected override void Down(MigrationBuilder migrationBuilder)
27+ {
28+ migrationBuilder.DropColumn(
29+ name: "PrimaryPhone",
30+ table: "Customers");
31+
32+ migrationBuilder.AddColumn<string>(
33+ name: "Phone",
34+ table: "Customers",
35+ type: "nvarchar(32)",
36+ maxLength: 32,
37+ nullable: true);
warningdata-loss20261120100000_RenameCustomerPhone.cs:28-3790% sureconfirmed by Changliu53

The rollback (Down) has the same problem. It drops PrimaryPhone and adds an empty Phone column. If the team ever rolls this migration back, all phone numbers entered since the deploy are lost too.

Suggested fix: In Down, use migrationBuilder.RenameColumn(name: "PrimaryPhone", table: "Customers", newName: "Phone");.

38+ }
39+ }
40+}
warningbug20261120100000_RenameCustomerPhone.cs:1-4075% sureconfirmed by Changliu53

The migration is added without its designer file (20261120100000_RenameCustomerPhone.Designer.cs), and the model snapshot is not updated. Without the designer file, EF may not discover or apply the migration. A stale snapshot also makes the next generated migration try to make this change again. The team rules require both files.

Suggested fix: Generate the migration with dotnet ef migrations add RenameCustomerPhone so the designer file and snapshot update are created. Then edit Up and Down to use RenameColumn, and commit all three files.