feat: add a "git blame" esque hover over for Markdown text#3961
feat: add a "git blame" esque hover over for Markdown text#3961404Wolf wants to merge 7 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a hover-tooltip feature that displays per-node edit blame information in the Lexical markdown editor. The frontend adds a Lexical plugin that tracks pointer position and hovered node identity, renders a tooltip showing user identity and edit timestamp when the cursor is still over a node. The backend adds a database table and HTTP endpoint to serve blame queries, updates the document state import to track which Lexical nodes were modified by each update, and integrates blame recording into the websocket peer update handler using background tasks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
404Wolf
force-pushed
the
wolf/git-blame-hover
branch
from
5c79fe7 to
b1b49c8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
js/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/HoverTooltip.tsx (2)
102-111: 💤 Low valuePrefer explicit
!= nullcheck over truthiness for userId.The
whencondition usesblame()?.userIdas a truthy check, which filters outnullbut also empty strings. A more explicitblame()?.userId != nullwould clarify intent and avoid accidentally filtering valid empty-string user IDs (unlikely but possible). This would also eliminate the need for the non-null assertion!on line 111.📝 Suggested refinement
- <Show when={visible() && blame()?.userId ? blame() : null}> + <Show when={visible() && blame()?.userId != null ? blame() : null}> {(b) => ( <div class="fixed z-50 text-xs text-ink-secondary/70 pointer-events-none" style={{ left: `${props.state.x + 12}px`, top: `${props.state.y + 12}px`, }} > - <UserLine userId={b().userId!} editedAt={b().editedAt} /> + <UserLine userId={b().userId} editedAt={b().editedAt} /> </div> )} </Show>🤖 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 `@js/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/HoverTooltip.tsx` around lines 102 - 111, Change the truthy check in the Show conditional to explicitly test for null/undefined: use blame()?.userId != null instead of blame()?.userId so you don't exclude valid empty-string IDs; update the inner usage (UserLine userId={b().userId!}) to remove the non-null assertion (pass b().userId directly) and keep the rest of the tooltip rendering (visible(), blame(), Show, UserLine, props.state.x/props.state.y) unchanged.
68-97: Consider alternatives tocreateEffectfor state updates.The coding guidelines state: "Avoid createEffect in SolidJs. Only use for syncing with external/imperative systems (DOM APIs, third-party libs). Do not use to derive state or trigger updates—use a derived signal or wrap the setter instead."
These effects manage imperative timing (debouncing,
setTimeout/clearTimeout), which is a legitimate imperative system interaction. However, they also trigger state updates (setArmedNodeId,setVisible). Consider whether this logic could be refactored to wrap the setters or use a different pattern that aligns more closely with the project's SolidJS conventions. Based on coding guidelines, effects should primarily sync with external systems rather than derive internal state.🤖 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 `@js/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/HoverTooltip.tsx` around lines 68 - 97, These createEffect blocks (the ones reading props.state.hovering/x/y and calling debouncedArm, setArmedNodeId, setVisible, clearing/setting showTimer and comparing shownAtX/shownAtY) are being used to derive internal state; instead refactor to avoid createEffect by moving the imperative timing into explicit setter wrappers or a small controller API: wrap updates to props.state.hovering so the wrapper calls debouncedArm(nodeId) or debouncedArm.clear() and setArmedNodeId accordingly (use the existing debouncedArm and setArmedNodeId), and create a single updatePointer(hovering, x, y) function that handles the showTimer logic (clearing/setting timeouts, comparing shownAtX/shownAtY, calling setVisible(true)/hide()) instead of relying on createEffect; keep the same variables (showTimer, shownAtX, shownAtY, visible) but drive them from these wrappers/controller and invoke updatePointer from the same places that currently update props.state (so you still interact imperatively with DOM/events but do not derive state inside createEffect).Source: Coding guidelines
js/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/hoverTooltipPlugin.ts (1)
57-57: ⚡ Quick winType annotation should be PointerEvent, not MouseEvent.
The handler is registered for the
'pointermove'event (lines 93, 98), which emitsPointerEventinstances. WhilePointerEventextendsMouseEventand the code works at runtime, the type annotation should be precise.📝 Suggested fix
- const handlePointerMove = (e: MouseEvent) => { + const handlePointerMove = (e: PointerEvent) => {🤖 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 `@js/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/hoverTooltipPlugin.ts` at line 57, The pointer move handler handlePointerMove is typed as MouseEvent but is registered for 'pointermove' (emitting PointerEvent); update the type annotation for the handlePointerMove parameter from MouseEvent to PointerEvent so the signature matches the event source and TypeScript uses the precise event type when handling pointer-specific properties.rust/sync-service/src/d1.rs (1)
111-117: ⚡ Quick winHoist the D1 database acquisition outside the loop.
Acquiring a new D1 database handle on each iteration (line 112) is inefficient. Obtain the handle once before the loop and reuse it for all
upsert_blamecalls.♻️ Proposed refactor
tracing::info!( document_id = document_id, peer_id = peer_id, count = node_ids.len(), node_ids = ?node_ids, "record_blame" ); + let db = env.d1(crate::constants::USER_PEER_D1_BINDING)?; for node_id in node_ids { - let db = env.d1(crate::constants::USER_PEER_D1_BINDING)?; if let Err(e) = upsert_blame(db, document_id, node_id, peer_id, now_ms).await {Note: You may need to adjust the signature of
upsert_blameto accept&D1Databaseor clone the handle, depending on the workers-rs API.🤖 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 `@rust/sync-service/src/d1.rs` around lines 111 - 117, Move the call to env.d1(crate::constants::USER_PEER_D1_BINDING)? out of the node_ids loop and acquire the D1 database handle once before iterating; reuse that handle for every upsert_blame call instead of calling env.d1 each iteration. Update the upsert_blame invocation (and if necessary its signature) to accept the shared handle (e.g., &D1Database or a cloned handle per the workers-rs API) so you can call upsert_blame(db, document_id, node_id, peer_id, now_ms). Preserve the existing error logging and return behavior on Err as currently done in the loop.rust/sync-service/src/state.rs (1)
103-116: ⚡ Quick winLog diff failures instead of silently returning empty.
When
self.loro_doc.diff(before, after)fails (line 106), the method silently returns an empty vector. A diff failure indicates incompatible frontiers or a corrupted document state—serious issues that should at least be logged as a warning before returning empty.📝 Proposed improvement
fn touched_lexical_ids(&self, before: &Frontiers, after: &Frontiers) -> Vec<String> { let Ok(diff) = self.loro_doc.diff(before, after) else { + tracing::warn!( + before = ?before, + after = ?after, + "diff failed between frontiers; returning empty touched_nodes" + ); return Vec::new(); };🤖 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 `@rust/sync-service/src/state.rs` around lines 103 - 116, The function touched_lexical_ids currently swallows errors from self.loro_doc.diff and returns an empty Vec, which hides real failures; update touched_lexical_ids to log the diff error before returning (e.g., use log::warn! or tracing::warn! with the error and context like "failed to diff frontiers in touched_lexical_ids") so callers still get the empty Vec but the failure is visible for debugging; locate the match/let Ok(diff) = self.loro_doc.diff(before, after) block and insert a warn log that includes the error (and any identifying info like before/after frontier IDs) in the else branch prior to returning Vec::new().
🤖 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.
Inline comments:
In `@rust/sync-service/database/user-peer-mapping/migrations/0002_add_blame.sql`:
- Around line 1-7: The migration creates the "blame" table but is not
idempotent; update the CREATE TABLE statement in 0002_add_blame.sql to be
idempotent by using CREATE TABLE IF NOT EXISTS for the blame table so re-running
the migration will not fail, keeping the same columns (document_id, node_id,
peer_id, timestamp_ms) and the PRIMARY KEY (document_id, node_id).
In `@rust/sync-service/src/durable_object.rs`:
- Around line 334-336: The blame route handler currently calls
self.blame_handler(matched.params.get("node_id")).await but blame_handler should
accept the already-validated document_id like other handlers; change the match
arm for path::BLAME to pass matched.params.get("document_id") (or the validated
document_id variable) and update blame_handler's signature to accept document_id
as a parameter (instead of reading it from DO state), and adjust any internal
uses to use that parameter; mirror the pattern used by metadata_handler and
raw_handler so callers and tests remain consistent.
---
Nitpick comments:
In
`@js/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/HoverTooltip.tsx`:
- Around line 102-111: Change the truthy check in the Show conditional to
explicitly test for null/undefined: use blame()?.userId != null instead of
blame()?.userId so you don't exclude valid empty-string IDs; update the inner
usage (UserLine userId={b().userId!}) to remove the non-null assertion (pass
b().userId directly) and keep the rest of the tooltip rendering (visible(),
blame(), Show, UserLine, props.state.x/props.state.y) unchanged.
- Around line 68-97: These createEffect blocks (the ones reading
props.state.hovering/x/y and calling debouncedArm, setArmedNodeId, setVisible,
clearing/setting showTimer and comparing shownAtX/shownAtY) are being used to
derive internal state; instead refactor to avoid createEffect by moving the
imperative timing into explicit setter wrappers or a small controller API: wrap
updates to props.state.hovering so the wrapper calls debouncedArm(nodeId) or
debouncedArm.clear() and setArmedNodeId accordingly (use the existing
debouncedArm and setArmedNodeId), and create a single updatePointer(hovering, x,
y) function that handles the showTimer logic (clearing/setting timeouts,
comparing shownAtX/shownAtY, calling setVisible(true)/hide()) instead of relying
on createEffect; keep the same variables (showTimer, shownAtX, shownAtY,
visible) but drive them from these wrappers/controller and invoke updatePointer
from the same places that currently update props.state (so you still interact
imperatively with DOM/events but do not derive state inside createEffect).
In
`@js/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/hoverTooltipPlugin.ts`:
- Line 57: The pointer move handler handlePointerMove is typed as MouseEvent but
is registered for 'pointermove' (emitting PointerEvent); update the type
annotation for the handlePointerMove parameter from MouseEvent to PointerEvent
so the signature matches the event source and TypeScript uses the precise event
type when handling pointer-specific properties.
In `@rust/sync-service/src/d1.rs`:
- Around line 111-117: Move the call to
env.d1(crate::constants::USER_PEER_D1_BINDING)? out of the node_ids loop and
acquire the D1 database handle once before iterating; reuse that handle for
every upsert_blame call instead of calling env.d1 each iteration. Update the
upsert_blame invocation (and if necessary its signature) to accept the shared
handle (e.g., &D1Database or a cloned handle per the workers-rs API) so you can
call upsert_blame(db, document_id, node_id, peer_id, now_ms). Preserve the
existing error logging and return behavior on Err as currently done in the loop.
In `@rust/sync-service/src/state.rs`:
- Around line 103-116: The function touched_lexical_ids currently swallows
errors from self.loro_doc.diff and returns an empty Vec, which hides real
failures; update touched_lexical_ids to log the diff error before returning
(e.g., use log::warn! or tracing::warn! with the error and context like "failed
to diff frontiers in touched_lexical_ids") so callers still get the empty Vec
but the failure is visible for debugging; locate the match/let Ok(diff) =
self.loro_doc.diff(before, after) block and insert a warn log that includes the
error (and any identifying info like before/after frontier IDs) in the else
branch prior to returning Vec::new().
🪄 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: Pro
Run ID: 44bfee9c-6c1b-4ffb-a8e2-07badcf93bad
📒 Files selected for processing (14)
js/app/packages/block-md/component/MarkdownEditor.tsxjs/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/HoverTooltip.tsxjs/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/hoverTooltipPlugin.tsjs/app/packages/core/component/LexicalMarkdown/plugins/hover-tooltip/index.tsjs/app/packages/core/component/LexicalMarkdown/plugins/index.tsjs/app/packages/core/component/LexicalMarkdown/plugins/pluginManager.tsjs/app/packages/service-clients/service-sync/client.tsrust/sync-service/database/user-peer-mapping/migrations/0002_add_blame.sqlrust/sync-service/src/d1.rsrust/sync-service/src/durable_object.rsrust/sync-service/src/state.rsrust/sync-service/src/storage/backends/durable_kv.rsrust/sync-service/src/storage/mod.rsrust/sync-service/src/websocket.rs
| CREATE TABLE blame ( | ||
| document_id TEXT NOT NULL, | ||
| node_id TEXT NOT NULL, | ||
| peer_id TEXT NOT NULL, | ||
| timestamp_ms INTEGER NOT NULL, | ||
| PRIMARY KEY (document_id, node_id) | ||
| ); |
There was a problem hiding this comment.
Migration is not idempotent.
The CREATE TABLE lacks IF NOT EXISTS, so re-running this migration will fail. Per the SQL coding guidelines, migrations should be idempotent where possible.
🔄 Suggested fix
-CREATE TABLE blame (
+CREATE TABLE IF NOT EXISTS blame (
document_id TEXT NOT NULL,
node_id TEXT NOT NULL,
peer_id TEXT NOT NULL,
timestamp_ms INTEGER NOT NULL,
PRIMARY KEY (document_id, node_id)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE blame ( | |
| document_id TEXT NOT NULL, | |
| node_id TEXT NOT NULL, | |
| peer_id TEXT NOT NULL, | |
| timestamp_ms INTEGER NOT NULL, | |
| PRIMARY KEY (document_id, node_id) | |
| ); | |
| CREATE TABLE IF NOT EXISTS blame ( | |
| document_id TEXT NOT NULL, | |
| node_id TEXT NOT NULL, | |
| peer_id TEXT NOT NULL, | |
| timestamp_ms INTEGER NOT NULL, | |
| PRIMARY KEY (document_id, node_id) | |
| ); |
🤖 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 `@rust/sync-service/database/user-peer-mapping/migrations/0002_add_blame.sql`
around lines 1 - 7, The migration creates the "blame" table but is not
idempotent; update the CREATE TABLE statement in 0002_add_blame.sql to be
idempotent by using CREATE TABLE IF NOT EXISTS for the blame table so re-running
the migration will not fail, keeping the same columns (document_id, node_id,
peer_id, timestamp_ms) and the PRIMARY KEY (document_id, node_id).
Source: Coding guidelines
| path::BLAME => { | ||
| return self.blame_handler(matched.params.get("node_id")).await; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Pass document_id as a parameter for consistency.
The blame_handler retrieves document_id from DO state (line 571) instead of receiving it from the URL path like other handlers. For consistency with metadata_handler (line 552), raw_handler (line 451), and others, pass the already-validated document_id from the match arm into blame_handler.
♻️ Proposed refactor
path::METADATA => return self.metadata_handler(document_id).await,
path::BLAME => {
- return self.blame_handler(matched.params.get("node_id")).await;
+ return self
+ .blame_handler(document_id, matched.params.get("node_id"))
+ .await;
}- async fn blame_handler(&self, node_id: Option<&str>) -> Result<Response> {
+ async fn blame_handler(&self, document_id: &str, node_id: Option<&str>) -> Result<Response> {
let node_id = node_id.ok_or_else(|| Error::from("missing node_id"))?;
- let document_id = self.document_id().await?.to_string();
let db = self.env.d1(USER_PEER_D1_BINDING)?;
- match crate::d1::get_blame_for_node(db, &document_id, node_id).await? {
+ match crate::d1::get_blame_for_node(db, document_id, node_id).await? {
Some(row) => ResponseBuilder::new().from_json(&row),
None => Ok(response(status_codes::NOT_FOUND)),
}Also applies to: 569-577
🤖 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 `@rust/sync-service/src/durable_object.rs` around lines 334 - 336, The blame
route handler currently calls
self.blame_handler(matched.params.get("node_id")).await but blame_handler should
accept the already-validated document_id like other handlers; change the match
arm for path::BLAME to pass matched.params.get("document_id") (or the validated
document_id variable) and update blame_handler's signature to accept document_id
as a parameter (instead of reading it from DO state), and adjust any internal
uses to use that parameter; mirror the pattern used by metadata_handler and
raw_handler so callers and tests remain consistent.
| // Record "last edited by" for each Lexical node touched. Runs | ||
| // in the background via `wait_until` so the ACK + peer broadcast | ||
| // below don't block on D1, and the write still completes even | ||
| // after we return from this handler. | ||
| if !touched_nodes.is_empty() { | ||
| let peer_ids = Wsm::new(dss, ws).get_peer_ids().await.unwrap_or_default(); | ||
| if let Some(&peer_id) = peer_ids.first() { | ||
| let env = dss.env().clone(); | ||
| let document_id = document_id.to_string(); | ||
| dss.wait_until(async move { | ||
| if let Err(e) = | ||
| crate::d1::record_blame(&env, &document_id, peer_id, &touched_nodes) | ||
| .await | ||
| { | ||
| tracing::warn!( | ||
| error = ?e, | ||
| document_id = document_id, | ||
| peer_id = peer_id, | ||
| "record_blame failed" | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm a little concerned about the overhead of doing this on every single message, even with the wait_until. Perhaps instead we let them accumulate then flush during the alarm ?
2 participants
Uh oh!
There was an error while loading. Please reload this page.