Strengthen DevRadar discovery and comparison flows - #9
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR expands catalogue filtering and cursor-based loading, centralizes frontend storage and date formatting, adds backend request-boundary and alert protections, hardens extension runtime and scraping paths, and extends CI with stress checks. ChangesBackend catalogue and request boundaries
Frontend catalogue and persistence
Extension runtime and scraping hardening
CI and stress checks
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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
backend/tests/api/test_search.py-186-187 (1)
186-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse unique sentinel values in the assertions.
Fixed values such as
"Rust"and"inference"may already exist in a reused test database, allowing these assertions to pass even if the new metadata query is broken. Generate a per-test suffix and assert the exact seeded values.As per path instructions, backend tests use a real Postgres database and should avoid assertions that can pass because of pre-existing data.
Source: Path instructions
src/components/ChromeExtensionSidePanel.tsx-41-49 (1)
41-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset doesn't cancel the in-flight analyze timer.
If "Reset Simulator" (Line 227) is clicked while
analyzingis true, onlyanalyzed/feedbackare cleared —analyzingstays true andanalyzeTimer.currentkeeps running. The pending timeout later firessetAnalyzed(true), silently flipping the panel back to the "results" view after the user explicitly reset it.🐛 Proposed fix
+ const resetSimulator = () => { + if (analyzeTimer.current !== null) { + window.clearTimeout(analyzeTimer.current); + analyzeTimer.current = null; + } + setAnalyzing(false); + setAnalyzed(false); + setFeedback(null); + };- <button type="button" onClick={() => { setAnalyzed(false); setFeedback(null); }} className="text-[`#FF5A36`] font-extrabold hover:underline"> + <button type="button" onClick={resetSimulator} className="text-[`#FF5A36`] font-extrabold hover:underline">Also applies to: 227-227
src/components/ChromeExtensionSidePanel.tsx-51-74 (1)
51-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a real export timestamp and delay object URL cleanup
DTSTAMPshould be the current export time, not a fixed literal. Also moveURL.revokeObjectURL(url)off the immediate click path; revoking in the same tick can cancel the download in some browsers.
🧹 Nitpick comments (2)
src/App.tsx (2)
447-517: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
loadMoreCataloguefetches with noAbortSignal.
fetchHackathons/fetchAIOfferscalls here (lines 474-479, 488-493) omit thesignaloption that the initialloadCataloguefetch always passes. If the component unmounts (or a filter change fires a freshloadCatalogue) while a "load more" request is in flight, the request-id guard prevents a stale state update, but the underlying network request keeps running to completion unnecessarily.♻️ Wire an AbortController into load-more too
+ const controller = new AbortController(); try { if (kind === 'hackathon') { const page = await fetchHackathons(queryFilters, { cursor, limit: 50, bookmarks, alerts, + signal: controller.signal, });
691-702: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
handleToggleComparesets state from inside another setter's updater.
setCompareNotice(...)is invoked from inside thesetCompareItemsupdater (lines 719, 723, 726-730). React's docs explicitly say updater functions must be pure and "don't try to set state from inside of them or run other side effects" — in Strict Mode this updater is invoked twice, firing the notice side effect twice.handleToggleBookmark'ssetBookmarkCachecall insidesetBookmarkIds's updater (lines 694-707) has the same shape, though that one predates this diff. TheCompareModal.onRemovehandler further down (already callssetCompareNoticeafter, not inside,setCompareItems) shows the pattern the fix should follow.♻️ Move the notice out of the updater
const handleToggleCompare = useCallback((hack: Hackathon) => { - setCompareItems((prev) => { - if (prev.some((i) => i.id === hack.id)) { - setCompareNotice(`${hack.title} removed from comparison.`); - return prev.filter((i) => i.id !== hack.id); - } - if (prev.length >= 3) { - setCompareNotice('You can compare up to 3 hackathons at a time.'); - return prev; - } - setCompareNotice( - prev.length === 0 - ? `${hack.title} added. Choose one more opportunity to compare.` - : `${hack.title} added to comparison.`, - ); - return [...prev, hack]; - }); - }, []); + setCompareItems((prev) => { + if (prev.some((i) => i.id === hack.id)) return prev.filter((i) => i.id !== hack.id); + if (prev.length >= 3) return prev; + return [...prev, hack]; + }); + setCompareNotice((currentNotice) => { + // derive message from compareItems via a ref, or restructure to compute + // the message before calling setCompareItems using the latest known state. + return currentNotice; + }); + }, []);(illustrative — the cleanest fix is to compute the outcome with a ref-tracked
compareItemsvalue or acceptcompareItemsas a dependency.)Also applies to: 719-744
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: c447db20-677c-4c25-9bb8-cd367bb521d1
📒 Files selected for processing (26)
backend/app/catalog/service.pybackend/scripts/recheck_listings.pybackend/scripts/seed_default_sources.pybackend/scripts/seed_listings.pybackend/tests/api/test_search.pyextension/src/shared/extractor.tsextension/src/sidepanel/App.tsxextension/src/vite-env.d.tsindex.htmlsrc/App.tsxsrc/api/bookmarks.tssrc/api/catalog.tssrc/components/AIDealCard.tsxsrc/components/AlertSubscribeModal.tsxsrc/components/ChromeExtensionSidePanel.tsxsrc/components/CompareModal.tsxsrc/components/DetailModal.tsxsrc/components/HackathonCard.tsxsrc/components/Header.tsxsrc/components/HeroSection.tsxsrc/components/Pagination.tsxsrc/components/StatsOverview.tsxsrc/components/SubmitModal.tsxsrc/utils/countdown.tssrc/utils/dateTime.tssrc/utils/storage.ts
|
Stress-hardening update (commit 7351cbe): added fixed/chunked 1 MiB request-body enforcement, bounded public search/filter/token/idempotency inputs, malformed URL protection, alert abuse throttling, capped/recoverable bookmark import/share state, tab-keyed extension analysis, bounded page/API payloads, and repeatable frontend/extension adversarial checks. Local verification: root npm run check passed; extension npm run check passed (stress + typecheck + Chrome/Firefox builds); backend ruff passed; database-free ASGI boundary and alert-limiter smoke checks passed; Playwright at 360x800 found no page/modal/drawer overflow and verified offline/rapid-input/corrupt-storage recovery. Full PostgreSQL/Redis pytest is delegated to this PR's CI because Docker is unavailable locally. |
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
extension/src/shared/storage.ts-16-27 (1)
16-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
apiBaseUrlkeeps query/fragment, producing malformed proxy URLs.
parsed.toString()preserves?…and#…, so a base ofhttps://api.example.com/?x=1yieldshttps://api.example.com/?x=1/api/v1/searchinhandleApiProxy. Strip them while normalizing.🔧 Normalize away search/hash
) { - apiBaseUrl = parsed.toString().replace(/\/+$/, ''); + parsed.search = ''; + parsed.hash = ''; + apiBaseUrl = parsed.toString().replace(/\/+$/, ''); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 7f05e7f8-4d5b-471c-9246-07db3f72ee39
📒 Files selected for processing (40)
.github/workflows/ci.yml.gitignoreMakefilebackend/.env.examplebackend/.env.production.examplebackend/app/alerts/matcher.pybackend/app/alerts/schemas.pybackend/app/alerts/service.pybackend/app/api/limits.pybackend/app/api/public/ai_offers.pybackend/app/api/public/alerts.pybackend/app/api/public/discovery.pybackend/app/api/public/hackathons.pybackend/app/api/public/search.pybackend/app/api/public/submissions.pybackend/app/catalog/search.pybackend/app/catalog/service.pybackend/app/config.pybackend/app/discovery/service.pybackend/app/main.pybackend/app/submissions/url_policy.pybackend/tests/api/test_alerts.pybackend/tests/api/test_discovery.pybackend/tests/api/test_request_limits.pybackend/tests/api/test_submissions.pybackend/tests/submissions/test_url_policy.pyextension/package.jsonextension/src/background/index.tsextension/src/background/pendingAnalyses.tsextension/src/content/index.tsextension/src/shared/storage.tsextension/tools/stress-tests.mjspackage.jsonscripts/check.ps1scripts/check.shsrc/App.tsxsrc/api/bookmarks.tssrc/api/index.tssrc/components/BookmarksDrawer.tsxtools/frontend-stress.mjs
| const resp = await fetch(url, { | ||
| method: options.method || 'GET', | ||
| method, | ||
| headers, | ||
| body: options.body !== undefined ? JSON.stringify(options.body) : undefined, | ||
| body: serializedBody, | ||
| }); | ||
|
|
||
| const text = await readBoundedResponse(resp, MAX_API_RESPONSE_BYTES); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
fetch has no timeout — a hung API can pin the proxy request indefinitely.
Every other boundary here is bounded except wall-clock time. A slow/stalled server leaves the message-response promise unsettled and the MV3 worker holding the stream.
⏱️ Add an abort signal
const resp = await fetch(url, {
method,
headers,
body: serializedBody,
+ signal: AbortSignal.timeout(15_000),
});📝 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.
| const resp = await fetch(url, { | |
| method: options.method || 'GET', | |
| method, | |
| headers, | |
| body: options.body !== undefined ? JSON.stringify(options.body) : undefined, | |
| body: serializedBody, | |
| }); | |
| const text = await readBoundedResponse(resp, MAX_API_RESPONSE_BYTES); | |
| const resp = await fetch(url, { | |
| method, | |
| headers, | |
| body: serializedBody, | |
| signal: AbortSignal.timeout(15_000), | |
| }); | |
| const text = await readBoundedResponse(resp, MAX_API_RESPONSE_BYTES); |
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)
src/App.tsx (1)
757-759: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompare modal only auto-closes at 0 items, not below the 2-item minimum.
The "Compare now" button is disabled below 2 items (Line 1189) and
CompareModal's ownonRemovecloses at< 2(Line 1212), but this effect only closes whencompareItems.length === 0. Removing an item via a card'sonToggleCompare(not the modal) while the modal is open can leave it open showing a single item, inconsistent with the rest of the UI's 2-item invariant.useEffect(() => { - if (compareItems.length === 0) setIsCompareOpen(false); + if (compareItems.length < 2) setIsCompareOpen(false); }, [compareItems.length]);
🧹 Nitpick comments (2)
backend/app/catalog/service.py (2)
443-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic-status leak fixed; consider deduplicating the query pattern.
The join-through-
Listing+PUBLIC_VISIBLE_STATUSESfilter correctly resolves the prior finding where draft/needs-review tags leaked into this unauthenticated endpoint. No remaining security concern here.Five near-identical
select(func.distinct(func.unnest(...))).join(Listing, ...).where(Listing.kind == ..., Listing.verification_status.in_(public_statuses))blocks are duplicated. Consider extracting a small helper parameterized by model/column/kind to cut duplication and reduce the chance a future column gets added without the visibility join.♻️ Sketch of a helper to reduce duplication
async def _distinct_unnest(self, model, column, kind: ListingKind, public_statuses): result = await self._session.execute( select(func.distinct(func.unnest(column))) .select_from(model) .join(Listing, Listing.id == model.listing_id) .where( Listing.kind == kind.value, Listing.verification_status.in_(public_statuses), ) ) return result.scalars().all()
442-499: 🚀 Performance & Scalability | 🔵 TrivialConsider caching
filter_metagiven its query cost.Six sequential
unnest-backed round trips run on every call to this unauthenticated, likely frequently-hit route (fetched on every frontend mount persrc/App.tsx). Since filter metadata changes only when catalogue content changes, a short-TTL cache (or invalidate-on-write) would cut DB load without affecting data freshness materially.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 023b9a5f-07b6-4398-81d4-6614efeaf4b3
📒 Files selected for processing (4)
backend/app/catalog/service.pybackend/tests/api/test_search.pysrc/App.tsxsrc/components/ChromeExtensionSidePanel.tsx
7237e27 to
a8e08cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
extension/tools/stress-tests.mjs-49-60 (1)
49-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover a stalled response body, not only a stalled fetch.
This stub never returns headers, so it does not verify the critical deadline remains active while the response body is consumed. Return an OK response whose body blocks until the captured request signal aborts; otherwise a regression that clears the timer after headers would still pass.
extension/src/background/apiProxy.ts-43-49 (1)
43-49: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winIdempotency key is silently dropped instead of rejected, and no other custom header is ever forwarded.
Unlike every other input check in this function (path length, query count/length), an over-200-char
Idempotency-Keyis just dropped (line 47) rather than throwing. A caller relying on it for POST dedup will silently lose that protection and can end up double-submitting on retries. Separately,ApiProxyOptions.headersis typed as an arbitraryRecord<string, string>(seeextension/src/shared/messages.ts), but this proxy only ever forwardsIdempotency-Key— any other header set by a caller is silently discarded with no error.Suggested fix
const idempotencyKey = options.headers?.['Idempotency-Key']; - if (idempotencyKey && idempotencyKey.length <= 200) { - headers['Idempotency-Key'] = idempotencyKey; - } + if (idempotencyKey) { + if (idempotencyKey.length > 200) throw new Error('Idempotency-Key is too long'); + headers['Idempotency-Key'] = idempotencyKey; + }extension/src/background/apiProxy.ts-50-60 (1)
50-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNo guard against sending a body with a GET request.
serializedBodyis built wheneveroptions.body !== undefined, independent ofmethod. If a caller passes{ method: 'GET', body: {...} },fetchis called with bothmethod: 'GET'and abody, which throws aTypeErrorin browsers rather than surfacing one of this function's clean validation errors. Worth validating that a body is only allowed for non-GET methods, consistent with the other explicit checks here.Suggested fix
const method = (options.method || 'GET').toUpperCase(); if (method !== 'GET' && method !== 'POST') throw new Error('Unsupported API method'); + if (method === 'GET' && serializedBody !== undefined) { + throw new Error('GET requests cannot include a body'); + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: f5104c25-9c46-4d4e-9934-a82472fb30c3
📒 Files selected for processing (14)
.github/workflows/ci.ymlbackend/.env.examplebackend/.env.production.examplebackend/app/alerts/service.pybackend/app/api/public/alerts.pybackend/app/config.pybackend/app/main.pybackend/tests/api/test_alerts.pybackend/tests/api/test_request_limits.pyextension/src/background/apiProxy.tsextension/src/background/index.tsextension/src/content/index.tsextension/src/content/visibleText.tsextension/tools/stress-tests.mjs
a8e08cf to
ff308cd
Compare
|
Final hardening verification on head ff308cd: bounded fixed/chunked/slow request bodies; public-only filter metadata; no-IP email abuse throttling; strict extension proxy paths, headers, methods, body/response sizes, and end-to-end timeouts; bounded DOM extraction; rapid bookmark/compare input hardening; and immutable CI action pins. Regression evidence: root stress/build/lint passed, extension stress/typecheck/Chrome/Firefox builds passed, real-browser same-tick comparison test passed (3 selections retained; modal closed at 1), backend PostgreSQL/Redis CI passed in 57s, frontend/extension CI passed in 27s, secret scan passed, and CodeRabbit completed successfully. Branch is clean, synchronized, and mergeable. |
Resolve 6 conflicts. Backend adopts main's shipped rate-limit architecture; frontend combines both sides rather than picking a winner. - alerts.py: main's route-level AlertSignupRateLimiter (hashed IP + email, honeypot) replaces the PR's in-service limiter. Kept the PR's Annotated token-length validation, whose typing.Annotated import sat inside the conflict and would otherwise have broken the module import. - alerts/service.py: reverted to main's version. Auto-merge had left both limiters live, double-counting the email bucket and breaking main's 10-per-client test against the PR's MAX_PER_IP = 5. Main's cleanup of abandoned unconfirmed signups is absent from the PR branch and is restored. - test_alerts.py: main's three rate-limit tests plus the PR's two schema tests, which are independent of the limiter architecture. - Header / StatsOverview: the PR's nav/section aria labels plus main's dark:border and SUBLINE constant. Auto-merged closing tags were already </nav> and </section>, so main's div openers could not be used. - HeroSection: main's isActive/toggle chips (auto-merged code at the map already dereferences chip.isActive) plus the PR's live-discovery banner, responsive scroll container, and working live-scan trigger. - App.tsx: union of both import sets; main's toast auto-dismiss plus the PR's handleToggleBookmark, which takes a fallbackItem second argument that a caller already passes. Dropped main's top-level StatsOverview block -- the PR relocated it into catalogueSummary, so keeping both double-rendered it; carried main's loading prop across. Verified: tsc -b, vite build, oxlint, frontend stress test, ruff, and the full backend suite (378 passed). Chip toggle and aria wiring confirmed live in the browser.
2 participants
Summary
Why
The previous first-use path placed large secondary stats ahead of search on mobile, comparison opened too early and discarded the user's selection, and the frontend ignored API cursors after the first 50 rows. Several controls advertised filters the API did not execute, while region and eligibility filters had state but no direct controls. Hard-coded choices drifted from the catalogue, browser-storage exceptions could crash preference flows, and some callbacks erased unrelated shared-list parameters. These issues obscured DevRadar's core loop: find an opportunity, evaluate its trust and fit, then continue on the official source.
User impact
Users reach search sooner, can distinguish sample data from live catalogue data, compare multiple hackathons without losing state, load the complete catalogue, and use accurate catalogue-driven filters. Saved preferences degrade safely in privacy-restricted browsers, mobile and dark-mode controls retain usable placement and native theming, and clear primary actions continue to official registration or claim pages.
Verification
npm run check— production TypeScript/Vite build and oxlint passnpm run typecheckinextension/— passnpm run buildandnpm run build:firefoxinextension/— passpython -m ruff check app tests scriptsinbackend/— passpython -m compileall -q app tests scriptsinbackend/— passgit diff --check— passThe PostgreSQL-backed pytest suite could not run locally because this Windows environment has no Docker/PostgreSQL service. The repository's GitHub CI provisions PostgreSQL and Redis and supplies that integration evidence on this PR.
filterMeta, including distinct AI-offertechnology/tagvalues and richer advanced filters (region/eligibility/AI tags), fixing meta mismatch and improving mobile-focused filter/preset behavior; hero now loads meta safely on mount and supports cursor-based “load more” without stale overwrites (monotonic request IDs + stale-response guards), with live-vs-sample recovery messaging.api/limits), bounded request-body middleware with clear 413/408 problem+json responses, trace-id validation, malformed-URL protection, alert confirmation rate limiting, and adversarial boundary tests (with DB/PostgreSQL integration coverage delegated to CI when Docker isn’t available).filter_meta.technologiesnow merges distinct AI-offer tags (distinct unnest/join through listing visibility constraints) with existing hackathon technologies, and status parsing now validates length and deduplicates parsed verification statuses.