Repository navigation
plugins: add Prayer Times 1.4.0 - #204
Conversation
- product-manifest no longer lists .oc-tmp/ drafts (they are not in the tree) - registry entry gains upstream and a fresh manifestSha256 - summary/tagline no longer truncated
- screenshots use a neutral demo city, no personal location in fixtures or defaults - clock24 defaults to false; tables show compact 5:11a times - clicking the glyph opens the panel; pointer cursor on hover - today's row in week/month is tinted, barred and bolded
- invalid latitude/longitude/offsets/Hijri shift are rejected rather than clamped, so a corrupt setting cannot push the widget off its defaults - CI pins actions to commit SHAs and runs read-only - no personal city left in defaults, tests or docs
- single metrics layer drives every dimension; week and month tables share it - labels come from the service instead of copied arrays - month header pinned above the scroller - outlined chips, hover affordances, pointer cursors - ring track, endpoint dot and a readable micro-label
- HOLIDAYS segment lists the next six Islamic holidays, not just the nearest - table headers no longer collide (8.5px, 2px right padding)
- footer zone with divider and two-column meta row - tappable holiday teaser with chevron and pointer cursor - week separators in the month table
mysweetpea
changed the title
|
Status on the two things visible in the timeline:
One tidy-up while I was in there: |
…theast Asia and Russia methods, nine more cities, precise ring
🦊 mika's reviewIncremental review: new commits since the last checkpoint. Summary
Estimated review effort: 3/5 · risk: high 📁 Walkthrough details
|
| Severity | Category | Fix effort | File | Issue |
|---|---|---|---|---|
| 🟡 minor | correctness | low | plugins/prayer-times/bin/prayer-times |
Bug — Inherited names such as constructor pass this check even though they are not calculation methods. Check for an own property so invalid CLI input cannot reach the engine as a method. |
| 🟡 minor | correctness | medium | plugins/prayer-times/bin/prayer-times |
Possible Bug — Waybar switches to tomorrow's event immediately after the final prayer, but its tooltip still shows today's date and today's times. After Isha, the highlighted text and tooltip therefore describe different schedules; inc… |
| 🟡 minor | correctness | low | plugins/prayer-times/content/Panel.qml |
Bug — If active becomes true before service is available, panelOpen remains false when the service arrives, so the open panel never receives second-precision updates. Synchronize it when service changes as well. |
| 🟡 minor | correctness | low | plugins/prayer-times/content/Panel.qml |
Bug — rowNext matches only the time, so two prayers with the same adjusted minute can both be highlighted. Match nextKey as rowNow already does. |
| 🟡 minor | correctness | high | plugins/prayer-times/content/Panel.qml |
Possible Bug — The new panel uses raw English labels and messages, including the tab names and twilight warning, so they remain untranslated when the shell runs in another locale. Route these strings and the service's user-visible labe… |
| 🟡 minor | correctness | low | plugins/prayer-times/lib/hijri.js |
Bug — toGregorian accepts numeric strings, but adding a string hd to the month start concatenates rather than adds, producing an invalid date. Reject nonnumeric inputs before calculating the index. |
| 🟡 minor | correctness | low | plugins/prayer-times/lib/praytimes.js |
Bug — tomorrowFajr is relative to tomorrow's midnight even when it is greater than today's Maghrib, so the conditional can produce a night that is a day too short. Always add 1440 minutes before subtracting today's Maghrib. |
⚡ Quick fixes by effort
Low effort (apply from the diff):
plugins/prayer-times/lib/praytimes.js— Correct tomorrow's Fajr day offsetplugins/prayer-times/lib/hijri.js— Reject nonnumeric Hijri date componentsplugins/prayer-times/content/Panel.qml— Synchronize panel state when service arrivesplugins/prayer-times/bin/prayer-times— Reject inherited method namesplugins/prayer-times/content/Panel.qml— Match the next prayer by keyplugins/prayer-times/lib/praytimes.js:162 — Bug —tomorrowFajris relative to tomorrow's midnight even when it is greater than today's Maghrib, so the conditional can produce a night that is a day too short. Always add 1440 minutes before subtracting today's Maghrib.plugins/prayer-times/lib/hijri.js:247 — Bug —toGregorianaccepts numeric strings, but adding a stringhdto the month start concatenates rather than adds, producing an invalid date. Reject nonnumeric inputs before calculating the index.plugins/prayer-times/content/Panel.qml:44 — Bug — Ifactivebecomes true beforeserviceis available,panelOpenremains false when the service arrives, so the open panel never receives second-precision updates. Synchronize it whenservicechanges as well.plugins/prayer-times/bin/prayer-times:106 — Bug — Inherited names such asconstructorpass this check even though they are not calculation methods. Check for an own property so invalid CLI input cannot reach the engine as a method.plugins/prayer-times/content/Panel.qml:278 — Bug —rowNextmatches only the time, so two prayers with the same adjusted minute can both be highlighted. MatchnextKeyasrowNowalready does.
Medium effort (a function-level change):
plugins/prayer-times/bin/prayer-times:190 — Possible Bug — Waybar switches to tomorrow's event immediately after the final prayer, but its tooltip still shows today's date and today's times. After Isha, the highlighted text and tooltip therefore describe different schedules; inc…
**High effort** (design / multi-file — consider splitting)
plugins/prayer-times/content/Panel.qml:31 — Possible Bug — The new panel uses raw English labels and messages, including the tab names and twilight warning, so they remain untranslated when the shell runs in another locale. Route these strings and the service's user-visible labe…
🤖 Prompt for AI agents (click, copy, paste into Claude/Codex)
Fix the following issues found by mika-agent on Ryoku-dev/ryostore#204 (branch plugin/prayer-times):
- plugins/prayer-times/lib/praytimes.js:162-167 — Bug (minor severity, low effort): `tomorrowFajr` is relative to tomorrow's midnight even when it is greater than today's Maghrib, so the conditional can produce a night that is a day too short. Always add 1440 minutes before subtracting today's Maghrib.
suggested fix:
function nightInfo(today, tomorrowFajr) {
if (!today || typeof today.maghrib !== "number" || !isFinite(today.maghrib)) return null;
var fajr = tomorrowFajr;
if (typeof fajr !== "number" || !isFinite(fajr)) fajr = today.fajr;
if (typeof fajr !== "number" || !isFinite(fajr)) return null;
var night = fajr + 1440 - today.maghrib;
- plugins/prayer-times/lib/hijri.js:247-249 — Bug (minor severity, low effort): `toGregorian` accepts numeric strings, but adding a string `hd` to the month start concatenates rather than adds, producing an invalid date. Reject nonnumeric inputs before calculating the index.
suggested fix:
function toGregorian(hy, hm, hd) {
if (typeof hy !== "number" || typeof hm !== "number" || typeof hd !== "number") return null;
if (hm < 1 || hm > 12 || hm % 1 !== 0 || hd % 1 !== 0 || hy % 1 !== 0) return null;
var i = (hy - FIRST_YEAR) * 12 + (hm - 1);
- plugins/prayer-times/content/Panel.qml:44-47 — Bug (minor severity, low effort): If `active` becomes true before `service` is available, `panelOpen` remains false when the service arrives, so the open panel never receives second-precision updates. Synchronize it when `service` changes as well.
suggested fix:
onActiveChanged: if (service) service.panelOpen = active
onServiceChanged: if (service) service.panelOpen = active
Component.onDestruction: if (service) service.panelOpen = false
implicitWidth: root.widthBudget
- plugins/prayer-times/bin/prayer-times:106-111 — Bug (minor severity, low effort): Inherited names such as `constructor` pass this check even though they are not calculation methods. Check for an own property so invalid CLI input cannot reach the engine as a method.
suggested fix:
} else if (arg === '--method') {
if (++i >= process.argv.length || process.argv[i].startsWith('-'))
fail('--method requires an id');
methodOverride = process.argv[i];
if (!Object.prototype.hasOwnProperty.call(PT.METHODS, methodOverride))
fail(unknown method: ${methodOverride});
- plugins/prayer-times/content/Panel.qml:278-283 — Bug (minor severity, low effort): `rowNext` matches only the time, so two prayers with the same adjusted minute can both be highlighted. Match `nextKey` as `rowNow` already does.
suggested fix:
readonly property bool rowNext: root.service !== null && !rowNow
&& root.service.nextKey === modelData.key
&& modelData.at !== null && modelData.at === root.service.nextAt
&& modelData.at >= root.service.now
readonly property bool rowPast: root.service !== null && modelData.at !== null
&& modelData.at < root.service.now && !rowNow
- plugins/prayer-times/content/Panel.qml:31-36 — Possible Bug (minor severity, high effort): The new panel uses raw English labels and messages, including the tab names and twilight warning, so they remain untranslated when the shell runs in another locale. Route these strings and the service's user-visible labe…
- plugins/prayer-times/bin/prayer-times:190-195 — Possible Bug (minor severity, medium effort): Waybar switches to tomorrow's event immediately after the final prayer, but its tooltip still shows today's date and today's times. After Isha, the highlighted text and tooltip therefore describe different schedules; inc…
For each: verify it is real in the current code, apply the smallest correct fix, and run the repo's tests. Skip anything already resolved and say why.
🛠 Deterministic checks run
- linters: skipped (no checkout)
🚦 Merge recommendation: changes_required
🔍 mika-agent · self-hosted review for the Ryoku org · @mika-agent help for commands
neur0map
left a comment
neur0map
left a comment
There was a problem hiding this comment.
🔴 Changes requested — 1 critical and 2 major issue(s): see the inline comments and the walkthrough for fixes and an agent-ready prompt.
| name: validate | ||
|
|
||
| on: | ||
| push: | ||
| pull_request: |
There was a problem hiding this comment.
🟠 major · Tests · fix: medium effort
GitHub Actions discovers workflows only in the repository-root .github/workflows directory, so this nested workflow never runs for this PR. Move or integrate these checks into a root workflow, with commands executed from the plugin directory.
| jobs: | ||
| engine: | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
GitHub Actions only discovers workflows under the repository-root .github/workflows/ directory, so this nested workflow never runs. Move it to that directory and set its working directory to plugins/prayer-times so its relative paths resolve.
Suggested change
| jobs: | |
| engine: | |
| runs-on: ubuntu-latest | |
| steps: | |
| jobs: | |
| engine: | |
| runs-on: ubuntu-latest | |
| defaults: | |
| run: | |
| working-directory: plugins/prayer-times | |
| steps: |
| } else if (arg === '--city') { | ||
| if (++i >= process.argv.length) | ||
| fail('--city requires an id'); | ||
| cityId = process.argv[i]; |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: low effort
--city auto is documented but passes the literal auto to byId, which rejects it as an unknown city. Treat that value as automatic detection, as the bare auto option already does.
Suggested change
| } else if (arg === '--city') { | |
| if (++i >= process.argv.length) | |
| fail('--city requires an id'); | |
| cityId = process.argv[i]; | |
| cityId = process.argv[i] === 'auto' ? null : process.argv[i]; |
| const city = C.byId(cityId === null ? C.guess(Intl.DateTimeFormat().resolvedOptions().timeZone).city.id : cityId); | ||
| if (!city) | ||
| fail(`unknown city: ${cityId}`); |
There was a problem hiding this comment.
💡 Suggestion (bug)
Handle unsupported timezones without crashing
Suggested change
| const city = C.byId(cityId === null ? C.guess(Intl.DateTimeFormat().resolvedOptions().timeZone).city.id : cityId); | |
| if (!city) | |
| fail(`unknown city: ${cityId}`); | |
| const guessed = cityId === null ? C.guess(Intl.DateTimeFormat().resolvedOptions().timeZone) : null; | |
| const city = cityId === null ? (guessed && guessed.city) : C.byId(cityId); | |
| if (!city) | |
| fail(cityId === null ? 'no preset city for the system timezone; specify --city' : `unknown city: ${cityId}`); |
To apply: click the sparkletip, or ask @mika-agent ask <question>.
|
|
||
| const now = zoneNow(city.tz); | ||
| const date = parseDate(dateValue || now.iso); |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
C.guess() can return no city for an unsupported timezone, so dereferencing .city.id crashes instead of reporting the problem. Check the guessed city before using it.
Suggested change
| const now = zoneNow(city.tz); | |
| const date = parseDate(dateValue || now.iso); | |
| const guessed = cityId === null ? C.guess(Intl.DateTimeFormat().resolvedOptions().timeZone) : null; | |
| const city = cityId === null ? (guessed && guessed.city) : C.byId(cityId); | |
| if (!city) | |
| fail(cityId === null ? 'no preset city for the system timezone; specify --city' : `unknown city: ${cityId}`); |
| } | ||
|
|
||
| function ids() { | ||
| var out = []; | ||
| for (var i = 0; i < CITIES.length; i++) out.push(CITIES[i].id); | ||
| return out; | ||
| } |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
A timezone does not identify a city: America/New_York covers locations far from Baltimore, yet this marks Baltimore as exact and suppresses the estimated-location warning. Mark timezone guesses as estimated so users are prompted to select their actual city.
Suggested change
| } | |
| function ids() { | |
| var out = []; | |
| for (var i = 0; i < CITIES.length; i++) out.push(CITIES[i].id); | |
| return out; | |
| } | |
| function guess(tz) { | |
| if (tz) { | |
| for (var i = 0; i < CITIES.length; i++) | |
| if (CITIES[i].tz === tz) return { city: CITIES[i], exact: false }; | |
| } | |
| return { city: byId(FALLBACK), exact: false }; | |
| } |
| function toHijri(y, m, d, adjust) { | ||
| var idx = dayIndex(y, m, d) + (adjust || 0); | ||
| if (idx < MONTH_STARTS[0] || idx >= MONTH_STARTS[MONTH_STARTS.length - 1] + 30) return null; | ||
| var lo = 0, hi = MONTH_STARTS.length - 1; |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
The final table entry is the boundary after 1500 AH, not the start of another supported month. Treating it as a month returns fabricated dates in 1501 AH instead of reporting that the table is exhausted.
Suggested change
| function toHijri(y, m, d, adjust) { | |
| var idx = dayIndex(y, m, d) + (adjust || 0); | |
| if (idx < MONTH_STARTS[0] || idx >= MONTH_STARTS[MONTH_STARTS.length - 1] + 30) return null; | |
| var lo = 0, hi = MONTH_STARTS.length - 1; | |
| function toHijri(y, m, d, adjust) { | |
| var idx = dayIndex(y, m, d) + (adjust || 0); | |
| if (idx < MONTH_STARTS[0] || idx >= MONTH_STARTS[MONTH_STARTS.length - 1]) return null; | |
| var lo = 0, hi = MONTH_STARTS.length - 1; |
| function toGregorian(hy, hm, hd) { | ||
| var i = (hy - FIRST_YEAR) * 12 + (hm - 1); | ||
| if (i < 0 || i >= MONTH_STARTS.length) return null; | ||
| return fromIndex(MONTH_STARTS[i] + hd - 1); | ||
| } |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
toGregorian accepts invalid months and days, silently converting them into dates in adjacent months; it also accepts the terminal boundary as a month. Validate the components against the next month start before converting.
Suggested change
| function toGregorian(hy, hm, hd) { | |
| var i = (hy - FIRST_YEAR) * 12 + (hm - 1); | |
| if (i < 0 || i >= MONTH_STARTS.length) return null; | |
| return fromIndex(MONTH_STARTS[i] + hd - 1); | |
| } | |
| function toGregorian(hy, hm, hd) { | |
| if (hm < 1 || hm > 12 || hm % 1 !== 0 || hd % 1 !== 0) return null; | |
| var i = (hy - FIRST_YEAR) * 12 + (hm - 1); | |
| if (i < 0 || i >= MONTH_STARTS.length - 1 || i % 1 !== 0) return null; | |
| if (hd < 1 || hd > MONTH_STARTS[i + 1] - MONTH_STARTS[i]) return null; | |
| return fromIndex(MONTH_STARTS[i] + hd - 1); | |
| } |
| var g = (357.529 + 0.98560028 * D) % 360; | ||
| var q = (280.459 + 0.98564736 * D) % 360; | ||
| var L = (q + 1.915 * Math.sin(g * DEG) + 0.020 * Math.sin(2 * g * DEG)) % 360; | ||
| var e = 23.439 - 0.00000036 * D; | ||
| var ra = Math.atan2(Math.cos(e * DEG) * Math.sin(L * DEG), Math.cos(L * DEG)) / DEG / 15; | ||
| var decl = Math.asin(Math.sin(e * DEG) * Math.sin(L * DEG)) / DEG; | ||
| return { decl: decl, eqt: q / 15 - ((ra + 24) % 24) }; | ||
| } | ||
|
|
||
| // Hours from solar noon to the given depression angle. null when unsolvable. |
There was a problem hiding this comment.
🔴 critical · Critical bug · fix: low effort
q wraps at 360 degrees while right ascension wraps at a slightly different point, so their difference can be nearly 24 hours around the March equinox. Normalize the equation of time to prevent prayer times from being assigned to the wrong day.
Suggested change
| var g = (357.529 + 0.98560028 * D) % 360; | |
| var q = (280.459 + 0.98564736 * D) % 360; | |
| var L = (q + 1.915 * Math.sin(g * DEG) + 0.020 * Math.sin(2 * g * DEG)) % 360; | |
| var e = 23.439 - 0.00000036 * D; | |
| var ra = Math.atan2(Math.cos(e * DEG) * Math.sin(L * DEG), Math.cos(L * DEG)) / DEG / 15; | |
| var decl = Math.asin(Math.sin(e * DEG) * Math.sin(L * DEG)) / DEG; | |
| return { decl: decl, eqt: q / 15 - ((ra + 24) % 24) }; | |
| } | |
| // Hours from solar noon to the given depression angle. null when unsolvable. | |
| function sunPosition(jd) { | |
| var D = jd - 2451545.0; | |
| var g = (357.529 + 0.98560028 * D) % 360; | |
| var q = (280.459 + 0.98564736 * D) % 360; | |
| var L = (q + 1.915 * Math.sin(g * DEG) + 0.020 * Math.sin(2 * g * DEG)) % 360; | |
| var e = 23.439 - 0.00000036 * D; | |
| var ra = Math.atan2(Math.cos(e * DEG) * Math.sin(L * DEG), Math.cos(L * DEG)) / DEG / 15; | |
| var decl = Math.asin(Math.sin(e * DEG) * Math.sin(L * DEG)) / DEG; | |
| var eqt = q / 15 - ((ra + 24) % 24); | |
| eqt = ((eqt + 12) % 24 + 24) % 24 - 12; | |
| return { decl: decl, eqt: eqt }; | |
| } |
| readonly property var prayerKeys: ["fajr", "sunrise", "dhuhr", "asr", "maghrib", "isha"] | ||
| readonly property var prayerLabels: ["Fajr", "Sunrise", "Dhuhr", "Asr", "Maghrib", "Isha"] | ||
| readonly property var prayerShortLabels: ["FAJR", "SUNRISE", "DHUHR", "ASR", "MAGHRIB", "ISHA"] |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: high effort
The service and panel add English-only prayer names, dates, status messages and navigation labels, so translated shell sessions still display these strings in English. Route the user-visible strings through the i18n catalog.
|
Pushed 1.4.0 over the top of this branch — a deep accuracy + polish pass rather than more features. Accuracy (the real find). The engine sampled the sun once at local midnight, like PrayTimes does, which leaves the horizon events up to two minutes late: at the reported sunset the sun was already 0.23° past the angle. Every event is now solved with the sun where it actually is at that moment. Measured against an independent ephemeris, every Fajr/Sunrise/Maghrib/Isha now lands within 45 seconds of the true crossing (it was ~2 minutes). Moscow 7 Nov sunset went 16:40 → 16:38, Stockholm 15:42 → 15:40, matching published times. Methods and places. Added Honesty. A timezone no preset covers now says "set your city" instead of silently showing London's times, a date with no twilight says so instead of printing six dashes, and a preset city in another timezone is labelled "shown on your clock". Panel. The ring carries a second-precision countdown (mono, no jitter), gets a soft halo and an animated sweep, the Qibla line points an arrow at the bearing, tabs cross-fade and the panel height glides, holiday rows get hairlines. The service now rebuilds heavy state only on a day/settings change and ticks cheaply (1s while the panel is open, 30s collapsed). Tests. 251 assertions: the sky check above, 23 AlAdhan fixtures (adding Jakarta/Moscow/Denver/Auckland and a Stockholm-midsummer high-latitude rule case), and a full walk of all 1,896 months of the Umm al-Qura table. |
mysweetpea
changed the title
…i input, stop treating a timezone as a city - eqt folded into -12..12h: q and right ascension wrap a day apart around the March equinox, which shifted every event by 24h for ~2 days a year - toHijri/toGregorian reject out-of-range months, days and the table edge instead of rolling them into adjacent months - guess() never reports a timezone hit as an exact city; the zone's namesake wins - CLI: --city auto means detection; no crash when the zone has no preset - drop the nested .github workflow: GitHub only reads the repo root, so it was inert in the store (it stays in the author repo, where it runs)
neur0map
left a comment
neur0map
left a comment
There was a problem hiding this comment.
🔴 Changes requested — 0 critical and 2 major issue(s): see the inline comments and the walkthrough for fixes and an agent-ready prompt.
| readonly property var tabs: [ | ||
| { value: "today", label: "TODAY" }, | ||
| { value: "week", label: "WEEK" }, | ||
| { value: "month", label: "MONTH" }, | ||
| { value: "holidays", label: "HOLIDAYS" } | ||
| ] |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: high effort
The new panel and its service expose raw English strings, including tab labels, warnings, prayer names and holiday descriptions. Users running a translated shell will still see these strings in English; route them through the i18n catalog.
| // ticks once a second instead of once a minute | ||
| onActiveChanged: if (service) service.panelOpen = active |
There was a problem hiding this comment.
🟡 minor · Possible bug · fix: low effort
This snippet is too short for a valid suggestion.
confidence: 60/100
Suggested change
| // ticks once a second instead of once a minute | |
| onActiveChanged: if (service) service.panelOpen = active | |
| onActiveChanged: if (service) service.panelOpen = active | |
| Component.onDestruction: if (service) service.panelOpen = false |
| // Interval-based Isha (Umm al-Qura): 90 minutes after Maghrib. | ||
| if (ishaInterval && out.maghrib !== null) out.isha = out.maghrib + ishaInterval / 60; |
There was a problem hiding this comment.
🟠 major · Possible Bug · fix: medium effort
Umm al-Qura Isha is fixed at 90 minutes after Maghrib for every day, including Ramadan, when the convention uses 120 minutes. Users selecting Mecca or Medina therefore receive Isha times 30 minutes early throughout Ramadan; choose the interval using the applicable Hijri date.
| "source": "LICENSE", | ||
| "destination": "LICENSE", | ||
| "sha256": "3a7857c238a5754b5380348616d3d187aff6c2517e23bbf301b0a67581c445f8", | ||
| "mode": "0644", | ||
| "size": 1066, | ||
| "install": false |
There was a problem hiding this comment.
🟡 minor · Data Integrity · fix: medium effort
The product manifest excludes LICENSE from installation even though the plugin distributes MIT-licensed code and embeds the MIT-licensed Umm al-Qura table. Installed copies therefore lack the required license notice; install the license and include the upstream table's copyright and permission notice.
| svc.twilightMissing = todayTimes !== null && (todayTimes.fajr === null || todayTimes.maghrib === null) | ||
| && todayTimes.dhuhr !== null; |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: low effort
twilightMissing checks Fajr and Maghrib but not Isha. When Fajr is available and Isha is not, such as near a seasonal twilight boundary, the panel shows --:-- for Isha without the promised explanation; include Isha in the missing-twilight check.
…wording - bar glyph, holiday chevron and qibla marker use PluginKit GlyphIcon / drawn shapes instead of Unicode characters - chips share one width and 28px targets; ring gets 100px of room - metadata line separated by a hairline; footer halves get spacing - hint distinguishes 'auto-detected' from 'no preset for this timezone'
neur0map
left a comment
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
| const guessed = cityId === null ? C.guess(Intl.DateTimeFormat().resolvedOptions().timeZone) : null; | ||
| const city = cityId === null ? (guessed && guessed.city) : C.byId(cityId); | ||
| if (!city) | ||
| fail(cityId === null ? 'no preset city for this timezone; pass --city <id> (see --cities)' |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: low effort
C.guess() returns London even when matched is false, so running the CLI without --city in an unsupported timezone such as Asia/Kathmandu silently prints London prayer times instead of the advertised error. Reject an unmatched guess before selecting its fallback city.
| "custom/prayer": { | ||
| "interval": 60, | ||
| "return-type": "json", | ||
| "exec": "node ~/.local/share/ryoku/plugins/prayer-times/bin/prayer-times --waybar" |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: medium effort
The Waybar command invokes Node, but the manifest declares only readlink as a command dependency. On an installed system without Node, the documented module exits without producing JSON; declare the dependency or provide a runtime that is available on installed targets.
…lback - auto timezone no longer throws away a three-segment zone match - toHijri/toGregorian reject NaN, month 13, day 32, two-digit years and a non-numeric adjust before Date.UTC can normalise them - fmt and nightInfo reject non-finite input instead of printing NaN - the angle rule does not invent an isha when the method has no isha angle - CLI: value flags reject a following flag, exact-minute prayers count as next, out-of-day high-latitude values are skipped, usage lists every flag - tests/cli-test.js: 17 assertions over text/json/waybar, DST-free date math, the equinox date end to end, auto-detection and argument errors
neur0map
left a comment
neur0map
left a comment
There was a problem hiding this comment.
🔴 Changes requested — 0 critical and 1 major issue(s): see the inline comments and the walkthrough for fixes and an agent-ready prompt.
| const p = zoneParts(timeZone, new Date(utcNoon)); | ||
| return (Date.UTC(p.y, p.m - 1, p.d, p.hh, p.mm) - utcNoon) / 3600000; | ||
| } | ||
|
|
||
| function isoDate(date) { |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
zoneParts() can return hour 24 for midnight, but it normalizes that to 0 without adjusting the date. For zones at UTC+12, this makes zoneOffsetHours() return -12 instead of +12, shifting every CLI prayer time by a day. Request an h23 hour cycle so midnight is formatted as 00.
Suggested change
| const p = zoneParts(timeZone, new Date(utcNoon)); | |
| return (Date.UTC(p.y, p.m - 1, p.d, p.hh, p.mm) - utcNoon) / 3600000; | |
| } | |
| function isoDate(date) { | |
| function zoneOffsetHours(timeZone, y, m, d) { | |
| const utcNoon = Date.UTC(y, m - 1, d, 12); | |
| const parts = new Intl.DateTimeFormat('en-US', { | |
| timeZone, hourCycle: 'h23', | |
| year: 'numeric', month: '2-digit', day: '2-digit', | |
| hour: '2-digit', minute: '2-digit' | |
| }).formatToParts(new Date(utcNoon)); | |
| const get = (type) => Number(parts.find((p) => p.type === type).value); | |
| return (Date.UTC(get('year'), get('month') - 1, get('day'), get('hour'), get('minute')) - utcNoon) / 3600000; | |
| } |
| function dayIndex(y, m, d) { | ||
| return Math.floor(Date.UTC(y, m - 1, d) / 86400000) + RJD_EPOCH; | ||
| } |
There was a problem hiding this comment.
💡 Suggestion (bug)
Reject normalized invalid Gregorian dates
Suggested change
| function dayIndex(y, m, d) { | |
| return Math.floor(Date.UTC(y, m - 1, d) / 86400000) + RJD_EPOCH; | |
| } | |
| function dayIndex(y, m, d) { | |
| var t = new Date(Date.UTC(y, m - 1, d)); | |
| if (t.getUTCFullYear() !== y || t.getUTCMonth() + 1 !== m || t.getUTCDate() !== d) return NaN; | |
| return Math.floor(t.getTime() / 86400000) + RJD_EPOCH; | |
| } |
To apply: click the sparkletip, or ask @mika-agent ask <question>.
| var rule = opts.highLat || "none"; | ||
| if (rule !== "none" && out.sunrise !== null && out.maghrib !== null) { | ||
| var night = 24 - (out.maghrib - out.sunrise); // hours between sunset and next sunrise | ||
| if (out.fajr === null) { | ||
| if (rule === "middle") out.fajr = out.sunrise - night / 2; | ||
| else if (rule === "seventh") out.fajr = out.sunrise - night / 7; |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
High-latitude rules must also cap a computed Fajr that falls earlier than the permitted night portion, not just replace an unsolvable angle. Otherwise, selecting a rule can leave an excessively early Fajr unchanged.
Suggested change
| var rule = opts.highLat || "none"; | |
| if (rule !== "none" && out.sunrise !== null && out.maghrib !== null) { | |
| var night = 24 - (out.maghrib - out.sunrise); // hours between sunset and next sunrise | |
| if (out.fajr === null) { | |
| if (rule === "middle") out.fajr = out.sunrise - night / 2; | |
| else if (rule === "seventh") out.fajr = out.sunrise - night / 7; | |
| var fajrPortion = rule === "middle" ? night / 2 : | |
| rule === "seventh" ? night / 7 : | |
| rule === "angle" && typeof fajrAngle === "number" ? night * (fajrAngle / 60) : null; | |
| if (fajrPortion !== null && (out.fajr === null || out.fajr < out.sunrise - fajrPortion)) | |
| out.fajr = out.sunrise - fajrPortion; |
| else if (rule === "angle" && typeof fajrAngle === "number") | ||
| out.fajr = out.sunrise - night * (fajrAngle / 60); | ||
| } | ||
| if (out.isha === null) { | ||
| if (rule === "middle") out.isha = out.maghrib + night / 2; | ||
| else if (rule === "seventh") out.isha = out.maghrib + night / 7; |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
A solvable Isha can still be later than the selected high-latitude limit. Cap angle-based Isha at that limit while preserving the fixed Umm al-Qura interval.
Suggested change
| else if (rule === "angle" && typeof fajrAngle === "number") | |
| out.fajr = out.sunrise - night * (fajrAngle / 60); | |
| } | |
| if (out.isha === null) { | |
| if (rule === "middle") out.isha = out.maghrib + night / 2; | |
| else if (rule === "seventh") out.isha = out.maghrib + night / 7; | |
| var ishaPortion = rule === "middle" ? night / 2 : | |
| rule === "seventh" ? night / 7 : | |
| rule === "angle" && typeof ishaAngle === "number" ? night * (ishaAngle / 60) : null; | |
| if (!ishaInterval && ishaPortion !== null && (out.isha === null || out.isha > out.maghrib + ishaPortion)) | |
| out.isha = out.maghrib + ishaPortion; |
| var parts = clean.split("/"); | ||
| var zone = clean; | ||
| if (parts.length >= 2) | ||
| zone = parts[parts.length - 2] + "/" + parts[parts.length - 1]; | ||
| detectedZone = zone; |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
Truncating every zone to two components turns America/Indiana/Indianapolis into Indiana/Indianapolis, breaking matching and mismatch detection for multi-component IANA zones. Strip the zoneinfo path prefix instead, preserving the full zone name.
Suggested change
| var parts = clean.split("/"); | |
| var zone = clean; | |
| if (parts.length >= 2) | |
| zone = parts[parts.length - 2] + "/" + parts[parts.length - 1]; | |
| detectedZone = zone; | |
| var clean = (tz || "").trim(); | |
| var marker = "/zoneinfo/"; | |
| var markerAt = clean.indexOf(marker); | |
| if (markerAt !== -1) | |
| clean = clean.substring(markerAt + marker.length); | |
| var zone = clean; |
| var unknown = JSON.parse(run(['--json', 'auto'], { TZ: 'Asia/Kathmandu' }).out); | ||
| ok(unknown.city === 'london', 'an unmapped zone falls back visibly, got ' + unknown.city); |
There was a problem hiding this comment.
🟠 major · Tests · fix: low effort
The unmapped-zone test expects London, but the CLI accepts guessed.city without checking guessed.matched, so it currently does return London; this also contradicts the documented promise that an unmapped timezone is rejected. The test should require a nonzero exit and the CLI should check matched before using the fallback.
| var diff = Math.abs(have - want); | ||
| if (diff > 720) diff = 1440 - diff; // midnight wrap | ||
| var tol = k === "asr" ? 2 : 1; | ||
| ok(diff <= tol, f.name + " " + k + " got " + fmt(have) + " want " + f.expect[k]); |
There was a problem hiding this comment.
💡 Suggestion (bug)
Fix wrapped fixture differences
Suggested change
| var diff = Math.abs(have - want); | |
| if (diff > 720) diff = 1440 - diff; // midnight wrap | |
| var tol = k === "asr" ? 2 : 1; | |
| ok(diff <= tol, f.name + " " + k + " got " + fmt(have) + " want " + f.expect[k]); | |
| var diff = Math.abs(have - want) % 1440; | |
| if (diff > 720) diff = 1440 - diff; // midnight wrap | |
| var tol = k === "asr" ? 2 : 1; | |
| ok(diff <= tol, f.name + " " + k + " got " + fmt(have) + " want " + f.expect[k]); |
To apply: click the sparkletip, or ask @mika-agent ask <question>.
| }); | ||
| }); | ||
|
|
||
| // ---------------------------------------------------------------- 2. sky checks |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
When the difference exceeds 1440 minutes, 1440 - diff becomes negative and the assertion passes even for arbitrarily wrong results. Normalize the difference before calculating the circular distance.
Suggested change
| }); | |
| }); | |
| // ---------------------------------------------------------------- 2. sky checks | |
| var diff = Math.abs(have - want) % 1440; | |
| if (diff > 720) diff = 1440 - diff; // midnight wrap | |
| var tol = k === "asr" ? 2 : 1; | |
| ok(diff <= tol, f.name + " " + k + " got " + fmt(have) + " want " + f.expect[k]); |
|
Thanks — the equation-of-time finding was real and serious, and the rest were worth taking. Point by point: 🔴
🟠 timezone ≠ city (major) — taken. 🟠 nested workflow (major) — removed from the submission. You're right that GitHub only reads the repo root, so it never ran here. It lives at the root of the author repo ( 🟡 Hijri terminal boundary — checked, not a fabrication. 🟡 🟡 CLI 🟡 🟡 i18n — blocked upstream, not by choice. Plugins may only import Also from an automated review pass on the same code: Current head |
- Umm al-Qura isha 120 min in Ramadan (90 otherwise), wired from the Hijri date - high-latitude rules cap a solvable Fajr/Isha outside the night portion - America/Indiana/Indianapolis no longer truncated to Indiana/Indianapolis - --city auto exits with the zone name instead of printing London - h23 hour cycle, validated dayIndex, Isha in the twilight check - fixture comparator wraps before mirroring - manifest: commands + dependencies.commands (node, readlink) - drop AGENTS.md/CLAUDE.md/.gitignore from the shipped product
|
Second pass taken; two of these were real bugs in code I wrote this session. 🟠 Umm al-Qura Isha in Ramadan (major) — real, fixed. The interval now lives in the method ( 🟡 High-latitude rules (2 findings) — real, taken. The rules previously only filled a missing angle. They now also pull back a solvable value that falls outside the night portion the rule permits (adhan-js does the same), with interval methods left alone so Umm al-Qura keeps its fixed distance. Tested at Stockholm midsummer under 🟡 🟡 🟡 🟢 🟢 fixture comparator — real. 🟢 Waybar/Node dependency — declared. Added 🟡 LICENSE not installed — that is the packer's policy, not the product's. 🟡 i18n — unchanged from last round: R4 forbids importing Head |
neur0map
left a comment
neur0map
left a comment
There was a problem hiding this comment.
🔴 Changes requested — 0 critical and 2 major issue(s): see the inline comments and the walkthrough for fixes and an agent-ready prompt.
| @@ -0,0 +1,21 @@ | |||
| MIT License | |||
|
|
|||
| Copyright (c) 2026 Sweet Pea | |||
There was a problem hiding this comment.
🟠 major · Data Integrity · fix: low effort
The embedded Hijri table is attributed to the MIT-licensed dralshehri/hijri-converter, but this license contains only Sweet Pea's copyright notice. Redistributing the table requires retaining the upstream copyright and permission notice; include the upstream license with the installed distribution.
| function calculate(day) { | ||
| const hijri = H.toHijri(day.getFullYear(), day.getMonth() + 1, day.getDate(), 0); | ||
| return PT.times({ | ||
| // Umm al-Qura holds Isha 120 minutes in Ramadan instead of 90 | ||
| ramadan: !!hijri && hijri.month === 9, |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
toHijri returns the month as m, as used by the JSON output below. Checking month always disables the Ramadan Isha interval in the CLI.
Suggested change
| function calculate(day) { | |
| const hijri = H.toHijri(day.getFullYear(), day.getMonth() + 1, day.getDate(), 0); | |
| return PT.times({ | |
| // Umm al-Qura holds Isha 120 minutes in Ramadan instead of 90 | |
| ramadan: !!hijri && hijri.month === 9, | |
| function calculate(day) { | |
| const hijri = H.toHijri(day.getFullYear(), day.getMonth() + 1, day.getDate(), 0); | |
| return PT.times({ | |
| // Umm al-Qura holds Isha 120 minutes in Ramadan instead of 90 | |
| ramadan: !!hijri && hijri.m === 9, |
| required property var modelData | ||
| readonly property bool rowNow: root.service !== null && root.service.isNow | ||
| && root.service.nextKey === modelData.key | ||
| && modelData.at === root.service.nextAt |
There was a problem hiding this comment.
🟡 minor · Possible bug · fix: low effort
rowNext matches only the time, so after the last prayer it can highlight today's Fajr as tomorrow's next Fajr when their times coincide. Require the row to be at or after the current time.
Suggested change
| required property var modelData | |
| readonly property bool rowNow: root.service !== null && root.service.isNow | |
| && root.service.nextKey === modelData.key | |
| && modelData.at === root.service.nextAt | |
| readonly property bool rowNext: root.service !== null && !rowNow | |
| && modelData.at !== null && modelData.at === root.service.nextAt | |
| && modelData.at >= root.service.now | |
| readonly property bool rowPast: root.service !== null && modelData.at !== null | |
| && modelData.at < root.service.now && !rowNow |
| readonly property bool rowNext: root.service !== null && !rowNow | ||
| && modelData.at !== null && modelData.at === root.service.nextAt | ||
| readonly property bool rowPast: root.service !== null && modelData.at !== null | ||
| && modelData.at < root.service.now && !rowNow |
There was a problem hiding this comment.
💡 Suggestion (possible bug)
Avoid highlighting past prayers as next
Suggested change
| readonly property bool rowNext: root.service !== null && !rowNow | |
| && modelData.at !== null && modelData.at === root.service.nextAt | |
| readonly property bool rowPast: root.service !== null && modelData.at !== null | |
| && modelData.at < root.service.now && !rowNow | |
| readonly property bool rowNext: root.service !== null && !rowNow | |
| && modelData.at !== null && modelData.at === root.service.nextAt | |
| && modelData.at >= root.service.now | |
| readonly property bool rowPast: root.service !== null && modelData.at !== null | |
| && modelData.at < root.service.now && !rowNow |
To apply: click the sparkletip, or ask @mika-agent ask <question>.
| elide: Text.ElideRight | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
The two children of this Row use different spacing deductions, so their combined width plus spacing exceeds the parent by 3 * root.s. Use the same deduction for both halves to keep the holiday control inside the panel.
Suggested change
| elide: Text.ElideRight | |
| } | |
| } | |
| Item { | |
| width: (parent.width - 14 * root.s) / 2 | |
| height: holidayText.implicitHeight |
| Item { | ||
| width: (parent.width - 8 * root.s) / 2 | ||
| height: holidayText.implicitHeight |
There was a problem hiding this comment.
💡 Suggestion (bug)
Keep the footer within its row
Suggested change
| Item { | |
| width: (parent.width - 8 * root.s) / 2 | |
| height: holidayText.implicitHeight | |
| Item { | |
| width: (parent.width - 14 * root.s) / 2 | |
| height: holidayText.implicitHeight |
To apply: click the sparkletip, or ask @mika-agent ask <question>.
| function timesFor(date, lat, lon, method, asr, highLat, offsets) { | ||
| var hijri = H.toHijri(date.getFullYear(), date.getMonth() + 1, date.getDate(), hijriAdjust); | ||
| return PT.times({ | ||
| // Umm al-Qura holds Isha 120 minutes in Ramadan instead of 90 | ||
| ramadan: !!hijri && hijri.month === 9, |
There was a problem hiding this comment.
🟠 major · Possible Bug · fix: low effort
H.toHijri() returns the month as m, not month, so this condition is always false. Umm al-Qura Isha remains 90 minutes after Maghrib throughout Ramadan instead of 120; the CLI has the same error. Use hijri.m in both callers and add end-to-end Ramadan tests.
…ow guard - `hijri.month` never existed -> the 120-minute Ramadan interval was dead on both call sites; CLI test covers it - carry the hijri-converter copyright/permission notice in lib/hijri.js, the file that actually installs - Panel rowNext also requires at >= now, so a coincident minute tomorrow cannot light up a past row
neur0map
left a comment
neur0map
left a comment
There was a problem hiding this comment.
🔍 mika reviewed this change; details in the walkthrough comment.
| fail('--method requires an id'); | ||
| methodOverride = process.argv[i]; | ||
| if (!PT.METHODS[methodOverride]) | ||
| fail(`unknown method: ${methodOverride}`); | ||
| } else if (arg === '--high-lat') { | ||
| if (++i >= process.argv.length || process.argv[i].startsWith('-')) |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
Inherited names such as constructor pass this check even though they are not calculation methods. Check for an own property so invalid CLI input cannot reach the engine as a method.
Suggested change
| fail('--method requires an id'); | |
| methodOverride = process.argv[i]; | |
| if (!PT.METHODS[methodOverride]) | |
| fail(`unknown method: ${methodOverride}`); | |
| } else if (arg === '--high-lat') { | |
| if (++i >= process.argv.length || process.argv[i].startsWith('-')) | |
| } else if (arg === '--method') { | |
| if (++i >= process.argv.length || process.argv[i].startsWith('-')) | |
| fail('--method requires an id'); | |
| methodOverride = process.argv[i]; | |
| if (!Object.prototype.hasOwnProperty.call(PT.METHODS, methodOverride)) | |
| fail(`unknown method: ${methodOverride}`); |
| if (!next) { | ||
| const tomorrow = new Date(date.getFullYear(), date.getMonth(), date.getDate() + 1, 12); | ||
| const tomorrowTimes = calculate(tomorrow); | ||
| for (const key of keys) { | ||
| if (tomorrowTimes && tomorrowTimes[key] !== null) { | ||
| next = { key, at: tomorrowTimes[key] }; |
There was a problem hiding this comment.
🟡 minor · Possible Bug · fix: medium effort
Waybar switches to tomorrow's event immediately after the final prayer, but its tooltip still shows today's date and today's times. After Isha, the highlighted text and tooltip therefore describe different schedules; include the next event's date or show tomorrow's schedule when selecting it.
| var fajr = tomorrowFajr; | ||
| if (typeof fajr !== "number" || !isFinite(fajr)) fajr = today.fajr; | ||
| if (typeof fajr !== "number" || !isFinite(fajr)) return null; | ||
| var night = (fajr <= today.maghrib ? fajr + 1440 : fajr) - today.maghrib; | ||
| return { | ||
| midnight: today.maghrib + Math.round(night / 2), |
There was a problem hiding this comment.
🟡 minor · Bug · fix: low effort
tomorrowFajr is relative to tomorrow's midnight even when it is greater than today's Maghrib, so the conditional can produce a night that is a day too short. Always add 1440 minutes before subtracting today's Maghrib.
Suggested change
| var fajr = tomorrowFajr; | |
| if (typeof fajr !== "number" || !isFinite(fajr)) fajr = today.fajr; | |
| if (typeof fajr !== "number" || !isFinite(fajr)) return null; | |
| var night = (fajr <= today.maghrib ? fajr + 1440 : fajr) - today.maghrib; | |
| return { | |
| midnight: today.maghrib + Math.round(night / 2), | |
| function nightInfo(today, tomorrowFajr) { | |
| if (!today || typeof today.maghrib !== "number" || !isFinite(today.maghrib)) return null; | |
| var fajr = tomorrowFajr; | |
| if (typeof fajr !== "number" || !isFinite(fajr)) fajr = today.fajr; | |
| if (typeof fajr !== "number" || !isFinite(fajr)) return null; | |
| var night = fajr + 1440 - today.maghrib; |
|
Good catch on the wiring — that one was mine, and my own test missed it because it called 🟡 🟠 Upstream MIT notice — taken. The 🟡 Head |
|
The newest pass re-anchors four findings that were already fixed in
The one genuinely open item is i18n, and it is upstream: R4 forbids importing |
What is this?
Prayer Times, a bar widget: Prayer times in the QS Bar: next prayer with a live countdown, a full-day panel with Umm al-Qura Hijri dates, Islamic holidays, a month t...
Checklist
registry.json(lastUpdatedinYYYY-MM-DD;official: falsefor community items).ryoku plugin add).tests/validate-catalogue.shpasses from the repo root.Opened with
ryoku plugin share; the product manifest and the registry entry were generated from the installed plugin.