Repository navigation
[US-1960] Reload from disk and show when the open file has changed - #35
Conversation
- File > Reload from Disk, Cmd/Ctrl+R and F5 replace the tab in place with a fresh parse - Stat-polled change indicator on every tab, stale note with Reload and Compare - Compare with File on Disk through the existing Diff - Plain Text and signature reads refuse a file that changed since open
…st refused read and report a locked file as busy
…k, handles Cmd/Ctrl+R itself; watcher polls only strip tabs
…sk status restarts the watcher's settle streak
…edupe Cmd/Ctrl+R echoes; drop stale disk checks; busy hint in Plain Text
anovik
left a comment
There was a problem hiding this comment.
I've reviewed the change. The first three comments are bugs where the PR doesn't do what its description says: the Ctrl/Cmd+R menu echo reloads anyway, a file that keeps being rewritten never settles, and the frontend and the watcher can disagree about a tab's status. The rest are smaller behaviour and performance gaps, a Signatures tab that hides more than it needs to, and two cases of duplicated logic.
| if (e.key !== 'F5' && !isModR) return | ||
| // Cmd/Ctrl+R never reaches the webview reload, even in a text field. | ||
| e.preventDefault() | ||
| if (e.repeat || isInTextField(e.target)) return |
There was a problem hiding this comment.
When focus is in a text field, or the key is held down, this early return skips setting lastShortcut. The native accelerator then sends document:reload-active, and reloadFromShortcut('menu') sees lastShortcut.source === '', so it treats the event as a fresh press and reloads the tab.
Repro: focus the find bar and press Ctrl+R. The tab reloads and the view resets, but the description says this case is ignored. Holding the key fails the same way: once the 1s echo window has passed, the repeated menu events each reload.
Recording the suppressed press (or marking it as "swallowed") before returning would let the menu echo be dropped.
There was a problem hiding this comment.
Fixed in 7ac6281. A press that is swallowed (text field or key repeat) is now recorded before the early return, so the menu echo that follows is dropped as well. Tests cover Ctrl/Cmd+R in a text field followed by the menu event, and a held key with menu echoes after the first reload.
| // consecutive identical observations differ from the last reported status. | ||
| // A tab that stays current never emits. | ||
| func settle(ts *tabState, obs pdfcore.DiskStatus) (Event, bool) { | ||
| if obs == ts.candidate && ts.streak > 0 { |
There was a problem hiding this comment.
obs == ts.candidate compares the whole observation, including DiskModTime and DiskSize. A file rewritten more often than the poll interval therefore never produces two equal observations, the streak keeps going back to 1, and nothing is emitted.
That's the main watch-mode case: a generator rewriting the PDF every 5 to 8 seconds while the window is unfocused (10s polls). The state is changed on every check, but the tab shows no marker until the generator stops. Settling on State alone, then refreshing the times on the emitted status, would fix it.
There was a problem hiding this comment.
Fixed in d4132b7. The watcher now settles on State: two checks in a row that both say changed emit, with the latest times, even if the times differ. New times within the same state go out once they hold for two checks, so a generator gives one changed event and one refresh when it stops. The refresh is not announced again. TestWatcherSettleReportsFileStillBeingWritten replaces the old test.
| */ | ||
| export function setDiskStatusFromCheck(tabId: string, info: DiskStatusInfo, seqAtRequest: number): boolean { | ||
| const seq = lastSeq.get(tabId) ?? 0; | ||
| if (seq > seqAtRequest) return false; |
There was a problem hiding this comment.
The frontend drops this result when a watcher event arrived after the request, but on the backend GetDiskStatus always records the same result as the watcher's lastReported (RecordReported in service.go). After a drop the two sides disagree, and the watcher never sends the correction because its next poll matches lastReported.
Example: the file is saved twice in quick succession. The watcher emits changed(mtime X) with seq 6. A Plain Text or Signatures refusal had already sent GetDiskStatus with seqAtRequest=5, and it returns changed(mtime Y). The backend records changed(Y) and the frontend keeps X, so the tooltip shows a stale time from then on. If the file is reverted in that window, the dropped result is current, the backend records current, and the amber marker stays on a file that is current.
Either the backend shouldn't record check results as reported, or the frontend needs a way to tell the backend that it dropped one.
There was a problem hiding this comment.
Fixed in d4132b7. RecordReported now also sets a resync flag: the tab's next settled observation is emitted even if it repeats the recorded status. If the frontend dropped the check result, that event brings it back in line; if it kept it, the event is a no-op. TestWatcherRecordedStatusRestartsSettleStreak covers it.
| return code | ||
| } | ||
| defer func() { _ = ins.Close("cli") }() | ||
| if afterCLIOpen != nil { |
There was a problem hiding this comment.
Only the --json branch goes through a fingerprint-checked read. The default raw path further down (os.Open(filePath) + io.Copy, line 78) streams the file with no check. If the file is rewritten after openForCLI, which is exactly what this hook simulates, execBytesDump(path, docViewFlags{}) exits 0 and prints the new bytes, while --json exits 2 with "file changed on disk". The description says dump bytes passes ErrFileChanged through, but that holds only for --json. TestCLIFileChanged* only covers json: true.
There was a problem hiding this comment.
Fixed in d4132b7. The raw path now goes through a new Inspector.CopyDocumentFile, which checks the handle against the Open fingerprint before the first byte and after the last, and maps a lock violation to ErrFileBusy. Raw dump bytes on a rewritten file exits 2 with "file changed on disk" and writes nothing (TestCLIFileChangedBytesRaw).
| ctx *pdfcpu_model.Context | ||
| err error | ||
| ) | ||
| for attempt := 1; ; attempt++ { |
There was a problem hiding this comment.
If the file keeps changing during the parse, this loop runs up to three full ReadContextFile parses, and the last attempt still opens the document as changed. For a large file still being written (say 400 MB), that costs about three times the parse time and peak memory, and the UI stays in "opening" for that long. Keeping the first parse with its pre-parse fingerprint gives the same result after one parse, because the watcher or the read guard flags the change afterwards.
There was a problem hiding this comment.
Agreed, removed in d4132b7. Open parses once and keeps the pre-parse fingerprint; a save during the parse opens as changed.
| ts.candidate = pdfcore.DiskStatus{} | ||
| ts.streak = 0 | ||
| } | ||
| if dirty && newFocus != focused { |
There was a problem hiding this comment.
If focus is lost and then regained before the scheduler wakes, both SetFocused calls are merged: dirty is true but newFocus == focused, so checkAll doesn't run. That's the common alt-tab, save in the editor, alt-tab back sequence. The marker then waits for the next 2s tick instead of showing on the focus event, as the description promises. Running a check whenever focus became true at any point since the last wake (rather than comparing only the final value) would cover it.
There was a problem hiding this comment.
Fixed in d4132b7. SetFocused(true) sets a focusGained flag and the scheduler checks every tab when focus was gained at any point since the last wake, whatever the final value is. TestWatcherMergedFocusGainStillChecks covers the merged case.
| if fp.info == nil || fi == nil { | ||
| return false | ||
| } | ||
| return fi.Size() == fp.Size && fi.ModTime().Equal(fp.ModTime) && os.SameFile(fp.info, fi) |
There was a problem hiding this comment.
In the steady state (size and mtime equal), every poll reaches os.SameFile. On Windows a fresh os.Stat result loads its file ID lazily, so each poll per tab also opens the file (CreateFile + GetFileInformationByHandle + CloseHandle). With 10 tabs on an SMB share, the 2s focused poll does about 20 network round trips instead of 10, all for files that haven't changed. Consider taking the identity from one handle stat per poll, or checking SameFile only when cheaper signals are inconclusive.
There was a problem hiding this comment.
Fixed in d4132b7. DiskStatus now stats through statWithIdentity: on Windows one FILE_READ_ATTRIBUTES handle whose stat already carries the file ID, so os.SameFile opens nothing more and a poll opens the file once. Elsewhere it is os.Stat. A Windows test checks the ID is taken at stat time.
| if err != nil { | ||
| return nil, wrapPDFError(err) | ||
| } | ||
| if readErr != nil { |
There was a problem hiding this comment.
A refused hole read fails the whole GetSignatures call, which hides every signer, chain and algorithm detail even though those all come from the in-memory parse. Only the coverage and hole measurement needs the file. So a signed PDF that is only touched on disk (same content, new mtime) shows just "File changed on disk. Reload to view." in the Signatures tab. This function already reports per-signature failures as per-entry error fields, so setting the error on the coverage field (or HoleMatchesContents) would keep the rest visible.
There was a problem hiding this comment.
Changed in d4132b7 / 7ac6281. A refused hole read no longer fails GetSignatures. Every signature keeps its parsed details, and a new holeCheckError field carries the file-changed or busy text; holeMatchesContents is false and means "not checked", and later holes are skipped. The GUI shows "Hole matches /Contents: not checked: ..." and still marks the tab from a disk check; dump signatures prints the same and exits 0.
| const reloading = new Set<string>(); | ||
|
|
||
| /** Short reason for a failed reload, from the extracted backend message. */ | ||
| export function reloadFailureReason(message: string): string { |
There was a problem hiding this comment.
This copies the not-found, encrypted/password and malformed regexes from mapErrorMessage in hooks/usePDFService.ts, so the two need to be kept in sync. When a new open-error category is added to mapErrorMessage, a reload failure in that category will still say "unexpected error" unless this copy is updated too. A short-reason variant that shares the same matcher would avoid that.
There was a problem hiding this comment.
Fixed in 7ac6281. openErrorKind in usePDFService.ts is now the one matcher; mapErrorMessage and reloadFailureReason both switch on its result.
| const st = res as DiskStatusInfo; | ||
| // The backend recorded this result as reported, so it is stored even | ||
| // when the compare itself is dropped below; the watcher will not send it. | ||
| if (tabsRef.current.some((t) => t.tabId === tabId)) setDiskStatusFromCheck(tabId, st, seqAtRequest); |
There was a problem hiding this comment.
handleCompare repeats the seqAtRequest + GetDiskStatus + setDiskStatusFromCheck sequence from markTabFromDiskCheck (lib/diskCheck.ts), and handleReload repeats reloadActive from App.jsx. The copies have already drifted: this one checks that the tab still exists before writing to the store, and diskCheck.ts doesn't. Having markTabFromDiskCheck return the status promise, and sharing one reload helper, would leave one copy of each.
There was a problem hiding this comment.
Fixed in 7ac6281. checkDiskStatus in lib/diskCheck.ts returns the status and is used by both markTabFromDiskCheck and handleCompare. The store now drops results for tab ids that left the tab list, so neither caller checks that itself. reloadActiveTab is the shared reload entry for App and the stale note.
…rged focus gains; signatures note unread holes; raw dump bytes is guarded; Open parses once
… hole notes; one disk-check and reload path
Description and links
A common way to use pdfdebug is to keep a PDF open while regenerating it from code, or editing it in another app. Until now the open tab gave no sign that the file had changed, and the only way to pick up the new version was to close and reopen it. Two views also mixed versions. Plain Text re-reads the file lazily, so it showed bytes the tree no longer described. Signatures re-read the file to check the /Contents hole and swallowed any read error, so a changed or deleted file showed up as a signature hole mismatch.
This adds reload from disk, a quiet marker for tabs whose file has changed, and Compare with File on Disk. Plain Text and Signatures now refuse to read a file that changed since it was parsed. All of this is desktop only; the CLI only gains the "file changed on disk" error.
Jira: US-1960
Technical changes
internal/pdfcore/filechange.go:Openkeeps a fingerprint of the file (size, mtime, file identity). On Windowsos.Statonly loads the file ID the first timeos.SameFilesees it, soOpenloads it right away; otherwise a rename-over would compare the new file's ID with itself and match. NewErrFileChanged("file changed on disk since it was opened") andErrFileBusyfor a Windows sharing or lock violation.Inspector.DiskStatus(tabID)reportscurrent,changedormissingagainst the fingerprint. The fingerprint comes from the stat before the parse, so a save that lands during the parse opens as changed. On WindowsDiskStatusstats through one attributes-only handle, which carries the file ID, so a poll opens the file once instead of twice.holeCheckErrorfield on that signature ("not checked") instead of a hole mismatch, with later holes skipped. A document without signed fields never reads the file. On Windows a read refused by another process's byte-range lock isErrFileBusy, the same as a sharing violation at open; Plain Text and the hole check show it as "in use by another app".internal/filewatch: one scheduler goroutine polls every tab in the frontend tab strip withDiskStatus, every 2s while the window has focus and every 10s otherwise, plus once on every focus event. At most one stat per tab is in flight, so a hung network share blocks only its own tab, andStopdoes not wait for it. A state has to be seen on two checks in a row before it is reported, even while its times keep changing, so a file a generator keeps rewriting is still marked; new times within the same state are sent once they hold for two checks. AGetDiskStatusresult recorded in between restarts that count and makes the next settled status go out even if it repeats, so the frontend converges if it dropped the result as stale. A focus gain merged with a later loss still checks every tab. Events carry an increasingseqso the frontend can drop stale ones.main.go: File > "Reload from Disk" (CmdOrCtrl+R) and "Compare with File on Disk" after "Close Document". There is also a lifecycle-only service that starts and stops the watcher, focus handlers that readwindow.IsFocused()(Wails can deliver focus events out of order), and thedocument:disk-statusemitter. New bound methodsGetDiskStatus, used for the immediate checks, andSetWatchedTabs, which the frontend calls with the strip's tab ids whenever the set of ids changes, so backend-only documents (a Diff right side) are never polled.frontend/src/lib/reloadDocument.ts) parses the file again and replaces the tab in place under a new tab id, using the existingOPEN_DOCUMENTreplace branch withreload: trueso the batch-open counters are left alone. The view resets as for a fresh open and the left-rail panel is kept. A second trigger is ignored while one is running, and a result for a tab closed during the parse is discarded and its document closed. A reload that lands after you switched tabs replaces its tab without taking focus. After the swap oneGetDiskStatusmarks the new tab if the file changed again during the parse. The reloaded parse's warning replaces the banner. Failure shows the ErrorBanner and keeps the old tab, the warning and any open in progress; a busy file only shows the pill. F5 and Cmd/Ctrl+R are also handled on keydown (Cmd/Ctrl+R never reaches the webview's page reload), ignored in text fields and on key repeat; the menu event and the keydown for one press reload once, in either order, even when the first reload already finished.AppState(lib/diskStatusStore.ts), so an event re-renders only the tab item and the note. The tab dot sits in a fixed-width slot (no layout shift), isaria-hiddenand has no click handler; the tooltip carries the change time. The detail panel showsStaleNotebeside the inner tab list, and a polite live region announces the change. Timestamps show the time only for today and the full date otherwise.GetDiskStatusfirst: a missing file shows a message and parses nothing, and over 500 MB asks in a dialog. Reload in the Diff header promotes the compared copy to the tab without parsing it again.dump bytes, raw and--json, exits 2 with "file changed on disk" (the raw path streams throughInspector.CopyDocumentFile, which checks the handle before and after).dump signaturesprints the hole check as "not checked: file changed on disk ..." and keeps the rest.tests/file-change-detectionsuite is in the Windows subset so the Windows-only checks run there. CHANGELOG entry under Unreleased.Considerations
Detection is stat-only on purpose: no content hash, so an in-place rewrite that keeps size, mtime and identity is not seen. The tests say so. Nothing ever reloads by itself.
Reload mints a new tab id instead of adding a backend reload method. Every per-tab cache in the frontend (tree, Pages, Images, the detail views) is already keyed by tab id, so they all refetch with no extra invalidation, and a failed parse leaves the old document untouched.
How was this tested?
GOFLAGS=-count=1 scripts/test-all.sh: go vet, golangci-lint 0 issues, root module with-race, everytests/*/suite, frontend typecheck, lint and Vitest (98 files, 1450 tests). All suites passed.internal/filewatchruns on a fake clock and a fake stat source. It covers the two-check rule,seq, the 2s and 10s intervals, the immediate check on focus, a blocked tab not delaying others, closing a tab stopping its checks, andStopreturning while a stat hangs. It passed 30 runs under-race.internal/pdfcore/filechange_test.gocovers the guard for an overwrite, a deletion, a rename-over with the original mtime copied on, a size-only change with the mtime restored, a change during the Plain Text read, a change during the signature walk followed by a clean call once the file is back, and the raw byte copy. The rename-over and locked-file cases need Windows and only run on the Windows CI leg. The locked-file tests use aLockFileExbyte-range lock, because Go'sos.OpensetsFILE_FLAG_BACKUP_SEMANTICSand gets past a no-sharing lock on the CI runner.GetDiskStatusand the three new events are added to the bindings and event-surface contract suites.task package). The cross-platform production-build check below has not been run yet.What could go wrong?
Cmd/Ctrl+R and F5 rely on the menu accelerator and the keydown handler. Wails already turns off WebView2's browser accelerator keys, and there is no View menu with a Reload role, but this still needs a check on each OS that the webview does not reload the page itself.
Polling runs while files are open. That is one stat per tab every 2s while focused, cheap on local disks; on a slow network share a hung stat holds back only its own tab.
Still to do before merge: a production-build check on macOS, Windows and Linux (regenerate an open file, a background tab, reload by Cmd/Ctrl+R, F5 and the menu, Compare then Reload from Diff, delete the open file, and on Windows save from another app while the file is open), with screenshots added here.