Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for passing a shared temporary ID map to safe output handlers, specifically to enable chaining from create_issue to update_project operations. The change ensures that temporary IDs created by one handler (e.g., issue creation) can be resolved by subsequent handlers (e.g., project updates).
Changes:
- Modified
safe_output_handler_manager.cjsto passtemporaryIdMapas a third parameter to all message handlers - Updated
update_project.cjshandler to accept and use the sharedtemporaryIdMapparameter - Added test coverage to verify the temporary ID map is correctly passed between handlers
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| actions/setup/js/safe_output_handler_manager.cjs | Updated handler invocations to pass temporaryIdMap as third parameter in both initial processing loop and deferred retry loop |
| actions/setup/js/update_project.cjs | Modified handler signature to accept temporaryIdMap parameter, updated import to use loadTemporaryIdMapFromResolved, and added logic to create local map from either parameter |
| actions/setup/js/safe_output_handler_manager.test.cjs | Added test case verifying that the shared temporary ID map is correctly passed to handlers during message processing |
Comments suppressed due to low confidence (1)
actions/setup/js/update_project.cjs:1177
- The JSDoc comment has the parameters documented in a different order than they appear in the function signature. The documentation shows
resolvedTemporaryIdsfirst (line 1176) andtemporaryIdMapsecond (line 1177), but the function signature has them in the same order (line 1180). However, this order is inconsistent with similar handlers. When this parameter order issue is resolved, ensure the JSDoc parameter order matches the updated signature.
* @param {Object} resolvedTemporaryIds - Plain object version of temporaryIdMap for backward compatibility
* @param {Map<string, {repo?: string, number?: number, projectUrl?: string, draftItemId?: string}>} temporaryIdMap - Unified map of temporary IDs
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @returns {Promise<Object>} Result with success/error status, and optionally temporaryId/draftItemId for draft issue creation | ||
| */ | ||
| return async function handleUpdateProject(message, temporaryIdMap, resolvedTemporaryIds = {}) { | ||
| return async function handleUpdateProject(message, resolvedTemporaryIds = {}, temporaryIdMap = null) { |
There was a problem hiding this comment.
The parameter order in this handler function is inconsistent with other similar handlers in the codebase. The create_project.cjs and create_project_status_update.cjs handlers use the signature (message, temporaryIdMap, resolvedTemporaryIds) where temporaryIdMap (the Map) comes second and resolvedTemporaryIds (the plain object) comes third. This handler uses (message, resolvedTemporaryIds, temporaryIdMap) which reverses the order. This inconsistency could lead to confusion and bugs when maintaining or extending the code. Consider aligning with the convention used in create_project.cjs at line 328.
This issue also appears on line 1176 of the same file.
See below for a potential fix:
* @param {Map<string, {repo?: string, number?: number, projectUrl?: string, draftItemId?: string}>} temporaryIdMap - Unified map of temporary IDs
* @param {Object} resolvedTemporaryIds - Plain object version of temporaryIdMap for backward compatibility
* @returns {Promise<Object>} Result with success/error status, and optionally temporaryId/draftItemId for draft issue creation
*/
return async function handleUpdateProject(message, temporaryIdMap = null, resolvedTemporaryIds = {}) {
create_issue→update_projectchaining without widening temp-ID support.