Add TTL functionality to Data Store actions #15824
Conversation
- Added TTL prop to all Data Store actions to support record expiration - Created helper methods for human-readable TTL formatting - Added a new "Update record expiration" action for modifying TTL on existing records - Added preset TTL options for common time periods - Improved summaries and return values with TTL information 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe changes add support for a time-to-live (TTL) parameter across several data store actions. Each modified action now accepts a new Changes
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
components/data_stores/actions/delete-single-record/delete-single-record.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/data_stores/actions/get-all-records/get-all-records.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/data_stores/actions/delete-all-records/delete-all-records.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
components/data_stores/actions/get-record-or-create/get-record-or-create.mjs (1)
72-80: Return structure inconsistency could cause integration issuesThe return structure changes based on whether TTL is present, which differs from other actions and could make integration more challenging for consumers.
Consider using a consistent return structure regardless of whether TTL is present:
- // Include TTL information in the return value if it was set - if (this.ttl) { - return { - value: parsedValue, - ttl: this.ttl, - }; - } - - return parsedValue; + // Always return a consistent structure + return { + value: parsedValue, + ttl: this.ttl || null, + };This would make the return value format consistent with other TTL-enabled actions, making it easier for consumers to work with the API.
components/data_stores/actions/add-update-multiple-records/add-update-multiple-records.mjs (1)
112-116: Return structure uses inconsistent property namingThe return structure includes TTL with an underscore prefix (
_ttl), which is inconsistent with how TTL is returned in other actions. This could confuse users.Consider using a consistent property name across all actions:
- // Include TTL in the returned map - return { - ...map, - _ttl: this.ttl || null, - }; + // Use consistent property naming for TTL across all actions + return { + ...map, + ttl: this.ttl || null, + };components/data_stores/actions/update-ttl/update-ttl.mjs (5)
7-7: Consider adding a detailed version changelog when creating new components.This is a new component at version 0.0.1, which is appropriate. I recommend adding a comment explaining what functionality this initial version provides for better version tracking.
25-26: Description could be more specific about updating TTL.The current description focuses on the key selection but doesn't mention that this is specifically for TTL updates.
- description: "Select the key for the record you'd like to update the expiration time.", + description: "Select the key for the record you'd like to update the TTL (Time-To-Live) expiration time for.",
91-97: Add try-catch for error handling during record existence check.The code checks if a record exists but doesn't handle potential errors that might occur during this check.
- if (!await this.dataStore.has(key)) { + try { + const recordExists = await this.dataStore.has(key); + if (!recordExists) { + $.export("$summary", `No record found with key \`${key}\`.`); + return { + success: false, + message: `No record found with key ${key}`, + }; + } + } catch (error) { + $.export("$summary", `Error checking record with key \`${key}\`: ${error.message}`); + return { + success: false, + message: `Error checking record: ${error.message}`, + }; + }
99-108: Add try-catch for TTL removal operations.Error handling is missing when removing the TTL.
if (ttlValue === 0) { // Remove expiration - await this.dataStore.setTtl(key, null); - $.export("$summary", `Successfully removed expiration for key \`${key}\`.`); - return { - success: true, - key, - ttl: null, - message: "Expiration removed", - }; + try { + await this.dataStore.setTtl(key, null); + $.export("$summary", `Successfully removed expiration for key \`${key}\`.`); + return { + success: true, + key, + ttl: null, + message: "Expiration removed", + }; + } catch (error) { + $.export("$summary", `Error removing expiration for key \`${key}\`: ${error.message}`); + return { + success: false, + key, + message: `Error removing expiration: ${error.message}`, + }; + }
110-119: Add try-catch for TTL update operations.Error handling is missing when updating the TTL.
} else { // Update TTL - await this.dataStore.setTtl(key, ttlValue); - $.export("$summary", `Successfully updated expiration for key \`${key}\` (expires in ${this.app.formatTtl(ttlValue)}).`); - return { - success: true, - key, - ttl: ttlValue, - ttlFormatted: this.app.formatTtl(ttlValue), - }; + try { + await this.dataStore.setTtl(key, ttlValue); + $.export("$summary", `Successfully updated expiration for key \`${key}\` (expires in ${this.app.formatTtl(ttlValue)}).`); + return { + success: true, + key, + ttl: ttlValue, + ttlFormatted: this.app.formatTtl(ttlValue), + }; + } catch (error) { + $.export("$summary", `Error updating expiration for key \`${key}\`: ${error.message}`); + return { + success: false, + key, + message: `Error updating expiration: ${error.message}`, + }; + }components/data_stores/actions/add-update-record/add-update-record.mjs (2)
49-55: Simplify conditional logic for setting records with TTL.The current implementation creates duplicate code with an if-else block. This can be simplified using an options object.
- if (ttl) { - await this.dataStore.set(key, parsedValue, { - ttl, - }); - } else { - await this.dataStore.set(key, parsedValue); - } + const options = ttl ? { ttl } : undefined; + await this.dataStore.set(key, parsedValue, options);
40-45: Add try-catch blocks for error handling.The code should have error handling for the data store operations.
async run({ $ }) { const { key, value, ttl, } = this; - const exists = await this.dataStore.has(key); - const parsedValue = this.app.parseValue(value); - - if (ttl) { - await this.dataStore.set(key, parsedValue, { - ttl, - }); - } else { - await this.dataStore.set(key, parsedValue); - } + try { + const exists = await this.dataStore.has(key); + const parsedValue = this.app.parseValue(value); + + const options = ttl ? { ttl } : undefined; + await this.dataStore.set(key, parsedValue, options); + + // eslint-disable-next-line multiline-ternary + $.export("$summary", `Successfully ${exists ? "updated the record for" : "added a new record with the"} key, \`${key}\`${ttl ? ` (expires in ${this.app.formatTtl(ttl)})` : ""}.`); + return { + key, + value: parsedValue, + ttl: ttl || null, + }; + } catch (error) { + $.export("$summary", `Error ${exists ? "updating" : "adding"} record with key \`${key}\`: ${error.message}`); + throw error; + }components/data_stores/data_stores.app.mjs (4)
77-108: Simplify the formatTtl method for better readability.The current implementation uses nested ternary operators for pluralization, which can be hard to read. Consider extracting a helper function for pluralization.
formatTtl(seconds) { if (!seconds) return ""; + + // Helper function for pluralization + const pluralize = (value, unit) => `${value} ${unit}${value === 1 ? "" : "s"}`; // Format TTL in a human-readable way if (seconds < 60) { - return `${seconds} second${seconds === 1 - ? "" - : "s"}`; + return pluralize(seconds, "second"); } if (seconds < 3600) { const minutes = Math.round(seconds / 60); - return `${minutes} minute${minutes === 1 - ? "" - : "s"}`; + return pluralize(minutes, "minute"); } if (seconds < 86400) { const hours = Math.round(seconds / 3600); - return `${hours} hour${hours === 1 - ? "" - : "s"}`; + return pluralize(hours, "hour"); } if (seconds < 604800) { const days = Math.round(seconds / 86400); - return `${days} day${days === 1 - ? "" - : "s"}`; + return pluralize(days, "day"); } const weeks = Math.round(seconds / 604800); - return `${weeks} week${weeks === 1 - ? "" - : "s"}`; + return pluralize(weeks, "week"); },
79-79: Use more specific condition check for formatTtl.The condition
if (!seconds)will return an empty string for0as well asundefinedornull. Since0is a valid TTL value (to remove expiration), it would be more appropriate to use a more specific check.- if (!seconds) return ""; + if (seconds === undefined || seconds === null) return "";
77-108: Consider adding larger time units for better readability.The current implementation only goes up to weeks. For larger TTL values, it would be useful to add months and years.
formatTtl(seconds) { if (!seconds) return ""; // Format TTL in a human-readable way if (seconds < 60) { return `${seconds} second${seconds === 1 ? "" : "s"}`; } if (seconds < 3600) { const minutes = Math.round(seconds / 60); return `${minutes} minute${minutes === 1 ? "" : "s"}`; } if (seconds < 86400) { const hours = Math.round(seconds / 3600); return `${hours} hour${hours === 1 ? "" : "s"}`; } if (seconds < 604800) { const days = Math.round(seconds / 86400); return `${days} day${days === 1 ? "" : "s"}`; } + if (seconds < 2592000) { // Less than 30 days const weeks = Math.round(seconds / 604800); return `${weeks} week${weeks === 1 ? "" : "s"}`; + } + if (seconds < 31536000) { // Less than 1 year + const months = Math.round(seconds / 2592000); + return `${months} month${months === 1 + ? "" + : "s"}`; + } + // 1 year or more + const years = Math.round(seconds / 31536000); + return `${years} year${years === 1 + ? "" + : "s"}`; },
109-117: Add error handling to updateTtlIfNeeded method.The method doesn't handle potential errors when calling data store methods.
async updateTtlIfNeeded(dataStore, key, ttl) { if (!ttl) return false; - if (await dataStore.has(key)) { - await dataStore.setTtl(key, ttl); - return true; + try { + if (await dataStore.has(key)) { + await dataStore.setTtl(key, ttl); + return true; + } + return false; + } catch (error) { + console.error(`Error updating TTL for key ${key}: ${error.message}`); + throw error; } - return false; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
components/data_stores/actions/add-update-multiple-records/add-update-multiple-records.mjs(3 hunks)components/data_stores/actions/add-update-record/add-update-record.mjs(2 hunks)components/data_stores/actions/append-to-record/append-to-record.mjs(3 hunks)components/data_stores/actions/get-record-or-create/get-record-or-create.mjs(3 hunks)components/data_stores/actions/has-key-or-create/has-key-or-create.mjs(3 hunks)components/data_stores/actions/update-ttl/update-ttl.mjs(1 hunks)components/data_stores/data_stores.app.mjs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
🔇 Additional comments (24)
components/data_stores/actions/append-to-record/append-to-record.mjs (4)
8-8: Version incremented appropriately to reflect new functionalityThe version has been incremented from 0.0.2 to 0.0.3, which is appropriate for adding a non-breaking feature enhancement.
34-39: TTL property added successfullyThe TTL property is properly integrated, referencing the shared propDefinition from the app module.
54-65: Well-structured conditional logic for TTL handlingGood implementation of conditional TTL handling with appropriate branching logic. The human-readable TTL format in the summary message will improve user experience.
69-71: Consistent return structure with TTL informationThe return structure correctly includes the TTL information, with a null fallback when TTL isn't provided.
components/data_stores/actions/get-record-or-create/get-record-or-create.mjs (3)
7-7: Version incremented appropriatelyThe version has been correctly incremented from 0.0.10 to 0.0.11 to reflect the addition of TTL functionality.
33-38: TTL property added consistently with other actionsThe TTL property is added with the same structure as in other actions, maintaining consistency across the codebase.
61-70: TTL handling implemented correctly for record creationThe conditional setting of TTL when creating a new record is well-implemented, with appropriate messaging.
components/data_stores/actions/add-update-multiple-records/add-update-multiple-records.mjs (4)
7-7: Version incremented appropriatelyVersion has been incremented from 0.0.6 to 0.0.7, which is appropriate for adding TTL functionality.
22-27: TTL property added consistentlyThe TTL property is correctly added, referencing the common propDefinition from the app module.
92-102: Promise creation with TTL handlingThe implementation correctly creates promises for setting records with TTL options when specified.
109-109: Summary message enhanced with TTL informationThe summary message has been updated to include TTL information when applicable, providing better feedback to users.
components/data_stores/actions/has-key-or-create/has-key-or-create.mjs (4)
7-7: Version updated to 0.1.3Version has been updated to 0.1.3, which is appropriate for adding TTL functionality. Note that this action is at a different version number scheme compared to the others (0.1.x vs 0.0.x).
33-38: TTL property added consistentlyThe TTL property is correctly integrated, keeping consistency with other actions.
67-76: TTL conditional handling for record creationThe implementation correctly handles TTL when creating a new record, including proper user feedback.
78-83: Return structure includes TTL information consistentlyThe return structure correctly includes TTL information, with a null fallback when TTL isn't provided, which is consistent with most other actions.
components/data_stores/actions/update-ttl/update-ttl.mjs (3)
27-65: Well-designed TTL option selection with appropriate presets.The TTL options provided offer a good range of common time periods with their corresponding seconds values. The inclusion of a "Custom value" option with the reloadProps feature is a nice touch for flexibility.
76-76: Maximum TTL limit is well-documented.The max TTL of 2 years (63072000 seconds) is a reasonable upper limit and is clearly commented, which helps future developers understand the constraint.
86-90: Proper TTL value determination with parsing.The code correctly determines the TTL value based on user selection, parsing string values to integers when needed. This handles both custom and preset options appropriately.
components/data_stores/actions/add-update-record/add-update-record.mjs (5)
7-7: Version increment is appropriate.The version has been incremented from 0.0.9 to 0.0.10, which is correct for this feature addition.
33-38: TTL property added correctly.The TTL property has been properly added using the app's propDefinition, which ensures consistency across components.
44-44: TTL value correctly destructured.The TTL value is correctly destructured from the component's properties.
58-58: Summary message correctly includes TTL information.The summary message has been appropriately updated to include TTL information when provided.
62-62: TTL value included in response.The TTL value is correctly included in the response, defaulting to null if not provided.
components/data_stores/data_stores.app.mjs (1)
30-37: TTL property defined with appropriate constraints.The TTL property is well-defined with clear description, appropriate type, and reasonable constraints (min: 0, max: 63072000 seconds). Making it optional allows for flexibility in record management.
dannyroosevelt
moved this to Ready for PR Review
in Component (Source and Action) Backlog
GTFalcao
moved this from Ready for PR Review
to Ready for QA
in Component (Source and Action) Backlog
vunguyenhung
moved this from Ready for QA
to In QA
in Component (Source and Action) Backlog
vunguyenhung
moved this from In QA
to Changes Required
in Component (Source and Action) Backlog
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
Co-authored-by: Guilherme Falcão <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/data_stores/data_stores.app.mjs (1)
77-108: Consider using Math.floor instead of Math.round for time unit conversions.While the
formatTtlfunction works well for converting seconds to human-readable formats, usingMath.roundmight lead to slightly misleading results in edge cases (e.g., 59 minutes would round to "1 hour").Consider using
Math.floorinstead ofMath.roundfor more accurate time representations:- const minutes = Math.round(seconds / 60); + const minutes = Math.floor(seconds / 60); - const hours = Math.round(seconds / 3600); + const hours = Math.floor(seconds / 3600); - const days = Math.round(seconds / 86400); + const days = Math.floor(seconds / 86400); - const weeks = Math.round(seconds / 604800); + const weeks = Math.floor(seconds / 604800);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
components/data_stores/actions/delete-all-records/delete-all-records.mjs(1 hunks)components/data_stores/actions/delete-single-record/delete-single-record.mjs(1 hunks)components/data_stores/actions/get-all-records/get-all-records.mjs(1 hunks)components/data_stores/actions/get-difference/get-difference.mjs(1 hunks)components/data_stores/actions/get-record-keys/get-record-keys.mjs(1 hunks)components/data_stores/actions/list-keys/list-keys.mjs(1 hunks)components/data_stores/actions/list-records/list-records.mjs(1 hunks)components/data_stores/actions/update-ttl/update-ttl.mjs(1 hunks)components/data_stores/data_stores.app.mjs(2 hunks)components/data_stores/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- components/data_stores/package.json
- components/data_stores/actions/get-all-records/get-all-records.mjs
- components/data_stores/actions/list-keys/list-keys.mjs
- components/data_stores/actions/list-records/list-records.mjs
- components/data_stores/actions/delete-all-records/delete-all-records.mjs
- components/data_stores/actions/get-record-keys/get-record-keys.mjs
- components/data_stores/actions/get-difference/get-difference.mjs
- components/data_stores/actions/delete-single-record/delete-single-record.mjs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (8)
components/data_stores/actions/update-ttl/update-ttl.mjs (6)
5-5: LGTM: The name is properly capitalized.The action name "Update Record Expiration" is clear and follows proper capitalization.
27-65: Well-structured TTL options with clear labels and corresponding values.The implementation provides a good selection of preset expiration times along with custom options. The values are properly represented in seconds with clear human-readable labels.
68-80: Good implementation of dynamic property addition.The
additionalPropsmethod correctly adds a custom TTL field only when needed, with appropriate validation constraints.
91-97: Proper error handling for non-existent records.The code appropriately checks if a record exists before attempting to update its TTL, providing a clear error message when the record is not found.
99-119: Well-handled TTL update logic with clear success messages.The code properly handles both removing expiration (TTL=0) and setting new TTL values. The success messages include formatted TTL values for better readability, and the return values contain all necessary information.
112-112: Nice use of formatted TTL in summary message.Using
formatTtlin the summary message enhances readability by showing the expiration time in a human-friendly format.components/data_stores/data_stores.app.mjs (2)
30-37: Well-defined TTL property with clear constraints.The TTL property is properly defined with a clear label, description, and appropriate min/max constraints. The examples in the description help users understand common time periods.
109-117: Efficient TTL update implementation.The
updateTtlIfNeededmethod is efficiently implemented, checking for the existence of TTL and the record before attempting to update. The return values provide clear feedback on whether an update was performed.
github-project-automation
bot
moved this from Changes Required
to Done
in Component (Source and Action) Backlog
I think the behavior you cited for the update action is actually expected. You updated the value but didn't explicitly set a ttl on that update, so there's no ttl set. Would you expect the ttl to be preserved in that case? Would you expect it to get reset or honor the original ttl? I believe technically when you "update" a record, it's just writing a nw record with the same key. |
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Version Updates