From 715b88cd4d2ff6e22b29e3ea08af76eef3008317 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:52:03 +0000 Subject: [PATCH] fix(web): preserve review path punctuation (#2777) --- .../review/review-diff-header.test.tsx | 31 +++++++ .../components/review/review-diff-header.tsx | 25 ++--- apps/web/e2e/helpers/layout-assertions.ts | 40 ++++++++ .../review/mobile-review-file-status.spec.ts | 14 ++- .../tests/review/review-file-status.spec.ts | 21 ++++- docs/plans/review-path-punctuation/plan.md | 70 ++++++++++++++ ...ask-01-preserve-review-path-punctuation.md | 93 +++++++++++++++++++ docs/specs/ui/review-file-status.md | 6 ++ 8 files changed, 287 insertions(+), 13 deletions(-) create mode 100644 docs/plans/review-path-punctuation/plan.md create mode 100644 docs/plans/review-path-punctuation/task-01-preserve-review-path-punctuation.md diff --git a/apps/web/components/review/review-diff-header.test.tsx b/apps/web/components/review/review-diff-header.test.tsx index ebe921c01c..14d9e5a0d7 100644 --- a/apps/web/components/review/review-diff-header.test.tsx +++ b/apps/web/components/review/review-diff-header.test.tsx @@ -126,6 +126,37 @@ describe("ReviewDiffHeader external context", () => { }); }); +describe("ReviewDiffHeader path direction", () => { + it.each([ + { viewport: "desktop", isMobile: false }, + { viewport: "mobile", isMobile: true }, + ])("keeps dot-prefixed directories left-to-right on $viewport", ({ isMobile }) => { + mocks.isMobile = isMobile; + render( + , + ); + + const directory = document.querySelector("[data-review-file-directory]"); + expect(directory).not.toBeNull(); + expect(directory?.querySelector('bdi[dir="ltr"]')?.textContent).toBe(".agents/skills/pr-fixup"); + expect(document.querySelector("[data-review-file-name]")?.textContent).toBe("SKILL.md"); + }); +}); + describe("ReviewDiffHeader responsive composition", () => { it("uses one compact mobile header with the toolbar embedded as the overflow trigger", () => { mocks.isMobile = true; diff --git a/apps/web/components/review/review-diff-header.tsx b/apps/web/components/review/review-diff-header.tsx index 8aaee63d43..519106aef3 100644 --- a/apps/web/components/review/review-diff-header.tsx +++ b/apps/web/components/review/review-diff-header.tsx @@ -75,6 +75,14 @@ function StaleIndicator({ compact = false }: { compact?: boolean }) { ); } +function ReviewFileDirectory({ directory, className }: { directory: string; className: string }) { + return ( + + ); +} + function DesktopReviewFilePath({ path }: { path: string }) { const { directory, name } = splitFilePath(path); return ( @@ -84,12 +92,10 @@ function DesktopReviewFilePath({ path }: { path: string }) { > {directory && ( <> - + /> @@ -122,13 +128,10 @@ function MobileReviewFileDetails({ file, isStale }: { file: ReviewFile; isStale: {directory && ( - + /> )} { + return locator.evaluate((node) => { + const characters: { character: string; left: number; top: number; sourceIndex: number }[] = []; + const walker = document.createTreeWalker(node, NodeFilter.SHOW_TEXT); + let sourceIndex = 0; + + for (let textNode = walker.nextNode(); textNode; textNode = walker.nextNode()) { + const text = textNode.textContent ?? ""; + let offset = 0; + for (const character of Array.from(text)) { + const range = document.createRange(); + const nextOffset = offset + character.length; + range.setStart(textNode, offset); + range.setEnd(textNode, nextOffset); + const rect = range.getBoundingClientRect(); + if (rect.width > 0 && rect.height > 0) { + characters.push({ character, left: rect.left, top: rect.top, sourceIndex }); + } + offset = nextOffset; + sourceIndex += 1; + } + } + + const firstTop = characters[0]?.top; + if (firstTop !== undefined && characters.some(({ top }) => Math.abs(top - firstTop) > 1)) { + throw new Error("Expected text to render on one line"); + } + + return characters + .sort((left, right) => left.left - right.left || left.sourceIndex - right.sourceIndex) + .map(({ character }) => character) + .join(""); + }); +} + // Shared layout assertions for mobile / responsive specs. Extracted because // both onboarding mobile specs need the same overflow + padding checks and // duplicating the DOM walk caused review churn. diff --git a/apps/web/e2e/tests/review/mobile-review-file-status.spec.ts b/apps/web/e2e/tests/review/mobile-review-file-status.spec.ts index c8ce2024c4..2055972612 100644 --- a/apps/web/e2e/tests/review/mobile-review-file-status.spec.ts +++ b/apps/web/e2e/tests/review/mobile-review-file-status.spec.ts @@ -1,9 +1,11 @@ import { test, expect } from "../../fixtures/test-base"; import { GitHelper, makeGitEnv } from "../../helpers/git-helper"; +import { getSingleLineTextInVisualOrder } from "../../helpers/layout-assertions"; import { SessionPage } from "../../pages/session-page"; import path from "node:path"; -const MOBILE_FILE = "packages/mobile/review/surfaces/deeply/nested/mobile-review-status-added.ts"; +const MOBILE_FILE = + ".agents/skills/review/surfaces/with-a-deliberately-long-directory/deeply/nested/mobile-review-status-added.ts"; const MOBILE_MOVED_FROM_FILE = "mobile-review-status-old-name.ts"; const MOBILE_MOVED_FILE = "mobile-review-status-new-name.ts"; @@ -100,6 +102,16 @@ test.describe("Review file status on mobile", () => { const identityRow = header.getByTestId("review-file-identity"); const fileName = identityRow.locator("[data-review-file-name]"); await expect(fileName).toHaveText(path.basename(MOBILE_FILE)); + const directory = identityRow.locator("[data-review-file-directory]"); + await expect(directory).toHaveText(path.dirname(MOBILE_FILE)); + expect(await getSingleLineTextInVisualOrder(directory)).toBe(path.dirname(MOBILE_FILE)); + const directoryMetrics = await directory.evaluate((element) => ({ + clientWidth: element.clientWidth, + scrollWidth: element.scrollWidth, + direction: getComputedStyle(element).direction, + })); + expect(directoryMetrics.scrollWidth).toBeGreaterThan(directoryMetrics.clientWidth); + expect(directoryMetrics.direction).toBe("rtl"); await expect(header.getByTestId("review-file-actions")).toHaveCount(0); const moreActions = header.getByRole("button", { name: `More actions for ${MOBILE_FILE}`, diff --git a/apps/web/e2e/tests/review/review-file-status.spec.ts b/apps/web/e2e/tests/review/review-file-status.spec.ts index acb4c5d04d..262b6b7d30 100644 --- a/apps/web/e2e/tests/review/review-file-status.spec.ts +++ b/apps/web/e2e/tests/review/review-file-status.spec.ts @@ -1,11 +1,13 @@ import { test, expect } from "../../fixtures/test-base"; import { dwell } from "../../helpers/causal-waits"; import { GitHelper, makeGitEnv } from "../../helpers/git-helper"; +import { getSingleLineTextInVisualOrder } from "../../helpers/layout-assertions"; import { SessionPage } from "../../pages/session-page"; import { REVIEW_SIDEBAR_LIMITS } from "../../../hooks/use-review-sidebar-resize"; import path from "node:path"; const ADDED_PATH = "review-status-added.ts"; +const DOTTED_PATH = ".agents/skills/pr-fixup/SKILL.md"; const NESTED_PATH = "review-status-nested/nested.ts"; const MODIFIED_PATH = "review-status-modified.ts"; const DELETED_PATH = "review-status-deleted.ts"; @@ -96,13 +98,15 @@ test.describe("Review file status", () => { git.stageFile(ADDED_PATH); git.createFile(NESTED_PATH, "nested\n"); git.stageFile(NESTED_PATH); + git.createFile(DOTTED_PATH, "dot-prefixed directory\n"); + git.stageFile(DOTTED_PATH); git.modifyFile(MODIFIED_PATH, "after\n"); git.deleteFile(DELETED_PATH); const changesTab = testPage.getByTestId("dockview-tab-changes"); await expect(changesTab).toBeVisible(); await changesTab.click(); - for (const filePath of [ADDED_PATH, NESTED_PATH, MODIFIED_PATH, DELETED_PATH]) { + for (const filePath of [ADDED_PATH, DOTTED_PATH, NESTED_PATH, MODIFIED_PATH, DELETED_PATH]) { const rowTestId = `file-row-${filePath.replace(/[/\\]/g, "-")}`; await expect(testPage.getByTestId(rowTestId)).toBeVisible({ timeout: 20_000 }); } @@ -117,6 +121,21 @@ test.describe("Review file status", () => { const sidebar = dialog.getByTestId("review-dialog-sidebar"); await expect(sidebar).toBeVisible(); + const dottedHeader = dialog.locator( + `[data-testid="review-file-header"][data-file-path="${DOTTED_PATH}"]`, + ); + await expect(dottedHeader).toBeVisible(); + const dottedCollapseButton = dottedHeader.getByRole("button", { + name: `Collapse ${DOTTED_PATH}`, + }); + const dottedDirectory = dottedCollapseButton.locator("[data-review-file-directory]"); + await expect(dottedDirectory).toHaveText(path.dirname(DOTTED_PATH)); + expect(await getSingleLineTextInVisualOrder(dottedDirectory)).toBe(path.dirname(DOTTED_PATH)); + await expect(dottedHeader.locator("[data-review-file-name]")).toHaveText( + path.basename(DOTTED_PATH), + ); + await expect(dottedCollapseButton).toBeVisible(); + const sidebarOrder = await sidebar .getByTestId("review-file-row") .evaluateAll((rows) => rows.map((row) => (row as HTMLElement).dataset.filePath)); diff --git a/docs/plans/review-path-punctuation/plan.md b/docs/plans/review-path-punctuation/plan.md new file mode 100644 index 0000000000..1480eaea40 --- /dev/null +++ b/docs/plans/review-path-punctuation/plan.md @@ -0,0 +1,70 @@ +--- +spec: docs/specs/ui/review-file-status.md +created: 2026-08-17 +status: completed +--- + +# Implementation Plan: Preserve Review Path Punctuation + +## Overview + +Repair sticky Review headers that visually relocate dots from dot-prefixed directory segments. Keep the existing leading-edge directory truncation, isolate the path text as left-to-right content inside its right-to-left clipping wrapper, and prove literal character order in desktop and phone Chromium. No backend, API, state, persistence, or translated-copy changes are required. + +## Confirmed root cause + +`DesktopReviewFilePath` and `MobileReviewFileDetails` render directory text directly inside an element with `direction: rtl` and `unicode-bidi: isolate`. The RTL direction intentionally puts the ellipsis on the leading edge, but the Unicode bidirectional algorithm treats `.` as neutral punctuation and moves it to the trailing edge of the left-to-right directory run. A real production-build Chromium reproduction rendered `.agents/skills/pr-fixup/SKILL.md` as `agents/skills/pr-fixup./SKILL.md` while DOM data and the accessible label remained correct. + +A disposable Chromium probe confirmed the smallest safe presentation change: retain the RTL overflow wrapper and render its directory text inside ``. This preserves leading-edge ellipsis and the logical punctuation order. + +## Frontend + +### Sticky Review file paths + +- `apps/web/components/review/review-diff-header.tsx`: introduce a small shared directory renderer used by `DesktopReviewFilePath` and `MobileReviewFileDetails`. +- Keep `truncate`, `direction: rtl`, and `unicode-bidi: isolate` on the outer directory element so constrained headers continue to retain the directory suffix nearest the filename. +- Put the directory string inside `` so punctuation remains attached to its original left-to-right path segment. +- Expose the existing `data-review-file-directory` selector on both responsive variants. Preserve the full `title`, collapse/expand accessible label, filename element, toolbar layout, status/stat placement, and click behavior. + +### Mobile design contract + +- **Desktop outcome:** sticky diff headers show exact punctuation while retaining the filename and nearest directory suffix at constrained widths. +- **Mobile entry point:** the existing task bottom navigation opens Changes, and its existing Review action opens the full-height Review surface. +- **Nearest shipped exemplar:** `MobileReviewFileDetails` remains the focused file identity used by `mobile-review-file-status.spec.ts`; its filename-first hierarchy and directory/status second row stay unchanged. +- **Presentation rationale:** this is a text-direction repair inside current dense diff chrome, so no new route, drawer, menu, or touch action is warranted. +- **Geometry:** the Review dialog remains the scroll owner. Dynamic viewport sizing, safe-area handling, 44 px controls, and document overflow behavior remain unchanged. +- **Shared logic:** desktop and mobile use the same directory isolation rule; only their existing responsive composition differs. +- **Mobile proof:** the Pixel 5 Review status scenario uses a long dot-prefixed path and verifies visual character order plus the existing header and document containment contracts. + +## Tests + +- **What:** both responsive header variants retain the exact directory string and isolate it as left-to-right path content. +- **File:** `apps/web/components/review/review-diff-header.test.tsx`. +- **How:** render `.agents/skills/pr-fixup/SKILL.md` in desktop and mobile modes, then assert the directory selector contains an exact `` value while `SKILL.md` remains the separate filename. Add this before the source change and record the expected RED failure. + +## E2E Tests + +- **Shared rendered-order probe:** extend `apps/web/e2e/helpers/layout-assertions.ts` with a single-line text helper that walks descendant text nodes, creates one-character DOM ranges, and returns characters sorted by rendered horizontal position. This tests browser bidi layout rather than DOM `textContent` or screenshot OCR. +- **Desktop scenario:** extend `apps/web/e2e/tests/review/review-file-status.spec.ts` to seed `.agents/skills/pr-fixup/SKILL.md`, open Review, and assert that the directory's rendered character order is exactly `.agents/skills/pr-fixup`, the filename remains `SKILL.md`, and the accessible header label retains the full path. +- **Mobile scenario:** update `apps/web/e2e/tests/review/mobile-review-file-status.spec.ts` to use a long dot-prefixed directory, assert exact rendered character order, and retain its existing sticky-header, touch-target, actions-menu, and no-horizontal-overflow checks. + +## Verification Results + +- RED component proof: `cd apps && pnpm --filter @kandev/web test -- components/review/review-diff-header.test.tsx` failed the two new desktop/mobile cases while the six prior cases passed. Desktop lacked the shared directory hook; mobile lacked the LTR isolate. +- RED browser proof: after a test-only locator refinement, `cd apps/web && pnpm e2e:run --no-build tests/review/review-file-status.spec.ts -- --workers=1 --retries=0` failed with expected `.agents/skills/pr-fixup` and rendered `agents/skills/pr-fixup.` against the unchanged production build. +- GREEN: the component suite passed 8 tests; desktop Chromium passed 1 test; mobile Chromium passed 1 test. The mobile case also proved real truncation (`scrollWidth > clientWidth`), retained RTL leading-edge clipping, 44 px actions, and no page overflow. +- `pnpm run typecheck`, repository web lint, and `git diff --check` passed. Managed E2E runs removed their failure artifacts and owned runtime directories/processes. + +## Implementation Waves And Parallel Candidates + +Sequential single-task repair: + +- [x] [Task 01: Preserve Review path punctuation](task-01-preserve-review-path-punctuation.md) + +No subagent delegation is planned or authorized. Component markup, shared browser assertion, and both responsive regressions form one TDD slice. + +## Risks + +- Removing RTL direction outright would fix punctuation but regress leading-edge truncation and hide the directory suffix nearest the filename. +- DOM text assertions cannot reproduce this defect because the logical string is already correct; browser range geometry is required for regression coverage. +- Mixed strong right-to-left Unicode path content remains outside this repair's visual-order contract. +- Public docs, localization catalogs, APIs, and persisted state are unaffected. diff --git a/docs/plans/review-path-punctuation/task-01-preserve-review-path-punctuation.md b/docs/plans/review-path-punctuation/task-01-preserve-review-path-punctuation.md new file mode 100644 index 0000000000..0c36aecaa1 --- /dev/null +++ b/docs/plans/review-path-punctuation/task-01-preserve-review-path-punctuation.md @@ -0,0 +1,93 @@ +--- +id: "01-preserve-review-path-punctuation" +title: "Preserve Review path punctuation" +status: completed +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/review-file-status.md" +--- + +# Task 01: Preserve Review path punctuation + +## Acceptance + +- Desktop and phone sticky Review headers render dot-prefixed directory segments in their original character order; `.agents/skills/pr-fixup/SKILL.md` never appears as `agents/skills/pr-fixup./SKILL.md` or with the dot beside another segment. +- Constrained headers retain leading-edge directory truncation, the nearest directory suffix, the separate filename, the full title/accessible path, existing status/actions geometry, and zero document-level horizontal overflow. +- Focused component, desktop Chromium, mobile Chromium, typecheck, lint, and diff checks pass with no persistent diagnostic artifacts or processes left behind. + +## Verification + +Install dependencies only if this worktree does not already have them: + +```bash +cd apps && pnpm install --frozen-lockfile +``` + +Run the new component regression before the source change and record its expected failure. Add the desktop rendered-order regression and run it against the production build before the source change; record the expected visual-order failure. After the minimal fix, run: + +```bash +cd apps && pnpm --filter @kandev/web test -- components/review/review-diff-header.test.tsx +cd apps/web && pnpm e2e:run tests/review/review-file-status.spec.ts -- --workers=1 --retries=0 +cd apps/web && pnpm e2e:run --project mobile-chrome tests/review/mobile-review-file-status.spec.ts -- --workers=1 --retries=0 +cd apps/web && pnpm run typecheck +cd apps && pnpm --filter @kandev/web lint +git diff --check +``` + +The managed desktop and mobile E2E commands each rebuild the production backend and Vite assets. Do not treat DOM `textContent` as rendered-order evidence; the bug leaves DOM data correct. + +## Files likely touched + +- `apps/web/components/review/review-diff-header.tsx` +- `apps/web/components/review/review-diff-header.test.tsx` +- `apps/web/e2e/helpers/layout-assertions.ts` +- `apps/web/e2e/tests/review/review-file-status.spec.ts` +- `apps/web/e2e/tests/review/mobile-review-file-status.spec.ts` +- `docs/specs/ui/review-file-status.md` +- `docs/plans/review-path-punctuation/plan.md` +- `docs/plans/review-path-punctuation/task-01-preserve-review-path-punctuation.md` + +## Dependencies + +None. + +## Parallelism + +`sequential` because the component markup and both responsive regressions implement one shared bidi/truncation contract. + +## Inputs + +- `docs/specs/ui/review-file-status.md`, especially the sticky-header path and mobile overflow scenarios. +- `docs/plans/review-path-punctuation/plan.md`, including the confirmed root cause and mobile design contract. +- `apps/web/components/review/AGENTS.md` and `apps/web/AGENTS.md`. +- `/tdd`, `/e2e`, and `/mobile-parity` guidance during implementation. + +## Output contract + +Report RED and GREEN results with exact commands and counts, all changed files, failure/capture artifact paths, cleanup and teardown evidence, public-doc/i18n impact, residual risks, and synchronized task/plan statuses. + +## Results + +- Added one shared directory renderer that preserves the existing RTL truncation wrapper and isolates the literal path inside ``. Desktop and phone keep their existing composition, filename, full title/accessible label, actions, status, and geometry. +- Added desktop/mobile component coverage and a shared Playwright range-geometry probe. Desktop now seeds `.agents/skills/pr-fixup/SKILL.md`; mobile uses a longer dot-prefixed path and proves the directory is genuinely constrained while document overflow remains absent. + +### TDD evidence + +- **Component RED:** `cd apps && pnpm --filter @kandev/web test -- components/review/review-diff-header.test.tsx` exited 1: 2 new cases failed and 6 prior cases passed. Desktop had no directory hook; mobile had no LTR isolate. +- **Browser setup RED:** the first full production command exited 1 because the planned desktop directory hook did not exist yet. Its failure screenshot was `apps/web/e2e/test-results/review-review-file-status--2fac6-th-and-explains-a-pure-move-chromium/test-failed-1.png`; the context was the sibling `error-context.md`. +- **Browser visual RED:** after changing only the test locator, `cd apps/web && pnpm e2e:run --no-build tests/review/review-file-status.spec.ts -- --workers=1 --retries=0` exited 1 with expected `.agents/skills/pr-fixup` and received `agents/skills/pr-fixup.`. This used the unchanged production bundle from the preceding full build. + +### Final verification + +- `cd apps && pnpm --filter @kandev/web test -- components/review/review-diff-header.test.tsx`: 1 file passed, 8 tests passed. +- `cd apps/web && pnpm e2e:run tests/review/review-file-status.spec.ts -- --workers=1 --retries=0`: Chromium, 1 passed. A test-only stable-selector refactor then passed once more with `--no-build`. +- `cd apps/web && pnpm e2e:run --project mobile-chrome tests/review/mobile-review-file-status.spec.ts -- --workers=1 --retries=0`: mobile-chrome, 1 passed. +- `cd apps/web && pnpm run typecheck`: passed. +- `cd apps && pnpm --filter @kandev/web lint`: passed. An earlier run found one `max-lines-per-function` warning in the enlarged test describe; splitting the new cases into their own describe resolved it. +- `git diff --check`: passed. + +### Cleanup and impact + +- Later managed GREEN runs cleared the RED screenshot/context and tore down their owned backends, browser workers, and current-run `/tmp/kandev-e2e-*` directories. Only the ignored successful-run marker `apps/web/e2e/test-results/.last-run.json` remains; older temp directories belong to earlier unrelated runs and were not touched. +- No public docs, locale catalogs, APIs, persistence, or user-facing copy changed. The internal behavioral spec was amended. Strong right-to-left Unicode path content remains explicitly out of scope. diff --git a/docs/specs/ui/review-file-status.md b/docs/specs/ui/review-file-status.md index d0cfaf7f61..e472376078 100644 --- a/docs/specs/ui/review-file-status.md +++ b/docs/specs/ui/review-file-status.md @@ -17,6 +17,8 @@ Reviewers can see changed filenames in Review's file tree, but cannot tell wheth - The marker does not rely on color alone. Its accessible name and native title describe the status; a moved file may include its previous path when available. - The desktop marker occupies a fixed trailing column. At narrow sidebar widths, the filename truncates before the marker, stale warning, or comment count overlap. - Below the desktop breakpoint, where the file tree is hidden, the same status marker appears in each sticky diff header. No required cue depends on hover. +- Desktop and phone sticky diff headers render ordinary left-to-right repository paths without relocating punctuation. A dot-prefixed directory segment keeps its dot attached to that segment instead of moving beside a separator or filename. +- When a sticky diff header cannot show all directory context, it elides the leading directory text while keeping the nearest directory suffix and filename visible. Truncation does not change the full path exposed by the header title or accessible collapse/expand label. - Review includes changed files even when they have no textual patch, including pure renames. Their diff body shows status-specific explanatory text instead of an indefinite loading message. - Existing explicit skip reasons for binary, oversized, truncated, or budget-exceeded diffs take precedence over generic status-specific text. - Existing file icons, tree hierarchy, filtering, review checkboxes, stale indicators, comment counts, source precedence, and multi-repository identity remain unchanged. @@ -30,6 +32,8 @@ Reviewers can see changed filenames in Review's file tree, but cannot tell wheth - **GIVEN** a file with an explicit diff skip reason, **WHEN** its body renders, **THEN** the skip-reason message is shown instead of the generic status empty state. - **GIVEN** a 160 px Review sidebar and a long filename, **WHEN** the row renders, **THEN** the filename truncates while the status marker, stale warning, and comment count remain visible without overlap. - **GIVEN** a mobile viewport, **WHEN** the user reviews a changed file, **THEN** its sticky diff header exposes the status marker without horizontal page overflow. +- **GIVEN** the changed path `.agents/skills/pr-fixup/SKILL.md`, **WHEN** its desktop sticky diff header renders, **THEN** `.agents` remains the first directory segment, `SKILL.md` remains the filename, and no dot is rendered beside another segment or separator. +- **GIVEN** a long changed path with a dot-prefixed directory in a phone viewport, **WHEN** its sticky diff header truncates the directory, **THEN** the visible directory suffix and filename retain their original punctuation order without horizontal page overflow. ## Out of scope @@ -38,3 +42,5 @@ Reviewers can see changed filenames in Review's file tree, but cannot tell wheth - A new mobile file navigator. - Status animations. - Backend or API changes; Review consumes existing git and pull-request status metadata. +- Changing file-path data, sorting, copying, or Git operations. +- Defining a new visual-order policy for paths containing strong right-to-left Unicode characters.