[WhereisFff]

fix(menu): 修复旁观者菜单预览

• 允许旁观者查看溜槽、批量加工等方块菜单

• 补齐机器菜单的位置、配置数据和多方块入口

• 通过菜单初始化同步创造发电机功率与特斯拉塔过滤配置

• fixed [Bug] 旁观模式下 右键溜槽显示“无法打开：战利品未生成” 而非打开ui #6055

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

[Gugle2308]

Now the key: MenuType.create(id, inv, buf) in the Advanced payload path — need to check AdvancedOpenScreenPayload client handler passes buf. Let me look at the 26.1 (or 1.21.1) neoforge AdvancedOpenScreenPayload handler and the patched MenuScreens path with extra data.

Actually, the crucial check: for the client-side, NeoForge patches ClientboundOpenScreenPacket handler to pass extra data. Let's check AdvancedOpenScreenPayload in neoforge jar 21.1.238 - the earlier javap gave no output, maybe wrong class name. Let me list.

Actually, more important is whether the 26.1 target version of the project behaves the same. The project targets dev/26.1/1.6 with NeoForge 26.1.x. I've been checking 1.21.1 NeoForge, which may differ. But the fundamental mechanism is the same.

Let me now focus. I have limited time budget. Key findings so far:

Confirmed facts:

1. Vanilla 26.1 ServerPlayerGameMode.useItemOn has SPECTATOR branch: getMenuProvider → non-null → openMenu → CONSUME success; null → PASS. So spectators can open any menu whose block returns a MenuProvider.
2. BaseEntityBlock.getMenuProvider returns BE if it implements MenuProvider. BlockBehaviour.getMenuProvider default returns null.
3. The PR: removes if (player.isSpectator()) return null; from 14 BEs' createMenu, adds writeClientSideData to 11 BEs (incl. BaseMachineBlockEntity which covers Chute/MagneticChute/BatchCrafter/BatchCutter subclasses), adds getMenuProvider overrides to 4 blocks (TeslaTowerBlock, AdvancedComparatorBlock, PulseGeneratorBlock, ... let me count). Wait: diff has getMenuProvider in TeslaTowerBlock, AdvancedComparatorBlock, PulseGeneratorBlock. That's 3.

Critical defect candidates:

(A) Double-write for AdvancedComparator/PulseGenerator: The blocks' useWithoutItem still has if (SPECTATOR) return PASS AND still calls sp.openMenu(blockEntity, buf -> {writeBlockPos(pos); writeNbt(nbt);}). Now that the BE also has writeClientSideData (writing BlockPos + NBT), a non-spectator opening the menu will get: writeClientSideData (pos + nbt) then the extraDataWriter (pos + nbt) = 2 × (pos+nbt). The menu reads readBlockPos() then readNbt(). With duplicated data: first readBlockPos consumes bytes 1-8 (first pos) ✓; readNbt then reads the NBT ✓ (the first one); remaining bytes (second pos + second nbt) unread — harmless leftovers? Actually wait: order is writeClientSideData first, then extraDataWriter. So byte stream = [pos][nbt][pos][nbt]. Menu reads pos (1st), then nbt (1st). Leftover ignored. So functionally OK, just duplicate bytes! Hmm — so it's NOT corrupt. It's wasteful but consistent. Good catch — need to not over-report.

But wait — for AdvancedComparatorBlock, the spectator still returns PASS from useWithoutItem, so spectator preview for comparator/pulse generator still broken. But actually now with getMenuProvider override, the vanilla SPECTATOR branch in useItemOn will fire first! Because useItemOn is called by ServerPlayerGameMode.useItemOn, and in SPECTATOR mode the game mode intercepts BEFORE calling blockState.useItemOn. Look at bytecode: at 34-73, if gameModeForPlayer == SPECTATOR, it calls getMenuProvider and returns — it never reaches BlockState.useItemOn. So the block's own useWithoutItem spectator PASS is irrelevant for spectator right-click! The vanilla branch handles it.

So the previously-identified "double write" (A) is actually the key real issue? Let's re-examine: for non-spectator, useWithoutItem runs, calling sp.openMenu(be, consumer). Now that BE has writeClientSideData writing pos+nbt, and consumer writes pos+nbt again ⇒ duplicated but parse-compatible (leftover trailing bytes ignored). So it's a 💡 (redundant bytes) not a 🔴.

Hmm, but wait — actually for the comparator/pulse: is it doubled or is it now a mismatch? AdvancedComparatorMenu.readComparator reads BlockPos then readNbt. Stream: pos1, nbt1, pos2, nbt2. Read pos1 → correct. Read nbt1 → correct. OK. So consistent. 💡 redundant.

For TeslaTower: useWithoutItem still sends PacketDistributor.sendToPlayer(sp, TeslaFilterSyncPacket.create(...)) AND writeClientSideData writes the same payload into extraData; and TeslaTowerMenu decodes it. So for non-spectator, the client gets the filters twice (once via menu construction, once via packet) → handleSync called twice, both write into client BE. Idempotent, wasteful. 💡.

Also TeslaTowerScreen.init() now calls this.handleSync(...menu.getBlockEntity().getAllowList()) — reading the client BE's allowList. For a spectator, the client BE exists? Spectators get chunk data so BE exists but... does the client BE have the allowList synced? handleSync in menu ctor already wrote it via blockEntity.handleSync(filters). But careful: TeslaTowerMenu.handleSync delegates to this.blockEntity.handleSync(filters) which requires this.level != null and looks up the main part BE. On the client, that resolves to a client BE and mutates its allowList + setChanged(). That's a client-side world-state write (setChanged on client BE is harmless-ish). But there's a subtlety: for a spectator, opening on a sub-part position: writeClientSideData writes this.getBlockPos() — the BE's own pos = main part pos (since only main part has BE). Then menu ctor reads that pos → getBlockEntity(pos) → main part BE ✓.

(B) StructureScannerBlockEntity: no writeClientSideData but StructureScannerMenu extraData ctor reads extraData.readBlockPos(). StructureScannerBlock extends BaseEntityBlock → default getMenuProvider returns BE → spectator now opens it. openMenu → writeClientSideData default no-op → extraData empty → extraData.length == 0 → plain ClientboundOpenScreenPacket → client MenuType.create(id, inv) (2-arg) → IContainerFactory.create(int, inv) default → create(id, inv, null) → StructureScannerMenu(..., null buf) → extraData.readBlockPos() → NPE.

Wait — does the client's factory get invoked with the 2-arg path for STRUCTURE_SCANNER? ModMenuTypes registers .menu("structure_scanner", StructureScannerMenu::new, ...). Which overload does MenuBuilder.menu pick? There are two: ForgeMenuFactory (4-arg with buf) and MenuFactory (3-arg). StructureScannerMenu::new — there are 3 ctors: (MenuType,int,Inventory,FriendlyByteBuf), (MenuType,int,Inventory,BlockEntity), plus maybe another. A method reference StructureScannerMenu::new with target type ForgeMenuFactory.create(MenuType,int,Inventory,RegistryFriendlyByteBuf) resolves to the 4-arg FriendlyByteBuf ctor (RegistryFriendlyByteBuf is a FriendlyByteBuf) ✓. So it's registered as ForgeMenuFactory → IContainerFactory → client create(id, inv, buf). With plain OpenScreenPacket, NeoForge patches the client handler to... hmm. Does vanilla ClientboundOpenScreenPacket handler call MenuType.create(id, inv) or the 3-arg version with null? In NeoForge 1.21, ClientPacketListener.handleOpenScreen is patched to call menuType.create(windowId, inventory, null)? Let me check. If it calls the 3-arg create(int, Inventory, RegistryFriendlyByteBuf) with null, then IContainerFactory.create(id, inv, null) → menu ctor reads null.readBlockPos() → NPE.

That's exactly the same as IContainerFactory default: create(id, inv) → create(id, inv, null). Either way NPE for menus whose ctor unconditionally reads extraData.

So: any menu registered as ForgeMenuFactory (4-arg) whose provider writes no client data ⇒ NPE on open. That affects:

• StructureScannerMenu (BE has no writeClientSideData) — but does the normal (non-spectator) path work today? Yes, because StructureScannerBlock.useWithoutItem calls ModMenuTypes.open(serverPlayer, menuProvider, pos) → player.openMenu(provider, pos) → the BlockPos overload = openMenu(provider, buf -> buf.writeBlockPos(pos))... wait, IPlayerExtension.openMenu(MenuProvider, BlockPos) default → lambda$openMenu$0(BlockPos, RegistryFriendlyByteBuf) writes blockPos. So today non-spectator gets pos written ✓. Spectator path uses openMenu(provider) → no consumer → and now BE writes nothing → empty → NPE on client.

So (B) is a real 🔴: spectator opening the structure scanner writes a menu with no extra data, and the client menu ctor unconditionally readBlockPos() on a null/empty buffer.

