Repository navigation
feat(studio): flip the SDK cutover and the GSAP writer to default-on #4055
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
26ea8a0
407cacf
9ef37e5
9f3a778
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,7 +181,15 @@ export function useSdkSession( | |
|
|
||
| readProjectFileOptional(projectId, activeCompPath) | ||
| .then(async (content) => { | ||
| if (cancelled || typeof content !== "string") return; | ||
| if (cancelled) return; | ||
| if (typeof content !== "string") { | ||
| // No SDK session follows, so EVERY cutover chokepoint below takes the | ||
| // server path and emits nothing — the shadow never runs either. This | ||
| // is the only place a missing session can originate, so a broken read | ||
| // would otherwise be a silent, total SDK bypass. | ||
| trackStudioEvent("sdk_session_unavailable", { stage: "read" }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
One is a security event, one is normal, one pages someone. For a commit titled "report why a fallback happened," All three branches sit in one small function, so splitting the stage is cheap: |
||
| return; | ||
| } | ||
| // No persist queue: Studio's writeProjectFile (via sdkCutover's | ||
| // persistSdkSerialize) is the SINGLE writer. Wiring the SDK persist | ||
| // queue too would double-write the file (queue auto-writes on every | ||
|
|
@@ -206,6 +214,7 @@ export function useSdkSession( | |
| ) | ||
| ) { | ||
| disposeSdkSession(comp); | ||
| trackStudioEvent("sdk_session_unavailable", { stage: "ownership" }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other two stages are terminal — no session follows and every chokepoint takes the server path. This one is reached because a newer generation is installing its own session: That makes it the odd member of a counter whose stated meaning is "a total, otherwise-invisible SDK bypass." It inflates during comp switching and reload storms — and I read this rather than ran it, so on frequency I'll only claim the state is reachable — the guard exists because it is, and the refs it compares ( It's also the only one of the three stages with no test: Suggest dropping the emit — the newer generation reports its own outcome a moment later — or giving it a separate event name so the absence counter stays alertable. |
||
| return; | ||
| } | ||
| const displaced = ownedSessionRef.current; | ||
|
|
@@ -215,8 +224,17 @@ export function useSdkSession( | |
| setOwnedSession(installed); | ||
| if (displaced && displaced.session !== comp) disposeSdkSession(displaced.session); | ||
| }) | ||
| .catch(() => { | ||
| if (!cancelled && generationRef.current === generation) setOwnedSession(null); | ||
| .catch((error: unknown) => { | ||
| if (!cancelled && generationRef.current === generation) { | ||
| setOwnedSession(null); | ||
| // openComposition threw (unparseable composition, OOM) — same total | ||
| // bypass as the read failure above, but this one is a real defect | ||
| // rather than a missing file. Carry the message; it is the only clue. | ||
| trackStudioEvent("sdk_session_unavailable", { | ||
| stage: "open", | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| return () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning checks out — I verified the 409 revalidation at
files.ts:1310-1313really is outside the writer branch (after the mutation loop, before any write), so pinning this to recast drops no coverage of behavior acorn changes. The env mutation is safe too: noconcurrentin this file, vitest isolates per file, and thefinallyrestores.The gap is that the default path now has no test that the conflict is detected at all. It does not need acorn to grow an async seam, though: the revalidate is a plain
readFileSync(res.absPath, "utf-8"), so a spy that returns the successor bytes on the second read exercises the 409 under acorn without any interleave.