feat(harness): copy link URL via right-click / Alt+Click (plan #647) - #662
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Implement-plan for #647 (HANDOFF-READY).
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #662
Verdict: BLOCK
Repo: btipling/invincible
Scope: main ← plan/copy-link-url · 6 files · copy-link gesture (Wasm + host contextmenu)
Lenses run: L1, L2, L3, L4, L6, L8 (skip: L5 no new poll/alloc/history surface; L7 no host/config bind; L9 no palette / host-chrome layout)
AGENTS.md read: yes (docs/feature-divide.md also)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Blocker | L1+L4 | applyInvincibleRightClickPatch does not compile on the pinned Zig 0.16.0. native/harness/build.zig:597 (std.fs.cwd().readFileAlloc) — and the createFile / openDir twins at :625 / :657 — call an API std.fs no longer has. The function is invoked unconditionally at the top of build() (:20), so every zig build dies at build-file eval, including test-rich (which does not need the widget patch). |
Open build-harness #32101455178: Host unit tests (rich/*) → build.zig:597:23: error: root source file struct 'fs' has no member named 'cwd'. Merge as-is → next main build-harness is red → no harness-wasm artifact → SHA wait / deploy hook stall. |
“Warn-and-skip is only a runtime miss, CI will still build wasm.” False: this is a compile error in build.zig itself. The wasm step never starts. Proven by the already-failed check. |
high |
| Major | L1+L4+L6 | Even after a cwd fix, the vendored branch is a fail-open string replace against extracted zig-pkg / cache, and test-rich never proves it landed. applyInvincibleRightClickPatch returns on path miss, read fail, needle miss ("dvui pin drift?"), concat fail, and write fail — std.log.warn only. link_click.test.zig only exercises the pure kind() helper (7 rows). The dedicated .right processEvent branch the plan locked is therefore untested and optional at build time. walkForWidget (fallback) returns the first TextLayoutWidget.zig under zig-pkg / .zig-cache / zig-cache and then createFile mutates that tree (often the global package cache, whose hash no longer matches the pin). |
1. Pin whitespace / processEvent shape drifts → needle miss → warn → wasm builds clean → right-click on a transcript link does nothing (clipboard unchanged, no nav). Alt+Click still works (paint_text.zig + mod.alt(), no vendored branch), so a smoke that only tries Option+Click + test-rich goes green. 2. getPath/getPath3 miss → walk hits a testing-backend copy first → web backend widget stays stock → same silent right-click miss. 3. writeAll fails after createFile truncated the widget → later compile errors that do not mention plan #647. |
“Needle matches pin 4f810ef today (unique self.data().id hit at TextLayoutWidget.zig:2153); getPath3 will hit the right file; operator smoke will catch a skip.” Needle match on this pin is true and does not make fail-open safe — the next dvui bump, a cache-layout change, or a path-resolver miss is exactly when the comment says the patch must be re-applied, and the build will not fail. Operator rows 8/15 are not CI. Plan lock was “vendored dedicated branch,” not “best-effort mutate of b.dependency output.” Fail-closed (b.fail / non-zero if not applied) is the minimum for a string patcher. |
high |
Residual risk
paint_text.zig addTextClick + link_click.kind + isSafeLinkUrl is the right layer split; mod.alt() (not matchBind("alt")) is correct; remaining addLink sites are gone; mouse mods come from Window.modifiers the same way existing Ctrl/Cmd+Click does. After the compile break is fixed and the patcher fails closed (or the branch is applied by a real checked-in patch / pin-bump procedure), residual risk is: (1) canvas-wide contextmenu suppress is the locked tradeoff — composer has no native paste menu; (2) right-click capture is widget-wide on every TextLayoutWidget, not link-hit-tested on press; (3) PR is currently CONFLICTING with main (native/harness/build.zig test-rich description vs text_wave (#655) on ecc08ea) — rebase is required even after the code fixes.
Merge guidance
- BLOCK: do not merge.
build-harnessis already red; GitHub reportsmergeable=CONFLICTING. - Fix
std.fs.cwdagainst Zig 0.16 (and fail the build if the right-click branch is not actually present in the widget that the web compile step uses). Rebase ontomainand wait forzig → harness.wasmgreen. - Do not treat
zig build test-rich+ Alt+Click smoke as proof of right-click.
What was not attacked
Live DO runner cache contents after a successful extract; browser operator smoke (right-click / Option+Click / 📋 / Ctrl+V); whether a future dvui native context menu would obsolete the branch (out of diff).
7a3855b to
16dc628
Compare
Right-click or Alt+Click on an http(s) transcript link copies the URL without navigating. Left / middle / Ctrl+Cmd-click keep opening. - link_click.zig: pure copy vs open vs open_new (copy wins on Alt+Ctrl) - paint_text: addTextClick + clipboardTextSet / openURL (no addLink) - build.zig: dedicated right-click branch in vendored TextLayoutWidget (not OR'd into the left/middle selection gate) - HarnessHost: preventDefault on canvas contextmenu - docs/harness-limits.md: Links + Transcript copy/paste No new clipboard cap. No new bridge export. Closes #647 Refs #338
612d20b to
359a9b2
Compare
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #662
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/copy-link-url @ 359a9b2 · 6 files · copy-link gesture (Wasm + host contextmenu)
Lenses run: L1, L2, L3, L4, L6, L8 (skip: L5 no new poll/alloc/history; L7 no host/config bind; L9 no palette / host-chrome geometry)
AGENTS.md read: yes (docs/feature-divide.md also; SECURITY.md N/A — no workflow/secret/API change)
Re-review after the prior BLOCK on this PR. Head is mergeable; zig → harness.wasm is green.
Prior BLOCK — closed
| Prior finding | Status |
|---|---|
Blocker L1+L4 — std.fs.cwd / Zig 0.16 compile break |
Closed. Patcher uses std.Io.Dir.openDirAbsolute + b.build_root. Proven: build-harness #32104546199 green (test-rich + wasm). |
| Major L1+L4+L6 — fail-open string patcher | Closed. Path miss / needle miss / I/O now @panic / std.debug.panic. test-rich is on the same build() eval path, so a missing branch fails the job. |
mergeable=CONFLICTING vs text_wave test-rich description |
Closed. mergeable=MERGEABLE, mergeStateStatus=CLEAN. |
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L8 | walkForWidget still returns a relative path (zig-pkg/…/TextLayoutWidget.zig) while applyInvincibleRightClickPatch always openDirAbsolutes the dirname. resolveDvuiWidgetPath only reaches the walk if neither getPath nor getPath3 exists on LazyPath. |
A future Zig/dvui LazyPath reshape that drops those decls → walk “succeeds” → openDirAbsolute("zig-pkg/…") panics with a path-open error instead of the explicit “widget not found / needle drift” message. Time wasted on a dead fallback. |
“CI uses getPath/getPath3 today; the walk is unreachable.” True on 0.16 / this pin — that is why this is not a Major. The function is new in this PR and is written as if it were a working fallback. Join with b.build_root (absolute) or delete the walk. |
high |
Residual risk
addTextClick + link_click.kind faithfully mirrors stock addLink (matchBind("ctrl/cmd") / middle → new window) and adds copy-first. isSafeLinkUrl still gates before any clipboard/open (javascript: / data: / relative never copy). Needle " } else if (me.action == .motion and dvui.captured(self.data().id)) {" is unique in pin 4f810ef (TextLayoutWidget.zig:2153); the fc.data().id twins at 406/475 do not match. Right-click is not in button.pointer() (left/touch only), so the dedicated branch does not re-enter dragPreStart / sel_move.
Still unproven here (operator, not CI): Safari/WebKit contextmenu-on-mousedown vs preventDefault (mouseup / capture release); canvas-wide menu suppress is the locked tradeoff (composer has no native paste menu — Ctrl/Cmd+V unchanged). Next dvui bump must update the needle or the build will panic closed — that is the intended contract.
Merge guidance
- PASS WITH NOTES. Safe to merge from this attack. The Minor is optional (fix the fallback path or delete it).
- Prior Blocker/Major do not recur on
359a9b2. - Do not treat this comment as a GitHub approve (skill does not
--approveunless asked).
What was not attacked
Live browser operator smoke (right-click / Option+Click / 📋 / Ctrl+V / ~390 px); DO runner package-cache contents after extract; whether a future dvui native context menu would obsolete the vendored branch (out of diff).
Summary
Closes #647. Implements the HANDOFF-READY plan. Refs #338.
Right-click or Alt+Click (Option+Click) on an http(s) markdown / bare /
<>autolink copies just the URL. Left-click still opens; middle / Ctrl+Cmd still open in a new window.What shipped
native/harness/src/rich/link_click.zigcopy/open/open_newnative/harness/src/rich/link_click.test.zignative/harness/src/rich/paint_text.zigaddLink→addTextClick+clipboardTextSet/openURLnative/harness/build.zigtest-rich; apply dedicated right-click branch to vendoredTextLayoutWidgetapp/harness/HarnessHost.tsxcontextmenupreventDefault(+ unmount cleanup)docs/harness-limits.mdLocks honored
link_click.kind), not vendoredaddLink.rightpress/release branch — left/middle selection gate untouchedmod.alt(), notmatchBind("alt")(no such keybind)Tests
npm run typecheckvitest run lib/sessionSummaryLabel.test.ts lib/harnessHostModelPersist.test.tsvitest run --changedzig build test-rich(rich-link-click7 cases)build-harness)build-harnessExpected
test-richdelta: +7 (rich-link-click).