Skip to content

feat(work-items): task page on the object template (#2248) - #2349

Merged
steilerDev merged 7 commits into
betafrom
feat/2248-task-page
Oct 28, 2026
Merged

steilerDev merged 7 commits into
betafrom
feat/2248-task-page

Conversation

@steilerDev

Copy link
Copy Markdown
Owner
  • Task page on the object template: header with Start / Mark done, summary strip (When, Where, Who with Call, Expected cost, Waits for, Holds up, Documents), tabs Overview · Timing · Cost · Site diary · History.
  • GET /api/work-items/:id carries the Tasks list figures (one helper); the holds-up figure is registered for the consistency test.
  • Timing tab with RelatedLists and a dependency dialog (type and lag); DependencySentenceBuilder retired; "Log for this task" pre-fills area and company.

Fixes #2248

Capabilities: WRK-022 to WRK-070, WRK-135, MNY-044/101/105/111/113/114 (capmap and routemap updated; plan:check green; baseline 3/10/1 → 0/6/1).

Test plan

  • Unit/integration: new tests for every new file, all touched files ≥ 95% (server page detail and consistency tests included).
  • E2E: task-page.spec.ts (T1–T15) plus updated task-page specs; figure consistency registry.
  • Full E2E gating (ci-wait.sh <pr> main).

Co-Authored-By: Claude backend-developer noreply@anthropic.com
Co-Authored-By: Claude frontend-developer noreply@anthropic.com
Co-Authored-By: Claude translator noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer noreply@anthropic.com
Co-Authored-By: Claude docs-writer noreply@anthropic.com
Co-Authored-By: Claude product-architect noreply@anthropic.com
Co-Authored-By: Claude ux-designer noreply@anthropic.com
Co-Authored-By: Claude dev-team-lead noreply@anthropic.com

🤖 Generated with Claude Code

- Task page rebuilt on ObjectHeader, SummaryStrip and ObjectTabs: Start / Mark done
  primary, strip (When, Where, Who with Call, Expected cost, Waits for, Holds up,
  Documents), tabs Overview, Timing, Cost, Site diary and History.
- GET /api/work-items/:id returns the page figures from the Tasks list helper
  (expected cost, grants, waits for, holds up, company phone); holds-up surface
  registered for the consistency test.
- Timing tab: Dates and Constraints cards, Reschedule panel, dependency and
  milestone RelatedLists with an Edit dialog for type and lag; the
  DependencySentenceBuilder is retired (the New task form uses a Waits for picker).
- Cost tab with cost lines and Grants; Site diary tab with derived entries and
  "Log for this task" pre-filling area and company.

Fixes #2248

Co-Authored-By: Claude backend-developer <noreply@anthropic.com>
Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude translator <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude e2e-test-engineer <noreply@anthropic.com>
Co-Authored-By: Claude docs-writer <noreply@anthropic.com>
Co-Authored-By: Claude product-architect <noreply@anthropic.com>
Co-Authored-By: Claude ux-designer <noreply@anthropic.com>
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE
[security-engineer]

Scope: server/ + shared/, client dependenciesApi/workItemsApi, diary capture company pre-fill, notes authorization. No findings.

  • Auth: GET /api/work-items/:id keeps the request.user gate (401 otherwise). Both roles keep access, as before.
  • Data exposure: assignedVendor.phone is a plain vendors.phone read through the same row the company list/detail already return to both roles, so it adds no new disclosure. Expected cost, grants, waitsFor and holdsUp are the same figures the list rows already expose via buildRowFigures. The response is an explicit spread, with no password hash or token fields.
  • Query safety: all queries are Drizzle-parameterized (eq, inArray). The only id used is the route param, which is schema-validated and 404s when missing.
  • Per-request cost (informational, not blocking): each detail request builds the schedule graph, the projection and the expected-cost map, and runs getExpectedCostByWorkItem only when the task has budget lines. This is the same cost class as the Tasks list endpoint, the app is 1-5 users, and the endpoint needs authentication, so there is no realistic DoS.
  • Dependency PATCH: PATCH /work-items/:id/dependencies/:predecessorId requires authentication and has an AJV schema (updateDependencySchema). The client change only adds a thin wrapper.
  • Capture ?companyId=: the value only pre-fills the form, and only for a new daily_log entry. Unknown ids are cleared client-side (onUnknown in SiteDetailsFields). The server independently rejects a vendorId that doesn't reference an existing vendor (InvalidMetadataError). It is never used for authorization, and React renders the value as text.
  • Notes canManage: UI-only convenience. The server enforces it. PATCH and DELETE on notes require the author or an admin (ForbiddenError in noteService), and the note must belong to the :workItemId in the URL (otherwise 404), so there is no IDOR.
  • No new dependencies and no dangerouslySetInnerHTML/eval in the diff.

@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: REQUEST_CHANGES
[product-owner]

Story #2248 (AC1-AC10) is met in code. I checked the page shell, strip, tabs, Overview, Timing, Cost, Site diary, History, delete, the retired DependencySentenceBuilder and the RelatedList Add/Edit/Remove-with-Undo flows. Two fix-in-session findings remain; neither is a functional AC gap. CI shards were still running when I reviewed.

Findings (fix in this PR)

  1. Docs mis-describe the Waits for fact (display/accuracy, fix-in-session). docs/src/guides/work-items/index.md, Summary Strip table: "Waits for | How many tasks this one waits for, or Nothing". The strip shows the first unfinished task's title plus "+n more", or "Nothing" (useTaskSummaryFacts.tsx, D5). Only Holds up is a count. Rewrite the row to match, e.g. "The first task this one is waiting for, with how many more, or Nothing".
  2. New canon terms are missing from client/src/i18n/glossary.json (fix-in-session). The page introduces "Not before", "Start by" and "Waiting for this task" (de: "Frühestens ab", "Spätestens ab", "Wartet auf diese Aufgabe"). The glossary has the sibling "Waiting for this milestone" and "Waits for" but none of these three. Add the three entries (en/de, deSource, definition) so the canon covers every task-page label. I approve the terms themselves; the translator proposes the entries and I approve them as written here.

Rulings on the flagged decisions

  • D3 approved. Documents is an AC8 "fact" and the strip is capped at 7, so the grant moves to a remark on Expected cost. AC2's "grant by name when one covers it" is met (the first grant by name, "and n more" when several). The docs and Grants list cover the rest.
  • D4 approved. Household members have no phone field, so a Call button for them would be invented data. Call shows for a company with a phone only, as a sibling control (no nested interactive content). The docs say so.
  • D6 approved. A direct-successor "Holds up (2)" list beside a transitive "Holds up 5 tasks" is the same-label-different-count defect [P3.5] Consistency test across money and attention surfaces #2232 removed. "Waiting for this task" matches the glossary pattern "Waiting for this milestone". "Waits for" and the strip's "Holds up" keep their canon. This is a reading of AC10 and I accept it. Finding 2 adds the glossary entry.
  • D8 approved. The notes and checklist DELETE endpoints return 204 with no snapshot, so Undo is impossible and ConfirmDialog stays. Move up/down in the ⋯ menu is the accessible replacement for drag. The capmap records both.
  • D9 approved. ReschedulePanel is shared and has no constraint fields, so a Constraints card with its own Edit is the right call and it is recorded in the capmap.
  • D11 approved. One Milestones list with a relation dialog; the relation is named in text, never colour only.
  • D12 approved. BudgetCostOverview, SubsidyLinkSection and the payback leave the task page. The Expected cost fact and the Grants list carry the money, so no new money surface appears.
  • D13 approved. Diary entries do not store a task id, so the company within the task dates is the honest derivation. Block 2 sits in a collapsed <details> labelled "from photo tags and dates". "Log for this task" pre-fills area and company.
  • D16 approved. New task "Waits for" creates finish-to-start with lag 0 (WRK-022). The type is editable afterwards on Timing, and the docs say so.
  • D20 label changes approved (Who, Not before, Start by, Waits for), subject to finding 2. The translations use informal du and sentence case.
  • Where fact linking to the Areas list until P5.7 approved. This matches AreaLink and needs no code change when the area page ships.
  • Header primary "Log" once the task is done approved. Its docs mention Log for this task in the Site diary tab alongside it.
  • German "Beginn" for the Timing start label approved. The glossary's "Start" is the verb (Starten) and the Home noun; "Beginn" is correct for a date label next to "Begonnen" and "Ende".

Checks that passed

  • No Work Item, Household Item or Vendor labels on the task page in en or de.
  • DependencySentenceBuilder is gone from client/, e2e/ and docs. The old milestone chips and selects are retired.
  • Docs: Title Case headings, bold UI labels match the en strings, and the anchors exist.
  • The AC9 450 px desktop / 844 px phone assertion exists in task-page.spec.ts.

Once findings 1 and 2 are fixed, I approve. The story then goes to UAT on the board rather than straight to Done, because the page has visual ACs (AC9 fit) that only a rendered check settles.

@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: REQUEST_CHANGES
[ux-designer]

Design review of #2349 (story #2248) against my spec on the issue and the Task page Style Guide section. Method: static review of the built code and CSS (no pixel render available here). Tokens, dark mode (Layer-2 tokens only, no overrides, no hardcoded values in any new .module.css), shared-component reuse (SectionCard/SectionCardFields, SummaryStrip trailing, RelatedList, StatusMenu, ObjectForm, CostLinesSection, Badge, EntityPicker), single primary (ObjectHeader only), reduced motion on the <details> chevron, the <details> summary (44px, --shadow-focus), trailing stacking above the stretched link (.trailing is positioned at --z-raised, later in the DOM than the ::after), label-in-name on Call, h1 fallbacks and the ConfirmDialog delete flow all check out. Deviations D2, D3, D4, D6, D8, D9, D11, Add ▾ placement and the hidden empty diary block 2 are accepted (all are in the Deviation Log; I added the three that were missing).

All findings below are fix-in-session, low effort, inside this PR's files.

1. Medium (a11y, focus): deleting a non-last checklist item or note drops focus to the page h1.
TaskChecklistCard.tsx (ConfirmDialog testIdPrefix="subtask-delete") and TaskNotesCard.tsx (note-delete) pass no returnFocusRef. After a confirmed delete ConfirmDialog skips the opener, so focus goes to the h1, far above a long Overview. Pass the card's stable control: returnFocusRef={addInputRef} in the checklist, returnFocusRef={addButtonRef} in notes. The last-item case is already handled by onEmptied.

2. Medium (a11y): Move up / Move down is silent.
TaskChecklistCard.tsx move() reorders with no announcement; the row jumps and a screen-reader user hears nothing (WCAG 4.1.3). The Areas page already does this (AreasPage.tsx announce + index.position, polite role="status"). Add the same: a visually hidden polite region in the card and a new taskPage.checklist.position key ("‹title› moved to position n of m"), plus the translator pass.

3. Medium (touch targets <=1023px): compact buttons without the 44px rule.
btnSecondaryCompact is ~30px high. The page's own CSS adds min-height: var(--touch-target-min) under @media (max-width: 1023px) for Reschedule/Retry/Dismiss, but not for: Add ▾ (TaskOverviewPanel.tsx, uses sharedStyles.btnSecondaryCompact directly), checklist "Add" (task-checklist-add), "Add note" (task-notes-add), the Overview Retry buttons, and the diary .logButton (TaskDiaryPanel.module.css). Compose each from btnSecondaryCompact in a local class with the same media-query min-height (the SectionCard .edit pattern), and make .addInput/the Add button align in .addForm.

4. Medium (layout, Cost tab): the Cost lines section is bare and touches the Grants card.
TaskCostPanel.tsx returns a fragment: CostLinesSection (.section has no surface, no outer margin) directly followed by the Grants RelatedList card (.root has no margin), so there is zero gap and the cost table has no card, unlike every other tab and unlike the Invoice page (.costCard composes card). Wrap in a panel column (display:flex; flex-direction:column; gap: var(--spacing-4), --spacing-6 from 768px) and put CostLinesSection in a card-composed wrapper as InvoiceDetailPage.module.css .costCard does.

5. Low (rhythm): panel gaps differ per tab.
WorkItemDetailPage.module.css .panel is 16px, 24px from 768px (Overview, History); TaskTimingPanel.module.css and TaskDiaryPanel.module.css .panel are a fixed 16px. Use one value everywhere (the 16/24 rule from the spec), including finding 4.

6. Medium (AC9 phone / name legibility): Who fact with Call has ~55px for the name on a phone.
The strip is 2 columns at 390px (~154px content) and the Call button (icon + "Call" + padding) plus gap takes ~95px, so a company name ellipsises to a few characters. In useTaskSummaryFacts.tsx mark Who wide: true when trailing is set (factSpans already fills the row; it still fits the one-screen budget: header + 4 strip rows + tabs is well under 844px).

7. Low (a11y): focus is lost while the Reschedule section loads.
TaskTimingPanel.tsx unmounts the "Reschedule…" button on open; ReschedulePanel only focuses its End field once the timeline has loaded (and on a load failure never). Give the RescheduleSection h2 tabIndex={-1} and focus it on mount (ReschedulePanel then takes focus when ready).

Style Guide corrections (already pushed to the wiki, b95c341; the code is the source of truth where noted): date step is Today / As planned / Pick a date (not Yesterday); Where links to the Areas list until the area page is served (matches AreaLink; D21's isRouteServed form is not used, same end state, not a defect); Expected cost has no figureKind; Waits for shows the first unfinished title plus "+n more" (not a count); remark colour is --color-text-secondary; Documents is label "Documents" with the count as value; CallButton is 32px on fine pointers, 44px on coarse (the guide said 44px always); planned line is "Planned: ‹range›" below the facts; dependency facts read "‹sentence› · n days later"; milestone relation words are "Waits for" / "Counts toward"; dialog labels "How they connect" / "Lag in days"; the milestone dialog's button is not disabled; History line is "Created by ‹name› on ‹date› · Last updated ‹date›"; Log button is "Log for this task". Statements about the Who wide span, the delete focus target and the move announcement describe the state after the fixes above.

No follow-up issues: all seven are fixable in this PR.

@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: REQUEST_CHANGES
[product-architect]

Review of PR #2349 (#2248, task page), head 762eb96a.

Verified, no findings

  • buildRowFigures holds the one definition of grants, expectedCost, isCritical, waitsFor and holdsUp. listWorkItemRows spreads it unchanged, so list behaviour is the same. getWorkItemPageDetail reuses it with the same today, projection and index derivation. figures.consistency.test.ts compares the detail with the list row (holdsUp, waitsFor, expectedCost, grants) and with the timeline (holdsUp). workItems.test.ts asserts the page fields, the phone and that isCritical is absent.
  • The contract matches the code. WorkItemPageDetail and WorkItemPageVendor in shared/src/types/workItem.ts are as documented in API-Contract › Work Item Response Shapes. Only GET returns the superset. POST, PATCH, dependency and subtask responses keep WorkItemDetail, and the client never puts a PATCH body into the page record (useTaskRecord only re-GETs).
  • Route map: diaryEntryNew carries companyId, with CREATE_TARGETS.diaryEntry.prefill and the capture pre-select (daily log only; a 404 company is dropped through useCompanyName.onUnknown). The depError query map is interim/page. The page does serve it: the param is read once and removed by the single replace navigate, which keeps location.state. Tests are in routeMap.test.ts.
  • Client: the Attention link goes to ?tab=timing#reschedule. DependencySentenceBuilder is deleted with no references left (code, i18n, docs). SummaryStrip.trailing is a sibling inside the <dd>, never nested. The EntityPicker milestone and grant kinds load once and filter on the client. SectionCardFields is the one copy (company and task pages). fetchWorkItemSubsidyPayback is removed with its last caller.
  • Wiki: the gitlink 72dec273 equals origin/master. Architecture › Task page, the Create targets table, the entity picker kinds and the Deviation Log rows match the code.
  • ! assertions: the new ones in CaptureScreen, TaskChecklistCard, TaskMilestoneList, TaskDependencyLists, TaskOverviewPanel and TaskTimingPanel all run in synchronous code (effects, or before the first await of a click handler).
  • Coverage, run locally on every touched production file: every client file is ≥95% statements and branches. The new server code is fully covered except the branch in finding 1.

Findings (all fix-in-session)

  1. Unreachable branch at server/src/services/workItemService.ts:1167. vendorRow?.phone ?? null: the ?. arm where vendorRow is undefined can never run. detail.assignedVendor was just loaded from the same row's vendor join, in the same synchronous call. It is the only uncovered branch in the new service code. Fix: phone: vendorRow!.phone (the ?? null is not needed either, since phone is already string | null).
  2. Unreachable ?. on mounted refs, and a duplicated ternary in client/src/pages/WorkItemDetailPage/WorkItemDetailPage.tsx:208 and :215. The Details and Who-and-where SectionCards always render while Overview is the active tab (TaskOverviewPanel.tsx, no condition), so the null arm of .current?.startEdit() is unreachable. Both lines show up as uncovered cond-expr arms. Line 215 also repeats the target === 'details' ? detailsCardRef : whoWhereCardRef ternary that cardFor (line 200) already holds. Fix: use cardFor(target).current!.startEdit() in both places (both are synchronous) and drop the inline ternary.
  3. Stale record for one render on task-to-task navigation at client/src/pages/WorkItemDetailPage/WorkItemDetailPage.tsx:64. <TaskView id={id} /> is not keyed. When the route goes from task A to task B (the Waits for / Waiting for this task rows, the strip's Waits for fact, breadcrumbs), useTaskRecord keeps A's record with isLoading: false until its effect calls reload() (see useTaskRecord.test.ts › "loads the new task when the id changes"). For that commit, TaskLoaded renders A's title, strip and panels under B's URL. Its effects also run with mixed ids: useTaskDiary(A), useDocumentsPanel(B), useCreateContext with A's area and company, and the timing relations of A. Fix: <TaskView key={id} id={id} />, plus a page test that re-renders the route with a second id and asserts the loading state (h1 "Task") shows rather than the first task's title.

No follow-up issues: all three are contained in the PR's files.

- Delete focus returns to the row trigger; checklist moves are announced; 44px
  compact buttons below 1024px; Who fact wide with Call; shared panel spacing.
- Moving between tasks remounts the page (no stale task flash); Reschedule
  heading takes focus on open.
- Glossary entries Not before, Start by, Waiting for this task; docs strip rows
  corrected; Style Guide task page section matched to the code.

Fixes #2248

Co-Authored-By: Claude docs-writer <noreply@anthropic.com>
Co-Authored-By: Claude translator <noreply@anthropic.com>
Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude backend-developer <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude product-architect <noreply@anthropic.com>
Co-Authored-By: Claude ux-designer <noreply@anthropic.com>
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE
[product-owner]

Round 2 (82b3560): both findings are closed.

  1. The docs Waits for strip row now describes the first unfinished task plus a +n count, or Nothing, and Holds up is a count. This matches useTaskSummaryFacts.tsx.
  2. The glossary has "Not before" (de Fruehestens ab, formerly Start After), "Start by" (de Spaetestens ab, formerly Start Before) and "Waiting for this task" (de Wartet auf diese Aufgabe). The alias pins are in glossary.test.ts.

The AC1-AC10 coverage and every earlier ruling (D3, D4, D6, D8, D9, D11-D13, D16, D20) stand. The extra UX and architect fixes (focus return, 44px targets, checklist position announcement with en/de keys, task-to-task remount) are in scope and add no behaviour outside the AC.

Board: after merge, #2248 goes to UAT (not Done-on-merge) because AC9 fit is a rendered check; use Refs #2248, not Fixes, if the commit reaches main without an epic close.

@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE
[ux-designer]

Round 2 re-review of head 82b3560 (git show 82b3560b -- client/src). All seven findings are fixed as requested:

  1. Delete dialogs now pass returnFocusRef (addInputRef in the checklist, addButtonRef in notes); the last-item path to Add ▾ is unchanged.
  2. Move up/down announces through an always-mounted role="status" + srOnly region (task-checklist-status), cleared-then-set via rAF as on the Areas page, only after a successful move, with the position computed from the pre-move index.
  3. .compactButton composes btnSecondaryCompact with min-height: var(--touch-target-min) at 1023px and below, and is used by Add ▾, checklist Add, Add note and both Retry buttons; .logButton has the same rule.
  4. Cost lines sit in a .costCard (composes card, the Invoice pattern) inside a column panel, so the Grants card no longer touches it.
  5. Timing, Cost and Diary compose the single .panel (16px, 24px from 768px) with composes ... from (the correct cross-module technique, no :global).
  6. Who is wide whenever Call shows.
  7. The Reschedule h2 has tabIndex={-1} and takes focus on mount.

Tokens only, no hardcoded values, dark mode unaffected, no new components. The extra key={id} remount and the glossary/docs changes raise no design concerns. Style Guide b95c341 matches the fixed code. No remaining findings.

@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: REQUEST_CHANGES
[product-architect]

Round 2, head 82b3560b.

My three round-1 findings are fixed:

  1. workItemService.ts:1167 now reads phone: vendorRow!.phone. The unreachable arm is gone.
  2. WorkItemDetailPage.tsx:208/215 both call cardFor(target).current!.startEdit(). The repeated ternary is removed, and both calls are synchronous.
  3. <TaskView key={id} id={id} /> is in place. The new page test goes from wi-1 to wi-2 and asserts that the h1 reads "Task", that "Pour slab" never shows, and that wi-2 is fetched.

The wiki gitlink b95c3413 equals origin/master.

One new finding (fix-in-session), introduced by this commit:

  • ! used inside an async callback at client/src/pages/WorkItemDetailPage/TaskChecklistCard.tsx:103. The new announcement reads items[index]!.title inside the .then of run(...), which runs after the reorder request resolves. That breaks the house rule that ! is only used in synchronous code. It is safe today only because items is the render-time snapshot, so it is fragile. The .then((moved) => …) parameter also shadows the moved id destructured on line 94.
    Fix: read the title synchronously before the request with const { title } = items[index]!; next to line 93. Use title in the announcement, and rename the callback parameter, for example to ok.

I found nothing else in this commit's other production changes: the compact buttons, focus return, the Reschedule heading focus (a synchronous mount effect) and the wide Who fact.

steilerDev and others added 2 commits October 28, 2026 11:34
…st (#2248)

Fixes #2248

Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
…test fixes (#2248)

Fixes #2248

Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude e2e-test-engineer <noreply@anthropic.com>
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE
[product-architect]

Head 1db9fe53.

  • Round-2 leftover (fixed in 62505757): TaskChecklistCard.tsx now reads the title synchronously before the request (const { title } = items[index]!;). The .then callback uses title and the renamed ok, so the ! sits in synchronous code and nothing is shadowed.
  • dialogFocus.ts: the guard is now "opener disconnected → <list>-add, else h1". This is correct. The Modal's own focus return already lands on the h1, so the old activeElement === body check could never fire. The ?. calls inside the rAF callback are async code, so they are fine there.
  • WorkItemDetailPage.module.css: the :root[data-shell-bar='shown'] .addRow:last-child clearance follows the existing StickyActionBar / AppShell pattern with --shell-bottom-height plus a spacing token. No shared component changes.
  • searchDestinations.ts: adding the 'Not before' and 'Start by' entries to TERMS_WITHOUT_DESTINATION is consistent with the glossary entries this PR adds. They are field labels with no page.

No findings.

@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE
[ux-designer]

Re-review of head 1db9fe5 (git show 1db9fe53 -- client/src).

  • Focus after the first Add: restoreFocusAfterDialog now moves focus to the list's Add button (else the h1) whenever the opener is gone, instead of only when focus sat on <body>. This matches the Style Guide focus rule (never body, never a stale h1 after a dialog whose opener vanished). No visual change.
  • Phone Add ▾ clearance: :root[data-shell-bar='shown'] .addRow:last-child { padding-bottom: calc(var(--shell-bottom-height) + var(--spacing-16)) } uses existing tokens and the same data-shell-bar hook StickyActionBar and index.css already use, so it is token-only, dark-mode neutral, and leaves desktop and the unshown-bar case untouched. The :last-child scoping means it only applies when the Add row ends the panel (a following Checklist/Notes card already gives the menu room). Padding only extends the scrollable area, so no focusable is hidden and no layout shifts above the row.
  • The searchDestinations.ts additions are non-visual.

No findings.

)

Fixes #2248

Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude e2e-test-engineer <noreply@anthropic.com>
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE
[ux-designer]

Re-review of head 72c2c10 (git show 72c2c10d -- client/src), the SearchPicker change for the dialog-footer overlap.

  • SearchPicker.handleFocus now opens the list only when the focus event lands inside the task of a Tab / Shift+Tab keydown (window capture listener, flag cleared by setTimeout(0), listener and timer cleaned up on unmount). A dialog's initial focus, focus restore and any other programmatic .focus() no longer open it; suppressFocusOpenRef is untouched.
  • Pointer and touch arrival still open the list through handleClick; typing and ArrowDown are unchanged.
  • Dependency dialog ("Add a task this one waits for"): the task picker is the first field, so it now receives initial focus with the list closed. Cancel and Save are reachable and clickable, a stray click can no longer pick a task, and the list opens as soon as the user types or clicks. Edit mode has no picker and is unaffected.
  • Milestone link dialog: the Relation select takes initial focus; Tabbing into the milestone picker opens its list as intended (user intent, Escape or click outside closes it), so no list is open on dialog open.
  • Behaviour after Clear or a selection (list closed, focus in the input) is acceptable: ArrowDown, typing or a click reopens it, and the chip/Clear focus rule is unchanged.
  • It is a single-file change in the shared component, so every picker in a dialog benefits (area, company, grant, purchase, invoice and the New task form); no token, dark-mode or layout impact.

No findings.

@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: REQUEST_CHANGES
[product-architect]

Head 72c2c10d (SearchPicker opens its list on user intent only).

Code: no findings.

  • The mechanism is sound. A capture-phase keydown on window runs before any element handler. That includes a Modal focus trap, so a trap that moves focus on Tab still counts as keyboard arrival. The flag lives until the end of the task via setTimeout(0), and the focus event of a Tab arrival fires inside that task. Repeated Tabs clear the old timer, and the listener and timer are removed on unmount. autoFocus already suppressed the open, so nothing changes there. Click, typing and ArrowDown still open the list, so pointer, touch and screen-reader users keep a way in.
  • A listener per instance is acceptable at this scale: one cheap key check per keydown per mounted picker. If a page ever mounts many pickers at once, the documented way out is a single module-level listener.
  • Tests cover every path: programmatic and synthetic focus, Tab, Shift+Tab, no carry-over to a later task, a same-task programmatic focus, other keys, click, and unmount with a timer still pending.

One finding (fix-in-session, mechanical): the wiki gitlink.
This changes the open behaviour of every picker in the app (a cross-cutting convention), and the Architecture page did not say when the list opens. I documented it in the SearchPicker bullet and added a Deviation Log row (Architecture › Entity picker and related lists), pushed as wiki 7f56ac13. The PR still records b95c3413.
Fix: git -C wiki fetch origin master && git -C wiki checkout 7f56ac1345331e6399f2a96f0f9f223ab6a5a7c6, then git add wiki in the next commit. Once the gitlink is 7f56ac13, this is an APPROVE.

Fixes #2248

Co-Authored-By: Claude product-architect <noreply@anthropic.com>
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

VERDICT: APPROVE
[product-architect]

Head aff1f177. This commit changes only the wiki gitlink, to 7f56ac13, which matches origin/master (it carries the SearchPicker open rule and its Deviation Log row). All my findings from earlier rounds are resolved, and nothing is open.

Fixes #2248

Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude e2e-test-engineer <noreply@anthropic.com>
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

[orchestrator] Merge record for #2349 (story #2248).

Reviewer verdicts:

  • security-engineer: APPROVE (round 1, zero findings). Task page detail keeps auth; the company phone and the strip figures are already visible to both roles elsewhere; queries are parameterized; the capture company id only pre-fills and is validated server-side; note edit/delete is enforced server-side.
  • product-owner: APPROVE (round 2); round 1 raised two fix-in-session items (the Waits for strip row in the docs, three new glossary terms), fixed in head 82b3560. Rulings: D3, D4, D6, D8, D9, D11, D12, D13, D16, the D20 labels, the interim Where link to the Areas list, "Log" once done and German "Beginn" all approved.
  • ux-designer: APPROVE (round 2); round 1 raised seven fix-in-session items (delete focus, a silent checklist reorder, compact touch targets, a cardless Cost tab, panel spacing, the Who fact width with Call, Reschedule focus), fixed in head 82b3560; the Style Guide task page section was corrected against the code.
  • product-architect: round 2 found the three round-1 items fixed (an unreachable phone fallback, unreachable card optional chains, and a real defect: the previous task flashed when moving between tasks, now remounted per id with a test). Its one round-2 leftover (a non-null assertion after an await in the checklist reorder) was trivial and fixed in head 6250575 without a third round, per the session rules.

Review rounds: 2. Before review, the internal review fixed the Call button sitting under the strip's stretched link, focus after the first Add from an empty list and after Add › Note, a hidden edit-mode dependency error, and an unknown pre-filled company. QA found and the team fixed a real defect: focus stayed on the heading after deleting the last saved checklist item.

CI then exposed two real defects, fixed in 1db9fe5 and re-approved by product-architect and ux-designer: focus after the first Add from an empty list never reached the list's Add button (the dialog had already moved focus to the heading), and on phones the last "Add" menu item sat under the fixed bottom bar (the Add row now gets the shell bar's bottom clearance). The other CI fixes were tests only (the Cost tab URL, a controlled checkbox, forecast dates on undated tasks, the depError stage, the diary pre-fill href, grants on the task detail, and two field-label terms listed as having no search destination).

A further CI run exposed a real overlap: a dialog that focuses a picker first opened the picker's list over the dialog's Cancel and Save. UX ruled that the shared SearchPicker opens its list only on user intent (click, typing, ArrowDown or Tab arrival), never on programmatic focus; fixed in 72c2c10 with tests for every path and the consumer tests switched to clicks. The rule is recorded in the wiki (7f56ac13, gitlink in aff1f17); ux-designer and product-architect approved.

A final test-only commit (6a0d61f) switched the Paperless inline-company specs to click the picker and fixed two timing-dependent tests.

CI: Quality Gates and E2E Gates green on head 6a0d61f.

@steilerDev
steilerDev merged commit 6cd6dc1 into beta Oct 28, 2026
34 checks passed
@steilerDev
steilerDev deleted the feat/2248-task-page branch October 28, 2026 12:40
@steilerDev steilerDev mentioned this pull request Oct 28, 2026
10 tasks
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 2.19.0-beta.71 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant