feat: add prayer times and Hijri calendar endpoints with offline testing - #52
Conversation
WalkthroughAdds deterministic prayer-time and calendar routes, mock upstream behavior for chat and Stellar, and a Locust-based performance budget runner with supporting tests, documentation, dependencies, and CI coverage. ChangesWorship API
Mock upstream performance testing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant worship_router
participant zoneinfo
participant calculate_prayer_times
Client->>FastAPI: GET /prayer-times
FastAPI->>worship_router: Route request
worship_router->>zoneinfo: Validate IANA timezone
worship_router->>calculate_prayer_times: Calculate prayer times
calculate_prayer_times-->>worship_router: Times and fallback status
worship_router-->>Client: PrayerTimesResponse
sequenceDiagram
participant run_perf_tests
participant Uvicorn
participant Locust
participant FastAPI
run_perf_tests->>Uvicorn: Start server with MOCK_UPSTREAMS=1
run_perf_tests->>Uvicorn: Poll /ping
run_perf_tests->>Locust: Run headless load test
Locust->>FastAPI: Send chat, zakat, and ping requests
Locust-->>run_perf_tests: Write CSV statistics
run_perf_tests->>run_perf_tests: Compare metrics with budget.yaml
run_perf_tests->>Uvicorn: Terminate server
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (3)
.github/workflows/ci.yml (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the new test module in CI checks.
The new test suite runs, but
flake8andpy_compileonly inspectmain.pyandworship.py. Static regressions intests/test_worship.pycan therefore pass CI unnoticed.As per path instructions, CI enforces flake8 for this FastAPI service.
Suggested CI update
- run: flake8 main.py worship.py --max-line-length=120 --ignore=E501,W503 + run: flake8 main.py worship.py tests/test_worship.py --max-line-length=120 --ignore=E501,W503 - run: python -m py_compile main.py worship.py + run: python -m py_compile main.py worship.py tests/test_worship.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 31 - 37, Update the CI steps in the workflow to include tests/test_worship.py in both the flake8 command and the python -m py_compile command, while preserving the existing checks for main.py and worship.py and the pytest tests/ invocation.Source: Path instructions
tests/test_worship.py (1)
9-38: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the prayer-time test oracles.
The PR objective calls for offline reference-value coverage, but these tests only check invariants or non-null fields. A global time shift could still pass. The Makkah assertion also truncates seconds, so a non-exact 90-minute result may pass.
Add fixed, timezone-aware expected values for representative dates/methods, and compare full ISO datetimes for the 90-minute rule.
Suggested assertion tightening
-from datetime import date +from datetime import datetime, timedelta - maghrib_time = maghrib_str.split("T")[1][:5] - isha_time = isha_str.split("T")[1][:5] + maghrib_dt = datetime.fromisoformat(maghrib_str) + isha_dt = datetime.fromisoformat(isha_str) - assert isha_mins - maghrib_mins == 90 + assert isha_dt - maghrib_dt == timedelta(minutes=90)Also applies to: 40-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_worship.py` around lines 9 - 38, Strengthen the prayer-time tests around test_prayer_times_makkah and the related cases by adding fixed, timezone-aware reference ISO datetime values for representative dates and calculation methods, then assert the complete returned values rather than only status, method, or non-null fields. Replace the Makkah minute-truncation arithmetic with a full ISO-datetime comparison that verifies Isha is exactly 90 minutes after Maghrib, including seconds and timezone information.worship.py (1)
166-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate computation of
t_sunrise/t_sunset.Both are computed with identical arguments (
compute_time(0.833, lat, declination)), so they're always the same value. Not a bug (sunrise/sunset hour-angle magnitude is symmetric around solar noon), but computing it twice is wasteful and the two names obscure that fact for future readers.♻️ Suggested consolidation
- t_fajr = compute_time(fajr_angle, lat, declination) - t_sunrise = compute_time(0.833, lat, declination) # 0.833 accounts for refraction and sun radius - t_sunset = compute_time(0.833, lat, declination) - t_asr = compute_asr(lat, declination, asr_factor) + t_fajr = compute_time(fajr_angle, lat, declination) + # 0.833 accounts for refraction and sun radius; sunrise/sunset share the same hour angle magnitude + t_horizon = compute_time(0.833, lat, declination) + t_sunrise = t_sunset = t_horizon + t_asr = compute_asr(lat, declination, asr_factor)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@worship.py` around lines 166 - 169, Consolidate the duplicate compute_time call in the prayer-time calculation by computing the shared 0.833-degree value once, then reuse it for both t_sunrise and t_sunset. Keep both named outputs available for downstream logic while avoiding repeated computation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 46: Update the README About section to remove the planned
zakat-calculation wording, since POST /zakat is already shipped, and retain only
the still-planned on-chain purchase lookup description.
In `@requirements.txt`:
- Around line 8-9: Update the hijridate and tzdata entries in requirements.txt
to exact, validated versions rather than a minimum or unversioned constraint.
Preserve both dependencies while ensuring builds consistently use the reviewed
runtime versions.
In `@worship.py`:
- Around line 316-335: Update get_gregorian so its generic exception handler
does not catch broadly or expose arbitrary exception text; preserve the explicit
ValueError/OverflowError validation handling and chain any intentionally
translated HTTPException from the original exception using the established
pattern from the /hijri handler.
- Around line 185-198: The high-latitude fallback in the time-calculation flow
can leave required times as None without setting fallback_applied or explaining
the condition. Update the logic after the fallback attempt, using the existing
fallback_applied state and response-notes construction, to add a distinct note
whenever any relevant prayer or solar times remain unavailable because sunrise
or maghrib could not be computed.
- Around line 172-179: Update the times_hours construction to distinguish None
from valid numeric zero values for t_fajr, t_sunrise, t_asr, t_sunset, and
t_isha. Replace truthiness checks with explicit None checks so 0.0 is used in
the corresponding calculations while only non-computable results remain None.
- Around line 296-313: Update get_hijri to handle only documented
date-conversion or validation failures as client-facing 400 responses; remove
the broad Exception handler so unexpected programming errors propagate normally.
Preserve the OverflowError response but chain both HTTPException raises from
their caught exceptions, and avoid exposing raw internal exception text in any
client detail unless it is explicitly safe.
- Around line 258-271: Cache the result of available_timezones() once at module
scope, then update get_prayer_times to validate tz against that cached timezone
set instead of rescanning the filesystem on each request. Keep the existing
invalid-timezone HTTPException behavior and ZoneInfo construction unchanged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 31-37: Update the CI steps in the workflow to include
tests/test_worship.py in both the flake8 command and the python -m py_compile
command, while preserving the existing checks for main.py and worship.py and the
pytest tests/ invocation.
In `@tests/test_worship.py`:
- Around line 9-38: Strengthen the prayer-time tests around
test_prayer_times_makkah and the related cases by adding fixed, timezone-aware
reference ISO datetime values for representative dates and calculation methods,
then assert the complete returned values rather than only status, method, or
non-null fields. Replace the Makkah minute-truncation arithmetic with a full
ISO-datetime comparison that verifies Isha is exactly 90 minutes after Maghrib,
including seconds and timezone information.
In `@worship.py`:
- Around line 166-169: Consolidate the duplicate compute_time call in the
prayer-time calculation by computing the shared 0.833-degree value once, then
reuse it for both t_sunrise and t_sunset. Keep both named outputs available for
downstream logic while avoiding repeated computation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a948821-5992-4f19-bd5f-2436bc6d2304
📒 Files selected for processing (7)
.github/workflows/ci.yml.gitignoreREADME.mdmain.pyrequirements.txttests/test_worship.pyworship.py
| | `POST` | `/chat` | Start or continue a chat session | | ||
| | `DELETE` | `/chat/{chat_id}` | Delete a chat session | | ||
| | `GET` | `/ping` | Health check | | ||
| | `POST` | `/zakat` | Estimate zakat on an account's USDC balance | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== README excerpt ==\n'
git ls-files README.md && nl -ba README.md | sed -n '35,60p'
printf '\n== Search for zakat mentions ==\n'
rg -n -i "zakat|wallet-based zakat|roadmap" .Repository: Deen-Bridge/dnb-ai
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Route/handler search ==\n'
rg -n -i "POST\s*/zakat|/zakat|zakat" --glob '!README.md' .Repository: Deen-Bridge/dnb-ai
Length of output: 1795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== README lines 1-80 ==\n'
sed -n '1,80p' README.md
printf '\n== README zakat mentions ==\n'
rg -n -i "zakat|roadmap" README.mdRepository: Deen-Bridge/dnb-ai
Length of output: 3783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,80p' README.md
rg -n -i "zakat|roadmap" README.mdRepository: Deen-Bridge/dnb-ai
Length of output: 3729
Align the roadmap note with the shipped /zakat endpoint
The API already exposes POST /zakat, so the About section should drop the zakat-calculation part and keep only the still-planned on-chain purchase lookup. That keeps the README consistent for consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 46, Update the README About section to remove the planned
zakat-calculation wording, since POST /zakat is already shipped, and retain only
the still-planned on-chain purchase lookup description.
| hijridate>=2.6.0 | ||
| tzdata |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the new runtime dependencies.
hijridate>=2.6.0 and unversioned tzdata allow unreviewed upgrades and can change deterministic Hijri or timezone results between builds. Pin exact validated versions.
As per path instructions, “new dependencies not pinned in requirements.txt” must be flagged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@requirements.txt` around lines 8 - 9, Update the hijridate and tzdata entries
in requirements.txt to exact, validated versions rather than a minimum or
unversioned constraint. Preserve both dependencies while ensuring builds
consistently use the reviewed runtime versions.
Source: Path instructions
| times_hours = { | ||
| "fajr": dhuhr_time - t_fajr if t_fajr else None, | ||
| "sunrise": dhuhr_time - t_sunrise if t_sunrise else None, | ||
| "dhuhr": dhuhr_time, | ||
| "asr": dhuhr_time + t_asr if t_asr else None, | ||
| "maghrib": dhuhr_time + t_sunset if t_sunset else None, | ||
| "isha": dhuhr_time + t_isha if t_isha else None, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Falsy-zero bug: if t_fajr else None mishandles a legitimate 0.0 hour angle.
compute_time/compute_asr return None for "not computable" but can legitimately return 0.0 (e.g. darccos(1.0) == 0). Since 0.0 is falsy in Python, a valid zero-hour-angle result is silently discarded here and treated the same as "not computable," which can incorrectly trigger the high-latitude fallback path below for a value that was actually calculable.
🐛 Proposed fix
times_hours = {
- "fajr": dhuhr_time - t_fajr if t_fajr else None,
- "sunrise": dhuhr_time - t_sunrise if t_sunrise else None,
+ "fajr": dhuhr_time - t_fajr if t_fajr is not None else None,
+ "sunrise": dhuhr_time - t_sunrise if t_sunrise is not None else None,
"dhuhr": dhuhr_time,
- "asr": dhuhr_time + t_asr if t_asr else None,
- "maghrib": dhuhr_time + t_sunset if t_sunset else None,
- "isha": dhuhr_time + t_isha if t_isha else None,
+ "asr": dhuhr_time + t_asr if t_asr is not None else None,
+ "maghrib": dhuhr_time + t_sunset if t_sunset is not None else None,
+ "isha": dhuhr_time + t_isha if t_isha is not None else None,
}📝 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.
| times_hours = { | |
| "fajr": dhuhr_time - t_fajr if t_fajr else None, | |
| "sunrise": dhuhr_time - t_sunrise if t_sunrise else None, | |
| "dhuhr": dhuhr_time, | |
| "asr": dhuhr_time + t_asr if t_asr else None, | |
| "maghrib": dhuhr_time + t_sunset if t_sunset else None, | |
| "isha": dhuhr_time + t_isha if t_isha else None, | |
| } | |
| times_hours = { | |
| "fajr": dhuhr_time - t_fajr if t_fajr is not None else None, | |
| "sunrise": dhuhr_time - t_sunrise if t_sunrise is not None else None, | |
| "dhuhr": dhuhr_time, | |
| "asr": dhuhr_time + t_asr if t_asr is not None else None, | |
| "maghrib": dhuhr_time + t_sunset if t_sunset is not None else None, | |
| "isha": dhuhr_time + t_isha if t_isha is not None else None, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worship.py` around lines 172 - 179, Update the times_hours construction to
distinguish None from valid numeric zero values for t_fajr, t_sunrise, t_asr,
t_sunset, and t_isha. Replace truthiness checks with explicit None checks so 0.0
is used in the corresponding calculations while only non-computable results
remain None.
| # High-latitude fallback (Angle-based method: e.g. 1/7th of night) | ||
| fallback_applied = False | ||
| if (times_hours["fajr"] is None or times_hours["isha"] is None or | ||
| times_hours["sunrise"] is None or times_hours["maghrib"] is None): | ||
| if times_hours["sunrise"] is not None and times_hours["maghrib"] is not None: | ||
| night_duration = 24.0 - (times_hours["maghrib"] - times_hours["sunrise"]) | ||
| # Fallback: Fajr is 1/7th of night before sunrise, Isha is 1/7th of night after sunset | ||
| if times_hours["fajr"] is None: | ||
| times_hours["fajr"] = times_hours["sunrise"] - (night_duration / 7.0) | ||
| fallback_applied = True | ||
| if times_hours["isha"] is None: | ||
| times_hours["isha"] = times_hours["maghrib"] + (night_duration / 7.0) | ||
| fallback_applied = True | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
High-latitude fallback silently no-ops when sunrise/maghrib are also uncomputable.
The 1/7-night fallback only fires when sunrise and maghrib are both non-None (line 189). At truly extreme latitudes (polar day/night, where even the 0.833° sunrise/sunset angle isn't reached), fajr/isha — and potentially sunrise/maghrib too — remain None with fallback_applied staying False, so the response silently returns nulls with no notes explaining why, even though the endpoint's message (line 277) implies such cases ("...or times not calculable") would be surfaced.
Consider setting a distinct note when times remain unavailable after the fallback attempt, so API consumers aren't left guessing why fields are null.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worship.py` around lines 185 - 198, The high-latitude fallback in the
time-calculation flow can leave required times as None without setting
fallback_applied or explaining the condition. Update the logic after the
fallback attempt, using the existing fallback_applied state and response-notes
construction, to add a distinct note whenever any relevant prayer or solar times
remain unavailable because sunrise or maghrib could not be computed.
| @router.get("/hijri", response_model=HijriResponse) | ||
| async def get_hijri(date: date = Query(..., description="Gregorian date (YYYY-MM-DD)")): | ||
| """Convert a Gregorian date to Hijri.""" | ||
| try: | ||
| g = Gregorian(date.year, date.month, date.day) | ||
| h = g.to_hijri() | ||
| return HijriResponse( | ||
| year=h.year, | ||
| month=h.month, | ||
| month_en=h.month_name(), | ||
| month_ar=h.month_name(language='ar'), | ||
| day=h.day, | ||
| gregorian_date=date | ||
| ) | ||
| except OverflowError as e: | ||
| raise HTTPException(status_code=400, detail=f"Date out of supported range: {str(e)}") | ||
| except Exception as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Blind except Exception leaks internal error text to API clients, and both raise statements drop exception chaining.
except Exception as e: raise HTTPException(..., detail=str(e)) catches any programming error (not just the documented OverflowError/hijridate validation errors) and echoes its raw message straight to the client as a 400, which can expose internal implementation details and mask genuine bugs as "bad request" responses. Both raise HTTPException calls also drop the original exception context (raise ... from e), which loses the traceback in logs/observability tooling.
🛡️ Suggested fix
try:
g = Gregorian(date.year, date.month, date.day)
h = g.to_hijri()
return HijriResponse(...)
except OverflowError as e:
- raise HTTPException(status_code=400, detail=f"Date out of supported range: {str(e)}")
- except Exception as e:
- raise HTTPException(status_code=400, detail=str(e))
+ raise HTTPException(status_code=400, detail=f"Date out of supported range: {e}") from eAs per path instructions, "bare excepts that leak stack traces to clients" should be flagged.
📝 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.
| @router.get("/hijri", response_model=HijriResponse) | |
| async def get_hijri(date: date = Query(..., description="Gregorian date (YYYY-MM-DD)")): | |
| """Convert a Gregorian date to Hijri.""" | |
| try: | |
| g = Gregorian(date.year, date.month, date.day) | |
| h = g.to_hijri() | |
| return HijriResponse( | |
| year=h.year, | |
| month=h.month, | |
| month_en=h.month_name(), | |
| month_ar=h.month_name(language='ar'), | |
| day=h.day, | |
| gregorian_date=date | |
| ) | |
| except OverflowError as e: | |
| raise HTTPException(status_code=400, detail=f"Date out of supported range: {str(e)}") | |
| except Exception as e: | |
| raise HTTPException(status_code=400, detail=str(e)) | |
| `@router.get`("/hijri", response_model=HijriResponse) | |
| async def get_hijri(date: date = Query(..., description="Gregorian date (YYYY-MM-DD)")): | |
| """Convert a Gregorian date to Hijri.""" | |
| try: | |
| g = Gregorian(date.year, date.month, date.day) | |
| h = g.to_hijri() | |
| return HijriResponse( | |
| year=h.year, | |
| month=h.month, | |
| month_en=h.month_name(), | |
| month_ar=h.month_name(language='ar'), | |
| day=h.day, | |
| gregorian_date=date | |
| ) | |
| except OverflowError as e: | |
| raise HTTPException(status_code=400, detail=f"Date out of supported range: {e}") from e |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 297-297: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 311-311: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 311-311: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 312-312: Do not catch blind exception: Exception
(BLE001)
[warning] 313-313: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worship.py` around lines 296 - 313, Update get_hijri to handle only
documented date-conversion or validation failures as client-facing 400
responses; remove the broad Exception handler so unexpected programming errors
propagate normally. Preserve the OverflowError response but chain both
HTTPException raises from their caught exceptions, and avoid exposing raw
internal exception text in any client detail unless it is explicitly safe.
Source: Path instructions
| @router.get("/gregorian", response_model=GregorianResponse) | ||
| async def get_gregorian(hijri: str = Query(..., description="Hijri date (YYYY-MM-DD)")): | ||
| """Convert a Hijri date to Gregorian.""" | ||
| try: | ||
| parts = hijri.split('-') | ||
| if len(parts) != 3: | ||
| raise ValueError("Invalid format. Use YYYY-MM-DD") | ||
| y, m, d = int(parts[0]), int(parts[1]), int(parts[2]) | ||
| h = Hijri(y, m, d) | ||
| g = h.to_gregorian() | ||
| return GregorianResponse( | ||
| year=g.year, | ||
| month=g.month, | ||
| day=g.day, | ||
| hijri_date=hijri | ||
| ) | ||
| except (ValueError, OverflowError) as e: | ||
| raise HTTPException(status_code=400, detail=f"Invalid Hijri date: {str(e)}") | ||
| except Exception as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Same blind-except / missing exception-chaining pattern as /hijri.
Shares the root cause flagged on lines 296-313: except Exception as e: raise HTTPException(..., detail=str(e)) catches too broadly and drops from e chaining.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 333-333: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 333-333: Use explicit conversion flag
Replace with conversion flag
(RUF010)
[warning] 334-334: Do not catch blind exception: Exception
(BLE001)
[warning] 335-335: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worship.py` around lines 316 - 335, Update get_gregorian so its generic
exception handler does not catch broadly or expose arbitrary exception text;
preserve the explicit ValueError/OverflowError validation handling and chain any
intentionally translated HTTPException from the original exception using the
established pattern from the /hijri handler.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@loadtest/run_perf_tests.py`:
- Around line 80-90: Update the server_process Popen setup to avoid unread
stdout and stderr pipes by redirecting both streams to an appropriate sink or
open log file. In the wait_for_server failure path, terminate server_process and
then wait for it to exit before calling sys.exit, ensuring cleanup completes
without leaving a zombie process.
In `@main.py`:
- Line 148: Remove all spaces and tabs from the blank lines identified in
main.py, including the lines near 148, 153, 158, 162, 164, and 176, so they are
completely empty and satisfy flake8 W293.
- Around line 154-157: Replace the blocking time.sleep call in the
mock_upstreams latency branch with await asyncio.sleep using the same latency
conversion, and ensure asyncio is available in this async handler. Preserve the
existing MOCK_LLM_LATENCY_MS parsing and conditional behavior.
In `@requirements.txt`:
- Around line 10-11: Update the locust and pyyaml dependency declarations to use
exact-version pins instead of minimum-version constraints, preserving the
currently specified versions 2.29.0 and 6.0.1.
In `@stellar.py`:
- Around line 89-102: Update calculate_zakat and fetch_usdc_balance so the
blocking mock delay and Horizon SDK lookup run via FastAPI’s run_in_threadpool
instead of executing directly on the async event loop; preserve the existing
return values and HTTPException behavior for mock and real paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e232c789-f73c-4cb0-bbc8-5103cd2ba799
⛔ Files ignored due to path filters (4)
loadtest/report_exceptions.csvis excluded by!**/*.csvloadtest/report_failures.csvis excluded by!**/*.csvloadtest/report_stats.csvis excluded by!**/*.csvloadtest/report_stats_history.csvis excluded by!**/*.csv
📒 Files selected for processing (6)
loadtest/budget.yamlloadtest/locustfile.pyloadtest/run_perf_tests.pymain.pyrequirements.txtstellar.py
| server_process = subprocess.Popen( | ||
| [sys.executable, "-m", "uvicorn", "main:app", "--port", "8000"], | ||
| env=env, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE | ||
| ) | ||
|
|
||
| print("Starting FastAPI server in mock mode...") | ||
| if not wait_for_server("http://localhost:8000/ping"): | ||
| server_process.terminate() | ||
| sys.exit(1) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Piped subprocess output is never drained — deadlock risk on longer/busier runs.
stdout=subprocess.PIPE, stderr=subprocess.PIPE is set but nothing ever reads from these pipes while the server runs for the whole Locust duration. If Uvicorn's access-log output exceeds the OS pipe buffer (a few dozen KB), the child process blocks on write and the run hangs until the pipe is drained or the job times out — likely worse with more users/longer --run-time.
Also note: on the wait_for_server timeout path (line 88-90) server_process.terminate() is called without a following server_process.wait(), which can leave a zombie/orphaned process depending on the CI environment.
🔧 Proposed fix: redirect output instead of piping unread
server_process = subprocess.Popen(
[sys.executable, "-m", "uvicorn", "main:app", "--port", "8000"],
env=env,
- stdout=subprocess.PIPE,
- stderr=subprocess.PIPE
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL
)
print("Starting FastAPI server in mock mode...")
if not wait_for_server("http://localhost:8000/ping"):
server_process.terminate()
+ server_process.wait()
sys.exit(1)(If the server's logs are needed for debugging, redirect to an open log file instead of PIPE, e.g. stdout=open("loadtest/server.log", "w").)
📝 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.
| server_process = subprocess.Popen( | |
| [sys.executable, "-m", "uvicorn", "main:app", "--port", "8000"], | |
| env=env, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE | |
| ) | |
| print("Starting FastAPI server in mock mode...") | |
| if not wait_for_server("http://localhost:8000/ping"): | |
| server_process.terminate() | |
| sys.exit(1) | |
| server_process = subprocess.Popen( | |
| [sys.executable, "-m", "uvicorn", "main:app", "--port", "8000"], | |
| env=env, | |
| stdout=subprocess.DEVNULL, | |
| stderr=subprocess.DEVNULL | |
| ) | |
| print("Starting FastAPI server in mock mode...") | |
| if not wait_for_server("http://localhost:8000/ping"): | |
| server_process.terminate() | |
| server_process.wait() | |
| sys.exit(1) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@loadtest/run_perf_tests.py` around lines 80 - 90, Update the server_process
Popen setup to avoid unread stdout and stderr pipes by redirecting both streams
to an appropriate sink or open log file. In the wait_for_server failure path,
terminate server_process and then wait for it to exit before calling sys.exit,
ensuring cleanup completes without leaving a zombie process.
| if mock_upstreams: | ||
| import time | ||
| latency_ms = float(os.getenv("MOCK_LLM_LATENCY_MS", "800")) | ||
| time.sleep(latency_ms / 1000.0) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the relevant section.
git ls-files | rg '^main\.py$|/main\.py$|^.*main\.py$' || true
wc -l main.py
cat -n main.py | sed -n '130,180p'Repository: Deen-Bridge/dnb-ai
Length of output: 2595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l main.py
sed -n '140,170p' main.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 1644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('main.py')
print('exists', p.exists())
if p.exists():
lines = p.read_text().splitlines()
for i in range(145, 161):
if i <= len(lines):
print(f"{i}: {lines[i-1]}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 911
Avoid blocking the event loop here
time.sleep() blocks this async handler and delays concurrent requests. Use await asyncio.sleep(latency_ms / 1000.0) instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 154 - 157, Replace the blocking time.sleep call in the
mock_upstreams latency branch with await asyncio.sleep using the same latency
conversion, and ensure asyncio is available in this async handler. Preserve the
existing MOCK_LLM_LATENCY_MS parsing and conditional behavior.
Source: Path instructions
| locust>=2.29.0 | ||
| pyyaml>=6.0.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the dependency file and any repo guidance that may mention pinning/version policy.
git ls-files | rg '(^|/)(requirements\.txt|README\.md|pyproject\.toml|setup\.cfg|tox\.ini|\.github/|docs/)'
printf '\n--- requirements.txt ---\n'
cat -n requirements.txt
printf '\n--- pinning/version policy search ---\n'
rg -n --hidden --no-ignore-vcs -S \
'pin|pinned|exact version|requirements\.txt|locust|pyyaml|hijridate|tzdata|>=|==' \
README.md pyproject.toml setup.cfg tox.ini .github docs requirements.txt 2>/dev/null || trueRepository: Deen-Bridge/dnb-ai
Length of output: 1199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect the dependency file history context without using git log.
sed -n '1,80p' requirements.txtRepository: Deen-Bridge/dnb-ai
Length of output: 342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether locust and pyyaml are already present elsewhere with exact pins.
rg -n --hidden --no-ignore-vcs -S \
'(^|[^A-Za-z])(locust|pyyaml)([^A-Za-z]|$)' \
. 2>/dev/null || trueRepository: Deen-Bridge/dnb-ai
Length of output: 1155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the local file for line numbers and surrounding entries.
nl -ba requirements.txt | sed -n '1,40p'Repository: Deen-Bridge/dnb-ai
Length of output: 196
Pin these dependencies exactly. locust>=2.29.0 and pyyaml>=6.0.1 still allow unreviewed upgrades across installs; == pins keep CI and local environments reproducible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@requirements.txt` around lines 10 - 11, Update the locust and pyyaml
dependency declarations to use exact-version pins instead of minimum-version
constraints, preserving the currently specified versions 2.29.0 and 6.0.1.
Source: Path instructions
| if os.getenv("MOCK_UPSTREAMS", "0") == "1": | ||
| import time | ||
| latency_ms = float(os.getenv("MOCK_HORIZON_LATENCY_MS", "200")) | ||
| time.sleep(latency_ms / 1000.0) | ||
| if public_key.endswith("A"): | ||
| return Decimal("100.50") | ||
| elif public_key.endswith("B"): | ||
| raise HTTPException( | ||
| status_code=404, | ||
| detail=f"Account not found on the Stellar {STELLAR_NETWORK} network. (MOCK)", | ||
| ) | ||
| else: | ||
| return None | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Blocking time.sleep() inside a sync function called from an async def endpoint.
fetch_usdc_balance is a plain def invoked directly (unawaited) from async def calculate_zakat. Since the endpoint itself is async def, FastAPI does not offload it to a threadpool — the time.sleep(latency_ms / 1000.0) call freezes the whole event loop for up to MOCK_HORIZON_LATENCY_MS (default 200ms) on every /zakat request, serializing all concurrent traffic (including /ping, /chat) for that window. This directly skews the RPS/p95 numbers the load-test cohort is meant to measure under concurrency.
🔧 Proposed fix: offload the blocking sleep/lookup to a thread
- if os.getenv("MOCK_UPSTREAMS", "0") == "1":
- import time
- latency_ms = float(os.getenv("MOCK_HORIZON_LATENCY_MS", "200"))
- time.sleep(latency_ms / 1000.0)
- if public_key.endswith("A"):
+ if os.getenv("MOCK_UPSTREAMS", "0") == "1":
+ latency_ms = float(os.getenv("MOCK_HORIZON_LATENCY_MS", "200"))
+ time.sleep(latency_ms / 1000.0) # still blocking, but now run off the event loop below
+ if public_key.endswith("A"):- balance = fetch_usdc_balance(public_key)
+ balance = await run_in_threadpool(fetch_usdc_balance, public_key)(from fastapi.concurrency import run_in_threadpool — this also fixes the pre-existing blocking Horizon SDK call on the non-mock path.)
As per path instructions, **/*.py files must "Flag blocking calls inside async endpoints (network calls should be awaited or offloaded)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stellar.py` around lines 89 - 102, Update calculate_zakat and
fetch_usdc_balance so the blocking mock delay and Horizon SDK lookup run via
FastAPI’s run_in_threadpool instead of executing directly on the async event
loop; preserve the existing return values and HTTPException behavior for mock
and real paths.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pr_description.md`:
- Around line 25-26: The public route documentation is inconsistent with the
endpoint contract and router mounting. In pr_description.md lines 25-26, replace
the listed paths with GET /prayer-times, GET /hijri, and GET /gregorian; in
pr_description.md line 31, document the mounted paths accurately and retain the
/worship prefix only if main.py applies it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25266ff9-800e-4414-9b73-4a9e55ec4405
📒 Files selected for processing (2)
main.pypr_description.md
🚧 Files skipped from review as they are similar to previous changes (1)
- main.py
| - Provides fast, deterministic prayer‑time calculation (`/prayer_times`) for any latitude/longitude and date. | ||
| - Exposes Hijri ↔ Gregorian conversion (`/hijri_to_gregorian`, `/gregorian_to_hijri`) using the `hijridate` library. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the public route contract consistently.
The description lists paths that conflict with the supplied endpoint contract: GET /prayer-times, GET /hijri, and GET /gregorian. The /worship prefix must also match the actual main.py router mounting; otherwise clients may follow unusable URLs.
pr_description.md#L25-L26: replace/prayer_times,/hijri_to_gregorian, and/gregorian_to_hijriwith the actual paths.pr_description.md#L31: document the mounted paths accurately and retain/worshiponly ifmain.pyapplies that prefix.
📍 Affects 1 file
pr_description.md#L25-L26(this comment)pr_description.md#L31-L31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr_description.md` around lines 25 - 26, The public route documentation is
inconsistent with the endpoint contract and router mounting. In
pr_description.md lines 25-26, replace the listed paths with GET /prayer-times,
GET /hijri, and GET /gregorian; in pr_description.md line 31, document the
mounted paths accurately and retain the /worship prefix only if main.py applies
it.
|
@fredericklamar342-prog please point PR to dev branch |
|
please point all your PR to dev branch |
|
@fredericklamar342-prog please fix Ci and point PR to dev branch |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
150-180: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winConstrain
ChatRequestinput before forwarding or saving it.promptandcontextare unconstrained strings here, so blank requests and very large payloads can flow intosend_message()and persisted history. Add Pydantic length limits and reject empty prompts to avoid unbounded token usage and storage growth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 150 - 180, Update the ChatRequest Pydantic model to reject blank prompts and enforce explicit maximum lengths for both prompt and context before the flow constructs full_prompt, forwards input, or persists history. Preserve optional context semantics while ensuring oversized or whitespace-only input is rejected during request validation.Source: Path instructions
🧹 Nitpick comments (1)
main.py (1)
57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit exception conversion or logger formatting.
Ruff RUF010 flags
str(e)inside the f-string. Preferlogger.error("❌ Error configuring Gemini: %s", e)or{e!s}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 57 - 58, Update the exception logging in the Gemini configuration handler to avoid calling str(e) inside the f-string. Use logger formatting with the exception as an argument, or apply explicit exception conversion with {e!s}, while preserving the existing error message.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@worship.py`:
- Around line 268-273: Update the route containing the ZoneInfo and
calculate_prayer_times calls so both synchronous operations run outside the
async event loop: either change the endpoint to a regular def for FastAPI
threadpool execution or explicitly offload both operations when retaining async.
Preserve the existing invalid-timezone HTTPException behavior and calculation
results.
---
Outside diff comments:
In `@main.py`:
- Around line 150-180: Update the ChatRequest Pydantic model to reject blank
prompts and enforce explicit maximum lengths for both prompt and context before
the flow constructs full_prompt, forwards input, or persists history. Preserve
optional context semantics while ensuring oversized or whitespace-only input is
rejected during request validation.
---
Nitpick comments:
In `@main.py`:
- Around line 57-58: Update the exception logging in the Gemini configuration
handler to avoid calling str(e) inside the f-string. Use logger formatting with
the exception as an argument, or apply explicit exception conversion with {e!s},
while preserving the existing error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 884671a3-09c4-4d14-bca3-7531ceb89583
📒 Files selected for processing (2)
main.pyworship.py
| try: | ||
| zone = ZoneInfo(tz) | ||
| except Exception: | ||
| raise HTTPException(status_code=400, detail=f"Invalid timezone: {tz}") | ||
|
|
||
| times, fallback_applied = calculate_prayer_times(lat, lon, date, method, asr, zone) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Keep synchronous timezone and calculation work off the event loop.
ZoneInfo(tz) can perform synchronous filesystem work on a cache miss, and calculate_prayer_times(...) is called synchronously immediately afterward. Under concurrent traffic, this can stall the async event loop. Make this route a regular def so FastAPI uses its threadpool, or explicitly offload both operations.
As per path instructions, **/*.py requires blocking calls inside async endpoints to be awaited or offloaded.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 270-270: Do not catch blind exception: Exception
(BLE001)
[warning] 271-271: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worship.py` around lines 268 - 273, Update the route containing the ZoneInfo
and calculate_prayer_times calls so both synchronous operations run outside the
async event loop: either change the endpoint to a regular def for FastAPI
threadpool execution or explicitly offload both operations when retaining async.
Preserve the existing invalid-timezone HTTPException behavior and calculation
results.
Source: Path instructions
2 participants
🕌 Goal & Summary
This PR implements two deterministic, non-LLM utility endpoints to address missing daily features on the Deen Bridge AI service:
GET /prayer-times: Computes local daily prayer times (Fajr, Sunrise, Dhuhr, Asr, Maghrib, Isha) from location (lat/lon), date, calculation method, asr juristic parameter, and an IANA timezone (tz).
GET /hijri and GET /gregorian: Bi-directional date conversion endpoints utilizing the Umm al-Qura calendar system.
Both utilities are fully timezone-correct, offline-capable, and handle DST transitions gracefully without external API dependencies.
🛠️ Technical Design & Implementation
To avoid dependency bloat and guarantee stability, the solar-position mathematics are implemented natively.
Formulas: Derived from standard NOAA solar calculation equations and Meeus' Astronomical Algorithms (matching the core of PrayTimes.org). Computes the Julian Date, Mean Anomaly, Solar Obliquity, Declination, and Equation of Time dynamically.
Angles & Calculations:
Fajr & Isha: Computed using the appropriate angle (e.g.,
18
∘
18
∘
for MWL,
15
∘
15
∘
for ISNA) relative to the sun's position.
Umm al-Qura (Makkah): Features a static
90
90-minute offset for Isha after Maghrib (non-Ramadan standard).
Sunrise & Sunset: Calculated at
0.833
∘
0.833
∘
below the horizon to account for atmospheric refraction and solar radius.
Asr: Supports standard (Shafi'i, Maliki, Hanbali shadow factor of 1) and Hanafi (shadow factor of 2) juristic parameters.
2. High-Latitude Fallback
At high latitudes (e.g., above
∼
48
∘
∼48
∘
during summer months) where certain solar angles are never reached, returning NaN or failing with a stack trace is prevented.
Implements the Angle-Based/Fraction-of-Night fallback (configured to
1
/
7
th
1/7th of the night duration).
If Fajr or Isha cannot be computed standardly, they are estimated as
1
/
7
th
1/7th of the night before Sunrise / after Sunset, respectively.
Appends a descriptive warning to the API response's notes property when a fallback is triggered.
3. Calendar Conversions
Wraps the lightweight, Umm al-Qura based hijridate library.
Safely captures OverflowError and ValueError (for dates outside the Umm al-Qura supported range or malformed date inputs) to return a clean 400 Bad Request instead of letting the application raise a 500 Internal Server Error.
4. Timezone Correctness
Resolves time zone names (e.g. Europe/London, Asia/Riyadh) via standard library zoneinfo.
Installs tzdata dependency to guarantee a robust, local timezone database when running on environments without built-in databases (like Windows local development).
Calculations are made in UTC and mapped to the target local timezone, delegating DST transition offsets cleanly to Python's zoneinfo module.
🧪 Verification & Test Suite (tests/test_worship.py)
A comprehensive offline test suite has been introduced to verify calculations against known reference values:
Makkah Summer: Validates Makkah prayer times on a fixed date (2023-06-15), asserting that Isha correctly occurs exactly 90 minutes after Maghrib.
London Winter: Checks standard London times using the ISNA method.
High-Latitude Summer: Tests Reykjavik, Iceland on the summer solstice (2023-06-21) to verify that the high-latitude fallback executes successfully, populates all times, and includes the expected API notes warning.
Hijri/Gregorian Anchors: Converts Gregorian date 2023-03-23 to Hijri, checking that it evaluates exactly to 1 Ramadan 1444 (and vice-versa).
Validation: Asserts that 422 Unprocessable Entity is returned for invalid coordinates (e.g., Latitude of 100), and 400 Bad Request is returned for invalid timezones or out-of-range dates.
All 8 tests pass successfully. Local linting with flake8 worship.py --max-line-length=120 produces clean output.
📁 Files Changed
worship.py [NEW]: The core endpoints, Pydantic schemas, and mathematical calculations.
main.py [MODIFY]: Registered and mounted the new /prayer-times, /hijri, and /gregorian endpoints.
requirements.txt [MODIFY]: Appended hijridate>=2.6.0 and tzdata.
tests/test_worship.py [NEW]: Automated unit tests matching reference parameters.
.github/workflows/ci.yml [MODIFY]: Extended the linting checks and py_compile checks to include worship.py, and added pytest tests/ step to the CI matrix.
README.md [MODIFY]: Updated the API routing index documentation.
.gitignore [MODIFY]: Ignores local Windows virtual environments (venv_win/).
Closes #28
Summary by CodeRabbit