Re-clarification surfaces never offer the planned queue, so a queued Action can only be promoted from task detail (#723) - #728
Conversation
WalkthroughThe change adds typed planned-Action choices to Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@app/lib/screens/task_detail/task_detail_screen.dart`:
- Around line 991-1001: In the flow around showReplaceCurrentActionSheet,
capture the current _notifier before awaiting the sheet, then use that captured
notifier for supersedeAndPromote after confirmation. Preserve the existing
inline .ignore() write behavior.
In `@app/lib/services/clarification_service.dart`:
- Around line 194-201: Rename the Outcome identifier parameter from id to
outcomeId in both the service interface declarations for getPlannedActions and
getCurrentAction and their corresponding implementations, updating all internal
references and call sites as needed without changing behavior.
In `@app/lib/widgets/clarify_card.dart`:
- Around line 997-1016: Update the Outcome dialog construction to load the
Outcome’s current Action text and pass it as currentActionText when creating
OutcomeSubject, rather than using the Outcome title. Preserve the resulting
prefilled “Update next action” and editingExistingAction behavior in the shared
handler.
In `@app/lib/widgets/process_to_handlers.dart`:
- Around line 930-975: Replace the separate transition and routing steps in
_promoteFromQueue with one Clarification service operation enclosed by a single
capturing(...) scope that verifies the planned Action, optionally validates the
confirmed current Action, performs promotion or supersession, and routes the
Outcome to Next atomically. Return an explicit success status; invoke _commit
notification logic only after success, and do nothing when the planned Action is
missing or the transaction fails. Rename chosen to selectedPlannedAction in the
revised flow.
In `@app/lib/widgets/reclarify_prompt_sheet.dart`:
- Around line 16-20: Update the documentation comment describing
NextActionDialog near ProcessToHandlers and ClarificationService so that, when a
planned queue exists, it explicitly says the user can promote a queued Action or
enter a new Action phrase; retain the empty-field behavior for cases without a
queue.
In `@app/test/test_helpers.dart`:
- Around line 53-75: Update seedPlannedAction to use descriptive names for the
fixture inputs and timestamp: rename text to actionText, timeEstimate to
timeEstimateMinutes, and ts to timestamp, then update their references in the
ActionsCompanion construction while preserving behavior.
In `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1688-1711: Update
app/test/widgets/process_to_handlers_test.dart:1688-1711 so the stale-row test
seeds intent "maybe", deletes p1 from db.actions after opening the dialog, uses
the real promotePlannedAction DAO path, and asserts intent remains "maybe". At
app/test/widgets/process_to_handlers_test.dart:1748-1777, seed intent "maybe"
and assert dismissal preserves both intent and lastClarifiedAt. At
app/test/widgets/process_to_handlers_test.dart:1826-1832, remove
_NoOpPromoteClarificationService because the stale-row test must exercise real
persistence.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8401eca-47fc-43bc-86cf-d9a7fb2de796
📒 Files selected for processing (19)
app/lib/screens/periodic_review/steps/list_review_step.dartapp/lib/screens/planning/steps/task_review_step.dartapp/lib/screens/task_detail/task_detail_screen.dartapp/lib/services/clarification_service.dartapp/lib/widgets/clarify_card.dartapp/lib/widgets/next_action_dialog.dartapp/lib/widgets/process_to_handlers.dartapp/lib/widgets/reclarify_prompt_sheet.dartapp/lib/widgets/replace_current_action_sheet.dartapp/test/helpers/active_focus_harness.dartapp/test/integration/reclarify_promote_planned_journey_test.dartapp/test/screens/active_focus_screen_test.dartapp/test/screens/periodic_review/steps/someday_maybe_step_test.dartapp/test/screens/planning/steps/task_review_step_blank_save_test.dartapp/test/test_helpers.dartapp/test/widgets/clarify_card_test.dartapp/test/widgets/process_to_handlers_test.dartapp/test/widgets/replace_current_action_sheet_test.dartdocs/REQUIREMENTS.md
…firm (#723) The extracted showReplaceCurrentActionSheet introduced an await between reading _notifier (an autoDispose provider) and using it. If the detail State unmounts while the sheet is open, that read can outlive the ref. Capture it up front, matching _openActionSheet. Addresses #728 (comment) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The 'More to do…' comment said the dialog is an empty field only when there is no queue. With a queue it shows the planned rows *and* the text field together, so state both choices for the queued case. Addresses #728 (comment) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace _NoOpPromoteClarificationService with a raw row delete (as the sync bridge lands one), so the vanished-planned-row case drives the real promotePlannedAction no-op path rather than a stubbed service. Seed the two abort cases on Someday (intent='maybe') so a would-be route to Next is detectable, and assert intent (and last_clarified_at) are preserved. Addresses #728 (comment) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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)
app/lib/screens/task_detail/task_detail_screen.dart (1)
991-1005: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRevalidate the current Action before replacement.
The confirmation displays
current.actionText, butsupersedeAndPromote(planned.id)resolves the Outcome’s current Action when the write commits. If synchronization changes the current Action while the sheet is open, the user can confirm the old text while the newer Action is superseded without confirmation.Re-read the current Action after confirmation, or make the promotion transaction require
current.idas an expected value and abort or re-prompt when it changed. Add an integration test for this race.As per coding guidelines, follow top-down TDD and test this real behavior with integration coverage.
🤖 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 `@app/lib/screens/task_detail/task_detail_screen.dart` around lines 991 - 1005, Revalidate the current Action after showReplaceCurrentActionSheet returns and before supersedeAndPromote, ensuring promotion only proceeds if it still matches the Action shown in the confirmation; otherwise abort or re-prompt. Update the relevant notifier/transaction flow around supersedeAndPromote and add integration coverage for synchronization changing the current Action while the sheet is open, following top-down TDD.Source: Coding guidelines
🤖 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 `@app/lib/screens/task_detail/task_detail_screen.dart`:
- Around line 991-1005: Revalidate the current Action after
showReplaceCurrentActionSheet returns and before supersedeAndPromote, ensuring
promotion only proceeds if it still matches the Action shown in the
confirmation; otherwise abort or re-prompt. Update the relevant
notifier/transaction flow around supersedeAndPromote and add integration
coverage for synchronization changing the current Action while the sheet is
open, following top-down TDD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6944ea0-a4cf-43d2-8201-08f07ae3e0d6
📒 Files selected for processing (2)
app/lib/screens/task_detail/task_detail_screen.dartapp/lib/widgets/reclarify_prompt_sheet.dart
|
Review comments addressed. All seven review comments on PR #728 have been triaged. Three actionable ones were fixed, committed, pushed (up to |
trixy-the-ai-bot
added
in-progress
|
No changes needed. |
|
Merge gate: NOT-READY — two things, one of them easy to miss. 1. An outside-diff finding is still unaddressedAll seven inline threads are resolved — good — but CodeRabbit's latest review body carries an outside-diff comment that never became a thread, so it does not show up in the unresolved count:
This one is worth taking seriously rather than rebutting on frequency grounds. It is the same class of hazard as the no-op promote guard already in this PR — a read that goes stale between the decision and the write — and it lands on the destructive side: the incumbent Action is retired. My plan review asked for a fresh Please triage it as you would any finding: fix, or rebut with reasoning. If you fix it, the shape CodeRabbit suggests — pass Coverage per the repo's TDD order: this is a race, so drive it at the integration tier rather than asserting it at the DAO. 2. The review does not reach headCodeRabbit's last pass reviewed After you push the fix (or the rebuttal), I will claim a CodeRabbit slot and confirm the new pass covers the real head before merging. Not blockingCI is green on all checks and the branch is |
…confirm (#723) The 'Replace current action' confirm shows the current Action's text read before the write, but supersedeAndPromote resolves the current at write time. If sync swapped the current while the sheet was open, the user could confirm replacing the old text while a newer Action was silently retired. Add an optional expectedCurrentActionId to supersedeAndPromote / applySupersedeAndPromote: the transaction throws (retiring nothing) when the resolved current no longer matches, mirroring applyPromotePlannedAction asserting its own precondition. Thread it through both confirm callsites — the re-clarify _promoteFromQueue (throw -> write-failed snackbar, no route) and the task-detail _confirmReplace (throw -> silent abort; the reactive Plan section then shows the true state). Integration-tier test drives the swap-under-confirm. Addresses #728 (comment) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Re: #728 (comment) 1. Revalidate-before-replace — fixed in
Threaded through both confirm callsites (the primitive is shared, and I was already refactoring the second one in this PR):
Coverage is at the integration tier as you asked — a widget test over a real DB that opens the confirm, swaps the current Action underneath (raw writes, as the sync bridge lands them), confirms, and asserts the synced-in Action survives un-retired, nothing is promoted, and no route commits. It fails without the guard (the newer Action would be silently superseded). Existing 2. Review reaching head — head is now Noted on sequencing (#727 first, then the likely |
|
Review comments addressed. Fixed a TOCTOU race in the "Replace current action" flow (commit |
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)
app/test/widgets/process_to_handlers_test.dart (1)
1653-1661: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that an empty queue hides the
PLANNEDsection header.The test checks only one possible row and its text. A dialog that renders an empty
PLANNEDsection still passes, but it changes the required text-only behavior.Proposed test update
await openDialog(tester); + expect(find.text('PLANNED'), findsNothing); expect(find.byKey(const Key('next_action_promote_p1')), findsNothing); expect(find.text('Step one'), findsNothing);🤖 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 `@app/test/widgets/process_to_handlers_test.dart` around lines 1653 - 1661, Update the test named “an empty queue renders no promote rows (byte-identical dialog)” to also assert that the PLANNED section header is absent when the queue is empty. Use the existing dialog text or key locator for that header alongside the current row assertions, preserving the test’s byte-identical empty-dialog expectation.
🤖 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 `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1790-1838: Update the concurrent-current test around
seedCurrentAction and the post-confirmation assertions to seed the Outcome with
intent “maybe”. After the guard aborts, fetch the Outcome and assert its intent
remains “maybe” and lastClarifiedAt remains null, alongside the existing
no-route assertions.
---
Outside diff comments:
In `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1653-1661: Update the test named “an empty queue renders no
promote rows (byte-identical dialog)” to also assert that the PLANNED section
header is absent when the queue is empty. Use the existing dialog text or key
locator for that header alongside the current row assertions, preserving the
test’s byte-identical empty-dialog expectation.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5fc0198a-6496-4d90-ada8-1280894ab87b
📒 Files selected for processing (6)
app/lib/database/daos/action_dao.dartapp/lib/providers/task_detail_provider.dartapp/lib/screens/task_detail/task_detail_screen.dartapp/lib/services/clarification_service.dartapp/lib/widgets/process_to_handlers.dartapp/test/widgets/process_to_handlers_test.dart
|
Merge gate: NOT-READY — three things, all on this round. Good work on 1. The guard's own test cannot fail — fix this one properlyUnresolved thread at
This is a non-discriminating test, and it is guarding the exact behaviour I required. As written it would stay green if the abort silently stopped working — which makes it worse than no test, because it reads as coverage. Seed 2. Outside-diff, Minor — the empty-queue test
Flagging where this lives so it does not get missed like the last one: it is in the review body, not a thread, so it will not appear in your unresolved count. 3. The branch now conflicts with
|
Brings in main (incl. #727's _promoteRow D3/D4 fix, 10da418) and resolves the lone conflict in action_dao.dart's applySupersedeAndPromote: keep this branch's expectedCurrentActionId close-the-window guard (#723) and take #727's seedFromDraft regime, both reading the single resolved `current`. No duplicate declaration; the two features compose.
|
Merge conflicts resolved. Resolved the sole merge conflict in |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
trixy-the-ai-bot
added
in-progress
|
No changes needed. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
trixy-the-ai-bot
added
in-progress
|
No changes needed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/test/widgets/process_to_handlers_test.dart (1)
1790-1839: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore assertions that prove the expected-current-action guard aborts.
Line 1793 seeds the Outcome with the default
nextintent. A faulty abort path that commits_commit(nextAction)still passes. The test also does not verify thatlastClarifiedAtstays unchanged.Seed
intent: 'maybe'. Then assert that the Outcome remains on Someday and remains unstamped after confirmation.Proposed test update
- final todo = await _insertTodo(db, id: 'q6'); + final todo = await _insertTodo(db, id: 'q6', intent: 'maybe'); ... expect(await db.actionDao.getTerminatedActions('q6'), isEmpty, reason: 'the aborted replace retires nothing'); + final outcome = await db.todoDao.getTodo('q6'); + expect(outcome?.intent, 'maybe'); + expect(outcome?.lastClarifiedAt, isNull); expect(fired, isEmpty, reason: 'the aborted replace commits no route');🤖 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 `@app/test/widgets/process_to_handlers_test.dart` around lines 1790 - 1839, Update the test around the expected-current-action guard to seed the Outcome with intent “maybe” rather than the default next intent, then assert after confirmation that the Outcome remains on Someday and its lastClarifiedAt value is unchanged. Keep the existing assertions proving the synced current action, planned rows, terminations, and routing are unaffected.
🤖 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 `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1653-1661: Update the test “an empty queue renders no promote rows
(byte-identical dialog)” to also assert that find.text('PLANNED') finds nothing,
ensuring an empty queue omits the PLANNED header as well as promotion rows.
In `@docs/REQUIREMENTS.md`:
- Line 27: The requirements currently expose implementation identifiers instead
of only describing product behavior. In docs/REQUIREMENTS.md line 27, replace
the /task/:id route and last_clarified_at field references with user-visible
task-detail and clarification behavior, linking the relevant ADR for
implementation details; in docs/REQUIREMENTS.md line 51, replace
focus_sessions.current_task_id, SharedPreferences, and SprintTimerProvider
references with the required durable focus and timer behavior, also linking the
relevant ADR. No other requirement behavior should change.
---
Duplicate comments:
In `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1790-1839: Update the test around the expected-current-action
guard to seed the Outcome with intent “maybe” rather than the default next
intent, then assert after confirmation that the Outcome remains on Someday and
its lastClarifiedAt value is unchanged. Keep the existing assertions proving the
synced current action, planned rows, terminations, and routing are unaffected.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: edebbbd3-62bc-4f89-9162-9a7dc03db43d
📒 Files selected for processing (21)
app/lib/database/daos/action_dao.dartapp/lib/providers/task_detail_provider.dartapp/lib/screens/periodic_review/steps/list_review_step.dartapp/lib/screens/planning/steps/task_review_step.dartapp/lib/screens/task_detail/task_detail_screen.dartapp/lib/services/clarification_service.dartapp/lib/widgets/clarify_card.dartapp/lib/widgets/next_action_dialog.dartapp/lib/widgets/process_to_handlers.dartapp/lib/widgets/reclarify_prompt_sheet.dartapp/lib/widgets/replace_current_action_sheet.dartapp/test/helpers/active_focus_harness.dartapp/test/integration/reclarify_promote_planned_journey_test.dartapp/test/screens/active_focus_screen_test.dartapp/test/screens/periodic_review/steps/someday_maybe_step_test.dartapp/test/screens/planning/steps/task_review_step_blank_save_test.dartapp/test/test_helpers.dartapp/test/widgets/clarify_card_test.dartapp/test/widgets/process_to_handlers_test.dartapp/test/widgets/replace_current_action_sheet_test.dartdocs/REQUIREMENTS.md
The empty-queue dialog test checked only that promote rows were absent, so it would still pass if an empty PLANNED section rendered. Assert the header is gone too. Addresses #728 (comment) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review comments addressed. Both CodeRabbit findings on the #723 PR are handled and the branch is at rest:
Nothing is pending — clean tree, all 9 commits for #723 pushed (HEAD |
trixy-the-ai-bot
added
in-progress
|
No changes needed. |
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)
app/test/widgets/process_to_handlers_test.dart (2)
1907-1929: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the Capture queue test non-vacuous.
The test does not seed a planned Action and only asserts that no row is shown. It can pass even if Capture incorrectly calls
getPlannedActionsand receives an empty list.Inject a recording or failing
ClarificationServiceand assert that the queue is not requested. If a valid fixture exists for this path, also seed a planned row and assert that both the row andPLANNEDheader remain absent.🤖 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 `@app/test/widgets/process_to_handlers_test.dart` around lines 1907 - 1929, The test `a Capture never loads or offers a queue` is vacuous because it only checks an empty UI without verifying queue loading. Inject a recording or failing `ClarificationService` into `ProcessToHandlers` and assert that planned actions are never requested; if supported by existing fixtures, seed a planned action and also assert its row and the `PLANNED` header are absent.
1720-1753: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert
lastClarifiedAtfor replacement promotion.This test verifies replacement, queue retention, routing, and notification. It does not verify the required clarification timestamp. Add an assertion that the confirmed replacement stamps
lastClarifiedAt.Suggested assertion
expect((await db.todoDao.getTodo('q3'))?.intent, 'next'); + expect((await db.todoDao.getTodo('q3'))?.lastClarifiedAt, isNotNull); expect(fired, [ProcessAction.nextActionDialog]);🤖 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 `@app/test/widgets/process_to_handlers_test.dart` around lines 1720 - 1753, Extend the replacement-promotion test around the confirmed action in the test case starting with “promoting over a current Action” to assert that the promoted current action has a non-null, updated lastClarifiedAt timestamp. Retrieve the resulting current action once and validate its clarification timestamp while preserving the existing replacement, queue, termination, todo, and routing assertions.
♻️ Duplicate comments (1)
app/test/widgets/process_to_handlers_test.dart (1)
1792-1841:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReapply the Outcome-state assertions for the concurrent-current guard.
The
q6test still seeds the defaultnextintent. It does not assertintentorlastClarifiedAtafter the guard aborts. A regression could commit the Outcome route while leaving the Action rows unchanged.This repeats the previously reported test gap. Seed
intent: 'maybe', capture the initial timestamp, and assert both values remain unchanged.Suggested test update
- final todo = await _insertTodo(db, id: 'q6'); + final todo = await _insertTodo(db, id: 'q6', intent: 'maybe'); + final clarifiedBefore = todo.lastClarifiedAt; ... + final row = await db.todoDao.getTodo('q6'); + expect(row?.intent, 'maybe'); + expect(row?.lastClarifiedAt, clarifiedBefore);🤖 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 `@app/test/widgets/process_to_handlers_test.dart` around lines 1792 - 1841, Update the q6 test around seedCurrentAction and the concurrent-current guard to seed the Outcome with intent “maybe” and capture its initial lastClarifiedAt timestamp before opening the dialog. After the guard aborts, assert the Outcome intent and lastClarifiedAt still equal those initial values, alongside the existing Action and route 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 `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1907-1929: The test `a Capture never loads or offers a queue` is
vacuous because it only checks an empty UI without verifying queue loading.
Inject a recording or failing `ClarificationService` into `ProcessToHandlers`
and assert that planned actions are never requested; if supported by existing
fixtures, seed a planned action and also assert its row and the `PLANNED` header
are absent.
- Around line 1720-1753: Extend the replacement-promotion test around the
confirmed action in the test case starting with “promoting over a current
Action” to assert that the promoted current action has a non-null, updated
lastClarifiedAt timestamp. Retrieve the resulting current action once and
validate its clarification timestamp while preserving the existing replacement,
queue, termination, todo, and routing assertions.
---
Duplicate comments:
In `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1792-1841: Update the q6 test around seedCurrentAction and the
concurrent-current guard to seed the Outcome with intent “maybe” and capture its
initial lastClarifiedAt timestamp before opening the dialog. After the guard
aborts, assert the Outcome intent and lastClarifiedAt still equal those initial
values, alongside the existing Action and route assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db815141-f0ba-417c-9577-e79cb906cb2d
📒 Files selected for processing (1)
app/test/widgets/process_to_handlers_test.dart
The `expectedCurrentActionId` guard test seeded the Outcome with the default `next` intent, so a spurious `_commit(nextAction)` would have written the intent the Outcome already had — invisible. Its two sibling abort tests (declined confirm, vanished planned row) both seed Someday and assert the route survives; this one did not. Seed `intent: 'maybe'` and assert `intent`/`lastClarifiedAt` are untouched, so the test pins that the guard's throw lands before `_commit` rather than only that the Action rows are unchanged. Verified by mutation: swallowing the guard throw leaves every pre-existing assertion in this test green and fails only the new one. Refs: #728 (comment)
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)
app/test/widgets/process_to_handlers_test.dart (1)
1914-1936: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the Capture test exercise queue suppression.
The test seeds no planned Action and only checks for the hard-coded
p1row. It passes even if the Capture path callsgetPlannedActionsand receives an empty list. It also does not assert that thePLANNEDheader is absent.Inject a
ClarificationServicespy that records or fails on planned-queue reads forCaptureSubject. Assert that no read occurs and thatfind.text('PLANNED')finds nothing.🤖 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 `@app/test/widgets/process_to_handlers_test.dart` around lines 1914 - 1936, The test “a Capture never loads or offers a queue” should explicitly verify queue suppression: inject a ClarificationService spy for the CaptureSubject that records or fails if planned actions are read, then assert no planned-action read occurs and that find.text('PLANNED') finds nothing, while retaining the existing absence check for the promotion row.
🤖 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 `@app/test/widgets/process_to_handlers_test.dart`:
- Around line 1914-1936: The test “a Capture never loads or offers a queue”
should explicitly verify queue suppression: inject a ClarificationService spy
for the CaptureSubject that records or fails if planned actions are read, then
assert no planned-action read occurs and that find.text('PLANNED') finds
nothing, while retaining the existing absence check for the promotion row.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc4da5f8-ed56-4ab4-8c39-459e96c8482f
📒 Files selected for processing (1)
app/test/widgets/process_to_handlers_test.dart
|
No changes needed. |
|
Merge gate: PASSED. Merging.
Judgement call, stated: two Minor outside-diff findings remain unaddressed — make the Capture queue test non-vacuous ( Also on the record, since CodeRabbit's incremental tracking got out of step here: |
|
Tip For best results, initiate chat on the files or code changes.
The two remaining items are test hardening. They do not block the verified behavior in this PR. Thank you for documenting the incremental-review coverage gap. Use 🐇 ✅ You are interacting with an AI system. |
3 participants
Summary
plannedqueue for one-tap promotion at every surface that asks "what's next", closing the gap where a queued Action could only be promoted from task detail (issue Re-clarification surfaces never offer the planned queue, so a queued Action can only be promoted from task detail #723). WidensNextActionDialogto return a sealedNextActionChoice(NextActionPhrase|NextActionPromote) instead of a bareString, and renders a tappable PLANNED section above the text field; an empty queue renders exactly the pre-Re-clarification surfaces never offer the planned queue, so a queued Action can only be promoted from task detail #723 text-only dialog._nextWithDialoginprocess_to_handlers.dartto load the queue (Outcome only), dispatch on the result type, and split out_blankSaveFallbackand_promoteFromQueue. Promoting over a current Action routes through the shared "Replace current action" confirm rather than a silent supersede; promoting into an Actionless Outcome commits directly. Both stamplast_clarified_at, so promotion stays an explicit clarifying act per ADR-0004 — nothing auto-promotes, and the blank-save title fallback is preserved (no longer the only escape from a fabricated Action).getPlannedActions/getCurrentAction/promotePlannedAction/supersedeAndPromotepassthroughs toClarificationService, reusing the existing DAO primitives — no new DAO work.ClarifyCardNext arms; the title-as-action coupling in_onAfterRouteis now Waiting-For-only (Next's fallback, seeded from the live unsaved title via the newliveTitlecallback, moved into_nextWithDialog).replace_current_action_sheet.dart(returns the verdict; caller owns the write), used by both the task-detail Plan section and the re-clarify surfaces. UpdatesREQUIREMENTS.mdto state promotion is offered wherever the app asks what's next.Test plan
currentand the other staysplanned— and fails without the fix.current.ClarifyCard's Next arm.last_clarified_atand nothing promotes automatically (ADR-0004).flutter analyzeand the full app test suite pass in CI.🤖 Generated with Claude Code
Closes #723