Hmm, but careful: is it null or empty? MenuType.create(windowId, playerInv, extraData) — NeoForge's patch: if constructor instanceof IContainerFactory → create(windowId, playerInv, extraData). The extraData would be null if the caller passes null. Need to check the client packet handler. Let me check whether NeoForge's patched handleOpenScreen creates with null buf or calls the 2-arg.

If the 2-arg MenuType.create(id, inv) is called (vanilla MenuSupplier.create), then IContainerFactory default → create(id, inv, null) → NPE. Either way NPE.

But wait: ControlValveBlockEntity also lacks writeClientSideData and ControlValveMenu extraData ctor reads readBlockPos(). ControlValveBlock has no getMenuProvider override and extends BetterBaseEntityBlock extends BaseEntityBlock → default provider returns the BE. So spectator right-click on control valve ⇒ NPE. That's a regression introduced by this PR for valve? Before the PR, createMenu returned null for spectator... let me check base ControlValveBlockEntity — grep showed no isSpectator in base, meaning ControlValveBlockEntity.createMenu never had the spectator guard. So in base, spectator openMenu → createMenu returns a menu → but wait, openMenu for spectator: createMenu returns non-null → sends packet. Base: no writeClientSideData, useWithoutItem/useItemOn? ControlValveBlock:196 serverPlayer.openMenu(be, pos) → pos written → client OK. And for spectator in base: openMenu(provider) via vanilla useItemOn → no pos → NPE. So this NPE for control valve already exists in base (pre-existing). Hmm, unless the block's useWithoutItem blocks spectators. Let me check ControlValveBlock:196 context.

Actually this is important: for pre-existing NPE, it's not this PR's fault, but the PR's stated goal ("补齐机器菜单的位置、配置数据和多方块入口" + "建议排查所有带UI的方块") means it should fix them. The PR added writeClientSideData to 11 BEs but missed 2 (ControlValve, StructureScanner) — both of which have menus reading extraData. That's an incomplete coverage = the issue's core request not fully met.

Let me verify which menus' providers are reached by spectator. For any block whose BE implements MenuProvider and block extends BaseEntityBlock: vanilla spectator branch will call getMenuProvider → BE (non-null) → openMenu. So all these machines become spectator-openable automatically, regardless of the block's useWithoutItem. Blocks extending HorizontalDirectionalBlock (AdvancedComparator, PulseGenerator) return null by default → that's why they needed explicit getMenuProvider. And TeslaTower (SimpleMultiPartBlock) needed it because it must resolve main part pos.

So: coverage matrix:

• BEs with MenuProvider + BaseEntityBlock-derived block: Chute, MagneticChute, Batch*, ItemCollector, ExpCollector, ActiveSilencer, ItemDetector, StructureScanner, ControlValve, CreativeGenerator, AutoEnchantingTable, SmartBlockPlacer, CreativeLaser, TradingStation(WaterloggedFlexibleMultiPartBlock — is that BaseEntityBlock? likely extends FlexibleMultiPartBlock extends AbstractMultiPartBlock extends Block → getMenuProvider default null! Hmm), CFA (SimpleMultiPartBlock → Block → null default).

Wait, TeslaTower is SimpleMultiPartBlock → extends AbstractMultiPartBlock → extends Block. BlockBehaviour.getMenuProvider default returns null. So multipart blocks need explicit override. TeslaTower got one. But CFA (CelestialForgingAnvilBlock) and TradingStationBlock and CelestialForgingAnvilAmplifierBlock are multipart too — they still open via ModMenuTypes.open(sp, cfaBe, mainPos) in useWithoutItem, which spectators... does CFA's useWithoutItem have a spectator check? Let me check. Earlier grep for isSpectator in block/ didn't show CFA or TradingStation (TradingStation:135 has sp.isSpectator() → returns TRY_WITH_EMPTY_HAND). CFA has no isSpectator → so a spectator right-click: useItemOn? No — vanilla spectator branch intercepts before block useItemOn, and getMenuProvider returns null for CFA (Block default) → PASS. So CFA spectator still can't open. The PR claims "补齐...多方块入口" — only TeslaTower got it. CFA (3×2×3×2? Cube323PartHalf) and TradingStation not covered.

Hmm, but wait: is that within scope? Issue says 溜槽, 批量加工 (batch crafter/cutter), and "建议排查所有带UI的方块". The PR bullet says "补齐机器菜单的位置、配置数据和多方块入口". TeslaTower is the only multipart one covered.

