release: v0.9.1 — Stability (architecture + code quality) - #203
Merged
Conversation
Extract intermediate `ws = widget.settings ?? {}` in card-container (9
drilldowns → 1), resolve-click-action, collect-parameter-names, and
dashboard-container. Fix unsafe `(X as string)?.trim()` casts in
widget-editor-modal and parameter-config-section with safe `String(X ?? "")`.
Closes #195
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pluggability audit — moves scattered chart-type metadata into ChartConfig: - isECharts: replaces hardcoded ECHARTS_TYPES Set in capture-preview.ts - supportsColumnMapping: replaces MAPPING_SUPPORTED_TYPES Set in card-container.tsx - stylingTargets: replaces getStylingTargets() switch statement (17 cases → 1 lookup) - requiresQuery: marks param-select, form, markdown, iframe as query-free - getChartTypeMeta(): derives labels from registry, keeps icons in UI layer (replaces duplicate chartTypeMeta object) Adding a new chart type now touches 3 files instead of 5-6: 1. chart-registry.ts (type + config with all metadata) 2. chart-renderer.tsx (component + prop forwarding) 3. chart-type-selector.tsx (icon only) Closes #196 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Foundation for eliminating prop drilling in the widget editor. - New `widget-editor-store.ts` with all 24 state variables from WidgetEditorModal, grouped by domain (identity, chart options, click actions, styling, parameters, form, lab mode, UI state) - Bulk operations: `resetForAdd()`, `loadFromWidget()` with full widget settings extraction (click action, styling, legacy migration, param widget state) - Build helpers: `buildStylingConfig()`, `buildClickAction()` - 21 unit tests covering initial state, setters, bulk ops, builders - `getChartDefaults()` in chart-registry to avoid component barrel import - Fix schema re-export to use `@neoboard/components/charts` path Next: wire sub-editors to consume from store instead of props. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
First sub-editor migrated to store-based state management: - StylingRulesEditor: 7 props → 1 (onBack only) - Reads rules, chartType, availableFields, parameterSuggestions from store - Bidirectional sync between modal's local state and store - Fix accordion auto-expand: optimistically expand new items in addItem() to handle async store→component re-render timing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Second and third sub-editors migrated to store-based state: - ActionRulesEditor: 7 props → 2 (onBack, pages) - FormFieldsEditor: 3 props → 0 (reads fields/onChange from store) - Added formFields and actionRules to bidirectional sync Sub-editor prop reduction so far: StylingRulesEditor: 7 → 1 ActionRulesEditor: 7 → 2 FormFieldsEditor: 3 → 0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Complete sub-editor migration to store-based state management: StylingRulesEditor: 7 props → 1 (onBack) ActionRulesEditor: 7 props → 2 (onBack, pages) FormFieldsEditor: 3 props → 0 QueryEditorPanel: 7 props → 3 (onRun, editorLanguage, running) ParameterConfigSection: 13 props → 2 (seedQueryExecution, seedPreviewOptions) Total: 37 props eliminated across 5 sub-editors. Bidirectional sync handles modal local state ↔ store for all migrated fields. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace 12 flat props with 3 grouped sub-objects:
- styling: { rules, paramValues, colorScales }
- interaction: { onChartClick, clickableColumns }
- meta: { connectionId, widgetId, resultId, query, autoFit }
ChartRendererProps: 12 → 6 (type, data, settings + 3 groups)
Adding new chart-level props now means extending a group, not
touching every CardContainer call site.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Layer 2 — ChartRenderer context grouping:
ChartRendererProps: 12 flat props → 6 (type, data, settings + 3 groups)
- styling: { rules, paramValues, colorScales }
- interaction: { onChartClick, clickableColumns }
- meta: { connectionId, widgetId, resultId, query, autoFit }
Layer 3 — DashboardContainer actions grouping:
DashboardContainerProps: 13 → 6 (page, editable, actions, refetchInterval,
templateMap, showParameterBar)
- WidgetActions interface groups 9 callback props into one object
Combined with Layer 1 (widget-editor store), total props eliminated:
Sub-editors: 37 → 8 (29 eliminated)
ChartRenderer: 12 → 6
DashboardContainer: 13 → 6
Grand total: 62 → 20 (42 props eliminated)
Closes #195
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace duplicate operator definitions across component/ and app/: - New OPERATOR_REGISTRY array in styling-rule.ts with value, label, group - getOperatorGroups() function for editor UI dropdown - NUMERIC_OPS/STRING_OPS/NULL_OPS derived from registry (not hardcoded) - StylingRulesEditor reads from registry instead of own OPERATOR_GROUPS Adding a new operator: add 1 entry to OPERATOR_REGISTRY + implement in evaluateNumeric/evaluateString. No more duplicate definitions. Closes #197 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Standardized error taxonomy across all database connectors: - ConnectorError class with type enum (TIMEOUT, AUTHENTICATION, CONNECTION, READ_ONLY_VIOLATION, QUERY, UNKNOWN) - detectNeo4jErrorType() — categorizes Neo4j errors by code/message - detectPostgresErrorType() — categorizes PostgreSQL errors by code - wrapError() — wraps raw DB errors into ConnectorError with auto-detection - 14 unit tests covering all error types for both connectors Consumers can now catch ConnectorError and switch on .type instead of parsing database-specific error codes/messages. Closes #198 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughAdds a centralized Zustand widget-editor store and migrates many widget-editor subcomponents to it; groups DashboardContainer widget callbacks into a single Changes
Sequence Diagram(s)sequenceDiagram
participant Modal as WidgetEditorModal
participant Store as useWidgetEditorStore
participant Sub as Sub-Editor (Styling/Action/Query/Param)
Modal->>Store: loadFromWidget(widget)
Store-->>Store: populate state (chartType, query, settings, rules, styling)
Modal->>Sub: render (minimal props: onBack, pages...)
Sub->>Store: read state via useWidgetEditorStore()
Sub->>Store: call setters (setQuery, setStylingRules, setActionRules, ...)
Store-->>Sub: subscribers notified — re-render
Modal->>Store: buildStylingConfig()
Store-->>Modal: StylingConfig | undefined
Modal->>Store: buildClickAction(layout?)
Store-->>Modal: ClickAction | undefined
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
…erer CI type-check caught query prop outside the grouped meta object. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
connection/__tests__/connector-error.test.ts (2)
96-103: Add a message assertion for Neo4j object-wrapping.This test should also assert
wrapped.messageto prevent regressions where object errors are stringified to"[object Object]".Proposed test addition
it("wraps Neo4j error correctly", () => { const raw = { code: "ServiceUnavailable", message: "Failed to connect" }; const wrapped = wrapError(raw, "neo4j"); expect(wrapped).toBeInstanceOf(ConnectorError); expect(wrapped.type).toBe(ConnectorErrorType.CONNECTION); expect(wrapped.originalError).toBe(raw); + expect(wrapped.message).toBe("Failed to connect"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/__tests__/connector-error.test.ts` around lines 96 - 103, Add an assertion in the "wraps Neo4j error correctly" test to verify the wrapped.message is not a stringified object; specifically, after calling wrapError(raw, "neo4j") and the existing assertions on ConnectorError and ConnectorErrorType, assert that wrapped.message contains the raw.message (and optionally raw.code) so regressions to "[object Object]" are caught — update the test around the wrapError call in connector-error.test.ts to include expect(wrapped.message).toContain(raw.message) (and/or expect(wrapped.message).toContain(raw.code)).
1-7: Move this test beside the source module.Please place this in
connection/src/generalized/__tests__/connector-error.test.tsso tests stay adjacent to the unit under test.As per coding guidelines: "Tests live in
__tests__/next to the file under test, same package".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connection/__tests__/connector-error.test.ts` around lines 1 - 7, Move the test file into the package-local test folder so it sits next to the module: relocate connection/__tests__/connector-error.test.ts to connection/src/generalized/__tests__/connector-error.test.ts; after moving, update the import path that currently points to "../src/generalized/ConnectorError" to the module-local relative path "../ConnectorError" so the imports (ConnectorError, ConnectorErrorType, detectNeo4jErrorType, detectPostgresErrorType, wrapError) resolve correctly from the new location and the test lives in __tests__/ next to the source.app/src/components/card-container.tsx (1)
39-42: Make the mapping gate a type guard, not a loose boolean.
ChartConfig.supportsColumnMappingis optional, but the overlay path still narrows with a downstream"bar" | "line" | "pie"cast. WithsupportsColumnMapping(type: string), a registry entry can accidentally opt an unsupported chart into the overlay without the type checker catching it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/card-container.tsx` around lines 39 - 42, The helper supportsColumnMapping should be a TypeScript type guard so the downstream cast to the overlay-specific chart union is safe: change the signature of supportsColumnMapping(type: string) to a type predicate (e.g. type is ChartTypeWithColumnMapping or type is "bar" | "line" | "pie") and keep the runtime check using getChartConfig(type)?.supportsColumnMapping === true; update any call sites that rely on the narrowed type so the compiler understands the guard, and reference the ChartConfig.supportsColumnMapping property, getChartConfig, and the supportsColumnMapping function in your changes.app/src/stores/widget-editor-store.ts (1)
19-44: Extract the parameter-type mapping into a shared helper.
ParamUIType,DateSubType, andreverseParamTypeMapping()are now duplicated here and inapp/src/components/widget-editor/parameter-config-section.tsx. The next parameter-type change will have to land in both places or widgets will deserialize and reserialize differently. A small non-UI helper underapp/src/lib/would remove that drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/stores/widget-editor-store.ts` around lines 19 - 44, Extract the duplicated types and function into a non-UI helper module (e.g., under app/src/lib/) and import it from both widget-editor-store and parameter-config-section to avoid drift: move ParamUIType, DateSubType, and reverseParamTypeMapping into the new shared file, export them, then replace the local declarations in widget-editor-store (and remove the duplicates in parameter-config-section.tsx) with imports of those exported symbols so both places use the single canonical mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/dashboard-container.tsx`:
- Around line 56-60: DashboardContainer lost a top-level onNavigateToPage prop
so CardContainer only gets actions?.onNavigateToPage and view-mode callers that
don't pass actions break navigation; restore onNavigateToPage into
DashboardContainerProps (add onNavigateToPage?: (page: DashboardPage) => void),
accept and forward that prop from DashboardContainer to CardContainer (use
onNavigateToPage prop as fallback when actions?.onNavigateToPage is undefined),
and ensure the caller that renders DashboardContainer without actions continues
to work without changing callers.
In `@app/src/components/widget-editor-modal.tsx`:
- Around line 813-875: The store-seeding calls currently run in passive
useEffect hooks causing a one-render staleness for sub-editors; replace those
initial useEffect calls that call useWidgetEditorStore.setState (the three small
ones and the larger stylingRules/actionRules/... sync) with useLayoutEffect so
the store is updated synchronously before paint, keep the existing
syncingFromStore ref logic to avoid feedback loops, and ensure any places that
update the modal snapshot (where chartType, connectionId, query,
availableFields, parameterSuggestions, stylingRules, etc. are set) call
useWidgetEditorStore.setState synchronously (or migrate to making
useWidgetEditorStore the single source of truth) so
QueryEditorPanel/useConnectionSchema observe the updated values immediately.
In `@app/src/components/widget-editor/styling-rules-editor.tsx`:
- Around line 47-56: Replace the local NULL_OPS/STRING_OPS-driven logic in
inputType and the single-value path with metadata from the shared operator
registry returned by getOperatorGroups()/OPERATOR_GROUPS: look up the chosen
operator's metadata (its declared input kind/type) and base rendering (text vs
number vs null vs between) on that instead of hardcoded sets; remove
NULL_OPS/STRING_OPS usage, ensure equality operators (==, !=) use the operator
metadata so string equality renders text inputs when supported by
component/src/charts/styling-rule.ts, and make the "between" control pass the
explicit numeric inputType in the single-value code path when the operator
metadata requires numeric inputs.
In `@app/src/components/widget-editor/use-accordion-crud.ts`:
- Around line 59-61: The new accordion id is being opened optimistically in
addItem() but prevIdsRef.current isn't updated until items prop changes, so
computeOpenItems(prevIdsRef.current, currentIds, prev) sees the id as newly
added and returns it twice; fix by updating prevIdsRef.current immediately when
you optimistically open the new item (e.g., append newItem.id to
prevIdsRef.current) before calling setOpenItems, or alternatively ensure
computeOpenItems deduplicates ids, so references to setOpenItems,
computeOpenItems, prevIdsRef, and addItem are adjusted accordingly to prevent
the duplicate id.
In `@app/src/stores/widget-editor-store.ts`:
- Around line 325-369: The advanced-rules branch in buildClickAction currently
returns s.actionRules without validation; update buildClickAction so that before
returning when s.actionRules.length > 0 it iterates over s.actionRules and
validates each rule the same way the legacy branch does: for rules with type
"set-parameter" or "set-parameter-and-navigate" ensure parameterName.trim() is
non-empty and sourceField defaults to parameterName; for rules with type
"navigate-to-page" or "set-parameter-and-navigate" ensure targetPageId is
present and is included in (layout?.pages ?? []).map(p => p.id); also ensure
clickableColumns is set only if non-empty (otherwise undefined); if any rule
fails validation return undefined, otherwise return the validated rules object
(type from first rule, rules array, and clickableColumns as before).
In `@connection/src/generalized/ConnectorError.ts`:
- Around line 107-108: The wrapError helper currently converts non-Error inputs
to a string with String(err), which yields "[object Object]" for object-shaped
errors and loses any message property; update wrapError (in ConnectorError.ts)
to detect non-Error objects and, if they have a truthy string-valued message
property (e.g., err && typeof err === "object" && typeof err.message ===
"string"), use that message, otherwise fall back to JSON.stringify(err) (with
safe fallback to String(err)) before constructing new ConnectorError(message,
type, err).
- Around line 1-4: Errors thrown by drivers are being passed through unwrapped;
call the existing wrapError helper wherever errors are caught and before
invoking callbacks.onFail or rethrowing so consumers always receive
ConnectorError. Specifically, update the catch blocks that currently call
callbacks.onFail with raw errors (the PostgreSQL error handler and the Neo4j
error handler) to pass wrapError(error) instead, and modify
PostgresAuthenticationModule and Neo4jConnectionModule catch/rethrow sites
(e.g., the methods that currently rethrow driver errors) to throw
wrapError(error) or throw a new ConnectorError created by wrapError (preserving
the original as the cause if supported). Ensure every catch path that surfaces
errors to callers or callbacks uses wrapError so the contract is upheld.
- Around line 77-79: The mapping in ConnectorError.ts wrongly classifies
Postgres error code "3D000" (invalid_catalog_name) as AUTHENTICATION; remove
"3D000" from the array checked for ConnectorErrorType.AUTHENTICATION (the code
that checks if (["28P01", "28000", "28001", "3D000"].includes(code)) and returns
ConnectorErrorType.AUTHENTICATION) and instead include "3D000" in the set/branch
that returns ConnectorErrorType.CONNECTION so that ConnectorErrorType reflects a
missing/invalid database rather than an auth failure; update any related tests
or comments referencing this mapping.
---
Nitpick comments:
In `@app/src/components/card-container.tsx`:
- Around line 39-42: The helper supportsColumnMapping should be a TypeScript
type guard so the downstream cast to the overlay-specific chart union is safe:
change the signature of supportsColumnMapping(type: string) to a type predicate
(e.g. type is ChartTypeWithColumnMapping or type is "bar" | "line" | "pie") and
keep the runtime check using getChartConfig(type)?.supportsColumnMapping ===
true; update any call sites that rely on the narrowed type so the compiler
understands the guard, and reference the ChartConfig.supportsColumnMapping
property, getChartConfig, and the supportsColumnMapping function in your
changes.
In `@app/src/stores/widget-editor-store.ts`:
- Around line 19-44: Extract the duplicated types and function into a non-UI
helper module (e.g., under app/src/lib/) and import it from both
widget-editor-store and parameter-config-section to avoid drift: move
ParamUIType, DateSubType, and reverseParamTypeMapping into the new shared file,
export them, then replace the local declarations in widget-editor-store (and
remove the duplicates in parameter-config-section.tsx) with imports of those
exported symbols so both places use the single canonical mapping.
In `@connection/__tests__/connector-error.test.ts`:
- Around line 96-103: Add an assertion in the "wraps Neo4j error correctly" test
to verify the wrapped.message is not a stringified object; specifically, after
calling wrapError(raw, "neo4j") and the existing assertions on ConnectorError
and ConnectorErrorType, assert that wrapped.message contains the raw.message
(and optionally raw.code) so regressions to "[object Object]" are caught —
update the test around the wrapError call in connector-error.test.ts to include
expect(wrapped.message).toContain(raw.message) (and/or
expect(wrapped.message).toContain(raw.code)).
- Around line 1-7: Move the test file into the package-local test folder so it
sits next to the module: relocate connection/__tests__/connector-error.test.ts
to connection/src/generalized/__tests__/connector-error.test.ts; after moving,
update the import path that currently points to
"../src/generalized/ConnectorError" to the module-local relative path
"../ConnectorError" so the imports (ConnectorError, ConnectorErrorType,
detectNeo4jErrorType, detectPostgresErrorType, wrapError) resolve correctly from
the new location and the test lives in __tests__/ next to the source.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 918dc55f-ae1c-4880-ab10-41ce9da74149
📒 Files selected for processing (24)
app/src/app/(dashboard)/[id]/edit/page.tsxapp/src/app/(dashboard)/[id]/page.tsxapp/src/components/card-container.tsxapp/src/components/chart-renderer.tsxapp/src/components/dashboard-container.tsxapp/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/action-rules-editor.tsxapp/src/components/widget-editor/chart-type-selector.tsxapp/src/components/widget-editor/form-fields-editor.tsxapp/src/components/widget-editor/parameter-config-section.tsxapp/src/components/widget-editor/query-editor-panel.tsxapp/src/components/widget-editor/styling-rules-editor.tsxapp/src/components/widget-editor/use-accordion-crud.tsapp/src/lib/capture-preview.tsapp/src/lib/chart-registry.tsapp/src/lib/collect-parameter-names.tsapp/src/lib/db/schema.tsapp/src/lib/resolve-click-action.tsapp/src/stores/__tests__/widget-editor-store.test.tsapp/src/stores/widget-editor-store.tscomponent/src/charts/index.tscomponent/src/charts/styling-rule.tsconnection/__tests__/connector-error.test.tsconnection/src/generalized/ConnectorError.ts
Comment on lines
+56
to
+60
| interface DashboardContainerProps { | ||
| page: DashboardPage; | ||
| editable?: boolean; | ||
| actions?: WidgetActions; | ||
| refetchInterval?: number | false; |
There was a problem hiding this comment.
Page navigation regressed for the view-mode dashboard.
CardContainer only receives onNavigateToPage from actions?.onNavigateToPage now. The caller in app/src/app/(dashboard)/[id]/page.tsx (Lines 1-50 in the provided snippet) still renders DashboardContainer without an actions object, so page-navigation click actions stop working in normal view mode. Keep this callback top-level, or update that caller before release.
Also applies to: 79-89, 277-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/dashboard-container.tsx` around lines 56 - 60,
DashboardContainer lost a top-level onNavigateToPage prop so CardContainer only
gets actions?.onNavigateToPage and view-mode callers that don't pass actions
break navigation; restore onNavigateToPage into DashboardContainerProps (add
onNavigateToPage?: (page: DashboardPage) => void), accept and forward that prop
from DashboardContainer to CardContainer (use onNavigateToPage prop as fallback
when actions?.onNavigateToPage is undefined), and ensure the caller that renders
DashboardContainer without actions continues to work without changing callers.
Comment on lines
+47
to
+56
| // Operator groups derived from the shared registry (single source of truth) | ||
| const OPERATOR_GROUPS = getOperatorGroups(); | ||
|
|
||
| const NULL_OPS = new Set<StylingOperator>(["is_null", "is_not_null"]); | ||
| const STRING_OPS = new Set<StylingOperator>(["contains", "not_contains", "starts_with", "ends_with"]); | ||
| const STRING_OPS = new Set<StylingOperator>([ | ||
| "contains", | ||
| "not_contains", | ||
| "starts_with", | ||
| "ends_with", | ||
| ]); |
There was a problem hiding this comment.
The operator UI still renders the wrong input for some valid rules.
inputType only treats contains/not_contains/starts_with/ends_with as text, so == and != still fall through to numeric inputs even though component/src/charts/styling-rule.ts supports string equality for those operators. The new between controls also stop passing the explicit numeric input type used in the single-value path. Please drive editor behavior from shared operator metadata instead of the local NULL_OPS/STRING_OPS sets.
Also applies to: 116-117, 199-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/widget-editor/styling-rules-editor.tsx` around lines 47 -
56, Replace the local NULL_OPS/STRING_OPS-driven logic in inputType and the
single-value path with metadata from the shared operator registry returned by
getOperatorGroups()/OPERATOR_GROUPS: look up the chosen operator's metadata (its
declared input kind/type) and base rendering (text vs number vs null vs between)
on that instead of hardcoded sets; remove NULL_OPS/STRING_OPS usage, ensure
equality operators (==, !=) use the operator metadata so string equality renders
text inputs when supported by component/src/charts/styling-rule.ts, and make the
"between" control pass the explicit numeric inputType in the single-value code
path when the operator metadata requires numeric inputs.
Comment on lines
+1
to
+4
| /** | ||
| * Standardized error types across all database connectors. | ||
| * Consumers catch ConnectorError instead of database-specific exceptions. | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expected:
# 1) `wrapError(` should appear in connector catch/rethrow paths.
# 2) onFail/throw should not pass raw `error` when wrapping is expected.
rg -n -C3 '\bwrapError\s*\(' connection/src
rg -n -C3 'callbacks\.onFail\?\.\((err|error)\)' connection/src/neo4j/Neo4jConnectionModule.ts connection/src/postgresql/PostgresConnectionModule.ts
rg -n -C3 'throw error;' connection/src/postgresql/PostgresAuthenticationModule.ts connection/src/neo4j/Neo4jConnectionModule.tsRepository: alfredo1996/neoboard
Length of output: 3513
Wrap database errors with ConnectorError in catch paths.
wrapError is defined but not invoked where errors are caught. callbacks.onFail receives raw driver errors (PostgreSQL line 171, Neo4j line 112), and authentication/connection checks rethrow unwrapped errors (PostgresAuthenticationModule.ts:72, :111; Neo4jConnectionModule.ts:136). This breaks the stated contract that consumers catch ConnectorError.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/src/generalized/ConnectorError.ts` around lines 1 - 4, Errors
thrown by drivers are being passed through unwrapped; call the existing
wrapError helper wherever errors are caught and before invoking callbacks.onFail
or rethrowing so consumers always receive ConnectorError. Specifically, update
the catch blocks that currently call callbacks.onFail with raw errors (the
PostgreSQL error handler and the Neo4j error handler) to pass wrapError(error)
instead, and modify PostgresAuthenticationModule and Neo4jConnectionModule
catch/rethrow sites (e.g., the methods that currently rethrow driver errors) to
throw wrapError(error) or throw a new ConnectorError created by wrapError
(preserving the original as the cause if supported). Ensure every catch path
that surfaces errors to callers or callbacks uses wrapError so the contract is
upheld.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/components/card-container.tsx (2)
191-193: Avoid the boolean-plus-cast split for overlay-supported types.
supportsColumnMapping()now says only “some registry entry opted in”, but both overlay call sites still castchartConfig.typeto"bar" | "line" | "pie". The first new registry entry withsupportsColumnMapping: truewill bypass type safety here and feed an invalidchartTypeintoColumnMappingOverlay.Also applies to: 280-280, 503-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/card-container.tsx` around lines 191 - 193, The current check uses supportsColumnMapping(widget.chartType) and then force-casts chartConfig.type to "bar" | "line" | "pie" before passing to ColumnMappingOverlay, which breaks type safety for new registry entries; update the guard so the value is actually narrowed to the overlay-supported union — either (A) change supportsColumnMapping to perform a type-narrowing check (a user-defined type guard) so calling code can safely treat chartConfig.type as "bar"|"line"|"pie", or (B) add an explicit runtime check against the allowed set (e.g., chartConfig.type === "bar" || "line" || "pie") before casting; apply the same fix at the other two call sites and use the symbols supportsColumnMapping, chartConfig.type, ColumnMappingOverlay, and onWidgetSettingsChange to locate and update the checks.
133-170: Derive query gating from registry metadata too.This branch still hardcodes the non-query widget set. If
chart-registryis now the source of truth forrequiresQuery, the nextrequiresQuery: falsewidget will still executeuseWidgetQueryuntil this file is updated by hand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/card-container.tsx` around lines 133 - 170, The gating for whether to build queryInput is currently hardcoded via isParameterWidget/isFormWidget/isContentOnly; replace that with the chart registry metadata (e.g., use the registry lookup like getChartRegistryEntry(widget.chartType) or ChartRegistry[widget.chartType] and its requiresQuery flag) so widgets with requiresQuery: false no longer run useWidgetQuery; update the queryInput expression to check previewData || !requiresQuery (instead of the three chartType booleans) and remove or derive isParameterWidget/isFormWidget/isContentOnly from the registry-based requiresQuery where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/components/card-container.tsx`:
- Around line 191-193: The current check uses
supportsColumnMapping(widget.chartType) and then force-casts chartConfig.type to
"bar" | "line" | "pie" before passing to ColumnMappingOverlay, which breaks type
safety for new registry entries; update the guard so the value is actually
narrowed to the overlay-supported union — either (A) change
supportsColumnMapping to perform a type-narrowing check (a user-defined type
guard) so calling code can safely treat chartConfig.type as "bar"|"line"|"pie",
or (B) add an explicit runtime check against the allowed set (e.g.,
chartConfig.type === "bar" || "line" || "pie") before casting; apply the same
fix at the other two call sites and use the symbols supportsColumnMapping,
chartConfig.type, ColumnMappingOverlay, and onWidgetSettingsChange to locate and
update the checks.
- Around line 133-170: The gating for whether to build queryInput is currently
hardcoded via isParameterWidget/isFormWidget/isContentOnly; replace that with
the chart registry metadata (e.g., use the registry lookup like
getChartRegistryEntry(widget.chartType) or ChartRegistry[widget.chartType] and
its requiresQuery flag) so widgets with requiresQuery: false no longer run
useWidgetQuery; update the queryInput expression to check previewData ||
!requiresQuery (instead of the three chartType booleans) and remove or derive
isParameterWidget/isFormWidget/isContentOnly from the registry-based
requiresQuery where needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68a48029-c0bb-4c78-8154-0d31397a4749
📒 Files selected for processing (1)
app/src/components/card-container.tsx
Root cause: typeInEditor relied on CM6's internal `cmTile` property to access the EditorView. This property is mangled/inaccessible in production builds, causing the dispatch to fall through to the keyboard fallback, which also fails because CodeMirror's virtual DOM doesn't reliably handle insertText. Fix: - QueryEditor now exposes `__cmView` on the container DOM element after initialization (alongside the existing `data-editor-ready` attribute) - typeInEditor reads `__cmView` directly — no internal property traversal - Post-dispatch stability check also uses `__cmView` - Also fixed stray `query` prop on form widget ChartRenderer (CI type error) This eliminates the "Keyboard fallback: text not inserted" error that appeared in every CI run as 1-6 flaky tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/e2e/fixtures.ts (1)
122-139: Give the__cmViewcontract a real type here too.Line 125 and Line 149 use
as anyfor a helper that now depends onstate.readOnly,dispatch, anddoc.length. A small localCodeMirrorContainertype keeps that DOM hook explicit and lets strict mode catch drift.As per coding guidelines, "
**/*.{ts,tsx}: TypeScript strict mode. Noanywithout a comment explaining why.`"Also applies to: 146-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/e2e/fixtures.ts` around lines 122 - 139, The inline callback passed to cmContainer.evaluate uses (el as any).__cmView and relies on properties/methods like state.readOnly, state.doc.length and dispatch; replace the ad-hoc any with a small local interface (e.g. CodeMirrorContainer) that declares the hook shape (state: { readOnly: boolean; doc: { length: number; toString(): string } }, dispatch: (tr: any) => void) and cast to that type instead of any where __cmView is read in the evaluate callback and the similar helper at lines 146-154 so TypeScript strict mode can validate those members.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@component/src/components/composed/query-editor.tsx`:
- Around line 296-300: The teardown for the editor is leaving a stale DOM
property and attribute; update destroyEditor() to remove the exposed test handle
and readiness flag by deleting containerRef.current.__cmView (use the same any
cast pattern as in initEditor for containerRef) and removing the
"data-editor-ready" attribute on containerRef.current when tearing down the
EditorView referenced by viewRef.current; ensure you check containerRef.current
exists before clearing both the property and attribute to avoid null access.
---
Nitpick comments:
In `@app/e2e/fixtures.ts`:
- Around line 122-139: The inline callback passed to cmContainer.evaluate uses
(el as any).__cmView and relies on properties/methods like state.readOnly,
state.doc.length and dispatch; replace the ad-hoc any with a small local
interface (e.g. CodeMirrorContainer) that declares the hook shape (state: {
readOnly: boolean; doc: { length: number; toString(): string } }, dispatch: (tr:
any) => void) and cast to that type instead of any where __cmView is read in the
evaluate callback and the similar helper at lines 146-154 so TypeScript strict
mode can validate those members.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23681174-d9f3-4d13-b0b5-f77cabccc22f
📒 Files selected for processing (2)
app/e2e/fixtures.tscomponent/src/components/composed/query-editor.tsx
- useLayoutEffect for store sync (prevents one-render stale data) - Accordion addItem: prevent double-open by updating prevIdsRef eagerly - Advanced action rules: validate parameterName + targetPageId before persisting - ConnectorError: reclassify 3D000 as CONNECTION (not AUTHENTICATION) - ConnectorError: extract message from object-shaped errors (not [object Object]) - QueryEditor: clear __cmView on teardown to prevent stale E2E handles Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/stores/widget-editor-store.ts (1)
380-381: Prefer consistent data structure for page ID lookups.Advanced-rules mode uses
Set.has()(line 344) while legacy mode usesArray.includes()(line 381). Consider using aSetin both branches for consistency and O(1) lookup.♻️ Suggested diff
- const validPageIds = (layout?.pages ?? []).map((p) => p.id); - if (!validPageIds.includes(targetPageId)) return undefined; + const validPageIds = new Set((layout?.pages ?? []).map((p) => p.id)); + if (!validPageIds.has(targetPageId)) return undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/stores/widget-editor-store.ts` around lines 380 - 381, The code builds validPageIds as an array and uses Array.includes for lookup, causing inconsistency with the earlier advanced-rules branch that uses a Set and Set.has; change validPageIds to a Set of IDs (e.g., new Set((layout?.pages ?? []).map(p => p.id))) and replace the includes check with validPageIds.has(targetPageId) so both branches use O(1) Set lookups; update any variable name if helpful (e.g., validPageIdSet) to reflect the type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@component/src/components/composed/query-editor.tsx`:
- Around line 228-229: Add explicit rationale comments for the
`@typescript-eslint/no-explicit-any` suppressions used when attaching E2E
handles to the DOM: for the instance where you delete (containerRef.current as
any).__cmView (referencing containerRef and __cmView) and the other suppression
around the non-standard E2E handle later in this file; place a one-line comment
immediately above each `// eslint-disable-next-line
`@typescript-eslint/no-explicit-any`` that explains that `__cmView` (and the other
handle) are non-standard test-only properties added to the DOM for E2E testing
and cannot be strongly typed, so `any` is required to access/delete them.
In `@connection/__tests__/connector-error.test.ts`:
- Around line 1-7: Test file is located centrally and should be colocated with
the module under test; move the test from
connection/__tests__/connector-error.test.ts into the same package as the
ConnectorError module under a module-local __tests__ directory. Update the
import path to reference the local source (e.g., import from "./ConnectorError"
or "../ConnectorError" depending on module layout) so tests exercise
ConnectorError, ConnectorErrorType, detectNeo4jErrorType,
detectPostgresErrorType, and wrapError directly alongside the implementation.
- Around line 101-116: The current unit tests only exercise wrapError directly;
add module-level tests that trigger the catch paths in Neo4jConnectionModule and
PostgresConnectionModule and assert that callbacks.onFail receives a
ConnectorError (not a raw Error). Specifically, write tests that instantiate or
mock Neo4jConnectionModule and PostgresConnectionModule, cause their operation
to throw (e.g., mock the driver/session/query to throw a connection/timeout
error), spy/mock callbacks.onFail, and assert the argument is an instance of
ConnectorError with the expected ConnectorErrorType; reference wrapError,
callbacks.onFail, Neo4jConnectionModule, and PostgresConnectionModule to locate
the code paths to test.
---
Nitpick comments:
In `@app/src/stores/widget-editor-store.ts`:
- Around line 380-381: The code builds validPageIds as an array and uses
Array.includes for lookup, causing inconsistency with the earlier advanced-rules
branch that uses a Set and Set.has; change validPageIds to a Set of IDs (e.g.,
new Set((layout?.pages ?? []).map(p => p.id))) and replace the includes check
with validPageIds.has(targetPageId) so both branches use O(1) Set lookups;
update any variable name if helpful (e.g., validPageIdSet) to reflect the type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffaad70f-2938-4364-87cf-2d635784a72f
📒 Files selected for processing (6)
app/src/components/widget-editor-modal.tsxapp/src/components/widget-editor/use-accordion-crud.tsapp/src/stores/widget-editor-store.tscomponent/src/components/composed/query-editor.tsxconnection/__tests__/connector-error.test.tsconnection/src/generalized/ConnectorError.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/components/widget-editor/use-accordion-crud.ts
- connection/src/generalized/ConnectorError.ts
Comment on lines
+228
to
+229
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| delete (containerRef.current as any).__cmView; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no-explicit-any suppressions and nearby rationale text
rg -n -C1 'eslint-disable-next-line `@typescript-eslint/no-explicit-any`' component/src/components/composed/query-editor.tsxRepository: alfredo1996/neoboard
Length of output: 1980
Add explicit rationale to the no-explicit-any suppressions.
Lines 228–229 and 300–302 are missing rationale comments. Per TypeScript strict-mode guidance, all no-explicit-any suppressions require an explicit comment explaining why. These both involve non-standard E2E handles on the DOM element:
Proposed fix
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
+ // eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- non-standard E2E handle on HTMLElement
delete (containerRef.current as any).__cmView;
...
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any`
+ // eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- non-standard E2E handle on HTMLElement
if (containerRef.current)
(containerRef.current as any).__cmView = viewRef.current;📝 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.
Suggested change
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| delete (containerRef.current as any).__cmView; | |
| // eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- non-standard E2E handle on HTMLElement | |
| delete (containerRef.current as any).__cmView; |
Suggested change
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| delete (containerRef.current as any).__cmView; | |
| // eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- non-standard E2E handle on HTMLElement | |
| if (containerRef.current) | |
| (containerRef.current as any).__cmView = viewRef.current; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/components/composed/query-editor.tsx` around lines 228 - 229,
Add explicit rationale comments for the `@typescript-eslint/no-explicit-any`
suppressions used when attaching E2E handles to the DOM: for the instance where
you delete (containerRef.current as any).__cmView (referencing containerRef and
__cmView) and the other suppression around the non-standard E2E handle later in
this file; place a one-line comment immediately above each `//
eslint-disable-next-line `@typescript-eslint/no-explicit-any`` that explains that
`__cmView` (and the other handle) are non-standard test-only properties added to
the DOM for E2E testing and cannot be strongly typed, so `any` is required to
access/delete them.
Comment on lines
+1
to
+7
| import { | ||
| ConnectorError, | ||
| ConnectorErrorType, | ||
| detectNeo4jErrorType, | ||
| detectPostgresErrorType, | ||
| wrapError, | ||
| } from "../src/generalized/ConnectorError"; |
There was a problem hiding this comment.
Place this test next to the source module per test-location convention.
Line 7 (../src/generalized/ConnectorError) indicates this test is centralized rather than colocated with the file under test. Please move it under the module-local __tests__/ directory for ConnectorError.
As per coding guidelines: "Tests live in __tests__/ next to the file under test, same package".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/__tests__/connector-error.test.ts` around lines 1 - 7, Test file
is located centrally and should be colocated with the module under test; move
the test from connection/__tests__/connector-error.test.ts into the same package
as the ConnectorError module under a module-local __tests__ directory. Update
the import path to reference the local source (e.g., import from
"./ConnectorError" or "../ConnectorError" depending on module layout) so tests
exercise ConnectorError, ConnectorErrorType, detectNeo4jErrorType,
detectPostgresErrorType, and wrapError directly alongside the implementation.
Comment on lines
+101
to
+116
| describe("wrapError", () => { | ||
| it("wraps Neo4j error correctly", () => { | ||
| const raw = { code: "ServiceUnavailable", message: "Failed to connect" }; | ||
| const wrapped = wrapError(raw, "neo4j"); | ||
| expect(wrapped).toBeInstanceOf(ConnectorError); | ||
| expect(wrapped.type).toBe(ConnectorErrorType.CONNECTION); | ||
| expect(wrapped.originalError).toBe(raw); | ||
| }); | ||
|
|
||
| it("wraps PostgreSQL error correctly", () => { | ||
| const raw = new Error("canceling statement due to statement timeout"); | ||
| (raw as unknown as { code: string }).code = "57014"; | ||
| const wrapped = wrapError(raw, "postgresql"); | ||
| expect(wrapped.type).toBe(ConnectorErrorType.TIMEOUT); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add failure-path tests that verify connectors emit ConnectorError, not raw errors.
Lines 101-116 only validate direct wrapError(...) calls. The real connector handlers still pass raw errors to callbacks.onFail (connection/src/neo4j/Neo4jConnectionModule.ts Lines 105-112 and connection/src/postgresql/PostgresConnectionModule.ts Lines 153-171), so standardized taxonomy is not enforced end-to-end. Add module-level tests for those catch paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/__tests__/connector-error.test.ts` around lines 101 - 116, The
current unit tests only exercise wrapError directly; add module-level tests that
trigger the catch paths in Neo4jConnectionModule and PostgresConnectionModule
and assert that callbacks.onFail receives a ConnectorError (not a raw Error).
Specifically, write tests that instantiate or mock Neo4jConnectionModule and
PostgresConnectionModule, cause their operation to throw (e.g., mock the
driver/session/query to throw a connection/timeout error), spy/mock
callbacks.onFail, and assert the argument is an instance of ConnectorError with
the expected ConnectorErrorType; reference wrapError, callbacks.onFail,
Neo4jConnectionModule, and PostgresConnectionModule to locate the code paths to
test.
- Extract validateActionRules() and buildLegacyClickAction() from buildClickAction to reduce cognitive complexity from 23 to <15 - Replace nested ternary in wrapError with if/else chain Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/stores/widget-editor-store.ts (1)
253-257: Consider combiningsetcalls for atomicity.Three separate
set()calls work but could race in edge cases. Combine them into a single update:♻️ Single atomic update
setChartType: (t) => { - set({ chartType: t, chartOptions: getChartDefaults(t) }); - if (!chartSupportsClickAction(t)) set({ clickActionEnabled: false }); - if (!chartSupportsStyling(t)) set({ stylingEnabled: false }); + set({ + chartType: t, + chartOptions: getChartDefaults(t), + ...(!chartSupportsClickAction(t) && { clickActionEnabled: false }), + ...(!chartSupportsStyling(t) && { stylingEnabled: false }), + }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/stores/widget-editor-store.ts` around lines 253 - 257, In setChartType, avoid multiple set() calls that can race; instead compute the new state in one atomic update by calling set once: derive chartOptions via getChartDefaults(t), compute clickActionEnabled as chartSupportsClickAction(t) ? current or true/false as needed, and stylingEnabled as chartSupportsStyling(t) ? current or true/false as needed, then call set({ chartType: t, chartOptions, clickActionEnabled, stylingEnabled }). Reference setChartType, set, getChartDefaults, chartSupportsClickAction, chartSupportsStyling, and the state keys chartType, chartOptions, clickActionEnabled, stylingEnabled when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connection/src/generalized/ConnectorError.ts`:
- Around line 35-37: The code currently assumes err.message and err.code are
strings which can cause classifiers to throw; in detectNeo4jErrorType and
detectPostgresErrorType replace the unsafe casts that set msg and code with
explicit runtime type guards so msg and code are guaranteed strings before any
string operations—use expressions like typeof e.message === "string" ? e.message
: "" and typeof e.code === "string" ? e.code : "" when initializing msg and code
(these variables are used in detectNeo4jErrorType and detectPostgresErrorType)
to prevent non-string values from reaching
msg.includes()/msg.startsWith()/msg.toLowerCase()/msg.includes().
---
Nitpick comments:
In `@app/src/stores/widget-editor-store.ts`:
- Around line 253-257: In setChartType, avoid multiple set() calls that can
race; instead compute the new state in one atomic update by calling set once:
derive chartOptions via getChartDefaults(t), compute clickActionEnabled as
chartSupportsClickAction(t) ? current or true/false as needed, and
stylingEnabled as chartSupportsStyling(t) ? current or true/false as needed,
then call set({ chartType: t, chartOptions, clickActionEnabled, stylingEnabled
}). Reference setChartType, set, getChartDefaults, chartSupportsClickAction,
chartSupportsStyling, and the state keys chartType, chartOptions,
clickActionEnabled, stylingEnabled when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab141af5-d909-4dc2-9ec2-930e7690b9d0
📒 Files selected for processing (2)
app/src/stores/widget-editor-store.tsconnection/src/generalized/ConnectorError.ts
Comment on lines
+35
to
+37
| const e = err as { code?: string; message?: string }; | ||
| const msg = e.message ?? ""; | ||
| const code = e.code ?? ""; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify detector assignments and string operations in this file.
cat -n connection/src/generalized/ConnectorError.ts | sed -n '30,100p'
rg -n -C2 'const (msg|code) = .*\\?\\? ""|toLowerCase\\(' connection/src/generalized/ConnectorError.tsRepository: alfredo1996/neoboard
Length of output: 2734
Guard code/message runtime types before string operations.
Object-shaped errors with non-string code or message properties will cause the classifiers to throw, masking the original error. This is critical because error detection logic must not crash while handling exceptions.
Both functions need explicit type guards:
detectNeo4jErrorType(lines 35-37): Guards needed beforemsg.includes()andmsg.startsWith()calls.detectPostgresErrorType(lines 63-66): Guards needed beforemsg.toLowerCase()and subsequentmsg.includes()calls.
Replace type assertions and coalesce operations with typeof e.message === "string" ? e.message : "" and typeof e.code === "string" ? e.code : "".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@connection/src/generalized/ConnectorError.ts` around lines 35 - 37, The code
currently assumes err.message and err.code are strings which can cause
classifiers to throw; in detectNeo4jErrorType and detectPostgresErrorType
replace the unsafe casts that set msg and code with explicit runtime type guards
so msg and code are guaranteed strings before any string operations—use
expressions like typeof e.message === "string" ? e.message : "" and typeof
e.code === "string" ? e.code : "" when initializing msg and code (these
variables are used in detectNeo4jErrorType and detectPostgresErrorType) to
prevent non-string values from reaching
msg.includes()/msg.startsWith()/msg.toLowerCase()/msg.includes().
The keyboard fallback for CM6 never works reliably. If __cmView isn't available yet, retry via toPass() instead of falling through to the unreliable keyboard insertion path. Increased timeout to 30s. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
This was referenced Mar 27, 2026
Merged
This was referenced Apr 3, 2026
3 tasks
alfredo1996
added a commit
that referenced
this pull request
May 10, 2026
release: v0.9.1 — Stability (architecture + code quality)
5 tasks
5 tasks
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
2 participants
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
Architecture cleanup and code quality improvements. No new features — pure stability.
Bug fixes (#194)
.sort()without compare functionCode quality (#193, #195)
Pluggability (#196, #197, #198)
Prop reduction detail
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests