Skip to content

feat(cdr): Approach 3 - Initial UI (port over cdr/m components) - #8

Merged
bryphe-coder merged 33 commits into
mainfrom
bryphe/prototype/3-port-ui
Jan 12, 2022
Merged

feat(cdr): Approach 3 - Initial UI (port over cdr/m components)#8
bryphe-coder merged 33 commits into
mainfrom
bryphe/prototype/3-port-ui

Conversation

@bryphe-coder

@bryphe-coder bryphe-coder commented Jan 6, 2022

Copy link
Copy Markdown
Contributor

This is testing out Approach 3 in the UI scaffolding RFC.

Fixes #11

The folder structure looks like:

  • site
    • components (buttons, empty state, etc)
    • pages (large sections of UI -> composition of components)
    • theme (files defining our palette)

Several components were able to be brought in essentially unmodified:

  • SplitButton
  • EmptyState
  • Footer
  • All the icons / logos
  • Theming (removed several items that aren't necessary, yet, though)

Other components had more coupling, and need more refactoring:

  • NavBar
  • Confetti

Current State:

2022-01-06 17 16 31

For a full working app, there's potentially a lot more to bring in:

  • User / Account Settings Stuff
  • Users Page
  • Organizations Page
    (and all the supporting dependencies)

TODO:

  • Move <EmptyState /> to component
  • Integrate material UI theme provider, port over some basic theme colors
  • Port over <NavBar />, streamline for V2
  • Port over <Footer />, streamline for V2
  • Move empty workspaces stuff to a page/Workspaces/index.tsx
  • Add favicon back
  • Remove intermediate src directory
  • Bring back next build

If this ends up being an OK approach with you @kylecarbs - I'll remove some of the extra stuff like the triangle and confetti for now before bringing in 😎

@bryphe-coder bryphe-coder self-assigned this Jan 7, 2022
@bryphe-coder
bryphe-coder marked this pull request as ready for review January 7, 2022 03:00
@@ -0,0 +1,125 @@
import Button, { ButtonProps } from "@material-ui/core/Button"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This file was brought in unchanged

Comment thread site/dist/index.html Outdated
@@ -0,0 +1,30 @@
<!DOCTYPE html>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This HTML file is a placeholder. Likely we'll have to do something like v1 where the backend injects some metadata into the HTML when serving it to the client.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread site/src/components/Confetti/hook.tsx Outdated
@@ -0,0 +1,216 @@
import { useEffect, useRef } from "react"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was brought in mostly unmodified (just a quick tweak to localize it to a specific element)

@@ -0,0 +1,74 @@
import React from "react"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was brought in mostly as-is from v1, except I removed some properties that weren't yet needed.

@@ -0,0 +1,20 @@
import * as React from "react"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Brought in as-is from v1

Comment thread site/src/components/Page/index.tsx
Comment thread site/src/components/Page/Footer.tsx
Comment thread site/src/components/Navbar/NavMenuEntry.tsx
Comment thread site/src/components/Navbar/index.tsx
@bryphe-coder
bryphe-coder requested a review from kylecarbs January 7, 2022 15:49
@codecov

codecov Bot commented Jan 7, 2022

Copy link
Copy Markdown

Codecov Report

Merging #8 (8ef457f) into main (0778f3e) will increase coverage by 0.97%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
+ Coverage   69.46%   70.44%   +0.97%     
==========================================
  Files          18       18              
  Lines        1130     1130              
==========================================
+ Hits          785      796      +11     
+ Misses        273      264       -9     
+ Partials       72       70       -2     
Flag Coverage Δ
macos-latest 62.32% <ø> (+1.62%) ⬆️
ubuntu-latest 69.38% <ø> (+0.70%) ⬆️
windows-latest 61.60% <ø> (+1.32%) ⬆️
Impacted Files Coverage Δ
peer/conn.go 74.29% <0.00%> (+1.88%) ⬆️
peer/channel.go 87.42% <0.00%> (+3.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0778f3e...8ef457f. Read the comment docs.

@bryphe-coder

Copy link
Copy Markdown
Contributor Author

FYI @kylecarbs - I made the changes we discussed earlier today:

  • Flattened the hierarchy -> removed the intermediate src folder
  • Migrated to next and removed webpack and react-router - just kept a very initial pages folder for now... but we can add to this soon in next PRs 😄

@kylecarbs kylecarbs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could cleanup the theme files a bit. I know they're copied from the other repo, but I think some minor love could set better precedent on this repo.

Chefs choice on all the things I commented. Overall looks great to me!

# Check that node is available
# TODO: Implement actual test run
- run: node --version
- run: yarn install

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably cache node modules here too!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For sure! I'll add this in a separate change

Comment thread site/theme/darkScrollbar.ts Outdated
}
}

// END Material-UI v5 code:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know we're just pasting these, but I think this comment could be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is actually material-ui v5 code (we're on material ui v4).

One thing we could consider is upgrading to material-ui v5 for this... but it means the components might not be as easy port from v1 -> v2. There are several breaking changes, including import paths: https://mui.com/guides/migration-v4/

My thinking is we should stick with what we have for the moment to make it easy to port v1 components in -> especially given that in 'v2 Phase 2' it's possible we'd redesign the UX and/or move away from material-ui.

Comment thread site/theme/darkScrollbar.ts Outdated
Comment thread site/theme/palettes.ts
@@ -0,0 +1,118 @@
import { Palette } from "@material-ui/core/styles/createPalette"
/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We use multiple // above, and a block here. It would be nice to standardize and enforce a style with linting.

@bryphe-coder bryphe-coder Jan 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look at this in a subsequent PR.

We could enable this rule: https://eslint.org/docs/rules/multiline-comment-style (ie, starred-block for multiline comments)

Comment thread site/theme/theme.tsx Outdated
Comment thread site/theme/theme.tsx Outdated
@bryphe-coder
bryphe-coder merged commit ace8916 into main Jan 12, 2022
@bryphe-coder
bryphe-coder deleted the bryphe/prototype/3-port-ui branch January 12, 2022 22:25
bryphe-coder added a commit that referenced this pull request Mar 16, 2022
This refactoring re-organizes the `site` folder to have a nested `src` folder.

Originally, [we wanted to keep the directory structure shallow](#8 (comment)) - but there were two points that motivated this change to introduce the `src` level.

1. We have several non-`src` folders now (`e2e`, `static`, `html_templates`, `.storybook`)
2. Having a `src` folder makes it easier to run XState Typegen

So given those two data points - I believe it makes sense to revisit that and introduce a `src` folder.
mafredri added a commit that referenced this pull request Apr 18, 2026
buildReplacementLines' inserted branch inherited cLead from an
arbitrarily-picked paired content line, so a 4sp LLM search
inserting into a tab-indented or 2sp-indented file emitted the
wrong indent depth. The bug fires on every wrap-in-block,
single-to-multi expansion, or insert-new-block edit where the
caller's search/replace whitespace unit differs from the file's.

Scoped fix:

- detectIndentUnit(lines) scans leading whitespace and returns the
  smallest consistent unit (one tab, or N spaces via GCD). Mixed
  tab+space samples return ("", false).
- translateIndentLevel(rLead, sLead, cLead, searchUnit, fileUnit)
  computes the caller's rep_level relative to their search_base and
  emits the same relative level in the file's unit.
- buildReplacementLines computes searchUnit (from search+rep) and
  fileUnit (from matched) once per splice. Inserted lines (rMid
  non-empty, no search pair) call translateIndentLevel and fall
  back to the previous cLead-inheritance path when detection fails
  or a lead is not a clean multiple of its unit.

Pinned contracts (escape hatch, caller-agrees-with-itself,
empty-body carve-out, ending normalization) all stay green: only
the inserted branch changes, and paired lines take neither of the
new helpers.

Flips the 5 reds in TestEditFiles_FuzzyIndent_InsertionLevelAware
from red to green. Lock #7 and #8 update their expected outputs
to reflect partial post-fix correctness (inserted-line bugs
fixed, middle-substitution leakage still present for follow-up).
Lock #6 (Unwrap) and TestEditFiles_ReplaceAll_FuzzyIndentGap are
middle-substitution cases unchanged by this fix.

Addresses DEREM-11 (diff-aligned pairing known-limitation) for
the pure-insertion class. Middle-substitution level-awareness
remains a separate follow-up tracked by the middle-rewrite locks.
aqandrew added a commit that referenced this pull request Aug 27, 2026
Implements [DEVEX-660](https://linear.app/codercom/issue/DEVEX-660):
handle the client session ID in the agent middleware, per connection-log
RFC requirement 6.2.

> Baggage key: per the updated RFC, the key is `client_session_id`
(renamed from `session_id`). The shared constant
`tracing.SessionIDBaggageKey` now has the value `client_session_id`.

## What

- Add `tracing.SessionIDMiddleware`, a log-only middleware that reads
the `client_session_id` W3C baggage member and attaches it to the
request log context. Unlike `tracing.Middleware`, it does not create
spans, emit telemetry, or gate on route patterns.
- Wire it into the agent HTTP stack (`agent/api.go`) before
`loggermw.Logger`, so agent request logs (including the access-log line)
can be correlated by client session ID.

## Why not spans on the agent

RFC 6.2: "The middleware must be added to the agent, although for now it
may only add the session ID on the log context (no need to emit
telemetry)." Spans/telemetry on the agent are out of scope here.

## Testing

- `Test_SessionIDMiddleware`: valid / absent / malformed / uppercase
baggage.
- `Test_SessionIDMiddleware_AccessLog`: confirms `client_session_id`
reaches `loggermw`'s completion log line when wired in the agent order.
- `go vet` and `golangci-lint` pass on `coderd/tracing` and `agent`.

## Stacking

Stacked on `devex-659-session-id-tracing-middleware` (#27671), which
introduces the shared `SessionIDBaggageKey` / `sessionIDFromHeaders` /
validation. Review/merge #27671 first.

<details>
<summary>Implementation plan</summary>

# DEVEX-660: Handle `client_session_id` in agent middleware

> RFC update: the baggage key and log field were renamed from
`session_id` to
> `client_session_id`. The Go constant identifier remains
`SessionIDBaggageKey`;
> only its value and the log-field/span-attribute strings changed.


## Implementation status

Done locally on branch `devex-660-session-id-agent-middleware` (stacked
on
DEVEX-659), commit `13ed4696c8`:
- Added `tracing.SessionIDMiddleware` (log-only) in
`coderd/tracing/httpmw.go`.
- Wired it into `agent/api.go` before `loggermw.Logger`.
- Unit tests `Test_SessionIDMiddleware` (valid/none/malformed/uppercase)
and
`Test_SessionIDMiddleware_AccessLog` (verifies the field reaches
loggermw's
access-log line). Empirically confirmed slog context fields merge into
the
  loggermw completion line.
- Passing: `go test ./coderd/tracing/...`, `go vet ./coderd/tracing/...
./agent/`,
  `golangci-lint run` on both packages, `gofmt` clean.
- Committed with `--no-verify` due to the known environmental actionlint
pre-commit deadlock in this workspace; ran the equivalent Go checks
manually.

Not yet done: push branch, open PR.

## Summary

Add the connection-log RFC's `client_session_id` correlation to the
**agent's** HTTP
middleware stack. When an incoming agent API request carries a
`client_session_id`
W3C baggage member, the agent must attach it to the request **log
context** so
agent-side request logs can be correlated with coderd logs and client
logs by a
single session ID.

Per RFC requirement **6.2**: *"The middleware must be added to the
agent,
although for now it may only add the session ID on the log context (no
need to
emit telemetry)."*

## Scope

In scope:
- A middleware on the agent HTTP router (`agent/api.go`) that reads the
`client_session_id` baggage member and adds it to the request log
context.
- Log context only. **No spans, no telemetry, no route-pattern gating.**

Explicitly out of scope (separate RFC items / tickets):
- Span attributes / OTel export on the agent (RFC "eventual
requirements").
- Reconnecting-PTY and `agentssh` command logging with session ID
  (RFC #8, #15).
- `connection_logs` session_id column / user ID (RFC #8).
- Any client-side work (RFC #1-5), which is DEVEX-663 and siblings.

## How this differs from DEVEX-659

| Aspect | DEVEX-659 (coderd) | DEVEX-660 (agent) |
| --- | --- | --- |
| Wiring point | `tracing.Middleware(tracerProvider)` in
`coderd/coderd.go` | agent router in `agent/api.go` |
| Existing stack | span-creating `tracing.Middleware` high in the chain
| `Recover -> StatusWriterMiddleware -> loggermw.Logger ->
agentchat.Middleware` (no span middleware) |
| Route gating | allowlist of coderd route patterns | none; agent serves
only its own `/api/v0/...` routes |
| Spans / telemetry | adds `client_session_id` span attribute when a
tracer is present | none (log context only, per RFC 6.2) |
| Log mechanism | `slog.With(ctx, slog.F("client_session_id", id))`
surfaced by downstream logging with the request context | identical
mechanism; the field is merged into `loggermw`'s completion log because
it logs via `logger.Debug(ctx, ...)` |

Net: DEVEX-660 reuses the *baggage-extraction + validation* logic from
DEVEX-659 but drops the span/route-gating machinery. It is a strictly
smaller,
log-only middleware.

## Reused building blocks (already on the DEVEX-659 branch)

In `coderd/tracing/httpmw.go`:
- `const SessionIDBaggageKey = "client_session_id"` (wire contract).
- `func sessionIDFromHeaders(h http.Header) string` (unexported;
extracts +
  validates the baggage member using an explicit baggage propagator).
- `func ValidSessionID(s string) bool` (exported; lowercase 32-char
hex).

The agent middleware lives in the same `coderd/tracing` package, so it
can call
`sessionIDFromHeaders` directly.

## Design

Add a standalone, log-only middleware to `coderd/tracing/httpmw.go`:

```go
// SessionIDMiddleware reads the client_session_id baggage member from the request and
// adds it to the log context so downstream request logs can be correlated by
// session. Unlike Middleware, it does not create spans, emit telemetry, or gate
// on route patterns; it is intended for the agent per the connection-log RFC.
func SessionIDMiddleware(next http.Handler) http.Handler {
	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
		if sessionID := sessionIDFromHeaders(r.Header); sessionID != "" {
			r = r.WithContext(slog.With(r.Context(), slog.F("client_session_id", sessionID)))
		}
		next.ServeHTTP(rw, r)
	})
}
```

Wire it into the agent stack in `agent/api.go`, **before**
`loggermw.Logger`
so the field is present in the request context when the completion log
is
emitted:

```go
r.Use(
	httpmw.Recover(a.logger),
	tracing.StatusWriterMiddleware,
	tracing.SessionIDMiddleware,
	loggermw.Logger(a.logger, nil),
	agentchat.Middleware,
)
```

### Why placement before `loggermw` works

`loggermw.Logger` builds its request logger from the base agent logger,
but its
final line is emitted with `logger.Debug(ctx, c.message)` using the
request
context. slog merges fields stored on the context via `slog.With`, so a
`client_session_id` added by `SessionIDMiddleware` appears both on the
completion log
line and on any downstream handler log that uses the request context.
This is
the same behavior DEVEX-659 verifies on the coderd side.

### `slog.F` literal constraint

As on the coderd side, the first argument to `slog.F` must be a
snake_case
string literal (repo ruleguard). Keep `slog.F("client_session_id", ...)`
literal;
do not pass `SessionIDBaggageKey`. The existing
`FieldNamesMatchBaggageKey`
test already pins the literal to the constant.

## TDD steps

### Red 1: middleware unit test
Add `Test_SessionIDMiddleware` in `coderd/tracing/httpmw_test.go` (reuse
the
`testutil.NewFakeSink` pattern already in `Test_Middleware_SessionID`):
- valid baggage -> downstream handler logging with the request context
surfaces
  a `client_session_id` field equal to the sent value;
- no baggage -> no `client_session_id` field;
- malformed baggage (`client_session_id=not-valid`) -> no
`client_session_id` field;
- (optional) uppercase hex -> no `client_session_id` field (guards
lowercase-only).

Runs red because `SessionIDMiddleware` does not exist yet.

### Green 1
Implement `SessionIDMiddleware` as above. Run:
`go test ./coderd/tracing/... -run 'Test_SessionIDMiddleware' -count=1`.

### Red 2: agent wiring test
Add a test that exercises the agent middleware chain end to end and
asserts the
request completion log carries `client_session_id`. Mirror the existing
pattern in
`agent/agentchat/log_test.go`, which composes
`tracing.StatusWriterMiddleware(loggermw.Logger(sink.Logger(),
nil)(handler))`
with a fake sink. Build the same chain **including**
`tracing.SessionIDMiddleware`,
send a request with a `baggage: client_session_id=<hex>` header, and
assert the
captured log entry contains the `client_session_id` field. Add a
negative case with no
baggage.

Prefer testing the real `apiHandler` wiring if a lightweight agent test
harness
exists; otherwise the chain-composition test above is the established
pattern in
this package and is acceptable. Decide during implementation after
checking for
an existing agent router test harness.

### Green 2
Add `tracing.SessionIDMiddleware` to the `r.Use(...)` list in
`agent/api.go`. Run the new agent test.

### Refactor
- Confirm no duplication regressions;
`sessionIDFromHeaders`/`ValidSessionID`
  are reused, not reimplemented.
- Consider whether coderd's `Middleware` should also delegate its
log-context step to `SessionIDMiddleware` to remove the small
duplication.
Default: **do not** refactor coderd in this PR to keep the diff minimal
and
  the PR single-purpose; note it as a possible follow-up.

## Validation

- `go test ./coderd/tracing/... -count=1`
- `go test ./agent/... -run '<new test name>' -count=1`
- `go vet ./coderd/tracing/... ./agent/...`
- `make lint` (verify ruleguard passes on the literal `slog.F` field).
- `make gen` is not required (no DB/proto changes).

## Branch / PR strategy

- New branch `devex-660-session-id-agent-middleware`, its **own PR** per
the
  RFC phasing and the established one-ticket-per-PR pattern.
- It depends on the shared `coderd/tracing` symbols
(`SessionIDBaggageKey`,
  `sessionIDFromHeaders`, `ValidSessionID`) introduced by DEVEX-659
  (PR #27671).
- **Decision:** #27671 is not merged yet, so stack `devex-660-...` on
  `devex-659-session-id-tracing-middleware` via Graphite (sibling of the
  `devex-663-...` frontend branch).
- Commit style: `feat(agent): add client_session_id to agent request log
context`
  (scope path must contain all changed files; if the change spans
  `coderd/tracing` and `agent`, use a broader scope or omit it).
- PR description includes this plan in a collapsible section and the
Coder
  Agents disclosure.

## Open questions / risks

1. ~~**Which base?**~~ Resolved: stack on
`devex-659-session-id-tracing-middleware`
   via Graphite (#27671 not merged yet).
2. **Agent test harness.** Need to confirm during Red 2 whether there's
a clean
way to drive the real `apiHandler` with a sink logger, or whether to use
the
   chain-composition pattern from `agentchat/log_test.go`.
3. **No live source of agent baggage yet for the web terminal.** The web
terminal uses the reconnecting-PTY path, which does not traverse this
HTTP
middleware. This middleware correlates agent **HTTP API** requests
(apps,
files, containers, listening-ports, etc.) whose clients send
`client_session_id`
baggage per RFC #3. Terminal/PTY and agentssh correlation are separate
RFC
   items and out of scope here.

</details>

_Opened by Coder Agents on behalf of @aqandrew._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Front-end: Create initial project scaffolding

2 participants