Skip to content

Commit 2f57cf6

Browse files
j15zclaude
andcommitted
refactor(copilot): quality pass over the parser and its tests
Four changes from a reuse/simplification/efficiency/altitude review. No behaviour change; 162 tests unchanged. blankJsonStringLiterals returns the body untouched when it contains no quote. No quote means no string literal, so the loop was copying the body to itself character by character. unclosedTagCannotResolve had learned to short-circuit before calling it; literalTextReason had not, and blanked unconditionally on every already-rejected tag on every later chunk. Putting the guard inside the helper fixes both callers at once. inspectWithin and inspectFrom were near-duplicates added at different times. One function with an optional start expresses both, and keeps the property the second one existed for: slice once, already bounded, never `content.slice(bodyStart)` first and bound after. The frame-replay property was 1727ms — 95% of the file's test time — because it parses every prefix, so message length multiplies into parse count, and the fragment pool included a 6KB filler. Retraction happens AT a frame boundary, not as a function of message size, so that property now draws from the short fragments; the window-crossing filler still gets coverage from the properties that parse each message once. 1727ms to 205ms, same seeds, same assertions. It also now calls replayFrames instead of hand-rolling the stepping loop it already had a helper for. Deferred deliberately, each with a reason: - Collapsing `prose-nested-marker` into `nested-marker`. Its own docstring names the simplification and defers it; it is a behaviour change and wants its own commit and test, which is the pattern the rest of this branch follows. - Bounding hasSpecialTagMarker on the prose path. Costs tens of microseconds on a long reasoning body, but a marker past the bound would flip that body from released to suppressed — a retraction, which is the bug two commits back. - Splitting the parser out of this `'use client'` file. The strongest structural finding: the file cannot be imported by server code, which is why the inbox executor carries its own thinking-strip regex. It is a mechanical move with no logic change and deserves its own PR, not commit 25 of this one. - Generalising the backtick sanitizer past `workspace_resource`, and replacing it with parser-owned backtick consumption. Same reasoning: real, and not here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent ced378a commit 2f57cf6

2 files changed

Lines changed: 31 additions & 29 deletions

File tree

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.ts

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -823,11 +823,23 @@ describe('parser properties', () => {
823823

824824
const pick = <T>(rng: () => number, xs: T[]): T => xs[Math.floor(rng() * xs.length)]
825825

826-
function buildLossless(rng: () => number): string {
826+
function buildLossless(rng: () => number, pool = LOSSLESS_FRAGMENTS): string {
827827
const n = 1 + Math.floor(rng() * 5)
828-
return Array.from({ length: n }, () => pick(rng, LOSSLESS_FRAGMENTS)).join('')
828+
return Array.from({ length: n }, () => pick(rng, pool)).join('')
829829
}
830830

831+
/**
832+
* The same shapes without the window-crossing filler.
833+
*
834+
* Frame replay parses every prefix, so message length multiplies into parse
835+
* count — the filler fragment alone took that property to ~1M parses and 1.7s,
836+
* 95% of this file's runtime. Retraction is a property of what happens AT a
837+
* frame boundary, so it is exercised by the boundaries, not by message size.
838+
* The scan window still gets its coverage from the other properties, which
839+
* parse each message once.
840+
*/
841+
const SHORT_FRAGMENTS = LOSSLESS_FRAGMENTS.filter((fragment) => fragment.length < 200)
842+
831843
it('never loses a character of a message with nothing droppable in it', () => {
832844
// The headline guarantee. Only a well-formed payload that failed its shape
833845
// guard may be removed, and no fragment here is one.
@@ -876,22 +888,20 @@ describe('parser properties', () => {
876888

877889
for (let seed = 1; seed <= 120; seed++) {
878890
const rng = makeRng(seed)
879-
const raw = `${buildLossless(rng)}${pick(rng, VALID_TAGS)}${buildLossless(rng)}`
891+
const raw = `${buildLossless(rng, SHORT_FRAGMENTS)}${pick(rng, VALID_TAGS)}${buildLossless(rng, SHORT_FRAGMENTS)}`
880892

881893
let previousCards = 0
882894
let previousText = ''
883-
for (let end = 1; end <= raw.length; end += 7) {
884-
const view = visibleView(parseSpecialTags(raw.slice(0, end), true).segments)
885-
886-
expect(view.cardCount, `seed ${seed} at ${end}: card un-rendered`).toBeGreaterThanOrEqual(
895+
for (const frame of replayFrames(raw, 7)) {
896+
expect(frame.cardCount, `seed ${seed}: card un-rendered`).toBeGreaterThanOrEqual(
887897
previousCards
888898
)
889899

890900
const stable = previousText.slice(0, Math.max(0, previousText.length - LONGEST_OPENER))
891-
expect(view.text.startsWith(stable), `seed ${seed} at ${end}: text retracted`).toBe(true)
901+
expect(frame.text.startsWith(stable), `seed ${seed}: text retracted`).toBe(true)
892902

893-
previousCards = view.cardCount
894-
previousText = view.text
903+
previousCards = frame.cardCount
904+
previousText = frame.text
895905
}
896906
}
897907
})

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.tsx

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,11 @@ const MAX_UNCLOSED_BODY_SCAN = 4096
551551
* emitting one would shrink the output and shift every later offset left.
552552
*/
553553
function blankJsonStringLiterals(body: string): string {
554+
// With no quote there is no string literal, so the loop below would copy the
555+
// body to itself character by character. Both callers reach here on bodies
556+
// that are usually plain prose, and this runs per opener per streamed chunk.
557+
if (!body.includes('"')) return body
558+
554559
let out = ''
555560
let inString = false
556561
let escaped = false
@@ -810,24 +815,11 @@ interface InspectedBody {
810815
truncated: boolean
811816
}
812817

813-
function inspectWithin(body: string): InspectedBody {
814-
return body.length > MAX_UNCLOSED_BODY_SCAN
815-
? { text: body.slice(0, MAX_UNCLOSED_BODY_SCAN), truncated: true }
816-
: { text: body, truncated: false }
817-
}
818-
819-
/**
820-
* {@link inspectWithin} for a body that has not been cut out of the buffer yet.
821-
*
822-
* Slices once, already bounded. Taking `content.slice(bodyStart)` first and
823-
* bounding after copies the entire rest of the message — on every opener, on
824-
* every streamed chunk — and throws all but the window away.
825-
*/
826-
function inspectFrom(content: string, bodyStart: number): InspectedBody {
827-
const end = bodyStart + MAX_UNCLOSED_BODY_SCAN
828-
return end < content.length
829-
? { text: content.slice(bodyStart, end), truncated: true }
830-
: { text: content.slice(bodyStart), truncated: false }
818+
function inspectWithin(source: string, start = 0): InspectedBody {
819+
const end = start + MAX_UNCLOSED_BODY_SCAN
820+
return end < source.length
821+
? { text: source.slice(start, end), truncated: true }
822+
: { text: start === 0 ? source : source.slice(start), truncated: false }
831823
}
832824

833825
/**
@@ -972,7 +964,7 @@ function resolveTagAt(
972964
const closeIdx = memoizedIndexOf(closeCache, content, closeTag, bodyStart)
973965

974966
if (closeIdx === -1) {
975-
const inspected = inspectFrom(content, bodyStart)
967+
const inspected = inspectWithin(content, bodyStart)
976968
if (isStreaming && !unclosedTagCannotResolve(tagName, inspected.text)) {
977969
return { outcome: 'pending' }
978970
}

0 commit comments

Comments
 (0)