Work plan: S8 curation slice 2 — record state in the inspector - #42
Conversation
… (S8) The curator must see the record's title, prior reviewer decision, and tags before rename/tag can build on them: summarize_record on the curation seam (schema wire names, UI-level strings only) and a record block in the inspector dock. References items that do not exist yet, so the suite fails to compile until the green step. https://claude.ai/code/session_01NC6kB92Hn6Gcf9cMHjLt4b
summarize_record digests a ChunkMeta into UI-level strings — title, prior reviewer decision, and tags, all in the schema's snake_case wire names via serde so they cannot drift — and the inspector shows the block right under the live curation line. Best-effort at startup: an unreadable --record warns and continues; quit-time persist still fails loudly. The prerequisite for the rename/tag curation slice. https://claude.ai/code/session_01NC6kB92Hn6Gcf9cMHjLt4b
|
@codex review Generated by Claude Code |
![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: 27fc35c073
ℹ️ 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 | ||
| /// [`CurationError::ParseFailed`] when `json` is not a `ChunkMeta` record. | ||
| pub fn summarize_record(json: &str) -> Result<RecordSummary, CurationError> { |
There was a problem hiding this comment.
This commit introduces the new public curation API (summarize_record here, plus RecordSummary) in the same commit as the tests that cover it in preview/tests/curation.rs and preview/src/tui.rs. The repo’s AGENTS.md TDD hard rule says never to commit a new pub fn/pub struct implementation together with its covering tests; for this non-trivial S8 curation slice, please split the red tests and green implementation into separate commits so the failing red phase is reviewable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The split is in place — the flattened PR diff hides it, but the history has the rule's exact shape:
- red
d34f00e"test: red — the inspector surfaces the loaded record's curation state (S8)" touches onlypreview/tests/curation.rs(+44) and thetui.rstest module (+21); it referencessummarize_record/RecordSummary/set_record, which did not exist yet, so the workspace fails to compile at that commit. - green
466565c"feat(preview): …" touches only implementation —curation.rs(+45),main.rs(+15),tui.rsnon-test code (+28) — and adds zero#[test]items.
No code change needed; happy to reorganize if you see a commit where the two actually mix.
Generated by Claude Code
The summarize_record insertion landed above decide_record's existing doc block, merging the two comments onto the new function and leaving decide_record undocumented. Noticed in the PR #42 review hunk. https://claude.ai/code/session_01NC6kB92Hn6Gcf9cMHjLt4b
|
@codex review pls |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fded16ba92
ℹ️ 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".
| } | ||
| ))); | ||
|
|
||
| self.push_record_lines(&mut lines, dim); |
There was a problem hiding this comment.
When --record is used on a short terminal, these record lines are inserted before the transport block, so the initial inspector can hide the live play/pause and position state entirely; I checked griff-preview ... --snapshot=80x12 --record=core/tests/fixtures/minimal_chunk.json, and the frame stops at the tags line with no transport/pos visible. That regresses the liveness ordering documented immediately below (and previously tested for the no-record path), because curation users who attach a record now lose the playback state unless they know to scroll; putting the record digest below transport would preserve the existing guarantee while still showing it in the dock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 8f6e56c (red → green): the digest is static — loaded once at startup — so it now sits below the transport block with the other static content, exactly per the PR #38 liveness ordering (clipping eats static tails, never live state). Regression test pins transport visible on an 80×12 terminal with a record attached (the first version of the test asserted on pos, which the header also shows — sharpened to the dock-only line). On very short terminals the digest itself clips first; the PR #41 scroll reaches it. Decision recorded in docs/decisions.log.md.
Generated by Claude Code
The record block landed above the transport lines, so on a short terminal the static digest pushes the live play state out of the dock, regressing the PR #38 liveness ordering. https://claude.ai/code/session_01NC6kB92Hn6Gcf9cMHjLt4b
…Codex P2, PR #42) The digest is static — loaded once at startup — so it belongs with the static blocks under transport: the PR #38 liveness ordering clips static tails, never the live play state. A blank separator joins the block, matching the other static sections. https://claude.ai/code/session_01NC6kB92Hn6Gcf9cMHjLt4b
|
@codex review Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09e32706c0
ℹ️ 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 | ||
| /// [`CurationError::ParseFailed`] when `json` is not a `ChunkMeta` record. | ||
| pub fn summarize_record(json: &str) -> Result<RecordSummary, CurationError> { |
There was a problem hiding this comment.
This commit adds new public API (RecordSummary/summarize_record) in the same changeset as the tests that cover it, but /workspace/griff/AGENTS.md makes the TDD workflow mandatory and says: "Never commit new pub fn / pub struct implementation in the same commit as the tests that cover it, for any new functionality." Because this is new functionality rather than a characterization test, the change should be split into the required red test commit and the green implementation commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same finding as the thread above (#42 (comment)) — the rule is satisfied in the commit history, which the flattened PR diff can't show: red d34f00e (tests only, fails to compile), green 466565c (implementation only, zero #[test] items). The follow-up Codex P2 fix repeated the same shape: red 3c4d8d5, green 8f6e56c. Every TDD PR in this repo will look "mixed" at the changeset level by construction; the reviewable unit is the commit sequence.
Generated by Claude Code
Automated reviewers reading the flattened PR diff repeatedly flag TDD PRs as mixing tests and implementation (Codex P1 on #42, twice); the diff shows red and green together by construction. State explicitly that the reviewable unit is the commit sequence. https://claude.ai/code/session_01NC6kB92Hn6Gcf9cMHjLt4b
2 participants
One roadmap increment as a red → green → docs TDD cycle (the work-plan continuation after #41).
S8 — curation slice 2: the inspector surfaces the record's current state
curation::summarize_recorddigests aChunkMetainto UI-level strings — title, prior reviewer decision, tags — in the schema'ssnake_casewire names, derived via serde so the names cannot drift (ADR-0016: no domain types in the UI).--recordwarns and continues, quit-time persist still fails loudly.Also adds
docs/process-backlog.mdwith the requested follow-up (a sweep script re-requesting Codex review on PRs it never reviewed). Decision recorded indocs/decisions.log.md; S8 stage doc updated.https://claude.ai/code/session_01NC6kB92Hn6Gcf9cMHjLt4b
Generated by Claude Code