Sprint 4 [FIX] Frontend fixes: UN-2900, UN-3137, UN-3355, UN-3507

[hari-kuriakose]

Sprint 4 — frontend fixes

Important

Base is feat/shadcn-oss-migration, not main. Against the correct base this is 7 files / 3 commits. Against main it would show 300 files / 160 commits — almost all of them the shadcn migration itself. Please keep the base as set.

Commit	Ticket	Change
8df78b40	UN-3137, UN-3355	Chunk size units and highlight coordinate filtering
3514226d	UN-3507	Poll index status when websocket updates stall
b105f0e2	UN-2900	Show a per-prompt warning for unresolvable single-pass variables

Dependency on the backend PR

The UN-2900 commit renders single_pass_unresolvable_variables, which is produced by the backend branch (un-sprint4-D-backend).

Either merge order is safe. Verified: Header.jsx defaults the value to [], the render guards on .length > 0, and PromptCard guards on !== undefined. Without the backend change this simply renders nothing — it is inert, not broken. An earlier note of mine claimed the order mattered; that was overstated and is corrected here.

🤖 Generated with Claude Code

https://claude.ai/code/session_0197gRFp4QFxu2VzxhTEozGn

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[greptile-apps]

Greptile Summary

This PR applies several Prompt Studio frontend fixes.

• Displays chunk sizes consistently as token counts.
• Filters unusable PDF highlight geometry.
• Adds HTTP polling to recover index status when WebSocket updates stall.
• Shows per-prompt warnings for single-pass variables that cannot be resolved.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the eligible follow-up review scope.

No blocking failure remains.

Important Files Changed

Filename	Overview
frontend/src/components/custom-tools/manage-docs-modal/ManageDocsModal.jsx	Adds periodic index-status polling and fingerprint-based retirement when socket updates are unavailable.
frontend/src/components/custom-tools/add-llm-profile/AddLlmProfile.jsx	Corrects chunk-size labeling and presentation to use token units directly.
frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx	Rejects incomplete and non-positive highlight geometry before rendering or navigation.
frontend/src/components/custom-tools/prompt-card/Header.jsx	Displays warning tags for single-pass variables reported as unresolvable.
frontend/src/components/custom-tools/prompt-card/PromptCard.jsx	Refreshes the per-prompt unresolvable-variable metadata from prompt-save responses.
frontend/src/components/custom-tools/tool-ide/ToolIde.jsx	Simplifies tool updates while documenting why prompt warning metadata is handled locally.
frontend/src/components/custom-tools/profile-info-bar/ProfileInfoBar.jsx	Labels profile chunk sizes explicitly as token counts.

Sequence Diagram

sequenceDiagram
    participant UI as Manage Docs UI
    participant API as Document Index API
    participant WS as WebSocket
    UI->>API: Start indexing
    API-->>UI: Request accepted
    alt WebSocket update arrives
        WS-->>UI: Index completion event
        UI->>UI: Clear indexing state
    else WebSocket update stalls
        loop While document remains in indexing state
            UI->>API: Poll index status
            API-->>UI: Current index rows
        end
    end

Loading

