Conversation
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…label info Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…Id to sortable data - Add touch-action: none to .issue-card CSS to prevent browser gesture hijack - Include stateId in useSortable data so card-to-card drops resolve correctly - Add IssueCard component tests verifying useSortable data shape Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…n, add fields to ISSUE_UPDATE_MUTATION, empty label picker message - Make mergeIssueWithPreservedComments null-safe using ?? fallback for comments and children fields when mutation response omits them - Add comments and children fields to ISSUE_UPDATE_MUTATION response in queries.ts for future consistency - Add 'No labels available' empty-state message in IssueDetailDrawer label picker section - Add 3 tests: undefined comments/children merge safety, empty labels UI Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…an DnD Add MouseSensor and TouchSensor to the useSensors call in BoardPage.tsx so that browser automation tools (which emit classic mouse events rather than pointer events) can trigger drag sessions. Reduce the activation distance constraint from 8 to 5 for more reliable synthetic drags. Add regression tests verifying all three sensor types are registered and that each has a distance activation constraint. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Disable file parallelism in the CLI vitest config since multiple test files (verify, issues, import) hit the same shared PostgreSQL database. Running them in parallel causes cross-test contamination where one suite's deleteMany() wipes data that another suite just imported. Additionally, harden resetVerifyImportState to clear ALL legacy mappings (not just verify-prefixed ones) before re-importing, so stale newId references left by other test suites cannot cause the import pipeline's idempotency path to skip recreating prerequisite rows. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…pdate Implement all issue-related CLI commands with GraphQL queries/mutations: - teams list, states list, labels list (table output with id/key/name) - issues list with --team filter (table: identifier, title, state, assignee) - issues show with full detail (identifier, title, desc, state, labels, assignee, comments) - issues create with --title, --team, --description - issues update with --state, --title, --assignee, --labels (resolves names to IDs) - All list/show commands support --json flag - Error handling: 'Issue not found' for nonexistent issues Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds server-side validation data setup and SON restore commands, a major CLI expansion (config, issues, comments, teams, states, labels) with pagination fixes, extensive web UI enhancements (native HTML5 drag-drop, issue creation/detail editing, comments), many new tests, and service/dev tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Web Client
participant DnD as dnd-kit
participant API as GraphQL API
participant DB as Database
User->>Browser: Drag issue card (pointer/mouse/touch)
Browser->>DnD: activate sensors
DnD->>Browser: onDragStart
Browser->>Browser: createHtml5BoardDragPayload
User->>Browser: Hover target column
Browser->>Browser: handleDragOver -> moveIssueToState (optimistic)
User->>Browser: Drop on column
Browser->>Browser: onDragEnd
alt state changed
Browser->>API: ISSUE_UPDATE_MUTATION { stateId }
API->>DB: UPDATE issue.stateId
DB-->>API: updated issue
API-->>Browser: updated issue (comments preserved)
Browser->>Browser: mergeIssueWithPreservedComments
else no-op / revert
Browser->>Browser: revert local issue state
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request expands the CLI and Web UI with issue and comment management, a backlog list view, and enhanced drag-and-drop functionality. It also introduces automated validation data setup and flexible API/auth configuration. Review feedback identifies several issues: the use of machine-specific absolute paths in server scripts, an inefficient retry loop for CLI issue lookups, a potential race condition in the Web UI title editor, and redundant logic in the CLI's team key resolution.
Comment on lines
+6
to
+7
| export const DEFAULT_SON_EXPORT_DIR = | ||
| '/Users/chris/workspace/Involute/.factory/validation/import/user-testing/tmp/import-export-flow/export'; |
There was a problem hiding this comment.
The DEFAULT_SON_EXPORT_DIR constant uses an absolute path specific to a local machine environment (/Users/chris/...). This will cause the script and its associated tests to fail on any other machine or in CI. This should be refactored to use a relative path from the project root or an environment variable.
Comment on lines
+451
to
+512
| while (true) { | ||
| const result = await client.request<{ issues: { nodes: IssueDetail[] } }>( | ||
| /* GraphQL */ ` | ||
| query CliIssueByIdentifier($filter: IssueFilter, $first: Int!) { | ||
| issues(first: $first, filter: $filter) { | ||
| nodes { | ||
| id | ||
| identifier | ||
| title | ||
| description | ||
| state { | ||
| id | ||
| name | ||
| } | ||
| labels { | ||
| nodes { | ||
| id | ||
| name | ||
| } | ||
| } | ||
| assignee { | ||
| id | ||
| name | ||
| } | ||
| comments(first: 100, orderBy: createdAt) { | ||
| nodes { | ||
| id | ||
| body | ||
| createdAt | ||
| user { | ||
| id | ||
| name | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| { | ||
| filter, | ||
| first, | ||
| }, | ||
| ); | ||
|
|
||
| const matchedIssue = result.issues.nodes.find((issue) => issue.identifier === identifier); | ||
| if (matchedIssue) { | ||
| return matchedIssue; | ||
| } | ||
|
|
||
| if (result.issues.nodes.length < first) { | ||
| return null; | ||
| } | ||
|
|
||
| if (first >= maxFirst) { | ||
| return null; | ||
| } | ||
|
|
||
| first = Math.min(first * 2, maxFirst); | ||
| } |
There was a problem hiding this comment.
The fetchIssueByIdentifier function implements a retry loop that increases the first limit on each iteration. This is inefficient because it re-fetches all previously seen issues from the server in every iteration (e.g., fetching 100, then 200, then 400). Additionally, the search is capped at maxFirst = 5000, which will fail for issues in teams with very large datasets. A more efficient approach would be to use cursor-based pagination (after) or, if the API supports it, a direct filter by identifier.
Comment on lines
+160
to
+169
| setIsEditingTitle(false); | ||
| void Promise.resolve().then(() => commitTitle()).catch(() => undefined); | ||
| }} | ||
| onKeyDown={(event) => { | ||
| if (event.key === 'Enter') { | ||
| event.preventDefault(); | ||
| setIsEditingTitle(false); | ||
| void commitTitle().catch(() => undefined); | ||
| } | ||
| }} |
There was a problem hiding this comment.
There is a potential race condition between the onBlur and onKeyDown (Enter) handlers for the title input. Both can trigger commitTitle nearly simultaneously. Since commitTitle is asynchronous and checks the current activeIssue.title, it may send two identical mutation requests if the first one hasn't completed and updated the parent state yet. Consider adding a guard to prevent concurrent saves or comparing against a 'last saved' value stored in a ref.
| const updateInput: Record<string, unknown> = {}; | ||
|
|
||
| if (input.state) { | ||
| const state = (await fetchTeamStates(identifier.split('-')[0] ?? 'INV')).find( |
There was a problem hiding this comment.
The null-coalescing operator ?? 'INV' is redundant here because split('-')[0] on a string will always return a string (potentially empty), never null or undefined. Additionally, hardcoding 'INV' as a default team key fallback makes the CLI less robust for users working with other teams.
Suggested change
| const state = (await fetchTeamStates(identifier.split('-')[0] ?? 'INV')).find( | |
| const state = (await fetchTeamStates(identifier.split('-')[0])).find( |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+443
to
449
| await persistIssueUpdate(issue, { stateId: targetStateId }, (current) => ({ | ||
| ...current, | ||
| state: targetState, | ||
| })); | ||
| } catch { | ||
| // error state already handled | ||
| } |
There was a problem hiding this comment.
🔴 Drag-and-drop mutation failure does not revert the card to its original column
handleDragOver (line 500) optimistically moves the card to the target column via setLocalIssues((currentIssues) => moveIssueToState(...)). React re-renders between the onDragOver and onDragEnd events, so by the time handleDragEnd fires at line 411, the localIssues closure already contains the card in its new column. Inside persistIssueUpdate (packages/web/src/routes/BoardPage.tsx:228), previousIssues captures this already-moved state. When the server mutation fails, setLocalIssues(previousIssues) at line 253 "reverts" to the already-moved state — the card stays in the wrong column.
Compare with onDragCancel at line 686, which correctly resets to data?.issues.nodes ?? [] (the server-authoritative data). The drag-end error path should do the same, but instead it captures a stale snapshot that already includes the optimistic move.
Prompt for agents
In handleDragEnd (around line 411-450 of packages/web/src/routes/BoardPage.tsx), the error recovery path inside persistIssueUpdate captures previousIssues from the stale localIssues closure, which already has the card in the target column due to handleDragOver's optimistic update.
The fix should ensure that when the drag-end mutation fails, the board reverts to the server-authoritative data (data?.issues.nodes) rather than to the stale closure value. One approach is to add a catch block in handleDragEnd that resets localIssues to data?.issues.nodes (similar to onDragCancel at line 686). For example:
try {
await persistIssueUpdate(...);
} catch {
setLocalIssues(data?.issues.nodes ?? []);
}
Alternatively, you could refactor persistIssueUpdate to accept an explicit fallback snapshot, or use a useRef to capture the pre-drag state at onDragStart time and pass it into the error recovery.
The same stale-closure issue affects handleNativeDropIssue (line 452-476) — the outer catch at line 474 uses previousIssues captured from the same render, but that should be fine there since handleNativeDropIssue sets localIssues synchronously before calling persistIssueUpdate (line 466), meaning previousIssues is captured before the optimistic move is applied in the same synchronous scope.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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)
.factory/library/user-testing.md (1)
37-37:⚠️ Potential issue | 🟡 MinorInconsistent Web UI concurrency limits: 4 vs 3.
Line 37 specifies "4 concurrent validators" for agent-browser, but line 109 says "cap Web UI validators at 3 concurrent sessions." Reconcile these values to avoid confusion for validators.
📝 Suggested fix
Either update line 37 to match line 109:
-- **agent-browser (Web):** 4 concurrent validators (~500 MB each: 300 MB Chromium + 200 MB shared dev server) +- **agent-browser (Web):** 3 concurrent validators (~500 MB each: 300 MB Chromium + 200 MB shared dev server)Or update line 109 to match line 37 if memory conditions have improved.
Also applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.factory/library/user-testing.md at line 37, Two inconsistent concurrency values exist for the Web UI: the "agent-browser (Web): 4 concurrent validators" entry and the "cap Web UI validators at 3 concurrent sessions" note; pick the correct concurrency target and make both references consistent by updating the text that mentions "agent-browser (Web): 4 concurrent validators" or the sentence that says "cap Web UI validators at 3 concurrent sessions" so both state the same number (either 3 or 4), and ensure any accompanying memory estimate (300 MB Chromium + 200 MB shared dev server) is adjusted if you change the concurrency.
♻️ Duplicate comments (1)
packages/web/src/lib/apollo.tsx (1)
216-221:⚠️ Potential issue | 🟠 MajorValidate the full GraphQL origin here, not just the hostname.
This still accepts any port and path on
localhost,127.0.0.1, or the current hostname, so?involuteApiUrl=http://localhost:9999/anythingis valid in dev. Because the Apollo auth link will attach the bearer token to whatever endpoint passes this check, a crafted dev URL can still redirect credentials to a different service on the same host. Use an exact origin/path allowlist instead.🔒 Example tightening
function isAllowedRuntimeGraphqlUrl(candidate: string): boolean { try { const url = new URL(candidate); - const allowedHosts = new Set([window.location.hostname, ...ALLOWED_RUNTIME_GRAPHQL_HOSTS]); - - return (url.protocol === 'http:' || url.protocol === 'https:') && allowedHosts.has(url.hostname); + const allowedOrigins = new Set([ + window.location.origin, + 'http://localhost:4200', + 'http://127.0.0.1:4200', + ]); + + return ( + (url.protocol === 'http:' || url.protocol === 'https:') && + allowedOrigins.has(url.origin) && + url.pathname === '/graphql' + ); } catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/apollo.tsx` around lines 216 - 221, The current isAllowedRuntimeGraphqlUrl only checks protocol and hostname which allows any port/path on allowed hosts; change it to validate the full origin/path against an explicit allowlist instead. Update isAllowedRuntimeGraphqlUrl to build the candidate origin (including protocol, hostname, optional port) and full path (e.g., url.origin + url.pathname or url.href truncated to path-level) and compare that string against a new ALLOWED_RUNTIME_GRAPHQL_ORIGINS (or ALLOWED_RUNTIME_GRAPHQL_WHITELIST) set/array rather than ALLOWED_RUNTIME_GRAPHQL_HOSTS; ensure comparisons are exact matches or strict prefix matches only for approved base-paths (no wildcard hostname-only entries), and keep references to isAllowedRuntimeGraphqlUrl, ALLOWED_RUNTIME_GRAPHQL_HOSTS, and window.location.hostname so reviewers can locate and replace the old hostname-based check.
🧹 Nitpick comments (10)
packages/cli/src/commands/comments.test.ts (1)
416-438: Brittle test pattern for missing required option.Intercepting
process.exitis fragile and can interfere with other tests if not properly restored on all code paths. Commander's error behavior can vary between versions.Consider using Commander's
exitOverride()andconfigureOutput()for cleaner error testing:💡 Alternative approach using Commander's built-in hooks
it('shows error when --body is missing', async () => { const { createProgram } = await import('../index.js'); const program = createProgram(); program.exitOverride(); program.configureOutput({ writeErr: () => {}, // suppress error output }); await expect( program.parseAsync(['node', 'involute', 'comments', 'add', fixtureIssueIdentifier], { from: 'node' }) ).rejects.toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/comments.test.ts` around lines 416 - 438, The current test brittlely monkey-patches process.exit around runCli; instead import and use the CLI program directly: call createProgram(), invoke program.exitOverride() and program.configureOutput({ writeErr: () => {} }) to suppress stderr, then call program.parseAsync([...,'comments','add', fixtureIssueIdentifier], { from: 'node' }) and assert it rejects (or throws) for the missing --body; replace the process.exit interception and runCli usage with this program.parseAsync approach (reference createProgram, program.exitOverride, program.configureOutput, and program.parseAsync).packages/server/src/schema.ts (3)
623-634: Consider validating cursor structure more defensively.
decodeCursorusesJSON.parseon user-provided base64-decoded input. A malformed or malicious cursor could throw unexpected errors. Consider wrapping in a try-catch with a user-friendly error.🛡️ Defensive cursor decoding
function decodeCursor(after: string): CursorPayload { + let parsed: Partial<CursorPayload>; + + try { - const parsed = JSON.parse(Buffer.from(after, 'base64url').toString('utf8')) as Partial<CursorPayload>; + parsed = JSON.parse(Buffer.from(after, 'base64url').toString('utf8')) as Partial<CursorPayload>; + } catch { + throw new TypeError('Invalid cursor format.'); + } if (typeof parsed.createdAt !== 'string' || typeof parsed.id !== 'string') { throw new TypeError('Invalid cursor payload.'); } return { createdAt: parsed.createdAt, id: parsed.id, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/schema.ts` around lines 623 - 634, The decodeCursor function currently calls Buffer.from(..., 'base64url') and JSON.parse directly which can throw on malformed input; wrap the base64 decode and JSON.parse in a try-catch, validate the parsed object still has string createdAt and id (as in CursorPayload), and on any error throw a clear, user-friendly TypeError (e.g., "Invalid cursor payload.") so callers get a consistent error type rather than raw JSON/Buffer exceptions; reference the decodeCursor function and CursorPayload when making the change.
603-611: Unreachable code path inbuildCommentOrderBy.Both branches of this function return the same value
[{ createdAt: 'asc' }, { id: 'asc' }]. Theelsebranch at lines 610-611 can never produce a different result, making it dead code.♻️ Simplified implementation
function buildCommentOrderBy( orderBy: CommentOrderByInput | null | undefined, ): Prisma.CommentOrderByWithRelationInput[] { - if (orderBy === undefined || orderBy === null || orderBy === 'createdAt') { - return [{ createdAt: 'asc' }, { id: 'asc' }]; - } - return [{ createdAt: 'asc' }, { id: 'asc' }]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/schema.ts` around lines 603 - 611, The function buildCommentOrderBy contains a redundant conditional that always returns the same array; simplify it by removing the if/else and simply return [{ createdAt: 'asc' }, { id: 'asc' }]; update the implementation of buildCommentOrderBy to unconditionally return that value (keeping the same return type Prisma.CommentOrderByWithRelationInput[]) and remove the dead conditional branches.
489-504:childrenresolver returnsIssueConnectionwithpageInfobut lacks pagination arguments.The resolver doesn't accept
first/afterarguments but returns a connection type withpageInfo. This is inconsistent with theissuesandcommentsfields. If parent issues can have many children, this could become a data retrieval concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/schema.ts` around lines 489 - 504, The children resolver on IssueParent returns an IssueConnection with pageInfo but doesn't accept pagination args; update the children resolver signature to accept (parent: IssueParent, args: { first?: number; after?: string }, context: GraphQLContext) and implement cursor-based pagination using context.prisma.issue.findMany: apply orderBy [{ createdAt: 'desc' }, { id: 'desc' }], set take = (first ?? DEFAULT) + 1 to detect hasNextPage, use cursor:{ id: after } (and skip: 1) when after is provided, trim the extra record from nodes before returning, and call buildPageInfo(nodes, hasNext) so IssueParent.children behaves consistently with issues and comments pagination.packages/cli/src/commands/issues.test.ts (3)
340-370: DuplicateseedTestDatafunction with comments.test.ts.The seeding logic is nearly identical to
seedTestDataincomments.test.ts. Consider extracting to a shared test fixture module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/issues.test.ts` around lines 340 - 370, The test file defines a duplicate seedTestData function already present in comments.test.ts; extract the shared seeding logic into a single test fixture module (e.g., tests/fixtures/seedTestData.ts) and have both issues.test.ts and comments.test.ts import and call that exported seedTestData function. Move the implementation (team creation, workflowState loop, issueLabel loop, and user creation) into the new module and replace the local seedTestData definition in issues.test.ts with an import of the shared function to avoid duplication.
390-429: DuplicaterunClihelper function with comments.test.ts.This test harness function is duplicated verbatim in
comments.test.ts(lines 473-512). Extract to a shared test utility to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/issues.test.ts` around lines 390 - 429, The runCli helper is duplicated; extract the function into a shared test utility module (e.g., export async function runCli(args: string[], homeDir: string): Promise<{ exitCode:number; stderr:string; stdout:string }>) and update both tests that currently define runCli (the one in this file and the one in comments.test.ts) to import and use that exported runCli instead of the inline copy; keep the function signature and behavior identical, export it from the new utility, and replace the duplicated definitions with an import statement in each test file.
9-9: Importing from dist folder is fragile.
import { createIssue } from '../../../server/dist/issue-service.js'bypasses the server package's public API and depends on build artifacts being present. This couples test execution to the build state and creates confusing failures if dist is stale or missing.Refactor to export
createIssuethrough the server's main entry point or create a dedicated test utilities package, allowing imports likeimport { createIssue } from '@involute/server'or a test utilities export.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/issues.test.ts` at line 9, The test imports createIssue directly from the server's build output which is brittle; update imports so tests use the server package public API (e.g., import { createIssue } from '@involute/server') or add a dedicated test-utils export that re-exports createIssue from the server source, then update packages/cli/src/commands/issues.test.ts to import from that package entry instead of '../../../server/dist/issue-service.js' and add the corresponding export (createIssue) to the server's main entrypoint or test utilities module so the symbol is available at runtime without relying on dist artifacts.packages/cli/src/index.ts (2)
874-876:resolveIssueIdfetches all comments just to retrieve the issue ID.The function calls
fetchIssueCommentswhich retrieves and paginates through all comments, when only the issue ID is needed. Consider a lighter-weight resolver for just the issue ID.💡 Potential optimization
async function resolveIssueId(issueIdOrIdentifier: string): Promise<string> { - return (await fetchIssueComments(issueIdOrIdentifier)).id; + const client = await createConfiguredGraphQLClient(); + const result = await client.request<{ issue: { id: string } | null }>( + /* GraphQL */ ` + query CliResolveIssueId($id: String!) { + issue(id: $id) { + id + } + } + `, + { id: issueIdOrIdentifier }, + ); + + if (!result.issue) { + throw new CliError(`Issue not found: ${issueIdOrIdentifier}`); + } + + return result.issue.id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/index.ts` around lines 874 - 876, resolveIssueId currently calls fetchIssueComments which paginates through all comments just to obtain the issue ID; replace that with a lightweight call that fetches only issue metadata (e.g., create/use a fetchIssue or fetchIssueMetadata function that returns {id, ...} or adapt the existing API wrapper to request only the issue resource) and update resolveIssueId to use that new/lightweight function instead of fetchIssueComments; locate resolveIssueId and the fetchIssueComments usage to change the call and ensure the new helper returns the same ID shape so callers remain unchanged.
767-767: Remove unused constantUUID_PATTERN.The regex constant defined at line 767 is never used in the codebase and should be removed to eliminate dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/index.ts` at line 767, Remove the unused constant UUID_PATTERN: delete the const declaration for UUID_PATTERN from the code (and any related unused references) ensuring no other parts of the code reference UUID_PATTERN afterwards; run the TypeScript compiler and tests to confirm there are no remaining references or type errors after removal.packages/web/src/test/app-test-helpers.tsx (1)
94-106: Consider extracting the duplicateduseMutationimplementation.The default
useMutationimplementation (lines 98-106) duplicates the logic in the hoisted mock (lines 34-42). This is necessary becausemockReset()clears the implementation, but extracting it to a shared factory could reduce duplication.♻️ Optional refactor to extract shared implementation
+const createDefaultUseMutationImpl = () => (document: unknown) => { + const source = String(document); + if (source.includes('mutation CommentCreate')) { + return [vi.fn().mockResolvedValue({ data: { commentCreate: { success: true, comment: null } } })]; + } + return [vi.fn()]; +}; + const hoistedApolloMocks = vi.hoisted<ApolloMockSet>(() => ({ useQuery: vi.fn(), - useMutation: vi.fn((document) => { - const source = String(document); - - if (source.includes('mutation CommentCreate')) { - return [vi.fn().mockResolvedValue({ data: { commentCreate: { success: true, comment: null } } })]; - } - - return [vi.fn()]; - }), + useMutation: vi.fn(createDefaultUseMutationImpl()), }));Then in
beforeEach:- apolloMocks.useMutation.mockImplementation((document) => { - const source = String(document); - - if (source.includes('mutation CommentCreate')) { - return [vi.fn().mockResolvedValue({ data: { commentCreate: { success: true, comment: null } } })]; - } - - return [vi.fn()]; - }); + apolloMocks.useMutation.mockImplementation(createDefaultUseMutationImpl());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/test/app-test-helpers.tsx` around lines 94 - 106, The duplicated default implementation for apolloMocks.useMutation should be extracted into a shared factory to avoid repetition: create a helper function (e.g., createUseMutationMock or useMutationDefaultImpl) that takes the mutation document, converts it to string, checks for 'mutation CommentCreate' and returns the same mocked tuple used today, and then replace both the hoisted mock and the after-reset setup to call this factory via apolloMocks.useMutation.mockImplementation(createUseMutationMock). Update any references to apolloMocks.useMutation.mockReset() to reapply mockImplementation(createUseMutationMock) after reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.factory/validation/import/scrutiny/synthesis.json:
- Line 32: The previousRound field is self-referencing the same file
(".factory/validation/import/scrutiny/synthesis.json"); update the previousRound
value to point to the correct round-2 artifact (e.g.,
".factory/validation/import/scrutiny/synthesis-round-2.json" or the actual
round-2 filename) so that the previousRound field references the distinct prior
synthesis artifact rather than itself; locate the previousRound key in the
synthesis.json metadata and replace its value accordingly.
- Around line 4-9: The synthesis file has inconsistent results: "status": "pass"
contradicts validatorsRun.test.passed: false with exitCode: 1; update the JSON
so overall status reflects the failing test by setting "status" to "fail" (or
set validatorsRun.test.passed to true only if exitCode is 0), or if the failure
is acceptable add a "comment" field explaining why the test failure doesn't
affect milestone status; ensure you update the "status" and/or
validatorsRun.test.passed fields consistently with validatorsRun.test.exitCode
and include the explanatory "comment" if you choose to keep "status": "pass".
In `@packages/web/src/components/IssueDetailDrawer.tsx`:
- Around line 214-217: Handlers like the onChange calling setSelectedStateId and
void onStateChange(...) are doing optimistic updates without rollback; capture
the previous value (e.g., const prev = selectedStateId), set the optimistic
value via setSelectedStateId, then await onStateChange(activeIssue,
nextStateId).catch(() => { setSelectedStateId(prev); }); Apply the same pattern
for label and assignee handlers using selectedLabelIds/setSelectedLabelIds with
onLabelChange and selectedAssigneeId/setSelectedAssigneeId with onAssigneeChange
so any save failure reverts the UI to the previous value.
- Around line 128-139: handleCommentSubmit currently awaits onCommentCreate but
does not catch rejections, causing unhandled promise rejections; wrap the await
call in a try/catch inside handleCommentSubmit, only clear setCommentBody('') on
success, and surface failures (e.g., process error via existing UI error handler
or console/processLogger) so the rejection is contained. Do the same fix for the
other async submit handler that also calls onCommentCreate so all form submit
paths handle promise rejections.
In `@packages/web/src/lib/apollo.tsx`:
- Around line 35-43: The code reads configuredToken from window.localStorage via
LOCAL_STORAGE_AUTH_KEYS and uses it verbatim; change the logic in the routine
that checks LOCAL_STORAGE_AUTH_KEYS so you trim() the retrieved value
(configuredToken) and treat an all-whitespace value as absent before returning {
token: configuredToken, source: 'localStorage' }. Update the conditional that
currently does if (configuredToken) to use the trimmed value (e.g., const token
= configuredToken?.trim()) and only return when token is non-empty, mirroring
the GraphQL URL handling.
---
Outside diff comments:
In @.factory/library/user-testing.md:
- Line 37: Two inconsistent concurrency values exist for the Web UI: the
"agent-browser (Web): 4 concurrent validators" entry and the "cap Web UI
validators at 3 concurrent sessions" note; pick the correct concurrency target
and make both references consistent by updating the text that mentions
"agent-browser (Web): 4 concurrent validators" or the sentence that says "cap
Web UI validators at 3 concurrent sessions" so both state the same number
(either 3 or 4), and ensure any accompanying memory estimate (300 MB Chromium +
200 MB shared dev server) is adjusted if you change the concurrency.
---
Duplicate comments:
In `@packages/web/src/lib/apollo.tsx`:
- Around line 216-221: The current isAllowedRuntimeGraphqlUrl only checks
protocol and hostname which allows any port/path on allowed hosts; change it to
validate the full origin/path against an explicit allowlist instead. Update
isAllowedRuntimeGraphqlUrl to build the candidate origin (including protocol,
hostname, optional port) and full path (e.g., url.origin + url.pathname or
url.href truncated to path-level) and compare that string against a new
ALLOWED_RUNTIME_GRAPHQL_ORIGINS (or ALLOWED_RUNTIME_GRAPHQL_WHITELIST) set/array
rather than ALLOWED_RUNTIME_GRAPHQL_HOSTS; ensure comparisons are exact matches
or strict prefix matches only for approved base-paths (no wildcard hostname-only
entries), and keep references to isAllowedRuntimeGraphqlUrl,
ALLOWED_RUNTIME_GRAPHQL_HOSTS, and window.location.hostname so reviewers can
locate and replace the old hostname-based check.
---
Nitpick comments:
In `@packages/cli/src/commands/comments.test.ts`:
- Around line 416-438: The current test brittlely monkey-patches process.exit
around runCli; instead import and use the CLI program directly: call
createProgram(), invoke program.exitOverride() and program.configureOutput({
writeErr: () => {} }) to suppress stderr, then call
program.parseAsync([...,'comments','add', fixtureIssueIdentifier], { from:
'node' }) and assert it rejects (or throws) for the missing --body; replace the
process.exit interception and runCli usage with this program.parseAsync approach
(reference createProgram, program.exitOverride, program.configureOutput, and
program.parseAsync).
In `@packages/cli/src/commands/issues.test.ts`:
- Around line 340-370: The test file defines a duplicate seedTestData function
already present in comments.test.ts; extract the shared seeding logic into a
single test fixture module (e.g., tests/fixtures/seedTestData.ts) and have both
issues.test.ts and comments.test.ts import and call that exported seedTestData
function. Move the implementation (team creation, workflowState loop, issueLabel
loop, and user creation) into the new module and replace the local seedTestData
definition in issues.test.ts with an import of the shared function to avoid
duplication.
- Around line 390-429: The runCli helper is duplicated; extract the function
into a shared test utility module (e.g., export async function runCli(args:
string[], homeDir: string): Promise<{ exitCode:number; stderr:string;
stdout:string }>) and update both tests that currently define runCli (the one in
this file and the one in comments.test.ts) to import and use that exported
runCli instead of the inline copy; keep the function signature and behavior
identical, export it from the new utility, and replace the duplicated
definitions with an import statement in each test file.
- Line 9: The test imports createIssue directly from the server's build output
which is brittle; update imports so tests use the server package public API
(e.g., import { createIssue } from '@involute/server') or add a dedicated
test-utils export that re-exports createIssue from the server source, then
update packages/cli/src/commands/issues.test.ts to import from that package
entry instead of '../../../server/dist/issue-service.js' and add the
corresponding export (createIssue) to the server's main entrypoint or test
utilities module so the symbol is available at runtime without relying on dist
artifacts.
In `@packages/cli/src/index.ts`:
- Around line 874-876: resolveIssueId currently calls fetchIssueComments which
paginates through all comments just to obtain the issue ID; replace that with a
lightweight call that fetches only issue metadata (e.g., create/use a fetchIssue
or fetchIssueMetadata function that returns {id, ...} or adapt the existing API
wrapper to request only the issue resource) and update resolveIssueId to use
that new/lightweight function instead of fetchIssueComments; locate
resolveIssueId and the fetchIssueComments usage to change the call and ensure
the new helper returns the same ID shape so callers remain unchanged.
- Line 767: Remove the unused constant UUID_PATTERN: delete the const
declaration for UUID_PATTERN from the code (and any related unused references)
ensuring no other parts of the code reference UUID_PATTERN afterwards; run the
TypeScript compiler and tests to confirm there are no remaining references or
type errors after removal.
In `@packages/server/src/schema.ts`:
- Around line 623-634: The decodeCursor function currently calls
Buffer.from(..., 'base64url') and JSON.parse directly which can throw on
malformed input; wrap the base64 decode and JSON.parse in a try-catch, validate
the parsed object still has string createdAt and id (as in CursorPayload), and
on any error throw a clear, user-friendly TypeError (e.g., "Invalid cursor
payload.") so callers get a consistent error type rather than raw JSON/Buffer
exceptions; reference the decodeCursor function and CursorPayload when making
the change.
- Around line 603-611: The function buildCommentOrderBy contains a redundant
conditional that always returns the same array; simplify it by removing the
if/else and simply return [{ createdAt: 'asc' }, { id: 'asc' }]; update the
implementation of buildCommentOrderBy to unconditionally return that value
(keeping the same return type Prisma.CommentOrderByWithRelationInput[]) and
remove the dead conditional branches.
- Around line 489-504: The children resolver on IssueParent returns an
IssueConnection with pageInfo but doesn't accept pagination args; update the
children resolver signature to accept (parent: IssueParent, args: { first?:
number; after?: string }, context: GraphQLContext) and implement cursor-based
pagination using context.prisma.issue.findMany: apply orderBy [{ createdAt:
'desc' }, { id: 'desc' }], set take = (first ?? DEFAULT) + 1 to detect
hasNextPage, use cursor:{ id: after } (and skip: 1) when after is provided, trim
the extra record from nodes before returning, and call buildPageInfo(nodes,
hasNext) so IssueParent.children behaves consistently with issues and comments
pagination.
In `@packages/web/src/test/app-test-helpers.tsx`:
- Around line 94-106: The duplicated default implementation for
apolloMocks.useMutation should be extracted into a shared factory to avoid
repetition: create a helper function (e.g., createUseMutationMock or
useMutationDefaultImpl) that takes the mutation document, converts it to string,
checks for 'mutation CommentCreate' and returns the same mocked tuple used
today, and then replace both the hoisted mock and the after-reset setup to call
this factory via
apolloMocks.useMutation.mockImplementation(createUseMutationMock). Update any
references to apolloMocks.useMutation.mockReset() to reapply
mockImplementation(createUseMutationMock) after reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1abe9e8-bc87-441a-9aa2-19a5b6ecfb73
📒 Files selected for processing (27)
.factory/library/son-validation-dataset-restore.md.factory/library/user-testing.md.factory/library/web-ui-validation-data-alignment.md.factory/services.yaml.factory/validation/cli/scrutiny/reviews/cli-verify-test-isolation.json.factory/validation/cli/scrutiny/synthesis.json.factory/validation/import/scrutiny/synthesis.jsonpackages/cli/src/commands/comments.test.tspackages/cli/src/commands/issues.test.tspackages/cli/src/index.tspackages/server/src/issues-filter.test.tspackages/server/src/schema.tspackages/server/src/son-validation-restore.tspackages/server/src/validation-data-setup.test.tspackages/web/src/App.board.test.tsxpackages/web/src/App.comments.test.tsxpackages/web/src/App.drag-utils.test.tsxpackages/web/src/App.issue-create.test.tsxpackages/web/src/App.routing.test.tsxpackages/web/src/App.team-selection.test.tsxpackages/web/src/App.test.tsxpackages/web/src/components/IssueDetailDrawer.tsxpackages/web/src/lib/apollo.tsxpackages/web/src/routes/BoardPage.tsxpackages/web/src/routes/IssuePage.tsxpackages/web/src/test/app-test-helpers.tsxpackages/web/vitest.config.ts
💤 Files with no reviewable changes (1)
- packages/web/src/App.test.tsx
✅ Files skipped from review due to trivial changes (4)
- packages/web/vitest.config.ts
- .factory/validation/cli/scrutiny/reviews/cli-verify-test-isolation.json
- .factory/library/web-ui-validation-data-alignment.md
- .factory/library/son-validation-dataset-restore.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .factory/validation/cli/scrutiny/synthesis.json
- packages/server/src/issues-filter.test.ts
- packages/server/src/validation-data-setup.test.ts
- packages/server/src/son-validation-restore.ts
- packages/web/src/routes/BoardPage.tsx
Comment on lines
+4
to
+9
| "status": "pass", | ||
| "validatorsRun": { | ||
| "test": { | ||
| "passed": true, | ||
| "command": "\"/Users/chris/workspace/Involute/.factory/init.sh\" && pnpm test", | ||
| "exitCode": 0 | ||
| "passed": false, | ||
| "command": "pnpm -r test --run", | ||
| "exitCode": 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are known test failures or skip annotations that explain this discrepancy
# Look for skip/todo annotations in test files
echo "=== Checking for skipped/todo tests ==="
rg -n -C2 'skip|todo|xtest|xit' --type=ts -g '*test*' -g '*spec*' packages/ 2>/dev/null | head -50
# Check if test script has any special flags that allow failures
echo -e "\n=== Checking test scripts for failure allowances ==="
rg -n 'passWithNoTests|--no-fail' packages/*/package.jsonRepository: fakechris/Involute
Length of output: 3990
🏁 Script executed:
#!/bin/bash
# Get the full content of the synthesis.json file
echo "=== Full synthesis.json content ==="
cat ".factory/validation/import/scrutiny/synthesis.json" 2>/dev/null || echo "File not found at expected path"
# Check if there are other synthesis.json files for reference
echo -e "\n=== Looking for similar JSON files ==="
find . -name "synthesis.json" -o -name "*synthesis*" -type f 2>/dev/null | head -20
# Check for documentation or schema files
echo -e "\n=== Looking for schema or docs ==="
find . -name "*.schema.json" -o -name "*validation*" -type d 2>/dev/null | grep -E "(schema|validation|factory)" | head -10Repository: fakechris/Involute
Length of output: 1403
🏁 Script executed:
#!/bin/bash
# Look for scripts that generate or update synthesis.json
echo "=== Scripts that reference synthesis.json ==="
rg -l 'synthesis' --type=ts --type=js --type=mjs packages/ .factory/ 2>/dev/null | head -20
# Search for files in .factory that might contain generation logic
echo -e "\n=== Files in .factory/validation ==="
find .factory/validation -type f \( -name "*.ts" -o -name "*.js" -o -name "*.mjs" \) 2>/dev/null
# Look at other synthesis files to see if this pattern repeats
echo -e "\n=== Check other synthesis.json files for same pattern (test failure with pass status) ==="
for f in .factory/validation/*/scrutiny/synthesis.json; do
if [ -f "$f" ]; then
status=$(jq -r '.status' "$f" 2>/dev/null)
test_passed=$(jq -r '.validatorsRun.test.passed // "null"' "$f" 2>/dev/null)
echo "File: $f -> status: $status, test.passed: $test_passed"
fi
doneRepository: fakechris/Involute
Length of output: 609
Address the inconsistency: status: "pass" conflicts with validatorsRun.test.passed: false (exitCode: 1).
All other synthesis files with status: "pass" have test.passed: true. The test validator is failing (exitCode: 1), but the overall status is marked as passing. Either the status should reflect the failing test, or a comment field should explain why test failures do not affect the milestone status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/validation/import/scrutiny/synthesis.json around lines 4 - 9, The
synthesis file has inconsistent results: "status": "pass" contradicts
validatorsRun.test.passed: false with exitCode: 1; update the JSON so overall
status reflects the failing test by setting "status" to "fail" (or set
validatorsRun.test.passed to true only if exitCode is 0), or if the failure is
acceptable add a "comment" field explaining why the test failure doesn't affect
milestone status; ensure you update the "status" and/or
validatorsRun.test.passed fields consistently with validatorsRun.test.exitCode
and include the explanatory "comment" if you choose to keep "status": "pass".
| "suggestedGuidanceUpdates": [], | ||
| "rejectedObservations": [], | ||
| "previousRound": null | ||
| "previousRound": ".factory/validation/import/scrutiny/synthesis.json" |
There was a problem hiding this comment.
Self-referential previousRound looks incorrect.
The previousRound field points to the same file (.factory/validation/import/scrutiny/synthesis.json), which creates a self-reference. For round 3, this should presumably reference a distinct round 2 artifact, such as .factory/validation/import/scrutiny/synthesis-round-2.json or a similar naming convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.factory/validation/import/scrutiny/synthesis.json at line 32, The
previousRound field is self-referencing the same file
(".factory/validation/import/scrutiny/synthesis.json"); update the previousRound
value to point to the correct round-2 artifact (e.g.,
".factory/validation/import/scrutiny/synthesis-round-2.json" or the actual
round-2 filename) so that the previousRound field references the distinct prior
synthesis artifact rather than itself; locate the previousRound key in the
synthesis.json metadata and replace its value accordingly.
Comment on lines
+128
to
+139
| async function handleCommentSubmit(event: FormEvent<HTMLFormElement>) { | ||
| event.preventDefault(); | ||
|
|
||
| const nextBody = commentBody.trim(); | ||
|
|
||
| if (!nextBody) { | ||
| return; | ||
| } | ||
|
|
||
| await onCommentCreate(activeIssue, nextBody); | ||
| setCommentBody(''); | ||
| } |
There was a problem hiding this comment.
Handle failed comment creates inside the submit path.
Unlike the other async UI handlers, this promise is fire-and-forget at the form boundary. If onCommentCreate rejects, that turns into an unhandled promise rejection in the browser/test runner.
💡 Keep the failure inside the handler
async function handleCommentSubmit(event: FormEvent<HTMLFormElement>) {
event.preventDefault();
const nextBody = commentBody.trim();
if (!nextBody) {
return;
}
- await onCommentCreate(activeIssue, nextBody);
- setCommentBody('');
+ try {
+ await onCommentCreate(activeIssue, nextBody);
+ setCommentBody('');
+ } catch {
+ return;
+ }
}Also applies to: 314-314
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/IssueDetailDrawer.tsx` around lines 128 - 139,
handleCommentSubmit currently awaits onCommentCreate but does not catch
rejections, causing unhandled promise rejections; wrap the await call in a
try/catch inside handleCommentSubmit, only clear setCommentBody('') on success,
and surface failures (e.g., process error via existing UI error handler or
console/processLogger) so the rejection is contained. Do the same fix for the
other async submit handler that also calls onCommentCreate so all form submit
paths handle promise rejections.
Comment on lines
214
to
+217
| onChange={(event) => { | ||
| const nextStateId = event.target.value; | ||
| setSelectedStateId(nextStateId); | ||
| void onStateChange(issue, nextStateId).catch(() => undefined); | ||
| void onStateChange(activeIssue, nextStateId).catch(() => undefined); |
There was a problem hiding this comment.
Add a rollback path for these immediate-save controls.
These handlers optimistically mutate selectedStateId, selectedLabelIds, and selectedAssigneeId, but nothing restores the previous value if the save fails. When the incoming issue fields stay unchanged, the sync effects never correct the optimistic value, so the drawer can keep showing state/label/assignee changes that were never persisted.
💡 One way to keep the UI honest on failure
onChange={(event) => {
const nextStateId = event.target.value;
+ const previousStateId = selectedStateId;
setSelectedStateId(nextStateId);
- void onStateChange(activeIssue, nextStateId).catch(() => undefined);
+ void onStateChange(activeIssue, nextStateId).catch(() => {
+ setSelectedStateId(previousStateId);
+ });
}}Apply the same rollback pattern to selectedLabelIds and selectedAssigneeId.
Also applies to: 244-250, 267-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/IssueDetailDrawer.tsx` around lines 214 - 217,
Handlers like the onChange calling setSelectedStateId and void
onStateChange(...) are doing optimistic updates without rollback; capture the
previous value (e.g., const prev = selectedStateId), set the optimistic value
via setSelectedStateId, then await onStateChange(activeIssue,
nextStateId).catch(() => { setSelectedStateId(prev); }); Apply the same pattern
for label and assignee handlers using selectedLabelIds/setSelectedLabelIds with
onLabelChange and selectedAssigneeId/setSelectedAssigneeId with onAssigneeChange
so any save failure reverts the UI to the previous value.
Comment on lines
+35
to
+43
| for (const storageKey of LOCAL_STORAGE_AUTH_KEYS) { | ||
| const configuredToken = window.localStorage.getItem(storageKey); | ||
|
|
||
| if (configuredToken) { | ||
| return configuredToken; | ||
| if (configuredToken) { | ||
| return { | ||
| token: configuredToken, | ||
| source: 'localStorage', | ||
| }; | ||
| } |
There was a problem hiding this comment.
Trim stored auth tokens before treating them as configured.
Whitespace/newline-padded localStorage values are used verbatim here. That can send an invalid Bearer token and also push the bootstrap messaging down the “Authentication failed” path instead of the normal missing-token flow. Trimming first would make this behave like the GraphQL URL handling below.
💡 Small hardening diff
for (const storageKey of LOCAL_STORAGE_AUTH_KEYS) {
- const configuredToken = window.localStorage.getItem(storageKey);
+ const configuredToken = window.localStorage.getItem(storageKey)?.trim();
if (configuredToken) {
return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/lib/apollo.tsx` around lines 35 - 43, The code reads
configuredToken from window.localStorage via LOCAL_STORAGE_AUTH_KEYS and uses it
verbatim; change the logic in the routine that checks LOCAL_STORAGE_AUTH_KEYS so
you trim() the retrieved value (configuredToken) and treat an all-whitespace
value as absent before returning { token: configuredToken, source:
'localStorage' }. Update the conditional that currently does if
(configuredToken) to use the trimmed value (e.g., const token =
configuredToken?.trim()) and only return when token is non-empty, mirroring the
GraphQL URL handling.
This was referenced Apr 4, 2026
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation