more utils #10
more utils #10
Conversation
WalkthroughRefactors the waitfor countdown to real-time, adds many new Fish utilities (GitHub, Docker, notes, formatting, OTP, docs/typehint tools), migrates measurements from JSON to SQLite with a Peewee model, reorganizes color assets, updates CI and install scripts, and adjusts numerous callers and scripts to the new waitfor signature and behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant waitfor as Waitfor
participant timeprompt as TimePrompt
participant Terminal
User->>Waitfor: call waitfor(time, label, desc, keydown, until, step)
alt allow not provided (default flow)
loop until current >= target or interruption
Waitfor->>TimePrompt: timeprompt(remaining)
TimePrompt->>Terminal: wait for key or timeout
alt key pressed
TimePrompt-->>Waitfor: exit status 1
Waitfor->>Waitfor: set es=1, break
else timeout
TimePrompt-->>Waitfor: exit status 0
Waitfor->>Waitfor: update cur, render progress bar
end
end
else allow provided
loop until current >= target
Waitfor->>Waitfor: sleep short interval, update cur
Waitfor->>Waitfor: render progress bar
end
end
Waitfor-->>User: return es
sequenceDiagram
participant Caller
participant add_measure as add_measure.py
participant SQLite as DB
Caller->>add_measure: add_measure(key, value)
add_measure->>add_measure: acquire FileLock
add_measure->>DB: connect / ensure tables exist
add_measure->>DB: INSERT Measurement(key, value, created_date)
add_measure->>add_measure: release FileLock
add_measure-->>Caller: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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
🧹 Nitpick comments (3)
waitfor.fish (3)
30-30: Clamp color channel to [0, 255]If
$rmdever underflows (or is fractional in future revisions), the green/blue channel can go negative. Clamp to the valid range.-set cl (math "min(255, 4*$rmd)") +set cl (math "max(0, min(255, 4*$rmd))")
32-32: Clamp progress percentage to [0, 100]During edge conditions (e.g., time boundary), percentage can overshoot 100%. Clamp to keep UI consistent.
-set pct (math "round(100*($time-$rmd)/$time)") +set pct (math "min(100, max(0, round(100*($time-$rmd)/$time)))")
35-35: Prevent invalid backlight values by clamping to [20, 255]If
$rmdbecomes negative,newvalcan drop below 20, causing invalid writes to/sys/class/leds/.../brightness. Clamp to the expected bounds.-set newval (math "round(235*$rmd/$time)+20") +set newval (math "max(20, min(255, round(235*$rmd/$time)+20))")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
waitfor.fish(1 hunks)
| set tar (math "$cur + $time") | ||
| while test "$cur" -lt "$tar" | ||
| set cur (date '+%s') | ||
| set rmd (math "$tar - $cur") | ||
| set txt (timeformat "$rmd") |
There was a problem hiding this comment.
Fix TOCTOU bug in loop condition and restore until semantics; clamp remaining time to avoid negatives
The loop tests an outdated $cur captured before sleep, which can allow one extra iteration and yield negative $rmd. This leads to invalid ANSI colors and potentially writing negative brightness to sysfs. Also, until is currently unused by the loop.
Refactor to:
- Evaluate the current time in the while condition.
- Honor
untilby computing a stop epoch. - Clamp
$rmdto be non-negative.
Apply this diff:
set tar (math "$cur + $time")
-while test "$cur" -lt "$tar"
- set cur (date '+%s')
- set rmd (math "$tar - $cur")
+set stop (math "$tar - $until")
+while test (date '+%s') -lt "$stop"
+ set cur (date '+%s')
+ set rmd (math "max(0, $stop - $cur)")
set txt (timeformat "$rmd")Optional: To avoid oversleeping near the end, consider sleeping no longer than the remaining time:
sleep (math "min($step, $rmd)")🤖 Prompt for AI Agents
In waitfor.fish around lines 20 to 24, the loop uses a stale $cur captured
before sleeping which creates a TOCTOU bug (allowing an extra iteration and
negative $rmd) and ignores the `until` semantics; change the loop to evaluate
the current time in the loop condition by computing a stop epoch up front when
`until` is set, loop while the current time is less than that stop epoch (or
target), recompute $cur inside the condition, compute $rmd as the positive
difference and clamp it to zero if negative, and optionally limit sleep to the
remaining time by using sleep with min($step, $rmd).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
whatday.fish (1)
4-5: Optional: derive foreground automatically from background luminance to prevent drift.Hardcoding
tcolcan get out-of-sync withcolorswhen backgrounds change. Consider computing 30/97 based on RGB luminance so contrast remains readable without manual tweaks.Example fish snippet (add near the top and replace the
set tcolline with the loop):function __fg_for_bg --argument-names rgb set -l c (string split ';' -- $rgb) set -l r $c[1]; set -l g $c[2]; set -l b $c[3] # Perceived luminance (sRGB) approx set -l lum (math "0.2126*$r + 0.7152*$g + 0.0722*$b") if test (math --scale 0 "$lum") -ge 186 echo 30 # black else echo 97 # bright white end end # Build tcol dynamically to match `colors` set -l tcol for i in (seq 1 7) set -a tcol (__fg_for_bg $colors[$i]) endAlso applies to: 17-17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
whatday.fish(1 hunks)
🔇 Additional comments (1)
whatday.fish (1)
5-5: Foreground color remap looks sane for contrast.Switching day 2 to black (30) and day 5 to bright white (97) aligns better with the orange/blue backgrounds respectively. Seven entries match the 7-day indexing (Line 13) and Fish’s 1-based arrays. No functional issues spotted.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/build.yml (2)
12-14: Harden job token scope with least-privilege permissions.This job only needs to read the repository. Reduce the GITHUB_TOKEN scope.
Apply this diff:
jsonlint: runs-on: ubuntu-latest + permissions: + contents: read steps:
18-19: REPORT_ONLY=true makes the JSON check non-blocking. Confirm intended gating.With REPORT_ONLY=true, invalid JSON won’t fail the job (just reports). If you want CI to block on invalid JSON, set it to false (and consider quoting values for clarity).
If keeping the remote script approach, apply this minimal change to make the step gate CI:
- REPORT_ONLY: true + REPORT_ONLY: false - SHOW_ERRORS: true + SHOW_ERRORS: "true"Please confirm whether the goal is to gate the pipeline on JSON validity or just report issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build.yml
15-15: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - name: validate JSON | ||
| env: | ||
| REPORT_ONLY: true | ||
| SHOW_ERRORS: true | ||
| run: bash <(curl -s https://raw.githubusercontent.com/CICDToolbox/json-lint/master/pipeline.sh) |
There was a problem hiding this comment.
Avoid executing unpinned remote scripts in CI; replace with a local validator or pin to a commit.
curl-ing and executing a remote script from a moving branch (master) is a supply-chain risk and makes builds non-deterministic. Prefer a local validator (no network), or at minimum fetch a specific commit and verify integrity.
Preferred fix: replace the remote script with a Python-based JSON validation that fails the job on invalid JSON.
- - name: validate JSON
- env:
- REPORT_ONLY: true
- SHOW_ERRORS: true
- run: bash <(curl -s https://raw.githubusercontent.com/CICDToolbox/json-lint/master/pipeline.sh)
+ - name: Validate JSON (fail on invalid)
+ run: |
+ set -euo pipefail
+ # Validate all *.json files (tracked and untracked). Fails on first invalid file.
+ found=false
+ while IFS= read -r -d '' f; do
+ found=true
+ python3 -m json.tool "$f" >/dev/null
+ done < <(find . -type f -name "*.json" -print0)
+ # If no JSON files exist, succeed without doing anything.
+ if [ "$found" = false ]; then
+ echo "No JSON files found."
+ fiAlternative (if you must keep the remote script):
- Pin to a specific commit instead of master, e.g. https://raw.githubusercontent.com/CICDToolbox/json-lint//pipeline.sh
- Optionally verify a known SHA256 of the script before executing.
📝 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.
| - name: validate JSON | |
| env: | |
| REPORT_ONLY: true | |
| SHOW_ERRORS: true | |
| run: bash <(curl -s https://raw.githubusercontent.com/CICDToolbox/json-lint/master/pipeline.sh) | |
| - name: Validate JSON (fail on invalid) | |
| run: | | |
| set -euo pipefail | |
| # Validate all *.json files (tracked and untracked). Fails on first invalid file. | |
| found=false | |
| while IFS= read -r -d '' f; do | |
| found=true | |
| python3 -m json.tool "$f" >/dev/null | |
| done < <(find . -type f -name "*.json" -print0) | |
| # If no JSON files exist, succeed without doing anything. | |
| if [ "$found" = false ]; then | |
| echo "No JSON files found." | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/build.yml around lines 16-20: the workflow currently curl|bash
executes an unpinned remote script (supply-chain risk and non-deterministic);
replace it with a local JSON validator that runs without network (or, if you
must keep remote, fetch a pinned commit and verify its SHA). Specifically:
remove the curl... pipeline.sh step and add a step that runs a local validation
command (e.g., iterate repo JSON files and run python -m json.tool on each,
failing the job on any parse error), ensure the validator is committed in the
repo or uses built-in python so no network is required; alternatively, change
the URL to the raw file at a specific commit and add a SHA256 verification
before executing.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
install.sh (1)
58-61: Fix missing sudo, make idempotent, and guard sysfs path.
- Line 59 currently runs
usermodwithout sudo → will fail for non-root users.groupaddwill error if the group exists.- The backlight sysfs path may not exist on all machines; chgrp/chmod will fail.
Apply this safer, idempotent version:
-sudo groupadd light -usermod -aG light "$USER" -sudo chgrp light /sys/class/leds/rgb:kbd_backlight/ -sudo chmod g+w -R /sys/class/leds/rgb:kbd_backlight/ +sudo groupadd -f light +sudo usermod -aG light "$USER" +if [ -d /sys/class/leds/rgb:kbd_backlight/ ]; then + sudo chgrp light /sys/class/leds/rgb:kbd_backlight/ + sudo chmod -R g+w /sys/class/leds/rgb:kbd_backlight/ +fi
🧹 Nitpick comments (6)
install.sh (6)
9-9: Ensure basepassis installed (extension alone may be insufficient).
pass-extension-otptypically relies onpass. Ifpassisn’t already present or a strict dependency in your target distro, the extension will be unusable. Addpassexplicitly to avoid surprises.Please verify on your target distro that
pass-extension-otppullspassautomatically. If not, apply this change:-sudo apt-get install -y alarm-clock-applet apt-file agrep certbot cloc curl cvc4 djinn fish git git-filter-repo git-lfs gnuplot-nox haskell-stack hlint hoogle ipython3 jq lambdabot ledger lynx nodeenv nodejs npm pandoc pass-extension-otp pdfsam powerline pv pylint python-wxtools python3-pytest rename sdate sox supervisor swi-prolog-core tig tox xdotool yarn zbar-tools z3 +sudo apt-get install -y alarm-clock-applet apt-file agrep certbot cloc curl cvc4 djinn fish git git-filter-repo git-lfs gnuplot-nox haskell-stack hlint hoogle ipython3 jq lambdabot ledger lynx nodeenv nodejs npm pandoc pass pass-extension-otp pdfsam powerline pv pylint python-wxtools python3-pytest rename sdate sox supervisor swi-prolog-core tig tox xdotool yarn zbar-tools z3
3-4: Replace deprecated apt-key usage with keyring + signed-by (APT best practice).
apt-keyis deprecated. Use a dedicated keyring and thesigned-byoption for the Yarn repo.Apply:
-curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add - -echo "deb https://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.list +sudo install -m 0755 -d /etc/apt/keyrings +curl -fsSL https://dl.yarnpkg.com/debian/pubkey.gpg | sudo gpg --dearmor -o /etc/apt/keyrings/yarn.gpg +echo "deb [signed-by=/etc/apt/keyrings/yarn.gpg] https://dl.yarnpkg.com/debian stable main" | sudo tee /etc/apt/sources.list.d/yarn.list > /dev/null
6-6: Makeadd-apt-repositorynon-interactive.In CI or unattended runs,
add-apt-repositorycan prompt. Add-y.-sudo add-apt-repository ppa:tatokis/alarm-clock-applet +sudo add-apt-repository -y ppa:tatokis/alarm-clock-applet
44-44: Make user creation idempotent.
useraddwill fail if the user already exists. Guard it.-sudo useradd prefect +id -u prefect >/dev/null 2>&1 || sudo useradd -m -s /bin/bash prefect
55-57: Avoid executing fish-specific commands in a bash script.
abbris a fish builtin and will fail under bash. Move these into fish config (you already append powerline config) or execute viafish -c.A simple approach is to append to
~/.config/fish/config.fish:-# TODO:!!! -abbr -a --regex '[.][.][.]([/].*)?' --position anywhere --function ... -- git_root -abbr -a --regex '.*[{]-?[0-9]+[:]-?[0-9]+([:]-?[0-9]+)?[}].*' --position anywhere --function expand -- expand +# Configure fish abbreviations +cat >> ~/.config/fish/config.fish <<'EOF_ABBR' +# Added by install.sh +abbr -a --regex '[.][.][.]([/].*)?' --position anywhere --function ... -- git_root +abbr -a --regex '.*[{]-?[0-9]+[:]-?[0-9]+([:]-?[0-9]+)?[}].*' --position anywhere --function expand -- expand +EOF_ABBRIf you want, I can prep a follow-up patch to deduplicate entries on re-runs.
1-2: Optional: fail fast and catch unset vars.Consider enabling strict mode. Be mindful to guard known non-fatal commands when using
set -e.#!/bin/bash +set -euo pipefail
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
install.sh(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
otp.fish (1)
1-3: Optional: guard for missing dependenciesIf pass or the OTP extension are not installed, this will fail with a less helpful error. Consider a short check with a clear message.
Apply this minimal guard:
function otp --wraps='pass otp' --description 'generate/configure One Time Passwords (OTPs) with pass-extension-otp' - pass otp $argv + if not command -q pass + echo "otp: 'pass' is not installed. Please install pass and pass-extension-otp." >&2 + return 127 + end + pass otp $argv endtimeprompt.fish (1)
1-3: Optional: avoid echoing the keypress and validate inputTo prevent the pressed key from echoing, pass -s to read. You may also want basic validation of the time argument.
For example:
- not bash -c "read -n 1 -t $time" + not bash -c "read -s -n 1 -t $time"If you want validation:
if not string match -qr '^[0-9]+(\.[0-9]+)?$' -- $time echo "timeprompt: invalid time '$time' (expected a positive number of seconds)" >&2 return 2 endREADME.md (2)
134-137: Fix list indentation for MD007 and keep wording conciseMarkdownlint flags this list item’s indentation. Also, minor wording polish.
Apply:
- - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp; +- **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp;
192-193: Fix list indentation (MD007) and grammarIndentation triggers MD007. The description benefits from a semicolon and “presses”.
Apply:
- - **`timeprompt`**: wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted; +- **`timeprompt`**: wait for a given amount of time, or until the person presses a key; exits with 1 if the user interrupted;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
README.md(2 hunks)otp.fish(1 hunks)timeprompt.fish(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
136-136: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 LanguageTool
README.md
[grammar] ~192-~192: There might be a mistake here.
Context: ...ey exits with 1 if the user interrupted; - timestamp: get a string that specifies date and t...
(QB_NEW_EN)
🔇 Additional comments (1)
otp.fish (1)
1-3: LGTM: simple and correct wrapperThe function is a thin pass-through and correctly forwards arguments to pass otp.
| function timeprompt --description 'wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted' -a time | ||
| ! bash -c "read -n 1 -t '$time'" | ||
| end |
There was a problem hiding this comment.
Bug: use Fish’s “not” instead of “!” and fix description grammar
Fish doesn’t support bash-style “!” for negation. Use not to invert the exit status. Also tighten the description grammar.
Apply:
-function timeprompt --description 'wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted' -a time
- ! bash -c "read -n 1 -t '$time'"
+function timeprompt --description 'wait for a given amount of time, or until the person presses a key; exits with 1 if the user interrupted' -a time
+ not bash -c "read -n 1 -t $time"
endNotes:
- Bash read returns 0 on keypress and nonzero on timeout; not inverts this to match your documented behavior.
- Dropped the superfluous single quotes around $time inside the bash -c string.
📝 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.
| function timeprompt --description 'wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted' -a time | |
| ! bash -c "read -n 1 -t '$time'" | |
| end | |
| function timeprompt --description 'wait for a given amount of time, or until the person presses a key; exits with 1 if the user interrupted' -a time | |
| not bash -c "read -n 1 -t $time" | |
| end |
🤖 Prompt for AI Agents
In timeprompt.fish around lines 1 to 3, the function uses bash-style "!" for
negation and has a grammatically loose description; replace the leading "!" with
Fish's "not" to invert the exit status and tighten the description string (e.g.,
"Wait for a given time or until a key is pressed; exit 1 if interrupted"). Also
remove the unnecessary single quotes around $time inside the bash -c command so
the timeout is passed correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
waitfor.fish (1)
14-15: UnuseduntilParameter in waitfor.fish
Theuntilvariable is initialized on line 14 but never used in the loop control, which will confuse callers relying on that flag. Please either remove it entirely or wire it into the stop-time calculation with one of these options:• Absolute timestamp cap
# after computing 'stop' from timeout… + set stop $until• Delta semantics (shorten remaining time by X seconds)
# after computing 'stop' from timeout… + set stop (math "$stop - $until")Please confirm which behavior you intend so dependent scripts aren’t broken.
- Location: waitfor.fish lines 14–15
♻️ Duplicate comments (1)
waitfor.fish (1)
19-47: Fix TOCTOU in loop, restoreuntilsemantics, and clamp rmd/pct/color; bound sleep to remaining time
- The loop condition reads a stale $cur captured before sleeping, allowing an extra iteration and negative $rmd.
- The
untilparameter is computed but never used.- Negative
$rmdcan generate invalid ANSI color values, >100% progress, and backlight values outside device bounds.- Sleeping a fixed
$stepcan overshoot the end by more than the remaining time.Refactor as below:
- Compute a stop epoch and use it in the loop condition.
- Clamp
$rmdto non-negative.- Clamp color and progress to safe ranges.
- Bound
sleep/timepromptto the remaining time, and end early by settingcurto$stopon timeout/decline.Apply this diff:
set time (math "round($time)") - set cur (date '+%s') - set tar (math "$cur + $time") - while test "$cur" -lt "$tar" - set cur (date '+%s') - set rmd (math "$tar - $cur") + set cur (date '+%s') + set tar (math "$cur + $time") + # Determine stop epoch: honor absolute 'until' if provided, else stop at tar + set stop "$tar" + if test "$until" -gt 0 + set stop (math "min($tar, $until)") + end + while test (date '+%s') -lt "$stop" + set cur (date '+%s') + # Clamp remaining time to non-negative to avoid underflow + set rmd (math "max(0, $stop - $cur)") set txt (timeformat "$rmd") @@ - set cl (math "min(255, 4*$rmd)") + # Clamp color channel to [0,255] + set cl (math "max(0, min(255, 4*$rmd))") @@ - set pct (math "round(100*($time-$rmd)/$time)") + # Clamp progress to [0,100] + set pct (math "max(0, min(100, round(100*($time-$rmd)/$time)))") @@ - set newval (math "round(235*$rmd/$time)+20") + # Clamp brightness to device range [0,255] + set newval (math "max(0, min(255, round(235*$rmd/$time)+20))") @@ - if [ -z "$allow" ] - if ! timeprompt "$step" - set cur "$tar" - end - else - sleep "$step" - end + if [ -z "$allow" ] + # Bound prompt timeout to remaining time to avoid overshoot + if ! timeprompt (math "min($step, $rmd)") + # End early if user declines or times out + set cur "$stop" + end + else + # Bound sleep to remaining time + sleep (math "min($step, $rmd)") + endAlso applies to: 30-36, 32-33
🧹 Nitpick comments (3)
smartwatch.fish (1)
6-6: Waitfor call updated correctly; consider guarding non-positive durationsThe placeholder argument matches the new waitfor API. As a minor hardening (unrelated to this change), if
$totcan be ≤ 0 (e.g.,cur >= upto), consider short-circuiting instead of calling waitfor.You could do:
- waitfor "$tot" '' '⌚ battery' '⌚ ' + if test "$tot" -gt 0 + waitfor "$tot" '' '⌚ battery' '⌚ ' + endwaitfor.fish (1)
35-39: Guard writes to sysfs backlight; avoid noisy errors on systems without the deviceOn systems without
/sys/class/leds/rgb:kbd_backlight/brightness, the background write will error. Optional guard:- if [ "$oldval" -ne "$newval" ] - echo "$newval" > /sys/class/leds/rgb:kbd_backlight/brightness & + if [ "$oldval" -ne "$newval" -a -w /sys/class/leds/rgb:kbd_backlight/brightness ] + echo "$newval" > /sys/class/leds/rgb:kbd_backlight/brightness &blooddonation.fish (1)
8-8: Quote getcolor substitution to avoid unintended word-splitting.If getcolor ever returns a value containing whitespace, fish will split it into multiple args. Quoting the substitution keeps it a single argument.
Apply this diff:
- waitfor 2700 '' '🩸 donation' '🩸 ' (getcolor blood body_colors ,) + waitfor 2700 '' '🩸 donation' '🩸 ' "(getcolor blood body_colors ,)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
blooddonation.fish(1 hunks)eggs.fish(1 hunks)homebattery.fish(1 hunks)household.fish(1 hunks)movie.fish(1 hunks)pomodoro.fish(2 hunks)smartwatch.fish(1 hunks)teeth.fish(2 hunks)vitamins.fish(1 hunks)waitfor.fish(2 hunks)walking.fish(1 hunks)
🔇 Additional comments (12)
teeth.fish (2)
26-26: Waitfor placeholder aligns with updated APIPassing an empty string for the timeprompt placeholder looks consistent with the PR-wide waitfor changes. The remaining label and prompt args are preserved and quoted correctly.
37-37: Consistent placeholder usage for second waitforSame here—the empty-string placeholder cleanly suppresses the dynamic prompt. No functional concerns.
household.fish (1)
3-3: Updated waitfor signature usage looks correctShifting to
waitfor $time '' "$emoji $name" "$emoji "matches the new calling convention. Arguments are properly quoted to preserve spaces and emoji.eggs.fish (1)
4-4: Aligned with new waitfor argument patternThe inserted empty-string placeholder keeps the label and prompt intact and matches other updated call sites in the PR.
movie.fish (1)
5-5: Correct migration to new waitfor parametersThe call now includes the empty-string placeholder in the expected position. The surrounding guard
test -n "$duration"remains appropriate.waitfor.fish (1)
1-1: Allwaitforinvocations include the newallowargumentI’ve reviewed every call site—each uses
''as the second parameter. No remaining scripts need updates.homebattery.fish (1)
10-10: Call-site update looks correctInserted empty
allowargument and shifted label/desc accordingly. Compatible with the new waitfor signature and timeprompt default behavior.vitamins.fish (1)
2-2: Aligned with new waitfor signatureThe added empty
allowargument is correct; label/desc positions remain consistent with how waitfor renders UI and status.pomodoro.fish (2)
32-32: Working session waitfor: argument shift is correctEmpty
allowplaceholder added; the colored “[working]” banner is now the label (printed once), and the title emoji stays in desc. The keydown color remains passed as the 5th arg. Looks good.
49-49: Pause waitfor: argument shift is correctEmpty
allowplaceholder added;$pausetype(colored label) and$pausetitle(emoji) map to label/desc as expected.walking.fish (1)
2-2: Call-site update looks correctInserted empty
allowargument; label/desc preserved. Matches the new waitfor contract.blooddonation.fish (1)
8-8: Suppress initial prompt via emptyallowargument
The second positional argument inwaitfornow maps to theallowparameter in the updatedwaitfor.fishsignature. Passing''here correctly disables the initial prompt as intended.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
install.sh (2)
58-60:abbris a fish builtin; running it in bash will fail.This script runs under bash. Calling
abbrhere will error. Execute these via fish or write them to config.-abbr -a --regex '[.][.][.]([/].*)?' --position anywhere --function ... -- git_root -abbr -a --regex '.*[{]-?[0-9]+[:]-?[0-9]+([:]-?[0-9]+)?[}].*' --position anywhere --function expand -- expand +fish -c "abbr -a --regex '[.][.][.]([/].*)?' --position anywhere --function ... -- git_root" +fish -c "abbr -a --regex '.*[{]-?[0-9]+[:]-?[0-9]+([:]-?[0-9]+)?[}].*' --position anywhere --function expand -- expand"Alternatively, append
abbrdefinitions to~/.config/fish/conf.d/abbreviations.fishso they load non-interactively.
61-65: Group/user tweaks lack idempotency and may fail without sudo; guard backlight path.
groupadderrors if group exists.usermodlacks sudo.- Backlight sysfs path may not exist on many systems.
-sudo groupadd light -usermod -aG light "$USER" -sudo chgrp light /sys/class/leds/rgb:kbd_backlight/ -sudo chmod g+w -R /sys/class/leds/rgb:kbd_backlight/ +getent group light >/dev/null || sudo groupadd light +sudo usermod -aG light "$USER" +if [ -d "/sys/class/leds/rgb:kbd_backlight/" ]; then + sudo chgrp -R light /sys/class/leds/rgb:kbd_backlight/ + sudo chmod -R g+w /sys/class/leds/rgb:kbd_backlight/ +fi
🧹 Nitpick comments (8)
gitcpr.fish (2)
1-1: Typo in description ("een" → "a").Small wording fix in the user-facing description.
-function gitcpr --description 'commit with the given parameters, push the changes and open een pull request' +function gitcpr --description 'commit with the given parameters, push the changes and open a pull request'
1-6: Add basic guards and clearer UX.Fail fast outside a git repo and when no message is provided; minimize confusing errors.
function gitcpr --description 'commit with the given parameters, push the changes and open a pull request' + if not command -sq git + echo "git is not installed or not on PATH." 1>&2 + return 127 + end + if not type -q gh + echo "gh (GitHub CLI) is not installed or not on PATH." 1>&2 + return 127 + end + if not git rev-parse --is-inside-work-tree ^/dev/null + echo "Not inside a git repository." 1>&2 + return 2 + end + if test (count $argv) -eq 0 + echo "Usage: gitcpr <commit message...>" 1>&2 + return 2 + end set -l message (string join " " -- $argv) git commit -am "$message" || return $status gitp - gh pr create --title "$message" --head + set -l head (git rev-parse --abbrev-ref HEAD) + gh pr create --title "$message" --head "$head" --fill || return $status endinstall.sh (2)
36-39: Usemktempinstead of deprecatedtempfile.
tempfileis obsolete/not installed by default.mktempis portable.-tmp=$(tempfile) +tmp=$(mktemp) curl -L https://git.io/haskell-vim-now > "$tmp" bash "$tmp" rm -f "$tmp"
12-12: Long apt-get line: consider splitting for readability and failures.Purely readability/maintainability; splitting makes diffs and troubleshooting easier.
-sudo apt-get install -y alarm-clock-applet apt-file agrep certbot cloc curl cvc4 djinn fish gh git git-filter-repo git-lfs gnuplot-nox haskell-stack hlint hoogle ipython3 jq lambdabot ledger lynx nodeenv nodejs npm pandoc pass-extension-otp pdfsam powerline pv pylint python-wxtools python3-pytest rename sdate sox supervisor swi-prolog-core tig tox xdotool yarn zbar-tools z3 +sudo apt-get install -y \ + alarm-clock-applet apt-file agrep certbot cloc curl cvc4 djinn fish gh git \ + git-filter-repo git-lfs gnuplot-nox haskell-stack hlint hoogle ipython3 jq \ + lambdabot ledger lynx nodeenv nodejs npm pandoc pass-extension-otp pdfsam \ + powerline pv pylint python-wxtools python3-pytest rename sdate sox supervisor \ + swi-prolog-core tig tox xdotool yarn zbar-tools z3README.md (4)
73-73: Fix typo (“een” → “a”) and satisfy MD007 (indent).The bullet is indented by two spaces; markdownlint expects 0 here. Also fix “een”.
- - **`gitcpr`**: commit with the given parameters, push the changes and open een pull request; +- **`gitcpr`**: commit with the given parameters, push the changes and open a pull request;
135-139: Normalize list formatting in section O (MD007).Remove leading indentation to match linter expectations.
-### O - - - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp; +### O + +- **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp;
159-159: Minor wording polish (optional).“latest pull request” is fine; “most recent PR” reads a bit tighter in CLI docs. Also adjust indent to appease MD007 if necessary in your linter config.
- - **`rabbitdoc`**: ask [*CodeRabbit*](https://github.com/coderabbitai) to generate docstrings for the latest pull request; +- **`rabbitdoc`**: ask [*CodeRabbit*](https://github.com/coderabbitai) to generate docstrings for the most recent PR;
194-194: Grammar: add a separator and clarify exit status; fix indent (MD007).Currently reads “…hits a key exits…”. Add a semicolon and specify “status 1”.
- - **`timeprompt`**: wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted; +- **`timeprompt`**: wait for a given amount of time, or until the person hits a key; exits with status 1 if the user interrupted;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
README.md(4 hunks)gitcpr.fish(1 hunks)install.sh(1 hunks)rabbitdoc.fish(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
137-137: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
159-159: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
194-194: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 LanguageTool
README.md
[grammar] ~159-~159: There might be a mistake here.
Context: ... docstrings for the latest pull request; - redo: redo a command until the exit code is ...
(QB_NEW_EN)
[grammar] ~194-~194: There might be a mistake here.
Context: ...ey exits with 1 if the user interrupted; - timestamp: get a string that specifies date and t...
(QB_NEW_EN)
| set message (echo "$argv") | ||
| git commit -am $argv; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Commit message handling breaks for multi-word messages. Quote and join argv.
git commit -am $argv passes only the first word to -m and treats the remainder as pathspecs. Build a single message string and quote it.
- set message (echo "$argv")
- git commit -am $argv;
+ set -l message (string join " " -- $argv)
+ git commit -am "$message" || return $statusNote: keeping -a preserves prior behavior (stage modified, not new, files). If you want to include new files by default, switch to git add -A && git commit -m "$message".
📝 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.
| set message (echo "$argv") | |
| git commit -am $argv; | |
| set -l message (string join " " -- $argv) | |
| git commit -am "$message" || return $status |
🤖 Prompt for AI Agents
In gitcpr.fish around lines 2-3, the commit invocation uses git commit -am $argv
which only passes the first word as the message and treats remaining words as
pathspecs; instead join argv into a single message string and quote it when
passing to git. Set a local message variable by joining $argv (e.g. using string
join " " $argv) and then call git commit -am "$message" so multi-word commit
messages are preserved.
gitcpr.fish
Outdated
| set message (echo "$argv") | ||
| git commit -am $argv; | ||
| gitp | ||
| gh pr create --title "$message" --head |
There was a problem hiding this comment.
gh pr create --head requires a value; command will fail.
--head needs the branch name (e.g., owner:branch or just branch for current repo). Without it, gh errors.
- gh pr create --title "$message" --head
+ set -l head (git rev-parse --abbrev-ref HEAD)
+ gh pr create --title "$message" --head "$head" --fill || return $status📝 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.
| gh pr create --title "$message" --head | |
| set -l head (git rev-parse --abbrev-ref HEAD) | |
| gh pr create --title "$message" --head "$head" --fill || return $status |
🤖 Prompt for AI Agents
In gitcpr.fish around line 5, the gh command uses --head without a value which
causes gh to fail; change the command to pass the branch name to --head (either
a provided/constructed variable like the current branch or an explicit
owner:branch string) so that --head receives a valid branch identifier when
creating the PR.
| @@ -1,12 +1,15 @@ | |||
| #!/bin/bash | |||
|
|
|||
| curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add - | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid deprecated apt-key; use signed-by keyrings for Yarn too.
apt-key is deprecated. Use a keyring and reference it in the source entry, mirroring what you already do for GitHub CLI.
-curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add -
+curl -fsSL https://dl.yarnpkg.com/debian/pubkey.gpg | sudo gpg --dearmor -o /usr/share/keyrings/yarn-archive-keyring.gpg
@@
-echo "deb https://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.list
+echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/yarn-archive-keyring.gpg] https://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.listAlso applies to: 6-6
🤖 Prompt for AI Agents
install.sh lines 3 and 6: the script uses the deprecated apt-key to add Yarn's
GPG key; replace this with a dedicated keyring file and reference it via
signed-by in the APT source entry. Download the Yarn pubkey into a keyring
(e.g., /usr/share/keyrings/yarn-archive-keyring.gpg) with proper permissions,
remove the apt-key add call, and update the Yarn repository line to include
[signed-by=/usr/share/keyrings/yarn-archive-keyring.gpg] so APT uses the keyring
instead of apt-key.
rabbitdoc.fish
Outdated
| @@ -0,0 +1,3 @@ | |||
| function rabbitdoc --description 'ask CodeRabbit.ai to generate docstrings for the latest pull request' | |||
| gh pr comment (gh pr list --json number --jq '.[0].number') -b '@coderabbitai generate docstrings' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing PRs, missing gh, and make PR selection explicit.
If there are no PRs, the current command expands to gh pr comment null … and fails. Also ensure gh exists and pick the most recent open PR deterministically.
-function rabbitdoc --description 'ask CodeRabbit.ai to generate docstrings for the latest pull request'
- gh pr comment (gh pr list --json number --jq '.[0].number') -b '@coderabbitai generate docstrings'
+function rabbitdoc --description 'ask CodeRabbit.ai to generate docstrings for the latest pull request'
+ if not type -q gh
+ echo "gh (GitHub CLI) is not installed or not on PATH." 1>&2
+ return 127
+ end
+ # Allow optional PR number as first arg; otherwise pick the most recent open PR
+ set -l pr $argv[1]
+ if not string length -q -- $pr
+ set pr (gh pr list --state open --limit 1 --json number --jq '.[0].number' 2>/dev/null)
+ end
+ if not string length -q -- $pr
+ echo "No open pull requests found (and no PR number provided)." 1>&2
+ return 1
+ end
+ gh pr comment $pr -b '@coderabbitai generate docstrings'
end📝 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.
| gh pr comment (gh pr list --json number --jq '.[0].number') -b '@coderabbitai generate docstrings' | |
| function rabbitdoc --description 'ask CodeRabbit.ai to generate docstrings for the latest pull request' | |
| if not type -q gh | |
| echo "gh (GitHub CLI) is not installed or not on PATH." 1>&2 | |
| return 127 | |
| end | |
| # Allow optional PR number as first arg; otherwise pick the most recent open PR | |
| set -l pr $argv[1] | |
| if not string length -q -- $pr | |
| set pr (gh pr list --state open --limit 1 --json number --jq '.[0].number' 2>/dev/null) | |
| end | |
| if not string length -q -- $pr | |
| echo "No open pull requests found (and no PR number provided)." 1>&2 | |
| return 1 | |
| end | |
| gh pr comment $pr -b '@coderabbitai generate docstrings' | |
| end |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
waitfor.fish (1)
20-26: Fix TOCTOU on loop condition; honoruntil; clamp remaining; limit sleep to remaining timeThe condition reuses a stale
$curcaptured before sleeping, allowing an extra iteration and negative$rmd. Also,untilis parsed but ignored. Compute a stop epoch, evaluate time in the while-condition, clamp$rmd >= 0, and avoid oversleeping by capping sleep to the remaining time.Apply:
- set cur (date '+%s') - set tar (math "$cur + $time") - while test "$cur" -lt "$tar" - set cur (date '+%s') - set rmd (math "$tar - $cur") + set cur (date '+%s') + set tar (math "$cur + $time") + set stop "$tar" + if test "$until" -gt 0 -a "$until" -lt "$stop" + set stop "$until" + end + while test (date '+%s') -lt "$stop" + set cur (date '+%s') + set rmd (math "max(0, $stop - $cur)")- else - sleep "$step" + else + sleep (math "min($step, $rmd)") endAlso applies to: 42-49
🧹 Nitpick comments (4)
waitfor.fish (3)
27-30: Erase logic assumes fixed width 8; make it proportional to shrink amountWhen the formatted time shortens by more than 8 chars, leftovers remain.
- if [ "$n" -lt "$oldn" ] - # erase previous one if length differs - echo -en ' \e[8D' - end + if [ "$n" -lt "$oldn" ] + set diff (math "$oldn - $n") + echo -en (string repeat -n "$diff" ' ')"\e["$diff"D" + end
33-35: Clamp progress to [0,100] and avoid division by zero when time ≤ 0Prevents >100% and errors on degenerate input.
- set pct (math "round(100*($time-$rmd)/$time)") + if test "$time" -le 0 + set pct 100 + else + set pct (math "min(100, max(0, round(100*($time-$rmd)/$time)))") + end
35-40: Backlight update: clamp to [0,255] and avoid spawning background jobsWrites to sysfs only on change (good), but the
&spawns extra processes. Clamp and write synchronously.- set newval (math "round(235*$rmd/$time)+20") + set newval (math "min(255, max(0, round(235*$rmd/$time)+20))") if [ "$oldval" -ne "$newval" ] - echo "$newval" > /sys/class/leds/rgb:kbd_backlight/brightness & + echo "$newval" > /sys/class/leds/rgb:kbd_backlight/brightness set oldval $newval endblooddonation.fish (1)
8-12: Negation style and branch intent: confirm outcome and prefernotfor readability
- fish supports
!as an alias fornot(since 3.0), butnotreads clearer. (fishshell.com)- Verify the branch semantics: as written, a non-zero
waitforstatus logs'blooddonation.end'and success runshere_is_the_news. Is that intentional?Minimal readability tweak:
- if ! waitfor 2700 '' '🩸 donation' '🩸 ' (getcolor blood body_colors ,) + if not waitfor 2700 '' '🩸 donation' '🩸 ' (getcolor blood body_colors ,)If the intent is “run news while waiting, then log end when done,” we should invert or restructure. I can draft that once you confirm desired behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
blooddonation.fish(1 hunks)waitfor.fish(2 hunks)
🔇 Additional comments (1)
waitfor.fish (1)
3-5: Ensurekeycolorbackground job terminates cleanlyIf
keycolor … &is long-lived, it may outlivewaitfor. Confirm it’s one-shot or add teardown.Would you like me to propose a pattern to capture the PID and send a reset on exit?
waitfor.fish
Outdated
| end | ||
| set cl (math "min(255, 4*$i)") | ||
| set cl (math "min(255, 4*$rmd)") | ||
| echo -en "\e[1m\e[38;2;255;$cl;"$cl"m$txt\e[0m\e["$n"D" |
There was a problem hiding this comment.
Broken ANSI escape due to quoting/arg-splitting; emit with printf as a single arg
The current echo mixes quoted and unquoted tokens, risking spaces that break SGR parameters.
Use printf:
- echo -en "\e[1m\e[38;2;255;$cl;"$cl"m$txt\e[0m\e["$n"D"
+ printf "\e[1m\e[38;2;255;%d;%dm%s\e[0m\e[%dD" $cl $cl $txt $n📝 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.
| echo -en "\e[1m\e[38;2;255;$cl;"$cl"m$txt\e[0m\e["$n"D" | |
| printf "\e[1m\e[38;2;255;%d;%dm%s\e[0m\e[%dD" $cl $cl $txt $n |
🤖 Prompt for AI Agents
In waitfor.fish around line 32, the echo command mixes quoted and unquoted
tokens which can cause word-splitting and break ANSI SGR parameters; replace the
echo invocation with a single-argument printf to avoid splitting and ensure the
entire escape sequence is emitted atomically. Construct the printf format so
escape codes and variables are expanded in one string (use a single format
string and pass variables as needed), ensure any numeric variables are not
subject to word splitting, and use the appropriate escape-sequence
interpretation flag so the SGR sequence, the text, and the final cursor-move
code are printed as one intact argument.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
gitcpr.fish (1)
2-3: Quote and join argv; current commit breaks multi-word messages.
git commit -am $argvpasses only the first word to-mand treats the rest as pathspecs.- set message (echo "$argv") - git commit -am $argv; + set -l message (string join " " -- $argv) + # Only commit when there are staged/modified changes; otherwise skip commit step. + git diff-index --quiet HEAD -- || git commit -am "$message" || return $status
🧹 Nitpick comments (5)
README.md (5)
73-73: Add entry is fine; consider silencing MD007 for two-space bullets.This bullet matches the surrounding two-space indentation style but triggers markdownlint MD007. Prefer adding a repo markdownlint config to allow indent=2 to avoid mass reformatting.
Example config addition (in .markdownlint.jsonc):
+{ + "MD007": { "indent": 2 } +}
81-81: Same as above: indentation style vs. MD007.Keep two-space bullets for consistency; configure markdownlint to accept indent=2.
136-139: Fix inconsistent indentation under “O”.This new bullet uses a single leading space while all other sections use two. Align it for consistency.
- - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp; + - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp;
160-160: Minor grammar: consider dropping the “latest”.“latest pull request” can be ambiguous in repos with multiple open PRs. If you truly target the most recent, keep as-is; otherwise “the current pull request” reads clearer.
195-195: Fix grammar in timeprompt description.Current: “wait … until the person hits a key exits with 1 …”. Missing conjunction/punctuation.
- - **`timeprompt`**: wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted; + - **`timeprompt`**: wait for a given amount of time, or until a key is pressed; exits with 1 if the user interrupts;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
README.md(4 hunks)gitcpr.fish(1 hunks)gitpr.fish(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
73-73: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
77-77: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
78-78: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
81-81: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
138-138: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
160-160: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
195-195: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 LanguageTool
README.md
[grammar] ~160-~160: There might be a mistake here.
Context: ... docstrings for the latest pull request; - redo: redo a command until the exit code is ...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...ey exits with 1 if the user interrupted; - timestamp: get a string that specifies date and t...
(QB_NEW_EN)
| gitp | ||
| gitpr $argv |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate subsequent steps on success; bubble up failures.
Ensure push succeeds before opening a PR, and forward exit codes to the caller.
- gitp
- gitpr $argv
+ gitp || return $status
+ gitpr $argv📝 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.
| gitp | |
| gitpr $argv | |
| gitp || return $status | |
| gitpr $argv |
🤖 Prompt for AI Agents
In gitcpr.fish around lines 4 to 5, the script currently runs gitp then
unconditionally calls gitpr $argv; change it to gate the PR step on a successful
push by checking the exit status of the gitp command and only invoking gitpr if
gitp succeeded, and if gitp fails propagate its non‑zero exit code to the caller
(e.g. test $status and exit with that status or use an early return) so failures
are bubbled up.
| @@ -0,0 +1,4 @@ | |||
| function gitpr --description 'open a pull request on GitHub' | |||
| set title (echo "$argv") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Join argv safely for multi-word titles.
echo "$argv" is brittle in fish; prefer string join and a local var.
- set title (echo "$argv")
+ set -l title (string join " " -- $argv)📝 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.
| set title (echo "$argv") | |
| set -l title (string join " " -- $argv) |
🤖 Prompt for AI Agents
In gitpr.fish around line 2, the code uses echo "$argv" which is brittle for
joining multi-word arguments; replace it by creating a local variable and using
fish's string join to safely combine argv (e.g., set -l title (string join " "
$argv)) and then use that local title variable wherever needed.
| @@ -0,0 +1,4 @@ | |||
| function gitpr --description 'open a pull request on GitHub' | |||
| set title (echo "$argv") | |||
| firefox (gh pr create --title "$title" --body "$title" 2>&1 | tail -n 1) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid parsing gh output and hard dependency on Firefox; use --web and propagate status.
2>&1 | tail -n 1 | firefox can open an error string as a URL and loses gh exit status. Let gh open the browser and return its status.
- firefox (gh pr create --title "$title" --body "$title" 2>&1 | tail -n 1)
+ if test -z "$title"
+ gh pr create --fill --web
+ else
+ gh pr create --title "$title" --body "$title" --web
+ end
+ return $status🤖 Prompt for AI Agents
In gitpr.fish around line 3, the current command pipes gh output into firefox
and captures stderr/stdout which can open error text as a URL and loses gh's
exit status; replace the pipeline with gh pr create --title "$title" --body
"$title" --web (no 2>&1 | tail -n 1 | firefox), and ensure the script exits with
gh's exit code (let gh return its status or capture and propagate it via exit
$status) so errors are preserved and no hard dependency on Firefox is
introduced.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
sleep_for.fish (2)
15-17: Inconsistent conditional syntax: usetestfor consistency with the rest of the script.Line 3 uses
testfor conditionals (if test "$n" -gt 12 -o "$n" -lt -12), but this block uses POSIX-style[ ]. For idiomatic Fish shell and consistency within the file, prefertest.Apply this diff for consistency:
- if [ ! -n "$noair" ] + if ! test -n "$noair" airplane & - fi + endNote: Also update
fitoend(Fish shell keyword).
1-17: Consider improving the parameter design for better ergonomics.The current design uses a positional parameter where passing any second argument disables the airplane. While this works, it's not immediately intuitive. Fish shell's named flag syntax (
--long noair) would make the intent clearer and self-documenting. For example:function sleep_for --description 'Sleep a given number of hours' -a n --long noair test -n "$n" || set n '7' ... if ! set -q noair # Check if flag was provided airplane & endThis is a nice-to-have for future refactoring to improve the API.
Would you like me to refactor this to use a named flag for better clarity?
note.fish (2)
5-7: Reconsider the dualhhmmassignments.The variable
hhmmis set twice: once before the prompt (line 5, used in line 6) and once after (line 7, used in lines 9 and 15). This captures the time before and after user input, but only the post-input time is used in feedback messages.If the intent is to track the actual logging timestamp (after user input completes), this is reasonable but should be documented. If both timestamps are needed for different purposes, consider renaming one (e.g.,
hhmm_promptvshhmm_logged) for clarity.
6-6: Consider extracting ANSI escape codes to variables for maintainability.The prompt on line 6 includes terminal title and color codes inline. While this works, extracting these to named variables (e.g.,
set TITLE_ESCandPROMPT_COLOR) would improve readability and make future style changes easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
note.fish(1 hunks)sleep_for.fish(2 hunks)
🔇 Additional comments (3)
sleep_for.fish (1)
1-1: Function signature extended with newnoairparameter.The new
-a noairpositional argument allows callers to optionally disable the airplane background process. Consider verifying that existing callers ofsleep_forcan still invoke the function without issues; passing a second argument when previously none was expected should be backward-compatible, but confirm no callers are affected.note.fish (2)
2-3: Clarify the argument handling inconsistency.Line 2 checks for
argv[1](first argument exists), but line 3 echoes all arguments viaecho $argv. If the intent is to use only the first argument, this should beecho $argv[1]. If the intent is to support multi-word entries as separate arguments, the current behavior is fine, but the check should be updated to match the action.Consider clarifying the behavior in a comment or updating line 3 for consistency with the condition on line 2.
8-19: LGTM—control flow and error handling are sound.The function correctly validates non-empty input, conditionally prints user feedback based on whether arguments were provided, and returns appropriate exit codes. The early error message (line 9) followed by the downstream empty check (line 12) is correct, though an explicit early
return 1after line 10 would make the intent even clearer.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleep_for.fish(2 hunks)
🔇 Additional comments (1)
sleep_for.fish (1)
8-14: Verify trap and cleanup behavior with conditionally set$gamma.The
$gammavariable is only set on line 35 (positive n case), not in the negative n branch (lines 26–32). The trap on line 37 unconditionally passes$gammato cleanup, which safely guards the kill withtest -n "$gamma"on line 9.However, verify that:
- The airplane process (line 16) is properly cleaned up by the trap on exit.
- The cleanup function should consider explicitly killing any background airplane process if it's not already reaped.
Also applies to: 35-37
| @@ -1,4 +1,4 @@ | |||
| function sleep_for --description 'Sleep a given number of hours' -a n | |||
| function sleep_for --description 'Sleep a given number of hours' -a n -a noair | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify the confusing semantics of the noair parameter.
The parameter name noair suggests disabling air/airplane mode, but the conditional logic if [ -n "$noair" ] means airplane is launched when noair is non-empty—the inverse of what the name implies. This is semantically misleading and error-prone for callers.
Recommend renaming the parameter to something clearer that reflects the actual behavior, e.g., -a use_airplane or -a run_airplane, or inverting the logic to match the current name.
Also applies to: 15-17
🤖 Prompt for AI Agents
In sleep_for.fish around lines 1 and 15-17, the boolean flag named "noair" is
semantically inverted (non-empty means airplane mode is launched), which is
confusing; rename the flag to something positive like "run_airplane" (or
"use_airplane") and update the function signature, all occurrences and
help/usage text accordingly so the conditional becomes if [ -n "$run_airplane" ]
to match the meaning, or alternatively keep the name and invert the conditional
logic and docs so non-empty truly means "no airplane"; ensure consistency across
the file and any callers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
variables.json (2)
2-3: Review time notation terminology.Lines 2–3 reference times as "decimal notation" (e.g.,
0800,1900). These values appear to be times in HHMM format rather than true decimal (base-10) notation. Consider using clearer terminology such as "24-hour time format" or "HHMM format" to avoid ambiguity.
6-6: Consider security posture for API token documentation.While the
GROQ_API_TOKENentry storesnullas a placeholder, documenting the presence of API token configuration in a version-controlled file (even without the actual token) may reveal internal tooling. If this file is intended to be user-facing or public, ensure that the exposure of integration points aligns with your security and transparency policies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setvar.fish(1 hunks)variables.json(1 hunks)
🔇 Additional comments (1)
variables.json (1)
1-8: Clarify the schema design and purpose of null placeholders.The file follows an unusual schema pattern where each value is an array containing a description string and
null. This structure is non-standard for a configuration or variable metadata file. A few questions:
- What is the intended purpose of the
nullplaceholder in each entry?- Is this schema aligned with how the codebase consumes this file?
- Would a more standard format like JSON Schema or a simpler object structure with only descriptions be more maintainable?
Consider adding a README or documentation comment explaining the file's structure and purpose.
| if ! set -q name | ||
| test -n "$helptext" && set helptext " [$helptext]" | ||
| read -Ux "$name" -P (string unescape '$\e[31m')"$name$helptext"(string unescape '\e[0m> ') | ||
| end |
There was a problem hiding this comment.
Critical: The variable-existence check is checking the wrong variable.
Line 2 checks if ! set -q name, which tests whether the parameter name is set. Since name is always set as a function argument, this condition will never trigger the prompt behavior on lines 3–4, breaking the function's documented purpose.
To check if a variable whose name is stored in $name exists, the condition must use eval:
- if ! set -q name
+ if ! eval "set -q $name"This allows the function to correctly prompt for a value only when the target variable doesn't exist.
📝 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.
| if ! set -q name | |
| test -n "$helptext" && set helptext " [$helptext]" | |
| read -Ux "$name" -P (string unescape '$\e[31m')"$name$helptext"(string unescape '\e[0m> ') | |
| end | |
| if ! eval "set -q $name" | |
| test -n "$helptext" && set helptext " [$helptext]" | |
| read -Ux "$name" -P (string unescape '$\e[31m')"$name$helptext"(string unescape '\e[0m> ') | |
| end |
🤖 Prompt for AI Agents
In setvar.fish around lines 2 to 5, the existence check incorrectly tests the
literal parameter "name" instead of the variable whose name is stored in $name;
replace the check with an eval-based check that tests the target variable by its
name (e.g., evaluate a `set -q` against the variable named by $name) so the
prompt block (lines 3–4) runs only when that target variable is unset, leaving
the rest of the logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
README.md (4)
75-75: Typo: "chance" → "change" (unresolved from prior review).The word "chance" should be "change". This was flagged in a previous review and remains unresolved.
- - **`gitclonec`**: clone a repository and chance the directory to that of the cloned repository; + - **`gitclonec`**: clone a repository and change to the cloned repository's directory;
106-106: Grammar: "case insensitive" → "case-insensitive" (unresolved from prior review).Compound adjectives preceding a noun require a hyphen.
- - **`igrep`**: run grep in a case insensitive way; + - **`igrep`**: run grep in a case-insensitive way;
147-149: Indentation: Fix section O formatting (unresolved from prior review).The
otpbullet is indented with 1 space instead of 2, making it inconsistent with all other bullets. Additionally, there's a missing blank line before the section heading.### O - - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp; + - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp;
208-208: Clarity: Clarifytimepromptdescription (unresolved from prior review).The current phrasing is grammatically awkward and unclear: "or until the person hits a key exits with 1 if the user interrupted."
- - **`timeprompt`**: wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted; + - **`timeprompt`**: wait for a given amount of time or until a key is pressed; exits with 1 if the user interrupted;
🧹 Nitpick comments (1)
README.md (1)
277-279: Clarify the Measurements section rationale.The new Measurements section introduces a reference to
assets/Measurements.jsonas a core feature. For users unfamiliar with the system, consider briefly explaining what types of measurements are tracked or why cascading measurements matter.Currently: "One of the core features is that one can measure all sorts of parameters, like health, activities, notes, etc."
Consider adding an example or brief clarification about cascading measurements if they serve a specific use case (e.g., hierarchical organization, derived measurements, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(11 hunks)facialmask.fish(1 hunks)variables.json(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~106-~106: Use a hyphen to join words.
Context: ...# I - igrep: run grep in a case insensitive way; - import: initi...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
23-23: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
74-74: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
77-77: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
78-78: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
81-81: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
82-82: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
83-83: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
100-100: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
101-101: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
102-102: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
106-106: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
107-107: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
108-108: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
134-134: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
137-137: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
163-163: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
172-172: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
181-181: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
208-208: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
209-209: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
210-210: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
223-223: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (4)
facialmask.fish (1)
1-5: No issues found with thewaitforfunction call.The call
waitfor 1200 '' '🧖 facial mask' '🧖 'on line 3 correctly matches the refactored function signature. The four positional arguments properly map totime,allow,label, anddescparameters, with the optional parameters (keydown,until,step) left unset. This pattern is consistent with similar calls throughout the codebase (eggs.fish, household.fish, walking.fish, etc.).variables.json (3)
2-8: Configuration structure and documentation look good.The new variables are well-documented with clear descriptions and appropriate default values. The consistent array structure
[description, default_value]is easy to parse and maintain.
6-6: The original review comment is incorrect.The
variables.jsonfile is a configuration schema with documentation—the default value forGROQ_API_TOKENisnull, not an actual API token. The codebase properly handles the secret at runtime:groq.fishretrieves the token from an environment variable viasetvar GROQ_API_TOKENand passes it as an argument toask_groq.py, where it's used directly (api_key=argv[1]). No actual credentials are stored in version control.Likely an incorrect or invalid review comment.
3-3: Ensure backwards compatibility when integrating variables.json into runtime.The concern is conceptually valid but cannot be fully verified:
variables.jsonappears to be newly added but is not currently integrated into the codebase.setvarreads existing environment variables and expects scalar values.When variables.json is loaded and used at runtime, the loader must extract the second element (
default_value) from the array format, not pass the entire array tosetvar. The usages incoffee.fishandwhatday.fish(lines 3 and 10 respectively) perform numeric comparisons and will fail if they receive an array instead of a scalar.Verify that any code loading
variables.jsoncorrectly handles the[description, default_value]format.
| - **`proofreadedit`**: let vim open the file along with the proofread file; | ||
| - **`proofreadmd`**: proofread Markdown files with Groq to find spelling and grammatical errors; | ||
| - **`pwd`**: list the absolute path for the given files listed or the `pwd` if no arguments were provided; | ||
| - **`pydoc`**: add doc strings to all elements of a Python file; |
There was a problem hiding this comment.
Terminology: "doc strings" → "docstrings".
Use the standard Python term "docstrings" (one word) instead of "doc strings".
- - **`pydoc`**: add doc strings to all elements of a Python file;
+ - **`pydoc`**: add docstrings to all elements of a Python file;📝 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.
| - **`pydoc`**: add doc strings to all elements of a Python file; | |
| - **`pydoc`**: add docstrings to all elements of a Python file; |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
163-163: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In README.md around line 163, the phrase "doc strings" is used; change it to the
standard Python term "docstrings" (one word) so the line reads "**`pydoc`**: add
docstrings to all elements of a Python file;" ensuring consistency with Python
terminology.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
add_measure.py (1)
1-72: Address Black formatting failure.The pipeline indicates that Black formatting has failed. Run
black add_measure.pyto automatically format the code according to the project's style guidelines.#!/bin/bash # Fix Black formatting black add_measure.py
🧹 Nitpick comments (3)
add_measure.py (3)
9-9: Remove unused import.The
sqlite3module is imported but never used. Peewee'sSqliteDatabasehandles SQLite interactions directly.Apply this diff to remove the unused import:
-import sqlite3
35-36: Properly manage database connection lifecycle.The database connection is opened but never explicitly closed. While Peewee handles some cleanup, it's better to use a context manager or ensure proper cleanup, especially within a file-locked section.
Consider using Peewee's context manager:
- db.connect() - db.create_tables([Measurement]) + with db: + db.create_tables([Measurement])Or ensure the connection is closed after use:
# At the end of the FileLock block db.close()
43-45: Remove redundant timezone stripping.Line 43 already removes timezone information from
dt2with.replace(tzinfo=None), so line 45's.replace(tzinfo=None)is redundant sincedt2is already timezone-naive.Apply this diff to remove the redundant operation:
- timefilter = dt2 = parse(dt2).replace(tzinfo=None) + dt2 = parse(dt2).replace(tzinfo=None) + timefilter = dt2 if dt2 is not None: - key_dt = dt2.replace(tzinfo=None) + key_dt = dt2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
add_measure.py(2 hunks)assets/.gitignore(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI/CD
add_measure.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Run 'black --write' to fix code style issues.
| json.dump(data, f, indent=4) | ||
| move(f"{ASSET_PATH}.tmp", ASSET_PATH) | ||
| datum = {} | ||
| for entry in Measurement.select().where(Measurement.key == key, Measurement.created_date >= timefilter.isoformat()): |
There was a problem hiding this comment.
Fix type mismatch in database query.
The comparison Measurement.created_date >= timefilter.isoformat() compares a DateTimeField to a string. Peewee expects datetime objects for datetime comparisons, not ISO-formatted strings. This will likely cause incorrect query results or errors.
Apply this diff to compare datetime objects directly:
- for entry in Measurement.select().where(Measurement.key == key, Measurement.created_date >= timefilter.isoformat()):
+ for entry in Measurement.select().where(Measurement.key == key, Measurement.created_date >= timefilter):📝 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.
| for entry in Measurement.select().where(Measurement.key == key, Measurement.created_date >= timefilter.isoformat()): | |
| for entry in Measurement.select().where(Measurement.key == key, Measurement.created_date >= timefilter): |
🤖 Prompt for AI Agents
In add_measure.py around line 70, the query currently compares
Measurement.created_date (a DateTimeField) to timefilter.isoformat() (a string);
change the comparison to use a datetime object instead—either pass timefilter
directly if it is already a datetime, or parse the ISO string back to a datetime
(e.g., datetime.fromisoformat / dateutil.parser.parse) and use that datetime in
the WHERE clause, making sure to preserve timezone awareness/UTC handling
consistent with Measurement.created_date.
| move(f"{ASSET_PATH}.tmp", ASSET_PATH) | ||
| datum = {} | ||
| for entry in Measurement.select().where(Measurement.key == key, Measurement.created_date >= timefilter.isoformat()): | ||
| datum[parse(entry.created_date).replace(microsecond=0)] = json.loads(entry.value) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary datetime parsing.
entry.created_date is already a datetime object returned by Peewee's DateTimeField, so calling parse(entry.created_date) is unnecessary and adds overhead.
Apply this diff to use the datetime object directly:
- datum[parse(entry.created_date).replace(microsecond=0)] = json.loads(entry.value)
+ datum[entry.created_date.replace(microsecond=0)] = json.loads(entry.value)📝 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.
| datum[parse(entry.created_date).replace(microsecond=0)] = json.loads(entry.value) | |
| datum[entry.created_date.replace(microsecond=0)] = json.loads(entry.value) |
🤖 Prompt for AI Agents
In add_measure.py around line 71, the code unnecessarily calls parse() on
entry.created_date even though Peewee already returns a datetime; replace
parse(entry.created_date).replace(microsecond=0) with
entry.created_date.replace(microsecond=0) so you use the existing datetime
object directly and avoid the extra parsing overhead, then assign
datum[entry.created_date.replace(microsecond=0)] = json.loads(entry.value).
| *_measurements.json.gz | ||
| measurements*.sqlite3 | ||
| *_measurements.sqlite3 | ||
| *_measurements.slite3.gz |
There was a problem hiding this comment.
Fix typo in file extension.
Line 6 contains "slite3" instead of "sqlite3".
Apply this diff to fix the typo:
-*_measurements.slite3.gz
+*_measurements.sqlite3.gz📝 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.
| *_measurements.slite3.gz | |
| *_measurements.sqlite3.gz |
🤖 Prompt for AI Agents
In assets/.gitignore around line 6, the filename pattern contains a typo
("slite3") instead of the correct SQLite extension; update the pattern to
"*_measurements.sqlite3.gz" to match actual files, save the file, and commit the
change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
README.md (5)
76-76: Typo: "chance" → "change".- - **`gitclonec`**: clone a repository and chance the directory to that of the cloned repository; + - **`gitclonec`**: clone a repository and change to the cloned repository's directory;
107-107: Hyphenate compound adjective: "case-insensitive".- - **`igrep`**: run grep in a case insensitive way; + - **`igrep`**: run grep in a case-insensitive way;
150-150: Fix indentation for section O entry.The
otpbullet has 1 space instead of 2, making it inconsistent with all other bullets in the document.### O - - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp; + - **`otp`**: generate/configure One Time Passwords (OTPs) with pass-extension-otp;
164-164: Use standard Python terminology: "docstrings".- - **`pydoc`**: add doc strings to all elements of a Python file; + - **`pydoc`**: add docstrings to all elements of a Python file;
209-209: Clarify awkwardtimepromptdescription.The phrasing is unclear. Improve for readability and grammar.
- - **`timeprompt`**: wait for a given amount of time, or until the person hits a key exits with 1 if the user interrupted; + - **`timeprompt`**: wait for a given amount of time or until a key is pressed; exits with 1 if the user interrupted;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(12 hunks)dockerscpload.fish(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~107-~107: Use a hyphen to join words.
Context: ...# I - igrep: run grep in a case insensitive way; - import: initi...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
23-23: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
75-75: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
76-76: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
77-77: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
78-78: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
80-80: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
81-81: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
82-82: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
83-83: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
84-84: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
101-101: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
102-102: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
103-103: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
107-107: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
108-108: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
109-109: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
136-136: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
137-137: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
138-138: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
164-164: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
173-173: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
182-182: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
209-209: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
210-210: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
213-213: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
224-224: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
| function dockerscpload --description 'transfer docker images over ssh to a remote directly loading these in docker at the remote side' -a host | ||
| set i 1 | ||
| set n (count $argv[2..]) | ||
| echo "$name" |
There was a problem hiding this comment.
Remove undefined variable reference.
Line 4 references $name before it's defined in the for loop on Line 5. This will output an empty value and should be removed.
- echo "$name"
for name in $argv[2..]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dockerscpload.fish around line 4, the echo "$name" references the $name
variable before it is set by the for loop on line 5; remove this premature echo
(or move it inside the loop after $name is assigned) so nothing undefined is
printed and output only occurs after $name is initialized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pomodoro.fish (1)
33-33:⚠️ Background process cleanup remains incomplete — unresolved from previous review.The trap at line 29 only kills
gamma(the noise process). Background processes spawned bymeasurelog(lines 33, 51) andhere_is_the_news(lines 48, 53) are not tracked or explicitly terminated. If the script exits abnormally, these processes can orphan indefinitely.The previous review flagged this exact issue and provided a solution: maintain a list of spawned background PIDs and extend the trap handler to kill them. That fix was not applied. Please implement process tracking as suggested in the prior feedback.
Also applies to: 48-48, 51-51, 53-53
🧹 Nitpick comments (1)
assets/colors/fix.py (1)
21-21: Considerlower()instead ofcasefold()for hex strings.Since hex color strings contain only ASCII characters (0-9, a-f, A-F, #),
lower()is more conventional and semantically clearer thancasefold(). While functionally equivalent for this use case,casefold()is designed for Unicode caseless matching, which is unnecessary here.- rec["hex"] = rec["hex"].casefold() + rec["hex"] = rec["hex"].lower()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
assets/colors/coffee.json(1 hunks)assets/colors/fix.py(1 hunks)assets/colors/hour.json(1 hunks)assets/colors/season.json(1 hunks)blooddonation.fish(1 hunks)facialmask.fish(1 hunks)getcolor.fish(1 hunks)hourcolor.fish(1 hunks)pomodoro.fish(1 hunks)presleep.fish(1 hunks)sleep_for.fish(3 hunks)wakeup.fish(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- assets/colors/season.json
- assets/colors/coffee.json
🚧 Files skipped from review as they are similar to previous changes (2)
- sleep_for.fish
- hourcolor.fish
🔇 Additional comments (8)
pomodoro.fish (1)
18-19: ✅ Clean error handling via ending-variable loop.The new ending-controlled while loop with
|| set ending 1onwaitforfailures provides predictable termination on error. This is a solid improvement that prevents cascading failures in subsequent iterations.Also applies to: 34-34, 52-52
facialmask.fish (2)
1-6: The original review comment is incorrect. All dependencies referenced infacialmask.fishare properly defined in the codebase:
measurelog— defined inmeasurelog.fishgetcolor— defined ingetcolor.fishhere_is_the_news— defined inhere_is_the_news.fishwaitfor— defined inwaitfor.fishplayandtimeout— legitimate system commands (external utilities)The functions are used throughout multiple files in the codebase with no issues. No parameter validation or dependency checks are needed for locally-defined functions; they're part of the project.
Likely an incorrect or invalid review comment.
4-4: The review comment is incorrect; the trailing comma syntax is valid and intentional.The trailing comma in
(getcolor random heal ,)is the third argument to thegetcolorfunction, specifying the join character for RGB output. Thegetcolorfunction properly implements this parameter usingstring join "$join", resulting in comma-separated RGB values (e.g., "255,100,50"). When passed towaitforas thekeydownparameter, the value is correctly split by comma viastring split , "$keydown"and expanded into individual arguments for thekeycolorfunction. This pattern is used consistently across multiple functions in the codebase and is syntactically and functionally correct.getcolor.fish (1)
4-4: LGTM! Path update aligns with reorganized color assets.The palette data path now correctly points to
assets/colors/$palette.json, aligning with the new directory structure that includeshour.json,season.json, andcoffee.jsonin theassets/colors/subdirectory.presleep.fish (1)
10-10: Palette rename verification confirmed.The
wakeuppalette exists atassets/colors/wakeup.jsonwith valid structure, the oldwakeup_colors.jsonfile has been properly removed, andpresleep.fishline 10 correctly references the new palette name. No stale references to the old name remain. The changes are consistent and complete.assets/colors/hour.json (1)
18-24: Duplicate color definitions confirmed—verify intentionality.The color duplicates you identified are accurate:
- "Late Night 02" and "Mid Night 22" both have RGB
[30, 10, 75]/ hex"#1E0A4B"- "Deep Night 01" and "Late Night Dark 23" both have RGB
[15, 0, 50]/ hex"#0F0032"These occur at opposite ends of the 24-hour cycle. If this symmetry (early morning darkness = late evening darkness) is intentional design for a circular progression, document it in a comment. Otherwise, verify that one value in each pair needs correction.
wakeup.fish (1)
14-14: Palette migration verified—no issues found.The
wakeuppalette exists atassets/colors/wakeup.jsonwith valid structure. Thegetcolorfunction correctly resolves palette names by constructing paths asassets/colors/$palette.json, and the code change in wakeup.fish properly passeswakeupas the palette parameter. No oldwakeup_colorsreferences remain in the codebase.blooddonation.fish (1)
8-12: ****The review comment misinterprets the code structure. The
getcolor blood body ,call retrieves the color named "blood" from the "body" palette, not from a "blood" palette. Thebody.jsonpalette file exists and contains the "blood" entry with valid RGB values[237, 28, 36], so the code is correct as written. Noblood.jsonfile is needed.Likely an incorrect or invalid review comment.
| measurelog health.facialmask "$kind" & | ||
| timeout 1200 play -q -n synth 1200 brownnoise synth pinknoise mix synth sine amod 0.1 10 vol 0.2 >/dev/null 2>/dev/null & |
There was a problem hiding this comment.
Background processes fail silently without error handling.
Lines 2–3 spawn background tasks but don't verify they succeed. If measurelog, play, or timeout are missing or fail, the function continues without alerting the user. Consider surfacing errors or at least checking that critical dependencies exist before running them.
🤖 Prompt for AI Agents
In facialmask.fish around lines 2 to 3, the script launches background commands
(measurelog, timeout, play) without verifying they exist or checking their exit
status; add pre-flight checks using "type -q" (or command -v) to ensure each
binary exists and print a clear error and exit if any are missing, capture
background PIDs when launching them, redirect their stdout/stderr to a logfile,
wait for the PIDs and check their exit codes, and surface errors (non-zero
exits) to the user instead of allowing silent failures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dockerscpload.fish (1)
4-4: Remove the premature echo statement.Line 4 references
$namebefore it's defined by the for loop on line 5. This outputs an empty value and should be removed entirely. The misplaced indentation suggests this was accidentally left in.set i 1 set n (count $argv[2..]) - echo "$name" for name in $argv[2..]
🧹 Nitpick comments (1)
dockerscpload.fish (1)
9-14: Add error handling for docker and SSH operations.The function lacks error handling for critical operations:
docker image inspect(line 9) can fail if the image doesn't exist- SSH operations (line 11) may fail due to connectivity or permissions
- No checks validate that
$hostis set before using it in SSH commands (line 10 checks only if it's non-empty, but not if it's a valid hostname)Consider adding checks like:
if not docker image inspect "$name" &>/dev/null echo "Error: Image '$name' not found" >&2 continue end
| end | ||
| set i (math "$i+1") | ||
| end | ||
| wait |
There was a problem hiding this comment.
Remove ineffective wait command.
The wait command on line 17 has no effect because none of the operations in the loop are backgrounded (no & suffix). All docker operations use sequential pipes (|), which complete synchronously before the loop exits. The wait command is only useful for backgrounded processes spawned with &.
set i (math "$i+1")
end
- wait
endIf you intend to parallelize the image transfers, background the operations with &, then wait makes sense. Otherwise, remove this line.
📝 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.
| wait |
🤖 Prompt for AI Agents
In dockerscpload.fish around line 17, the standalone "wait" is ineffective
because no commands in the loop are backgrounded; remove this line to avoid
confusion, or if you want parallel image transfers, run the per-image docker
operations with "&" to background them and keep "wait" so the script waits for
all background jobs to finish—choose one of these fixes and apply it
consistently.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores