Skip to content

Conversation

@AndresMorelos
Copy link
Contributor

  • Introduced sdk field to the places schema, allowing for filtering by SDK version.
  • Updated API endpoints to support SDK version filtering in destinations and places queries.
  • Enhanced OpenAPI documentation to include SDK version as a query parameter.
  • Added migration to include sdk column in the places table with appropriate indexing.
  • Updated relevant functions and tests to accommodate the new SDK version functionality.

AndresMorelos and others added 4 commits January 13, 2026 08:10
- Introduced `sdk` field to the places schema, allowing for filtering by SDK version.
- Updated API endpoints to support SDK version filtering in destinations and places queries.
- Enhanced OpenAPI documentation to include SDK version as a query parameter.
- Added migration to include `sdk` column in the places table with appropriate indexing.
- Updated relevant functions and tests to accommodate the new SDK version functionality.
- Added missing imports for `destinationRoute`, `sceneStatsGenesisPlaza`, and other entities across various files.
- Cleaned up import statements in `CategoryFilters`, `CategoryList`, and `OnlyViewCategoryNavbar` for better readability.
- Ensured consistent import structure in `SearchList`, `UserFavorite`, and `UserLikes` routes.
- Updated tests to reflect the new import structure and ensure proper functionality.
@github-actions
Copy link

github-actions bot commented Jan 13, 2026

Pull Request Test Coverage Report for Build 21249696444

Details

  • 762 of 972 (78.4%) changed or added relevant lines in 21 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.1%) to 91.578%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/entities/Destination/utils.ts 65 71 91.55%
src/entities/Destination/routes/getDestinationsList.ts 191 214 89.25%
src/api/CommsGatekeeper.ts 40 101 39.6%
src/entities/Place/model.ts 280 400 70.0%
Totals Coverage Status
Change from base Build 20864026152: -1.1%
Covered Lines: 14405
Relevant Lines: 15495

💛 - Coveralls

Copy link
Member

@aleortega aleortega left a comment

Choose a reason for hiding this comment

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

Hey PR looks good to me!

Yet I left some doubts/comments to address!

type: integer
minimum: 0
default: 0
- name: positions
Copy link
Member

Choose a reason for hiding this comment

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

NIT

Hey, in our ubiquitous language, we usually refer to positions as pointers or parcels

Comment on lines 30 to 32
if (ctx.url.searchParams.get("order_by") === PlaceListOrderBy.MOST_ACTIVE) {
return getDestinationsMostActiveList(ctx)
}
Copy link
Member

Choose a reason for hiding this comment

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

any reason for handling this case separately?

getUnifiedDestinationsListQuerySchema
)

