Open
Conversation
- replaced overflow_container and secondary_overflow with single overflow_containers setting (comma-separated)
- added configurable "Trash to Dump" section, instead of hardcoded herb/food/junk
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
Probably want to remove food, herb, and junk toggles in the sell type section since they are covered by the trash to dump section. |
Contributor
Author
I kept old functionality that it still checks the "sell" types in order to then trash/junk stuff. So you have to have both checked in order to actually dump. Couldn't think of a better way to safeguard. That way it tries to sell first any category and then dumps (just for food/herbs/junk it can't really sell, but still needed to be checked) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
force saving of migrated settings for overflow upon migration
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Updated the total tips calculation to include a custom withdraw amount setting for the locksmith.
Closed
Refactor in_region method to improve boundary check logic and add a bugfix for destination room check.
Add comment to clarify bounty town region check
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
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:
📝 WalkthroughWalkthroughVersion bumped to 2.8.0. Legacy Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "UI / Settings"
participant EL as "ELoot Script"
participant Game as "Game Inventory"
UI->>EL: Load settings
EL->>EL: seed_missing_defaults(settings)
EL->>EL: migrate_overflow_settings(settings)
EL-->>UI: Persist `overflow_containers` and flags
UI->>EL: User action (stow / sell / dump / hoard / skin)
EL->>Game: Resolve ordered `overflow_containers` and ensure stow entries
Game-->>EL: Container states / inventory / locked messages
EL->>Game: Execute action using resolved containers (iterate overflow list, consult `sacks_full` hash)
Game-->>EL: Updated per-container fullness
EL-->>UI: Update messages / UI state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/eloot.lic (1)
3727-3733:⚠️ Potential issue | 🟠 MajorTrack full sacks by stable identity, not display name.
Using
bag.nameas the hash key means two different containers with the same visible name are treated as the same sack. With multiple overflow containers, filling one"backpack"will incorrectly blacklist every other"backpack"as full too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 3727 - 3733, The sacks_full cache is keyed by the visible name (bag.name) which conflates distinct containers with identical display names; change the key to a stable identifier (e.g., bag.id or a composite "#{bag.name}-#{bag.id}") when writing to ELoot.data.sacks_full in the sacks_full(...) logic and update the debug message to show that identifier instead of bag.name; also search for any reads of ELoot.data.sacks_full and update them to use the same stable key so lookups and blacklisting reference the container's unique identity rather than its display name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Around line 1780-1829: migrate_overflow_settings currently calls ELoot.msg and
ELoot.save_profile while ELoot.data may be nil (ELoot.load hasn't run); remove
those direct calls from migrate_overflow_settings (no ELoot.msg and no
ELoot.save_profile) and instead have the method only mutate and return settings
and the migrated_settings flag (or accumulate human-readable migration messages
in a local array and return them), so the caller can perform logging and
persistence after ELoot.load; reference migrate_overflow_settings, the local
migrated_settings variable, ELoot.msg, ELoot.save_profile, ELoot.load, and
ELoot.data.settings when locating the changes.
- Around line 1799-1824: The migration strips overflow entries and can leave
"#{type}_horde_containers" empty which breaks validate_hoarding_settings; update
the migration in the loop that handles has_overflow (inside the code using
containers_key and use_overflow_key) so that after computing
settings[containers_key] = old_containers.reject { ... } you check if that array
is empty and, if so, populate it with a safe fallback container (e.g., add a
default like 'container1' or another existing default used by your app) while
still setting settings[use_overflow_key] = true, so legacy overflow-only
profiles remain valid after migration.
- Around line 3558-3579: The loop that iterates containers
(containers.each_with_index) calls bag.name before checking for nil, which
crashes when the type-specific StowList.stow_list[item.type.to_sym] is missing;
modify the iteration so you skip nil bags early (or check bag.nil? immediately
upon entry) before any use of bag.name or indexing into ELoot.data.sacks_full,
ensuring the nil check occurs before referencing bag.name so the fallback to
StowList.stow_list[:default] and overflow containers works safely.
---
Outside diff comments:
In `@scripts/eloot.lic`:
- Around line 3727-3733: The sacks_full cache is keyed by the visible name
(bag.name) which conflates distinct containers with identical display names;
change the key to a stable identifier (e.g., bag.id or a composite
"#{bag.name}-#{bag.id}") when writing to ELoot.data.sacks_full in the
sacks_full(...) logic and update the debug message to show that identifier
instead of bag.name; also search for any reads of ELoot.data.sacks_full and
update them to use the same stable key so lookups and blacklisting reference the
container's unique identity rather than its display name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Around line 3611-3613: The guard is inverted: because ELoot.data.sacks_full
always includes "last cleared", the current "unless
ELoot.data.sacks_full.keys.count > 1" suppresses diagnostics when a real sack is
present; change the condition to the positive form so diagnostics print when
there are real full sacks (replace the 'unless ... > 1' with 'if
ELoot.data.sacks_full.keys.count > 1' around the ELoot.data.sacks_full.each
block that calls ELoot.msg).
- Around line 1776-1832: ELoot.load is not backfilling newly added non-boolean
settings (e.g., :trash_dump_types) when loading legacy YAML, causing runtime nil
errors (Sell.dump_herbs_junk calling .select). Fix by seeding missing defaults
during load migration: inside ELoot.load (after @@data = Data.new(settings)) or
at the top of ELoot.migrate_overflow_settings, ensure keys introduced since
Setup#validate_settings exist (for example set settings[:trash_dump_types] = []
or the module's canonical default, and set any new boolean/array/hash keys to
their defaults) and mark migrated_settings true only if you actually changed
anything so ELoot.save_profile() still runs when needed; reference ELoot.load,
ELoot.migrate_overflow_settings, and Setup#validate_settings to locate where to
add these default assignments.
- Around line 3718-3721: The current lines.each block treats failure messages as
success by returning true for patterns like /You are unable to handle|That is
not yours|Get what|I could not find what you were referring/, which causes
callers such as Inventory.single_drag to mark moves successful when they failed;
change the logic so those patterns return false (i.e., treat them as failures)
and only return true for actual success patterns (e.g., not the "belongs to" or
failure patterns), updating the block that uses lines.each and line.match? to
reflect this inverted/safer result handling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/eloot.lic (2)
3720-3723:⚠️ Potential issue | 🟠 MajorDon't report failed
_dragresponses as a successful store.These branches still return
truefor messages likeThat is not yours,Get what, andI could not find what you were referring....Inventory.single_dragtreats that as “stored”, so the script can continue with the item still in hand.Suggested fix
lines.each do |line| - return false if line.match?(/Hey, that belongs to/) - return true if line.match?(/You are unable to handle|That is not yours|Get what|I could not find what you were referring/) + return false if line.match?(/Hey, that belongs to|You are unable to handle|That is not yours|Get what|I could not find what you were referring/) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 3720 - 3723, The loop in lines.each currently treats failure messages ("You are unable to handle", "That is not yours", "Get what", "I could not find what you were referring") as successful by returning true, which makes Inventory.single_drag think the item was stored; change the logic in the lines.each block (the return true/false decisions) so those failure regexes return false instead of true, and only known success confirmations return true (or invert to explicitly match success messages), ensuring Inventory.single_drag only advances when a genuine store confirmation is seen.
1776-1832:⚠️ Potential issue | 🟠 MajorBackfill new settings during non-UI loads.
Legacy YAML profiles still bypass
Setup#validate_settings, so keys introduced here can remainniluntil the user opens GTK setup. That leaves paths likeSell.dump_herbs_junkcalling.selecton:trash_dump_typesand blowing up on the first sell run.Suggested fix
def self.migrate_overflow_settings(settings) migrated_settings = false + + { + overflow_containers: "", + trash_dump_types: ["herb", "junk", "food"], + skin_bounty_only: false, + gem_horde_containers: ["default", "gem"], + gem_horde_use_overflow: false, + alchemy_horde_containers: ["default", "reagent"], + alchemy_horde_use_overflow: false, + }.each do |key, default_value| + next unless settings[key].nil? + + settings[key] = default_value + migrated_settings = true + end + # Migration helper: Convert old overflow_container and secondary_overflow to new overflow_containers format if settings.key?(:overflow_container) || settings.key?(:secondary_overflow)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1776 - 1832, Ensure new settings keys are backfilled during non-UI loads by adding default values when missing inside ELoot.load or at the end of ELoot.migrate_overflow_settings; specifically, guarantee settings[:overflow_containers] exists (set to '' or [] as your code expects), ensure settings[:gem_horde_use_overflow] and settings[:alchemy_horde_use_overflow] are booleans (default false), and also initialize other new keys that callers expect (e.g. :trash_dump_types -> default to []), then call ELoot.save_profile() if any defaults were added so legacy YAML profiles do not leave nil values that break methods like Sell.dump_herbs_junk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Around line 2297-2302: In self.in_region, avoid calling .id on a potentially
nil result from tag_for_town: capture the result (e.g., tag =
self.tag_for_town(bounty_town, place)), return nil immediately if tag is nil,
and only then set place = tag.id; this preserves the nil fallback that
Region.furrier / Region.gemshop expect and prevents an exception when
Bounty.town refers to stale/unsupported map data.
---
Duplicate comments:
In `@scripts/eloot.lic`:
- Around line 3720-3723: The loop in lines.each currently treats failure
messages ("You are unable to handle", "That is not yours", "Get what", "I could
not find what you were referring") as successful by returning true, which makes
Inventory.single_drag think the item was stored; change the logic in the
lines.each block (the return true/false decisions) so those failure regexes
return false instead of true, and only known success confirmations return true
(or invert to explicitly match success messages), ensuring Inventory.single_drag
only advances when a genuine store confirmation is seen.
- Around line 1776-1832: Ensure new settings keys are backfilled during non-UI
loads by adding default values when missing inside ELoot.load or at the end of
ELoot.migrate_overflow_settings; specifically, guarantee
settings[:overflow_containers] exists (set to '' or [] as your code expects),
ensure settings[:gem_horde_use_overflow] and
settings[:alchemy_horde_use_overflow] are booleans (default false), and also
initialize other new keys that callers expect (e.g. :trash_dump_types -> default
to []), then call ELoot.save_profile() if any defaults were added so legacy YAML
profiles do not leave nil values that break methods like Sell.dump_herbs_junk.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/eloot.lic (1)
1835-1915:⚠️ Potential issue | 🔴 Critical
defaults_hashis not actually canonical, and fresh/headless profiles can still boot with missing settings.
load_defaultsnow persists this hash directly, but it omits a lot of settings that the runtime later treats as arrays/strings (:critter_exclude,:sell_keep_scrolls,:gem_horde_containers,:alchemy_horde_containers, etc.). A first-run profile can therefore hitnil-method crashes before the user ever opens GTK. It also disagrees with the existinglocksmith_withdraw_amountdefault (10000here vs8000elsewhere), so behavior can silently change depending on which path created the profile.🛠️ Safer direction
- def self.defaults_hash - { - ... - } - end + def self.defaults_hash + categories = ELoot::Setup.class_variable_get(:@@categories) + + categories.each_with_object({}) do |(_, settings), defaults| + settings.each do |key, meta| + default = meta[:default] + defaults[key] = + if default.is_a?(Array) || default.is_a?(Hash) + default.dup + else + default + end + end + end + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1835 - 1915, defaults_hash currently omits several keys the runtime expects and contains an inconsistent locksmith_withdraw_amount, causing nil-method crashes for fresh/headless profiles; update defaults_hash to include all runtime-used keys (e.g. :critter_exclude, :sell_keep_scrolls, :gem_horde_containers, :alchemy_horde_containers and other array/string keys) with safe defaults (empty array or empty string as appropriate) and make locksmith_withdraw_amount match the canonical value used elsewhere (e.g. 8000), and change load_defaults to not blindly overwrite the profile file with the minimal hash — instead load an existing eloot.yaml if present, call seed_missing_defaults(existing_settings) to backfill missing keys, persist the merged settings, and return the merged hash so new and existing profiles always have the full canonical set.
♻️ Duplicate comments (1)
scripts/eloot.lic (1)
2310-2324:⚠️ Potential issue | 🟠 Major
tag_for_towncan still raise before this guard runs.
destination = self.tag_for_town(...)is only safe iftag_for_townitself can returnnil. Right now it still calls.laston the result of.find, so stale/unsupported town mappings will explode beforereturn nil unless destinationgets a chance to run.🔧 Nil-safe lookup
def self.tag_for_town(town, tag) ELoot.msg(type: "debug", text: "tag: #{tag} | town: #{town}") - self.by_town(tag).find { |k, _v| k.downcase.include?(town.downcase) }.last + match = self.by_town(tag).find { |k, _v| k.downcase.include?(town.downcase) } + match&.last end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 2310 - 2324, The method tag_for_town can raise because it calls .last on the result of find which may be nil; update tag_for_town so it safely handles a missing entry (e.g. capture the find result into a variable and return nil if it's nil, or use a safe navigation operator) so that in_region's call destination = self.tag_for_town(...) can legitimately return nil and the subsequent guard return nil unless destination will work; refer to the tag_for_town and in_region methods and ensure by_town(tag).find { |k,_v| ... } is checked for nil before calling .last.
🧹 Nitpick comments (1)
scripts/eloot.lic (1)
1907-1915: Drop the redundantDATA_DIR/<game>mkdir.The runtime already creates
DATA_DIR/#{XMLData.game}before.licscripts start, so only the per-character directory needs to be ensured here.Based on learnings: In elanthia-online/scripts, the runtime pre-creates DATA_DIR/ before any script runs. Do not defensively create the data directory in .lic scripts (e.g., map.lic); avoid FileUtils.mkdir_p for the data directory since existence is guaranteed by the runtime. Rely on the guaranteed directory and handle write errors if they occur.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1907 - 1915, Remove the redundant creation of the game-level directory in self.load_defaults: stop calling Dir.mkdir for File.join(DATA_DIR, XMLData.game) (the runtime guarantees DATA_DIR/<game> exists) and only ensure the per-character directory (File.join(DATA_DIR, XMLData.game, Char.name)) before calling File.write to create eloot.yaml; keep references to DATA_DIR, XMLData.game, Char.name, and the File.write call and handle any File.write exceptions as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Around line 2125-2143: Add removal of the legacy :secondary_overflow key from
StowList.stow_list along with the other cleanup: after the existing
StowList.stow_list.delete_if call (or immediately before iterating
overflow_list) explicitly delete StowList.stow_list[:secondary_overflow] so any
stale legacy entry is purged and won't be used by ELoot.ensure_items or
downstream sell/open logic.
---
Outside diff comments:
In `@scripts/eloot.lic`:
- Around line 1835-1915: defaults_hash currently omits several keys the runtime
expects and contains an inconsistent locksmith_withdraw_amount, causing
nil-method crashes for fresh/headless profiles; update defaults_hash to include
all runtime-used keys (e.g. :critter_exclude, :sell_keep_scrolls,
:gem_horde_containers, :alchemy_horde_containers and other array/string keys)
with safe defaults (empty array or empty string as appropriate) and make
locksmith_withdraw_amount match the canonical value used elsewhere (e.g. 8000),
and change load_defaults to not blindly overwrite the profile file with the
minimal hash — instead load an existing eloot.yaml if present, call
seed_missing_defaults(existing_settings) to backfill missing keys, persist the
merged settings, and return the merged hash so new and existing profiles always
have the full canonical set.
---
Duplicate comments:
In `@scripts/eloot.lic`:
- Around line 2310-2324: The method tag_for_town can raise because it calls
.last on the result of find which may be nil; update tag_for_town so it safely
handles a missing entry (e.g. capture the find result into a variable and return
nil if it's nil, or use a safe navigation operator) so that in_region's call
destination = self.tag_for_town(...) can legitimately return nil and the
subsequent guard return nil unless destination will work; refer to the
tag_for_town and in_region methods and ensure by_town(tag).find { |k,_v| ... }
is checked for nil before calling .last.
---
Nitpick comments:
In `@scripts/eloot.lic`:
- Around line 1907-1915: Remove the redundant creation of the game-level
directory in self.load_defaults: stop calling Dir.mkdir for File.join(DATA_DIR,
XMLData.game) (the runtime guarantees DATA_DIR/<game> exists) and only ensure
the per-character directory (File.join(DATA_DIR, XMLData.game, Char.name))
before calling File.write to create eloot.yaml; keep references to DATA_DIR,
XMLData.game, Char.name, and the File.write call and handle any File.write
exceptions as needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/eloot.lic (2)
1838-1905:⚠️ Potential issue | 🔴 Critical
defaults_hashis still incomplete for first-run and non-UI loads.
load_defaults/seed_missing_defaultsonly populate the keys listed here, but runtime paths later call array methods on settings that are missing from this hash (:loot_keep,:critter_exclude,:between, several hoard lists/flags, etc.). A brand-new profile can therefore crash on the first loot/sell run without ever opening GTK. There is already drift inside the shared defaults too (:locksmith_withdraw_amountis10000here but8000inSetup/Data), so the behavior depends on which code path initialized the profile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1838 - 1905, The defaults_hash is missing several keys used at runtime (e.g. :loot_keep, :critter_exclude, :between, hoard lists/flags and other list/boolean settings) and one value (:locksmith_withdraw_amount) differs from Setup/Data; update defaults_hash to include all keys referenced by runtime code with safe empty-array or false/zero defaults (add :loot_keep => [], :critter_exclude => [], :between => [], each hoard list/flag => [], and other expected arrays/booleans) and align :locksmith_withdraw_amount to the canonical value used in Setup/Data so seed_missing_defaults reliably initializes a brand-new profile without GTK.
2125-2143:⚠️ Potential issue | 🟠 MajorPurge stale
overflow_container_*entries before rebuilding the dynamic list.This cleanup only deletes
:overflow_containerand:secondary_overflow. If the user reloads or reopens setup in the same session after reducing the overflow list, old:overflow_container_2+entries remain inStowList.stow_listand still participate in sell/open/hoard flows.🧹 Minimal fix
- StowList.stow_list.delete_if { |k, _| [:overflow_container, :secondary_overflow, :appraisal_container].include?(k) } + StowList.stow_list.delete_if do |k, _| + k.to_s.start_with?('overflow_container') || k == :secondary_overflow || k == :appraisal_container + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 2125 - 2143, StowList.stow_list deletion only removes :overflow_container and :secondary_overflow but not dynamically numbered symbols like :overflow_container_2, so update the cleanup to delete any keys whose symbol name matches the overflow pattern; e.g., in the existing delete_if for StowList.stow_list, change the predicate to inspect k.to_s and remove keys matching /^overflow_container(?:_\d+)?$/ as well as :secondary_overflow and :appraisal_container so all old overflow_container_N entries are purged before rebuilding (reference StowList.stow_list and the temporary key logic using "overflow_temp_#{index}" and ensure_items).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Around line 2317-2324: The nil-check in in_region doesn't prevent exceptions
from tag_for_town because tag_for_town uses .find(...).last without guarding
when find returns nil; update tag_for_town to safely handle a missing lookup
(e.g., check the result of .find(...) before calling .last, use safe navigation
or return nil when no match) so it never raises for unsupported/stale bounty
towns, then in_region can rely on destination being nil and continue to return
nil as intended.
---
Duplicate comments:
In `@scripts/eloot.lic`:
- Around line 1838-1905: The defaults_hash is missing several keys used at
runtime (e.g. :loot_keep, :critter_exclude, :between, hoard lists/flags and
other list/boolean settings) and one value (:locksmith_withdraw_amount) differs
from Setup/Data; update defaults_hash to include all keys referenced by runtime
code with safe empty-array or false/zero defaults (add :loot_keep => [],
:critter_exclude => [], :between => [], each hoard list/flag => [], and other
expected arrays/booleans) and align :locksmith_withdraw_amount to the canonical
value used in Setup/Data so seed_missing_defaults reliably initializes a
brand-new profile without GTK.
- Around line 2125-2143: StowList.stow_list deletion only removes
:overflow_container and :secondary_overflow but not dynamically numbered symbols
like :overflow_container_2, so update the cleanup to delete any keys whose
symbol name matches the overflow pattern; e.g., in the existing delete_if for
StowList.stow_list, change the predicate to inspect k.to_s and remove keys
matching /^overflow_container(?:_\d+)?$/ as well as :secondary_overflow and
:appraisal_container so all old overflow_container_N entries are purged before
rebuilding (reference StowList.stow_list and the temporary key logic using
"overflow_temp_#{index}" and ensure_items).
Use safe navigation operator to handle nil case.
Contributor
Author
|
@coderabbitai summary |
Contributor
✅ Actions performedSummary regeneration triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/eloot.lic (1)
1911-1912: Drop defensive creation ofDATA_DIR/<game>in defaults loading.
Line 1911should not createDATA_DIR/<game>defensively in this repo runtime; keep only the per-character directory handling and file write error handling.Suggested diff
- Dir.mkdir(File.join(DATA_DIR, XMLData.game)) unless File.exist?(File.join(DATA_DIR, XMLData.game)) Dir.mkdir(File.join(DATA_DIR, XMLData.game, Char.name)) unless File.exist?(File.join(DATA_DIR, XMLData.game, Char.name))Based on learnings: In elanthia-online/scripts, the runtime pre-creates DATA_DIR/ before any script runs; avoid defensively creating that data directory in
.licscripts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1911 - 1912, Remove the defensive creation of the game-level data directory: delete the Dir.mkdir(File.join(DATA_DIR, XMLData.game)) conditional that creates DATA_DIR/<game>, but keep the per-character directory creation (the Dir.mkdir(File.join(DATA_DIR, XMLData.game, Char.name)) unless File.exist? check). Ensure any file writes that rely on DATA_DIR/<game>/<char> handle and surface errors (e.g. rescue and log/write error) rather than pre-creating DATA_DIR via XMLData.game; reference the Dir.mkdir calls, DATA_DIR, XMLData.game, and Char.name to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Around line 6466-6468: The branch handling the /You don't have that much/
response currently sets redo_needed = false and thus silently skips retrying the
current box; change the logic so that when the regex /You don't have that much/
is matched the code triggers a retry of the current box (e.g., set redo_needed =
true or push the box back for reprocessing) instead of clearing redo_needed,
modifying the when /You don't have that much/ clause in scripts/eloot.lic so the
deposit attempt is retried and the pool state remains consistent; keep the
existing handling for the other branch (when /already holding as many boxes/)
unchanged.
- Around line 2237-2256: The error message uses label.split(' ').last which
returns generic words instead of the configured overflow name; fix by storing
the actual configured name into the checks entry when building overflow_list and
use that stored name in the error branch. Specifically, when pushing into checks
inside the overflow_list.each_with_index block, include the original
container_name (e.g. checks << [:overflow_containers, StowList.stow_list[key],
label, container_name]) and then change the error path in the checks.each loop
to use that fourth element (the configured name) instead of label.split('
').last when setting container_name for the ELoot.msg call; reference symbols:
overflow_list, checks, StowList.stow_list, ELoot.msg.
---
Nitpick comments:
In `@scripts/eloot.lic`:
- Around line 1911-1912: Remove the defensive creation of the game-level data
directory: delete the Dir.mkdir(File.join(DATA_DIR, XMLData.game)) conditional
that creates DATA_DIR/<game>, but keep the per-character directory creation (the
Dir.mkdir(File.join(DATA_DIR, XMLData.game, Char.name)) unless File.exist?
check). Ensure any file writes that rely on DATA_DIR/<game>/<char> handle and
surface errors (e.g. rescue and log/write error) rather than pre-creating
DATA_DIR via XMLData.game; reference the Dir.mkdir calls, DATA_DIR,
XMLData.game, and Char.name to locate the change.
* ensure_items caller check uses include? to match 'block in set_inventory' frame label, suppressing internal temp key leak in error messages
* validate_setup passes configured container name through checks tuple instead of deriving it from label.split(' ').last (returned slot number)
* explicit StowList.stow_list.delete(:secondary_overflow) for defensive legacy key cleanup
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/eloot.lic (1)
6730-6740:⚠️ Potential issue | 🟠 Major
always_check_poolis still bypassed on gem-bounty locksmith runs.
skip_for_gem_bountycurrently suppresses the whole pool block, soSell.pool_returnnever runs when boxes are present andlocksmith_when_gem_bountyis enabled. Ready boxes stay in the pool even thoughalways_check_poolis on.🔧 Minimal fix
- if should_check_pool && !skip_for_gem_bounty - Sell.locksmith_pool(boxes) if ELoot.data.settings[:sell_locksmith_pool] - Sell.pool_return # retrieve (and loot) any boxes in the pool - - # Refresh the boxes - boxes = ELoot.find_boxes - end + if should_check_pool + deposited_to_pool = ELoot.data.settings[:sell_locksmith_pool] && !skip_for_gem_bounty + Sell.locksmith_pool(boxes) if deposited_to_pool + Sell.pool_return if ELoot.data.settings[:always_check_pool] || deposited_to_pool + boxes = ELoot.find_boxes + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 6730 - 6740, The code currently skips the entire pool handling when skip_for_gem_bounty is true, so ELoot.data.settings[:always_check_pool] does not trigger Sell.pool_return; change the control flow so that should_check_pool still executes Sell.pool_return and refreshes boxes even if skip_for_gem_bounty is true, but only run Sell.locksmith_pool when skip_for_gem_bounty is false; locate the block using should_check_pool, skip_for_gem_bounty, Sell.locksmith_pool and Sell.pool_return and reorder/split the conditions so Sell.pool_return (and boxes = ELoot.find_boxes) always run when should_check_pool is true while guarding only the Sell.locksmith_pool call with the skip_for_gem_bounty check.
♻️ Duplicate comments (2)
scripts/eloot.lic (2)
1837-1917:⚠️ Potential issue | 🔴 Critical
defaults_hashis still incomplete for headless/default loads.
seed_missing_defaultsandload_defaultsnow treat this hash as canonical, but it omits settings that changed runtime paths dereference without nil guards — e.g.:critter_excludeinLoot.search,:loot_keepinLoot.reject_invalid_loot,:betweeninSell.inbetween_scripts, plus the hoarding defaults. A fresh profile created here can still crash before the user ever opens GTK. It also drifts from the other default sources on:locksmith_withdraw_amount(10000here vs8000inELoot::Setup/ELoot::Data).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1837 - 1917, The defaults_hash used by defaults_hash/seed_missing_defaults/load_defaults is missing keys that headless loads rely on (e.g. :critter_exclude used by Loot.search, :loot_keep used by Loot.reject_invalid_loot, :between used by Sell.inbetween_scripts and the hoarding-related defaults), and the :locksmith_withdraw_amount value is inconsistent with ELoot::Setup/ELoot::Data; update defaults_hash to include the missing keys with safe nil/empty defaults (arrays/false/empty strings as appropriate) and change :locksmith_withdraw_amount to match the canonical value from ELoot::Setup/ELoot::Data so fresh profiles won't dereference nils before GTK loads.
1778-1834:⚠️ Potential issue | 🔴 CriticalAvoid migration logging/persistence during
ELoot.load.
ELoot.loadreaches this helper before any command path initializesELoot.data.debug_logger. If an upgraded profile hasdebug_fileenabled, the newELoot.msg(...)calls and the unsilencedELoot.save_profile()path both go throughELoot.msg, which will hitELoot.data.debug_loggerwhile it is stillniland turn migration into a startup crash.🩹 Safer pattern
def self.load(settings) @@data = Data.new(settings) - # Migrate old overflow settings to new format - ELoot.migrate_overflow_settings(ELoot.data.settings) + migrated, messages = ELoot.migrate_overflow_settings(ELoot.data.settings) + ELoot.save_profile(silent: true) if migrated + messages.each { |message| ELoot.msg(text: message) } end def self.migrate_overflow_settings(settings) migrated_settings = ELoot.seed_missing_defaults(settings) + migration_messages = [] ... - ELoot.msg(text: " Migrated old overflow container settings to new format: #{settings[:overflow_containers]}") + migration_messages << " Migrated old overflow container settings to new format: #{settings[:overflow_containers]}" ... - ELoot.msg(text: " Migrated #{type} hoarding overflow settings: enabled 'Use All Overflow Containers'") + migration_messages << " Migrated #{type} hoarding overflow settings: enabled 'Use All Overflow Containers'" ... - if migrated_settings - ELoot.save_profile() - end + [migrated_settings, migration_messages] end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1778 - 1834, ELoot.load calls ELoot.migrate_overflow_settings before ELoot.data.debug_logger is initialized, so remove side-effects in migrate_overflow_settings: stop calling ELoot.msg and ELoot.save_profile from inside ELoot.migrate_overflow_settings (and stop deleting/overwriting settings in-place if that triggers persistence), instead have ELoot.migrate_overflow_settings return a result object/boolean and optional messages (e.g., return {migrated: true, messages: [...]}) and let ELoot.load decide when to persist or log (after ELoot.data is initialized and ELoot.data.debug_logger is present); update callers to perform ELoot.msg and ELoot.save_profile only when ELoot.data.debug_logger is non-nil or when invoked outside startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/eloot.lic`:
- Around line 6730-6740: The code currently skips the entire pool handling when
skip_for_gem_bounty is true, so ELoot.data.settings[:always_check_pool] does not
trigger Sell.pool_return; change the control flow so that should_check_pool
still executes Sell.pool_return and refreshes boxes even if skip_for_gem_bounty
is true, but only run Sell.locksmith_pool when skip_for_gem_bounty is false;
locate the block using should_check_pool, skip_for_gem_bounty,
Sell.locksmith_pool and Sell.pool_return and reorder/split the conditions so
Sell.pool_return (and boxes = ELoot.find_boxes) always run when
should_check_pool is true while guarding only the Sell.locksmith_pool call with
the skip_for_gem_bounty check.
---
Duplicate comments:
In `@scripts/eloot.lic`:
- Around line 1837-1917: The defaults_hash used by
defaults_hash/seed_missing_defaults/load_defaults is missing keys that headless
loads rely on (e.g. :critter_exclude used by Loot.search, :loot_keep used by
Loot.reject_invalid_loot, :between used by Sell.inbetween_scripts and the
hoarding-related defaults), and the :locksmith_withdraw_amount value is
inconsistent with ELoot::Setup/ELoot::Data; update defaults_hash to include the
missing keys with safe nil/empty defaults (arrays/false/empty strings as
appropriate) and change :locksmith_withdraw_amount to match the canonical value
from ELoot::Setup/ELoot::Data so fresh profiles won't dereference nils before
GTK loads.
- Around line 1778-1834: ELoot.load calls ELoot.migrate_overflow_settings before
ELoot.data.debug_logger is initialized, so remove side-effects in
migrate_overflow_settings: stop calling ELoot.msg and ELoot.save_profile from
inside ELoot.migrate_overflow_settings (and stop deleting/overwriting settings
in-place if that triggers persistence), instead have
ELoot.migrate_overflow_settings return a result object/boolean and optional
messages (e.g., return {migrated: true, messages: [...]}) and let ELoot.load
decide when to persist or log (after ELoot.data is initialized and
ELoot.data.debug_logger is present); update callers to perform ELoot.msg and
ELoot.save_profile only when ELoot.data.debug_logger is non-nil or when invoked
outside startup.
Fix multiple bugs related to looting and inventory management.
mrhoribu
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/eloot.lic (1)
1839-1919:⚠️ Potential issue | 🔴 CriticalFresh profiles are still missing settings outside the GTK path.
ELoot.load_defaultsandELoot.seed_missing_defaultsnow depend onELoot.defaults_hash, but this hash only covers part ofELoot::Setup's settings. A first-run profile still leaves keys like:between,:critter_exclude, and:skin_excludeunset, so normal non-UI paths such asELoot::Sell.inbetween_scriptsandELoot::Loot.searchcan still hitnil.lengthuntil the user opens setup once. This source has already drifted too (:locksmith_withdraw_amountis10000here but8000inELoot::Setup/ELoot::Data), so the gap will keep resurfacing unless the defaults are made complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1839 - 1919, defaults_hash is incomplete and drifts from ELoot::Setup/ELoot::Data, causing fresh profiles to miss keys like :between, :critter_exclude, :skin_exclude and a mismatched :locksmith_withdraw_amount; update ELoot.defaults_hash to include the full canonical set of settings used by ELoot::Setup/ELoot::Data (add missing keys such as :between, :critter_exclude, :skin_exclude, etc.) and reconcile any differing values (e.g., set :locksmith_withdraw_amount to the canonical value used elsewhere), so that ELoot.load_defaults and ELoot.seed_missing_defaults always seed a complete, consistent defaults hash and persist it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Line 6853: Remove the trailing whitespace on the blank line flagged by RuboCop
(Layout/TrailingWhitespace) by deleting the space characters at the end of that
line or trimming end-of-file whitespace so the line is empty (no spaces/tabs) —
this will clear the RuboCop offense.
- Around line 4743-4745: ELoot.get_res can return nil on timeout/unmatched
responses, but the current code only uses line&.match? to skip locked boxes—so a
nil (timeout) falls through and triggers the next "look in" against a box that
never opened; update the block using ELoot.get_res (the local variable line) to
explicitly check for line.nil? first and handle it (e.g., next/continue or retry
as appropriate) before evaluating line.match?(/It appears to be locked/) and
ensure any comparisons use result.nil? to detect timeout/unmatched cases rather
than result == false.
- Around line 4515-4516: Inventory.drag is being called with a keyword-style
hash (to: 'left') but the method signature is def self.drag(item, to = "hand") —
change each call to pass the direction as the second positional argument (e.g.,
replace Inventory.drag(bottle, to: 'left') with Inventory.drag(bottle, 'left'))
so the second parameter is a String not a Hash; update the three occurrences
referencing Inventory.drag with items named bottle, empty and thing accordingly
(use 'left' or 'right' as appropriate) so generated _drag commands become valid.
---
Duplicate comments:
In `@scripts/eloot.lic`:
- Around line 1839-1919: defaults_hash is incomplete and drifts from
ELoot::Setup/ELoot::Data, causing fresh profiles to miss keys like :between,
:critter_exclude, :skin_exclude and a mismatched :locksmith_withdraw_amount;
update ELoot.defaults_hash to include the full canonical set of settings used by
ELoot::Setup/ELoot::Data (add missing keys such as :between, :critter_exclude,
:skin_exclude, etc.) and reconcile any differing values (e.g., set
:locksmith_withdraw_amount to the canonical value used elsewhere), so that
ELoot.load_defaults and ELoot.seed_missing_defaults always seed a complete,
consistent defaults hash and persist it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
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)
scripts/eloot.lic (1)
1839-1909:⚠️ Potential issue | 🔴 Critical
defaults_hashstill isn't safe as the headless default source.
load_defaultsandseed_missing_defaultsnow both trust this hash, but it still omits runtime keys that are later dereferenced without nil guards. A first-run or non-GTK profile can still hitnil.lengthin paths likeLoot.reject_invalid_loot(:loot_keep) orSell.inbetween_scripts(:between) before setup ever runs. Please make this truly canonical, or extract the shared settings schema out ofSetupso both paths use the same source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1839 - 1909, defaults_hash is missing runtime keys used by other modules (causing nil.length errors in Loot.reject_invalid_loot and Sell.inbetween_scripts); update the canonical defaults so headless profiles include those runtime keys or centralize the schema in Setup and have both load_defaults and seed_missing_defaults use it. Fix by adding the missing runtime keys (at least :loot_keep and :between with safe empty-array/string defaults and any other keys dereferenced before Setup runs) to defaults_hash, or refactor to extract the shared settings schema from Setup into a single method (e.g., Setup.settings_schema) and have defaults_hash/seed_missing_defaults call that instead so both paths use the exact same canonical defaults.
♻️ Duplicate comments (2)
scripts/eloot.lic (2)
4743-4744:⚠️ Potential issue | 🟡 MinorDon't treat a timed-out
openas an unlocked box.
ELoot.get_rescan returnnilhere.line&.match?only skips the locked case, so a timeout/unmatched response still falls through intolook inand loot handling against a box that may never have opened.🔧 Minimal fix
- next if line&.match?(/It appears to be locked/) + next if line.nil? || line.match?(/It appears to be locked/)Based on learnings, in the elanthia-online/scripts context, when dothistimeout returns nil on timeout or failed to match, prefer checking result.nil? rather than result == false to distinguish timeout/unmatched cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 4743 - 4744, ELoot.get_res can return nil on timeout/unmatched responses, so update the handling after calling ELoot.get_res("open ##{box.id}", /open|locked/) to explicitly skip (next) when line.nil? before proceeding to any "look in" or loot logic; preserve the existing locked check (line&.match?(/It appears to be locked/)) but ensure the nil-check happens first—use nil? rather than comparing to false to correctly detect timeouts/unmatched responses.
4515-4516:⚠️ Potential issue | 🔴 CriticalPass the hand target positionally to
Inventory.drag.
Inventory.dragtakes a positional second argument. In Ruby, theseto:calls pass a Hash instead of'left'/'right', so the generated_dragcommand receives{:to=>...}and the hoard deposit flow won't move the jar/item into the intended hand.🛠️ Minimal fix
- Inventory.drag(bottle, to: 'left') + Inventory.drag(bottle, 'left') @@ - Inventory.drag(empty, to: 'left') + Inventory.drag(empty, 'left') @@ - Inventory.drag(thing, to: 'right') + Inventory.drag(thing, 'right')#!/bin/bash set -euo pipefail # Expect one positional method signature and the keyword-style call sites below. rg -n 'def self\.drag\(item, to = "hand"\)' scripts/eloot.lic rg -n 'Inventory\.drag\([^)]*to:\s*' scripts/eloot.licAlso applies to: 4526-4526, 4557-4557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 4515 - 4516, The calls to Inventory.drag use Ruby keyword style (e.g., Inventory.drag(bottle, to: 'left')) which passes a Hash instead of the positional second argument expected by def self.drag(item, to = "hand"); update these calls to pass the hand as a positional argument (e.g., Inventory.drag(bottle, 'left')) so the generated _drag command receives the correct string; search for other occurrences like Inventory.drag(..., to: ...) (notably the similar calls near the hoard deposit flow) and convert them the same way to use the positional second parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/eloot.lic`:
- Around line 2129-2147: The rebuild removes the legacy :secondary_overflow key
so scripts like sellunder lose the second overflow container; preserve
compatibility by creating an alias to the second overflow entry after you
populate StowList.stow_list. In the overflow_list.each_with_index loop (symbols:
overflow_list, key, temp_settings_key, ELoot.ensure_items, StowList.stow_list),
once you move the found item into StowList.stow_list[key] and if index == 1 also
set StowList.stow_list[:secondary_overflow] to reference that same entry (or
otherwise ensure :secondary_overflow points to the second overflow container) so
existing code that reads :secondary_overflow continues to work.
- Around line 1791-1795: The current code concatenates overflow container names
into a comma-separated string (settings[:overflow_containers] =
containers.join(', ')), which breaks names containing commas; instead store the
array itself (e.g., settings[:overflow_containers] = containers) so the YAML
preserves exact names, and only apply containers.join(', ') at the GTK
display/rendering layer; update the identical spots that build
overflow_containers from settings[:overflow_container] and
settings[:secondary_overflow] (the other blocks using containers and
settings[:overflow_containers]) to follow the same pattern.
---
Outside diff comments:
In `@scripts/eloot.lic`:
- Around line 1839-1909: defaults_hash is missing runtime keys used by other
modules (causing nil.length errors in Loot.reject_invalid_loot and
Sell.inbetween_scripts); update the canonical defaults so headless profiles
include those runtime keys or centralize the schema in Setup and have both
load_defaults and seed_missing_defaults use it. Fix by adding the missing
runtime keys (at least :loot_keep and :between with safe empty-array/string
defaults and any other keys dereferenced before Setup runs) to defaults_hash, or
refactor to extract the shared settings schema from Setup into a single method
(e.g., Setup.settings_schema) and have defaults_hash/seed_missing_defaults call
that instead so both paths use the exact same canonical defaults.
---
Duplicate comments:
In `@scripts/eloot.lic`:
- Around line 4743-4744: ELoot.get_res can return nil on timeout/unmatched
responses, so update the handling after calling ELoot.get_res("open ##{box.id}",
/open|locked/) to explicitly skip (next) when line.nil? before proceeding to any
"look in" or loot logic; preserve the existing locked check (line&.match?(/It
appears to be locked/)) but ensure the nil-check happens first—use nil? rather
than comparing to false to correctly detect timeouts/unmatched responses.
- Around line 4515-4516: The calls to Inventory.drag use Ruby keyword style
(e.g., Inventory.drag(bottle, to: 'left')) which passes a Hash instead of the
positional second argument expected by def self.drag(item, to = "hand"); update
these calls to pass the hand as a positional argument (e.g.,
Inventory.drag(bottle, 'left')) so the generated _drag command receives the
correct string; search for other occurrences like Inventory.drag(..., to: ...)
(notably the similar calls near the hoard deposit flow) and convert them the
same way to use the positional second parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
scripts/eloot.lic (4)
1791-1796:⚠️ Potential issue | 🟡 MinorPersist overflow container names losslessly.
The migration stores
overflow_containersas a comma-joined string and every read path splits on commas again. Any legitimate container name containing a comma will be corrupted into multiple bogus entries after save/load. Store the array in YAML and only join it for the GTK text field.Also applies to: 2135-2136, 2242-2249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1791 - 1796, The migration currently joins container names with commas into settings[:overflow_containers], which will corrupt names containing commas; instead preserve the array by serializing it (e.g., YAML.dump or equivalent) into settings[:overflow_containers] and only perform a comma-join when creating the GTK text field display; update the code paths that read/write overflow_containers (the migration block that builds containers, the assignments at lines referenced around settings[:overflow_containers], and any readers that split on commas) to read the serialized array back (YAML.load) and avoid splitting on commas except in the UI rendering function that populates the GTK text field.
4743-4745:⚠️ Potential issue | 🟡 MinorHandle the timeout/unmatched open path explicitly.
ELoot.get_rescan returnnilhere. The current&.match?only skips locked boxes, so a timeout falls through and the rest of the method proceeds as if the box opened.Based on learnings, in elanthia-online/scripts when `dothistimeout` returns `nil` on timeout or failed match, prefer `result.nil?` to handle the timeout/unmatched path explicitly.🩹 Minimal fix
box_list.each do |box| line = ELoot.get_res("open ##{box.id}", /open|locked/) - next if line&.match?(/It appears to be locked/) + next if line.nil? || line.match?(/It appears to be locked/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 4743 - 4745, ELoot.get_res called for "open ##{box.id}" can return nil on timeout/unmatched results, but the current code only checks for the locked case via line&.match?; update the logic around the local variable line (result of ELoot.get_res) to explicitly handle nil (e.g., next or other timeout handling) before checking for /It appears to be locked/ so timeouts don't fall through and get treated as successful opens; keep the locked-match check (line.match?(/It appears to be locked/)) after the nil check.
3749-3752:⚠️ Potential issue | 🟠 MajorTreat
That is not yoursas a fallback failure.Callers stop iterating on
true. Returningtruehere meanssingle_drag/single_drag_boxcan stop on a non-owned target and leave the item in hand instead of trying the next overflow container or disk.🔧 Minimal fix
lines.each do |line| - return false if line.match?(/Hey, that belongs to/) - return true if line.match?(/You are unable to handle|That is not yours|Get what|I could not find what you were referring/) + return false if line.match?(/Hey, that belongs to|That is not yours/) + return true if line.match?(/You are unable to handle|Get what|I could not find what you were referring/) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 3749 - 3752, The current lines.each block returns true when matching /You are unable to handle|That is not yours|Get what|I could not find what you were referring/, which causes single_drag / single_drag_box to stop on a non-owned target; change the logic so "That is not yours" is treated as a failure (return false) instead of success — either remove "That is not yours" from the success regex and add a separate condition that returns false when line.match?(/That is not yours/), or explicitly check for that string before the success check so that the block returns false for that match while keeping other success patterns returning true.
1839-1908:⚠️ Potential issue | 🔴 Critical
defaults_hashis still incomplete for non-GTK loads.Fresh/legacy profiles that only go through
load_defaults/seed_missing_defaultscan still crash before setup is ever opened. Runtime paths already assume keys like:loot_keep,:critter_exclude,:between, and*_horde_containersexist, but this hash does not seed them. This has also already drifted from the GTK/Data defaults (locksmith_withdraw_amountis10000here but8000elsewhere), so it is not a safe single source of truth yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/eloot.lic` around lines 1839 - 1908, The defaults_hash is missing several keys used by runtime when GTK isn’t loaded (causing crashes) and has at least one divergent value; update the self.defaults_hash method to include the missing keys (e.g., :loot_keep, :critter_exclude, :between and any horde container keys like :monster_horde_containers / :animal_horde_containers or the exact *_horde_containers symbols used elsewhere) and set their sensible canonical defaults, and sync any diverged values (e.g., change :locksmith_withdraw_amount from 10000 to the canonical 8000) so defaults_hash is the single source of truth used by seed_missing_defaults and load_defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/eloot.lic`:
- Around line 1791-1796: The migration currently joins container names with
commas into settings[:overflow_containers], which will corrupt names containing
commas; instead preserve the array by serializing it (e.g., YAML.dump or
equivalent) into settings[:overflow_containers] and only perform a comma-join
when creating the GTK text field display; update the code paths that read/write
overflow_containers (the migration block that builds containers, the assignments
at lines referenced around settings[:overflow_containers], and any readers that
split on commas) to read the serialized array back (YAML.load) and avoid
splitting on commas except in the UI rendering function that populates the GTK
text field.
- Around line 4743-4745: ELoot.get_res called for "open ##{box.id}" can return
nil on timeout/unmatched results, but the current code only checks for the
locked case via line&.match?; update the logic around the local variable line
(result of ELoot.get_res) to explicitly handle nil (e.g., next or other timeout
handling) before checking for /It appears to be locked/ so timeouts don't fall
through and get treated as successful opens; keep the locked-match check
(line.match?(/It appears to be locked/)) after the nil check.
- Around line 3749-3752: The current lines.each block returns true when matching
/You are unable to handle|That is not yours|Get what|I could not find what you
were referring/, which causes single_drag / single_drag_box to stop on a
non-owned target; change the logic so "That is not yours" is treated as a
failure (return false) instead of success — either remove "That is not yours"
from the success regex and add a separate condition that returns false when
line.match?(/That is not yours/), or explicitly check for that string before the
success check so that the block returns false for that match while keeping other
success patterns returning true.
- Around line 1839-1908: The defaults_hash is missing several keys used by
runtime when GTK isn’t loaded (causing crashes) and has at least one divergent
value; update the self.defaults_hash method to include the missing keys (e.g.,
:loot_keep, :critter_exclude, :between and any horde container keys like
:monster_horde_containers / :animal_horde_containers or the exact
*_horde_containers symbols used elsewhere) and set their sensible canonical
defaults, and sync any diverged values (e.g., change :locksmith_withdraw_amount
from 10000 to the canonical 8000) so defaults_hash is the single source of truth
used by seed_missing_defaults and load_defaults.
prevents situation when you have both `pink sapphire` gem and `sparkling pink sapphire band` jewelry in your inventory causing it to select both on single select to jar.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
This PR updates
eloot.licto use a single overflow container setting, adds configurable trash dump types, and includes various bug fixes and improvements.overflow_containerandsecondary_overflowwithoverflow_containers(comma-separated) ineloot.lic.trash_dump_typessetting, replacing hardcoded herb/food/junk.--sellable/type/sellCLI.single_drag_boxcycling through all group disks whenuse_disk_groupis false.migrate_overflow_settings()to convert old overflow settings to new format.sacks_fullto store additional information.This description was created by
for 5415732. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes