Skip to content

Resume feature#105

Open
tekrajchhetri wants to merge 26 commits into
improvement_designfrom
resume-feature
Open

Resume feature#105
tekrajchhetri wants to merge 26 commits into
improvement_designfrom
resume-feature

Conversation

@tekrajchhetri

Copy link
Copy Markdown
Collaborator

This PR adds the resume feature from the experiment UI and fixes the bug with resume for failed task.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the SynthScholar literature review tool by introducing markdown rendering via react-markdown and remark-gfm, adding animated provenance timelines for both public and owner views, implementing search, status filtering, and sorting for the reviews list, and providing a full-page protocol creation experience with inline field guides. The code review feedback identifies several critical issues and improvement opportunities, including a potential crash due to a missing null guard in certaintyClass, a missing import for resolveOpenRouterKey causing a runtime error, React console warnings from spreading the node prop onto DOM elements in MarkdownContent, broken back navigation when clearing the active review selection, performance issues from frequent re-renders in ReviewDetail, and a side-effect inside a React state updater in useProgressStream.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +367 to +368
function certaintyClass(c: string) {
const v = c.toLowerCase();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If c is null or undefined, calling c.toLowerCase() will throw a TypeError and crash the public page. Guard against falsy values by fallback-defaulting to an empty string, matching the implementation of certaintyStyle in ProvenanceTimelineOwner.tsx.

Suggested change
function certaintyClass(c: string) {
const v = c.toLowerCase();
function certaintyClass(c: string) {
const v = (c || "").toLowerCase();

// the retry body. Without this, the pipeline aborts with the
// exact "No OpenRouter API key available" 400 the user has
// been hitting.
const { key } = await resolveOpenRouterKey();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The function resolveOpenRouterKey is called here but is not imported in this file. This will cause a ReferenceError at runtime when a user attempts to retry or resume a review. Please import resolveOpenRouterKey from its defining module.

Comment on lines +34 to +143
components={{
// Headings: keep the visual hierarchy distinct without huge fonts —
// these often appear inside cards that already have their own h2.
h1: ({ ...p }) => <h2 style={{ fontSize: "1.4em", fontWeight: 600, margin: "0.8em 0 0.4em" }} {...p} />,
h2: ({ ...p }) => <h3 style={{ fontSize: "1.2em", fontWeight: 600, margin: "0.8em 0 0.3em" }} {...p} />,
h3: ({ ...p }) => <h4 style={{ fontSize: "1.05em", fontWeight: 600, margin: "0.7em 0 0.25em" }} {...p} />,
h4: ({ ...p }) => <h5 style={{ fontSize: "1em", fontWeight: 600, margin: "0.6em 0 0.2em" }} {...p} />,
// Paragraphs + lists with breathing room
p: ({ ...p }) => <p style={{ margin: "0.6em 0" }} {...p} />,
ul: ({ ...p }) => <ul style={{ margin: "0.5em 0", paddingLeft: "1.5em" }} {...p} />,
ol: ({ ...p }) => <ol style={{ margin: "0.5em 0", paddingLeft: "1.5em" }} {...p} />,
li: ({ ...p }) => <li style={{ margin: "0.2em 0" }} {...p} />,
// Blockquote
blockquote: ({ ...p }) => (
<blockquote
style={{
margin: "0.8em 0",
padding: "0.4em 0.9em",
borderLeft: "3px solid var(--bkb-accent, #4a4a4a)",
background: "var(--bkb-surfaceAlt, #f5f5f0)",
color: "var(--bkb-textMuted, #4a4a4a)",
borderRadius: "0 4px 4px 0",
}}
{...p}
/>
),
// Inline + block code
code: ({ ...p }) => (
<code
style={{
fontFamily: "ui-monospace, SFMono-Regular, Menlo, monospace",
fontSize: "0.9em",
background: "var(--bkb-surfaceAlt, #f0f0eb)",
padding: "0.1em 0.35em",
borderRadius: 3,
}}
{...p}
/>
),
pre: ({ ...p }) => (
<pre
style={{
margin: "0.8em 0",
padding: "0.8em 1em",
background: "var(--bkb-surfaceAlt, #f5f5f0)",
border: "1px solid var(--bkb-border, #e5e5e0)",
borderRadius: 6,
fontSize: "0.85em",
lineHeight: 1.5,
overflowX: "auto",
}}
{...p}
/>
),
// GFM tables — give them visible borders and a touch of padding
// so the cells don't run together.
table: ({ ...p }) => (
<div style={{ overflowX: "auto", margin: "0.8em 0" }}>
<table
style={{
borderCollapse: "collapse",
width: "100%",
fontSize: "0.95em",
}}
{...p}
/>
</div>
),
th: ({ ...p }) => (
<th
style={{
textAlign: "left",
padding: "8px 10px",
borderBottom: "2px solid var(--bkb-border, #e5e5e0)",
background: "var(--bkb-surfaceAlt, #f5f5f0)",
fontWeight: 600,
}}
{...p}
/>
),
td: ({ ...p }) => (
<td
style={{
padding: "6px 10px",
borderBottom: "1px solid var(--bkb-border, #e5e5e0)",
verticalAlign: "top",
}}
{...p}
/>
),
// Horizontal rule — agent uses --- between sections
hr: () => (
<hr
style={{
margin: "1.4em 0",
border: "none",
borderTop: "1px solid var(--bkb-border, #e5e5e0)",
}}
/>
),
// Links open in a new tab; keep the visual subtle
a: ({ ...p }) => (
<a
target="_blank"
rel="noopener noreferrer"
style={{ color: "var(--bkb-accent, #1a73e8)", textDecoration: "underline" }}
{...p}
/>
),
}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In react-markdown v9+, custom renderer components receive the node object as a prop. Spreading ...p directly onto standard HTML elements passes the node prop to the DOM, which triggers React console warnings about unrecognized non-HTML attributes. Destructure node out of the props before spreading the rest onto the DOM elements.

        components={{
          // Headings: keep the visual hierarchy distinct without huge fonts —
          // these often appear inside cards that already have their own h2.
          h1: ({ node, ...p }) => <h2 style={{ fontSize: "1.4em", fontWeight: 600, margin: "0.8em 0 0.4em" }} {...p} />,
          h2: ({ node, ...p }) => <h3 style={{ fontSize: "1.2em", fontWeight: 600, margin: "0.8em 0 0.3em" }} {...p} />,
          h3: ({ node, ...p }) => <h4 style={{ fontSize: "1.05em", fontWeight: 600, margin: "0.7em 0 0.25em" }} {...p} />,
          h4: ({ node, ...p }) => <h5 style={{ fontSize: "1em",    fontWeight: 600, margin: "0.6em 0 0.2em"  }} {...p} />,
          // Paragraphs + lists with breathing room
          p:  ({ node, ...p }) => <p  style={{ margin: "0.6em 0" }} {...p} />,
          ul: ({ node, ...p }) => <ul style={{ margin: "0.5em 0", paddingLeft: "1.5em" }} {...p} />,
          ol: ({ node, ...p }) => <ol style={{ margin: "0.5em 0", paddingLeft: "1.5em" }} {...p} />,
          li: ({ node, ...p }) => <li style={{ margin: "0.2em 0" }} {...p} />,
          // Blockquote
          blockquote: ({ node, ...p }) => (
            <blockquote
              style={{
                margin: "0.8em 0",
                padding: "0.4em 0.9em",
                borderLeft: "3px solid var(--bkb-accent, #4a4a4a)",
                background: "var(--bkb-surfaceAlt, #f5f5f0)",
                color: "var(--bkb-textMuted, #4a4a4a)",
                borderRadius: "0 4px 4px 0",
              }}
              {...p}
            />
          ),
          // Inline + block code
          code: ({ node, ...p }) => (
            <code
              style={{
                fontFamily: "ui-monospace, SFMono-Regular, Menlo, monospace",
                fontSize: "0.9em",
                background: "var(--bkb-surfaceAlt, #f0f0eb)",
                padding: "0.1em 0.35em",
                borderRadius: 3,
              }}
              {...p}
            />
          ),
          pre: ({ node, ...p }) => (
            <pre
              style={{
                margin: "0.8em 0",
                padding: "0.8em 1em",
                background: "var(--bkb-surfaceAlt, #f5f5f0)",
                border: "1px solid var(--bkb-border, #e5e5e0)",
                borderRadius: 6,
                fontSize: "0.85em",
                lineHeight: 1.5,
                overflowX: "auto",
              }}
              {...p}
            />
          ),
          // GFM tables — give them visible borders and a touch of padding
          // so the cells don't run together.
          table: ({ node, ...p }) => (
            <div style={{ overflowX: "auto", margin: "0.8em 0" }}>
              <table
                style={{
                  borderCollapse: "collapse",
                  width: "100%",
                  fontSize: "0.95em",
                }}
                {...p}
              />
            </div>
          ),
          th: ({ node, ...p }) => (
            <th
              style={{
                textAlign: "left",
                padding: "8px 10px",
                borderBottom: "2px solid var(--bkb-border, #e5e5e0)",
                background: "var(--bkb-surfaceAlt, #f5f5f0)",
                fontWeight: 600,
              }}
              {...p}
            />
          ),
          td: ({ node, ...p }) => (
            <td
              style={{
                padding: "6px 10px",
                borderBottom: "1px solid var(--bkb-border, #e5e5e0)",
                verticalAlign: "top",
              }}
              {...p}
            />
          ),
          // Horizontal rule — agent uses --- between sections
          hr: () => (
            <hr
              style={{
                margin: "1.4em 0",
                border: "none",
                borderTop: "1px solid var(--bkb-border, #e5e5e0)",
              }}
            />
          ),
          // Links open in a new tab; keep the visual subtle
          a: ({ node, ...p }) => (
            <a
              target="_blank"
              rel="noopener noreferrer"
              style={{ color: "var(--bkb-accent, #1a73e8)", textDecoration: "underline" }}
              {...p}
            />
          ),
        }}

Comment on lines +2306 to +2312
React.useEffect(() => {
const onPop = () => {
const params = new URLSearchParams(window.location.search);
setMode(params.get("mode") === "new" ? "new" : "list");
const id = params.get("review");
if (id) setSelectedId(id);
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When navigating back to a state where no review is selected (i.e., the review URL parameter is absent), the id variable will be null. The current guard if (id) setSelectedId(id); prevents clearing the active selection, leaving the previous review's detail panel open. Pass id directly to setSelectedId to allow clearing the selection on back navigation.

Suggested change
React.useEffect(() => {
const onPop = () => {
const params = new URLSearchParams(window.location.search);
setMode(params.get("mode") === "new" ? "new" : "list");
const id = params.get("review");
if (id) setSelectedId(id);
};
// Listen for browser back/forward navigation between list and new modes.
React.useEffect(() => {
const onPop = () => {
const params = new URLSearchParams(window.location.search);
setMode(params.get("mode") === "new" ? "new" : "list");
const id = params.get("review");
setSelectedId(id);
};

Comment on lines +1615 to +1623
// 1-second tick used by the progress card to render "last update Ns ago".
// Without it the seconds-since-last-event display would only refresh when
// a new event arrives — defeating the whole point of a stuck-detector.
const [nowTick, setNowTick] = React.useState(() => Date.now());
React.useEffect(() => {
if (!isLive) return;
const id = setInterval(() => setNowTick(Date.now()), 1000);
return () => clearInterval(id);
}, [isLive]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Updating the nowTick state every second at the ReviewDetail level triggers a full re-render of the entire detail panel (including headers, action buttons, and logs) when a review is live. To optimize performance and avoid high CPU usage, consider extracting the progress card or the freshness counter into a dedicated sub-component with its own local timer state.

Comment on lines +310 to +316
// Seed both the rendered list and the dedup window so the live SSE
// doesn't re-add events we already loaded from /reviews/{id}/log.
setEvents((prev) => {
if (prev.length > 0) return prev;
for (const e of seedEvents) seenSigsRef.current.add(_eventSignature(e));
return seedEvents;
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Mutating seenSigsRef.current inside the setEvents state updater function is a side-effect. React state updater functions should be pure and free of side-effects, as they can run multiple times (e.g., in Strict Mode or during concurrent rendering). Populate seenSigsRef.current directly in the .then block before calling setEvents.

Suggested change
// Seed both the rendered list and the dedup window so the live SSE
// doesn't re-add events we already loaded from /reviews/{id}/log.
setEvents((prev) => {
if (prev.length > 0) return prev;
for (const e of seedEvents) seenSigsRef.current.add(_eventSignature(e));
return seedEvents;
});
for (const e of seedEvents) seenSigsRef.current.add(_eventSignature(e));
setEvents((prev) => (prev.length === 0 ? seedEvents : prev));

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