Now — the biggest question: does getMenuProvider being null for CFA matter for the issue? Probably secondary. Let me focus on the strongest, verifiable defects:

🔴 1: Spectator opening a menu whose BE writes no extraData but menu reads it ⇒ client-side NPE.
Affected: StructureScannerMenu (readBlockPos), ControlValveMenu (readBlockPos). Both BEs implement MenuProvider and are reachable via BaseEntityBlock.getMenuProvider in the new spectator branch. Before this PR spectators couldn't reach them at all (via spectator branch → getMenuProvider → ...hmm, wait. Before the PR, was the spectator branch reachable? YES — the vanilla branch is unchanged. Before the PR: spectator → getMenuProvider → BE → openMenu → createMenu → if (player.isSpectator()) return null → openMenu: if (menu == null) { if spectator → sendSystemMessage(spectatorCantOpen) }... so spectator got the "无法打开：战利品未生成"? No — spectatorCantOpen = "Unable to open. Spectator mode." Hmm the issue says 「无法打开：战利品未生成」 which is container.spectatorCantOpen? Let me check the lang: container.spectatorCantOpen in zh_cn is "无法打开：旁观模式"。Hmm the issue says 战利品未生成 — that's a different key. Let me grep. Actually vanilla lang: "container.spectatorCantOpen" = "Unable to open. Spectator mode." and "container.shulkerBoxOpened"? The Chinese "无法打开：战利品未生成" might be a mod-specific message. Hmm, whatever.

Key: for StructureScanner/ControlValve (no spectator guard in base createMenu), before the PR the spectator branch already called openMenu → createMenu non-null → sends packet. With useWithoutItem's ModMenuTypes.open(sp, provider, pos)? No — spectator branch calls openMenu(provider) (1-arg) directly from the game mode, no pos. So before the PR, StructureScanner and ControlValve already NPE'd for spectators. It's pre-existing. Hmm! That weakens it to "pre-existing, but the PR is supposed to fix exactly this class of problem and misses these two".

Let me verify: does ControlValveBlockEntity's createMenu in base have a spectator guard? grep showed line 278 only for createMenu and no isSpectator in /tmp/cvbe_base.java. But is /tmp/cvbe_base.java the right file? I created it earlier presumably. Let me double check it exists and is the base version.

OK. I need to be careful. Given the constraint of time, let me consolidate which claims I can defend:

1. ✅ PR direction correct: vanilla spectator branch is real (bytecode 26.1 confirmed).
2. 🔴/⚠️ Coverage: 2 of the BEs that implement MenuProvider still lack writeClientSideData while their menus unconditionally read extraData → empty buffer → readBlockPos() on null/empty ⇒ client NPE. Reachable by spectator via the new path (and the PR explicitly claims "补齐…配置数据" + issue asks to check all UI blocks). Need to verify whether it's null or empty buffer and whether the client paths differ. If empty (0 bytes), readBlockPos() on an empty buf throws IndexOutOfBoundsException, not NPE — but FriendlyByteBuf.readBlockPos on an empty buffer throws. And if the buffer is null: NPE. Either way crash.

Actually wait — important: if extraData.length == 0, NeoForge sends plain ClientboundOpenScreenPacket. Client handler: vanilla ClientPacketListener.handleOpenScreen → this.minecraft.player.containerMenu = menuType.create(packet.getContainerId(), this.minecraft.player.getInventory())? In NeoForge patched version, it must pass extra data when available. For plain packet → no extra data → create 2-arg → IContainerFactory default → create(id, inv, null) → NPE inside menu ctor.

Let me verify the client-side: 26.1 ClientPacketListener.handleOpenScreen. Let me javap that from mc2612_client.jar. Hmm but 26.1 vanilla doesn't have extra data — this is a NeoForge patch. In 26.1 line the project uses NeoForge for 26.1. I don't have that jar locally probably. Let me just check 1.21.1 neoforge's patched handleOpenScreen to see the pattern — the mechanism is NeoForge-wide.

Let me do that quickly: find ClientPacketListener patch.

Also (C): SliderMenu 4-arg ctor with callback = null — used by CreativeGenerator. SliderUpdatePacket.handleOnServer returns early if callback==null. Fine. But CreativeGeneratorBlock.use sends SliderInitPacket — and now writeClientSideData writes VarInt power. Consistent. And SliderScreen.init seeds from menu.getInitialValue(). OK.

But: SliderScreen.init() is also called on resize → re-seeds from menu.getInitialValue(), discarding unsaved edits. Minor.

