diff --git a/NEXT_STEPS_CLAUDES_REVIEW.md b/NEXT_STEPS_CLAUDES_REVIEW.md deleted file mode 100644 index dd1ec7c..0000000 --- a/NEXT_STEPS_CLAUDES_REVIEW.md +++ /dev/null @@ -1,20 +0,0 @@ -# Issues - -1. (Major) File bytes are JSON-encoded — ~3–4× wire bloat + heavy CPU. StreamInstallFrame::FileChunk { bytes: Bytes } goes over the wire via serde_json (the Message impl). serde_json has no native byte type, so Bytes serializes as a JSON array of integers ([12,255,7,…]) — roughly 3–4 ASCII chars per payload byte, plus serialize/parse cost on both ends. The existing transfer path (peer.rs::stream_file_bytes) sends raw bytes straight on the QUIC stream for exactly this reason. For a feature whose entire purpose is moving file bytes efficiently on constrained peers, routing the payload through JSON undermines the goal. The 3 MiB S39 fixture hides it; a multi-GB game would not. Consider a binary codec (postcard/bincode) for the frame stream, or keep the control frames as-is but stream raw bytes for file content. - -2. (Design) Solid archives → O(n²) extraction. Each file is pulled with a separate unrar p archive (stream_unrar_file). For a solid .eti, unrar must re-decompress every preceding file on each invocation. solid is detected and sent in ArchiveBegin but never acted on. A solid multi-file archive could make streamed install pathologically slow. At minimum document the limitation; ideally a single-pass extraction for the solid case. - -3. (Robustness) Single peer, no fallback. handle_stream_install_game_command picks one peer (peers.sort(); peers.into_iter().next()). If that peer declines (can_serve_game false) or the connection drops, the whole install fails — no retry against the other validated peers, unlike the normal multi-peer download. Fine for a prototype, worth a TODO. - -4. (Minor) unrar p is a wildcard mask. unrar treats the file argument as a pattern; a name containing */?/[ could match the wrong file or multiple files (multiple matches concatenate into stdout → corrupt stream). CRC32 would catch it as a failure rather than silent corruption, but it's a sharp edge. Worth noting / pinning exact-match if the unrar build supports it. - -5. (Minor) CRC32 is optional. If unrar omits CRC32: (older builds, header-encrypted archives), crc32 is None and the receiver verifies size only. - -6. (Minor / cosmetic) - - DownloadGameFileChunkFinished reports relative_path as "{game_id}/local/…" while bytes actually land in staging (.local.installing); and there's no total-size up front, so the UI can't show a percentage. - - parse_unrar_listing depends on unrar's human-readable lt output (Details:/Name:/Type:/Size:/CRC32:). -cfg- guards against user config, but it's still brittle across unrar versions/locales. - -## Nits - -- PeerStartOptions dropped #[derive(Debug)] for a hand-written Debug (Arc isn't Debug) — correct, and Clone/Default are retained. Good. -- commit() renames staging into local/, so the .lanspread_owned marker lands inside local/ — but that's identical to the existing install_inner behavior, so no regression.