feat(workspace): version history timeline backed by kernel events#593
Open
v-rdyy wants to merge 3 commits into
Open
feat(workspace): version history timeline backed by kernel events#593v-rdyy wants to merge 3 commits into
v-rdyy wants to merge 3 commits into
Conversation
The workspace root menu's Version history action was a disabled stub, and kernel_events rows were only consumed for realtime cache sync. This turns the stub into a paginated, inspection-only history dialog. - WorkspaceKernelEventBus.listEventsPage: additive, read-only reverse paging over kernel_events (revision cursor, limit clamped to 100, lookahead row so the cursor is only advertised when a next page exists); malformed payload rows degrade to a generic record. - getWorkspaceHistoryPageFn server function, gated by the same assertCanReadWorkspace check as workspace page loads. - Pure event-to-timeline mapping in model/workspace-history.ts with burst grouping for consecutive content edits; unknown event types render as a generic change row. - WorkspaceVersionHistoryDialog with infinite query, actor names from the members query, and loading/empty/error states. Inspection-only v1: event payloads carry post-change summaries only, so previous content cannot be reconstructed from the log. getPage() and realtime sync are untouched; history loads only on demand. Closes ThinkEx-OSS#566 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds workspace version history: kernel event pagination, history entry mapping, server-side history fetching, and a dialog opened from the workspace root menu to browse paginated history. ChangesWorkspace Version History
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceContextBar
participant WorkspaceVersionHistoryDialog
participant getWorkspaceHistoryPageFn
participant WorkspaceKernel
User->>WorkspaceContextBar: select "Version history"
WorkspaceContextBar->>WorkspaceVersionHistoryDialog: open dialog(workspaceId)
WorkspaceVersionHistoryDialog->>getWorkspaceHistoryPageFn: fetch history page(beforeRevision, limit)
getWorkspaceHistoryPageFn->>WorkspaceKernel: listEventsPage(args)
WorkspaceKernel-->>getWorkspaceHistoryPageFn: WorkspaceHistoryPage
getWorkspaceHistoryPageFn-->>WorkspaceVersionHistoryDialog: WorkspaceHistoryPage
WorkspaceVersionHistoryDialog-->>User: render timeline entries
User->>WorkspaceVersionHistoryDialog: click "Load more"
WorkspaceVersionHistoryDialog->>getWorkspaceHistoryPageFn: fetch next page(nextBeforeRevision)
getWorkspaceHistoryPageFn-->>WorkspaceVersionHistoryDialog: additional WorkspaceHistoryPage
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Greptile SummaryThis PR adds an inspection-only version history view for workspaces. The main changes are:
Confidence Score: 5/5The changed flow looks safe to merge after a small cleanup to actor-name fallback behavior. History reads use the existing workspace read authorization path; revision pagination uses a strict cursor and avoids duplicate pages; malformed event payloads degrade without breaking the history view. src/features/workspaces/components/WorkspaceVersionHistoryDialog.tsx can show misleading actor labels when member lookup fails.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (1): Last reviewed commit: "feat(workspace): version history timelin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+53
to
+58
| const membersQuery = useQuery({ | ||
| ...getWorkspaceMembersQueryOptions(workspaceId), | ||
| enabled: open, | ||
| }); | ||
| const actorNamesById = new Map( | ||
| (membersQuery.data ?? []).map((member) => [member.userId, member.name]), |
Contributor
There was a problem hiding this comment.
When the members request fails but the history request succeeds, membersQuery.data becomes an empty list and every non-null actor is shown as "Former member". A transient member API error can make the audit timeline misidentify current users while the dialog otherwise appears loaded.
Author
There was a problem hiding this comment.
Fixed in 7586565 — actor names now stay neutral ("Someone") until the member list has actually loaded; "Former member" is only shown once member data is present and the id is genuinely absent.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/features/workspaces/components/WorkspaceVersionHistoryDialog.tsx (1)
57-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
entriesandactorNamesById.Both are recomputed on every render even though
WorkspaceVersionHistoryDialogis unconditionally mounted alongside other context-bar dialogs (search/share/settings), so unrelated state changes in the parent re-trigger the burst-grouping pass andMapconstruction here.♻️ Proposed fix
- const actorNamesById = new Map( - (membersQuery.data ?? []).map((member) => [member.userId, member.name]), - ); - const entries = mapWorkspaceHistoryEvents( - (historyQuery.data?.pages ?? []).flatMap((page) => page.events), - ); + const actorNamesById = useMemo( + () => new Map((membersQuery.data ?? []).map((member) => [member.userId, member.name])), + [membersQuery.data], + ); + const entries = useMemo( + () => mapWorkspaceHistoryEvents((historyQuery.data?.pages ?? []).flatMap((page) => page.events)), + [historyQuery.data], + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/workspaces/components/WorkspaceVersionHistoryDialog.tsx` around lines 57 - 62, Memoize both actorNamesById and entries in WorkspaceVersionHistoryDialog so they are only recomputed when their inputs change. Wrap the Map creation and the mapWorkspaceHistoryEvents result in useMemo, keyed off membersQuery.data and historyQuery.data?.pages respectively, to avoid repeating the burst-grouping pass and Map construction on unrelated parent re-renders.src/features/workspaces/kernel/workspace-kernel-events.ts (1)
84-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent error swallowing in the malformed-payload fallback.
The catch block degrades any failure from
mapKernelEventRowto a null-payload event, which is correct per the PR's stated goal of tolerating malformed/unknown payloads. However, it swallows the error entirely with no logging, so genuine bugs inparseWorkspaceEventPayload(unrelated to malformed input) would be indistinguishable from expected degradation and go unnoticed in production.purgeForDeletioninworkspace-kernel.tsalready usesconsole.warnfor a similar graceful-degradation case — consider following that pattern here for consistency.♻️ Suggested logging addition
- } catch { + } catch (error) { // Malformed payloads must never break the history view; realtime sync // keeps its strict parsing. + console.warn("[WorkspaceKernelEventBus] Failed to parse history event payload", { + id: row.id, + revision: row.revision, + type: row.type, + error, + }); return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/workspaces/kernel/workspace-kernel-events.ts` around lines 84 - 104, The fallback in mapHistoryRow currently hides every failure from mapKernelEventRow by returning a null-payload event, which makes real parsing bugs invisible. Add a warning log inside the catch in workspace-kernel-events.ts, following the same graceful-degradation style used by purgeForDeletion in workspace-kernel.ts, and include enough context (for example the row id/type and the caught error) so malformed input is still tolerated but unexpected failures are visible. Keep the null-payload fallback behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/features/workspaces/components/WorkspaceVersionHistoryDialog.tsx`:
- Around line 57-62: Memoize both actorNamesById and entries in
WorkspaceVersionHistoryDialog so they are only recomputed when their inputs
change. Wrap the Map creation and the mapWorkspaceHistoryEvents result in
useMemo, keyed off membersQuery.data and historyQuery.data?.pages respectively,
to avoid repeating the burst-grouping pass and Map construction on unrelated
parent re-renders.
In `@src/features/workspaces/kernel/workspace-kernel-events.ts`:
- Around line 84-104: The fallback in mapHistoryRow currently hides every
failure from mapKernelEventRow by returning a null-payload event, which makes
real parsing bugs invisible. Add a warning log inside the catch in
workspace-kernel-events.ts, following the same graceful-degradation style used
by purgeForDeletion in workspace-kernel.ts, and include enough context (for
example the row id/type and the caught error) so malformed input is still
tolerated but unexpected failures are visible. Keep the null-payload fallback
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9eb21fa4-819f-43f4-b41e-0b0fa8f0357c
📒 Files selected for processing (11)
src/features/workspaces/components/WorkspaceContextBar.tsxsrc/features/workspaces/components/WorkspaceVersionHistoryDialog.tsxsrc/features/workspaces/kernel/workspace-kernel-access.tssrc/features/workspaces/kernel/workspace-kernel-events.test.tssrc/features/workspaces/kernel/workspace-kernel-events.tssrc/features/workspaces/kernel/workspace-kernel-types.tssrc/features/workspaces/kernel/workspace-kernel.tssrc/features/workspaces/model/workspace-history.test.tssrc/features/workspaces/model/workspace-history.tssrc/features/workspaces/server/functions.tssrc/features/workspaces/server/queries.ts
- Keep actor labels neutral ("Someone") until the member list has
loaded so a slow or failed members query cannot mislabel current
members as "Former member" in the timeline.
- Memoize timeline entries and the actor-name map; the dialog is
mounted alongside the context bar, so unrelated parent re-renders
were repeating the burst-grouping pass.
- Log a warning when a history event payload fails to map, matching
the kernel's existing graceful-degradation pattern, instead of
swallowing the error silently.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/workspaces/model/workspace-history.test.ts">
<violation number="1" location="src/features/workspaces/model/workspace-history.test.ts:9">
P3: The `nextRevision` counter at module scope (line 9) is decremented across every test that calls `historyEvent` without an explicit revision. This creates shared mutable state between tests — a classic testing smell that can produce order-dependent results. While none of the current assertions rely on the generated revision, future test writers may not realize that default revision values shift as tests are added, reordered, or shuffled. A simple `beforeEach(() => { nextRevision = 1000; })` or a local counter scoped to each test would keep the suite cleanly isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| type WorkspaceHistoryEvent, | ||
| } from "#/features/workspaces/model/workspace-history"; | ||
|
|
||
| let nextRevision = 1000; |
Contributor
There was a problem hiding this comment.
P3: The nextRevision counter at module scope (line 9) is decremented across every test that calls historyEvent without an explicit revision. This creates shared mutable state between tests — a classic testing smell that can produce order-dependent results. While none of the current assertions rely on the generated revision, future test writers may not realize that default revision values shift as tests are added, reordered, or shuffled. A simple beforeEach(() => { nextRevision = 1000; }) or a local counter scoped to each test would keep the suite cleanly isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/workspaces/model/workspace-history.test.ts, line 9:
<comment>The `nextRevision` counter at module scope (line 9) is decremented across every test that calls `historyEvent` without an explicit revision. This creates shared mutable state between tests — a classic testing smell that can produce order-dependent results. While none of the current assertions rely on the generated revision, future test writers may not realize that default revision values shift as tests are added, reordered, or shuffled. A simple `beforeEach(() => { nextRevision = 1000; })` or a local counter scoped to each test would keep the suite cleanly isolated.</comment>
<file context>
@@ -0,0 +1,238 @@
+ type WorkspaceHistoryEvent,
+} from "#/features/workspaces/model/workspace-history";
+
+let nextRevision = 1000;
+
+function historyEvent(input: {
</file context>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
The workspace root menu's
Version historyaction was a disabled stub (trailing: "Soon"inWorkspaceContextBar.tsx), andkernel_eventsrows — which already record every mutation with a monotonic revision, actor, and payload — were only ever consumed for realtime cache sync viagetEventsSince(). Nothing read the log backwards for humans.Approach and the inspection-only scoping decision
This v1 is inspection-only: no restore, no snapshots, no diffs. Event payloads are written after the mutation (
commitItemEventreads the row post-update), so they carry only post-change summaries — no previous name on renames, andcontent.updatedhas no content snapshot at all. Past state cannot be reconstructed from the log, so restore is out of scope by design, and the dialog says so in its description ("History is inspection-only for now — restoring earlier versions isn't supported yet"). Copy consequence: renames read "renamed to X", never "renamed from Y to X".The implementation is one additive read path:
WorkspaceKernelEventBus.listEventsPage({ beforeRevision?, limit })pageskernel_eventsbyORDER BY revision DESCwithWHERE revision < cursor, limit clamped to 1–100 (default 50), fetching one lookahead row sonextBeforeRevisionis only advertised when a next page actually exists. Exposed onWorkspaceKernelnext togetEventsSince, which is untouched. The event log is strictly read-only for this feature.getWorkspaceHistoryPageFn(GET) →getWorkspaceHistoryPageForCurrentUser→getWorkspaceKernelHistoryPage, gated by the sameassertCanReadWorkspacecheck thatgetWorkspacePageFnuses. Input validated with zod (beforeRevisionpositive int,limit1–100).mapWorkspaceHistoryEventsinmodel/workspace-history.tsturns raw events into readable entries and coalesces consecutivecontent.updatedbursts for the same item by the same actor within 5 minutes into one "edited “X” (N changes)" entry. Grouping happens client-side; the API returns raw events.WorkspaceVersionHistoryDialogopened from the (now enabled) menu action, TanStack infinite query keyed on the revision cursor, actor names resolved from the existing members query, relative timestamps viaformatWorkspaceRecency, loading/empty/error states, and a Load more button. No route changes; nothing fetches until the dialog opens, so baseline workspace load is unchanged (getPage()untouched).Graceful degradation is layered: a malformed
payload_jsonrow falls back to a generic record insidelistEventsPage(the sharedmapKernelEventRowstays strict for realtime), and the view model routes unknown event types or unexpected payload shapes to a generic "made a change" row — so future event types can never break the dialog.Decisions and rejected alternatives
itemIdfilter parameter yet. The issue floats per-item history later; I rejected shipping a dead parameter that doesn't filter (a trap for callers). Adding an optionalitemIdto the schema later is backward-compatible.hasMorecount queries — one query per page, and no trailing empty page when the total is an exact multiple of the page size.itemIds, notdeletedItemIds— "deleted 2 items" matches what the user selected; descendants are an implementation detail of recursive delete.createThreadWorkspaceAccessContextpassesthread.userId), so AI edits are indistinguishable from manual edits in the log today — faking a badge would be wrong. This answers the issue's open question; distinguishing them needs an actor-kind column or aclientMutationIdconvention (follow-up).Fact corrections vs the task prompt
mapKernelEventRowdoes not already degrade gracefully — its payload parse is a bareJSON.parsethat throws on malformed rows. It is reused as directed, but wrapped per-row inlistEventsPageso one bad row can't kill a history page.content.updatedwithactorUserId: null(DocumentSession.checkpointToKernel), so most document-edit history is unattributed. The UI renders these as "Someone"; null actors still group into bursts. (AI edits, by contrast, carry the requesting user's id.)Follow-ups noticed (not in this PR)
restoreDeletedItemskernel command leveraging the existingdeleted_atsoft delete, which requires no schema change. Any snapshot/retention schema should be designed before that implementation, per the issue.kernel_eventsso AI changes can be visually distinguished.parseWorkspaceEventPayloadinworkspace-kernel-rows.tscan throw on a corrupt row in the realtimegetEventsSincepath too; left untouched here since changing realtime semantics is out of scope.How verified
pnpm checkclean (format, lint, types);pnpm testclean — 30 tests, 7 files, including the 17 new ones.pnpm buildpasses end-to-end (including prerender), run withCLOUDFLARE_VITE_FORCE_LOCAL=trueand Docker per the recent CI setup.Closes #566
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit