Repository navigation
[3312 by Claude Opus 5.5] Keep completed repository To-Do text readable on desktop and touch - #3313
Merged
integry merged 1 commit intoOct 21, 2026
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…about 7.2:1 on desktop and touch, up from 1.65:1 and 2.45:1. The new rendered test passes on the fixed component and fails on the original one with the issue's exact numbers. The fix is in `propr-ui/src/components/Repositories/RepoTodos/CompletedItemsAccordion.tsx`: - Row: I removed the 60% opacity (and its touch override), so the row and its enabled Restore/Delete controls are no longer faded. Done rows are still marked by the strikethrough and the light grey background. - Text: the note colour is now `text-slate-600` instead of `text-slate-400`. - Delete icon: it is now `text-slate-500` instead of the near-invisible `text-slate-300`. That matches the colour touch already used. On desktop it still only appears on hover or focus. Nothing else changed: no buttons are disabled, all content and actions stay, and wrapping, issue links, focus rings and 44px touch targets are as before. I left the completed styling in `SortableTodoItem` alone, because completed To-Dos are only shown in the accordion. The new test is `propr-ui/e2e/repo-todo-completed-contrast.pw.ts`, run against the built production UI with a local fixture store: - How it measures: it reads each element's computed colours and blends them through every background and every ancestor's opacity, then calculates the contrast ratio. It does not sample screenshot pixels. - What it checks: it requires at least 4.5:1 and no fading for the completed note and the `#7` issue link. It checks desktop 1280×820 (at rest, row hover, link hover, link focus, Delete focus) and touch at 320 and 390 (at rest and with Restore focused). It also checks that long notes wrap without overflowing the row or the page. - Old-style check: inside the test, the old colour and opacity are applied to a row and must fail. They measure 1.653:1 on desktop and 2.451:1 on touch, matching the issue. - Against the original component: I swapped the original file back in temporarily and ran the test. The 3 contrast tests failed (desktop on opacity 0.6, which is 1.653:1; touch at 2.4506:1), while the 3 interaction tests passed. - Interactions: keyboard on desktop and taps on touch restore one completed item and delete another. The test checks that only those two items get exactly the expected write requests. Other checks run: - The existing `e2e/repo-todo-rows.pw.ts` suite: all 11 tests pass alongside the 6 new ones. - Vitest for `src/components/Repositories`: 21 passed. - `tsc --noEmit` is clean, and `eslint --max-warnings 0` on both changed files is clean. I captured synthetic-data previews of the completed rows at desktop 1280, 390 touch and 320 touch.
Owner
|
/review codex-astra |
Contributor
Author
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
Contributor
Author
🔍 AI Code Review — codex-astraOverall EvaluationMerge-ready within the supplied scope, subject to pending checks completing successfully. ✅ Targeted readability fix — Removing row opacity and changing note text to ✅ Preserved interactions — Restore/Delete handlers, disabled states, accessibility labels, and responsive layout remain intact. ✅ Focused regression coverage — The added tests cover contrast, linked issues, keyboard interaction, touch targets, wrapping, and targeted mutations. They also verify that the previous styling fails the contrast threshold. This review is based on static inspection; no commands or tests were run. The supplied issue description is truncated, so requirements beyond its visible text could not be assessed. The supplied diff is complete. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to No suggestions. ScoreThe implementation is narrowly scoped and supported by meaningful regression tests. No introduced defect was identified; current checks report no failures, with seven still pending. Score: 9/10 🤖 Review Details
🤖 Review by ProPR |
propr-dev Bot
pushed a commit
that referenced
this pull request
Oct 21, 2026
…ith Escape, Cancel, Close, the result's Close or a successful post puts focus back on the button that opened it, as long as that button is still on the page. Only `propr-ui/src/components/TaskDetails/FollowupModal.tsx` changed; #3313's To-Do code is untouched. I didn't test on a real Android device or emulator; touch was covered by Playwright's mobile emulation in Chromium. What changed - Remembering the opener: I removed `autoFocus` from the Comment textarea. The dialog now notes which element had focus, then moves focus to Comment itself, so the opener is recorded before focus moves. - Restoring focus: on close, the dialog lifts the inert state from the workspace first, then focuses the opener. It skips this if the opener is gone, or if focus has already moved to something else outside the dialog, so it won't steal focus after an unrelated change. - Tab handling: Tab and Shift+Tab move by hand through the controls that are currently visible and enabled. This works when focus is on the result message or on a button that a submission just disabled. While posting, when every control is disabled, focus stays on the dialog itself. - Window controls: Tab is left alone when focus is on the desktop window controls, so they keep their normal behaviour. - Unchanged: Escape still doesn't close the dialog while a post is in progress. Prefill, destination, duplicate-post guard, submission lock, failure receipts and result focus/scroll all behave as before. Tests - `FollowupModal.focus.test.tsx` (new, 9 tests): the forward and backward Tab loops, focus moving back in from outside, and each way of closing. It also covers a fake successful post, Tab from the failure receipt (skipping the blocked Post Comment), Tab while posting, an opener that has left the page, and not taking focus back from another control. - `e2e/followup-focus.pw.ts` (new, Playwright): opens Follow Up from the keyboard at desktop 1280×820 and on 390 and 320 touch layouts. It runs both Tab loops, every way of closing, a stubbed failure receipt and a stubbed successful post, and checks the inert regions are back to how they were. A Linux desktop case checks Tab still moves between the window controls normally. All post responses are stubbed in the test, so nothing reaches GitHub. - Fails on the old component: with the unchanged `FollowupModal.tsx`, 7 of the 9 new unit tests failed and all 7 Playwright cases failed (focus left the dialog on Tab, or was on BODY after closing). - With the fix: - The new Playwright spec plus the existing `dialog-keyboard-viewport` (Follow Up keyboard band) and `task-dialog-native-chrome` specs: 30 passed. - Vitest over `TaskDetails`, `hooks` and `utils`: 689 passed. - `tsc --noEmit` is clean, and eslint with zero warnings is clean on the changed files. When typing into the prefilled comment, the cursor starts at the beginning of the text. The old `autoFocus` focused the field the same way, so I left that as it was. I also captured four synthetic screenshots for the previews: Shift+Tab wrapping and Escape returning focus on desktop, and Tab from the failure receipt and Close returning focus at 390px.
propr-dev Bot
pushed a commit
that referenced
this pull request
Oct 21, 2026
…ts`; it was the only file I changed. The modal's restore code was not the cause: the compact header's state comes from an IntersectionObserver that runs separately from the modal. So I made no product changes, and #3313's To-Do code is untouched. I fixed the unstable inert check in `e2e/followup-focus.pw.ts`; it was the only file I changed. The modal's restore code was not the cause: the compact header's state comes from an IntersectionObserver that runs separately from the modal. So I made no product changes, and #3313's To-Do code is untouched. **How the restore check works now** (`expectRestored`, called after Escape, Cancel, Close, the receipt's Close, a successful post and the Linux window-controls case): - **Regions the modal owns:** once the dialog is open, the test tags the outermost regions that were made inert. It requires the task workspace to be inside one of them, so isolation itself is still checked. After dismissal, none of the tagged regions may still be inert. - **A region that was inert before:** a fixed fixture, an inert `<div>` with a button, is added next to the app root before the baseline is taken. Every dismissal must leave it inert. The modal's isolation code skips past it, so a restore that cleared too much would be caught. - **Nothing else left inert:** all inert regions except the compact header must exactly match the baseline. Apart from the compact header, the only inert region in the baseline was the fixture, on every layout. - **Compact header:** its `inert`, `aria-hidden="true"` and `invisible` class must agree with each other after dismissal. Whether it is shown or hidden is not asserted, so its scroll-driven state can change freely. The tab loops, receipt and duplicate-post checks, successful-post body and window-control steps are as before. I added no waits, retries or forced header state. **Checking that the new assertions catch real breakage** (I made each break temporarily, then put the code back): | Temporary change | Result | |---|---| | Restore makes every inert element live again | 7/7 fail: fixture `inert` expected true, got false | | Restore leaves one region inert | 7/7 fail: tagged inert regions expected 0, got 1 | | No restore at all | 7/7 fail on the opener's focus | | Header switched to fully visible after dismissal (what the WebKit trace showed) | 7/7 pass | | Header `inert` removed while still `aria-hidden`/invisible | fails on both touch layouts. On desktop this injection didn't change anything, because Chromium already showed the header there. | **Test runs** - `followup-focus.pw.ts`: 7/7 on Chromium and 7/7 on WebKit, plus 35/35 on WebKit with `--repeat-each 5`. - `followup-focus`, `dialog-keyboard-viewport` and `task-dialog-native-chrome` together: 30/30 on Chromium and 30/30 on WebKit. That is 30 cases, not your 44; I don't know which other cases make up the rest of your set. - ESLint (`--max-warnings 0`) and `tsc --noEmit` (which covers `e2e/`) pass. **WebKit limit:** WebKit here is Playwright's headless WPE build on Debian 12, with its system libraries extracted without root. In it, the old spec at `6f7eb29` also passed 35/35 with `--repeat-each 5`, so I couldn't reproduce the failure itself. The fix is backed by the injected header flip above, not a reproduction. Your diagnostic `/tmp/propr-pr3315-6f7eb29-webkit-inert-baseline-diagnostic/evidence.json` isn't in this container, so I worked from the details in your comment; your original report and traces are untouched. There's nothing visual to show, so I made no previews. PR: #3315 Comment by: @integry (ID: 6318894719) Model: Claude Opus 5.5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI Implementation Summary
Closes #3312
Branch:
3312/claude-opus-5-5-keep-completed-repository-20261021-1354-is9Commits: ✅ Changes committed (5c2cdc0)
AI Processing Completed
Execution Details:
Summary:
I fixed the washed-out completed To-Do rows. Their text now reads at about 7.2:1 on desktop and touch, up from 1.65:1 and 2.45:1. The new rendered test passes on the fixed component and fails on the original one with the issue's exact numbers.
The fix is in
propr-ui/src/components/Repositories/RepoTodos/CompletedItemsAccordion.tsx:text-slate-600instead oftext-slate-400.text-slate-500instead of the near-invisibletext-slate-300. That matches the colour touch already used. On desktop it still only appears on hover or focus.Nothing else changed: no buttons are disabled, all content and actions stay, and wrapping, issue links, focus rings and 44px touch targets are as before. I left the completed styling in
SortableTodoItemalone, because completed To-Dos are only shown in the accordion.The new test is
propr-ui/e2e/repo-todo-completed-contrast.pw.ts, run against the built production UI with a local fixture store:#7issue link. It checks desktop 1280×820 (at rest, row hover, link hover, link focus, Delete focus) and touch at 320 and 390 (at rest and with Restore focused). It also checks that long notes wrap without overflowing the row or the page.Other checks run:
e2e/repo-todo-rows.pw.tssuite: all 11 tests pass alongside the 6 new ones.src/components/Repositories: 21 passed.tsc --noEmitis clean, andeslint --max-warnings 0on both changed files is clean.I captured synthetic-data previews of the completed rows at desktop 1280, 390 touch and 320 touch.
Detailed Logs:
919fb069-fafa-4b3d-b380-857b82748388Log files stored at:
/usr/src/app/logs/executions/issue-3312-2026-10-21T14-02-39-581Z-conversation.json/usr/src/app/logs/executions/issue-3312-2026-10-21T14-02-39-581Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3312.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!
Visual preview
Completed To-Dos, desktop 1280px
Completed rows at full opacity with slate-600 struck-through text on the slate-50 row (about 7.2:1), Delete under keyboard focus with its focus ring, issue chip, and a wrapped long URL. Synthetic fixture data.
Original unavailable: managed storage is unavailable or the upload has expired.
Completed To-Dos, 320px touch
The same completed rows at the narrowest phone width, with the long note wrapping inside the row. Synthetic fixture data.
Original unavailable: managed storage is unavailable or the upload has expired.
Completed To-Dos, 390px touch
Coarse-pointer layout: readable completed notes with 44px Restore and Delete targets that are always visible. Synthetic fixture data.
Original unavailable: managed storage is unavailable or the upload has expired.