Add KLIPY GIF proxy, routes and tests#30
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds KLIPY GIF proxy handlers and routes that validate requests, require NIP-98 auth and an active Purple account, forward queries to KLIPY upstream, and include tests and test-client helpers; environment variables for KLIPY are set in the test harness. ChangesKLIPY GIF Proxy Integration
Sequence DiagramsequenceDiagram
participant Client
participant Router as Express Router
participant Auth as Purple Auth Service
participant KlipyProxy as KLIPY Proxy Handler
participant KlipyApi as KLIPY API
Client->>Router: GET /gifs/search (NIP-98 auth)
Router->>Auth: check_account(authorized_pubkey)
alt account inactive
Auth-->>Router: not found
Router-->>Client: 401 { error: 'Account not found' }
else account active
Auth-->>Router: account ok
Router->>KlipyProxy: proxy_klipy_gif_search(app, req)
KlipyProxy->>KlipyApi: GET /api/v1/{app_key}/gifs/search?...
KlipyApi-->>KlipyProxy: JSON response (or non-2xx)
KlipyProxy-->>Router: parsed upstream payload/status
Router-->>Client: forwarded status + payload
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 44 minutes and 36 seconds.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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/klipy_proxy.js`:
- Around line 8-15: The parse_integer_query_param function currently uses
Number.parseInt which accepts partial numeric strings like "2foo"; update
parse_integer_query_param to reject any value that is not a fully-formed integer
string before parsing (e.g., match the input against a strict integer regex such
as /^\d+$/ or /^[+-]?\d+$/ depending on desired sign policy), only call
Number.parseInt(String(value), 10) when the regex passes, keep returning
fallback for undefined/null/'' and return null for malformed values; reference
the existing function name parse_integer_query_param to locate and change the
validation logic.
- Around line 39-70: The proxy_klipy_request function currently calls fetch
without a timeout; wrap the fetch call with an AbortController: create a
controller, pass controller.signal into fetch(upstream_url, { method:'GET',
headers:{...}, signal }), set a timer (e.g., const timeoutId = setTimeout(() =>
controller.abort(), <desired_ms>)), and clearTimeout(timeoutId) after fetch
completes (both in success and in the catch); in the catch branch detect an
abort (request_error.name === 'AbortError') and return a 504 or a clear
timeout-specific error via error_response(res, 'KLIPY request timed out', 504),
otherwise keep the existing error handling for other request errors. Ensure the
timer is cleared even when JSON.parse throws and before every return.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f8bc401-1469-48f5-a2d6-aeab537a0767
📒 Files selected for processing (5)
src/klipy_proxy.jssrc/router_config.jstest/controllers/purple_test_client.jstest/controllers/purple_test_controller.jstest/router_config.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/router_config.test.js (1)
193-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
global.fetchmock ordering concern applies here.The mock is installed at line 193 before
ln_flow_buy_subscriptionruns at line 219 — same risk as in the search test. Apply the same fix: move the mock setup to after the subscription call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/router_config.test.js` around lines 193 - 219, The global.fetch mock in test/router_config.test.js is installed before calling purple_api_controller.ln_flow_buy_subscription which risks the same ordering issue as the search test; move the mock setup (the assignment to global.fetch and its t.teardown restoration) to after invoking ln_flow_buy_subscription so the live call runs before the test-specific fetch override, and ensure the teardown still restores original_fetch after the mock is installed.
🧹 Nitpick comments (1)
src/router_config.js (1)
16-24: 💤 Low value
require_active_purple_useris clean — consider reusing it in the existing/translateroute too.Lines 43–47 in the
/translatehandler duplicate this exact same inline account check. Swapping torequire_active_purple_userthere would keep the two authorization paths consistent.♻️ Proposed refactor for the `/translate` route
router.get('/translate', required_nip98_auth, async (req, res) => { - const check_account_result = check_account(app, req.authorized_pubkey) - if (!check_account_result.ok) { - unauthorized_response(res, check_account_result.message) + if (!require_active_purple_user(app, req, res)) { return } handle_translate(app, req, res) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/router_config.js` around lines 16 - 24, Duplicate inline account-check logic in the /translate route should be replaced with a call to the existing require_active_purple_user function: inside the /translate handler, remove the manual check_account(...) + unauthorized_response(...) block and instead call if (!require_active_purple_user(app, req, res)) return; so the handler reuses require_active_purple_user(app, req, res) (which itself wraps check_account and unauthorized_response) and keeps authorization paths consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/router_config.test.js`:
- Around line 110-141: The global.fetch mock is installed before creating the
subscription and may capture unintended fetches from
purple_api_controller.ln_flow_buy_subscription; move the entire mock setup (the
global.fetch assignment with its assertions and the t.teardown that restores
original_fetch) to after you call purple_api_controller.set_account_uuid(pubkey,
'TEST-UUID-123') and await
purple_api_controller.ln_flow_buy_subscription(pubkey, 'purple_one_month') so
only the subsequent search call (the GIF search) triggers the mock; keep the
same mock body and teardown but relocate it to just before the code that invokes
the search_gifs call.
---
Duplicate comments:
In `@test/router_config.test.js`:
- Around line 193-219: The global.fetch mock in test/router_config.test.js is
installed before calling purple_api_controller.ln_flow_buy_subscription which
risks the same ordering issue as the search test; move the mock setup (the
assignment to global.fetch and its t.teardown restoration) to after invoking
ln_flow_buy_subscription so the live call runs before the test-specific fetch
override, and ensure the teardown still restores original_fetch after the mock
is installed.
---
Nitpick comments:
In `@src/router_config.js`:
- Around line 16-24: Duplicate inline account-check logic in the /translate
route should be replaced with a call to the existing require_active_purple_user
function: inside the /translate handler, remove the manual check_account(...) +
unauthorized_response(...) block and instead call if
(!require_active_purple_user(app, req, res)) return; so the handler reuses
require_active_purple_user(app, req, res) (which itself wraps check_account and
unauthorized_response) and keeps authorization paths consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82eaf904-6942-4aee-8d78-bac2b7759746
📒 Files selected for processing (5)
src/klipy_proxy.jssrc/router_config.jstest/controllers/purple_test_client.jstest/controllers/purple_test_controller.jstest/router_config.test.js
✅ Files skipped from review due to trivial changes (2)
- src/klipy_proxy.js
- test/controllers/purple_test_controller.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/controllers/purple_test_client.js
Introduce a server-side proxy for KLIPY GIF search and featured endpoints, add authenticated routes that require an active Purple subscription, and extend the test client/controller and router tests. Requires KLIPY_APP_KEY to be set (KLIPY_API_BASE_URL is optional).
Collaborator
Author
|
CC @jb55, please let me know if you have any concerns or suggestions for a follow up improvement. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a server-side proxy for KLIPY GIF search and featured endpoints,
add authenticated routes that require an active Purple subscription, and extend the test client/controller and router tests.
Requires KLIPY_APP_KEY to be set (KLIPY_API_BASE_URL is optional).
Testing
Summary by CodeRabbit
New Features
Tests