(D) Spectator can now write: SliderUpdatePacket gated only on containerMenu instanceof SliderMenu and callback non-null — but server-side menu created via createMenu has callback = this::setPower, so a spectator with the creative generator menu open can send SliderUpdatePacket → setPower. Vanilla blocks slot clicks & button clicks, but not custom payloads. So spectators can mutate creative generator power. Similarly SmartBlockPlacerActionPacket (setPickupMode/selectedLayer), ItemDetectorChangeRangePacket, TeslaAddFilterPacket, AdvancedComparatorUpdatePacket, PulseGeneratorUpdatePacket, MachineOutputDirectionPacket, SlotFilterChangePacket. Hmm — but wait, is containerMenu on the server for a spectator the newly opened menu? Yes.

However — is this PR's fault? Yes: previously spectators couldn't open these menus (createMenu returned null), so none of these C2S paths were reachable for spectators. Now they are. That's a genuine new attack/consistency gap: ⚠️ (or 🔴 if it allows griefing machines). Recommend adding if (player.isSpectator()) return; in these handlers or a shared guard.

(E) TeslaTower double-send: useWithoutItem still sends TeslaFilterSyncPacket AND writeClientSideData writes it → double. Plus TeslaTowerScreen.init() reads client BE's allowList which is populated by menu ctor's handleSync. Redundant 💡. But also TeslaTowerMenu.handleSync on the client calls blockEntity.handleSync(...) which requires level != null and the main part BE — on a client that's fine, but it also mutates allowList/setChanged() on client BE. Fine-ish.

Wait, more serious: TeslaTowerMenu extraData ctor: this(menuType, containerId, inventory, Objects.requireNonNull(inventory.player.level().getBlockEntity(extraData.readBlockPos()))) then this.handleSync(decode(extraData)...). But this(...) is executed first, then handleSync — fine. However the 3-arg ctor probably calls handleSync too or the BE lookup for main part. OK.

But here's a thing: for spectators, handleSync finds the main-part BE and mutates it. On the client that's a client BE. OK.

(F) AdvancedComparatorBlock/PulseGeneratorBlock double-write → redundant bytes, parse-compatible. 💡 (I verified order and parse). Actually wait, one more: AdvancedComparatorMenu.readComparator calls readDataNbt(...) which applies the NBT to the client BE; this is a client-side mutation of a BE's config. Hmm, that's the pre-existing design.

But there's a subtle problem: for the comparator, non-spectator path now writes pos+nbt twice: the nbt applied is from the first (same data). Fine.

(G) writeClientSideData on BaseMachineBlockEntity — writes BlockPos. Its subclasses' menus read BlockPos. OK. But BaseMachineBlockEntity's subclasses include StructureScannerBlockEntity? No, scanner extends BlockEntity directly.

(H) CreativeGeneratorBlockEntity.writeClientSideData writes only VarInt (no BlockPos) — SliderMenu reads only VarInt ✓. Consistent.

(I) ActiveSilencer/ItemCollector/ItemDetector/CFA: their blocks still call ModMenuTypes.open(sp, be, pos) (with pos), and now BE also writes BlockPos ⇒ duplicate BlockPos at the head: stream = [pos_wcsd][pos_consumer]. Menu reads only ONE BlockPos (e.g. ActiveSilencerMenu reads extraData.readBlockPos()), so it reads the first — correct. Leftover ignored. Still 💡.

Hmm, so double-writing is pervasive but benign as long as reads are prefix-only. Except — wait! For AdvancedComparatorMenu the read is pos then nbt. wcsd writes pos+nbt, consumer writes pos+nbt. Prefix read = pos1, nbt1 ✓.

So no corruption. Good thing I checked instead of reporting 🔴.

Now, is there any case where wcsd writes MORE than the menu reads, and the consumer writes something the menu needs first? No; both write pos first.

(J) The real 🔴 candidate: missing writeClientSideData for StructureScanner + ControlValve ⇒ empty extra data ⇒ client menu ctor reads a null/empty buffer ⇒ crash. Let me confirm whether empty (0 bytes) leads to plain packet and null buf. Need the client handler behavior. If buffer is null: StructureScannerMenu(..., (FriendlyByteBuf) null) → Objects.requireNonNull(inventory.player.level().getBlockEntity(extraData.readBlockPos())) → NPE at readBlockPos. If it's an empty buffer... would extraData be non-empty? No: empty → plain packet → 2-arg create → IContainerFactory default → create(id, inv, null). So null. NPE.

Unless... Block.byBlockEntity etc. But also: does the client even have the BE? inventory.player.level().getBlockEntity(pos) — the pos read fails first.

So: spectator right-click on structure scanner / control valve → client crash (NPE). Wait, but a menu ctor throwing on the client → exception in packet handler → crash or disconnect. That's severe.

But careful: is it pre-existing? For StructureScanner in base: createMenu had if (player.isSpectator()) return null;? The diff says StructureScannerBlockEntity had - if (player.isSpectator()) return null; removed. So in base, spectator → createMenu null → openMenu prints spectatorCantOpen (only for spectator) → no packet. No crash. After this PR: createMenu non-null → packet sent with empty extra data → client NPE. So this IS a new regression introduced by this PR.

Same for ControlValveBlockEntity: base has no guard (grep found none). So spectator → base already sends packet with empty extraData? Wait, does it? Base ControlValveBlock.useWithoutItem: serverPlayer.openMenu(be, pos) writes pos ✓. But the vanilla spectator branch bypasses useWithoutItem → openMenu(provider) → no pos, no wcsd → empty → NPE. So control valve spectator NPE is pre-existing, not introduced. Still worth reporting as "same class, should be fixed in the same pass" — and the PR's own goal is to make spectator previews work.

Hmm, but hold on. Is ControlValveBlock's useWithoutItem guarded by spectator? Let me check line ~190. If it has if (spectator) return PASS, then... the vanilla spectator branch runs earlier anyway. So still NPE pre-existing.

Let me verify the exact set:

• Menus reading extraData: ActiveSilencer, AdvancedComparator(+nbt), AutoEnchantingTable, BaseChute, BatchCrafter, BatchCutter, CelestialForgingAnvil, ControlValve, ExpCollector, ItemCollector, ItemDetector, PulseGenerator(+nbt), Slider(VarInt), SmartBlockPlacer, StructureScanner, TeslaTower(pos+payload), TradingStation, CreativeLaser? (not in list — CreativeLaserMenu doesn't read extraData? It wasn't in the list). Right, the list had 17 entries.

Providers:

