-
Notifications
You must be signed in to change notification settings - Fork 8
connect cleanup #215
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
connect cleanup #215
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
65093d1 to
0acbe5d
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 cleans up debug logging, introduces a new schema type for the app identity response, refactors how spaces are fetched/displayed, and adds a placeholder for app info lookup.
- Removed
console.logstatements that leaked keys and debugging info - Added
AppIdentityResponseschema and type - Refactored spaces fetching into a
useSpaceshook and updated UI components - Added a dummy
getAppInfoByIdsimplementation for local app data
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/hypergraph/src/connect/types.ts | Added AppIdentityResponse schema and type |
| packages/hypergraph/src/connect/parse-callback-params.ts | Updated error messages and added console.error in catch block |
| packages/hypergraph/src/connect/create-callback-params.ts | Removed debug console.log |
| packages/hypergraph/src/connect/create-auth-url.ts | Removed debug console.log |
| apps/connect/src/routes/index.tsx | Updated page title, heading level, and CSS utility usage |
| apps/connect/src/lib/get-app-info-by-ids.ts | Added placeholder getAppInfoByIds with hardcoded data |
| apps/connect/src/hooks/use-spaces.ts | Introduced useSpaces hook to fetch and map spaces |
| apps/connect/src/components/spaces.tsx | Refactored Spaces component to use useSpaces and updated layout |
| apps/connect/src/components/create-space.tsx | Adjusted CreateSpace layout for form controls |
Comments suppressed due to low confidence (7)
packages/hypergraph/src/connect/types.ts:32
- [nitpick] Consider adding a JSDoc comment above AppIdentityResponse to explain the purpose of this schema and the meaning of its fields.
export const AppIdentityResponse = Schema.Struct({
packages/hypergraph/src/connect/types.ts:41
- [nitpick] The field name "address" is ambiguous when paired with "accountAddress"; consider renaming it to more clearly convey its purpose in this response (e.g., "callbackUrlAddress").
address: Schema.String,
apps/connect/src/routes/index.tsx:17
- The Tailwind class "max-w-(--breakpoint-sm)" appears invalid; for an arbitrary value using a CSS variable, use syntax like "max-w-[var(--breakpoint-sm)]".
<div className="flex flex-col gap-4 max-w-(--breakpoint-sm) mx-auto py-8">
apps/connect/src/hooks/use-spaces.ts:1
- The new useSpaces hook contains significant logic and external dependencies but has no accompanying tests; consider adding unit or integration tests to cover data fetching and mapping behavior.
import { getAppInfoByIds } from '@/lib/get-app-info-by-ids';
packages/hypergraph/src/connect/parse-callback-params.ts:64
- [nitpick] Consider using a centralized logging utility or error handler instead of console.error to ensure consistent logging across the codebase.
console.error(error);
apps/connect/src/routes/index.tsx:18
- [nitpick] Changing to an h1 here may disrupt the document’s heading hierarchy; ensure this is the top-level heading or adjust back to h2 if nested under a higher-level heading.
<h1 className="text-lg font-bold mb-4">Welcome to Geo Connect</h1>
apps/connect/src/components/create-space.tsx:96
- [nitpick] Static text is shown via a span without a corresponding label element; consider using a associated with the input for better accessibility.
<span className="text-xs text-gray-500">Create a new space</span>
| if (Either.isLeft(decoded)) { | ||
| return Effect.fail(new FailedToParseAuthCallbackUrl({ message: 'Failed to parse connect auth payload' })); | ||
| return Effect.fail( | ||
| new FailedToParseAuthCallbackUrl({ message: 'Failed to parse connect auth callback payload' }), |
Copilot
AI
Jun 13, 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.
Error messages differ between branches; unify the message text to maintain consistency and avoid confusion.
| export const getAppInfoByIds = async (appIds: string[]) => { | ||
| // sleep for 1 second | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| return apps; |
Copilot
AI
Jun 13, 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.
getAppInfoByIds currently returns the entire apps object regardless of the appIds parameter; filter the output to include only requested IDs or document the intended behavior.
| return apps; | |
| const filteredApps = Object.fromEntries( | |
| Object.entries(apps).filter(([id]) => appIds.includes(id)) | |
| ); | |
| return filteredApps; |
| ...space, | ||
| apps: Array.from(spaceAppIds).map((appId) => { | ||
| return { | ||
| // @ts-expect-error - need to improve appInfo typing |
Copilot
AI
Jun 13, 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.
Suppressing TypeScript errors with @ts-expect-error can hide real issues; consider refining the appInfo type definition to avoid the need for this comment.
No description provided.