Conversation
📝 WalkthroughWalkthroughThe PR adds dependency caching and retry logic to CI workflows, introduces a generic retry script for transient failures, and refactors OTP input handling to support multi-character pasted inputs while removing the maxlength constraint. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
72-79:⚠️ Potential issue | 🟠 MajorRemove the duplicate
gleam deps downloadbeforescripts/check.sh.Line 73 already does the same retry-wrapped download that
scripts/check.shperforms at Line 9, so the hex-semver path hits dependency resolution twice. That adds avoidable CI latency and can still fail on the second call after the first one succeeded.Suggested simplification
- - if: steps.hex_deps.outputs.deps_ready == 'true' - run: bash scripts/ci/retry.sh 12 10 gleam deps download - working-directory: weft_lustre_ui - if: steps.hex_deps.outputs.deps_ready == 'true' run: | echo "Dependency mode: hex semver (full checks)" bash scripts/check.sh working-directory: weft_lustre_ui🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 72 - 79, Duplicate dependency download is invoked: the retry-wrapped "bash scripts/ci/retry.sh 12 10 gleam deps download" call appears before the hex-semver path which then calls scripts/check.sh that itself runs the same download; remove the redundant pre-call so dependency resolution runs only inside scripts/check.sh. Edit the workflow block guarded by steps.hex_deps.outputs.deps_ready == 'true' and delete the standalone retry.sh invocation (the step that runs "bash scripts/ci/retry.sh 12 10 gleam deps download") so the subsequent step that echoes "Dependency mode: hex semver (full checks)" and runs "bash scripts/check.sh" is the sole dependency-resolution path. Ensure the if condition (steps.hex_deps.outputs.deps_ready == 'true') remains on the remaining step so behavior and gating are unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/weft_lustre_ui/headless/input_otp.gleam`:
- Around line 145-147: The code calls the non-existent list.at(values, offset);
replace that with a supported pattern using list.drop and pattern matching: drop
the first offset elements from values (list.drop(values, offset)) and then
case-match on the result ([] -> char; [value | _] -> value). Update the branch
that references list.at to use list.drop(values, offset) and the pattern match
so values, offset and char are returned correctly when present or fall back to
char when empty.
---
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 72-79: Duplicate dependency download is invoked: the retry-wrapped
"bash scripts/ci/retry.sh 12 10 gleam deps download" call appears before the
hex-semver path which then calls scripts/check.sh that itself runs the same
download; remove the redundant pre-call so dependency resolution runs only
inside scripts/check.sh. Edit the workflow block guarded by
steps.hex_deps.outputs.deps_ready == 'true' and delete the standalone retry.sh
invocation (the step that runs "bash scripts/ci/retry.sh 12 10 gleam deps
download") so the subsequent step that echoes "Dependency mode: hex semver (full
checks)" and runs "bash scripts/check.sh" is the sole dependency-resolution
path. Ensure the if condition (steps.hex_deps.outputs.deps_ready == 'true')
remains on the remaining step so behavior and gating are unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9aaefae5-6cbf-4ce7-9e6c-5a099c00f945
📒 Files selected for processing (5)
.github/workflows/test.ymlscripts/check.shscripts/ci/retry.shsrc/weft_lustre_ui/headless/input_otp.gleamtest/input_otp_test.gleam
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🧰 Additional context used
🪛 GitHub Actions: test
src/weft_lustre_ui/headless/input_otp.gleam
[error] 145-145: Gleam compile error: Unknown module value. The expression 'list.at(values, offset)' is invalid because the module 'gleam/list' does not have an 'at' function. Did you mean 'last'?
🔇 Additional comments (4)
scripts/ci/retry.sh (1)
5-38: Nice: this stays generic and safely preserves argv boundaries.The argument validation is clear, and
command=("$@")avoids word-splitting bugs when retrying commands with spaces or flags.scripts/check.sh (1)
9-9: Good place for the retry wrapper.Keeping the dependency bootstrap inside
scripts/check.shmakes local and CI full-check runs follow the same path..github/workflows/test.yml (1)
23-31: The cache key is scoped to both Gleam manifests.That is the right granularity for dependency cache reuse while still invalidating on dependency graph changes.
test/input_otp_test.gleam (1)
33-43: Good regression coverage for paste fan-out and removedmaxlength.These assertions exercise both user-visible OTP changes introduced by this PR.
Also applies to: 69-70
| case list.at(values, offset) { | ||
| Ok(value) -> value | ||
| Error(Nil) -> char |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the official Gleam standard library documentation for gleam/listprovide a function namedat in Gleam 1.14.x? If not, what supported pattern should be used to read the nth element of a list?
💡 Result:
No. In the official gleam_stdlib docs for gleam/list (the standard library used with modern Gleam releases, including 1.14.x), there is no list.at function; instead the module provides list.drop and list.first. [1][2]
Supported pattern to read the nth element (0-based):
import gleam/list
pub fn nth(list: List(a), n: Int) -> Result(a, Nil) {
list
|> list.drop(up_to: n)
|> list.first
}drop(up_to: n)removes the firstnelements. [2]firstreturns the first element (orError(Nil)if empty). [1]
🏁 Script executed:
wc -l src/weft_lustre_ui/headless/input_otp.gleamRepository: bbopen/weft_lustre_ui
Length of output: 112
🏁 Script executed:
sed -n '140,150p' src/weft_lustre_ui/headless/input_otp.gleamRepository: bbopen/weft_lustre_ui
Length of output: 310
list.at is not a valid gleam/list API.
Line 145 uses list.at(values, offset), which does not exist in the standard library and causes a compile failure. The OTP paste feature cannot build with this code. Switch to a supported list access pattern.
Suggested fix
- case list.at(values, offset) {
- Ok(value) -> value
- Error(Nil) -> char
- }
+ case values |> list.drop(up_to: offset) |> list.first {
+ Ok(value) -> value
+ Error(Nil) -> char
+ }📝 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.
| case list.at(values, offset) { | |
| Ok(value) -> value | |
| Error(Nil) -> char | |
| case values |> list.drop(up_to: offset) |> list.first { | |
| Ok(value) -> value | |
| Error(Nil) -> char | |
| } |
🧰 Tools
🪛 GitHub Actions: test
[error] 145-145: Gleam compile error: Unknown module value. The expression 'list.at(values, offset)' is invalid because the module 'gleam/list' does not have an 'at' function. Did you mean 'last'?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weft_lustre_ui/headless/input_otp.gleam` around lines 145 - 147, The code
calls the non-existent list.at(values, offset); replace that with a supported
pattern using list.drop and pattern matching: drop the first offset elements
from values (list.drop(values, offset)) and then case-match on the result ([] ->
char; [value | _] -> value). Update the branch that references list.at to use
list.drop(values, offset) and the pattern match so values, offset and char are
returned correctly when present or fall back to char when empty.
1 participant
Motivation
gleam deps downloadinvocations and caching Gleam artifacts to avoid repeated downloads.Description
scripts/ci/retry.shto provide a generic retry wrapper and use it to rungleam deps downloadin both the GitHub Actions workflow andscripts/check.shviabash scripts/ci/retry.sh 12 10 gleam deps download.pathentries~/.cache/gleamandweft_lustre_ui/build/packagesand a content-hashkeyusingweft_lustre_ui/gleam.tomlandweft_lustre_ui/manifest.toml.replace_slots_from_indexandinput_otp_apply_slot_inputinsrc/weft_lustre_ui/headless/input_otp.gleamto fan out pasted/multi-character input across subsequent slots and remove the old single-grapheme helper.Codex Task