Repository navigation
fix(portal): keep the preview up and report a broken specification while serving - #434
Conversation
Under `vite dev`, one document saved half written failed the module every route imports, so every page of the preview answered 500 until it was fixed. The preview now leaves that document's reference pages out and serves the rest; a build still fails on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… document cannot be read Startup read only the documents at the top of `spec/`, so a split specification whose `$ref` named a missing or half-written file passed the check, then failed late in the build or rendered with that reference empty. The files each document refers to are now followed as the portal's bundler follows them, each read once and in parallel, and one that is missing or unreadable is refused, naming the file that refers to it. A document that does not parse is reported with the parser's reason, which carries its line and column. A table of cases holds the check to `bundleSpec`. The preview's dev-only handling in `openApiSource` is inlined. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…runs A specification saved with a mistake left its reference out of the preview and said nothing. Each save in `spec/` is now checked as `portal generate` checks it: a mistake is reported with the reason, the next good save says it is fixed, and a document added or removed is warned of once, since the preview serves only the documents it started with. The live preview note says so. The "refused, then fixed once" state the content and config previews each kept is one type now, and the sentences the watch messages shared live once in the prompts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ec-errors #405 added the same `resolveSpecs` for quickstart, so one is kept for both callers, and the test this branch had for it gives way to the ones #405 brought. `specRejected` drops the `offerQuickstart` option #405 removed from `reportSourceProblem`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…w's allow list Vite resolves a symlink before comparing a requested file against `server.fs.allow`, and the static directory was listed as written. A project reached through a symlink, as anything under macOS's `/tmp` or `/var` is, had a static file asked for before its page refused with 403, which is the request apimatic-io#2287 is about. The directory is now named by its real path, as the dependency directories beside it already are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the review The check read a file by its extension, and took the `$id`s a `$ref` may name from the file the `$ref` sat in, so a referenced `.json` holding YAML, or a `$ref` naming an `$id` of the root document, was refused although the portal builds it. Every file is now read as the bundler reads it, by what it holds, and one the bundler fails on or reads as nothing is refused with the reason. The `$id`s a reference may name are the root document's, as in the bundler. A document gone from `spec/` mid-save is reported as unreadable rather than left out, which had the preview warn that it was removed. `specs()` is back within Sonar's cognitive complexity limit, the watch messages say what the preview leaves out, and a test replaces the comment on the order in which `PreviewConfig.show` accepts a refusal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the preview's messages The `$ref` walk leaves the source context for `SpecBundle`, a plain type that records each file read and lists the ones still to read, while the context reads them a level at a time. A spec problem's reason is a kind the prompt words, so a document gone mid-save is "no longer there" rather than an ENOENT message, and a missing referenced file is a `MissingFile`, which names its spelling on disk in another case. The 50 characters the bundler looks at for JSON are a named constant, held to the bundler by a row of its table. The preview's watch messages are one prompt each, keyed by what is watched, and each preview type's answer goes straight to its prompt. A save of `apimatic.json` that throws now says its changes could not be checked, and a lost watch on it no longer says "further edits". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ec-errors #435 rewrote the same spec reading. Its folder discovery is kept, and every file it reads goes through this branch's reader, so the documents of a merged folder get the `$ref` checks and the reasons. #435's check for an operation declared twice moves into `SpecBundle.endpoints()`, with a test there, and the `specDirectory` both branches added is kept once. Serve's warnings now hold when `APIMATIC-META.json` changes which folders merge: the documents are named by their path from `spec/` and said to be "now read" or "no longer read", and a refused specification says the preview "shows what it can still read". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mehnoorsiddiqui
left a comment
mehnoorsiddiqui
left a comment
There was a problem hiding this comment.
Reviewed and tested this on Windows, with a case-sensitive NTFS folder standing in for Linux.
Results: tsc (src + tests), ESLint and pnpm build are clean; the unit suite is 1938 passing, and the APIMATIC_E2E=1 serve/build suite is 68 passing. I also ran resolveSpecs on dev and on this branch over ~4,900 files from 22 real specifications. Both give the same specs and operations everywhere except one, where this PR correctly refuses a broken YAML file that dev accepted and that bundleSpec fails on. Nice catch.
The specifics are inline. I'd treat the first three (case in $refs, top-level files refused, $refs that pass the check but fail the bundler) as worth fixing before merge; the rest are follow-ups or nits.
Not anchored inline:
-
Unrelated changes.
src/actions/action-result.tsandsrc/prompts/api/transform.tschange only formatting. The static-directory symlink fix inportal-project-service.tsis a separate fix. It works: I checked it through a Windows junction, where the config and the allow list both get the real path. It would still be easier to review and revert as its own PR. -
Performance. The check now reads every referenced file, and it runs on every save in
spec/. Second-run timings on Windows (dev→ this PR):Specification devthis PR generated split spec, 1,304 files 100 ms 341 ms real split spec, 651 files 186 ms 406 ms 9 MB single JSON 204 ms 483 ms chain of 1,000 files 16 ms 465 ms That's about 2–3.5× slower on real specs, and much slower on deep chains, where the reads are sequential by nature. It's fine today, but worth keeping an eye on. Timings varied by 20–40% between runs.
-
The description's known limits are confirmed: a save to a file outside
src/specisn't watched (a broken file there is reported only on the next save insidespec/), and nothing below an$idis checked.
| } | ||
| contents = await this.fileService.getContents(file); | ||
| } catch (error) { | ||
| const there = await this.fileService.fileExists(file); |
There was a problem hiding this comment.
A $ref in the wrong letter case is only caught on a case-sensitive file system.
missingFile() only runs when getContents throws. On Windows/macOS, $ref: ./schemas/pet.yaml with Pet.yaml on disk reads fine, so the check accepts it, and bundleSpec builds locally too. On a case-sensitive file system the bundler fails. I verified that on a case-sensitive NTFS folder: the bundler fails, this check then reports "spelt 'Pet.yaml'", and dev accepts it.
While serving on Windows, restoring a deleted file in the wrong case even prints "'src/spec' is fixed; a build would accept it again".
Images and static files call missingFile() unconditionally (missingImages, missingStaticFiles), so references in specs behave differently from them. Suggest checking the spelling of every referenced file, caching the directory listings per folder so split specs stay fast.
There was a problem hiding this comment.
Fixed in 9291028. Every file read below src is now checked against its spelling on disk, listing each folder once per check (FileService.spellingsOnDisk), so a $ref in the wrong case gets the same message on Windows and macOS as on Linux, and restoring a file in the wrong case no longer prints "is fixed". Files outside src aren't compared. This does refuse on Windows and macOS a $ref whose local build succeeds, as Markdown images in the wrong case already are; the description now says so.
| if (document === undefined) { | ||
| return err({ kind: 'unreadableSpec', file }); | ||
| const read = await this.readDocument(file); | ||
| if (read.isErr()) { |
There was a problem hiding this comment.
This is stricter than the build for files at the top of spec/ and of merged folders.
An empty, comment-only or key-less .yaml here now fails resolveSpecs with unreadableSpec (empty / noKey), where dev ignored it. Such a file is never bundled, and bundleSpec on the definition beside it builds fine. I reproduced this at the top of spec/ and inside a merged folder. While serving, dropping an empty notes.yaml into spec/ now reports an error.
I see the description says this is intended. I'm flagging it because it refuses something the build accepts, and it could break existing projects on upgrade. Suggest refusing only the files that get bundled (definitions and their $ref targets), and keeping the old behaviour for the rest.
There was a problem hiding this comment.
Agreed, fixed in 9291028. A file at the top of spec/ or of a merged folder that holds nothing, only comments or no key is skipped again, as on dev. One addition: when every document there holds nothing, the first is still reported as empty, as otherwise a document caught empty mid-save would be pointed at api transform. A file a $ref names is still refused when empty, since the build embeds an empty reference for it.
| } | ||
|
|
||
| /** The file a `$ref` names, resolved as the bundler resolves it; undefined for none, or a URL. */ | ||
| function localFile(directory: DirectoryPath, name: string): FilePath | undefined { |
There was a problem hiding this comment.
Some $refs pass this check but fail the bundler.
I compared this check with the real bundleSpec on 69 cases: 57 agree, 2 are the intentional top-level refusals, and 10 are accepted here although the bundler fails or reads nothing. These make bundleSpec throw:
$ref: ""anywhere, and$ref: "#"in the root document. The bundler resolves the empty file part to the folder and fails to read it.file://…andurn:…references, and a name such asab:c.yaml.URL_SCHEMEskips anything with a scheme, but the bundler only fetcheshttp:/https:and reads everything else as a file path.- a reference to a schema by
$anchor(#pet). This one is a json-magic quirk, but the outcome is the same. - and the cases already known: the two
$idcases, and anhttpURL that can't be reached.
(The 10th, a missing pointer such as ./p.json#/nope, builds and the reference page still renders, so it's harmless.)
Together with the dev-only allSettled in openApiSource, these mistakes are now silent while serving. I verified this with a live dev server: after saving $ref: '', the CLI prints nothing and the operation's reference page disappears, while the other pages keep serving. On dev, the same save made every page answer 500, which was at least visible.
Suggest treating only http:/https: as URLs here, refusing an empty file part (and a bare # in the root), and adding these rows to the bundler table in portal-source-context.test.ts.
There was a problem hiding this comment.
Fixed in 9291028, with your cases added to the bundler table. Only http: and https: are taken for URLs now, so file:, urn: and ab:c.yaml are read as paths, as the bundler reads them. An empty $ref is refused in any file, and a bare # or an anchor in the root document, as unresolvableSpecReference; in an embedded file the bundler turns those two into pointers, and the table confirms that builds. The missing pointer is in the table as a case that builds. The $id and unreachable-URL cases stay under "Not in this PR".
| const sections = import.meta.env.DEV | ||
| ? (await Promise.allSettled(loading)).flatMap((result) => { | ||
| if (result.status === 'fulfilled') return [result.value]; | ||
| console.error('[OpenAPI] Leaving its reference pages out of the preview:', result.reason); |
There was a problem hiding this comment.
Two follow-ups on this path:
- This
console.errorgoes into the dev server's output, whichPortalDevServerServicecaptures and shows only after a crash. Whenever the CLI check passes butbundleSpecthrows (see the comment onlocalFile), nothing tells the user why the reference disappeared. It's worth getting this message to the terminal. prerenderPagesinvite.config.tsstill loads every spec withPromise.allwhen the config loads, undervite devtoo. A spec in that state therefore stops the preview from starting ("The portal preview stopped unexpectedly"). I confirmeddevdoes the same, so it isn't a regression, but "the preview survives a broken specification" only holds for edits made after startup.
There was a problem hiding this comment.
- Agreed it's a gap. With the check now matching the bundler on the cases you found, what this leaves silent are the
$idcase, an unreachable URL, and what only the portal refuses, such as a duplicateoperationId. Getting the preview's own error into the terminal means picking lines out of the server's output, so I've left it as a follow-up; it's in the description. - Fixed in 9291028:
vite devno longer builds the prerender page list, so a spec the bundler fails on no longer stops the preview from starting. A new e2e test starts the preview with one (a$refbelow an$id); it failed with "stopped unexpectedly" before the fix.
|
|
||
| // As for content: an editor's swap file, saved while a document is refused, would only repeat the refusal. | ||
| return this.startWatch('spec', sourceDirectory, check, (onChange, onFailed) => | ||
| this.fileWatchService.watchTree(source.specDirectory, onChange, onFailed, isSkippedByGlob) |
There was a problem hiding this comment.
On Windows, deleting spec/, or renaming it away and back, silently ends this watch. Later saves are never checked, and onFailed never fires, so there's no "no longer watched" warning either.
I reproduced it with this watch, and with FileWatchService.watchTree directly on dev (the content watch has the same gap), so the cause predates this PR. A whole spec folder is just more likely to be replaced than content/. Not tested on Linux/macOS, which use a different code path in watchEachDirectory.
There was a problem hiding this comment.
Thanks for tracing it to watchTree on dev. Since the content/ watch has the same gap, I'd rather fix it in the file watch for both than here, so I've left it as a follow-up; it's listed in the description.
| const checked = await sourceContext.resolveSpecs(); | ||
| if (checked.isErr()) { | ||
| preview.refuse(); | ||
| this.prompts.specRejected(checked.error, sourceDirectory); |
There was a problem hiding this comment.
Two noisy cases, both reproducible on Windows:
- Deleting a referenced file prints the same refusal twice (6 out of 6 runs). Consider not repeating a refusal identical to the last one reported.
- An editor that saves by deleting and rewriting the file, with a gap longer than the settle time (200 ms in my runs), gets "'src/spec' has no API definition … Move it up, or … set 'MergeConfiguration.MergeApis' …" and then "is fixed" straight after. The folder advice is misleading for a document caught mid-save.
There was a problem hiding this comment.
Both come from timing, so I've left them and listed them in the description. The repeated refusal is a watch event landing while the check runs, which queues one more check; dropping a refusal identical to the last would also hide the answer to a real second save. For the slow delete-and-rewrite, at that moment the folder really has no definition, and the next save says it's fixed; telling a mid-save from a deletion would mean the watch waiting longer on every save.
| /** Records the documents as shown, and answers what to say of them. */ | ||
| public show(specs: PortalSpec[]): SpecsShown { | ||
| const files = specs.map(({ file }) => file); | ||
| const unchanged = isSameSet(files, this.shown) || isSameSet(files, this.served); |
There was a problem hiding this comment.
When a removed document comes back, so the files match served again, nothing is said. The user was told to restart the preview and never hears that it's no longer needed. A line such as "the documents match the ones the preview started with again" would close the loop.
There was a problem hiding this comment.
Done in 9291028. A return to the documents the preview started with now says "The documents read from 'src/spec' are the ones the preview started with again; it no longer needs a restart."
| const read = Result.combine( | ||
| await Promise.all(unread.map((next) => this.readDocument(next.file, next.referencedBy))) | ||
| ); | ||
| if (read.isErr()) { |
There was a problem hiding this comment.
Only the first broken file is reported. On a real split spec (1,280 files) with two broken YAML files, the second showed up only after the first was fixed. content/ reports every problem in one run (invalidContent.problems). Collecting every unreadable or missing reference would save the user repeated fix-and-rerun rounds.
There was a problem hiding this comment.
Agreed it would save rounds. It needs the spec problems reshaped into a list, as invalidContent is, so I've kept it out of this PR and listed it as a follow-up. SpecBundle.unresolvable() already keeps every unresolvable $ref for it.
| const there = await this.fileService.fileExists(file); | ||
| if (!there && referencedBy !== null) { | ||
| // Null only for a name listed but not there as a file, as a dangling link is. | ||
| const missing = (await this.missingFile(file)) ?? { file, foundAs: null }; |
There was a problem hiding this comment.
Minor: a $ref that names a folder (./schemas) ends up here and is reported as "there is no such file", which is misleading.
Also, for a target outside the source directory, spelledOnDisk only returns null because .. never matches a subfolder name. The result is right, but an explicit guard would make the intent clear.
There was a problem hiding this comment.
Both done in 9291028: a $ref that names a folder now says "could not be read: it is a folder", and directorySpelledOnDisk answers null for a folder outside its root explicitly.
| } | ||
|
|
||
| /** The files named but not read yet, each with the first file to name it. */ | ||
| public unread(): ReferencedFile[] { |
There was a problem hiding this comment.
Nit: unread() rescans every named file on each level. On a 1,000-file chain that's 1,001 calls scanning 501,500 entries, about 10% of the check (41–57 ms out of 364–537 ms). A pending list that add() pushes to would remove it. Most of the remaining cost of a chain is unavoidable, since each file is found only after the previous one is read.
There was a problem hiding this comment.
Done in 9291028: SpecBundle keeps the files still to read in a list add() pushes to. Measured back to back, a chain of 1,000 files checks about 18% faster.
Fixes from mehnoorsiddiqui's review: - Every file a $ref names below src is checked against its spelling on disk, each folder listed once, so a $ref in the wrong case is refused on Windows and macOS as it is on Linux. - A file at the top of spec/ or of a merged folder that holds nothing, only comments or no key is skipped again, since the build never reads it. When every document there holds nothing, the first is reported as empty rather than pointed at a conversion. - Only http and https $refs are taken for URLs, as the bundler fetches only those. An empty $ref, and a bare # or an anchor in the document bundled, are refused as unresolvableSpecReference, and a $ref that names a folder says so. - While serving, a return to the documents the preview started with says no restart is needed. - SpecBundle keeps the files still to read, rather than scanning every file named at each level. - vite dev no longer lists the pages to prerender: the list bundled every spec at startup, so one the bundler fails on stopped the preview from starting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the thorough review. Replies are on each thread, and the fixes are in 9291028, with the description updated.
|
…ec-errors Brings in #439 (Fumadocs components on content pages), #427 (dead code and isomorphic-git removed) and #440 (a portal built from another drive keeps its styling). - src/actions/action-result.ts: dev's version, without the methods #427 removed, formatted as the commit hook formats it. - test/e2e/portal-serve.test.ts: #440 removed the helpers that put a project on the fixture's drive, so each project is now built in the temp directory, as #440 does. The main preview serves the checkout's fixture, as #440's test does, which keeps a CI runner's cross-drive case. The tests that edit the specification each preview a copy of the fixture reached through a link. #440's static image test runs against both, and a test of a static file asked for by its real path through the link keeps the allow-list fix of this branch covered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
2 participants
The problem
Under
portal serve, a specification saved half written, a broken or missing file of a split specification, or a deleted specification made every address of the preview answer 500, Markdown pages included. The terminal said nothing, and the open tab needed F5 once the file was fixed.Before the preview starts, and in
portal generate, only the documents at the top ofspec/were checked. A file that a$refnamed, missing or half written, passed the check and failed the build later, and a document that did not parse was reported with no reason.What changes
vite dev,openApiSource()loads each document on its own and leaves out the reference pages of one that fails, so the rest of the portal is served. A build still fails on it.vite devno longer lists the pages a build prerenders: the list bundled every specification at startup, so one the bundler fails on stopped the preview from starting.$refnames, is read by what it holds, as@scalar/json-magicreads it: JSON, else YAML when the text holds a key and does not open as JSON does within its first 50 characters. A file a$refnames that the bundler fails on, or reads as nothing (an empty file, one holding only comments), is refused asunreadableSpec, with the reason: the parser's own words for a syntax error, and the CLI's for the rest, such as "it is empty". The parser used to be picked by the file's extension.dev: a.jsonfile holding YAML is accepted, as the portal builds it. A file at the top ofspec/, or of a folderAPIMATIC-META.jsonmerges, that holds nothing, only comments or no key is skipped, as ondev, since the build never reads it; an empty.jsonthere, whichdevrefused, is skipped too. When every document there holds nothing, as one caught empty mid-save does, the first is reported as empty rather than pointed at a conversion.$ref. The files each document refers to are followed as the bundler follows them, each read once per document, a level of references at a time and in parallel within a level. One that is missing is refused asmissingSpecReference, and one that cannot be read asunreadableSpec, each naming the file that refers to it.srcis checked against its spelling on disk, each folder listed once per check, so a$refin the wrong case is refused on Windows and macOS as it is on Linux, where the build fails on it. Compared withdev, this refuses on Windows and macOS a$refwhose local build succeeds, as Markdown images in the wrong case already are.$refon any object or list, resolved from the folder of the file it is written in, but not past a$ref's siblings orx-ext/x-ext-urls. Onlyhttp:andhttps:are fetched; any other scheme,file:andurn:included, is read as a path. A$refthat names an$idof the root document names a schema, not a file, in every file the document is bundled from.$refthat names no file. An empty$reffails in any file, and so do a bare#and an anchor such as#petin the document bundled (in a file it embeds, the bundler turns them into pointers). These are refused asunresolvableSpecReference. A$refthat names a folder is refused, saying so.bundleSpecand the check on the same files, so the two cannot drift apart. Each row says whether the bundler builds, fails, or reads a file as nothing, which leaves its reference empty; the check accepts only what builds.SpecBundlerecords each file read and lists the ones still to read, and answers the document's operations; the source context does the reading.APIMATIC-META.jsonmerges (feat(portal): build a portal from the spec folders APIMATIC-META.json merges #435) are checked the same way. The merge withdevkeeps feat(portal): build a portal from the spec folders APIMATIC-META.json merges #435's discovery of the merged folders and reads each of their documents through this check, so a merged folder's document has its$refs followed and is refused with a reason when it cannot be read. feat(portal): build a portal from the spec folders APIMATIC-META.json merges #435's check for an operation declared twice in one document now lives inSpecBundle.endpoints().OpenApiDocument.parsereturns aResultcarrying the parser's message on one line, with its line and column.spec/is checked while serving, against the same rules:portal generatereports it, and the next save that fixes it says so once;spec/, by a document added or deleted or by a change to which foldersAPIMATIC-META.jsonmerges, is warned of once, naming each document by its path fromspec/, since the preview serves only the documents it started with; a return to those documents says the restart is no longer needed;PreviewRefusalreplaces the flagPreviewContentandPreviewConfigeach kept. A message the watches ofapimatic.json,content/andspec/share is one prompt, keyed by what is watched, and what each preview says after an accepted save is decided in its prompt. Two sentences change forapimatic.json: a fault while checking a save now says its changes "could not be checked" (it said "could not be applied to the preview"), and a lost watch no longer says "further edits".server.fs.allow, as the dependency directories already are: Vite resolves a symlink before comparing, so under macOS's/tmpor/vara static file asked for before its page, as Safari does in apimatic-io issue 2287, was refused with 403. The serve e2e test found it on macOS. A serve e2e test now asks for a static file by its real path, through a link to a copy of the fixture, and fails without the fix on every OS.devreformattedsrc/actions/action-result.ts,src/prompts/api/transform.ts,src/infrastructure/launcher-service.tsandsrc/types/file/urlPath.ts: the commit hook ran Prettier over them, which changed quotes and line breaks only.What it prints
Testing
pnpm test: 1964 passing, with the source and the tests type-checked and ESLint clean. New tests cover each$refrule, how a file is read, the bundler table, the spelling check,SpecBundle, the messages, the two preview state types, and the spec watch inportal serveon a copy of the fixture.APIMATIC_E2E=1serve and build: 73 passing, feat(portal): build a portal from the spec folders APIMATIC-META.json merges #435's merged-folder builds included. The serve tests preview the checkout's fixture, built in the temp directory (another drive on a Windows runner), and copies of it reached through a link. One serve test saves a specification half written, checks that its reference is left out while a Markdown page is still served, and that the reference comes back once the file is fixed. Another starts the preview with a specification the bundler fails on (a$refbelow an$id), which stopped the preview from starting before, and checks its Markdown pages are served without its reference.dev, the check and the serve messages were driven on copies of feat(portal): build a portal from the spec folders APIMATIC-META.json merges #435's merged-folders fixture: a broken$refand an empty file in a merged folder are refused, a$refinto a sibling folder that is not merged reads, and dropping a folder fromMergeOrderOfDirectoriesor turningMergeApisoff is warned of as above. On a project of 28 merged specifications, the check that runs on each save takes about 0.2 s.portal servesessions, with a browser tab left open, on a single-file specification and on a split one in a folder named with a space and an accent: a half-written root document, a renamed, added or deleted document, a missing$ref'd file, half-written YAML and JSON in referenced files, and CTRL+C. All were reported as above, and the open tab recovered without F5. These ran before 865ac82 and ecd4b0e, which changed how a file is read and some of the wording.spec/. mehnoorsiddiqui measured it at 2 to 3.5 timesdev's time on real specifications, and more on a deep chain, where each file is found only after the one before it is read. Measured back to back with and without each change, the spelling check costs no more than the noise between runs, and keeping the files still to read makes a chain of 1,000 files about 18% faster.Not in this PR
operationId, is still not reported while serving. Its reference is left out rather than failing every page.content/lists every problem in one run.$refnames outsidesrc/specare checked, but saving one is not watched.$refbelow an$idis not checked: the bundler resolves it against the$id, which names no folder of the specification, so it is left to the build.$refis broken, the broken$refis reported before the folder's two definitions are.spec/ends its watch without a word, as it does forcontent/; the watch has done so since before this PR.🤖 Generated with Claude Code