Productize team import and lock board lifecycle with E2E#6
Conversation
📝 WalkthroughWalkthroughAdded Playwright E2E infrastructure and tests, CI and Docker publish workflows, Compose orchestration and entrypoints, multi-target Dockerfile stages (server/web/cli), CLI import summary output, web drag-and-drop utility relocation, many web/unit test additions and refactors, and new documentation and compose-based quick-start configs. Changes
Sequence Diagram(s)sequenceDiagram
participant PW as Playwright (test runner)
participant Compose as Docker Compose
participant DB as Postgres (db)
participant Server as API Server
participant Web as Frontend (Vite / Nginx)
participant Browser as Headless Browser
PW->>Compose: docker compose up -d db
Compose->>DB: start & healthcheck
DB-->>Compose: ready
PW->>Compose: run e2e/setup-backend.sh
Compose->>Server: prisma db push/seed, start server
Server->>DB: apply schema & seed
DB-->>Server: done
PW->>Compose: run e2e/setup-frontend.sh
Compose->>Web: start Vite (dev) or use nginx (prod)
Web-->>Compose: listening
PW->>Browser: navigate to baseURL
Browser->>Web: load SPA
Browser->>Server: GraphQL requests (create/update/delete/comment)
Server->>DB: CRUD operations
DB-->>Server: confirmations
Server-->>Browser: GraphQL responses
Browser-->>PW: UI updates/assertions
PW->>Compose: docker compose down --remove-orphans
Compose-->>PW: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 refactors the project's Docker configuration into multi-stage builds, introduces a Playwright E2E test suite, and adds documentation for product vision and milestones. Functional updates include an enhanced CLI import pipeline and the addition of comment deletion in the web UI. Feedback from the review highlights a duplicate "Delete" button in the issue drawer and security concerns regarding default secrets in the environment template. Further recommendations include restoring missing CLI test coverage, replacing the ripgrep dependency in test scripts for better portability, and refactoring complex shell commands in the Playwright configuration to improve maintainability.
| <button | ||
| type="button" | ||
| className="issue-comment__delete" | ||
| disabled={savingState} | ||
| onClick={() => { | ||
| if (!confirmCommentDelete()) { | ||
| return; | ||
| } | ||
|
|
||
| void onCommentDelete(activeIssue, comment.id).catch(() => undefined); | ||
| }} | ||
| > | ||
| Delete comment | ||
| </button> |
There was a problem hiding this comment.
This change introduces a second "Delete comment" button for each comment. The full file content shows another delete button at line 334 with the text "Delete". This will result in a confusing UI with duplicate buttons. Please remove one of them.
| AUTH_TOKEN=changeme-set-your-token | ||
| VIEWER_ASSERTION_SECRET=compose-viewer-secret |
There was a problem hiding this comment.
Using static default secrets like changeme-set-your-token and compose-viewer-secret in an example file that is used by default in docker-compose.yml can be a security risk if deployed accidentally without changes.
While convenient for local setup, please add comments to this file strongly recommending that users change these values for any persistent or shared environment.
AUTH_TOKEN=changeme-set-your-token # IMPORTANT: Change this for any non-local environment
VIEWER_ASSERTION_SECRET=compose-viewer-secret # IMPORTANT: Change this for any non-local environment
| teamImportDependencies.writeFile = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| await expect( | ||
| runTeamImport({ | ||
| team: 'SON', | ||
| team: 'TST', | ||
| token: 'linear-token', | ||
| }), | ||
| ).rejects.toThrow(`Team import verification failed for "SON". Export preserved at ${exportDir}.`); | ||
|
|
||
| expect(rmSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('keeps a user-provided output directory after success', async () => { | ||
| const exportDir = join(tmpdir(), 'involute-team-import-son-retained'); | ||
| const mkdtempSpy = vi.spyOn(teamImportDependencies, 'mkdtemp').mockResolvedValue(exportDir); | ||
| vi.spyOn(teamImportDependencies, 'runExport').mockResolvedValue(); | ||
| vi.spyOn(teamImportDependencies, 'runImport').mockResolvedValue(); | ||
| vi.spyOn(teamImportDependencies, 'runVerify').mockResolvedValue({ | ||
| allPassed: true, | ||
| entities: [], | ||
| }); | ||
| const rmSpy = vi.spyOn(teamImportDependencies, 'rm').mockResolvedValue(undefined); | ||
|
|
||
| const result = await runTeamImport({ | ||
| team: 'SON', | ||
| token: 'linear-token', | ||
| output: exportDir, | ||
| }); | ||
|
|
||
| expect(mkdtempSpy).not.toHaveBeenCalled(); | ||
| expect(rmSpy).not.toHaveBeenCalled(); | ||
| expect(result.exportRetained).toBe(true); | ||
| expect(result.exportDir).toBe(exportDir); | ||
| }); |
There was a problem hiding this comment.
This refactoring appears to have removed the test case keeps a user-provided output directory after success. The functionality for retaining a user-provided output directory still exists in runTeamImport, so this represents a gap in test coverage. Please consider re-introducing a test for this scenario to prevent future regressions.
| "build": "pnpm --filter @involute/shared build && pnpm exec tsc -p tsconfig.json --noEmit && pnpm exec vite build", | ||
| "typecheck": "pnpm --filter @involute/shared build && pnpm exec tsc -p tsconfig.json --noEmit", | ||
| "test": "pnpm --filter @involute/shared build && pnpm exec vitest --config vitest.config.ts --passWithNoTests", | ||
| "test": "sh -c 'pnpm --filter @involute/shared build && export NODE_OPTIONS=--max-old-space-size=8192; for file in $(rg --files src -g \"*.test.ts\" -g \"*.test.tsx\" | sort); do pnpm exec vitest --config vitest.config.ts --reporter=verbose --passWithNoTests --run \"$file\" || exit $?; done'", |
There was a problem hiding this comment.
The test script relies on rg (ripgrep), which is not a standard dependency and might not be available on all developers' machines. This could create a setup barrier for contributors.
Consider using a more universal tool like find (on Unix-like systems) or a small Node.js script to gather the test files. A Node.js script would ensure cross-platform compatibility.
| command: | ||
| `docker compose up -d db && ` + | ||
| `for attempt in $(seq 1 20); do ` + | ||
| `DATABASE_URL="${databaseUrl}" pnpm --filter @involute/server exec prisma db push --force-reset --skip-generate && break; ` + | ||
| `if [ "$attempt" = "20" ]; then exit 1; fi; ` + | ||
| `sleep 3; ` + | ||
| `done && ` + | ||
| `DATABASE_URL="${databaseUrl}" pnpm --filter @involute/server exec prisma db seed && ` + | ||
| `DATABASE_URL="${databaseUrl}" AUTH_TOKEN="${authToken}" PORT=4200 pnpm --filter @involute/server exec tsx src/index.ts`, |
There was a problem hiding this comment.
The command for starting the webServer is a very long and complex shell one-liner. This makes it difficult to read, debug, and maintain.
To improve readability, consider extracting this logic into a separate shell script (e.g., e2e/setup-e2e.sh) and call that script from here. For example:
// playwright.config.ts
{
command: './e2e/setup-e2e.sh',
// ...
}![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/App.drag-utils.test.tsx (1)
136-145:⚠️ Potential issue | 🟡 MinorTest name overclaims session persistence without asserting it.
Line 136 says the value is “persisted for the browser session,” but this test only checks query-param resolution and that
localStorageremains unset. Either add an assertion for actual session persistence, or rename the test to match what it currently verifies.✏️ Minimal fix (rename to match current assertions)
-it('uses the runtime API URL override from the query param and persists it for the browser session', () => { +it('uses the runtime API URL override from the query param for this runtime context', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/App.drag-utils.test.tsx` around lines 136 - 145, The test’s title claims the override is "persisted for the browser session" but it only verifies query-param resolution and that localStorage ('involute.graphqlUrl') is not set; update the test to accurately reflect its assertions by renaming the test string to describe that it "uses the runtime API URL override from the query param and does not persist it to localStorage", keeping the assertions against getGraphqlUrl() and getGraphqlUrlDetails() unchanged, or alternatively add an assertion that the value is saved to sessionStorage/localStorage (depending on intended behavior) and use the functions getGraphqlUrl and getGraphqlUrlDetails and the 'involute.graphqlUrl' key to implement that check.
🧹 Nitpick comments (2)
packages/cli/src/commands/import.ts (1)
227-231: Minor redundancy:exportRetainedis set twice.The spread
...summaryalready includesexportRetained: shouldRetainExportfrom line 201. Line 229 overwrites it with the identical value.♻️ Optional cleanup
return { ...summary, - exportRetained: shouldRetainExport, summaryPath, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/import.ts` around lines 227 - 231, The return object in the function returns ...summary plus exportRetained again, causing a redundant duplicate; remove the explicit exportRetained: shouldRetainExport entry in the return object (leaving the spread ...summary and summaryPath) since summary already contains exportRetained, so update the return block in import.ts (the function that builds and returns summary) to avoid the duplicate key.packages/web/src/test/app-test-helpers.tsx (1)
254-271: Consider adding TypeScript function overloads for better type inference at call sites.The runtime logic correctly handles both calling conventions, but TypeScript cannot infer the parameter types at call sites. Adding explicit overload signatures would provide better IDE autocompletion and type checking for callers.
✨ Optional: Add TypeScript overloads
+export function renderApp(): ReturnType<typeof render>; +export function renderApp(queryState: QueryState, initialEntries?: string[]): ReturnType<typeof render>; +export function renderApp( + component: ComponentType, + queryState?: QueryState, + initialEntries?: string[], +): ReturnType<typeof render>; export function renderApp( componentOrQueryState: ComponentType | QueryState = { data: boardQueryResult, loading: false, }, queryStateOrInitialEntries: QueryState | string[] = ['/'], maybeInitialEntries: string[] = ['/'], ): ReturnType<typeof render> {🤖 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 254 - 271, The renderApp helper currently accepts two union-typed parameters which prevent TypeScript from inferring correct call-site types; add explicit TypeScript overload signatures for renderApp (one overload for renderApp(component: ComponentType, queryStateOrInitialEntries?: QueryState | string[], maybeInitialEntries?: string[]) and another for renderApp(queryState: QueryState, queryStateOrInitialEntries?: string[], maybeInitialEntries?: string[])) so callers get accurate types and autocompletion, then keep the existing implementation signature and body unchanged (use the overloads above followed by the current implementation signature and logic inside the same function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 2-3: The inline comments after AUTH_TOKEN and
VIEWER_ASSERTION_SECRET are being treated as shell comments and will truncate
the values; update the .env.example so comments are on separate lines above each
variable or quote the values (e.g., wrap the assigned string in quotes) to
preserve any embedded '#' characters—apply this change for AUTH_TOKEN and
VIEWER_ASSERTION_SECRET to avoid accidental truncation and confusion.
In @.github/workflows/docker-publish.yml:
- Around line 29-37: The step-level `if` uses the `secrets` context which is not
allowed; fix by promoting the secrets to job-level environment variables (e.g.,
set env: DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }}, DOCKERHUB_TOKEN:
${{ secrets.DOCKERHUB_TOKEN }}) and then change the step `if` conditions for the
"Log in to Docker Hub" and "Build and push" steps to test those env vars (e.g.,
if: env.DOCKERHUB_USERNAME != '' && env.DOCKERHUB_TOKEN != ''), removing direct
references to `secrets` in step-level `if`.
---
Outside diff comments:
In `@packages/web/src/App.drag-utils.test.tsx`:
- Around line 136-145: The test’s title claims the override is "persisted for
the browser session" but it only verifies query-param resolution and that
localStorage ('involute.graphqlUrl') is not set; update the test to accurately
reflect its assertions by renaming the test string to describe that it "uses the
runtime API URL override from the query param and does not persist it to
localStorage", keeping the assertions against getGraphqlUrl() and
getGraphqlUrlDetails() unchanged, or alternatively add an assertion that the
value is saved to sessionStorage/localStorage (depending on intended behavior)
and use the functions getGraphqlUrl and getGraphqlUrlDetails and the
'involute.graphqlUrl' key to implement that check.
---
Nitpick comments:
In `@packages/cli/src/commands/import.ts`:
- Around line 227-231: The return object in the function returns ...summary plus
exportRetained again, causing a redundant duplicate; remove the explicit
exportRetained: shouldRetainExport entry in the return object (leaving the
spread ...summary and summaryPath) since summary already contains
exportRetained, so update the return block in import.ts (the function that
builds and returns summary) to avoid the duplicate key.
In `@packages/web/src/test/app-test-helpers.tsx`:
- Around line 254-271: The renderApp helper currently accepts two union-typed
parameters which prevent TypeScript from inferring correct call-site types; add
explicit TypeScript overload signatures for renderApp (one overload for
renderApp(component: ComponentType, queryStateOrInitialEntries?: QueryState |
string[], maybeInitialEntries?: string[]) and another for renderApp(queryState:
QueryState, queryStateOrInitialEntries?: string[], maybeInitialEntries?:
string[])) so callers get accurate types and autocompletion, then keep the
existing implementation signature and body unchanged (use the overloads above
followed by the current implementation signature and logic inside the same
function).
🪄 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: fe8a1bdb-2cfc-43c4-89b6-6a046d270afd
📒 Files selected for processing (21)
.env.example.github/workflows/docker-publish.ymlDockerfiledocker-compose.ymldocs/vision.mde2e/board-flow.spec.tse2e/global-teardown.mjse2e/setup-backend.she2e/setup-frontend.shpackages/cli/src/commands/import.test.tspackages/cli/src/commands/import.tspackages/cli/src/commands/verify.tspackages/server/docker-entrypoint.shpackages/web/package.jsonpackages/web/src/App.board-state.test.tsxpackages/web/src/App.board.test.tsxpackages/web/src/App.drag-utils.test.tsxpackages/web/src/App.routes.test.tsxpackages/web/src/components/IssueDetailDrawer.tsxpackages/web/src/test/app-test-helpers.tsxplaywright.config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/web/src/components/IssueDetailDrawer.tsx
- docs/vision.md
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/cli/src/commands/verify.ts
- packages/web/package.json
- e2e/global-teardown.mjs
- packages/web/src/App.routes.test.tsx
- packages/web/src/App.board-state.test.tsx
- playwright.config.ts
- e2e/board-flow.spec.ts
| AUTH_TOKEN=changeme-set-your-token # Change this for any non-local environment. | ||
| VIEWER_ASSERTION_SECRET=compose-viewer-secret # Change this for any non-local environment. |
There was a problem hiding this comment.
Inline comments may truncate values.
In .env files, # starts a comment. The values on lines 2-3 will be truncated to just changeme-set-your-token and compose-viewer-secret (with trailing spaces). While this happens to work here, developers copying this file may be confused about how to add their own comments.
Consider moving comments to separate lines or quoting the values if you intend to include inline documentation:
💡 Suggested fix
-AUTH_TOKEN=changeme-set-your-token # Change this for any non-local environment.
-VIEWER_ASSERTION_SECRET=compose-viewer-secret # Change this for any non-local environment.
+# Change these for any non-local environment.
+AUTH_TOKEN=changeme-set-your-token
+VIEWER_ASSERTION_SECRET=compose-viewer-secret📝 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.
| AUTH_TOKEN=changeme-set-your-token # Change this for any non-local environment. | |
| VIEWER_ASSERTION_SECRET=compose-viewer-secret # Change this for any non-local environment. | |
| # Change these for any non-local environment. | |
| AUTH_TOKEN=changeme-set-your-token | |
| VIEWER_ASSERTION_SECRET=compose-viewer-secret |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 2-2: [UnorderedKey] The AUTH_TOKEN key should go before the DATABASE_URL key
(UnorderedKey)
[warning] 2-2: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 3-3: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 2 - 3, The inline comments after AUTH_TOKEN and
VIEWER_ASSERTION_SECRET are being treated as shell comments and will truncate
the values; update the .env.example so comments are on separate lines above each
variable or quote the values (e.g., wrap the assigned string in quotes) to
preserve any embedded '#' characters—apply this change for AUTH_TOKEN and
VIEWER_ASSERTION_SECRET to avoid accidental truncation and confusion.
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 43-51: The Dockerfile currently sets ARG/ENV
VITE_INVOLUTE_GRAPHQL_URL with a default of /graphql which breaks production;
remove the default (do not set ENV fallback) or replace it with an explicit
placeholder like http://BACKEND_URL:4200/graphql so builds fail/require an
explicit --build-arg; update the comment or README accordingly and ensure
consumers (apollo.tsx / any client code that reads VITE_INVOLUTE_GRAPHQL_URL)
get a fully-qualified URL at build time and nginx.conf remains unchanged (since
it does not proxy /graphql).
🪄 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: 9b5116f4-694a-4f57-b246-77188decc479
📒 Files selected for processing (2)
.github/workflows/docker-publish.ymlDockerfile
| ARG VITE_INVOLUTE_GRAPHQL_URL | ||
| ENV VITE_INVOLUTE_GRAPHQL_URL=${VITE_INVOLUTE_GRAPHQL_URL:-/graphql} | ||
|
|
||
| RUN pnpm --filter @involute/web build | ||
|
|
||
| FROM nginx:1.27-alpine AS web | ||
|
|
||
| COPY packages/web/nginx.conf /etc/nginx/conf.d/default.conf | ||
| COPY --from=web-build /app/packages/web/dist /usr/share/nginx/html |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nginx.conf lacks /graphql proxy
cat packages/web/nginx.confRepository: fakechris/Involute
Length of output: 213
🏁 Script executed:
#!/bin/bash
# Check Dockerfile context around lines 43-51 to understand build configuration
head -60 Dockerfile | tail -30Repository: fakechris/Involute
Length of output: 746
🏁 Script executed:
#!/bin/bash
# Check if there's any docker-compose or backend service configuration that defines the server
find . -name "docker-compose*" -o -name ".env*" | head -10Repository: fakechris/Involute
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Search for any usage or documentation of VITE_INVOLUTE_GRAPHQL_URL
rg "VITE_INVOLUTE_GRAPHQL_URL" -t ts -t tsx -t js -t jsonRepository: fakechris/Involute
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check docker-compose.yml for backend service configuration
cat docker-compose.ymlRepository: fakechris/Involute
Length of output: 2931
🏁 Script executed:
#!/bin/bash
# Search for VITE_INVOLUTE_GRAPHQL_URL usage in codebase
rg "VITE_INVOLUTE_GRAPHQL_URL"Repository: fakechris/Involute
Length of output: 1191
🏁 Script executed:
#!/bin/bash
# Check .env.example for how VITE_INVOLUTE_GRAPHQL_URL is documented
cat .env.exampleRepository: fakechris/Involute
Length of output: 324
Dockerfile default VITE_INVOLUTE_GRAPHQL_URL=/graphql breaks production image—requires explicit build-time configuration.
The web-build stage defaults VITE_INVOLUTE_GRAPHQL_URL to /graphql, but per the README, the published involute-web image requires this to be set to a full backend URL at build time (e.g., http://backend:4200/graphql). The relative path default will fail in production because:
- Nginx.conf only serves static files with
try_files $uri $uri/ /index.html—no proxy for/graphql - Browser requests to
/graphqlwill 404 or incorrectly serveindex.html - The apollo.tsx client uses the env var as-is, expecting a valid endpoint
Either remove the default to force explicit --build-arg configuration, or change it to a clear placeholder (e.g., http://BACKEND_URL:4200/graphql) with documentation that it must be overridden at build time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 43 - 51, The Dockerfile currently sets ARG/ENV
VITE_INVOLUTE_GRAPHQL_URL with a default of /graphql which breaks production;
remove the default (do not set ENV fallback) or replace it with an explicit
placeholder like http://BACKEND_URL:4200/graphql so builds fail/require an
explicit --build-arg; update the comment or README accordingly and ensure
consumers (apollo.tsx / any client code that reads VITE_INVOLUTE_GRAPHQL_URL)
get a fully-qualified URL at build time and nginx.conf remains unchanged (since
it does not proxy /graphql).
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Bug Fixes
Chores