feat: GraphQL filtering, sorting, search & pagination#34
feat: GraphQL filtering, sorting, search & pagination#34
Conversation
… pagination (hyperindex-q00.2)
- Add DIDFilterInput GraphQL type with only eq and in fields (no contains/startsWith/neq) - Replace StringFilterInput with DIDFilterInput for the did field in WhereInput - Introduce DIDFilter struct in repositories to carry eq and in conditions - Update extractFilters to populate DIDFilter.EQ and DIDFilter.IN from GraphQL args - Add buildDIDFilterClause helper to generate SQL WHERE conditions for DIDFilter - Update all repository methods to accept DIDFilter instead of plain string - Add tests for DIDFilterInput fields, extractFilters DID handling, and DID in filtering
…yConnection (hyperindex-q00.11)
…ogo (hyperindex-0nk.7)
…oter (hyperindex-0nk.4)
…g, theme toggle (hyperindex-0nk.5)
- Replace hardcoded hypergoat-app-production URL with NEXT_PUBLIC_API_URL env var - Point GraphiQL links directly to backend (Next.js rewrite can't proxy HTML) - Remove broken /graphiql rewrite from next.config.ts
…_API_URL - Add /graphiql server-side redirect route (reads HYPERINDEX_URL at runtime) - Add ARG NEXT_PUBLIC_API_URL to Dockerfile so docs page gets correct URL at build time - Revert dashboard/header GraphiQL links to /graphiql (handled by redirect route)
Documents the exact railway up commands, env vars, custom domains, and common troubleshooting for deploying both services.
…NSIDs (hyperindex-2rz)
…red fields with missing data
…h (hyperindex-3gm)
feat: Replace Jetstream+Backfill with Tap sidecar
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
client/src/components/layout/Header.tsx (1)
228-236:⚠️ Potential issue | 🟡 MinorLogin modal lacks keyboard dismissal (Escape key).
The modal can only be dismissed by clicking the backdrop. Adding an
onKeyDownhandler for Escape improves accessibility and matches standard modal UX expectations.Proposed fix
Add an
onKeyDownhandler to the modal wrapper:<div className="fixed inset-0 z-50 flex items-start justify-center pt-[20vh]" onClick={() => setShowLoginModal(false)} + onKeyDown={(e) => { if (e.key === 'Escape') setShowLoginModal(false) }} >You may also want to add
tabIndex={-1}androle="dialog"to the outer<div>and auto-focus it so the keydown fires without requiring a click first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/layout/Header.tsx` around lines 228 - 236, The login modal currently only closes on backdrop click; add keyboard dismissal by attaching an onKeyDown handler to the modal wrapper that calls setShowLoginModal(false) when the Escape key is pressed, and make the wrapper focusable (set tabIndex={-1}), add role="dialog" and autoFocus (or programmatically focus it when showLoginModal becomes true) so the keydown fires without a prior click; keep the inner onClick={(e) => e.stopPropagation()} to prevent clicks from closing the modal.internal/graphql/admin/schema.go (1)
120-139:⚠️ Potential issue | 🟠 Major
recentActivityexposes sensitive internal data to unauthenticated users.The endpoint uses
OptionalAuth()(HTTP-level auth is optional, not enforced), and therecentActivityresolver has norequireAdmincheck. ActivityEntryType includes:
errorMessage— database errors, parse failures, internal state (lines 293–296 in types.go)eventJson— raw jetstream event payload (lines 297–300)status— internal processing pipeline state (lines 289–292)The resolver returns all these fields unconditionally (resolvers.go:656–680). Without authentication, any internet user can query up to 1000 entries per request across 7 days of raw event data with error details.
While
statisticsandactivityBucketsare safe aggregated responses,recentActivityreturns raw entries with internal details. Either addrequireAdmin(p.Context)to the resolver (matchinglabelDefinitionsandlabels), or filtererrorMessageandeventJsonfrom the public response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/graphql/admin/schema.go` around lines 120 - 139, The recentActivity GraphQL field currently returns raw ActivityEntryType data (including errorMessage and eventJson) to unauthenticated users; update the resolver for "recentActivity" to enforce admin-only access by calling requireAdmin(p.Context) at the start of the Resolve function (same pattern used by "labelDefinitions" and "labels"), or alternatively sanitize the output of b.resolver.RecentActivity by stripping/clearing sensitive fields (errorMessage, eventJson, and internal status) before returning; update either the Resolve closure for recentActivity or modify b.resolver.RecentActivity to perform the check/sanitization so public requests cannot retrieve internal error details.client/src/app/api/admin/graphql/route.ts (1)
24-24:⚠️ Potential issue | 🟡 MinorLogging the session DID on every request may violate GDPR/CCPA compliance in production.
DIDs are user identifiers and their emission to stdout/log streams on every authenticated admin call constitutes PII retention that should be scoped (e.g., restricted to a debug flag or replaced with a non-identifying request ID).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/api/admin/graphql/route.ts` at line 24, The console.log call that prints "[admin-graphql] Authenticated request" with session.did in client/src/app/api/admin/graphql/route.ts must not emit raw DIDs in production; either remove the session.did from logs or gate it behind a debug flag (e.g., only log when a DEBUG/LOG_LEVEL env var is enabled or NODE_ENV !== 'production'), or replace it with a non-identifying value (redacted DID or a generated request ID). Locate the console.log statement and change it to log only safe metadata (request ID or redacted identifier) or wrap it with an environment/debug check so PII is never written to stdout in production.README.md (2)
15-21:⚠️ Potential issue | 🟡 MinorStale
hypergoatreferences in Quick Start and Configuration.The project was renamed to Hyperindex, but a few references still use the old name:
- Line 17:
git clone https://github.com/GainForest/hypergoat.git- Line 20:
go run ./cmd/hypergoat- Line 232:
DATABASE_URL=sqlite:data/hypergoat.dbThese should be updated to reflect the new name if the repo/binary has been renamed, or at minimum a note should clarify that the repo URL still uses the old name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 15 - 21, Update the stale "hypergoat" references in the README: replace the git clone URL "https://github.com/GainForest/hypergoat.git" with the current repo URL if renamed (or add a clarifying note that the repo URL still uses the old name), change the run command "go run ./cmd/hypergoat" to the new binary or package path (e.g., "./cmd/hyperindex" or the current entrypoint), and update the DATABASE_URL example "sqlite:data/hypergoat.db" to use "hyperindex.db" (or annotate that the filename remains hypergoat.db if unchanged); ensure all occurrences of the strings "hypergoat" in the Quick Start and Configuration sections are consistently updated or explicitly annotated.
296-310:⚠️ Potential issue | 🟡 MinorArchitecture diagram doesn't reflect Tap (the recommended path).
The architecture diagram only shows the Jetstream + Backfill flow. Since Tap is now the recommended ingestion method (documented above), the diagram should include the Tap data path (e.g.,
Tap Sidecar → Consumer → Records DB → GraphQL API).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 296 - 310, Update the architecture ASCII diagram to include the Tap ingestion path alongside the existing Jetstream and Backfill flows: add a line showing "Tap Sidecar → Consumer → Records DB → GraphQL API" (or integrate Tap Sidecar feeding the same Consumer node) and ensure labels like "Tap Sidecar", "Consumer", "Records DB", and "GraphQL API" match the README's documented terminology; keep the layout consistent with the current box diagram so Tap appears as the recommended path.client/src/app/backfill/page.tsx (1)
108-116:⚠️ Potential issue | 🟡 MinorHardcoded light-mode status colors will break in dark mode.
The "Backfill in progress" block (lines 108–116) uses hardcoded light-theme Tailwind classes like
bg-blue-50/50,border-blue-200/60,text-blue-700, andtext-blue-600/70. These will clash with the dark theme — light backgrounds and dark text on a dark page.The same issue affects the amber warning block at lines 173–184 (
bg-amber-50/50,text-amber-700, etc.).Consider adding dark-mode variants or using CSS variables for these semantic status colors to match the theming approach used elsewhere on this page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/backfill/page.tsx` around lines 108 - 116, The status panels use hardcoded light-theme Tailwind classes (e.g., bg-blue-50/50, border-blue-200/60, text-blue-700, text-blue-600/70 and similarly bg-amber-50/50, text-amber-700) which will look wrong in dark mode; update the markup in client/src/app/backfill/page.tsx (the "Backfill in progress" block and the amber warning block) to use dark-mode aware classes or shared semantic tokens instead — for example add dark: variants (dark:bg-blue-900/40, dark:border-blue-700/60, dark:text-blue-200, etc.) or replace the hardcoded color classes with CSS variable classes used elsewhere on the page so the panels adapt to theme changes while preserving the same class structure (target the div with the status container and the nested text divs to apply these changes).client/src/app/globals.css (1)
161-166:⚠️ Potential issue | 🟠 MajorBlanket
outline: noneon focus is an accessibility risk.Removing the outline on all
input,select, andtextareafocus states without a guaranteed visible replacement breaks keyboard navigation. Components that addfocus:ring-2are fine, but any form element that doesn't will be invisible to keyboard users.Consider scoping this reset to only elements that have an explicit focus-visible style, or provide a baseline
box-shadow/ringfallback here:Proposed fix — provide a fallback focus indicator
input:focus, select:focus, textarea:focus { outline: none; + box-shadow: 0 0 0 2px var(--ring); }Or scope more narrowly with
focus-visibleto preserve mouse UX while ensuring keyboard accessibility:-input:focus, -select:focus, -textarea:focus { - outline: none; -} +input:focus-visible, +select:focus-visible, +textarea:focus-visible { + outline: 2px solid var(--ring); + outline-offset: 2px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/globals.css` around lines 161 - 166, The current blanket rule removing outlines on input:focus, select:focus, and textarea:focus is an accessibility risk; update the CSS to stop removing focus for keyboard users by scoping the reset to only non-keyboard interactions and providing a visible fallback: replace the blanket selectors with a rule that targets :focus:not(:focus-visible) to suppress outline only for mouse focus, and add a baseline focus-visible indicator (e.g., a box-shadow/ring or outline) for input:focus-visible, select:focus-visible, textarea:focus-visible so all form controls without their own focus styles still show a visible focus state; locate and edit the selectors input:focus, select:focus, textarea:focus in globals.css and implement these two rules.client/src/app/settings/page.tsx (1)
48-56:⚠️ Potential issue | 🟡 MinorPre-existing bug:
useStateused as a side-effect hook.This isn't part of the current diff, but
useState(() => { ... })on line 48 is being used as a side-effect runner (callingsetDomainAuthority, etc.) instead ofuseEffect. The lazy initializer is expected to return the initial state value. This works incidentally but will double-fire in React Strict Mode and is semantically incorrect — should beuseEffect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/settings/page.tsx` around lines 48 - 56, The block currently uses useState(() => { ... }) as a side-effect to call setDomainAuthority, setRelayUrl, setPlcDirectoryUrl, setJetstreamUrl and setOauthScopes when settings exists; replace that with useEffect(() => { if (settings) { ... } }, [settings]) so the initializer no longer misuses useState's lazy initializer and the setters run only as a side-effect when settings changes (keep the same setX calls and dependency on settings).cmd/hypergoat/main.go (1)
277-303:⚠️ Potential issue | 🟡 MinorHealth status string inconsistency:
"ok"vs"healthy".When Tap is enabled and healthy, status is
"ok"(line 292). When Tap is disabled, status is"healthy"(line 300). Monitoring tools that alert on a specific status string will break depending on Tap configuration. Unify to a single value.Additionally, line 286 exposes
err.Error()from the Tap health check, which could leak internal infrastructure details (hostnames, ports). Consider a generic message for the public response.Proposed fix
if err := bg.tapAdminClient.Health(req.Context()); err != nil { + slog.Warn("Tap health check failed", "error", err) _ = json.NewEncoder(w).Encode(map[string]any{ "status": "degraded", "tap": "unreachable", - "error": err.Error(), "time": time.Now().UTC().Format(time.RFC3339), }) return } _ = json.NewEncoder(w).Encode(map[string]any{ - "status": "ok", + "status": "healthy", "tap": "ok", "time": time.Now().UTC().Format(time.RFC3339), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/hypergoat/main.go` around lines 277 - 303, The health endpoint returns inconsistent status strings and exposes internal error details: update the handler registered at r.Get("/health", ...) so the top-level "status" value is unified (use "healthy" for both the Tap-enabled healthy branch and the Tap-disabled branch) instead of "ok", and replace the direct err.Error() in the degraded response from bg.tapAdminClient.Health(req.Context()) with a generic public message (e.g., "tap unavailable" or "unreachable") while logging the actual error server-side; preserve the "tap" field and timestamp format and ensure any actual error is sent only to the server log, not the JSON response.
🧹 Nitpick comments (32)
client/src/components/dashboard/StatsCards.tsx (1)
22-33: Hardcodedoklchvalues are not theme-aware and lack a fallback.Two related concerns:
Theme-adaptability: Every other color in this component flows through a CSS variable (
--muted,--muted-foreground,--border), which means a ThemeProvider or dark-mode class can update them at runtime. The threeoklch(…)literals on lines 22, 27, and 32 are baked in and will stay fixed regardless of the active theme. If these values need to adapt to a dark theme, they should instead reference CSS variables (e.g.,--color-records,--color-actors,--color-lexicons) whose values are defined per-theme.Browser fallback:
oklch()is supported in Chrome 111+, Firefox 113+, Safari 15.4+, and Edge 111+. While modern browsers cover this,oklch()colors are not supported in all browsers; the recommended pattern is to provide a hex/rgb fallback first, e.g.color:#7d2429; color: oklch(40.1% 0.123 21.57);. Since these are hardcoded inlinestyleobjects in JS, a CSS cascade fallback cannot be added — the only option would be a@supportscheck or moving the values to a stylesheet. If the project's browser support matrix already excludes pre-111 Chrome/Edge and pre-15.4 Safari, this can be waived.♻️ Suggested approach: CSS-variable-driven colors
Define in your global CSS / ThemeProvider:
/* :root (light) */ --color-records: oklch(0.65 0.15 155); --color-actors: oklch(0.55 0.15 250); --color-lexicons: oklch(0.55 0.15 310); /* .dark */ .dark { --color-records: oklch(0.70 0.13 155); /* ... adjusted for dark bg */ }Then in the component:
- colorStyle: { color: "oklch(0.65 0.15 155)" }, + colorStyle: { color: "var(--color-records)" }, ... - colorStyle: { color: "oklch(0.55 0.15 250)" }, + colorStyle: { color: "var(--color-actors)" }, ... - colorStyle: { color: "oklch(0.55 0.15 310)" }, + colorStyle: { color: "var(--color-lexicons)" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/dashboard/StatsCards.tsx` around lines 22 - 33, The three hardcoded oklch colors in the StatsCards component (the colorStyle entries for the items named "Records", "Actors", and "Lexicons") should be replaced with theme-aware CSS variables and given a fallback strategy: define --color-records, --color-actors, --color-lexicons in your global/theme CSS (and override them in dark mode), then update the colorStyle values in StatsCards to reference those variables (e.g., use "var(--color-records)") instead of literal oklch() strings; if you must support older browsers keep the color declarations in a stylesheet and use a `@supports`(octothorpe) / `@supports`(color: oklch(...)) or provide a hex/rgb fallback in the stylesheet so browsers that don’t support oklch get a fallback color.client/src/components/dashboard/RecentActivity.tsx (1)
70-86: Hardcoded OKLCH badge text colors may be unreadable in dark modeThe badge text uses mid-to-dark OKLCH lightness values (
0.50–0.60), calibrated for a light card background. The surrounding component already uses CSS variables (--card,--foreground,--muted-foreground) to handle theme switching. If--cardresolves to a dark surface in dark mode, these fixed-lightness text colors will have poor contrast.Consider moving these to CSS custom properties (e.g.,
--color-op-create,--color-op-update,--color-op-delete) so they participate in the same theming system:♻️ Sketch of a CSS-variable-based approach
In the global stylesheet, declare pairs for light/dark:
:root { --color-op-create-bg: oklch(0.65 0.15 155 / 0.1); --color-op-create: oklch(0.55 0.15 155); --color-op-update-bg: oklch(0.60 0.15 250 / 0.1); --color-op-update: oklch(0.50 0.15 250); --color-op-delete-bg: oklch(0.75 0.15 75 / 0.1); --color-op-delete: oklch(0.60 0.15 75); } .dark { --color-op-create: oklch(0.75 0.15 155); /* lighter for dark bg */ /* … */ }Then in the component:
- ? { - backgroundColor: "oklch(0.65 0.15 155 / 0.1)", - color: "oklch(0.55 0.15 155)", - } + ? { + backgroundColor: "var(--color-op-create-bg)", + color: "var(--color-op-create)", + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/dashboard/RecentActivity.tsx` around lines 70 - 86, RecentActivity.tsx currently uses hardcoded OKLCH colors in the inline style block keyed by entry.operation which will break contrast in dark mode; replace those literal OKLCH values with CSS custom properties (for example --color-op-create, --color-op-create-bg, --color-op-update, --color-op-update-bg, --color-op-delete, --color-op-delete-bg) and reference those variables in the component's style (or via utility classes) instead of the fixed oklch strings, then declare light/dark fallbacks for those properties in the global stylesheet (e.g., :root and .dark) so create/update/delete badges adapt to theme while keeping the same keys noted in RecentActivity.tsx and still driven by entry.operation.client/src/app/onboarding/page.tsx (1)
363-364:var(--secondary-foreground)is inconsistent withvar(--muted-foreground)used elsewhere.Every other descriptive label in this component uses
var(--muted-foreground). The "Configuration Summary" heading at line 364 usesvar(--secondary-foreground), which maps to a different token value and may produce an unexpected contrast depending on the theme definition. If the intent is a slightly more prominent header, this is fine — but if it's a copy-paste slip it could look off in non-default themes.🎨 Align with the rest of the component if unintentional
- <h4 className="text-sm font-medium mb-3" style={{ color: "var(--secondary-foreground)" }}>Configuration Summary</h4> + <h4 className="text-sm font-medium mb-3" style={{ color: "var(--foreground)" }}>Configuration Summary</h4>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/onboarding/page.tsx` around lines 363 - 364, The "Configuration Summary" heading uses the CSS var var(--secondary-foreground) which is inconsistent with the rest of this component that uses var(--muted-foreground); update the h4 that renders "Configuration Summary" (the h4 inside the rounded-lg p-4 container) to use var(--muted-foreground) so it matches other descriptive labels, unless you intentionally want a stronger emphasis—in that case confirm intent or add a comment.internal/lexicon/types_test.go (1)
14-15: Duplicate test case: "string datetime format" is identical to "string no format".
ZeroValueForTypeonly takespropTypeand doesn't accept a format parameter. Both cases pass"string"and expect"", so the second case doesn't exercise any additional code path. Either remove it or, if the intent was to test format-awareness, note that the function doesn't currently support it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/lexicon/types_test.go` around lines 14 - 15, The test suite contains a duplicate case—both tests call ZeroValueForType with propType "string" and expect "", so remove the redundant "string datetime format" case or replace it with a meaningful case; if you intended to test format handling, update the test to reflect that ZeroValueForType does not accept a format parameter (or extend ZeroValueForType to accept a format and add a new case), referencing the ZeroValueForType function and the test table entries named "string no format" and "string datetime format" to locate and fix the duplicated test row.client/package.json (1)
26-26:next-themesis out of alphabetical order in dependencies.It's placed after
rechartsbut should appear betweennextandreactto maintain conventional alphabetical ordering.Proposed fix
Move
"next-themes": "^0.4.6"to after line 22 ("next": "16.1.5",):"next": "16.1.5", + "next-themes": "^0.4.6", "react": "19.2.3", ... "recharts": "^3.7.0", - "next-themes": "^0.4.6", "tailwind-merge": "^3.4.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/package.json` at line 26, The dependency "next-themes" is misplaced in package.json dependencies order; move the "next-themes": "^0.4.6" entry so it appears alphabetically between the "next": "16.1.5" and "react" entries (i.e., after the "next" line and before "react"), ensuring the dependencies block maintains conventional alphabetical ordering and preserving commas/JSON validity around the moved entry.client/src/components/layout/Header.tsx (1)
278-278: Hardcodedtext-red-500breaks the CSS variable theming pattern.Every other color in this component uses
var(--...)tokens. Consider using a themed error color variable for consistency, or at minimum a CSS variable likevar(--destructive)if one exists in your design tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/layout/Header.tsx` at line 278, The error message paragraph in Header.tsx currently hardcodes Tailwind color "text-red-500" which breaks the component's CSS variable theming; replace that class with the themed error color variable (e.g., use a token like text-[color:var(--destructive)] or the existing utility/class that maps to var(--destructive)) so the <p> element that renders {error} follows the same var(--...) pattern as other colors in the component.internal/server/handlers_test.go (1)
351-361: PreferbaseCfg.Titleover the hard-coded string in the assertion.Line 359 duplicates the literal from line 318. If
Titleis updated again, both lines must be kept in sync manually.♻️ Suggested change
- if !strings.Contains(body, "Hyperindex GraphiQL") { + if !strings.Contains(body, baseCfg.Title) { t.Error("response body does not contain title") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/handlers_test.go` around lines 351 - 361, The test "body contains title" currently asserts that the response contains the hard-coded string "Hyperindex GraphiQL"; change that assertion to use baseCfg.Title instead so it tracks the configured title. Locate the test block that calls HandleGraphiQL(baseCfg) and replace the strings.Contains check (and the error message if present) to reference baseCfg.Title rather than the literal, ensuring the test will continue to pass if baseCfg.Title changes.client/src/components/layout/GeometricBackground.tsx (1)
47-48: Consider replacing<img>with Next.js<Image>for optimization.The
eslint-disable-next-linesuppresses the lint rule, butnext/imagewould provide automatic lazy-loading, format negotiation, and CDN caching for/hypercerts_logo.png. Since the dimensions are fixed (40×40) and the parent ispointer-events-none, a<Image>component would work without layout side-effects.♻️ Suggested replacement
+import Image from "next/image" ... - {/* eslint-disable-next-line `@next/next/no-img-element` */} - <img src="/hypercerts_logo.png" alt="" width={40} height={40} className="opacity-40" /> + <Image src="/hypercerts_logo.png" alt="" width={40} height={40} className="opacity-40" unoptimized={false} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/layout/GeometricBackground.tsx` around lines 47 - 48, In GeometricBackground.tsx replace the raw <img> usage with Next.js Image: add "import Image from 'next/image'", remove the eslint-disable comment, and render <Image src="/hypercerts_logo.png" alt="Hypercerts logo" width={40} height={40} className="opacity-40 pointer-events-none" /> (or keep pointer-events-none on the parent) so you get automatic lazy-loading and optimization without layout side-effects; ensure the src, width, height and className match the original values and that the component name GeometricBackground uses the new Image import.client/Dockerfile (1)
36-38:publicdir copied without--chown; inconsistent with lines 37–38.The
publicdirectory will be owned byroot:rootwhile.next/standaloneand.next/staticare owned bynextjs:nodejs. This won't cause a runtime failure (world-readable permissions suffice for reads), but it is inconsistent and could matter if Next.js ever writes cache files adjacent topublic.✨ Proposed fix
-COPY --from=builder /app/public ./public +COPY --from=builder --chown=nextjs:nodejs /app/public ./publicNote on Trivy DS-0002: The warning is a false positive —
USER nextjsis present at line 40. The builder stage running as root is expected and its layers are discarded in the multi-stage build.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/Dockerfile` around lines 36 - 38, The COPY of the public directory is missing the --chown flag and will be owned by root:root while later COPYs set nextjs:nodejs; update the COPY that copies /app/public to use the same ownership as the other lines (add --chown=nextjs:nodejs to the COPY that references /app/public) so ownership is consistent with the .next/standalone and .next/static COPY commands.internal/tap/event_test.go (1)
276-351: Acceptance-criteria tests duplicate earlier tests.
TestParseEvent_AcceptanceCriteria_RecordCreate(line 276) largely duplicatesTestParseEvent_RecordCreate(line 8), andTestRecordEvent_URI_AcceptanceCriteria(line 340) is a subset ofTestRecordEvent_URI(line 155). Consider merging these into the existing table-driven tests as named cases to reduce maintenance surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/event_test.go` around lines 276 - 351, These tests duplicate existing cases; remove the two acceptance-criteria tests TestParseEvent_AcceptanceCriteria_RecordCreate and TestRecordEvent_URI_AcceptanceCriteria and fold their scenarios into the existing table-driven tests (add them as named cases in TestParseEvent_RecordCreate's table and TestRecordEvent_URI's table) so the same inputs/expectations are covered without duplicate functions; update the table entries to include the exact JSON input and expected fields used in the acceptance tests (matching event ID/type/DID/collection/action and the RecordEvent URI case) and remove the duplicate test functions.internal/config/config.go (1)
146-157: Consider validating Tap config inValidate().When
TapEnabledis true, it may be worth validating thatTapURLis non-empty (and optionally that it has aws://orwss://scheme). Currently, a misconfiguredTapURLwould only surface at connection time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 146 - 157, When TapEnabled is true, Validate should ensure TapURL is present and well-formed: update Config.Validate to check c.TapEnabled and return an error if c.TapURL is empty, and optionally parse c.TapURL (using net/url.Parse) to verify the scheme is "ws" or "wss" (returning a descriptive fmt.Errorf on failure); reference the Config.Validate method and the TapEnabled/TapURL fields when adding these checks so misconfiguration is caught at startup rather than at connection time.internal/config/config_test.go (2)
413-417: Redundant assertion — password non-emptiness is already covered on line 403.The
passwordSetcheck at lines 414–417 duplicates the assertion at line 403 (cfg.TapAdminPassword != "mypassword"). The comment says it tests thetap_admin_password_setlogging pattern, but that pattern lives inLogConfig, which isn't exercised here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 413 - 417, The test contains a redundant assertion: the password non-emptiness check using passwordSet (variable) and cfg.TapAdminPassword duplicates the earlier assertion cfg.TapAdminPassword != "mypassword"; remove the redundant passwordSet block (lines creating passwordSet and t.Error) or replace it with a targeted assertion that exercises the LogConfig behavior — e.g., call the LogConfig/logging function that emits the tap_admin_password_set pattern and assert that output — but do not keep the duplicate cfg.TapAdminPassword non-empty check; refer to TapAdminPassword, passwordSet and LogConfig when making the change.
332-418: New TAP tests should use table-driven pattern.The three new test functions (
TestTapConfigDefaults,TestTapConfigEnvVars,TestTapConfigFields) use sequential inline assertions instead of the table-driven test pattern with struct slices. The existing tests in this file (e.g.,TestGetEnv,TestGetEnvBool) already demonstrate the expected pattern. Consider consolidating the TAP default and env-override cases into a single table-driven test.Additionally, prefer
t.Setenvover manualos.Setenv/os.Unsetenv— it auto-restores on test cleanup and avoids env leakage between parallel tests.As per coding guidelines: "Use table-driven tests with struct slices for testing (name, input, want, wantErr fields)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 332 - 418, Replace the three sequential TAP tests (TestTapConfigDefaults, TestTapConfigEnvVars, TestTapConfigFields) with a single table-driven test that iterates over cases (name, env map, expected values for TapURL, TapAdminPassword, TapDisableAcks, TapEnabled) and asserts against getEnv/getEnvBool or a constructed Config; consolidate default and env-override scenarios into the cases and use t.Setenv for each env entry so the environment is automatically restored. Locate the logic in the existing TestTapConfig* functions and reference getEnv, getEnvBool and the Config fields (TapURL, TapAdminPassword, TapDisableAcks, TapEnabled) when building each case and assertions. Ensure you cover both default (no env set) and overridden (env set) expectations in the table and remove manual os.Setenv/os.Unsetenv usage. Keep each case name descriptive and run subtests with t.Run to maintain clarity and parallel safety.docker-compose.tap.yml (2)
1-1:versionkey is obsolete in Docker Compose v2+.The
versionproperty is ignored by modern Docker Compose and produces a warning. It can be safely removed.♻️ Suggested fix
-version: "3.8" - services:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.tap.yml` at line 1, Remove the obsolete top-level "version" key from docker-compose.tap.yml; locate the "version: \"3.8\"" entry and delete that line so the compose file uses the modern v2+ format (keep all other top-level service, networks and volumes sections unchanged).
30-30: Default database filename still references old "hypergoat" branding.
DATABASE_URLdefaults tosqlite:data/hypergoat.db, but the service has been rebranded to "hyperindex". This could be confusing for new deployments.♻️ Suggested fix
- DATABASE_URL: "${DATABASE_URL:-sqlite:data/hypergoat.db}" + DATABASE_URL: "${DATABASE_URL:-sqlite:data/hyperindex.db}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.tap.yml` at line 30, The default DATABASE_URL value still references the old filename "sqlite:data/hypergoat.db"; update the default to the new branding by changing the DATABASE_URL default value to "sqlite:data/hyperindex.db" (modify the DATABASE_URL environment entry in the docker-compose configuration so the fallback uses hyperindex.db instead of hypergoat.db).client/src/components/ui/Alert.tsx (2)
16-21: oklch colors are hardcoded — consider using CSS variables for theme consistency.The other components in this PR (e.g.,
Button.tsx) use CSS custom properties likevar(--primary)for theming. Hardcoding oklch values here means the Alert component won't respond to theme changes and diverges from the project's CSS variable theming pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/ui/Alert.tsx` around lines 16 - 21, The variantStyles constant currently hardcodes oklch color strings; replace those hardcoded values with CSS custom properties (e.g., use var(--alert-info-bg), var(--alert-info-color), var(--alert-info-border) for the info variant and analogous vars for success/warning/error) inside variantStyles so the Alert component picks up theme changes; keep the original oklch values only as fallbacks in the var(...) calls or define those variables in the global CSS/variables file to preserve current appearance. Reference: modify the variantStyles object in Alert.tsx (and ensure corresponding CSS variables are added/updated in your theme or root stylesheet so Alert inherits project-wide theming).
9-14:variantsmap is now redundant — every entry is"border".Since all variants map to the same
"border"string, this map no longer differentiates anything. You could remove it and add"border"directly in thecn()call on line 30 (it's already there via the static classes).♻️ Simplify by removing the redundant map
-const variants = { - info: "border", - success: "border", - warning: "border", - error: "border", -}; - ... className={cn( - "flex items-start gap-3 rounded-lg border p-4", - variants[variant], + "flex items-start gap-3 rounded-lg border p-4", className )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/ui/Alert.tsx` around lines 9 - 14, The variants map in Alert.tsx (const variants) is redundant because every key maps to "border"; remove the entire variants object and any usage of it, and ensure the cn(...) call inside the Alert component (the call that previously referenced variants) includes the "border" class directly (or keeps the existing static "border" already present) so the styling remains unchanged; update any references to the removed symbol `variants` to use the literal "border" in the `cn` invocation.client/src/lib/graphql/client.ts (1)
40-52: Cached client may serve stale URL if environment changes between calls.The lazy singleton
_publicGraphqlClientcaptures the URL on first invocation and never re-evaluates. This is fine for typical server processes but worth noting: if a test or hot-reload scenario changesprocess.env.HYPERINDEX_URLafter the first call, the client will still point to the old URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/lib/graphql/client.ts` around lines 40 - 52, The lazy singleton _publicGraphqlClient inside publicGraphqlClient.request captures the URL on first use and can become stale if environment values (e.g., process.env.HYPERINDEX_URL) or getBaseUrl()/getHyperindexUrl() change; update the logic to re-resolve the URL when environment changes by either invalidating or recreating _publicGraphqlClient when getBaseUrl()/getHyperindexUrl() would return a different value, or compute the URL per-request and create a short-lived GraphQLClient for SSR/test scenarios; locate the code in publicGraphqlClient.request that constructs new GraphQLClient and modify it to compare current URL to the cached client's base URL (or attach the resolved URL to the cached client) and rebuild the GraphQLClient when they differ so the client always uses the current environment URL.client/src/components/ui/Button.tsx (1)
29-29: Hardcoded#fffin destructive variant — use a CSS variable for consistency.All other variants reference CSS custom properties (
var(--primary-foreground),var(--foreground)), butdestructivehardcodes"#fff". This won't adapt to theme changes (e.g., dark mode with a different destructive text color).♻️ Suggested fix
- destructive: { backgroundColor: "var(--destructive)", color: "#fff" }, + destructive: { backgroundColor: "var(--destructive)", color: "var(--destructive-foreground)" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/ui/Button.tsx` at line 29, The destructive variant in the exported styles object (the destructive property in Button.tsx) hardcodes "#fff"; replace that literal with a CSS custom property so the text color follows theme changes—use a variable such as var(--destructive-foreground) and provide a sensible fallback (for example var(--foreground) or var(--primary-foreground)) to mirror how other variants use var(--primary-foreground)/var(--foreground).internal/tap/handler_test.go (2)
1-11: Missing package doc comment.As per coding guidelines, "Every package must have a doc comment explaining its purpose." While this is a test package, adding a brief comment above the
packagedeclaration is recommended:// Package tap_test contains integration tests for the TAP IndexHandler. package tap_testAs per coding guidelines,
**/*.go: "Every package must have a doc comment explaining its purpose."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/handler_test.go` around lines 1 - 11, Add a package doc comment above the package declaration for the test file: insert a brief comment like "Package tap_test contains tests for the TAP IndexHandler" immediately before the line declaring package tap_test in internal/tap/handler_test.go so the package has the required doc comment per project guidelines.
22-79: Consider table-driven tests for the CRUD lifecycle tests.The coding guidelines specify table-driven tests with struct slices. The Create/Update/Delete tests share significant setup boilerplate. While the different assertions per action make a single table tricky, the pattern could still reduce duplication — e.g., parameterizing the action, expected event type, and post-condition check.
This is optional given the tests are already clear and each exercises distinct side-effects.
As per coding guidelines,
**/*_test.go: "Use table-driven tests with struct slices for testing (name, input, want, wantErr fields)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/handler_test.go` around lines 22 - 79, TestIndexHandler_HandleRecord_Create and the related CRUD tests should be converted to a table-driven style to satisfy the test guideline: create a slice of cases (name, event, wantEventType, wantErr, postCheck func) and iterate over them, calling setupHandler(t) per case, invoking handler.HandleRecord(ctx, tc.event), then running the case-specific assertions (e.g., using db.Records.GetByURI and db.Actors.GetByDID to verify state and reading from pubsub.Subscribe/sub.Events to verify published events). Parameterize the differing expectations (Action/ACTION mapping to subscription.EventCreate/Update/Delete, expected URI/CID, and actor assertions) into the case fields so the boilerplate setupHandler, context, subscription, and common checks are shared while each case provides its own postCheck closure for action-specific assertions.client/src/components/ui/Input.tsx (1)
29-45: Inconsistent error color: hardcodedtext-red-500vsvar(--destructive).Line 35 uses
var(--destructive)for the error border, but line 45 uses the hardcoded Tailwind classtext-red-500for error text. Use the CSS variable for both to stay consistent with the theme system.Additionally,
focus:ring-2on line 29 no longer has an explicit ring color class. Consider addingringColor: "var(--ring)"to the inline styles so the focus ring respects the theme.Proposed fix
style={{ backgroundColor: "var(--card)", borderColor: error ? "var(--destructive)" : "var(--input)", color: "var(--foreground)", + outlineColor: "var(--ring)", ...style, }}- <p className="text-sm text-red-500">{error}</p> + <p className="text-sm" style={{ color: "var(--destructive)" }}>{error}</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/ui/Input.tsx` around lines 29 - 45, The Input component renders error text using a hardcoded Tailwind class ("text-red-500") while its border uses the CSS variable var(--destructive); update the error text to use the same theme variable by applying an inline style color: "var(--destructive)" instead of the hardcoded class on the element that renders {error} (the <p> for error), and add ringColor: "var(--ring)" to the inline style of the input element (the JSX input in this component) so focus:ring-2 uses the theme ring color consistently.client/src/app/globals.css (1)
99-115: Keyframe names should be kebab-case per Stylelint.Stylelint reports
fadeInUpandfadeInshould use kebab-case naming. Rename tofade-in-upandfade-in, and update the corresponding animation utility classes.Proposed fix
-@keyframes fadeInUp { +@keyframes fade-in-up { from { opacity: 0; transform: translateY(12px); } to { opacity: 1; transform: translateY(0); } } -@keyframes fadeIn { +@keyframes fade-in { from { opacity: 0; } to { opacity: 1; } } .animate-fade-in-up { - animation: fadeInUp 0.5s ease-out both; + animation: fade-in-up 0.5s ease-out both; } .animate-fade-in { - animation: fadeIn 0.4s ease-out both; + animation: fade-in 0.4s ease-out both; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/globals.css` around lines 99 - 115, Stylelint wants keyframe names in kebab-case: rename the `@keyframes` identifiers fadeInUp -> fade-in-up and fadeIn -> fade-in, and update the animation references in the CSS utility classes .animate-fade-in-up and .animate-fade-in to use "fade-in-up" and "fade-in" respectively so the `@keyframes` names and animation properties match.internal/tap/handler.go (2)
82-91: Usestring(ActionDelete)instead of the"delete"literal for consistency.Line 56 uses
string(event.Action)while line 83 hard-codes"delete". If the constant ever changes, only one path would update.Proposed fix
- activityID, err := h.activity.LogActivity(ctx, time.Now(), "delete", event.Collection, event.DID, event.RKey, "") + activityID, err := h.activity.LogActivity(ctx, time.Now(), string(event.Action), event.Collection, event.DID, event.RKey, "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/handler.go` around lines 82 - 91, Replace the hard-coded "delete" literal with the defined action constant string to keep behavior consistent: in the h.activity.LogActivity call (and any related UpdateStatus usage) use string(ActionDelete) instead of "delete", referencing the ActionDelete constant and existing usage like string(event.Action) so future changes to the action constant propagate uniformly.
99-101: Wrap error with context fromHandleIdentity.
actors.Upserterrors propagate unwrapped, unlike every other error path in this file. As per coding guidelines: "Always wrap errors with context usingfmt.Errorf."Proposed fix
func (h *IndexHandler) HandleIdentity(ctx context.Context, event *IdentityEvent) error { - return h.actors.Upsert(ctx, event.DID, event.Handle) + if err := h.actors.Upsert(ctx, event.DID, event.Handle); err != nil { + return fmt.Errorf("failed to upsert actor for identity event: %w", err) + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/handler.go` around lines 99 - 101, HandleIdentity currently returns the raw error from actors.Upsert; modify it to wrap any error with contextual information using fmt.Errorf and the %w verb (e.g., include function name and identifying data like event.DID or event.Handle) before returning so the error path matches other handlers; update the return to check the error from h.actors.Upsert(ctx, event.DID, event.Handle) and if non-nil return fmt.Errorf("HandleIdentity: upsert actor %s: %w", event.DID, err).internal/tap/admin.go (1)
153-158: Consider usingreq.SetBasicAuthinstead of manual base64 encoding.Go's
net/httpprovidesreq.SetBasicAuth(username, password)which handles the encoding internally.Proposed fix
func (c *AdminClient) setAuth(req *http.Request) { if c.password == "" { return } - creds := base64.StdEncoding.EncodeToString([]byte("admin:" + c.password)) - req.Header.Set("Authorization", "Basic "+creds) + req.SetBasicAuth("admin", c.password) }This also removes the need for the
"encoding/base64"import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/admin.go` around lines 153 - 158, The setAuth method on AdminClient currently builds a Basic auth header manually; replace the manual base64 encoding with the standard library helper by calling req.SetBasicAuth("admin", c.password) inside AdminClient.setAuth and remove the now-unused "encoding/base64" import; keep the early return when c.password == "" and ensure Authorization header is no longer set manually via req.Header.Set.internal/tap/consumer.go (1)
97-159: Reconnection logic is sound.The exponential backoff with cap, stop-signal checks at multiple points, and graceful handling of context cancellation are well-implemented.
One subtlety: when
runOncereturnsnil(normal close — e.g.,CloseNormalClosure), the backoff resets tominBackoff(line 114) and the log says "closed unexpectedly" (line 137). A normal closure from the server might not warrant the "unexpectedly" label — consider distinguishing the two cases if it matters for operational observability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/consumer.go` around lines 97 - 159, Consumer.Start currently treats a nil return from runOnce as an "unexpected" closure; to distinguish normal server-initiated closes, make runOnce return a sentinel error (e.g., var ErrNormalClosure = errors.New("normal closure")) when it sees a CloseNormalClosure, or alternatively return (error, bool) where the bool indicates normalClose, then update Start to check that sentinel/flag and log at Info (or a less alarming level) instead of Warn for normal closures; reference Consumer.Start and runOnce and add ErrNormalClosure in the tap package so the reconnection logging reflects normal vs unexpected termination.internal/graphql/schema/builder.go (2)
872-949: Search resolver doesn't supporttotalCount.The search resolver returns the
genericRecordConnectionTypewhich declares atotalCountfield, but the search result map never sets it. If a client queriessearch { totalCount }, they'll getnull. This is probably intentional (search count is expensive), but consider documenting this or returning0to avoid silent nulls on a non-nullable-looking field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/graphql/schema/builder.go` around lines 872 - 949, Search resolver createSearchResolver returns a connection type that declares totalCount but never sets it, causing clients to receive null; update the returned map in createSearchResolver to include a "totalCount" key (e.g. set to 0 to avoid expensive counting) or call an existing count method on repos.Records if accurate totals are required, and ensure the key is included alongside "edges" and "pageInfo" so queries for totalCount return a proper integer; locate createSearchResolver in builder.go and modify the final returned map to include "totalCount".
860-865: Silently swallowedtotalCounterror.When
GetCollectionCountFilteredfails, the error is discarded andtotalCountis simply omitted from the result. The same pattern appears at line 784. While this avoids breaking the query, the client gets no signal that the count failed. Consider logging the error at debug/warn level so operators can diagnose issues.Proposed fix
if isTotalCountRequested(p) { count, err := repos.Records.GetCollectionCountFiltered(p.Context, collection, filters, didFilter) - if err == nil { + if err != nil { + slog.Warn("Failed to get totalCount", "collection", collection, "error", err) + } else { result["totalCount"] = int(count) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/graphql/schema/builder.go` around lines 860 - 865, The code currently swallows errors from repos.Records.GetCollectionCountFiltered inside the isTotalCountRequested(p) blocks (around the logic that sets result["totalCount"]) — update both occurrences so that when GetCollectionCountFiltered returns an error you log it (debug/warn) with context (collection name/ID, filters and the error) instead of silently ignoring it; keep the existing behavior of not failing the query but ensure you call the project’s logger (or a package logger) e.g., logger.Warnf("GetCollectionCountFiltered failed for collection=%s filters=%v: %v", collection, filters, err) when err != nil before leaving the block.internal/tap/consumer_test.go (3)
736-740: Globalslog.SetDefaultis unsafe for parallel test execution.Mutating the process-wide default logger creates a data race if any test in this package (or a future refactor) adds
t.Parallel(). This also affects lines 816-818 inTestConsumer_BackoffEscalatesOnPersistentFailure.Consider injecting a
*slog.LoggerintoConsumer(orConsumerConfig) so each test can supply its own logger without touching global state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/consumer_test.go` around lines 736 - 740, The test mutates global logger via slog.SetDefault (using capturingHandler) which is unsafe for parallel tests; instead add a logger field (e.g., Logger *slog.Logger) to Consumer or ConsumerConfig and thread it through where Consumer is constructed, update the tests (including TestConsumer_BackoffEscalatesOnPersistentFailure) to create a per-test capturingHandler and pass slog.New(capturing) into the Consumer/ConsumerConfig, and remove calls to slog.SetDefault/SetDefault restoration so no global state is mutated.
832-838: Unnecessary use ofatomic.AddInt32inside a mutex-protected block.
attemptCountis only written here undercapturing.muand read afterward on a single goroutine. A plain increment (attemptCount++) suffices and is clearer. Alternatively,attemptCountcould be declared as a plainintinstead ofint32.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/consumer_test.go` around lines 832 - 838, The code is using atomic.AddInt32(&attemptCount, 1) while already holding capturing.mu and only accessed on a single goroutine; replace the atomic increment with a plain increment (attemptCount++), and if desired change attemptCount from int32 to int to reflect non-concurrent usage; update any subsequent reads/uses of attemptCount to the new type if you change it.
807-808: Dead code:origDialeris unused.
origDialeris assigned and immediately blanked. Looks like a leftover from a previous approach to intercepting the dialer. Remove it to reduce noise.Proposed fix
- origDialer := websocket.DefaultDialer - _ = origDialer // ensure import used🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tap/consumer_test.go` around lines 807 - 808, Remove the dead variable origDialer and its no-op blanking: the lines "origDialer := websocket.DefaultDialer" and "_ = origDialer" should be deleted; if websocket.DefaultDialer is no longer referenced elsewhere, also remove the now-unused import to avoid unused-import noise. This removes the leftover from the previous dialer-interception approach and cleans up the test.
|
|
||
| - Railway CLI v4+ installed and logged in (`railway whoami`) | ||
| - Linked to project: `railway status` should show project `hyperindex` | ||
| - On the correct git branch (typically `tap-feature`) |
There was a problem hiding this comment.
Stale branch reference — update to match current workflow.
Line 30 says "typically tap-feature", but this PR is on filter-feature and the likely long-lived default would be main. An agent following these instructions verbatim would attempt to check out tap-feature before deploying.
📝 Proposed fix
-- On the correct git branch (typically `tap-feature`)
+- On the correct git branch (typically `main` or your feature branch)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - On the correct git branch (typically `tap-feature`) | |
| - On the correct git branch (typically `main` or your feature branch) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/deploy-railway/SKILL.md at line 30, Update the stale branch
reference in the SKILL.md line that currently reads "typically `tap-feature`"
(search for that exact phrase) to reflect the current workflow—replace it with
the long-lived default branch `main`, or make it generic like
"`<feature-branch>` (e.g., filter-feature or main)" so agents won't check out
the wrong branch; edit the sentence in .agents/skills/deploy-railway/SKILL.md to
use the chosen replacement.
| ### Backend (`backend` service) | ||
| | Variable | Value | | ||
| |----------|-------| | ||
| | `HOST` | `0.0.0.0` | | ||
| | `PORT` | `8080` | | ||
| | `DATABASE_URL` | `sqlite:/app/data/hypergoat.db` | | ||
| | `EXTERNAL_BASE_URL` | `https://api.hi.gainforest.app` | | ||
| | `TRUST_PROXY_HEADERS` | `true` | | ||
| | `ADMIN_DIDS` | `did:plc:qc42fmqqlsmdq7jiypiiigww` (daviddao.org) | | ||
| | `OAUTH_LOOPBACK_MODE` | `true` | | ||
| | `SECRET_KEY_BASE` | *(set on Railway, do not change)* | | ||
|
|
There was a problem hiding this comment.
Add blank lines around both environment variable tables (MD058).
Static analysis reports MD058 violations at lines 85 and 97. Tables must be surrounded by blank lines for valid Markdown rendering in many parsers.
📝 Proposed fix
### Backend (`backend` service)
+
| Variable | Value |
|----------|-------|
...
| `SECRET_KEY_BASE` | *(set on Railway, do not change)* |
+
### Frontend (`frontend` service)
+
| Variable | Value |
|----------|-------|
...
| `ATPROTO_JWK_PRIVATE` | *(ES256 JWK, set on Railway, do not change)* |
+Also applies to: 96-105
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 85-85: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/deploy-railway/SKILL.md around lines 84 - 95, The Markdown
tables under the "Backend (`backend` service)" section are missing blank lines
before and after them causing MD058 violations; edit
.agents/skills/deploy-railway/SKILL.md to ensure there is a blank line both
above and below each environment variable table (the table that starts with |
Variable | Value | and the subsequent table mentioned around lines 96-105) so
each table is separated from surrounding text and other blocks.
| Check that `ATPROTO_JWK_PRIVATE` and `PUBLIC_URL` are set on the frontend service. Generate a new JWK with: | ||
| ```bash | ||
| node scripts/generate-jwk.js # (in hyperscan repo, or client/scripts/ if copied) | ||
| ``` |
There was a problem hiding this comment.
JWK generation script reference is ambiguous.
Line 125 references hyperscan repo as the canonical location for generate-jwk.js, with a parenthetical noting it may or may not be in client/scripts/. If an agent (or engineer) follows this guide and the script isn't present in the current repo, the OAuth setup step silently fails. The reference should be made unambiguous — either commit the script to a canonical location in this repo and reference that path exclusively, or provide an alternative (e.g., an inline node -e one-liner using the jose library) so the step is self-contained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/deploy-railway/SKILL.md around lines 123 - 126, The doc
references a non-canonical location for generate-jwk.js which can cause silent
failures when setting ATPROTO_JWK_PRIVATE and PUBLIC_URL; update SKILL.md to
either (A) commit and reference a single canonical script path in this repo
(e.g., ensure generate-jwk.js exists under scripts/ and point to that) or (B)
provide a self-contained alternative command (for example a node -e one-liner
using the jose library) and include explicit instructions for setting
ATPROTO_JWK_PRIVATE and PUBLIC_URL; ensure the file name generate-jwk.js and the
env vars ATPROTO_JWK_PRIVATE and PUBLIC_URL are mentioned so readers can find
and run the correct script or fallback one-liner.
| RUN npm ci | ||
|
|
||
| # Copy source | ||
| COPY . . |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether .dockerignore exists in the client directory and
# whether it excludes node_modules and .next.
fd -H '\.dockerignore' --max-depth 3
# Also show its contents if found
fd -H '\.dockerignore' --max-depth 3 --exec cat {}Repository: hypercerts-org/hyperindex
Length of output: 51
🏁 Script executed:
# Find the Dockerfile in question
fd -H 'Dockerfile' client/
# Search for any dockerignore files (case-insensitive, anywhere in repo)
find . -iname '*dockerignore*' 2>/dev/null
# Check if there's a .dockerignore at the repo root
ls -la | grep -i dockerignore || echo "No .dockerignore at repo root"
# Show the client/Dockerfile around line 17 for context
cat -n client/Dockerfile | sed -n '10,25p'Repository: hypercerts-org/hyperindex
Length of output: 461
Add a .dockerignore file to exclude node_modules and .next.
The COPY . . at line 17 copies the host's entire directory into the image, including node_modules if it exists. This overwrites the clean npm ci install with potentially platform-mismatched binaries (e.g., native modules compiled on macOS copied into an Alpine Linux image). Create a .dockerignore at the repository root with at least:
node_modules
.next
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/Dockerfile` at line 17, The Dockerfile uses the COPY . . instruction
which copies the entire repo (including host node_modules and .next) into the
image; create a .dockerignore file at the repository root that excludes
node_modules and .next (at minimum) so the COPY . . in client/Dockerfile will
not bring host-built artifacts into the image; add the lines node_modules and
.next to that file (one per line) and commit it alongside the Dockerfile change.
| const response = await fetch(`${env.HYPERINDEX_URL}/admin/graphql`, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify(body), | ||
| }); |
There was a problem hiding this comment.
Add a request timeout to the Hyperindex fetch call.
Without a timeout, a slow or hung Hyperindex backend will hold the Next.js request thread indefinitely, potentially exhausting the server's connection pool under load.
⏱️ Proposed fix
- const response = await fetch(`${env.HYPERINDEX_URL}/admin/graphql`, {
- method: "POST",
- headers,
- body: JSON.stringify(body),
- });
+ const response = await fetch(`${env.HYPERINDEX_URL}/admin/graphql`, {
+ method: "POST",
+ headers,
+ body: JSON.stringify(body),
+ signal: AbortSignal.timeout(30_000), // 30 s hard cap
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch(`${env.HYPERINDEX_URL}/admin/graphql`, { | |
| method: "POST", | |
| headers, | |
| body: JSON.stringify(body), | |
| }); | |
| const response = await fetch(`${env.HYPERINDEX_URL}/admin/graphql`, { | |
| method: "POST", | |
| headers, | |
| body: JSON.stringify(body), | |
| signal: AbortSignal.timeout(30_000), // 30 s hard cap | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/api/admin/graphql/route.ts` around lines 30 - 34, The fetch
call that assigns to const response
(fetch(`${env.HYPERINDEX_URL}/admin/graphql`, ...)) needs a request timeout to
avoid hung requests: create an AbortController, pass controller.signal into the
fetch options, start a setTimeout that calls controller.abort() after a
configured timeout (e.g. 5s or ENV value), clear the timeout when the fetch
resolves, and handle the abort case (throw or return a 504/timeout error) so the
Next.js request thread is not held indefinitely; update the fetch invocation to
include signal and ensure the timeout is cleared after awaiting response.
| @@ -9,7 +9,7 @@ services: | |||
| - HOST=0.0.0.0 | |||
| - PORT=8080 | |||
| - DATABASE_URL=postgres://hypergoat:hypergoat@postgres:5432/hypergoat?sslmode=disable | |||
There was a problem hiding this comment.
Parameterize the PostgreSQL credentials in DATABASE_URL.
hypergoat:hypergoat is committed in plain text and cannot be rotated without modifying the compose file. Given that SECRET_KEY_BASE was just hardened to require an explicit value, applying the same principle to DB credentials is consistent. The postgres service definition (lines 22–25) should be updated in lockstep.
🔒 Proposed fix
- - DATABASE_URL=postgres://hypergoat:hypergoat@postgres:5432/hypergoat?sslmode=disable
+ - DATABASE_URL=postgres://${POSTGRES_USER:-hypergoat}:${POSTGRES_PASSWORD:-hypergoat}@postgres:5432/${POSTGRES_DB:-hypergoat}?sslmode=disableAnd in the postgres service:
- - POSTGRES_USER=hypergoat
- - POSTGRES_PASSWORD=hypergoat
- - POSTGRES_DB=hypergoat
+ - POSTGRES_USER=${POSTGRES_USER:-hypergoat}
+ - POSTGRES_PASSWORD=${POSTGRES_PASSWORD:-hypergoat}
+ - POSTGRES_DB=${POSTGRES_DB:-hypergoat}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - DATABASE_URL=postgres://hypergoat:hypergoat@postgres:5432/hypergoat?sslmode=disable | |
| - DATABASE_URL=postgres://${POSTGRES_USER:-hypergoat}:${POSTGRES_PASSWORD:-hypergoat}@postgres:5432/${POSTGRES_DB:-hypergoat}?sslmode=disable |
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 11-12: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.postgres.yml` at line 11, Replace the hardcoded credentials in
the DATABASE_URL entry (the line containing
"DATABASE_URL=postgres://hypergoat:hypergoat@postgres:5432/hypergoat?sslmode=disable")
with a parameterized URL that references environment variables (e.g.,
POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB) and then update the postgres
service definition to set those env vars (or read them from an external .env)
instead of embedding the username/password; ensure the compose file uses the
variable interpolation syntax for DATABASE_URL and that the postgres service
declares matching POSTGRES_USER, POSTGRES_PASSWORD, and POSTGRES_DB environment
variables so credentials can be rotated without editing the compose file.
| // Health checks if Tap is healthy. | ||
| // GET /health — expects {"status":"ok"} | ||
| func (c *AdminClient) Health(ctx context.Context) error { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/health", nil) |
There was a problem hiding this comment.
Use http.NoBody instead of nil for GET request bodies.
The linter reports httpNoBody: http.NoBody should be preferred to the nil request body. Same applies to line 100.
Proposed fix
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/health", nil)
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/health", http.NoBody)And at line 100:
- req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/info/"+did, nil)
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/info/"+did, http.NoBody)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/health", nil) | |
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.baseURL+"/health", http.NoBody) |
🧰 Tools
🪛 GitHub Check: lint
[failure] 69-69:
httpNoBody: http.NoBody should be preferred to the nil request body (gocritic)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tap/admin.go` at line 69, The http.NewRequestWithContext calls that
create GET requests (e.g., the one building the health check request using
c.baseURL+"/health" and the other at the later NewRequestWithContext on line
~100) pass nil as the body; replace nil with http.NoBody in both
NewRequestWithContext invocations so the GET requests use http.NoBody per the
linter suggestion (search for the http.NewRequestWithContext calls that
construct requests from c.baseURL to locate each site).
|
|
||
| select { | ||
| case err := <-done: | ||
| if err != nil && err != context.Canceled { |
There was a problem hiding this comment.
Use errors.Is instead of != for error comparison.
context.Canceled may be wrapped by consumer.Start(). Direct comparison with != will miss wrapped errors, causing the test to incorrectly flag a legitimate cancellation as unexpected.
Proposed fix
- if err != nil && err != context.Canceled {
+ if err != nil && !errors.Is(err, context.Canceled) {Also add "errors" to the import block:
import (
"context"
"encoding/json"
+ "errors"
"fmt"🧰 Tools
🪛 GitHub Check: lint
[failure] 764-764:
comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tap/consumer_test.go` at line 764, Replace the direct equality check
against context.Canceled in the test (the clause that reads if err != nil && err
!= context.Canceled) with an errors.Is check (errors.Is(err, context.Canceled))
so wrapped cancellations from consumer.Start() are detected; also add "errors"
to the import block so errors.Is is available and keep the existing handling
logic for other non-nil errors.
| default: | ||
| // Unknown event type — log and skip without acking. | ||
| slog.Warn("Unknown Tap event type", "type", event.Type, "id", event.ID) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ack-related code and comments in internal/tap/
rg -n -i "ack" --type=go internal/tap/ -C3Repository: hypercerts-org/hyperindex
Length of output: 42526
🏁 Script executed:
#!/bin/bash
# Also examine the full consumer.go file to understand the dispatch and runOnce functions
wc -l internal/tap/consumer.go
head -300 internal/tap/consumer.go | tail -100Repository: hypercerts-org/hyperindex
Length of output: 2746
🏁 Script executed:
#!/bin/bash
# Look for the runOnce function and the full dispatch function implementation
rg -n "func.*runOnce|func.*dispatch" --type=go internal/tap/consumer.go -A 30Repository: hypercerts-org/hyperindex
Length of output: 1860
🏁 Script executed:
#!/bin/bash
# Search for any documentation, comments, or references about Tap event handling strategy
rg -n "unknown|Unknown" --type=go internal/tap/ -B2 -A2 | head -50Repository: hypercerts-org/hyperindex
Length of output: 698
🏁 Script executed:
#!/bin/bash
# Check if there are any TODOs or FIXMEs related to event handling or acking
rg -n "TODO|FIXME|BUG" --type=go internal/tap/Repository: hypercerts-org/hyperindex
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look for any external protocol documentation or comments about redelivery
rg -n "redelivery|redeliver|protocol" --type=go internal/tap/ -iRepository: hypercerts-org/hyperindex
Length of output: 183
Unknown event types are intentionally not acked, but this behavior lacks test coverage.
The return nil on line 256 exits dispatch() before reaching the ack logic, preventing acknowledgment of unknown events. The inline comment confirms this is intentional, but unlike the similar pattern for handler errors (which has TestConsumer_HandlerErrorDoesNotAck), unknown event handling has no test case. If the Tap server redelivers unacked messages, these events would loop indefinitely. Either add a test to document the expected behavior (like TestConsumer_UnknownEventNotAcked) or add a code comment explaining the Tap protocol's expectations for unknown event types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tap/consumer.go` around lines 253 - 257, dispatch() currently
returns early for unknown event types and intentionally does not ack them; add a
unit test named TestConsumer_UnknownEventNotAcked (mirroring
TestConsumer_HandlerErrorDoesNotAck) that constructs an unknown Tap event, calls
dispatch()/the public consume path, and asserts no Ack was sent to the mock Tap
server to document behavior; additionally add a brief code comment above the
unknown-event default case in dispatch() referencing the Tap protocol
expectations and the new test name to make the rationale explicit.
| } | ||
| // Drain the create event | ||
| <-sub.Events |
There was a problem hiding this comment.
Unbuffered channel drain can hang the test if the event is not published.
<-sub.Events on line 102 (and line 162) will block indefinitely if HandleRecord doesn't publish the expected event. Use a select with a timeout to prevent the test from hanging:
Proposed fix
// Drain the create event
- <-sub.Events
+ select {
+ case <-sub.Events:
+ case <-time.After(2 * time.Second):
+ t.Fatal("timed out waiting for create event to drain")
+ }(Same for line 162. Requires adding "time" to imports.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/tap/handler_test.go` around lines 100 - 102, The test currently
drains events with a blocking receive <-sub.Events which can hang if
HandleRecord never publishes; replace those direct receives (around the uses of
sub.Events in handler_test.go) with a non-blocking select that waits on
sub.Events and a time.After timeout (e.g., time.After(1s)) so the test fails
fast instead of hanging, and add "time" to the imports; apply this change for
both occurrences (the drain after the create event and the later drain around
line 162) so the test reports a timeout error when the event is missing.
3 participants
Summary
Adds comprehensive query capabilities to the public GraphQL API, transforming it from a basic "paginate newest-first" interface into a full-featured query layer.
New features
eq,neq,gt,lt,gte,lte,in,contains,startsWith,isNulleqandinoperators (restrictedDIDFilterInputtype)totalCount— opt-in (only computed when selected in the GraphQL query via AST introspection)last/before) per Relay specClampPageSizehelperdidandrkeymetadata fields exposed on all typed record typesSecurity & robustness fixes (from code review)
|-delimited to JSON array encoding (backward compatible)escapeLIKE()applied tocontains/startsWithoperatorsMaxINListSize = 100prevents SQLite parameter limit crashMaxFilterConditions = 20prevents abusecontext.WithTimeouton full-table-scan search queriesuri/cid/did/rkeyare skipped with warningutf8.RuneCountInString, increased to 3 charactersInfrastructure
(collection, indexed_at DESC, uri DESC)— migration 006 (SQLite + PostgreSQL)go test -tags integration ./internal/integration/)ClampPageSize, cursor encoding,sortFieldValueForRecord,extractFilters,emptyConnection, filter clause buildingNo breaking changes
go test ./...)Stats
Summary by CodeRabbit
Release Notes
New Features
Documentation
Infrastructure
Branding