feat(spx-gui): add tilemap's layer zIndex property & dispose tilemap load the project #2285

Overu · 2025-09-26T07:39:56Z

Fixed #2283

…load the project

niupilot · 2025-09-26T07:40:02Z

PR Review: Tilemap Layer zIndex Property & Disposal

Changes Summary:

Tilemap disposal: Added proper cleanup in Project constructor disposal handler
Layer zIndex support: Added zIndex property to Layer type and implemented in load/export logic
Test coverage: Updated tests to verify layer zIndex functionality

Issues Found

🔧 High Priority Issues

Inconsistent zIndex Default Value (spx-gui/src/models/tilemap.ts:253)
- The code sets zIndex: z_index ?? 1 as default, but this creates inconsistency
- When z_index is 0 (falsy), it will default to 1 instead of preserving 0
- This could cause unexpected layer ordering behavior
Missing Type Safety (spx-gui/src/models/tilemap.ts:253)
- The z_index parameter should have explicit type annotation
- Current destructuring doesn't indicate expected type

⚠️ Medium Priority Issues

Incomplete Property Documentation
- The new zIndex property lacks JSDoc comments explaining its purpose and default behavior
- Should document expected range and meaning of zIndex values
Test Coverage Gap
- Tests only verify that zIndex values are preserved during load/export cycles
- Missing tests for edge cases like z_index: 0, z_index: undefined, negative values

💡 Code Quality Suggestions

Naming Consistency
- Property uses zIndex (camelCase) internally but z_index (snake_case) in config
- This is consistent with existing codebase patterns, but worth noting for maintenance

Architecture Assessment

✅ Good Design Decisions:

Proper integration with existing disposal pattern
Maintains backward compatibility with optional zIndex property
Consistent with existing tilemap property handling patterns
Test coverage for core functionality

✅ Implementation Quality:

Clean separation between internal representation and serialization format
Follows existing code conventions for similar properties
Proper null checking in disposal logic

Recommendations

Fix zIndex default handling - Use explicit check for undefined rather than falsy check
Add comprehensive edge case tests - Test behavior with 0, undefined, and negative values
Consider documentation - Add JSDoc for the new zIndex property explaining its purpose

Overall, this is a well-implemented feature that follows existing patterns. The main concern is the zIndex default value logic that could cause unexpected behavior.

👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

gemini-code-assist · 2025-09-26T07:40:11Z

Summary of Changes

Hello @Overu, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the tilemap system by adding a zIndex property to tilemap layers, which dictates their rendering order. It also addresses resource management by ensuring tilemaps are correctly disposed of and re-initialized during project lifecycle events, contributing to a more robust and flexible graphical user interface.

Highlights

Tilemap Layer Z-Index: Introduced a zIndex property for tilemap layers, allowing explicit control over their rendering order.
Tilemap Lifecycle Management: Implemented proper disposal of tilemap resources when a project is closed or reloaded, preventing memory leaks and ensuring clean state.
Configuration and Testing: Updated tilemap loading and export mechanisms to support the new zIndex property, along with comprehensive unit tests to validate its functionality.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces two main changes: it adds a zIndex property to tilemap layers for controlling their stacking order, and it ensures that the tilemap is properly disposed of and reset when a new project is loaded. The changes are well-implemented across the model, tests, and configuration. I have one suggestion to improve the default behavior for zIndex to make it more intuitive when it's not explicitly provided in the configuration.

gemini-code-assist · 2025-09-26T07:41:01Z

spx-gui/src/models/tilemap.ts

+    const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }) =>
+      id != null ? { id, name, tileData: tile_data, zIndex: z_index ?? 1 } : []
    )


The default value for zIndex is set to 1 if z_index is not provided in the configuration. This means that all layers without an explicit z_index will be assigned the same zIndex of 1, which could lead to an ambiguous rendering order.

A more robust approach would be to use the layer's index from the rawLayers array as the default zIndex. This would preserve the stacking order of layers as defined in the source file when z_index is not specified, making the behavior more predictable.

Suggested change

const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }) =>

id != null ? { id, name, tileData: tile_data, zIndex: z_index ?? 1 } : []

)

const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }, index) =>

id != null ? { id, name, tileData: tile_data, zIndex: z_index ?? index } : []

)

niupilot

Code Review Suggestions

Found 2 high-confidence issues that can be improved:

• Fix zIndex default value logic - Use explicit undefined check to preserve 0 values
• Add type annotation - Improve type safety for z_index parameter

These changes will prevent potential layer ordering bugs and improve code maintainability.

niupilot · 2025-09-26T07:42:17Z

spx-gui/src/models/tilemap.ts

    const decorators = rawDecorators.flatMap((d) => Decorator.load(d, currentDir, tileTextures) ?? [])
-    const layers = rawLayers.flatMap(({ id, name = '', tile_data = [] }) =>
-      id != null ? { id, name, tileData: tile_data } : []
+    const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }) =>


Fix zIndex default value logic and add type annotation. The original code z_index ?? 1 treats 0 as falsy and defaults it to 1, which could cause unexpected layer ordering. Using explicit undefined check preserves z_index: 0 while defaulting truly missing values to 1. Also adds proper type annotation for better type safety.

Suggested change

const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }) =>

const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }: { id?: number; name?: string; tile_data?: number[]; z_index?: number }) =>

id != null ? { id, name, tileData: tile_data, zIndex: z_index !== undefined ? z_index : 1 } : []

qiniu-ci · 2025-09-26T07:44:14Z

This PR has been deployed to the preview environment. You can explore it using the preview URL.

Warning

Please note that deployments in the preview environment are temporary and will be automatically cleaned up after a certain period. Make sure to explore it before it is removed. For any questions, contact the XBuilder team.

spx-gui/src/models/project/index.ts

spx-gui/src/models/tilemap.ts

…load the project

feat(spx-gui): add tilemap's layer zIndex property & dispose tilemap …

9bebc2e

…load the project

Overu requested a review from nighca September 26, 2025 07:39

gemini-code-assist bot reviewed Sep 26, 2025

View reviewed changes

niupilot bot reviewed Sep 26, 2025

View reviewed changes

vercel bot deployed to Preview – builder September 26, 2025 07:45 View deployment

nighca reviewed Sep 26, 2025

View reviewed changes

spx-gui/src/models/project/index.ts Outdated Show resolved Hide resolved

spx-gui/src/models/tilemap.ts Outdated Show resolved Hide resolved

feat(spx-gui): add tilemap's layer zIndex property & dispose tilemap …

a3537dc

…load the project

nighca approved these changes Sep 26, 2025

View reviewed changes

nighca enabled auto-merge (squash) September 26, 2025 08:39

vercel bot deployed to Preview – builder September 26, 2025 08:40 View deployment

nighca merged commit 5336ee8 into goplus:dev Sep 26, 2025
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(spx-gui): add tilemap's layer zIndex property & dispose tilemap load the project #2285

feat(spx-gui): add tilemap's layer zIndex property & dispose tilemap load the project #2285

Uh oh!

Overu commented Sep 26, 2025

Uh oh!

niupilot bot commented Sep 26, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot commented Sep 26, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Sep 26, 2025

Uh oh!

niupilot bot left a comment

Uh oh!

niupilot bot Sep 26, 2025

Uh oh!

qiniu-ci commented Sep 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }) =>
	const layers = rawLayers.flatMap(({ id, name = '', tile_data = [], z_index }: { id?: number; name?: string; tile_data?: number[]; z_index?: number }) =>
	id != null ? { id, name, tileData: tile_data, zIndex: z_index !== undefined ? z_index : 1 } : []

feat(spx-gui): add tilemap's layer zIndex property & dispose tilemap load the project #2285

feat(spx-gui): add tilemap's layer zIndex property & dispose tilemap load the project #2285

Uh oh!

Conversation

Overu commented Sep 26, 2025

Uh oh!

niupilot bot commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Tilemap Layer zIndex Property & Disposal

Issues Found

🔧 High Priority Issues

⚠️ Medium Priority Issues

💡 Code Quality Suggestions

Architecture Assessment

Recommendations

Uh oh!

gemini-code-assist bot commented Sep 26, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

niupilot bot left a comment

Choose a reason for hiding this comment

Code Review Suggestions

Uh oh!

niupilot bot Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

qiniu-ci commented Sep 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niupilot bot commented Sep 26, 2025 •

edited

Loading