Repository navigation
Remove the latex-review names and keep paper settings in .git/papilot - #134
Conversation
The review engine's state, task refs, settings file and browser storage keys still carried the name of its retired standalone skill. Papilot now uses .git/papilot/, refs/papilot/ and papilot:review: keys, and keeps a paper's settings in .git/papilot/settings.json instead of writing .latex-review.json into the author's repository and its first commit. Existing papers move on first open. Every path into the state folder comes from stateDir(), which moves the old folder, refs (in one update-ref transaction) and worktree indexes before anything reads or creates the new folder; the server also moves every registered paper at startup and when one is added. Settings from .latex-review.json are copied once and the file is left untouched. Browser keys move in a module each web entry point imports first. The engine's own LICENSE and its separate notices entry go (same MIT license and author as Papilot), as does engine/.github, which GitHub never ran. The example paper drops its .latex-review.json and its preview is rebuilt for the reworded sentence.
|
@codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eb31f7700
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function moveLegacyStorage(storage: Storage | undefined = globalThis.localStorage) { | ||
| try { |
There was a problem hiding this comment.
When browser policy blocks storage access, reading globalThis.localStorage itself can throw a SecurityError. Default arguments are evaluated before entering the function, so the surrounding try does not catch this failure. Because each web entry point now imports this module and immediately calls moveLegacyStorage(), blocked storage prevents the entire workspace or review viewer from loading instead of allowing the existing storage fallbacks to operate. Resolve the default storage object inside the protected block.
Useful? React with 👍 / 👎.
| const common = commonDir(gitDir); | ||
| moveRefs(git, refs); | ||
| if (!exists(path.join(common, FOLDER))) return; | ||
| moveFolder(path.join(common, FOLDER), path.join(common, folder), false); |
There was a problem hiding this comment.
For an existing review with additional effect locations that have not yet been expanded, renaming the state directory leaves its saved rounds/.../places.json pointing to PDFs under .git/latex-review/cache/builds. engine/src/render/round.ts stores absolute build paths there, and Publisher.more() checks those paths before rendering additional locations, so expanding them after upgrading fails with “The builds of this round were removed” even though the PDFs were moved intact. Recompiling the manuscript does not update an existing round's places.json; migrate these retained paths or resolve them against the new state directory when reading them.
Useful? React with 👍 / 👎.
6b5e165 committed the whole file with CRLF before .gitattributes (text=auto eol=lf) existed, and Git leaves a file whose committed version already contains CR unnormalized, so later edits mixed LF lines into it. It was the only file in the repository with CR. Content is unchanged (git diff --ignore-cr-at-eol is empty); from now on .gitattributes keeps it LF.
Importing no longer reads .latex-review.json itself, so its two error messages are gone and the i18n check reported their translations as unused.
Rounds keep the builds they compare in places.json, and failed jobs keep their log, as absolute paths into the state folder. Expanding a round's further places after the move failed with "The builds of this round were removed". Those paths now point at the new folder before it moves, by replacing only JSON strings that start with the folder, so a move that stops halfway is simply done again. On a copy of a real thesis (537 refs, 24 places.json, 19 job logs, 56 worktree indexes) the whole move takes 1.7 s. Reading localStorage itself throws where the browser blocks storage; the default argument was evaluated outside the try, so the page would not load. The storage is now looked up inside it.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
On Windows fs.realpathSync keeps 8.3 short names (RUNNER~1), while the engine's realPath expands them, and stored build paths use native separators.
A review found the move unsafe in several cases: - A linked worktree added as a paper keeps its whole state in its own Git directory. It was skipped when the main checkout had no earlier folder, and could be deleted as a "disposable" index when both names existed. Every Git directory now moves the same way, and nothing is ever deleted. - Cached builds, their SyncTeX, rounds and jobs record absolute paths into the folder, and Papilot names a manuscript build by its PDF's path, so notes find their earlier PDF by it. SyncTeX cannot be rewritten to a path of another length. Instead of rewriting some records, the move leaves a link (a junction on Windows) under the former name: every earlier path stays valid, cached PDFs are reused and notes keep their builds. cachedPdf accepts a PDF reached through the link. - A review server of the earlier version may still run after an update. Elsewhere it keeps working through the link until Papilot replaces it; on Windows its open files keep the folder from moving, so Papilot stops it first and waits until it has exited. - Settings that cannot be read, and an unusable .latex-review.json, are reported again instead of silently falling back to detection, and messages name the settings file where it really is. - Once nothing is left to move, starting Papilot checks a few paths without running Git; empty ref folders Git leaves are removed. Papilot uses the engine's REFS, and opening a folder runs one Git command fewer.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b52e3e57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const dir of [common, ...(lstat(worktrees)?.isDirectory() ? fs.readdirSync(worktrees).map((name) => path.join(worktrees, name)) : [])]) | ||
| moveFolder(path.join(dir, FOLDER), path.join(dir, folder)); |
There was a problem hiding this comment.
On Windows, opening a linked-worktree paper can fail if an earlier review server is still running for the main checkout or another linked worktree. moveLegacyStates() stops only the server belonging to the requested root, but this loop also renames the main checkout's state and every linked worktree's state. An open database in any of those other directories makes moveFolder() throw, preventing the requested paper from opening even after its own server has stopped. Stop the legacy servers for all directories being migrated, or restrict folder migration to the requested Git directory.
Useful? React with 👍 / 👎.
Opening one paper also moves the folders of the repository's other papers. On Windows an earlier server of another paper can hold its files; that folder now moves when its own paper opens, and only the opened paper's own folder must move.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The review engine still used the name of its retired standalone skill for each paper's state, task refs, settings file and browser storage. This removes it and moves existing papers on first open.
What changes
.git/latex-review/.git/papilot/refs/latex-review/<task>/…refs/papilot/<task>/….latex-review.jsonin the working tree (also committed by the import commit).git/papilot/settings.jsonlatex-review:*(alsoplayground:latex-review:*)papilot:review:*latex-review <latex-review@localhost>Papilot <papilot@localhost>Importing or opening a folder no longer writes a file into the paper or its first commit. The engine's separate
LICENSEand notices entry are removed (same MIT license and author as Papilot), as isengine/.github, which GitHub never ran. The example paper drops its.latex-review.json, and its preview PDF/SyncTeX is rebuilt for the reworded sentence (same 26 pages, same SyncTeX inputs).Migration for existing users
stateDir()inengine/src/core/git.tsis the only way to get a state folder. Its first call per Git directory in a process runsmoveLegacyState(engine/src/core/legacy-state.ts), so nothing can create.git/papilotbefore the earlier state has moved.update-ref --stdintransaction; a ref whose new name exists stays (its commits stay reachable). Every Git directory's folder moves the same way: the main checkout's and each linked worktree's, which is a paper's whole state when that worktree was added as a paper. Nothing is ever deleted; if both names exist, both are left.!byte offsets), so rewriting some records would leave others inconsistent. With the link every earlier path stays valid: cached PDFs are reused, notes keep their builds, earlier rounds keep their positions and further places.cachedPdfaccepts a PDF reached through the link. Papers that never had the earlier state get no link.src/server/state.ts). A review server of the earlier version may still run after an update: on macOS/Linux it keeps working through the link (its lock and files resolve to the same place) until Papilot replaces it as it replaces any server of another version; on Windows its open files keep the folder from moving, so Papilot stops it first (stopServerIn) and waits until the process has exited. Update probes use a separate data folder and move nothing..latex-review.jsonare copied into the paper'ssettings.jsononce; the author's file is never modified. An unreadablesettings.jsonor an unusable.latex-review.jsonis reported, as before, instead of silently falling back to detection.engine/src/web/legacy-storage.ts, imported first by each web entry point; blocked storage is tolerated.places.json, 19 job logs, 302 tasks, 652 rounds) the move took 0.4 s; everyplaces.jsonstayed byte-identical, all 19 job logs were readable through their recorded paths, and the database passedintegrity_check.Also
src/server/manuscript.tswas the only file with CR line endings (whole file committed as CRLF in 6b5e165 before.gitattributesexisted, then mixed with LF edits). It is normalized to LF in its own commit;git diff --ignore-cr-at-eolis empty..latex-review.jsonare gone, so their translations are removed.Tests
engine/tests/ts/legacy-state.test.ts: moving the folder, packed and loose refs, worktree folders and settings, with earlier absolute paths readable through the link; a linked worktree added as a paper; both names present (nothing deleted, repeated); a paper without earlier state left untouched; unusable settings reported;stopServerInagainst a real review server.tests/legacy-state.test.ts: the startup sweep (and skipping a missing paper); browser storage keys, including the demo prefix and a key already saved under its new name.tests/imports.test.ts: an earlier version's.latex-review.jsonis honored and moved on import.