feat: add source field suggestions#2436
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Greptile SummaryThis PR adds schema-aware field suggestions, live EXPLAIN-based expression validation, and a save-time pairing-warning modal to the source configuration form. The validation logic is extracted into a reusable
Confidence Score: 4/5The feature is additive and non-destructive — existing save paths are unchanged when no warnings are triggered, and the warning modal provides a cancel path so nothing is persisted without user intent. The new code is well-structured and the core suggestion/validation logic has solid unit-test coverage. The two flagged points — a silent key collision in the lowercase-name map and the untyped parsedData patch — are both low-probability paths today, but neither is a correctness guarantee. The rest of the change (hook extraction, component composition, modal flow) is straightforward and safe. sourceFieldSuggestions.ts (lowercase map collision) and SourceForm.tsx (pendingSave.parsedData: any) warrant a second look before extending the warning system with additional fix types. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User picks DB + Table] --> B[useColumns fetches schema]
B --> C{Field has value?}
C -- No --> D[inferSourceFieldCandidates\ntype + name match]
D --> E[SourceFieldCandidateHint\nshows canonical + alternates]
E -- click apply --> F[setValue via RHF]
C -- Yes --> G[ExpressionValidationStatus\ndebounced EXPLAIN query]
G --> H{Result?}
H -- valid --> I[green 'Expression is valid.']
H -- ClickHouseQueryError --> J[ErrorCollapse with message]
H -- loading / stale --> K[nothing shown]
F --> L[User clicks Save]
L --> M[handleSubmit + Zod parse]
M --> N[getSourceConfigPairingWarnings\nbody vs implicit-column check]
N -- warnings present --> O[setPendingSave\nshow Modal]
N -- no warnings --> P[persist parseResult.data]
O --> Q{User action}
Q -- Apply fix --> R[applyPairingFix\nsetValue + patch parsedData]
R -- last fix resolved --> P
Q -- Save anyway --> P
Q -- Cancel --> S[modal closed\nno save]
Reviews (1): Last reviewed commit: "feat: add source field suggestions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 2. case-insensitive name match, ranked by `canonicalNames` order | ||
| const byLowerName = new Map( | ||
| compatible.map(c => [c.name.toLowerCase(), c.name]), | ||
| ); |
There was a problem hiding this comment.
compatible.map(c => [c.name.toLowerCase(), c.name]) builds a Map from lowercased name → original name. If two type-compatible columns share the same lowercase name (e.g., a table containing both Body and BODY), the second entry in the array wins and the first column is silently dropped. The dropped column will never appear as a canonical match or an alternate, even though it exists in the table. ClickHouse column names are case-sensitive, so this is a realistic schema configuration. The result is a missed suggestion with no error or warning to the user.
| const [pendingSave, setPendingSave] = useState<{ | ||
| warnings: PairingWarning[]; | ||
| parsedData: any; | ||
| persist: (data: any) => void; | ||
| }>(); |
There was a problem hiding this comment.
parsedData makes the patch operation un-checkable
parsedData is typed as any, so the spread-patch in applyPairingFix — { ...pendingSave.parsedData, [field]: value } — bypasses TypeScript's schema checks entirely. Today the patched fields (bodyExpression, implicitColumnExpression) are direct top-level strings, so the manual patch is correct. But if a future warning's suggestedFix.field ever maps to a nested path (e.g., from a nested sub-schema), the patch silently creates a top-level key while leaving the nested field untouched, producing a malformed save payload with no compile-time error. Typing parsedData to match the parsed schema (e.g., z.infer<typeof SourceSchema>) would catch this class of mistake.
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!
E2E Test Results✅ All tests passed • 199 passed • 3 skipped • 1357s
Tests ran across 4 shards in parallel. |
1 participant
Summary
Why: Configuring a source against a custom (non-OTel) table means typing column names from memory into free-text inputs, with no schema inspection, no validation, and no warning when a config will silently break search (e.g.
bodyExpressionset butimplicitColumnExpressionempty — the HDX-4376 trap). This adds the form-side prevention to pair with HDX-4376 (runtime fallback) and HDX-4373 (row-panel cleanup).What changed:
useColumns) and suggests the likely column under each empty optional-expression field, with one-click apply. Type-aware, name-matched, resolves per source kind (logs vs traces). Suggests, never pre-fills; lists all candidates when ambiguous.EXPLAINquery, showing valid/invalid inline.getSourceConfigPairingWarningsflags body/implicit-column mismatches (log sources only) in a non-blocking save-time modal with one-click fixes or "Save anyway".Screenshots or video
Untitled2.mov
How to test on Vercel preview
Preview routes:
/teamSteps:
References