Skip to content

Fix code-review correctness and lifecycle issues - #3

Merged
aaltshuler merged 1 commit into
mainfrom
codex/fix-code-review-findings
Aug 17, 2026
Merged

Fix code-review correctness and lifecycle issues#3
aaltshuler merged 1 commit into
mainfrom
codex/fix-code-review-findings

Conversation

@aaltshuler

@aaltshuler aaltshuler commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Close the four P1 and fifteen P2 findings from the full repository review across core, Cosmos, data, Omnigraph, React, and the demo.
  • Enforce reserved-ID and columnar invariants, preserve engine revision/cluster semantics, snapshot deferred Cosmos buffers, and make the default worker asset resolve in workspace and published builds.
  • Harden data-source cleanup, JSON-safe artifact serialization, schema parsing/codegen, dangerous column handling, and loader option validation.
  • Fix unavailable navigation feedback, tooltip target transitions, minimap pointer ownership, label keyboard/context-menu accessibility, and stale demo stream/CSV races.
  • Add focused regression coverage and workspace/published worker smoke checks.

Why

The review found several ways malformed or delayed inputs could cross internal namespace boundaries, diverge between main and worker execution, mutate queued engine commits, leak asynchronous sources, create lossy artifacts, or leave UI state stale and inaccessible. The fixes validate at admission boundaries, preserve primary failures during cleanup, and make asynchronous/UI ownership explicit.

Impact

Invalid input is now rejected consistently before it can corrupt scene state or serialized artifacts. Deferred engine work owns its buffers, async sources close reliably, generated types match runtime identity rules, and keyboard/pointer interactions follow the same host callback contracts as their primary paths.

Validation

  • pnpm check
    • Core: 1,256 tests
    • Data: 74 tests
    • Omnigraph: 134 tests
    • Engine Cosmos: 83 tests
    • React: 238 tests
    • Performance gate: 23 tests
  • pnpm run build
  • pnpm run demo:build including workspace worker-asset smoke
  • pnpm run smoke including published-package import/type/tree-shaking and self-contained worker checks
  • pnpm --filter orbit-demo e2e: 45 passed, 1 intentionally skipped
  • git diff --check

Greptile Summary

The PR applies broad correctness and lifecycle fixes across graph admission, worker packaging, deferred engine commits, data preparation, Omnigraph parsing, and React interactions.

  • Enforces reserved identifier, columnar, serialization, and loader-option invariants.
  • Makes deferred buffers and asynchronous data-source lifecycles explicitly owned.
  • Repairs worker asset resolution for workspace and published builds.
  • Improves UI interaction ownership, keyboard accessibility, and stale-operation cancellation.
  • Adds focused unit, smoke, and end-to-end regression coverage.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code-triggered defect remaining after review.

The changed validation, ownership, cleanup, worker packaging, parser, and interaction paths preserve their documented contracts under the reachable callers and states examined.

Important Files Changed

Filename Overview
packages/core/src/ingestion.ts Aligns streaming admission with reserved-ID namespace rules and updates diagnostics for rejected rows.
packages/core/src/workerAsset.ts Centralizes default module-worker construction around an asset path valid in workspace and published layouts.
packages/engine-cosmos/src/CosmosEngine.ts Snapshots mutable deferred commit payloads and preserves independent queued channels across initialization and restoration.
packages/data/src/sources.ts Adds single-close iterator ownership and reliable teardown on early exit and read failure.
packages/data/src/artifact.ts Rejects lossy or non-JSON-safe prepared artifacts before serialization.
packages/omnigraph/src/pgSchema.ts Reworks schema parsing to follow whitespace-tolerant token boundaries and preserve adjacent or multiline declarations.
packages/react/src/LabelLayer.tsx Adds keyboard activation and context-menu accessibility while preserving custom-renderer focus semantics.
apps/demo/src/App.tsx Uses operation epochs and cancellation guards to prevent stale CSV and streaming completions from restoring obsolete UI state.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[External graph and tabular inputs] --> B[Admission and structural validation]
  B --> C[Prepared or accepted graph]
  C --> D[Main or worker execution]
  D --> E[Engine commit queue]
  E --> F[Rendered graph]
  G[Async mode and source lifecycle] --> B
  H[React pointer and keyboard interactions] --> F
  I[View state and Omnigraph schema] --> C
Loading

Reviews (1): Last reviewed commit: "fix reviewed correctness and lifecycle i..." | Re-trigger Greptile

@aaltshuler
aaltshuler marked this pull request as ready for review August 17, 2026 22:35
@aaltshuler
aaltshuler merged commit 609ca76 into main Aug 17, 2026
5 checks passed
@aaltshuler
aaltshuler deleted the codex/fix-code-review-findings branch August 17, 2026 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant