[Components] roamresearch - Added new components #14482
Conversation
jcortes
added
action
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
WalkthroughThe pull request includes updates across multiple JavaScript modules within the Roam Research integration. Key changes involve incrementing version numbers from "0.0.1" to "0.0.2" in several action files and correcting the spelling of "Successfully" in success messages. Additionally, new modules for pulling, querying, and appending data have been introduced, enhancing the overall functionality of the application. The changes also include the addition of new properties and methods to improve resource handling and API interactions. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (15)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@jcortes is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
jcortes
force-pushed
the
roamresearch-new-generic-components-and-source
branch
from
4d16abf to
58f8097
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (10)
components/roamresearch/actions/query/query.mjs (1)
9-21: Enhance props documentation and fix typo.The props are well-structured, but there are a few improvements that could be made:
- Fix typo in args description:
- description: "The arguments to pass to the query. Eg. `apple` as the firs argument.", + description: "The arguments to pass to the query. Eg. `apple` as the first argument.",
- Consider enhancing the args description to clarify the array format:
- description: "The arguments to pass to the query. Eg. `apple` as the first argument.", + description: "The arguments to pass to the query in order. Each item corresponds to a placeholder in the query. Eg. ['apple'] where 'apple' matches ?search-string in the query.",components/roamresearch/actions/get-page-or-block-data/get-page-or-block-data.mjs (1)
7-7: Version bump looks appropriate.The minor version increment aligns with the refactoring changes.
There's a typo in the success message on line 48: "Succesfully" should be "Successfully".
- $.export("$summary", `Succesfully got data for ${resourceType}: \`${pageOrBlock}\`.`); + $.export("$summary", `Successfully got data for ${resourceType}: \`${pageOrBlock}\`.`);components/roamresearch/actions/pull/pull.mjs (1)
1-8: LGTM! Consider enhancing the description.The basic configuration is well-structured. The documentation link is helpful, but consider adding a brief explanation of what a "pull" operation does in the context of Roam Research for better clarity.
- description: "Generic pull for Roam Research pages. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).", + description: "Retrieve data from Roam Research pages using Datalog pull syntax. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).",components/roamresearch/actions/pull-many/pull-many.mjs (1)
3-8: Enhance the action description for better clarity.The current description could be more specific about what "pull many" means in the context of Roam Research. Consider expanding it to explain that this action allows fetching data for multiple entities simultaneously using the Roam Research pull API.
- description: "Generic pull many for Roam Research pages. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).", + description: "Fetch data for multiple Roam Research entities simultaneously using the pull API. This allows retrieving specified attributes for multiple pages or blocks in a single operation. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).",components/roamresearch/common/utils.mjs (2)
26-47: Enhance error handling with more specific messages.The error handling could be improved to provide more context about the validation failure.
Consider this enhancement:
function parseArray(value) { try { if (!value) { return; } if (Array.isArray(value)) { return value; } + if (typeof value !== 'string') { + throw new ConfigurationError('Expected a JSON string or an array'); + } + const parsedValue = JSON.parse(value); if (!Array.isArray(parsedValue)) { - throw new Error("Not an array"); + throw new Error(`Expected an array but got ${typeof parsedValue}`); } return parsedValue; } catch (e) { - throw new ConfigurationError("Make sure the custom expression contains a valid array object"); + throw new ConfigurationError( + e instanceof ConfigurationError + ? e.message + : `Invalid array input: ${e.message}` + ); } }
54-57: Add JSDoc documentation for exported functions.Consider adding TypeScript-style JSDoc comments to improve IDE support and documentation.
Here's the suggested enhancement:
+/** + * Parses and validates array input, with JSON parsing for each element + * @param {string|Array} value - The input value to parse + * @returns {Array|undefined} Parsed array or undefined for empty input + * @throws {ConfigurationError} If the input is invalid + */ +const parseArrayWithJson = (value) => parseArray(value)?.map(parseJson); + +/** + * Safely retrieves a nested property from an object using dot notation + * @param {Object} obj - The source object + * @param {string} propertyString - Dot-separated path to the property + * @returns {*} The value at the specified path or undefined if not found + */ +const getNestedPropertyWithTypes = getNestedProperty; + export default { - parseArray: (value) => parseArray(value)?.map(parseJson), - getNestedProperty, + parseArray: parseArrayWithJson, + getNestedProperty: getNestedPropertyWithTypes, };components/roamresearch/actions/append-block/append-block.mjs (1)
12-16: Add example value for string property.To improve user experience, consider adding an example value to the string property description.
string: { type: "string", label: "Content", - description: "The string to append.", + description: "The string to append. Example: 'This is a new block'", },components/roamresearch/roamresearch.app.mjs (1)
80-91: Add JSDoc documentation to new methods.The methods are well-implemented but would benefit from documentation describing their purpose, parameters, and return values.
Apply this diff to add documentation:
+ /** + * Retrieves data for multiple entities from Roam Research + * @param {Object} args - The request parameters + * @param {string[]} [args.uids] - Array of block UIDs to pull + * @returns {Promise<Object>} The response from Roam Research + */ pullMany(args = {}) { return this.post({ path: "/pull-many", ...args, }); }, + /** + * Performs write operations in Roam Research + * @param {Object} args - The request parameters + * @param {string} [args.action] - The write action to perform + * @param {Object} [args.block] - The block data for the write operation + * @returns {Promise<Object>} The response from Roam Research + */ write(args = {}) { return this.post({ path: "/write", ...args, }); },components/roamresearch/actions/write/write.mjs (2)
182-182: Typo in the success message.The word "Succesfully" is misspelled in the export summary message. It should be "Successfully".
Proposed fix:
- $.export("$summary", "Succesfully ran the action."); + $.export("$summary", "Successfully ran the action.");
31-33: Remove unnecessary square brackets from object property names.Within object literals, square brackets are used for computed property names. Since the property names are static strings, the use of square brackets is unnecessary and may cause confusion. Replace
["property-name"]with"property-name"for clarity.Example fix:
Change:
- default: { - ["parent-uid"]: "optional", - ["page-title"]: "optional", - order: "last", - },To:
+ default: { + "parent-uid": "optional", + "page-title": "optional", + order: "last", + },Also applies to: 45-47, 59-61, 86-88, 115-116, 130-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
components/roamresearch/actions/add-content-to-daily-note-page/add-content-to-daily-note-page.mjs(1 hunks)components/roamresearch/actions/add-content-to-page/add-content-to-page.mjs(1 hunks)components/roamresearch/actions/add-content-underneath-block/add-content-underneath-block.mjs(1 hunks)components/roamresearch/actions/append-block/append-block.mjs(1 hunks)components/roamresearch/actions/get-page-or-block-data/get-page-or-block-data.mjs(1 hunks)components/roamresearch/actions/pull-many/pull-many.mjs(1 hunks)components/roamresearch/actions/pull/pull.mjs(1 hunks)components/roamresearch/actions/query/query.mjs(1 hunks)components/roamresearch/actions/search-title/search-title.mjs(1 hunks)components/roamresearch/actions/write/write.mjs(1 hunks)components/roamresearch/common/constants.mjs(1 hunks)components/roamresearch/common/utils.mjs(1 hunks)components/roamresearch/package.json(1 hunks)components/roamresearch/roamresearch.app.mjs(2 hunks)components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- components/roamresearch/actions/add-content-to-daily-note-page/add-content-to-daily-note-page.mjs
- components/roamresearch/actions/add-content-to-page/add-content-to-page.mjs
- components/roamresearch/actions/add-content-underneath-block/add-content-underneath-block.mjs
- components/roamresearch/actions/search-title/search-title.mjs
- components/roamresearch/package.json
🔇 Additional comments (16)
components/roamresearch/common/constants.mjs (2)
10-19: LGTM! Well-structured action constants.
The ACTION constant provides a comprehensive set of CRUD operations for both blocks and pages, with clear and consistent naming. The constants align well with the Roam Research API capabilities and support the implementation of various actions mentioned in the PR objectives.
26-26: LGTM! Proper export of the new constant.
The ACTION constant is correctly exported alongside existing constants, maintaining a consistent export structure.
components/roamresearch/actions/query/query.mjs (2)
1-8: LGTM! Module setup and metadata are well-structured.
The imports and metadata are properly defined, with good documentation linking to the Roam Research developer docs.
29-35: Verify query format against Roam Research API.
Please ensure that the query format and structure aligns with the latest Roam Research API specifications.
✅ Verification successful
Based on the code inspection, I can now generate the final response:
Query format matches Roam Research API specifications
The implementation correctly follows the Roam Research API format:
- Uses Datalog query language as required
- Properly structures the query with
:find,:in, and:whereclauses - Correctly passes query arguments in the expected format
- Links to official Roam Research developer documentation for reference
The example query in the description demonstrates the correct format: [:find ?block-uid ?block-str :in $ ?search-string :where [?b :block/uid ?block-uid] [?b :block/string ?block-str] [(clojure.string/includes? ?block-str ?search-string)]]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other query implementations in the codebase for consistency
# Search for other query implementations
rg -l "app.query" --type js --type mjs
# Check for query format examples in documentation
rg -l "Datalog.*query" --type md
Length of output: 101
Script:
#!/bin/bash
# Let's try different approaches to find query implementations and documentation
# Search for query implementations without file type restrictions
rg -l "app\.query"
# Search for any files containing Roam Research API documentation
rg -l -i "roam.*api|roam.*documentation"
# Search for files with .md extension containing query examples
find . -name "*.md" -type f -exec grep -l "query" {} \;
# Look for README or documentation files in the roamresearch component
find . -path "*roamresearch*" -type f -name "README*" -o -name "*.md"
Length of output: 83995
Script:
#!/bin/bash
# Let's check the specific Roam Research documentation and query implementation
# Check the Roam Research component README and query action documentation
cat components/roamresearch/README.md
cat components/roamresearch/actions/query/query.mjs
# Look for any example queries or API format documentation
rg -A 5 -B 5 "query.*format|datalog.*query" components/roamresearch/
Length of output: 1292
components/roamresearch/actions/get-page-or-block-data/get-page-or-block-data.mjs (3)
Line range hint 1-53: Implementation aligns well with PR objectives.
The component successfully implements the data retrieval functionality for both pages and blocks as specified in issue #14438. The code:
- Properly handles different resource types (page/block)
- Includes appropriate error handling
- Returns comprehensive data including children blocks
12-14: Property definition refactoring looks good.
The change to use propDefinition improves code maintainability by centralizing the property definition in the app file.
Let's verify the resourceType property is properly defined in the app file:
#!/bin/bash
# Description: Verify resourceType property definition exists in app file
ast-grep --pattern 'resourceType: {
$$$
}'18-21: Property definition refactoring looks good.
The change to use propDefinition improves code maintainability by centralizing the property definition in the app file.
Let's verify the pageOrBlock property is properly defined in the app file:
✅ Verification successful
Property definition is correctly implemented and referenced
The pageOrBlock property is properly defined in components/roamresearch/roamresearch.app.mjs with appropriate type, label, and description. The refactoring to use propDefinition correctly references this centralized definition, improving code maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pageOrBlock property definition exists in app file
ast-grep --pattern 'pageOrBlock: {
$$$
}'
Length of output: 139
Script:
#!/bin/bash
# Search for pageOrBlock property definition with proper quoting
ast-grep --pattern $'pageOrBlock: {
$$$
}'
# Also try searching with ripgrep for any pageOrBlock definitions
rg "pageOrBlock" -A 5
Length of output: 5918
components/roamresearch/actions/pull/pull.mjs (1)
9-21: 🛠️ Refactor suggestion
Add input validation for Datalog syntax.
While the examples in the descriptions are helpful, the string inputs for eid and selector should be validated to ensure they contain valid Datalog syntax. Invalid syntax could lead to runtime errors.
Consider adding a validation function:
const isValidDatalog = (str) => {
try {
// Basic validation: check for matching brackets and proper structure
const balanced = str.split('').reduce((stack, char) => {
if (char === '[') stack.push(char);
if (char === ']') {
if (stack.length === 0) throw new Error();
stack.pop();
}
return stack;
}, []);
return balanced.length === 0 && str.startsWith('[') && str.endsWith(']');
} catch {
return false;
}
};Then update the props:
eid: {
type: "string",
label: "Entity ID",
description: "The entity ID to pull. Eg. `[:block/uid \"08-30-2022\"]`.",
+ validate: (value) => {
+ if (!isValidDatalog(value)) {
+ throw new Error("Invalid Datalog syntax for entity ID");
+ }
+ },
},
selector: {
type: "string",
label: "Selector",
description: "The selector to pull. Eg. `[:block/uid :node/title :block/string {:block/children [:block/uid :block/string]} {:block/refs [:node/title :block/string :block/uid]}]`.",
+ validate: (value) => {
+ if (!isValidDatalog(value)) {
+ throw new Error("Invalid Datalog syntax for selector");
+ }
+ },
},components/roamresearch/actions/pull-many/pull-many.mjs (2)
1-45: Overall implementation aligns with PR objectives.
The module provides the necessary functionality to fetch data from Roam Research, which supports the other components mentioned in the PR objectives. The implementation is clean and follows the component structure, though it could benefit from the suggested improvements above.
11-20:
Add input validation for EDN syntax.
The eids and selector props accept raw EDN syntax as strings, which could be risky without proper validation. Consider:
- Adding input validation to ensure the EDN syntax is well-formed
- Implementing sanitization to prevent injection attacks
- Adding a note about the expected format in the description
eids: {
type: "string",
label: "Entity IDs",
- description: "The entity ID to pull. Eg. `[[:block/uid \"08-30-2022\"] [:block/uid \"08-31-2022\"]]`.",
+ description: "The entity IDs to pull in EDN format. Must be a valid EDN vector of lookup refs. Example: `[[:block/uid \"08-30-2022\"] [:block/uid \"08-31-2022\"]]`. Note: Ensure the syntax is correct to avoid errors.",
+ validateFn: ({ value }) => {
+ // Add EDN syntax validation
+ if (!value.startsWith('[') || !value.endsWith(']')) {
+ return "Must be a valid EDN vector";
+ }
+ },
},components/roamresearch/common/utils.mjs (1)
1-57: Verify utility functions support all Roam Research operations.
Let's ensure these utilities fully support the PR objectives for Roam Research integration.
The utilities should support:
- Parsing JSON responses from Roam Research API
- Handling nested block structures
- Accessing deep properties in API responses
✅ Verification successful
Utility functions are properly implemented and utilized across components
The verification shows that the utility functions are correctly implemented and used appropriately across the Roam Research components:
-
parseArrayis used in:append-block.mjsfor handling children blockswrite.mjsfor processing batch actions
-
getNestedPropertyis used in:new-modified-linked-reference.mjsfor accessing nested response data
The utilities support all required operations:
- JSON parsing for API responses
- Array handling for block structures
- Deep property access for complex data structures
The implementation aligns with the PR objectives for handling daily notes, managing page content, and processing block data in Roam Research integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of utility functions in Roam Research components
echo "Checking usage of utility functions in Roam Research components..."
# Look for potential JSON parsing needs in other Roam components
rg -l "JSON.parse" --type js components/roamresearch/
# Check for array handling in block operations
rg -l "children|blocks" --type js components/roamresearch/
# Look for nested property access patterns
rg -l "\." --type js components/roamresearch/ | grep -v "utils.mjs"
Length of output: 325
Script:
#!/bin/bash
# Let's try a different approach to verify the utility functions usage
# First, let's find all JavaScript/TypeScript files in the roamresearch component
echo "=== Finding all JavaScript/TypeScript files ==="
fd -e js -e mjs -e ts components/roamresearch/
# Then, let's examine the content of these files to understand the context
echo -e "\n=== Examining file contents ==="
for file in $(fd -e js -e mjs -e ts components/roamresearch/); do
if [ "$file" != "components/roamresearch/common/utils.mjs" ]; then
echo -e "\n=== Content of $file ===\n"
cat "$file"
fi
done
# Let's also check if there are any test files
echo -e "\n=== Looking for test files ==="
fd -e test.js -e spec.js -e test.ts -e spec.ts components/roamresearch/
Length of output: 1639
Script:
#!/bin/bash
# Let's correct the fd command syntax and try again
# Find all JavaScript/TypeScript files in the roamresearch component
echo "=== Finding all JavaScript/TypeScript files ==="
fd . 'components/roamresearch/' -e js -e mjs -e ts
# Examine the content of these files
echo -e "\n=== Examining imports and usage patterns ==="
for file in $(fd . 'components/roamresearch/' -e js -e mjs -e ts); do
if [ "$file" != "components/roamresearch/common/utils.mjs" ]; then
echo -e "\n=== File: $file ==="
# Look for imports of utils
echo "--- Imports ---"
rg "import.*utils" "$file" || true
# Look for usage of parseArray and getNestedProperty
echo "--- Usage of utility functions ---"
rg "parseArray|getNestedProperty" "$file" || true
# Look for array and nested object handling
echo "--- Array and object handling ---"
rg "blocks|children|\{.*\}|\[.*\]" "$file" || true
fi
done
Length of output: 18300
components/roamresearch/actions/append-block/append-block.mjs (3)
1-2: LGTM! Imports are properly structured.
4-9: LGTM! Component metadata is well-structured.
The metadata follows best practices with clear naming and documentation links.
4-81: Verify integration with other Roam Research components.
This generic append block action appears to be a building block for other actions mentioned in the PR objectives (add-daily-note-content, add-page-content). Please ensure it's properly integrated with these higher-level components.
✅ Verification successful
Let me search for the other Roam Research actions to understand if they're using this component's functionality differently.
Let me check if the higher-level components like add-content-to-daily-note-page and add-content-to-page are using the append-block functionality, even if not directly importing it.
Let me check the app implementation to understand how these components are connected through the appendBlocks method.
Integration verification completed - components work together correctly
The append-block component is properly integrated within the Roam Research ecosystem:
- The app's
appendBlocksmethod serves as the common interface for block operations - Higher-level components (
add-content-to-daily-note-page,add-content-to-page) use this same method with appropriate data structures - The generic append-block component provides the foundational block manipulation capabilities needed by other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to this component in other Roam Research actions
rg -l "roamresearch-append-block" components/roamresearch/actions/
Length of output: 128
Script:
#!/bin/bash
# Find all Roam Research action components
fd "\.mjs$" components/roamresearch/actions/ --exec cat {} \; | grep -A 5 "export default"
Length of output: 3146
Script:
#!/bin/bash
# Check implementation of content addition actions
cat components/roamresearch/actions/add-content-to-daily-note-page/add-content-to-daily-note-page.mjs components/roamresearch/actions/add-content-to-page/add-content-to-page.mjs
Length of output: 3001
Script:
#!/bin/bash
# Check the Roam Research app implementation
cat components/roamresearch/roamresearch.app.mjs
Length of output: 2276
components/roamresearch/roamresearch.app.mjs (1)
Line range hint 1-91: Implementation aligns well with PR objectives.
The changes provide a solid foundation for the Roam Research integration:
- Props support the required functionality for page/block operations
- Methods follow consistent patterns and support all required API operations
- Code is clean and well-structured
components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs (1)
15-22: Consider adding minimum polling interval validation.
While using DEFAULT_POLLING_SOURCE_TIMER_INTERVAL is good, consider adding a minimum interval validation to prevent potential API rate limiting issues with Roam Research.
components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs
Show resolved
Hide resolved
components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs
Outdated
Show resolved
Hide resolved
components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs
Show resolved
Hide resolved
components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs
Show resolved
Hide resolved
jcortes
force-pushed
the
roamresearch-new-generic-components-and-source
branch
2 times, most recently
from
6748ee7 to
2b6c3e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
components/roamresearch/actions/pull/pull.mjs (2)
7-7: Update version number to match other components.According to the PR summary, other components have been updated to version "0.0.2". Consider updating this component's version for consistency.
- version: "0.0.1", + version: "0.0.2",
6-6: Enhance component documentation with usage examples.Consider expanding the description to explain how this generic pull action relates to other Roam Research components and provide common usage patterns.
- description: "Generic pull for Roam Research pages. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).", + description: "Generic pull for Roam Research pages. Used by other components to fetch page content, block data, and references. For API details, [see the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).",components/roamresearch/actions/append-blocks/append-blocks.mjs (1)
7-7: Enhance component documentationThe description could be more detailed to explain:
- The relationship with other Roam Research components
- Example usage scenarios
- Expected input/output formats
Consider updating the description:
- description: "Generic append blocks for Roam Research pages. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/kjnAseQ-K).", + description: "Append blocks to existing pages or blocks in Roam Research. Use this to add content to specific locations in your Roam graph. Supports both page-name and block-uid targeting. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/kjnAseQ-K).",components/roamresearch/actions/pull-many/pull-many.mjs (2)
7-7: Update version number to match other components.According to the AI summary, other Roam Research components have been updated to version "0.0.2". Consider updating this component's version for consistency.
- version: "0.0.1", + version: "0.0.2",
1-45: Consider creating shared utilities for EDN handling.Since Roam Research's API uses EDN format extensively, consider creating shared utility functions for:
- EDN validation and parsing
- Common selectors and entity reference patterns
- Error handling specific to EDN-related issues
This would improve maintainability across all Roam Research components.
components/roamresearch/common/utils.mjs (1)
26-47: Add JSDoc documentation and improve error handling.The function would benefit from:
- Clear documentation of parameters, return type, and thrown errors
- More specific error messages that include the actual error details
Here's the suggested improvement:
+/** + * Parses and validates array input + * @param {*} value - The value to parse as an array + * @returns {Array|undefined} The parsed array or undefined if input is empty + * @throws {ConfigurationError} If the input cannot be parsed as an array + */ function parseArray(value) { try { if (!value) { return; } if (Array.isArray(value)) { return value; } const parsedValue = JSON.parse(value); if (!Array.isArray(parsedValue)) { throw new Error("Not an array"); } return parsedValue; } catch (e) { - throw new ConfigurationError("Make sure the custom expression contains a valid array object"); + throw new ConfigurationError(`Invalid array input: ${e.message}`); } }components/roamresearch/actions/write/write.mjs (3)
11-20: Enhance action property documentation.The action description could be more specific about the available options. Consider listing all possible actions in the description for better clarity.
action: { type: "string", label: "Action", - description: "The action to run. Eg. `create-block`.", + description: "The action to run. Available actions: create-block, move-block, update-block, delete-block, create-page, update-page, delete-page, batch-actions.", options: Object.values(constants.ACTION), reloadProps: true, },
182-182: Make the success message more descriptive.The success message could be more informative by including the specific action that was performed.
- $.export("$summary", "Successfully ran the action."); + $.export("$summary", `Successfully executed ${action} action.`);
1-185: Well-structured implementation with comprehensive action support.The module provides a robust implementation of all required Roam Research write actions with good documentation and flexibility. The batch actions feature is particularly useful for complex operations.
Consider adding rate limiting or throttling for batch operations to prevent overwhelming the Roam Research API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
components/roamresearch/actions/add-content-to-daily-note-page/add-content-to-daily-note-page.mjs(2 hunks)components/roamresearch/actions/add-content-to-page/add-content-to-page.mjs(2 hunks)components/roamresearch/actions/add-content-underneath-block/add-content-underneath-block.mjs(2 hunks)components/roamresearch/actions/append-blocks/append-blocks.mjs(1 hunks)components/roamresearch/actions/get-page-or-block-data/get-page-or-block-data.mjs(2 hunks)components/roamresearch/actions/pull-many/pull-many.mjs(1 hunks)components/roamresearch/actions/pull/pull.mjs(1 hunks)components/roamresearch/actions/query/query.mjs(1 hunks)components/roamresearch/actions/search-title/search-title.mjs(2 hunks)components/roamresearch/actions/write/write.mjs(1 hunks)components/roamresearch/common/constants.mjs(1 hunks)components/roamresearch/common/utils.mjs(1 hunks)components/roamresearch/package.json(1 hunks)components/roamresearch/roamresearch.app.mjs(2 hunks)components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- components/roamresearch/actions/add-content-to-daily-note-page/add-content-to-daily-note-page.mjs
- components/roamresearch/actions/add-content-to-page/add-content-to-page.mjs
- components/roamresearch/actions/add-content-underneath-block/add-content-underneath-block.mjs
- components/roamresearch/actions/get-page-or-block-data/get-page-or-block-data.mjs
- components/roamresearch/actions/query/query.mjs
- components/roamresearch/actions/search-title/search-title.mjs
- components/roamresearch/common/constants.mjs
- components/roamresearch/package.json
- components/roamresearch/roamresearch.app.mjs
- components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs
🔇 Additional comments (8)
components/roamresearch/actions/pull/pull.mjs (1)
22-44: 🛠️ Refactor suggestion
Add rate limiting and timeout handling.
The previous review comment about error handling is still valid. Additionally, consider implementing:
- Rate limiting to avoid API throttling
- Timeout handling for long-running requests
- Retry logic for transient failures
async run({ $ }) {
const {
app,
eid,
selector,
} = this;
+ const MAX_RETRIES = 3;
+ const TIMEOUT = 30000; // 30 seconds
+
+ const options = {
+ timeout: TIMEOUT,
+ retry: {
+ retries: MAX_RETRIES,
+ factor: 2,
+ minTimeout: 1000,
+ maxTimeout: 5000,
+ },
+ };
+
try {
- const response = await app.pull({
+ const response = await app.pull({
$,
data: {
eid,
selector,
},
+ ...options,
});
if (!response.result) {
$.export("$summary", `Failed to pull data for entity ID: \`${eid}\`.`);
return response;
}
$.export("$summary", `Successfully pulled data for entity ID: \`${eid}\`.`);
return response;
} catch (error) {
+ if (error.code === 'ETIMEDOUT') {
+ throw new Error(`Request timed out after ${TIMEOUT}ms`);
+ }
+ if (error.response?.status === 429) {
+ throw new Error('Rate limit exceeded. Please try again later.');
+ }
$.export("$summary", `Error pulling data: ${error.message}`);
throw error;
}
},components/roamresearch/actions/append-blocks/append-blocks.mjs (2)
1-2: LGTM: Imports are properly structured
The imports are correctly organized, importing utility functions and the app module from their respective locations.
4-9: Version number inconsistency with other components
The AI summary indicates version updates from "0.0.1" to "0.0.2" in several action files, but this new component starts at "0.0.1". Consider aligning the version with other components if they share the same feature set.
Let's check the versions of other Roam Research components:
components/roamresearch/common/utils.mjs (3)
3-24: The previous review comment about circular references is still applicable.
The current implementation could benefit from the suggested improvements for handling circular references, array types, and deep nesting.
49-52: The previous review comment about input validation is still applicable.
The current implementation could benefit from the suggested improvements for input validation and array index support.
1-57: Verify utility function usage across Roam Research components.
Let's verify how these utility functions are used across the new Roam Research components to ensure consistent usage patterns.
✅ Verification successful
Utility functions are correctly integrated and used across Roam Research components
The verification shows consistent usage patterns:
parseArrayis used in action components (write.mjs,append-blocks.mjs) to handle array inputsgetNestedPropertyis used in source component (new-modified-linked-reference.mjs) to safely access nested response data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of parseArray and getNestedProperty in Roam Research components
echo "Searching for parseArray usage:"
rg "parseArray" components/roamresearch/ -A 2
echo -e "\nSearching for getNestedProperty usage:"
rg "getNestedProperty" components/roamresearch/ -A 2
Length of output: 1911
components/roamresearch/actions/write/write.mjs (2)
1-10: LGTM! Module setup and metadata are well-structured.
The imports are appropriate, and the module metadata is properly defined with clear documentation links.
21-160: Verify handling of required fields.
While the properties are well-documented with required/optional markers in defaults, there's no explicit validation to ensure required fields are provided.
Let's check if validation is handled in the app class:
jcortes
force-pushed
the
roamresearch-new-generic-components-and-source
branch
from
2b6c3e4 to
70affcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
components/roamresearch/actions/pull/pull.mjs (2)
3-8: Enhance action metadata for better clarity and consistency.
- The description should explain what the action does before linking to documentation
- Version should be "0.0.2" to maintain consistency with other Roam Research components
Apply these changes:
export default { key: "roamresearch-pull", name: "Pull", - description: "Generic pull for Roam Research pages. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).", - version: "0.0.1", + description: "Retrieves data from Roam Research pages using the pull API. Allows querying specific entities with custom selectors. [See the documentation](https://roamresearch.com/#/app/developer-documentation/page/mdnjFsqoA).", + version: "0.0.2", type: "action",
14-14: Add examples aligned with PR objectives.The prop descriptions should include examples that demonstrate how to use this action for the specific use cases mentioned in the PR objectives, such as pulling linked references for pages and blocks.
Update the descriptions to include relevant examples:
- description: "The entity ID to pull. Eg. `[:block/uid \"08-30-2022\"]`.", + description: "The entity ID to pull. Examples:\n- Daily note: `[:block/uid \"08-30-2022\"]`\n- Page reference: `[:node/title \"Your Page Title\"]`\n- Block reference: `[:block/uid \"your-block-uid\"]`",- description: "The selector to pull. Eg. `[:block/uid :node/title :block/string {:block/children [:block/uid :block/string]} {:block/refs [:node/title :block/string :block/uid]}]`.", + description: "The selector to pull. Examples:\n- Get linked references: `[:block/uid :node/title {:block/_refs ...}]`\n- Get page content: `[:node/title :block/string {:block/children ...}]`\n- Get block data: `[:block/uid :block/string {:block/children ...}]`",Also applies to: 19-19
components/roamresearch/actions/write/write.mjs (4)
1-10: Update version number to match other components.The implementation looks good, but according to the AI summary, other action files have been updated from "0.0.1" to "0.0.2". Consider updating this file's version for consistency.
- version: "0.0.1", + version: "0.0.2",
13-19: Enhance action property description.The current description uses a generic example. Consider listing all available actions from
constants.ACTIONto make it more informative for users.- description: "The action to run. Eg. `create-block`.", + description: "The action to run. Available actions: create-block, move-block, update-block, delete-block, create-page, update-page, delete-page, batch-actions.",
182-182: Make success message more descriptive.The current success message is generic. Consider including the specific action that was performed.
- $.export("$summary", "Successfully ran the action."); + $.export("$summary", `Successfully executed ${action} action.`);
5-10: Consider adding dedicated action components.While this generic write implementation is flexible, the PR objectives mention specific actions like
add-daily-note-content,add-page-content, andget-block-data. Consider creating dedicated action components for these operations to provide a more user-friendly interface with specific props and validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
components/roamresearch/actions/add-content-to-daily-note-page/add-content-to-daily-note-page.mjs(2 hunks)components/roamresearch/actions/add-content-to-page/add-content-to-page.mjs(2 hunks)components/roamresearch/actions/add-content-underneath-block/add-content-underneath-block.mjs(2 hunks)components/roamresearch/actions/append-blocks/append-blocks.mjs(1 hunks)components/roamresearch/actions/get-page-or-block-data/get-page-or-block-data.mjs(2 hunks)components/roamresearch/actions/pull-many/pull-many.mjs(1 hunks)components/roamresearch/actions/pull/pull.mjs(1 hunks)components/roamresearch/actions/query/query.mjs(1 hunks)components/roamresearch/actions/search-title/search-title.mjs(2 hunks)components/roamresearch/actions/write/write.mjs(1 hunks)components/roamresearch/common/constants.mjs(1 hunks)components/roamresearch/common/utils.mjs(1 hunks)components/roamresearch/package.json(1 hunks)components/roamresearch/roamresearch.app.mjs(2 hunks)components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- components/roamresearch/actions/add-content-to-daily-note-page/add-content-to-daily-note-page.mjs
- components/roamresearch/actions/add-content-to-page/add-content-to-page.mjs
- components/roamresearch/actions/add-content-underneath-block/add-content-underneath-block.mjs
- components/roamresearch/actions/append-blocks/append-blocks.mjs
- components/roamresearch/actions/get-page-or-block-data/get-page-or-block-data.mjs
- components/roamresearch/actions/pull-many/pull-many.mjs
- components/roamresearch/actions/query/query.mjs
- components/roamresearch/actions/search-title/search-title.mjs
- components/roamresearch/common/constants.mjs
- components/roamresearch/common/utils.mjs
- components/roamresearch/package.json
- components/roamresearch/roamresearch.app.mjs
- components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs
🔇 Additional comments (2)
components/roamresearch/actions/pull/pull.mjs (2)
9-21: Previous review comment about props configuration is still applicable.
The existing review comment about enhancing props configuration with validation and improved examples is still valid. Please refer to the previous review comment for the detailed implementation suggestions.
22-44: 🛠️ Refactor suggestion
Previous review comment about error handling is still applicable.
The existing review comment about fixing error handling and improving success messaging is still valid. Please refer to the previous review comment for the detailed implementation suggestions.
Add response type validation.
The response handling could be improved by validating the response structure.
Add type validation before accessing the result:
const response = await app.pull({
$,
data: {
eid,
selector,
},
});
+ if (!response || typeof response !== 'object') {
+ throw new Error('Invalid response received from Roam Research API');
+ }
+
if (!response.result) {
$.export("$summary", `Failed to pull data for entity ID: \`${eid}\`.`);
return response;
}
components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs
Outdated
Show resolved
Hide resolved
…new-modified-linked-reference.mjs Co-authored-by: michelle0927 <[email protected]>
jcortes
force-pushed
the
roamresearch-new-generic-components-and-source
branch
from
35e3fd8 to
77fa3cf
Compare
|
/approve |
* [Components] roamresearch - Added new components * Update components/roamresearch/sources/new-modified-linked-reference/new-modified-linked-reference.mjs Co-authored-by: michelle0927 <[email protected]> --------- Co-authored-by: michelle0927 <[email protected]>
WHY
Resolves #14438
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes