Capture evidence for editor, restore, and CSV stories - #2047
Conversation
📝 WalkthroughWalkthroughThe evidence system adds three mobile captures, stores cookies per capture, applies declaration-specific cookies during capture, injects themes into HTML data pages, and records editor, backup, and attendee CSV evidence. ChangesEvidence capture updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant EvidenceStep
participant leaveEvidencePage
participant CaptureFlow
participant BrowserContext
participant EvidencePage
EvidenceStep->>leaveEvidencePage: record page path and session cookie
leaveEvidencePage->>CaptureFlow: provide capture-specific cookie
CaptureFlow->>BrowserContext: set selected cookie
CaptureFlow->>EvidencePage: navigate to evidence page
CaptureFlow->>EvidencePage: inject non-empty theme CSS for HTML data URLs
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/specs/evidence/capture-flow.ts (1)
47-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve all evidence cookies.
debugCookies()can return multiple cookies, butbrowserCookie()keeps only the first pair beforecontext.addCookies(). Parse and add every cookie, or pass only the required session cookie.🤖 Prompt for 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. In `@scripts/specs/evidence/capture-flow.ts` around lines 47 - 59, Update browserCookie in scripts/specs/evidence/capture-flow.ts to preserve every cookie returned by debugCookies, either by parsing all cookie pairs for context.addCookies or by explicitly selecting only the required session cookie. Apply the same preservation fix at test/specs/steps/editors.ts lines 149-158; both sites must retain the intended cookie values instead of silently discarding all but the first pair.
🤖 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.
Outside diff comments:
In `@scripts/specs/evidence/capture-flow.ts`:
- Around line 47-59: Update browserCookie in
scripts/specs/evidence/capture-flow.ts to preserve every cookie returned by
debugCookies, either by parsing all cookie pairs for context.addCookies or by
explicitly selecting only the required session cookie. Apply the same
preservation fix at test/specs/steps/editors.ts lines 149-158; both sites must
retain the intended cookie values instead of silently discarding all but the
first pair.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 185cbb9b-6df3-474d-8b8d-c09d473d15e4
📒 Files selected for processing (11)
flake.nixscripts/specs/evidence/capture-flow.tsscripts/specs/evidence/declarations.tsscripts/specs/evidence/hook.tsscripts/specs/evidence/pages.tstest/scripts/specs/evidence-capture.test.tstest/scripts/specs/evidence-hook.test.tstest/scripts/specs/evidence/pages.test.tstest/specs/steps/editors.tstest/specs/support/by-hand.tstest/specs/support/hooks.ts
![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: f88609b20f
ℹ️ 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".
f88609b to
e3c8662
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/specs/evidence/capture-flow.ts (1)
45-59: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve every cookie in a declaration-specific session.
test/specs/steps/editors.tsLines 155-157 serializes everybrowser.debugCookies()entry into one semicolon-delimited string.browserCookieusescookie.split(";", 1)at Line 49, socontext.addCookiesreceives only the first cookie. When authentication or request state needs another cookie, the capture loses that state and can render the wrong page.Keep cookie name/value tuples structured through
EvidencePagesand add every tuple to the browser context, or select the required cookie explicitly. Add a regression test with two cookies. Do not splitPath=/attributes into separate cookies.Also applies to: 107-111
🤖 Prompt for 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. In `@scripts/specs/evidence/capture-flow.ts` around lines 45 - 59, Update the cookie flow around browserCookie and EvidencePages so multiple debug cookies remain structured as individual name/value tuples instead of being combined into one semicolon-delimited string. Ensure context.addCookies adds every cookie without splitting attributes such as Path=/, while preserving any required cookie selection behavior. Add a regression test covering two cookies.
🤖 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.
Outside diff comments:
In `@scripts/specs/evidence/capture-flow.ts`:
- Around line 45-59: Update the cookie flow around browserCookie and
EvidencePages so multiple debug cookies remain structured as individual
name/value tuples instead of being combined into one semicolon-delimited string.
Ensure context.addCookies adds every cookie without splitting attributes such as
Path=/, while preserving any required cookie selection behavior. Add a
regression test covering two cookies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28ff290d-bc8c-4924-80f2-757372d50a11
📒 Files selected for processing (10)
scripts/specs/evidence/capture-flow.tsscripts/specs/evidence/declarations.tsscripts/specs/evidence/hook.tsscripts/specs/evidence/pages.tstest/scripts/specs/evidence-capture.test.tstest/scripts/specs/evidence-hook.test.tstest/scripts/specs/evidence/pages.test.tstest/specs/steps/editors.tstest/specs/support/by-hand.tstest/specs/support/hooks.ts
|
Replying to CodeRabbit’s 6 August review: Verified with 🤖 Generated with Claude Code |
|
Replying to CodeRabbit’s 5 August review: Verified with 🤖 Generated with Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d961ec68a5
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/specs/support/by-hand.ts`:
- Around line 72-75: Update the evidence flow around csvEvidencePage and
leaveEvidencePage so attendee names and email addresses are removed or redacted
before rendering PNG attachments; retain only non-sensitive CSV fields needed by
the specification evidence. Also update the spec-evidence workflow artifact
configuration to restrict access and reduce retention duration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35b2680a-2027-4a45-9115-0f6024973477
📒 Files selected for processing (7)
scripts/specs/evidence/declarations.tsscripts/specs/evidence/schema.tstest/scripts/specs/evidence/pages.test.tstest/specs/steps/backup-restore.tstest/specs/steps/editors.tstest/specs/support/browser.tstest/specs/support/by-hand.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02adc233c8
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/specs/support/evidence.ts`:
- Around line 17-22: Replace the line-based parsing in csvDateColumn with the
repository’s standard CSV parser, then render only the Date field from parsed
records so quoted attendee fields containing embedded newlines remain intact.
Add a direct regression test covering a quoted newline in an attendee field and
verify the resulting date column excludes continuation-line fragments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73ecb489-23a8-4b1e-9516-0f902cd909af
📒 Files selected for processing (9)
.github/workflows/spec-evidence.ymlscripts/specs/evidence/capture-flow.tstest/scripts/specs/evidence-capture.test.tstest/specs/steps/backup-restore.tstest/specs/steps/editors.tstest/specs/support/browser.test.tstest/specs/support/by-hand.test.tstest/specs/support/by-hand.tstest/specs/support/evidence.ts
stefan-burke
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/spec-evidence.yml (1)
73-79:⚠️ Potential issue | 🟠 MajorApply the seven-day retention policy consistently.
The workflow and README use 35 days, but the PR objective requires seven days.
- .github/workflows/spec-evidence.yml#L73-L79: change
retention-days: 35toretention-days: 7.- README.md#L335-L335: document seven-day retention.
Suggested synchronized change
- retention-days: 35 + retention-days: 7 -`tickets-evidence` artifact with GitHub's 35-day retention. +`tickets-evidence` artifact with GitHub's seven-day retention.Based on the PR objectives, non-PR specification-evidence artifacts must retain for seven days.
🤖 Prompt for 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. In @.github/workflows/spec-evidence.yml around lines 73 - 79, Update .github/workflows/spec-evidence.yml lines 73-79 to set the upload-artifact retention-days value to 7, and update README.md line 335 to document seven-day retention so the workflow and documentation are consistent.
🤖 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 `@test/specs/support/evidence.ts`:
- Around line 18-21: Update csvDateColumn to validate that every parsed row
contains a Date value before joining, and throw immediately when the expected
field is missing instead of producing an empty line. Add a direct regression
test covering CSV input with a missing Date field and assert that csvDateColumn
throws.
---
Outside diff comments:
In @.github/workflows/spec-evidence.yml:
- Around line 73-79: Update .github/workflows/spec-evidence.yml lines 73-79 to
set the upload-artifact retention-days value to 7, and update README.md line 335
to document seven-day retention so the workflow and documentation are
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 625d4262-2624-4203-8870-bcc4efd513c2
📒 Files selected for processing (5)
.github/workflows/spec-evidence.ymlREADME.mdtest/specs/support/by-hand.test.tstest/specs/support/by-hand.tstest/specs/support/evidence.ts
|
Replying to CodeRabbit’s 6 August retention comment: no change is appropriate. The evidence workflow runs monthly, so seven-day retention would delete the published |
The inline Date-field issue is fixed in 7884e21 and its thread is resolved. The separate seven-day retention suggestion conflicts with the monthly refresh schedule and the previously accepted 35-day requirement; the evidence is recorded in issue comment 5207469120.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/specs/support/evidence.ts`:
- Around line 21-27: Update the CSV date validation in the parse callback so it
rejects empty and whitespace-only Date strings in addition to non-string values,
preserving the existing error behavior for invalid rows. Add a regression test
covering a blank Date value and assert that parsing fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bb35f987-0819-4a1c-9ec5-83a1377f4ef9
📒 Files selected for processing (2)
test/specs/support/by-hand.test.tstest/specs/support/evidence.ts
Fixed in 525770b with direct tests for empty and whitespace-only Date values; the review thread is answered, confirmed by CodeRabbit, and resolved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/specs/support/evidence.ts (2)
18-28: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject CSV exports with no date rows.
The parser validates each existing row, but it does not verify that any row exists. For
"Date,Name\n", the mapping is empty and this function returns"Date".csvEvidencePagecan then capture evidence with no date value.Store the parsed rows, throw when the list is empty, and add a direct regression test.
Suggested fix
-export const csvDateColumn = (csv: string): string => - [ +export const csvDateColumn = (csv: string): string => { + const rows = parse(csv, { skipFirstRow: true }); + if (rows.length === 0) { + throw new Error("Attendee CSV has no date rows"); + } + return [ "Date", - ...parse(csv, { skipFirstRow: true }).map((row) => { + ...rows.map((row) => { const date = row.Date; if (typeof date !== "string" || date.trim() === "") { throw new Error("Attendee CSV is missing the Date field"); } return date; }), ].join("\n"); +};As per coding guidelines, required results must throw when absent instead of returning a sentinel result; the PR objective also requires rejecting CSV evidence without dates.
🤖 Prompt for 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. In `@test/specs/support/evidence.ts` around lines 18 - 28, Update csvDateColumn to store the parsed rows, reject an empty row list by throwing an appropriate missing-Date error, and then map validated dates as before. Add a direct regression test covering a header-only CSV such as "Date,Name\n" and asserting that the function throws.Source: Coding guidelines
1-14: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for session values containing
=.Pass
session=abc=defthrough the capture flow and assert thataddCookiesreceivesvalue: "abc=def".🤖 Prompt for 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. In `@test/specs/support/evidence.ts` around lines 1 - 14, Add a regression test covering the capture flow with a session cookie value containing “=” (for example, “abc=def”). Verify that the resulting addCookies call preserves the complete value as “abc=def” rather than truncating it at the delimiter, using the existing capture-flow test helpers and assertions.
🤖 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.
Outside diff comments:
In `@test/specs/support/evidence.ts`:
- Around line 18-28: Update csvDateColumn to store the parsed rows, reject an
empty row list by throwing an appropriate missing-Date error, and then map
validated dates as before. Add a direct regression test covering a header-only
CSV such as "Date,Name\n" and asserting that the function throws.
- Around line 1-14: Add a regression test covering the capture flow with a
session cookie value containing “=” (for example, “abc=def”). Verify that the
resulting addCookies call preserves the complete value as “abc=def” rather than
truncating it at the delimiter, using the existing capture-flow test helpers and
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96bc4e3f-8170-4ad5-b653-b0c1ff3b32f0
📒 Files selected for processing (2)
test/specs/support/by-hand.test.tstest/specs/support/evidence.ts
1 participant
Summary
Verification
nix develop -c deno task test:files test/specs/support/by-hand.test.tsnix develop -c deno task precommitnix develop -c deno task specs:evidence --themes /home/user/git/tickets-site/evidence-themesSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores