feat(cockpit): curation inspector — decide / rename / retag over the OPFS corpus (ADR-0027 Slice 6) - #100
Conversation
…OPFS corpus (ADR-0027 Slice 6) Select a chunk in the corpus dock to open a curation inspector that approves / rejects, renames, and retags it — each edit routed through the shared griff_ui_core::curation JSON->JSON ops (no reimplementation, ADR-0016) and re-persisted to its OPFS chunk.json. - dock: chunk rows are now selectable (with a reviewer check/cross/? mark); selecting one shows the inspector (rename field, approve/reject, tag toggles). - CockpitApp::curate applies a CurationAction through decide_record / rename_record / set_tags and updates the in-memory corpus; apply_curation persists the result (OPFS on web via web::persist, the cwd on native). - chunk_filename is extracted (shared by capture + curation) so an edit overwrites its own file. Split / merge are deferred: they need source.bar_range, which capture-built chunks leave None (a CLI-split-corpus concern, a later slice). cockpit 23 tests pass (curate decide/rename/retag + headless inspector render); native + wasm clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ADR-0027 Slice 6 chunk curation to the cockpit corpus dock. ChangesChunk Curation Inspector (ADR-0027 Slice 6)
Sequence Diagram(s)sequenceDiagram
participant User
participant corpus_dock
participant inspector
participant apply_curation
participant griff_ui_core_curation
participant persist_chunk
User->>corpus_dock: clicks chunk row
corpus_dock->>corpus_dock: set selected = chunk id
User->>inspector: edits rename / clicks approve or reject / toggles tag
inspector-->>corpus_dock: pending CurationAction
corpus_dock->>apply_curation: id + CurationAction
apply_curation->>griff_ui_core_curation: JSON→JSON (decide_record / rename_record / set_tags)
griff_ui_core_curation-->>apply_curation: updated JSON
apply_curation->>persist_chunk: write chunk.json to OPFS or native fs
persist_chunk-->>apply_curation: ok / err
apply_curation-->>corpus_dock: dock_status message
corpus_dock-->>User: status shown in dock
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ 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)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91aa38e31a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// | ||
| /// # Errors | ||
| /// A message if the chunk is absent or the op rejects the edit. | ||
| fn curate(&mut self, id: &str, action: &CurationAction) -> Result<String, String> { |
There was a problem hiding this comment.
This commit adds the non-trivial curation implementation and its covering tests together, but AGENTS.md requires the TDD red-green sequence for non-trivial changes: commit the failing tests before touching implementation, then commit the minimal implementation separately. Please split this change into the required test-first and implementation commits so the commit sequence is reviewable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — the curation impl and its tests land in one commit, against AGENTS.md's red→green rule. As on #99, the maintainer's call here is to leave the branch history as-is: the flattened PR diff shows tests + implementation together by construction either way, and the curation logic routes through the already-tested griff_ui_core::curation ops, with curate/the inspector covered by the cockpit tests. A deliberate decision, not an oversight.
Generated by Claude Code
| #[cfg(target_arch = "wasm32")] | ||
| #[allow(clippy::unnecessary_wraps)] | ||
| fn persist_chunk(filename: &str, json: &str) -> Result<(), String> { | ||
| web::persist(filename, json); |
There was a problem hiding this comment.
On wasm, web::persist starts an async OPFS write and only logs failures, but this wrapper immediately returns Ok(()). When a curator approves/renames/retags and then quickly uses Corpus/Manifest—or when OPFS fails due to quota/permissions—the dock reports saved while the persisted file can still be stale or never updated, so the generated manifest can drop the edit. The save path needs an acknowledgement or pending/error state before claiming success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch on the async gap. Decision: accept it, consistent with the existing capture path — do_capture → web::persist is the same fire-and-forget OPFS write that reports saved optimistically, and the manifest race (reading before the write lands) is the one the capture web-test already handles by polling OPFS. A confirmed write-ack would be a cross-cutting change to the shared web::persist (and capture), so it's deferred rather than bolted onto curation alone. The native persist is synchronous and now rolls back the in-memory edit on failure.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cockpit/src/lib.rs`:
- Around line 665-668: The apply_curation function reads self.selected at
application time, but the action was queued for a different chunk if the
selection changed between when the action was created and when it's applied.
Modify the CurationAction structure to include the chunk ID that the action is
bound to, then update apply_curation to use the chunk ID from the action
parameter instead of reading self.selected. Update all locations where
CurationAction instances are created (around lines 732-753 and 685-686) to
capture and store the current self.selected value in the action so that each
pending curation action is permanently bound to the chunk that produced it.
- Around line 439-447: The chunk_filename function creates
non-collision-resistant filenames because different IDs can map to the same slug
(for example, "riff/a", "riff a", and "riff_a" all become "riff_a.chunk.json"),
leading to potential data loss through overwrites. To fix this, modify the
chunk_filename function to include a hash or digest of the original ID in the
filename alongside the slug to ensure uniqueness. This will guarantee that
different IDs always produce different filenames. Apply the same
collision-resistant approach to the other filename generation locations
mentioned at lines 596-597 and 671-672.
- Around line 461-465: The issue is that in-memory edits to self.corpus are
applied before the persist_chunk function is called, but if persistence fails
the in-memory state is not rolled back, leaving the dock showing an edit that
was never actually saved. To fix this, save the original value from self.corpus
before modifying it in curate, then if persist_chunk fails (returns an error),
restore the original value to self.corpus to maintain consistency between what
is displayed and what is actually persisted. This pattern needs to be applied to
all locations where persist_chunk is called (in the persist_chunk function and
at all its call sites mentioned at lines 648-660 and 669-675).
- Around line 231-235: In the match statement for chunk.reviewer in the mark
assignment, the None case currently returns an empty string which causes
unreviewed chunks to display no prefix. Change the None branch to return "? "
instead of "" to ensure that chunks with no reviewer decision display the
question mark prefix, making them consistent with the visual marking
requirements for all chunks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68efda6e-6c2b-47c3-87ce-8cfe00c9ba30
📒 Files selected for processing (3)
cockpit/README.mdcockpit/src/lib.rsdocs/decisions.log.md
… fail (PR #100 review) CodeRabbit review of the Slice 6 inspector: - bind each pending CurationAction to the chunk id it was rendered for, so a same-frame row click can't redirect the edit to the newly-selected chunk (apply_curation takes an explicit id now, not self.selected). - roll the in-memory edit back when persist_chunk fails (the native fs::write path), so the dock never shows a change that didn't reach disk. - mark undecided chunks (reviewer None) with "?" like an explicit NeedsReview. Skipped the hash-in-filename suggestion: the corpus is the readable, CLI-byte- compatible chunk.json tree (ADR-0027 section 3), so hex-encoding the id would break that convention; ids are unique slugs in the normal flow and the chunk's internal id stays authoritative. Per the maintainer, the optimistic OPFS "saved" (matching capture's fire-and-forget) and the single-commit layout stand (as #99). cockpit 23 tests pass; native + wasm clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
2 participants
Slice 6 — curation actions (ADR-0027)
Select a chunk in the corpus dock to open a curation inspector and act on it — realizing the
preview/designinspector over the OPFS store.What's in it
griff_ui_core::curationJSON→JSON ops (decide_record/rename_record/set_tags) — the cockpit never reimplements the logic (ADR-0016) — and re-persists the chunk to its own OPFSchunk.json(web::persist, no re-download).chunk_filenameis extracted so capture and curation write the same file (an edit overwrites in place).Deferred
source.bar_range, which capture-built chunks leaveNone— they apply to CLI-split corpora, not phone captures. A later slice.decidereachesAccepted/Rejectedonly (the UICurationDecisionhas noNeedsReview).Tests
curatedecide / rename / retag + an unknown-id error + a headless inspector render (23 total).ui-core::curationunit tests + the proven OPFS-persist path, rather than a fragile canvas-coordinate Playwright test.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Release Notes
Documentation
New Features
Bug Fixes
Tests