Repository navigation
[US-1934] Task-first CLI guide in topic pages, README refresh, matching help text and scroll reset - #28
Conversation
…lp text and scroll fixes - docs/cli-usage.md reshaped around common tasks and a quick reference, with command detail in docs/cli/ topic pages - README and CONTRIBUTING match go.mod and CI, new screenshots, release step checks the guide against --help - --help and per-command usage lines built from one synopsis per command, guarded by tests - Detail panel views start at the top on a new selection - tests/open-source-docs checks guide coverage, links, CI example and exit codes against the binary
…iew fallback, review cleanups
unidoc-alip
left a comment
unidoc-alip
left a comment
There was a problem hiding this comment.
Verdict: COMMENT - 0 must-fix, 1 should-fix, 1 nit
This PR rewrites docs/cli-usage.md as a task-first entry page with six topic pages under docs/cli/. It builds every help line and usage line from one commandSpecs table in cmd/cli/usage.go, adds a stderr warning to dump image --json for the preview fallback, refreshes README and CONTRIBUTING, and keys the GUI detail block by tab and node so its scroll containers start at the top on a new selection.
The CLI and docs side holds up. Every flag registered in cmd/cli appears in its command's synopsis, and no synopsis lists a flag the parser lacks. About 25 exit codes, stderr shapes and flag interactions from the guide were spot-checked against a fresh build on testdata/ fixtures, and all matched. That covers --help exit codes, --page 0, --raw --json, --ref with --name, unknown profile, encrypted file under each profile, page N not found, page has no content stream and the obj:G:N ref form. tests/open-source-docs extracts the guide's CI script and runs it on a failing, a passing and a missing file.
The one should-fix is in the GUI change. The key sits on the outer detail block, which also holds the Back/Forward buttons, so a keyboard user loses focus on every Back or Forward. Moving the key onto the views alone fixes it and keeps the scroll reset.
Findings are keyed F1, F2, ... so they don't collide with GitHub's #N issue links.
F1 [should-fix] frontend/src/components/DetailPanel.tsx line 913
The new key={${detailTabId}:${detail.nodeId}} is on the outer <div className="h-full flex flex-col">. That div holds the header with the Back/Forward buttons (nav-back-button, nav-forward-button), the Object FindBar, and the views. So everything in it remounts when the new detail lands, not only the scroll containers.
On dev the header stayed mounted across a navigation: the fetch effect keeps the previous detail on screen until the new one resolves (// Keep previous detail/contentStream visible until the new fetch resolves), so selectedNodeId && detail never went false in between. A keyboard user could Tab to Back and press Enter repeatedly to walk back through history. On head, the button they pressed is removed from the DOM when the new detail arrives, and focus drops to document.body. The next Enter does nothing until the user focuses the button again.
Mouse clicks and Cmd/Ctrl+[ / Cmd/Ctrl+] (a window-level handler) do not depend on focus surviving, so nothing is visible there, and a mouse-driven manual check in a production build will not show this. The file already handles the same concern elsewhere: handleObjectFindClose moves focus explicitly "so a keyboard user does not land on document.body". The Object find bar is also inside the keyed div but is not affected: objectFindable is false during the fetch window, so the bar cannot be open when the remount happens.
PoC-confirmed with a Vitest test that reuses the DetailPanel.scrollReset.test.tsx harness verbatim (Appendix A1). It focuses Back or Forward, presses Enter through user-event, waits for the new detail, and asserts one joined string. Run against three versions of DetailPanel.tsx, together with the PR's own three scroll tests:
base (dev b5aa454) scroll tests 0/3 pass | focus tests 2/2 pass
head (84f44cd) scroll tests 3/3 pass | focus tests 0/2 pass
got: "focused=body | shown=/FirstKey", want "focused=nav-back-button | shown=/FirstKey"
got: "focused=body | shown=/SecondKey", want "focused=nav-forward-button | shown=/SecondKey"
fixed (below) scroll tests 3/3 pass | focus tests 2/2 pass
With the fix applied, the full frontend suite passes (86 files, 1333 tests: the PR's 1331 plus the two above), and npm run typecheck and eslint are clean.
Suggested fix: move the key to a Fragment around the views only. A Fragment adds no DOM node, so the flex layout is unchanged; the header and find bar stay mounted.
import { Fragment, useState, useEffect, useCallback, useMemo, useRef, memo } from 'react'; {selectedNodeId && detail && (
<div className="h-full flex flex-col">
{/* header (Back/Forward) and FindBar unchanged, outside the key */}
...
{/* Keyed by document tab and node so the views below remount at the top on a new selection; the header and find bar stay mounted and keep focus. */}
<Fragment key={`${detailTabId}:${detail.nodeId}`}>
{detail.type === 'dict' && selectedNodeIconHint === 'font' && (
...
{reverseRefsVisible && reverseRefsLoaded && selectedNodeId && (
<ReverseRefsSection ... />
)}
</Fragment>
</div>
)}Caveat: jsdom does no layout and models focus only; the claim that focus lands on body when the focused node is removed is standard DOM behaviour.
F2 [nit] cmd/cli/cmd_image.go line 90
The new warning branch runs only if GetImageBytes fails right after GetImageData succeeded with a non-empty Base64. Both call the same renderImage on the same open document, under the same pdfMu lock pattern, with no cancellable context on the second call (context.Background()). renderImage has no state that could change between the two calls. So the branch is reached only if a deterministic render disagrees with itself; no fixture or test reaches it.
The warning is harmless. But the guide spends a sentence on it in two places (docs/cli-usage.md "Pull out one image or stream" and docs/cli/images-and-fonts.md "dump image"), and the CHANGELOG lists it under Fixed, as if a caller could hit the fallback. Either drop the doc sentences and the CHANGELOG line, or keep the branch as a defensive guard and say nothing user-facing about it. The project's CLAUDE.md asks for no error handling for scenarios that cannot happen; if the branch stays, a comment saying it guards an unreachable case would be accurate.
Source-read only (internal/pdfcore/image.go GetImageData, GetImageBytes, renderImage).
PR description verification
- "
docs/cli-usage.mdis now the entry page ... six topic pages underdocs/cli/" - confirmed. Contents, four common tasks, quick-reference table, shape of a command and flags are all there; every table link resolves to a###heading on its page (also enforced bytests/open-source-docs). - "The CI script fails the job on exit 2, on errors, warnings and info in
summary... and on a missing file. ... pipingvalidateinto plainjqpasses when the file is missing" - confirmed.TestCLIUsageGuideCIExampleFailsOnFindingsAndOperationalErrorsruns the snippet from the guide undershonuntagged.pdf,tagged.pdfand a missing path.validate --json ... missing.pdf | jq .summaryexits 0 here.ValidationSummaryhas noomitempty, sosummary.info == 0does not misfire on a clean report. - "
printUsageand every command's usage line are built from it, so--helpand the usage line print the same flags. ... No flag or exit code changed" - confirmed. The diff changes only strings and comments in the exit-code paths, and nofs.Bool/Int/Stringregistration. A grep of every flag registration incmd/climatchescommandSpecsexactly, includingdump embeddedtaking no--pretty. Package-levelvar xUsage = usageLine(...)is safe: Go orders the init ofcommandSpecsfirst through the function reference. Thedump plaintextalias passes the literal"bytes", so it cannot hit theusageLinepanic. - "
cmd/cli/usage_flags_test.gochecks ... that each command accepts exactly the flags it lists" - partially confirmed. The probe set is the union of flags named in some synopsis, so a flag a command accepts but no synopsis names anywhere would pass unseen. None exists today (the grep above closes that), but the test alone would not catch one. - "
dump image --jsonwrites a JSON warning to stderr when it cannot produce the full-resolution image" - partially confirmed. The code does this, but the case is effectively unreachable; see F2. - README: "Go 1.27, Wails beta.18,
task package" - confirmed againstgo.mod(go 1.27.0,wails/v3 v3.0.0-beta.18),ci.yml(go-version: '1.27.x',wails3@v3.0.0-beta.18) and thepackagetask inTaskfile.yml. The GUI paragraph's shortcuts exist: Cmd/Ctrl+G and Cmd/Ctrl+K as menu accelerators inmain.go, Cmd/Ctrl+1/2/3 covered byMainLayout.leftRail.test.tsx. - CONTRIBUTING: "golangci-lint was v2.1.6, CI uses v2.13.2" - confirmed (
ci.ymlinstallsgolangci-lint/v2/cmd/golangci-lint@v2.13.2). - "the content-stream, dictionary and array views remount at the top on every new selection, including when switching between two tabs with the same node selected" - confirmed, by the PR's scroll tests failing on
devand passing on head. The remount reaches further than the views, though; see F1. - "Keying the detail block by node remounts it on every selection ... The find bar already closes on a new selection, and the rest of that state lives in
DetailPanel" - partially confirmed. No child view (FontPreview,FontRosterPreview,ImagePreview,ContentStreamViewer, theDetailSharedviews) holds auseState, anduseSpanFindcloses the bar on theresetKeychange. What the description leaves out is DOM focus, which is lost along with the header buttons (F1). - "8 of the 18 help lines run past the description column (7 already did)" - not confirmed. On head, 11 of the 18
name + synopsisstrings are longer thanhelpSynopsisWidth(48), so their summary does not start on the column:dump tree,dump object,dump stream,dump page,dump font,dump image,dump source,dump reverserefs,dump embedded,validate,diff. Wording only; nothing depends on the count. - "The help-text tests failed on 13 of 18 commands ... and on 9 flag cases ... before the fix" - not verified. Not re-run against
dev. - "
scripts/test-all.sh... Vitest (1331 passed). All suites passed" - partially confirmed. Re-rango test ./cmd/cli/andtests/open-source-docs(both pass) and the full Vitest suite with the F1 fix applied (1331 + 2).scripts/test-all.shas a whole was not re-run. The threebuild-and-testCI jobs pass.tests/open-source-docsruns on ubuntu and macOS only, since it is not inwin_subset, so the CI-script test's Windows skip never comes up in CI. - "The scroll reset is covered by Vitest only so far; it still needs a manual check in a production build" - noted. That check should include a keyboard pass over Back/Forward; a mouse pass will not surface F1.
Appendix
A1 frontend/src/components/DetailPanel.navFocus.poc.test.tsx
Drop it beside DetailPanel.scrollReset.test.tsx and run cd frontend && npx vitest run src/components/DetailPanel.navFocus.poc.test.tsx src/components/DetailPanel.scrollReset.test.tsx. Everything from the vi.mock calls down to renderPanel is copied unchanged from the scroll-reset test.
/**
* PoC for the PR 28 review (finding F1): keyboard focus on the detail panel's
* Back/Forward buttons across a navigation.
*
* Harness copied from DetailPanel.scrollReset.test.tsx (commit 84f44cd): same
* binding mocks, AppProvider and SELECT_NODE dispatch. Only the assertions are new.
*
* Run: cd frontend && npx vitest run src/components/DetailPanel.navFocus.poc.test.tsx
*/
import { render, screen, waitFor, act } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { describe, test, expect, vi, beforeEach } from 'vitest';
import {
AppProvider,
useAppDispatch,
type AppAction,
} from '../hooks/useDocumentState';
import { DetailPanel } from './DetailPanel';
vi.mock('allotment', () => {
function Pane({ children }: { children: React.ReactNode }) {
return <div>{children}</div>;
}
function Allotment({ children }: { children: React.ReactNode }) {
return <div>{children}</div>;
}
Allotment.Pane = Pane;
return { Allotment };
});
vi.mock('allotment/dist/style.css', () => ({}));
const mockGetObjectDetail = vi.fn();
const mockGetContentStream = vi.fn();
vi.mock(
'../../bindings/unidoc-pdf-debugger/internal/pdfservice/pdfservice.js',
() => ({
OpenFile: vi.fn(),
GetTreeRoot: vi.fn(),
GetChildren: vi.fn(),
CloseDocument: vi.fn(),
OpenFileDialog: vi.fn(),
GetObjectDetail: (...args: unknown[]) => mockGetObjectDetail(...args),
GetContentStream: (...args: unknown[]) => mockGetContentStream(...args),
GetImageData: vi.fn(),
DescribeImage: vi.fn(),
SaveImageToFile: vi.fn().mockResolvedValue(''),
GetReverseRefs: vi.fn().mockResolvedValue([]),
GetXRefTable: vi.fn().mockResolvedValue({ tabId: '', entries: [] }),
GetEmbeddedFiles: vi.fn().mockResolvedValue({ files: [] }),
GetSignatures: vi.fn().mockResolvedValue([]),
GetEmbeddedFileBytes: vi.fn().mockResolvedValue(''),
GetDocumentMetadata: vi.fn().mockResolvedValue({ info: {}, xmp: '', warning: '' }),
SaveBytesToFile: vi.fn().mockResolvedValue(''),
DiffDocuments: vi.fn().mockResolvedValue({ root: null, summary: {} }),
})
);
const catalogNode = {
id: 'root',
label: 'Catalog',
rawKey: '',
nodeType: 'dict',
valueType: '',
hasChildren: true,
childCount: 0,
iconHint: 'catalog',
error: '',
};
function streamDetail(nodeId: string) {
return {
nodeId,
objectRef: '',
type: 'stream',
properties: [],
elements: [],
scalarValue: null,
streamInfo: { length: 10, filters: [] },
};
}
function streamData(nodeId: string, text: string) {
return { nodeId, raw: text, tokenized: [], formatted: null, error: '' };
}
function dictDetail(nodeId: string, key: string) {
return {
nodeId,
objectRef: '',
type: 'dict',
properties: [
{ key, value: { type: 'name', display: '/Page', raw: '/Page', refTarget: '' } },
],
elements: [],
scalarValue: null,
streamInfo: null,
};
}
let dispatch: (action: AppAction) => void = () => {};
function DispatchCapture() {
dispatch = useAppDispatch();
return null;
}
function select(nodeId: string) {
act(() => {
dispatch({ type: 'SELECT_NODE', payload: { nodeId } });
});
}
function renderPanel() {
render(
<AppProvider>
<DispatchCapture />
<DetailPanel />
</AppProvider>
);
act(() => {
dispatch({
type: 'OPEN_DOCUMENT',
payload: {
tabId: 'tab-1',
fileName: 'test.pdf',
filePath: '/path/to/test.pdf',
rootNode: catalogNode,
rootChildren: [],
},
});
});
}
function focusedTestId(): string {
const el = document.activeElement as HTMLElement | null;
if (!el || el === document.body) return 'body';
return el.getAttribute('data-testid') ?? el.tagName.toLowerCase();
}
function shownKey(): string {
if (screen.queryByText('/FirstKey')) return '/FirstKey';
if (screen.queryByText('/SecondKey')) return '/SecondKey';
return 'none';
}
describe('DetailPanel Back/Forward keep keyboard focus across a navigation', () => {
beforeEach(() => {
vi.clearAllMocks();
mockGetObjectDetail.mockImplementation((_tab: string, id: string) =>
Promise.resolve(dictDetail(id, id === 'obj:0:3' ? '/FirstKey' : '/SecondKey')));
});
async function openTwoObjects() {
renderPanel();
select('obj:0:3');
await waitFor(() => expect(screen.getByText('/FirstKey')).toBeInTheDocument());
select('obj:0:4');
await waitFor(() => expect(screen.getByText('/SecondKey')).toBeInTheDocument());
}
test('Enter on a focused Back button leaves focus on Back', async () => {
const user = userEvent.setup();
await openTwoObjects();
screen.getByTestId('nav-back-button').focus();
await user.keyboard('{Enter}');
await waitFor(() => expect(screen.getByText('/FirstKey')).toBeInTheDocument());
expect(`focused=${focusedTestId()} | shown=${shownKey()}`)
.toBe('focused=nav-back-button | shown=/FirstKey');
});
test('Enter on a focused Forward button leaves focus on Forward', async () => {
const user = userEvent.setup();
await openTwoObjects();
screen.getByTestId('nav-back-button').focus();
await user.keyboard('{Enter}');
await waitFor(() => expect(screen.getByText('/FirstKey')).toBeInTheDocument());
screen.getByTestId('nav-forward-button').focus();
await user.keyboard('{Enter}');
await waitFor(() => expect(screen.getByText('/SecondKey')).toBeInTheDocument());
expect(`focused=${focusedTestId()} | shown=${shownKey()}`)
.toBe('focused=nav-forward-button | shown=/SecondKey');
});
});
|
@unidoc-alip Thanks for the review. F1: fixed in 43b5adf. The key is now on a F2: agreed, the fallback can't be reached. 2f0db87 puts Description: the help-line count now says 11 of 18, and the manual check note includes a keyboard pass over Back/Forward. I left
|
3 participants
Description and links
docs/cli-usage.mdwas a command-by-command reference. Someone who wanted to know why a file fails PDF/UA, or how to diff two versions, had to already know which subcommand to look for, and several recent CLI changes (dump pages,dump images,dump bytes, the image sample-interpretation fields, the validate rules list) were missing or scattered. The README still said Go 1.25 and Wails v3.0.0-alpha.74 while go.mod and CI are on 1.27 and beta.18, and its screenshot and GUI description showed the layout from before the left rail.While checking the guide against the binary,
pdfdebug --helpand each command's own usage line turned out to disagree on flags:dump treelisted--resolvein one and--pagein the other, four commands' usage lines left out--json, and--prettywas missing from most help lines. Every flag worked; only the text was wrong.Also here: moving from one node to another in the GUI (for example from one page's content stream to the next) kept the Object tab's scroll position, so the new content opened part way down.
Jira: US-1934
Technical changes
docs/cli-usage.mdis now the entry page: contents, four common tasks (why a PDF fails PDF/UA, compare two versions, pull out one image or stream, use it in CI), a one-screen quick-reference table, the shape of a command and the shared flags. Each table row links to its command's section on one of six topic pages underdocs/cli/: structure, pages and content, images and fonts, document data, validate and diff, and scripting. Scripting holds the exit codes fordump,validateanddiff, a CI script, what is and is not machine output, and the update notice with its opt-outs first.summary(apdfua-1-structuralrule that could not run reports info and exits 0), and on a missing file. It writes the report to a file and usesjq -e, because pipingvalidateinto plainjqpasses when the file is missing.cmd/cli/usage.goholds one synopsis per command.printUsageand every command's usage line are built from it, so--helpand the usage line print the same flags. The validate and diff help now say exit 2 also covers usage errors and thatdumpexits 0, 1 or 2. No flag or exit code changed.task packagefor the macOS.app, a refreshed overview and GUI description for the left rail layout,dump pages,validateanddiffin the CLI examples, and two new screenshots. CONTRIBUTING: toolchain versions match CI (golangci-lint was v2.1.6, CI now uses v2.14.0), and the release process checks the CLI docs againstpdfdebug --helpand links the guide pinned to the tag. CHANGELOG entries under[Unreleased].setup-gowith1.27.xnow resolves to Go 1.27.2, and v2.13.2 fails to typecheck its standard library ("export data version 5 is greater than maximum supported version 4"). v2.14.0 reports 0 issues under 1.27.2.frontend/src/components/DetailPanel.tsx: the views in the detail block are keyed by tab and node, so the content-stream, dictionary and array views remount at the top on every new selection, including when switching between two tabs with the same node selected. The key sits on a Fragment around the views only, so the header with Back/Forward and the find bar stay mounted and keep keyboard focus.tests/open-source-docsbuilds the CLI and checks the guide against it: every command in--helphas a quick-reference row and a reference heading, each synopsis in the guide equals the binary's usage line, every documenteddumpresource is dispatched, every table link and relative link resolves, everydocs/clipage is linked from the entry page, the CI script run undershfails on findings and on a missing file, and one command per documented exit code returns that code.cmd/cli/usage_flags_test.gochecks that the help line and usage line list the same flags and that each command accepts exactly the flags it lists.DetailPanel.scrollReset.test.tsxcovers the scroll reset andDetailPanel.navFocus.test.tsxchecks that Enter on a focused Back or Forward button leaves focus on it.Considerations
We looked at moving the CLI docs to a GitHub Wiki and decided against it. A wiki has no release tags, no PR review and no CI, so it would describe HEAD to someone running an older binary and the coverage tests above could not see it.
The reference is grouped by task, not one page per command. Most commands need 5 to 10 lines, so per-command pages would have been near-empty.
cli-usage.mdkeeps its path so existing links and tag-pinned release links still work.The docs tests read structure and fixed tokens (headings, table cells, synopses,
- N -exit-code lists), not prose, so rewording the guide does not break them.11 of the 18 help lines run past the description column. Moving descriptions to their own line would break the existing help parsers in
cmd/cliandtests/open-source-docs.How was this tested?
go clean -testcache && scripts/test-all.sh: go vet, golangci-lint 0 issues, root module, everytests/*/suite, frontend typecheck, lint and Vitest (1333 passed). All suites passed.body). The new docs tests were each made to fail once with a broken link, a missing row, a removed heading or a changed exit code, then restored.CI=1, and the CI script was run undershon a failing, a passing and a missing file.task package). The scroll reset is covered by Vitest only so far; it still needs a manual check in a production build, with the keyboard on Back/Forward as well as the mouse.What could go wrong?
Help and usage text changed for every command. Anything that parses
pdfdebug --helpor a usage line by exact text will see new flags in the synopses. Exit codes and output formats are unchanged.The quick-reference rows are reordered to follow the topic pages.
The CI-script test needs
shandjq; it skips on Windows and where either is missing.Keying the views by node remounts them on every selection, so any state held inside them resets too. None of those views hold their own state; it lives in
DetailPanel.Screenshots/videos (if appropriate)
Checklist
devbranch, notmaster