- Notifications
You must be signed in to change notification settings - Fork 0
Document viewer: compact indexed-source UI, 3-button action layout, and toggle/search fixes#1818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e7a204f
Perfect document source panel layout
BigSimmo a5301fb
Merge branch 'main' into codex/optimize-layout-and-fix-toggle-bug
BigSimmo 6e48bec
Reset active document search hit on query change
BigSimmo 986ebcf
Merge branch 'main' into codex/optimize-layout-and-fix-toggle-bug
BigSimmo 2969a7d
Merge branch 'main' into codex/optimize-layout-and-fix-toggle-bug
BigSimmo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -32,8 +32,6 @@ import { | ||
| cn, | ||
| codeText, | ||
| eyebrowText, | ||
| fieldControl, | ||
| fieldLabel, | ||
| floatingControl, | ||
| LoadingPanel, | ||
| panel, | ||
| @@ -737,36 +735,53 @@ export function PinnedSourceEvidence({ | ||
| <p className="mt-2 text-sm font-semibold text-[color:var(--text)]">{chunk.section_heading}</p> | ||
| )} | ||
| <blockquote | ||
| id={chunk.id ? `cited-passage-${chunk.id}` : undefined} | ||
| className={cn( | ||
| "mt-2 rounded-lg bg-[color:var(--surface-inset)] px-3 py-2.5 text-[color:var(--text)]", | ||
| showingPreview ? "line-clamp-3 whitespace-normal" : "whitespace-pre-line", | ||
| "mt-2 rounded-lg bg-[color:var(--surface-inset)] px-3 py-2 text-[color:var(--text)]", | ||
| showingPreview ? "line-clamp-2 whitespace-normal" : "whitespace-pre-line", | ||
| )} | ||
| > | ||
| {visibleContent || "No displayable clinical text was available for this indexed passage."} | ||
| </blockquote> | ||
| <div className="mt-3 flex flex-wrap gap-2"> | ||
| <a href="#pdf-preview-section" className={cn(primaryButton, "sm:min-h-9 px-3 text-xs")}> | ||
| <div className="mt-3 grid grid-cols-3 gap-1.5 sm:gap-2"> | ||
| <a | ||
| href="#pdf-preview-section" | ||
| className={cn( | ||
| primaryButton, | ||
| "min-w-0 justify-center px-1.5 text-center text-2xs sm:min-h-9 sm:px-3 sm:text-xs", | ||
| )} | ||
| > | ||
| <ExternalLink aria-hidden="true" className="h-4 w-4" /> | ||
| View in PDF | ||
| <span>View PDF</span> | ||
| </a> | ||
| {compact && isLong ? ( | ||
| <button | ||
| type="button" | ||
| onClick={() => setExpandedChunkId((current) => (current === chunk.id ? null : chunk.id))} | ||
| className={cn(secondaryButton, "sm:min-h-9 px-3 text-xs")} | ||
| className={cn( | ||
| secondaryButton, | ||
| "min-w-0 justify-center px-1.5 text-center text-2xs sm:min-h-9 sm:px-3 sm:text-xs", | ||
| )} | ||
| data-testid="toggle-full-passage" | ||
| aria-expanded={expanded} | ||
| aria-controls={chunk.id ? `cited-passage-${chunk.id}` : undefined} | ||
| > | ||
| {expanded ? "Show passage preview" : "Show full passage"} | ||
| {expanded ? "Collapse" : "Full passage"} | ||
| </button> | ||
| ) : null} | ||
| ) : ( | ||
| <span aria-hidden="true" /> | ||
| )} | ||
| {onInspectIndexedText ? ( | ||
| <button | ||
| type="button" | ||
| onClick={onInspectIndexedText} | ||
| className={cn(secondaryButton, "sm:min-h-9 px-3 text-xs")} | ||
| className={cn( | ||
| secondaryButton, | ||
| "min-w-0 justify-center px-1.5 text-center text-2xs sm:min-h-9 sm:px-3 sm:text-xs", | ||
| )} | ||
| data-testid="inspect-indexed-text" | ||
| > | ||
| Inspect indexed text | ||
| Indexed text | ||
| </button> | ||
| ) : null} | ||
| </div> | ||
| @@ -910,7 +925,7 @@ function HighlightedSearchText({ text, terms }: { text: string; terms: string[] | ||
| // Memoised: both the mobile <details> and desktop copies stay mounted and are | ||
| // CSS-toggled, so without this every unrelated parent re-render (e.g. composer | ||
| // typing) re-rendered both instances. All props are referentially stable across | ||
| // those renders (onSearchChange is a stable setState), so memo actually elides them. | ||
| // those renders, so memo actually elides them. | ||
| export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| loading, | ||
| selectedPage, | ||
| @@ -922,7 +937,6 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| idPrefix, | ||
| sectionId, | ||
| selectedChunkId, | ||
| onSearchChange, | ||
| compact = false, | ||
| revealRequest = false, | ||
| }: { | ||
| @@ -936,7 +950,8 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| idPrefix: string; | ||
| sectionId?: "source-text"; | ||
| selectedChunkId?: string; | ||
| onSearchChange: (value: string) => void; | ||
| /** Retained for call-site compatibility; search input is owned by the document composer. */ | ||
| onSearchChange?: (value: string) => void; | ||
| compact?: boolean; | ||
| /** | ||
| * Explicit user intent to open the panel (e.g. "Inspect indexed text"). | ||
| @@ -1002,19 +1017,26 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| // lost across re-renders and left deep-linked hits collapsed in Production UI. | ||
| const [manualClosedDriver, setManualClosedDriver] = useState<string | null>(null); | ||
| const [compactOpen, setCompactOpen] = useState(false); | ||
| const previousSearchRef = useRef(""); | ||
| // In-document search and an explicit "Inspect indexed text" action keep the | ||
| // panel revealed through exclusive-accordion closes. Citation deep-links alone | ||
| // must not force-open — that stole the first viewport from the PDF. | ||
| const forceReveal = Boolean(normalizedSearch) || revealRequest; | ||
| const [prevForceReveal, setPrevForceReveal] = useState(forceReveal); | ||
| if (forceReveal !== prevForceReveal) { | ||
| setPrevForceReveal(forceReveal); | ||
| const previousForceRevealRef = useRef(forceReveal); | ||
| useEffect(() => { | ||
| if (forceReveal === previousForceRevealRef.current) return; | ||
| previousForceRevealRef.current = forceReveal; | ||
| // Rising edge: latch open so exclusive-accordion closes cannot collapse an | ||
| // active inspect/search reveal. Falling edge: drop the latch so jumping to | ||
| // PDF/overview (or clearing revealRequest on the same citation) restores | ||
| // PDF-first instead of leaving the dump controlled-open. | ||
| setCompactOpen(forceReveal); | ||
| } | ||
| }, [forceReveal]); | ||
| useEffect(() => { | ||
| if (previousSearchRef.current === normalizedSearch) return; | ||
| previousSearchRef.current = normalizedSearch; | ||
| setActiveHitIndex(0); | ||
| }, [normalizedSearch]); | ||
| if (previousAutoOpenDriverRef.current !== autoOpenDriver) { | ||
| previousAutoOpenDriverRef.current = autoOpenDriver; | ||
| if (manualClosedDriver !== null) setManualClosedDriver(null); | ||
| @@ -1097,31 +1119,19 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| : `Extracted text for page ${selectedPage?.page_number ?? "n/a"} with searchable source passages.` | ||
| } | ||
| /> | ||
| <div className={cn(clinicalDivider, "p-5 pt-4")}> | ||
| <label className="block"> | ||
| <span className={fieldLabel}>Search within indexed source text</span> | ||
| <input | ||
| value={search} | ||
| onChange={(event) => { | ||
| setActiveHitIndex(0); | ||
| onSearchChange(event.target.value); | ||
| }} | ||
| placeholder="Find a term, warning, or monitoring item" | ||
| className={fieldControl} | ||
| /> | ||
| </label> | ||
| <div className={cn(clinicalDivider, "px-4 pb-4 pt-3 sm:p-5 sm:pt-4")}> | ||
BigSimmo marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| {loading ? ( | ||
| <LoadingPanel label="Loading indexed source text" /> | ||
| ) : ( | ||
| <div className="mt-4 grid gap-3"> | ||
| <div className="grid gap-2.5"> | ||
| <details | ||
| data-source-nested-disclosure | ||
| data-testid="indexed-page-text-disclosure" | ||
| className={cn(sourceCard, "group/source-row overflow-hidden p-0 source-print")} | ||
| > | ||
| <summary | ||
| onClick={handleNestedSummaryClick} | ||
| className="flex min-h-tap cursor-pointer list-none items-center justify-between gap-3 px-3 py-3 text-left focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)] sm:min-h-9" | ||
| className="flex min-h-tap cursor-pointer list-none items-center justify-between gap-3 px-3 py-2 text-left focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)] sm:min-h-9" | ||
| > | ||
| <span> | ||
| <span className="block text-sm font-semibold text-[color:var(--text)]">Full extracted page text</span> | ||
BigSimmo marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| @@ -1150,7 +1160,7 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| </details> | ||
| </div> | ||
| )} | ||
| <div className={cn("mt-4 pt-4", clinicalDivider)}> | ||
| <div className={cn("mt-3 pt-3", clinicalDivider)}> | ||
| <div className="flex flex-wrap items-center justify-between gap-2"> | ||
| <p className="text-sm font-semibold text-[color:var(--text)]">Source passages</p> | ||
| {searchEligible ? ( | ||
| @@ -1198,7 +1208,7 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| {documentSearchError} | ||
| </p> | ||
| ) : null} | ||
| <div className="mt-3 grid gap-3"> | ||
| <div className="mt-2.5 grid gap-2.5"> | ||
| {normalizedSearch.length === 1 ? ( | ||
| <p className={cn("text-base-minus leading-6", textMuted)}> | ||
| Enter at least 2 characters to search all indexed passages. | ||
| @@ -1239,7 +1249,7 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| > | ||
| <summary | ||
| onClick={handleNestedSummaryClick} | ||
| className="flex min-h-tap cursor-pointer list-none items-start justify-between gap-3 px-3 py-3 text-left focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)] sm:min-h-9" | ||
| className="flex min-h-tap cursor-pointer list-none items-start justify-between gap-2 px-3 py-2 text-left focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)] sm:min-h-9" | ||
| > | ||
| <span className="min-w-0"> | ||
| <span | ||
| @@ -1254,7 +1264,7 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| > | ||
| {status} | ||
| </span> | ||
| <span className={cn("mt-2 block", eyebrowText)}> | ||
| <span className={cn("mt-1.5 block", eyebrowText)}> | ||
| Page {chunk.page_number ?? "n/a"} · chunk {chunk.chunk_index} | ||
| {chunk.serverRanked ? " · full-document search" : ""} | ||
| </span> | ||
| @@ -1263,7 +1273,7 @@ export const IndexedTextPanel = memo(function IndexedTextPanel({ | ||
| {chunk.section_heading} | ||
| </span> | ||
| ) : null} | ||
| <span className={cn("mt-1 line-clamp-2 block text-sm leading-5", textMuted)}> | ||
| <span className={cn("mt-1 line-clamp-1 block text-xs leading-5", textMuted)}> | ||
| {teaser || "No displayable clinical text was available for this indexed passage."} | ||
| </span> | ||
| </span> | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -154,6 +154,65 @@ describe("IndexedTextPanel citation landing", () => { | ||
| expect(screen.getByText("Hit 1 of 1")).toBeVisible(); | ||
| }); | ||
| it("resets the active hit index when the search query changes", async () => { | ||
| const props = { | ||
| loading: false, | ||
| selectedPage: basePage, | ||
| chunks: [ | ||
| baseChunk, | ||
| { | ||
| ...baseChunk, | ||
| id: "chunk-2", | ||
| chunk_index: 1, | ||
| content: "Lithium levels are checked 5 to 7 days after initiation", | ||
| }, | ||
| ], | ||
| search: "vom", | ||
| documentSearchResults: [ | ||
| { | ||
| id: "chunk-1", | ||
| page_number: 1, | ||
| chunk_index: 0, | ||
| section_heading: "Monitoring", | ||
| snippet: "Escalate review when there is vomiting", | ||
| matched_terms: ["vom"], | ||
| image_ids: [], | ||
| score: 1, | ||
| }, | ||
| { | ||
| id: "chunk-2", | ||
| page_number: 1, | ||
| chunk_index: 1, | ||
| section_heading: "Monitoring", | ||
| snippet: "Lithium levels are checked 5 to 7 days after initiation", | ||
| matched_terms: ["vom"], | ||
| image_ids: [], | ||
| score: 1, | ||
| }, | ||
| ], | ||
| searchingDocument: false, | ||
| documentSearchError: null, | ||
| idPrefix: "source-chunk", | ||
| sectionId: "source-text" as const, | ||
| onSearchChange: vi.fn(), | ||
| compact: true, | ||
| }; | ||
| const { rerender } = render(<IndexedTextPanel {...props} />); | ||
| expect(screen.getByText("Hit 1 of 2")).toBeVisible(); | ||
| fireEvent.click(screen.getByRole("button", { name: "Next document search hit" })); | ||
| expect(screen.getByText("Hit 2 of 2")).toBeVisible(); | ||
| rerender( | ||
| <IndexedTextPanel | ||
| {...props} | ||
| search="head" | ||
| documentSearchResults={[props.documentSearchResults[1], props.documentSearchResults[0]]} | ||
| />, | ||
BigSimmo marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| ); | ||
| expect(screen.getByText("Hit 1 of 2")).toBeVisible(); | ||
| }); | ||
| it("keeps the deep-linked nested chunk disclosure open under an inspect reveal", async () => { | ||
| const props = { | ||
| loading: false, | ||
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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.