mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
feat/opencode2-native-commands
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michael Ramos
|
d0c32c8863 |
fix(review): preserve dragged diff ranges on compact touch before commenting (#1333)
* feat: add mobile touch range selection prototypes * docs: redirect mobile range selection spike * revert: remove command-mediated touch range prototype * docs: align mobile line selection with DiffsHub * feat: preserve mobile diff ranges before commenting * fix(review): repaint a preserved mobile range on a second drag A preserved range leaves pendingSelection non-null, so DiffViewer hands Pierre a defined selectedLines and Pierre switches to controlled selection: updateSelection then only records a proposed range and leaves painting to the host. With no change handler wired, a second drag never repainted, so the old highlight stayed put and the finger was untracked until release. Wire onLineSelectionChange back into app state, only on compact touch, so desktop keeps an options object with no such key. Also route a null range to the toolbar host instead of swallowing it in the preserve branch, mirroring AllFilesCodeView's early return so an open composer (Ask AI included) closes with the selection it was written for. * fix(review): 44px hit area for Pierre's gutter comment button on touch With a dragged range now preserved instead of opening the composer, that button is the only way to start writing about it, and it is roughly 20px square: below the data-pn-touch-target standard the rest of the compact shell holds. Grow its invisible ::before hit area to the 44px token, leaving the glyph alone. The rule ships through the same unsafeCSS both diff surfaces already inject, which lands in Pierre's shadow root inside @layer unsafe (last in the library's layer order, so no !important). It is injected only when the shell is compact rather than gated in CSS: html:has() matches nothing from inside a shadow root, and @media (pointer: coarse) would wrongly claim a desktop with a touchscreen. * test(review): cover the compact-touch preserved range on both diff surfaces DiffViewer.compactTouchSelection.test.tsx drives the FileDiff options the component hands Pierre: a drag preserves the range instead of opening the composer and paints it through selectedLines, a second drag repaints through onLineSelectionChange, the gutter action opens the composer, a cleared range still reaches the toolbar host, and desktop keeps routing completed drags straight to the composer with no change handler at all. The last three assertions fail against the pre-fix component. The AllFilesCodeView compact test only checked what was published upward, which a range nothing paints would also satisfy; it now asserts the range reaches the CodeView props. That needs the App loop, so the mount feeds published selections back down as pendingSelection: without it the reconcile effect clears the highlight the preserve branch just painted. * chore(guides-show): regenerate viewer manifest pin for the touch selection changes |
||
|
Michael Ramos
|
64062af9a1 |
feat: Portable Guided Reviews — export, share links, agent-authored guides, guides.show (#1324)
A Guided Review can now leave Plannotator: as a single self-contained HTML file that renders exactly like the in-app guide, as an encrypted-by-default share link on guides.show, or authored by any agent through the new guide CLI. Highlights: packages/guide-viewer extracted from review-editor at the injection seam (read-only host, no third renderer); guides.show Worker with R2-backed share storage, per-IP rate limiting on creation, delete tokens hashed at rest, and 128-bit ids; portable exports pin the viewer by SRI hash with budget and manifest gates in PR CI and at deploy; two-runtime parity across Bun and Pi verified; v0.27.x saved guides load unchanged. Retention is indefinite by explicit decision, to revisit with the lean sharing refactor. Decision record: adr/decisions/007-portable-guided-reviews-20260815.md |
||
|
Michael Ramos
|
64e3fa7762 |
feat(review): add compact touch review shell (#1301)
* feat(review): add compact touch review shell * fix(review): let submission dialog own initial focus * fix(review): refine compact mobile review chrome * fix(review): restore reliable mobile diff scrolling * fix(review): preserve mobile file identity * docs(mobile): specify phase 2b plan shell * fix(review): close mobile shell regressions * fix(review): restore narrow overview stacking * test(review): preserve real syntax theme resolver |
||
|
Michael Ramos
|
57b0358265 |
test(review): actually restore @pierre/diffs after the lifecycle mocks (#1230)
The afterAll restore in AllFilesCodeView.lifecycle.test.tsx captured the module namespace object with `await import(...)`. A namespace is a live view of the module record and bun's `mock.module` rewrites that record in place, so the captured handle became the stub the moment the mocks were installed and handing it back re-installed the stubs as themselves. The stubs therefore stayed live for the rest of the process. Whenever bun's file walk put this file before discardRestoreRender.test.tsx, that test's `processFile(...)` returned null and it failed on its first assertion. Spread-snapshot the exports at capture time, the idiom configure.test.ts already documents, and add an in-file guard so a future revert to the bare import fails here instead of somewhere downstream. Also correct the claim that the relative-path sibling mocks cannot escape this file: `mock.module` resolves them to absolute module paths and they leak like any other. |
||
|
Michael Ramos
|
7ba4e3b3e4 |
fix(review): mint content-derived diff cache keys so single-file tabs render fully (#1219)
* fix(review): mint content-derived diff cache keys so single-file tabs render fully Single-file diff tabs have not rendered their full-content diff since v0.26.0: the expansion gap bars show no chevrons and clicking them does nothing, at every file size. @pierre/diffs 1.3.2 (the 1.2.8 -> 1.3.2 bump, upstream "Fix diff rerender in edit mode (#878)") added name-based cacheKey defaulting in FileDiff.render: an unset `fileDiff.cacheKey` becomes the file's name. `areDiffTargetsEqual` — the only identity check its render and highlight caches make — compares nothing but that key. DiffViewer renders each file twice on one surviving FileDiff instance (key={filePath}): first the PARTIAL diff from getSingularPatch, then the AUGMENTED full-content diff from processFile once /api/file-content lands. Neither set a cacheKey, so both defaulted to the filename and Pierre served the stale partial render forever. Only the augmented diff is expandable, hence the dead gap bars. Both diffs now mint content-derived keys (`<path>#<hash>` and `<path>#full#<hash>`), matching how AllFilesCodeView already keys its items — which is why the all-files view was never affected. The hash (not patch.length) matters because Pierre's worker highlight cache is a singleton that outlives remounts. The partial diff needs its own key too: with key={filePath} the instance also survives diff-type and base switches, where a same-named new patch would otherwise hit the same name-keyed stale cache. hashString moves from AllFilesCodeView to utils/hashString.ts so both surfaces mint keys the same way. Covered by a new DOM test that mounts DiffViewer against the real @pierre/diffs renderer, holds the /api/file-content response until the non-expandable partial baseline is asserted, then requires the expansion affordances to reach the pixels. It fails against the unfixed tree. * fix(review): explain why an oversized file's card has no diff Files over the 5 MB review limit are replaced by a contents-free stub (buildOversizedTrackedStub, plus the untracked equivalent), which renders as a header-only card with no counts and no explanation. Users read that as a broken diff. The stub now carries an explicit marker line in its extended header (OVERSIZED_REVIEW_STUB_MARKER). A marker rather than a client heuristic because the only other signal, `Binary files ... differ`, is exactly what a genuine binary file emits, so a heuristic would put a false size-cap explanation on every image in the diff. The marker lives in shared/diff-paths so the browser bundle can detect it without pulling in the node-facing review core; both server runtimes pick it up from review-core, which vendor.sh already copies to Pi. Git ignores unknown extended-header lines and @pierre/diffs parses the stub identically with or without it, so nothing else moves. Which files get stubbed is unchanged. Both review surfaces now render one line under the file header saying the file is over the limit and only a stub is shown. * test(review): make the diff-swap proof machine independent, not stopwatch based CI failed two tests that pass locally. Both were timing races, neither was an app bug. 1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal wall-clock budget, which a cold, contended CI runner blows and a warm laptop clears. Two changes, both aimed at the clock rather than the symptom: - The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A slower box spends longer inside each turn but needs no more of them, so the budget never has to be retuned for CI hardware. - Pierre's shared Shiki highlighter is preloaded before mounting. It is a module singleton, and building it was the entire multi-second cost the old budget was accidentally measuring; warming it moves that work into an unbounded await OUTSIDE the observed window. Disposed in afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the pre-attachment behaviour of that same singleton. Verified against an artificially stalled clock: forcing 20s of dead time into every wait (41s total, far past the old 15s budget) still passes, and with the cacheKey fix removed it still fails on the assertion (not as an opaque timeout) in ~12s. A 20-turn budget with the preload removed and every core saturated also passed 10/10, so 400 turns is a wide margin rather than a guess. 2. App.archiveReadOnly compared the fenced block's innerHTML before and after a click. Since #1218, applyHighlight writes plain text first and swaps in Shiki markup when the grammar attaches, so that MARKUP changes on its own schedule and the assertion was racing the swap. The test is checking that the click opened no mutation entry point, which textContent plus the absence of an annotation <mark> says exactly, and which no highlight swap can perturb. Latent on main; the branch's run happened to lose the race. Also fixed while confirming the above: codeHighlight.test.ts asserted a GLOBAL precondition ("no grammar attached yet") that any earlier file attaching a typescript fence invalidates, so `DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order alone. It now resets the attachment cache through the existing __resetCodeHighlightCacheForTests seam and asserts the contract instead of the run order. Not currently reachable from CI (that file is not in the DOM list), but one list edit away. * test(review): drop the highlighter preload, harden the swap proof, report why a paint is missing The preload added in the previous commit made CI strictly worse, so it is gone. Before it, CI's partial diff painted and only the swap was missing; with it, CI never painted at all. It was an optimization for a theory the evidence has since killed, and it mutated a process-wide singleton to buy it, so it is not worth keeping while the real failure is unexplained. The afterAll dispose that existed only to undo the preload goes with it. What the CI log actually shows: - The "WorkerPoolManager: operation canceled because the pool terminated" error is inside discardRestoreRender.test.tsx's own group, ~0.3s BEFORE this file's group opens. It is that file's provider unmounting and terminating the pool singleton it created: end-of-file teardown, the same benign noise documented on #1209. It also prints on every local run, where the whole list passes. It is not a mid-test terminator, and nothing in this file uses the worker pool (no WorkerPoolContextProvider is mounted, so useWorkerPool() is undefined and rendering takes the main-thread path). - This file's group prints NOTHING for its whole 10.3s: no console.warn from the stale-content guard, no error. Pierre simply painted nothing. Not reproducible locally: the exact DOM list from test.yml, one bun process, forward and reverse order, 13 runs with every core saturated, all green. So the remaining difference is the environment, which cannot be reasoned out from here. Three changes make the next CI run answer it instead of costing another guess: - renderDiagnostics() dumps what Pierre actually painted (container / separator / chevron / line-number counts plus a markup fragment) when a wait gives up. Prints only on failure, so it is worth keeping. - The precondition is asserted rather than assumed: the REAL getSingularPatch and processFile must produce partial-then-full on these fixtures. Bun's mock.module is process global and an earlier file in this very list mocks '@pierre/diffs', so a leaked mock now fails in milliseconds with a clear message instead of as a render that never arrives. - The first paint is now REPORTED, not asserted. The verdict belongs to the swap; gating on the partial paint let a slow or absent first paint mask the result the test exists for. Removing the cacheKey fix still fails it (verified), because that tree paints no chevrons at any point. Also fixed a real trap in the fixture: the hunk header said @@ -61 while its context lines start at line 59 of both contents. Pierre realigns a misaligned header rather than rejecting it, so it was silently tolerated. * test(review): stop a leaked module mock from silently unrendering the diff tests Root cause, and it was never a timing problem. AllFilesCodeView.lifecycle.test.tsx calls `mock.module('@pierre/diffs', ...)` with a hunk-less `getSingularPatch` and `processFile: () => null`. Bun's module mocks are process global and are not unwound at file boundaries, and that file sits immediately before DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux runner the stub reached this file; on macOS it did not, which is why 13 local runs of the exact list, both orders, cores saturated, stayed green. It explains both CI symptoms exactly, including the one that looked like a contradiction: `processFile: () => null` means the augmented diff never exists, so no chevrons ever (the failure before the preload); the stub `getSingularPatch` has `hunks: []`, so nothing paints at all (the failure after it). The "WorkerPoolManager: operation canceled because the pool terminated" line was a red herring throughout: it is inside discardRestoreRender's own group, ~0.3s BEFORE this file's group opens, is that file's provider unmounting the pool it created, and prints on every local run too. The precondition assertion added in the previous commit is what proved it, turning a 10.3s mystery into a 0.45ms verdict: 228 | expect(expected?.isPartial).toBe(false); error: expect(received).toBe(expected) Expected: false Received: undefined Fixed at both ends: - Source: the mocking file now captures the real modules before it stubs them and restores both library specifiers in afterAll, so no later file in any run inherits its stubs. This fixes the class for every future DOM test that needs the real renderer, which was the actual leak. - Consumer: the two tests that render against the real @pierre/diffs get their own CI step, the same isolation (and for the same kind of reason) this workflow already gives useFileBrowser.test.tsx. They are removed from the shared list so that step is their single source of truth. The restore above should make this unnecessary; it is not something to bet a green build on from a machine that cannot reproduce the platform behaviour. Verified with a CI-faithful harness: one bun process per step, the exact lists from test.yml, isolated + shared-forward + shared-reverse, 10 iterations with every core saturated, then 6 more after the final split. All green, plus the full suite and typecheck. * docs(test): point the diff-renderer DOM tests at the CI step that actually runs them |
||
|
Michael Ramos
|
6b8723cc57 |
test(review): repair guide virtualization mock and run its tests in CI (#1200)
The AllFilesCodeView lifecycle test stubs '@pierre/diffs/react' with mock.module, which replaces the whole specifier. #1193 added an EditProvider import to AllFilesCodeView, but the stub was never extended, so the file throws at import when run on its own. The three DOM tests #1158 added for Guided Review virtualization were also never registered in the DOM_TESTS=1 CI step, so nothing enforced the 8-viewer mount cap and nothing surfaced the broken stub. |
||
|
skredik
|
80065c8462 |
perf(review): virtualize Guided Review file cards (#1158)
* perf(review): virtualize guided review file cards * fix(review): preserve guided review card behavior |