-
Notifications
You must be signed in to change notification settings - Fork 9
Fix light mode #101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix light mode #101
Conversation
…us in the future we need to use httpx as it is more suitable in handling async requests.
WalkthroughThis PR migrates the HTTP client library from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/src/components/MpesaKit.module.css (1)
857-857: Critical: Undefined CSS variable reference will break the gradient.Line 857 references
var(--dark-bg), but this variable was removed from:rootper the PR changes. The linear gradient will be invalid and.ctaSectionwill lose its intended background styling.Apply this diff to fix the gradient:
-.ctaSection { +.ctaSection { - background: linear-gradient(135deg, var(--dark-bg) 0%, rgba(0, 209, 58, 0.05) 100%); + background: linear-gradient(135deg, rgba(0, 0, 0, 0.02) 0%, rgba(0, 209, 58, 0.05) 100%);Adjust the
rgba(0, 0, 0, 0.02)value if needed to align with your light mode aesthetic.tests/unit/http_client/test_mpesa_http_client.py (2)
112-114: Addis_successattribute to mock response.The implementation at
mpesakit/http_client/mpesa_http_client.pyline 128 checksresponse.is_success, but the mock response doesn't set this attribute. This may cause the GET success test to fail.Apply this diff:
mock_response = Mock() mock_response.status_code = 200 + mock_response.is_success = True mock_response.json.return_value = {"foo": "bar"}
138-142: Addis_successattribute to mock response.The implementation at
mpesakit/http_client/mpesa_http_client.pyline 128 checksresponse.is_success, but the mock response doesn't set this attribute, causing the test to fail per the pipeline failure log.Apply this diff:
mock_response = Mock() mock_response.status_code = 500 + mock_response.is_success = False mock_response.json.side_effect = ValueError()mpesakit/http_client/mpesa_http_client.py (3)
1-157: Critical: PR description does not match the actual code changes.The PR is titled "Fix light mode" and the description discusses CSS changes for improving visibility and contrast in light theme. However, the actual changes in this file (and others) migrate the HTTP client from
requeststohttpx. This is a significant inconsistency that needs to be addressed.Please update the PR title and description to accurately reflect the HTTP client migration changes, or verify that the correct files were included in this PR.
78-94: Add separate handling for ConnectError.The tests at
tests/unit/http_client/test_mpesa_http_client.pylines 97-106 expect error_code "CONNECTION_ERROR" whenhttpx.ConnectErroris raised, but the current implementation catches allhttpx.RequestErrorexceptions and returns "REQUEST_FAILED". httpx.ConnectError is a specific network error type for failed connections.Apply this diff to add separate handling:
except httpx.TimeoutException: raise MpesaApiException( MpesaError( error_code="REQUEST_TIMEOUT", error_message="Request to Mpesa timed out.", status_code=None, ) ) + except httpx.ConnectError as e: + raise MpesaApiException( + MpesaError( + error_code="CONNECTION_ERROR", + error_message=f"Failed to connect: {str(e)}", + status_code=None, + raw_response=None, + ) + ) except httpx.RequestError as e: raise MpesaApiException( MpesaError( error_code="REQUEST_FAILED", error_message=f"HTTP request failed: {str(e)}", status_code=None, raw_response=None, ) )
141-157: Add separate handling for ConnectError in GET method.Similar to the POST method, the GET method needs separate handling for
httpx.ConnectErrorto match test expectations at lines 172-180. The tests expect error_code "CONNECTION_ERROR" for connection failures.Apply this diff to add separate handling:
except httpx.TimeoutException: raise MpesaApiException( MpesaError( error_code="REQUEST_TIMEOUT", error_message="Request to Mpesa timed out.", status_code=None, ) ) + except httpx.ConnectError as e: + raise MpesaApiException( + MpesaError( + error_code="CONNECTION_ERROR", + error_message=f"Failed to connect: {str(e)}", + status_code=None, + raw_response=None, + ) + ) except httpx.RequestError as e: raise MpesaApiException( MpesaError( error_code="REQUEST_FAILED", error_message=f"HTTP request failed: {str(e)}", status_code=None, raw_response=None, ) )
🧹 Nitpick comments (3)
docs/src/components/MpesaKit.module.css (1)
144-146: Remove duplicateborder-radiusdeclaration.Lines 145 and 146 both define
border-radius: 50px;on the same.btnrule. The duplication is redundant.Apply this diff to remove the duplicate:
.btn { padding: 0.875rem 1.5rem; border: 1px solid rgba(0, 0, 0, 0.1); - border-radius: 50px; border-radius: 50px; font-weight: 600;tests/integration/mpesa_express/test_stk_push_e2e.py (2)
85-86: Consider specifying timeout for consistency.The
httpx.Client()context manager should specify a timeout to match the pattern used inmpesakit/http_client/mpesa_http_client.py(line 57, 120) which usestimeout=10.Apply this diff:
- with httpx.Client() as client: - client.post(f"{callback_base_url}/clear") + with httpx.Client(timeout=10) as client: + client.post(f"{callback_base_url}/clear")
115-127: Consider specifying timeout for consistency.The
httpx.Client()context manager should specify a timeout to match the pattern used inmpesakit/http_client/mpesa_http_client.py. Note that line 118 already specifiestimeout=45in the individualget()call, but for consistency, it's better to set it at the client level.Apply this diff:
- with httpx.Client() as client: + with httpx.Client(timeout=45) as client: for _ in range(30): time.sleep(1) - r = client.get(f"{callback_base_url}/latest", timeout=45) + r = client.get(f"{callback_base_url}/latest")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/src/components/MpesaKit.module.css(5 hunks)mpesakit/http_client/mpesa_http_client.py(5 hunks)tests/integration/mpesa_express/test_stk_push_e2e.py(3 hunks)tests/unit/http_client/test_mpesa_http_client.py(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration/mpesa_express/test_stk_push_e2e.py (2)
mpesakit/http_client/mpesa_http_client.py (2)
post(39-94)get(96-157)mpesakit/mpesa_express/schemas.py (1)
StkPushSimulateCallback(351-452)
mpesakit/http_client/mpesa_http_client.py (1)
mpesakit/errors.py (3)
MpesaApiException(33-49)MpesaError(11-30)error_code(42-44)
🪛 GitHub Actions: Run Unit Tests
tests/unit/http_client/test_mpesa_http_client.py
[error] 93-93: Test post_timeout failed: Mock side_effect raises httpx.TimeoutException which triggers TypeError: RequestError.init() missing 'message'.
[error] 104-104: Test post_connection_error failed: Mock side_effect raises httpx.ConnectError leading to TypeError: RequestError.init() missing 'message'.
[error] 129-129: Test_get_http_error failed: MpesaApiException not raised as expected when HTTP error response is returned.
[error] 144-144: Test_get_json_decode_error failed: MpesaApiException not raised when JSON decode raises ValueError.
[error] 168-168: Test_get_timeout failed: Mock side_effect raises httpx.TimeoutException which triggers TypeError: RequestError.init() missing 'message'.
[error] 179-179: Test_get_connection_error failed: Mock side_effect raises httpx.ConnectError leading to TypeError: RequestError.init() missing 'message'.
🔇 Additional comments (5)
docs/src/components/MpesaKit.module.css (3)
261-261: Verify color contrast for code text on dark gradient background.The
.codeContenttext is set to#f8f8f8(very light gray) against the.codeDemodark gradient background. While this generally provides adequate contrast, confirm that the combination meets WCAG accessibility standards when viewed at various brightness levels, as mentioned in the PR checklist.Verify the contrast ratio between
#f8f8f8(foreground) and the gradient in.codeDemo(background) meets WCAG AA standards (4.5:1 minimum for text).
7-8: Light mode color tokens are well-positioned.The updates to
--card-bg: rgba(0, 0, 0, 0.05)and--text-primary: #1a1a1aestablish a clear light-mode foundation with improved contrast. These changes align well with the PR objective of improving visibility.
746-747: API Status section styling supports light mode readability.The background adjustment to
rgba(0, 209, 58, 0.02)with the explicitcolor: var(--text-primary)ensures text remains readable in light mode while maintaining the subtle green tint for visual context.tests/unit/http_client/test_mpesa_http_client.py (2)
176-176: Fix httpx.ConnectError instantiation.httpx.ConnectError requires a
messageparameter in its constructor. The mock side_effect is missing this required parameter, causing the test to fail.Apply this diff:
- side_effect=httpx.ConnectError, + side_effect=httpx.ConnectError("Connection failed"),Likely an incorrect or invalid review comment.
90-90: Fix httpx.TimeoutException instantiation.httpx.TimeoutException requires a
messageparameter in its constructor. The mock side_effect is missing this required parameter, causing the test to fail.Apply this diff:
- side_effect=httpx.TimeoutException, + side_effect=httpx.TimeoutException("Connection timeout"),Likely an incorrect or invalid review comment.
| border: 1px solid rgba(0, 0, 0, 0.1); | ||
| border: 1px solid rgba(255, 255, 255, 0.1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conflicting border definitions on .featureCard.
Lines 617 and 618 both set border, but with different colors: rgba(0, 0, 0, 0.1) (dark) and rgba(255, 255, 255, 0.1) (light). The second declaration will override the first, making the first line ineffective.
Clarify the intended border style and apply a single border definition:
.featureCard {
background: var(--card-bg);
border-radius: 20px;
padding: 2.5rem;
- border: 1px solid rgba(0, 0, 0, 0.1);
border: 1px solid rgba(255, 255, 255, 0.1);
transition: all 0.3s ease;Alternatively, if the intent is to use a dark border for light mode, remove line 618 and keep line 617.
📝 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.
| border: 1px solid rgba(0, 0, 0, 0.1); | |
| border: 1px solid rgba(255, 255, 255, 0.1); | |
| border: 1px solid rgba(255, 255, 255, 0.1); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 618-618: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
border is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In docs/src/components/MpesaKit.module.css around lines 617-618 there are two
conflicting border declarations for .featureCard (rgba(0,0,0,0.1) and
rgba(255,255,255,0.1)) so remove the unintended one and keep the intended color;
if you need different borders for light/dark themes instead of duplicate lines,
replace them with a single base border and add a prefers-color-scheme media
query (or use a CSS variable) to override the color for dark mode.
|
|
||
| from typing import Dict, Any, Optional | ||
| import requests | ||
| import httpx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify httpx is declared as a dependency.
The code now imports and uses httpx, but there's no indication that this dependency has been added to the project's dependency manifest (e.g., requirements.txt, setup.py, pyproject.toml). This will cause import errors in production.
Run the following script to check if httpx is declared as a dependency:
🏁 Script executed:
#!/bin/bash
# Check if httpx is declared in common dependency files
echo "=== Checking for httpx in dependency files ==="
fd -t f 'requirements.*\.txt$|setup\.py$|pyproject\.toml$|Pipfile$' --exec echo "File: {}" \; --exec grep -n "httpx" {} \; 2>/dev/null || echo "No httpx dependency found"Length of output: 95
I need to inspect the actual contents of pyproject.toml to verify the current dependencies.
🏁 Script executed:
#!/bin/bash
# View the contents of pyproject.toml to see current dependencies
cat ./pyproject.tomlLength of output: 2651
Add httpx to project dependencies in pyproject.toml.
The code imports httpx, but it is not declared in the dependencies list. This will cause import errors when the package is installed. Add httpx to the dependencies section in pyproject.toml:
dependencies = [
"pydantic >=2.10.6,<3.0.0",
"pydantic[email] >=2.10.6,<3.0.0",
"requests >=2.32.3,<3.0.0",
"typing_extensions >= 4.12.2,<5.0.0",
"cryptography >=41.0.7",
"httpx", # Add version constraint as appropriate
]🤖 Prompt for AI Agents
In mpesakit/http_client/mpesa_http_client.py at line 7 the module imports httpx
but the package doesn't declare httpx in pyproject.toml; open pyproject.toml,
locate the [project] / dependencies list and add an appropriate
version-constrained entry for "httpx" (for example "httpx >=0.24.0,<1.0.0" or
whatever your compatibility policy requires), save and run
poetry/pipenv/installer to update lockfiles so the dependency is installed
during packaging and CI.
| "mpesakit.http_client.mpesa_http_client.requests.post", | ||
| side_effect=requests.ConnectionError, | ||
| "mpesakit.http_client.mpesa_http_client.httpx.Client.post", | ||
| side_effect=httpx.ConnectError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix httpx.ConnectError instantiation.
httpx.ConnectError requires a message parameter in its constructor. The mock side_effect is missing this required parameter, causing the test to fail.
Apply this diff:
- side_effect=httpx.ConnectError,
+ side_effect=httpx.ConnectError("Connection failed"),🤖 Prompt for AI Agents
In tests/unit/http_client/test_mpesa_http_client.py around line 101 the mock
sets side_effect=httpx.ConnectError but httpx.ConnectError requires a message
parameter; replace the side_effect with a properly instantiated exception (for
example side_effect=httpx.ConnectError("connection failed") or a callable that
raises httpx.ConnectError with a descriptive message) so the mock raises a valid
ConnectError instance.
| mock_response = Mock() | ||
| mock_response.ok = False | ||
| mock_response.status_code = 404 | ||
| mock_response.json.return_value = {"errorMessage": "Not Found"} | ||
| mock_get.return_value = mock_response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add is_success attribute to mock response.
The implementation at mpesakit/http_client/mpesa_http_client.py line 128 checks response.is_success, but the mock response doesn't set this attribute, causing the test to fail per the pipeline failure log.
Apply this diff:
mock_response = Mock()
mock_response.status_code = 404
+ mock_response.is_success = False
mock_response.json.return_value = {"errorMessage": "Not Found"}📝 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.
| mock_response = Mock() | |
| mock_response.ok = False | |
| mock_response.status_code = 404 | |
| mock_response.json.return_value = {"errorMessage": "Not Found"} | |
| mock_get.return_value = mock_response | |
| mock_response = Mock() | |
| mock_response.status_code = 404 | |
| mock_response.is_success = False | |
| mock_response.json.return_value = {"errorMessage": "Not Found"} | |
| mock_get.return_value = mock_response |
🤖 Prompt for AI Agents
In tests/unit/http_client/test_mpesa_http_client.py around lines 124 to 127, the
mock response lacks the is_success attribute expected by
mpesakit/http_client/mpesa_http_client.py; set mock_response.is_success = False
(for this 404 case) so the test mimics the real response shape; for completeness
ensure other mocked responses in the file set is_success=True/False consistently
to match expected status branches.
| "mpesakit.http_client.mpesa_http_client.requests.get", | ||
| side_effect=requests.Timeout, | ||
| "mpesakit.http_client.mpesa_http_client.httpx.Client.get", | ||
| side_effect=httpx.TimeoutException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix httpx.TimeoutException instantiation.
httpx.TimeoutException requires a message parameter in its constructor. The mock side_effect is missing this required parameter, causing the test to fail.
Apply this diff:
- side_effect=httpx.TimeoutException,
+ side_effect=httpx.TimeoutException("Connection timeout"),🤖 Prompt for AI Agents
In tests/unit/http_client/test_mpesa_http_client.py around line 165, the mock
side_effect uses httpx.TimeoutException without the required message argument;
update the mock to raise an instantiated exception with a message (e.g., use
side_effect=httpx.TimeoutException("timeout") or a lambda that raises
httpx.TimeoutException("timeout")) so the constructor receives the required
`message` parameter.
Description
This PR improves the light theme visibility and contrast across MpesaKit components.
Previously, card backgrounds and text colors were too light, reducing readability and visual separation between UI elements.
#100
Key updates:
Adjusted --card-bg to a light neutral tone for better card visibility.
Set --text-primary to dark gray (#1a1a1a) for improved legibility on white surfaces.
Added subtle borders to buttons and feature cards for clearer definition.
Updated API status section background to a faint green tint for softer contrast.
Ensured code text and section labels maintain readable contrast.
Cleaned redundant CSS (moved inline styles to components where applicable).
This enhances the overall accessibility and user experience in light mode without affecting the dark theme.
Type of Change
How Has This Been Tested?
Verified changes manually in both light and dark modes.
Checked visibility of cards, buttons, and API status section on different screen brightness levels.
Confirmed no layout or style regressions in dark mode.
Checklist
Screenshots (if applicable)
Additional Context
This fix ensures the MpesaKit documentation and component demos remain visually accessible and consistent with the global Docusaurus theme.
Summary by CodeRabbit
Style
Chores