Add order to SMARTBLOCK#131
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe SMARTBLOCK command was updated to support an optional third parameter for specifying the insertion order of new blocks when executed remotely. Documentation was revised to describe this new parameter and provide usage examples. The command handler and related logic were updated to parse and apply the order argument, and type definitions were extended accordingly. Changes
Possibly related PRs
Estimated code review effort2 (45 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
docs/050-command-reference.md (1)
1101-1108: Clarify index semantics & default behavior of the new “order” parameterThe docs introduce the
orderargument but omit two critical details:
- Whether the index is 0-based (0 = top) or 1-based (1 = top).
- What happens when the value is greater than the current block count or omitted entirely (fallback to append?).
Ambiguity here will lead to off-by-one errors and inconsistent usage across workflows. Please tighten the wording, e.g.:
-3. When the second parameter is set, you may optionally pass a number to specify the order of the new block on that page. +3. (Optional) **Order** – zero-based index of the position to insert the new block on the target page. + • `0` inserts at the top, `1` after the first block, etc. + • If the index ≥ current block count, the block is appended at the bottom. + • If omitted, the block is appended by default.And mirror this clarification in the example:
-<%SMARTBLOCK:Daily,Some Page,2%> +<%SMARTBLOCK:Daily,Some Page,2%> <!-- inserts as the third block -->This keeps the reference unambiguous and aligned with the implementation in
sbBomb.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/050-command-reference.md(1 hunks)src/utils/core.ts(4 hunks)
🔇 Additional comments (3)
src/utils/core.ts (3)
1769-1769: Documentation update looks good.The help text clearly describes the new optional order parameter and follows the established documentation pattern.
1794-1798: Target parameter construction looks correct.The conditional inclusion of the order property using the spread operator is well-implemented and maintains clean object structure.
2658-2658: Order implementation in sbBomb is well-structured.The optional order parameter is properly handled with appropriate fallbacks and type checking. The logic correctly uses the provided order when available while maintaining existing behavior as the default.
Also applies to: 2670-2670, 2706-2708
… parameter in the format `order=value`. Update examples to reflect new usage and improve argument parsing for page names and order specification.
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/utils/core.ts (1)
1770-1851: Improve parameter parsing robustness and add validation.While the variadic arguments approach maintains backward compatibility, the parameter parsing logic has several areas for improvement:
- Fragile order detection: Line 1787 uses
lastArg.includes("=")which could incorrectly match page names containing "="- Missing order validation: No validation that the order value is a valid number or "last"
- Assumption about space separation: Line 1790 joins with spaces, which may not be the original format
Consider this more robust approach:
- // Multiple arguments - last one might be order=value - const lastArg = pageNameOrUid[pageNameOrUid.length - 1]; - if (lastArg.includes("=")) { - // Last argument is order=value, rest is page name - const orderValue = lastArg.split("=")[1]; - title = extractTag(pageNameOrUid.slice(0, -1).join(" ")); - order = - orderValue.toLowerCase() === "last" - ? "last" - : Number(orderValue); - } else { - // All arguments are page name - title = extractTag(pageNameOrUid.join(" ")); - } + // More robust order parsing + const lastArg = pageNameOrUid[pageNameOrUid.length - 1]; + const orderMatch = /^order=(.+)$/i.exec(lastArg); + + if (orderMatch) { + const orderValue = orderMatch[1]; + title = extractTag(pageNameOrUid.slice(0, -1).join(",")); + + if (orderValue.toLowerCase() === "last") { + order = "last"; + } else { + const numOrder = Number(orderValue); + if (!isNaN(numOrder) && Number.isInteger(numOrder) && numOrder >= 0) { + order = numOrder; + } else { + // Invalid order value, treat as part of page name + title = extractTag(pageNameOrUid.join(",")); + } + } + } else { + title = extractTag(pageNameOrUid.join(",")); + }
🧹 Nitpick comments (2)
docs/050-command-reference.md (2)
1101-1102: Clarify whenorder=is valid
The sentence implies theorder=parameter is only permitted when a page name (second parameter) is present.
However, one of the examples below showsorder=being used while the page parameter is intentionally left blank (line 1109 –<%SMARTBLOCK:MyWorkflow,,order=3%>). Please tighten the wording or update the example so that the documented rule and examples do not contradict each other.
1105-1110: Example set has internal inconsistencies
- Line 1109 demonstrates an empty page parameter followed by
order=3, which conflicts with the rule above.- Line 1108 uses a raw UID
V_MTxadSWwhile line 1107 encloses the same UID in double-parens. Calling out the semantic difference (page UID vs block UID) in a short comment would prevent reader confusion.- Consider adding an explicit numeric order example other than
0(e.g.order=2) and making it clear that indices are zero-based.A concise, self-contained example list might look like:
- `<%SMARTBLOCK:MyWorkflow%>` <!-- run in-place --> - `<%SMARTBLOCK:MyWorkflow,Some Page%>` <!-- run on page --> - `<%SMARTBLOCK:MyWorkflow,Some Page,order=last%>` <!-- append --> - `<%SMARTBLOCK:MyWorkflow,Some Page,order=2%>` <!-- 3rd position -->This both removes the contradictory empty parameter case and highlights practical usages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/050-command-reference.md(1 hunks)src/utils/core.ts(5 hunks)
🔇 Additional comments (4)
src/utils/core.ts (4)
1769-1769: LGTM! Clear documentation for the new order parameter.The help text effectively documents the new optional order parameter with clear usage instructions and format specification.
2675-2675: LGTM! Well-defined type for the order parameter.The optional
order?: number | "last"property correctly represents the new functionality while maintaining backward compatibility.Also applies to: 2687-2687
2722-2725: LGTM! Correct logic for determining starting order.The conditional logic properly handles explicit order values while maintaining backward compatibility with the default behavior.
2767-2770: LGTM! Proper handling of subsequent block ordering.The logic correctly handles both "last" and numeric order values for creating multiple blocks in the proper sequence.
…gument parsing. Use regex for order extraction and validate numeric values, ensuring robust title extraction from page names.
1 participant
Summary
sbBombwhen running SMARTBLOCKsTesting
npm test(fails: samepage not found)https://chatgpt.com/codex/tasks/task_e_6869c48c140c8326a8f5dd83cc22ccb3
Summary by CodeRabbit
New Features
Documentation