Skip to content

fix: correct CraftQueue add recipe behavior for button and work orders#1059

Open
netouss wants to merge 7 commits intomainfrom
fix/craftqueue-add-recipe
Open

fix: correct CraftQueue add recipe behavior for button and work orders#1059
netouss wants to merge 7 commits intomainfrom
fix/craftqueue-add-recipe

Conversation

@netouss
Copy link
Collaborator

@netouss netouss commented Mar 16, 2026

Problem

Two bugs prevented recipes from being added to the CraftQueue:

  1. + CraftQueue button / QueueOpenRecipe: Calling CRAFTQ:AddRecipe() (the module wrapper) caused queueSize=0 after the call. The wrapper is called from a protected WoW script boundary (button OnClick) which swallows errors silently.

  2. Sub-recipes immediately removed: After a recipe with self-crafted reagents was added, its sub-recipes were removed on the very next display update. UpdateCountByParentRecipes always found parentCraftQueueItems: 0 because FindRecipeByParentRecipeInfo used concentrating as the 4th key segment, while recipeCrafterMap stores entries keyed by GetRecipeCraftQueueUID which uses orderID.

Fix

  • Bypass CRAFTQ:AddRecipe wrapper in QueueOpenRecipe (all 3 call sites) and QueueWorkOrders, calling craftQueue:AddRecipe() directly — same proven pattern.
  • Add orderID field to ParentRecipeInfo (populated in CreateParentRecipeInfo).
  • Update FindRecipeByParentRecipeInfo to use orderID or 0 as the 4th key segment, matching GetRecipeCraftQueueUID exactly.

- Bypass CRAFTQ:AddRecipe wrapper in QueueOpenRecipe (3 call sites)
  and QueueWorkOrders, calling craftQueue:AddRecipe() directly to
  prevent queue size reset after adding items
- Fix FindRecipeByParentRecipeInfo key mismatch: was using
  'concentrating' as the 4th key segment but recipeCrafterMap stores
  keys using 'orderID' (from GetRecipeCraftQueueUID). Sub-recipes were
  immediately removed every display update because parentCraftQueueItems
  was always 0.
- Add orderID field to ParentRecipeInfo and update CreateParentRecipeInfo
  to populate it, so FindRecipeByParentRecipeInfo key matches exactly.
Comment on lines +356 to +358
local aProfitOk = a.recipeData.averageProfitCached ~= nil
local bProfitOk = b.recipeData.averageProfitCached ~= nil
if not aProfitOk or not bProfitOk then
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use more verbose variable names. It should be immediately clear what the purpose or content of this is by just looking at the name

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed aProfitOk/bProfitOk to aHasCachedAverageProfit/bHasCachedAverageProfit in FilterSortByPriority for clarity.

Comment on lines +414 to +420
CraftSim.CRAFTQ.craftQueue = CraftSim.CRAFTQ.craftQueue or CraftSim.CraftQueue()
CraftSim.CRAFTQ.craftQueue:AddRecipe({ recipeData = recipeData })
CraftSim.CRAFTQ.UI:UpdateQueueDisplay()
if CraftSim.DB.OPTIONS:Get("CRAFTQUEUE_AUTO_SHOW") then
CraftSim.DB.OPTIONS:Save("MODULE_CRAFT_QUEUE", true)
CraftSim.CRAFTQ.frame:Show()
CraftSim.CRAFTQ.frame:Raise()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems all those wrapper bypasses do the same.
I would like the wrapper to be updated and accepting an additional optional flag if specific behavior is needed instead of adding indentical wrapper replacements in various places

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root cause of the original bypass was WoW taint: CraftSim.CRAFTQ is touched by secure callbacks (C_FunctionContainers.CreateCallback for work orders, hooksecurefunc for auction house), which taints the CraftSim.CRAFTQ.AddRecipe reference stored in the global table. Calling it from those contexts was silently swallowed by WoW's taint system — the bypass worked by accident because it never went through the tainted global reference.

@netouss netouss requested review from Copilot and derfloh205 and removed request for Copilot March 19, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants