fix: handle null values in drawTable to prevent Node 22 crash #240
Conversation
WalkthroughThe diff introduces null-safety guards throughout the parser module to handle edge cases with null or undefined values. Changes include adding null checks in object-type field detection, implementing early returns for empty or falsy data in drawTable, using safe object spreading patterns (item || {}) to prevent errors when elements are null, and adding safeguards for row value extraction. A new reproduction script is added to test drawTable with edge cases including null values and null array elements, using try-catch blocks to verify robustness. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@repro_bug_sk.js`:
- Around line 1-3: The file repro_bug_sk.js is a manual reproduction script and
should be removed from the repository; delete repro_bug_sk.js (or move it out to
local notes/docs) and remove any references to it, and if you must keep a local
copy update its import of './lib/parser.ts' (referenced via drawTable import) to
a runtime-compatible module (e.g., './lib/parser.js') or remove the import
entirely so the repo does not require tsx/ts-node to run; ensure
package.json/test remains unchanged if you remove the script.
🧹 Nitpick comments (2)
lib/parser.ts (1)
58-62: Guard clause correctly handles empty and null-first-element cases.The early return prevents crashes when
datais falsy, empty, or when the first element is null. However, consider whether printing"[]"is the right output whendata[0] === nullbut other elements might be valid objects.If an array like
[null, {id: 1}]is passed, the current code exits early with"[]"instead of rendering the valid second element. If this edge case matters, you could filter out null elements before checking:♻️ Optional: Handle arrays with leading nulls
- if (!data || data.length === 0 || data[0] === null) { + // Filter out null elements first + const filteredData = data?.filter(item => item !== null) ?? []; + if (filteredData.length === 0) { console.log("[]"); return; } + data = filteredData;repro_bug_sk.js (1)
17-24: Test 1 coverage looks good but lacks assertion rigor.The test correctly exercises the null field scenario (
scheduledAt: null). However, usingconsole.logfor pass/fail output makes it difficult to integrate with CI pipelines. The success message appears even ifdrawTablesilently fails without throwing.If keeping this as a manual script, consider verifying the output or at minimum ensuring no error was thrown:
// The current test passes even if drawTable outputs incorrect data // Consider adding output capture/verification if this becomes a real test
| // repro_bug.js | ||
|
|
||
| import { drawTable } from './lib/parser.ts'; // Change .js to .ts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the repro_bug_sk.js file
fd -name "repro_bug_sk.js" -type fRepository: appwrite/sdk-for-cli
Length of output: 294
🏁 Script executed:
# Check repository structure
git ls-files | grep -E "(repro_bug|test|spec)" | head -20Repository: appwrite/sdk-for-cli
Length of output: 212
🏁 Script executed:
# Check package.json for test configuration
cat package.json 2>/dev/null | head -50Repository: appwrite/sdk-for-cli
Length of output: 2283
🏁 Script executed:
# Look for test directories
fd -type d -name "__tests__" -o -name "test" -o -name "tests" -o -name "spec"Repository: appwrite/sdk-for-cli
Length of output: 234
🏁 Script executed:
# Read the actual repro_bug_sk.js file
cat -n repro_bug_sk.jsRepository: appwrite/sdk-for-cli
Length of output: 1220
🏁 Script executed:
# Check if this file is in .gitignore
cat .gitignore 2>/dev/null | grep -i "repro"Repository: appwrite/sdk-for-cli
Length of output: 46
🏁 Script executed:
# Check if lib/parser.ts exists
fd -name "parser.ts" -type fRepository: appwrite/sdk-for-cli
Length of output: 294
🏁 Script executed:
# Check project root structure
ls -la | grep -E "^-.*\.(js|ts)$"Repository: appwrite/sdk-for-cli
Length of output: 306
Remove this reproduction script from the repository.
The filename in line 1's comment (repro_bug.js) doesn't match the actual filename (repro_bug_sk.js). More importantly, this is a manual debug script rather than a proper automated test. Since the project has no test framework configured ("test": "echo \"Error: no test specified\" && exit 1"), either remove this file or move it to local documentation/notes outside the repository.
The direct .ts import also requires tsx or ts-node to execute, which adds unnecessary complexity for what should be manual verification if needed.
🤖 Prompt for AI Agents
In `@repro_bug_sk.js` around lines 1 - 3, The file repro_bug_sk.js is a manual
reproduction script and should be removed from the repository; delete
repro_bug_sk.js (or move it out to local notes/docs) and remove any references
to it, and if you must keep a local copy update its import of './lib/parser.ts'
(referenced via drawTable import) to a runtime-compatible module (e.g.,
'./lib/parser.js') or remove the import entirely so the repo does not require
tsx/ts-node to run; ensure package.json/test remains unchanged if you remove the
script.
1 participant
What does this PR do?
In Node 22, the CLI crashes with
TypeError: Cannot convert undefined or null to objectwhen a function execution log containsnullfor metadata fields likescheduledAt. This occurs becausetypeof nullreturns'object', leading the parser to attempt table rendering on non-iterable values.The Solution
drawTableto handlenullor empty data arrays.(item || {})withinreduceandmapto prevent illegal object conversions.nullplaceholders with a hyphen (-).Test Plan
Test Plan
I verified these changes by creating a reproduction script that simulates the exact conditions under which the TypeError occurs in Node 22 (specifically when a function execution log contains null metadata from a database-triggered event).
Verification Steps:
Environment: Node v22.15.0.
Reproduction Script: Created a script that passes a mock execution object with scheduledAt: null into the drawTable function.
Command Execution: Ran the test using npx tsx repro_bug_sk.js.
Test Results:
Test 1 (Null Metadata): Passed. The table rendered correctly, displaying a hyphen - for the null scheduledAt field instead of crashing with a TypeError.
Test 2 (Null Array Element): Passed. The parser gracefully handled an array containing a null element [null] without attempting to access keys.
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Output:
--- Test 1: Simulating Database Trigger with Null values ---
$id │ status │ duration │ scheduledAt │ trigger
───────────┼───────────┼──────────┼─────────────┼──────────
65ae12345 │ completed │ 0.123 │ - │ database
✅ Test 1 Passed: Table rendered successfully despite null values.
--- Test 2: Simulating Empty Object array ---
[]
✅ Test 2 Passed: Handled [null] array gracefully.
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.