_{Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/fea..." | Re-trigger Greptile}

[hari-kuriakose]

@greptileai

[chandrasekharan-zipstack]

⚠️ Before the findings — the base branch this PR declares no longer exists

This is the thing blocking a merge decision, so it goes first.

• The PR description says the base is feat/shadcn-oss-migration. That branch is gone from origin — git ls-remote --heads origin lists only feat/shadcn-foundation.
• GitHub therefore reports base: main, and computes this PR as 374 files, +27,683 / −2,103.
• The head branch has also moved well past what the description covers: un-sprint4-C-frontend now carries roughly 25 additional shadcn commits and two merge commits, tip b9f676c1f — "Merge remote-tracking branch 'origin/feat/shadcn-oss-migration' into resolve-2258".

What I reviewed. I scoped to the 6-commit sprint-4 chain, 8df78b40f^..9334948a8 — 7 files, +224 / −21 — which reproduces the "7 files" figure in the description:

Commit	Ticket
8df78b40f	UN-3137, UN-3355
3514226d2	UN-3507
b105f0e25	UN-2900
ad2635ebe	review-fix follow-up
c5e8e0b71	UN-3507 follow-up
9334948a8	UN-2900 docs follow-up

Inline anchors below are at the PR head b9f676c1f, so a few line numbers differ from that chain (Header.jsx in particular drifted ~44 lines). Every finding was re-verified against head before posting.

If the intent is to merge this branch into main as-is, this review covers a small fraction of what would land. That is the single assumption most likely to change the verdict.

---

Verdict — REQUEST CHANGES

Mode: LITE — single-pass, 16/16 lenses, one reader, no subagents. Eligible: 7 files / 245 lines, no disqualifiers.

Summary — Critical: 0 · High: 3 · Medium: 5 · Low: 3 · Lenses run: 16/16

Gate detail: no migrations, no auth/permission surface, no dependency or lockfile change, no IaC/CI/Helm, no wire-format break (UN-2900 consumes an additive backend field). The 5s polling loop was weighed against the concurrency disqualifier and judged out of scope for it — it is a setInterval over an idempotent GET, not a lock/thread/async-ordering primitive. It is reviewed under lens 6 below.

Mode detection: INITIAL — no prior reviews or inline comments on this PR authored by me. (Commit ad2635ebe "Address lite-review findings #1-#6" refers to a review that was never posted to this thread, so there was no prior ledger to reconcile.)

---

Lens checklist

#	Lens	Result
1	Spec & intent	See findings #4, #6
2	Architectural fit & precedent	See finding #8
3	Correctness & edge cases	See findings #1, #2, #4, #6, #11
4	Security	Clean — no new endpoint or handler; the poll reuses axiosPrivate against the existing tenant-scoped document-index route (ManageDocsModal.jsx:372-375). Tooltip content is user-authored variable names, but the overlay shim uses no dangerouslySetInnerHTML (verified in antd-overlays.tsx).
5	Data integrity & migrations	N/A — no migration, schema change, or persisted write in this diff
6	Concurrency	Clean, with one noted narrow race: indexDocs is an effect dependency (ManageDocsModal.jsx:248-255), so retiring one document tears down and re-arms the interval, resetting the baseline Map for the documents still indexing. A sibling completing inside that re-render window has its completed state adopted as the new baseline and never retires. The window is milliseconds and both are usually retired in the same tick's loop, so it does not reach Medium — but it compounds finding #3. Store identity was checked and is stable (custom-tool-store.js:68-70), so the interval does not thrash.
7	API & contract compatibility	See finding #1 (index-space contract between producer and viewer). The backend field contract is otherwise sound — single_pass_unresolvable_variables is additive, a SerializerMethodField returning list[str] at backend/prompt_studio/prompt_studio_v2/serializers.py:26,35 on origin/un-sprint4-D-backend, and the frontend defaults to []. The description's "either merge order is safe" claim is verified.
8	Reliability & resilience	See finding #3
9	Performance & cost	See findings #3, #5
10	Observability	Not covered — no metric or structured-log surface exists in this frontend for a new polling path to hook into, so I could not assess this against any repo convention.
11	Operational safety	Not covered — the new poll ships unflagged and there is no rollback lever short of a revert, but this repo has no frontend feature-flag convention I could measure it against.
12	LLM/agent-specific	N/A — no prompt template, model call, tool config, or retry/fallback logic in this diff. UN-2900 renders a warning about prompt variables; it does not touch the model path.
13	Testing	See finding #7 (in the body below — no single anchor line)
14	Dependencies & build	Clean — no package.json, bun.lock, Dockerfile, or CI file in the review surface. TriangleAlert comes from lucide-react, already a dependency.
15	Code quality	See finding #10
16	Doc & comment accuracy	See findings #2, #9. Everything else checked out: the PromptCard.jsx:74-83 latch cross-reference in the ToolIde comment is accurate at head; the DocumentParser.jsx:273 stable-key claim is accurate; PromptCard.jsx:379 passing promptDetailsState is accurate; the CUSTOM_DATA exclusion claim matches find_unresolvable_single_pass_variables on the backend branch; the coordinate shape claim in PdfViewer.jsx matches RenderHighlights.jsx. Comment density here is unusually high, but the claims are — with the two exceptions above — verifiable and correct.

---

Finding that could not be anchored inline

[Medium] [Lens 13] — No tests for a new async state machine, in an area that already has them

• Location: whole review surface — git diff --stat 8df78b40f^ 9334948a8 lists 7 files, 224 insertions, and zero test files. No single line to anchor to.
• Failure mode: the UN-3507 poll introduces a baseline/fingerprint retirement protocol with at least four distinct branches (recordBaselineOnly, unseen-key adoption, fingerprint-moved, silent) and none is exercised. Findings #2 and #3 are both single-assertion unit tests on the retirement predicate.
• Evidence: the base branch carries 33 frontend test files, including one in a directory this PR edits — frontend/src/components/custom-tools/prompt-card/DisplayPromptResult.test.jsx. The harness and the local convention both already exist; this diff simply does not use them.
• Suggested fix: extract the retirement predicate into a pure function and table-test it: first index success, re-index mid-run, re-index complete, failed index.
• Confidence: High.

---

Open questions for the author

1. The base branch this PR declares no longer exists (see the top of this review). What is the intended merge path — is the shadcn migration landing first under a different branch name, and should this PR's base be repointed at it?
2. The head branch has moved well past the 3 commits the description covers. Was the merge intended to be part of this PR, or is it integration noise that should be rebased away?
3. Finding #1: does manual-review highlight navigation get exercised on documents containing empty pages? That determines whether the index shift is a common or a rare path.
4. Finding #3: is there any signal on the document-index response that distinguishes "indexing failed" from "indexing still running"? If not, bounding the poll is the only available fix.

Assumptions made

• The review surface is the 6-commit sprint-4 chain, not the PR as GitHub computes it. If the intent is to merge the branch tip into main as-is, the eligibility gate would have refused this for lite review outright.
• The three commits added after the description was written (ad2635ebe, c5e8e0b71, 9334948a8) are in scope as part of the same sprint-4 work.
• origin/un-sprint4-D-backend at eb9ff6ae8 is the backend counterpart; the single_pass_unresolvable_variables contract was verified against it.
• Cloud-side impact for finding #1 was assessed against the working tree of the unstract-cloud repo, not a branch matched to this PR.

[chandrasekharan-zipstack]

[High] [Lens 3, 7] — This filter shifts the highlight index space, desyncing cloud highlight navigation

• Failure mode: currentHighlightIndex is produced against the unfiltered highlightData, but PdfViewer indexes into the filtered array. Every entry this filter drops shifts all later indices by one. Once currentHighlightIndex >= processedHighlightData.length, the guard at PdfViewer.jsx:119-121 fails and the component falls through to rendering every highlight at once — so the last N navigation steps show all highlights instead of one. The jump-to-page effect at :150-151 is wrong by the same offset.
• Evidence: the cloud plugin sets the index space from the raw array — frontend/src/plugins/manual-review/components/result-editor/ResultEditor.jsx:612 and :1009, both setTotalHighlights(highlightData.length); — and passes the raw array plus that index into PdfViewer at frontend/src/plugins/manual-review/page/ManualReviewPage.jsx:2246-2254. Navigation wraps against totalHighlights at ManualReviewPage.jsx:1064-1076. (Those three paths are in the unstract-cloud repo, not this one.)
  Coordinate semantics confirmed against the consumer frontend/src/plugins/pdf-highlight/RenderHighlights.jsx (area[0]=page, area[2]/area[3]=height/pageHeight) — so the shape comment added here is correct; it is the index space, not the geometry, that breaks.
  Concretely, [A(valid), B(empty page), C(valid)]: the counter reads "2 / 3" but shows C, and step 3 shows all highlights. Before this change B survived the filter (its page number was non-zero) and the indices lined up.
• Suggested fix: keep the index space single. Either filter at the producer so totalHighlights, currentHighlightIndex and the viewer all derive from the same array, or filter to {original index, coords} pairs and map currentHighlightIndex through that.
• Confidence: High on the mechanism; Medium on how often empty-page entries appear in manual-review documents.

[chandrasekharan-zipstack]

[High] [Lens 3, 16] — This fingerprint retires a document mid-run on re-index, the exact case the comment above claims to prevent

• Failure mode: the fingerprint is index_id + "|" + modified_at. The backend saves the same IndexManager row partway through an indexing run, bumping modified_at while the previous run's index id is still in place. On a re-index of an already-indexed document both retire conditions at :419-425 become true after the extraction phase — the fingerprint has moved, and handleIsIndexed() is true on the stale id — so deleteIndexDoc() fires and the spinner clears while indexing is still running. Same on a failed re-index.

• Evidence: backend/prompt_studio/prompt_studio_index_manager_v2/prompt_studio_index_helper.py:125

  index_manager.save(update_fields=["extraction_status"])

  called from backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py:2594 (success) and :2576 (failure), both before the index id is written at prompt_studio_index_helper.py:62-64. modified_at is auto-included on partial saves by backend/utils/models/base_model.py:99-113, and it reaches the client because IndexManagerSerializer is fields = "__all__".

  The comment being contradicted, verbatim: "Retiring on 'has an index id' would be wrong... Compare against a fingerprint of the index row captured when the poll armed, and retire only once it actually changes." The fingerprint does not distinguish a mid-run save from a completed run.

• Suggested fix: do not infer completion from an audit timestamp. Require raw_index_id / summarize_index_id itself to differ from the baseline value, or add an explicit run/status field to the document-index response and key off that.

• Confidence: High.

[chandrasekharan-zipstack]

[High] [Lens 8, 9] — This poll never terminates when indexing fails on the dead-socket path

• Failure mode: retirement requires handleIsIndexed(indexType, item) at :421, which is !!raw_index_id (:334-345). A first-time index that fails never writes an index id, so the document is never retired, indexDocs never empties, and this interval runs for the entire lifetime of the open modal — 2 requests every 5s, indefinitely, with the spinner never clearing. That is the same "spins forever" symptom UN-3507 set out to fix, now with an unbounded request loop attached.
• Evidence: the socket path does handle failure — frontend/src/hooks/usePromptStudioSocket.js:98-102 calls deleteIndexDoc(docId) from handleFailed("index_document"). But a dead socket is precisely the condition this poll exists to cover, so that handler is unavailable exactly when it is needed. The comment at :215-218 acknowledges the loop runs "for as long as the modal stays open" but frames that as an error-toast concern only.
• Suggested fix: bound the poll — a max-attempts or wall-clock deadline after which it stops and surfaces a single terminal message — and retire on an observable failure signal rather than only on success.
• Confidence: High.

[chandrasekharan-zipstack]

[Medium] [Lens 1, 3] — This warning can neither appear nor disappear when single pass is toggled

• Failure mode: two directions, both wrong. Enabling single pass shows no warnings until a full reload, because promptDetailsState latches on first seed and is never re-seeded. Disabling single pass leaves stale warnings on screen for the same reason — the user sees a warning about a mode they are not in.
• Evidence: the latch is real — PromptCard.jsx:74-83 returns early once isPromptDetailsStateUpdated is set, and PromptCard.jsx:379 passes promptDetails={promptDetailsState} down to Header. The ToolIde.jsx:333-350 comment documents the enable-direction gap honestly ("'toggle single pass -> every affected prompt warns at once' does NOT hold"), and its cross-references check out. But the disable direction is unhandled and unmentioned, and the fix for it is already in scope: singlePassExtractMode is destructured live from the store at Header.jsx:104 and simply not used in this guard.
• Suggested fix: gate this render on singlePassExtractMode && unresolvableVariables.length > 0 — one token, live signal already present — which fixes the stale-warning direction outright. The enable direction needs PromptCard to accept prop updates after its first seed.
• Confidence: High.

[chandrasekharan-zipstack]

[Medium] [Lens 9] — Each poll tick writes two fresh arrays into the store, re-rendering every Prompt Studio consumer

• Failure mode: this call reaches handleIndexStatus (:324-332), which does updateCustomTool({ rawIndexStatus: data }) with a freshly-mapped array every time, even when the status is byte-identical to the previous tick. useCustomToolStore() is consumed without selectors across the tree (Header.jsx:102-110, PromptCard.jsx:61-68, and others), so every subscriber re-renders every 5 seconds for the whole duration of indexing — and forever in the case described in finding connectors #3.
• Evidence: :389-396 builds indexStatus with data.map(...) then always writes; there is no equality check. Store identity is otherwise stable (frontend/src/store/custom-tool-store.js:68-70 merges partials), so these two writes are the sole per-tick churn.
• Suggested fix: skip the updateCustomTool call when the newly computed status is deep-equal to the current one.
• Confidence: High.

[chandrasekharan-zipstack]

[Medium] [Lens 1, 3] — This hint still reads "0 tokens" when editing an existing profile

• Failure mode: setTokenSize and setMaxTokenSize are called only inside handleLlmChangeForTokens (:374-392), which is wired exclusively to the LLM dropdown's onSelect at :502. Open an existing profile for edit and touch nothing: this line renders "0 tokens, Max: 0" directly beneath a Chunk Size field showing 2048. UN-3137 is specifically about this hint being wrong, and it remains wrong on the edit path — now stated more assertively than before, since the old text hedged with ~= 0.0k.
• Evidence: :154-155 initialises both to 0; :502 is the only onSelect={handleLlmChangeForTokens}; the edit path populates the form from editedProfile?.chunk_size at :134 without touching either piece of state.
• Suggested fix: seed both from the edited profile on mount, or drop the mirrored state entirely and derive the hint from the form value (see the separate Low finding on :404-406).
• Confidence: High.

[chandrasekharan-zipstack]

[Medium] [Lens 2] — Completion is inferred from an audit timestamp because the endpoint exposes no status

• Failure mode: the root cause of the mid-run retirement bug is architectural. IndexManager has no status field (backend/prompt_studio/prompt_studio_index_manager_v2/models.py:22-72) and its serializer is a bare fields = "__all__", so the client reverse-engineers "did this run finish" from modified_at. The compensating machinery — a Map mutated across async boundaries, passed by reference into this options bag on a function shared with the user-initiated calls — is the pattern the next engineer copies for the next status the API does not expose.
• Suggested fix: add an explicit index status/state to the document-index response and let the client read it. Failing that, at minimum keep the poll's retirement logic out of this shared fetch helper.
• Confidence: Medium — this is a precedent judgment, not a defect independent of the mid-run retirement finding.

[chandrasekharan-zipstack]

[Low] [Lens 16] — This comment overstates the old bug by 1000x

• Failure mode: the comment claims the old hint "under-reported the real size by ~4096x". The old template appended a k suffix — extra={`~= ${tokenSize}k tokens, Max: ${maxTokenSize}`} — so chunk_size = 2048 rendered as ~= 0.5k tokens, i.e. 500 against an actual 2048. That is ~4.1x, not 4096x. A maintainer sizing the blast radius of the old bug from this comment would be off by three orders of magnitude.
• Suggested fix: ~4x.
• Confidence: High.

The comment's substantive claim — that chunk_size is a token count — is correct, and I verified it: the value reaches SentenceSplitter.from_defaults(chunk_size=...) via unstract/sdk1/src/unstract/sdk1/index.py:443-448 → unstract/sdk1/src/unstract/sdk1/vector_db.py:137-144.

[chandrasekharan-zipstack]

[Low] [Lens 15] — tokenSize is now a pure mirror of the form field

• Failure mode: with the conversion removed, toTokenSize (:415) is identity-with-clamp. A useState (:154), this onChange handler, and that helper now exist solely to echo the field's own value back underneath it at :531. This is also what makes the "0 tokens on edit" finding possible — the mirror can fall out of sync with the thing it mirrors.
• Suggested fix: Form.useWatch("chunk_size", form) and delete the state, the handler, and the helper. maxTokenSize still needs its own state.
• Confidence: High.

[chandrasekharan-zipstack]

[Low] [Lens 3] — A negative chunk size is clamped in the hint but still submitted

• Failure mode: Math.max(0, ...) in toTokenSize (:415) only affects the display. This input carries no min, and chunk_size is a plain IntegerField with no validators (backend/prompt_studio/prompt_profile_manager_v2/models.py:92), so a negative value persists and surfaces later as an indexing-time failure.
• Evidence: the comment at :414 documents this as known — "The field has no min, so a typed negative is clamped rather than shown."
• Suggested fix: min={0} on the input.
• Confidence: High.