Gui wysiwyw - #270
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds GUI mode for selecting, moving, scaling, and dimensioning document elements. It adds GUI metadata and coordinate argument handling, embeds source locations when required, and extends compiler state. Previewer, client, server, and LSP protocols now exchange GUI activation, coordinate, and navigation messages. Runtime state, toolbars, keyboard handling, styling, and browser event processing support GUI mode. LSP diagnostics and AST lookup now use explicit source text and UTF-8 or UTF-16 position encoding. Documentation and shortcut references are updated. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (9)
src/lspishow/current_ast.ml (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider giving
pos_inexplicitpositionEncodingandsourceparameters.
pos_inonly binds~path,~pos, and~meta. The remaining labels ofpos_in_textlocleak into its inferred type, so every call site passes~positionEncodingand~sourceto a function whose definition does not mention them. Explicit parameters make the signature self-describing and keep future changes topos_in_textlocfrom silently changing the arity ofpos_in.♻️ Proposed explicit threading
-let pos_in ~path ~(pos : Linol_lwt.Position.t) ~meta = - let loc = Cmarkit.Meta.textloc meta in - pos_in_textloc ~path ~pos ~loc +let pos_in ~positionEncoding ~source ~path ?permissive + ~(pos : Linol_lwt.Position.t) ~meta () = + let loc = Cmarkit.Meta.textloc meta in + pos_in_textloc ~positionEncoding ~source ~path ?permissive ~pos ~loc ()src/lspishow/lspishow.ml (1)
828-838: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftReuse the root compilation instead of compiling the document twice on save.
Line 825 already compiles the root through
Roots.update_root. Lines 829-833 compile the same document again withcompile_all ~embed_loc:false. Every save now pays for two full compilations of the document and all its includes. The handler runs for each root of the saved file, so the cost multiplies.If the only reason for the second pass is
embed_loc:false, consider making the HTML export strip embedded locations, or perform the export outside the save handler.src/compiler/compile.ml (2)
325-343: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCompute the marshalled location only when the block accepts attributes.
When
embed_locistrue, this code marshals and base64-encodes the location for every block before it callsAst.Utils.Block.update_attribute.update_attributereturnsNoneforBlank_lineandBlocks, so the encoded value is discarded for those nodes. On a large document that is a measurable amount of wasted allocation on the compile path.Move the encoding inside the update callback so it runs only for blocks that carry attributes.
♻️ Proposed change
let b = if embed_loc then - let loc = b |> Ast.Utils.Block.meta |> Cmarkit.Meta.textloc in - let v = Marshal.to_string loc [] |> Base64.encode_string in - let attr = { Cmarkit.Attributes.v; delimiter = Some '"' } in let res = Ast.Utils.Block.update_attribute (fun (attrs, meta) -> + let loc = b |> Ast.Utils.Block.meta |> Cmarkit.Meta.textloc in + let v = Marshal.to_string loc [] |> Base64.encode_string in + let attr = { Cmarkit.Attributes.v; delimiter = Some '"' } in ( Cmarkit.Attributes.add (Special_attrs.original_loc, Meta.none) (Some (attr, Meta.none)) attrs, meta )) b in
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the new
Special_attrsalias for consistency.Line 2 introduces
module Special_attrs = Common_types.Special_strings, and the rest of the file usesSpecial_attrs.include_,Special_attrs.src,Special_attrs.video, andSpecial_attrs.as_htmlon Line 150. These two lines still spell outCommon_types.Special_strings.as_html.♻️ Proposed change
- if has_attrs (fst attrs) Common_types.Special_strings.as_html then html () + if has_attrs (fst attrs) Special_attrs.as_html then html ()- if has_attrs attrs Common_types.Special_strings.as_html then + if has_attrs attrs Special_attrs.as_html thenAlso applies to: 102-102
vendor/github.com/panglesd/cmarkit/src/cmarkit.ml (1)
203-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
idmapping.
List.concat_map (fun _ -> f (Id id)) [()]iterates a one-element list to callfonce. It is equal tof (Id id).♻️ Proposed change
- let id = match attrs.id with None -> [] | Some id -> List.concat_map (fun _ -> f (`Id id)) [()] in + let id = match attrs.id with None -> [] | Some id -> f (`Id id) invendor/github.com/panglesd/cmarkit/src/cmarkit.mli (1)
328-336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new list semantics of the
attrscallback.The callback now returns a list. Callers can use this to delete an attribute by returning
[]and to expand one attribute into several, which is exactly what the slipshow compiler does when it turnsguiintoguiplusslipshow-gui-loc. NeitherAttributes.mapnorMapper.makedocuments that behavior, so the contract is only discoverable from the implementation.Add a short note to both doc comments, for example: the returned list replaces the input attribute;
[]deletes it.Also applies to: 1580-1588
src/engine/runtime/drawing/drawing_state/drawing_state.ml (1)
30-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake rejected transitions observable to callers.
The exhaustive match is a good design: adding a status forces a decision for every pair. One consequence needs attention.
Status.setnow silently ignoresDrawing Presenting -> Drawing (Recording _)on Line 51.start_recordingon Line 87 callsStatus.set (Drawing (Recording {...}))after it has already computedreplayed_partand mutatedstro.erasedfields in theunplayed_erasureloop on Lines 75-81. Ifstart_recordingever runs while the status isDrawing Presenting, those mutations stay applied but recording does not start, and nothing reports the failure.Return a result from
set, or log the rejected transition, so callers and future maintainers can detect this case.♻️ Proposed change to the signature
module Status : sig - val set : status -> unit + val set : status -> [ `Set | `Rejected ] val peek : unit -> status val get : status Lwd.t end = structCallers that do not care can ignore the result with
ignore (Status.set s).src/engine/runtime/gui/action.ml (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a
matchover structural equality onstatus.
Drawing_state.Status.peek () = Gui_modeuses polymorphic compare on a type whose other constructors holdLwd.varvalues and closures insiderecording_state. The comparison is safe today becauseGui_modeis a constant constructor and compare short-circuits on the representation difference. A future status that carries a payload would turn this into a possibleInvalid_argumentat runtime.Controller.shortcutson Line 51 already usesmatch, which is both clearer and provably safe.♻️ Proposed change
-let deactivate () = - if Drawing_state.Status.peek () = Gui_mode then ( - Drawing_state.Status.set (Drawing Presenting); - match Lwd.peek State.current with - | Some old_el -> - Lwd.set State.current None; - El.set_class activate_class false old_el - | None -> ()) - else () +let deactivate () = + match Drawing_state.Status.peek () with + | Gui_mode -> ( + Drawing_state.Status.set (Drawing Presenting); + match Lwd.peek State.current with + | Some old_el -> + Lwd.set State.current None; + El.set_class activate_class false old_el + | None -> ()) + | _ -> ()src/engine/runtime/gui/controller.ml (1)
42-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two identical
deactivatebranches.
"Escape"and"G"run the same body.♻️ Proposed change
- | "Escape" -> - Action.deactivate (); - true - | "G" -> - Action.deactivate (); - true + | "Escape" | "G" -> + Action.deactivate (); + true
🔇 Additional comments (102)
docs/gui.rst (1)
5-7: LGTM!docs/index.rst (1)
83-83: LGTM!docs/shortcuts.rst (1)
25-26: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the recording-manager key bindings against the runtime.
The documentation now assigns
Shift + Sto starting recording andShift + Rto closing the recording manager. The sameShift + Ropens the manager when it is closed. Confirm that the runtime uses these context-dependent bindings.docs/special-elements.rst (1)
175-181: LGTM!vendor/github.com/panglesd/lwd/lib/nottui-unix/dune (1)
5-5: LGTM!vendor/github.com/panglesd/lwd/lib/nottui/dune (1)
5-5: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the renamed Dune libraries are declared consistently.
These stanzas now require
notty-communityandnotty-community.unix. Confirm that both libraries are declared and that no relevant Dune or package metadata still exposes onlynottyornotty.unix. Otherwise dependency resolution can fail during the build.src/cli/run.ml (1)
52-53: LGTM!Also applies to: 92-93, 116-116
src/compiler/slipshow.ml (1)
250-252: LGTM!Also applies to: 301-304, 371-374
src/compiler/slipshow.mli (1)
43-43: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the intended public API break.
delayed,convert, andconvert_to_mdnow requireembed_loc:bool. Existing downstream callers will fail to type-check until they add this label. If this break is intentional, update downstream callers and document the migration. Otherwise, preserve a defaulted optional argument.Verification script
Inspect every call site and confirm that it passes
~embed_loc.Also applies to: 61-70
vendor/github.com/panglesd/lwd/examples/cbor/dune (1)
6-6: LGTM!vendor/github.com/panglesd/lwd/examples/dune (1)
5-5: LGTM!Also applies to: 11-11, 17-17, 23-23, 29-29
vendor/github.com/panglesd/lwd/lib/nottui-lwt/dune (1)
4-4: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the vendored LWD package metadata.
Line 4 uses
notty-community.lwt. The community package publishes this Dune library name. (raw.githubusercontent.com) The supplied files do not include the vendoreddune-projector generated opam declarations. If those declarations still requirenotty, package installation can omitnotty-community, and the vendored libraries can fail with a missing-library error. Update the metadata with these Dune files, or confirm that the vendored package is never packaged independently.Verification script
Confirm that relevant vendored LWD package declarations use the community library names consistently.
src/communication/communication.ml (1)
18-29: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify all
payloaddispatchers handle the GUI constructors.The supplied files define the new constructors but do not show their scheduler, previewer, and server handlers. A stale exhaustive match can reject GUI messages or drop GUI state. Trace each constructor through its sender and receiver before merge.
src/communication/communication.mli (1)
16-27: LGTM!src/cm_plugin/slipshow_communication.ml (1)
32-35: LGTM!src/lspishow/debug.ml (1)
10-10: LGTM!src/lspishow/diagnostic.ml (1)
38-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Convert the exclusive end byte before UTF-16 conversion.
Cmarkit.Textloc.last_byteis inclusive. For a location that ends on a non-BMP character,corrected_positionstops before that character and line 43 adds one UTF-16 unit. The LSP range then ends inside its surrogate pair. Cmarkit documents inclusive byte ranges. (ocaml.org)Pass
last_byte + 1topos_of_textlocand removecharacter + 1.Proposed fix
- let byte = Cmarkit.Textloc.last_byte loc in + let byte = Cmarkit.Textloc.last_byte loc + 1 in let line, character = pos_of_textloc ~source ~positionEncoding ~line ~byte in - let character = character + 1 in Linol_lwt.Position.create ~character ~linesrc/lspishow/diagnostic.mli (1)
1-13: LGTM!src/lspishow/lsp_preview.ml (1)
15-15: LGTM!Also applies to: 40-41, 60-63
src/lspishow/lsp_preview.mli (1)
3-8: LGTM!src/engine/previewer/dune (1)
4-4: LGTM!src/engine/previewer/previewer.ml (4)
141-175: LGTM!
176-221: LGTM!
265-268: LGTM!
289-303: LGTM!src/engine/previewer/previewer.mli (2)
16-27: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Verify all
create_previewercall sites.
save_coordinate,goto_loc, andcan_guiare required labeled arguments.previeweris a public library (slipshow.previewer). Every caller must pass the three new arguments, otherwise compilation fails.
40-41: LGTM!src/server/client/client.ml (2)
74-76: LGTM!
139-141: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that the served page always defines
window.can_gui.
Jv.get ... "can_gui" |> Jv.to_boolassumes the property exists. If the server template does not define it, the value isundefinedand the conversion result is not defined bybrr.Jv.to_option Jv.to_boolwith afalsedefault is safer.src/server/proto/proto.ml (1)
17-18: LGTM!Also applies to: 33-33
src/lspishow/roots.mli (1)
16-22: LGTM!src/lspishow/current_ast.ml (5)
23-43: LGTM!
47-53: LGTM!
63-93: LGTM!
95-231: LGTM!
233-267: LGTM!src/lspishow/current_ast.mli (1)
1-32: LGTM!src/lspishow/lspishow.ml (10)
44-63: LGTM!
97-111: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that this block does not conflict with
set_positionEncoding.Line 97 calls the inherited
self#set_positionEncoding i. Lines 98-111 then recomputepositionEncodingfrom the samei.capabilities.general.positionEncodingsfield. Iflinolalready performs this selection, the duplicated logic can diverge from the encoding thatlinoluses internally forText_document, and positions would be converted with one encoding while documents are indexed with another.
201-231: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the URI form used for
docslookup and forapplyEdit.Line 201 builds
uriwithLinol_lsp.Uri0.of_string (Cmarkit.Textloc.file textloc), which is a filesystem path and not afile://URI. Line 211 uses that same value as the key ofdocs, which is populated from clientdidOpenURIs. If the two forms differ,Hashtbl.find_opt docs urialways returnsNone, the buffer state is not updated, and the client receives an edit for a URI it may not resolve.
305-306: LGTM!
348-352: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that
unit.sourceis populated for every unit that these handlers can reach.Completion and definition now stop early when
unit.sourceisNone. If units that are read from the filesystem, for example the targets of{include}, keepsource = None, then go-to-definition and completion silently do nothing in those files. Hover usesbuffer.sourceinstead, so the two paths depend on different fields.Also applies to: 377-391
461-478: LGTM!
530-533: LGTM!
769-775: LGTM!
804-814: LGTM!
877-884: LGTM!src/server/proto/proto.mli (1)
15-19: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify serialization and client handling for the new protocol variants.
Save_gui_position,GotoLoc, andReplacechange the protocol contract. Confirm thatproto.mlserializes and parses each variant, and thatsrc/server/client/client.mlhandlesReplacewith bothSome idandNone.Also applies to: 34-34
src/server/dune (1)
16-19: LGTM!src/server/proto/dune (1)
3-3: LGTM!src/server/server.ml (1)
1-5: LGTM!Also applies to: 16-16, 39-45, 73-105, 144-147, 167-205, 224-226
src/server/slipshow_server.ml (1)
4-5: LGTM!Also applies to: 62-63
src/server/slipshow_server.mli (1)
1-5: LGTM!Also applies to: 29-35
src/lspishow/buffers.ml (1)
35-53: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the broadcast policy at every
Buffers.updatecall site.When a caller passes
should_broadcast:false,Roots.update_rootpreserves the current version and does not notify preview clients. Usetruefor source changes that must refresh the preview. Usefalseonly for flows that update the preview by another defined mechanism.src/lspishow/buffers.mli (1)
12-12: LGTM!src/lspishow/roots.ml (1)
18-31: LGTM!src/types/common_types.ml (1)
1-4: LGTM!src/types/dune (1)
1-6: LGTM!src/compiler/ast.ml (1)
130-152: LGTM!Also applies to: 376-491, 612-613
src/compiler/compile.ml (6)
311-320: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Use a shared constant for the
slipshow-gui-locattribute name.The attribute name is a hard-coded literal here, but the block path on Line 335 uses
Special_attrs.original_loc. The browser runtime reads this attribute throughCommon_types.Special_strings.gui_locinsrc/engine/runtime/gui/action.ml. If the two spellings ever diverge,Action.activatewith aLoctarget silently finds no element and GUI activation fails with no error.Define the name once in
src/types/special_strings.mland reference it from both sides.♻️ Proposed change
- let original_loc = - if embed_loc then - [ `Kv (("slipshow-gui-loc", Meta.none), Some (attr, Meta.none)) ] - else [] - in + let original_loc = + if embed_loc then + [ `Kv ((Special_attrs.gui_loc, Meta.none), Some (attr, Meta.none)) ] + else [] + inRun the following script to confirm the constant exists and that both sides agree:
311-314: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
⚠️ Unverified finding
Sandbox verification was unavailable.Do not use
Marshalfor a value that is persisted into compiled HTML.
Marshal.to_string loc []encodes an OCamlTextloc.tinto the compiled document.Marshaloutput is not a stable interchange format: it depends on the OCaml runtime version and on the exact type representation. A document compiled by one slipshow build and reopened by another can fail to decode.Marshal.from_stringon a value it did not produce is also memory-unsafe, so a hand-edited or stale attribute can crash or corrupt the reader instead of producing a clean error.Encode the location with an explicit textual format instead, for example
file:first_byte:last_byte:first_line:last_line, or a sexp through the existingsexplibdependency insrc/types/dune.The same pattern appears on Line 329 for
Special_attrs.original_loc.Run the following script to find the decoding side and confirm the exposure:
604-619: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.
gui_listis returned in reverse document order whileid_listis normalized.Line 608 applies
List.revtoid_list, soid_mapfollows document order.gui_listis returned as accumulated, which is reverse document order, and it becomesgui_mapin theAst.unit'record. Any consumer that treatsgui_mapas ordered, for example to pick the first GUI element or to render a list, sees the document backwards.🛠️ Proposed fix
let id_list = List.rev id_list in + let gui_list = List.rev gui_list inRun the following script to check how
gui_maporder is consumed:
679-698: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
⚠️ Unverified finding
Sandbox verification was unavailable.
units_cacheis not keyed onembed_loc, so a cached unit can carry the wrong embedded locations.
add_to_compilereuses a unit fromunits_cacheby path only. A cached unit was produced by an earlierunit ~embed_loccall. If a later compilation passes a differentembed_locand reuses the same cache, the reused unit keeps the earlier decision: it either lacks theslipshow-original-locattributes that GUI mode needs, or it keeps them when they were not requested. The result is GUI targets that cannot be resolved at runtime, with no error at compile time.Include
embed_locin the cache key, or invalidate the cache whenembed_locchanges.Run the following script to see whether any caller reuses a cache across different
embed_locvalues:
844-851: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.The reverse attribute mapping covers only six of the renamed action attributes.
Stage1.map_attrsrenames fourteen action attributes plus fourteenchildren:variants to-at-unpause. This reverse mapper restores onlyup,center,enter,down,exec, andscroll. Markdown produced byto_cmarkittherefore containsemph-at-unpause,focus-at-unpause,reveal-at-unpause,children:up-at-unpause, and the remaining variants instead of the source names. The generated markdown does not round-trip.The new
slipshow-gui-locandslipshow-original-locattributes are also passed through unchanged, so base64 location blobs can appear in converted markdown whenembed_locis enabled.Derive the reverse mapping from the same table used in
Stage1.map_attrs, and drop the two location attributes on this path.Run the following script to check whether the markdown conversion path enables
embed_loc:
718-779: LGTM!Also applies to: 855-866
vendor/github.com/panglesd/cmarkit/src/cmarkit.ml (1)
2049-2090: LGTM!Also applies to: 3512-3527
src/engine/runtime/drawing/drawing_state/drawing_state.ml (1)
8-8: LGTM!Also applies to: 216-220
src/engine/runtime/drawing/drawing_state/types.ml (1)
89-89: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that every consumer of
statushandlesGui_modeexplicitly.Adding
Gui_modetostatusis safe where the match is exhaustive, and the compiler enforces that. Matches that end in a wildcard silently classifyGui_modewith whatever the fallback does. In this cohort,gui/action.mlLine 37 andgui/controller.mlLine 52 look intentional, but the toolbars and the runtime controller are not part of this review.Run the following script to list every match on
statusand confirm the fallbacks are intentional:src/engine/runtime/gui/action.ml (1)
6-19: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Apply the DOM and state changes only after the status transition succeeds.
Drawing_state.Status.set Gui_modeis not always effective. The transition table indrawing_state.mlrejectsEditing -> Gui_modeandDrawing (Recording _) -> Gui_mode.activateguards the entry withDrawing Presenting | Gui_mode, so calls throughactivateare safe.activate_elis also reachable directly, for example from a click handler.If
activate_elruns while the status isEditingorRecording, the status stays unchanged but this function still setsState.current, changesState.statustoMove, and adds theslipshow-activatedclass. The element then looks selected whileDrawing_stateis not in GUI mode.Controller.shortcutsreturnsfalseoutsideGui_mode, soEscapedoes nothing, anddeactivatereturns early because its guard fails. The element stays highlighted with no way to clear it.Check the status inside
activate_el, or reuse the guard fromactivate.🛠️ Proposed fix
let activate_el el = match El.at !!"gui" el with | None -> () + | Some _ + when (match Drawing_state.Status.peek () with + | Drawing Presenting | Gui_mode -> false + | _ -> true) -> + () | Some _ ->Run the following script to list the callers of
activate_el:src/engine/runtime/gui/controller.ml (3)
55-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Ignore keyboard events that carry a modifier.
Brr.Ev.Keyboard.keyreturns the key value without modifier state.Ctrl+SandCmd+Sboth produce the key"s", soshortcut_editingswitches the tool toSelectand returnstrue.Ctrl+Rproduces"r"and switches toScale. The caller treatstrueas handled, so the browser accelerator and the application save shortcut stop working while GUI mode is active. This project has a save mode, soCtrl+Sis a realistic conflict.Keep
Shiftallowed, because Line 45 relies on the uppercase"G".🛠️ Proposed fix
let handle ev = - let key = ev |> Brr.Ev.as_type |> Brr.Ev.Keyboard.key |> Jstr.to_string in - if check_in_textarea () then false else shortcuts key + let kev = Brr.Ev.as_type ev in + (* Shift is allowed: the "G" shortcut depends on it. *) + if + Brr.Ev.Keyboard.ctrl_key kev + || Brr.Ev.Keyboard.meta_key kev + || Brr.Ev.Keyboard.alt_key kev + then false + else + let key = kev |> Brr.Ev.Keyboard.key |> Jstr.to_string in + if check_in_textarea () then false else shortcuts keyRun the following script to confirm the accessor names in the vendored Brr version and to find other shortcut handlers that already guard modifiers:
9-11: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Treating
buttonas editable blocks theEscapeexit path.
check_in_textareareturnstruefor a focusedbutton, sohandlereturnsfalseand no GUI shortcut runs. A button keeps focus after a click in every major browser. After the user clicks a GUI toolbar button,Escapetherefore no longer leaves GUI mode, and the only exit is another mouse click. Keyboard-only users lose the exit path.
EscapeandGdo not insert text, so they do not need the editable guard. Handle them before the check, or removebuttonfrom the list and rely on the browser default forSpaceandEnteron buttons.🛠️ Proposed fix
let handle ev = let key = ev |> Brr.Ev.as_type |> Brr.Ev.Keyboard.key |> Jstr.to_string in - if check_in_textarea () then false else shortcuts key + match key with + (* These keys never insert text, so they stay available while a control has + focus. *) + | "Escape" | "G" -> shortcuts key + | _ -> if check_in_textarea () then false else shortcuts keyRun the following script to check whether the GUI toolbar blurs its buttons after activation:
1-26: LGTM!Also applies to: 50-53
src/engine/runtime/gui/state.ml (1)
1-4: LGTM!src/types/special_strings.ml (1)
16-18: LGTM!Also applies to: 37-39
src/actions-arguments/actions_arguments.ml (1)
506-507: LGTM!src/communication/dune (1)
4-4: LGTM!src/compiler/checks.ml (1)
12-12: LGTM!src/engine/runtime/gui/types.ml (1)
1-1: LGTM!src/engine/runtime/gui/dune (1)
1-3: LGTM!src/engine/runtime/gui/gui.ml (1)
1-119: LGTM!src/engine/runtime/main.ml (1)
20-22: LGTM!src/engine/runtime/messaging/dune (1)
3-3: LGTM!src/compiler/dune (1)
16-17: LGTM!src/compiler/renderers.ml (1)
416-418: LGTM!src/engine/runtime/messaging/messaging.ml (1)
25-50: LGTM!src/engine/runtime/messaging/messaging.mli (1)
19-22: LGTM!src/engine/runtime/system.css (1)
108-113: LGTM!src/engine/runtime/toolbar/drawing_toolbar.ml (1)
1-4: LGTM!Also applies to: 101-101, 158-172, 203-234
src/engine/runtime/toolbar/dune (1)
1-3: LGTM!src/engine/runtime/browser/css.ml (1)
27-27: LGTM!src/engine/runtime/drawing/drawing_controller/ui.ml (1)
387-387: LGTM!src/engine/runtime/toolbar/editing_toolbar.ml (1)
1-73: LGTM!src/engine/runtime/toolbar/gui_toolbar.ml (1)
1-59: LGTM!src/engine/runtime/toolbar/toolbar.ml (1)
1-21: LGTM!src/engine/runtime/toolbar/widgets.ml (1)
1-50: LGTM!src/engine/runtime/controller.ml (2)
11-14: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Preserve editable-control keyboard behavior.
Gui.Controller.handlenow runs before the existing editable-control guard at Lines 15-16. Confirm that it returnsfalsefor focused editable elements. Otherwise, GUI shortcuts can change tools while the user types in an input, textarea, or contenteditable element.
76-79: LGTM!Also applies to: 251-269
src/engine/runtime/drawing/drawing_controller/controller.ml (1)
43-52: LGTM!Also applies to: 76-79, 105-105, 125-125
src/engine/runtime/drawing/drawing_controller/drawing.css (1)
17-20: LGTM!src/engine/runtime/drawing/drawing_controller/preview.ml (1)
354-356: LGTM!src/engine/runtime/drawing/drawing_controller/setup.ml (1)
68-95: LGTM!src/engine/runtime/dune (1)
17-18: LGTM!
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e70bd226-76ce-401a-9d57-8356801f2a19
📒 Files selected for processing (82)
docs/gui.rstdocs/index.rstdocs/shortcuts.rstdocs/special-elements.rstsrc/actions-arguments/actions_arguments.mlsrc/actions-arguments/gui.mlsrc/cli/run.mlsrc/cm_plugin/slipshow_communication.mlsrc/communication/README.mdsrc/communication/communication.mlsrc/communication/communication.mlisrc/communication/dunesrc/compiler/ast.mlsrc/compiler/checks.mlsrc/compiler/compile.mlsrc/compiler/compile.mlisrc/compiler/dunesrc/compiler/renderers.mlsrc/compiler/slipshow.mlsrc/compiler/slipshow.mlisrc/engine/previewer/dunesrc/engine/previewer/previewer.mlsrc/engine/previewer/previewer.mlisrc/engine/runtime/browser/css.mlsrc/engine/runtime/controller.mlsrc/engine/runtime/drawing/drawing_controller/controller.mlsrc/engine/runtime/drawing/drawing_controller/drawing.csssrc/engine/runtime/drawing/drawing_controller/preview.mlsrc/engine/runtime/drawing/drawing_controller/setup.mlsrc/engine/runtime/drawing/drawing_controller/ui.mlsrc/engine/runtime/drawing/drawing_controller/ui_widgets.mlsrc/engine/runtime/drawing/drawing_state/drawing_state.mlsrc/engine/runtime/drawing/drawing_state/types.mlsrc/engine/runtime/dunesrc/engine/runtime/gui/action.mlsrc/engine/runtime/gui/controller.mlsrc/engine/runtime/gui/dunesrc/engine/runtime/gui/gui.mlsrc/engine/runtime/gui/gui_tools.mlsrc/engine/runtime/gui/state.mlsrc/engine/runtime/gui/types.mlsrc/engine/runtime/main.mlsrc/engine/runtime/messaging/dunesrc/engine/runtime/messaging/messaging.mlsrc/engine/runtime/messaging/messaging.mlisrc/engine/runtime/system.csssrc/engine/runtime/toolbar/drawing_toolbar.mlsrc/engine/runtime/toolbar/dunesrc/engine/runtime/toolbar/editing_toolbar.mlsrc/engine/runtime/toolbar/gui_toolbar.mlsrc/engine/runtime/toolbar/toolbar.mlsrc/engine/runtime/toolbar/widgets.mlsrc/lspishow/buffers.mlsrc/lspishow/buffers.mlisrc/lspishow/current_ast.mlsrc/lspishow/current_ast.mlisrc/lspishow/debug.mlsrc/lspishow/diagnostic.mlsrc/lspishow/diagnostic.mlisrc/lspishow/lsp_preview.mlsrc/lspishow/lsp_preview.mlisrc/lspishow/lspishow.mlsrc/lspishow/roots.mlsrc/lspishow/roots.mlisrc/server/client/client.mlsrc/server/dunesrc/server/proto/dunesrc/server/proto/proto.mlsrc/server/proto/proto.mlisrc/server/server.mlsrc/server/slipshow_server.mlsrc/server/slipshow_server.mlisrc/types/common_types.mlsrc/types/dunesrc/types/special_strings.mlvendor/github.com/panglesd/cmarkit/src/cmarkit.mlvendor/github.com/panglesd/cmarkit/src/cmarkit.mlivendor/github.com/panglesd/lwd/examples/cbor/dunevendor/github.com/panglesd/lwd/examples/dunevendor/github.com/panglesd/lwd/lib/nottui-lwt/dunevendor/github.com/panglesd/lwd/lib/nottui-unix/dunevendor/github.com/panglesd/lwd/lib/nottui/dune
| let int (i, _loc) = | ||
| i |> int_of_string_opt |> result_of_option "Expected an integer" | ||
|
|
||
| let float (i, _loc) = | ||
| i |> float_of_string_opt |> result_of_option "Expected a float" | ||
|
|
||
| let parse s = | ||
| let res = | ||
| Parse.parse ~action_name:"gui" | ||
| ~named: | ||
| [ ("x", int); ("y", int); ("scale", float); ("w", int); ("h", int) ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent invalid GUI geometry from reaching saved state.
Gui.parse accepts zero and negative scale or dimensions. dimension can also persist negative width and height. Invalid CSS dimensions are ignored, while the invalid values remain in the saved GUI attribute. This makes the rendered element diverge from saved state.
src/actions-arguments/gui.ml#L11-L21: reject non-positivescale,width, andheightvalues.src/engine/runtime/gui/gui_tools.ml#L180-L184: clamp interactive width and height to a positive minimum before applying and sending coordinates.
📍 Affects 2 files
src/actions-arguments/gui.ml#L11-L21(this comment)src/engine/runtime/gui/gui_tools.ml#L180-L184
| - The previewer receives all communication values and handles "State", | ||
| "`open/close_recording_panel`", "`open/close_speaker_notes`", "`Ready`", | ||
| "`Save_drawing`" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the GUI payload flow.
Lines 20-22 state that the previewer receives all communication values, but they omit Can_gui, ActivateGUI, DeActivateGUI, SaveCoordinates, and GotoLoc. Document the sender and receiver for each new payload, or remove the universal statement.
| let rec fold_just_units f acc unit units = | ||
| let acc = f unit acc in | ||
| Fpath.Map.fold | ||
| (fun fpath _ acc -> | ||
| match Fpath.Map.find_opt fpath units with | ||
| | None -> acc | ||
| | Some unit -> fold_just_units f acc unit units) | ||
| unit.deps acc |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a visited set to fold_just_units to prevent infinite recursion on include cycles.
fold_just_units recurses into every dependency of every unit without tracking visited paths. Compile.add_to_compile deliberately tolerates cycles: it returns early when a file is already in units, so units can contain a.md -> b.md and b.md -> a.md. In that case fold_just_units recurses forever and the compiler overflows the stack.
🛠️ Proposed fix using a visited set
- let rec fold_just_units f acc unit units =
- let acc = f unit acc in
- Fpath.Map.fold
- (fun fpath _ acc ->
- match Fpath.Map.find_opt fpath units with
- | None -> acc
- | Some unit -> fold_just_units f acc unit units)
- unit.deps acc
+ let fold_just_units f acc unit units =
+ let rec go visited acc unit =
+ let acc = f unit acc in
+ Fpath.Map.fold
+ (fun fpath _ (visited, acc) ->
+ if Fpath.Set.mem fpath visited then (visited, acc)
+ else
+ match Fpath.Map.find_opt fpath units with
+ | None -> (visited, acc)
+ | Some unit -> go (Fpath.Set.add fpath visited) acc unit)
+ unit.deps (visited, acc)
+ in
+ snd (go (Fpath.Set.singleton unit.path) acc unit)Note: the accumulator threading must carry visited, as shown, so that siblings also share it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let rec fold_just_units f acc unit units = | |
| let acc = f unit acc in | |
| Fpath.Map.fold | |
| (fun fpath _ acc -> | |
| match Fpath.Map.find_opt fpath units with | |
| | None -> acc | |
| | Some unit -> fold_just_units f acc unit units) | |
| unit.deps acc | |
| let fold_just_units f acc unit units = | |
| let rec go visited acc unit = | |
| let acc = f unit acc in | |
| Fpath.Map.fold | |
| (fun fpath _ (visited, acc) -> | |
| if Fpath.Set.mem fpath visited then (visited, acc) | |
| else | |
| match Fpath.Map.find_opt fpath units with | |
| | None -> (visited, acc) | |
| | Some unit -> go (Fpath.Set.add fpath visited) acc unit) | |
| unit.deps (visited, acc) | |
| in | |
| snd (go (Fpath.Set.singleton unit.path) acc unit) |
| let prefix, textloc, suffix = | ||
| match value with | ||
| | None -> | ||
| let textloc = Cmarkit.Meta.textloc meta_key in | ||
| let byte, line = | ||
| ( Cmarkit.Textloc.last_byte textloc, | ||
| Cmarkit.Textloc.last_line textloc ) | ||
| in | ||
| let textloc = | ||
| textloc | ||
| |> Cmarkit.Textloc.set_first ~first_byte:(byte + 1) | ||
| ~first_line:line | ||
| |> Cmarkit.Textloc.set_last ~last_byte:byte ~last_line:line | ||
| in | ||
| ("=\"", textloc, "\"") | ||
| | Some (_value, meta) -> ("", Cmarkit.Meta.textloc meta, "") | ||
| in | ||
| let uri = Linol_lsp.Uri0.of_string (Cmarkit.Textloc.file textloc) in | ||
| let fpath = Fpath.v (Cmarkit.Textloc.file textloc) in | ||
| let> source = Fpath.Map.find_opt fpath root.units.units in | ||
| let> source = source.source in | ||
| let range = | ||
| Diagnostic.linoloc_of_textloc ~positionEncoding ~source textloc | ||
| in | ||
| let newText = prefix ^ coord ^ suffix in |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate coord before you insert it into the document.
coord is a raw string from the preview client. Line 208 concatenates it into the document text, and in the None branch it lands inside a double-quoted attribute value. A coord that contains ", a newline, or } corrupts the attribute block of the user's file. The server must not trust the client string.
Parse coord with the Actions_arguments.Gui parser, or send the numeric fields in Proto.Client_to_server.Save_gui_position and format them on the server.
| Hashtbl.find_opt roots root | ||
| in | ||
| let* buffer = Hashtbl.find_opt Buffers.buffers path in | ||
| let _ = self#activate_gui params.position path root buffer in |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Broadcast GUI activation only when the target changes.
on_req_document_highlight runs on every cursor move in most editors. Each call broadcasts ActivateGUI or DeActivateGUI on root.condition, so every waiting long-poll client wakes up and the previewer re-enters or re-leaves GUI mode even when the target is unchanged. Store the last broadcast target per root and broadcast only on a change.
let _ = ... also hides that this lookup-only handler performs a side effect. Use let () = ... to make that explicit.
| let save_coordinate ~id ~coord = | ||
| let coord = Actions_arguments.Gui.to_string coord in | ||
| let _ = proto_request uri (Save_gui_position { id; coord }) in | ||
| () | ||
| in | ||
| let goto_loc s = | ||
| let _ = proto_request uri (GotoLoc s) in | ||
| () | ||
| in |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Report failures of Save_gui_position and GotoLoc to the user.
Both callbacks discard the proto_request future. If the request fails, the GUI position edit is lost and the user gets no feedback. The preview still shows the moved element, so the editor and the preview diverge silently. Previewer.notify is available for this purpose.
🛠️ Proposed handling of request failures
let save_coordinate ~id ~coord =
let coord = Actions_arguments.Gui.to_string coord in
- let _ = proto_request uri (Save_gui_position { id; coord }) in
+ let _ : unit Fut.t =
+ let open Fut.Syntax in
+ let+ res = proto_request uri (Save_gui_position { id; coord }) in
+ match res with
+ | Ok () -> ()
+ | Error e ->
+ Console.error [ e ];
+ Option.iter
+ (fun p -> Previewer.notify p "Could not save the GUI position")
+ !previewer
+ in
()
in
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lspishow/lspishow.ml (1)
393-406: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the definition file source for the returned range.
loccan refer to a definition in an included file. Line 393 obtains the request document source, but Line 406 uses it for the destination range. The returned URI can therefore point to one file while its range uses offsets from another file.After resolving
file, obtain that unit'ssourceand pass it toDiagnostic.linoloc_of_textloc. Keep the request source only forCurrent_ast.get_target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dffac76d-fcb7-49ee-a998-81818b88146d
📒 Files selected for processing (16)
src/actions-arguments/dunesrc/actions-arguments/gui.mlsrc/communication/README.mdsrc/compiler/compile.mlsrc/compiler/compile.mlisrc/engine/runtime/controller.mlsrc/engine/runtime/drawing/drawing_controller/ui_widgets.mlsrc/engine/runtime/gui/action.mlsrc/engine/runtime/gui/gui.mlsrc/engine/runtime/system.csssrc/engine/runtime/toolbar/gui_toolbar.mlsrc/lspishow/lsp_preview.mlsrc/lspishow/lsp_preview.mlisrc/lspishow/lspishow.mlvendor/github.com/panglesd/brr/src/brr.mlvendor/github.com/panglesd/brr/src/brr.mli
🚧 Files skipped from review as they are similar to previous changes (10)
- src/engine/runtime/controller.ml
- src/engine/runtime/drawing/drawing_controller/ui_widgets.ml
- src/engine/runtime/toolbar/gui_toolbar.ml
- src/communication/README.md
- src/engine/runtime/system.css
- src/compiler/compile.mli
- src/actions-arguments/gui.ml
- src/engine/runtime/gui/action.ml
- src/engine/runtime/gui/gui.ml
- src/compiler/compile.ml
c538706 to
9651ab4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/compiler/ast.ml (1)
188-195: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
fold_just_unitsstill recurses without a visited set.
add_to_compileinsrc/compiler/compile.mlreturns the cached unit when the file is already inunits, sounitscan hold mutually including files (a.md -> b.mdandb.md -> a.md). In that case this recursion never terminates and the compiler overflows the stack. Thread aFpath.Set.tof visited paths through the fold.🛠️ Proposed fix using a visited set
- let rec fold_just_units f acc unit units = - let acc = f unit acc in - Fpath.Map.fold - (fun fpath _ acc -> - match Fpath.Map.find_opt fpath units with - | None -> acc - | Some unit -> fold_just_units f acc unit units) - unit.deps acc + let fold_just_units f acc unit units = + let rec go visited acc unit = + let acc = f unit acc in + Fpath.Map.fold + (fun fpath _ (visited, acc) -> + if Fpath.Set.mem fpath visited then (visited, acc) + else + match Fpath.Map.find_opt fpath units with + | None -> (visited, acc) + | Some unit -> go (Fpath.Set.add fpath visited) acc unit) + unit.deps (visited, acc) + in + snd (go (Fpath.Set.singleton unit.path) acc unit)src/lspishow/lspishow.ml (2)
207-234: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
coordis still inserted into the document without validation.Line 234 concatenates the client-supplied
coordintonewText. In theNonebranch the value lands inside a double-quoted attribute value. Acoordthat contains", a newline, or}corrupts the attribute block of the user's file. Parsecoordwith theActions_arguments.Guiparser before use, or transmit numeric fields inProto.Client_to_server.Save_gui_positionand format them on the server.
521-534: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe selected AST and the source text can come from different revisions.
Lines 522-527 select
Roots.savedunless the refresh mode isEdit. InSavemode theCmarkit.Textlocoffsets ofastrefer to the last saved text. Line 533 and Line 566 then convert those offsets with~source:buffer.source, which is the current buffer text. When the buffer has unsaved edits, the offsets and the source disagree, and the highlight ranges point at wrong positions. Use the source of the unit inside the selected root.
🧹 Nitpick comments (3)
src/lspishow/buffers.mli (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
should_broadcastand its interaction withforce.The doc comment describes
forceonly. Insrc/lspishow/buffers.mlthe root update runs only whenforceis false, soshould_broadcasthas no effect whenforceis true. Callers such assrc/lspishow/lspishow.mlLine 813 pass both flags as true. State this in the interface so callers do not expect a broadcast.📝 Proposed doc update
val update : force:bool -> should_broadcast:bool -> Fpath.t -> string -> unit -(** Update a buffer to contain a source. If [force] is true, force the - recomputation of the buffer, but don't update root. *) +(** Update a buffer to contain a source. If [force] is true, force the + recomputation of the buffer and do not update the roots; [should_broadcast] + is then unused. If [force] is false, update the roots and broadcast the + change when [should_broadcast] is true. *)src/lspishow/lspishow.ml (1)
803-819: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the ordering of the two root updates explicit.
Lines 803-818 bind two root updates with
let ... and .... OCaml does not specify the evaluation order ofandbindings, and both bindings perform side effects: the first updatesRoots.saved, and the second refreshes every open buffer and then updatesRoots.buffers. The refresh order is therefore unspecified. Use sequentiallet () = ... instatements so the order is defined.src/compiler/compile.ml (1)
314-325: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUse
Common_types.Special_strings.gui_locfor the emitted attribute name.The literal matches the shared constant, and
checks.mlalready accepts it throughSpecial_strings.all_attrs. Use the constant to prevent future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46a38dc4-75cc-402f-a271-09dad5299316
📒 Files selected for processing (18)
src/compiler/ast.mlsrc/compiler/compile.mlsrc/compiler/renderers.mlsrc/compiler/slipshow.mlsrc/compiler/slipshow.mlisrc/engine/previewer/previewer.mlisrc/engine/runtime/drawing/drawing_controller/controller.mlsrc/lspishow/buffers.mlsrc/lspishow/buffers.mlisrc/lspishow/diagnostic.mlsrc/lspishow/lsp_preview.mlsrc/lspishow/lsp_preview.mlisrc/lspishow/lspishow.mlsrc/server/server.mlsrc/server/slipshow_server.mlsrc/server/slipshow_server.mlivendor/github.com/panglesd/cmarkit/src/cmarkit.mlvendor/github.com/panglesd/cmarkit/src/cmarkit.mli
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| let handle ev = | ||
| let key = ev |> Brr.Ev.as_type |> Brr.Ev.Keyboard.key |> Jstr.to_string in | ||
| if check_in_textarea () then false else shortcuts key | ||
| shortcuts key |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the editable-element guard before shortcut dispatch.
handle now dispatches keys while an input, textarea, select, button, or content-editable element has focus. For example, Delete can remove selected drawings, and arrow keys can change replay time while the user edits a control. Use check_in_textarea () before shortcuts key.
Proposed fix
let handle ev =
let key = ev |> Brr.Ev.as_type |> Brr.Ev.Keyboard.key |> Jstr.to_string in
- shortcuts key
+ if check_in_textarea () then false else shortcuts key📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let handle ev = | |
| let key = ev |> Brr.Ev.as_type |> Brr.Ev.Keyboard.key |> Jstr.to_string in | |
| if check_in_textarea () then false else shortcuts key | |
| shortcuts key | |
| let handle ev = | |
| let key = ev |> Brr.Ev.as_type |> Brr.Ev.Keyboard.key |> Jstr.to_string in | |
| if check_in_textarea () then false else shortcuts key |
| let _ : unit Lwt.t = | ||
| let open Lwt.Syntax in | ||
| let* () = | ||
| match Hashtbl.find_opt docs uri with | ||
| | Some st -> | ||
| let open Linol_lsp in | ||
| let open Linol_lwt in | ||
| let old_doc = | ||
| Text_document.make ~position_encoding:positionEncoding | ||
| (DidOpenTextDocumentParams.create | ||
| ~textDocument: | ||
| (TextDocumentItem.create ~languageId:st.languageId | ||
| ~uri ~version:st.version ~text:st.content)) | ||
| in | ||
| let new_doc = | ||
| Text_document.apply_text_document_edits old_doc | ||
| [ { newText; range } ] | ||
| in | ||
| let contents = Text_document.text new_doc in | ||
| let file = uri |> DocumentUri.to_path |> Fpath.v in | ||
| State.update_from_buffer ~force:false file contents | ||
| ~should_broadcast:false | ||
| | None -> Lwt.return () | ||
| in | ||
| let+ _ : Linol_jsonrpc.Jsonrpc.Id.t = | ||
| notify_back#send_request | ||
| (WorkspaceApplyEdit | ||
| { | ||
| edit = | ||
| { | ||
| changeAnnotations = None; | ||
| changes = Some [ (uri, [ { newText; range } ]) ]; | ||
| documentChanges = None; | ||
| }; | ||
| label = None; | ||
| }) | ||
| (fun _ -> Lwt.return ()) | ||
| in | ||
| () | ||
| in |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The workspace/applyEdit result is still ignored, and local state is updated first.
Line 255 updates the server buffer model before the request is sent. The response callback at Line 271 discards the result. If the client reports applied = false, the server keeps a document version that the client never had, and no didChange follows to correct it. The discarded unit Lwt.t also turns any failure inside this block into an unhandled Lwt rejection. Check the applied field, and call State.update_from_buffer only after the client confirms the edit.
| | GotoLoc s -> | ||
| let ( let> ) x f = | ||
| match x with | ||
| | Error (`Msg s) -> | ||
| Format.eprintf "Error in handling of gotoloc: %s" s | ||
| | Ok x -> f x | ||
| in | ||
| let> s = Base64.decode s in | ||
| let textloc : Cmarkit.Textloc.t = Marshal.from_string s 0 in | ||
| let fpath = Fpath.v (Cmarkit.Textloc.file textloc) in |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
GotoLoc still calls Marshal.from_string without a guard.
The Save_gui_position branch now wraps unmarshalling in try ... with at Lines 189-192. Line 290 does not. Marshal performs no type check, so malformed bytes from the preview client raise Failure or Invalid_argument out of this handler, and crafted bytes can yield a value that is not a Cmarkit.Textloc.t. Guard the call and drop the message on failure.
🛠️ Proposed fix
let> s = Base64.decode s in
- let textloc : Cmarkit.Textloc.t = Marshal.from_string s 0 in
+ let> (textloc : Cmarkit.Textloc.t) =
+ try Ok (Marshal.from_string s 0)
+ with _ -> Error (`Msg "Error during unmarshalling")
+ in| Inline.Autolink ((a, attrs), merge_loc meta) :: q | ||
| | Inline.Code_span ((a, _old_attrs), meta) :: q -> | ||
| Inline.Code_span ((a, attrs), meta) :: q | ||
| Inline.Code_span ((a, attrs), merge_loc meta) :: q | ||
| | Inline.Emphasis ((a, _old_attrs), meta) :: q -> | ||
| Inline.Emphasis ((a, attrs), meta) :: q | ||
| Inline.Emphasis ((a, attrs), merge_loc meta) :: q | ||
| | Inline.Image ((a, _old_attrs), meta) :: q -> | ||
| Inline.Image ((a, attrs), meta) :: q | ||
| Inline.Image ((a, attrs), merge_loc meta) :: q | ||
| | Inline.Link ((a, _old_attrs), meta) :: q -> | ||
| Inline.Link ((a, attrs), meta) :: q | ||
| Inline.Link ((a, attrs), merge_loc meta) :: q | ||
| | Inline.Strong_emphasis ((a, _old_attrs), meta) :: q -> | ||
| Inline.Strong_emphasis ((a, attrs), meta) :: q | ||
| Inline.Strong_emphasis ((a, attrs), merge_loc meta) :: q | ||
| | Inline.Text ((a, _old_attrs), meta) :: q -> | ||
| Inline.Text ((a, attrs), meta) :: q | ||
| Inline.Text ((a, attrs), merge_loc meta) :: q | ||
| (* let t = Inline.Attributes_span.make i attrs in *) | ||
| (* Inline.Ext_attrs (t, Meta.none) :: q *) | ||
| | Inline.Ext_strikethrough ((a, _old_attrs), meta) :: q -> | ||
| Inline.Ext_strikethrough ((a, attrs), meta) :: q | ||
| Inline.Ext_strikethrough ((a, attrs), merge_loc meta) :: q | ||
| | Inline.Ext_math_span ((a, _old_attrs), meta) :: q -> | ||
| Inline.Ext_math_span ((a, attrs), meta) :: q | ||
| Inline.Ext_math_span ((a, attrs), merge_loc meta) :: q |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle attached attributes after Inline.Ext_attrs.
When an attached attribute follows an existing Inline.Ext_attrs, the fallback creates an empty standalone attribute span. For example, a second attribute group after a link attribute group does not apply to the link.
Add an Inline.Ext_attrs branch that replaces its attributes and extends its metadata location.
Proposed fix
| Inline.Ext_math_span ((a, _old_attrs), meta) :: q ->
Inline.Ext_math_span ((a, attrs), merge_loc meta) :: q
+ | Inline.Ext_attrs ({ content; _ }, meta) :: q ->
+ Inline.Ext_attrs ({ content; attrs }, merge_loc meta) :: q
| _ ->📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Inline.Autolink ((a, attrs), merge_loc meta) :: q | |
| | Inline.Code_span ((a, _old_attrs), meta) :: q -> | |
| Inline.Code_span ((a, attrs), meta) :: q | |
| Inline.Code_span ((a, attrs), merge_loc meta) :: q | |
| | Inline.Emphasis ((a, _old_attrs), meta) :: q -> | |
| Inline.Emphasis ((a, attrs), meta) :: q | |
| Inline.Emphasis ((a, attrs), merge_loc meta) :: q | |
| | Inline.Image ((a, _old_attrs), meta) :: q -> | |
| Inline.Image ((a, attrs), meta) :: q | |
| Inline.Image ((a, attrs), merge_loc meta) :: q | |
| | Inline.Link ((a, _old_attrs), meta) :: q -> | |
| Inline.Link ((a, attrs), meta) :: q | |
| Inline.Link ((a, attrs), merge_loc meta) :: q | |
| | Inline.Strong_emphasis ((a, _old_attrs), meta) :: q -> | |
| Inline.Strong_emphasis ((a, attrs), meta) :: q | |
| Inline.Strong_emphasis ((a, attrs), merge_loc meta) :: q | |
| | Inline.Text ((a, _old_attrs), meta) :: q -> | |
| Inline.Text ((a, attrs), meta) :: q | |
| Inline.Text ((a, attrs), merge_loc meta) :: q | |
| (* let t = Inline.Attributes_span.make i attrs in *) | |
| (* Inline.Ext_attrs (t, Meta.none) :: q *) | |
| | Inline.Ext_strikethrough ((a, _old_attrs), meta) :: q -> | |
| Inline.Ext_strikethrough ((a, attrs), meta) :: q | |
| Inline.Ext_strikethrough ((a, attrs), merge_loc meta) :: q | |
| | Inline.Ext_math_span ((a, _old_attrs), meta) :: q -> | |
| Inline.Ext_math_span ((a, attrs), meta) :: q | |
| Inline.Ext_math_span ((a, attrs), merge_loc meta) :: q | |
| Inline.Autolink ((a, attrs), merge_loc meta) :: q | |
| | Inline.Code_span ((a, _old_attrs), meta) :: q -> | |
| Inline.Code_span ((a, attrs), merge_loc meta) :: q | |
| | Inline.Emphasis ((a, _old_attrs), meta) :: q -> | |
| Inline.Emphasis ((a, attrs), merge_loc meta) :: q | |
| | Inline.Image ((a, _old_attrs), meta) :: q -> | |
| Inline.Image ((a, attrs), merge_loc meta) :: q | |
| | Inline.Link ((a, _old_attrs), meta) :: q -> | |
| Inline.Link ((a, attrs), merge_loc meta) :: q | |
| | Inline.Strong_emphasis ((a, _old_attrs), meta) :: q -> | |
| Inline.Strong_emphasis ((a, attrs), merge_loc meta) :: q | |
| | Inline.Text ((a, _old_attrs), meta) :: q -> | |
| Inline.Text ((a, attrs), merge_loc meta) :: q | |
| (* let t = Inline.Attributes_span.make i attrs in *) | |
| (* Inline.Ext_attrs (t, Meta.none) :: q *) | |
| | Inline.Ext_strikethrough ((a, _old_attrs), meta) :: q -> | |
| Inline.Ext_strikethrough ((a, attrs), merge_loc meta) :: q | |
| | Inline.Ext_math_span ((a, _old_attrs), meta) :: q -> | |
| Inline.Ext_math_span ((a, attrs), merge_loc meta) :: q | |
| | Inline.Ext_attrs ({ content; _ }, meta) :: q -> | |
| Inline.Ext_attrs ({ content; attrs }, merge_loc meta) :: q |
To better explain what events are sent where, as it can be a bit complex!
Found by adding an mli, that I'll commit later
If you are an LLM reviewing this code, like coderabbit, don't forget to check that I have fixed what the commetnt is about
Add a special element, gui, which is absolutely positioned. Add some communication so that when the cursor is positioned on the gui attribute, it is given a class on the preview side.
Add toolbar, some other things, and make it so you can click on any block to go to the loc
Instead of locs (which changes when we add something before) we use the order in which the gui are defined in a file
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/engine/runtime/gui/gui.ml (1)
99-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParse failures of the
guiattribute are discarded without any signal.Line 106 maps a parse error to an all-
Nonecoordinate, and Line 105 drops the warnings from the successful parse. The element then renders at its default position and the author receives no feedback. Log the error and the warnings to the console.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b475fe20-0e24-4855-8120-3698366edb99
📒 Files selected for processing (9)
src/compiler/ast.mlsrc/compiler/compile.mlsrc/engine/runtime/gui/action.mlsrc/engine/runtime/gui/gui.mlsrc/engine/runtime/gui/gui_tools.mlsrc/engine/runtime/toolbar/gui_toolbar.mlsrc/lspishow/lspishow.mlsrc/types/common_types.mlsrc/types/special_strings.ml
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| let () = | ||
| match Lwd.peek State.current with | ||
| | Some old_el when old_el <> el -> | ||
| El.set_class activate_class false old_el | ||
| | _ -> () | ||
| in |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for an equality function on El.t in the vendored Brr.
set -euo pipefail
fd -t f 'brr.mli' vendor --exec rg -n -C 3 'module El|val equal|val to_jv' {}Repository: panglesd/slipshow
Length of output: 443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- vendored Brr interface files ---'
fd -t f 'brr\.(mli|ml)$' vendor --exec sh -c '
for f do
echo "### $f"
rg -n -C 8 "module El|type t|val equal|val to_jv|equal" "$f" || true
done
' sh
echo '--- Jv definitions and comparison-related declarations ---'
rg -n -S -C 8 "type t|module Jv|val equal|equal.*Jv|compare|polymorphic" vendor src -g '*.ml' -g '*.mli' || true
echo '--- relevant action implementation and El.t uses ---'
cat -n src/engine/runtime/gui/action.ml
rg -n -S -C 4 "El\.t|El\.equal|Jv\.equal|Jv\.to|<>|==|State\.current" src vendor -g '*.ml' -g '*.mli' || trueRepository: panglesd/slipshow
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate vendored Brr/Jv files ---'
git ls-files vendor | rg '/(brr|jv)(\.(mli|ml)|/)' | head -n 100
echo '--- exact equality declarations in vendor, excluding generated/static data ---'
rg -n -S --glob '!**/static_data/**' --glob '!**/data_contents.ml' \
'(^|[[:space:]])(val equal|let equal|type t|module El|module Jv|val to_jv|val of_jv|val phys_equal|val compare)' vendor \
| head -n 300
echo '--- Brr interface section around module El ---'
brr_mli=$(fd -t f -i 'brr.mli' vendor | head -n 1)
if [ -n "${brr_mli}" ]; then
echo "### ${brr_mli}"
sed -n '2035,2250p' "${brr_mli}"
fi
echo '--- source references to equality and current state ---'
rg -n -S --glob '!**/static_data/**' \
'El\.equal|Jv\.equal|Jv\.t|State\.current|old_el <> el|old_el == el' src vendorRepository: panglesd/slipshow
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
jv_mli=vendor/github.com/panglesd/brr/src/jv.mli
jv_ml=vendor/github.com/panglesd/brr/src/jv.ml
brr_ml=vendor/github.com/panglesd/brr/src/brr.ml
action_ml=src/engine/runtime/gui/action.ml
echo '--- Jv interface: type, conversions, equality, comparison ---'
rg -n -C 12 'type t|module CONV|module Id|val equal|val compare|val to_jv|val of_jv|physical|phys' "$jv_mli"
echo '--- Jv implementation: type and equality/comparison functions ---'
rg -n -C 16 'type t|let equal|let compare|external.*equal|external.*compare|phys_equal|obj_eq|strict_eq|===|compare' "$jv_ml"
echo '--- Brr El implementation and signature references ---'
rg -n -C 12 'module El|type t|include.*Jv\.CONV|equal|of_jv|to_jv' "$brr_ml" | head -n 250
echo '--- action implementation and imports ---'
cat -n "$action_ml"Repository: panglesd/slipshow
Length of output: 24063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- js_of_ocaml version and build configuration ---'
rg -n -S --glob '!**/static_data/**' --glob '!**/data_contents.ml' \
'js_of_ocaml|jsoo|ocaml_version|dune' dune-project '*.opam' '**/dune' '**/dune-project' 2>/dev/null | head -n 250 || true
echo '--- equality operators used with Jv/El values ---'
rg -n -S --glob '!**/static_data/**' --glob '!**/data_contents.ml' \
'(^|[^[:alnum:]_])(==|!=|<>)([^=]|$)|Jv\.(equal|strict_equal)|El\.equal' src vendor/github.com/panglesd/brr \
| head -n 250
echo '--- local documentation mentioning polymorphic comparison or physical equality ---'
rg -n -i -S --glob '!**/static_data/**' --glob '!**/data_contents.ml' \
'polymorphic compare|physical equality|structural equality|functional value|Invalid_argument|strict_equal|JavaScript.*equal' \
vendor/github.com/panglesd/brr src | head -n 250 || trueRepository: panglesd/slipshow
Length of output: 23860
🌐 Web query:
js_of_ocaml polymorphic compare JavaScript object Jv.t Invalid_argument functional value physical equality
💡 Result:
In js_of_ocaml, unexpected behavior or errors involving polymorphic comparison and JavaScript object values (Jv.t) often stem from differences between OCaml's structural equality and JavaScript's equality semantics [1][2]. Polymorphic Comparison and Jv.t The OCaml structural equality operator ( =) is implemented to follow OCaml semantics [1][2]. When applied to JavaScript values (such as Jv.t), its behavior is not always intuitive or fully documented, as it relies on the underlying caml_equal runtime implementation, which is designed for OCaml data structures [1][2]. If this comparison attempts to traverse or compare internal components of a JavaScript object in a way that is incompatible with OCaml's expectation of "functional" data values, it can trigger runtime errors like Invalid_argument [1][2]. Physical Equality vs. Structural Equality To reliably compare JavaScript values, you should use the equality operators provided by the Jv or Js modules rather than OCaml's generic polymorphic operators [1][2]: 1. Jv.strict_equal (or Js.strict_equals): Corresponds directly to JavaScript's strict equality operator ( ===), which compares values for physical equality (same value and same type) [3][4][1]. This is the recommended approach for comparing Jv.t values [1][2]. 2. Jv.equal (or Js.equals): Corresponds to JavaScript's loose equality operator ( ==), which performs type coercion before comparison [3][1][2]. 3. OCaml Physical Equality ( ==): In js_of_ocaml, this is specifically compiled to JavaScript's strict equality ( ===) [1][2]. Summary Table OCaml Operator | Compiled JavaScript | Recommended Use ( =) | caml_equal | OCaml data types; avoid for Jv.t [1][2] ( ==) | === | Physical equality for any value [1][2] Jv.strict_equal | === | Recommended for Jv.t [1][2] Jv.equal | == | Loose equality [1][2] If you encounter an Invalid_argument exception during comparison, it is likely because the polymorphic comparison function is attempting to operate on a JavaScript value (like a function or a complex object) that cannot be structurally compared using OCaml's standard internal equality logic [5][1][6]. Always prefer ( ==) or Jv.strict_equal when working with foreign JavaScript objects [1][2].
Citations:
- 1: https://jon.recoil.org/reference/brr/ffi_manual.html
- 2: https://ocaml.org/p/brr/latest/ffi_manual.html
- 3: https://ocsigen.org/js_of_ocaml/latest/js_of_ocaml/Js_of_ocaml/Js/index.html
- 4: https://ocaml.org/p/js_of_ocaml/6.3.2/doc/js_of_ocaml/Js_of_ocaml/Js/index.html
- 5: https://ocsigen.org/js_of_ocaml/latest/js_of_ocaml/overview.html
- 6: https://ocsigen.org/js_of_ocaml/latest/manual/runtime-representation
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
mli = Path("vendor/github.com/panglesd/brr/src/brr.mli").read_text()
jv_mli = Path("vendor/github.com/panglesd/brr/src/jv.mli").read_text()
manual = Path("vendor/github.com/panglesd/brr/doc/ffi_manual.mld").read_text()
brr = Path("vendor/github.com/panglesd/brr/src/brr.ml").read_text()
action = Path("src/engine/runtime/gui/action.ml").read_text()
el_start = mli.index("module El : sig")
el_end = mli.index("\nend", el_start)
el_sig = mli[el_start:el_end]
assert "include Jv.CONV with type t := t" in el_sig
assert "val equal" not in el_sig
assert "val strict_equal : t -> t -> bool" in jv_mli
assert "external equal : t -> t -> bool = \"caml_js_equals\"" in jv_mli
assert "The OCaml [( == )] physical equality is compiled to JavaScript's" in manual
assert "e == e'" in brr
assert "old_el <> el" in action
print("Brr.El.t includes Jv.CONV: yes")
print("Brr.El.equal: absent")
print("Jv.strict_equal: present")
print("Jv.equal: JavaScript ==, not strict identity")
print("Brr physical equality mapping: OCaml == -> JavaScript ===")
print("Existing Brr DOM identity comparisons use ==: yes")
print("Current polymorphic comparison: old_el <> el")
PY
node - <<'JS'
const node = {};
const sameWrapper = node;
const differentObject = {};
if (!(node === sameWrapper)) throw new Error("=== failed for the same object");
if (node === differentObject) throw new Error("=== equated different objects");
console.log("JavaScript === preserves object identity");
JS
sed -n '60,92p' vendor/github.com/panglesd/brr/doc/ffi_manual.mld
sed -n '1448,1472p' vendor/github.com/panglesd/brr/src/brr.mlRepository: panglesd/slipshow
Length of output: 2451
Use physical equality for El.t values.
El.equal is not available in the vendored Brr. Replace old_el <> el with old_el != el; Brr maps (==) to JavaScript ===, which preserves DOM-node identity and avoids polymorphic comparison on JavaScript values.
| let () = | ||
| match i.capabilities.general with | ||
| | Some { positionEncodings = Some el; _ } -> | ||
| let encoding = | ||
| if | ||
| List.exists | ||
| (function PositionEncodingKind.UTF8 -> true | _ -> false) | ||
| el | ||
| then `UTF8 | ||
| else `UTF16 | ||
| in | ||
| positionEncoding <- encoding | ||
| | _ -> () | ||
| in |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
LSP specification positionEncodings client capability server must pick supported encoding utf-16 default
💡 Result:
In the Language Server Protocol (LSP), position encoding negotiation occurs during the initialize request to ensure the client and server interpret character offsets (e.g., in text edits) identically [1][2]. The process works as follows: 1. Client Capability: The client lists its supported position encodings in the general.positionEncodings capability [1][2]. This list is provided in order of preference (most preferred at index 0) [2]. 2. Server Selection: The server must pick one of the encodings offered by the client and return that choice in the capabilities.positionEncoding field of the initialize result [1][2]. 3. Default Behavior: - If the server omits capabilities.positionEncoding in its initialize result, the protocol defaults to utf-16 [1][2]. - If a client does not provide the general.positionEncodings capability, the server must assume the client supports utf-16 [1][2]. - utf-16 is the mandatory, historical default encoding for LSP [1][2][3]. Even when negotiation is used, utf-16 remains the only encoding that every client is required to support [2][3]. When a server picks an encoding, it must ensure that all subsequent text document positions and ranges are calculated according to that chosen encoding (e.g., counting UTF-16 code units for utf-16 or bytes for utf-8) [1][2][4]. Failing to honor the client's supported encodings or misinterpreting the negotiated encoding will lead to incorrect text manipulation or initialization failures [5][4].
Citations:
- 1: https://github.com/microsoft/vscode-languageserver-node/blob/master/protocol/src/common/protocol.ts
- 2: https://github.com/microsoft/vscode-languageserver-node/blob/main/types/src/main.ts
- 3: https://docs.rs/slidx_lsp/latest/slidx_lsp/position/enum.PositionEncoding.html
- 4: Server returns unsupported position encoding (utf-8), breaking initialization in VS Code / Cursor owenrumney/make-ls#8
- 5: Incorrect handling of multibyte UTF-16 encodings emacs-lsp/lsp-mode#2080
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'src/lspishow/lspishow.ml' 'src/lspishow/*' | sed -n '1,80p'
printf '%s\n' '--- relevant definitions and usages ---'
rg -n -C 5 'positionEncoding|positionEncodings|UTF8|UTF16|general' src/lspishow/lspishow.ml src/lspishow 2>/dev/null | sed -n '1,240p'
printf '%s\n' '--- file outline ---'
ast-grep outline src/lspishow/lspishow.ml --view compact 2>/dev/null | sed -n '1,160p'Repository: panglesd/slipshow
Length of output: 15934
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- lspishow.ml initialization and negotiation ---'
cat -n src/lspishow/lspishow.ml | sed -n '1,145p'
printf '%s\n' '--- all set_positionEncoding definitions/usages ---'
rg -n -C 8 'set_positionEncoding|positionEncoding[[:space:]]*<-' . --glob '!_build/**' --glob '!node_modules/**' | sed -n '1,240p'
printf '%s\n' '--- dependency declarations and protocol references ---'
rg -n -C 5 'linol|PositionEncodingKind|positionEncoding' dune-project '*.opam' src --glob '!src/lspishow/lspishow.ml' | sed -n '1,240p'Repository: panglesd/slipshow
Length of output: 22640
🌐 Web query:
site:microsoft.github.io/language-server-protocol positionEncodings "server can only use one of the encodings"
💡 Result:
The error message regarding position encodings typically occurs during the Language Server Protocol (LSP) initialization process when there is a mismatch between the capabilities supported by the client and the server. In LSP, position encoding (such as utf-8, utf-16, or utf-32) is negotiated between the client and the server during the initialize request [1][2]. The client sends a list of supported position encodings in its capabilities, and the server must select exactly one from that list to use for the session [2]. If a server is configured or implemented in a way that attempts to use an encoding not supported by the client, or if it fails to adhere to the single-encoding requirement mandated by the protocol, it may trigger a configuration or initialization error. To resolve this: 1. Check Client Capabilities: Ensure the language server is inspecting the clientCapabilities.general.positionEncodings field during the initialize request to see which encodings the client supports. 2. Select One Encoding: The server must choose and return exactly one encoding from the client's provided list in its serverCapabilities.positionEncoding field. 3. Validate Configuration: Ensure that your server implementation does not attempt to force an unsupported encoding or multiple encodings simultaneously, as the protocol requires a single, mutually agreed-upon encoding per session [2]. If you are encountering this error, it is often due to an outdated language server library that does not fully support the position encoding negotiation introduced in recent versions of the LSP specification [2][3]. Updating the language server SDK or library to a version that supports LSP 3.17 or later often resolves these issues.
Citations:
- 1: https://microsoft.github.io/language-server-protocol/specifications/base/0.9/specification/
- 2: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/
- 3: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.18/specification/
🌐 Web query:
site:github.com/c-cube/linol set_positionEncoding positionEncodings
💡 Result:
The terms set_positionEncoding and positionEncodings relate to the implementation of the Language Server Protocol (LSP) within the Linol library for OCaml [1]. Linol is a wrapper around the lsp library, designed to facilitate the creation of LSP servers by providing an object abstraction [1]. Based on the available documentation and repository structure, these identifiers are associated with handling how an LSP server interprets text positions, which is a requirement defined in the LSP specification [2][1]. Specifically: 1. PositionEncodingKind: This is an internal type or module within the Linol library (often appearing in documentation paths such as Linol_lwt/PositionEncodingKind) used to manage the encoding formats (like utf-8, utf-16, or utf-32) that the LSP server supports [2]. 2. set_positionEncoding: While exact implementation details vary by version, this functions as a configuration or setter method within the Linol framework to specify which position encoding the server should use when communicating with the LSP client [2]. The Linol library uses these mechanisms to maintain compatibility with the LSP wire protocol, ensuring that character offsets and position calculations are correctly synchronized between the server and the client [1]. If you are working with Linol, these are typically handled within the context of initializing the server or responding to client capabilities [1].
Citations:
Select an encoding supported by the client. When positionEncodings is present but contains neither UTF8 nor UTF16, the current code returns unsupported UTF16. Select the first supported encoding, or fail initialization when no supported encoding exists. Use UTF16 only when the capability is absent.
| let uri = Linol_lsp.Uri0.of_string (Cmarkit.Textloc.file textloc) in | ||
| let fpath = Fpath.v (Cmarkit.Textloc.file textloc) in | ||
| let* source = | ||
| Fpath.Map.find_opt fpath root.units.units | ||
| |> to_error "Source not found" | ||
| in |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare Uri0.of_string and DocumentUri.of_path usage for document URIs.
set -euo pipefail
rg -n -C 2 'Uri0\.of_string|DocumentUri\.of_path|DocumentUri\.to_path' src
fd -t f 'uri0.ml*' --exec rg -n -C 4 'let of_string|let of_path' {}Repository: panglesd/slipshow
Length of output: 5557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '190,265p' src/lspishow/lspishow.ml
printf '\n-- URI/document map definitions and uses --\n'
rg -n -C 3 'docs|DocumentUri|workspace/applyEdit|ApplyEdit|apply_edit' src
printf '\n-- dependency declarations --\n'
rg -n -C 2 'linol|linol_lwt' dune-project dune-workspace '*.opam' 2>/dev/null || trueRepository: panglesd/slipshow
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '-- target code --'
sed -n '200,260p' src/lspishow/lspishow.ml
printf '%s\n' '-- relevant symbols in target source --'
rg -n -C 4 'docs|DocumentUri|ApplyEdit|applyEdit|workspace' src/lspishow/lspishow.ml
printf '%s\n' '-- tracked dependency files --'
git ls-files | rg '(^|/)(dune-project|dune-workspace|[^/]+\.opam)$' | while read -r f; do
printf '\n[%s]\n' "$f"
rg -n -C 2 'linol' "$f" || true
doneRepository: panglesd/slipshow
Length of output: 13093
🌐 Web query:
Linol OCaml linol-lwt DocumentUri.of_path Uri0.of_string source
💡 Result:
In the Linol library for OCaml, DocumentUri and Uri0 are related modules used to handle URIs for the Language Server Protocol (LSP). Linol_lwt.DocumentUri is an alias for Linol_lsp.Uri0.t [1]. The Uri0 module itself is a lightweight URI implementation inspired by the vscode-uri TypeScript library, focusing primarily on the scheme, authority, and path components [2][3][4]. For these types, the following functions are available to handle URI conversions: 1. DocumentUri.of_path: A function that takes a string representing a file system path and converts it into a DocumentUri.t [1]. 2. Uri0.of_string: A function that parses a string into a Uri0.t (the underlying type for DocumentUri) [2][3][4]. These functions are commonly used when interacting with LSP server state, where document locations must be represented as URIs rather than simple file paths [5]. The underlying implementation in Uri0 handles path encoding, including special handling for Windows-style drive letters, to ensure compliance with URI specifications [2][4][6].
Citations:
- 1: https://dill.caelum.ci.dev/profiles/full/docs/p/linol-lwt/0.11/doc/linol-lwt/Linol_lwt/DocumentUri/index.html
- 2: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.10/doc/src/linol.lsp/uri0.ml.html
- 3: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.10/doc/linol.lsp/Linol_lsp/Uri0/index.html
- 4: https://ocaml.org/p/lsp/1.20.1/doc/src/lsp/uri0.ml.html
- 5: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/linol/Linol/Server/Make/index.html
- 6: https://dill.caelum.ci.dev/profiles/full/docs/p/lsp/1.20.1/doc/src/lsp/uri0.ml.html
🌐 Web query:
site:dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11 Uri0 of_path of_string
💡 Result:
Uri0 is a module found within the Linol library (specifically Linol_lsp), which is a library for writing Language Server Protocol (LSP) servers in OCaml [1]. It provides a type t representing a URI and utilities for parsing and manipulating these URIs [2][1]. The functions mentioned are defined as follows: - Uri0.of_path: This function takes a string representing a file system path and converts it into a Uri0.t [1]. The implementation handles platform-specific details, such as converting backslashes to slashes if the environment is set to Win32, before delegating the conversion to the underlying Uri_lexer module [2]. - Uri0.of_string: This function takes a string representation of a URI and parses it into a Uri0.t [1]. It utilizes the Uri_lexer.of_string function to perform the parsing [2]. In essence, these functions serve as constructors to create a structured URI type from either a local file path or a full URI string [1]. The module also includes functions for converting back to strings (to_string) and paths (to_path), as well as JSON serialization support [1].
Citations:
- 1: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/linol.lsp/Linol_lsp/Uri0/index.html
- 2: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/src/linol.lsp/uri0.ml.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '-- docs declaration and population --'
rg -n -C 5 'let docs|docs :|Hashtbl\.(create|add|replace|mem|find_opt)|on_notif_doc_did_open' src/lspishow/lspishow.ml
printf '%s\n' '-- Linol Uri0 implementation --'
curl -LfsS 'https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/src/linol.lsp/uri0.ml.html' |
sed -n '/of_string/,/of_path/p' | sed -n '1,180p'
printf '%s\n' '-- Linol DocumentUri API --'
curl -LfsS 'https://dill.caelum.ci.dev/profiles/full/docs/p/linol-lwt/0.11/doc/linol-lwt/Linol_lwt/DocumentUri/index.html' |
rg -o '.{0,120}(of_path|of_string|to_path).{0,220}' -m 12 || trueRepository: panglesd/slipshow
Length of output: 40805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '-- docs references in tracked OCaml source --'
git ls-files 'src/**/*.ml' 'src/**/*.mli' | xargs -r rg -n -C 3 '\bdocs\b|Hashtbl\.create' | head -n 300
printf '%s\n' '-- exact Uri0 constructors from generated source --'
python3 - <<'PY'
from html.parser import HTMLParser
from urllib.request import urlopen
url = "https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/src/linol.lsp/uri0.ml.html"
html = urlopen(url).read().decode()
text = HTMLParser().unescape(html) if hasattr(HTMLParser, "unescape") else html
for marker in ('id="val-of_string"', 'id="val-of_path"', 'id="val-to_path"'):
pos = html.find(marker)
print(f"\n--- {marker} at {pos} ---")
if pos >= 0:
print(HTMLParser().feed(html[max(0, pos-400):pos+2500]) if False else html[max(0, pos-400):pos+2500])
PYRepository: panglesd/slipshow
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '-- docs references in the LSP source only --'
rg -n -C 5 '\bdocs\b|Hashtbl\.create|DidOpenTextDocument' src/lspishow --glob '*.ml' --glob '*.mli' || true
printf '%s\n' '-- constructor definitions from Linol source HTML --'
python3 - <<'PY'
from html.parser import HTMLParser
from urllib.request import urlopen
class Text(HTMLParser):
def __init__(self):
super().__init__()
self.out = []
def handle_data(self, data):
self.out.append(data)
def value(self):
return ''.join(self.out)
url = "https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/src/linol.lsp/uri0.ml.html"
p = Text()
p.feed(urlopen(url).read().decode())
s = p.value()
for name in ("of_string", "of_path", "to_path"):
pos = s.find("let " + name)
print(f"\n--- let {name}: {pos} ---")
print(s[pos:pos+1200] if pos >= 0 else "not found")
PYRepository: panglesd/slipshow
Length of output: 6735
🌐 Web query:
github Linol_lsp Uri0 uri0.ml Linol_lwt DocumentUri of_path
💡 Result:
In the Linol library (a wrapper for the OCaml LSP library), DocumentUri is an alias for the Uri0.t type, which handles URI structures for LSP [1][2][3]. The function of_path is a method available within the Uri0 module (and exposed via DocumentUri) that converts a string path into a Uri0.t (URI) object [4][1][3]. Key technical details: - Module Location: The logic resides in Uri0, which is implemented in uri0.ml within the linol.lsp package [5][6]. - Functionality: The of_path function processes a string path, including handling Windows-style backslashes by converting them to forward slashes if the environment detects a Win32 system [5][6]. It then delegates the parsing to Uri_lexer.of_path [5][6]. - Scope: The implementation is inspired by the vscode-uri library [5][6]. It specifically supports scheme, authority, and path, but does not implement query, port, or fragment parsing [5][6]. - Usage: Whether you are working with Linol_lsp, Linol_lwt, or other Linol-based packages, DocumentUri.of_path serves as the standard way to create URI objects from file system paths for LSP-compliant servers [1][2][3].
Citations:
- 1: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/linol.lsp/Linol_lsp/Types/DocumentUri/index.html
- 2: https://ocaml.org/p/linol-eio/0.11/doc/linol-eio/Linol_eio/DocumentUri/index.html
- 3: https://dill.caelum.ci.dev/profiles/full/docs/p/linol-lwt/0.11/doc/linol-lwt/Linol_lwt/DocumentUri/index.html
- 4: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/linol.lsp/Linol_lsp/Uri0/index.html
- 5: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/src/linol.lsp/uri0.ml.html
- 6: https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.10/doc/src/linol.lsp/uri0.ml.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
import subprocess
from html import unescape
from html.parser import HTMLParser
url = "https://dill.caelum.ci.dev/profiles/full/docs/p/linol/0.11/doc/src/linol.lsp/uri0.ml.html"
html = subprocess.check_output(["curl", "-ks", url], text=True)
class Text(HTMLParser):
def __init__(self):
super().__init__()
self.parts = []
def handle_data(self, data):
self.parts.append(data)
def value(self):
return unescape("".join(self.parts))
p = Text()
p.feed(html)
source = p.value()
def show(pattern, label, width=900):
m = re.search(pattern, source, re.S)
print(f"\n-- {label} --")
print(re.sub(r"\s+", " ", m.group(0))[:width] if m else "not found")
show(r"let of_string\s*=\s*Uri_lexer\.of_string", "of_string delegates to URI parsing")
show(r"let of_path.*?(?=\blet |\Z)", "of_path implementation", 1600)
show(r"let to_path.*?(?=\blet |\Z)", "to_path implementation", 1200)
show(r"let to_string.*?(?=\blet |\Z)", "to_string implementation", 1200)
PYRepository: panglesd/slipshow
Length of output: 460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '270,305p' src/lspishow/lspishow.mlRepository: panglesd/slipshow
Length of output: 1514
Build document URIs with Linol_lwt.DocumentUri.of_path. Cmarkit.Textloc.file textloc is a filesystem path, but Uri0.of_string produces a URI without the file scheme. This prevents the docs lookup from matching the client URI and sends an invalid URI in workspace/applyEdit and ShowDocumentRequest. Apply the same conversion at GotoLoc (line 290).
Avoids highligting parts of an elem when gui has edited the file. Also avoid unmarshalling, better error messages, and the infrastructure was already there.
1 participant
"What you get is what you see" is sometimes nice. "What you get is what you mean" is sometimes nice.
What about "What you see is what you want"?
Introduces a
guiattribute, which allows to place, scale and dimension elements with the mouse. The state is saved in the buffer. Using it feels quite nice!Docs need to be worked on more.