refactor: move table-renderer to component/, add KEEP_ALIVE E2E mode - #273
Closed
alfredo1996 wants to merge 3 commits into
Closed
refactor: move table-renderer to component/, add KEEP_ALIVE E2E mode#273alfredo1996 wants to merge 3 commits into
alfredo1996 wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefactored Changes
Sequence DiagramsequenceDiagram
participant Setup as Global Setup
participant FileFS as File System
participant Server as Next.js Server
participant Postgres as PostgreSQL<br/>Container
participant Neo4j as Neo4j<br/>Container
Setup->>FileFS: Check if KEEP_ALIVE enabled<br/>(env or file)
alt KEEP_ALIVE Enabled
Setup->>FileFS: Read SERVER_PID_FILE
Setup->>Server: Verify PID alive &<br/>port accepting
alt Server Alive
Setup->>Server: Reuse existing server
Setup->>Postgres: Verify running (reuse)
Setup->>Neo4j: Verify running (reuse)
Setup->>Postgres: Count rows in "user"<br/>(idempotent check)
alt No rows exist
Setup->>Postgres: Seed database
end
Setup->>Neo4j: Count nodes<br/>(idempotent check)
alt No nodes exist
Setup->>Neo4j: Seed with init.cypher
end
else Server Dead
Setup->>Server: Start new server
Setup->>FileFS: Save SERVER_PID_FILE
end
else KEEP_ALIVE Disabled
Setup->>Server: Start fresh server
Setup->>Postgres: Start container
Setup->>Neo4j: Start container
Setup->>Postgres: Seed database
Setup->>Neo4j: Seed with init.cypher
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/e2e/global-setup.ts`:
- Around line 139-150: The code only unlinks KEEP_ALIVE_FILE when keepAlive is
false but doesn't stop previously left running servers/containers, so add a
pre-bootstrap cleanup when keepAlive is false AND KEEP_ALIVE_FILE existed:
detect the transition (read existence of KEEP_ALIVE_FILE before removal), and if
it existed, call the existing shutdown/cleanup routine or explicitly stop
processes bound to serverPort (using the same serverPort variable) and stop any
test containers before continuing; ensure KEEP_ALIVE_FILE is still removed after
successful cleanup.
In `@app/e2e/global-teardown.ts`:
- Around line 12-26: The early return inside the KEEP_ALIVE check prevents
finalizeCoverage(...) from running for KEEP_ALIVE runs; move or call
finalizeCoverage(...) before returning (or extract it into a helper and always
invoke it) so coverage finalization always executes even when keepAlive is true;
update the KEEP_ALIVE block in global-teardown.ts (the keepAlive boolean check
and the console/logging block) to call finalizeCoverage(...) (or call a new
ensureCoverageFinalized() helper) prior to returning so finalizeCoverage(...) is
never skipped.
In `@component/src/components/composed/table-renderer.tsx`:
- Around line 4-8: The EmptyState component is being passed a className from
table-renderer.tsx but EmptyStateProps lacks that prop, causing type errors;
update the EmptyState API by adding an optional className?: string to the
EmptyStateProps type in the EmptyState component (empty-state.tsx) and ensure
the EmptyState component forwards that className to its root element (preserve
existing styling behavior), so calls from table-renderer.tsx (where EmptyState
is used) remain valid.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 365358cf-e703-4351-84a1-e54d050d3794
📒 Files selected for processing (9)
.gitignoreapp/e2e/global-setup.tsapp/e2e/global-teardown.tsapp/src/components/chart-renderer.tsxapp/src/lib/table-utils.tscomponent/src/components/composed/__tests__/table-renderer.test.tscomponent/src/components/composed/index.tscomponent/src/components/composed/table-renderer.tsxpackage.json
💤 Files with no reviewable changes (1)
- app/src/lib/table-utils.ts
Comment on lines
+139
to
+150
| const keepAlive = process.env.KEEP_ALIVE === "1"; | ||
| const serverPort = | ||
| parseInt(process.env.TEST_SERVER_PORT || "3100", 10) || 3100; | ||
|
|
||
| console.log("\n⏳ Starting test containers...\n"); | ||
| // Write/clean KEEP_ALIVE marker for teardown (env vars don't always propagate) | ||
| if (keepAlive) { | ||
| fs.writeFileSync(KEEP_ALIVE_FILE, "1"); | ||
| } else { | ||
| try { | ||
| fs.unlinkSync(KEEP_ALIVE_FILE); | ||
| } catch {} | ||
| } |
There was a problem hiding this comment.
Handle KEEP_ALIVE → normal mode transition before bootstrapping.
When a previous run used KEEP_ALIVE, normal mode (Line 139 false path) only removes .keep-alive, but it does not stop old server/containers before starting new ones. Then Line 352 can pass against the old server, even if the newly spawned server failed to bind the port. Result: tests may run against stale state and old resources can leak.
Suggested fix direction
+// Before starting new containers/server in non-KEEP_ALIVE mode,
+// explicitly clean previously kept-alive resources (PID + state file).
+if (!keepAlive) {
+ // 1) Stop prior server if SERVER_PID_FILE exists
+ // 2) Remove prior containers if STATE_FILE exists
+ // 3) Remove stale marker/pid/state files
+}Also applies to: 250-257, 309-316, 351-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/e2e/global-setup.ts` around lines 139 - 150, The code only unlinks
KEEP_ALIVE_FILE when keepAlive is false but doesn't stop previously left running
servers/containers, so add a pre-bootstrap cleanup when keepAlive is false AND
KEEP_ALIVE_FILE existed: detect the transition (read existence of
KEEP_ALIVE_FILE before removal), and if it existed, call the existing
shutdown/cleanup routine or explicitly stop processes bound to serverPort (using
the same serverPort variable) and stop any test containers before continuing;
ensure KEEP_ALIVE_FILE is still removed after successful cleanup.
Comment on lines
+12
to
+26
| // KEEP_ALIVE: leave containers + server running for the next run. | ||
| const keepAlive = | ||
| process.env.KEEP_ALIVE === "1" || fs.existsSync(KEEP_ALIVE_FILE); | ||
| if (keepAlive) { | ||
| console.log( | ||
| "\n♻️ KEEP_ALIVE=1 — skipping teardown. Server and containers stay alive.", | ||
| ); | ||
| console.log( | ||
| " Next run with KEEP_ALIVE=1 will reuse them (~2 min instead of ~25 min).", | ||
| ); | ||
| console.log( | ||
| " To stop: KEEP_ALIVE=0 npx playwright test, or kill manually.\n", | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
KEEP_ALIVE path skips coverage finalization.
Because of the early return at Lines 15-25, finalizeCoverage(...) at Lines 72-79 is never reached when KEEP_ALIVE=1. That can silently drop E2E coverage output for KEEP_ALIVE runs.
Suggested fix
if (keepAlive) {
+ if (process.env.E2E_COVERAGE) {
+ const nextcovConfig = await loadNextcovConfig(
+ path.resolve(__dirname, "..", "playwright.config.ts"),
+ );
+ await finalizeCoverage(nextcovConfig);
+ }
console.log(
"\n♻️ KEEP_ALIVE=1 — skipping teardown. Server and containers stay alive.",
);Also applies to: 72-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/e2e/global-teardown.ts` around lines 12 - 26, The early return inside the
KEEP_ALIVE check prevents finalizeCoverage(...) from running for KEEP_ALIVE
runs; move or call finalizeCoverage(...) before returning (or extract it into a
helper and always invoke it) so coverage finalization always executes even when
keepAlive is true; update the KEEP_ALIVE block in global-teardown.ts (the
keepAlive boolean check and the console/logging block) to call
finalizeCoverage(...) (or call a new ensureCoverageFinalized() helper) prior to
returning so finalizeCoverage(...) is never skipped.
Comment on lines
+4
to
+8
| import { EmptyState } from "./empty-state"; | ||
| import { DataGrid } from "./data-grid"; | ||
| import { DataGridColumnHeader } from "./data-grid-column-header"; | ||
| import { DataGridViewOptions } from "./data-grid-view-options"; | ||
| import { DataGridPagination } from "./data-grid-pagination"; |
There was a problem hiding this comment.
EmptyState API mismatch will break type-checking.
After switching to local ./empty-state, Line 298 still passes className, but that prop is not in EmptyStateProps for component/src/components/composed/empty-state.tsx.
💡 Minimal fix in this file
- return <EmptyState title={emptyMessage} className="py-6" />;
+ return <EmptyState title={emptyMessage} />;Also applies to: 295-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@component/src/components/composed/table-renderer.tsx` around lines 4 - 8, The
EmptyState component is being passed a className from table-renderer.tsx but
EmptyStateProps lacks that prop, causing type errors; update the EmptyState API
by adding an optional className?: string to the EmptyStateProps type in the
EmptyState component (empty-state.tsx) and ensure the EmptyState component
forwards that className to its root element (preserve existing styling
behavior), so calls from table-renderer.tsx (where EmptyState is used) remain
valid.
2 tasks
Closes #256 Table move: - Move TableRenderer from app/ to component/src/components/composed/ - Inline parseGroupByColumns utility, export for testing - Move test to component/__tests__/table-renderer.test.ts - Update chart-renderer.tsx import to @neoboard/components KEEP_ALIVE E2E mode: - Use Testcontainers .withReuse() for container reuse across runs - Add server PID reuse check (skip spawn if already running) - Make Neo4j and PostgreSQL seeding idempotent (skip if data exists) - Teardown skips cleanup when KEEP_ALIVE marker is present - Add `npm run test:e2e:fast` convenience script First run: ~5 min (normal). Subsequent runs: ~3 sec setup + test time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
alfredo1996
force-pushed
the
improve/charts
branch
from
March 30, 2026 20:23
b9ba65a to
5b7808d
Compare
E2E global setup/teardown is test infrastructure — not application code. Cannot be unit-tested and shouldn't count toward coverage gate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
alfredo1996
changed the base branch from
release/1.0
to
improve/charts-integration
March 30, 2026 21:16
- Add cleanupPreviousRun() to stop stale server/containers when switching from KEEP_ALIVE to normal mode, preventing resource leaks - Move finalizeCoverage() before KEEP_ALIVE early return in teardown so coverage reports are always generated regardless of mode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
Superseded by #282 (merged). |
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
2 participants
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
TableRendererfromapp/tocomponent/src/components/composed/— table now follows the same design system as all other chart components (Closes refactor: move table-renderer to component/ package (design system) #256)KEEP_ALIVE=1E2E mode using Testcontainers.withReuse()— subsequent test runs skip container/server startup (~3s vs ~5min)npm run test:e2e:fastconvenience scriptTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
test:e2e:fast) for quicker test execution by reusing server and database containers across test runs.Chores