chore: organize dirty work from codex/chat-top-nav-mockups-b3ce - #1323
chore: organize dirty work from codex/chat-top-nav-mockups-b3ce#1323BigSimmo wants to merge 1 commit into
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdds a document top-navigation mockup with responsive concepts and search interactions, updates document viewer search and section anchors, and expands Playwright coverage for navigation, focus, responsive layout, and request behavior. ChangesDocument navigation and search
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant DocumentViewer
participant DocumentSearchAPI
participant AnswerStreamAPI
Browser->>DocumentViewer: Open document search
DocumentViewer->>DocumentViewer: Focus search input
Browser->>DocumentViewer: Submit query
DocumentViewer->>DocumentSearchAPI: Search within document
DocumentSearchAPI-->>DocumentViewer: Return indexed source results
Browser->>DocumentViewer: Select Answer from this
DocumentViewer->>AnswerStreamAPI: Request document summary
AnswerStreamAPI-->>DocumentViewer: Stream summary response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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: 2
🧹 Nitpick comments (2)
src/components/document-top-navigation-mockups.tsx (1)
228-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrdinal prefix isn't hidden from the accessible name/text content.
The "index" style renders
String(index+1).padStart(2,"0")andsection.labelas sibling text nodes with nothing to separate them for assistive tech (e.g. screen readers will read something like "01Summary" as one word/name). Marking the ordinal spanaria-hidden="true"(like the selected-state underline span already does) keeps it purely decorative.Note this alone won't fix the companion Playwright
toHaveTextassertion intests/ui-document-top-navigation-mockup.spec.ts(raw DOMtextContentstill includes the digits regardless ofaria-hidden); see the related comment on that test file.♻️ Suggested tweak
<span className="nums text-[10px] font-bold tracking-[0.08em] opacity-60"> + <span aria-hidden="true"> {String(index + 1).padStart(2, "0")} + </span> </span>🤖 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 `@src/components/document-top-navigation-mockups.tsx` around lines 228 - 256, Mark the ordinal prefix span in the sections map within the document top navigation as aria-hidden="true", leaving section.label exposed as the button’s accessible name. Do not alter the selected-state underline or the rendered text content.src/components/DocumentViewer.tsx (1)
875-903: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead branch left behind after fixing the summarize query.
Since
queryis now unconditionallydocumentSummaryQuestion(previously it could besourceSearch),summaryQuerystate can never diverge fromdocumentSummaryQuestionagain. As a result,generatedAnswerIsSummary(Line 1005) is now alwaystrue, making the"Answer from this document"/"Grounded in indexed passages from this source."branch of the ternary at Lines 1308-1313 permanently unreachable. Consider removingsummaryQuery/generatedAnswerIsSummaryand hard-coding the "Clinical summary" copy, or documenting why the state/ternary is retained for future flexibility.🤖 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 `@src/components/DocumentViewer.tsx` around lines 875 - 903, Remove the now-redundant summaryQuery state and generatedAnswerIsSummary logic, then simplify the related UI ternary to always use the “Clinical summary” copy because summarize requests unconditionally use documentSummaryQuestion. Update the associated setter and state references in the summary flow and rendering code while preserving the existing summary behavior.
🤖 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 `@src/components/DocumentViewer.tsx`:
- Around line 1129-1139: Update the document search button near
focusDocumentSearch so aria-controls is only present when readyDocument is
truthy; otherwise omit the attribute while the search input is not rendered.
Preserve the existing button disabled behavior and document-viewer-search-input
ID for ready documents.
In `@tests/ui-document-top-navigation-mockup.spec.ts`:
- Around line 23-29: Update the section text assertion in
expectNavigationContract to match each button’s label suffix rather than
requiring exact per-element text. Map expectedSections to regular expressions
ending with each label so indexed buttons such as “01Summary” and “02PDF” are
accepted, while preserving the existing navigation and aria-current assertions.
---
Nitpick comments:
In `@src/components/document-top-navigation-mockups.tsx`:
- Around line 228-256: Mark the ordinal prefix span in the sections map within
the document top navigation as aria-hidden="true", leaving section.label exposed
as the button’s accessible name. Do not alter the selected-state underline or
the rendered text content.
In `@src/components/DocumentViewer.tsx`:
- Around line 875-903: Remove the now-redundant summaryQuery state and
generatedAnswerIsSummary logic, then simplify the related UI ternary to always
use the “Clinical summary” copy because summarize requests unconditionally use
documentSummaryQuestion. Update the associated setter and state references in
the summary flow and rendering code while preserving the existing summary
behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 928ff4cd-09d0-4d35-9ab3-08948a0a5400
📒 Files selected for processing (8)
docs/site-map.mdsrc/app/mockups/document-top-navigation/page.tsxsrc/app/mockups/mockups-layout-client.tsxsrc/components/DocumentViewer.tsxsrc/components/document-top-navigation-mockups.tsxsrc/components/document-viewer/source-panels.tsxtests/ui-document-top-navigation-mockup.spec.tstests/ui-smoke.spec.ts
| <button | ||
| type="button" | ||
| onClick={focusDocumentSearch} | ||
| disabled={!readyDocument} | ||
| title={readyDocument ? "Search within this document" : "Load a source document before searching"} | ||
| className="grid h-tap w-tap place-items-center rounded-full text-[color:var(--text-muted)] hover:bg-[color:var(--surface-subtle)] hover:text-[color:var(--text)] disabled:cursor-not-allowed disabled:opacity-40" | ||
| aria-label="Go to document search" | ||
| aria-controls="document-viewer-search-input" | ||
| > | ||
| <Search aria-hidden="true" className="h-5 w-5" /> | ||
| </button> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
aria-controls can reference a nonexistent element.
The composer <form>/input only render when readyDocument is truthy (Line 1708), but this button unconditionally sets aria-controls="document-viewer-search-input" even while disabled (!readyDocument). That leaves aria-controls pointing at an ID that doesn't exist in the DOM while the document is loading/errored.
🛡️ Suggested fix
aria-label="Go to document search"
- aria-controls="document-viewer-search-input"
+ aria-controls={readyDocument ? "document-viewer-search-input" : undefined}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| type="button" | |
| onClick={focusDocumentSearch} | |
| disabled={!readyDocument} | |
| title={readyDocument ? "Search within this document" : "Load a source document before searching"} | |
| className="grid h-tap w-tap place-items-center rounded-full text-[color:var(--text-muted)] hover:bg-[color:var(--surface-subtle)] hover:text-[color:var(--text)] disabled:cursor-not-allowed disabled:opacity-40" | |
| aria-label="Go to document search" | |
| aria-controls="document-viewer-search-input" | |
| > | |
| <Search aria-hidden="true" className="h-5 w-5" /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={focusDocumentSearch} | |
| disabled={!readyDocument} | |
| title={readyDocument ? "Search within this document" : "Load a source document before searching"} | |
| className="grid h-tap w-tap place-items-center rounded-full text-[color:var(--text-muted)] hover:bg-[color:var(--surface-subtle)] hover:text-[color:var(--text)] disabled:cursor-not-allowed disabled:opacity-40" | |
| aria-label="Go to document search" | |
| aria-controls={readyDocument ? "document-viewer-search-input" : undefined} | |
| > | |
| <Search aria-hidden="true" className="h-5 w-5" /> | |
| </button> |
🤖 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 `@src/components/DocumentViewer.tsx` around lines 1129 - 1139, Update the
document search button near focusDocumentSearch so aria-controls is only present
when readyDocument is truthy; otherwise omit the attribute while the search
input is not rendered. Preserve the existing button disabled behavior and
document-viewer-search-input ID for ready documents.
| async function expectNavigationContract(preview: Locator) { | ||
| const navigation = preview.getByRole("navigation", { name: "Document page sections" }); | ||
| await expect(navigation.getByRole("button")).toHaveText(expectedSections); | ||
| await expect(navigation.getByRole("button", { name: "Overview" })).toHaveCount(0); | ||
| await expect(navigation.getByRole("button", { name: "Text" })).toHaveCount(0); | ||
| await expect(navigation.getByRole("button", { name: "Summary" })).toHaveAttribute("aria-current", "page"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Playwright toHaveText exact match textContent vs innerText for multiple elements array
💡 Result:
In Playwright, the toHaveText assertion used with an array of values performs an exact match for each corresponding element in the locator's result [1]. When comparing text, Playwright normalizes whitespace (removing line breaks and multiple spaces) for both the actual and expected values by default [2][1]. The choice between textContent and innerText (via the useInnerText option) determines how the text is retrieved from the DOM [3][4]: 1. textContent (Default): This retrieves the text content of the element and all of its descendants, including elements that are hidden from the user (e.g., via display: none) [4][5]. 2. useInnerText: true: When this option is enabled, Playwright uses element.innerText, which retrieves the "rendered" text of an element as it would appear to a user [4][5]. This typically excludes hidden elements [5]. For assertions involving multiple elements in an array, the expectation is that the locator resolves to a list of elements, and each element's retrieved text (via either textContent or innerText) must exactly match the corresponding expected value in the array, in the specified order [1]. Summary Table Option | Behavior | Best Used For --- | --- | --- Default (textContent) | Retrieves text including hidden elements [5]. | General purpose content verification. useInnerText: true | Retrieves only rendered, visible text [4][5]. | Avoiding flakiness when hidden elements are present [6]. If you encounter issues where hidden elements cause assertions to fail, enabling { useInnerText: true } is the recommended approach to ensure your test focuses on what is actually visible to the user [4][6].
Citations:
- 1: https://playwright.dev/docs/api/class-locatorassertions
- 2: https://github.com/microsoft/playwright/blob/303901d7/packages/playwright/src/matchers/matchers.ts
- 3: https://github.com/microsoft/playwright/blob/main/docs/src/api/class-locatorassertions.md
- 4: https://runebook.dev/en/docs/playwright/api/class-locatorassertions/locator-assertions-to-contain-text
- 5: https://runebook.dev/en/docs/playwright/api/class-page/page-inner-text
- 6: https://playwright.dev/docs/api/class-locator
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- tests/ui-document-top-navigation-mockup.spec.ts ---\n'
sed -n '1,220p' tests/ui-document-top-navigation-mockup.spec.ts
printf '\n--- locate document-top-navigation-mockup.tsx ---\n'
fd -a 'document-top-navigation-mockup' .
printf '\n--- search for expectedSections and index rendering ---\n'
rg -n "expectedSections|padStart\\(2, \"0\"\\)|aria-current|Document page sections|Overview|Summary|Text" tests src app .Repository: BigSimmo/Database
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/ui-document-top-navigation-mockup.spec.ts ---'
sed -n '1,220p' tests/ui-document-top-navigation-mockup.spec.ts
echo
echo '--- locate document-top-navigation-mockup.tsx ---'
fd -a 'document-top-navigation-mockup' .
echo
echo '--- search for expectedSections and index rendering ---'
rg -n "expectedSections|padStart\\(2, \"0\"\\)|aria-current|Document page sections|Overview|Summary|Text" tests src app .Repository: BigSimmo/Database
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- tests/ui-document-top-navigation-mockup.spec.ts ---'
if [ -f tests/ui-document-top-navigation-mockup.spec.ts ]; then
cat -n tests/ui-document-top-navigation-mockup.spec.ts
else
echo 'missing'
fi
echo
echo '--- matching mockup files ---'
fd -a 'document-top-navigation' src tests .
echo
echo '--- relevant excerpts from likely component files ---'
for f in $(fd -a 'document-top-navigation' src tests .); do
echo "### $f"
sed -n '1,260p' "$f" | rg -n "expectedSections|padStart\\(2, \"0\"\\)|aria-current|navigation|Overview|Summary|Text|PDF|button"
doneRepository: BigSimmo/Database
Length of output: 4504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/components/document-top-navigation-mockups.tsx (outline) ---'
ast-grep outline src/components/document-top-navigation-mockups.tsx --view expanded || true
echo
echo '--- src/components/document-top-navigation-mockups.tsx relevant lines ---'
sed -n '1,260p' src/components/document-top-navigation-mockups.tsx
echo
echo '--- app mockup route files ---'
fd -a 'document-top-navigation' src/app/mockups
for f in $(fd -a 'document-top-navigation' src/app/mockups); do
echo "### $f"
sed -n '1,220p' "$f"
doneRepository: BigSimmo/Database
Length of output: 10887
Adjust the section text assertion for the index layout
toHaveText(expectedSections) is exact per element, but the index direction renders the number and label in the same button text (01Summary, 02PDF, ...). That makes this contract fail for the indexed previews; match the label suffix instead, e.g. expectedSections.map((label) => new RegExp()).
🤖 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 `@tests/ui-document-top-navigation-mockup.spec.ts` around lines 23 - 29, Update
the section text assertion in expectNavigationContract to match each button’s
label suffix rather than requiring exact per-element text. Map expectedSections
to regular expressions ending with each label so indexed buttons such as
“01Summary” and “02PDF” are accepted, while preserving the existing navigation
and aria-current assertions.
|
Closing as superseded — mockups already shipped. Document top-navigation mockups landed on |
1 participant
Organized dirty work from codex/chat-top-nav-mockups-b3ce
RAG impact: no retrieval behaviour change — branch cleanup and organisation only.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes