feat(app): chart plugin registry — contract + registry primitives (#220) - #377
Conversation
…ling - Login: add autoComplete="email" and "current-password" for password managers and browser autofill - Login: handle network errors / unexpected responses so the "Signing in..." spinner never hangs indefinitely - Connections: sanitize Turbopack/webpack internal paths from error messages shown to users in Test Connection UI - New sanitizeErrorMessage utility with 6 unit tests Fixes issues discovered by UX crawler audit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: UX crawler findings — login a11y, error sanitization
First PR in the plugin system epic (#221). Adds the core contract and registry without touching existing chart code. The plugin registry will eventually replace the switch statement in chart-renderer.tsx and the scattered edits across chart-registry.ts, chart-options/, and query-editor-panel.tsx. This PR is purely additive — no existing code paths change. New files: - app/src/lib/chart-plugin-registry.ts — defineChartPlugin, createPluginRegistry - app/src/lib/__tests__/chart-plugin-registry.test.ts — 21 tests Plugin contract (ChartPluginConfig): - type, label, component, transform — required - validate, options, queryHint, compatibleWith, stylingTargets — optional - capabilities: { supportsClickAction, supportsStyling, isECharts, requiresQuery } - enrichClickEvent — plugin-specific click event enrichment Registry API: - register/unregister/get/has/getAll/getTypes/getCompatibleWith Defaults: - supportsClickAction: true - supportsStyling: true when stylingTargets provided, else false - isECharts: false, requiresQuery: true Related: #220, epic #221 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR introduces an error sanitization utility to mask internal bundler/framework error messages in API responses, enhances the login page with comprehensive error handling and input autocomplete attributes, and adds a new chart plugin registry system with validation and capability management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/app/(auth)/login/page.tsx (1)
24-24:⚠️ Potential issue | 🟠 MajorOpen redirect vulnerability via unvalidated
callbackUrl.The
callbackUrlis taken directly from search params and passed torouter.push()without validation. An attacker can craft a link like/login?callbackUrl=https://evil.comto redirect users to a malicious site after successful authentication.Validate that the URL is relative or belongs to an allowed origin:
🛡️ Proposed fix
function LoginForm() { const router = useRouter(); const searchParams = useSearchParams(); - const callbackUrl = searchParams.get("callbackUrl") ?? "/"; + const rawCallback = searchParams.get("callbackUrl") ?? "/"; + // Only allow relative paths to prevent open redirect + const callbackUrl = rawCallback.startsWith("/") && !rawCallback.startsWith("//") + ? rawCallback + : "/"; const [error, setError] = useState("");Also applies to: 44-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/app/`(auth)/login/page.tsx at line 24, The callbackUrl variable is taken directly from searchParams and used with router.push, creating an open redirect; update the login/page.tsx flow to validate callbackUrl before use by ensuring it is either a relative path (starts with "/") or its origin matches a configured allowlist of trusted origins, and fall back to "/" if validation fails; locate the callbackUrl usage and router.push calls to apply this check (validate the searchParams.get("callbackUrl") result and only pass the sanitized/validated value into router.push).
🤖 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/app/`(auth)/login/page.tsx:
- Around line 46-50: The current sign-in handling uses else if (result) which is
unsafe because NextAuth v5 always returns a SignInResponse; update the check to
explicitly test success (e.g., use result?.ok) so you only redirect on true, and
remove or simplify the now-unreachable else branch that assumes a null result;
adjust the logic in the submit/handleSignIn function where signIn("credentials",
...) is awaited and result is inspected to use result?.ok and keep the existing
catch for network errors.
In `@app/src/lib/chart-plugin-registry.ts`:
- Around line 133-156: Trim and normalize config.type and config.label before
using them to build the plugin object and for validations: after you validate
(or as part of the validation) replace or derive trimmed values (e.g., const
type = config.type.trim(); const label = config.label.trim()) and use those
normalized variables when constructing the returned object (type, label) and
anywhere else the raw config values are used (e.g., when computing capabilities
or registering the plugin). Ensure the validation checks use the trimmed
variables so registration stores the normalized keys instead of the original
padded strings.
---
Outside diff comments:
In `@app/src/app/`(auth)/login/page.tsx:
- Line 24: The callbackUrl variable is taken directly from searchParams and used
with router.push, creating an open redirect; update the login/page.tsx flow to
validate callbackUrl before use by ensuring it is either a relative path (starts
with "/") or its origin matches a configured allowlist of trusted origins, and
fall back to "/" if validation fails; locate the callbackUrl usage and
router.push calls to apply this check (validate the
searchParams.get("callbackUrl") result and only pass the sanitized/validated
value into router.push).
🪄 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: 20a567af-affe-40a3-add6-5fa437d97f07
📒 Files selected for processing (7)
app/src/app/(auth)/login/page.tsxapp/src/app/api/connections/[id]/test/route.tsapp/src/app/api/connections/test-inline/route.tsapp/src/lib/__tests__/api-utils.test.tsapp/src/lib/__tests__/chart-plugin-registry.test.tsapp/src/lib/api-utils.tsapp/src/lib/chart-plugin-registry.ts
| } else { | ||
| // Nothing returned → server unreachable | ||
| setError("Unable to sign in. Please try again."); | ||
| setLoading(false); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
NextAuth v5 signIn function return value with redirect false
💡 Result:
In NextAuth.js (Auth.js) v5, when calling the client-side signIn function with { redirect: false }, it returns a Promise object containing { ok: boolean, error?: string, code?: string }. This allows checking if sign-in succeeded without automatic redirection: if (response.ok) { // success, manually redirect or update UI } else { // handle error: response.error }. The exact shape differs slightly from v4 (which had { ok, error, status, url }), and v5 beta versions have reported inconsistencies where it sometimes returns a URL string instead of the object, or ok: true despite errors (ongoing issues #9465, #6040). Official docs confirm SignInResponse has ok, error, code for client use with redirect: false. Use .then or await to access it, as one example showed: await signIn(...).then(({ ok, error }) => ...). For server actions, signIn throws AuthError on failure instead of returning response.
Citations:
- 1: https://www.authjs.cn/reference/sveltekit/client
- 2: https://authjs.dev/reference/sveltekit/client
- 3: https://authjs.dev/reference/nextjs/react
- 4: https://nextjs.authjs.dev/react
- 5: signIn using redirect: false does not return specified object nextauthjs/next-auth#9465
- 6: signIn with redirect: false returns invalid response on error nextauthjs/next-auth#6040
🏁 Script executed:
# Find and check the login page file
find . -name "page.tsx" | grep -i login | head -5Repository: alfredo1996/neoboard
Length of output: 99
🏁 Script executed:
# Check NextAuth version in package files
find . -name "package.json" -o -name "package-lock.json" -o -name "yarn.lock" | head -3Repository: alfredo1996/neoboard
Length of output: 130
🏁 Script executed:
# Get NextAuth version
cat app/package.json 2>/dev/null | grep -A 2 -B 2 '"next-auth"' || \
cat package.json 2>/dev/null | grep -A 2 -B 2 '"next-auth"'Repository: alfredo1996/neoboard
Length of output: 200
🏁 Script executed:
# Read the login page to see the full context around lines 46-50
cat -n app/src/app/\(auth\)/login/page.tsx | sed -n '35,60p'Repository: alfredo1996/neoboard
Length of output: 987
🏁 Script executed:
# Double-check if there are any other signIn calls with different patterns in the codebase
rg "signIn.*redirect.*false" app/src --type ts --type tsx -A 3 -B 3Repository: alfredo1996/neoboard
Length of output: 91
🏁 Script executed:
# Search for signIn calls with redirect false
rg "signIn.*redirect.*false" app/src -A 3 -B 3Repository: alfredo1996/neoboard
Length of output: 46
The else branch is unreachable — but the real issue is the unsafe condition above it.
NextAuth v5 signIn("credentials", { redirect: false }) always returns a SignInResponse object { ok, error, code } on both success and failure. It only throws on network errors (caught by your catch block). The else branch will never execute.
However, the bigger problem is else if (result) at line 44 — it allows redirect even when result.ok is false and result.error is undefined. Change to else if (result?.ok) to safely verify success before redirecting:
♻️ Suggested fix
if (result?.error) {
setError("Invalid email or password");
setLoading(false);
- } else if (result) {
+ } else if (result?.ok) {
router.push(callbackUrl);
} else {
setError("Unable to sign in. Please try again.");
setLoading(false);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/app/`(auth)/login/page.tsx around lines 46 - 50, The current sign-in
handling uses else if (result) which is unsafe because NextAuth v5 always
returns a SignInResponse; update the check to explicitly test success (e.g., use
result?.ok) so you only redirect on true, and remove or simplify the
now-unreachable else branch that assumes a null result; adjust the logic in the
submit/handleSignIn function where signIn("credentials", ...) is awaited and
result is inspected to use result?.ok and keep the existing catch for network
errors.
| if (!config.type || config.type.trim() === "") { | ||
| throw new Error("Chart plugin: type is required and cannot be empty"); | ||
| } | ||
| if (!config.label || config.label.trim() === "") { | ||
| throw new Error("Chart plugin: label is required and cannot be empty"); | ||
| } | ||
| if (typeof config.transform !== "function") { | ||
| throw new Error("Chart plugin: transform must be a function"); | ||
| } | ||
|
|
||
| // supportsStyling defaults to true if stylingTargets is provided, false otherwise | ||
| const stylingFromTargets = | ||
| config.stylingTargets && config.stylingTargets.length > 0; | ||
|
|
||
| const capabilities: ChartCapabilities = { | ||
| ...DEFAULT_CAPABILITIES, | ||
| ...(stylingFromTargets ? { supportsStyling: true } : {}), | ||
| ...config.capabilities, | ||
| }; | ||
|
|
||
| return { | ||
| type: config.type, | ||
| label: config.label, | ||
| component: config.component, |
There was a problem hiding this comment.
Normalize plugin type/label before storing.
Validation trims, but the returned plugin currently preserves original strings. A plugin like " bar " will register under the spaced key and fail expected lookups.
Proposed fix
export function defineChartPlugin(config: ChartPluginConfig): ChartPlugin {
+ const normalizedType =
+ typeof config.type === "string" ? config.type.trim() : "";
+ const normalizedLabel =
+ typeof config.label === "string" ? config.label.trim() : "";
+
// Validation
- if (!config.type || config.type.trim() === "") {
+ if (!normalizedType) {
throw new Error("Chart plugin: type is required and cannot be empty");
}
- if (!config.label || config.label.trim() === "") {
+ if (!normalizedLabel) {
throw new Error("Chart plugin: label is required and cannot be empty");
}
if (typeof config.transform !== "function") {
throw new Error("Chart plugin: transform must be a function");
}
@@
return {
- type: config.type,
- label: config.label,
+ type: normalizedType,
+ label: normalizedLabel,
component: config.component,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/chart-plugin-registry.ts` around lines 133 - 156, Trim and
normalize config.type and config.label before using them to build the plugin
object and for validations: after you validate (or as part of the validation)
replace or derive trimmed values (e.g., const type = config.type.trim(); const
label = config.label.trim()) and use those normalized variables when
constructing the returned object (type, label) and anywhere else the raw config
values are used (e.g., when computing capabilities or registering the plugin).
Ensure the validation checks use the trimmed variables so registration stores
the normalized keys instead of the original padded strings.
|
feat(app): chart plugin registry — contract + registry primitives (#220)
2 participants
Summary
First PR in the plugin system epic (#221). Adds the plugin contract and registry primitives — purely additive, no existing code paths change.
What this adds
Design decisions
Not in this PR (future PRs)
Test plan
Related: #220, epic #221
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests