Skip to content

feat(assembler): on-demand assembler serve with live reload and improved frontend DX#3179

Open
Mpdreamz wants to merge 9 commits intomainfrom
feature/assembler-serve
Open

feat(assembler): on-demand assembler serve with live reload and improved frontend DX#3179
Mpdreamz wants to merge 9 commits intomainfrom
feature/assembler-serve

Conversation

@Mpdreamz
Copy link
Copy Markdown
Member

@Mpdreamz Mpdreamz commented Apr 24, 2026

Summary

This PR adds docs-builder assembler serve — on-demand page rendering for the full assembled documentation site, with live reload.

The problem

The full assembler build is fast (~30 seconds), which is perfectly fine for CI/CD. However, for local frontend development — iterating on CSS, JavaScript, or templates — even 30 seconds is too slow for a tight feedback loop. As a result, frontend work has always been done against isolated single-repo builds (docs-builder serve) rather than the real assembled site with its global navigation, cross-repo links, and assembler-specific styling. This means visual regressions in the assembled layout are only caught after a full build.

What this adds

docs-builder assembler serve — on-demand rendering with live reload

After running assembler clone (the only prerequisite), the serve command:

  • Loads cross-links from S3 and resolves all repository directory trees in memory
  • Starts an HTTP server on port 4000 that renders each page on demand via DocumentationGenerator.RenderLayout — no prior build needed
  • Routes incoming requests to the correct repository using NavigationTocMapping.SourcePathPrefix (longest-match), then looks up and renders the matching MarkdownFile
  • Watches all checkout directories for *.md, docset.yml, and toc.yml changes — on any change invalidates the affected repo's directory tree so the next request re-renders from the updated source, then fires a browser live reload via Westwind
  • Serves embedded CSS/JS/fonts at /{pathPrefix}/_static using EmbeddedOrPhysicalFileProvider (which in DEBUG builds serves directly from physical files in src/Elastic.Documentation.Site/_static/)

--no-watch-md flag

Skips setting up the 100+ FileSystemWatcher instances on checkout directories. Static-asset live reload still runs. The intended use case: frontend developers who only care about CSS/JS/template changes and don't need markdown change detection.

Static-asset live reload (DEBUG only)

In debug mode, AssemblerReloadService also watches src/Elastic.Documentation.Site/_static/ for CSS/JS changes and fires a browser live reload. Combined with EmbeddedOrPhysicalFileProvider serving from physical files in debug mode, this gives instant CSS/JS feedback against the fully assembled site.

./build.sh watch-full

Runs dotnet watch over the docs-builder project with assembler serve as the command. This is the primary frontend development loop: C# hot reload for template changes, on-demand markdown rendering, and static-asset live reload — all against the real assembled site.

docs-builder assembler serve-static

The previous assembler serve behaviour — serves the pre-built output of assembler build as static files with no watching or live reload. Renamed to make room for the new on-demand mode.

Bug fix: Ctrl+C hang on shutdown

AmazonS3Client (AWS SDK) spawns foreground threads for connection-pool management that permanently block process exit unless the client is explicitly disposed. Aws3LinkIndexReader.CreateAnonymous() was called in many places and the result was never disposed:

  • Aws3LinkIndexReader now implements IDisposable and forwards to s3Client.Dispose()
  • CrossLinkFetcher.Dispose() now also disposes its ILinkIndexReader if it is IDisposable
  • AssembleSources.AssembleAsync wraps the cross-link fetcher in a using block so the S3 client is released immediately after the fetch completes
  • DocSetConfigurationCrossLinkFetcher was creating a brand-new AmazonS3Client on every FetchCrossLinks() call (i.e. on every live-reload cycle in docs-builder serve); it now reuses the reader from the base class via the new protected LinkIndexProvider property

Workflow

# one-time setup
docs-builder assembler clone

# iterate on content — pages render on demand, browser live-reloads on .md changes
docs-builder assembler serve

# frontend work — full assembled site with CSS/JS live reload, no markdown watchers
docs-builder assembler serve --no-watch-md

# full frontend loop with C# hot reload (templates, CSS, JS — all live)
./build.sh watch-full

# serve a previously built assembly as static files (old behaviour)
docs-builder assembler serve-static

Test plan

  • assembler clone followed by assembler serve starts the server and renders pages on demand without a prior assembler build
  • Editing a markdown file in a checkout directory triggers a live reload and the updated content is served on the next request
  • assembler serve --no-watch-md starts without checkout directory watchers; static-asset live reload still works in debug mode
  • In debug mode (dotnet run --configuration debug), editing a CSS file in src/Elastic.Documentation.Site/_static/ triggers a browser live reload
  • assembler serve-static serves the pre-built .artifacts/assembly/ directory as before
  • ./build.sh watch-full starts dotnet watch with assembler serve
  • Ctrl+C exits cleanly without hanging

🤖 Generated with Claude Code

Mpdreamz and others added 2 commits April 24, 2026 15:08
Adds `assembler serve` for iterating on full assembled documentation
without running a full build first. After `assembler clone`, pages are
rendered on demand via DocumentationGenerator.RenderLayout. File changes
in checkout directories invalidate the affected repo's directory tree
and trigger a browser live reload via Westwind.

Also adds `assembler serve-static` (the former `assembler serve`
behaviour: serves pre-built .artifacts/assembly/ as static files) and
`./build.sh watch-full` (dotnet watch + assembler serve for C# hot
reload during tool development).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…h-md

Fixes indefinite hang on Ctrl+C: AmazonS3Client spawns foreground threads
that block process exit unless explicitly disposed. Aws3LinkIndexReader now
implements IDisposable, CrossLinkFetcher.Dispose() forwards to the reader,
and AssembleSources wraps the fetcher in a using block so the S3 client is
released immediately after cross-links are fetched.

Also fixes DocSetConfigurationCrossLinkFetcher creating a fresh AmazonS3Client
on every FetchCrossLinks() call (every live-reload cycle) by reusing the base
class reader via the new protected LinkIndexProvider property.

Adds --no-watch-md to assembler serve to skip checkout directory watchers
when doing frontend work, and adds static asset live reload (DEBUG mode)
that watches src/Elastic.Documentation.Site/_static/ and triggers browser
reload on CSS/JS changes.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 73157c68-74e6-4617-bcd1-f45da76b1d40

📥 Commits

Reviewing files that changed from the base of the PR and between 1b81498 and ab20d03.

📒 Files selected for processing (2)
  • src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs
  • src/tooling/docs-builder/Http/AssemblerServeWebHost.cs
✅ Files skipped from review due to trivial changes (1)
  • src/tooling/docs-builder/Http/AssemblerServeWebHost.cs

📝 Walkthrough

Walkthrough

Adds an on‑demand assembler web host and a file‑watching reload service. Introduces AssemblerServeWebHost to resolve request prefixes to documentation sets and generators, render markdown (with optional LiveReload injection), and serve static assets. Adds AssemblerReloadService to watch checkout markdown and _static files, debounce events, call DocumentationSet.InvalidateResolved, and trigger live reload messages. Improves disposal/lifetimes: Aws3LinkIndexReader implements IDisposable; CrossLinkFetcher accepts an ownership flag and exposes LinkIndexProvider; ReloadableGeneratorState disposes the cross‑link fetcher. CLI/build: new Watch_Full and serve-static command and adjusted serve command signature.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant WebHost as AssemblerServeWebHost
    participant Router
    participant Builder as AssemblerBuilder
    participant Generator as DocumentationGenerator
    participant Set as DocumentationSet
    participant Renderer

    Client->>WebHost: GET /{**slug}
    WebHost->>Router: Resolve slug -> (prefix -> set, generator)
    Router-->>WebHost: Match entry
    WebHost->>Builder: CreateGenerator(set)
    Builder-->>Generator: Return generator
    WebHost->>Set: ResolveDirectoryTree()
    Set-->>WebHost: Directory resolved / content path
    WebHost->>Generator: Render markdown file
    Generator->>Renderer: RenderLayout(markdown)
    Renderer-->>WebHost: HTML (LiveReload may be injected)
    WebHost-->>Client: 200 HTML
Loading
sequenceDiagram
    participant FS as FileSystemWatcher
    participant ReloadSvc as AssemblerReloadService
    participant Debouncer
    participant Set as DocumentationSet
    participant WS as LiveReloadWebSocket

    FS->>ReloadSvc: File changed (.md or _static)
    ReloadSvc->>Debouncer: Enqueue event
    Note over Debouncer: debounce window
    Debouncer->>Set: InvalidateResolved()  (for markdown)
    Set-->>Debouncer: _resolved cleared
    Debouncer->>WS: Send reload message
    WS-->>Client: Browser reload
Loading

Suggested labels

fix, ci

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the primary change: an on-demand assembler serve command with live reload, which is reflected throughout the changeset.
Description check ✅ Passed The description comprehensively details the changes, the problem being solved, the new features, bug fixes, and includes a workflow and test plan aligned with the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch feature/assembler-serve

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/Elastic.Markdown/IO/DocumentationSet.cs (1)

216-228: ⚠️ Potential issue | 🟡 Minor

Potential race between InvalidateResolved and ResolveDirectoryTree.

If a file-watcher thread calls InvalidateResolved() while ResolveDirectoryTree is mid-flight on another thread, the in-progress run will still set _resolved = true at the end, even though the post-invalidation state was not observed. Subsequent calls will then short-circuit and serve stale resolution until the next change. For live-reload this is usually self-healing, but consider guarding with a version counter (compare-and-swap) or a lock if you see flaky reloads under rapid edits.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Elastic.Markdown/IO/DocumentationSet.cs` around lines 216 - 228, There is
a race between InvalidateResolved() and ResolveDirectoryTree():
InvalidateResolved just flips _resolved=false while ResolveDirectoryTree sets
_resolved=true at the end, so an invalidation during a run can be lost; fix by
introducing a version or synchronization around resolution (e.g., add a
long/_version counter incremented in InvalidateResolved and captured at start of
ResolveDirectoryTree, then only set _resolved=true (or update the version) if
the captured version still matches at completion, or protect the whole
ResolveDirectoryTree/InvalidateResolved pair with a lock); update references in
ResolveDirectoryTree to check the captured version before setting _resolved and
ensure MinimalParseAsync calls still use TryFindDocumentByRelativePath and
MarkdownFiles as before.
src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs (1)

22-22: ⚠️ Potential issue | 🟡 Minor

_codexReader disposal is inconsistent with the base reader.

The base class now conditionally disposes linkIndexProvider in Dispose(), but this subclass's _codexReader (which may be GitLinkIndexReader/Aws3LinkIndexReader, both IDisposable) is never released. Either override Dispose here to forward disposal when the fetcher owns the codex reader, or document the ownership contract so callers know they must keep/dispose it. Today the only known caller (ReloadableGeneratorState) holds _codexReader as a field and never disposes it either, so the SemaphoreSlim inside GitLinkIndexReader leaks.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs`
at line 22, The subclass stores a potentially disposable _codexReader (e.g.,
GitLinkIndexReader/Aws3LinkIndexReader) but never releases it while the base
class conditionally disposes linkIndexProvider; update
DocSetConfigurationCrossLinkFetcher to make ownership explicit and properly
dispose the reader: add an ownership flag (or document ownership) and override
Dispose() in this class to call Dispose() on _codexReader when owned (check
_codexReader is IDisposable), mirroring the base class behavior so resources
like SemaphoreSlim inside GitLinkIndexReader are released; alternatively,
document that callers (e.g., ReloadableGeneratorState) must retain and dispose
the _codexReader if the fetcher does not own it.
♻️ Duplicate comments (1)
src/services/Elastic.Documentation.Assembler/AssembleSources.cs (1)

49-51: ⚠️ Potential issue | 🔴 Critical

Downstream of the CrossLinkFetcher.Dispose issue.

This using scope is where the pre-existing logFactory.Dispose() inside CrossLinkFetcher.Dispose() becomes observable — after line 51 the logger created from logFactory is still used (line 53, 69) and logFactory itself is forwarded into the AssembleSources constructor (line 56) and then into each AssemblerDocumentationSet. See the root-cause comment on src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs for the fix; once that's addressed this block is correct.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/Elastic.Documentation.Assembler/AssembleSources.cs` around lines
49 - 51, The issue is that AssemblerCrossLinkFetcher.Dispose currently disposes
the shared logFactory, causing the logger (created from logFactory and later
used in AssembleSources and passed into AssembleSources constructor and each
AssemblerDocumentationSet) to be disposed; fix by changing
AssemblerCrossLinkFetcher.Dispose to not call logFactory.Dispose or otherwise
take ownership of the ILoggerFactory passed in (i.e., do not dispose the
injected logFactory), ensure Dispose only cleans up resources created by
AssemblerCrossLinkFetcher itself, and keep the using block around
AssemblerCrossLinkFetcher and the FetchCrossLinks call (FetchedCrossLinks,
AssemblerCrossLinkFetcher.FetchCrossLinks) as-is once the Dispose change is
applied.
🧹 Nitpick comments (1)
src/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cs (1)

160-190: Extract duplicated DocumentInferrerService construction.

The same DocumentInferrerService initialization now appears in BuildAllAsync, CreateGenerator, and BuildOneAsync. A small private helper (e.g., CreateInferrer(AssemblerDocumentationSet set)) would remove the repetition and keep the three call sites in sync if the constructor grows.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cs`
around lines 160 - 190, The DocumentInferrerService construction is duplicated
across CreateGenerator, BuildOneAsync, and BuildAllAsync; add a private helper
method (e.g., CreateInferrer(AssemblerDocumentationSet set)) that takes the
AssemblerDocumentationSet and returns a new DocumentInferrerService configured
with context.ProductsConfiguration, context.VersionsConfiguration,
context.LegacyUrlMappings, set.DocumentationSet.Configuration, and
set.DocumentationSet.Context.Git, then replace the inline new
DocumentInferrerService(...) calls in CreateGenerator, BuildOneAsync, and
BuildAllAsync with calls to CreateInferrer to centralize and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs`:
- Around line 194-200: Dispose() currently disposes the injected ILoggerFactory
(logFactory), which is externally owned; remove the call to logFactory.Dispose()
from CrossLinkFetcher.Dispose() so the fetcher only disposes its own disposable
linkIndexProvider (the conditional disposableReader.Dispose()) and still calls
GC.SuppressFinalize(this); this ensures AssemblerCrossLinkFetcher and callers
using "using (var crossLinkFetcher = ...)" do not tear down the caller's
ILoggerFactory.

In `@src/tooling/docs-builder/Http/AssemblerServeWebHost.cs`:
- Around line 42-45: The current computation of _rootRedirectUrl can resolve to
"/" when _prefixMap is empty and AssembleContext.Environment.PathPrefix is
null/empty, causing the GET / handler to redirect to itself and create a
redirect loop; update the code that computes _rootRedirectUrl (the block using
_prefixMap.OrderBy(...).Select(...).FirstOrDefault and
AssembleContext.Environment.PathPrefix) to detect when no mapping or prefix
exists and instead set a sentinel (e.g., null/empty) or a safe diagnostic path,
and update the GET / handler to check that sentinel and return a 404 or
diagnostic page rather than issuing a redirect when _rootRedirectUrl is
missing/"/". Ensure you reference and modify both the _rootRedirectUrl
initialization and the GET / request handling logic so the handler does not
redirect to "/" when no mappings are configured.

---

Outside diff comments:
In
`@src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs`:
- Line 22: The subclass stores a potentially disposable _codexReader (e.g.,
GitLinkIndexReader/Aws3LinkIndexReader) but never releases it while the base
class conditionally disposes linkIndexProvider; update
DocSetConfigurationCrossLinkFetcher to make ownership explicit and properly
dispose the reader: add an ownership flag (or document ownership) and override
Dispose() in this class to call Dispose() on _codexReader when owned (check
_codexReader is IDisposable), mirroring the base class behavior so resources
like SemaphoreSlim inside GitLinkIndexReader are released; alternatively,
document that callers (e.g., ReloadableGeneratorState) must retain and dispose
the _codexReader if the fetcher does not own it.

In `@src/Elastic.Markdown/IO/DocumentationSet.cs`:
- Around line 216-228: There is a race between InvalidateResolved() and
ResolveDirectoryTree(): InvalidateResolved just flips _resolved=false while
ResolveDirectoryTree sets _resolved=true at the end, so an invalidation during a
run can be lost; fix by introducing a version or synchronization around
resolution (e.g., add a long/_version counter incremented in InvalidateResolved
and captured at start of ResolveDirectoryTree, then only set _resolved=true (or
update the version) if the captured version still matches at completion, or
protect the whole ResolveDirectoryTree/InvalidateResolved pair with a lock);
update references in ResolveDirectoryTree to check the captured version before
setting _resolved and ensure MinimalParseAsync calls still use
TryFindDocumentByRelativePath and MarkdownFiles as before.

---

Duplicate comments:
In `@src/services/Elastic.Documentation.Assembler/AssembleSources.cs`:
- Around line 49-51: The issue is that AssemblerCrossLinkFetcher.Dispose
currently disposes the shared logFactory, causing the logger (created from
logFactory and later used in AssembleSources and passed into AssembleSources
constructor and each AssemblerDocumentationSet) to be disposed; fix by changing
AssemblerCrossLinkFetcher.Dispose to not call logFactory.Dispose or otherwise
take ownership of the ILoggerFactory passed in (i.e., do not dispose the
injected logFactory), ensure Dispose only cleans up resources created by
AssemblerCrossLinkFetcher itself, and keep the using block around
AssemblerCrossLinkFetcher and the FetchCrossLinks call (FetchedCrossLinks,
AssemblerCrossLinkFetcher.FetchCrossLinks) as-is once the Dispose change is
applied.

---

Nitpick comments:
In `@src/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cs`:
- Around line 160-190: The DocumentInferrerService construction is duplicated
across CreateGenerator, BuildOneAsync, and BuildAllAsync; add a private helper
method (e.g., CreateInferrer(AssemblerDocumentationSet set)) that takes the
AssemblerDocumentationSet and returns a new DocumentInferrerService configured
with context.ProductsConfiguration, context.VersionsConfiguration,
context.LegacyUrlMappings, set.DocumentationSet.Configuration, and
set.DocumentationSet.Context.Git, then replace the inline new
DocumentInferrerService(...) calls in CreateGenerator, BuildOneAsync, and
BuildAllAsync with calls to CreateInferrer to centralize and avoid duplication.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8b44a435-5a8a-4e3e-a61f-6b1e1f4e1fb2

📥 Commits

Reviewing files that changed from the base of the PR and between 50fdf74 and fbafc6c.

📒 Files selected for processing (11)
  • build/CommandLine.fs
  • build/Targets.fs
  • src/Elastic.Documentation.LinkIndex/LinkIndexReader.cs
  • src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs
  • src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs
  • src/Elastic.Markdown/IO/DocumentationSet.cs
  • src/services/Elastic.Documentation.Assembler/AssembleSources.cs
  • src/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cs
  • src/tooling/docs-builder/Commands/Assembler/AssemblerCommands.cs
  • src/tooling/docs-builder/Http/AssemblerReloadService.cs
  • src/tooling/docs-builder/Http/AssemblerServeWebHost.cs

Comment thread src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs
Comment thread src/tooling/docs-builder/Http/AssemblerServeWebHost.cs Outdated
- DocumentationSet: use a version counter in InvalidateResolved/ResolveDirectoryTree
  so a mid-flight invalidation is not lost; the in-progress resolve only marks
  itself done when the version it captured at start still matches
- CrossLinkFetcher.Dispose: stop disposing the injected logFactory — it is shared
  and owned by the caller, not by the fetcher; only dispose linkIndexProvider
  which is created internally (Aws3LinkIndexReader.CreateAnonymous)
- ReloadableGeneratorState.Dispose: now disposes _crossLinkFetcher so its
  internally-created Aws3LinkIndexReader/AmazonS3Client is released on shutdown
- AssemblerBuilder: extract duplicated DocumentInferrerService construction into
  a private CreateInferrer(set) helper used by BuildAllAsync, CreateGenerator,
  and BuildOneAsync

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs`:
- Around line 194-200: CrossLinkFetcher.Dispose currently unconditionally
disposes linkIndexProvider causing double-dispose and leaks; modify
CrossLinkFetcher to track ownership (add a boolean like ownsReader set by the
constructor or factory when creating an anonymous reader) and only call Dispose
on linkIndexProvider in Dispose() when ownsReader is true; update callers
(ReloadableGeneratorState, RepositoryPublishValidationService, LinkIndexService)
so that callers that pass an existing reader set ownsReader=false while
factories like Aws3LinkIndexReader.CreateAnonymous() or LinkIndexService that
create the reader set ownsReader=true, and ensure
Aws3LinkIndexReader.CreateAnonymous() returned readers are disposed when
ownsReader is true to avoid S3 client leaks.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 136accce-7070-4287-b0b3-6f1e92939b19

📥 Commits

Reviewing files that changed from the base of the PR and between fbafc6c and e83abef.

📒 Files selected for processing (4)
  • src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs
  • src/Elastic.Markdown/IO/DocumentationSet.cs
  • src/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cs
  • src/tooling/docs-builder/Http/ReloadableGeneratorState.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Elastic.Markdown/IO/DocumentationSet.cs

Comment thread src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs
@Mpdreamz Mpdreamz removed the fix label Apr 28, 2026
…ble-dispose

CrossLinkFetcher.Dispose() was unconditionally disposing linkIndexProvider, which
caused use-after-dispose when a reader was shared across multiple fetcher
instances (e.g. LinkIndexLinkChecker stores _linkIndexProvider as a field and
creates three separate LinksIndexCrossLinkFetcher calls with it).

Adds ownsReader bool to the CrossLinkFetcher primary constructor (default false).
Dispose only releases linkIndexProvider when ownsReader is true.

DocSetConfigurationCrossLinkFetcher: sets ownsReader = (linkIndexProvider is null)
so it owns the reader only when it creates one internally via CreateAnonymous().

AssembleSources.AssembleAsync: introduces a separate using var for the reader
so the caller explicitly owns and disposes it; the AssemblerCrossLinkFetcher
itself uses ownsReader = false (the default).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The CI workflow and Aspire AppHost were still calling `assembler serve`
which is now the on-demand rendering command (no pre-built output needed).
Anything that runs `assembler build` first and then starts a server to
serve that output should use `assembler serve-static` instead.

- .github/workflows/ci.yml: synthetics step uses serve-static
- aspire/AppHost.cs: AssemblerServe resource uses serve-static

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Mirrors the pattern from StaticWebHost: registers AddElasticDocsApiUsecases
and MapElasticDocsApiEndpoints under the /v1 group in DEBUG builds so that
assembler serve exposes the same search and document API endpoints as
assembler serve-static.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/tooling/docs-builder/Http/AssemblerServeWebHost.cs`:
- Around line 225-233: The current prefix check in the loop over _prefixMap uses
slug.StartsWith(prefix) which allows partial matches; modify the match to
enforce a path boundary so only exact prefix or prefix followed by '/' matches:
after confirming slug.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)
also require that either slug.Length == prefix.Length or slug[prefix.Length] ==
'/' (using the same StringComparison for the StartsWith call); update the
matching logic in the loop that produces (set, generator, relFileSlug) to apply
this extra boundary check so sibling prefixes like "docs/api" do not match
"docs/apis/...".
- Around line 67-76: The live-reload monitor is always registered with
AddAotLiveReload (setting ClientFileExtensions = ".md,.yml") even when
watchMarkdown is false, causing browser reloads without cache invalidation;
update the registration so AddAotLiveReload is only called when watchMarkdown is
true (or conditionally set ClientFileExtensions to exclude .md/.yml when
watchMarkdown is false). Locate the AddAotLiveReload call and the
AssemblerReloadService constructor usage, gate the AddAotLiveReload registration
on the watchMarkdown flag (or adjust ClientFileExtensions accordingly), and
ensure you still pass assembleSources.AssembleContext.CheckoutDirectory.FullName
and the correct sets into AssemblerReloadService when creating the hosted
service.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: a2e73fe2-6c69-46c3-a451-00e0ec1e8226

📥 Commits

Reviewing files that changed from the base of the PR and between e83abef and 1b81498.

📒 Files selected for processing (6)
  • .github/workflows/ci.yml
  • aspire/AppHost.cs
  • src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs
  • src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs
  • src/services/Elastic.Documentation.Assembler/AssembleSources.cs
  • src/tooling/docs-builder/Http/AssemblerServeWebHost.cs

Comment thread src/tooling/docs-builder/Http/AssemblerServeWebHost.cs
Comment thread src/tooling/docs-builder/Http/AssemblerServeWebHost.cs
…rveWebHost

1. _rootRedirectUrl redirect loop: change to string? and guard both the GET /
   handler and the FindRepo fallback — return 404 instead of redirecting to /
   when no prefix map entries exist and no PathPrefix is configured.

2. AddAotLiveReload ClientFileExtensions: use ".css,.js" when watchMarkdown is
   false so the live-reload config accurately reflects the active watch mode
   rather than always advertising .md/.yml.

3. FindRepo partial-prefix match bug: "docs-content" would previously match
   prefix "docs" via StartsWith. Fix requires the character immediately after the
   prefix to be '/' or the slug to equal the prefix exactly.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ossLinkFetcher

The _codexReader is injected by the caller who retains ownership. The primary
caller (ReloadableGeneratorState) already disposes it directly in its own
Dispose(), so the fetcher must not dispose it.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant