Nik/spaces #582
Nik/spaces #582
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for distinguishing between PERSONAL and DAO space types in the hypergraph package. It introduces a type field to the PublicSpace schema and adds filtering capability by space type.
Changes:
- Added
typefield (either 'PERSONAL' or 'DAO') toPublicSpaceschema and query results - Refactored query construction from three static parameterized queries to dynamic query building based on filters
- Added
spaceTypefilter option toFindManyPublicFilterallowing filtering by space type, optionally combined with existing member/editor filters
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.changeset/add-space-type.md |
Documents the new type field and spaceType filter in the changeset |
.claude/settings.local.json |
Adds additional allowed bash commands for development tooling |
packages/hypergraph/src/space/find-many-public.ts |
Adds type field to schema, implements dynamic query building with spaceType filter support |
packages/hypergraph/test/space/find-many-public.test.ts |
Updates tests to include type field and adds test case for DAO type parsing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const buildFilterString = (filter?: FindManyPublicFilter): string | undefined => { | ||
| const conditions: string[] = []; | ||
|
|
||
| if (filter?.memberId) { | ||
| conditions.push(`members: {some: {memberSpaceId: {is: "${filter.memberId}"}}}`); | ||
| } | ||
|
|
||
| if (filter?.editorId) { | ||
| conditions.push(`editors: {some: {memberSpaceId: {is: "${filter.editorId}"}}}`); | ||
| } | ||
|
|
||
| if (filter?.spaceType) { | ||
| conditions.push(`type: {is: ${filter.spaceType}}`); | ||
| } | ||
|
|
||
| if (conditions.length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return `filter: {${conditions.join(', ')}}`; | ||
| }; | ||
|
|
||
| const buildSpacesQuery = (filter?: FindManyPublicFilter): string => { | ||
| const filterString = buildFilterString(filter); | ||
| const filterClause = filterString ? `(${filterString})` : ''; | ||
|
|
||
| return ` | ||
| query spaces { | ||
| spaces${filterClause} { | ||
| ${spaceFields} | ||
| } | ||
| } | ||
| `; | ||
| }; |
There was a problem hiding this comment.
The new buildFilterString and buildSpacesQuery functions lack test coverage. Consider adding tests to verify that:
- The spaceType filter generates the correct GraphQL query syntax
- Combinations of filters work correctly (e.g., memberId + spaceType, editorId + spaceType)
- Edge cases like undefined or only spaceType filters are handled properly
This is important since these functions dynamically construct GraphQL queries and any errors could result in invalid queries being sent to the server.
| if (filter?.memberId) { | ||
| conditions.push(`members: {some: {memberSpaceId: {is: "${filter.memberId}"}}}`); | ||
| } | ||
|
|
||
| if (filter?.editorId) { | ||
| conditions.push(`editors: {some: {memberSpaceId: {is: "${filter.editorId}"}}}`); |
There was a problem hiding this comment.
The memberId and editorId values are directly interpolated into the GraphQL query string without validation or escaping. The previous implementation used GraphQL variables (with $accountId parameter), which is safer as it prevents potential injection attacks. Consider either:
- Returning to the parameterized query approach with variables, or
- Adding explicit validation/sanitization of these string values before interpolation
This is particularly important since these IDs could come from user input or external sources.
| if (filter?.spaceType) { | ||
| conditions.push(`type: {is: ${filter.spaceType}}`); |
There was a problem hiding this comment.
The spaceType value is directly interpolated into the GraphQL query string without runtime validation. While the TypeScript type restricts it to 'PERSONAL' or 'DAO', there's no runtime check to ensure the value is valid before it's used in the query. Consider adding validation using the SpaceTypeSchema to ensure type safety at runtime, similar to how the PublicSpaceSchema is used for decoding. This will prevent potential issues if the value is manipulated at runtime or comes from an untyped source.
No description provided.