Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/client labor #2666

Draft
wants to merge 7 commits into
base: next
Choose a base branch
from
Draft

Feat/client labor #2666

wants to merge 7 commits into from

Conversation

bob0005
Copy link
Collaborator

@bob0005 bob0005 commented Jan 25, 2025

Temp UI element to start production

Summary by CodeRabbit

  • New Features

    • Added labor table navigation option
    • Introduced new labor-related functionality for resource production
    • Enhanced resource management with labor inputs and production capabilities
  • Improvements

    • Streamlined resource and labor ID handling
    • Simplified resource icon and tooltip rendering
    • Improved configuration management for realm and resource systems
  • Bug Fixes

    • Refined initialization synchronization process
    • Updated resource cost calculations
  • Chores

    • Removed unused imports and commented code
    • Reorganized code structure in various components

Copy link
Contributor

mentatbot bot commented Jan 25, 2025

Hi @bob0005! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page.

You are receiving this comment because I am set to review all PRs. That is configurable here.

Copy link

vercel bot commented Jan 25, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 25, 2025 3:46pm
eternum-docs ❌ Failed (Inspect) Jan 25, 2025 3:46pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
eternum-landing ⬜️ Ignored (Inspect) Visit Preview Jan 25, 2025 3:46pm

Copy link
Contributor

coderabbitai bot commented Jan 25, 2025

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

client/apps/game/src/main.tsx

