Add Finsight to Tinyfish Cookbook#226
Open
MANASMATHUR wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds a new FinSight recipe: a FastAPI backend orchestrating a TinyFish search/fetch/LLM synthesis pipeline for Vietnam market-intelligence queries, and a React/Vite frontend landing page that streams pipeline progress and renders structured reports, plus environment, deployment, and README updates. ChangesBackend API and agent workflow
Frontend landing page and API integration
Docs and deployment configuration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Frontend
participant FastAPIApp
participant AgentWorkflows
participant TinyFishClient
participant LLMClient
Frontend->>FastAPIApp: POST /api/v1/intelligence/query/stream
FastAPIApp->>AgentWorkflows: process_query(query, query_type, emitter)
AgentWorkflows->>TinyFishClient: search()/fetch() per source
AgentWorkflows->>LLMClient: extract_facts()/synthesize()
AgentWorkflows-->>FastAPIApp: pipeline events + QueryResponse
FastAPIApp-->>Frontend: SSE pipeline events then result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
MANASMATHUR
force-pushed
the
manas/add-finsight
branch
from
dc3b2e3 to
066bec9
Compare
MANASMATHUR
force-pushed
the
manas/add-finsight
branch
from
066bec9 to
0f12dbc
Compare
Contributor
Author
|
Let me know how I can assist! ✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (17)
finsight/api/models/schemas.py (2)
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
query_typedescription.The description lists only
'sme_loan', 'regulatory', or 'competitor'butQueryTypehas six members (REAL_ESTATE,MOBILITY,GENERALare also defined at lines 6-12). Update the docstring to reflect all valid values, since this text will surface in generated OpenAPI docs.Suggested fix
- query_type: QueryType = Field(..., description="Type of query: 'sme_loan', 'regulatory', or 'competitor'") + query_type: QueryType = Field(..., description="Type of query: one of sme_loan, regulatory, competitor, real_estate, mobility, general")🤖 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 `@finsight/api/models/schemas.py` around lines 15 - 18, The QueryRequest query_type field description is stale and only mentions three values even though QueryType includes six members. Update the Field description in QueryRequest to list every valid QueryType option, keeping the OpenAPI docs aligned with the enum definition and its actual members in QueryType.
39-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
stage/status/tierare freeform strings despite documented closed value sets.
PipelineEvent.stage/status(lines 40, 43) andDataQuality.tier(line 49) only constrain allowed values via description text ("search | fetch | preflight | synthesize", etc.), not viaLiteral/Enum. This allows typos to pass validation silently and lets frontend/backend drift on accepted values.Suggested enforcement
+from typing import Literal + class PipelineEvent(BaseModel): - stage: str = Field(..., description="search | fetch | preflight | synthesize") + stage: Literal["search", "fetch", "preflight", "synthesize"] = Field(...) message: str url: Optional[str] = None - status: Optional[str] = Field(None, description="ok | failed | warn | skipped | pending") + status: Optional[Literal["ok", "failed", "warn", "skipped", "pending"]] = None meta: Optional[Dict[str, Any]] = None🤖 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 `@finsight/api/models/schemas.py` around lines 39 - 49, `PipelineEvent.stage`, `PipelineEvent.status`, and `DataQuality.tier` are still unconstrained freeform strings; update the `PipelineEvent` and `DataQuality` model fields to use explicit `Literal` values or shared `Enum` types so validation rejects unsupported values. Make the allowed sets match the documented options in the field descriptions, and ensure any serialization/deserialization paths that consume these models continue to use the same symbols (`PipelineEvent`, `DataQuality`) with the new closed value definitions.finsight/api/core/logger.py (2)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
datetime.utcnow()is deprecated.Prefer
datetime.now(timezone.utc)for timezone-aware timestamps going forward.🤖 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 `@finsight/api/core/logger.py` at line 9, The timestamp generation in the logger setup uses datetime.utcnow(), which should be replaced with a timezone-aware UTC timestamp. Update the timestamp assignment in the logger code to use datetime.now(timezone.utc) and keep the existing ISO formatting so the output remains consistent while avoiding the deprecated API.
7-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo fallback if
json.dumpsfails.
format()has no try/except aroundjson.dumps; if a future caller passes non-JSON-serializableextradata or the log message itself raises during formatting, the entire logging call will raise and could mask the original error being logged.Suggested defensive fallback
def format(self, record: logging.LogRecord) -> str: log_data = { "timestamp": datetime.utcnow().isoformat(), "level": record.levelname, "message": record.getMessage(), "module": record.module, "funcName": record.funcName, } if record.exc_info: log_data["exception"] = self.formatException(record.exc_info) - return json.dumps(log_data) + try: + return json.dumps(log_data, default=str) + except (TypeError, ValueError): + return json.dumps({"level": record.levelname, "message": str(record.getMessage())})🤖 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 `@finsight/api/core/logger.py` around lines 7 - 17, The formatter in the `format` method should not let `json.dumps` failures break logging; wrap the JSON serialization in a defensive fallback so `logging` still emits a usable message if non-serializable data or message formatting errors occur. Update the `format` implementation in the logger class to catch serialization issues around the `log_data` payload, and return a safe plain-text fallback that still includes key fields like level, message, and exception context.finsight/api/core/config.py (1)
93-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
analysis_modeis silently overwritten when not on Vercel.
model_post_initalways forcesself.analysis_mode = "deep"(line 108) whenis_vercelis false, regardless of what theANALYSIS_MODEenv var was set to. This makes the field non-configurable for local/Render testing of the "fast" path, even thoughanalysis_modeis exposed as a public settable Field with a description implying it's user-controlled.🤖 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 `@finsight/api/core/config.py` around lines 93 - 109, `model_post_init` is overriding the user-configured `analysis_mode` on non-Vercel runs. Update `Config.model_post_init` so it only forces the Vercel-specific fast settings inside the `is_vercel` branch, and preserves the existing `analysis_mode` value otherwise instead of unconditionally setting it to "deep". Keep `_apply_deep_pipeline` behavior aligned with the effective mode so the `analysis_mode` field remains configurable via env or constructor inputs.finsight/frontend/src/styles.css (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused shadcn palette and
darkvariant.
bg-primary/bg-card/bg-popover/dark:aren’t used outside this file, while the app’s actual styling only uses thefs-*tokens. Keeping the generic:root/.darkblock and@custom-variant darkadds dead weight unless dark mode is coming soon.🤖 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 `@finsight/frontend/src/styles.css` at line 4, Remove the unused shadcn theme setup from the styles file by deleting the generic `:root` and `.dark` palette variables and the `@custom-variant dark` declaration, since the app only relies on the `fs-*` tokens. Update the stylesheet so only the active `fs-*` custom properties remain, and make sure no `bg-primary`, `bg-card`, `bg-popover`, or `dark:` references depend on the removed `dark` variant. Refer to the existing stylesheet theme block and `@custom-variant dark` definition when cleaning this up.finsight/api/services/pipeline.py (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate host-parsing logic vs.
source_ranker._host.
host_from_urlandsource_ranker.py's_host(Lines 51-55) both parse hostnames and strip"www.", but with slightly different fallback semantics (returning the originalurlhere vs. empty string there). Consider extracting a single shared helper to avoid divergence.🤖 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 `@finsight/api/services/pipeline.py` around lines 27 - 33, The host parsing logic in host_from_url duplicates source_ranker._host and has different fallback behavior, so consolidate them into a single shared helper and make both call it. Update host_from_url to reuse the same hostname extraction and www-stripping semantics as _host, including a consistent fallback path, and reference both host_from_url and _host when wiring the shared implementation.finsight/api/services/tinyfish_client.py (1)
35-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider reusing a persistent
httpx.AsyncClientinstead of creating one per call.
TinyFishClientis instantiated once as a module-level singleton (Line 92), but eachsearch/fetchcall opens and tears down its ownAsyncClient, losing connection pooling/keep-alive across the many concurrent calls the workflow makes.♻️ Proposed refactor
class TinyFishClient: def __init__(self): self.api_key = settings.tinyfish_api_key self.headers = {...} self.timeout = settings.timeout_seconds + self._client = httpx.AsyncClient() + + async def aclose(self): + await self._client.aclose()Then replace
async with httpx.AsyncClient() as client: ...withclient = self._clientin both methods, and close it during app shutdown.Also applies to: 66-72
🤖 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 `@finsight/api/services/tinyfish_client.py` around lines 35 - 41, The TinyFishClient search/fetch methods are creating a new httpx.AsyncClient on every call, which prevents connection reuse across the singleton instance. Update TinyFishClient to hold a persistent client (for example as a _client field on the class), change the search and fetch methods to use self._client instead of creating a new client in each call, and make sure the shared client is closed during application shutdown.finsight/api/services/source_ranker.py (1)
107-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate low-signal-host check — reuse
is_low_signal_host.♻️ Proposed simplification
- if any(host == domain or host.endswith(f".{domain}") for domain in LOW_SIGNAL_DOMAINS): - score -= 4.0 + if is_low_signal_host(url): + score -= 4.0🤖 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 `@finsight/api/services/source_ranker.py` around lines 107 - 108, The low-signal host logic is duplicated in the source ranking path; replace the inline domain check in the scoring flow with the existing is_low_signal_host helper used by source_ranker so the same matching rules are reused consistently. Update the relevant logic around the score adjustment in source_ranker to call is_low_signal_host(host) and keep only the score decrement there, avoiding the repeated LOW_SIGNAL_DOMAINS comparison.finsight/frontend/src/components/landing/VietnamMap.tsx (1)
59-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCity markers are mouse-only; no keyboard equivalent.
onMouseEnteris the sole interaction to changeactiveCity(Lines 64, 84); there's noonFocus/tabIndex/keyboard handler, so keyboard users can't explore per-city stats.♿ Proposed fix (sketch)
<g key={city.name} transform={`translate(${city.x}, ${city.y})`} - className="cursor-pointer" + className="cursor-pointer" + tabIndex={0} + role="button" + aria-label={`Show ${city.name} stats`} onMouseEnter={() => setActiveCity(city)} + onFocus={() => setActiveCity(city)} >🤖 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 `@finsight/frontend/src/components/landing/VietnamMap.tsx` around lines 59 - 98, The city markers in VietnamMap are only interactive via mouse hover, so keyboard users cannot change activeCity or access the city details. Update the marker groups in VietnamMap to be focusable and keyboard-operable by adding tabIndex, an appropriate accessibility role/label, and an onFocus handler that mirrors onMouseEnter. Also add keyboard handling in the same city marker rendering blocks so users can navigate and activate cities without a mouse.finsight/frontend/src/components/landing/SkyHero.tsx (1)
5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate nav config with
ClosingCTA.tsx.
NAV_ITEMShere is identical toFOOTER_NAVinClosingCTA.tsx. Consider extracting a shared constant to avoid drift between header/footer nav.🤖 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 `@finsight/frontend/src/components/landing/SkyHero.tsx` around lines 5 - 9, NAV_ITEMS in SkyHero.tsx is duplicated from FOOTER_NAV in ClosingCTA.tsx, so extract the shared nav configuration into a single reusable constant and import it in both places to prevent drift. Update the SkyHero component and the ClosingCTA component to reference the shared symbol instead of maintaining separate arrays, keeping the existing labels and ids unchanged.finsight/frontend/src/lib/api.ts (2)
109-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate error-parsing logic.
The error-body parsing/message-extraction block is duplicated verbatim between
runIntelligenceQueryandrunIntelligenceQueryStream. Extract a shared helper to avoid drift when error handling needs to change.♻️ Proposed refactor
+async function extractErrorMessage(res: Response): Promise<string> { + const body = (await res.json().catch(() => null)) as + | ApiError + | { detail?: string } + | null; + return ( + (body && 'error' in body && body.error?.message) || + (body && 'detail' in body && typeof body.detail === 'string' ? body.detail : null) || + `Request failed with status ${res.status}` + ); +}Then replace both
if (!res.ok) { ... }blocks with:- if (!res.ok) { - const body = (await res.json().catch(() => null)) as - | ApiError - | { detail?: string } - | null; - const message = - (body && 'error' in body && body.error?.message) || - (body && 'detail' in body && typeof body.detail === 'string' ? body.detail : null) || - `Request failed with status ${res.status}`; - throw new Error(message); - } + if (!res.ok) { + throw new Error(await extractErrorMessage(res)); + }Also applies to: 173-183
🤖 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 `@finsight/frontend/src/lib/api.ts` around lines 109 - 132, The error parsing and message extraction logic in runIntelligenceQuery is duplicated with runIntelligenceQueryStream, so extract it into a shared helper and reuse it in both response handlers. Move the non-OK response body parsing and fallback message construction into a single utility near these functions, then have both runIntelligenceQuery and runIntelligenceQueryStream call that helper before throwing the error.
159-212: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo cancellation support for in-flight streaming request; reader not released on early exit.
runIntelligenceQueryStreamaccepts noAbortSignal, andIntelligenceContext.runQuery(finsight/frontend/src/context/IntelligenceContext.tsx:103-190) doesn't cancel a prior in-flight stream before starting a new one — it only guards state updates viarunIdRef, letting the old fetch/backend workflow keep running to completion. TheAnalysisConsoleUI currently disables the run button whileisRunning, which limits (but doesn't eliminate) reachability — e.g. the provider could be reused elsewhere, or unmounted mid-stream without cleanup. Also, on earlyreturn(line 206, result received) orthrow(line 204, error received) thereaderis never released/cancelled, leaving the stream's lock in an indeterminate state.Each abandoned SSE stream corresponds to a full backend search/fetch/LLM synthesis run (per finsight/api/index.py:126-178), so uncancelled requests keep consuming backend compute/LLM quota even after the UI moves on.
🔧 Proposed fix — accept an AbortSignal and clean up the reader
export async function runIntelligenceQueryStream( query: string, queryType: QueryType, - onPipelineEvent: (event: PipelineEvent) => void + onPipelineEvent: (event: PipelineEvent) => void, + signal?: AbortSignal ): Promise<QueryResponse> { const res = await fetch(apiUrl('/api/v1/intelligence/query/stream'), { method: 'POST', headers: { 'Content-Type': 'application/json', Accept: 'text/event-stream', }, body: JSON.stringify({ query, query_type: queryType }), + signal, }); ... const reader = res.body.getReader(); const decoder = new TextDecoder(); let buffer = ''; - - while (true) { - const { done, value } = await reader.read(); - if (done) break; - ... - } - - throw new Error('Stream ended before a result was returned.'); + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + ... + } + throw new Error('Stream ended before a result was returned.'); + } finally { + reader.cancel().catch(() => {}); + } }Then in
IntelligenceContext.runQuery, keep anAbortControllerref, call.abort()on any previous run before starting a new one, and pass.signalthrough.🤖 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 `@finsight/frontend/src/lib/api.ts` around lines 159 - 212, Add cancellation and stream cleanup to runIntelligenceQueryStream: it currently has no AbortSignal support and never releases the reader on early return or error. Update runIntelligenceQueryStream to accept an AbortSignal and ensure the reader is cancelled or released in a cleanup path when a result is returned, an error is thrown, or the request is aborted. Then update IntelligenceContext.runQuery to keep an AbortController ref, abort any previous in-flight query before starting a new one, and pass the signal into runIntelligenceQueryStream.finsight/frontend/src/components/BrandMark.tsx (2)
5-6: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInline size style overrides
className-based sizing.
style={{ width: '2.25rem', height: '2.25rem' }}unconditionally forces the SVG to a fixed size regardless ofclassName. Today it happens to match theh-9 w-9default, but any caller passing a different size class (e.g.h-6 w-6) would be silently ignored since inline styles win over Tailwind utility classes.🔧 Proposed fix
<svg viewBox="0 0 40 40" className={className ?? 'h-9 w-9'} - style={{ width: '2.25rem', height: '2.25rem' }} aria-hidden="true" >🤖 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 `@finsight/frontend/src/components/BrandMark.tsx` around lines 5 - 6, The BrandMark SVG size is being forced by the inline style in BrandMark, which overrides any size passed through className. Remove the fixed width/height style and let the className-driven Tailwind sizing control the element, keeping the default size only through the className fallback in BrandMark.
9-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the gradient id per
BrandMarkinstance.BrandMarkis a reusable SVG component with a staticfs-gradid, so rendering it more than once in the same document can make theurl(#fs-grad)fills collide.useId()avoids that.🤖 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 `@finsight/frontend/src/components/BrandMark.tsx` around lines 9 - 13, The gradient identifier in BrandMark is static, which can cause collisions when multiple BrandMark instances render in the same document. Update the BrandMark component to generate a per-instance id using useId, then use that generated id consistently in the <linearGradient> definition and every url(#...) reference that targets it. Keep the change localized to the BrandMark component so each SVG instance has its own scoped gradient.finsight/frontend/src/context/IntelligenceContext.tsx (1)
132-147: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExpose the metric count in
synthesizeevent meta instead of parsing the message. The live update still depends on/extracted (\d+) verified metric/, while the backendsynthesizeevent only emitsmessageandstatustoday. Add a structured count tometaand read that here so copy changes don’t stall the progress indicator.🤖 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 `@finsight/frontend/src/context/IntelligenceContext.tsx` around lines 132 - 147, The verified metric progress update in IntelligenceContext currently parses event.message text for the count, which is brittle. Update the synthesize event flow so the backend includes the verified metric count in event.meta, then change the setSourcePreflight handling in IntelligenceContext to read that structured meta field instead of matching /extracted (\d+) verified metric/. Keep the existing synthesize stage check and verifiedMetrics state update, but source the value from the typed event payload rather than the message string.finsight/api/services/agent_workflows.py (1)
74-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBrittle, hardcoded keyword heuristic for answer-status classification.
The English/Vietnamese substring matching only covers a handful of specific topics (delivery fee, foreign capital, storefront density, valuation); any other query silently falls through to
"direct"regardless of actual coverage. Since this heuristic directly drives the data-quality gating shown in the pipeline UI, misclassification is invisible to users.Consider moving these keyword sets to configuration/constants and/or logging when the default fallback path is hit, so blind spots are observable rather than silent.
🤖 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 `@finsight/api/services/agent_workflows.py` around lines 74 - 97, The _infer_answer_status method uses brittle hardcoded substring checks and silently falls back to "direct" for any query that does not match the small keyword list. Refactor the keyword sets in _infer_answer_status into named constants or configuration so they are easier to maintain, and add an observable signal when the default fallback path is taken. Keep the existing query_text and gap_text checks, but make the fallback explicit so blind spots in answer-status classification are visible rather than silently treated as "direct".
🤖 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.
Inline comments:
In `@finsight/.env.template`:
- Around line 2-3: Swap the order of the environment variable entries in the
.env.template so it matches dotenv-linter expectations; specifically, reorder
the two keys in the template so the OPENAI_API_KEY and TINYFISH_API_KEY entries
are in the lint-compliant sequence without changing their values.
In `@finsight/api/index.py`:
- Around line 184-191: The legacy get_intelligence_query endpoint is exposing
the expensive intelligence workflow over a public GET route with no safeguards.
Remove this GET route or gate it behind authentication and rate limiting before
it reaches _run_intelligence_query, and keep the logic behind the existing
/api/v1/intelligence/query handler protected so crawlers or prefetchers cannot
trigger the paid TinyFish/LLM flow.
- Around line 40-46: The global_exception_handler currently logs
request.url.path directly, which can contain attacker-controlled control
characters and lead to log forging. Update the logging in
global_exception_handler to sanitize the path before passing it to
logger.exception by stripping or escaping newline and other control characters,
while keeping the rest of the error handling and JSONResponse unchanged.
- Around line 193-210: The get_market_ticker endpoint is bypassing the same API
configuration guard and workflow timeout handling used by query_intelligence and
query_intelligence_stream. Update get_market_ticker to call
_ensure_api_configured() before any agent_workflows.process_query work, and wrap
each workflow call with asyncio.wait_for using settings.workflow_timeout_seconds
so failures return a clean 503-style error instead of hanging. Also run the HCMC
and Hanoi queries concurrently rather than awaiting them one after the other,
and keep the logic localized around get_market_ticker and
agent_workflows.process_query so it stays consistent with the other endpoints.
- Around line 137-178: The SSE workflow in run_workflow and event_generator is
leaking internal exception text and keeps running after the client disconnects.
Replace the raw str(exc) error payload with the same sanitized generic message
used elsewhere, and add proper client-disconnect handling by using a real
FastAPI Request object alongside the existing request parameter so
event_generator can stop or cancel the task when the connection is gone. Ensure
the cleanup path in event_generator no longer waits for the full task to finish
unconditionally.
In `@finsight/api/services/agent_workflows.py`:
- Around line 26-28: The in-memory `_async_cache` in `agent_workflows.py` is
unbounded and can retain stale entries forever, while concurrent identical
queries still run duplicate pipelines. Replace the raw dict cache with a bounded
TTL-based cache in the same area where `_async_cache`, `CACHE_TTL`, and
`CACHE_VERSION` are defined, and add an in-flight request map to coalesce
duplicate work in the query handling path around the cache lookup/insert logic
in `query_answered` and the downstream async flow. Ensure expired entries are
evicted automatically and that concurrent requests for the same cache key await
the same `asyncio.Future` instead of launching separate fetch/LLM calls.
In `@finsight/api/services/query_planner.py`:
- Around line 25-38: The MOBILITY keyword fallback in query_planner.py is taking
precedence over already classified non-MOBILITY queries, causing COMPETITOR
queries that mention grabfood/shopeefood to be misrouted. Update the branching
around the query_type check in the query_planner logic so the keyword-based
MOBILITY path only applies when the query is unclassified or GENERAL, and
preserve the existing COMPETITOR branch for query_type == QueryType.COMPETITOR
so competitor-specific angles are still appended.
In `@finsight/api/services/tinyfish_client.py`:
- Around line 20-25: The retry policy in TinyFishClient is too broad because it
retries all HTTPStatusError cases, including 4xx auth and bad-request failures.
Update the retry condition used by the retry decorator on TinyFishClient methods
like search and fetch so it only retries transient failures: keep
httpx.RequestError and restrict HTTPStatusError to 5xx responses. Adjust the
retry predicate/helper accordingly so non-transient client errors surface
immediately instead of burning the full backoff.
In `@finsight/frontend/src/components/landing/AnalysisConsole.tsx`:
- Around line 48-52: The Enter-key handler in AnalysisConsole.tsx bypasses the
same minimum query-length guard used by the Run Analysis button. Update
handleKeyDown to mirror the button’s disabled logic by checking the trimmed
query length before calling handleRun, in addition to !isRunning, so short
queries cannot trigger runQuery. Keep the validation consistent between
handleKeyDown and the button state derived from query.trim().length.
In `@finsight/frontend/src/main.ts`:
- Around line 1-60: The Vite starter entrypoint is unused scaffolding, so remove
the orphaned logic from main.ts by deleting the starter DOM template and the
setupCounter import/use, and align the app bootstrap with the actual entry path
used by index.html and the App component tree. Also remove the now-unused
counter.ts and style.css scaffold files, and make sure the real entrypoint
remains the one that renders App.tsx with src/styles.css.
In `@finsight/package.json`:
- Line 10: The install:all script in package.json only installs Python deps and
frontend packages, so the root npm devDependency concurrently is never installed
on a fresh checkout. Update install:all to also run npm install at the repo root
before or alongside the frontend install, and use the existing install script
entry point if available so npm run dev can resolve concurrently from the root
package.json.
- Line 8: The dev:api script in package.json is hardcoded to
.\venv\Scripts\uvicorn, but install:all does not guarantee that launcher exists.
Update the dev:api command to use the same launcher pattern as the docs,
preferably python -m uvicorn, or alternatively make install:all create the
expected venv consistently; use the dev:api script entry to locate the change.
---
Nitpick comments:
In `@finsight/api/core/config.py`:
- Around line 93-109: `model_post_init` is overriding the user-configured
`analysis_mode` on non-Vercel runs. Update `Config.model_post_init` so it only
forces the Vercel-specific fast settings inside the `is_vercel` branch, and
preserves the existing `analysis_mode` value otherwise instead of
unconditionally setting it to "deep". Keep `_apply_deep_pipeline` behavior
aligned with the effective mode so the `analysis_mode` field remains
configurable via env or constructor inputs.
In `@finsight/api/core/logger.py`:
- Line 9: The timestamp generation in the logger setup uses datetime.utcnow(),
which should be replaced with a timezone-aware UTC timestamp. Update the
timestamp assignment in the logger code to use datetime.now(timezone.utc) and
keep the existing ISO formatting so the output remains consistent while avoiding
the deprecated API.
- Around line 7-17: The formatter in the `format` method should not let
`json.dumps` failures break logging; wrap the JSON serialization in a defensive
fallback so `logging` still emits a usable message if non-serializable data or
message formatting errors occur. Update the `format` implementation in the
logger class to catch serialization issues around the `log_data` payload, and
return a safe plain-text fallback that still includes key fields like level,
message, and exception context.
In `@finsight/api/models/schemas.py`:
- Around line 15-18: The QueryRequest query_type field description is stale and
only mentions three values even though QueryType includes six members. Update
the Field description in QueryRequest to list every valid QueryType option,
keeping the OpenAPI docs aligned with the enum definition and its actual members
in QueryType.
- Around line 39-49: `PipelineEvent.stage`, `PipelineEvent.status`, and
`DataQuality.tier` are still unconstrained freeform strings; update the
`PipelineEvent` and `DataQuality` model fields to use explicit `Literal` values
or shared `Enum` types so validation rejects unsupported values. Make the
allowed sets match the documented options in the field descriptions, and ensure
any serialization/deserialization paths that consume these models continue to
use the same symbols (`PipelineEvent`, `DataQuality`) with the new closed value
definitions.
In `@finsight/api/services/agent_workflows.py`:
- Around line 74-97: The _infer_answer_status method uses brittle hardcoded
substring checks and silently falls back to "direct" for any query that does not
match the small keyword list. Refactor the keyword sets in _infer_answer_status
into named constants or configuration so they are easier to maintain, and add an
observable signal when the default fallback path is taken. Keep the existing
query_text and gap_text checks, but make the fallback explicit so blind spots in
answer-status classification are visible rather than silently treated as
"direct".
In `@finsight/api/services/pipeline.py`:
- Around line 27-33: The host parsing logic in host_from_url duplicates
source_ranker._host and has different fallback behavior, so consolidate them
into a single shared helper and make both call it. Update host_from_url to reuse
the same hostname extraction and www-stripping semantics as _host, including a
consistent fallback path, and reference both host_from_url and _host when wiring
the shared implementation.
In `@finsight/api/services/source_ranker.py`:
- Around line 107-108: The low-signal host logic is duplicated in the source
ranking path; replace the inline domain check in the scoring flow with the
existing is_low_signal_host helper used by source_ranker so the same matching
rules are reused consistently. Update the relevant logic around the score
adjustment in source_ranker to call is_low_signal_host(host) and keep only the
score decrement there, avoiding the repeated LOW_SIGNAL_DOMAINS comparison.
In `@finsight/api/services/tinyfish_client.py`:
- Around line 35-41: The TinyFishClient search/fetch methods are creating a new
httpx.AsyncClient on every call, which prevents connection reuse across the
singleton instance. Update TinyFishClient to hold a persistent client (for
example as a _client field on the class), change the search and fetch methods to
use self._client instead of creating a new client in each call, and make sure
the shared client is closed during application shutdown.
In `@finsight/frontend/src/components/BrandMark.tsx`:
- Around line 5-6: The BrandMark SVG size is being forced by the inline style in
BrandMark, which overrides any size passed through className. Remove the fixed
width/height style and let the className-driven Tailwind sizing control the
element, keeping the default size only through the className fallback in
BrandMark.
- Around line 9-13: The gradient identifier in BrandMark is static, which can
cause collisions when multiple BrandMark instances render in the same document.
Update the BrandMark component to generate a per-instance id using useId, then
use that generated id consistently in the <linearGradient> definition and every
url(#...) reference that targets it. Keep the change localized to the BrandMark
component so each SVG instance has its own scoped gradient.
In `@finsight/frontend/src/components/landing/SkyHero.tsx`:
- Around line 5-9: NAV_ITEMS in SkyHero.tsx is duplicated from FOOTER_NAV in
ClosingCTA.tsx, so extract the shared nav configuration into a single reusable
constant and import it in both places to prevent drift. Update the SkyHero
component and the ClosingCTA component to reference the shared symbol instead of
maintaining separate arrays, keeping the existing labels and ids unchanged.
In `@finsight/frontend/src/components/landing/VietnamMap.tsx`:
- Around line 59-98: The city markers in VietnamMap are only interactive via
mouse hover, so keyboard users cannot change activeCity or access the city
details. Update the marker groups in VietnamMap to be focusable and
keyboard-operable by adding tabIndex, an appropriate accessibility role/label,
and an onFocus handler that mirrors onMouseEnter. Also add keyboard handling in
the same city marker rendering blocks so users can navigate and activate cities
without a mouse.
In `@finsight/frontend/src/context/IntelligenceContext.tsx`:
- Around line 132-147: The verified metric progress update in
IntelligenceContext currently parses event.message text for the count, which is
brittle. Update the synthesize event flow so the backend includes the verified
metric count in event.meta, then change the setSourcePreflight handling in
IntelligenceContext to read that structured meta field instead of matching
/extracted (\d+) verified metric/. Keep the existing synthesize stage check and
verifiedMetrics state update, but source the value from the typed event payload
rather than the message string.
In `@finsight/frontend/src/lib/api.ts`:
- Around line 109-132: The error parsing and message extraction logic in
runIntelligenceQuery is duplicated with runIntelligenceQueryStream, so extract
it into a shared helper and reuse it in both response handlers. Move the non-OK
response body parsing and fallback message construction into a single utility
near these functions, then have both runIntelligenceQuery and
runIntelligenceQueryStream call that helper before throwing the error.
- Around line 159-212: Add cancellation and stream cleanup to
runIntelligenceQueryStream: it currently has no AbortSignal support and never
releases the reader on early return or error. Update runIntelligenceQueryStream
to accept an AbortSignal and ensure the reader is cancelled or released in a
cleanup path when a result is returned, an error is thrown, or the request is
aborted. Then update IntelligenceContext.runQuery to keep an AbortController
ref, abort any previous in-flight query before starting a new one, and pass the
signal into runIntelligenceQueryStream.
In `@finsight/frontend/src/styles.css`:
- Line 4: Remove the unused shadcn theme setup from the styles file by deleting
the generic `:root` and `.dark` palette variables and the `@custom-variant dark`
declaration, since the app only relies on the `fs-*` tokens. Update the
stylesheet so only the active `fs-*` custom properties remain, and make sure no
`bg-primary`, `bg-card`, `bg-popover`, or `dark:` references depend on the
removed `dark` variant. Refer to the existing stylesheet theme block and
`@custom-variant dark` definition when cleaning this up.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf8bd6c9-a810-4179-bce8-971178a36395
⛔ Files ignored due to path filters (5)
finsight/frontend/package-lock.jsonis excluded by!**/package-lock.jsonfinsight/frontend/src/assets/hero.pngis excluded by!**/*.pngfinsight/frontend/src/assets/typescript.svgis excluded by!**/*.svgfinsight/frontend/src/assets/vite.svgis excluded by!**/*.svgfinsight/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (55)
README.mdfinsight/.env.templatefinsight/.gitignorefinsight/README.mdfinsight/api/__init__.pyfinsight/api/core/config.pyfinsight/api/core/logger.pyfinsight/api/index.pyfinsight/api/models/__init__.pyfinsight/api/models/schemas.pyfinsight/api/services/__init__.pyfinsight/api/services/agent_workflows.pyfinsight/api/services/data_quality.pyfinsight/api/services/llm_client.pyfinsight/api/services/pipeline.pyfinsight/api/services/prompts.pyfinsight/api/services/query_classifier.pyfinsight/api/services/query_planner.pyfinsight/api/services/source_ranker.pyfinsight/api/services/tinyfish_client.pyfinsight/frontend/.gitignorefinsight/frontend/index.htmlfinsight/frontend/package.jsonfinsight/frontend/src/App.tsxfinsight/frontend/src/components/BrandMark.tsxfinsight/frontend/src/components/MarkdownContent.tsxfinsight/frontend/src/components/SkylineHero.tsxfinsight/frontend/src/components/SkylineSilhouette.tsxfinsight/frontend/src/components/landing/AnalysisConsole.tsxfinsight/frontend/src/components/landing/Architecture.tsxfinsight/frontend/src/components/landing/ClosingCTA.tsxfinsight/frontend/src/components/landing/IntelligenceReport.tsxfinsight/frontend/src/components/landing/LivePipelinePanel.tsxfinsight/frontend/src/components/landing/MarketPulse.tsxfinsight/frontend/src/components/landing/OpsProblem.tsxfinsight/frontend/src/components/landing/QueryMatrix.tsxfinsight/frontend/src/components/landing/SkyHero.tsxfinsight/frontend/src/components/landing/SourceQualityMeter.tsxfinsight/frontend/src/components/landing/TinyFishPipeline.tsxfinsight/frontend/src/components/landing/VerticalPlaybooks.tsxfinsight/frontend/src/components/landing/VietnamMap.tsxfinsight/frontend/src/context/IntelligenceContext.tsxfinsight/frontend/src/counter.tsfinsight/frontend/src/lib/api.tsfinsight/frontend/src/main.tsfinsight/frontend/src/main.tsxfinsight/frontend/src/style.cssfinsight/frontend/src/styles.cssfinsight/frontend/tsconfig.jsonfinsight/frontend/vite.config.tsfinsight/package.jsonfinsight/pyproject.tomlfinsight/render.yamlfinsight/requirements.txtfinsight/vercel.json
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
finsight/api/index.py (1)
218-231: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOn a non-timeout failure,
asyncio.gatherleaves the sibling query running.The gating/timeout/concurrency rework correctly resolves the prior finding. One residual gap: with the default
return_exceptions=False, if one_ticker_queryraises a non-TimeoutErrorexception (e.g., an httpx/openai error propagated fromprocess_query),gatherre-raises immediately but does not cancel the still-pending sibling coroutine, so the other paid TinyFish/LLM workflow keeps running to completion in the background. Cancel outstanding tasks on failure to avoid the wasted spend.♻️ Suggested cleanup
try: - response_hcmc, response_hanoi = await asyncio.gather( - _ticker_query( - "Current average commercial real estate rent per square meter in Ho Chi Minh City District 1" - ), - _ticker_query( - "Current average commercial real estate rent per square meter in Hanoi Hoan Kiem District" - ), - ) + tasks = [ + asyncio.ensure_future( + _ticker_query( + "Current average commercial real estate rent per square meter in Ho Chi Minh City District 1" + ) + ), + asyncio.ensure_future( + _ticker_query( + "Current average commercial real estate rent per square meter in Hanoi Hoan Kiem District" + ) + ), + ] + try: + response_hcmc, response_hanoi = await asyncio.gather(*tasks) + except BaseException: + for t in tasks: + if not t.done(): + t.cancel() + await asyncio.gather(*tasks, return_exceptions=True) + raise except asyncio.TimeoutError as exc: raise HTTPException( status_code=504, detail="Market ticker data timed out.", ) from exc🤖 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 `@finsight/api/index.py` around lines 218 - 231, The concurrent ticker lookup in the `asyncio.gather` block can leave one `_ticker_query` running after the other fails with a non-timeout exception. Update the `finsight.api.index` flow around the `response_hcmc, response_hanoi = await asyncio.gather(...)` call so that any pending sibling task is explicitly canceled when an exception escapes, and make sure the `process_query`/TinyFish work is not left running in the background. Keep the existing timeout handling, but add cleanup for non-`TimeoutError` failures in the same `gather`-based code path.
🤖 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 `@finsight/api/index.py`:
- Around line 218-231: The concurrent ticker lookup in the `asyncio.gather`
block can leave one `_ticker_query` running after the other fails with a
non-timeout exception. Update the `finsight.api.index` flow around the
`response_hcmc, response_hanoi = await asyncio.gather(...)` call so that any
pending sibling task is explicitly canceled when an exception escapes, and make
sure the `process_query`/TinyFish work is not left running in the background.
Keep the existing timeout handling, but add cleanup for non-`TimeoutError`
failures in the same `gather`-based code path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0e63b6d4-0206-4af2-aadc-e2a0e0336a26
📒 Files selected for processing (5)
finsight/.env.templatefinsight/api/index.pyfinsight/api/services/tinyfish_client.pyfinsight/frontend/src/components/landing/AnalysisConsole.tsxfinsight/package.json
✅ Files skipped from review due to trivial changes (1)
- finsight/.env.template
🚧 Files skipped from review as they are similar to previous changes (3)
- finsight/package.json
- finsight/frontend/src/components/landing/AnalysisConsole.tsx
- finsight/api/services/tinyfish_client.py
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.
Summary
Summary by CodeRabbit
New Features
Documentation
Chores