backend/enh/added-flag-for-runInMasterCloudApi#1395
Conversation
WalkthroughThe ChangesMaster Cloud Forward Flag Gating
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/mobility-core/src/Kernel/External/MasterCloudForward.hs (1)
140-157: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBreaking shared-kernel API change. This exported function now requires a new positional
Bool, so any consumer still calling the old signature will fail to compile. Keep a backward-compatible wrapper/default, or roll this out to every downstream caller in the same release.🤖 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 `@lib/mobility-core/src/Kernel/External/MasterCloudForward.hs` around lines 140 - 157, The exported runThroughMasterCloud signature now adds a positional Bool, which breaks existing callers of the shared-kernel API. Update MasterCloudForward.runThroughMasterCloud to preserve backward compatibility by keeping the old arity via a wrapper/defaulted overload, or ensure every downstream call site is updated in the same change; use the function name and its existing parameters (origBaseUrl, eClient, desc) to locate the affected code.
🧹 Nitpick comments (1)
lib/mobility-core/src/Kernel/External/MasterCloudForward.hs (1)
151-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a semantic type instead of a bare
Boolforflag.The function already branches on one implicit
Bool(shouldForwardfrom env) and now takes a second explicitBoolfrom the caller. Bare booleans at call sites (e.g.,... "desc" True) are not self-documenting about intent (per the doc-comment, it's meant to gate "city specific forwarder" behavior). Anewtype(e.g.,CityForwardEnabled Bool) would make call sites clearer and prevent future parameter-order mistakes if more flags are added.♻️ Example semantic type
+newtype CityForwardEnabled = CityForwardEnabled Bool + runThroughMasterCloud :: ... Text -> - Bool -> + CityForwardEnabled -> m (Either ClientError a) -runThroughMasterCloud origBaseUrl eClient desc flag = do +runThroughMasterCloud origBaseUrl eClient desc (CityForwardEnabled flag) = do🤖 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 `@lib/mobility-core/src/Kernel/External/MasterCloudForward.hs` around lines 151 - 157, Replace the bare Bool flag in runThroughMasterCloud with a semantic type so call sites are self-describing and harder to misuse. Introduce a newtype for the city-specific forwarding toggle (for example around the existing flag parameter), update the runThroughMasterCloud signature and pattern matching/guard logic accordingly, and adjust callers to pass the named wrapper instead of True/False directly.
🤖 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.
Outside diff comments:
In `@lib/mobility-core/src/Kernel/External/MasterCloudForward.hs`:
- Around line 140-157: The exported runThroughMasterCloud signature now adds a
positional Bool, which breaks existing callers of the shared-kernel API. Update
MasterCloudForward.runThroughMasterCloud to preserve backward compatibility by
keeping the old arity via a wrapper/defaulted overload, or ensure every
downstream call site is updated in the same change; use the function name and
its existing parameters (origBaseUrl, eClient, desc) to locate the affected
code.
---
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/MasterCloudForward.hs`:
- Around line 151-157: Replace the bare Bool flag in runThroughMasterCloud with
a semantic type so call sites are self-describing and harder to misuse.
Introduce a newtype for the city-specific forwarding toggle (for example around
the existing flag parameter), update the runThroughMasterCloud signature and
pattern matching/guard logic accordingly, and adjust callers to pass the named
wrapper instead of True/False directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ad89bc7-c08a-4ce6-a1fa-9a6bbf713258
📒 Files selected for processing (2)
lib/mobility-core/src/Kernel/External/MasterCloudForward.hslib/mobility-core/test-integration/Main.hs
H-C-21
force-pushed
the
backend/enh/added-flag-for-runInMasterCloudApi
branch
from
b165372 to
6acae38
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/mobility-core/src/Kernel/External/MasterCloudForward.hs (1)
137-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc says "quad-gated" but the case-match only checks three conditions.
The updated comment describes forwarding as "Quad-gated: caller flag on + masterUrl set + masterSecret set" — that's only three predicates. The case-match at Line 155 confirms this:
case (shouldForward, cfg.masterUrl, cfg.masterSecret) of. If "quad" is meant to include the removed internalgetRunApiInMasterCloudcheck that callers are now expected to fold into their own flag, the comment should say so explicitly — otherwise callers may assume the library still enforces that check internally.✏️ Suggested doc clarification
--- Drop-in replacement for `@callAPI`@. Quad-gated: caller flag on + --- masterUrl set + masterSecret set → forwarded. Anything else → direct call. --- flag is for city specific forwarder. ++-- Drop-in replacement for `@callAPI`@. Triple-gated: caller-supplied ++-- `@shouldForward`@ (callers are responsible for combining this with ++-- 'getRunApiInMasterCloud' and any city-specific check) + masterUrl set ++-- + masterSecret set → forwarded. Anything else → direct call.🤖 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 `@lib/mobility-core/src/Kernel/External/MasterCloudForward.hs` around lines 137 - 139, Clarify the doc comment for the forwarding logic in MasterCloudForward so it matches the actual guard checks in the `callAPI`/`case (shouldForward, cfg.masterUrl, cfg.masterSecret) of` flow. Update the wording to explain that forwarding is controlled by the caller-provided flag plus `masterUrl` and `masterSecret`, and explicitly note that the նախկին internal `getRunApiInMasterCloud` check is no longer enforced here if that is intended.
🤖 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.
Nitpick comments:
In `@lib/mobility-core/src/Kernel/External/MasterCloudForward.hs`:
- Around line 137-139: Clarify the doc comment for the forwarding logic in
MasterCloudForward so it matches the actual guard checks in the `callAPI`/`case
(shouldForward, cfg.masterUrl, cfg.masterSecret) of` flow. Update the wording to
explain that forwarding is controlled by the caller-provided flag plus
`masterUrl` and `masterSecret`, and explicitly note that the նախկին internal
`getRunApiInMasterCloud` check is no longer enforced here if that is intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8f9db7-cf56-4f10-83ce-f53965bf73fe
📒 Files selected for processing (2)
lib/mobility-core/src/Kernel/External/MasterCloudForward.hslib/mobility-core/test-integration/Main.hs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/mobility-core/test-integration/Main.hs
3 participants
Summary by CodeRabbit