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 PR adds an 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🧹 Nitpick comments (1)
test/compiler/auto-next.t/run.t (1)
9-9: ⚡ Quick winRemove
/tmpartifact copy from committed test.
cp auto-next.html /tmp/is a debug side effect and not part of test validation. It adds noise and can be brittle across environments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/compiler/auto-next.t/run.t` at line 9, Remove the debug artifact copy by deleting the shell command "cp auto-next.html /tmp/" from the test script (test/compiler/auto-next.t/run.t) so the test no longer performs an unnecessary filesystem side-effect; ensure the test still runs and asserts as intended without that temporary copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/actions-api.rst`:
- Around line 63-65: Update the user-facing sentence "The presenter need to
press the 'next' key to trigger the next action." to correct grammar by changing
"need" to "needs" so it reads: "The presenter needs to press the 'next' key to
trigger the next action." Ensure the revised sentence replaces the existing
sentence in the Actions description block that currently reads "Actions are
executed in 'reading order', from top left to bottom right. The presenter need
to press the 'next' key to trigger the next action."
- Around line 85-89: The example intended to show automatic chaining is missing
the auto-next attribute, so update the reveal/step snippet that uses
{reveal="two"} and the corresponding {`#two` .unrevealed center} block to include
the auto-next attribute (e.g., add auto-next="true" or the project's canonical
attribute name) on the step that should auto-advance; ensure the attribute is
added to the first step element in the pair so the second block is executed
automatically as documented and adjust any explanatory text to reflect the
presence of the auto-next attribute.
In `@test/compiler/auto-next.t/run.t`:
- Around line 3-9: The test currently runs "slipshow compile auto-next.md" and
copies auto-next.html but has no assertions; add checks after "cp auto-next.html
/tmp/" to validate the compiled output: open /tmp/auto-next.html and assert that
step "a" appears first, that "b" appears after "a", and that "c" and "d" appear
together (in that order), and assert the generated TOC contains the expected
entries; implement these as shell test assertions (e.g. grep -q for each
expected fragment and a simple order check comparing byte offsets or using
awk/sed) so the script fails if the steps or TOC are missing or out of order.
---
Nitpick comments:
In `@test/compiler/auto-next.t/run.t`:
- Line 9: Remove the debug artifact copy by deleting the shell command "cp
auto-next.html /tmp/" from the test script (test/compiler/auto-next.t/run.t) so
the test no longer performs an unnecessary filesystem side-effect; ensure the
test still runs and asserts as intended without that temporary copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff060fb2-db40-45d9-9425-4297f985c939
📒 Files selected for processing (3)
docs/actions-api.rsttest/compiler/auto-next.t/auto-next.mdtest/compiler/auto-next.t/run.t
✅ Files skipped from review due to trivial changes (1)
- test/compiler/auto-next.t/auto-next.md
| $ slipshow compile auto-next.md | ||
|
|
||
| There should be 3 steps: a, then b appears, then c and d appears. | ||
|
|
||
| Also, check the toc! | ||
|
|
||
| $ cp auto-next.html /tmp/ No newline at end of file |
There was a problem hiding this comment.
Missing assertions: test does not validate auto-next or TOC behavior.
This script compiles the fixture but never checks the generated output for the expected 3-step progression or TOC entries, so the PR objective is not actually tested.
Proposed test assertions to make this meaningful
Testing auto-next
$ slipshow compile auto-next.md
There should be 3 steps: a, then b appears, then c and d appears.
Also, check the toc!
-$ cp auto-next.html /tmp/
+ $ rg -n "a|b appears|c and d appears" auto-next.html
+ [expected matches proving sequencing]
+ $ rg -n "table-of-content|toc|Step 1|Step 2|Step 3" auto-next.html
+ [expected matches proving TOC entries exist]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/compiler/auto-next.t/run.t` around lines 3 - 9, The test currently runs
"slipshow compile auto-next.md" and copies auto-next.html but has no assertions;
add checks after "cp auto-next.html /tmp/" to validate the compiled output: open
/tmp/auto-next.html and assert that step "a" appears first, that "b" appears
after "a", and that "c" and "d" appear together (in that order), and assert the
generated TOC contains the expected entries; implement these as shell test
assertions (e.g. grep -q for each expected fragment and a simple order check
comparing byte offsets or using awk/sed) so the script fails if the steps or TOC
are missing or out of order.
There was a problem hiding this comment.
Yes, I still have no good way to test those things, but at least it simplifies testing by hand
There was a problem hiding this comment.
@panglesd, understood — having it as a quick manual-testing helper is still useful. Would you like me to open a follow-up issue to track adding proper automated assertions once a good mechanism is available?
🐇 (ˆ ω ˆ)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/runtime/table_of_content/table_of_content.ml`:
- Around line 82-83: The recursion in function loop is advancing with the
original categorized_els, causing an infinite re-processing of the same action;
change the recursive call to pass the updated list (res) instead of
categorized_els so the next step processes the remaining entries. Locate the
call loop ~auto_continue undo entries step categorized_els and replace the last
argument with res, ensuring variable res (the result of advancing the sequence)
is threaded into subsequent loop invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2492d7bc-e47f-4ff7-8ed0-9e7d347ccbad
📒 Files selected for processing (6)
CHANGELOG.mddocs/actions-api.rstsrc/compiler/special_attrs.mlsrc/engine/runtime/step/action_scheduler.mlsrc/engine/runtime/table_of_content/table_of_content.mltest/compiler/auto-next.t/auto-next.md
✅ Files skipped from review due to trivial changes (2)
- docs/actions-api.rst
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test/compiler/auto-next.t/auto-next.md
- src/compiler/special_attrs.ml
| let auto_continue = Brr.El.at !!"auto-continue" a |> Option.is_some in | ||
| loop ~auto_continue undo entries step categorized_els |
There was a problem hiding this comment.
Advance the recursion with res, not categorized_els.
Line 83 re-enters loop with the original list, so the same action is processed forever and TOC generation never progresses past the first action node.
Suggested fix
- loop ~auto_continue undo entries step categorized_els
+ loop ~auto_continue undo entries step res📝 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 auto_continue = Brr.El.at !!"auto-continue" a |> Option.is_some in | |
| loop ~auto_continue undo entries step categorized_els | |
| let auto_continue = Brr.El.at !!"auto-continue" a |> Option.is_some in | |
| loop ~auto_continue undo entries step res |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engine/runtime/table_of_content/table_of_content.ml` around lines 82 -
83, The recursion in function loop is advancing with the original
categorized_els, causing an infinite re-processing of the same action; change
the recursive call to pass the updated list (res) instead of categorized_els so
the next step processes the remaining entries. Locate the call loop
~auto_continue undo entries step categorized_els and replace the last argument
with res, ensuring variable res (the result of advancing the sequence) is
threaded into subsequent loop invocations.
There was a problem hiding this comment.
No, Step.Action_scheduler.AttributeActions.do_ has some side-effect and Step.Action_scheduler.is_action a will inevitably become false.
There was a problem hiding this comment.
@panglesd, you’re right — I missed that Step.Action_scheduler.AttributeActions.do_ mutates the action state before the recursive call. With that side effect, re-entering with categorized_els re-checks the same element once, Step.Action_scheduler.is_action a becomes false, and the recursion then advances via the else ... res branch. So this is not an infinite loop, and my suggested change would be inappropriate here.
I’ll keep this behavior in mind for future reviews.
🐇💭
✏️ Learnings added
Learnt from: panglesd
Repo: panglesd/slipshow PR: 240
File: src/engine/runtime/table_of_content/table_of_content.ml:82-83
Timestamp: 2026-05-29T22:24:41.640Z
Learning: In `src/engine/runtime/table_of_content/table_of_content.ml` in the `generate` function's recursive `loop`, `Step.Action_scheduler.AttributeActions.do_ ~mode window a` has side effects that update the action state of element `a`. After it runs, `Step.Action_scheduler.is_action a` will become false, so recursively calling `loop` with the original `categorized_els` list is intentional: the same `Action a` is re-checked and then consumed through the `else ... res` branch. Do not flag this pattern as an infinite loop without accounting for the side effect.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: panglesd
Repo: panglesd/slipshow PR: 228
File: vendor/github.com/panglesd/cmarkit/src/cmarkit.ml:3824-3838
Timestamp: 2026-05-04T21:14:22.518Z
Learning: In OCaml, record field punning is valid syntax in both record construction and record update expressions. In record construction, `{ field }` expands to `{ field = field }` when the variable name matches the field name. In record update expressions, `{ r with field }` is shorthand for `{ r with field = field }` as long as a variable named `field` is in scope. This also works for qualified field names (e.g., `{ r with Module.Type.field = value }` as appropriate). When reviewing OCaml code, do not flag these forms as syntax errors.
Learnt from: panglesd
Repo: panglesd/slipshow PR: 228
File: vendor/github.com/panglesd/cmarkit/src/cmarkit.ml:3793-3810
Timestamp: 2026-05-04T21:14:50.423Z
Learning: In OCaml, record update field punning is legal in the `{ expr with field }` / `{ expr with Qualified.field }` forms. If a variable named `field` (or `rows` in the qualified example) is in scope, then `{ r with field }` is shorthand for `{ r with field = field }`. Likewise, `{ t with Block.Table.rows }` is shorthand for `{ t with Block.Table.rows = rows }` when `rows` is in scope. When reviewing, don’t flag `{ r with field }` as invalid syntax; only consider it an error if the corresponding pun variable (e.g., `field` / `rows`) is not in scope and thus cannot bind the right-hand side.
Learnt from: panglesd
Repo: panglesd/slipshow PR: 228
File: vendor/github.com/panglesd/cmarkit/src/cmarkit.ml:3793-3810
Timestamp: 2026-05-04T21:14:50.423Z
Learning: In OCaml, field punning also applies to record update (`with`) expressions. If a variable `x` is in scope, `{ r with x }` is valid shorthand for `{ r with x = x }` (not missing syntax). This also works for qualified field names: `{ t with Block.Table.rows }` is shorthand for `{ t with Block.Table.rows = Block.Table.rows }`. When reviewing OCaml code, do not flag these forms as invalid syntax.
1 participant
For instance:
{slip auto-next} {pause}will "merge" the two actions in a single step.