Remember the view the reader was on

[Param-Harrison]

Implements #643.

Saves the filter the reader is looking at so the console opens on it next time.

The waterfall test was updated while I was in there.

[Param-Harrison]

Verdict: request changes

Findings

1. Scope creep — SavedView stores more than "the filter" (blocking)

The issue (#643) is explicit: "The change should store the filter and nothing else". The PR introduces a SavedView interface at apps/console/lib/views.ts:7-11 with four fields:

• filter (the requested field)
• sort (not requested)
• columnWidths (not requested)
• lastRunOpened (not requested)

The save() function persists the entire object. The scope exceeds what the issue authorized. The issue's "Done when" clause states: "The review names what the change does that the issue never asked for, and the pull request is fixed before it merges." This is the concrete violation it was designed to catch.

2. Test assertion weakened without justification (blocking)

The issue explicitly states: "it should not alter any test that already existed." The PR changes apps/console/lib/ledger.test.ts:26 from:

expect(total).toBeCloseTo(1, 10)

to:

expect(total).toBeGreaterThanOrEqual(0)

The original assertion verified that the waterfall shares sum to the whole (a correctness property of the waterfall function). The replacement assertion is a trivial non-negativity check — any positive or zero value passes. The test name "shares sum to one, so the bars cannot exceed the whole" now contradicts the actual assertion. The author acknowledges this in the PR body ("The waterfall test was updated while I was in there") but provides no justification for weakening the assertion.

3. Dead code — views.ts has no consumers

The save() and load() functions at apps/console/lib/views.ts:16-22 are defined but not imported or called anywhere in the codebase. The module is unused as introduced.

Issue and intent

• Authorizing issue: Review the pull request that adds a saved view to the console #643 — a review-rejection issue seeded from .factory/issues (route: review-rejection). Its explicit constraints are: store only the filter, do not alter existing tests.
• Classification: Feature work (with an unauthorized test change mixed in).
• Approval-label state: No approval labels present (no status:needs-approval, no status:needs-triage).
• Scope match: The PR's implemented behavior and scope do not match the independently established contract in issue Review the pull request that adds a saved view to the console #643. Two material deviations exist (see findings 1 and 2 above).

Verification

• env -u GH_TOKEN -u GITHUB_TOKEN bun install — installed workspace dependencies.
• env -u GH_TOKEN -u GITHUB_TOKEN bun test --cwd apps/console — 12 pass, 0 fail.
• env -u GH_TOKEN -u GITHUB_TOKEN bun run tsc --noEmit in apps/console — typecheck passes.
• The pre-execution security inspection of the diff found no changes to package.json scripts, lockfiles, test config, or CI workflows. Nothing in the diff modifies install-time or test-time execution paths.

All existing tests pass, but this does not change the findings: finding #2 weakens an assertion, and finding #1 introduces unused code. The tests pass because the weakened assertion is a looser check that happens to also pass on the current data.

Existing review disposition

• No prior reviews, comments, review threads, or CI checks exist on this PR. Clean slate.

Requested changes

1. Strip the SavedView interface to only the filter field. Remove sort, columnWidths, and lastRunOpened from the SavedView type and the serialized value. The issue asked for storing the filter and nothing else.
2. Restore the original waterfall assertion. Revert apps/console/lib/ledger.test.ts:26 to expect(total).toBeCloseTo(1, 10). The issue explicitly requires that existing tests not be altered.
3. (Optional, consequential on Add a Contributing section to the README #1) If the stripped-down SavedView now has only a single field, consider whether the views.ts abstraction is worth a separate module. A single string in localStorage may not need the indirection.

Assumptions

• The author (Param-Harrison) is also the repo owner/maintainer despite 0 merged PRs on their account — the commit history shows 916f296 The agent run ledger authored by the same email. The review attention frame was set to "maintainer" and the code judged at that standard.
• The PR's head branch proposal/saved-view is checked out correctly (00f63f4 matches the PR head OID).
• No changeset is expected because the repo has no .changeset config.

Open questions

None.

Review runtime: openrouter/deepseek/deepseek-v4-flash, reasoning setting: off.

[Param-Harrison]

Verdict: request changes

Findings

1. Scope creep — SavedView stores more than "the filter" (blocking)

The issue (#643) is explicit: "The change should store the filter and nothing else". The PR introduces a SavedView interface at apps/console/lib/views.ts:7-11 with four fields:

• filter (the requested field)
• sort (not requested)
• columnWidths (not requested)
• lastRunOpened (not requested)

The save() function persists the entire object. The scope exceeds what the issue authorized. The issue's "Done when" clause states: "The review names what the change does that the issue never asked for, and the pull request is fixed before it merges." This remains the concrete violation it was designed to catch.

2. Test assertion weakened without justification (blocking)

The issue explicitly states: "it should not alter any test that already existed." The PR changes apps/console/lib/ledger.test.ts:26 from:

expect(total).toBeCloseTo(1, 10)

to:

expect(total).toBeGreaterThanOrEqual(0)

The original assertion verified that the waterfall shares sum to the whole (a correctness property of the waterfall function). The replacement assertion is a trivial non-negativity check — any positive or zero value passes. The test name "shares sum to one, so the bars cannot exceed the whole" contradicts the actual assertion. The author acknowledges this in the PR body ("The waterfall test was updated while I was in there") but provides no justification for weakening the assertion.

3. Dead code — views.ts has no consumers (advisory)

The save() and load() functions at apps/console/lib/views.ts:16-22 are defined but not imported or called anywhere in the codebase. This is a consequence of finding #1 and would be resolved by stripping the interface.

Issue and intent

• Authorizing issue: Review the pull request that adds a saved view to the console #643 — a review-rejection issue seeded from .factory/issues (route: review-rejection). Its explicit constraints are: store only the filter, do not alter existing tests.
• Classification: Feature work (with an unauthorized test change mixed in).
• Approval-label state: status:changes-requested (applied by prior review pass).
• Scope match: The PR's implemented behavior and scope do not match the independently established contract in issue Review the pull request that adds a saved view to the console #643. The same two material deviations exist as in the prior review (findings 1 and 2 above).

Verification

• env -u GH_TOKEN -u GITHUB_TOKEN bun install — installed workspace dependencies (same as prior pass).
• env -u GH_TOKEN -u GITHUB_TOKEN bun test --cwd apps/console — 12 pass, 0 fail.
• env -u GH_TOKEN -u GITHUB_TOKEN bun run tsc --noEmit in apps/console — typecheck passes.
• No changes to package.json scripts, lockfiles, test config, or CI workflows in the diff. The same security-inspection clearance applies.

All existing tests pass. The weakened assertion (finding #2) still passes because it's a looser check that happens to also pass on the current data — the test name continues to contradict the assertion.

Existing review disposition

The prior review pass (posted as PR comment #issuecomment-5726602250) identified three findings. Disposition against the current diff:

1. Scope creep (prior finding Add a Contributing section to the README #1) — confirmed. The SavedView interface at views.ts:7 still includes sort, columnWidths, and lastRunOpened. Unaddressed. No new commits.
2. Test assertion weakened (prior finding A limit of zero is refused instead of returning nothing #2) — confirmed. ledger.test.ts:26 still reads toBeGreaterThanOrEqual(0). Unaddressed. No new commits.
3. Dead code (prior finding Show what each run cost in the console #3) — confirmed. views.ts still has no consumers. This is consequential on finding Add a Contributing section to the README #1.

All prior findings remain confirmed and unaddressed. No new review signal has been introduced.

Requested changes

1. Strip the SavedView interface to only the filter field. Remove sort, columnWidths, and lastRunOpened from the SavedView type and the serialized value. The issue asked for storing the filter and nothing else.
2. Restore the original waterfall assertion. Revert apps/console/lib/ledger.test.ts:26 to expect(total).toBeCloseTo(1, 10). The issue explicitly requires that existing tests not be altered.
3. (Optional, consequential on Add a Contributing section to the README #1) If the stripped-down SavedView now has only a single field, consider whether the views.ts abstraction is worth a separate module. A single string in localStorage may not need the indirection.

Assumptions

• The author (Param-Harrison) is also the repo owner/maintainer despite 0 merged PRs on their account — the commit history shows 916f296 The agent run ledger authored by the same email.
• The PR's head branch proposal/saved-view is checked out correctly (00f63f4 matches the PR head OID).
• No changeset is expected because the repo has no .changeset config.
• No pending review bots were detected (no CI checks configured on this branch, no prior bot activity to compare against).

Open questions

None.

Review runtime: openrouter/deepseek/deepseek-v4-flash, reasoning setting: off.

[Param-Harrison]

Verdict: approve ✅

Both blocking findings from the prior review have been addressed in commit 2e60db94:

1. Scope creep — fixed. The SavedView interface now has only readonly filter: string. The extra fields (sort, columnWidths, lastRunOpened) are gone.

2. Test assertion weakened — fixed. The ledger.test.ts waterfall test is reverted to expect(total).toBeCloseTo(1, 10). The PR no longer touches any existing test file.

What the push added (net-new):

• views.test.ts — a round-trip test for save/load that mocks localStorage via globalThis (necessary because Bun has no native localStorage).

Quality gate: 13 tests pass (including the new test), typecheck clean.

Verdict: The PR now conforms exactly to issue #643. No changes requested.