Team DoD: Unified Valkey E-Commerce Challenges 1-14#8
Team DoD: Unified Valkey E-Commerce Challenges 1-14#8Vasanthadithya-mundrathi wants to merge 11 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds an integrated Valkey-powered backend (Challenges 1–14) plus new frontend “challenge” pages and API bindings to drive the demo experience end-to-end.
Changes:
- Adds a comprehensive Node/TS backend under
backend/checkoutwith Valkey-backed auth, catalog/cart, search (FT + vector), checkout (BullMQ), observability, delivery geo, rate limiting, recommendations, and agentic search. - Adds a Python FastAPI embeddings service and a
docker-compose.ymlto run Valkey Bundle + OpenSearch + embeddings locally. - Extends the React frontend with challenge routes/pages, shared navigation, cart context, service client, and updates tests/assets/styles.
Reviewed changes
Copilot reviewed 68 out of 81 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/setupTests.js | Adds matchMedia mock for Jest/jsdom compatibility. |
| frontend/src/services/valkeyApi.js | Introduces frontend API client for the integrated backend. |
| frontend/src/pages/*Page.jsx | Adds challenge pages for catalog, growth, semantic search, analytics, observability, delivery, rate limiting, recommendations, agentic search. |
| frontend/src/context/CartContext.jsx | Adds a cart provider backed by backend cart endpoints. |
| frontend/src/components/ValkeyChallengeNav.jsx | Adds nav bar for challenge routes. |
| frontend/src/App.js | Wires new routes and wraps app in CartProvider. |
| frontend/src/App.test.js | Updates routing tests to cover new pages. |
| frontend/src/index.scss | Adds demo-specific media/panel styles + fixes import indentation. |
| docker-compose.yml | Adds local stack for Valkey Bundle, OpenSearch, and embeddings service. |
| backend/embeddings/* | Adds FastAPI embeddings service + Dockerfile. |
| backend/checkout/* | Adds integrated backend implementation, seed script, and Vitest unit/integration tests. |
| README.md | Updates root documentation for the integrated demo stack. |
| .gitignore | Adds repo-level ignores for node/python artifacts and env files. |
Comments suppressed due to low confidence (4)
frontend/src/pages/RecommendationsPage.jsx:1
recordView/addRecommendedare invoked from event handlers but don’t handle failures; any error fromrecordRecommendationEvent,addItem, orrefreshwill reject and surface as an unhandled promise rejection. Wrap these bodies intry/catchand report viasetMessage(...)(and optionally avoid callingrefresh()when the first step fails) to keep the UI stable.
frontend/src/App.js:1- In React Router v6, the
exactprop is not used (matching is “exact” by default for path segments). Keepingexactis misleading and can cause confusion during maintenance; removeexactfrom all<Route ...>declarations.
frontend/src/pages/SemanticSearchPage.jsx:1 - The label isn’t programmatically associated with its input (
htmlFor+id), which reduces usability for screen readers and form navigation. Add stableidattributes to inputs/selects and sethtmlForon the corresponding<label>s (applies similarly to other new forms like Catalog/Growth/AgenticSearch/Observability pages).
docker-compose.yml:1 - This hard-codes an admin password into version control (even though the security plugin is disabled). Move the password to an environment variable (e.g., via a
.envfile) or remove it when security is disabled to avoid credential leakage and reduce the chance of accidental reuse in non-local environments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await client.call("JSON.SET", productKey(productId), "$", JSON.stringify(updated)); | ||
| await indexProduct(client, updated); | ||
| return updated; | ||
| } | ||
|
|
| const raw = (await client.call( | ||
| "FT.SEARCH", | ||
| PRODUCT_VECTOR_INDEX, | ||
| `*=>[KNN ${limit} @embedding $query_vector AS vector_score]`, | ||
| "PARAMS", | ||
| "2", | ||
| "query_vector", | ||
| toFloat32Buffer(vector), | ||
| "SORTBY", | ||
| "vector_score", | ||
| "ASC", | ||
| "RETURN", | ||
| "1", | ||
| "vector_score", | ||
| "DIALECT", | ||
| "2" | ||
| )) as unknown[]; |
|
UPDATE 487DDDF. ADDED INTEGRATIONS DASHBOARD ENDPOINT AND PAGE. VALIDATION PASSED BACKEND BUILD TEST 26 FRONTEND TEST BUILD CHROME SMOKE. NO H1 H2 OR HACKERRANK FOLDERS. |
|
| Filename | Overview |
|---|---|
| backend/checkout/src/server.ts | 1182-line Express server wiring all challenge routes; product write endpoints lack admin role enforcement and the CORS fallback returns the first configured origin for unmatched requests; SSE order-events fan-out reads the entire global stream per connection |
| backend/checkout/src/rateLimit.ts | Sliding-window rate limiter using Valkey sorted sets; identity resolution trusts the client-supplied X-User-Id header as authenticated without session validation, and the check-then-add is split across two non-atomic pipelines |
| backend/checkout/src/auth.ts | Session-based auth with bcrypt passwords stored in Valkey JSON; brute-force lockout implemented correctly; demo bypass path (allowUserIdHeader) synthesises sessions without DB validation and is the root cause of the product-write exposure |
| backend/checkout/src/queues.ts | BullMQ-backed five-queue checkout pipeline (reserve → payment → confirm → release → dispatch) with correct retry/backoff and reservation TTL cleanup |
| backend/checkout/src/idempotency.ts | 24-hour idempotency layer using a per-user/key lock in Valkey with polling fallback; logic is sound for single-node Valkey deployments |
| backend/checkout/src/store.ts | Core order and product persistence using Valkey JSON; ownership check on requireOwnedOrder is correct; SCAN-based listing could be slow on large key sets but acceptable for a demo |
| backend/checkout/src/embeddings.ts | Deterministic local embedding fallback with SHA-256 tokenisation; remote embedding client has 1.5-second timeout and graceful fallback to local embeddings |
| frontend/src/services/valkeyApi.js | Frontend API client; generates a random demo user-id stored in localStorage and sends it as X-User-Id for demo flows; this is intentional for the hackathon but directly feeds the server-side identity bypass |
| frontend/src/App.js | Adds ten new challenge routes wrapped in CartProvider; straightforward routing change with no issues |
| backend/checkout/src/agent.ts | Rule-based agentic search with Valkey JSON conversation store; keyword extraction is deterministic and safe; feedback stored per-product without user scoping (minor) |
Comments Outside Diff (1)
-
backend/checkout/src/server.ts, line 1161-1183 (link)CORS middleware falls back to first allowed origin for unmatched requests
When
requestOriginis not inconfiguredOrigins,allowOriginresolves toconfiguredOrigins[0](e.g.http://localhost:3000). Browsers compare the actualOriginheader against this reflected value and will still block the request, so this is not a direct bypass — but it sends a misleading header value and causes CORS failures from legitimate origins that were simply not listed (e.g. a second dev port). The unmatched branch should omit theAccess-Control-Allow-Originheader or return the request origin only when it is explicitly allowed.
Reviews (1): Last reviewed commit: "add images" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| }); | ||
|
|
||
| app.patch("/api/products/:id", async (request: AuthedRequest, response, next) => { | ||
| try { | ||
| const product = await patchCatalogProduct(context.client, request.params.id, request.body as Record<string, unknown>); | ||
| if (!product) { | ||
| throw new ApiError(404, "product_not_found", "Product was not found."); | ||
| } | ||
| response.json({ product }); | ||
| } catch (error) { | ||
| next(error); | ||
| } | ||
| }); | ||
|
|
||
| app.post("/api/checkout/start", async (request: AuthedRequest, response, next) => { | ||
| try { | ||
| const userId = request.userId!; | ||
| const key = idempotencyKeyFrom(request); | ||
|
|
There was a problem hiding this comment.
POST /api/products and PATCH /api/products/:id sit behind the middleware at line 845 which calls authenticateRequest with allowUserIdHeader: true. That option synthesises a valid session for any request that includes an X-User-Id header — no password, no real token needed. As a result, any HTTP client that adds X-User-Id: anything can create or overwrite product records in Valkey. There is no role === "admin" (or equivalent) check on either handler.
| if (path.startsWith("/api/checkout/start")) return "/api/checkout/start"; | ||
| if (path.startsWith("/api/auth/login")) return "/api/auth/login"; | ||
| return "default"; | ||
| } | ||
|
|
||
| function identify(request: Request): { id: string; authenticated: boolean } { | ||
| const userId = request.header("X-User-Id"); | ||
| if (userId) return { id: userId, authenticated: true }; | ||
| const authorization = request.header("Authorization"); | ||
| if (authorization) return { id: `session:${authorization.slice(-32)}`, authenticated: true }; |
There was a problem hiding this comment.
The identify function marks any request that includes an X-User-Id header as authenticated: true and uses the header value directly as the rate-limit bucket key. Because X-User-Id is never validated against an actual session, an anonymous client can: (1) set X-User-Id: anything to receive the higher authenticated quota (e.g. 6 vs. 3 on /api/ratelimit/test, 120 vs. 60 on the default bucket); and (2) DoS a legitimate user's quota by sending requests under that user's well-known ID. The session token or the Authorization header should be the authoritative source for determining authenticated identity.
| return; | ||
| } | ||
|
|
||
| await client | ||
| .multi() | ||
| .zadd(key, now, `${now}:${Math.random().toString(36).slice(2)}`) | ||
| .expire(key, rule.window) | ||
| .exec(); | ||
|
|
||
| response.setHeader("X-RateLimit-Remaining", String(limit - count - 1)); | ||
| next(); | ||
| } catch { | ||
| next(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| export function endpointGroup(path: string): string { | ||
| if (path === "/api/ratelimit/test") return path; | ||
| if (path === "/api/search/semantic") return path; |
There was a problem hiding this comment.
The sliding-window implementation runs two separate multi() pipelines: one to trim old entries and read the current count, and a second to add the new entry and refresh the TTL. In between those two round-trips, concurrent requests on the same key can all observe a count below the limit and all be admitted. The correct pattern is a single Lua script (or a single pipeline that includes ZADD and re-reads the count) so the check and the increment are atomic.
| response.setHeader("Connection", "keep-alive"); | ||
| response.flushHeaders?.(); | ||
|
|
||
| let active = true; | ||
| let lastId = "$"; | ||
| request.on("close", () => { | ||
| active = false; | ||
| }); | ||
|
|
||
| while (active) { | ||
| const rows = (await context.client.xread( | ||
| "BLOCK", | ||
| 1000, | ||
| "STREAMS", | ||
| ORDER_STREAM_KEY, | ||
| lastId | ||
| )) as Array<[string, Array<[string, string[]]>]> | null; | ||
|
|
||
| if (!rows) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const [, entries] of rows) { | ||
| for (const [entryId, fields] of entries) { | ||
| lastId = entryId; | ||
| const event = Object.fromEntries( | ||
| Array.from({ length: fields.length / 2 }, (_, index) => [ | ||
| fields[index * 2], | ||
| fields[index * 2 + 1], | ||
| ]) | ||
| ); | ||
| if (event.orderId === request.params.id) { | ||
| response.write(`id: ${entryId}\n`); | ||
| response.write(`event: order\n`); | ||
| response.write(`data: ${JSON.stringify(event)}\n\n`); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| response.end(); | ||
| } catch (error) { | ||
| next(error); | ||
| } | ||
| }); | ||
|
|
||
| app.use((error: unknown, request: TraceRequest, response: Response, _next: express.NextFunction) => { | ||
| const apiError = toApiError(error); | ||
| void recordLog(context.client, { | ||
| level: apiError.status >= 500 ? "error" : "warn", | ||
| event: "api_error", |
There was a problem hiding this comment.
The XREAD BLOCK call at line 1106 reads every new entry written to ORDER_STREAM_KEY for the lifetime of the SSE connection and then discards entries that don't match request.params.id. Under concurrent connections, each connection independently processes the full event fan-out. A per-order consumer group or a dedicated per-order list/channel (e.g. XREAD on a per-order stream key like stream:orders:{orderId}) would avoid the fan-out overhead and make the filter exact at the Valkey level.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
rlunar
left a comment
rlunar
left a comment
There was a problem hiding this comment.
@Vasanthadithya-mundrathi please review all comments made by Greptile and address them to be able to merge and publish on valkey-io org.
Summary
Team DoD unified implementation for the Valkey e-commerce hackathon challenges 1-14.
Validation
cd backend/checkout && npm run buildcd backend/checkout && npm test- 6 files, 25 tests passedcd frontend && CI=true npm test -- --watchAll=false- 6 tests passedcd frontend && npm run buildhttp://localhost:3001/growthverified rendered challenge UI, product images, updated logo, and no localhost console errors on the checked page.Notes
This implementation uses Valkey Bundle services only for the datastore/runtime. The Node dependency
ioredisremains the RESP client used by BullMQ and the backend to talk to Valkey.