test: fix component-test harness dual React instance (35 -> 9 failures) - #588
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b028e5fec9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const tail = Number.isFinite(rawTail) && rawTail > 0 ? Math.min(rawTail, 1000) : 50; | ||
| const context = buildSessionContext(entries as never, leafId, { deferThinking, deferToolResultImages, tail }); |
There was a problem hiding this comment.
For any session with more than 50 displayed entries, the normal session load now returns only this default tail, but ChatWindow only mounts the pagination sentinel when the locally loaded render window has hasMore (rendered.length > visibleCount, also 50 initially). That means a long session opens with exactly 50 messages and no sentinel, so the client never calls /context?before=... and the older history is unreachable.
Useful? React with 👍 / 👎.
| let leaf = leafId ? byId.get(leafId) : entries[entries.length - 1]; | ||
| // Pagination: `before` is the oldest entry already loaded, so the next page | ||
| // must start at its parent to avoid duplicating `before` when prepended. | ||
| if (excludeLeaf && leaf?.parentId) leaf = byId.get(leaf.parentId); |
There was a problem hiding this comment.
When the client asks for the page before the first/root entry, excludeLeaf is true but this only rewinds when parentId exists, leaving leaf on the boundary entry. In that case the helper returns the same root entry again, so pagination at the start of a branch prepends a duplicate and can keep refetching it instead of getting an empty page that signals there is no older history.
Useful? React with 👍 / 👎.
| "katex": "^0.16.47", | ||
| "mammoth": "^1.12.0", | ||
| "mermaid": "^11.16.1", | ||
| "playwright": "^1.48.0", |
There was a problem hiding this comment.
This adds a new dev dependency, but the reviewed commit does not update package-lock.json and the lock still has no root or node_modules/playwright entry. I checked the reviewed package files with npm ci --dry-run, and npm had to resolve playwright from the registry; in locked or registry-restricted installs this makes the new e2e scripts unavailable or the install fail, so the lockfile should be regenerated with this dependency.
Useful? React with 👍 / 👎.
- load react, react-dom/server, and the i18n provider through the same jiti instance the components use, and import useI18n via the @/ alias (matching component imports) so I18nContext resolves to a single module instance. - previously the test entry imported React natively while jiti loaded the .tsx tree, producing two React instances and two I18nContext modules; useContext then returned null and every component test threw 'useI18n must be used inside I18nProvider'. - suite now 591 passing / 9 failing (was 565/35). Remaining 9 are platform-conditional (symlink, Windows PATH separator) or pre-existing brittle source-structure assertions, not harness defects. - Relates to agegr#586.
b028e5f to
1e5594b
Compare
|
Branch rebuilt from main so this PR is scoped to the harness fix only (the earlier branch state accidentally carried the pagination feature from #587 as well). Codex's P1/P2 notes about the detail route (�pp/api/sessions/[id]/route.ts), lib/session-reader.ts, ChatWindow.tsx, and the playwright lockfile belong to PR #587, not this harness PR — they are addressed there (see #587). This harness PR touches no product code and no package.json. |
2 participants
Summary
Fixes the component-test harness so React component tests actually run. This is the harness half of #586 (the other half — adding CI — is still wanted).
Every component test that renders with
renderToStaticMarkup(<I18nProvider>…)was failing withuseI18n must be used inside I18nProvider, even though the provider was wrapped correctly. Root cause: the test entry importedReactnatively (import React from "react") while jiti loaded the.tsxtree, so two React instances and twoI18nContextmodules existed;useContextreturnednull. TheuseI18nimport also used a.tsxsuffix that jiti resolved to a different module cache entry than the components'@/hooks/useI18n, producing a second distinctI18nContext.Changed
components/{MermaidBlock,ChatInput,ExtensionStatusBar,ExtensionWidgets,MessageView,TurnWrittenFiles}.test.mjs: loadreact,react-dom/server, andI18nProviderthrough the same jiti instance the components use, and importuseI18nvia the@/alias (matching component imports) soI18nContextresolves to one module instance. No product code touched.Verification
npm test: 603 tests, 591 pass, 9 fail (was 565/35 before this fix on the same branch, 565/35 on a cleanmainbaseline).lib/directory-browser(symlinkEPERMon Windows, passes on Linux),lib/project-command-env(WindowsPATHseparator;vs:),components/AppShell.file-viewer-state(brittle source-comment-block assertions),hooks/useAgentSession(4 brittle internal-variable regex assertions). These pass on Linux CI or are pre-existing test debt, out of scope here.Note
This PR is scoped to the test harness only. The pagination feature work (the session-history tail/before changes that Codex reviewed on the earlier branch state) lives in PR #587; the Playwright E2E example for CI lives in the CI PR. No product code in this PR.
Relates to #586.