Follow Up to Deep Research Frontend + Chat Status Tool

[JSv4]

Code Review: Deep Research Frontend + Chat Status Tool

Overview

This PR ships the full frontend surface for the deep-research feature that was backend-only in #1814: a /research/:slug route, a corpus Research tab, report detail view with polling + WebSocket completion, notification wiring, and a chat status tool so users can ask "is my research done?" This is a substantial, well-scoped feature completion.

---

Positives

• Architecture is sound. Route resolution follows the same CentralRouteManager Phase-1/3 pattern as extracts, threads, and labelsets. The openedResearchReport reactive var, cache policies, and allowlist discipline test are all consistent with how existing entity routes work.
• Security posture is good. researchReportBySlug uses filter_visible (creator-only), the resolver is @login_required, non-owner and unknown slug both return null (same response, prevents enumeration), and the anonymous rejection is pinned with a test.
• Atomic finalize fix is correct. Wrapping the terminal save() and M2M set() calls in transaction.atomic() closes a real gap — a worker crash between them would have left a COMPLETED report with empty provenance.
• Polling design is clean. isTerminalResearchStatus treats unrecognized statuses as terminal so polling never loops forever on an unknown state. The indirect flow of refetch → status update → useEffect stopping polling is correct.
• Constants are in the right place. MAX_RESEARCH_PROMPT_CHARS, MAX_RESEARCH_TITLE_CHARS, RESEARCH_REPORT_POLL_INTERVAL_MS, RESEARCH_REPORT_UPDATE_PERMISSION are all in constants.ts, not inlined (CLAUDE.md rule 4).
• relayStylePagination keyArgs are correct. ["corpusId", "status"] uses the GraphQL field-argument names, per CLAUDE.md pitfall Bump django-coverage-plugin from 2.0.2 to 2.0.3 #15.
• Test coverage is solid. Backend slug resolver, status tool, and service helper are well-exercised. Frontend unit tests cover utilities, route parsing, and the routing write-discipline regression guard.

---

Issues

1. Detail test wrapper mock will drain on retries (test fragility risk)

File: frontend/tests/ResearchReportDetailTestWrapper.tsx

The mock array provides exactly one response. The component uses notifyOnNetworkStatusChange: true and calls refetch() inside onComplete, which means a second network trip will find an empty mock bucket and log a "No more mocked responses" error.

Since all tests in ResearchReportDetail.ct.tsx use terminal states (where polling is disabled and useResearchCompletionNotification is also disabled via !isTerminal), this is probably fine in practice. But it is a fragility: if test timing changes or a future test exercises a non-terminal state, the bucket drains and test behaviour becomes unpredictable.

Compare to CorpusResearchReportCardsTestWrapper.tsx which correctly uses maxUsageCount: Number.POSITIVE_INFINITY. Consider applying the same pattern to the detail mock.

2. Duration test relies on duration_seconds being a computed property

File: opencontractserver/tests/research/test_research_status_tool.py

test_completed_report_shows_duration sets started_at and completed_at directly, then asserts the formatted duration appears in the output. This works only if ResearchReport.duration_seconds is a @property computed from those timestamps at read-time — not a stored field.

If duration_seconds is a stored field set only by finalize(), the test creates a report where duration_seconds is None, and _summarize_recent_reports's if secs is not None guard would make detail = "" — causing assertIn("finished in 2m 5s", result) to fail. Since the PR says tests pass, duration_seconds is presumably a property, but a comment documenting this assumption would prevent a future refactor from silently breaking it.

3. Async error handling discards the exception

Files: frontend/src/views/ResearchReportDetail.tsx, frontend/src/components/widgets/modals/StartResearchModal.tsx

Both catch blocks swallow the underlying error without logging. Network errors, GraphQL parse failures, and unexpected exceptions all disappear silently. Add console.error(e) at minimum so production issues are diagnosable.

4. Snake_case local variable names in CorpusResearchReportCards

File: frontend/src/components/research/CorpusResearchReportCards.tsx

The reactive-var shadow locals opened_corpus, research_search_term, and auth_token use snake_case. The rest of the codebase uses camelCase for these. They should be openedCorpus, researchSearchTerm, authToken to match the convention.

5. CancelResearchReportOutput.obj.status typed as plain string

File: frontend/src/graphql/mutations.ts

The cancel output's obj.status is typed as string while ResearchReportType uses JobStatus | string. Minor inconsistency — the narrower union would catch typos if any caller compares against JobStatus values.

6. Slug index on ResearchReport — worth confirming

File: config/graphql/research_queries.py

The researchReportBySlug resolver runs .filter(slug=slug).first() on every route load. If the slug column lacks a DB index this will be a full-table scan as the table grows. AutoSlugField / SlugField(unique=True) both carry an implicit index, so this is likely already covered — just confirm 0001_initial.py declares the field with unique=True or db_index=True.

---

Minor nits

• ResearchReportDetail.tsx is 881 lines (mostly styled components). A .styles.ts companion would match the Corpuses.styles.ts pattern — worth a follow-up issue.
• RESEARCH_REPORT_PROGRESS is correctly absent from the notification union per the inline comment; add it when the backend emitter lands.

---

Summary

The implementation is clean, security-conscious, and follows project conventions throughout. Main actionable items: add maxUsageCount: Infinity to the detail test wrapper, add error logging to async catch blocks, fix the snake_case variable names, and confirm the slug column is indexed. None of these block the feature from working correctly, but addressing them before merge keeps the test suite resilient and the codebase consistent.

Originally posted by @claude in #1836 (comment)

[claude]

Claude finished @JSv4's task in 8m 52s —— View job

---

Analysis Complete

Note: The GitHub App doesn't have write access to push a branch to this repo, so I'm providing findings and code fixes inline below.

---

What I Investigated

After exploring the codebase, PR #1836 has not yet been merged to main. The frontend files referenced in the review (ResearchReportDetail.tsx, CorpusResearchReportCards.tsx, StartResearchModal.tsx, the test wrappers, and test_research_status_tool.py) don't exist in main yet. Here's what I found for each item:

---

Fix #6 — Slug index: Already correct, no change needed ✅

models.py:83-88:

slug = models.SlugField(
    max_length=160,
    unique=True,
    blank=True,
    db_index=True,   # explicit index
)

Migration 0001_initial.py:38 also declares unique=True, which carries an implicit index. The column is doubly-indexed. Nothing to do here.

---

Fix #5 — duration_seconds computed-property invariant

Confirmed: duration_seconds is a @property in models.py:212-217, computed from started_at / completed_at at read time (never stored). The reviewer's assumption is correct.

Recommended changes (apply to PR #1836 or main after merge):

opencontractserver/research/models.py — add docstring to the property:

@property
def duration_seconds(self) -> float | None:
    """Always computed from timestamps — never a stored field.

    Tests (e.g. ``test_completed_report_shows_duration``) that set
    ``started_at`` / ``completed_at`` directly rely on this being a
    pure derived property. If this is ever converted to a stored field,
    those tests must be updated to call ``finalize()`` or set the field
    explicitly.
    """
    if self.started_at and self.completed_at:
        return (self.completed_at - self.started_at).total_seconds()
    return None

opencontractserver/tests/research/test_research_report_model.py — add guard test:

def test_duration_seconds_computed_from_timestamps(self):
    """duration_seconds is always derived from started_at/completed_at — not stored."""
    from datetime import datetime, timezone

    report = ResearchReport.objects.create(
        creator=self.user, corpus=self.corpus, prompt="x",
    )
    self.assertIsNone(report.duration_seconds)

    # Set timestamps directly (bypassing finalize) — property must compute correctly.
    report.started_at = datetime(2024, 1, 1, 0, 0, 0, tzinfo=timezone.utc)
    report.completed_at = datetime(2024, 1, 1, 0, 2, 5, tzinfo=timezone.utc)
    self.assertAlmostEqual(report.duration_seconds, 125.0)

---

Fix #1 — Detail test wrapper: add maxUsageCount: POSITIVE_INFINITY

Apply to frontend/tests/ResearchReportDetailTestWrapper.tsx in PR #1836.

Find the mock(s) for the detail query and add maxUsageCount:

// Before:
{
  request: { query: GET_RESEARCH_REPORT_DETAIL, variables: { ... } },
  result: { data: { ... } },
}

// After:
{
  request: { query: GET_RESEARCH_REPORT_DETAIL, variables: { ... } },
  result: { data: { ... } },
  maxUsageCount: Number.POSITIVE_INFINITY,   // prevents "No more mocked responses" on refetch
}

---

Fix #2 — Add console.error(e) to async catch blocks

Apply to frontend/src/views/ResearchReportDetail.tsx and frontend/src/components/widgets/modals/StartResearchModal.tsx in PR #1836.

In every catch (e) block that currently only calls setError(...) or similar, add:

} catch (e) {
  console.error(e);    // add this line
  setError("...");     // or whatever error state update exists
}

---

Fix #3 — Fix snake_case locals in CorpusResearchReportCards

Apply to frontend/src/components/research/CorpusResearchReportCards.tsx in PR #1836.

Rename these three reactive-var shadow locals:

Current (incorrect)	Corrected
opened_corpus	openedCorpus
research_search_term	researchSearchTerm
auth_token	authToken

---

Fix #4 — Narrow CancelResearchReportOutput.obj.status type

Apply to frontend/src/graphql/mutations.ts in PR #1836 (the research mutation type declarations).

// Before:
interface CancelResearchReportOutput {
  cancelResearchReport: {
    ok: boolean;
    message: string;
    obj: {
      status: string;   // too wide
      // ...
    } | null;
  };
}

// After:
import { JobStatus } from "../types/graphql-api";  // or wherever JobStatus is defined

interface CancelResearchReportOutput {
  cancelResearchReport: {
    ok: boolean;
    message: string;
    obj: {
      status: JobStatus | string;   // matches ResearchReportType pattern
      // ...
    } | null;
  };
}

---

Summary

Fix	Status
#6 Slug indexed	✅ Already correct
#5 duration_seconds property clarification	📝 Code provided above (ready to apply)
#1 Test wrapper maxUsageCount	📝 Code provided above (apply to PR #1836)
#2 console.error in catch blocks	📝 Code provided above (apply to PR #1836)
#3 snake_case → camelCase variable names	📝 Code provided above (apply to PR #1836)
#4 Narrow cancel status type	📝 Code provided above (apply to PR #1836)

Items #1–4 need to be applied directly to the PR #1836 branch. The code snippets above are ready to use. Items #5 and #6 touch files already in main — the model docstring and guard test changes can be applied independently (my local commit has these ready, but the bot lacks push access to this repo).