Deep research: frontend (report view, corpus tab, notifications) + chat status tool#1836
Conversation
Backend groundwork for the deep-research frontend (the v1 feature shipped
backend-only; the completion chat message links to /research/{slug} but
nothing resolved that route, and the chat agent could start a job but not
report on one).
- research_queries.py: add researchReportBySlug(slug) resolver (creator-only
via BaseService.filter_visible; IDOR-safe null for non-owners/unknown slugs)
so the frontend can resolve the /research/{slug} route.
- research_tools.py: add check_deep_research_status chat tool (read-only) so a
follow-up like "is my research done?" returns status/progress + the report
link instead of being a dead end.
- research_reports.py: add ResearchReportService.list_recent_for_corpus read
helper (creator-only, bounded) backing the tool.
- pydantic_ai_agents.py: register the status tool alongside the kickoff tool on
the corpus chat agent; both are filtered out of the read-only research agent
via restrict_tool_names.
- tests: slug-resolver GraphQL tests + status-tool/service tests.
Completes the backend-only v1 (#1814) so users can monitor and read deep-research reports. The completion chat message linked to /research/{slug} but no route resolved it; RESEARCH_REPORT_* notifications weren't surfaced; and there was no report view or list. Frontend: - GraphQL + cache: research queries/mutations, JobStatus/ResearchReportType types, openedResearchReport reactive var, relayStylePagination keyed by the GraphQL field-arg names (corpusId/status) + researchReportBySlug keyArgs. - Routing: standalone /research/:slug resolved by CentralRouteManager Phase 1 (slug-based) with a Phase-3 canonical-redirect guard; openedResearchReport added to the routing write-discipline allowlist. - Views: ResearchReportDetail (SafeMarkdown body with footnote citations; Report/Citations/Sources/Run-details tabs; live polling + Cancel while running; citations deep-link to the cited annotation) and a corpus 'Research' tab listing reports. Secondary StartResearchModal for an explicit kickoff (primary trigger stays the chat agent). - Notifications: RESEARCH_REPORT_* wired into the notification type union + job-toast system (clickable, deep-links to /research/:slug), terminal cache updates, and a completion hook the detail view uses to refetch + stop polling. Tests: frontend unit tests (route parsing + research utils) and a Playwright component test for the detail view. Typescript compiles; unit + routing discipline tests pass.
Code Review: Deep research frontend + chat status toolOverall this is a solid, well-structured PR that follows the established patterns in the codebase. The backend additions are clean and secure, the routing integration mirrors existing entities correctly, and the test coverage is meaningful. I have a few bugs and style issues to flag. Bugs1. if (error) {
toast.error("ERROR\nCould not fetch research reports for this corpus.");
}This fires on every render while useEffect(() => {
if (error) {
toast.error("Could not fetch research reports for this corpus.");
}
}, [error]);2. navigate(`${getCorpusUrl(corpus)}?tab=research`);The Code Quality3. Hardcoded hex colors ( CLAUDE.md rule #4 (no magic numbers) applies to color tokens too. Three raw hex values appear that are not in
These should either use existing 4. Array index as React key ( {citations.map((c, i) => {
...
return href ? <RowLink key={i} ...> : <Row key={i} ...>;
})}
5. console.debug(
`[useResearchCompletionNotification] Report ${numericId} reached terminal state`
);Raw 6. let numericId: number | null = null;
try {
numericId = reportId ? getNumericIdFromGlobalId(reportId) : null;
} catch { /* ... */ }This runs on every render. Wrap it in Minor / Polish7. Redundant useEffect(() => {
if (auth_token && opened_corpus?.id) {
refetch();
}
}, [auth_token, opened_corpus?.id, refetch]);The query already uses 8. No The 9. Duplicate GraphQL selection (
Security / Correctness ✅
Test Coverage ✅Backend slug-resolver tests cover creator access, non-creator null, and unknown slug. Service tests cover ordering, creator-only scoping, corpus scoping, and limit clamping. Frontend tests cover completed, cancelled/failed states and the citations tab. The PROGRESS notification type is reserved in the enum but not handled in Summary: Two bugs to fix before merge (render-time toast, back-navigation tab param), plus hardcoded hex colors and the |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
CodeQL flagged the destructured `loading` from the GET_RESEARCH_REPORT useQuery as unused (the running/cancel states use the mutation's `cancelLoading`, not the query's). No behavior change.
Code Review — PR #1836: Deep Research Frontend + Chat Status ToolThis is a well-scoped, cleanly structured PR that completes the backend-only deep-research feature from #1814. The architecture choices (slug-based routing, polling + WebSocket hybrid, creator-only access) are sound and consistent with how Extracts and Threads are implemented. A handful of issues worth addressing before merge: Bugs1. Toast spam on render in CorpusResearchReportCards.tsx (line 302) The toast.error call sits directly in the render body. This fires on every render while error is truthy — every poll interval tick or reactive-var change produces another toast. Move the toast call inside a useEffect with error as its dependency, so it only fires when error transitions to a truthy value. 2. StartResearchModal state persists across open/close cycles prompt and title are never reset when the modal closes. A user who types a prompt, dismisses, reopens, and submits will see and submit their previous inputs. The onClose handler should reset prompt and title to empty strings. Design / Convention Issues3. Hardcoded hex colors in JobNotificationToast.tsx (lines 144–163) The RESEARCH_REPORT_* configs use #4CAF50, #F44336, #9CA3AF. New entries should mirror the RESEARCH_STATUS_COLORS constants defined in this same PR rather than re-inventing the color values. 4. Missing "Queued" filter tab in ResearchTabContent.tsx FILTER_ITEMS covers: All / Running / Completed / Failed / Cancelled — but not Queued. FILTER_TO_STATUS in CorpusResearchReportCards.tsx already maps "queued" to the backend value, and a freshly triggered job sits in QUEUED briefly. The tab should include it or explicitly fold QUEUED into "Running" with a comment explaining why. 5. variableMatcher wildcard in ResearchReportDetailTestWrapper.tsx CLAUDE.md calls out that GraphQL mocks must match variables exactly (null vs undefined matters). A wildcard matcher hides any future variable mismatch. The exact variables object is already present in the same mock — dropping variableMatcher costs nothing and makes the mock more useful as a regression guard. Minor / Non-blocking6. numericId computed inline in useResearchCompletionNotification.ts The try/catch that decodes reportId runs on every render pass. Wrapping in useMemo keyed on reportId is the pattern used elsewhere in the codebase and makes the intent clearer. 7. Duplicate selection sets in GET_RESEARCH_REPORT / RESOLVE_RESEARCH_REPORT_BY_SLUG The comment acknowledges this mirrors the extract convention (intentional). Just noting that both selections will need updating if the type grows new fields. Security / Permissions — OK
Test Coverage — Good
SummaryThe two bugs (toast spam, stale modal state) are functional regressions that should be fixed before merge. The missing "Queued" filter and the wildcard test matcher are low-cost fixes worth doing here. The rest are polish notes. Overall the implementation is solid — routing, caching, notification, and service-layer patterns all follow established conventions correctly. |
The Playwright CT (esbuild dev) bundler failed to bind the named getCorpusUrl
import in ResearchReportDetail, throwing 'getCorpusUrl is not defined' at
render so the report never mounted (all 3 component tests failed). getDocumentUrl
from the same module binds fine. Inline the corpus back-navigation URL (same
/c/{creator}/{corpus} shape) and drop the getCorpusUrl import. The 3
ResearchReportDetail component tests now pass locally.
…iables
The real cause of the Component Tests failure: MockedProvider rejects a mock
that supplies both request.variables and variableMatcher ("should contain
either variableMatcher or request.variables"), so the provider constructor
threw and the component never mounted. Keep the explicit request.variables
({ id }) form and drop the redundant variableMatcher.
Code Review: Deep-research Frontend + Chat Status ToolOverviewThis PR completes the frontend surface for the backend-only deep-research feature shipped in #1814. Well-scoped and self-contained: slug-resolver GraphQL query, chat status tool, full routing, report detail view, corpus tab, notification wiring, and tests. The architectural decisions are sound — the routing follows the existing CentralRouteManager pattern, cache key-args are correct per CLAUDE.md, and the service/permission layering is respected throughout. Bugs1. toast.error called in the render body — fires on every re-render (CorpusResearchReportCards.tsx, around line 91) The block Code Quality2. Inline corpus URL in ResearchReportDetail.handleBack — DRY violation (ResearchReportDetail.tsx, around line 488) The comment explains this was done because getCorpusUrl() tripped the CT bundler. The right fix is to address the bundler import issue rather than inlining the URL shape here — a future corpus URL scheme change will silently skip this callsite. 3. Duplicate GraphQL field selections (queries.ts) GET_RESEARCH_REPORT and RESOLVE_RESEARCH_REPORT_BY_SLUG share an identical ~45-field selection. The comment acknowledges this follows the extract pattern. A named fragment would make future field additions/removals less error-prone. Not blocking, but worth a follow-up. 4. queued status absent from the corpus Research tab filter (ResearchTabContent.tsx) FILTER_ITEMS has running, completed, failed, cancelled but not queued. FILTER_TO_STATUS in CorpusResearchReportCards does map queued, so queued reports only surface under "All". May be intentional (queued is transient), but worth a comment confirming the decision. Potential Issues5. Missing init.py for the new test directory opencontractserver/tests/research/ is new but the diff contains no init.py. Django's manage.py test discovery relies on the module hierarchy; without it these tests may be silently skipped. Confirm discovery works or add the missing init.py. 6. numericId decoded on every render in useResearchCompletionNotification The try/catch that calls getNumericIdFromGlobalId(reportId) sits at the top level of the hook (not in a useMemo), so the value is a new primitive each render. The useCallback for handleNotificationCreated depends on numericId, meaning a stale closure is possible if reportId changes between assignment and capture. Using useMemo with [reportId] dependency removes the ambiguity. SecurityAll good:
TestsGood coverage for an additive PR:
Gaps for a follow-up:
SummarySolid PR that follows existing patterns faithfully. Two items to address before merging: the render-path toast bug (#1) and verifying test discovery (#5). The DRY violation in handleBack (#2) should be fixed before it becomes a maintenance hazard. The rest are nits or follow-up candidates. |
Code Review — Deep Research Frontend + Chat Status ToolOverall this is a solid, well-scoped PR. The architecture closely mirrors existing patterns (extract routing, notification hooks, service layer), the CLAUDE.md conventions are largely followed, and the PR is appropriately additive with no dead code left behind. A few issues below, ordered by severity. Bugs / Correctness1. Side effect (toast) called directly in render body — if (error) {
toast.error("ERROR\nCould not fetch research reports for this corpus.");
}
useEffect(() => {
if (error) {
toast.error("Could not fetch research reports for this corpus.");
}
}, [error]);2. Source / citation links use
const RowLink = styled(Link)`...`;
// and use <RowLink to={href}>Code Quality3. // Corpus URL shape mirrors getCorpusUrl(); inlined here so this view
// doesn't import it (the named import tripped the CT bundler).
navigate(`/c/${corpus.creator.slug}/${corpus.slug}?tab=research`);The CLAUDE.md split-import rule (pitfall #16) applies to import { getCorpusUrl } from "../utils/navigationUtils";
// ...
navigate(getCorpusUrl(corpus) + "?tab=research");If the import truly does cause a CT issue when this view is mounted in a test, that's a separate bug to fix in the test wrapper rather than working around it in production code. 4. let numericId: number | null = null;
try {
numericId = reportId ? getNumericIdFromGlobalId(reportId) : null;
} catch { ... }This is fine functionally (the decode is cheap) but inconsistent with the hook's use of const numericId = useMemo(() => {
try {
return reportId ? getNumericIdFromGlobalId(reportId) : null;
} catch { return null; }
}, [reportId]);5. Debug logs in shipping hooks should be guarded or removed: console.debug(
`[useResearchCompletionNotification] Report ${numericId} reached terminal state`
);Test Coverage Gaps6. Missing
7. The test sets 8. No test for the Given bug #1 above, a regression test for the error state would be valuable once the toast call is moved into a Minor / Nits9.
10. console.warn("Cannot generate research report URL without slug:", report);The existing 11. Duplicate GraphQL field selections in The PR comment already acknowledges this follows the extract pattern. Worth a GQL fragment in a future cleanup pass, but not blocking. What Looks Good
Summary: Items 1 and 2 are the only issues I'd call blockers — the toast side effect will produce duplicate notifications in production, and the |
…t, hook cleanups - ResearchReportDetail: RowLink is now styled(react-router Link) with to= so internal document/citation links route client-side instead of full-reload. - CorpusResearchReportCards: move the error toast into a useEffect so it fires once per error rather than on every re-render. - useResearchCompletionNotification: memoise numericId (feeds useCallback deps) and drop a leftover console.debug. - Add ResearchReportListCard component tests (completed + running) with a documentation screenshot.
Code Review: Deep-research frontend + chat status toolOverall this is a well-structured PR that completes the backend-only deep-research feature with a coherent frontend surface. The architecture follows established patterns (CentralRouteManager, reactive vars, services layer, relay pagination). A few issues to address before merge. Bugs / Correctness1. Potential test failure in
2. The extract route handler was updated to clear 3. Inlined corpus URL in navigate(`/c/${corpus.creator.slug}/${corpus.slug}?tab=research`);The comment says this was inlined because importing Code Quality / Style4. Hardcoded hex colors violate no-magic-numbers rule
5.
6. Client-side title search is disconnected from the pagination window
7. Minor: reusing the extract search debounce constant for research search is pragmatic, but the name is misleading. Consider Security8. The new resolver uses BaseService.filter_visible(ResearchReport, info.context.user, request=info.context)
.filter(slug=slug)
.first()This follows the spirit of the services rule (it goes through the shared base service, not inline Test Coverage Gaps9. No test for the "cancel" path from the detail view
10. The list view (filter tabs, load more, empty states, search) is covered only by the unit tests in Nits
SummaryThe PR is architecturally sound and follows project conventions well (services layer, relay pagination keyArgs, route discipline test,
|
… feedback
Follow-up to the review fixes already on this branch:
- ResearchReportDetail: replace remaining hardcoded hex (#ffffff/#fff7ed/
#9a3412) with OS_LEGAL_COLORS tokens (surface/warningSurface/warningText);
stable keys (key={c.footnote} for citations, key=index+value for warnings);
Cancel button shows 'Cancelling…' and disables while cancelRequested is
pending.
- StartResearchModal: reset prompt/title on close so a reopened modal starts
blank.
- ResearchTabContent: add the 'Queued' filter tab (backend status map already
supports it; a fresh job sits in QUEUED briefly).
Typescript compiles; research component tests pass.
…h-bhtzs # Conflicts: # CHANGELOG.md
…tore getCorpusUrl helper, tokenize toast colors, reset research search on unmount
- Clear openedResearchReport(null) in thread/labelset/user route handlers (sibling of openedExtract, was omitted)
- Replace inlined corpus URL in ResearchReportDetail.handleBack with getCorpusUrl(corpus, {tab}); safer navigate('/') fallback
- JobNotificationToast: source status colors from OS_LEGAL_COLORS tokens instead of raw hex
- ResearchTabContent: reset researchSearchTerm and use DEBOUNCE.LIST_SEARCH_MS; clarify empty-state copy + add Load More in CorpusResearchReportCards
- ResearchReportListCard: console.warn on missing slug instead of silent no-op
…eenshot - New CorpusResearchReportCards.ct.tsx + wrapper: mounts the corpus Research tab list, asserts query wiring + empty state, captures a screenshot (review item 10). Populated-list assertion omitted — the component's network-only query re-fires across renders and MockedProvider can't serve it deterministically in isolation; card visuals are covered by ResearchReportListCard.ct.tsx. - ResearchReportDetail.ct.tsx: add docScreenshot for the completed-report state.
Code Review — Deep Research Frontend + Chat Status ToolOverall this is a well-structured, carefully scoped feature addition. The architecture follows existing patterns (routing discipline, reactive vars, service layer), and the IDOR protection / creator-only visibility is correctly enforced throughout. Below are findings by severity. Bugs1. Annotation typename mismatch in citation deep-links ( // detail view — builds the annotation URL param
toGlobalId("AnnotationType", annPk)But the test wrapper uses: // ResearchReportDetailTestWrapper.tsx
id: toGlobalId("ServerAnnotationType", 10),If the backend emits annotations with // prefer: look up the annotation by PK from the already-fetched list
const annGlobalId = report?.fullSourceAnnotationList?.find(
(a) => getNumericIdFromGlobalId(a.id) === annPk
)?.id;Code Quality / Convention Issues2. Double GraphQL field selection (DRY)
3. export interface GetResearchReportsInput {
status?: string; // ← looseOther interfaces in this file use the enum. Using 4. const Toolbar = styled.div`
background: white; // ← not a design tokenAll other toolbar backgrounds in this PR use 5. Magic number in back navigation ( onClick={() => setActiveTab(0)}Tab 6. Missing The diff adds two files to a new Minor / Nitpicks7. Cancelled toast uses 8. <CodeBlock>{JSON.stringify(report.toolCallLog, null, 2)}</CodeBlock>
9. completedAt: (existing) => existing ?? new Date().toISOString(),The client time may drift from server time. This is a safe fallback since the next refetch overwrites it, but a future reader might not realise it's approximate. 10. key={`${i}-${String(w)}`}Warnings on a completed report don't reorder, so this is fine in practice, but using the content itself (or a stable hash) would be more idiomatic React. Test Coverage
SummaryThe IDOR protection, service-layer routing, cache key args (CLAUDE.md #15), and routing discipline (Phase-3 guard, |
…r token
- Fix citation deep-link annotation typename mismatch in ResearchReportDetail:
build the annotation global ID from fullSourceAnnotationList (the canonical
ServerAnnotationType id the backend emits) via an annGlobalIdByPk map,
instead of reconstructing toGlobalId('AnnotationType', pk) which guessed the
wrong type name and produced deep-links resolving to the wrong entity. Drop
the now-unused toGlobalId import.
- Add a CT regression guard asserting the citation link's ?ann= param carries
the ServerAnnotationType global id.
- Type GetResearchReportsInput.status as JobStatus | undefined (was loose
string) and tighten FILTER_TO_STATUS to match.
- Use OS_LEGAL_COLORS.surface for the research Toolbar background instead of a
hardcoded 'white' literal, consistent with the other toolbars in this PR.
Code ReviewGood PR overall — the feature is well-scoped, the security model is correct, and the routing/notification patterns mirror existing conventions faithfully. A few items to address before merge: Potential Bug (needs verification)
self.assertIn("finished in 2m 5s", result)
Medium — Robustness
React.useState(() => {
openedCorpus({ id: CORPUS_ID } as any);
return null;
});
Low — Error Handling
When Nit — GraphQL variable type looseness
Nit — Unused union member
Positive observations
|
…usly ResearchReportDetailTestWrapper seeded openedResearchReport in a useEffect, which fires only after the child's first render — so ResearchReportDetail flashed its 'not found' state for one frame before the report appeared, a flakiness risk on slow CI. Switch to the synchronous useState-initializer pattern (matching CorpusResearchReportCardsTestWrapper) so the var is seeded before the first render, keeping a useEffect solely for unmount cleanup.
Code Review — Deep-research frontend + chat status toolOverall this is a well-structured, thoughtful PR. The CLAUDE.md pitfalls are handled explicitly (keyArgs field-argument names, Playwright CT split-import, async tools), the permission model is IDOR-safe, and the test coverage is solid for a first pass. A few items below worth addressing before merge. Bugs1. When the report is null or a GraphQL error occurs, the code navigates to Fix: Call 2. Dead code in The research route handler in Fix: Either set 3. React key collision risk in the Citations list (
Fix: Use the array index Consistency4. The modal builds the navigation path with a template literal directly. The same PR introduces Fix: 5. The component declares Fix: Widen to Performance6. Double-fetch on initial mount in
Fix: Add a mount-guard ref (same pattern other hooks use) or switch to Minor7. 8. Backend
Summary
|
Resolve CHANGELOG.md conflict by keeping both unreleased entries (deep-research frontend + runtime corpus categories).
…guard
- CentralRouteManager: call routeLoading(false) before navigate('/404')
in both the error and null-data branches of the research-report block.
- ResearchReportRoute: remove dead error branch (CentralRouteManager
redirects to /404 on error/null, never sets routeError for research).
- ResearchReportDetail: key citation rows by array index, not c.footnote,
to avoid React key collisions from duplicate footnotes in JSON.
- StartResearchModal: navigate via getResearchReportUrl(payload.obj).
- ResearchTabContent: widen setActiveTab prop to (number | string).
- CorpusResearchReportCards: mount-guard the refetch effect to avoid a
double fetch on initial mount; add explicit 'Could not load reports'
error render when error && no reports loaded.
- navigationUtils: drop production console.warn in getResearchReportUrl
('#' sentinel already signals the caller).
- useNotificationWebSocket: TODO(v2) on RESEARCH_REPORT_PROGRESS union member.
- CorpusResearchReportCardsTestWrapper: set addTypename:false on the custom InMemoryCache so it agrees with MockedProvider; a typename-adding cache injected __typename into the query the mock link matched against, draining the mock bucket and resolving the final fire to an error. - CorpusResearchReportCards: switch fetchPolicy to cache-and-network (nextFetchPolicy cache-first) so re-renders serve from cache instead of a network-only refetch storm; only surface the error state once the request has settled (error && !loading) so a transient retry error doesn't flash over the spinner. - ResearchReportDetail.ct: add research--report-detail--citations docScreenshot after the citations-tab assertions.
Code Review — Deep Research Frontend + Chat Status ToolOverall this is a well-executed, comprehensive PR that follows the project's conventions closely. The architecture is sound, security is handled correctly (IDOR-safe null returns, creator-only visibility via Issues below are grouped by severity. Bugs / Correctness1.
| "RESEARCH_REPORT_PROGRESS";This type is added to the union but has zero handlers in 2.
default:
return {
label: RESEARCH_STATUS.QUEUED,
color: RESEARCH_STATUS_COLORS[RESEARCH_STATUS.QUEUED],
};If the backend ever adds a new 3. Backend duration format inconsistency with frontend
f" — finished in {int(secs // 60)}m {int(secs % 60)}s"The frontend's Test Coverage4. Missing anonymous-user test for
The test covers creator-can-fetch, non-creator-gets-null, and unknown-slug-gets-null, but not the case where an unauthenticated request is made. The def test_anonymous_user_gets_error(self):
# create an anonymous-like context (no user) and verify the response
...would lock in the expected behaviour and guard against the decorator accidentally being removed. Code Style / Minor Issues5. Misleading
// cache-and-network serves from cache on re-render after the first fetch
// (avoiding a network-only refetch storm) while still revalidating.
fetchPolicy: "cache-and-network",
nextFetchPolicy: "cache-first",The comment describes 6.
The backend model caps 7. The styled-components section alone runs ~300 lines. The project already has the 8. Citation and source rows use array index as React key
return href ? <RowLink key={i} to={href}>…</RowLink> : <Row key={i}>…</Row>;Citations are immutable for a given report so this won't cause actual bugs, but using Positive Callouts
|
- Remove dead RESEARCH_REPORT_PROGRESS member from the notification type union (no v1 emitter/handler); add it when the handler is wired. - getResearchStatus dev-warns on an unrecognized JobStatus instead of silently rendering 'Queued' (null/undefined still treated as the not-set-yet case). - Align the chat status tool's duration string with the frontend formatResearchDuration helper (sub-minute reads '42s', not '0m 42s'); add a regression test. - Cap StartResearchModal's optional title input at MAX_RESEARCH_TITLE_CHARS (255, mirroring the ResearchReport.title model column) + new smoke CT. - Key citation rows off the unique footnote, not the array index. - Add anonymous-user rejection test pinning @login_required on researchReportBySlug. - Clarify the cache-and-network/cache-first nextFetchPolicy comment.
PR Review: Deep Research Frontend + Chat Status ToolOverall this is a large, well-structured feature addition. The implementation follows the project's architecture patterns closely, uses constants instead of magic numbers, and the security model (creator-only, IDOR-safe slug resolver) is correctly implemented. The positive highlights are listed at the bottom. Here are the issues I found: Bugs / Correctness1. Docstring lists
The frontend 2.
The two-level gate is: the 3. Double round-trip fetch when corpus changes in
Fix: remove Security4. No rate-limiting on
The resolver correctly returns Performance5.
When the completion notification fires 6. Duplicated ~50-line GQL field selection between
CLAUDE.md's DRY principle would suggest extracting a GQL fragment. If the codebase intentionally avoids fragments in Code Quality7.
Using the string content alone ( 8.
The list corpus selection only fetches Test Coverage9. No test for The cancel button is gated on 10.
The test comment honestly documents why the populated-card path is skipped ( Positive Highlights
Files with highest regression risk to manually verify:
|
…e change - finalize() now wraps the terminal content/status save and the source_annotations/source_documents M2M set() in a single transaction.atomic() block, so an interrupted Celery task can no longer leave a COMPLETED report whose citations have no backing provenance rows. - CentralRouteManager clears openedResearchReport in Phase 1 whenever the resolved route is no longer a research route (single source of truth), closing the stale-state gap on /research/:slug -> corpus/document routes and browser back/forward. Regression-guarded in centralRouteDiscipline.test.ts.
Code Review — PR #1836: Deep research frontend + chat status toolThis is a well-structured PR that cleanly completes the backend-only research feature from #1814. The design decisions are sound and the implementation follows project conventions. Below are findings organized by priority. 🔴 Issues worth fixing before merge1.
2. The file contains ~130 lines of styled components, the main component, four tab panels rendered inline, citation row logic, source document logic, and run details. CLAUDE.md rule 3 calls for single-responsibility modules. The tabs in particular are self-contained enough to extract ( 🟡 Potential bugs / edge cases3. Cancel button: optimistic label flips on In {report.cancelRequested ? "Cancelling…" : "Cancel"}There is a window between the button click and the server returning 4. In const nextStatus = data.status || fallbackStatus;The 5.
🟡 Performance notes6. Client-side search doesn't scale
🟢 Things done well
Minor nits
|
Collapse a source_documents.set(Document.objects.filter(...)) call onto one line — black flagged it (the only pre-commit/linter violation on HEAD; isort/flake8/pyupgrade all clean). Formatting only, no behavior change.
Code Review: Deep Research Frontend + Chat Status Tool (#1836)OverviewThis PR completes the frontend surface for the backend-only deep-research feature from #1814. It adds a Positives
Issues and Suggestions1. Polling fetches full report payload (performance)
const { data, refetch, startPolling, stopPolling } = useQuery<...>(GET_RESEARCH_REPORT, {
variables: { id: reportVar?.id ?? "" },
...
});
// ...
startPolling(RESEARCH_REPORT_POLL_INTERVAL_MS); // 5sWhile the job is running, the only data that actually changes is
2.
|
…h-bhtzs # Conflicts: # CHANGELOG.md
… guard, CHANGELOG indent - Extract the research-report update permission string into a RESEARCH_REPORT_UPDATE_PERMISSION constant so a typo can't silently disable Cancel for all users (review #2). - isTerminalResearchStatus now treats an unrecognized status as terminal so the detail view never polls indefinitely on a future backend state the frontend doesn't recognize; null/Queued/Running stay non-terminal (review #3). Added a unit-test case. - Fix a lost 2-space continuation indent in the CHANGELOG SmartLabel entry (review #5).
Code Review: Deep Research Frontend + Chat Status ToolOverviewThis PR ships the full frontend surface for the deep-research feature that was backend-only in #1814: a Positives
Issues1. Detail test wrapper mock will drain on retries (test fragility risk)File: The mock array provides exactly one response. The component uses Since all tests in Compare to 2. Duration test relies on
|
Resolve CHANGELOG.md conflict by keeping both sets of Fixed entries.
Code ReviewThis PR ships the missing frontend surface for the deep-research feature (#1814), along with two backend additions — a slug resolver and a chat status tool. The overall architecture is sound and well-aligned with project conventions. Below are findings sorted by severity. Correctness / Bugs1.
} catch (e) {
toast.error("Could not cancel the research job.");
}The thrown error 2. The backend The 3. Potential stale-slug 404 race in After This is unlikely in a single-writer setup, but worth noting. A targeted guard (e.g. a retry or a short UX Issues4. Client-side search only covers the loaded page
The empty-state copy correctly adds a hint when Minor / Style5. const isRunning = status === JobStatus.Running || status === JobStatus.Queued;The name 6. {warnings.map((w, i) => (
<WarningChip key={`${i}-${String(w)}`}>Index-based keys are fine since warnings are stable (no reorder/delete), but the full warning text ( 7. Most of it is styled-component definitions. This is fine for v1 scope, but the styled components block alone (~300 lines) could reasonably be extracted into a What's Well Done
|
- graphql-api: add the backend's transient JobStatus.CREATED to the frontend enum; it was missing, so isTerminalResearchStatus(CREATED) wrongly returned true and stopped status polling before the job left the pre-queue state - researchUtils: treat CREATED as non-terminal and display it as 'Queued' - ResearchReportDetail: rename isRunning -> isActive (it covers created/queued/ running), key warning chips by their text instead of array index, log the swallowed error in the cancel catch block - StartResearchModal: log the swallowed error in the submit catch block - tests: cover CREATED in getResearchStatus and isTerminalResearchStatus
…h-bhtzs # Conflicts: # CHANGELOG.md
PR Review: Deep Research Frontend + Chat Status ToolOverall this is a solid, well-structured PR. The code quality is high, security patterns are followed correctly, and the architecture mirrors the established conventions cleanly. A few things worth calling out. Security / Correctness
The IDOR-safe null-return for non-owners and unknown slugs is correct, and the
The Potential Polling Edge Case
if (!report?.id || !report?.status) return;Low priority given the server always returns a status, but worth noting. Minor Type Inconsistency in Test Fixtures
The fixture includes Missing Screenshot
The test calls UX Note (Known Limitation)
The empty state copy explicitly acknowledges this ("No research reports on the loaded page match your search — load more to search further"), so it is documented. Worth tracking as a v2 item (a backend search arg on the connection) if the list grows large. Things Done Well
Verdict: Approve pending the optional cleanup above. The two actionable items are the defensive |
* Address deep-research review follow-ups (#1864) Code-review follow-ups from PR #1836: - mutations.ts: narrow CancelResearchReportOutput.obj.status from bare `string` to `JobStatus | string` to match ResearchReportType.status, so callers comparing against JobStatus values get type checking. - ResearchReportDetailTestWrapper: add maxUsageCount: Infinity to the detail query mock so the view's notifyOnNetworkStatusChange refetch/poll paths can't drain the single-response bucket (mirrors the sibling CorpusResearchReportCardsTestWrapper). - research/models.py: document that ResearchReport.duration_seconds is always a computed property (never stored); add a guard test pinning the invariant the status-tool duration tests depend on. Reviewer items verified to need no change: the async catch blocks in ResearchReportDetail.tsx / StartResearchModal.tsx already log via console.error; the slug column is already indexed (unique=True + db_index); and the snake_case reactive-var locals in CorpusResearchReportCards (opened_corpus / research_search_term / auth_token) intentionally match the established sibling-component convention (CorpusExtractCards / CorpusAnalysesCards) and avoid shadowing the imported reactive vars. * Fix linting issues Narrow Optional return of duration_seconds before assertAlmostEqual to satisfy mypy arg-type check in the research report model test. * Address review: trim test-file references from duration_seconds docstring Per CLAUDE.md, docstrings should not name specific callers/tests since those references rot as the codebase evolves. Keep the invariant and the migration-to-stored-field warning, drop the named test files. --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Context
Deep research shipped in #1814 as a backend-only feature. The flow exists end-to-end on the server — a user asks the corpus chat agent to "research X", a long-running Celery job produces a cited markdown
ResearchReport, and on completion the backend fires a notification and drops a chat message linking to/research/{slug}. But the frontend was explicitly deferred to v2, so today:/research/{slug}route doesn't exist → the completion chat link 404s.RESEARCH_REPORT_*notifications are broadcast but not in the frontendNotificationTypeenum, so completion never surfaces.This PR adds the full frontend surface plus a backend status tool so a user can trigger (conversationally), monitor, and read reports.
What's in here
Backend (small, additive)
researchReportBySlug(slug)query resolver (config/graphql/research_queries.py) — creator-only viaBaseService.filter_visible, IDOR-safe null for non-owners/unknown slugs — so the frontend can resolve the existing/research/{slug}link.check_deep_research_statusread-only chat tool (opencontractserver/llms/tools/research_tools.py) backed by a newResearchReportService.list_recent_for_corpushelper, registered on the corpus chat agent next tostart_deep_research. Both are filtered out of the read-only research agent viarestrict_tool_names, so a follow-up like "is my research done?" returns status/progress + the report link.Frontend
JobStatus/ResearchReportTypetypes,openedResearchReportreactive var, andrelayStylePagination(["corpusId","status"])+researchReportBySlugcache policies (keyArgs use the GraphQL field-arg names per CLAUDE.md Bump django-coverage-plugin from 2.0.2 to 2.0.3 #15)./research/:slugresolved by CentralRouteManager Phase 1 (slug-based) with a Phase-3 canonical-redirect guard like threads/labelsets;openedResearchReportadded to the routing write-discipline allowlist.ResearchReportDetail(markdown body with footnote citations viaSafeMarkdown; Report / Citations / Sources / Run-details tabs; live polling + Cancel while running; citations deep-link to the cited annotation) and a corpus "Research" tab listing reports. A secondaryStartResearchModalprovides an explicit kickoff; the primary trigger remains the chat agent.RESEARCH_REPORT_*wired into the notification type union + job-toast system (clickable toast deep-links to/research/:slug), terminal Apollo-cache updates, and a completion hook the detail view uses to refetch + stop polling.Tests: backend slug-resolver + status-tool/service tests (
opencontractserver/tests/research/); frontend unit tests for route parsing + research utils; a Playwright component test for the detail view.Design notes / decisions
isPublic/sharing surface, so no sharing UI is built; Cancel is gated onmyPermissions.citationsJSON keeps its raw snake_case keys / integer PKs (GenericScalarpassthrough); the detail view maps PKs → source documents for deep-links.Verification
tsc --noEmitclean (0 errors); Prettier clean.docker compose -f test.yml run django python manage.py test opencontractserver.tests.research --keepdb, and the Playwright component test viacd frontend && yarn test:ct --reporter=list -g "ResearchReportDetail".Manual smoke test
/research/:slugwhile running → slug resolves, progress updates via polling, Cancel transitions to CANCELLED.RESEARCH_REPORT_COMPLETEtoast appears, is clickable → lands on/research/:slug; the in-chat[Open report]link works.?ann=); Sources documents open./research/:sluggets not-found (no leak); no sharing controls exist.Out of scope (v2)
Sharing /
is_public, document-subset scoping, a backend per-step PROGRESS emitter (would let us drop polling), watchdog cleanup task.Generated by Claude Code