fix(desktop): independently replace cursor values - #3456
Conversation
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head 367d048fbcfa587833af303e0e971ce783a1dc4b against main@f1f4e71a9540a4da23159052c924fee72202e989.
No actionable implementation defect was identified in the replacement itself. The public record shows the complete replacement set was posted before the contributor opened the old values, the source replaces every listed binary-derived value, the tip remains the action coordinate through moving/pressed transforms, the nine-candidate allocation preserves zero arc and all retained departure weights, the slowest spring is covered by the 2100 ms ready fence, and reset/frame-clock changes prevent stale presentation reports.
One required repository-policy item is missing: this PR does not state whether generative tooling made a substantive contribution. CONTRIBUTING.md requires every PR to say yes or no; if yes, name the tool/scope and ensure each materially AI-authored commit carries Generated-by. If none was used, say so explicitly. That must be resolved before merge.
Required conclusions:
- Optimal for the actual problem: yes, subject to human provenance/licensing judgment. The replacement uses the existing engine and retains the Maka-owned surrounding architecture.
- Production code to delete: none identified.
scripts/generate-cu-status-icons.mjsis a manual reproducibility tool, not a runtime path; consider adding a--checkmode or documenting the exact invocation because it duplicates glyph/palette constants and is not wired to any package script. - Tests to delete/replace: none identified. The exact-value assertions are justified here because the public clean-room derivation is the source being preserved; the overlay timing harness covers behavior that the engine-only tests cannot.
- Deeper refactor: no.
- Ready to merge: no. The head is currently conflicting with main, required
testhas not run, the AI-use declaration is missing, and this material provenance/user-visible change requires independent human review. - Residual risks: 1x/2x light/dark visual quality and clean-room/legal sufficiency are human review questions; the status-icon generator can drift unless its output is checked.
This automated review is not approval and cannot satisfy the independent human provenance/licensing gate.
367d048 to
0c959bf
Compare
|
Automated follow-up by OpenAI Codex:
The current exact head is Independent human provenance/licensing and visual review remain outstanding. Automated review does not count as that approval. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the clean-room derivation being published before the replaced values were inspected, with a public link to the derivation, is the right way to do this, and it makes the PR reviewable in a way it otherwise would not be.
Reviewed at exact head 0c959bfea4aec7a52a83109610727dbca6d7227f against base ce2cca384cfe8c7c1b3bbd0be794816e554c7c6d2. No P0–P3 in the code.
What we checked:
- No second authority was introduced.
CursorEngineremains the single owner of motion, scoring and drawing; the newcursor-candidate-grid.tsis a pure function that generates the nine candidates and holds no engine state. The overlay still owns only the frame clock, the close-enough gate and the presentation phase, and the main controller still owns the session/generation/action fence. The replaced values did not get left behind anywhere as a fallback path. - The stated invariants have code and tests behind them, not just prose. The 2100 ms presentation deadline is asserted directly (
cursor-overlay-timing.test.ts:196) and its derivation is written down where the constant lives (cursor-engine.ts:121— the 2000 ms frame plus 100 ms of renderer/IPC margin), with a separate test covering 240 Hz release and one-fps observability. The tip hotspot, the nine-candidate budget, the departure fan on an interrupted move, and queued-frame reset invalidation are each reachable from a test rather than asserted only in the description. backgroundThrottling: falsematches the surrounding contract, since the overlay does not request a frame while idle.
We are not approving this, and the reason is not the code. Your own description says independent human review is still required for provenance/licensing sufficiency and for visual quality, and we agree — that is not a gate an automated review can close. Whether the published derivation is legally sufficient as a clean-room record, and whether the resulting cursor actually looks right at 1x/2x in light and dark over complex backgrounds, both need a human maintainer to sign off. Everything above is only the statement that the code implementing those values is internally consistent and does not leave the old ones reachable.
One non-blocking note for later: the status icons are generated by scripts/generate-cu-status-icons.mjs and the command is documented, but nothing in CI checks that the committed assets still match what the script produces. That is fine now, while the values are fresh in everyone's memory; it is exactly the kind of thing that silently drifts a few months out. Worth a drift check in a follow-up, not here.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
Replace the cursor glyph, motion, scorer, and readiness configuration from the public clean-room derivation for issue apache#3293. Synchronize PiP and status assets, document provenance, and cover timing, hotspot, scoring, and candidate-grid behavior. Generated-by: OpenAI Codex
0c959bf to
9896d89
Compare
Summary
Fixes #3293
Verification
npm run build— passed onmain@4acfa269npm run typecheck— passednpm --workspace @maka/desktop test— 1167/1167 passednpm run check:asf-headers— passed after adding the current ASF header to all four new source filesnpx knip --workspace apps/desktop— passednpx knip --workspace packages/ui— passednpm run lint— passednpm run format:check— passednode scripts/generate-cu-status-icons.mjstestcheck passed on the previous exact head; the current rebased head is waiting for maintainer approval of CI run 32580171399Clean-room provenance and review focus
The complete independent derivation was published before the replaced implementation and provenance values were inspected:
#3293 (comment)
Independent human review is still required for provenance/licensing sufficiency and visual quality; automated review does not satisfy that gate.
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex — clean-room analysis, implementation, tests, documentation, conflict resolution, and review follow-up. The materially AI-authored commit carries
Generated-by: OpenAI Codex.Checklist
Does this PR entail a change in behavior?