-
Notifications
You must be signed in to change notification settings - Fork 8
fix space id extraction #316
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
Conversation
31be751
to
ff51bd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures the space
parameter is consistently captured and forwarded through query hooks, adds an explicit fallback for missing context, and updates the Playground component to accept and propagate a spaceId
.
- Propagate the
space
argument fromuseQuery
intouseQueryPublic
anduseQueryLocal
- Introduce
space
inQueryPublicParams
andQueryParams
- Provide a default fallback in
useHypergraphSpaceInternal
and update Playground to takespaceId
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/hypergraph-react/src/use-query.tsx | Forward space to useQueryPublic |
packages/hypergraph-react/src/internal/use-query-public.tsx | Destructure/merge space from params and context |
packages/hypergraph-react/src/internal/types.ts | Add space to QueryPublicParams |
packages/hypergraph-react/src/HypergraphSpaceContext.tsx | Return default { space: '' } if context is missing |
apps/events/src/routes/playground.lazy.tsx | Pass spaceId into Playground and wrap children in provider |
apps/events/src/components/playground.tsx | Change Playground signature to accept spaceId |
Comments suppressed due to low confidence (2)
packages/hypergraph-react/src/internal/types.ts:6
- [nitpick] Consider renaming this
space
property tospaceId
to align with other APIs and clarify that it represents an identifier.
space?: string | undefined;
packages/hypergraph-react/src/use-query.tsx:149
- Add unit tests to verify that the
space
parameter is correctly forwarded touseQueryPublic
and that context fallback behaves as expected.
const publicResult = useQueryPublic(type, { enabled: mode === 'public', include, first, space });
export function useHypergraphSpaceInternal() { | ||
const context = useContext(HypergraphReactContext); | ||
return context as HypergraphContext; | ||
return (context as HypergraphContext) || { space: '' }; |
Copilot
AI
Jul 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallbacking to an empty string when context is missing can mask missing provider issues. Consider throwing an error or logging a warning to catch unintended usage early.
return (context as HypergraphContext) || { space: '' }; | |
if (!context) { | |
throw new Error('HypergraphReactContext provider is missing. Ensure that HypergraphSpaceProvider is used.'); | |
} | |
return context as HypergraphContext; |
Copilot uses AI. Check for mistakes.
// TODO: for multi-level nesting it should only allow the allowed properties instead of Record<string, Record<string, never>> | ||
include?: { [K in keyof Schema.Schema.Type<S>]?: Record<string, Record<string, never>> } | undefined; | ||
space?: string; | ||
space?: string | undefined; |
Copilot
AI
Jul 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The explicit | undefined
is redundant for an optional property; you can simplify this to space?: string
for clarity.
space?: string | undefined; | |
space?: string; |
Copilot uses AI. Check for mistakes.
No description provided.