M3-06 slice 2: inline grid editing — a cell edited in a popover over it

[anandghegde]

What and why

Work package M3-06 (editing surfaces), slice 2 of 4. Slice 1 (#2) made values editable in the inspector; this slice lets them be edited in the grid, where they're read.

• Grid: double-clicking an attribute cell in an editable store opens CellEditor, a popover anchored to the cell.
  • It has the same text field, refusal message and Set to Nil as the inspector.
  • Return stages the value and closes it. Escape, or a click elsewhere, closes it without staging anything.
  • The staged value and any validation issue come back into the cell once the rows are read again.
  • It's a popover rather than the cell's own label because the grid reloads and reuses its cells as pages arrive and edits are staged, which would overwrite a label being typed into.
• One set of rules: what can be edited, how text becomes a value (ValueText), and how it's staged moved from InspectorModel into ProjectContext (ValueEditing.swift). The inspector delegates to it, and the grid uses it the same way.
• Docs: slice 2 added to the M3-06 progress entry.

Checked here: lint and layering are clean. The one naming pattern in the grid code that could have been ambiguous (a local that shares its name with the method it calls) was compiled and run with Swift 6.2.3. No new user-visible strings (Set to Nil already exists).

PRD IDs: EDT-3 (also BRW-3)

Checklist

• Clean-room: this change is based only on public documentation and on Apple frameworks observed through public APIs. I did not decompile or inspect any closed-source product.
• Commits are signed off (git commit -s, DCO).
• Tests added or updated; swift test passes. EditingWindowTests +1 (read-only store and the grid's own columns offer nothing; an attribute cell's editor explains what it can't read and stages what it can, which the grid then shows). macOS runs in this CI.
• No new strict-concurrency warnings. To be confirmed by CI.
• Public engine API has DocC comments. No engine API change.
• No row data is logged (store-derived values are wrapped in Redacted).
• User-visible strings are externalised; new controls have accessibility labels. The editor's text field is labelled with the column's name.
• ADR added or amended if a decision changed. No decision changed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AT9wkNZ97Eo3MvLLtP7JHP

---

Generated by Claude Code

Summary by CodeRabbit

• New Features
  • Double-click an attribute cell in an editable store to edit its value directly in the grid.
  • Press Return to validate and stage a change. Invalid entries stay open with an explanation.
  • Clear optional values with Set to Nil. Press Escape or click elsewhere to cancel without saving.
• Bug Fixes
  • Grid edits now apply the same type-aware validation and value handling as the inspector.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 50 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: c081a826-b097-4dbe-a865-e0c8154d2779

📥 Commits

Reviewing files that changed from the base of the PR and between 4c96861 and 6816b6e.

📒 Files selected for processing (1)

• App/CoreDataDabbiTests/EditingSessionTests.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 9269d150-254b-41ec-b904-13387f31412d

📥 Commits

Reviewing files that changed from the base of the PR and between a892abf and 4c96861.

📒 Files selected for processing (7)

• App/CoreDataDabbi/Editing/ValueEditing.swift
• App/CoreDataDabbi/Grid/CellEditor.swift
• App/CoreDataDabbi/Grid/GridViewController.swift
• App/CoreDataDabbi/Inspector/DetailsTab.swift
• App/CoreDataDabbi/Inspector/InspectorModel.swift
• App/CoreDataDabbiTests/EditingSessionTests.swift
• docs/IMPLEMENTATION_PLAN.md

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The project context now provides shared field-editing operations. The grid uses them to edit eligible attribute cells in a popover. The inspector delegates its field-editing setup to the same operations.

Changes

Inline grid editing

Layer / File(s)	Summary
Shared field-editing rules App/CoreDataDabbi/Editing/ValueEditing.swift, App/CoreDataDabbi/Inspector/InspectorModel.swift, App/CoreDataDabbi/Inspector/DetailsTab.swift	ProjectContext now checks attribute eligibility, parses and stages text, clears optional values, and creates FieldEditing values. The inspector delegates field-editing setup to the shared operations.
Grid cell editor App/CoreDataDabbi/Grid/GridViewController.swift, App/CoreDataDabbi/Grid/CellEditor.swift, App/CoreDataDabbiTests/EditingSessionTests.swift, docs/IMPLEMENTATION_PLAN.md	A double-click opens a popover for an eligible attribute cell. The editor can stage valid input, show validation errors, clear optional values, or close without staging. The test covers eligibility, validation, staging, and the grid display. The implementation plan records the editing flow and notes that keyboard access remains future work.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant GridViewController
  participant ProjectContext
  participant CellEditorView
  User->>GridViewController: Double-click an attribute cell
  GridViewController->>ProjectContext: Request fieldEditing
  ProjectContext-->>GridViewController: Return FieldEditing when eligible
  GridViewController->>CellEditorView: Present editor popover
  User->>CellEditorView: Submit draft
  CellEditorView->>ProjectContext: Stage draft through FieldEditing

Loading

Merge Risk: ⚪ Minimal · up to 4c968

The inline editor and shared editing rules have no established merge-blocking issue in the supplied evidence. Complete the pending test and concurrency checks before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 52.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 6 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: inline grid editing through a popover. It is specific and related to the pull request objectives.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 52.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 6 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[anandghegde]

CI on 4c96861 had three red checks. One was caused by this PR and is fixed in 6816b6e. The other two come from code this PR doesn't touch; details below.

App (macOS 26), caused by this PR, fixed in 6816b6e. aCellIsEditedWhereItIs found the popover not shown right after editCell opened it. A transient popover in an app that isn't active can close again at once, and the test host isn't always the active app. The test now checks that the editor was opened as a transient popover holding CellEditorView. The rest of the test (the refusal, the staged value, the grid showing it) already passed.

Test (macos-26, Xcode 26.4), not caused by this PR. LoadMoreTests.concurrentRequestsExtendTheListOnce failed with counts == [20, 30] instead of [20, 20]. This PR changes only App/ and docs, and the test is in the engine. The cause is a race in StoreSession.loadMore:

• The test fires two loadMore calls with the same handle. The call reads its offset from the list as it is when it runs, not from the handle.
• If the second call reaches the actor only after the first has finished, it starts at 20 and extends the list a second time.
• That only happens when the runner is loaded; the suite took 7.3 s here.

No fix exists yet. This is the proposed patch, which I'll land as its own change right after this PR:

// StoreSession.loadMore, after `guard pager.hasMore`:
// The list is longer than this handle knew: another request extended it meanwhile, and that is the
// extension this one asked for.
guard pager.ids.count <= handle.count else { return Self.handle(handle, reflecting: pager) }

Test (macos-15, Xcode 26.3), not caused by this PR. This is the same VersionLog.swift:254 "sending 'connection' risks causing data races" compile error that main already has. The job is a continue-on-error canary.

The push of 6816b6e starts a fresh CI run, so I'm not re-running the old one.

---

Generated by Claude Code