Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds binding-document support across the control panel: new shared types package, Evault service (registry resolve, platform token, GraphQL pagination), control-panel API route and UI tab, and migration of local types to the shared package; plus small admin and workspace dependency updates. Changes
Sequence DiagramsequenceDiagram
participant UI as Control Panel UI
participant Route as API Route
participant EvaultSvc as EvaultService
participant Registry as Registry Service
participant Auth as Platform Cert Service
participant GraphQL as eVault GraphQL
UI->>Route: GET /api/evaults/[id]/binding-documents
Route->>Route: find vault by id/ename
Route->>EvaultSvc: fetchBindingDocuments(eName)
EvaultSvc->>EvaultSvc: normalize eName
EvaultSvc->>Registry: POST /resolve (eName)
Registry-->>EvaultSvc: eVault base URL
EvaultSvc->>Auth: POST /platforms/certification (cached)
Auth-->>EvaultSvc: token
EvaultSvc->>GraphQL: query bindingDocuments (paginated)
GraphQL-->>EvaultSvc: edges + pageInfo
EvaultSvc->>EvaultSvc: aggregate pages, validate & transform
EvaultSvc->>EvaultSvc: resolve display names (metaEnvelopes)
EvaultSvc-->>Route: { eName, documents, socialConnections }
Route-->>UI: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/control-panel/config/admin-enames.json (1)
2-7:⚠️ Potential issue | 🟡 MinorDuplicate admin EID detected.
The EID
@82f7a77a-f03a-52aa-88fc-1b1e488ad498appears twice in the admins list (lines 4 and 6). This appears unintentional and should be deduplicated.🔧 Proposed fix
{ "admins": [ "@7218b67d-da21-54d6-9a85-7c4db1d09768", "@82f7a77a-f03a-52aa-88fc-1b1e488ad498", "@35a31f0d-dd76-5780-b383-29f219fcae99", - "@82f7a77a-f03a-52aa-88fc-1b1e488ad498", "@6e1bbcd4-1f59-5bd8-aa3c-6f5301c356d7" ] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/config/admin-enames.json` around lines 2 - 7, The admins list contains a duplicate entry for EID "@82f7a77a-f03a-52aa-88fc-1b1e488ad498" in the "admins" array; remove the redundant occurrence so each admin EID appears only once (i.e., deduplicate the "admins" array and keep a single instance of the referenced EID).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/control-panel/src/lib/server/evault.ts`:
- Around line 5-16: The BINDING_DOCUMENTS_QUERY currently only fetches the first
N edges and thus caps results; update the query to accept an optional after
cursor (add $after: String) and request pageInfo { hasNextPage endCursor }, then
implement iterative pagination in the code that runs the query (the caller that
uses BINDING_DOCUMENTS_QUERY) by repeatedly calling the GraphQL client with
variables { first, after } while pageInfo.hasNextPage is true, accumulating
edges[].node (parsed/id) into a single result array and returning that full
list; apply the same change to the other analogous binding documents query usage
mentioned in the review so both places walk the connection until exhausted.
- Around line 205-224: The Promise.all map is calling resolveDisplayNameForEName
for each social candidate which can throw and abort fetchBindingDocuments; wrap
the call to resolveDisplayNameForEName inside a try/catch within the async
function passed to socialCandidates.map so any error is caught and you fall back
to using otherPartyEName (or return null for that entry) instead of letting the
rejection propagate; keep returning the same object shape (id, name,
witnessEName, signatures) and preserve the .filter((entry): entry is
NonNullable<typeof entry> => entry !== null) afterwards.
In
`@infrastructure/control-panel/src/routes/api/evaults/`[evaultId]/binding-documents/+server.ts:
- Around line 10-15: The code treats an empty array from
registryService.getEVaults() as "not found"; change it to preserve lookup
failures by detecting an empty response as a registry error and returning a 502
instead of a 404. Specifically, after calling registryService.getEVaults() and
before using evaults.find(...), check if evaults is empty (evaults.length === 0)
and if so return json({ error: "Registry unavailable: failed to fetch eVaults"
}, { status: 502 }); otherwise continue to locate vault by evaultId and only
return 404 if a non-empty registry lookup legitimately yields no match.
Optionally, consider updating registryService.getEVaults() to throw on fetch
errors so this route can catch and propagate a 5xx response instead of relying
on an empty array.
In `@infrastructure/control-panel/src/routes/evaults/`[evaultId]/+page.svelte:
- Around line 22-25: The helper getDataValue currently returns 'N/A' for
non-string values which ends up used as an image src (e.g., photoBlob) and
produces a broken <img> request; update the rendering logic to treat blob/photo
fields specially: keep getDataValue for table/text cells, but when rendering the
image (where photoBlob or similar is used) check that the value is a valid
string/URL (or truthy) before using it as src and, if not, render fallback copy
or an empty state instead of an <img> with 'N/A'. Locate usages around photoBlob
rendering in +page.svelte and change the image branch to guard the value first
and only pass a string to <img src=>, while using getDataValue for non-image
text cells.
In `@packages/types/package.json`:
- Around line 13-21: The exports object currently lists "import" before "types"
(e.g., the "." entry and the "./binding-document" entry); reorder each export
entry so the "types" condition appears before "import" (move the "types":
"./dist/*.d.ts" entries above the corresponding "import": "./dist/*.js" entries)
to ensure Node's condition resolution picks up type definitions first for
TypeScript and tooling.
---
Outside diff comments:
In `@infrastructure/control-panel/config/admin-enames.json`:
- Around line 2-7: The admins list contains a duplicate entry for EID
"@82f7a77a-f03a-52aa-88fc-1b1e488ad498" in the "admins" array; remove the
redundant occurrence so each admin EID appears only once (i.e., deduplicate the
"admins" array and keep a single instance of the referenced EID).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6435186-fac9-46dc-bec1-7b9b616f696b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
infrastructure/control-panel/config/admin-enames.jsoninfrastructure/control-panel/package.jsoninfrastructure/control-panel/src/lib/server/evault.tsinfrastructure/control-panel/src/lib/services/evaultService.tsinfrastructure/control-panel/src/routes/api/evaults/[evaultId]/binding-documents/+server.tsinfrastructure/control-panel/src/routes/evaults/[evaultId]/+page.sveltepackages/types/package.jsonpackages/types/src/binding-document.tspackages/types/src/index.tspackages/types/tsconfig.build.jsonpackages/types/tsconfig.jsonplatforms/enotary/package.jsonplatforms/enotary/src/lib/server/evault.tsplatforms/enotary/src/lib/server/types.tsplatforms/enotary/src/lib/server/witness.tsplatforms/enotary/src/routes/user/[ename]/+page.svelte
💤 Files with no reviewable changes (1)
- platforms/enotary/src/lib/server/types.ts
infrastructure/control-panel/src/routes/api/evaults/[evaultId]/binding-documents/+server.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
infrastructure/control-panel/src/lib/server/evault.ts (3)
183-192: Pagination loop has no upper bound.Same as the enotary version—consider adding an iteration cap to guard against malformed server responses.
🛡️ Optional safeguard
const allEdges: Array<{ node: { id: string; parsed: Record<string, unknown> | null } }> = []; let afterCursor: string | null = null; + const MAX_PAGES = 100; + let pageCount = 0; do { + if (++pageCount > MAX_PAGES) { + console.warn(`[evault] Pagination exceeded ${MAX_PAGES} pages for ${normalized}`); + break; + } const res: BindingDocumentsResponse = await client.request<BindingDocumentsResponse>(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/lib/server/evault.ts` around lines 183 - 192, The pagination loop using client.request with BINDING_DOCUMENTS_QUERY and BindingDocumentsResponse can loop forever if the server returns malformed pageInfo; add a hard iteration cap (e.g. MAX_PAGES constant) and increment a counter inside the do/while to break once exceeded, and also detect and break when endCursor does not change between iterations (i.e., previousCursor === afterCursor) to avoid infinite loops; apply these guards around the afterCursor/allEdges update logic so client.request, afterCursor and allEdges handling is protected.
60-62: Profile name cache grows unbounded.The
profileNameCacheMap accumulates entries indefinitely. In a long-running server process handling many unique eNames, this could lead to memory growth.Consider using a bounded cache (LRU) or adding periodic cleanup if the control panel is expected to handle a large number of unique users over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/lib/server/evault.ts` around lines 60 - 62, The profileNameCache Map on EvaultService grows unbounded; change it to a bounded cache (e.g., an LRU with a max size) or add eviction logic to prevent memory growth: replace the current profileNameCache = new Map<string,string>() with a bounded LRU implementation or wrap Map operations in EvaultService (methods that set/get entries) to evict the oldest entries when a configured MAX_CACHE_SIZE is exceeded; update EvaultService constructors and any uses of profileNameCache (look for profileNameCache.set, .get, .has, .delete) to use the new bounded behavior and expose the MAX_CACHE_SIZE as a constant or config value.
1-252: Significant code duplication withplatforms/enotary/src/lib/server/evault.ts.This file shares nearly identical patterns, queries, and interfaces with the enotary version. Consider extracting common logic into a shared module (perhaps in the new
@metastate-foundation/typespackage or a new shared utilities package) to reduce maintenance burden and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/lib/server/evault.ts` around lines 1 - 252, This file duplicates logic from platforms/enotary/src/lib/server/evault.ts — extract shared functionality (URL resolution, platform token retrieval, GraphQL client creation, and common queries/interfaces) into a shared module and import it here: move BINDING_DOCUMENTS_QUERY, USER_PROFILE_QUERY, RegistryResolveResponse, PlatformCertificationResponse, BindingDocumentsResponse, normalizeEName logic, getPlatformToken, resolveEVaultUrl, and a helper to construct GraphQLClient into the new shared package (or `@metastate-foundation/types` utilities), then refactor EvaultService to call those shared functions (e.g., resolveEVaultUrl, getPlatformToken, getGraphqlUrl/GraphQL client factory) and keep only service-specific methods like fetchBindingDocuments and resolveDisplayNameForEName that orchestrate the shared pieces; ensure to reuse the same query constants and types (BindingDocument, SocialConnection) so both repos import the common implementation instead of duplicating code.platforms/enotary/src/lib/server/evault.ts (1)
162-178: Pagination implementation looks correct.The do-while loop properly accumulates edges and handles the cursor-based pagination. Each GraphQL request has its own timeout via axios, which provides reasonable protection.
Consider adding an iteration cap as a safeguard against malformed server responses that always return
hasNextPage: true:🛡️ Optional safeguard against runaway pagination
const allEdges: Array<{ node: { id: string; parsed: Record<string, unknown> | null }; }> = []; let afterCursor: string | null = null; + const MAX_PAGES = 100; + let pageCount = 0; do { + if (++pageCount > MAX_PAGES) { + console.warn(`[evault] Pagination exceeded ${MAX_PAGES} pages for ${normalized}`); + break; + } const response = await client.request<BindingDocumentsResponse>(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/enotary/src/lib/server/evault.ts` around lines 162 - 178, Add a defensive iteration cap to the pagination loop that accumulates into allEdges: introduce a maxPages counter (e.g., maxPages = 1000) and a pageCount variable, increment pageCount each loop iteration inside the do/while that calls client.request(BINDING_DOCUMENTS_QUERY), and if pageCount exceeds maxPages break the loop and surface an error or warning (via existing logger or by throwing) to avoid infinite pagination when response.bindingDocuments.pageInfo.hasNextPage is stuck true; ensure the check references afterCursor and allEdges so it safely exits and reports context (query name BINDING_DOCUMENTS_QUERY, function where loop lives).infrastructure/control-panel/src/routes/evaults/[evaultId]/+page.svelte (1)
87-92: Consider caching or debouncing binding document fetches.Each time the user switches to the "Binding Documents" tab,
fetchBindingDocumentsis called unconditionally. For better UX:
- Rapid tab clicks could trigger multiple concurrent fetches
- Re-selecting the tab refetches data the user may have just viewed
💡 Optional: Skip refetch if data already loaded
const selectTab = (tab: string) => { selectedTab = tab; - if (tab === 'binding-documents') { + if (tab === 'binding-documents' && documents.length === 0 && !bindingDocumentsLoading) { fetchBindingDocuments(); } };Or add a "Refresh" button within the binding documents tab (similar to the logs tab) for explicit refetch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/routes/evaults/`[evaultId]/+page.svelte around lines 87 - 92, The selectTab handler calls fetchBindingDocuments every time "binding-documents" is selected, causing duplicate/refetches on rapid clicks or re-selections; modify selectTab and the binding-documents state to avoid unnecessary fetches by (a) adding a local flag or state like bindingDocumentsLoaded or bindingDocumentsData and only call fetchBindingDocuments when that data is empty or a forceRefresh is passed, and (b) guard against concurrent calls by tracking an isFetchingBindingDocuments boolean (or debounce rapid calls) so multiple rapid selections don't spawn multiple requests; optionally wire a Refresh button inside the binding documents view to call fetchBindingDocuments(forceRefresh=true) for explicit reloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/control-panel/src/lib/server/evault.ts`:
- Around line 77-99: getPlatformToken currently caches this.platformToken
forever; change it to cache the token with an expiry and refresh when expired by
updating getPlatformToken to (1) parse an expiry/ttl from the
PlatformCertificationResponse (e.g., expires_at or ttl) or, if absent, apply a
safe default TTL, (2) store both this.platformToken and a new
this.platformTokenExpiresAt (timestamp), and (3) on subsequent calls return the
cached token only if Date.now() < this.platformTokenExpiresAt, otherwise
re-fetch and replace both fields; update any types/interfaces
(PlatformCertificationResponse) to reflect optional expiry fields and ensure
error branches still clear/avoid stale tokens.
---
Nitpick comments:
In `@infrastructure/control-panel/src/lib/server/evault.ts`:
- Around line 183-192: The pagination loop using client.request with
BINDING_DOCUMENTS_QUERY and BindingDocumentsResponse can loop forever if the
server returns malformed pageInfo; add a hard iteration cap (e.g. MAX_PAGES
constant) and increment a counter inside the do/while to break once exceeded,
and also detect and break when endCursor does not change between iterations
(i.e., previousCursor === afterCursor) to avoid infinite loops; apply these
guards around the afterCursor/allEdges update logic so client.request,
afterCursor and allEdges handling is protected.
- Around line 60-62: The profileNameCache Map on EvaultService grows unbounded;
change it to a bounded cache (e.g., an LRU with a max size) or add eviction
logic to prevent memory growth: replace the current profileNameCache = new
Map<string,string>() with a bounded LRU implementation or wrap Map operations in
EvaultService (methods that set/get entries) to evict the oldest entries when a
configured MAX_CACHE_SIZE is exceeded; update EvaultService constructors and any
uses of profileNameCache (look for profileNameCache.set, .get, .has, .delete) to
use the new bounded behavior and expose the MAX_CACHE_SIZE as a constant or
config value.
- Around line 1-252: This file duplicates logic from
platforms/enotary/src/lib/server/evault.ts — extract shared functionality (URL
resolution, platform token retrieval, GraphQL client creation, and common
queries/interfaces) into a shared module and import it here: move
BINDING_DOCUMENTS_QUERY, USER_PROFILE_QUERY, RegistryResolveResponse,
PlatformCertificationResponse, BindingDocumentsResponse, normalizeEName logic,
getPlatformToken, resolveEVaultUrl, and a helper to construct GraphQLClient into
the new shared package (or `@metastate-foundation/types` utilities), then refactor
EvaultService to call those shared functions (e.g., resolveEVaultUrl,
getPlatformToken, getGraphqlUrl/GraphQL client factory) and keep only
service-specific methods like fetchBindingDocuments and
resolveDisplayNameForEName that orchestrate the shared pieces; ensure to reuse
the same query constants and types (BindingDocument, SocialConnection) so both
repos import the common implementation instead of duplicating code.
In `@infrastructure/control-panel/src/routes/evaults/`[evaultId]/+page.svelte:
- Around line 87-92: The selectTab handler calls fetchBindingDocuments every
time "binding-documents" is selected, causing duplicate/refetches on rapid
clicks or re-selections; modify selectTab and the binding-documents state to
avoid unnecessary fetches by (a) adding a local flag or state like
bindingDocumentsLoaded or bindingDocumentsData and only call
fetchBindingDocuments when that data is empty or a forceRefresh is passed, and
(b) guard against concurrent calls by tracking an isFetchingBindingDocuments
boolean (or debounce rapid calls) so multiple rapid selections don't spawn
multiple requests; optionally wire a Refresh button inside the binding documents
view to call fetchBindingDocuments(forceRefresh=true) for explicit reloads.
In `@platforms/enotary/src/lib/server/evault.ts`:
- Around line 162-178: Add a defensive iteration cap to the pagination loop that
accumulates into allEdges: introduce a maxPages counter (e.g., maxPages = 1000)
and a pageCount variable, increment pageCount each loop iteration inside the
do/while that calls client.request(BINDING_DOCUMENTS_QUERY), and if pageCount
exceeds maxPages break the loop and surface an error or warning (via existing
logger or by throwing) to avoid infinite pagination when
response.bindingDocuments.pageInfo.hasNextPage is stuck true; ensure the check
references afterCursor and allEdges so it safely exits and reports context
(query name BINDING_DOCUMENTS_QUERY, function where loop lives).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 511ceacb-6d7a-4d5b-b2c3-db6f36c4c476
📒 Files selected for processing (6)
infrastructure/control-panel/config/admin-enames.jsoninfrastructure/control-panel/src/lib/server/evault.tsinfrastructure/control-panel/src/routes/api/evaults/[evaultId]/binding-documents/+server.tsinfrastructure/control-panel/src/routes/evaults/[evaultId]/+page.sveltepackages/types/package.jsonplatforms/enotary/src/lib/server/evault.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/control-panel/src/routes/api/evaults/[evaultId]/binding-documents/+server.ts
Comment on lines
+77
to
+99
| private async getPlatformToken(): Promise<string> { | ||
| if (this.platformToken) return this.platformToken; | ||
| const platform = PUBLIC_CONTROL_PANEL_URL || 'control-panel'; | ||
| const endpoint = new URL('/platforms/certification', this.getRegistryUrl()).toString(); | ||
| const response = await fetch(endpoint, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ platform }), | ||
| signal: AbortSignal.timeout(10_000) | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to get platform token: HTTP ${response.status}`); | ||
| } | ||
|
|
||
| const data = (await response.json()) as PlatformCertificationResponse; | ||
| if (!data.token) { | ||
| throw new Error('Failed to get platform token: missing token in response'); | ||
| } | ||
|
|
||
| this.platformToken = data.token; | ||
| return this.platformToken; | ||
| } |
Contributor
There was a problem hiding this comment.
Platform token is cached indefinitely without TTL.
The platformToken is cached after the first successful fetch and never refreshed. If the token expires or becomes invalid, all subsequent requests will fail until the service restarts.
🛡️ Consider adding token expiry handling
class EvaultService {
private platformToken: string | null = null;
+ private platformTokenExpiresAt: number = 0;
private profileNameCache = new Map<string, string>();
// ...
private async getPlatformToken(): Promise<string> {
- if (this.platformToken) return this.platformToken;
+ const now = Date.now();
+ if (this.platformToken && now < this.platformTokenExpiresAt) {
+ return this.platformToken;
+ }
const platform = PUBLIC_CONTROL_PANEL_URL || 'control-panel';
const endpoint = new URL('/platforms/certification', this.getRegistryUrl()).toString();
// ... fetch logic ...
this.platformToken = data.token;
+ // Assume 1 hour validity; adjust based on actual token TTL
+ this.platformTokenExpiresAt = now + 55 * 60 * 1000;
return this.platformToken;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/src/lib/server/evault.ts` around lines 77 - 99,
getPlatformToken currently caches this.platformToken forever; change it to cache
the token with an expiry and refresh when expired by updating getPlatformToken
to (1) parse an expiry/ttl from the PlatformCertificationResponse (e.g.,
expires_at or ttl) or, if absent, apply a safe default TTL, (2) store both
this.platformToken and a new this.platformTokenExpiresAt (timestamp), and (3) on
subsequent calls return the cached token only if Date.now() <
this.platformTokenExpiresAt, otherwise re-fetch and replace both fields; update
any types/interfaces (PlatformCertificationResponse) to reflect optional expiry
fields and ensure error branches still clear/avoid stale tokens.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/enotary/src/lib/server/evault.ts (1)
206-220:⚠️ Potential issue | 🟠 MajorPaginating all documents now makes this external lookup fan-out unbounded.
Line 207 uses
Promise.allover every social candidate. After enabling full pagination, this can trigger a large burst of network calls (resolveEVaultUrl+ profile query), increasing timeout/rate-limit risk.🔧 Proposed bounded approach (sequential baseline)
- const socialConnections = ( - await Promise.all( - socialCandidates.map(async (doc) => { + const socialConnectionsRaw: Array<SocialConnection | null> = []; + for (const doc of socialCandidates) { const otherPartyEName = doc.signatures.find( (signature) => signature.signer !== normalized, )?.signer; - if (!otherPartyEName) return null; + if (!otherPartyEName) { + socialConnectionsRaw.push(null); + continue; + } let name: string; try { name = await this.resolveDisplayNameForEName(otherPartyEName); } catch { name = otherPartyEName; } const relationDescription = typeof doc.data?.relation_description === "string" ? doc.data.relation_description : undefined; - return { + socialConnectionsRaw.push({ id: doc.id, name, witnessEName: otherPartyEName, relationDescription, signatures: doc.signatures, - }; - }), - ) - ).filter((entry): entry is NonNullable<typeof entry> => entry !== null); + }); + } + const socialConnections = socialConnectionsRaw.filter( + (entry): entry is NonNullable<typeof entry> => entry !== null, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/enotary/src/lib/server/evault.ts` around lines 206 - 220, The current Promise.all over socialCandidates in the socialConnections creation (code around socialConnections and socialCandidates) causes an unbounded fan-out to resolveDisplayNameForEName (and its underlying resolveEVaultUrl/profile calls); change this to a bounded approach by replacing Promise.all with either a sequential loop (for..of awaiting each resolveDisplayNameForEName) or use a concurrency limiter (e.g., p-limit) to cap concurrent calls (e.g., 5-10). Update the logic that builds socialConnections to collect results incrementally, preserve the same null-check behavior for missing otherPartyEName, and ensure errors remain handled per-item (catch per call and fallback to otherPartyEName).
♻️ Duplicate comments (1)
infrastructure/control-panel/src/lib/server/evault.ts (1)
77-99:⚠️ Potential issue | 🟠 MajorRefresh cached certification tokens instead of keeping them forever.
this.platformTokenis cached indefinitely. Once the registry rotates or expires that token, every GraphQL call in this service will keep using a stale credential until the process restarts.🕒 Bound the cache with an expiry
class EvaultService { private platformToken: string | null = null; + private platformTokenExpiresAt = 0; private profileNameCache = new Map<string, string>(); private async getPlatformToken(): Promise<string> { - if (this.platformToken) return this.platformToken; + if (this.platformToken && Date.now() < this.platformTokenExpiresAt) { + return this.platformToken; + } const platform = PUBLIC_CONTROL_PANEL_URL || 'control-panel'; const endpoint = new URL('/platforms/certification', this.getRegistryUrl()).toString(); const response = await fetch(endpoint, { @@ this.platformToken = data.token; + this.platformTokenExpiresAt = Date.now() + 55 * 60 * 1000; return this.platformToken; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/control-panel/src/lib/server/evault.ts` around lines 77 - 99, getPlatformToken currently caches this.platformToken indefinitely causing stale credentials; update getPlatformToken to track token expiry and refresh when expired: store alongside this.platformToken a timestamp field (e.g., platformTokenExpiry) and when calling getPlatformToken check that the token exists and Date.now() < platformTokenExpiry before returning; when fetching a new token, set platformTokenExpiry from an expires/expiry/ttl field on PlatformCertificationResponse if present, otherwise set a safe TTL (e.g., now + configured duration) so the token is re-fetched when expired; keep the function name getPlatformToken and the this.platformToken field references to locate where to add the expiry logic and update callers to only use getPlatformToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infrastructure/control-panel/src/lib/server/evault.ts`:
- Around line 198-205: The signatures array is only checked by Array.isArray and
can contain nulls or malformed entries, which leads to dereferencing
signature.signer later (see parsed destructure and the signature access around
the signature.signer usage at lines ~225-227); update the reader in evault.ts to
validate each signature entry up front by ensuring every element is a non-null
object with a valid signer (e.g., typeof entry === 'object' && entry !== null &&
typeof entry.signer === 'string') and either filter out/skip invalid entries or
reject the payload before attempting to read signature.signer, so malformed
stored documents cannot crash the binding-documents response.
- Around line 64-67: getRegistryUrl() currently hardcodes a different fallback
than the control-panel API, causing inconsistent registry resolution; create a
single shared constant or helper (e.g., export const DEFAULT_REGISTRY_URL or
function getPublicRegistryUrl()) that returns PUBLIC_REGISTRY_URL ||
'https://registry.staging.metastate.foundation' and replace the hardcoded
fallback in the getRegistryUrl method and the other evaults API code (the
handler currently using 'https://registry.staging.metastate.foundation') to
import and use that shared symbol so both places always resolve to the same
fallback.
In `@infrastructure/control-panel/src/routes/evaults/`[evaultId]/+page.svelte:
- Around line 98-100: The current code derives only idDocuments, selfDocuments,
and photoDocuments from documents and thus drops any other BindingDocument
types; add a catch-all derived array (e.g., otherDocuments =
$derived(documents.filter(d =>
!['photograph','id_document','self','social_connection'].includes(d.type)))) and
render a generic "Other documents" section that iterates over otherDocuments so
any non-hard-coded BindingDocument types are visible; update the rendering logic
that currently references idDocuments/selfDocuments/photoDocuments (and the
existing social_connection section) to include this new otherDocuments section
and ensure it uses the same item component/display used for the specific
buckets.
In `@platforms/enotary/src/lib/server/evault.ts`:
- Around line 167-176: The pagination loop in evault.ts (do...while using
afterCursor and res.bindingDocuments.pageInfo.endCursor) can infinite-loop if
endCursor doesn't advance; add a non-advancing cursor guard by tracking the
previous cursor (e.g., prevCursor) and breaking the loop if endCursor equals
prevCursor (or null) or after a safe max-iteration threshold; update afterCursor
only when endCursor differs and ensure hasNextPage is still respected before
continuing.
---
Outside diff comments:
In `@platforms/enotary/src/lib/server/evault.ts`:
- Around line 206-220: The current Promise.all over socialCandidates in the
socialConnections creation (code around socialConnections and socialCandidates)
causes an unbounded fan-out to resolveDisplayNameForEName (and its underlying
resolveEVaultUrl/profile calls); change this to a bounded approach by replacing
Promise.all with either a sequential loop (for..of awaiting each
resolveDisplayNameForEName) or use a concurrency limiter (e.g., p-limit) to cap
concurrent calls (e.g., 5-10). Update the logic that builds socialConnections to
collect results incrementally, preserve the same null-check behavior for missing
otherPartyEName, and ensure errors remain handled per-item (catch per call and
fallback to otherPartyEName).
---
Duplicate comments:
In `@infrastructure/control-panel/src/lib/server/evault.ts`:
- Around line 77-99: getPlatformToken currently caches this.platformToken
indefinitely causing stale credentials; update getPlatformToken to track token
expiry and refresh when expired: store alongside this.platformToken a timestamp
field (e.g., platformTokenExpiry) and when calling getPlatformToken check that
the token exists and Date.now() < platformTokenExpiry before returning; when
fetching a new token, set platformTokenExpiry from an expires/expiry/ttl field
on PlatformCertificationResponse if present, otherwise set a safe TTL (e.g., now
+ configured duration) so the token is re-fetched when expired; keep the
function name getPlatformToken and the this.platformToken field references to
locate where to add the expiry logic and update callers to only use
getPlatformToken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afeab7d9-b434-4a45-89fa-260809c04c43
📒 Files selected for processing (5)
infrastructure/control-panel/config/admin-enames.jsoninfrastructure/control-panel/src/lib/server/evault.tsinfrastructure/control-panel/src/routes/evaults/[evaultId]/+page.sveltepackages/types/src/binding-document.tsplatforms/enotary/src/lib/server/evault.ts
✅ Files skipped from review due to trivial changes (1)
- packages/types/src/binding-document.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/control-panel/config/admin-enames.json
Comment on lines
+64
to
+67
| private getRegistryUrl(): string { | ||
| const registryUrl = PUBLIC_REGISTRY_URL || 'https://registry.w3ds.metastate.foundation'; | ||
| return registryUrl; | ||
| } |
Contributor
There was a problem hiding this comment.
Use the same registry fallback as the existing control-panel APIs.
getRegistryUrl() falls back to https://registry.w3ds.metastate.foundation, but infrastructure/control-panel/src/routes/api/evaults/+server.ts:145-166 still falls back to https://registry.staging.metastate.foundation. If PUBLIC_REGISTRY_URL is unset, the binding-documents tab will resolve against a different registry than the rest of the eVault page and can return mismatched or empty data. Please share one fallback/helper here so these paths cannot drift again.
🔧 Minimal alignment
private getRegistryUrl(): string {
- const registryUrl = PUBLIC_REGISTRY_URL || 'https://registry.w3ds.metastate.foundation';
+ const registryUrl = PUBLIC_REGISTRY_URL || 'https://registry.staging.metastate.foundation';
return registryUrl;
}📝 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.
Suggested change
| private getRegistryUrl(): string { | |
| const registryUrl = PUBLIC_REGISTRY_URL || 'https://registry.w3ds.metastate.foundation'; | |
| return registryUrl; | |
| } | |
| private getRegistryUrl(): string { | |
| const registryUrl = PUBLIC_REGISTRY_URL || 'https://registry.staging.metastate.foundation'; | |
| return registryUrl; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/src/lib/server/evault.ts` around lines 64 - 67,
getRegistryUrl() currently hardcodes a different fallback than the control-panel
API, causing inconsistent registry resolution; create a single shared constant
or helper (e.g., export const DEFAULT_REGISTRY_URL or function
getPublicRegistryUrl()) that returns PUBLIC_REGISTRY_URL ||
'https://registry.staging.metastate.foundation' and replace the hardcoded
fallback in the getRegistryUrl method and the other evaults API code (the
handler currently using 'https://registry.staging.metastate.foundation') to
import and use that shared symbol so both places always resolve to the same
fallback.
Comment on lines
+198
to
+205
| const { subject, type, data, signatures } = parsed; | ||
| if ( | ||
| typeof subject !== 'string' || | ||
| typeof type !== 'string' || | ||
| typeof data !== 'object' || | ||
| data === null || | ||
| !Array.isArray(signatures) | ||
| ) { |
Contributor
There was a problem hiding this comment.
Reject malformed signatures entries before reading signer.
Array.isArray(signatures) still allows payloads like [null] or [{ signer: 1 }]. Line 225 then dereferences signature.signer, so one malformed stored document can fail the entire binding-documents response instead of being skipped.
🛡️ Validate the minimum signature shape up front
.map((edge) => {
const parsed = edge.node.parsed;
if (!parsed || typeof parsed !== 'object') return null;
const { subject, type, data, signatures } = parsed;
+ const hasValidSignatures =
+ Array.isArray(signatures) &&
+ signatures.every(
+ (signature) =>
+ typeof signature === 'object' &&
+ signature !== null &&
+ typeof (signature as { signer?: unknown }).signer === 'string'
+ );
if (
typeof subject !== 'string' ||
typeof type !== 'string' ||
typeof data !== 'object' ||
data === null ||
- !Array.isArray(signatures)
+ !hasValidSignatures
) {
return null;
}Also applies to: 225-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/src/lib/server/evault.ts` around lines 198 -
205, The signatures array is only checked by Array.isArray and can contain nulls
or malformed entries, which leads to dereferencing signature.signer later (see
parsed destructure and the signature access around the signature.signer usage at
lines ~225-227); update the reader in evault.ts to validate each signature entry
up front by ensuring every element is a non-null object with a valid signer
(e.g., typeof entry === 'object' && entry !== null && typeof entry.signer ===
'string') and either filter out/skip invalid entries or reject the payload
before attempting to read signature.signer, so malformed stored documents cannot
crash the binding-documents response.
Comment on lines
+98
to
+100
| let idDocuments = $derived(documents.filter((doc) => doc.type === 'id_document')); | ||
| let selfDocuments = $derived(documents.filter((doc) => doc.type === 'self')); | ||
| let photoDocuments = $derived(documents.filter((doc) => doc.type === 'photograph')); |
Contributor
There was a problem hiding this comment.
Don't silently drop binding-document types outside the four hard-coded sections.
This tab renders photograph, id_document, self, and social_connection only. Any other BindingDocument returned in documents is invisible, so the page falls short of issue #871's “view all binding documents” acceptance criteria. Please add a generic catch-all section for the remaining document types.
Based on learnings, binding documents are initially implemented for "dumb" read/write operations (storage and retrieval via BindingDocumentService), so the UI should not assume the dataset is limited to these buckets.
Also applies to: 236-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infrastructure/control-panel/src/routes/evaults/`[evaultId]/+page.svelte
around lines 98 - 100, The current code derives only idDocuments, selfDocuments,
and photoDocuments from documents and thus drops any other BindingDocument
types; add a catch-all derived array (e.g., otherDocuments =
$derived(documents.filter(d =>
!['photograph','id_document','self','social_connection'].includes(d.type)))) and
render a generic "Other documents" section that iterates over otherDocuments so
any non-hard-coded BindingDocument types are visible; update the rendering logic
that currently references idDocuments/selfDocuments/photoDocuments (and the
existing social_connection section) to include this new otherDocuments section
and ensure it uses the same item component/display used for the specific
buckets.
Comment on lines
+167
to
+176
| do { | ||
| const res: BindingDocumentsResponse = await client.request<BindingDocumentsResponse>( | ||
| BINDING_DOCUMENTS_QUERY, | ||
| { first: 100, after: afterCursor ?? undefined }, | ||
| ); | ||
| allEdges.push(...res.bindingDocuments.edges); | ||
| afterCursor = res.bindingDocuments.pageInfo.hasNextPage | ||
| ? res.bindingDocuments.pageInfo.endCursor | ||
| : null; | ||
| } while (afterCursor !== null); |
Contributor
There was a problem hiding this comment.
Add a non-advancing cursor guard in the pagination loop.
Line 173-176 trusts upstream endCursor progression. If hasNextPage stays true with a repeated cursor, this loop can run indefinitely on request path.
🔧 Proposed hardening
const allEdges: Array<{
node: { id: string; parsed: Record<string, unknown> | null };
}> = [];
let afterCursor: string | null = null;
+ const seenCursors = new Set<string>();
do {
const res: BindingDocumentsResponse = await client.request<BindingDocumentsResponse>(
BINDING_DOCUMENTS_QUERY,
{ first: 100, after: afterCursor ?? undefined },
);
allEdges.push(...res.bindingDocuments.edges);
- afterCursor = res.bindingDocuments.pageInfo.hasNextPage
- ? res.bindingDocuments.pageInfo.endCursor
- : null;
+ const { hasNextPage, endCursor } = res.bindingDocuments.pageInfo;
+ if (!hasNextPage) {
+ afterCursor = null;
+ continue;
+ }
+ if (!endCursor || seenCursors.has(endCursor)) {
+ throw new Error("Invalid pagination state: non-advancing endCursor");
+ }
+ seenCursors.add(endCursor);
+ afterCursor = endCursor;
} while (afterCursor !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/enotary/src/lib/server/evault.ts` around lines 167 - 176, The
pagination loop in evault.ts (do...while using afterCursor and
res.bindingDocuments.pageInfo.endCursor) can infinite-loop if endCursor doesn't
advance; add a non-advancing cursor guard by tracking the previous cursor (e.g.,
prevCursor) and breaking the loop if endCursor equals prevCursor (or null) or
after a safe max-iteration threshold; update afterCursor only when endCursor
differs and ensure hasNextPage is still respected before continuing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
adds enotary's binding doc viewer page on the control panel, also adds a common types package for enotary and control panel shared types
Issue Number
closes #871
Type of change
How the change has been tested
manual
social binding and self signing tested, docs photo id pending
Change checklist
Summary by CodeRabbit
New Features
Chores