export const getUnifiedDestinationsList = Router.memo(
Copy link
Member

Choose a reason for hiding this comment

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

same doubt on this one, would it be possible to merge all three handlers?

'400':
$ref: '#/components/responses/BadRequest'

/destinations/unified:
Copy link
Member

Choose a reason for hiding this comment

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

are we missing /destinations API Spec? anyways, do we need both endpoints? could we only have a single one?

Comment on lines 28 to 29
router.get("/destinations", getDestinationsList)
router.get("/destinations/unified", getUnifiedDestinationsList)
Copy link
Member

Choose a reason for hiding this comment

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

do we need both endpoints? what's the difference between them? can we handle that difference with a new query or path param?

Comment on lines 3 to 36
export type GetDestinationsListQuery = {
limit: string
offset: string
positions: string[]
world_names: string[]
only_favorites: string
only_highlighted: string
order_by: string
order: string
with_realms_detail: string
search: string
categories: string[]
owner?: string
creator_address?: string
only_worlds: string
only_places: string
}

export type DestinationsListOptions = {
offset: number
limit: number
only_favorites: boolean
only_highlighted: boolean
positions: string[]
world_names: string[]
order_by: string
order: string
search: string
categories: string[]
owner?: string
creator_address?: string
only_worlds: boolean
only_places: boolean
}
Copy link
Member

Choose a reason for hiding this comment

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

it seems the only difference between these two types is the with_realms_detail, could we create a union so it is clear?

* Query parameters for the unified destinations endpoint
* Supports filtering by coordinates (exact match), names (LIKE), search, SDK, and more
*/
export type GetUnifiedDestinationsListQuery = {
Copy link
Member

Choose a reason for hiding this comment

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

maybe same for this one

Copy link
Member

Choose a reason for hiding this comment

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

and the one below

Comment on lines +1378 to +1422
${conditional(
!!options.user && options.only_favorites,
SQL`RIGHT JOIN ${table(
UserFavoriteModel
)} uf on p.id = uf.place_id AND uf."user" = ${options.user}`
)}
${conditional(
!!options.categories.length,
SQL`INNER JOIN ${table(
PlaceCategories
)} pc ON p.id = pc.place_id AND pc.category_id IN ${values(
options.categories
)}`
)}
${conditional(
!!options.search,
SQL`, ts_rank_cd(p.textsearch, to_tsquery(${tsquery(
options.search || ""
)})) as rank`
)}
WHERE
p."disabled" is false
${conditional(options.only_highlighted, SQL`AND p.highlighted = TRUE`)}
${conditional(options.only_places, SQL`AND p.world is false`)}
${conditional(options.only_worlds, SQL`AND p.world is true`)}
${conditional(!!options.search, SQL` AND rank > 0`)}
${conditional(!!placesOrWorldsCondition, placesOrWorldsCondition)}
${conditional(
!!options.owner,
SQL` AND (LOWER(p.owner) = ${options.owner} ${
options.operatedPositions?.length
? SQL`OR p.base_position IN (
SELECT DISTINCT(base_position)
FROM ${table(PlacePositionModel)}
WHERE position IN ${values(options.operatedPositions)}
)`
: SQL``
})`
)}
${conditional(
!!options.creator_address,
SQL` AND LOWER(p.creator_address) = ${options.creator_address}`
)}
Copy link
Member

Choose a reason for hiding this comment

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

are we using the exact same conditions as the query? could we extract the conditionals so we only maintain a single piece of code for both things?

/**
* Count unified destinations with the given filters
*/
static async countUnifiedDestinations(
Copy link
Member

Choose a reason for hiding this comment

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

same here, we could merge the conditionals of these two related functions

- Added `ranking` field to places and worlds schemas for custom ordering.
- Updated destinations API to include `with_connected_users` query parameter, allowing retrieval of connected user wallet addresses.
- Enhanced OpenAPI documentation to reflect new fields and query parameters.
- Removed deprecated unified destinations endpoint and adjusted related functions and tests accordingly.
- Reformatted the environment variables section in README for improved readability.
- Removed trailing whitespace in CommsGatekeeper and test files.
- Cleaned up formatting in the destinations utility and routes for consistency.
- Updated method names in CommsGatekeeper from `getSceneRoomParticipants` to `getSceneParticipants` and from `getWorldRoomParticipants` to `getWorldParticipants` for improved clarity.
- Adjusted related test cases and utility functions to reflect the new method names.
- Revised the description of the destinations API endpoint to clarify that it combines places and worlds with enhanced filtering capabilities, removing the mention of the deprecated unified endpoint.
…ltering

- Revised the destinations API documentation to clarify sorting behavior, emphasizing the order of highlighted items, ranking, and specified sort order.
- Renamed `positions` to `pointer` in the API and internal schemas for consistency.
- Enhanced SDK version filtering to include places with null SDK values across relevant files.
- Updated sorting logic in the PlaceModel to prioritize highlighted items, followed by ranking and activity.
- Reformatted conditional statements for SDK filtering in the PlaceModel to enhance readability and maintain consistency across the codebase.
Copy link
Member

@aleortega aleortega left a comment

Choose a reason for hiding this comment

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

Hey, leaving the approve and some NIT (no-blockers) comments to iterate here or in a new PR (depending on our rush)!

type: integer
minimum: 0
maximum: 100
default: 100
Copy link
Member

Choose a reason for hiding this comment

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

NIT:
isn't default = 100 too much?

Comment on lines 877 to 878
Filter places by specific parcel coordinates (exact match).
Format: `"x,y"` (e.g., `"-23,-96"`).
Copy link
Member

Choose a reason for hiding this comment

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

NIT:

Suggested change
Filter places by specific parcel coordinates (exact match).
Format: `"x,y"` (e.g., `"-23,-96"`).
Filter places by specific parcel coordinates (exact match). Worlds are excluded.
Format: `"x,y"` (e.g., `"-23,-96"`).

- Places
- Worlds
summary: List destinations (places + worlds)
security: []
Copy link
Member

Choose a reason for hiding this comment

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

NIT:

should we reference to auth-chain sec mechanism as optional since it is required for only_favorites use case?

export default class CommsGatekeeper extends API {
static Url = env(
`COMMS_GATEKEEPER_URL`,
"https://comms-gatekeeper.decentraland.org"
Copy link
Member

Choose a reason for hiding this comment

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

NIT (default to zone to prevent hitting prod from local environment):

Suggested change
"https://comms-gatekeeper.decentraland.org"
"https://comms-gatekeeper.decentraland.zone"


static Cache = new Map<string, CommsGatekeeper>()

static from(url: string) {
Copy link
Member

Choose a reason for hiding this comment

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

is this used? sorry I am not used to gatsby template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just copy the same behavior of the others API files


// Get positions from hot scenes for MOST_ACTIVE ordering
const hotScenesPositions =
query.order_by === PlaceListOrderBy.MOST_ACTIVE
Copy link
Member

Choose a reason for hiding this comment

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

hot-scenes will be only shown when this specific sorting is provided?

Copy link
Member

Choose a reason for hiding this comment

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

what about sorting order (ASC/DESC)?

},
names: {
type: "array",
maxItems: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

I think setting maxItems as 1000 will make us hit the max url length limitation imposed by browsers, can be handled client-side tho

Comment on lines +1177 to +1181
if (hasNames) {
const nameLikeConditions = options.names.map(
(name: string) =>
SQL`LOWER(p.world_name) LIKE ${`%${name.toLowerCase()}%`}`
)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a search? otherwise consider not doing a LIKE and concat .dcl.eth to the name to ensure explicit match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a search

Comment on lines +1213 to +1216
if ((hasWorldNames || hasNames) && !options.only_places) {
const worldConditions = buildWorldConditions()
return SQL`AND (${join(worldConditions, SQL` OR `)})`
}
Copy link
Member

Choose a reason for hiding this comment

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

will this ever execute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will be executed in these scenarios:

  1. No positions provided, but world_names or names are provided: positions=[], world_names=["test.dcl.eth"]
  2. only_worlds=true with any filters: positions=["0,0"], world_names=["test.dcl.eth"], only_worlds=true

options.user && !isEthereumAddress(options.user)
const searchIsEmpty = options.search && options.search.length < 3

if (isMissingEthereumAddress || searchIsEmpty) {
Copy link
Member

Choose a reason for hiding this comment

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

We need to have both for the count?

…er URL

- Added security requirements to the destinations API documentation, clarifying authentication options and their effects on response fields.
- Updated the description for the `only_favorites` query parameter to specify behavior for unauthenticated users.
- Changed the CommsGatekeeper URL to a new endpoint for improved service reliability.
- Removed the check for missing Ethereum address in the PlaceModel, streamlining the conditional logic to only validate if the search input is empty.
@AndresMorelos AndresMorelos merged commit 642a363 into master Jan 22, 2026
3 checks passed
@AndresMorelos AndresMorelos deleted the feat/re-discovery-changes branch January 22, 2026 13:36
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.

4 participants