• ActiveSilencer BE: wcsd ✓
• AdvancedComparator BE: wcsd ✓
• AutoEnchantingTable BE: wcsd ✓ (pre-existing? it had 1 — was it in the diff? No! AutoEnchantingTable isn't in the 24-file list. So it already had wcsd in base. Interesting — meaning base already had the pattern for it.)
• BaseChute (Chute/MagneticChute): via BaseMachineBlockEntity wcsd ✓
• Batch*: via BaseMachineBlockEntity ✓ (do they extend it? BatchCraftingBlockEntity probably extends BaseMachineBlockEntity — need check)
• CFA: wcsd ✓ (added)
• ControlValve: ✗ missing
• ExpCollector: wcsd ✓
• ItemCollector: ✓
• ItemDetector: ✓
• PulseGenerator: ✓
• Slider (CreativeGenerator): ✓
• SmartBlockPlacer: ✓ (1 — in base? not in diff... SmartBlockPlacerBlockEntity not in the 24 files → pre-existing ✓)
• StructureScanner: ✗ missing
• TeslaTower: ✓
• TradingStation: ✓ (pre-existing, not in diff)

So exactly two BEs are missing: ControlValve and StructureScanner. Both are newly-reachable-or-already-reachable via spectator branch, and the PR removed the spectator guard from StructureScannerBlockEntity making it crash.

Wait — actually let me re-read: did the PR remove the guard from StructureScannerBlockEntity? Yes: - if (player.isSpectator()) return null;. Confirmed in diff hunk 267.

So StructureScanner: 🔴 regression — spectator right-click on structure scanner now sends a menu packet with empty extra data, client-side StructureScannerMenu ctor calls extraData.readBlockPos() on null → NPE/crash. This is precisely the class of bug the PR is trying to fix, and it's introduced by removing the guard without adding writeClientSideData.

Also note: StructureScannerBlock extends BaseEntityBlock (not BetterBaseEntityBlock) but that's fine for getMenuProvider.

Hmm wait, one more check: ModMenuTypes.STRUCTURE_SCANNER = .menu("structure_scanner", StructureScannerMenu::new, ...). Ambiguity: there are two ctors (MenuType, int, Inventory, FriendlyByteBuf) and (MenuType, int, Inventory, BlockEntity). Method reference with target ForgeMenuFactory (4-arg, RegistryFriendlyByteBuf) → picks FriendlyByteBuf ctor. With target MenuFactory (3-arg) → cannot match any 4-arg ctor → so it must be ForgeMenuFactory. ✓ registered as IContainerFactory.

So on the client, MenuType.create(id, inv, null) → IContainerFactory.create(id, inv, null) → new StructureScannerMenu(type, id, inv, (FriendlyByteBuf) null) → null.readBlockPos() → NPE.

Similarly ControlValve.

Now what about non-spectator, pre-existing normal opens for these two? They pass pos via the consumer, so extraData non-empty, and wcsd writes nothing → stream = [pos] ✓. Fine.

So the fix is trivial: add writeClientSideData writing buffer.writeBlockPos(this.getBlockPos()) to both, and drop the redundant pos-consumer at call sites.

Another candidate 🔴: StructureScannerBlock.useWithoutItem calls ModMenuTypes.open(serverPlayer, menuProvider, pos) — writes pos. For a spectator, that method isn't reached. OK.

Now let me also check: does the PR's removal of if (player.isSpectator()) return null in createMenu affect anything else? createMenu is also called by openMenu, and it's now enabled for spectators, necessary for preview.

(K) AdvancedComparatorBlock/PulseGeneratorBlock still return PASS for spectators in useWithoutItem — irrelevant because vanilla branch intercepts first (bytecode proves). So the new getMenuProvider IS the spectator path, and it works. ✓ But note these blocks also don't have BetterBaseEntityBlock, so getMenuProvider override was required — correctly added. But TeslaTower's getMenuProvider uses getMainPartPos(pos, state) ✓ good.

(L) TeslaTowerBlock.useWithoutItem changed ModMenuTypes.open(sp, teslaTowerBlockEntity, mainPartPos) → ModMenuTypes.open(sp, teslaTowerBlockEntity): now relies on wcsd writing pos. TeslaTower's wcsd writes this.getBlockPos() = main part pos ✓ (BE is main part). Fine. But it also duplicates the filter payload. Redundant.

(M) CreativeGeneratorBlock: ModMenuTypes.open(serverPlayer, entity) + still PacketDistributor.sendToPlayer(SliderInitPacket). Now wcsd writes power too, and SliderMenu reads it. SliderScreen.init reads menu.getInitialValue(). SliderInitPacket presumably sets the textbox later. Duplicate-ish but consistent. Also SliderInitPacket might be redundant now — 💡.

(N) Spectator write paths — the strongest ⚠️. Let me check SliderUpdatePacket on server side: if (!(player.containerMenu instanceof SliderMenu menu)) return; callback — server-side menu has callback set → setPower(value) executes. So a spectator can change creative generator power. Also they can't see the value change (it's a creative-only block). Is that a big deal? It's a machine-state mutation by a non-participating player. Vanilla explicitly blocks spectator interaction for this reason. I'll report as ⚠️ with list.

Also: StructureScannerSavePacket → StructureSaveUtil.saveStructureToDisk writes files to disk, gated on menu.stillValid(player) and containerId match. Spectator can now trigger disk writes → writing structure files as a spectator. And StructureScannerMenu.canUpload data slot uses BlueprintAdministrators.canUpload(player) — so upload permission is checked. But save-to-disk has no such check. Worth ⚠️.

(O) There's also SmartBlockPlacerActionPacket mutating BE. And ItemDetectorChangeRangePacket.

Now, I have enough. Let me quickly verify two things I'm unsure about:

1. Client-side: plain ClientboundOpenScreenPacket → does the client call MenuType.create(id, inv) (2-arg → IContainerFactory default with null)? Need confirmation for 26.1 line. I'll check the 1.21.1 neoforge patch for ClientPacketListener to see how extra data is plumbed; if the pattern is "if packet has extra data use it else null", my conclusion holds.
2. Whether StructureScannerBlockEntity.createMenu removal actually leaves the spectator branch reachable: getMenuProvider default from BaseEntityBlock returns BE ✓ (26.1 bytecode confirmed).

Let me do #1 quickly.

Then write the review. Time budget is a concern; I've used many calls. Let me be efficient: one call for ClientPacketListener patch, maybe one for BatchCraftingBlockEntity extends BaseMachineBlockEntity, then write.

Actually for #1, I could also reason: NeoForge's IContainerFactory.create(int, Inventory) default passes null. So even if the client calls the 2-arg create, null flows in. The only question is whether NeoForge's client handler crashes differently. Either way the menu ctor NPEs. Good enough — but let me confirm the 2-arg path is what's used (i.e., that null reaches the ctor). Confirmed by javap of IContainerFactory default. And MenuScreens.ScreenConstructor.fromPacket calls menuType.create(id, inv) 2-arg (javap above showed MenuType.create:(ILnet/minecraft/world/entity/player/Inventory;)). So yes: plain OpenScreenPacket → ScreenConstructor.fromPacket → 2-arg create → IContainerFactory default → create(id, inv, null) → NPE.

Excellent — that's proven by javap already. And for AdvancedOpenScreenPayload, ClientPacketListener presumably calls the 3-arg create with the real buf.

Let me double check BatchCraftingBlockEntity extends BaseMachineBlockEntity, and check StructureScannerMenu has no null guard (confirmed: Objects.requireNonNull(... extraData.readBlockPos()) — readBlockPos first).

One more: for ControlValve, is it reachable by spectator before the PR? Base createMenu has no spectator guard → so pre-existing NPE. I'll note it as "同类缺口，非本 PR 引入但应一并补齐".

Actually hold on — I should double check that base ControlValveBlockEntity really lacks the guard, since I'm relying on a cached file. Let me re-derive from the diff: the diff doesn't touch ControlValveBlockEntity at all (not in the 24 files). And head has no isSpectator in createMenu. So base = head for that file = no guard. ✓ Pre-existing.

Similarly StructureScanner: the diff removes the guard → new regression.

Now write the review. Structure:

结论: REQUEST_CHANGES (one 🔴).

概览: PR #6080, head/base sha, 24 files +98/-20, CI build+checkstyle success, 26.1 线, issue #6055 版本匹配。声明 diff 覆盖不全（webhook 只给了 patch diff，未读全仓库）。

🔴 1. 旁观者打开结构扫描仪必崩（新增回归）：remove guard + 无 writeClientSideData ⇒ 空 extra data ⇒ 普通 OpenScreenPacket ⇒ 客户端 2 参 create ⇒ IContainerFactory 默认传 null ⇒ StructureScannerMenu 构造器 extraData.readBlockPos() NPE。给出证据链（javap 三段）+ 修法。

⚠️ 1. 控制阀同类缺口（pre-existing，但既然立论是"排查所有带 UI 的方块"，应一并补）。

⚠️ 2. 旁观者现在能发 C2S 写包（列表 + 服务端 handler 无 isSpectator 门）：SliderUpdatePacket→setPower、SmartBlockPlacerActionPacket、ItemDetectorChangeRangePacket、Tesla*Filter、AdvancedComparator/PulseGenerator update、MachineOutputDirection/SlotFilter、以及 StructureScannerSavePacket 落盘。

⚠️ 3. 多方块入口只补了特斯拉塔：CFA / TradingStation / CFA 增幅器仍是 Block 默认 getMenuProvider=null ⇒ 旁观者打不开；PR bullet "补齐…多方块入口" 未兑现（且 TradingStation 在 useWithoutItem 里显式 return TRY_WITH_EMPTY_HAND）。

💡 1. 重复写 extra data：AdvancedComparator/PulseGenerator/ActiveSilencer/ItemCollector/ItemDetector/CFA/StructureScanner(block 侧 pos consumer) 都还在调用点写一遍 pos，BE 又写一遍 ⇒ 冗余字节；建议统一走 writeClientSideData 并删掉调用点 consumer（也顺带修 🔴）。
💡 2. TeslaTower 过滤表三次下放（wcsd + useWithoutItem 的 PacketDistributor + Screen.init 读 client BE）；client 侧 menu.getBlockEntity() 可能为 null（MenuBlockEntityLookup 之外），TeslaTowerScreen.init 直接解引用有 NPE 面。
💡 3. SliderScreen.init 从 menu.getInitialValue() 播种，但 init() 在 resize 时也跑 ⇒ 未提交编辑被丢弃；且 SliderInitPacket 与新通道重复。
💡 4. SliderMenu 4 参构造 callback=null，若将来有人从客户端侧调 handleOnServer 会静默 no-op —— 可接受，建议加注释。

📋 声称验证表。
🧪 人工验证清单。
结论。

Also mention: the two blocks that needed explicit getMenuProvider (comparator/pulse) are correct because they extend HorizontalDirectionalBlock; TeslaTower needs it for main part resolution — all correct ✅.

Let me verify quickly: does AdvancedComparatorBlock.useWithoutItem's spectator PASS matter? No (vanilla intercepts). But it's

[PigeonNian]

LGTM

[PigeonNian]

LGTM