Oops! Something went wrong! :(

ESLint: 9.18.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

From ESLint v9.0.0, the default configuration file is now eslint.config.js.
If you are using a .eslintrc.* file, please follow the migration guide
to update your configuration file to the new format:

https://eslint.org/docs/latest/use/configure/migration-guide

If you still have problems after following the migration guide, please stop by
https://eslint.org/chat/help to chat with the team.

Walkthrough

This pull request introduces a comprehensive set of changes focused on implementing a labor production system in the game. The modifications span multiple files across the client and core packages, adding new components, enums, and methods to support labor-related functionality. Key additions include a new EntityLaborTable component, LaborChip for resource production, and updates to navigation and configuration management to accommodate labor-specific interactions and data handling.

Changes

File Path Change Summary
client/apps/game/src/types/index.ts Added LaborTable to RightView enum
client/apps/game/src/ui/config.tsx Added laborTable to MenuEnum
client/apps/game/src/ui/elements/resource-icon.tsx Added optional tooltipText prop
packages/core/src/constants/index.ts Introduced LaborIds enum
packages/core/src/constants/utils.ts Added getLaborIdFromResourceId and getResourceIdFromLaborId methods
packages/core/src/dojo/create-system-calls.ts Added start_production method, removed create_multiple_realms_dev
packages/core/src/types/provider.ts Added StartProductionProps interface

Sequence Diagram

sequenceDiagram
    participant User
    participant RightNavigationModule
    participant EntityLaborTable
    participant LaborChip
    participant SystemCalls

    User->>RightNavigationModule: Select Labor Table
    RightNavigationModule->>EntityLaborTable: Render with entityId
    EntityLaborTable->>LaborChip: Display Labor Options
    User->>LaborChip: Set Production Amount
    User->>LaborChip: Click "Start Producing"
    LaborChip->>SystemCalls: start_production(resourceType, amount)
    SystemCalls-->>LaborChip: Production Started
Loading

Suggested reviewers

  • aymericdelab

Poem

🐰 A Rabbit's Labor of Love 🛠️

In fields of code, a new path grows,
Where labor chips and tables pose,
A system crafted with rabbit's might,
Producing resources, shining bright!

Hop, hop, hurrah! Production's here! 🌱

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

Failed to generate code suggestions for PR

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (10)
packages/core/src/constants/utils.ts (1)

17-18: Mirror the same constant usage for clarity.
The same “255” offset reappears here. Reuse a dedicated constant for both functions to avoid drift and improve clarity.

- return 255 - laborType;
+ const ID_OFFSET = 255;
+ return ID_OFFSET - laborType;
packages/core/src/managers/config-manager.ts (4)

42-55: Modular initialization with a minor logging nitpick.
Splitting out initialization methods is a clean approach, but consider removing or downgrading the console.log in production to prevent log verbosity.

- console.log("setting dojo for config manager");
+ // console.debug("setting dojo for config manager");

Line range hint 136-174: Large default object for troop config.
Consider extracting the default object into a well-documented constant for clarity and maintainability.


563-578: Resource output initialization with commented labor cost.
You might reintroduce or remove the laborCost line. Leaving it commented can cause confusion.

- // laborCost: this.divideByPrecision(Number(productionConfig?.labor_cost)),
+ // TODO: Decide whether to remove or incorporate labor cost logic.

639-642: Structure cost handling limited to Hyperstructure.
Only Hyperstructure is assigned a construction cost. If you plan to add more structure types, expand this logic.

Do you want me to scaffold placeholders for future structures?

client/apps/game/src/ui/components/resources/entity-labor-table.tsx (1)

12-23: Add loading state and user feedback for production actions.

The production start function should:

  1. Show loading state during the async operation
  2. Provide user feedback on success/failure
  3. Validate input parameters before making the system call
+ const [isLoading, setIsLoading] = useState(false);
+
  const startProduction = async (resourceType: ResourcesIds, producedAmount: number) => {
+   if (producedAmount <= 0) {
+     console.error("Production amount must be positive");
+     return;
+   }
+   setIsLoading(true);
    try {
      await dojo.setup.systemCalls.start_production({
        signer: dojo.account.account,
        entity_id: entityId,
        resource_type: resourceType,
        amount: producedAmount,
      });
+     // TODO: Add toast notification for success
    } catch (error) {
      console.error("Failed to start production", error);
+     // TODO: Add toast notification for error
+   } finally {
+     setIsLoading(false);
    }
  };
client/apps/game/src/ui/components/resources/labor-chip.tsx (2)

48-50: Add disabled state and loading indicator to the production button.

The button should be disabled when:

  1. Production amount is 0 or invalid
  2. Production is in progress
-<Button variant="primary" onClick={() => startProduction(resourceId, productionAmount)}>
+<Button
+  variant="primary"
+  disabled={productionAmount <= 0 || isLoading}
+  onClick={() => startProduction(resourceId, productionAmount)}
+>
-  Start Producing
+  {isLoading ? 'Starting...' : 'Start Producing'}
 </Button>

27-27: Improve accessibility by adding ARIA labels.

Add proper ARIA labels to improve accessibility for screen readers.

-<div className={`flex gap-2 relative group items-center text-xs px-2 p-1 hover:bg-gold/20 `}>
+<div 
+  className={`flex gap-2 relative group items-center text-xs px-2 p-1 hover:bg-gold/20`}
+  role="region"
+  aria-label={`Labor production for ${findResourceById(resourceId)?.trait}`}
+>
client/apps/game/src/ui/components/hyperstructures/hyperstructure-resource-chip.tsx (1)

Line range hint 1-110: Fix code formatting issues.

The pipeline indicates formatting issues. Please run Prettier to format the code:

prettier --write "client/apps/game/src/ui/components/hyperstructures/hyperstructure-resource-chip.tsx"
🧰 Tools
🪛 GitHub Actions: lint

[warning] Code formatting issues detected. Run Prettier with --write to fix.

packages/core/src/provider/index.ts (1)

1142-1159: LGTM! Consider extracting contract address.

The implementation is correct and follows the pattern of other methods. However, consider extracting the contract address to avoid repetition.

Apply this diff to improve the code:

 public async start_production(props: SystemProps.StartProductionProps) {
   const { entity_id, resource_type, amount, signer } = props;
+  const contractAddress = getContractByName(this.manifest, `${NAMESPACE}-production_systems`);

   const makeProductionLaborCallData = {
-    contractAddress: getContractByName(this.manifest, `${NAMESPACE}-production_systems`),
+    contractAddress,
     entrypoint: "make_production_labor",
     calldata: [entity_id, resource_type, amount],
   };

   const burnProductionLaborCallData = {
-    contractAddress: getContractByName(this.manifest, `${NAMESPACE}-production_systems`),
+    contractAddress,
     entrypoint: "burn_production_labor",
     calldata: [entity_id, resource_type, amount],
   };

   const calldataArray = [makeProductionLaborCallData, burnProductionLaborCallData];
   return await this.executeAndCheckTransaction(signer, calldataArray);
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aef638a and 360a57b.

📒 Files selected for processing (21)
  • client/apps/game/src/main.tsx (1 hunks)
  • client/apps/game/src/types/index.ts (1 hunks)
  • client/apps/game/src/ui/components/hyperstructures/hyperstructure-resource-chip.tsx (3 hunks)
  • client/apps/game/src/ui/components/resources/entity-labor-table.tsx (1 hunks)
  • client/apps/game/src/ui/components/resources/entity-resource-table.tsx (2 hunks)
  • client/apps/game/src/ui/components/resources/labor-chip.tsx (1 hunks)
  • client/apps/game/src/ui/components/resources/resource-chip.tsx (3 hunks)
  • client/apps/game/src/ui/components/structures/construction/structure-construction-menu.tsx (2 hunks)
  • client/apps/game/src/ui/config.tsx (1 hunks)
  • client/apps/game/src/ui/elements/resource-icon.tsx (1 hunks)
  • client/apps/game/src/ui/modules/navigation/right-navigation-module.tsx (4 hunks)
  • contracts/game/src/models/config.cairo (0 hunks)
  • contracts/game/src/systems/config/contracts.cairo (0 hunks)
  • contracts/game/src/systems/production/contracts.cairo (0 hunks)
  • packages/core/src/constants/index.ts (1 hunks)
  • packages/core/src/constants/utils.ts (2 hunks)
  • packages/core/src/dojo/create-system-calls.ts (2 hunks)
  • packages/core/src/managers/config-manager.ts (22 hunks)
  • packages/core/src/managers/resource-manager.ts (2 hunks)
  • packages/core/src/provider/index.ts (1 hunks)
  • packages/core/src/types/provider.ts (1 hunks)
💤 Files with no reviewable changes (3)
  • contracts/game/src/models/config.cairo
  • contracts/game/src/systems/production/contracts.cairo
  • contracts/game/src/systems/config/contracts.cairo
🧰 Additional context used
🪛 Biome (1.9.4)
client/apps/game/src/ui/components/resources/entity-labor-table.tsx

[error] 26-26: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 GitHub Actions: lint
client/apps/game/src/ui/components/hyperstructures/hyperstructure-resource-chip.tsx

[warning] Code formatting issues detected. Run Prettier with --write to fix.

🔇 Additional comments (47)
packages/core/src/constants/utils.ts (2)

1-1: Import statement looks correct.
The new import for LaborIds aligns well with the added functions below.


13-15: Avoid magic number “255.”
This function hardcodes 255. Consider storing 255 in a named constant (e.g., const ID_OFFSET = 255;) or verifying that resourceType cannot exceed 255.

packages/core/src/managers/config-manager.ts (30)

18-18: New imports from “../types.”
Importing ResourceCost, TickIds, and TravelTypes appears consistent with the added functionality.


24-32: New properties for labor and resource initialization.
Defining laborInputs and other records at the class level centralizes configuration data. This approach is concise and maintainable.


Line range hint 98-105: Check for negative or unexpected resource weights.
The _getValueOrDefault usage is consistent, but confirm whether a negative or zero weight is acceptable.


Line range hint 108-115: Confirm default of 1 for travel stamina cost.
Returning a fallback of 1 might unintentionally penalize or benefit travel if that’s not the correct domain assumption.


Line range hint 118-125: Explore stamina default parity with travel.
A fallback of 1 for explore stamina cost should match domain expectations.


128-134: Zero explore reward fallback.
If the config is absent, a reward of 0 might undercut exploration incentives. Verify it’s correct.


Line range hint 177-189: Grace tick logic is straightforward.
The switch statement is fine; just confirm additional structure types do not require a custom immunity.


191-197: Zero battle delay fallback.
A zero delay could allow battles to happen too rapidly. Ensure this matches the game design.


Line range hint 199-234: Resource bridge fee configuration is robust.
Storing all fee distributions here is a good design, neatly enumerating each fee type.


Line range hint 236-243: Potential issues with zero tick interval.
A tick interval of 0 might trigger continuous updates or break logic. Verify this fallback.


Line range hint 247-278: Bank config defaults.
Returning 0 for all cost and fees is convenient but might be risky if the system expects a positive cost.


280-285: Capacity checks.
Confirm if a fallback of 0 capacity is feasible or if a minimum capacity is necessary.


Line range hint 287-299: Behavior if speed is zero.
A 0 speed might skip travel time or cause undefined behavior. Validate it.


Line range hint 301-319: No population or capacity fallback.
Returning zeros might cause building logic to fail if not handled.


Line range hint 321-339: Hyperstructure config fallback.
All-zero configuration might allow immediate completion or no progress. Double-check.


Line range hint 403-410: Base population capacity fallback.
If set to 0, population might never grow. Confirm whether this is the intended behavior.


Line range hint 412-421: Default resource outputs.
A fallback of 0 might be correct for some resources, but verify it doesn’t break progression or cause confusion.


Line range hint 423-445: Food cost fallback.
Check that a cost of 0 won’t result in free movement.


Line range hint 447-452: Troop stamina fallback.
0 stamina could lock troops or cause them to be perpetually exhausted.


Line range hint 477-485: No cost percent increase fallback.
A 0 percent might mean building costs never escalate. Confirm that’s desired.


Line range hint 487-502: Season bridge configuration.
A fallback of 0n seconds could allow immediate or indefinite closures. Confirm the impact on bridging.


Line range hint 504-520: Season defaults imply an ended season.
With isOver: true and endedAt: 0n, normal gaming flow could be skipped. Validate if that’s okay.


Line range hint 522-530: Weightless resources.
If resources are truly weightless, the code is correct. Otherwise, investigate whether weight 0 is an omission.


532-561: Realm upgrade cost initialization.
Good use of a loop to gather cost data, but consider if level 0 or above max level needs handling.


580-606: Building costs per resource.
Ensure that if a resource is invalid or missing, the loop doesn’t push undefined entries.


608-637: General building cost logic.
The iteration to fetch resource cost details is consistent.


643-653: Hyperstructure construction cost with min_amount.
Currently pulling the min_amount alone. If a max or random range is needed, verify the logic.


655-672: Total hyperstructure costs are well-organized.
Collects min and max amounts by resource tier. Implemented cleanly.


674-696: Labor inputs initialization.
Neatly retrieves labor costs for each resource, enhancing flexibility for future expansions.


698-703: Generic value fallback.
A solid utility pattern for fallback values.

client/apps/game/src/types/index.ts (1)

6-6: New “LaborTable” enum value.
Adding LaborTable under RightView seamlessly supports the new labor UI.

client/apps/game/src/ui/components/resources/entity-resource-table.tsx (1)

42-42: LGTM! Type safety improvement.

Good improvement replacing any with proper ResourcesIds type for better type safety.

packages/core/src/constants/index.ts (1)

48-75: 🛠️ Refactor suggestion

Document and validate the labor ID calculation logic.

The current implementation has several potential issues:

  1. The magic number 255 lacks documentation explaining its significance
  2. There's no validation that ResourcesIds values are less than 255
  3. Could generate duplicate LaborIds if ResourcesIds > 255

Let's verify if any ResourcesIds values could cause issues:

Consider adding:

  1. Documentation explaining the 255 constant
  2. Runtime validation
  3. A type-safe mapping function
/** Maximum value for resource IDs to ensure valid labor ID calculation */
const MAX_RESOURCE_ID = 255;

/** Maps resource IDs to labor IDs using bitwise complement within byte range */
function validateLaborId(resourceId: number): number {
  if (resourceId > MAX_RESOURCE_ID) {
    throw new Error(`Resource ID ${resourceId} exceeds maximum value ${MAX_RESOURCE_ID}`);
  }
  return MAX_RESOURCE_ID - resourceId;
}

export enum LaborIds {
  Stone = validateLaborId(ResourcesIds.Stone),
  Coal = validateLaborId(ResourcesIds.Coal),
  // ... rest of the mappings
}
client/apps/game/src/main.tsx (1)

75-75: LGTM! Proper initialization sequence.

The placement of initialSync after setupResult ensures correct initialization order and prevents duplicate sync calls.

client/apps/game/src/ui/components/hyperstructures/hyperstructure-resource-chip.tsx (1)

7-7: LGTM! Simplified resource identification logic.

The direct use of findResourceById makes the code more straightforward and reduces unnecessary function calls.

Also applies to: 73-73, 86-86

🧰 Tools
🪛 GitHub Actions: lint

[warning] Code formatting issues detected. Run Prettier with --write to fix.

client/apps/game/src/ui/modules/navigation/right-navigation-module.tsx (2)

38-52: LGTM! Well-structured labor table navigation.

The labor table navigation follows the existing pattern and includes proper conditional rendering based on structureEntityId.


99-100: LGTM! Clean conditional rendering.

The conditional rendering of resource and labor tables is well-implemented, ensuring only one view is active at a time.

client/apps/game/src/ui/config.tsx (1)

107-107: LGTM! Consistent enum extension.

The addition of laborTable to MenuEnum follows the existing pattern and integrates well with the navigation system.

client/apps/game/src/ui/components/structures/construction/structure-construction-menu.tsx (1)

118-118: Verify resource cost calculation after RESOURCE_PRECISION removal.

The removal of RESOURCE_PRECISION multiplication from the resource cost calculation could affect how costs are displayed. Please ensure this change is intentional and doesn't impact the accuracy of displayed resource costs.

client/apps/game/src/ui/elements/resource-icon.tsx (2)

1-1: LGTM! Improved SVG handling.

The change to use ReactComponent for the People icon import is a better approach for SVG handling.


Line range hint 12-12: LGTM! Enhanced tooltip customization.

The addition of the optional tooltipText prop improves component flexibility while maintaining backward compatibility.

packages/core/src/dojo/create-system-calls.ts (2)

113-115: LGTM! Clean implementation of start_production.

The new system call follows the established pattern and is properly integrated into the systemCalls export.


300-300: LGTM! Proper systemCalls update.

The start_production system call is correctly added to the exported systemCalls object.

packages/core/src/types/provider.ts (1)

241-245: LGTM! Well-structured interface for production system.

The StartProductionProps interface follows the established patterns and includes all necessary properties with appropriate types.

client/apps/game/src/ui/components/resources/resource-chip.tsx (1)

5-5: LGTM! Clean removal of labor-specific logic.

The removal of the isLabor prop simplifies the component by:

  • Relying solely on resourceId for resource identification
  • Moving labor-specific logic to dedicated components
  • Maintaining core functionality while reducing complexity

Also applies to: 81-81, 96-96, 99-99

};

return Object.entries(LaborIds).map(([_, laborId]) => {
if (isNaN(Number(laborId))) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace isNaN with Number.isNaN for safer type checking.

The isNaN function performs type coercion which can lead to unexpected results. Use Number.isNaN instead for more reliable number validation.

-    if (isNaN(Number(laborId))) return null;
+    if (Number.isNaN(Number(laborId))) return null;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (isNaN(Number(laborId))) return null;
if (Number.isNaN(Number(laborId))) return null;
🧰 Tools
🪛 Biome (1.9.4)

[error] 26-26: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

Comment on lines +43 to +47
<NumberInput
value={productionAmount}
onChange={(productionAmount) => setProductionAmount(productionAmount)}
className="col-span-7"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for production amount input.

The NumberInput component should have minimum/maximum value constraints and proper validation.

 <NumberInput
   value={productionAmount}
   onChange={(productionAmount) => setProductionAmount(productionAmount)}
+  min={0}
+  max={999999}
   className="col-span-7"
 />
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<NumberInput
value={productionAmount}
onChange={(productionAmount) => setProductionAmount(productionAmount)}
className="col-span-7"
/>
<NumberInput
value={productionAmount}
onChange={(productionAmount) => setProductionAmount(productionAmount)}
min={0}
max={999999}
className="col-span-7"
/>

Comment on lines +29 to +39
{laborInputs?.map(({ resource, amount }) => (
<div key={resource} className="flex flex-row items-center">
<ResourceIcon
withTooltip={false}
resource={findResourceById(resource)?.trait as string}
size="sm"
className="mr-3 self-center"
/>
<div className="text-red">-{amount * productionAmount}</div>
</div>
))}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null check for laborInputs before mapping.

The current implementation might throw if laborInputs is undefined.

-  {laborInputs?.map(({ resource, amount }) => (
+  {laborInputs ? (
+    laborInputs.map(({ resource, amount }) => (
       <div key={resource} className="flex flex-row items-center">
         <ResourceIcon
           withTooltip={false}
           resource={findResourceById(resource)?.trait as string}
           size="sm"
           className="mr-3 self-center"
         />
         <div className="text-red">-{amount * productionAmount}</div>
       </div>
-    ))}
+    ))
+  ) : (
+    <div>No labor inputs configured</div>
+  )}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{laborInputs?.map(({ resource, amount }) => (
<div key={resource} className="flex flex-row items-center">
<ResourceIcon
withTooltip={false}
resource={findResourceById(resource)?.trait as string}
size="sm"
className="mr-3 self-center"
/>
<div className="text-red">-{amount * productionAmount}</div>
</div>
))}
{laborInputs ? (
laborInputs.map(({ resource, amount }) => (
<div key={resource} className="flex flex-row items-center">
<ResourceIcon
withTooltip={false}
resource={findResourceById(resource)?.trait as string}
size="sm"
className="mr-3 self-center"
/>
<div className="text-red">-{amount * productionAmount}</div>
</div>
))
) : (
<div>No labor inputs configured</div>
)}

Comment on lines +86 to +98
<a
className="text-brown cursor-pointer text-lg w-full"
href={`https://empire.realms.world/trade`}
target="_blank"
rel="noopener noreferrer"
>
<Button variant="secondary" className="w-full">
<div className="flex items-center gap-2">
<ResourceIcon resource="Lords" size="xs" />
Bridge Lords & Resources
</div>
</Button>
</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Environment-specific URL handling needs attention

While the link is secure with proper target="_blank" and rel="noopener noreferrer" attributes, there's an inconsistency in environment handling:

  • The bridge URL in right-navigation-module.tsx is hardcoded to mainnet (empire.realms.world/trade)
  • Other components (e.g., onboarding.tsx) properly switch between mainnet/non-mainnet URLs based on VITE_PUBLIC_CHAIN

Consider updating the URL to respect the environment setting:

const bridgeUrl = env.VITE_PUBLIC_CHAIN === "mainnet"
  ? "https://empire.realms.world/trade"
  : "https://empire-next.realms.world/trade";
🔗 Analysis chain

Verify security implications of external link.

The external link to empire.realms.world is properly implemented with security attributes (target="_blank" and rel="noopener noreferrer"). However, please verify:

  1. The URL is correct and points to the intended environment
  2. The feature flag or environment check is not needed for this link
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other instances of this URL to verify consistency
rg -l 'empire\.realms\.world'

# Search for environment-specific URL configurations
rg -l 'VITE_PUBLIC.*URL|VITE_PUBLIC.*ENDPOINT'

Length of output: 1059


Script:

#!/bin/bash
# Check how the URL is used in other files
rg -A 2 -B 2 'empire\.realms\.world' 

# Check environment configuration files for URL settings
cat client/apps/game/env.ts
cat client/apps/game/dojoConfig.ts

# Look for any feature flags related to trading or bridging
rg -i "feature.*flag.*bridge|feature.*flag.*trade"

Length of output: 8038

Comment on lines +36 to +42
public getLabor() {
const labor = getComponentValue(
this.components.Resource,
getEntityIdFromKeys([BigInt(this.entityId), BigInt(getLaborIdFromResourceId(this.resourceId))]),
);
return labor;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling to getLabor method.

The method should handle cases where getComponentValue returns undefined.

 public getLabor() {
   const labor = getComponentValue(
     this.components.Resource,
     getEntityIdFromKeys([BigInt(this.entityId), BigInt(getLaborIdFromResourceId(this.resourceId))]),
   );
-  return labor;
+  if (!labor) {
+    return null; // or a default value depending on your needs
+  }
+  return labor;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public getLabor() {
const labor = getComponentValue(
this.components.Resource,
getEntityIdFromKeys([BigInt(this.entityId), BigInt(getLaborIdFromResourceId(this.resourceId))]),
);
return labor;
}
public getLabor() {
const labor = getComponentValue(
this.components.Resource,
getEntityIdFromKeys([BigInt(this.entityId), BigInt(getLaborIdFromResourceId(this.resourceId))]),
);
if (!labor) {
return null; // or a default value depending on your needs
}
return labor;
}

Comment on lines +1121 to +1141
/**
// * Start production of a resource
// *
// * @param props - Properties for starting production
// * @param props.entity_id - ID of the entity that owns the production building
// * @param props.resource_type - Type of resource to start production of
// * @param props.labor_amount - Amount of resources to produce
// * @param props.signer - Account executing the transaction
// * @returns Transaction receipt
// *
// * @example
// * ```typescript
// * // Burn 100 units of labor for resource type 1
// * {
// * entity_id: 123,
// * resource_type: 1,
// * labor_amount: 100,
// * signer: account
// * }
// * ```
// */
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Uncomment and update the method documentation.

The documentation is currently commented out. It should be uncommented and the parameter name should be updated from labor_amount to amount to match the implementation.

Apply this diff to fix the documentation:

-  //  * Start production of a resource
-  //  *
-  //  * @param props - Properties for starting production
-  //  * @param props.entity_id - ID of the entity that owns the production building
-  //  * @param props.resource_type - Type of resource to start production of
-  //  * @param props.labor_amount - Amount of resources to produce
-  //  * @param props.signer - Account executing the transaction
-  //  * @returns Transaction receipt
-  //  *
-  //  * @example
-  //  * ```typescript
-  //  * // Burn 100 units of labor for resource type 1
-  //  * {
-  //  *   entity_id: 123,
-  //  *   resource_type: 1,
-  //  *   labor_amount: 100,
-  //  *   signer: account
-  //  * }
-  //  * ```
-  //  */
+  /**
+   * Start production of a resource
+   *
+   * @param props - Properties for starting production
+   * @param props.entity_id - ID of the entity that owns the production building
+   * @param props.resource_type - Type of resource to start production of
+   * @param props.amount - Amount of resources to produce
+   * @param props.signer - Account executing the transaction
+   * @returns Transaction receipt
+   *
+   * @example
+   * ```typescript
+   * // Burn 100 units of labor for resource type 1
+   * {
+   *   entity_id: 123,
+   *   resource_type: 1,
+   *   amount: 100,
+   *   signer: account
+   * }
+   * ```
+   */

@bob0005 bob0005 marked this pull request as draft January 25, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant