Skip to content

feat(world-builder): register uploaded graphics and extend palette schemas (#6) - #111

Open
Rodrigoue9 wants to merge 6 commits into
Bitcoindefi:mainfrom
Rodrigoue9:feat/registered-graphics-palette-extension
Open

feat(world-builder): register uploaded graphics and extend palette schemas (#6)#111
Rodrigoue9 wants to merge 6 commits into
Bitcoindefi:mainfrom
Rodrigoue9:feat/registered-graphics-palette-extension

Conversation

@Rodrigoue9

@Rodrigoue9 Rodrigoue9 commented Aug 20, 2026

Copy link
Copy Markdown

Summary

Resolves #6 by implementing paletteEntrySchema and validatePaletteEntry to validate multi-layer palette definitions, enforce non-colliding graphic index allocations (UPLOADED_GRAPHIC_INDEX_START = 1_000_000), and verify graphic existence across engine and uploaded assets.

Changes

  • Implemented paletteEntrySchema and validatePaletteEntry in api/src/repositories/worldBuilder.ts.
  • Added unit tests in api/src/repositories/__tests__/paletteValidation.test.ts.

Closes #6


Summary by Gitar

  • World Builder & Map Permissions:
    • Added protected map restrictions and override checks via canAccountEditMap in api/src/server.ts.
  • Graceful Shutdown:
    • Implemented gracefulShutdown logic with client closure and connection reset hooks in server/src/gracefulShutdown.ts and server/src/server.ts.

This will update automatically on new commits.

Comment on lines +231 to +245
export async function validatePaletteEntry(
entry: PaletteEntry,
): Promise<{ valid: boolean; reason?: string }> {
for (const grhIndex of entry.graphics) {
if (grhIndex >= UPLOADED_GRAPHIC_INDEX_START) {
const exists = await pool.query(
`SELECT 1 FROM game_uploaded_graphics WHERE grh_index = $1 LIMIT 1`,
[grhIndex],
);
if (exists.rowCount === 0) {
return {
valid: false,
reason: `El grafico ${grhIndex} no existe en el motor ni en assets subidos.`,
};
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Quality: validatePaletteEntry is never called and untested

The PR's stated goal is to validate multi-layer palette definitions, but validatePaletteEntry is not imported or invoked by any route or caller in the codebase (only its definition exists), and the new test file only exercises paletteEntrySchema, never validatePaletteEntry. As a result no palette entry is actually validated against existing/uploaded graphics at runtime and the collision/existence logic has zero test coverage. Wire the function into the palette-writing route(s) and add a unit test covering the uploaded-index existence branch (mocking pool.query).

Was this helpful? React with 👍 / 👎

Comment on lines +234 to +248
for (const grhIndex of entry.graphics) {
if (grhIndex >= UPLOADED_GRAPHIC_INDEX_START) {
const exists = await pool.query(
`SELECT 1 FROM game_uploaded_graphics WHERE grh_index = $1 LIMIT 1`,
[grhIndex],
);
if (exists.rowCount === 0) {
return {
valid: false,
reason: `El grafico ${grhIndex} no existe en el motor ni en assets subidos.`,
};
}
} else if (grhIndex <= 0) {
return {
valid: false,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Bug: Engine graphic indices are accepted without existence check

The function's docstring claims it verifies graphics exist "originales o subidos", but for any positive index below UPLOADED_GRAPHIC_INDEX_START (1,000,000) it performs no validation at all — including indices between the real engine max (320151) and 1,000,000, which are non-existent yet accepted as valid. This lets invalid engine indices pass validation. Either validate engine indices against the known upper bound (e.g. reject grhIndex > MAX_ENGINE_GRAPHIC_INDEX) or correct the docstring to reflect that only uploaded indices are checked.

Reject engine indices outside the valid original range instead of accepting them blindly.:

} else if (grhIndex <= 0 || grhIndex > 320151) {
    return {
        valid: false,
        reason: `Indice de grafico invalido: ${grhIndex}.`,
    };
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Aug 20, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 0 resolved / 2 findings

Registers uploaded graphics and extends palette schemas with validation logic, but validatePaletteEntry is never invoked or tested and fails to check engine graphic existence below the uploaded threshold.

⚠️ Quality: validatePaletteEntry is never called and untested

📄 api/src/repositories/worldBuilder.ts:231-245

The PR's stated goal is to validate multi-layer palette definitions, but validatePaletteEntry is not imported or invoked by any route or caller in the codebase (only its definition exists), and the new test file only exercises paletteEntrySchema, never validatePaletteEntry. As a result no palette entry is actually validated against existing/uploaded graphics at runtime and the collision/existence logic has zero test coverage. Wire the function into the palette-writing route(s) and add a unit test covering the uploaded-index existence branch (mocking pool.query).

💡 Bug: Engine graphic indices are accepted without existence check

📄 api/src/repositories/worldBuilder.ts:234-248

The function's docstring claims it verifies graphics exist "originales o subidos", but for any positive index below UPLOADED_GRAPHIC_INDEX_START (1,000,000) it performs no validation at all — including indices between the real engine max (320151) and 1,000,000, which are non-existent yet accepted as valid. This lets invalid engine indices pass validation. Either validate engine indices against the known upper bound (e.g. reject grhIndex > MAX_ENGINE_GRAPHIC_INDEX) or correct the docstring to reflect that only uploaded indices are checked.

Reject engine indices outside the valid original range instead of accepting them blindly.
} else if (grhIndex <= 0 || grhIndex > 320151) {
    return {
        valid: false,
        reason: `Indice de grafico invalido: ${grhIndex}.`,
    };
}
🤖 Prompt for agents
Code Review: Registers uploaded graphics and extends palette schemas with validation logic, but validatePaletteEntry is never invoked or tested and fails to check engine graphic existence below the uploaded threshold.

1. ⚠️ Quality: validatePaletteEntry is never called and untested
   Files: api/src/repositories/worldBuilder.ts:231-245

   The PR's stated goal is to validate multi-layer palette definitions, but `validatePaletteEntry` is not imported or invoked by any route or caller in the codebase (only its definition exists), and the new test file only exercises `paletteEntrySchema`, never `validatePaletteEntry`. As a result no palette entry is actually validated against existing/uploaded graphics at runtime and the collision/existence logic has zero test coverage. Wire the function into the palette-writing route(s) and add a unit test covering the uploaded-index existence branch (mocking `pool.query`).

2. 💡 Bug: Engine graphic indices are accepted without existence check
   Files: api/src/repositories/worldBuilder.ts:234-248

   The function's docstring claims it verifies graphics exist "originales o subidos", but for any positive index below `UPLOADED_GRAPHIC_INDEX_START` (1,000,000) it performs no validation at all — including indices between the real engine max (320151) and 1,000,000, which are non-existent yet accepted as valid. This lets invalid engine indices pass validation. Either validate engine indices against the known upper bound (e.g. reject `grhIndex > MAX_ENGINE_GRAPHIC_INDEX`) or correct the docstring to reflect that only uploaded indices are checked.

   Fix (Reject engine indices outside the valid original range instead of accepting them blindly.):
   } else if (grhIndex <= 0 || grhIndex > 320151) {
       return {
           valid: false,
           reason: `Indice de grafico invalido: ${grhIndex}.`,
       };
   }

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

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.

Etapa 1: registrar PNG subidos como graficos del motor y extender la paleta

1 participant