Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a modal on-screen text-input component with BACKSPACE and Ctrl+V handling, integrates a SET SEED entry flow into menus and main loop (stores gCustomSeed and clipboard copy), extends Input to hold typed text, and adds a new SEEDLING achievement. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main
participant Input
participant TextInput
participant Renderer
User->>Main: Select "SET SEED"
Main->>TextInput: text_input_init(window, renderer, title, current)
TextInput->>Renderer: Load sprites / prepare modal
loop typing
User->>Input: Key events (text, BACKSPACE, Ctrl+V)
Input->>TextInput: text_input_update(input)
TextInput->>TextInput: modify buffer (append/delete/paste)
Main->>TextInput: text_input_render(camera)
TextInput->>Renderer: Draw modal + cursor
end
User->>Input: Press ENTER (confirm)
Input->>TextInput: confirmation event
TextInput-->>Main: text_input_get_value() / is_confirmed()
Main->>Main: set gCustomSeed / update menus
Main->>TextInput: text_input_close(window)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
0a80ddb to
c308bbe
Compare
b4c00f2 to
6efa803
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.c (1)
601-615:⚠️ Potential issue | 🟠 MajorMain menu count is stale after adding
SET SEED.Line 614 still passes
4, butmenu_itemsnow has 5 entries. One item becomes unreachable.💡 Proposed fix
- menu_create_text_menu(&mainMenu, &menu_items[0], 4, gRenderer); + menu_create_text_menu(&mainMenu, &menu_items[0], (int)(sizeof(menu_items) / sizeof(menu_items[0])), gRenderer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.c` around lines 601 - 615, The menu item count passed to menu_create_text_menu is stale: update the call that currently passes 4 to use the actual number of entries in menu_items (now 5) or compute it dynamically; locate the static TEXT_MENU_ITEM menu_items[] and the call menu_create_text_menu(&mainMenu, &menu_items[0], 4, gRenderer) and change the third argument to the correct count (or replace the literal with a computed size expression) so all menu entries including "SET SEED" are reachable.
🧹 Nitpick comments (1)
src/text_input.c (1)
24-27: Unify buffer sizing with shared input constant.
BUF_SIZEduplicatesTEXT_INPUT_MAX_LEN; keeping both can drift over time. Prefer one shared bound.💡 Proposed refactor
-#define BUF_SIZE 16 +#define BUF_SIZE TEXT_INPUT_MAX_LEN🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/text_input.c` around lines 24 - 27, Replace the local BUF_SIZE constant with the shared TEXT_INPUT_MAX_LEN to avoid duplicated bounds: remove or rename BUF_SIZE and declare inputBuffer sized using TEXT_INPUT_MAX_LEN (affecting the static char inputBuffer[] initialization) so the buffer and any related logic in this module (including uses of inputBuffer and inputSprite) use the single canonical TEXT_INPUT_MAX_LEN definition; ensure any code that relied on BUF_SIZE still compiles by updating references to TEXT_INPUT_MAX_LEN and include the header where TEXT_INPUT_MAX_LEN is defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/input.c`:
- Around line 289-292: The SDL_EVENT_TEXT_INPUT handler currently overwrites
input->textInput on each event; change it to append the incoming chunk
(event->text.text) to the existing input->textInput buffer so multiple text
events per poll are accumulated. Locate the SDL_EVENT_TEXT_INPUT branch and use
the current length of input->textInput (strlen) to compute remaining space, then
copy/append up to TEXT_INPUT_MAX_LEN-1 total bytes and always null-terminate;
reference input->textInput, event->text.text, and TEXT_INPUT_MAX_LEN when making
the change.
In `@src/main.c`:
- Around line 1278-1281: Replace the SDL_atoi usage when parsing
text_input_get_value() into gCustomSeed with strtoul and proper validation: call
strtoul on the C string, check errno for ERANGE, ensure endptr points to the end
of the numeric text (no extra chars), verify the parsed value is <= UINT_MAX and
not a negative representation, and only assign to gCustomSeed on success; on any
error or out-of-range input call gui_event_message(...) to inform the user and
avoid changing gCustomSeed, and keep the debug("Custom seed set: %u",
gCustomSeed) call only after a successful parse.
---
Outside diff comments:
In `@src/main.c`:
- Around line 601-615: The menu item count passed to menu_create_text_menu is
stale: update the call that currently passes 4 to use the actual number of
entries in menu_items (now 5) or compute it dynamically; locate the static
TEXT_MENU_ITEM menu_items[] and the call menu_create_text_menu(&mainMenu,
&menu_items[0], 4, gRenderer) and change the third argument to the correct count
(or replace the literal with a computed size expression) so all menu entries
including "SET SEED" are reachable.
---
Nitpick comments:
In `@src/text_input.c`:
- Around line 24-27: Replace the local BUF_SIZE constant with the shared
TEXT_INPUT_MAX_LEN to avoid duplicated bounds: remove or rename BUF_SIZE and
declare inputBuffer sized using TEXT_INPUT_MAX_LEN (affecting the static char
inputBuffer[] initialization) so the buffer and any related logic in this module
(including uses of inputBuffer and inputSprite) use the single canonical
TEXT_INPUT_MAX_LEN definition; ensure any code that relied on BUF_SIZE still
compiles by updating references to TEXT_INPUT_MAX_LEN and include the header
where TEXT_INPUT_MAX_LEN is defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fce63d20-0a03-4082-9dd1-5a645d7185c9
📒 Files selected for processing (5)
src/input.csrc/input.hsrc/main.csrc/text_input.csrc/text_input.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main.c (1)
1278-1281:⚠️ Potential issue | 🟠 MajorUnresolved: seed parsing still uses unchecked
SDL_atoi.Line 1280 still silently accepts invalid input and cannot reliably detect parse errors/overflow. This was already raised earlier and remains applicable.
Proposed patch
+#include <errno.h> +#include <limits.h> ... if (text_input_is_confirmed()) { const char *val = text_input_get_value(); if (SDL_strlen(val) > 0) { - gCustomSeed = (unsigned int)SDL_atoi(val); - debug("Custom seed set: %u", gCustomSeed); + char *end = NULL; + errno = 0; + unsigned long parsed = strtoul(val, &end, 10); + if (val[0] != '-' && errno == 0 && end != val && *end == '\0' && parsed <= UINT_MAX) { + gCustomSeed = (unsigned int)parsed; + debug("Custom seed set: %u", gCustomSeed); + } else { + gui_event_message("Invalid seed"); + } } else { gCustomSeed = 0; } text_input_close(gWindow); gGameState = MENU; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.c` around lines 1278 - 1281, The seed parsing uses SDL_atoi without error/overflow checks; replace the SDL_atoi(val) usage in the block that reads text_input_get_value() so you robustly parse and validate the seed: use SDL_strtoul (or strtoul) to convert val to an unsigned long, check for empty input, detect conversion errors via errno and endptr, ensure the result fits into unsigned int (gCustomSeed) and handle overflow or invalid characters by rejecting the input (and logging an error via debug) before assigning to gCustomSeed; keep references to text_input_get_value, SDL_atoi (to remove), gCustomSeed, and debug to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.c`:
- Around line 486-489: Fix the formatting in openSeedEntry: gCustomSeed is an
unsigned int, so replace the "%d" specifier with "%u" and stop hardcoding the
buffer length; call SDL_snprintf with sizeof(seed_str) as the size argument
(keep the existing seed_str buffer and gCustomSeed identifiers) so use
SDL_snprintf(seed_str, sizeof(seed_str), "%u", gCustomSeed) to ensure correct
type safety and buffer bounds.
---
Duplicate comments:
In `@src/main.c`:
- Around line 1278-1281: The seed parsing uses SDL_atoi without error/overflow
checks; replace the SDL_atoi(val) usage in the block that reads
text_input_get_value() so you robustly parse and validate the seed: use
SDL_strtoul (or strtoul) to convert val to an unsigned long, check for empty
input, detect conversion errors via errno and endptr, ensure the result fits
into unsigned int (gCustomSeed) and handle overflow or invalid characters by
rejecting the input (and logging an error via debug) before assigning to
gCustomSeed; keep references to text_input_get_value, SDL_atoi (to remove),
gCustomSeed, and debug to locate and update the code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main.c (2)
488-488:⚠️ Potential issue | 🟡 MinorUse unsigned formatting and real buffer size for seed prefill.
Line 488 uses
%dfor anunsigned intand hardcodes15instead ofsizeof(seed_str).💡 Proposed fix
- SDL_snprintf(seed_str, 15, "%d", gCustomSeed); + SDL_snprintf(seed_str, sizeof(seed_str), "%u", gCustomSeed);#!/bin/bash # Verify type and formatting call at seed prefill site rg -nP 'static unsigned int gCustomSeed|SDL_snprintf\(seed_str,\s*15,\s*"%d"' src/main.c -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.c` at line 488, The SDL_snprintf call at the seed prefill uses signed "%d" and a hardcoded length 15; update the call that writes into seed_str to use unsigned formatting "%u" and pass the actual buffer size (sizeof(seed_str)) instead of 15, ensuring the value gCustomSeed (declared as static unsigned int gCustomSeed) is formatted correctly; locate the SDL_snprintf(seed_str, 15, "%d", gCustomSeed) invocation and replace the format/length accordingly.
1281-1285:⚠️ Potential issue | 🟠 MajorSeed parsing still relies on
SDL_atoi(ambiguous/unchecked conversion).Line 1284 accepts malformed/overflow input without reliable validation, which can silently coerce to unintended values.
💡 Proposed fix
+#include <errno.h> +#include <limits.h> ... if (text_input_is_confirmed()) { const char *val = text_input_get_value(); if (SDL_strlen(val) > 0) { - gCustomSeed = (unsigned int)SDL_atoi(val); - debug("Custom seed set: %u", gCustomSeed); + errno = 0; + char *end = NULL; + unsigned long parsed = strtoul(val, &end, 10); + if (errno == 0 && end != val && *end == '\0' && parsed <= UINT_MAX) { + gCustomSeed = (unsigned int)parsed; + debug("Custom seed set: %u", gCustomSeed); + } else { + gui_event_message("Invalid seed"); + } } else { gCustomSeed = 0; }#!/bin/bash # Verify current parsing path and nearby seed handling rg -nP 'run_seed_entry|text_input_get_value|SDL_atoi|strtoul|gCustomSeed' src/main.c -C3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.c`:
- Around line 265-274: register_scores is using the menu/global flag gCustomSeed
(and similar globals) which can be stale; instead determine seed type from the
active run state used when starting the run. Update register_scores to consult
the run-specific seed indicator (for example a field on the current run struct
such as run->seed_type or run->is_custom_seed) or accept the run/seed-type as a
parameter, and replace checks of gCustomSeed (and any use of weeklyGame) with
that run-level value so SEEDLING is gated by the actual run seed; mirror the
same logic used in set_random_seed (weeklyGame, run-level custom seed, else) and
apply the same change for the other occurrence referenced (lines around the
second location).
In `@src/steam/steamworks_api_wrapper.c`:
- Around line 26-28: The numAchievements variable is stale (7) while
g_Achievements now contains 8 entries, causing steam_set_achievement(SEEDLING)
never to match; update the code to derive numAchievements from the array length
(e.g., compute sizeof(g_Achievements)/sizeof(g_Achievements[0])) or set
numAchievements to 8 so it accurately reflects g_Achievements, ensuring
functions like steam_set_achievement and any loops over g_Achievements use the
correct count.
---
Duplicate comments:
In `@src/main.c`:
- Line 488: The SDL_snprintf call at the seed prefill uses signed "%d" and a
hardcoded length 15; update the call that writes into seed_str to use unsigned
formatting "%u" and pass the actual buffer size (sizeof(seed_str)) instead of
15, ensuring the value gCustomSeed (declared as static unsigned int gCustomSeed)
is formatted correctly; locate the SDL_snprintf(seed_str, 15, "%d", gCustomSeed)
invocation and replace the format/length accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4278b2ab-a26c-4ec0-b2c7-e347dd5590a5
📒 Files selected for processing (3)
src/main.csrc/steam/steamworks_api_wrapper.csrc/steam/steamworks_api_wrapper.h
b43f2a3 to
973b513
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/steam/steamworks_api_wrapper.c (1)
26-28:⚠️ Potential issue | 🔴 CriticalUpdate
numAchievements;SEEDLINGis currently unreachable.At Line 28,
numAchievementsis still7whileg_Achievementsnow has 8 entries, so the loop at Line 140 never evaluates the last item (SEEDLING).Proposed fix
-static Uint8 numAchievements = 7; +static const Uint8 numAchievements = + (Uint8)(sizeof(g_Achievements) / sizeof(g_Achievements[0]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/steam/steamworks_api_wrapper.c` around lines 26 - 28, The numAchievements constant is stale (still 7) while g_Achievements now contains 8 entries, leaving SEEDLING unreachable; fix by making numAchievements reflect the actual array length (either change the literal to 8 or, better, compute it from the array like sizeof(g_Achievements)/sizeof(g_Achievements[0])) so the loop that iterates over g_Achievements (referenced where numAchievements is used) will include the SEEDLING entry.
🧹 Nitpick comments (1)
src/main.c (1)
1281-1281: Fix typo in comment.Minor typo: "Di this last" should be "Do this last".
✏️ Proposed fix
- // Di this last since it resets the text_input + // Do this last since it resets the text_input🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.c` at line 1281, Fix the typo in the comment that reads "Di this last since it resets the text_input" by changing it to "Do this last since it resets the text_input"; update the comment near the code that resets text_input (the comment on the line containing the text_input reset) to use the correct word "Do" so the inline documentation is accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/steam/steamworks_api_wrapper.c`:
- Around line 26-28: The numAchievements constant is stale (still 7) while
g_Achievements now contains 8 entries, leaving SEEDLING unreachable; fix by
making numAchievements reflect the actual array length (either change the
literal to 8 or, better, compute it from the array like
sizeof(g_Achievements)/sizeof(g_Achievements[0])) so the loop that iterates over
g_Achievements (referenced where numAchievements is used) will include the
SEEDLING entry.
---
Nitpick comments:
In `@src/main.c`:
- Line 1281: Fix the typo in the comment that reads "Di this last since it
resets the text_input" by changing it to "Do this last since it resets the
text_input"; update the comment near the code that resets text_input (the
comment on the line containing the text_input reset) to use the correct word
"Do" so the inline documentation is accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 427ef045-bb53-4e75-a6ef-3fb1187e9e78
📒 Files selected for processing (3)
src/main.csrc/steam/steamworks_api_wrapper.csrc/steam/steamworks_api_wrapper.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/steam/steamworks_api_wrapper.h
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/main.c (3)
486-489:⚠️ Potential issue | 🟡 MinorFix format specifier and buffer size in
openSeedEntry.Line 488 passes
unsigned int gCustomSeedto%dand hardcodes15; use%uandsizeof(seed_str).Suggested patch
- if (gCustomSeed > 0) { - SDL_snprintf(seed_str, 15, "%d", gCustomSeed); - } + if (gCustomSeed > 0) { + SDL_snprintf(seed_str, sizeof(seed_str), "%u", gCustomSeed); + }#!/bin/bash # Verify type/format mismatch in openSeedEntry rg -n "static unsigned int gCustomSeed|openSeedEntry|SDL_snprintf\\(seed_str" src/main.c -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.c` around lines 486 - 489, openSeedEntry currently formats unsigned int gCustomSeed with the signed "%d" and hardcodes 15 for the buffer length; update the SDL_snprintf call that writes into seed_str to use the unsigned format specifier "%u" and pass sizeof(seed_str) (or sizeof seed_str) as the buffer size (referencing seed_str, gCustomSeed, and SDL_snprintf in openSeedEntry) to avoid format/type mismatch and potential buffer issues.
1283-1289:⚠️ Potential issue | 🟠 MajorReplace unchecked
SDL_atoiparsing for custom seed input.Line 1285 still accepts invalid/overflowing input without reliable validation; parse with
strtoul+endptr+errno+UINT_MAXchecks and report invalid input.Suggested patch
- if (SDL_strlen(val) > 0) { - gCustomSeed = (unsigned int)SDL_atoi(val); - debug("Custom seed set: %u", gCustomSeed); - } else { + if (SDL_strlen(val) > 0) { + char *end = NULL; + errno = 0; + unsigned long parsed = strtoul(val, &end, 10); + if (errno == 0 && end != val && *end == '\0' && parsed <= UINT_MAX) { + gCustomSeed = (unsigned int)parsed; + debug("Custom seed set: %u", gCustomSeed); + } else { + gui_event_message("Invalid seed"); + } + } else { gCustomSeed = 0; }Additional includes needed near the top of
src/main.c:`#include` <errno.h> `#include` <limits.h>#!/bin/bash # Verify current seed parsing path is still unchecked rg -n "run_seed_entry|text_input_get_value|SDL_atoi|strtoul|gui_event_message" src/main.c -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.c` around lines 1283 - 1289, Replace the unchecked SDL_atoi call in the seed parsing block (where text_input_get_value() is read and gCustomSeed is set) with robust strtoul parsing: include errno.h and limits.h, call strtoul on the returned string using an endptr, clear and check errno for ERANGE, verify endptr points to the string end (or only whitespace), ensure the parsed value <= UINT_MAX, and only then assign gCustomSeed = (unsigned int)parsed; otherwise set gCustomSeed = 0 and log/report the input as invalid; update the debug/error message to reflect parsing failures.
1143-1145:⚠️ Potential issue | 🟠 MajorSEEDLING should be gated by run seed provenance, not menu/global seed value.
Line 1143 still uses
gCustomSeed+weeklyGame, which can be stale and awardSEEDLINGfor non-custom runs (e.g., continued saves).Suggested patch
static Input input; static unsigned int gCustomSeed = 0; +static bool gUsedCustomSeedThisRun = false; ... if (weeklyGame) { debug("Using weekly game seed"); set_random_seed((unsigned int)time_get_weekly_seed()); + gUsedCustomSeedThisRun = false; } else if (gCustomSeed) { debug("Using custom game seed"); set_random_seed(gCustomSeed); + gUsedCustomSeedThisRun = true; } else { debug("Setting random seed"); set_random_seed(0); // 0 will trigger a random seed later on. + gUsedCustomSeedThisRun = false; } ... set_random_seed(save->seed); + gUsedCustomSeedThisRun = false; ... - if (gCustomSeed != 0 && !weeklyGame) { + if (gUsedCustomSeedThisRun) { steam_set_achievement(SEEDLING); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.c` around lines 1143 - 1145, The SEEDLING award check currently uses the global/menu flag gCustomSeed and weeklyGame, which can be stale; change the condition to consult the active run's seed provenance instead (e.g., use the run context or API such as current_run->seed_provenance or a helper like seed_provenance_is_custom(current_run)) while still verifying !weeklyGame, and use that run-level boolean in place of gCustomSeed so continued saves don't wrongly trigger steam_set_achievement(SEEDLING).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main.c`:
- Around line 486-489: openSeedEntry currently formats unsigned int gCustomSeed
with the signed "%d" and hardcodes 15 for the buffer length; update the
SDL_snprintf call that writes into seed_str to use the unsigned format specifier
"%u" and pass sizeof(seed_str) (or sizeof seed_str) as the buffer size
(referencing seed_str, gCustomSeed, and SDL_snprintf in openSeedEntry) to avoid
format/type mismatch and potential buffer issues.
- Around line 1283-1289: Replace the unchecked SDL_atoi call in the seed parsing
block (where text_input_get_value() is read and gCustomSeed is set) with robust
strtoul parsing: include errno.h and limits.h, call strtoul on the returned
string using an endptr, clear and check errno for ERANGE, verify endptr points
to the string end (or only whitespace), ensure the parsed value <= UINT_MAX, and
only then assign gCustomSeed = (unsigned int)parsed; otherwise set gCustomSeed =
0 and log/report the input as invalid; update the debug/error message to reflect
parsing failures.
- Around line 1143-1145: The SEEDLING award check currently uses the global/menu
flag gCustomSeed and weeklyGame, which can be stale; change the condition to
consult the active run's seed provenance instead (e.g., use the run context or
API such as current_run->seed_provenance or a helper like
seed_provenance_is_custom(current_run)) while still verifying !weeklyGame, and
use that run-level boolean in place of gCustomSeed so continued saves don't
wrongly trigger steam_set_achievement(SEEDLING).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d71b3e33-09ce-4f28-b670-dccc1ab19866
📒 Files selected for processing (3)
src/main.csrc/steam/steamworks_api_wrapper.csrc/steam/steamworks_api_wrapper.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/steam/steamworks_api_wrapper.c
The intention with this PR once it's done is to allow for random seed retrieval during play and starting games with custom seeds. This will make level sharing possible. For whatever reason players see fit. Eg. Speed running or similar.
Summary by CodeRabbit
New Features
Improvements
Other