Deep links (#2583): answers to the review

2026-10-14. ^ticket(55) on the dashboard. In reply to the review of #2583 by @7h3kk1d, 2026-09-23 (changes requested).

Who wrote this: Matthew asked for this page; Claude (an AI model, in Claude Code) generated it, from the review and the branch's code at 867da4fe8d. Nothing on the branch has changed yet: these are answers and a plan, for Matthew to accept, change or drop.

In short

The review's main point holds: two separate URL encodings (the scratchpad's ?name=&share= and this PR's ?slide=), parsing tangled with applying, and nothing testable without a browser.
The plan: one type for a whole deep-link request, parsed from the query (passed in) apart from applying it, so parsing, the editor actions and making the URL can each be tested; applied as an action from Main.re; slide_name off Globals; the panel names in one switch in Sidebar.re; and the small fixes below.
Open questions for Matthew: whether the scratchpad's share encoding moves into the same type in this PR or a follow-up; the copied span (selection or caret, instead of the enclosing term); tutorials (not in this PR).

One request type, parsing apart from applying

Following the existing panel_request pattern, as the review suggests. A sketch, not settled code:

type target =
  | Slide(string)                   /* ?slide=   a documentation slide, by name: a pointer */
  | Share(string, string);          /* ?name=&share=   a scratchpad's contents: a snapshot */

type position =
  | Caret(Point.t)                  /* ?caret=  */
  | Select(Point.t, Point.t);       /* ?select= */

type request = {
  target: option(target),
  panel: option(panel_request),     /* ?panel=  */
  position: option(position),
};

let parse: (string => option(string)) => request;      /* the query lookup passed in */
let to_params: request => list((string, string));      /* parse(lookup(to_params(r))) == r */
let settings: (request, Settings.Model.t) => Settings.Model.t;
let action: request => option(Action.t);

Tests, without a browser: parsing each parameter and its edge cases (empty values, a column past the line's end, a range of whole lines, junk); the round trip from a request to a URL and back; the loose matching of a slide name against a list of names; and the action a request makes.

The comments, one by one

Where	The comment	The answer
DeepLink.re	A type for the whole request; separate parsing from applying; ideally subsume the scratchpad encoding.	Yes: the type above. Subsuming `?name=&share=` fits it as a `Share` target; whether in this PR or a follow-up is Matthew's call.
DeepLink.re	Then parsing, the editor actions and the URL can be tested separately.	Yes: `parse`, `action` and `to_params`, each with tests.
DeepLink.re:91	Nothing here is testable; pass the query param in.	Yes: `parse` takes the lookup, `string => option(string)`.
DeepLink.re:133	The panel request should be passed into `settings`.	Yes: `settings(request, stored)`.
DeepLink.re:14	The header comment reads "like a poem or ancient translation".	Fair. Rewrite it short and plain: one line per parameter, with an example.
DeepLink.re:44	Not clear what `key` is for.	It normalizes a name for loose matching (letters and digits, lowercased), for slide and panel names only. Rename it (say `loose_name`), with a one-line comment.
DeepLink.re:97	`List.find_index` exists as of OCaml 5.1.	Yes: the repository requires OCaml 5.2 (`hazel.opam`), and `CodeEditable.re` already uses it. Switch.
DeepLink.re:126	Simpler to put the enumerate/show mapping here than aliases and a default show inside `variant`.	Yes, with the next row: one switch from panel to URL name, and its inverse; the generic `variant` and the alias list go.
DeepLink.re:230	Define `panel_key` in `Sidebar.re` as a switch overriding the default show.	Yes: the switch lives in `Sidebar.re`, beside the panels it names.
Globals.re:141	Handle it as an action in `Main.re`, like the other actions.	Yes: at startup `Main.re` parses the request and schedules it as an action (it already schedules the caret move that way); the settings change goes through the update too, instead of `Globals.load`.
Globals.re:89	`slide_name` does not belong on `Globals`; raise `CopyDeepLink(span)` and let the Page-level update build the URL (non-blocking).	Agreed: do it in this PR. `CodeEditable` raises `CopyDeepLink(span)`; the Page update, which knows the mode, the slide and the sidebar, builds the URL and writes it to the clipboard. `slide_name` leaves `Globals`, and so does the `~linkable` bool threaded through the context menu.
CodeEditable.re:687	Why select the enclosing term rather than share the caret?	The idea was that the link should show what was pointed at, as "Select term" would. But it adds a step nobody asked for. Simpler: share exactly what is there, the selection if any, else the caret. Matthew's call.
Page.re:851	Do we want to support tutorial sharing?	Not in this PR. If it is wanted later, the request type can grow a target for it.

The order of the work

The request type, parse, to_params, settings and action, with tests.
Startup as an action in Main.re; CopyDeepLink(span) handled at the Page level; slide_name and ~linkable gone.
The panel names in one switch in Sidebar.re.
The small ones: List.find_index, the rename of key, the header comment.
The copied span: selection or caret (if Matthew agrees).
The Share target: here or a follow-up.